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

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