fix(gcp): fail closed on region-less storage SKUs and ambiguous SKU matches

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)

providers/gcp/internal/skumatch/slotprices.go

@@ -0,0 +1,45 @@
+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 {
+	onDemandSKU   string
+	commitmentSKU string
+
+	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, &p.onDemandSet, sku, price, "on-demand")
+	case SlotCommitment:
+		return setOnce(&p.Commitment, &p.commitmentSKU, &p.commitmentSet, sku, price, "commitment")
+	}
+	return nil
+}
+
+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, *set = price, sku.SkuId, true
+	return nil
+}

providers/gcp/internal/skumatch/slotprices_test.go

@@ -0,0 +1,60 @@
+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"}
+	// 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
+		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: "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 {
+		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)
+		})
+	}
+}

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)
+		})
+	}
+}

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
 }
 
@@ -324,6 +326,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 +346,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.

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)
+		})
+	}
+}