diff --git a/internal/config/suppressions_build.go b/internal/config/suppressions_build.go index 2bf6e7c9..e595f9ff 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 e8c85f22..b7433fd1 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 4b43d6a0..ab380e3e 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 282a8491..f097f0ac 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 92d44d95..8f8a47c8 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 8de1f79f..f406cae8 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) +}