From 41de6061660f0e04b465ebca89cd3fe0d3bb1d04 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 18:19:30 +0100 Subject: [PATCH] fix(config): match purchase suppressions across AWS EC2 slugs AWS EC2 recs reach suppressions as "ec2" (scheduler-collected) or the canonical "compute" (purchase requests), and matching compared the exact service string, so a suppression under one spelling did not hide recs under the other. Normalise AWS ec2 to compute on both the suppression and rec side when building the scheduler match key, so existing rows of either spelling work without a migration, and write the canonical slug from BuildSuppressions. Azure and GCP compute are unchanged. Matching is in Go only; no SQL filters by suppression. Closes #1412 Co-Authored-By: Claude Opus 5.5 --- internal/config/suppressions_build.go | 5 +-- internal/config/suppressions_build_test.go | 26 +++++++++++ internal/config/types.go | 13 ++++++ internal/scheduler/scheduler.go | 10 +++-- .../scheduler/scheduler_suppressions_test.go | 34 ++++++++++++++ .../suppression_variant_integration_test.go | 44 +++++++++++++++++++ 6 files changed, 125 insertions(+), 7 deletions(-) diff --git a/internal/config/suppressions_build.go b/internal/config/suppressions_build.go index 2bf6e7c94..e595f9ffc 100644 --- a/internal/config/suppressions_build.go +++ b/internal/config/suppressions_build.go @@ -32,7 +32,7 @@ func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket { b := suppressionBucket{ accountID: accountID, provider: rec.Provider, - service: rec.Service, + service: CanonicalSuppressionService(rec.Provider, rec.Service), region: rec.Region, resourceType: rec.ResourceType, engine: rec.Engine, @@ -43,8 +43,7 @@ func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket { b.unknownVariant = true return b } - isAWSEC2 := rec.Provider == string(common.ProviderAWS) && - (rec.Service == string(common.ServiceEC2) || rec.Service == string(common.ServiceCompute)) + isAWSEC2 := b.provider == string(common.ProviderAWS) && b.service == string(common.ServiceCompute) if isAWSEC2 && v.Platform == "" && v.Tenancy == "" { // Saved before Details existed (#453) and retried without re-matching: // the bought platform/tenancy is unknown, so a known {az,"",""} would diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index e8c85f22e..b7433fd13 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -203,3 +203,29 @@ 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")) +} + +// 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) { + now := time.Date(2026, 4, 24, 0, 0, 0, 0, time.UTC) + gp := &GlobalConfig{GracePeriodDays: map[string]int{"aws": 7, "azure": 7}} + sups := BuildSuppressions([]RecommendationRecord{ + {Provider: "aws", Service: "ec2", Region: "r", ResourceType: "m5.large", Count: 3}, + {Provider: "aws", Service: "compute", Region: "r", ResourceType: "m5.large", Count: 2}, + {Provider: "azure", Service: "compute", Region: "r", ResourceType: "D2", Count: 1}, + }, "exec-1", gp, now) + require.Len(t, sups, 2) + assert.Equal(t, "compute", sups[0].Service) + assert.Equal(t, 5, sups[0].SuppressedCount) + assert.Equal(t, "compute", sups[1].Service) + assert.Equal(t, "azure", sups[1].Provider) +} diff --git a/internal/config/types.go b/internal/config/types.go index 4b43d6a0f..ab380e3ec 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -757,6 +757,19 @@ 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 +// cloud-commitments-go common package has no such mapping. +func CanonicalSuppressionService(provider, service string) string { + if provider == "aws" && service == "ec2" { + return "compute" + } + return service +} + // AppliesTo reports whether the suppression counts against a recommendation // on the same 6-tuple whose variant is v. A row without a variant (legacy) // applies to every variant, and a nil v (the rec's Details could not be read) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 282a8491f..f097f0ace 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1425,8 +1425,9 @@ func aggregateSuppressions(sups []config.PurchaseSuppression) map[suppressionKey for i := range sups { sup := sups[i] k := suppressionKey{ - accountID: sup.AccountID, provider: sup.Provider, service: sup.Service, - region: sup.Region, resourceType: sup.ResourceType, engine: sup.Engine, + accountID: sup.AccountID, provider: sup.Provider, + service: config.CanonicalSuppressionService(sup.Provider, sup.Service), + region: sup.Region, resourceType: sup.ResourceType, engine: sup.Engine, } index[k] = append(index[k], sup) } @@ -1470,8 +1471,9 @@ func applySuppressionIndex(recs []config.RecommendationRecord, index map[suppres accountID = *rec.CloudAccountID } k := suppressionKey{ - accountID: accountID, provider: rec.Provider, service: rec.Service, - region: rec.Region, resourceType: rec.ResourceType, engine: rec.Engine, + accountID: accountID, provider: rec.Provider, + service: config.CanonicalSuppressionService(rec.Provider, rec.Service), + region: rec.Region, resourceType: rec.ResourceType, engine: rec.Engine, } a := matchingSuppressions(&rec, index[k]) if a == nil { diff --git a/internal/scheduler/scheduler_suppressions_test.go b/internal/scheduler/scheduler_suppressions_test.go index 92d44d95c..8f8a47c8a 100644 --- a/internal/scheduler/scheduler_suppressions_test.go +++ b/internal/scheduler/scheduler_suppressions_test.go @@ -358,3 +358,37 @@ func TestApplySuppressions_UnreadableRecDetailsMatchEveryRow(t *testing.T) { require.Len(t, out, 1) 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 +// suppression hides the rec whichever spelling each uses. Azure and GCP +// "compute" stay distinct from "ec2". +func TestApplySuppressions_EC2SlugSpellings(t *testing.T) { + cases := []struct { + name, provider, recService, supService string + wantCount int + }{ + {"aws compute sup hides ec2 rec", "aws", "ec2", "compute", 2}, + {"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}, + {"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}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := &mockSuppressionStore{ + recs: []config.RecommendationRecord{{ID: "r1", Provider: tc.provider, Service: tc.recService, + Region: "r", ResourceType: "m5.large", Count: 5}}, + sups: []config.PurchaseSuppression{{ExecutionID: "e1", Provider: tc.provider, Service: tc.supService, + Region: "r", ResourceType: "m5.large", SuppressedCount: 3, ExpiresAt: time.Now().Add(time.Hour)}}, + } + s := &Scheduler{config: store} + recs, err := s.ListRecommendations(context.Background(), config.RecommendationFilter{}) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, tc.wantCount, recs[0].Count) + }) + } +} diff --git a/internal/scheduler/suppression_variant_integration_test.go b/internal/scheduler/suppression_variant_integration_test.go index 8de1f79f4..f406cae88 100644 --- a/internal/scheduler/suppression_variant_integration_test.go +++ b/internal/scheduler/suppression_variant_integration_test.go @@ -102,3 +102,47 @@ func TestLegacyEC2RetrySuppressesBoughtVariant_RealPostgres(t *testing.T) { require.NoError(t, err) assert.Empty(t, got, "the bought variant must be fully suppressed") } + +// TestSuppressionsMatchAcrossEC2Slugs_RealPostgres is the issue #1412 +// regression on a real database: an "ec2" rec is hidden by a "compute" +// suppression row and a "compute" rec by an "ec2" row (both spellings exist in +// stored data), while an Azure "compute" rec is not hidden by an AWS row. +func TestSuppressionsMatchAcrossEC2Slugs_RealPostgres(t *testing.T) { + ctx := t.Context() + pg := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(pg.DB) + + rec := func(id, provider, service, resourceType string) config.RecommendationRecord { + return config.RecommendationRecord{ + ID: id, Provider: provider, Service: service, Region: "us-east-1", ResourceType: resourceType, + Count: 5, Term: 1, Payment: "all-upfront", + Details: []byte(`{"availability_zone":"","platform":"","tenancy":""}`), + } + } + require.NoError(t, store.UpsertRecommendations(ctx, time.Now(), []config.RecommendationRecord{ + rec("ec2-rec", "aws", "ec2", "m5.large"), + rec("compute-rec", "aws", "compute", "c5.large"), + rec("azure-rec", "azure", "compute", "m5.large"), + }, nil)) + + for _, row := range []struct{ provider, service, resourceType string }{ + {"aws", "compute", "m5.large"}, + {"aws", "ec2", "c5.large"}, + } { + _, err := pg.DB.Pool().Exec(ctx, ` + INSERT INTO purchase_suppressions (execution_id, account_id, provider, service, region, + resource_type, engine, suppressed_count, expires_at) + VALUES ($1, '', $2, $3, 'us-east-1', $4, '', 2, NOW() + INTERVAL '1 day')`, + uuid.NewString(), row.provider, row.service, row.resourceType) + require.NoError(t, err) + } + + s := &Scheduler{config: store, isLambda: true} + got, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + counts := map[string]int{} + for _, r := range got { + counts[r.ID] = r.Count + } + assert.Equal(t, map[string]int{"ec2-rec": 3, "compute-rec": 3, "azure-rec": 5}, counts) +}