From ed44ecf1cae130f346396738b3b945b4d0957116 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 19:37:08 +0200 Subject: [PATCH] fix(aws/recommendations): fail the detail on an invalid average-instances or SP utilization A present-but-invalid AverageNumberOfInstancesUsedPerHour (RI) or EstimatedAverageUtilization (SP) was zeroed, which --target-coverage reads as no signal and passes the rec through unsized; --min-pool-size also keeps avg<=0 recs. These sizing inputs now error like the money fields, so the detail is dropped and reported through IncompleteRecommendationsError. Display-only fields still degrade to 0 with a warning. Refs #54 Co-Authored-By: Claude Opus 5.5 --- providers/aws/recommendations/parser_ri.go | 35 +++++---- .../aws/recommendations/parser_ri_test.go | 75 ++++++++++++++++--- providers/aws/recommendations/parser_sp.go | 19 +++-- .../aws/recommendations/parser_sp_test.go | 51 +++++++++++-- 4 files changed, 140 insertions(+), 40 deletions(-) diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 79336150..68f0a0b4 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -92,7 +92,9 @@ func (c *Client) parseRecommendationDetail(ctx context.Context, details *types.R } // Parse RI utilization signals used by --target-coverage sizing - c.parseRIUtilizationSignals(rec, details) + if err := c.parseRIUtilizationSignals(rec, details); err != nil { + return nil, fmt.Errorf("failed to parse RI utilization signals: %w", err) + } // Parse service-specific details if err := c.parseServiceSpecificDetails(ctx, rec, details, params.Service); err != nil { @@ -108,23 +110,26 @@ func (c *Client) parseRecommendationDetail(ctx context.Context, details *types.R } // parseRIUtilizationSignals populates AverageInstancesUsedPerHour and -// RecommendedUtilization from the CE response. Both fields are *string in the -// SDK; nil or unparseable values leave the destination at zero, which the -// --target-coverage sizing path treats as "no signal" and skips. -func (c *Client) parseRIUtilizationSignals(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) { - // Route through parseOptionalFloatOrWarn so a non-finite/negative value - // (which strconv.ParseFloat accepts / passes through) degrades to 0 rather - // than being stored as a live signal. The downstream --target-coverage - // guards are all `<= 0`, and NaN <= 0 is false, so a stored NaN would be - // treated as a real signal and produce NaN purchase counts. - // - // The field label carries service/account context so a warning still - // identifies which row was corrupt (the pre-refactor inline logs did). +// RecommendedUtilization from the CE response. +// +// AverageNumberOfInstancesUsedPerHour is a sizing input: --target-coverage +// treats zero as "no signal" and passes the rec through at AWS's full count +// (and --min-pool-size keeps avg<=0 recs), so a present-but-invalid value +// (unparsable, non-finite, negative, empty) must fail the detail instead of +// degrading to 0. A nil field is genuinely absent and stays 0. AverageUtilization +// is display-only here and degrades to 0 with a warning. +func (c *Client) parseRIUtilizationSignals(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) error { + // The field label carries service/account context so an error still + // identifies which row was corrupt. ctx := fmt.Sprintf("service=%s account=%s", rec.Service, rec.Account) - rec.AverageInstancesUsedPerHour = parseOptionalFloatOrWarn( - "AverageNumberOfInstancesUsedPerHour ("+ctx+")", details.AverageNumberOfInstancesUsedPerHour) + avg, err := parseOptionalFloat("AverageNumberOfInstancesUsedPerHour ("+ctx+")", details.AverageNumberOfInstancesUsedPerHour) + if err != nil { + return err + } + rec.AverageInstancesUsedPerHour = avg rec.RecommendedUtilization = parseOptionalFloatOrWarn( "AverageUtilization ("+ctx+")", details.AverageUtilization) + return nil } // parseRecommendedQuantity extracts the recommended quantity from details. The diff --git a/providers/aws/recommendations/parser_ri_test.go b/providers/aws/recommendations/parser_ri_test.go index 214ec005..a34166e9 100644 --- a/providers/aws/recommendations/parser_ri_test.go +++ b/providers/aws/recommendations/parser_ri_test.go @@ -586,9 +586,10 @@ func TestParseRecommendations_EmptyInput(t *testing.T) { // TestParseRIUtilizationSignals covers the AverageNumberOfInstancesUsedPerHour // and AverageUtilization fields added for issue #338 (--target-coverage). -// Verifies both successful parses, nil-pointer fallback to zero, and -// parse-failure fallback to zero — the sizing path in cmd/helpers.go treats -// zero as "no signal" so the fallback behavior matters. +// Verifies successful parses and nil-pointer fallback to zero. A present but +// invalid average-instances value is an error (zero means "no signal" to the +// sizing path, so it must not be fabricated); an invalid AverageUtilization is +// display-only and degrades to zero. func TestParseRIUtilizationSignals(t *testing.T) { client := &Client{} @@ -597,6 +598,7 @@ func TestParseRIUtilizationSignals(t *testing.T) { details *types.ReservationPurchaseRecommendationDetail wantAvgInstances float64 wantUtilization float64 + wantErr bool }{ { name: "both fields parsed", @@ -614,13 +616,12 @@ func TestParseRIUtilizationSignals(t *testing.T) { wantUtilization: 0, }, { - name: "unparseable AverageNumberOfInstancesUsedPerHour → that field zero, other still parses", + name: "unparseable AverageNumberOfInstancesUsedPerHour is an error", details: &types.ReservationPurchaseRecommendationDetail{ AverageNumberOfInstancesUsedPerHour: aws.String("not-a-number"), AverageUtilization: aws.String("90.0"), }, - wantAvgInstances: 0, - wantUtilization: 90.0, + wantErr: true, }, { name: "unparseable AverageUtilization → that field zero, other still parses", @@ -641,15 +642,22 @@ func TestParseRIUtilizationSignals(t *testing.T) { wantUtilization: 0, }, { - // NaN/Inf parse to a nil error under strconv.ParseFloat; they must - // degrade to 0, not be stored as a live signal (NaN <= 0 is false, - // so a stored NaN would drive NaN purchase counts in sizing). - name: "non-finite values degrade to zero", + // NaN parses to a nil error under strconv.ParseFloat; it must be + // rejected, not stored as a live signal (NaN <= 0 is false, so a + // stored NaN would drive NaN purchase counts in sizing). + name: "non-finite AverageNumberOfInstancesUsedPerHour is an error", details: &types.ReservationPurchaseRecommendationDetail{ AverageNumberOfInstancesUsedPerHour: aws.String("NaN"), + }, + wantErr: true, + }, + { + name: "non-finite AverageUtilization degrades to zero (display only)", + details: &types.ReservationPurchaseRecommendationDetail{ + AverageNumberOfInstancesUsedPerHour: aws.String("3"), AverageUtilization: aws.String("+Inf"), }, - wantAvgInstances: 0, + wantAvgInstances: 3, wantUtilization: 0, }, } @@ -657,7 +665,12 @@ func TestParseRIUtilizationSignals(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { rec := &common.Recommendation{} - client.parseRIUtilizationSignals(rec, tt.details) + err := client.parseRIUtilizationSignals(rec, tt.details) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) assert.Equal(t, tt.wantAvgInstances, rec.AverageInstancesUsedPerHour) assert.Equal(t, tt.wantUtilization, rec.RecommendedUtilization) }) @@ -767,3 +780,41 @@ func captureStdoutAndLog(t *testing.T, fn func()) (stdout, logged string) { require.NoError(t, r.Close()) return stdout, logBuf.String() } + +// FIXTURE-BASED (CE-shaped ReservationPurchaseRecommendationDetail, not live AWS). +// go#54: a present-but-invalid AverageNumberOfInstancesUsedPerHour used to +// become 0 = "no signal", so --target-coverage passed the rec through unsized +// and --min-pool-size kept it. It must now drop the detail and report it. +func TestParseRecommendations_InvalidAverageInstancesIsReportedNotZeroed(t *testing.T) { + detail := func(avg *string) types.ReservationPurchaseRecommendationDetail { + return types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("5"), + EstimatedMonthlySavingsAmount: aws.String("100.00"), + AverageNumberOfInstancesUsedPerHour: avg, + InstanceDetails: &types.InstanceDetails{EC2InstanceDetails: &types.EC2InstanceDetails{ + InstanceType: aws.String("m5.large"), Platform: aws.String("Linux/UNIX"), Region: aws.String("us-east-1"), + }}, + } + } + params := common.RecommendationParams{Service: common.ServiceEC2, Term: "1yr", PaymentOption: "no-upfront"} + for _, bad := range []string{"abc", "NaN", "-1", "Inf", ""} { + t.Run("invalid "+bad, func(t *testing.T) { + awsRecs := []types.ReservationPurchaseRecommendation{{RecommendationDetails: []types.ReservationPurchaseRecommendationDetail{ + detail(aws.String("10")), detail(aws.String(bad)), + }}} + recs, err := (&Client{}).parseRecommendations(context.Background(), awsRecs, params) + var incomplete *IncompleteRecommendationsError + require.ErrorAs(t, err, &incomplete) + assert.Equal(t, 1, incomplete.FailedDetails) + require.Len(t, recs, 1, "the valid sibling survives") + assert.Equal(t, 10.0, recs[0].AverageInstancesUsedPerHour) + }) + } + t.Run("nil stays a kept rec with no signal", func(t *testing.T) { + awsRecs := []types.ReservationPurchaseRecommendation{{RecommendationDetails: []types.ReservationPurchaseRecommendationDetail{detail(nil)}}} + recs, err := (&Client{}).parseRecommendations(context.Background(), awsRecs, params) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Zero(t, recs[0].AverageInstancesUsedPerHour) + }) +} diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 7aafbae6..e508330e 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -81,9 +81,10 @@ func parseOptionalFloat(field string, s *string) (float64, error) { // parseOptionalFloatOrWarn parses a *string as float64 but treats present-but- // unparseable as a non-fatal warning (logs and returns 0). Use ONLY for -// non-money fields (utilization percentages, averages) where a bad value -// degrades gracefully. For money fields use parseOptionalFloat and propagate -// the error. +// display-only fields (savings percentage, RI AverageUtilization, SP on-demand +// spend) where a bad value degrades gracefully. Money fields and any value a +// sizing path reads (average instances used, SP estimated utilization) must use +// parseOptionalFloat and propagate the error: 0 means "no signal" to sizing. func parseOptionalFloatOrWarn(field string, s *string) float64 { val, err := parseOptionalFloat(field, s) if err != nil { @@ -171,10 +172,14 @@ func (c *Client) parseSavingsPlanDetail( // two-tier treatment (hard error on cost, warn-and-continue on percentages). savingsPercent := parseOptionalFloatOrWarn("EstimatedSavingsPercentage", detail.EstimatedSavingsPercentage) // EstimatedAverageUtilization carries the "if you buy exactly this commitment, - // what % of it will AWS expect to be used" signal. Used by --target-coverage - // sizing in cmd/helpers.go; zero (nil pointer or parse failure) means "no signal" - // and the sizing path leaves the recommendation unchanged. - recommendedUtilization := parseOptionalFloatOrWarn("EstimatedAverageUtilization", detail.EstimatedAverageUtilization) + // what % of it will AWS expect to be used" signal. It is a sizing input: + // --target-coverage treats zero as "no signal" and leaves the recommendation + // unsized, so a nil pointer stays 0 but a present-but-invalid value fails the + // detail rather than degrading to 0. + recommendedUtilization, err := parseOptionalFloat("EstimatedAverageUtilization", detail.EstimatedAverageUtilization) + if err != nil { + return nil, err + } // onDemandCost is the canonical monthly on-demand baseline for this SP // recommendation. AWS Cost Explorer returns the average hourly on-demand // spend over the lookback period in CurrentAverageHourlyOnDemandSpend; diff --git a/providers/aws/recommendations/parser_sp_test.go b/providers/aws/recommendations/parser_sp_test.go index fb0f008e..e490c49a 100644 --- a/providers/aws/recommendations/parser_sp_test.go +++ b/providers/aws/recommendations/parser_sp_test.go @@ -167,6 +167,7 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) { utilizationStr *string wantUtilization float64 wantAvgInstancesIs float64 + wantErr bool }{ { name: "field present and parseable", @@ -181,10 +182,9 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) { wantAvgInstancesIs: 0, }, { - name: "field unparseable → zero (parseOptionalFloat logs warn)", - utilizationStr: aws.String("not-a-number"), - wantUtilization: 0, - wantAvgInstancesIs: 0, + name: "field unparseable is an error (sizing input, go#54)", + utilizationStr: aws.String("not-a-number"), + wantErr: true, }, } @@ -195,8 +195,11 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) { EstimatedAverageUtilization: tt.utilizationStr, } rec, err := client.parseSavingsPlanDetail(detail, ¶ms, types.SupportedSavingsPlansTypeComputeSp) - require.NoError(t, err, - "EstimatedAverageUtilization is a non-money field; parse failures must not propagate as errors") + if tt.wantErr { + require.Error(t, err, "EstimatedAverageUtilization is a sizing input; zero means no signal, so a bad value must fail the detail") + return + } + require.NoError(t, err) require.NotNil(t, rec) assert.Equal(t, tt.wantUtilization, rec.RecommendedUtilization, "SP utilization should be parsed into rec.RecommendedUtilization") @@ -503,3 +506,39 @@ func TestExtractEC2SPFieldsNormalizesRegion(t *testing.T) { assert.Empty(t, got.instanceFamily) }) } + +// FIXTURE-BASED (CE-shaped SavingsPlansPurchaseRecommendationDetail, not live AWS). +// go#54: a present-but-invalid EstimatedAverageUtilization used to become 0 = +// "no signal", so --target-coverage left the SP unsized at AWS's commitment. +func TestParseSavingsPlansRecommendations_InvalidEstimatedUtilizationIsReported(t *testing.T) { + detail := func(util *string) types.SavingsPlansPurchaseRecommendationDetail { + return types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("1.5"), + EstimatedMonthlySavingsAmount: aws.String("100"), + UpfrontCost: aws.String("0"), + EstimatedAverageUtilization: util, + } + } + params := &common.RecommendationParams{Service: common.ServiceSavingsPlansCompute, Term: "1yr", PaymentOption: "no-upfront"} + planType := types.SupportedSavingsPlansTypeComputeSp + for _, bad := range []string{"abc", "NaN", "-1", "Inf", ""} { + t.Run("invalid "+bad, func(t *testing.T) { + spRec := &types.SavingsPlansPurchaseRecommendation{SavingsPlansPurchaseRecommendationDetails: []types.SavingsPlansPurchaseRecommendationDetail{ + detail(aws.String("90")), detail(aws.String(bad)), + }} + recs, err := (&Client{}).parseSavingsPlansRecommendations(spRec, params, planType, 0) + var incomplete *IncompleteRecommendationsError + require.ErrorAs(t, err, &incomplete) + assert.Equal(t, 1, incomplete.FailedDetails) + require.Len(t, recs, 1) + assert.Equal(t, 90.0, recs[0].RecommendedUtilization) + }) + } + t.Run("nil stays a kept rec with no signal", func(t *testing.T) { + spRec := &types.SavingsPlansPurchaseRecommendation{SavingsPlansPurchaseRecommendationDetails: []types.SavingsPlansPurchaseRecommendationDetail{detail(nil)}} + recs, err := (&Client{}).parseSavingsPlansRecommendations(spRec, params, planType, 0) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Zero(t, recs[0].RecommendedUtilization) + }) +}