Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions internal/config/suppressions_build.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down
26 changes: 26 additions & 0 deletions internal/config/suppressions_build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
13 changes: 13 additions & 0 deletions internal/config/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 6 additions & 4 deletions internal/scheduler/scheduler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down
34 changes: 34 additions & 0 deletions internal/scheduler/scheduler_suppressions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
44 changes: 44 additions & 0 deletions internal/scheduler/suppression_variant_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Loading