From 8d4be0d1a9286036f8df5f2384becc384480597f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 1 Oct 2026 22:16:02 +0200 Subject: [PATCH] fix(aws): report incomplete recommendation collections Return valid RI recommendations with typed detail and scope failures through the parser, collection fan-out and filtered provider adapter. Keep total API failures and cancellation fatal so consumers can choose their existing policy. Verify mixed responses through the real AWS SDK, exact nested counts, empty versus all-invalid results, filtering and cancellation during enrichment. Refs LeanerCloud/cloud-commitments-go#54 --- providers/aws/recommendations/client.go | 106 +++++------ providers/aws/recommendations/client_test.go | 22 ++- .../aws/recommendations/collection_error.go | 46 +++++ .../recommendations/collection_error_test.go | 168 ++++++++++++++++++ providers/aws/recommendations/parser_ri.go | 21 ++- .../aws/recommendations/parser_ri_test.go | 11 +- providers/aws/service_client.go | 6 +- .../aws/service_client_completeness_test.go | 55 ++++++ 8 files changed, 353 insertions(+), 82 deletions(-) create mode 100644 providers/aws/recommendations/collection_error.go create mode 100644 providers/aws/recommendations/collection_error_test.go create mode 100644 providers/aws/service_client_completeness_test.go diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index e84ed2e..ee69bfc 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -13,7 +13,6 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/concurrency" - "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" ) // maxRecommendationPages caps the number of pages fetched per Cost Explorer @@ -257,12 +256,7 @@ var defaultDiscoveryTerms = []string{"1yr", "3yr"} // rows and render as distinct UI rows for free. var defaultDiscoveryPaymentOptions = []string{"all-upfront", "partial-upfront", "no-upfront"} -// fetchSingleComboRecs fetches recommendations for one (term, payment) pair. -// If the context is already done before the call, it returns (nil, ctx.Err()). -// If GetRecommendations returns an error after ctx cancellation, it also -// returns (nil, ctx.Err()) so the caller exits the sweep immediately. Per-combo -// errors (throttle, 5xx) return (nil, err) with ctx.Err() == nil, signaling -// skip-and-continue tolerance in the outer loop. +// fetchSingleComboRecs retains incomplete responses but makes cancellation terminal. func (c *Client) fetchSingleComboRecs(ctx context.Context, service common.ServiceType, term, payment string) ([]common.Recommendation, error) { if ctx.Err() != nil { return nil, ctx.Err() @@ -279,20 +273,13 @@ func (c *Client) fetchSingleComboRecs(ctx context.Context, service common.Servic Region: "", } recs, err := c.GetRecommendations(ctx, ¶ms) - if err != nil { - // A canceled / deadline-exceeded ctx is NOT a per-combo - // failure to be tolerated -- every subsequent combo - // would just hit the same dead context and waste time - // while we accumulate "failures" that hide the real - // reason. Short-circuit so the caller sees the ctx - // error verbatim. Per-combo errors (throttle, 5xx) - // keep the existing skip-and-continue tolerance. - if ctx.Err() != nil { - return nil, ctx.Err() - } - return nil, err + if ctx.Err() != nil { + return nil, ctx.Err() } - return recs, nil + if canceled := collectionCancellation(err); canceled != nil { + return nil, canceled + } + return recs, err } // GetRecommendationsForService fetches recommendations for a specific @@ -303,31 +290,30 @@ func (c *Client) fetchSingleComboRecs(ctx context.Context, service common.Servic // params.PaymentOption so the resulting slice contains every combo // for the user to choose from in the UI. // -// A per-call Cost Explorer error is tolerated and skipped so a single -// throttle on one (term, payment) combo doesn't suppress the others; -// only an error where every combo fails is propagated. This mirrors -// the "continue on per-service error" tolerance in GetAllRecommendations. +// Failed details or scopes accompany surviving results as a typed diagnostic. +// If every API call fails, the ordinary error remains fatal. func (c *Client) GetRecommendationsForService(ctx context.Context, service common.ServiceType) ([]common.Recommendation, error) { allRecs := make([]common.Recommendation, 0) var lastErr error + incomplete := &IncompleteRecommendationsError{} successCount := 0 - attempts := 0 for _, term := range defaultDiscoveryTerms { for _, payment := range defaultDiscoveryPaymentOptions { - attempts++ recs, err := c.fetchSingleComboRecs(ctx, service, term, payment) if err != nil { - if ctx.Err() != nil { - return nil, ctx.Err() + if canceled := collectionCancellation(err); canceled != nil { + return nil, canceled } lastErr = err - continue + if !incomplete.addFailure(fmt.Errorf("service %s term %s payment %s: %w", service, term, payment, err)) { + continue + } } successCount++ allRecs = append(allRecs, recs...) } } - if successCount == 0 && attempts > 0 && lastErr != nil { + if successCount == 0 && lastErr != nil { return nil, fmt.Errorf("all (term, payment) variants failed for service %s: %w", service, lastErr) } // Enrich each rec with 7-day daily coverage history so the frontend @@ -335,8 +321,12 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo // are skipped inside AttachDailyUsageHistory (no per-SKU CE coverage // breakdown available). Errors are logged and skipped per-tuple so a // single CE failure doesn't suppress the rest of the collection. - if len(allRecs) > 0 { - c.AttachDailyUsageHistory(ctx, allRecs) + c.AttachDailyUsageHistory(ctx, allRecs) + if err := ctx.Err(); err != nil { + return nil, err + } + if len(incomplete.Causes) > 0 { + return allRecs, incomplete } return allRecs, nil } @@ -350,11 +340,8 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo // the canonical order EC2 → RDS → ElastiCache → OpenSearch → Redshift after // all goroutines finish so order-sensitive consumers stay stable. // -// Behavior change vs the previous sequential loop: per-service errors are -// now logged at WARN via mergeServiceResults — the previous loop swallowed -// them silently with a bare `continue`, leaving operators no signal when a -// single service was misbehaving. Mirrors the Azure parallelisation in -// providers/azure/recommendations.go (closes #258, commit b10326c5). +// Partial results retain diagnostics through mergeServiceResults; callers decide +// whether their workflow can use an incomplete collection. func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommendation, error) { var ( ec2Recs, rdsRecs, cacheRecs, osRecs, redshiftRecs []common.Recommendation @@ -436,41 +423,32 @@ type serviceResult struct { recs []common.Recommendation } -// mergeServiceResults logs per-service errors at WARN and appends successful -// results in the order the slice is passed — callers must preserve the -// canonical EC2 → RDS → ElastiCache → OpenSearch → Redshift → SavingsPlans -// order so that order-sensitive consumers stay stable. -// -// Partial failure is tolerated: as long as at least one service succeeded, the -// successful services' recommendations are returned with a nil error and the -// failures are logged at WARN. But when EVERY service errored (e.g. a sustained -// Cost Explorer throttle that exhausts each service's per-combo retries), the -// merge returns a wrapped error instead of an empty-but-nil-error result -// (08-H4). Returning (recs, nil) on a total failure makes a throttled run -// indistinguishable from "no savings available", which an operator can misread -// as "nothing to buy": the same hazard the per-service all-combos-failed guard -// in GetRecommendationsForService prevents one level down. +// mergeServiceResults preserves service order and incomplete responses. +// All ordinary service failures remain fatal, even when no rows were expected. func mergeServiceResults(results ...serviceResult) ([]common.Recommendation, error) { - total := 0 + var out []common.Recommendation + incomplete := &IncompleteRecommendationsError{} failures := 0 var lastErr error for i := range results { - total += len(results[i].recs) - if results[i].err != nil { - failures++ - lastErr = results[i].err - } - } - out := make([]common.Recommendation, 0, total) - for i := range results { - if results[i].err != nil { - logging.Warnf("AWS %s recommendations: %v", results[i].name, results[i].err) - continue + result := &results[i] + if result.err != nil { + if canceled := collectionCancellation(result.err); canceled != nil { + return nil, canceled + } + lastErr = result.err + if !incomplete.addFailure(fmt.Errorf("service %s: %w", result.name, result.err)) { + failures++ + continue + } } - out = append(out, results[i].recs...) + out = append(out, result.recs...) } if failures == len(results) && failures > 0 { return nil, fmt.Errorf("all %d AWS recommendation services failed: %w", failures, lastErr) } + if len(incomplete.Causes) > 0 { + return out, incomplete + } return out, nil } diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index fbb7e67..70867e7 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -499,7 +499,10 @@ func TestGetAllRecommendations(t *testing.T) { recs, err := client.GetAllRecommendations(context.Background()) - require.NoError(t, err) + var incomplete *IncompleteRecommendationsError + require.ErrorAs(t, err, &incomplete) + require.Equal(t, 24, incomplete.FailedDetails) + require.Zero(t, incomplete.FailedScopes) // Only EC2 will successfully parse since the mock returns EC2 details for all services // Other services will fail parsing and be skipped assert.NotEmpty(t, recs) @@ -536,8 +539,10 @@ func TestGetAllRecommendations_SomeServicesFail(t *testing.T) { recs, err := client.GetAllRecommendations(context.Background()) - // Should not error even if some services fail - require.NoError(t, err) + var incomplete *IncompleteRecommendationsError + require.ErrorAs(t, err, &incomplete) + require.Equal(t, 24, incomplete.FailedDetails) + require.Zero(t, incomplete.FailedScopes) // Should have recommendations from services that succeeded assert.NotEmpty(t, recs) } @@ -654,7 +659,10 @@ func TestGetAllRecommendations_IncludesSavingsPlans(t *testing.T) { client := NewClientWithAPI(mockAPI, "us-east-1") recs, err := client.GetAllRecommendations(context.Background()) - require.NoError(t, err) + var incomplete *IncompleteRecommendationsError + require.ErrorAs(t, err, &incomplete) + require.Equal(t, 24, incomplete.FailedDetails) + require.Zero(t, incomplete.FailedScopes) // At least one EC2 rec must be present (existing services not regressed). var ec2Count, spCount int @@ -925,7 +933,7 @@ func TestMergeServiceResults_AllFailIsError(t *testing.T) { assert.Contains(t, err.Error(), "all", "error should signal that every service failed") assert.Nil(t, recs) - // Partial failure is still tolerated: surviving service's recs returned, nil error. + // Partial failure retains the surviving service with an incomplete diagnostic. ec2Rec := common.Recommendation{Service: common.ServiceEC2} recs, err = mergeServiceResults( serviceResult{name: "EC2", recs: []common.Recommendation{ec2Rec}}, @@ -935,7 +943,9 @@ func TestMergeServiceResults_AllFailIsError(t *testing.T) { serviceResult{name: "Redshift", err: throttle}, serviceResult{name: "SavingsPlans", err: throttle}, ) - require.NoError(t, err, "a single surviving service must keep the run successful") + var incomplete *IncompleteRecommendationsError + require.ErrorAs(t, err, &incomplete) + assert.Equal(t, 5, incomplete.FailedScopes) assert.Len(t, recs, 1) assert.Equal(t, common.ServiceEC2, recs[0].Service) diff --git a/providers/aws/recommendations/collection_error.go b/providers/aws/recommendations/collection_error.go new file mode 100644 index 0000000..5669884 --- /dev/null +++ b/providers/aws/recommendations/collection_error.go @@ -0,0 +1,46 @@ +package recommendations + +import ( + "context" + "errors" + "fmt" +) + +// IncompleteRecommendationsError accompanies survivors of an incomplete collection. +// Each cause represents one rejected RI detail or one failed collection scope. +type IncompleteRecommendationsError struct { + FailedDetails int + FailedScopes int + Causes []error +} + +func (e *IncompleteRecommendationsError) Error() string { + return fmt.Sprintf("incomplete AWS recommendations: %d failed details, %d failed scopes: %v", + e.FailedDetails, e.FailedScopes, errors.Join(e.Causes...)) +} + +func (e *IncompleteRecommendationsError) Unwrap() []error { return e.Causes } + +// addFailure returns whether the failed scope received a usable response. +func (e *IncompleteRecommendationsError) addFailure(err error) bool { + var partial *IncompleteRecommendationsError + if errors.As(err, &partial) { + e.FailedDetails += partial.FailedDetails + e.FailedScopes += partial.FailedScopes + e.Causes = append(e.Causes, partial.Causes...) + return true + } + e.FailedScopes++ + e.Causes = append(e.Causes, err) + return false +} + +func collectionCancellation(err error) error { + if errors.Is(err, context.Canceled) { + return context.Canceled + } + if errors.Is(err, context.DeadlineExceeded) { + return context.DeadlineExceeded + } + return nil +} diff --git a/providers/aws/recommendations/collection_error_test.go b/providers/aws/recommendations/collection_error_test.go new file mode 100644 index 0000000..9da9e88 --- /dev/null +++ b/providers/aws/recommendations/collection_error_test.go @@ -0,0 +1,168 @@ +package recommendations + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" + "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +type incompleteCollectionAPI struct { + mockCostExplorerAPI + request func(*costexplorer.GetReservationPurchaseRecommendationInput) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) + coverage func() +} + +func (m *incompleteCollectionAPI) GetReservationPurchaseRecommendation(_ context.Context, in *costexplorer.GetReservationPurchaseRecommendationInput, _ ...func(*costexplorer.Options)) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + return m.request(in) +} + +func (m *incompleteCollectionAPI) GetReservationCoverage(_ context.Context, _ *costexplorer.GetReservationCoverageInput, _ ...func(*costexplorer.Options)) (*costexplorer.GetReservationCoverageOutput, error) { + if m.coverage != nil { + m.coverage() + } + return &costexplorer.GetReservationCoverageOutput{}, nil +} + +func incompleteRIDetails(valid bool, bad int) []types.ReservationPurchaseRecommendation { + var blocks []types.ReservationPurchaseRecommendation + if valid { + blocks = append(blocks, types.ReservationPurchaseRecommendation{RecommendationDetails: []types.ReservationPurchaseRecommendationDetail{{ + RecommendedNumberOfInstancesToPurchase: aws.String("2"), EstimatedMonthlyOnDemandCost: aws.String("30"), + InstanceDetails: &types.InstanceDetails{RDSInstanceDetails: &types.RDSInstanceDetails{InstanceType: aws.String("db.t3.medium"), Region: aws.String("us-east-1")}}, + }}}) + } + for range bad { + blocks = append(blocks, types.ReservationPurchaseRecommendation{RecommendationDetails: []types.ReservationPurchaseRecommendationDetail{{RecommendedNumberOfInstancesToPurchase: aws.String("bad")}}}) + } + return blocks +} + +func TestCollectionCompleteness_Combos(t *testing.T) { + apiFailure := errors.New("access denied") + for _, tc := range []struct { + name string + valid bool + bad, failed, rows, details, scopes int + fatal bool + }{ + {name: "valid", valid: true, rows: 6}, + {name: "empty"}, + {name: "mixed details", valid: true, bad: 2, rows: 6, details: 12}, + {name: "all invalid", bad: 2, details: 12}, + {name: "empty and API failure", failed: 1, scopes: 1}, + {name: "valid and API failure", valid: true, failed: 1, rows: 5, scopes: 1}, + {name: "invalid and API failure", bad: 2, failed: 5, details: 2, scopes: 5}, + {name: "total API failure", failed: 6, fatal: true}, + } { + t.Run(tc.name, func(t *testing.T) { + calls := 0 + api := &incompleteCollectionAPI{request: func(*costexplorer.GetReservationPurchaseRecommendationInput) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + calls++ + if calls <= tc.failed { + return nil, apiFailure + } + return &costexplorer.GetReservationPurchaseRecommendationOutput{Recommendations: incompleteRIDetails(tc.valid, tc.bad)}, nil + }} + recs, err := NewClientWithAPI(api, "us-east-1").GetRecommendationsForService(context.Background(), common.ServiceRDS) + require.Equal(t, 6, calls) + require.Len(t, recs, tc.rows) + var incomplete *IncompleteRecommendationsError + if tc.fatal { + require.ErrorIs(t, err, apiFailure) + require.False(t, errors.As(err, &incomplete)) + return + } + if tc.details+tc.scopes == 0 { + require.NoError(t, err) + return + } + require.ErrorAs(t, err, &incomplete) + require.Equal(t, tc.details, incomplete.FailedDetails) + require.Equal(t, tc.scopes, incomplete.FailedScopes) + require.Len(t, incomplete.Causes, tc.details+tc.scopes) + for _, cause := range incomplete.Causes { + require.Contains(t, cause.Error(), "service rds term ") + require.Contains(t, cause.Error(), " payment ") + } + if tc.bad > 0 { + block := 0 + if tc.valid { + block = 1 + } + require.Contains(t, err.Error(), fmt.Sprintf("block %d detail 0", block)) + require.Contains(t, err.Error(), fmt.Sprintf("block %d detail 0", block+1)) + } + if tc.failed > 0 { + require.ErrorIs(t, err, apiFailure) + } + }) + } +} + +func TestCollectionCompleteness_NestedServices(t *testing.T) { + detailFailure := errors.New("bad quantity") + apiFailure := errors.New("access denied") + partial := &IncompleteRecommendationsError{FailedDetails: 1, FailedScopes: 1, Causes: []error{detailFailure, apiFailure}} + recs, err := mergeServiceResults( + serviceResult{name: "RDS", recs: []common.Recommendation{{Count: 2}}, err: fmt.Errorf("wrapped: %w", partial)}, + serviceResult{name: "EC2", err: apiFailure}, + ) + outerRecs, outerErr := mergeServiceResults(serviceResult{name: "nested", recs: recs, err: err}, serviceResult{name: "empty"}) + var incomplete *IncompleteRecommendationsError + require.ErrorAs(t, outerErr, &incomplete) + require.Len(t, outerRecs, 1) + require.Equal(t, 1, incomplete.FailedDetails) + require.Equal(t, 2, incomplete.FailedScopes) + require.Len(t, incomplete.Causes, 3) + require.ErrorIs(t, outerErr, detailFailure) + require.ErrorIs(t, outerErr, apiFailure) + for _, cause := range incomplete.Causes { + var nested *IncompleteRecommendationsError + require.False(t, errors.As(cause, &nested)) + } + _, emptyErr := mergeServiceResults(serviceResult{name: "invalid", err: partial}) + require.ErrorAs(t, emptyErr, &incomplete) + require.Equal(t, 1, incomplete.FailedDetails) +} + +func TestCollectionCompleteness_Cancellation(t *testing.T) { + for _, terminal := range []error{context.Canceled, context.DeadlineExceeded} { + t.Run(terminal.Error(), func(t *testing.T) { + calls := 0 + api := &incompleteCollectionAPI{request: func(*costexplorer.GetReservationPurchaseRecommendationInput) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + calls++ + return nil, fmt.Errorf("SDK: %w", terminal) + }} + recs, err := NewClientWithAPI(api, "us-east-1").GetRecommendationsForService(context.Background(), common.ServiceRDS) + require.ErrorIs(t, err, terminal) + require.Nil(t, recs) + require.Equal(t, 1, calls) + recs, err = mergeServiceResults(serviceResult{name: "valid", recs: []common.Recommendation{{Count: 1}}}, serviceResult{name: "canceled", err: terminal}) + require.ErrorIs(t, err, terminal) + require.Nil(t, recs) + var incomplete *IncompleteRecommendationsError + require.False(t, errors.As(err, &incomplete)) + }) + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + coverageCalls := 0 + api := &incompleteCollectionAPI{ + request: func(*costexplorer.GetReservationPurchaseRecommendationInput) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + return &costexplorer.GetReservationPurchaseRecommendationOutput{Recommendations: incompleteRIDetails(true, 1)}, nil + }, + coverage: func() { coverageCalls++; cancel() }, + } + recs, err := NewClientWithAPI(api, "us-east-1").GetRecommendationsForService(ctx, common.ServiceRDS) + require.Positive(t, coverageCalls) + require.ErrorIs(t, err, context.Canceled) + require.Nil(t, recs) +} diff --git a/providers/aws/recommendations/parser_ri.go b/providers/aws/recommendations/parser_ri.go index 71b1861..ccb0e29 100644 --- a/providers/aws/recommendations/parser_ri.go +++ b/providers/aws/recommendations/parser_ri.go @@ -17,18 +17,23 @@ import ( // parseRecommendations converts AWS recommendations to common.Recommendation format. func (c *Client) parseRecommendations(ctx context.Context, awsRecs []types.ReservationPurchaseRecommendation, params common.RecommendationParams) ([]common.Recommendation, error) { var recommendations []common.Recommendation + incomplete := &IncompleteRecommendationsError{} for r := range awsRecs { awsRec := &awsRecs[r] for i := range awsRec.RecommendationDetails { + if err := ctx.Err(); err != nil { + return nil, err + } details := &awsRec.RecommendationDetails[i] rec, err := c.parseRecommendationDetail(ctx, details, params) if err != nil { - // log (stderr), never fmt.Print (stdout): this package is - // linked into cmd/cudly-mcp, whose stdio transport owns - // stdout for JSON-RPC framing. A warning printed here during - // a cudly_search_recommendations call would be interleaved - // into the protocol stream and corrupt the session. + if canceled := collectionCancellation(err); canceled != nil { + return nil, canceled + } + incomplete.FailedDetails++ + incomplete.Causes = append(incomplete.Causes, fmt.Errorf("service %s term %s payment %s block %d detail %d: %w", + params.Service, params.Term, params.PaymentOption, r, i, err)) log.Printf("Warning: Failed to parse recommendation detail %d: %v", i, err) continue } @@ -39,6 +44,12 @@ func (c *Client) parseRecommendations(ctx context.Context, awsRecs []types.Reser } } + if err := ctx.Err(); err != nil { + return nil, err + } + if incomplete.FailedDetails > 0 { + return recommendations, incomplete + } return recommendations, nil } diff --git a/providers/aws/recommendations/parser_ri_test.go b/providers/aws/recommendations/parser_ri_test.go index c54d164..146d6cc 100644 --- a/providers/aws/recommendations/parser_ri_test.go +++ b/providers/aws/recommendations/parser_ri_test.go @@ -443,7 +443,7 @@ func TestParseRecommendations(t *testing.T) { assert.Equal(t, "us-west-2", recs[1].Region) } -func TestParseRecommendations_SkipsInvalidDetails(t *testing.T) { +func TestParseRecommendations_ReportsInvalidDetails(t *testing.T) { client := &Client{} awsRecs := []types.ReservationPurchaseRecommendation{ @@ -498,8 +498,9 @@ func TestParseRecommendations_SkipsInvalidDetails(t *testing.T) { recs, err := client.parseRecommendations(context.Background(), awsRecs, params) - require.NoError(t, err) - // Should have 2 valid recommendations, skipping the invalid one + var incomplete *IncompleteRecommendationsError + require.ErrorAs(t, err, &incomplete) + assert.Equal(t, 1, incomplete.FailedDetails) assert.Len(t, recs, 2) assert.Equal(t, "t3.medium", recs[0].ResourceType) assert.Equal(t, "r5.xlarge", recs[1].ResourceType) @@ -611,7 +612,7 @@ func TestParseRIUtilizationSignals(t *testing.T) { // during a cudly_search_recommendations call injected a bare line of prose // into the middle of the JSON-RPC stream and broke the client session. // -// TestParseRecommendations_SkipsInvalidDetails above already drives this exact +// TestParseRecommendations_ReportsInvalidDetails above already drives this exact // code path, but it only asserts the returned recommendation count -- it stayed // green the entire time the bug was live. This test asserts the property that // actually matters: nothing reaches stdout, whatever is logged. @@ -645,7 +646,7 @@ func TestParseRecommendations_WarningsNeverGoToStdout(t *testing.T) { stdout, logged := captureStdoutAndLog(t, func() { recs, err := client.parseRecommendations(context.Background(), awsRecs, params) - require.NoError(t, err) + require.Error(t, err) assert.Empty(t, recs, "the single invalid detail must be skipped") }) diff --git a/providers/aws/service_client.go b/providers/aws/service_client.go index 33a91ef..a93cd60 100644 --- a/providers/aws/service_client.go +++ b/providers/aws/service_client.go @@ -3,6 +3,7 @@ package aws import ( "context" + "errors" "fmt" "github.com/aws/aws-sdk-go-v2/aws" @@ -77,12 +78,13 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p return nil, fmt.Errorf("params cannot be nil") } recs, err := r.client.GetRecommendations(ctx, params) - if err != nil { + var incomplete *recommendations.IncompleteRecommendationsError + if err != nil && !errors.As(err, &incomplete) { return nil, err } recs = applyRecommendationFilters(recs, *params) - return recs, nil + return recs, err } // applyRecommendationFilters applies account and region filters to recommendations. diff --git a/providers/aws/service_client_completeness_test.go b/providers/aws/service_client_completeness_test.go new file mode 100644 index 0000000..ac05112 --- /dev/null +++ b/providers/aws/service_client_completeness_test.go @@ -0,0 +1,55 @@ +package aws + +import ( + "context" + "fmt" + "io" + "net/http" + "strings" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" +) + +type recommendationHTTPFixture func(*http.Request) (*http.Response, error) + +func (f recommendationHTTPFixture) Do(req *http.Request) (*http.Response, error) { return f(req) } + +func TestRecommendationsClientAdapter_IncompleteSDKResponse(t *testing.T) { + calls := 0 + client := NewRecommendationsClient(aws.Config{ + Region: "us-east-1", Credentials: aws.CredentialsProviderFunc(func(context.Context) (aws.Credentials, error) { + return aws.Credentials{AccessKeyID: "synthetic", SecretAccessKey: "synthetic"}, nil + }), + HTTPClient: recommendationHTTPFixture(func(req *http.Request) (*http.Response, error) { + if req.Header.Get("X-Amz-Target") != "AWSInsightsIndexService.GetReservationPurchaseRecommendation" { + return nil, fmt.Errorf("unexpected SDK request: %s", req.Header.Get("X-Amz-Target")) + } + calls++ + body := `{"Recommendations":[{"RecommendationDetails":[ + {"RecommendedNumberOfInstancesToPurchase":"2","EstimatedMonthlyOnDemandCost":"30","AccountId":"included","InstanceDetails":{"RDSInstanceDetails":{"InstanceType":"db.t3.medium","Region":"us-east-1"}}}, + {"RecommendedNumberOfInstancesToPurchase":"1","EstimatedMonthlyOnDemandCost":"30","AccountId":"excluded","InstanceDetails":{"RDSInstanceDetails":{"InstanceType":"db.t3.medium","Region":"us-east-1"}}}, + {"RecommendedNumberOfInstancesToPurchase":"1","EstimatedMonthlyOnDemandCost":"30","AccountId":"included","InstanceDetails":{"RDSInstanceDetails":{"InstanceType":"db.t3.medium","Region":"eu-west-1"}}}, + {"RecommendedNumberOfInstancesToPurchase":"not-a-number"}]}]}` + return &http.Response{StatusCode: http.StatusOK, Header: http.Header{"Content-Type": {"application/x-amz-json-1.1"}}, Body: io.NopCloser(strings.NewReader(body)), Request: req}, nil + }), + }) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{ + Service: common.ServiceRDS, Term: "1yr", PaymentOption: "no-upfront", LookbackPeriod: "7d", + Region: "us-east-1", AccountFilter: []string{"included"}, + }) + require.Error(t, err, "malformed detail must not produce a successful short menu") + var incomplete *recommendations.IncompleteRecommendationsError + require.ErrorAs(t, err, &incomplete) + require.Equal(t, 1, incomplete.FailedDetails) + require.Zero(t, incomplete.FailedScopes) + require.Len(t, incomplete.Causes, 1) + require.Contains(t, incomplete.Error(), "service rds term 1yr payment no-upfront block 0 detail 3") + require.Len(t, recs, 1, "incomplete survivors must still receive account and region filters") + require.Equal(t, 2, recs[0].Count) + require.Equal(t, 1, calls) +}