From b46a5baa44ad3955df7644d314d26e26eee99597 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 11 Jun 2026 01:01:49 -0700 Subject: [PATCH 1/2] fix(scheduler): propagate ambient fallback GetRecommendations error The AWS ambient/self-account path in fetchAndConvert retried with the global default term/payment when the primary sweep returned zero recommendations, but discarded the retry's error (recs, _ =). A misconfigured DefaultPayment/DefaultTerm or a Cost Explorer failure on that fallback was therefore invisible: the operator saw zero recommendations with no signal. Handle the error and fail loud: wrap it with provider, term, payment, and lookback context and return it so the collection surfaces the failure instead of silently presenting an empty result. Also fix the stale doc comments on the deprecated converters in providers/aws/recommendations/converters.go: they claimed to "silently default" to NoUpfront/OneYear/SevenDays, but since the *E variants were introduced they return the empty enum, which Cost Explorer rejects. Regression test TestScheduler_CollectAWSRecommendations_FallbackError asserts the fallback error is propagated with context and no recommendations are returned; it fails on the pre-fix code. Closes #1168 --- internal/scheduler/scheduler.go | 11 ++++-- internal/scheduler/scheduler_test.go | 42 +++++++++++++++++++++ providers/aws/recommendations/converters.go | 27 +++++++------ 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 7da7e3228..5cde7ec9f 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -879,10 +879,13 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, PaymentOption: globalCfg.DefaultPayment, LookbackPeriod: fmt.Sprintf("%dd", lookbackDays), } - var recErr error - recs, recErr = recClient.GetRecommendations(ctx, params) - if recErr != nil { - logging.Warnf("fetchAndConvert: %s GetRecommendations fallback failed: %v", providerName, recErr) + recs, err = recClient.GetRecommendations(ctx, params) + if err != nil { + // Fail loud: a misconfigured DefaultPayment/DefaultTerm or a CE + // failure on this fallback must surface to the operator instead + // of silently presenting as "zero recommendations". + return nil, fmt.Errorf("failed to get %s recommendations with default term/payment fallback (term=%s, payment=%s, lookback=%s): %w", + providerName, params.Term, params.PaymentOption, params.LookbackPeriod, err) } } result := s.convertRecommendations(recs, providerName) diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 596964251..0fc012ca0 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -1642,6 +1642,48 @@ func TestScheduler_CollectAWSRecommendations_FallbackToFiltered(t *testing.T) { assert.Len(t, recs, 1) } +// Regression test for COR-05 (#1168): when the primary sweep returns zero +// recommendations and the default term/payment fallback GetRecommendations +// call fails, the error must be propagated instead of being silently +// swallowed (`recs, _ =`) and presenting as "zero recommendations". +func TestScheduler_CollectAWSRecommendations_FallbackError(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockFactory := new(MockProviderFactory) + mockProvider := new(MockProvider) + mockRecClient := new(MockRecommendationsClient) + t.Cleanup(func() { + mockFactory.AssertExpectations(t) + mockProvider.AssertExpectations(t) + mockRecClient.AssertExpectations(t) + }) + + globalCfg := &config.GlobalConfig{ + DefaultTerm: 3, + DefaultPayment: "all-upfront", + } + + fallbackErr := errors.New("ValidationException: invalid PaymentOption") + + mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) + mockProvider.On("GetRecommendationsClient", ctx).Return(mockRecClient, nil) + mockRecClient.On("GetAllRecommendations", ctx).Return([]common.Recommendation{}, nil) // Empty -> triggers fallback + mockRecClient.On("GetRecommendations", ctx, mock.AnythingOfType("common.RecommendationParams")).Return(nil, fallbackErr) + + scheduler := &Scheduler{ + config: mockStore, + providerFactory: mockFactory, + } + + recs, _, err := scheduler.collectAWSRecommendations(ctx, globalCfg) + require.Error(t, err, "fallback GetRecommendations failure must propagate, not be swallowed") + require.ErrorIs(t, err, fallbackErr) + assert.Contains(t, err.Error(), "default term/payment fallback", "error must identify the fallback path") + assert.Contains(t, err.Error(), "term=3yr") + assert.Contains(t, err.Error(), "payment=all-upfront") + assert.Nil(t, recs, "no recommendations must be returned when the fallback fails") +} + // fakeSTSClient is a minimal in-test STSClient implementation used by the // ambient host-account tagging tests (issue #604). The fakeAccountID + err // fields are set by each test case to drive the GetCallerIdentity response diff --git a/providers/aws/recommendations/converters.go b/providers/aws/recommendations/converters.go index e4a7e457f..e9e6032f8 100644 --- a/providers/aws/recommendations/converters.go +++ b/providers/aws/recommendations/converters.go @@ -31,10 +31,11 @@ func getServiceStringForCostExplorer(service common.ServiceType) string { // convertPaymentOption converts payment option string to AWS type. // -// Deprecated: this function silently defaults to NoUpfront for unrecognized -// values and is retained only for the legacy RI recommendation-fetch path in -// client.go (owned by #865/#1075). New callers must use convertPaymentOptionE -// and propagate the error. See open-questions/fix-aws-converters.md OQ-1. +// Deprecated: this function silently returns the empty ("") PaymentOption for +// unrecognized values, which Cost Explorer rejects with a validation error. It +// is retained only for the legacy RI recommendation-fetch path in client.go +// (owned by #865/#1075). New callers must use convertPaymentOptionE and +// propagate the error. See open-questions/fix-aws-converters.md OQ-1. func convertPaymentOption(option string) types.PaymentOption { v, _ := convertPaymentOptionE(option) return v @@ -72,10 +73,11 @@ func convertTermInYearsE(term string) (types.TermInYears, error) { // convertTermInYears converts term string to AWS type. // -// Deprecated: this function silently defaults to OneYear for unrecognized -// values and is retained only for the legacy RI recommendation-fetch path in -// client.go (owned by #865/#1075). New callers must use convertTermInYearsE -// and propagate the error. See open-questions/fix-aws-converters.md OQ-1. +// Deprecated: this function silently returns the empty ("") TermInYears for +// unrecognized values, which Cost Explorer rejects with a validation error. It +// is retained only for the legacy RI recommendation-fetch path in client.go +// (owned by #865/#1075). New callers must use convertTermInYearsE and +// propagate the error. See open-questions/fix-aws-converters.md OQ-1. func convertTermInYears(term string) types.TermInYears { v, _ := convertTermInYearsE(term) return v @@ -99,10 +101,11 @@ func convertLookbackPeriodE(period string) (types.LookbackPeriodInDays, error) { // convertLookbackPeriod converts lookback period string to AWS type. // -// Deprecated: this function silently defaults to SevenDays for unrecognized -// values and is retained only for the legacy RI recommendation-fetch path in -// client.go (owned by #865/#1075). New callers must use convertLookbackPeriodE -// and propagate the error. See open-questions/fix-aws-converters.md OQ-1. +// Deprecated: this function silently returns the empty ("") LookbackPeriodInDays +// for unrecognized values, which Cost Explorer rejects with a validation error. +// It is retained only for the legacy RI recommendation-fetch path in client.go +// (owned by #865/#1075). New callers must use convertLookbackPeriodE and +// propagate the error. See open-questions/fix-aws-converters.md OQ-1. func convertLookbackPeriod(period string) types.LookbackPeriodInDays { v, _ := convertLookbackPeriodE(period) return v From aca70a08c6c836fbaf17a9afd2914b120d375a87 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 26 Jun 2026 18:36:25 +0200 Subject: [PATCH 2/2] docs(aws/recommendations): align converters_test docstrings with empty-enum behaviour The converters_test.go docstrings still claimed the legacy wrappers "silently default to NoUpfront/OneYear/SevenDays" -- the same stale claim that #1230 just fixed in converters.go. The wrappers actually return the empty ("") enum on unrecognized values, which Cost Explorer rejects with a validation error. This mirrors the converters.go doc-only fix into the sibling test file so the source-of-truth comment is consistent across both. No behaviour change; no test assertions modified. --- .../aws/recommendations/converters_test.go | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/providers/aws/recommendations/converters_test.go b/providers/aws/recommendations/converters_test.go index 231540c40..d428d1552 100644 --- a/providers/aws/recommendations/converters_test.go +++ b/providers/aws/recommendations/converters_test.go @@ -86,10 +86,12 @@ func TestGetServiceStringForCostExplorer(t *testing.T) { } func TestConvertPaymentOption(t *testing.T) { - // convertPaymentOption is the legacy wrapper that silently defaults to NoUpfront - // for unknown values (used by client.go RI path, owned by #865/#1075). - // This test documents that silent-default behaviour; new callers should use - // convertPaymentOptionE which returns an error on unrecognised values. + // convertPaymentOption is the legacy wrapper used by client.go (RI path, + // owned by #865/#1075); it silently returns the empty ("") PaymentOption + // for unrecognized values, which Cost Explorer then rejects with a + // validation error. This test covers the valid cases only; the fail-loud + // path is tested by TestConvertPaymentOptionE_FailLoud. New callers must + // use convertPaymentOptionE and propagate the error. tests := []struct { name string option string @@ -122,10 +124,11 @@ func TestConvertPaymentOption(t *testing.T) { // TestConvertPaymentOptionE_FailLoud is the regression test for H3: // convertPaymentOptionE must return an error on any unrecognised payment option -// instead of silently substituting NoUpfront (the old behaviour of the -// convertPaymentOption default branch). Callers on the SP recommendation path -// use this erroring variant so a typo or new/renamed option is caught -// before the wrong recs are queried. +// instead of silently substituting the empty ("") PaymentOption (the old +// behaviour of the convertPaymentOption default branch, which surfaced to CE +// as a confusing validation error). Callers on the SP recommendation path use +// this erroring variant so a typo or new/renamed option is caught before the +// wrong recs are queried. func TestConvertPaymentOptionE_FailLoud(t *testing.T) { tests := []struct { name string @@ -136,7 +139,7 @@ func TestConvertPaymentOptionE_FailLoud(t *testing.T) { {"All upfront", "all-upfront", types.PaymentOptionAllUpfront, false}, {"Partial upfront", "partial-upfront", types.PaymentOptionPartialUpfront, false}, {"No upfront", "no-upfront", types.PaymentOptionNoUpfront, false}, - // These must error, not default to NoUpfront (H3 regression guard): + // These must error, not return the empty PaymentOption (H3 regression guard): {"Unknown option errors", "unknown", "", true}, {"Empty string errors", "", "", true}, {"Mixed case errors", "All-Upfront", "", true}, @@ -158,7 +161,7 @@ func TestConvertPaymentOptionE_FailLoud(t *testing.T) { // TestConvertTermInYearsE_FailLoud is the regression test for L1: // convertTermInYearsE must error on unrecognised terms rather than silently -// defaulting to OneYear. +// returning the empty ("") TermInYears (which CE then rejects). func TestConvertTermInYearsE_FailLoud(t *testing.T) { tests := []struct { name string @@ -170,7 +173,7 @@ func TestConvertTermInYearsE_FailLoud(t *testing.T) { {"1 numeric", "1", types.TermInYearsOneYear, false}, {"3yr", "3yr", types.TermInYearsThreeYears, false}, {"3 numeric", "3", types.TermInYearsThreeYears, false}, - // These must error, not default to OneYear (L1 regression guard): + // These must error, not return the empty TermInYears (L1 regression guard): {"Unknown term errors", "unknown", "", true}, {"Empty string errors", "", "", true}, {"2yr errors", "2yr", "", true}, @@ -192,7 +195,7 @@ func TestConvertTermInYearsE_FailLoud(t *testing.T) { // TestConvertLookbackPeriodE_FailLoud is the regression test for L2: // convertLookbackPeriodE must error on unrecognised periods rather than -// silently defaulting to SevenDays. +// silently returning the empty ("") LookbackPeriodInDays (which CE then rejects). func TestConvertLookbackPeriodE_FailLoud(t *testing.T) { tests := []struct { name string @@ -206,7 +209,7 @@ func TestConvertLookbackPeriodE_FailLoud(t *testing.T) { {"30 numeric", "30", types.LookbackPeriodInDaysThirtyDays, false}, {"60d", "60d", types.LookbackPeriodInDaysSixtyDays, false}, {"60 numeric", "60", types.LookbackPeriodInDaysSixtyDays, false}, - // These must error, not default to SevenDays (L2 regression guard): + // These must error, not return the empty LookbackPeriodInDays (L2 regression guard): {"Unknown period errors", "unknown", "", true}, {"Empty string errors", "", "", true}, {"90d errors", "90d", "", true}, @@ -228,8 +231,10 @@ func TestConvertLookbackPeriodE_FailLoud(t *testing.T) { func TestConvertTermInYears(t *testing.T) { // convertTermInYears is the legacy wrapper used by client.go (RI path); - // it silently returns OneYear for unrecognised values. This test covers the - // valid cases only; the fail-loud path is tested by TestConvertTermInYearsE_FailLoud. + // it silently returns the empty ("") TermInYears for unrecognised values, + // which Cost Explorer then rejects with a validation error. This test + // covers the valid cases only; the fail-loud path is tested by + // TestConvertTermInYearsE_FailLoud. tests := []struct { name string term string @@ -267,8 +272,10 @@ func TestConvertTermInYears(t *testing.T) { func TestConvertLookbackPeriod(t *testing.T) { // convertLookbackPeriod is the legacy wrapper used by client.go (RI path); - // it silently returns SevenDays for unrecognised values. This test covers valid - // cases only; the fail-loud path is tested by TestConvertLookbackPeriodE_FailLoud. + // it silently returns the empty ("") LookbackPeriodInDays for unrecognised + // values, which Cost Explorer then rejects with a validation error. This + // test covers valid cases only; the fail-loud path is tested by + // TestConvertLookbackPeriodE_FailLoud. tests := []struct { name string period string