fix(cmd): fail the run when an account name cannot be resolved for an account filter

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)
+}

cmd/helpers.go

@@ -42,13 +42,15 @@ 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
 }
 
 // NewAccountAliasCache creates a new account alias cache.
 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.

cmd/helpers_test.go

@@ -99,15 +99,15 @@ 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{
 					AccountId: aws.String("999888777666"),
 				}).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)
+}

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

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).