From 962f7a3c02b217f228e7083705235d400afd6b14 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:49:08 +0200 Subject: [PATCH 1/4] fix(gcp): paginate Cloud SQL and Memorystore SKU listing and cap recommendations by page The real billing wrapper read one SKU page and dropped nextPageToken. The recommender loop compared a per-item counter to maxRecsPages, so more than 20 recommendations hard-errored. Request an explicit page size and cap items at pages*pageSize; follow nextPageToken for SKUs with a page cap. Co-Authored-By: Claude Opus 5.5 --- providers/gcp/services/cloudsql/client.go | 51 ++++++++++-- .../gcp/services/cloudsql/pagination_test.go | 77 +++++++++++++++++++ providers/gcp/services/memorystore/client.go | 51 ++++++++++-- .../services/memorystore/pagination_test.go | 77 +++++++++++++++++++ 4 files changed, 240 insertions(+), 16 deletions(-) create mode 100644 providers/gcp/services/cloudsql/pagination_test.go create mode 100644 providers/gcp/services/memorystore/pagination_test.go diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 52038182..5a689f10 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -21,8 +21,25 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) -// maxRecsPages caps GCP Recommender API iteration. -const maxRecsPages = 20 +// recsPageSize is the page size requested from the Recommender API, and +// maxRecsPages caps how many pages one listing may span. The iterator hands +// out items, so the item cap is the product of the two. +const ( + recsPageSize = 100 + maxRecsPages = 20 + maxRecsItems = recsPageSize * maxRecsPages +) + +// checkRecsCap fails loud when a successful Next() would exceed maxRecsItems. +func checkRecsCap(itemIdx int, nextErr error) error { + if nextErr == nil && itemIdx >= maxRecsItems { + return fmt.Errorf("cloudsql: GetRecommendations cap reached: more than %d recommendations (%d pages of %d)", maxRecsItems, maxRecsPages, recsPageSize) + } + return nil +} + +// maxSKUPages caps Cloud Billing SKU listing pagination. +const maxSKUPages = 200 // SQLAdminService interface for SQL admin operations (enables mocking). type SQLAdminService interface { @@ -106,7 +123,24 @@ type realBillingService struct { } func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { - return r.service.Services.Skus.List(serviceID).Do() + merged := &cloudbilling.ListSkusResponse{} + pageToken := "" + for page := 0; page < maxSKUPages; page++ { + call := r.service.Services.Skus.List(serviceID) + if pageToken != "" { + call = call.PageToken(pageToken) + } + resp, err := call.Do() + if err != nil { + return nil, err + } + merged.Skus = append(merged.Skus, resp.Skus...) + if resp.NextPageToken == "" { + return merged, nil + } + pageToken = resp.NextPageToken + } + return nil, fmt.Errorf("%s: SKU listing for %s exceeded %d pages", "cloudsql", serviceID, maxSKUPages) } // realRecommenderIterator wraps the real recommender iterator. @@ -173,21 +207,22 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio c.projectID, c.region) req := &recommenderpb.ListRecommendationsRequest{ - Parent: parent, + Parent: parent, + PageSize: recsPageSize, } it := recClient.ListRecommendations(ctx, req) - for pageIdx := 0; ; pageIdx++ { + for itemIdx := 0; ; itemIdx++ { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("context canceled during pagination: %w", err) } - if pageIdx >= maxRecsPages { - return nil, fmt.Errorf("cloudsql: GetRecommendations iteration cap (%d items) reached", maxRecsPages) - } rec, err := it.Next() if errors.Is(err, iterator.Done) { break } + if capErr := checkRecsCap(itemIdx, err); capErr != nil { + return nil, capErr + } if err != nil { // Iterator errors must propagate so callers don't silently act // on a partial recommendation list -- see the computeengine diff --git a/providers/gcp/services/cloudsql/pagination_test.go b/providers/gcp/services/cloudsql/pagination_test.go new file mode 100644 index 00000000..6485c9d2 --- /dev/null +++ b/providers/gcp/services/cloudsql/pagination_test.go @@ -0,0 +1,77 @@ +package cloudsql + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "cloud.google.com/go/recommender/apiv1/recommenderpb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" + "google.golang.org/api/option" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func activeRecs(n int) []*recommenderpb.Recommendation { + recs := make([]*recommenderpb.Recommendation, n) + for i := range recs { + recs[i] = &recommenderpb.Recommendation{ + StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE}, + } + } + return recs +} + +// A listing that spans several pages but stays under the page cap must be +// returned in full. Before the fix the cap applied per item (20), so any +// region with more than 20 recommendations failed outright. +func TestGetRecommendations_ManyItemsWithinPageCap(t *testing.T) { + n := 3 * recsPageSize + c, _ := NewClient(context.Background(), "p", "us-central1") + c.SetRecommenderClient(&MockRecommenderClient{iterator: &MockRecommenderIterator{recommendations: activeRecs(n)}}) + c.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{}}) + + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + assert.Len(t, recs, n) +} + +// A genuine cap breach still fails loud. +func TestGetRecommendations_PageCapBreachFails(t *testing.T) { + c, _ := NewClient(context.Background(), "p", "us-central1") + c.SetRecommenderClient(&MockRecommenderClient{iterator: &MockRecommenderIterator{recommendations: activeRecs(maxRecsItems + 1)}}) + c.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{}}) + + _, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "cap reached") +} + +func TestRealBillingService_ListSKUs_FollowsNextPageToken(t *testing.T) { + var tokens []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tok := r.URL.Query().Get("pageToken") + tokens = append(tokens, tok) + w.Header().Set("Content-Type", "application/json") + switch tok { + case "": + fmt.Fprint(w, `{"skus":[{"skuId":"A"}],"nextPageToken":"t2"}`) + case "t2": + fmt.Fprint(w, `{"skus":[{"skuId":"B"}],"nextPageToken":"t3"}`) + default: + fmt.Fprint(w, `{"skus":[{"skuId":"C"}]}`) + } + })) + defer srv.Close() + + api, err := cloudbilling.NewService(context.Background(), option.WithEndpoint(srv.URL), option.WithoutAuthentication()) + require.NoError(t, err) + resp, err := (&realBillingService{service: api}).ListSKUs("services/x") + require.NoError(t, err) + require.Len(t, resp.Skus, 3) + assert.Equal(t, []string{"", "t2", "t3"}, tokens) +} diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 75693c66..2caec2fb 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -22,8 +22,25 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) -// maxRecsPages caps GCP Recommender API iteration. -const maxRecsPages = 20 +// recsPageSize is the page size requested from the Recommender API, and +// maxRecsPages caps how many pages one listing may span. The iterator hands +// out items, so the item cap is the product of the two. +const ( + recsPageSize = 100 + maxRecsPages = 20 + maxRecsItems = recsPageSize * maxRecsPages +) + +// checkRecsCap fails loud when a successful Next() would exceed maxRecsItems. +func checkRecsCap(itemIdx int, nextErr error) error { + if nextErr == nil && itemIdx >= maxRecsItems { + return fmt.Errorf("memorystore: GetRecommendations cap reached: more than %d recommendations (%d pages of %d)", maxRecsItems, maxRecsPages, recsPageSize) + } + return nil +} + +// maxSKUPages caps Cloud Billing SKU listing pagination. +const maxSKUPages = 200 // RedisService interface for Redis operations. type RedisService interface { @@ -100,7 +117,24 @@ type realBillingService struct { } func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { - return r.service.Services.Skus.List(serviceID).Do() + merged := &cloudbilling.ListSkusResponse{} + pageToken := "" + for page := 0; page < maxSKUPages; page++ { + call := r.service.Services.Skus.List(serviceID) + if pageToken != "" { + call = call.PageToken(pageToken) + } + resp, err := call.Do() + if err != nil { + return nil, err + } + merged.Skus = append(merged.Skus, resp.Skus...) + if resp.NextPageToken == "" { + return merged, nil + } + pageToken = resp.NextPageToken + } + return nil, fmt.Errorf("%s: SKU listing for %s exceeded %d pages", "memorystore", serviceID, maxSKUPages) } // realRecommenderIterator wraps the real recommender iterator (10-L4: makes @@ -168,21 +202,22 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio c.projectID, c.region) req := &recommenderpb.ListRecommendationsRequest{ - Parent: parent, + Parent: parent, + PageSize: recsPageSize, } it := recClient.ListRecommendations(ctx, req) - for pageIdx := 0; ; pageIdx++ { + for itemIdx := 0; ; itemIdx++ { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("context canceled during pagination: %w", err) } - if pageIdx >= maxRecsPages { - return nil, fmt.Errorf("memorystore: GetRecommendations iteration cap (%d items) reached", maxRecsPages) - } rec, err := it.Next() if errors.Is(err, iterator.Done) { break } + if capErr := checkRecsCap(itemIdx, err); capErr != nil { + return nil, capErr + } if err != nil { // Iterator errors must propagate so callers don't silently act // on a partial recommendation list -- see the computeengine diff --git a/providers/gcp/services/memorystore/pagination_test.go b/providers/gcp/services/memorystore/pagination_test.go new file mode 100644 index 00000000..f10e0b4c --- /dev/null +++ b/providers/gcp/services/memorystore/pagination_test.go @@ -0,0 +1,77 @@ +package memorystore + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "cloud.google.com/go/recommender/apiv1/recommenderpb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" + "google.golang.org/api/option" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func activeRecs(n int) []*recommenderpb.Recommendation { + recs := make([]*recommenderpb.Recommendation, n) + for i := range recs { + recs[i] = &recommenderpb.Recommendation{ + StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE}, + } + } + return recs +} + +// A listing that spans several pages but stays under the page cap must be +// returned in full. Before the fix the cap applied per item (20), so any +// region with more than 20 recommendations failed outright. +func TestGetRecommendations_ManyItemsWithinPageCap(t *testing.T) { + n := 3 * recsPageSize + c, _ := NewClient(context.Background(), "p", "us-central1") + c.SetRecommenderClient(&MockRecommenderClient{recommendations: activeRecs(n)}) + c.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{}}) + + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + assert.Len(t, recs, n) +} + +// A genuine cap breach still fails loud. +func TestGetRecommendations_PageCapBreachFails(t *testing.T) { + c, _ := NewClient(context.Background(), "p", "us-central1") + c.SetRecommenderClient(&MockRecommenderClient{recommendations: activeRecs(maxRecsItems + 1)}) + c.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{}}) + + _, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "cap reached") +} + +func TestRealBillingService_ListSKUs_FollowsNextPageToken(t *testing.T) { + var tokens []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tok := r.URL.Query().Get("pageToken") + tokens = append(tokens, tok) + w.Header().Set("Content-Type", "application/json") + switch tok { + case "": + fmt.Fprint(w, `{"skus":[{"skuId":"A"}],"nextPageToken":"t2"}`) + case "t2": + fmt.Fprint(w, `{"skus":[{"skuId":"B"}],"nextPageToken":"t3"}`) + default: + fmt.Fprint(w, `{"skus":[{"skuId":"C"}]}`) + } + })) + defer srv.Close() + + api, err := cloudbilling.NewService(context.Background(), option.WithEndpoint(srv.URL), option.WithoutAuthentication()) + require.NoError(t, err) + resp, err := (&realBillingService{service: api}).ListSKUs("services/x") + require.NoError(t, err) + require.Len(t, resp.Skus, 3) + assert.Equal(t, []string{"", "t2", "t3"}, tokens) +} From a445cfe42896014269f273c19ff687033d7a67be Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:54:58 +0200 Subject: [PATCH 2/4] fix(gcp): share one fail-loud SKU paging helper and tighten pagination tests Add internal/skulist.All (Pages-based, page cap errors instead of returning a partial list) and use it from cloudsql, memorystore and cloudstorage. Cache the SKU catalog per client so pricing N recommendations lists it once. Test the cap, the requested PageSize, the exact-cap boundary and the cache. Co-Authored-By: Claude Opus 5.5 --- providers/gcp/internal/skulist/skulist.go | 32 +++++++ .../gcp/internal/skulist/skulist_test.go | 54 ++++++++++++ providers/gcp/services/cloudsql/client.go | 52 ++++++------ .../gcp/services/cloudsql/pagination_test.go | 85 ++++++++++++------- providers/gcp/services/cloudstorage/client.go | 7 +- providers/gcp/services/memorystore/client.go | 52 ++++++------ .../services/memorystore/pagination_test.go | 85 ++++++++++++------- 7 files changed, 260 insertions(+), 107 deletions(-) create mode 100644 providers/gcp/internal/skulist/skulist.go create mode 100644 providers/gcp/internal/skulist/skulist_test.go diff --git a/providers/gcp/internal/skulist/skulist.go b/providers/gcp/internal/skulist/skulist.go new file mode 100644 index 00000000..e9a5fe82 --- /dev/null +++ b/providers/gcp/internal/skulist/skulist.go @@ -0,0 +1,32 @@ +// Package skulist reads every page of a Cloud Billing catalog SKU listing. +package skulist + +import ( + "context" + "fmt" + + "google.golang.org/api/cloudbilling/v1" +) + +// DefaultMaxPages bounds one listing. The largest catalog service seen live +// (Compute Engine) spans 7 pages of 5000 SKUs. +const DefaultMaxPages = 200 + +// All returns the SKUs of every page of call. It errors, never returning a +// partial list, if the listing needs more than maxPages pages. +func All(ctx context.Context, call *cloudbilling.ServicesSkusListCall, maxPages int) ([]*cloudbilling.Sku, error) { + var skus []*cloudbilling.Sku + pages := 0 + err := call.Pages(ctx, func(resp *cloudbilling.ListSkusResponse) error { + pages++ + if pages > maxPages { + return fmt.Errorf("SKU listing exceeded %d pages", maxPages) + } + skus = append(skus, resp.Skus...) + return nil + }) + if err != nil { + return nil, err + } + return skus, nil +} diff --git a/providers/gcp/internal/skulist/skulist_test.go b/providers/gcp/internal/skulist/skulist_test.go new file mode 100644 index 00000000..9afe9ef9 --- /dev/null +++ b/providers/gcp/internal/skulist/skulist_test.go @@ -0,0 +1,54 @@ +package skulist + +import ( + "context" + "fmt" + "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" +) + +func newCall(t *testing.T, h http.HandlerFunc) *cloudbilling.ServicesSkusListCall { + srv := httptest.NewServer(h) + t.Cleanup(srv.Close) + api, err := cloudbilling.NewService(context.Background(), option.WithEndpoint(srv.URL), option.WithoutAuthentication()) + require.NoError(t, err) + return api.Services.Skus.List("services/x") +} + +func TestAll_FollowsNextPageToken(t *testing.T) { + var tokens []string + call := newCall(t, func(w http.ResponseWriter, r *http.Request) { + tok := r.URL.Query().Get("pageToken") + tokens = append(tokens, tok) + w.Header().Set("Content-Type", "application/json") + switch tok { + case "": + fmt.Fprint(w, `{"skus":[{"skuId":"A"}],"nextPageToken":"t2"}`) + case "t2": + fmt.Fprint(w, `{"skus":[{"skuId":"B"}],"nextPageToken":"t3"}`) + default: + fmt.Fprint(w, `{"skus":[{"skuId":"C"}]}`) + } + }) + skus, err := All(context.Background(), call, 3) + require.NoError(t, err) + require.Len(t, skus, 3) + assert.Equal(t, []string{"", "t2", "t3"}, tokens) +} + +func TestAll_ExceedingCapErrorsInsteadOfReturningPartialList(t *testing.T) { + call := newCall(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"skus":[{"skuId":"A"}],"nextPageToken":"more"}`) + }) + skus, err := All(context.Background(), call, 3) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeded 3 pages") + assert.Nil(t, skus) +} diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 5a689f10..5980c377 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -7,6 +7,7 @@ import ( "fmt" "log" "strings" + "sync" "time" recommender "cloud.google.com/go/recommender/apiv1" @@ -18,12 +19,13 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/billingcurrency" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skulist" "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) -// recsPageSize is the page size requested from the Recommender API, and -// maxRecsPages caps how many pages one listing may span. The iterator hands -// out items, so the item cap is the product of the two. +// recsPageSize is the page size requested from the Recommender API. The +// iterator hands out items, so the cap is maxRecsPages pages of recsPageSize +// items, i.e. maxRecsItems (2000) items in total. const ( recsPageSize = 100 maxRecsPages = 20 @@ -38,9 +40,6 @@ func checkRecsCap(itemIdx int, nextErr error) error { return nil } -// maxSKUPages caps Cloud Billing SKU listing pagination. -const maxSKUPages = 200 - // SQLAdminService interface for SQL admin operations (enables mocking). type SQLAdminService interface { ListInstances(projectID string) (*sqladmin.InstancesListResponse, error) @@ -66,6 +65,8 @@ type RecommenderClient interface { // Client handles GCP Cloud SQL commitments. type Client struct { + skuMu sync.Mutex + skuCache *cloudbilling.ListSkusResponse ctx context.Context projectID string region string @@ -123,24 +124,11 @@ type realBillingService struct { } func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { - merged := &cloudbilling.ListSkusResponse{} - pageToken := "" - for page := 0; page < maxSKUPages; page++ { - call := r.service.Services.Skus.List(serviceID) - if pageToken != "" { - call = call.PageToken(pageToken) - } - resp, err := call.Do() - if err != nil { - return nil, err - } - merged.Skus = append(merged.Skus, resp.Skus...) - if resp.NextPageToken == "" { - return merged, nil - } - pageToken = resp.NextPageToken + 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 nil, fmt.Errorf("%s: SKU listing for %s exceeded %d pages", "cloudsql", serviceID, maxSKUPages) + return &cloudbilling.ListSkusResponse{Skus: skus}, nil } // realRecommenderIterator wraps the real recommender iterator. @@ -385,7 +373,7 @@ func (c *Client) getSQLPricing(ctx context.Context, tier, region string, termYea return nil, err } - skus, err := svc.ListSKUs("services/9662-B51E-5089") + skus, err := c.listSKUs(svc) if err != nil { return nil, fmt.Errorf("failed to list SKUs: %w", err) } @@ -617,3 +605,19 @@ func contains(slice []string, str string) bool { } return false } + +// listSKUs returns the service's SKU catalog, listing it once per client so +// that pricing N recommendations costs one catalog read instead of N. +func (c *Client) listSKUs(svc BillingService) (*cloudbilling.ListSkusResponse, error) { + c.skuMu.Lock() + defer c.skuMu.Unlock() + if c.skuCache != nil { + return c.skuCache, nil + } + skus, err := svc.ListSKUs("services/9662-B51E-5089") + if err != nil { + return nil, err + } + c.skuCache = skus + return skus, nil +} diff --git a/providers/gcp/services/cloudsql/pagination_test.go b/providers/gcp/services/cloudsql/pagination_test.go index 6485c9d2..47fc0c4f 100644 --- a/providers/gcp/services/cloudsql/pagination_test.go +++ b/providers/gcp/services/cloudsql/pagination_test.go @@ -2,16 +2,12 @@ package cloudsql import ( "context" - "fmt" - "net/http" - "net/http/httptest" "testing" "cloud.google.com/go/recommender/apiv1/recommenderpb" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/api/cloudbilling/v1" - "google.golang.org/api/option" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" ) @@ -40,38 +36,69 @@ func TestGetRecommendations_ManyItemsWithinPageCap(t *testing.T) { assert.Len(t, recs, n) } -// A genuine cap breach still fails loud. -func TestGetRecommendations_PageCapBreachFails(t *testing.T) { +// Exactly maxRecsItems recommendations are within the cap; the old per-item +// check rejected anything past 20. +func TestGetRecommendations_ExactlyAtCapSucceeds(t *testing.T) { + c, _ := NewClient(context.Background(), "p", "us-central1") + c.SetRecommenderClient(&MockRecommenderClient{iterator: &MockRecommenderIterator{recommendations: activeRecs(maxRecsItems)}}) + c.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{}}) + + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + assert.Len(t, recs, maxRecsItems) +} + +// A genuine cap breach still fails loud and returns no partial list. +func TestGetRecommendations_PastCapFails(t *testing.T) { c, _ := NewClient(context.Background(), "p", "us-central1") c.SetRecommenderClient(&MockRecommenderClient{iterator: &MockRecommenderIterator{recommendations: activeRecs(maxRecsItems + 1)}}) c.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{}}) - _, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.Error(t, err) - assert.Contains(t, err.Error(), "cap reached") + assert.Nil(t, recs) } -func TestRealBillingService_ListSKUs_FollowsNextPageToken(t *testing.T) { - var tokens []string - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tok := r.URL.Query().Get("pageToken") - tokens = append(tokens, tok) - w.Header().Set("Content-Type", "application/json") - switch tok { - case "": - fmt.Fprint(w, `{"skus":[{"skuId":"A"}],"nextPageToken":"t2"}`) - case "t2": - fmt.Fprint(w, `{"skus":[{"skuId":"B"}],"nextPageToken":"t3"}`) - default: - fmt.Fprint(w, `{"skus":[{"skuId":"C"}]}`) - } - })) - defer srv.Close() +// The cap is expressed in pages, so the request must ask for the page size +// the cap assumes. +type reqCapture struct { + RecommenderClient + req *recommenderpb.ListRecommendationsRequest +} - api, err := cloudbilling.NewService(context.Background(), option.WithEndpoint(srv.URL), option.WithoutAuthentication()) - require.NoError(t, err) - resp, err := (&realBillingService{service: api}).ListSKUs("services/x") +func (r *reqCapture) ListRecommendations(ctx context.Context, req *recommenderpb.ListRecommendationsRequest) RecommenderIterator { + r.req = req + return r.RecommenderClient.ListRecommendations(ctx, req) +} + +func TestGetRecommendations_RequestsExplicitPageSize(t *testing.T) { + c, _ := NewClient(context.Background(), "p", "us-central1") + capture := &reqCapture{RecommenderClient: &MockRecommenderClient{iterator: &MockRecommenderIterator{recommendations: nil}}} + c.SetRecommenderClient(capture) + + _, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) - require.Len(t, resp.Skus, 3) - assert.Equal(t, []string{"", "t2", "t3"}, tokens) + require.NotNil(t, capture.req) + assert.EqualValues(t, recsPageSize, capture.req.PageSize) +} + +// Pricing N recommendations lists the SKU catalog once. +type countingBilling struct { + BillingService + calls int +} + +func (b *countingBilling) ListSKUs(id string) (*cloudbilling.ListSkusResponse, error) { + b.calls++ + return b.BillingService.ListSKUs(id) +} + +func TestListSKUs_CachedPerClient(t *testing.T) { + b := &countingBilling{BillingService: &MockBillingService{skus: &cloudbilling.ListSkusResponse{}}} + c, _ := NewClient(context.Background(), "p", "us-central1") + for i := 0; i < 3; i++ { + _, err := c.listSKUs(b) + require.NoError(t, err) + } + assert.Equal(t, 1, b.calls) } diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index a88f04aa..43e95b54 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -13,6 +13,7 @@ import ( "google.golang.org/api/option" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skulist" "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) @@ -97,7 +98,11 @@ type realBillingService struct { } func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { - return r.service.Services.Skus.List(serviceID).Do() + 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. diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 2caec2fb..9315ac06 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -7,6 +7,7 @@ import ( "fmt" "log" "strings" + "sync" "time" recommender "cloud.google.com/go/recommender/apiv1" @@ -19,12 +20,13 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/billingcurrency" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skulist" "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) -// recsPageSize is the page size requested from the Recommender API, and -// maxRecsPages caps how many pages one listing may span. The iterator hands -// out items, so the item cap is the product of the two. +// recsPageSize is the page size requested from the Recommender API. The +// iterator hands out items, so the cap is maxRecsPages pages of recsPageSize +// items, i.e. maxRecsItems (2000) items in total. const ( recsPageSize = 100 maxRecsPages = 20 @@ -39,9 +41,6 @@ func checkRecsCap(itemIdx int, nextErr error) error { return nil } -// maxSKUPages caps Cloud Billing SKU listing pagination. -const maxSKUPages = 200 - // RedisService interface for Redis operations. type RedisService interface { ListInstances(ctx context.Context, req *redispb.ListInstancesRequest) RedisIterator @@ -77,6 +76,8 @@ type RecommenderClient interface { // Client handles GCP Memorystore (Redis) commitments. type Client struct { + skuMu sync.Mutex + skuCache *cloudbilling.ListSkusResponse ctx context.Context projectID string region string @@ -117,24 +118,11 @@ type realBillingService struct { } func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { - merged := &cloudbilling.ListSkusResponse{} - pageToken := "" - for page := 0; page < maxSKUPages; page++ { - call := r.service.Services.Skus.List(serviceID) - if pageToken != "" { - call = call.PageToken(pageToken) - } - resp, err := call.Do() - if err != nil { - return nil, err - } - merged.Skus = append(merged.Skus, resp.Skus...) - if resp.NextPageToken == "" { - return merged, nil - } - pageToken = resp.NextPageToken + 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 nil, fmt.Errorf("%s: SKU listing for %s exceeded %d pages", "memorystore", serviceID, maxSKUPages) + return &cloudbilling.ListSkusResponse{Skus: skus}, nil } // realRecommenderIterator wraps the real recommender iterator (10-L4: makes @@ -354,7 +342,7 @@ func (c *Client) getRedisPricing(ctx context.Context, tier, region string, termY return nil, err } - skus, err := billingSvc.ListSKUs("services/D559-82DA-3A56") + skus, err := c.listSKUs(billingSvc) if err != nil { return nil, fmt.Errorf("failed to list SKUs: %w", err) } @@ -575,3 +563,19 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend return rec } + +// listSKUs returns the service's SKU catalog, listing it once per client so +// that pricing N recommendations costs one catalog read instead of N. +func (c *Client) listSKUs(svc BillingService) (*cloudbilling.ListSkusResponse, error) { + c.skuMu.Lock() + defer c.skuMu.Unlock() + if c.skuCache != nil { + return c.skuCache, nil + } + skus, err := svc.ListSKUs("services/D559-82DA-3A56") + if err != nil { + return nil, err + } + c.skuCache = skus + return skus, nil +} diff --git a/providers/gcp/services/memorystore/pagination_test.go b/providers/gcp/services/memorystore/pagination_test.go index f10e0b4c..00372858 100644 --- a/providers/gcp/services/memorystore/pagination_test.go +++ b/providers/gcp/services/memorystore/pagination_test.go @@ -2,16 +2,12 @@ package memorystore import ( "context" - "fmt" - "net/http" - "net/http/httptest" "testing" "cloud.google.com/go/recommender/apiv1/recommenderpb" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/api/cloudbilling/v1" - "google.golang.org/api/option" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" ) @@ -40,38 +36,69 @@ func TestGetRecommendations_ManyItemsWithinPageCap(t *testing.T) { assert.Len(t, recs, n) } -// A genuine cap breach still fails loud. -func TestGetRecommendations_PageCapBreachFails(t *testing.T) { +// Exactly maxRecsItems recommendations are within the cap; the old per-item +// check rejected anything past 20. +func TestGetRecommendations_ExactlyAtCapSucceeds(t *testing.T) { + c, _ := NewClient(context.Background(), "p", "us-central1") + c.SetRecommenderClient(&MockRecommenderClient{recommendations: activeRecs(maxRecsItems)}) + c.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{}}) + + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + assert.Len(t, recs, maxRecsItems) +} + +// A genuine cap breach still fails loud and returns no partial list. +func TestGetRecommendations_PastCapFails(t *testing.T) { c, _ := NewClient(context.Background(), "p", "us-central1") c.SetRecommenderClient(&MockRecommenderClient{recommendations: activeRecs(maxRecsItems + 1)}) c.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{}}) - _, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.Error(t, err) - assert.Contains(t, err.Error(), "cap reached") + assert.Nil(t, recs) } -func TestRealBillingService_ListSKUs_FollowsNextPageToken(t *testing.T) { - var tokens []string - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tok := r.URL.Query().Get("pageToken") - tokens = append(tokens, tok) - w.Header().Set("Content-Type", "application/json") - switch tok { - case "": - fmt.Fprint(w, `{"skus":[{"skuId":"A"}],"nextPageToken":"t2"}`) - case "t2": - fmt.Fprint(w, `{"skus":[{"skuId":"B"}],"nextPageToken":"t3"}`) - default: - fmt.Fprint(w, `{"skus":[{"skuId":"C"}]}`) - } - })) - defer srv.Close() +// The cap is expressed in pages, so the request must ask for the page size +// the cap assumes. +type reqCapture struct { + RecommenderClient + req *recommenderpb.ListRecommendationsRequest +} - api, err := cloudbilling.NewService(context.Background(), option.WithEndpoint(srv.URL), option.WithoutAuthentication()) - require.NoError(t, err) - resp, err := (&realBillingService{service: api}).ListSKUs("services/x") +func (r *reqCapture) ListRecommendations(ctx context.Context, req *recommenderpb.ListRecommendationsRequest) RecommenderIterator { + r.req = req + return r.RecommenderClient.ListRecommendations(ctx, req) +} + +func TestGetRecommendations_RequestsExplicitPageSize(t *testing.T) { + c, _ := NewClient(context.Background(), "p", "us-central1") + capture := &reqCapture{RecommenderClient: &MockRecommenderClient{recommendations: nil}} + c.SetRecommenderClient(capture) + + _, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) - require.Len(t, resp.Skus, 3) - assert.Equal(t, []string{"", "t2", "t3"}, tokens) + require.NotNil(t, capture.req) + assert.EqualValues(t, recsPageSize, capture.req.PageSize) +} + +// Pricing N recommendations lists the SKU catalog once. +type countingBilling struct { + BillingService + calls int +} + +func (b *countingBilling) ListSKUs(id string) (*cloudbilling.ListSkusResponse, error) { + b.calls++ + return b.BillingService.ListSKUs(id) +} + +func TestListSKUs_CachedPerClient(t *testing.T) { + b := &countingBilling{BillingService: &MockBillingService{skus: &cloudbilling.ListSkusResponse{}}} + c, _ := NewClient(context.Background(), "p", "us-central1") + for i := 0; i < 3; i++ { + _, err := c.listSKUs(b) + require.NoError(t, err) + } + assert.Equal(t, 1, b.calls) } From 737bd94dabbd15a632104547cc05acfc6994abd3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 21:10:50 +0200 Subject: [PATCH 3/4] fix(gcp/computeengine): follow every page of the billing SKU listing Co-Authored-By: Claude Sonnet 5.5 --- .../gcp/services/computeengine/client.go | 15 ++++- .../gcp/services/computeengine/paging_test.go | 64 +++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 providers/gcp/services/computeengine/paging_test.go diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 7c3f6167..c857d51c 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -29,6 +29,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/retry" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skulist" "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) @@ -335,13 +336,23 @@ func (r *realMachineTypesService) Close() error { // realBillingService wraps the real cloudbilling.APIService. type realBillingService struct { + ctx context.Context service *cloudbilling.APIService } +// ListSKUs returns every SKU of the service: the Compute Engine catalog is +// 33,453 SKUs over 7 pages of 5000, so a single call misses most of it. func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusResponse, error) { - return r.service.Services.Skus.List(serviceID).Do() + skus, err := skulist.All(r.ctx, r.service.Services.Skus.List(serviceID).PageSize(skuPageSize), skulist.DefaultMaxPages) + if err != nil { + return nil, fmt.Errorf("computeengine: list SKUs for %s: %w", serviceID, err) + } + return &cloudbilling.ListSkusResponse{Skus: skus}, nil } +// skuPageSize is the largest page the catalog serves. +const skuPageSize = 5000 + // realRecommenderIterator wraps the real recommender iterator. type realRecommenderIterator struct { it *recommender.RecommendationIterator @@ -1099,7 +1110,7 @@ func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, return nil, fmt.Errorf("failed to create billing service: %w", err) } - return &realBillingService{service: service}, nil + return &realBillingService{ctx: ctx, service: service}, nil } // extractComputePricingFromSKUs extracts on-demand and commitment pricing from SKU list. diff --git a/providers/gcp/services/computeengine/paging_test.go b/providers/gcp/services/computeengine/paging_test.go new file mode 100644 index 00000000..9e1ce6ad --- /dev/null +++ b/providers/gcp/services/computeengine/paging_test.go @@ -0,0 +1,64 @@ +package computeengine + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" + "google.golang.org/api/option" +) + +// catalogServer serves total synthetic SKUs in pages of pageSize and records the +// page tokens it was asked for. +func catalogServer(t *testing.T, total, pageSize int, tokens *[]string) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + token := r.URL.Query().Get("pageToken") + *tokens = append(*tokens, token) + start := 0 + if token != "" { + start, _ = strconv.Atoi(token) + } + end := min(start+pageSize, total) + body := `{"skus":[` + for i := start; i < end; i++ { + if i > start { + body += "," + } + body += fmt.Sprintf(`{"skuId":"sku-%d"}`, i) + } + body += `]` + if end < total { + body += fmt.Sprintf(`,"nextPageToken":"%d"`, end) + } + body += `}` + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) +} + +func billingAgainst(t *testing.T, srv *httptest.Server) *realBillingService { + t.Helper() + api, err := cloudbilling.NewService(context.Background(), option.WithEndpoint(srv.URL), option.WithoutAuthentication()) + require.NoError(t, err) + return &realBillingService{ctx: context.Background(), service: api} +} + +// The live Compute Engine catalog is 33,453 SKUs over 7 pages of 5000; reading +// only the first page left the commitment SKUs unseen. +func TestRealBillingService_ListSKUsFollowsEveryPage(t *testing.T) { + var tokens []string + srv := catalogServer(t, 33453, 5000, &tokens) + defer srv.Close() + + resp, err := billingAgainst(t, srv).ListSKUs("services/6F81-5844-456A") + require.NoError(t, err) + assert.Len(t, resp.Skus, 33453) + assert.Len(t, tokens, 7) +} From d88cb8a2c3e6f9c6970f85b1e3482084adda7e51 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 23:20:24 +0200 Subject: [PATCH 4/4] test(gcp/computeengine): build the paged catalog response with strings.Builder Co-Authored-By: Claude Sonnet 5.5 --- .../gcp/services/computeengine/paging_test.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/providers/gcp/services/computeengine/paging_test.go b/providers/gcp/services/computeengine/paging_test.go index 9e1ce6ad..94672849 100644 --- a/providers/gcp/services/computeengine/paging_test.go +++ b/providers/gcp/services/computeengine/paging_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "strconv" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -26,20 +27,21 @@ func catalogServer(t *testing.T, total, pageSize int, tokens *[]string) *httptes start, _ = strconv.Atoi(token) } end := min(start+pageSize, total) - body := `{"skus":[` + var body strings.Builder + body.WriteString(`{"skus":[`) for i := start; i < end; i++ { if i > start { - body += "," + body.WriteString(",") } - body += fmt.Sprintf(`{"skuId":"sku-%d"}`, i) + fmt.Fprintf(&body, `{"skuId":"sku-%d"}`, i) } - body += `]` + body.WriteString("]") if end < total { - body += fmt.Sprintf(`,"nextPageToken":"%d"`, end) + fmt.Fprintf(&body, `,"nextPageToken":"%d"`, end) } - body += `}` + body.WriteString("}") w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(body)) + _, _ = w.Write([]byte(body.String())) })) }