diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index ecf0edc58..2d652baf4 100644 --- a/cmd/multi_service_filters.go +++ b/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. diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index ded57e93b..f742435ec 100644 --- a/cmd/multi_service_filters_test.go +++ b/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") + }) + } +} diff --git a/providers/aws/service_client.go b/providers/aws/service_client.go index 3804b1223..2232eba15 100644 --- a/providers/aws/service_client.go +++ b/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) } } diff --git a/providers/aws/service_client_test.go b/providers/aws/service_client_test.go index c95233dea..c01f7ab13 100644 --- a/providers/aws/service_client_test.go +++ b/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") + }) +}