diff --git a/providers/gcp/internal/skumatch/skumatch.go b/providers/gcp/internal/skumatch/skumatch.go index 94509df7..25f838e7 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 0771cd08..c4e73afc 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -324,6 +324,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 @@ -343,15 +344,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 7054c2ef..aff7dee6 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -281,6 +281,8 @@ 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 + for _, sku := range skus { if !skuMatchesStorageClass(sku, storageClass, region) { continue @@ -303,15 +305,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. @@ -343,20 +342,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 ceb77f2a..34d48ee0 100644 --- a/providers/gcp/services/cloudstorage/client_test.go +++ b/providers/gcp/services/cloudstorage/client_test.go @@ -199,6 +199,7 @@ func TestSkuMatchesStorageClass(t *testing.T) { storageClass string region string regions []string + geoType string expected bool }{ { @@ -226,13 +227,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 { @@ -243,6 +303,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 486623ad..5fe04da3 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -286,6 +286,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 @@ -305,15 +306,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.