diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 559071a99..b4ee3b9cb 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "log" "strings" "time" @@ -441,44 +442,7 @@ func (c *ComputeEngineClient) PurchaseCommitment(ctx context.Context, rec common } defer svc.Close() - // Determine plan based on term - plan := "TWELVE_MONTH" - if rec.Term == "3yr" || rec.Term == "3" { - plan = "THIRTY_SIX_MONTH" - } - - // GCP's computepb.Commitment has no Labels field, and the RegionCommitments - // client exposes no SetLabels call — CUDs cannot be tagged or labeled via - // the API. Encode the source into Description so customers can still - // filter with `gcloud compute commitments list --filter="description:..."`. - description := fmt.Sprintf("CUD for %s", rec.ResourceType) - if opts.Source != "" { - description = fmt.Sprintf("%s [%s=%s]", description, common.PurchaseTagKey, opts.Source) - } - - // GCP requires both VCPU and MEMORY_MB in a single commitment insert. - commitment := &computepb.Commitment{ - Name: stringPtr(fmt.Sprintf("cud-%d", time.Now().Unix())), - Plan: stringPtr(plan), - Type: stringPtr("GENERAL_PURPOSE"), - Description: stringPtr(description), - Resources: []*computepb.ResourceCommitment{ - { - Type: stringPtr("VCPU"), - Amount: int64Ptr(int64(rec.Count)), - }, - { - Type: stringPtr("MEMORY_MB"), - Amount: int64Ptr(int64(rec.Count) * 4096), - }, - }, - } - - insertReq := &computepb.InsertRegionCommitmentRequest{ - Project: c.projectID, - Region: c.region, - CommitmentResource: commitment, - } + insertReq, commitmentName := c.buildInsertRequest(rec, opts) // Exponential backoff on RESOURCE_EXHAUSTED: BaseDelay 1s with 2× growth // capped at MaxDelay 4s gives the same 1s/2s/4s sequence the open-coded @@ -513,12 +477,82 @@ func (c *ComputeEngineClient) PurchaseCommitment(ctx context.Context, rec common } result.Success = true - result.CommitmentID = *commitment.Name + result.CommitmentID = commitmentName result.Cost = rec.CommitmentCost return result, nil } +// buildInsertRequest assembles the RegionCommitments.Insert request for a +// purchase, threading opts.IdempotencyToken into both GCP idempotency levers so +// a re-drive of the same execution cannot create a second CUD (issue #654, the +// financial double-buy). It returns the request and the commitment name (used as +// the resulting CommitmentID). +// +// - RequestId is GCP's native server-side idempotency key on Insert, which the +// API documents as preventing clients from accidentally creating duplicate +// commitments. It MUST be a valid non-zero UUID, so we format the token (a +// SHA-256 hex digest) into a deterministic canonical UUID via +// common.IdempotencyGUID — the same mechanism PR #653 used for the Azure +// reservationOrderID. The same token always yields the same RequestId, so a +// second Insert is a server-side no-op rather than a new purchase. +// - Name is also derived from the token as defense in depth: commitment names +// are unique per project+region, so a re-drive that somehow reached Insert +// (e.g. RequestId expired) collides on the name and GCP rejects it with +// ALREADY_EXISTS instead of creating a duplicate. +// +// An empty token preserves the prior non-idempotent timestamp-based name (the +// CLI path, which has no owning execution). The token is masked in logs via +// common.MaskToken and never logged verbatim. +func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts common.PurchaseOptions) (*computepb.InsertRegionCommitmentRequest, string) { + plan := "TWELVE_MONTH" + if rec.Term == "3yr" || rec.Term == "3" { + plan = "THIRTY_SIX_MONTH" + } + + // GCP's computepb.Commitment has no Labels field, and the RegionCommitments + // client exposes no SetLabels call — CUDs cannot be tagged or labeled via + // the API. Encode the source into Description so customers can still filter + // with `gcloud compute commitments list --filter="description:..."`. + description := fmt.Sprintf("CUD for %s", rec.ResourceType) + if opts.Source != "" { + description = fmt.Sprintf("%s [%s=%s]", description, common.PurchaseTagKey, opts.Source) + } + + commitmentName := idempotentCommitmentName(opts.IdempotencyToken) + + // GCP requires both VCPU and MEMORY_MB in a single commitment insert. + commitment := &computepb.Commitment{ + Name: stringPtr(commitmentName), + Plan: stringPtr(plan), + Type: stringPtr("GENERAL_PURPOSE"), + Description: stringPtr(description), + Resources: []*computepb.ResourceCommitment{ + { + Type: stringPtr("VCPU"), + Amount: int64Ptr(int64(rec.Count)), + }, + { + Type: stringPtr("MEMORY_MB"), + Amount: int64Ptr(int64(rec.Count) * 4096), + }, + }, + } + + insertReq := &computepb.InsertRegionCommitmentRequest{ + Project: c.projectID, + Region: c.region, + CommitmentResource: commitment, + } + if requestID := common.IdempotencyGUID(opts.IdempotencyToken); requestID != "" { + insertReq.RequestId = stringPtr(requestID) + log.Printf("GCP CUD purchase using idempotent request ID for token %s (commitment %s); a re-drive will not double-purchase (issue #654)", + common.MaskToken(opts.IdempotencyToken), commitmentName) + } + + return insertReq, commitmentName +} + // ValidateOffering validates that a machine type exists func (c *ComputeEngineClient) ValidateOffering(ctx context.Context, rec common.Recommendation) error { validTypes, err := c.GetValidResourceTypes(ctx) @@ -823,6 +857,33 @@ func extractCostImpactFromRecommendation(gcpRec *recommenderpb.Recommendation, r } } +// idempotentNameTokenLen is how many leading hex characters of the idempotency +// token are folded into the derived commitment name. A GCP commitment name must +// match RFC1035 (1-63 chars, lowercase [a-z]([-a-z0-9]*[a-z0-9])?); the "cud-" +// prefix is 4 chars, so 32 hex chars (128 bits, collision-free at any realistic +// volume) keeps the result at 36 chars, well under the 63-char limit. +const idempotentNameTokenLen = 32 + +// idempotentCommitmentName derives a deterministic, RFC1035-valid GCP commitment +// name from an idempotency token (issue #654). The same token always yields the +// same name, so a re-drive collides on the unique-per-project+region name and +// GCP rejects the duplicate with ALREADY_EXISTS instead of creating a second +// CUD. An empty token preserves the prior non-idempotent timestamp-based name +// (the CLI path, which has no owning execution). +// +// The token is a lowercase SHA-256 hex digest, so the leading chars are already +// valid RFC1035 name characters and need no further sanitisation. +func idempotentCommitmentName(token string) string { + if token == "" { + return fmt.Sprintf("cud-%d", time.Now().Unix()) + } + t := strings.ToLower(token) + if len(t) > idempotentNameTokenLen { + t = t[:idempotentNameTokenLen] + } + return "cud-" + t +} + // Helper functions func stringPtr(s string) *string { return &s diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 7e8dd62f8..ffcb4adc4 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -24,7 +24,8 @@ type MockCommitmentsService struct { listErr error insertErr error index int - lastInsertReq *computepb.InsertRegionCommitmentRequest // captured for assertions + lastInsertReq *computepb.InsertRegionCommitmentRequest // captured for assertions + insertReqs []*computepb.InsertRegionCommitmentRequest // every Insert call (re-drive assertions) } func (m *MockCommitmentsService) List(ctx context.Context, req *computepb.ListRegionCommitmentsRequest) CommitmentsIterator { @@ -33,6 +34,7 @@ func (m *MockCommitmentsService) List(ctx context.Context, req *computepb.ListRe func (m *MockCommitmentsService) Insert(ctx context.Context, req *computepb.InsertRegionCommitmentRequest) (CommitmentsOperation, error) { m.lastInsertReq = req + m.insertReqs = append(m.insertReqs, req) if m.insertErr != nil { return nil, m.insertErr } @@ -511,6 +513,71 @@ func TestComputeEngineClient_PurchaseCommitment_OmitsTagWhenSourceEmpty(t *testi assert.NotContains(t, desc, common.PurchaseTagKey) } +// TestComputeEngineClient_PurchaseCommitment_IdempotentReDrive is the issue #654 +// regression: re-driving the same execution (identical IdempotencyToken) must +// not create a second CUD. We assert the second Insert carries the *same* +// deterministic RequestId (GCP's native server-side dedupe key) and the *same* +// commitment Name (the defense-in-depth ALREADY_EXISTS guard) as the first, so +// GCP treats the re-drive as a no-op rather than a double-purchase. +func TestComputeEngineClient_PurchaseCommitment_IdempotentReDrive(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} + client.SetCommitmentsService(mockService) + + rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 5} + token := common.DeriveIdempotencyToken("exec-654", 0) + opts := common.PurchaseOptions{Source: common.PurchaseSourceWeb, IdempotencyToken: token} + + r1, err := client.PurchaseCommitment(ctx, rec, opts) + require.NoError(t, err) + require.True(t, r1.Success) + + r2, err := client.PurchaseCommitment(ctx, rec, opts) + require.NoError(t, err) + require.True(t, r2.Success) + + require.Len(t, mockService.insertReqs, 2, "both calls reach Insert; GCP dedupes server-side on RequestId") + + first, second := mockService.insertReqs[0], mockService.insertReqs[1] + + // Native idempotency key: same token -> same non-empty valid UUID RequestId. + wantGUID := common.IdempotencyGUID(token) + require.NotEmpty(t, wantGUID, "token must yield a valid idempotency GUID") + assert.Equal(t, wantGUID, first.GetRequestId(), "first RequestId must be the derived GUID") + assert.Equal(t, first.GetRequestId(), second.GetRequestId(), "re-drive must reuse the same RequestId (no double-buy)") + assert.NotEqual(t, "00000000-0000-0000-0000-000000000000", first.GetRequestId(), "zero UUID is rejected by GCP") + + // Defense in depth: same token -> same deterministic commitment name. + require.NotNil(t, first.CommitmentResource) + require.NotNil(t, second.CommitmentResource) + assert.Equal(t, first.CommitmentResource.GetName(), second.CommitmentResource.GetName(), + "re-drive must reuse the same commitment name (GCP rejects the duplicate with ALREADY_EXISTS)") + assert.Equal(t, r1.CommitmentID, r2.CommitmentID, "re-drive must report the same commitment ID") +} + +// TestComputeEngineClient_PurchaseCommitment_EmptyTokenNoRequestID confirms the +// CLI path (no owning execution, empty token) keeps its prior non-idempotent +// behaviour: no RequestId is set and the name is the timestamp-based fallback. +func TestComputeEngineClient_PurchaseCommitment_EmptyTokenNoRequestID(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} + client.SetCommitmentsService(mockService) + + rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 1} + + _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.NoError(t, err) + require.NotNil(t, mockService.lastInsertReq) + assert.Empty(t, mockService.lastInsertReq.GetRequestId(), "empty token must not set a RequestId") + require.NotNil(t, mockService.lastInsertReq.CommitmentResource) + assert.Contains(t, mockService.lastInsertReq.CommitmentResource.GetName(), "cud-", + "empty token keeps the timestamp-based name") +} + func TestComputeEngineClient_PurchaseCommitment_3Year(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1")