diff --git a/internal/api/handler_ri_exchange_history_notes_test.go b/internal/api/handler_ri_exchange_history_notes_test.go new file mode 100644 index 000000000..b085cd356 --- /dev/null +++ b/internal/api/handler_ri_exchange_history_notes_test.go @@ -0,0 +1,48 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// #1050's two RI exchange notes reach the History row as written: the +// source-RI guard's refusal and the outcome-unknown resolve note. A raw +// provider error on another row is still summarized. +func TestHandler_getRIExchangeHistory_ShowsRIExchangeNotes(t *testing.T) { + const ( + sourceNote = "RI exchange source RI is held by an exchange in progress: source RI(s) ri-0a1b2c3d already have an exchange in progress; resolve that exchange first" + outcomeNote = "outcome unknown: the provider returned an error; verify the source RIs and target reservation at AWS, then POST /api/ri-exchange/resolve/550e8400-e29b-41d4-a716-000000010501" + rawProvider = "RequestError: send request failed for arn:aws:ec2:us-east-1:123456789012:reserved-instances/ri-leak" + ) + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "uid"}, nil) + mockAuth.grantScoped("u-a") + store := new(MockConfigStore) + store.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{{ID: "u-a", Name: "a", Provider: "aws", ExternalID: exchangeAcctA}}, nil + } + store.On("GetRIExchangeHistoryForAccounts", ctx, mock.Anything, 500, mock.Anything).Return([]config.RIExchangeRecord{ + {ID: "source", AccountID: exchangeAcctA, Status: "failed", Error: sourceNote}, + {ID: "outcome", AccountID: exchangeAcctA, Status: "processing", Error: outcomeNote}, + {ID: "raw", AccountID: exchangeAcctA, Status: "failed", Error: rawProvider}, + }, nil).Once() + h := &Handler{auth: mockAuth, config: store} + + result, err := h.getRIExchangeHistory(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer tok"}}) + require.NoError(t, err) + got := map[string]string{} + for _, r := range result.(*RIExchangeHistoryResponse).Records { + got[r.ID] = r.Error + } + assert.Equal(t, sourceNote, got["source"]) + assert.Equal(t, outcomeNote, got["outcome"]) + assert.NotContains(t, got["raw"], "ri-leak", "a raw provider error is still summarized") +} diff --git a/internal/api/handler_ri_exchange_outcome_unknown_test.go b/internal/api/handler_ri_exchange_outcome_unknown_test.go index ed02d43dd..5bf7a4edd 100644 --- a/internal/api/handler_ri_exchange_outcome_unknown_test.go +++ b/internal/api/handler_ri_exchange_outcome_unknown_test.go @@ -10,6 +10,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -52,6 +53,7 @@ func assertOutcomeUnknown(t *testing.T, store *MockConfigStore, id string, resp assert.Contains(t, text, "/api/ri-exchange/resolve/"+id, "%s must tell the operator how to resolve it", name) assert.NotContains(t, text, outcomeUnknownLeak, "%s must not carry raw provider text", name) } + assert.Equal(t, note, purchase.SanitizeStoredError(note), "History shows the stored note as written") store.AssertNotCalled(t, "FailRIExchange", mock.Anything, mock.Anything, mock.Anything) store.AssertNotCalled(t, "CompleteRIExchangeWithPayment", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index e3a99959d..eb8bdfc86 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -71,9 +71,15 @@ var bareNotePatterns = []string{ `exchange reservation returned invalid ceiling "` + shortValuePattern + `"`, // config.refuseRIExchangeSourceInFlightDirect refusal, stored whole from // err.Error() (sentinel text first); only the shared RI ids vary. - `RI exchange source RI is held by an exchange in progress: source RI\(s\) ` + directGuardIDListPattern + ` are held by a direct exchange that may have reached AWS; an operator must verify the outcome at AWS and settle the record`, + `RI exchange source RI is held by an exchange in progress: source RI\(s\) ` + riIDListPattern + ` are held by a direct exchange that may have reached AWS; an operator must verify the outcome at AWS and settle the record`, // Scheduler note for a provider name it does not know. `unknown provider "` + shortValuePattern + `" \(valid: aws, azure, gcp\)`, + // The source-RI guard's refusal (config refuseRIExchangeSourceInFlight): only + // the shared RI ids, never the blocking exchange (#1050, #1428). + `RI exchange source RI is held by an exchange in progress: source RI\(s\) ` + riIDListPattern + ` already have an exchange in progress; resolve that exchange first`, + // The outcome-unknown note (api leaveExchangeOutcomeUnknown): fixed text, the + // sanitized SummarizeFailure reason and the exchange id to resolve (#1050). + `outcome unknown: (?:` + joinAlt(append(categoryNotePatterns(), regexp.QuoteMeta(outcomeUnconfirmedMessage))) + `); verify the source RIs and target reservation at AWS, then POST /api/ri-exchange/resolve/(?i:` + uuidPattern + `)`, } // categoryNotePatterns matches a collection failure stored as ": @@ -98,15 +104,14 @@ const ( commitmentIDPattern = `[A-Za-z0-9._:/-]{1,256}` identityKeyPattern = `[A-Za-z0-9._:/+-]{1,256}` idListPattern = identityKeyPattern + `(?:, ` + identityKeyPattern + `)*` + // riIDListPattern is the ", "-joined source RI ids of an RI exchange. + riIDListPattern = `[A-Za-z0-9-]{1,64}(?:, [A-Za-z0-9-]{1,64})*` // quotedTextPattern is the body of a Go %q string: printable characters // other than '"' and '\\', ASCII space, or an escape. quotedTextPattern = `(?:[^"\\\p{C}\p{Z}]| |\\[ -~])*` skippedNotePattern = `\d+ recommendation\(s\) without a frozen baseline skipped \(under-buy\): ` + idListPattern ) -// directGuardIDListPattern is the ", "-joined lowercase RI ids the direct-execute guard names. -const directGuardIDListPattern = `[A-Za-z0-9-]{1,64}(?:, [A-Za-z0-9-]{1,64})*` - // serverNoteHeads are server notes whose tail can carry a wrapped store or // config error. Only the fixed head is ever shown; the tail is dropped. var serverNoteHeads = []string{ diff --git a/internal/purchase/errnotes_ri_exchange_test.go b/internal/purchase/errnotes_ri_exchange_test.go new file mode 100644 index 000000000..f979e9584 --- /dev/null +++ b/internal/purchase/errnotes_ri_exchange_test.go @@ -0,0 +1,64 @@ +package purchase + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// The two #1050 RI exchange notes are stored as the record's error and read +// back through SanitizeStoredError on History, so they must pass verbatim, +// and anything that is not their closed shape must still fall back. +const ( + riExchangeSourceInFlightNote = "RI exchange source RI is held by an exchange in progress: source RI(s) ri-0a1b2c3d, 1ba8e2e3-0c4f-4a7e-9a7b-5d6e7f8a9b0c already have an exchange in progress; resolve that exchange first" + riExchangeOutcomeUnknownHead = "outcome unknown: " + riExchangeOutcomeUnknownTail = "; verify the source RIs and target reservation at AWS, then POST /api/ri-exchange/resolve/550e8400-e29b-41d4-a716-000000010501" +) + +func TestSanitizeStoredError_RIExchangeNotesPassVerbatim(t *testing.T) { + reasons := []string{ + ErrCategoryMessage(ErrCategoryUnknown), + ErrCategoryMessage(ErrCategoryAccessDenied), + ErrCategoryMessage(ErrCategoryUnavailable), + ErrCategoryMessage(ErrCategoryLimit), + outcomeUnconfirmedMessage, + } + notes := make([]string, 0, len(reasons)+1) + notes = append(notes, riExchangeSourceInFlightNote) + for _, reason := range reasons { + notes = append(notes, riExchangeOutcomeUnknownHead+reason+riExchangeOutcomeUnknownTail) + } + // The resolve route accepts an upper-case id, so the note may carry one. + notes = append(notes, riExchangeOutcomeUnknownHead+outcomeUnconfirmedMessage+riExchangeOutcomeUnknownTail[:len(riExchangeOutcomeUnknownTail)-36]+strings.ToUpper(riExchangeOutcomeUnknownTail[len(riExchangeOutcomeUnknownTail)-36:])) + notes = append(notes, sourceInFlightNote(strings.Repeat("a", 64))) + for _, note := range notes { + assert.Equal(t, note, SanitizeStoredError(note)) + } +} + +func TestSanitizeStoredError_RIExchangeNotesStayClosed(t *testing.T) { + leak := "arn:aws:ec2:us-east-1:123456789012:reserved-instances/ri-leak" + for _, in := range []string{ + // A raw provider reason in place of a summarized one. + riExchangeOutcomeUnknownHead + "RequestError: send request failed for " + leak + riExchangeOutcomeUnknownTail, + // A resolve id that is not a UUID. + riExchangeOutcomeUnknownHead + ErrCategoryMessage(ErrCategoryUnknown) + "; verify the source RIs and target reservation at AWS, then POST /api/ri-exchange/resolve/" + leak, + // RI ids carrying text the guard never writes. + "RI exchange source RI is held by an exchange in progress: source RI(s) ri-1 " + leak + " already have an exchange in progress; resolve that exchange first", + // Text ahead of either note (the patterns are anchored at the head). + leak + " " + riExchangeSourceInFlightNote, + leak + ": " + riExchangeOutcomeUnknownHead + outcomeUnconfirmedMessage + riExchangeOutcomeUnknownTail, + // An RI id past 64 characters, and an empty RI list. + sourceInFlightNote(strings.Repeat("a", 65)), + sourceInFlightNote(""), + } { + out := SanitizeStoredError(in) + assert.NotEqual(t, in, out, in) + assert.NotContains(t, out, leak) + } +} + +func sourceInFlightNote(ids string) string { + return "RI exchange source RI is held by an exchange in progress: source RI(s) " + ids + " already have an exchange in progress; resolve that exchange first" +}