From eecf9bd807a0d2102e9953f3ef1d271074f09550 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 04:52:42 +0100 Subject: [PATCH 1/2] fix(config): scope suppression tenancy matching to AWS EC2 AppliesTo canonicalised tenancy "shared" to "default" for every provider and service, so a stored non-EC2 row (e.g. rds, azure compute) with "shared" matched a "default" variant. Limit it to AWS EC2 through the same isAWSEC2 predicate SuppressionVariant uses. detailsAvailabilityZone now trims like the other Details-AZ readers, a non-string zone yields "", and the stale SuppressionVariant doc comment is corrected. common.NormalizeTenancyName is not adopted: it maps unknown tenancies to "", which would make distinct unknown values match. Part of #1232 Co-Authored-By: Claude Opus 5.5 --- internal/config/recommendation_identity.go | 25 ++++++++++----- internal/config/suppressions_build_test.go | 36 ++++++++++++++++++++-- internal/config/types.go | 4 ++- 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index 150c9dac9..52fdaa39c 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -66,17 +66,19 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) { } // SuppressionVariant is the rec's availability zone, platform and tenancy as -// the purchase suppression key carries them (issue #816): the same AZ field and -// the same Details read (ComputeIdentity) as RecIdentityKey, so the write side -// (BuildSuppressions) and the read side (the scheduler's subtraction pass) -// cannot drift from the identity key or from each other. +// the purchase suppression key carries them (issue #816): the platform and +// tenancy come from the same Details read (ComputeIdentity) as RecIdentityKey, +// so the write side (BuildSuppressions) and the read side (the scheduler's +// subtraction pass) cannot drift from it or from each other. For AWS EC2 only, +// legacy tenancy "shared" is canonicalised to "default" and an AZ stored only +// in Details is used when the AZ field is empty (issue #1232). func (r *RecommendationRecord) SuppressionVariant() (SuppressionVariant, error) { ci, err := r.ComputeIdentity() if err != nil { return SuppressionVariant{}, err } v := SuppressionVariant{AvailabilityZone: r.AvailabilityZone, Platform: ci.Platform, Tenancy: ci.Tenancy} - if r.Provider == "aws" && CanonicalSuppressionService(r.Provider, r.Service) == "compute" { + if isAWSEC2(r.Provider, r.Service) { v.Tenancy = canonicalSuppressionTenancy(v.Tenancy) if v.AvailabilityZone == "" { // Rows saved 2026-10-09..12 hold the zone only in Details (#1232). @@ -86,8 +88,16 @@ func (r *RecommendationRecord) SuppressionVariant() (SuppressionVariant, error) return v, nil } +// isAWSEC2 is the scope of the legacy-shape canonicalisation: only AWS EC2 +// ever stored tenancy "shared" or the zone only in Details. +func isAWSEC2(provider, service string) bool { + return provider == "aws" && CanonicalSuppressionService(provider, service) == "compute" +} + // canonicalSuppressionTenancy maps the pre-2026-05-20 EC2 tenancy spelling // "shared" to today's "default" so old and new suppressions and recs match. +// common.NormalizeTenancyName is not used: it also case-folds and maps every +// unknown value to "", which would make distinct unknown tenancies match. func canonicalSuppressionTenancy(t string) string { if t == "shared" { return "default" @@ -96,7 +106,8 @@ func canonicalSuppressionTenancy(t string) string { } // detailsAvailabilityZone reads the zonal AZ from Details; "" when absent or -// unreadable (ComputeIdentity has already reported a decode error). +// unreadable (ComputeIdentity has already reported a decode error), trimmed +// like the other Details-AZ readers. A non-string zone yields "". func (r *RecommendationRecord) detailsAvailabilityZone() string { var d struct { AvailabilityZone string `json:"availability_zone"` @@ -104,5 +115,5 @@ func (r *RecommendationRecord) detailsAvailabilityZone() string { if json.Unmarshal(r.Details, &d) != nil { return "" } - return d.AvailabilityZone + return strings.TrimSpace(d.AvailabilityZone) } diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index 2ce34e095..cf43d69af 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -292,7 +292,37 @@ func TestBuildSuppressions_LegacyShapesCanonicalised(t *testing.T) { } 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"})) + def := &SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"} + variant := &SuppressionVariant{"us-east-1a", "Linux/UNIX", "shared"} + for name, tc := range map[string]struct { + provider, service string + want bool + }{ + "aws ec2 stored-side shared": {"aws", "ec2", true}, + "aws compute slug": {"aws", "compute", true}, + "aws rds untouched": {"aws", "rds", false}, + "azure compute untouched": {"azure", "compute", false}, + } { + t.Run(name, func(t *testing.T) { + stored := PurchaseSuppression{Provider: tc.provider, Service: tc.service, Variant: variant} + assert.Equal(t, tc.want, stored.AppliesTo(def)) + }) + } + ec2 := PurchaseSuppression{Provider: "aws", Service: "ec2", Variant: variant} + assert.False(t, ec2.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated"})) +} + +func TestRecommendationRecord_SuppressionVariant_DetailsAZ(t *testing.T) { + mk := func(details string) RecommendationRecord { + return RecommendationRecord{Provider: "aws", Service: "ec2", Details: []byte(details)} + } + for name, tc := range map[string]struct{ details, want string }{ + "padded AZ is trimmed": {`{"availability_zone":" us-east-1a "}`, "us-east-1a"}, + "non-string AZ is empty": {`{"availability_zone":5}`, ""}, + } { + t.Run(name, func(t *testing.T) { + rec := mk(tc.details) + assert.Equal(t, tc.want, rec.detailsAvailabilityZone()) + }) + } } diff --git a/internal/config/types.go b/internal/config/types.go index 1b090d396..bb69d13f8 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -798,7 +798,9 @@ func (s *PurchaseSuppression) AppliesTo(v *SuppressionVariant) bool { return true } a, b := *s.Variant, *v - a.Tenancy, b.Tenancy = canonicalSuppressionTenancy(a.Tenancy), canonicalSuppressionTenancy(b.Tenancy) + if isAWSEC2(s.Provider, s.Service) { + a.Tenancy, b.Tenancy = canonicalSuppressionTenancy(a.Tenancy), canonicalSuppressionTenancy(b.Tenancy) + } return a == b } From f1eb5441e932e0be62fbf37d288b755516f3cf4a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 04:59:00 +0100 Subject: [PATCH 2/2] refactor(config): tidy suppression tenancy and AZ helpers Reuse the package isAWSEC2 in suppressions_build.go instead of a shadowing local copy. AppliesTo canonicalises only the stored side, since SuppressionVariant already canonicalises every rec variant. Tenancy "shared" now matches case-insensitively after trimming. Correct the detailsAvailabilityZone comment (a non-string zone is treated as absent) and test the padded zone through SuppressionVariant. Part of #1232 Co-Authored-By: Claude Opus 5.5 --- internal/config/recommendation_identity.go | 6 +++--- internal/config/suppressions_build.go | 4 +--- internal/config/suppressions_build_test.go | 16 +++++++++++----- internal/config/types.go | 4 +++- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index 52fdaa39c..1c12e5961 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -99,15 +99,15 @@ func isAWSEC2(provider, service string) bool { // common.NormalizeTenancyName is not used: it also case-folds and maps every // unknown value to "", which would make distinct unknown tenancies match. func canonicalSuppressionTenancy(t string) string { - if t == "shared" { + if strings.EqualFold(strings.TrimSpace(t), "shared") { return "default" } return t } // detailsAvailabilityZone reads the zonal AZ from Details; "" when absent or -// unreadable (ComputeIdentity has already reported a decode error), trimmed -// like the other Details-AZ readers. A non-string zone yields "". +// unreadable, trimmed like the other Details-AZ readers. A non-string zone is +// treated as absent (regional), which can under-suppress; no producer writes one. func (r *RecommendationRecord) detailsAvailabilityZone() string { var d struct { AvailabilityZone string `json:"availability_zone"` diff --git a/internal/config/suppressions_build.go b/internal/config/suppressions_build.go index e595f9ffc..5051cb31c 100644 --- a/internal/config/suppressions_build.go +++ b/internal/config/suppressions_build.go @@ -3,7 +3,6 @@ package config import ( "time" - "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" ) @@ -43,8 +42,7 @@ func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket { b.unknownVariant = true return b } - isAWSEC2 := b.provider == string(common.ProviderAWS) && b.service == string(common.ServiceCompute) - if isAWSEC2 && v.Platform == "" && v.Tenancy == "" { + if isAWSEC2(b.provider, b.service) && 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 // leave the real variant unsuppressed (double-buy). Wildcard instead. diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index cf43d69af..06533a00e 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -313,16 +313,22 @@ func TestPurchaseSuppression_AppliesTo_LegacySharedTenancy(t *testing.T) { } func TestRecommendationRecord_SuppressionVariant_DetailsAZ(t *testing.T) { - mk := func(details string) RecommendationRecord { - return RecommendationRecord{Provider: "aws", Service: "ec2", Details: []byte(details)} - } for name, tc := range map[string]struct{ details, want string }{ "padded AZ is trimmed": {`{"availability_zone":" us-east-1a "}`, "us-east-1a"}, "non-string AZ is empty": {`{"availability_zone":5}`, ""}, } { t.Run(name, func(t *testing.T) { - rec := mk(tc.details) - assert.Equal(t, tc.want, rec.detailsAvailabilityZone()) + rec := RecommendationRecord{Provider: "aws", Service: "ec2", Details: []byte(tc.details)} + v, err := rec.SuppressionVariant() + require.NoError(t, err) + assert.Equal(t, tc.want, v.AvailabilityZone) }) } } + +func TestCanonicalSuppressionTenancy(t *testing.T) { + assert.Equal(t, "default", canonicalSuppressionTenancy(" Shared ")) + assert.Equal(t, "default", canonicalSuppressionTenancy("shared")) + assert.Equal(t, "Dedicated", canonicalSuppressionTenancy("Dedicated")) + assert.Equal(t, "default", canonicalSuppressionTenancy("default")) +} diff --git a/internal/config/types.go b/internal/config/types.go index bb69d13f8..fcf632dbf 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -798,8 +798,10 @@ func (s *PurchaseSuppression) AppliesTo(v *SuppressionVariant) bool { return true } a, b := *s.Variant, *v + // Only the stored side needs canonicalising: SuppressionVariant already + // canonicalises every rec variant, so a raw "shared" never reaches v. if isAWSEC2(s.Provider, s.Service) { - a.Tenancy, b.Tenancy = canonicalSuppressionTenancy(a.Tenancy), canonicalSuppressionTenancy(b.Tenancy) + a.Tenancy = canonicalSuppressionTenancy(a.Tenancy) } return a == b }