From 32881eb6ac18f1cf9a34beefda4f88d97970b53b Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 08:59:53 +0000 Subject: [PATCH] fix(config): canonicalise dash-free Savings Plans spellings in suppressions The purchase manager's mapSavingsPlansSlug accepts both the hyphenated per-type Savings Plans slugs (savings-plans-compute) and the dash-free ones (savingsplans-compute), and the purchase API stores the service a client sends verbatim. A direct API client that bought with the dash-free spelling therefore wrote a suppression the scheduler compared literally against the hyphenated rec, so the rec it had just bought stayed on show and could be bought again. The UI only sends the hyphenated form, so this is reachable only through direct API clients. CanonicalSuppressionService now maps the four dash-free per-type spellings to the library constants. The "savingsplans" umbrella is left alone so one umbrella suppression can never hide every plan type, and non-AWS providers (Azure's "savingsplans" service) pass through. The Savings Plans slug map moves to a package var so a drift test can range over it and check each per-type spelling against the config alias table, as the legacy AWS slug test already does. Co-Authored-By: Claude Opus 5.5 --- internal/config/suppressions_build_test.go | 8 +++++ internal/config/types.go | 29 +++++++++++++---- internal/purchase/execution.go | 31 +++++++++++-------- internal/purchase/slug_alias_drift_test.go | 22 +++++++++++++ .../scheduler/scheduler_suppressions_test.go | 7 ++++- 5 files changed, 77 insertions(+), 20 deletions(-) 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},