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 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