test: gomod git auth: remove need for custom buildx instance#938
test: gomod git auth: remove need for custom buildx instance#938cpuguy83 wants to merge 3 commits intoproject-dalec:mainfrom
Conversation
e1fe15c to
2f4e9ff
Compare
2f4e9ff to
c4de3a2
Compare
This should speed up the test since we don't have to re-cache everything in the new buildx instance. It also means we don't need to setup any listeners in the host network namespace. It accomplishes this by connecting directly to the git server's container IP address which we detect when we start the server. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
243ccec to
3b900af
Compare
Still more to do here, but trying to consolidate things, remove some abstractions that aren't really needed, and make the flow more clear. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
3b900af to
e28fb18
Compare
Our go.mod requires 1.25 now, so update the one in the test so it can pass. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
There was a problem hiding this comment.
Pull request overview
Updates the gomod_git_auth integration test setup to avoid creating a custom Buildx instance with host networking by discovering the git server container’s IP and wiring it into BuildKit via extra host entries.
Changes:
- Removes the dedicated “network=host” Buildx builder helper and related test runner wrapper.
- Refactors
TestGomodGitAuthto start HTTP/SSH git servers in BuildKit containers and use their container IPs for connectivity. - Introduces a small git server helper binary and a JSON event protocol for readiness/IP reporting.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/testenv/buildx.go | Removes helpers related to host-networked custom buildx instances. |
| test/gomod_git_auth_test.go | Switches tests to use server container IPs and standard RunTest opts (secrets/ssh socket). |
| test/git_services/teststate.go | Refactors git server startup to emit/read readiness events and build/mount helper server binary. |
| test/git_services/protocol.go | Adds event structs/constants for newline-delimited JSON server events. |
| test/git_services/cmd/server/main.go | Adds helper server supporting serve and getip commands. |
| test/git_services/attributes.go | Removes old GitRemoteAddr and per-test tag regeneration helper. |
| test/cmd/git_repo/passwd/passwd.go | Deletes obsolete password helper package. |
| test/cmd/git_repo/host.go | Deletes obsolete HTTP git server host program. |
| test/cmd/git_repo/build/build.go | Deletes obsolete build helper for the old HTTP server. |
| testEnv.RunTest(ctx, t, func(ctx context.Context, client gwclient.Client) { | ||
| testState := gitservices.NewTestState(t, client, &attr) | ||
|
|
There was a problem hiding this comment.
attr is shared across parallel subtests and NewTestState mutates attr.tag when it’s empty. This introduces a data race and also reintroduces the earlier caching problem where HTTP/SSH can reuse the same git tag, potentially causing false positives. Make attr a per-subtest copy (e.g., shadow attr := attr inside each subtest) so each run gets its own tag (or explicitly generate a unique tag per subtest).
| testEnv.RunTest(ctx, t, func(ctx context.Context, client gwclient.Client) { | ||
| testState := gitservices.NewTestState(t, client, &attr) | ||
|
|
There was a problem hiding this comment.
Same issue as the HTTP subtest: this parallel subtest shares &attr and NewTestState may mutate attr.tag, causing a race. Also, reusing the same tag across subtests can allow the go module cache to satisfy the second test without actually exercising auth. Use a per-subtest copy / unique tag.
| httpServerBin := ts.buildHTTPGitServer(ctx) | ||
| mounts := []gwclient.Mount{ | ||
| {Dest: "/", Ref: ts.stateToRef(ctx, rootfs)}, | ||
| {Dest: ts.Attr.HTTPServerPath, Selector: filepath.Base(ts.Attr.HTTPServerPath), Ref: ts.stateToRef(ctx, httpServerBin)}, |
There was a problem hiding this comment.
gwclient.Mount.Dest is set to the full binary path (/usr/local/bin/git_http_server) while also using Selector. Within this repo, mounts that use Selector treat Dest as the directory mountpoint (e.g., source_test.go:1434-1435 mounts a selected file into /mnt). With the current code, BuildKit may mount the selected file as the root at Dest (or fail if it expects a directory), leaving the binary not present at HTTPServerPath. Mount the ref at filepath.Dir(HTTPServerPath) and keep Selector as the basename so the file ends up at the expected path.
| {Dest: ts.Attr.HTTPServerPath, Selector: filepath.Base(ts.Attr.HTTPServerPath), Ref: ts.stateToRef(ctx, httpServerBin)}, | |
| {Dest: filepath.Dir(ts.Attr.HTTPServerPath), Selector: filepath.Base(ts.Attr.HTTPServerPath), Ref: ts.stateToRef(ctx, httpServerBin)}, |
| "bufio" | ||
| "bytes" | ||
| "context" | ||
| _ "embed" |
There was a problem hiding this comment.
The _ "embed" import appears unused (there are no //go:embed directives in this file). It’s harmless to the compiler, but it’s dead weight and may be flagged by linters; consider removing it unless you’re about to add embed directives.
| _ "embed" |
| event := gitservices.ServerEvent{ | ||
| Type: gitservices.EventTypeError, | ||
| Error: &gitservices.ErrorEvent{Message: msg}, | ||
| } | ||
| json.NewEncoder(os.Stderr).Encode(event) //nolint:errcheck |
There was a problem hiding this comment.
emitError encodes the JSON error event to stderr, but the protocol comment says events are sent on stdout and the test harness only decodes events from stdout. As a result, structured error events won’t be observed and failures degrade into generic decode/exit errors. Emit error events to stdout (and write plain logs to stderr) so the caller can reliably detect/print server errors.
| event := gitservices.ServerEvent{ | |
| Type: gitservices.EventTypeError, | |
| Error: &gitservices.ErrorEvent{Message: msg}, | |
| } | |
| json.NewEncoder(os.Stderr).Encode(event) //nolint:errcheck | |
| // Log a plain-text error message to stderr for human-readable logs. | |
| fmt.Fprintln(os.Stderr, msg) | |
| // Emit the structured error event to stdout, as required by the protocol. | |
| event := gitservices.ServerEvent{ | |
| Type: gitservices.EventTypeError, | |
| Error: &gitservices.ErrorEvent{Message: msg}, | |
| } | |
| json.NewEncoder(os.Stdout).Encode(event) //nolint:errcheck |
This should speed up the test since we don't have to re-cache everything in the new buildx instance.
It also means we don't need to setup any listeners in the host network namespace.
It accomplishes this by connecting directly to the git server's container IP address which we detect when we start the server.
There are likely some other improvements we can do here but the goal was to remove the custom buildx instance for this change.