Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.
Where
providers/aws/recommendations/parser_sp.go:367-390 (the returned common.Recommendation never sets Region; the CE-supplied region is stashed only in Details.Region at :387, extracted at :252-261)providers/aws/service_client.go:123-137 (filterByIncludedRegions) and :140-154 (filterByExcludedRegions)- Evidence that EC2Instance SPs are genuinely region-bound:
providers/aws/services/savingsplans/client.go:310-329 (the client region-filters the offering lookup on spDetails.Region) and providers/aws/ladder/adapters.go:129-131 (the ladder excludes out-of-region EC2Instance SPs)
- Same
rec.Region predicate on the CLI path: cmd/multi_service_filters.go:89 and :126; per-account persistence at internal/config/resolver.go:43
What
Savings Plans recommendations are returned with Region left as the empty string. The region CE supplies is parsed and kept, but only inside Details.Region, which neither region filter reads.
Both filters then treat the empty string as "region-agnostic":
filterByIncludedRegions compares rec.Region against the include set with no Savings Plans carve-out, so an empty region matches nothing and the rec is dropped.filterByExcludedRegions treats an empty region as "not excluded", so the rec survives.
Empty region is being used as if it meant "region-agnostic", which is only true for Compute, SageMaker and Database Savings Plans. EC2Instance Savings Plans are region-bound and family-bound, as the purchase path and the ladder both already recognise.
Failure scenario
(a) Include direction. Any caller that sets RecommendationParams.IncludeRegions (persisted per account via internal/config/resolver.go:43, and reachable from the CLI via cmd/multi_service_filters.go) loses 100% of Savings Plans recommendations. An account filtering to us-east-1 loses its us-east-1 EC2Instance SP recommendation along with every Compute SP recommendation. Savings Plans are typically the largest savings line on an AWS account, so the biggest opportunity silently disappears and the result looks like "no SP savings available".
(b) Exclude direction. --exclude-regions eu-west-1 leaves an EC2Instance SP whose Details.Region is eu-west-1 in the result set and eligible for purchase. The operator explicitly excluded that region and a region-locked commitment is bought there anyway, where it will apply only to eu-west-1 capacity.
Both directions fail silently: no warning, no count of dropped or retained recs.
Fix direction
- Set
Region on the recommendation for EC2_INSTANCE_SP from ec2Fields.region, normalised through normalizeRegionName.
- Change the two filters so the exemption is gated on the recommendation genuinely being region-agnostic (a
common.CommitmentSavingsPlan whose plan type is Compute / SageMaker / Database) rather than on Region == "". An unknown or empty region on a region-bound plan type should not silently pass an explicit region filter in either direction.
- Regression tests for both directions: an EC2Instance SP for
us-east-1 must survive IncludeRegions=["us-east-1"], and must be dropped by ExcludeRegions=["us-east-1"].
Related
Reviewed commit:
be11bdcb5. Note:origin/mainmoved to3e9660d06during the review; re-verify against currentmainbefore changing code, since a finding may have been fixed or moved.Where
providers/aws/recommendations/parser_sp.go:367-390(the returnedcommon.Recommendationnever setsRegion; the CE-supplied region is stashed only inDetails.Regionat:387, extracted at:252-261)providers/aws/service_client.go:123-137(filterByIncludedRegions) and:140-154(filterByExcludedRegions)providers/aws/services/savingsplans/client.go:310-329(the client region-filters the offering lookup onspDetails.Region) andproviders/aws/ladder/adapters.go:129-131(the ladder excludes out-of-region EC2Instance SPs)rec.Regionpredicate on the CLI path:cmd/multi_service_filters.go:89and:126; per-account persistence atinternal/config/resolver.go:43What
Savings Plans recommendations are returned with
Regionleft as the empty string. The region CE supplies is parsed and kept, but only insideDetails.Region, which neither region filter reads.Both filters then treat the empty string as "region-agnostic":
filterByIncludedRegionscomparesrec.Regionagainst the include set with no Savings Plans carve-out, so an empty region matches nothing and the rec is dropped.filterByExcludedRegionstreats an empty region as "not excluded", so the rec survives.Empty region is being used as if it meant "region-agnostic", which is only true for Compute, SageMaker and Database Savings Plans. EC2Instance Savings Plans are region-bound and family-bound, as the purchase path and the ladder both already recognise.
Failure scenario
(a) Include direction. Any caller that sets
RecommendationParams.IncludeRegions(persisted per account viainternal/config/resolver.go:43, and reachable from the CLI viacmd/multi_service_filters.go) loses 100% of Savings Plans recommendations. An account filtering tous-east-1loses itsus-east-1EC2Instance SP recommendation along with every Compute SP recommendation. Savings Plans are typically the largest savings line on an AWS account, so the biggest opportunity silently disappears and the result looks like "no SP savings available".(b) Exclude direction.
--exclude-regions eu-west-1leaves an EC2Instance SP whoseDetails.Regioniseu-west-1in the result set and eligible for purchase. The operator explicitly excluded that region and a region-locked commitment is bought there anyway, where it will apply only toeu-west-1capacity.Both directions fail silently: no warning, no count of dropped or retained recs.
Fix direction
Regionon the recommendation forEC2_INSTANCE_SPfromec2Fields.region, normalised throughnormalizeRegionName.common.CommitmentSavingsPlanwhose plan type is Compute / SageMaker / Database) rather than onRegion == "". An unknown or empty region on a region-bound plan type should not silently pass an explicit region filter in either direction.us-east-1must surviveIncludeRegions=["us-east-1"], and must be dropped byExcludeRegions=["us-east-1"].Related
providers/aws/service_client.goor the missingRegionon the AWS SP parser, so this code path is not fixed by that work.