From 23d64159806624d942bc817a0c9d833bed59f01d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 11 Jun 2026 01:07:21 -0700 Subject: [PATCH] fix(providers/aws): fail loud on unparseable RI cost fields parseAWSCostDetails silently swallowed strconv.ParseFloat errors for UpfrontCost, EstimatedMonthlyOnDemandCost, and RecurringStandardMonthlyCost, leaving the destination at 0. An unparseable UpfrontCost would surface an all-upfront RI recommendation showing $0 upfront, a wrong money figure feeding effective-savings math and purchase decisions, with no signal. Make a present-but-unparseable cost string a hard error, consistent with parseCostInformation in the same file. The caller chain already handles this: parseRecommendationDetail propagates the error and parseRecommendations logs a warning and skips the recommendation, so a malformed CE response drops that detail loudly instead of fabricating zeros. Regression test covers all three malformed fields and was confirmed failing against the pre-fix code. Closes #1171 --- providers/aws/recommendations/parser_ri.go | 30 +++++-- .../aws/recommendations/parser_ri_test.go | 88 +++++++++++++++++++ 2 files changed, 110 insertions(+), 8 deletions(-) diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 35aa90df9..6c8148d63 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -69,7 +69,9 @@ func (c *Client) parseRecommendationDetail(ctx context.Context, details *types.R } // Parse AWS-provided cost details - c.parseAWSCostDetails(rec, details) + if err := c.parseAWSCostDetails(rec, details); err != nil { + return nil, fmt.Errorf("failed to parse AWS cost details: %w", err) + } // Parse RI utilization signals used by --target-coverage sizing c.parseRIUtilizationSignals(rec, details) @@ -152,16 +154,25 @@ func (c *Client) parseCostInformation(details *types.ReservationPurchaseRecommen } // parseAWSCostDetails extracts upfront, on-demand, and recurring monthly cost from AWS details. -func (c *Client) parseAWSCostDetails(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) { +// +// A present-but-unparseable cost string is a hard error, consistent with +// parseCostInformation: silently leaving the field at zero would surface a +// 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 { if details.UpfrontCost != nil { - if upfront, err := strconv.ParseFloat(*details.UpfrontCost, 64); err == nil { - rec.CommitmentCost = upfront + upfront, err := strconv.ParseFloat(*details.UpfrontCost, 64) + if err != nil { + return fmt.Errorf("failed to parse upfront cost %q: %w", *details.UpfrontCost, err) } + rec.CommitmentCost = upfront } if details.EstimatedMonthlyOnDemandCost != nil { - if onDemand, err := strconv.ParseFloat(*details.EstimatedMonthlyOnDemandCost, 64); err == nil { - rec.OnDemandCost = onDemand + onDemand, err := strconv.ParseFloat(*details.EstimatedMonthlyOnDemandCost, 64) + if err != nil { + return fmt.Errorf("failed to parse estimated monthly on-demand cost %q: %w", *details.EstimatedMonthlyOnDemandCost, err) } + rec.OnDemandCost = onDemand } else { // EstimatedMonthlyOnDemandCost absent from AWS CE response — OnDemandCost // will be 0 and the scheduler's nonZeroPtr will store nil, causing the @@ -172,10 +183,13 @@ 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 { - if monthly, err := strconv.ParseFloat(*details.RecurringStandardMonthlyCost, 64); err == nil { - rec.RecurringMonthlyCost = &monthly + monthly, err := strconv.ParseFloat(*details.RecurringStandardMonthlyCost, 64) + if err != nil { + return fmt.Errorf("failed to parse recurring standard monthly cost %q: %w", *details.RecurringStandardMonthlyCost, err) } + rec.RecurringMonthlyCost = &monthly } + return nil } // serviceParserFunc defines the signature for service-specific parsers diff --git a/providers/aws/recommendations/parser_ri_test.go b/providers/aws/recommendations/parser_ri_test.go index 4657fcb0c..8d1ecc2cf 100644 --- a/providers/aws/recommendations/parser_ri_test.go +++ b/providers/aws/recommendations/parser_ri_test.go @@ -260,6 +260,94 @@ func TestParseRecommendationDetail_WithAccountAndCosts(t *testing.T) { assert.Equal(t, 650.00, rec.OnDemandCost) } +// TestParseRecommendationDetail_MalformedCostFields is the regression test for +// COR-07 (#1171): a present-but-unparseable upfront / on-demand / recurring +// cost string must fail loud instead of silently leaving the field at 0, +// which would surface a wrong money figure (e.g. $0 upfront on an all-upfront +// RI) into savings math and purchase decisions. +func TestParseRecommendationDetail_MalformedCostFields(t *testing.T) { + client := &Client{} + + params := common.RecommendationParams{ + Service: common.ServiceEC2, + PaymentOption: "all-upfront", + Term: "1yr", + LookbackPeriod: "7d", + } + + baseDetails := func() *types.ReservationPurchaseRecommendationDetail { + return &types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("2"), + EstimatedMonthlySavingsAmount: aws.String("150.00"), + EstimatedMonthlySavingsPercentage: aws.String("30.0"), + AccountId: aws.String("123456789012"), + UpfrontCost: aws.String("500.00"), + EstimatedMonthlyOnDemandCost: aws.String("650.00"), + RecurringStandardMonthlyCost: aws.String("42.50"), + 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("shared"), + }, + }, + } + } + + tests := []struct { + name string + mutate func(d *types.ReservationPurchaseRecommendationDetail) + errContains string + }{ + { + name: "malformed upfront cost", + mutate: func(d *types.ReservationPurchaseRecommendationDetail) { + d.UpfrontCost = aws.String("not-a-number") + }, + errContains: `failed to parse upfront cost "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"`, + }, + { + name: "malformed recurring monthly cost", + mutate: func(d *types.ReservationPurchaseRecommendationDetail) { + d.RecurringStandardMonthlyCost = aws.String("") + }, + errContains: `failed to parse recurring standard monthly cost ""`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + details := baseDetails() + tt.mutate(details) + + rec, err := client.parseRecommendationDetail(context.Background(), details, params) + + require.Error(t, err) + assert.Nil(t, rec) + assert.Contains(t, err.Error(), "failed to parse AWS cost details") + assert.Contains(t, err.Error(), tt.errContains) + }) + } + + // Sanity check: the same details with all cost fields well-formed parse + // cleanly, so the failures above are attributable to the malformed field. + rec, err := client.parseRecommendationDetail(context.Background(), baseDetails(), params) + require.NoError(t, err) + require.NotNil(t, rec) + assert.Equal(t, 500.00, rec.CommitmentCost) + assert.Equal(t, 650.00, rec.OnDemandCost) + require.NotNil(t, rec.RecurringMonthlyCost) + assert.Equal(t, 42.50, *rec.RecurringMonthlyCost) +} + func TestParseRecommendations(t *testing.T) { client := &Client{}