Skip to content

fix(azure/savingsplans): take the inventory resource type from the SKU, not the ARM name - #494

Merged
cristim merged 2 commits into
mainfrom
fix/492-azure-sp-resource-type
Oct 17, 2026
Merged

cristim merged 2 commits into
mainfrom
fix/492-azure-sp-resource-type

Conversation

@cristim

@cristim cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Summary

convertSavingsPlan (providers/azure/services/savingsplans/client.go) set the inventory commitment's ResourceType from the ARM resource name. That name is the last segment of the savings plan ID (GUID-shaped in the armbillingbenefits SDK examples), while recommendations use "Compute". recfilter.DuplicateChecker keys on ResourceType, so an owned plan would never suppress a repeat recommendation and the duplicate guard failed open.

  • ResourceType now comes from sku.name: Compute_Savings_Plan (the existing armSkuComputeSavingsPlan const, the only savings plan SKU in the SDK) maps to a shared computeResourceType = "Compute", which the recommendation side now also uses. The match is case-insensitive (Azure SKU names are), so one plan with different casing does not block the subscription.
  • A missing, empty or unrecognized SKU on an owned plan now fails GetExistingCommitments (fail closed, the same path as the existing empty-region and scope errors). The error names the plan ID and says to extend the SKU mapping in convertSavingsPlan.
  • sp.Name is no longer read for the type; CommitmentID still comes from sp.ID. Plans owned by other subscriptions are skipped before conversion, as on main.

Closes #492

Not verified live

As the issue says, no real SavingsPlanClient.NewListAllPager response was inspected. The SKU value and the GUID-shaped name come from the SDK model and examples. If a real response lacked sku.name, inventory now errors and purchasing stops for that subscription (fail closed) instead of silently allowing a duplicate.

Tests

  • TestOwnedPlanSuppressesComputeRecommendation: a fixture with a GUID name and sku.name: Compute_Savings_Plan, the recommendation built by the real convertBenefitRecommendation from an SDK-shaped row, through the real client and DuplicateChecker. Fails on origin/main (the recommendation passes through), passes after.
  • FailsClosedOnBadSKU: missing SKU, nil name, empty, and unknown SKU make GetExistingCommitments error; the message names the plan ID.
  • TestGetExistingCommitments_SKUMatchIsCaseInsensitive.
  • Existing fixtures that set "name": "Compute" now also carry a SKU; the happy-path test asserts "Compute".

Mutation probes (scratch copies): reading sp.Name again, an inventory-only wrong const, an unknown SKU returning nil, and accepting any SKU each fail tests.

Verification

make build, make vet, (cd providers/azure && GOWORK=off go test -race -short ./...), pinned golangci-lint v2.10.1 (0 issues), gofmt, and the commit/pre-push hooks pass. Fixture-based evidence only.

Local review: two adversarial passes by a non-author reviewer. Pass 1 asked for the plan ID in the errors, case-insensitive matching, and an end-to-end recommendation in the dedupe test (all in the second commit); pass 2 found nothing actionable.

Overlap with #491

#491 also edits this file. Its appendCoveringForeignPlan and new tests call the single-value convertSavingsPlan and use plan fixtures without a SKU, so whichever PR merges second needs a rebase plus a small compile and fixture fix (one textual hunk in appendOwnedPlans).

🤖 Generated with Claude Code

cristim and others added 2 commits October 17, 2026 02:36
…U, not the ARM name

convertSavingsPlan set ResourceType from the ARM resource name, which is
GUID-shaped, while recommendations use "Compute". recfilter.DuplicateChecker
keys on ResourceType, so an owned plan never suppressed a repeat
recommendation and the duplicate guard failed open.

ResourceType now comes from sku.name: Compute_Savings_Plan maps to the shared
computeResourceType constant used on both sides. A missing, empty or unknown
SKU returns an error, so GetExistingCommitments fails closed through the
existing "incomplete savings plan inventory" path. There is no fallback to Name.

Not verified against a live response: the SKU value comes from the
armbillingbenefits SDK examples.

Closes #492

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…U case-insensitively

SKU errors now include the plan ID and say the SKU mapping needs extending.
The SKU is matched with strings.EqualFold against the existing
armSkuComputeSavingsPlan constant, so one plan with different casing does not
block the whole subscription. The dedupe test now builds its recommendation
through convertBenefitRecommendation, so a wrong constant on either side fails it.

Refs #492

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim cristim added urgency/this-sprint Within the current sprint priority/p1 Next up; this sprint severity/high Significant harm impact/few Limited audience effort/s Hours type/bug Defect labels Oct 17, 2026
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 7 billable files and costs up to $1.75.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 21 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 71 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: b1da4f2c-3dc0-49ef-b1d0-9226ab3a7edc
📥 Commits

Reviewing files that changed from the base of the PR and between 8aec9ef and bbe16c7.

📒 Files selected for processing: 7
  • providers/azure/services/savingsplans/benefit_recommendations.go
  • providers/azure/services/savingsplans/client.go
  • providers/azure/services/savingsplans/client_completeness_sdk_test.go
  • providers/azure/services/savingsplans/client_pager_cap_test.go
  • providers/azure/services/savingsplans/client_test.go
  • providers/azure/services/savingsplans/resource_type_test.go
  • providers/azure/services/savingsplans/scope_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Independent review verdict: CLEAN at bbe16c7

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, focused on the money path. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Inventory and recommendations now emit the same type:

  • Inventory: savingsPlanResourceType (client.go:249) returns computeResourceType ("Compute", client.go:237), assigned at :267.
  • Recommendations: use the same constant (benefit_recommendations.go:239).
  • SKU matching: strings.EqualFold(sku, "Compute_Savings_Plan") (client.go:246, constant at benefit_recommendations.go:35), so SKU casing is tolerated. The ARM name (sp.Name) is no longer read.
  • Dedupe key: neither side sets Region, and both use ServiceSavingsPlansAll, so the exact, case-sensitive key (pkg/recfilter/dedupe.go:130-131) matches.

A missing or unknown SKU fails loud:

  • convertSavingsPlan errors with the plan ID (client.go:243-248).
  • appendOwnedPlans wraps that error (:193-196), so GetExistingCommitments fails.
  • There is no wildcard and no silent skip.

End to end: on an inventory error, AdjustRecommendationsForExisting returns the unfiltered recs plus the error (dedupe.go:48-51). The real-money consumer, cloud-commitments-cli, refuses to purchase on that error (cmd/multi_service.go:885-899, verified during the #459 review), so this fails closed end to end.

Other consumers: no other consumer of the savings-plan inventory ResourceType exists in pkg or providers/azure.

What was attacked and how (GOWORK=off, no Azure calls):

  • go vet ./... is clean. go test -race -count=1 ./... passes 1925 in 16 packages.
  • Fail-before: with client.go/benefit_recommendations.go reverted, Happy yields "my-savings-plan" instead of "Compute".
  • Mutation probes, both killed:
    • Reading the ARM name again fails Happy, TestOwnedPlanSuppressesComputeRecommendation and SKUMatchIsCaseInsensitive.
    • Dropping the nil-SKU guard fails FailsClosedOnBadSKU.

Not verified: against a live Azure response, as the body says.

Gate: labels are mirrored from #492 (p1). Merge once CI is green.

@cristim
cristim merged commit c4c7158 into main Oct 17, 2026
15 checks passed
@cristim
cristim deleted the fix/492-azure-sp-resource-type branch October 17, 2026 00:51
IAmJSD added a commit to IAmJSD/cloud-commitments-go that referenced this pull request Oct 17, 2026
…r return

LeanerCloud#494 made convertSavingsPlan return an error and require a SKU. Propagate
the error for plans billed to another subscription too, and give the
covering-path fixtures the Compute_Savings_Plan SKU.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/few Limited audience priority/p1 Next up; this sprint severity/high Significant harm type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(azure): savings plan inventory uses the ARM resource name as ResourceType, so the duplicate guard likely never matches

1 participant