Merge remote-tracking branch 'origin/main' into agent-network/gateway-protocol-conformance

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

e2e/harness/client.go

@@ -33,12 +33,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
 	}
@@ -48,9 +72,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,

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")
 		},

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:

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")
+}

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
 }

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
 	}