mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-27 01:51:30 +02:00
[management] Derive the settings ETag at a precision the store preserves
The validator a bootstrap returns was permanently unusable on PostgreSQL. POST derives it from the in-memory row, whose CreatedAt carries nanoseconds, while every later comparison derives it from a row read back out of the store — and PostgreSQL truncates timestamps to microseconds, MySQL DATETIME to whole seconds without an fsp. The two never agreed again, so the documented "conditional PUT without an intervening GET" answered 412 forever. Hashing CreatedAt at whole-second precision fixes it at the root: seconds is the floor every supported engine round-trips, so the validator no longer depends on which store is behind it. The cost is that a delete and re-bootstrap inside the same second, onto the same endpoint with the same toggles, derives the same validator — which needs a self-addressed endpoint reclaimed within one second, since a labeled bootstrap draws a fresh label. The whole suite ran green against this bug, because every test uses the sqlite test store and sqlite preserves nanoseconds. The regression guard is therefore on the type rather than through a store: it asserts the validator is unchanged by truncation at each engine's precision, so it holds without running the suite against all three. Also from review: the concurrency test recorded the winning writer's value in a slice left zero for the loser, and zero is a retention the API documents as "keep indefinitely" — so the assertion could be satisfied without matching the writer that won. It now uses a sentinel and asserts equality. The stale-delete e2e assertion accepted any error, which a server error or a state-guard refusal would have satisfied; it now requires a precondition failure. And the 412 message both conditional writes return is a single constant, since on delete the message is the only thing separating staleness from the state guards.
This commit is contained in:
@@ -251,7 +251,10 @@ func TestSettingsConditionalWrites(t *testing.T) {
|
||||
|
||||
// The delete is conditional too, and refusing a stale one leaves the
|
||||
// endpoint claimed.
|
||||
require.Error(t, fresh.DeleteSettingsIfMatch(ctx, etag), "a stale precondition must refuse the delete")
|
||||
err = fresh.DeleteSettingsIfMatch(ctx, etag)
|
||||
require.Error(t, err, "a stale precondition must refuse the delete")
|
||||
require.True(t, rest.IsPreconditionFailed(err),
|
||||
"the delete must be refused for staleness rather than for a state guard or a server error, got: %v", err)
|
||||
stillThere, err := fresh.GetSettings(ctx)
|
||||
require.NoError(t, err, "read after the refused delete must succeed")
|
||||
assert.Equal(t, planned.Endpoint, stillThere.Endpoint, "the refused delete must leave the endpoint claimed")
|
||||
|
||||
@@ -545,6 +545,13 @@ func (m *managerImpl) DeleteBudgetRule(ctx context.Context, accountID, userID, r
|
||||
return nil
|
||||
}
|
||||
|
||||
// stalePreconditionMsg is the refusal both conditional settings writes return.
|
||||
// Shared so the two cannot drift: DeleteSettings answers 412 for its state
|
||||
// guards as well, so the message is the only thing telling a client that it is
|
||||
// working from an old read rather than tripping over providers or a serving
|
||||
// proxy.
|
||||
const stalePreconditionMsg = "if-match precondition failed: the settings have changed since they were read; GET them again and retry"
|
||||
|
||||
// UpdateSettings replaces the mutable account-level settings — the collection
|
||||
// toggles and retention — on the account's row. The identity fields (Domain,
|
||||
// ProxyAddress) are assigned at bootstrap (CreateSettings) and immutable: the
|
||||
@@ -589,7 +596,7 @@ func (m *managerImpl) UpdateSettings(ctx context.Context, userID string, setting
|
||||
// working from an old read" is the more accurate answer than "the
|
||||
// endpoint is immutable".
|
||||
if !precondition.Matches(existing.ETag()) {
|
||||
return status.Errorf(status.PreconditionFailed, "if-match precondition failed: the settings have changed since they were read; GET them again and retry")
|
||||
return status.Errorf(status.PreconditionFailed, "%s", stalePreconditionMsg)
|
||||
}
|
||||
|
||||
// The identity echo is compared leniently (trimmed, case-insensitive):
|
||||
@@ -679,7 +686,7 @@ func (m *managerImpl) DeleteSettings(ctx context.Context, accountID, userID stri
|
||||
// before the state guards: a caller working from an old read should
|
||||
// learn that first, not be told about providers it may not know exist.
|
||||
if !precondition.Matches(existing.ETag()) {
|
||||
return status.Errorf(status.PreconditionFailed, "if-match precondition failed: the settings have changed since they were read; GET them again and retry")
|
||||
return status.Errorf(status.PreconditionFailed, "%s", stalePreconditionMsg)
|
||||
}
|
||||
|
||||
providers, err := tx.GetAccountAgentNetworkProviders(ctx, store.LockingStrengthNone, accountID)
|
||||
|
||||
@@ -79,12 +79,18 @@ func TestUpdateSettingsPreconditionSerializesConcurrentWriters(t *testing.T) {
|
||||
// a diff and is about to write the whole object back would.
|
||||
shared := created.ETag()
|
||||
|
||||
// noWrite is a retention value neither writer sends and the API would
|
||||
// never store, so an assertion that lands on it is a test bug rather than
|
||||
// a silently satisfied comparison. Zero would not do: the API documents 0
|
||||
// as "keep indefinitely", so it is a value the row could legitimately hold.
|
||||
const noWrite = -1
|
||||
|
||||
var (
|
||||
wg sync.WaitGroup
|
||||
start = make(chan struct{})
|
||||
errs = make([]error, 2)
|
||||
wrote = []int{7, 21}
|
||||
winner = make([]int, 2)
|
||||
wg sync.WaitGroup
|
||||
start = make(chan struct{})
|
||||
errs = make([]error, 2)
|
||||
wrote = []int{7, 21}
|
||||
returned = []int{noWrite, noWrite}
|
||||
)
|
||||
for i := range 2 {
|
||||
wg.Add(1)
|
||||
@@ -94,17 +100,19 @@ func TestUpdateSettingsPreconditionSerializesConcurrentWriters(t *testing.T) {
|
||||
updated, err := f.manager.UpdateSettings(ctx, userID, updateFor(created, wrote[i]), ifMatch(t, shared))
|
||||
errs[i] = err
|
||||
if err == nil {
|
||||
winner[i] = updated.AccessLogRetentionDays
|
||||
returned[i] = updated.AccessLogRetentionDays
|
||||
}
|
||||
}()
|
||||
}
|
||||
close(start)
|
||||
wg.Wait()
|
||||
|
||||
succeeded := 0
|
||||
succeeded, winner := 0, noWrite
|
||||
for i, err := range errs {
|
||||
if err == nil {
|
||||
succeeded++
|
||||
winner = wrote[i]
|
||||
assert.Equal(t, wrote[i], returned[i], "the winner's response must carry what it sent")
|
||||
continue
|
||||
}
|
||||
assert.Truef(t, isPreconditionFailed(err),
|
||||
@@ -115,7 +123,7 @@ func TestUpdateSettingsPreconditionSerializesConcurrentWriters(t *testing.T) {
|
||||
// The row must carry the winner's value and nothing blended.
|
||||
stored, err := f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, accountID)
|
||||
require.NoError(t, err, "the row must survive the race")
|
||||
assert.Contains(t, winner, stored.AccessLogRetentionDays,
|
||||
assert.Equal(t, winner, stored.AccessLogRetentionDays,
|
||||
"the stored row must be exactly what the winning writer sent")
|
||||
assert.NotEqual(t, shared, stored.ETag(), "the surviving row must derive a new validator")
|
||||
}
|
||||
|
||||
@@ -98,6 +98,19 @@ const etagLength = 16
|
||||
// held across that gap would then authorize a write against what is really a
|
||||
// different resource. CreatedAt is what distinguishes the re-bootstrapped row.
|
||||
//
|
||||
// CreatedAt is hashed at whole-second precision because the validator has to
|
||||
// agree across a store round-trip. A freshly bootstrapped row derives its
|
||||
// validator in memory, from a time.Time carrying nanoseconds, while every
|
||||
// later comparison derives it from a row read back out of the store — and the
|
||||
// engines truncate: PostgreSQL to microseconds, MySQL DATETIME to whole
|
||||
// seconds without an fsp. At nanosecond precision the two never agree again,
|
||||
// so the validator a bootstrap hands out is permanently unusable. Seconds is
|
||||
// the floor every supported engine preserves. The cost is that a delete and
|
||||
// re-bootstrap within the same second, onto the same endpoint and the same
|
||||
// toggles, derives the same validator; a labeled bootstrap draws a fresh
|
||||
// random label, so that needs a self-addressed endpoint reclaimed inside one
|
||||
// second.
|
||||
//
|
||||
// Adding a field to Settings means deciding whether it belongs here; the
|
||||
// field-count guard in the tests is what forces that decision.
|
||||
func (s *Settings) ETag() string {
|
||||
@@ -109,7 +122,7 @@ func (s *Settings) ETag() string {
|
||||
s.EnablePromptCollection,
|
||||
s.RedactPii,
|
||||
s.AccessLogRetentionDays,
|
||||
s.CreatedAt.UnixNano(),
|
||||
s.CreatedAt.Unix(),
|
||||
)
|
||||
return hex.EncodeToString(h.Sum(nil))[:etagLength]
|
||||
}
|
||||
|
||||
@@ -11,8 +11,10 @@ import (
|
||||
|
||||
// etagSettings is a fully populated settings row — every hashed field set to a
|
||||
// distinctive value — so a mutation test can flip exactly one thing at a time.
|
||||
// The timestamp carries sub-second precision on purpose: a whole-second value
|
||||
// would make the precision test below pass without proving anything.
|
||||
func etagSettings() *Settings {
|
||||
created := time.Date(2026, 8, 11, 9, 30, 0, 0, time.UTC)
|
||||
created := time.Date(2026, 8, 11, 9, 30, 0, 123456789, time.UTC)
|
||||
return &Settings{
|
||||
AccountID: "acc-1",
|
||||
Domain: "cool-otter.eu.proxy.netbird.io",
|
||||
@@ -73,7 +75,7 @@ func TestSettings_ETagSensitivity(t *testing.T) {
|
||||
{"prompt collection", func(s *Settings) { s.EnablePromptCollection = false }},
|
||||
{"redact pii", func(s *Settings) { s.RedactPii = false }},
|
||||
{"retention", func(s *Settings) { s.AccessLogRetentionDays = 14 }},
|
||||
{"created at", func(s *Settings) { s.CreatedAt = s.CreatedAt.Add(time.Nanosecond) }},
|
||||
{"created at", func(s *Settings) { s.CreatedAt = s.CreatedAt.Add(time.Second) }},
|
||||
// Moving characters across the Domain/ProxyAddress boundary leaves
|
||||
// the two fields' concatenation byte-identical, so this case passes
|
||||
// only because the tuple is delimited.
|
||||
@@ -120,6 +122,37 @@ func TestSettings_ETagExclusions(t *testing.T) {
|
||||
assert.Equal(t, baseline, touched.ETag(), "a write that changed nothing must not move the validator")
|
||||
}
|
||||
|
||||
// TestSettings_ETagSurvivesTimestampTruncation pins the store round-trip the
|
||||
// validator has to survive. A freshly bootstrapped row derives its validator
|
||||
// in memory, from a time.Time carrying nanoseconds; every later comparison
|
||||
// derives it from a row read back out of the store, and the engines truncate
|
||||
// on the way through — PostgreSQL to microseconds, MySQL DATETIME to whole
|
||||
// seconds without an fsp. If the hash is sensitive below its coarsest engine's
|
||||
// precision, the validator a bootstrap hands out never matches again and the
|
||||
// documented "conditional PUT without an intervening GET" is a permanent 412.
|
||||
//
|
||||
// Asserted here on the type rather than through a store, so it holds without
|
||||
// running the suite against every engine — which is what let this through the
|
||||
// first time, since sqlite preserves nanoseconds and every other test uses it.
|
||||
func TestSettings_ETagSurvivesTimestampTruncation(t *testing.T) {
|
||||
inMemory := etagSettings()
|
||||
require.NotZero(t, inMemory.CreatedAt.Nanosecond(), "the fixture must carry sub-second precision to prove anything")
|
||||
|
||||
for name, truncation := range map[string]time.Duration{
|
||||
"postgres (microseconds)": time.Microsecond,
|
||||
"mysql (milliseconds)": time.Millisecond,
|
||||
"mysql datetime (seconds)": time.Second,
|
||||
} {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
roundTripped := etagSettings()
|
||||
roundTripped.CreatedAt = roundTripped.CreatedAt.Truncate(truncation)
|
||||
|
||||
assert.Equal(t, inMemory.ETag(), roundTripped.ETag(),
|
||||
"a validator derived before the write must still match one derived after reading the row back")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestSettings_ETagOfDefaults covers the pre-bootstrap view, which GET serves
|
||||
// as a real representation and therefore validates like one. It must derive
|
||||
// without panicking on the zero CreatedAt, and it must not collide with a
|
||||
|
||||
Reference in New Issue
Block a user