Repository navigation
fix(aws/savingsplans): purchase EC2Instance SP for the recommended family/region, fail loud on ambiguity (adversarial-review C1) - #1450
Conversation
…mily/region, fail loud on ambiguity (adversarial-review C1)
Prior to this fix, an EC2Instance Savings Plan purchase could silently commit
to the wrong instance family. The Cost Explorer SavingsPlansDetails struct
carries InstanceFamily, Region, and OfferingId per-recommendation, but
parseSavingsPlanDetail discarded all three. At purchase time,
buildSPOfferingsInput applied only a region filter (from the client's
configured region, which can differ from the rec's region), with no
instanceFamily filter, so lookupOfferingID returned the
lexicographically-smallest offering ID across ALL instance families in the
region -- potentially buying c6g when CE recommended m5 on a 1-3 year
commitment.
Fix:
- pkg/common: add InstanceFamily, Region, OfferingID fields to SavingsPlanDetails
- parser_sp.go: extract and persist those fields from detail.SavingsPlansDetails
for EC2Instance SPs only (Compute/SageMaker/Database remain family-agnostic)
- client.go (savingsplans):
* findOfferingID: when CE provides OfferingID, use it directly (safest path)
* buildSPOfferingsInput: accept instanceFamily + recRegion; apply both as
DescribeSavingsPlansOfferings filters for EC2Instance SPs, using the
rec's region (not client config region)
* new lookupEC2OfferingIDStrict: validates result set spans exactly one
family and one region; fails loud (error, no purchase) if ambiguous or empty
* Compute/SageMaker/Database SP path unchanged (lookupOfferingID)
Regression tests:
- TestEC2InstanceSP_ResolvesCorrectFamilyAndRegion: mock only matches when
both instanceFamily and region filters are present -- fails pre-fix
- TestEC2InstanceSP_MultiFamilyResponseFailsLoud: multi-family response
now errors instead of silently picking lex-smallest -- fails pre-fix
- TestEC2InstanceSP_MultiRegionResponseFailsLoud: same for multi-region
- TestEC2InstanceSP_CEProvidedOfferingIDUsedDirectly: direct OfferingID path
- TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured: 4 sub-cases
verifying parser captures all three fields -- fails pre-fix
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 58 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 (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited 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. Your next review will be available in 58 minutes. |
Money Impact
An EC2Instance Savings Plan purchase could silently commit to the wrong instance family on a 1-3 year commitment. When Cost Explorer recommended (for example) an
m5SP inus-east-1, the purchase path discarded the family/region metadata and returned the lexicographically-smallest offering ID across all instance families in the client's configured region -- potentially buying ac6gcommitment instead, or in a different region if the client region differed from the CE recommendation.This is a real-money, silent defect: the API returns 200/OK, the commitment is created, and the audit log shows "success" -- all against the wrong workload.
Root Cause
Three gaps in the code path, each independently necessary for the bug:
Parser drops CE's routing data:
parseSavingsPlanDetail(parser_sp.go) discardeddetail.SavingsPlansDetailsentirely, soInstanceFamily,Region, andOfferingIdwere never persisted ontocommon.SavingsPlanDetails.No
instanceFamilyfilter at purchase:buildSPOfferingsInputapplied only a region filter (and used the client's configured region, not the rec's region), soDescribeSavingsPlansOfferingsreturned offerings for all instance families.Silent winner selection:
lookupOfferingIDreturned the lexicographically-smallest ID from all results -- no validation that a single family/region was returned.Fix
pkg/common/types.go: AddInstanceFamily,Region,OfferingIDfields toSavingsPlanDetails(omitempty; backward-compatible).providers/aws/recommendations/parser_sp.go: Extract and persist all three fields from CE'sSavingsPlansDetailsfor EC2Instance SPs. Compute/SageMaker/Database SPs stay family-agnostic (zero values).providers/aws/services/savingsplans/client.go:findOfferingID: when CE providesOfferingID, use it directly (safest path, no API call needed).buildSPOfferingsInput: applyinstanceFamily+regionfilters for EC2Instance SPs using the rec's own values from CE (not the client's configured region).lookupEC2OfferingIDStrict: collects full offering structs, extracts Properties, validates the result set spans exactly one instance family and one region -- errors loud if ambiguous or empty. Never picks silently.lookupOfferingID(Compute/SageMaker/Database path): unchanged behavior, now delegates pagination to sharedcollectSPOfferingshelper.Regression Tests (all must fail pre-fix, pass post-fix)
TestEC2InstanceSP_ResolvesCorrectFamilyAndRegion: mock only matches when request carries BOTHinstanceFamily=m5ANDregion=us-east-1filters. Pre-fix: mock never matches (no instanceFamily filter sent).TestEC2InstanceSP_MultiFamilyResponseFailsLoud: multi-family API response now errors; pre-fix silently returned the lex-smallest (c6g).TestEC2InstanceSP_MultiRegionResponseFailsLoud: same for multi-region ambiguity.TestEC2InstanceSP_CEProvidedOfferingIDUsedDirectly: OfferingID fast path; pre-fix the field didn't exist.TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured: 4 sub-cases verifying the parser captures all three CE fields correctly.Test plan
go build ./...inproviders/awsandpkgmodules: cleango vet ./...: cleangocyclo -over 10: clean (all functions under threshold)go test ./...inproviders/aws: 1097 passgo test ./...inpkg: 704 pass🤖 Generated with claude-flow