fix(cmd): match Savings Plans region filters on Details.Region (#1881)

The provider-side half of #1582 was already merged. This closes the CLI half, which the issue names explicitly and which was left untouched.

AWS Savings Plans recommendations never populate the top-level `Region`. `providers/aws/recommendations/parser_sp.go` sets it only inside the nested `Details: &common.SavingsPlanDetails{...}` struct. `passesDimensionFilters` was calling `shouldIncludeRegion(rec.Region, cfg)` with that bare field, which is `""` for every SP recommendation, so `--include-regions` silently dropped all of them and `--exclude-regions` silently let them all through. The user saw fewer recommendations, or the wrong ones, with no explanation either way.

## What changed

`passesDimensionFilters` routes through a four-line helper that asks `IsRegionAgnostic` first and otherwise matches on `EffectiveRegion`. Those two predicates already existed on the provider side and are reused rather than reimplemented; they were unexported, so the change exports them, which is a pure identifier rename with no logic touched. `cmd` already imports `providers/aws`, so no new dependency edge. Moving them to `pkg/common` was rejected because `isAccountLevelSPPlanType` compares against `sptypes.SavingsPlanType` SDK members and the `pkg/` module carries no `savingsplans` dependency.

**Empty is not region-agnostic.** The fix deliberately does not make a blank `Region` match every filter. A genuinely region-agnostic Savings Plan (Compute, SageMaker, Database) and an EC2Instance plan whose region failed to parse are different things, and only the first is exempt. `IsRegionAgnostic` decides that on positive plan-type evidence, so the fix cannot turn a silent drop into a silent over-inclusion, which on a purchasing tool is the worse failure.

Exporting those helpers widened what they can be handed, so the second commit confines them to `common.ProviderAWS`. While package-private they only ever saw recommendations the AWS parsers built, and "these Details are AWS Savings Plans Details" held by construction; exported, the invariant rested on no caller making that mistake. `common.SavingsPlanDetails` and `common.CommitmentSavingsPlan` are shared types, and Azure's savings plans client type-asserts on the former in three places, so a non-AWS recommendation carrying those fields would have been read with the AWS meaning. Not reachable today: no Azure code constructs a `common.Recommendation` with `CommitmentSavingsPlan`, the only non-AWS assignment of that constant being on a `common.Commitment`, a different type. The guard restores an invariant this change weakened rather than defending a reachable state, and it sits in the helpers so every caller inherits it instead of just the one call site.

## How it was verified

`TestApplyFilters_SavingsPlansRegionFilters` drives seven cases through the real `applyFilters` path. With the production line reverted it fails three ways: an in-region EC2Instance SP dropped by `--include-regions`, a region-agnostic Compute SP dropped by `--include-regions`, and an EC2Instance SP leaking past `--exclude-regions`. The four already-correct cases pin the over-inclusion trap, since an EC2Instance SP with an empty `Details.Region` is region-scoped, not agnostic, and stays dropped.

`TestRegionHelpers_NonAWSRecommendation` pins the provider gate, and each half was checked for teeth independently rather than together: removing only the `IsRegionAgnostic` guard fails `IsRegionAgnostic_is_false_for_a_non-AWS_rec`, and removing only the `EffectiveRegion` guard fails `EffectiveRegion_does_not_read_Details_of_a_non-AWS_rec`. Tested jointly, the first assertion aborting would have masked whether the second was pinned at all.

`go build ./...`, the full `cmd` package, the `providers/aws` module, `go vet` and `gofmt` all clean. `gocyclo -over 10` clean, confirmed to be scanning by a `-over 6` run reporting 134 functions. `passesDimensionFilters` stays at complexity 5.

The first CI attempt failed in `lint` before analysing any code: `golangci-lint config verify` fetches its JSON schema over the network and the request timed out. Re-run, green.

## Sibling dimension filters: checked, neither is the same defect

**Instance type** does drop SP recommendations, because `ResourceType` is empty. But `SavingsPlanDetails` has no instance type; it has `InstanceFamily` (`"m5"`), a different granularity, only for EC2Instance plans. Matching one against the other needs a semantics decision this issue does not specify, and there is no provider-side instance-type filter to mirror. Left alone deliberately.

**Engine** has no bare-field bug: `SavingsPlanDetails` carries no engine attribute, because Cost Explorer returns none for any SP plan type.

## Noted, not fixed

`shouldIncludeRegion` does not skip blank entries the way the provider's `regionSet` does, so `--include-regions "us-east-1,"` can admit an unknown-region recommendation. Pre-existing, affects all recommendation types, and strictly improved for Savings Plans by this change.

Not verified: no live AWS run, no credentials in this environment.

Closes #1582

Author: cristim

cmd/multi_service_filters.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 
 	"github.com/LeanerCloud/CUDly/pkg/common"
+	awsprovider "github.com/LeanerCloud/CUDly/providers/aws"
 )
 
 // applyFilters applies region, instance type, engine, and engine version filters to recommendations.
@@ -86,7 +87,7 @@ func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVers
 // dimension filters here are pure functions of rec + cfg with no side
 // effects. Pool-size filtering is handled with logging in applyFilters.
 func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool {
-	if !shouldIncludeRegion(rec.Region, cfg) {
+	if !shouldIncludeRecommendationRegion(rec, cfg) {
 		return false
 	}
 	if !shouldIncludeInstanceType(rec.ResourceType, cfg) {
@@ -122,6 +123,25 @@ func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool {
 	return rec.AverageInstancesUsedPerHour >= cfg.MinPoolSize
 }
 
+// shouldIncludeRecommendationRegion applies the region filters to a whole
+// recommendation rather than to its bare Region field. Savings Plans
+// recommendations leave the top-level Region empty and carry the
+// EC2Instance-scoped region in Details instead, so matching on rec.Region
+// alone dropped every SP recommendation under --include-regions and leaked
+// region-scoped ones past --exclude-regions (#1582).
+//
+// Both predicates are the provider package's, which already filters AWS
+// recommendations on these semantics, rather than a second implementation.
+// Recommendations from other providers are unaffected: both predicates are
+// gated on common.CommitmentSavingsPlan and fall through to the plain
+// rec.Region comparison for everything else.
+func shouldIncludeRecommendationRegion(rec *common.Recommendation, cfg *Config) bool {
+	if awsprovider.IsRegionAgnostic(*rec) {
+		return true
+	}
+	return shouldIncludeRegion(awsprovider.EffectiveRegion(*rec), cfg)
+}
+
 // shouldIncludeRegion checks if a region should be included based on filters.
 func shouldIncludeRegion(region string, cfg *Config) bool {
 	// If include list is specified, region must be in it.

cmd/multi_service_filters_test.go

@@ -528,3 +528,104 @@ func TestApplyFilters_DropExtendedSupport(t *testing.T) {
 	assert.Contains(t, d.FormatOneLine(), common.DropExtendedSupport,
 		"drop summary should name the --include-extended-support category")
 }
+
+// TestApplyFilters_SavingsPlansRegionFilters covers the CLI half of #1582.
+// Savings Plans recommendations never populate the top-level rec.Region
+// (parser_sp.go stores the CE-supplied region in Details.Region instead), so
+// filtering on the bare field dropped every SP recommendation whenever
+// --include-regions was set, and leaked region-scoped EC2Instance SPs past
+// --exclude-regions. The provider-side filters were fixed first; these cases
+// pin the same semantics on the CLI path.
+func TestApplyFilters_SavingsPlansRegionFilters(t *testing.T) {
+	ec2SP := func(region string) common.Recommendation {
+		return common.Recommendation{
+			Provider:       common.ProviderAWS,
+			Service:        common.ServiceSavingsPlansEC2Instance,
+			CommitmentType: common.CommitmentSavingsPlan,
+			Count:          1,
+			Details: &common.SavingsPlanDetails{
+				PlanType:       "EC2Instance",
+				InstanceFamily: "m5",
+				Region:         region,
+			},
+		}
+	}
+	computeSP := func() common.Recommendation {
+		return common.Recommendation{
+			Provider:       common.ProviderAWS,
+			Service:        common.ServiceSavingsPlansCompute,
+			CommitmentType: common.CommitmentSavingsPlan,
+			Count:          1,
+			Details:        &common.SavingsPlanDetails{PlanType: "Compute"},
+		}
+	}
+
+	tests := []struct {
+		name           string
+		rec            common.Recommendation
+		includeRegions []string
+		excludeRegions []string
+		wantKept       bool
+	}{
+		{
+			name:           "EC2Instance SP in the included region survives",
+			rec:            ec2SP("us-east-1"),
+			includeRegions: []string{"us-east-1"},
+			wantKept:       true,
+		},
+		{
+			name:           "EC2Instance SP outside the included region is dropped",
+			rec:            ec2SP("eu-west-1"),
+			includeRegions: []string{"us-east-1"},
+			wantKept:       false,
+		},
+		{
+			name:           "region-agnostic Compute SP survives an include filter",
+			rec:            computeSP(),
+			includeRegions: []string{"us-east-1"},
+			wantKept:       true,
+		},
+		{
+			name:           "EC2Instance SP in an excluded region is dropped",
+			rec:            ec2SP("eu-west-1"),
+			excludeRegions: []string{"eu-west-1"},
+			wantKept:       false,
+		},
+		{
+			name:           "region-agnostic Compute SP survives an exclude filter",
+			rec:            computeSP(),
+			excludeRegions: []string{"eu-west-1"},
+			wantKept:       true,
+		},
+		{
+			// Cost Explorer omitted SavingsPlansDetails.Region. An EC2Instance
+			// SP is region-scoped, so an unknown region must not be treated as
+			// region-agnostic and waved past an explicit region filter.
+			name:           "EC2Instance SP with an unknown region is not over-included",
+			rec:            ec2SP(""),
+			includeRegions: []string{"us-east-1"},
+			wantKept:       false,
+		},
+		{
+			name:           "reservation rec with an unknown region is still dropped",
+			rec:            common.Recommendation{Service: common.ServiceRDS, CommitmentType: common.CommitmentReservedInstance, Count: 1},
+			includeRegions: []string{"us-east-1"},
+			wantKept:       false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg := Config{
+				IncludeRegions: tt.includeRegions,
+				ExcludeRegions: tt.excludeRegions,
+			}
+			want := 0
+			if tt.wantKept {
+				want = 1
+			}
+			got := applyFilters([]common.Recommendation{tt.rec}, &cfg, nil, nil, "", common.NewDropSummary())
+			assert.Len(t, got, want, "region filter kept the wrong number of recommendations")
+		})
+	}
+}

providers/aws/service_client.go

@@ -132,7 +132,7 @@ func filterByAccounts(recs []common.Recommendation, accounts []string) []common.
 	return filtered
 }
 
-// effectiveRegion returns the region to use for region-filter matching.
+// EffectiveRegion returns the region to use for region-filter matching.
 // Savings Plans recommendations never populate the top-level rec.Region
 // (GetSavingsPlansPurchaseRecommendation is account-level and carries no
 // region parameter -- see applyRecommendationFilters); EC2Instance Savings
@@ -142,10 +142,17 @@ func filterByAccounts(recs []common.Recommendation, accounts []string) []common.
 // EC2Instance SP recs on their real region instead of dropping them via an
 // always-empty top-level Region. Compute/SageMaker/Database SP recs are
 // genuinely region-agnostic (Details.Region stays "" for them), so this
-// still returns "" for those, and isRegionAgnostic is what decides that such
+// still returns "" for those, and IsRegionAgnostic is what decides that such
 // a rec is exempt from region filtering (see #1495).
-func effectiveRegion(rec common.Recommendation) string {
-	if rec.Region != "" {
+//
+// The Details fallback is confined to ProviderAWS. common.SavingsPlanDetails
+// is a shared type that Azure's savingsplans client also asserts on, so
+// exporting this predicate (#1582) made it reachable with a non-AWS rec whose
+// Details mean something else. Only AWS builds Savings Plans recommendations
+// today and parser_sp.go stamps ProviderAWS on every one, so the gate changes
+// no behaviour now; it keeps the AWS-only reading from outliving that invariant.
+func EffectiveRegion(rec common.Recommendation) string {
+	if rec.Region != "" || rec.Provider != common.ProviderAWS {
 		return rec.Region
 	}
 	if sp, ok := rec.Details.(*common.SavingsPlanDetails); ok && sp != nil {
@@ -154,7 +161,7 @@ func effectiveRegion(rec common.Recommendation) string {
 	return ""
 }
 
-// isRegionAgnostic reports whether rec legitimately belongs to no single
+// IsRegionAgnostic reports whether rec legitimately belongs to no single
 // region and must therefore be exempt from both region filters: an
 // account-level Savings Plan (Compute/SageMaker/Database), which
 // GetSavingsPlansPurchaseRecommendation returns without any region because
@@ -186,8 +193,13 @@ func effectiveRegion(rec common.Recommendation) string {
 // unknown (nil Details, a non-SavingsPlanDetails payload, a plan type this
 // build has never heard of) stays region-scoped and is filtered
 // conservatively rather than exempted.
-func isRegionAgnostic(rec common.Recommendation) bool {
-	if rec.CommitmentType != common.CommitmentSavingsPlan || effectiveRegion(rec) != "" {
+//
+// ProviderAWS is required for the same reason EffectiveRegion requires it:
+// this exemption is a statement about AWS Savings Plans products, and both
+// common.CommitmentSavingsPlan and common.SavingsPlanDetails are shared types
+// another provider could populate with different semantics.
+func IsRegionAgnostic(rec common.Recommendation) bool {
+	if rec.Provider != common.ProviderAWS || rec.CommitmentType != common.CommitmentSavingsPlan || EffectiveRegion(rec) != "" {
 		return false
 	}
 	sp, ok := rec.Details.(*common.SavingsPlanDetails)
@@ -236,14 +248,14 @@ func regionSet(regions []string) map[string]bool {
 
 // filterByIncludedRegions filters recommendations to only included regions.
 // Region-agnostic recommendations (account-level Savings Plans -- see
-// isRegionAgnostic) are always kept: an include filter narrows region-scoped
+// IsRegionAgnostic) are always kept: an include filter narrows region-scoped
 // recs, it must not silently drop recs that belong to no region at all.
 func filterByIncludedRegions(recs []common.Recommendation, regions []string) []common.Recommendation {
 	regionMap := regionSet(regions)
 
 	filtered := make([]common.Recommendation, 0, len(recs))
 	for _, rec := range recs {
-		if isRegionAgnostic(rec) || regionMap[effectiveRegion(rec)] {
+		if IsRegionAgnostic(rec) || regionMap[EffectiveRegion(rec)] {
 			filtered = append(filtered, rec)
 		}
 	}
@@ -253,14 +265,14 @@ func filterByIncludedRegions(recs []common.Recommendation, regions []string) []c
 
 // filterByExcludedRegions filters out recommendations from excluded regions.
 // Region-agnostic recommendations (account-level Savings Plans -- see
-// isRegionAgnostic) are never excluded: they do not belong to any of the
+// IsRegionAgnostic) are never excluded: they do not belong to any of the
 // excluded regions.
 func filterByExcludedRegions(recs []common.Recommendation, regions []string) []common.Recommendation {
 	regionMap := regionSet(regions)
 
 	filtered := make([]common.Recommendation, 0, len(recs))
 	for _, rec := range recs {
-		if isRegionAgnostic(rec) || !regionMap[effectiveRegion(rec)] {
+		if IsRegionAgnostic(rec) || !regionMap[EffectiveRegion(rec)] {
 			filtered = append(filtered, rec)
 		}
 	}

providers/aws/service_client_test.go

@@ -352,12 +352,14 @@ func TestApplyRecommendationFilters_SavingsPlanRegion(t *testing.T) {
 	accountLevelSP := common.Recommendation{
 		Account:        "111",
 		Region:         "",
+		Provider:       common.ProviderAWS,
 		CommitmentType: common.CommitmentSavingsPlan,
 		Details:        &common.SavingsPlanDetails{PlanType: "Compute"},
 	}
 	ec2InstanceSP := common.Recommendation{
 		Account:        "222",
 		Region:         "",
+		Provider:       common.ProviderAWS,
 		CommitmentType: common.CommitmentSavingsPlan,
 		Details:        &common.SavingsPlanDetails{PlanType: "EC2Instance", Region: "us-east-1"},
 	}
@@ -410,7 +412,7 @@ func TestApplyRecommendationFilters_SavingsPlanRegion(t *testing.T) {
 // "us-east-1 only" filter and reach the purchase path, to be bought in
 // whatever region the service client resolved. This test fails on the
 // empty-region-means-agnostic version and passes with the
-// CommitmentSavingsPlan-gated isRegionAgnostic.
+// CommitmentSavingsPlan-gated IsRegionAgnostic.
 func TestApplyRecommendationFilters_RegionlessReservationNotExempt(t *testing.T) {
 	regionlessRI := common.Recommendation{
 		Account:        "111",
@@ -453,12 +455,12 @@ func TestApplyRecommendationFilters_RegionlessReservationNotExempt(t *testing.T)
 // empty effective region, i.e. a region-less reservation (see
 // TestApplyRecommendationFilters_RegionlessReservationNotExempt for why those
 // exist). An account-level Savings Plan would NOT pin this: it
-// short-circuits on isRegionAgnostic before the map is ever consulted, so
+// short-circuits on IsRegionAgnostic before the map is ever consulted, so
 // that subtest would pass with or without the blank skip and prove nothing.
 func TestApplyRecommendationFilters_BlankRegionEntryIsNotAMatcher(t *testing.T) {
 	// Not region-agnostic (a reservation), but carries no region because
-	// Cost Explorer omitted the field. effectiveRegion is "" and
-	// isRegionAgnostic is false, so this rec reaches the map lookup.
+	// Cost Explorer omitted the field. EffectiveRegion is "" and
+	// IsRegionAgnostic is false, so this rec reaches the map lookup.
 	regionlessRI := common.Recommendation{
 		Account:        "111",
 		CommitmentType: common.CommitmentReservedInstance,
@@ -504,13 +506,14 @@ func TestApplyRecommendationFilters_BlankRegionEntryIsNotAMatcher(t *testing.T)
 // in whatever region the service client resolved -- the same defect, one plan
 // type over.
 //
-// This test fails on the CommitmentSavingsPlan-only isRegionAgnostic and
+// This test fails on the CommitmentSavingsPlan-only IsRegionAgnostic and
 // passes once the exemption also requires isAccountLevelSPPlanType.
 func TestApplyRecommendationFilters_RegionlessEC2InstanceSPNotExempt(t *testing.T) {
 	// An EC2Instance SP whose CE payload carried no region: region-scoped,
 	// but with nothing to match a region filter against.
 	regionlessEC2SP := common.Recommendation{
 		Account:        "111",
+		Provider:       common.ProviderAWS,
 		CommitmentType: common.CommitmentSavingsPlan,
 		Details:        &common.SavingsPlanDetails{PlanType: "EC2Instance"},
 	}
@@ -532,6 +535,7 @@ func TestApplyRecommendationFilters_RegionlessEC2InstanceSPNotExempt(t *testing.
 		for _, planType := range []string{"Compute", "SageMaker", "Database"} {
 			accountLevelSP := common.Recommendation{
 				Account:        "111",
+				Provider:       common.ProviderAWS,
 				CommitmentType: common.CommitmentSavingsPlan,
 				Details:        &common.SavingsPlanDetails{PlanType: planType},
 			}
@@ -548,6 +552,7 @@ func TestApplyRecommendationFilters_RegionlessEC2InstanceSPNotExempt(t *testing.
 		// conservative direction is to filter it, not to exempt it.
 		unknownSP := common.Recommendation{
 			Account:        "111",
+			Provider:       common.ProviderAWS,
 			CommitmentType: common.CommitmentSavingsPlan,
 			Details:        &common.SavingsPlanDetails{PlanType: "SomeFutureSP"},
 		}
@@ -559,6 +564,7 @@ func TestApplyRecommendationFilters_RegionlessEC2InstanceSPNotExempt(t *testing.
 	t.Run("an SP carrying no Details at all is not exempt", func(t *testing.T) {
 		noDetailsSP := common.Recommendation{
 			Account:        "111",
+			Provider:       common.ProviderAWS,
 			CommitmentType: common.CommitmentSavingsPlan,
 		}
 		got := applyRecommendationFilters([]common.Recommendation{noDetailsSP},
@@ -569,6 +575,7 @@ func TestApplyRecommendationFilters_RegionlessEC2InstanceSPNotExempt(t *testing.
 	t.Run("an EC2Instance SP that DOES carry its region is filtered on that region", func(t *testing.T) {
 		ec2SPInUsEast := common.Recommendation{
 			Account:        "111",
+			Provider:       common.ProviderAWS,
 			CommitmentType: common.CommitmentSavingsPlan,
 			Details:        &common.SavingsPlanDetails{PlanType: "EC2Instance", Region: "us-east-1"},
 		}
@@ -581,3 +588,56 @@ func TestApplyRecommendationFilters_RegionlessEC2InstanceSPNotExempt(t *testing.
 		assert.Empty(t, dropped, "a non-matching region must still be dropped")
 	})
 }
+
+// TestRegionHelpers_NonAWSRecommendation pins the provider gate on the two
+// exported region helpers.
+//
+// While they were package-private they could only ever see recommendations the
+// AWS parsers built, so "these Details are AWS Savings Plans Details" held by
+// construction. Exporting them for the CLI (#1582) removed that guarantee: any
+// caller can now pass any recommendation. common.CommitmentSavingsPlan and
+// common.SavingsPlanDetails are shared types, and Azure already type-asserts on
+// SavingsPlanDetails in its own savingsplans client, so the shape below is one
+// an exported helper must handle even though no Azure code builds a Savings
+// Plans *recommendation* today.
+func TestRegionHelpers_NonAWSRecommendation(t *testing.T) {
+	nonAWSSP := common.Recommendation{
+		Provider:       common.ProviderAzure,
+		CommitmentType: common.CommitmentSavingsPlan,
+		Details:        &common.SavingsPlanDetails{PlanType: "Compute", Region: "eastus"},
+	}
+
+	t.Run("EffectiveRegion does not read Details of a non-AWS rec", func(t *testing.T) {
+		assert.Empty(t, EffectiveRegion(nonAWSSP),
+			"a non-AWS recommendation must report its own Region, not an AWS-shaped reading of Details")
+	})
+
+	t.Run("EffectiveRegion returns a non-AWS rec's own Region unchanged", func(t *testing.T) {
+		withRegion := nonAWSSP
+		withRegion.Region = "westeurope"
+		assert.Equal(t, "westeurope", EffectiveRegion(withRegion))
+	})
+
+	t.Run("IsRegionAgnostic is false for a non-AWS rec", func(t *testing.T) {
+		// Deliberately region-less. Given a populated Details.Region the
+		// EffectiveRegion(rec) != "" arm rejects the rec on its own and the
+		// provider gate is never reached, so the assertion would hold with or
+		// without the gate and prove nothing. Every other arm has to pass for
+		// this case to have teeth: CommitmentSavingsPlan, an empty effective
+		// region, and an account-level plan type.
+		nonAWSAccountLevelSP := common.Recommendation{
+			Provider:       common.ProviderAzure,
+			CommitmentType: common.CommitmentSavingsPlan,
+			Details:        &common.SavingsPlanDetails{PlanType: "Compute"},
+		}
+		assert.False(t, IsRegionAgnostic(nonAWSAccountLevelSP),
+			"the account-level exemption is a statement about AWS Savings Plans products")
+	})
+
+	t.Run("the same rec on ProviderAWS still gets the AWS reading", func(t *testing.T) {
+		awsSP := nonAWSSP
+		awsSP.Provider = common.ProviderAWS
+		assert.Equal(t, "eastus", EffectiveRegion(awsSP),
+			"the gate must not change behaviour for AWS recs, or it would break the #1582 fix")
+	})
+}