diff --git a/e2e/agentnetwork/settings_cluster_validation_test.go b/e2e/agentnetwork/settings_cluster_validation_test.go index 65ad5f451..af0382ad5 100644 --- a/e2e/agentnetwork/settings_cluster_validation_test.go +++ b/e2e/agentnetwork/settings_cluster_validation_test.go @@ -27,11 +27,13 @@ import ( // to a cluster that cannot serve it has to be refused up front rather than // leaving the account with a dead gateway. // -// One combined server, two proxies in the same cluster: the centralised one -// makes the cluster live-but-unusable, the embedded one added afterwards -// makes it usable (the capability is any-true across the cluster's live -// proxies), so both the refusal and the acceptance are exercised against the -// same account and the same cluster address. +// One combined server and one cluster address, walked through three states: +// a live centralised proxy (refused), that proxy stopped so nothing in the +// cluster is live any more (still refused — the record of what the cluster is +// outlives its heartbeats), and finally an embedded proxy (accepted, the +// capability being any-true across the cluster's live proxies). Same account, +// same address, so nothing but the cluster's state accounts for the different +// answers. func TestSettingsBootstrapValidatesProxyCluster(t *testing.T) { ctx := context.Background() @@ -49,6 +51,7 @@ func TestSettingsBootstrapValidatesProxyCluster(t *testing.T) { "NB_PROXY_PRIVATE": "false", }) require.NoError(t, err, "start centralised proxy") + // Terminated mid-test; the cleanup only covers an early failure. t.Cleanup(func() { _ = central.Terminate(context.Background()) }) waitClusterPrivate(ctx, t, fresh, cluster, false) @@ -66,6 +69,21 @@ func TestSettingsBootstrapValidatesProxyCluster(t *testing.T) { assert.Empty(t, after.Endpoint, "a refused bootstrap must not assign an endpoint") assert.Empty(t, after.ProxyAddress, "a refused bootstrap must not pin a cluster") + // Stopping the centralised proxy must not turn the refusal into an + // acceptance: the cluster's proxy rows outlive their heartbeats (only the + // hourly stale reaper removes them), so the cluster is still on record as + // one that cannot serve the gateway. Judging on liveness instead would + // make "wait for the proxy to go quiet" a way to pin the account's + // immutable endpoint to a cluster that can never serve it. + require.NoError(t, central.Terminate(ctx), "stop the centralised proxy") + waitClusterAbsent(ctx, t, fresh, cluster) + + _, err = fresh.CreateSettings(ctx, api.AgentNetworkSettingsCreateRequest{ + ProxyAddress: ptr(cluster), + }) + require.Error(t, err, "an offline cluster with no embedded proxy on record must stay refused") + requireClientError(t, err) + // Add an embedded proxy to the same cluster: now it can serve a private // service, and the very same request must go through. embedded, err := harness.StartProxy(ctx, fresh, proxyToken) @@ -120,3 +138,36 @@ func waitClusterPrivate(ctx context.Context, t *testing.T, c *harness.Combined, } t.Fatalf("cluster %s never reported supports_private=%v: %s", clusterAddr, want, last) } + +// waitClusterAbsent polls the domains endpoint until clusterAddr is no longer +// offered, i.e. management sees no live proxy in it. The free-domain list is +// built from the active clusters, so this is how a proxy going away becomes +// observable — while the cluster's rows, and so its capability record, remain. +func waitClusterAbsent(ctx context.Context, t *testing.T, c *harness.Combined, clusterAddr string) { + t.Helper() + + deadline := time.Now().Add(90 * time.Second) + var last string + for time.Now().Before(deadline) { + domains, err := c.API().ReverseProxyDomains.List(ctx) + if err != nil { + last = "list domains: " + err.Error() + } else { + listed := false + for _, d := range domains { + if d.Domain == clusterAddr { + listed = true + break + } + } + if !listed { + return + } + last = "cluster still listed as active" + } + if !waitBeforeRetry(ctx, 2*time.Second) { + break + } + } + t.Fatalf("cluster %s never dropped out of the active list: %s", clusterAddr, last) +} diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index d470bd284..7954cb46e 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -878,33 +878,45 @@ func (m *managerImpl) bootstrapSelfAddressed(ctx context.Context, settings *type // proxy — and the endpoint it allocates is immutable, so the account is left // with a dead gateway that only a DeleteSettings/re-bootstrap can undo. // -// Only a cluster management can actually judge is rejected: one whose live -// proxies have reported their capabilities. A cluster nothing is connected to -// is left alone, because pinning ahead of the proxy's first connection is a -// legitimate order (the dedicated path claims an address the same way, before -// any proxy declares it). +// Whether management knows the cluster is decided on the proxy rows +// themselves, never on how fresh their heartbeats are: a cluster's rows +// outlive its proxies' liveness (only the stale-proxy reaper removes them), so +// a cluster that exists stays judged as one. Judging on liveness instead would +// make the same centralised cluster pass or fail depending on whether its +// proxies happened to have heartbeated in the last couple of minutes. +// +// The single opening left is a cluster management holds no proxy row for at +// all: pinning ahead of a proxy's first connection is a legitimate order — the +// dedicated path claims an address the same way, before any proxy declares it. func (m *managerImpl) validateGatewayCluster(ctx context.Context, accountID, clusterAddr string) error { - private := m.store.GetClusterSupportsPrivate(ctx, clusterAddr) - if private == nil { - // No live proxy in the cluster reported its capabilities: either - // nothing is connected there yet, or the proxies predate capability - // reporting. Nothing to judge — let the pin through. - return nil - } - - available, err := m.accountClusterAddresses(ctx, accountID) + known, err := m.accountKnowsCluster(ctx, accountID, clusterAddr) if err != nil { return err } - if !slices.Contains(available, clusterAddr) { - // Live, but not a cluster this account may route through: another - // account's BYOP cluster. Its proxies filter foreign mappings out on - // delivery, so the pin would be dead on arrival. - return status.Errorf(status.InvalidArgument, - "proxy cluster %s is not available to this account", clusterAddr) + + if !known { + // Not in the account's view. A shared cluster would have been in it, + // so a proxy row elsewhere for this address can only be another + // account's BYOP cluster: its proxies filter foreign mappings out on + // delivery, making the pin dead on arrival. + foreign, err := m.store.IsClusterAddressConflicting(ctx, clusterAddr, accountID) + if err != nil { + return fmt.Errorf("check proxy cluster ownership: %w", err) + } + if foreign { + return status.Errorf(status.InvalidArgument, + "proxy cluster %s is not available to this account", clusterAddr) + } + // No proxy has ever declared this address: an address-first pin. + return nil } - if !*private { + // A cluster management knows has to prove it can serve the gateway, and + // only a live embedded proxy proves that. Both an explicit false and an + // unreported capability (nothing live in the cluster, or proxies predating + // capability reporting) fail here: unusable and unproven are the same + // answer for a decision that cannot be revisited later. + if private := m.store.GetClusterSupportsPrivate(ctx, clusterAddr); private == nil || !*private { return status.Errorf(status.InvalidArgument, "proxy cluster %s cannot serve the agent network gateway: the gateway is reachable only from connected peers, "+ "which needs at least one connected embedded proxy (netbird proxy) in the cluster", clusterAddr) @@ -912,34 +924,31 @@ func (m *managerImpl) validateGatewayCluster(ctx context.Context, accountID, clu return nil } -// accountClusterAddresses lists the active proxy cluster addresses the account -// may pin its gateway to: its own (BYOP) clusters plus the shared ones. This -// mirrors the free-domain allow list the dashboard offers as cluster choices, -// so the API accepts exactly what the UI can present. Addresses are stored as -// the proxy declared them; they are normalised here so the comparison against -// a normalised proxy_address is not defeated by case. -func (m *managerImpl) accountClusterAddresses(ctx context.Context, accountID string) ([]string, error) { - byop, err := m.store.GetActiveProxyClusterAddressesForAccount(ctx, accountID) +// accountKnowsCluster reports whether clusterAddr is a proxy cluster in the +// account's view — one of its own (BYOP) clusters or a shared one. The cluster +// listing is not gated on heartbeats, so this answer does not change while a +// cluster's proxies are merely offline. Addresses are stored as the proxy +// declared them, so both sides are normalised: hostnames are case-insensitive +// and the pin must not be sidesteppable by casing. +func (m *managerImpl) accountKnowsCluster(ctx context.Context, accountID, clusterAddr string) (bool, error) { + clusters, err := m.store.GetProxyClusters(ctx, accountID) if err != nil { - return nil, fmt.Errorf("list account proxy clusters: %w", err) - } - shared, err := m.store.GetActiveProxyClusterAddresses(ctx) - if err != nil { - return nil, fmt.Errorf("list shared proxy clusters: %w", err) + return false, fmt.Errorf("list proxy clusters: %w", err) } - addresses := make([]string, 0, len(byop)+len(shared)) - for _, addr := range slices.Concat(byop, shared) { - normalized, err := types.NormalizeHostname(addr) + for _, cluster := range clusters { + normalized, err := types.NormalizeHostname(cluster.Address) if err != nil { - // A cluster address the proxy declared in a shape we cannot - // normalise is not one an endpoint can be allocated beneath. - log.WithContext(ctx).Debugf("skipping unusable proxy cluster address %q: %s", addr, err) + // An address declared in a shape we cannot normalise is not one an + // endpoint can be allocated beneath. + log.WithContext(ctx).Debugf("skipping unusable proxy cluster address %q: %s", cluster.Address, err) continue } - addresses = append(addresses, normalized) + if normalized == clusterAddr { + return true, nil + } } - return addresses, nil + return false, nil } // bootstrapLabeled allocates a labeled endpoint one label beneath the given diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index 2f128c688..9b6c29d80 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -66,17 +66,27 @@ func (f *bootstrapFixture) createSettings(ctx context.Context, accountID, userID return f.manager.CreateSettings(ctx, userID, types.DefaultSettings(accountID), proxyAddress, endpoint) } -// seedProxy registers a connected proxy in clusterAddr so the labeled -// bootstrap path has a real cluster to validate against. accountID empty -// makes it a shared (NetBird-operated) cluster; private mirrors the -// capability an embedded `netbird proxy` reports, nil an unreported one. +func ptrTo[T any](v T) *T { return &v } + +// seedProxy registers a proxy in clusterAddr, heartbeating now, so the labeled +// bootstrap path has a real cluster to validate against. accountID empty makes +// it a shared (NetBird-operated) cluster; private mirrors the capability an +// embedded `netbird proxy` reports, nil an unreported one. func (f *bootstrapFixture) seedProxy(t *testing.T, proxyID, accountID, clusterAddr string, private *bool) { + t.Helper() + f.seedProxyAt(t, proxyID, accountID, clusterAddr, private, time.Now().UTC()) +} + +// seedProxyAt is seedProxy with an explicit last-seen, for cases that need a +// proxy whose heartbeat has aged past the active window while its row (and so +// its cluster) is still on record. +func (f *bootstrapFixture) seedProxyAt(t *testing.T, proxyID, accountID, clusterAddr string, private *bool, lastSeen time.Time) { t.Helper() p := &proxy.Proxy{ ID: proxyID, ClusterAddress: clusterAddr, Status: proxy.StatusConnected, - LastSeen: time.Now().UTC(), + LastSeen: lastSeen, Capabilities: proxy.Capabilities{Private: private}, } if accountID != "" { @@ -89,8 +99,7 @@ func (f *bootstrapFixture) seedProxy(t *testing.T, proxyID, accountID, clusterAd // embedded proxy, which is what the labeled bootstrap requires. func (f *bootstrapFixture) seedEmbeddedCluster(t *testing.T, clusterAddr string) { t.Helper() - private := true - f.seedProxy(t, "proxy-"+clusterAddr, "", clusterAddr, &private) + f.seedProxy(t, "proxy-"+clusterAddr, "", clusterAddr, ptrTo(true)) } // TestCreateSettingsRequiresPermission pins the gate: bootstrap assigns the @@ -255,36 +264,54 @@ func TestCreateProviderHasNoSettingsSideEffects(t *testing.T) { assert.Error(t, err, "provider create must not conjure a settings row") } -// TestCreateSettingsAllowsUnjudgeableCluster pins the address-first carve-out: -// a cluster nothing is connected to yet cannot be judged, so the pin is -// allowed — the same order the dedicated path documents (claim the address, -// connect the proxy after). A stale cluster whose proxies have aged out of -// the active window reads the same way. -func TestCreateSettingsAllowsUnjudgeableCluster(t *testing.T) { +// TestCreateSettingsAllowsUnknownCluster pins the one opening left: a cluster +// management holds no proxy row for cannot be judged, so the pin is allowed — +// the same order the dedicated path documents (claim the address, connect the +// proxy after). +func TestCreateSettingsAllowsUnknownCluster(t *testing.T) { ctx := context.Background() - private := true + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - cases := map[string]func(f *bootstrapFixture, t *testing.T){ - "no proxy at all": func(*bootstrapFixture, *testing.T) {}, - "heartbeat aged out": func(f *bootstrapFixture, t *testing.T) { - require.NoError(t, f.store.SaveProxy(ctx, &proxy.Proxy{ - ID: "proxy-stale", - ClusterAddress: "future.example.com", - Status: proxy.StatusConnected, - LastSeen: time.Now().UTC().Add(-time.Hour), - Capabilities: proxy.Capabilities{Private: &private}, - }), "seeding a stale proxy must succeed") - }, + created, err := f.createSettings(ctx, "account1", "user1", "future.example.com", "") + require.NoError(t, err, "a cluster no proxy has ever declared must stay pinnable") + assert.Equal(t, "future.example.com", created.ProxyAddress) +} + +// TestCreateSettingsRejectsOfflineCluster is the guard against deciding on +// heartbeat freshness. A centralised cluster is refused while its proxies are +// live; the same cluster must stay refused once they stop heartbeating, which +// takes only a couple of minutes (proxyActiveThreshold). Judging on liveness +// would turn "wait for the proxy to go quiet" into a way to pin the account's +// immutable endpoint to a cluster that can never serve it. +func TestCreateSettingsRejectsOfflineCluster(t *testing.T) { + ctx := context.Background() + notPrivate := false + + cases := map[string]*bool{ + "centralised proxy gone quiet": ¬Private, + // A cluster that could serve the gateway still has to have something + // live in it to prove so at bootstrap: refusing is the safe direction + // (reconnect the proxy and retry) where accepting is permanent. + "embedded proxy gone quiet": ptrTo(true), } - for name, seed := range cases { + for name, private := range cases { t.Run(name, func(t *testing.T) { f := newBootstrapFixture(t) - seed(f, t) + f.seedProxyAt(t, "proxy1", "", "offline.example.com", private, + time.Now().UTC().Add(-time.Hour)) f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - created, err := f.createSettings(ctx, "account1", "user1", "future.example.com", "") - require.NoError(t, err, "a cluster with nothing live in it must stay pinnable") - assert.Equal(t, "future.example.com", created.ProxyAddress) + _, err := f.createSettings(ctx, "account1", "user1", "offline.example.com", "") + require.Error(t, err, "a known cluster with nothing live in it must be rejected") + var sErr *status.Error + require.ErrorAs(t, err, &sErr) + assert.Equal(t, status.InvalidArgument, sErr.Type(), "rejection must be a validation error") + assert.Contains(t, err.Error(), "connected embedded proxy", + "the error must say a live embedded proxy is what is missing") + + _, err = f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, "account1") + assert.Error(t, err, "no row may be left behind by a rejected bootstrap") }) } } @@ -312,21 +339,32 @@ func TestCreateSettingsRequiresPrivateCluster(t *testing.T) { } // TestCreateSettingsRejectsForeignCluster pins tenant isolation on the pin: an -// account-owned (BYOP) cluster belongs to the account that runs it, and is not -// a cluster another account may hang its gateway beneath — even though it is -// private-capable. +// account-owned (BYOP) cluster belongs to the account that runs it and is not +// one another account may hang its gateway beneath, even though it is +// private-capable. Ownership does not lapse with the heartbeat either, so the +// refusal holds while the foreign cluster is offline. func TestCreateSettingsRejectsForeignCluster(t *testing.T) { ctx := context.Background() - f := newBootstrapFixture(t) - private := true - f.seedProxy(t, "proxy1", "account2", "byop.account2.example.com", &private) - f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - _, err := f.createSettings(ctx, "account1", "user1", "byop.account2.example.com", "") - require.Error(t, err, "another account's BYOP cluster must be rejected") - var sErr *status.Error - require.ErrorAs(t, err, &sErr) - assert.Equal(t, status.InvalidArgument, sErr.Type(), "rejection must be a validation error") + cases := map[string]time.Time{ + "live": time.Now().UTC(), + "offline": time.Now().UTC().Add(-time.Hour), + } + for name, lastSeen := range cases { + t.Run(name, func(t *testing.T) { + f := newBootstrapFixture(t) + f.seedProxyAt(t, "proxy1", "account2", "byop.account2.example.com", ptrTo(true), lastSeen) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + + _, err := f.createSettings(ctx, "account1", "user1", "byop.account2.example.com", "") + require.Error(t, err, "another account's BYOP cluster must be rejected") + var sErr *status.Error + require.ErrorAs(t, err, &sErr) + assert.Equal(t, status.InvalidArgument, sErr.Type(), "rejection must be a validation error") + assert.Contains(t, err.Error(), "not available to this account", + "the error must say the cluster is not the account's to use") + }) + } } // TestCreateSettingsAcceptsOwnPrivateCluster pins the BYOP happy path: the @@ -334,8 +372,7 @@ func TestCreateSettingsRejectsForeignCluster(t *testing.T) { func TestCreateSettingsAcceptsOwnPrivateCluster(t *testing.T) { ctx := context.Background() f := newBootstrapFixture(t) - private := true - f.seedProxy(t, "proxy1", "account1", "byop.account1.example.com", &private) + f.seedProxy(t, "proxy1", "account1", "byop.account1.example.com", ptrTo(true)) f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) created, err := f.createSettings(ctx, "account1", "user1", "byop.account1.example.com", "")