From f4d2b5642085b887afa7100710a5cf67e295e4a4 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Wed, 12 Aug 2026 05:58:39 +0000 Subject: [PATCH] [infrastructure] Resolve the module directory in readonly mode and thread ctx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems with the lookup that finds this module's source for a suite in another module. A caller that vendors its dependencies puts the go command in automatic vendor mode, where `go list -m -f {{.Dir}}` succeeds and reports an EMPTY directory — vendor/ holds packages, not module source, so there is nothing for it to report. The harness took that for a missing module and told the caller to run `go mod download` for a dependency that was already there. Ask in readonly mode instead, which resolves against the module graph and answers for both a cached module and a local replacement, without writing to go.mod. The lookup also ran on context.Background(), so a StartCombined, StartClient or StartProxy whose context was cancelled could not stop it. repoRoot and moduleDir now take the caller's context. The vendor-mode case has a regression test, which needs no network: a temporary main module with a local replacement, vendored. It fails without the flag with the misleading "run `go mod download`" error. moduleDir takes the module path as a parameter so that test can name its own. The assertions move to testify, matching the repository's guidelines. --- e2e/harness/client.go | 2 +- e2e/harness/combined.go | 2 +- e2e/harness/options_test.go | 135 +++++++++++++++++++++--------------- e2e/harness/paths.go | 27 +++++--- e2e/harness/proxy.go | 2 +- 5 files changed, 100 insertions(+), 68 deletions(-) diff --git a/e2e/harness/client.go b/e2e/harness/client.go index 0dac79c07..0d7f016a6 100644 --- a/e2e/harness/client.go +++ b/e2e/harness/client.go @@ -61,7 +61,7 @@ func StartClient(ctx context.Context, c *Combined, setupKey string, opts ...Clie opt(&o) } - root, err := repoRoot() + root, err := repoRoot(ctx) if err != nil { return nil, err } diff --git a/e2e/harness/combined.go b/e2e/harness/combined.go index a1045a9a6..e03f9f256 100644 --- a/e2e/harness/combined.go +++ b/e2e/harness/combined.go @@ -122,7 +122,7 @@ func StartCombined(ctx context.Context, opts ...CombinedOption) (*Combined, erro opt(&o) } - root, err := repoRoot() + root, err := repoRoot(ctx) if err != nil { return nil, err } diff --git a/e2e/harness/options_test.go b/e2e/harness/options_test.go index 0e55ac462..9ebeba55c 100644 --- a/e2e/harness/options_test.go +++ b/e2e/harness/options_test.go @@ -3,11 +3,15 @@ package harness import ( + "context" "fmt" "os" + "os/exec" "path/filepath" - "strings" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // The options exist so a suite can ask for a deployment this harness would not @@ -18,18 +22,15 @@ import ( func TestCombinedEnvGeolocation(t *testing.T) { var off combinedOptions - if got := combinedEnv(off)["NB_DISABLE_GEOLOCATION"]; got != "true" { - t.Errorf("geolocation should be off by default, NB_DISABLE_GEOLOCATION = %q", got) - } + assert.Equal(t, "true", combinedEnv(off)["NB_DISABLE_GEOLOCATION"], + "geolocation should be off by default") var on combinedOptions WithGeolocation()(&on) - if _, set := combinedEnv(on)["NB_DISABLE_GEOLOCATION"]; set { - t.Error("WithGeolocation must leave NB_DISABLE_GEOLOCATION unset, so the server downloads the database") - } - if combinedEnv(on)["NB_SETUP_PAT_ENABLED"] != "true" { - t.Error("the setup PAT must stay enabled whatever else is configured; Bootstrap depends on it") - } + assert.NotContains(t, combinedEnv(on), "NB_DISABLE_GEOLOCATION", + "WithGeolocation must leave NB_DISABLE_GEOLOCATION unset, so the server downloads the database") + assert.Equal(t, "true", combinedEnv(on)["NB_SETUP_PAT_ENABLED"], + "the setup PAT must stay enabled whatever else is configured; Bootstrap depends on it") } // The config file carries the same decision as the environment variable, and the @@ -50,14 +51,10 @@ func TestCombinedConfigGeolocation(t *testing.T) { opt(&o) } cfg := fmt.Sprintf(combinedConfigYAML, combinedExposedURL, !o.geolocation, containerIssuer) - if !strings.Contains(cfg, tc.want) { - t.Errorf("config should contain %q, got:\n%s", tc.want, cfg) - } + assert.Contains(t, cfg, tc.want, "geolocation not rendered as expected") // The issuer is the last verb; a mis-ordered argument list would put // the boolean here instead and the server would fail to start. - if !strings.Contains(cfg, `issuer: "`+containerIssuer+`"`) { - t.Errorf("issuer not rendered, got:\n%s", cfg) - } + assert.Contains(t, cfg, `issuer: "`+containerIssuer+`"`, "issuer not rendered") }) } } @@ -68,12 +65,8 @@ func TestWithServerEnvOverrides(t *testing.T) { WithServerEnv(map[string]string{"NB_SETUP_PAT_ENABLED": "false"})(&o) env := combinedEnv(o) - if env["NB_LOG_LEVEL"] != "debug" { - t.Errorf("added variable missing, NB_LOG_LEVEL = %q", env["NB_LOG_LEVEL"]) - } - if env["NB_SETUP_PAT_ENABLED"] != "false" { - t.Errorf("a suite must be able to override a default, NB_SETUP_PAT_ENABLED = %q", env["NB_SETUP_PAT_ENABLED"]) - } + assert.Equal(t, "debug", env["NB_LOG_LEVEL"], "added variable missing") + assert.Equal(t, "false", env["NB_SETUP_PAT_ENABLED"], "a suite must be able to override a default") } // Two agents on one network cannot share an alias, so the name has to reach both @@ -81,14 +74,10 @@ func TestWithServerEnvOverrides(t *testing.T) { // also what the peer is addressable by through the API. func TestWithClientName(t *testing.T) { o := clientOptions{name: clientAlias} - if o.name != "client" { - t.Fatalf("unexpected default client name %q", o.name) - } + require.Equal(t, "client", o.name, "unexpected default client name") WithClientName("peer2")(&o) - if o.name != "peer2" { - t.Errorf("WithClientName did not take, name = %q", o.name) - } + assert.Equal(t, "peer2", o.name, "WithClientName did not take") } // repoRoot has to recognise this module rather than merely finding a go.mod, or a @@ -96,41 +85,77 @@ func TestWithClientName(t *testing.T) { // component Dockerfiles in it. func TestIsModule(t *testing.T) { dir := t.TempDir() + other := filepath.Join(dir, "go.mod") - if err := os.WriteFile(other, []byte("module example.com/other\n\ngo 1.25\n"), 0o600); err != nil { - t.Fatal(err) - } - if isModule(other, modulePath) { - t.Error("another module's go.mod must not be taken for this repo") - } + require.NoError(t, os.WriteFile(other, []byte("module example.com/other\n\ngo 1.25\n"), 0o600)) + assert.False(t, isModule(other, modulePath), "another module's go.mod must not be taken for this repo") ours := filepath.Join(dir, "ours.mod") - if err := os.WriteFile(ours, []byte("// a comment\n\nmodule "+modulePath+"\n\ngo 1.25\n"), 0o600); err != nil { - t.Fatal(err) - } - if !isModule(ours, modulePath) { - t.Error("this repo's go.mod was not recognised") - } + require.NoError(t, os.WriteFile(ours, []byte("// a comment\n\nmodule "+modulePath+"\n\ngo 1.25\n"), 0o600)) + assert.True(t, isModule(ours, modulePath), "this repo's go.mod was not recognised") - if isModule(filepath.Join(dir, "absent.mod"), modulePath) { - t.Error("a missing go.mod must not report a match") - } + assert.False(t, isModule(filepath.Join(dir, "absent.mod"), modulePath), + "a missing go.mod must not report a match") } // Running from inside the repo, repoRoot finds it by walking up — the module -// lookup is only the fallback, and this asserts the walk still wins so an -// in-repo run never depends on the module cache. +// lookup is only the fallback, and this asserts the walk still wins so an in-repo +// run never depends on the module cache. func TestRepoRootFindsThisRepo(t *testing.T) { - root, err := repoRoot() - if err != nil { - t.Fatalf("repoRoot: %v", err) - } - if !isModule(filepath.Join(root, "go.mod"), modulePath) { - t.Errorf("repoRoot returned %s, which is not this module", root) - } + root, err := repoRoot(context.Background()) + require.NoError(t, err) + assert.True(t, isModule(filepath.Join(root, "go.mod"), modulePath), + "repoRoot returned %s, which is not this module", root) + for _, f := range []string{combinedDockerfile, clientDockerfile} { - if _, err := os.Stat(filepath.Join(root, f)); err != nil { - t.Errorf("%s is not present under the reported root %s: %v", f, root, err) - } + _, err := os.Stat(filepath.Join(root, f)) + assert.NoError(t, err, "%s is not present under the reported root %s", f, root) } } + +// A caller that vendors its dependencies puts the go command in automatic vendor +// mode, where `go list -m -f {{.Dir}}` succeeds and reports an EMPTY directory: +// vendor/ holds packages, not module source. Without -mod=readonly the lookup +// would come back empty and the harness would report a missing module for a +// dependency that is present. +func TestModuleDirResolvesUnderVendorMode(t *testing.T) { + if _, err := exec.LookPath("go"); err != nil { + t.Skip("no go tool on PATH") + } + ctx := context.Background() + + base := t.TempDir() + dep := filepath.Join(base, "dep") + main := filepath.Join(base, "main") + require.NoError(t, os.MkdirAll(dep, 0o750)) + require.NoError(t, os.MkdirAll(main, 0o750)) + + // A local replacement rather than a real dependency, so this needs no network. + require.NoError(t, os.WriteFile(filepath.Join(dep, "go.mod"), + []byte("module example.com/dep\n\ngo 1.25\n"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(dep, "dep.go"), + []byte("package dep\n"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(main, "go.mod"), + []byte("module example.com/main\n\ngo 1.25\n\nrequire example.com/dep v0.0.0\n\nreplace example.com/dep v0.0.0 => ../dep\n"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(main, "main.go"), + []byte("package main\n\nimport _ \"example.com/dep\"\n\nfunc main() {}\n"), 0o600)) + + t.Chdir(main) + vendor := exec.CommandContext(ctx, "go", "mod", "vendor") + out, err := vendor.CombinedOutput() + require.NoError(t, err, "go mod vendor: %s", out) + + dir, err := moduleDir(ctx, "example.com/dep") + require.NoError(t, err, "the module must still resolve with a vendor directory present") + assert.Equal(t, dep, dir, "resolved the wrong directory") +} + +// A cancelled context has to stop the lookup rather than leaving the caller +// waiting on a subprocess it has already given up on. +func TestModuleDirHonoursContext(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := moduleDir(ctx, modulePath) + assert.Error(t, err, "a cancelled context must fail the lookup") +} diff --git a/e2e/harness/paths.go b/e2e/harness/paths.go index 550b8ad49..569c32efc 100644 --- a/e2e/harness/paths.go +++ b/e2e/harness/paths.go @@ -25,7 +25,7 @@ const modulePath = "github.com/netbirdio/netbird" // whichever version that suite depends on, which is the right one: the server it // tests against is then built from the same revision as the client library it // was compiled with. -func repoRoot() (string, error) { +func repoRoot(ctx context.Context) (string, error) { dir, err := os.Getwd() if err != nil { return "", err @@ -40,7 +40,7 @@ func repoRoot() (string, error) { } dir = parent } - return moduleDir() + return moduleDir(ctx, modulePath) } // isModule reports whether the go.mod at path declares the given module. @@ -57,21 +57,28 @@ func isModule(path, want string) bool { return false } -// moduleDir asks the go tool where this module's source is, which for a -// dependent module is its extracted copy in the module cache. The cache is -// read-only, and a Docker build context is only ever read. -func moduleDir() (string, error) { - cmd := exec.CommandContext(context.Background(), "go", "list", "-m", "-f", "{{.Dir}}", modulePath) +// moduleDir asks the go tool where a module's source is, which for a dependent +// module is its extracted copy in the module cache. The cache is read-only, and +// a Docker build context is only ever read. +// +// -mod=readonly is required rather than cosmetic. A caller that vendors its +// dependencies puts the go command in automatic vendor mode, where this lookup +// succeeds with an EMPTY directory — vendor/ holds packages, not module source, +// so there is nothing to report. Asking in readonly mode resolves against the +// module graph instead, which answers for both a cached module and a local +// replacement, and neither writes to go.mod. +func moduleDir(ctx context.Context, module string) (string, error) { + cmd := exec.CommandContext(ctx, "go", "list", "-mod=readonly", "-m", "-f", "{{.Dir}}", module) out, err := cmd.Output() if err != nil { - return "", fmt.Errorf("locate %s: %w", modulePath, err) + return "", fmt.Errorf("locate %s: %w", module, err) } dir := strings.TrimSpace(string(out)) if dir == "" { - return "", fmt.Errorf("locate %s: the go tool reported no directory; run `go mod download %s`", modulePath, modulePath) + return "", fmt.Errorf("locate %s: the go tool reported no directory; run `go mod download %s`", module, module) } if _, err := os.Stat(dir); err != nil { - return "", fmt.Errorf("locate %s: %w", modulePath, err) + return "", fmt.Errorf("locate %s: %w", module, err) } return dir, nil } diff --git a/e2e/harness/proxy.go b/e2e/harness/proxy.go index 85f3518d4..3d709b439 100644 --- a/e2e/harness/proxy.go +++ b/e2e/harness/proxy.go @@ -43,7 +43,7 @@ type Proxy struct { // or override any NB_PROXY_* var (e.g. NB_PROXY_TUNNEL_CACHE_TTL for tests that // need a short authorization-cache window). func StartProxy(ctx context.Context, c *Combined, proxyToken string, envOverrides ...map[string]string) (*Proxy, error) { - root, err := repoRoot() + root, err := repoRoot(ctx) if err != nil { return nil, err }