diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d8a50ac..d80102e9 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/providers/gcp/internal/skumatch/skumatch.go b/providers/gcp/internal/skumatch/skumatch.go index 25f838e7..d275c85b 100644 --- a/providers/gcp/internal/skumatch/skumatch.go +++ b/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,9 +82,8 @@ 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) @@ -91,8 +91,10 @@ func Slot(sku *cloudbilling.Sku) (PriceSlot, error) { 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 } diff --git a/providers/gcp/internal/skumatch/skumatch_test.go b/providers/gcp/internal/skumatch/skumatch_test.go index 69a7c21c..eabf9a19 100644 --- a/providers/gcp/internal/skumatch/skumatch_test.go +++ b/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}, diff --git a/providers/gcp/internal/skumatch/slotprices.go b/providers/gcp/internal/skumatch/slotprices.go index ed0ef812..2358757a 100644 --- a/providers/gcp/internal/skumatch/slotprices.go +++ b/providers/gcp/internal/skumatch/slotprices.go @@ -10,18 +10,36 @@ 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. @@ -29,8 +47,10 @@ func (p *SlotPrices) Add(slot PriceSlot, sku *cloudbilling.Sku, price float64) e 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 } diff --git a/providers/gcp/internal/skumatch/slotprices_test.go b/providers/gcp/internal/skumatch/slotprices_test.go index a52a1d44..77c4edeb 100644 --- a/providers/gcp/internal/skumatch/slotprices_test.go +++ b/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) }) } } diff --git a/providers/gcp/internal/skumatch/term.go b/providers/gcp/internal/skumatch/term.go new file mode 100644 index 00000000..81737e5e --- /dev/null +++ b/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) +} diff --git a/providers/gcp/internal/skumatch/term_test.go b/providers/gcp/internal/skumatch/term_test.go new file mode 100644 index 00000000..19827451 --- /dev/null +++ b/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") + } +} diff --git a/providers/gcp/services/cloudsql/ambiguity_test.go b/providers/gcp/services/cloudsql/ambiguity_test.go index 79c0ac42..c0b30c7b 100644 --- a/providers/gcp/services/cloudsql/ambiguity_test.go +++ b/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) diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 7a17f2a7..e06f411e 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/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. diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go index e48c96d1..36611f1c 100644 --- a/providers/gcp/services/cloudsql/client_test.go +++ b/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) } diff --git a/providers/gcp/services/cloudsql/term_offering_test.go b/providers/gcp/services/cloudsql/term_offering_test.go new file mode 100644 index 00000000..219f6dd1 --- /dev/null +++ b/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) +} diff --git a/providers/gcp/services/cloudsql/term_test.go b/providers/gcp/services/cloudsql/term_test.go new file mode 100644 index 00000000..fb17c8f0 --- /dev/null +++ b/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") + }) +} diff --git a/providers/gcp/services/cloudsql/tiered_rates_test.go b/providers/gcp/services/cloudsql/tiered_rates_test.go index 055d5696..f61d771d 100644 --- a/providers/gcp/services/cloudsql/tiered_rates_test.go +++ b/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) diff --git a/providers/gcp/services/cloudsql/usage_type_test.go b/providers/gcp/services/cloudsql/usage_type_test.go index fe90ba6a..c1aa80e4 100644 --- a/providers/gcp/services/cloudsql/usage_type_test.go +++ b/providers/gcp/services/cloudsql/usage_type_test.go @@ -30,7 +30,7 @@ func TestExtractor_UsageTypeDecidesCommitmentSlot(t *testing.T) { usageTypeSKU("db-n1-standard-1 Cloud SQL", "OnDemand", 50000000), usageTypeSKU("db-n1-standard-1 Cloud SQL Committed Use Discount", "Commit1Yr", 42000000), } - onDemand, commitment, _, err := extractSQLPricingFromSKUs(skus, "db-n1-standard-1", "us-central1") + onDemand, commitment, _, err := extractSQLPricingFromSKUs(skus, "db-n1-standard-1", "us-central1", 1) require.NoError(t, err) assert.InDelta(t, 0.05, onDemand, 1e-9) assert.InDelta(t, 0.042, commitment, 1e-9) @@ -39,7 +39,7 @@ func TestExtractor_UsageTypeDecidesCommitmentSlot(t *testing.T) { func TestExtractor_UnknownUsageTypeIsAnError(t *testing.T) { t.Parallel() skus := []*cloudbilling.Sku{usageTypeSKU("db-n1-standard-1 Cloud SQL", "Commit5Yr", 42000000)} - _, _, _, err := extractSQLPricingFromSKUs(skus, "db-n1-standard-1", "us-central1") + _, _, _, err := extractSQLPricingFromSKUs(skus, "db-n1-standard-1", "us-central1", 1) require.Error(t, err) assert.Contains(t, err.Error(), "unrecognized usage type") } diff --git a/providers/gcp/services/cloudstorage/ambiguity_test.go b/providers/gcp/services/cloudstorage/ambiguity_test.go index ade347af..0b470ec0 100644 --- a/providers/gcp/services/cloudstorage/ambiguity_test.go +++ b/providers/gcp/services/cloudstorage/ambiguity_test.go @@ -74,7 +74,7 @@ func TestExtractStoragePricing_RegionAndAmbiguity(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - onDemand, commitment, _, err := extractStoragePricingFromSKUs(tt.skus, class, region) + onDemand, commitment, _, err := extractStoragePricingFromSKUs(tt.skus, class, region, 1) if tt.wantErr != "" { require.Error(t, err) assert.Contains(t, err.Error(), tt.wantErr) diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 1842cf40..44b4475d 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -168,9 +168,9 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation // GetOfferingDetails retrieves Cloud Storage 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.getStoragePricing(ctx, rec.ResourceType, c.region, termYears) @@ -241,7 +241,7 @@ func (c *Client) getStoragePricing(ctx context.Context, storageClass, region str return nil, fmt.Errorf("failed to list SKUs: %w", err) } - onDemandPrice, commitmentPrice, currency, err := extractStoragePricingFromSKUs(skus.Skus, storageClass, region) + onDemandPrice, commitmentPrice, currency, err := extractStoragePricingFromSKUs(skus.Skus, storageClass, region, termYears) if err != nil { return nil, err } @@ -280,7 +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) { +func extractStoragePricingFromSKUs(skus []*cloudbilling.Sku, storageClass, region string, termYears int) (onDemand, commitment float64, currency string, err error) { var prices skumatch.SlotPrices for _, sku := range skus { @@ -310,7 +310,11 @@ func extractStoragePricingFromSKUs(skus []*cloudbilling.Sku, storageClass, regio } } - 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 } // extractStoragePriceFromSKU returns a per-GiB-hour rate regardless of the catalog unit. diff --git a/providers/gcp/services/cloudstorage/client_test.go b/providers/gcp/services/cloudstorage/client_test.go index 34d48ee0..09717d8c 100644 --- a/providers/gcp/services/cloudstorage/client_test.go +++ b/providers/gcp/services/cloudstorage/client_test.go @@ -441,6 +441,16 @@ func TestCloudStorageClient_DeprecatedRecommenderClientIsNotCalled(t *testing.T) // storageMockSkus returns a slice with both an on-demand and a commitment SKU for // the given storage class and region. Required by tests that exercise GetOfferingDetails // after the issue #1020 fix (fabricated commitment prices are no longer allowed). +// as3Year relabels the 1-year commitment SKUs of skus as 3-year ones. +func as3Year(skus []*cloudbilling.Sku) []*cloudbilling.Sku { + for _, sku := range skus { + if sku.Category.UsageType == "Commit1Yr" { + sku.Category.UsageType = "Commit3Yr" + } + } + return skus +} + func storageMockSkus(storageClass string, onDemandNanos, commitmentNanos int64) []*cloudbilling.Sku { const region = "us-central1" return []*cloudbilling.Sku{ @@ -526,7 +536,7 @@ func TestCloudStorageClient_GetOfferingDetails_3yr(t *testing.T) { // Both on-demand and commitment SKUs required after the issue #1020 fix. mockService := &MockBillingService{ skus: &cloudbilling.ListSkusResponse{ - Skus: storageMockSkus("NEARLINE", 10000000, 7000000), + Skus: as3Year(storageMockSkus("NEARLINE", 10000000, 7000000)), }, } client.SetBillingService(mockService) @@ -634,7 +644,7 @@ func TestCloudStorageClient_GetStoragePricing_3Year(t *testing.T) { // without a commitment SKU, getStoragePricing returns an error. mockService := &MockBillingService{ skus: &cloudbilling.ListSkusResponse{ - Skus: storageMockSkus("STANDARD", 26000000, 18200000), + Skus: as3Year(storageMockSkus("STANDARD", 26000000, 18200000)), }, } client.SetBillingService(mockService) @@ -703,6 +713,9 @@ func TestStoragePricingUnits_PublicConsumers(t *testing.T) { skus[0].PricingInfo[0].PricingExpression.UsageUnit = tc.onDemandUnit skus[1].PricingInfo[0].PricingExpression.UsageUnit = tc.commitmentUnit skus[0].PricingInfo[0].PricingExpression.DisplayQuantity = 1000 + commit3 := *skus[1] + commit3.Category = &cloudbilling.Category{UsageType: "Commit3Yr"} + skus = append(skus, &commit3) unrelated := storageMockSkus("NEARLINE", 26000000, 20000000)[0] unrelated.PricingInfo[0].PricingExpression.UsageUnit = "unknown" skus = append(skus, unrelated) diff --git a/providers/gcp/services/cloudstorage/term_test.go b/providers/gcp/services/cloudstorage/term_test.go new file mode 100644 index 00000000..19e14a71 --- /dev/null +++ b/providers/gcp/services/cloudstorage/term_test.go @@ -0,0 +1,72 @@ +package cloudstorage + +import ( + "fmt" + "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" +) + +// TestExtractStoragePricing_CommitmentTerm covers issue #254: 1-year and +// 3-year commitment SKUs priced differently must not be conflated. +func TestExtractStoragePricing_CommitmentTerm(t *testing.T) { + const class, region = "STANDARD", "europe-west4" + here := []string{region} + sku := func(id, usage string, nanos int64) *cloudbilling.Sku { + return storageSKU(id, "Standard Storage Netherlands", usage, here, nanos) + } + od, y1, y3 := sku("od", "OnDemand", 20000000), sku("y1", "Commit1Yr", 15000000), sku("y3", "Commit3Yr", 10000000) + y1dup := sku("y1-dup", "Commit1Yr", 14000000) + 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.015}, {3, 0.010}} { + t.Run(fmt.Sprintf("%s/%dyr", order, tc.term), func(t *testing.T) { + onDemand, commitment, _, err := extractStoragePricingFromSKUs(skus, class, region, tc.term) + require.NoError(t, err) + assert.InDelta(t, 0.02/averageHoursPerMonth, onDemand, 1e-15) + assert.InDelta(t, tc.want/averageHoursPerMonth, commitment, 1e-15) + }) + } + } + t.Run("same-term duplicate at different prices is ambiguous", func(t *testing.T) { + _, _, _, err := extractStoragePricingFromSKUs([]*cloudbilling.Sku{od, y1, y3, y1dup}, class, 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 := extractStoragePricingFromSKUs([]*cloudbilling.Sku{od, y1}, class, region, 3) + require.NoError(t, err) + assert.Zero(t, commitment) + }) + t.Run("unknown term is an error", func(t *testing.T) { + _, _, _, err := extractStoragePricingFromSKUs([]*cloudbilling.Sku{od, y1, y3}, class, region, 2) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported commitment term") + }) +} + +// An unsupported term must error instead of being priced as one year (#254). +func TestCloudStorageClient_GetOfferingDetails_UnsupportedTermErrors(t *testing.T) { + t.Parallel() + skus := storageMockSkus("STANDARD", 26000000, 19500000) + commit3 := *skus[1] + commit3.Category = &cloudbilling.Category{UsageType: "Commit3Yr"} + commit3.PricingInfo = []*cloudbilling.PricingInfo{{PricingExpression: &cloudbilling.PricingExpression{ + UsageUnit: "GiBy.mo", + TieredRates: []*cloudbilling.TierRate{{UnitPrice: &cloudbilling.Money{CurrencyCode: "USD", Nanos: 15000000}}}, + }}} + client := storageCatalogClient(t, append(skus, &commit3)) + + details, err := client.GetOfferingDetails(t.Context(), common.Recommendation{ResourceType: "STANDARD", Term: "2yr", PaymentOption: "monthly"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported commitment term") + assert.Nil(t, details) +} diff --git a/providers/gcp/services/cloudstorage/tiered_rates_test.go b/providers/gcp/services/cloudstorage/tiered_rates_test.go index 6e6fb648..3b2d315e 100644 --- a/providers/gcp/services/cloudstorage/tiered_rates_test.go +++ b/providers/gcp/services/cloudstorage/tiered_rates_test.go @@ -29,7 +29,7 @@ func TestExtractStoragePricing_FreeFirstTierUsesPaidTier(t *testing.T) { onDemand, commitment, currency, err := extractStoragePricingFromSKUs([]*cloudbilling.Sku{ od, freeFirstTierStorageSKU("STANDARD Storage in us-central1 Committed Use Discount", "Commit1Yr", 16000000), - }, "STANDARD", "us-central1") + }, "STANDARD", "us-central1", 1) require.NoError(t, err) assert.InDelta(t, 0.02/averageHoursPerMonth, onDemand, 1e-15) assert.InDelta(t, 0.016/averageHoursPerMonth, commitment, 1e-15) diff --git a/providers/gcp/services/cloudstorage/usage_type_test.go b/providers/gcp/services/cloudstorage/usage_type_test.go index d79aad30..fe65d03a 100644 --- a/providers/gcp/services/cloudstorage/usage_type_test.go +++ b/providers/gcp/services/cloudstorage/usage_type_test.go @@ -31,7 +31,7 @@ func TestExtractor_UsageTypeDecidesCommitmentSlot(t *testing.T) { usageTypeSKU("STANDARD Storage in us-central1", "OnDemand", 50000000), usageTypeSKU("STANDARD Storage in us-central1 Committed Use Discount", "Commit1Yr", 42000000), } - onDemand, commitment, _, err := extractStoragePricingFromSKUs(skus, "STANDARD", "us-central1") + onDemand, commitment, _, err := extractStoragePricingFromSKUs(skus, "STANDARD", "us-central1", 1) require.NoError(t, err) assert.Greater(t, onDemand, commitment) assert.Greater(t, commitment, 0.0) @@ -40,7 +40,7 @@ func TestExtractor_UsageTypeDecidesCommitmentSlot(t *testing.T) { func TestExtractor_UnknownUsageTypeIsAnError(t *testing.T) { t.Parallel() skus := []*cloudbilling.Sku{usageTypeSKU("STANDARD Storage in us-central1", "Commit5Yr", 42000000)} - _, _, _, err := extractStoragePricingFromSKUs(skus, "STANDARD", "us-central1") + _, _, _, err := extractStoragePricingFromSKUs(skus, "STANDARD", "us-central1", 1) require.Error(t, err) assert.Contains(t, err.Error(), "unrecognized usage type") } diff --git a/providers/gcp/services/memorystore/ambiguity_test.go b/providers/gcp/services/memorystore/ambiguity_test.go index 5004555f..e4accb15 100644 --- a/providers/gcp/services/memorystore/ambiguity_test.go +++ b/providers/gcp/services/memorystore/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 := extractPricingFromSKUs(tt.skus, tier, region) + onDemand, commitment, _, err := extractPricingFromSKUs(tt.skus, tier, region, 1) if tt.wantErr != "" { require.Error(t, err) assert.Contains(t, err.Error(), tt.wantErr) diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index cec60757..7bf9bf9d 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -174,9 +174,9 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation // GetOfferingDetails retrieves Memorystore 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.getRedisPricing(ctx, rec.ResourceType, c.region, termYears) @@ -245,7 +245,7 @@ func (c *Client) getRedisPricing(ctx context.Context, tier, region string, termY return nil, fmt.Errorf("failed to list SKUs: %w", err) } - onDemandPrice, commitmentPrice, currency, err := extractPricingFromSKUs(skus.Skus, tier, region) + onDemandPrice, commitmentPrice, currency, err := extractPricingFromSKUs(skus.Skus, tier, region, termYears) if err != nil { return nil, err } @@ -287,7 +287,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) { +func extractPricingFromSKUs(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) { @@ -313,7 +313,11 @@ func extractPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (onDe } } - 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 } // extractPriceFromSKU extracts the unit price from a SKU. diff --git a/providers/gcp/services/memorystore/client_coverage_test.go b/providers/gcp/services/memorystore/client_coverage_test.go index b81d864f..c5bbfc2c 100644 --- a/providers/gcp/services/memorystore/client_coverage_test.go +++ b/providers/gcp/services/memorystore/client_coverage_test.go @@ -148,7 +148,7 @@ func TestExtractPricingFromSKUs_ValidPricing(t *testing.T) { }, } - onDemand, commitment, currency, err := extractPricingFromSKUs(skus, "M1", "us-central1") + onDemand, commitment, currency, err := extractPricingFromSKUs(skus, "M1", "us-central1", 1) require.NoError(t, err) assert.Greater(t, onDemand, 0.0) assert.Greater(t, commitment, 0.0) @@ -182,7 +182,7 @@ func TestExtractPricingFromSKUs_NoMatchingSKUs(t *testing.T) { }, } - onDemand, commitment, currency, err := extractPricingFromSKUs(skus, "M1", "us-central1") + onDemand, commitment, currency, err := extractPricingFromSKUs(skus, "M1", "us-central1", 1) require.NoError(t, err) assert.Equal(t, 0.0, onDemand) assert.Equal(t, 0.0, commitment) diff --git a/providers/gcp/services/memorystore/client_test.go b/providers/gcp/services/memorystore/client_test.go index b65197ae..8ee6d6d8 100644 --- a/providers/gcp/services/memorystore/client_test.go +++ b/providers/gcp/services/memorystore/client_test.go @@ -662,6 +662,7 @@ func TestMemorystoreClient_GetOfferingDetails_RecurringMonthlyCost(t *testing.T) client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: []*cloudbilling.Sku{ currencySKU("od", "redis-basic Cloud Memorystore Redis", "OnDemand", region, "EUR", int64(onDemandHourly*1e9)), currencySKU("cud", "redis-basic Cloud Memorystore Redis commitment 1yr", "Commit1Yr", region, "EUR", int64(commitmentHourly*1e9)), + currencySKU("cud-3y", "redis-basic Cloud Memorystore Redis commitment 3yr", "Commit3Yr", region, "EUR", int64(commitmentHourly*1e9)), }}}) for term, months := range map[string]float64{"1yr": 12, "3yr": 36} { diff --git a/providers/gcp/services/memorystore/term_offering_test.go b/providers/gcp/services/memorystore/term_offering_test.go new file mode 100644 index 00000000..36412657 --- /dev/null +++ b/providers/gcp/services/memorystore/term_offering_test.go @@ -0,0 +1,48 @@ +package memorystore + +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 TestMemorystoreClient_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", "redis-basic Cloud Memorystore Redis", "OnDemand", region, "EUR", 200000000), + currencySKU("cud", "redis-basic Cloud Memorystore Redis commitment 1yr", "Commit1Yr", region, "EUR", 137000000), + }}}) + + details, err := client.GetOfferingDetails(ctx, common.Recommendation{ResourceType: "redis-basic", 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 TestMemorystoreClient_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", "redis-basic Cloud Memorystore Redis", "OnDemand", region, "EUR", 200000000), + currencySKU("cud", "redis-basic Cloud Memorystore Redis commitment 1yr", "Commit1Yr", region, "EUR", 137000000), + currencySKU("cud-3y", "redis-basic Cloud Memorystore Redis commitment 3yr", "Commit3Yr", region, "EUR", 100000000), + }}}) + + details, err := client.GetOfferingDetails(ctx, common.Recommendation{ResourceType: "redis-basic", Region: region, Term: "2yr", PaymentOption: "monthly"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported commitment term") + assert.Nil(t, details) +} diff --git a/providers/gcp/services/memorystore/term_test.go b/providers/gcp/services/memorystore/term_test.go new file mode 100644 index 00000000..4ffa4fe8 --- /dev/null +++ b/providers/gcp/services/memorystore/term_test.go @@ -0,0 +1,51 @@ +package memorystore + +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 = "STANDARD", "europe-west4" + sku := func(id, usage string, nanos int64) *cloudbilling.Sku { + return currencySKU(id, "Redis Capacity "+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 := extractPricingFromSKUs(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 := extractPricingFromSKUs([]*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 := extractPricingFromSKUs([]*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 := extractPricingFromSKUs([]*cloudbilling.Sku{od, y1, y3}, tier, region, 2) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported commitment term") + }) +} diff --git a/providers/gcp/services/memorystore/tiered_rates_test.go b/providers/gcp/services/memorystore/tiered_rates_test.go index d7a34a19..b340b865 100644 --- a/providers/gcp/services/memorystore/tiered_rates_test.go +++ b/providers/gcp/services/memorystore/tiered_rates_test.go @@ -29,7 +29,7 @@ func TestExtractPricing_FreeFirstTierUsesPaidTier(t *testing.T) { onDemand, commitment, currency, err := extractPricingFromSKUs([]*cloudbilling.Sku{ od, freeFirstTierSKU("cud", tier+" Memorystore 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) diff --git a/providers/gcp/services/memorystore/usage_type_test.go b/providers/gcp/services/memorystore/usage_type_test.go index 294e24ef..1a87deb4 100644 --- a/providers/gcp/services/memorystore/usage_type_test.go +++ b/providers/gcp/services/memorystore/usage_type_test.go @@ -30,7 +30,7 @@ func TestExtractor_UsageTypeDecidesCommitmentSlot(t *testing.T) { usageTypeSKU("Memorystore Redis STANDARD_HA", "OnDemand", 50000000), usageTypeSKU("Memorystore Redis STANDARD_HA Committed Use Discount", "Commit1Yr", 42000000), } - onDemand, commitment, _, err := extractPricingFromSKUs(skus, "STANDARD_HA", "us-central1") + onDemand, commitment, _, err := extractPricingFromSKUs(skus, "STANDARD_HA", "us-central1", 1) require.NoError(t, err) assert.InDelta(t, 0.05, onDemand, 1e-9) assert.InDelta(t, 0.042, commitment, 1e-9) @@ -39,7 +39,7 @@ func TestExtractor_UsageTypeDecidesCommitmentSlot(t *testing.T) { func TestExtractor_UnknownUsageTypeIsAnError(t *testing.T) { t.Parallel() skus := []*cloudbilling.Sku{usageTypeSKU("Memorystore Redis STANDARD_HA", "Commit5Yr", 42000000)} - _, _, _, err := extractPricingFromSKUs(skus, "STANDARD_HA", "us-central1") + _, _, _, err := extractPricingFromSKUs(skus, "STANDARD_HA", "us-central1", 1) require.Error(t, err) assert.Contains(t, err.Error(), "unrecognized usage type") }