From 1e68c0d8bc847507397f720c292f75e965307aaa Mon Sep 17 00:00:00 2001 From: vincent Date: Wed, 2 Sep 2026 19:50:19 -0400 Subject: [PATCH 1/3] mpg: route connect and proxy through the public Machines API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fly mpg connect and fly mpg proxy (v1 and v2) now try the public Machines API first — GetManagedPostgresCluster for cluster lookup, GetManagedPostgresUserCredentials for connect's credentials — falling back to the legacy client only on a classified 404. This completes the migration pattern already used by status, attach, detach, and destroy. Depends on flyctl#5048, which decouples proxy from any credential/status dependency; this diff deliberately adds no status gate to proxy for the same reason. connect's cluster-status handling is rewritten around the real public status enum (ready, standby_ready, creating, deleting, failed, deleted, initializing), traced from source across mpg, ui-ex, and nomad-firecracker. The prior "error" status check was based on a false premise — it is not a real public cluster status — and is removed. Every non-ready status now refuses with a specific message before any credential call; unrecognized values fail closed. The legacy fallback path keeps its original pre-migration behavior unchanged (credentials.Status check, original WARN text), since legacy's status vocabulary differs from the public API's and applying the new classifier there would have been a regression. Default user/database resolve to the fixed constants fly-user/fly-db, which are hardcoded server-side (mpg orchestrator's default-user and default-database provisioning steps), not derived per-cluster. Co-Authored-By: Claude Sonnet 5 --- internal/command/mpg/v1/run_connect.go | 50 +- internal/command/mpg/v1/run_connect_test.go | 121 ++++ internal/command/mpg/v1/run_proxy.go | 309 ++++++++++- internal/command/mpg/v1/run_proxy_test.go | 578 +++++++++++++++++++- internal/command/mpg/v2/run_connect.go | 50 +- internal/command/mpg/v2/run_connect_test.go | 121 ++++ internal/command/mpg/v2/run_proxy.go | 309 ++++++++++- internal/command/mpg/v2/run_proxy_test.go | 578 +++++++++++++++++++- 8 files changed, 1998 insertions(+), 118 deletions(-) create mode 100644 internal/command/mpg/v1/run_connect_test.go create mode 100644 internal/command/mpg/v2/run_connect_test.go diff --git a/internal/command/mpg/v1/run_connect.go b/internal/command/mpg/v1/run_connect.go index 8f10e81262..d90351b4a6 100644 --- a/internal/command/mpg/v1/run_connect.go +++ b/internal/command/mpg/v1/run_connect.go @@ -3,6 +3,7 @@ package cmdv1 import ( "context" "fmt" + "io" "os" "os/exec" "os/signal" @@ -77,14 +78,13 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string) ( } } - cluster, params, credentials, err := GetMpgConnectParams(ctx, localProxyPort, username, clusterID, resolvedOrgSlug) + cluster, useLegacy, params, credentials, err := GetMpgConnectParams(ctx, localProxyPort, username, clusterID, resolvedOrgSlug) if err != nil { return err } - if cluster.Status != "ready" { - fmt.Fprintf(io.ErrOut, "%s Cluster is not in ready state, currently: %s\n", aurora.Yellow("WARN"), cluster.Status) - } + // Gated on useLegacy; see maybeWarnLegacyNotReady's doc comment for why. + maybeWarnLegacyNotReady(io.ErrOut, useLegacy, cluster) psqlPath, err := exec.LookPath("psql") if err != nil { @@ -103,15 +103,7 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string) ( return err } - user := credentials.User - password := credentials.Password - - // Use selected database or fall back to default from credentials - if db == "" { - db = credentials.DBName - } - - connectUrl := fmt.Sprintf("postgresql://%s:%s@localhost:%s/%s", user, password, localProxyPort, db) + connectUrl := buildConnectURL(credentials, db, localProxyPort) // Allow Ctrl+C signals to hit psql psqlCtx, psqlCancel := context.WithCancel(context.WithoutCancel(ctx)) @@ -174,3 +166,35 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string) ( return err } + +// buildConnectURL composes the psql connection URL from resolved credentials +// and the proxy port. db follows the priority used by RunConnect: an explicit +// --database value (or interactive prompt result) wins over credentials.DBName. +// credentials.DBName is the plan-required default ("fly-db") on both the +// public default-user and public explicit-user paths, so a non-interactive +// `fly mpg connect --user alice` lands on postgresql://.../fly-db. +func buildConnectURL(credentials *mpgv1.GetManagedClusterCredentialsResponse, db string, localProxyPort string) string { + if db == "" { + db = credentials.DBName + } + + return fmt.Sprintf("postgresql://%s:%s@localhost:%s/%s", credentials.User, credentials.Password, localProxyPort, db) +} + +// maybeWarnLegacyNotReady restores the pre-migration legacy-path "Cluster +// is not in ready state" stderr warning. It is gated on useLegacy so the +// public path (which already refuses non-ready clusters via +// connectStatusRefusal) stays silent — see connectStatusRefusal's doc +// comment for the legacy/public status-split rationale. The warning +// format is preserved verbatim from the pre-migration code +// (commit 81f75427b^): aurora.Yellow("WARN") + " Cluster is not in ready +// state, currently: \n". The function is a thin side-effecting +// helper so it can be unit-tested with a bytes.Buffer without involving +// the agent/establish code path or RunConnect's exec/psql machinery. +func maybeWarnLegacyNotReady(errOut io.Writer, useLegacy bool, cluster *mpgv1.ManagedCluster) { + if !useLegacy || cluster == nil || cluster.Status == "ready" { + return + } + + fmt.Fprintf(errOut, "%s Cluster is not in ready state, currently: %s\n", aurora.Yellow("WARN"), cluster.Status) +} diff --git a/internal/command/mpg/v1/run_connect_test.go b/internal/command/mpg/v1/run_connect_test.go new file mode 100644 index 0000000000..c996e7eb3f --- /dev/null +++ b/internal/command/mpg/v1/run_connect_test.go @@ -0,0 +1,121 @@ +package cmdv1 + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/require" + mpgv1 "github.com/superfly/flyctl/internal/uiex/mpg/v1" +) + +// TestBuildConnectURLPublicExplicitUserDefaultsToFlyDB pins the public +// explicit-user connect path so it can never regress to the bad +// postgresql://.../ helper output. Credentials are resolved the same way the +// public branch of resolveConnectCredentials does, then handed straight to +// buildConnectURL the same way RunConnect does. +func TestBuildConnectURLPublicExplicitUserDefaultsToFlyDB(t *testing.T) { + creds := &mpgv1.GetManagedClusterCredentialsResponse{ + User: "alice", + Password: "a", + DBName: "fly-db", // mirrors the fixed public explicit-user branch. + } + + got := buildConnectURL(creds, "", "16380") + require.Equal(t, "postgresql://alice:a@localhost:16380/fly-db", got) +} + +// TestBuildConnectURLRespectsExplicitDatabase verifies the priority order: +// an explicit --database value wins over credentials.DBName. +func TestBuildConnectURLRespectsExplicitDatabase(t *testing.T) { + creds := &mpgv1.GetManagedClusterCredentialsResponse{ + User: "alice", + Password: "a", + DBName: "fly-db", + } + + got := buildConnectURL(creds, "app-db", "16380") + require.Equal(t, "postgresql://alice:a@localhost:16380/app-db", got) +} + +// TestBuildConnectURLEmptyDBNameFallsThrough is a guard for the historical +// bug shape: if the explicit-user public branch ever returns "" again, the +// URL will not silently land on postgresql://.../ (empty path). It will land +// on postgresql://.../ with the user-controlled --database fallback in +// RunConnect still preferring the flag value when present; without that +// fallback the URL here is malformed, which surfaces in tests rather than +// silently connecting to the wrong database. +func TestBuildConnectURLEmptyDBNameFallsThrough(t *testing.T) { + creds := &mpgv1.GetManagedClusterCredentialsResponse{ + User: "alice", + Password: "a", + DBName: "", + } + + got := buildConnectURL(creds, "", "16380") + require.Equal(t, "postgresql://alice:a@localhost:16380/", got) +} + +// TestMaybeWarnLegacyNotReady pins the pre-migration warning format and +// the useLegacy/status gate. See maybeWarnLegacyNotReady's doc comment +// for the gate rationale and the warning-text provenance. +func TestMaybeWarnLegacyNotReady(t *testing.T) { + const name = "test-cluster" + tests := []struct { + name string + useLegacy bool + status string + wantWarn bool + }{ + // Public path (useLegacy == false). connectStatusRefusal refuses + // non-ready public-path clusters upstream, so the useLegacy + // gate is the only thing that matters here. + {name: "public+creating silent", useLegacy: false, status: "creating", wantWarn: false}, + + // Legacy path (useLegacy == true). "ready" is silent (the + // status gate short-circuits before the fprintf); "creating" + // is the pre-migration non-refused case. The legacy-only + // "error" status is real on the legacy path but rejected by + // the public classifier — pinned here so a future migration + // does not silently drop it. + {name: "legacy+ready silent", useLegacy: true, status: "ready", wantWarn: false}, + {name: "legacy+creating warns", useLegacy: true, status: "creating", wantWarn: true}, + {name: "legacy+error warns", useLegacy: true, status: "error", wantWarn: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + cluster := &mpgv1.ManagedCluster{Name: name, Status: tt.status} + maybeWarnLegacyNotReady(&buf, tt.useLegacy, cluster) + + got := buf.String() + if !tt.wantWarn { + require.Empty(t, got, "no warning expected for useLegacy=%v status=%q", tt.useLegacy, tt.status) + + return + } + // Robust match against the pre-migration rendered text. We + // assert on "WARN" (the literal aurora payload when not a + // TTY) and on the exact "currently: " interpolation, + // rather than asserting the aurora-wrapped string verbatim, + // so this stays green regardless of the test harness's color + // configuration. + require.Contains(t, got, "WARN", "warning must contain the literal 'WARN' marker") + require.Contains(t, got, "Cluster is not in ready state, currently: "+tt.status) + require.True(t, bytes.HasSuffix(buf.Bytes(), []byte("\n")), "warning must end with a newline (pre-migration format)") + }) + } +} + +// TestMaybeWarnLegacyNotReadyNilCluster pins the defensive nil-cluster +// short-circuit. RunConnect only calls the helper with the cluster +// returned by GetMpgConnectParams, which is never nil on the success +// path, but the helper is public and the nil guard prevents a panic if +// the contract ever loosens. +func TestMaybeWarnLegacyNotReadyNilCluster(t *testing.T) { + var buf bytes.Buffer + require.NotPanics(t, func() { + maybeWarnLegacyNotReady(&buf, true, nil) + }) + require.Empty(t, buf.String()) +} diff --git a/internal/command/mpg/v1/run_proxy.go b/internal/command/mpg/v1/run_proxy.go index a82b04a159..44d28c5451 100644 --- a/internal/command/mpg/v1/run_proxy.go +++ b/internal/command/mpg/v1/run_proxy.go @@ -2,15 +2,30 @@ package cmdv1 import ( "context" + "errors" "fmt" + "strconv" + fly "github.com/superfly/fly-go" + "github.com/superfly/fly-go/flaps" "github.com/superfly/flyctl/agent" "github.com/superfly/flyctl/internal/flag" + "github.com/superfly/flyctl/internal/flapsutil" "github.com/superfly/flyctl/internal/flyutil" + "github.com/superfly/flyctl/internal/uiex/mpg" mpgv1 "github.com/superfly/flyctl/internal/uiex/mpg/v1" "github.com/superfly/flyctl/proxy" ) +// defaultMPGUser is the literal username used when resolving the default +// fly-user on the public Machines API for Connect. Verified against the +// orchestrator and ui-ex repos. +const defaultMPGUser = "fly-user" + +// defaultMPGDatabase is the literal database name used when no --database +// flag is given and no interactive prompt answer is available. +const defaultMPGDatabase = "fly-db" + func RunProxy(ctx context.Context, clusterID string, resolvedOrgSlug string, proxyPort string) error { _, params, err := GetMpgProxyParams(ctx, proxyPort, clusterID, resolvedOrgSlug) if err != nil { @@ -29,12 +44,12 @@ func GetMpgProxyParams( clusterID string, resolvedOrgSlug string, ) (*mpgv1.ManagedCluster, *proxy.ConnectParams, error) { - response, err := getCluster(ctx, clusterID) + response, _, port, err := getCluster(ctx, clusterID) if err != nil { return nil, nil, err } - cluster, params, err := buildProxyParams(ctx, response, localProxyPort, resolvedOrgSlug) + cluster, params, err := buildProxyParams(ctx, response, port, localProxyPort, resolvedOrgSlug) if err != nil { return nil, nil, err } @@ -44,48 +59,143 @@ func GetMpgProxyParams( // GetMpgConnectParams builds proxy connection parameters and resolves the // database credentials needed by fly mpg connect. +// +// The returned useLegacy bool mirrors the value computed inside getCluster +// (true when the public Machines API returned a classified 404 and we fell +// back to the legacy ui-ex client; false when the public API succeeded). +// RunConnect uses it to gate the pre-migration "Cluster is not in ready +// state" stderr warning, which is meaningful only on the legacy path — +// see connectStatusRefusal's doc comment for why the public path does not +// need it. func GetMpgConnectParams( ctx context.Context, localProxyPort string, username string, clusterID string, resolvedOrgSlug string, -) (*mpgv1.ManagedCluster, *proxy.ConnectParams, *mpgv1.GetManagedClusterCredentialsResponse, error) { - response, err := getCluster(ctx, clusterID) +) (*mpgv1.ManagedCluster, bool, *proxy.ConnectParams, *mpgv1.GetManagedClusterCredentialsResponse, error) { + response, useLegacy, port, err := getCluster(ctx, clusterID) if err != nil { - return nil, nil, nil, err + return nil, false, nil, nil, err } - credentials, err := resolveConnectCredentials(ctx, response, username) + credentials, err := resolveConnectCredentials(ctx, response, useLegacy, username) if err != nil { - return nil, nil, nil, err + return nil, false, nil, nil, err } - cluster, params, err := buildProxyParams(ctx, response, localProxyPort, resolvedOrgSlug) + cluster, params, err := buildProxyParams(ctx, response, port, localProxyPort, resolvedOrgSlug) if err != nil { - return nil, nil, nil, err + return nil, false, nil, nil, err } - return cluster, params, credentials, nil + return cluster, useLegacy, params, credentials, nil } -func getCluster(ctx context.Context, clusterID string) (*mpgv1.GetManagedClusterResponse, error) { - mpgClient := mpgv1.ClientFromContext(ctx) - response, err := mpgClient.GetManagedClusterById(ctx, clusterID) +// getCluster retrieves cluster details via the public Machines API, falling +// back to the legacy MPGv1 client only when the public call returns a +// classified 404. Any other non-nil public error is returned immediately, +// wrapped in "failed retrieving cluster %s: %w". The returned useLegacy flag +// indicates which client +// produced the response so downstream credential resolution can choose the +// appropriate code path. The returned port is the public API's advertised +// direct-endpoint port (nil on the legacy path, which has no port field) — +// carried as a separate return value rather than a field on the legacy +// mpgv1.ManagedCluster type, since that type is part of the private/legacy +// client package (internal/uiex/mpg) that this migration must not modify. +func getCluster(ctx context.Context, clusterID string) (*mpgv1.GetManagedClusterResponse, bool, *int, error) { + flapsClient := flapsutil.ClientFromContext(ctx) + publicCluster, err := flapsClient.GetManagedPostgresCluster(ctx, clusterID) + if err == nil { + response, port := publicToLegacyClusterResponse(publicCluster) + return &response, false, port, nil + } + + if !errors.Is(err, flaps.ErrFlapsNotFound) { + return nil, false, nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) + } + + legacyClient := mpgv1.ClientFromContext(ctx) + response, err := legacyClient.GetManagedClusterById(ctx, clusterID) if err != nil { - return nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) + return nil, true, nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) } - return &response, nil + return &response, true, nil, nil +} + +// publicToLegacyClusterResponse wraps the public Machines API cluster in the +// legacy ui-ex envelope so that downstream callers (proxyParams, +// resolveConnectCredentials) can read IpAssignments.Direct and Status without +// translation. The legacy shape preserves the bare-address column for +// proxyParams.RemoteHost and the Status field for connectStatusRefusal. +// +// The public API's advertised Endpoints.Primary.Direct.Port is returned +// separately (not as a field on the legacy mpgv1.ManagedCluster type) so +// proxyParams can dial the real advertised port instead of the legacy +// hardcoded 5432, without adding a public-API-only field to the private +// legacy client package. A nil port return means "no port info" (the legacy +// path never calls this function at all, so in practice this only happens +// if ever called with a zero-value input); a non-nil pointer — even to 0 — +// means the public path returned some value, and *0 is treated as invalid +// by proxyParams. +func publicToLegacyClusterResponse(c flaps.ManagedPostgresCluster) (mpgv1.GetManagedClusterResponse, *int) { + port := c.Endpoints.Primary.Direct.Port + + return mpgv1.GetManagedClusterResponse{ + Data: mpgv1.ManagedCluster{ + Id: c.ID, + Name: c.Name, + Status: c.Status, + Region: c.Region, + Plan: c.Plan, + Disk: c.DiskSizeGB, + Replicas: c.Replicas, + Organization: fly.Organization{Name: c.Organization.Name, Slug: c.Organization.Slug}, + IpAssignments: mpg.ManagedClusterIpAssignments{Direct: c.Endpoints.Primary.Direct.Host}, + }, + }, &port } +// resolveConnectCredentials returns the credentials used by RunConnect. +// useLegacy=true routes through the legacy ui-ex client; useLegacy=false +// routes through the public Machines API's +// GetManagedPostgresUserCredentials. The default user (no --user flag) +// resolves to defaultMPGUser ("fly-user"); explicit users pass the flag +// value straight through. The public path has no envelope-level DBName, +// so both public branches fall back to defaultMPGDatabase ("fly-db") — +// which run_connect.go's buildConnectURL uses to land on the plan-required +// default when neither --database nor an interactive prompt supplies one. +// +// Status gating is split across the two paths because their vocabularies +// differ; see connectStatusRefusal's doc comment for the full rationale. +// Briefly: the public path consults response.Data.Status BEFORE any +// credentials call (a 7-value enum + fail-closed default); the legacy +// path keeps the pre-migration post-fetch credentials.Status + +// credentials.Password check ("error" is a real legacy value but not a +// public one, and the legacy envelope's Status field is semantically +// distinct from cluster status). func resolveConnectCredentials( ctx context.Context, response *mpgv1.GetManagedClusterResponse, + useLegacy bool, username string, ) (*mpgv1.GetManagedClusterCredentialsResponse, error) { + // Status classifier runs BEFORE any credentials call on the public + // path only — see connectStatusRefusal's doc comment for the + // legacy/public split. response.Data.Status is populated on the public + // path via publicToLegacyClusterResponse (from c.Status); it is + // intentionally NOT consulted on the legacy path. + if !useLegacy { + if err := connectStatusRefusal(response.Data.Name, response.Data.Status); err != nil { + return nil, err + } + } + var credentials mpgv1.GetManagedClusterCredentialsResponse - if username != "" { + + switch { + case username != "" && useLegacy: mpgClient := mpgv1.ClientFromContext(ctx) userCreds, err := mpgClient.GetUserCredentials(ctx, response.Data.Id, username) if err != nil { @@ -97,32 +207,159 @@ func resolveConnectCredentials( Password: userCreds.Data.Password, DBName: response.Credentials.DBName, } - } else { + case username != "": + flapsClient := flapsutil.ClientFromContext(ctx) + userCreds, err := flapsClient.GetManagedPostgresUserCredentials(ctx, response.Data.Id, username) + if err != nil { + return nil, fmt.Errorf("failed retrieving credentials for user %s: %w", username, err) + } + + credentials = mpgv1.GetManagedClusterCredentialsResponse{ + User: userCreds.Username, + Password: userCreds.Password, + DBName: defaultMPGDatabase, + } + case useLegacy: credentials = response.Credentials - } + default: + flapsClient := flapsutil.ClientFromContext(ctx) + userCreds, err := flapsClient.GetManagedPostgresUserCredentials(ctx, response.Data.Id, defaultMPGUser) + if err != nil { + return nil, fmt.Errorf("failed retrieving credentials for user %s: %w", defaultMPGUser, err) + } - if username == "" { - if credentials.Status == "initializing" { - return nil, fmt.Errorf("cluster is still initializing, wait a bit more") + credentials = mpgv1.GetManagedClusterCredentialsResponse{ + User: userCreds.Username, + Password: userCreds.Password, + DBName: defaultMPGDatabase, } + } - if credentials.Status == "error" || credentials.Password == "" { - return nil, fmt.Errorf("error getting cluster password") + if useLegacy { + // ORIGINAL pre-migration legacy status/password checks, + // restored verbatim. credentials.Status is only checked on the + // default-user path because the explicit-user legacy + // GetUserCredentials response does not populate that field + // (only User/Password/DBName). See connectStatusRefusal's doc + // comment for why this path is intentionally distinct from the + // public classifier. + if username == "" { + if credentials.Status == "initializing" { + return nil, fmt.Errorf("cluster is still initializing, wait a bit more") + } + + if credentials.Status == "error" || credentials.Password == "" { + return nil, fmt.Errorf("error getting cluster password") + } + } else if credentials.Password == "" { + return nil, fmt.Errorf("error getting user password") } } else if credentials.Password == "" { + // Public path: empty-password fallback after the credentials + // call. The wording tracks whether the user was explicit + // (flag/prompt) or defaulted to the plan-required fly-user, + // preserving the existing pre-fix messages. The public path + // has no envelope-level Status to inspect post-fetch, so the + // pre-credential connectStatusRefusal call above is the only + // status gate. + if username == "" { + return nil, fmt.Errorf("error getting cluster password") + } + return nil, fmt.Errorf("error getting user password") } return &credentials, nil } +// connectStatusRefusal is the explicit status classifier for fly mpg +// connect. It returns nil when the cluster's status permits a connect +// attempt and a deliberate, status-specific refusal error otherwise. The +// complete emitted set of "status" values from a successful public MPG +// cluster lookup (traced across the mpg / nomad-firecracker / ui-ex code +// paths this session) is: +// +// ready, standby_ready, creating, deleting, failed, deleted, initializing +// +// "error" is NOT a real public cluster status value; it does not appear +// in the public enum and is rejected if encountered. Behavior per status: +// +// - ready: proceed, no message. +// - standby_ready: refuse — a standby replica is not a connect target. +// Fail-closed pending a product decision on standby +// semantics; not even a "warn and continue" — refused +// outright. +// - creating: refuse — friendly wording distinct from initializing +// since the cluster is in the process of coming up, +// not stuck mid-provision. +// - deleting: refuse — cluster is going away, connections +// meaningless. +// - deleted: refuse — terminal state, no connect possible. +// - failed: refuse — accurate wording ("cluster is in a failed +// state"), not the misleading "error getting cluster +// password" misdiagnosis. +// - initializing: refuse — neutral wording because this now covers +// many underlying states (degraded, updating, +// resizing, credential rotation, promotion, and +// future unmapped states), not just fresh +// provisioning. Do not over-promise "wait a bit more" +// for a state that may not be purely transient. +// - default: refuse — unknown / unrecognized statuses fail +// closed with the actual status string quoted so the +// user (and on-call) can see what the public API +// actually returned. Do NOT let unknown values fall +// through to credential resolution. +// +// The classifier applies ONLY to the public path (useLegacy == false), +// BEFORE any public credential call, on both the default-user and +// explicit-user paths. It is intentionally NOT applied to the legacy +// path (useLegacy == true): the legacy status vocabulary differs from +// the public API's — e.g. "error" is a real legacy cluster status value +// but not a public one — and the pre-migration legacy code understood it +// natively via credentials.Status (the legacy credentials envelope's own +// status field, populated post-fetch and semantically distinct from +// response.Data.Status). Applying this public-only classifier to legacy +// responses would mis-handle legacy-only statuses as "unrecognized +// state" and silently drop the credentials.Status check. +// +// The classifier is NOT applied to fly mpg proxy — that command +// deliberately does not gate on status because the raw TCP proxy never +// uses credentials, only the direct IP/port; see RunProxy / +// GetMpgProxyParams. Adding a status gate there would reintroduce the +// exact coupling flyctl#5048 was built to remove. +// +// name is the cluster identifier included in the refusal message so the +// user knows which cluster the refusal applies to. It comes from +// response.Data.Name (populated on both the public and legacy paths). +func connectStatusRefusal(name, status string) error { + switch status { + case "ready": + return nil + case "standby_ready": + return fmt.Errorf("cluster %s is a standby replica and cannot be used with fly mpg connect", name) + case "creating": + return fmt.Errorf("cluster %s is still being created, wait a bit more", name) + case "deleting": + return fmt.Errorf("cluster %s is being deleted and cannot be connected to", name) + case "deleted": + return fmt.Errorf("cluster %s has been deleted and cannot be connected to", name) + case "failed": + return fmt.Errorf("cluster %s is in a failed state", name) + case "initializing": + return fmt.Errorf("cluster %s is not currently ready for connections (status: initializing)", name) + default: + return fmt.Errorf("cluster %s is in an unrecognized state (%q) and cannot be connected to", name, status) + } +} + func buildProxyParams( ctx context.Context, response *mpgv1.GetManagedClusterResponse, + port *int, localProxyPort string, resolvedOrgSlug string, ) (*mpgv1.ManagedCluster, *proxy.ConnectParams, error) { - cluster, params, err := proxyParams(response, localProxyPort, resolvedOrgSlug, flag.GetBindAddr(ctx), nil) + cluster, params, err := proxyParams(response, port, localProxyPort, resolvedOrgSlug, flag.GetBindAddr(ctx), nil) if err != nil { return nil, nil, err } @@ -147,6 +384,7 @@ func buildProxyParams( func proxyParams( response *mpgv1.GetManagedClusterResponse, + port *int, localProxyPort string, resolvedOrgSlug string, bindAddr string, @@ -157,8 +395,29 @@ func proxyParams( return nil, nil, fmt.Errorf("error getting cluster IP") } + // remotePort is the port the proxy will dial on the remote host. The + // legacy path has no port field at all, so it always calls this with + // port == nil and we fall back to "5432" exactly as before. The public + // path passes the advertised Endpoints.Primary.Direct.Port through + // getCluster so a non-5432 public port dials that real port instead of + // being silently rewritten to 5432. A public API that advertises port 0 + // is treated as an invalid/unexpected value and surfaces as an error — + // distinguishing it from the legacy "no port field" case, where 5432 is + // the historical default and the safe fallback. The *int rather than a + // separate bool encodes the "optional value" semantic directly: nil = + // no port info (legacy), non-nil (even *port == 0) = public path + // advertised some value. + remotePort := "5432" + if port != nil { + if *port == 0 { + return nil, nil, fmt.Errorf("error getting cluster port") + } + + remotePort = strconv.Itoa(*port) + } + return cluster, &proxy.ConnectParams{ - Ports: []string{localProxyPort, "5432"}, + Ports: []string{localProxyPort, remotePort}, OrganizationSlug: resolvedOrgSlug, Dialer: dialer, BindAddr: bindAddr, diff --git a/internal/command/mpg/v1/run_proxy_test.go b/internal/command/mpg/v1/run_proxy_test.go index 884d0daf9d..5067f29c62 100644 --- a/internal/command/mpg/v1/run_proxy_test.go +++ b/internal/command/mpg/v1/run_proxy_test.go @@ -8,6 +8,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + fly "github.com/superfly/fly-go" + "github.com/superfly/fly-go/flaps" + "github.com/superfly/flyctl/internal/flapsutil" "github.com/superfly/flyctl/internal/mock" "github.com/superfly/flyctl/internal/uiex/mpg" mpgv1 "github.com/superfly/flyctl/internal/uiex/mpg/v1" @@ -53,7 +56,7 @@ func TestProxyParamsIgnoreCredentials(t *testing.T) { } dialer := &testDialer{} - cluster, params, err := proxyParams(&response, "15432", "test-org", "127.0.0.2", dialer) + cluster, params, err := proxyParams(&response, nil, "15432", "test-org", "127.0.0.2", dialer) require.NoError(t, err) assert.Same(t, &response.Data, cluster) assert.Equal(t, []string{"15432", "5432"}, params.Ports) @@ -66,42 +69,285 @@ func TestProxyParamsIgnoreCredentials(t *testing.T) { } func TestProxyParamsRequireDirectIP(t *testing.T) { - cluster, params, err := proxyParams(&mpgv1.GetManagedClusterResponse{}, "15432", "test-org", "127.0.0.1", nil) + cluster, params, err := proxyParams(&mpgv1.GetManagedClusterResponse{}, nil, "15432", "test-org", "127.0.0.1", nil) require.EqualError(t, err, "error getting cluster IP") assert.Nil(t, cluster) assert.Nil(t, params) } +// samplePublicCluster is a representative public Machines API cluster payload. +func samplePublicCluster() flaps.ManagedPostgresCluster { + return flaps.ManagedPostgresCluster{ + ID: "mpg-123", + Name: "test-cluster", + Status: "ready", + Region: "ord", + Plan: "development", + DiskSizeGB: 10, + Replicas: 1, + Organization: flaps.ManagedPostgresOrganization{Name: "Test Org", Slug: "test-org"}, + Endpoints: flaps.ManagedPostgresEndpoints{ + Primary: struct { + Direct flaps.ManagedPostgresEndpoint `json:"direct"` + Pooler flaps.ManagedPostgresEndpoint `json:"pooler"` + }{ + Direct: flaps.ManagedPostgresEndpoint{Host: "10.0.0.1", Port: 5432}, + Pooler: flaps.ManagedPostgresEndpoint{Host: "10.0.0.2", Port: 6432}, + }, + }, + } +} + +// sampleLegacyCluster is a representative legacy MPGv1 cluster payload. +// Direct is a bare address (no port) — the historical shape of this column. +func sampleLegacyCluster() mpgv1.GetManagedClusterResponse { + return mpgv1.GetManagedClusterResponse{ + Data: mpgv1.ManagedCluster{ + Id: "mpg-123", Name: "test-cluster", Region: "ord", Status: "ready", + Plan: "development", Disk: 10, Replicas: 1, + Organization: fly.Organization{Name: "Test Org", Slug: "test-org"}, + IpAssignments: mpg.ManagedClusterIpAssignments{Direct: "10.0.0.1"}, + }, + Credentials: mpgv1.GetManagedClusterCredentialsResponse{ + Status: "ready", User: "app", Password: "secret", + DBName: "app", ConnectionUri: "postgres://app:secret@10.0.0.1:5432/app", + }, + } +} + +func TestGetCluster(t *testing.T) { + tests := []struct { + name string + publicCluster flaps.ManagedPostgresCluster + publicErr error + legacyResponse mpgv1.GetManagedClusterResponse + legacyErr error + wantUseLegacy bool + wantDirectHost string + wantStatus string + wantErr string + wantLegacyCalls int + }{ + { + name: "public success maps to legacy shape", + publicCluster: samplePublicCluster(), + wantUseLegacy: false, + wantDirectHost: "10.0.0.1", + wantStatus: "ready", + wantLegacyCalls: 0, + }, + { + name: "public success with empty host preserves empty", + publicCluster: func() flaps.ManagedPostgresCluster { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Host = "" + + return c + }(), + wantUseLegacy: false, + wantDirectHost: "", + wantStatus: "ready", + wantLegacyCalls: 0, + }, + { + name: "classified 404 falls back to legacy", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: flaps.ErrFlapsNotFound, + legacyResponse: sampleLegacyCluster(), + wantUseLegacy: true, + wantDirectHost: "10.0.0.1", + wantStatus: "ready", + wantLegacyCalls: 1, + }, + { + name: "classified 404 with legacy failure propagates legacy error", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: flaps.ErrFlapsNotFound, + legacyResponse: mpgv1.GetManagedClusterResponse{}, + legacyErr: errors.New("legacy denied"), + wantErr: "failed retrieving cluster mpg-123: legacy denied", + wantLegacyCalls: 1, + }, + { + name: "non-404 public error returns without fallback", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: errors.New("boom"), + wantErr: "failed retrieving cluster mpg-123: boom", + wantLegacyCalls: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + publicCalls, legacyCalls := 0, 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresClusterFunc: func(_ context.Context, id string) (flaps.ManagedPostgresCluster, error) { + publicCalls++ + require.Equal(t, "mpg-123", id) + + return tt.publicCluster, tt.publicErr + }, + }) + ctx = mpgv1.NewContextWithClient(ctx, &mock.MpgV1Client{ + GetManagedClusterByIdFunc: func(_ context.Context, id string) (mpgv1.GetManagedClusterResponse, error) { + legacyCalls++ + require.Equal(t, "mpg-123", id) + + return tt.legacyResponse, tt.legacyErr + }, + }) + + got, useLegacy, _, err := getCluster(ctx, "mpg-123") + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + require.Nil(t, got) + require.Equal(t, 1, publicCalls) + require.Equal(t, tt.wantLegacyCalls, legacyCalls) + + return + } + require.NoError(t, err) + require.Equal(t, tt.wantUseLegacy, useLegacy) + require.NotNil(t, got) + require.Equal(t, tt.wantDirectHost, got.Data.IpAssignments.Direct) + require.Equal(t, tt.wantStatus, got.Data.Status) + require.Equal(t, 1, publicCalls) + require.Equal(t, tt.wantLegacyCalls, legacyCalls) + }) + } +} + +// TestProxyParamsPublicNeverTouchesCredentials verifies the invariant that +// the proxy code path never resolves credentials: getCluster must only hit +// the cluster lookup endpoint, not the credentials endpoint, on the public- +// success branch. This pins RunProxy against accidentally growing a +// credentials dependency. +func TestProxyParamsPublicNeverTouchesCredentials(t *testing.T) { + credCalls, legacyCredCalls := 0, 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { + return samplePublicCluster(), nil + }, + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + + return flaps.ManagedPostgresUserCredentials{}, nil + }, + }) + ctx = mpgv1.NewContextWithClient(ctx, &mock.MpgV1Client{ + GetUserCredentialsFunc: func(context.Context, string, string) (mpgv1.GetUserCredentialsResponse, error) { + legacyCredCalls++ + + return mpgv1.GetUserCredentialsResponse{}, nil + }, + }) + + response, useLegacy, port, err := getCluster(ctx, "mpg-123") + require.NoError(t, err) + require.NotNil(t, response) + require.False(t, useLegacy) + require.Equal(t, 0, credCalls, "getCluster (public-success path) must never resolve credentials") + require.Equal(t, 0, legacyCredCalls, "getCluster must never reach the legacy client on public success") + + // proxyParams does not take a ctx, so it cannot make any HTTP call; it is + // structurally incapable of leaking credentials. Verify it produces the + // expected bare-host RemoteHost from the public-converted response. The + // credCalls / legacyCredCalls counters were already asserted to be 0 + // above, immediately after getCluster returned, so proxyParams has + // nothing to regress there. + cluster, params, err := proxyParams(response, port, "15432", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.NotNil(t, cluster) + require.NotNil(t, params) + require.Equal(t, "10.0.0.1", params.RemoteHost) +} + func TestResolveDefaultConnectCredentials(t *testing.T) { + // The legacy default-user connect path (useLegacy == true) restores + // the pre-migration post-fetch logic: the legacy credentials + // envelope's own Status field (credentials.Status, semantically + // distinct from cluster status) is checked for "initializing"/"error", + // plus an empty-password fallback. These cases pin that behavior + // with the original error messages. The public status classifier is + // intentionally NOT applied here; that is covered by + // TestResolveConnectCredentialsPublicStatusClassifier below. See + // connectStatusRefusal's doc comment for the legacy/public status- + // split rationale. + const name = "test-cluster" tests := []struct { - name string - credentials mpgv1.GetManagedClusterCredentialsResponse - err string + name string + clusterSt string // response.Data.Status — cluster status. NOT consulted on the legacy path. + credStatus string // credentials.Status — the legacy envelope's own status field (post-fetch). + credPwd string // credentials.Password — empty-password fallback when status checks pass. + err string }{ { - name: "initializing", - credentials: mpgv1.GetManagedClusterCredentialsResponse{Status: "initializing"}, - err: "cluster is still initializing, wait a bit more", + // ORIGINAL legacy: empty password is the empty-password + // fallback; status field is irrelevant when password is empty. + name: "empty password", + clusterSt: "ready", + credStatus: "ready", + credPwd: "", + err: "error getting cluster password", + }, + { + // REGRESSION GUARD: Data.Status="ready" with credentials + // .Status="initializing" and a non-empty password MUST refuse + // on the legacy path. Applying the public-only 7-value + // status classifier to the legacy path (where it does not + // belong) would silently drop the credentials.Status check + // and let this case proceed to psql with possibly-invalid + // credentials. This is the regression this test pins. + name: "ready cluster with stale initializing credentials refuses", + clusterSt: "ready", + credStatus: "initializing", + credPwd: "secret", + err: "cluster is still initializing, wait a bit more", }, { - name: "error status", - credentials: mpgv1.GetManagedClusterCredentialsResponse{Status: "error", Password: "password"}, - err: "error getting cluster password", + // REGRESSION GUARD: Data.Status="ready" with credentials + // .Status="error" and a non-empty password MUST refuse on + // the legacy path. Same reasoning as above. ("error" is a + // real legacy status value but not a public one — the public + // classifier rejects it via its default arm.) + name: "ready cluster with stale error credentials refuses", + clusterSt: "ready", + credStatus: "error", + credPwd: "secret", + err: "error getting cluster password", }, { - name: "empty password", - credentials: mpgv1.GetManagedClusterCredentialsResponse{Status: "ready"}, - err: "error getting cluster password", + // The cluster status alone ("creating", "failed", etc.) is + // NOT consulted on the legacy path — only credentials.Status + // and credentials.Password are. So a "ready"-credentials + // envelope with a non-empty password proceeds regardless of + // the cluster status field. + name: "creating cluster with ready credentials proceeds", + clusterSt: "creating", + credStatus: "ready", + credPwd: "secret", + err: "", // no error }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - response := &mpgv1.GetManagedClusterResponse{Credentials: tt.credentials} + response := &mpgv1.GetManagedClusterResponse{ + Data: mpgv1.ManagedCluster{Name: name, Status: tt.clusterSt}, + Credentials: mpgv1.GetManagedClusterCredentialsResponse{ + Status: tt.credStatus, + Password: tt.credPwd, + }, + } - credentials, err := resolveConnectCredentials(context.Background(), response, "") + credentials, err := resolveConnectCredentials(context.Background(), response, true, "") + if tt.err == "" { + require.NoError(t, err) + require.NotNil(t, credentials) + return + } require.EqualError(t, err, tt.err) assert.Nil(t, credentials) }) @@ -110,9 +356,12 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { func TestResolveExplicitUserConnectCredentials(t *testing.T) { response := &mpgv1.GetManagedClusterResponse{ - Data: mpgv1.ManagedCluster{Id: "cluster-id"}, + Data: mpgv1.ManagedCluster{ + Id: "cluster-id", + Name: "test-cluster", + Status: "ready", + }, Credentials: mpgv1.GetManagedClusterCredentialsResponse{ - Status: "initializing", DBName: "default-db", }, } @@ -130,7 +379,7 @@ func TestResolveExplicitUserConnectCredentials(t *testing.T) { } ctx := mpgv1.NewContextWithClient(context.Background(), client) - credentials, err := resolveConnectCredentials(ctx, response, "app-user") + credentials, err := resolveConnectCredentials(ctx, response, true, "app-user") require.NoError(t, err) assert.Equal(t, "app-user", credentials.User) @@ -139,6 +388,7 @@ func TestResolveExplicitUserConnectCredentials(t *testing.T) { } func TestResolveExplicitUserConnectCredentialsErrors(t *testing.T) { + const name = "test-cluster" t.Run("empty password", func(t *testing.T) { client := &mock.MpgV1Client{ GetUserCredentialsFunc: func(context.Context, string, string) (mpgv1.GetUserCredentialsResponse, error) { @@ -147,7 +397,10 @@ func TestResolveExplicitUserConnectCredentialsErrors(t *testing.T) { } ctx := mpgv1.NewContextWithClient(context.Background(), client) - credentials, err := resolveConnectCredentials(ctx, &mpgv1.GetManagedClusterResponse{}, "app-user") + response := &mpgv1.GetManagedClusterResponse{ + Data: mpgv1.ManagedCluster{Name: name, Status: "ready"}, + } + credentials, err := resolveConnectCredentials(ctx, response, true, "app-user") require.EqualError(t, err, "error getting user password") assert.Nil(t, credentials) @@ -161,9 +414,292 @@ func TestResolveExplicitUserConnectCredentialsErrors(t *testing.T) { } ctx := mpgv1.NewContextWithClient(context.Background(), client) - credentials, err := resolveConnectCredentials(ctx, &mpgv1.GetManagedClusterResponse{}, "app-user") + response := &mpgv1.GetManagedClusterResponse{ + Data: mpgv1.ManagedCluster{Name: name, Status: "ready"}, + } + credentials, err := resolveConnectCredentials(ctx, response, true, "app-user") require.EqualError(t, err, "failed retrieving credentials for user app-user: request failed") assert.Nil(t, credentials) }) } + +// TestResolveConnectCredentialsPublic covers the public-API code paths for +// Connect credentials: default-user goes through +// GetManagedPostgresUserCredentials("fly-user") and DBName defaults to +// "fly-db"; explicit-user passes the flag value straight through. Data.Status +// is set to "ready" on every response here so the status classifier (which +// runs before any credentials call) does not interfere — see +// TestResolveConnectCredentialsPublicStatusClassifier for the dedicated +// status-classifier coverage. See connectStatusRefusal's doc comment for +// the legacy/public status-split rationale. +func TestResolveConnectCredentialsPublic(t *testing.T) { + response := &mpgv1.GetManagedClusterResponse{ + Data: mpgv1.ManagedCluster{Id: "cluster-id", Name: "test-cluster", Status: "ready"}, + } + + t.Run("default user resolves fly-user from public API", func(t *testing.T) { + credCalls := 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(_ context.Context, clusterID, username string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + require.Equal(t, "cluster-id", clusterID) + require.Equal(t, "fly-user", username) + + return flaps.ManagedPostgresUserCredentials{Username: "fly-user", Password: "p"}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "") + require.NoError(t, err) + require.Equal(t, 1, credCalls) + require.Equal(t, "fly-user", credentials.User) + require.Equal(t, "p", credentials.Password) + require.Equal(t, "fly-db", credentials.DBName) + }) + + t.Run("explicit user passes flag value through", func(t *testing.T) { + credCalls := 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(_ context.Context, clusterID, username string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + require.Equal(t, "cluster-id", clusterID) + require.Equal(t, "alice", username) + + return flaps.ManagedPostgresUserCredentials{Username: "alice", Password: "a"}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "alice") + require.NoError(t, err) + require.Equal(t, 1, credCalls) + require.Equal(t, "alice", credentials.User) + require.Equal(t, "a", credentials.Password) + // The public path has no envelope DBName; explicit users fall back to + // defaultMPGDatabase ("fly-db") so that run_connect.go's psql URL + // construction lands on the plan-required default when neither + // --database nor an interactive prompt supplies one. + require.Equal(t, "fly-db", credentials.DBName) + }) + + t.Run("default user empty password mirrors legacy error", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{Username: "fly-user", Password: ""}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "") + require.EqualError(t, err, "error getting cluster password") + assert.Nil(t, credentials) + }) + + t.Run("explicit user empty password returns user error", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{Username: "alice", Password: ""}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "alice") + require.EqualError(t, err, "error getting user password") + assert.Nil(t, credentials) + }) + + t.Run("default user public credentials error propagates", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{}, errors.New("denied") + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "") + require.EqualError(t, err, "failed retrieving credentials for user fly-user: denied") + assert.Nil(t, credentials) + }) + + t.Run("explicit user public credentials error propagates", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{}, errors.New("missing user") + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "alice") + require.EqualError(t, err, "failed retrieving credentials for user alice: missing user") + assert.Nil(t, credentials) + }) +} + +// TestPublicToLegacyClusterResponse verifies that the public-API cluster is +// converted to the legacy ui-ex shape, preserving the bare-address column for +// proxyParams and the Status field for downstream status checks +// (connectStatusRefusal / maybeWarnLegacyNotReady). +func TestPublicToLegacyClusterResponse(t *testing.T) { + got, _ := publicToLegacyClusterResponse(samplePublicCluster()) + require.Equal(t, "mpg-123", got.Data.Id) + require.Equal(t, "test-cluster", got.Data.Name) + require.Equal(t, "ready", got.Data.Status) + require.Equal(t, "ord", got.Data.Region) + require.Equal(t, "development", got.Data.Plan) + require.Equal(t, 10, got.Data.Disk) + require.Equal(t, 1, got.Data.Replicas) + require.Equal(t, "Test Org", got.Data.Organization.Name) + require.Equal(t, "test-org", got.Data.Organization.Slug) + require.Equal(t, "10.0.0.1", got.Data.IpAssignments.Direct) +} + +// TestResolveConnectCredentialsPublicStatusClassifier proves the public-path +// status classifier short-circuits BEFORE any credentials resolution call, +// for both the default-user and explicit-user connect paths. It exercises +// connectStatusRefusal (via resolveConnectCredentials) for the full 9-value +// status matrix — the 7 documented public statuses plus the "error" / +// "degraded" unrecognized sentinels — and asserts on each: +// +// - the exact deliberate refusal message, with the cluster name +// interpolated; +// - the credentials endpoint is never called (credCalls == 0). +// +// On the proceed status ("ready") it asserts: +// +// - no refusal error; +// - the credentials endpoint IS called exactly once (credCalls == 1). +// +// The classifier applies ONLY to the public path (useLegacy == false). The +// legacy path (useLegacy == true) is intentionally NOT exercised here — it +// uses its original pre-migration credentials.Status / credentials.Password +// post-fetch logic instead, pinned by TestResolveDefaultConnectCredentials +// above. +func TestResolveConnectCredentialsPublicStatusClassifier(t *testing.T) { + const name = "test-cluster" + const unknownStatus = "degraded" + tests := []struct { + status string + wantErr string // expected refusal error message; "" means proceed. + }{ + {"ready", ""}, + {"standby_ready", "cluster " + name + " is a standby replica and cannot be used with fly mpg connect"}, + {"creating", "cluster " + name + " is still being created, wait a bit more"}, + {"deleting", "cluster " + name + " is being deleted and cannot be connected to"}, + {"deleted", "cluster " + name + " has been deleted and cannot be connected to"}, + {"failed", "cluster " + name + " is in a failed state"}, + {"initializing", "cluster " + name + " is not currently ready for connections (status: initializing)"}, + // Sentinel: "error" is NOT a real public cluster status — it is + // rejected by the default arm of the classifier. + {"error", `cluster ` + name + ` is in an unrecognized state ("error") and cannot be connected to`}, + // Sentinel: an arbitrary future / unmapped status also fails closed. + {unknownStatus, `cluster ` + name + ` is in an unrecognized state ("` + unknownStatus + `") and cannot be connected to`}, + } + + for _, tt := range tests { + t.Run(tt.status+"/default_user_public", func(t *testing.T) { + c := samplePublicCluster() + c.Status = tt.status + c.Name = name + response, _ := publicToLegacyClusterResponse(c) + + credCalls := 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + + return flaps.ManagedPostgresUserCredentials{Username: "fly-user", Password: "p"}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, &response, false, "") + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + assert.Nil(t, credentials) + require.Equal(t, 0, credCalls, "refusal must short-circuit before any credentials call (default user, public path)") + + return + } + require.NoError(t, err) + require.Equal(t, 1, credCalls, "proceed must hit the credentials endpoint exactly once") + require.NotNil(t, credentials) + }) + + t.Run(tt.status+"/explicit_user_public", func(t *testing.T) { + c := samplePublicCluster() + c.Status = tt.status + c.Name = name + response, _ := publicToLegacyClusterResponse(c) + + credCalls := 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + + return flaps.ManagedPostgresUserCredentials{Username: "alice", Password: "p"}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, &response, false, "alice") + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + assert.Nil(t, credentials) + require.Equal(t, 0, credCalls, "refusal must short-circuit before any credentials call (explicit user, public path)") + + return + } + require.NoError(t, err) + require.Equal(t, 1, credCalls, "proceed must hit the credentials endpoint exactly once") + require.NotNil(t, credentials) + }) + } +} + +// TestProxyParamsPublicNonDefaultPort proves that the public path dials the +// actual advertised port from Endpoints.Primary.Direct.Port instead of the +// legacy hardcoded 5432. The legacy path is also pinned to 5432 here so the +// regression surface is locked down on both sides. The public-zero case is +// pinned to an error so a genuinely-advertised port 0 cannot be silently +// treated like the legacy "no port field" default. +func TestProxyParamsPublicNonDefaultPort(t *testing.T) { + t.Run("public port 5433 dials 5433, not 5432", func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = 5433 + response, port := publicToLegacyClusterResponse(c) + + _, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, "10.0.0.1", params.RemoteHost) + require.Equal(t, []string{"16380", "5433"}, params.Ports) + }) + + t.Run("public port 5432 stays 5432 (no-op for the default)", func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = 5432 + response, port := publicToLegacyClusterResponse(c) + + _, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, []string{"16380", "5432"}, params.Ports) + }) + + t.Run("legacy port stays hardcoded 5432", func(t *testing.T) { + response := sampleLegacyCluster() + // legacy path never calls publicToLegacyClusterResponse, so its port + // value is always nil — the proxyParams fallback kicks in and "5432" + // is used exactly as before. + _, params, err := proxyParams(&response, nil, "16380", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, []string{"16380", "5432"}, params.Ports) + }) + + t.Run("public port 0 surfaces as an error instead of silently dialing 5432", func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = 0 + response, port := publicToLegacyClusterResponse(c) + // Sanity check: the adapter still produces a non-nil pointer (so the + // public-vs-legacy distinction is preserved). + require.NotNil(t, port) + require.Equal(t, 0, *port) + + cluster, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) + require.EqualError(t, err, "error getting cluster port") + assert.Nil(t, cluster) + assert.Nil(t, params) + }) +} diff --git a/internal/command/mpg/v2/run_connect.go b/internal/command/mpg/v2/run_connect.go index 6113a141ea..1c53b4c490 100644 --- a/internal/command/mpg/v2/run_connect.go +++ b/internal/command/mpg/v2/run_connect.go @@ -3,6 +3,7 @@ package cmdv2 import ( "context" "fmt" + "io" "os" "os/exec" "os/signal" @@ -77,14 +78,13 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string, p } } - cluster, params, credentials, err := GetMpgConnectParams(ctx, localProxyPort, username, clusterID, resolvedOrgSlug) + cluster, useLegacy, params, credentials, err := GetMpgConnectParams(ctx, localProxyPort, username, clusterID, resolvedOrgSlug) if err != nil { return err } - if cluster.Status != "ready" { - fmt.Fprintf(io.ErrOut, "%s Cluster is not in ready state, currently: %s\n", aurora.Yellow("WARN"), cluster.Status) - } + // Gated on useLegacy; see maybeWarnLegacyNotReady's doc comment for why. + maybeWarnLegacyNotReady(io.ErrOut, useLegacy, cluster) psqlPath, err := exec.LookPath("psql") if err != nil { @@ -103,15 +103,7 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string, p return err } - user := credentials.User - password := credentials.Password - - // Use selected database or fall back to default from credentials - if db == "" { - db = credentials.DBName - } - - connectUrl := fmt.Sprintf("postgresql://%s:%s@localhost:%s/%s", user, password, localProxyPort, db) + connectUrl := buildConnectURL(credentials, db, localProxyPort) // Allow Ctrl+C signals to hit psql psqlCtx, psqlCancel := context.WithCancel(context.WithoutCancel(ctx)) @@ -174,3 +166,35 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string, p return err } + +// buildConnectURL composes the psql connection URL from resolved credentials +// and the proxy port. db follows the priority used by RunConnect: an explicit +// --database value (or interactive prompt result) wins over credentials.DBName. +// credentials.DBName is the plan-required default ("fly-db") on both the +// public default-user and public explicit-user paths, so a non-interactive +// `fly mpg connect --user alice` lands on postgresql://.../fly-db. +func buildConnectURL(credentials *mpgv2.GetClusterCredentialsResponse, db string, localProxyPort string) string { + if db == "" { + db = credentials.DBName + } + + return fmt.Sprintf("postgresql://%s:%s@localhost:%s/%s", credentials.User, credentials.Password, localProxyPort, db) +} + +// maybeWarnLegacyNotReady restores the pre-migration legacy-path "Cluster +// is not in ready state" stderr warning. It is gated on useLegacy so the +// public path (which already refuses non-ready clusters via +// connectStatusRefusal) stays silent — see connectStatusRefusal's doc +// comment for the legacy/public status-split rationale. The warning +// format is preserved verbatim from the pre-migration code +// (commit 81f75427b^): aurora.Yellow("WARN") + " Cluster is not in ready +// state, currently: \n". The function is a thin side-effecting +// helper so it can be unit-tested with a bytes.Buffer without involving +// the agent/establish code path or RunConnect's exec/psql machinery. +func maybeWarnLegacyNotReady(errOut io.Writer, useLegacy bool, cluster *mpgv2.ManagedCluster) { + if !useLegacy || cluster == nil || cluster.Status == "ready" { + return + } + + fmt.Fprintf(errOut, "%s Cluster is not in ready state, currently: %s\n", aurora.Yellow("WARN"), cluster.Status) +} diff --git a/internal/command/mpg/v2/run_connect_test.go b/internal/command/mpg/v2/run_connect_test.go new file mode 100644 index 0000000000..887343912a --- /dev/null +++ b/internal/command/mpg/v2/run_connect_test.go @@ -0,0 +1,121 @@ +package cmdv2 + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/require" + mpgv2 "github.com/superfly/flyctl/internal/uiex/mpg/v2" +) + +// TestBuildConnectURLPublicExplicitUserDefaultsToFlyDB pins the public +// explicit-user connect path so it can never regress to the bad +// postgresql://.../ helper output. Credentials are resolved the same way the +// public branch of resolveConnectCredentials does, then handed straight to +// buildConnectURL the same way RunConnect does. +func TestBuildConnectURLPublicExplicitUserDefaultsToFlyDB(t *testing.T) { + creds := &mpgv2.GetClusterCredentialsResponse{ + User: "alice", + Password: "a", + DBName: "fly-db", // mirrors the fixed public explicit-user branch. + } + + got := buildConnectURL(creds, "", "16380") + require.Equal(t, "postgresql://alice:a@localhost:16380/fly-db", got) +} + +// TestBuildConnectURLRespectsExplicitDatabase verifies the priority order: +// an explicit --database value wins over credentials.DBName. +func TestBuildConnectURLRespectsExplicitDatabase(t *testing.T) { + creds := &mpgv2.GetClusterCredentialsResponse{ + User: "alice", + Password: "a", + DBName: "fly-db", + } + + got := buildConnectURL(creds, "app-db", "16380") + require.Equal(t, "postgresql://alice:a@localhost:16380/app-db", got) +} + +// TestBuildConnectURLEmptyDBNameFallsThrough is a guard for the historical +// bug shape: if the explicit-user public branch ever returns "" again, the +// URL will not silently land on postgresql://.../ (empty path). It will land +// on postgresql://.../ with the user-controlled --database fallback in +// RunConnect still preferring the flag value when present; without that +// fallback the URL here is malformed, which surfaces in tests rather than +// silently connecting to the wrong database. +func TestBuildConnectURLEmptyDBNameFallsThrough(t *testing.T) { + creds := &mpgv2.GetClusterCredentialsResponse{ + User: "alice", + Password: "a", + DBName: "", + } + + got := buildConnectURL(creds, "", "16380") + require.Equal(t, "postgresql://alice:a@localhost:16380/", got) +} + +// TestMaybeWarnLegacyNotReady pins the pre-migration warning format and +// the useLegacy/status gate. See maybeWarnLegacyNotReady's doc comment +// for the gate rationale and the warning-text provenance. +func TestMaybeWarnLegacyNotReady(t *testing.T) { + const name = "test-cluster" + tests := []struct { + name string + useLegacy bool + status string + wantWarn bool + }{ + // Public path (useLegacy == false). connectStatusRefusal refuses + // non-ready public-path clusters upstream, so the useLegacy + // gate is the only thing that matters here. + {name: "public+creating silent", useLegacy: false, status: "creating", wantWarn: false}, + + // Legacy path (useLegacy == true). "ready" is silent (the + // status gate short-circuits before the fprintf); "creating" + // is the pre-migration non-refused case. The legacy-only + // "error" status is real on the legacy path but rejected by + // the public classifier — pinned here so a future migration + // does not silently drop it. + {name: "legacy+ready silent", useLegacy: true, status: "ready", wantWarn: false}, + {name: "legacy+creating warns", useLegacy: true, status: "creating", wantWarn: true}, + {name: "legacy+error warns", useLegacy: true, status: "error", wantWarn: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + cluster := &mpgv2.ManagedCluster{Name: name, Status: tt.status} + maybeWarnLegacyNotReady(&buf, tt.useLegacy, cluster) + + got := buf.String() + if !tt.wantWarn { + require.Empty(t, got, "no warning expected for useLegacy=%v status=%q", tt.useLegacy, tt.status) + + return + } + // Robust match against the pre-migration rendered text. We + // assert on "WARN" (the literal aurora payload when not a + // TTY) and on the exact "currently: " interpolation, + // rather than asserting the aurora-wrapped string verbatim, + // so this stays green regardless of the test harness's color + // configuration. + require.Contains(t, got, "WARN", "warning must contain the literal 'WARN' marker") + require.Contains(t, got, "Cluster is not in ready state, currently: "+tt.status) + require.True(t, bytes.HasSuffix(buf.Bytes(), []byte("\n")), "warning must end with a newline (pre-migration format)") + }) + } +} + +// TestMaybeWarnLegacyNotReadyNilCluster pins the defensive nil-cluster +// short-circuit. RunConnect only calls the helper with the cluster +// returned by GetMpgConnectParams, which is never nil on the success +// path, but the helper is public and the nil guard prevents a panic if +// the contract ever loosens. +func TestMaybeWarnLegacyNotReadyNilCluster(t *testing.T) { + var buf bytes.Buffer + require.NotPanics(t, func() { + maybeWarnLegacyNotReady(&buf, true, nil) + }) + require.Empty(t, buf.String()) +} diff --git a/internal/command/mpg/v2/run_proxy.go b/internal/command/mpg/v2/run_proxy.go index bcff1c67fa..f7927d3a04 100644 --- a/internal/command/mpg/v2/run_proxy.go +++ b/internal/command/mpg/v2/run_proxy.go @@ -2,15 +2,30 @@ package cmdv2 import ( "context" + "errors" "fmt" + "strconv" + fly "github.com/superfly/fly-go" + "github.com/superfly/fly-go/flaps" "github.com/superfly/flyctl/agent" "github.com/superfly/flyctl/internal/flag" + "github.com/superfly/flyctl/internal/flapsutil" "github.com/superfly/flyctl/internal/flyutil" + "github.com/superfly/flyctl/internal/uiex/mpg" mpgv2 "github.com/superfly/flyctl/internal/uiex/mpg/v2" "github.com/superfly/flyctl/proxy" ) +// defaultMPGUser is the literal username used when resolving the default +// fly-user on the public Machines API for Connect. Verified against the +// orchestrator and ui-ex repos. +const defaultMPGUser = "fly-user" + +// defaultMPGDatabase is the literal database name used when no --database +// flag is given and no interactive prompt answer is available. +const defaultMPGDatabase = "fly-db" + func RunProxy(ctx context.Context, clusterID string, resolvedOrgSlug string, proxyPort string) error { _, params, err := GetMpgProxyParams(ctx, proxyPort, clusterID, resolvedOrgSlug) if err != nil { @@ -29,12 +44,12 @@ func GetMpgProxyParams( clusterID string, resolvedOrgSlug string, ) (*mpgv2.ManagedCluster, *proxy.ConnectParams, error) { - response, err := getCluster(ctx, clusterID) + response, _, port, err := getCluster(ctx, clusterID) if err != nil { return nil, nil, err } - cluster, params, err := buildProxyParams(ctx, response, localProxyPort, resolvedOrgSlug) + cluster, params, err := buildProxyParams(ctx, response, port, localProxyPort, resolvedOrgSlug) if err != nil { return nil, nil, err } @@ -44,48 +59,143 @@ func GetMpgProxyParams( // GetMpgConnectParams builds proxy connection parameters and resolves the // database credentials needed by fly mpg connect. +// +// The returned useLegacy bool mirrors the value computed inside getCluster +// (true when the public Machines API returned a classified 404 and we fell +// back to the legacy ui-ex client; false when the public API succeeded). +// RunConnect uses it to gate the pre-migration "Cluster is not in ready +// state" stderr warning, which is meaningful only on the legacy path — +// see connectStatusRefusal's doc comment for why the public path does not +// need it. func GetMpgConnectParams( ctx context.Context, localProxyPort string, username string, clusterID string, resolvedOrgSlug string, -) (*mpgv2.ManagedCluster, *proxy.ConnectParams, *mpgv2.GetClusterCredentialsResponse, error) { - response, err := getCluster(ctx, clusterID) +) (*mpgv2.ManagedCluster, bool, *proxy.ConnectParams, *mpgv2.GetClusterCredentialsResponse, error) { + response, useLegacy, port, err := getCluster(ctx, clusterID) if err != nil { - return nil, nil, nil, err + return nil, false, nil, nil, err } - credentials, err := resolveConnectCredentials(ctx, response, username) + credentials, err := resolveConnectCredentials(ctx, response, useLegacy, username) if err != nil { - return nil, nil, nil, err + return nil, false, nil, nil, err } - cluster, params, err := buildProxyParams(ctx, response, localProxyPort, resolvedOrgSlug) + cluster, params, err := buildProxyParams(ctx, response, port, localProxyPort, resolvedOrgSlug) if err != nil { - return nil, nil, nil, err + return nil, false, nil, nil, err } - return cluster, params, credentials, nil + return cluster, useLegacy, params, credentials, nil } -func getCluster(ctx context.Context, clusterID string) (*mpgv2.GetClusterResponse, error) { - mpgClient := mpgv2.ClientFromContext(ctx) - response, err := mpgClient.GetClusterById(ctx, clusterID) +// getCluster retrieves cluster details via the public Machines API, falling +// back to the legacy MPGv1 client only when the public call returns a +// classified 404. Any other non-nil public error is returned immediately, +// wrapped in "failed retrieving cluster %s: %w". The returned useLegacy flag +// indicates which client +// produced the response so downstream credential resolution can choose the +// appropriate code path. The returned port is the public API's advertised +// direct-endpoint port (nil on the legacy path, which has no port field) — +// carried as a separate return value rather than a field on the legacy +// mpgv2.ManagedCluster type, since that type is part of the private/legacy +// client package (internal/uiex/mpg) that this migration must not modify. +func getCluster(ctx context.Context, clusterID string) (*mpgv2.GetClusterResponse, bool, *int, error) { + flapsClient := flapsutil.ClientFromContext(ctx) + publicCluster, err := flapsClient.GetManagedPostgresCluster(ctx, clusterID) + if err == nil { + response, port := publicToLegacyClusterResponse(publicCluster) + return &response, false, port, nil + } + + if !errors.Is(err, flaps.ErrFlapsNotFound) { + return nil, false, nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) + } + + legacyClient := mpgv2.ClientFromContext(ctx) + response, err := legacyClient.GetClusterById(ctx, clusterID) if err != nil { - return nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) + return nil, true, nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) } - return &response, nil + return &response, true, nil, nil +} + +// publicToLegacyClusterResponse wraps the public Machines API cluster in the +// legacy ui-ex envelope so that downstream callers (proxyParams, +// resolveConnectCredentials) can read IpAssignments.Direct and Status without +// translation. The legacy shape preserves the bare-address column for +// proxyParams.RemoteHost and the Status field for connectStatusRefusal. +// +// The public API's advertised Endpoints.Primary.Direct.Port is returned +// separately (not as a field on the legacy mpgv2.ManagedCluster type) so +// proxyParams can dial the real advertised port instead of the legacy +// hardcoded 5432, without adding a public-API-only field to the private +// legacy client package. A nil port return means "no port info" (the legacy +// path never calls this function at all, so in practice this only happens +// if ever called with a zero-value input); a non-nil pointer — even to 0 — +// means the public path returned some value, and *0 is treated as invalid +// by proxyParams. +func publicToLegacyClusterResponse(c flaps.ManagedPostgresCluster) (mpgv2.GetClusterResponse, *int) { + port := c.Endpoints.Primary.Direct.Port + + return mpgv2.GetClusterResponse{ + Data: mpgv2.ManagedCluster{ + Id: c.ID, + Name: c.Name, + Status: c.Status, + Region: c.Region, + Plan: c.Plan, + Disk: c.DiskSizeGB, + Replicas: c.Replicas, + Organization: fly.Organization{Name: c.Organization.Name, Slug: c.Organization.Slug}, + IpAssignments: mpg.ManagedClusterIpAssignments{Direct: c.Endpoints.Primary.Direct.Host}, + }, + }, &port } +// resolveConnectCredentials returns the credentials used by RunConnect. +// useLegacy=true routes through the legacy ui-ex client; useLegacy=false +// routes through the public Machines API's +// GetManagedPostgresUserCredentials. The default user (no --user flag) +// resolves to defaultMPGUser ("fly-user"); explicit users pass the flag +// value straight through. The public path has no envelope-level DBName, +// so both public branches fall back to defaultMPGDatabase ("fly-db") — +// which run_connect.go's buildConnectURL uses to land on the plan-required +// default when neither --database nor an interactive prompt supplies one. +// +// Status gating is split across the two paths because their vocabularies +// differ; see connectStatusRefusal's doc comment for the full rationale. +// Briefly: the public path consults response.Data.Status BEFORE any +// credentials call (a 7-value enum + fail-closed default); the legacy +// path keeps the pre-migration post-fetch credentials.Status + +// credentials.Password check ("error" is a real legacy value but not a +// public one, and the legacy envelope's Status field is semantically +// distinct from cluster status). func resolveConnectCredentials( ctx context.Context, response *mpgv2.GetClusterResponse, + useLegacy bool, username string, ) (*mpgv2.GetClusterCredentialsResponse, error) { + // Status classifier runs BEFORE any credentials call on the public + // path only — see connectStatusRefusal's doc comment for the + // legacy/public split. response.Data.Status is populated on the public + // path via publicToLegacyClusterResponse (from c.Status); it is + // intentionally NOT consulted on the legacy path. + if !useLegacy { + if err := connectStatusRefusal(response.Data.Name, response.Data.Status); err != nil { + return nil, err + } + } + var credentials mpgv2.GetClusterCredentialsResponse - if username != "" { + + switch { + case username != "" && useLegacy: mpgClient := mpgv2.ClientFromContext(ctx) userCreds, err := mpgClient.GetUserCredentials(ctx, response.Data.Id, username) if err != nil { @@ -97,32 +207,159 @@ func resolveConnectCredentials( Password: userCreds.Data.Password, DBName: response.Credentials.DBName, } - } else { + case username != "": + flapsClient := flapsutil.ClientFromContext(ctx) + userCreds, err := flapsClient.GetManagedPostgresUserCredentials(ctx, response.Data.Id, username) + if err != nil { + return nil, fmt.Errorf("failed retrieving credentials for user %s: %w", username, err) + } + + credentials = mpgv2.GetClusterCredentialsResponse{ + User: userCreds.Username, + Password: userCreds.Password, + DBName: defaultMPGDatabase, + } + case useLegacy: credentials = response.Credentials - } + default: + flapsClient := flapsutil.ClientFromContext(ctx) + userCreds, err := flapsClient.GetManagedPostgresUserCredentials(ctx, response.Data.Id, defaultMPGUser) + if err != nil { + return nil, fmt.Errorf("failed retrieving credentials for user %s: %w", defaultMPGUser, err) + } - if username == "" { - if credentials.Status == "initializing" { - return nil, fmt.Errorf("cluster is still initializing, wait a bit more") + credentials = mpgv2.GetClusterCredentialsResponse{ + User: userCreds.Username, + Password: userCreds.Password, + DBName: defaultMPGDatabase, } + } - if credentials.Status == "error" || credentials.Password == "" { - return nil, fmt.Errorf("error getting cluster password") + if useLegacy { + // ORIGINAL pre-migration legacy status/password checks, + // restored verbatim. credentials.Status is only checked on the + // default-user path because the explicit-user legacy + // GetUserCredentials response does not populate that field + // (only User/Password/DBName). See connectStatusRefusal's doc + // comment for why this path is intentionally distinct from the + // public classifier. + if username == "" { + if credentials.Status == "initializing" { + return nil, fmt.Errorf("cluster is still initializing, wait a bit more") + } + + if credentials.Status == "error" || credentials.Password == "" { + return nil, fmt.Errorf("error getting cluster password") + } + } else if credentials.Password == "" { + return nil, fmt.Errorf("error getting user password") } } else if credentials.Password == "" { + // Public path: empty-password fallback after the credentials + // call. The wording tracks whether the user was explicit + // (flag/prompt) or defaulted to the plan-required fly-user, + // preserving the existing pre-fix messages. The public path + // has no envelope-level Status to inspect post-fetch, so the + // pre-credential connectStatusRefusal call above is the only + // status gate. + if username == "" { + return nil, fmt.Errorf("error getting cluster password") + } + return nil, fmt.Errorf("error getting user password") } return &credentials, nil } +// connectStatusRefusal is the explicit status classifier for fly mpg +// connect. It returns nil when the cluster's status permits a connect +// attempt and a deliberate, status-specific refusal error otherwise. The +// complete emitted set of "status" values from a successful public MPG +// cluster lookup (traced across the mpg / nomad-firecracker / ui-ex code +// paths this session) is: +// +// ready, standby_ready, creating, deleting, failed, deleted, initializing +// +// "error" is NOT a real public cluster status value; it does not appear +// in the public enum and is rejected if encountered. Behavior per status: +// +// - ready: proceed, no message. +// - standby_ready: refuse — a standby replica is not a connect target. +// Fail-closed pending a product decision on standby +// semantics; not even a "warn and continue" — refused +// outright. +// - creating: refuse — friendly wording distinct from initializing +// since the cluster is in the process of coming up, +// not stuck mid-provision. +// - deleting: refuse — cluster is going away, connections +// meaningless. +// - deleted: refuse — terminal state, no connect possible. +// - failed: refuse — accurate wording ("cluster is in a failed +// state"), not the misleading "error getting cluster +// password" misdiagnosis. +// - initializing: refuse — neutral wording because this now covers +// many underlying states (degraded, updating, +// resizing, credential rotation, promotion, and +// future unmapped states), not just fresh +// provisioning. Do not over-promise "wait a bit more" +// for a state that may not be purely transient. +// - default: refuse — unknown / unrecognized statuses fail +// closed with the actual status string quoted so the +// user (and on-call) can see what the public API +// actually returned. Do NOT let unknown values fall +// through to credential resolution. +// +// The classifier applies ONLY to the public path (useLegacy == false), +// BEFORE any public credential call, on both the default-user and +// explicit-user paths. It is intentionally NOT applied to the legacy +// path (useLegacy == true): the legacy status vocabulary differs from +// the public API's — e.g. "error" is a real legacy cluster status value +// but not a public one — and the pre-migration legacy code understood it +// natively via credentials.Status (the legacy credentials envelope's own +// status field, populated post-fetch and semantically distinct from +// response.Data.Status). Applying this public-only classifier to legacy +// responses would mis-handle legacy-only statuses as "unrecognized +// state" and silently drop the credentials.Status check. +// +// The classifier is NOT applied to fly mpg proxy — that command +// deliberately does not gate on status because the raw TCP proxy never +// uses credentials, only the direct IP/port; see RunProxy / +// GetMpgProxyParams. Adding a status gate there would reintroduce the +// exact coupling flyctl#5048 was built to remove. +// +// name is the cluster identifier included in the refusal message so the +// user knows which cluster the refusal applies to. It comes from +// response.Data.Name (populated on both the public and legacy paths). +func connectStatusRefusal(name, status string) error { + switch status { + case "ready": + return nil + case "standby_ready": + return fmt.Errorf("cluster %s is a standby replica and cannot be used with fly mpg connect", name) + case "creating": + return fmt.Errorf("cluster %s is still being created, wait a bit more", name) + case "deleting": + return fmt.Errorf("cluster %s is being deleted and cannot be connected to", name) + case "deleted": + return fmt.Errorf("cluster %s has been deleted and cannot be connected to", name) + case "failed": + return fmt.Errorf("cluster %s is in a failed state", name) + case "initializing": + return fmt.Errorf("cluster %s is not currently ready for connections (status: initializing)", name) + default: + return fmt.Errorf("cluster %s is in an unrecognized state (%q) and cannot be connected to", name, status) + } +} + func buildProxyParams( ctx context.Context, response *mpgv2.GetClusterResponse, + port *int, localProxyPort string, resolvedOrgSlug string, ) (*mpgv2.ManagedCluster, *proxy.ConnectParams, error) { - cluster, params, err := proxyParams(response, localProxyPort, resolvedOrgSlug, flag.GetBindAddr(ctx), nil) + cluster, params, err := proxyParams(response, port, localProxyPort, resolvedOrgSlug, flag.GetBindAddr(ctx), nil) if err != nil { return nil, nil, err } @@ -147,6 +384,7 @@ func buildProxyParams( func proxyParams( response *mpgv2.GetClusterResponse, + port *int, localProxyPort string, resolvedOrgSlug string, bindAddr string, @@ -157,8 +395,29 @@ func proxyParams( return nil, nil, fmt.Errorf("error getting cluster IP") } + // remotePort is the port the proxy will dial on the remote host. The + // legacy path has no port field at all, so it always calls this with + // port == nil and we fall back to "5432" exactly as before. The public + // path passes the advertised Endpoints.Primary.Direct.Port through + // getCluster so a non-5432 public port dials that real port instead of + // being silently rewritten to 5432. A public API that advertises port 0 + // is treated as an invalid/unexpected value and surfaces as an error — + // distinguishing it from the legacy "no port field" case, where 5432 is + // the historical default and the safe fallback. The *int rather than a + // separate bool encodes the "optional value" semantic directly: nil = + // no port info (legacy), non-nil (even *port == 0) = public path + // advertised some value. + remotePort := "5432" + if port != nil { + if *port == 0 { + return nil, nil, fmt.Errorf("error getting cluster port") + } + + remotePort = strconv.Itoa(*port) + } + return cluster, &proxy.ConnectParams{ - Ports: []string{localProxyPort, "5432"}, + Ports: []string{localProxyPort, remotePort}, OrganizationSlug: resolvedOrgSlug, Dialer: dialer, BindAddr: bindAddr, diff --git a/internal/command/mpg/v2/run_proxy_test.go b/internal/command/mpg/v2/run_proxy_test.go index 90134aef15..ca376f2d41 100644 --- a/internal/command/mpg/v2/run_proxy_test.go +++ b/internal/command/mpg/v2/run_proxy_test.go @@ -8,6 +8,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + fly "github.com/superfly/fly-go" + "github.com/superfly/fly-go/flaps" + "github.com/superfly/flyctl/internal/flapsutil" "github.com/superfly/flyctl/internal/mock" "github.com/superfly/flyctl/internal/uiex/mpg" mpgv2 "github.com/superfly/flyctl/internal/uiex/mpg/v2" @@ -53,7 +56,7 @@ func TestProxyParamsIgnoreCredentials(t *testing.T) { } dialer := &testDialer{} - cluster, params, err := proxyParams(&response, "15432", "test-org", "127.0.0.2", dialer) + cluster, params, err := proxyParams(&response, nil, "15432", "test-org", "127.0.0.2", dialer) require.NoError(t, err) assert.Same(t, &response.Data, cluster) assert.Equal(t, []string{"15432", "5432"}, params.Ports) @@ -66,42 +69,285 @@ func TestProxyParamsIgnoreCredentials(t *testing.T) { } func TestProxyParamsRequireDirectIP(t *testing.T) { - cluster, params, err := proxyParams(&mpgv2.GetClusterResponse{}, "15432", "test-org", "127.0.0.1", nil) + cluster, params, err := proxyParams(&mpgv2.GetClusterResponse{}, nil, "15432", "test-org", "127.0.0.1", nil) require.EqualError(t, err, "error getting cluster IP") assert.Nil(t, cluster) assert.Nil(t, params) } +// samplePublicCluster is a representative public Machines API cluster payload. +func samplePublicCluster() flaps.ManagedPostgresCluster { + return flaps.ManagedPostgresCluster{ + ID: "mpg-123", + Name: "test-cluster", + Status: "ready", + Region: "ord", + Plan: "development", + DiskSizeGB: 10, + Replicas: 1, + Organization: flaps.ManagedPostgresOrganization{Name: "Test Org", Slug: "test-org"}, + Endpoints: flaps.ManagedPostgresEndpoints{ + Primary: struct { + Direct flaps.ManagedPostgresEndpoint `json:"direct"` + Pooler flaps.ManagedPostgresEndpoint `json:"pooler"` + }{ + Direct: flaps.ManagedPostgresEndpoint{Host: "10.0.0.1", Port: 5432}, + Pooler: flaps.ManagedPostgresEndpoint{Host: "10.0.0.2", Port: 6432}, + }, + }, + } +} + +// sampleLegacyCluster is a representative legacy MPGv1 cluster payload. +// Direct is a bare address (no port) — the historical shape of this column. +func sampleLegacyCluster() mpgv2.GetClusterResponse { + return mpgv2.GetClusterResponse{ + Data: mpgv2.ManagedCluster{ + Id: "mpg-123", Name: "test-cluster", Region: "ord", Status: "ready", + Plan: "development", Disk: 10, Replicas: 1, + Organization: fly.Organization{Name: "Test Org", Slug: "test-org"}, + IpAssignments: mpg.ManagedClusterIpAssignments{Direct: "10.0.0.1"}, + }, + Credentials: mpgv2.GetClusterCredentialsResponse{ + Status: "ready", User: "app", Password: "secret", + DBName: "app", ConnectionUri: "postgres://app:secret@10.0.0.1:5432/app", + }, + } +} + +func TestGetCluster(t *testing.T) { + tests := []struct { + name string + publicCluster flaps.ManagedPostgresCluster + publicErr error + legacyResponse mpgv2.GetClusterResponse + legacyErr error + wantUseLegacy bool + wantDirectHost string + wantStatus string + wantErr string + wantLegacyCalls int + }{ + { + name: "public success maps to legacy shape", + publicCluster: samplePublicCluster(), + wantUseLegacy: false, + wantDirectHost: "10.0.0.1", + wantStatus: "ready", + wantLegacyCalls: 0, + }, + { + name: "public success with empty host preserves empty", + publicCluster: func() flaps.ManagedPostgresCluster { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Host = "" + + return c + }(), + wantUseLegacy: false, + wantDirectHost: "", + wantStatus: "ready", + wantLegacyCalls: 0, + }, + { + name: "classified 404 falls back to legacy", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: flaps.ErrFlapsNotFound, + legacyResponse: sampleLegacyCluster(), + wantUseLegacy: true, + wantDirectHost: "10.0.0.1", + wantStatus: "ready", + wantLegacyCalls: 1, + }, + { + name: "classified 404 with legacy failure propagates legacy error", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: flaps.ErrFlapsNotFound, + legacyResponse: mpgv2.GetClusterResponse{}, + legacyErr: errors.New("legacy denied"), + wantErr: "failed retrieving cluster mpg-123: legacy denied", + wantLegacyCalls: 1, + }, + { + name: "non-404 public error returns without fallback", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: errors.New("boom"), + wantErr: "failed retrieving cluster mpg-123: boom", + wantLegacyCalls: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + publicCalls, legacyCalls := 0, 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresClusterFunc: func(_ context.Context, id string) (flaps.ManagedPostgresCluster, error) { + publicCalls++ + require.Equal(t, "mpg-123", id) + + return tt.publicCluster, tt.publicErr + }, + }) + ctx = mpgv2.NewContextWithClient(ctx, &mock.MpgV2Client{ + GetClusterByIdFunc: func(_ context.Context, id string) (mpgv2.GetClusterResponse, error) { + legacyCalls++ + require.Equal(t, "mpg-123", id) + + return tt.legacyResponse, tt.legacyErr + }, + }) + + got, useLegacy, _, err := getCluster(ctx, "mpg-123") + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + require.Nil(t, got) + require.Equal(t, 1, publicCalls) + require.Equal(t, tt.wantLegacyCalls, legacyCalls) + + return + } + require.NoError(t, err) + require.Equal(t, tt.wantUseLegacy, useLegacy) + require.NotNil(t, got) + require.Equal(t, tt.wantDirectHost, got.Data.IpAssignments.Direct) + require.Equal(t, tt.wantStatus, got.Data.Status) + require.Equal(t, 1, publicCalls) + require.Equal(t, tt.wantLegacyCalls, legacyCalls) + }) + } +} + +// TestProxyParamsPublicNeverTouchesCredentials verifies the invariant that +// the proxy code path never resolves credentials: getCluster must only hit +// the cluster lookup endpoint, not the credentials endpoint, on the public- +// success branch. This pins RunProxy against accidentally growing a +// credentials dependency. +func TestProxyParamsPublicNeverTouchesCredentials(t *testing.T) { + credCalls, legacyCredCalls := 0, 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { + return samplePublicCluster(), nil + }, + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + + return flaps.ManagedPostgresUserCredentials{}, nil + }, + }) + ctx = mpgv2.NewContextWithClient(ctx, &mock.MpgV2Client{ + GetUserCredentialsFunc: func(context.Context, string, string) (mpgv2.GetUserCredentialsResponse, error) { + legacyCredCalls++ + + return mpgv2.GetUserCredentialsResponse{}, nil + }, + }) + + response, useLegacy, port, err := getCluster(ctx, "mpg-123") + require.NoError(t, err) + require.NotNil(t, response) + require.False(t, useLegacy) + require.Equal(t, 0, credCalls, "getCluster (public-success path) must never resolve credentials") + require.Equal(t, 0, legacyCredCalls, "getCluster must never reach the legacy client on public success") + + // proxyParams does not take a ctx, so it cannot make any HTTP call; it is + // structurally incapable of leaking credentials. Verify it produces the + // expected bare-host RemoteHost from the public-converted response. The + // credCalls / legacyCredCalls counters were already asserted to be 0 + // above, immediately after getCluster returned, so proxyParams has + // nothing to regress there. + cluster, params, err := proxyParams(response, port, "15432", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.NotNil(t, cluster) + require.NotNil(t, params) + require.Equal(t, "10.0.0.1", params.RemoteHost) +} + func TestResolveDefaultConnectCredentials(t *testing.T) { + // The legacy default-user connect path (useLegacy == true) restores + // the pre-migration post-fetch logic: the legacy credentials + // envelope's own Status field (credentials.Status, semantically + // distinct from cluster status) is checked for "initializing"/"error", + // plus an empty-password fallback. These cases pin that behavior + // with the original error messages. The public status classifier is + // intentionally NOT applied here; that is covered by + // TestResolveConnectCredentialsPublicStatusClassifier below. See + // connectStatusRefusal's doc comment for the legacy/public status- + // split rationale. + const name = "test-cluster" tests := []struct { - name string - credentials mpgv2.GetClusterCredentialsResponse - err string + name string + clusterSt string // response.Data.Status — cluster status. NOT consulted on the legacy path. + credStatus string // credentials.Status — the legacy envelope's own status field (post-fetch). + credPwd string // credentials.Password — empty-password fallback when status checks pass. + err string }{ { - name: "initializing", - credentials: mpgv2.GetClusterCredentialsResponse{Status: "initializing"}, - err: "cluster is still initializing, wait a bit more", + // ORIGINAL legacy: empty password is the empty-password + // fallback; status field is irrelevant when password is empty. + name: "empty password", + clusterSt: "ready", + credStatus: "ready", + credPwd: "", + err: "error getting cluster password", + }, + { + // REGRESSION GUARD: Data.Status="ready" with credentials + // .Status="initializing" and a non-empty password MUST refuse + // on the legacy path. Applying the public-only 7-value + // status classifier to the legacy path (where it does not + // belong) would silently drop the credentials.Status check + // and let this case proceed to psql with possibly-invalid + // credentials. This is the regression this test pins. + name: "ready cluster with stale initializing credentials refuses", + clusterSt: "ready", + credStatus: "initializing", + credPwd: "secret", + err: "cluster is still initializing, wait a bit more", }, { - name: "error status", - credentials: mpgv2.GetClusterCredentialsResponse{Status: "error", Password: "password"}, - err: "error getting cluster password", + // REGRESSION GUARD: Data.Status="ready" with credentials + // .Status="error" and a non-empty password MUST refuse on + // the legacy path. Same reasoning as above. ("error" is a + // real legacy status value but not a public one — the public + // classifier rejects it via its default arm.) + name: "ready cluster with stale error credentials refuses", + clusterSt: "ready", + credStatus: "error", + credPwd: "secret", + err: "error getting cluster password", }, { - name: "empty password", - credentials: mpgv2.GetClusterCredentialsResponse{Status: "ready"}, - err: "error getting cluster password", + // The cluster status alone ("creating", "failed", etc.) is + // NOT consulted on the legacy path — only credentials.Status + // and credentials.Password are. So a "ready"-credentials + // envelope with a non-empty password proceeds regardless of + // the cluster status field. + name: "creating cluster with ready credentials proceeds", + clusterSt: "creating", + credStatus: "ready", + credPwd: "secret", + err: "", // no error }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - response := &mpgv2.GetClusterResponse{Credentials: tt.credentials} + response := &mpgv2.GetClusterResponse{ + Data: mpgv2.ManagedCluster{Name: name, Status: tt.clusterSt}, + Credentials: mpgv2.GetClusterCredentialsResponse{ + Status: tt.credStatus, + Password: tt.credPwd, + }, + } - credentials, err := resolveConnectCredentials(context.Background(), response, "") + credentials, err := resolveConnectCredentials(context.Background(), response, true, "") + if tt.err == "" { + require.NoError(t, err) + require.NotNil(t, credentials) + return + } require.EqualError(t, err, tt.err) assert.Nil(t, credentials) }) @@ -110,9 +356,12 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { func TestResolveExplicitUserConnectCredentials(t *testing.T) { response := &mpgv2.GetClusterResponse{ - Data: mpgv2.ManagedCluster{Id: "cluster-id"}, + Data: mpgv2.ManagedCluster{ + Id: "cluster-id", + Name: "test-cluster", + Status: "ready", + }, Credentials: mpgv2.GetClusterCredentialsResponse{ - Status: "initializing", DBName: "default-db", }, } @@ -130,7 +379,7 @@ func TestResolveExplicitUserConnectCredentials(t *testing.T) { } ctx := mpgv2.NewContextWithClient(context.Background(), client) - credentials, err := resolveConnectCredentials(ctx, response, "app-user") + credentials, err := resolveConnectCredentials(ctx, response, true, "app-user") require.NoError(t, err) assert.Equal(t, "app-user", credentials.User) @@ -139,6 +388,7 @@ func TestResolveExplicitUserConnectCredentials(t *testing.T) { } func TestResolveExplicitUserConnectCredentialsErrors(t *testing.T) { + const name = "test-cluster" t.Run("empty password", func(t *testing.T) { client := &mock.MpgV2Client{ GetUserCredentialsFunc: func(context.Context, string, string) (mpgv2.GetUserCredentialsResponse, error) { @@ -147,7 +397,10 @@ func TestResolveExplicitUserConnectCredentialsErrors(t *testing.T) { } ctx := mpgv2.NewContextWithClient(context.Background(), client) - credentials, err := resolveConnectCredentials(ctx, &mpgv2.GetClusterResponse{}, "app-user") + response := &mpgv2.GetClusterResponse{ + Data: mpgv2.ManagedCluster{Name: name, Status: "ready"}, + } + credentials, err := resolveConnectCredentials(ctx, response, true, "app-user") require.EqualError(t, err, "error getting user password") assert.Nil(t, credentials) @@ -161,9 +414,292 @@ func TestResolveExplicitUserConnectCredentialsErrors(t *testing.T) { } ctx := mpgv2.NewContextWithClient(context.Background(), client) - credentials, err := resolveConnectCredentials(ctx, &mpgv2.GetClusterResponse{}, "app-user") + response := &mpgv2.GetClusterResponse{ + Data: mpgv2.ManagedCluster{Name: name, Status: "ready"}, + } + credentials, err := resolveConnectCredentials(ctx, response, true, "app-user") require.EqualError(t, err, "failed retrieving credentials for user app-user: request failed") assert.Nil(t, credentials) }) } + +// TestResolveConnectCredentialsPublic covers the public-API code paths for +// Connect credentials: default-user goes through +// GetManagedPostgresUserCredentials("fly-user") and DBName defaults to +// "fly-db"; explicit-user passes the flag value straight through. Data.Status +// is set to "ready" on every response here so the status classifier (which +// runs before any credentials call) does not interfere — see +// TestResolveConnectCredentialsPublicStatusClassifier for the dedicated +// status-classifier coverage. See connectStatusRefusal's doc comment for +// the legacy/public status-split rationale. +func TestResolveConnectCredentialsPublic(t *testing.T) { + response := &mpgv2.GetClusterResponse{ + Data: mpgv2.ManagedCluster{Id: "cluster-id", Name: "test-cluster", Status: "ready"}, + } + + t.Run("default user resolves fly-user from public API", func(t *testing.T) { + credCalls := 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(_ context.Context, clusterID, username string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + require.Equal(t, "cluster-id", clusterID) + require.Equal(t, "fly-user", username) + + return flaps.ManagedPostgresUserCredentials{Username: "fly-user", Password: "p"}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "") + require.NoError(t, err) + require.Equal(t, 1, credCalls) + require.Equal(t, "fly-user", credentials.User) + require.Equal(t, "p", credentials.Password) + require.Equal(t, "fly-db", credentials.DBName) + }) + + t.Run("explicit user passes flag value through", func(t *testing.T) { + credCalls := 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(_ context.Context, clusterID, username string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + require.Equal(t, "cluster-id", clusterID) + require.Equal(t, "alice", username) + + return flaps.ManagedPostgresUserCredentials{Username: "alice", Password: "a"}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "alice") + require.NoError(t, err) + require.Equal(t, 1, credCalls) + require.Equal(t, "alice", credentials.User) + require.Equal(t, "a", credentials.Password) + // The public path has no envelope DBName; explicit users fall back to + // defaultMPGDatabase ("fly-db") so that run_connect.go's psql URL + // construction lands on the plan-required default when neither + // --database nor an interactive prompt supplies one. + require.Equal(t, "fly-db", credentials.DBName) + }) + + t.Run("default user empty password mirrors legacy error", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{Username: "fly-user", Password: ""}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "") + require.EqualError(t, err, "error getting cluster password") + assert.Nil(t, credentials) + }) + + t.Run("explicit user empty password returns user error", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{Username: "alice", Password: ""}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "alice") + require.EqualError(t, err, "error getting user password") + assert.Nil(t, credentials) + }) + + t.Run("default user public credentials error propagates", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{}, errors.New("denied") + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "") + require.EqualError(t, err, "failed retrieving credentials for user fly-user: denied") + assert.Nil(t, credentials) + }) + + t.Run("explicit user public credentials error propagates", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{}, errors.New("missing user") + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "alice") + require.EqualError(t, err, "failed retrieving credentials for user alice: missing user") + assert.Nil(t, credentials) + }) +} + +// TestPublicToLegacyClusterResponse verifies that the public-API cluster is +// converted to the legacy ui-ex shape, preserving the bare-address column for +// proxyParams and the Status field for downstream status checks +// (connectStatusRefusal / maybeWarnLegacyNotReady). +func TestPublicToLegacyClusterResponse(t *testing.T) { + got, _ := publicToLegacyClusterResponse(samplePublicCluster()) + require.Equal(t, "mpg-123", got.Data.Id) + require.Equal(t, "test-cluster", got.Data.Name) + require.Equal(t, "ready", got.Data.Status) + require.Equal(t, "ord", got.Data.Region) + require.Equal(t, "development", got.Data.Plan) + require.Equal(t, 10, got.Data.Disk) + require.Equal(t, 1, got.Data.Replicas) + require.Equal(t, "Test Org", got.Data.Organization.Name) + require.Equal(t, "test-org", got.Data.Organization.Slug) + require.Equal(t, "10.0.0.1", got.Data.IpAssignments.Direct) +} + +// TestResolveConnectCredentialsPublicStatusClassifier proves the public-path +// status classifier short-circuits BEFORE any credentials resolution call, +// for both the default-user and explicit-user connect paths. It exercises +// connectStatusRefusal (via resolveConnectCredentials) for the full 9-value +// status matrix — the 7 documented public statuses plus the "error" / +// "degraded" unrecognized sentinels — and asserts on each: +// +// - the exact deliberate refusal message, with the cluster name +// interpolated; +// - the credentials endpoint is never called (credCalls == 0). +// +// On the proceed status ("ready") it asserts: +// +// - no refusal error; +// - the credentials endpoint IS called exactly once (credCalls == 1). +// +// The classifier applies ONLY to the public path (useLegacy == false). The +// legacy path (useLegacy == true) is intentionally NOT exercised here — it +// uses its original pre-migration credentials.Status / credentials.Password +// post-fetch logic instead, pinned by TestResolveDefaultConnectCredentials +// above. +func TestResolveConnectCredentialsPublicStatusClassifier(t *testing.T) { + const name = "test-cluster" + const unknownStatus = "degraded" + tests := []struct { + status string + wantErr string // expected refusal error message; "" means proceed. + }{ + {"ready", ""}, + {"standby_ready", "cluster " + name + " is a standby replica and cannot be used with fly mpg connect"}, + {"creating", "cluster " + name + " is still being created, wait a bit more"}, + {"deleting", "cluster " + name + " is being deleted and cannot be connected to"}, + {"deleted", "cluster " + name + " has been deleted and cannot be connected to"}, + {"failed", "cluster " + name + " is in a failed state"}, + {"initializing", "cluster " + name + " is not currently ready for connections (status: initializing)"}, + // Sentinel: "error" is NOT a real public cluster status — it is + // rejected by the default arm of the classifier. + {"error", `cluster ` + name + ` is in an unrecognized state ("error") and cannot be connected to`}, + // Sentinel: an arbitrary future / unmapped status also fails closed. + {unknownStatus, `cluster ` + name + ` is in an unrecognized state ("` + unknownStatus + `") and cannot be connected to`}, + } + + for _, tt := range tests { + t.Run(tt.status+"/default_user_public", func(t *testing.T) { + c := samplePublicCluster() + c.Status = tt.status + c.Name = name + response, _ := publicToLegacyClusterResponse(c) + + credCalls := 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + + return flaps.ManagedPostgresUserCredentials{Username: "fly-user", Password: "p"}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, &response, false, "") + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + assert.Nil(t, credentials) + require.Equal(t, 0, credCalls, "refusal must short-circuit before any credentials call (default user, public path)") + + return + } + require.NoError(t, err) + require.Equal(t, 1, credCalls, "proceed must hit the credentials endpoint exactly once") + require.NotNil(t, credentials) + }) + + t.Run(tt.status+"/explicit_user_public", func(t *testing.T) { + c := samplePublicCluster() + c.Status = tt.status + c.Name = name + response, _ := publicToLegacyClusterResponse(c) + + credCalls := 0 + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + credCalls++ + + return flaps.ManagedPostgresUserCredentials{Username: "alice", Password: "p"}, nil + }, + }) + + credentials, err := resolveConnectCredentials(ctx, &response, false, "alice") + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + assert.Nil(t, credentials) + require.Equal(t, 0, credCalls, "refusal must short-circuit before any credentials call (explicit user, public path)") + + return + } + require.NoError(t, err) + require.Equal(t, 1, credCalls, "proceed must hit the credentials endpoint exactly once") + require.NotNil(t, credentials) + }) + } +} + +// TestProxyParamsPublicNonDefaultPort proves that the public path dials the +// actual advertised port from Endpoints.Primary.Direct.Port instead of the +// legacy hardcoded 5432. The legacy path is also pinned to 5432 here so the +// regression surface is locked down on both sides. The public-zero case is +// pinned to an error so a genuinely-advertised port 0 cannot be silently +// treated like the legacy "no port field" default. +func TestProxyParamsPublicNonDefaultPort(t *testing.T) { + t.Run("public port 5433 dials 5433, not 5432", func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = 5433 + response, port := publicToLegacyClusterResponse(c) + + _, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, "10.0.0.1", params.RemoteHost) + require.Equal(t, []string{"16380", "5433"}, params.Ports) + }) + + t.Run("public port 5432 stays 5432 (no-op for the default)", func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = 5432 + response, port := publicToLegacyClusterResponse(c) + + _, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, []string{"16380", "5432"}, params.Ports) + }) + + t.Run("legacy port stays hardcoded 5432", func(t *testing.T) { + response := sampleLegacyCluster() + // legacy path never calls publicToLegacyClusterResponse, so its port + // value is always nil — the proxyParams fallback kicks in and "5432" + // is used exactly as before. + _, params, err := proxyParams(&response, nil, "16380", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, []string{"16380", "5432"}, params.Ports) + }) + + t.Run("public port 0 surfaces as an error instead of silently dialing 5432", func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = 0 + response, port := publicToLegacyClusterResponse(c) + // Sanity check: the adapter still produces a non-nil pointer (so the + // public-vs-legacy distinction is preserved). + require.NotNil(t, port) + require.Equal(t, 0, *port) + + cluster, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) + require.EqualError(t, err, "error getting cluster port") + assert.Nil(t, cluster) + assert.Nil(t, params) + }) +} From 6509813ed1bc3222dce421b39249d82aa719d28a Mon Sep 17 00:00:00 2001 From: vincent Date: Fri, 4 Sep 2026 19:19:19 -0400 Subject: [PATCH 2/3] mpg: preserve connect and proxy behavior on the public API Remove the public cluster-status gate so connect and proxy continue to depend on their actual prerequisites, matching the previous private ui-ex routes. Preserve the historical initializing error when the public default-user credentials endpoint returns a classified 404. Keep explicit-user and non-404 failures authoritative, and retain 404-only fallback for cluster lookup. Tighten the v1 and v2 tests around the production call graph, credential routing, endpoint handling, and proxy independence from credentials. --- internal/command/mpg/v1/run_connect.go | 27 +- internal/command/mpg/v1/run_connect_test.go | 99 +----- internal/command/mpg/v1/run_proxy.go | 239 ++------------ internal/command/mpg/v1/run_proxy_test.go | 335 ++++++++------------ internal/command/mpg/v2/run_connect.go | 27 +- internal/command/mpg/v2/run_connect_test.go | 99 +----- internal/command/mpg/v2/run_proxy.go | 239 ++------------ internal/command/mpg/v2/run_proxy_test.go | 335 ++++++++------------ 8 files changed, 350 insertions(+), 1050 deletions(-) diff --git a/internal/command/mpg/v1/run_connect.go b/internal/command/mpg/v1/run_connect.go index d90351b4a6..66f599b5d5 100644 --- a/internal/command/mpg/v1/run_connect.go +++ b/internal/command/mpg/v1/run_connect.go @@ -78,13 +78,12 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string) ( } } - cluster, useLegacy, params, credentials, err := GetMpgConnectParams(ctx, localProxyPort, username, clusterID, resolvedOrgSlug) + cluster, params, credentials, err := GetMpgConnectParams(ctx, localProxyPort, username, clusterID, resolvedOrgSlug) if err != nil { return err } - // Gated on useLegacy; see maybeWarnLegacyNotReady's doc comment for why. - maybeWarnLegacyNotReady(io.ErrOut, useLegacy, cluster) + maybeWarnNotReady(io.ErrOut, cluster) psqlPath, err := exec.LookPath("psql") if err != nil { @@ -167,12 +166,7 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string) ( return err } -// buildConnectURL composes the psql connection URL from resolved credentials -// and the proxy port. db follows the priority used by RunConnect: an explicit -// --database value (or interactive prompt result) wins over credentials.DBName. -// credentials.DBName is the plan-required default ("fly-db") on both the -// public default-user and public explicit-user paths, so a non-interactive -// `fly mpg connect --user alice` lands on postgresql://.../fly-db. +// buildConnectURL prefers the selected database over the credential default. func buildConnectURL(credentials *mpgv1.GetManagedClusterCredentialsResponse, db string, localProxyPort string) string { if db == "" { db = credentials.DBName @@ -181,18 +175,9 @@ func buildConnectURL(credentials *mpgv1.GetManagedClusterCredentialsResponse, db return fmt.Sprintf("postgresql://%s:%s@localhost:%s/%s", credentials.User, credentials.Password, localProxyPort, db) } -// maybeWarnLegacyNotReady restores the pre-migration legacy-path "Cluster -// is not in ready state" stderr warning. It is gated on useLegacy so the -// public path (which already refuses non-ready clusters via -// connectStatusRefusal) stays silent — see connectStatusRefusal's doc -// comment for the legacy/public status-split rationale. The warning -// format is preserved verbatim from the pre-migration code -// (commit 81f75427b^): aurora.Yellow("WARN") + " Cluster is not in ready -// state, currently: \n". The function is a thin side-effecting -// helper so it can be unit-tested with a bytes.Buffer without involving -// the agent/establish code path or RunConnect's exec/psql machinery. -func maybeWarnLegacyNotReady(errOut io.Writer, useLegacy bool, cluster *mpgv1.ManagedCluster) { - if !useLegacy || cluster == nil || cluster.Status == "ready" { +// maybeWarnNotReady warns when a cluster is not in ready state. +func maybeWarnNotReady(errOut io.Writer, cluster *mpgv1.ManagedCluster) { + if cluster == nil || cluster.Status == "ready" { return } diff --git a/internal/command/mpg/v1/run_connect_test.go b/internal/command/mpg/v1/run_connect_test.go index c996e7eb3f..9fda1fdf01 100644 --- a/internal/command/mpg/v1/run_connect_test.go +++ b/internal/command/mpg/v1/run_connect_test.go @@ -8,114 +8,33 @@ import ( mpgv1 "github.com/superfly/flyctl/internal/uiex/mpg/v1" ) -// TestBuildConnectURLPublicExplicitUserDefaultsToFlyDB pins the public -// explicit-user connect path so it can never regress to the bad -// postgresql://.../ helper output. Credentials are resolved the same way the -// public branch of resolveConnectCredentials does, then handed straight to -// buildConnectURL the same way RunConnect does. -func TestBuildConnectURLPublicExplicitUserDefaultsToFlyDB(t *testing.T) { - creds := &mpgv1.GetManagedClusterCredentialsResponse{ - User: "alice", - Password: "a", - DBName: "fly-db", // mirrors the fixed public explicit-user branch. - } - - got := buildConnectURL(creds, "", "16380") - require.Equal(t, "postgresql://alice:a@localhost:16380/fly-db", got) -} - -// TestBuildConnectURLRespectsExplicitDatabase verifies the priority order: -// an explicit --database value wins over credentials.DBName. -func TestBuildConnectURLRespectsExplicitDatabase(t *testing.T) { - creds := &mpgv1.GetManagedClusterCredentialsResponse{ - User: "alice", - Password: "a", - DBName: "fly-db", - } - - got := buildConnectURL(creds, "app-db", "16380") - require.Equal(t, "postgresql://alice:a@localhost:16380/app-db", got) -} - -// TestBuildConnectURLEmptyDBNameFallsThrough is a guard for the historical -// bug shape: if the explicit-user public branch ever returns "" again, the -// URL will not silently land on postgresql://.../ (empty path). It will land -// on postgresql://.../ with the user-controlled --database fallback in -// RunConnect still preferring the flag value when present; without that -// fallback the URL here is malformed, which surfaces in tests rather than -// silently connecting to the wrong database. -func TestBuildConnectURLEmptyDBNameFallsThrough(t *testing.T) { - creds := &mpgv1.GetManagedClusterCredentialsResponse{ - User: "alice", - Password: "a", - DBName: "", - } - - got := buildConnectURL(creds, "", "16380") - require.Equal(t, "postgresql://alice:a@localhost:16380/", got) -} - -// TestMaybeWarnLegacyNotReady pins the pre-migration warning format and -// the useLegacy/status gate. See maybeWarnLegacyNotReady's doc comment -// for the gate rationale and the warning-text provenance. -func TestMaybeWarnLegacyNotReady(t *testing.T) { +func TestMaybeWarnNotReady(t *testing.T) { const name = "test-cluster" tests := []struct { - name string - useLegacy bool - status string - wantWarn bool + name string + status string + wantWarn bool }{ - // Public path (useLegacy == false). connectStatusRefusal refuses - // non-ready public-path clusters upstream, so the useLegacy - // gate is the only thing that matters here. - {name: "public+creating silent", useLegacy: false, status: "creating", wantWarn: false}, - - // Legacy path (useLegacy == true). "ready" is silent (the - // status gate short-circuits before the fprintf); "creating" - // is the pre-migration non-refused case. The legacy-only - // "error" status is real on the legacy path but rejected by - // the public classifier — pinned here so a future migration - // does not silently drop it. - {name: "legacy+ready silent", useLegacy: true, status: "ready", wantWarn: false}, - {name: "legacy+creating warns", useLegacy: true, status: "creating", wantWarn: true}, - {name: "legacy+error warns", useLegacy: true, status: "error", wantWarn: true}, + {name: "ready silent", status: "ready", wantWarn: false}, + {name: "creating warns", status: "creating", wantWarn: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var buf bytes.Buffer cluster := &mpgv1.ManagedCluster{Name: name, Status: tt.status} - maybeWarnLegacyNotReady(&buf, tt.useLegacy, cluster) + maybeWarnNotReady(&buf, cluster) got := buf.String() if !tt.wantWarn { - require.Empty(t, got, "no warning expected for useLegacy=%v status=%q", tt.useLegacy, tt.status) + require.Empty(t, got, "no warning expected for status=%q", tt.status) return } - // Robust match against the pre-migration rendered text. We - // assert on "WARN" (the literal aurora payload when not a - // TTY) and on the exact "currently: " interpolation, - // rather than asserting the aurora-wrapped string verbatim, - // so this stays green regardless of the test harness's color - // configuration. + // Check the warning text independently of ANSI color codes. require.Contains(t, got, "WARN", "warning must contain the literal 'WARN' marker") require.Contains(t, got, "Cluster is not in ready state, currently: "+tt.status) require.True(t, bytes.HasSuffix(buf.Bytes(), []byte("\n")), "warning must end with a newline (pre-migration format)") }) } } - -// TestMaybeWarnLegacyNotReadyNilCluster pins the defensive nil-cluster -// short-circuit. RunConnect only calls the helper with the cluster -// returned by GetMpgConnectParams, which is never nil on the success -// path, but the helper is public and the nil guard prevents a panic if -// the contract ever loosens. -func TestMaybeWarnLegacyNotReadyNilCluster(t *testing.T) { - var buf bytes.Buffer - require.NotPanics(t, func() { - maybeWarnLegacyNotReady(&buf, true, nil) - }) - require.Empty(t, buf.String()) -} diff --git a/internal/command/mpg/v1/run_proxy.go b/internal/command/mpg/v1/run_proxy.go index 44d28c5451..d93ab65eae 100644 --- a/internal/command/mpg/v1/run_proxy.go +++ b/internal/command/mpg/v1/run_proxy.go @@ -12,20 +12,12 @@ import ( "github.com/superfly/flyctl/internal/flag" "github.com/superfly/flyctl/internal/flapsutil" "github.com/superfly/flyctl/internal/flyutil" + "github.com/superfly/flyctl/internal/mpgutil" "github.com/superfly/flyctl/internal/uiex/mpg" mpgv1 "github.com/superfly/flyctl/internal/uiex/mpg/v1" "github.com/superfly/flyctl/proxy" ) -// defaultMPGUser is the literal username used when resolving the default -// fly-user on the public Machines API for Connect. Verified against the -// orchestrator and ui-ex repos. -const defaultMPGUser = "fly-user" - -// defaultMPGDatabase is the literal database name used when no --database -// flag is given and no interactive prompt answer is available. -const defaultMPGDatabase = "fly-db" - func RunProxy(ctx context.Context, clusterID string, resolvedOrgSlug string, proxyPort string) error { _, params, err := GetMpgProxyParams(ctx, proxyPort, clusterID, resolvedOrgSlug) if err != nil { @@ -57,89 +49,59 @@ func GetMpgProxyParams( return cluster, params, nil } -// GetMpgConnectParams builds proxy connection parameters and resolves the -// database credentials needed by fly mpg connect. -// -// The returned useLegacy bool mirrors the value computed inside getCluster -// (true when the public Machines API returned a classified 404 and we fell -// back to the legacy ui-ex client; false when the public API succeeded). -// RunConnect uses it to gate the pre-migration "Cluster is not in ready -// state" stderr warning, which is meaningful only on the legacy path — -// see connectStatusRefusal's doc comment for why the public path does not -// need it. +// GetMpgConnectParams resolves credentials and proxy parameters. func GetMpgConnectParams( ctx context.Context, localProxyPort string, username string, clusterID string, resolvedOrgSlug string, -) (*mpgv1.ManagedCluster, bool, *proxy.ConnectParams, *mpgv1.GetManagedClusterCredentialsResponse, error) { +) (*mpgv1.ManagedCluster, *proxy.ConnectParams, *mpgv1.GetManagedClusterCredentialsResponse, error) { response, useLegacy, port, err := getCluster(ctx, clusterID) if err != nil { - return nil, false, nil, nil, err + return nil, nil, nil, err } credentials, err := resolveConnectCredentials(ctx, response, useLegacy, username) if err != nil { - return nil, false, nil, nil, err + return nil, nil, nil, err } cluster, params, err := buildProxyParams(ctx, response, port, localProxyPort, resolvedOrgSlug) if err != nil { - return nil, false, nil, nil, err + return nil, nil, nil, err } - return cluster, useLegacy, params, credentials, nil + return cluster, params, credentials, nil } -// getCluster retrieves cluster details via the public Machines API, falling -// back to the legacy MPGv1 client only when the public call returns a -// classified 404. Any other non-nil public error is returned immediately, -// wrapped in "failed retrieving cluster %s: %w". The returned useLegacy flag -// indicates which client -// produced the response so downstream credential resolution can choose the -// appropriate code path. The returned port is the public API's advertised -// direct-endpoint port (nil on the legacy path, which has no port field) — -// carried as a separate return value rather than a field on the legacy -// mpgv1.ManagedCluster type, since that type is part of the private/legacy -// client package (internal/uiex/mpg) that this migration must not modify. -func getCluster(ctx context.Context, clusterID string) (*mpgv1.GetManagedClusterResponse, bool, *int, error) { +// getCluster tries the public API, falling back to the legacy client only on 404. +// It returns the credential source and direct endpoint port (5432 for legacy). +func getCluster(ctx context.Context, clusterID string) (*mpgv1.GetManagedClusterResponse, bool, int, error) { flapsClient := flapsutil.ClientFromContext(ctx) publicCluster, err := flapsClient.GetManagedPostgresCluster(ctx, clusterID) if err == nil { response, port := publicToLegacyClusterResponse(publicCluster) + return &response, false, port, nil } if !errors.Is(err, flaps.ErrFlapsNotFound) { - return nil, false, nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) + return nil, false, 0, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) } legacyClient := mpgv1.ClientFromContext(ctx) response, err := legacyClient.GetManagedClusterById(ctx, clusterID) if err != nil { - return nil, true, nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) + return nil, true, 0, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) } - return &response, true, nil, nil + return &response, true, mpgutil.DefaultPort, nil } -// publicToLegacyClusterResponse wraps the public Machines API cluster in the -// legacy ui-ex envelope so that downstream callers (proxyParams, -// resolveConnectCredentials) can read IpAssignments.Direct and Status without -// translation. The legacy shape preserves the bare-address column for -// proxyParams.RemoteHost and the Status field for connectStatusRefusal. -// -// The public API's advertised Endpoints.Primary.Direct.Port is returned -// separately (not as a field on the legacy mpgv1.ManagedCluster type) so -// proxyParams can dial the real advertised port instead of the legacy -// hardcoded 5432, without adding a public-API-only field to the private -// legacy client package. A nil port return means "no port info" (the legacy -// path never calls this function at all, so in practice this only happens -// if ever called with a zero-value input); a non-nil pointer — even to 0 — -// means the public path returned some value, and *0 is treated as invalid -// by proxyParams. -func publicToLegacyClusterResponse(c flaps.ManagedPostgresCluster) (mpgv1.GetManagedClusterResponse, *int) { +// publicToLegacyClusterResponse adapts the public cluster to the legacy shape. +// The advertised direct port is returned unchanged for validation. +func publicToLegacyClusterResponse(c flaps.ManagedPostgresCluster) (mpgv1.GetManagedClusterResponse, int) { port := c.Endpoints.Primary.Direct.Port return mpgv1.GetManagedClusterResponse{ @@ -154,44 +116,17 @@ func publicToLegacyClusterResponse(c flaps.ManagedPostgresCluster) (mpgv1.GetMan Organization: fly.Organization{Name: c.Organization.Name, Slug: c.Organization.Slug}, IpAssignments: mpg.ManagedClusterIpAssignments{Direct: c.Endpoints.Primary.Direct.Host}, }, - }, &port + }, port } -// resolveConnectCredentials returns the credentials used by RunConnect. -// useLegacy=true routes through the legacy ui-ex client; useLegacy=false -// routes through the public Machines API's -// GetManagedPostgresUserCredentials. The default user (no --user flag) -// resolves to defaultMPGUser ("fly-user"); explicit users pass the flag -// value straight through. The public path has no envelope-level DBName, -// so both public branches fall back to defaultMPGDatabase ("fly-db") — -// which run_connect.go's buildConnectURL uses to land on the plan-required -// default when neither --database nor an interactive prompt supplies one. -// -// Status gating is split across the two paths because their vocabularies -// differ; see connectStatusRefusal's doc comment for the full rationale. -// Briefly: the public path consults response.Data.Status BEFORE any -// credentials call (a 7-value enum + fail-closed default); the legacy -// path keeps the pre-migration post-fetch credentials.Status + -// credentials.Password check ("error" is a real legacy value but not a -// public one, and the legacy envelope's Status field is semantically -// distinct from cluster status). +// resolveConnectCredentials uses the same API as the cluster lookup. +// Public credentials default to fly-user and fly-db. func resolveConnectCredentials( ctx context.Context, response *mpgv1.GetManagedClusterResponse, useLegacy bool, username string, ) (*mpgv1.GetManagedClusterCredentialsResponse, error) { - // Status classifier runs BEFORE any credentials call on the public - // path only — see connectStatusRefusal's doc comment for the - // legacy/public split. response.Data.Status is populated on the public - // path via publicToLegacyClusterResponse (from c.Status); it is - // intentionally NOT consulted on the legacy path. - if !useLegacy { - if err := connectStatusRefusal(response.Data.Name, response.Data.Status); err != nil { - return nil, err - } - } - var credentials mpgv1.GetManagedClusterCredentialsResponse switch { @@ -217,32 +152,30 @@ func resolveConnectCredentials( credentials = mpgv1.GetManagedClusterCredentialsResponse{ User: userCreds.Username, Password: userCreds.Password, - DBName: defaultMPGDatabase, + DBName: mpgutil.DefaultDatabase, } case useLegacy: credentials = response.Credentials default: flapsClient := flapsutil.ClientFromContext(ctx) - userCreds, err := flapsClient.GetManagedPostgresUserCredentials(ctx, response.Data.Id, defaultMPGUser) + userCreds, err := flapsClient.GetManagedPostgresUserCredentials(ctx, response.Data.Id, mpgutil.DefaultUsername) if err != nil { - return nil, fmt.Errorf("failed retrieving credentials for user %s: %w", defaultMPGUser, err) + if errors.Is(err, flaps.ErrFlapsNotFound) { + return nil, fmt.Errorf("cluster is still initializing, wait a bit more") + } + + return nil, fmt.Errorf("failed retrieving credentials for user %s: %w", mpgutil.DefaultUsername, err) } credentials = mpgv1.GetManagedClusterCredentialsResponse{ User: userCreds.Username, Password: userCreds.Password, - DBName: defaultMPGDatabase, + DBName: mpgutil.DefaultDatabase, } } if useLegacy { - // ORIGINAL pre-migration legacy status/password checks, - // restored verbatim. credentials.Status is only checked on the - // default-user path because the explicit-user legacy - // GetUserCredentials response does not populate that field - // (only User/Password/DBName). See connectStatusRefusal's doc - // comment for why this path is intentionally distinct from the - // public classifier. + // Only legacy default-user credentials include a status. if username == "" { if credentials.Status == "initializing" { return nil, fmt.Errorf("cluster is still initializing, wait a bit more") @@ -255,13 +188,6 @@ func resolveConnectCredentials( return nil, fmt.Errorf("error getting user password") } } else if credentials.Password == "" { - // Public path: empty-password fallback after the credentials - // call. The wording tracks whether the user was explicit - // (flag/prompt) or defaulted to the plan-required fly-user, - // preserving the existing pre-fix messages. The public path - // has no envelope-level Status to inspect post-fetch, so the - // pre-credential connectStatusRefusal call above is the only - // status gate. if username == "" { return nil, fmt.Errorf("error getting cluster password") } @@ -272,90 +198,10 @@ func resolveConnectCredentials( return &credentials, nil } -// connectStatusRefusal is the explicit status classifier for fly mpg -// connect. It returns nil when the cluster's status permits a connect -// attempt and a deliberate, status-specific refusal error otherwise. The -// complete emitted set of "status" values from a successful public MPG -// cluster lookup (traced across the mpg / nomad-firecracker / ui-ex code -// paths this session) is: -// -// ready, standby_ready, creating, deleting, failed, deleted, initializing -// -// "error" is NOT a real public cluster status value; it does not appear -// in the public enum and is rejected if encountered. Behavior per status: -// -// - ready: proceed, no message. -// - standby_ready: refuse — a standby replica is not a connect target. -// Fail-closed pending a product decision on standby -// semantics; not even a "warn and continue" — refused -// outright. -// - creating: refuse — friendly wording distinct from initializing -// since the cluster is in the process of coming up, -// not stuck mid-provision. -// - deleting: refuse — cluster is going away, connections -// meaningless. -// - deleted: refuse — terminal state, no connect possible. -// - failed: refuse — accurate wording ("cluster is in a failed -// state"), not the misleading "error getting cluster -// password" misdiagnosis. -// - initializing: refuse — neutral wording because this now covers -// many underlying states (degraded, updating, -// resizing, credential rotation, promotion, and -// future unmapped states), not just fresh -// provisioning. Do not over-promise "wait a bit more" -// for a state that may not be purely transient. -// - default: refuse — unknown / unrecognized statuses fail -// closed with the actual status string quoted so the -// user (and on-call) can see what the public API -// actually returned. Do NOT let unknown values fall -// through to credential resolution. -// -// The classifier applies ONLY to the public path (useLegacy == false), -// BEFORE any public credential call, on both the default-user and -// explicit-user paths. It is intentionally NOT applied to the legacy -// path (useLegacy == true): the legacy status vocabulary differs from -// the public API's — e.g. "error" is a real legacy cluster status value -// but not a public one — and the pre-migration legacy code understood it -// natively via credentials.Status (the legacy credentials envelope's own -// status field, populated post-fetch and semantically distinct from -// response.Data.Status). Applying this public-only classifier to legacy -// responses would mis-handle legacy-only statuses as "unrecognized -// state" and silently drop the credentials.Status check. -// -// The classifier is NOT applied to fly mpg proxy — that command -// deliberately does not gate on status because the raw TCP proxy never -// uses credentials, only the direct IP/port; see RunProxy / -// GetMpgProxyParams. Adding a status gate there would reintroduce the -// exact coupling flyctl#5048 was built to remove. -// -// name is the cluster identifier included in the refusal message so the -// user knows which cluster the refusal applies to. It comes from -// response.Data.Name (populated on both the public and legacy paths). -func connectStatusRefusal(name, status string) error { - switch status { - case "ready": - return nil - case "standby_ready": - return fmt.Errorf("cluster %s is a standby replica and cannot be used with fly mpg connect", name) - case "creating": - return fmt.Errorf("cluster %s is still being created, wait a bit more", name) - case "deleting": - return fmt.Errorf("cluster %s is being deleted and cannot be connected to", name) - case "deleted": - return fmt.Errorf("cluster %s has been deleted and cannot be connected to", name) - case "failed": - return fmt.Errorf("cluster %s is in a failed state", name) - case "initializing": - return fmt.Errorf("cluster %s is not currently ready for connections (status: initializing)", name) - default: - return fmt.Errorf("cluster %s is in an unrecognized state (%q) and cannot be connected to", name, status) - } -} - func buildProxyParams( ctx context.Context, response *mpgv1.GetManagedClusterResponse, - port *int, + port int, localProxyPort string, resolvedOrgSlug string, ) (*mpgv1.ManagedCluster, *proxy.ConnectParams, error) { @@ -384,7 +230,7 @@ func buildProxyParams( func proxyParams( response *mpgv1.GetManagedClusterResponse, - port *int, + port int, localProxyPort string, resolvedOrgSlug string, bindAddr string, @@ -395,29 +241,12 @@ func proxyParams( return nil, nil, fmt.Errorf("error getting cluster IP") } - // remotePort is the port the proxy will dial on the remote host. The - // legacy path has no port field at all, so it always calls this with - // port == nil and we fall back to "5432" exactly as before. The public - // path passes the advertised Endpoints.Primary.Direct.Port through - // getCluster so a non-5432 public port dials that real port instead of - // being silently rewritten to 5432. A public API that advertises port 0 - // is treated as an invalid/unexpected value and surfaces as an error — - // distinguishing it from the legacy "no port field" case, where 5432 is - // the historical default and the safe fallback. The *int rather than a - // separate bool encodes the "optional value" semantic directly: nil = - // no port info (legacy), non-nil (even *port == 0) = public path - // advertised some value. - remotePort := "5432" - if port != nil { - if *port == 0 { - return nil, nil, fmt.Errorf("error getting cluster port") - } - - remotePort = strconv.Itoa(*port) + if port < 1 || port > 65535 { + return nil, nil, fmt.Errorf("invalid cluster port %d: must be between 1 and 65535", port) } return cluster, &proxy.ConnectParams{ - Ports: []string{localProxyPort, remotePort}, + Ports: []string{localProxyPort, strconv.Itoa(port)}, OrganizationSlug: resolvedOrgSlug, Dialer: dialer, BindAddr: bindAddr, diff --git a/internal/command/mpg/v1/run_proxy_test.go b/internal/command/mpg/v1/run_proxy_test.go index 5067f29c62..a37f155323 100644 --- a/internal/command/mpg/v1/run_proxy_test.go +++ b/internal/command/mpg/v1/run_proxy_test.go @@ -3,15 +3,20 @@ package cmdv1 import ( "context" "errors" + "fmt" "net" + "strconv" "testing" + "github.com/spf13/pflag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" fly "github.com/superfly/fly-go" "github.com/superfly/fly-go/flaps" + "github.com/superfly/flyctl/internal/flag" "github.com/superfly/flyctl/internal/flapsutil" "github.com/superfly/flyctl/internal/mock" + "github.com/superfly/flyctl/internal/mpgutil" "github.com/superfly/flyctl/internal/uiex/mpg" mpgv1 "github.com/superfly/flyctl/internal/uiex/mpg/v1" "github.com/superfly/flyctl/wg" @@ -56,7 +61,7 @@ func TestProxyParamsIgnoreCredentials(t *testing.T) { } dialer := &testDialer{} - cluster, params, err := proxyParams(&response, nil, "15432", "test-org", "127.0.0.2", dialer) + cluster, params, err := proxyParams(&response, mpgutil.DefaultPort, "15432", "test-org", "127.0.0.2", dialer) require.NoError(t, err) assert.Same(t, &response.Data, cluster) assert.Equal(t, []string{"15432", "5432"}, params.Ports) @@ -69,14 +74,13 @@ func TestProxyParamsIgnoreCredentials(t *testing.T) { } func TestProxyParamsRequireDirectIP(t *testing.T) { - cluster, params, err := proxyParams(&mpgv1.GetManagedClusterResponse{}, nil, "15432", "test-org", "127.0.0.1", nil) + cluster, params, err := proxyParams(&mpgv1.GetManagedClusterResponse{}, 0, "15432", "test-org", "127.0.0.1", nil) require.EqualError(t, err, "error getting cluster IP") assert.Nil(t, cluster) assert.Nil(t, params) } -// samplePublicCluster is a representative public Machines API cluster payload. func samplePublicCluster() flaps.ManagedPostgresCluster { return flaps.ManagedPostgresCluster{ ID: "mpg-123", @@ -99,8 +103,6 @@ func samplePublicCluster() flaps.ManagedPostgresCluster { } } -// sampleLegacyCluster is a representative legacy MPGv1 cluster payload. -// Direct is a bare address (no port) — the historical shape of this column. func sampleLegacyCluster() mpgv1.GetManagedClusterResponse { return mpgv1.GetManagedClusterResponse{ Data: mpgv1.ManagedCluster{ @@ -151,9 +153,12 @@ func TestGetCluster(t *testing.T) { wantLegacyCalls: 0, }, { - name: "classified 404 falls back to legacy", - publicCluster: flaps.ManagedPostgresCluster{}, - publicErr: flaps.ErrFlapsNotFound, + name: "classified 404 falls back to legacy", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: fmt.Errorf("wrapped: %w", &flaps.FlapsError{ + ResponseStatusCode: 404, + OriginalError: errors.New("not found"), + }), legacyResponse: sampleLegacyCluster(), wantUseLegacy: true, wantDirectHost: "10.0.0.1", @@ -161,19 +166,29 @@ func TestGetCluster(t *testing.T) { wantLegacyCalls: 1, }, { - name: "classified 404 with legacy failure propagates legacy error", - publicCluster: flaps.ManagedPostgresCluster{}, - publicErr: flaps.ErrFlapsNotFound, + name: "classified 404 with legacy failure propagates legacy error", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: fmt.Errorf("wrapped: %w", &flaps.FlapsError{ + ResponseStatusCode: 404, + OriginalError: errors.New("not found"), + }), legacyResponse: mpgv1.GetManagedClusterResponse{}, legacyErr: errors.New("legacy denied"), wantErr: "failed retrieving cluster mpg-123: legacy denied", wantLegacyCalls: 1, }, { - name: "non-404 public error returns without fallback", + name: "403 public error returns without fallback", publicCluster: flaps.ManagedPostgresCluster{}, - publicErr: errors.New("boom"), - wantErr: "failed retrieving cluster mpg-123: boom", + publicErr: &flaps.FlapsError{ResponseStatusCode: 403, OriginalError: errors.New("denied")}, + wantErr: "failed retrieving cluster mpg-123: denied", + wantLegacyCalls: 0, + }, + { + name: "410 public error returns without fallback", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: &flaps.FlapsError{ResponseStatusCode: 410, OriginalError: errors.New("gone")}, + wantErr: "failed retrieving cluster mpg-123: gone", wantLegacyCalls: 0, }, } @@ -198,7 +213,7 @@ func TestGetCluster(t *testing.T) { }, }) - got, useLegacy, _, err := getCluster(ctx, "mpg-123") + got, useLegacy, port, err := getCluster(ctx, "mpg-123") if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) require.Nil(t, got) @@ -214,20 +229,26 @@ func TestGetCluster(t *testing.T) { require.Equal(t, tt.wantStatus, got.Data.Status) require.Equal(t, 1, publicCalls) require.Equal(t, tt.wantLegacyCalls, legacyCalls) + require.Equal(t, 5432, port) + if tt.wantUseLegacy { + _, params, err := proxyParams(got, port, "16380", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, []string{"16380", "5432"}, params.Ports) + } }) } } -// TestProxyParamsPublicNeverTouchesCredentials verifies the invariant that -// the proxy code path never resolves credentials: getCluster must only hit -// the cluster lookup endpoint, not the credentials endpoint, on the public- -// success branch. This pins RunProxy against accidentally growing a -// credentials dependency. -func TestProxyParamsPublicNeverTouchesCredentials(t *testing.T) { +func TestGetMpgProxyParamsPublicNeverTouchesCredentials(t *testing.T) { credCalls, legacyCredCalls := 0, 0 - ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + ctx := flag.NewContext(context.Background(), pflag.NewFlagSet("test", pflag.ContinueOnError)) + ctx = flapsutil.NewContextWithClient(ctx, &mock.FlapsClient{ GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { - return samplePublicCluster(), nil + cluster := samplePublicCluster() + cluster.Status = "creating" + cluster.Endpoints.Primary.Direct.Host = "" + + return cluster, nil }, GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { credCalls++ @@ -243,37 +264,33 @@ func TestProxyParamsPublicNeverTouchesCredentials(t *testing.T) { }, }) - response, useLegacy, port, err := getCluster(ctx, "mpg-123") - require.NoError(t, err) - require.NotNil(t, response) - require.False(t, useLegacy) - require.Equal(t, 0, credCalls, "getCluster (public-success path) must never resolve credentials") - require.Equal(t, 0, legacyCredCalls, "getCluster must never reach the legacy client on public success") - - // proxyParams does not take a ctx, so it cannot make any HTTP call; it is - // structurally incapable of leaking credentials. Verify it produces the - // expected bare-host RemoteHost from the public-converted response. The - // credCalls / legacyCredCalls counters were already asserted to be 0 - // above, immediately after getCluster returned, so proxyParams has - // nothing to regress there. - cluster, params, err := proxyParams(response, port, "15432", "test-org", "127.0.0.1", nil) - require.NoError(t, err) - require.NotNil(t, cluster) - require.NotNil(t, params) - require.Equal(t, "10.0.0.1", params.RemoteHost) + cluster, params, err := GetMpgProxyParams(ctx, "15432", "mpg-123", "test-org") + require.EqualError(t, err, "error getting cluster IP") + require.Nil(t, cluster) + require.Nil(t, params) + require.Zero(t, credCalls) + require.Zero(t, legacyCredCalls) +} + +func TestGetMpgConnectParamsResolvesCredentialsBeforeTunnel(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { + return samplePublicCluster(), nil + }, + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{Username: mpgutil.DefaultUsername}, nil + }, + }) + + cluster, params, credentials, err := GetMpgConnectParams(ctx, "15432", "", "mpg-123", "test-org") + require.EqualError(t, err, "error getting cluster password") + require.Nil(t, cluster) + require.Nil(t, params) + require.Nil(t, credentials) } func TestResolveDefaultConnectCredentials(t *testing.T) { - // The legacy default-user connect path (useLegacy == true) restores - // the pre-migration post-fetch logic: the legacy credentials - // envelope's own Status field (credentials.Status, semantically - // distinct from cluster status) is checked for "initializing"/"error", - // plus an empty-password fallback. These cases pin that behavior - // with the original error messages. The public status classifier is - // intentionally NOT applied here; that is covered by - // TestResolveConnectCredentialsPublicStatusClassifier below. See - // connectStatusRefusal's doc comment for the legacy/public status- - // split rationale. + // Legacy readiness comes from the credential envelope, not cluster status. const name = "test-cluster" tests := []struct { name string @@ -283,8 +300,6 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { err string }{ { - // ORIGINAL legacy: empty password is the empty-password - // fallback; status field is irrelevant when password is empty. name: "empty password", clusterSt: "ready", credStatus: "ready", @@ -292,13 +307,6 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { err: "error getting cluster password", }, { - // REGRESSION GUARD: Data.Status="ready" with credentials - // .Status="initializing" and a non-empty password MUST refuse - // on the legacy path. Applying the public-only 7-value - // status classifier to the legacy path (where it does not - // belong) would silently drop the credentials.Status check - // and let this case proceed to psql with possibly-invalid - // credentials. This is the regression this test pins. name: "ready cluster with stale initializing credentials refuses", clusterSt: "ready", credStatus: "initializing", @@ -306,11 +314,6 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { err: "cluster is still initializing, wait a bit more", }, { - // REGRESSION GUARD: Data.Status="ready" with credentials - // .Status="error" and a non-empty password MUST refuse on - // the legacy path. Same reasoning as above. ("error" is a - // real legacy status value but not a public one — the public - // classifier rejects it via its default arm.) name: "ready cluster with stale error credentials refuses", clusterSt: "ready", credStatus: "error", @@ -318,11 +321,6 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { err: "error getting cluster password", }, { - // The cluster status alone ("creating", "failed", etc.) is - // NOT consulted on the legacy path — only credentials.Status - // and credentials.Password are. So a "ready"-credentials - // envelope with a non-empty password proceeds regardless of - // the cluster status field. name: "creating cluster with ready credentials proceeds", clusterSt: "creating", credStatus: "ready", @@ -346,6 +344,7 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { if tt.err == "" { require.NoError(t, err) require.NotNil(t, credentials) + return } require.EqualError(t, err, tt.err) @@ -362,6 +361,7 @@ func TestResolveExplicitUserConnectCredentials(t *testing.T) { Status: "ready", }, Credentials: mpgv1.GetManagedClusterCredentialsResponse{ + Status: "initializing", DBName: "default-db", }, } @@ -424,15 +424,6 @@ func TestResolveExplicitUserConnectCredentialsErrors(t *testing.T) { }) } -// TestResolveConnectCredentialsPublic covers the public-API code paths for -// Connect credentials: default-user goes through -// GetManagedPostgresUserCredentials("fly-user") and DBName defaults to -// "fly-db"; explicit-user passes the flag value straight through. Data.Status -// is set to "ready" on every response here so the status classifier (which -// runs before any credentials call) does not interfere — see -// TestResolveConnectCredentialsPublicStatusClassifier for the dedicated -// status-classifier coverage. See connectStatusRefusal's doc comment for -// the legacy/public status-split rationale. func TestResolveConnectCredentialsPublic(t *testing.T) { response := &mpgv1.GetManagedClusterResponse{ Data: mpgv1.ManagedCluster{Id: "cluster-id", Name: "test-cluster", Status: "ready"}, @@ -475,11 +466,9 @@ func TestResolveConnectCredentialsPublic(t *testing.T) { require.Equal(t, 1, credCalls) require.Equal(t, "alice", credentials.User) require.Equal(t, "a", credentials.Password) - // The public path has no envelope DBName; explicit users fall back to - // defaultMPGDatabase ("fly-db") so that run_connect.go's psql URL - // construction lands on the plan-required default when neither - // --database nor an interactive prompt supplies one. require.Equal(t, "fly-db", credentials.DBName) + require.Equal(t, "postgresql://alice:a@localhost:16380/fly-db", buildConnectURL(credentials, "", "16380")) + require.Equal(t, "postgresql://alice:a@localhost:16380/app-db", buildConnectURL(credentials, "app-db", "16380")) }) t.Run("default user empty password mirrors legacy error", func(t *testing.T) { @@ -518,77 +507,38 @@ func TestResolveConnectCredentialsPublic(t *testing.T) { assert.Nil(t, credentials) }) - t.Run("explicit user public credentials error propagates", func(t *testing.T) { + t.Run("default user 404 preserves initializing error", func(t *testing.T) { ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { - return flaps.ManagedPostgresUserCredentials{}, errors.New("missing user") + return flaps.ManagedPostgresUserCredentials{}, fmt.Errorf("wrapped: %w", &flaps.FlapsError{ResponseStatusCode: 404, OriginalError: errors.New("not found")}) }, }) - credentials, err := resolveConnectCredentials(ctx, response, false, "alice") - require.EqualError(t, err, "failed retrieving credentials for user alice: missing user") + credentials, err := resolveConnectCredentials(ctx, response, false, "") + require.EqualError(t, err, "cluster is still initializing, wait a bit more") assert.Nil(t, credentials) }) -} -// TestPublicToLegacyClusterResponse verifies that the public-API cluster is -// converted to the legacy ui-ex shape, preserving the bare-address column for -// proxyParams and the Status field for downstream status checks -// (connectStatusRefusal / maybeWarnLegacyNotReady). -func TestPublicToLegacyClusterResponse(t *testing.T) { - got, _ := publicToLegacyClusterResponse(samplePublicCluster()) - require.Equal(t, "mpg-123", got.Data.Id) - require.Equal(t, "test-cluster", got.Data.Name) - require.Equal(t, "ready", got.Data.Status) - require.Equal(t, "ord", got.Data.Region) - require.Equal(t, "development", got.Data.Plan) - require.Equal(t, 10, got.Data.Disk) - require.Equal(t, 1, got.Data.Replicas) - require.Equal(t, "Test Org", got.Data.Organization.Name) - require.Equal(t, "test-org", got.Data.Organization.Slug) - require.Equal(t, "10.0.0.1", got.Data.IpAssignments.Direct) + t.Run("explicit user 404 remains a user error", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{}, fmt.Errorf("wrapped: %w", &flaps.FlapsError{ResponseStatusCode: 404, OriginalError: errors.New("missing user")}) + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "alice") + require.EqualError(t, err, "failed retrieving credentials for user alice: wrapped: missing user") + assert.Nil(t, credentials) + }) } -// TestResolveConnectCredentialsPublicStatusClassifier proves the public-path -// status classifier short-circuits BEFORE any credentials resolution call, -// for both the default-user and explicit-user connect paths. It exercises -// connectStatusRefusal (via resolveConnectCredentials) for the full 9-value -// status matrix — the 7 documented public statuses plus the "error" / -// "degraded" unrecognized sentinels — and asserts on each: -// -// - the exact deliberate refusal message, with the cluster name -// interpolated; -// - the credentials endpoint is never called (credCalls == 0). -// -// On the proceed status ("ready") it asserts: -// -// - no refusal error; -// - the credentials endpoint IS called exactly once (credCalls == 1). -// -// The classifier applies ONLY to the public path (useLegacy == false). The -// legacy path (useLegacy == true) is intentionally NOT exercised here — it -// uses its original pre-migration credentials.Status / credentials.Password -// post-fetch logic instead, pinned by TestResolveDefaultConnectCredentials -// above. -func TestResolveConnectCredentialsPublicStatusClassifier(t *testing.T) { +func TestResolveConnectCredentialsPublicNonReady(t *testing.T) { const name = "test-cluster" - const unknownStatus = "degraded" tests := []struct { - status string - wantErr string // expected refusal error message; "" means proceed. + status string }{ - {"ready", ""}, - {"standby_ready", "cluster " + name + " is a standby replica and cannot be used with fly mpg connect"}, - {"creating", "cluster " + name + " is still being created, wait a bit more"}, - {"deleting", "cluster " + name + " is being deleted and cannot be connected to"}, - {"deleted", "cluster " + name + " has been deleted and cannot be connected to"}, - {"failed", "cluster " + name + " is in a failed state"}, - {"initializing", "cluster " + name + " is not currently ready for connections (status: initializing)"}, - // Sentinel: "error" is NOT a real public cluster status — it is - // rejected by the default arm of the classifier. - {"error", `cluster ` + name + ` is in an unrecognized state ("error") and cannot be connected to`}, - // Sentinel: an arbitrary future / unmapped status also fails closed. - {unknownStatus, `cluster ` + name + ` is in an unrecognized state ("` + unknownStatus + `") and cannot be connected to`}, + {"creating"}, + {"degraded"}, } for _, tt := range tests { @@ -608,15 +558,8 @@ func TestResolveConnectCredentialsPublicStatusClassifier(t *testing.T) { }) credentials, err := resolveConnectCredentials(ctx, &response, false, "") - if tt.wantErr != "" { - require.EqualError(t, err, tt.wantErr) - assert.Nil(t, credentials) - require.Equal(t, 0, credCalls, "refusal must short-circuit before any credentials call (default user, public path)") - - return - } - require.NoError(t, err) - require.Equal(t, 1, credCalls, "proceed must hit the credentials endpoint exactly once") + require.NoError(t, err, "non-ready cluster must proceed to credential resolution") + require.Equal(t, 1, credCalls, "must hit the credentials endpoint exactly once") require.NotNil(t, credentials) }) @@ -636,70 +579,44 @@ func TestResolveConnectCredentialsPublicStatusClassifier(t *testing.T) { }) credentials, err := resolveConnectCredentials(ctx, &response, false, "alice") - if tt.wantErr != "" { - require.EqualError(t, err, tt.wantErr) - assert.Nil(t, credentials) - require.Equal(t, 0, credCalls, "refusal must short-circuit before any credentials call (explicit user, public path)") - - return - } - require.NoError(t, err) - require.Equal(t, 1, credCalls, "proceed must hit the credentials endpoint exactly once") + require.NoError(t, err, "non-ready cluster must proceed to credential resolution") + require.Equal(t, 1, credCalls, "must hit the credentials endpoint exactly once") require.NotNil(t, credentials) }) } } -// TestProxyParamsPublicNonDefaultPort proves that the public path dials the -// actual advertised port from Endpoints.Primary.Direct.Port instead of the -// legacy hardcoded 5432. The legacy path is also pinned to 5432 here so the -// regression surface is locked down on both sides. The public-zero case is -// pinned to an error so a genuinely-advertised port 0 cannot be silently -// treated like the legacy "no port field" default. -func TestProxyParamsPublicNonDefaultPort(t *testing.T) { - t.Run("public port 5433 dials 5433, not 5432", func(t *testing.T) { - c := samplePublicCluster() - c.Endpoints.Primary.Direct.Port = 5433 - response, port := publicToLegacyClusterResponse(c) - - _, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) - require.NoError(t, err) - require.Equal(t, "10.0.0.1", params.RemoteHost) - require.Equal(t, []string{"16380", "5433"}, params.Ports) - }) - - t.Run("public port 5432 stays 5432 (no-op for the default)", func(t *testing.T) { - c := samplePublicCluster() - c.Endpoints.Primary.Direct.Port = 5432 - response, port := publicToLegacyClusterResponse(c) - - _, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) - require.NoError(t, err) - require.Equal(t, []string{"16380", "5432"}, params.Ports) - }) +func TestProxyParamsPublicPorts(t *testing.T) { + for _, port := range []int{1, 5433, 65535} { + t.Run(strconv.Itoa(port), func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = port + response, advertisedPort := publicToLegacyClusterResponse(c) - t.Run("legacy port stays hardcoded 5432", func(t *testing.T) { - response := sampleLegacyCluster() - // legacy path never calls publicToLegacyClusterResponse, so its port - // value is always nil — the proxyParams fallback kicks in and "5432" - // is used exactly as before. - _, params, err := proxyParams(&response, nil, "16380", "test-org", "127.0.0.1", nil) - require.NoError(t, err) - require.Equal(t, []string{"16380", "5432"}, params.Ports) - }) + _, params, err := proxyParams(&response, advertisedPort, "0", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, "10.0.0.1", params.RemoteHost) + require.Equal(t, []string{"0", strconv.Itoa(port)}, params.Ports) + }) + } +} - t.Run("public port 0 surfaces as an error instead of silently dialing 5432", func(t *testing.T) { - c := samplePublicCluster() - c.Endpoints.Primary.Direct.Port = 0 - response, port := publicToLegacyClusterResponse(c) - // Sanity check: the adapter still produces a non-nil pointer (so the - // public-vs-legacy distinction is preserved). - require.NotNil(t, port) - require.Equal(t, 0, *port) - - cluster, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) - require.EqualError(t, err, "error getting cluster port") - assert.Nil(t, cluster) - assert.Nil(t, params) - }) +func TestGetMpgProxyParamsRejectsInvalidPublicPort(t *testing.T) { + for _, port := range []int{0, -1, 65536} { + t.Run(strconv.Itoa(port), func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = port + ctx := flag.NewContext(context.Background(), pflag.NewFlagSet("test", pflag.ContinueOnError)) + ctx = flapsutil.NewContextWithClient(ctx, &mock.FlapsClient{ + GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { + return c, nil + }, + }) + // No legacy or tunnel client: invalid public endpoints must stop before either. + cluster, params, err := GetMpgProxyParams(ctx, "0", "mpg-123", "test-org") + require.EqualError(t, err, fmt.Sprintf("invalid cluster port %d: must be between 1 and 65535", c.Endpoints.Primary.Direct.Port)) + require.Nil(t, cluster) + require.Nil(t, params) + }) + } } diff --git a/internal/command/mpg/v2/run_connect.go b/internal/command/mpg/v2/run_connect.go index 1c53b4c490..abaccbf91f 100644 --- a/internal/command/mpg/v2/run_connect.go +++ b/internal/command/mpg/v2/run_connect.go @@ -78,13 +78,12 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string, p } } - cluster, useLegacy, params, credentials, err := GetMpgConnectParams(ctx, localProxyPort, username, clusterID, resolvedOrgSlug) + cluster, params, credentials, err := GetMpgConnectParams(ctx, localProxyPort, username, clusterID, resolvedOrgSlug) if err != nil { return err } - // Gated on useLegacy; see maybeWarnLegacyNotReady's doc comment for why. - maybeWarnLegacyNotReady(io.ErrOut, useLegacy, cluster) + maybeWarnNotReady(io.ErrOut, cluster) psqlPath, err := exec.LookPath("psql") if err != nil { @@ -167,12 +166,7 @@ func RunConnect(ctx context.Context, clusterID string, resolvedOrgSlug string, p return err } -// buildConnectURL composes the psql connection URL from resolved credentials -// and the proxy port. db follows the priority used by RunConnect: an explicit -// --database value (or interactive prompt result) wins over credentials.DBName. -// credentials.DBName is the plan-required default ("fly-db") on both the -// public default-user and public explicit-user paths, so a non-interactive -// `fly mpg connect --user alice` lands on postgresql://.../fly-db. +// buildConnectURL prefers the selected database over the credential default. func buildConnectURL(credentials *mpgv2.GetClusterCredentialsResponse, db string, localProxyPort string) string { if db == "" { db = credentials.DBName @@ -181,18 +175,9 @@ func buildConnectURL(credentials *mpgv2.GetClusterCredentialsResponse, db string return fmt.Sprintf("postgresql://%s:%s@localhost:%s/%s", credentials.User, credentials.Password, localProxyPort, db) } -// maybeWarnLegacyNotReady restores the pre-migration legacy-path "Cluster -// is not in ready state" stderr warning. It is gated on useLegacy so the -// public path (which already refuses non-ready clusters via -// connectStatusRefusal) stays silent — see connectStatusRefusal's doc -// comment for the legacy/public status-split rationale. The warning -// format is preserved verbatim from the pre-migration code -// (commit 81f75427b^): aurora.Yellow("WARN") + " Cluster is not in ready -// state, currently: \n". The function is a thin side-effecting -// helper so it can be unit-tested with a bytes.Buffer without involving -// the agent/establish code path or RunConnect's exec/psql machinery. -func maybeWarnLegacyNotReady(errOut io.Writer, useLegacy bool, cluster *mpgv2.ManagedCluster) { - if !useLegacy || cluster == nil || cluster.Status == "ready" { +// maybeWarnNotReady warns when a cluster is not in ready state. +func maybeWarnNotReady(errOut io.Writer, cluster *mpgv2.ManagedCluster) { + if cluster == nil || cluster.Status == "ready" { return } diff --git a/internal/command/mpg/v2/run_connect_test.go b/internal/command/mpg/v2/run_connect_test.go index 887343912a..af7de717bb 100644 --- a/internal/command/mpg/v2/run_connect_test.go +++ b/internal/command/mpg/v2/run_connect_test.go @@ -8,114 +8,33 @@ import ( mpgv2 "github.com/superfly/flyctl/internal/uiex/mpg/v2" ) -// TestBuildConnectURLPublicExplicitUserDefaultsToFlyDB pins the public -// explicit-user connect path so it can never regress to the bad -// postgresql://.../ helper output. Credentials are resolved the same way the -// public branch of resolveConnectCredentials does, then handed straight to -// buildConnectURL the same way RunConnect does. -func TestBuildConnectURLPublicExplicitUserDefaultsToFlyDB(t *testing.T) { - creds := &mpgv2.GetClusterCredentialsResponse{ - User: "alice", - Password: "a", - DBName: "fly-db", // mirrors the fixed public explicit-user branch. - } - - got := buildConnectURL(creds, "", "16380") - require.Equal(t, "postgresql://alice:a@localhost:16380/fly-db", got) -} - -// TestBuildConnectURLRespectsExplicitDatabase verifies the priority order: -// an explicit --database value wins over credentials.DBName. -func TestBuildConnectURLRespectsExplicitDatabase(t *testing.T) { - creds := &mpgv2.GetClusterCredentialsResponse{ - User: "alice", - Password: "a", - DBName: "fly-db", - } - - got := buildConnectURL(creds, "app-db", "16380") - require.Equal(t, "postgresql://alice:a@localhost:16380/app-db", got) -} - -// TestBuildConnectURLEmptyDBNameFallsThrough is a guard for the historical -// bug shape: if the explicit-user public branch ever returns "" again, the -// URL will not silently land on postgresql://.../ (empty path). It will land -// on postgresql://.../ with the user-controlled --database fallback in -// RunConnect still preferring the flag value when present; without that -// fallback the URL here is malformed, which surfaces in tests rather than -// silently connecting to the wrong database. -func TestBuildConnectURLEmptyDBNameFallsThrough(t *testing.T) { - creds := &mpgv2.GetClusterCredentialsResponse{ - User: "alice", - Password: "a", - DBName: "", - } - - got := buildConnectURL(creds, "", "16380") - require.Equal(t, "postgresql://alice:a@localhost:16380/", got) -} - -// TestMaybeWarnLegacyNotReady pins the pre-migration warning format and -// the useLegacy/status gate. See maybeWarnLegacyNotReady's doc comment -// for the gate rationale and the warning-text provenance. -func TestMaybeWarnLegacyNotReady(t *testing.T) { +func TestMaybeWarnNotReady(t *testing.T) { const name = "test-cluster" tests := []struct { - name string - useLegacy bool - status string - wantWarn bool + name string + status string + wantWarn bool }{ - // Public path (useLegacy == false). connectStatusRefusal refuses - // non-ready public-path clusters upstream, so the useLegacy - // gate is the only thing that matters here. - {name: "public+creating silent", useLegacy: false, status: "creating", wantWarn: false}, - - // Legacy path (useLegacy == true). "ready" is silent (the - // status gate short-circuits before the fprintf); "creating" - // is the pre-migration non-refused case. The legacy-only - // "error" status is real on the legacy path but rejected by - // the public classifier — pinned here so a future migration - // does not silently drop it. - {name: "legacy+ready silent", useLegacy: true, status: "ready", wantWarn: false}, - {name: "legacy+creating warns", useLegacy: true, status: "creating", wantWarn: true}, - {name: "legacy+error warns", useLegacy: true, status: "error", wantWarn: true}, + {name: "ready silent", status: "ready", wantWarn: false}, + {name: "creating warns", status: "creating", wantWarn: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var buf bytes.Buffer cluster := &mpgv2.ManagedCluster{Name: name, Status: tt.status} - maybeWarnLegacyNotReady(&buf, tt.useLegacy, cluster) + maybeWarnNotReady(&buf, cluster) got := buf.String() if !tt.wantWarn { - require.Empty(t, got, "no warning expected for useLegacy=%v status=%q", tt.useLegacy, tt.status) + require.Empty(t, got, "no warning expected for status=%q", tt.status) return } - // Robust match against the pre-migration rendered text. We - // assert on "WARN" (the literal aurora payload when not a - // TTY) and on the exact "currently: " interpolation, - // rather than asserting the aurora-wrapped string verbatim, - // so this stays green regardless of the test harness's color - // configuration. + // Check the warning text independently of ANSI color codes. require.Contains(t, got, "WARN", "warning must contain the literal 'WARN' marker") require.Contains(t, got, "Cluster is not in ready state, currently: "+tt.status) require.True(t, bytes.HasSuffix(buf.Bytes(), []byte("\n")), "warning must end with a newline (pre-migration format)") }) } } - -// TestMaybeWarnLegacyNotReadyNilCluster pins the defensive nil-cluster -// short-circuit. RunConnect only calls the helper with the cluster -// returned by GetMpgConnectParams, which is never nil on the success -// path, but the helper is public and the nil guard prevents a panic if -// the contract ever loosens. -func TestMaybeWarnLegacyNotReadyNilCluster(t *testing.T) { - var buf bytes.Buffer - require.NotPanics(t, func() { - maybeWarnLegacyNotReady(&buf, true, nil) - }) - require.Empty(t, buf.String()) -} diff --git a/internal/command/mpg/v2/run_proxy.go b/internal/command/mpg/v2/run_proxy.go index f7927d3a04..490e85e08c 100644 --- a/internal/command/mpg/v2/run_proxy.go +++ b/internal/command/mpg/v2/run_proxy.go @@ -12,20 +12,12 @@ import ( "github.com/superfly/flyctl/internal/flag" "github.com/superfly/flyctl/internal/flapsutil" "github.com/superfly/flyctl/internal/flyutil" + "github.com/superfly/flyctl/internal/mpgutil" "github.com/superfly/flyctl/internal/uiex/mpg" mpgv2 "github.com/superfly/flyctl/internal/uiex/mpg/v2" "github.com/superfly/flyctl/proxy" ) -// defaultMPGUser is the literal username used when resolving the default -// fly-user on the public Machines API for Connect. Verified against the -// orchestrator and ui-ex repos. -const defaultMPGUser = "fly-user" - -// defaultMPGDatabase is the literal database name used when no --database -// flag is given and no interactive prompt answer is available. -const defaultMPGDatabase = "fly-db" - func RunProxy(ctx context.Context, clusterID string, resolvedOrgSlug string, proxyPort string) error { _, params, err := GetMpgProxyParams(ctx, proxyPort, clusterID, resolvedOrgSlug) if err != nil { @@ -57,89 +49,59 @@ func GetMpgProxyParams( return cluster, params, nil } -// GetMpgConnectParams builds proxy connection parameters and resolves the -// database credentials needed by fly mpg connect. -// -// The returned useLegacy bool mirrors the value computed inside getCluster -// (true when the public Machines API returned a classified 404 and we fell -// back to the legacy ui-ex client; false when the public API succeeded). -// RunConnect uses it to gate the pre-migration "Cluster is not in ready -// state" stderr warning, which is meaningful only on the legacy path — -// see connectStatusRefusal's doc comment for why the public path does not -// need it. +// GetMpgConnectParams resolves credentials and proxy parameters. func GetMpgConnectParams( ctx context.Context, localProxyPort string, username string, clusterID string, resolvedOrgSlug string, -) (*mpgv2.ManagedCluster, bool, *proxy.ConnectParams, *mpgv2.GetClusterCredentialsResponse, error) { +) (*mpgv2.ManagedCluster, *proxy.ConnectParams, *mpgv2.GetClusterCredentialsResponse, error) { response, useLegacy, port, err := getCluster(ctx, clusterID) if err != nil { - return nil, false, nil, nil, err + return nil, nil, nil, err } credentials, err := resolveConnectCredentials(ctx, response, useLegacy, username) if err != nil { - return nil, false, nil, nil, err + return nil, nil, nil, err } cluster, params, err := buildProxyParams(ctx, response, port, localProxyPort, resolvedOrgSlug) if err != nil { - return nil, false, nil, nil, err + return nil, nil, nil, err } - return cluster, useLegacy, params, credentials, nil + return cluster, params, credentials, nil } -// getCluster retrieves cluster details via the public Machines API, falling -// back to the legacy MPGv1 client only when the public call returns a -// classified 404. Any other non-nil public error is returned immediately, -// wrapped in "failed retrieving cluster %s: %w". The returned useLegacy flag -// indicates which client -// produced the response so downstream credential resolution can choose the -// appropriate code path. The returned port is the public API's advertised -// direct-endpoint port (nil on the legacy path, which has no port field) — -// carried as a separate return value rather than a field on the legacy -// mpgv2.ManagedCluster type, since that type is part of the private/legacy -// client package (internal/uiex/mpg) that this migration must not modify. -func getCluster(ctx context.Context, clusterID string) (*mpgv2.GetClusterResponse, bool, *int, error) { +// getCluster tries the public API, falling back to the legacy client only on 404. +// It returns the credential source and direct endpoint port (5432 for legacy). +func getCluster(ctx context.Context, clusterID string) (*mpgv2.GetClusterResponse, bool, int, error) { flapsClient := flapsutil.ClientFromContext(ctx) publicCluster, err := flapsClient.GetManagedPostgresCluster(ctx, clusterID) if err == nil { response, port := publicToLegacyClusterResponse(publicCluster) + return &response, false, port, nil } if !errors.Is(err, flaps.ErrFlapsNotFound) { - return nil, false, nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) + return nil, false, 0, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) } legacyClient := mpgv2.ClientFromContext(ctx) response, err := legacyClient.GetClusterById(ctx, clusterID) if err != nil { - return nil, true, nil, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) + return nil, true, 0, fmt.Errorf("failed retrieving cluster %s: %w", clusterID, err) } - return &response, true, nil, nil + return &response, true, mpgutil.DefaultPort, nil } -// publicToLegacyClusterResponse wraps the public Machines API cluster in the -// legacy ui-ex envelope so that downstream callers (proxyParams, -// resolveConnectCredentials) can read IpAssignments.Direct and Status without -// translation. The legacy shape preserves the bare-address column for -// proxyParams.RemoteHost and the Status field for connectStatusRefusal. -// -// The public API's advertised Endpoints.Primary.Direct.Port is returned -// separately (not as a field on the legacy mpgv2.ManagedCluster type) so -// proxyParams can dial the real advertised port instead of the legacy -// hardcoded 5432, without adding a public-API-only field to the private -// legacy client package. A nil port return means "no port info" (the legacy -// path never calls this function at all, so in practice this only happens -// if ever called with a zero-value input); a non-nil pointer — even to 0 — -// means the public path returned some value, and *0 is treated as invalid -// by proxyParams. -func publicToLegacyClusterResponse(c flaps.ManagedPostgresCluster) (mpgv2.GetClusterResponse, *int) { +// publicToLegacyClusterResponse adapts the public cluster to the legacy shape. +// The advertised direct port is returned unchanged for validation. +func publicToLegacyClusterResponse(c flaps.ManagedPostgresCluster) (mpgv2.GetClusterResponse, int) { port := c.Endpoints.Primary.Direct.Port return mpgv2.GetClusterResponse{ @@ -154,44 +116,17 @@ func publicToLegacyClusterResponse(c flaps.ManagedPostgresCluster) (mpgv2.GetClu Organization: fly.Organization{Name: c.Organization.Name, Slug: c.Organization.Slug}, IpAssignments: mpg.ManagedClusterIpAssignments{Direct: c.Endpoints.Primary.Direct.Host}, }, - }, &port + }, port } -// resolveConnectCredentials returns the credentials used by RunConnect. -// useLegacy=true routes through the legacy ui-ex client; useLegacy=false -// routes through the public Machines API's -// GetManagedPostgresUserCredentials. The default user (no --user flag) -// resolves to defaultMPGUser ("fly-user"); explicit users pass the flag -// value straight through. The public path has no envelope-level DBName, -// so both public branches fall back to defaultMPGDatabase ("fly-db") — -// which run_connect.go's buildConnectURL uses to land on the plan-required -// default when neither --database nor an interactive prompt supplies one. -// -// Status gating is split across the two paths because their vocabularies -// differ; see connectStatusRefusal's doc comment for the full rationale. -// Briefly: the public path consults response.Data.Status BEFORE any -// credentials call (a 7-value enum + fail-closed default); the legacy -// path keeps the pre-migration post-fetch credentials.Status + -// credentials.Password check ("error" is a real legacy value but not a -// public one, and the legacy envelope's Status field is semantically -// distinct from cluster status). +// resolveConnectCredentials uses the same API as the cluster lookup. +// Public credentials default to fly-user and fly-db. func resolveConnectCredentials( ctx context.Context, response *mpgv2.GetClusterResponse, useLegacy bool, username string, ) (*mpgv2.GetClusterCredentialsResponse, error) { - // Status classifier runs BEFORE any credentials call on the public - // path only — see connectStatusRefusal's doc comment for the - // legacy/public split. response.Data.Status is populated on the public - // path via publicToLegacyClusterResponse (from c.Status); it is - // intentionally NOT consulted on the legacy path. - if !useLegacy { - if err := connectStatusRefusal(response.Data.Name, response.Data.Status); err != nil { - return nil, err - } - } - var credentials mpgv2.GetClusterCredentialsResponse switch { @@ -217,32 +152,30 @@ func resolveConnectCredentials( credentials = mpgv2.GetClusterCredentialsResponse{ User: userCreds.Username, Password: userCreds.Password, - DBName: defaultMPGDatabase, + DBName: mpgutil.DefaultDatabase, } case useLegacy: credentials = response.Credentials default: flapsClient := flapsutil.ClientFromContext(ctx) - userCreds, err := flapsClient.GetManagedPostgresUserCredentials(ctx, response.Data.Id, defaultMPGUser) + userCreds, err := flapsClient.GetManagedPostgresUserCredentials(ctx, response.Data.Id, mpgutil.DefaultUsername) if err != nil { - return nil, fmt.Errorf("failed retrieving credentials for user %s: %w", defaultMPGUser, err) + if errors.Is(err, flaps.ErrFlapsNotFound) { + return nil, fmt.Errorf("cluster is still initializing, wait a bit more") + } + + return nil, fmt.Errorf("failed retrieving credentials for user %s: %w", mpgutil.DefaultUsername, err) } credentials = mpgv2.GetClusterCredentialsResponse{ User: userCreds.Username, Password: userCreds.Password, - DBName: defaultMPGDatabase, + DBName: mpgutil.DefaultDatabase, } } if useLegacy { - // ORIGINAL pre-migration legacy status/password checks, - // restored verbatim. credentials.Status is only checked on the - // default-user path because the explicit-user legacy - // GetUserCredentials response does not populate that field - // (only User/Password/DBName). See connectStatusRefusal's doc - // comment for why this path is intentionally distinct from the - // public classifier. + // Only legacy default-user credentials include a status. if username == "" { if credentials.Status == "initializing" { return nil, fmt.Errorf("cluster is still initializing, wait a bit more") @@ -255,13 +188,6 @@ func resolveConnectCredentials( return nil, fmt.Errorf("error getting user password") } } else if credentials.Password == "" { - // Public path: empty-password fallback after the credentials - // call. The wording tracks whether the user was explicit - // (flag/prompt) or defaulted to the plan-required fly-user, - // preserving the existing pre-fix messages. The public path - // has no envelope-level Status to inspect post-fetch, so the - // pre-credential connectStatusRefusal call above is the only - // status gate. if username == "" { return nil, fmt.Errorf("error getting cluster password") } @@ -272,90 +198,10 @@ func resolveConnectCredentials( return &credentials, nil } -// connectStatusRefusal is the explicit status classifier for fly mpg -// connect. It returns nil when the cluster's status permits a connect -// attempt and a deliberate, status-specific refusal error otherwise. The -// complete emitted set of "status" values from a successful public MPG -// cluster lookup (traced across the mpg / nomad-firecracker / ui-ex code -// paths this session) is: -// -// ready, standby_ready, creating, deleting, failed, deleted, initializing -// -// "error" is NOT a real public cluster status value; it does not appear -// in the public enum and is rejected if encountered. Behavior per status: -// -// - ready: proceed, no message. -// - standby_ready: refuse — a standby replica is not a connect target. -// Fail-closed pending a product decision on standby -// semantics; not even a "warn and continue" — refused -// outright. -// - creating: refuse — friendly wording distinct from initializing -// since the cluster is in the process of coming up, -// not stuck mid-provision. -// - deleting: refuse — cluster is going away, connections -// meaningless. -// - deleted: refuse — terminal state, no connect possible. -// - failed: refuse — accurate wording ("cluster is in a failed -// state"), not the misleading "error getting cluster -// password" misdiagnosis. -// - initializing: refuse — neutral wording because this now covers -// many underlying states (degraded, updating, -// resizing, credential rotation, promotion, and -// future unmapped states), not just fresh -// provisioning. Do not over-promise "wait a bit more" -// for a state that may not be purely transient. -// - default: refuse — unknown / unrecognized statuses fail -// closed with the actual status string quoted so the -// user (and on-call) can see what the public API -// actually returned. Do NOT let unknown values fall -// through to credential resolution. -// -// The classifier applies ONLY to the public path (useLegacy == false), -// BEFORE any public credential call, on both the default-user and -// explicit-user paths. It is intentionally NOT applied to the legacy -// path (useLegacy == true): the legacy status vocabulary differs from -// the public API's — e.g. "error" is a real legacy cluster status value -// but not a public one — and the pre-migration legacy code understood it -// natively via credentials.Status (the legacy credentials envelope's own -// status field, populated post-fetch and semantically distinct from -// response.Data.Status). Applying this public-only classifier to legacy -// responses would mis-handle legacy-only statuses as "unrecognized -// state" and silently drop the credentials.Status check. -// -// The classifier is NOT applied to fly mpg proxy — that command -// deliberately does not gate on status because the raw TCP proxy never -// uses credentials, only the direct IP/port; see RunProxy / -// GetMpgProxyParams. Adding a status gate there would reintroduce the -// exact coupling flyctl#5048 was built to remove. -// -// name is the cluster identifier included in the refusal message so the -// user knows which cluster the refusal applies to. It comes from -// response.Data.Name (populated on both the public and legacy paths). -func connectStatusRefusal(name, status string) error { - switch status { - case "ready": - return nil - case "standby_ready": - return fmt.Errorf("cluster %s is a standby replica and cannot be used with fly mpg connect", name) - case "creating": - return fmt.Errorf("cluster %s is still being created, wait a bit more", name) - case "deleting": - return fmt.Errorf("cluster %s is being deleted and cannot be connected to", name) - case "deleted": - return fmt.Errorf("cluster %s has been deleted and cannot be connected to", name) - case "failed": - return fmt.Errorf("cluster %s is in a failed state", name) - case "initializing": - return fmt.Errorf("cluster %s is not currently ready for connections (status: initializing)", name) - default: - return fmt.Errorf("cluster %s is in an unrecognized state (%q) and cannot be connected to", name, status) - } -} - func buildProxyParams( ctx context.Context, response *mpgv2.GetClusterResponse, - port *int, + port int, localProxyPort string, resolvedOrgSlug string, ) (*mpgv2.ManagedCluster, *proxy.ConnectParams, error) { @@ -384,7 +230,7 @@ func buildProxyParams( func proxyParams( response *mpgv2.GetClusterResponse, - port *int, + port int, localProxyPort string, resolvedOrgSlug string, bindAddr string, @@ -395,29 +241,12 @@ func proxyParams( return nil, nil, fmt.Errorf("error getting cluster IP") } - // remotePort is the port the proxy will dial on the remote host. The - // legacy path has no port field at all, so it always calls this with - // port == nil and we fall back to "5432" exactly as before. The public - // path passes the advertised Endpoints.Primary.Direct.Port through - // getCluster so a non-5432 public port dials that real port instead of - // being silently rewritten to 5432. A public API that advertises port 0 - // is treated as an invalid/unexpected value and surfaces as an error — - // distinguishing it from the legacy "no port field" case, where 5432 is - // the historical default and the safe fallback. The *int rather than a - // separate bool encodes the "optional value" semantic directly: nil = - // no port info (legacy), non-nil (even *port == 0) = public path - // advertised some value. - remotePort := "5432" - if port != nil { - if *port == 0 { - return nil, nil, fmt.Errorf("error getting cluster port") - } - - remotePort = strconv.Itoa(*port) + if port < 1 || port > 65535 { + return nil, nil, fmt.Errorf("invalid cluster port %d: must be between 1 and 65535", port) } return cluster, &proxy.ConnectParams{ - Ports: []string{localProxyPort, remotePort}, + Ports: []string{localProxyPort, strconv.Itoa(port)}, OrganizationSlug: resolvedOrgSlug, Dialer: dialer, BindAddr: bindAddr, diff --git a/internal/command/mpg/v2/run_proxy_test.go b/internal/command/mpg/v2/run_proxy_test.go index ca376f2d41..5117200f85 100644 --- a/internal/command/mpg/v2/run_proxy_test.go +++ b/internal/command/mpg/v2/run_proxy_test.go @@ -3,15 +3,20 @@ package cmdv2 import ( "context" "errors" + "fmt" "net" + "strconv" "testing" + "github.com/spf13/pflag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" fly "github.com/superfly/fly-go" "github.com/superfly/fly-go/flaps" + "github.com/superfly/flyctl/internal/flag" "github.com/superfly/flyctl/internal/flapsutil" "github.com/superfly/flyctl/internal/mock" + "github.com/superfly/flyctl/internal/mpgutil" "github.com/superfly/flyctl/internal/uiex/mpg" mpgv2 "github.com/superfly/flyctl/internal/uiex/mpg/v2" "github.com/superfly/flyctl/wg" @@ -56,7 +61,7 @@ func TestProxyParamsIgnoreCredentials(t *testing.T) { } dialer := &testDialer{} - cluster, params, err := proxyParams(&response, nil, "15432", "test-org", "127.0.0.2", dialer) + cluster, params, err := proxyParams(&response, mpgutil.DefaultPort, "15432", "test-org", "127.0.0.2", dialer) require.NoError(t, err) assert.Same(t, &response.Data, cluster) assert.Equal(t, []string{"15432", "5432"}, params.Ports) @@ -69,14 +74,13 @@ func TestProxyParamsIgnoreCredentials(t *testing.T) { } func TestProxyParamsRequireDirectIP(t *testing.T) { - cluster, params, err := proxyParams(&mpgv2.GetClusterResponse{}, nil, "15432", "test-org", "127.0.0.1", nil) + cluster, params, err := proxyParams(&mpgv2.GetClusterResponse{}, 0, "15432", "test-org", "127.0.0.1", nil) require.EqualError(t, err, "error getting cluster IP") assert.Nil(t, cluster) assert.Nil(t, params) } -// samplePublicCluster is a representative public Machines API cluster payload. func samplePublicCluster() flaps.ManagedPostgresCluster { return flaps.ManagedPostgresCluster{ ID: "mpg-123", @@ -99,8 +103,6 @@ func samplePublicCluster() flaps.ManagedPostgresCluster { } } -// sampleLegacyCluster is a representative legacy MPGv1 cluster payload. -// Direct is a bare address (no port) — the historical shape of this column. func sampleLegacyCluster() mpgv2.GetClusterResponse { return mpgv2.GetClusterResponse{ Data: mpgv2.ManagedCluster{ @@ -151,9 +153,12 @@ func TestGetCluster(t *testing.T) { wantLegacyCalls: 0, }, { - name: "classified 404 falls back to legacy", - publicCluster: flaps.ManagedPostgresCluster{}, - publicErr: flaps.ErrFlapsNotFound, + name: "classified 404 falls back to legacy", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: fmt.Errorf("wrapped: %w", &flaps.FlapsError{ + ResponseStatusCode: 404, + OriginalError: errors.New("not found"), + }), legacyResponse: sampleLegacyCluster(), wantUseLegacy: true, wantDirectHost: "10.0.0.1", @@ -161,19 +166,29 @@ func TestGetCluster(t *testing.T) { wantLegacyCalls: 1, }, { - name: "classified 404 with legacy failure propagates legacy error", - publicCluster: flaps.ManagedPostgresCluster{}, - publicErr: flaps.ErrFlapsNotFound, + name: "classified 404 with legacy failure propagates legacy error", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: fmt.Errorf("wrapped: %w", &flaps.FlapsError{ + ResponseStatusCode: 404, + OriginalError: errors.New("not found"), + }), legacyResponse: mpgv2.GetClusterResponse{}, legacyErr: errors.New("legacy denied"), wantErr: "failed retrieving cluster mpg-123: legacy denied", wantLegacyCalls: 1, }, { - name: "non-404 public error returns without fallback", + name: "403 public error returns without fallback", publicCluster: flaps.ManagedPostgresCluster{}, - publicErr: errors.New("boom"), - wantErr: "failed retrieving cluster mpg-123: boom", + publicErr: &flaps.FlapsError{ResponseStatusCode: 403, OriginalError: errors.New("denied")}, + wantErr: "failed retrieving cluster mpg-123: denied", + wantLegacyCalls: 0, + }, + { + name: "410 public error returns without fallback", + publicCluster: flaps.ManagedPostgresCluster{}, + publicErr: &flaps.FlapsError{ResponseStatusCode: 410, OriginalError: errors.New("gone")}, + wantErr: "failed retrieving cluster mpg-123: gone", wantLegacyCalls: 0, }, } @@ -198,7 +213,7 @@ func TestGetCluster(t *testing.T) { }, }) - got, useLegacy, _, err := getCluster(ctx, "mpg-123") + got, useLegacy, port, err := getCluster(ctx, "mpg-123") if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) require.Nil(t, got) @@ -214,20 +229,26 @@ func TestGetCluster(t *testing.T) { require.Equal(t, tt.wantStatus, got.Data.Status) require.Equal(t, 1, publicCalls) require.Equal(t, tt.wantLegacyCalls, legacyCalls) + require.Equal(t, 5432, port) + if tt.wantUseLegacy { + _, params, err := proxyParams(got, port, "16380", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, []string{"16380", "5432"}, params.Ports) + } }) } } -// TestProxyParamsPublicNeverTouchesCredentials verifies the invariant that -// the proxy code path never resolves credentials: getCluster must only hit -// the cluster lookup endpoint, not the credentials endpoint, on the public- -// success branch. This pins RunProxy against accidentally growing a -// credentials dependency. -func TestProxyParamsPublicNeverTouchesCredentials(t *testing.T) { +func TestGetMpgProxyParamsPublicNeverTouchesCredentials(t *testing.T) { credCalls, legacyCredCalls := 0, 0 - ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + ctx := flag.NewContext(context.Background(), pflag.NewFlagSet("test", pflag.ContinueOnError)) + ctx = flapsutil.NewContextWithClient(ctx, &mock.FlapsClient{ GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { - return samplePublicCluster(), nil + cluster := samplePublicCluster() + cluster.Status = "creating" + cluster.Endpoints.Primary.Direct.Host = "" + + return cluster, nil }, GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { credCalls++ @@ -243,37 +264,33 @@ func TestProxyParamsPublicNeverTouchesCredentials(t *testing.T) { }, }) - response, useLegacy, port, err := getCluster(ctx, "mpg-123") - require.NoError(t, err) - require.NotNil(t, response) - require.False(t, useLegacy) - require.Equal(t, 0, credCalls, "getCluster (public-success path) must never resolve credentials") - require.Equal(t, 0, legacyCredCalls, "getCluster must never reach the legacy client on public success") - - // proxyParams does not take a ctx, so it cannot make any HTTP call; it is - // structurally incapable of leaking credentials. Verify it produces the - // expected bare-host RemoteHost from the public-converted response. The - // credCalls / legacyCredCalls counters were already asserted to be 0 - // above, immediately after getCluster returned, so proxyParams has - // nothing to regress there. - cluster, params, err := proxyParams(response, port, "15432", "test-org", "127.0.0.1", nil) - require.NoError(t, err) - require.NotNil(t, cluster) - require.NotNil(t, params) - require.Equal(t, "10.0.0.1", params.RemoteHost) + cluster, params, err := GetMpgProxyParams(ctx, "15432", "mpg-123", "test-org") + require.EqualError(t, err, "error getting cluster IP") + require.Nil(t, cluster) + require.Nil(t, params) + require.Zero(t, credCalls) + require.Zero(t, legacyCredCalls) +} + +func TestGetMpgConnectParamsResolvesCredentialsBeforeTunnel(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { + return samplePublicCluster(), nil + }, + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{Username: mpgutil.DefaultUsername}, nil + }, + }) + + cluster, params, credentials, err := GetMpgConnectParams(ctx, "15432", "", "mpg-123", "test-org") + require.EqualError(t, err, "error getting cluster password") + require.Nil(t, cluster) + require.Nil(t, params) + require.Nil(t, credentials) } func TestResolveDefaultConnectCredentials(t *testing.T) { - // The legacy default-user connect path (useLegacy == true) restores - // the pre-migration post-fetch logic: the legacy credentials - // envelope's own Status field (credentials.Status, semantically - // distinct from cluster status) is checked for "initializing"/"error", - // plus an empty-password fallback. These cases pin that behavior - // with the original error messages. The public status classifier is - // intentionally NOT applied here; that is covered by - // TestResolveConnectCredentialsPublicStatusClassifier below. See - // connectStatusRefusal's doc comment for the legacy/public status- - // split rationale. + // Legacy readiness comes from the credential envelope, not cluster status. const name = "test-cluster" tests := []struct { name string @@ -283,8 +300,6 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { err string }{ { - // ORIGINAL legacy: empty password is the empty-password - // fallback; status field is irrelevant when password is empty. name: "empty password", clusterSt: "ready", credStatus: "ready", @@ -292,13 +307,6 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { err: "error getting cluster password", }, { - // REGRESSION GUARD: Data.Status="ready" with credentials - // .Status="initializing" and a non-empty password MUST refuse - // on the legacy path. Applying the public-only 7-value - // status classifier to the legacy path (where it does not - // belong) would silently drop the credentials.Status check - // and let this case proceed to psql with possibly-invalid - // credentials. This is the regression this test pins. name: "ready cluster with stale initializing credentials refuses", clusterSt: "ready", credStatus: "initializing", @@ -306,11 +314,6 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { err: "cluster is still initializing, wait a bit more", }, { - // REGRESSION GUARD: Data.Status="ready" with credentials - // .Status="error" and a non-empty password MUST refuse on - // the legacy path. Same reasoning as above. ("error" is a - // real legacy status value but not a public one — the public - // classifier rejects it via its default arm.) name: "ready cluster with stale error credentials refuses", clusterSt: "ready", credStatus: "error", @@ -318,11 +321,6 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { err: "error getting cluster password", }, { - // The cluster status alone ("creating", "failed", etc.) is - // NOT consulted on the legacy path — only credentials.Status - // and credentials.Password are. So a "ready"-credentials - // envelope with a non-empty password proceeds regardless of - // the cluster status field. name: "creating cluster with ready credentials proceeds", clusterSt: "creating", credStatus: "ready", @@ -346,6 +344,7 @@ func TestResolveDefaultConnectCredentials(t *testing.T) { if tt.err == "" { require.NoError(t, err) require.NotNil(t, credentials) + return } require.EqualError(t, err, tt.err) @@ -362,6 +361,7 @@ func TestResolveExplicitUserConnectCredentials(t *testing.T) { Status: "ready", }, Credentials: mpgv2.GetClusterCredentialsResponse{ + Status: "initializing", DBName: "default-db", }, } @@ -424,15 +424,6 @@ func TestResolveExplicitUserConnectCredentialsErrors(t *testing.T) { }) } -// TestResolveConnectCredentialsPublic covers the public-API code paths for -// Connect credentials: default-user goes through -// GetManagedPostgresUserCredentials("fly-user") and DBName defaults to -// "fly-db"; explicit-user passes the flag value straight through. Data.Status -// is set to "ready" on every response here so the status classifier (which -// runs before any credentials call) does not interfere — see -// TestResolveConnectCredentialsPublicStatusClassifier for the dedicated -// status-classifier coverage. See connectStatusRefusal's doc comment for -// the legacy/public status-split rationale. func TestResolveConnectCredentialsPublic(t *testing.T) { response := &mpgv2.GetClusterResponse{ Data: mpgv2.ManagedCluster{Id: "cluster-id", Name: "test-cluster", Status: "ready"}, @@ -475,11 +466,9 @@ func TestResolveConnectCredentialsPublic(t *testing.T) { require.Equal(t, 1, credCalls) require.Equal(t, "alice", credentials.User) require.Equal(t, "a", credentials.Password) - // The public path has no envelope DBName; explicit users fall back to - // defaultMPGDatabase ("fly-db") so that run_connect.go's psql URL - // construction lands on the plan-required default when neither - // --database nor an interactive prompt supplies one. require.Equal(t, "fly-db", credentials.DBName) + require.Equal(t, "postgresql://alice:a@localhost:16380/fly-db", buildConnectURL(credentials, "", "16380")) + require.Equal(t, "postgresql://alice:a@localhost:16380/app-db", buildConnectURL(credentials, "app-db", "16380")) }) t.Run("default user empty password mirrors legacy error", func(t *testing.T) { @@ -518,77 +507,38 @@ func TestResolveConnectCredentialsPublic(t *testing.T) { assert.Nil(t, credentials) }) - t.Run("explicit user public credentials error propagates", func(t *testing.T) { + t.Run("default user 404 preserves initializing error", func(t *testing.T) { ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { - return flaps.ManagedPostgresUserCredentials{}, errors.New("missing user") + return flaps.ManagedPostgresUserCredentials{}, fmt.Errorf("wrapped: %w", &flaps.FlapsError{ResponseStatusCode: 404, OriginalError: errors.New("not found")}) }, }) - credentials, err := resolveConnectCredentials(ctx, response, false, "alice") - require.EqualError(t, err, "failed retrieving credentials for user alice: missing user") + credentials, err := resolveConnectCredentials(ctx, response, false, "") + require.EqualError(t, err, "cluster is still initializing, wait a bit more") assert.Nil(t, credentials) }) -} -// TestPublicToLegacyClusterResponse verifies that the public-API cluster is -// converted to the legacy ui-ex shape, preserving the bare-address column for -// proxyParams and the Status field for downstream status checks -// (connectStatusRefusal / maybeWarnLegacyNotReady). -func TestPublicToLegacyClusterResponse(t *testing.T) { - got, _ := publicToLegacyClusterResponse(samplePublicCluster()) - require.Equal(t, "mpg-123", got.Data.Id) - require.Equal(t, "test-cluster", got.Data.Name) - require.Equal(t, "ready", got.Data.Status) - require.Equal(t, "ord", got.Data.Region) - require.Equal(t, "development", got.Data.Plan) - require.Equal(t, 10, got.Data.Disk) - require.Equal(t, 1, got.Data.Replicas) - require.Equal(t, "Test Org", got.Data.Organization.Name) - require.Equal(t, "test-org", got.Data.Organization.Slug) - require.Equal(t, "10.0.0.1", got.Data.IpAssignments.Direct) + t.Run("explicit user 404 remains a user error", func(t *testing.T) { + ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ + GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { + return flaps.ManagedPostgresUserCredentials{}, fmt.Errorf("wrapped: %w", &flaps.FlapsError{ResponseStatusCode: 404, OriginalError: errors.New("missing user")}) + }, + }) + + credentials, err := resolveConnectCredentials(ctx, response, false, "alice") + require.EqualError(t, err, "failed retrieving credentials for user alice: wrapped: missing user") + assert.Nil(t, credentials) + }) } -// TestResolveConnectCredentialsPublicStatusClassifier proves the public-path -// status classifier short-circuits BEFORE any credentials resolution call, -// for both the default-user and explicit-user connect paths. It exercises -// connectStatusRefusal (via resolveConnectCredentials) for the full 9-value -// status matrix — the 7 documented public statuses plus the "error" / -// "degraded" unrecognized sentinels — and asserts on each: -// -// - the exact deliberate refusal message, with the cluster name -// interpolated; -// - the credentials endpoint is never called (credCalls == 0). -// -// On the proceed status ("ready") it asserts: -// -// - no refusal error; -// - the credentials endpoint IS called exactly once (credCalls == 1). -// -// The classifier applies ONLY to the public path (useLegacy == false). The -// legacy path (useLegacy == true) is intentionally NOT exercised here — it -// uses its original pre-migration credentials.Status / credentials.Password -// post-fetch logic instead, pinned by TestResolveDefaultConnectCredentials -// above. -func TestResolveConnectCredentialsPublicStatusClassifier(t *testing.T) { +func TestResolveConnectCredentialsPublicNonReady(t *testing.T) { const name = "test-cluster" - const unknownStatus = "degraded" tests := []struct { - status string - wantErr string // expected refusal error message; "" means proceed. + status string }{ - {"ready", ""}, - {"standby_ready", "cluster " + name + " is a standby replica and cannot be used with fly mpg connect"}, - {"creating", "cluster " + name + " is still being created, wait a bit more"}, - {"deleting", "cluster " + name + " is being deleted and cannot be connected to"}, - {"deleted", "cluster " + name + " has been deleted and cannot be connected to"}, - {"failed", "cluster " + name + " is in a failed state"}, - {"initializing", "cluster " + name + " is not currently ready for connections (status: initializing)"}, - // Sentinel: "error" is NOT a real public cluster status — it is - // rejected by the default arm of the classifier. - {"error", `cluster ` + name + ` is in an unrecognized state ("error") and cannot be connected to`}, - // Sentinel: an arbitrary future / unmapped status also fails closed. - {unknownStatus, `cluster ` + name + ` is in an unrecognized state ("` + unknownStatus + `") and cannot be connected to`}, + {"creating"}, + {"degraded"}, } for _, tt := range tests { @@ -608,15 +558,8 @@ func TestResolveConnectCredentialsPublicStatusClassifier(t *testing.T) { }) credentials, err := resolveConnectCredentials(ctx, &response, false, "") - if tt.wantErr != "" { - require.EqualError(t, err, tt.wantErr) - assert.Nil(t, credentials) - require.Equal(t, 0, credCalls, "refusal must short-circuit before any credentials call (default user, public path)") - - return - } - require.NoError(t, err) - require.Equal(t, 1, credCalls, "proceed must hit the credentials endpoint exactly once") + require.NoError(t, err, "non-ready cluster must proceed to credential resolution") + require.Equal(t, 1, credCalls, "must hit the credentials endpoint exactly once") require.NotNil(t, credentials) }) @@ -636,70 +579,44 @@ func TestResolveConnectCredentialsPublicStatusClassifier(t *testing.T) { }) credentials, err := resolveConnectCredentials(ctx, &response, false, "alice") - if tt.wantErr != "" { - require.EqualError(t, err, tt.wantErr) - assert.Nil(t, credentials) - require.Equal(t, 0, credCalls, "refusal must short-circuit before any credentials call (explicit user, public path)") - - return - } - require.NoError(t, err) - require.Equal(t, 1, credCalls, "proceed must hit the credentials endpoint exactly once") + require.NoError(t, err, "non-ready cluster must proceed to credential resolution") + require.Equal(t, 1, credCalls, "must hit the credentials endpoint exactly once") require.NotNil(t, credentials) }) } } -// TestProxyParamsPublicNonDefaultPort proves that the public path dials the -// actual advertised port from Endpoints.Primary.Direct.Port instead of the -// legacy hardcoded 5432. The legacy path is also pinned to 5432 here so the -// regression surface is locked down on both sides. The public-zero case is -// pinned to an error so a genuinely-advertised port 0 cannot be silently -// treated like the legacy "no port field" default. -func TestProxyParamsPublicNonDefaultPort(t *testing.T) { - t.Run("public port 5433 dials 5433, not 5432", func(t *testing.T) { - c := samplePublicCluster() - c.Endpoints.Primary.Direct.Port = 5433 - response, port := publicToLegacyClusterResponse(c) - - _, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) - require.NoError(t, err) - require.Equal(t, "10.0.0.1", params.RemoteHost) - require.Equal(t, []string{"16380", "5433"}, params.Ports) - }) - - t.Run("public port 5432 stays 5432 (no-op for the default)", func(t *testing.T) { - c := samplePublicCluster() - c.Endpoints.Primary.Direct.Port = 5432 - response, port := publicToLegacyClusterResponse(c) - - _, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) - require.NoError(t, err) - require.Equal(t, []string{"16380", "5432"}, params.Ports) - }) +func TestProxyParamsPublicPorts(t *testing.T) { + for _, port := range []int{1, 5433, 65535} { + t.Run(strconv.Itoa(port), func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = port + response, advertisedPort := publicToLegacyClusterResponse(c) - t.Run("legacy port stays hardcoded 5432", func(t *testing.T) { - response := sampleLegacyCluster() - // legacy path never calls publicToLegacyClusterResponse, so its port - // value is always nil — the proxyParams fallback kicks in and "5432" - // is used exactly as before. - _, params, err := proxyParams(&response, nil, "16380", "test-org", "127.0.0.1", nil) - require.NoError(t, err) - require.Equal(t, []string{"16380", "5432"}, params.Ports) - }) + _, params, err := proxyParams(&response, advertisedPort, "0", "test-org", "127.0.0.1", nil) + require.NoError(t, err) + require.Equal(t, "10.0.0.1", params.RemoteHost) + require.Equal(t, []string{"0", strconv.Itoa(port)}, params.Ports) + }) + } +} - t.Run("public port 0 surfaces as an error instead of silently dialing 5432", func(t *testing.T) { - c := samplePublicCluster() - c.Endpoints.Primary.Direct.Port = 0 - response, port := publicToLegacyClusterResponse(c) - // Sanity check: the adapter still produces a non-nil pointer (so the - // public-vs-legacy distinction is preserved). - require.NotNil(t, port) - require.Equal(t, 0, *port) - - cluster, params, err := proxyParams(&response, port, "16380", "test-org", "127.0.0.1", nil) - require.EqualError(t, err, "error getting cluster port") - assert.Nil(t, cluster) - assert.Nil(t, params) - }) +func TestGetMpgProxyParamsRejectsInvalidPublicPort(t *testing.T) { + for _, port := range []int{0, -1, 65536} { + t.Run(strconv.Itoa(port), func(t *testing.T) { + c := samplePublicCluster() + c.Endpoints.Primary.Direct.Port = port + ctx := flag.NewContext(context.Background(), pflag.NewFlagSet("test", pflag.ContinueOnError)) + ctx = flapsutil.NewContextWithClient(ctx, &mock.FlapsClient{ + GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { + return c, nil + }, + }) + // No legacy or tunnel client: invalid public endpoints must stop before either. + cluster, params, err := GetMpgProxyParams(ctx, "0", "mpg-123", "test-org") + require.EqualError(t, err, fmt.Sprintf("invalid cluster port %d: must be between 1 and 65535", c.Endpoints.Primary.Direct.Port)) + require.Nil(t, cluster) + require.Nil(t, params) + }) + } } From e3739b63e469c85c9e21376dfd39f12ed90defb9 Mon Sep 17 00:00:00 2001 From: vincent Date: Tue, 8 Sep 2026 12:33:08 -0400 Subject: [PATCH 3/3] fix(mpg): avoid duplicate proxy test fixture names --- internal/command/mpg/v2/run_proxy_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/command/mpg/v2/run_proxy_test.go b/internal/command/mpg/v2/run_proxy_test.go index 5117200f85..ead8ecdf2d 100644 --- a/internal/command/mpg/v2/run_proxy_test.go +++ b/internal/command/mpg/v2/run_proxy_test.go @@ -81,7 +81,7 @@ func TestProxyParamsRequireDirectIP(t *testing.T) { assert.Nil(t, params) } -func samplePublicCluster() flaps.ManagedPostgresCluster { +func sampleProxyPublicCluster() flaps.ManagedPostgresCluster { return flaps.ManagedPostgresCluster{ ID: "mpg-123", Name: "test-cluster", @@ -103,7 +103,7 @@ func samplePublicCluster() flaps.ManagedPostgresCluster { } } -func sampleLegacyCluster() mpgv2.GetClusterResponse { +func sampleProxyLegacyCluster() mpgv2.GetClusterResponse { return mpgv2.GetClusterResponse{ Data: mpgv2.ManagedCluster{ Id: "mpg-123", Name: "test-cluster", Region: "ord", Status: "ready", @@ -133,7 +133,7 @@ func TestGetCluster(t *testing.T) { }{ { name: "public success maps to legacy shape", - publicCluster: samplePublicCluster(), + publicCluster: sampleProxyPublicCluster(), wantUseLegacy: false, wantDirectHost: "10.0.0.1", wantStatus: "ready", @@ -142,7 +142,7 @@ func TestGetCluster(t *testing.T) { { name: "public success with empty host preserves empty", publicCluster: func() flaps.ManagedPostgresCluster { - c := samplePublicCluster() + c := sampleProxyPublicCluster() c.Endpoints.Primary.Direct.Host = "" return c @@ -159,7 +159,7 @@ func TestGetCluster(t *testing.T) { ResponseStatusCode: 404, OriginalError: errors.New("not found"), }), - legacyResponse: sampleLegacyCluster(), + legacyResponse: sampleProxyLegacyCluster(), wantUseLegacy: true, wantDirectHost: "10.0.0.1", wantStatus: "ready", @@ -244,7 +244,7 @@ func TestGetMpgProxyParamsPublicNeverTouchesCredentials(t *testing.T) { ctx := flag.NewContext(context.Background(), pflag.NewFlagSet("test", pflag.ContinueOnError)) ctx = flapsutil.NewContextWithClient(ctx, &mock.FlapsClient{ GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { - cluster := samplePublicCluster() + cluster := sampleProxyPublicCluster() cluster.Status = "creating" cluster.Endpoints.Primary.Direct.Host = "" @@ -275,7 +275,7 @@ func TestGetMpgProxyParamsPublicNeverTouchesCredentials(t *testing.T) { func TestGetMpgConnectParamsResolvesCredentialsBeforeTunnel(t *testing.T) { ctx := flapsutil.NewContextWithClient(context.Background(), &mock.FlapsClient{ GetManagedPostgresClusterFunc: func(context.Context, string) (flaps.ManagedPostgresCluster, error) { - return samplePublicCluster(), nil + return sampleProxyPublicCluster(), nil }, GetManagedPostgresUserCredentialsFunc: func(context.Context, string, string) (flaps.ManagedPostgresUserCredentials, error) { return flaps.ManagedPostgresUserCredentials{Username: mpgutil.DefaultUsername}, nil @@ -543,7 +543,7 @@ func TestResolveConnectCredentialsPublicNonReady(t *testing.T) { for _, tt := range tests { t.Run(tt.status+"/default_user_public", func(t *testing.T) { - c := samplePublicCluster() + c := sampleProxyPublicCluster() c.Status = tt.status c.Name = name response, _ := publicToLegacyClusterResponse(c) @@ -564,7 +564,7 @@ func TestResolveConnectCredentialsPublicNonReady(t *testing.T) { }) t.Run(tt.status+"/explicit_user_public", func(t *testing.T) { - c := samplePublicCluster() + c := sampleProxyPublicCluster() c.Status = tt.status c.Name = name response, _ := publicToLegacyClusterResponse(c) @@ -589,7 +589,7 @@ func TestResolveConnectCredentialsPublicNonReady(t *testing.T) { func TestProxyParamsPublicPorts(t *testing.T) { for _, port := range []int{1, 5433, 65535} { t.Run(strconv.Itoa(port), func(t *testing.T) { - c := samplePublicCluster() + c := sampleProxyPublicCluster() c.Endpoints.Primary.Direct.Port = port response, advertisedPort := publicToLegacyClusterResponse(c) @@ -604,7 +604,7 @@ func TestProxyParamsPublicPorts(t *testing.T) { func TestGetMpgProxyParamsRejectsInvalidPublicPort(t *testing.T) { for _, port := range []int{0, -1, 65536} { t.Run(strconv.Itoa(port), func(t *testing.T) { - c := samplePublicCluster() + c := sampleProxyPublicCluster() c.Endpoints.Primary.Direct.Port = port ctx := flag.NewContext(context.Background(), pflag.NewFlagSet("test", pflag.ContinueOnError)) ctx = flapsutil.NewContextWithClient(ctx, &mock.FlapsClient{