diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index b2d858b04..9aa5bdb37 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -88,8 +88,9 @@ func TestFetchRecommendationsForRegion_InterruptIsNotReportedAsProviderFailure(t client := &MockRecommendationsClient{} client.On("GetRecommendations", mock.Anything, mock.Anything).Return(nil, ctx.Err()) - got := fetchRecommendationsForRegion(ctx, client, common.ServiceRDS, "us-east-1", Config{TermYears: 1}) + got, err := fetchRecommendationsForRegion(ctx, client, common.ServiceRDS, "us-east-1", Config{TermYears: 1}) + assert.ErrorIs(t, err, context.Canceled) assert.Nil(t, got) assert.NotContains(t, out.String(), "Failed to fetch recommendations") } @@ -111,20 +112,31 @@ func TestFetchAllRecs_InterruptStopsFanOutAndDiscardsPartialData(t *testing.T) { client.AssertNumberOfCalls(t, "GetRecommendations", 1) // later regions are never queried } -func TestFetchAllRecs_ActiveContextProviderErrorStaysRecoverable(t *testing.T) { - out := captureAppLog(t) +func TestFetchAllRecs_ProviderErrorFailsTheRun(t *testing.T) { + captureAppLog(t) cfg := Config{Regions: []string{"us-east-1", "us-west-2"}, Coverage: 100, TermYears: 1, PaymentOption: "partial-upfront"} client := &MockRecommendationsClient{} client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "us-east-1" })). - Return(nil, errors.New("throttling")).Once() - rec := common.Recommendation{Service: common.ServiceRDS, Region: "us-west-2", ResourceType: "db.t3.small", Count: 3, EstimatedSavings: 10, SavingsPercentage: 10} - client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "us-west-2" })). - Return([]common.Recommendation{rec}, nil).Once() + Return(nil, errors.New("expired SSO token")).Once() + + recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCache(aws.Config{}), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + + require.Error(t, err, "a fetch failure must not read as 'no recommendations'") + assert.Contains(t, err.Error(), "expired SSO token") + assert.Empty(t, recs) + client.AssertNumberOfCalls(t, "GetRecommendations", 1) +} + +func TestFetchAllRecs_EmptyResultIsNotAnError(t *testing.T) { + captureAppLog(t) + cfg := Config{Regions: []string{"us-east-1"}, Coverage: 100, TermYears: 1, PaymentOption: "partial-upfront"} + client := &MockRecommendationsClient{} + client.On("GetRecommendations", mock.Anything, mock.Anything).Return([]common.Recommendation{}, nil) - recs, _, _ := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCache(aws.Config{}), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCache(aws.Config{}), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) - assert.Len(t, recs, 1, "a provider error with an active context only loses that region") - assert.Contains(t, out.String(), "Failed to fetch recommendations") + require.NoError(t, err) + assert.Empty(t, recs) } func TestCheckDuplicates_InterruptedCheckIsTerminalInDryAndRealRuns(t *testing.T) { diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 2085869b6..1636cf2aa 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "log" "sync/atomic" @@ -188,7 +189,7 @@ func runToolMultiService(ctx context.Context, cfg Config) { // Phase 1: collect all recommendations without purchasing. AppLogger.Printf("\nšŸ“„ Fetching recommendations from all services...\n") allRecs, drops, err := fetchAllRecs(ctx, awsCfg, recClient, accountCache, servicesToProcess, engineData, cfg, coverageMap) - exitOnExclusionError(err) + exitOnFetchError(err) exitIfInterrupted(ctx, "fetching recommendations") // Phase 2: score, enforce the run-wide instance cap, and display. @@ -1051,6 +1052,13 @@ func hasDatabaseRecs(recs []common.Recommendation) bool { // exitOnExclusionError aborts the run before any purchase when the // extended-support exclusion cannot be applied (issue #2147). +func exitOnFetchError(err error) { + if errors.Is(err, errRecommendationFetch) { + log.Fatalf("Cannot complete the recommendation fetch: %v", err) + } + exitOnExclusionError(err) +} + func exitOnExclusionError(err error) { if err != nil { log.Fatalf("Cannot apply extended-support exclusion: %v", err) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 831ed41e5..87cd19f40 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -508,7 +508,13 @@ func processRegionRecommendations( AppLogger.Printf("\n šŸ“ [%d/%d] Region: %s\n", regionIndex, totalRegions, region) // Fetch recommendations - recs := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg) + recs, fetchErr := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg) + if fetchErr != nil { + // Legacy test-only path: the CLI run goes through fetchAndFilterRegionRecs, + // which returns this error to the caller. + AppLogger.Printf(" āŒ Failed to fetch recommendations: %v\n", fetchErr) + return result + } if len(recs) == 0 { AppLogger.Printf(" ā„¹ļø No recommendations found\n") return result @@ -582,14 +588,21 @@ func processRegionRecommendations( return result } +// errRecommendationFetch marks a failure to obtain recommendations from the +// provider, as opposed to a successful but empty answer. +var errRecommendationFetch = errors.New("recommendation fetch failed") + // fetchRecommendationsForRegion fetches recommendations from AWS for a specific region. +// A provider failure is returned, not logged away: an empty slice with a nil +// error means the provider answered and had nothing to recommend. An +// incomplete-but-usable answer (partial sweep) is returned with a warning. func fetchRecommendationsForRegion( ctx context.Context, recClient provider.RecommendationsClient, service common.ServiceType, region string, cfg Config, -) []common.Recommendation { +) ([]common.Recommendation, error) { termStr := "1yr" if cfg.TermYears == 3 { termStr = "3yr" @@ -622,7 +635,7 @@ func fetchRecommendationsForRegion( var incomplete *recommendations.IncompleteRecommendationsError if errors.As(err, &incomplete) { AppLogger.Printf(" āš ļø %v\n", incomplete) - return recs + return recs, nil } if partial := azureprovider.AsPartialSubscriptionFailure(err); partial != nil { // Keep the subscriptions that did answer, but say plainly that the @@ -634,18 +647,17 @@ func fetchRecommendationsForRegion( for _, f := range partial.Failed { AppLogger.Printf(" subscription %s: %v\n", f.SubscriptionID, f.Err) } - return recs + return recs, nil } - if ctx.Err() != nil { + if ctxErr := ctx.Err(); ctxErr != nil { // Interrupted, not a provider failure: the caller stops on ctx.Err(). - return nil + return nil, ctxErr } if err != nil { - AppLogger.Printf(" āŒ Failed to fetch recommendations: %v\n", err) - return nil + return nil, fmt.Errorf("%w: %s in %s: %w", errRecommendationFetch, getServiceDisplayName(service), region, err) } - return recs + return recs, nil } // stampSavingsPlanRegions copies the region Cost Explorer reports for an EC2 @@ -854,7 +866,10 @@ func fetchAndFilterRegionRecs( ) ([]common.Recommendation, error) { AppLogger.Printf("\n šŸ“ [%d/%d] Region: %s\n", regionIndex, totalRegions, region) - recs := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg) + recs, err := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg) + if err != nil { + return nil, err + } if len(recs) == 0 { AppLogger.Printf(" ā„¹ļø No recommendations found\n") return nil, nil @@ -930,14 +945,16 @@ func fetchAllRecs( if ctx.Err() != nil { return nil, drops, nil } - log.Printf("āŒ Failed to determine regions for %s: %v", getServiceDisplayName(service), err) - continue + return nil, nil, fmt.Errorf("%w: determining regions for %s: %w", errRecommendationFetch, getServiceDisplayName(service), err) } for i, region := range regions { if ctx.Err() != nil { return nil, drops, nil // partial data from an interrupted fetch is not a result } recs, err := fetchAndFilterRegionRecs(ctx, awsCfg, recClient, accountCache, service, region, i+1, len(regions), engineData, cfg, coverageMap, drops) + if ctx.Err() != nil { + return nil, drops, nil // interrupted mid-fetch: not a result, not a failure + } if err != nil { return nil, nil, err } diff --git a/cmd/recommendation_completeness_test.go b/cmd/recommendation_completeness_test.go index cfee43e17..9368d2761 100644 --- a/cmd/recommendation_completeness_test.go +++ b/cmd/recommendation_completeness_test.go @@ -67,9 +67,20 @@ func runCompletenessScenario(t *testing.T, service, regionMode, details string) } var stdout, stderr bytes.Buffer child.Stdout, child.Stderr = &stdout, &stderr - require.NoError(t, child.Run(), "stdout: %s\nstderr: %s", &stdout, &stderr) - fixture.assertRequests(t) + runErr := child.Run() logs := stdout.String() + stderr.String() + if details == "api-error" || details == "failed-type" { + // A provider failure is a failed run, not "nothing to buy". + var exitErr *exec.ExitError + require.ErrorAs(t, runErr, &exitErr, logs) + require.Equal(t, 1, exitErr.ExitCode(), logs) + require.Contains(t, logs, "Cannot complete the recommendation fetch") + require.Contains(t, logs, "fixture denied recommendations") + require.NoFileExists(t, output) + return + } + require.NoError(t, runErr, "stdout: %s\nstderr: %s", &stdout, &stderr) + fixture.assertRequests(t) for _, failure := range []string{"Could not check", "Failed to query", "request send failed", "certificate", "TLS handshake"} { require.NotContains(t, logs, failure, "unexpected ancillary error") } @@ -125,14 +136,8 @@ func assertSPCompleteness(t *testing.T, output, service, details, logs string) { warnings, failedDetails = len(types), 1 case "late-page": warnings, failedScopes = len(types), 1 - case "failed-type": - if service == "savingsplans" { - survivors = types[:3] - } else { - survivors = nil - } } - if details == "empty" || details == "all-invalid" || details == "api-error" { + if details == "empty" || details == "all-invalid" { survivors = nil } require.Equal(t, warnings, strings.Count(logs, "incomplete AWS recommendations:"), logs) @@ -143,15 +148,8 @@ func assertSPCompleteness(t *testing.T, output, service, details, logs string) { require.Contains(t, logs, "not-a-number") } } - failures := 0 - switch details { - case "api-error": - failures = len(types) - case "failed-type": - failures = 1 - } - require.Equal(t, failures, strings.Count(logs, "Failed to fetch recommendations:"), logs) - if failures > 0 || details == "late-page" { + require.NotContains(t, logs, "Failed to fetch recommendations:") + if details == "late-page" { require.Contains(t, logs, "fixture denied recommendations") } require.NotContains(t, logs, "Region discovery incomplete") @@ -203,12 +201,8 @@ func assertCompletenessDiagnostics(t *testing.T, regionMode, details, stdout, st } else { require.NotContains(t, stdout, "incomplete AWS recommendations") } - if details == "api-error" { - require.Contains(t, stdout+stderr, "fixture denied recommendations") - } else { - require.NotContains(t, stdout+stderr, "Failed to fetch recommendations", "partial collection treated as fatal") - require.NotContains(t, stdout+stderr, "failed to discover regions", "partial collection treated as fatal") - } + require.NotContains(t, stdout+stderr, "Failed to fetch recommendations", "partial collection treated as fatal") + require.NotContains(t, stdout+stderr, "failed to discover regions", "partial collection treated as fatal") } func assertCompletenessCSV(t *testing.T, output, details, logs string) { @@ -262,14 +256,16 @@ func TestRecommendationCompletenessHelpers(t *testing.T) { client.On("GetRecommendationsForService", ctx, common.ServiceRDS).Return(tc.recs, tc.err) client.On("GetRecommendations", ctx, mock.Anything).Return(tc.recs, tc.err) regions, err := discoverRegionsForService(ctx, client, common.ServiceRDS) - got := fetchRecommendationsForRegion(ctx, client, common.ServiceRDS, "us-east-1", Config{TermYears: 1}) + got, fetchErr := fetchRecommendationsForRegion(ctx, client, common.ServiceRDS, "us-east-1", Config{TermYears: 1}) if !tc.keep { require.ErrorIs(t, err, tc.err) + require.ErrorIs(t, fetchErr, tc.err) require.Nil(t, regions) require.Nil(t, got) require.NotContains(t, output.String(), "incomplete AWS recommendations") } else { require.NoError(t, err) + require.NoError(t, fetchErr) require.Equal(t, tc.recs, got) require.Len(t, regions, len(tc.recs)) require.Equal(t, 2, strings.Count(output.String(), "2 failed details, 1 failed scopes")) diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index d2b804c8e..fcb28beea 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -419,7 +419,8 @@ func TestFetchRecommendationsForRegion_SavingsPlanDoesNotFilterOnEndpointRegion( Run(func(args mock.Arguments) { got = args.Get(1).(*common.RecommendationParams) }). Return([]common.Recommendation{}, nil) - fetchRecommendationsForRegion(context.Background(), client, tc.service, spAPIRegion, Config{TermYears: 1}) + _, err := fetchRecommendationsForRegion(context.Background(), client, tc.service, spAPIRegion, Config{TermYears: 1}) + require.NoError(t, err) require.NotNil(t, got, tc.service) assert.Equal(t, tc.wantRegion, got.Region, tc.service) diff --git a/cmd/savings_plan_region_test.go b/cmd/savings_plan_region_test.go index d13725aff..a9f42b2dd 100644 --- a/cmd/savings_plan_region_test.go +++ b/cmd/savings_plan_region_test.go @@ -32,7 +32,8 @@ func TestFetchRecommendationsForRegion_SavingsPlanRowsCarryDetailsRegion(t *test client.On("GetRecommendations", mock.Anything, mock.Anything). Return([]common.Recommendation{ec2InstanceSPRow(), compute}, nil) - got := fetchRecommendationsForRegion(context.Background(), client, common.ServiceSavingsPlansEC2Instance, spAPIRegion, Config{TermYears: 1}) + got, err := fetchRecommendationsForRegion(context.Background(), client, common.ServiceSavingsPlansEC2Instance, spAPIRegion, Config{TermYears: 1}) + require.NoError(t, err) require.Len(t, got, 2) assert.Equal(t, "eu-west-2", got[0].Region) diff --git a/cmd/sp_account_scope_test.go b/cmd/sp_account_scope_test.go index 667b514af..9c309bf23 100644 --- a/cmd/sp_account_scope_test.go +++ b/cmd/sp_account_scope_test.go @@ -33,8 +33,9 @@ func TestFetchRecommendationsForRegion_PassesSPAccountScope(t *testing.T) { Run(func(args mock.Arguments) { got = args.Get(1).(*common.RecommendationParams) }). Return([]common.Recommendation{}, nil) - fetchRecommendationsForRegion(context.Background(), client, common.ServiceSavingsPlansCompute, spAPIRegion, + _, err := fetchRecommendationsForRegion(context.Background(), client, common.ServiceSavingsPlansCompute, spAPIRegion, Config{TermYears: 1, SPAccountScope: "payer"}) + require.NoError(t, err) require.NotNil(t, got) assert.Equal(t, "payer", got.SPAccountScope)