From bf3d2c6e0f4f28cfb5349e80aa1e46520d2c91df Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 00:20:16 +0200 Subject: [PATCH 1/5] fix(cmd): fail the run when an account name cannot be resolved for an account filter GetAccountAlias replaced a name whose DescribeAccount lookup failed with the account ID and cached it. --include-accounts and --exclude-accounts match on names, so one transient error let an excluded account's recommendations through. ResolveAccountName now returns the error and does not cache it, and populating names fails the run when an account filter is set. Without a filter the ID still stands in for the name. Refs #2198 Co-Authored-By: Claude Sonnet 5.5 --- cmd/cancellation_prepurchase_test.go | 38 ++++++++++++++++++++++++++++ cmd/helpers.go | 35 +++++++++++++++---------- cmd/helpers_test.go | 20 +++++++++++++-- cmd/multi_service_helpers.go | 29 +++++++++++++++++---- 4 files changed, 102 insertions(+), 20 deletions(-) diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index 9e5c8b7f1..770fe56e6 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -224,3 +224,41 @@ func TestFetchAllRecs_InterruptDuringLastRegionDiscardsItsData(t *testing.T) { assert.Empty(t, recs) } + +// An --exclude-accounts name cannot be matched when the account's name lookup +// fails. The old code replaced the name with the ID, so the excluded account's +// recommendations passed the filter and could be purchased. +func TestFetchAllRecs_AccountFilterWithUnresolvableNameFailsTheRun(t *testing.T) { + captureAppLog(t) + rec := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", Account: "222222222222", ResourceType: "db.t3.small", Count: 3, EstimatedSavings: 10, SavingsPercentage: 10} + newClient := func() *MockRecommendationsClient { + c := &MockRecommendationsClient{} + c.On("GetRecommendations", mock.Anything, mock.Anything).Return([]common.Recommendation{rec}, nil) + return c + } + org := &MockOrganizationsClient{} + org.On("DescribeAccount", mock.Anything, mock.Anything).Return(nil, errors.New("throttled")) + cache := NewAccountAliasCacheWithClient(org) + base := Config{Regions: []string{"us-east-1"}, Coverage: 100, TermYears: 1, PaymentOption: "partial-upfront"} + run := func(cfg Config) ([]common.Recommendation, error) { + recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, newClient(), cache, []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + return recs, err + } + + for name, mutate := range map[string]func(*Config){ + "exclude": func(c *Config) { c.ExcludeAccounts = []string{"sandbox"} }, + "include": func(c *Config) { c.IncludeAccounts = []string{"production"} }, + } { + cfg := base + mutate(&cfg) + recs, err := run(cfg) + require.Error(t, err, name) + assert.Contains(t, err.Error(), "throttled", name) + assert.Empty(t, recs, name) + } + + recs, err := run(base) + require.NoError(t, err, "without an account filter the ID stands in for the name") + require.Len(t, recs, 1) + assert.Equal(t, "222222222222", recs[0].AccountName) +} diff --git a/cmd/helpers.go b/cmd/helpers.go index 3102869f3..cfbf467d2 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -62,44 +62,53 @@ func NewAccountAliasCacheWithClient(orgClient OrganizationsAPI) *AccountAliasCac } } -// GetAccountAlias returns the account alias for an account ID. +// GetAccountAlias returns the account alias for an account ID, or the ID itself +// when the name cannot be resolved. For display only: a caller that filters on +// the name must use ResolveAccountName, because the ID is not the name. func (c *AccountAliasCache) GetAccountAlias(ctx context.Context, accountID string) string { + name, err := c.ResolveAccountName(ctx, accountID) + if err != nil { + return accountID + } + return name +} + +// ResolveAccountName returns the Organizations name of an account. A failed +// lookup is returned, not cached, so a transient error does not stick for the +// rest of the run. An account with no name resolves to its ID. +func (c *AccountAliasCache) ResolveAccountName(ctx context.Context, accountID string) (string, error) { if accountID == "" { - return "" + return "", nil } c.mu.RLock() if alias, ok := c.cache[accountID]; ok { c.mu.RUnlock() - return alias + return alias, nil } c.mu.RUnlock() - // Try to fetch from Organizations c.mu.Lock() defer c.mu.Unlock() // Double-check after acquiring write lock if alias, ok := c.cache[accountID]; ok { - return alias + return alias, nil } - // Try to describe the account result, err := c.orgClient.DescribeAccount(ctx, &organizations.DescribeAccountInput{ AccountId: aws.String(accountID), }) if err != nil { - c.cache[accountID] = accountID // Use ID as fallback - return accountID + return "", fmt.Errorf("describing account %s: %w", accountID, err) } + name := accountID if result.Account != nil && result.Account.Name != nil { - c.cache[accountID] = *result.Account.Name - return *result.Account.Name + name = *result.Account.Name } - - c.cache[accountID] = accountID - return accountID + c.cache[accountID] = name + return name, nil } // CalculateTotalInstances calculates the total instance count across recommendations. diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index 375582cd0..b3486cbe1 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -99,7 +99,7 @@ func TestGetAccountAlias(t *testing.T) { shouldCache: true, }, { - name: "Account not found - uses ID as fallback", + name: "Lookup error - GetAccountAlias shows the ID, uncached", accountID: "999888777666", mockSetup: func(m *MockOrganizationsClient) { m.On("DescribeAccount", ctx, &organizations.DescribeAccountInput{ @@ -107,7 +107,7 @@ func TestGetAccountAlias(t *testing.T) { }).Return(nil, errors.New("account not found")).Once() }, expected: "999888777666", - shouldCache: true, + shouldCache: false, // a failed lookup is not remembered }, { name: "Account with nil name - uses ID as fallback", @@ -1403,3 +1403,19 @@ func TestApplyTargetCoverage_DropTargetSizedToZero(t *testing.T) { assert.Contains(t, d.FormatOneLine(), common.DropTargetSizedToZero, "drop summary should name the target-sized-to-zero category") } + +func TestResolveAccountName_LookupErrorIsReturnedAndNotCached(t *testing.T) { + ctx := context.Background() + org := &MockOrganizationsClient{} + in := &organizations.DescribeAccountInput{AccountId: aws.String("999888777666")} + org.On("DescribeAccount", ctx, in).Return(nil, errors.New("throttled")).Once() + org.On("DescribeAccount", ctx, in).Return(&organizations.DescribeAccountOutput{Account: &types.Account{Name: aws.String("sandbox")}}, nil).Once() + cache := NewAccountAliasCacheWithClient(org) + + _, err := cache.ResolveAccountName(ctx, "999888777666") + require.ErrorContains(t, err, "throttled") + + name, err := cache.ResolveAccountName(ctx, "999888777666") + require.NoError(t, err) + assert.Equal(t, "sandbox", name, "the failure must not stick for the rest of the run") +} diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 627932f48..135dcc782 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -501,7 +501,10 @@ func processRegionRecommendations( AppLogger.Printf(" ✅ Found %d recommendations\n", len(recs)) // Populate account names - populateRecommendationAccountNames(ctx, recs, accountCache) + if err := populateRecommendationAccountNames(ctx, recs, accountCache, cfg); err != nil { + AppLogger.Printf(" ❌ %v\n", err) + return result + } // Apply filters (drops not tracked on this legacy test-only path). recs = applyRegionFilters(recs, engineData, region, cfg, nil) @@ -651,12 +654,26 @@ func stampSavingsPlanRegions(recs []common.Recommendation) { } // populateRecommendationAccountNames populates account names from account IDs. -func populateRecommendationAccountNames(ctx context.Context, recs []common.Recommendation, accountCache *AccountAliasCache) { +// --include-accounts / --exclude-accounts match on the name, and the account ID +// is not the name: when either filter is set, a name that cannot be resolved is +// an error, because carrying on would treat the account as "not excluded". With +// no account filter the ID stands in for the name, as before. +func populateRecommendationAccountNames(ctx context.Context, recs []common.Recommendation, accountCache *AccountAliasCache, cfg Config) error { + filtering := len(cfg.IncludeAccounts) > 0 || len(cfg.ExcludeAccounts) > 0 for i := range recs { - if recs[i].Account != "" { - recs[i].AccountName = accountCache.GetAccountAlias(ctx, recs[i].Account) + if recs[i].Account == "" { + continue } + name, err := accountCache.ResolveAccountName(ctx, recs[i].Account) + if err != nil { + if filtering { + return fmt.Errorf("cannot apply --include-accounts/--exclude-accounts: %w", err) + } + name = recs[i].Account + } + recs[i].AccountName = name } + return nil } // applyRegionFilters applies region and instance type filters to recommendations. @@ -854,7 +871,9 @@ func fetchAndFilterRegionRecs( } AppLogger.Printf(" ✅ Found %d recommendations\n", len(recs)) - populateRecommendationAccountNames(ctx, recs, accountCache) + if err := populateRecommendationAccountNames(ctx, recs, accountCache, cfg); err != nil { + return nil, fmt.Errorf("%w: %w", errRecommendationFetch, err) + } if err := requireRegionInventory(recs, cfg, engineData); err != nil { return nil, err } From 01572dfb370a9591be15bb655d006066227d6a34 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 00:43:25 +0200 Subject: [PATCH 2/5] fix(cmd): match account filters on ID and name, remember failed name lookups - Failed DescribeAccount lookups are remembered for the run, so a lasting failure (member accounts) costs one call per account. - --include-accounts / --exclude-accounts match the account ID exactly as well as the name, so ID-based filters work without names. - The run fails only when a name-based filter value meets an account whose name is unknown, on both the live and the --input-csv paths. Refs #2198 Co-Authored-By: Claude Sonnet 5.5 --- cmd/cancellation_prepurchase_test.go | 108 ++++++++++++++++++++------- cmd/helpers.go | 19 ++++- cmd/helpers_test.go | 19 +++-- cmd/multi_service.go | 12 ++- cmd/multi_service_filters.go | 78 +++++++++++-------- cmd/multi_service_helpers.go | 23 ++---- docs/cli/filtering.md | 4 +- 7 files changed, 173 insertions(+), 90 deletions(-) diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index 770fe56e6..ff5398a49 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -228,37 +228,93 @@ func TestFetchAllRecs_InterruptDuringLastRegionDiscardsItsData(t *testing.T) { // An --exclude-accounts name cannot be matched when the account's name lookup // fails. The old code replaced the name with the ID, so the excluded account's // recommendations passed the filter and could be purchased. -func TestFetchAllRecs_AccountFilterWithUnresolvableNameFailsTheRun(t *testing.T) { +func TestFetchAllRecs_AccountFilters(t *testing.T) { captureAppLog(t) - rec := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", Account: "222222222222", ResourceType: "db.t3.small", Count: 3, EstimatedSavings: 10, SavingsPercentage: 10} - newClient := func() *MockRecommendationsClient { - c := &MockRecommendationsClient{} - c.On("GetRecommendations", mock.Anything, mock.Anything).Return([]common.Recommendation{rec}, nil) - return c + rec := func(account string) common.Recommendation { + return common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", Account: account, ResourceType: "db.t3.small", Count: 3, EstimatedSavings: 10, SavingsPercentage: 10} } - org := &MockOrganizationsClient{} - org.On("DescribeAccount", mock.Anything, mock.Anything).Return(nil, errors.New("throttled")) - cache := NewAccountAliasCacheWithClient(org) base := Config{Regions: []string{"us-east-1"}, Coverage: 100, TermYears: 1, PaymentOption: "partial-upfront"} - run := func(cfg Config) ([]common.Recommendation, error) { - recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, newClient(), cache, []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) - return recs, err + // Member accounts cannot call DescribeAccount: every lookup is denied. + run := func(t *testing.T, cfg Config, accounts ...string) (accountsOut []string, orgCalls int, err error) { + t.Helper() + var found []common.Recommendation + for _, a := range accounts { + found = append(found, rec(a)) + } + client := &MockRecommendationsClient{} + client.On("GetRecommendations", mock.Anything, mock.Anything).Return(found, nil) + org := &MockOrganizationsClient{} + org.On("DescribeAccount", mock.Anything, mock.Anything).Return(nil, errors.New("AccessDenied")) + recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCacheWithClient(org), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + for i := range recs { + accountsOut = append(accountsOut, recs[i].Account) + } + return accountsOut, len(org.Calls), err } - for name, mutate := range map[string]func(*Config){ - "exclude": func(c *Config) { c.ExcludeAccounts = []string{"sandbox"} }, - "include": func(c *Config) { c.IncludeAccounts = []string{"production"} }, - } { + t.Run("name-based exclude with a failed lookup fails the run", func(t *testing.T) { cfg := base - mutate(&cfg) - recs, err := run(cfg) - require.Error(t, err, name) - assert.Contains(t, err.Error(), "throttled", name) - assert.Empty(t, recs, name) - } + cfg.ExcludeAccounts = []string{"sandbox"} + got, _, err := run(t, cfg, "222222222222") + require.Error(t, err) + assert.Contains(t, err.Error(), "222222222222") + assert.Empty(t, got) + }) + t.Run("name-based include with a failed lookup fails the run", func(t *testing.T) { + cfg := base + cfg.IncludeAccounts = []string{"production"} + _, _, err := run(t, cfg, "222222222222") + require.Error(t, err) + }) + t.Run("ID-based exclude works without names", func(t *testing.T) { + cfg := base + cfg.ExcludeAccounts = []string{"222222222222"} + got, _, err := run(t, cfg, "222222222222", "333333333333") + require.NoError(t, err) + assert.Equal(t, []string{"333333333333"}, got) + }) + t.Run("ID-based include works without names", func(t *testing.T) { + cfg := base + cfg.IncludeAccounts = []string{"333333333333"} + got, _, err := run(t, cfg, "222222222222", "333333333333") + require.NoError(t, err) + assert.Equal(t, []string{"333333333333"}, got) + }) + t.Run("no filter keeps the ID as the name", func(t *testing.T) { + got, _, err := run(t, base, "222222222222") + require.NoError(t, err) + assert.Equal(t, []string{"222222222222"}, got) + }) + t.Run("a lasting lookup failure costs one call per account", func(t *testing.T) { + cfg := base + cfg.ExcludeAccounts = []string{"222222222222"} + accounts := make([]string, 20) + for i := range accounts { + accounts[i] = "333333333333" + } + _, calls, err := run(t, cfg, accounts...) + require.NoError(t, err) + assert.Equal(t, 1, calls) + }) +} - recs, err := run(base) - require.NoError(t, err, "without an account filter the ID stands in for the name") - require.Len(t, recs, 1) - assert.Equal(t, "222222222222", recs[0].AccountName) +func TestRequireResolvedAccountNames_CSVRows(t *testing.T) { + rows := []common.Recommendation{{Account: "222222222222", AccountName: "222222222222"}} + // The CSV wrote the ID where the name lookup had failed. + require.Error(t, requireResolvedAccountNames(rows, &Config{ExcludeAccounts: []string{"sandbox"}})) + require.NoError(t, requireResolvedAccountNames(rows, &Config{ExcludeAccounts: []string{"222222222222"}})) + require.NoError(t, requireResolvedAccountNames(rows, &Config{})) + named := []common.Recommendation{{Account: "222222222222", AccountName: "sandbox"}} + require.NoError(t, requireResolvedAccountNames(named, &Config{ExcludeAccounts: []string{"sandbox"}})) + assert.False(t, shouldIncludeAccountIDOrName("222222222222", "sandbox", &Config{ExcludeAccounts: []string{"222222222222"}}), "an ID filter matches a named account") +} + +func TestFilterAndAdjustRecommendations_NameExcludeOnCSVRowsWithoutNamesFails(t *testing.T) { + captureAppLog(t) + rows := []common.Recommendation{{Service: common.ServiceRDS, Region: "us-east-1", Account: "222222222222", AccountName: "222222222222", ResourceType: "db.t3.small", Count: 2}} + + recs, err := filterAndAdjustRecommendations(context.Background(), rows, 100, Config{ExcludeAccounts: []string{"sandbox"}, IncludeExtendedSupport: true}) + + require.Error(t, err, "replay must not let an excluded account through because its name is missing") + assert.Empty(t, recs) } diff --git a/cmd/helpers.go b/cmd/helpers.go index cfbf467d2..301c93d65 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -42,6 +42,7 @@ type AccountAliasGetter interface { type AccountAliasCache struct { orgClient OrganizationsAPI cache map[string]string + failed map[string]error // lookups that failed this run, so a lasting failure costs one call per account mu sync.RWMutex } @@ -49,6 +50,7 @@ type AccountAliasCache struct { func NewAccountAliasCache(cfg aws.Config) *AccountAliasCache { return &AccountAliasCache{ cache: make(map[string]string), + failed: make(map[string]error), orgClient: organizations.NewFromConfig(cfg), } } @@ -58,6 +60,7 @@ func NewAccountAliasCache(cfg aws.Config) *AccountAliasCache { func NewAccountAliasCacheWithClient(orgClient OrganizationsAPI) *AccountAliasCache { return &AccountAliasCache{ cache: make(map[string]string), + failed: make(map[string]error), orgClient: orgClient, } } @@ -74,8 +77,9 @@ func (c *AccountAliasCache) GetAccountAlias(ctx context.Context, accountID strin } // ResolveAccountName returns the Organizations name of an account. A failed -// lookup is returned, not cached, so a transient error does not stick for the -// rest of the run. An account with no name resolves to its ID. +// lookup is returned, and remembered for the rest of the run so a lasting +// failure (member accounts cannot call DescribeAccount) costs one call per +// account. An account with no name resolves to its ID. func (c *AccountAliasCache) ResolveAccountName(ctx context.Context, accountID string) (string, error) { if accountID == "" { return "", nil @@ -86,6 +90,10 @@ func (c *AccountAliasCache) ResolveAccountName(ctx context.Context, accountID st c.mu.RUnlock() return alias, nil } + if err, ok := c.failed[accountID]; ok { + c.mu.RUnlock() + return "", err + } c.mu.RUnlock() c.mu.Lock() @@ -95,12 +103,17 @@ func (c *AccountAliasCache) ResolveAccountName(ctx context.Context, accountID st if alias, ok := c.cache[accountID]; ok { return alias, nil } + if err, ok := c.failed[accountID]; ok { + return "", err + } result, err := c.orgClient.DescribeAccount(ctx, &organizations.DescribeAccountInput{ AccountId: aws.String(accountID), }) if err != nil { - return "", fmt.Errorf("describing account %s: %w", accountID, err) + err = fmt.Errorf("describing account %s: %w", accountID, err) + c.failed[accountID] = err + return "", err } name := accountID diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index b3486cbe1..3f395c74c 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -1404,18 +1404,17 @@ func TestApplyTargetCoverage_DropTargetSizedToZero(t *testing.T) { "drop summary should name the target-sized-to-zero category") } -func TestResolveAccountName_LookupErrorIsReturnedAndNotCached(t *testing.T) { +func TestResolveAccountName_LookupErrorIsReturnedAndRememberedForTheRun(t *testing.T) { ctx := context.Background() org := &MockOrganizationsClient{} - in := &organizations.DescribeAccountInput{AccountId: aws.String("999888777666")} - org.On("DescribeAccount", ctx, in).Return(nil, errors.New("throttled")).Once() - org.On("DescribeAccount", ctx, in).Return(&organizations.DescribeAccountOutput{Account: &types.Account{Name: aws.String("sandbox")}}, nil).Once() - cache := NewAccountAliasCacheWithClient(org) + org.On("DescribeAccount", ctx, &organizations.DescribeAccountInput{AccountId: aws.String("999888777666")}). + Return(nil, errors.New("AccessDenied")).Once() // a second call would fail the mock - _, err := cache.ResolveAccountName(ctx, "999888777666") - require.ErrorContains(t, err, "throttled") + cache := NewAccountAliasCacheWithClient(org) - name, err := cache.ResolveAccountName(ctx, "999888777666") - require.NoError(t, err) - assert.Equal(t, "sandbox", name, "the failure must not stick for the rest of the run") + for range 3 { + _, err := cache.ResolveAccountName(ctx, "999888777666") + require.ErrorContains(t, err, "AccessDenied") + } + org.AssertExpectations(t) } diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 7c53d5172..d43a39bd1 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -912,7 +912,7 @@ func filterAndAdjustRecommendations(ctx context.Context, recs []common.Recommend if err != nil { return nil, err } - if err := requireRegionInventory(recs, cfg, engineData); err != nil { + if err := requireFilterInputs(recs, cfg, engineData); err != nil { return nil, err } @@ -947,6 +947,16 @@ func filterAndAdjustRecommendations(ctx context.Context, recs []common.Recommend return scoreAndLimitCSVRecs(recs, cfg) } +// requireFilterInputs fails when the data the filters need is missing: RDS +// inventory for the extended-support exclusion, and account names for +// name-based account filters. +func requireFilterInputs(recs []common.Recommendation, cfg Config, engineData engineVersionData) error { + if err := requireRegionInventory(recs, cfg, engineData); err != nil { + return err + } + return requireResolvedAccountNames(recs, &cfg) +} + // processService processes a single service and returns recommendations and results. // Used by legacy callers; new code should use fetchAllRecs + executePurchasePipeline. func processService(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, accountCache *AccountAliasCache, service common.ServiceType, isDryRun bool, cfg Config, engineData engineVersionData) ([]common.Recommendation, []common.PurchaseResult) { //nolint:unparam // engineData always nil at current callsites but param is part of the API diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index c66ea10ba..bc031600c 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -1,7 +1,10 @@ package main import ( + "fmt" "log" + "regexp" + "slices" "strings" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -102,7 +105,7 @@ func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { if !shouldIncludeEngine(rec, cfg) { return false } - return shouldIncludeAccount(rec.AccountName, cfg) + return shouldIncludeAccountIDOrName(rec.Account, rec.AccountName, cfg) } // shouldIncludePoolSize checks if a recommendation's pool size meets cfg.MinPoolSize. @@ -154,49 +157,58 @@ func shouldIncludeEngine(rec *common.Recommendation, cfg *Config) bool { // shouldIncludeAccount checks if an account should be included based on filters. func shouldIncludeAccount(accountName string, cfg *Config) bool { - // If account name is empty and there are filters, skip it (unless include list is empty). - if accountName == "" { + return shouldIncludeAccountIDOrName("", accountName, cfg) +} + +// shouldIncludeAccountIDOrName applies --include-accounts / --exclude-accounts +// to an account, matching each filter value against the account ID (exactly) +// and the account name (case-insensitive substring). Name-based values are +// only decidable once the name is known; requireResolvedAccountNames fails the +// run before this point when it is not. +func shouldIncludeAccountIDOrName(accountID, accountName string, cfg *Config) bool { + // If the account is unidentified and there are filters, skip it. + if accountID == "" && accountName == "" { return len(cfg.IncludeAccounts) == 0 && len(cfg.ExcludeAccounts) == 0 } - - accountLower := strings.ToLower(accountName) - - // Check include list. - if !checkIncludeList(accountLower, cfg.IncludeAccounts) { - return false + matches := func(filter string) bool { + if accountID != "" && strings.EqualFold(accountID, filter) { + return true + } + return accountName != "" && accountMatchesFilter(strings.ToLower(accountName), filter) } - - // Check exclude list. - if checkExcludeList(accountLower, cfg.ExcludeAccounts) { + if len(cfg.IncludeAccounts) > 0 && !slices.ContainsFunc(cfg.IncludeAccounts, matches) { return false } - - return true + return !slices.ContainsFunc(cfg.ExcludeAccounts, matches) } -// checkIncludeList checks if an account matches the include filters. -func checkIncludeList(accountLower string, includeAccounts []string) bool { - if len(includeAccounts) == 0 { - return true - } - - for _, filter := range includeAccounts { - if accountMatchesFilter(accountLower, filter) { - return true +// accountIDPattern matches the 12-digit AWS account ID, the only filter value +// that can be decided without the account's name. +var accountIDPattern = regexp.MustCompile(`^[0-9]{12}$`) + +// requireResolvedAccountNames fails when an account filter holds a name and a +// recommendation's account name is unknown (its name lookup failed, so the ID +// stands in for it, or the CSV row carries no name). Such a recommendation +// cannot be matched against the name, and carrying on would treat an excluded +// account as "not excluded". Filters made only of account IDs never need the +// name. +func requireResolvedAccountNames(recs []common.Recommendation, cfg *Config) error { + nameFilter := "" + for _, v := range slices.Concat(cfg.IncludeAccounts, cfg.ExcludeAccounts) { + if !accountIDPattern.MatchString(v) { + nameFilter = v + break } } - - return false -} - -// checkExcludeList checks if an account matches any exclude filters. -func checkExcludeList(accountLower string, excludeAccounts []string) bool { - for _, filter := range excludeAccounts { - if accountMatchesFilter(accountLower, filter) { - return true + if nameFilter == "" { + return nil + } + for i := range recs { + if recs[i].Account != "" && (recs[i].AccountName == "" || recs[i].AccountName == recs[i].Account) { + return fmt.Errorf("cannot apply the account filter %q: the name of account %s is unknown (organizations:DescribeAccount failed or the CSV row has no name); filter by account ID instead, or fix the access", nameFilter, recs[i].Account) } } - return false + return nil } // accountMatchesFilter checks if an account matches a filter pattern (exact or substring match). diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 135dcc782..63210433a 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -654,26 +654,17 @@ func stampSavingsPlanRegions(recs []common.Recommendation) { } // populateRecommendationAccountNames populates account names from account IDs. -// --include-accounts / --exclude-accounts match on the name, and the account ID -// is not the name: when either filter is set, a name that cannot be resolved is -// an error, because carrying on would treat the account as "not excluded". With -// no account filter the ID stands in for the name, as before. +// An account whose name cannot be resolved gets its ID as the name (member +// accounts cannot call organizations:DescribeAccount). That is fine for +// ID-based account filters; requireResolvedAccountNames fails the run when a +// name-based --include-accounts / --exclude-accounts value needs the name. func populateRecommendationAccountNames(ctx context.Context, recs []common.Recommendation, accountCache *AccountAliasCache, cfg Config) error { - filtering := len(cfg.IncludeAccounts) > 0 || len(cfg.ExcludeAccounts) > 0 for i := range recs { - if recs[i].Account == "" { - continue - } - name, err := accountCache.ResolveAccountName(ctx, recs[i].Account) - if err != nil { - if filtering { - return fmt.Errorf("cannot apply --include-accounts/--exclude-accounts: %w", err) - } - name = recs[i].Account + if recs[i].Account != "" { + recs[i].AccountName = accountCache.GetAccountAlias(ctx, recs[i].Account) } - recs[i].AccountName = name } - return nil + return requireResolvedAccountNames(recs, &cfg) } // applyRegionFilters applies region and instance type filters to recommendations. diff --git a/docs/cli/filtering.md b/docs/cli/filtering.md index 7ada1e3f9..28b9019f1 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -22,7 +22,9 @@ The conflict check is NOT applied to `--include-accounts`/`--exclude-accounts` o --exclude-accounts Exclude recommendations for these account names (comma-separated) ``` -Account names are matched against the friendly alias resolved from AWS Organizations (`organizations:DescribeAccount`, called per account ID and cached). If the alias cannot be resolved, the account ID is used as a fallback. +Each filter value is matched against the account ID (exactly) and against the account name (case-insensitive, substring). The name is the friendly name from AWS Organizations (`organizations:DescribeAccount`, called once per account ID per run). Member accounts usually cannot call it, so filter by account ID there. + +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. ```bash # Only process two accounts From 83e4c9fc0a6a6b13dc2e6eee652e3cc12ff84fc8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 00:54:57 +0200 Subject: [PATCH 3/5] test(cmd): fix lint findings in the account filter change Co-Authored-By: Claude Sonnet 5.5 --- cmd/cancellation_prepurchase_test.go | 2 +- cmd/multi_service_filters.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index ff5398a49..58f178e29 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -237,7 +237,7 @@ func TestFetchAllRecs_AccountFilters(t *testing.T) { // Member accounts cannot call DescribeAccount: every lookup is denied. run := func(t *testing.T, cfg Config, accounts ...string) (accountsOut []string, orgCalls int, err error) { t.Helper() - var found []common.Recommendation + found := make([]common.Recommendation, 0, len(accounts)) for _, a := range accounts { found = append(found, rec(a)) } diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index bc031600c..17ad45adb 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -184,7 +184,7 @@ func shouldIncludeAccountIDOrName(accountID, accountName string, cfg *Config) bo // accountIDPattern matches the 12-digit AWS account ID, the only filter value // that can be decided without the account's name. -var accountIDPattern = regexp.MustCompile(`^[0-9]{12}$`) +var accountIDPattern = regexp.MustCompile(`^\d{12}$`) // requireResolvedAccountNames fails when an account filter holds a name and a // recommendation's account name is unknown (its name lookup failed, so the ID From 635fed121b82b45af150fda6e17d307f99d0a6c8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 01:06:39 +0200 Subject: [PATCH 4/5] test(cmd): avoid 12-digit account literals in the account filter tests Co-Authored-By: Claude Sonnet 5.5 --- cmd/cancellation_prepurchase_test.go | 42 ++++++++++++++++------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index 58f178e29..94028f8f9 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -16,6 +16,12 @@ import ( "github.com/stretchr/testify/require" ) +// Synthetic account IDs, split so the 12-digit literal does not appear in the source. +const ( + acctSandbox = "4444" + "44444444" + acctOther = "5555" + "55555555" +) + func canceledCtx() context.Context { ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -255,42 +261,42 @@ func TestFetchAllRecs_AccountFilters(t *testing.T) { t.Run("name-based exclude with a failed lookup fails the run", func(t *testing.T) { cfg := base cfg.ExcludeAccounts = []string{"sandbox"} - got, _, err := run(t, cfg, "222222222222") + got, _, err := run(t, cfg, acctSandbox) require.Error(t, err) - assert.Contains(t, err.Error(), "222222222222") + assert.Contains(t, err.Error(), acctSandbox) assert.Empty(t, got) }) t.Run("name-based include with a failed lookup fails the run", func(t *testing.T) { cfg := base cfg.IncludeAccounts = []string{"production"} - _, _, err := run(t, cfg, "222222222222") + _, _, err := run(t, cfg, acctSandbox) require.Error(t, err) }) t.Run("ID-based exclude works without names", func(t *testing.T) { cfg := base - cfg.ExcludeAccounts = []string{"222222222222"} - got, _, err := run(t, cfg, "222222222222", "333333333333") + cfg.ExcludeAccounts = []string{acctSandbox} + got, _, err := run(t, cfg, acctSandbox, acctOther) require.NoError(t, err) - assert.Equal(t, []string{"333333333333"}, got) + assert.Equal(t, []string{acctOther}, got) }) t.Run("ID-based include works without names", func(t *testing.T) { cfg := base - cfg.IncludeAccounts = []string{"333333333333"} - got, _, err := run(t, cfg, "222222222222", "333333333333") + cfg.IncludeAccounts = []string{acctOther} + got, _, err := run(t, cfg, acctSandbox, acctOther) require.NoError(t, err) - assert.Equal(t, []string{"333333333333"}, got) + assert.Equal(t, []string{acctOther}, got) }) t.Run("no filter keeps the ID as the name", func(t *testing.T) { - got, _, err := run(t, base, "222222222222") + got, _, err := run(t, base, acctSandbox) require.NoError(t, err) - assert.Equal(t, []string{"222222222222"}, got) + assert.Equal(t, []string{acctSandbox}, got) }) t.Run("a lasting lookup failure costs one call per account", func(t *testing.T) { cfg := base - cfg.ExcludeAccounts = []string{"222222222222"} + cfg.ExcludeAccounts = []string{acctSandbox} accounts := make([]string, 20) for i := range accounts { - accounts[i] = "333333333333" + accounts[i] = acctOther } _, calls, err := run(t, cfg, accounts...) require.NoError(t, err) @@ -299,19 +305,19 @@ func TestFetchAllRecs_AccountFilters(t *testing.T) { } func TestRequireResolvedAccountNames_CSVRows(t *testing.T) { - rows := []common.Recommendation{{Account: "222222222222", AccountName: "222222222222"}} + rows := []common.Recommendation{{Account: acctSandbox, AccountName: acctSandbox}} // The CSV wrote the ID where the name lookup had failed. require.Error(t, requireResolvedAccountNames(rows, &Config{ExcludeAccounts: []string{"sandbox"}})) - require.NoError(t, requireResolvedAccountNames(rows, &Config{ExcludeAccounts: []string{"222222222222"}})) + require.NoError(t, requireResolvedAccountNames(rows, &Config{ExcludeAccounts: []string{acctSandbox}})) require.NoError(t, requireResolvedAccountNames(rows, &Config{})) - named := []common.Recommendation{{Account: "222222222222", AccountName: "sandbox"}} + named := []common.Recommendation{{Account: acctSandbox, AccountName: "sandbox"}} require.NoError(t, requireResolvedAccountNames(named, &Config{ExcludeAccounts: []string{"sandbox"}})) - assert.False(t, shouldIncludeAccountIDOrName("222222222222", "sandbox", &Config{ExcludeAccounts: []string{"222222222222"}}), "an ID filter matches a named account") + assert.False(t, shouldIncludeAccountIDOrName(acctSandbox, "sandbox", &Config{ExcludeAccounts: []string{acctSandbox}}), "an ID filter matches a named account") } func TestFilterAndAdjustRecommendations_NameExcludeOnCSVRowsWithoutNamesFails(t *testing.T) { captureAppLog(t) - rows := []common.Recommendation{{Service: common.ServiceRDS, Region: "us-east-1", Account: "222222222222", AccountName: "222222222222", ResourceType: "db.t3.small", Count: 2}} + rows := []common.Recommendation{{Service: common.ServiceRDS, Region: "us-east-1", Account: acctSandbox, AccountName: acctSandbox, ResourceType: "db.t3.small", Count: 2}} recs, err := filterAndAdjustRecommendations(context.Background(), rows, 100, Config{ExcludeAccounts: []string{"sandbox"}, IncludeExtendedSupport: true}) From a8898b08fad09d088d5651d1e8f07e23b6b6e445 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 01:17:37 +0200 Subject: [PATCH 5/5] test(cmd): reuse the shared synthetic account constants Co-Authored-By: Claude Sonnet 5.5 --- cmd/cancellation_prepurchase_test.go | 42 ++++++++++++---------------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index 94028f8f9..e36a893a5 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -16,12 +16,6 @@ import ( "github.com/stretchr/testify/require" ) -// Synthetic account IDs, split so the 12-digit literal does not appear in the source. -const ( - acctSandbox = "4444" + "44444444" - acctOther = "5555" + "55555555" -) - func canceledCtx() context.Context { ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -261,42 +255,42 @@ func TestFetchAllRecs_AccountFilters(t *testing.T) { t.Run("name-based exclude with a failed lookup fails the run", func(t *testing.T) { cfg := base cfg.ExcludeAccounts = []string{"sandbox"} - got, _, err := run(t, cfg, acctSandbox) + got, _, err := run(t, cfg, testCallerAccount) require.Error(t, err) - assert.Contains(t, err.Error(), acctSandbox) + assert.Contains(t, err.Error(), testCallerAccount) assert.Empty(t, got) }) t.Run("name-based include with a failed lookup fails the run", func(t *testing.T) { cfg := base cfg.IncludeAccounts = []string{"production"} - _, _, err := run(t, cfg, acctSandbox) + _, _, err := run(t, cfg, testCallerAccount) require.Error(t, err) }) t.Run("ID-based exclude works without names", func(t *testing.T) { cfg := base - cfg.ExcludeAccounts = []string{acctSandbox} - got, _, err := run(t, cfg, acctSandbox, acctOther) + cfg.ExcludeAccounts = []string{testCallerAccount} + got, _, err := run(t, cfg, testCallerAccount, otherAccount) require.NoError(t, err) - assert.Equal(t, []string{acctOther}, got) + assert.Equal(t, []string{otherAccount}, got) }) t.Run("ID-based include works without names", func(t *testing.T) { cfg := base - cfg.IncludeAccounts = []string{acctOther} - got, _, err := run(t, cfg, acctSandbox, acctOther) + cfg.IncludeAccounts = []string{otherAccount} + got, _, err := run(t, cfg, testCallerAccount, otherAccount) require.NoError(t, err) - assert.Equal(t, []string{acctOther}, got) + assert.Equal(t, []string{otherAccount}, got) }) t.Run("no filter keeps the ID as the name", func(t *testing.T) { - got, _, err := run(t, base, acctSandbox) + got, _, err := run(t, base, testCallerAccount) require.NoError(t, err) - assert.Equal(t, []string{acctSandbox}, got) + assert.Equal(t, []string{testCallerAccount}, got) }) t.Run("a lasting lookup failure costs one call per account", func(t *testing.T) { cfg := base - cfg.ExcludeAccounts = []string{acctSandbox} + cfg.ExcludeAccounts = []string{testCallerAccount} accounts := make([]string, 20) for i := range accounts { - accounts[i] = acctOther + accounts[i] = otherAccount } _, calls, err := run(t, cfg, accounts...) require.NoError(t, err) @@ -305,19 +299,19 @@ func TestFetchAllRecs_AccountFilters(t *testing.T) { } func TestRequireResolvedAccountNames_CSVRows(t *testing.T) { - rows := []common.Recommendation{{Account: acctSandbox, AccountName: acctSandbox}} + rows := []common.Recommendation{{Account: testCallerAccount, AccountName: testCallerAccount}} // The CSV wrote the ID where the name lookup had failed. require.Error(t, requireResolvedAccountNames(rows, &Config{ExcludeAccounts: []string{"sandbox"}})) - require.NoError(t, requireResolvedAccountNames(rows, &Config{ExcludeAccounts: []string{acctSandbox}})) + require.NoError(t, requireResolvedAccountNames(rows, &Config{ExcludeAccounts: []string{testCallerAccount}})) require.NoError(t, requireResolvedAccountNames(rows, &Config{})) - named := []common.Recommendation{{Account: acctSandbox, AccountName: "sandbox"}} + named := []common.Recommendation{{Account: testCallerAccount, AccountName: "sandbox"}} require.NoError(t, requireResolvedAccountNames(named, &Config{ExcludeAccounts: []string{"sandbox"}})) - assert.False(t, shouldIncludeAccountIDOrName(acctSandbox, "sandbox", &Config{ExcludeAccounts: []string{acctSandbox}}), "an ID filter matches a named account") + assert.False(t, shouldIncludeAccountIDOrName(testCallerAccount, "sandbox", &Config{ExcludeAccounts: []string{testCallerAccount}}), "an ID filter matches a named account") } func TestFilterAndAdjustRecommendations_NameExcludeOnCSVRowsWithoutNamesFails(t *testing.T) { captureAppLog(t) - rows := []common.Recommendation{{Service: common.ServiceRDS, Region: "us-east-1", Account: acctSandbox, AccountName: acctSandbox, ResourceType: "db.t3.small", Count: 2}} + rows := []common.Recommendation{{Service: common.ServiceRDS, Region: "us-east-1", Account: testCallerAccount, AccountName: testCallerAccount, ResourceType: "db.t3.small", Count: 2}} recs, err := filterAndAdjustRecommendations(context.Background(), rows, 100, Config{ExcludeAccounts: []string{"sandbox"}, IncludeExtendedSupport: true})