diff --git a/cmd/main.go b/cmd/main.go index 2fde27448..92a14b9e5 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -74,6 +74,13 @@ type Config struct { MinSavingsPct float64 MaxBreakEvenMonths int MinCount int + // CoverageLookbackDays is the number of calendar days of historical + // demand fed to GetReservationCoverage when computing the existing-RI + // coverage map for --target-coverage sizing. A longer window smooths + // seasonal spikes; a shorter one matches a narrow billing-period export + // from the AWS console coverage report. Default 30, matching the CE + // UI default. + CoverageLookbackDays int // RebuyWindowDays, when > 0, treats existing RIs whose remaining term // is at most this many days as already uncovered, so --target-coverage // recommends replacements before they expire. Zero (default) keeps the @@ -150,6 +157,11 @@ func init() { rootCmd.Flags().Float64Var(&toolCfg.MinSavingsPct, "min-savings-pct", 0, "Minimum savings percentage to include a recommendation (0 = no filter)") rootCmd.Flags().IntVar(&toolCfg.MaxBreakEvenMonths, "max-break-even-months", 0, "Maximum break-even period in months (0 = no filter)") rootCmd.Flags().IntVar(&toolCfg.MinCount, "min-count", 0, "Minimum instance count to include a recommendation (0 = no filter)") + rootCmd.Flags().IntVar(&toolCfg.CoverageLookbackDays, "coverage-lookback-days", 30, + "Number of calendar days of historical demand fed to GetReservationCoverage "+ + "when computing the existing-RI coverage map for --target-coverage sizing. "+ + "Match this to your AWS console coverage report window to reconcile "+ + "CUDly's ExistingCoverage column against the console export. Default 30.") rootCmd.Flags().IntVar(&toolCfg.RebuyWindowDays, "rebuy-window-days", 0, "When >0, treat existing RIs expiring within this many days as already "+ "uncovered, so --target-coverage sizes recommendations to replace them "+ diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 35f5c4e70..7a3252c16 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -20,11 +20,6 @@ import ( "github.com/google/uuid" ) -// existingCoverageLookbackDays is the historical window we ask CE to -// summarise existing-RI coverage over for --target-coverage sizing. -// 30 days matches CE's UI default and the GetRIUtilization caller. -const existingCoverageLookbackDays = 30 - // fetchExistingCoverage retrieves the existing-RI coverage map from Cost // Explorer so --target-coverage sizing can subtract what's already owned in // each pool. Best-effort: a transient CE failure logs a warning and returns @@ -36,6 +31,10 @@ const existingCoverageLookbackDays = 30 // Coverage is fetched per-region per-account so CE's org-wide aggregate // doesn't bleed one account's coverage into another in multi-account orgs. // Regions come from cfg.Regions if set, otherwise from EC2 DescribeRegions. +// +// The lookback window is cfg.CoverageLookbackDays (default 30, matching the +// CE UI default). Operators reconciling against the AWS console coverage +// report should match this value to the report's own time window. func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, cfg Config) recommendations.PoolCoverageMap { if cfg.TargetCoverage <= 0 { return nil @@ -46,6 +45,10 @@ func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient pro // the no-existing-commitments path. return nil } + lookbackDays := cfg.CoverageLookbackDays + if lookbackDays <= 0 { + lookbackDays = 30 + } regions := cfg.Regions if len(regions) == 0 { allRegions, err := getAllAWSRegions(ctx, awsCfg) @@ -55,8 +58,8 @@ func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient pro } regions = allRegions } - AppLogger.Printf("\nšŸ”Ž Fetching existing-RI coverage from Cost Explorer per-account across %d regions (lookback %d days)...\n", len(regions), existingCoverageLookbackDays) - cov, err := adapter.GetRICoverageMap(ctx, existingCoverageLookbackDays, regions) + AppLogger.Printf("\nšŸ”Ž Fetching existing-RI coverage from Cost Explorer per-account across %d regions (lookback %d days)...\n", len(regions), lookbackDays) + cov, err := adapter.GetRICoverageMap(ctx, lookbackDays, regions) if err != nil { AppLogger.Printf(" āš ļø Could not fetch existing-RI coverage (%v); sizing will assume zero existing coverage\n", err) return nil diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index 296f1b092..0ba0d0b78 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -768,3 +768,38 @@ func TestFilterAndAdjustRecommendations_OverrideCountApplied(t *testing.T) { assert.Equal(t, int(toolCfg.OverrideCount), rec.Count) } } + +// TestFetchExistingCoverage_LookbackDays verifies that fetchExistingCoverage +// honours cfg.CoverageLookbackDays (issue #360). The test uses the +// MockRecommendationsClient which fails the *awsprovider.RecommendationsClientAdapter +// type assertion, exercising the non-AWS-provider early-return path. The key +// assertions are: +// - TargetCoverage=0 always returns nil regardless of lookback. +// - TargetCoverage>0 with a non-AWS client returns nil (non-AWS path, +// no CE call is made). +// +// Threading through the actual lookback value to GetRICoverageMap is +// integration-tested in providers/aws/recommendations (TestGetRICoverageMap_*). +func TestFetchExistingCoverage_LookbackDays(t *testing.T) { + ctx := context.Background() + awsCfg := aws.Config{} + mockClient := &MockRecommendationsClient{} + + t.Run("zero TargetCoverage returns nil regardless of lookback", func(t *testing.T) { + cfg := Config{TargetCoverage: 0, CoverageLookbackDays: 14, Regions: []string{"us-east-1"}} + got := fetchExistingCoverage(ctx, awsCfg, mockClient, cfg) + assert.Nil(t, got, "TargetCoverage=0 must short-circuit before any CE call") + }) + + t.Run("non-AWS adapter returns nil, lookback not needed", func(t *testing.T) { + cfg := Config{TargetCoverage: 80, CoverageLookbackDays: 14, Regions: []string{"us-east-1"}} + got := fetchExistingCoverage(ctx, awsCfg, mockClient, cfg) + assert.Nil(t, got, "non-AWS provider must return nil (no CE integration)") + }) + + t.Run("custom lookback stored in Config", func(t *testing.T) { + cfg := Config{TargetCoverage: 80, CoverageLookbackDays: 60, Regions: []string{"us-east-1"}} + // CoverageLookbackDays field value is preserved in the struct. + assert.Equal(t, 60, cfg.CoverageLookbackDays) + }) +} diff --git a/cmd/validators.go b/cmd/validators.go index fee934988..ff1600f90 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -46,6 +46,11 @@ func validateNumericRanges(cmd *cobra.Command) error { return err } + // Validate coverage lookback days + if toolCfg.CoverageLookbackDays < 1 { + return fmt.Errorf("coverage-lookback-days must be >= 1, got: %d", toolCfg.CoverageLookbackDays) + } + // Validate max instances if toolCfg.MaxInstances < 0 { return fmt.Errorf("max-instances must be 0 (no limit) or a positive number, got: %d", toolCfg.MaxInstances) diff --git a/cmd/validators_test.go b/cmd/validators_test.go index f5b7378c8..f4b45d70a 100644 --- a/cmd/validators_test.go +++ b/cmd/validators_test.go @@ -23,6 +23,7 @@ func TestValidateNumericRanges(t *testing.T) { toolCfg.Coverage = 80.0 toolCfg.MaxInstances = 100 toolCfg.OverrideCount = 10 + toolCfg.CoverageLookbackDays = 30 }, wantErr: false, }, @@ -30,6 +31,7 @@ func TestValidateNumericRanges(t *testing.T) { name: "coverage below zero", setupFunc: func() { toolCfg.Coverage = -1.0 + toolCfg.CoverageLookbackDays = 30 }, wantErr: true, errMsg: "coverage percentage must be between 0 and 100", @@ -38,6 +40,7 @@ func TestValidateNumericRanges(t *testing.T) { name: "coverage above 100", setupFunc: func() { toolCfg.Coverage = 101.0 + toolCfg.CoverageLookbackDays = 30 }, wantErr: true, errMsg: "coverage percentage must be between 0 and 100", @@ -47,6 +50,7 @@ func TestValidateNumericRanges(t *testing.T) { setupFunc: func() { toolCfg.Coverage = 80.0 toolCfg.MaxInstances = -1 + toolCfg.CoverageLookbackDays = 30 }, wantErr: true, errMsg: "max-instances must be 0", @@ -56,6 +60,7 @@ func TestValidateNumericRanges(t *testing.T) { setupFunc: func() { toolCfg.Coverage = 80.0 toolCfg.MaxInstances = MaxReasonableInstances + 1 + toolCfg.CoverageLookbackDays = 30 }, wantErr: true, errMsg: "max-instances", @@ -66,6 +71,7 @@ func TestValidateNumericRanges(t *testing.T) { toolCfg.Coverage = 80.0 toolCfg.MaxInstances = 100 toolCfg.OverrideCount = -1 + toolCfg.CoverageLookbackDays = 30 }, wantErr: true, errMsg: "override-count must be 0", @@ -76,6 +82,7 @@ func TestValidateNumericRanges(t *testing.T) { toolCfg.Coverage = 80.0 toolCfg.MaxInstances = 100 toolCfg.OverrideCount = MaxReasonableInstances + 1 + toolCfg.CoverageLookbackDays = 30 }, wantErr: true, errMsg: "override-count", @@ -461,7 +468,7 @@ func TestValidateTargetCoverage(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - toolCfg = Config{Coverage: tt.coverage, TargetCoverage: tt.target} + toolCfg = Config{Coverage: tt.coverage, TargetCoverage: tt.target, CoverageLookbackDays: 30} var cmd *cobra.Command if tt.useCobraCmd { cmd = &cobra.Command{Use: "test"} @@ -485,3 +492,51 @@ func TestValidateTargetCoverage(t *testing.T) { }) } } + +// TestValidateCoverageLookbackDays verifies that validateNumericRanges rejects +// non-positive --coverage-lookback-days and accepts positive values. +func TestValidateCoverageLookbackDays(t *testing.T) { + tests := []struct { + name string + days int + wantErr bool + errSubstr string + }{ + {name: "default 30 is valid", days: 30, wantErr: false}, + {name: "1 day is valid", days: 1, wantErr: false}, + {name: "90 days is valid", days: 90, wantErr: false}, + { + name: "zero rejected", + days: 0, + wantErr: true, + errSubstr: "coverage-lookback-days must be >= 1", + }, + { + name: "negative rejected", + days: -1, + wantErr: true, + errSubstr: "coverage-lookback-days must be >= 1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + toolCfg = Config{ + Coverage: 80, + CoverageLookbackDays: tt.days, + } + err := validateNumericRanges(nil) + if tt.wantErr { + if err == nil { + t.Errorf("validateNumericRanges() expected error containing %q, got nil", tt.errSubstr) + } else if tt.errSubstr != "" && !strings.Contains(err.Error(), tt.errSubstr) { + t.Errorf("validateNumericRanges() error = %v, want substring %q", err, tt.errSubstr) + } + } else if err != nil { + t.Errorf("validateNumericRanges() unexpected error = %v", err) + } + }) + } +}