mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-17 12:19:07 +02:00
chore(agentnetwork): fix config grouping, dead code, comments, coverage
- Move the new zone config key into the existing AgentNetwork config group (management/internals/server/config/config.go) instead of a sibling top-level field, wire modules.go to the new path, add the matching field to combined/cmd/config.go's AgentNetworkConfig and its mapping (it was previously unreachable in the combined binary), and document the key in infrastructure_files/management.json.tmpl and combined/config.yaml.example. - Delete PickUnique and its three tests: Task 5 removed its last production caller, leaving it dead exported code with a stale words.go comment pointing at it. - Reword two test comments that referenced our private review process instead of stating what the test locks down / why TargetId stays pinned to Cluster. - Add TestBootstrapSettings_NonRetryableErrorFailsImmediately: a regression that dropped the isUniqueConstraintError gate and retried on every error would leave every existing allocator test green. - Fix TestSynthesizeServiceForDomain_DegenerateInput's docstring: the early-return guard is an optimisation, not what makes "" and "localhost" resolve to no service. - Replace manager.go's allocation-comment archaeology (a deleted per-cluster "taken" set, an `accountID[:4]` suffix, "~68 minutes") with the actual invariant, and note why each retry attempt gets its own transaction (a failed statement poisons the enclosing transaction on postgres). - Collapse the per-service "no matching zone apex" debug log in account.go into a single line per call instead of one per skipped service. - Document why idx_agent_network_settings_cluster_subdomain must stay on mysql: its index tag is what sizes subdomain as varchar(191) rather than longtext, which the new unique index requires.
This commit is contained in:
Brad Ison
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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())
|
||||
|
||||
@@ -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
|
||||
// `-<fallbackSuffix>` 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 {
|
||||
|
||||
@@ -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 <word>-<suffix>; 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) {
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user