diff --git a/docs/testing.md b/docs/testing.md index 7a7fa96..832eeb6 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -196,24 +196,67 @@ mirrors. ### How much of the suite runs on Ministack -Deliberately almost none: exactly one end-to-end test -([ministack_integration_test.go](../internal/testutil/ministack_integration_test.go)) -covering provision → instance `available` → endpoint discovery → -`dbconn` connect → PG-major assertion → DDL smoke. Everything else — all -parser, planner, executor, and connection behavior — runs on the -data-plane tier against real PostgreSQL. That split is policy, not -accident: Ministack exists only for the seam where the engine talks to -AWS APIs, and its share grows only when AWS-facing features land, never -by moving core-logic tests onto it. Planned growth, in dependency order: +Deliberately almost none: one test, three subtests sharing one provisioned +cluster ([ministack_integration_test.go](../internal/testutil/ministack_integration_test.go)) +— provisioning costs minutes, so the tier provisions once and orders the +password rotation last. Each subtest pins one AWS seam: + +- **provision & connect** — provision → instance `available` → endpoint + discovery → `dbconn` connect → PG-major assertion → DDL smoke; +- **control-plane error contract** — unknown identifiers and duplicate + creations surface as the AWS SDK's typed RDS faults, matched with + `errors.As`. One documented emulator divergence: Ministack emits the + duplicate-instance wire code with a `Fault` suffix real AWS omits, so + the SDK leaves it a generic API error. The test pins the divergent code + exactly — it fails the day the emulator is fixed, forcing the + workaround's removal in favor of the typed match production code uses; +- **password rotation** — what a rotation does to a running schema change + (see below), plus pg-sprite's contract that the resulting auth failure + is terminal, not retryable. + +### What a password rotation does to a running schema change + +PostgreSQL authenticates a session at connection time only, so a master +password rotation does **not** break a schema change in flight — the +established sessions keep working, possibly for hours. The failure lands +on the next *dial*: the pool growing past its idle set, a +`MaxConnLifetime` recycle, or a reconnect after a network blip. That +delayed failure carries no causal link to the rotation an operator will +remember, which is exactly why it is pinned by a test: the stale +credentials fail with SQLSTATE `28P01`, and `dbconn.Retryable` classifies +that as terminal, so the engine surfaces one clean failure rather than +retrying against an endpoint that will keep refusing it. + +This is the design input for the planned credential-refresh hook: because +the failure is per-dial, the hook must resolve credentials per-dial +(`BeforeConnect`), not capture a password at pool construction. It matters +most where the engine dials twice far apart — an executor that acquires a +fresh connection for a post-failure verdict, hours into an index build, +must not lose a provable verdict to a rotation that happened in between. + +Everything else — all parser, planner, executor, and connection +behavior — runs on the data-plane tier against real PostgreSQL. That +split is policy, not accident: Ministack exists only for the seam where +the engine talks to AWS APIs, and its share grows only when AWS-facing +features land, never by moving core-logic tests onto it. Planned growth, +in dependency order: - reader/writer topology tests — writer-endpoint targeting with a reader - present, endpoint re-discovery after a global-cluster failover — once - the engine has endpoint-selection logic to test. Which endpoint a schema - change targets is a *safety* property, not a performance one: DDL against - a reader endpoint fails in confusing ways, and against the wrong cluster - member is worse — so when endpoint selection lands it must be visible in - the plan report, not just inside `dbconn`; -- Secrets Manager DSN resolution, when that feature lands. + present, endpoint re-discovery after a global-cluster failover + (metadata-level: every Ministack endpoint resolves to one shared + container) — once the engine has endpoint-selection logic to test. Which + endpoint a schema change targets is a *safety* property, not a + performance one: DDL against a reader endpoint fails in confusing ways, + and against the wrong cluster member is worse — so when endpoint + selection lands it must be visible in the plan report, not just inside + `dbconn`; +- Secrets Manager DSN resolution, when that feature lands; +- rotation *recovery* — the engine re-resolving credentials and + reconnecting mid-schema-change — once `pkg/dbconn` grows a + credential-refresh hook (per-dial, for the reason above). + +Logical-replication behavior is a data-plane concern and is tested on +real PostgreSQL, never on this tier. The harness and its test are behind the `ministack` build tag: a plain `go test ./...` (and therefore `make test`) never compiles them, so the @@ -239,7 +282,7 @@ stays unit-only so pushes remain fast. | Verify-full TLS against a live TLS-only server | [pkg/dbconn/tls_integration_test.go](../pkg/dbconn/tls_integration_test.go) | | Targeted blocker termination | [pkg/dbconn/dbconn_integration_test.go](../pkg/dbconn/dbconn_integration_test.go) | | Test harness self-checks | [internal/testutil](../internal/testutil/postgres_test.go) | -| RDS control-plane provisioning → endpoint discovery → `dbconn` connect (Ministack) | [internal/testutil/ministack_integration_test.go](../internal/testutil/ministack_integration_test.go) | +| RDS control-plane provisioning → endpoint discovery → `dbconn` connect, error contract, password-rotation behavior (Ministack) | [internal/testutil/ministack_integration_test.go](../internal/testutil/ministack_integration_test.go) | | Parse boundary, typed operations, and advisory rewrites | [pkg/statement](../pkg/statement/statement_test.go), [operation tests](../pkg/statement/ops_test.go) | | Native / copy-and-swap / refuse classification and safer SQL | [pkg/planner](../pkg/planner/planner_test.go) | | Backend routing and copy-and-swap unavailable disposition | [pkg/router](../pkg/router/router_test.go) | diff --git a/go.mod b/go.mod index 1a106ff..e582d9d 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/aws/aws-sdk-go-v2 v1.43.4 github.com/aws/aws-sdk-go-v2/credentials v1.17.5 github.com/aws/aws-sdk-go-v2/service/rds v1.124.1 + github.com/aws/smithy-go v1.27.6 github.com/jackc/pgx/v5 v5.10.0 github.com/moby/moby/api v1.54.2 github.com/moby/moby/client v0.4.0 @@ -25,7 +26,6 @@ require ( github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.35 // indirect github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.15 // indirect github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.35 // indirect - github.com/aws/smithy-go v1.27.6 // indirect github.com/cenkalti/backoff/v4 v4.3.0 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/containerd/errdefs v1.0.0 // indirect diff --git a/internal/testutil/ministack.go b/internal/testutil/ministack.go index 6ec66d2..e020c13 100644 --- a/internal/testutil/ministack.go +++ b/internal/testutil/ministack.go @@ -21,6 +21,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/aws-sdk-go-v2/service/rds" + "github.com/jackc/pgx/v5" "github.com/moby/moby/api/types/container" "github.com/moby/moby/api/types/network" "github.com/moby/moby/client" @@ -43,6 +44,10 @@ const ( // the sibling database container, which dominates this budget. auroraProvisionDeadline = 5 * time.Minute auroraProvisionPoll = 2 * time.Second + // rotationDeadline bounds how long a rotated master password may take + // to land on the running database after ModifyDBCluster returns. + rotationDeadline = time.Minute + rotationPoll = time.Second // rdsStatusAvailable is the RDS API status of a usable instance. rdsStatusAvailable = "available" // endpointDialTimeout bounds the reachability probe of the discovered @@ -109,12 +114,87 @@ func auroraEngineVersion(major int) string { return fmt.Sprintf("%d.1", major) } +// AuroraCluster is a provisioned Ministack aurora-postgresql cluster and +// the control-plane client that owns it. Tests drive further control-plane +// operations (rotation, duplicate creation, discovery of unknown +// identifiers) through Client against ClusterID and InstanceID. +type AuroraCluster struct { + // Client is the RDS control-plane client bound to the Ministack gateway. + Client *rds.Client + // ClusterID is the DBClusterIdentifier of the provisioned cluster. + ClusterID string + // InstanceID is the DBInstanceIdentifier of the cluster's sole instance. + InstanceID string + + // addr is the host:port the test connects to — the discovered cluster + // endpoint when reachable, otherwise the sibling container's + // host-published address (see ProvisionAuroraPostgres). + addr string + // password is the master password the cluster currently accepts. + // Rotate keeps it in sync with the control plane so URL never goes + // silently stale after a rotation. + password string +} + +// URL returns a connection URL for the cluster's database using the +// master password the cluster currently accepts. After Rotate, that is +// the rotated password. +func (c *AuroraCluster) URL() string { + return c.URLWithPassword(c.password) +} + +// URLWithPassword returns a connection URL using the given master +// password — for tests that deliberately present stale or wrong +// credentials. +// +// sslmode=disable: the sibling database container runs plain PostgreSQL +// without TLS, and the endpoint is not an *.rds.amazonaws.com hostname, +// so the production TLS path is out of scope for this tier (it is +// proven by pkg/dbconn's TLS integration tests). +func (c *AuroraCluster) URLWithPassword(password string) string { + return fmt.Sprintf("postgres://%s:%s@%s/%s?sslmode=disable", + fixtureUser, password, c.addr, fixtureDatabase) +} + +// Rotate changes the cluster's master password through ModifyDBCluster, +// waits until the running database accepts the new password, and updates +// the handle so URL reflects the credentials the cluster now accepts. +// The previous password remains available to the caller for +// deliberately-stale connections via URLWithPassword. +func (c *AuroraCluster) Rotate(t *testing.T, newPassword string) { + t.Helper() + ctx := t.Context() + _, err := c.Client.ModifyDBCluster(ctx, &rds.ModifyDBClusterInput{ + DBClusterIdentifier: aws.String(c.ClusterID), + MasterUserPassword: aws.String(newPassword), + ApplyImmediately: aws.Bool(true), + }) + require.NoError(t, err, "rotate master password via ModifyDBCluster") + + // The rotation must land on the real database, not just the control + // plane's metadata: poll until the new password authenticates. + rotatedURL := c.URLWithPassword(newPassword) + require.Eventuallyf(t, func() bool { + conn, err := pgx.Connect(ctx, rotatedURL) + if err != nil { + return false + } + if err := conn.Close(ctx); err != nil { + t.Logf("close rotation probe connection: %v", err) + } + return true + }, rotationDeadline, rotationPoll, + "rotated master password did not become usable within the deadline") + + c.password = newPassword +} + // ProvisionAuroraPostgres starts a Ministack container, provisions an // aurora-postgresql cluster and instance through the real RDS control-plane -// API, waits until the instance is available, and returns a connection URL -// for the cluster's database. The PostgreSQL major follows PG_VERSION: the -// cluster's database is a real postgres container of that major. -func ProvisionAuroraPostgres(t *testing.T) string { +// API, waits until the instance is available, and returns the cluster +// handle. The PostgreSQL major follows PG_VERSION: the cluster's database +// is a real postgres container of that major. +func ProvisionAuroraPostgres(t *testing.T) *AuroraCluster { t.Helper() if os.Getenv("SKIP_INTEGRATION") != "" { t.Skip("SKIP_INTEGRATION set; skipping test that needs Docker") @@ -229,12 +309,13 @@ func ProvisionAuroraPostgres(t *testing.T) string { addr = siblingHostAddr(t, ctr, clusterID) } - // sslmode=disable: the sibling database container runs plain PostgreSQL - // without TLS, and the endpoint is not an *.rds.amazonaws.com hostname, - // so the production TLS path is out of scope for this tier (it is - // proven by pkg/dbconn's TLS integration tests). - return fmt.Sprintf("postgres://%s:%s@%s/%s?sslmode=disable", - fixtureUser, fixturePassword, addr, fixtureDatabase) + return &AuroraCluster{ + Client: clnt, + ClusterID: clusterID, + InstanceID: instanceID, + addr: addr, + password: fixturePassword, + } } // tcpReachable reports whether addr accepts a TCP connection within diff --git a/internal/testutil/ministack_integration_test.go b/internal/testutil/ministack_integration_test.go index 57df284..c2196b9 100644 --- a/internal/testutil/ministack_integration_test.go +++ b/internal/testutil/ministack_integration_test.go @@ -7,6 +7,11 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/rds" + "github.com/aws/aws-sdk-go-v2/service/rds/types" + "github.com/aws/smithy-go" + "github.com/jackc/pgx/v5/pgconn" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -14,16 +19,27 @@ import ( "github.com/block/pg-sprite/pkg/dbconn" ) -// TestAuroraControlPlaneProvisionAndConnect proves the AWS-boundary flow -// end to end: an aurora-postgresql cluster provisioned through the real RDS -// control-plane API is discoverable, its endpoint accepts connections -// through pkg/dbconn (bounded session defaults included), the server runs -// the requested PostgreSQL major, and DDL executes. -func TestAuroraControlPlaneProvisionAndConnect(t *testing.T) { - url := testutil.ProvisionAuroraPostgres(t) +// TestAuroraControlPlane proves the AWS-boundary seams against one shared +// provisioned cluster: provisioning a cluster costs minutes, so the +// subtests share it rather than provisioning three times. They run in +// order, and PasswordRotation runs last because it changes the cluster's +// master password. +func TestAuroraControlPlane(t *testing.T) { + cluster := testutil.ProvisionAuroraPostgres(t) + t.Run("ProvisionAndConnect", func(t *testing.T) { provisionAndConnect(t, cluster) }) + t.Run("ErrorContract", func(t *testing.T) { errorContract(t, cluster) }) + t.Run("PasswordRotation", func(t *testing.T) { passwordRotation(t, cluster) }) +} + +// provisionAndConnect proves the AWS-boundary flow end to end: an +// aurora-postgresql cluster provisioned through the real RDS control-plane +// API is discoverable, its endpoint accepts connections through pkg/dbconn +// (bounded session defaults included), the server runs the requested +// PostgreSQL major, and DDL executes. +func provisionAndConnect(t *testing.T, cluster *testutil.AuroraCluster) { pool, err := dbconn.NewPool(t.Context(), dbconn.Config{ - URL: url, + URL: cluster.URL(), LockTimeout: 300 * time.Millisecond, }) require.NoError(t, err, "connect to provisioned cluster endpoint via dbconn") @@ -51,3 +67,103 @@ func TestAuroraControlPlaneProvisionAndConnect(t *testing.T) { require.NoError(t, pool.QueryRow(t.Context(), "SELECT to_regclass($1)::oid", schema+".t").Scan(&oid)) assert.NotNil(t, oid, "created table must be visible in the catalog") } + +// errorContract proves the control-plane error contract the engine's +// discovery code will rely on: unknown identifiers and duplicate creations +// surface as the AWS SDK's typed RDS faults, matchable with errors.As — +// never by message text. +func errorContract(t *testing.T, cluster *testutil.AuroraCluster) { + ctx := t.Context() + + _, err := cluster.Client.DescribeDBClusters(ctx, &rds.DescribeDBClustersInput{ + DBClusterIdentifier: aws.String("pgsprite-does-not-exist"), + }) + var clusterNotFound *types.DBClusterNotFoundFault + require.ErrorAs(t, err, &clusterNotFound, + "describing an unknown cluster must surface the typed not-found fault") + + _, err = cluster.Client.CreateDBCluster(ctx, &rds.CreateDBClusterInput{ + DBClusterIdentifier: aws.String(cluster.ClusterID), + Engine: aws.String("aurora-postgresql"), + MasterUsername: aws.String("pgsprite"), + MasterUserPassword: aws.String("test-password-do-not-use"), + }) + var clusterExists *types.DBClusterAlreadyExistsFault + require.ErrorAs(t, err, &clusterExists, + "creating a duplicate cluster must surface the typed already-exists fault") + + _, err = cluster.Client.CreateDBInstance(ctx, &rds.CreateDBInstanceInput{ + DBInstanceIdentifier: aws.String(cluster.InstanceID), + DBClusterIdentifier: aws.String(cluster.ClusterID), + Engine: aws.String("aurora-postgresql"), + DBInstanceClass: aws.String("db.t3.medium"), + }) + // Real AWS emits wire code "DBInstanceAlreadyExists", which the SDK + // maps to types.DBInstanceAlreadyExistsFault — production code must + // match that typed fault with errors.As, exactly like the two cases + // above. Ministack diverges: it emits "DBInstanceAlreadyExistsFault", + // which the SDK leaves as a generic API error. Pin the divergent code + // exactly so this assertion fails the day the emulator is fixed, and + // this workaround is replaced by the typed errors.As match. + var apiErr smithy.APIError + require.ErrorAs(t, err, &apiErr, + "creating a duplicate instance must surface an RDS API error") + require.Equal(t, "DBInstanceAlreadyExistsFault", apiErr.ErrorCode(), + "emulator no longer emits its divergent duplicate-instance code — assert types.DBInstanceAlreadyExistsFault with errors.As instead of this pin") +} + +// passwordRotation proves what a master-password rotation does to a +// running schema change, and pins pg-sprite's contract for the failure. +// PostgreSQL never re-authenticates an established session, so in-flight +// work keeps running through the rotation; the stale credentials fail on +// the next dial — a pool recycle, growth past the idle set, or a +// reconnect — with SQLSTATE 28P01, which pg-sprite classifies as +// terminal: one clean failure, never a retry storm against an +// auth-failing endpoint. +func passwordRotation(t *testing.T, cluster *testutil.AuroraCluster) { + // A pool dialed with the pre-rotation password, with one session + // checked out — a schema change in flight. + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{ + URL: cluster.URL(), + LockTimeout: 300 * time.Millisecond, + }) + require.NoError(t, err, "connect with the pre-rotation password") + t.Cleanup(pool.Close) + held, err := pool.Acquire(t.Context()) + require.NoError(t, err, "check out a session before the rotation") + var result int + require.NoError(t, held.QueryRow(t.Context(), "SELECT 1").Scan(&result)) + + const rotatedPassword = "test-password-rotated-do-not-use" + cluster.Rotate(t, rotatedPassword) + + // The established session sails through the rotation: PostgreSQL + // authenticates at connection time only. + require.NoError(t, held.QueryRow(t.Context(), "SELECT 2").Scan(&result), + "an established session must keep working through a rotation") + assert.Equal(t, 2, result) + held.Release() + + // The failure lands on the next dial. Reset stands in for the ways a + // pool re-dials in production — MaxConnLifetime expiry, growth past + // the idle set, a reconnect after a network blip. + pool.Reset() + _, err = pool.Acquire(t.Context()) + var pgErr *pgconn.PgError + require.ErrorAs(t, err, &pgErr, "a dial with the stale password must fail with a server auth error") + assert.Equal(t, "28P01", pgErr.Code, "stale password must be refused as invalid_password") + + // pg-sprite's own contract for that failure: auth errors are terminal, + // not transient — the engine surfaces one clean failure instead of + // retrying against an endpoint that will keep refusing it. + assert.False(t, dbconn.Retryable(err), "an auth failure must not be classified as retryable") + + // The rotated credentials connect through dbconn; the handle's URL + // reflects them after Rotate. + fresh, err := dbconn.NewPool(t.Context(), dbconn.Config{ + URL: cluster.URL(), + LockTimeout: 300 * time.Millisecond, + }) + require.NoError(t, err, "connect with the rotated password") + fresh.Close() +}