From 9b6e470e860cfead3f4a3c403d467e9002cc1e75 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 03:02:46 +0100 Subject: [PATCH 1/2] fix(config): canonicalise every AWS service alias in purchase suppressions Rec rows persisted before slug normalisation still carry the legacy AWS slugs rds, elasticache, opensearch and redshift (see mapServiceSlug in internal/purchase/execution.go). A retried legacy execution writes a suppression under the legacy slug, current recs use relational-db, cache, search and data-warehouse, so the suppression missed the rec and allowed a double buy. Same shape as #1412 (ec2). memorydb has no canonical slug and non-AWS providers pass through unchanged. Closes #1420 Co-Authored-By: Claude Opus 5.5 --- internal/config/suppressions_build_test.go | 32 +++++++++++++++---- internal/config/types.go | 28 ++++++++++++---- .../scheduler/scheduler_suppressions_test.go | 14 ++++++-- 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index b7433fd13..8008f9337 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -205,12 +205,32 @@ func TestBuildSuppressions_ComputeSlugEC2(t *testing.T) { } func TestCanonicalSuppressionService(t *testing.T) { - assert.Equal(t, "compute", CanonicalSuppressionService("aws", "ec2")) - assert.Equal(t, "compute", CanonicalSuppressionService("aws", "compute")) - assert.Equal(t, "rds", CanonicalSuppressionService("aws", "rds")) - assert.Equal(t, "compute", CanonicalSuppressionService("azure", "compute")) - assert.Equal(t, "ec2", CanonicalSuppressionService("azure", "ec2")) - assert.Equal(t, "compute", CanonicalSuppressionService("gcp", "compute")) + cases := []struct{ provider, service, want string }{ + {"aws", "ec2", "compute"}, + {"aws", "rds", "relational-db"}, + {"aws", "elasticache", "cache"}, + {"aws", "opensearch", "search"}, + {"aws", "redshift", "data-warehouse"}, + {"aws", "compute", "compute"}, + {"aws", "relational-db", "relational-db"}, + {"aws", "cache", "cache"}, + {"aws", "search", "search"}, + {"aws", "data-warehouse", "data-warehouse"}, + {"aws", "memorydb", "memorydb"}, + {"azure", "compute", "compute"}, + {"azure", "ec2", "ec2"}, + {"azure", "rds", "rds"}, + {"gcp", "compute", "compute"}, + {"gcp", "redshift", "redshift"}, + {"gcp", "elasticache", "elasticache"}, + } + for _, tc := range cases { + t.Run(tc.provider+"/"+tc.service, func(t *testing.T) { + got := CanonicalSuppressionService(tc.provider, tc.service) + assert.Equal(t, tc.want, got) + assert.Equal(t, got, CanonicalSuppressionService(tc.provider, got), "idempotent") + }) + } } // Issue #1412: AWS "ec2" and "compute" recs in one request share a bucket and diff --git a/internal/config/types.go b/internal/config/types.go index 9b8393f6f..13b89e799 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -760,15 +760,29 @@ type SuppressionVariant struct { Tenancy string `json:"tenancy"` } -// CanonicalSuppressionService maps the legacy AWS EC2 slug "ec2" to the -// canonical "compute" slug so a suppression and a rec match whichever spelling -// each was stored under (issue #1412). 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 +// awsLegacyServiceSlugs maps the AWS service slugs that rec rows persisted +// before slug normalisation still carry to their canonical spelling. +// memorydb has no canonical slug and is deliberately absent. +var awsLegacyServiceSlugs = map[string]string{ + "ec2": "compute", + "rds": "relational-db", + "elasticache": "cache", + "opensearch": "search", + "redshift": "data-warehouse", +} + +// 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. func CanonicalSuppressionService(provider, service string) string { - if provider == "aws" && service == "ec2" { - return "compute" + if provider != "aws" { + return service + } + if canonical, ok := awsLegacyServiceSlugs[service]; ok { + return canonical } return service } diff --git a/internal/scheduler/scheduler_suppressions_test.go b/internal/scheduler/scheduler_suppressions_test.go index 8f8a47c8a..d9d66d712 100644 --- a/internal/scheduler/scheduler_suppressions_test.go +++ b/internal/scheduler/scheduler_suppressions_test.go @@ -359,8 +359,8 @@ func TestApplySuppressions_UnreadableRecDetailsMatchEveryRow(t *testing.T) { assert.Equal(t, 3, out[0].Count) } -// TestApplySuppressions_EC2SlugSpellings is the issue #1412 regression: AWS -// EC2 recs and suppressions can be stored as "ec2" or "compute", and a +// TestApplySuppressions_EC2SlugSpellings covers issues #1412 and #1420: 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". func TestApplySuppressions_EC2SlugSpellings(t *testing.T) { @@ -372,6 +372,16 @@ func TestApplySuppressions_EC2SlugSpellings(t *testing.T) { {"aws ec2 sup hides compute rec", "aws", "compute", "ec2", 2}, {"aws same slug compute", "aws", "compute", "compute", 2}, {"aws unrelated service not hidden", "aws", "ec2", "rds", 5}, + {"aws legacy rds sup hides relational-db rec", "aws", "relational-db", "rds", 2}, + {"aws relational-db sup hides legacy rds rec", "aws", "rds", "relational-db", 2}, + {"aws legacy elasticache sup hides cache rec", "aws", "cache", "elasticache", 2}, + {"aws cache sup hides legacy elasticache rec", "aws", "elasticache", "cache", 2}, + {"aws legacy opensearch sup hides search rec", "aws", "search", "opensearch", 2}, + {"aws search sup hides legacy opensearch rec", "aws", "opensearch", "search", 2}, + {"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}, + {"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}, {"gcp compute sup does not hide ec2 rec", "gcp", "ec2", "compute", 5}, From 6bfbc199bdc66f64922526e9c3e200c68dc4894b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 03:12:36 +0100 Subject: [PATCH 2/2] test(config): cross-reference AWS alias tables and cover write-side alias merge The legacy alias list lives in two places (mapServiceSlug and config.awsLegacyServiceSlugs); comments now point each at the other so a new alias is not added to only one. Rename the scheduler slug test to reflect that it covers all AWS legacy slugs, and add a BuildSuppressions case proving an rds rec and a relational-db rec merge into one canonical row (#1420). Co-Authored-By: Claude Opus 5.5 --- internal/config/suppressions_build_test.go | 13 +++++++++++++ internal/config/types.go | 3 ++- internal/purchase/execution.go | 3 ++- internal/scheduler/scheduler_suppressions_test.go | 4 ++-- 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index 8008f9337..7c7d3d55c 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -233,6 +233,19 @@ func TestCanonicalSuppressionService(t *testing.T) { } } +// Issue #1420: a legacy "rds" rec and a canonical "relational-db" rec for the +// same tuple merge into one canonical suppression row with the summed count. +func TestBuildSuppressions_MergesRDSAliases(t *testing.T) { + gp := &GlobalConfig{GracePeriodDays: map[string]int{"aws": 7}} + sups := BuildSuppressions([]RecommendationRecord{ + {Provider: "aws", Service: "rds", Region: "r", ResourceType: "db.r5.large", Count: 3}, + {Provider: "aws", Service: "relational-db", Region: "r", ResourceType: "db.r5.large", Count: 2}, + }, "exec-1", gp, time.Now()) + require.Len(t, sups, 1) + assert.Equal(t, "relational-db", sups[0].Service) + assert.Equal(t, 5, sups[0].SuppressedCount) +} + // Issue #1412: AWS "ec2" and "compute" recs in one request share a bucket and // are written under the canonical slug; Azure compute is untouched. func TestBuildSuppressions_CanonicalisesAWSEC2Slug(t *testing.T) { diff --git a/internal/config/types.go b/internal/config/types.go index 13b89e799..504aa5840 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -762,7 +762,8 @@ type SuppressionVariant struct { // awsLegacyServiceSlugs maps the AWS service slugs that rec rows persisted // before slug normalisation still carry to their canonical spelling. -// memorydb has no canonical slug and is deliberately absent. +// memorydb has no canonical slug and is deliberately absent. Mirrors the legacy +// entries of mapServiceSlug in internal/purchase/execution.go. var awsLegacyServiceSlugs = map[string]string{ "ec2": "compute", "rds": "relational-db", diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 214d27643..30a8ff1d4 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1299,7 +1299,8 @@ func mapServiceSlug(service string) (common.ServiceType, bool) { "cache": common.ServiceCache, "search": common.ServiceSearch, "data-warehouse": common.ServiceDataWarehouse, - // Legacy AWS-only slugs. + // Legacy AWS-only slugs. A new legacy alias must also be added to + // config.awsLegacyServiceSlugs, else suppressions miss it (#1420). "ec2": common.ServiceEC2, "rds": common.ServiceRDS, "elasticache": common.ServiceElastiCache, diff --git a/internal/scheduler/scheduler_suppressions_test.go b/internal/scheduler/scheduler_suppressions_test.go index d9d66d712..67cbc3089 100644 --- a/internal/scheduler/scheduler_suppressions_test.go +++ b/internal/scheduler/scheduler_suppressions_test.go @@ -359,11 +359,11 @@ func TestApplySuppressions_UnreadableRecDetailsMatchEveryRow(t *testing.T) { assert.Equal(t, 3, out[0].Count) } -// TestApplySuppressions_EC2SlugSpellings covers issues #1412 and #1420: AWS +// TestApplySuppressions_AWSLegacySlugSpellings covers issues #1412 and #1420: 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". -func TestApplySuppressions_EC2SlugSpellings(t *testing.T) { +func TestApplySuppressions_AWSLegacySlugSpellings(t *testing.T) { cases := []struct { name, provider, recService, supService string wantCount int