diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index 06533a00e..6d054c9c1 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -217,6 +217,14 @@ func TestCanonicalSuppressionService(t *testing.T) { {"aws", "search", "search"}, {"aws", "data-warehouse", "data-warehouse"}, {"aws", "memorydb", "memorydb"}, + {"aws", "savingsplans-compute", "savings-plans-compute"}, + {"aws", "savingsplans-ec2instance", "savings-plans-ec2instance"}, + {"aws", "savingsplans-sagemaker", "savings-plans-sagemaker"}, + {"aws", "savingsplans-database", "savings-plans-database"}, + {"aws", "savings-plans-compute", "savings-plans-compute"}, + {"aws", "savingsplans", "savingsplans"}, + {"azure", "savingsplans", "savingsplans"}, + {"azure", "savingsplans-compute", "savingsplans-compute"}, {"azure", "compute", "compute"}, {"azure", "ec2", "ec2"}, {"azure", "rds", "rds"}, diff --git a/internal/config/types.go b/internal/config/types.go index 06b931371..b6004e6b7 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -782,12 +782,26 @@ var awsLegacyServiceSlugs = map[string]string{ string(common.ServiceRedshift): string(common.ServiceDataWarehouse), } -// CanonicalSuppressionService maps legacy AWS service slugs to their canonical -// slug so a suppression and a rec match whichever spelling each was stored -// under (issues #1412, #1420). Other providers' services, including Azure and -// GCP "compute", are returned unchanged. It lives in config, the lowest -// package both the scheduler and BuildSuppressions import; the pinned -// cloud-commitments-go common package has no such mapping. +// awsSavingsPlansSpellings maps the dash-free spelling of each per-plan-type +// Savings Plans slug, which the purchase manager's mapSavingsPlansSlug also +// accepts, to the hyphenated library constant that recs carry (issue #1422). +// The "savingsplans" umbrella is deliberately absent: folding it into one plan +// type would let a single umbrella suppression hide another plan type's recs. +// A purchase test checks every per-type key of mapSavingsPlansSlug against it. +var awsSavingsPlansSpellings = map[string]string{ + "savingsplans-compute": string(common.ServiceSavingsPlansCompute), + "savingsplans-ec2instance": string(common.ServiceSavingsPlansEC2Instance), + "savingsplans-sagemaker": string(common.ServiceSavingsPlansSageMaker), + "savingsplans-database": string(common.ServiceSavingsPlansDatabase), +} + +// CanonicalSuppressionService maps legacy AWS service slugs, and the dash-free +// spellings of the per-type Savings Plans slugs, to their canonical slug so a +// suppression and a rec match whichever spelling each was stored under +// (issues #1412, #1420, #1422). Other providers' services, including Azure and +// GCP "compute" and Azure "savingsplans", are returned unchanged. It lives in +// config, the lowest package both the scheduler and BuildSuppressions import; +// the pinned cloud-commitments-go common package has no such mapping. func CanonicalSuppressionService(provider, service string) string { if provider != "aws" { return service @@ -795,6 +809,9 @@ func CanonicalSuppressionService(provider, service string) string { if canonical, ok := awsLegacyServiceSlugs[service]; ok { return canonical } + if canonical, ok := awsSavingsPlansSpellings[service]; ok { + return canonical + } return service } diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 16679db4b..17c57efca 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1341,23 +1341,28 @@ var legacyAWSServiceSlugs = map[string]common.ServiceType{ // flip the coverage_extra_test.go case (line ~60) to assert // ServiceType("savings-plans") (default-arm pass-through). The // "savingsplans-*" keys are forward-compat and should stay. +// +// A new per-plan-type spelling must also be added to +// config.awsSavingsPlansSpellings, else a suppression stored under it misses +// recs (#1422); a test ranges over savingsPlansSlugs to enforce that. func mapSavingsPlansSlug(service string) (common.ServiceType, bool) { - slugs := map[string]common.ServiceType{ - "savings-plans": common.ServiceSavingsPlansAll, - "savingsplans": common.ServiceSavingsPlansAll, - "savings-plans-compute": common.ServiceSavingsPlansCompute, - "savingsplans-compute": common.ServiceSavingsPlansCompute, - "savings-plans-ec2instance": common.ServiceSavingsPlansEC2Instance, - "savingsplans-ec2instance": common.ServiceSavingsPlansEC2Instance, - "savings-plans-sagemaker": common.ServiceSavingsPlansSageMaker, - "savingsplans-sagemaker": common.ServiceSavingsPlansSageMaker, - "savings-plans-database": common.ServiceSavingsPlansDatabase, - "savingsplans-database": common.ServiceSavingsPlansDatabase, - } - svc, ok := slugs[service] + svc, ok := savingsPlansSlugs[service] return svc, ok } +var savingsPlansSlugs = map[string]common.ServiceType{ + "savings-plans": common.ServiceSavingsPlansAll, + "savingsplans": common.ServiceSavingsPlansAll, + "savings-plans-compute": common.ServiceSavingsPlansCompute, + "savingsplans-compute": common.ServiceSavingsPlansCompute, + "savings-plans-ec2instance": common.ServiceSavingsPlansEC2Instance, + "savingsplans-ec2instance": common.ServiceSavingsPlansEC2Instance, + "savings-plans-sagemaker": common.ServiceSavingsPlansSageMaker, + "savingsplans-sagemaker": common.ServiceSavingsPlansSageMaker, + "savings-plans-database": common.ServiceSavingsPlansDatabase, + "savingsplans-database": common.ServiceSavingsPlansDatabase, +} + // updatePlanProgress advances the ramp schedule after a purchase. // Direct-execute purchases (Opportunities flow) have no plan to advance -- // PlanID is empty and the Postgres UUID column would reject the query diff --git a/internal/purchase/slug_alias_drift_test.go b/internal/purchase/slug_alias_drift_test.go index 739088705..b437836c6 100644 --- a/internal/purchase/slug_alias_drift_test.go +++ b/internal/purchase/slug_alias_drift_test.go @@ -42,3 +42,25 @@ func TestLegacyAWSSlugsMatchSuppressionAliases(t *testing.T) { }) } } + +// TestSavingsPlansSpellingsMatchSuppressionAliases checks that every per-type +// Savings Plans spelling the purchase manager accepts canonicalises, for +// suppressions, to the slug it buys, so a suppression stored under the +// dash-free spelling still hides the hyphenated rec (#1422). The umbrella +// spellings must stay umbrellas: folding one into a plan type would let one +// umbrella suppression hide every plan type's recs. +func TestSavingsPlansSpellingsMatchSuppressionAliases(t *testing.T) { + for _, pt := range common.SavingsPlansPlanTypes() { + require.Contains(t, savingsPlansSlugs, string(pt), "per-type slug %q was removed from savingsPlansSlugs", pt) + } + for key, want := range savingsPlansSlugs { + t.Run(key, func(t *testing.T) { + canonical := config.CanonicalSuppressionService("aws", key) + if want == common.ServiceSavingsPlansAll { + assert.Equal(t, key, canonical, "umbrella spelling %q must pass through unchanged", key) + return + } + assert.Equal(t, string(want), canonical, "spelling %q is missing from config.awsSavingsPlansSpellings", key) + }) + } +} diff --git a/internal/scheduler/scheduler_suppressions_test.go b/internal/scheduler/scheduler_suppressions_test.go index fa21add52..818ca9d70 100644 --- a/internal/scheduler/scheduler_suppressions_test.go +++ b/internal/scheduler/scheduler_suppressions_test.go @@ -359,7 +359,7 @@ func TestApplySuppressions_UnreadableRecDetailsMatchEveryRow(t *testing.T) { assert.Equal(t, 3, out[0].Count) } -// TestApplySuppressions_AWSLegacySlugSpellings covers issues #1412 and #1420: AWS +// TestApplySuppressions_AWSLegacySlugSpellings covers issues #1412, #1420 and #1422: AWS // recs and suppressions can be stored under a legacy or canonical slug, and a // suppression hides the rec whichever spelling each uses. Azure and GCP // "compute" stay distinct from "ec2". @@ -381,6 +381,11 @@ func TestApplySuppressions_AWSLegacySlugSpellings(t *testing.T) { {"aws legacy redshift sup hides data-warehouse rec", "aws", "data-warehouse", "redshift", 2}, {"aws data-warehouse sup hides legacy redshift rec", "aws", "redshift", "data-warehouse", 2}, {"aws memorydb sup does not hide cache rec", "aws", "cache", "memorydb", 5}, + {"aws dash-free compute SP sup hides compute SP rec", "aws", "savings-plans-compute", "savingsplans-compute", 2}, + {"aws compute SP sup hides dash-free compute SP rec", "aws", "savingsplans-compute", "savings-plans-compute", 2}, + {"aws dash-free database SP sup hides database SP rec", "aws", "savings-plans-database", "savingsplans-database", 2}, + {"aws umbrella SP sup does not hide compute SP rec", "aws", "savings-plans-compute", "savingsplans", 5}, + {"aws compute SP sup does not hide sagemaker SP rec", "aws", "savings-plans-sagemaker", "savingsplans-compute", 5}, {"azure legacy rds sup does not hide relational-db rec", "azure", "relational-db", "rds", 5}, {"azure compute sup hides compute rec", "azure", "compute", "compute", 2}, {"azure ec2 sup does not hide compute rec", "azure", "compute", "ec2", 5},