fix(azure): switch 7 reservation clients to two-step calculatePrice->purchase flow (closes #677) - #680
Merged
cristim merged 6 commits intoMay 22, 2026
Conversation
…e 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.
…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
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR introduces a shared reservations helper that performs Azure’s calculatePrice→purchase two-step flow (with session-timeout retry) and refactors all reservation-based Azure service clients and their tests to delegate purchase orchestration to that helper. ChangesAzure Reservations Two-Step Purchase Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/azure/services/cache/client.go (1)
296-313:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
opts.Sourcebefore relying on tag-based dedupe.After removing the client-generated order ID, this flow only emits the purchase-automation tag when
opts.Sourceis set. IfSourceis empty, a re-drive can run a freshcalculatePrice -> purchasesequence with no stable dedupe signal, which breaks the PR’s idempotency goal. Please fail fast here or provide another mandatory idempotency key. The same invariant appears in the sibling Search, Cosmos DB, and Database clients.Suggested guard
func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, DryRun: false, Success: false, Timestamp: time.Now(), } + + if opts.Source == "" { + result.Error = fmt.Errorf("purchase source is required for idempotent Azure reservation purchases") + return result, result.Error + } termYears := 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/services/cache/client.go` around lines 296 - 313, The code currently calls applyPurchaseAutomationTag(requestBody, opts.Source) and proceeds without validating opts.Source; ensure opts.Source is non-empty before relying on tag-based dedupe by adding a guard that fails fast (returning an error in result) when opts.Source == "" so purchase flows (e.g., reservations.DoPurchaseTwoStep) cannot proceed without a stable idempotency key; update the same validation pattern where other clients use applyPurchaseAutomationTag (Search, Cosmos DB, Database) to enforce the invariant.
🧹 Nitpick comments (2)
providers/azure/services/search/client_test.go (1)
830-860: ⚡ Quick winCover the tag that now gates idempotency.
This regression test proves the two HTTP calls, but it still passes if
applyPurchaseAutomationTagstops sendingcommon.PurchaseTagKey. Since duplicate protection now depends on that tag, add a case withcommon.PurchaseOptions{Source: ...}and assert the capturedcalculatePricebody contains it. This same check would be valuable in the sibling client suites too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/services/search/client_test.go` around lines 830 - 860, The test TestSearchClient_PurchaseCommitment_TwoStepFlow must assert that the idempotency tag is sent; modify the test to call client.PurchaseCommitment with a PurchaseOptions including Source (e.g. common.PurchaseOptions{Source: "tests"}) and update the mock for the calculatePrice POST (the MockHTTP matcher used in this test) to capture and validate the request body contains the tag key/value produced by applyPurchaseAutomationTag (use common.PurchaseTagKey and the Source value) before returning the mocked response; keep existing assertions and call counts intact.providers/azure/services/managedredis/client_test.go (1)
511-529: 💤 Low valueConsider adding a test for
opts.Sourcetag injection.The
PurchaseCommitmentmethod conditionally injects tags whenopts.Sourceis non-empty (lines 273-275 in client.go), but no test verifies this behavior. Consider adding a test that captures the request body and asserts thetagsfield is present whenopts.Sourceis set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/services/managedredis/client_test.go` around lines 511 - 529, Add a new unit test (or extend TestPurchaseCommitment_Success) to verify tag injection by PurchaseCommitment when PurchaseOptions.Source is set: use the same mockHTTPClient and set PurchaseOptions{Source: "some-source"}, then have the mock's Do matcher for the purchase request capture and unmarshal the request body and assert the JSON contains a "tags" object with the expected source value; keep the existing calculatePrice stub call (calcPriceRespJSON) and response assertions (result.Success, CommitmentID, Cost) so only the purchase request matcher is enhanced to validate the tags injection by PurchaseCommitment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/azure/services/internal/reservations/purchase.go`:
- Around line 119-123: The retry backoff currently uses time.Sleep which ignores
ctx cancellation; update the retry path in purchase.go (the loop that checks
IsSessionTimeout(purchaseErr) and uses purchaseRetryDelay and
attempt/purchaseMaxAttempts) to wait on a context-aware timer instead: replace
time.Sleep(purchaseRetryDelay) with a select that either returns early on
ctx.Done() (propagating ctx.Err()) or waits on time.After(purchaseRetryDelay)
(or a time.Timer.C) before continuing, so cancellation/deadline stops the
backoff immediately.
---
Outside diff comments:
In `@providers/azure/services/cache/client.go`:
- Around line 296-313: The code currently calls
applyPurchaseAutomationTag(requestBody, opts.Source) and proceeds without
validating opts.Source; ensure opts.Source is non-empty before relying on
tag-based dedupe by adding a guard that fails fast (returning an error in
result) when opts.Source == "" so purchase flows (e.g.,
reservations.DoPurchaseTwoStep) cannot proceed without a stable idempotency key;
update the same validation pattern where other clients use
applyPurchaseAutomationTag (Search, Cosmos DB, Database) to enforce the
invariant.
---
Nitpick comments:
In `@providers/azure/services/managedredis/client_test.go`:
- Around line 511-529: Add a new unit test (or extend
TestPurchaseCommitment_Success) to verify tag injection by PurchaseCommitment
when PurchaseOptions.Source is set: use the same mockHTTPClient and set
PurchaseOptions{Source: "some-source"}, then have the mock's Do matcher for the
purchase request capture and unmarshal the request body and assert the JSON
contains a "tags" object with the expected source value; keep the existing
calculatePrice stub call (calcPriceRespJSON) and response assertions
(result.Success, CommitmentID, Cost) so only the purchase request matcher is
enhanced to validate the tags injection by PurchaseCommitment.
In `@providers/azure/services/search/client_test.go`:
- Around line 830-860: The test TestSearchClient_PurchaseCommitment_TwoStepFlow
must assert that the idempotency tag is sent; modify the test to call
client.PurchaseCommitment with a PurchaseOptions including Source (e.g.
common.PurchaseOptions{Source: "tests"}) and update the mock for the
calculatePrice POST (the MockHTTP matcher used in this test) to capture and
validate the request body contains the tag key/value produced by
applyPurchaseAutomationTag (use common.PurchaseTagKey and the Source value)
before returning the mocked response; keep existing assertions and call counts
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 335e0f54-1f25-44a7-95d6-7a93113af4fb
📒 Files selected for processing (16)
providers/azure/services/cache/client.goproviders/azure/services/cache/client_test.goproviders/azure/services/compute/client.goproviders/azure/services/compute/client_test.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/cosmosdb/client_test.goproviders/azure/services/database/client.goproviders/azure/services/database/client_test.goproviders/azure/services/internal/reservations/purchase.goproviders/azure/services/internal/reservations/purchase_test.goproviders/azure/services/managedredis/client.goproviders/azure/services/managedredis/client_test.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
- 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.
Member
Author
|
Addressed CodeRabbit findings in commit 86b49ef.
Tests: 320 passed across 8 azure/services packages, @coderabbitai review |
Contributor
|
Triggering the incremental review on the new commit. ✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
providers/azure/services/internal/reservations/purchase.go (1)
137-137: ⚡ Quick winPrefer
bytes.NewReaderoverstrings.NewReader(string(bodyBytes)).Converting
[]bytetostringand then toio.Readerallocates an intermediate string on every HTTP call. Usebytes.NewReader(bodyBytes)for a zero-copy alternative.♻️ Proposed fix
Update the import block:
import ( + "bytes" "context" "encoding/json"Apply to line 137 in
doCalculatePrice:- req, err := http.NewRequestWithContext(ctx, http.MethodPost, calcURL, strings.NewReader(string(bodyBytes))) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, calcURL, bytes.NewReader(bodyBytes))Apply to line 170 in
doPurchase:- req, err := http.NewRequestWithContext(ctx, http.MethodPost, purchaseURL, strings.NewReader(string(bodyBytes))) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, purchaseURL, bytes.NewReader(bodyBytes))Also applies to: 170-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/services/internal/reservations/purchase.go` at line 137, Replace the intermediate string allocation when creating the HTTP request body in doCalculatePrice and doPurchase: use bytes.NewReader(bodyBytes) instead of strings.NewReader(string(bodyBytes)); also update the import block to add "bytes" (and remove "strings" if it becomes unused) so the code compiles without the extra allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@providers/azure/services/internal/reservations/purchase.go`:
- Line 137: Replace the intermediate string allocation when creating the HTTP
request body in doCalculatePrice and doPurchase: use bytes.NewReader(bodyBytes)
instead of strings.NewReader(string(bodyBytes)); also update the import block to
add "bytes" (and remove "strings" if it becomes unused) so the code compiles
without the extra allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce3ada37-905b-494c-9f93-fc6cf030552e
📒 Files selected for processing (3)
providers/azure/services/internal/reservations/purchase.goproviders/azure/services/managedredis/client_test.goproviders/azure/services/search/client_test.go
…der(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.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
… 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.
… 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.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
cristim
added a commit
that referenced
this pull request
May 22, 2026
…oss 5 services (closes #685) Replace space-containing displayName format strings with underscore-form prefixes and pipe through SanitizeDisplayName in all 5 Azure service clients (cache, compute, cosmosdb, database, search). Fixes HTTP 400 DisplayNameInvalid errors from Azure's Reservations API when SKU names contain unusual characters. Extends each service's PurchaseCommitment test with a body-capture assertion that guards the [A-Za-z0-9_-]{1,64} allowlist going forward. The capture mocks the calculatePrice POST so it works correctly with the two-step calculatePrice->purchase flow established by #680.
Merged
4 tasks
cristim
added a commit
that referenced
this pull request
May 22, 2026
…Price (closes #685) (#686) * fix(azure/reservations): add SanitizeDisplayName helper for Azure DisplayName allowlist Azure's Reservations API rejects DisplayName values that contain characters outside [A-Za-z0-9_-] with HTTP 400 DisplayNameInvalid. Add a shared helper in services/internal/reservations that replaces non-conforming characters with '_', collapses consecutive non-conforming chars into a single '_', and truncates to 64 characters. * fix(azure): use SanitizeDisplayName + underscore-form displayName across 5 services (closes #685) Replace space-containing displayName format strings with underscore-form prefixes and pipe through SanitizeDisplayName in all 5 Azure service clients (cache, compute, cosmosdb, database, search). Fixes HTTP 400 DisplayNameInvalid errors from Azure's Reservations API when SKU names contain unusual characters. Extends each service's PurchaseCommitment test with a body-capture assertion that guards the [A-Za-z0-9_-]{1,64} allowlist going forward. The capture mocks the calculatePrice POST so it works correctly with the two-step calculatePrice->purchase flow established by #680. * feat(azure/reservations): rich self-describing displayName for purchases Extend the Azure DisplayName fix from #686 with a richer, parseable format that mirrors the AWS RI CSV's ReservationId column shape so operators can identify reservations in the Azure portal without cross-referencing the purchase audit log. Format (each segment separated by '-'): {svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand} vm -eastus -Standard_D2a_v4-1x-1yr-allup -20260522T190000-a1b2c3d4 redis -westeurope-Premium_P1 -1x-1yr-allup -20260522T190000-a1b2c3d4 cosmos -northeurope-EnableCassandra-1x-1yr-allup-20260522T190000-a1b2c3d4 sql -centralus -GP_Gen5_2 -1x-1yr-allup -20260522T190000-a1b2c3d4 search -westus2 -standard2 -1x-1yr-allup -20260522T190000-a1b2c3d4 Output is always sanitized to Azure's [A-Za-z0-9_-]{1,64} allowlist. When the composed string would exceed 64 chars (long region + long SKU + high count), the builder progressively drops the optional tail segments — random suffix first, then timestamp, then payment-option. The service code, region, SKU, count, and term are NEVER dropped: those are the high-signal segments operators rely on. With every optional segment dropped, the joined required segments are truncated as a last-resort safety net so the builder never emits >64 chars. Previous format was `VM_Reservation_<SKU>` (and equivalents per service) — operators reported these were indistinguishable across purchases. The new identifier carries enough context to identify the specific reservation purchase from the portal alone. Implementation lives in providers/azure/services/internal/reservations/displayname.go alongside the existing SanitizeDisplayName helper, and is wired into all 5 service clients (cache, compute, cosmosdb, database, search). Time and random sources are injectable so tests can pin them; tests cover happy-path, per-service shape, length-fit truncation at each drop priority (random → timestamp → payment), payment normalization, sanitization of dirty input, embedded-dash collapse, determinism, and UTC normalization. The 5 per-service allowlist regression tests are extended to also assert the service-code prefix and SKU presence so accidental swaps in the literal strings are caught. Refs #685 * fix(azure/reservations): repair progressive-drop loop in BuildDisplayName Address CodeRabbit findings on PR #686. Critical: BuildDisplayName's progressive-drop loop was broken. It called SanitizeDisplayName(candidate) inside the loop before checking the length cap. Because the sanitizer hard-truncates to 64 chars, the condition len(candidate) <= 64 was vacuously true on iteration 1 and the loop exited immediately, returning a mid-string-truncated full format instead of cleanly dropping the random suffix, timestamp, and payment segments. The existing tests passed only by coincidence (all test inputs happened to land their required segments within the first 64 bytes of the full format). Fix: check the pre-sanitized length to gate segment-dropping, and sanitize only on the exit path. Each segment is already conformant via normalizeSegment, so the in-loop sanitization was both wrong and redundant. Add three regression tests that fail against the pre-fix code: - DropLoopActuallyDropsTimestamp: forces the loop to drop both random and timestamp; asserts the exact clean output and that no timestamp digits leak in. - DropLoopActuallyDropsPayment: forces required-only fallback; asserts no payment fragment survives. - ZeroNowReplacedByWallClock: asserts a zero-value Now is replaced by time.Now() rather than emitting the placeholder "00010101T000000". Also address three minor doc/safety findings on the same file: - Clarify isAllowedDisplayNameChar godoc so the [A-Za-z0-9-] description matches reality (underscores are passed through by the caller). - Guard the zero-value Now case so production callers that forget to set it get a real timestamp instead of "00010101T000000". - Document generateRandSuffix's silent fallback for non-nil src shorter than 4 bytes. Verification: stash-revert the production code change with the new tests in place; all three regression tests fail with exactly the expected mid-truncated outputs. Restore the fix; all 68 reservations tests pass and all 620 azure-package tests pass.
This was referenced May 25, 2026
cristim
added a commit
that referenced
this pull request
May 26, 2026
…eTwoStep (#729) PR #680 (commit 65ecbf8) switched all 7 Azure reservation executors to DoPurchaseTwoStep, which mints a fresh server-side reservationOrderId per calculatePrice call and dropped the deterministic IdempotencyToken threading PR #653 had added. A re-drive of the same (executionID, recIndex) would create a SECOND Azure reservation, regressing #641's invariant. Option A (client-supplied order ID) is not viable: Azure rejects client-supplied IDs in the two-step flow (the very reason PR #680 had to adopt it). The fix implements Option B from the issue, mirroring the AWS EC2 findRIByIdempotencyToken pattern: * ApplyPurchaseTags now stamps two tags on every purchase body: purchase-automation (attribution, existing) and cudly-idempotency-token (new, the deterministic per-rec token). * FindReservationOrderByIdempotencyToken lists reservation orders via the Microsoft.Capacity REST endpoint, filters by tag, skips terminal-failed states (Cancelled/Failed/Expired), and follows nextLink pagination. * DoIdempotentPurchaseTwoStep wraps DoPurchaseTwoStep with the lookup-first/short-circuit guard. Empty token falls straight through (CLI legacy path). Failed lookups refuse to purchase rather than risk a double-buy (mirrors EC2 safety contract). All 7 service executors (cache, compute, cosmosdb, database, managedredis, search, synapse) now call DoIdempotentPurchaseTwoStep with opts.IdempotencyToken and ApplyPurchaseTags with both opts.Source and opts.IdempotencyToken. 20 new tests cover both invariants: same token produces one reservation (short-circuit), different tokens produce distinct reservations. DoPurchaseTwoStep retains its previous behaviour for callers without an owning execution. Per-service existing tests continue to pass unchanged. Closes #721, partial-restore of #641 invariant for Azure, unblocks #639.
5 tasks
cristim
added a commit
that referenced
this pull request
May 27, 2026
…rs/purchase/action (#744) * fix(iac/azure): custom role grants Microsoft.Capacity/reservationOrders/purchase/action The built-in Reservation Purchaser role lacks Microsoft.Capacity/reservationOrders/purchase/action, which the two-step calculatePrice -> purchase flow (PR #680) requires. Replace it with a custom role that explicitly enumerates the runtime actions used by the reservation + savings-plan purchase paths, assignable at subscription scope AND /providers/Microsoft.Capacity. Fixes 403 AuthorizationFailed on live reservation purchases for newly onboarded Azure subscriptions. Closes #731 (if open). * fix(iac/azure): mirror reservation-purchaser custom role on host SP + add Reader The host container-app identity was still on the built-in Reservation Purchaser role (which omits reservationOrders/purchase/action, the same bug this PR fixes customer-side). Adds the same custom role plus a subscription-scope Reader, factoring the role definition into a shared module so customer + host stay in lockstep. - New module: terraform/modules/iam/azure/cudly-reservation-role (main.tf / variables.tf / outputs.tf) - single source of truth for the custom role's action list - iac/federation/azure-target/terraform/main.tf: switch from inlined azurerm_role_definition to the shared module (no behavioural change) - terraform/modules/compute/azure/container-apps/main.tf: replace built-in Reservation Purchaser assignment with custom-role module + add subscription-scope Reader for host-account resource enumeration Refs #731.
This was referenced May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
400 BadRequest "Session timed out - Call CalculatePrice again"error that affects newer Azure SKU families (e.g.,Standard_B2ats_v2, Burstable v2 series) when purchasing reservationsPUT reservationOrders/{client-generated-id}pattern with Azure's required two-step flow:POST calculatePriceto mint an Azure-assignedreservationOrderId, thenPOST reservationOrders/{azure-id}/purchaseproviders/azure/services/internal/reservations(new package) used by all 7 affected clientsServices changed (7)
compute,database,cache,search,cosmosdb,managedredis,synapseImplementation notes
Two-step flow:
calculatePricemints a session-boundreservationOrderId;purchasecommits using that Azure-assigned ID. On"Session timed out"from the purchase endpoint,calculatePriceis re-run from scratch (up to 3 attempts). Other 4xx/5xx errors return immediately.Idempotency (Option B): Azure now mints the order ID, so client-supplied UUIDs are no longer viable. Caller-level deduplication uses the
purchase-automationtag already attached to each reservation request body. No schema change required.Lambda budget (#667):
calculatePrice(~1-2 s) +purchase(~2-5 s) + auth (~1 s) = ~8 s worst-case per purchase, well within the 60 s Lambda limit.API version: Both endpoints pinned to
2022-11-01(last stable GA version).github.com/google/uuiddependency removed from all 7 clients.Test plan
cd providers/azure && go test ./... -count=1- 530 tests pass,go vetcleanTestDoPurchaseTwoStep_SessionTimeoutThenSuccess)TestPurchaseCommitment_withSourceverifies automation tag appears in calculatePrice request bodyCloses #677
Refs #641, #667, #632, #669, #671
Summary by CodeRabbit
New Features
Bug Fixes
Tests