diff --git a/providers/aws/recommendations/converters.go b/providers/aws/recommendations/converters.go index 499b5bb69..e4a7e457f 100644 --- a/providers/aws/recommendations/converters.go +++ b/providers/aws/recommendations/converters.go @@ -1,6 +1,7 @@ package recommendations import ( + "fmt" "strings" "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" @@ -28,55 +29,102 @@ func getServiceStringForCostExplorer(service common.ServiceType) string { } } -// convertPaymentOption converts payment option string to AWS type +// convertPaymentOption converts payment option string to AWS type. +// +// Deprecated: this function silently defaults to NoUpfront for unrecognized +// values and is retained only for the legacy RI recommendation-fetch path in +// client.go (owned by #865/#1075). New callers must use convertPaymentOptionE +// and propagate the error. See open-questions/fix-aws-converters.md OQ-1. func convertPaymentOption(option string) types.PaymentOption { + v, _ := convertPaymentOptionE(option) + return v +} + +// convertPaymentOptionE converts a payment option string to the AWS Cost Explorer +// typed enum, returning an error for any value not in the known set. +// Known values: "all-upfront", "partial-upfront", "no-upfront". +func convertPaymentOptionE(option string) (types.PaymentOption, error) { switch option { case "all-upfront": - return types.PaymentOptionAllUpfront + return types.PaymentOptionAllUpfront, nil case "partial-upfront": - return types.PaymentOptionPartialUpfront + return types.PaymentOptionPartialUpfront, nil case "no-upfront": - return types.PaymentOptionNoUpfront + return types.PaymentOptionNoUpfront, nil default: - return types.PaymentOptionNoUpfront + return "", fmt.Errorf("unsupported payment option %q: must be one of all-upfront, partial-upfront, no-upfront", option) } } -// convertTermInYears converts term string to AWS type -func convertTermInYears(term string) types.TermInYears { - if term == "3yr" || term == "3" { - return types.TermInYearsThreeYears +// convertTermInYearsE converts a term string to the AWS Cost Explorer typed enum, +// returning an error for any value not in the known set. +// Known values: "1yr", "1", "3yr", "3". +func convertTermInYearsE(term string) (types.TermInYears, error) { + switch term { + case "1yr", "1": + return types.TermInYearsOneYear, nil + case "3yr", "3": + return types.TermInYearsThreeYears, nil + default: + return "", fmt.Errorf("unsupported term %q: must be one of 1yr, 1, 3yr, 3", term) } - return types.TermInYearsOneYear } -// convertLookbackPeriod converts lookback period string to AWS type -func convertLookbackPeriod(period string) types.LookbackPeriodInDays { +// convertTermInYears converts term string to AWS type. +// +// Deprecated: this function silently defaults to OneYear for unrecognized +// values and is retained only for the legacy RI recommendation-fetch path in +// client.go (owned by #865/#1075). New callers must use convertTermInYearsE +// and propagate the error. See open-questions/fix-aws-converters.md OQ-1. +func convertTermInYears(term string) types.TermInYears { + v, _ := convertTermInYearsE(term) + return v +} + +// convertLookbackPeriodE converts a lookback period string to the AWS Cost Explorer +// typed enum, returning an error for any value not in the known set. +// Known values: "7d", "7", "30d", "30", "60d", "60". +func convertLookbackPeriodE(period string) (types.LookbackPeriodInDays, error) { switch period { case "7d", "7": - return types.LookbackPeriodInDaysSevenDays + return types.LookbackPeriodInDaysSevenDays, nil case "30d", "30": - return types.LookbackPeriodInDaysThirtyDays + return types.LookbackPeriodInDaysThirtyDays, nil case "60d", "60": - return types.LookbackPeriodInDaysSixtyDays + return types.LookbackPeriodInDaysSixtyDays, nil default: - return types.LookbackPeriodInDaysSevenDays + return "", fmt.Errorf("unsupported lookback period %q: must be one of 7d, 30d, 60d", period) } } -// convertSavingsPlansPaymentOption converts payment option for Savings Plans -func convertSavingsPlansPaymentOption(option string) types.PaymentOption { - return convertPaymentOption(option) +// convertLookbackPeriod converts lookback period string to AWS type. +// +// Deprecated: this function silently defaults to SevenDays for unrecognized +// values and is retained only for the legacy RI recommendation-fetch path in +// client.go (owned by #865/#1075). New callers must use convertLookbackPeriodE +// and propagate the error. See open-questions/fix-aws-converters.md OQ-1. +func convertLookbackPeriod(period string) types.LookbackPeriodInDays { + v, _ := convertLookbackPeriodE(period) + return v +} + +// convertSavingsPlansPaymentOption converts payment option for Savings Plans, +// returning an error for unrecognized values. This is the fail-loud variant +// used by the SP recommendation path. +func convertSavingsPlansPaymentOption(option string) (types.PaymentOption, error) { + return convertPaymentOptionE(option) } -// convertSavingsPlansTermInYears converts term for Savings Plans -func convertSavingsPlansTermInYears(term string) types.TermInYears { - return convertTermInYears(term) +// convertSavingsPlansTermInYears converts term for Savings Plans, +// returning an error for unrecognized values. +func convertSavingsPlansTermInYears(term string) (types.TermInYears, error) { + return convertTermInYearsE(term) } -// convertSavingsPlansLookbackPeriod converts lookback period for Savings Plans -func convertSavingsPlansLookbackPeriod(period string) types.LookbackPeriodInDays { - return convertLookbackPeriod(period) +// convertSavingsPlansLookbackPeriod converts lookback period for Savings Plans, +// returning an error for unrecognized values. +func convertSavingsPlansLookbackPeriod(period string) (types.LookbackPeriodInDays, error) { + return convertLookbackPeriodE(period) } // normalizeRegionName converts AWS region display names to region codes diff --git a/providers/aws/recommendations/converters_test.go b/providers/aws/recommendations/converters_test.go index a37fe20b4..231540c40 100644 --- a/providers/aws/recommendations/converters_test.go +++ b/providers/aws/recommendations/converters_test.go @@ -86,6 +86,10 @@ func TestGetServiceStringForCostExplorer(t *testing.T) { } func TestConvertPaymentOption(t *testing.T) { + // convertPaymentOption is the legacy wrapper that silently defaults to NoUpfront + // for unknown values (used by client.go RI path, owned by #865/#1075). + // This test documents that silent-default behaviour; new callers should use + // convertPaymentOptionE which returns an error on unrecognised values. tests := []struct { name string option string @@ -106,16 +110,6 @@ func TestConvertPaymentOption(t *testing.T) { option: "no-upfront", expected: types.PaymentOptionNoUpfront, }, - { - name: "Unknown defaults to no upfront", - option: "unknown", - expected: types.PaymentOptionNoUpfront, - }, - { - name: "Empty string defaults to no upfront", - option: "", - expected: types.PaymentOptionNoUpfront, - }, } for _, tt := range tests { @@ -126,7 +120,116 @@ func TestConvertPaymentOption(t *testing.T) { } } +// TestConvertPaymentOptionE_FailLoud is the regression test for H3: +// convertPaymentOptionE must return an error on any unrecognised payment option +// instead of silently substituting NoUpfront (the old behaviour of the +// convertPaymentOption default branch). Callers on the SP recommendation path +// use this erroring variant so a typo or new/renamed option is caught +// before the wrong recs are queried. +func TestConvertPaymentOptionE_FailLoud(t *testing.T) { + tests := []struct { + name string + option string + expected types.PaymentOption + expectError bool + }{ + {"All upfront", "all-upfront", types.PaymentOptionAllUpfront, false}, + {"Partial upfront", "partial-upfront", types.PaymentOptionPartialUpfront, false}, + {"No upfront", "no-upfront", types.PaymentOptionNoUpfront, false}, + // These must error, not default to NoUpfront (H3 regression guard): + {"Unknown option errors", "unknown", "", true}, + {"Empty string errors", "", "", true}, + {"Mixed case errors", "All-Upfront", "", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := convertPaymentOptionE(tt.option) + if tt.expectError { + assert.Error(t, err, "convertPaymentOptionE(%q) must error", tt.option) + assert.Empty(t, result) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, result) + } + }) + } +} + +// TestConvertTermInYearsE_FailLoud is the regression test for L1: +// convertTermInYearsE must error on unrecognised terms rather than silently +// defaulting to OneYear. +func TestConvertTermInYearsE_FailLoud(t *testing.T) { + tests := []struct { + name string + term string + expected types.TermInYears + expectError bool + }{ + {"1yr", "1yr", types.TermInYearsOneYear, false}, + {"1 numeric", "1", types.TermInYearsOneYear, false}, + {"3yr", "3yr", types.TermInYearsThreeYears, false}, + {"3 numeric", "3", types.TermInYearsThreeYears, false}, + // These must error, not default to OneYear (L1 regression guard): + {"Unknown term errors", "unknown", "", true}, + {"Empty string errors", "", "", true}, + {"2yr errors", "2yr", "", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := convertTermInYearsE(tt.term) + if tt.expectError { + assert.Error(t, err, "convertTermInYearsE(%q) must error", tt.term) + assert.Empty(t, result) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, result) + } + }) + } +} + +// TestConvertLookbackPeriodE_FailLoud is the regression test for L2: +// convertLookbackPeriodE must error on unrecognised periods rather than +// silently defaulting to SevenDays. +func TestConvertLookbackPeriodE_FailLoud(t *testing.T) { + tests := []struct { + name string + period string + expected types.LookbackPeriodInDays + expectError bool + }{ + {"7d", "7d", types.LookbackPeriodInDaysSevenDays, false}, + {"7 numeric", "7", types.LookbackPeriodInDaysSevenDays, false}, + {"30d", "30d", types.LookbackPeriodInDaysThirtyDays, false}, + {"30 numeric", "30", types.LookbackPeriodInDaysThirtyDays, false}, + {"60d", "60d", types.LookbackPeriodInDaysSixtyDays, false}, + {"60 numeric", "60", types.LookbackPeriodInDaysSixtyDays, false}, + // These must error, not default to SevenDays (L2 regression guard): + {"Unknown period errors", "unknown", "", true}, + {"Empty string errors", "", "", true}, + {"90d errors", "90d", "", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := convertLookbackPeriodE(tt.period) + if tt.expectError { + assert.Error(t, err, "convertLookbackPeriodE(%q) must error", tt.period) + assert.Empty(t, result) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, result) + } + }) + } +} + func TestConvertTermInYears(t *testing.T) { + // convertTermInYears is the legacy wrapper used by client.go (RI path); + // it silently returns OneYear for unrecognised values. This test covers the + // valid cases only; the fail-loud path is tested by TestConvertTermInYearsE_FailLoud. tests := []struct { name string term string @@ -152,16 +255,6 @@ func TestConvertTermInYears(t *testing.T) { term: "1", expected: types.TermInYearsOneYear, }, - { - name: "Empty defaults to one year", - term: "", - expected: types.TermInYearsOneYear, - }, - { - name: "Unknown defaults to one year", - term: "unknown", - expected: types.TermInYearsOneYear, - }, } for _, tt := range tests { @@ -173,6 +266,9 @@ func TestConvertTermInYears(t *testing.T) { } func TestConvertLookbackPeriod(t *testing.T) { + // convertLookbackPeriod is the legacy wrapper used by client.go (RI path); + // it silently returns SevenDays for unrecognised values. This test covers valid + // cases only; the fail-loud path is tested by TestConvertLookbackPeriodE_FailLoud. tests := []struct { name string period string @@ -208,16 +304,6 @@ func TestConvertLookbackPeriod(t *testing.T) { period: "60", expected: types.LookbackPeriodInDaysSixtyDays, }, - { - name: "Empty defaults to seven days", - period: "", - expected: types.LookbackPeriodInDaysSevenDays, - }, - { - name: "Unknown defaults to seven days", - period: "unknown", - expected: types.LookbackPeriodInDaysSevenDays, - }, } for _, tt := range tests { @@ -229,30 +315,46 @@ func TestConvertLookbackPeriod(t *testing.T) { } func TestConvertSavingsPlansPaymentOption(t *testing.T) { - // This function delegates to convertPaymentOption, so we just verify it works - result := convertSavingsPlansPaymentOption("all-upfront") + // SP wrappers now return (value, error); valid options must succeed. + result, err := convertSavingsPlansPaymentOption("all-upfront") + assert.NoError(t, err) assert.Equal(t, types.PaymentOptionAllUpfront, result) - result = convertSavingsPlansPaymentOption("partial-upfront") + result, err = convertSavingsPlansPaymentOption("partial-upfront") + assert.NoError(t, err) assert.Equal(t, types.PaymentOptionPartialUpfront, result) + + // Unknown option must error (not silently default). + _, err = convertSavingsPlansPaymentOption("bogus") + assert.Error(t, err, "convertSavingsPlansPaymentOption(bogus) must error") } func TestConvertSavingsPlansTermInYears(t *testing.T) { - // This function delegates to convertTermInYears, so we just verify it works - result := convertSavingsPlansTermInYears("3yr") + result, err := convertSavingsPlansTermInYears("3yr") + assert.NoError(t, err) assert.Equal(t, types.TermInYearsThreeYears, result) - result = convertSavingsPlansTermInYears("1yr") + result, err = convertSavingsPlansTermInYears("1yr") + assert.NoError(t, err) assert.Equal(t, types.TermInYearsOneYear, result) + + // Unknown term must error. + _, err = convertSavingsPlansTermInYears("bogus") + assert.Error(t, err, "convertSavingsPlansTermInYears(bogus) must error") } func TestConvertSavingsPlansLookbackPeriod(t *testing.T) { - // This function delegates to convertLookbackPeriod, so we just verify it works - result := convertSavingsPlansLookbackPeriod("7d") + result, err := convertSavingsPlansLookbackPeriod("7d") + assert.NoError(t, err) assert.Equal(t, types.LookbackPeriodInDaysSevenDays, result) - result = convertSavingsPlansLookbackPeriod("30d") + result, err = convertSavingsPlansLookbackPeriod("30d") + assert.NoError(t, err) assert.Equal(t, types.LookbackPeriodInDaysThirtyDays, result) + + // Unknown period must error. + _, err = convertSavingsPlansLookbackPeriod("bogus") + assert.Error(t, err, "convertSavingsPlansLookbackPeriod(bogus) must error") } func TestNormalizeRegionName(t *testing.T) { diff --git a/providers/aws/recommendations/parser_services.go b/providers/aws/recommendations/parser_services.go index cb456c58e..d69cd7017 100644 --- a/providers/aws/recommendations/parser_services.go +++ b/providers/aws/recommendations/parser_services.go @@ -29,14 +29,24 @@ func (c *Client) parseRDSDetails(_ context.Context, rec *common.Recommendation, if rdsDetails.Region != nil { rec.Region = normalizeRegionName(*rdsDetails.Region) } + // AZConfig is intentionally left empty when CE omits DeploymentOption. + // Single-AZ and multi-AZ RDS RIs have different prices and cover different + // workloads; silently defaulting to single-az would cause findOfferingID to + // query for (and potentially buy) a single-AZ RI even for a multi-AZ workload. + // The downstream findOfferingID rejects an empty AZConfig with an explicit + // error rather than proceeding with a fabricated value (M4 fix). if rdsDetails.DeploymentOption != nil { - if *rdsDetails.DeploymentOption == "Multi-AZ" { + // Map only the exact CE tokens; an unrecognized value must not be folded + // into single-az (which would drive findOfferingID to the wrong RI class + // and a mis-buy). Fail loud instead so the bad token surfaces (CR #1085). + switch strings.TrimSpace(*rdsDetails.DeploymentOption) { + case "Multi-AZ": rdsInfo.AZConfig = "multi-az" - } else { + case "Single-AZ": rdsInfo.AZConfig = "single-az" + default: + return fmt.Errorf("unrecognized RDS DeploymentOption %q: expected \"Multi-AZ\" or \"Single-AZ\"", *rdsDetails.DeploymentOption) } - } else { - rdsInfo.AZConfig = "single-az" } rec.Details = rdsInfo @@ -69,12 +79,28 @@ func (c *Client) parseElastiCacheDetails(_ context.Context, rec *common.Recommen // resolveEC2Tenancy maps a Cost Explorer tenancy value to the EC2 RI API // tenancy string. CE uses "shared" for the default tenancy; "dedicated" maps -// directly. Any nil or unrecognised value is treated as default. -func resolveEC2Tenancy(tenancy *string) string { - if tenancy != nil && *tenancy == "dedicated" { - return string(ec2types.TenancyDedicated) +// directly. A nil pointer (CE omitted the field) also maps to default, because +// CE only populates the field when it is non-default. +// +// Unknown tenancy values (e.g. "host" for Dedicated Hosts, which have no +// corresponding RI product) return an error so the caller fails loud rather +// than silently querying for and buying a default-tenancy RI on behalf of a +// workload that requires a different tenancy class (M5 fix). +func resolveEC2Tenancy(tenancy *string) (string, error) { + if tenancy == nil || *tenancy == "shared" { + return string(ec2types.TenancyDefault), nil + } + switch *tenancy { + case "dedicated": + return string(ec2types.TenancyDedicated), nil + default: + return "", fmt.Errorf( + "unrecognised EC2 tenancy %q from Cost Explorer: "+ + "must be shared (default) or dedicated; "+ + "host tenancy has no corresponding RI product", + *tenancy, + ) } - return string(ec2types.TenancyDefault) } // resolveEC2Scope maps a Cost Explorer availability zone value to the EC2 RI @@ -126,7 +152,11 @@ func (c *Client) parseEC2Details(ctx context.Context, rec *common.Recommendation if ec2Details.Region != nil { rec.Region = normalizeRegionName(*ec2Details.Region) } - ec2Info.Tenancy = resolveEC2Tenancy(ec2Details.Tenancy) + tenancy, tenancyErr := resolveEC2Tenancy(ec2Details.Tenancy) + if tenancyErr != nil { + return tenancyErr + } + ec2Info.Tenancy = tenancy ec2Info.Scope = resolveEC2Scope(ec2Details.AvailabilityZone) c.enrichFromCatalogue(ctx, ec2Info) diff --git a/providers/aws/recommendations/parser_services_test.go b/providers/aws/recommendations/parser_services_test.go index e01c2bd94..d224d3695 100644 --- a/providers/aws/recommendations/parser_services_test.go +++ b/providers/aws/recommendations/parser_services_test.go @@ -65,7 +65,11 @@ func TestParseRDSDetails(t *testing.T) { }, }, { - name: "RDS details without deployment option defaults to single-az", + // M4 regression test: when CE omits DeploymentOption, AZConfig must be + // left empty rather than silently defaulted to "single-az". + // single-AZ and multi-AZ RDS RIs have different prices; guessing wrong + // would cause findOfferingID to buy the wrong offering class. + name: "RDS details without deployment option leaves AZConfig empty (M4)", details: &types.ReservationPurchaseRecommendationDetail{ InstanceDetails: &types.InstanceDetails{ RDSInstanceDetails: &types.RDSInstanceDetails{ @@ -81,7 +85,26 @@ func TestParseRDSDetails(t *testing.T) { dbDetails, ok := rec.Details.(*common.DatabaseDetails) require.True(t, ok) assert.Equal(t, "aurora-postgresql", dbDetails.Engine) - assert.Equal(t, "single-az", dbDetails.AZConfig) + // AZConfig must be empty, not "single-az" (M4 fix). + assert.Empty(t, dbDetails.AZConfig, "AZConfig must not be silently defaulted to single-az") + }, + }, + { + // CR #1085 regression test: an unrecognized DeploymentOption must error + // rather than being silently folded into "single-az". The pre-fix code + // used an else branch that mapped anything non-"Multi-AZ" to "single-az", + // which could cause findOfferingID to query and buy the wrong RI class. + name: "Unknown DeploymentOption errors (CR #1085)", + expectError: true, + details: &types.ReservationPurchaseRecommendationDetail{ + InstanceDetails: &types.InstanceDetails{ + RDSInstanceDetails: &types.RDSInstanceDetails{ + InstanceType: aws.String("db.r5.large"), + DatabaseEngine: aws.String("mysql"), + Region: aws.String("us-east-1"), + DeploymentOption: aws.String("Multi-AZ-Readable-Standbys"), + }, + }, }, }, { @@ -330,6 +353,41 @@ func TestParseEC2Details(t *testing.T) { }, expectError: true, }, + { + // M5 regression test: "host" tenancy (Dedicated Hosts) has no + // corresponding EC2 RI product. Previously this silently collapsed + // to "default", which would cause findOfferingID to look up (and + // potentially buy) a default-tenancy RI for a Dedicated Host workload. + // The parser must error so the caller can decide, rather than + // silently buying the wrong product. + name: "EC2 host tenancy errors (M5)", + details: &types.ReservationPurchaseRecommendationDetail{ + InstanceDetails: &types.InstanceDetails{ + EC2InstanceDetails: &types.EC2InstanceDetails{ + InstanceType: aws.String("m5.large"), + Platform: aws.String("Linux/UNIX"), + Region: aws.String("us-east-1"), + Tenancy: aws.String("host"), + }, + }, + }, + expectError: true, + }, + { + // M5: an unrecognised tenancy value should also error. + name: "EC2 unknown tenancy errors (M5)", + details: &types.ReservationPurchaseRecommendationDetail{ + InstanceDetails: &types.InstanceDetails{ + EC2InstanceDetails: &types.EC2InstanceDetails{ + InstanceType: aws.String("m5.large"), + Platform: aws.String("Linux/UNIX"), + Region: aws.String("us-east-1"), + Tenancy: aws.String("future-unknown-tenancy"), + }, + }, + }, + expectError: true, + }, } for _, tt := range tests { diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 6eebc4712..41b865eaf 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -37,11 +37,23 @@ func (c *Client) getSavingsPlansRecommendations(ctx context.Context, params comm var allRecommendations []common.Recommendation for _, planType := range planTypes { + paymentOption, err := convertSavingsPlansPaymentOption(params.PaymentOption) + if err != nil { + return nil, fmt.Errorf("invalid payment option for Savings Plans recommendation: %w", err) + } + termInYears, err := convertSavingsPlansTermInYears(params.Term) + if err != nil { + return nil, fmt.Errorf("invalid term for Savings Plans recommendation: %w", err) + } + lookbackPeriod, err := convertSavingsPlansLookbackPeriod(params.LookbackPeriod) + if err != nil { + return nil, fmt.Errorf("invalid lookback period for Savings Plans recommendation: %w", err) + } input := &costexplorer.GetSavingsPlansPurchaseRecommendationInput{ SavingsPlansType: planType, - PaymentOption: convertSavingsPlansPaymentOption(params.PaymentOption), - TermInYears: convertSavingsPlansTermInYears(params.Term), - LookbackPeriodInDays: convertSavingsPlansLookbackPeriod(params.LookbackPeriod), + PaymentOption: paymentOption, + TermInYears: termInYears, + LookbackPeriodInDays: lookbackPeriod, AccountScope: types.AccountScopeLinked, } diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index fa1a0ebf5..2b31d1cf1 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -341,6 +341,13 @@ func canonicalizeEC2Scope(s string) string { // of timing out the Lambda budget (issue #688). const maxOfferingPages = 5 +// defaultEC2Platform is the EC2 RI product-description value for Linux/UNIX instances. +// Used as a fallback in exchange-package helpers (FindConvertibleOffering, +// ListTargetOfferings) where callers may legitimately omit the platform; never +// used as a silent fallback on the purchase path (see M2/M3 in +// 19-hardcoded-fallbacks-aws.md). +const defaultEC2Platform = "Linux/UNIX" + // convertEC2PaymentOption maps a rec payment-option slug to the AWS // DescribeReservedInstancesOfferings OfferingType enum value. func convertEC2PaymentOption(option string) (types.OfferingTypeValues, error) { @@ -367,11 +374,17 @@ type ec2OfferingQuery struct { } // buildEC2OfferingQuery resolves the typed lookup parameters from a rec, -// canonicalising legacy tenancy/scope values and applying API defaults. -func buildEC2OfferingQuery(rec common.Recommendation, details *common.ComputeDetails, duration int64) ec2OfferingQuery { - platform := details.Platform - if platform == "" { - platform = "Linux/UNIX" +// canonicalising legacy tenancy/scope values. Returns an error when Platform is +// empty: on the purchase path the CE parser always populates it from the +// recommendation payload, so an empty Platform signals a malformed rec rather +// than a value that should be fabricated (M2/M3 fix, see 19-hardcoded-fallbacks-aws.md). +func buildEC2OfferingQuery(rec common.Recommendation, details *common.ComputeDetails, duration int64) (ec2OfferingQuery, error) { + if details.Platform == "" { + return ec2OfferingQuery{}, fmt.Errorf( + "EC2 recommendation for %s is missing Platform: "+ + "refusing to fabricate a product-description for the RI offering lookup", + rec.ResourceType, + ) } tenancy := canonicalizeEC2Tenancy(details.Tenancy) if tenancy == "" { @@ -383,11 +396,11 @@ func buildEC2OfferingQuery(rec common.Recommendation, details *common.ComputeDet } return ec2OfferingQuery{ instanceType: types.InstanceType(rec.ResourceType), - productDesc: types.RIProductDescription(platform), + productDesc: types.RIProductDescription(details.Platform), tenancy: types.Tenancy(tenancy), scope: scope, duration: duration, - } + }, nil } // describeInputFromQuery builds the SDK request struct for one page of the @@ -424,7 +437,10 @@ func (c *Client) buildEC2QueryFromRec(rec common.Recommendation) (ec2OfferingQue if err != nil { return ec2OfferingQuery{}, err } - q := buildEC2OfferingQuery(rec, details, c.getDurationValue(rec.Term)) + q, err := buildEC2OfferingQuery(rec, details, c.getDurationValue(rec.Term)) + if err != nil { + return ec2OfferingQuery{}, err + } q.wantOfferingType = wantOfferingType return q, nil } @@ -658,7 +674,7 @@ func (c *Client) ListConvertibleReservedInstances(ctx context.Context) ([]Conver }, { Name: aws.String("offering-class"), - Values: []string{"convertible"}, + Values: []string{string(types.OfferingClassTypeConvertible)}, }, }, } @@ -726,7 +742,7 @@ func (c *Client) FindConvertibleOffering(ctx context.Context, params FindConvert } productDesc := params.ProductDescription if productDesc == "" { - productDesc = "Linux/UNIX" + productDesc = defaultEC2Platform } filters := []types.Filter{ @@ -735,7 +751,7 @@ func (c *Client) FindConvertibleOffering(ctx context.Context, params FindConvert {Name: aws.String("instance-tenancy"), Values: []string{tenancy}}, {Name: aws.String("scope"), Values: []string{scope}}, {Name: aws.String("duration"), Values: []string{fmt.Sprintf("%d", duration)}}, - {Name: aws.String("offering-class"), Values: []string{"convertible"}}, + {Name: aws.String("offering-class"), Values: []string{string(types.OfferingClassTypeConvertible)}}, } input := &ec2.DescribeReservedInstancesOfferingsInput{ @@ -804,7 +820,7 @@ func normalizeTargetOfferingsParams(p ListTargetOfferingsParams) (tenancy, scope } productDesc = p.ProductDescription if productDesc == "" { - productDesc = "Linux/UNIX" + productDesc = defaultEC2Platform } // OfferingType: typed field when non-empty; empty string means "all // payment options" (the caller didn't specify). Leave unset so AWS diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 7f1083423..44ecee07d 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -775,3 +775,48 @@ func TestClient_PurchaseCommitment_NameTagInCreateTagsRequest(t *testing.T) { mockEC2.AssertExpectations(t) } + +// TestBuildEC2OfferingQuery_EmptyPlatformErrors is the M2/M3 regression test: +// buildEC2OfferingQuery must return an error when Platform is empty rather than +// silently substituting "Linux/UNIX". On the purchase path the CE parser always +// populates Platform from the recommendation payload; an empty value signals a +// malformed rec, not a value to be fabricated. +func TestBuildEC2OfferingQuery_EmptyPlatformErrors(t *testing.T) { + rec := common.Recommendation{ + ResourceType: "m5.large", + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.ComputeDetails{ + InstanceType: "m5.large", + Platform: "", // intentionally empty + Tenancy: "default", + Scope: "Region", + }, + } + details := rec.Details.(*common.ComputeDetails) + + _, err := buildEC2OfferingQuery(rec, details, OneYearSeconds) + assert.Error(t, err, "buildEC2OfferingQuery must error when Platform is empty (M2/M3 fix)") + assert.Contains(t, err.Error(), "Platform") +} + +// TestBuildEC2OfferingQuery_ValidPlatform asserts the happy path still works. +func TestBuildEC2OfferingQuery_ValidPlatform(t *testing.T) { + rec := common.Recommendation{ + ResourceType: "m5.large", + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.ComputeDetails{ + InstanceType: "m5.large", + Platform: "Linux/UNIX", + Tenancy: "default", + Scope: "Region", + }, + } + details := rec.Details.(*common.ComputeDetails) + + q, err := buildEC2OfferingQuery(rec, details, OneYearSeconds) + assert.NoError(t, err) + assert.Equal(t, types.RIProductDescription("Linux/UNIX"), q.productDesc) + assert.Equal(t, types.Tenancy("default"), q.tenancy) +} diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index fe65f85e9..58e16a3dd 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -303,6 +303,23 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, if !ok || details == nil { return "", fmt.Errorf("invalid service details for RDS") } + // AZConfig must be explicitly set: single-AZ and multi-AZ RDS RIs have + // different prices and do not cover each other's demand. An empty AZConfig + // means the CE recommendation omitted DeploymentOption; guessing single-az + // risks buying the wrong RI class. Fail loud so the caller can decide. + // Validate the full enum, not just the empty case: a non-empty typo would + // otherwise fall through to multiAZ==false in paginateRDSOfferings and + // silently drive a single-AZ lookup -- the same mis-buy class as the old + // default (CR #1085). + switch details.AZConfig { + case "single-az", "multi-az": + // valid + case "": + return "", fmt.Errorf("RDS AZConfig is empty: CE recommendation did not include DeploymentOption; " + + "refusing to guess single-az vs multi-az (see M4 in 19-hardcoded-fallbacks-aws.md)") + default: + return "", fmt.Errorf("invalid RDS AZConfig %q: must be single-az or multi-az", details.AZConfig) + } offeringType, err := c.convertPaymentOption(rec.PaymentOption) if err != nil { return "", fmt.Errorf("invalid payment option: %w", err) @@ -310,12 +327,53 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, return c.paginateRDSOfferings(ctx, rec, details, offeringType, execID) } +// rdsOfferingPageResult holds the outcome of a single DescribeReservedDBInstancesOfferings page. +type rdsOfferingPageResult struct { + id string // non-empty when a match was found + marker *string // pagination cursor for the next page; nil when exhausted +} + +// fetchRDSOfferingPage calls DescribeReservedDBInstancesOfferings for one page and +// scans the results. It returns a match ID when found, a non-nil marker when more +// pages remain, or an error on API/offering-validation failure. +func (c *Client) fetchRDSOfferingPage(ctx context.Context, baseInput *rds.DescribeReservedDBInstancesOfferingsInput, marker *string, rec common.Recommendation, offeringType string, tag string, page int, t0 time.Time) (rdsOfferingPageResult, error) { + input := *baseInput + input.Marker = marker + + pageStart := time.Now() + result, err := c.client.DescribeReservedDBInstancesOfferings(ctx, &input) + if err != nil { + log.Printf("purchase[%s]: RDS findOfferingID page %d failed after %s (total %s): %v", + tag, page, time.Since(pageStart), time.Since(t0), err) + return rdsOfferingPageResult{}, fmt.Errorf("failed to describe offerings: %w", err) + } + log.Printf("purchase[%s]: RDS findOfferingID page %d: %d offerings in %s", + tag, page, len(result.ReservedDBInstancesOfferings), time.Since(pageStart)) + + id, scanErr := scanRDSOfferingPage(result.ReservedDBInstancesOfferings, rec, offeringType) + if scanErr != nil { + return rdsOfferingPageResult{}, scanErr + } + if id != "" { + log.Printf("purchase[%s]: RDS findOfferingID found match on page %d after %s total", tag, page, time.Since(t0)) + return rdsOfferingPageResult{id: id}, nil + } + var nextMarker *string + if result.Marker != nil && aws.ToString(result.Marker) != "" { + nextMarker = result.Marker + } + return rdsOfferingPageResult{marker: nextMarker}, nil +} + // paginateRDSOfferings walks DescribeReservedDBInstancesOfferings pages and returns // the first matching offering ID. It caps at maxOfferingPages to prevent Lambda // timeout exhaustion (issue #688). func (c *Client) paginateRDSOfferings(ctx context.Context, rec common.Recommendation, details *common.DatabaseDetails, offeringType string, execID string) (string, error) { multiAZ := details.AZConfig == "multi-az" - normalizedEngine := c.normalizeEngineName(details.Engine) + normalizedEngine, err := c.normalizeEngineName(details.Engine) + if err != nil { + return "", fmt.Errorf("cannot look up RDS offering: %w", err) + } duration := c.getDurationString(rec.Term) tag := execID @@ -326,51 +384,39 @@ func (c *Client) paginateRDSOfferings(ctx context.Context, rec common.Recommenda log.Printf("purchase[%s]: RDS findOfferingID starting (class=%s engine=%s multi-az=%v duration=%s payment=%s)", tag, rec.ResourceType, normalizedEngine, multiAZ, duration, offeringType) + baseInput := &rds.DescribeReservedDBInstancesOfferingsInput{ + DBInstanceClass: aws.String(rec.ResourceType), + ProductDescription: aws.String(normalizedEngine), + MultiAZ: aws.Bool(multiAZ), + Duration: aws.String(duration), + OfferingType: aws.String(offeringType), + MaxRecords: aws.Int32(100), + } + var marker *string - page := 0 - for { + for page := 1; ; page++ { if err := ctx.Err(); err != nil { return "", err } - page++ if page > maxOfferingPages { return "", fmt.Errorf("pagination cap reached after %d pages for RDS %s %s multi-az=%v %s (issue #688)", maxOfferingPages, rec.ResourceType, details.Engine, multiAZ, rec.PaymentOption) } - input := &rds.DescribeReservedDBInstancesOfferingsInput{ - DBInstanceClass: aws.String(rec.ResourceType), - ProductDescription: aws.String(normalizedEngine), - MultiAZ: aws.Bool(multiAZ), - Duration: aws.String(duration), - OfferingType: aws.String(offeringType), - MaxRecords: aws.Int32(100), - Marker: marker, - } - pageStart := time.Now() - result, err := c.client.DescribeReservedDBInstancesOfferings(ctx, input) + pr, err := c.fetchRDSOfferingPage(ctx, baseInput, marker, rec, offeringType, tag, page, t0) if err != nil { - log.Printf("purchase[%s]: RDS findOfferingID page %d failed after %s (total %s): %v", - tag, page, time.Since(pageStart), time.Since(t0), err) - return "", fmt.Errorf("failed to describe offerings: %w", err) + return "", err } - log.Printf("purchase[%s]: RDS findOfferingID page %d: %d offerings in %s", - tag, page, len(result.ReservedDBInstancesOfferings), time.Since(pageStart)) - if id, scanErr := scanRDSOfferingPage(result.ReservedDBInstancesOfferings, rec, offeringType); scanErr != nil { - return "", scanErr - } else if id != "" { - log.Printf("purchase[%s]: RDS findOfferingID found match on page %d after %s total", - tag, page, time.Since(t0)) - return id, nil + if pr.id != "" { + return pr.id, nil } - if result.Marker == nil || aws.ToString(result.Marker) == "" { + if pr.marker == nil { break } - marker = result.Marker + marker = pr.marker } - log.Printf("purchase[%s]: RDS findOfferingID exhausted %d page(s) in %s -- no match", - tag, page, time.Since(t0)) + log.Printf("purchase[%s]: RDS findOfferingID exhausted pages in %s -- no match", tag, time.Since(t0)) return "", fmt.Errorf("no offerings found for RDS %s %s multi-az=%v %s after %d page(s) (issue #688)", - rec.ResourceType, details.Engine, multiAZ, rec.PaymentOption, page) + rec.ResourceType, details.Engine, multiAZ, rec.PaymentOption, maxOfferingPages) } // scanRDSOfferingPage finds a matching offering in a single page of results. @@ -504,38 +550,63 @@ func (c *Client) convertPaymentOption(option string) (string, error) { } } -// normalizeEngineName converts engine names to AWS API format -func (c *Client) normalizeEngineName(engine string) string { +// normalizeEngineName maps an RDS engine string to the exact product-description +// value required by DescribeReservedDBInstancesOfferings. It returns an error +// for engine names that are ambiguous (Oracle, SQL Server -- multiple editions +// exist at different prices) or that contain "aurora" without specifying a +// database engine (aurora-mysql vs aurora-postgresql), so the caller never +// silently buys an offering for the wrong engine edition. +// +// Unambiguous engines (mysql, postgresql, mariadb) are returned verbatim after +// case-normalisation. Engine strings that are already in the canonical +// lower-case AWS form (e.g. "aurora-mysql") pass through unchanged. +func (c *Client) normalizeEngineName(engine string) (string, error) { engineLower := strings.ToLower(engine) if strings.Contains(engineLower, "aurora") { if strings.Contains(engineLower, "mysql") { - return "aurora-mysql" + return "aurora-mysql", nil } if strings.Contains(engineLower, "postgres") { - return "aurora-postgresql" + return "aurora-postgresql", nil } - log.Printf("WARNING: Unknown Aurora variant %q, defaulting to aurora-mysql", engine) - return "aurora-mysql" + return "", fmt.Errorf( + "ambiguous Aurora engine %q: CE must supply the specific variant "+ + "(aurora-mysql or aurora-postgresql); refusing to guess", + engine, + ) } if strings.Contains(engineLower, "mysql") { - return "mysql" + return "mysql", nil } if strings.Contains(engineLower, "postgres") { - return "postgresql" + return "postgresql", nil } if strings.Contains(engineLower, "mariadb") { - return "mariadb" - } - if strings.Contains(engineLower, "oracle") { - return "oracle-se2" - } - if strings.Contains(engineLower, "sqlserver") || strings.Contains(engineLower, "sql-server") { - return "sqlserver-se" - } - - return engineLower + return "mariadb", nil + } + // Only the bare family name is ambiguous (CE returns "Oracle" / "SQL Server" + // with no edition). An edition-qualified token like oracle-se2 or + // sqlserver-web is already a valid RDS ProductDescription, so pass it through + // rather than rejecting it -- rejecting it contradicted the "supply the exact + // edition" guidance in this very error message (CR #1085). + if engineLower == "oracle" { + return "", fmt.Errorf( + "ambiguous Oracle engine %q: CE must supply the exact edition "+ + "(e.g. oracle-se2, oracle-ee); refusing to guess", + engine, + ) + } + if engineLower == "sqlserver" || engineLower == "sql-server" { + return "", fmt.Errorf( + "ambiguous SQL Server engine %q: CE must supply the exact edition "+ + "(e.g. sqlserver-se, sqlserver-ee, sqlserver-web, sqlserver-ex); refusing to guess", + engine, + ) + } + + return engineLower, nil } // createPurchaseTags creates standard tags for the purchase. The tag shape diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index 7541ca874..6c4f4bb74 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -13,6 +13,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/rds/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) // MockRDSClient implements RDSAPI for testing @@ -480,27 +481,49 @@ func TestClient_NormalizeEngineName(t *testing.T) { client := &Client{} tests := []struct { - name string - input string - expected string + name string + input string + expected string + expectError bool }{ - {"Aurora MySQL uppercase", "Aurora-MySQL", "aurora-mysql"}, - {"Aurora PostgreSQL mixed case", "Aurora-PostgreSQL", "aurora-postgresql"}, - {"Aurora default", "Aurora", "aurora-mysql"}, - {"MySQL", "MySQL", "mysql"}, - {"PostgreSQL", "PostgreSQL", "postgresql"}, - {"MariaDB", "MariaDB", "mariadb"}, - {"Oracle", "Oracle-EE", "oracle-se2"}, - {"SQL Server hyphenated", "sql-server-ex", "sqlserver-se"}, - {"SQL Server camelcase", "SQLServer", "sqlserver-se"}, - {"Already normalized postgres", "postgres", "postgresql"}, - {"Unknown engine", "custom-db", "custom-db"}, + {"Aurora MySQL uppercase", "Aurora-MySQL", "aurora-mysql", false}, + {"Aurora PostgreSQL mixed case", "Aurora-PostgreSQL", "aurora-postgresql", false}, + // Previously "Aurora" without a database variant silently defaulted to + // aurora-mysql. Now it must error: aurora-mysql and aurora-postgresql have + // different RI prices and the caller must supply the precise variant. + {"Aurora no variant errors", "Aurora", "", true}, + {"MySQL", "MySQL", "mysql", false}, + {"PostgreSQL", "PostgreSQL", "postgresql", false}, + {"MariaDB", "MariaDB", "mariadb", false}, + // Bare family names are ambiguous; only the bare names error. + {"Oracle bare errors", "oracle", "", true}, + {"Oracle title-case bare errors", "Oracle", "", true}, + {"SQL Server bare errors", "sqlserver", "", true}, + {"SQL Server hyphen bare errors", "sql-server", "", true}, + {"SQL Server camelcase errors", "SQLServer", "", true}, + // Edition-qualified strings are valid RDS ProductDescription values and must pass through. + {"Oracle EE edition passes", "oracle-ee", "oracle-ee", false}, + {"Oracle SE2 edition passes", "oracle-se2", "oracle-se2", false}, + {"Oracle EE mixed case passes", "Oracle-EE", "oracle-ee", false}, + {"SQL Server SE edition passes", "sqlserver-se", "sqlserver-se", false}, + {"SQL Server web edition passes", "sqlserver-web", "sqlserver-web", false}, + {"SQL Server hyphen-ex edition passes", "sql-server-ex", "sql-server-ex", false}, + {"Already normalized aurora-mysql", "aurora-mysql", "aurora-mysql", false}, + {"Already normalized aurora-postgresql", "aurora-postgresql", "aurora-postgresql", false}, + {"Already normalized postgres", "postgres", "postgresql", false}, + {"Unknown engine passes through", "custom-db", "custom-db", false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := client.normalizeEngineName(tt.input) - assert.Equal(t, tt.expected, result) + result, err := client.normalizeEngineName(tt.input) + if tt.expectError { + assert.Error(t, err, "expected error for engine %q", tt.input) + assert.Empty(t, result) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, result) + } }) } } @@ -798,6 +821,124 @@ func TestFindOfferingID_HappyPath(t *testing.T) { assert.Equal(t, "offering-ok", id) } +// TestFindOfferingID_EmptyAZConfig_Errors is the M4 regression test: +// findOfferingID must error when AZConfig is empty rather than silently +// assuming single-az. An empty AZConfig means the CE recommendation did not +// include DeploymentOption; single-AZ and multi-AZ RDS RIs have different +// prices so fabricating the value risks buying the wrong offering class. +func TestFindOfferingID_EmptyAZConfig_Errors(t *testing.T) { + mockRDS := &MockRDSClient{} + t.Cleanup(func() { mockRDS.AssertExpectations(t) }) + client := &Client{client: mockRDS, region: "us-east-1"} + + rec := common.Recommendation{ + Service: common.ServiceRelationalDB, + ResourceType: "db.r5.large", + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.DatabaseDetails{ + Engine: "mysql", + AZConfig: "", // not set -- CE omitted DeploymentOption + }, + } + + // No mock calls should be made: findOfferingID must fail before the API call. + _, err := client.findOfferingID(context.Background(), rec, "") + + require.Error(t, err, "findOfferingID must error when AZConfig is empty (M4)") + assert.Contains(t, err.Error(), "AZConfig") +} + +// TestFindOfferingID_InvalidAZConfig_Errors is the CR #1085 regression test: +// findOfferingID must reject a non-empty but invalid AZConfig value rather than +// silently falling through to multiAZ==false in paginateRDSOfferings (which +// would treat the bad value as single-AZ, the same mis-buy as the old default). +func TestFindOfferingID_InvalidAZConfig_Errors(t *testing.T) { + mockRDS := &MockRDSClient{} + t.Cleanup(func() { mockRDS.AssertExpectations(t) }) + client := &Client{client: mockRDS, region: "us-east-1"} + + rec := common.Recommendation{ + Service: common.ServiceRelationalDB, + ResourceType: "db.r5.large", + Region: "us-east-1", + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.DatabaseDetails{ + Engine: "mysql", + AZConfig: "typo-az", // non-empty but not a valid enum value + }, + } + + // No mock calls should be made: findOfferingID must fail before the API call. + _, err := client.findOfferingID(context.Background(), rec, "") + + require.Error(t, err, "findOfferingID must error on invalid non-empty AZConfig (CR #1085)") + assert.Contains(t, err.Error(), "AZConfig") + assert.Contains(t, err.Error(), "typo-az") +} + +// TestNormalizeEngineName_AmbiguousErrors is the M6 regression test: +// normalizeEngineName must error for bare Oracle/SQL Server/Aurora inputs. +// CE returns "Oracle" (title-case) for any Oracle engine when no edition is +// specified; the normalizer rejects the bare name so the caller surfaces the +// problem rather than silently guessing an edition. +// Edition-qualified strings (oracle-se2, sqlserver-web, etc.) are valid RDS +// ProductDescription values and must pass through (see CR #1085). +func TestNormalizeEngineName_AmbiguousErrors(t *testing.T) { + client := &Client{} + + ambiguous := []string{ + "oracle", + "Oracle", + "sqlserver", + "SQLServer", + "sql-server", + "Aurora", + "aurora", + "aurora-unknown", + } + + for _, engine := range ambiguous { + t.Run(engine, func(t *testing.T) { + _, err := client.normalizeEngineName(engine) + assert.Error(t, err, "engine %q must error (M6 regression guard)", engine) + }) + } +} + +// TestNormalizeEngineName_EditionTokensPassThrough is the CR #1085 regression +// test: edition-qualified Oracle and SQL Server tokens must pass through +// normalizeEngineName rather than being rejected by the bare-name ambiguity +// check. These are valid RDS ProductDescription values that CE can supply. +func TestNormalizeEngineName_EditionTokensPassThrough(t *testing.T) { + client := &Client{} + + cases := []struct { + input string + expected string + }{ + {"oracle-se2", "oracle-se2"}, + {"oracle-ee", "oracle-ee"}, + {"Oracle-EE", "oracle-ee"}, + {"oracle-se2-ex", "oracle-se2-ex"}, + {"sqlserver-se", "sqlserver-se"}, + {"sqlserver-ee", "sqlserver-ee"}, + {"sqlserver-web", "sqlserver-web"}, + {"sqlserver-ex", "sqlserver-ex"}, + {"sql-server-ex", "sql-server-ex"}, + {"sql-server-se", "sql-server-se"}, + } + + for _, tc := range cases { + t.Run(tc.input, func(t *testing.T) { + result, err := client.normalizeEngineName(tc.input) + assert.NoError(t, err, "edition token %q must pass through (not ambiguous)", tc.input) + assert.Equal(t, tc.expected, result) + }) + } +} + // TestClient_PurchaseCommitment_NoToken_RichReservationName asserts the // no-token CLI path (issue #687) composes a self-describing // ReservedDBInstanceId carrying the service code, region, SKU, count, and diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index c970a286e..4cea975ee 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -262,58 +262,42 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, 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). -func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, execID string) (string, error) { - spDetails, ok := rec.Details.(*common.SavingsPlanDetails) - if !ok { - return "", fmt.Errorf("invalid service details for Savings Plans") - } - - // Per-plan-type client (post-split): the client's planType is the - // source of truth. Reject mismatched recommendations rather than - // silently buying the wrong product. Umbrella legacy clients - // (c.planType == "") fall back to rec.Details.PlanType to preserve - // pre-split behaviour. - planType, convErr := convertPlanType(spDetails.PlanType) - if c.planType != "" { - if convErr != nil { - return "", convErr - } - if planType != c.planType { - return "", fmt.Errorf( - "recommendation plan type %q does not match client scope %q", - spDetails.PlanType, c.planType, - ) - } - planType = c.planType - } else if convErr != nil { +// resolveSPPlanType resolves the effective plan type for an offering lookup. +// When the client is scoped to a specific plan type (post-split), it validates +// that the recommendation matches and returns c.planType. Umbrella/legacy +// clients (c.planType == "") fall back to spDetails.PlanType. +func (c *Client) resolveSPPlanType(spPlanType string) (types.SavingsPlanType, error) { + planType, convErr := convertPlanType(spPlanType) + if c.planType == "" { + // Legacy umbrella client: accept any convertible plan type. + return planType, convErr + } + // Scoped client: reject mismatches to prevent buying the wrong product. + if convErr != nil { return "", convErr } - - termSeconds := convertTermToSeconds(rec.Term) - paymentOption := convertPaymentOption(rec.PaymentOption) - - tag := execID - if tag == "" { - tag = "no-exec" + if planType != c.planType { + return "", fmt.Errorf( + "recommendation plan type %q does not match client scope %q", + spPlanType, c.planType, + ) } + return c.planType, nil +} - t0 := time.Now() - log.Printf("purchase[%s]: SavingsPlans findOfferingID starting (planType=%s term=%s payment=%s)", - tag, planType, rec.Term, rec.PaymentOption) - - // Pin to USD so non-USD currency offerings are excluded server-side. - // 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. +// 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 { input := &savingsplans.DescribeSavingsPlansOfferingsInput{ PlanTypes: []types.SavingsPlanType{planType}, Durations: []int64{termSeconds}, PaymentOptions: []types.SavingsPlanPaymentOption{paymentOption}, - Currencies: []types.CurrencyCode{types.CurrencyCodeUsd}, + // 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. if planType == types.SavingsPlanTypeEc2Instance { if c.region == "" { log.Printf("purchase[%s]: SavingsPlans findOfferingID: client region is empty; skipping region filter", tag) @@ -326,7 +310,41 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, } } } + return input +} + +// 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). +func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, execID string) (string, error) { + spDetails, ok := rec.Details.(*common.SavingsPlanDetails) + if !ok { + return "", fmt.Errorf("invalid service details for Savings Plans") + } + + planType, err := c.resolveSPPlanType(spDetails.PlanType) + if err != nil { + return "", err + } + termSeconds, err := convertTermToSeconds(rec.Term) + if err != nil { + return "", err + } + paymentOption, err := convertPaymentOption(rec.PaymentOption) + if err != nil { + 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) if err != nil { log.Printf("purchase[%s]: SavingsPlans findOfferingID failed after %s: %v", tag, time.Since(t0), err) @@ -352,29 +370,34 @@ func convertPlanType(planType string) (types.SavingsPlanType, error) { } } -// convertTermToSeconds converts a term string to seconds for AWS API -func convertTermToSeconds(term string) int64 { - if term == "3yr" || term == "3" { - return 94608000 // 3 years in seconds (365 * 3 * 86400) - } - if term != "1yr" && term != "1" && term != "" { - log.Printf("WARNING: unknown Savings Plans term %q, defaulting to 1 year", term) +// convertTermToSeconds converts a term string to seconds for the AWS Savings +// Plans API. Returns an error on any unrecognized or empty input so callers +// fail loud rather than silently buying the wrong commitment length. +func convertTermToSeconds(term string) (int64, error) { + switch term { + case "3yr", "3": + return 94608000, nil // 3 years in seconds (365 * 3 * 86400) + case "1yr", "1": + return 31536000, nil // 1 year in seconds (365 * 86400) + default: + return 0, fmt.Errorf("unsupported Savings Plans term %q: must be one of 1yr, 1, 3yr, 3", term) } - return 31536000 // 1 year in seconds (365 * 86400) } -// convertPaymentOption converts a payment option string to AWS SDK type -func convertPaymentOption(paymentOption string) types.SavingsPlanPaymentOption { +// convertPaymentOption converts a payment option string to the AWS SDK type. +// Returns an error on any unrecognized or empty input so callers fail loud +// rather than silently buying the wrong (and potentially most expensive) +// payment option. +func convertPaymentOption(paymentOption string) (types.SavingsPlanPaymentOption, error) { switch paymentOption { case "All Upfront", "all-upfront": - return types.SavingsPlanPaymentOptionAllUpfront + return types.SavingsPlanPaymentOptionAllUpfront, nil case "Partial Upfront", "partial-upfront": - return types.SavingsPlanPaymentOptionPartialUpfront + return types.SavingsPlanPaymentOptionPartialUpfront, nil case "No Upfront", "no-upfront": - return types.SavingsPlanPaymentOptionNoUpfront + return types.SavingsPlanPaymentOptionNoUpfront, nil default: - log.Printf("WARNING: unknown Savings Plans payment option %q, defaulting to AllUpfront", paymentOption) - return types.SavingsPlanPaymentOptionAllUpfront + return "", fmt.Errorf("unsupported Savings Plans payment option %q: must be one of All Upfront, all-upfront, Partial Upfront, partial-upfront, No Upfront, no-upfront", paymentOption) } } diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index 1f0eb6164..e52191a1e 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -926,18 +926,25 @@ func TestClient_FindOfferingID_AllPaymentOptions(t *testing.T) { tests := []struct { name string paymentOption string + expectError bool }{ - {"All Upfront", "All Upfront"}, - {"all-upfront", "all-upfront"}, - {"Partial Upfront", "Partial Upfront"}, - {"partial-upfront", "partial-upfront"}, - {"No Upfront", "No Upfront"}, - {"no-upfront", "no-upfront"}, + {"All Upfront", "All Upfront", false}, + {"all-upfront", "all-upfront", false}, + {"Partial Upfront", "Partial Upfront", false}, + {"partial-upfront", "partial-upfront", false}, + {"No Upfront", "No Upfront", false}, + {"no-upfront", "no-upfront", false}, + // Unknown payment option must error, not silently default to All Upfront + // (regression for H1: prevents silent all-cash-outlay purchase on a typo). + {"unknown payment option errors", "bogus-option", true}, + // Empty payment option must also error (regression for H1). + {"empty payment option errors", "", true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) client := &Client{ client: mockSP, region: "us-east-1", @@ -953,34 +960,47 @@ func TestClient_FindOfferingID_AllPaymentOptions(t *testing.T) { }, } - mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). - Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ - SearchResults: []types.SavingsPlanOffering{ - {OfferingId: aws.String("offering-123")}, - }, - }, nil) + if !tt.expectError { + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + {OfferingId: aws.String("offering-123")}, + }, + }, nil).Once() + } err := client.ValidateOffering(context.Background(), rec) - assert.NoError(t, err) - mockSP.AssertExpectations(t) + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported Savings Plans payment option") + mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings") + } else { + assert.NoError(t, err) + } }) } } func TestClient_FindOfferingID_TermVariations(t *testing.T) { tests := []struct { - name string - term string + name string + term string + expectError bool }{ - {"1yr term", "1yr"}, - {"3yr term", "3yr"}, - {"3 numeric term", "3"}, - {"default term", "invalid"}, + {"1yr term", "1yr", false}, + {"3yr term", "3yr", false}, + {"1 numeric term", "1", false}, + {"3 numeric term", "3", false}, + // Unknown term must error, not silently default to 1yr (regression for H2). + {"unknown term errors", "invalid", true}, + // Empty term must also error, not silently default to 1yr (regression for H2). + {"empty term errors", "", true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) client := &Client{ client: mockSP, region: "us-east-1", @@ -996,16 +1016,23 @@ func TestClient_FindOfferingID_TermVariations(t *testing.T) { }, } - mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). - Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ - SearchResults: []types.SavingsPlanOffering{ - {OfferingId: aws.String("offering-123")}, - }, - }, nil) + if !tt.expectError { + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + {OfferingId: aws.String("offering-123")}, + }, + }, nil).Once() + } err := client.ValidateOffering(context.Background(), rec) - assert.NoError(t, err) - mockSP.AssertExpectations(t) + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported Savings Plans term") + mockSP.AssertNotCalled(t, "DescribeSavingsPlansOfferings") + } else { + assert.NoError(t, err) + } }) } }