From c4f7a9d6e457fb402663a59fb20a924f0e86475b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 01:26:48 +0200 Subject: [PATCH 1/2] fix(filters): --regions also narrows EC2 Instance Savings Plan recs Savings Plans are fetched once, so --regions never reached them. Region-scoped plans are now kept only when their own region is listed (unknown region is dropped); Compute, SageMaker and Database plans stay unfiltered. Also applies to --csv-input runs. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 4 ++ cmd/multi_service_filters.go | 13 ++++++- cmd/multi_service_filters_test.go | 63 +++++++++++++++++++++++++++++++ cmd/multi_service_helpers.go | 4 +- docs/cli/README.md | 2 +- docs/cli/filtering.md | 2 +- 6 files changed, 83 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 856759c8a..b387e9d62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ 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. Applies to `--csv-input` runs too. - 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 c66ea10ba..509cb16d2 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -2,6 +2,7 @@ package main import ( "log" + "slices" "strings" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -124,11 +125,21 @@ 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. 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 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..652231222 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -753,3 +753,66 @@ 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) + } + }) +} diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 627932f48..1feadf97f 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..ea8c8fdfd 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. 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 469cd6e67..987673bc9 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -39,7 +39,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. The same applies to `--csv-input` runs. `--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 From a9a567d9e30774bea135f5b0f419c3c879a37b20 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 01:37:40 +0200 Subject: [PATCH 2/2] fix(filters): leave --input-csv Savings Plan rows unfiltered by --regions CSV rows carry no Provider or CommitmentType, so Details.Region is not readable and the new narrowing dropped every Savings Plan row. Gate it off in CSV mode and document that rows are used as supplied. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 3 ++- cmd/multi_service_filters.go | 5 ++++- cmd/multi_service_filters_test.go | 22 ++++++++++++++++++++++ docs/cli/README.md | 2 +- docs/cli/filtering.md | 2 +- 5 files changed, 30 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b387e9d62..a4d9cf771 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - `--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. Applies to `--csv-input` runs too. + 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 509cb16d2..b66679ca3 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -131,12 +131,15 @@ func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool { // 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 } region := awsprovider.EffectiveRegion(*rec) - if common.IsSavingsPlan(rec.Service) && len(cfg.Regions) > 0 && !slices.Contains(cfg.Regions, region) { + if cfg.CSVInput == "" && common.IsSavingsPlan(rec.Service) && len(cfg.Regions) > 0 && !slices.Contains(cfg.Regions, region) { return false } return shouldIncludeRegion(region, cfg) diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index 652231222..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" @@ -816,3 +818,23 @@ func TestApplyFilters_SavingsPlansRegionsFlag(t *testing.T) { } }) } + +// 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/docs/cli/README.md b/docs/cli/README.md index ea8c8fdfd..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 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. 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 987673bc9..2a4312577 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -39,7 +39,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`, `--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. The same applies to `--csv-input` runs. `--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