From 5742a87ab1a71edd05172faa2e4ba05d5fbb045c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 21:10:03 +0200 Subject: [PATCH] fix(aws/recommendations): reject NaN/Inf in SP money parsing (follow-up to #1455) parseOptionalFloat used strconv.ParseFloat, which accepts "NaN", "Inf", "+Inf", "-Inf" (and Infinity variants) as valid with a nil error. A Cost Explorer money field carrying one of these would therefore flow straight through as a corrupt non-finite value into the purchase money fields (HourlyCommitmentToPurchase, EstimatedMonthlySavingsAmount, UpfrontCost) and, via parseOptionalFloatOrWarn, into the OnDemandCost baseline. Reject non-finite values at the parse boundary the same way as unparseable ones (math.IsNaN / math.IsInf -> error). This is the root-cause fix for both unresolved CodeRabbit Major threads on #1455: - money fields: NaN/Inf now error -> the recommendation is dropped, no corrupt figure reaches the scheduler/frontend. - OnDemandCost (CurrentAverageHourlyOnDemandSpend): NaN previously survived parseOptionalFloatOrWarn as NaN; it now degrades to 0 -> nil (frontend reconstruction), matching the documented "unavailable" path. Tests: - Extended TestParseSavingsPlanDetail_MoneyFieldUnparseable with NaN/+Inf/-Inf money-field cases. - New TestParseOptionalFloat_RejectsNonFinite unit guard. Verified both FAIL without the guard and PASS with it. --- providers/aws/recommendations/parser_sp.go | 9 ++++ .../aws/recommendations/parser_sp_test.go | 44 ++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 87ec21468..d6524ff9a 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log" + "math" "strconv" "strings" "time" @@ -192,6 +193,11 @@ func (c *Client) parseSavingsPlansRecommendations( // the same class rather than silently substituting 0. Callers must propagate // this error and drop the recommendation so a corrupt money figure never // reaches the scheduler or frontend. +// - non-nil pointer that parses to NaN or ±Inf: returns (0, error). Go's +// strconv.ParseFloat accepts "NaN", "Inf", "+Inf", "-Inf" (and Infinity +// 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. func parseOptionalFloat(field string, s *string) (float64, error) { if s == nil { return 0, nil @@ -200,6 +206,9 @@ func parseOptionalFloat(field string, s *string) (float64, error) { if err != nil { return 0, fmt.Errorf("failed to parse %s %q: %w", field, *s, err) } + if math.IsNaN(val) || math.IsInf(val, 0) { + return 0, fmt.Errorf("%s %q is not a finite number", field, *s) + } return val, nil } diff --git a/providers/aws/recommendations/parser_sp_test.go b/providers/aws/recommendations/parser_sp_test.go index 3065fc32f..422af406d 100644 --- a/providers/aws/recommendations/parser_sp_test.go +++ b/providers/aws/recommendations/parser_sp_test.go @@ -350,15 +350,55 @@ func TestParseSavingsPlanDetail_MoneyFieldUnparseable(t *testing.T) { UpfrontCost: aws.String("not-a-float"), }, }, + // NaN / ±Inf are accepted by strconv.ParseFloat with a nil error, so + // without an explicit non-finite guard these would flow through as a + // corrupt money value. They must be rejected like any unparseable field. + { + name: "NaN HourlyCommitmentToPurchase", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("NaN"), + }, + }, + { + name: "Inf EstimatedMonthlySavingsAmount", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("1.0"), + EstimatedMonthlySavingsAmount: aws.String("+Inf"), + }, + }, + { + name: "negative Inf UpfrontCost", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("1.0"), + UpfrontCost: aws.String("-Inf"), + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { rec, err := client.parseSavingsPlanDetail(tt.detail, ¶ms, types.SupportedSavingsPlansTypeComputeSp) require.Error(t, err, - "present-but-unparseable money field must return an error, not a silently-fabricated $0") + "present-but-unparseable or non-finite money field must return an error, not a silently-fabricated value") assert.Nil(t, rec, - "rec must be nil when a money field is unparseable") + "rec must be nil when a money field is unparseable or non-finite") + }) + } +} + +// 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. +func TestParseOptionalFloat_RejectsNonFinite(t *testing.T) { + for _, s := range []string{"NaN", "Inf", "+Inf", "-Inf", "Infinity", "-Infinity"} { + 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) + assert.Zero(t, v) }) } + // Sanity: a normal finite value still parses. + v, err := parseOptionalFloat("TestField", aws.String("12.5")) + require.NoError(t, err) + assert.InDelta(t, 12.5, v, 0.0001) }