diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index e3a99959..3a9bb094 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -42,6 +42,8 @@ var serverNotePatterns = []string{ regexp.QuoteMeta(redshiftNoJournalRefusal), regexp.QuoteMeta(redshiftRecentBuyRefusal), regexp.QuoteMeta(redshiftSettleRefusal), + regexp.QuoteMeta(gcpDuplicateCheckFailedNote), + regexp.QuoteMeta(gcpRecentCommitmentNote), `EC2 recommendation for ` + resourceTypePattern + ` has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation`, `zonal RI offering needs an availability zone; re-fetch the recommendation`, `email notifier not configured for this deployment`, diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 88f6b68f..a080a761 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -61,6 +61,8 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal, + gcpDuplicateCheckFailedNote, + gcpRecentCommitmentNote, "EC2 recommendation for m5.large has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation", "zonal RI offering needs an availability zone; re-fetch the recommendation", "email notifier not configured for this deployment", @@ -213,7 +215,7 @@ func TestSanitizeStoredError_ResolveNoteExchangeIDIsClosed(t *testing.T) { // The Redshift gate texts are fixed: an appended execution id or raw text must // not pass verbatim. func TestSanitizeStoredError_RedshiftGateRefusalsAreClosed(t *testing.T) { - for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal} { + for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote} { assert.Equal(t, text, SanitizeStoredError(text)) for _, variant := range []string{ text + " (execution 00000000-0000-0000-0000-000000000000, running)", diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 2cfb48bb..2a986d61 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -781,6 +781,7 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf // Build the list of selected indices once so the fan-out closure only // has to look up rec[i] (no second pass over the full slice). selected := selectedIndices(exec.Recommendations) + ownGCPNames := gcpOwnCommitmentNames(exec, selected) if len(selected) == 0 { logging.Infof("purchase[%s]: no selected recommendations, nothing to execute (account=%s plan=%q)", exec.ExecutionID, accountID, plan.Name) @@ -833,7 +834,7 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf recOpts := opts recOpts.IdempotencyToken = common.DeriveIdempotencyToken(idempotencyLineageKey(exec), i) recOpts.AttemptJournal = m.attemptJournal - purchaseResult, err := m.executeSinglePurchase(ctx, rec, provCfg, recOpts) + purchaseResult, err := m.executeSinglePurchase(ctx, rec, provCfg, recOpts, ownGCPNames) return recPurchaseOutcome{index: i, purchase: purchaseResult, err: err}, nil }, getMaxAccountParallelism()) @@ -1247,7 +1248,7 @@ func logRecCtxErr(executionID, recTuple string, elapsed time.Duration, recCtxErr // provCfg carries optional per-account credentials; pass nil to use ambient credentials. // opts carries execution-level metadata (the source surface) that providers stamp // onto the commitment they create. -func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.RecommendationRecord, provCfg *provider.Config, opts common.PurchaseOptions) (common.PurchaseResult, error) { +func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.RecommendationRecord, provCfg *provider.Config, opts common.PurchaseOptions, ownGCPNames map[string]bool) (common.PurchaseResult, error) { // Per-purchase Info logs tagged with the owning execution ID so a // CloudWatch filter on the execution UUID surfaces every step of the // purchase attempt -- provider construction, service-client lookup, @@ -1345,13 +1346,11 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen // CloudWatch shows whether a failure came from the SDK call itself // (which the AWS SDK retries up to 3× × 30s by default) versus // something earlier in the flow — issue #667. - tCall := time.Now() - // go's Redshift dedupe bounds its DescribeReservedNodes match window by RequestedAt, which must - // be the lineage's first dispatch time, never this call's. - opts, err = withDispatchTime(rec, opts) + opts, err = preSubmitChecks(recCtx, serviceClient, rec, recommendation, opts, ownGCPNames) if err != nil { return common.PurchaseResult{}, err } + tCall := time.Now() result, err := serviceClient.PurchaseCommitment(recCtx, recommendation, opts) elapsed := time.Since(tCall) if err != nil { @@ -1381,6 +1380,18 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen return result, nil } +// preSubmitChecks runs what must hold before PurchaseCommitment: the GCP +// recent-commitment refusal (platform#682, a beforeSubmission error) and go's +// Redshift dispatch time, which bounds its DescribeReservedNodes match window +// by RequestedAt and must be the lineage's first dispatch time, never this +// call's. +func preSubmitChecks(ctx context.Context, client provider.ServiceClient, rec config.RecommendationRecord, recommendation common.Recommendation, opts common.PurchaseOptions, ownGCPNames map[string]bool) (common.PurchaseOptions, error) { + if err := gcpRecentCommitmentRefusal(ctx, client, recommendation, ownGCPNames); err != nil { + return opts, err + } + return withDispatchTime(rec, opts) +} + // purchaseCallError wraps a failed PurchaseCommitment. When the run context // ended mid-call the request may have landed, so the error also carries // ErrOutcomeUnknown: the row records an unknown outcome, never a plain failure. diff --git a/internal/purchase/gcp_recent_check.go b/internal/purchase/gcp_recent_check.go new file mode 100644 index 00000000..fe6be1b4 --- /dev/null +++ b/internal/purchase/gcp_recent_check.go @@ -0,0 +1,131 @@ +package purchase + +import ( + "context" + "errors" + "fmt" + "slices" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Fixed refusal notes (platform#682). Both are stored verbatim and are on the +// stored-error allowlist (errnotes.go), so neither may carry provider text. +const ( + gcpDuplicateCheckFailedNote = "GCP duplicate check could not list existing commitments; no purchase was submitted" + gcpRecentCommitmentNote = "a GCP commitment for this project, region and machine family started in the last 24 hours; no purchase was submitted, to avoid a duplicate" +) + +// gcpCommitmentNameTokenLen mirrors computeengine idempotentNameTokenLen +// (providers/gcp/services/computeengine/client.go:1411). +const gcpCommitmentNameTokenLen = 32 + +// gcpCommitmentName mirrors computeengine idempotentCommitmentName +// (client.go:1423-1434) for a lowercase hex token: "cud-" + token[:32]. The +// parity test drives the real client, so a rule change there fails it. +func gcpCommitmentName(token string) string { + if len(token) > gcpCommitmentNameTokenLen { + token = token[:gcpCommitmentNameTokenLen] + } + return "cud-" + token +} + +func isGCPCompute(providerName string, service common.ServiceType) bool { + return common.ProviderType(providerName) == common.ProviderGCP && service == common.ServiceCompute +} + +// gcpOwnCommitmentNames returns the commitment names this execution's selected +// GCP compute recs will create, built from the same per-rec tokens the purchase +// path passes to the provider. A listed commitment with one of these names was +// created by this lineage (a lost-response re-drive, or an earlier rec of the +// same run), so it is not a duplicate of the rec being bought. +func gcpOwnCommitmentNames(exec *config.PurchaseExecution, selected []int) map[string]bool { + names := make(map[string]bool) + for _, i := range selected { + rec := exec.Recommendations[i] + service, _ := mapServiceSlug(rec.Service) + if !isGCPCompute(rec.Provider, service) { + continue + } + names[gcpCommitmentName(common.DeriveIdempotencyToken(idempotencyLineageKey(exec), i))] = true + } + return names +} + +// recentCommitmentFilter is the optional GCP filter recfilter asserts on the +// client it lists through (pkg/recfilter/dedupe.go:16-18, :71). +type recentCommitmentFilter interface { + FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error) +} + +// gcpRecentDedupeClient lists without this execution's own commitments and +// forwards the GCP filter. Embedding the interface does not promote +// FilterRecommendationsForRecentCommitments, and without it recfilter would +// fall back to its generic map path, which does not understand CUDs. +type gcpRecentDedupeClient struct { + provider.ServiceClient + own map[string]bool +} + +func (c gcpRecentDedupeClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { + all, err := c.ServiceClient.GetExistingCommitments(ctx) + if err != nil { + return nil, err + } + return slices.DeleteFunc(slices.Clone(all), func(cm common.Commitment) bool { return c.own[cm.CommitmentID] }), nil +} + +// FilterRecommendationsForRecentCommitments stamps the project onto the recs +// from the listed commitments (each carries Account = the client's project) and +// delegates. The platform does not know the project in ADC mode, so the rec +// cannot carry it itself. +func (c gcpRecentDedupeClient) FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error) { + inner, ok := c.ServiceClient.(recentCommitmentFilter) + if !ok { + return nil, nil, errors.New("GCP service client has no recent-commitment filter") + } + if len(existing) == 0 { + return nil, nil, errors.New("no recent commitments to filter against") + } + project := existing[0].Account + for i := range existing { + if existing[i].Account != project { + return nil, nil, fmt.Errorf("recent commitments span projects %q and %q", project, existing[i].Account) + } + } + stamped := slices.Clone(recs) + for i := range stamped { + stamped[i].Account = project + } + return inner.FilterRecommendationsForRecentCommitments(stamped, existing) +} + +// gcpRecentCommitmentRefusal refuses a GCP compute rec when the project already +// has a commitment of the rec's region and machine family that started in the +// last 24 hours, and fails closed when the commitments cannot be listed. It +// returns nil for every other rec. A refusal is a beforeSubmission error: the +// purchase call is never made. +func gcpRecentCommitmentRefusal(ctx context.Context, client provider.ServiceClient, rec common.Recommendation, ownNames map[string]bool) error { + if !isGCPCompute(string(rec.Provider), rec.Service) { + return nil + } + rec.CommitmentType = common.CommitmentCUD + passed, filtered, err := recfilter.NewDuplicateChecker(0).AdjustRecommendationsForExisting( + ctx, []common.Recommendation{rec}, gcpRecentDedupeClient{ServiceClient: client, own: ownNames}) + switch { + case err != nil: + logging.Errorf("GCP duplicate check for %s in %s failed: %v", rec.ResourceType, rec.Region, err) + return beforeSubmission(errors.New(gcpDuplicateCheckFailedNote)) + case len(filtered) > 0: + return beforeSubmission(errors.New(gcpRecentCommitmentNote)) + case len(passed) != 1: + logging.Errorf("GCP duplicate check for %s in %s returned %d passed recs, want 1", rec.ResourceType, rec.Region, len(passed)) + return beforeSubmission(errors.New(gcpDuplicateCheckFailedNote)) + } + return nil +} diff --git a/internal/purchase/gcp_recent_check_test.go b/internal/purchase/gcp_recent_check_test.go new file mode 100644 index 00000000..0fe9e0a0 --- /dev/null +++ b/internal/purchase/gcp_recent_check_test.go @@ -0,0 +1,406 @@ +package purchase + +// #682 P1: a GCP compute rec is refused before PurchaseCommitment when the +// project already holds a same-region, same-family commitment that started in +// the last 24 hours, and fails closed when listing errors. The real library GCP +// client runs against a fake commitments service, so the dedupe, the filter and +// the Insert name are the pinned library's own. + +import ( + "context" + "errors" + "net/http" + "sync" + "testing" + "time" + + "cloud.google.com/go/compute/apiv1/computepb" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/services/computeengine" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "google.golang.org/api/googleapi" + "google.golang.org/api/iterator" + "google.golang.org/protobuf/proto" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +const ( + cudProject = "proj" + cudRegion = "us-central1" +) + +var ( + familyN2 = computepb.Commitment_GENERAL_PURPOSE_N2.String() + familyE2 = computepb.Commitment_GENERAL_PURPOSE_E2.String() +) + +type cudIter struct { + items []*computepb.Commitment + err error +} + +func (it *cudIter) Next() (*computepb.Commitment, error) { + if it.err != nil { + return nil, it.err + } + if len(it.items) == 0 { + return nil, iterator.Done + } + next := it.items[0] + it.items = it.items[1:] + return next, nil +} + +// fakeCUDService stores commitments like the API: Insert adds a live commitment +// and answers 409 for a name that already exists. +type fakeCUDService struct { + mu sync.Mutex + items []*computepb.Commitment + listErr error + inserted []string +} + +func (f *fakeCUDService) List(context.Context, *computepb.ListRegionCommitmentsRequest) computeengine.CommitmentsIterator { + f.mu.Lock() + defer f.mu.Unlock() + return &cudIter{items: append([]*computepb.Commitment(nil), f.items...), err: f.listErr} +} + +func (f *fakeCUDService) find(name string) *computepb.Commitment { + for _, c := range f.items { + if c.GetName() == name { + return c + } + } + return nil +} + +func (f *fakeCUDService) Insert(_ context.Context, req *computepb.InsertRegionCommitmentRequest) (computeengine.CommitmentsOperation, error) { + f.mu.Lock() + defer f.mu.Unlock() + res := req.GetCommitmentResource() + f.inserted = append(f.inserted, res.GetName()) + if f.find(res.GetName()) != nil { + return nil, &googleapi.Error{Code: http.StatusConflict} + } + stored := proto.Clone(res).(*computepb.Commitment) + stored.Status = proto.String(computepb.Commitment_ACTIVE.String()) + stored.StartTimestamp = proto.String(time.Now().UTC().Format(time.RFC3339)) + f.items = append(f.items, stored) + return fakeCommitmentOp{}, nil +} + +func (f *fakeCUDService) Get(_ context.Context, req *computepb.GetRegionCommitmentRequest) (*computepb.Commitment, error) { + f.mu.Lock() + defer f.mu.Unlock() + if c := f.find(req.GetCommitment()); c != nil { + return c, nil + } + return nil, errors.New("not found") +} + +func (f *fakeCUDService) Close() error { return nil } + +func (f *fakeCUDService) insertCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.inserted) +} + +// cudFixture is a 4 vCPU / 16 GB, 12-month commitment, matching what a +// cudRec (term 1) asks Insert for. +func cudFixture(name, family string, age time.Duration) *computepb.Commitment { + return &computepb.Commitment{ + Name: proto.String(name), + Plan: proto.String(computepb.Commitment_TWELVE_MONTH.String()), + Type: proto.String(family), + Status: proto.String(computepb.Commitment_ACTIVE.String()), + StartTimestamp: proto.String(time.Now().Add(-age).UTC().Format(time.RFC3339)), + Resources: []*computepb.ResourceCommitment{ + {Type: proto.String(computepb.ResourceCommitment_VCPU.String()), Amount: proto.Int64(4)}, + {Type: proto.String(computepb.ResourceCommitment_MEMORY.String()), Amount: proto.Int64(16384)}, + }, + } +} + +func cudRec(t *testing.T, family, region string) config.RecommendationRecord { + t.Helper() + blob, err := common.MarshalServiceDetails(&common.ComputeDetails{GCPCommitmentType: family, MemoryGB: 16}) + require.NoError(t, err) + return config.RecommendationRecord{ + Provider: "gcp", Service: "compute", Region: region, ResourceType: "n2-standard-4", + Count: 4, Term: 1, Payment: "monthly", Selected: true, Details: blob, + } +} + +const cudLineage = "lineage-682" + +func cudOwnName(index int) string { + return gcpCommitmentName(common.DeriveIdempotencyToken(cudLineage, index)) +} + +// gcpRun drives processPurchaseRecommendations for the recs against svc. +func gcpRun(t *testing.T, svc *fakeCUDService, lineage string, recs ...config.RecommendationRecord) ([]string, bool, []config.RecommendationRecord) { + t.Helper() + client, err := computeengine.NewClient(cudProject, cudRegion) + require.NoError(t, err) + client.SetCommitmentsService(svc) + + store := new(MockConfigStore) + factory, prov := new(MockProviderFactory), new(MockProvider) + store.On("SavePurchaseHistory", mock.Anything, mock.Anything).Return(nil).Maybe() + factory.On("CreateAndValidateProvider", mock.Anything, "gcp", mock.Anything).Return(prov, nil) + prov.On("GetServiceClient", mock.Anything, common.ServiceCompute, mock.Anything).Return(client, nil) + m := &Manager{config: store, providerFactory: factory} + + exec := &config.PurchaseExecution{ExecutionID: "e", PlanID: "p", IdempotencyKey: lineage, Recommendations: recs} + _, _, errs, allRefused, procErr := m.processPurchaseRecommendations(context.Background(), exec, &config.PurchasePlan{Name: "P"}, cudProject, nil, "") + require.NoError(t, procErr) + return errs, allRefused, exec.Recommendations +} + +func TestGCPRecent_RefusedBeforePurchase(t *testing.T) { + cases := map[string]struct { + svc *fakeCUDService + note string + }{ + "recent same-family commitment": { + &fakeCUDService{items: []*computepb.Commitment{cudFixture("someone-else", familyN2, time.Hour)}}, + gcpRecentCommitmentNote, + }, + "listing errors": { + &fakeCUDService{listErr: errors.New("malformed commitment")}, + gcpDuplicateCheckFailedNote, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + errs, allRefused, recs := gcpRun(t, tc.svc, cudLineage, cudRec(t, familyN2, cudRegion)) + require.Len(t, errs, 1) + assert.Equal(t, tc.note, recs[0].Error, "the stored rec error is exactly the fixed note") + assert.Equal(t, tc.note, SanitizeStoredError(recs[0].Error)) + assert.Zero(t, tc.svc.insertCount(), "no Insert reached the provider") + assert.True(t, allRefused, "every rec refused before submission feeds no_submission") + assert.False(t, recs[0].Purchased) + }) + } +} + +func TestGCPRecent_Purchased(t *testing.T) { + cases := map[string]struct { + items []*computepb.Commitment + region string + }{ + "recent commitment in another family": {[]*computepb.Commitment{cudFixture("other", familyE2, time.Hour)}, cudRegion}, + "recent commitment in another region": {[]*computepb.Commitment{cudFixture("other", familyN2, time.Hour)}, "europe-west1"}, + "commitment older than 24h": {[]*computepb.Commitment{cudFixture("old", familyN2, 25*time.Hour)}, cudRegion}, + "own name after a lost response": {[]*computepb.Commitment{cudFixture(cudOwnName(0), familyN2, time.Minute)}, cudRegion}, + "no commitments, project unknown": {nil, cudRegion}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + svc := &fakeCUDService{items: tc.items} + errs, allRefused, recs := gcpRun(t, svc, cudLineage, cudRec(t, familyN2, tc.region)) + require.Empty(t, errs) + assert.False(t, allRefused) + assert.True(t, recs[0].Purchased) + assert.Equal(t, 1, svc.insertCount(), "the purchase call ran") + }) + } +} + +func TestGCPRecent_OwnNameRetryAdoptsExistingCommitment(t *testing.T) { + svc := &fakeCUDService{items: []*computepb.Commitment{cudFixture(cudOwnName(0), familyN2, time.Minute)}} + errs, _, recs := gcpRun(t, svc, cudLineage, cudRec(t, familyN2, cudRegion)) + require.Empty(t, errs) + assert.Equal(t, []string{cudOwnName(0)}, svc.inserted, "Insert hit the 409 and the library adopted the commitment") + assert.Equal(t, cudOwnName(0), recs[0].PurchaseID) +} + +func TestGCPRecent_TwoSameFamilyRecsInOneExecutionBothBought(t *testing.T) { + svc := &fakeCUDService{} + errs, allRefused, recs := gcpRun(t, svc, cudLineage, cudRec(t, familyN2, cudRegion), cudRec(t, familyN2, cudRegion)) + require.Empty(t, errs, "an earlier rec of the same run is not a duplicate of a later one") + assert.False(t, allRefused) + assert.True(t, recs[0].Purchased && recs[1].Purchased) + assert.ElementsMatch(t, []string{cudOwnName(0), cudOwnName(1)}, svc.inserted) +} + +func TestGCPRecent_FanOutSiblingsOwnTheirOwnNames(t *testing.T) { + root := "root-lineage" + recs := []config.RecommendationRecord{cudRec(t, familyN2, cudRegion)} + a := gcpOwnCommitmentNames(&config.PurchaseExecution{IdempotencyKey: root + ":acct-a", Recommendations: recs}, []int{0}) + b := gcpOwnCommitmentNames(&config.PurchaseExecution{IdempotencyKey: root + ":acct-b", Recommendations: recs}, []int{0}) + require.Len(t, a, 1) + require.Len(t, b, 1) + for name := range a { + assert.False(t, b[name], "siblings derive different names") + } + for _, acct := range []string{"acct-a", "acct-b"} { + svc := &fakeCUDService{} // each cloud account is its own project + errs, _, got := gcpRun(t, svc, root+":"+acct, recs...) + require.Empty(t, errs) + assert.True(t, got[0].Purchased) + } +} + +func TestGCPRecent_RefusedPlusBoughtIsNotAllRefused(t *testing.T) { + svc := &fakeCUDService{items: []*computepb.Commitment{cudFixture("someone-else", familyN2, time.Hour)}} + errs, allRefused, recs := gcpRun(t, svc, cudLineage, cudRec(t, familyN2, cudRegion), cudRec(t, familyE2, cudRegion)) + require.Len(t, errs, 1) + assert.False(t, allRefused) + assert.False(t, recs[0].Purchased) + assert.True(t, recs[1].Purchased) + assert.Equal(t, []string{cudOwnName(1)}, svc.inserted) +} + +func TestGCPOwnCommitmentNames_OnlySelectedGCPComputeRecs(t *testing.T) { + gcp := cudRec(t, familyN2, cudRegion) + unselected := gcp + unselected.Selected = false + aws := config.RecommendationRecord{Provider: "aws", Service: "ec2", Selected: true} + otherGCP := config.RecommendationRecord{Provider: "gcp", Service: "relational-db", Selected: true} + exec := &config.PurchaseExecution{IdempotencyKey: cudLineage, Recommendations: []config.RecommendationRecord{gcp, unselected, aws, otherGCP}} + assert.Equal(t, map[string]bool{cudOwnName(0): true}, gcpOwnCommitmentNames(exec, selectedIndices(exec.Recommendations))) +} + +// The mirror must equal the name the pinned library gives Insert. +func TestGCPCommitmentName_MatchesLibraryInsertName(t *testing.T) { + svc := &fakeCUDService{} + client, err := computeengine.NewClient(cudProject, cudRegion) + require.NoError(t, err) + client.SetCommitmentsService(svc) + token := common.DeriveIdempotencyToken(cudLineage, 3) + rec := common.Recommendation{ResourceType: "n2-standard-4", Term: "1yr", Count: 4, Details: common.ComputeDetails{MemoryGB: 16}} + + _, err = client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token}) + require.NoError(t, err) + require.Len(t, svc.inserted, 1) + assert.Equal(t, svc.inserted[0], gcpCommitmentName(token)) +} + +// filterSpy is a client that implements the GCP filter and records its input. +type filterSpy struct { + *MockServiceClient + calls int + gotRecs []common.Recommendation + filtered bool +} + +func (s *filterSpy) FilterRecommendationsForRecentCommitments(recs []common.Recommendation, _ []common.Commitment) (passed, filtered []common.Recommendation, err error) { + s.calls++ + s.gotRecs = recs + if s.filtered { + return nil, recs, nil + } + return recs, nil, nil +} + +func spyRecent(own bool) []common.Commitment { + id := "other" + if own { + id = cudOwnName(0) + } + return []common.Commitment{{ + Provider: common.ProviderGCP, Service: common.ServiceCompute, CommitmentType: common.CommitmentCUD, + Account: cudProject, Region: cudRegion, State: common.CommitmentStateActive, + StartDate: time.Now().Add(-time.Hour), ResourceType: familyN2, CommitmentID: id, + }} +} + +func gcpGuardRec() common.Recommendation { + return common.Recommendation{Provider: common.ProviderGCP, Service: common.ServiceCompute, Region: cudRegion, + ResourceType: "n2-standard-4", Details: &common.ComputeDetails{GCPCommitmentType: familyN2}} +} + +func TestGCPRecentRefusal_ForwardsTheGCPFilterWithTheListedProject(t *testing.T) { + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(spyRecent(false), nil) + spy := &filterSpy{MockServiceClient: inner, filtered: true} + + err := gcpRecentCommitmentRefusal(context.Background(), spy, gcpGuardRec(), nil) + + require.Error(t, err) + assert.Equal(t, gcpRecentCommitmentNote, err.Error()) + var notSubmitted *beforeSubmissionError + assert.True(t, errors.As(err, ¬Submitted)) + require.Equal(t, 1, spy.calls, "the wrapper forwarded the provider filter") + assert.Equal(t, cudProject, spy.gotRecs[0].Account, "project stamped from the listed commitments") + assert.Equal(t, common.CommitmentCUD, spy.gotRecs[0].CommitmentType) +} + +func TestGCPRecentRefusal_DropsOwnCommitmentsBeforeFiltering(t *testing.T) { + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(spyRecent(true), nil) + spy := &filterSpy{MockServiceClient: inner, filtered: true} + + require.NoError(t, gcpRecentCommitmentRefusal(context.Background(), spy, gcpGuardRec(), map[string]bool{cudOwnName(0): true})) + assert.Zero(t, spy.calls, "nothing recent and foreign is left, so the filter is not reached") +} + +// A recent CUD bought by another CUDly lineage (another plan step, a +// re-approval, or a sibling account sharing the project) carries a valid +// CUDly-shaped name that is not this execution's, so it must count as a recent +// commitment and refuse the rec, not be mistaken for an own commitment. +func TestGCPRecentRefusal_OtherLineageCUDIsRefused(t *testing.T) { + recent := spyRecent(false) + recent[0].CommitmentID = gcpCommitmentName(common.DeriveIdempotencyToken("other-lineage", 0)) + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(recent, nil) + spy := &filterSpy{MockServiceClient: inner, filtered: true} + + err := gcpRecentCommitmentRefusal(context.Background(), spy, gcpGuardRec(), map[string]bool{cudOwnName(0): true}) + + require.Error(t, err) + assert.Equal(t, gcpRecentCommitmentNote, err.Error()) + var notSubmitted *beforeSubmissionError + assert.True(t, errors.As(err, ¬Submitted)) + require.Equal(t, 1, spy.calls, "the other lineage's CUD reached the provider filter as a recent commitment") + inner.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) +} + +// Recent commitments from two projects cannot all describe this rec's project, +// so the check fails closed instead of stamping the first project and letting +// a duplicate in the other one through. +func TestGCPRecentRefusal_CommitmentsFromTwoProjectsFailClosed(t *testing.T) { + recent := append(spyRecent(false), spyRecent(false)...) + recent[1].Account = "other-project" + recent[1].CommitmentID = "other-2" + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(recent, nil) + spy := &filterSpy{MockServiceClient: inner, filtered: false} + + err := gcpRecentCommitmentRefusal(context.Background(), spy, gcpGuardRec(), nil) + + require.Error(t, err) + assert.Equal(t, gcpDuplicateCheckFailedNote, err.Error()) + var notSubmitted *beforeSubmissionError + assert.True(t, errors.As(err, ¬Submitted)) + assert.Zero(t, spy.calls, "the provider filter is never reached with a mixed-project list") +} + +// A client without the GCP filter must fail closed, never reach recfilter's +// generic map path. +func TestGCPRecentRefusal_ClientWithoutFilterFailsClosed(t *testing.T) { + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(spyRecent(false), nil) + + err := gcpRecentCommitmentRefusal(context.Background(), inner, gcpGuardRec(), nil) + + require.Error(t, err) + assert.Equal(t, gcpDuplicateCheckFailedNote, err.Error()) +} + +func TestGCPRecentRefusal_OtherRecsNeverTouchTheClient(t *testing.T) { + client := new(MockServiceClient) // any call panics: no expectations registered + for _, rec := range []common.Recommendation{ + {Provider: common.ProviderAWS, Service: common.ServiceEC2}, + {Provider: common.ProviderAzure, Service: common.ServiceCompute}, + {Provider: common.ProviderGCP, Service: common.ServiceRelationalDB}, + } { + assert.NoError(t, gcpRecentCommitmentRefusal(context.Background(), client, rec, nil), "%s/%s", rec.Provider, rec.Service) + } +} diff --git a/internal/purchase/late_accounts_test.go b/internal/purchase/late_accounts_test.go index 3d333b93..9c134cdf 100644 --- a/internal/purchase/late_accounts_test.go +++ b/internal/purchase/late_accounts_test.go @@ -266,7 +266,7 @@ func TestExecuteSinglePurchase_OnlyACutOffCallIsOutcomeUnknown(t *testing.T) { prov.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(client, nil) m := &Manager{config: store, providerFactory: factory} _, err := m.executeSinglePurchase(ctx, config.RecommendationRecord{Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1}, - nil, common.PurchaseOptions{}) + nil, common.PurchaseOptions{}, nil) require.Error(t, err) assert.Equal(t, cut, errors.Is(err, common.ErrOutcomeUnknown), "cut=%v: %v", cut, err) cancel() diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index e468b12f..f9ba224d 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -774,6 +774,9 @@ func TestManager_RecoverStrandedApprovals_GCPRedrives(t *testing.T) { t.Cleanup(func() { mockFactory.AssertExpectations(t) }) t.Cleanup(func() { mockProvider.AssertExpectations(t) }) t.Cleanup(func() { mockServiceClient.AssertExpectations(t) }) + // The GCP recent-commitment check (#682) lists live commitments first; + // none exist, so the re-drive proceeds to the purchase. + mockServiceClient.On("GetExistingCommitments", mock.Anything).Return([]common.Commitment(nil), nil) stranded := config.PurchaseExecution{ ExecutionID: "exec-gcp-stranded", diff --git a/internal/purchase/plan_step_proofs_test.go b/internal/purchase/plan_step_proofs_test.go index 98c13855..c2fc2bf1 100644 --- a/internal/purchase/plan_step_proofs_test.go +++ b/internal/purchase/plan_step_proofs_test.go @@ -96,6 +96,11 @@ func tokenCapture(t *testing.T, providerName string, exec *config.PurchaseExecut store.On("SavePurchaseHistory", ctx, mock.Anything).Return(nil) factory.On("CreateAndValidateProvider", mock.Anything, providerName, mock.Anything).Return(prov, nil) prov.On("GetServiceClient", mock.Anything, mock.Anything, mock.Anything).Return(svc, nil) + if providerName == "gcp" { + // The GCP recent-commitment check (#682) lists live commitments before + // buying; none exist here, so every rec proceeds to PurchaseCommitment. + svc.On("GetExistingCommitments", mock.Anything).Return([]common.Commitment(nil), nil) + } var mu sync.Mutex tokens := map[string]string{} svc.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")). diff --git a/internal/purchase/requested_at_test.go b/internal/purchase/requested_at_test.go index c3f8c40b..4de5e571 100644 --- a/internal/purchase/requested_at_test.go +++ b/internal/purchase/requested_at_test.go @@ -128,7 +128,7 @@ func TestExecuteSinglePurchase_EmptyTokenOptsOtherwiseUntouched(t *testing.T) { in := common.PurchaseOptions{Source: common.PurchaseSourceWeb, ExecutionID: "exec-1"} before := time.Now() - _, err := m.executeSinglePurchase(context.Background(), scopedTestRec("acct-A"), nil, in) + _, err := m.executeSinglePurchase(context.Background(), scopedTestRec("acct-A"), nil, in, nil) after := time.Now() require.NoError(t, err) @@ -371,7 +371,7 @@ func TestExecuteSinglePurchase_RedshiftWithoutRequestedAtRefuses(t *testing.T) { mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceRedshift, mock.Anything).Return(mockServiceClient, nil).Maybe() m := &Manager{providerFactory: mockFactory} - _, err := m.executeSinglePurchase(context.Background(), redshiftScopedRec("acct-A"), nil, common.PurchaseOptions{ExecutionID: "e"}) + _, err := m.executeSinglePurchase(context.Background(), redshiftScopedRec("acct-A"), nil, common.PurchaseOptions{ExecutionID: "e"}, nil) require.Error(t, err) mockServiceClient.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) diff --git a/internal/purchase/zonal_az_test.go b/internal/purchase/zonal_az_test.go index c79a2114..26479568 100644 --- a/internal/purchase/zonal_az_test.go +++ b/internal/purchase/zonal_az_test.go @@ -49,7 +49,7 @@ func TestExecuteSinglePurchase_ZonalRecKeepsAvailabilityZone(t *testing.T) { Run(func(args mock.Arguments) { got = args.Get(1).(common.Recommendation) }). Return(common.PurchaseResult{Success: true, CommitmentID: "ri-1"}, nil) - _, err := managerServing(t, sc).executeSinglePurchase(context.Background(), zonalRecRecord(t, "us-east-1b"), nil, common.PurchaseOptions{ExecutionID: "exec-az"}) + _, err := managerServing(t, sc).executeSinglePurchase(context.Background(), zonalRecRecord(t, "us-east-1b"), nil, common.PurchaseOptions{ExecutionID: "exec-az"}, nil) require.NoError(t, err) d, ok := got.Details.(*common.ComputeDetails) @@ -81,7 +81,7 @@ func TestExecuteSinglePurchase_StaleZonalRowWithoutAZFailsClosed(t *testing.T) { c := ec2svc.NewClient(awsConfigForTest()) c.SetEC2API(api) - _, err := managerServing(t, c).executeSinglePurchase(context.Background(), zonalRecRecord(t, ""), nil, common.PurchaseOptions{ExecutionID: "exec-stale"}) + _, err := managerServing(t, c).executeSinglePurchase(context.Background(), zonalRecRecord(t, ""), nil, common.PurchaseOptions{ExecutionID: "exec-stale"}, nil) require.Error(t, err) assert.Contains(t, err.Error(), "re-fetch") assert.Zero(t, api.purchases, "no purchase call")