diff --git a/CHANGELOG.md b/CHANGELOG.md index 388f6888c..5bab5c6b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- Recommendations with no account ID or name are now reported in the drop summary + when an account filter (`--include-accounts` or `--exclude-accounts`) removes + them (#1613). - A purchase result with `Success=true` and a non-nil `Error` is now treated as a failure in the Ctrl-C summary, the audit-stop message, the per-service success/failed counts and the non-zero exit. The CSV `Success` column keeps the diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 0e2baa92c..9a1876bfa 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -68,7 +68,10 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers // operator-scoping choices, not drops worth surfacing in the summary. // --min-pool-size drops are counted separately in applyFilters before // this function runs, so no drop reason is surfaced here. - return *rec, false, "" + // The exception is a recommendation with no account ID or name under an + // account filter: it is not a scoping choice but data the filter + // cannot place, so it is counted. + return *rec, false, unidentifiedAccountDropReason(rec, cfg) } // Apply engine version filters - adjust instance count by subtracting extended support versions. @@ -168,6 +171,42 @@ func shouldIncludeEngine(rec *common.Recommendation, cfg *Config) bool { return filtersFromConfig(cfg).IncludesEngine(rec) } +// DropSummary reasons for a recommendation that carries neither an account ID +// nor an account name and is dropped by an account filter. With both filters +// set the include label is used. +const ( + dropUnidentifiedInclude = "unidentified-account (--include-accounts)" + dropUnidentifiedExclude = "unidentified-account (--exclude-accounts)" +) + +// unidentifiedAccountDropReason returns the drop reason when rec has no account +// ID or name and the account filter alone removed it, else "". +func unidentifiedAccountDropReason(rec *common.Recommendation, cfg *Config) string { + if !isUnidentifiedAccount(rec) || !passesWithoutAccountFilters(rec, cfg) { + return "" + } + if len(cfg.IncludeAccounts) > 0 { + return dropUnidentifiedInclude + } + if len(cfg.ExcludeAccounts) > 0 { + return dropUnidentifiedExclude + } + return "" +} + +func isUnidentifiedAccount(rec *common.Recommendation) bool { + return rec.Account == "" && rec.AccountName == "" +} + +// passesWithoutAccountFilters reports whether rec passes every dimension filter +// except the account ones, so an unidentified-account drop is only counted for +// a recommendation the account filter alone removed. +func passesWithoutAccountFilters(rec *common.Recommendation, cfg *Config) bool { + c := *cfg + c.IncludeAccounts, c.ExcludeAccounts = nil, nil + return passesDimensionFilters(rec, &c) +} + // shouldIncludeAccount checks if an account should be included based on filters. func shouldIncludeAccount(accountName string, cfg *Config) bool { return shouldIncludeAccountIDOrName("", accountName, cfg) diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index a98a52698..84589bc50 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "fmt" "log" "os" @@ -382,7 +383,7 @@ func TestShouldIncludeAccount(t *testing.T) { { name: "In include list", accountID: "123456789012", - includeAccounts: []string{"123456789012", "210987654321"}, + includeAccounts: []string{"123456789012", "111111111111"}, excludeAccounts: []string{}, expected: true, }, @@ -945,3 +946,84 @@ func TestRequireExactExcludes(t *testing.T) { }) } } + +const ( + testUnidentifiedAcct = "123456789012" + testOtherAcct = "999888777666" +) + +func TestApplyFilters_UnidentifiedAccountUnderInclude(t *testing.T) { + unidentified := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", Count: 1} + named := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", Count: 1, Account: testUnidentifiedAcct, AccountName: "prod"} + tests := []struct { + name string + cfg Config + recs []common.Recommendation + wantKept int + wantDrops int + }{ + {"include drops it and counts it", Config{IncludeExtendedSupport: true, IncludeAccounts: []string{"prod"}}, []common.Recommendation{unidentified, named}, 1, 1}, + {"include with other region mismatch is not counted", Config{IncludeExtendedSupport: true, IncludeAccounts: []string{"prod"}, IncludeRegions: []string{"eu-west-1"}}, []common.Recommendation{unidentified}, 0, 0}, + {"no account filter keeps it", Config{IncludeExtendedSupport: true}, []common.Recommendation{unidentified}, 1, 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + drops := common.NewDropSummary() + got := applyFilters(tt.recs, &tt.cfg, nil, nil, "", drops) + assert.Len(t, got, tt.wantKept) + assert.Equal(t, tt.wantDrops, drops.Total()) + if tt.wantDrops > 0 { + assert.Contains(t, drops.FormatOneLine(), "unidentified-account") + } + }) + } +} + +func TestApplyFilters_UnidentifiedAccountUnderExclude(t *testing.T) { + unidentified := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", Count: 1} + named := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", Count: 1, Account: testUnidentifiedAcct, AccountName: "prod"} + other := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", Count: 1, Account: testOtherAcct, AccountName: "dev"} + tests := []struct { + name string + cfg Config + recs []common.Recommendation + wantKept int + wantDrops int + wantReason string + }{ + {"exclude-only drops and counts it, keeps others", Config{IncludeExtendedSupport: true, ExcludeAccounts: []string{testUnidentifiedAcct}}, []common.Recommendation{unidentified, named, other}, 1, 1, "unidentified-account (--exclude-accounts)"}, + {"exclude-only with other region mismatch is not counted", Config{IncludeExtendedSupport: true, ExcludeAccounts: []string{testUnidentifiedAcct}, IncludeRegions: []string{"eu-west-1"}}, []common.Recommendation{unidentified}, 0, 0, ""}, + {"include and exclude count it once under the include reason", Config{IncludeExtendedSupport: true, IncludeAccounts: []string{"dev"}, ExcludeAccounts: []string{testUnidentifiedAcct}}, []common.Recommendation{unidentified, other}, 1, 1, "unidentified-account (--include-accounts)"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + drops := common.NewDropSummary() + got := applyFilters(tt.recs, &tt.cfg, nil, nil, "", drops) + assert.Len(t, got, tt.wantKept) + assert.Equal(t, tt.wantDrops, drops.Total()) + if tt.wantReason != "" { + assert.Contains(t, drops.FormatOneLine(), tt.wantReason) + } + }) + } +} + +func TestCSVExcludeOnly_UnidentifiedRowFilteredOutWithoutError(t *testing.T) { + unidentified := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", Count: 1} + other := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", Count: 1, Account: testOtherAcct, AccountName: "dev"} + cfg := Config{IncludeExtendedSupport: true, ExcludeAccounts: []string{testUnidentifiedAcct}} + recs := []common.Recommendation{unidentified, other} + + assert.NoError(t, requireResolvedAccountNames(recs, &cfg)) + got := applyFilters(recs, &cfg, nil, nil, "", common.NewDropSummary()) + if assert.Len(t, got, 1) { + assert.Equal(t, testOtherAcct, got[0].Account) + } +} + +func TestPopulateRecommendationAccountNames_UnidentifiedRecUnderExcludeOnly(t *testing.T) { + cache := NewAccountAliasCacheWithClient(&MockOrganizationsClient{}) + recs := []common.Recommendation{{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro"}} + + assert.NoError(t, populateRecommendationAccountNames(context.Background(), recs, cache, Config{ExcludeAccounts: []string{testUnidentifiedAcct}})) +} diff --git a/docs/cli/filtering.md b/docs/cli/filtering.md index 19838dc1d..c8c4206a8 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -28,6 +28,8 @@ An `--exclude-accounts` value that matches no account exactly but is part of one When a filter value is a name (anything other than a 12-digit account ID) and an account's name cannot be resolved, cudly stops with an error instead of guessing: carrying on would treat an account you excluded as "not excluded". ID-only filters never need the name. The same rule applies to `--input-csv` replay, where the name comes from the CSV's `AccountName` column; a row whose name column holds the account ID counts as unresolved. +A recommendation with neither an account ID nor an account name (for example a CSV row with empty `Account` and `AccountName`) cannot be placed by an account filter, so it is dropped under either `--include-accounts` or `--exclude-accounts`. On live runs the end-of-run drop summary itemises it as `unidentified-account (--include-accounts)` or `unidentified-account (--exclude-accounts)` (the include label when both filters are set), provided it passed every other filter. On `--input-csv` runs there is no drop summary, so such rows are counted only in the "filtered out N" line. + ```bash # Only process two accounts cudly --services rds --include-accounts prod-account,staging-account