From a3440c8394e5c94150b9e7c5305a74ec8a270ef9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 17:03:58 +0200 Subject: [PATCH 1/6] feat(azure/reservations): add shared two-step calculatePrice->purchase helper Introduces providers/azure/services/internal/reservations with DoPurchaseTwoStep, IsSessionTimeout, CalculatePriceURL, and PurchaseURL. The helper centralises the two-call flow (calculatePrice mints a session-bound reservationOrderId; purchase commits it) and retries up to 3 times on Session timed out 400s by re-running calculatePrice from scratch. Refs #677. --- .../internal/reservations/purchase.go | 184 +++++++++++++++ .../internal/reservations/purchase_test.go | 215 ++++++++++++++++++ 2 files changed, 399 insertions(+) create mode 100644 providers/azure/services/internal/reservations/purchase.go create mode 100644 providers/azure/services/internal/reservations/purchase_test.go diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go new file mode 100644 index 000000000..45dfe1f6e --- /dev/null +++ b/providers/azure/services/internal/reservations/purchase.go @@ -0,0 +1,184 @@ +// 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 ( + "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) + time.Sleep(purchaseRetryDelay) + 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, strings.NewReader(string(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, strings.NewReader(string(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) +} From 02930ecdf772edf089a997dba3af3711045ca2d4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 17:04:37 +0200 Subject: [PATCH 2/6] fix(azure): switch 7 reservation clients to calculatePrice->purchase flow Fixes the 400 "Session timed out" errors on newer Azure SKU families (e.g., Standard_B2ats_v2, Burstable v2) by replacing the broken single-step PUT reservationOrders/{client-id} pattern with the required two-step flow: 1. POST calculatePrice -- mints an Azure session-bound reservationOrderId 2. POST reservationOrders/{azure-id}/purchase -- commits the order All 7 affected clients now delegate to the shared reservations helper: compute, database, cache, search, cosmosdb, managedredis, synapse. Idempotency (Option B): Azure mints the order ID at calculatePrice time so client-supplied IDs are no longer feasible. Caller-level deduplication uses the purchase-automation tag already attached to each reservation request. The `uuid` dependency is removed from all 7 clients. Tests updated to the two-step mock pattern (calculatePrice + purchase HTTP expectations). Total: 530 tests pass, go vet clean. Closes #677 Refs #641, #667, #632, #669, #671 --- providers/azure/services/cache/client.go | 36 +-- providers/azure/services/cache/client_test.go | 58 +++-- providers/azure/services/compute/client.go | 76 ++---- .../azure/services/compute/client_test.go | 217 ++++++++++++------ providers/azure/services/cosmosdb/client.go | 40 +--- .../azure/services/cosmosdb/client_test.go | 58 +++-- providers/azure/services/database/client.go | 40 +--- .../azure/services/database/client_test.go | 58 +++-- .../azure/services/managedredis/client.go | 37 +-- .../services/managedredis/client_test.go | 42 +++- providers/azure/services/search/client.go | 36 +-- .../azure/services/search/client_test.go | 131 ++++++----- providers/azure/services/synapse/client.go | 61 ++--- .../azure/services/synapse/client_test.go | 76 +++++- 14 files changed, 487 insertions(+), 479 deletions(-) diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index b08c7a8b8..46f6bf9bc 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,14 +273,6 @@ 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) - termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { termYears = 3 @@ -309,12 +301,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 +309,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..7ce1a854c 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -916,16 +916,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", @@ -937,8 +944,9 @@ func TestCacheClient_PurchaseCommitment_Success(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +955,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", @@ -962,6 +972,8 @@ func TestCacheClient_PurchaseCommitment_3YearTerm(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +982,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", @@ -985,6 +999,7 @@ func TestCacheClient_PurchaseCommitment_Accepted(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestCacheClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -1010,7 +1025,9 @@ 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", @@ -1020,7 +1037,7 @@ func TestCacheClient_PurchaseCommitment_HTTPError(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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,10 +1046,12 @@ 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", @@ -1043,4 +1062,5 @@ func TestCacheClient_PurchaseCommitment_BadStatus(t *testing.T) { require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "reservation purchase failed with status 400") + mockHTTP.AssertExpectations(t) } diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index a336bcc4b..0fa8615ed 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, @@ -457,16 +427,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..c936e546b 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -6,6 +6,7 @@ import ( "errors" "net/http" "strconv" + "strings" "testing" "time" @@ -437,16 +438,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", @@ -458,8 +488,9 @@ func TestComputeClient_PurchaseCommitment_Success(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +499,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", @@ -483,6 +517,8 @@ func TestComputeClient_PurchaseCommitment_3YearTerm(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +527,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", @@ -506,6 +545,7 @@ func TestComputeClient_PurchaseCommitment_Accepted(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestComputeClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -531,7 +571,11 @@ 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", @@ -541,7 +585,7 @@ func TestComputeClient_PurchaseCommitment_HTTPError(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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,10 +594,17 @@ 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", @@ -564,70 +615,86 @@ func TestComputeClient_PurchaseCommitment_BadStatus(t *testing.T) { 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{}) + 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{}) + require.NoError(t, err) + assert.True(t, result.Success) + assert.Equal(t, "order-second", result.CommitmentID) + mockHTTP.AssertExpectations(t) } // TestComputeClient_ConvertAzureVMRecommendation_NilGuards pins the new diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index af1db742c..a0de864dd 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,17 +266,6 @@ 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) - termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { termYears = 3 @@ -305,13 +294,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 +302,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..fdffe30e5 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -714,16 +714,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", @@ -735,8 +742,9 @@ func TestCosmosDBClient_PurchaseCommitment_Success(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +753,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", @@ -760,6 +770,8 @@ func TestCosmosDBClient_PurchaseCommitment_3YearTerm(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +780,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", @@ -783,6 +797,7 @@ func TestCosmosDBClient_PurchaseCommitment_Accepted(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestCosmosDBClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -808,7 +823,9 @@ 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", @@ -818,7 +835,7 @@ func TestCosmosDBClient_PurchaseCommitment_HTTPError(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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,10 +844,12 @@ 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", @@ -841,6 +860,7 @@ func TestCosmosDBClient_PurchaseCommitment_BadStatus(t *testing.T) { require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "reservation purchase failed with status 400") + mockHTTP.AssertExpectations(t) } // TestCosmosDBClient_ConvertAzureCosmosRecommendation_NilGuards pins the diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index 758f29901..5765a7050 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,17 +274,6 @@ 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) - termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { termYears = 3 @@ -313,13 +302,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 +310,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..1d5af5082 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -825,16 +825,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", @@ -846,8 +853,9 @@ func TestDatabaseClient_PurchaseCommitment_Success(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +864,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", @@ -871,6 +881,8 @@ func TestDatabaseClient_PurchaseCommitment_3YearTerm(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +891,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", @@ -894,6 +908,7 @@ func TestDatabaseClient_PurchaseCommitment_Accepted(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestDatabaseClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -919,7 +934,9 @@ 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", @@ -929,7 +946,7 @@ func TestDatabaseClient_PurchaseCommitment_HTTPError(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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,10 +955,12 @@ 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", @@ -952,6 +971,7 @@ func TestDatabaseClient_PurchaseCommitment_BadStatus(t *testing.T) { require.Error(t, err) assert.False(t, result.Success) assert.Contains(t, err.Error(), "reservation purchase failed with status 400") + mockHTTP.AssertExpectations(t) } func TestDatabaseClient_ValidateOffering_Valid(t *testing.T) { diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index a13be9e86..f00ea5199 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, @@ -254,11 +255,6 @@ func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common. 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 +270,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 +280,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 +288,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..48118b879 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -503,10 +503,20 @@ 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{ @@ -514,14 +524,19 @@ func TestPurchaseCommitment_Success(t *testing.T) { }, common.PurchaseOptions{}) 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{ @@ -529,12 +544,18 @@ func TestPurchaseCommitment_3yr(t *testing.T) { }, common.PurchaseOptions{}) 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{ @@ -560,7 +581,9 @@ 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{ @@ -568,13 +591,18 @@ func TestPurchaseCommitment_HTTPError(t *testing.T) { }, common.PurchaseOptions{}) 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{ diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 181a2150c..bfe41a1dd 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,15 +246,6 @@ 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) - termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { termYears = 3 @@ -282,12 +274,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 +282,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..7052add05 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -617,16 +617,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", @@ -638,8 +645,9 @@ func TestSearchClient_PurchaseCommitment_Success(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +656,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", @@ -663,6 +673,8 @@ func TestSearchClient_PurchaseCommitment_3YearTerm(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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 +683,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", @@ -686,6 +700,7 @@ func TestSearchClient_PurchaseCommitment_Accepted(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) require.NoError(t, err) assert.True(t, result.Success) + mockHTTP.AssertExpectations(t) } func TestSearchClient_PurchaseCommitment_TokenError(t *testing.T) { @@ -711,7 +726,9 @@ 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", @@ -721,7 +738,7 @@ func TestSearchClient_PurchaseCommitment_HTTPError(t *testing.T) { result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) 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,10 +747,12 @@ 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", @@ -744,6 +763,7 @@ func TestSearchClient_PurchaseCommitment_BadStatus(t *testing.T) { 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 +825,36 @@ 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{}) + 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) } diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index c793590cc..94dc3d508 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, @@ -267,28 +266,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 +285,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..a81ca451d 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) @@ -507,13 +519,19 @@ func TestPurchaseCommitment_success(t *testing.T) { result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) 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) @@ -525,21 +543,60 @@ func TestPurchaseCommitment_3yrTerm(t *testing.T) { } 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{} + 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{}) + require.Error(t, err) + assert.False(t, result.Success) +} + +func TestPurchaseCommitment_httpError(t *testing.T) { + // calculatePrice network failure. 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(nil, errors.New("network error")).Once() cred := &mockTokenCredential{token: "test-token"} c := NewClientWithHTTP(cred, "sub-123", "eastus", mHTTP) @@ -548,6 +605,7 @@ func TestPurchaseCommitment_apiError(t *testing.T) { result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) require.Error(t, err) assert.False(t, result.Success) + assert.Contains(t, err.Error(), "calculatePrice HTTP call") } func TestPurchaseCommitment_tokenError(t *testing.T) { From 86b49eff47c2a418ad01c02257b0a3c10db0fb69 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 17:19:53 +0200 Subject: [PATCH 3/6] fix(azure/reservations): address CodeRabbit review on PR #680 - Honor ctx cancellation during retry backoff in DoPurchaseTwoStep: replace time.Sleep with a select on ctx.Done() so a deadline or cancel propagates immediately instead of blocking for purchaseRetryDelay. - Add tag-injection regression tests to search and managedredis suites: assert purchase-automation tag is present in the request body when opts.Source is set and absent when it is empty. --- .../internal/reservations/purchase.go | 6 +- .../services/managedredis/client_test.go | 56 +++++++++++++++++++ .../azure/services/search/client_test.go | 56 +++++++++++++++++++ 3 files changed, 117 insertions(+), 1 deletion(-) diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index 45dfe1f6e..e56ae351f 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -119,7 +119,11 @@ func DoPurchaseTwoStep(ctx context.Context, httpClient HTTPClient, calcURL strin if IsSessionTimeout(purchaseErr) && attempt < purchaseMaxAttempts { log.Printf("reservation purchase session timed out (attempt %d/%d), re-running calculatePrice in %s", attempt, purchaseMaxAttempts, purchaseRetryDelay) - time.Sleep(purchaseRetryDelay) + select { + case <-time.After(purchaseRetryDelay): + case <-ctx.Done(): + return "", fmt.Errorf("reservation purchase canceled during retry delay: %w", ctx.Err()) + } continue } return "", purchaseErr diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index 48118b879..af580fdc4 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" @@ -719,3 +720,58 @@ 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, and absent +// when opts.Source is empty. +func TestPurchaseCommitment_TagInjection(t *testing.T) { + const orderID = "mr-tag-test-order" + const source = "cudly-web" + + for _, tc := range []struct { + name string + source string + expectTag bool + }{ + {"tag_present_when_source_set", source, true}, + {"tag_absent_when_source_empty", "", false}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + 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: tc.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{}) + if tc.expectTag { + require.True(t, hasTags, "tags field must be present in purchase body when Source is set") + assert.Equal(t, tc.source, tags[common.PurchaseTagKey], "tag value must match opts.Source") + } else { + assert.False(t, hasTags, "tags field must be absent in purchase body when Source is empty") + } + }) + } +} diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index 7052add05..628868e2d 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" @@ -858,3 +859,58 @@ func TestSearchClient_PurchaseCommitment_TwoStepFlow(t *testing.T) { 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, and absent when opts.Source is empty. +func TestSearchClient_PurchaseCommitment_TagInjection(t *testing.T) { + const orderID = "azure-search-tag-test" + const source = "cudly-web" + + for _, tc := range []struct { + name string + source string + expectTag bool + }{ + {"tag_present_when_source_set", source, true}, + {"tag_absent_when_source_empty", "", false}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + 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: tc.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{}) + if tc.expectTag { + require.True(t, hasTags, "tags field must be present in calculatePrice body when Source is set") + assert.Equal(t, tc.source, tags[common.PurchaseTagKey], "tag value must match opts.Source") + } else { + assert.False(t, hasTags, "tags field must be absent in calculatePrice body when Source is empty") + } + mockHTTP.AssertExpectations(t) + }) + } +} From b170ad1ad12350111eb5eb827330e21e932fbad9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 17:31:42 +0200 Subject: [PATCH 4/6] refactor(azure/reservations): use bytes.NewReader over strings.NewReader(string(bs)) Address remaining CR nitpick from PR #680 pass 2. Wrapping a []byte in an io.Reader via strings.NewReader(string(bodyBytes)) forces a copy of the underlying bytes; bytes.NewReader takes the slice directly. Two callsites fixed (doCalculatePrice + doPurchase). Tests + go build + go vet clean. --- providers/azure/services/internal/reservations/purchase.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index e56ae351f..ad23cc6dc 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -26,6 +26,7 @@ package reservations import ( + "bytes" "context" "encoding/json" "fmt" @@ -134,7 +135,7 @@ func DoPurchaseTwoStep(ctx context.Context, httpClient HTTPClient, calcURL strin // 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, strings.NewReader(string(bodyBytes))) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, calcURL, bytes.NewReader(bodyBytes)) if err != nil { return "", fmt.Errorf("build calculatePrice request: %w", err) } @@ -167,7 +168,7 @@ func doCalculatePrice(ctx context.Context, httpClient HTTPClient, calcURL string // 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, strings.NewReader(string(bodyBytes))) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, purchaseURL, bytes.NewReader(bodyBytes)) if err != nil { return fmt.Errorf("build purchase request: %w", err) } From 54314db0070a44e1874ad34402ca88faabcfe8f6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 18:00:40 +0200 Subject: [PATCH 5/6] fix(azure): require non-empty opts.Source before relying on tag-based dedupe CodeRabbit pass-3 round-1 A1 (outside-diff Major): Azure's reservation API mints the order ID server-side in calculatePrice, so client-supplied GUIDs cannot deduplicate re-driven purchases. The only stable dedupe signal CUDly controls is the purchase-automation tag derived from opts.Source via applyPurchaseAutomationTag (or the inline equivalent in compute / managedredis). When opts.Source is empty the tag is dropped silently and a re-driven purchase cannot recognise the prior attempt, producing duplicate reservations. Add a guard at the entry of PurchaseCommitment in every Azure service that uses the two-step calculatePrice -> purchase flow with tag-based dedupe so the request fails fast before any cloud API call: if opts.Source == "" { result.Error = fmt.Errorf("purchase source is required for ...") return result, result.Error } Services covered (7): cache, database, search, cosmosdb, synapse, compute, managedredis. Savingsplans is intentionally excluded -- it uses the Azure OrderAlias SDK with native DisplayName-based dedup and takes opts.PurchaseOptions as `_`, so the same invariant does not apply. Per-service regression tests pin the guard: every PurchaseCommitment *_RequiresSource test asserts the call returns an error containing "purchase source is required" and that no HTTP call is issued. Existing tests that previously passed common.PurchaseOptions{} are updated to pass Source: common.PurchaseSourceCLI so they continue to exercise the intended code paths. Production safety: the call sites in internal/purchase/execution.go already populate opts.Source from m.normalizePurchaseSource(exec); the guard only blocks the silent untagged fallback (when the upstream NormalizeSource rejects a malformed value), which is exactly the case the CR finding flags as a duplicate-purchase risk. go build ./... + go vet ./... + go test ./providers/azure/... + tests for callers (internal/purchase, cmd) all pass. --- providers/azure/services/cache/client.go | 11 ++ providers/azure/services/cache/client_test.go | 33 ++++- providers/azure/services/compute/client.go | 11 ++ .../azure/services/compute/client_test.go | 37 ++++-- providers/azure/services/cosmosdb/client.go | 11 ++ .../azure/services/cosmosdb/client_test.go | 32 ++++- providers/azure/services/database/client.go | 11 ++ .../azure/services/database/client_test.go | 32 ++++- .../azure/services/managedredis/client.go | 11 ++ .../services/managedredis/client_test.go | 115 +++++++++--------- providers/azure/services/search/client.go | 11 ++ .../azure/services/search/client_test.go | 114 +++++++++-------- providers/azure/services/synapse/client.go | 11 ++ .../azure/services/synapse/client_test.go | 37 ++++-- 14 files changed, 333 insertions(+), 144 deletions(-) diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 46f6bf9bc..b8f44b121 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -273,6 +273,17 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme 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 := 1 if rec.Term == "3yr" || rec.Term == "3" { termYears = 3 diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 7ce1a854c..ee8d97d9a 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -941,7 +941,7 @@ 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.Equal(t, "cache-order-001", result.CommitmentID) @@ -969,7 +969,7 @@ 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) @@ -996,7 +996,7 @@ 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) @@ -1013,7 +1013,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") @@ -1034,7 +1034,7 @@ func TestCacheClient_PurchaseCommitment_HTTPError(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(), "calculatePrice HTTP call") @@ -1058,9 +1058,30 @@ func TestCacheClient_PurchaseCommitment_BadStatus(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(), "reservation purchase failed with status 400") 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 0fa8615ed..95ffc5bfb 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -410,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) diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index c936e546b..6c95517d9 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -485,7 +485,7 @@ 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.Equal(t, "order-vm-001", result.CommitmentID) @@ -514,7 +514,7 @@ 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) @@ -542,7 +542,7 @@ 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) @@ -559,7 +559,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") @@ -582,7 +582,7 @@ func TestComputeClient_PurchaseCommitment_HTTPError(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(), "calculatePrice HTTP call") @@ -611,7 +611,7 @@ func TestComputeClient_PurchaseCommitment_BadStatus(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(), "reservation purchase failed with status 400") @@ -649,7 +649,7 @@ func TestComputeClient_PurchaseCommitment_TwoStepFlow(t *testing.T) { CommitmentCost: 500.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) // CommitmentID must be the Azure-minted ID, not a client-generated GUID. @@ -690,13 +690,34 @@ func TestComputeClient_PurchaseCommitment_SessionTimeoutRetry(t *testing.T) { })).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{}) + 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_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 // contract: unusable SDK payloads (nil, wrong concrete type, nil Properties) // produce a nil *Recommendation so the caller can filter it out. Before diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index a0de864dd..b69fea4eb 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -266,6 +266,17 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco 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 := 1 if rec.Term == "3yr" || rec.Term == "3" { termYears = 3 diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index fdffe30e5..dd90e110c 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -739,7 +739,7 @@ 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.Equal(t, "cosmos-order-001", result.CommitmentID) @@ -767,7 +767,7 @@ 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) @@ -794,7 +794,7 @@ 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) @@ -811,7 +811,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") @@ -832,7 +832,7 @@ func TestCosmosDBClient_PurchaseCommitment_HTTPError(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(), "calculatePrice HTTP call") @@ -856,13 +856,33 @@ func TestCosmosDBClient_PurchaseCommitment_BadStatus(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(), "reservation purchase failed with status 400") 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 // new contract: unusable SDK payloads produce a nil *Recommendation. func TestCosmosDBClient_ConvertAzureCosmosRecommendation_NilGuards(t *testing.T) { diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index 5765a7050..9c5bd9fd2 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -274,6 +274,17 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco 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 := 1 if rec.Term == "3yr" || rec.Term == "3" { termYears = 3 diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 1d5af5082..2c1aa2dfe 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -850,7 +850,7 @@ 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.Equal(t, "db-order-001", result.CommitmentID) @@ -878,7 +878,7 @@ 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) @@ -905,7 +905,7 @@ 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) @@ -922,7 +922,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") @@ -943,7 +943,7 @@ func TestDatabaseClient_PurchaseCommitment_HTTPError(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(), "calculatePrice HTTP call") @@ -967,13 +967,33 @@ func TestDatabaseClient_PurchaseCommitment_BadStatus(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(), "reservation purchase failed with status 400") 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) { ctx := context.Background() client := NewClient(nil, "test-subscription", "eastus") diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index f00ea5199..6c1c95dbc 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -249,6 +249,17 @@ 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 diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index af580fdc4..8c187235b 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -522,7 +522,7 @@ func TestPurchaseCommitment_Success(t *testing.T) { 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.Equal(t, "mr-order-001", result.CommitmentID) @@ -542,7 +542,7 @@ func TestPurchaseCommitment_3yr(t *testing.T) { 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) @@ -561,7 +561,7 @@ func TestPurchaseCommitment_Accepted(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.NoError(t, err) assert.True(t, result.Success) } @@ -573,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") @@ -589,7 +589,7 @@ func TestPurchaseCommitment_HTTPError(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(), "calculatePrice HTTP call") @@ -608,7 +608,7 @@ func TestPurchaseCommitment_BadStatus(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(), "reservation purchase failed with status 400") @@ -620,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) }) @@ -722,56 +742,41 @@ func TestRedisPricingStruct(t *testing.T) { } // TestPurchaseCommitment_TagInjection verifies that the purchase-automation tag -// is present in the purchase request body when opts.Source is set, and absent -// when opts.Source is empty. +// 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" - for _, tc := range []struct { - name string - source string - expectTag bool - }{ - {"tag_present_when_source_set", source, true}, - {"tag_absent_when_source_empty", "", false}, - } { - tc := tc - t.Run(tc.name, func(t *testing.T) { - 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: tc.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{}) - if tc.expectTag { - require.True(t, hasTags, "tags field must be present in purchase body when Source is set") - assert.Equal(t, tc.source, tags[common.PurchaseTagKey], "tag value must match opts.Source") - } else { - assert.False(t, hasTags, "tags field must be absent in purchase body when Source is empty") - } - }) - } + 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 bfe41a1dd..25d0dd414 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -246,6 +246,17 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm 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 := 1 if rec.Term == "3yr" || rec.Term == "3" { termYears = 3 diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index 628868e2d..c15fc9d0c 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -643,7 +643,7 @@ 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.Equal(t, "search-order-001", result.CommitmentID) @@ -671,7 +671,7 @@ 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) @@ -698,7 +698,7 @@ 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) @@ -715,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") @@ -736,7 +736,7 @@ func TestSearchClient_PurchaseCommitment_HTTPError(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(), "calculatePrice HTTP call") @@ -760,7 +760,7 @@ func TestSearchClient_PurchaseCommitment_BadStatus(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(), "reservation purchase failed with status 400") @@ -851,7 +851,7 @@ func TestSearchClient_PurchaseCommitment_TwoStepFlow(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.Equal(t, azureMintedOrderID, result.CommitmentID, @@ -862,55 +862,61 @@ func TestSearchClient_PurchaseCommitment_TwoStepFlow(t *testing.T) { // TestSearchClient_PurchaseCommitment_TagInjection verifies that the // purchase-automation tag is present in the calculatePrice request body when -// opts.Source is set, and absent when opts.Source is empty. +// 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" - for _, tc := range []struct { - name string - source string - expectTag bool - }{ - {"tag_present_when_source_set", source, true}, - {"tag_absent_when_source_empty", "", false}, - } { - tc := tc - t.Run(tc.name, func(t *testing.T) { - 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: tc.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{}) - if tc.expectTag { - require.True(t, hasTags, "tags field must be present in calculatePrice body when Source is set") - assert.Equal(t, tc.source, tags[common.PurchaseTagKey], "tag value must match opts.Source") - } else { - assert.False(t, hasTags, "tags field must be absent in calculatePrice body when Source is empty") - } - mockHTTP.AssertExpectations(t) - }) - } + 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 94dc3d508..555147aae 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -251,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 diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index a81ca451d..c17baa66b 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -516,7 +516,7 @@ 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.Equal(t, "syn-order-001", result.CommitmentID) @@ -537,7 +537,7 @@ func TestPurchaseCommitment_3yrTerm(t *testing.T) { 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) } @@ -585,7 +585,7 @@ func TestPurchaseCommitment_apiError(t *testing.T) { 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) } @@ -602,7 +602,7 @@ func TestPurchaseCommitment_httpError(t *testing.T) { 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) assert.Contains(t, err.Error(), "calculatePrice HTTP call") @@ -613,7 +613,7 @@ func TestPurchaseCommitment_tokenError(t *testing.T) { 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) } @@ -762,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") @@ -772,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") @@ -782,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) } @@ -791,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) { From d283edc46d11751ff59654d3ee84ca434cf29674 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 18:06:12 +0200 Subject: [PATCH 6/6] test(azure): assert purchase-automation tag is sent in calculatePrice body CodeRabbit pass-3 round-1 N1 (nitpick) + N2 (nitpick covered by same shape): the existing TwoStepFlow / Success regression tests for the Azure two-step calculatePrice -> purchase flow only assert HTTP call counts and outcome -- they do not capture the calculatePrice body or check that opts.Source has been embedded as a purchase-automation tag. Without that assertion the dedupe invariant (the only stable signal Azure's server-minted-order-ID API exposes) could regress silently: tag drop elsewhere in the code (e.g. helper refactor, JSON shape change) would not fail the suite. Add a *_PurchaseCommitment_TagInjection test in each service that lacks one today: cache, database, cosmosdb, compute. Each test: 1. Wires a mock that captures the calculatePrice request body via io.ReadAll and rewinds it with bytes.NewReader so the production code sees an intact request. 2. Drives PurchaseCommitment with common.PurchaseOptions{Source: PurchaseSourceWeb}. 3. Unmarshals the captured body and asserts the top-level "tags" map contains common.PurchaseTagKey ("purchase-automation") -> source. Services already covered by an equivalent test, intentionally untouched: - search: TestSearchClient_PurchaseCommitment_TagInjection - managedredis: TestPurchaseCommitment_TagInjection - synapse: TestPurchaseCommitment_withSource (asserts both "purchase-automation" and the source value are present in the captured body) savingsplans excluded -- different API (OrderAlias) with native DisplayName-based dedupe; opts.Source is `_`-ignored at the entry point. Net effect: the tag invariant is now pinned by a dedicated regression test for every Azure two-step service. --- providers/azure/services/cache/client_test.go | 42 +++++++++++++++++ .../azure/services/compute/client_test.go | 46 +++++++++++++++++++ .../azure/services/cosmosdb/client_test.go | 42 +++++++++++++++++ .../azure/services/database/client_test.go | 42 +++++++++++++++++ 4 files changed, 172 insertions(+) diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index ee8d97d9a..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" @@ -1065,6 +1066,47 @@ func TestCacheClient_PurchaseCommitment_BadStatus(t *testing.T) { 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 diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index 6c95517d9..dfc7a89f8 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -1,9 +1,11 @@ package compute import ( + "bytes" "context" "encoding/json" "errors" + "io" "net/http" "strconv" "strings" @@ -697,6 +699,50 @@ func TestComputeClient_PurchaseCommitment_SessionTimeoutRetry(t *testing.T) { 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). diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index dd90e110c..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" @@ -863,6 +864,47 @@ func TestCosmosDBClient_PurchaseCommitment_BadStatus(t *testing.T) { 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 diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 2c1aa2dfe..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" @@ -974,6 +975,47 @@ func TestDatabaseClient_PurchaseCommitment_BadStatus(t *testing.T) { 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