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 }