diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 29fd978dc..98243869c 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -221,10 +221,10 @@ func TestHandler_forgotPassword_RateLimitExceeded(t *testing.T) { } // --------------------------------------------------------------------------- -// applyOverrideSlices — exercise all branches +// overlayOverrideSlices — exercise all branches // --------------------------------------------------------------------------- -func TestApplyOverrideSlices(t *testing.T) { +func TestOverlayOverrideSlices(t *testing.T) { override := &config.AccountServiceOverride{} req := AccountServiceOverrideRequest{ IncludeEngines: []string{"mysql"}, @@ -235,7 +235,10 @@ func TestApplyOverrideSlices(t *testing.T) { ExcludeTypes: []string{"db.r5.large"}, } - applyOverrideSlices(override, req) + overlayOverrideSlices(override, req, map[string]bool{ + "include_engines": true, "exclude_engines": true, "include_regions": true, + "exclude_regions": true, "include_types": true, "exclude_types": true, + }) assert.Equal(t, []string{"mysql"}, override.IncludeEngines) assert.Equal(t, []string{"postgres"}, override.ExcludeEngines) @@ -245,12 +248,12 @@ func TestApplyOverrideSlices(t *testing.T) { assert.Equal(t, []string{"db.r5.large"}, override.ExcludeTypes) } -func TestApplyOverrideSlices_NilFields(t *testing.T) { +func TestOverlayOverrideSlices_AbsentKeysKept(t *testing.T) { override := &config.AccountServiceOverride{ IncludeEngines: []string{"existing"}, } - // Nil fields should not overwrite existing values - applyOverrideSlices(override, AccountServiceOverrideRequest{}) + // Keys absent from the body must not overwrite existing values + overlayOverrideSlices(override, AccountServiceOverrideRequest{}, map[string]bool{}) assert.Equal(t, []string{"existing"}, override.IncludeEngines) } diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index b3dbddb06..a9fff55cd 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -79,6 +79,16 @@ type AccountServiceOverrideRequest struct { ExcludeTypes []string `json:"exclude_types,omitempty"` } +// serviceOverrideOverlayKeys are the JSON keys of AccountServiceOverrideRequest. +// A key present in the PUT body replaces the stored value (null clears it); an +// absent key keeps it. +var serviceOverrideOverlayKeys = []string{ + "enabled", "term", "payment", "coverage", "ramp_schedule", + "include_engines", "exclude_engines", + "include_regions", "exclude_regions", + "include_types", "exclude_types", +} + // validCredentialTypes is the set of accepted credential type names. var validCredentialTypes = map[string]bool{ "aws_access_keys": true, @@ -1171,10 +1181,13 @@ func (h *Handler) saveAccountServiceOverride(ctx context.Context, httpReq *event return nil, err } - _, err = h.requireAccountAccess(ctx, session, accountID) + account, err := h.requireAccountAccess(ctx, session, accountID) if err != nil { return nil, err } + // The store's lock key is built from the id, so every spelling of one + // account must reach it as the stored (canonical) one. + accountID = account.ID var req AccountServiceOverrideRequest err = json.Unmarshal([]byte(httpReq.Body), &req) @@ -1182,19 +1195,39 @@ func (h *Handler) saveAccountServiceOverride(ctx context.Context, httpReq *event return nil, NewClientError(400, "invalid request body") } - now := time.Now() - existing, err := h.config.GetAccountServiceOverride(ctx, accountID, provider, service) + present, err := presentKeys(httpReq.Body, serviceOverrideOverlayKeys) if err != nil { - return nil, fmt.Errorf("accounts: get existing override: %w", err) + return nil, err } - override := buildServiceOverride(accountID, provider, service, req, existing, now) + // Fetch the commitment options before the transaction opens: the merge + // callback runs while it holds a pooled connection, so it must not take + // another (see commitmentOptsSnapshot). + combos := h.prefetchCommitmentOptions(ctx, provider) + + // The read, merge and write run in one locked transaction (issue #1775), + // so two single-field PUTs cannot merge onto the same stale row and lose + // one change. The audit event commits with the write (issue #1493). + audit := config.AuditContext{Actor: configAuditActor(session)} + override, err := h.config.UpdateAccountServiceOverrideAtomic(ctx, accountID, provider, service, audit, + func(existing *config.AccountServiceOverride) (*config.AccountServiceOverride, error) { + return mergeAndValidateServiceOverride(accountID, provider, service, req, existing, present, combos) + }) + if err != nil { + return nil, fmt.Errorf("accounts: %w", err) + } + + return override, nil +} - // Defense-in-depth: reject invalid (term, payment) combos before persisting. - // checkCommitmentOptionCombo is permissive when commitmentOpts is nil or - // probe data is absent (ErrNoData) — the frontend's hardcoded rules are the - // primary gate in those cases. - if err := h.checkCommitmentOptionCombo(ctx, config.ServiceConfig{ +// mergeAndValidateServiceOverride is the read-merge step of an override PUT, +// run inside the store's locked transaction: it merges the request onto the +// stored row, then rejects an invalid (term, payment) combo on the merged +// result. The check is permissive when no probe data exists, as for the +// service-config PUT. It does no I/O. +func mergeAndValidateServiceOverride(accountID, provider, service string, req AccountServiceOverrideRequest, existing *config.AccountServiceOverride, present map[string]bool, combos commitmentOptsSnapshot) (*config.AccountServiceOverride, error) { + override := buildServiceOverride(accountID, provider, service, req, existing, present, time.Now()) + if err := combos.check(config.ServiceConfig{ Provider: override.Provider, Service: override.Service, Term: derefInt(override.Term), @@ -1202,81 +1235,65 @@ func (h *Handler) saveAccountServiceOverride(ctx context.Context, httpReq *event }); err != nil { return nil, err } - - // The write and its config_audit_events row commit together (issue #1493). - audit := config.AuditContext{Actor: configAuditActor(session)} - if err := h.config.WithTx(ctx, func(tx pgx.Tx) error { - return h.config.SaveAccountServiceOverrideAuditedTx(ctx, tx, override, audit) - }); err != nil { - return nil, fmt.Errorf("accounts: %w", err) - } - return override, nil } -// buildServiceOverride constructs an AccountServiceOverride from request and existing data. -func buildServiceOverride(accountID, provider, service string, req AccountServiceOverrideRequest, existing *config.AccountServiceOverride, now time.Time) *config.AccountServiceOverride { - override := &config.AccountServiceOverride{ - AccountID: accountID, - Provider: provider, - Service: service, - UpdatedAt: now, - } - +// buildServiceOverride constructs an AccountServiceOverride by overlaying the +// request onto the stored row (or an empty one). present names the request +// keys the body carried: only those replace the stored value, so a one-field +// PUT keeps the rest (#1775). A present key with a null value clears it. +func buildServiceOverride(accountID, provider, service string, req AccountServiceOverrideRequest, existing *config.AccountServiceOverride, present map[string]bool, now time.Time) *config.AccountServiceOverride { + override := &config.AccountServiceOverride{} if existing != nil { - override.ID = existing.ID - override.CreatedAt = existing.CreatedAt + *override = *existing } else { override.ID = uuid.New().String() override.CreatedAt = now } + override.AccountID, override.Provider, override.Service = accountID, provider, service + override.UpdatedAt = now - applyServiceOverrideFields(override, req) + overlayOverrideScalars(override, req, present) + overlayOverrideSlices(override, req, present) return override } -// applyServiceOverrideFields copies sparse request fields onto an override. -func applyServiceOverrideFields(o *config.AccountServiceOverride, req AccountServiceOverrideRequest) { - applyOverrideScalars(o, req) - applyOverrideSlices(o, req) -} - -func applyOverrideScalars(o *config.AccountServiceOverride, req AccountServiceOverrideRequest) { - if req.Enabled != nil { +func overlayOverrideScalars(o *config.AccountServiceOverride, req AccountServiceOverrideRequest, present map[string]bool) { + if present["enabled"] { o.Enabled = req.Enabled } - if req.Term != nil { + if present["term"] { o.Term = req.Term } - if req.Payment != nil { + if present["payment"] { o.Payment = req.Payment } - if req.Coverage != nil { + if present["coverage"] { o.Coverage = req.Coverage } - if req.RampSchedule != nil { + if present["ramp_schedule"] { o.RampSchedule = req.RampSchedule } } -func applyOverrideSlices(o *config.AccountServiceOverride, req AccountServiceOverrideRequest) { - if req.IncludeEngines != nil { +func overlayOverrideSlices(o *config.AccountServiceOverride, req AccountServiceOverrideRequest, present map[string]bool) { + if present["include_engines"] { o.IncludeEngines = req.IncludeEngines } - if req.ExcludeEngines != nil { + if present["exclude_engines"] { o.ExcludeEngines = req.ExcludeEngines } - if req.IncludeRegions != nil { + if present["include_regions"] { o.IncludeRegions = req.IncludeRegions } - if req.ExcludeRegions != nil { + if present["exclude_regions"] { o.ExcludeRegions = req.ExcludeRegions } - if req.IncludeTypes != nil { + if present["include_types"] { o.IncludeTypes = req.IncludeTypes } - if req.ExcludeTypes != nil { + if present["exclude_types"] { o.ExcludeTypes = req.ExcludeTypes } } diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index beeb0362e..e3a0b6db4 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -1080,12 +1080,8 @@ func TestSaveAccountServiceOverride_InvalidCombo_Returns400(t *testing.T) { auth: mockAuth, config: store, commitmentOpts: &stubCommitmentOpts{ - validateFn: func(_ context.Context, provider, service string, term int, payment string) (bool, error) { - assert.Equal(t, "aws", provider) - assert.Equal(t, "rds", service) - assert.Equal(t, 3, term) - assert.Equal(t, "no-upfront", payment) - return false, nil + getFn: func(context.Context) (commitmentopts.Options, error) { + return commitmentopts.Options{"aws": {"rds": {{Provider: "aws", Service: "rds", TermYears: 1, Payment: "all-upfront"}}}}, nil }, }, } @@ -1115,8 +1111,8 @@ func TestSaveAccountServiceOverride_ValidCombo_Saves(t *testing.T) { auth: mockAuth, config: store, commitmentOpts: &stubCommitmentOpts{ - validateFn: func(context.Context, string, string, int, string) (bool, error) { - return true, nil + getFn: func(context.Context) (commitmentopts.Options, error) { + return commitmentopts.Options{"aws": {"rds": {{Provider: "aws", Service: "rds", TermYears: 1, Payment: "all-upfront"}}}}, nil }, }, } @@ -1163,8 +1159,8 @@ func TestSaveAccountServiceOverride_NoProbeData_Permissive(t *testing.T) { auth: mockAuth, config: store, commitmentOpts: &stubCommitmentOpts{ - validateFn: func(context.Context, string, string, int, string) (bool, error) { - return false, commitmentopts.ErrNoData + getFn: func(context.Context) (commitmentopts.Options, error) { + return nil, commitmentopts.ErrNoData }, }, } diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index f1f6b78c1..bad6784da 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -382,7 +382,7 @@ func mergeServiceConfig(existing *config.ServiceConfig, cfg config.ServiceConfig } // presentKeys returns the subset of keys that appear as top-level fields in the -// JSON body. A malformed body is reported as a client error so the caller +// JSON body, matched case-insensitively like encoding/json binds them. A malformed body is reported as a client error so the caller // returns 400 rather than silently overlaying nothing. func presentKeys(body string, keys []string) (map[string]bool, error) { var raw map[string]json.RawMessage @@ -391,7 +391,7 @@ func presentKeys(body string, keys []string) (map[string]bool, error) { } present := make(map[string]bool, len(keys)) for _, k := range keys { - if _, ok := raw[k]; ok { + if bodyHasKey(raw, k) { present[k] = true } } diff --git a/internal/api/handler_override_merge_integration_test.go b/internal/api/handler_override_merge_integration_test.go new file mode 100644 index 000000000..ac95fedf0 --- /dev/null +++ b/internal/api/handler_override_merge_integration_test.go @@ -0,0 +1,146 @@ +//go:build integration + +package api + +// Real-Postgres tests for the account service override PUT merge (issue +// #1775): a single-field PUT keeps every other stored field, and two +// concurrent single-field PUTs both persist. A mock cannot hold the row and +// advisory locks the race depends on. + +import ( + "context" + "fmt" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// overrideMergeRaceRounds is how many concurrent PUT pairs the race test fires. +const overrideMergeRaceRounds = 40 + +func fullOverrideFor(accountID string) *config.AccountServiceOverride { + o := fullStoredOverride() + o.ID, o.AccountID, o.CreatedAt = "", accountID, o.CreatedAt.UTC() + return o +} + +// A single-field PUT over a fully populated row, through the real store, +// changes that field only. +func TestOverrideMerge_SingleKeyPutKeepsOtherFieldsInStore(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditFixture(ctx, t) + seed := fullOverrideFor(f.acct.ID) + require.NoError(t, f.store.SaveAccountServiceOverride(ctx, seed)) + + f.put(ctx, t, "rds", `{"term":3}`) + + got, err := f.store.GetAccountServiceOverride(ctx, f.acct.ID, "aws", "rds") + require.NoError(t, err) + require.NotNil(t, got) + want := fullOverrideFor(f.acct.ID) + want.Term = ovPtr(3) + assert.Equal(t, want.Enabled, got.Enabled) + assert.Equal(t, want.Term, got.Term) + assert.Equal(t, want.Payment, got.Payment) + assert.Equal(t, want.Coverage, got.Coverage) + assert.Equal(t, want.RampSchedule, got.RampSchedule) + assert.Equal(t, want.IncludeEngines, got.IncludeEngines) + assert.Equal(t, want.ExcludeEngines, got.ExcludeEngines) + assert.Equal(t, want.IncludeRegions, got.IncludeRegions) + assert.Equal(t, want.ExcludeRegions, got.ExcludeRegions) + assert.Equal(t, want.IncludeTypes, got.IncludeTypes) + assert.Equal(t, want.ExcludeTypes, got.ExcludeTypes) +} + +// The reviewer's repro for #1775 (PR #1788): reset the row to term 1, +// all-upfront and exclude_regions [eu-west-1], then PUT {"exclude_regions": +// ["r-N"]} and {"term":3} at once. Both return 200; unless the read-merge-write +// is one locked transaction, one change is lost every round (an exclusion +// silently reverted widens what can be bought). +func TestOverrideMerge_ConcurrentSingleKeyPutsKeepBothKeys(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditFixture(ctx, t) + + lost := 0 + for round := 1; round <= overrideMergeRaceRounds; round++ { + require.NoError(t, f.store.SaveAccountServiceOverride(ctx, &config.AccountServiceOverride{ + AccountID: f.acct.ID, Provider: "aws", Service: "rds", + Term: ovPtr(1), Payment: ovPtr("all-upfront"), ExcludeRegions: []string{"eu-west-1"}, + })) + region := fmt.Sprintf("r-%d", round) + + bodies := []string{fmt.Sprintf(`{"exclude_regions":[%q]}`, region), `{"term":3}`} + start := make(chan struct{}) + errs := make([]error, len(bodies)) + var wg sync.WaitGroup + for i, body := range bodies { + wg.Add(1) + go func(i int, body string) { + defer wg.Done() + <-start + _, errs[i] = f.h.saveAccountServiceOverride(ctx, adminRequest(body), f.acct.ID+"/service-overrides/aws/rds") + }(i, body) + } + close(start) + wg.Wait() + for i, err := range errs { + require.NoError(t, err, "PUT %d (%s)", i, bodies[i]) + } + + got, err := f.store.GetAccountServiceOverride(ctx, f.acct.ID, "aws", "rds") + require.NoError(t, err) + if got.Term == nil || *got.Term != 3 || len(got.ExcludeRegions) != 1 || got.ExcludeRegions[0] != region { + lost++ + t.Logf("round %d: term=%v (want 3) exclude_regions=%v (want [%s])", round, got.Term, got.ExcludeRegions, region) + } + require.Equal(t, ovPtr("all-upfront"), got.Payment, "round %d: untouched field must survive", round) + } + require.Zero(t, lost, "%d of %d rounds lost one of two concurrent PUTs", lost, overrideMergeRaceRounds) +} + +// Two spellings of one account id must take the same lock: with the raw path id +// in the lock key, a first-time override raced by a lower- and an upper-case PUT +// merged both onto an empty row and the second write NULLed the first's field. +func TestOverrideMerge_ConcurrentFirstPutsAcrossIDSpellingsKeepBothKeys(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditFixture(ctx, t) + + lost := 0 + for round := 1; round <= overrideMergeRaceRounds; round++ { + _, err := f.pg.DB.Pool().Exec(ctx, `DELETE FROM account_service_overrides WHERE account_id = $1`, f.acct.ID) + require.NoError(t, err) + region := fmt.Sprintf("r-%d", round) + paths := []string{f.acct.ID, strings.ToUpper(f.acct.ID)} + bodies := []string{fmt.Sprintf(`{"exclude_regions":[%q]}`, region), `{"term":3}`} + + start := make(chan struct{}) + errs := make([]error, len(bodies)) + var wg sync.WaitGroup + for i := range bodies { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + _, errs[i] = f.h.saveAccountServiceOverride(ctx, adminRequest(bodies[i]), paths[i]+"/service-overrides/aws/rds") + }(i) + } + close(start) + wg.Wait() + for i, e := range errs { + require.NoError(t, e, "PUT %d (%s)", i, bodies[i]) + } + + got, err := f.store.GetAccountServiceOverride(ctx, f.acct.ID, "aws", "rds") + require.NoError(t, err) + if got.Term == nil || *got.Term != 3 || len(got.ExcludeRegions) != 1 || got.ExcludeRegions[0] != region { + lost++ + t.Logf("round %d: term=%v exclude_regions=%v (want 3, [%s])", round, got.Term, got.ExcludeRegions, region) + } + } + require.Zero(t, lost, "%d of %d rounds lost one of two concurrent first PUTs", lost, overrideMergeRaceRounds) +} diff --git a/internal/api/handler_override_merge_test.go b/internal/api/handler_override_merge_test.go new file mode 100644 index 000000000..2404e99c4 --- /dev/null +++ b/internal/api/handler_override_merge_test.go @@ -0,0 +1,237 @@ +package api + +import ( + "context" + "encoding/json" + "reflect" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/commitmentopts" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +const overrideMergeAccount = "11111111-1111-1111-1111-111111111111" + +func ovPtr[T any](v T) *T { return &v } + +// fullStoredOverride is a stored override with every overlay field set. +func fullStoredOverride() *config.AccountServiceOverride { + return &config.AccountServiceOverride{ + ID: "ov-1", AccountID: overrideMergeAccount, Provider: "aws", Service: "rds", + Enabled: ovPtr(true), Term: ovPtr(1), Payment: ovPtr("all-upfront"), + Coverage: ovPtr(80.0), RampSchedule: ovPtr("weekly-25pct"), + IncludeEngines: []string{"postgres"}, ExcludeEngines: []string{"mysql"}, + IncludeRegions: []string{"us-east-1"}, ExcludeRegions: []string{"eu-west-1"}, + IncludeTypes: []string{"db.m5.large"}, ExcludeTypes: []string{"db.t3.micro"}, + CreatedAt: time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC), + } +} + +// putOverride runs the PUT handler against a store holding existing and +// returns the row handed to SaveAccountServiceOverrideAuditedTx (nil if none). +func putOverride(t *testing.T, existing *config.AccountServiceOverride, body string, opts CommitmentOptsInterface) (*config.AccountServiceOverride, error) { + t.Helper() + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + store.On("GetAccountServiceOverride", ctx, overrideMergeAccount, "aws", "rds").Return(existing, nil) + var saved *config.AccountServiceOverride + store.On("SaveAccountServiceOverrideAuditedTx", ctx, mock.Anything, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { saved = args.Get(2).(*config.AccountServiceOverride) }).Return(nil) + h := &Handler{auth: mockAuth, config: store} + if opts != nil { + h.commitmentOpts = opts + } + _, err := h.saveAccountServiceOverride(ctx, adminRequest(body), overrideMergeAccount+"/service-overrides/aws/rds") + return saved, err +} + +// overrideMergeCases maps each overlay key to a body that changes it and the +// edit that produces the expected row from the stored one. +var overrideMergeCases = map[string]struct { + body string + apply func(*config.AccountServiceOverride) +}{ + "enabled": {`{"enabled":false}`, func(o *config.AccountServiceOverride) { o.Enabled = ovPtr(false) }}, + "term": {`{"term":3}`, func(o *config.AccountServiceOverride) { o.Term = ovPtr(3) }}, + "payment": {`{"payment":"no-upfront"}`, func(o *config.AccountServiceOverride) { o.Payment = ovPtr("no-upfront") }}, + "coverage": {`{"coverage":0}`, func(o *config.AccountServiceOverride) { o.Coverage = ovPtr(0.0) }}, + "ramp_schedule": {`{"ramp_schedule":"immediate"}`, func(o *config.AccountServiceOverride) { o.RampSchedule = ovPtr("immediate") }}, + "include_engines": {`{"include_engines":["aurora"]}`, func(o *config.AccountServiceOverride) { o.IncludeEngines = []string{"aurora"} }}, + "exclude_engines": {`{"exclude_engines":["oracle"]}`, func(o *config.AccountServiceOverride) { o.ExcludeEngines = []string{"oracle"} }}, + "include_regions": {`{"include_regions":["us-west-2"]}`, func(o *config.AccountServiceOverride) { o.IncludeRegions = []string{"us-west-2"} }}, + "exclude_regions": {`{"exclude_regions":["ap-south-1"]}`, func(o *config.AccountServiceOverride) { o.ExcludeRegions = []string{"ap-south-1"} }}, + "include_types": {`{"include_types":["db.r5.large"]}`, func(o *config.AccountServiceOverride) { o.IncludeTypes = []string{"db.r5.large"} }}, + "exclude_types": {`{"exclude_types":["db.t3.small"]}`, func(o *config.AccountServiceOverride) { o.ExcludeTypes = []string{"db.t3.small"} }}, +} + +// A single-key PUT (what the Settings UI sends) changes that key and keeps +// every other stored field (#1775). +func TestSaveAccountServiceOverride_SingleKeyKeepsOtherFields(t *testing.T) { + for key, tc := range overrideMergeCases { + t.Run(key, func(t *testing.T) { + want := fullStoredOverride() + tc.apply(want) + got, err := putOverride(t, fullStoredOverride(), tc.body, nil) + require.NoError(t, err) + require.NotNil(t, got) + assert.WithinDuration(t, time.Now(), got.UpdatedAt, time.Minute) + got.UpdatedAt = time.Time{} + assert.Equal(t, want, got) + }) + } +} + +// An explicit JSON null clears only that key back to inherit. +func TestSaveAccountServiceOverride_ExplicitNullClearsOnlyThatKey(t *testing.T) { + for key := range overrideMergeCases { + t.Run(key, func(t *testing.T) { + want := fullStoredOverride() + clearOverrideKey(want, key) + got, err := putOverride(t, fullStoredOverride(), `{"`+key+`":null}`, nil) + require.NoError(t, err) + got.UpdatedAt = time.Time{} + assert.Equal(t, want, got) + }) + } +} + +func clearOverrideKey(o *config.AccountServiceOverride, key string) { + switch key { + case "enabled": + o.Enabled = nil + case "term": + o.Term = nil + case "payment": + o.Payment = nil + case "coverage": + o.Coverage = nil + case "ramp_schedule": + o.RampSchedule = nil + case "include_engines": + o.IncludeEngines = nil + case "exclude_engines": + o.ExcludeEngines = nil + case "include_regions": + o.IncludeRegions = nil + case "exclude_regions": + o.ExcludeRegions = nil + case "include_types": + o.IncludeTypes = nil + case "exclude_types": + o.ExcludeTypes = nil + } +} + +// Key matching follows encoding/json: a differently-cased key still counts as sent. +func TestSaveAccountServiceOverride_KeyMatchIsCaseInsensitive(t *testing.T) { + want := fullStoredOverride() + want.Term = ovPtr(3) + got, err := putOverride(t, fullStoredOverride(), `{"Term":3}`, nil) + require.NoError(t, err) + got.UpdatedAt = time.Time{} + assert.Equal(t, want, got) +} + +// [] sets an empty list, which is not the same as null clearing the key. +func TestSaveAccountServiceOverride_EmptyListSetsEmptyNotNil(t *testing.T) { + want := fullStoredOverride() + want.IncludeRegions = []string{} + got, err := putOverride(t, fullStoredOverride(), `{"include_regions":[]}`, nil) + require.NoError(t, err) + got.UpdatedAt = time.Time{} + assert.Equal(t, want, got) +} + +// With no stored row the result carries only the sent fields. +func TestSaveAccountServiceOverride_NoExistingRowKeepsOnlySentFields(t *testing.T) { + got, err := putOverride(t, nil, `{"term":1,"include_regions":["us-east-1"]}`, nil) + require.NoError(t, err) + assert.NotEmpty(t, got.ID) + assert.Equal(t, ovPtr(1), got.Term) + assert.Equal(t, []string{"us-east-1"}, got.IncludeRegions) + assert.Nil(t, got.Enabled) + assert.Nil(t, got.Payment) + assert.Nil(t, got.Coverage) + assert.Nil(t, got.RampSchedule) + assert.Nil(t, got.IncludeEngines) + assert.Nil(t, got.ExcludeTypes) +} + +// The (term, payment) check sees the merged pair, not just the sent key. +func TestSaveAccountServiceOverride_ComboCheckedOnMergedRow(t *testing.T) { + existing := &config.AccountServiceOverride{ + ID: "ov-1", AccountID: overrideMergeAccount, Provider: "aws", Service: "rds", + Payment: ovPtr("no-upfront"), + } + // The options offer only 1yr no-upfront: the merged 3yr no-upfront is + // refused, though the sent term alone (no payment) would pass. + opts := &stubCommitmentOpts{ + getFn: func(context.Context) (commitmentopts.Options, error) { + return commitmentopts.Options{"aws": {"rds": {{Provider: "aws", Service: "rds", TermYears: 1, Payment: "no-upfront"}}}}, nil + }, + } + got, err := putOverride(t, existing, `{"term":3}`, opts) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.message, "3yr no-upfront") + assert.Nil(t, got, "a refused combo must not be saved") +} + +// Every json field of the request is merged: a new field must be added to the +// overlay allowlist (and to overrideMergeCases) or this fails. +func TestAccountServiceOverrideRequest_EveryFieldIsMerged(t *testing.T) { + var tags []string + rt := reflect.TypeOf(AccountServiceOverrideRequest{}) + for i := 0; i < rt.NumField(); i++ { + tags = append(tags, strings.Split(rt.Field(i).Tag.Get("json"), ",")[0]) + } + caseKeys := make([]string, 0, len(overrideMergeCases)) + for k := range overrideMergeCases { + caseKeys = append(caseKeys, k) + } + assert.ElementsMatch(t, tags, serviceOverrideOverlayKeys) + assert.ElementsMatch(t, tags, caseKeys) + + // The same set holds on the stored side: no request key is unknown to the row. + var row map[string]json.RawMessage + b, err := json.Marshal(fullStoredOverride()) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(b, &row)) + for _, k := range tags { + assert.Contains(t, row, k) + } +} + +// The store locks on the id it is given, so a mixed-case path id must reach it +// as the stored account's canonical id. +func TestSaveAccountServiceOverride_UsesCanonicalAccountID(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + const canonical = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + store.GetCloudAccountFn = func(_ context.Context, _ string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: canonical, Provider: "aws"}, nil + } + var gotID string + store.On("UpdateAccountServiceOverrideAtomic", ctx, mock.Anything, "aws", "rds", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { gotID = args.String(1) }). + Return(&config.AccountServiceOverride{}, nil) + h := &Handler{auth: mockAuth, config: store} + + _, err := h.saveAccountServiceOverride(ctx, adminRequest(`{"term":1}`), strings.ToUpper(canonical)+"/service-overrides/aws/rds") + require.NoError(t, err) + assert.Equal(t, canonical, gotID) +} diff --git a/internal/api/handler_present_keys_test.go b/internal/api/handler_present_keys_test.go new file mode 100644 index 000000000..1c1ed0ff0 --- /dev/null +++ b/internal/api/handler_present_keys_test.go @@ -0,0 +1,16 @@ +package api + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// encoding/json binds "Term" to the term field, so presentKeys must see it as +// present too, otherwise the overlay silently drops a value that was decoded. +func TestPresentKeys_CaseInsensitive(t *testing.T) { + got, err := presentKeys(`{"Term":3,"PAYMENT":null,"coverage":80}`, []string{"term", "payment", "coverage", "enabled"}) + require.NoError(t, err) + assert.Equal(t, map[string]bool{"term": true, "payment": true, "coverage": true}, got) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index d7b1da79e..f581b162f 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -4498,11 +4498,14 @@ paths: put: operationId: saveAccountServiceOverride tags: [Accounts] - summary: Create or replace an account's override for one service (admin only) + summary: Create or update an account's override for one service (admin only) description: > - Upserts the override for this account, provider and service. The - override is replaced as a whole: fields omitted from the body are - cleared and inherit the global service configuration. + Upserts the override for this account, provider and service. The body + is merged into the stored override: a field omitted from the body keeps + its stored value, a field sent as null is cleared and inherits the + global service configuration, and any other value replaces it. With no + stored override, only the fields sent are set. The AWS term and payment + combination is checked on the merged result. parameters: - $ref: '#/components/parameters/CSRFToken' requestBody: @@ -8250,40 +8253,53 @@ components: AccountServiceOverrideRequest: type: object - description: Fields to override. Omitted fields inherit the global value. + description: > + Fields to override. Omitted fields keep their stored value (or inherit + the global value when nothing is stored); null clears a field. properties: enabled: + nullable: true type: boolean term: + nullable: true type: integer payment: + nullable: true type: string coverage: + nullable: true type: number format: double ramp_schedule: + nullable: true type: string include_engines: + nullable: true type: array items: type: string exclude_engines: + nullable: true type: array items: type: string include_regions: + nullable: true type: array items: type: string exclude_regions: + nullable: true type: array items: type: string include_types: + nullable: true type: array items: type: string exclude_types: + nullable: true type: array items: type: string diff --git a/internal/api/override_transfer.go b/internal/api/override_transfer.go index a08881c6b..45e620a9c 100644 --- a/internal/api/override_transfer.go +++ b/internal/api/override_transfer.go @@ -15,8 +15,9 @@ import ( // UpdatedAt are informational: import ignores them. // // An import row is the desired state of the override: an unset field clears -// that field, unlike the sparse PUT /api/accounts/:id/service-overrides/... -// merge. Overrides missing from the file are left alone. +// that field, unlike the PUT /api/accounts/:id/service-overrides/... merge, +// which keeps the stored value of any key the body omits. Overrides missing +// from the file are left alone. type OverrideTransferRow struct { AccountID string `json:"account_id,omitempty"` AccountName string `json:"account_name,omitempty"` diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 51a01f14f..14ca8b13c 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -679,6 +679,12 @@ type StoreInterface interface { // write them in a consistent key order. A write that changes no setting // records nothing. override is updated to the stored row. SaveAccountServiceOverrideAuditedTx(ctx context.Context, tx pgx.Tx, override *AccountServiceOverride, audit AuditContext) error + // UpdateAccountServiceOverrideAtomic serializes a read-merge-write of one + // override under the same lock as SaveAccountServiceOverrideAuditedTx and + // audits the write. merge receives the stored row (nil when none) and + // returns the full row to write; its error aborts the write and is + // propagated unchanged. Returns the saved row. + UpdateAccountServiceOverrideAtomic(ctx context.Context, accountID, provider, service string, audit AuditContext, merge func(existing *AccountServiceOverride) (*AccountServiceOverride, error)) (*AccountServiceOverride, error) // DeleteAccountServiceOverrideAuditedTx deletes the override inside the // caller's transaction and records the deleted row in config_audit_events. // Deleting a missing override records nothing. diff --git a/internal/config/store_postgres_config_audit.go b/internal/config/store_postgres_config_audit.go index 792159f60..73b86124f 100644 --- a/internal/config/store_postgres_config_audit.go +++ b/internal/config/store_postgres_config_audit.go @@ -137,17 +137,68 @@ func scanAccountServiceOverride(row pgx.Row) (*AccountServiceOverride, error) { // Callers writing several overrides in one tx must call this in a consistent // key order, or two such transactions could deadlock on each other's locks. func (s *PostgresStore) SaveAccountServiceOverrideAuditedTx(ctx context.Context, tx pgx.Tx, o *AccountServiceOverride, audit AuditContext) error { - key := accountServiceOverrideLockPrefix + o.AccountID + "/" + o.Provider + "/" + o.Service + before, err := lockAndReadOverride(ctx, tx, o.AccountID, o.Provider, o.Service) + if err != nil { + return err + } + return writeOverrideAudited(ctx, tx, o, before, audit) +} + +// UpdateAccountServiceOverrideAtomic is a serialized read-merge-write of one +// override (issue #1775). In one transaction it takes the same advisory lock +// and FOR UPDATE read as SaveAccountServiceOverrideAuditedTx, hands the stored +// row (nil when none) to merge, and writes and audits the row merge returns. +// Two partial PUTs of one override therefore cannot merge onto the same stale +// base and lose a field. merge must not modify existing, and must not touch +// the database: the transaction holds a pooled connection. An error from +// merge aborts the transaction and is returned unchanged. The saved row is +// returned. +func (s *PostgresStore) UpdateAccountServiceOverrideAtomic(ctx context.Context, accountID, provider, service string, audit AuditContext, merge func(existing *AccountServiceOverride) (*AccountServiceOverride, error)) (*AccountServiceOverride, error) { + var saved *AccountServiceOverride + err := s.WithTx(ctx, func(tx pgx.Tx) error { + before, err := lockAndReadOverride(ctx, tx, accountID, provider, service) + if err != nil { + return err + } + next, err := merge(before) + if err != nil { + return err + } + if next == nil || next.AccountID != accountID || next.Provider != provider || next.Service != service { + return fmt.Errorf("service override update for %s/%s/%s must return a row with the same key", accountID, provider, service) + } + if err := writeOverrideAudited(ctx, tx, next, before, audit); err != nil { + return err + } + saved = next + return nil + }) + if err != nil { + return nil, err + } + return saved, nil +} + +// lockAndReadOverride takes the override's transaction-scoped advisory lock, +// then reads the stored row FOR UPDATE (nil when none). +func lockAndReadOverride(ctx context.Context, tx pgx.Tx, accountID, provider, service string) (*AccountServiceOverride, error) { + key := accountServiceOverrideLockPrefix + accountID + "/" + provider + "/" + service if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock(hashtextextended($1, 0))", key); err != nil { - return fmt.Errorf("failed to lock service override: %w", err) + return nil, fmt.Errorf("failed to lock service override: %w", err) } before, err := scanAccountServiceOverride(tx.QueryRow(ctx, `SELECT `+accountServiceOverrideColumns+` FROM account_service_overrides WHERE account_id = $1 AND provider = $2 AND service = $3 FOR UPDATE`, - o.AccountID, o.Provider, o.Service)) + accountID, provider, service)) if err != nil { - return fmt.Errorf("failed to read service override: %w", err) + return nil, fmt.Errorf("failed to read service override: %w", err) } + return before, nil +} + +// writeOverrideAudited upserts o, updates o to the stored row and records the +// change against before. +func writeOverrideAudited(ctx context.Context, tx pgx.Tx, o, before *AccountServiceOverride, audit AuditContext) error { after, err := upsertAccountServiceOverrideReturning(ctx, tx, o) if err != nil { return err diff --git a/internal/config/store_postgres_config_audit_integration_test.go b/internal/config/store_postgres_config_audit_integration_test.go index 1d6b747f4..1503cb57a 100644 --- a/internal/config/store_postgres_config_audit_integration_test.go +++ b/internal/config/store_postgres_config_audit_integration_test.go @@ -133,3 +133,58 @@ func TestOverrideAudit_AfterIsTheStoredRow(t *testing.T) { assert.Equal(t, 33.33, after["coverage"]) assert.Equal(t, 33.33, *o.Coverage, "the caller's override is updated to the stored row") } + +// TestOverrideAtomic_MergeErrorAbortsUnchanged: an error from merge rolls the +// transaction back (no row, no event) and reaches the caller as the same +// value, so a ClientError such as the combo 400 is not rewrapped. +func TestOverrideAtomic_MergeErrorAbortsUnchanged(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditStoreFixture(ctx, t) + sentinel := errors.New("combo refused") + + got, err := f.store.UpdateAccountServiceOverrideAtomic(ctx, f.account, "aws", "rds", AuditContext{}, + func(existing *AccountServiceOverride) (*AccountServiceOverride, error) { + assert.Nil(t, existing) + return nil, sentinel + }) + require.Nil(t, got) + require.Same(t, sentinel, err, "merge error must be returned unchanged") + stored, gerr := f.store.GetAccountServiceOverride(ctx, f.account, "aws", "rds") + require.NoError(t, gerr) + assert.Nil(t, stored) +} + +// TestOverrideAtomic_QueuesBehindCompetingWriter: the merge sees the row a +// concurrent writer committed, not the one that existed when the call began. +func TestOverrideAtomic_QueuesBehindCompetingWriter(t *testing.T) { + ctx := context.Background() + f := newOverrideAuditStoreFixture(ctx, t) + require.NoError(t, saveAccountServiceOverride(ctx, f.store.db, &AccountServiceOverride{AccountID: f.account, Provider: "aws", Service: "rds", Term: intPtrAudit(1)})) + + writer := f.hold(ctx, t) + _, err := writer.Exec(ctx, "SELECT pg_advisory_xact_lock(hashtextextended($1, 0))", + accountServiceOverrideLockPrefix+f.account+"/aws/rds") + require.NoError(t, err) + _, err = writer.Exec(ctx, `UPDATE account_service_overrides SET term = 3 WHERE account_id = $1`, f.account) + require.NoError(t, err) + + var seen *int + done := make(chan error, 1) + go func() { + _, e := f.store.UpdateAccountServiceOverrideAtomic(ctx, f.account, "aws", "rds", AuditContext{Actor: AuditActor{Identity: "admin@example.com"}}, + func(existing *AccountServiceOverride) (*AccountServiceOverride, error) { + seen = existing.Term + next := *existing + next.Coverage = floatPtrAudit(50) + return &next, nil + }) + done <- e + }() + testhelpers.WaitForLockWait(ctx, t, f.store.db, "") + require.NoError(t, writer.Commit(ctx)) + require.NoError(t, testhelpers.Await(t, done)) + require.NotNil(t, seen) + assert.Equal(t, 3, *seen, "merge must read the committed row") +} + +func floatPtrAudit(v float64) *float64 { return &v } diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 4b75bdc86..16cd49f26 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1806,6 +1806,40 @@ func (m *MockConfigStore) SaveAccountServiceOverrideAuditedTx(ctx context.Contex return args.Error(0) } +// UpdateAccountServiceOverrideAtomic mocks the locked read-merge-write of one +// override. With an expectation it dispatches through m.Called; otherwise it +// emulates the store via the mocked WithTx, GetAccountServiceOverride, merge +// and SaveAccountServiceOverrideAuditedTx, so tests that seed those keep working. +func (m *MockConfigStore) UpdateAccountServiceOverrideAtomic(ctx context.Context, accountID, provider, service string, audit config.AuditContext, merge func(*config.AccountServiceOverride) (*config.AccountServiceOverride, error)) (*config.AccountServiceOverride, error) { + m.record("UpdateAccountServiceOverrideAtomic", ctx, accountID, provider, service, audit, merge) + if isExpected(&m.Mock, "UpdateAccountServiceOverrideAtomic") { + args := m.Called(ctx, accountID, provider, service, audit, merge) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(*config.AccountServiceOverride) + if !ok { + m.reporter.wrongReturnType("UpdateAccountServiceOverrideAtomic", "*config.AccountServiceOverride", args.Get(0)) + } + return v, args.Error(1) + } + var next *config.AccountServiceOverride + err := m.WithTx(ctx, func(tx pgx.Tx) error { + existing, err := m.GetAccountServiceOverride(ctx, accountID, provider, service) + if err != nil { + return err + } + if next, err = merge(existing); err != nil { + return err + } + return m.SaveAccountServiceOverrideAuditedTx(ctx, tx, next, audit) + }) + if err != nil { + return nil, err + } + return next, nil +} + // DeleteAccountServiceOverrideAuditedTx falls back to // DeleteAccountServiceOverride when no expectation is registered. func (m *MockConfigStore) DeleteAccountServiceOverrideAuditedTx(ctx context.Context, tx pgx.Tx, accountID, provider, service string, audit config.AuditContext) error {