From c969c5fafa10fcd7f444aceb89c208392c7c6000 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 21:00:18 +0200 Subject: [PATCH 1/3] fix(cmd): fail the run when recommendation fetching fails fetchRecommendationsForRegion logged provider errors and returned nil, and fetchAllRecs logged region-discovery failures and continued, so an expired session or denied Cost Explorer call read as "Nothing to purchase" with exit 0. Both now return the error and the run exits 1. Empty answers and incomplete-but-usable answers keep their current behaviour. Refs #2190 Co-Authored-By: Claude Sonnet 5.5 --- cmd/cancellation_prepurchase_test.go | 32 +++++++++++------ cmd/multi_service.go | 10 +++++- cmd/multi_service_helpers.go | 41 +++++++++++++++------- cmd/recommendation_completeness_test.go | 46 +++++++++++-------------- cmd/savings_plan_purchase_test.go | 3 +- cmd/savings_plan_region_test.go | 3 +- cmd/sp_account_scope_test.go | 3 +- 7 files changed, 87 insertions(+), 51 deletions(-) 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) From e543c3465d637dde2d0f1781f70bf94cca2421e0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 21:17:52 +0200 Subject: [PATCH 2/3] fix(cmd): discover recommendation regions from Cost Explorer Region iteration used ec2:DescribeRegions, which lists only the caller's opted-in regions. Cost Explorer returns linked-account recommendations in regions the caller has not opted into, so a default run dropped them (25 recommendations from the API, 23 from the CLI on a live payer). The regions now come from the recommendations for the run's term, payment option and lookback. DescribeRegions is no longer called for them. Refs #2189 Co-Authored-By: Claude Sonnet 5.5 --- cmd/cancellation_prepurchase_test.go | 48 ++++++++++++++-- cmd/multi_service.go | 2 +- cmd/multi_service_helpers.go | 56 ++++++------------- cmd/multi_service_helpers_test.go | 7 ++- cmd/recommendation_completeness_proxy_test.go | 21 +++---- cmd/recommendation_completeness_test.go | 14 +---- cmd/savings_plan_purchase_test.go | 4 +- 7 files changed, 78 insertions(+), 74 deletions(-) diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index 9aa5bdb37..e51f815ff 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -71,15 +71,55 @@ func TestFetchExistingCoverage_CanceledContextIsTerminalEvenOnDryRun(t *testing. assert.Nil(t, got) } -func TestHandleRegionDiscoveryError_CanceledContextDoesNotFallBackToDiscovery(t *testing.T) { +func TestDetermineRegionsForService_CanceledContextIsTerminal(t *testing.T) { captureAppLog(t) - client := &MockRecommendationsClient{} // no expectations: any call fails the test + client := &MockRecommendationsClient{} + client.On("GetRecommendations", mock.Anything, mock.Anything).Return(nil, canceledCtx().Err()) - regions, err := handleRegionDiscoveryError(canceledCtx(), client, common.ServiceRDS, errors.New("describe regions: context canceled")) + regions, err := determineRegionsForService(canceledCtx(), client, common.ServiceRDS, Config{TermYears: 1}) require.ErrorIs(t, err, context.Canceled) assert.Nil(t, regions) - client.AssertNotCalled(t, "GetRecommendationsForService", mock.Anything, mock.Anything) +} + +func TestDetermineRegionsForService_DiscoversRegionsTheCallerHasNotOptedInto(t *testing.T) { + captureAppLog(t) + cfg := Config{TermYears: 1, PaymentOption: "no-upfront", Coverage: 100} + rec := func(region string) common.Recommendation { + return common.Recommendation{Service: common.ServiceEC2, Region: region, ResourceType: "m5.large", Count: 1} + } + client := &MockRecommendationsClient{} + // aws.Config{} has no credentials, so any ec2:DescribeRegions call would + // fail: the regions must come from the recommendations. + client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "" })). + Return([]common.Recommendation{rec("us-east-1"), rec("eu-central-2"), rec("eu-central-2")}, nil).Once() + + regions, err := determineRegionsForService(context.Background(), client, common.ServiceEC2, cfg) + + require.NoError(t, err) + assert.Equal(t, []string{"eu-central-2", "us-east-1"}, regions) +} + +func TestFetchAllRecs_DefaultRunKeepsRecommendationsInRegionsOutsideTheCallersOptIns(t *testing.T) { + captureAppLog(t) + cfg := Config{TermYears: 1, PaymentOption: "no-upfront", Coverage: 100} + rec := func(region string) common.Recommendation { + return common.Recommendation{Service: common.ServiceRDS, Region: region, ResourceType: "db.t3.small", Count: 3, EstimatedSavings: 10, SavingsPercentage: 10} + } + client := &MockRecommendationsClient{} + // Cost Explorer answers the same account-wide list for every call; the + // per-region pass keeps the rows of its own region. + client.On("GetRecommendations", mock.Anything, mock.Anything). + Return([]common.Recommendation{rec("us-east-1"), rec("eu-central-2")}, nil) + + recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCache(aws.Config{}), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + + require.NoError(t, err) + var regions []string + for i := range recs { + regions = append(regions, recs[i].Region) + } + assert.ElementsMatch(t, []string{"us-east-1", "eu-central-2"}, regions) } func TestFetchRecommendationsForRegion_InterruptIsNotReportedAsProviderFailure(t *testing.T) { diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 1636cf2aa..179a27579 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -915,7 +915,7 @@ func filterAndAdjustRecommendations(ctx context.Context, recs []common.Recommend // processService processes a single service and returns recommendations and results. // Used by legacy callers; new code should use fetchAllRecs + executePurchasePipeline. func processService(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, accountCache *AccountAliasCache, service common.ServiceType, isDryRun bool, cfg Config, engineData engineVersionData) ([]common.Recommendation, []common.PurchaseResult) { //nolint:unparam // engineData always nil at current callsites but param is part of the API - regionsToProcess, err := determineRegionsForService(ctx, awsCfg, recClient, service, cfg.Regions) + regionsToProcess, err := determineRegionsForService(ctx, recClient, service, cfg) if err != nil { log.Printf("āŒ Failed to determine regions: %v", err) return nil, nil diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 87cd19f40..627932f48 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -109,19 +109,14 @@ func getAllAWSRegionsWithClient(ctx context.Context, ec2Client EC2ClientInterfac return regions, nil } -// discoverRegionsForService discovers regions that have recommendations for a specific service. -func discoverRegionsForService(ctx context.Context, client provider.RecommendationsClient, service common.ServiceType) ([]string, error) { - recs, err := client.GetRecommendationsForService(ctx, service) - var incomplete *recommendations.IncompleteRecommendationsError - if errors.As(err, &incomplete) { - AppLogger.Printf(" āš ļø Region discovery incomplete: %v\n", incomplete) - } else if partial := azureprovider.AsPartialSubscriptionFailure(err); partial != nil { - // Region discovery is best-effort: the subscriptions that answered - // still tell us where to look. Report the gap rather than dropping - // the discovered regions or failing outright. - AppLogger.Printf(" āš ļø Region discovery incomplete: %d of %d Azure subscriptions succeeded\n", - partial.Succeeded, partial.Attempted) - } else if err != nil { +// discoverRegionsForService returns the regions Cost Explorer has +// recommendations in for the run's term, payment option and lookback. The +// regions come from the recommendations themselves, not from ec2:DescribeRegions: +// Cost Explorer returns linked-account recommendations in regions the caller +// has not opted into, and iterating only the caller's regions dropped them. +func discoverRegionsForService(ctx context.Context, client provider.RecommendationsClient, service common.ServiceType, cfg Config) ([]string, error) { + recs, err := fetchRecommendationsForRegion(ctx, client, service, "", cfg) + if err != nil { return nil, err } @@ -316,7 +311,7 @@ func executePurchase(ctx context.Context, rec common.Recommendation, region stri } // determineRegionsForService determines which regions to process for a given service. -func determineRegionsForService(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, service common.ServiceType, configuredRegions []string) ([]string, error) { +func determineRegionsForService(ctx context.Context, recClient provider.RecommendationsClient, service common.ServiceType, cfg Config) ([]string, error) { // Savings Plans are account-level, not regional - only query once, even // when --regions lists several. The recommendation call returns the same // account-wide set for every region, so honoring --regions here repeats @@ -330,35 +325,18 @@ func determineRegionsForService(ctx context.Context, awsCfg aws.Config, recClien } // If regions are explicitly configured, use those - if len(configuredRegions) > 0 { - return configuredRegions, nil - } - - // Default to all AWS regions for other services - AppLogger.Printf("šŸŒ Processing all AWS regions for %s...\n", getServiceDisplayName(service)) - allRegions, err := getAllAWSRegions(ctx, awsCfg) - if err != nil { - return handleRegionDiscoveryError(ctx, recClient, service, err) - } - - AppLogger.Printf("šŸ“ Processing %d region(s)\n", len(allRegions)) - return allRegions, nil -} - -// handleRegionDiscoveryError handles errors during region discovery by falling back to auto-discovery. -func handleRegionDiscoveryError(ctx context.Context, recClient provider.RecommendationsClient, service common.ServiceType, originalErr error) ([]string, error) { - if ctxErr := ctx.Err(); ctxErr != nil { - return nil, ctxErr + if len(cfg.Regions) > 0 { + return cfg.Regions, nil } - AppLogger.Printf("āŒ Failed to get AWS regions: %v\n", originalErr) - AppLogger.Printf("šŸ” Falling back to auto-discovery...\n") - discoveredRegions, err := discoverRegionsForService(ctx, recClient, service) + AppLogger.Printf("šŸŒ Discovering regions with %s recommendations...\n", getServiceDisplayName(service)) + regions, err := discoverRegionsForService(ctx, recClient, service, cfg) if err != nil { - return nil, fmt.Errorf("failed to discover regions: %w", err) + return nil, err } - return discoveredRegions, nil + AppLogger.Printf("šŸ“ Processing %d region(s)\n", len(regions)) + return regions, nil } // engineVersionData holds the results of engine version queries. @@ -940,7 +918,7 @@ func fetchAllRecs( AppLogger.Printf("šŸ” Fetching %s recommendations\n", getServiceDisplayName(service)) AppLogger.Printf("ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”ā”\n") - regions, err := determineRegionsForService(ctx, awsCfg, recClient, service, cfg.Regions) + regions, err := determineRegionsForService(ctx, recClient, service, cfg) if err != nil { if ctx.Err() != nil { return nil, drops, nil diff --git a/cmd/multi_service_helpers_test.go b/cmd/multi_service_helpers_test.go index a67a19798..dfee0d931 100644 --- a/cmd/multi_service_helpers_test.go +++ b/cmd/multi_service_helpers_test.go @@ -161,10 +161,11 @@ func TestDiscoverRegionsForService(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockClient := &MockRecommendationsClient{} - mockClient.On("GetRecommendationsForService", ctx, tt.service).Return(tt.mockReturns, nil) + mockClient.On("GetRecommendations", ctx, mock.MatchedBy(func(p *common.RecommendationParams) bool { + return p.Service == tt.service && p.Region == "" + })).Return(tt.mockReturns, nil) - // Now we can use the actual function directly since it accepts an interface - regions, err := discoverRegionsForService(ctx, mockClient, tt.service) + regions, err := discoverRegionsForService(ctx, mockClient, tt.service, Config{TermYears: 1}) assert.NoError(t, err) assert.Equal(t, tt.expectedRegions, regions) diff --git a/cmd/recommendation_completeness_proxy_test.go b/cmd/recommendation_completeness_proxy_test.go index 8c9359a8a..08ec61fe1 100644 --- a/cmd/recommendation_completeness_proxy_test.go +++ b/cmd/recommendation_completeness_proxy_test.go @@ -238,9 +238,6 @@ func (p *completenessProxy) operation(host, op string, call int) (int, string, s return 200, "application/x-amz-json-1.1", `{"Recommendations":[{"RecommendationDetails":[` + details[p.details] + `]}]}` } if host == "ec2.us-east-1.amazonaws.com" && op == "DescribeRegions" { - if p.regions == "fallback" && call == 1 { - return 400, "text/xml", `UnauthorizedOperationfixture denied region discovery` - } return 200, "text/xml", `us-east-1` } p.t.Errorf("unexpected operation %s %s", host, op) @@ -264,20 +261,16 @@ func (p *completenessProxy) assertRequests(t *testing.T) { p.assertSPRequests(t) return } + // Region discovery is one Cost Explorer call; each discovered region is + // then fetched once. Discovery finds a region only when a recommendation + // survived parsing. ceCalls := 1 - if p.regions == "fallback" { - ceCalls = 6 - if p.details == "valid" || p.details == "mixed" { - ceCalls++ - } + if p.regions == "default" && (p.details == "valid" || p.details == "mixed") { + ceCalls++ } require.Equal(t, ceCalls, p.requests["AWSInsightsIndexService.GetReservationPurchaseRecommendation"], "operations=%v", p.requests) - // --include-extended-support skips the RDS exclusion queries, so only region discovery lists regions. - wantRegions := 1 - if p.regions == "explicit" { - wantRegions = 0 - } - require.Equal(t, wantRegions, p.requests["DescribeRegions"], "region calls") + // Regions come from the recommendations; DescribeRegions is never called. + require.Zero(t, p.requests["DescribeRegions"], "region calls") t.Logf("actual root command, config loader, SDK and CSV; synthetic operations=%v", p.requests) } diff --git a/cmd/recommendation_completeness_test.go b/cmd/recommendation_completeness_test.go index 9368d2761..c56217490 100644 --- a/cmd/recommendation_completeness_test.go +++ b/cmd/recommendation_completeness_test.go @@ -28,7 +28,7 @@ func TestRecommendationCompletenessCommand(t *testing.T) { runCompletenessCommandChild(t) return } - for _, regionMode := range []string{"explicit", "default", "fallback"} { + for _, regionMode := range []string{"explicit", "default"} { for _, details := range []string{"valid", "mixed", "all-invalid", "empty", "api-error"} { t.Run(regionMode+"/"+details, func(t *testing.T) { runCompletenessScenario(t, "rds", regionMode, details) @@ -187,17 +187,10 @@ func assertCompletenessDiagnostics(t *testing.T, regionMode, details, stdout, st t.Helper() malformed := details == "mixed" || details == "all-invalid" if malformed { - count := 1 - if regionMode == "fallback" { - count = 6 - } - want := fmt.Sprintf("incomplete AWS recommendations: %d failed details, 0 failed scopes:", count) + want := "incomplete AWS recommendations: 1 failed details, 0 failed scopes:" for _, diagnostic := range []string{want, "not-a-number", "detail"} { require.Contains(t, stdout, diagnostic, "stderr: %s", stderr) } - if regionMode == "fallback" { - require.Contains(t, stdout, "Region discovery incomplete") - } } else { require.NotContains(t, stdout, "incomplete AWS recommendations") } @@ -253,9 +246,8 @@ func TestRecommendationCompletenessHelpers(t *testing.T) { t.Cleanup(func() { AppLogger.SetOutput(previous) }) ctx := context.Background() client := &MockRecommendationsClient{} - 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) + regions, err := discoverRegionsForService(ctx, client, common.ServiceRDS, 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) diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index fcb28beea..5540a8c47 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -24,14 +24,14 @@ func TestDetermineRegionsForService_SavingsPlanCollectedOnceWithConfiguredRegion common.ServiceSavingsPlansCompute, common.ServiceSavingsPlansEC2Instance, common.ServiceSavingsPlansSageMaker, common.ServiceSavingsPlansDatabase, } { - regions, err := determineRegionsForService(context.Background(), aws.Config{}, nil, svc, []string{"eu-west-1", "us-west-2", "ap-south-1"}) + regions, err := determineRegionsForService(context.Background(), nil, svc, Config{Regions: []string{"eu-west-1", "us-west-2", "ap-south-1"}}) require.NoError(t, err) assert.Equal(t, []string{spAPIRegion}, regions, svc) } } func TestDetermineRegionsForService_ConfiguredRegionsKeptForRegionalServices(t *testing.T) { - regions, err := determineRegionsForService(context.Background(), aws.Config{}, nil, common.ServiceRDS, []string{"eu-west-1", "us-west-2"}) + regions, err := determineRegionsForService(context.Background(), nil, common.ServiceRDS, Config{Regions: []string{"eu-west-1", "us-west-2"}}) require.NoError(t, err) assert.Equal(t, []string{"eu-west-1", "us-west-2"}, regions) } From 52b3b1319f6779f398eec49ea33db5ec9e0c63dc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 23:15:34 +0200 Subject: [PATCH 3/3] test(cmd): fix lint findings in the region discovery tests Co-Authored-By: Claude Sonnet 5.5 --- cmd/cancellation_prepurchase_test.go | 2 +- cmd/recommendation_completeness_proxy_test.go | 5 ++--- cmd/recommendation_completeness_test.go | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index e51f815ff..9e5c8b7f1 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -115,7 +115,7 @@ func TestFetchAllRecs_DefaultRunKeepsRecommendationsInRegionsOutsideTheCallersOp recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCache(aws.Config{}), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) require.NoError(t, err) - var regions []string + regions := make([]string, 0, len(recs)) for i := range recs { regions = append(regions, recs[i].Region) } diff --git a/cmd/recommendation_completeness_proxy_test.go b/cmd/recommendation_completeness_proxy_test.go index 08ec61fe1..d5d3a4f11 100644 --- a/cmd/recommendation_completeness_proxy_test.go +++ b/cmd/recommendation_completeness_proxy_test.go @@ -185,9 +185,8 @@ func (p *completenessProxy) respond(req *http.Request) *http.Response { } p.mu.Lock() p.requests[op]++ - call := p.requests[op] p.mu.Unlock() - status, contentType, payload := p.operation(req.Host, op, call) + status, contentType, payload := p.operation(req.Host, op) if p.service != "rds" && req.Host == "ce.us-east-1.amazonaws.com" && op == "AWSInsightsIndexService.GetSavingsPlansPurchaseRecommendation" { status, contentType, payload = p.spRecommendation(body) } @@ -196,7 +195,7 @@ func (p *completenessProxy) respond(req *http.Request) *http.Response { Body: io.NopCloser(strings.NewReader(payload)), ContentLength: int64(len(payload))} } -func (p *completenessProxy) operation(host, op string, call int) (int, string, string) { +func (p *completenessProxy) operation(host, op string) (int, string, string) { responses := map[string]string{ "DescribeDBInstances": "DBInstances", "DescribeDBMajorEngineVersions": "DBMajorEngineVersions", diff --git a/cmd/recommendation_completeness_test.go b/cmd/recommendation_completeness_test.go index c56217490..4f897eb01 100644 --- a/cmd/recommendation_completeness_test.go +++ b/cmd/recommendation_completeness_test.go @@ -90,7 +90,7 @@ func runCompletenessScenario(t *testing.T, service, regionMode, details string) case rdsExpiryService: assertRDSExpiryCSV(t, output, details, logs) case "rds": - assertCompletenessDiagnostics(t, regionMode, details, stdout.String(), stderr.String()) + assertCompletenessDiagnostics(t, details, stdout.String(), stderr.String()) assertCompletenessCSV(t, output, details, logs) default: assertSPCompleteness(t, output, service, details, logs) @@ -183,7 +183,7 @@ func assertSPCompleteness(t *testing.T, output, service, details, logs string) { require.Contains(t, logs, "CSV report written") } -func assertCompletenessDiagnostics(t *testing.T, regionMode, details, stdout, stderr string) { +func assertCompletenessDiagnostics(t *testing.T, details, stdout, stderr string) { t.Helper() malformed := details == "mixed" || details == "all-invalid" if malformed {