Skip to content

testutil: AWS-boundary test tier via Ministack RDS/Aurora - #11

Open
Kiran01bm wants to merge 1 commit into
kiran01bm/p2-5-linterfrom
kiran01bm/ministack-aurora
Open

testutil: AWS-boundary test tier via Ministack RDS/Aurora#11
Kiran01bm wants to merge 1 commit into
kiran01bm/p2-5-linterfrom
kiran01bm/ministack-aurora

Conversation

@Kiran01bm

Copy link
Copy Markdown
Collaborator

Summary

Adds the AWS-boundary test tier: an aurora-postgresql cluster is provisioned through the real RDS control-plane API against Ministack (MIT-licensed, tokenless), and the test proves provisioning → instance available → endpoint discovery → connect through pkg/dbconn. Because Ministack needs no license key, the tier runs on every code PR — forks included — as a real merge gate.

What

  • internal/testutil/ministack.goProvisionAuroraPostgres: starts a pinned ministackorg/ministack:1.4.13-full container (Docker socket mounted so it can run the sibling PostgreSQL container backing the cluster), provisions cluster + instance via the AWS SDK, polls to available, discovers the endpoint, and returns a connection URL on a pinned host-published port.
  • internal/testutil/ministack_integration_test.go — end-to-end assertions: server major matches PG_VERSION, dbconn bounded session settings apply, DDL executes and is introspectable.
  • make test-aws-boundary target and an aws-boundary CI job wired into all-green.
  • docs/testing.md — the three-tier statement (data plane / AWS boundary / real Aurora), with each tier's "proves / cannot prove" spelled out.

Why

The engine's production seam with AWS (RDS control plane, endpoint discovery, later Secrets Manager DSN resolution and IAM auth) had no test tier at all. Ministack covers exactly that seam without weakening the data-plane rule: core logic keeps running against real PostgreSQL, and the provisioned database is a real postgres:<major> container that follows PG_VERSION across the full 14 → 18 matrix — this tier never stands in for real Aurora semantics.

Notable constraints found by live-running the harness:

  • the full image is required — the slim edition's readiness falls back to a TCP probe that can never succeed from inside the emulator container in this topology, leaving the instance creating forever;
  • the connection uses a pinned RDS_BASE_PORT host port rather than the API-returned endpoint address, because container IPs are not host-routable on macOS;
  • the emulator mounts the host Docker socket, so the tier must only run on Docker hosts it owns (ephemeral CI runners, local dev).
┌────────────┐  RDS API   ┌───────────────┐  docker.sock  ┌──────────────────┐
│ test (Go)  │───────────▶│ Ministack ctr │──────────────▶│ postgres:<major> │
│            │            │ (gateway 4566)│    starts     │ sibling ctr      │
│            │◀───────────│ endpoint info │               │                  │
│            │            └───────────────┘               │                  │
│            │  pgx connect (pinned host port)            │                  │
│            │────────────────────────────────────────────▶                  │
└────────────┘                                            └──────────────────┘

References

  • Ministack — MIT-licensed AWS emulator backing the tier
  • docs/testing.md — three-tier statement and the "How much of the suite runs on Ministack" policy

Ministack (MIT, tokenless) replaces the LocalStack plan: the tier now
runs on every code PR as a real merge gate, and the provisioned database
follows PG_VERSION across the full 14-18 range. The full image is
required so instance readiness is an authenticated probe, and the
connection uses a pinned host-published port because the emulator's
container-internal endpoint address is not routable from macOS hosts.
@Kiran01bm
Kiran01bm marked this pull request as ready for review August 6, 2026 10:25
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@aparajon

aparajon commented Aug 6, 2026

Copy link
Copy Markdown

🤖 Review requested by Armand and performed by his agent — same two lenses used across this stack (#10, #15, #14, #9, #8, #7, #2): pg-sprite as an OSS-first, best-in-class Postgres DDL tool, and pg-sprite as a clean integration target for an orchestrator. Reviewed at head 2c556d3. An adversarial correctness pass is posted separately.

The documentation in this PR is the best part of it and I want to be specific about why: the tier table's "Cannot prove" column, and the "How much of the suite runs on Ministack — deliberately almost none" section, are the two things that stop an emulator tier from metastasizing. Every project that adds one of these ends up, eighteen months later, with core logic quietly running against the emulator because it was faster, and nobody able to say which claims are still real. Writing the ceiling down before the tier has any weight is the only thing that has ever prevented that, and the "its share grows only when AWS-facing features land, never by moving core-logic tests onto it" sentence is exactly the right form of the rule. The tokenless/MIT choice so forks get the same gate is also the right call, and the LIFO cleanup ordering comment — instance delete before cluster delete, because RDS refuses to drop a cluster with instances — is the kind of detail that's obvious only after it has bitten you.

My concern is timing rather than direction. The adversarial comment shows this tier currently has nothing of pg-sprite's on the tested path: no production code makes an AWS API call, and the test's assertions all pass unchanged against a plain PostgreSQL container in 1.5 seconds. So the harness and the docs are an investment in a seam that doesn't exist yet — which is fine — but the PR wires it into the required merge gate today.

OSS lens

  1. A required merge gate should only fail for reasons the author can act on. aws-boundary is in all-green's needs, so a Docker Hub rate-limit, a Ministack image tag going away, a regression in the emulator's readiness probe, or the freePort race (adversarial finding 4) blocks every merge in the repo — for a tier that, today, asserts nothing about pg-sprite that the data-plane tier doesn't already assert. The asymmetry is worth noting against Docs: vision, architecture, status sweep; tag-driven release machinery #2, which found the opposite problem on the release path: tagging publishes binaries with no gate at all. I'd land the harness, the Makefile target, and the docs exactly as written, and either leave the job out of all-green or run it on a schedule until the first AWS-facing feature lands — then promote it, at which point a failure means something an author can fix.

  2. The dependency rubric applies and wasn't invoked. SAFETY.md requires a recorded decision for a new dependency ("copy small things, take pinned dependencies only for load-bearing expertise"), and this adds fourteen AWS modules plus smithy-go. The rubric is arguably about core dependencies only — but internal/testutil/ministack.go is a plain .go file, not a _test.go one, so the SDK sits in the module graph for anyone who runs go install github.com/block/pg-sprite/cmd/pg-sprite or vendors the repo. A //go:build ministack tag (or moving the harness into a _test.go) confines it to people who ask for it, and either way this is worth a line in SAFETY.md's dependency note so the next reader can see the decision was made rather than defaulted into.

  3. The Docker-socket requirement is enforced by a comment. Both the Go source and the CI job say clearly that this must never run on a shared or self-hosted runner, which is the right instinct — but the enforcement is prose. runs-on: ubuntu-latest is a literal today and a variable the first time someone adds a runner label. If the constraint matters (it does — the emulator gets the daemon), it's worth a guard in the job or the target: refuse when a RUNNER_ENVIRONMENT other than github-hosted is set, or require an explicit opt-in variable so a self-hosted move fails loudly instead of silently granting daemon access.

  4. The tier can't cover the one AWS behavior that exists in production code today, and it's worth saying so in the table. pkg/dbconn/rds.go is the entire current AWS surface: IsRDSHost's hostname regex, the embedded global bundle, and verify-full. No emulator can exercise that end to end, because the bundle is Amazon's real CA and nothing local can present a chain to it. That isn't a flaw in this PR — it's a structural gap that an emulator tier can't close, and the honest fix is a seam (an injectable root pool, so a generated CA can stand in) rather than more infrastructure. The "Cannot prove" column is exactly the right place to record it; right now a reader could reasonably infer the RDS TLS path is what "AWS boundary" means.

Integration lens

  1. The planned-growth list is the right list, and IAM auth is the item that justifies the whole tier — consider landing it alongside. Secrets Manager resolution and RDS IAM tokens through dbconn.Config.BeforeConnect are genuine pg-sprite logic with real failure modes an emulator is uniquely good at: a token that expires mid-run, a refresh that races a reconnect, a BeforeConnect that silently reuses a stale token. That is the first moment this harness will catch something the data-plane tier can't, and it would also settle question 1 by making the gate meaningful the day it turns on.

  2. Reader/writer topology is the higher-value item on that list for an orchestrator, and it's listed first — good. Worth adding one line about what an orchestrator needs from it: which endpoint a schema change targets is a safety property, not a performance one (running DDL against a reader endpoint fails in confusing ways, and against the wrong cluster member is worse). When endpoint selection lands it should be visible in the plan report per Add pkg/plan: one versioned dry-run report for both front doors #8, not just inside dbconn.

  3. PGVersion() drives both tiers, which is a nice property worth stating as an invariant. The provisioned database follows PG_VERSION exactly like the data-plane containers, so the two tiers can't silently drift onto different majors. That's load-bearing for the "never a substitute" rule — a tier pinned to one major would quietly become the place version-specific behavior hides. The test already asserts it (server_version_num / 10000); the docs could claim it explicitly.

Verified solid

The harness reads the endpoint back from DescribeDBClusters rather than trusting what it sent, which is the right shape for a discovery test even though the value is then unused (see the adversarial comment). Cleanup ordering is correct and correctly explained — t.Cleanup LIFO puts the instance delete ahead of the cluster delete, matching RDS's own rule — and it goes through the API rather than the container, because the sibling database is a peer of the Ministack container and terminating Ministack alone would leak it. That's a real leak the author clearly hit and fixed. The image is pinned with a documented MINISTACK_IMAGE escape hatch, and the full-vs-slim note explains a failure mode (a TCP readiness probe passing before PostgreSQL accepts logins) that would otherwise cost the next person a day. Fixture credentials are named as fixtures at the point of definition. The changes.outputs.code guard keeps docs-only PRs off the tier. CGO_ENABLED=0 go build ./... passes at this head.

This review was generated by Claude Code (claude-fable-5).

@aparajon

aparajon commented Aug 6, 2026

Copy link
Copy Markdown

🤖 Adversarial correctness review requested by Armand and performed by his agent — separate from the two-lens pass. Method: for a test tier, the attack is "what would still pass if the thing under test were removed". I took the tier's own assertions, swapped Ministack for a plain PostgreSQL container, and ran them at head 2c556d3. The reproduction is in the collapsed block at the end.

Findings, most severe first

1. No pg-sprite code is on the path this tier tests, and every assertion passes without Ministack. TestAuroraControlPlaneProvisionAndConnect asserts three things: the server major matches PG_VERSION, dbconn applies its bounded session settings, and DDL executes and is introspectable. I ran those three assertions verbatim against testutil.StartPostgres(t) instead of ProvisionAuroraPostgres(t):

--- PASS: TestAdvSameAssertionsWithoutMinistack (1.47s)

All three pass, in a second and a half, with no emulator, no AWS SDK, and no Docker socket. That is because pg-sprite makes no AWS API calls at allgreping for the SDK outside internal/testutil returns nothing, and pkg/dbconn/rds.go, the entire current AWS surface, is a hostname regex plus an embedded CA bundle. So the provisioning flow under test is the AWS SDK's RDS client talking to Ministack: two third-party components agreeing with each other, with none of the code this repo ships in between.

This isn't an argument against the harness — the PR's own "planned growth" list names the features that will put real code on this seam, and it's a reasonable list. It's an argument about what the tier can be claimed to prove today, and specifically about wiring it into all-green now (raised in the lens comment). The version-major assertion is genuinely worth keeping — it catches an emulator silently substituting a different image — but it's a test of Ministack, which is a fine thing to have and a different thing from a merge gate on pg-sprite.

2. The docs claim the test connects to the discovered endpoint; the code deliberately connects around it. docs/testing.md's "Proves" cell reads "endpoint discovery, connecting to the discovered endpoint through pkg/dbconn". The code does discover the endpoint, asserts it is non-empty, and then throws it away:

require.NotEmpty(t, aws.ToString(clusters.DBClusters[0].Endpoint), …)
require.NotZero(t, aws.ToInt32(clusters.DBClusters[0].Port), …)

// The discovered endpoint address is container-internal; connect via the
// pinned host-published port instead (see dbPort above).
return fmt.Sprintf("postgres://%s:%s@localhost:%d/%s?sslmode=disable", …)

The reason is legitimate and well explained — container IPs aren't host-routable on macOS. But the consequence is that "connect to the discovered endpoint" is proven only to the extent of "the string was not empty", and the connection is made to an address the test computed for itself before provisioning even started. The dbPort it connects to is the same port it passed in as RDS_BASE_PORT, so nothing about the returned endpoint feeds the connection. Given how careful the rest of the tier table is about the proves/cannot-prove boundary, this cell should say what it does: discovery returns a well-formed endpoint; the connection uses a side channel because the returned address is not reachable from the host.

3. make test runs this tier too, so the Docker-socket mount is not opt-in. make test is go test -race ./..., which includes ./internal/testutil/, and TestAuroraControlPlaneProvisionAndConnect is guarded only by SKIP_INTEGRATION. So the documented rule — "run it only on Docker hosts you own" — is violated by the default command AGENTS.md tells contributors to run, on their own laptop, with their personal Docker daemon. In CI the same thing means the tier runs twice per PR: once inside the test job and again in the dedicated aws-boundary job, doubling both the wall-clock cost and the flake surface of a required gate. A build tag (//go:build ministack) on the harness and its test makes make test-aws-boundary the only way in, which is what the docs already describe.

4. freePort is a documented TOCTOU race, now on the required merge gate, with a five-minute detection cost. The helper binds :0, reads the port, closes the listener, and returns the number — and says so:

// The port is released before returning, so a collision is possible but
// unlikely within a test's lifetime.

The window here isn't a test's lifetime, though: the port is handed to Ministack as RDS_BASE_PORT and isn't bound until the emulator later starts the sibling container, which is after the image pull. On a busy runner (the test job's own testcontainers are running concurrently — see finding 3), that's a wide window. And the failure isn't a clean bind error the test can report: the sibling container fails to publish, the instance never reaches available, and require.Eventuallyf burns the full auroraProvisionDeadline before failing with "did not become available", which reads as an emulator bug rather than a port collision. Holding the listener open until Ministack starts, or retrying on collision, converts a five-minute mystery into an instant retry.

5. config.LoadDefaultConfig drags in the whole credential chain for a client that immediately overrides it. The client is built with static credentials, but LoadDefaultConfig still pulls the discovery machinery — visible in the module downloads:

aws-sdk-go-v2/service/sso, service/ssooidc, service/sts,
feature/ec2/imds, internal/ini, internal/configsources,
service/internal/presigned-url, internal/endpoints/v2

Fourteen AWS modules plus smithy-go land in go.mod for a harness that needs service/rds and a static credentials provider. Constructing aws.Config{Region: …, Credentials: …} directly instead drops most of that. It also removes a real hazard for a test harness: LoadDefaultConfig reads ~/.aws/config, AWS_PROFILE, and the EC2 instance-metadata endpoint, so a developer with an AWS profile set has environment leaking into a test that is supposed to be hermetic — and on an EC2-hosted runner, an IMDS probe that has to time out.

6. EngineVersion is a bare major, which real RDS is unlikely to accept. EngineVersion: aws.String(strconv.Itoa(major)) sends "16" for aurora-postgresql. Real RDS engine versions are full version strings (16.4), and this is exactly the class of call-shape difference an emulator smooths over. It doesn't matter while the tier only ever talks to Ministack — but the tier's stated purpose is to exercise "the real RDS control-plane API", so a call the emulator accepts and AWS would reject is the specific failure mode worth guarding against. If the tier is meant to be a rehearsal for real API calls, it's worth constructing them the way AWS requires even where Ministack is lenient.

Probed and held

The cleanup path is correct and I tried to break it: t.Cleanup runs LIFO, so the instance delete genuinely precedes the cluster delete, matching RDS's refusal to drop a cluster that still has instances — and both go through the API rather than the container, because the sibling database is a peer of the Ministack container and would otherwise leak. Cleanup uses context.WithoutCancel(t.Context()), correctly, since t.Context() is already cancelled by then. Fixture credentials are inert and labelled as fixtures where they're defined. The full-image requirement is a real constraint, not cargo-culting: the slim edition's TCP readiness probe can report available before PostgreSQL accepts logins. The image is pinned with an MINISTACK_IMAGE override, and the needs.changes.outputs.code guard keeps the tier off docs-only PRs. PGVersion() genuinely drives the provisioned major, and the test asserts it rather than assuming it. CGO_ENABLED=0 go build ./... passes at this head.

Reproduction

internal/testutil/adv11_integration_test.go — the tier's assertions with Ministack removed
package testutil_test

import (
	"strconv"
	"testing"
	"time"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/internal/testutil"
	"github.com/block/pg-sprite/pkg/dbconn"
)

// The AWS-boundary test's assertions, verbatim, with the Ministack
// provisioning replaced by a plain PostgreSQL container. If they all still
// pass, no assertion in that test depends on the RDS control plane.
func TestAdvSameAssertionsWithoutMinistack(t *testing.T) {
	url := testutil.StartPostgres(t)

	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{
		URL:         url,
		LockTimeout: 300 * time.Millisecond,
	})
	require.NoError(t, err, "connect via dbconn")
	t.Cleanup(pool.Close)

	requestedMajor, err := strconv.Atoi(testutil.PGVersion())
	require.NoError(t, err)
	var versionNum int
	require.NoError(t, pool.QueryRow(t.Context(),
		"SELECT current_setting('server_version_num')::int").Scan(&versionNum))
	assert.Equal(t, requestedMajor, versionNum/10000, "server major must match the requested major")

	var lockTimeout string
	require.NoError(t, pool.QueryRow(t.Context(), "SHOW lock_timeout").Scan(&lockTimeout))
	assert.Equal(t, "300ms", lockTimeout)

	schema := testutil.NewSchema(t, pool)
	_, err = pool.Exec(t.Context(), "CREATE TABLE "+schema+".t (id bigint PRIMARY KEY)")
	require.NoError(t, err)
	var oid *uint32
	require.NoError(t, pool.QueryRow(t.Context(),
		"SELECT to_regclass($1)::oid", schema+".t").Scan(&oid))
	assert.NotNil(t, oid)
}
--- PASS: TestAdvSameAssertionsWithoutMinistack (1.47s)

This review was generated by Claude Code (claude-fable-5). Finding 1 was reproduced with the test above; findings 2–6 are static analysis of the harness, Makefile, and workflow.

@aparajon

aparajon commented Aug 6, 2026

Copy link
Copy Markdown

🤖 Correction to my adversarial comment above, from the same agent.

Finding 3 said the tier running inside make test was undocumented — that "the documented rule … is violated by the default command AGENTS.md tells contributors to run". That was wrong, and I should have read further down the file I was quoting from. docs/testing.md states it plainly in this same PR:

The tier runs in CI as the aws-boundary merge-gate job and inside make test when Docker is available.

So it is documented, deliberate, and not a contradiction the author missed. What stands from that finding is the cost rather than the surprise: the tier runs twice per PR (once inside the test job's ./..., once in the dedicated aws-boundary job), and a contributor running make test on their own machine gets the Docker-socket mount without asking for it — which is worth a note next to the "run it only on Docker hosts you own" guidance, since a laptop is exactly the host that guidance is about. The build-tag suggestion still applies if you want make test-aws-boundary to be the only way in; it's a preference, not a fix for a defect.

Apologies for the mischaracterization — the rest of that comment stands as written.

This correction was generated by Claude Code (claude-fable-5).

@aparajon aparajon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving on Armand's behalf. My two-lens review and adversarial correctness pass are posted above — the findings there are for follow-up, not fix-before-merge blockers.

This approval was submitted by Claude Code (claude-fable-5) at Armand's direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants