From f66947ffc118111d12644b273f26753412922ef0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 03:51:26 +0100 Subject: [PATCH] fix(config): match legacy EC2 tenancy and AZ shapes in purchase suppressions Part of #1232. Two legacy AWS EC2 shapes produced suppression variants that never matched the current rec, so a retried purchase could double-buy. Details tenancy "shared" (pre-2026-05-20) is now canonicalised to "default" in SuppressionVariant (write side and rec side) and in AppliesTo (so already stored "shared" rows match). A rec whose AZ field is empty but whose Details carry an AZ now uses the Details AZ; the field still wins when both are set. Non-EC2 services are unchanged. Co-Authored-By: Claude Opus 5.5 --- internal/config/recommendation_identity.go | 31 +++++++++++++++- internal/config/suppressions_build_test.go | 34 +++++++++++++++++ internal/config/types.go | 7 +++- .../scheduler/scheduler_suppressions_test.go | 37 +++++++++++++++++++ 4 files changed, 107 insertions(+), 2 deletions(-) diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index cc42e0372..150c9dac9 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -75,5 +75,34 @@ func (r *RecommendationRecord) SuppressionVariant() (SuppressionVariant, error) if err != nil { return SuppressionVariant{}, err } - return SuppressionVariant{AvailabilityZone: r.AvailabilityZone, Platform: ci.Platform, Tenancy: ci.Tenancy}, nil + v := SuppressionVariant{AvailabilityZone: r.AvailabilityZone, Platform: ci.Platform, Tenancy: ci.Tenancy} + if r.Provider == "aws" && CanonicalSuppressionService(r.Provider, r.Service) == "compute" { + v.Tenancy = canonicalSuppressionTenancy(v.Tenancy) + if v.AvailabilityZone == "" { + // Rows saved 2026-10-09..12 hold the zone only in Details (#1232). + v.AvailabilityZone = r.detailsAvailabilityZone() + } + } + return v, nil +} + +// canonicalSuppressionTenancy maps the pre-2026-05-20 EC2 tenancy spelling +// "shared" to today's "default" so old and new suppressions and recs match. +func canonicalSuppressionTenancy(t string) string { + if t == "shared" { + return "default" + } + return t +} + +// detailsAvailabilityZone reads the zonal AZ from Details; "" when absent or +// unreadable (ComputeIdentity has already reported a decode error). +func (r *RecommendationRecord) detailsAvailabilityZone() string { + var d struct { + AvailabilityZone string `json:"availability_zone"` + } + if json.Unmarshal(r.Details, &d) != nil { + return "" + } + return d.AvailabilityZone } diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index 7c7d3d55c..2ce34e095 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -262,3 +262,37 @@ func TestBuildSuppressions_CanonicalisesAWSEC2Slug(t *testing.T) { assert.Equal(t, "compute", sups[1].Service) assert.Equal(t, "azure", sups[1].Provider) } + +// TestBuildSuppressions_LegacyShapesCanonicalised pins the two legacy EC2 +// shapes of issue #1232: Details tenancy "shared" (pre-2026-05-20) is the same +// capacity as "default", and an AZ stored only in Details is used when the AZ +// field is empty (the field wins when both are set). +func TestBuildSuppressions_LegacyShapesCanonicalised(t *testing.T) { + mk := func(service, field, details string) RecommendationRecord { + return RecommendationRecord{Provider: "aws", Service: service, Region: "us-east-1", ResourceType: "m5.large", + Count: 1, AvailabilityZone: field, Details: []byte(details)} + } + for name, tc := range map[string]struct { + rec RecommendationRecord + want SuppressionVariant + }{ + "legacy shared tenancy": {mk("ec2", "us-east-1a", `{"platform":"Linux/UNIX","tenancy":"shared"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}}, + "compute slug shared": {mk("compute", "", `{"platform":"Linux/UNIX","tenancy":"shared"}`), SuppressionVariant{"", "Linux/UNIX", "default"}}, + "AZ only in details": {mk("ec2", "", `{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"default"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}}, + "field wins over details": {mk("ec2", "us-east-1b", `{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"default"}`), SuppressionVariant{"us-east-1b", "Linux/UNIX", "default"}}, + "modern rec unchanged": {mk("ec2", "us-east-1a", `{"availability_zone":"us-east-1a","platform":"Linux/UNIX","tenancy":"dedicated"}`), SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated"}}, + "non-EC2 untouched": {RecommendationRecord{Provider: "aws", Service: "rds", Region: "r", ResourceType: "db", Count: 1, Details: []byte(`{"availability_zone":"us-east-1a","tenancy":"shared"}`)}, SuppressionVariant{"", "", "shared"}}, + } { + t.Run(name, func(t *testing.T) { + sups := BuildSuppressions([]RecommendationRecord{tc.rec}, "exec-1", &GlobalConfig{}, time.Now()) + require.Len(t, sups, 1) + assert.Equal(t, &tc.want, sups[0].Variant) + }) + } +} + +func TestPurchaseSuppression_AppliesTo_LegacySharedTenancy(t *testing.T) { + stored := PurchaseSuppression{Variant: &SuppressionVariant{"us-east-1a", "Linux/UNIX", "shared"}} + assert.True(t, stored.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}), "old stored shared row must match a default rec") + assert.False(t, stored.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated"})) +} diff --git a/internal/config/types.go b/internal/config/types.go index 504aa5840..1b090d396 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -794,7 +794,12 @@ func CanonicalSuppressionService(provider, service string) string { // matches every row: both fall back to the pre-#816 6-tuple match, which // over-suppresses (under-buys) rather than over-shows. func (s *PurchaseSuppression) AppliesTo(v *SuppressionVariant) bool { - return s.Variant == nil || v == nil || *s.Variant == *v + if s.Variant == nil || v == nil { + return true + } + a, b := *s.Variant, *v + a.Tenancy, b.Tenancy = canonicalSuppressionTenancy(a.Tenancy), canonicalSuppressionTenancy(b.Tenancy) + return a == b } // RecommendationFilter parameterises ListStoredRecommendations and the diff --git a/internal/scheduler/scheduler_suppressions_test.go b/internal/scheduler/scheduler_suppressions_test.go index 67cbc3089..fa21add52 100644 --- a/internal/scheduler/scheduler_suppressions_test.go +++ b/internal/scheduler/scheduler_suppressions_test.go @@ -402,3 +402,40 @@ func TestApplySuppressions_AWSLegacySlugSpellings(t *testing.T) { }) } } + +// TestApplySuppressions_LegacyEC2Shapes covers the two legacy shapes of issue +// #1232 end to end: a suppression stored with tenancy "shared", or built from a +// rec whose AZ lives only in Details, must still hide the modern rec. +func TestApplySuppressions_LegacyEC2Shapes(t *testing.T) { + modern := ec2VariantRec("modern", "us-east-1a", "Linux/UNIX", "default") + azOnlyInDetails := modern + azOnlyInDetails.AvailabilityZone = "" + sharedPurchase := ec2VariantRec("p", "us-east-1a", "Linux/UNIX", "shared") + exp := time.Now().Add(time.Hour) + for name, tc := range map[string]struct { + rec config.RecommendationRecord + sups []config.PurchaseSuppression + want int + }{ + "stored shared row vs default rec": {modern, []config.PurchaseSuppression{{ExecutionID: "e", AccountID: "acct-1", Provider: "aws", Service: "compute", Region: "us-east-1", + ResourceType: "m5.large", SuppressedCount: 2, ExpiresAt: exp, Variant: &config.SuppressionVariant{AvailabilityZone: "us-east-1a", Platform: "Linux/UNIX", Tenancy: "shared"}}}, 3}, + "built from shared rec": {modern, config.BuildSuppressions([]config.RecommendationRecord{withCount(sharedPurchase, 2)}, "e", + &config.GlobalConfig{GracePeriodDays: map[string]int{"aws": 7}}, time.Now()), 3}, + "rec AZ only in details, row has AZ": {azOnlyInDetails, config.BuildSuppressions([]config.RecommendationRecord{withCount(modern, 2)}, "e", + &config.GlobalConfig{GracePeriodDays: map[string]int{"aws": 7}}, time.Now()), 3}, + "purchase AZ only in details": {modern, config.BuildSuppressions([]config.RecommendationRecord{withCount(azOnlyInDetails, 2)}, "e", + &config.GlobalConfig{GracePeriodDays: map[string]int{"aws": 7}}, time.Now()), 3}, + } { + t.Run(name, func(t *testing.T) { + index := aggregateSuppressions(tc.sups) + out := applySuppressionIndex([]config.RecommendationRecord{tc.rec}, index) + require.Len(t, out, 1) + assert.Equal(t, tc.want, out[0].Count) + }) + } +} + +func withCount(r config.RecommendationRecord, n int) config.RecommendationRecord { + r.Count = n + return r +}