diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index 9e5c8b7f1..e36a893a5 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -224,3 +224,97 @@ 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_AccountFilters(t *testing.T) { + captureAppLog(t) + 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} + } + base := Config{Regions: []string{"us-east-1"}, Coverage: 100, TermYears: 1, PaymentOption: "partial-upfront"} + // 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() + found := make([]common.Recommendation, 0, len(accounts)) + 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 + } + + 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, testCallerAccount) + require.Error(t, err) + 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, testCallerAccount) + require.Error(t, err) + }) + t.Run("ID-based exclude works without names", func(t *testing.T) { + cfg := base + cfg.ExcludeAccounts = []string{testCallerAccount} + got, _, err := run(t, cfg, testCallerAccount, otherAccount) + require.NoError(t, err) + assert.Equal(t, []string{otherAccount}, got) + }) + t.Run("ID-based include works without names", func(t *testing.T) { + cfg := base + cfg.IncludeAccounts = []string{otherAccount} + got, _, err := run(t, cfg, testCallerAccount, otherAccount) + require.NoError(t, err) + 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, testCallerAccount) + require.NoError(t, err) + 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{testCallerAccount} + accounts := make([]string, 20) + for i := range accounts { + accounts[i] = otherAccount + } + _, calls, err := run(t, cfg, accounts...) + require.NoError(t, err) + assert.Equal(t, 1, calls) + }) +} + +func TestRequireResolvedAccountNames_CSVRows(t *testing.T) { + 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{testCallerAccount}})) + require.NoError(t, requireResolvedAccountNames(rows, &Config{})) + named := []common.Recommendation{{Account: testCallerAccount, AccountName: "sandbox"}} + require.NoError(t, requireResolvedAccountNames(named, &Config{ExcludeAccounts: []string{"sandbox"}})) + 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: testCallerAccount, AccountName: testCallerAccount, 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 3102869f3..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,48 +60,68 @@ 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, } } -// 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, 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 "" + return "", nil } c.mu.RLock() if alias, ok := c.cache[accountID]; ok { c.mu.RUnlock() - return alias + return alias, nil + } + if err, ok := c.failed[accountID]; ok { + c.mu.RUnlock() + return "", err } 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 + } + if err, ok := c.failed[accountID]; ok { + return "", err } - // 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 + err = fmt.Errorf("describing account %s: %w", accountID, err) + c.failed[accountID] = err + return "", 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..3f395c74c 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,18 @@ 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_LookupErrorIsReturnedAndRememberedForTheRun(t *testing.T) { + ctx := context.Background() + org := &MockOrganizationsClient{} + org.On("DescribeAccount", ctx, &organizations.DescribeAccountInput{AccountId: aws.String("999888777666")}). + Return(nil, errors.New("AccessDenied")).Once() // a second call would fail the mock + + cache := NewAccountAliasCacheWithClient(org) + + 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..17ad45adb 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(`^\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 +// 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 627932f48..63210433a 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,17 @@ func stampSavingsPlanRegions(recs []common.Recommendation) { } // populateRecommendationAccountNames populates account names from account IDs. -func populateRecommendationAccountNames(ctx context.Context, recs []common.Recommendation, accountCache *AccountAliasCache) { +// 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 { for i := range recs { if recs[i].Account != "" { recs[i].AccountName = accountCache.GetAccountAlias(ctx, recs[i].Account) } } + return requireResolvedAccountNames(recs, &cfg) } // applyRegionFilters applies region and instance type filters to recommendations. @@ -854,7 +862,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 } 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