From 77e5ac776bf8f37f35a31be2a8444e8659e0d67f Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Wed, 12 Aug 2026 18:19:25 +0900 Subject: [PATCH] [infrastructure] Let a suite outside this repo use the e2e harness (#7176) e2e/harness documents itself as feature-agnostic, but three details assumed the caller lives in this repo, so the terraform provider's acceptance suite would otherwise carry a second harness for the same product. repoRoot took the first module root above the working directory as the Docker build context, which from another module is the caller's own root, with no combined/Dockerfile.multistage in it. It now requires that ancestor to be this module, and otherwise asks the go tool for the source: for a dependent, the extracted directory of the version it pins, so the server matches the client library it was compiled against. That lookup uses -mod=readonly, since automatic vendor mode otherwise reports an empty Dir. Geolocation was disabled unconditionally. Agent-network ingest does not use it, but location-based posture checks need the database, and a rule management cannot evaluate fails rather than passing. StartClient pinned one network alias and set no hostname, so a second agent could not start and a peer's name was arbitrary. Management records that hostname, making it the peer's name in the API. The client entrypoint is copied with an explicit mode: git tracks it 100755, but the module cache extracts 0444, so a dependent's build produced a container exiting with "permission denied". Adds CombinedOption, WithGeolocation, WithServerEnv, ClientOption and WithClientName. --- e2e/harness/Dockerfile.client | 6 +- e2e/harness/client.go | 36 +++++++- e2e/harness/combined.go | 72 ++++++++++++--- e2e/harness/config.go | 7 +- e2e/harness/options_test.go | 161 ++++++++++++++++++++++++++++++++++ e2e/harness/paths.go | 67 ++++++++++++-- e2e/harness/proxy.go | 2 +- 7 files changed, 328 insertions(+), 23 deletions(-) create mode 100644 e2e/harness/options_test.go diff --git a/e2e/harness/Dockerfile.client b/e2e/harness/Dockerfile.client index 114577d60..74a3ec245 100644 --- a/e2e/harness/Dockerfile.client +++ b/e2e/harness/Dockerfile.client @@ -20,5 +20,9 @@ ENV NETBIRD_BIN="/usr/local/bin/netbird" \ NB_ENABLE_CAPTURE="false" \ NB_ENTRYPOINT_SERVICE_TIMEOUT="30" ENTRYPOINT [ "/usr/local/bin/netbird-entrypoint.sh" ] -COPY client/netbird-entrypoint.sh /usr/local/bin/netbird-entrypoint.sh +# --chmod because the build context is not always a git checkout. A suite in +# another module builds from this module's extracted copy in the module cache, +# where every file is 0444 — the cache drops the executable bit git records — and +# a bare COPY then produces an entrypoint the runtime cannot exec. +COPY --chmod=0755 client/netbird-entrypoint.sh /usr/local/bin/netbird-entrypoint.sh COPY --from=builder /out/netbird /usr/local/bin/netbird diff --git a/e2e/harness/client.go b/e2e/harness/client.go index f53d0ea64..0d7f016a6 100644 --- a/e2e/harness/client.go +++ b/e2e/harness/client.go @@ -32,12 +32,36 @@ type Client struct { container testcontainers.Container } +// clientOptions is what the ClientOption values assemble. +type clientOptions struct { + name string +} + +// ClientOption adjusts how StartClient runs the agent. +type ClientOption func(*clientOptions) + +// WithClientName names the agent, which sets both its network alias and its +// container hostname. The hostname matters beyond addressing: the agent reports +// it to management at registration, so it is the name the peer appears under in +// the API. +// +// Required to run more than one agent against the same server — the default name +// is shared, and two containers cannot hold the same alias on one network. +func WithClientName(name string) ClientOption { + return func(o *clientOptions) { o.name = name } +} + // StartClient builds the client image and runs it on the combined server's // network, joining via the given setup key. The image entrypoint brings the // daemon up automatically; callers wait for connectivity with WaitConnected / // WaitProxyPeer. -func StartClient(ctx context.Context, c *Combined, setupKey string) (*Client, error) { - root, err := repoRoot() +func StartClient(ctx context.Context, c *Combined, setupKey string, opts ...ClientOption) (*Client, error) { + o := clientOptions{name: clientAlias} + for _, opt := range opts { + opt(&o) + } + + root, err := repoRoot(ctx) if err != nil { return nil, err } @@ -47,9 +71,13 @@ func StartClient(ctx context.Context, c *Combined, setupKey string) (*Client, er } req := testcontainers.ContainerRequest{ - Image: clientImage, + Image: clientImage, + // The agent reports the container's hostname to management, so this is + // the name the peer is addressable by in the API as well as on the + // network. The entrypoint takes no hostname flag of its own. + Hostname: o.name, Networks: []string{c.network.Name}, - NetworkAliases: map[string][]string{c.network.Name: {clientAlias}}, + NetworkAliases: map[string][]string{c.network.Name: {o.name}}, Env: map[string]string{ "NB_MANAGEMENT_URL": combinedExposedURL, "NB_SETUP_KEY": setupKey, diff --git a/e2e/harness/combined.go b/e2e/harness/combined.go index b2f0d89d2..e03f9f256 100644 --- a/e2e/harness/combined.go +++ b/e2e/harness/combined.go @@ -61,11 +61,68 @@ type Combined struct { workDir string } +// combinedOptions is what the CombinedOption values assemble. +type combinedOptions struct { + geolocation bool + env map[string]string +} + +// CombinedOption adjusts how StartCombined boots the server. The defaults suit a +// suite that only drives the API; the options exist for the ones that need more +// of the product than that. +type CombinedOption func(*combinedOptions) + +// WithGeolocation leaves the GeoLite database download enabled. It is off by +// default because the download adds startup latency that most suites get nothing +// for. A suite asserting on location-based posture checks needs it: management +// evaluates those rules against the database, and without it the rule fails +// instead of passing without having been checked. +func WithGeolocation() CombinedOption { + return func(o *combinedOptions) { o.geolocation = true } +} + +// WithServerEnv adds environment variables to the combined container, overriding +// the defaults on a key collision. For settings this harness does not model +// directly, so a suite needing one does not have to fork the harness to get it. +func WithServerEnv(env map[string]string) CombinedOption { + return func(o *combinedOptions) { + if o.env == nil { + o.env = map[string]string{} + } + for k, v := range env { + o.env[k] = v + } + } +} + +// combinedEnv is the combined container's environment: setup-PAT enabled so the +// caller can mint an admin token through /api/setup, geolocation off unless the +// suite asked for it, and whatever the suite added on top. +func combinedEnv(o combinedOptions) map[string]string { + env := map[string]string{ + "NB_SETUP_PAT_ENABLED": "true", + } + if !o.geolocation { + // Skip the GeoLite DB download — it blocks startup and agent-network + // ingest doesn't use geolocation. + env["NB_DISABLE_GEOLOCATION"] = "true" + } + for k, v := range o.env { + env[k] = v + } + return env +} + // StartCombined builds the combined server from its multistage Dockerfile and // boots it with setup-PAT enabled on a fresh shared network, returning once the // API is serving. The caller still owns minting the admin PAT via Bootstrap. -func StartCombined(ctx context.Context) (*Combined, error) { - root, err := repoRoot() +func StartCombined(ctx context.Context, opts ...CombinedOption) (*Combined, error) { + var o combinedOptions + for _, opt := range opts { + opt(&o) + } + + root, err := repoRoot(ctx) if err != nil { return nil, err } @@ -88,7 +145,7 @@ func StartCombined(ctx context.Context) (*Combined, error) { return nil, fmt.Errorf("create work dir: %w", err) } - cfg := fmt.Sprintf(combinedConfigYAML, combinedExposedURL, containerIssuer) + cfg := fmt.Sprintf(combinedConfigYAML, combinedExposedURL, !o.geolocation, containerIssuer) if err := os.WriteFile(filepath.Join(workDir, "config.yaml"), []byte(cfg), 0o644); err != nil { //nolint:gosec // non-secret config, bind-mounted and read by the container _ = net.Remove(ctx) return nil, fmt.Errorf("write combined config: %w", err) @@ -112,13 +169,8 @@ func StartCombined(ctx context.Context) (*Combined, error) { ExposedPorts: []string{combinedHTTPPort}, Networks: []string{net.Name}, NetworkAliases: map[string][]string{net.Name: {combinedAlias}}, - Env: map[string]string{ - "NB_SETUP_PAT_ENABLED": "true", - // Skip the GeoLite DB download — it blocks startup and agent-network - // ingest doesn't use geolocation. - "NB_DISABLE_GEOLOCATION": "true", - }, - Cmd: []string{"--config", "/nb/config.yaml"}, + Env: combinedEnv(o), + Cmd: []string{"--config", "/nb/config.yaml"}, HostConfigModifier: func(hc *container.HostConfig) { hc.Binds = append(hc.Binds, workDir+":/nb") }, diff --git a/e2e/harness/config.go b/e2e/harness/config.go index 71b3656c5..f0952b18c 100644 --- a/e2e/harness/config.go +++ b/e2e/harness/config.go @@ -15,6 +15,11 @@ package harness // server is required to load it — a broken path or malformed file fails startup // rather than silently falling back to the compiled-in rates, and TestMain then // fails with the container logs. +// +// disableGeoliteUpdate is a parameter rather than a fixed true because a suite +// that exercises geolocation needs the database: management can only evaluate a +// location rule with GeoLite loaded, and a rule it cannot evaluate fails rather +// than passing vacuously. See WithGeolocation. const combinedConfigYAML = `server: listenAddress: ":8080" exposedAddress: "%s" @@ -25,7 +30,7 @@ const combinedConfigYAML = `server: authSecret: "e2e-relay-secret" dataDir: "/nb/data" disableAnonymousMetrics: true - disableGeoliteUpdate: true + disableGeoliteUpdate: %t auth: issuer: "%s" store: diff --git a/e2e/harness/options_test.go b/e2e/harness/options_test.go new file mode 100644 index 000000000..8a5557a83 --- /dev/null +++ b/e2e/harness/options_test.go @@ -0,0 +1,161 @@ +//go:build e2e + +package harness + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "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 +// otherwise give it. What they configure is a container environment and a config +// file, both assembled before anything is started, so they are checkable without +// Docker — which is the point: a wiring mistake here would otherwise only show up +// as a puzzling failure minutes into a container run. + +func TestCombinedEnvGeolocation(t *testing.T) { + var off combinedOptions + assert.Equal(t, "true", combinedEnv(off)["NB_DISABLE_GEOLOCATION"], + "geolocation should be off by default") + + var on combinedOptions + WithGeolocation()(&on) + 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 +// server needs both to agree: disableGeoliteUpdate suppresses the download even +// when geolocation itself is enabled. +func TestCombinedConfigGeolocation(t *testing.T) { + for _, tc := range []struct { + name string + opts []CombinedOption + want string + }{ + {name: "default", want: "disableGeoliteUpdate: true"}, + {name: "with geolocation", opts: []CombinedOption{WithGeolocation()}, want: "disableGeoliteUpdate: false"}, + } { + t.Run(tc.name, func(t *testing.T) { + var o combinedOptions + for _, opt := range tc.opts { + opt(&o) + } + cfg := fmt.Sprintf(combinedConfigYAML, combinedExposedURL, !o.geolocation, containerIssuer) + 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. + assert.Contains(t, cfg, `issuer: "`+containerIssuer+`"`, "issuer not rendered") + }) + } +} + +func TestWithServerEnvOverrides(t *testing.T) { + var o combinedOptions + WithServerEnv(map[string]string{"NB_LOG_LEVEL": "debug"})(&o) + WithServerEnv(map[string]string{"NB_SETUP_PAT_ENABLED": "false"})(&o) + + env := combinedEnv(o) + 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 +// the alias and the hostname. The hostname is the one management records, so it is +// also what the peer is addressable by through the API. +func TestWithClientName(t *testing.T) { + o := clientOptions{name: clientAlias} + require.Equal(t, "client", o.name, "unexpected default client name") + + WithClientName("peer2")(&o) + 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 +// suite in another module gets its own root and a build context without the +// component Dockerfiles in it. +func TestIsModule(t *testing.T) { + dir := t.TempDir() + + other := filepath.Join(dir, "go.mod") + 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") + 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") + + 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. +func TestRepoRootFindsThisRepo(t *testing.T) { + 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} { + _, 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.ErrorIs(t, err, context.Canceled, "a cancelled context must stop the lookup") +} diff --git a/e2e/harness/paths.go b/e2e/harness/paths.go index d7df6bbfa..569c32efc 100644 --- a/e2e/harness/paths.go +++ b/e2e/harness/paths.go @@ -3,27 +3,82 @@ package harness import ( + "context" "fmt" "os" + "os/exec" "path/filepath" + "strings" ) -// repoRoot walks up from the working directory to the module root (the -// directory holding go.mod), so the Docker build context is correct no matter -// which package the test runs from. -func repoRoot() (string, error) { +// modulePath is this module, used both to recognise the repo when walking up +// from the working directory and to locate it when the suite lives elsewhere. +const modulePath = "github.com/netbirdio/netbird" + +// repoRoot returns the directory the component Dockerfiles are built from. +// +// Walking up from the working directory finds it for any test inside this repo, +// no matter which package it runs from. A suite in another module gets a +// different answer that way — its own module root, where combined/Dockerfile +// does not exist — so the ancestor has to be this module and not merely some +// module. When it is not, the build context is the extracted module directory of +// 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(ctx context.Context) (string, error) { dir, err := os.Getwd() if err != nil { return "", err } for { - if _, statErr := os.Stat(filepath.Join(dir, "go.mod")); statErr == nil { + if isModule(filepath.Join(dir, "go.mod"), modulePath) { return dir, nil } parent := filepath.Dir(dir) if parent == dir { - return "", fmt.Errorf("go.mod not found above %s", dir) + break } dir = parent } + return moduleDir(ctx, modulePath) +} + +// isModule reports whether the go.mod at path declares the given module. +func isModule(path, want string) bool { + b, err := os.ReadFile(path) + if err != nil { + return false + } + for _, line := range strings.Split(string(b), "\n") { + if rest, ok := strings.CutPrefix(strings.TrimSpace(line), "module "); ok { + return strings.TrimSpace(rest) == want + } + } + return false +} + +// 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", 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`", module, module) + } + if _, err := os.Stat(dir); err != nil { + 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 }