diff --git a/pkg/common/tokens.go b/pkg/common/tokens.go index b1a299187..51754bd8c 100644 --- a/pkg/common/tokens.go +++ b/pkg/common/tokens.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "encoding/hex" "fmt" + "strings" ) // GenerateApprovalToken returns a 32-byte cryptographically secure random @@ -58,3 +59,37 @@ func MaskToken(token string) string { } return token[:8] + "..." } + +// IdempotencyGUID formats an idempotency token as a deterministic canonical GUID +// (8-4-4-4-12 lowercase hex) for use as an Azure reservationOrderID (issue #641). +// The Azure Reservations API path is reservationOrders/{guid} and a PUT is +// idempotent on a stable order ID, so deriving the GUID from the token makes a +// re-drive re-PUT the same order rather than create a second reservation. +// +// It uses the first 32 hex characters (128 bits) of the token, which is itself a +// SHA-256 hex digest, so the GUID is deterministic and collision-free at any +// realistic purchase volume. Returns "" when token is shorter than 32 hex chars +// (e.g. empty) so callers keep their prior non-idempotent ID behaviour. +func IdempotencyGUID(token string) string { + if len(token) < 32 { + return "" + } + h := strings.ToLower(token[:32]) + if _, err := hex.DecodeString(h); err != nil { + return "" + } + return fmt.Sprintf("%s-%s-%s-%s-%s", h[0:8], h[8:12], h[12:16], h[16:20], h[20:32]) +} + +// ReservationOrderID returns the Azure reservationOrderID to PUT for a purchase: +// the deterministic GUID derived from token when one is supplied (issue #641, so +// a re-drive re-PUTs the same idempotent order), otherwise fallback (the caller's +// prior non-idempotent ID, e.g. a random GUID or a timestamp). Centralising the +// choice keeps each executor's PurchaseCommitment a single statement and avoids +// repeating the same empty-token guard across every Azure service. +func ReservationOrderID(token, fallback string) string { + if guid := IdempotencyGUID(token); guid != "" { + return guid + } + return fallback +} diff --git a/pkg/common/tokens_test.go b/pkg/common/tokens_test.go index 2d536a2c6..00fab00d6 100644 --- a/pkg/common/tokens_test.go +++ b/pkg/common/tokens_test.go @@ -85,3 +85,40 @@ func TestMaskToken_EmptyAndShort(t *testing.T) { assert.Equal(t, "12345678", MaskToken("12345678"), "exactly 8 chars is returned unchanged") assert.Equal(t, "12345678...", MaskToken("123456789"), "9 chars is truncated to 8 + ellipsis") } + +func TestIdempotencyGUID_DeterministicCanonicalForm(t *testing.T) { + token := DeriveIdempotencyToken("exec-1", 0) + + a := IdempotencyGUID(token) + b := IdempotencyGUID(token) + assert.Equal(t, a, b, "same token must yield the same GUID") + + // Canonical 8-4-4-4-12 lowercase-hex GUID shape. + assert.Regexp(t, `^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`, a) + assert.Len(t, a, 36) +} + +func TestIdempotencyGUID_DistinctTokensDistinctGUIDs(t *testing.T) { + g0 := IdempotencyGUID(DeriveIdempotencyToken("exec-1", 0)) + g1 := IdempotencyGUID(DeriveIdempotencyToken("exec-1", 1)) + assert.NotEqual(t, g0, g1, "different recs must get different order IDs") +} + +func TestIdempotencyGUID_ShortTokenReturnsEmpty(t *testing.T) { + assert.Equal(t, "", IdempotencyGUID(""), "empty token must yield empty so callers keep their fallback") + assert.Equal(t, "", IdempotencyGUID("abc"), "sub-32-char token must yield empty") +} + +func TestReservationOrderID(t *testing.T) { + token := DeriveIdempotencyToken("exec-1", 0) + + // With a token: deterministic GUID, never the fallback. + got := ReservationOrderID(token, "fallback-id") + assert.Equal(t, IdempotencyGUID(token), got, "a supplied token must derive the GUID") + assert.Equal(t, got, ReservationOrderID(token, "other-fallback"), + "same token must yield the same order ID regardless of fallback") + + // No usable token: caller's fallback is returned verbatim. + assert.Equal(t, "fallback-id", ReservationOrderID("", "fallback-id")) + assert.Equal(t, "fallback-id", ReservationOrderID("abc", "fallback-id")) +} diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 6bf71cde2..b08c7a8b8 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -273,7 +273,10 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme Timestamp: time.Now(), } - reservationOrderID := uuid.New().String() + // Derive a deterministic reservationOrderID from the idempotency token (issue + // #641) so a re-drive re-PUTs the same idempotent Azure reservation order + // instead of creating a second; fall back to a random GUID otherwise. + reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String()) apiVersion := "2022-11-01" purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=%s", reservationOrderID, apiVersion) diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 81ec94f1a..a336bcc4b 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -457,7 +457,12 @@ func (c *ComputeClient) PurchaseCommitment(ctx context.Context, rec common.Recom return result, result.Error } - reservationOrderID := uuid.New().String() + // When an idempotency token is supplied (issue #641) the reservationOrderID is + // derived deterministically from it. The Azure Reservations API PUTs to + // reservationOrders/{id} and is idempotent on a stable order ID, so a re-drive + // re-PUTs the same order and returns the existing reservation rather than + // creating a second. Otherwise mint a random GUID (prior behaviour). + reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String()) purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=2022-11-01", reservationOrderID) diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index c92a27278..426368ec9 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -566,6 +566,70 @@ func TestComputeClient_PurchaseCommitment_BadStatus(t *testing.T) { assert.Contains(t, err.Error(), "reservation purchase failed with status 400") } +// purchaseURLFromMock returns the reservation PUT URL captured by the mock HTTP +// client for the last successful PurchaseCommitment call. The Azure Reservations +// API PUTs to .../reservationOrders/{id}; the {id} segment is the order ID that +// makes the request idempotent. +func purchaseURLFromMock(t *testing.T, m *mocks.MockHTTPClient) string { + t.Helper() + for i := len(m.Calls) - 1; i >= 0; i-- { + req, ok := m.Calls[i].Arguments.Get(0).(*http.Request) + if ok && req != nil { + return req.URL.String() + } + } + t.Fatalf("no HTTP request captured by mock") + return "" +} + +// TestComputeClient_PurchaseCommitment_IdempotentReDrive is the issue #641 +// regression test: a re-drive with the *same* IdempotencyToken must not create a +// second reservation. Because the Azure Reservations API PUTs to +// reservationOrders/{id} and is idempotent on a stable order ID, "no second +// reservation" is proven by the two re-drives PUTting to the *same* +// reservationOrders/{id} URL (and yielding the same CommitmentID). A distinct +// token must target a distinct order so unrelated purchases never collide. +func TestComputeClient_PurchaseCommitment_IdempotentReDrive(t *testing.T) { + ctx := context.Background() + rec := common.Recommendation{ + ResourceType: "Standard_D2s_v3", + Term: "1yr", + Count: 1, + CommitmentCost: 2000.0, + } + token := common.DeriveIdempotencyToken("exec-641", 0) + + purchase := func(tok string) (common.PurchaseResult, string) { + mockHTTP := &mocks.MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + mockHTTP.On("Do", mock.Anything).Return( + mocks.CreateMockHTTPResponse(http.StatusOK, `{"id": "reservation-123"}`), nil) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{IdempotencyToken: tok}) + require.NoError(t, err) + require.True(t, result.Success) + return result, purchaseURLFromMock(t, mockHTTP) + } + + first, firstURL := purchase(token) + second, secondURL := purchase(token) + + // Same token => same idempotent order ID => same PUT URL => Azure re-PUTs the + // existing order rather than minting a second reservation. + assert.Equal(t, first.CommitmentID, second.CommitmentID, + "same idempotency token must reuse the same reservationOrderID") + assert.Equal(t, firstURL, secondURL, + "re-drive must PUT to the same reservationOrders/{id} URL") + assert.Contains(t, firstURL, common.IdempotencyGUID(token), + "order ID must be derived deterministically from the token") + + // Distinct token => distinct order so independent purchases don't collide. + other, otherURL := purchase(common.DeriveIdempotencyToken("exec-641", 1)) + assert.NotEqual(t, first.CommitmentID, other.CommitmentID, + "different recs must get different reservation orders") + assert.NotEqual(t, firstURL, otherURL) +} + // TestComputeClient_ConvertAzureVMRecommendation_NilGuards pins the new // contract: unusable SDK payloads (nil, wrong concrete type, nil Properties) // produce a nil *Recommendation so the caller can filter it out. Before diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index fd7e73f30..af1db742c 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -266,8 +266,11 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco Timestamp: time.Now(), } - // Build reservation purchase request - reservationOrderID := uuid.New().String() + // Build reservation purchase request. Derive a deterministic + // reservationOrderID from the idempotency token (issue #641) so a re-drive + // re-PUTs the same idempotent Azure reservation order instead of creating a + // second; fall back to a random GUID otherwise. + reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String()) // Construct the Azure Reservations API request apiVersion := "2022-11-01" diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index 18e705fe4..758f29901 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -274,8 +274,11 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco Timestamp: time.Now(), } - // Build reservation purchase request - reservationOrderID := uuid.New().String() + // Build reservation purchase request. Derive a deterministic + // reservationOrderID from the idempotency token (issue #641) so a re-drive + // re-PUTs the same idempotent Azure reservation order instead of creating a + // second; fall back to a random GUID otherwise. + reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String()) // Construct the Azure Reservations API request apiVersion := "2022-11-01" diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index c450a8edc..181a2150c 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -16,6 +16,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/search/armsearch" + "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" @@ -244,7 +245,11 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm Timestamp: time.Now(), } - reservationOrderID := fmt.Sprintf("search-reservation-%d", time.Now().Unix()) + // Derive a deterministic reservationOrderID from the idempotency token (issue + // #641) so a re-drive re-PUTs the same idempotent Azure reservation order + // instead of creating a second; fall back to the prior timestamped ID + // otherwise. + reservationOrderID := common.ReservationOrderID(opts.IdempotencyToken, uuid.New().String()) apiVersion := "2022-11-01" purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=%s", reservationOrderID, apiVersion) diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index d7cb3ca04..5a9c00f63 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -804,3 +804,60 @@ func TestSearchClient_ValidateOffering_Invalid(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "invalid Azure Search SKU") } + +// searchPurchaseURLFromMock returns the reservation PUT URL captured by the mock +// HTTP client for the last PurchaseCommitment call. The {id} segment of +// .../reservationOrders/{id} is the order ID that makes the request idempotent. +func searchPurchaseURLFromMock(t *testing.T, m *MockHTTPClient) string { + t.Helper() + for i := len(m.Calls) - 1; i >= 0; i-- { + req, ok := m.Calls[i].Arguments.Get(0).(*http.Request) + if ok && req != nil { + return req.URL.String() + } + } + t.Fatalf("no HTTP request captured by mock") + return "" +} + +// TestSearchClient_PurchaseCommitment_IdempotentReDrive is the issue #641 +// regression test for the search executor, whose prior reservationOrderID was a +// non-idempotent timestamp ("search-reservation-"). A re-drive with the +// same IdempotencyToken must now PUT to the same reservationOrders/{id} URL so +// Azure re-PUTs the existing order rather than creating a second reservation. +func TestSearchClient_PurchaseCommitment_IdempotentReDrive(t *testing.T) { + ctx := context.Background() + rec := common.Recommendation{ + ResourceType: "standard", + Term: "1yr", + Count: 1, + CommitmentCost: 3000.0, + } + token := common.DeriveIdempotencyToken("exec-641", 0) + + purchase := func(tok string) (common.PurchaseResult, string) { + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + mockHTTP.On("Do", mock.Anything).Return( + createMockHTTPResponse(http.StatusOK, `{"id": "reservation-123"}`), nil) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{IdempotencyToken: tok}) + require.NoError(t, err) + require.True(t, result.Success) + return result, searchPurchaseURLFromMock(t, mockHTTP) + } + + first, firstURL := purchase(token) + second, secondURL := purchase(token) + + assert.Equal(t, first.CommitmentID, second.CommitmentID, + "same idempotency token must reuse the same reservationOrderID") + assert.Equal(t, firstURL, secondURL, + "re-drive must PUT to the same reservationOrders/{id} URL") + assert.Contains(t, firstURL, common.IdempotencyGUID(token), + "order ID must be derived deterministically from the token, not a timestamp") + + other, otherURL := purchase(common.DeriveIdempotencyToken("exec-641", 1)) + assert.NotEqual(t, first.CommitmentID, other.CommitmentID) + assert.NotEqual(t, firstURL, otherURL) +}