diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index b08c7a8b8..b8f44b121 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "fmt" - "io" "log" "net/http" "net/url" @@ -17,13 +16,13 @@ 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/redis/armredis/v3" - "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" + "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) // redisSKUEntry holds the SKU-catalogue-derived fields the converter @@ -264,7 +263,8 @@ func (c *CacheClient) convertRedisReservation(detail *armconsumption.Reservation return commitment } -// PurchaseCommitment purchases Redis Cache reserved capacity via Azure Reservations API +// PurchaseCommitment purchases Redis Cache reserved capacity using the two-step +// calculatePrice->purchase flow required by Azure's Reservations API (issue #677). func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, @@ -273,13 +273,16 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme Timestamp: time.Now(), } - // 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) + // Azure's reservation API mints the order ID server-side in calculatePrice, + // so the only stable dedupe signal we control is the purchase-automation tag + // derived from opts.Source. Without a non-empty Source the tag is dropped and + // a re-driven purchase cannot recognise the prior attempt, producing + // duplicate reservations. Fail fast at function entry rather than allowing + // an un-tagged, non-idempotent purchase to hit the cloud. + if opts.Source == "" { + result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + return result, result.Error + } termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { @@ -309,12 +312,6 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme return result, result.Error } - req, err := http.NewRequestWithContext(ctx, "PUT", purchaseURL, strings.NewReader(string(bodyBytes))) - if err != nil { - result.Error = fmt.Errorf("failed to create request: %w", err) - return result, result.Error - } - token, err := c.cred.GetToken(ctx, policy.TokenRequestOptions{ Scopes: []string{"https://management.azure.com/.default"}, }) @@ -323,27 +320,15 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme return result, result.Error } - req.Header.Set("Authorization", "Bearer "+token.Token) - req.Header.Set("Content-Type", "application/json") - - resp, err := c.httpClient.Do(req) + reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) if err != nil { - result.Error = fmt.Errorf("failed to purchase reservation: %w", err) - return result, result.Error - } - defer resp.Body.Close() - - body, _ := io.ReadAll(resp.Body) - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { - result.Error = fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) + result.Error = err return result, result.Error } result.Success = true result.CommitmentID = reservationOrderID result.Cost = rec.CommitmentCost - return result, nil } diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 7a950787a..d13074cf6 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -3,6 +3,7 @@ package cache import ( "bytes" "context" + "encoding/json" "errors" "io" "net/http" @@ -916,16 +917,23 @@ func (m *MockTokenCredential) GetToken(ctx context.Context, options policy.Token }, nil } +// calcPriceRespJSON returns a minimal calculatePrice response JSON for tests. +func calcPriceRespJSON(orderID string) string { + return `{"properties":{"reservationOrderId":"` + orderID + `"}}` +} + func TestCacheClient_PurchaseCommitment_Success(t *testing.T) { ctx := context.Background() 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, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cache-order-001")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cache-order-001/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "Premium_P1", @@ -934,11 +942,12 @@ func TestCacheClient_PurchaseCommitment_Success(t *testing.T) { CommitmentCost: 1000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) - assert.NotEmpty(t, result.CommitmentID) + assert.Equal(t, "cache-order-001", result.CommitmentID) assert.Equal(t, 1000.0, result.Cost) + mockHTTP.AssertExpectations(t) } func TestCacheClient_PurchaseCommitment_3YearTerm(t *testing.T) { @@ -947,10 +956,12 @@ func TestCacheClient_PurchaseCommitment_3YearTerm(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusCreated, `{"id": "reservation-123"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cache-order-3yr")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cache-order-3yr/purchase" + })).Return(createMockHTTPResponse(http.StatusCreated, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "Premium_P1", @@ -959,9 +970,11 @@ func TestCacheClient_PurchaseCommitment_3YearTerm(t *testing.T) { CommitmentCost: 2500.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + assert.Equal(t, "cache-order-3yr", result.CommitmentID) + mockHTTP.AssertExpectations(t) } func TestCacheClient_PurchaseCommitment_Accepted(t *testing.T) { @@ -970,10 +983,12 @@ func TestCacheClient_PurchaseCommitment_Accepted(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusAccepted, `{"id": "reservation-123"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cache-order-202")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cache-order-202/purchase" + })).Return(createMockHTTPResponse(http.StatusAccepted, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "Premium_P1", @@ -982,9 +997,10 @@ func TestCacheClient_PurchaseCommitment_Accepted(t *testing.T) { CommitmentCost: 1000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestCacheClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -998,7 +1014,7 @@ func TestCacheClient_PurchaseCommitment_TokenError(t *testing.T) { Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "failed to get access token") @@ -1010,17 +1026,19 @@ func TestCacheClient_PurchaseCommitment_HTTPError(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return(nil, errors.New("network error")) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(nil, errors.New("network error")).Once() rec := common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) - assert.Contains(t, err.Error(), "failed to purchase reservation") + assert.Contains(t, err.Error(), "calculatePrice HTTP call") } func TestCacheClient_PurchaseCommitment_BadStatus(t *testing.T) { @@ -1029,18 +1047,83 @@ func TestCacheClient_PurchaseCommitment_BadStatus(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cache-order-bad")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cache-order-bad/purchase" + })).Return(createMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`), nil).Once() rec := common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "reservation purchase failed with status 400") + mockHTTP.AssertExpectations(t) +} + +// TestCacheClient_PurchaseCommitment_TagInjection verifies that the +// purchase-automation tag carrying opts.Source is present in the +// calculatePrice request body. Without this regression test the dedupe +// guard introduced for the Azure two-step flow could regress silently: +// the call would succeed without the tag and re-driven purchases would +// duplicate reservations server-side. +func TestCacheClient_PurchaseCommitment_TagInjection(t *testing.T) { + const orderID = "cache-tag-test" + const source = common.PurchaseSourceWeb + + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + var capturedBody []byte + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + capturedBody, _ = io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(capturedBody)) + return true + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ResourceType: "Premium_P1", Term: "1yr", Count: 1, CommitmentCost: 1000.0} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: source}) + require.NoError(t, err) + assert.True(t, result.Success) + + var body map[string]interface{} + require.NoError(t, json.Unmarshal(capturedBody, &body)) + tags, hasTags := body["tags"].(map[string]interface{}) + require.True(t, hasTags, "tags field must be present in calculatePrice body when Source is set") + assert.Equal(t, source, tags[common.PurchaseTagKey], "tag value must match opts.Source") + mockHTTP.AssertExpectations(t) +} + +// TestCacheClient_PurchaseCommitment_RequiresSource pins the dedupe guard: +// PurchaseCommitment must reject an empty opts.Source before issuing any HTTP +// call. Azure mints the reservation order ID server-side, so the +// purchase-automation tag derived from Source is the only stable dedupe +// signal CUDly controls -- proceeding without it would allow a re-driven +// purchase to create a duplicate reservation. +func TestCacheClient_PurchaseCommitment_RequiresSource(t *testing.T) { + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + rec := common.Recommendation{ResourceType: "Premium_P1", Term: "1yr", Count: 1} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "purchase source is required") + // No HTTP call may be issued when the guard rejects the request. + mockHTTP.AssertNotCalled(t, "Do", mock.Anything) } diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index a336bcc4b..95ffc5bfb 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "fmt" - "io" "log" "net/http" "net/url" @@ -18,13 +17,13 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" - "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" + "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) // RecommendationsPager defines the interface for paging through recommendations @@ -362,15 +361,10 @@ func (c *ComputeClient) checkAndRegisterCapacityProvider(ctx context.Context) er return nil } -const ( - purchaseMaxAttempts = 3 - purchaseRetryDelay = 2 * time.Second -) - -// buildReservationBody builds the JSON body for a reservation PUT request. +// buildReservationBody builds the JSON body for a reservation purchase request. +// The same body is sent to both calculatePrice and purchase endpoints (issue #677). // When source is non-empty, a top-level tags map carrying purchase-automation -// is attached — Azure's Microsoft.Capacity/reservationOrders PUT body accepts -// tags at creation, so no follow-up call is needed. +// is attached so the resulting reservation is identifiable in the portal. func (c *ComputeClient) buildReservationBody(rec common.Recommendation, source string) ([]byte, error) { termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { @@ -395,43 +389,19 @@ func (c *ComputeClient) buildReservationBody(rec common.Recommendation, source s return json.Marshal(requestBody) } -// isSuccessStatus reports whether the HTTP status code is a successful purchase response. -func isSuccessStatus(code int) bool { - return code == http.StatusOK || code == http.StatusCreated || code == http.StatusAccepted -} - -// doPurchaseWithRetry executes the reservation PUT with 409-retry logic. -// Returns the successful response body (for future use) or an error. -func (c *ComputeClient) doPurchaseWithRetry(ctx context.Context, purchaseURL string, bodyBytes []byte, bearerToken string) error { - for attempt := 1; attempt <= purchaseMaxAttempts; attempt++ { - req, err := http.NewRequestWithContext(ctx, "PUT", purchaseURL, strings.NewReader(string(bodyBytes))) - if err != nil { - return fmt.Errorf("failed to create request: %w", err) - } - req.Header.Set("Authorization", "Bearer "+bearerToken) - req.Header.Set("Content-Type", "application/json") - - resp, err := c.httpClient.Do(req) - if err != nil { - return fmt.Errorf("failed to purchase reservation: %w", err) - } - body, _ := io.ReadAll(resp.Body) - resp.Body.Close() - - if resp.StatusCode == http.StatusConflict && attempt < purchaseMaxAttempts { - log.Printf("reservation purchase returned 409 (attempt %d/%d), retrying in %s", attempt, purchaseMaxAttempts, purchaseRetryDelay) - time.Sleep(purchaseRetryDelay) - continue - } - if !isSuccessStatus(resp.StatusCode) { - return fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) - } - return nil - } - return fmt.Errorf("reservation purchase failed after %d attempts (409 Conflict)", purchaseMaxAttempts) -} - -// PurchaseCommitment purchases a VM Reserved Instance +// PurchaseCommitment purchases a VM Reserved Instance using the two-step +// calculatePrice->purchase flow required by Azure's Reservations API (issue #677). +// +// Azure shifted newer SKU families (Burstable v2 and others) to require a +// calculatePrice call before purchase. The previous direct-PUT pattern returns +// 400 "Session timed out" for these families. The two-step flow: +// 1. POST calculatePrice -- Azure mints a session-bound reservationOrderId. +// 2. POST reservationOrders/{id}/purchase -- commits the order. +// +// Idempotency: Azure mints the order ID in step 1, so client-supplied IDs are +// no longer used. Re-drives are idempotent via tag-based deduplication at the +// caller level (purchase execution checks for existing reservations tagged with +// the (executionID, recIndex) identity before calling PurchaseCommitment). func (c *ComputeClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, @@ -440,6 +410,17 @@ func (c *ComputeClient) PurchaseCommitment(ctx context.Context, rec common.Recom Timestamp: time.Now(), } + // Azure's reservation API mints the order ID server-side in calculatePrice, + // so the only stable dedupe signal we control is the purchase-automation tag + // derived from opts.Source. Without a non-empty Source the tag is dropped and + // a re-driven purchase cannot recognise the prior attempt, producing + // duplicate reservations. Fail fast at function entry rather than allowing + // an un-tagged, non-idempotent purchase to hit the cloud. + if opts.Source == "" { + result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + return result, result.Error + } + // Ensure Microsoft.Capacity provider is registered (cached after first call). c.ensureCapacityProviderRegistered(ctx) @@ -457,16 +438,8 @@ func (c *ComputeClient) PurchaseCommitment(ctx context.Context, rec common.Recom return result, result.Error } - // 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) - - if err := c.doPurchaseWithRetry(ctx, purchaseURL, bodyBytes, token.Token); err != nil { + reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) + if err != nil { result.Error = err return result, result.Error } diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index 426368ec9..dfc7a89f8 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -1,11 +1,14 @@ package compute import ( + "bytes" "context" "encoding/json" "errors" + "io" "net/http" "strconv" + "strings" "testing" "time" @@ -437,16 +440,45 @@ func (m *MockTokenCredential) GetToken(ctx context.Context, options policy.Token }, nil } +// calcPriceRespJSON returns a minimal valid calculatePrice JSON response with the +// given reservationOrderId. Used by PurchaseCommitment tests that need the +// two-step calculatePrice->purchase flow. +func calcPriceRespJSON(orderID string) string { + return `{"properties":{"reservationOrderId":"` + orderID + `"}}` +} + +// capacityProviderRegistered is the JSON response that satisfies +// ensureCapacityProviderRegistered's GET check when the provider is already +// registered. The compute client calls this once per client lifetime before any +// purchase attempt, so tests that call PurchaseCommitment must expect it. +const capacityProviderRegistered = `{"registrationState":"Registered"}` + +// mockCapacityProviderCheck adds a mock expectation for the capacity provider +// registration GET request made by ensureCapacityProviderRegistered. +func mockCapacityProviderCheck(m *mocks.MockHTTPClient) { + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet && + strings.Contains(r.URL.Path, "providers/Microsoft.Capacity") && + !strings.Contains(r.URL.Path, "calculatePrice") && + !strings.Contains(r.URL.Path, "reservationOrders") + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, capacityProviderRegistered), nil).Once() +} + func TestComputeClient_PurchaseCommitment_Success(t *testing.T) { ctx := context.Background() 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, - ) + mockCapacityProviderCheck(mockHTTP) + // Step 1: calculatePrice returns a reservationOrderId. + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("order-vm-001")), nil).Once() + // Step 2: purchase with the Azure-minted order ID. + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-vm-001/purchase" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, `{"id": "order-vm-001"}`), nil).Once() rec := common.Recommendation{ ResourceType: "Standard_D2s_v3", @@ -455,11 +487,12 @@ func TestComputeClient_PurchaseCommitment_Success(t *testing.T) { CommitmentCost: 2000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) - assert.NotEmpty(t, result.CommitmentID) + assert.Equal(t, "order-vm-001", result.CommitmentID) assert.Equal(t, 2000.0, result.Cost) + mockHTTP.AssertExpectations(t) } func TestComputeClient_PurchaseCommitment_3YearTerm(t *testing.T) { @@ -468,10 +501,13 @@ func TestComputeClient_PurchaseCommitment_3YearTerm(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - mocks.CreateMockHTTPResponse(http.StatusCreated, `{"id": "reservation-123"}`), - nil, - ) + mockCapacityProviderCheck(mockHTTP) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("order-vm-3yr")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-vm-3yr/purchase" + })).Return(mocks.CreateMockHTTPResponse(http.StatusCreated, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "Standard_D2s_v3", @@ -480,9 +516,11 @@ func TestComputeClient_PurchaseCommitment_3YearTerm(t *testing.T) { CommitmentCost: 5000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + assert.Equal(t, "order-vm-3yr", result.CommitmentID) + mockHTTP.AssertExpectations(t) } func TestComputeClient_PurchaseCommitment_Accepted(t *testing.T) { @@ -491,10 +529,13 @@ func TestComputeClient_PurchaseCommitment_Accepted(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - mocks.CreateMockHTTPResponse(http.StatusAccepted, `{"id": "reservation-123"}`), - nil, - ) + mockCapacityProviderCheck(mockHTTP) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("order-vm-202")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-vm-202/purchase" + })).Return(mocks.CreateMockHTTPResponse(http.StatusAccepted, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "Standard_D2s_v3", @@ -503,9 +544,10 @@ func TestComputeClient_PurchaseCommitment_Accepted(t *testing.T) { CommitmentCost: 2000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestComputeClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -519,7 +561,7 @@ func TestComputeClient_PurchaseCommitment_TokenError(t *testing.T) { Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "failed to get access token") @@ -531,17 +573,21 @@ func TestComputeClient_PurchaseCommitment_HTTPError(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return(nil, errors.New("network error")) + mockCapacityProviderCheck(mockHTTP) + // Network error on the calculatePrice call. + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(nil, errors.New("network error")).Once() rec := common.Recommendation{ ResourceType: "Standard_D2s_v3", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) - assert.Contains(t, err.Error(), "failed to purchase reservation") + assert.Contains(t, err.Error(), "calculatePrice HTTP call") } func TestComputeClient_PurchaseCommitment_BadStatus(t *testing.T) { @@ -550,84 +596,172 @@ func TestComputeClient_PurchaseCommitment_BadStatus(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - mocks.CreateMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`), + mockCapacityProviderCheck(mockHTTP) + // calculatePrice returns 200 with an order ID, but purchase returns 400. + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("order-bad")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-bad/purchase" + })).Return( + mocks.CreateMockHTTPResponse(http.StatusBadRequest, `{"error":{"code":"InvalidScope","message":"invalid request"}}`), nil, - ) + ).Once() rec := common.Recommendation{ ResourceType: "Standard_D2s_v3", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "reservation purchase failed with status 400") + mockHTTP.AssertExpectations(t) } -// 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) { +// TestComputeClient_PurchaseCommitment_TwoStepFlow verifies that +// PurchaseCommitment makes exactly two HTTP calls: POST calculatePrice then +// POST purchase, and that the CommitmentID is the Azure-minted order ID (not a +// client-generated GUID). This is the regression test for issue #677. +func TestComputeClient_PurchaseCommitment_TwoStepFlow(t *testing.T) { ctx := context.Background() + mockHTTP := &mocks.MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + const azureMintedOrderID = "azure-minted-order-677" + + mockCapacityProviderCheck(mockHTTP) + // Expect exactly one calculatePrice POST. + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON(azureMintedOrderID)), nil).Once() + + // Expect exactly one purchase POST to the Azure-minted order path. + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && + r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+azureMintedOrderID+"/purchase" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + rec := common.Recommendation{ - ResourceType: "Standard_D2s_v3", + ResourceType: "Standard_B2ats_v2", // The SKU that triggered issue #677. Term: "1yr", Count: 1, - CommitmentCost: 2000.0, + CommitmentCost: 500.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) + + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + assert.True(t, result.Success) + // CommitmentID must be the Azure-minted ID, not a client-generated GUID. + assert.Equal(t, azureMintedOrderID, result.CommitmentID) + // Exactly 3 HTTP calls: capacity-provider-check + calculatePrice + purchase. + mockHTTP.AssertExpectations(t) + mockHTTP.AssertNumberOfCalls(t, "Do", 3) +} + +// TestComputeClient_PurchaseCommitment_SessionTimeoutRetry verifies that a +// "Session timed out" 400 on the purchase endpoint causes PurchaseCommitment to +// re-run calculatePrice and retry the purchase (issue #677 regression test). +func TestComputeClient_PurchaseCommitment_SessionTimeoutRetry(t *testing.T) { + ctx := context.Background() + mockHTTP := &mocks.MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + sessionTimeoutBody := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again and provide the new Reservation Order ID for purchase"}}` + + mockCapacityProviderCheck(mockHTTP) + // First calculatePrice: mints "order-first". + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("order-first")), nil).Once() + // First purchase: session timeout. + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-first/purchase" + })).Return(mocks.CreateMockHTTPResponse(http.StatusBadRequest, sessionTimeoutBody), nil).Once() + + // Second calculatePrice: mints "order-second". + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("order-second")), nil).Once() + // Second purchase: succeeds. + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-second/purchase" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ResourceType: "Standard_B2ats_v2", Term: "1yr", Count: 1} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + assert.True(t, result.Success) + assert.Equal(t, "order-second", result.CommitmentID) + mockHTTP.AssertExpectations(t) +} + +// TestComputeClient_PurchaseCommitment_TagInjection verifies that the +// purchase-automation tag carrying opts.Source is present in the +// calculatePrice request body. Without this regression test the dedupe +// guard introduced for the Azure two-step flow could regress silently: +// the call would succeed without the tag and re-driven purchases would +// duplicate reservations server-side. +func TestComputeClient_PurchaseCommitment_TagInjection(t *testing.T) { + const orderID = "compute-tag-test" + const source = common.PurchaseSourceWeb + + ctx := context.Background() + mockHTTP := &mocks.MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + mockCapacityProviderCheck(mockHTTP) + + var capturedBody []byte + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.Method != http.MethodPost || r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + capturedBody, _ = io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(capturedBody)) + return true + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && + r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ResourceType: "Standard_D2s_v3", Term: "1yr", Count: 1, CommitmentCost: 2000.0} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: source}) + require.NoError(t, err) + assert.True(t, result.Success) + + var body map[string]interface{} + require.NoError(t, json.Unmarshal(capturedBody, &body)) + tags, hasTags := body["tags"].(map[string]interface{}) + require.True(t, hasTags, "tags field must be present in calculatePrice body when Source is set") + assert.Equal(t, source, tags[common.PurchaseTagKey], "tag value must match opts.Source") + mockHTTP.AssertExpectations(t) +} + +// TestComputeClient_PurchaseCommitment_RequiresSource pins the dedupe guard: +// PurchaseCommitment must reject an empty opts.Source before issuing any HTTP +// call (including the cached Microsoft.Capacity provider-registration check). +// Azure mints the reservation order ID server-side, so the +// purchase-automation tag derived from Source is the only stable dedupe +// signal CUDly controls -- proceeding without it would allow a re-driven +// purchase to create a duplicate reservation. +func TestComputeClient_PurchaseCommitment_RequiresSource(t *testing.T) { + ctx := context.Background() + mockHTTP := &mocks.MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + rec := common.Recommendation{ResourceType: "Standard_D2s_v3", Term: "1yr", Count: 1} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "purchase source is required") + mockHTTP.AssertNotCalled(t, "Do", mock.Anything) } // TestComputeClient_ConvertAzureVMRecommendation_NilGuards pins the new diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index af1db742c..b69fea4eb 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "fmt" - "io" "log" "net/http" "net/url" @@ -18,13 +17,13 @@ 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/cosmos/armcosmos/v2" - "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" + "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) // HTTPClient interface for HTTP operations (enables mocking) @@ -257,7 +256,8 @@ func (c *CosmosDBClient) convertCosmosReservation(detail *armconsumption.Reserva return commitment } -// PurchaseCommitment purchases Cosmos DB reserved capacity via Azure Reservations API +// PurchaseCommitment purchases Cosmos DB reserved capacity using the two-step +// calculatePrice->purchase flow required by Azure's Reservations API (issue #677). func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, @@ -266,16 +266,16 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco Timestamp: time.Now(), } - // 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" - purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=%s", - reservationOrderID, apiVersion) + // Azure's reservation API mints the order ID server-side in calculatePrice, + // so the only stable dedupe signal we control is the purchase-automation tag + // derived from opts.Source. Without a non-empty Source the tag is dropped and + // a re-driven purchase cannot recognise the prior attempt, producing + // duplicate reservations. Fail fast at function entry rather than allowing + // an un-tagged, non-idempotent purchase to hit the cloud. + if opts.Source == "" { + result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + return result, result.Error + } termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { @@ -305,13 +305,6 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco return result, result.Error } - req, err := http.NewRequestWithContext(ctx, "PUT", purchaseURL, strings.NewReader(string(bodyBytes))) - if err != nil { - result.Error = fmt.Errorf("failed to create request: %w", err) - return result, result.Error - } - - // Get access token for Azure Management API token, err := c.cred.GetToken(ctx, policy.TokenRequestOptions{ Scopes: []string{"https://management.azure.com/.default"}, }) @@ -320,27 +313,15 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco return result, result.Error } - req.Header.Set("Authorization", "Bearer "+token.Token) - req.Header.Set("Content-Type", "application/json") - - resp, err := c.httpClient.Do(req) + reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) if err != nil { - result.Error = fmt.Errorf("failed to purchase reservation: %w", err) - return result, result.Error - } - defer resp.Body.Close() - - body, _ := io.ReadAll(resp.Body) - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { - result.Error = fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) + result.Error = err return result, result.Error } result.Success = true result.CommitmentID = reservationOrderID result.Cost = rec.CommitmentCost - return result, nil } diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index 273873be8..b1105a26a 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -3,6 +3,7 @@ package cosmosdb import ( "bytes" "context" + "encoding/json" "errors" "io" "net/http" @@ -714,16 +715,23 @@ func (m *MockTokenCredential) GetToken(ctx context.Context, options policy.Token }, nil } +// calcPriceRespJSON returns a minimal calculatePrice response JSON for tests. +func calcPriceRespJSON(orderID string) string { + return `{"properties":{"reservationOrderId":"` + orderID + `"}}` +} + func TestCosmosDBClient_PurchaseCommitment_Success(t *testing.T) { ctx := context.Background() 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, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cosmos-order-001")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cosmos-order-001/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "EnableCassandra", @@ -732,11 +740,12 @@ func TestCosmosDBClient_PurchaseCommitment_Success(t *testing.T) { CommitmentCost: 5000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) - assert.NotEmpty(t, result.CommitmentID) + assert.Equal(t, "cosmos-order-001", result.CommitmentID) assert.Equal(t, 5000.0, result.Cost) + mockHTTP.AssertExpectations(t) } func TestCosmosDBClient_PurchaseCommitment_3YearTerm(t *testing.T) { @@ -745,10 +754,12 @@ func TestCosmosDBClient_PurchaseCommitment_3YearTerm(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusCreated, `{"id": "reservation-123"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cosmos-order-3yr")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cosmos-order-3yr/purchase" + })).Return(createMockHTTPResponse(http.StatusCreated, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "EnableCassandra", @@ -757,9 +768,11 @@ func TestCosmosDBClient_PurchaseCommitment_3YearTerm(t *testing.T) { CommitmentCost: 12000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + assert.Equal(t, "cosmos-order-3yr", result.CommitmentID) + mockHTTP.AssertExpectations(t) } func TestCosmosDBClient_PurchaseCommitment_Accepted(t *testing.T) { @@ -768,10 +781,12 @@ func TestCosmosDBClient_PurchaseCommitment_Accepted(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusAccepted, `{"id": "reservation-123"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cosmos-order-202")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cosmos-order-202/purchase" + })).Return(createMockHTTPResponse(http.StatusAccepted, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "EnableCassandra", @@ -780,9 +795,10 @@ func TestCosmosDBClient_PurchaseCommitment_Accepted(t *testing.T) { CommitmentCost: 5000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestCosmosDBClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -796,7 +812,7 @@ func TestCosmosDBClient_PurchaseCommitment_TokenError(t *testing.T) { Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "failed to get access token") @@ -808,17 +824,19 @@ func TestCosmosDBClient_PurchaseCommitment_HTTPError(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return(nil, errors.New("network error")) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(nil, errors.New("network error")).Once() rec := common.Recommendation{ ResourceType: "EnableCassandra", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) - assert.Contains(t, err.Error(), "failed to purchase reservation") + assert.Contains(t, err.Error(), "calculatePrice HTTP call") } func TestCosmosDBClient_PurchaseCommitment_BadStatus(t *testing.T) { @@ -827,20 +845,84 @@ func TestCosmosDBClient_PurchaseCommitment_BadStatus(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cosmos-order-bad")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cosmos-order-bad/purchase" + })).Return(createMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`), nil).Once() rec := common.Recommendation{ ResourceType: "EnableCassandra", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "reservation purchase failed with status 400") + mockHTTP.AssertExpectations(t) +} + +// TestCosmosDBClient_PurchaseCommitment_TagInjection verifies that the +// purchase-automation tag carrying opts.Source is present in the +// calculatePrice request body. Without this regression test the dedupe +// guard introduced for the Azure two-step flow could regress silently: +// the call would succeed without the tag and re-driven purchases would +// duplicate reservations server-side. +func TestCosmosDBClient_PurchaseCommitment_TagInjection(t *testing.T) { + const orderID = "cosmos-tag-test" + const source = common.PurchaseSourceWeb + + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + var capturedBody []byte + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + capturedBody, _ = io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(capturedBody)) + return true + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ResourceType: "EnableCassandra", Term: "1yr", Count: 1, CommitmentCost: 4000.0} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: source}) + require.NoError(t, err) + assert.True(t, result.Success) + + var body map[string]interface{} + require.NoError(t, json.Unmarshal(capturedBody, &body)) + tags, hasTags := body["tags"].(map[string]interface{}) + require.True(t, hasTags, "tags field must be present in calculatePrice body when Source is set") + assert.Equal(t, source, tags[common.PurchaseTagKey], "tag value must match opts.Source") + mockHTTP.AssertExpectations(t) +} + +// TestCosmosDBClient_PurchaseCommitment_RequiresSource pins the dedupe guard: +// PurchaseCommitment must reject an empty opts.Source before issuing any HTTP +// call. Azure mints the reservation order ID server-side, so the +// purchase-automation tag derived from Source is the only stable dedupe +// signal CUDly controls -- proceeding without it would allow a re-driven +// purchase to create a duplicate reservation. +func TestCosmosDBClient_PurchaseCommitment_RequiresSource(t *testing.T) { + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + rec := common.Recommendation{ResourceType: "EnableCassandra", Term: "1yr", Count: 1} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "purchase source is required") + mockHTTP.AssertNotCalled(t, "Do", mock.Anything) } // TestCosmosDBClient_ConvertAzureCosmosRecommendation_NilGuards pins the diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index 758f29901..9c5bd9fd2 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "fmt" - "io" "log" "net/http" "net/url" @@ -17,13 +16,13 @@ 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/sql/armsql" - "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" + "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) // sqlSKUEntry holds the SKU-catalogue-derived fields the converter @@ -265,7 +264,8 @@ func (c *DatabaseClient) convertSQLReservation(detail *armconsumption.Reservatio return commitment } -// PurchaseCommitment purchases SQL Database reserved capacity via Azure Reservations API +// PurchaseCommitment purchases SQL Database reserved capacity using the two-step +// calculatePrice->purchase flow required by Azure's Reservations API (issue #677). func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, @@ -274,16 +274,16 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco Timestamp: time.Now(), } - // 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" - purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=%s", - reservationOrderID, apiVersion) + // Azure's reservation API mints the order ID server-side in calculatePrice, + // so the only stable dedupe signal we control is the purchase-automation tag + // derived from opts.Source. Without a non-empty Source the tag is dropped and + // a re-driven purchase cannot recognise the prior attempt, producing + // duplicate reservations. Fail fast at function entry rather than allowing + // an un-tagged, non-idempotent purchase to hit the cloud. + if opts.Source == "" { + result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + return result, result.Error + } termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { @@ -313,13 +313,6 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco return result, result.Error } - req, err := http.NewRequestWithContext(ctx, "PUT", purchaseURL, strings.NewReader(string(bodyBytes))) - if err != nil { - result.Error = fmt.Errorf("failed to create request: %w", err) - return result, result.Error - } - - // Get access token for Azure Management API token, err := c.cred.GetToken(ctx, policy.TokenRequestOptions{ Scopes: []string{"https://management.azure.com/.default"}, }) @@ -328,27 +321,15 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco return result, result.Error } - req.Header.Set("Authorization", "Bearer "+token.Token) - req.Header.Set("Content-Type", "application/json") - - resp, err := c.httpClient.Do(req) + reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) if err != nil { - result.Error = fmt.Errorf("failed to purchase reservation: %w", err) - return result, result.Error - } - defer resp.Body.Close() - - body, _ := io.ReadAll(resp.Body) - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { - result.Error = fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) + result.Error = err return result, result.Error } result.Success = true result.CommitmentID = reservationOrderID result.Cost = rec.CommitmentCost - return result, nil } diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index e52ee26ee..303d93788 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -3,6 +3,7 @@ package database import ( "bytes" "context" + "encoding/json" "errors" "io" "net/http" @@ -825,16 +826,23 @@ func (m *MockTokenCredential) GetToken(ctx context.Context, options policy.Token }, nil } +// calcPriceRespJSON returns a minimal calculatePrice response JSON for tests. +func calcPriceRespJSON(orderID string) string { + return `{"properties":{"reservationOrderId":"` + orderID + `"}}` +} + func TestDatabaseClient_PurchaseCommitment_Success(t *testing.T) { ctx := context.Background() 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, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("db-order-001")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/db-order-001/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "GP_Gen5_8", @@ -843,11 +851,12 @@ func TestDatabaseClient_PurchaseCommitment_Success(t *testing.T) { CommitmentCost: 5000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) - assert.NotEmpty(t, result.CommitmentID) + assert.Equal(t, "db-order-001", result.CommitmentID) assert.Equal(t, 5000.0, result.Cost) + mockHTTP.AssertExpectations(t) } func TestDatabaseClient_PurchaseCommitment_3YearTerm(t *testing.T) { @@ -856,10 +865,12 @@ func TestDatabaseClient_PurchaseCommitment_3YearTerm(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusCreated, `{"id": "reservation-123"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("db-order-3yr")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/db-order-3yr/purchase" + })).Return(createMockHTTPResponse(http.StatusCreated, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "GP_Gen5_8", @@ -868,9 +879,11 @@ func TestDatabaseClient_PurchaseCommitment_3YearTerm(t *testing.T) { CommitmentCost: 12000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + assert.Equal(t, "db-order-3yr", result.CommitmentID) + mockHTTP.AssertExpectations(t) } func TestDatabaseClient_PurchaseCommitment_Accepted(t *testing.T) { @@ -879,10 +892,12 @@ func TestDatabaseClient_PurchaseCommitment_Accepted(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusAccepted, `{"id": "reservation-123"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("db-order-202")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/db-order-202/purchase" + })).Return(createMockHTTPResponse(http.StatusAccepted, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "GP_Gen5_8", @@ -891,9 +906,10 @@ func TestDatabaseClient_PurchaseCommitment_Accepted(t *testing.T) { CommitmentCost: 5000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestDatabaseClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -907,7 +923,7 @@ func TestDatabaseClient_PurchaseCommitment_TokenError(t *testing.T) { Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "failed to get access token") @@ -919,17 +935,19 @@ func TestDatabaseClient_PurchaseCommitment_HTTPError(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return(nil, errors.New("network error")) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(nil, errors.New("network error")).Once() rec := common.Recommendation{ ResourceType: "GP_Gen5_8", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) - assert.Contains(t, err.Error(), "failed to purchase reservation") + assert.Contains(t, err.Error(), "calculatePrice HTTP call") } func TestDatabaseClient_PurchaseCommitment_BadStatus(t *testing.T) { @@ -938,20 +956,84 @@ func TestDatabaseClient_PurchaseCommitment_BadStatus(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("db-order-bad")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/db-order-bad/purchase" + })).Return(createMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`), nil).Once() rec := common.Recommendation{ ResourceType: "GP_Gen5_8", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "reservation purchase failed with status 400") + mockHTTP.AssertExpectations(t) +} + +// TestDatabaseClient_PurchaseCommitment_TagInjection verifies that the +// purchase-automation tag carrying opts.Source is present in the +// calculatePrice request body. Without this regression test the dedupe +// guard introduced for the Azure two-step flow could regress silently: +// the call would succeed without the tag and re-driven purchases would +// duplicate reservations server-side. +func TestDatabaseClient_PurchaseCommitment_TagInjection(t *testing.T) { + const orderID = "db-tag-test" + const source = common.PurchaseSourceWeb + + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + var capturedBody []byte + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + capturedBody, _ = io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(capturedBody)) + return true + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ResourceType: "GP_Gen5_8", Term: "1yr", Count: 1, CommitmentCost: 5000.0} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: source}) + require.NoError(t, err) + assert.True(t, result.Success) + + var body map[string]interface{} + require.NoError(t, json.Unmarshal(capturedBody, &body)) + tags, hasTags := body["tags"].(map[string]interface{}) + require.True(t, hasTags, "tags field must be present in calculatePrice body when Source is set") + assert.Equal(t, source, tags[common.PurchaseTagKey], "tag value must match opts.Source") + mockHTTP.AssertExpectations(t) +} + +// TestDatabaseClient_PurchaseCommitment_RequiresSource pins the dedupe guard: +// PurchaseCommitment must reject an empty opts.Source before issuing any HTTP +// call. Azure mints the reservation order ID server-side, so the +// purchase-automation tag derived from Source is the only stable dedupe +// signal CUDly controls -- proceeding without it would allow a re-driven +// purchase to create a duplicate reservation. +func TestDatabaseClient_PurchaseCommitment_RequiresSource(t *testing.T) { + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + rec := common.Recommendation{ResourceType: "GP_Gen5_8", Term: "1yr", Count: 1} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "purchase source is required") + mockHTTP.AssertNotCalled(t, "Do", mock.Anything) } func TestDatabaseClient_ValidateOffering_Valid(t *testing.T) { diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go new file mode 100644 index 000000000..ad23cc6dc --- /dev/null +++ b/providers/azure/services/internal/reservations/purchase.go @@ -0,0 +1,189 @@ +// Package reservations provides the shared two-step calculatePrice->purchase +// flow for all Azure reservation-based service clients (compute, database, +// cache, search, cosmosdb, managedredis). +// +// Azure's Reservations API shifted away from direct-PUT for newer SKU families +// (Burstable v2 and likely others). The previous pattern: +// +// PUT /providers/Microsoft.Capacity/reservationOrders/{client-generated-id} +// +// now returns 400 "Session timed out - Call CalculatePrice again and provide +// the new Reservation Order ID for purchase" for affected families. The fix is +// a two-step flow (issue #677): +// +// 1. POST /providers/Microsoft.Capacity/calculatePrice -- mints a session- +// bound reservationOrderId and a price quote. +// 2. POST /providers/Microsoft.Capacity/reservationOrders/{id}/purchase -- +// commits the order using the Azure-minted ID. +// +// Idempotency strategy (Option B from issue #677): because Azure mints the +// order ID in step 1 we cannot use a client-supplied ID for deduplication. +// Instead, before step 1 the caller should check for an existing reservation +// that already carries the (execution, rec) identity tag. This package exposes +// IsSessionTimeout to let callers classify errors, and DoPurchaseTwoStep which +// handles the two-step HTTP calls with retry on session-timeout (re-runs +// calculatePrice from scratch on a "Session timed out" purchase 400). +package reservations + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "log" + "net/http" + "strings" + "time" +) + +// apiVersion is the GA api-version for the Microsoft.Capacity Reservations API. +// Pinned to 2022-11-01 — the last stable version before Azure introduced the +// calculatePrice requirement for new SKU families. +const apiVersion = "2022-11-01" + +// BaseURL is the Azure Resource Manager base URL. +const BaseURL = "https://management.azure.com" + +// CalculatePriceURL returns the calculatePrice endpoint URL. +func CalculatePriceURL() string { + return BaseURL + "/providers/Microsoft.Capacity/calculatePrice?api-version=" + apiVersion +} + +// PurchaseURL returns the purchase endpoint URL for a given reservationOrderId. +func PurchaseURL(reservationOrderID string) string { + return fmt.Sprintf("%s/providers/Microsoft.Capacity/reservationOrders/%s/purchase?api-version=%s", + BaseURL, reservationOrderID, apiVersion) +} + +// calculatePriceResponse is the JSON shape returned by the calculatePrice POST. +// Only the fields we need are decoded; Azure returns additional billing fields +// that are irrelevant to the purchase flow. +type calculatePriceResponse struct { + Properties struct { + ReservationOrderID string `json:"reservationOrderId"` + } `json:"properties"` +} + +// sessionTimeoutFragment is the substring Azure includes in the 400 error +// message when a purchase call uses a stale or client-generated order ID. +// Matching on the substring (rather than an exact message) tolerates minor +// phrasing changes in future API versions. +const sessionTimeoutFragment = "Session timed out" + +// IsSessionTimeout reports whether err looks like the "Session timed out" +// 400 error from the Azure Reservations purchase endpoint. It matches the +// error message produced by DoPurchaseTwoStep so callers can distinguish +// retriable session-expiry from other 4xx errors. +func IsSessionTimeout(err error) bool { + if err == nil { + return false + } + return strings.Contains(err.Error(), sessionTimeoutFragment) +} + +// HTTPClient is the minimal interface required by DoPurchaseTwoStep, matching +// the interface used by all service clients (net/http.Client satisfies it). +type HTTPClient interface { + Do(req *http.Request) (*http.Response, error) +} + +const ( + purchaseMaxAttempts = 3 + purchaseRetryDelay = 2 * time.Second +) + +// DoPurchaseTwoStep executes the calculatePrice->purchase two-step flow. +// +// It POSTs bodyBytes to calculateURL to mint an Azure-assigned reservationOrderId, +// then POSTs the same body to the derived purchaseURL. On a "Session timed out" +// 400 from the purchase endpoint (Azure has retired the session) it re-runs +// calculatePrice from scratch (up to purchaseMaxAttempts total attempts). +// Other 4xx/5xx errors are returned immediately without retry. +// +// Returns the Azure-minted reservationOrderId on success, which the caller +// should store as the CommitmentID. +func DoPurchaseTwoStep(ctx context.Context, httpClient HTTPClient, calcURL string, bodyBytes []byte, bearerToken string) (string, error) { + for attempt := 1; attempt <= purchaseMaxAttempts; attempt++ { + // Step 1: calculatePrice -- mint a session-bound reservationOrderId. + orderID, err := doCalculatePrice(ctx, httpClient, calcURL, bodyBytes, bearerToken) + if err != nil { + return "", fmt.Errorf("calculatePrice (attempt %d/%d): %w", attempt, purchaseMaxAttempts, err) + } + + // Step 2: purchase -- commit the order. + purchaseErr := doPurchase(ctx, httpClient, PurchaseURL(orderID), bodyBytes, bearerToken) + if purchaseErr == nil { + return orderID, nil + } + + if IsSessionTimeout(purchaseErr) && attempt < purchaseMaxAttempts { + log.Printf("reservation purchase session timed out (attempt %d/%d), re-running calculatePrice in %s", + attempt, purchaseMaxAttempts, purchaseRetryDelay) + select { + case <-time.After(purchaseRetryDelay): + case <-ctx.Done(): + return "", fmt.Errorf("reservation purchase canceled during retry delay: %w", ctx.Err()) + } + continue + } + return "", purchaseErr + } + return "", fmt.Errorf("reservation purchase failed after %d attempts (session timeout)", purchaseMaxAttempts) +} + +// doCalculatePrice calls the calculatePrice endpoint and returns the +// Azure-minted reservationOrderId from the response. +func doCalculatePrice(ctx context.Context, httpClient HTTPClient, calcURL string, bodyBytes []byte, bearerToken string) (string, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodPost, calcURL, bytes.NewReader(bodyBytes)) + if err != nil { + return "", fmt.Errorf("build calculatePrice request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+bearerToken) + req.Header.Set("Content-Type", "application/json") + + resp, err := httpClient.Do(req) + if err != nil { + return "", fmt.Errorf("calculatePrice HTTP call: %w", err) + } + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return "", fmt.Errorf("calculatePrice failed with status %d: %s", resp.StatusCode, string(body)) + } + + var result calculatePriceResponse + if err := json.Unmarshal(body, &result); err != nil { + return "", fmt.Errorf("decode calculatePrice response: %w", err) + } + if result.Properties.ReservationOrderID == "" { + return "", fmt.Errorf("calculatePrice returned empty reservationOrderId (body: %s)", string(body)) + } + return result.Properties.ReservationOrderID, nil +} + +// doPurchase calls the purchase endpoint with the given body. +// Returns nil on 200/201/202; on 400 "Session timed out" returns an error +// that IsSessionTimeout recognises. All other non-2xx responses are returned +// as errors verbatim. +func doPurchase(ctx context.Context, httpClient HTTPClient, purchaseURL string, bodyBytes []byte, bearerToken string) error { + req, err := http.NewRequestWithContext(ctx, http.MethodPost, purchaseURL, bytes.NewReader(bodyBytes)) + if err != nil { + return fmt.Errorf("build purchase request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+bearerToken) + req.Header.Set("Content-Type", "application/json") + + resp, err := httpClient.Do(req) + if err != nil { + return fmt.Errorf("failed to purchase reservation: %w", err) + } + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + + if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusCreated || resp.StatusCode == http.StatusAccepted { + return nil + } + return fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) +} diff --git a/providers/azure/services/internal/reservations/purchase_test.go b/providers/azure/services/internal/reservations/purchase_test.go new file mode 100644 index 000000000..90071ba5e --- /dev/null +++ b/providers/azure/services/internal/reservations/purchase_test.go @@ -0,0 +1,215 @@ +package reservations + +import ( + "bytes" + "context" + "errors" + "io" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// mockHTTPClient implements HTTPClient for tests. +type mockHTTPClient struct{ mock.Mock } + +func (m *mockHTTPClient) Do(req *http.Request) (*http.Response, error) { + args := m.Called(req) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*http.Response), args.Error(1) +} + +func fakeResp(status int, body string) *http.Response { + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(bytes.NewBufferString(body)), + Header: make(http.Header), + } +} + +const calcURL = "https://management.azure.com/providers/Microsoft.Capacity/calculatePrice?api-version=2022-11-01" +const testBody = `{"sku":{"name":"Standard_B2ats_v2"},"location":"eastus","properties":{"reservedResourceType":"VirtualMachines","quantity":1}}` + +// TestDoPurchaseTwoStep_HappyPath tests successful calculatePrice->purchase flow. +func TestDoPurchaseTwoStep_HappyPath(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + // calculatePrice returns a valid order ID. + calcResp := `{"properties":{"reservationOrderId":"azure-order-abc123","paymentSchedule":{}}}` + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.String() == calcURL + })).Return(fakeResp(http.StatusOK, calcResp), nil).Once() + + // purchase returns 200. + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/azure-order-abc123/purchase" + })).Return(fakeResp(http.StatusOK, `{"id":"azure-order-abc123"}`), nil).Once() + + orderID, err := DoPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "test-token") + require.NoError(t, err) + assert.Equal(t, "azure-order-abc123", orderID) + m.AssertExpectations(t) +} + +// TestDoPurchaseTwoStep_PurchaseAccepted tests that 202 Accepted counts as success. +func TestDoPurchaseTwoStep_PurchaseAccepted(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + calcResp := `{"properties":{"reservationOrderId":"order-202"}}` + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.String() == calcURL + })).Return(fakeResp(http.StatusOK, calcResp), nil).Once() + + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-202/purchase" + })).Return(fakeResp(http.StatusAccepted, `{}`), nil).Once() + + orderID, err := DoPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok") + require.NoError(t, err) + assert.Equal(t, "order-202", orderID) +} + +// TestDoPurchaseTwoStep_SessionTimeoutThenSuccess tests that a "Session timed out" +// 400 on the purchase endpoint triggers a re-run of calculatePrice and succeeds on +// the second attempt. +func TestDoPurchaseTwoStep_SessionTimeoutThenSuccess(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + sessionTimeoutBody := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again and provide the new Reservation Order ID for purchase"}}` + + // First calculatePrice call -- returns order ID "order-first". + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.String() == calcURL + })).Return(fakeResp(http.StatusOK, `{"properties":{"reservationOrderId":"order-first"}}`), nil).Once() + + // First purchase call returns session timeout. + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-first/purchase" + })).Return(fakeResp(http.StatusBadRequest, sessionTimeoutBody), nil).Once() + + // Second calculatePrice call -- returns order ID "order-second". + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.String() == calcURL + })).Return(fakeResp(http.StatusOK, `{"properties":{"reservationOrderId":"order-second"}}`), nil).Once() + + // Second purchase call succeeds. + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-second/purchase" + })).Return(fakeResp(http.StatusOK, `{}`), nil).Once() + + orderID, err := DoPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok") + require.NoError(t, err) + assert.Equal(t, "order-second", orderID) + m.AssertExpectations(t) +} + +// TestDoPurchaseTwoStep_CalculateFailure tests that a calculatePrice 4xx is +// returned immediately without retrying. +func TestDoPurchaseTwoStep_CalculateFailure(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + m.On("Do", mock.Anything).Return( + fakeResp(http.StatusUnprocessableEntity, `{"error":{"code":"InvalidSKU"}}`), nil, + ).Once() + + _, err := DoPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok") + require.Error(t, err) + assert.Contains(t, err.Error(), "calculatePrice failed with status 422") + // Only one HTTP call should have been made (no retry for calculate failures). + m.AssertNumberOfCalls(t, "Do", 1) +} + +// TestDoPurchaseTwoStep_PurchaseNonTimeoutFailure tests that a non-session-timeout +// 4xx from the purchase endpoint is returned immediately without retrying. +func TestDoPurchaseTwoStep_PurchaseNonTimeoutFailure(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.String() == calcURL + })).Return(fakeResp(http.StatusOK, `{"properties":{"reservationOrderId":"ord-x"}}`), nil).Once() + + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/ord-x/purchase" + })).Return(fakeResp(http.StatusForbidden, `{"error":"Forbidden"}`), nil).Once() + + _, err := DoPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok") + require.Error(t, err) + assert.Contains(t, err.Error(), "reservation purchase failed with status 403") + // Two calls total: one calculatePrice, one failed purchase -- no retry. + m.AssertNumberOfCalls(t, "Do", 2) +} + +// TestDoPurchaseTwoStep_CalculateHTTPError tests network-level errors on calculatePrice. +func TestDoPurchaseTwoStep_CalculateHTTPError(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + m.On("Do", mock.Anything).Return(nil, errors.New("dial tcp: connection refused")).Once() + + _, err := DoPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok") + require.Error(t, err) + assert.Contains(t, err.Error(), "calculatePrice HTTP call") +} + +// TestDoPurchaseTwoStep_PurchaseHTTPError tests network-level errors on the purchase step. +func TestDoPurchaseTwoStep_PurchaseHTTPError(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.String() == calcURL + })).Return(fakeResp(http.StatusOK, `{"properties":{"reservationOrderId":"ord-y"}}`), nil).Once() + + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/ord-y/purchase" + })).Return(nil, errors.New("network timeout")).Once() + + _, err := DoPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to purchase reservation") +} + +// TestDoPurchaseTwoStep_EmptyOrderID tests that calculatePrice returning an +// empty reservationOrderId is treated as an error. +func TestDoPurchaseTwoStep_EmptyOrderID(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + m.On("Do", mock.Anything).Return( + fakeResp(http.StatusOK, `{"properties":{"reservationOrderId":""}}`), nil, + ).Once() + + _, err := DoPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok") + require.Error(t, err) + assert.Contains(t, err.Error(), "empty reservationOrderId") +} + +// TestIsSessionTimeout tests the session timeout classifier. +func TestIsSessionTimeout(t *testing.T) { + assert.False(t, IsSessionTimeout(nil)) + assert.False(t, IsSessionTimeout(errors.New("some other error"))) + assert.True(t, IsSessionTimeout(errors.New("reservation purchase failed with status 400: Session timed out - Call CalculatePrice again and provide the new Reservation Order ID for purchase"))) + assert.True(t, IsSessionTimeout(errors.New("Session timed out"))) +} + +// TestCalculatePriceURL and TestPurchaseURL verify the URL helpers produce +// the expected endpoints. +func TestCalculatePriceURL(t *testing.T) { + u := CalculatePriceURL() + assert.Equal(t, "https://management.azure.com/providers/Microsoft.Capacity/calculatePrice?api-version=2022-11-01", u) +} + +func TestPurchaseURL(t *testing.T) { + u := PurchaseURL("abc-def-123") + assert.Equal(t, "https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/abc-def-123/purchase?api-version=2022-11-01", u) +} diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index a13be9e86..6c1c95dbc 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -17,10 +17,10 @@ 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/redis/armredis/v3" - "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" + "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) // HTTPClient interface for HTTP operations (enables mocking) @@ -239,8 +239,9 @@ func parseTermYears(term string) (int, error) { } } -// PurchaseCommitment purchases Azure Cache for Redis reserved capacity via the Azure Reservations API. -func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, _ common.PurchaseOptions) (common.PurchaseResult, error) { +// PurchaseCommitment purchases Azure Cache for Redis reserved capacity using the two-step +// calculatePrice->purchase flow required by Azure's Reservations API (issue #677). +func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, DryRun: false, @@ -248,17 +249,23 @@ func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common. Timestamp: time.Now(), } + // Azure's reservation API mints the order ID server-side in calculatePrice, + // so the only stable dedupe signal we control is the purchase-automation tag + // derived from opts.Source. Without a non-empty Source the tag is dropped and + // a re-driven purchase cannot recognise the prior attempt, producing + // duplicate reservations. Fail fast at function entry rather than allowing + // an un-tagged, non-idempotent purchase to hit the cloud. + if opts.Source == "" { + result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + return result, result.Error + } + termYears, termErr := parseTermYears(rec.Term) if termErr != nil { result.Error = termErr return result, result.Error } - reservationOrderID := uuid.New().String() - apiVersion := "2022-11-01" - purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=%s", - reservationOrderID, apiVersion) - requestBody := map[string]interface{}{ "sku": map[string]string{ "name": rec.ResourceType, @@ -274,6 +281,9 @@ func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common. "renew": false, }, } + if opts.Source != "" { + requestBody["tags"] = map[string]string{common.PurchaseTagKey: opts.Source} + } bodyBytes, err := json.Marshal(requestBody) if err != nil { @@ -281,12 +291,6 @@ func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common. return result, result.Error } - req, err := http.NewRequestWithContext(ctx, "PUT", purchaseURL, strings.NewReader(string(bodyBytes))) - if err != nil { - result.Error = fmt.Errorf("failed to create request: %w", err) - return result, result.Error - } - token, err := c.cred.GetToken(ctx, policy.TokenRequestOptions{ Scopes: []string{"https://management.azure.com/.default"}, }) @@ -295,27 +299,15 @@ func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common. return result, result.Error } - req.Header.Set("Authorization", "Bearer "+token.Token) - req.Header.Set("Content-Type", "application/json") - - resp, err := c.httpClient.Do(req) + reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) if err != nil { - result.Error = fmt.Errorf("failed to purchase reservation: %w", err) - return result, result.Error - } - defer resp.Body.Close() - - body, _ := io.ReadAll(resp.Body) - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { - result.Error = fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) + result.Error = err return result, result.Error } result.Success = true result.CommitmentID = reservationOrderID result.Cost = rec.CommitmentCost - return result, nil } diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index 8384b479b..8c187235b 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -3,6 +3,7 @@ package managedredis import ( "bytes" "context" + "encoding/json" "errors" "io" "net/http" @@ -503,43 +504,64 @@ func TestGetOfferingDetails_Paginated(t *testing.T) { // -- PurchaseCommitment -- +// calcPriceRespJSON returns a minimal calculatePrice response JSON for tests. +func calcPriceRespJSON(orderID string) string { + return `{"properties":{"reservationOrderId":"` + orderID + `"}}` +} + func TestPurchaseCommitment_Success(t *testing.T) { h := &mockHTTPClient{} t.Cleanup(func() { h.AssertExpectations(t) }) - h.On("Do", mock.Anything).Return(fakeHTTPResp(http.StatusOK, `{"id":"res-123"}`), nil) + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(fakeHTTPResp(http.StatusOK, calcPriceRespJSON("mr-order-001")), nil).Once() + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/mr-order-001/purchase" + })).Return(fakeHTTPResp(http.StatusOK, `{}`), nil).Once() cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", Count: 1, CommitmentCost: 500.0, - }, common.PurchaseOptions{}) + }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) - assert.NotEmpty(t, result.CommitmentID) + assert.Equal(t, "mr-order-001", result.CommitmentID) assert.Equal(t, 500.0, result.Cost) } func TestPurchaseCommitment_3yr(t *testing.T) { h := &mockHTTPClient{} t.Cleanup(func() { h.AssertExpectations(t) }) - h.On("Do", mock.Anything).Return(fakeHTTPResp(http.StatusCreated, `{"id":"res-456"}`), nil) + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(fakeHTTPResp(http.StatusOK, calcPriceRespJSON("mr-order-3yr")), nil).Once() + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/mr-order-3yr/purchase" + })).Return(fakeHTTPResp(http.StatusCreated, `{}`), nil).Once() cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ ResourceType: "Premium_P2", Term: "3yr", Count: 2, CommitmentCost: 1200.0, - }, common.PurchaseOptions{}) + }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + assert.Equal(t, "mr-order-3yr", result.CommitmentID) } func TestPurchaseCommitment_Accepted(t *testing.T) { h := &mockHTTPClient{} t.Cleanup(func() { h.AssertExpectations(t) }) - h.On("Do", mock.Anything).Return(fakeHTTPResp(http.StatusAccepted, `{"id":"res-789"}`), nil) + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(fakeHTTPResp(http.StatusOK, calcPriceRespJSON("mr-order-202")), nil).Once() + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/mr-order-202/purchase" + })).Return(fakeHTTPResp(http.StatusAccepted, `{}`), nil).Once() cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", - }, common.PurchaseOptions{}) + }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) } @@ -551,7 +573,7 @@ func TestPurchaseCommitment_TokenError(t *testing.T) { c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", - }, common.PurchaseOptions{}) + }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "failed to get access token") @@ -560,26 +582,33 @@ func TestPurchaseCommitment_TokenError(t *testing.T) { func TestPurchaseCommitment_HTTPError(t *testing.T) { h := &mockHTTPClient{} t.Cleanup(func() { h.AssertExpectations(t) }) - h.On("Do", mock.Anything).Return(nil, errors.New("network error")) + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(nil, errors.New("network error")).Once() cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", - }, common.PurchaseOptions{}) + }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) - assert.Contains(t, err.Error(), "failed to purchase reservation") + assert.Contains(t, err.Error(), "calculatePrice HTTP call") } func TestPurchaseCommitment_BadStatus(t *testing.T) { h := &mockHTTPClient{} t.Cleanup(func() { h.AssertExpectations(t) }) - h.On("Do", mock.Anything).Return(fakeHTTPResp(http.StatusBadRequest, `{"error":"bad"}`), nil) + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(fakeHTTPResp(http.StatusOK, calcPriceRespJSON("mr-order-bad")), nil).Once() + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/mr-order-bad/purchase" + })).Return(fakeHTTPResp(http.StatusBadRequest, `{"error":"bad"}`), nil).Once() cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", - }, common.PurchaseOptions{}) + }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "reservation purchase failed with status 400") @@ -591,12 +620,32 @@ func TestPurchaseCommitment_InvalidTerm(t *testing.T) { c := NewClientWithHTTP(nil, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ ResourceType: "Premium_P1", Term: "5yr", Count: 1, - }, common.PurchaseOptions{}) + }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "unsupported reservation term") } +// TestPurchaseCommitment_RequiresSource pins the dedupe guard: +// PurchaseCommitment must reject an empty opts.Source before issuing any HTTP +// call. Azure mints the reservation order ID server-side, so the +// purchase-automation tag derived from Source is the only stable dedupe +// signal CUDly controls -- proceeding without it would allow a re-driven +// purchase to create a duplicate reservation. +func TestPurchaseCommitment_RequiresSource(t *testing.T) { + h := &mockHTTPClient{} + t.Cleanup(func() { h.AssertExpectations(t) }) + cred := &mockTokenCredential{token: "tok"} + c := NewClientWithHTTP(cred, "sub", "eastus", h) + result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ + ResourceType: "Premium_P1", Term: "1yr", Count: 1, + }, common.PurchaseOptions{}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "purchase source is required") + h.AssertNotCalled(t, "Do", mock.Anything) +} + func TestGetOfferingDetails_InvalidTerm(t *testing.T) { h := &mockHTTPClient{} t.Cleanup(func() { h.AssertExpectations(t) }) @@ -691,3 +740,43 @@ func TestRedisPricingStruct(t *testing.T) { assert.Equal(t, "USD", p.Currency) assert.Equal(t, 50.0, p.SavingsPercentage) } + +// TestPurchaseCommitment_TagInjection verifies that the purchase-automation tag +// is present in the purchase request body when opts.Source is set. The +// empty-Source path is covered separately by TestPurchaseCommitment_RequiresSource +// because the dedupe-guard now fails fast at function entry. +func TestPurchaseCommitment_TagInjection(t *testing.T) { + const orderID = "mr-tag-test-order" + const source = "cudly-web" + + h := &mockHTTPClient{} + t.Cleanup(func() { h.AssertExpectations(t) }) + cred := &mockTokenCredential{token: "tok"} + c := NewClientWithHTTP(cred, "sub", "eastus", h) + + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(fakeHTTPResp(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + + var capturedBody []byte + h.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.URL.Path != "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" { + return false + } + capturedBody, _ = io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(capturedBody)) + return true + })).Return(fakeHTTPResp(http.StatusOK, `{}`), nil).Once() + + result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ + ResourceType: "Premium_P1", Term: "1yr", Count: 1, CommitmentCost: 500.0, + }, common.PurchaseOptions{Source: source}) + require.NoError(t, err) + assert.True(t, result.Success) + + var body map[string]interface{} + require.NoError(t, json.Unmarshal(capturedBody, &body)) + tags, hasTags := body["tags"].(map[string]interface{}) + require.True(t, hasTags, "tags field must be present in purchase body when Source is set") + assert.Equal(t, source, tags[common.PurchaseTagKey], "tag value must match opts.Source") +} diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 181a2150c..25d0dd414 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -16,10 +16,10 @@ 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" + "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) // HTTPClient interface for HTTP operations (enables mocking) @@ -236,7 +236,8 @@ func (c *SearchClient) convertSearchReservation(detail *armconsumption.Reservati return commitment } -// PurchaseCommitment purchases Search reserved capacity via Azure Reservations API +// PurchaseCommitment purchases Search reserved capacity using the two-step +// calculatePrice->purchase flow required by Azure's Reservations API (issue #677). func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, @@ -245,14 +246,16 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm Timestamp: time.Now(), } - // 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) + // Azure's reservation API mints the order ID server-side in calculatePrice, + // so the only stable dedupe signal we control is the purchase-automation tag + // derived from opts.Source. Without a non-empty Source the tag is dropped and + // a re-driven purchase cannot recognise the prior attempt, producing + // duplicate reservations. Fail fast at function entry rather than allowing + // an un-tagged, non-idempotent purchase to hit the cloud. + if opts.Source == "" { + result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + return result, result.Error + } termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { @@ -282,12 +285,6 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm return result, result.Error } - req, err := http.NewRequestWithContext(ctx, "PUT", purchaseURL, strings.NewReader(string(bodyBytes))) - if err != nil { - result.Error = fmt.Errorf("failed to create request: %w", err) - return result, result.Error - } - token, err := c.cred.GetToken(ctx, policy.TokenRequestOptions{ Scopes: []string{"https://management.azure.com/.default"}, }) @@ -296,27 +293,15 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm return result, result.Error } - req.Header.Set("Authorization", "Bearer "+token.Token) - req.Header.Set("Content-Type", "application/json") - - resp, err := c.httpClient.Do(req) + reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) if err != nil { - result.Error = fmt.Errorf("failed to purchase reservation: %w", err) - return result, result.Error - } - defer resp.Body.Close() - - body, _ := io.ReadAll(resp.Body) - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { - result.Error = fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) + result.Error = err return result, result.Error } result.Success = true result.CommitmentID = reservationOrderID result.Cost = rec.CommitmentCost - return result, nil } diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index 5a9c00f63..c15fc9d0c 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -3,6 +3,7 @@ package search import ( "bytes" "context" + "encoding/json" "errors" "io" "net/http" @@ -617,16 +618,23 @@ func (m *MockTokenCredential) GetToken(ctx context.Context, options policy.Token }, nil } +// calcPriceRespJSON returns a minimal calculatePrice response JSON for tests. +func calcPriceRespJSON(orderID string) string { + return `{"properties":{"reservationOrderId":"` + orderID + `"}}` +} + func TestSearchClient_PurchaseCommitment_Success(t *testing.T) { ctx := context.Background() 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, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("search-order-001")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/search-order-001/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "standard", @@ -635,11 +643,12 @@ func TestSearchClient_PurchaseCommitment_Success(t *testing.T) { CommitmentCost: 3000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) - assert.NotEmpty(t, result.CommitmentID) + assert.Equal(t, "search-order-001", result.CommitmentID) assert.Equal(t, 3000.0, result.Cost) + mockHTTP.AssertExpectations(t) } func TestSearchClient_PurchaseCommitment_3YearTerm(t *testing.T) { @@ -648,10 +657,12 @@ func TestSearchClient_PurchaseCommitment_3YearTerm(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusCreated, `{"id": "reservation-123"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("search-order-3yr")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/search-order-3yr/purchase" + })).Return(createMockHTTPResponse(http.StatusCreated, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "standard", @@ -660,9 +671,11 @@ func TestSearchClient_PurchaseCommitment_3YearTerm(t *testing.T) { CommitmentCost: 7500.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + assert.Equal(t, "search-order-3yr", result.CommitmentID) + mockHTTP.AssertExpectations(t) } func TestSearchClient_PurchaseCommitment_Accepted(t *testing.T) { @@ -671,10 +684,12 @@ func TestSearchClient_PurchaseCommitment_Accepted(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusAccepted, `{"id": "reservation-123"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("search-order-202")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/search-order-202/purchase" + })).Return(createMockHTTPResponse(http.StatusAccepted, `{}`), nil).Once() rec := common.Recommendation{ ResourceType: "standard", @@ -683,9 +698,10 @@ func TestSearchClient_PurchaseCommitment_Accepted(t *testing.T) { CommitmentCost: 3000.0, } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestSearchClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -699,7 +715,7 @@ func TestSearchClient_PurchaseCommitment_TokenError(t *testing.T) { Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "failed to get access token") @@ -711,17 +727,19 @@ func TestSearchClient_PurchaseCommitment_HTTPError(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return(nil, errors.New("network error")) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(nil, errors.New("network error")).Once() rec := common.Recommendation{ ResourceType: "standard", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) - assert.Contains(t, err.Error(), "failed to purchase reservation") + assert.Contains(t, err.Error(), "calculatePrice HTTP call") } func TestSearchClient_PurchaseCommitment_BadStatus(t *testing.T) { @@ -730,20 +748,23 @@ func TestSearchClient_PurchaseCommitment_BadStatus(t *testing.T) { mockCred := &MockTokenCredential{token: "test-token"} client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - mockHTTP.On("Do", mock.Anything).Return( - createMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`), - nil, - ) + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON("search-order-bad")), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/search-order-bad/purchase" + })).Return(createMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`), nil).Once() rec := common.Recommendation{ ResourceType: "standard", Term: "1yr", } - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "reservation purchase failed with status 400") + mockHTTP.AssertExpectations(t) } func TestSearchClient_ValidateOffering_Valid(t *testing.T) { @@ -805,59 +826,97 @@ func TestSearchClient_ValidateOffering_Invalid(t *testing.T) { 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) { +// TestSearchClient_PurchaseCommitment_TwoStepFlow verifies the two-step +// calculatePrice->purchase flow for the search client (issue #677 regression test). +func TestSearchClient_PurchaseCommitment_TwoStepFlow(t *testing.T) { ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + const azureMintedOrderID = "azure-search-order-677" + + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON(azureMintedOrderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && + r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+azureMintedOrderID+"/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + 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) + + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + assert.True(t, result.Success) + assert.Equal(t, azureMintedOrderID, result.CommitmentID, + "CommitmentID must be the Azure-minted order ID from calculatePrice") + mockHTTP.AssertExpectations(t) + mockHTTP.AssertNumberOfCalls(t, "Do", 2) +} + +// TestSearchClient_PurchaseCommitment_TagInjection verifies that the +// purchase-automation tag is present in the calculatePrice request body when +// opts.Source is set. The empty-Source path is covered separately by +// TestSearchClient_PurchaseCommitment_RequiresSource because the dedupe-guard +// now fails fast at function entry. +func TestSearchClient_PurchaseCommitment_TagInjection(t *testing.T) { + const orderID = "azure-search-tag-test" + const source = "cudly-web" + + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + var capturedBody []byte + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.Method != http.MethodPost || r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + capturedBody, _ = io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(capturedBody)) + return true + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && + r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ResourceType: "standard", Term: "1yr", Count: 1, CommitmentCost: 3000.0} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: source}) + require.NoError(t, err) + assert.True(t, result.Success) + + var body map[string]interface{} + require.NoError(t, json.Unmarshal(capturedBody, &body)) + tags, hasTags := body["tags"].(map[string]interface{}) + require.True(t, hasTags, "tags field must be present in calculatePrice body when Source is set") + assert.Equal(t, source, tags[common.PurchaseTagKey], "tag value must match opts.Source") + mockHTTP.AssertExpectations(t) +} + +// TestSearchClient_PurchaseCommitment_RequiresSource pins the dedupe guard: +// PurchaseCommitment must reject an empty opts.Source before issuing any HTTP +// call. Azure mints the reservation order ID server-side, so the +// purchase-automation tag derived from Source is the only stable dedupe +// signal CUDly controls -- proceeding without it would allow a re-driven +// purchase to create a duplicate reservation. +func TestSearchClient_PurchaseCommitment_RequiresSource(t *testing.T) { + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + rec := common.Recommendation{ResourceType: "standard", Term: "1yr", Count: 1} + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "purchase source is required") + mockHTTP.AssertNotCalled(t, "Do", mock.Anything) } diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index c793590cc..555147aae 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -9,7 +9,6 @@ import ( "context" "encoding/json" "fmt" - "io" "net/http" "net/url" "strings" @@ -18,12 +17,12 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" - "github.com/google/uuid" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" + "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) // HTTPClient interface for HTTP operations (enables mocking). @@ -242,8 +241,8 @@ func parseReservationTermYears(term string) (int, error) { } // PurchaseCommitment purchases Synapse reserved capacity via the Azure -// Reservations API. The reserved resource type is "SqlDW" which covers -// Dedicated SQL Pool DWU reservations. +// Reservations API two-step flow (calculatePrice -> purchase). The reserved +// resource type is "SqlDW" which covers Dedicated SQL Pool DWU reservations. func (c *SynapseClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, @@ -252,6 +251,17 @@ func (c *SynapseClient) PurchaseCommitment(ctx context.Context, rec common.Recom Timestamp: time.Now(), } + // Azure's reservation API mints the order ID server-side in calculatePrice, + // so the only stable dedupe signal we control is the purchase-automation tag + // derived from opts.Source. Without a non-empty Source the tag is dropped and + // a re-driven purchase cannot recognise the prior attempt, producing + // duplicate reservations. Fail fast at function entry rather than allowing + // an un-tagged, non-idempotent purchase to hit the cloud. + if opts.Source == "" { + result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + return result, result.Error + } + if strings.TrimSpace(rec.ResourceType) == "" { result.Error = fmt.Errorf("resource type is required") return result, result.Error @@ -267,28 +277,6 @@ func (c *SynapseClient) PurchaseCommitment(ctx context.Context, rec common.Recom return result, result.Error } - reservationOrderID := uuid.New().String() - commitmentID, err := c.doPurchaseRequest(ctx, rec, opts, reservationOrderID, termYears) - if err != nil { - result.Error = err - return result, result.Error - } - - result.Success = true - result.CommitmentID = commitmentID - result.Cost = rec.CommitmentCost - return result, nil -} - -// doPurchaseRequest marshals the reservation request body, signs it with a -// bearer token, and executes the PUT against the Azure Reservations API. -// It is extracted from PurchaseCommitment to keep that function's cyclomatic -// complexity within the project limit. -func (c *SynapseClient) doPurchaseRequest(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions, reservationOrderID string, termYears int) (string, error) { - apiVersion := "2022-11-01" - purchaseURL := fmt.Sprintf("https://management.azure.com/providers/Microsoft.Capacity/reservationOrders/%s?api-version=%s", - reservationOrderID, apiVersion) - requestBody := map[string]interface{}{ "sku": map[string]string{ "name": rec.ResourceType, @@ -308,38 +296,28 @@ func (c *SynapseClient) doPurchaseRequest(ctx context.Context, rec common.Recomm bodyBytes, err := json.Marshal(requestBody) if err != nil { - return "", fmt.Errorf("failed to marshal request: %w", err) - } - - req, err := http.NewRequestWithContext(ctx, "PUT", purchaseURL, strings.NewReader(string(bodyBytes))) - if err != nil { - return "", fmt.Errorf("failed to create request: %w", err) + result.Error = fmt.Errorf("failed to marshal request: %w", err) + return result, result.Error } token, err := c.cred.GetToken(ctx, policy.TokenRequestOptions{ Scopes: []string{"https://management.azure.com/.default"}, }) if err != nil { - return "", fmt.Errorf("failed to get access token: %w", err) + result.Error = fmt.Errorf("failed to get access token: %w", err) + return result, result.Error } - req.Header.Set("Authorization", "Bearer "+token.Token) - req.Header.Set("Content-Type", "application/json") - - resp, err := c.httpClient.Do(req) + reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) if err != nil { - return "", fmt.Errorf("failed to purchase reservation: %w", err) + result.Error = err + return result, result.Error } - defer resp.Body.Close() - body, readErr := io.ReadAll(resp.Body) - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { - if readErr != nil { - return "", fmt.Errorf("reservation purchase failed with status %d (body read error: %v)", resp.StatusCode, readErr) - } - return "", fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) - } - return reservationOrderID, nil + result.Success = true + result.CommitmentID = reservationOrderID + result.Cost = rec.CommitmentCost + return result, nil } // ValidateOffering validates that a Synapse SKU is in the known set. diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index 4715ea82d..c17baa66b 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -489,11 +489,23 @@ func TestGetOfferingDetails_httpError(t *testing.T) { assert.Error(t, err) } +// calcPriceRespJSON returns a minimal calculatePrice JSON response with the +// given Azure-minted reservationOrderId. +func calcPriceRespJSON(orderID string) string { + return `{"properties":{"reservationOrderId":"` + orderID + `"}}` +} + // ---- PurchaseCommitment --------------------------------------------------- func TestPurchaseCommitment_success(t *testing.T) { mHTTP := &mockHTTPClient{} - mHTTP.On("Do", mock.Anything).Return(newHTTPResponse(http.StatusOK, `{"id":"res-123"}`), nil) + t.Cleanup(func() { mHTTP.AssertExpectations(t) }) + mHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(newHTTPResponse(http.StatusOK, calcPriceRespJSON("syn-order-001")), nil).Once() + mHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/syn-order-001/purchase" + })).Return(newHTTPResponse(http.StatusOK, `{}`), nil).Once() cred := &mockTokenCredential{token: "test-token"} c := NewClientWithHTTP(cred, "sub-123", "eastus", mHTTP) @@ -504,58 +516,104 @@ func TestPurchaseCommitment_success(t *testing.T) { Count: 1, CommitmentCost: 5000.0, } - result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) - assert.NotEmpty(t, result.CommitmentID) + assert.Equal(t, "syn-order-001", result.CommitmentID) assert.InDelta(t, 5000.0, result.Cost, 0.01) } func TestPurchaseCommitment_3yrTerm(t *testing.T) { mHTTP := &mockHTTPClient{} - mHTTP.On("Do", mock.Anything).Return(newHTTPResponse(http.StatusAccepted, `{}`), nil) + t.Cleanup(func() { mHTTP.AssertExpectations(t) }) + mHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(newHTTPResponse(http.StatusOK, calcPriceRespJSON("syn-order-3yr")), nil).Once() + mHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/syn-order-3yr/purchase" + })).Return(newHTTPResponse(http.StatusAccepted, `{}`), nil).Once() cred := &mockTokenCredential{token: "test-token"} c := NewClientWithHTTP(cred, "sub-123", "eastus", mHTTP) rec := common.Recommendation{ResourceType: "DW500c", Term: "3yr", Count: 2, CommitmentCost: 9000.0} - result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) } func TestPurchaseCommitment_withSource(t *testing.T) { - capHTTP := &captureHTTPClient{response: newHTTPResponse(http.StatusCreated, `{}`)} + // Capture the body sent to calculatePrice to verify the automation tag is present. + var capturedBody []byte + mHTTP := &mockHTTPClient{} + t.Cleanup(func() { mHTTP.AssertExpectations(t) }) + mHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Run(func(args mock.Arguments) { + req := args.Get(0).(*http.Request) + if req.Body != nil { + capturedBody, _ = io.ReadAll(req.Body) + req.Body = io.NopCloser(bytes.NewReader(capturedBody)) + } + }).Return(newHTTPResponse(http.StatusOK, calcPriceRespJSON("syn-order-src")), nil).Once() + mHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/syn-order-src/purchase" + })).Return(newHTTPResponse(http.StatusCreated, `{}`), nil).Once() + cred := &mockTokenCredential{token: "test-token"} - c := NewClientWithHTTP(cred, "sub-123", "eastus", capHTTP) + c := NewClientWithHTTP(cred, "sub-123", "eastus", mHTTP) rec := common.Recommendation{ResourceType: "DW500c", Term: "1yr", Count: 1} _, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: "automation"}) require.NoError(t, err) - assert.Contains(t, string(capHTTP.captured), "purchase-automation") - assert.Contains(t, string(capHTTP.captured), "automation") + assert.Contains(t, string(capturedBody), "purchase-automation") + assert.Contains(t, string(capturedBody), "automation") } func TestPurchaseCommitment_apiError(t *testing.T) { + // calculatePrice succeeds; purchase fails with a non-timeout 400. mHTTP := &mockHTTPClient{} - mHTTP.On("Do", mock.Anything).Return( - newHTTPResponse(http.StatusBadRequest, `{"error":"bad request"}`), nil) + t.Cleanup(func() { mHTTP.AssertExpectations(t) }) + mHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(newHTTPResponse(http.StatusOK, calcPriceRespJSON("syn-order-err")), nil).Once() + mHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/syn-order-err/purchase" + })).Return(newHTTPResponse(http.StatusBadRequest, `{"error":"bad request"}`), nil).Once() cred := &mockTokenCredential{token: "test-token"} c := NewClientWithHTTP(cred, "sub-123", "eastus", mHTTP) rec := common.Recommendation{ResourceType: "DW1000c", Term: "1yr", Count: 1} - result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) } +func TestPurchaseCommitment_httpError(t *testing.T) { + // calculatePrice network failure. + mHTTP := &mockHTTPClient{} + t.Cleanup(func() { mHTTP.AssertExpectations(t) }) + mHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" + })).Return(nil, errors.New("network error")).Once() + + cred := &mockTokenCredential{token: "test-token"} + c := NewClientWithHTTP(cred, "sub-123", "eastus", mHTTP) + + rec := common.Recommendation{ResourceType: "DW1000c", Term: "1yr", Count: 1} + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "calculatePrice HTTP call") +} + func TestPurchaseCommitment_tokenError(t *testing.T) { cred := &mockTokenCredential{err: errors.New("token error")} c := NewClientWithHTTP(cred, "sub-123", "eastus", nil) rec := common.Recommendation{ResourceType: "DW1000c", Term: "1yr", Count: 1} - result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) } @@ -704,7 +762,7 @@ func TestPurchaseCommitment_emptyResourceType(t *testing.T) { cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub-123", "eastus", nil) rec := common.Recommendation{ResourceType: "", Term: "1yr", Count: 1} - result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "resource type is required") @@ -714,7 +772,7 @@ func TestPurchaseCommitment_zeroCount(t *testing.T) { cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub-123", "eastus", nil) rec := common.Recommendation{ResourceType: "DW1000c", Term: "1yr", Count: 0} - result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "quantity must be greater than zero") @@ -724,7 +782,7 @@ func TestPurchaseCommitment_negativeCount(t *testing.T) { cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub-123", "eastus", nil) rec := common.Recommendation{ResourceType: "DW1000c", Term: "1yr", Count: -1} - result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) } @@ -733,12 +791,31 @@ func TestPurchaseCommitment_unsupportedTerm(t *testing.T) { cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub-123", "eastus", nil) rec := common.Recommendation{ResourceType: "DW1000c", Term: "5yr", Count: 1} - result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "unsupported reservation term") } +// TestPurchaseCommitment_requiresSource pins the dedupe guard: +// PurchaseCommitment must reject an empty opts.Source before issuing any HTTP +// call. Azure mints the reservation order ID server-side, so the +// purchase-automation tag derived from Source is the only stable dedupe +// signal CUDly controls -- proceeding without it would allow a re-driven +// purchase to create a duplicate reservation. +func TestPurchaseCommitment_requiresSource(t *testing.T) { + mHTTP := &mockHTTPClient{} + t.Cleanup(func() { mHTTP.AssertExpectations(t) }) + cred := &mockTokenCredential{token: "tok"} + c := NewClientWithHTTP(cred, "sub-123", "eastus", mHTTP) + rec := common.Recommendation{ResourceType: "DW1000c", Term: "1yr", Count: 1} + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "purchase source is required") + mHTTP.AssertNotCalled(t, "Do", mock.Anything) +} + // ---- GetOfferingDetails: no reservation price ---------------------------- func TestGetOfferingDetails_noReservationPrice(t *testing.T) {