From 0b4ca2a1552c92341002978edf1253cc552325db Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 11:31:18 +0100 Subject: [PATCH 1/4] refactor(gcp): share the savings and tier-matching helpers Move the savings-percentage formula and the SKU tier matcher, copied verbatim in cloudsql, cloudstorage and memorystore, into internal/skumatch as SavingsPercent and MatchesTier. Callers switched, copies deleted; behavior is unchanged. Closes #95 Co-Authored-By: Claude Opus 5.5 --- providers/gcp/internal/skumatch/savings.go | 11 +++ .../gcp/internal/skumatch/savings_test.go | 71 +++++++++++++++++++ providers/gcp/internal/skumatch/skumatch.go | 6 ++ providers/gcp/services/cloudsql/client.go | 15 +--- .../gcp/services/cloudsql/client_test.go | 3 +- providers/gcp/services/cloudstorage/client.go | 8 +-- providers/gcp/services/memorystore/client.go | 15 +--- .../gcp/services/memorystore/client_test.go | 3 +- 8 files changed, 97 insertions(+), 35 deletions(-) create mode 100644 providers/gcp/internal/skumatch/savings.go create mode 100644 providers/gcp/internal/skumatch/savings_test.go diff --git a/providers/gcp/internal/skumatch/savings.go b/providers/gcp/internal/skumatch/savings.go new file mode 100644 index 00000000..efcc4add --- /dev/null +++ b/providers/gcp/internal/skumatch/savings.go @@ -0,0 +1,11 @@ +package skumatch + +// SavingsPercent returns the percentage a commitment saves against on-demand +// over a term. commitmentTotal must already be a term total (per-hour price * +// hoursInTerm), on the same basis as onDemandPrice * hoursInTerm. A zero +// on-demand total yields NaN or Inf, so callers must reject a zero on-demand +// price first. +func SavingsPercent(onDemandPrice, hoursInTerm, commitmentTotal float64) float64 { + onDemandTotal := onDemandPrice * hoursInTerm + return ((onDemandTotal - commitmentTotal) / onDemandTotal) * 100 +} diff --git a/providers/gcp/internal/skumatch/savings_test.go b/providers/gcp/internal/skumatch/savings_test.go new file mode 100644 index 00000000..07500bee --- /dev/null +++ b/providers/gcp/internal/skumatch/savings_test.go @@ -0,0 +1,71 @@ +package skumatch + +import ( + "math" + "testing" + + "github.com/stretchr/testify/assert" + "google.golang.org/api/cloudbilling/v1" +) + +func TestSavingsPercent(t *testing.T) { + t.Parallel() + tests := []struct { + name string + onDemand, hours, cm float64 + want float64 + }{ + {"typical 16 percent", 0.05, 8760, 0.042 * 8760, 16}, + {"commitment equals on-demand", 1, 100, 100, 0}, + {"commitment above on-demand is negative", 1, 100, 150, -50}, + {"free commitment saves everything", 1, 100, 0, 100}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.InDelta(t, tt.want, SavingsPercent(tt.onDemand, tt.hours, tt.cm), 1e-9) + }) + } + + t.Run("zero on-demand with commitment is -Inf", func(t *testing.T) { + t.Parallel() + assert.True(t, math.IsInf(SavingsPercent(0, 8760, 100), -1)) + }) + t.Run("zero hours with commitment is -Inf", func(t *testing.T) { + t.Parallel() + assert.True(t, math.IsInf(SavingsPercent(1, 0, 100), -1)) + }) + t.Run("zero on-demand and zero commitment is NaN", func(t *testing.T) { + t.Parallel() + assert.True(t, math.IsNaN(SavingsPercent(0, 8760, 0))) + }) + t.Run("negative on-demand flips the sign", func(t *testing.T) { + t.Parallel() + assert.InDelta(t, 200, SavingsPercent(-1, 100, 100), 1e-9) + }) +} + +func TestMatchesTier(t *testing.T) { + t.Parallel() + global := &cloudbilling.GeoTaxonomy{Type: "GLOBAL"} + tests := []struct { + name string + sku *cloudbilling.Sku + tier, region string + want bool + }{ + {"token and region match", &cloudbilling.Sku{Description: "Commitment db-n1-standard-1", ServiceRegions: []string{"us-central1"}}, "db-n1-standard-1", "us-central1", true}, + {"case-insensitive", &cloudbilling.Sku{Description: "COMMITMENT DB-N1-STANDARD-1", ServiceRegions: []string{"US-CENTRAL1"}}, "db-n1-standard-1", "us-central1", true}, + {"tier is a prefix of another tier", &cloudbilling.Sku{Description: "db-n1-standard-16", ServiceRegions: []string{"us-central1"}}, "db-n1-standard-1", "us-central1", false}, + {"wrong region", &cloudbilling.Sku{Description: "db-n1-standard-1", ServiceRegions: []string{"europe-west1"}}, "db-n1-standard-1", "us-central1", false}, + {"global SKU", &cloudbilling.Sku{Description: "db-n1-standard-1", GeoTaxonomy: global}, "db-n1-standard-1", "us-central1", true}, + {"empty tier never matches", &cloudbilling.Sku{Description: "anything", ServiceRegions: []string{"us-central1"}}, "", "us-central1", false}, + {"empty regions without taxonomy", &cloudbilling.Sku{Description: "db-n1-standard-1"}, "db-n1-standard-1", "us-central1", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, MatchesTier(tt.sku, tt.tier, tt.region)) + }) + } +} diff --git a/providers/gcp/internal/skumatch/skumatch.go b/providers/gcp/internal/skumatch/skumatch.go index d275c85b..666e6bf1 100644 --- a/providers/gcp/internal/skumatch/skumatch.go +++ b/providers/gcp/internal/skumatch/skumatch.go @@ -29,6 +29,12 @@ func InRegion(sku *cloudbilling.Sku, region string) bool { return false } +// MatchesTier reports whether the SKU description names tier as a whole token +// and the SKU applies in region. +func MatchesTier(sku *cloudbilling.Sku, tier, region string) bool { + return HasToken(sku.Description, tier) && InRegion(sku, region) +} + // HasToken reports whether the SKU description contains token as a whole // identifier, case-insensitively. "db-n1-standard-1" does not match inside // "db-n1-standard-16" and "STANDARD" does not match inside "STANDARD_HA". diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 960010a4..128a11de 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -298,7 +298,7 @@ func (c *Client) getSQLPricing(ctx context.Context, tier, region string, termYea // same basis as onDemandPrice * hoursInTerm. Without this, the savings // percentage would be nearly 100% (per-hour price vs term total). commitmentPriceTerm := commitmentPrice * hoursInTerm - savingsPercentage := calculateSQLSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPriceTerm) + savingsPercentage := skumatch.SavingsPercent(onDemandPrice, hoursInTerm, commitmentPriceTerm) return &SQLPricing{ HourlyRate: commitmentPrice, @@ -328,7 +328,7 @@ func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, 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) { + if !skumatch.MatchesTier(sku, tier, region) { continue } @@ -374,17 +374,6 @@ func extractSQLPriceFromSKU(sku *cloudbilling.Sku) (price float64, currency stri return price, rate.UnitPrice.CurrencyCode } -// calculateSQLSavingsPercentage calculates the savings percentage. -func calculateSQLSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 { - onDemandTotal := onDemandPrice * hoursInTerm - return ((onDemandTotal - commitmentPrice) / onDemandTotal) * 100 -} - -// skuMatchesTier checks if a SKU matches the tier and region. -func skuMatchesTier(sku *cloudbilling.Sku, tier, region string) bool { - return skumatch.HasToken(sku.Description, tier) && skumatch.InRegion(sku, region) -} - // contains checks if a slice contains a string. func contains(slice []string, str string) bool { for _, s := range slice { diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go index 36611f1c..f8ac2818 100644 --- a/providers/gcp/services/cloudsql/client_test.go +++ b/providers/gcp/services/cloudsql/client_test.go @@ -14,6 +14,7 @@ import ( "google.golang.org/genproto/googleapis/type/money" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) // MockSQLAdminService mocks the SQLAdminService interface. @@ -260,7 +261,7 @@ func TestSkuMatchesTier(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - result := skuMatchesTier(tt.sku, tt.tier, tt.region) + result := skumatch.MatchesTier(tt.sku, tt.tier, tt.region) assert.Equal(t, tt.expected, result) }) } diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 6e99a869..55b568f6 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -251,7 +251,7 @@ func (c *Client) getStoragePricing(ctx context.Context, storageClass, region str hoursInTerm := common.HoursPerYear * float64(termYears) commitmentPriceTerm := commitmentPrice * hoursInTerm - savingsPercentage := calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPriceTerm) + savingsPercentage := skumatch.SavingsPercent(onDemandPrice, hoursInTerm, commitmentPriceTerm) return &StoragePricing{ HourlyRate: commitmentPrice, @@ -337,12 +337,6 @@ func extractStoragePriceFromSKU(sku *cloudbilling.Sku) (price float64, currency return price, rate.UnitPrice.CurrencyCode, nil } -// calculateStorageSavingsPercentage calculates the savings percentage. -func calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 { - onDemandTotal := onDemandPrice * hoursInTerm - 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. diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 62b7fc94..5922d04f 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -261,7 +261,7 @@ func (c *Client) getRedisPricing(ctx context.Context, tier, region string, termY // same basis as onDemandPrice * hoursInTerm. Without this, the savings // percentage would be nearly 100% (per-hour price vs term total). commitmentPriceTerm := commitmentPrice * hoursInTerm - savingsPercentage := calculateSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPriceTerm) + savingsPercentage := skumatch.SavingsPercent(onDemandPrice, hoursInTerm, commitmentPriceTerm) return &RedisPricing{ HourlyRate: commitmentPrice, @@ -290,7 +290,7 @@ func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, 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) { + if !skumatch.MatchesTier(sku, tier, region) { continue } @@ -336,17 +336,6 @@ func extractPriceFromSKU(sku *cloudbilling.Sku) (price float64, currency string) return price, rate.UnitPrice.CurrencyCode } -// calculateSavingsPercentage calculates the savings percentage. -func calculateSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 { - onDemandTotal := onDemandPrice * hoursInTerm - return ((onDemandTotal - commitmentPrice) / onDemandTotal) * 100 -} - -// skuMatchesTier checks if a SKU matches the tier and region. -func skuMatchesTier(sku *cloudbilling.Sku, tier, region string) bool { - return skumatch.HasToken(sku.Description, tier) && skumatch.InRegion(sku, region) -} - // memorystoreRedisServiceID is the Cloud Billing service for Cloud Memorystore for Redis. const memorystoreRedisServiceID = "services/5AF5-2C11-D467" diff --git a/providers/gcp/services/memorystore/client_test.go b/providers/gcp/services/memorystore/client_test.go index 8ee6d6d8..cd65575a 100644 --- a/providers/gcp/services/memorystore/client_test.go +++ b/providers/gcp/services/memorystore/client_test.go @@ -15,6 +15,7 @@ import ( "google.golang.org/genproto/googleapis/type/money" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) // Mock implementations @@ -263,7 +264,7 @@ func TestSkuMatchesTier(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - result := skuMatchesTier(tt.sku, tt.tier, tt.region) + result := skumatch.MatchesTier(tt.sku, tt.tier, tt.region) assert.Equal(t, tt.expected, result) }) } From 4010da40509799212bcda2ee0c99282df76a0b02 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 11:55:34 +0100 Subject: [PATCH 2/4] refactor(gcp): share the billing-service wrapper across SKU-priced clients Move the BillingService interface, the real Cloud Billing wrapper and the get-or-create logic, identical in cloudsql, cloudstorage and memorystore except for the service name, into internal/skulist. Each client keeps BillingService as an alias so its mocks are unchanged. Co-Authored-By: Claude Opus 5.5 --- providers/gcp/internal/skulist/billing.go | 42 ++++++++++++++ .../gcp/internal/skulist/billing_test.go | 56 +++++++++++++++++++ providers/gcp/services/cloudsql/client.go | 37 ++---------- providers/gcp/services/cloudstorage/client.go | 37 ++---------- providers/gcp/services/memorystore/client.go | 37 ++---------- 5 files changed, 110 insertions(+), 99 deletions(-) create mode 100644 providers/gcp/internal/skulist/billing.go create mode 100644 providers/gcp/internal/skulist/billing_test.go diff --git a/providers/gcp/internal/skulist/billing.go b/providers/gcp/internal/skulist/billing.go new file mode 100644 index 00000000..b44851f8 --- /dev/null +++ b/providers/gcp/internal/skulist/billing.go @@ -0,0 +1,42 @@ +package skulist + +import ( + "context" + "fmt" + + "google.golang.org/api/cloudbilling/v1" + "google.golang.org/api/option" +) + +// BillingService lists the SKUs of one Cloud Billing service (enables mocking). +type BillingService interface { + ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) +} + +// realBillingService lists SKUs through the Cloud Billing API; name labels the +// owning GCP service in errors. +type realBillingService struct { + service *cloudbilling.APIService + name string +} + +func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { + skus, err := All(context.Background(), r.service.Services.Skus.List(serviceID), DefaultMaxPages) + if err != nil { + return nil, fmt.Errorf("%s: list SKUs for %s: %w", r.name, serviceID, err) + } + return &cloudbilling.ListSkusResponse{Skus: skus}, nil +} + +// Resolve returns existing when set, else a BillingService backed by a new +// Cloud Billing client. name labels the owning GCP service in errors. +func Resolve(ctx context.Context, existing BillingService, name string, opts ...option.ClientOption) (BillingService, error) { + if existing != nil { + return existing, nil + } + service, err := cloudbilling.NewService(ctx, opts...) + if err != nil { + return nil, fmt.Errorf("failed to create billing service: %w", err) + } + return &realBillingService{service: service, name: name}, nil +} diff --git a/providers/gcp/internal/skulist/billing_test.go b/providers/gcp/internal/skulist/billing_test.go new file mode 100644 index 00000000..0a871ea1 --- /dev/null +++ b/providers/gcp/internal/skulist/billing_test.go @@ -0,0 +1,56 @@ +package skulist + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" + "google.golang.org/api/option" +) + +type stubBilling struct{} + +func (stubBilling) ListSKUs(string) (*cloudbilling.ListSkusResponse, error) { return nil, nil } + +func TestResolve(t *testing.T) { + t.Parallel() + + t.Run("existing service is returned as is", func(t *testing.T) { + t.Parallel() + existing := stubBilling{} + got, err := Resolve(context.Background(), existing, "cloudsql") + require.NoError(t, err) + assert.Equal(t, existing, got) + }) + + t.Run("lists SKUs through the API", func(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"skus":[{"skuId":"A"}]}`)) + })) + t.Cleanup(srv.Close) + svc, err := Resolve(context.Background(), nil, "cloudsql", option.WithEndpoint(srv.URL), option.WithoutAuthentication()) + require.NoError(t, err) + resp, err := svc.ListSKUs("services/X") + require.NoError(t, err) + require.Len(t, resp.Skus, 1) + assert.Equal(t, "A", resp.Skus[0].SkuId) + }) + + t.Run("list error names the owning service and the billing service", func(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "boom", http.StatusInternalServerError) + })) + t.Cleanup(srv.Close) + svc, err := Resolve(context.Background(), nil, "memorystore", option.WithEndpoint(srv.URL), option.WithoutAuthentication()) + require.NoError(t, err) + _, err = svc.ListSKUs("services/X") + require.Error(t, err) + assert.Contains(t, err.Error(), "memorystore: list SKUs for services/X") + }) +} diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 128a11de..d0d07978 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -19,6 +19,9 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) +// BillingService lists SKUs of one Cloud Billing service (enables mocking). +type BillingService = skulist.BillingService + // SQLAdminService interface for SQL admin operations (enables mocking). type SQLAdminService interface { ListInstances(projectID string) (*sqladmin.InstancesListResponse, error) @@ -26,11 +29,6 @@ type SQLAdminService interface { ListTiers(projectID string) (*sqladmin.TiersListResponse, error) } -// BillingService interface for billing operations (enables mocking). -type BillingService interface { - ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) -} - // RecommenderIterator preserves the legacy iteration API for source compatibility. // // Deprecated: Cloud SQL has no project-scoped commitment recommender (issue #294). @@ -102,19 +100,6 @@ func (r *realSQLAdminService) ListTiers(projectID string) (*sqladmin.TiersListRe return r.service.Tiers.List(projectID).Do() } -// realBillingService wraps the real cloudbilling.APIService. -type realBillingService struct { - service *cloudbilling.APIService -} - -func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { - skus, err := skulist.All(context.Background(), r.service.Services.Skus.List(serviceID), skulist.DefaultMaxPages) - if err != nil { - return nil, fmt.Errorf("%s: list SKUs for %s: %w", "cloudsql", serviceID, err) - } - return &cloudbilling.ListSkusResponse{Skus: skus}, nil -} - // GetServiceType returns the service type. func (c *Client) GetServiceType() common.ServiceType { return common.ServiceRelationalDB @@ -272,7 +257,7 @@ type SQLPricing struct { // It returns an error when commitment pricing is absent from the catalog rather // than fabricating a price from a hardcoded discount factor (issue #1020). func (c *Client) getSQLPricing(ctx context.Context, tier, region string, termYears int) (*SQLPricing, error) { - svc, err := c.getOrCreateBillingService(ctx) + svc, err := skulist.Resolve(ctx, c.billingService, "cloudsql", c.clientOpts...) if err != nil { return nil, err } @@ -309,20 +294,6 @@ func (c *Client) getSQLPricing(ctx context.Context, tier, region string, termYea }, nil } -// getOrCreateBillingService returns the billing service, creating it if needed. -func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, error) { - if c.billingService != nil { - return c.billingService, nil - } - - service, err := cloudbilling.NewService(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create billing service: %w", err) - } - - return &realBillingService{service: service}, nil -} - // 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, termYears int) (onDemand, commitment float64, currency string, err error) { diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 55b568f6..77f5d890 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -18,6 +18,9 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) +// BillingService lists SKUs of one Cloud Billing service (enables mocking). +type BillingService = skulist.BillingService + // StorageService interface for storage operations (enables mocking). type StorageService interface { Buckets(ctx context.Context, projectID string) BucketIterator @@ -50,11 +53,6 @@ type RecommenderIterator interface { Next() (*recommenderpb.Recommendation, error) } -// BillingService interface for billing operations (enables mocking). -type BillingService interface { - ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) -} - // Client handles GCP Cloud Storage commitments. type Client struct { ctx context.Context @@ -90,19 +88,6 @@ func (c *Client) SetBillingService(svc BillingService) { c.billingService = svc } -// realBillingService wraps the real cloudbilling.APIService. -type realBillingService struct { - service *cloudbilling.APIService -} - -func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { - skus, err := skulist.All(context.Background(), r.service.Services.Skus.List(serviceID), skulist.DefaultMaxPages) - if err != nil { - return nil, fmt.Errorf("%s: list SKUs for %s: %w", "cloudstorage", serviceID, err) - } - return &cloudbilling.ListSkusResponse{Skus: skus}, nil -} - // GetServiceType returns the service type. func (c *Client) GetServiceType() common.ServiceType { return common.ServiceStorage @@ -228,7 +213,7 @@ type StoragePricing struct { // It returns an error when commitment pricing is absent from the catalog rather // than fabricating a price from a hardcoded discount factor (issue #1020). func (c *Client) getStoragePricing(ctx context.Context, storageClass, region string, termYears int) (*StoragePricing, error) { - svc, err := c.getOrCreateBillingService(ctx) + svc, err := skulist.Resolve(ctx, c.billingService, "cloudstorage", c.clientOpts...) if err != nil { return nil, err } @@ -262,20 +247,6 @@ func (c *Client) getStoragePricing(ctx context.Context, storageClass, region str }, nil } -// getOrCreateBillingService returns the billing service, creating it if needed. -func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, error) { - if c.billingService != nil { - return c.billingService, nil - } - - service, err := cloudbilling.NewService(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create billing service: %w", err) - } - - return &realBillingService{service: service}, nil -} - // extractStoragePricingFromSKUs extracts on-demand and commitment pricing from SKU list. func extractStoragePricingFromSKUs(skus []*cloudbilling.Sku, storageClass, region string, termYears int) (onDemand, commitment float64, currency string, err error) { var prices skumatch.SlotPrices diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 5922d04f..0c8e5d79 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -19,6 +19,9 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) +// BillingService lists SKUs of one Cloud Billing service (enables mocking). +type BillingService = skulist.BillingService + // RedisService interface for Redis operations. type RedisService interface { ListInstances(ctx context.Context, req *redispb.ListInstancesRequest) RedisIterator @@ -36,11 +39,6 @@ type CreateInstanceOperation interface { Wait(ctx context.Context, opts ...gax.CallOption) (*redispb.Instance, error) } -// BillingService interface for Cloud Billing operations. -type BillingService interface { - ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) -} - // RecommenderIterator interface for iterating recommendations. type RecommenderIterator interface { Next() (*recommenderpb.Recommendation, error) @@ -91,19 +89,6 @@ func (c *Client) SetBillingService(svc BillingService) { // Deprecated: Memorystore has no project-scoped commitment recommender (issue #294). func (c *Client) SetRecommenderClient(_ RecommenderClient) {} -// realBillingService wraps the actual Cloud Billing service. -type realBillingService struct { - service *cloudbilling.APIService -} - -func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { - skus, err := skulist.All(context.Background(), r.service.Services.Skus.List(serviceID), skulist.DefaultMaxPages) - if err != nil { - return nil, fmt.Errorf("%s: list SKUs for %s: %w", "memorystore", serviceID, err) - } - return &cloudbilling.ListSkusResponse{Skus: skus}, nil -} - // GetServiceType returns the service type. func (c *Client) GetServiceType() common.ServiceType { return common.ServiceCache @@ -235,7 +220,7 @@ type RedisPricing struct { // It returns an error when commitment pricing is absent from the catalog rather // than fabricating a price from a hardcoded discount factor (issue #1020). func (c *Client) getRedisPricing(ctx context.Context, tier, region string, termYears int) (*RedisPricing, error) { - billingSvc, err := c.getOrCreateBillingService(ctx) + billingSvc, err := skulist.Resolve(ctx, c.billingService, "memorystore", c.clientOpts...) if err != nil { return nil, err } @@ -272,20 +257,6 @@ func (c *Client) getRedisPricing(ctx context.Context, tier, region string, termY }, nil } -// getOrCreateBillingService returns the billing service, creating it if needed. -func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, error) { - if c.billingService != nil { - return c.billingService, nil - } - - service, err := cloudbilling.NewService(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create billing service: %w", err) - } - - return &realBillingService{service: service}, nil -} - // extractPricingFromSKUs extracts on-demand and commitment pricing from SKU list. func extractPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string, termYears int) (onDemand, commitment float64, currency string, err error) { var prices skumatch.SlotPrices From e5c29f143f9c07e0a35f3892799aff3e67a0547d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 11:55:57 +0100 Subject: [PATCH 3/4] refactor(gcp): share the SKU unit-price extractor extractSQLPriceFromSKU (cloudsql) and extractPriceFromSKU (memorystore) were identical. Replace both with skumatch.UnitPrice. cloudstorage keeps its own extractor because it also converts usage units. Co-Authored-By: Claude Opus 5.5 --- .../gcp/internal/skumatch/paid_rate_test.go | 28 +++++++++++++++++++ providers/gcp/internal/skumatch/skumatch.go | 14 ++++++++++ providers/gcp/services/cloudsql/client.go | 18 +----------- .../services/cloudsql/tiered_rates_test.go | 5 ++-- providers/gcp/services/memorystore/client.go | 18 +----------- .../memorystore/client_coverage_test.go | 11 ++++---- .../services/memorystore/tiered_rates_test.go | 5 ++-- 7 files changed, 56 insertions(+), 43 deletions(-) diff --git a/providers/gcp/internal/skumatch/paid_rate_test.go b/providers/gcp/internal/skumatch/paid_rate_test.go index d3009c3d..ccb40320 100644 --- a/providers/gcp/internal/skumatch/paid_rate_test.go +++ b/providers/gcp/internal/skumatch/paid_rate_test.go @@ -35,3 +35,31 @@ func TestPaidRate(t *testing.T) { }) } } + +func TestUnitPrice(t *testing.T) { + t.Parallel() + info := func(rates ...*cloudbilling.TierRate) []*cloudbilling.PricingInfo { + return []*cloudbilling.PricingInfo{{PricingExpression: &cloudbilling.PricingExpression{TieredRates: rates}}} + } + tests := []struct { + name string + sku *cloudbilling.Sku + wantPrice float64 + wantCurrency string + }{ + {"no pricing info", &cloudbilling.Sku{}, 0, ""}, + {"nil expression", &cloudbilling.Sku{PricingInfo: []*cloudbilling.PricingInfo{{}}}, 0, ""}, + {"all tiers free", &cloudbilling.Sku{PricingInfo: info(tier(0, 0))}, 0, ""}, + {"units and nanos", &cloudbilling.Sku{PricingInfo: info(tier(2, 500000000))}, 2.5, "USD"}, + {"free tier skipped", &cloudbilling.Sku{PricingInfo: info(tier(0, 0), tier(0, 50000000))}, 0.05, "USD"}, + {"only first pricing info is read", &cloudbilling.Sku{PricingInfo: append(info(tier(1, 0)), info(tier(9, 0))...)}, 1, "USD"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + price, currency := UnitPrice(tt.sku) + assert.InDelta(t, tt.wantPrice, price, 1e-12) + assert.Equal(t, tt.wantCurrency, currency) + }) + } +} diff --git a/providers/gcp/internal/skumatch/skumatch.go b/providers/gcp/internal/skumatch/skumatch.go index 666e6bf1..4a188968 100644 --- a/providers/gcp/internal/skumatch/skumatch.go +++ b/providers/gcp/internal/skumatch/skumatch.go @@ -124,3 +124,17 @@ func PaidRate(expr *cloudbilling.PricingExpression) *cloudbilling.TierRate { } return nil } + +// UnitPrice returns the first paid unit price of the SKU's first pricing info +// and its currency, or 0 and "" when the SKU has no paid rate. It does not +// convert usage units. +func UnitPrice(sku *cloudbilling.Sku) (price float64, currency string) { + if len(sku.PricingInfo) == 0 { + return 0, "" + } + rate := PaidRate(sku.PricingInfo[0].PricingExpression) + if rate == nil { + return 0, "" + } + return float64(rate.UnitPrice.Units) + float64(rate.UnitPrice.Nanos)/1e9, rate.UnitPrice.CurrencyCode +} diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index d0d07978..b189bf4a 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -303,7 +303,7 @@ func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string, te continue } - price, curr := extractSQLPriceFromSKU(sku) + price, curr := skumatch.UnitPrice(sku) if price == 0 { continue } @@ -329,22 +329,6 @@ func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string, te return prices.OnDemand, commitmentPrice, currency, nil } -// extractSQLPriceFromSKU extracts the unit price from a SKU. -func extractSQLPriceFromSKU(sku *cloudbilling.Sku) (price float64, currency string) { - if len(sku.PricingInfo) == 0 { - return 0, "" - } - - pricingInfo := sku.PricingInfo[0] - rate := skumatch.PaidRate(pricingInfo.PricingExpression) - if rate == nil { - return 0, "" - } - - price = float64(rate.UnitPrice.Units) + float64(rate.UnitPrice.Nanos)/1e9 - return price, rate.UnitPrice.CurrencyCode -} - // contains checks if a slice contains a string. func contains(slice []string, str string) bool { for _, s := range slice { diff --git a/providers/gcp/services/cloudsql/tiered_rates_test.go b/providers/gcp/services/cloudsql/tiered_rates_test.go index f61d771d..e72b772a 100644 --- a/providers/gcp/services/cloudsql/tiered_rates_test.go +++ b/providers/gcp/services/cloudsql/tiered_rates_test.go @@ -3,6 +3,7 @@ package cloudsql import ( "testing" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/api/cloudbilling/v1" @@ -22,7 +23,7 @@ func TestExtractSQLPricing_FreeFirstTierUsesPaidTier(t *testing.T) { const tier, region = "db-n1-standard-1", "us-central1" od := freeFirstTierSKU("od", tier+" Cloud SQL", "OnDemand", region, 50000000) - price, currency := extractSQLPriceFromSKU(od) + price, currency := skumatch.UnitPrice(od) assert.InDelta(t, 0.05, price, 1e-12) assert.Equal(t, "USD", currency) @@ -39,6 +40,6 @@ func TestExtractSQLPricing_FreeFirstTierUsesPaidTier(t *testing.T) { func TestExtractSQLPricing_AllTiersFreeIsSkipped(t *testing.T) { t.Parallel() sku := freeFirstTierSKU("od", "db-n1-standard-1 Cloud SQL", "OnDemand", "us-central1", 0) - price, _ := extractSQLPriceFromSKU(sku) + price, _ := skumatch.UnitPrice(sku) assert.Zero(t, price) } diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 0c8e5d79..7209c033 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -265,7 +265,7 @@ func extractPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string, termY continue } - price, curr := extractPriceFromSKU(sku) + price, curr := skumatch.UnitPrice(sku) if price == 0 { continue } @@ -291,22 +291,6 @@ func extractPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string, termY return prices.OnDemand, commitmentPrice, currency, nil } -// extractPriceFromSKU extracts the unit price from a SKU. -func extractPriceFromSKU(sku *cloudbilling.Sku) (price float64, currency string) { - if len(sku.PricingInfo) == 0 { - return 0, "" - } - - pricingInfo := sku.PricingInfo[0] - rate := skumatch.PaidRate(pricingInfo.PricingExpression) - if rate == nil { - return 0, "" - } - - price = float64(rate.UnitPrice.Units) + float64(rate.UnitPrice.Nanos)/1e9 - return price, rate.UnitPrice.CurrencyCode -} - // memorystoreRedisServiceID is the Cloud Billing service for Cloud Memorystore for Redis. const memorystoreRedisServiceID = "services/5AF5-2C11-D467" diff --git a/providers/gcp/services/memorystore/client_coverage_test.go b/providers/gcp/services/memorystore/client_coverage_test.go index c5bbfc2c..658eb2b6 100644 --- a/providers/gcp/services/memorystore/client_coverage_test.go +++ b/providers/gcp/services/memorystore/client_coverage_test.go @@ -3,6 +3,7 @@ package memorystore import ( "testing" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/api/cloudbilling/v1" @@ -28,7 +29,7 @@ func TestExtractPriceFromSKU_ValidPrice(t *testing.T) { }, } - price, currency := extractPriceFromSKU(sku) + price, currency := skumatch.UnitPrice(sku) assert.Equal(t, 1.5, price) assert.Equal(t, "USD", currency) } @@ -39,7 +40,7 @@ func TestExtractPriceFromSKU_NoPricingInfo(t *testing.T) { PricingInfo: []*cloudbilling.PricingInfo{}, } - price, currency := extractPriceFromSKU(sku) + price, currency := skumatch.UnitPrice(sku) assert.Equal(t, 0.0, price) assert.Equal(t, "", currency) } @@ -54,7 +55,7 @@ func TestExtractPriceFromSKU_NoPricingExpression(t *testing.T) { }, } - price, currency := extractPriceFromSKU(sku) + price, currency := skumatch.UnitPrice(sku) assert.Equal(t, 0.0, price) assert.Equal(t, "", currency) } @@ -71,7 +72,7 @@ func TestExtractPriceFromSKU_NoTieredRates(t *testing.T) { }, } - price, currency := extractPriceFromSKU(sku) + price, currency := skumatch.UnitPrice(sku) assert.Equal(t, 0.0, price) assert.Equal(t, "", currency) } @@ -92,7 +93,7 @@ func TestExtractPriceFromSKU_NoUnitPrice(t *testing.T) { }, } - price, currency := extractPriceFromSKU(sku) + price, currency := skumatch.UnitPrice(sku) assert.Equal(t, 0.0, price) assert.Equal(t, "", currency) } diff --git a/providers/gcp/services/memorystore/tiered_rates_test.go b/providers/gcp/services/memorystore/tiered_rates_test.go index b340b865..da9e5297 100644 --- a/providers/gcp/services/memorystore/tiered_rates_test.go +++ b/providers/gcp/services/memorystore/tiered_rates_test.go @@ -3,6 +3,7 @@ package memorystore import ( "testing" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/api/cloudbilling/v1" @@ -22,7 +23,7 @@ func TestExtractPricing_FreeFirstTierUsesPaidTier(t *testing.T) { const tier, region = "STANDARD_HA", "us-central1" od := freeFirstTierSKU("od", tier+" Memorystore", "OnDemand", region, 50000000) - price, currency := extractPriceFromSKU(od) + price, currency := skumatch.UnitPrice(od) assert.InDelta(t, 0.05, price, 1e-12) assert.Equal(t, "USD", currency) @@ -39,6 +40,6 @@ func TestExtractPricing_FreeFirstTierUsesPaidTier(t *testing.T) { func TestExtractPricing_AllTiersFreeIsSkipped(t *testing.T) { t.Parallel() sku := freeFirstTierSKU("od", "STANDARD_HA Memorystore", "OnDemand", "us-central1", 0) - price, _ := extractPriceFromSKU(sku) + price, _ := skumatch.UnitPrice(sku) assert.Zero(t, price) } From 9575b1689c984da1d97de1d13608c3df64c6837a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 11:59:04 +0100 Subject: [PATCH 4/4] fix(gcp): reject non-positive on-demand and commitment prices The pricing guards in cloudsql, memorystore and cloudstorage rejected only zero. A negative catalog price passed them and flipped the sign of the savings percentage. Reject <= 0 and fail closed; one test per service. Co-Authored-By: Claude Opus 5.5 --- providers/gcp/services/cloudsql/client.go | 4 +- .../services/cloudsql/negative_price_test.go | 55 +++++++++++++++++++ providers/gcp/services/cloudstorage/client.go | 4 +- .../cloudstorage/negative_price_test.go | 38 +++++++++++++ providers/gcp/services/memorystore/client.go | 4 +- .../memorystore/negative_price_test.go | 55 +++++++++++++++++++ 6 files changed, 154 insertions(+), 6 deletions(-) create mode 100644 providers/gcp/services/cloudsql/negative_price_test.go create mode 100644 providers/gcp/services/cloudstorage/negative_price_test.go create mode 100644 providers/gcp/services/memorystore/negative_price_test.go diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index b189bf4a..f6d85465 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -271,10 +271,10 @@ func (c *Client) getSQLPricing(ctx context.Context, tier, region string, termYea if err != nil { return nil, err } - if onDemandPrice == 0 { + if onDemandPrice <= 0 { return nil, fmt.Errorf("no pricing found for Cloud SQL tier %s", tier) } - if commitmentPrice == 0 { + if commitmentPrice <= 0 { return nil, fmt.Errorf("no commitment pricing found for Cloud SQL tier %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", tier, region) } diff --git a/providers/gcp/services/cloudsql/negative_price_test.go b/providers/gcp/services/cloudsql/negative_price_test.go new file mode 100644 index 00000000..dda4113e --- /dev/null +++ b/providers/gcp/services/cloudsql/negative_price_test.go @@ -0,0 +1,55 @@ +package cloudsql + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +func negPriceSkus(onDemandNanos, commitNanos int64) []*cloudbilling.Sku { + sku := func(desc, usage string, nanos int64) *cloudbilling.Sku { + return &cloudbilling.Sku{ + Description: desc, + Category: &cloudbilling.Category{UsageType: usage}, + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{{PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{{UnitPrice: &cloudbilling.Money{CurrencyCode: "USD", Nanos: nanos}}}, + }}}, + } + } + return []*cloudbilling.Sku{ + sku("db-n1-standard-1 Cloud SQL", "OnDemand", onDemandNanos), + sku("db-n1-standard-1 Cloud SQL commitment", "Commit1Yr", commitNanos), + } +} + +// A negative price would flip the sign of the savings percentage, so pricing +// must fail closed instead of reporting it. +func TestGetSQLPricing_RejectsNegativePrices(t *testing.T) { + t.Parallel() + tests := []struct { + name string + onDemand, commit int64 + wantErr string + }{ + {"negative on-demand", -50000000, 42000000, "no pricing found"}, + {"negative commitment", 50000000, -42000000, "no commitment pricing found"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, err := NewClient(ctx, "test-project", "us-central1") + require.NoError(t, err) + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: negPriceSkus(tt.onDemand, tt.commit)}}) + + pricing, err := client.getSQLPricing(ctx, "db-n1-standard-1", "us-central1", 1) + require.Error(t, err) + assert.Nil(t, pricing) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 77f5d890..7e1e60bf 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -227,10 +227,10 @@ func (c *Client) getStoragePricing(ctx context.Context, storageClass, region str if err != nil { return nil, err } - if onDemandPrice == 0 { + if onDemandPrice <= 0 { return nil, fmt.Errorf("no pricing found for Cloud Storage class %s", storageClass) } - if commitmentPrice == 0 { + if commitmentPrice <= 0 { return nil, fmt.Errorf("no commitment pricing found for Cloud Storage class %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", storageClass, region) } diff --git a/providers/gcp/services/cloudstorage/negative_price_test.go b/providers/gcp/services/cloudstorage/negative_price_test.go new file mode 100644 index 00000000..2a52b628 --- /dev/null +++ b/providers/gcp/services/cloudstorage/negative_price_test.go @@ -0,0 +1,38 @@ +package cloudstorage + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// A negative price would flip the sign of the savings percentage, so pricing +// must fail closed instead of reporting it. +func TestGetStoragePricing_RejectsNegativePrices(t *testing.T) { + t.Parallel() + tests := []struct { + name string + onDemand, commit int64 + wantErr string + }{ + {"negative on-demand", -50000000, 42000000, "no pricing found"}, + {"negative commitment", 50000000, -42000000, "no commitment pricing found"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, err := NewClient(ctx, "test-project", "us-central1") + require.NoError(t, err) + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: storageMockSkus("STANDARD", tt.onDemand, tt.commit)}}) + + pricing, err := client.getStoragePricing(ctx, "STANDARD", "us-central1", 1) + require.Error(t, err) + assert.Nil(t, pricing) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 7209c033..3e53664d 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -234,10 +234,10 @@ func (c *Client) getRedisPricing(ctx context.Context, tier, region string, termY if err != nil { return nil, err } - if onDemandPrice == 0 { + if onDemandPrice <= 0 { return nil, fmt.Errorf("no pricing found for Memorystore tier %s", tier) } - if commitmentPrice == 0 { + if commitmentPrice <= 0 { return nil, fmt.Errorf("no commitment pricing found for Memorystore tier %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", tier, region) } diff --git a/providers/gcp/services/memorystore/negative_price_test.go b/providers/gcp/services/memorystore/negative_price_test.go new file mode 100644 index 00000000..060b4976 --- /dev/null +++ b/providers/gcp/services/memorystore/negative_price_test.go @@ -0,0 +1,55 @@ +package memorystore + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +func negPriceSkus(onDemandNanos, commitNanos int64) []*cloudbilling.Sku { + sku := func(desc, usage string, nanos int64) *cloudbilling.Sku { + return &cloudbilling.Sku{ + Description: desc, + Category: &cloudbilling.Category{UsageType: usage}, + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{{PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{{UnitPrice: &cloudbilling.Money{CurrencyCode: "USD", Nanos: nanos}}}, + }}}, + } + } + return []*cloudbilling.Sku{ + sku("Memorystore Redis STANDARD_HA instance", "OnDemand", onDemandNanos), + sku("Memorystore Redis STANDARD_HA commitment 1yr", "Commit1Yr", commitNanos), + } +} + +// A negative price would flip the sign of the savings percentage, so pricing +// must fail closed instead of reporting it. +func TestGetRedisPricing_RejectsNegativePrices(t *testing.T) { + t.Parallel() + tests := []struct { + name string + onDemand, commit int64 + wantErr string + }{ + {"negative on-demand", -50000000, 42000000, "no pricing found"}, + {"negative commitment", 50000000, -42000000, "no commitment pricing found"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, err := NewClient(ctx, "test-project", "us-central1") + require.NoError(t, err) + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: negPriceSkus(tt.onDemand, tt.commit)}}) + + pricing, err := client.getRedisPricing(ctx, "STANDARD_HA", "us-central1", 1) + require.Error(t, err) + assert.Nil(t, pricing) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +}