From 77aec79a039c1f62ccee00ea49e40c56640f2c72 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 20:04:15 +0300 Subject: [PATCH] fix(aws/savingsplans): purchase EC2Instance SP for the recommended family/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 --- pkg/common/types.go | 18 ++ providers/aws/recommendations/parser_sp.go | 31 ++ .../aws/recommendations/parser_sp_test.go | 105 +++++++ providers/aws/services/savingsplans/client.go | 274 +++++++++++++----- .../aws/services/savingsplans/client_test.go | 196 +++++++++++++ 5 files changed, 558 insertions(+), 66 deletions(-) diff --git a/pkg/common/types.go b/pkg/common/types.go index 9a494611d..fc1fd9e18 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -552,6 +552,24 @@ type SavingsPlanDetails struct { PlanType string `json:"plan_type"` // Compute, EC2Instance, SageMaker HourlyCommitment float64 `json:"hourly_commitment"` Coverage string `json:"coverage,omitempty"` + // InstanceFamily is the EC2 instance family recommended by Cost Explorer + // (e.g. "m5"). Only populated for EC2Instance Savings Plans; empty for + // Compute, SageMaker and Database plans which are family-agnostic. Used at + // purchase time to add an instanceFamily filter to DescribeSavingsPlansOfferings + // so the offering resolves to the correct family rather than the + // lexicographically-smallest across all families in the region. + InstanceFamily string `json:"instance_family,omitempty"` + // Region is the AWS region recommended by Cost Explorer for this EC2Instance + // Savings Plan (e.g. "us-east-1"). Only populated for EC2Instance plans; + // Compute, SageMaker and Database plans are global and carry no region. + // Used at purchase time as the region filter for DescribeSavingsPlansOfferings + // instead of the client's configured region, which may differ. + Region string `json:"region,omitempty"` + // OfferingID is the exact Savings Plans offering ID returned by Cost Explorer + // in SavingsPlansDetails.OfferingId. When non-empty the purchase path uses it + // directly and skips DescribeSavingsPlansOfferings entirely, which is the + // safest possible resolution. Only populated when CE provides it. + OfferingID string `json:"offering_id,omitempty"` } func (d SavingsPlanDetails) GetServiceType() ServiceType { diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 41b865eaf..ea3e852a6 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -194,6 +194,30 @@ func parseOptionalFloat(field string, s *string) float64 { // hoursPerMonth is the standard AWS billing constant for monthly cost calculations. const hoursPerMonth = 730.0 +// ec2SPFields holds the Cost Explorer SavingsPlansDetails fields that are +// specific to EC2Instance Savings Plans. Extracted from parseSavingsPlanDetail +// to keep that function's cyclomatic complexity under the gocyclo-10 threshold. +type ec2SPFields struct { + instanceFamily string + region string + offeringID string +} + +// extractEC2SPFields reads InstanceFamily, Region, and OfferingId from the +// CE SavingsPlansDetails nested struct for EC2Instance plan recommendations. +// Returns a zero-value struct (all empty strings) for non-EC2Instance plan +// types and when SavingsPlansDetails is nil, so callers do not need a nil guard. +func extractEC2SPFields(planType types.SupportedSavingsPlansType, detail *types.SavingsPlansPurchaseRecommendationDetail) ec2SPFields { + if planType != types.SupportedSavingsPlansTypeEc2InstanceSp || detail.SavingsPlansDetails == nil { + return ec2SPFields{} + } + return ec2SPFields{ + instanceFamily: aws.ToString(detail.SavingsPlansDetails.InstanceFamily), + region: aws.ToString(detail.SavingsPlansDetails.Region), + offeringID: aws.ToString(detail.SavingsPlansDetails.OfferingId), + } +} + func (c *Client) parseSavingsPlanDetail( detail *types.SavingsPlansPurchaseRecommendationDetail, params common.RecommendationParams, @@ -244,6 +268,10 @@ func (c *Client) parseSavingsPlanDetail( accountID = aws.ToString(detail.AccountId) } + // Extract EC2Instance-specific routing data from CE's SavingsPlansDetails. + // See extractEC2SPFields for the extraction logic and field semantics. + ec2Fields := extractEC2SPFields(planType, detail) + // RecurringMonthlyCost is the portion of the SP commitment that appears // on monthly bills (i.e. excludes upfront payments). // @@ -286,6 +314,9 @@ func (c *Client) parseSavingsPlanDetail( PlanType: planTypeStr, HourlyCommitment: hourlyCommitment, Coverage: fmt.Sprintf("%.1f%%", savingsPercent), + InstanceFamily: ec2Fields.instanceFamily, + Region: ec2Fields.region, + OfferingID: ec2Fields.offeringID, }, } } diff --git a/providers/aws/recommendations/parser_sp_test.go b/providers/aws/recommendations/parser_sp_test.go index f896d427c..1a16d56b5 100644 --- a/providers/aws/recommendations/parser_sp_test.go +++ b/providers/aws/recommendations/parser_sp_test.go @@ -203,3 +203,108 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) { }) } } + +// TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured is the C1 parser +// regression test. It asserts that parseSavingsPlanDetail persists InstanceFamily, +// Region, and OfferingID from CE's SavingsPlansDetails onto the recommendation so +// the purchase path can use them. Pre-fix: the field was ignored; assertions on +// InstanceFamily/Region/OfferingID would fail. Post-fix: the fields are captured. +func TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured(t *testing.T) { + client := &Client{} + params := common.RecommendationParams{ + Service: common.ServiceSavingsPlansEC2Instance, + PaymentOption: "no-upfront", + Term: "1yr", + } + + tests := []struct { + name string + detail *types.SavingsPlansPurchaseRecommendationDetail + planType types.SupportedSavingsPlansType + wantFamily string + wantRegion string + wantOfferingID string + }{ + { + name: "EC2Instance SP with all CE fields populated", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("0.50"), + EstimatedMonthlySavingsAmount: aws.String("30.00"), + EstimatedSavingsPercentage: aws.String("20.0"), + SavingsPlansDetails: &types.SavingsPlansDetails{ + InstanceFamily: aws.String("m5"), + Region: aws.String("us-east-1"), + OfferingId: aws.String("ce-offering-abc123"), + }, + }, + planType: types.SupportedSavingsPlansTypeEc2InstanceSp, + wantFamily: "m5", + wantRegion: "us-east-1", + wantOfferingID: "ce-offering-abc123", + }, + { + name: "EC2Instance SP with partial CE fields (OfferingId absent)", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("1.00"), + EstimatedMonthlySavingsAmount: aws.String("50.00"), + EstimatedSavingsPercentage: aws.String("15.0"), + SavingsPlansDetails: &types.SavingsPlansDetails{ + InstanceFamily: aws.String("c5"), + Region: aws.String("eu-west-1"), + }, + }, + planType: types.SupportedSavingsPlansTypeEc2InstanceSp, + wantFamily: "c5", + wantRegion: "eu-west-1", + wantOfferingID: "", + }, + { + name: "EC2Instance SP with nil SavingsPlansDetails (defensive case)", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("0.75"), + EstimatedMonthlySavingsAmount: aws.String("20.00"), + EstimatedSavingsPercentage: aws.String("10.0"), + SavingsPlansDetails: nil, + }, + planType: types.SupportedSavingsPlansTypeEc2InstanceSp, + wantFamily: "", + wantRegion: "", + wantOfferingID: "", + }, + { + name: "Compute SP: SavingsPlansDetails ignored — no family/region populated", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("2.00"), + EstimatedMonthlySavingsAmount: aws.String("100.00"), + EstimatedSavingsPercentage: aws.String("25.0"), + SavingsPlansDetails: &types.SavingsPlansDetails{ + // CE may return these for Compute SPs but we must not use them + // to avoid imposing a family/region constraint on a global plan. + InstanceFamily: aws.String("m5"), + Region: aws.String("us-east-1"), + }, + }, + planType: types.SupportedSavingsPlansTypeComputeSp, + wantFamily: "", // Compute is family-agnostic; must stay empty + wantRegion: "", // Compute is global; must stay empty + wantOfferingID: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec := client.parseSavingsPlanDetail(tt.detail, params, tt.planType) + require.NotNil(t, rec) + + spDetails, ok := rec.Details.(*common.SavingsPlanDetails) + require.True(t, ok, "Details must be *common.SavingsPlanDetails") + + assert.Equal(t, tt.wantFamily, spDetails.InstanceFamily, + "InstanceFamily must be captured from CE SavingsPlansDetails for EC2Instance SPs") + assert.Equal(t, tt.wantRegion, spDetails.Region, + "Region must be captured from CE SavingsPlansDetails for EC2Instance SPs") + assert.Equal(t, tt.wantOfferingID, spDetails.OfferingID, + "OfferingID must be captured from CE SavingsPlansDetails when present") + }) + } +} diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index 4cea975ee..bc80f0c30 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -285,9 +285,16 @@ func (c *Client) resolveSPPlanType(spPlanType string) (types.SavingsPlanType, er return c.planType, nil } -// buildSPOfferingsInput constructs the DescribeSavingsPlansOfferings request, -// adding a region filter for EC2Instance plans when the client region is known. -func (c *Client) buildSPOfferingsInput(planType types.SavingsPlanType, termSeconds int64, paymentOption types.SavingsPlanPaymentOption, tag string) *savingsplans.DescribeSavingsPlansOfferingsInput { +// buildSPOfferingsInput constructs the DescribeSavingsPlansOfferings request. +// For EC2Instance plans it adds region and instanceFamily filters using the +// recommendation's own region/family (from CE SavingsPlansDetails). recRegion +// and instanceFamily are ignored for Compute, SageMaker, and Database plans +// which are global and carry no region or family property. +// +// Supported filter attributes for DescribeSavingsPlansOfferings: +// - SavingsPlanOfferingFilterAttributeRegion ("region") +// - SavingsPlanOfferingFilterAttributeInstanceFamily ("instanceFamily") +func (c *Client) buildSPOfferingsInput(planType types.SavingsPlanType, termSeconds int64, paymentOption types.SavingsPlanPaymentOption, instanceFamily, recRegion, tag string) *savingsplans.DescribeSavingsPlansOfferingsInput { input := &savingsplans.DescribeSavingsPlansOfferingsInput{ PlanTypes: []types.SavingsPlanType{planType}, Durations: []int64{termSeconds}, @@ -295,24 +302,109 @@ func (c *Client) buildSPOfferingsInput(planType types.SavingsPlanType, termSecon // Pin to USD so non-USD currency offerings are excluded server-side. Currencies: []types.CurrencyCode{types.CurrencyCodeUsd}, } - // EC2Instance SPs are region-scoped; add a region filter so only the - // offering for the client's region is returned. Compute, SageMaker, and - // Database SPs are global and do not carry a region property. + // EC2Instance SPs are region-scoped and family-scoped. Apply both filters + // using the recommendation's own values from CE (recRegion, instanceFamily) + // rather than the client's configured region, which may differ from the + // region CE recommended. Compute, SageMaker, and Database SPs are global + // and do not carry region or family properties. if planType == types.SavingsPlanTypeEc2Instance { - if c.region == "" { - log.Printf("purchase[%s]: SavingsPlans findOfferingID: client region is empty; skipping region filter", tag) - } else { - input.Filters = []types.SavingsPlanOfferingFilterElement{ - { - Name: types.SavingsPlanOfferingFilterAttributeRegion, - Values: []string{c.region}, - }, + filterRegion := recRegion + if filterRegion == "" { + // Fall back to the client's region if CE did not supply one. Log so + // operators can detect when the recommendation is missing the field. + filterRegion = c.region + if filterRegion != "" { + log.Printf("purchase[%s]: SavingsPlans buildSPOfferingsInput: rec has no region; falling back to client region %s", tag, filterRegion) } } + + var filters []types.SavingsPlanOfferingFilterElement + if filterRegion != "" { + filters = append(filters, types.SavingsPlanOfferingFilterElement{ + Name: types.SavingsPlanOfferingFilterAttributeRegion, + Values: []string{filterRegion}, + }) + } else { + log.Printf("purchase[%s]: SavingsPlans buildSPOfferingsInput: EC2Instance SP has no region in rec and client has no region; skipping region filter", tag) + } + if instanceFamily != "" { + filters = append(filters, types.SavingsPlanOfferingFilterElement{ + Name: types.SavingsPlanOfferingFilterAttributeInstanceFamily, + Values: []string{instanceFamily}, + }) + } + if len(filters) > 0 { + input.Filters = filters + } } return input } +// spOffering holds the offering ID and the Properties-extracted instance family +// and region for a single DescribeSavingsPlansOfferings result. Used by the +// EC2Instance strict lookup to validate that the surviving offerings are +// unambiguous before committing to one. +type spOffering struct { + id string + instanceFamily string + region string +} + +// decodeOfferingProps converts a DescribeSavingsPlansOfferings result entry +// into an spOffering by extracting the OfferingId and reading the Properties +// slice for the instance-family and region values. Extracted to keep +// collectSPOfferings under the gocyclo-10 threshold. +func decodeOfferingProps(o types.SavingsPlanOffering) (spOffering, bool) { + if o.OfferingId == nil { + return spOffering{}, false + } + off := spOffering{id: *o.OfferingId} + for _, p := range o.Properties { + switch p.Name { + case types.SavingsPlanOfferingPropertyKeyInstanceFamily: + off.instanceFamily = aws.ToString(p.Value) + case types.SavingsPlanOfferingPropertyKeyRegion: + off.region = aws.ToString(p.Value) + } + } + return off, true +} + +// collectSPOfferings paginates DescribeSavingsPlansOfferings and returns every +// result as an spOffering (with Properties decoded). The caller validates the +// result set's consistency and picks the appropriate offering ID. +func (c *Client) collectSPOfferings(ctx context.Context, input *savingsplans.DescribeSavingsPlansOfferingsInput) ([]spOffering, error) { + t0 := time.Now() + page := 0 + var offerings []spOffering + for { + if err := ctx.Err(); err != nil { + return nil, err + } + page++ + if page > maxOfferingPages { + return nil, fmt.Errorf("pagination cap reached after %d pages for Savings Plans offering lookup (issue #688)", maxOfferingPages) + } + result, err := c.client.DescribeSavingsPlansOfferings(ctx, input) + if err != nil { + log.Printf("purchase[SavingsPlans]: DescribeSavingsPlansOfferings page %d failed after %s: %v", page, time.Since(t0), err) + return nil, fmt.Errorf("failed to describe Savings Plans offerings: %w", err) + } + log.Printf("purchase[SavingsPlans]: DescribeSavingsPlansOfferings page %d returned %d results in %s", + page, len(result.SearchResults), time.Since(t0)) + for _, o := range result.SearchResults { + if off, ok := decodeOfferingProps(o); ok { + offerings = append(offerings, off) + } + } + if result.NextToken == nil || aws.ToString(result.NextToken) == "" { + break + } + input.NextToken = result.NextToken + } + return offerings, nil +} + // findOfferingID finds the appropriate Savings Plans offering ID. // execID is the purchase execution UUID for log correlation; pass "" when // calling outside of a purchase flow (ValidateOffering, GetOfferingDetails). @@ -322,6 +414,19 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, return "", fmt.Errorf("invalid service details for Savings Plans") } + tag := execID + if tag == "" { + tag = "no-exec" + } + + // If CE supplied an exact offering ID, use it directly — this is the + // safest path because it bypasses DescribeSavingsPlansOfferings filtering + // entirely and always resolves to exactly the workload CE recommended. + if spDetails.OfferingID != "" { + log.Printf("purchase[%s]: SavingsPlans findOfferingID: using CE-provided OfferingID %s (skipping DescribeSavingsPlansOfferings)", tag, spDetails.OfferingID) + return spDetails.OfferingID, nil + } + planType, err := c.resolveSPPlanType(spDetails.PlanType) if err != nil { return "", err @@ -335,17 +440,18 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, return "", err } - tag := execID - if tag == "" { - tag = "no-exec" - } - t0 := time.Now() log.Printf("purchase[%s]: SavingsPlans findOfferingID starting (planType=%s term=%s payment=%s)", tag, planType, rec.Term, rec.PaymentOption) - input := c.buildSPOfferingsInput(planType, termSeconds, paymentOption, tag) - offeringID, err := c.lookupOfferingID(ctx, input) + input := c.buildSPOfferingsInput(planType, termSeconds, paymentOption, spDetails.InstanceFamily, spDetails.Region, tag) + + var offeringID string + if planType == types.SavingsPlanTypeEc2Instance { + offeringID, err = c.lookupEC2OfferingIDStrict(ctx, input, spDetails.InstanceFamily, spDetails.Region) + } else { + offeringID, err = c.lookupOfferingID(ctx, input) + } if err != nil { log.Printf("purchase[%s]: SavingsPlans findOfferingID failed after %s: %v", tag, time.Since(t0), err) } else { @@ -354,6 +460,60 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, return offeringID, err } +// lookupEC2OfferingIDStrict resolves the offering ID for an EC2Instance Savings +// Plan and validates that the surviving offerings are unambiguous: all must +// belong to the same instance family and the same region. If the result set +// spans multiple families or multiple regions, or is empty, the function +// returns an error rather than silently committing to the wrong workload. +// +// expectedFamily and expectedRegion are the values from the CE recommendation; +// they are included in the error message when the result set is ambiguous so +// operators can diagnose mismatches quickly. +func (c *Client) lookupEC2OfferingIDStrict(ctx context.Context, input *savingsplans.DescribeSavingsPlansOfferingsInput, expectedFamily, expectedRegion string) (string, error) { + offerings, err := c.collectSPOfferings(ctx, input) + if err != nil { + return "", err + } + if len(offerings) == 0 { + return "", fmt.Errorf("no EC2Instance Savings Plans offerings found (expected family=%q region=%q) — cannot purchase", expectedFamily, expectedRegion) + } + + // Validate that all surviving offerings share a single instance family and + // a single region. Multiple distinct values mean the filters did not narrow + // the result set enough to identify the correct workload unambiguously. + families := make(map[string]struct{}) + regions := make(map[string]struct{}) + for _, o := range offerings { + if o.instanceFamily != "" { + families[o.instanceFamily] = struct{}{} + } + if o.region != "" { + regions[o.region] = struct{}{} + } + } + + if len(families) > 1 { + return "", fmt.Errorf( + "EC2Instance SP offering lookup returned %d distinct instance families %v (expected %q region=%q): ambiguous — refusing to purchase to avoid committing to the wrong workload", + len(families), mapKeys(families), expectedFamily, expectedRegion, + ) + } + if len(regions) > 1 { + return "", fmt.Errorf( + "EC2Instance SP offering lookup returned %d distinct regions %v (expected family=%q region=%q): ambiguous — refusing to purchase", + len(regions), mapKeys(regions), expectedFamily, expectedRegion, + ) + } + + // Sort IDs for a stable, deterministic result. + ids := make([]string, 0, len(offerings)) + for _, o := range offerings { + ids = append(ids, o.id) + } + sort.Strings(ids) + return ids[0], nil +} + // convertPlanType converts a plan type string to AWS SDK type func convertPlanType(planType string) (types.SavingsPlanType, error) { switch planType { @@ -414,56 +574,27 @@ const maxOfferingPages = 5 // truncation warning for the commitment path). const maxCommitmentPages = 100 -// lookupOfferingID performs the API call(s) to find the offering ID. -// DescribeSavingsPlansOfferings accepts PlanTypes/Durations/PaymentOptions/ -// Currencies/Filters that narrow the result set to at most a handful of -// results. All pages are accumulated so the caller receives a stable, -// lexicographically-sorted first offering rather than a result that depends -// on AWS response ordering (finding 08-L1). +// lookupOfferingID resolves the offering ID for non-EC2Instance Savings Plans +// (Compute, SageMaker, Database). These plan types are global and family-agnostic +// so no family/region ambiguity check is needed; a stable sort provides a +// deterministic tie-break when multiple offerings survive the server-side filters. +// EC2Instance SPs use lookupEC2OfferingIDStrict instead. func (c *Client) lookupOfferingID(ctx context.Context, input *savingsplans.DescribeSavingsPlansOfferingsInput) (string, error) { - t0 := time.Now() - page := 0 - var offeringIDs []string - for { - if err := ctx.Err(); err != nil { - return "", err - } - - page++ - if page > maxOfferingPages { - return "", fmt.Errorf("pagination cap reached after %d pages for Savings Plans offering lookup (issue #688)", - maxOfferingPages) - } - - result, err := c.client.DescribeSavingsPlansOfferings(ctx, input) - if err != nil { - log.Printf("purchase[SavingsPlans]: DescribeSavingsPlansOfferings page %d failed after %s: %v", - page, time.Since(t0), err) - return "", fmt.Errorf("failed to describe Savings Plans offerings: %w", err) - } - log.Printf("purchase[SavingsPlans]: DescribeSavingsPlansOfferings page %d returned %d results in %s", - page, len(result.SearchResults), time.Since(t0)) - - for _, offering := range result.SearchResults { - if offering.OfferingId == nil { - continue - } - offeringIDs = append(offeringIDs, *offering.OfferingId) - } - - if result.NextToken == nil || aws.ToString(result.NextToken) == "" { - break - } - input.NextToken = result.NextToken + offerings, err := c.collectSPOfferings(ctx, input) + if err != nil { + return "", err } - - if len(offeringIDs) == 0 { - return "", fmt.Errorf("no Savings Plans offerings found after %d page(s) (issue #688)", page) + if len(offerings) == 0 { + return "", fmt.Errorf("no Savings Plans offerings found (issue #688)") + } + ids := make([]string, 0, len(offerings)) + for _, o := range offerings { + ids = append(ids, o.id) } // Sort for a stable, deterministic tie-break when multiple offerings // survive the server-side filters. The first ID after sorting is returned. - sort.Strings(offeringIDs) - return offeringIDs[0], nil + sort.Strings(ids) + return ids[0], nil } // ValidateOffering checks if a Savings Plans offering exists @@ -560,6 +691,17 @@ func (c *Client) GetValidResourceTypes(ctx context.Context) ([]string, error) { }, nil } +// mapKeys extracts the keys from a string set (map[string]struct{}) as a +// sorted slice for deterministic error messages. +func mapKeys(m map[string]struct{}) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + // buildSavingsPlanTags returns the tag map to stamp onto a newly-created // Savings Plan. The Tags map on CreateSavingsPlanInput accepts tags at // purchase time, so no follow-up call is needed. When source is empty the diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index e52191a1e..d42441d8a 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -1302,3 +1302,199 @@ func TestLookupOfferingID_DeterministicSortAcrossPages(t *testing.T) { assert.Equal(t, "offering-aaa", id, "findOfferingID must sort across pages before selecting") } + +// --- C1 adversarial-review regression tests --- +// These tests pin the fix for the wrong-family purchase defect: an EC2Instance +// SP recommendation carries InstanceFamily and Region from Cost Explorer, and +// the purchase path must (a) filter DescribeSavingsPlansOfferings by both, and +// (b) fail loud when the result set is ambiguous (multiple families or regions). +// +// Pre-fix state: buildSPOfferingsInput applied only a region filter (from the +// client's configured region, not the rec's region), no instanceFamily filter, +// so lookupOfferingID silently picked the lexicographically-smallest offering +// across all families in the region — potentially buying the wrong workload on +// a 1-3 year commitment. + +// ec2Rec constructs an EC2Instance SP recommendation for m5/us-east-1 as CE +// would return it, including the InstanceFamily and Region fields that were +// dropped by the pre-fix parser. +func ec2Rec() common.Recommendation { + return common.Recommendation{ + ResourceType: "EC2Instance", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.SavingsPlanDetails{ + PlanType: "EC2Instance", + HourlyCommitment: 0.5, + InstanceFamily: "m5", + Region: "us-east-1", + }, + } +} + +// spOfferingWithProps constructs a SavingsPlanOffering with the given +// instanceFamily and region in its Properties slice, matching the shape the +// DescribeSavingsPlansOfferings API returns. +func spOfferingWithProps(id, family, region string) types.SavingsPlanOffering { + return types.SavingsPlanOffering{ + OfferingId: aws.String(id), + Properties: []types.SavingsPlanOfferingProperty{ + { + Name: types.SavingsPlanOfferingPropertyKeyInstanceFamily, + Value: aws.String(family), + }, + { + Name: types.SavingsPlanOfferingPropertyKeyRegion, + Value: aws.String(region), + }, + }, + } +} + +// TestEC2InstanceSP_ResolvesCorrectFamilyAndRegion is the primary C1 regression +// test. The mock is configured to only return the correct m5/us-east-1 offering +// when the request includes BOTH the instanceFamily and region filters. Pre-fix: +// findOfferingID sent no instanceFamily filter, so mock.MatchedBy wouldn't +// match and the call would be unregistered — the test would fail. Post-fix: the +// new buildSPOfferingsInput adds both filters, the mock matches, and the m5 +// offering is returned. +func TestEC2InstanceSP_ResolvesCorrectFamilyAndRegion(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{ + client: mockSP, + region: "us-west-2", // client region differs from rec region to confirm rec region wins + planType: types.SavingsPlanTypeEc2Instance, + } + + // The mock only matches when the request carries both the instanceFamily + // filter for "m5" and the region filter for "us-east-1". A request without + // the instanceFamily filter (the pre-fix path) would not match and the mock + // framework would report an unexpected call. + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, + mock.MatchedBy(func(in *savingsplans.DescribeSavingsPlansOfferingsInput) bool { + var hasFamily, hasRegion bool + for _, f := range in.Filters { + if f.Name == types.SavingsPlanOfferingFilterAttributeInstanceFamily { + for _, v := range f.Values { + if v == "m5" { + hasFamily = true + } + } + } + if f.Name == types.SavingsPlanOfferingFilterAttributeRegion { + for _, v := range f.Values { + if v == "us-east-1" { + hasRegion = true + } + } + } + } + return hasFamily && hasRegion + })). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + spOfferingWithProps("m5-offering-us-east-1", "m5", "us-east-1"), + }, + }, nil) + + id, err := client.findOfferingID(context.Background(), ec2Rec(), "test-exec") + require.NoError(t, err) + assert.Equal(t, "m5-offering-us-east-1", id, + "EC2Instance SP must resolve to the m5/us-east-1 offering, not a different family") +} + +// TestEC2InstanceSP_MultiFamilyResponseFailsLoud pins the fail-loud behaviour +// when DescribeSavingsPlansOfferings returns offerings spanning more than one +// instance family. Pre-fix: lookupOfferingID silently picked "c6g-offering-..." +// (lexicographically smallest), committing money to the wrong workload. Post-fix: +// lookupEC2OfferingIDStrict detects the ambiguity and returns an explicit error. +func TestEC2InstanceSP_MultiFamilyResponseFailsLoud(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{ + client: mockSP, + region: "us-east-1", + planType: types.SavingsPlanTypeEc2Instance, + } + + // Return two offerings with different instance families. This simulates an + // API response where the server-side filter did not narrow down to one family + // (e.g., because InstanceFamily was absent from the rec and no filter was + // applied). Pre-fix: returns "c6g-offering-..." silently. Post-fix: error. + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + spOfferingWithProps("c6g-offering-us-east-1", "c6g", "us-east-1"), + spOfferingWithProps("m5-offering-us-east-1", "m5", "us-east-1"), + }, + }, nil) + + _, err := client.findOfferingID(context.Background(), ec2Rec(), "test-exec") + require.Error(t, err, "must error when offerings span multiple instance families") + assert.Contains(t, err.Error(), "distinct instance families", + "error message must name the ambiguity so operators can diagnose it") +} + +// TestEC2InstanceSP_MultiRegionResponseFailsLoud is analogous to the +// multi-family test but for a response spanning multiple regions. Ensures that +// region ambiguity is also caught and surfaced explicitly. +func TestEC2InstanceSP_MultiRegionResponseFailsLoud(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{ + client: mockSP, + region: "us-east-1", + planType: types.SavingsPlanTypeEc2Instance, + } + + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + spOfferingWithProps("m5-offering-us-east-1", "m5", "us-east-1"), + spOfferingWithProps("m5-offering-eu-west-1", "m5", "eu-west-1"), + }, + }, nil) + + _, err := client.findOfferingID(context.Background(), ec2Rec(), "test-exec") + require.Error(t, err, "must error when offerings span multiple regions") + assert.Contains(t, err.Error(), "distinct regions", + "error message must identify region ambiguity") +} + +// TestEC2InstanceSP_CEProvidedOfferingIDUsedDirectly asserts that when Cost +// Explorer supplies an OfferingId in SavingsPlansDetails, findOfferingID uses +// it directly without calling DescribeSavingsPlansOfferings. Pre-fix: the field +// did not exist on common.SavingsPlanDetails; the lookup always went through +// DescribeSavingsPlansOfferings. Post-fix: the direct path is taken, the API +// is not called. +func TestEC2InstanceSP_CEProvidedOfferingIDUsedDirectly(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{ + client: mockSP, + region: "us-east-1", + planType: types.SavingsPlanTypeEc2Instance, + } + + rec := common.Recommendation{ + ResourceType: "EC2Instance", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.SavingsPlanDetails{ + PlanType: "EC2Instance", + HourlyCommitment: 0.5, + InstanceFamily: "m5", + Region: "us-east-1", + OfferingID: "ce-provided-offering-id-abc123", + }, + } + + // DescribeSavingsPlansOfferings must NOT be called when CE supplies the ID. + mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings") + + id, err := client.findOfferingID(context.Background(), rec, "test-exec") + require.NoError(t, err) + assert.Equal(t, "ce-provided-offering-id-abc123", id, + "CE-provided OfferingID must be used directly, skipping DescribeSavingsPlansOfferings") +}