Repository navigation
fix(aws/recommendations): reject NaN/Inf in SP money parsing (follow-up to #1455) - #1461
Merged
Merged
Conversation
…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.
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSavings Plans float parsing now rejects NaN and infinite values. Tests cover invalid non-finite money fields, direct optional-float parsing, and continued acceptance of finite numbers. ChangesSavings Plan float validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
Addressed the Fable adversarial-review finding: the RI sibling parser ( @coderabbitai review |
Contributor
|
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 20, 2026
…_ri, ondemand_series, RI-utilization (follow-up to #1461) (#1473) * 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 <Field> ..." wording. * 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. * 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.
cristim
added a commit
that referenced
this pull request
Jul 21, 2026
The prior commit's code comments cited "#1461" as the tracking issue for the RI utilization filter bug. #1461 is an unrelated PR (NaN/Inf rejection in SP money parsing). The bug this fix addresses was introduced by PR #1361, per the review that surfaced it; correct all comment references accordingly.
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
The prior commit's code comments cited "#1461" as the tracking issue for the RI utilization filter bug. #1461 is an unrelated PR (NaN/Inf rejection in SP money parsing). The bug this fix addresses was introduced by PR #1361, per the review that surfaced it; correct all comment references accordingly.
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
… decisions (follow-up to #1361) (#1479) * fix(aws/ladder): scope RI utilization query to EC2+region for reshape decisions GetRIUtilization built GetReservationUtilization with no Filter at all, so it blended utilization across every reserved-resource type in the account (RDS, ElastiCache, OpenSearch, Redshift, standard EC2 RIs) and every region into one SUBSCRIPTION_ID-grouped number. The ladder ConvertibleRI (buffer) layer reads this as if it were EC2-convertible utilization for one account/region, then uses it to decide whether to trigger a real RI reshape/exchange -- an underutilized RI in an unrelated service or region could trigger a reshape, or mask a genuinely poorly-utilized convertible RI. Add a SERVICE+REGION Filter to the CE call, mirroring the same pattern already used by GetRICoverageMap in coverage.go. Also intersect the (now EC2+region-scoped) response against the account's own convertible RI IDs before aggregating in GetLayerStates, since a standard (non-convertible) EC2 RI in the same account/region would otherwise still pass the SERVICE+REGION filter and blend into the layer's UtilizationPct. Threads a region parameter through GetRIUtilization's callers (providers/aws/ladder, providers/aws/service_client.go, internal/server, internal/api). Follow-up to #1361. * fix(aws/ladder): correct comment reference from #1461 to #1361 The prior commit's code comments cited "#1461" as the tracking issue for the RI utilization filter bug. #1461 is an unrelated PR (NaN/Inf rejection in SP money parsing). The bug this fix addresses was introduced by PR #1361, per the review that surfaced it; correct all comment references accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1455 — two unaddressed CodeRabbit Major threads
Part of the adversarial-sweep over recently-merged PRs. #1455 merged with two unresolved CodeRabbit
Majorthreads onparser_sp.go, both money-integrity.parseOptionalFloatusedstrconv.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 flowed through as a corrupt non-finite value into the purchase money fields (HourlyCommitmentToPurchase,EstimatedMonthlySavingsAmount,UpfrontCost) and into theOnDemandCostbaseline.Fix (root cause, one guard)
Reject non-finite values at the parse boundary (
math.IsNaN/math.IsInf-> error):CurrentAverageHourlyOnDemandSpendpreviously survivedparseOptionalFloatOrWarnas NaN; it now degrades to 0 -> nil (frontend reconstruction), matching the documented "unavailable" path.Verification
TestParseSavingsPlanDetail_MoneyFieldUnparseablewith NaN/+Inf/-Inf money cases + newTestParseOptionalFloat_RejectsNonFinite.go build ./...,go vet, full recommendations package (424 tests) green.Summary by CodeRabbit
Bug Fixes
NaNand infinity are now rejected when processing AWS recommendation data.Tests