diff --git a/management/internals/modules/agentnetwork/handlers/handlers_test.go b/management/internals/modules/agentnetwork/handlers/handlers_test.go index 9d855c05d..1c7f5fd31 100644 --- a/management/internals/modules/agentnetwork/handlers/handlers_test.go +++ b/management/internals/modules/agentnetwork/handlers/handlers_test.go @@ -92,6 +92,13 @@ func newAgentNetworkHandlerFixture(t *testing.T) *agentNetworkHandlerFixture { } func (f *agentNetworkHandlerFixture) do(t *testing.T, method, path, body string) *httptest.ResponseRecorder { + t.Helper() + return f.doWithHeaders(t, method, path, body, nil) +} + +// doWithHeaders is do with request headers, for the cases where the header is +// the thing under test (conditional requests). +func (f *agentNetworkHandlerFixture) doWithHeaders(t *testing.T, method, path, body string, headers map[string]string) *httptest.ResponseRecorder { t.Helper() var reader io.Reader if body != "" { @@ -101,6 +108,9 @@ func (f *agentNetworkHandlerFixture) do(t *testing.T, method, path, body string) if body != "" { req.Header.Set("Content-Type", "application/json") } + for name, value := range headers { + req.Header.Set(name, value) + } req = nbcontext.SetUserAuthInRequest(req, auth.UserAuth{ UserId: testUserID, AccountId: testAccountID, diff --git a/management/internals/modules/agentnetwork/handlers/settings_handler.go b/management/internals/modules/agentnetwork/handlers/settings_handler.go index 0a365f9ce..e56140458 100644 --- a/management/internals/modules/agentnetwork/handlers/settings_handler.go +++ b/management/internals/modules/agentnetwork/handlers/settings_handler.go @@ -60,12 +60,20 @@ func (h *handler) createSettings(w http.ResponseWriter, r *http.Request) { util.WriteError(r.Context(), err, w) return } + // Emitting the validator here lets a client that just bootstrapped issue a + // conditional PUT without an intervening GET. + util.SetETag(w, created.ETag()) util.WriteJSONObject(r.Context(), w, created.ToAPIResponse()) } // updateSettings replaces the mutable settings fields on the account's row. // A request carrying a cluster bootstraps the row when the account doesn't // have one yet. +// +// An If-Match header makes the update conditional: it is honoured against the +// stored row inside the write's transaction, and a stale validator is refused +// with 412 rather than overwriting what changed since the client read. Omitting +// the header keeps the pre-existing last-write-wins behaviour. func (h *handler) updateSettings(w http.ResponseWriter, r *http.Request) { userAuth, err := nbcontext.GetUserAuthFromContext(r.Context()) if err != nil { @@ -82,11 +90,12 @@ func (h *handler) updateSettings(w http.ResponseWriter, r *http.Request) { settings := &types.Settings{AccountID: userAuth.AccountId} settings.FromAPIRequest(&req) - updated, err := h.manager.UpdateSettings(r.Context(), userAuth.UserId, settings) + updated, err := h.manager.UpdateSettings(r.Context(), userAuth.UserId, settings, util.IfMatch(r)) if err != nil { util.WriteError(r.Context(), err, w) return } + util.SetETag(w, updated.ETag()) util.WriteJSONObject(r.Context(), w, updated.ToAPIResponse()) } @@ -94,6 +103,11 @@ func (h *handler) updateSettings(w http.ResponseWriter, r *http.Request) { // The manager refuses (412) while providers exist or a proxy is actively // serving the endpoint; a later POST bootstraps fresh, allocating a new // endpoint. +// +// An If-Match header makes the delete conditional, and is worth sending here +// even more than on update: both existing guards are about state rather than +// staleness, so nothing else stops a client from deleting a row that was +// replaced since it read one. func (h *handler) deleteSettings(w http.ResponseWriter, r *http.Request) { userAuth, err := nbcontext.GetUserAuthFromContext(r.Context()) if err != nil { @@ -101,7 +115,7 @@ func (h *handler) deleteSettings(w http.ResponseWriter, r *http.Request) { return } - if err := h.manager.DeleteSettings(r.Context(), userAuth.AccountId, userAuth.UserId); err != nil { + if err := h.manager.DeleteSettings(r.Context(), userAuth.AccountId, userAuth.UserId, util.IfMatch(r)); err != nil { util.WriteError(r.Context(), err, w) return } @@ -123,5 +137,9 @@ func (h *handler) getSettings(w http.ResponseWriter, r *http.Request) { util.WriteError(r.Context(), err, w) return } + // The pre-bootstrap defaults are a representation like any other and carry + // a validator too, so an If-Match taken before bootstrap cannot silently + // match the row that appeared since. + util.SetETag(w, settings.ETag()) util.WriteJSONObject(r.Context(), w, settings.ToAPIResponse()) } diff --git a/management/internals/modules/agentnetwork/handlers/settings_handler_test.go b/management/internals/modules/agentnetwork/handlers/settings_handler_test.go index 400208e1c..293c49147 100644 --- a/management/internals/modules/agentnetwork/handlers/settings_handler_test.go +++ b/management/internals/modules/agentnetwork/handlers/settings_handler_test.go @@ -393,3 +393,202 @@ func TestSettingsHandler_DeleteReleasesEndpointForFreshBootstrap(t *testing.T) { "the fresh row must carry bootstrap defaults, not the deleted row's toggles") assert.NotNil(t, second.CreatedAt, "the fresh row is persisted and carries timestamps") } + +// bootstrapForETag bootstraps a settings row and returns the response body +// alongside the validator the bootstrap emitted, which is what a client would +// carry into its first conditional write. +func bootstrapForETag(t *testing.T, f *agentNetworkHandlerFixture) (api.AgentNetworkSettings, string) { + t.Helper() + + rec := f.do(t, http.MethodPost, "/agent-network/settings", + `{"proxy_address": "eu.proxy.netbird.io", "enable_prompt_collection": true, "redact_pii": true, "access_log_retention_days": 14}`) + require.Equal(t, http.StatusOK, rec.Code, "bootstrap POST must succeed: %s", rec.Body.String()) + + var settings api.AgentNetworkSettings + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &settings)) + + etag := rec.Header().Get("ETag") + require.NotEmpty(t, etag, "bootstrap must emit a validator so a client can PUT without an intervening GET") + return settings, etag +} + +// putBody renders a complete settings update — every field, with the identity +// echo the endpoint requires — so the conditional-request tests differ only in +// their headers. +func putBody(settings api.AgentNetworkSettings, redactPii bool, retention int) string { + return fmt.Sprintf( + `{"endpoint": %q, "proxy_address": %q, "enable_log_collection": true, "enable_prompt_collection": true, "redact_pii": %t, "access_log_retention_days": %d}`, + settings.Endpoint, settings.ProxyAddress, redactPii, retention) +} + +// TestSettingsHandler_EmitsETag pins that every read and every write hands the +// client back a validator, quoted as a strong entity-tag. Without one on the +// write responses a client would have to re-GET after every update to stay +// able to make the next one conditional. +func TestSettingsHandler_EmitsETag(t *testing.T) { + f := newAgentNetworkHandlerFixture(t) + + // The pre-bootstrap defaults are a representation too, and validate like + // one — an If-Match taken here must not match the row that appears later. + rec := f.do(t, http.MethodGet, "/agent-network/settings", "") + require.Equal(t, http.StatusOK, rec.Code) + defaultsETag := rec.Header().Get("ETag") + assert.NotEmpty(t, defaultsETag, "the unbootstrapped view must carry a validator") + + settings, bootstrapETag := bootstrapForETag(t, f) + assert.Regexp(t, `^"[0-9a-f]+"$`, bootstrapETag, "the validator must be a quoted strong entity-tag") + assert.NotEqual(t, defaultsETag, bootstrapETag, "bootstrapping must move the validator") + + rec = f.do(t, http.MethodGet, "/agent-network/settings", "") + require.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, bootstrapETag, rec.Header().Get("ETag"), + "reading an unchanged row must derive the same validator the bootstrap returned") + + rec = f.do(t, http.MethodPut, "/agent-network/settings", putBody(settings, false, 7)) + require.Equal(t, http.StatusOK, rec.Code, "update must succeed: %s", rec.Body.String()) + assert.NotEqual(t, bootstrapETag, rec.Header().Get("ETag"), + "an update that changed the representation must return a different validator") +} + +// TestSettingsHandler_PutIfMatch walks the conditional-update contract. The +// stale case is the one the feature exists for: a client that planned against +// an earlier read must be refused rather than silently reverting whatever +// changed in between — RedactPii above all, where a silent revert turns a +// compliance control off with no error and no drift warning. +func TestSettingsHandler_PutIfMatch(t *testing.T) { + t.Run("matching validator succeeds", func(t *testing.T) { + f := newAgentNetworkHandlerFixture(t) + settings, etag := bootstrapForETag(t, f) + + rec := f.doWithHeaders(t, http.MethodPut, "/agent-network/settings", + putBody(settings, false, 7), map[string]string{"If-Match": etag}) + require.Equal(t, http.StatusOK, rec.Code, + "a matching precondition must be honoured: got %d body=%s", rec.Code, rec.Body.String()) + assert.NotEqual(t, etag, rec.Header().Get("ETag"), + "the response must carry the new validator, not the one that was matched") + }) + + t.Run("stale validator is refused and changes nothing", func(t *testing.T) { + f := newAgentNetworkHandlerFixture(t) + settings, stale := bootstrapForETag(t, f) + + // Someone else writes in between — the dashboard operator enabling + // something the planning client never saw. + rec := f.do(t, http.MethodPut, "/agent-network/settings", putBody(settings, true, 21)) + require.Equal(t, http.StatusOK, rec.Code, "the intervening update must succeed: %s", rec.Body.String()) + var intervened api.AgentNetworkSettings + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &intervened)) + + rec = f.doWithHeaders(t, http.MethodPut, "/agent-network/settings", + putBody(settings, false, 7), map[string]string{"If-Match": stale}) + require.Equal(t, http.StatusPreconditionFailed, rec.Code, + "a stale precondition must be refused: got %d body=%s", rec.Code, rec.Body.String()) + + // Asserting the state, not just the status: a partial write would pass + // a status-only check. + rec = f.do(t, http.MethodGet, "/agent-network/settings", "") + require.Equal(t, http.StatusOK, rec.Code) + var after api.AgentNetworkSettings + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &after)) + assert.Equal(t, intervened, after, "the refused update must leave the row byte-identical") + }) + + t.Run("star matches the existing row", func(t *testing.T) { + f := newAgentNetworkHandlerFixture(t) + settings, _ := bootstrapForETag(t, f) + + rec := f.doWithHeaders(t, http.MethodPut, "/agent-network/settings", + putBody(settings, false, 7), map[string]string{"If-Match": "*"}) + assert.Equal(t, http.StatusOK, rec.Code, + "* must match any current representation: got %d body=%s", rec.Code, rec.Body.String()) + }) + + t.Run("no precondition still succeeds", func(t *testing.T) { + f := newAgentNetworkHandlerFixture(t) + settings, _ := bootstrapForETag(t, f) + + // The back-compatibility guarantee: clients that predate conditional + // requests — the dashboard among them — keep last-write-wins. + rec := f.do(t, http.MethodPut, "/agent-network/settings", putBody(settings, false, 7)) + assert.Equal(t, http.StatusOK, rec.Code, + "an unconditional update must keep working: got %d body=%s", rec.Code, rec.Body.String()) + }) + + t.Run("precondition is checked before the immutability echo", func(t *testing.T) { + f := newAgentNetworkHandlerFixture(t) + settings, stale := bootstrapForETag(t, f) + + rec := f.do(t, http.MethodPut, "/agent-network/settings", putBody(settings, true, 21)) + require.Equal(t, http.StatusOK, rec.Code, "the intervening update must succeed: %s", rec.Body.String()) + + // A client stale enough to hold an old validator may be stale in its + // identity echo too. Answering 412 tells it the useful thing — go and + // read again — where 422 would send it hunting an immutability bug. + body := fmt.Sprintf( + `{"endpoint": "other.gateway.example.com", "proxy_address": %q, "enable_log_collection": true, "enable_prompt_collection": true, "redact_pii": false, "access_log_retention_days": 7}`, + settings.ProxyAddress) + rec = f.doWithHeaders(t, http.MethodPut, "/agent-network/settings", body, + map[string]string{"If-Match": stale}) + assert.Equal(t, http.StatusPreconditionFailed, rec.Code, + "staleness must be reported ahead of the identity mismatch: got %d body=%s", rec.Code, rec.Body.String()) + }) +} + +// TestSettingsHandler_DeleteIfMatch covers the conditional delete, which +// carries more weight than the conditional update: both existing delete guards +// are about state — no providers, no serving proxy — so nothing else stops a +// client from deleting a row that was replaced since it read one. +func TestSettingsHandler_DeleteIfMatch(t *testing.T) { + t.Run("stale validator is refused and the row survives", func(t *testing.T) { + f := newAgentNetworkHandlerFixture(t) + settings, stale := bootstrapForETag(t, f) + + rec := f.do(t, http.MethodPut, "/agent-network/settings", putBody(settings, true, 21)) + require.Equal(t, http.StatusOK, rec.Code, "the intervening update must succeed: %s", rec.Body.String()) + + rec = f.doWithHeaders(t, http.MethodDelete, "/agent-network/settings", "", + map[string]string{"If-Match": stale}) + require.Equal(t, http.StatusPreconditionFailed, rec.Code, + "a stale precondition must refuse the delete: got %d body=%s", rec.Code, rec.Body.String()) + + rec = f.do(t, http.MethodGet, "/agent-network/settings", "") + require.Equal(t, http.StatusOK, rec.Code) + var after api.AgentNetworkSettings + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &after)) + assert.Equal(t, settings.Endpoint, after.Endpoint, "the refused delete must leave the row in place") + }) + + t.Run("matching validator deletes", func(t *testing.T) { + f := newAgentNetworkHandlerFixture(t) + _, etag := bootstrapForETag(t, f) + + rec := f.doWithHeaders(t, http.MethodDelete, "/agent-network/settings", "", + map[string]string{"If-Match": etag}) + require.Equal(t, http.StatusOK, rec.Code, + "a matching precondition must be honoured: got %d body=%s", rec.Code, rec.Body.String()) + + rec = f.do(t, http.MethodGet, "/agent-network/settings", "") + require.Equal(t, http.StatusOK, rec.Code) + var after api.AgentNetworkSettings + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &after)) + assert.Empty(t, after.Endpoint, "the row must be gone") + }) + + t.Run("precondition is checked before the state guards", func(t *testing.T) { + f := newAgentNetworkHandlerFixture(t) + settings, stale := bootstrapForETag(t, f) + + rec := f.do(t, http.MethodPut, "/agent-network/settings", putBody(settings, true, 21)) + require.Equal(t, http.StatusOK, rec.Code, "the intervening update must succeed: %s", rec.Body.String()) + f.seedProvider(t, "prov-precondition") + + // Both refusals are 412, so the status cannot tell them apart — the + // message must, or a stale client is sent to delete providers it may + // not even know about. + rec = f.doWithHeaders(t, http.MethodDelete, "/agent-network/settings", "", + map[string]string{"If-Match": stale}) + require.Equal(t, http.StatusPreconditionFailed, rec.Code, "the delete must be refused: %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), "if-match", + "staleness must be reported ahead of the provider guard: %s", rec.Body.String()) + }) +} diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 379672989..5e4274910 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -22,6 +22,7 @@ import ( "github.com/netbirdio/netbird/management/server/permissions/modules" "github.com/netbirdio/netbird/management/server/permissions/operations" "github.com/netbirdio/netbird/management/server/store" + httputil "github.com/netbirdio/netbird/shared/management/http/util" "github.com/netbirdio/netbird/shared/management/status" ) @@ -71,8 +72,8 @@ type Manager interface { GetSettings(ctx context.Context, accountID, userID string) (*types.Settings, error) CreateSettings(ctx context.Context, userID string, settings *types.Settings, proxyAddress, endpoint string) (*types.Settings, error) - UpdateSettings(ctx context.Context, userID string, settings *types.Settings) (*types.Settings, error) - DeleteSettings(ctx context.Context, accountID, userID string) error + UpdateSettings(ctx context.Context, userID string, settings *types.Settings, precondition *httputil.Precondition) (*types.Settings, error) + DeleteSettings(ctx context.Context, accountID, userID string, precondition *httputil.Precondition) error ListConsumption(ctx context.Context, accountID, userID string) ([]*types.Consumption, error) ListAccessLogs(ctx context.Context, accountID, userID string, filter types.AgentNetworkAccessLogFilter) ([]*types.AgentNetworkAccessLog, int64, error) @@ -554,7 +555,11 @@ func (m *managerImpl) DeleteBudgetRule(ctx context.Context, accountID, userID, r // Because the collection toggles change the synthesised service config // (prompt-capture gating, access-log emission), a reconcile is triggered so // the proxy and peer network maps converge on the new state. -func (m *managerImpl) UpdateSettings(ctx context.Context, userID string, settings *types.Settings) (*types.Settings, error) { +// +// precondition carries the caller's If-Match, and is nil for an unconditional +// update — last write wins, which is what the dashboard wants and what every +// client that predates conditional requests gets. +func (m *managerImpl) UpdateSettings(ctx context.Context, userID string, settings *types.Settings, precondition *httputil.Precondition) (*types.Settings, error) { if err := m.requirePermission(ctx, settings.AccountID, userID, modules.AgentNetworkSettings, operations.Update); err != nil { return nil, err } @@ -573,6 +578,20 @@ func (m *managerImpl) UpdateSettings(ctx context.Context, userID string, setting return fmt.Errorf("get agent network settings: %w", err) } + // Evaluated here, under the row lock and inside the write's own + // transaction, rather than in the handler: comparing before the + // transaction only narrows the race, since two requests can both pass + // the check before either writes. Locking the row first makes it a + // genuine compare-and-set. + // + // It comes before the identity comparison because a client holding a + // stale validator is stale in its identity echo too, and "you are + // 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") + } + // The identity echo is compared leniently (trimmed, case-insensitive): // the stored values are normalized lowercase, and a client replaying a // GET response must never be rejected over casing it didn't choose. @@ -635,7 +654,12 @@ func hostnamesEquivalent(supplied, stored string) bool { // is not reserved. That full-reset semantic is what gives clients that model // immutability as replace-on-change (e.g. Terraform's RequiresReplace) a real // path: tear down providers, delete, re-create. -func (m *managerImpl) DeleteSettings(ctx context.Context, accountID, userID string) error { +// +// precondition carries the caller's If-Match, and is nil for an unconditional +// delete. It matters more here than on update: the two guards above are about +// state rather than staleness, so without it nothing stops a client from +// deleting a row that was replaced since it last read one. +func (m *managerImpl) DeleteSettings(ctx context.Context, accountID, userID string, precondition *httputil.Precondition) error { if err := m.requirePermission(ctx, accountID, userID, modules.AgentNetworkSettings, operations.Delete); err != nil { return err } @@ -651,6 +675,13 @@ func (m *managerImpl) DeleteSettings(ctx context.Context, accountID, userID stri return fmt.Errorf("get agent network settings: %w", err) } + // Under the row lock, for the same reason as in UpdateSettings, and + // 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") + } + providers, err := tx.GetAccountAgentNetworkProviders(ctx, store.LockingStrengthNone, accountID) if err != nil { return fmt.Errorf("get agent network providers: %w", err) @@ -1100,11 +1131,13 @@ func (*mockManager) CreateSettings(_ context.Context, _ string, s *types.Setting return s, nil } -func (*mockManager) UpdateSettings(_ context.Context, _ string, s *types.Settings) (*types.Settings, error) { +func (*mockManager) UpdateSettings(_ context.Context, _ string, s *types.Settings, _ *httputil.Precondition) (*types.Settings, error) { return s, nil } -func (*mockManager) DeleteSettings(_ context.Context, _, _ string) error { return nil } +func (*mockManager) DeleteSettings(_ context.Context, _, _ string, _ *httputil.Precondition) error { + return nil +} func (*mockManager) ListConsumption(_ context.Context, _, _ string) ([]*types.Consumption, error) { return nil, nil diff --git a/management/internals/modules/agentnetwork/settings_etag_test.go b/management/internals/modules/agentnetwork/settings_etag_test.go new file mode 100644 index 000000000..db54df5e9 --- /dev/null +++ b/management/internals/modules/agentnetwork/settings_etag_test.go @@ -0,0 +1,191 @@ +package agentnetwork + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strconv" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" + "github.com/netbirdio/netbird/management/server/permissions/modules" + "github.com/netbirdio/netbird/management/server/permissions/operations" + "github.com/netbirdio/netbird/management/server/store" + httputil "github.com/netbirdio/netbird/shared/management/http/util" + "github.com/netbirdio/netbird/shared/management/status" +) + +// ifMatch builds the precondition a client sending this validator would +// produce, by going through the same header parse the handler uses rather than +// reaching past it. +func ifMatch(t *testing.T, etag string) *httputil.Precondition { + t.Helper() + + r := httptest.NewRequest(http.MethodPut, "/", nil) + r.Header.Set("If-Match", strconv.Quote(etag)) + return httputil.IfMatch(r) +} + +// updateFor renders a complete update for the given row, echoing the identity +// fields the endpoint requires and setting retention to tell writers apart. +func updateFor(settings *types.Settings, retention int) *types.Settings { + return &types.Settings{ + AccountID: settings.AccountID, + Domain: settings.Domain, + ProxyAddress: settings.ProxyAddress, + EnableLogCollection: true, + EnablePromptCollection: true, + RedactPii: true, + AccessLogRetentionDays: retention, + } +} + +// TestUpdateSettingsPreconditionSerializesConcurrentWriters is the test the +// design rests on. Two writers start from the same validator and race; exactly +// one may win. +// +// An implementation that compares the validator before opening the write +// transaction passes every sequential test in this suite and fails here: both +// writers read the same row, both find their precondition satisfied, and both +// then write — which is the lost update the feature exists to prevent, merely +// narrowed to a smaller window. Holding the row under LockingStrengthUpdate +// and comparing inside the write's own transaction is what makes it a genuine +// compare-and-set. +// +// The test store is sqlite, which serializes writers of its own accord, so +// what this pins directly is the outcome — exactly one success — rather than +// the mechanism. It still has teeth against the check-before-transaction +// shape, whose two reads interleave freely before either write. Running it +// against postgres (NB_STORE_ENGINE_POSTGRES_DSN) exercises real concurrent +// transactions. +func TestUpdateSettingsPreconditionSerializesConcurrentWriters(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + + const accountID, userID = "account1", "user1" + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Create, true) + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Update, true) + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Update, true) + + created, err := f.createSettings(ctx, accountID, userID, "cluster1.example.com", "") + require.NoError(t, err, "bootstrap must succeed") + + // Both writers plan against this one read, as a client that read, computed + // a diff and is about to write the whole object back would. + shared := created.ETag() + + var ( + wg sync.WaitGroup + start = make(chan struct{}) + errs = make([]error, 2) + wrote = []int{7, 21} + winner = make([]int, 2) + ) + for i := range 2 { + wg.Add(1) + go func() { + defer wg.Done() + <-start + updated, err := f.manager.UpdateSettings(ctx, userID, updateFor(created, wrote[i]), ifMatch(t, shared)) + errs[i] = err + if err == nil { + winner[i] = updated.AccessLogRetentionDays + } + }() + } + close(start) + wg.Wait() + + succeeded := 0 + for i, err := range errs { + if err == nil { + succeeded++ + continue + } + assert.Truef(t, isPreconditionFailed(err), + "the losing writer must be refused for staleness, got: %v (writer %d)", err, i) + } + require.Equal(t, 1, succeeded, "exactly one writer may win: %v", errs) + + // 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, + "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") +} + +// TestUpdateSettingsUnconditionalIgnoresStaleness pins the back-compatibility +// half: without a precondition the manager keeps last-write-wins, which is +// what the dashboard relies on and what every client written before this +// existed does. +func TestUpdateSettingsUnconditionalIgnoresStaleness(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + + const accountID, userID = "account1", "user1" + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Create, true) + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Update, true) + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Update, true) + + created, err := f.createSettings(ctx, accountID, userID, "cluster1.example.com", "") + require.NoError(t, err, "bootstrap must succeed") + + _, err = f.manager.UpdateSettings(ctx, userID, updateFor(created, 21), nil) + require.NoError(t, err, "the first unconditional update must succeed") + + // The second writer is working from a read that is now stale, and with no + // precondition it overwrites regardless. + updated, err := f.manager.UpdateSettings(ctx, userID, updateFor(created, 7), nil) + require.NoError(t, err, "an unconditional update must not be refused for staleness") + assert.Equal(t, 7, updated.AccessLogRetentionDays, "last write wins without a precondition") +} + +// TestDeleteSettingsPreconditionRefusesStale pins the conditional delete at +// the manager level: a stale validator refuses, and the row is still there +// afterwards. Deletion is the destructive operation and its two other guards +// are about state rather than staleness, so this is the only thing standing +// between a client working from an old read and a released endpoint. +func TestDeleteSettingsPreconditionRefusesStale(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + + const accountID, userID = "account1", "user1" + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Create, true) + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Update, true) + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Delete, true) + f.expectPermission(accountID, userID, modules.AgentNetworkSettings, operations.Delete, true) + + created, err := f.createSettings(ctx, accountID, userID, "cluster1.example.com", "") + require.NoError(t, err, "bootstrap must succeed") + stale := created.ETag() + + updated, err := f.manager.UpdateSettings(ctx, userID, updateFor(created, 21), nil) + require.NoError(t, err, "the intervening update must succeed") + + err = f.manager.DeleteSettings(ctx, accountID, userID, ifMatch(t, stale)) + require.Error(t, err, "a stale precondition must refuse the delete") + assert.True(t, isPreconditionFailed(err), "the refusal must be a precondition failure, got: %v", err) + + stored, err := f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, accountID) + require.NoError(t, err, "the refused delete must leave the row in place") + assert.Equal(t, created.Domain, stored.Domain, "the endpoint must not have been released") + + // The validator the intervening update returned is the current one, and + // deleting with it goes through. + require.NoError(t, f.manager.DeleteSettings(ctx, accountID, userID, ifMatch(t, updated.ETag())), + "a matching precondition must be honoured") + _, err = f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, accountID) + assert.Error(t, err, "the row must be gone") +} + +// isPreconditionFailed reports whether err is the 412-mapped status error. +func isPreconditionFailed(err error) bool { + var sErr *status.Error + return errors.As(err, &sErr) && sErr.Type() == status.PreconditionFailed +} diff --git a/management/server/agentnetwork_budgetrule_realstack_test.go b/management/server/agentnetwork_budgetrule_realstack_test.go index 95f9c35dc..2f9bb5252 100644 --- a/management/server/agentnetwork_budgetrule_realstack_test.go +++ b/management/server/agentnetwork_budgetrule_realstack_test.go @@ -117,7 +117,7 @@ func TestAgentNetwork_UpdateSettings_PreservesImmutableAndTogglesCollection(t *t EnablePromptCollection: true, RedactPii: true, AccessLogRetentionDays: before.AccessLogRetentionDays, - }) + }, nil) require.NoError(t, err, "UpdateSettings must succeed") assert.Equal(t, before.Domain, updated.Domain, "domain is immutable and must be preserved") assert.Equal(t, before.ProxyAddress, updated.ProxyAddress, "proxy address is immutable and must be preserved") @@ -147,7 +147,7 @@ func TestAgentNetwork_UpdateSettings_PreservesImmutableAndTogglesCollection(t *t EnablePromptCollection: false, RedactPii: false, AccessLogRetentionDays: before.AccessLogRetentionDays, - }) + }, nil) assert.Error(t, err, "a mismatched identity echo must be rejected") assert.ErrorContains(t, err, "immutable", "the rejection must name the immutability rule") })