diff --git a/combined/cmd/config.go b/combined/cmd/config.go index 890c86876..137a6bd88 100644 --- a/combined/cmd/config.go +++ b/combined/cmd/config.go @@ -83,6 +83,7 @@ type ServerConfig struct { // AgentNetworkConfig contains agent-network (LLM gateway) configuration. type AgentNetworkConfig struct { PricingDefaultsFile string `yaml:"pricingDefaultsFile"` + Zone string `yaml:"zone"` } // TLSConfig contains TLS/HTTPS settings @@ -732,6 +733,7 @@ func (c *CombinedConfig) ToManagementConfig() (*nbconfig.Config, error) { PerAccountHighestSupportedSyncMessageVersion: c.Server.PerAccountSupportedSyncMessageVersions, AgentNetwork: nbconfig.AgentNetwork{ PricingDefaultsFile: c.Server.AgentNetwork.PricingDefaultsFile, + Zone: c.Server.AgentNetwork.Zone, }, }, nil } diff --git a/combined/config.yaml.example b/combined/config.yaml.example index 085e4344f..79540db40 100644 --- a/combined/config.yaml.example +++ b/combined/config.yaml.example @@ -147,3 +147,11 @@ server: # # is re-read periodically (mtime poll). An explicitly configured path that # # fails to load fails startup; runtime reload errors keep the previous table. # pricingDefaultsFile: "pricing.yaml" + # + # # Parent DNS zone that Agent Network gateway endpoints are allocated + # # under, producing .. Empty (the default) preserves the + # # legacy behaviour of deriving the endpoint from the serving cluster, so + # # self-hosted deployments are unaffected. Captured onto each settings row + # # when that row is created; changing it later does not move existing + # # tenants. + # zone: "gateway.example.com" diff --git a/infrastructure_files/management.json.tmpl b/infrastructure_files/management.json.tmpl index 32f083917..7be6d1b09 100644 --- a/infrastructure_files/management.json.tmpl +++ b/infrastructure_files/management.json.tmpl @@ -39,6 +39,9 @@ ] }, "DisableDefaultPolicy": $NETBIRD_MGMT_DISABLE_DEFAULT_POLICY, + "AgentNetwork": { + "Zone": "$NETBIRD_AGENT_NETWORK_ZONE" + }, "Datadir": "", "DataStoreEncryptionKey": "$NETBIRD_DATASTORE_ENC_KEY", "StoreConfig": { diff --git a/management/internals/modules/agentnetwork/allocate_test.go b/management/internals/modules/agentnetwork/allocate_test.go index e63a48032..8b7d84153 100644 --- a/management/internals/modules/agentnetwork/allocate_test.go +++ b/management/internals/modules/agentnetwork/allocate_test.go @@ -234,7 +234,9 @@ func TestBootstrapSettings_ConcurrentBootstrapReturnsWinnersRow(t *testing.T) { Return(nil, status.Errorf(status.NotFound, "agent network settings not found")), // The insert loses the race. The message shape is the sqlite wording // for a primary-key violation on account_id (not the subdomain - // index), per the review finding this test locks down. + // index); this test locks down that the retry path recognizes that + // shape as a race loss and re-reads the winner's row, rather than + // misclassifying it as a subdomain conflict. mockStore.EXPECT(). ExecuteInTransaction(gomock.Any(), gomock.Any()). DoAndReturn(func(_ context.Context, f func(store.Store) error) error { @@ -259,3 +261,36 @@ func TestBootstrapSettings_ConcurrentBootstrapReturnsWinnersRow(t *testing.T) { require.NotNil(t, settings) assert.Same(t, winner, settings, "the loser must return the concurrent winner's row, not retry past it") } + +// TestBootstrapSettings_NonRetryableErrorFailsImmediately guards the +// isUniqueConstraintError branch itself: a regression that dropped that check +// and retried on every ExecuteInTransaction error would leave every other test +// in this file green, because none of them feed the loop a non-collision +// failure. A generic store error must surface immediately, wrapped, and must +// not be retried — asserting ExecuteInTransaction was called exactly once is +// what proves the loop didn't retry. +func TestBootstrapSettings_NonRetryableErrorFailsImmediately(t *testing.T) { + ctx := context.Background() + ctrl := gomock.NewController(t) + mockStore := store.NewMockStore(ctrl) + + mockStore.EXPECT(). + GetAgentNetworkSettings(gomock.Any(), store.LockingStrengthNone, "account1"). + Return(nil, status.Errorf(status.NotFound, "agent network settings not found")) + + mockStore.EXPECT(). + ExecuteInTransaction(gomock.Any(), gomock.Any()). + Return(errors.New("connection refused")). + Times(1) + + m := &managerImpl{ + store: mockStore, + labelRng: rand.New(rand.NewSource(5)), + } + + settings, err := m.bootstrapSettingsIfNeeded(ctx, "account1", "cluster1.example.com") + require.Error(t, err, "a non-collision store error must surface, not be swallowed") + assert.Nil(t, settings) + assert.Contains(t, err.Error(), "create agent network settings", + "the non-retryable error must be wrapped and returned, not retried past") +} diff --git a/management/internals/modules/agentnetwork/domainlookup_test.go b/management/internals/modules/agentnetwork/domainlookup_test.go index a77a72070..105648c4a 100644 --- a/management/internals/modules/agentnetwork/domainlookup_test.go +++ b/management/internals/modules/agentnetwork/domainlookup_test.go @@ -100,7 +100,12 @@ func TestSynthesizeServiceForDomain_UnknownLabel(t *testing.T) { } // TestSynthesizeServiceForDomain_DegenerateInput — empty and single-label -// hostnames have no subdomain to look up and must not reach the store. +// hostnames have no dot to cut a subdomain label from, so they resolve to no +// service, same as any other unowned hostname. The early-return guard that +// catches them is an optimisation (it skips a store round trip that would +// only miss anyway), not what makes this case correct — "" and "localhost" +// would still come back nil, nil even without it, via the same not-found +// fallthrough TestSynthesizeServiceForDomain_UnknownLabel exercises. func TestSynthesizeServiceForDomain_DegenerateInput(t *testing.T) { ctx := context.Background() s, cleanup, err := store.NewTestStoreFromSQL(ctx, "", t.TempDir()) diff --git a/management/internals/modules/agentnetwork/labelgen/labelgen.go b/management/internals/modules/agentnetwork/labelgen/labelgen.go index 549767096..c81a3691d 100644 --- a/management/internals/modules/agentnetwork/labelgen/labelgen.go +++ b/management/internals/modules/agentnetwork/labelgen/labelgen.go @@ -2,18 +2,11 @@ package labelgen import ( - "fmt" "math/rand" "sort" "sync" ) -// pickAttempts caps the random retries before falling back to the -// suffixed form. Eight is a soft compromise: with a near-empty taken -// set the very first pick almost always succeeds; when the wordlist is -// densely populated the fallback eventually fires anyway. -const pickAttempts = 8 - var ( dedupOnce sync.Once uniqWords []string @@ -37,43 +30,15 @@ func uniqueWords() []string { return uniqWords } -// PickUnique selects a label not already in `taken`. It tries up to -// pickAttempts random picks; on exhaustion it scans the deduplicated -// wordlist for any remaining free entry, and if none is left appends -// `-` to a deterministic word and returns. The caller -// is responsible for seeding rng (math/rand). -func PickUnique(rng *rand.Rand, taken map[string]struct{}, fallbackSuffix string) string { - pool := uniqueWords() - if len(pool) == 0 { - return fallbackSuffix - } - - for i := 0; i < pickAttempts; i++ { - w := pool[rng.Intn(len(pool))] - if _, ok := taken[w]; !ok { - return w - } - } - - for _, w := range pool { - if _, ok := taken[w]; !ok { - return w - } - } - - w := pool[rng.Intn(len(pool))] - return fmt.Sprintf("%s-%s", w, fallbackSuffix) -} - // PickTuple returns an adjective-noun label such as "brave-otter". It is still // a single DNS label. // -// Unlike PickUnique it takes no `taken` set and has no fallback suffix. The -// noun pool holds 857 entries, which is ample per cluster but a hard ceiling -// once labels must be unique across one shared zone; pairing an adjective with -// a noun spans len(adjectives) * 857 instead. Uniqueness is enforced by a -// database constraint and retried by the caller, rather than guessed from a -// pre-read set that a concurrent allocation can invalidate. +// It takes no `taken` set and has no fallback suffix. The noun pool holds 857 +// entries, which is ample per cluster but a hard ceiling once labels must be +// unique across one shared zone; pairing an adjective with a noun spans +// len(adjectives) * 857 instead. Uniqueness is enforced by a database +// constraint and retried by the caller, rather than guessed from a pre-read +// set that a concurrent allocation can invalidate. func PickTuple(rng *rand.Rand) string { nouns := uniqueWords() if len(nouns) == 0 || len(adjectives) == 0 { diff --git a/management/internals/modules/agentnetwork/labelgen/labelgen_test.go b/management/internals/modules/agentnetwork/labelgen/labelgen_test.go index 7e12fc133..79cdd2382 100644 --- a/management/internals/modules/agentnetwork/labelgen/labelgen_test.go +++ b/management/internals/modules/agentnetwork/labelgen/labelgen_test.go @@ -9,78 +9,6 @@ import ( "github.com/stretchr/testify/require" ) -// TestPickUnique_DeterministicWithSeededRng locks the property the -// caller relies on: same seed + same taken set → same pick. Without -// that, the bootstrap flow can't reproduce a label across retries. -func TestPickUnique_DeterministicWithSeededRng(t *testing.T) { - taken := map[string]struct{}{} - - rngA := rand.New(rand.NewSource(42)) - rngB := rand.New(rand.NewSource(42)) - - a := PickUnique(rngA, taken, "abcd") - b := PickUnique(rngB, taken, "abcd") - - assert.Equal(t, a, b, "Same seed and taken set must produce identical pick") -} - -// TestPickUnique_AvoidsTakenWordsWhenMostAreReserved seeds taken with -// every word in the pool except a handful and confirms PickUnique -// finds one of the remaining free entries instead of returning the -// fallback form. -func TestPickUnique_AvoidsTakenWordsWhenMostAreReserved(t *testing.T) { - pool := uniqueWords() - require.NotEmpty(t, pool, "wordlist must be populated for the test to mean anything") - - free := map[string]struct{}{ - pool[0]: {}, - pool[len(pool)/2]: {}, - pool[len(pool)-1]: {}, - } - - taken := make(map[string]struct{}, len(pool)) - for _, w := range pool { - if _, ok := free[w]; ok { - continue - } - taken[w] = struct{}{} - } - - rng := rand.New(rand.NewSource(7)) - got := PickUnique(rng, taken, "abcd") - - _, isFree := free[got] - assert.True(t, isFree, "PickUnique must return one of the free words; got %q", got) - assert.NotContains(t, got, "-", "Free pick must not be the suffix fallback form") -} - -// TestPickUnique_FallsBackWhenAllReserved exhausts the pool and -// confirms PickUnique appends the supplied suffix instead of -// returning a duplicate. -func TestPickUnique_FallsBackWhenAllReserved(t *testing.T) { - pool := uniqueWords() - - taken := make(map[string]struct{}, len(pool)) - for _, w := range pool { - taken[w] = struct{}{} - } - - rng := rand.New(rand.NewSource(99)) - got := PickUnique(rng, taken, "abcd") - - assert.True(t, strings.HasSuffix(got, "-abcd"), "Exhausted pool must produce -; got %q", got) - - prefix := strings.TrimSuffix(got, "-abcd") - found := false - for _, w := range pool { - if w == prefix { - found = true - break - } - } - assert.True(t, found, "Fallback prefix must be drawn from the wordlist; got %q", prefix) -} - // TestUniqueWords_DropsDuplicates guards against authoring slips in // words.go: every entry must be unique and DNS-safe. func TestUniqueWords_DropsDuplicates(t *testing.T) { diff --git a/management/internals/modules/agentnetwork/labelgen/words.go b/management/internals/modules/agentnetwork/labelgen/words.go index 2028ff23d..e0bba28f7 100644 --- a/management/internals/modules/agentnetwork/labelgen/words.go +++ b/management/internals/modules/agentnetwork/labelgen/words.go @@ -6,7 +6,7 @@ // hand-checked to avoid offensive, brand, or region-specific terms. package labelgen -// words is the pool PickUnique selects from. The slice is intentionally +// words is the pool PickTuple draws its noun from. The slice is intentionally // not sorted — random picks distribute across the list naturally. var words = []string{ "acorn", "adobe", "agate", "alder", "almond", "alpine", "amber", "amethyst", diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 4cf7f75b4..7b7620c7e 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -690,13 +690,9 @@ func (m *managerImpl) bootstrapSettingsIfNeeded(ctx context.Context, accountID, return nil, fmt.Errorf("get agent network settings: %w", err) } - // Allocate a subdomain and insert in one transaction, retrying on a unique - // violation. This replaces a read-then-write over a pre-computed "taken" - // set, which had three defects: the set was per-cluster (wrong once the - // endpoint hangs off a shared zone), the read and the write were not - // atomic, and the exhaustion fallback appended accountID[:4] — constant for - // every account created within the same ~68 minutes — with no retry and no - // uniqueness check, so two such accounts could be handed the same label. + // Labels must be unique across the whole zone; the database's unique index + // enforces that, and the loop below retries with a fresh label whenever an + // attempt is rejected. now := time.Now().UTC() settings := &types.Settings{ AccountID: accountID, @@ -723,6 +719,10 @@ func (m *managerImpl) bootstrapSettingsIfNeeded(ctx context.Context, accountID, accountID) } + // Each attempt gets its own transaction wrapping a single INSERT: on + // postgres a failed statement poisons the enclosing transaction, so a + // fresh transaction per attempt is what makes the retry loop work on + // that dialect at all. err := m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { return transaction.CreateAgentNetworkSettings(ctx, settings) }) diff --git a/management/internals/server/config/config.go b/management/internals/server/config/config.go index b893bcf6a..98876b577 100644 --- a/management/internals/server/config/config.go +++ b/management/internals/server/config/config.go @@ -67,15 +67,6 @@ type Config struct { HighestSupportedSyncMessageVersion *int PerAccountHighestSupportedSyncMessageVersion map[string]int - - // AgentNetworkZone is the parent DNS zone that Agent Network gateway - // endpoints are allocated under, producing .. - // - // Empty (the default) preserves the legacy behaviour of deriving the - // endpoint from the serving cluster, so self-hosted deployments are - // unaffected. It is captured onto each settings row when that row is - // created; changing it later does not move existing tenants. - AgentNetworkZone string } // GetAuthAudiences returns the audience from the http config and device authorization flow config @@ -213,6 +204,15 @@ type AgentNetwork struct { // prefill with). An explicitly configured path that fails to load // fails startup; runtime reload errors keep the previous table. PricingDefaultsFile string + + // Zone is the parent DNS zone that Agent Network gateway endpoints are + // allocated under, producing .. + // + // Empty (the default) preserves the legacy behaviour of deriving the + // endpoint from the serving cluster, so self-hosted deployments are + // unaffected. It is captured onto each settings row when that row is + // created; changing it later does not move existing tenants. + Zone string } // ReverseProxy contains reverse proxy configuration in front of management. diff --git a/management/internals/server/modules.go b/management/internals/server/modules.go index 08490e47a..899ac0fd1 100644 --- a/management/internals/server/modules.go +++ b/management/internals/server/modules.go @@ -202,7 +202,7 @@ func (s *BaseServer) AgentNetworkManager() agentnetwork.Manager { s.PermissionsManager(), s.AccountManager(), s.ServiceProxyController(), - s.Config.AgentNetworkZone, + s.Config.AgentNetwork.Zone, ) // Sweep expired agent-network access logs per account retention, // reusing the reverse-proxy cleanup interval config. diff --git a/management/server/store/store.go b/management/server/store/store.go index c0e086378..3aab631e4 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -672,6 +672,12 @@ func getMigrationsPostAuto(ctx context.Context) []migrationFunc { // The pre-existing idx_agent_network_settings_cluster_subdomain is // left in place: it is non-unique and indexes subdomain alone // (Cluster carries no tag), so it neither conflicts nor suffices. + // It must also stay for a second, load-bearing reason on mysql: + // its gorm:"index:" tag on the Subdomain field is what makes gorm + // size that column as varchar(191) instead of longtext. mysql + // cannot put a longtext column in a unique index at all, so + // dropping this "redundant" index as unneeded would silently + // break the migration above on that dialect. return migration.CreateIndexIfNotExists[agentNetworkTypes.Settings]( ctx, db, "idx_agent_network_settings_subdomain_unique", "subdomain", ) diff --git a/management/server/types/account.go b/management/server/types/account.go index a0f5c9c6a..059fc20cf 100644 --- a/management/server/types/account.go +++ b/management/server/types/account.go @@ -254,6 +254,7 @@ func (a *Account) SynthesizePrivateServiceZones(peerID string) []nbdns.CustomZon peerGroups := a.GetPeerGroups(peerID) zonesByApex := map[string]*nbdns.CustomZone{} + var skippedNoZoneApex []string for _, svc := range a.Services { if svc == nil || !svc.Enabled || !svc.Private { @@ -275,9 +276,12 @@ func (a *Account) SynthesizePrivateServiceZones(peerID string) []nbdns.CustomZon // This service passed every gate above (enabled, private, // AccessGroups, connected proxy peers) and would otherwise have // emitted a record, but its domain matches neither its DNSZone, - // its ProxyCluster, nor any validated custom-domain row. - log.Debugf("private-zone synth: svc %s domain=%s cluster=%s dns_zone=%q has no matching zone apex, skipping", - svc.ID, svc.Domain, svc.ProxyCluster, svc.DNSZone) + // its ProxyCluster, nor any validated custom-domain row. Collected + // rather than logged here — this runs per peer x per service, and + // logging inline here would reintroduce the per-peer noise the + // "0 zones" diagnostic below deliberately avoids. + skippedNoZoneApex = append(skippedNoZoneApex, + fmt.Sprintf("%s(domain=%s cluster=%s dns_zone=%q)", svc.ID, svc.Domain, svc.ProxyCluster, svc.DNSZone)) continue } @@ -331,6 +335,10 @@ func (a *Account) SynthesizePrivateServiceZones(peerID string) []nbdns.CustomZon svc.ID, svc.Domain, svc.ProxyCluster, len(proxyPeers), skippedDisconnected) } } + if len(skippedNoZoneApex) > 0 { + log.Debugf("private-zone synth: peer %s account %s skipped %d service(s) with no matching zone apex: %s", + peerID, a.Id, len(skippedNoZoneApex), strings.Join(skippedNoZoneApex, ", ")) + } out := make([]nbdns.CustomZone, 0, len(zonesByApex)) for _, zone := range zonesByApex {