diff --git a/CHANGELOG.md b/CHANGELOG.md index 856759c8a..a4d9cf771 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `--regions` now also narrows EC2 Instance Savings Plan recommendations to the listed + regions by the plan's own region (previously only `--include-regions` did); a plan + with no known region is dropped. Compute, SageMaker and Database plans stay + unfiltered. In `--input-csv` mode `--regions` still does not narrow Savings + Plans (rows are used as supplied). - A `--purchase` run now stops, and exits 1, when an audit record cannot be written durably (disk full, log removed or made unwritable mid-run) instead of warning and buying more with no audit trail. The audit log is also probed diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 17ad45adb..978c79546 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -127,11 +127,24 @@ func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool { // Recommendations from other providers are unaffected: both predicates are // gated on common.CommitmentSavingsPlan and fall through to the plain // rec.Region comparison for everything else. +// +// A region-scoped Savings Plan (EC2 Instance, or one whose region Cost +// Explorer omitted) is also narrowed by --regions on its own region. Savings +// Plans are fetched once, not per region (determineRegionsForService), so +// --regions never reaches them through the fetch loop. An unknown region +// does not match, so it is dropped rather than bought outside the scope. +// Not applied in --input-csv mode: CSV rows carry no Provider or +// CommitmentType, so their Details.Region is not readable here, and the rows +// are used as supplied. func shouldIncludeRecommendationRegion(rec *common.Recommendation, cfg *Config) bool { if awsprovider.IsRegionAgnostic(*rec) { return true } - return shouldIncludeRegion(awsprovider.EffectiveRegion(*rec), cfg) + region := awsprovider.EffectiveRegion(*rec) + if cfg.CSVInput == "" && common.IsSavingsPlan(rec.Service) && len(cfg.Regions) > 0 && !slices.Contains(cfg.Regions, region) { + return false + } + return shouldIncludeRegion(region, cfg) } // shouldIncludeRegion checks if a region should be included based on filters. diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index f5982abb0..5c913376e 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -3,6 +3,8 @@ package main import ( "fmt" "log" + "os" + "path/filepath" "testing" "time" @@ -753,3 +755,86 @@ func TestApplyFilters_MinPoolSizeMultiRegionMatchesPreExtractionBehaviour(t *tes assert.Equal(t, distinctBelowThreshold*len(regions), totalDropMinPoolSizeNew, "summed multi-region --min-pool-size drop count should match today's pre-existing inflation factor") } + +// TestApplyFilters_SavingsPlansRegionsFlag pins that --regions narrows +// region-scoped (EC2 Instance) Savings Plans by Details.Region while +// account-level plans stay unfiltered, and that it ANDs with +// --include-regions / --exclude-regions. +func TestApplyFilters_SavingsPlansRegionsFlag(t *testing.T) { + sp := func(svc common.ServiceType, planType, region string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, + Service: svc, + CommitmentType: common.CommitmentSavingsPlan, + Count: 1, + Details: &common.SavingsPlanDetails{PlanType: planType, Region: region}, + } + } + ec2 := func(region string) common.Recommendation { + return sp(common.ServiceSavingsPlansEC2Instance, "EC2Instance", region) + } + + tests := []struct { + name string + rec common.Recommendation + regions []string + include []string + exclude []string + want bool + }{ + {"EC2 SP in another region dropped", ec2("us-east-1"), []string{"eu-west-2"}, nil, nil, false}, + {"EC2 SP in listed region kept", ec2("eu-west-2"), []string{"eu-west-2"}, nil, nil, true}, + {"Compute SP kept", sp(common.ServiceSavingsPlansCompute, "Compute", ""), []string{"eu-west-2"}, nil, nil, true}, + {"SageMaker SP kept", sp(common.ServiceSavingsPlansSageMaker, "SageMaker", ""), []string{"eu-west-2"}, nil, nil, true}, + {"Database SP kept", sp(common.ServiceSavingsPlansDatabase, "Database", ""), []string{"eu-west-2"}, nil, nil, true}, + {"EC2 SP with unknown region dropped", ec2(""), []string{"eu-west-2"}, nil, nil, false}, + {"empty --regions leaves EC2 SP alone", ec2("us-east-1"), nil, nil, nil, true}, + {"regions AND include: outside include dropped", ec2("eu-west-2"), []string{"eu-west-2"}, []string{"us-east-1"}, nil, false}, + {"regions AND include: both match kept", ec2("eu-west-2"), []string{"eu-west-2", "us-east-1"}, []string{"eu-west-2"}, nil, true}, + {"regions AND exclude: excluded dropped", ec2("us-east-1"), []string{"eu-west-2", "us-east-1"}, nil, []string{"us-east-1"}, false}, + {"regions AND exclude: other kept", ec2("eu-west-2"), []string{"eu-west-2", "us-east-1"}, nil, []string{"us-east-1"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := Config{Regions: tt.regions, IncludeRegions: tt.include, ExcludeRegions: tt.exclude} + got := applyFilters([]common.Recommendation{tt.rec}, &cfg, nil, nil, "", common.NewDropSummary()) + assert.Equal(t, tt.want, len(got) == 1) + }) + } + + t.Run("reservation rec is not narrowed by --regions here", func(t *testing.T) { + ri := common.Recommendation{Service: common.ServiceRDS, CommitmentType: common.CommitmentReservedInstance, Region: "us-east-1", Count: 1} + got := applyFilters([]common.Recommendation{ri}, &Config{Regions: []string{"eu-west-2"}}, nil, nil, "", common.NewDropSummary()) + assert.Len(t, got, 1) + }) + + t.Run("eu-west-2 scenario", func(t *testing.T) { + recs := []common.Recommendation{ec2("us-east-1"), ec2("eu-west-2"), sp(common.ServiceSavingsPlansCompute, "Compute", "")} + got := applyFilters(recs, &Config{Regions: []string{"eu-west-2"}}, nil, nil, "", common.NewDropSummary()) + if assert.Len(t, got, 2) { + assert.Equal(t, common.ServiceSavingsPlansEC2Instance, got[0].Service) + assert.Equal(t, "eu-west-2", got[0].Details.(*common.SavingsPlanDetails).Region) + assert.Equal(t, common.ServiceSavingsPlansCompute, got[1].Service) + } + }) +} + +// TestApplyFilters_SavingsPlansRegionsFlagCSVModeUnchanged pins that +// --regions does not narrow Savings Plan rows loaded from --input-csv: CSV +// rows carry no Provider or CommitmentType, so they are used as supplied. +func TestApplyFilters_SavingsPlansRegionsFlagCSVModeUnchanged(t *testing.T) { + csvPath := filepath.Join(t.TempDir(), "sp.csv") + content := `Service,Region,ResourceType,Count,Term,PaymentOption,PlanType,HourlyCommitment,InstanceFamily,DetailsRegion +savings-plans-compute,,,1,1yr,no-upfront,Compute,1.5,, +savings-plans-ec2instance,,,1,1yr,no-upfront,EC2Instance,1.5,m5,eu-west-2 +savings-plans-ec2instance,,,1,1yr,no-upfront,EC2Instance,1.5,m5,us-east-1 +` + require.NoError(t, os.WriteFile(csvPath, []byte(content), 0o600)) + recs, err := loadRecommendationsFromCSV(csvPath) + require.NoError(t, err) + require.Len(t, recs, 3) + + cfg := Config{CSVInput: csvPath, Regions: []string{"eu-west-2"}} + got := applyFilters(recs, &cfg, nil, nil, "", common.NewDropSummary()) + assert.Len(t, got, 3) +} diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 63210433a..3a9f8d0b7 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -317,8 +317,8 @@ func determineRegionsForService(ctx context.Context, recClient provider.Recommen // account-wide set for every region, so honoring --regions here repeats // each Savings Plan once per listed region and, now that Savings Plan // purchases work, would buy it that many times. EC2Instance plans are - // still narrowed to the requested regions by the region filters on - // Details.Region. + // still narrowed to the requested regions by shouldIncludeRecommendationRegion + // on Details.Region. if common.IsSavingsPlan(service) { AppLogger.Printf("🌍 Fetching account-level Savings Plans recommendations...\n") return []string{spAPIRegion}, nil // Single query for account-level data diff --git a/docs/cli/README.md b/docs/cli/README.md index b853b757f..daf3bb7de 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -24,7 +24,7 @@ All flags belong to the root command unless noted otherwise. |------|-------|---------|-------------| | `--services` | `-s` | `rds` | Comma-separated list of services to process. Valid values: `rds`, `elasticache`, `ec2`, `opensearch`, `redshift`, `memorydb`, `savingsplans` (fans out to all four SP types), `savingsplans-compute`, `savingsplans-ec2instance`, `savingsplans-sagemaker`, `savingsplans-database`. The legacy alias `elasticsearch` maps to `opensearch`. | | `--all-services` | | `false` | Process all supported services; equivalent to listing every service in `--services`. | -| `--regions` | `-r` | (all opted-in regions) | AWS regions to process (comma-separated or repeated). When empty, cudly enumerates all opted-in AWS regions via EC2 `DescribeRegions`; only if that listing fails does it fall back to discovering regions from Cost Explorer recommendations. Savings Plans are account-level, so with `--regions` empty they are always queried once via `us-east-1`. The `--include-regions` / `--exclude-regions` scoping filters are applied to the fetched recommendations afterwards (see [filtering.md](filtering.md)). | +| `--regions` | `-r` | (all opted-in regions) | AWS regions to process (comma-separated or repeated). When empty, cudly enumerates all opted-in AWS regions via EC2 `DescribeRegions`; only if that listing fails does it fall back to discovering regions from Cost Explorer recommendations. Savings Plans are account-level, so they are always queried once via `us-east-1`; `--regions` then narrows EC2 Instance Savings Plans by their own region, while Compute, SageMaker and Database plans are unaffected. In `--input-csv` mode `--regions` does not narrow Savings Plans (rows are used as supplied). The `--include-regions` / `--exclude-regions` scoping filters are applied to the fetched recommendations afterwards (see [filtering.md](filtering.md)). | ### Coverage and sizing diff --git a/docs/cli/filtering.md b/docs/cli/filtering.md index 4f381b7ad..1cfbe9d3f 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -41,7 +41,7 @@ cudly --services rds --exclude-accounts sandbox --exclude-regions Exclude recommendations for these AWS regions (comma-separated) ``` -When `--regions` is omitted, cudly takes the regions from the Cost Explorer recommendations themselves (not from EC2 `DescribeRegions`), so recommendations in regions that only a linked account has opted into are included. Savings Plans are account-level and are always queried once via `us-east-1`; `--regions` is ignored for them, and `--include-regions` / `--exclude-regions` narrow only EC2 Instance Savings Plans, which are tied to a region; Compute, SageMaker and Database plans are region-agnostic and skip the region filter. `--include-regions` and `--exclude-regions` are applied afterwards as a filter on the fetched recommendations (matching each recommendation's region), so they reduce the results but do not change which regions are queried. If both `--regions` and `--include-regions` are set, the queried regions come from `--regions` and the results are then narrowed by the include/exclude lists. +When `--regions` is omitted, cudly takes the regions from the Cost Explorer recommendations themselves (not from EC2 `DescribeRegions`), so recommendations in regions that only a linked account has opted into are included. Savings Plans are account-level and are always queried once via `us-east-1`; `--regions`, `--include-regions` and `--exclude-regions` narrow only EC2 Instance Savings Plans, which are tied to a region (matched on the plan's own region; one with no known region is dropped when `--regions` is set); Compute, SageMaker and Database plans are region-agnostic and skip the region filters. In `--input-csv` mode `--regions` still does not narrow Savings Plans (rows are used as supplied). `--include-regions` and `--exclude-regions` are applied afterwards as a filter on the fetched recommendations (matching each recommendation's region), so they reduce the results but do not change which regions are queried. If both `--regions` and `--include-regions` are set, the queried regions come from `--regions` and the results are then narrowed by the include/exclude lists. ```bash # Only US and EU recommendations