diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index 150c9dac9..1c12e5961 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,17 +88,26 @@ 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" { + 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). +// 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"` @@ -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.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 2ce34e095..06533a00e 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -292,7 +292,43 @@ 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) { + 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 := 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 1b090d396..fcf632dbf 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -798,7 +798,11 @@ func (s *PurchaseSuppression) AppliesTo(v *SuppressionVariant) bool { return true } a, b := *s.Variant, *v - a.Tenancy, b.Tenancy = canonicalSuppressionTenancy(a.Tenancy), canonicalSuppressionTenancy(b.Tenancy) + // 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 = canonicalSuppressionTenancy(a.Tenancy) + } return a == b }