fix(filters): match account filters exactly and refuse ambiguous excludes

CHANGELOG.md

@@ -85,6 +85,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
   `--min-pool-size` and `--min-savings-pct` before any API call. NaN used to
   pass validation and silently switch `--target-coverage` runs to
   `--coverage` sizing (#2136).
+- `--include-accounts` and `--exclude-accounts` are both exact now: they match
+  the account name or ID exactly (case-insensitive, surrounding whitespace
+  ignored) instead of by substring. Previously `--include-accounts dev` also selected `devops-prod`,
+  so a purchase run could buy in accounts that were not named. Behavior change:
+  anyone relying on substring matching must now list the full account names or
+  IDs. An `--exclude-accounts` value that matches no account exactly but is
+  part of account names now stops the run, naming those accounts, instead of
+  being applied as a substring (#2237).
 
 ### Changed
 

cmd/cancellation_prepurchase_test.go

@@ -11,6 +11,8 @@ import (
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
 	awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws"
 	"github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/aws/aws-sdk-go-v2/service/organizations"
+	orgtypes "github.com/aws/aws-sdk-go-v2/service/organizations/types"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -321,6 +323,13 @@ func TestRequireResolvedAccountNames_CSVRows(t *testing.T) {
 	assert.False(t, shouldIncludeAccountIDOrName(testCallerAccount, "sandbox", &Config{ExcludeAccounts: []string{testCallerAccount}}), "an ID filter matches a named account")
 }
 
+func TestRequireNamesResolved_BlankFilterValueIsNotANameFilter(t *testing.T) {
+	unresolved := []common.Recommendation{{Account: testCallerAccount, AccountName: testCallerAccount}}
+
+	require.NoError(t, requireNamesResolved(unresolved, &Config{ExcludeAccounts: []string{" "}}))
+	require.Error(t, requireNamesResolved(unresolved, &Config{ExcludeAccounts: []string{" ", "sandbox"}}))
+}
+
 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}}
@@ -330,3 +339,75 @@ func TestFilterAndAdjustRecommendations_NameExcludeOnCSVRowsWithoutNamesFails(t
 	require.Error(t, err, "replay must not let an excluded account through because its name is missing")
 	assert.Empty(t, recs)
 }
+
+func TestFetchAllRecs_ExcludeAccountsExactInAnotherBatchIsNotAmbiguous(t *testing.T) {
+	captureAppLog(t)
+	rec := func(region, account string) common.Recommendation {
+		return common.Recommendation{Service: common.ServiceRDS, Region: region, ResourceType: "db.t3.small", Count: 3, EstimatedSavings: 10, SavingsPercentage: 10, Account: account}
+	}
+	client := &MockRecommendationsClient{}
+	// Region discovery sees both; each per-region batch sees only its own account.
+	usProd, euProd := rec("us-east-1", "123456789012"), rec("eu-central-2", "999888777666")
+	client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "" })).
+		Return([]common.Recommendation{usProd, euProd}, nil)
+	client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "us-east-1" })).
+		Return([]common.Recommendation{usProd}, nil)
+	client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "eu-central-2" })).
+		Return([]common.Recommendation{euProd}, nil)
+	org := &MockOrganizationsClient{}
+	for id, name := range map[string]string{"123456789012": "prod", "999888777666": "prod-eu"} {
+		org.On("DescribeAccount", mock.Anything, mock.MatchedBy(func(in *organizations.DescribeAccountInput) bool { return *in.AccountId == id })).
+			Return(&organizations.DescribeAccountOutput{Account: &orgtypes.Account{Name: aws.String(name)}}, nil)
+	}
+	cfg := Config{TermYears: 1, PaymentOption: "no-upfront", Coverage: 100, ExcludeAccounts: []string{"prod"}}
+
+	awsCfg, _ := newAWSFixtureConfig(t)
+	recs, _, err := fetchAllRecs(context.Background(), awsCfg, client, NewAccountAliasCacheWithClient(org), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil)
+
+	require.NoError(t, err)
+	for i := range recs {
+		assert.NotEqual(t, "prod", recs[i].AccountName)
+	}
+}
+
+func TestFetchAllRecs_ExcludeAccountsPartialOnlyIsRefused(t *testing.T) {
+	captureAppLog(t)
+	rec := func(region, account string) common.Recommendation {
+		return common.Recommendation{Service: common.ServiceRDS, Region: region, ResourceType: "db.t3.small", Count: 3, EstimatedSavings: 10, SavingsPercentage: 10, Account: account}
+	}
+	usProd, euProd := rec("us-east-1", "123456789012"), rec("eu-central-2", "999888777666")
+	run := func(t *testing.T, exclude string) ([]common.Recommendation, error) {
+		t.Helper()
+		client := &MockRecommendationsClient{}
+		client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "" })).
+			Return([]common.Recommendation{usProd, euProd}, nil)
+		client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "us-east-1" })).
+			Return([]common.Recommendation{usProd}, nil)
+		client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { return p.Region == "eu-central-2" })).
+			Return([]common.Recommendation{euProd}, nil)
+		org := &MockOrganizationsClient{}
+		for id, name := range map[string]string{"123456789012": "prod-us", "999888777666": "prod-eu"} {
+			org.On("DescribeAccount", mock.Anything, mock.MatchedBy(func(in *organizations.DescribeAccountInput) bool { return *in.AccountId == id })).
+				Return(&organizations.DescribeAccountOutput{Account: &orgtypes.Account{Name: aws.String(name)}}, nil)
+		}
+		cfg := Config{TermYears: 1, PaymentOption: "no-upfront", Coverage: 100, ExcludeAccounts: []string{exclude}}
+		awsCfg, _ := newAWSFixtureConfig(t)
+		recs, _, err := fetchAllRecs(context.Background(), awsCfg, client, NewAccountAliasCacheWithClient(org), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil)
+		return recs, err
+	}
+
+	t.Run("a substring matching no account exactly is refused", func(t *testing.T) {
+		recs, err := run(t, "prod")
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "matches no account exactly")
+		assert.Contains(t, err.Error(), "prod-us")
+		assert.Contains(t, err.Error(), "prod-eu")
+		assert.Empty(t, recs)
+	})
+	t.Run("an exact name in another case excludes only that account", func(t *testing.T) {
+		recs, err := run(t, "PROD-US")
+		require.NoError(t, err)
+		require.Len(t, recs, 1)
+		assert.Equal(t, "prod-eu", recs[0].AccountName)
+	})
+}

cmd/helpers.go

@@ -66,6 +66,17 @@ func NewAccountAliasCacheWithClient(orgClient OrganizationsAPI) *AccountAliasCac
 	}
 }
 
+// knownAccounts lists every account whose name resolved this run.
+func (c *AccountAliasCache) knownAccounts() []common.Recommendation {
+	c.mu.RLock()
+	defer c.mu.RUnlock()
+	out := make([]common.Recommendation, 0, len(c.cache))
+	for id, name := range c.cache {
+		out = append(out, common.Recommendation{Account: id, AccountName: name})
+	}
+	return out
+}
+
 // 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.

cmd/main.go

@@ -127,8 +127,8 @@ func init() {
 	rootCmd.Flags().StringSliceVar(&toolCfg.ExcludeInstanceTypes, "exclude-instance-types", []string{}, "Exclude these instance types (comma-separated)")
 	rootCmd.Flags().StringSliceVar(&toolCfg.IncludeEngines, "include-engines", []string{}, "Only include these engines (comma-separated, e.g., 'redis,mysql,postgresql')")
 	rootCmd.Flags().StringSliceVar(&toolCfg.ExcludeEngines, "exclude-engines", []string{}, "Exclude these engines (comma-separated)")
-	rootCmd.Flags().StringSliceVar(&toolCfg.IncludeAccounts, "include-accounts", []string{}, "Only include recommendations for these account names (comma-separated)")
-	rootCmd.Flags().StringSliceVar(&toolCfg.ExcludeAccounts, "exclude-accounts", []string{}, "Exclude recommendations for these account names (comma-separated)")
+	rootCmd.Flags().StringSliceVar(&toolCfg.IncludeAccounts, "include-accounts", []string{}, "Only include recommendations for these accounts, matched exactly (case-insensitive) on account name or ID (comma-separated)")
+	rootCmd.Flags().StringSliceVar(&toolCfg.ExcludeAccounts, "exclude-accounts", []string{}, "Exclude recommendations for these accounts, matched exactly (case-insensitive) on account name or ID; a value that only partly matches account names stops the run (comma-separated)")
 	rootCmd.Flags().Int32Var(&toolCfg.MaxInstances, "max-instances", 0, "Maximum total number of instances to purchase (0 = no limit)")
 	rootCmd.Flags().Int32Var(&toolCfg.OverrideCount, "override-count", 0, "Override recommendation count with fixed number for all selected RIs (0 = use recommendation or coverage)")
 	rootCmd.Flags().StringVar(&toolCfg.ValidationProfile, "validation-profile", "", "AWS profile to use for validating running instances (if different from main profile)")

cmd/multi_service_filters.go

@@ -174,20 +174,17 @@ func shouldIncludeAccount(accountName string, cfg *Config) bool {
 }
 
 // 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.
+// to an account, matching each filter value against the account ID and the
+// account name, both trimmed and case-insensitive, never by substring.
+// Name-based values are only decidable once the name is known;
+// requireNamesResolved 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
 	}
 	matches := func(filter string) bool {
-		if accountID != "" && strings.EqualFold(accountID, filter) {
-			return true
-		}
-		return accountName != "" && accountMatchesFilter(strings.ToLower(accountName), filter)
+		return accountMatchesFilter(accountID, filter) || accountMatchesFilter(accountName, filter)
 	}
 	if len(cfg.IncludeAccounts) > 0 && !slices.ContainsFunc(cfg.IncludeAccounts, matches) {
 		return false
@@ -199,19 +196,30 @@ func shouldIncludeAccountIDOrName(accountID, accountName string, cfg *Config) bo
 // 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
+// requireResolvedAccountNames runs both account-filter pre-checks over a full
+// set of recommendations: requireNamesResolved and requireExactExcludes.
+func requireResolvedAccountNames(recs []common.Recommendation, cfg *Config) error {
+	if err := requireNamesResolved(recs, cfg); err != nil {
+		return err
+	}
+	return requireExactExcludes(recs, cfg)
+}
+
+// requireNamesResolved 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 {
+func requireNamesResolved(recs []common.Recommendation, cfg *Config) error {
 	nameFilter := ""
 	for _, v := range slices.Concat(cfg.IncludeAccounts, cfg.ExcludeAccounts) {
-		if !accountIDPattern.MatchString(v) {
-			nameFilter = v
-			break
+		v = strings.TrimSpace(v)
+		if v == "" || accountIDPattern.MatchString(v) {
+			continue
 		}
+		nameFilter = v
+		break
 	}
 	if nameFilter == "" {
 		return nil
@@ -224,8 +232,45 @@ func requireResolvedAccountNames(recs []common.Recommendation, cfg *Config) erro
 	return nil
 }
 
-// accountMatchesFilter checks if an account matches a filter pattern (exact or substring match).
-func accountMatchesFilter(accountLower, filter string) bool {
-	filterLower := strings.ToLower(filter)
-	return filterLower == accountLower || strings.Contains(accountLower, filterLower)
+// requireExactExcludes fails when an --exclude-accounts value matches no
+// account exactly (name or ID) but is part of other account names: the
+// operator likely meant a substring, which no longer excludes anything, so
+// carrying on would buy in accounts they meant to skip. A value matching
+// nothing at all is a harmless no-op. recs must hold every account of the run,
+// not one batch, or an exact account in another batch is missed.
+func requireExactExcludes(recs []common.Recommendation, cfg *Config) error {
+	for _, raw := range cfg.ExcludeAccounts {
+		if partial := partialOnlyMatches(recs, raw); len(partial) > 0 {
+			return fmt.Errorf("--exclude-accounts %q matches no account exactly but is part of \"%s\"; list the full account names to exclude", strings.TrimSpace(raw), strings.Join(partial, `", "`))
+		}
+	}
+	return nil
+}
+
+// partialOnlyMatches returns the sorted account names that contain the filter
+// as a substring, or nil when the filter is blank or matches an account exactly.
+func partialOnlyMatches(recs []common.Recommendation, filter string) []string {
+	f := strings.ToLower(strings.TrimSpace(filter))
+	if f == "" {
+		return nil
+	}
+	var partial []string
+	for i := range recs {
+		if accountMatchesFilter(recs[i].Account, f) || accountMatchesFilter(recs[i].AccountName, f) {
+			return nil
+		}
+		if n := recs[i].AccountName; strings.Contains(strings.ToLower(n), f) && !slices.Contains(partial, n) {
+			partial = append(partial, n)
+		}
+	}
+	slices.Sort(partial)
+	return partial
+}
+
+// accountMatchesFilter reports whether value (an account name or ID) equals the
+// filter, ignoring surrounding whitespace and case. Never a substring match, so
+// "dev" does not select "devops-prod". An empty value or filter never matches.
+func accountMatchesFilter(value, filter string) bool {
+	value, filter = strings.TrimSpace(value), strings.TrimSpace(filter)
+	return value != "" && filter != "" && strings.EqualFold(value, filter)
 }

cmd/multi_service_filters_test.go

@@ -872,3 +872,76 @@ savings-plans-ec2instance,,,1,1yr,no-upfront,EC2Instance,1.5,m5,us-east-1
 		})
 	}
 }
+
+func TestShouldIncludeAccountIDOrNameExactMatch(t *testing.T) {
+	const id = "123456789012"
+	tests := []struct {
+		name        string
+		accountName string
+		include     []string
+		exclude     []string
+		want        bool
+	}{
+		{"include dev does not select devops-prod", "devops-prod", []string{"dev"}, nil, false},
+		{"exclude dev does not drop devops-prod", "devops-prod", nil, []string{"dev"}, true},
+		{"include exact name different case", "DevOps-Prod", []string{"devops-prod"}, nil, true},
+		{"exclude exact name different case", "devops-prod", nil, []string{"DEVOPS-PROD"}, false},
+		{"include exact ID", "devops-prod", []string{id}, nil, true},
+		{"exclude exact ID", "devops-prod", nil, []string{id}, false},
+		{"include name with surrounding whitespace", "dev", []string{"  dev\t"}, nil, true},
+		{"exclude ID with surrounding whitespace", "dev", nil, []string{" " + id + " "}, false},
+		{"include substring of ID does not match", "devops-prod", []string{"12345"}, nil, false},
+		{"exclude substring of ID does not drop", "devops-prod", nil, []string{"9012"}, true},
+		{"blank include never matches", "dev", []string{"  "}, nil, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg := &Config{IncludeAccounts: tt.include, ExcludeAccounts: tt.exclude}
+			assert.Equal(t, tt.want, shouldIncludeAccountIDOrName(id, tt.accountName, cfg))
+		})
+	}
+}
+
+func TestRequireExactExcludes(t *testing.T) {
+	recs := []common.Recommendation{
+		{
+			Account:     "123456789012",
+			AccountName: "dev-1",
+		},
+		{
+			Account:     "111222333444",
+			AccountName: "devops",
+		},
+		{
+			Account:     "999888777666",
+			AccountName: "prod",
+		},
+	}
+	tests := []struct {
+		name    string
+		exclude []string
+		include []string
+		wantErr []string
+	}{
+		{"partial exclude errors and names accounts", []string{"dev"}, nil, []string{`"dev-1", "devops"`, `"dev"`}},
+		{"partial exclude with whitespace and case", []string{" DEV "}, nil, []string{`"dev-1", "devops"`}},
+		{"exact name exclude works", []string{"DevOps"}, nil, nil},
+		{"exact ID exclude works", []string{"123456789012"}, nil, nil},
+		{"exclude matching nothing is a no-op", []string{"zzz"}, nil, nil},
+		{"include stays exact and does not error", nil, []string{"dev"}, nil},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := requireResolvedAccountNames(recs, &Config{ExcludeAccounts: tt.exclude, IncludeAccounts: tt.include})
+			if tt.wantErr == nil {
+				assert.NoError(t, err)
+				return
+			}
+			if assert.Error(t, err) {
+				for _, w := range tt.wantErr {
+					assert.Contains(t, err.Error(), w)
+				}
+			}
+		})
+	}
+}

cmd/multi_service_helpers.go

@@ -657,15 +657,15 @@ func stampSavingsPlanRegions(recs []common.Recommendation) {
 // populateRecommendationAccountNames populates account names from account IDs.
 // 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
+// ID-based account filters; requireNamesResolved 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)
+	return requireNamesResolved(recs, &cfg)
 }
 
 // applyRegionFilters applies region and instance type filters to recommendations.
@@ -950,5 +950,10 @@ func fetchAllRecs(
 			all = append(all, recs...)
 		}
 	}
+	// Checked once over every account seen in any batch, including those the
+	// filters dropped, so an exact account in another batch is not missed.
+	if err := requireExactExcludes(accountCache.knownAccounts(), &cfg); err != nil {
+		return nil, nil, fmt.Errorf("%w: %w", errRecommendationFetch, err)
+	}
 	return all, drops, nil
 }