diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index be0f7b76b..1d3ec1540 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -26,6 +26,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" @@ -309,11 +310,11 @@ func (h *Handler) keepUncanceledListing(ctx context.Context, purchaseID string, if state == "" { state = config.ListingStatePending } - cancelDetail := cancelErr.Error() + _, cancelDetail := purchase.SummarizeProviderError(cancelErr) if isRoleAssumptionFailure(cancelErr) { cancelDetail = marketplaceRoleAssumptionMessage } - msg := fmt.Sprintf("marketplace listing %s was created but could not be saved, and canceling it failed (%s); the listing may still be active on AWS", listing.ListingID, cancelDetail) + msg := fmt.Sprintf("marketplace listing %s was created but could not be saved, and canceling it failed (%s); the listing may still be active on AWS (reference: purchase %s)", listing.ListingID, cancelDetail, purchaseID) if err := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, listing.ListingID, state); err != nil { if isListingEndedConflict(err) { return listingCanceledConcurrently(listing.ListingID) @@ -472,8 +473,8 @@ func roleAssumptionOperationError(err error) *smithy.OperationError { const marketplaceSellerNotRegisteredReason = "seller_not_registered" // mapAWSMarketplaceError maps an AWS SDK error to an appropriate ClientError. -// AWS client-fault errors (4xx-category codes) produce a 4xx response with the -// original AWS message so the caller gets actionable feedback. SellerNotRegistered +// AWS client-fault errors (4xx-category codes) produce a 400 with a category +// message, never the AWS text, which carries ARNs and request IDs. SellerNotRegistered // is the exception: its 400 carries marketplaceSellerNotRegisteredMessage and // reason seller_not_registered instead. All other errors produce a 502 // (AWS-side failure). STS AssumeRole failures are classified by @@ -489,12 +490,23 @@ func mapAWSMarketplaceError(opMsg string, err error) error { map[string]any{"reason": marketplaceSellerNotRegisteredReason}) } if awsMarketplaceClientFaultCodes[apiErr.ErrorCode()] || apiErr.ErrorFault() == smithy.FaultClient { - return NewClientError(400, apiErr.ErrorMessage()) + logging.Warnf("marketplace: %s: provider rejected the request: %v", opMsg, err) + category, msg := purchase.SummarizeProviderError(err) + if category == purchase.ErrCategoryUnknown { + msg = marketplaceRejectedMessage + } + return NewClientError(400, msg) } } - return NewClientError(502, opMsg+": "+err.Error()) + logging.Warnf("marketplace: %s: %v", opMsg, err) + _, msg := purchase.SummarizeProviderError(err) + return NewClientError(502, opMsg+": "+msg) } +// marketplaceRejectedMessage is the fixed text for a client-fault rejection +// the error classifier has no category for; the provider's text stays in the log. +const marketplaceRejectedMessage = "AWS rejected the listing request as invalid: check the listing parameters" + // isKnownNonStandardOfferingClass reports true when offering_class is // explicitly set to a non-standard value (e.g. "convertible"). An empty string // returns false because the class is not yet known and will be resolved lazily. diff --git a/internal/api/handler_marketplace_attempt.go b/internal/api/handler_marketplace_attempt.go index eaeba0de4..f98cfaab6 100644 --- a/internal/api/handler_marketplace_attempt.go +++ b/internal/api/handler_marketplace_attempt.go @@ -208,7 +208,7 @@ func (h *Handler) createAndPersistListing(ctx context.Context, purchaseID string // the listing unless it rejected the request as a client fault, so only // then is the token dropped; otherwise the retry resends it. h.releaseMarketplaceClaim(ctx, purchaseID, claim, !isClientFaultError(mapped)) - logging.Warnf("marketplace: CreateReservedInstancesListing for purchase %s failed: %v", purchaseID, err) + logging.Warnf("marketplace: CreateReservedInstancesListing for purchase %s failed", purchaseID) return ec2MarketplaceListingResult{}, mapped } diff --git a/internal/api/handler_marketplace_cancel.go b/internal/api/handler_marketplace_cancel.go index 9b24fda3e..76287e127 100644 --- a/internal/api/handler_marketplace_cancel.go +++ b/internal/api/handler_marketplace_cancel.go @@ -148,7 +148,7 @@ func (h *Handler) cancelListing(ctx context.Context, purchaseID string, seen *co if err != nil { ended, ok := endedMarketplaceListing(ctx, ec2Client, listingID, err) if !ok { - logging.Warnf("marketplace: CancelReservedInstancesListing for listing %s failed: %v", listingID, err) + logging.Warnf("marketplace: CancelReservedInstancesListing for listing %s failed", listingID) return nil, mapAWSMarketplaceError("AWS cancel listing failed", err) } result = ended diff --git a/internal/api/handler_marketplace_compensation_integration_test.go b/internal/api/handler_marketplace_compensation_integration_test.go index fb361a55d..7c381e23e 100644 --- a/internal/api/handler_marketplace_compensation_integration_test.go +++ b/internal/api/handler_marketplace_compensation_integration_test.go @@ -69,7 +69,9 @@ func TestMarketplaceListCancelFailureKeepsListingRecorded(t *testing.T) { require.True(t, ok, "%v", err) assert.Equal(t, 502, ce.code) assert.NotContains(t, ce.message, "rolled back") - assert.Contains(t, ce.message, "RequestLimitExceeded") + // The provider's text stays in the log; the user gets the summary and a reference (#1702). + assert.NotContains(t, ce.message, "RequestLimitExceeded") + assert.Contains(t, ce.message, "reference: purchase "+row.PurchaseID) stored, err := store.GetPurchaseHistoryByPurchaseID(ctx, row.PurchaseID) require.NoError(t, err) diff --git a/internal/api/handler_marketplace_credentials_test.go b/internal/api/handler_marketplace_credentials_test.go index e99d2cd15..8a30b128e 100644 --- a/internal/api/handler_marketplace_credentials_test.go +++ b/internal/api/handler_marketplace_credentials_test.go @@ -13,6 +13,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-sdk-go-v2/aws" awscreds "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/smithy-go" @@ -248,7 +249,8 @@ func TestMarketplaceList_CompensationAssumeRoleDeniedReturnsFixedMessage(t *test require.True(t, ok, "expected a client error, got %v", err) assert.Equal(t, 502, ce.code) assert.Contains(t, ce.message, marketplaceRoleAssumptionMessage) - assert.NotContains(t, ce.message, marketplaceHostAccount) + // The purchase ID in the reference is a test UUID whose last segment equals the fake host account. + assert.NotContains(t, strings.ReplaceAll(ce.message, validMarketplacePurchaseID, ""), marketplaceHostAccount) assert.NotContains(t, ce.message, "arn:aws:") } @@ -468,7 +470,11 @@ func TestMarketplaceHandlers_NoCloudAccountRow(t *testing.T) { } } -func TestMapAWSMarketplaceError_GenuineEC2ErrorsKeepTheirOwnMessage(t *testing.T) { +func TestMapAWSMarketplaceError_GenuineEC2ErrorsKeepTheirOwnCategory(t *testing.T) { + want := map[string]string{ + "InvalidReservedInstancesId.NotFound": purchase.ErrCategoryMessage(purchase.ErrCategoryUnavailable), + "UnauthorizedOperation": purchase.ErrCategoryMessage(purchase.ErrCategoryAccessDenied), + } for _, code := range []string{"InvalidReservedInstancesId.NotFound", "UnauthorizedOperation"} { t.Run(code, func(t *testing.T) { const msg = "the ec2 service said no" @@ -482,7 +488,7 @@ func TestMapAWSMarketplaceError_GenuineEC2ErrorsKeepTheirOwnMessage(t *testing.T require.True(t, ok) assert.Equal(t, 400, ce.code) - assert.Equal(t, msg, ce.message) + assert.Equal(t, want[code], ce.message, "a category message, not the provider text") assert.NotEqual(t, marketplaceRoleAssumptionMessage, ce.message) }) } @@ -565,6 +571,6 @@ func TestMapAWSMarketplaceError_OtherClientFaultsHaveNoReason(t *testing.T) { require.True(t, ok) assert.Equal(t, 400, ce.code) - assert.Equal(t, "bad price", ce.message) + assert.Equal(t, marketplaceRejectedMessage, ce.message) assert.Nil(t, ce.Details()) } diff --git a/internal/api/handler_marketplace_test.go b/internal/api/handler_marketplace_test.go index 8814d7f8f..997a767f8 100644 --- a/internal/api/handler_marketplace_test.go +++ b/internal/api/handler_marketplace_test.go @@ -9,6 +9,7 @@ import ( ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" "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/aws/aws-sdk-go-v2/aws" smithy "github.com/aws/smithy-go" @@ -274,7 +275,7 @@ func TestMarketplaceList_AWSClientFaultMapsTo400(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 400, ce.code) - assert.Contains(t, err.Error(), "bad RI") + assert.Contains(t, err.Error(), marketplaceRejectedMessage) } func TestMarketplaceList_AWSServerFaultMapsTo502(t *testing.T) { @@ -399,7 +400,7 @@ func TestMarketplaceList_DBFailureAndCancelFailureKeepsListing(t *testing.T) { assert.Equal(t, 502, ce.code) assert.NotContains(t, ce.message, "rolled back") assert.Contains(t, ce.message, "ril-default") - assert.Contains(t, ce.message, "throttled") + assert.Contains(t, ce.message, purchase.ErrCategoryMessage(purchase.ErrCategoryUnknown)) assert.Contains(t, ce.message, tc.wantMsg) assert.Equal(t, 1, ec2.cancelCallCount) cfgStore.AssertNotCalled(t, "ReleaseMarketplaceListingClaim", mock.Anything, validMarketplacePurchaseID, mock.Anything, mock.Anything, mock.Anything) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 7cc4348c1..104196a7f 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -552,15 +552,7 @@ func logRawExecuteFailure(executionID string, err error) { func summarizeExecuteFailure(executionID string, err error) (category, message string) { logRawExecuteFailure(executionID, err) - category, message = purchase.SummarizeProviderError(err) - if category == purchase.ErrCategoryUnknown { - // Only known server-authored refusals (for example "re-fetch the - // recommendation") stay readable; any other text is the category message. - if text, ok := purchase.ServerNoteMessage(err); ok { - message = text - } - } - return category, message + return purchase.SummarizeFailure(err) } // unclaimedExecuteError answers a failure where the funnel returned no row: @@ -1500,7 +1492,8 @@ func (h *Handler) approveWithDelay(ctx context.Context, execution *config.Purcha logging.Errorf("purchase[%s]: token approve failed to schedule: %v", execution.ExecutionID, err) return nil, NewClientError(500, "failed to schedule execution") } - return nil, NewClientError(500, fmt.Sprintf("failed to schedule execution %s: %v", execution.ExecutionID, err)) + logging.Errorf("purchase[%s]: failed to schedule execution: %v", execution.ExecutionID, err) + return nil, NewClientError(500, fmt.Sprintf("failed to schedule execution (reference: execution %s)", execution.ExecutionID)) } // Best-effort scheduled notification email. h.sendPurchaseScheduledEmail(ctx, updated, actor) diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index db258827e..7240e1d1e 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -35,6 +35,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/ctxutil" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-lambda-go/events" "github.com/jackc/pgx/v5" ) @@ -597,7 +598,7 @@ func (h *Handler) calculateAzureRevoke(ctx context.Context, req *events.LambdaFu }, nil) if err != nil { if isAzureClientError(err) { - return nil, NewClientError(400, fmt.Sprintf("Azure refund calculation rejected: %v", err)) + return nil, azureRefundRejected("Azure refund calculation rejected", purchaseID, err) } return nil, fmt.Errorf("revoke/calculate: CalculateRefund failed: %w", err) } @@ -824,7 +825,7 @@ func (h *Handler) callAzureReturn( // Step 1: CalculateRefund -> sessionID + quoted amount (TOCTOU check). quantity := int32(record.Count) // #nosec G115 -- Azure reservation count validated at purchase; bounded by API limits (<: +// " per failed provider. +func categoryNotePatterns() []string { + out := make([]string, 0, 4) + for _, c := range []string{ErrCategoryAccessDenied, ErrCategoryUnavailable, ErrCategoryLimit, ErrCategoryUnknown} { + out = append(out, regexp.QuoteMeta(ErrCategoryMessage(c))) + } + return out +} + const ( + // amountPattern is a decimal amount; shortValuePattern is a short value with no + // whitespace, quotes or newline. Anything wider falls back to the fixed head. + amountPattern = `\d+(?:\.\d+)?` + shortValuePattern = `[A-Za-z0-9._+-]{1,32}` idPattern = `[^;\s,()]+` idListPattern = idPattern + `(?:, ` + idPattern + `)*` skippedNotePattern = `\d+ recommendation\(s\) without a frozen baseline skipped \(under-buy\): ` + idListPattern @@ -56,13 +94,56 @@ var serverNoteHeads = []string{ `scheduled purchase skipped: it was due too long ago`, } +// exchangeNoteHeads are the fixed heads of the exchange notes whose tail is not closed. +var exchangeNoteHeads = []string{ + `RI exchange daily cap exceeded`, + `reserved ceiling`, + `exchange reservation returned invalid ceiling`, + `unknown provider`, +} + var redriveReasonPattern = regexp.QuoteMeta(redriveReasonAWSNoGuard) + `|` + regexp.QuoteMeta(redriveReasonAzureSavingsPlan) + `|provider "[^"]*" is not known to reject a duplicate purchase, so re-driving this could buy a second commitment` -// A rec's ": " head, as the writer's per-rec join adds it. -var serverNoteRE = regexp.MustCompile(`^(?:[A-Za-z0-9][A-Za-z0-9._-]*: )?(?:` + strings.Join(serverNotePatterns, "|") + `)(?:; |$)`) +// resourceTypePattern is the ": " head the executor's per-rec join +// adds (execution.go: fmt.Sprintf("%s: %v", rec.ResourceType, err)). The head is +// closed: a Savings Plan type from a fixed list, or an instance, node or machine +// type shape whose family carries a digit, so an account number, IPv4 address, +// hostname, DB user or ARN never fits: +// - Savings Plan types: Compute, EC2Instance, SageMaker (aws-sdk SavingsPlanType) +// and Database, plus Azure's "Compute" +// - AWS: [db.|cache.].[.search] (m5.large, u-6tb1.metal, +// mac2-m2pro.metal, ultrawarm1.medium.search, m7i.metal-24xl) and db.serverless +// - Azure SQL: GP_Gen5_8 vCore SKUs and the tier names GENERAL_PURPOSE, BUSINESS_CRITICAL, HYPERSCALE +// - Azure: Standard_|Basic_ sizes (Standard_D2, Standard_NV24ads_A10_v5) and the +// unprefixed forms the provider sends (D2, D2s_v3, NC6s_v3) +// - GCP: -[-]... with a closed series word +// (n2-standard-2, n2d-custom-2-8192-ext, a2-ultragpu-8g, c3d-highmem-360-lssd, e2-micro) +const resourceTypePattern = `(?:` + + `Compute|EC2Instance|SageMaker|Database|db\.serverless` + + `|(?:db\.|cache\.)?(?:[a-z]{1,4}[0-9][a-z0-9]{0,5}(?:-[a-z][a-z0-9]{1,7})?|u-[0-9]{1,2}tb[0-9]|ultrawarm1)\.(?:nano|micro|small|medium|large|[0-9]{0,3}xlarge|xlplus|metal(?:-[0-9]{1,2}xl)?)(?:\.search)?` + + `|(?:Standard|Basic)_[A-Za-z]{1,4}[0-9][A-Za-z0-9_-]{0,24}` + + `|[A-Z]{1,3}[0-9]{1,3}[a-z]{0,4}(?:_[A-Za-z0-9]{1,4}){0,2}|[A-Z]{1,3}_Gen[0-9]_[0-9]{1,3}|GENERAL_PURPOSE|BUSINESS_CRITICAL|HYPERSCALE` + + `|[a-z][0-9][a-z]{0,2}-(?:standard|highmem|highcpu|highgpu|ultragpu|megagpu|megamem|ultramem|hypermem|custom|micro|small|medium)(?:-[a-z0-9]{1,8}){0,3}` + + `)` + +// providerHeadPattern is the collection writer's ": " head. +const providerHeadPattern = `(?:aws|azure|gcp)` + +const ( + resourceHead = `(?:` + resourceTypePattern + `: )?` + categoryHead = `(?:(?:` + providerHeadPattern + `|` + resourceTypePattern + `): )?` +) + +func joinAlt(parts []string) string { return strings.Join(parts, "|") } + +var serverNoteRE = regexp.MustCompile(`^(?:` + + resourceHead + `(?:` + joinAlt(serverNotePatterns) + `)` + + `|` + `(?:` + joinAlt(bareNotePatterns) + `)` + + `|` + categoryHead + `(?:` + joinAlt(categoryNotePatterns()) + `)` + + `)(?:; |$)`) -var serverNoteHeadRE = regexp.MustCompile(`^((?:[A-Za-z0-9][A-Za-z0-9._-]*: )?(?:` + strings.Join(serverNoteHeads, "|") + `))(?:: [^;]*)?(?:; |$)`) +var serverNoteHeadRE = regexp.MustCompile(`^(` + resourceHead + `(?:` + joinAlt(serverNoteHeads) + `)|(?:` + joinAlt(exchangeNoteHeads) + `))(?:(?:: | ")[^;]*)?(?:; |$)`) // renderServerNotes returns s with every "; "-separated item either kept // verbatim (a closed allowlisted note) or cut to its fixed head (a known note diff --git a/internal/purchase/errnotes_tail_test.go b/internal/purchase/errnotes_tail_test.go index 851d9cbd0..ac0271efb 100644 --- a/internal/purchase/errnotes_tail_test.go +++ b/internal/purchase/errnotes_tail_test.go @@ -31,7 +31,7 @@ func TestSanitizeStoredError_TailedNotesShowOnlyTheirHead(t *testing.T) { {ErrStaleAzurePricing.Error() + ": Azure recommendation \"r-1\" no longer has the approved price", ErrStaleAzurePricing.Error()}, {ErrStepCommitmentCap.Error() + ": recommendation \"r-1\" is not priced in USD, so it cannot be compared against the USD cap", ErrStepCommitmentCap.Error()}, {ErrClaimedRowRefused.Error() + ": " + fakeConnectError, ErrClaimedRowRefused.Error()}, - {"x: ramp not advanced: " + fakeConnectError + "; recovered after reap; provider purchase succeeded", "x: ramp not advanced; recovered after reap; provider purchase succeeded"}, + {"m5.large: ramp not advanced: " + fakeConnectError + "; recovered after reap; provider purchase succeeded", "m5.large: ramp not advanced; recovered after reap; provider purchase succeeded"}, {partialRunPrefix + "ramp not advanced: " + fakeConnectError, partialRunPrefix + "ramp not advanced"}, } for _, c := range cases { diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index e192b7239..cccc1e40e 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -2,6 +2,7 @@ package purchase import ( "fmt" + "slices" "testing" "time" @@ -55,11 +56,28 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { "no notification email set in Settings → General and no account contact emails configured", "no notification email set in Settings → General", "FROM_EMAIL not configured for this deployment", + "config load failed", + "exchange record has no region; cannot execute safely", + "daily spending cap is not configured (RIExchangeMaxDailyUSD is 0)", + "per-exchange spending cap is not configured (RIExchangeMaxPerExchangeUSD is 0)", + "RI exchange daily cap exceeded: spend 900.000000 exceeds cap 800.000000", + "RI exchange daily cap exceeded: initial 10.000000, remaining 5.000000", + "daily spending cap check failed", + `reserved ceiling "x" is not a decimal`, + "exchange reservation returned no record ID", + `exchange reservation returned invalid ceiling "x"`, + `unknown provider "AWS" (valid: aws, azure, gcp)`, + ErrCategoryMessage(ErrCategoryAccessDenied), } + exchangeNotes := notes[slices.Index(notes, "config load failed"):slices.Index(notes, ErrCategoryMessage(ErrCategoryAccessDenied))] for _, n := range notes { assert.NotEmpty(t, n) assert.Equal(t, n, SanitizeStoredError(n), n) - assert.Equal(t, n, SanitizeStoredError(partialRunPrefix + "x: " + n)[len(partialRunPrefix)+3:], "behind a resource head: "+n) + if slices.Contains(exchangeNotes, n) { + continue + } + head := "m5.large: " + assert.Equal(t, n, SanitizeStoredError(partialRunPrefix + head + n)[len(partialRunPrefix)+len(head):], "behind a resource head: "+n) } joined := notes[0] + "; " + notes[1] + "; " + notes[6] assert.Equal(t, joined, SanitizeStoredError(joined)) @@ -87,3 +105,96 @@ const legacyPlanStepNoRecommendationsNote = "plan step has no recommendations at func TestErrPlanStepNoRecommendations_NamesNoInternalIssue(t *testing.T) { assert.NotContains(t, ErrPlanStepNoRecommendations.Error(), "#") } + +// A recommendation collection failure is stored as one ": " item per failed provider. +func TestSanitizeStoredError_CollectionCategoryForm(t *testing.T) { + stored := "aws: " + ErrCategoryMessage(ErrCategoryAccessDenied) + "; azure: " + ErrCategoryMessage(ErrCategoryUnknown) + assert.Equal(t, stored, SanitizeStoredError(stored)) + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(stored+"; gcp: raw text for project my-project")) +} + +// A variable part outside its closed alphabet (a newline, a provider or DB +// error text) never reaches the output: only the fixed head or nothing shows. +func TestSanitizeStoredError_ExchangeNoteTailsAreClosed(t *testing.T) { + cases := []struct{ in, want string }{ + {"RI exchange daily cap exceeded: spend 1\nDETAIL pq: boom exceeds cap 2", "RI exchange daily cap exceeded"}, + {"RI exchange daily cap exceeded: initial 1.0, remaining pq: connection refused", "RI exchange daily cap exceeded"}, + {"RI exchange daily cap exceeded: spend x y exceeds cap 2.0", "RI exchange daily cap exceeded"}, + {"reserved ceiling \"a\nb\" is not a decimal", "reserved ceiling"}, + {"reserved ceiling \"pq: boom\" is not a decimal", "reserved ceiling"}, + {"exchange reservation returned invalid ceiling \"x\ny\"", "exchange reservation returned invalid ceiling"}, + {"unknown provider \"evil\nprovider\" (valid: aws, azure, gcp)", "unknown provider"}, + {"unknown provider \"db error: password for x\" (valid: aws, azure, gcp)", "unknown provider"}, + } + for _, c := range cases { + assert.Equal(t, c.want, SanitizeStoredError(c.in), c.in) + } + for _, in := range []string{ + "config load failed\nsecret", + "config load failed: pq: boom", + "exchange record has no region; cannot execute safely\nx", + "daily spending cap check failed: pq: boom", + "exchange reservation returned no record ID: pq: boom", + "daily spending cap is not configured (RIExchangeMaxDailyUSD is 0)\nx", + "aws: " + ErrCategoryMessage(ErrCategoryUnknown) + "\nraw", + } { + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(in), in) + } +} + +// The head ahead of a note must be a real resource type (per-rec notes) or +// aws|azure|gcp (collection notes); an account number, address, host or DB user +// in that slot is never shown. +func TestSanitizeStoredError_HeadsAreClosed(t *testing.T) { + note := "plan step refused by commitment cap" + category := ErrCategoryMessage(ErrCategoryUnavailable) + for _, h := range []string{ + "m5.large", "db.t4g.micro", "cache.t3.micro", "dc2.large", "ra3.xlplus", "ra3.16xlarge", + "t3.small.search", "Standard_D2_v3", "Basic_A1", "n2-standard-2", "e2-micro", "c3-highcpu-8", + // Savings Plan types (aws-sdk SavingsPlanType, azure computeResourceType). + "Compute", "EC2Instance", "SageMaker", "Database", + "db.serverless", "u-6tb1.metal", "u-6tb1.112xlarge", "mac2-m2pro.metal", "ultrawarm1.medium.search", + "m7i.metal-24xl", "n2-custom-4-8192", "n2d-custom-2-8192-ext", "e2-custom-4-8192", "e2-custom-medium-2048", + "a2-ultragpu-8g", "a2-highgpu-1g", "a3-megagpu-8g", "c3d-highmem-360-lssd", "m1-ultramem-40", "h4d-highmem-192", + "Standard_D2", "Standard_B2ats_v2", "Standard_NV24ads_A10_v5", "Standard_NC6s_v3", "Basic_C0", + "D2", "D2s_v3", "NC6s_v3", "A1", + "GP_Gen5_8", "BC_Gen5_16", "GENERAL_PURPOSE", "c7i-flex.large", + } { + assert.Equal(t, h+": "+note, SanitizeStoredError(h+": "+note), h) + assert.Equal(t, h+": "+category, SanitizeStoredError(h+": "+category), h) + } + for _, h := range []string{"aws", "azure", "gcp"} { + assert.Equal(t, h+": "+category, SanitizeStoredError(h+": "+category), h) + } + for _, h := range []string{ + "123456789012", "10.20.30.40", "db-prod.internal", "db.internal.example", "cudly_app", "web1.prod.example", + "m5.large.example.com", "ec2.large.example", "arn:aws:iam::123456789012:role/x", "evil", "AWS", + } { + unknown := ErrCategoryMessage(ErrCategoryUnknown) + assert.Equal(t, unknown, SanitizeStoredError(h+": "+category), h) + assert.Equal(t, unknown, SanitizeStoredError(h+": "+note), h) + } + for _, in := range []string{ + "123456789012: config load failed", + "10.20.30.40: config load failed", + "db-prod.internal: daily spending cap check failed", + "cudly_app: the requested commitment is not available from the provider", + "m5.large: config load failed", + "aws: " + note, + } { + out := SanitizeStoredError(in) + assert.NotContains(t, out, ": ", in) + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), out, in) + } +} + +func TestSanitizeStoredError_ResolveNoteExchangeIDIsClosed(t *testing.T) { + base := `operator:completed: account="` + "1234" + "56789012" + `" region="us-east-1" checked_at=2026-01-02T03:04:05Z` + ok := base + ` exchange_id="abc-123" reason="done"` + assert.Equal(t, ok, SanitizeStoredError(ok)) + for _, id := range []string{`a b`, `db-prod.internal`, `x\"y`, `a_b`, ``} { + in := base + ` exchange_id="` + id + `" reason="done"` + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(in), id) + } +} diff --git a/internal/purchase/errsummary.go b/internal/purchase/errsummary.go index c2edf57fa..f218996ba 100644 --- a/internal/purchase/errsummary.go +++ b/internal/purchase/errsummary.go @@ -59,6 +59,19 @@ func SummarizeProviderError(err error) (category, message string) { return category, ErrCategoryMessage(category) } +// SummarizeFailure is SummarizeProviderError for errors that may also be a +// known server-authored refusal: those keep their own readable text, since +// the category message would hide what to fix. +func SummarizeFailure(err error) (category, message string) { + category, message = SummarizeProviderError(err) + if category == ErrCategoryUnknown { + if text, ok := ServerNoteMessage(err); ok { + message = text + } + } + return category, message +} + func classifyTypedError(err error) string { var apiErr interface{ ErrorCode() string } var azErr *azcore.ResponseError diff --git a/internal/scheduler/collection_error_summary_test.go b/internal/scheduler/collection_error_summary_test.go new file mode 100644 index 000000000..19837d9f8 --- /dev/null +++ b/internal/scheduler/collection_error_summary_test.go @@ -0,0 +1,39 @@ +package scheduler + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "google.golang.org/api/googleapi" +) + +// A failed provider's own error text carries project and account identifiers +// and request IDs; the stored last_collection_error and the collect result +// hold only a category message. +func TestCollectRecommendations_StoresSummarizedProviderError(t *testing.T) { + t.Setenv("GCP_PROJECT_ID", "proj-a") + raw := fmt.Errorf("get GCP recommendations: %w", &googleapi.Error{ + Code: 403, + Message: "Permission denied on resource project my-project-123456789012; request 11111111-2222-3333-4444-555555555555", + Errors: []googleapi.ErrorItem{{Reason: "forbidden", Message: "caller arn:aws:sts::123456789012:assumed-role/x/y"}}, + }) + s, store := newPartialSweepScheduler(t, "gcp", raw) + expectScheduledCollection(store, "gcp") + var stored string + store.On("SetRecommendationsCollectionError", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { stored = args.String(1) }).Return(nil).Once() + + result, err := s.CollectRecommendations(context.Background(), "") + + require.NoError(t, err) + for _, got := range []string{stored, result.FailedProviders["gcp"]} { + for _, needle := range []string{"my-project-123456789012", "11111111-2222", "arn:aws", "123456789012", "googleapi"} { + assert.NotContains(t, got, needle) + } + } + assert.Equal(t, "gcp: the cloud provider refused the purchase: CUDly's role is not allowed to make it in this account", stored) +} diff --git a/internal/scheduler/collection_stopped_notes_test.go b/internal/scheduler/collection_stopped_notes_test.go new file mode 100644 index 000000000..74d9814ff --- /dev/null +++ b/internal/scheduler/collection_stopped_notes_test.go @@ -0,0 +1,41 @@ +package scheduler + +import ( + "context" + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +// The fixed collection-stopped notes are shown as written on the read path. +func TestCollectionStoppedNotes_ShowAsWritten(t *testing.T) { + for _, n := range []string{collectionStoppedLoadingSettings, collectionStoppedIncomplete, collectionStoppedSaving} { + assert.Equal(t, n, purchase.SanitizeStoredError(n)) + assert.Equal(t, n+"; "+n, purchase.SanitizeStoredError(n+"; "+n)) + } +} + +// A failed rows write stores a fixed note; the database error text stays in +// the log. +func TestPersistCollection_StoresNoRawWriteError(t *testing.T) { + raw := fmt.Errorf("write recommendations: failed to connect to `host=db-prod.internal user=cudly_app database=cudly`: " + + "dial error (dial tcp 10.20.30.40:5432: connect: connection refused)") + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + store.On("UpsertRecommendations", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(raw).Once() + var stored string + store.On("SetRecommendationsCollectionError", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { stored = args.String(1) }).Return(nil).Once() + s := &Scheduler{config: store} + + s.persistCollection(context.Background(), nil, nil, nil, nil) + + assert.Equal(t, collectionStoppedSaving, stored) + assert.Equal(t, stored, purchase.SanitizeStoredError(stored)) + for _, needle := range []string{"db-prod.internal", "cudly_app", "10.20.30.40", "dial"} { + assert.NotContains(t, stored, needle) + } +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 1f1b52352..4fdba7fb2 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -372,7 +372,7 @@ func (s *Scheduler) collectAllProviders(ctx context.Context, globalCfg *config.G } if out.err != nil { logging.Errorf("Failed to collect %s recommendations: %v", providerName, out.err) - failedProviders[providerName] = out.err.Error() + _, failedProviders[providerName] = purchase.SummarizeFailure(out.err) continue } successfulProviders = append(successfulProviders, providerName) @@ -448,7 +448,7 @@ func (s *Scheduler) clearCollectionStartedBestEffort(ctx context.Context, ownerT func (s *Scheduler) persistCollection(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, failedProviders map[string]string, partial *config.RecommendationsCollectionPartial) { if err := s.upsertWithMarker(ctx, recs, successfulCollects, partial); err != nil { logging.Errorf("Failed to persist recommendations: %v", err) - if setErr := s.config.SetRecommendationsCollectionError(ctx, err.Error()); setErr != nil { + if setErr := s.config.SetRecommendationsCollectionError(ctx, collectionStoppedSaving); setErr != nil { logging.Errorf("Failed to record write error: %v", setErr) } return @@ -469,6 +469,7 @@ func (s *Scheduler) persistCollection(ctx context.Context, recs []config.Recomme const ( collectionStoppedLoadingSettings = "recommendations collection stopped before saving: could not load settings" collectionStoppedIncomplete = "recommendations collection stopped before saving: it was canceled or ran out of time" + collectionStoppedSaving = "recommendations collection stopped before saving: could not save the collected recommendations" ) // recordCollectionStoppedBestEffort records why a collection ended without diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index cbb886828..827c99f36 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -515,7 +515,8 @@ func TestScheduler_CollectRecommendations_UnknownProvider(t *testing.T) { assert.Contains(t, result.FailedProviders, "azure ") mockStore.AssertCalled(t, "SetRecommendationsCollectionError", mock.Anything, mock.AnythingOfType("string")) assert.Contains(t, recordedErr, `unknown provider "AWS"`) - assert.Contains(t, recordedErr, `unknown provider "azure "`) + assert.NotContains(t, recordedErr, `"azure "`, "a name outside the token alphabet shows only the fixed head") + assert.Contains(t, recordedErr, "azure : unknown provider") } func TestScheduler_CollectAWSRecommendations(t *testing.T) {