From c99bd7fb4a20c71eef6a857872211b900ddebc38 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 22:29:56 +0100 Subject: [PATCH 1/3] fix(gcp): fail closed on region-less storage SKUs and ambiguous SKU matches Cloud Storage's skuMatchesStorageClass still treated a nil ServiceRegions as "every region" while an empty slice matched none. It now uses the shared skumatch.InRegion rule: named regions match only themselves, and a SKU with no regions matches only when GeoTaxonomy.Type is GLOBAL. The Cloud SQL, Memorystore and Cloud Storage extractors kept whichever matching SKU came last for each price slot, so catalog order decided the price. They now collect through skumatch.SlotPrices, which accepts agreeing duplicates and returns an "ambiguous catalog" error when two matching SKUs feed the same slot at different prices, the same rule computeengine's rateSlot already applies. Closes #97 Co-Authored-By: Claude Opus 5.5 --- providers/gcp/internal/skumatch/skumatch.go | 5 +- providers/gcp/internal/skumatch/slotprices.go | 40 +++++++++ .../gcp/internal/skumatch/slotprices_test.go | 53 +++++++++++ .../gcp/services/cloudsql/ambiguity_test.go | 72 +++++++++++++++ providers/gcp/services/cloudsql/client.go | 10 +-- .../services/cloudstorage/ambiguity_test.go | 88 +++++++++++++++++++ providers/gcp/services/cloudstorage/client.go | 24 ++--- .../gcp/services/cloudstorage/client_test.go | 65 +++++++++++++- .../services/memorystore/ambiguity_test.go | 72 +++++++++++++++ providers/gcp/services/memorystore/client.go | 10 +-- 10 files changed, 408 insertions(+), 31 deletions(-) create mode 100644 providers/gcp/internal/skumatch/slotprices.go create mode 100644 providers/gcp/internal/skumatch/slotprices_test.go create mode 100644 providers/gcp/services/cloudsql/ambiguity_test.go create mode 100644 providers/gcp/services/cloudstorage/ambiguity_test.go create mode 100644 providers/gcp/services/memorystore/ambiguity_test.go diff --git a/providers/gcp/internal/skumatch/skumatch.go b/providers/gcp/internal/skumatch/skumatch.go index c16abdbf..09fdcda2 100644 --- a/providers/gcp/internal/skumatch/skumatch.go +++ b/providers/gcp/internal/skumatch/skumatch.go @@ -81,8 +81,9 @@ const ( // included) is an error rather than a guess, because a wrong slot silently // mislabels the commitment price. // -// Known gap: Commit1Yr and Commit3Yr share SlotCommitment, so callers that see -// both terms keep whichever SKU comes last. +// Known gap: Commit1Yr and Commit3Yr share SlotCommitment (#254). Callers +// collect through SlotPrices, so a 1Yr and a 3Yr SKU at different prices make +// the lookup fail as ambiguous instead of keeping whichever comes last. func Slot(sku *cloudbilling.Sku) (PriceSlot, error) { if sku.Category == nil { return 0, fmt.Errorf("sku %s has no category, cannot tell commitment from on-demand", sku.SkuId) diff --git a/providers/gcp/internal/skumatch/slotprices.go b/providers/gcp/internal/skumatch/slotprices.go new file mode 100644 index 00000000..92c5b720 --- /dev/null +++ b/providers/gcp/internal/skumatch/slotprices.go @@ -0,0 +1,40 @@ +package skumatch + +import ( + "fmt" + + "google.golang.org/api/cloudbilling/v1" +) + +// SlotPrices collects at most one price per slot from the SKUs that matched a +// lookup. The catalog order carries no meaning, so a later SKU must not +// silently replace an earlier one: two matching SKUs that feed the same slot +// at different prices make the lookup ambiguous and Add returns an error. +// SKUs that repeat the same price agree and are accepted. +type SlotPrices struct { + OnDemand float64 + Commitment float64 + + onDemandSKU string + commitmentSKU string +} + +// Add records price for sku in slot. SlotNone is ignored. +func (p *SlotPrices) Add(slot PriceSlot, sku *cloudbilling.Sku, price float64) error { + switch slot { + case SlotOnDemand: + return setOnce(&p.OnDemand, &p.onDemandSKU, sku, price, "on-demand") + case SlotCommitment: + return setOnce(&p.Commitment, &p.commitmentSKU, sku, price, "commitment") + } + return nil +} + +func setOnce(value *float64, skuID *string, sku *cloudbilling.Sku, price float64, slotName string) error { + if *skuID != "" && *value != price { + return fmt.Errorf("ambiguous catalog: %s SKU %s (%q) prices %v but matching SKU %s prices %v", + slotName, sku.SkuId, sku.Description, price, *skuID, *value) + } + *value, *skuID = price, sku.SkuId + return nil +} diff --git a/providers/gcp/internal/skumatch/slotprices_test.go b/providers/gcp/internal/skumatch/slotprices_test.go new file mode 100644 index 00000000..2dd2be18 --- /dev/null +++ b/providers/gcp/internal/skumatch/slotprices_test.go @@ -0,0 +1,53 @@ +package skumatch + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +func TestSlotPrices_Add(t *testing.T) { + a := &cloudbilling.Sku{SkuId: "A", Description: "first"} + b := &cloudbilling.Sku{SkuId: "B", Description: "second"} + type add struct { + slot PriceSlot + sku *cloudbilling.Sku + price float64 + } + tests := []struct { + name string + adds []add + wantOnDemand float64 + wantCommitment float64 + wantErr string + }{ + {name: "one price per slot", adds: []add{{SlotOnDemand, a, 0.05}, {SlotCommitment, b, 0.04}}, wantOnDemand: 0.05, wantCommitment: 0.04}, + {name: "agreeing duplicates are accepted", adds: []add{{SlotOnDemand, a, 0.05}, {SlotOnDemand, b, 0.05}}, wantOnDemand: 0.05}, + {name: "conflicting on-demand is an error", adds: []add{{SlotOnDemand, a, 0.05}, {SlotOnDemand, b, 0.8}}, wantErr: "ambiguous catalog: on-demand SKU B"}, + {name: "conflicting on-demand in reverse order is an error", adds: []add{{SlotOnDemand, b, 0.8}, {SlotOnDemand, a, 0.05}}, wantErr: "ambiguous catalog: on-demand SKU A"}, + {name: "conflicting commitment is an error", adds: []add{{SlotCommitment, a, 0.04}, {SlotCommitment, b, 0.03}}, wantErr: "ambiguous catalog: commitment SKU B"}, + {name: "SlotNone is ignored", adds: []add{{SlotOnDemand, a, 0.05}, {SlotNone, b, 0.01}}, wantOnDemand: 0.05}, + {name: "slots do not interfere", adds: []add{{SlotOnDemand, a, 0.05}, {SlotCommitment, a, 0.04}, {SlotCommitment, b, 0.04}}, wantOnDemand: 0.05, wantCommitment: 0.04}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var p SlotPrices + var err error + for _, ad := range tt.adds { + if err = p.Add(ad.slot, ad.sku, ad.price); err != nil { + break + } + } + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantOnDemand, p.OnDemand) + assert.Equal(t, tt.wantCommitment, p.Commitment) + }) + } +} diff --git a/providers/gcp/services/cloudsql/ambiguity_test.go b/providers/gcp/services/cloudsql/ambiguity_test.go new file mode 100644 index 00000000..79c0ac42 --- /dev/null +++ b/providers/gcp/services/cloudsql/ambiguity_test.go @@ -0,0 +1,72 @@ +package cloudsql + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// TestExtractPricing_AmbiguousMatches covers issue #97: the extractor used to +// keep whichever matching SKU came last, so catalog order decided the price. +func TestExtractPricing_AmbiguousMatches(t *testing.T) { + const tier, decoy, region = "db-n1-standard-1", "db-n1-standard-16", "europe-west4" + od := func(id, tierName string, nanos int64) *cloudbilling.Sku { + return currencySKU(id, "Cloud SQL for MySQL "+tierName+" in "+region, "OnDemand", region, "USD", nanos) + } + cud := func(id, usage string, nanos int64) *cloudbilling.Sku { + return currencySKU(id, "Cloud SQL for MySQL "+tier+" commitment in "+region, usage, region, "USD", nanos) + } + tests := []struct { + name string + skus []*cloudbilling.Sku + wantOnDemand float64 + wantCommitment float64 + wantErr string + }{ + { + name: "superstring tier SKU after the real one is ignored", + skus: []*cloudbilling.Sku{od("od", tier, 50000000), od("od-decoy", decoy, 800000000), cud("cud", "Commit1Yr", 40000000)}, + wantOnDemand: 0.05, wantCommitment: 0.04, + }, + { + name: "superstring tier SKU before the real one is ignored", + skus: []*cloudbilling.Sku{od("od-decoy", decoy, 800000000), od("od", tier, 50000000), cud("cud", "Commit1Yr", 40000000)}, + wantOnDemand: 0.05, wantCommitment: 0.04, + }, + { + name: "duplicate on-demand SKUs at the same price agree", + skus: []*cloudbilling.Sku{od("od-a", tier, 50000000), od("od-b", tier, 50000000), cud("cud", "Commit1Yr", 40000000)}, + wantOnDemand: 0.05, wantCommitment: 0.04, + }, + { + name: "two on-demand SKUs at different prices are ambiguous", + skus: []*cloudbilling.Sku{od("od-a", tier, 50000000), od("od-b", tier, 60000000), cud("cud", "Commit1Yr", 40000000)}, + wantErr: "ambiguous catalog: on-demand SKU od-b", + }, + { + name: "same conflict in reverse order is still ambiguous", + skus: []*cloudbilling.Sku{od("od-b", tier, 60000000), od("od-a", tier, 50000000), cud("cud", "Commit1Yr", 40000000)}, + wantErr: "ambiguous catalog: on-demand SKU od-a", + }, + { + name: "1Yr and 3Yr commitment SKUs at different prices are ambiguous", + skus: []*cloudbilling.Sku{od("od", tier, 50000000), cud("cud-1y", "Commit1Yr", 40000000), cud("cud-3y", "Commit3Yr", 30000000)}, + wantErr: "ambiguous catalog: commitment SKU cud-3y", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + onDemand, commitment, _, err := extractSQLPricingFromSKUs(tt.skus, tier, region) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.InDelta(t, tt.wantOnDemand, onDemand, 1e-12) + assert.InDelta(t, tt.wantCommitment, commitment, 1e-12) + }) + } +} diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index d80ae5f1..b8f07204 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -422,6 +422,7 @@ func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, // extractSQLPricingFromSKUs extracts on-demand and commitment pricing from the SKU list. // Cloud SQL committed-use discounts are SKUs whose Category.UsageType is Commit1Yr or Commit3Yr. func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (onDemand, commitment float64, currency string, err error) { + var prices skumatch.SlotPrices for _, sku := range skus { if !skuMatchesTier(sku, tier, region) { continue @@ -441,15 +442,12 @@ func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (o if slotErr != nil { return 0, 0, "", slotErr } - switch slot { - case skumatch.SlotCommitment: - commitment = price - case skumatch.SlotOnDemand: - onDemand = price + if err := prices.Add(slot, sku, price); err != nil { + return 0, 0, "", err } } - return onDemand, commitment, currency, nil + return prices.OnDemand, prices.Commitment, currency, nil } // extractSQLPriceFromSKU extracts the unit price from a SKU. diff --git a/providers/gcp/services/cloudstorage/ambiguity_test.go b/providers/gcp/services/cloudstorage/ambiguity_test.go new file mode 100644 index 00000000..ade347af --- /dev/null +++ b/providers/gcp/services/cloudstorage/ambiguity_test.go @@ -0,0 +1,88 @@ +package cloudstorage + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +func storageSKU(id, description, usageType string, regions []string, nanos int64) *cloudbilling.Sku { + return &cloudbilling.Sku{ + SkuId: id, + Description: description, + Category: &cloudbilling.Category{UsageType: usageType}, + ServiceRegions: regions, + PricingInfo: []*cloudbilling.PricingInfo{{ + PricingExpression: &cloudbilling.PricingExpression{ + UsageUnit: "GiBy.mo", + TieredRates: []*cloudbilling.TierRate{{ + UnitPrice: &cloudbilling.Money{CurrencyCode: "USD", Nanos: nanos}, + }}, + }, + }}, + } +} + +// TestExtractStoragePricing_RegionAndAmbiguity covers issue #97: a SKU with no +// region list used to price every region, and the last matching SKU won. +func TestExtractStoragePricing_RegionAndAmbiguity(t *testing.T) { + const class, region = "STANDARD", "europe-west4" + here := []string{region} + tests := []struct { + name string + skus []*cloudbilling.Sku + wantOnDemand float64 + wantCommitment float64 + wantErr string + }{ + { + name: "region-less SKU of unknown scope does not override the regional price", + skus: []*cloudbilling.Sku{ + storageSKU("od", "Standard Storage Netherlands", "OnDemand", here, 20000000), + storageSKU("od-nil", "Standard Storage Taiwan", "OnDemand", nil, 30000000), + storageSKU("cud", "Standard Storage Netherlands commitment", "Commit1Yr", here, 15000000), + storageSKU("cud-nil", "Standard Storage Taiwan commitment", "Commit1Yr", nil, 25000000), + }, + wantOnDemand: 0.02 / averageHoursPerMonth, + wantCommitment: 0.015 / averageHoursPerMonth, + }, + { + name: "region-less SKU alone yields no price", + skus: []*cloudbilling.Sku{ + storageSKU("od-nil", "Standard Storage Taiwan", "OnDemand", nil, 30000000), + }, + wantOnDemand: 0, + }, + { + name: "two regional SKUs at different prices are ambiguous", + skus: []*cloudbilling.Sku{ + storageSKU("od-a", "Standard Storage Netherlands", "OnDemand", here, 20000000), + storageSKU("od-b", "Standard Storage Netherlands", "OnDemand", here, 23000000), + }, + wantErr: "ambiguous catalog: on-demand SKU od-b", + }, + { + name: "two regional SKUs at the same price agree", + skus: []*cloudbilling.Sku{ + storageSKU("od-a", "Standard Storage Netherlands", "OnDemand", here, 20000000), + storageSKU("od-b", "Standard Storage Netherlands", "OnDemand", here, 20000000), + }, + wantOnDemand: 0.02 / averageHoursPerMonth, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + onDemand, commitment, _, err := extractStoragePricingFromSKUs(tt.skus, class, region) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.InDelta(t, tt.wantOnDemand, onDemand, 1e-15) + assert.InDelta(t, tt.wantCommitment, commitment, 1e-15) + }) + } +} diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 43e95b54..582551cb 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -280,6 +280,7 @@ func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, // extractStoragePricingFromSKUs extracts on-demand and commitment pricing from SKU list. func extractStoragePricingFromSKUs(skus []*cloudbilling.Sku, storageClass, region string) (onDemand, commitment float64, currency string, err error) { + var prices skumatch.SlotPrices currency = "USD" for _, sku := range skus { @@ -303,15 +304,12 @@ func extractStoragePricingFromSKUs(skus []*cloudbilling.Sku, storageClass, regio if slotErr != nil { return 0, 0, "", slotErr } - switch slot { - case skumatch.SlotCommitment: - commitment = price - case skumatch.SlotOnDemand: - onDemand = price + if err := prices.Add(slot, sku, price); err != nil { + return 0, 0, "", err } } - return onDemand, commitment, currency, nil + return prices.OnDemand, prices.Commitment, currency, nil } // extractStoragePriceFromSKU returns a per-GiB-hour rate regardless of the catalog unit. @@ -347,20 +345,14 @@ func calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPri return ((onDemandTotal - commitmentPrice) / onDemandTotal) * 100 } +// skuMatchesStorageClass checks that the SKU is the capacity SKU of storageClass +// (description anchored on " storage ", early-delete SKUs excluded) and +// that it applies in region under the shared skumatch.InRegion rule. func skuMatchesStorageClass(sku *cloudbilling.Sku, storageClass, region string) bool { description := strings.ToLower(sku.Description) if !strings.HasPrefix(description, strings.ToLower(storageClass)+" storage ") || strings.Contains(description, "(early delete)") { return false } - if sku.ServiceRegions != nil { - for _, serviceRegion := range sku.ServiceRegions { - if strings.EqualFold(serviceRegion, region) { - return true - } - } - return false - } - - return true + return skumatch.InRegion(sku, region) } diff --git a/providers/gcp/services/cloudstorage/client_test.go b/providers/gcp/services/cloudstorage/client_test.go index 2a48129f..bbedb2fa 100644 --- a/providers/gcp/services/cloudstorage/client_test.go +++ b/providers/gcp/services/cloudstorage/client_test.go @@ -190,6 +190,7 @@ func TestSkuMatchesStorageClass(t *testing.T) { storageClass string region string regions []string + geoType string expected bool }{ { @@ -217,13 +218,72 @@ func TestSkuMatchesStorageClass(t *testing.T) { expected: false, }, { - name: "No regions specified - matches description only", + name: "nil regions without taxonomy match no region", description: "Standard Storage multi-region", storageClass: "Standard", region: "us-central1", regions: nil, + expected: false, + }, + { + name: "empty regions without taxonomy match no region", + description: "Standard Storage multi-region", + storageClass: "Standard", + region: "us-central1", + regions: []string{}, + expected: false, + }, + { + name: "nil regions with GLOBAL taxonomy match every region", + description: "Standard Storage global", + storageClass: "Standard", + region: "us-central1", + regions: nil, + geoType: "GLOBAL", + expected: true, + }, + { + name: "empty regions with GLOBAL taxonomy match every region", + description: "Standard Storage global", + storageClass: "Standard", + region: "us-central1", + regions: []string{}, + geoType: "GLOBAL", + expected: true, + }, + { + name: "nil regions with REGIONAL taxonomy match no region", + description: "Standard Storage multi-region", + storageClass: "Standard", + region: "us-central1", + regions: nil, + geoType: "REGIONAL", + expected: false, + }, + { + name: "region match is case-insensitive", + description: "Standard Storage Iowa", + storageClass: "STANDARD", + region: "US-CENTRAL1", + regions: []string{"us-central1"}, expected: true, }, + { + name: "class must be a whole leading word, not a prefix of a longer class", + description: "Standard-HA Storage Iowa", + storageClass: "Standard", + region: "us-central1", + regions: []string{"us-central1"}, + expected: false, + }, + { + name: "class inside a longer class name does not match", + description: "Multi-Regional Storage US", + storageClass: "Regional", + region: "us", + regions: []string{"us"}, + expected: false, + }, } for _, tt := range tests { @@ -233,6 +293,9 @@ func TestSkuMatchesStorageClass(t *testing.T) { Category: &cloudbilling.Category{UsageType: "OnDemand"}, ServiceRegions: tt.regions, } + if tt.geoType != "" { + sku.GeoTaxonomy = &cloudbilling.GeoTaxonomy{Type: tt.geoType} + } result := skuMatchesStorageClass(sku, tt.storageClass, tt.region) assert.Equal(t, tt.expected, result) }) diff --git a/providers/gcp/services/memorystore/ambiguity_test.go b/providers/gcp/services/memorystore/ambiguity_test.go new file mode 100644 index 00000000..5004555f --- /dev/null +++ b/providers/gcp/services/memorystore/ambiguity_test.go @@ -0,0 +1,72 @@ +package memorystore + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// TestExtractPricing_AmbiguousMatches covers issue #97: the extractor used to +// keep whichever matching SKU came last, so catalog order decided the price. +func TestExtractPricing_AmbiguousMatches(t *testing.T) { + const tier, decoy, region = "STANDARD", "STANDARD_HA", "europe-west4" + od := func(id, tierName string, nanos int64) *cloudbilling.Sku { + return currencySKU(id, "Redis Capacity "+tierName+" in "+region, "OnDemand", region, "USD", nanos) + } + cud := func(id, usage string, nanos int64) *cloudbilling.Sku { + return currencySKU(id, "Redis Capacity "+tier+" commitment in "+region, usage, region, "USD", nanos) + } + tests := []struct { + name string + skus []*cloudbilling.Sku + wantOnDemand float64 + wantCommitment float64 + wantErr string + }{ + { + name: "superstring tier SKU after the real one is ignored", + skus: []*cloudbilling.Sku{od("od", tier, 50000000), od("od-decoy", decoy, 800000000), cud("cud", "Commit1Yr", 40000000)}, + wantOnDemand: 0.05, wantCommitment: 0.04, + }, + { + name: "superstring tier SKU before the real one is ignored", + skus: []*cloudbilling.Sku{od("od-decoy", decoy, 800000000), od("od", tier, 50000000), cud("cud", "Commit1Yr", 40000000)}, + wantOnDemand: 0.05, wantCommitment: 0.04, + }, + { + name: "duplicate on-demand SKUs at the same price agree", + skus: []*cloudbilling.Sku{od("od-a", tier, 50000000), od("od-b", tier, 50000000), cud("cud", "Commit1Yr", 40000000)}, + wantOnDemand: 0.05, wantCommitment: 0.04, + }, + { + name: "two on-demand SKUs at different prices are ambiguous", + skus: []*cloudbilling.Sku{od("od-a", tier, 50000000), od("od-b", tier, 60000000), cud("cud", "Commit1Yr", 40000000)}, + wantErr: "ambiguous catalog: on-demand SKU od-b", + }, + { + name: "same conflict in reverse order is still ambiguous", + skus: []*cloudbilling.Sku{od("od-b", tier, 60000000), od("od-a", tier, 50000000), cud("cud", "Commit1Yr", 40000000)}, + wantErr: "ambiguous catalog: on-demand SKU od-a", + }, + { + name: "1Yr and 3Yr commitment SKUs at different prices are ambiguous", + skus: []*cloudbilling.Sku{od("od", tier, 50000000), cud("cud-1y", "Commit1Yr", 40000000), cud("cud-3y", "Commit3Yr", 30000000)}, + wantErr: "ambiguous catalog: commitment SKU cud-3y", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + onDemand, commitment, _, err := extractPricingFromSKUs(tt.skus, tier, region) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.InDelta(t, tt.wantOnDemand, onDemand, 1e-12) + assert.InDelta(t, tt.wantCommitment, commitment, 1e-12) + }) + } +} diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index cddd7c8c..22cf569e 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -390,6 +390,7 @@ func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, // extractPricingFromSKUs extracts on-demand and commitment pricing from SKU list. func extractPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (onDemand, commitment float64, currency string, err error) { + var prices skumatch.SlotPrices for _, sku := range skus { if !skuMatchesTier(sku, tier, region) { continue @@ -409,15 +410,12 @@ func extractPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (onDe if slotErr != nil { return 0, 0, "", slotErr } - switch slot { - case skumatch.SlotCommitment: - commitment = price - case skumatch.SlotOnDemand: - onDemand = price + if err := prices.Add(slot, sku, price); err != nil { + return 0, 0, "", err } } - return onDemand, commitment, currency, nil + return prices.OnDemand, prices.Commitment, currency, nil } // extractPriceFromSKU extracts the unit price from a SKU. From fcf7114e770814c123184100227b91b763bc57fc Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 21 Oct 2026 09:43:26 +0100 Subject: [PATCH 2/3] style(gcp): reorder structs for govet fieldalignment Main now enables govet fieldalignment. Reorder the fields of skumatch.SlotPrices (added by this PR) so the gcp lint job stays green, and reorder the cloudsql, cloudstorage and memorystore Client and *Pricing structs so their temporary //nolint:govet markers can go. Field order only: every literal of these types is keyed, and none is marshaled or reflected, so behavior is unchanged. skuMu moves last (pointer-free) and keeps a comment saying it guards skuCache. Co-Authored-By: Claude Opus 5.5 --- providers/gcp/internal/skumatch/slotprices.go | 6 +++--- providers/gcp/services/cloudsql/client.go | 12 +++++++----- providers/gcp/services/cloudstorage/client.go | 8 ++++---- providers/gcp/services/memorystore/client.go | 12 +++++++----- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/providers/gcp/internal/skumatch/slotprices.go b/providers/gcp/internal/skumatch/slotprices.go index 92c5b720..44b9204d 100644 --- a/providers/gcp/internal/skumatch/slotprices.go +++ b/providers/gcp/internal/skumatch/slotprices.go @@ -12,11 +12,11 @@ import ( // at different prices make the lookup ambiguous and Add returns an error. // SKUs that repeat the same price agree and are accepted. type SlotPrices struct { - OnDemand float64 - Commitment float64 - onDemandSKU string commitmentSKU string + + OnDemand float64 + Commitment float64 } // Add records price for sku in slot. SlotNone is ignored. diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 6b4de59c..7a17f2a7 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -47,15 +47,17 @@ type RecommenderClient interface { } // Client handles GCP Cloud SQL commitments. -type Client struct { //nolint:govet // fieldalignment: file owned by open PR #471 - skuMu sync.Mutex +type Client struct { skuCache *cloudbilling.ListSkusResponse ctx context.Context projectID string region string - clientOpts []option.ClientOption sqlAdminService SQLAdminService billingService BillingService + clientOpts []option.ClientOption + // skuMu guards skuCache. It sits last because it holds no pointers + // (govet fieldalignment). + skuMu sync.Mutex } // NewClient creates a new GCP Cloud SQL client. @@ -258,11 +260,11 @@ func (c *Client) GetValidResourceTypes(ctx context.Context) ([]string, error) { } // SQLPricing contains pricing information for Cloud SQL. -type SQLPricing struct { //nolint:govet // fieldalignment: file owned by open PR #471 +type SQLPricing struct { + Currency string HourlyRate float64 CommitmentPrice float64 OnDemandPrice float64 - Currency string SavingsPercentage float64 } diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 561d3189..1842cf40 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -59,13 +59,13 @@ type BillingService interface { } // Client handles GCP Cloud Storage commitments. -type Client struct { //nolint:govet // fieldalignment: file owned by open PR #471 +type Client struct { ctx context.Context projectID string region string - clientOpts []option.ClientOption storageService StorageService billingService BillingService + clientOpts []option.ClientOption } // NewClient creates a new GCP Cloud Storage client. @@ -219,11 +219,11 @@ func (c *Client) GetValidResourceTypes(ctx context.Context) ([]string, error) { } // StoragePricing contains pricing information for Cloud Storage. -type StoragePricing struct { //nolint:govet // fieldalignment: file owned by open PR #471 +type StoragePricing struct { + Currency string HourlyRate float64 CommitmentPrice float64 OnDemandPrice float64 - Currency string SavingsPercentage float64 } diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index ad91a410..cec60757 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -53,15 +53,17 @@ type RecommenderClient interface { } // Client handles GCP Memorystore (Redis) commitments. -type Client struct { //nolint:govet // fieldalignment: file owned by open PR #471 - skuMu sync.Mutex +type Client struct { skuCache *cloudbilling.ListSkusResponse ctx context.Context projectID string region string - clientOpts []option.ClientOption redisService RedisService billingService BillingService + clientOpts []option.ClientOption + // skuMu guards skuCache. It sits last because it holds no pointers + // (govet fieldalignment). + skuMu sync.Mutex } // NewClient creates a new GCP Memorystore client. @@ -221,11 +223,11 @@ func (c *Client) GetValidResourceTypes(ctx context.Context) ([]string, error) { } // RedisPricing contains pricing information for Memorystore Redis. -type RedisPricing struct { //nolint:govet // fieldalignment: file owned by open PR #471 +type RedisPricing struct { + Currency string HourlyRate float64 CommitmentPrice float64 OnDemandPrice float64 - Currency string SavingsPercentage float64 } From 0c7fb9a398f42e0e65f677f106394a2a3a89e3cb Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 21 Oct 2026 09:58:48 +0100 Subject: [PATCH 3/3] fix(gcp): track SlotPrices slot presence explicitly SlotPrices treated a slot as filled only when it held a non-empty SKU ID. Nothing upstream rejects a matched SKU with an empty SkuId, so a second matching SKU at a different price could overwrite the first instead of being reported as an ambiguous catalog. Track presence with per-slot flags and cover ID-less conflicts in TestSlotPrices_Add. Co-Authored-By: Claude Opus 5.5 --- providers/gcp/internal/skumatch/slotprices.go | 15 ++++++++++----- .../gcp/internal/skumatch/slotprices_test.go | 7 +++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/providers/gcp/internal/skumatch/slotprices.go b/providers/gcp/internal/skumatch/slotprices.go index 44b9204d..ed0ef812 100644 --- a/providers/gcp/internal/skumatch/slotprices.go +++ b/providers/gcp/internal/skumatch/slotprices.go @@ -17,24 +17,29 @@ type SlotPrices struct { OnDemand float64 Commitment float64 + + // onDemandSet and commitmentSet record that a slot holds a price. They + // do not rely on the SKU ID, which the catalog type allows to be empty. + onDemandSet bool + commitmentSet bool } // Add records price for sku in slot. SlotNone is ignored. func (p *SlotPrices) Add(slot PriceSlot, sku *cloudbilling.Sku, price float64) error { switch slot { case SlotOnDemand: - return setOnce(&p.OnDemand, &p.onDemandSKU, sku, price, "on-demand") + return setOnce(&p.OnDemand, &p.onDemandSKU, &p.onDemandSet, sku, price, "on-demand") case SlotCommitment: - return setOnce(&p.Commitment, &p.commitmentSKU, sku, price, "commitment") + return setOnce(&p.Commitment, &p.commitmentSKU, &p.commitmentSet, sku, price, "commitment") } return nil } -func setOnce(value *float64, skuID *string, sku *cloudbilling.Sku, price float64, slotName string) error { - if *skuID != "" && *value != price { +func setOnce(value *float64, skuID *string, set *bool, sku *cloudbilling.Sku, price float64, slotName string) error { + if *set && *value != price { return fmt.Errorf("ambiguous catalog: %s SKU %s (%q) prices %v but matching SKU %s prices %v", slotName, sku.SkuId, sku.Description, price, *skuID, *value) } - *value, *skuID = price, sku.SkuId + *value, *skuID, *set = price, sku.SkuId, true return nil } diff --git a/providers/gcp/internal/skumatch/slotprices_test.go b/providers/gcp/internal/skumatch/slotprices_test.go index 2dd2be18..a52a1d44 100644 --- a/providers/gcp/internal/skumatch/slotprices_test.go +++ b/providers/gcp/internal/skumatch/slotprices_test.go @@ -11,6 +11,9 @@ import ( func TestSlotPrices_Add(t *testing.T) { a := &cloudbilling.Sku{SkuId: "A", Description: "first"} b := &cloudbilling.Sku{SkuId: "B", Description: "second"} + // SKUs without an ID must still mark their slot as filled. + noID1 := &cloudbilling.Sku{Description: "no id one"} + noID2 := &cloudbilling.Sku{Description: "no id two"} type add struct { slot PriceSlot sku *cloudbilling.Sku @@ -29,6 +32,10 @@ func TestSlotPrices_Add(t *testing.T) { {name: "conflicting on-demand in reverse order is an error", adds: []add{{SlotOnDemand, b, 0.8}, {SlotOnDemand, a, 0.05}}, wantErr: "ambiguous catalog: on-demand SKU A"}, {name: "conflicting commitment is an error", adds: []add{{SlotCommitment, a, 0.04}, {SlotCommitment, b, 0.03}}, wantErr: "ambiguous catalog: commitment SKU B"}, {name: "SlotNone is ignored", adds: []add{{SlotOnDemand, a, 0.05}, {SlotNone, b, 0.01}}, wantOnDemand: 0.05}, + {name: "conflicting on-demand without SKU IDs is an error", adds: []add{{SlotOnDemand, noID1, 0.05}, {SlotOnDemand, noID2, 0.8}}, wantErr: "ambiguous catalog: on-demand SKU (\"no id two\")"}, + {name: "conflicting commitment without SKU IDs is an error", adds: []add{{SlotCommitment, noID1, 0.04}, {SlotCommitment, noID2, 0.03}}, wantErr: "ambiguous catalog: commitment SKU (\"no id two\")"}, + {name: "ID-less SKU then a conflicting SKU is an error", adds: []add{{SlotOnDemand, noID1, 0.05}, {SlotOnDemand, b, 0.8}}, wantErr: "ambiguous catalog: on-demand SKU B"}, + {name: "agreeing duplicates without SKU IDs are accepted", adds: []add{{SlotOnDemand, noID1, 0.05}, {SlotOnDemand, noID2, 0.05}}, wantOnDemand: 0.05}, {name: "slots do not interfere", adds: []add{{SlotOnDemand, a, 0.05}, {SlotCommitment, a, 0.04}, {SlotCommitment, b, 0.04}}, wantOnDemand: 0.05, wantCommitment: 0.04}, } for _, tt := range tests {