From f9d430bf389632a5c279d7719d56ddd149b7862f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 20 Jul 2026 15:47:47 +0200 Subject: [PATCH 1/3] fix(aws/recommendations): close NaN/Inf/negative gap on the RI parser too Addresses the Fable adversarial-review finding on this PR: the SP-path non-finite guard did not fully close the bug class. The RI sibling parser (parseCostInformation, parseAWSCostDetails) parsed the same CE money fields (EstimatedMonthlySavingsAmount/Percentage, UpfrontCost, EstimatedMonthlyOnDemandCost, RecurringStandardMonthlyCost) with bare strconv.ParseFloat and no guard, so a "NaN"/"Inf" value still flowed into CommitmentCost/OnDemandCost/savings/RecurringMonthlyCost. The doc comments also claimed the SP path "mirrors the RI path", which was false after the first commit. Changes: - Route both RI money parsers through the shared parseOptionalFloat, restoring genuine SP/RI parity (comments now hold). - parseOptionalFloat also rejects negative values (every field it parses is a non-negative financial metric; mirrors the repo's stricter parseSPFloat). - utilization.go parseFloat degrades NaN/Inf to 0 with a warning, so a single non-finite hours value can't poison every downstream utilization aggregate. Tests: - New TestRICostParsers_RejectNonFiniteAndNegative (verified it fails when the RI routing is reverted). - Extended TestParseOptionalFloat_RejectsNonFinite with negative cases + a zero-is-valid sanity check. - Updated TestParseRecommendationDetail_MalformedCostFields error-string assertions to the now-unified "failed to parse ..." wording. --- providers/aws/recommendations/parser_ri.go | 38 +++++++------- .../aws/recommendations/parser_ri_test.go | 6 +-- providers/aws/recommendations/parser_sp.go | 7 +++ .../aws/recommendations/parser_sp_test.go | 51 +++++++++++++++++-- providers/aws/recommendations/utilization.go | 9 ++++ 5 files changed, 83 insertions(+), 28 deletions(-) diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 6c8148d63..4cf253955 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -132,22 +132,17 @@ func (c *Client) parseRecommendedQuantity(details *types.ReservationPurchaseReco // historical on-demand demand. This is the 100%-coverage baseline the dashboard // scaling in summarizeRecommendationsWithCoverage depends on (issue #215 audit). func (c *Client) parseCostInformation(details *types.ReservationPurchaseRecommendationDetail) (float64, float64, error) { - var estimatedSavings, savingsPercent float64 - - if details.EstimatedMonthlySavingsAmount != nil { - val, err := strconv.ParseFloat(*details.EstimatedMonthlySavingsAmount, 64) - if err != nil { - return 0, 0, fmt.Errorf("failed to parse estimated savings %q: %w", *details.EstimatedMonthlySavingsAmount, err) - } - estimatedSavings = val + // Route through parseOptionalFloat so a present-but-non-finite CE money value + // (NaN/Inf parse to a nil error under strconv.ParseFloat) is rejected the same + // way as on the SP path, keeping this parser and the SP parser at genuine + // parity. A nil pointer yields (0, nil). + estimatedSavings, err := parseOptionalFloat("EstimatedMonthlySavingsAmount", details.EstimatedMonthlySavingsAmount) + if err != nil { + return 0, 0, err } - - if details.EstimatedMonthlySavingsPercentage != nil { - val, err := strconv.ParseFloat(*details.EstimatedMonthlySavingsPercentage, 64) - if err != nil { - return 0, 0, fmt.Errorf("failed to parse savings percentage %q: %w", *details.EstimatedMonthlySavingsPercentage, err) - } - savingsPercent = val + savingsPercent, err := parseOptionalFloat("EstimatedMonthlySavingsPercentage", details.EstimatedMonthlySavingsPercentage) + if err != nil { + return 0, 0, err } return estimatedSavings, savingsPercent, nil @@ -160,17 +155,18 @@ func (c *Client) parseCostInformation(details *types.ReservationPurchaseRecommen // wrong money figure (e.g. a $0 upfront on an all-upfront RI) into the // effective-savings math and purchase decisions with no signal. func (c *Client) parseAWSCostDetails(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) error { + // All money fields route through parseOptionalFloat for the non-finite guard. if details.UpfrontCost != nil { - upfront, err := strconv.ParseFloat(*details.UpfrontCost, 64) + upfront, err := parseOptionalFloat("UpfrontCost", details.UpfrontCost) if err != nil { - return fmt.Errorf("failed to parse upfront cost %q: %w", *details.UpfrontCost, err) + return err } rec.CommitmentCost = upfront } if details.EstimatedMonthlyOnDemandCost != nil { - onDemand, err := strconv.ParseFloat(*details.EstimatedMonthlyOnDemandCost, 64) + onDemand, err := parseOptionalFloat("EstimatedMonthlyOnDemandCost", details.EstimatedMonthlyOnDemandCost) if err != nil { - return fmt.Errorf("failed to parse estimated monthly on-demand cost %q: %w", *details.EstimatedMonthlyOnDemandCost, err) + return err } rec.OnDemandCost = onDemand } else { @@ -183,9 +179,9 @@ func (c *Client) parseAWSCostDetails(rec *common.Recommendation, details *types. // RecurringStandardMonthlyCost is the recurring charge per month for this RI. // It is distinct from CommitmentCost (upfront) and EstimatedMonthlySavingsAmount. if details.RecurringStandardMonthlyCost != nil { - monthly, err := strconv.ParseFloat(*details.RecurringStandardMonthlyCost, 64) + monthly, err := parseOptionalFloat("RecurringStandardMonthlyCost", details.RecurringStandardMonthlyCost) if err != nil { - return fmt.Errorf("failed to parse recurring standard monthly cost %q: %w", *details.RecurringStandardMonthlyCost, err) + return err } rec.RecurringMonthlyCost = &monthly } diff --git a/providers/aws/recommendations/parser_ri_test.go b/providers/aws/recommendations/parser_ri_test.go index 8d1ecc2cf..5c46162f9 100644 --- a/providers/aws/recommendations/parser_ri_test.go +++ b/providers/aws/recommendations/parser_ri_test.go @@ -305,21 +305,21 @@ func TestParseRecommendationDetail_MalformedCostFields(t *testing.T) { mutate: func(d *types.ReservationPurchaseRecommendationDetail) { d.UpfrontCost = aws.String("not-a-number") }, - errContains: `failed to parse upfront cost "not-a-number"`, + errContains: `failed to parse UpfrontCost "not-a-number"`, }, { name: "malformed on-demand cost", mutate: func(d *types.ReservationPurchaseRecommendationDetail) { d.EstimatedMonthlyOnDemandCost = aws.String("$650.00") }, - errContains: `failed to parse estimated monthly on-demand cost "$650.00"`, + errContains: `failed to parse EstimatedMonthlyOnDemandCost "$650.00"`, }, { name: "malformed recurring monthly cost", mutate: func(d *types.ReservationPurchaseRecommendationDetail) { d.RecurringStandardMonthlyCost = aws.String("") }, - errContains: `failed to parse recurring standard monthly cost ""`, + errContains: `failed to parse RecurringStandardMonthlyCost ""`, }, } diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index d683b8920..33d977cd9 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -198,6 +198,10 @@ func (c *Client) parseSavingsPlansRecommendations( // variants) as valid with a nil error, so without this guard a corrupt // non-finite money value would flow straight through to the scheduler and // frontend. Reject non-finite values the same way as unparseable ones. +// - non-nil pointer that parses to a negative value: returns (0, error). Every +// field this parses (SP/RI cost, commitment, savings, savings %, utilization) +// is non-negative by construction; a negative is corrupt in the same way a +// non-finite value is. Mirrors the repo's stricter parseSPFloat helper. func parseOptionalFloat(field string, s *string) (float64, error) { if s == nil { return 0, nil @@ -209,6 +213,9 @@ func parseOptionalFloat(field string, s *string) (float64, error) { if math.IsNaN(val) || math.IsInf(val, 0) { return 0, fmt.Errorf("%s %q is not a finite number", field, *s) } + if val < 0 { + return 0, fmt.Errorf("%s %q is negative, which is invalid for a financial metric", field, *s) + } return val, nil } diff --git a/providers/aws/recommendations/parser_sp_test.go b/providers/aws/recommendations/parser_sp_test.go index 422af406d..65cc1473b 100644 --- a/providers/aws/recommendations/parser_sp_test.go +++ b/providers/aws/recommendations/parser_sp_test.go @@ -388,17 +388,60 @@ func TestParseSavingsPlanDetail_MoneyFieldUnparseable(t *testing.T) { // TestParseOptionalFloat_RejectsNonFinite is the unit-level guard for the // NaN/±Inf money-parsing fix: strconv.ParseFloat accepts these strings with a -// nil error, so parseOptionalFloat must reject them explicitly. +// nil error, so parseOptionalFloat must reject them explicitly. It also rejects +// negative values (invalid for the non-negative financial metrics it parses). func TestParseOptionalFloat_RejectsNonFinite(t *testing.T) { - for _, s := range []string{"NaN", "Inf", "+Inf", "-Inf", "Infinity", "-Infinity"} { + for _, s := range []string{"NaN", "Inf", "+Inf", "-Inf", "Infinity", "-Infinity", "-5", "-0.01"} { t.Run(s, func(t *testing.T) { v, err := parseOptionalFloat("TestField", aws.String(s)) - require.Error(t, err, "non-finite value %q must be rejected", s) + require.Error(t, err, "invalid value %q must be rejected", s) assert.Zero(t, v) }) } - // Sanity: a normal finite value still parses. + // Sanity: a normal finite value still parses; zero is valid. v, err := parseOptionalFloat("TestField", aws.String("12.5")) require.NoError(t, err) assert.InDelta(t, 12.5, v, 0.0001) + z, err := parseOptionalFloat("TestField", aws.String("0")) + require.NoError(t, err) + assert.Zero(t, z) +} + +// TestRICostParsers_RejectNonFiniteAndNegative verifies the RI money parsers +// (parseCostInformation, parseAWSCostDetails) reject NaN/Inf/negative CE money +// values -- the same class the SP path rejects -- since both now route through +// parseOptionalFloat. Regression guard for the incomplete-fix finding on #1461. +func TestRICostParsers_RejectNonFiniteAndNegative(t *testing.T) { + client := &Client{} + + costInfoCases := []struct { + name string + details *types.ReservationPurchaseRecommendationDetail + }{ + {"NaN savings amount", &types.ReservationPurchaseRecommendationDetail{EstimatedMonthlySavingsAmount: aws.String("NaN")}}, + {"Inf savings pct", &types.ReservationPurchaseRecommendationDetail{EstimatedMonthlySavingsPercentage: aws.String("+Inf")}}, + {"negative savings amount", &types.ReservationPurchaseRecommendationDetail{EstimatedMonthlySavingsAmount: aws.String("-10")}}, + } + for _, tc := range costInfoCases { + t.Run("parseCostInformation/"+tc.name, func(t *testing.T) { + _, _, err := client.parseCostInformation(tc.details) + require.Error(t, err) + }) + } + + costDetailCases := []struct { + name string + details *types.ReservationPurchaseRecommendationDetail + }{ + {"NaN upfront", &types.ReservationPurchaseRecommendationDetail{UpfrontCost: aws.String("NaN")}}, + {"Inf on-demand", &types.ReservationPurchaseRecommendationDetail{EstimatedMonthlyOnDemandCost: aws.String("-Inf")}}, + {"negative recurring", &types.ReservationPurchaseRecommendationDetail{RecurringStandardMonthlyCost: aws.String("-1.5")}}, + } + for _, tc := range costDetailCases { + t.Run("parseAWSCostDetails/"+tc.name, func(t *testing.T) { + var rec common.Recommendation + err := client.parseAWSCostDetails(&rec, tc.details) + require.Error(t, err) + }) + } } diff --git a/providers/aws/recommendations/utilization.go b/providers/aws/recommendations/utilization.go index 2e8fa14dc..c7307d0bb 100644 --- a/providers/aws/recommendations/utilization.go +++ b/providers/aws/recommendations/utilization.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log" + "math" "strconv" "time" @@ -153,5 +154,13 @@ func parseFloat(s string) float64 { log.Printf("warning: failed to parse float %q: %v", s, err) return 0 } + // Degrade non-finite values to 0: strconv.ParseFloat accepts "NaN"/"Inf" + // with a nil error, and a single NaN accumulated into purchasedHours / + // totalActualHours / unusedHours would poison every downstream utilization + // aggregate (NaN propagates through all arithmetic). + if math.IsNaN(f) || math.IsInf(f, 0) { + log.Printf("warning: non-finite float %q treated as 0", s) + return 0 + } return f } From 3203771e181a80e108e7da71be92a02647e89dff Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 20 Jul 2026 16:00:54 +0200 Subject: [PATCH 2/3] fix(aws/recommendations): guard remaining non-finite float parses in package Second round of the Fable adversarial review on this PR: it found the same non-finite bug class still open at three more parse sites in the recommendations package that the first two commits missed. - ondemand_series.go accumulateDailyResults: a "NaN"/"Inf" CE daily amount parsed with a nil error and slipped past the downstream all-zero fail-loud check (NaN != 0), poisoning the ladder baseline the engine sizes purchases from. Now fails loud on non-finite. NO negative guard here -- CE unblended cost is legitimately negative on credit/refund days. - parser_ri.go parseRIUtilizationSignals: NaN/Inf were stored into AverageInstancesUsedPerHour / RecommendedUtilization; the --target-coverage guards are all `<= 0` and NaN <= 0 is false, so a stored NaN was treated as a live signal and produced NaN purchase counts. Now routed through parseOptionalFloatOrWarn (warn + 0). - parser_ri.go parseRecommendedQuantity: fmt.Sscanf %f also accepts NaN/Inf; a non-finite quantity would corrupt the purchase count. Fail loud. Tests: - ondemand_series_test.go: TestGetOnDemandSeries_NonFiniteAmountFails. - parser_ri_test.go: non-finite-degrades-to-zero case in the utilization-signals table. - parser_sp_test.go: TestUtilizationParseFloat_DegradesNonFinite for the utilization.go parseFloat guard added in the previous commit. --- .../aws/recommendations/ondemand_series.go | 10 +++++++ .../recommendations/ondemand_series_test.go | 20 +++++++++++++ providers/aws/recommendations/parser_ri.go | 28 +++++++++---------- .../aws/recommendations/parser_ri_test.go | 12 ++++++++ .../aws/recommendations/parser_sp_test.go | 10 +++++++ 5 files changed, 66 insertions(+), 14 deletions(-) diff --git a/providers/aws/recommendations/ondemand_series.go b/providers/aws/recommendations/ondemand_series.go index 159d7a0ee..d6b09dc5c 100644 --- a/providers/aws/recommendations/ondemand_series.go +++ b/providers/aws/recommendations/ondemand_series.go @@ -3,6 +3,7 @@ package recommendations import ( "context" "fmt" + "math" "sort" "strconv" "time" @@ -249,6 +250,15 @@ func accumulateDailyResults(byDate map[string]float64, out *costexplorer.GetCost return fmt.Errorf("cannot parse CE amount %q for day %s: %w", aws.ToString(mv.Amount), dateStr, err) } + // strconv.ParseFloat accepts "NaN"/"Inf" with a nil error; a non-finite + // daily amount would slip past the downstream all-zero fail-loud check + // (NaN != 0) and poison the ladder baseline. Fail loud here instead. No + // negative guard: CE unblended cost is legitimately negative on + // credit/refund days. + if math.IsNaN(usd) || math.IsInf(usd, 0) { + return fmt.Errorf("CE amount %q for day %s is not a finite number", + aws.ToString(mv.Amount), dateStr) + } // Divide total daily USD by 24 to get USD/hr. Derived from DAILY // granularity: 1 day = 24 hours; not a magic constant. byDate[dateStr] = usd / 24.0 diff --git a/providers/aws/recommendations/ondemand_series_test.go b/providers/aws/recommendations/ondemand_series_test.go index b5f8d582c..d7d7e2b29 100644 --- a/providers/aws/recommendations/ondemand_series_test.go +++ b/providers/aws/recommendations/ondemand_series_test.go @@ -248,6 +248,26 @@ func TestGetOnDemandSeries_ParseErrorFails(t *testing.T) { assert.Contains(t, err.Error(), "cannot parse CE amount") } +// TestGetOnDemandSeries_NonFiniteAmountFails verifies that a "NaN"/"Inf" CE +// amount (which strconv.ParseFloat accepts with a nil error) fails loud rather +// than poisoning the daily series that seeds the ladder baseline. +func TestGetOnDemandSeries_NonFiniteAmountFails(t *testing.T) { + for _, bad := range []string{"NaN", "Inf", "-Inf"} { + t.Run(bad, func(t *testing.T) { + pages := []*costexplorer.GetCostAndUsageOutput{{ + ResultsByTime: []types.ResultByTime{{ + TimePeriod: &types.DateInterval{Start: aws.String("2026-01-01")}, + Total: map[string]types.MetricValue{onDemandMetric: {Amount: aws.String(bad)}}, + }}, + }} + client := newOnDemandClient(&mockOnDemandCE{pages: pages}) + _, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a finite number") + }) + } +} + // TestGetOnDemandSeries_BadDateFails verifies that a malformed CE period-start // date fails loud instead of being skipped or misdated // (feedback_no_silent_fallbacks). diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 4cf253955..35c13f617 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -89,20 +89,15 @@ func (c *Client) parseRecommendationDetail(ctx context.Context, details *types.R // 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) { - if details.AverageNumberOfInstancesUsedPerHour != nil { - if v, err := strconv.ParseFloat(*details.AverageNumberOfInstancesUsedPerHour, 64); err == nil { - rec.AverageInstancesUsedPerHour = v - } else { - log.Printf("WARNING: failed to parse AverageNumberOfInstancesUsedPerHour %q for RI recommendation (service=%s, account=%s): %v", *details.AverageNumberOfInstancesUsedPerHour, rec.Service, rec.Account, err) - } - } - if details.AverageUtilization != nil { - if v, err := strconv.ParseFloat(*details.AverageUtilization, 64); err == nil { - rec.RecommendedUtilization = v - } else { - log.Printf("WARNING: failed to parse AverageUtilization %q for RI recommendation (service=%s, account=%s): %v", *details.AverageUtilization, rec.Service, rec.Account, err) - } - } + // 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. + rec.AverageInstancesUsedPerHour = parseOptionalFloatOrWarn( + "AverageNumberOfInstancesUsedPerHour", details.AverageNumberOfInstancesUsedPerHour) + rec.RecommendedUtilization = parseOptionalFloatOrWarn( + "AverageUtilization", details.AverageUtilization) } // parseRecommendedQuantity extracts the recommended quantity from details @@ -121,6 +116,11 @@ func (c *Client) parseRecommendedQuantity(details *types.ReservationPurchaseReco } return 0, fmt.Errorf("failed to parse quantity '%s' as float or int", qty) } + // Sscanf %f accepts "NaN"/"Inf"; a non-finite quantity would corrupt the + // purchase count (int(math.Round(NaN)) is undefined). Fail loud. + if math.IsNaN(count) || math.IsInf(count, 0) { + return 0, fmt.Errorf("recommended quantity %q is not a finite number", qty) + } return int(math.Round(count)), nil } diff --git a/providers/aws/recommendations/parser_ri_test.go b/providers/aws/recommendations/parser_ri_test.go index 5c46162f9..aabcb80b0 100644 --- a/providers/aws/recommendations/parser_ri_test.go +++ b/providers/aws/recommendations/parser_ri_test.go @@ -541,6 +541,18 @@ func TestParseRIUtilizationSignals(t *testing.T) { wantAvgInstances: 0, 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", + details: &types.ReservationPurchaseRecommendationDetail{ + AverageNumberOfInstancesUsedPerHour: aws.String("NaN"), + AverageUtilization: aws.String("+Inf"), + }, + wantAvgInstances: 0, + wantUtilization: 0, + }, } for _, tt := range tests { diff --git a/providers/aws/recommendations/parser_sp_test.go b/providers/aws/recommendations/parser_sp_test.go index 65cc1473b..a6a84b903 100644 --- a/providers/aws/recommendations/parser_sp_test.go +++ b/providers/aws/recommendations/parser_sp_test.go @@ -407,6 +407,16 @@ func TestParseOptionalFloat_RejectsNonFinite(t *testing.T) { assert.Zero(t, z) } +// TestUtilizationParseFloat_DegradesNonFinite guards the warn-and-zero +// utilization parser: NaN/Inf must degrade to 0 so a single non-finite hours +// value can't poison the utilization aggregates (NaN propagates through sums). +func TestUtilizationParseFloat_DegradesNonFinite(t *testing.T) { + for _, s := range []string{"NaN", "Inf", "+Inf", "-Inf", "not-a-number"} { + assert.Zero(t, parseFloat(s), "non-finite/unparseable %q must degrade to 0", s) + } + assert.InDelta(t, 42.5, parseFloat("42.5"), 0.0001) +} + // TestRICostParsers_RejectNonFiniteAndNegative verifies the RI money parsers // (parseCostInformation, parseAWSCostDetails) reject NaN/Inf/negative CE money // values -- the same class the SP path rejects -- since both now route through From 59465577f9d9f17fe46f6713fe8f098e086841f3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 20 Jul 2026 16:29:06 +0200 Subject: [PATCH 3/3] fix(aws): guard non-finite/negative in ladder SP commitment + RI quantity (Fable follow-ups) Addresses the Fable open-PR review findings on this PR: - ladder/adapters.go mapActiveSP (MED, live money path): SavingsPlan.Commitment parsed with bare strconv.ParseFloat and no non-finite guard, despite the doc claiming it "fails loud on a non-numeric Commitment (money path)". A NaN/Inf commitment flowed through sumSPHourlyCost / sumExpiringSPHourlyCost into the ladder layer-state totals the engine sizes purchases from. Now rejects NaN/Inf/negative (a commitment is a non-negative money rate). - parser_ri.go parseRecommendedQuantity (LOW): rejected non-finite but still accepted a negative purchase count ("-3" via Sscanf/Atoi). Now rejects negatives. - parser_ri.go parseRIUtilizationSignals (LOW): the refactor dropped service/account context from the parse-failure warning; the field label now carries "service=.. account=.." so an operator can identify the corrupt row. Regression tests: TestSPLister_NonFiniteOrNegativeCommitmentFails (verified fails when the guard is reverted), plus NaN/Inf/negative cases in the quantity table. --- providers/aws/ladder/adapters.go | 9 ++++++ providers/aws/ladder/adapters_test.go | 19 +++++++++++ providers/aws/recommendations/parser_ri.go | 18 ++++++++--- .../aws/recommendations/parser_ri_test.go | 32 +++++++++++++++++++ 4 files changed, 73 insertions(+), 5 deletions(-) diff --git a/providers/aws/ladder/adapters.go b/providers/aws/ladder/adapters.go index 4c2708f56..1b858c1c1 100644 --- a/providers/aws/ladder/adapters.go +++ b/providers/aws/ladder/adapters.go @@ -3,6 +3,7 @@ package ladder import ( "context" "fmt" + "math" "strconv" "time" @@ -158,6 +159,14 @@ func mapActiveSP(sp sptypes.SavingsPlan) (ActiveSP, error) { return ActiveSP{}, fmt.Errorf("ListActiveSPs: cannot parse Commitment %q for SP %s: %w", commitment, *sp.SavingsPlanId, err) } + // strconv.ParseFloat accepts "NaN"/"Inf" with a nil error; a non-finite or + // negative hourly commitment would flow through sumSPHourlyCost / + // sumExpiringSPHourlyCost into the ladder layer-state totals the engine + // sizes purchases from. Fail loud (a commitment is a non-negative money rate). + if math.IsNaN(hourly) || math.IsInf(hourly, 0) || hourly < 0 { + return ActiveSP{}, fmt.Errorf("ListActiveSPs: Commitment %q for SP %s is not a finite non-negative number", + commitment, *sp.SavingsPlanId) + } start, err := parseSPDate("Start", sp.Start, *sp.SavingsPlanId) if err != nil { return ActiveSP{}, err diff --git a/providers/aws/ladder/adapters_test.go b/providers/aws/ladder/adapters_test.go index 743bedc36..a82d3764c 100644 --- a/providers/aws/ladder/adapters_test.go +++ b/providers/aws/ladder/adapters_test.go @@ -268,6 +268,25 @@ func TestSPLister_InvalidCommitmentFails(t *testing.T) { assert.Contains(t, err.Error(), "cannot parse Commitment") } +// TestSPLister_NonFiniteOrNegativeCommitmentFails verifies that a "NaN"/"Inf" +// or negative Commitment (which strconv.ParseFloat accepts / passes through) +// fails loud rather than flowing into the ladder layer-state SP-cost totals. +func TestSPLister_NonFiniteOrNegativeCommitmentFails(t *testing.T) { + for _, bad := range []string{"NaN", "Inf", "-Inf", "-1.5"} { + t.Run(bad, func(t *testing.T) { + sp := makeSPEntry("sp-nonfinite", string(sptypes.SavingsPlanTypeCompute), bad, "", sptypes.SavingsPlanStateActive) + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, + } + lister := newSPLister(mock) + + _, err := lister.ListActiveSPs(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a finite non-negative number") + }) + } +} + // TestSPLister_BadDatesFail verifies fail-loud on missing or unparseable // Start/End dates: a silently zero EndDate would drop the SP from // sumExpiringSPHourlyCost and understate expiring commitment. diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 35c13f617..a8d7ff3d4 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -94,10 +94,14 @@ func (c *Client) parseRIUtilizationSignals(rec *common.Recommendation, details * // 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). + ctx := fmt.Sprintf("service=%s account=%s", rec.Service, rec.Account) rec.AverageInstancesUsedPerHour = parseOptionalFloatOrWarn( - "AverageNumberOfInstancesUsedPerHour", details.AverageNumberOfInstancesUsedPerHour) + "AverageNumberOfInstancesUsedPerHour ("+ctx+")", details.AverageNumberOfInstancesUsedPerHour) rec.RecommendedUtilization = parseOptionalFloatOrWarn( - "AverageUtilization", details.AverageUtilization) + "AverageUtilization ("+ctx+")", details.AverageUtilization) } // parseRecommendedQuantity extracts the recommended quantity from details @@ -112,14 +116,18 @@ func (c *Client) parseRecommendedQuantity(details *types.ReservationPurchaseReco _, err := fmt.Sscanf(qty, "%f", &count) if err != nil { if intCount, atoiErr := strconv.Atoi(qty); atoiErr == nil { + if intCount < 0 { + return 0, fmt.Errorf("recommended quantity %q is negative", qty) + } return intCount, nil } return 0, fmt.Errorf("failed to parse quantity '%s' as float or int", qty) } // Sscanf %f accepts "NaN"/"Inf"; a non-finite quantity would corrupt the - // purchase count (int(math.Round(NaN)) is undefined). Fail loud. - if math.IsNaN(count) || math.IsInf(count, 0) { - return 0, fmt.Errorf("recommended quantity %q is not a finite number", qty) + // purchase count (int(math.Round(NaN)) is undefined). A negative count is + // likewise invalid for a purchase quantity. Fail loud on both. + if math.IsNaN(count) || math.IsInf(count, 0) || count < 0 { + return 0, fmt.Errorf("recommended quantity %q is not a finite non-negative number", qty) } return int(math.Round(count)), nil diff --git a/providers/aws/recommendations/parser_ri_test.go b/providers/aws/recommendations/parser_ri_test.go index aabcb80b0..3ab72efff 100644 --- a/providers/aws/recommendations/parser_ri_test.go +++ b/providers/aws/recommendations/parser_ri_test.go @@ -77,6 +77,38 @@ func TestParseRecommendedQuantity(t *testing.T) { expected: 0, expectError: true, }, + { + name: "NaN quantity rejected", + details: &types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("NaN"), + }, + expected: 0, + expectError: true, + }, + { + name: "Inf quantity rejected", + details: &types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("+Inf"), + }, + expected: 0, + expectError: true, + }, + { + name: "negative float quantity rejected", + details: &types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("-3.0"), + }, + expected: 0, + expectError: true, + }, + { + name: "negative int quantity rejected", + details: &types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("-3"), + }, + expected: 0, + expectError: true, + }, } for _, tt := range tests {