From fabfacee5559bf8dd13517e8af0bd8eadcb557f5 Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Mon, 3 Aug 2026 17:20:44 +0200 Subject: [PATCH] feat(store): globally unique subdomains and an insert that surfaces conflicts Once an endpoint hangs off a shared zone rather than a per-cluster address, subdomain labels must be unique across the whole zone rather than within one cluster. Uniqueness was previously advisory -- a pre-read "taken" set with no database constraint -- so this adds a unique index on the column and makes the database the arbiter. CreateAgentNetworkSettings is a plain INSERT that returns the driver error unwrapped, both of which the allocator depends on: SaveAgentNetworkSettings is an upsert (which cannot conflict) and wraps failures in a generic internal error, discarding the message that unique-violation detection needs. Note for operators: the index is created by a migration that fails, and therefore blocks startup, on a deployment that already holds two rows with the same subdomain on different clusters -- which was legal under the old per-cluster scheme. Audit for duplicates before upgrading. --- .../server/store/sql_store_agentnetwork.go | 15 ++++ .../sql_store_agentnetwork_settings_test.go | 77 +++++++++++++++++++ management/server/store/store.go | 17 ++++ management/server/store/store_mock.go | 14 ++++ 4 files changed, 123 insertions(+) create mode 100644 management/server/store/sql_store_agentnetwork_settings_test.go diff --git a/management/server/store/sql_store_agentnetwork.go b/management/server/store/sql_store_agentnetwork.go index b72dc735f..cdfe743b9 100644 --- a/management/server/store/sql_store_agentnetwork.go +++ b/management/server/store/sql_store_agentnetwork.go @@ -346,6 +346,21 @@ func (s *SqlStore) SaveAgentNetworkSettings(ctx context.Context, settings *agent return nil } +// CreateAgentNetworkSettings inserts a new settings row. +// +// Unlike SaveAgentNetworkSettings (an upsert) this is a plain INSERT, and it +// returns the driver error unwrapped. Both properties are required by the +// subdomain allocator: it relies on the unique index rejecting a duplicate +// label, and on being able to recognise that rejection so it can retry with a +// fresh label instead of surfacing an error. +func (s *SqlStore) CreateAgentNetworkSettings(ctx context.Context, settings *agentNetworkTypes.Settings) error { + if err := s.db.Create(settings).Error; err != nil { + log.WithContext(ctx).Debugf("failed to create agent network settings: %v", err) + return err + } + return nil +} + // IncrementAgentNetworkConsumption atomically upserts the consumption // row keyed on (account, dim_kind, dim_id, window_seconds, window_start) // and adds the supplied deltas. Concurrent calls from multiple proxy diff --git a/management/server/store/sql_store_agentnetwork_settings_test.go b/management/server/store/sql_store_agentnetwork_settings_test.go new file mode 100644 index 000000000..e9b1e1308 --- /dev/null +++ b/management/server/store/sql_store_agentnetwork_settings_test.go @@ -0,0 +1,77 @@ +package store + +import ( + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" +) + +// TestAgentNetworkSettings_SubdomainIsGloballyUnique is the guard for the whole +// allocation scheme: the label is now globally unique rather than per-cluster, +// and the allocator depends on the DATABASE saying no. Two different accounts on +// two different clusters must not be able to hold the same subdomain. +func TestAgentNetworkSettings_SubdomainIsGloballyUnique(t *testing.T) { + ctx := context.Background() + s, cleanup, err := NewTestStoreFromSQL(ctx, "", t.TempDir()) + require.NoError(t, err, "real sqlite test store must come up") + defer cleanup() + + first := &agentNetworkTypes.Settings{ + AccountID: "acc-unique-1", + Cluster: "eu.proxy.example", + Subdomain: "brave-otter", + Zone: "gateway.example", + } + require.NoError(t, s.CreateAgentNetworkSettings(ctx, first), "first insert must succeed") + + // Deliberately a different account AND a different cluster: under the old + // per-cluster scheme this was legal, and it is exactly what must now fail. + second := &agentNetworkTypes.Settings{ + AccountID: "acc-unique-2", + Cluster: "us.proxy.example", + Subdomain: "brave-otter", + Zone: "gateway.example", + } + err = s.CreateAgentNetworkSettings(ctx, second) + require.Error(t, err, "duplicate subdomain must be rejected by the unique index") + + // The allocator recognises conflicts by matching the driver's message, so an + // error that does not carry a unique-violation signature is useless to it + // even though it is non-nil. These are the three signatures management's + // isUniqueConstraintError matches (postgres / mysql / sqlite). + msg := err.Error() + assert.True(t, + strings.Contains(msg, "(SQLSTATE 23505)") || + strings.Contains(msg, "Error 1062 (23000)") || + strings.Contains(msg, "UNIQUE constraint failed"), + "error must be the raw driver error, recognisable as a unique violation; got %q", msg) +} + +// TestAgentNetworkSettings_CreateThenReadBack keeps CreateAgentNetworkSettings +// honest as an insert path: the row it writes must be fully readable, including +// the new Zone column. +func TestAgentNetworkSettings_CreateThenReadBack(t *testing.T) { + ctx := context.Background() + s, cleanup, err := NewTestStoreFromSQL(ctx, "", t.TempDir()) + require.NoError(t, err, "real sqlite test store must come up") + defer cleanup() + + want := &agentNetworkTypes.Settings{ + AccountID: "acc-readback-1", + Cluster: "eu.proxy.example", + Subdomain: "swift-heron", + Zone: "gateway.example", + } + require.NoError(t, s.CreateAgentNetworkSettings(ctx, want)) + + got, err := s.GetAgentNetworkSettings(ctx, LockingStrengthNone, "acc-readback-1") + require.NoError(t, err, "the inserted row must be readable") + assert.Equal(t, "swift-heron", got.Subdomain) + assert.Equal(t, "gateway.example", got.Zone, "the Zone column must round-trip") + assert.Equal(t, "swift-heron.gateway.example", got.Endpoint(), "endpoint derives from zone") +} diff --git a/management/server/store/store.go b/management/server/store/store.go index 1beea72fd..9bb2e8890 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -362,6 +362,7 @@ type Store interface { GetAllAgentNetworkSettings(ctx context.Context, lockStrength LockingStrength) ([]*agentNetworkTypes.Settings, error) GetAgentNetworkSettingsByCluster(ctx context.Context, lockStrength LockingStrength, cluster string) ([]*agentNetworkTypes.Settings, error) SaveAgentNetworkSettings(ctx context.Context, settings *agentNetworkTypes.Settings) error + CreateAgentNetworkSettings(ctx context.Context, settings *agentNetworkTypes.Settings) error IncrementAgentNetworkConsumption(ctx context.Context, accountID string, kind agentNetworkTypes.ConsumptionDimension, dimID string, windowSeconds int64, windowStart time.Time, tokensIn, tokensOut int64, costUSD float64) error IncrementAgentNetworkConsumptionBatch(ctx context.Context, accountID string, keys []agentNetworkTypes.ConsumptionKey, tokensIn, tokensOut int64, costUSD float64) error GetAgentNetworkConsumption(ctx context.Context, lockStrength LockingStrength, accountID string, kind agentNetworkTypes.ConsumptionDimension, dimID string, windowSeconds int64, windowStart time.Time) (*agentNetworkTypes.Consumption, error) @@ -658,6 +659,22 @@ func getMigrationsPostAuto(ctx context.Context) []migrationFunc { func(db *gorm.DB) error { return migration.FoldCostAggregatesIntoBuckets[agentNetworkTypes.AgentNetworkUsage](ctx, db) }, + func(db *gorm.DB) error { + // Enforce globally-unique agent-network subdomains. + // + // Uniqueness used to be per-cluster and advisory (a pre-read + // "taken" set with no DB constraint). Once the endpoint hangs off a + // shared zone the label must be unique across that whole zone, and + // the allocator depends on the database rejecting duplicates so it + // can retry with a fresh label. + // + // 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. + return migration.CreateIndexIfNotExists[agentNetworkTypes.Settings]( + ctx, db, "idx_agent_network_settings_subdomain_unique", "subdomain", + ) + }, } } diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 428632a86..f006fc00b 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -268,6 +268,20 @@ func (mr *MockStoreMockRecorder) CreateAgentNetworkAccessLog(ctx, entry, groups return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateAgentNetworkAccessLog", reflect.TypeOf((*MockStore)(nil).CreateAgentNetworkAccessLog), ctx, entry, groups) } +// CreateAgentNetworkSettings mocks base method. +func (m *MockStore) CreateAgentNetworkSettings(ctx context.Context, settings *types.Settings) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreateAgentNetworkSettings", ctx, settings) + ret0, _ := ret[0].(error) + return ret0 +} + +// CreateAgentNetworkSettings indicates an expected call of CreateAgentNetworkSettings. +func (mr *MockStoreMockRecorder) CreateAgentNetworkSettings(ctx, settings interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateAgentNetworkSettings", reflect.TypeOf((*MockStore)(nil).CreateAgentNetworkSettings), ctx, settings) +} + // CreateAgentNetworkUsage mocks base method. func (m *MockStore) CreateAgentNetworkUsage(ctx context.Context, usage *types.AgentNetworkUsage, groups []types.AgentNetworkUsageGroup) error { m.ctrl.T.Helper()