deps: moby/buildkit v0.28+ upgrade via testcontainers-go v0.42 migration (closes #64, #65)#935
Merged
oskarszoon merged 8 commits intoMay 26, 2026
Conversation
Prepares the cascade required by the moby/buildkit v0.28+ upgrade. testcontainers-go v0.40 -> v0.42 changes: - MappedPort return type: nat.Port -> network.Port - wait.ForSQL callback signature: func(host string, port nat.Port) string -> func(host string, port string) string testcontainers-aerospike-go v0.3.2 -> v0.3.7 absorbs the same network.Port change in the fork. Repo is intentionally non-compiling at this commit; source-file migration follows in the next commit. Issue: bsv-blockchain#926
…0.42 testcontainers-go v0.42 changed the testcontainers Container.MappedPort signature from (ctx, nat.Port) (nat.Port, error) to (ctx, string) (network.Port, error). Updates callers across the test/ packages: - Drop docker/go-connections/nat imports; switch to github.com/moby/moby/api/types/network. - MappedPort/GetMappedPort callers now pass raw "<num>/tcp" strings. - Result is the new network.Port struct; .Int() replaced with int(p.Num()), .Port() unchanged. - wait.ForSQL callback signature: func(host, port string) string (port pre-extracted). - test/utils/svnode/docker.go: HostConfigModifier now receives moby/moby/api/types/container.HostConfig; PortBindings/PortMap/PortBinding migrated to network.* equivalents; PortBinding.HostIP is now netip.Addr (use netip.IPv4Unspecified()). No behavior changes. Build clean after this commit. Issue: bsv-blockchain#926
Move docker/docker and docker/go-connections to indirect (no longer used directly after the nat.Port -> network.Port migration), and surface moby/moby/api as direct (now imported by test/utils/svnode/docker.go). Issue: bsv-blockchain#926
The testcontainers v0.42 migration introduced net/netip but placed it after the first third-party import. Move it back to the stdlib block to satisfy gci/goimports. Issue: bsv-blockchain#926
The previous migration commit (3f7a3bde) inlined the `port` argument directly into the connection string. testcontainers-go v0.42 hands the URL builder the full network.Port.String() representation ("5432/tcp"), not just the port number — the result was an unparseable DSN "postgres://...:5432/tcp/postgres" and the container failed to come up under Test_processTransactionInternalPostgres.
Mirror the canonical pattern from testcontainers-go/modules/postgres@v0.42.0: parse the port via network.MustParsePort and use .Port() to extract the numeric component.
Issue: bsv-blockchain#926
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: This PR successfully migrates testcontainers-go from v0.40 to v0.42 and bumps moby/buildkit to v0.29.0, closing Dependabot security alerts #64 and #65. The migration is well-executed with systematic API surface changes applied consistently across test files. Changes verified:
No issues found. The migration follows the testcontainers v0.42 breaking changes correctly, addresses the security vulnerabilities as intended, and maintains test integrity. |
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-22 15:02 UTC |
testcontainers-go v0.42 stopped accepting the legacy "<host>:<container>/proto" combined string in ContainerRequest.ExposedPorts (rejects with "invalid start port"). Host-port pinning now must go through HostConfigModifier's PortBindings. Migrates four redpanda test launchers to: - ExposedPorts: ["<port>/tcp"] (container side only) - HostConfigModifier sets network.PortMap with explicit host:container binding via netip.IPv4Unspecified() for 0.0.0.0 Fixes CI failures introduced by the v0.42 migration: - util/kafka TestFlushBytesRegression_MessageTooLarge - util/kafka TestFlushBytesRegression_DefaultBatchMax Files updated: - util/kafka/kafkatest/kafkatest.go (MustStartEnv) - util/kafka/flush_bytes_regression_test.go (startRedpanda) - test/longtest/util/kafka/container.go (RunTestContainer + RunTestContainerTLS) - test/longtest/util/kafka/kafka_test.go (NewTestContainerWrapper) Issue: bsv-blockchain#926
Two related changes: 1. RunPostgresTestContainer: defensively Terminate the container on every error path between GenericContainer and the final connectivity check. GenericContainer(Started:true) starts the container before running the wait strategy; if the strategy returns a non-nil error (as happened with the wait.ForSQL signature bug from earlier in this branch, but also possible on legitimate timeouts) the previous code returned without calling Terminate, leaving a running postgres container per failed run. 2. Restrict the Ryuk-reaper opt-out to CI. Ryuk pulls testcontainers/ryuk from Docker Hub which is the documented flake source for CI rate limits; that is why the global init() set TESTCONTAINERS_RYUK_DISABLED=true. On local dev machines the reaper is the only safety net against panic/SIGKILL/OOM leaks (we explicitly cleanup on graceful exit but nothing covers ungraceful exit). Detect CI via the CI env var and only disable the reaper there; locally the reaper runs and auto-removes containers when the test process dies. Users can override either way by exporting TESTCONTAINERS_RYUK_DISABLED explicitly. Issue: bsv-blockchain#926
|
freemans13
approved these changes
May 22, 2026
ordishs
approved these changes
May 26, 2026
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
Approved.
Closes Dependabot alerts #64 and #65 cleanly via the testcontainers-go v0.42 cascade. The nat.Port → network.Port migration is mechanical and consistent across all 8 touched test files. Bundled wins:
- Postgres container-leak fix on partial
GenericContainerfailure - Ryuk gating refined to CI-only (preserves local panic/SIGKILL cleanup)
- Full AGENTS.md verification battery green, including smoke + sequential
Non-blocking follow-ups for later:
Microsoft/hcsshimv0.14.0-rc.1 is transitive RC — bump off once GA landsk8s.io0.33.3 → 0.35.2 andstructured-merge-diffv4 → v6 are sizeable jumps; worth a sanity check if kuberesolver paths flake post-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes Dependabot alerts #64 (BuildKit malicious frontend file escape) and #65 (BuildKit Git URL subdir traversal). Deferred from #925; tracked in #926.
Bumps
github.com/moby/buildkitv0.25.1 → v0.29.0 via the testcontainers-go cascade:testcontainers-gov0.40.0 → v0.42.0 (+modules/compose,modules/postgres)testcontainers-aerospike-gov0.3.2 → v0.3.7moby/buildkitupdated transitively to v0.29.0 (past v0.28.1 fix line)Source migration
nat.Port→network.Portacross 8 test files (testcontainers v0.42 surface change:MappedPortnow returnsnetwork.Portstruct frommoby/moby/api/types/network; takes a rawstring;wait.ForSQLcallback signature changed;HostConfiglives atmoby/moby/api/types/container;PortBinding.HostIPis nownetip.Addr).5 commits:
chore(deps): bump testcontainers-go to v0.42 + aerospike fork to v0.3.7refactor(test): migrate nat.Port -> network.Port for testcontainers v0.42chore(deps): go mod tidy after testcontainers v0.42 migrationstyle(test): fix import ordering in svnode/docker.gofix(test): parse port in wait.ForSQL callback for postgres containerTest plan
AGENTS.md verification battery (post-rebase smoke):
go build ./...go test ./...— 8445 pass, 0 failgo test -race ./services/propagation/...— 110 pass (full race run pre-rebase had only the postgres bug; now fixed)go vet ./...— same 4 pre-existing baseline issues as chore(deps): bump minor versions for vulnerability fixes #925golangci-lint run,staticcheck ./...— cleangovulncheck ./...— zeromoby/buildkitfindings (alerts Ensure first transaction in subtree is coinbase placeholder #64, Legacy improvements to support teratestnet network #65 closed); remaining vulns are Go stdlib + libp2p + deferred docker (matches chore(deps): bump minor versions for vulnerability fixes #925 baseline)gosec ./...— 717 pre-existing findings, no newmake smoketest— deferred to CImake sequentialtest— deferred to CINotes
test/longtest/util/postgres/container.gofailed to compile on its own after the testcontainers bump (wait.ForSQLcallback signature). The other 9 files were migrated anyway for hygiene (no more danglingnat.Port/docker/go-connections/natreferences intest/).network.Portis a struct, not a string-alias. Methods:.Num() uint16,.Port() string. The aerospike-go fork's v0.3.7 bump absorbed the API change.HostConfig.PortBindingsinmoby/moby/api/types/containerusesnetwork.PortMapkeyed bynetwork.Portstructs (viaMustParsePort);PortBinding.HostIPisnetip.Addr(usenetip.IPv4Unspecified()for0.0.0.0).Closes #926.