diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 770b3979b..706c1d115 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -273,14 +273,13 @@ 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. + // Source is required so the resulting reservation is attributable to CUDly + // in the portal via the purchase-automation tag. The dedupe key for + // idempotent re-drives is now opts.IdempotencyToken (issue #721, applied + // in reservations.DoIdempotentPurchaseTwoStep); source remains mandatory + // for attribution. if opts.Source == "" { - result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } @@ -312,7 +311,7 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme "renew": false, }, } - applyPurchaseAutomationTag(requestBody, opts.Source) + reservations.ApplyPurchaseTags(requestBody, opts.Source, opts.IdempotencyToken) bodyBytes, err := json.Marshal(requestBody) if err != nil { @@ -328,7 +327,7 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme return result, result.Error } - reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) + reservationOrderID, err := reservations.DoIdempotentPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token, opts.IdempotencyToken) if err != nil { result.Error = err return result, result.Error @@ -677,14 +676,3 @@ func (c *CacheClient) fetchSKUCatalogue(ctx context.Context) map[string]redisSKU } return out } - -// applyPurchaseAutomationTag attaches the purchase-automation tag to an Azure -// reservation request body when source is non-empty. Extracted out of -// PurchaseCommitment to keep the function under the cyclomatic-complexity -// threshold enforced by the pre-commit hook. -func applyPurchaseAutomationTag(body map[string]interface{}, source string) { - if source == "" { - return - } - body["tags"] = map[string]string{common.PurchaseTagKey: source} -} diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 93cd3a6e4..f97e0f42a 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -363,9 +363,11 @@ func (c *ComputeClient) checkAndRegisterCapacityProvider(ctx context.Context) er // 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 so the resulting reservation is identifiable in the portal. -func (c *ComputeClient) buildReservationBody(rec common.Recommendation, source string) ([]byte, error) { +// The purchase-automation and cudly-idempotency-token tags are attached via +// reservations.ApplyPurchaseTags so the resulting reservation is identifiable +// in the portal AND a re-driven purchase can find it via tag lookup before +// buying a duplicate (issue #721). +func (c *ComputeClient) buildReservationBody(rec common.Recommendation, source, idempotencyToken string) ([]byte, error) { termYears := 1 if rec.Term == "3yr" || rec.Term == "3" { termYears = 3 @@ -391,9 +393,7 @@ func (c *ComputeClient) buildReservationBody(rec common.Recommendation, source s "renew": false, }, } - if source != "" { - requestBody["tags"] = map[string]string{common.PurchaseTagKey: source} - } + reservations.ApplyPurchaseTags(requestBody, source, idempotencyToken) return json.Marshal(requestBody) } @@ -407,9 +407,11 @@ func (c *ComputeClient) buildReservationBody(rec common.Recommendation, source s // 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). +// no longer used. Re-drives are idempotent via tag-based deduplication +// performed inside reservations.DoIdempotentPurchaseTwoStep: every purchase +// body carries the cudly-idempotency-token tag derived from opts.IdempotencyToken, +// and a re-drive lists existing reservation orders and short-circuits when an +// order already carries the same tag (issue #721). func (c *ComputeClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, @@ -418,21 +420,20 @@ 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. + // Source is required so the resulting reservation is attributable to CUDly + // in the portal via the purchase-automation tag. The dedupe key for + // idempotent re-drives is now opts.IdempotencyToken (issue #721, applied + // in reservations.DoIdempotentPurchaseTwoStep); source remains mandatory + // for attribution. if opts.Source == "" { - result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } // Ensure Microsoft.Capacity provider is registered (cached after first call). c.ensureCapacityProviderRegistered(ctx) - bodyBytes, err := c.buildReservationBody(rec, opts.Source) + bodyBytes, err := c.buildReservationBody(rec, opts.Source, opts.IdempotencyToken) if err != nil { result.Error = fmt.Errorf("failed to marshal request: %w", err) return result, result.Error @@ -446,7 +447,7 @@ func (c *ComputeClient) PurchaseCommitment(ctx context.Context, rec common.Recom return result, result.Error } - reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) + reservationOrderID, err := reservations.DoIdempotentPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token, opts.IdempotencyToken) 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 5aa86b1ef..579a81e29 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -925,7 +925,7 @@ func TestBuildReservationBody_IncludesPurchaseAutomationTag(t *testing.T) { c := &ComputeClient{region: "eastus", subscriptionID: "sub-abc"} rec := common.Recommendation{ResourceType: "Standard_D2s_v3", Count: 1, Term: "1yr"} - body, err := c.buildReservationBody(rec, common.PurchaseSourceWeb) + body, err := c.buildReservationBody(rec, common.PurchaseSourceWeb, "") require.NoError(t, err) var got map[string]interface{} @@ -935,17 +935,38 @@ func TestBuildReservationBody_IncludesPurchaseAutomationTag(t *testing.T) { assert.Equal(t, common.PurchaseSourceWeb, tags[common.PurchaseTagKey]) } -func TestBuildReservationBody_OmitsTagsWhenSourceEmpty(t *testing.T) { +func TestBuildReservationBody_OmitsTagsWhenSourceAndTokenEmpty(t *testing.T) { c := &ComputeClient{region: "eastus", subscriptionID: "sub-abc"} rec := common.Recommendation{ResourceType: "Standard_D2s_v3", Count: 1, Term: "1yr"} - body, err := c.buildReservationBody(rec, "") + body, err := c.buildReservationBody(rec, "", "") require.NoError(t, err) var got map[string]interface{} require.NoError(t, json.Unmarshal(body, &got)) _, present := got["tags"] - assert.False(t, present, "tags must be absent when source is empty") + assert.False(t, present, "tags must be absent when both source and idempotency token are empty") +} + +// TestBuildReservationBody_IncludesIdempotencyTokenTag pins the issue #721 +// fix: the cudly-idempotency-token tag MUST ride along with the +// purchase-automation tag in the reservation body so a re-driven purchase +// can find the prior reservation via FindReservationOrderByIdempotencyToken +// and skip the duplicate buy. +func TestBuildReservationBody_IncludesIdempotencyTokenTag(t *testing.T) { + c := &ComputeClient{region: "eastus", subscriptionID: "sub-abc"} + rec := common.Recommendation{ResourceType: "Standard_D2s_v3", Count: 1, Term: "1yr"} + token := common.DeriveIdempotencyToken("exec-721-compute", 0) + + body, err := c.buildReservationBody(rec, common.PurchaseSourceWeb, token) + require.NoError(t, err) + + var got map[string]interface{} + require.NoError(t, json.Unmarshal(body, &got)) + tags, ok := got["tags"].(map[string]interface{}) + require.True(t, ok, "tags map missing from reservation body") + assert.Equal(t, common.PurchaseSourceWeb, tags[common.PurchaseTagKey]) + assert.Equal(t, token, tags[common.IdempotencyTagKey], "idempotency tag must be stamped when token is supplied") } // --- Issue #148: VCPU/MemoryGB enrichment via cached SKU catalogue --- diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index c28928382..85b5b6d97 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -266,14 +266,13 @@ 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. + // Source is required so the resulting reservation is attributable to CUDly + // in the portal via the purchase-automation tag. The dedupe key for + // idempotent re-drives is now opts.IdempotencyToken (issue #721, applied + // in reservations.DoIdempotentPurchaseTwoStep); source remains mandatory + // for attribution. if opts.Source == "" { - result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } @@ -305,7 +304,7 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco "renew": false, }, } - applyPurchaseAutomationTag(requestBody, opts.Source) + reservations.ApplyPurchaseTags(requestBody, opts.Source, opts.IdempotencyToken) bodyBytes, err := json.Marshal(requestBody) if err != nil { @@ -321,7 +320,7 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco return result, result.Error } - reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) + reservationOrderID, err := reservations.DoIdempotentPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token, opts.IdempotencyToken) if err != nil { result.Error = err return result, result.Error @@ -751,14 +750,3 @@ func detailsFromCosmosSKU(sku string) common.NoSQLDetails { } return d } - -// applyPurchaseAutomationTag attaches the purchase-automation tag to an Azure -// reservation request body when source is non-empty. Extracted out of -// PurchaseCommitment to keep the function under the cyclomatic-complexity -// threshold enforced by the pre-commit hook. -func applyPurchaseAutomationTag(body map[string]interface{}, source string) { - if source == "" { - return - } - body["tags"] = map[string]string{common.PurchaseTagKey: source} -} diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index 3311f2a23..f214501c5 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -274,14 +274,13 @@ 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. + // Source is required so the resulting reservation is attributable to CUDly + // in the portal via the purchase-automation tag. The dedupe key for + // idempotent re-drives is now opts.IdempotencyToken (issue #721, applied + // in reservations.DoIdempotentPurchaseTwoStep); source remains mandatory + // for attribution. if opts.Source == "" { - result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } @@ -313,7 +312,7 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco "renew": false, }, } - applyPurchaseAutomationTag(requestBody, opts.Source) + reservations.ApplyPurchaseTags(requestBody, opts.Source, opts.IdempotencyToken) bodyBytes, err := json.Marshal(requestBody) if err != nil { @@ -329,7 +328,7 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco return result, result.Error } - reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) + reservationOrderID, err := reservations.DoIdempotentPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token, opts.IdempotencyToken) if err != nil { result.Error = err return result, result.Error @@ -696,14 +695,3 @@ func detailsFromSQLSKU(sku string) common.DatabaseDetails { } return d } - -// applyPurchaseAutomationTag attaches the purchase-automation tag to an Azure -// reservation request body when source is non-empty. Extracted out of -// PurchaseCommitment to keep the function under the cyclomatic-complexity -// threshold enforced by the pre-commit hook. -func applyPurchaseAutomationTag(body map[string]interface{}, source string) { - if source == "" { - return - } - body["tags"] = map[string]string{common.PurchaseTagKey: source} -} diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index ad23cc6dc..d736dafdf 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -1,6 +1,6 @@ // Package reservations provides the shared two-step calculatePrice->purchase // flow for all Azure reservation-based service clients (compute, database, -// cache, search, cosmosdb, managedredis). +// cache, search, cosmosdb, managedredis, synapse). // // Azure's Reservations API shifted away from direct-PUT for newer SKU families // (Burstable v2 and likely others). The previous pattern: @@ -16,13 +16,26 @@ // 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). +// Idempotency strategy (Option B from issue #677, finished by issue #721): +// because Azure mints the order ID in step 1 we cannot use a client-supplied ID +// for deduplication (PR #653's IdempotencyGUID pattern). Instead, every +// purchase request body carries two tags: +// +// - purchase-automation= -- cosmetic attribution. +// - cudly-idempotency-token= -- the (execution, rec) +// identity from common.DeriveIdempotencyToken. +// +// Before invoking the two-step flow, DoIdempotentPurchaseTwoStep lists existing +// reservation orders and short-circuits when one already carries the same +// idempotency token, so a re-driven purchase of a stranded execution (issue +// #636) reuses the prior reservation rather than buying a second one. Mirrors +// the AWS EC2 findRIByIdempotencyToken pattern (providers/aws/services/ec2). +// +// This package exposes IsSessionTimeout to let callers classify errors, +// DoPurchaseTwoStep (the raw two-step flow without the dedupe guard, retained +// for callers that have no IdempotencyToken), DoIdempotentPurchaseTwoStep +// (the guarded wrapper that every service executor uses), and ApplyPurchaseTags +// (the canonical place where both tags are written into a request body). package reservations import ( @@ -35,6 +48,8 @@ import ( "net/http" "strings" "time" + + "github.com/LeanerCloud/CUDly/pkg/common" ) // apiVersion is the GA api-version for the Microsoft.Capacity Reservations API. @@ -56,6 +71,38 @@ func PurchaseURL(reservationOrderID string) string { BaseURL, reservationOrderID, apiVersion) } +// ReservationOrdersListURL returns the list-reservation-orders endpoint URL. +// The endpoint is tenant-wide (no subscription prefix): the caller's bearer +// token determines visibility, and the idempotency-token tag is globally +// unique per (execution, rec), so a tenant-wide search returns the correct +// order regardless of which subscription executed the purchase. +func ReservationOrdersListURL() string { + return BaseURL + "/providers/Microsoft.Capacity/reservationOrders?api-version=" + apiVersion +} + +// ApplyPurchaseTags stamps the two purchase-attribution tags onto an Azure +// reservation purchase request body. Either tag is omitted when its source +// value is empty. When both are empty, no tags map is added (preserving the +// untagged shape some legacy CLI test paths expect). +// +// The tags are sent verbatim in the calculatePrice + purchase request bodies +// and are persisted by Azure onto the resulting reservation order so the +// idempotency tag can be read back by FindReservationOrderByIdempotencyToken +// on a re-drive. +func ApplyPurchaseTags(body map[string]interface{}, source, idempotencyToken string) { + if source == "" && idempotencyToken == "" { + return + } + tags := map[string]string{} + if source != "" { + tags[common.PurchaseTagKey] = source + } + if idempotencyToken != "" { + tags[common.IdempotencyTagKey] = idempotencyToken + } + body["tags"] = tags +} + // 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. @@ -163,6 +210,161 @@ func doCalculatePrice(ctx context.Context, httpClient HTTPClient, calcURL string return result.Properties.ReservationOrderID, nil } +// reservationOrdersListResponse is the JSON shape of a single page returned by +// the list-reservation-orders endpoint. Only the fields needed for the +// idempotency-token lookup are decoded. +type reservationOrdersListResponse struct { + Value []struct { + // Name is the reservation order GUID; this is what Azure mints as + // reservationOrderId during calculatePrice and what DoPurchaseTwoStep + // returns on success. + Name string `json:"name"` + // Tags carries any tags stamped on the order at purchase time. + Tags map[string]string `json:"tags"` + // Properties.ProvisioningState lets us skip terminal-failed orders so a + // cancelled/failed reservation with the same idempotency tag does not + // suppress a legitimate fresh purchase. Anything outside the + // suppressing-failure set is treated as a duplicate that should + // short-circuit (mirrors the AWS pattern of including in-flight states, + // not just succeeded ones). + Properties struct { + ProvisioningState string `json:"provisioningState"` + } `json:"properties"` + } `json:"value"` + NextLink string `json:"nextLink"` +} + +// reservationOrderTerminalFailedStates enumerates the provisioning states for +// which an existing reservation order MUST NOT suppress a fresh purchase. +// A cancelled/failed/expired order with a matching idempotency tag was +// either rolled back or aged out; the recommendation is still owed and the +// re-drive must be allowed through. +var reservationOrderTerminalFailedStates = map[string]struct{}{ + "Cancelled": {}, + "Failed": {}, + "Expired": {}, +} + +// FindReservationOrderByIdempotencyToken lists reservation orders visible to +// the bearer token and returns the reservation order ID (GUID) of the FIRST +// order tagged with the supplied idempotency token. Returns ("", false, nil) +// when no matching order is found and ("", false, err) on a transport / 4xx / +// decode failure. +// +// Pagination follows nextLink; the entire listing is walked because Azure has +// no server-side tag filter on this endpoint. Reservation order listings are +// small (one entry per purchase ever made on the tenant, typically tens to low +// hundreds), so the full walk is cheap and runs only on purchase paths where +// an idempotency token is supplied (i.e. never on the CLI legacy path). +// +// Terminal-failed orders (Cancelled, Failed, Expired) are skipped so they do +// not suppress a legitimate fresh purchase of the same recommendation -- this +// mirrors the EC2 dedupe guard's state filter (active + payment-pending only). +func FindReservationOrderByIdempotencyToken(ctx context.Context, httpClient HTTPClient, bearerToken, idempotencyToken string) (string, bool, error) { + if idempotencyToken == "" { + return "", false, nil + } + + nextURL := ReservationOrdersListURL() + for nextURL != "" { + page, err := fetchReservationOrdersPage(ctx, httpClient, nextURL, bearerToken) + if err != nil { + return "", false, err + } + if orderID, found := matchReservationOrderInPage(page, idempotencyToken); found { + return orderID, true, nil + } + nextURL = page.NextLink + } + + return "", false, nil +} + +// fetchReservationOrdersPage GETs a single page of the list-reservation-orders +// endpoint and decodes the response. Extracted from +// FindReservationOrderByIdempotencyToken so the outer pagination loop stays +// under the gocyclo:10 threshold. +func fetchReservationOrdersPage(ctx context.Context, httpClient HTTPClient, pageURL, bearerToken string) (*reservationOrdersListResponse, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, pageURL, nil) + if err != nil { + return nil, fmt.Errorf("build list reservation orders 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 nil, fmt.Errorf("list reservation orders HTTP call: %w", err) + } + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return nil, fmt.Errorf("list reservation orders failed with status %d: %s", resp.StatusCode, string(body)) + } + + var page reservationOrdersListResponse + if err := json.Unmarshal(body, &page); err != nil { + return nil, fmt.Errorf("decode list reservation orders response: %w", err) + } + return &page, nil +} + +// matchReservationOrderInPage scans one decoded page for an order tagged with +// the idempotency token. Terminal-failed (Cancelled/Failed/Expired) orders +// are skipped so they do not suppress a legitimate fresh purchase. Extracted +// from FindReservationOrderByIdempotencyToken to keep the function under the +// gocyclo:10 threshold enforced by the pre-commit hook. +func matchReservationOrderInPage(page *reservationOrdersListResponse, idempotencyToken string) (string, bool) { + for _, order := range page.Value { + if order.Tags[common.IdempotencyTagKey] != idempotencyToken { + continue + } + if _, terminalFailed := reservationOrderTerminalFailedStates[order.Properties.ProvisioningState]; terminalFailed { + continue + } + if order.Name == "" { + continue + } + return order.Name, true + } + return "", false +} + +// DoIdempotentPurchaseTwoStep is the dedupe-guarded wrapper around +// DoPurchaseTwoStep that every Azure service executor should use. Flow: +// +// 1. If idempotencyToken is empty, fall straight through to DoPurchaseTwoStep +// (preserves the CLI path's pre-issue-721 behaviour, which has no owning +// execution and so no token to dedupe on). +// 2. Otherwise, look for an existing reservation order already tagged with +// the token. If found, short-circuit and return its order ID -- this is a +// re-drive of an execution that already created the reservation; buying +// again would double-charge the customer (issues #641, #721). +// 3. Otherwise, call DoPurchaseTwoStep. The caller is responsible for having +// stamped the idempotency tag into bodyBytes via ApplyPurchaseTags so the +// resulting order is tagged and the NEXT re-drive will short-circuit at +// step 2. +// +// A failed lookup must NOT fall through to a purchase: doing so would defeat +// the guard and risk a double-buy on a re-drive. The lookup error is returned +// verbatim, and the recovery sweep treats the recommendation as not-yet- +// purchased and retries the whole guarded path (mirroring the EC2 EC2 +// findRIByIdempotencyToken safety contract in providers/aws/services/ec2). +func DoIdempotentPurchaseTwoStep(ctx context.Context, httpClient HTTPClient, calcURL string, bodyBytes []byte, bearerToken, idempotencyToken string) (string, error) { + if idempotencyToken != "" { + existingID, found, err := FindReservationOrderByIdempotencyToken(ctx, httpClient, bearerToken, idempotencyToken) + if err != nil { + return "", fmt.Errorf("idempotency lookup failed before Azure reservation purchase (refusing to purchase to avoid a possible double-buy): %w", err) + } + if found { + log.Printf("Azure reservation order for idempotency token %s already exists (%s); skipping purchase (issue #721 re-drive)", common.MaskToken(idempotencyToken), existingID) + return existingID, nil + } + } + return DoPurchaseTwoStep(ctx, httpClient, calcURL, bodyBytes, bearerToken) +} + // 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 diff --git a/providers/azure/services/internal/reservations/purchase_test.go b/providers/azure/services/internal/reservations/purchase_test.go index 90071ba5e..3e81bfaca 100644 --- a/providers/azure/services/internal/reservations/purchase_test.go +++ b/providers/azure/services/internal/reservations/purchase_test.go @@ -11,6 +11,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/CUDly/pkg/common" ) // mockHTTPClient implements HTTPClient for tests. @@ -213,3 +215,463 @@ 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) } + +func TestReservationOrdersListURL(t *testing.T) { + u := ReservationOrdersListURL() + assert.Equal(t, "https://management.azure.com/providers/Microsoft.Capacity/reservationOrders?api-version=2022-11-01", u) +} + +// ---- ApplyPurchaseTags ---------------------------------------------------- + +// TestApplyPurchaseTags_BothTags pins the issue #721 invariant: both the +// purchase-automation tag and the cudly-idempotency-token tag must be written +// into the same tags map so the resulting Azure reservation order is BOTH +// attributable AND findable by FindReservationOrderByIdempotencyToken on a +// re-drive. +func TestApplyPurchaseTags_BothTags(t *testing.T) { + body := map[string]interface{}{} + ApplyPurchaseTags(body, common.PurchaseSourceWeb, "idem-tok-abc") + + tags, ok := body["tags"].(map[string]string) + require.True(t, ok, "tags map must be present when at least one tag is supplied") + assert.Equal(t, common.PurchaseSourceWeb, tags[common.PurchaseTagKey]) + assert.Equal(t, "idem-tok-abc", tags[common.IdempotencyTagKey]) + assert.Len(t, tags, 2, "no extra tag keys must be written") +} + +func TestApplyPurchaseTags_OnlySource(t *testing.T) { + body := map[string]interface{}{} + ApplyPurchaseTags(body, common.PurchaseSourceCLI, "") + + tags, ok := body["tags"].(map[string]string) + require.True(t, ok) + assert.Equal(t, common.PurchaseSourceCLI, tags[common.PurchaseTagKey]) + _, hasIdem := tags[common.IdempotencyTagKey] + assert.False(t, hasIdem, "idempotency tag must be absent when token is empty (CLI path)") +} + +func TestApplyPurchaseTags_OnlyIdempotency(t *testing.T) { + body := map[string]interface{}{} + ApplyPurchaseTags(body, "", "idem-only") + + tags, ok := body["tags"].(map[string]string) + require.True(t, ok) + _, hasSource := tags[common.PurchaseTagKey] + assert.False(t, hasSource, "purchase-automation tag must be absent when source is empty") + assert.Equal(t, "idem-only", tags[common.IdempotencyTagKey]) +} + +// TestApplyPurchaseTags_NoTags verifies the CLI legacy shape: with both +// source and idempotency token empty the body's tags field must not even be +// present. This preserves the existing snapshot tests in the per-service +// client_test.go files that assert the absence of "tags" when source is "". +func TestApplyPurchaseTags_NoTags(t *testing.T) { + body := map[string]interface{}{"sku": "Standard_D2s_v3"} + ApplyPurchaseTags(body, "", "") + + _, present := body["tags"] + assert.False(t, present, "tags must be omitted entirely when both source and token are empty") +} + +// ---- FindReservationOrderByIdempotencyToken ------------------------------- + +// orderListJSON renders a single-page list-reservation-orders response with +// the given orders. Used by all FindReservationOrderByIdempotencyToken tests. +func orderListJSON(orders []struct { + Name string + IdempotencyToken string + ProvisioningState string + OtherTags map[string]string +}, nextLink string) string { + out := `{"value":[` + for i, o := range orders { + if i > 0 { + out += "," + } + out += `{"name":"` + o.Name + `","tags":{` + first := true + if o.IdempotencyToken != "" { + out += `"` + common.IdempotencyTagKey + `":"` + o.IdempotencyToken + `"` + first = false + } + for k, v := range o.OtherTags { + if !first { + out += "," + } + out += `"` + k + `":"` + v + `"` + first = false + } + out += `},"properties":{"provisioningState":"` + o.ProvisioningState + `"}}` + } + out += `]` + if nextLink != "" { + out += `,"nextLink":"` + nextLink + `"` + } + out += "}" + return out +} + +const listURL = "https://management.azure.com/providers/Microsoft.Capacity/reservationOrders?api-version=2022-11-01" + +func TestFindReservationOrderByIdempotencyToken_Match(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + body := orderListJSON([]struct { + Name string + IdempotencyToken string + ProvisioningState string + OtherTags map[string]string + }{ + {Name: "order-other", IdempotencyToken: "other-tok", ProvisioningState: "Succeeded"}, + {Name: "order-match", IdempotencyToken: "wanted-tok", ProvisioningState: "Succeeded"}, + }, "") + + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet && r.URL.String() == listURL + })).Return(fakeResp(http.StatusOK, body), nil).Once() + + orderID, found, err := FindReservationOrderByIdempotencyToken(ctx, m, "tok", "wanted-tok") + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, "order-match", orderID) +} + +func TestFindReservationOrderByIdempotencyToken_NoMatch(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + body := orderListJSON([]struct { + Name string + IdempotencyToken string + ProvisioningState string + OtherTags map[string]string + }{ + {Name: "order-1", IdempotencyToken: "some-other-tok", ProvisioningState: "Succeeded"}, + }, "") + + m.On("Do", mock.Anything).Return(fakeResp(http.StatusOK, body), nil).Once() + + orderID, found, err := FindReservationOrderByIdempotencyToken(ctx, m, "tok", "wanted-tok") + require.NoError(t, err) + assert.False(t, found) + assert.Empty(t, orderID) +} + +// TestFindReservationOrderByIdempotencyToken_SkipsTerminalFailed pins the +// state filter: a cancelled/failed/expired order carrying the same idempotency +// tag MUST NOT short-circuit a legitimate fresh purchase. Mirrors the AWS EC2 +// findRIByIdempotencyToken filter (state in active|payment-pending). +func TestFindReservationOrderByIdempotencyToken_SkipsTerminalFailed(t *testing.T) { + for _, state := range []string{"Cancelled", "Failed", "Expired"} { + t.Run(state, func(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + body := orderListJSON([]struct { + Name string + IdempotencyToken string + ProvisioningState string + OtherTags map[string]string + }{ + {Name: "order-dead", IdempotencyToken: "wanted-tok", ProvisioningState: state}, + }, "") + + m.On("Do", mock.Anything).Return(fakeResp(http.StatusOK, body), nil).Once() + + orderID, found, err := FindReservationOrderByIdempotencyToken(ctx, m, "tok", "wanted-tok") + require.NoError(t, err) + assert.False(t, found, "terminal-failed (%s) order must not short-circuit a fresh purchase", state) + assert.Empty(t, orderID) + }) + } +} + +// TestFindReservationOrderByIdempotencyToken_AcceptsInFlightStates verifies +// that orders in non-terminal states (Succeeded, Pending, Creating, +// ConfirmedBilling) DO short-circuit the purchase: those orders are either +// already-paid-for or currently being processed and a re-drive would create +// a duplicate. +func TestFindReservationOrderByIdempotencyToken_AcceptsInFlightStates(t *testing.T) { + for _, state := range []string{"Succeeded", "Pending", "Creating", "ConfirmedBilling", ""} { + t.Run(state, func(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + body := orderListJSON([]struct { + Name string + IdempotencyToken string + ProvisioningState string + OtherTags map[string]string + }{ + {Name: "order-live", IdempotencyToken: "wanted-tok", ProvisioningState: state}, + }, "") + + m.On("Do", mock.Anything).Return(fakeResp(http.StatusOK, body), nil).Once() + + orderID, found, err := FindReservationOrderByIdempotencyToken(ctx, m, "tok", "wanted-tok") + require.NoError(t, err) + assert.True(t, found, "non-terminal state %q must short-circuit a re-drive", state) + assert.Equal(t, "order-live", orderID) + }) + } +} + +func TestFindReservationOrderByIdempotencyToken_HTTPError(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + m.On("Do", mock.Anything).Return(nil, errors.New("dial tcp: connection refused")).Once() + + _, found, err := FindReservationOrderByIdempotencyToken(ctx, m, "tok", "wanted-tok") + require.Error(t, err) + assert.False(t, found) + assert.Contains(t, err.Error(), "list reservation orders HTTP call") +} + +func TestFindReservationOrderByIdempotencyToken_403(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + m.On("Do", mock.Anything).Return(fakeResp(http.StatusForbidden, `{"error":"insufficient permissions"}`), nil).Once() + + _, found, err := FindReservationOrderByIdempotencyToken(ctx, m, "tok", "wanted-tok") + require.Error(t, err) + assert.False(t, found) + assert.Contains(t, err.Error(), "status 403") +} + +func TestFindReservationOrderByIdempotencyToken_EmptyToken(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + // No HTTP call expected: empty token short-circuits at function entry. + _, found, err := FindReservationOrderByIdempotencyToken(ctx, m, "tok", "") + require.NoError(t, err) + assert.False(t, found) + m.AssertNotCalled(t, "Do", mock.Anything) +} + +func TestFindReservationOrderByIdempotencyToken_PaginatedFollowsNextLink(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + nextURL := "https://management.azure.com/providers/Microsoft.Capacity/reservationOrders?api-version=2022-11-01&$skiptoken=p2" + + // Page 1: no match, but a nextLink to page 2. + page1 := orderListJSON([]struct { + Name string + IdempotencyToken string + ProvisioningState string + OtherTags map[string]string + }{ + {Name: "order-p1", IdempotencyToken: "other-tok", ProvisioningState: "Succeeded"}, + }, nextURL) + // Page 2: the match. + page2 := orderListJSON([]struct { + Name string + IdempotencyToken string + ProvisioningState string + OtherTags map[string]string + }{ + {Name: "order-p2-match", IdempotencyToken: "wanted-tok", ProvisioningState: "Succeeded"}, + }, "") + + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.String() == listURL + })).Return(fakeResp(http.StatusOK, page1), nil).Once() + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.URL.String() == nextURL + })).Return(fakeResp(http.StatusOK, page2), nil).Once() + + orderID, found, err := FindReservationOrderByIdempotencyToken(ctx, m, "tok", "wanted-tok") + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, "order-p2-match", orderID) + m.AssertExpectations(t) +} + +// ---- DoIdempotentPurchaseTwoStep ----------------------------------------- + +// TestDoIdempotentPurchaseTwoStep_EmptyToken_NoLookup pins the CLI legacy path: +// when no idempotency token is supplied the wrapper falls straight through to +// the raw DoPurchaseTwoStep (no list call), preserving the pre-issue-721 +// behaviour for callers without an owning execution. +func TestDoIdempotentPurchaseTwoStep_EmptyToken_NoLookup(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + // Only the standard calculatePrice + purchase calls -- no list call. + 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-no-tok"}}`), 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-no-tok/purchase" + })).Return(fakeResp(http.StatusOK, `{}`), nil).Once() + + orderID, err := DoIdempotentPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok", "") + require.NoError(t, err) + assert.Equal(t, "order-no-tok", orderID) + // No GET to the list endpoint must have happened. + m.AssertNotCalled(t, "Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet + })) +} + +// TestDoIdempotentPurchaseTwoStep_NoMatch_FallsThroughToPurchase verifies a +// first-time purchase: the lookup returns no match, so the wrapper proceeds +// with the two-step purchase flow as normal. +func TestDoIdempotentPurchaseTwoStep_NoMatch_FallsThroughToPurchase(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + // Step 1: list call returns empty. + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet && r.URL.String() == listURL + })).Return(fakeResp(http.StatusOK, `{"value":[]}`), nil).Once() + // Step 2: calculatePrice. + 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-fresh"}}`), nil).Once() + // Step 3: purchase. + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/order-fresh/purchase" + })).Return(fakeResp(http.StatusOK, `{}`), nil).Once() + + orderID, err := DoIdempotentPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok", "fresh-tok-1") + require.NoError(t, err) + assert.Equal(t, "order-fresh", orderID) + m.AssertExpectations(t) +} + +// TestDoIdempotentPurchaseTwoStep_Match_ShortCircuits is THE invariant test +// from issue #721: a re-driven purchase with the same idempotency token MUST +// NOT issue a second calculatePrice/purchase call. The list call finds the +// existing order and the wrapper returns it directly. +func TestDoIdempotentPurchaseTwoStep_Match_ShortCircuits(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + body := orderListJSON([]struct { + Name string + IdempotencyToken string + ProvisioningState string + OtherTags map[string]string + }{ + {Name: "order-already-bought", IdempotencyToken: "redrive-tok", ProvisioningState: "Succeeded"}, + }, "") + + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet && r.URL.String() == listURL + })).Return(fakeResp(http.StatusOK, body), nil).Once() + + orderID, err := DoIdempotentPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok", "redrive-tok") + require.NoError(t, err) + assert.Equal(t, "order-already-bought", orderID) + // CRITICAL: zero POST calls -- no calculatePrice, no purchase. The + // regression test that proves issue #721 is fixed. + m.AssertNotCalled(t, "Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost + })) + m.AssertExpectations(t) +} + +// TestDoIdempotentPurchaseTwoStep_LookupFailure_DoesNotPurchase pins the +// safety contract: a failed lookup MUST NOT fall through to a purchase. If +// it did, the dedupe guard could be silently bypassed by a transient list +// failure and a re-drive would double-buy. Mirrors the EC2 safety pattern. +func TestDoIdempotentPurchaseTwoStep_LookupFailure_DoesNotPurchase(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet + })).Return(fakeResp(http.StatusInternalServerError, `{"error":"upstream down"}`), nil).Once() + + _, err := DoIdempotentPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok", "tok-failing") + require.Error(t, err) + assert.Contains(t, err.Error(), "idempotency lookup failed") + assert.Contains(t, err.Error(), "refusing to purchase") + // No POSTs at all -- the failed list must abort the whole flow. + m.AssertNotCalled(t, "Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost + })) +} + +// TestDoIdempotentPurchaseTwoStep_DifferentTokens_DistinctReservations verifies +// the dedupe key isolates per-token: two purchases with different tokens see +// independent lookup results and both purchase normally. testify/mock's +// Once()/.Return() ordering with multiple matchers on the same method is +// fragile across calls, so each purchase is exercised against its own freshly +// programmed mock to keep the per-call expectations clearly partitioned. +func TestDoIdempotentPurchaseTwoStep_DifferentTokens_DistinctReservations(t *testing.T) { + ctx := context.Background() + tok1 := common.DeriveIdempotencyToken("exec-1", 0) + tok2 := common.DeriveIdempotencyToken("exec-1", 1) + require.NotEqual(t, tok1, tok2, "test premise: derived tokens must differ by recIndex") + + runOne := func(t *testing.T, idemTok, mintedOrderID string) string { + t.Helper() + m := &mockHTTPClient{} + // Lookup: empty (no prior order for this token). + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet && r.URL.String() == listURL + })).Return(fakeResp(http.StatusOK, `{"value":[]}`), nil).Once() + // calculatePrice mints the order ID. + 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":"`+mintedOrderID+`"}}`), nil).Once() + // purchase succeeds. + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+mintedOrderID+"/purchase" + })).Return(fakeResp(http.StatusOK, `{}`), nil).Once() + + got, err := DoIdempotentPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok", idemTok) + require.NoError(t, err) + m.AssertExpectations(t) + return got + } + + orderID1 := runOne(t, tok1, "order-A") + orderID2 := runOne(t, tok2, "order-B") + assert.Equal(t, "order-A", orderID1) + assert.Equal(t, "order-B", orderID2) + assert.NotEqual(t, orderID1, orderID2, "distinct idempotency tokens must produce distinct reservations") +} + +// TestDoIdempotentPurchaseTwoStep_PreservesTwoStepFlow verifies the two-step +// flow's session-timeout retry semantics from PR #680 still work under the +// new wrapper -- the wrapper is purely additive for the lookup, and once it +// falls through to DoPurchaseTwoStep the original retry behaviour applies. +func TestDoIdempotentPurchaseTwoStep_PreservesTwoStepFlow(t *testing.T) { + m := &mockHTTPClient{} + ctx := context.Background() + + sessionTimeoutBody := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again"}}` + + // Lookup: no match. + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet + })).Return(fakeResp(http.StatusOK, `{"value":[]}`), nil).Once() + // calculatePrice #1. + 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":"first"}}`), nil).Once() + // purchase #1 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/first/purchase" + })).Return(fakeResp(http.StatusBadRequest, sessionTimeoutBody), nil).Once() + // calculatePrice #2 (retry). + 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":"second"}}`), nil).Once() + // purchase #2 succeeds. + m.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/second/purchase" + })).Return(fakeResp(http.StatusOK, `{}`), nil).Once() + + orderID, err := DoIdempotentPurchaseTwoStep(ctx, m, calcURL, []byte(testBody), "tok", "retry-tok") + require.NoError(t, err) + assert.Equal(t, "second", orderID) + m.AssertExpectations(t) +} diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index 6c1c95dbc..c0bf315f6 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -249,14 +249,13 @@ 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. + // Source is required so the resulting reservation is attributable to CUDly + // in the portal via the purchase-automation tag. The dedupe key for + // idempotent re-drives is now opts.IdempotencyToken (issue #721, applied + // in reservations.DoIdempotentPurchaseTwoStep); source remains mandatory + // for attribution. if opts.Source == "" { - result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } @@ -281,9 +280,7 @@ func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common. "renew": false, }, } - if opts.Source != "" { - requestBody["tags"] = map[string]string{common.PurchaseTagKey: opts.Source} - } + reservations.ApplyPurchaseTags(requestBody, opts.Source, opts.IdempotencyToken) bodyBytes, err := json.Marshal(requestBody) if err != nil { @@ -299,7 +296,7 @@ func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common. return result, result.Error } - reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) + reservationOrderID, err := reservations.DoIdempotentPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token, opts.IdempotencyToken) if err != nil { result.Error = err return result, result.Error diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index e865ea61f..c38f285f9 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -247,14 +247,13 @@ 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. + // Source is required so the resulting reservation is attributable to CUDly + // in the portal via the purchase-automation tag. The dedupe key for + // idempotent re-drives is now opts.IdempotencyToken (issue #721, applied + // in reservations.DoIdempotentPurchaseTwoStep); source remains mandatory + // for attribution. if opts.Source == "" { - result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } @@ -286,7 +285,7 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm "renew": false, }, } - applyPurchaseAutomationTag(requestBody, opts.Source) + reservations.ApplyPurchaseTags(requestBody, opts.Source, opts.IdempotencyToken) bodyBytes, err := json.Marshal(requestBody) if err != nil { @@ -302,7 +301,7 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm return result, result.Error } - reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) + reservationOrderID, err := reservations.DoIdempotentPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token, opts.IdempotencyToken) if err != nil { result.Error = err return result, result.Error @@ -579,14 +578,3 @@ func (c *SearchClient) convertAzureSearchRecommendation(ctx context.Context, azu return rec } - -// applyPurchaseAutomationTag attaches the purchase-automation tag to an Azure -// reservation request body when source is non-empty. Extracted out of -// PurchaseCommitment to keep the function under the cyclomatic-complexity -// threshold enforced by the pre-commit hook. -func applyPurchaseAutomationTag(body map[string]interface{}, source string) { - if source == "" { - return - } - body["tags"] = map[string]string{common.PurchaseTagKey: source} -} diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index 555147aae..6611cccee 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -251,14 +251,13 @@ 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. + // Source is required so the resulting reservation is attributable to CUDly + // in the portal via the purchase-automation tag. The dedupe key for + // idempotent re-drives is now opts.IdempotencyToken (issue #721, applied + // in reservations.DoIdempotentPurchaseTwoStep); source remains mandatory + // for attribution. if opts.Source == "" { - result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } @@ -292,7 +291,7 @@ func (c *SynapseClient) PurchaseCommitment(ctx context.Context, rec common.Recom "renew": false, }, } - applyPurchaseAutomationTag(requestBody, opts.Source) + reservations.ApplyPurchaseTags(requestBody, opts.Source, opts.IdempotencyToken) bodyBytes, err := json.Marshal(requestBody) if err != nil { @@ -308,7 +307,7 @@ func (c *SynapseClient) PurchaseCommitment(ctx context.Context, rec common.Recom return result, result.Error } - reservationOrderID, err := reservations.DoPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token) + reservationOrderID, err := reservations.DoIdempotentPurchaseTwoStep(ctx, c.httpClient, reservations.CalculatePriceURL(), bodyBytes, token.Token, opts.IdempotencyToken) if err != nil { result.Error = err return result, result.Error @@ -502,12 +501,3 @@ func (c *SynapseClient) convertSynapseRecommendation(azureRec armconsumption.Res Details: details, } } - -// applyPurchaseAutomationTag attaches the purchase-automation tag to an Azure -// reservation request body when source is non-empty. -func applyPurchaseAutomationTag(body map[string]interface{}, source string) { - if source == "" { - return - } - body["tags"] = map[string]string{common.PurchaseTagKey: source} -} diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index c17baa66b..48b726006 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -842,19 +842,11 @@ func TestGetOfferingDetails_noReservationPrice(t *testing.T) { assert.Contains(t, err.Error(), "pricing data unavailable") } -// ---- applyPurchaseAutomationTag ------------------------------------------- - -func TestApplyPurchaseAutomationTag_withSource(t *testing.T) { - body := map[string]interface{}{} - applyPurchaseAutomationTag(body, "api") - tags, ok := body["tags"].(map[string]string) - require.True(t, ok) - assert.Equal(t, "api", tags[common.PurchaseTagKey]) -} - -func TestApplyPurchaseAutomationTag_emptySource(t *testing.T) { - body := map[string]interface{}{} - applyPurchaseAutomationTag(body, "") - _, ok := body["tags"] - assert.False(t, ok) -} +// ---- reservation tag application ------------------------------------------ +// +// The per-service applyPurchaseAutomationTag helper was removed when tag +// application moved to reservations.ApplyPurchaseTags (issue #721, so the +// idempotency-token tag rides alongside the purchase-automation tag). The +// helper's contract is now exercised in providers/azure/services/internal/reservations +// package tests; the synapse-side coverage is via the executor-level +// PurchaseCommitment tests above.