feat(azure/ri-exchange): find compatible offerings and execute exchange (closes #596) - #1515
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds Azure Convertible RI exchange pricing and execution through provider LRO operations and authenticated API endpoints. It adds request validation, subscription and source-ownership authorization, permission and payment guardrails, routing, OpenAPI contracts, and comprehensive tests. ChangesAzure RI Exchange Pricing and Execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/api/openapi.yaml (1)
724-731: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the server-side item cap with
maxItems: 50.The handler rejects more than
maxAzureExchangeItems(50) sources/targets; the spec currently advertises unbounded arrays, so clients only learn the limit from a 400. Checkov flags the same gap.📘 Proposed spec change (repeat for both endpoints)
sources: type: array + maxItems: 50 items: type: object targets: type: array + maxItems: 50 items: type: objectAlso applies to: 776-783
🤖 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 `@internal/api/openapi.yaml` around lines 724 - 731, Add maxItems: 50 to both sources and targets array schemas in the affected OpenAPI endpoint definitions, matching the server-side maxAzureExchangeItems limit. Apply the constraint consistently to both occurrences while preserving their existing array item schemas.Source: Linters/SAST tools
internal/api/handler_ri_exchange.go (2)
569-588: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against a nil
previewbefore dereferencing.
checkAzureExchangeMoneyGuardrailsdereferencespreviewunconditionally. The provider returns a non-nil preview on success today, butazureExchangeClientis an injectable interface, so a(nil, nil, nil)return panics on the money path instead of failing closed.🛡️ Proposed guard
func checkAzureExchangeMoneyGuardrails(preview *azurecompute.ExchangePreview, maxRat *big.Rat, currency string) error { + if preview == nil { + return NewClientError(422, "Azure did not return a pricing preview; refusing to execute") + } if len(preview.PolicyErrors) > 0 {Note the same nil
previewwould also be dereferenced at Line 647 (preview.SessionID) and Lines 660-661.🤖 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 `@internal/api/handler_ri_exchange.go` around lines 569 - 588, Update checkAzureExchangeMoneyGuardrails to reject a nil preview before accessing PolicyErrors, NetPayable, or any other fields, returning an appropriate client/error response so the exchange fails closed. Also guard the money-path uses of preview.SessionID and related fields in the surrounding exchange flow to prevent nil dereferences from an injectable azureExchangeClient.
269-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFile now exceeds the 500-line limit.
This addition pushes
handler_ri_exchange.gowell past 500 lines. Consider splitting the Azure exchange request/response types, validation, and handlers into a dedicated file (e.g.handler_ri_exchange_azure.go) within the same package.As per coding guidelines: "Follow Domain-Driven Design with bounded contexts, keep files under 500 lines, and use typed interfaces for public APIs."
🤖 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 `@internal/api/handler_ri_exchange.go` around lines 269 - 677, handler_ri_exchange.go now exceeds the 500-line limit because of the Azure exchange implementation. Move the Azure-specific request/response types, validation and conversion helpers, authorization/guardrail functions, error mapping, and Handler methods such as getAzureCompatibleOfferings and executeAzureExchange into a dedicated same-package file named handler_ri_exchange_azure.go, preserving behavior and symbol visibility.Source: Coding guidelines
🤖 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 `@internal/api/handler_ri_exchange.go`:
- Around line 569-588: Update checkAzureExchangeMoneyGuardrails to reject a nil
preview before accessing PolicyErrors, NetPayable, or any other fields,
returning an appropriate client/error response so the exchange fails closed.
Also guard the money-path uses of preview.SessionID and related fields in the
surrounding exchange flow to prevent nil dereferences from an injectable
azureExchangeClient.
- Around line 269-677: handler_ri_exchange.go now exceeds the 500-line limit
because of the Azure exchange implementation. Move the Azure-specific
request/response types, validation and conversion helpers,
authorization/guardrail functions, error mapping, and Handler methods such as
getAzureCompatibleOfferings and executeAzureExchange into a dedicated
same-package file named handler_ri_exchange_azure.go, preserving behavior and
symbol visibility.
In `@internal/api/openapi.yaml`:
- Around line 724-731: Add maxItems: 50 to both sources and targets array
schemas in the affected OpenAPI endpoint definitions, matching the server-side
maxAzureExchangeItems limit. Apply the constraint consistently to both
occurrences while preserving their existing array item schemas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a397985-0e28-4fe2-9e15-3a81825a1f1d
📒 Files selected for processing (8)
internal/api/handler.gointernal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_test.gointernal/api/openapi.yamlinternal/api/router.goproviders/azure/services/compute/client.goproviders/azure/services/compute/exchange_operations.goproviders/azure/services/compute/exchange_operations_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Ready for human merge (closes #596). Money path.
E2E limitation (stated in the diff): no Azure sandbox tenant, so verification is mock-driven + this adversarial review + your merge. Follow-ups filed: #1517 (frontend UI wiring), #1518 (exchange audit-trail persistence, pre-existing on AWS+Azure), plus a new small auth-hardening issue for a pre-existing negative- Held for your merge. |
Recovered stranded work: Azure exchange billing-scope authorization fixThe review agent working this PR was interrupted mid-flight and left this change committed-but-unpushed plus uncommitted on disk. Recovering it here rather than re-implementing. Finding: Fix: derive the billing scope from Verification: Note this PR was UNSTABLE before this push; a CI watcher is armed on the new commit and I will report what it finds. @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/api/handler_ri_exchange_test.go (2)
2537-2556: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment is attached to the wrong test.
The
TestCheckAzureExchangeMoneyGuardrails_NilPreviewRefusedparagraph (Lines 2537-2540) sits directly above..._CurrencyCaseInsensitive, so the nil-preview test at Line 2552 is undocumented andgo doc/IDE hover shows the wrong rationale.♻️ Move the comment above its test
-// TestCheckAzureExchangeMoneyGuardrails_NilPreviewRefused pins the -// defensive nil check: a future azureExchangeClient implementation that -// mistakenly returns (nil, nil, nil) from CalculateExchange must not panic -// this handler. // TestCheckAzureExchangeMoneyGuardrails_CurrencyCaseInsensitive pins the // currency comparison to the same case-insensitive rule // checkAzureExecuteConstraints uses for its isUSD test. With an exact-match // comparison a request of "usd" took the USD cap path there and was then // always rejected here as a mismatch against Azure's "USD", making a // well-formed request permanently unexecutable. func TestCheckAzureExchangeMoneyGuardrails_CurrencyCaseInsensitive(t *testing.T) { preview := &azurecompute.ExchangePreview{SessionID: "sess-1", NetPayable: toPtr(10.00), NetPayableCurrency: "USD"} require.NoError(t, checkAzureExchangeMoneyGuardrails(preview, big.NewRat(100, 1), "usd")) } +// TestCheckAzureExchangeMoneyGuardrails_NilPreviewRefused pins the +// defensive nil check: a future azureExchangeClient implementation that +// mistakenly returns (nil, nil, nil) from CalculateExchange must not panic +// this handler. func TestCheckAzureExchangeMoneyGuardrails_NilPreviewRefused(t *testing.T) {🤖 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 `@internal/api/handler_ri_exchange_test.go` around lines 2537 - 2556, Move the nil-preview defensive-check comment so it directly precedes TestCheckAzureExchangeMoneyGuardrails_NilPreviewRefused, and keep the currency-comparison rationale directly above TestCheckAzureExchangeMoneyGuardrails_CurrencyCaseInsensitive. Do not alter the test implementations.
1791-1791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest file has grown well past the 500-line ceiling.
This file is now ~2650 lines; the Azure exchange suite (offerings, execute, money guardrails, scoping) is cohesive enough to live in its own
handler_ri_exchange_azure_test.goin the same package. Deferable, but the file will keep growing with the follow-up frontend/audit work.As per coding guidelines, "keep files under 500 lines".
🤖 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 `@internal/api/handler_ri_exchange_test.go` at line 1791, Split the cohesive Azure exchange tests from handler_ri_exchange_test.go into a new handler_ri_exchange_azure_test.go in the same package, including the offerings, execute, money guardrails, and scoping suites. Keep shared setup and test behavior unchanged, and ensure the original file is reduced toward the 500-line guideline.Source: Coding guidelines
🤖 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 `@internal/api/handler_ri_exchange_test.go`:
- Around line 2537-2556: Move the nil-preview defensive-check comment so it
directly precedes TestCheckAzureExchangeMoneyGuardrails_NilPreviewRefused, and
keep the currency-comparison rationale directly above
TestCheckAzureExchangeMoneyGuardrails_CurrencyCaseInsensitive. Do not alter the
test implementations.
- Line 1791: Split the cohesive Azure exchange tests from
handler_ri_exchange_test.go into a new handler_ri_exchange_azure_test.go in the
same package, including the offerings, execute, money guardrails, and scoping
suites. Keep shared setup and test behavior unchanged, and ensure the original
file is reduced toward the 500-line guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1551be3-efd6-4ed0-ab5f-2d36418b6b27
📒 Files selected for processing (7)
internal/api/handler.gointernal/api/handler_purchases.gointernal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_test.gointernal/api/openapi.yamlproviders/azure/services/compute/exchange_operations.goproviders/azure/services/compute/exchange_operations_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/api/openapi.yaml
- providers/azure/services/compute/exchange_operations.go
- internal/api/handler_ri_exchange.go
|
Addressed both nitpicks from the latest review in
Verified nothing was lost in the move: 86 test functions before, 86 after. Not done, flagging rather than silently expanding scope: @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --no-ext-diff --format=fuller --stat 3de62e1
git diff --no-ext-diff 3de62e1^ 3de62e1 -- \
internal/api/handler_ri_exchange_test.go \
internal/api/handler_ri_exchange_azure_test.go
printf '\n== Test function declarations ==\n'
rg -n --glob 'handler_ri_exchange{,_azure}_test.go' \
'^func Test[A-Za-z0-9_]+\s*\(t \*testing\.T\)' internal/api
printf '\n== Package/import structure ==\n'
for f in internal/api/handler_ri_exchange_test.go internal/api/handler_ri_exchange_azure_test.go; do
if [ -f "$f" ]; then
echo "-- $f"
sed -n '1,100p' "$f"
fi
done
printf '\n== Guardrail test/comment context ==\n'
rg -n -A10 -B12 \
'TestCheckAzureExchangeMoneyGuardrails_(CurrencyCaseInsensitive|NilPreviewRefused)' \
internal/api/handler_ri_exchange_azure_test.go internal/api/handler_ri_exchange_test.go || trueLength of output: 110710 🏁 Script executed: #!/bin/bash
set -euo pipefail
before="$(mktemp)"
after="$(mktemp)"
trap 'rm -f "$before" "$after"' EXIT
git grep -hE '^func Test[A-Za-z0-9_]+[[:space:]]*\(t \*testing\.T\)' 3de62e1^ -- \
internal/api/handler_ri_exchange_test.go \
internal/api/handler_ri_exchange_azure_test.go 2>/dev/null \
| sed -E 's/^func (Test[A-Za-z0-9_]+).*/\1/' | sort -u > "$before"
git grep -hE '^func Test[A-Za-z0-9_]+[[:space:]]*\(t \*testing\.T\)' 3de62e1 -- \
internal/api/handler_ri_exchange_test.go \
internal/api/handler_ri_exchange_azure_test.go 2>/dev/null \
| sed -E 's/^func (Test[A-Za-z0-9_]+).*/\1/' | sort -u > "$after"
printf '== Test declaration counts ==\n'
printf 'before: '; wc -l < "$before"
printf 'after: '; wc -l < "$after"
printf '\n== Removed test declarations ==\n'
comm -23 "$before" "$after" || true
printf '\n== Added test declarations ==\n'
comm -13 "$before" "$after" || true
printf '\n== Duplicate test declarations after ==\n'
git grep -hE '^func Test[A-Za-z0-9_]+[[:space:]]*\(t \*testing\.T\)' 3de62e1 -- \
internal/api/handler_ri_exchange_test.go \
internal/api/handler_ri_exchange_azure_test.go 2>/dev/null \
| sed -E 's/^func (Test[A-Za-z0-9_]+).*/\1/' | sort | uniq -d
printf '\n== Whitespace errors ==\n'
git diff --check 3de62e1^ 3de62e1Length of output: 1601
The Azure suite move preserves the same-package setup, and all 86 test declarations are retained exactly with no duplicates. The two guardrail comments now each immediately precede the test they explain, and the diff is whitespace-clean. I agree that splitting the remaining AWS-focused portion of |
✅ Re-verified after post-review drift — ready for human merge (closes #596)After the earlier review, cr-watch agents pushed additional commits (billing-scope derivation, failed-terminal-status refusal, test split). Those were re-reviewed adversarially and found sound — plus one real finding, now fixed:
CI green, CodeRabbit SUCCESS. Held for your merge (money path). |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 32 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 39 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 19 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/api/openapi.yaml (2)
741-757: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the 500/502 responses these endpoints actually return.
The handler tests pin failure codes the spec omits: a transient Azure fault maps to 500 (
TestGetAzureCompatibleOfferings_TransientErrorIs500) and a source-ownership listing failure maps to 502 (TestExecuteAzureExchange_SourceOwnershipLookupFailureRefuses). Both operations should list them so generated clients handle them.📝 Suggested addition (apply to both operations)
'422': $ref: '`#/components/responses/UnprocessableEntity`' + '500': + $ref: '`#/components/responses/InternalServerError`' + '502': + $ref: '`#/components/responses/BadGateway`'plus new entries under
components.responsesmirroring the existingUnprocessableEntityshape.Same gap applies to
/api/ri-exchange/azure-instances/exchange(lines 808-824).🤖 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 `@internal/api/openapi.yaml` around lines 741 - 757, Add 500 and 502 response entries to both Azure compatible-offerings and Azure-instances exchange operations, alongside their existing responses. Define corresponding reusable entries under components.responses, mirroring the existing UnprocessableEntity response shape, so generated clients expose both failure statuses.
742-747: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider typing the 200 payloads.
Both success responses are declared as bare
type: object, thoughAzureCompatibleOfferingsResponse/AzureExecuteExchangeResponseare concrete shapes (session id, status, nullable money fields). Reusable schemas here would let the frontend follow-up generate types instead of hand-rolling them, and would document the nullable-vs-zero money semantics the handler is careful about.As per coding guidelines, "use typed interfaces for public APIs".
Also applies to: 809-814
🤖 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 `@internal/api/openapi.yaml` around lines 742 - 747, Replace the bare object schemas for the 200 responses of the Azure compatible offerings and execute exchange endpoints with reusable schemas matching AzureCompatibleOfferingsResponse and AzureExecuteExchangeResponse. Define their concrete fields, including session identifiers, status values, and nullable money fields, then reference those schemas from both success responses so generated clients preserve the documented nullable-versus-zero semantics.Source: Coding guidelines
internal/api/handler_ri_exchange_azure_test.go (1)
1-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFile is ~1740 lines, well past the 500-line ceiling.
Natural split points already exist in the section banners: validation/conversion helpers, allowed_accounts scoping + ordering, money-path guardrails, and the
#1527source-ownership suite. Shared fixtures (mockAzureExchangeOpsClient,ownsAzureSource, the body constants) would live in one file the others import from the same package.As per coding guidelines, "keep files under 500 lines".
🤖 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 `@internal/api/handler_ri_exchange_azure_test.go` around lines 1 - 100, Split the oversized Azure exchange test file into focused files under 500 lines, using the existing section banners as boundaries for validation/conversion helpers, allowed_accounts scoping and ordering, money-path guardrails, and the `#1527` source-ownership tests. Keep shared fixtures such as mockAzureExchangeOpsClient, ownsAzureSource, allowAnyAccountScope, body constants, and request builders in one shared test file so all same-package tests can reuse them.Source: Coding guidelines
providers/azure/services/compute/exchange_operations.go (1)
1-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew file is 560 lines, over the repo's 500-line ceiling.
Types (
CompatibleOffering,ExchangePreview,ExchangeResult,ExchangeTarget) and the caller/LRO plumbing split cleanly from the operation logic — e.g.exchange_types.go+exchange_operations.go— which would also keep validation/extraction helpers easier to locate.As per coding guidelines, "Follow Domain-Driven Design with bounded contexts, keep files under 500 lines, and use typed interfaces for public APIs."
🤖 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/compute/exchange_operations.go` around lines 1 - 31, Split the oversized exchange_operations.go file to keep each file under the repository’s 500-line limit. Move the public exchange data types—CompatibleOffering, ExchangePreview, ExchangeResult, and ExchangeTarget—into a focused exchange_types.go file, while retaining the CalculateExchange, ExecuteExchange, and related operation/validation helpers in exchange_operations.go.Source: Coding guidelines
🤖 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 `@internal/api/handler_ri_exchange.go`:
- Around line 817-852: Move the executeAzureExchange documentation block so it
immediately precedes the executeAzureExchange function, and keep
parseAzureExecuteRequest preceded only by its own request-parsing documentation.
Preserve the existing comment text and function behavior.
- Around line 497-511: Update targetLocations to canonicalize each target's
Location, such as by lower-casing it, before checking seen and appending to the
result. Deduplicate and populate Regions using the normalized values while
preserving the existing output behavior.
---
Nitpick comments:
In `@internal/api/handler_ri_exchange_azure_test.go`:
- Around line 1-100: Split the oversized Azure exchange test file into focused
files under 500 lines, using the existing section banners as boundaries for
validation/conversion helpers, allowed_accounts scoping and ordering, money-path
guardrails, and the `#1527` source-ownership tests. Keep shared fixtures such as
mockAzureExchangeOpsClient, ownsAzureSource, allowAnyAccountScope, body
constants, and request builders in one shared test file so all same-package
tests can reuse them.
In `@internal/api/openapi.yaml`:
- Around line 741-757: Add 500 and 502 response entries to both Azure
compatible-offerings and Azure-instances exchange operations, alongside their
existing responses. Define corresponding reusable entries under
components.responses, mirroring the existing UnprocessableEntity response shape,
so generated clients expose both failure statuses.
- Around line 742-747: Replace the bare object schemas for the 200 responses of
the Azure compatible offerings and execute exchange endpoints with reusable
schemas matching AzureCompatibleOfferingsResponse and
AzureExecuteExchangeResponse. Define their concrete fields, including session
identifiers, status values, and nullable money fields, then reference those
schemas from both success responses so generated clients preserve the documented
nullable-versus-zero semantics.
In `@providers/azure/services/compute/exchange_operations.go`:
- Around line 1-31: Split the oversized exchange_operations.go file to keep each
file under the repository’s 500-line limit. Move the public exchange data
types—CompatibleOffering, ExchangePreview, ExchangeResult, and
ExchangeTarget—into a focused exchange_types.go file, while retaining the
CalculateExchange, ExecuteExchange, and related operation/validation helpers in
exchange_operations.go.
🪄 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: bbe0c6d4-6a03-47d1-8a39-658b6e920ca9
📒 Files selected for processing (12)
internal/api/handler.gointernal/api/handler_purchases.gointernal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_azure_test.gointernal/api/handler_ri_exchange_test.gointernal/api/openapi.yamlinternal/api/router.goproviders/azure/services/compute/client.goproviders/azure/services/compute/exchange.goproviders/azure/services/compute/exchange_operations.goproviders/azure/services/compute/exchange_operations_test.goproviders/azure/services/compute/exchange_test.go
The Regions dimension of a permission constraint was matched with
containsAny, the same any-overlap rule the other dimensions use. That
rule is safe for AccountIDs, Providers and Services because a request
names exactly one value on each of them, so any-match and all-match
coincide. Regions is the one dimension a single request legitimately
spans several values on: an Azure RI exchange submits every target's
location at once via targetLocations.
The result was an authorization bypass on a money path. A caller
permitted only in eastus could attach a westus target to the exchange;
containsAny found eastus in the permitted set, returned true, and the
irreversible exchange executed for BOTH regions. "Regions limits to
specific regions" cannot mean "limits to requests that mention at least
one permitted region".
matchAllRegionsConstraint now backs that dimension: every requested
region must be permitted, comparison is case- and whitespace-
insensitive, and the empty-list semantics are unchanged (an
unconstrained permission, or a request naming no region, still
matches). Every other dimension keeps containsAny. Single-region
callers (the AWS reshape execute path, purchaseConstraintSets) are
unaffected: for a one-element request all-match and any-match are the
same test, and the case-insensitivity only widens what they match.
Alongside it, targetLocations canonicalizes each location to trimmed
lower case before de-duplicating, so the constraint set names each
target region exactly once instead of demanding a permission for two
spellings of one place ("EastUS", the casing the Azure portal shows,
and "eastus", the casing its APIs return). Target validation now
rejects a whitespace-only location as well, which previously survived
the empty check and would reach the permission check as "", a region
no permission can name.
Regression tests cover the bypass through both matchConstraints and
the real HasPermission entry point, the case and whitespace handling
on both sides, and the targetLocations normalization both directly and
through executeAzureExchange's submitted constraint set. Each fails
against the pre-fix code.
Refs #596
Pushed
|
| Caller | Regions | Effect |
|---|---|---|
handler_ri_exchange.go checkAzureExecuteConstraints |
targetLocations(...) — multi-value |
the path being fixed |
handler_ri_exchange.go AWS reshape execute |
[]string{region} — single |
unchanged; all-match == any-match at n=1 |
handler_purchases.go purchaseConstraintSets |
[]string{rec.Region} — single |
unchanged, same reason |
(service_api.go touches Regions only on the permission side of the APIPermission conversion; handler_recommendations.go's Regions is a response field, not a constraint.) For a one-element request the two rules are the same test, and the added case-insensitivity only widens what they match, so nothing that passed before fails now.
CR threads addressed
targetLocationscasing — locations are now canonicalized to trimmed lower case before the dedup, so the constraint set names each target region exactly once instead of demanding a permission for two spellings of one place (EastUS, the casing the Azure portal shows, andeastus, the casing its APIs return).executeAzureExchangedoc block — moved offparseAzureExecuteRequestand ontoexecuteAzureExchange, where godoc will attribute it correctly.
Follow-on found while reviewing the above
The targetLocations doc comment justified normalization by "the permission side matches with exact string comparison (auth.containsAny)" — no longer true once the Regions matcher became case-insensitive. Rewrote it to state the actual reason (dedup accuracy, two layers agreeing rather than one resting on the other's leniency) and to record that the raw Location is what reaches Azure, which is safe since Azure resolves either casing to the same region.
That also exposed a real gap: target validation rejected location == "" but not a whitespace-only location, which survived the check and would reach the permission check as "" after trimming — a region no permission can name. Validation is now trim-aware (shared by both the offerings and execute paths).
Nothing dismissed
Tests — each confirmed to FAIL against the pre-fix code
TestMatchConstraints_RegionsRequireEveryRequestedRegion(11 cases) — the bypass, single/multi permitted and unpermitted, permission-side and request-side casing, whitespace, both empty-list semantics, AWS-style names. Asserted throughmatchConstraints, not the new matcher directly, so re-wiring the dimension back tomatchStringListConstraintsfails these rather than leaving them vacuously green.TestHasPermission_RegionConstraintDeniesUnpermittedTargetRegion— the same bypass through the realHasPermissionentry point the handlers call.TestTargetLocations_CanonicalizesCase(6 cases) andTestExecuteAzureExchange_ConstraintRegionsAreLowerCased— normalization directly and through the constraint setexecuteAzureExchangeactually submits.TestValidateAzureOfferingsBody/target_location_is_whitespace_only.
Pre-fix failure was verified by temporarily reverting each production change and re-running, not assumed.
Gates (exit codes)
| Gate | Exit |
|---|---|
go build ./... |
0 |
go vet ./internal/... |
0 |
go test ./... |
0 |
gocyclo -over 10 -ignore "_test\.go" . (CI's exact command) |
0 |
golangci-lint run full repo, v2.10.1 (the version ci.yml pins) |
0 — 0 issues. |
pre-commit (all hooks, no --no-verify) |
0 |
One note for the record: validateAzureExchangeTargets sits at complexity exactly 10, right on the -over 10 gate boundary. My change swapped one comparison for another so the count is unchanged, but the next added branch in that function will redden pre-commit repo-wide.
@coderabbitai review
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Commit metadata and changed files =='
git show --no-ext-diff --format=fuller --stat 55f98da
git diff --check 55f98da^ 55f98da
git diff --no-ext-diff --unified=100 55f98da^ 55f98da -- \
internal/auth \
internal/api/handler_ri_exchange.go \
internal/api/handler_ri_exchange_azure_test.go \
internal/api/handler_purchases.go \
internal/api/handler_ri_exchange_test.go
printf '%s\n' '== Shared authorization symbols and all call sites =='
rg -n -C 12 \
'func .*matchConstraints|func .*matchStringListConstraints|func .*matchAllRegionsConstraint|matchConstraints\(|PermissionConstraints|\.Regions' \
internal \
-g '*.go'
printf '%s\n' '== Azure validation, normalization, and constraint construction =='
ast-grep outline internal/api/handler_ri_exchange.go \
--items all --type function,method,struct \
--match 'Azure|targetLocations|Constraint|validate' 2>/dev/null || true
rg -n -C 16 \
'targetLocations|validateAzureExchangeTargets|checkAzureExecuteConstraints|executeAzureExchange|parseAzureExecuteRequest|Location|Regions' \
internal/api/handler_ri_exchange.go \
internal/api/handler_ri_exchange_azure_test.go
printf '%s\n' '== Focused authorization and Azure regression tests =='
fd -e go . internal | rg '(auth|permission|handler_ri_exchange).*_test\.go$' | sort
rg -n -C 14 \
'TestMatchConstraints_RegionsRequireEveryRequestedRegion|TestHasPermission_RegionConstraintDeniesUnpermittedTargetRegion|TestTargetLocations_CanonicalizesCase|TestExecuteAzureExchange_ConstraintRegionsAreLowerCased|whitespace_only' \
internal -g '*_test.go'Length of output: 202152 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Exact commit patch: authorization and Azure handler =='
git diff --no-ext-diff --unified=45 55f98da^ 55f98da -- \
internal/auth/service_group.go \
internal/auth/service_group_test.go \
internal/api/handler_ri_exchange.go \
internal/api/handler_ri_exchange_azure_test.go
printf '%s\n' '== Region constraint types, validation, and persistence boundaries =='
rg -n -C 12 \
'type PermissionConstraints|Regions[[:space:]]+\[\]string|validate.*[Cc]onstraint|Constraint.*validate|constraints.*valid|PermissionConstraints' \
internal/auth \
internal/api \
-g '*.go'
printf '%s\n' '== Complete new matcher tests and permission evaluation context =='
sed -n '1410,1605p' internal/auth/service_group_test.go
sed -n '210,320p' internal/auth/service_group.go
printf '%s\n' '== Production request-side region construction (excluding tests) =='
rg -n -C 8 \
'PermissionConstraints\{|Regions:' \
internal \
-g '*.go' \
-g '!**/*_test.go'
printf '%s\n' '== All all-match helper uses and direct matcher tests =='
rg -n -C 8 \
'matchAllRegionsConstraint|matchStringListConstraints|containsAny' \
internal/auth \
-g '*.go'Length of output: 250680
The shared matcher now correctly requires every requested region to be permitted, while preserving the documented unconstrained/unspecified empty-list behavior. The production call-site check confirms the behavior change is limited to the Azure multi-target case; AWS reshape and purchase paths still submit one region per constraint set. The Azure path also correctly canonicalizes the authorization-only region set, rejects whitespace-only locations, and retains the original location for Azure. The new tests meaningfully pin the prior bypass, normalization, and handler wiring. ✏️ Learnings added
✅ Action performedReview finished.
|
|
Re-requesting review: the latest commit on this PR postdates your last review, so that review does not cover the current HEAD. Fixes landed since then were verified locally (build, tests, vet, gocyclo, and golangci-lint at the CI-pinned version), and CI on this PR is green across all workflows. Please review the current HEAD. @coderabbitai review |
|
✅ Action performedReview finished.
|
An Azure exchange mutates two sets of reservations: the ones handed back (sources) and the ones acquired (targets). Only the targets reached the Regions dimension of the execute:ri-exchange constraint check, so a caller permitted solely in eastus could POST an exchange whose source was their own subscription's westeurope Standard_M128 3-year reservation and whose target was a single eastus reservation. targetLocations returned ["eastus"], the region constraint passed, requireAzureSourceOwnership passed (the westeurope reservation IS billed to that subscription, and that gate keys on BillingScopeID rather than region), and the exchange committed: a westeurope commitment the caller was never authorized to touch was consumed and relocated to eastus, irreversibly. No other gate covered it -- ExchangeableReservation.Region was consulted by nothing. The AWS analog is safe only because AWS exchanges are same-region; cross-region is this feature's stated purpose. exchangeRegions now folds each source reservation's own region into the same normalized, de-duplicated set as the target locations, so every region the operation touches must be permitted. Source regions come from the tenant listing rather than the request body, which names only reservation ids; the listing is fetched once in authorizeAzureExchangeExecution and reused by requireAzureSourceOwnership so both source-side gates judge the same data. That listing now precedes the constraint check, since the Regions dimension cannot be assembled without it. A source whose Region Azure did not report (documented as possible for AppliedScopeType == Shared) or that is absent from the listing contributes an unknown-region sentinel instead of being dropped: dropping it would make "we do not know where this is" mean "unconstrained", the fail-open shape fixed in #1495. A permission with no Regions constraint is unaffected, matching unattributedAccountConstraint's posture on the AccountIDs dimension. Regression coverage asserts the security property rather than the constraint set as built: an eastus-only caller is denied a westeurope source, an unreported source region denies, and the sentinel does not block an unscoped caller. The first two fail against the pre-fix call site by reaching CalculateExchange.
|
An independent adversarial review confirmed the region-matcher fix already on this branch is correct, but found it covers only half the exchange. Pushed 879bc1b to close the other half. What was wrongAn Azure exchange mutates two sets of reservations: the ones handed back (sources) and the ones acquired (targets). Only the targets reached the Concrete scenario: a caller with This is an authorization-scope gap (unauthorized mutation of a foreign-region resource), not unbounded loss — Azure still requires the new commitment to be worth at least the returned reservation's prorated value, and net cash stays bounded by The fix
Empty-region trap: Regression coverage
The first two were verified to FAIL against the pre-fix call site ( Gates, all from the worktree at this commit: Follow-ups filed (out of scope here)
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="879bc1b"
printf '%s\n' '== Commit and changed files =='
git show --no-ext-diff --format=fuller --stat "$commit"
git diff --check "${commit}^" "$commit"
git diff --no-ext-diff --unified=90 "${commit}^" "$commit" -- \
internal/api/handler_ri_exchange.go \
internal/api/handler_ri_exchange_azure_test.go \
internal/auth
printf '%s\n' '== Relevant handler structure =='
ast-grep outline internal/api/handler_ri_exchange.go --items all \
--type function,method,struct \
--match 'Azure|exchangeRegions|Ownership|Constraint|authorize' 2>/dev/null || true
printf '%s\n' '== Execution authorization and region helper contexts =='
rg -n -C 24 \
'func \(h \*Handler\) (executeAzureExchange|authorizeAzureExchangeExecution|requireAzureSourceOwnership|checkAzureExecuteConstraints)|func exchangeRegions|unknown-region|targetLocations|ListExchangeableReservations|CalculateExchange|ExecuteExchange' \
internal/api/handler_ri_exchange.go
printf '%s\n' '== Models used for source-region lookup =='
rg -n -C 12 \
'type ExchangeableReservation|Region[[:space:]]+string|BillingScopeID' \
providers/azure/services/compute \
-g '*.go'
printf '%s\n' '== Focused regression tests =='
rg -n -C 22 \
'TestExecuteAzureExchange_(SourceRegionOutsidePermissionRefused|UnreportedSourceRegionRefused|UnreportedSourceRegionAllowedWithoutRegionScope)|TestExchangeRegions|exchangeRegions|unknown-region|ConstraintSetPinsAllDimensions' \
internal/api/handler_ri_exchange_azure_test.go \
internal/auth \
-g '*_test.go'
printf '%s\n' '== All production execution region constraint construction =='
rg -n -C 10 \
'checkAzureExecuteConstraints\(|PermissionConstraints\{|Regions:' \
internal/api \
-g '*.go' -g '!**/*_test.go'Length of output: 207192
The execution path now correctly lists source reservations before constraint evaluation, derives a normalized union of target and source regions, and reuses that same listing for source-ownership validation. This closes the cross-region source mutation gap without creating a fail-open path for shared-scope reservations whose source region is unreported: The added tests meaningfully pin the security property by ensuring unauthorized or unreported source regions never reach 🐇🔎 ✏️ Learnings added
✅ Action performedReview finished.
|
Folding source regions into the Regions dimension put the constraint check ahead of requireAzureSourceOwnership, and the two together formed an enumeration oracle. A caller scoped to subscription A and permitted only in eastus received two distinguishable 403s for a reservation id they do not own: an id that exists in eastus but is billed to subscription B cleared the Regions dimension and was refused by the ownership gate, while an id that does not exist -- or lives in an unpermitted region -- tripped the sentinel or the foreign region and was refused by the constraint check with a different message. The difference confirms "this reservation id exists, in one of my permitted regions, in a subscription I am not scoped to", which is exactly what requireAzureSourceOwnership's deliberately identical denials withhold. Ownership now runs immediately after the tenant listing and before the constraint check, so the whole path is as indistinguishable as the gate already was. The listing is in hand at that point, so there is no extra round trip, and executeAzureExchange no longer needs the listing handed back to it. It also sharpens the sentinel: every source reaching exchangeRegions is known-owned, so unknown-region means only "owned, but Azure reported no region" rather than doubling as "not yours" or "not real". The missing-source branch stays as a fail-closed default for any future caller without that guarantee. The existing TestRequireAzureSourceOwnership_DenialsAreIndistinguishable stays green with this bug present, because it calls the gate directly rather than driving the path. The new test drives the handler and asserts both probes return the same status and body; it fails against the previous ordering with the two messages diverging.
|
Round-2 review finding addressed in 849679b. D1 — the gate ordering was an enumeration oracleFolding the source regions into the A caller scoped to subscription
The difference confirms "this reservation id exists, in one of my permitted regions, in a subscription I am not scoped to". Fix: Test: Two existing tests whose subject is a source-side gate now legitimately return before the constraint check ever runs, so they use a Gates, all re-run at 849679b
CI on the previous commit 879bc1b was green across all five workflows; a fresh watcher is armed for this one. D2 filed, not fixed here#1656 — The PR body also now records that the @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="849679b"
printf '%s\n' '== Commit metadata and patch =='
git show --no-ext-diff --format=fuller --stat "$commit"
git diff --check "${commit}^" "$commit"
git diff --no-ext-diff --unified=100 "${commit}^" "$commit" -- \
internal/api/handler_ri_exchange.go \
internal/api/handler_ri_exchange_azure_test.go
printf '%s\n' '== Handler structure =='
ast-grep outline internal/api/handler_ri_exchange.go --items all \
--type function,method,struct \
--match 'Azure|authorize|Ownership|exchangeRegions|Constraint' 2>/dev/null || true
printf '%s\n' '== Authorization-ordering and error-path context =='
rg -n -C 30 \
'func \(h \*Handler\) (executeAzureExchange|authorizeAzureExchangeExecution|listOwnedAzureReservations|requireAzureSourceOwnership|checkAzureExecuteConstraints)|func exchangeRegions|ListExchangeableReservations|requireAzureSubscriptionScope|CalculateExchange|ExecuteExchange|unknownRegionConstraint' \
internal/api/handler_ri_exchange.go
printf '%s\n' '== Handler-level indistinguishable-denial regression coverage =='
rg -n -C 35 \
'TestExecuteAzureExchange_UnownedSourceDenialsAreIndistinguishable|newAzureExecuteSourceGateHandler|RequireAzureSourceOwnership_DenialsAreIndistinguishable|SourceRegionOutsidePermissionRefused|UnreportedSourceRegion' \
internal/api/handler_ri_exchange_azure_test.go
printf '%s\n' '== Call sites and listing implementation/interface =='
rg -n -C 12 \
'\.authorizeAzureExchangeExecution\s*\(|\.listOwnedAzureReservations\s*\(|\.requireAzureSourceOwnership\s*\(|\.checkAzureExecuteConstraints\s*\(|ListExchangeableReservations\s*\(' \
internal/api providers/azure/services/compute \
-g '*.go'Length of output: 192218 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Current head and reviewed commit =='
git rev-parse HEAD
git show -s --format='%H%n%P%n%s' 849679b
git merge-base --is-ancestor 849679b HEAD && echo '849679b-is-ancestor-of-HEAD=yes'
printf '%s\n' '== Changes since reviewed commit =='
git diff --name-status 849679b..HEAD
git diff --check 849679b..HEAD
printf '%s\n' '== Exact commit changed-file scope =='
git diff-tree --no-commit-id --name-status -r 849679b
printf '%s\n' '== Full-path authorization sequence references =='
rg -n -C 6 \
'authorizeAzureExchangeExecution|requireAzureSourceOwnership\(owned|checkAzureExecuteConstraints\(ctx.*exchangeRegions|CalculateExchange\(ctx' \
internal/api/handler_ri_exchange.goLength of output: 8494
The current HEAD is exactly ✏️ Learnings added
|
Summary
Implements the remaining two steps of Azure Convertible RI exchange parity with AWS EC2 (closes #596, closes #1527, refs #473): pricing a proposed exchange (compatible offerings) and executing it.
POST /api/ri-exchange/azure-instances/compatible-offerings(view:purchases) -- runsarmreservations.CalculateExchangeClient.BeginPostagainst the caller's source reservations and target SKUs, returns the priced candidate offerings plus a cost preview. Read-only, commits nothing.POST /api/ri-exchange/azure-instances/exchange(execute:ri-exchange, deliberately separate fromexecute:purchases) -- commits the exchange.Client-layer operations (
CalculateExchange,ExecuteExchange) live inproviders/azure/services/compute/exchange_operations.go, wrapping the asyncarmreservationsLRO APIs with an injected caller-func test seam so no real polling ever runs in tests.Deviations from the issue text (verified against the armreservations@v1.1.0 module cache)
CalculateExchangeprices a proposed purchase and requires a request body (source reservations + target SKUs/regions/quantities); it cannot be expressed as aGET ?reservation_id=query.BeginPost, notBeginDoExchange: that's the actual method name onExchangeClientin this SDK version.execute:ri-exchange, notmanage:purchases:manage:purchasesdoes not exist anywhere in this codebase's permission set (verified across everyrequirePermission/requirePermissionConstraintscall site).execute:ri-exchangeis the existing AWS execute gate, already deliberately separate fromexecute:purchasesfor the same reason (RI exchanges are financially irreversible).PossibleReservationTermValues()includes P1Y/P3Y/P5Y in this SDK version (the issue and my own initial plan assumed only P1Y/P3Y). The term whitelist is driven entirely from the SDK's own enum rather than a hardcoded list, so this required no code change -- just a corrected assumption, caught by a test that initially used an SDK-valid "invalid" term.Money-path design: the execute handler never trusts a client-supplied session
CalculateExchange's response carries aSessionIDthatExecuteExchangecommits verbatim. A design that lets the client pick which priorCalculateExchangesession to execute would let stale pricing, a race with a rate change, or a forged session bypass every guardrail below. Instead,executeAzureExchange:CalculateExchangeitself against the caller's own sources/targets (server-side re-quote, ignoring anything price-shaped the client might send).ExecuteExchangewith theSessionIDthat fresh call just returned.Guardrails enforced against the fresh quote (all 422 on violation):
PolicyErrorsblocks execution.nilNetPayableis refused rather than treated as a free exchange.NetPayableexceeding the caller's mandatorymax_payment_duecap blocks execution (both figures included in the error).execute:ri-exchangeadditionally goes throughrequirePermissionConstraints(SEC-01, issue #1141):AccountIDsfrom the resolved CloudAccount (falling back to theunattributedAccountConstraintsentinel so an unregistered subscription still fails closed),Providers/Servicesfixed to azure/compute, every target region, andMaxPurchaseAmountfrom the cap.Every one of these four guardrails has a dedicated test that fails when the guard is removed (verified by hand: temporarily disabling each check locally reproduces a test failure, then reverted -- confirmed no diff remains against the committed code).
Other fail-loud rules: no quantity/term coercion (unknown term -> 400, no P1Y fallback); nil Azure client (unregistered subscription) -> 404, not a graceful empty state (unlike the list endpoint -- these two endpoints were asked to price/execute something specific and cannot silently do nothing); typed SDK enum constants throughout; money fields are pointers end to end so an absent amount is never read as free; 4xx-vs-5xx classification via the existing
isAzureClientError.Commits
feat(azure/compute): CalculateExchange and DoExchange client operations-- provider-layer client + table-driven tests (request-builder wiring, validation, nil-properties/empty-session errors, policy-error extraction, nil-vs-zero money fields, ctx-cancel passthrough).feat(api): Azure RI exchange compatible-offerings and execute endpoints-- the two handlers, wire types, router entries, openapi.yaml.test(api): Azure exchange handler coverage-- auth fail-closed, every validation reject, and the four money-path guardrails (also folds in the golangci-lint v2.10.1 findings this branch introduced: two err-shadow fixes matching the existingexecuteExchangestyle, two godot fixes, two gocritic nits).Gate results
cd providers/azure && go build ./...cd providers/azure && go test ./services/compute/... -racego build ./...(root)go vet ./...(root)go test ./internal/api/... -racego test ./...(root, full repo)ok, exit 0gocyclo -over 10 -ignore "_test\.go" .(CI-exact invocation)golangci-lint run ./internal/api/... --new-from-rev=ab682d939(v2.10.1, CI-pinned)golangci-lint run ./services/compute/... --new-from-rev=ab682d939(v2.10.1, CI-pinned)grep -rn "&http.Client{" providers/azure/E2E limitation
No Azure sandbox tenant exists for a live end-to-end exchange. Verification is mock-driven (the table above) plus this PR being held for an independent Fable adversarial review and human merge before it ships, per the team's money-path process.
Follow-ups (filed separately, out of scope here)
Closes #596.
Summary by CodeRabbit
Source-reservation ownership scoping (closes #1527)
Added during adversarial review. Every other gate on these endpoints constrains the destination of an exchange:
allowed_accountsscope, theexecute:ri-exchangeAccountIDsconstraint, and the derived target billing scope all key offsubscription_id. The sources were validated only for a non-emptyreservation_idandquantity >= 1.Azure reservation orders are tenant-scoped, and
ListExchangeableReservationsenumerates the whole tenant by design ("the Azure Capacity exchange API operates on reservation order IDs which span subscriptions"). So a caller authorized for subscription A could name subscription B's reservation ids, hand B's commitments back, and buy the replacement into A's billing scope -- with Azure RBAC on the reservation order as the only backstop.Each source must now be billed to the authorized subscription, checked against the reservation's own
BillingScopeID(Azure: "Subscription that will be charged for purchasing Reservation", and the scope an exchange refunds it to). This is the right discriminator even forAppliedScopeType: Shared, which governs which subscriptions receive the discount rather than which one paid; anAppliedScopescheck would pass for nearly every reservation.Fails closed throughout -- a source missing from the listing, one Azure reports without a billing scope, and a failed listing call are all refused. Denials are byte-identical whether the reservation belongs to someone else or does not exist, so the gate cannot be used to enumerate reservation ids elsewhere in the tenant. Applied to the pricing endpoint too, where the same gap leaked another subscription's commitment value.
Deployment note: the gate depends on Azure populating
billingScopeIdin theReservation_Listresponse. If a tenant's API version omits it, exchanges refuse with a clear message rather than silently permitting -- correct for a money path, but worth confirming against a live tenant before the first production exchange.Source-region scoping and gate ordering (adversarial review round 2)
An Azure exchange mutates two sets of reservations: the ones handed back (sources) and the ones acquired (targets). Only the targets reached the
Regionsdimension of theexecute:ri-exchangeconstraint check, so a caller withConstraints{AccountIDs:[acct-A], Regions:["eastus"]}could POST an exchange whose source was acct-A's own westeurope reservation and whose target was in eastus: the region constraint saw only["eastus"]and passed, the ownership gate above passed (the reservation genuinely is billed to that subscription), and a westeurope commitment the caller was never authorized to touch was consumed and relocated.exchangeRegionsnow folds every source's own region into the same set, so each region the operation touches must be permitted.A source whose
RegionAzure did not report contributes anunknown-regionsentinel rather than being dropped -- dropping it would make "we do not know where this is" mean "unconstrained", the fail-open shape fixed in #1495. A permission with noRegionsconstraint is unaffected.The gates were then reordered so ownership is checked before the constraint check. With the constraint check first, the two formed an enumeration oracle: a caller scoped to subscription A and permitted only in eastus got a different 403 for an unowned reservation that exists in eastus (constraint passes, ownership refuses) than for one that does not exist or lives elsewhere (constraint refuses first). That difference confirms "this id exists, in one of my permitted regions, in a subscription I am not scoped to" -- precisely what the ownership gate's identical denials were written to withhold.
TestExecuteAzureExchange_UnownedSourceDenialsAreIndistinguishabledrives the whole handler and asserts both probes return the same body; the pre-existing unit test could not catch it, because it calls the gate directly rather than the path.Follow-ups filed (not fixed here)
PollUntilDonetimeout commits a second exchange, each half inside the cap. AWSexecuteExchangeshares the gap.AppliedScopeTypesilently forced toShared, reallocating the discount away from a deliberately Single-scoped reservation.execute:ri-exchangemissing fromadminCarvedOuts, soadmin:*bypasses every constraint above.matchStringListConstraintsremains any-match forAccountIDs/Providers/Services(latent).listExchangeableAzureRIsreturns the tenant-wide listing unscoped, a shorter path to the information these gates withhold. Pre-existing, verified byte-identical at the merge base.Note for whoever picks up #1643: the sentinel and that issue are coupled.
ExchangeableReservation.Regionis documented as possibly empty forAppliedScopeType == Shared, and per #1643 every reservation this endpoint creates is Shared -- so if that documented empty-region case is real, a region-scoped caller could be locked out of re-exchanging reservations this very endpoint minted. Likely theoretical (ARM populates location for VM reservations regardless of applied scope, since Shared governs discount application rather than capacity location), but the two should be resolved with each other in mind.