fix(gcp): price 1-year and 3-year commitment SKUs separately

CHANGELOG.md

@@ -222,6 +222,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Fixed
 
+- **`gcp`:** the Cloud SQL, Memorystore and Cloud Storage price extractors keep 1-year and 3-year commitment SKUs in separate slots and return the price for the requested term, so a catalog that lists both terms no longer fails as ambiguous (#254). Two SKUs of the same term at different prices still fail closed, and a recommendation term other than 1yr or 3yr (for example "2yr") now errors instead of being priced as one year.
 - **`azure`:** recommendations from the Consumption path (compute, database, cache, cosmosdb, managedredis, synapse) and the Savings Plan benefit path now set `CommitmentCurrency` to `USD`, because their commitment amounts come from sources already required to be USD. The Advisor and legacy paths carry no checked commitment amounts and stay empty (#623).
 - **AWS/OpenSearch:** the STS account-ID lookup used for post-purchase tagging caches only a successful result, so one throttled or timed-out `GetCallerIdentity` no longer disables tagging for the client's lifetime (#53).
 - **AWS/recommendations:** `normalizeRegionName` maps the current `Europe (Ireland|Frankfurt|London|Paris|Stockholm)` Cost Explorer display names to region codes, so coverage lookups keyed by region code match those recommendations (#53).

providers/gcp/internal/skumatch/skumatch.go

@@ -61,7 +61,8 @@ type PriceSlot int
 
 const (
 	SlotOnDemand PriceSlot = iota
-	SlotCommitment
+	SlotCommit1Yr
+	SlotCommit3Yr
 	// SlotNone marks SKUs that are neither on-demand nor committed-use prices.
 	SlotNone
 )
@@ -81,18 +82,19 @@ 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 (#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.
+// Commit1Yr and Commit3Yr are separate slots so a catalog carrying both terms
+// prices each one on its own; SlotPrices.Commitment picks the requested term.
 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)
 	}
 	switch sku.Category.UsageType {
 	case usageOnDemand:
 		return SlotOnDemand, nil
-	case usageCommit1Yr, usageCommit3Yr:
-		return SlotCommitment, nil
+	case usageCommit1Yr:
+		return SlotCommit1Yr, nil
+	case usageCommit3Yr:
+		return SlotCommit3Yr, nil
 	case usagePreemptible, usageSpot:
 		return SlotNone, nil
 	}

providers/gcp/internal/skumatch/skumatch_test.go

@@ -63,8 +63,8 @@ func TestSlot(t *testing.T) {
 		wantErr string
 	}{
 		{name: "on demand", sku: &cloudbilling.Sku{Category: &cloudbilling.Category{UsageType: "OnDemand"}}, want: SlotOnDemand},
-		{name: "1 year commitment without the word in the description", sku: &cloudbilling.Sku{Description: "Committed Use Discount: Cloud SQL DB custom CORE", Category: &cloudbilling.Category{UsageType: "Commit1Yr"}}, want: SlotCommitment},
-		{name: "3 year commitment", sku: &cloudbilling.Sku{Category: &cloudbilling.Category{UsageType: "Commit3Yr"}}, want: SlotCommitment},
+		{name: "1 year commitment without the word in the description", sku: &cloudbilling.Sku{Description: "Committed Use Discount: Cloud SQL DB custom CORE", Category: &cloudbilling.Category{UsageType: "Commit1Yr"}}, want: SlotCommit1Yr},
+		{name: "3 year commitment", sku: &cloudbilling.Sku{Category: &cloudbilling.Category{UsageType: "Commit3Yr"}}, want: SlotCommit3Yr},
 		{name: "description saying commitment on an on-demand SKU", sku: &cloudbilling.Sku{Description: "No commitment required", Category: &cloudbilling.Category{UsageType: "OnDemand"}}, want: SlotOnDemand},
 		{name: "preemptible", sku: &cloudbilling.Sku{Category: &cloudbilling.Category{UsageType: "Preemptible"}}, want: SlotNone},
 		{name: "spot", sku: &cloudbilling.Sku{Category: &cloudbilling.Category{UsageType: "Spot"}}, want: SlotNone},

providers/gcp/internal/skumatch/slotprices.go

@@ -10,27 +10,47 @@ import (
 // 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.
+// SKUs that repeat the same price agree and are accepted. The 1-year and
+// 3-year commitment slots are independent, so a catalog with both terms is not
+// ambiguous; Commitment selects the one the caller asked for.
 type SlotPrices struct {
-	onDemandSKU   string
-	commitmentSKU string
+	onDemandSKU string
+	commit1SKU  string
+	commit3SKU  string
 
-	OnDemand   float64
-	Commitment float64
+	OnDemand 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
+	commit1Yr float64
+	commit3Yr float64
+
+	// The *Set flags 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
+	commit1Set  bool
+	commit3Set  bool
+}
+
+// Commitment returns the commitment price collected for termYears (1 or 3), or
+// 0 when no SKU of that term matched. Any other term is an error.
+func (p *SlotPrices) Commitment(termYears int) (float64, error) {
+	switch termYears {
+	case 1:
+		return p.commit1Yr, nil
+	case 3:
+		return p.commit3Yr, nil
+	}
+	return 0, fmt.Errorf("unsupported commitment term %d years, want 1 or 3", termYears)
 }
 
 // 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")
+	case SlotCommit1Yr:
+		return setOnce(&p.commit1Yr, &p.commit1SKU, &p.commit1Set, sku, price, "1-year commitment")
+	case SlotCommit3Yr:
+		return setOnce(&p.commit3Yr, &p.commit3SKU, &p.commit3Set, sku, price, "3-year commitment")
 	}
 	return nil
 }

providers/gcp/internal/skumatch/slotprices_test.go

@@ -26,17 +26,17 @@ func TestSlotPrices_Add(t *testing.T) {
 		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: "one price per slot", adds: []add{{SlotOnDemand, a, 0.05}, {SlotCommit1Yr, 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: "conflicting commitment is an error", adds: []add{{SlotCommit1Yr, a, 0.04}, {SlotCommit1Yr, b, 0.03}}, wantErr: "ambiguous catalog: 1-year 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: "conflicting commitment without SKU IDs is an error", adds: []add{{SlotCommit1Yr, noID1, 0.04}, {SlotCommit1Yr, noID2, 0.03}}, wantErr: "ambiguous catalog: 1-year 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},
+		{name: "slots do not interfere", adds: []add{{SlotOnDemand, a, 0.05}, {SlotCommit1Yr, a, 0.04}, {SlotCommit1Yr, b, 0.04}}, wantOnDemand: 0.05, wantCommitment: 0.04},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -54,7 +54,7 @@ func TestSlotPrices_Add(t *testing.T) {
 			}
 			require.NoError(t, err)
 			assert.Equal(t, tt.wantOnDemand, p.OnDemand)
-			assert.Equal(t, tt.wantCommitment, p.Commitment)
+			assert.Equal(t, tt.wantCommitment, p.commit1Yr)
 		})
 	}
 }

providers/gcp/internal/skumatch/term.go

@@ -0,0 +1,17 @@
+package skumatch
+
+import "fmt"
+
+// TermYears parses a recommendation term label into commitment years. It
+// accepts "1yr", "1" and "" (an unset term has always meant one year) for one
+// year and "3yr", "3" for three. Anything else is an error: guessing a term
+// would price the wrong commitment.
+func TermYears(term string) (int, error) {
+	switch term {
+	case "", "1yr", "1":
+		return 1, nil
+	case "3yr", "3":
+		return 3, nil
+	}
+	return 0, fmt.Errorf("unsupported commitment term %q, want 1yr or 3yr", term)
+}

providers/gcp/internal/skumatch/term_test.go

@@ -0,0 +1,21 @@
+package skumatch
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestTermYears(t *testing.T) {
+	for term, want := range map[string]int{"": 1, "1yr": 1, "1": 1, "3yr": 3, "3": 3} {
+		got, err := TermYears(term)
+		require.NoError(t, err, term)
+		assert.Equal(t, want, got, term)
+	}
+	for _, term := range []string{"2yr", "2", "5yr", "3yrs", "1Yr", " 3yr", "monthly"} {
+		_, err := TermYears(term)
+		require.Error(t, err, term)
+		assert.Contains(t, err.Error(), "unsupported commitment term")
+	}
+}

providers/gcp/services/cloudsql/ambiguity_test.go

@@ -50,15 +50,10 @@ func TestExtractPricing_AmbiguousMatches(t *testing.T) {
 			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)
+			onDemand, commitment, _, err := extractSQLPricingFromSKUs(tt.skus, tier, region, 1)
 			if tt.wantErr != "" {
 				require.Error(t, err)
 				assert.Contains(t, err.Error(), tt.wantErr)

providers/gcp/services/cloudsql/client.go

@@ -187,9 +187,9 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation
 
 // GetOfferingDetails retrieves Cloud SQL offering details from GCP Billing API.
 func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) {
-	termYears := 1
-	if rec.Term == "3yr" || rec.Term == "3" {
-		termYears = 3
+	termYears, err := skumatch.TermYears(rec.Term)
+	if err != nil {
+		return nil, err
 	}
 
 	pricing, err := c.getSQLPricing(ctx, rec.ResourceType, c.region, termYears)
@@ -282,7 +282,7 @@ func (c *Client) getSQLPricing(ctx context.Context, tier, region string, termYea
 		return nil, fmt.Errorf("failed to list SKUs: %w", err)
 	}
 
-	onDemandPrice, commitmentPrice, currency, err := extractSQLPricingFromSKUs(skus.Skus, tier, region)
+	onDemandPrice, commitmentPrice, currency, err := extractSQLPricingFromSKUs(skus.Skus, tier, region, termYears)
 	if err != nil {
 		return nil, err
 	}
@@ -325,7 +325,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) {
+func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string, termYears int) (onDemand, commitment float64, currency string, err error) {
 	var prices skumatch.SlotPrices
 	for _, sku := range skus {
 		if !skuMatchesTier(sku, tier, region) {
@@ -351,7 +351,11 @@ func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (o
 		}
 	}
 
-	return prices.OnDemand, prices.Commitment, currency, nil
+	commitmentPrice, termErr := prices.Commitment(termYears)
+	if termErr != nil {
+		return 0, 0, "", termErr
+	}
+	return prices.OnDemand, commitmentPrice, currency, nil
 }
 
 // extractSQLPriceFromSKU extracts the unit price from a SKU.

providers/gcp/services/cloudsql/client_test.go

@@ -544,6 +544,11 @@ func TestCloudSQLClient_GetOfferingDetails_3Year(t *testing.T) {
 	mockService := &MockBillingService{
 		skus: &cloudbilling.ListSkusResponse{Skus: sqlMockSkus("db-n1-standard-1", "us-central1")},
 	}
+	for _, sku := range mockService.skus.Skus {
+		if sku.Category.UsageType == "Commit1Yr" {
+			sku.Category.UsageType = "Commit3Yr"
+		}
+	}
 	client.SetBillingService(mockService)
 
 	rec := common.Recommendation{
@@ -711,6 +716,7 @@ func TestCloudSQLClient_GetOfferingDetails_RecurringMonthlyCost(t *testing.T) {
 	client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: []*cloudbilling.Sku{
 		currencySKU("od", "db-n1-standard-1 Cloud SQL", "OnDemand", region, "EUR", int64(onDemandHourly*1e9)),
 		currencySKU("cud", "db-n1-standard-1 Cloud SQL commitment 1yr", "Commit1Yr", region, "EUR", int64(commitmentHourly*1e9)),
+		currencySKU("cud-3y", "db-n1-standard-1 Cloud SQL commitment 3yr", "Commit3Yr", region, "EUR", int64(commitmentHourly*1e9)),
 	}}})
 
 	for term, months := range map[string]float64{"1yr": 12, "3yr": 36} {
@@ -751,7 +757,7 @@ func TestExtractSQLPricingFromSKUs_IgnoresLargerTierAndRegionlessSKUs(t *testing
 		sku("db-n1-standard-1 Cloud SQL", nil, 99),
 	}
 
-	onDemand, _, _, err := extractSQLPricingFromSKUs(skus, "db-n1-standard-1", "europe-west4")
+	onDemand, _, _, err := extractSQLPricingFromSKUs(skus, "db-n1-standard-1", "europe-west4", 1)
 	require.NoError(t, err)
 	assert.Equal(t, 1.0, onDemand)
 }

providers/gcp/services/cloudsql/term_offering_test.go

@@ -0,0 +1,48 @@
+package cloudsql
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/api/cloudbilling/v1"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+)
+
+// A catalog that only lists 1-year commitment SKUs must not price a 3-year
+// request (#254): the missing term is an error, not a zero price.
+func TestSQLClient_GetOfferingDetails_3yrWithOnly1yrCatalogErrors(t *testing.T) {
+	t.Parallel()
+	const region = "europe-west1"
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", region)
+	client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: []*cloudbilling.Sku{
+		currencySKU("od", "db-n1-standard-1 Cloud SQL", "OnDemand", region, "EUR", 200000000),
+		currencySKU("cud", "db-n1-standard-1 Cloud SQL commitment 1yr", "Commit1Yr", region, "EUR", 137000000),
+	}}})
+
+	details, err := client.GetOfferingDetails(ctx, common.Recommendation{ResourceType: "db-n1-standard-1", Region: region, Term: "3yr", PaymentOption: "monthly"})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "no commitment pricing found")
+	assert.Nil(t, details)
+}
+
+// An unsupported term must error instead of being priced as one year (#254).
+func TestSQLClient_GetOfferingDetails_UnsupportedTermErrors(t *testing.T) {
+	t.Parallel()
+	const region = "europe-west1"
+	ctx := context.Background()
+	client, _ := NewClient(ctx, "test-project", region)
+	client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: []*cloudbilling.Sku{
+		currencySKU("od", "db-n1-standard-1 Cloud SQL", "OnDemand", region, "EUR", 200000000),
+		currencySKU("cud", "db-n1-standard-1 Cloud SQL commitment 1yr", "Commit1Yr", region, "EUR", 137000000),
+		currencySKU("cud-3y", "db-n1-standard-1 Cloud SQL commitment 3yr", "Commit3Yr", region, "EUR", 100000000),
+	}}})
+
+	details, err := client.GetOfferingDetails(ctx, common.Recommendation{ResourceType: "db-n1-standard-1", Region: region, Term: "2yr", PaymentOption: "monthly"})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "unsupported commitment term")
+	assert.Nil(t, details)
+}

providers/gcp/services/cloudsql/term_test.go

@@ -0,0 +1,51 @@
+package cloudsql
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/api/cloudbilling/v1"
+)
+
+// TestExtractPricing_CommitmentTerm covers issue #254: 1-year and 3-year
+// commitment SKUs priced differently must not be conflated.
+func TestExtractPricing_CommitmentTerm(t *testing.T) {
+	const tier, region = "db-n1-standard-1", "europe-west4"
+	sku := func(id, usage string, nanos int64) *cloudbilling.Sku {
+		return currencySKU(id, "Cloud SQL for MySQL "+tier+" in "+region, usage, region, "USD", nanos)
+	}
+	od, y1, y3 := sku("od", "OnDemand", 50000000), sku("y1", "Commit1Yr", 40000000), sku("y3", "Commit3Yr", 30000000)
+	y1dup := sku("y1-dup", "Commit1Yr", 39000000)
+	orders := map[string][]*cloudbilling.Sku{"1yr first": {od, y1, y3}, "3yr first": {od, y3, y1}}
+
+	for order, skus := range orders {
+		for _, tc := range []struct {
+			term int
+			want float64
+		}{{1, 0.04}, {3, 0.03}} {
+			t.Run(fmt.Sprintf("%s/%dyr", order, tc.term), func(t *testing.T) {
+				onDemand, commitment, _, err := extractSQLPricingFromSKUs(skus, tier, region, tc.term)
+				require.NoError(t, err)
+				assert.InDelta(t, 0.05, onDemand, 1e-12)
+				assert.InDelta(t, tc.want, commitment, 1e-12)
+			})
+		}
+	}
+	t.Run("same-term duplicate at different prices is ambiguous", func(t *testing.T) {
+		_, _, _, err := extractSQLPricingFromSKUs([]*cloudbilling.Sku{od, y1, y3, y1dup}, tier, region, 1)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "ambiguous catalog: 1-year commitment SKU y1-dup")
+	})
+	t.Run("only the other term present yields no commitment", func(t *testing.T) {
+		_, commitment, _, err := extractSQLPricingFromSKUs([]*cloudbilling.Sku{od, y1}, tier, region, 3)
+		require.NoError(t, err)
+		assert.Zero(t, commitment)
+	})
+	t.Run("unknown term is an error", func(t *testing.T) {
+		_, _, _, err := extractSQLPricingFromSKUs([]*cloudbilling.Sku{od, y1, y3}, tier, region, 2)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "unsupported commitment term")
+	})
+}

providers/gcp/services/cloudsql/tiered_rates_test.go

@@ -29,7 +29,7 @@ func TestExtractSQLPricing_FreeFirstTierUsesPaidTier(t *testing.T) {
 	onDemand, commitment, currency, err := extractSQLPricingFromSKUs([]*cloudbilling.Sku{
 		od,
 		freeFirstTierSKU("cud", tier+" Cloud SQL commitment 1yr", "Commit1Yr", region, 42000000),
-	}, tier, region)
+	}, tier, region, 1)
 	require.NoError(t, err)
 	assert.InDelta(t, 0.05, onDemand, 1e-12)
 	assert.InDelta(t, 0.042, commitment, 1e-12)