diff --git a/go.mod b/go.mod index 39546390..5bbd6e09 100644 --- a/go.mod +++ b/go.mod @@ -85,7 +85,7 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.4.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.4.0 github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261008100513-a9a426224031 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261009122502-bcc0dfc49227 github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007130138-e24345ce68f8 github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261009093408-2c18f303abfc github.com/aws/aws-lambda-go v1.47.0 diff --git a/go.sum b/go.sum index c8e552af..f46e925e 100644 --- a/go.sum +++ b/go.sum @@ -94,8 +94,8 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapp github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261008100513-a9a426224031 h1:e3DVjk0VIGfDTW3flfD7j6aDiae9T8JINQciL3IXPjo= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261008100513-a9a426224031/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695 h1:DfNEBzFS7/MaBZljLGRRRUaarfnXXZyBE/6iozzp9/k= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695/go.mod h1:d4nsy61/Ptib0SxqMg0ble3yksVm3+QbtUtmGja82PA= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261009122502-bcc0dfc49227 h1:S9pAwtTnwt02+Fo1uVWYgtmGyB1koQAHmBSficHOZ2E= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261009122502-bcc0dfc49227/go.mod h1:yJoe/zCikjjlGsy47BZTTuKUcLkLVDwqgLEYklWjlm4= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007130138-e24345ce68f8 h1:4qx5bagtW25H5X+whaZ/3tWXP7VGyG/qcypTh8yK7Hc= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007130138-e24345ce68f8/go.mod h1:iIvPuWLWd+SXqLHAQNdodYMUAAcq7PnwfScXWgARt5A= github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261009093408-2c18f303abfc h1:hCLUvpI5U42BRh4M9EvjKaBg70P2ijbKxO9eNn7wMK0= diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index 57687027..82132a4a 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -46,7 +46,7 @@ type scopeTestKind struct { // the post-scope constraint checks and reaches the mutation. func scopeTestRec(id string, account *string) config.RecommendationRecord { return config.RecommendationRecord{ - ID: id, Provider: "aws", Service: "ec2", Region: "us-east-1", + ID: id, Provider: "aws", Service: "rds", Region: "us-east-1", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 100, CloudAccountID: account, } } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index b1831712..232f14ca 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2013,9 +2013,14 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event // potentially a re-drive. purchase.RedriveRefusalReason is the same // predicate the reaper's automatic re-drive gates on: it returns a // reason exactly when the provider offers nothing that would collapse - // the second attempt onto the first, which today means Azure - // savings-plans (no server-side idempotency key, timestamp-derived - // order alias) plus any provider the predicate does not recognize. + // the second attempt onto the first, which today means AWS EC2 and + // Redshift (no client token; a lost response is reported as outcome + // unknown), Azure savings-plans (no server-side idempotency key, + // timestamp-derived order alias) plus any provider the predicate does + // not recognize. A failed EC2 or Redshift row therefore cannot be + // retried; the operator checks the account and starts a new purchase. + // If plan steps ever carry recommendations (platform#609), a failed EC2 + // step will be un-retryable too and needs its own recovery path. // // This gate runs FIRST and, unlike the threshold below, ?force=true // does NOT override it: a savings plan cannot be canceled, so there diff --git a/internal/api/handler_purchases_retry_fanout_test.go b/internal/api/handler_purchases_retry_fanout_test.go index bd584086..5449c500 100644 --- a/internal/api/handler_purchases_retry_fanout_test.go +++ b/internal/api/handler_purchases_retry_fanout_test.go @@ -56,7 +56,7 @@ func fanoutPlanAccounts() []config.CloudAccount { func fanoutRecs() []config.RecommendationRecord { return []config.RecommendationRecord{ { - Provider: "aws", Service: "ec2", ResourceType: "m5.large", + Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, Term: 1, UpfrontCost: 300, Selected: true, }, diff --git a/internal/api/handler_purchases_retry_redrive_test.go b/internal/api/handler_purchases_retry_redrive_test.go index d7b00137..a14f33cf 100644 --- a/internal/api/handler_purchases_retry_redrive_test.go +++ b/internal/api/handler_purchases_retry_redrive_test.go @@ -42,6 +42,8 @@ const ( redriveUnknownExecID = "22222222-3333-4444-5555-666666666604" redriveForceExecID = "22222222-3333-4444-5555-666666666605" redriveMixedExecID = "22222222-3333-4444-5555-666666666606" + redriveAWSNoTokenExecID = "22222222-3333-4444-5555-666666666607" + redriveAWSRDSExecID = "22222222-3333-4444-5555-666666666608" redriveLineageKey = "lineage-1668" ) @@ -248,7 +250,7 @@ func TestRetryOfLandedAzureSavingsPlanIsNotForceOverridable(t *testing.T) { // front of it. The AWS rec here is deliberately first, so a gate that checked // only the leading rec would pass this row straight through to Azure. func TestRetryOfMixedExecutionWithOneAzureSavingsPlanIsRefused(t *testing.T) { - failed := redriveFailedRow(redriveMixedExecID, "aws", "ec2") + failed := redriveFailedRow(redriveMixedExecID, "aws", "rds") failed.Recommendations = append(failed.Recommendations, config.RecommendationRecord{ Provider: "azure", Service: "savingsplans", @@ -301,3 +303,42 @@ func TestRetryOfFailedAzureReservationStillPurchasesOnce(t *testing.T) { assert.Equal(t, []string{common.DeriveIdempotencyToken(failed.IdempotencyKey, 0)}, tokens, "the retry must fire exactly one purchase, under the predecessor's token so Azure's two-step lookup dedupes it") } + +// TestRetryOfFailedAWSEC2OrRedshiftRowIsRefused is the MON-02 platform +// regression guard. EC2 and Redshift purchases take no client token, so the +// provider sends them once and reports a lost response as "purchase outcome +// unknown": the commitment may exist. Retrying such a row must not create a +// successor that buys again, whatever spelling the row carries (the canonical +// compute and data-warehouse slugs dispatch to the same clients as ec2 and +// redshift) and with ?force=true too. +func TestRetryOfFailedAWSEC2OrRedshiftRowIsRefused(t *testing.T) { + for _, tc := range []struct{ provider, service string }{ + {"aws", "ec2"}, {"aws", "compute"}, {"aws", "redshift"}, {"aws", "data-warehouse"}, {"", "ec2"}, + } { + tc := tc + t.Run(tc.provider+"/"+tc.service, func(t *testing.T) { + failed := redriveFailedRow(redriveAWSNoTokenExecID, tc.provider, tc.service) + failed.Error = "failed to purchase EC2 RI: purchase outcome unknown: the request may have been sent and the commitment may exist; reconcile before retrying" + + for _, req := range []*events.LambdaFunctionURLRequest{sessionRetryReq(), sessionRetryReqWithForce()} { + tokens, err := purchasesFiredByRetry(t, failed, req) + + assert.Empty(t, tokens, + "a retry of an EC2/Redshift row after an unknown outcome must buy nothing") + assertRedriveRefused(t, err, "Check EC2 Reserved Instances or Redshift Reserved Nodes") + } + }) + } +} + +// TestRetryOfFailedAWSRDSRowStillPurchasesOnce is the over-blocking control: +// RDS uses a deterministic reservation ID, so its retry is safe and must keep +// working. A predicate that refused every AWS row would pass the test above. +func TestRetryOfFailedAWSRDSRowStillPurchasesOnce(t *testing.T) { + failed := redriveFailedRow(redriveAWSRDSExecID, "aws", "rds") + + tokens, err := purchasesFiredByRetry(t, failed, sessionRetryReq()) + + require.NoError(t, err, "an AWS RDS retry is idempotent and must still be allowed") + assert.Len(t, tokens, 1, "the RDS retry must purchase exactly once") +} diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 2b7a9cac..fef11890 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3767,7 +3767,7 @@ func TestHandler_retryPurchase_Admin_AllowsAny(t *testing.T) { Status: "failed", Error: "send failed: transient SES throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} // Admin (Administrators-group member) modeled as a retry-any holder; the @@ -3790,7 +3790,7 @@ func TestHandler_retryPurchase_StoresHashOfEmailedToken(t *testing.T) { ExecutionID: retryExecID, Status: "failed", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100, CloudAccountID: &accountID}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100, CloudAccountID: &accountID}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} handler, mockConfig, _ := buildSessionRetryHandler(failed, session, true, false) @@ -3822,7 +3822,7 @@ func TestHandler_retryPurchase_RetryAny_AllowsAny(t *testing.T) { Status: "failed", Error: "send failed: transient SES throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "ops@example.com"} runSessionRetryAllowed(t, failed, session, true, false, sessionRetryReq()) @@ -3836,7 +3836,7 @@ func TestHandler_retryPurchase_RetryOwn_AllowsCreator(t *testing.T) { Error: "send failed: SES recipient mailbox full", CreatedByUserID: &creator, RetryAttemptN: 2, // already retried twice - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "u1@example.com"} newExec, updated := runSessionRetryAllowed(t, failed, session, false, true, sessionRetryReq()) @@ -3946,7 +3946,7 @@ func TestHandler_retryPurchase_PersistentFailure_NoMatch_AllowsRetry(t *testing. Status: "failed", Error: "send failed: SES throttle exceeded, please retry", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3960,7 +3960,7 @@ func TestHandler_retryPurchase_Threshold_BlocksAtFive_NoForce(t *testing.T) { Status: "failed", RetryAttemptN: 5, // already at the threshold CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3983,7 +3983,7 @@ func TestHandler_retryPurchase_Threshold_AllowsWithForce(t *testing.T) { Status: "failed", RetryAttemptN: 5, CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -3998,7 +3998,7 @@ func TestHandler_retryPurchase_JustUnderThreshold_AllowsNoForce(t *testing.T) { Status: "failed", RetryAttemptN: 4, // n=4 < threshold=5 → allowed CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -4060,7 +4060,7 @@ func TestHandler_retryPurchase_ConcurrentRetryRace_SecondCASLoses(t *testing.T) ExecutionID: retryExecID, Status: "failed", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). Both @@ -4114,7 +4114,7 @@ func TestHandler_retryPurchase_PreservesPlanMetadata(t *testing.T) { Status: "failed", Error: "send failed: transient SES throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID} // Caller owns the row; retry-own authorizes it (issue #907). @@ -4181,7 +4181,7 @@ func TestHandler_retryPurchase_PermissionConstraintsDenied(t *testing.T) { Error: "send failed: transient SES throttle", CreatedByUserID: &creator, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", Region: "us-east-1", Term: 1, UpfrontCost: 5000}, + {Provider: "aws", Service: "rds", Region: "us-east-1", Term: 1, UpfrontCost: 5000}, }, } session := &Session{UserID: retryCallerID, Email: "capped@example.com"} @@ -4232,7 +4232,7 @@ func TestPersistRetryExecution_ApprovalTokenNotUUID(t *testing.T) { Status: "failed", Error: "ses throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} // Caller owns the row; retry-own authorizes it (issue #907). @@ -4261,7 +4261,7 @@ func TestPersistRetryExecution_ApprovalTokenExpiresAtSet(t *testing.T) { Status: "failed", Error: "ses throttle", CreatedByUserID: &creator, - Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}}, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}}, } session := &Session{UserID: retryCallerID, Email: "admin@example.com"} diff --git a/internal/api/purchase_pricing.go b/internal/api/purchase_pricing.go index 333e7652..02f18e8d 100644 --- a/internal/api/purchase_pricing.go +++ b/internal/api/purchase_pricing.go @@ -264,10 +264,12 @@ func checkRequestDetailDiscriminators(req, match *config.RecommendationRecord, i // Platform/Scope/AZConfig/MemoryGB) precisely because its true // configuration was never recorded, so there is nothing known to // contradict. But once origin IS known, an empty match is ALSO a mismatch, -// not a pass: buildOfferingFilters substitutes the provider default for an -// empty match field, which can silently buy a different configuration than -// the one origin recorded (e.g. origin=dedicated tenancy, match=legacy- -// empty -> the purchase would resolve to the default-tenancy substitute). +// not a pass: a client may substitute a default for an empty match field, +// which can silently buy a different configuration than the one origin +// recorded (e.g. origin=dedicated tenancy, match=legacy-empty -> the purchase +// would resolve to the default-tenancy substitute). The AWS EC2 client +// rejects an empty Platform, Tenancy or Scope outright, but this check still +// has to refuse it here, before the purchase is attempted. // "Unknown" is therefore refused right alongside "different", not treated // as compatible. func purchaseDetailMismatch(origin, match common.ServiceDetails) string { diff --git a/internal/api/retry_carveout_test.go b/internal/api/retry_carveout_test.go index bd6e5db9..d89e0381 100644 --- a/internal/api/retry_carveout_test.go +++ b/internal/api/retry_carveout_test.go @@ -42,7 +42,7 @@ func retryCarveoutFailedExec() *config.PurchaseExecution { Error: "send failed: transient SES throttle", CreatedByUserID: &creator, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", Term: 1, UpfrontCost: 100}, + {Provider: "aws", Service: "rds", Term: 1, UpfrontCost: 100}, }, } } diff --git a/internal/purchase/armed_redrive_test.go b/internal/purchase/armed_redrive_test.go index 7c6fb635..79f61edf 100644 --- a/internal/purchase/armed_redrive_test.go +++ b/internal/purchase/armed_redrive_test.go @@ -217,12 +217,13 @@ func TestFirstAzureSavingsPlanPurchaseStillExecutes(t *testing.T) { } // TestSafeProviderRetrySuccessorStillExecutes is the second negative control: -// the guard must be as narrow as the provider gap. An AWS retry successor -// reproduces its ClientToken, so the provider collapses a re-drive onto the -// original and the retry must still execute. +// the guard must be as narrow as the provider gap. An AWS RDS retry successor +// reproduces its deterministic reservation ID, so the provider collapses a +// re-drive onto the original and the retry must still execute. EC2 and +// Redshift are the AWS services that lack such a guard (see the Refuses tests). func TestSafeProviderRetrySuccessorStillExecutes(t *testing.T) { - exec := armedExecution("aws", "ec2", 3, "approved") - mgr, store, rec := armedHarness(t, common.ServiceEC2) + exec := armedExecution("aws", "rds", 3, "approved") + mgr, store, rec := armedHarness(t, common.ServiceRDS) store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, []string{"approved", "pending", "notified"}, "running") @@ -231,7 +232,7 @@ func TestSafeProviderRetrySuccessorStillExecutes(t *testing.T) { require.NoError(t, mgr.ProcessMessage(context.Background(), string(body))) assert.Equal(t, 1, rec.count(), - "an AWS retry successor dedupes at the provider via ClientToken and must still execute; the guard must not widen beyond the providers that lack a duplicate guard") + "an AWS RDS retry successor dedupes at the provider via its reservation ID and must still execute; the guard must not widen beyond the providers that lack a duplicate guard") } // TestArmedRowIsDefusedNotLeftArmed pins the disposition of a refused row. A diff --git a/internal/purchase/aws_no_token_redrive_test.go b/internal/purchase/aws_no_token_redrive_test.go new file mode 100644 index 00000000..d187a832 --- /dev/null +++ b/internal/purchase/aws_no_token_redrive_test.go @@ -0,0 +1,161 @@ +package purchase + +import ( + "context" + "encoding/json" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// --- MON-02: AWS EC2 and Redshift have no duplicate guard -------------- +// +// The EC2 and Redshift purchase APIs take no client token or caller ID, so the +// provider sends them once and returns "purchase outcome unknown" when the +// response is lost. The commitment may exist, so nothing may buy again until +// someone has checked the account. These tests pin where that refusal reaches. + +// awsNoTokenSlugs are the rec.Service spellings that dispatch to the AWS EC2 or +// Redshift purchase clients (mapServiceSlug: canonical and legacy). +var awsNoTokenSlugs = []string{"ec2", "compute", "redshift", "data-warehouse"} + +// TestAWSRedriveGuardReachMatchesDispatchReach states the guard as an iff +// against dispatch, like the Azure savings-plans test: a value that dispatches +// to the EC2/Redshift client but is not refused is a double-buy hole. +func TestAWSRedriveGuardReachMatchesDispatchReach(t *testing.T) { + m := NewManager(ManagerConfig{}) + + candidates := []string{ + "ec2", "compute", "redshift", "data-warehouse", + "rds", "relational-db", "elasticache", "cache", "opensearch", "search", "memorydb", + "savings-plans", "savingsplans", "savings-plans-compute", "savingsplans-ec2instance", + string(common.ServiceEC2), string(common.ServiceCompute), string(common.ServiceRedshift), + string(common.ServiceDataWarehouse), string(common.ServiceRDS), string(common.ServiceNoSQL), + "EC2", "Compute", " ec2", "ec2 ", "ec2\n", "Redshift", "data_warehouse", "", "unknown", + } + + for _, provider := range []string{"aws", ""} { + for _, service := range candidates { + provider, service := provider, service + t.Run("provider="+provider+"/service="+service, func(t *testing.T) { + svc := m.mapServiceType(service) + dispatchesToNoTokenClient := svc == common.ServiceEC2 || svc == common.ServiceCompute || + svc == common.ServiceRedshift || svc == common.ServiceDataWarehouse + refused := recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}) != "" + + assert.Equal(t, dispatchesToNoTokenClient, refused, + "guard reach and dispatch reach must match for %q: dispatches=%v refused=%v", service, dispatchesToNoTokenClient, refused) + }) + } + } +} + +// TestAWSRedriveRefusalSetIsExact is the closed-set form, with negative +// controls so a predicate that refuses every AWS row (or every service named +// compute on any provider) fails here. +func TestAWSRedriveRefusalSetIsExact(t *testing.T) { + for _, provider := range []string{"aws", ""} { + for _, service := range awsNoTokenSlugs { + reason := recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}) + assert.Contains(t, reason, "Check EC2 Reserved Instances or Redshift Reserved Nodes", + "provider %q service %q has no duplicate guard and must be refused with the operator action", provider, service) + } + for _, service := range []string{"rds", "relational-db", "elasticache", "cache", "memorydb", "opensearch", "search", "savingsplans", "savings-plans"} { + assert.Empty(t, recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}), + "provider %q service %q has a provider-side duplicate guard and must stay retryable", provider, service) + } + } + // Same slugs on other providers keep their own rules. + for _, provider := range []string{"azure", "gcp"} { + for _, service := range []string{"compute", "data-warehouse"} { + assert.Empty(t, recRedriveRefusalReason(config.RecommendationRecord{Provider: provider, Service: service}), + "%s %s is not an AWS no-token path and must stay retryable", provider, service) + } + } + // Any one unsafe rec makes the whole execution unsafe. + mixed := &config.PurchaseExecution{Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "rds"}, {Provider: "aws", Service: "redshift"}, + }} + assert.NotEmpty(t, RedriveRefusalReason(mixed)) + assert.False(t, allRecsSafeToRedrive(&config.PurchaseExecution{ExecutionID: "x", Recommendations: mixed.Recommendations})) +} + +// TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes drives the real +// executor funnel: a retry successor (RetryAttemptN > 0) of an EC2/Redshift +// purchase buys nothing and is persisted as failed with the reason, while a +// first purchase of the same service still buys exactly once. +func TestAWSNoTokenRetrySuccessorIsRefusedAndFirstBuyStillExecutes(t *testing.T) { + m := NewManager(ManagerConfig{}) + for _, service := range awsNoTokenSlugs { + service := service + t.Run(service, func(t *testing.T) { + svcType := m.mapServiceType(service) + + retry := armedExecution("aws", service, 2, "approved") + mgr, store, rec := armedHarness(t, svcType) + var saved []config.PurchaseExecution + store.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + saved = append(saved, *e) + return nil + } + store.On("GetExecutionByID", mock.Anything, retry.ExecutionID).Return(retry, nil).Maybe() + expectClaim(store, retry, []string{"approved", "pending", "notified"}, "running") + body, err := json.Marshal(AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: retry.ExecutionID}) + require.NoError(t, err) + _ = mgr.ProcessMessage(context.Background(), string(body)) + + assert.Equal(t, 0, rec.count(), "a retry after a possibly-landed %s purchase must not buy again", service) + require.NotEmpty(t, saved) + final := saved[len(saved)-1] + assert.Equal(t, "failed", final.Status) + assert.Contains(t, final.Error, "Check EC2 Reserved Instances or Redshift Reserved Nodes") + + first := armedExecution("aws", service, 0, "approved") + mgr, store, rec = armedHarness(t, svcType) + store.On("GetExecutionByID", mock.Anything, first.ExecutionID).Return(first, nil).Maybe() + expectClaim(store, first, []string{"approved", "pending", "notified"}, "running") + require.NoError(t, mgr.ProcessMessage(context.Background(), string(body))) + assert.Equal(t, 1, rec.count(), "a first %s purchase must still go through; the guard is for retries only", service) + }) + } +} + +// TestRecoverStrandedApprovals_EC2RowIsFailedNotRedriven pins the sweep: a +// stranded approved EC2 row may have bought before the run died, so the sweep +// must make zero provider calls and tell the operator to check the account. +func TestRecoverStrandedApprovals_EC2RowIsFailedNotRedriven(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockFactory := new(MockProviderFactory) // any CreateAndValidateProvider call would panic: no expectation set + + stranded := config.PurchaseExecution{ + ExecutionID: "exec-ec2-stranded", + PlanID: "plan-ec2", + Status: "approved", + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200, Selected: true}, + }, + } + failedRow := stranded + failedRow.Status = "failed" + mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold).Return([]config.PurchaseExecution{stranded}, nil) + mockStore.On("TransitionExecutionStatus", ctx, "exec-ec2-stranded", []string{"approved"}, "failed", (*string)(nil)).Return(&failedRow, nil) + var saved *config.PurchaseExecution + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }).Return(nil) + + mgr := &Manager{config: mockStore, providerFactory: mockFactory, dashboardURL: "https://dashboard.example.com"} + recovered, err := mgr.RecoverStrandedApprovals(ctx) + + require.NoError(t, err) + assert.Equal(t, 1, recovered) + require.NotNil(t, saved) + assert.Equal(t, "failed", saved.Status) + assert.Contains(t, saved.Error, "start a new purchase") + assert.NotContains(t, saved.Error, "then Retry", "Retry is refused for this row, so the message must not point at it") + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) +} diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 20d6f6b9..7bbb2d3f 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1104,9 +1104,11 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen // // Legacy rows persisted before #453 carry an empty rec.Details — // DecodeServiceDetailsFor returns a zero-valued typed pointer for - // those, and the cloud client's buildOfferingFilters substitutes - // defaults (Platform=Linux/UNIX, Tenancy=default, etc.). Engine is - // preserved on the record column too, so we use it as a last-resort + // those. The AWS EC2 client does not fill the gaps: an empty Platform + // or an empty or unknown Tenancy or Scope is rejected before the request + // is sent (providers/aws ec2/client.go buildEC2OfferingQuery), so the + // purchase fails loudly instead of buying a default configuration. Engine + // is preserved on the record column too, so we use it as a last-resort // fallback for DB/Cache services to avoid silently mis-purchasing a // legacy non-default-engine rec as the default engine. details, detailsErr := common.DecodeServiceDetailsFor(rec.Service, rec.Details) diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 3d1630e4..9d48f57a 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -374,7 +374,9 @@ func armedRedriveRefusal(exec *config.PurchaseExecution) error { // canceled. // // Safe providers / services (issue #639): -// - AWS (all services): tag-guard or ClientToken deduplication (#636/#638). +// - AWS RDS, ElastiCache, MemoryDB, OpenSearch and Savings Plans: a +// deterministic reservation ID or ClientToken collapses a repeat onto the +// first attempt (#636/#638). // - Azure reservations (compute, relational-db, cache, nosql, memorydb, // search, data-warehouse): DoIdempotentPurchaseTwoStep performs a // tag-based lookup before purchasing (#729 / #721). @@ -382,6 +384,10 @@ func armedRedriveRefusal(exec *config.PurchaseExecution) error { // the token (#654). // // NOT safe: +// - AWS EC2 and Redshift (slugs ec2, compute, redshift, data-warehouse): the +// purchase APIs take no client token, so a lost response may have bought the +// commitment and a second attempt would buy another (MON-02). The provider +// sends the call once and reports "purchase outcome unknown". // - Azure savings-plans: the OrderAlias API uses time.Now().UnixNano() as // the alias name; there is no server-side idempotency key and no // tag-based lookup implemented yet. Re-driving would create a duplicate @@ -415,7 +421,15 @@ func RedriveRefusalReason(exec *config.PurchaseExecution) string { func recRedriveRefusalReason(rec config.RecommendationRecord) string { switch rec.Provider { case "", "aws": - // Empty provider is legacy AWS. All AWS services honor IdempotencyToken. + // Empty provider is legacy AWS. RDS, ElastiCache, MemoryDB, OpenSearch + // and Savings Plans collapse a repeat onto the first attempt. EC2 and + // Redshift cannot: their purchase APIs take no client token or caller + // ID, and the provider sends them once and reports a lost response as + // "purchase outcome unknown". The commitment may exist, so a second + // attempt must not be made until someone has checked the account. + if awsPurchaseHasNoDuplicateGuard(rec.Service) { + return "AWS EC2 and Redshift purchases have no provider-side duplicate guard, and a lost response means the earlier attempt may already have bought the commitment. Check EC2 Reserved Instances or Redshift Reserved Nodes in the target account and region; if it is missing, start a new purchase instead of retrying" + } return "" case "azure": // Azure savings-plans uses a timestamp-based alias name and has no @@ -434,6 +448,25 @@ func recRedriveRefusalReason(rec config.RecommendationRecord) string { } } +// awsPurchaseHasNoDuplicateGuard reports whether a recommendation's service +// slug dispatches to the AWS EC2 or Redshift purchase path. The canonical +// slugs (compute, data-warehouse) and the legacy ones (ec2, redshift) reach the +// same client, so all four must be covered or the refusal can be bypassed by +// spelling. Matching goes through mapServiceSlug, the same table the executor +// dispatches on, so the two cannot drift. +func awsPurchaseHasNoDuplicateGuard(service string) bool { + svc, ok := mapServiceSlug(service) + if !ok { + return false + } + switch svc { + case common.ServiceEC2, common.ServiceCompute, common.ServiceRedshift, common.ServiceDataWarehouse: + return true + default: + return false + } +} + // claimAndRedrive atomically claims a stranded execution (by transitioning its // status from "approved" to "running") and then re-drives it via // executeAndFinalize. It is extracted from RecoverStrandedApprovals to keep that @@ -537,7 +570,11 @@ func (m *Manager) safeFail(ctx context.Context, exec *config.PurchaseExecution) return false, fmt.Errorf("failed to transition stranded execution %s to failed: %w", exec.ExecutionID, txErr) } - updated.Error = "execution was approved but its purchase run was interrupted before completing and never finalized; failed by the recovery sweep so it is not silently stuck (issue #632). Verify on the cloud provider that no commitment was created, then Retry." + next := "Verify on the cloud provider that no commitment was created, then Retry." + if RedriveRefusalReason(exec) != "" { + next = "Retry is refused for this purchase because a repeat could buy a second commitment. Verify on the cloud provider whether the commitment was created; if it was not, start a new purchase." + } + updated.Error = "execution was approved but its purchase run was interrupted before completing and never finalized; failed by the recovery sweep so it is not silently stuck (issue #632). " + next if saveErr := m.config.SavePurchaseExecution(ctx, updated); saveErr != nil { // The atomic flip to "failed" already landed via TransitionExecutionStatus; // only the explanatory error string failed to persist. Log loudly but diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index 9ccd7b76..1920bdb6 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -599,7 +599,7 @@ func TestManager_RecoverStrandedApprovals_AWSOnlyRedrives(t *testing.T) { Status: "approved", StepNumber: 1, Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, }, } runningRow := stranded @@ -634,7 +634,7 @@ func TestManager_RecoverStrandedApprovals_AWSOnlyRedrives(t *testing.T) { // executeSinglePurchase wraps ctx in a per-rec WithTimeout before calling // CreateAndValidateProvider, so we must match any context, not ctx itself. mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) - mockProvider.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) + mockProvider.On("GetServiceClient", mock.Anything, common.ServiceRDS, "us-east-1").Return(mockServiceClient, nil) mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ Success: true, CommitmentID: "ri-idempotent-12345", @@ -1097,7 +1097,7 @@ func TestManager_RecoverStrandedApprovals_AWSRedrive_PersistenceFailurePropagate PlanID: "plan-aws-persist-456", Status: "approved", Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, }, } runningRow := stranded @@ -1130,7 +1130,7 @@ func TestManager_RecoverStrandedApprovals_AWSRedrive_PersistenceFailurePropagate Account: aws.String("123456789012"), }, nil) mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) - mockProvider.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) + mockProvider.On("GetServiceClient", mock.Anything, common.ServiceRDS, "us-east-1").Return(mockServiceClient, nil) mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ Success: true, CommitmentID: "ri-persist-fail-12345", @@ -1178,7 +1178,7 @@ func TestManager_RecoverStrandedApprovals_AWSRedrive_ExecAndPersistBothFail(t *t PlanID: "plan-aws-both-fail", Status: "approved", Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 200.0, Selected: true, Purchased: false}, }, } runningRow := stranded @@ -1240,7 +1240,7 @@ func TestManager_RecoverStrandedApprovals_AWSClaimLost_NoRedrive(t *testing.T) { PlanID: "plan-aws-claimed", Status: "approved", Recommendations: []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300.0, Selected: true}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300.0, Selected: true}, }, } diff --git a/internal/purchase/ramp_step_progress_integration_test.go b/internal/purchase/ramp_step_progress_integration_test.go index 742f48f6..f2b4b929 100644 --- a/internal/purchase/ramp_step_progress_integration_test.go +++ b/internal/purchase/ramp_step_progress_integration_test.go @@ -140,7 +140,7 @@ func newRampStepFixture(ctx context.Context, t *testing.T) *rampStepFixture { mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.AnythingOfType("email.NotificationData")).Return(nil).Maybe() mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil).Maybe() - mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceEC2, mock.Anything).Return(mockServiceClient, nil).Maybe() + mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceRDS, mock.Anything).Return(mockServiceClient, nil).Maybe() mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.Anything, mock.AnythingOfType("common.PurchaseOptions")). Run(func(args mock.Arguments) { opts := args.Get(2).(common.PurchaseOptions) @@ -195,7 +195,7 @@ func (f *rampStepFixture) purchaseCount() int { // rampStepRecommendation is the single rec every execution in these tests buys. func rampStepRecommendation() []config.RecommendationRecord { return []config.RecommendationRecord{ - {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Selected: true}, } } diff --git a/internal/purchase/reaper_test.go b/internal/purchase/reaper_test.go index 3969904e..50606420 100644 --- a/internal/purchase/reaper_test.go +++ b/internal/purchase/reaper_test.go @@ -36,10 +36,21 @@ func stuckExec(id, status string) config.PurchaseExecution { } } -// stuckExecWithAWSRecs builds a stuck execution with a single AWS EC2 rec. -// allRecsSafeToRedrive returns true for pure-AWS recs, so the reaper's -// canonical error includes "safe to retry" (F4 fix). +// stuckExecWithAWSRecs builds a stuck execution with a single AWS RDS rec. +// RDS honors a deterministic reservation ID, so allRecsSafeToRedrive is true and +// the reaper's canonical error includes "safe to retry" (F4 fix). EC2 and +// Redshift are the AWS services that are not safe; see stuckExecWithAWSEC2Rec. func stuckExecWithAWSRecs(id, status string) config.PurchaseExecution { + e := stuckExec(id, status) + e.Recommendations = []config.RecommendationRecord{ + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Count: 1}, + } + return e +} + +// stuckExecWithAWSEC2Rec is the unsafe AWS case: EC2 purchases take no client +// token, so a stuck row may already have bought the commitment. +func stuckExecWithAWSEC2Rec(id, status string) config.PurchaseExecution { e := stuckExec(id, status) e.Recommendations = []config.RecommendationRecord{ {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Count: 1}, @@ -81,6 +92,33 @@ func TestReapStuckExecutions_StaleApprovedFlippedToFailed(t *testing.T) { store.AssertExpectations(t) } +// TestReapStuckExecutions_EC2RowIsNotAdvertisedAsSafeToRetry: a reaped EC2 row +// may have bought before the run died, and Retry is refused for it, so the +// canonical error must not say "safe to retry". The RDS test above is the +// positive control that keeps an always-refusing predicate from passing. +func TestReapStuckExecutions_EC2RowIsNotAdvertisedAsSafeToRetry(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + reapAfter := 10 * time.Minute + + row := stuckExecWithAWSEC2Rec("exec-ec2", "running") + transitioned := row + transitioned.Status = failedStatus + store.On("ListStuckExecutions", ctx, stuckStatuses, reapAfter).Return([]config.PurchaseExecution{row}, nil) + store.On("TransitionExecutionStatus", ctx, "exec-ec2", stuckStatuses, failedStatus, (*string)(nil)).Return(&transitioned, nil) + var saved *config.PurchaseExecution + store.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }).Return(nil) + + result, err := newReaperManager(store).ReapStuckExecutions(ctx, reapAfter) + + require.NoError(t, err) + assert.Equal(t, 1, result.Reaped) + require.NotNil(t, saved) + assert.Contains(t, saved.Error, "reaped after") + assert.NotContains(t, saved.Error, "safe to retry") +} + func TestReapStuckExecutions_StaleRunningFlippedToFailed(t *testing.T) { ctx := context.Background() store := new(MockConfigStore)