diff --git a/internal/api/handler_ri_exchange_direct_list.go b/internal/api/handler_ri_exchange_direct_list.go new file mode 100644 index 000000000..28eba6f42 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list.go @@ -0,0 +1,132 @@ +package api + +import ( + "context" + "fmt" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" +) + +func (h *Handler) listRIExchangeDirectExecutions(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + session, err := h.requirePermission(ctx, req, "execute", "ri-exchange") + if err != nil { + return nil, err + } + filter, err := parseRIExchangeDirectListQuery(req.QueryStringParameters) + if err != nil { + return nil, err + } + store, ok := h.config.(config.RIExchangeDirectListStore) + if !ok { + return nil, NewClientError(503, "RI exchange resolve storage is unavailable") + } + accounts, err := h.riExchangeDirectAccounts(ctx) + if err != nil { + return nil, err + } + allowed, err := h.visibleRIExchangeDirectPairs(ctx, session, store, accounts) + if mapped := mapRIExchangeResolveError(err); mapped != nil { + return nil, mapped + } + filter.Allowed = allowed + page, err := store.ListUnresolvedRIExchangeDirectExecutions(ctx, filter) + if mapped := mapRIExchangeResolveError(err); mapped != nil { + return nil, mapped + } + now := time.Now().UTC() + resp := riExchangeDirectListResponse{Executions: make([]riExchangeDirectListItem, 0, len(page.Rows))} + for i := range page.Rows { + resp.Executions = append(resp.Executions, newRIExchangeDirectListItem(&page.Rows[i], now)) + } + if page.Next != nil { + resp.NextCursor = encodeRIExchangeDirectCursor(page.Next) + } + return resp, nil +} + +// visibleRIExchangeDirectPairs keeps the (account, region) pairs of unresolved +// rows that the session may see: its account scope allows the account (a row +// with no registered account needs an unrestricted scope) and the grant on +// execute:ri-exchange covers the same account, provider, service and region +// the direct execute checks. The grant is evaluated by the authorization +// service, never reimplemented here, so OR'd grants, a user API key's own +// narrower permissions and region spelling behave as they do on execute. +func (h *Handler) visibleRIExchangeDirectPairs(ctx context.Context, session *Session, store config.RIExchangeDirectListStore, accounts map[string]config.CloudAccount) ([]config.RIExchangeDirectAccountRegion, error) { + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + pairs, err := store.ListUnresolvedRIExchangeDirectAccountRegions(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list unresolved direct exchange accounts: %w", err) + } + var visible []config.RIExchangeDirectAccountRegion + for _, p := range pairs { + account, inScope := riExchangeDirectPairAccount(scope, accounts, p) + if !inScope { + continue + } + granted, err := h.riExchangeDirectPairGranted(ctx, session, account, p.Region) + if err != nil { + return nil, err + } + if granted { + visible = append(visible, p) + } + } + return visible, nil +} + +// riExchangeDirectAccounts loads the registered AWS accounts by ID, so the scope +// check can also match an account's AWS account number. +func (h *Handler) riExchangeDirectAccounts(ctx context.Context) (map[string]config.CloudAccount, error) { + provider := string(common.ProviderAWS) + accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{Provider: &provider}) + if err != nil { + return nil, fmt.Errorf("failed to list cloud accounts: %w", err) + } + byID := make(map[string]config.CloudAccount, len(accounts)) + for i := range accounts { + byID[accounts[i].ID] = accounts[i] + } + return byID, nil +} + +// riExchangeDirectPairAccount returns the account the grant is checked +// against (the unattributed sentinel for a row with no registered account) and +// whether the session's account scope covers it. A row with no account needs +// an unrestricted scope. A group may list an account by its ID or by its AWS +// account number (the group write keeps an external id verbatim), so either +// form admits the row; external ids are unique per provider, so the number +// cannot name another AWS account. +func riExchangeDirectPairAccount(scope auth.AccountScope, accounts map[string]config.CloudAccount, p config.RIExchangeDirectAccountRegion) (string, bool) { + if p.CloudAccountID == nil { + return unattributedAccountConstraint, scope.AllowsAll() + } + id := *p.CloudAccountID + if scope.Allows(id, "") { + return id, true + } + external := accounts[id].ExternalID + return id, external != "" && scope.Allows(external, "") +} + +// riExchangeDirectPairGranted asks the authorization service whether the +// grant covers an AWS EC2 exchange in the account and region. A denial is +// false; any other failure is an error. +func (h *Handler) riExchangeDirectPairGranted(ctx context.Context, session *Session, account, region string) (bool, error) { + err := h.requirePermissionConstraints(ctx, session, "execute", "ri-exchange", []auth.PermissionConstraints{{ + AccountIDs: []string{account}, + Providers: []string{string(common.ProviderAWS)}, + Services: []string{string(common.ServiceEC2)}, + Regions: []string{region}, + }}) + if ce, isClient := IsClientError(err); isClient && ce.code == 403 { + return false, nil + } + return err == nil, err +} diff --git a/internal/api/handler_ri_exchange_direct_list_failclosed_test.go b/internal/api/handler_ri_exchange_direct_list_failclosed_test.go new file mode 100644 index 000000000..b6586961c --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list_failclosed_test.go @@ -0,0 +1,102 @@ +package api + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +type failingDirectListStore struct { + *directListStore + pairsErr, listErr error +} + +func (s *failingDirectListStore) ListUnresolvedRIExchangeDirectAccountRegions(ctx context.Context) ([]config.RIExchangeDirectAccountRegion, error) { + if s.pairsErr != nil { + return nil, s.pairsErr + } + return s.directListStore.ListUnresolvedRIExchangeDirectAccountRegions(ctx) +} + +func (s *failingDirectListStore) ListUnresolvedRIExchangeDirectExecutions(ctx context.Context, f config.RIExchangeDirectListFilter) (*config.RIExchangeDirectListPage, error) { + if s.listErr != nil { + return nil, s.listErr + } + return s.directListStore.ListUnresolvedRIExchangeDirectExecutions(ctx, f) +} + +func callDirectList(t *testing.T, store *failingDirectListStore, authErr error) (any, error) { + t.Helper() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, mock.Anything, "execute", "ri-exchange").Return(true, nil).Maybe() + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return([]string(nil), nil).Maybe() + if authErr != nil { + mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, "execute", "ri-exchange", mock.Anything).Return(false, authErr) + } else { + mockAuth.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, "execute", "ri-exchange", mock.Anything).Return(true, nil).Maybe() + } + h := &Handler{config: store, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}} + return h.listRIExchangeDirectExecutions(context.Background(), req) +} + +// A failure anywhere in the visibility chain is a server error with no +// partial list: nothing is shown on a guess. +func TestListRIExchangeDirectExecutions_FailsClosed(t *testing.T) { + rows := []config.RIExchangeDirectRow{ + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctB), "eu-west-1", "unknown", 2), + } + boom := errors.New("backend down") + call := callDirectList + tests := []struct { + name string + store *failingDirectListStore + auth error + }{ + {"the pair query fails", &failingDirectListStore{directListStore: dlStore(rows...), pairsErr: boom}, nil}, + {"the page query fails", &failingDirectListStore{directListStore: dlStore(rows...), listErr: boom}, nil}, + {"the constraint check fails with a non-403 error", &failingDirectListStore{directListStore: dlStore(rows...)}, boom}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + out, err := call(t, tc.store, tc.auth) + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient, "a backend failure is a 5xx, not a refusal: %v", err) + assert.Nil(t, out, "no partial list") + }) + } + t.Run("control: the same setup lists both rows", func(t *testing.T) { + out, err := call(t, &failingDirectListStore{directListStore: dlStore(rows...)}, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1, dlID2}, dlIDs(t, out)) + }) +} + +// Before migration 163 the list answers 503, whichever of its two store reads +// notices first; it is not a generic 500. +func TestListRIExchangeDirectExecutions_NotReadyIs503(t *testing.T) { + rows := []config.RIExchangeDirectRow{dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1)} + notReady := fmt.Errorf("list unresolved direct exchange rows: %w", config.ErrRIExchangeDirectResolveNotReady) + for name, store := range map[string]*failingDirectListStore{ + "the pair query": {directListStore: dlStore(rows...), pairsErr: notReady}, + "the page query": {directListStore: dlStore(rows...), listErr: notReady}, + } { + t.Run(name, func(t *testing.T) { + out, err := callDirectList(t, store, nil) + require.Error(t, err) + assert.Equal(t, 503, clientCode(t, err)) + assert.Nil(t, out) + }) + } +} diff --git a/internal/api/handler_ri_exchange_direct_list_item_test.go b/internal/api/handler_ri_exchange_direct_list_item_test.go new file mode 100644 index 000000000..c23be4485 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list_item_test.go @@ -0,0 +1,152 @@ +package api + +import ( + "encoding/json" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" +) + +// Unit tests for the pieces of GET /api/ri-exchange/direct-executions that need +// no store (#1836): what a stored row exposes, the resolvable hint, query +// validation and the cursor. + +const ( + dlAcctA = scopedInAccount + dlAcctB = scopedOutAccount + dlID1 = "00000000-0000-4000-8000-000000000001" + dlID2 = "00000000-0000-4000-8000-000000000002" + dlID3 = "00000000-0000-4000-8000-000000000003" + dlID4 = "00000000-0000-4000-8000-000000000004" +) + +var dlBase = time.Now().UTC().Truncate(time.Microsecond).Add(-time.Hour) + +func dlPtr(s string) *string { return &s } + +func dlRow(id string, account *string, region, status string, minutes int) config.RIExchangeDirectRow { + at := dlBase.Add(time.Duration(minutes) * time.Minute) + r := config.RIExchangeDirectRow{ + ID: id, Status: status, CloudAccountID: account, Region: region, + SourceIDs: []string{"ri-" + id}, Targets: json.RawMessage(`[{"offering_id":"o","count":1}]`), + MaxPaymentDue: "100", MaxPaymentCurrency: "USD", ExecutedBy: "alice@example.com", CreatedAt: at, + } + if status != "submitted" { + r.CompletedAt = &at + } + return r +} + +func keysOf[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +} + +// [fail-first] The response carries exactly the allowlisted keys: never the raw +// error, the executing user id or the API key id. +func TestRIExchangeDirectListItem_KeysAreAllowlisted(t *testing.T) { + row := dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1) + row.Error = "InvalidParameterValue for account 123456789012 from 10.1.2.3: raw provider text" + + raw, err := json.Marshal(riExchangeDirectListResponse{ + Executions: []riExchangeDirectListItem{newRIExchangeDirectListItem(&row, time.Now())}, NextCursor: ""}) + require.NoError(t, err) + + var top map[string]json.RawMessage + require.NoError(t, json.Unmarshal(raw, &top)) + assert.ElementsMatch(t, []string{"executions", "next_cursor"}, keysOf(top)) + var items []map[string]any + require.NoError(t, json.Unmarshal(top["executions"], &items)) + require.Len(t, items, 1) + assert.ElementsMatch(t, []string{"id", "status", "provider", "region", "cloud_account_id", "source_ids", "targets", "max_payment_due", + "max_payment_currency", "created_at", "executed_by", "error_summary", "resolvable", "resolvable_at"}, keysOf(items[0])) + + assert.Equal(t, purchase.SanitizeStoredError(row.Error), items[0]["error_summary"]) + for _, leak := range []string{"123456789012", "10.1.2.3", "raw provider text"} { + assert.NotContains(t, string(raw), leak) + } + assert.Equal(t, "aws", items[0]["provider"]) +} + +func TestRIExchangeDirectListItem_CloudAccountID(t *testing.T) { + attributed := dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1) + unattributed := dlRow(dlID2, nil, "eu-west-1", "unknown", 1) + + raw, err := json.Marshal(newRIExchangeDirectListItem(&attributed, time.Now())) + require.NoError(t, err) + assert.Contains(t, string(raw), `"cloud_account_id":"`+dlAcctA+`"`) + + raw, err = json.Marshal(newRIExchangeDirectListItem(&unattributed, time.Now())) + require.NoError(t, err) + assert.Contains(t, string(raw), `"cloud_account_id":null`) +} + +func TestRIExchangeDirectListItem_ResolvableHint(t *testing.T) { + now := time.Now().UTC() + at := func(r config.RIExchangeDirectRow, created time.Time) config.RIExchangeDirectRow { + r.CreatedAt = created + if r.CompletedAt != nil { + r.CompletedAt = &created + } + return r + } + sub := at(dlRow(dlID1, nil, "eu-west-1", "submitted", 0), now.Add(-5*time.Minute)) + oldSub := at(dlRow(dlID2, nil, "eu-west-1", "submitted", 0), now.Add(-config.RIExchangeDirectResolveSubmittedAfter-time.Minute)) + unk := at(dlRow(dlID3, nil, "eu-west-1", "unknown", 0), now.Add(-2*time.Minute)) + freshUnk := at(dlRow(dlID4, nil, "eu-west-1", "unknown", 0), now.Add(-10*time.Second)) + + got := func(r config.RIExchangeDirectRow) riExchangeDirectListItem { + return newRIExchangeDirectListItem(&r, now) + } + assert.False(t, got(sub).Resolvable) + assert.True(t, got(sub).ResolvableAt.Equal(sub.CreatedAt.Add(config.RIExchangeDirectResolveSubmittedAfter))) + assert.True(t, got(oldSub).Resolvable) + assert.True(t, got(unk).Resolvable) + assert.True(t, got(unk).ResolvableAt.Equal(unk.CompletedAt.Add(config.RIExchangeDirectResolveUnknownCheckAfter))) + assert.False(t, got(freshUnk).Resolvable) +} + +func TestParseRIExchangeDirectListQuery(t *testing.T) { + for name, q := range map[string]map[string]string{ + "bad status": {"status": "completed"}, + "limit zero": {"limit": "0"}, + "limit too big": {"limit": "201"}, + "limit not int": {"limit": "ten"}, + "bad region": {"region": "EU West"}, + "bad cursor": {"cursor": "!!!"}, + "cursor no id": {"cursor": "MjAyNi0wMS0wMVQwMDowMDowMFp8bm90LWEtdXVpZA"}, + "ri_id control": {"ri_id": "ri\n1"}, + "ri_id too long": {"ri_id": strings.Repeat("a", riExchangeDirectRIIDMax+1)}, + } { + _, err := parseRIExchangeDirectListQuery(q) + assert.Equal(t, 400, clientCode(t, err), name) + } + + f, err := parseRIExchangeDirectListQuery(map[string]string{"status": "unknown", "region": "eu-west-1", "ri_id": "RI-X", "limit": "7"}) + require.NoError(t, err) + assert.Equal(t, []string{"unknown"}, f.Statuses) + assert.Equal(t, "eu-west-1", f.Region) + assert.Equal(t, "RI-X", f.RIID) + assert.Equal(t, 7, f.Limit) + + f, err = parseRIExchangeDirectListQuery(nil) + require.NoError(t, err) + assert.Equal(t, config.RIExchangeDirectListDefaultLimit, f.Limit) +} + +func TestRIExchangeDirectCursor_RoundTripKeepsMicroseconds(t *testing.T) { + at := time.Date(2026, 11, 7, 10, 20, 30, 123456000, time.UTC) + got, err := decodeRIExchangeDirectCursor(encodeRIExchangeDirectCursor(&config.RIExchangeDirectCursor{CreatedAt: at, ID: dlID2})) + require.NoError(t, err) + assert.True(t, got.CreatedAt.Equal(at)) + assert.Equal(t, dlID2, got.ID) +} diff --git a/internal/api/handler_ri_exchange_direct_list_route_test.go b/internal/api/handler_ri_exchange_direct_list_route_test.go new file mode 100644 index 000000000..1e3c1dd24 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list_route_test.go @@ -0,0 +1,20 @@ +package api + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// The list route is session-authenticated, never public, and read-only. +func TestListRIExchangeDirectExecutions_RouteIsAuthUserGET(t *testing.T) { + r := NewRouter(&Handler{}) + for _, route := range r.routes { + if route.ExactPath == "/api/ri-exchange/direct-executions" { + assert.Equal(t, "GET", route.Method) + assert.Equal(t, AuthUser, route.Auth) + return + } + } + t.Fatal("GET /api/ri-exchange/direct-executions is not registered") +} diff --git a/internal/api/handler_ri_exchange_direct_list_test.go b/internal/api/handler_ri_exchange_direct_list_test.go new file mode 100644 index 000000000..5c7e62b47 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_list_test.go @@ -0,0 +1,282 @@ +package api + +import ( + "context" + "encoding/json" + "sort" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Handler tests for GET /api/ri-exchange/direct-executions (#1836). The store +// is an in-memory fixture that applies the allowed set and the keyset the way +// the SQL does (covered against real PostgreSQL in internal/config); these +// tests pin which (account, region) pairs the handler hands it. + +type directListStore struct { + *MockConfigStore + rows []config.RIExchangeDirectRow + lastFilter config.RIExchangeDirectListFilter + listCalls int + pairCalls int +} + +func (s *directListStore) ListUnresolvedRIExchangeDirectAccountRegions(context.Context) ([]config.RIExchangeDirectAccountRegion, error) { + s.pairCalls++ + seen := map[string]bool{} + var out []config.RIExchangeDirectAccountRegion + for _, r := range s.rows { + k := pairKey(r.CloudAccountID, r.Region) + if !seen[k] { + seen[k] = true + out = append(out, config.RIExchangeDirectAccountRegion{CloudAccountID: r.CloudAccountID, Region: r.Region}) + } + } + return out, nil +} + +func pairKey(account *string, region string) string { + if account == nil { + return "|" + region + } + return *account + "|" + region +} + +func (s *directListStore) ListUnresolvedRIExchangeDirectExecutions(_ context.Context, f config.RIExchangeDirectListFilter) (*config.RIExchangeDirectListPage, error) { + s.listCalls++ + s.lastFilter = f + allowed := map[string]bool{} + for _, p := range f.Allowed { + allowed[pairKey(p.CloudAccountID, p.Region)] = true + } + var rows []config.RIExchangeDirectRow + for _, r := range s.rows { + if !allowed[pairKey(r.CloudAccountID, r.Region)] { + continue + } + if f.After != nil && !r.CreatedAt.After(f.After.CreatedAt) && (!r.CreatedAt.Equal(f.After.CreatedAt) || r.ID <= f.After.ID) { + continue + } + rows = append(rows, r) + } + sort.Slice(rows, func(i, j int) bool { + if !rows[i].CreatedAt.Equal(rows[j].CreatedAt) { + return rows[i].CreatedAt.Before(rows[j].CreatedAt) + } + return rows[i].ID < rows[j].ID + }) + page := &config.RIExchangeDirectListPage{} + if f.Limit > 0 && len(rows) > f.Limit { + rows = rows[:f.Limit] + page.Next = &config.RIExchangeDirectCursor{CreatedAt: rows[f.Limit-1].CreatedAt, ID: rows[f.Limit-1].ID} + } + page.Rows = rows + return page, nil +} + +func (s *directListStore) GetRIExchangeDirectExecution(context.Context, string) (*config.RIExchangeDirectRow, error) { + return nil, config.ErrRIExchangeResolveNotFound +} + +func dlStore(rows ...config.RIExchangeDirectRow) *directListStore { + s := &directListStore{MockConfigStore: new(MockConfigStore), rows: rows} + s.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { return nil, nil } + return s +} + +// dlCall runs the handler as a session user holding perms, scoped to accounts +// (nil means unrestricted). +func dlCall(t *testing.T, store *directListStore, perms []auth.Permission, accounts []string, query map[string]string) (any, error) { + t.Helper() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() + mockAuth.grantPermissionsScoped(perms, accounts) + h := &Handler{config: store, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}, QueryStringParameters: query} + return h.listRIExchangeDirectExecutions(context.Background(), req) +} + +func dlIDs(t *testing.T, out any) []string { + t.Helper() + resp, ok := out.(riExchangeDirectListResponse) + require.True(t, ok) + ids := make([]string, len(resp.Executions)) + for i, e := range resp.Executions { + ids[i] = e.ID + } + return ids +} + +func regionGrant(regions ...string) []auth.Permission { + perms := make([]auth.Permission, 0, len(regions)) + for _, r := range regions { + perms = append(perms, auth.Permission{Action: auth.ActionExecute, Resource: "ri-exchange", + Constraints: &auth.PermissionConstraints{Regions: []string{r}}}) + } + return perms +} + +func TestListRIExchangeDirectExecutions_NeedsExecutePermission(t *testing.T) { + store := dlStore(dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1)) + _, err := dlCall(t, store, []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourcePurchases}}, nil, nil) + assert.Equal(t, 403, clientCode(t, err)) + assert.Zero(t, store.pairCalls+store.listCalls, "a refused caller must not reach the store") +} + +func TestListRIExchangeDirectExecutions_AccountScope(t *testing.T) { + store := dlStore( + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctB), "eu-west-1", "unknown", 2), + dlRow(dlID3, nil, "eu-west-1", "submitted", 3), + ) + t.Run("scoped session sees its account only, never an unattributed row", func(t *testing.T) { + out, err := dlCall(t, store, riExecute, []string{dlAcctA}, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1}, dlIDs(t, out)) + }) + t.Run("unrestricted session sees every account and the unattributed row", func(t *testing.T) { + out, err := dlCall(t, store, riExecute, nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1, dlID2, dlID3}, dlIDs(t, out)) + }) + t.Run("nothing visible is an empty list, not an error", func(t *testing.T) { + out, err := dlCall(t, store, riExecute, []string{"33333333-3333-4333-8333-333333333333"}, nil) + require.NoError(t, err) + raw, _ := json.Marshal(out) + assert.JSONEq(t, `{"executions":[],"next_cursor":""}`, string(raw)) + }) +} + +func TestListRIExchangeDirectExecutions_GrantRegions(t *testing.T) { + store := dlStore( + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctA), "us-east-1", "unknown", 2), + dlRow(dlID3, dlPtr(dlAcctA), "ap-south-1", "unknown", 3), + ) + t.Run("a grant restricted to one region", func(t *testing.T) { + out, err := dlCall(t, store, regionGrant("us-east-1"), nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID2}, dlIDs(t, out)) + }) + t.Run("two grants are OR'd", func(t *testing.T) { + out, err := dlCall(t, store, regionGrant("us-east-1", "eu-west-1"), nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1, dlID2}, dlIDs(t, out)) + }) + t.Run("region spelling is not case sensitive", func(t *testing.T) { + out, err := dlCall(t, store, regionGrant("EU-WEST-1"), nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID1}, dlIDs(t, out)) + }) + t.Run("an account-constrained grant hides other accounts' rows", func(t *testing.T) { + other := dlStore(dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), dlRow(dlID2, dlPtr(dlAcctB), "eu-west-1", "unknown", 2)) + perms := []auth.Permission{{Action: auth.ActionExecute, Resource: "ri-exchange", + Constraints: &auth.PermissionConstraints{AccountIDs: []string{dlAcctB}}}} + out, err := dlCall(t, other, perms, nil, nil) + require.NoError(t, err) + assert.Equal(t, []string{dlID2}, dlIDs(t, out)) + }) +} + +// A user API key is judged on its own permissions, not its owner's. +func TestListRIExchangeDirectExecutions_UserAPIKeyIsNarrowerThanItsOwner(t *testing.T) { + store := dlStore( + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctA), "us-east-1", "unknown", 2), + ) + mockAuth := new(MockAuthService) + mockAuth.On("HasAPIKeyPermissionAPI", mock.Anything, "user-key", "execute", "ri-exchange").Return("u2", "key-1", true, nil) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, "u2").Return([]string(nil), nil) + inRegion := func(region string) any { + return mock.MatchedBy(func(sets []auth.PermissionConstraints) bool { + return len(sets) == 1 && len(sets[0].Regions) == 1 && sets[0].Regions[0] == region + }) + } + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, "key-1", "u2", "execute", "ri-exchange", inRegion("us-east-1")).Return(true, nil) + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, "key-1", "u2", "execute", "ri-exchange", inRegion("eu-west-1")).Return(false, nil) + h := &Handler{config: store, auth: mockAuth} + ctx := contextWithPrincipal(context.Background(), &Principal{Kind: PrincipalUserAPIKey, UserID: "u2", APIKeyID: "key-1"}) + + out, err := h.listRIExchangeDirectExecutions(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "user-key"}}) + + require.NoError(t, err) + assert.Equal(t, []string{dlID2}, dlIDs(t, out)) + mockAuth.AssertNotCalled(t, "HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +// Rows of a denied region sit between visible ones: every returned row and +// every cursor belongs to a visible pair, and paging is complete. +func TestListRIExchangeDirectExecutions_CursorNeverPointsAtADeniedRow(t *testing.T) { + store := dlStore( + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctA), "us-east-1", "unknown", 2), + dlRow(dlID3, dlPtr(dlAcctA), "eu-west-1", "unknown", 3), + dlRow(dlID4, dlPtr(dlAcctA), "us-east-1", "unknown", 4), + ) + denied := map[string]bool{dlID2: true, dlID4: true} + var got []string + query := map[string]string{"limit": "1"} + for page := 0; page < 5; page++ { + out, err := dlCall(t, store, regionGrant("eu-west-1"), nil, query) + require.NoError(t, err) + got = append(got, dlIDs(t, out)...) + next := out.(riExchangeDirectListResponse).NextCursor + if next == "" { + break + } + cur, err := decodeRIExchangeDirectCursor(next) + require.NoError(t, err) + assert.False(t, denied[cur.ID], "the cursor points at a row the caller cannot see") + query = map[string]string{"limit": "1", "cursor": next} + } + assert.Equal(t, []string{dlID1, dlID3}, got) +} + +func TestListRIExchangeDirectExecutions_NoStore503(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() + mockAuth.grantPermissions(riExecute) + h := &Handler{config: new(MockConfigStore), auth: mockAuth} + _, err := h.listRIExchangeDirectExecutions(context.Background(), + &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}}) + assert.Equal(t, 503, clientCode(t, err)) +} + +// A group may list an account by its AWS account number. The list must admit +// the row of the account that number names, not hide it because only the +// internal id was compared. +func TestListRIExchangeDirectExecutions_AllowedAccountsMayNameTheAWSAccountNumber(t *testing.T) { + rows := []config.RIExchangeDirectRow{ + dlRow(dlID1, dlPtr(dlAcctA), "eu-west-1", "unknown", 1), + dlRow(dlID2, dlPtr(dlAcctB), "eu-west-1", "unknown", 2), + } + accounts := []config.CloudAccount{ + {ID: dlAcctA, Provider: "aws", ExternalID: "111122223333"}, + {ID: dlAcctB, Provider: "aws", ExternalID: "999999999999"}, + } + for name, tc := range map[string]struct { + allowed []string + want []string + }{ + "the account number": {[]string{"111122223333"}, []string{dlID1}}, + "the internal id": {[]string{dlAcctA}, []string{dlID1}}, + "the other account's number": {[]string{"999999999999"}, []string{dlID2}}, + "a number that is nobody's": {[]string{"000000000000"}, nil}, + "both, one by number and one by id": {[]string{"111122223333", dlAcctB}, []string{dlID1, dlID2}}, + } { + t.Run(name, func(t *testing.T) { + store := dlStore(rows...) + store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { return accounts, nil } + out, err := dlCall(t, store, riExecute, tc.allowed, nil) + require.NoError(t, err) + assert.ElementsMatch(t, tc.want, dlIDs(t, out)) + }) + } +} diff --git a/internal/api/handler_ri_exchange_direct_resolve.go b/internal/api/handler_ri_exchange_direct_resolve.go new file mode 100644 index 000000000..67311df6a --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve.go @@ -0,0 +1,234 @@ +package api + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "strconv" + "strings" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" + "github.com/aws/aws-lambda-go/events" +) + +// Operator list of AWS direct RI-exchange rows that still hold their source +// RIs (#1836). An accept whose outcome was lost leaves the row submitted or +// unknown, and the in-flight guard then refuses every later exchange of those +// RIs until a person who checked AWS resolves it. + +const riExchangeDirectRIIDMax = 200 + +type riExchangeDirectListItem struct { + ID string `json:"id"` + Status string `json:"status"` + Provider string `json:"provider"` + Region string `json:"region"` + CloudAccountID *string `json:"cloud_account_id"` + SourceIDs []string `json:"source_ids"` + Targets json.RawMessage `json:"targets"` + MaxPaymentDue string `json:"max_payment_due"` + MaxPaymentCurrency string `json:"max_payment_currency"` + CreatedAt time.Time `json:"created_at"` + ExecutedBy string `json:"executed_by"` + ErrorSummary string `json:"error_summary"` + Resolvable bool `json:"resolvable"` + ResolvableAt time.Time `json:"resolvable_at"` +} + +type riExchangeDirectListResponse struct { + Executions []riExchangeDirectListItem `json:"executions"` + NextCursor string `json:"next_cursor"` +} + +func parseRIExchangeDirectListQuery(q map[string]string) (config.RIExchangeDirectListFilter, error) { + var f config.RIExchangeDirectListFilter + if s := q["status"]; s != "" { + if s != config.RIExchangeDirectStatusSubmitted && s != config.RIExchangeDirectStatusUnknown { + return f, NewClientError(400, `status must be "submitted" or "unknown"`) + } + f.Statuses = []string{s} + } + if r := q["region"]; r != "" { + if !riExchangeResolveRegionRe.MatchString(r) { + return f, NewClientError(400, "region is not a valid AWS region") + } + f.Region = r + } + if id := q["ri_id"]; id != "" { + if len(id) > riExchangeDirectRIIDMax || hasControlRune(id) { + return f, NewClientError(400, "ri_id is not a valid reserved instance id") + } + f.RIID = id + } + return parseRIExchangeDirectListPaging(q, f) +} + +// parseRIExchangeDirectListPaging reads the limit and cursor into f. +func parseRIExchangeDirectListPaging(q map[string]string, f config.RIExchangeDirectListFilter) (config.RIExchangeDirectListFilter, error) { + f.Limit = config.RIExchangeDirectListDefaultLimit + if l := q["limit"]; l != "" { + n, err := strconv.Atoi(l) + if err != nil || n < 1 || n > config.RIExchangeDirectListMaxLimit { + return f, NewClientError(400, fmt.Sprintf("limit must be 1 to %d", config.RIExchangeDirectListMaxLimit)) + } + f.Limit = n + } + if c := q["cursor"]; c != "" { + cur, err := decodeRIExchangeDirectCursor(c) + if err != nil { + return f, err + } + f.After = cur + } + return f, nil +} + +// The cursor is opaque to callers: the created_at and id of the last row of +// the previous page. It is a position, not a credential, because the store +// only ever returns rows the caller may see. +func encodeRIExchangeDirectCursor(c *config.RIExchangeDirectCursor) string { + return base64.RawURLEncoding.EncodeToString([]byte(c.CreatedAt.UTC().Format(time.RFC3339Nano) + "|" + c.ID)) +} + +func decodeRIExchangeDirectCursor(s string) (*config.RIExchangeDirectCursor, error) { + raw, err := base64.RawURLEncoding.DecodeString(s) + bad := NewClientError(400, "cursor is not valid") + if err != nil { + return nil, bad + } + at, id, found := strings.Cut(string(raw), "|") + if !found || validateUUID(id) != nil { + return nil, bad + } + t, err := time.Parse(time.RFC3339Nano, at) + if err != nil { + return nil, bad + } + return &config.RIExchangeDirectCursor{CreatedAt: t, ID: id}, nil +} + +// newRIExchangeDirectListItem is the one place a stored row becomes a response +// item, so the exposed fields are exactly these: the raw stored error, the +// executing user id and the API key id never leave the server. +func newRIExchangeDirectListItem(r *config.RIExchangeDirectRow, now time.Time) riExchangeDirectListItem { + at := resolvableRIExchangeDirectAt(r) + return riExchangeDirectListItem{ + ID: r.ID, Status: r.Status, Provider: string(common.ProviderAWS), Region: r.Region, + CloudAccountID: r.CloudAccountID, SourceIDs: r.SourceIDs, Targets: r.Targets, + MaxPaymentDue: r.MaxPaymentDue, MaxPaymentCurrency: r.MaxPaymentCurrency, + CreatedAt: r.CreatedAt.UTC(), ExecutedBy: r.ExecutedBy, + ErrorSummary: purchase.SanitizeStoredError(r.Error), + Resolvable: !now.Before(at), ResolvableAt: at, + } +} + +// resolvableRIExchangeDirectAt is when the store's rules first admit a +// resolve: a submitted row once its settle age has passed, an unknown row once +// the operator's AWS check can postdate its completion by the required margin. +func resolvableRIExchangeDirectAt(r *config.RIExchangeDirectRow) time.Time { + if r.Status == config.RIExchangeDirectStatusSubmitted || r.CompletedAt == nil { + return r.CreatedAt.Add(config.RIExchangeDirectResolveSubmittedAfter).UTC() + } + return r.CompletedAt.Add(config.RIExchangeDirectResolveUnknownCheckAfter).UTC() +} + +type riExchangeDirectResolveResponse struct { + ID string `json:"id"` + Status string `json:"status"` + ResolvedStatus string `json:"resolved_status"` + ResolvedAt time.Time `json:"resolved_at"` + Note string `json:"note"` + Replayed bool `json:"replayed"` +} + +// resolveRIExchangeDirectExecution records an operator's outcome for a stuck +// AWS direct row. Authorization runs against the ROW's account, region and +// payment cap, not the deployment's, because the row may belong to another +// account; an unknown, out-of-scope or non-AWS row is the same 404. +func (h *Handler) resolveRIExchangeDirectExecution(ctx context.Context, req *events.LambdaFunctionURLRequest, id string) (any, error) { + session, err := h.requirePermission(ctx, req, "execute", "ri-exchange") + if err != nil { + return nil, err + } + // The resolver is stamped on the row, so it needs an identified person. + if session.UserID == apiKeyAdminUserID { + return nil, NewClientError(403, "resolving an RI exchange needs a signed-in user or a user API key") + } + if idErr := validateUUID(id); idErr != nil { + return nil, idErr + } + store, ok := h.config.(config.RIExchangeDirectResolveStore) + if !ok { + return nil, NewClientError(503, "RI exchange resolve storage is unavailable") + } + row, err := store.GetRIExchangeDirectExecution(ctx, id) + if mapped := mapRIExchangeResolveError(err); mapped != nil { + return nil, mapped + } + if authErr := h.authorizeRIExchangeDirectRow(ctx, session, row); authErr != nil { + return nil, authErr + } + resolve, err := buildRIExchangeDirectResolve(session, row, req.Body) + if err != nil { + return nil, err + } + res, err := store.ResolveRIExchangeDirectExecution(ctx, resolve) + if mapped := mapRIExchangeResolveError(err); mapped != nil { + return nil, mapped + } + if !res.Replayed { + logging.Warnf("RI exchange direct row resolved by operator: id=%s actor=%q via=%s outcome=%s", + id, session.UserID, credentialKind(ctx, session), res.ResolvedStatus) + } + return riExchangeDirectResolveResponse{ID: res.ID, Status: res.Status, ResolvedStatus: res.ResolvedStatus, + ResolvedAt: res.ResolvedAt.UTC(), Note: res.Note, Replayed: res.Replayed}, nil +} + +// buildRIExchangeDirectResolve validates the body against the row and builds +// the store request. The note must fit the store's byte limit, so an overlong +// reason is a clear 400 here rather than the store's. +func buildRIExchangeDirectResolve(session *Session, row *config.RIExchangeDirectRow, raw string) (config.RIExchangeDirectResolve, error) { + body, checkedAt, err := decodeRIExchangeDirectResolve(raw, row.Region) + if err != nil { + return config.RIExchangeDirectResolve{}, err + } + note := body.note(checkedAt) + if len(note) > config.RIExchangeDirectResolveNoteMax { + return config.RIExchangeDirectResolve{}, NewClientError(400, "reason too long once encoded; shorten it") + } + return config.RIExchangeDirectResolve{ + ID: row.ID, Outcome: body.Outcome, ExpectedStatus: body.ExpectedStatus, Note: note, + CheckedAt: checkedAt, ResolvedByUserID: resolveCreatorUserID(session), ResolvedBy: fourEyesActorIdentity(session), + }, nil +} + +// authorizeRIExchangeDirectRow applies the session's account scope and the +// grant on execute:ri-exchange to the row's own account, region and +// max_payment_due (the most the original request could have committed). A +// row outside the account scope reads as missing. +func (h *Handler) authorizeRIExchangeDirectRow(ctx context.Context, session *Session, row *config.RIExchangeDirectRow) error { + scope, err := h.getAccountScope(ctx, session) + if err != nil { + return fmt.Errorf("failed to get allowed accounts: %w", err) + } + accounts, err := h.riExchangeDirectAccounts(ctx) + if err != nil { + return err + } + account, inScope := riExchangeDirectPairAccount(scope, accounts, config.RIExchangeDirectAccountRegion{CloudAccountID: row.CloudAccountID, Region: row.Region}) + if !inScope { + return errNotFound + } + return h.requireRIExchangeConstraintsFor(ctx, session, account, row.Region, func() (float64, error) { + due, ok := parseRIExchangeAmount(row.MaxPaymentDue) + if !ok { + return 0, fmt.Errorf("unreadable payment cap on direct exchange %s", row.ID) + } + return due, nil + }) +} diff --git a/internal/api/handler_ri_exchange_direct_resolve_authz_test.go b/internal/api/handler_ri_exchange_direct_resolve_authz_test.go new file mode 100644 index 000000000..9502156cc --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_authz_test.go @@ -0,0 +1,143 @@ +package api + +import ( + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveRIExchangeDirect_Authorization(t *testing.T) { + unattributed := dlRow(dlID2, nil, drRegion, "unknown", 3) + tests := []struct { + name string + caller drCaller + id string + want int // 0 = allowed + }{ + {"execute:ri-exchange, unrestricted", drCaller{perms: riExecute}, dlID1, 0}, + {"view-only user", drCaller{perms: []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourcePurchases}}}, dlID1, 403}, + {"admin API key has no identity to stamp", drCaller{adminKey: true}, dlID1, 403}, + {"row account inside the session's accounts", drCaller{perms: riExecute, accounts: []string{dlAcctA}}, dlID1, 0}, + {"row account outside the session's accounts reads as missing", drCaller{perms: riExecute, accounts: []string{dlAcctB}}, dlID1, 404}, + {"unattributed row needs an unrestricted session", drCaller{perms: riExecute, accounts: []string{dlAcctA}}, dlID2, 404}, + {"unattributed row, unrestricted session", drCaller{perms: riExecute}, dlID2, 0}, + {"unknown id", drCaller{perms: riExecute}, dlID3, 404}, + {"id that is not a uuid", drCaller{perms: riExecute}, "not-a-uuid", 400}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store := drStore(dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1), unattributed) + _, err := tc.caller.call(store, tc.id, drFailedBody) + if tc.want == 0 { + require.NoError(t, err) + assert.Equal(t, 1, store.called) + return + } + assert.Equal(t, 0, store.called, "a refused caller must not reach the store") + if tc.want == 404 { + require.ErrorIs(t, err, errNotFound) + return + } + assert.Equal(t, tc.want, clientCode(t, err)) + }) + } +} + +// The grant is judged against the ROW's account, region and payment cap. The +// deployment's own account is a different one in every case. +func TestResolveRIExchangeDirect_ConstraintsUseTheRowsAccountAndRegion(t *testing.T) { + grant := func(c auth.PermissionConstraints) []auth.Permission { + return []auth.Permission{{Action: auth.ActionExecute, Resource: "ri-exchange", Constraints: &c}} + } + unattributed := dlRow(dlID2, nil, drRegion, "unknown", 3) + tests := []struct { + name string + perms []auth.Permission + id string + want int // 0 = allowed + }{ + {"grant for the row's region", grant(auth.PermissionConstraints{Regions: []string{drRegion}}), dlID1, 0}, + {"grant for another region", grant(auth.PermissionConstraints{Regions: []string{"us-east-1"}}), dlID1, 403}, + {"grant for the row's account", grant(auth.PermissionConstraints{AccountIDs: []string{dlAcctA}}), dlID1, 0}, + {"grant for another account", grant(auth.PermissionConstraints{AccountIDs: []string{dlAcctB}}), dlID1, 403}, + {"grant for the deployment's account only", grant(auth.PermissionConstraints{AccountIDs: []string{"33333333-3333-4333-8333-333333333333"}}), dlID1, 403}, + {"account-constrained grant never covers an unattributed row", grant(auth.PermissionConstraints{AccountIDs: []string{dlAcctA}}), dlID2, 403}, + {"cap below the row's max_payment_due (100)", riCapped(99.99), dlID1, 403}, + {"cap equal to it", riCapped(100), dlID1, 0}, + {"cap above it", riCapped(5000), dlID1, 0}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store := drStore(dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1), unattributed) + _, err := drCaller{perms: tc.perms}.call(store, tc.id, drFailedBody) + if tc.want == 0 { + require.NoError(t, err) + assert.Equal(t, 1, store.called) + return + } + assert.Equal(t, tc.want, clientCode(t, err)) + assert.Zero(t, store.called, "a refused caller must not reach the store") + }) + } +} + +func TestResolveRIExchangeDirect_CappedGrantRefusesUnreadableAmount(t *testing.T) { + for _, due := range []string{"", "abc", "-1", "NaN", "+Inf"} { + t.Run(due, func(t *testing.T) { + row := dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1) + row.MaxPaymentDue = due + store := drStore(row) + _, err := drCaller{perms: riCapped(5000)}.call(store, dlID1, drFailedBody) + require.Error(t, err) + assert.Zero(t, store.called) + }) + } +} + +// An uncapped grant never reads the amount, so an unreadable one passes. +func TestResolveRIExchangeDirect_UncappedGrantIgnoresTheAmount(t *testing.T) { + row := dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1) + row.MaxPaymentDue = "abc" + store := drStore(row) + _, err := drCaller{perms: riExecute}.call(store, dlID1, drFailedBody) + require.NoError(t, err) + assert.Equal(t, 1, store.called) +} + +// No refusal or answer for one row may carry another row's id, and an +// out-of-scope row answers exactly like a missing one. +func TestResolveRIExchangeDirect_ResponsesNameOnlyTheRequestedRow(t *testing.T) { + var seen []string + collect := func(out any, err error) { + if err != nil { + seen = append(seen, err.Error()) + return + } + seen = append(seen, fmt.Sprintf("%+v", out)) + } + for _, err := range []error{ + nil, + fmt.Errorf("%w: the exchange is already completed", config.ErrRIExchangeResolveConflict), + fmt.Errorf("%w: checked_at is in the future", config.ErrRIExchangeResolveInvalid), + config.ErrRIExchangeResolveNotFound, + } { + store := drStore() + store.err = err + collect(drCaller{perms: riExecute}.call(store, dlID1, drFailedBody)) + } + collect(drCaller{perms: riExecute, accounts: []string{dlAcctB}}.call(drStore(), dlID1, drFailedBody)) + collect(drCaller{perms: riExecute}.call(drStore(), dlID3, drFailedBody)) + collect(drCaller{perms: riCapped(1)}.call(drStore(), dlID1, drFailedBody)) + collect(drCaller{perms: riExecute}.call(drStore(), dlID1, `{"outcome":"nope"}`)) + for _, s := range seen { + assert.NotContains(t, s, drOtherID) + } + + _, outErr := drCaller{perms: riExecute, accounts: []string{dlAcctB}}.call(drStore(), dlID1, drFailedBody) + _, missErr := drCaller{perms: riExecute, accounts: []string{dlAcctB}}.call(drStore(), dlID3, drFailedBody) + assert.Equal(t, missErr, outErr, "out of scope must be indistinguishable from missing") +} diff --git a/internal/api/handler_ri_exchange_direct_resolve_body_test.go b/internal/api/handler_ri_exchange_direct_resolve_body_test.go new file mode 100644 index 000000000..62f75f1f3 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_body_test.go @@ -0,0 +1,83 @@ +package api + +import ( + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Body validation for the direct-row resolve. Every case is a 400 that never +// reaches the store. +func TestResolveRIExchangeDirect_BodyValidation(t *testing.T) { + check := func(region, at string) string { + return fmt.Sprintf(`"cloud_check":{"checked_region":%q,"checked_at":%q}`, region, at) + } + good := check(drRegion, "2026-10-19T08:00:00Z") + reason := `"reason":"source RIs still active in the EC2 console"` + head := `{"outcome":"failed","expected_status":"unknown",` + reason + `,"acknowledge_unknown_outcome":true,` + for name, body := range map[string]string{ + "empty": ``, + "unknown field": head + good + `,"exchange_id":"x"}`, + "two documents": drFailedBody + drFailedBody, + "oversized body": `{"outcome":"completed","reason":"` + strings.Repeat("x", 5000) + `"}`, + "bad outcome": `{"outcome":"canceled","expected_status":"unknown",` + reason + `,` + good + `}`, + "missing outcome": `{"expected_status":"unknown",` + reason + `,` + good + `}`, + "missing expected status": `{"outcome":"completed",` + reason + `,` + good + `}`, + "bad expected status": `{"outcome":"completed","expected_status":"completed",` + reason + `,` + good + `}`, + "short reason (9 runes)": `{"outcome":"completed","expected_status":"unknown","reason":"123456789",` + good + `}`, + "whitespace reason": `{"outcome":"completed","expected_status":"unknown","reason":" ",` + good + `}`, + "long reason (301 runes)": `{"outcome":"completed","expected_status":"unknown","reason":"` + strings.Repeat("é", 301) + `",` + good + `}`, + "300 emoji overflow the note's byte limit": `{"outcome":"completed","expected_status":"unknown","reason":"` + strings.Repeat("😀", 300) + `",` + good + `}`, + "control char in reason": `{"outcome":"completed","expected_status":"unknown","reason":"source RIs\nstill active",` + good + `}`, + "missing cloud check": `{"outcome":"completed","expected_status":"unknown",` + reason + `}`, + "cloud check null": head + `"cloud_check":null}`, + "failed without ack": `{"outcome":"failed","expected_status":"unknown",` + reason + `,` + good + `}`, + "failed with ack false": `{"outcome":"failed","expected_status":"unknown",` + reason + `,"acknowledge_unknown_outcome":false,` + good + `}`, + "wrong region": head + check("us-east-1", "2026-10-19T08:00:00Z") + `}`, + "malformed region": head + check("EU WEST", "2026-10-19T08:00:00Z") + `}`, + "empty region": head + check("", "2026-10-19T08:00:00Z") + `}`, + "bad checked_at": head + check(drRegion, "yesterday") + `}`, + "missing checked_at": head + `"cloud_check":{"checked_region":"eu-west-1"}}`, + "account in the cloud check": head + `"cloud_check":{"checked_region":"eu-west-1","checked_account_id":"123","checked_at":"2026-10-19T08:00:00Z"}}`, + } { + t.Run(name, func(t *testing.T) { + store := drStore() + _, err := drCaller{perms: riExecute}.call(store, dlID1, body) + assert.Equal(t, 400, clientCode(t, err)) + assert.Zero(t, store.called, "an invalid body must not reach the store") + }) + } +} + +// Reasons at the rune boundaries are accepted, and the stored note quotes them. +func TestResolveRIExchangeDirect_ReasonBoundaries(t *testing.T) { + for name, reason := range map[string]string{ + "10 runes": strings.Repeat("é", 10), + "300 runes": strings.Repeat("é", 300), + "trimmed to 10 runes": " " + strings.Repeat("a", 10) + " ", + "a quote is escaped once": `say "ok" to AWS`, + } { + t.Run(name, func(t *testing.T) { + store := drStore() + body := fmt.Sprintf(`{"outcome":"completed","expected_status":"unknown","reason":%q,"cloud_check":{"checked_region":%q,"checked_at":"2026-10-19T08:00:00Z"}}`, reason, drRegion) + _, err := drCaller{perms: riExecute}.call(store, dlID1, body) + require.NoError(t, err) + assert.Contains(t, store.got.Note, fmt.Sprintf("reason=%q", strings.TrimSpace(reason))) + }) + } +} + +// completed does not need the acknowledgement; failed with it passes, and the +// note records it. +func TestResolveRIExchangeDirect_AckOnlyForFailed(t *testing.T) { + store := drStore() + _, err := drCaller{perms: riExecute}.call(store, dlID1, drBody("completed", "")) + require.NoError(t, err) + assert.NotContains(t, store.got.Note, "ack=true") + assert.Equal(t, config.RIExchangeDirectStatusCompleted, store.got.Outcome) +} diff --git a/internal/api/handler_ri_exchange_direct_resolve_integration_test.go b/internal/api/handler_ri_exchange_direct_resolve_integration_test.go new file mode 100644 index 000000000..aa3eb866a --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_integration_test.go @@ -0,0 +1,124 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "fmt" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// One AWS RI keeps the second execute's idempotency key apart from the first +// execute's, so the refusal and the later success come from the in-flight +// guard alone. +const directResolveSecondBody = `{ + "ri_ids": ["ri-0a1b2c3d"], + "targets": [{"offering_id": "` + auditOfferingA + `", "count": 1}], + "max_payment_due_usd": "500.00", + "region": "eu-central-1" +}` + +func directResolveHistoryRecord(ri string) *config.RIExchangeRecord { + return &config.RIExchangeRecord{AccountID: guardAcct, Region: "eu-central-1", SourceRIIDs: []string{ri}, SourceInstanceType: "m5.large", + SourceCount: 1, TargetOfferingID: "o", TargetInstanceType: "m6i.large", TargetCount: 1, PaymentDue: "10.00", Status: "processing", Mode: "auto"} +} + +func listDirectRows(t *testing.T, h *Handler) riExchangeDirectListResponse { + t.Helper() + res, err := h.listRIExchangeDirectExecutions(auditListRequest()) + require.NoError(t, err) + // Round-trip through JSON, as a client sees it. + raw, err := json.Marshal(res) + require.NoError(t, err) + var out riExchangeDirectListResponse + require.NoError(t, json.Unmarshal(raw, &out)) + return out +} + +func auditListRequest() (context.Context, *events.LambdaFunctionURLRequest) { + return context.Background(), &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer tok"}} +} + +// #1836 end to end against a real database: a lost accept leaves an unknown +// row, the guard refuses the same RI, the operator lists and resolves the row +// through the HTTP handlers, and every path then proceeds. +func TestDirectExchangeResolve_EndToEnd(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()) + store := config.NewPostgresStore(container.DB) + lostAccept := true + h, calls := directGuardHandler(t, store, func(context.Context, exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { + if lostAccept { + return "", nil, common.ErrOutcomeUnknown + } + return "riex-1", &exchange.ExchangeQuoteSummary{PaymentDueUSDStr: "40.250000", PaymentDueRaw: "40.25", CurrencyCode: "USD"}, nil + }) + + // 1. The accept outcome is lost. + _, err := h.executeExchange(auditRequest(ctx, awsAuditBody)) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + require.Equal(t, 502, ce.code) + require.Equal(t, 1, *calls) + + // 2. The same RI is refused, with no further AWS call. + _, err = h.executeExchange(auditRequest(ctx, directResolveSecondBody)) + ce, ok = IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 409, ce.code) + assert.Equal(t, 1, *calls, "a refused execute must not call AWS") + + // 3. The list shows the row, with no raw error. + rows := listDirectRows(t, h).Executions + require.Len(t, rows, 1) + row := rows[0] + assert.Equal(t, config.RIExchangeDirectStatusUnknown, row.Status) + assert.Equal(t, []string{"ri-0a1b2c3d", "ri-4e5f6a7b"}, row.SourceIDs) + + // 4. Resolve through the handler. The row's times are set relative to the + // database clock, and the operator's check is one minute and a bit after + // completion, read back from the database so no Go clock is involved. + _, err = container.DB.Exec(ctx, `UPDATE ri_exchange_direct_executions + SET created_at = now() - interval '2 hours', completed_at = now() - interval '1 hour' WHERE id = $1`, row.ID) + require.NoError(t, err) + var checkedAt time.Time + require.NoError(t, container.DB.QueryRow(ctx, + `SELECT completed_at + interval '2 minutes' FROM ri_exchange_direct_executions WHERE id = $1`, row.ID).Scan(&checkedAt)) + body := fmt.Sprintf(`{"outcome":"failed","expected_status":"unknown","reason":"checked at AWS, no exchange happened", + "acknowledge_unknown_outcome":true,"cloud_check":{"checked_region":"eu-central-1","checked_at":%q}}`, checkedAt.UTC().Format(time.RFC3339)) + res, err := h.resolveRIExchangeDirectExecution(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, Body: body}, row.ID) + require.NoError(t, err) + resolved, ok := res.(riExchangeDirectResolveResponse) + require.True(t, ok) + assert.Equal(t, config.RIExchangeDirectStatusFailed, resolved.ResolvedStatus) + assert.Equal(t, config.RIExchangeDirectStatusUnknown, resolved.Status, "the original status is kept") + assert.False(t, resolved.Replayed) + + // 5. The list no longer shows the row. + assert.Empty(t, listDirectRows(t, h).Executions) + + // 6. A direct execute of the RI proceeds, and so do AUTO and approve + // reservations of the formerly held RIs. + lostAccept = false + _, err = h.executeExchange(auditRequest(ctx, directResolveSecondBody)) + require.NoError(t, err, "a direct execute of a resolved RI proceeds") + assert.Equal(t, 2, *calls) + + _, err = store.ReserveRIExchange(ctx, directResolveHistoryRecord("ri-0a1b2c3d"), "1000", "1000") + require.NoError(t, err, "AUTO reservation of a resolved RI") + approving := directResolveHistoryRecord("ri-4e5f6a7b") + require.NoError(t, store.SaveRIExchangeRecord(ctx, approving)) + _, err = store.ReserveApprovedRIExchange(ctx, approving.ID, "5000", "1000") + require.NoError(t, err, "approval of a resolved RI") +} diff --git a/internal/api/handler_ri_exchange_direct_resolve_openapi_test.go b/internal/api/handler_ri_exchange_direct_resolve_openapi_test.go new file mode 100644 index 000000000..9b2682635 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_openapi_test.go @@ -0,0 +1,49 @@ +package api + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +// A late finalize can move a resolved row to completed or failed, so the +// resolve response must document every provider status. +func TestOpenAPIDirectResolveStatusEnumMatchesGoStatuses(t *testing.T) { + _, thisFile, _, _ := runtime.Caller(0) + data, err := os.ReadFile(filepath.Join(filepath.Dir(thisFile), "openapi.yaml")) + require.NoError(t, err) + type enumProp struct { + Enum []string `yaml:"enum"` + } + var doc struct { + Paths map[string]struct { + Post struct { + Responses map[string]struct { + Content struct { + JSON struct { + Schema struct { + Properties struct { + Status enumProp `yaml:"status"` + } `yaml:"properties"` + } `yaml:"schema"` + } `yaml:"application/json"` + } `yaml:"content"` + } `yaml:"responses"` + } `yaml:"post"` + } `yaml:"paths"` + } + require.NoError(t, yaml.Unmarshal(data, &doc)) + path, ok := doc.Paths["/api/ri-exchange/direct-executions/{id}/resolve"] + require.True(t, ok, "resolve path is documented") + require.ElementsMatch(t, []string{ + config.RIExchangeDirectStatusSubmitted, + config.RIExchangeDirectStatusUnknown, + config.RIExchangeDirectStatusCompleted, + config.RIExchangeDirectStatusFailed, + }, path.Post.Responses["200"].Content.JSON.Schema.Properties.Status.Enum) +} diff --git a/internal/api/handler_ri_exchange_direct_resolve_request.go b/internal/api/handler_ri_exchange_direct_resolve_request.go new file mode 100644 index 000000000..b904f6163 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_request.go @@ -0,0 +1,105 @@ +package api + +import ( + "encoding/json" + "fmt" + "io" + "strings" + "time" + "unicode/utf8" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// riExchangeDirectResolveRequest is the body of POST +// /api/ri-exchange/direct-executions/{id}/resolve. expected_status is the +// status the operator saw in the list; outcome "failed" releases the source +// RIs and needs acknowledge_unknown_outcome. +type riExchangeDirectResolveRequest struct { + Outcome string `json:"outcome"` + ExpectedStatus string `json:"expected_status"` + Reason string `json:"reason"` + Acknowledge bool `json:"acknowledge_unknown_outcome"` + CloudCheck *riExchangeDirectResolveCloudCheck `json:"cloud_check"` +} + +type riExchangeDirectResolveCloudCheck struct { + Region string `json:"checked_region"` + CheckedAt string `json:"checked_at"` +} + +// decodeRIExchangeDirectResolve reads one JSON document and checks its shape +// against the row being resolved. Which times the store admits is decided +// there, on the database clock. +func decodeRIExchangeDirectResolve(raw, rowRegion string) (riExchangeDirectResolveRequest, time.Time, error) { + var body riExchangeDirectResolveRequest + if len(raw) > riExchangeResolveBodyMax { + return body, time.Time{}, NewClientError(400, "RI exchange resolve body is too large") + } + decoder := json.NewDecoder(strings.NewReader(raw)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&body); err != nil { + return body, time.Time{}, NewClientError(400, "Invalid RI exchange resolve body") + } + if err := decoder.Decode(new(any)); err != io.EOF { + return body, time.Time{}, NewClientError(400, "Expected one RI exchange resolve body") + } + body.Reason = strings.TrimSpace(body.Reason) + if err := body.validate(); err != nil { + return body, time.Time{}, err + } + at, err := body.checkedAt(rowRegion) + return body, at, err +} + +func (b riExchangeDirectResolveRequest) validate() error { + switch { + case b.Outcome != config.RIExchangeDirectStatusFailed && b.Outcome != config.RIExchangeDirectStatusCompleted: + return NewClientError(400, `outcome must be "failed" or "completed"`) + case b.ExpectedStatus != config.RIExchangeDirectStatusSubmitted && b.ExpectedStatus != config.RIExchangeDirectStatusUnknown: + return NewClientError(400, `expected_status must be "submitted" or "unknown"`) + case utf8.RuneCountInString(b.Reason) < riExchangeResolveReasonMin || utf8.RuneCountInString(b.Reason) > riExchangeResolveReasonMax: + return NewClientError(400, fmt.Sprintf("reason must be %d to %d characters", riExchangeResolveReasonMin, riExchangeResolveReasonMax)) + case hasControlRune(b.Reason): + return NewClientError(400, "reason must not contain control characters") + } + return b.validateCheckAndAck() +} + +func (b riExchangeDirectResolveRequest) validateCheckAndAck() error { + switch { + case b.CloudCheck == nil: + return NewClientError(400, "cloud_check is required: say where you verified the exchange at AWS") + case b.Outcome == config.RIExchangeDirectStatusFailed && !b.Acknowledge: + return NewClientError(400, "acknowledge_unknown_outcome is required to release the source RIs: CUDly cannot see whether the exchange happened") + } + return nil +} + +// checkedAt validates the shape of the operator's AWS-side check: the region +// it names must be the row's own. +func (b riExchangeDirectResolveRequest) checkedAt(rowRegion string) (time.Time, error) { + c := b.CloudCheck + if !riExchangeResolveRegionRe.MatchString(c.Region) || c.Region != rowRegion { + return time.Time{}, NewClientError(400, "checked_region must be the region of the exchange") + } + at, err := time.Parse(time.RFC3339, c.CheckedAt) + if err != nil { + return time.Time{}, NewClientError(400, "checked_at must be an RFC 3339 timestamp") + } + return at.UTC(), nil +} + +// note is the exact text stored in resolution_note. The fields are in a fixed +// order and quoted, so a repeat of the same request produces the same string +// and a different one cannot. That includes the status the operator saw: a +// repeat that differs only in expected_status is a conflict, while an identical +// retry stays a replay even after a late finalize moved the row's status. +func (b riExchangeDirectResolveRequest) note(checkedAt time.Time) string { + s := fmt.Sprintf("operator:%s: region=%q checked_at=%s expected=%s", + b.Outcome, b.CloudCheck.Region, checkedAt.Format(time.RFC3339Nano), b.ExpectedStatus) + if b.Outcome == config.RIExchangeDirectStatusFailed { + s += " ack=true" + } + return s + fmt.Sprintf(" reason=%q", b.Reason) +} diff --git a/internal/api/handler_ri_exchange_direct_resolve_route_test.go b/internal/api/handler_ri_exchange_direct_resolve_route_test.go new file mode 100644 index 000000000..4523d6147 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_route_test.go @@ -0,0 +1,30 @@ +package api + +import ( + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" +) + +func TestResolveRIExchangeDirect_RouteIsAuthUserPOST(t *testing.T) { + r := NewRouter(&Handler{}) + for _, route := range r.routes { + if route.PathPrefix == "/api/ri-exchange/direct-executions/" && route.PathSuffix == "/resolve" { + assert.Equal(t, "POST", route.Method) + assert.Equal(t, AuthUser, route.Auth, "the outer middleware enforces the session and CSRF") + return + } + } + t.Fatal("POST /api/ri-exchange/direct-executions/{id}/resolve is not registered") +} + +// The settle age rests on a bounded window: the direct execute takes one +// deadline before saving the row, the terminal save gets its own timeout, and +// a margin covers clock skew. A later timeout bump must not silently open the +// window. +func TestResolveRIExchangeDirect_SettleAgeCoversTheExecutionWindow(t *testing.T) { + window := riExchangeExecutionTimeout + riExchangeTerminalSaveTimeout + time.Minute + assert.Greater(t, config.RIExchangeDirectResolveSubmittedAfter, window) +} diff --git a/internal/api/handler_ri_exchange_direct_resolve_test.go b/internal/api/handler_ri_exchange_direct_resolve_test.go new file mode 100644 index 000000000..ebf3fb326 --- /dev/null +++ b/internal/api/handler_ri_exchange_direct_resolve_test.go @@ -0,0 +1,254 @@ +package api + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Handler tests for POST /api/ri-exchange/direct-executions/{id}/resolve +// (#1836). The store is a FIXTURE; the row lock, the settle age and the +// checked_at bounds are covered against real PostgreSQL in internal/config. + +const ( + drRegion = "eu-west-1" + // drOtherID is a second stuck row. Nothing returned for the first may name it. + drOtherID = dlID4 +) + +var drResolvedAt = time.Date(2026, 11, 8, 9, 0, 0, 0, time.UTC) + +type directResolveStore struct { + *directListStore + rows map[string]config.RIExchangeDirectRow + called int + got config.RIExchangeDirectResolve + res *config.RIExchangeDirectResolveResult + err error +} + +func (s *directResolveStore) GetRIExchangeDirectExecution(_ context.Context, id string) (*config.RIExchangeDirectRow, error) { + r, ok := s.rows[id] + if !ok { + return nil, config.ErrRIExchangeResolveNotFound + } + return &r, nil +} + +func (s *directResolveStore) ResolveRIExchangeDirectExecution(_ context.Context, r config.RIExchangeDirectResolve) (*config.RIExchangeDirectResolveResult, error) { + s.called++ + s.got = r + if s.res != nil || s.err != nil { + return s.res, s.err + } + return &config.RIExchangeDirectResolveResult{ID: r.ID, Status: r.ExpectedStatus, ResolvedStatus: r.Outcome, + ResolvedAt: drResolvedAt, Note: r.Note}, nil +} + +// drStore holds a stuck row in drRegion for account A, and a second row of +// account B whose id must never appear in a response for the first. +func drStore(rows ...config.RIExchangeDirectRow) *directResolveStore { + if len(rows) == 0 { + rows = []config.RIExchangeDirectRow{dlRow(dlID1, dlPtr(dlAcctA), drRegion, "unknown", 1)} + } + s := &directResolveStore{directListStore: dlStore(), rows: map[string]config.RIExchangeDirectRow{}} + for _, r := range rows { + s.rows[r.ID] = r + } + if _, ok := s.rows[drOtherID]; !ok { + s.rows[drOtherID] = dlRow(drOtherID, dlPtr(dlAcctB), drRegion, "unknown", 2) + } + return s +} + +// drBody is a valid request for an unknown row in drRegion. +func drBody(outcome, extra string) string { + return fmt.Sprintf(`{"outcome":%q,"expected_status":"unknown","reason":"source RIs still active in the EC2 console"%s,`+ + `"cloud_check":{"checked_region":%q,"checked_at":%q}}`, + outcome, extra, drRegion, riResolveAt.Format(time.RFC3339)) +} + +var drFailedBody = drBody("failed", `,"acknowledge_unknown_outcome":true`) + +type drCaller struct { + perms []auth.Permission + accounts []string // nil means unrestricted + adminKey bool + email string +} + +func (c drCaller) call(store *directResolveStore, id, body string) (any, error) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID, Email: c.email}, nil).Maybe() + mockAuth.grantPermissionsScoped(c.perms, c.accounts) + h := &Handler{config: store, auth: mockAuth, + // The deployment's account is a third one: only the ROW's account may + // decide authorization. + reshapeAccountResolver: func(context.Context) (string, error) { return "33333333-3333-4333-8333-333333333333", nil }} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}, Body: body} + if c.adminKey { + h.apiKey = apgAdminKy + req.Headers = map[string]string{"x-api-key": apgAdminKy} + } + return h.resolveRIExchangeDirectExecution(context.Background(), req, id) +} + +func TestResolveRIExchangeDirect_PassesOperatorDecisionToStore(t *testing.T) { + logs := captureLogs(t) + store := drStore() + out, err := drCaller{perms: riExecute, email: "ops@example.com"}.call(store, dlID1, drFailedBody) + require.NoError(t, err) + require.Equal(t, 1, store.called) + got := store.got + assert.Equal(t, dlID1, got.ID) + assert.Equal(t, config.RIExchangeDirectStatusFailed, got.Outcome) + assert.Equal(t, config.RIExchangeDirectStatusUnknown, got.ExpectedStatus) + assert.Equal(t, riResolveAt, got.CheckedAt) + require.NotNil(t, got.ResolvedByUserID) + assert.Equal(t, apgUserID, *got.ResolvedByUserID) + assert.Equal(t, "ops@example.com", got.ResolvedBy) + assert.Equal(t, `operator:failed: region="eu-west-1" checked_at=2026-10-19T08:00:00Z expected=unknown ack=true reason="source RIs still active in the EC2 console"`, got.Note) + resp, ok := out.(riExchangeDirectResolveResponse) + require.True(t, ok) + assert.Equal(t, riExchangeDirectResolveResponse{ID: dlID1, Status: "unknown", ResolvedStatus: "failed", + ResolvedAt: drResolvedAt, Note: got.Note}, resp) + for _, want := range []string{dlID1, apgUserID, "outcome=failed", "via="} { + assert.Contains(t, logs.String(), want) + } + + completed := drStore() + _, err = drCaller{perms: riExecute}.call(completed, dlID1, drBody("completed", "")) + require.NoError(t, err) + assert.Equal(t, `operator:completed: region="eu-west-1" checked_at=2026-10-19T08:00:00Z expected=unknown reason="source RIs still active in the EC2 console"`, completed.got.Note) +} + +// The store replays only a byte-identical note and refuses any other as a +// conflict, so two checks a fraction of a second apart must not share a note. +func TestResolveRIExchangeDirect_SubSecondCheckedAtChangesNote(t *testing.T) { + body := func(at string) string { + return fmt.Sprintf(`{"outcome":"completed","expected_status":"unknown","reason":"source RIs still active in the EC2 console",`+ + `"cloud_check":{"checked_region":%q,"checked_at":%q}}`, drRegion, at) + } + first, second := drStore(), drStore() + _, err := drCaller{perms: riExecute}.call(first, dlID1, body("2026-10-19T08:00:00.1Z")) + require.NoError(t, err) + _, err = drCaller{perms: riExecute}.call(second, dlID1, body("2026-10-19T08:00:00.2Z")) + require.NoError(t, err) + assert.NotEqual(t, first.got.Note, second.got.Note, "equal notes would replay instead of conflicting") + assert.Contains(t, first.got.Note, "checked_at=2026-10-19T08:00:00.1Z") +} + +func TestResolveRIExchangeDirect_ReplayIsReportedAndNotLogged(t *testing.T) { + logs := captureLogs(t) + store := drStore() + store.res = &config.RIExchangeDirectResolveResult{ID: dlID1, Status: "unknown", ResolvedStatus: "failed", + ResolvedAt: drResolvedAt, Note: "n", Replayed: true} + out, err := drCaller{perms: riExecute}.call(store, dlID1, drFailedBody) + require.NoError(t, err) + assert.True(t, out.(riExchangeDirectResolveResponse).Replayed) + assert.NotContains(t, logs.String(), "resolved by operator", "a replay wrote nothing, so it logs no resolution") +} + +func TestResolveRIExchangeDirect_MapsStoreOutcomes(t *testing.T) { + tests := []struct { + name string + err error + want int + }{ + {"missing at resolve time", config.ErrRIExchangeResolveNotFound, 404}, + {"migration not applied", config.ErrRIExchangeDirectResolveNotReady, 503}, + {"checked_at outside the window", fmt.Errorf("%w: checked_at is in the future", config.ErrRIExchangeResolveInvalid), 400}, + {"already settled", fmt.Errorf("%w: the exchange is already completed", config.ErrRIExchangeResolveConflict), 409}, + {"may still be in flight", fmt.Errorf("%w: the exchange may still be in flight; wait until 2026-11-08T09:10:00Z", config.ErrRIExchangeResolveConflict), 409}, + {"already resolved differently", fmt.Errorf("%w: the exchange was already resolved as failed", config.ErrRIExchangeResolveConflict), 409}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store := drStore() + store.err = tc.err + _, err := drCaller{perms: riExecute}.call(store, dlID1, drFailedBody) + if tc.want == 404 { + require.ErrorIs(t, err, errNotFound) + return + } + assert.Equal(t, tc.want, clientCode(t, err)) + }) + } + t.Run("an unexpected store failure is not a client error", func(t *testing.T) { + store := drStore() + store.err = errors.New("connection reset") + _, err := drCaller{perms: riExecute}.call(store, dlID1, drFailedBody) + require.Error(t, err) + _, isClient := IsClientError(err) + assert.False(t, isClient) + }) +} + +func TestResolveRIExchangeDirect_Needs503WithoutTheStore(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: apgUserID}, nil).Maybe() + mockAuth.grantPermissions(riExecute) + h := &Handler{config: new(MockConfigStore), auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}, Body: drFailedBody} + _, err := h.resolveRIExchangeDirectExecution(context.Background(), req, dlID1) + assert.Equal(t, 503, clientCode(t, err)) +} + +// The status the operator saw is part of the replay identity: a repeat that +// changes only expected_status must not read as the same request. +func TestResolveRIExchangeDirect_NoteDiffersByExpectedStatus(t *testing.T) { + body := func(expected string) string { + return fmt.Sprintf(`{"outcome":"failed","expected_status":%q,"reason":"source RIs still active in the EC2 console",`+ + `"acknowledge_unknown_outcome":true,"cloud_check":{"checked_region":%q,"checked_at":%q}}`, + expected, drRegion, riResolveAt.Format(time.RFC3339)) + } + unknown, submitted := drStore(), drStore() + _, err := drCaller{perms: riExecute}.call(unknown, dlID1, body("unknown")) + require.NoError(t, err) + _, err = drCaller{perms: riExecute}.call(submitted, dlID1, body("submitted")) + require.NoError(t, err) + assert.Contains(t, unknown.got.Note, "expected=unknown") + assert.Contains(t, submitted.got.Note, "expected=submitted") + assert.NotEqual(t, unknown.got.Note, submitted.got.Note) +} + +// A group may list an account by its AWS account number, which the group write +// keeps verbatim. The row's registered account must be admitted by that entry, +// as by its internal id; another account's number must not admit it. +func TestResolveRIExchangeDirect_AllowedAccountsMayNameTheAWSAccountNumber(t *testing.T) { + accounts := []config.CloudAccount{ + {ID: dlAcctA, Provider: "aws", ExternalID: "111122223333"}, + {ID: dlAcctB, Provider: "aws", ExternalID: "999999999999"}, + } + for name, tc := range map[string]struct { + allowed []string + denied bool + }{ + "by the account number": {[]string{"111122223333"}, false}, + "by the internal id": {[]string{dlAcctA}, false}, + "another account's number": {[]string{"999999999999"}, true}, + "a number that is nobody's": {[]string{"000000000000"}, true}, + } { + t.Run(name, func(t *testing.T) { + store := drStore() + store.ListCloudAccountsFn = func(context.Context, config.CloudAccountFilter) ([]config.CloudAccount, error) { return accounts, nil } + _, err := drCaller{perms: riExecute, accounts: tc.allowed}.call(store, dlID1, drFailedBody) + if !tc.denied { + require.NoError(t, err) + return + } + require.ErrorIs(t, err, errNotFound, "a row outside the caller's accounts reads as not found") + assert.Zero(t, store.called) + }) + } +} diff --git a/internal/api/handler_ri_exchange_resolve.go b/internal/api/handler_ri_exchange_resolve.go index be5924492..e226906d4 100644 --- a/internal/api/handler_ri_exchange_resolve.go +++ b/internal/api/handler_ri_exchange_resolve.go @@ -118,6 +118,16 @@ func (h *Handler) requireRIExchangeResolveConstraints(ctx context.Context, sessi if err != nil { return fmt.Errorf("failed to resolve cloud account scope: %w", err) } + return h.requireRIExchangeConstraintsFor(ctx, session, cloudAccountID, region, func() (float64, error) { + return h.riExchangeHoldAmount(ctx, id) + }) +} + +// requireRIExchangeConstraintsFor is the constraint check shared by the +// history resolve and the direct-row resolve: the grant must allow the AWS EC2 +// exchange in the given account and region, and when it caps the amount, +// amount() must fit under the cap. A grant without a cap never calls amount. +func (h *Handler) requireRIExchangeConstraintsFor(ctx context.Context, session *Session, cloudAccountID, region string, amount func() (float64, error)) error { c := auth.PermissionConstraints{ AccountIDs: []string{cloudAccountID}, Providers: []string{string(common.ProviderAWS)}, @@ -135,7 +145,7 @@ func (h *Handler) requireRIExchangeResolveConstraints(ctx context.Context, sessi if otherErr := check(0); otherErr != nil { return otherErr } - due, err := h.riExchangeHoldAmount(ctx, id) + due, err := amount() if err != nil { return err } @@ -155,13 +165,23 @@ func (h *Handler) riExchangeHoldAmount(ctx context.Context, id string) (float64, if record == nil { return 0, errNotFound } - due, err := strconv.ParseFloat(record.PaymentDue, 64) - if err != nil || math.IsNaN(due) || math.IsInf(due, 0) || due < 0 { + due, ok := parseRIExchangeAmount(record.PaymentDue) + if !ok { return 0, fmt.Errorf("unreadable hold amount on exchange %s", id) } return due, nil } +// parseRIExchangeAmount reads a stored amount, refusing a value that is not a +// finite non-negative number. +func parseRIExchangeAmount(s string) (float64, bool) { + due, err := strconv.ParseFloat(s, 64) + if err != nil || math.IsNaN(due) || math.IsInf(due, 0) || due < 0 { + return 0, false + } + return due, true +} + func mapRIExchangeResolveError(err error) error { switch { case err == nil: @@ -172,6 +192,8 @@ func mapRIExchangeResolveError(err error) error { return NewClientError(400, err.Error()) case errors.Is(err, config.ErrRIExchangeResolveConflict): return NewClientError(409, err.Error()) + case errors.Is(err, config.ErrRIExchangeDirectResolveNotReady): + return NewClientError(503, err.Error()) } return err } diff --git a/internal/api/handler_ri_exchange_resolve_test.go b/internal/api/handler_ri_exchange_resolve_test.go index 17fd9b6aa..4e5384fd9 100644 --- a/internal/api/handler_ri_exchange_resolve_test.go +++ b/internal/api/handler_ri_exchange_resolve_test.go @@ -288,3 +288,42 @@ func TestResolveRIExchange_CappedGrantMissingHoldIsNotFound(t *testing.T) { require.ErrorIs(t, err, errNotFound) assert.Zero(t, store.called) } + +// The constraints check must use the checked region and the deployment's own +// account, not a fixed pair: each case differs from the other cases in exactly +// the dimension a hard-coded region or a swapped account would hide. +func TestResolveRIExchange_ConstraintsUseTheCheckedRegionAndTheDeploymentAccount(t *testing.T) { + grant := func(c auth.PermissionConstraints) []auth.Permission { + return []auth.Permission{{Action: auth.ActionExecute, Resource: "ri-exchange", Constraints: &c}} + } + tests := []struct { + name string + perms []auth.Permission + region string + deployment string + want int // 0 = allowed + }{ + {"grant for the checked region, which is not us-east-1", grant(auth.PermissionConstraints{Regions: []string{"eu-west-1"}}), "eu-west-1", scopedInAccount, 0}, + {"grant for us-east-1 against another checked region", grant(auth.PermissionConstraints{Regions: []string{"us-east-1"}}), "eu-west-1", scopedInAccount, 403}, + {"grant for the deployment's account", grant(auth.PermissionConstraints{AccountIDs: []string{scopedInAccount}}), "us-east-1", scopedInAccount, 0}, + {"grant for another account than the deployment's", grant(auth.PermissionConstraints{AccountIDs: []string{scopedInAccount}}), "us-east-1", scopedOutAccount, 403}, + {"grant for the other account, matching the deployment", grant(auth.PermissionConstraints{AccountIDs: []string{scopedOutAccount}}), "us-east-1", scopedOutAccount, 0}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store := &resolveRIStore{MockConfigStore: new(MockConfigStore)} + h, req := apgCaller{perms: tc.perms}.handlerAndReq(store.MockConfigStore, + strings.ReplaceAll(riResolveFailedBody, "us-east-1", tc.region)) + h.config = store + h.reshapeAccountResolver = func(context.Context) (string, error) { return tc.deployment, nil } + _, err := h.resolveRIExchange(context.Background(), req, resolveRIID) + if tc.want == 0 { + require.NoError(t, err) + assert.Equal(t, 1, store.called) + return + } + assert.Equal(t, tc.want, clientCode(t, err)) + assert.Zero(t, store.called, "a refused caller must not reach the store") + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 9dde67325..18943e73a 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2881,6 +2881,274 @@ paths: '500': $ref: '#/components/responses/InternalError' + /api/ri-exchange/direct-executions: + get: + operationId: listRIExchangeDirectExecutions + tags: [RIExchange] + summary: List AWS direct RI exchanges that still hold their source RIs + description: > + An AWS direct execute whose outcome was never recorded stays `submitted` + or `unknown`, and the in-flight guard then refuses every later exchange + of its source RIs. This lists those unresolved rows, oldest first, so an + operator can check AWS and resolve them. Requires `execute:ri-exchange`. + A row is listed only when the session's allowed accounts cover its + account (a row with no registered account needs an unrestricted + session) and the grant on `execute:ri-exchange` covers its account and + region, evaluated as the direct execute evaluates it. Rows outside that + are absent, not an error, so an empty list can mean nothing is stuck or + nothing is visible. `error_summary` is a sanitized category message; the + stored provider text, the executing user id and the API key id are never + returned. `resolvable_at` is the earliest time the store would accept a + resolve: ten minutes after creation for a `submitted` row, one minute + after completion for an `unknown` row. + parameters: + - name: status + in: query + required: false + schema: + type: string + enum: [submitted, unknown] + description: Default is both. + - name: ri_id + in: query + required: false + schema: + type: string + maxLength: 200 + description: Source RI id, matched trimmed and case-insensitively. + - name: region + in: query + required: false + schema: + type: string + pattern: '^[a-z0-9-]{1,32}$' + - name: limit + in: query + required: false + schema: + type: integer + minimum: 1 + maximum: 200 + default: 50 + - name: cursor + in: query + required: false + schema: + type: string + description: The `next_cursor` of the previous page. Opaque. + responses: + '200': + description: One page of unresolved direct exchanges + content: + application/json: + schema: + type: object + required: [executions, next_cursor] + properties: + executions: + type: array + items: + type: object + required: [id, status, provider, region, cloud_account_id, source_ids, targets, max_payment_due, max_payment_currency, created_at, executed_by, error_summary, resolvable, resolvable_at] + properties: + id: + type: string + format: uuid + status: + type: string + enum: [submitted, unknown] + provider: + type: string + enum: [aws] + region: + type: string + cloud_account_id: + type: string + nullable: true + description: The registered cloud account of the exchange, null when it mapped to none. + source_ids: + type: array + items: + type: string + targets: + type: array + items: + type: object + max_payment_due: + type: string + max_payment_currency: + type: string + created_at: + type: string + format: date-time + executed_by: + type: string + error_summary: + type: string + resolvable: + type: boolean + resolvable_at: + type: string + format: date-time + next_cursor: + type: string + description: Empty at the end of the list. + '400': + description: Invalid `status`, `region`, `ri_id`, `limit` or `cursor`. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + $ref: '#/components/responses/Forbidden' + '503': + description: RI exchange resolve storage is unavailable. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + + /api/ri-exchange/direct-executions/{id}/resolve: + parameters: + - $ref: '#/components/parameters/ResourceID' + post: + operationId: resolveRIExchangeDirectExecution + tags: [RIExchange] + summary: Record an operator's outcome for a stuck AWS direct RI exchange + description: > + Settles a row from `GET /api/ri-exchange/direct-executions` after the + operator checked AWS. The row keeps its original status and gains the + resolution, and its source RIs stop blocking later exchanges. `failed` + releases them and requires `acknowledge_unknown_outcome`. Requires + `execute:ri-exchange`, checked against the row's own account, region + and `max_payment_due` (not the deployment's), and a signed-in user or + user API key (the admin API key is refused). A `submitted` row is + refused until 10 minutes after it was created, and `checked_at` must + be at or after that time (for an `unknown` row, one minute after it + completed) and not in the future. `expected_status` must be the status + the row has now. A repeat of the same request returns the stored + result with `replayed: true`; a repeat that differs in any field, + including `expected_status`, is a conflict. A row outside the session's allowed + accounts reads as not found. + parameters: + - $ref: '#/components/parameters/CSRFToken' + requestBody: + required: true + content: + application/json: + schema: + type: object + additionalProperties: false + required: [outcome, expected_status, reason, cloud_check] + properties: + outcome: + type: string + enum: [failed, completed] + expected_status: + type: string + enum: [submitted, unknown] + description: The status the operator saw in the list. + reason: + type: string + minLength: 10 + maxLength: 300 + description: > + Free text, trimmed, without control characters. At most + 300 characters, and the stored note at most 1000 bytes, so + non-ASCII text needs fewer characters. Past that the API + answers 400 "reason too long once encoded; shorten it". + acknowledge_unknown_outcome: + type: boolean + description: Must be true when outcome is failed. + cloud_check: + type: object + additionalProperties: false + required: [checked_region, checked_at] + description: Where and when the operator verified the exchange at AWS. + properties: + checked_region: + type: string + pattern: '^[a-z0-9-]{1,32}$' + description: Must be the region of the exchange. + checked_at: + type: string + format: date-time + responses: + '200': + description: Row resolved, or the same resolve replayed + content: + application/json: + schema: + type: object + required: [id, status, resolved_status, resolved_at, note, replayed] + properties: + id: + type: string + format: uuid + status: + type: string + enum: [submitted, unknown, completed, failed] + description: > + The row's provider status, which resolving leaves + unchanged. A late finalize may have set it to + `completed` or `failed` since. `resolved_status` is the + operator's resolution. + resolved_status: + type: string + enum: [completed, failed] + resolved_at: + type: string + format: date-time + note: + type: string + replayed: + type: boolean + '400': + description: > + Invalid id or body: body over 4096 bytes, unknown fields, a + missing or invalid field, a reason over 300 characters or whose + stored note exceeds 1000 bytes ("reason too long once encoded; + shorten it"), a region other than the row's, or a `checked_at` + outside the allowed window. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + description: > + CSRF validation failed, the session lacks `execute:ri-exchange` or + fails its constraints for the row's account, region or amount, or + the caller is the admin API key. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '404': + description: No such AWS direct row, or its account is outside the session's allowed accounts. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '409': + description: > + The row cannot be resolved this way: already resolved differently, + already settled, its status is not `expected_status`, or a + `submitted` row may still be in flight. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '503': + description: RI exchange resolve storage is unavailable. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + /api/ri-exchange/resolve/{id}: parameters: - $ref: '#/components/parameters/ResourceID' diff --git a/internal/api/router.go b/internal/api/router.go index 5bb2f961b..18d6be8c6 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -334,6 +334,11 @@ func (r *Router) registerRoutes() { // Operator resolve of a stale processing exchange (#692). AuthUser, so the // outer middleware enforces the session and CSRF. {PathPrefix: "/api/ri-exchange/resolve/", Method: "POST", Handler: r.resolveRIExchangeHandler, Auth: AuthUser}, + // Operator list of AWS direct-execute rows that still hold their source RIs (#1836). + {ExactPath: "/api/ri-exchange/direct-executions", Method: "GET", Handler: r.listRIExchangeDirectExecutionsHandler, Auth: AuthUser}, + // Operator resolve of one such row. AuthUser, so the outer middleware + // enforces the session and CSRF. + {PathPrefix: "/api/ri-exchange/direct-executions/", PathSuffix: "/resolve", Method: "POST", Handler: r.resolveRIExchangeDirectExecutionHandler, Auth: AuthUser}, // Commitment Laddering endpoints (flag-gated default-off, issue #1336). // GET returns all per-account ladder configs; PUT inserts or updates one. @@ -925,6 +930,14 @@ func (r *Router) rejectRIExchangeHandler(ctx context.Context, req *events.Lambda return r.h.rejectRIExchange(ctx, params["id"], req.QueryStringParameters["token"]) } +func (r *Router) listRIExchangeDirectExecutionsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return r.h.listRIExchangeDirectExecutions(ctx, req) +} + +func (r *Router) resolveRIExchangeDirectExecutionHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.resolveRIExchangeDirectExecution(ctx, req, params["id"]) +} + func (r *Router) resolveRIExchangeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.resolveRIExchange(ctx, req, params["id"]) } diff --git a/internal/config/ri_exchange_direct_list.go b/internal/config/ri_exchange_direct_list.go new file mode 100644 index 000000000..3043fb571 --- /dev/null +++ b/internal/config/ri_exchange_direct_list.go @@ -0,0 +1,57 @@ +package config + +import ( + "encoding/json" + "time" +) + +// RIExchangeDirectRow is an AWS direct-execute audit row as the operator +// resolve API reads it. It carries no executing user id or API key id. Error +// is the raw stored provider text: callers pass it through +// purchase.SanitizeStoredError and never return it as is. +type RIExchangeDirectRow struct { + ID string + Status string + CloudAccountID *string + Region string + SourceIDs []string + Targets json.RawMessage + MaxPaymentDue string + MaxPaymentCurrency string + Error string + ExecutedBy string + CreatedAt time.Time + CompletedAt *time.Time + ResolvedAt *time.Time +} + +// RIExchangeDirectAccountRegion is one (account, region) pair of a row. A nil +// CloudAccountID is a row whose deployment mapped to no registered account. +type RIExchangeDirectAccountRegion struct { + CloudAccountID *string + Region string +} + +// RIExchangeDirectCursor is the keyset position after the last row of a page. +type RIExchangeDirectCursor struct { + CreatedAt time.Time + ID string +} + +// RIExchangeDirectListFilter selects unresolved AWS direct rows. Allowed is +// the set of (account, region) pairs the caller may see, derived by the +// handler from the caller's own authorization; an empty set returns nothing. +type RIExchangeDirectListFilter struct { + Allowed []RIExchangeDirectAccountRegion + Statuses []string // submitted and/or unknown; empty means both + RIID string // matched like the in-flight guard: trimmed, lower case + Region string + Limit int + After *RIExchangeDirectCursor +} + +// RIExchangeDirectListPage is one page, oldest first. Next is nil at the end. +type RIExchangeDirectListPage struct { + Rows []RIExchangeDirectRow + Next *RIExchangeDirectCursor +} diff --git a/internal/config/ri_exchange_direct_resolve.go b/internal/config/ri_exchange_direct_resolve.go new file mode 100644 index 000000000..660cb5700 --- /dev/null +++ b/internal/config/ri_exchange_direct_resolve.go @@ -0,0 +1,61 @@ +package config + +import ( + "context" + "errors" + "time" +) + +// RIExchangeDirectResolveSubmittedAfter is how long a submitted AWS direct row +// must have existed before an operator may resolve it: the direct execute +// bounds its claim and provider call by one deadline taken before the row is +// saved (#1841), so no live process can still finalize it after this long. +// Measured on the database clock, like created_at. +const RIExchangeDirectResolveSubmittedAfter = 10 * time.Minute + +// RIExchangeDirectResolveNoteMax bounds resolution_note, in bytes. +const RIExchangeDirectResolveNoteMax = 1000 + +// RIExchangeDirectResolveUnknownCheckAfter is how long after an unknown row's +// completed_at the operator's AWS check must have been made, to allow for +// DescribeReservedInstances eventual consistency. +const RIExchangeDirectResolveUnknownCheckAfter = time.Minute + +// RIExchangeDirectResolve asks to settle a stuck AWS direct row on an +// operator's word. Outcome is completed or failed; ExpectedStatus is the +// status (submitted or unknown) the operator saw. Note is the complete text +// stored in resolution_note, built by the caller so a repeat compares equal +// as a whole. CheckedAt is when the operator checked AWS. +type RIExchangeDirectResolve struct { + ID string + Outcome string + ExpectedStatus string + Note string + CheckedAt time.Time + ResolvedByUserID *string + ResolvedBy string +} + +// RIExchangeDirectResolveResult is the row after the call. Status is the +// original status, unchanged. Replayed is true when an identical earlier +// request had already resolved the row and nothing was written. +type RIExchangeDirectResolveResult struct { + ID string + Status string + ResolvedStatus string + ResolvedAt time.Time + Note string + Replayed bool +} + +// ErrRIExchangeDirectResolveNotReady: the resolution migration is not applied yet. +var ErrRIExchangeDirectResolveNotReady = errors.New("resolving direct RI exchanges is not available until the database migration is applied") + +// RIExchangeDirectResolveStore is the store surface of the direct-row list and +// resolve endpoints. +type RIExchangeDirectResolveStore interface { + RIExchangeDirectListStore + ResolveRIExchangeDirectExecution(ctx context.Context, req RIExchangeDirectResolve) (*RIExchangeDirectResolveResult, error) +} + +var _ RIExchangeDirectResolveStore = (*PostgresStore)(nil) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 6e13cb0d4..435e1921f 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3488,8 +3488,9 @@ func normaliseRIExchangeSourceIDs(sources []string) []string { // refuseRIExchangeSourceInFlightDirect is the sibling guard for direct // executes: an AWS ri_exchange_direct_executions row still submitted (the // accept may be committing) or unknown (it may have committed) holds its -// source RIs. It normalises its own input and compares against the stored ids -// normalised the same way, so spelling and padding cannot slip an RI past it. +// source RIs until an operator resolves it (resolved_at set, #1836). It +// normalises its own input and compares against the stored ids normalised the +// same way, so spelling and padding cannot slip an RI past it. // Like refuseRIExchangeSourceInFlight it runs under the daily-spend advisory // lock, fails closed on a lookup error, and logs the blocking row's id rather // than returning it (#1428). @@ -3498,6 +3499,13 @@ func refuseRIExchangeSourceInFlightDirect(ctx context.Context, tx pgx.Tx, source if len(normalised) == 0 { return nil } + resolvedFilter := " AND resolved_at IS NULL" + if ready, err := directResolveSchemaReady(ctx, tx); err != nil { + logging.Errorf("RI exchange reservation: schema version check failed: %v", err) + return errors.New("failed to check RI exchange source RIs held by a direct execute") + } else if !ready { + resolvedFilter = "" + } var blocker string var shared []string err := tx.QueryRow(ctx, ` @@ -3505,7 +3513,7 @@ func refuseRIExchangeSourceInFlightDirect(ctx context.Context, tx pgx.Tx, source FROM ( SELECT id, created_at, ARRAY(SELECT lower(btrim(x, E' \t\n\r\f\x0B')) FROM unnest(source_ids) x) AS ids FROM ri_exchange_direct_executions - WHERE provider = 'aws' AND status IN ('submitted', 'unknown') + WHERE provider = 'aws' AND status IN ('submitted', 'unknown')`+resolvedFilter+` ) d WHERE ids && $1::text[] ORDER BY created_at, id diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index fa378c975..f45cc1a29 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -3586,6 +3586,7 @@ func TestPGXMock_ReserveApprovedRIExchange_ReservesCeilingExcludingOwnRow(t *tes mock.ExpectQuery("SELECT id::text, ARRAY").WithArgs([]string{"ri-1"}, &id). WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) // Nor does an in-flight direct execute (#1829). + expectDirectResolveVersion(mock, directResolveMigrationVersion) mock.ExpectQuery("FROM ri_exchange_direct_executions").WithArgs([]string{"ri-1"}). WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) mock.ExpectExec("UPDATE ri_exchange_history SET payment_due").WithArgs(id, "1000.000000"). diff --git a/internal/config/store_postgres_ri_exchange_direct.go b/internal/config/store_postgres_ri_exchange_direct.go index fc6637167..e40c5b38a 100644 --- a/internal/config/store_postgres_ri_exchange_direct.go +++ b/internal/config/store_postgres_ri_exchange_direct.go @@ -6,6 +6,7 @@ import ( "fmt" "strings" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/jackc/pgx/v5" ) @@ -103,6 +104,12 @@ func (s *PostgresStore) FinalizeRIExchangeDirectExecution(ctx context.Context, i if err := outcome.Validate(); err != nil { return err } + ready, err := directResolveSchemaReady(ctx, s.db) + if err != nil { + // The provider outcome must not be lost over a version read: the plain UPDATE is right on either schema. + logging.Errorf("ri-exchange direct execute %s: schema version unreadable, finalizing without the resolution check: %v", id, err) + ready = false + } query := ` UPDATE ri_exchange_direct_executions SET status = $2, @@ -113,9 +120,8 @@ func (s *PostgresStore) FinalizeRIExchangeDirectExecution(ctx context.Context, i error = $7, completed_at = now() WHERE id = $1 - AND status = 'submitted' - ` - tag, err := s.db.Exec(ctx, query, + AND status = 'submitted'` + args := []any{ id, outcome.Status, nullStringFromString(outcome.ProviderReference), @@ -123,7 +129,16 @@ func (s *PostgresStore) FinalizeRIExchangeDirectExecution(ctx context.Context, i nullStringFromString(outcome.NetPayableCurrency), nullStringFromString(outcome.ProviderStatus), nullStringFromString(outcome.Error), - ) + } + if !ready { + // No resolved_status column yet (or the schema version is unreadable): nothing to check against. + return s.finalizeDirectPlain(ctx, id, query, args) + } + return s.finalizeDirectChecked(ctx, id, query, args, outcome) +} + +func (s *PostgresStore) finalizeDirectPlain(ctx context.Context, id, query string, args []any) error { + tag, err := s.db.Exec(ctx, query, args...) if err != nil { return fmt.Errorf("failed to finalize ri exchange direct execution %s: %w", id, err) } @@ -132,3 +147,20 @@ func (s *PostgresStore) FinalizeRIExchangeDirectExecution(ctx context.Context, i } return nil } + +func (s *PostgresStore) finalizeDirectChecked(ctx context.Context, id, query string, args []any, outcome RIExchangeDirectOutcome) error { + var resolved *string + err := s.db.QueryRow(ctx, query+"\n\t\tRETURNING resolved_status", args...).Scan(&resolved) + if errors.Is(err, pgx.ErrNoRows) { + return fmt.Errorf("finalize ri exchange direct execution %s: %w", id, ErrRIExchangeDirectExecutionNotSubmitted) + } + if err != nil { + return fmt.Errorf("failed to finalize ri exchange direct execution %s: %w", id, err) + } + // A late finalize can outlive an operator's resolution. Both facts are + // kept; a contradiction is logged because no reader lists resolved rows. + if resolved != nil && outcome.Status != RIExchangeDirectStatusUnknown && *resolved != outcome.Status { + logging.Errorf("ri-exchange direct execute %s: provider outcome %q contradicts operator resolution %q", id, outcome.Status, *resolved) + } + return nil +} diff --git a/internal/config/store_postgres_ri_exchange_direct_get.go b/internal/config/store_postgres_ri_exchange_direct_get.go new file mode 100644 index 000000000..1c54cd2da --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_get.go @@ -0,0 +1,66 @@ +package config + +import ( + "context" + "errors" + "fmt" + + "github.com/jackc/pgx/v5" +) + +// ListUnresolvedRIExchangeDirectAccountRegions returns the distinct (account, +// region) pairs of unresolved AWS rows, so the handler can ask the +// authorization service about each pair once. The unresolved partial index +// serves it. +func (s *PostgresStore) ListUnresolvedRIExchangeDirectAccountRegions(ctx context.Context) ([]RIExchangeDirectAccountRegion, error) { + if err := requireDirectResolveSchema(ctx, s.db); err != nil { + return nil, err + } + rows, err := s.db.Query(ctx, ` + SELECT DISTINCT cloud_account_id, COALESCE(region, '') + FROM ri_exchange_direct_executions + WHERE provider = 'aws' AND status IN ('submitted', 'unknown') AND resolved_at IS NULL + ORDER BY 1 NULLS FIRST, 2`) + if err != nil { + return nil, fmt.Errorf("failed to list unresolved ri exchange direct accounts: %w", err) + } + defer rows.Close() + var out []RIExchangeDirectAccountRegion + for rows.Next() { + var p RIExchangeDirectAccountRegion + if err := rows.Scan(&p.CloudAccountID, &p.Region); err != nil { + return nil, fmt.Errorf("failed to scan unresolved ri exchange direct account: %w", err) + } + out = append(out, p) + } + return out, rows.Err() +} + +// GetRIExchangeDirectExecution returns one AWS direct row of any status, or +// ErrRIExchangeResolveNotFound for a missing or non-AWS row. +func (s *PostgresStore) GetRIExchangeDirectExecution(ctx context.Context, id string) (*RIExchangeDirectRow, error) { + if err := requireDirectResolveSchema(ctx, s.db); err != nil { + return nil, err + } + r, err := scanRIExchangeDirectRow(s.db.QueryRow(ctx, ` + SELECT`+riExchangeDirectRowColumns+` + FROM ri_exchange_direct_executions + WHERE id = $1 AND provider = 'aws'`, id)) + if errors.Is(err, pgx.ErrNoRows) { + return nil, ErrRIExchangeResolveNotFound + } + if err != nil { + return nil, fmt.Errorf("failed to read ri exchange direct execution %s: %w", id, err) + } + return &r, nil +} + +// RIExchangeDirectListStore is the read side of the direct-row resolve API. +// Errors are the ErrRIExchangeResolve* values. +type RIExchangeDirectListStore interface { + ListUnresolvedRIExchangeDirectAccountRegions(ctx context.Context) ([]RIExchangeDirectAccountRegion, error) + ListUnresolvedRIExchangeDirectExecutions(ctx context.Context, f RIExchangeDirectListFilter) (*RIExchangeDirectListPage, error) + GetRIExchangeDirectExecution(ctx context.Context, id string) (*RIExchangeDirectRow, error) +} + +var _ RIExchangeDirectListStore = (*PostgresStore)(nil) diff --git a/internal/config/store_postgres_ri_exchange_direct_get_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_get_integration_test.go new file mode 100644 index 000000000..02267d500 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_get_integration_test.go @@ -0,0 +1,69 @@ +//go:build integration + +package config + +import ( + "reflect" + "strings" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// [fail-first] The distinct pairs read behind the handler's authorization. +func TestPostgresStoreDB_RIExchangeDirectList_AccountRegionPairs(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + a1 := strPtr("acc-1") + env.insertDirect(t, directSpec{account: a1}) + env.insertDirect(t, directSpec{account: a1, status: "submitted"}) + env.insertDirect(t, directSpec{account: nil, region: "us-east-1"}) + env.insertDirect(t, directSpec{account: a1, resolved: true, region: "ap-south-1"}) + env.insertDirect(t, directSpec{provider: "azure", account: a1}) + env.insertDirect(t, directSpec{status: "completed", account: strPtr("acc-9"), region: "sa-east-1"}) + + pairs, err := env.storeA.ListUnresolvedRIExchangeDirectAccountRegions(env.ctx) + require.NoError(t, err) + require.Len(t, pairs, 2) + assert.Nil(t, pairs[0].CloudAccountID) + assert.Equal(t, "us-east-1", pairs[0].Region) + assert.Equal(t, "acc-1", *pairs[1].CloudAccountID) + assert.Equal(t, "eu-west-1", pairs[1].Region) +} + +// [fail-first] Get returns an AWS row of any status, and not found for Azure or +// a missing id. +func TestPostgresStoreDB_GetRIExchangeDirectExecution(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + resolved := env.insertDirect(t, directSpec{account: acc, resolved: true, errText: "raw provider text"}) + az := env.insertDirect(t, directSpec{provider: "azure"}) + + got, err := env.storeA.GetRIExchangeDirectExecution(env.ctx, resolved) + require.NoError(t, err) + assert.Equal(t, resolved, got.ID) + assert.Equal(t, "unknown", got.Status) + assert.Equal(t, "eu-west-1", got.Region) + assert.Equal(t, "acc-1", *got.CloudAccountID) + assert.Equal(t, "100", got.MaxPaymentDue) + assert.Equal(t, "raw provider text", got.Error) + assert.NotNil(t, got.ResolvedAt) + assert.NotNil(t, got.CompletedAt) + assert.JSONEq(t, `[{"offering_id":"o","count":1}]`, string(got.Targets)) + + _, err = env.storeA.GetRIExchangeDirectExecution(env.ctx, az) + require.ErrorIs(t, err, ErrRIExchangeResolveNotFound) + _, err = env.storeA.GetRIExchangeDirectExecution(env.ctx, uuid.NewString()) + require.ErrorIs(t, err, ErrRIExchangeResolveNotFound) +} + +// The read type never carries who executed through which credential. +func TestRIExchangeDirectRow_HasNoCredentialFields(t *testing.T) { + typ := reflect.TypeOf(RIExchangeDirectRow{}) + for i := 0; i < typ.NumField(); i++ { + name := strings.ToLower(typ.Field(i).Name) + assert.NotContains(t, name, "userid", typ.Field(i).Name) + assert.NotContains(t, name, "apikey", typ.Field(i).Name) + } +} diff --git a/internal/config/store_postgres_ri_exchange_direct_list.go b/internal/config/store_postgres_ri_exchange_direct_list.go new file mode 100644 index 000000000..d3e7278b9 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_list.go @@ -0,0 +1,135 @@ +package config + +import ( + "context" + "fmt" + "time" + + "github.com/jackc/pgx/v5" +) + +// The list's page size: the default, and the most a caller may ask for. +const ( + RIExchangeDirectListDefaultLimit = 50 + RIExchangeDirectListMaxLimit = 200 +) + +const riExchangeDirectRowColumns = ` + id::text, status, cloud_account_id, COALESCE(region, ''), source_ids, targets::text, + max_payment_due::text, max_payment_currency, COALESCE(error, ''), executed_by, + created_at, completed_at, resolved_at` + +func scanRIExchangeDirectRow(row pgx.Row) (RIExchangeDirectRow, error) { + var r RIExchangeDirectRow + var targets string + err := row.Scan(&r.ID, &r.Status, &r.CloudAccountID, &r.Region, &r.SourceIDs, &targets, + &r.MaxPaymentDue, &r.MaxPaymentCurrency, &r.Error, &r.ExecutedBy, + &r.CreatedAt, &r.CompletedAt, &r.ResolvedAt) + r.Targets = []byte(targets) + return r, err +} + +// ListUnresolvedRIExchangeDirectExecutions returns one keyset page of +// unresolved AWS direct rows (submitted or unknown), oldest first, restricted +// to the filter's allowed (account, region) pairs. The pairs travel as two +// arrays so no caller input reaches the SQL text, a NULL account matches only +// a NULL pair entry, and a NULL region matches the empty region. +func (s *PostgresStore) ListUnresolvedRIExchangeDirectExecutions(ctx context.Context, f RIExchangeDirectListFilter) (*RIExchangeDirectListPage, error) { + limit, statuses, err := validateRIExchangeDirectListFilter(f) + if err != nil { + return nil, err + } + if schemaErr := requireDirectResolveSchema(ctx, s.db); schemaErr != nil { + return nil, schemaErr + } + page := &RIExchangeDirectListPage{} + if len(f.Allowed) == 0 { + return page, nil + } + accounts, regions := allowedListArrays(f.Allowed) + riID, region := optionalListText(normaliseRIExchangeSourceIDs([]string{f.RIID})), optionalListText([]string{f.Region}) + afterAt, afterID := listCursorArgs(f.After) + + rows, err := s.db.Query(ctx, ` + SELECT`+riExchangeDirectRowColumns+` + FROM ri_exchange_direct_executions t + WHERE provider = 'aws' AND resolved_at IS NULL + AND status = ANY($1::text[]) + AND EXISTS (SELECT 1 FROM unnest($2::text[], $3::text[]) a(acc, reg) + WHERE t.cloud_account_id IS NOT DISTINCT FROM a.acc AND COALESCE(t.region, '') = a.reg) + AND ($4::text IS NULL OR $4 = ANY(ARRAY(SELECT lower(btrim(x, E' \t\n\r\f\x0B')) FROM unnest(source_ids) x))) + AND ($5::text IS NULL OR region = $5) + AND ($6::timestamptz IS NULL OR (created_at, id) > ($6, $7::uuid)) + ORDER BY created_at, id + LIMIT $8`, + statuses, accounts, regions, riID, region, afterAt, afterID, limit+1) + if err != nil { + return nil, fmt.Errorf("failed to list unresolved ri exchange direct executions: %w", err) + } + defer rows.Close() + for rows.Next() { + r, err := scanRIExchangeDirectRow(rows) + if err != nil { + return nil, fmt.Errorf("failed to scan ri exchange direct execution: %w", err) + } + page.Rows = append(page.Rows, r) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("failed to list unresolved ri exchange direct executions: %w", err) + } + if len(page.Rows) > limit { + page.Rows = page.Rows[:limit] + last := page.Rows[limit-1] + page.Next = &RIExchangeDirectCursor{CreatedAt: last.CreatedAt, ID: last.ID} + } + return page, nil +} + +// validateRIExchangeDirectListFilter applies the limit default and cap and +// checks the status filter. +func validateRIExchangeDirectListFilter(f RIExchangeDirectListFilter) (limit int, statuses []string, err error) { + limit = f.Limit + if limit <= 0 { + limit = RIExchangeDirectListDefaultLimit + } + if limit > RIExchangeDirectListMaxLimit { + return 0, nil, fmt.Errorf("%w: limit must be at most %d", ErrRIExchangeResolveInvalid, RIExchangeDirectListMaxLimit) + } + statuses = f.Statuses + if len(statuses) == 0 { + statuses = []string{RIExchangeDirectStatusSubmitted, RIExchangeDirectStatusUnknown} + } + for _, st := range statuses { + if st != RIExchangeDirectStatusSubmitted && st != RIExchangeDirectStatusUnknown { + return 0, nil, fmt.Errorf("%w: status must be %q or %q", ErrRIExchangeResolveInvalid, RIExchangeDirectStatusSubmitted, RIExchangeDirectStatusUnknown) + } + } + return limit, statuses, nil +} + +// allowedListArrays splits the allowed pairs into the two parallel arrays the +// query unnests. +func allowedListArrays(allowed []RIExchangeDirectAccountRegion) (accounts []*string, regions []string) { + accounts = make([]*string, len(allowed)) + regions = make([]string, len(allowed)) + for i, p := range allowed { + accounts[i], regions[i] = p.CloudAccountID, p.Region + } + return accounts, regions +} + +// listCursorArgs is the keyset cursor as nullable SQL arguments. +func listCursorArgs(after *RIExchangeDirectCursor) (at *time.Time, id *string) { + if after == nil { + return nil, nil + } + return &after.CreatedAt, &after.ID +} + +// optionalListText is the first non-empty value as a nullable SQL argument. +func optionalListText(vals []string) *string { + if len(vals) == 0 || vals[0] == "" { + return nil + } + return &vals[0] +} diff --git a/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go new file mode 100644 index 000000000..399701923 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_list_integration_test.go @@ -0,0 +1,228 @@ +//go:build integration + +package config + +import ( + "sort" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for the #1836 list and get reads. + +// directSpec describes one audit row for the list tests. +type directSpec struct { + provider, status string + account *string + region string + sources []string + createdAt time.Time + nullRegion bool + resolved bool + errText string +} + +// insertDirect writes an audit row exactly as described and returns its id. +func (e *riExchangeIntegrationEnv) insertDirect(t *testing.T, s directSpec) string { + t.Helper() + if s.provider == "" { + s.provider = "aws" + } + if s.status == "" { + s.status = "unknown" + } + var region *string + if s.region != "" || (s.provider == "aws" && !s.nullRegion) { + if s.region == "" { + s.region = "eu-west-1" + } + region = &s.region + } + if len(s.sources) == 0 { + s.sources = []string{"ri-" + uuid.NewString()} + } + if s.createdAt.IsZero() { + s.createdAt = time.Now().UTC().Truncate(time.Microsecond) + } + var errText *string + if s.errText != "" { + errText = &s.errText + } + var id string + require.NoError(t, e.poolA.QueryRow(e.ctx, ` + INSERT INTO ri_exchange_direct_executions + (provider, cloud_account_id, region, source_ids, targets, max_payment_due, max_payment_currency, + status, error, executed_by, created_at, completed_at, + resolved_status, resolved_by, resolved_at, resolution_note) + VALUES ($1, $2, $3, $4, '[{"offering_id":"o","count":1}]'::jsonb, 100, 'USD', + $5, $6, 'tester', $7, CASE WHEN $5 = 'submitted' THEN NULL ELSE $7::timestamptz END, + CASE WHEN $8 THEN 'failed' END, CASE WHEN $8 THEN 'op' END, CASE WHEN $8 THEN now() END, CASE WHEN $8 THEN 'note' END) + RETURNING id::text`, + s.provider, s.account, region, s.sources, s.status, errText, s.createdAt, s.resolved).Scan(&id)) + return id +} + +func rowIDs(rows []RIExchangeDirectRow) []string { + ids := make([]string, len(rows)) + for i, r := range rows { + ids[i] = r.ID + } + return ids +} + +func allow(account *string, region string) []RIExchangeDirectAccountRegion { + return []RIExchangeDirectAccountRegion{{CloudAccountID: account, Region: region}} +} + +// [fail-first] Only unresolved submitted and unknown AWS rows are listed. +func TestPostgresStoreDB_RIExchangeDirectList_OnlyUnresolvedAWSInFlight(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + want := []string{ + env.insertDirect(t, directSpec{status: "submitted", account: acc}), + env.insertDirect(t, directSpec{status: "unknown", account: acc}), + } + env.insertDirect(t, directSpec{provider: "azure", status: "unknown", account: acc, region: "eu-west-1"}) // same account and region, so only the provider predicate excludes it + env.insertDirect(t, directSpec{status: "completed", account: acc}) + env.insertDirect(t, directSpec{status: "failed", account: acc}) + env.insertDirect(t, directSpec{status: "unknown", account: acc, resolved: true}) + env.insertDirect(t, directSpec{status: "submitted", account: acc, resolved: true}) + + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allow(acc, "eu-west-1")}) + require.NoError(t, err) + assert.ElementsMatch(t, want, rowIDs(page.Rows)) + assert.Nil(t, page.Next) +} + +// [fail-first] The status, region and normalised ri_id filters. +func TestPostgresStoreDB_RIExchangeDirectList_Filters(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + sub := env.insertDirect(t, directSpec{status: "submitted", account: acc, sources: []string{"ri-a"}}) + unk := env.insertDirect(t, directSpec{status: "unknown", account: acc, sources: []string{" RI-PADDED ", "ri-b"}}) + other := env.insertDirect(t, directSpec{status: "unknown", account: acc, region: "us-east-1", sources: []string{"ri-c"}}) + allowed := []RIExchangeDirectAccountRegion{{acc, "eu-west-1"}, {acc, "us-east-1"}} + list := func(f RIExchangeDirectListFilter) []string { + f.Allowed = allowed + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, f) + require.NoError(t, err) + ids := rowIDs(page.Rows) + sort.Strings(ids) + return ids + } + sorted := func(ids ...string) []string { sort.Strings(ids); return ids } + + assert.Equal(t, sorted(sub, unk, other), list(RIExchangeDirectListFilter{}), "default is both statuses") + assert.Equal(t, []string{sub}, list(RIExchangeDirectListFilter{Statuses: []string{"submitted"}})) + assert.Equal(t, sorted(unk, other), list(RIExchangeDirectListFilter{Statuses: []string{"unknown"}})) + assert.Equal(t, []string{other}, list(RIExchangeDirectListFilter{Region: "us-east-1"})) + assert.Equal(t, []string{unk}, list(RIExchangeDirectListFilter{RIID: "ri-padded"}), "stored padded upper case matches") + assert.Equal(t, []string{unk}, list(RIExchangeDirectListFilter{RIID: " Ri-B "}), "padded mixed-case request matches") + assert.Empty(t, list(RIExchangeDirectListFilter{RIID: "ri-none"})) + + _, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allowed, Statuses: []string{"completed"}}) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) + _, err = env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allowed, Limit: 201}) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) +} + +// [fail-first] The allowed (account, region) set bounds the page, a NULL +// account matches only a NULL entry, and an empty set lists nothing. +func TestPostgresStoreDB_RIExchangeDirectList_AllowedAccountRegionSet(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + a1, a2 := strPtr("acc-1"), strPtr("acc-2") + r1 := env.insertDirect(t, directSpec{account: a1}) + r2 := env.insertDirect(t, directSpec{account: a2}) + rOtherRegion := env.insertDirect(t, directSpec{account: a1, region: "us-east-1"}) + rNull := env.insertDirect(t, directSpec{account: nil}) + list := func(allowed []RIExchangeDirectAccountRegion) []string { + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allowed}) + require.NoError(t, err) + ids := rowIDs(page.Rows) + sort.Strings(ids) + return ids + } + sorted := func(ids ...string) []string { sort.Strings(ids); return ids } + + assert.Equal(t, []string{r1}, list(allow(a1, "eu-west-1")), "one account, one region") + assert.Equal(t, sorted(r1, r2), list([]RIExchangeDirectAccountRegion{{a1, "eu-west-1"}, {a2, "eu-west-1"}})) + assert.Equal(t, sorted(r1, rOtherRegion), list([]RIExchangeDirectAccountRegion{{a1, "eu-west-1"}, {a1, "us-east-1"}})) + assert.Equal(t, []string{rNull}, list(allow(nil, "eu-west-1")), "a NULL account is listed only for a NULL entry") + assert.Empty(t, list(allow(strPtr("acc-3"), "eu-west-1"))) + assert.Empty(t, list(allow(a1, "ap-south-1")), "right account, wrong region") + assert.Empty(t, list(nil), "an empty set lists nothing") +} + +// [fail-first] Keyset pages are complete and ordered with equal created_at. +func TestPostgresStoreDB_RIExchangeDirectList_KeysetPagination(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + base := time.Now().UTC().Truncate(time.Microsecond).Add(-time.Hour) + all := make([]string, 0, 7) + for _, at := range []time.Time{base, base, base, base, base.Add(time.Second), base.Add(time.Second), base.Add(2 * time.Second)} { + all = append(all, env.insertDirect(t, directSpec{account: acc, createdAt: at})) + } + type key struct { + at time.Time + id string + } + full, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allow(acc, "eu-west-1")}) + require.NoError(t, err) + keys := make([]key, 0, len(full.Rows)) + for _, r := range full.Rows { + keys = append(keys, key{r.CreatedAt, r.ID}) + } + require.Len(t, keys, len(all)) + require.True(t, sort.SliceIsSorted(keys, func(i, j int) bool { + if !keys[i].at.Equal(keys[j].at) { + return keys[i].at.Before(keys[j].at) + } + return keys[i].id < keys[j].id + }), "oldest first, id breaks ties") + + var got []string + var after *RIExchangeDirectCursor + for pages := 0; pages < 10; pages++ { + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, + RIExchangeDirectListFilter{Allowed: allow(acc, "eu-west-1"), Limit: 2, After: after}) + require.NoError(t, err) + got = append(got, rowIDs(page.Rows)...) + if page.Next == nil { + break + } + require.Len(t, page.Rows, 2) + after = page.Next + } + assert.Equal(t, rowIDs(full.Rows), got, "pages concatenate to the full ordered list with no gap or repeat") +} + +// [fail-first] An AWS row with a NULL region is listed for the empty region, +// the same pair the pair read reports for it. +func TestPostgresStoreDB_RIExchangeDirectList_NullRegionRowIsListable(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + acc := strPtr("acc-1") + id := env.insertDirect(t, directSpec{account: acc, nullRegion: true}) + env.insertDirect(t, directSpec{account: acc}) + + page, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Allowed: allow(acc, "")}) + require.NoError(t, err) + require.Equal(t, []string{id}, rowIDs(page.Rows)) + assert.Equal(t, "", page.Rows[0].Region) +} + +// [fail-first] A bad filter is invalid even when nothing is allowed, never an +// empty result. +func TestPostgresStoreDB_RIExchangeDirectList_BadFilterIsInvalidWithEmptyAllowSet(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + for name, f := range map[string]RIExchangeDirectListFilter{ + "limit": {Limit: 201}, + "status": {Statuses: []string{"completed"}}, + } { + _, err := env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, f) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid, name) + } +} diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve.go b/internal/config/store_postgres_ri_exchange_direct_resolve.go new file mode 100644 index 000000000..095944272 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolve.go @@ -0,0 +1,156 @@ +package config + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/jackc/pgx/v5" +) + +// riExchangeDirectResolveRow is the locked row the resolve rules run against. +// dbNow is the database clock after the lock was granted. +type riExchangeDirectResolveRow struct { + status string + createdAt time.Time + completedAt, resolvedAt *time.Time + resolvedStatus, note *string + dbNow time.Time +} + +// ResolveRIExchangeDirectExecution settles an AWS direct row an operator has +// verified at AWS. It records the resolution beside the row's original status +// and completed_at, which are never rewritten, and the in-flight guard then +// stops treating the row as holding its source RIs. One transaction locks the +// row, so two resolves and a concurrent finalize each see the other's commit. +// Every time comparison uses the database clock. +func (s *PostgresStore) ResolveRIExchangeDirectExecution(ctx context.Context, req RIExchangeDirectResolve) (*RIExchangeDirectResolveResult, error) { + if err := validateRIExchangeDirectResolve(req); err != nil { + return nil, err + } + if err := requireDirectResolveSchema(ctx, s.db); err != nil { + return nil, err + } + res := &RIExchangeDirectResolveResult{ID: req.ID, ResolvedStatus: req.Outcome, Note: req.Note} + err := s.WithTx(ctx, func(tx pgx.Tx) error { + row, err := lockRIExchangeDirectForResolve(ctx, tx, req.ID) + if err != nil { + return err + } + if row.resolvedAt != nil { + return replayRIExchangeDirectResolve(req, row, res) + } + if err := checkRIExchangeDirectResolve(req, row); err != nil { + return err + } + return applyRIExchangeDirectResolve(ctx, tx, req, res) + }) + if err != nil { + return nil, err + } + return res, nil +} + +func validateRIExchangeDirectResolve(req RIExchangeDirectResolve) error { + switch { + case req.Outcome != RIExchangeDirectStatusCompleted && req.Outcome != RIExchangeDirectStatusFailed: + return fmt.Errorf("%w: outcome must be %q or %q", ErrRIExchangeResolveInvalid, RIExchangeDirectStatusCompleted, RIExchangeDirectStatusFailed) + case req.ExpectedStatus != RIExchangeDirectStatusSubmitted && req.ExpectedStatus != RIExchangeDirectStatusUnknown: + return fmt.Errorf("%w: expected status must be %q or %q", ErrRIExchangeResolveInvalid, RIExchangeDirectStatusSubmitted, RIExchangeDirectStatusUnknown) + case req.Note == "" || len(req.Note) > RIExchangeDirectResolveNoteMax: + return fmt.Errorf("%w: note must be 1 to %d bytes", ErrRIExchangeResolveInvalid, RIExchangeDirectResolveNoteMax) + case req.CheckedAt.IsZero(): + return fmt.Errorf("%w: the cloud check must name a time", ErrRIExchangeResolveInvalid) + case req.ResolvedBy == "": + return fmt.Errorf("%w: the resolver must be named", ErrRIExchangeResolveInvalid) + } + return nil +} + +func lockRIExchangeDirectForResolve(ctx context.Context, tx pgx.Tx, id string) (riExchangeDirectResolveRow, error) { + var row riExchangeDirectResolveRow + err := tx.QueryRow(ctx, ` + SELECT status, created_at, completed_at, resolved_at, resolved_status, resolution_note, clock_timestamp() + FROM ri_exchange_direct_executions + WHERE id = $1 AND provider = 'aws' + FOR UPDATE`, id, + ).Scan(&row.status, &row.createdAt, &row.completedAt, &row.resolvedAt, &row.resolvedStatus, &row.note, &row.dbNow) + if errors.Is(err, pgx.ErrNoRows) { + return row, ErrRIExchangeResolveNotFound + } + if err != nil { + return row, fmt.Errorf("failed to lock ri exchange direct execution %s: %w", id, err) + } + return row, nil +} + +// replayRIExchangeDirectResolve answers a request that found the row already +// resolved: success only when it holds exactly what this request would have +// written, else a conflict. +func replayRIExchangeDirectResolve(req RIExchangeDirectResolve, row riExchangeDirectResolveRow, res *RIExchangeDirectResolveResult) error { + if row.resolvedStatus != nil && *row.resolvedStatus == req.Outcome && row.note != nil && *row.note == req.Note { + res.Status = row.status + res.ResolvedAt = *row.resolvedAt + res.Replayed = true + return nil + } + return fmt.Errorf("%w: the exchange was already resolved as %s", ErrRIExchangeResolveConflict, derefString(row.resolvedStatus)) +} + +func derefString(s *string) string { + if s == nil { + return "" + } + return *s +} + +func checkRIExchangeDirectResolve(req RIExchangeDirectResolve, row riExchangeDirectResolveRow) error { + switch { + case row.status != RIExchangeDirectStatusSubmitted && row.status != RIExchangeDirectStatusUnknown: + return fmt.Errorf("%w: the exchange is already %s", ErrRIExchangeResolveConflict, row.status) + case row.status != req.ExpectedStatus: + return fmt.Errorf("%w: the exchange is %s, not %s", ErrRIExchangeResolveConflict, row.status, req.ExpectedStatus) + } + var earliest time.Time + if row.status == RIExchangeDirectStatusSubmitted { + earliest = row.createdAt.Add(RIExchangeDirectResolveSubmittedAfter) + if row.dbNow.Before(earliest) { + return fmt.Errorf("%w: the exchange may still be in flight; wait until %s", + ErrRIExchangeResolveConflict, earliest.UTC().Format(time.RFC3339)) + } + } else { + if row.completedAt == nil { + return errors.New("failed to resolve ri exchange direct execution: unknown row has no completed_at") + } + earliest = row.completedAt.Add(RIExchangeDirectResolveUnknownCheckAfter) + } + // Postgres keeps microseconds, so compare the operator's time at that + // precision: a nanosecond tail must neither pass nor fail a bound. + checked := req.CheckedAt.Truncate(time.Microsecond) + switch { + case checked.After(row.dbNow): + return fmt.Errorf("%w: checked_at is in the future", ErrRIExchangeResolveInvalid) + case checked.Before(earliest): + return fmt.Errorf("%w: checked_at must be at or after %s", ErrRIExchangeResolveInvalid, earliest.UTC().Format(time.RFC3339Nano)) + } + return nil +} + +func applyRIExchangeDirectResolve(ctx context.Context, tx pgx.Tx, req RIExchangeDirectResolve, res *RIExchangeDirectResolveResult) error { + err := tx.QueryRow(ctx, ` + UPDATE ri_exchange_direct_executions + SET resolved_status = $2, resolved_by_user_id = $3::uuid, resolved_by = $4, + resolved_at = clock_timestamp(), resolution_note = $5 + WHERE id = $1 AND provider = 'aws' AND resolved_at IS NULL AND status = $6 + RETURNING status, resolved_at`, + req.ID, req.Outcome, req.ResolvedByUserID, req.ResolvedBy, req.Note, req.ExpectedStatus, + ).Scan(&res.Status, &res.ResolvedAt) + if errors.Is(err, pgx.ErrNoRows) { + return fmt.Errorf("%w: the exchange changed while resolving", ErrRIExchangeResolveConflict) + } + if err != nil { + return fmt.Errorf("failed to resolve ri exchange direct execution %s: %w", req.ID, err) + } + return nil +} diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve_boundary_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_resolve_boundary_integration_test.go new file mode 100644 index 000000000..f9ffc28b6 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolve_boundary_integration_test.go @@ -0,0 +1,84 @@ +//go:build integration + +package config + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL boundary coverage for the #1836 resolve: the checked_at +// bounds and the replay variants. + +// [fail-first] checked_at bounds: exactly the bound is accepted and one +// microsecond less is refused, for both statuses and both outcomes; a time in +// the future and one in another zone are judged as instants. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_CheckedAtBounds(t *testing.T) { + for _, outcome := range []string{"completed", "failed"} { + t.Run("submitted_"+outcome, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "submitted", "1 hour", "0") + bound := env.directTimes(t, id).created.Add(RIExchangeDirectResolveSubmittedAfter) + + _, err := env.resolve(t, directResolveReq(id, outcome, "submitted", bound.Add(-time.Microsecond))) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) + _, err = env.resolve(t, directResolveReq(id, outcome, "submitted", time.Now().UTC().Add(time.Hour))) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid, "future") + _, err = env.resolve(t, directResolveReq(id, outcome, "submitted", bound.Add(900*time.Nanosecond))) + require.NoError(t, err, "a nanosecond tail past the bound truncates to the bound and is accepted") + }) + t.Run("unknown_"+outcome, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "unknown", "2 hours", "1 hour") + bound := env.directTimes(t, id).completed.Add(RIExchangeDirectResolveUnknownCheckAfter) + + _, err := env.resolve(t, directResolveReq(id, outcome, "unknown", bound.Add(-time.Microsecond))) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) + zone := time.FixedZone("UTC+14", 14*3600) + _, err = env.resolve(t, directResolveReq(id, outcome, "unknown", bound.In(zone))) + require.NoError(t, err, "exactly the bound, given in another zone") + }) + } +} + +// [fail-first] An unknown row has no age rule, only the check bound: 59s after +// completion is refused, 61s accepted. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_UnknownBound(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + a := env.directAged(t, "unknown", "2 minutes", "2 minutes") + completed := env.directTimes(t, a).completed + + _, err := env.resolve(t, directResolveReq(a, "failed", "unknown", completed.Add(59*time.Second))) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid) + _, err = env.resolve(t, directResolveReq(a, "failed", "unknown", completed.Add(61*time.Second))) + require.NoError(t, err) +} + +// [fail-first] A repeat of the same request replays and writes nothing; a +// different outcome or note conflicts. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_Replay(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "unknown", "2 hours", "1 hour") + req := directResolveReq(id, "failed", "unknown", time.Now().UTC().Add(-time.Minute)) + first, err := env.resolve(t, req) + require.NoError(t, err) + + again, err := env.resolve(t, req) + require.NoError(t, err) + assert.True(t, again.Replayed) + assert.Equal(t, "unknown", again.Status) + assert.True(t, first.ResolvedAt.Equal(again.ResolvedAt), "resolved_at is not rewritten") + + other := req + other.Outcome = "completed" + other.Note = "operator:completed: checked" + _, err = env.resolve(t, other) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) + diffNote := req + diffNote.Note = "a different note" + _, err = env.resolve(t, diffNote) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) +} diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve_concurrent_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_resolve_concurrent_integration_test.go new file mode 100644 index 000000000..a19778a7c --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolve_concurrent_integration_test.go @@ -0,0 +1,155 @@ +//go:build integration + +package config + +import ( + "bytes" + "strings" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for #1836: the resolve against concurrent resolves +// and against a concurrent Finalize, in both orders. + +type lockedBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *lockedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *lockedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// captureLogs routes the package logger to a locked buffer (Finalize runs in a +// goroutine). Not parallel: the logger is process-global. +func captureLogs(t *testing.T) *lockedBuffer { + t.Helper() + out := &lockedBuffer{} + prev := logging.SetOutput(out) + t.Cleanup(func() { logging.SetOutput(prev) }) + return out +} + +// [fail-first] Two concurrent resolves of one row: exactly one wins, and the +// stored resolution is the winner's. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_ConcurrentOneWins(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "unknown", "2 hours", "1 hour") + checked := time.Now().UTC().Add(-time.Minute) + reqs := []RIExchangeDirectResolve{directResolveReq(id, "failed", "unknown", checked), directResolveReq(id, "completed", "unknown", checked)} + stores := []*PostgresStore{env.storeA, env.storeB} + + errs := make([]error, 2) + start := make(chan struct{}) + var wg sync.WaitGroup + for i := range reqs { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + _, errs[i] = stores[i].ResolveRIExchangeDirectExecution(env.ctx, reqs[i]) + }(i) + } + close(start) + wg.Wait() + + winner := -1 + for i, err := range errs { + if err == nil { + require.Equal(t, -1, winner, "two resolves both succeeded") + winner = i + continue + } + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) + } + require.NotEqual(t, -1, winner, "one resolve must win") + var got string + require.NoError(t, env.poolA.QueryRow(env.ctx, `SELECT resolved_status FROM ri_exchange_direct_executions WHERE id = $1`, id).Scan(&got)) + assert.Equal(t, reqs[winner].Outcome, got) +} + +// holdRow opens a transaction that has written the row and not committed. +func (e *riExchangeIntegrationEnv) holdRow(t *testing.T, id, setSQL string) (commit func(), pid int) { + t.Helper() + tx, err := e.poolA.Begin(e.ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = tx.Rollback(e.ctx) }) + tag, err := tx.Exec(e.ctx, `UPDATE ri_exchange_direct_executions SET `+setSQL+` WHERE id = $1`, id) + require.NoError(t, err) + require.EqualValues(t, 1, tag.RowsAffected()) + return func() { require.NoError(t, tx.Commit(e.ctx)) }, testhelpers.BackendPID(e.ctx, t, tx) +} + +// [fail-first] Finalize wins the row lock first: the resolve waits, then sees +// the new status and conflicts. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_FinalizeFirst(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "submitted", "1 hour", "0") + commit, pid := env.holdRow(t, id, `status = 'unknown', completed_at = now()`) + + done := make(chan error, 1) + go func() { + _, err := env.resolve(t, directResolveReq(id, "failed", "submitted", time.Now().UTC().Add(-time.Minute))) + done <- err + }() + if err, answered := testhelpers.AwaitBlockedBy(env.ctx, t, env.poolB, pid, done); answered { + t.Fatalf("resolve answered (%v) while a finalize held the row", err) + } + commit() + err := <-done + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) + assert.Contains(t, err.Error(), "unknown") +} + +// [fail-first] Resolve wins first: Finalize waits on the row lock, then still +// stores the true outcome beside the resolution, and logs when they differ. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_ResolveFirstThenFinalize(t *testing.T) { + for _, tc := range []struct { + name, resolved, finalStatus string + wantLog bool + }{ + {"contradiction is logged", "failed", RIExchangeDirectStatusCompleted, true}, + {"agreement is not", "completed", RIExchangeDirectStatusCompleted, false}, + {"unknown does not contradict", "failed", RIExchangeDirectStatusUnknown, false}, + } { + t.Run(tc.name, func(t *testing.T) { + logs := captureLogs(t) + env := newRIExchangeIntegrationEnv(t) + id := env.directAged(t, "submitted", "1 hour", "0") + commit, pid := env.holdRow(t, id, + `resolved_status = '`+tc.resolved+`', resolved_by = 'op', resolved_at = now(), resolution_note = 'n'`) + + done := make(chan error, 1) + go func() { + done <- env.storeB.FinalizeRIExchangeDirectExecution(env.ctx, id, RIExchangeDirectOutcome{Status: tc.finalStatus}) + }() + if err, answered := testhelpers.AwaitBlockedBy(env.ctx, t, env.poolA, pid, done); answered { + t.Fatalf("finalize answered (%v) while the resolution was uncommitted", err) + } + commit() + require.NoError(t, <-done) + + var status, rs string + require.NoError(t, env.poolA.QueryRow(env.ctx, + `SELECT status, resolved_status FROM ri_exchange_direct_executions WHERE id = $1`, id).Scan(&status, &rs)) + assert.Equal(t, tc.finalStatus, status, "the provider outcome is stored") + assert.Equal(t, tc.resolved, rs, "beside the operator's resolution") + assert.Equal(t, tc.wantLog, strings.Contains(logs.String(), "contradicts operator resolution"), logs.String()) + }) + } +} diff --git a/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go new file mode 100644 index 000000000..e5e4cffdc --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolve_integration_test.go @@ -0,0 +1,124 @@ +//go:build integration + +package config + +import ( + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for the #1836 store resolve. Fixtures set times +// relative to the database clock; the store must compare on that clock. + +// directAged inserts an AWS row aged on the DB clock: created_at is now()-age +// and, for a non-submitted row, completed_at is now()-completedAgo. +func (e *riExchangeIntegrationEnv) directAged(t *testing.T, status, age, completedAgo string) string { + t.Helper() + id := e.directRow(t, "aws", status, "ri-g1836-"+status+"-"+age) + _, err := e.poolA.Exec(e.ctx, ` + UPDATE ri_exchange_direct_executions + SET created_at = now() - ($2::text)::interval, + completed_at = CASE WHEN status = 'submitted' THEN NULL ELSE now() - ($3::text)::interval END + WHERE id = $1`, id, age, completedAgo) + require.NoError(t, err) + return id +} + +type directTimes struct{ created, completed time.Time } + +func (e *riExchangeIntegrationEnv) directTimes(t *testing.T, id string) directTimes { + t.Helper() + var d directTimes + var completed *time.Time + require.NoError(t, e.poolA.QueryRow(e.ctx, + `SELECT created_at, completed_at FROM ri_exchange_direct_executions WHERE id = $1`, id).Scan(&d.created, &completed)) + if completed != nil { + d.completed = *completed + } + return d +} + +func directResolveReq(id, outcome, expected string, checkedAt time.Time) RIExchangeDirectResolve { + return RIExchangeDirectResolve{ID: id, Outcome: outcome, ExpectedStatus: expected, Note: "operator:" + outcome + ": checked", + CheckedAt: checkedAt, ResolvedBy: "ops@example.com"} +} + +func (e *riExchangeIntegrationEnv) resolve(t *testing.T, req RIExchangeDirectResolve) (*RIExchangeDirectResolveResult, error) { + t.Helper() + return e.storeA.ResolveRIExchangeDirectExecution(e.ctx, req) +} + +// [fail-first] A submitted row resolves only after the settle age, measured on +// the DB clock; the original status and completed_at stay, the resolution is +// recorded, and the guard stops blocking. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_SubmittedAgeBoundary(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + young := env.directAged(t, "submitted", "9 minutes", "0") + old := env.directAged(t, "submitted", "10 minutes 1 second", "0") + + _, err := env.resolve(t, directResolveReq(young, "failed", "submitted", time.Now().UTC())) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict) + assert.Contains(t, err.Error(), "wait until") + require.ErrorIs(t, env.reserve(t, testAccountID(836100000001), "ri-g1836-submitted-9 minutes"), ErrRIExchangeSourceInFlight) + + times := env.directTimes(t, old) + uid := "7d444840-9dc0-11d1-b245-5ffdce74fad2" + req := directResolveReq(old, "failed", "submitted", times.created.Add(RIExchangeDirectResolveSubmittedAfter)) + req.ResolvedByUserID = &uid + res, err := env.resolve(t, req) + require.NoError(t, err) + assert.Equal(t, "submitted", res.Status, "the original status is returned unchanged") + assert.Equal(t, "failed", res.ResolvedStatus) + assert.False(t, res.Replayed) + + var status, rs, by, note, userID string + var completedNull bool + require.NoError(t, env.poolA.QueryRow(env.ctx, ` + SELECT status, resolved_status, resolved_by, resolution_note, resolved_by_user_id::text, completed_at IS NULL + FROM ri_exchange_direct_executions WHERE id = $1`, old).Scan(&status, &rs, &by, ¬e, &userID, &completedNull)) + assert.Equal(t, []any{"submitted", "failed", "ops@example.com", req.Note, uid, true}, []any{status, rs, by, note, userID, completedNull}) + require.NoError(t, env.reserve(t, testAccountID(836100000002), "ri-g1836-submitted-10 minutes 1 second"), "resolved row releases its RI") +} + +// [fail-first] Rows that must not resolve: Azure and unknown ids are not found, +// finalized rows and a wrong expected status conflict, bad input is invalid. +func TestPostgresStoreDB_ResolveRIExchangeDirectExecution_Refusals(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + now := time.Now().UTC() + az := env.insertDirect(t, directSpec{provider: "azure"}) + _, err := env.resolve(t, directResolveReq(az, "failed", "unknown", now)) + require.ErrorIs(t, err, ErrRIExchangeResolveNotFound) + _, err = env.resolve(t, directResolveReq("00000000-0000-0000-0000-000000000000", "failed", "unknown", now)) + require.ErrorIs(t, err, ErrRIExchangeResolveNotFound) + + for _, status := range []string{"completed", "failed"} { + id := env.directAged(t, status, "2 hours", "1 hour") + _, err = env.resolve(t, directResolveReq(id, "failed", "unknown", now)) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict, status) + assert.Contains(t, err.Error(), status) + } + + unk := env.directAged(t, "unknown", "2 hours", "1 hour") + _, err = env.resolve(t, directResolveReq(unk, "failed", "submitted", now)) + require.ErrorIs(t, err, ErrRIExchangeResolveConflict, "expected_status mismatch") + + bad := []RIExchangeDirectResolve{ + directResolveReq(unk, "unknown", "unknown", now), + directResolveReq(unk, "failed", "completed", now), + directResolveReq(unk, "failed", "unknown", time.Time{}), + } + noNote := directResolveReq(unk, "failed", "unknown", now) + noNote.Note = "" + longNote := directResolveReq(unk, "failed", "unknown", now) + longNote.Note = strings.Repeat("x", RIExchangeDirectResolveNoteMax+1) + noBy := directResolveReq(unk, "failed", "unknown", now) + noBy.ResolvedBy = "" + for i, req := range append(bad, noNote, longNote, noBy) { + _, err = env.resolve(t, req) + require.ErrorIs(t, err, ErrRIExchangeResolveInvalid, "case %d", i) + } +} diff --git a/internal/config/store_postgres_ri_exchange_direct_resolved_guard_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_resolved_guard_integration_test.go new file mode 100644 index 000000000..0b449947b --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_resolved_guard_integration_test.go @@ -0,0 +1,125 @@ +//go:build integration + +package config + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real-PostgreSQL coverage for #1836: an operator-resolved direct row no +// longer holds its source RIs, on all three guarded paths; an unresolved one +// still does. + +// resolveDirectRow stamps the resolution columns directly, as the resolve +// store method will, without changing status or completed_at. +func (e *riExchangeIntegrationEnv) resolveDirectRow(t *testing.T, id, resolvedStatus string) { + t.Helper() + tag, err := e.poolA.Exec(e.ctx, ` + UPDATE ri_exchange_direct_executions + SET resolved_status = $2, resolved_by = 'operator', resolved_at = now(), resolution_note = 'checked at AWS' + WHERE id = $1`, id, resolvedStatus) + require.NoError(t, err) + require.EqualValues(t, 1, tag.RowsAffected()) +} + +// [fail-first] A resolved submitted or unknown row stops refusing an AUTO +// reservation, while an unresolved row of the same status still refuses. +func TestPostgresStoreDB_ReserveRIExchange_ResolvedDirectRowDoesNotBlock(t *testing.T) { + for _, status := range []string{"submitted", "unknown"} { + for _, resolved := range []string{"completed", "failed"} { + t.Run(status+"_"+resolved, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directRow(t, "aws", status, "ri-g1836-res") + env.directRow(t, "aws", status, "ri-g1836-open") + + err := env.reserve(t, testAccountID(836000000001), "ri-g1836-res") + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "unresolved row blocks") + + env.resolveDirectRow(t, id, resolved) + require.NoError(t, env.reserve(t, testAccountID(836000000002), "ri-g1836-res"), "resolved row releases its RI") + require.ErrorIs(t, env.reserve(t, testAccountID(836000000003), "ri-g1836-open"), ErrRIExchangeSourceInFlight, "another unresolved row still blocks") + }) + } + } +} + +// [fail-first] The approval path releases a resolved row's RI too, and the +// refusal text for a still-unresolved row never names a resolved row. +func TestPostgresStoreDB_ReserveApprovedRIExchange_ResolvedDirectRowDoesNotBlock(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + cap := env.capAbove(t, time.Now().UTC()) + resolved := env.directRow(t, "aws", "unknown", "ri-g1836-appr-res", "ri-g1836-appr-shared") + open := env.directRow(t, "aws", "unknown", "ri-g1836-appr-open", "ri-g1836-appr-shared") + + row := env.processingWith(t, testAccountID(836000000011), "ri-g1836-appr-res", "ri-g1836-appr-open") + defer env.release(t, row.ID) + _, err := env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "both rows unresolved") + + env.resolveDirectRow(t, resolved, "completed") + _, err = env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "the open row still blocks") + assert.Contains(t, err.Error(), "ri-g1836-appr-open") + assert.NotContains(t, err.Error(), "ri-g1836-appr-res", "a resolved row's RI is not named") + assert.NotContains(t, err.Error(), open) + assert.NotContains(t, err.Error(), resolved) + + env.resolveDirectRow(t, open, "failed") + _, err = env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.NoError(t, err, "every holder resolved") +} + +// [fail-first] The direct Save admits a source RI whose holder was resolved, +// and still refuses one whose holder is unresolved. +func TestRIExchangeDirectSave_ResolvedHolderDoesNotBlock(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + held := awsDirectSave("ri-g1836-save") + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, held)) + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-save")), ErrRIExchangeSourceInFlight, "unresolved holder") + + env.resolveDirectRow(t, held.ID, "failed") + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-save")), "resolved holder releases the RI") + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-save")), ErrRIExchangeSourceInFlight, "the new submitted row holds it again") +} + +// [fail-first] The full matrix on the approval path: a submitted or unknown +// row, resolved as completed or as failed, stops blocking; unresolved blocks. +func TestPostgresStoreDB_ReserveApprovedRIExchange_ResolvedMatrix(t *testing.T) { + for _, status := range []string{"submitted", "unknown"} { + for _, resolved := range []string{"completed", "failed"} { + t.Run(status+"_"+resolved, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + cap := env.capAbove(t, time.Now().UTC()) + id := env.directRow(t, "aws", status, "ri-g1836-mx") + row := env.processingWith(t, testAccountID(836000000021), "ri-g1836-mx") + defer env.release(t, row.ID) + + _, err := env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.ErrorIs(t, err, ErrRIExchangeSourceInFlight, "unresolved row blocks") + env.resolveDirectRow(t, id, resolved) + _, err = env.storeA.ReserveApprovedRIExchange(env.ctx, row.ID, cap, cap) + require.NoError(t, err, "resolved row releases its RI") + }) + } + } +} + +// [fail-first] The full matrix on the direct Save path. +func TestRIExchangeDirectSave_ResolvedMatrix(t *testing.T) { + for _, status := range []string{"submitted", "unknown"} { + for _, resolved := range []string{"completed", "failed"} { + t.Run(status+"_"+resolved, func(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + id := env.directRow(t, "aws", status, "ri-g1836-sv") + + require.ErrorIs(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-sv")), ErrRIExchangeSourceInFlight, "unresolved row blocks") + env.resolveDirectRow(t, id, resolved) + require.NoError(t, env.storeA.SaveRIExchangeDirectExecution(env.ctx, awsDirectSave("ri-g1836-sv")), "resolved row releases its RI") + }) + } + } +} diff --git a/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go index 0546f1491..0c62bd21a 100644 --- a/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_runbook_integration_test.go @@ -5,58 +5,133 @@ package config import ( "os" "regexp" + "strconv" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// runbookSettleSQL reads the settle statement from the runbook itself, so the -// operator's copy and the tested statement cannot drift. The :'name' psql -// placeholders become positional parameters. -func runbookSettleSQL(t *testing.T) string { +// runbookBreakGlassSQLRaw reads the last-resort UPDATE from the runbook +// itself, so the operator's copy and the tested statement cannot drift. +func runbookBreakGlassSQLRaw(t *testing.T) string { t.Helper() raw, err := os.ReadFile("../../runbooks/ri-exchange-direct-execution-settle.md") require.NoError(t, err) m := regexp.MustCompile("(?s)```sql\n(-- :'id'[^`]*)```").FindSubmatch(raw) require.NotNil(t, m, "runbook has no sql block") - return strings.NewReplacer(":'new_status'", "$1::text", ":'id'", "$2::uuid", ":'note'", "$3::text").Replace(string(m[1])) + return string(m[1]) } -// #1829 runbook: the settle statement releases only rows it is safe to release. -func TestRIExchangeDirectRunbook_SettleStatement(t *testing.T) { +// runbookBreakGlassSQL turns the :'name' psql placeholders into positional +// parameters. +func runbookBreakGlassSQL(t *testing.T) string { + t.Helper() + return strings.NewReplacer( + ":'outcome'", "$1::text", ":'id'", "$2::uuid", ":'operator'", "$3::text", + ":'note'", "$4::text", ":'checked_at'", "$5::timestamptz", ":'expected_status'", "$6::text", + ).Replace(runbookBreakGlassSQLRaw(t)) +} + +// The intervals in the SQL are the store's constants, not copies that can drift. +func TestRIExchangeDirectRunbook_IntervalsMatchStore(t *testing.T) { + sql := runbookBreakGlassSQL(t) + minutes := func(re string) []time.Duration { + matches := regexp.MustCompile(re).FindAllStringSubmatch(sql, -1) + out := make([]time.Duration, 0, len(matches)) + for _, m := range matches { + n, err := strconv.Atoi(m[1]) + require.NoError(t, err) + out = append(out, time.Duration(n)*time.Minute) + } + return out + } + assert.Equal(t, []time.Duration{RIExchangeDirectResolveSubmittedAfter, RIExchangeDirectResolveSubmittedAfter}, + minutes(`interval '(\d+) minutes'`), "both submitted-age intervals equal the store constant") + assert.Equal(t, []time.Duration{RIExchangeDirectResolveUnknownCheckAfter}, + minutes(`interval '(\d+) minute'`), "the unknown check interval equals the store constant") +} + +// The note limit in the SQL is the store's constant. +func TestRIExchangeDirectRunbook_NoteLimitMatchesStore(t *testing.T) { + m := regexp.MustCompile(`octet_length\(v\.n\) <= (\d+)`).FindStringSubmatch(runbookBreakGlassSQLRaw(t)) + require.NotNil(t, m, "runbook SQL has no note length guard") + n, err := strconv.Atoi(m[1]) + require.NoError(t, err) + assert.Equal(t, RIExchangeDirectResolveNoteMax, n) +} + +// The last-resort statement resolves only rows the store would resolve, under +// the same age and checked_at rules, and a resolved row stops blocking. +func TestRIExchangeDirectRunbook_BreakGlassStatement(t *testing.T) { env := newRIExchangeIntegrationEnv(t) - sql := runbookSettleSQL(t) - age := func(id, interval string) { - _, err := env.poolA.Exec(env.ctx, "UPDATE ri_exchange_direct_executions SET created_at = now() - $2::interval WHERE id = $1", id, interval) + sql := runbookBreakGlassSQL(t) + // Fixtures are relative to the database clock; the checked_at values are + // computed in SQL from the row's own times, never from Go time. + checkedAt := func(id, offset string) time.Time { + var at time.Time + require.NoError(t, env.poolA.QueryRow(env.ctx, ` + SELECT CASE WHEN status = 'submitted' THEN created_at + interval '10 minutes' ELSE completed_at + interval '1 minute' END + + ($2::text)::interval + FROM ri_exchange_direct_executions WHERE id = $1`, id, offset).Scan(&at)) + return at + } + agedFor := func(provider, status, ri, createdAgo, completedAgo string) string { + id := env.directRow(t, provider, status, ri) + _, err := env.poolA.Exec(env.ctx, ` + UPDATE ri_exchange_direct_executions + SET created_at = now() - ($2::text)::interval, + completed_at = CASE WHEN status = 'submitted' THEN NULL ELSE now() - ($3::text)::interval END + WHERE id = $1`, id, createdAgo, completedAgo) require.NoError(t, err) + return id } - settle := func(id string) int64 { - tag, err := env.poolA.Exec(env.ctx, sql, "failed", id, "checked at AWS") + aged := func(status, ri, createdAgo, completedAgo string) string { + return agedFor("aws", status, ri, createdAgo, completedAgo) + } + settleWith := func(id, outcome, expected, operator, note string, at time.Time) int64 { + tag, err := env.poolA.Exec(env.ctx, sql, outcome, id, operator, note, at, expected) require.NoError(t, err) return tag.RowsAffected() } + settle := func(id, outcome string, at time.Time) int64 { + var status string + require.NoError(t, env.poolA.QueryRow(env.ctx, "SELECT status FROM ri_exchange_direct_executions WHERE id = $1", id).Scan(&status)) + return settleWith(id, outcome, status, "operator-a", "checked at AWS", at) + } + + unknown := aged("unknown", "ri-g1836-rb-unknown", "2 hours", "1 hour") + assert.EqualValues(t, 0, settle(unknown, "failed", checkedAt(unknown, "-1 microsecond")), "unknown: checked_at just before completed_at + 1 minute") + assert.EqualValues(t, 0, settle(unknown, "failed", checkedAt(unknown, "10 hours")), "unknown: checked_at in the future") + require.ErrorIs(t, env.reserve(t, testAccountID(850000000001), "ri-g1836-rb-unknown"), ErrRIExchangeSourceInFlight, "an unresolved row keeps blocking") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "submitted", "operator-a", "checked at AWS", checkedAt(unknown, "0")), "a status other than the one seen is not resolved") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", "operator-a", strings.Repeat("x", RIExchangeDirectResolveNoteMax+1), checkedAt(unknown, "0")), "a note over the store limit is not stored") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", "operator-a", strings.Repeat("x", RIExchangeDirectResolveNoteMax), checkedAt(unknown, "0")), "a note that overflows only with the checked_at prefix is not stored") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", "operator-a", "", checkedAt(unknown, "0")), "an empty note is not stored") + assert.EqualValues(t, 0, settleWith(unknown, "failed", "unknown", " ", "checked at AWS", checkedAt(unknown, "0")), "an empty operator is not recorded") + bound := checkedAt(unknown, "0") + assert.EqualValues(t, 1, settle(unknown, "failed", bound), "unknown: exactly completed_at + 1 minute") + assert.EqualValues(t, 0, settle(unknown, "failed", checkedAt(unknown, "0")), "a resolved row is not resolved twice") + var status, resolved, by, note string + var userID *string + require.NoError(t, env.poolA.QueryRow(env.ctx, ` + SELECT status, resolved_status, resolved_by, resolution_note, resolved_by_user_id::text + FROM ri_exchange_direct_executions WHERE id = $1`, unknown).Scan(&status, &resolved, &by, ¬e, &userID)) + assert.Equal(t, []any{"unknown", "failed", "operator-a", "break-glass checked_at=" + bound.UTC().Format("2006-01-02T15:04:05.000000Z") + " note=checked at AWS", (*string)(nil)}, []any{status, resolved, by, note, userID}) + require.NoError(t, env.reserve(t, testAccountID(850000000001), "ri-g1836-rb-unknown"), "a resolved row stops blocking the guard") + + old := aged("submitted", "ri-g1836-rb-old", "11 minutes", "0") + assert.EqualValues(t, 0, settle(old, "completed", checkedAt(old, "-1 microsecond")), "submitted: checked_at before created_at + 10 minutes") + assert.EqualValues(t, 1, settle(old, "completed", checkedAt(old, "0")), "submitted older than 10 minutes, checked at exactly the bound") + + young := aged("submitted", "ri-g1836-rb-young", "6 minutes", "0") + assert.EqualValues(t, 0, settle(young, "failed", checkedAt(young, "0")), "submitted inside the 10 minute window") - unknown := env.directRow(t, "aws", "unknown", "ri-g1829-rb-unknown") - age(unknown, "1 minute") - assert.EqualValues(t, 1, settle(unknown), "a fresh unknown row is settled") - assert.EqualValues(t, 0, settle(unknown), "a settled row is not settled twice") - var status, errText string - require.NoError(t, env.poolA.QueryRow(env.ctx, "SELECT status, error FROM ri_exchange_direct_executions WHERE id = $1", unknown).Scan(&status, &errText)) - assert.Equal(t, "failed", status) - assert.Contains(t, errText, "checked at AWS") - require.NoError(t, env.reserve(t, testAccountID(850000000001), "ri-g1829-rb-unknown"), "a settled row stops blocking the guard") - - old := env.directRow(t, "aws", "submitted", "ri-g1829-rb-old") - age(old, "11 minutes") - assert.EqualValues(t, 1, settle(old), "a submitted row older than 10 minutes is settled") - - young := env.directRow(t, "aws", "submitted", "ri-g1829-rb-young") - age(young, "6 minutes") - assert.EqualValues(t, 0, settle(young), "a submitted row inside the 10 minute window is not settled") - - assert.EqualValues(t, 0, settle(env.directRow(t, "azure", "unknown", "ri-g1829-rb-azure")), "Azure rows are untouched") - assert.EqualValues(t, 0, settle(env.directRow(t, "aws", "completed", "ri-g1829-rb-done")), "a completed row is final") - assert.EqualValues(t, 0, settle(env.directRow(t, "aws", "failed", "ri-g1829-rb-failed")), "a failed row is final") + // Old enough and checked late enough that only the provider test refuses it. + azure := agedFor("azure", "unknown", "ri-g1836-rb-azure", "2 hours", "1 hour") + assert.EqualValues(t, 0, settle(azure, "failed", checkedAt(azure, "0")), "Azure rows are untouched") + done := env.directRow(t, "aws", "completed", "ri-g1836-rb-done") + assert.EqualValues(t, 0, settle(done, "failed", time.Now().UTC().Add(-time.Minute)), "a completed row is final") } diff --git a/internal/config/store_postgres_ri_exchange_direct_schema_integration_test.go b/internal/config/store_postgres_ri_exchange_direct_schema_integration_test.go new file mode 100644 index 000000000..468bb4597 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_schema_integration_test.go @@ -0,0 +1,41 @@ +//go:build integration + +package config + +import ( + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The app deploys before migration 163 is applied (#1836): the guard and +// finalize must keep working without the resolution columns, and the operator +// paths must refuse rather than fail on a missing column. +func TestPostgresStoreDB_RIExchangeDirect_BeforeResolutionMigration(t *testing.T) { + env := newRIExchangeIntegrationEnv(t) + require.NoError(t, migrations.MigrateToVersion(env.ctx, env.poolA, getTestMigrationsPath(), migrationBelow(t, directResolveMigrationVersion))) + var cols int + require.NoError(t, env.poolA.QueryRow(env.ctx, `SELECT count(*) FROM information_schema.columns WHERE table_name = 'ri_exchange_direct_executions' AND column_name = 'resolved_at'`).Scan(&cols)) + require.Zero(t, cols, "precondition: the resolution columns are gone") + + held := env.directRow(t, "aws", "submitted", "ri-g1836-n1-held") + require.ErrorIs(t, env.reserve(t, testAccountID(836200000001), "ri-g1836-n1-held"), ErrRIExchangeSourceInFlight, "the guard still holds the RI") + require.NoError(t, env.reserve(t, testAccountID(836200000002), "ri-g1836-n1-free")) + + outcome := RIExchangeDirectOutcome{Status: RIExchangeDirectStatusCompleted, ProviderReference: "riex-1", + NetPayable: strPtr("40.25"), NetPayableCurrency: "USD"} + require.NoError(t, env.storeA.FinalizeRIExchangeDirectExecution(env.ctx, held, outcome)) + require.ErrorIs(t, env.storeA.FinalizeRIExchangeDirectExecution(env.ctx, held, outcome), ErrRIExchangeDirectExecutionNotSubmitted) + + _, err := env.storeA.ListUnresolvedRIExchangeDirectAccountRegions(env.ctx) + assert.ErrorIs(t, err, ErrRIExchangeDirectResolveNotReady) + _, err = env.storeA.ListUnresolvedRIExchangeDirectExecutions(env.ctx, RIExchangeDirectListFilter{Limit: 10}) + assert.ErrorIs(t, err, ErrRIExchangeDirectResolveNotReady) + _, err = env.storeA.GetRIExchangeDirectExecution(env.ctx, held) + assert.ErrorIs(t, err, ErrRIExchangeDirectResolveNotReady) + _, err = env.resolve(t, directResolveReq(held, "failed", "submitted", time.Now().UTC())) + assert.ErrorIs(t, err, ErrRIExchangeDirectResolveNotReady) +} diff --git a/internal/config/store_postgres_ri_exchange_direct_schema_test.go b/internal/config/store_postgres_ri_exchange_direct_schema_test.go new file mode 100644 index 000000000..76ce4e226 --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_direct_schema_test.go @@ -0,0 +1,145 @@ +package config + +import ( + "bytes" + "context" + "errors" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/pashagolub/pgxmock/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func expectDirectResolveVersion(mock pgxmock.PgxPoolIface, version int) { + mock.ExpectQuery("SELECT version, dirty FROM schema_migrations"). + WillReturnRows(pgxmock.NewRows([]string{"version", "dirty"}).AddRow(version, false)) +} + +// Before migration 163 the guard and finalize must not name resolved_at or +// resolved_status (#1836); the queries with it are pinned by the neighboring tests. +func TestPGXMock_DirectResolveBeforeMigration_QueriesOmitResolutionColumns(t *testing.T) { + notReady := directResolveMigrationVersion - 1 + + t.Run("source guard", func(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + rec := validDirectExecution() + rec.Provider = "aws" + mock.ExpectBegin() + mock.ExpectExec("SELECT pg_advisory_xact_lock").WithArgs(riExchangeDailySpendLockKey). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) + mock.ExpectQuery("SELECT id::text, ARRAY").WithArgs(anyArgsCfg(2)...). + WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) + expectDirectResolveVersion(mock, notReady) + mock.ExpectQuery(`status IN \('submitted', 'unknown'\)\s*\) d`).WithArgs([]string{"res-1"}). + WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) + mock.ExpectExec("INSERT INTO ri_exchange_direct_executions").WithArgs(anyArgsCfg(15)...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + require.NoError(t, store.SaveRIExchangeDirectExecution(context.Background(), rec)) + assert.NoError(t, mock.ExpectationsWereMet()) + }) + + outcome := RIExchangeDirectOutcome{Status: RIExchangeDirectStatusCompleted, ProviderReference: "riex-1"} + t.Run("finalize", func(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + expectDirectResolveVersion(mock, notReady) + mock.ExpectExec(`UPDATE ri_exchange_direct_executions(.|\n)*AND status = 'submitted'$`).WithArgs(anyArgsCfg(7)...). + WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + require.NoError(t, store.FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome)) + assert.NoError(t, mock.ExpectationsWereMet()) + }) + + t.Run("finalize of a finalized row is refused", func(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + expectDirectResolveVersion(mock, notReady) + mock.ExpectExec("UPDATE ri_exchange_direct_executions").WithArgs(anyArgsCfg(7)...). + WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + err := store.FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome) + require.ErrorIs(t, err, ErrRIExchangeDirectExecutionNotSubmitted) + assert.NoError(t, mock.ExpectationsWereMet()) + }) + + t.Run("list, get and resolve refuse before touching the table", func(t *testing.T) { + for name, call := range map[string]func(*PostgresStore) error{ + "account regions": func(s *PostgresStore) error { + _, err := s.ListUnresolvedRIExchangeDirectAccountRegions(context.Background()) + return err + }, + "list": func(s *PostgresStore) error { + _, err := s.ListUnresolvedRIExchangeDirectExecutions(context.Background(), RIExchangeDirectListFilter{Limit: 10}) + return err + }, + "get": func(s *PostgresStore) error { + _, err := s.GetRIExchangeDirectExecution(context.Background(), directExecTestID) + return err + }, + "resolve": func(s *PostgresStore) error { + _, err := s.ResolveRIExchangeDirectExecution(context.Background(), RIExchangeDirectResolve{ID: directExecTestID, Outcome: "failed", ExpectedStatus: "submitted", Note: "n", CheckedAt: time.Now(), ResolvedBy: "ops"}) + return err + }, + } { + t.Run(name, func(t *testing.T) { + mock := newMock(t) + expectDirectResolveVersion(mock, notReady) + require.ErrorIs(t, call(storeWith(mock)), ErrRIExchangeDirectResolveNotReady) + assert.NoError(t, mock.ExpectationsWereMet()) + }) + } + }) +} + +// The constant must name this change's migration file, so a renumbering that +// forgets it fails here instead of failing closed in production. +func TestDirectResolveMigrationVersionNamesTheMigrationFile(t *testing.T) { + name := fmt.Sprintf("%06d_ri_exchange_direct_resolution.up.sql", directResolveMigrationVersion) + _, err := os.Stat(filepath.Join("..", "database", "postgres", "migrations", name)) + require.NoError(t, err, "directResolveMigrationVersion no longer matches the migration file") +} + +// A failed version read must not lose the provider outcome. +func TestPGXMock_FinalizeDirectExecution_UnreadableSchemaVersionStillFinalizes(t *testing.T) { + mock := newMock(t) + mock.ExpectQuery("SELECT version, dirty FROM schema_migrations").WillReturnError(errors.New("connection reset")) + mock.ExpectExec("UPDATE ri_exchange_direct_executions").WithArgs(anyArgsCfg(7)...). + WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + outcome := RIExchangeDirectOutcome{Status: RIExchangeDirectStatusCompleted, ProviderReference: "riex-1"} + require.NoError(t, storeWith(mock).FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome)) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// A failed version read in the guard refuses the reservation: reading it as +// "not ready" would drop the resolved_at filter and reading it as "ready" would +// name a column that may not exist. Neither may look like a held source RI. +func TestPGXMock_DirectGuard_UnreadableSchemaVersionRefuses(t *testing.T) { + var logs bytes.Buffer + prev := logging.SetOutput(&logs) + t.Cleanup(func() { logging.SetOutput(prev) }) + + mock := newMock(t) + rec := validDirectExecution() + rec.Provider = "aws" + mock.ExpectBegin() + mock.ExpectExec("SELECT pg_advisory_xact_lock").WithArgs(riExchangeDailySpendLockKey). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) + mock.ExpectQuery("SELECT id::text, ARRAY").WithArgs(anyArgsCfg(2)...). + WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) + mock.ExpectQuery("SELECT version, dirty FROM schema_migrations").WillReturnError(errors.New("connection reset")) + mock.ExpectRollback() + + err := storeWith(mock).SaveRIExchangeDirectExecution(context.Background(), rec) + + require.ErrorContains(t, err, "failed to check RI exchange source RIs held by a direct execute") + assert.NotErrorIs(t, err, ErrRIExchangeSourceInFlight) + assert.Contains(t, logs.String(), "schema version check failed", "the refusal comes from the version read, not from the blocker query") + assert.NotContains(t, logs.String(), "source RI check failed") + assert.NoError(t, mock.ExpectationsWereMet()) +} diff --git a/internal/config/store_postgres_ri_exchange_direct_test.go b/internal/config/store_postgres_ri_exchange_direct_test.go index a0513f2a7..94bfb6672 100644 --- a/internal/config/store_postgres_ri_exchange_direct_test.go +++ b/internal/config/store_postgres_ri_exchange_direct_test.go @@ -70,6 +70,7 @@ func TestPGXMock_SaveRIExchangeDirectExecution_AWSLocksAndGuardsBeforeInsert(t * WillReturnResult(pgxmock.NewResult("SELECT", 1)) mock.ExpectQuery("SELECT id::text, ARRAY").WithArgs([]string{"RI-1", "ri-1"}, (*string)(nil)). WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) + expectDirectResolveVersion(mock, directResolveMigrationVersion) mock.ExpectQuery("FROM ri_exchange_direct_executions").WithArgs([]string{"ri-1"}). WillReturnRows(pgxmock.NewRows([]string{"id", "shared"})) mock.ExpectExec("INSERT INTO ri_exchange_direct_executions").WithArgs(anyArgsCfg(15)...). @@ -135,9 +136,10 @@ func TestPGXMock_FinalizeRIExchangeDirectExecution_CompareAndSet(t *testing.T) { t.Run("submitted row is finalized", func(t *testing.T) { mock := newMock(t) store := storeWith(mock) - mock.ExpectExec(`UPDATE ri_exchange_direct_executions(.|\n)*WHERE id = \$1(.|\n)*AND status = 'submitted'`). + expectDirectResolveVersion(mock, directResolveMigrationVersion) + mock.ExpectQuery(`UPDATE ri_exchange_direct_executions(.|\n)*WHERE id = \$1(.|\n)*AND status = 'submitted'(.|\n)*RETURNING resolved_status`). WithArgs(directExecTestID, "completed", pgxmock.AnyArg(), outcome.NetPayable, pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg()). - WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + WillReturnRows(pgxmock.NewRows([]string{"resolved_status"}).AddRow((*string)(nil))) require.NoError(t, store.FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome)) assert.NoError(t, mock.ExpectationsWereMet()) }) @@ -145,9 +147,10 @@ func TestPGXMock_FinalizeRIExchangeDirectExecution_CompareAndSet(t *testing.T) { t.Run("finalized or missing row is refused", func(t *testing.T) { mock := newMock(t) store := storeWith(mock) - mock.ExpectExec("UPDATE ri_exchange_direct_executions"). + expectDirectResolveVersion(mock, directResolveMigrationVersion) + mock.ExpectQuery("UPDATE ri_exchange_direct_executions"). WithArgs(anyArgsCfg(7)...). - WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + WillReturnRows(pgxmock.NewRows([]string{"resolved_status"})) err := store.FinalizeRIExchangeDirectExecution(context.Background(), directExecTestID, outcome) require.ErrorIs(t, err, ErrRIExchangeDirectExecutionNotSubmitted) assert.NoError(t, mock.ExpectationsWereMet()) diff --git a/internal/config/store_postgres_schema_version.go b/internal/config/store_postgres_schema_version.go index 731feba5d..2203bedec 100644 --- a/internal/config/store_postgres_schema_version.go +++ b/internal/config/store_postgres_schema_version.go @@ -12,6 +12,11 @@ import ( // columns to purchase_executions (issue #999). const settlementMigrationVersion = 155 +// directResolveMigrationVersion is the migration that adds the resolution +// columns to ri_exchange_direct_executions (#1836). It mirrors +// directResolutionMigration in internal/database/postgres/migrations. +const directResolveMigrationVersion = 163 + // rowQuerier is satisfied by both the store's connection and a pgx.Tx, so a // schema check can run inside the caller's transaction. type rowQuerier interface { @@ -45,3 +50,28 @@ func settlementSchemaReady(ctx context.Context, q rowQuerier) (bool, error) { } return version > settlementMigrationVersion || (version == settlementMigrationVersion && !dirty), nil } + +// directResolveSchemaReady reports whether the direct-exchange resolution +// columns exist. Same N-1 rule as settlementSchemaReady: until migration 163 is +// applied the guard and finalize drop their resolved_at references and the +// list, get and resolve paths answer ErrRIExchangeDirectResolveNotReady. +func directResolveSchemaReady(ctx context.Context, q rowQuerier) (bool, error) { + version, dirty, found, err := readSchemaVersion(ctx, q) + if err != nil || !found { + return false, err + } + return version > directResolveMigrationVersion || (version == directResolveMigrationVersion && !dirty), nil +} + +// requireDirectResolveSchema fails with ErrRIExchangeDirectResolveNotReady +// while the resolution columns are missing. +func requireDirectResolveSchema(ctx context.Context, q rowQuerier) error { + ready, err := directResolveSchemaReady(ctx, q) + if err != nil { + return err + } + if !ready { + return ErrRIExchangeDirectResolveNotReady + } + return nil +} diff --git a/internal/config/store_postgres_schema_version_test.go b/internal/config/store_postgres_schema_version_test.go index dc51d7474..045c5123a 100644 --- a/internal/config/store_postgres_schema_version_test.go +++ b/internal/config/store_postgres_schema_version_test.go @@ -53,3 +53,28 @@ func TestSettlementSchemaReady(t *testing.T) { }) } } + +// The direct-resolve paths run before migration 163 is applied (#1836). +func TestDirectResolveSchemaReady(t *testing.T) { + t.Parallel() + v := directResolveMigrationVersion + cases := []struct { + name string + row fakeVersionRow + want bool + }{ + {"nothing applied", fakeVersionRow{err: pgx.ErrNoRows}, false}, + {"before the migration", fakeVersionRow{version: v - 1}, false}, + {"at the migration", fakeVersionRow{version: v}, true}, + {"at the migration, dirty", fakeVersionRow{version: v, dirty: true}, false}, + {"after it, even with a later dirty migration", fakeVersionRow{version: v + 1, dirty: true}, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := directResolveSchemaReady(context.Background(), fakeVersionQuerier{tc.row}) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} diff --git a/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.down.sql b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.down.sql new file mode 100644 index 000000000..b7c75ae1e --- /dev/null +++ b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.down.sql @@ -0,0 +1,10 @@ +-- Reverse 000163: drop the operator-resolution columns (issue #1836). +-- Lossy by design: resolutions are dropped, so resolved rows that are still +-- submitted or unknown block their source RIs again, the fail-safe direction. +ALTER TABLE ri_exchange_direct_executions + DROP CONSTRAINT IF EXISTS ri_exchange_direct_executions_resolution_all_or_none, + DROP COLUMN IF EXISTS resolved_status, + DROP COLUMN IF EXISTS resolved_by_user_id, + DROP COLUMN IF EXISTS resolved_by, + DROP COLUMN IF EXISTS resolved_at, + DROP COLUMN IF EXISTS resolution_note; diff --git a/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.up.sql b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.up.sql new file mode 100644 index 000000000..092ac4fa0 --- /dev/null +++ b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution.up.sql @@ -0,0 +1,24 @@ +-- Operator resolution of a stuck AWS direct RI exchange row (issue #1836). +-- The original status and completed_at are never rewritten; the operator's +-- verdict is recorded beside them, and a resolved row stops holding its +-- source RIs (the guard adds `resolved_at IS NULL`). +ALTER TABLE ri_exchange_direct_executions + ADD COLUMN IF NOT EXISTS resolved_status TEXT + CONSTRAINT ri_exchange_direct_executions_resolved_status_check + CHECK (resolved_status IN ('completed', 'failed')), + ADD COLUMN IF NOT EXISTS resolved_by_user_id UUID, + ADD COLUMN IF NOT EXISTS resolved_by TEXT, + ADD COLUMN IF NOT EXISTS resolved_at TIMESTAMPTZ, + ADD COLUMN IF NOT EXISTS resolution_note TEXT; + +-- A resolution is whole or absent. resolved_by_user_id stays optional (a +-- break-glass SQL settle or an API key has no user id) but never exists +-- without a resolution. +ALTER TABLE ri_exchange_direct_executions + DROP CONSTRAINT IF EXISTS ri_exchange_direct_executions_resolution_all_or_none; +ALTER TABLE ri_exchange_direct_executions + ADD CONSTRAINT ri_exchange_direct_executions_resolution_all_or_none + CHECK ((resolved_status IS NULL) = (resolved_by IS NULL) + AND (resolved_status IS NULL) = (resolved_at IS NULL) + AND (resolved_status IS NULL) = (resolution_note IS NULL) + AND (resolved_by_user_id IS NULL OR resolved_status IS NOT NULL)); diff --git a/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution_test.go b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution_test.go new file mode 100644 index 000000000..7f1ac67a8 --- /dev/null +++ b/internal/database/postgres/migrations/000163_ri_exchange_direct_resolution_test.go @@ -0,0 +1,126 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// directResolutionMigration is this migration's version. A renumber at merge +// changes this constant and the two file names, nothing else. +const directResolutionMigration uint = 163 + +var directResolutionColumns = []string{"resolved_status", "resolved_by_user_id", "resolved_by", "resolved_at", "resolution_note"} + +func directResolutionColumnState(t *testing.T, ctx context.Context, pool *pgxpool.Pool) map[string]bool { + t.Helper() + rows, err := pool.Query(ctx, ` + SELECT column_name, is_nullable = 'YES' FROM information_schema.columns + WHERE table_name = 'ri_exchange_direct_executions' AND column_name = ANY($1)`, directResolutionColumns) + require.NoError(t, err) + defer rows.Close() + nullable := map[string]bool{} + for rows.Next() { + var name string + var isNullable bool + require.NoError(t, rows.Scan(&name, &isNullable)) + nullable[name] = isNullable + } + require.NoError(t, rows.Err()) + return nullable +} + +// insertDirectRow inserts an unknown AWS row with the given extra columns and +// their SQL literal values. +func insertDirectRow(ctx context.Context, pool *pgxpool.Pool, cols, vals string) error { + _, err := pool.Exec(ctx, ` + INSERT INTO ri_exchange_direct_executions + (provider, source_ids, targets, max_payment_due, max_payment_currency, executed_by, status, completed_at`+cols+`) + VALUES ('aws', ARRAY['ri-1'], '[{"offering_id":"o"}]'::jsonb, 100, 'USD', 'alice@example.com', 'unknown', now()`+vals+`)`) + return err +} + +// TestMigration_RIExchangeDirectResolution pins the #1836 migration against a +// real PostgreSQL: five nullable columns appear, a resolution must be whole or +// absent, resolved_status is limited to completed or failed, the up statement +// is idempotent, and the down migration drops the columns. +func TestMigration_RIExchangeDirectResolution(t *testing.T) { + t.Parallel() + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container := testhelpers.RequireEmptyDatabase(ctx, t) + pool := container.DB.Pool() + + prev := previousMigrationVersion(t, directResolutionMigration) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + require.Empty(t, directResolutionColumnState(t, ctx, pool), "columns must not exist before the migration") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, directResolutionMigration)) + state := directResolutionColumnState(t, ctx, pool) + require.Len(t, state, len(directResolutionColumns)) + for _, c := range directResolutionColumns { + assert.True(t, state[c], "%s must be nullable", c) + } + + const whole = ", resolved_status, resolved_by, resolved_at, resolution_note" + // An unresolved row (all NULL) and whole resolutions are accepted. + require.NoError(t, insertDirectRow(ctx, pool, "", "")) + require.NoError(t, insertDirectRow(ctx, pool, whole, ", 'failed', 'ops', now(), 'note'")) + require.NoError(t, insertDirectRow(ctx, pool, whole+", resolved_by_user_id", + ", 'completed', 'ops', now(), 'note', '00000000-0000-0000-0000-000000000001'")) + + // Each partial resolution is refused. + for name, c := range map[string]struct{ cols, vals string }{ + "status only": {", resolved_status", ", 'failed'"}, + "no note": {", resolved_status, resolved_by, resolved_at", ", 'failed', 'ops', now()"}, + "no resolved_at": {", resolved_status, resolved_by, resolution_note", ", 'failed', 'ops', 'n'"}, + "no resolved_by": {", resolved_status, resolved_at, resolution_note", ", 'failed', now(), 'n'"}, + "no status": {", resolved_by, resolved_at, resolution_note", ", 'ops', now(), 'n'"}, + "note only": {", resolution_note", ", 'n'"}, + "user id only (no resolution)": {", resolved_by_user_id", ", '00000000-0000-0000-0000-000000000001'"}, + } { + err := insertDirectRow(ctx, pool, c.cols, c.vals) + require.Error(t, err, name) + assert.Contains(t, err.Error(), "ri_exchange_direct_executions_resolution_all_or_none", name) + } + + // A whole resolution with a verdict outside completed/failed is refused. + for _, bad := range []string{"unknown", "submitted", ""} { + err := insertDirectRow(ctx, pool, whole, fmt.Sprintf(", '%s', 'ops', now(), 'n'", bad)) + require.Error(t, err, "resolved_status %q", bad) + assert.Contains(t, err.Error(), "ri_exchange_direct_executions_resolved_status_check", "resolved_status %q", bad) + } + + up, err := os.ReadFile(filepath.Join(migrationsPath, fmt.Sprintf("%06d_ri_exchange_direct_resolution.up.sql", directResolutionMigration))) + require.NoError(t, err) + _, err = pool.Exec(ctx, string(up)) + require.NoError(t, err, "the up statement must be idempotent") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + assert.Empty(t, directResolutionColumnState(t, ctx, pool), "down migration must drop the columns") + var rows int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM ri_exchange_direct_executions`).Scan(&rows)) + assert.Equal(t, 3, rows, "down is lossy for the columns only, never the rows") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, directResolutionMigration)) + state = directResolutionColumnState(t, ctx, pool) + require.Len(t, state, len(directResolutionColumns), "up again restores the columns") + err = insertDirectRow(ctx, pool, ", resolved_status", ", 'failed'") + require.Error(t, err, "up again restores the all-or-none CHECK") + assert.Contains(t, err.Error(), "ri_exchange_direct_executions_resolution_all_or_none") + err = insertDirectRow(ctx, pool, whole, ", 'unknown', 'ops', now(), 'n'") + require.Error(t, err, "up again restores the enum CHECK") + assert.Contains(t, err.Error(), "ri_exchange_direct_executions_resolved_status_check") +} diff --git a/known-issues.md b/known-issues.md index dd115a3b1..64be58531 100644 --- a/known-issues.md +++ b/known-issues.md @@ -145,11 +145,14 @@ AUTO and approved exchanges share, so repeated direct executes can spend past it The in-flight source-RI guard only stops two exchanges taking the same RI; it does not cap total spend. -### No operator resolve endpoint for direct RI exchange rows (issue #1829 follow-up) - -A direct row left `submitted` or `unknown` keeps its source RIs blocked until a DBA -settles it with the SQL in `runbooks/ri-exchange-direct-execution-settle.md`. A -resolve endpoint (and a list of unresolved direct rows) is tracked in #1836. +### Direct finalize reads the schema version outside its statement (issue #1836) + +`FinalizeRIExchangeDirectExecution` reads the schema version before its UPDATE, +not in the same statement. If migration 163 lands between the two, a late +provider outcome that contradicts an operator resolution is finalized without +the contradiction being logged (the row keeps both facts either way). A version +read that fails is logged and finalizes without the check. Neither loses the +provider outcome. ### RI id matching is exact for stored history rows (issue #1829) diff --git a/runbooks/ri-exchange-direct-execution-settle.md b/runbooks/ri-exchange-direct-execution-settle.md index fb3acb1f2..d6f088234 100644 --- a/runbooks/ri-exchange-direct-execution-settle.md +++ b/runbooks/ri-exchange-direct-execution-settle.md @@ -12,57 +12,134 @@ because the exchange may already have happened at AWS. recorded `failed`. Migration 000131's wording on `unknown` is stale for AWS. The refusal the operator sees names the shared RIs and says to verify the outcome at AWS. +Resolving a row records what you found at AWS and releases its RIs. The row keeps its +original `status`; the resolution is stored beside it (`resolved_status`, `resolved_by`, +`resolved_at`, `resolution_note`), and rows with a resolution no longer block anything. ## Step 1: confirm the outcome at AWS Run `DescribeReservedInstances` (and check the target offering) for the source RIs -named in the refusal. Decide: +named in the refusal, in the row's region. Decide: -- no exchange happened: settle as `failed`; -- the exchange happened: settle as `completed`. +- no exchange happened: resolve as `failed`; +- the exchange happened: resolve as `completed`. -Do not guess. Marking a row `failed` releases the RIs for another exchange. +Do not guess. Resolving `failed` releases the RIs for another exchange. Note the time you +ran the check: the resolve asks for it (`checked_at`). -## Step 2: find the audit row id +## Step 2: list the stuck rows (API, primary path) -The refusal hides the blocking row's id. Find it in the server log (search for -"held by direct execute"), or query by the RI id named in the refusal (`:'ri'`; set the psql variables first, for example -`psql -v ri=ri-0abc -v id= -v new_status=failed -v note='checked at AWS'`, or `\set id ''`): +You need `execute:ri-exchange` on the row's account and region, as a signed-in user or a +user API key (the admin API key is refused). Placeholders: ``, ``, +``, ``. + +```sh +curl -s -H "Authorization: Bearer " \ + "https:///api/ri-exchange/direct-executions?status=unknown®ion=&ri_id=" +``` + +All query parameters are optional (`status` is `submitted` or `unknown`, default both; +`limit` 1 to 200; `cursor` from `next_cursor`). Each item has `id`, `status`, +`source_ids`, `created_at`, `error_summary`, `resolvable` and `resolvable_at`. Resolved +rows are not listed. A row outside your allowed accounts is not listed either. + +## Step 3: resolve the row (API, primary path) + +```sh +curl -s -X POST \ + -H "Authorization: Bearer " -H "X-CSRF-Token: " \ + -H "Content-Type: application/json" \ + "https:///api/ri-exchange/direct-executions//resolve" \ + -d '{ + "outcome": "failed", + "expected_status": "unknown", + "reason": "DescribeReservedInstances shows the RIs still active, no exchange", + "acknowledge_unknown_outcome": true, + "cloud_check": {"checked_region": "", "checked_at": ""} + }' +``` + +- `expected_status` is the status you saw in the list. If the row changed meanwhile you get 409. +- `outcome: "failed"` needs `acknowledge_unknown_outcome: true`. `completed` does not. +- `reason` is 10 to 300 characters, no control characters, and the stored note must fit 1000 bytes. + Non-ASCII text needs fewer characters; the API answers 400 "reason too long once encoded; shorten it". +- `checked_region` must be the row's region. A row with an empty region cannot be resolved through the + API (400 on region); use the break-glass statement below. +- A repeat of the same request returns the stored result with `replayed: true`. A repeat that + differs in any field, including `expected_status`, is a 409, not a replay. +- 409 with "wait until" and a time means the row is too young; see "Why the age" below. +- 400 "checked_at must be at or after" and a time means your AWS check is too early; see + "Why checked_at has a lower bound" below. Check again at AWS, then retry with the new time. + +## Why the age + +A `submitted` row can only be resolved 10 minutes after it was created. The claim and +the AWS call share one deadline of 4 minutes measured from just before the row is +saved, so a `submitted` row younger than 10 minutes (about 4 minutes plus 6 minutes of +margin for clock skew and finalize) may still be in flight. Resolving it `failed` would +release the RI while AWS might still commit the accept. `created_at` is taken with +`clock_timestamp()` after the guard lock is acquired, so a lock wait does not age the +row. An `unknown` row has no age rule: its accept already returned. + +## Why checked_at has a lower bound + +Your AWS check only counts if it was made after the exchange could no longer change. +So `checked_at` must not be in the future, and must be at or after: + +- for a `submitted` row: `created_at` plus 10 minutes (the point where the process + that was running it is certainly finished); +- for an `unknown` row: `completed_at` plus 1 minute (the accept had returned, and the + 1 minute allows for EC2 `DescribeReservedInstances` being eventually consistent). + +A check made earlier proves nothing in either direction. The list shows `resolvable_at`, +which is that same earliest time, so a check at or after it is accepted. + +## LAST RESORT: settle the row in SQL + +Use this only when the API itself is unavailable. It bypasses the API's permission and +account checks and its audit log line, so a second person should review it. Run it only +after Step 1, in the application database, with the same rules as the API. + +Find the row id from the server log (search for "held by direct execute"), or by RI id +(set the psql variables first, for example +`psql -v ri=ri-0abc -v id= -v outcome=failed -v expected_status=unknown -v operator= -v checked_at='2025-01-01T12:00:00Z' -v note='checked at AWS'`): ```sql -SELECT id, status, created_at +SELECT id, status, created_at, completed_at FROM ri_exchange_direct_executions - WHERE provider = 'aws' AND status IN ('submitted', 'unknown') + WHERE provider = 'aws' AND status IN ('submitted', 'unknown') AND resolved_at IS NULL AND EXISTS (SELECT 1 FROM unnest(source_ids) x WHERE lower(btrim(x, E' \t\n\r\f\x0B')) = lower(btrim(:'ri', E' \t\n\r\f\x0B'))); ``` -## Step 3: settle the row (the interim DBA step) - -This rewrites rows that the audit design otherwise treats as final. It is the -interim until the resolve path (#1836) exists. Run only after Step 1. - ```sql --- :'id' = the audit row id; :'new_status' = 'failed' (AWS shows no exchange) or 'completed' (AWS shows it happened) +-- :'id' = the audit row id; :'expected_status' = the status the SELECT showed ('submitted' or 'unknown'); +-- :'outcome' = 'failed' (AWS shows no exchange) or 'completed' (AWS shows it happened); +-- :'operator' = your name; :'checked_at' = when you checked AWS; :'note' = what you found UPDATE ri_exchange_direct_executions - SET status = :'new_status', - completed_at = COALESCE(completed_at, now()), - error = concat_ws(E'\n', error, 'operator-settled ' || now()::text || ': ' || :'note') + SET resolved_status = :'outcome', + resolved_by_user_id = NULL, + resolved_by = :'operator', + resolved_at = now(), + resolution_note = v.n + FROM (SELECT 'break-glass checked_at=' || to_char(:'checked_at'::timestamptz AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"') + || ' note=' || :'note' AS n) v WHERE id = :'id' AND provider = 'aws' + AND resolved_at IS NULL + AND status = :'expected_status' AND status IN ('submitted', 'unknown') - AND (status = 'unknown' OR created_at < now() - interval '10 minutes'); --- Expect exactly 1 row updated. 0 rows means: wrong id, already settled, or a 'submitted' row younger --- than 10 minutes that may still be in flight. Do NOT widen the WHERE clause; wait and re-check. + AND octet_length(v.n) <= 1000 + AND btrim(:'operator') <> '' AND btrim(:'note') <> '' + AND ( (status = 'submitted' AND created_at <= now() - interval '10 minutes' + AND :'checked_at'::timestamptz >= created_at + interval '10 minutes') + OR (status = 'unknown' AND :'checked_at'::timestamptz >= completed_at + interval '1 minute')) + AND :'checked_at'::timestamptz <= now(); +-- Expect exactly 1 row updated. 0 rows means: wrong id, already resolved, a status that changed since +-- the SELECT, an empty operator or note, a stored note (with its prefix) over 1000 bytes, a 'submitted' row younger than 10 minutes that may still be in +-- flight, or a checked_at that is too early or in the future. +-- Do NOT widen the WHERE clause; wait, check AWS again and re-run. ``` -- **Why the age check:** the claim and the AWS call share one deadline of 4 minutes - (`riExchangeExecutionTimeout`) measured from just before the row is saved, so a - `submitted` row younger than 10 minutes (about 4 minutes plus 6 minutes of margin for - clock skew and finalize) may still be in flight. Marking it `failed` would release the RI while AWS might still - commit the accept, and the handler's own finalize would then lose the real outcome. -- **Why `created_at` can be trusted:** it is taken with `clock_timestamp()` after the - guard lock is acquired, so a lock wait does not age the row. A `submitted` row older - than 10 minutes is past the execution window, claim included. -- **Why `error` is appended:** it keeps the original failure text. -- An `unknown` row is settled at any age: its accept already returned. +The status and `error` of the row are left as they were; the columns above are all five +that make up a resolution (the table rejects a partial one).