mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-25 00:51:28 +02:00
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
@@ -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",
|
||||
)
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user