From 4c298ec7bfcbec4d41fa37922e6d0591a7fbfb7f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 30 Sep 2026 02:22:29 +0200 Subject: [PATCH 1/2] fix(gcp): select explicitly typed vcpu recommendation amounts Reject unknown, missing and ambiguous resource amounts instead of using accelerator quantities or an untyped overview number as vCPUs. Accept decimal integer strings and safe numeric quantities while preserving existing memory aliases and cost-only recommendations. Verify resource ordering and error propagation through the public provider and real SDK against local fixture servers. Closes #80 --- providers/gcp/recommendations_sdk_test.go | 77 +++++++-- .../gcp/services/computeengine/client.go | 149 +++++++++++------- .../gcp/services/computeengine/client_test.go | 141 ++++++++++++++--- 3 files changed, 272 insertions(+), 95 deletions(-) diff --git a/providers/gcp/recommendations_sdk_test.go b/providers/gcp/recommendations_sdk_test.go index b257a9b..55acfbc 100644 --- a/providers/gcp/recommendations_sdk_test.go +++ b/providers/gcp/recommendations_sdk_test.go @@ -22,6 +22,7 @@ import ( "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" "google.golang.org/grpc/test/bufconn" + "google.golang.org/protobuf/types/known/structpb" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" ) @@ -39,14 +40,31 @@ func TestComputeRecommendationsThroughSDK(t *testing.T) { const central = "projects/recommendation-project/locations/us-central1/recommenders/google.compute.commitment.UsageCommitmentRecommender" const west = "projects/recommendation-project/locations/europe-west1/recommenders/google.compute.commitment.UsageCommitmentRecommender" cases := []struct { - name string - empty bool - failure map[string]bool + name string + empty bool + failure map[string]bool + resources []string + stringVCPU bool + invalid bool }{ {name: "regional recommendations and pagination"}, {name: "empty success", empty: true}, {name: "all regions denied", failure: map[string]bool{central: true, west: true}}, {name: "partial regional success", failure: map[string]bool{west: true}}, + {name: "accelerator memory cpu", resources: []string{"ACCELERATOR", "MEMORY", "VCPU"}}, + {name: "accelerator cpu memory", resources: []string{"ACCELERATOR", "VCPU", "MEMORY"}}, + {name: "memory accelerator cpu", resources: []string{"MEMORY", "ACCELERATOR", "VCPU"}}, + {name: "memory cpu accelerator", resources: []string{"MEMORY", "VCPU", "ACCELERATOR"}}, + {name: "cpu accelerator memory", resources: []string{"VCPU", "ACCELERATOR", "MEMORY"}}, + {name: "cpu memory accelerator", resources: []string{"VCPU", "MEMORY", "ACCELERATOR"}}, + {name: "local SSD before cpu", resources: []string{"LOCAL_SSD", "VCPU", "MEMORY"}}, + {name: "local SSD after cpu", resources: []string{"VCPU", "MEMORY", "LOCAL_SSD"}}, + {name: "string cpu", resources: []string{"ACCELERATOR", "VCPU", "MEMORY"}, stringVCPU: true}, + {name: "lowercase memory", resources: []string{"memory", "VCPU"}}, + {name: "legacy lowercase memory", resources: []string{"memory_mb", "VCPU"}}, + {name: "unknown resource", resources: []string{"UNKNOWN", "VCPU", "MEMORY"}, invalid: true}, + {name: "unknown after cpu", resources: []string{"VCPU", "MEMORY", "UNKNOWN"}, invalid: true}, + {name: "missing cpu with overview", resources: []string{"ACCELERATOR", "MEMORY"}, invalid: true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -72,6 +90,13 @@ func TestComputeRecommendationsThroughSDK(t *testing.T) { listener := bufconn.Listen(1024 * 1024) defer listener.Close() server := grpc.NewServer() + recommendation := func(savings int64, state recommenderpb.RecommendationStateInfo_State, region string) *recommenderpb.Recommendation { + rec := sdkCostRecommendation(savings, state) + if tc.resources != nil { + rec.Content = sdkResourceAmounts(tc.resources, tc.stringVCPU, region) + } + return rec + } recommenderpb.RegisterRecommenderServer(server, &recommendationSDKServer{ list: func(ctx context.Context, req *recommenderpb.ListRecommendationsRequest) (*recommenderpb.ListRecommendationsResponse, error) { mu.Lock() @@ -89,19 +114,19 @@ func TestComputeRecommendationsThroughSDK(t *testing.T) { return &recommenderpb.ListRecommendationsResponse{}, nil } if req.GetParent() == west { - return &recommenderpb.ListRecommendationsResponse{Recommendations: []*recommenderpb.Recommendation{sdkCostRecommendation(30, recommenderpb.RecommendationStateInfo_ACTIVE)}}, nil + return &recommenderpb.ListRecommendationsResponse{Recommendations: []*recommenderpb.Recommendation{recommendation(30, recommenderpb.RecommendationStateInfo_ACTIVE, "europe-west1")}}, nil } switch req.GetPageToken() { case "": return &recommenderpb.ListRecommendationsResponse{ Recommendations: []*recommenderpb.Recommendation{ - sdkCostRecommendation(10, recommenderpb.RecommendationStateInfo_ACTIVE), - sdkCostRecommendation(999, recommenderpb.RecommendationStateInfo_DISMISSED), + recommendation(10, recommenderpb.RecommendationStateInfo_ACTIVE, "us-central1"), + recommendation(999, recommenderpb.RecommendationStateInfo_DISMISSED, "us-central1"), }, NextPageToken: "second-page", }, nil case "second-page": - return &recommenderpb.ListRecommendationsResponse{Recommendations: []*recommenderpb.Recommendation{sdkCostRecommendation(20, recommenderpb.RecommendationStateInfo_ACTIVE)}}, nil + return &recommenderpb.ListRecommendationsResponse{Recommendations: []*recommenderpb.Recommendation{recommendation(20, recommenderpb.RecommendationStateInfo_ACTIVE, "us-central1")}}, nil default: return nil, status.Error(codes.InvalidArgument, "unexpected page token") } @@ -124,9 +149,13 @@ func TestComputeRecommendationsThroughSDK(t *testing.T) { adapter, err := NewProviderWithProject(ctx, "recommendation-project", options...).GetRecommendationsClient(ctx) require.NoError(t, err) recs, err := adapter.GetRecommendationsForService(ctx, common.ServiceCompute) - if len(tc.failure) == 2 { + if len(tc.failure) == 2 || tc.invalid { require.Error(t, err) - assert.Equal(t, codes.PermissionDenied, status.Code(err)) + if !tc.invalid { + assert.Equal(t, codes.PermissionDenied, status.Code(err)) + } else { + assert.Contains(t, err.Error(), "VCPU") + } assert.Contains(t, err.Error(), "all 2 GCP recommendation service calls failed across 2 regions") assert.Nil(t, recs) } else { @@ -136,6 +165,13 @@ func TestComputeRecommendationsThroughSDK(t *testing.T) { assert.Equal(t, common.ProviderGCP, rec.Provider) assert.Equal(t, common.ServiceCompute, rec.Service) assert.Equal(t, "recommendation-project", rec.Account) + assert.Empty(t, rec.ResourceType) + if tc.resources != nil { + assert.Equal(t, 4, rec.Count) + assert.Equal(t, common.ComputeDetails{MemoryGB: 6}, rec.Details) + } else { + assert.Zero(t, rec.Count) + } if rec.EstimatedSavings == 30 { assert.Equal(t, "europe-west1", rec.Region) } else { @@ -156,7 +192,7 @@ func TestComputeRecommendationsThroughSDK(t *testing.T) { defer mu.Unlock() assert.Equal(t, 1, regionsCalls) expectedRequests := []string{central + "|", west + "|"} - if !tc.empty && !tc.failure[central] { + if !tc.empty && !tc.failure[central] && !tc.invalid { expectedRequests = append(expectedRequests, central+"|second-page") } assert.ElementsMatch(t, expectedRequests, requests) @@ -164,6 +200,27 @@ func TestComputeRecommendationsThroughSDK(t *testing.T) { } } +// Synthetic explicit-filter operations exercise the generic API contract, not a captured cloud response. +func sdkResourceAmounts(kinds []string, stringVCPU bool, region string) *recommenderpb.RecommendationContent { + group := &recommenderpb.OperationGroup{} + for _, kind := range kinds { + amount := structpb.NewNumberValue(map[string]float64{"VCPU": 4, "MEMORY": 6144, "memory": 6144, "memory_mb": 6144, "ACCELERATOR": 2, "LOCAL_SSD": 375, "UNKNOWN": 99}[kind]) + if kind == "VCPU" && stringVCPU { + amount = structpb.NewStringValue("4") + } + group.Operations = append(group.Operations, &recommenderpb.Operation{ + Action: "AdD", ResourceType: "compute.googleapis.com/Commitment", + Resource: "//compute.googleapis.com/projects/recommendation-project/regions/" + region + "/commitments/cud-001", + Path: "/resources/*/amount", PathFilters: map[string]*structpb.Value{"/resources/*/type": structpb.NewStringValue(kind)}, + PathValue: &recommenderpb.Operation_Value{Value: amount}, + }) + } + return &recommenderpb.RecommendationContent{ + OperationGroups: []*recommenderpb.OperationGroup{group}, + Overview: &structpb.Struct{Fields: map[string]*structpb.Value{"numericValue": structpb.NewNumberValue(999)}}, + } +} + // A cost-only fixture verifies listing without inventing purchasable machine resources. func sdkCostRecommendation(savings int64, state recommenderpb.RecommendationStateInfo_State) *recommenderpb.Recommendation { return &recommenderpb.Recommendation{ diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index d7be69b..89740bd 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -6,6 +6,8 @@ import ( "errors" "fmt" "log" + "math" + "strconv" "strings" "time" @@ -399,16 +401,13 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio } it := recClient.ListRecommendations(ctx, req) - for pageIdx := 0; ; pageIdx++ { + for pageIdx := 0; pageIdx < maxRecsPages; pageIdx++ { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("context canceled during pagination: %w", err) } - if pageIdx >= maxRecsPages { - return nil, fmt.Errorf("computeengine: GetRecommendations iteration cap (%d items) reached", maxRecsPages) - } rec, err := it.Next() if errors.Is(err, iterator.Done) { - break + return recommendations, nil } if err != nil { // Iterator errors (quota, auth, transient 5xx) must propagate so @@ -426,13 +425,16 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio continue } - converted := c.convertGCPRecommendation(ctx, rec, params) + converted, err := c.convertGCPRecommendation(ctx, rec, params) + if err != nil { + return nil, fmt.Errorf("computeengine: recommendation %q: %w", rec.GetName(), err) + } if converted != nil { recommendations = append(recommendations, *converted) } } - return recommendations, nil + return nil, fmt.Errorf("computeengine: GetRecommendations iteration cap (%d items) reached", maxRecsPages) } // GetExistingCommitments retrieves existing Compute Engine CUDs. @@ -1095,7 +1097,7 @@ func skuMatchesMachineType(sku *cloudbilling.Sku, machineType, region string) bo // EstimatedSavings from the Recommender payload is the authoritative savings signal. // Returns nil when the params.Term is unrecognized so the caller skips an // unroutable recommendation rather than queuing a purchase with an invalid plan. -func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { +func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) (*common.Recommendation, error) { // GCP CUDs are billed monthly with no upfront option; force "monthly" // unconditionally and log any non-monthly input so scheduler // misconfiguration is visible. Supersedes the monthly stamp introduced @@ -1114,7 +1116,7 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend } if _, err := termPlan(term); err != nil { log.Printf("computeengine: skipping recommendation with unrecognized term %q: %v", term, err) - return nil + return nil, nil } rec := &common.Recommendation{ @@ -1140,7 +1142,11 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend } extractCostImpactFromRecommendation(gcpRec, rec) - extractVCPUCountFromRecommendation(gcpRec, rec) + count, err := vcpuCountFromOperationGroups(gcpRec.GetContent()) + if err != nil { + return nil, err + } + rec.Count = count extractMemoryMBFromRecommendation(gcpRec, rec) c.enrichRecWithPricing(ctx, rec) @@ -1158,7 +1164,7 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend rec.RecurringMonthlyCost = &monthly } - return rec + return rec, nil } // enrichRecWithPricing fills CommitmentCost, OnDemandCost, SavingsPercentage, @@ -1277,12 +1283,7 @@ func extractCostImpactFromRecommendation(gcpRec *recommenderpb.Recommendation, r } } -// isMemoryAmountOp returns true when op's path_filters indicate a memory -// resource type. Used by extractVCPUCountFromRecommendation to skip the memory -// sibling of the VCPU operation in a GCP commitment resource operation group. -// Matches both "MEMORY" (the canonical ResourceCommitment.Type enum member) and -// the legacy "MEMORY_MB" spelling, since the Recommender's path_filter encoding -// is not contractually documented (issue #1022). +// Retain the legacy MEMORY_MB spelling accepted by existing memory fixtures. func isMemoryAmountOp(op *recommenderpb.Operation) bool { for filterKey, filterVal := range op.GetPathFilters() { if !strings.Contains(strings.ToLower(filterKey), "type") { @@ -1297,63 +1298,93 @@ func isMemoryAmountOp(op *recommenderpb.Operation) bool { return false } -// vcpuCountFromOperationGroups walks the commitment operation groups and -// returns the VCPU count encoded in the operation's numeric value, or 0 if -// none is found. Extracted from extractVCPUCountFromRecommendation to keep -// cyclomatic complexity in check. -func vcpuCountFromOperationGroups(content *recommenderpb.RecommendationContent) int { - for _, opGroup := range content.GetOperationGroups() { - for _, op := range opGroup.GetOperations() { - if !strings.Contains(strings.ToLower(op.GetResourceType()), "commitment") { - continue +func vcpuCountFromOperationGroups(content *recommenderpb.RecommendationContent) (int, error) { + count, amounts := 0, false + for _, group := range content.GetOperationGroups() { + for _, op := range group.GetOperations() { + kind, err := recommendationAmountType(op) + if err != nil { + return 0, fmt.Errorf("VCPU extraction: %w", err) } - if !strings.Contains(strings.ToLower(op.GetPath()), "amount") { + if kind == "" { continue } - if isMemoryAmountOp(op) { + amounts = true + if kind != computepb.ResourceCommitment_VCPU.String() { continue } - if v := op.GetValue(); v != nil { - if nv, ok := v.GetKind().(*structpb.Value_NumberValue); ok && nv.NumberValue > 0 { - return int(nv.NumberValue) - } + if count != 0 { + return 0, fmt.Errorf("multiple VCPU amounts in recommendation") + } + count, err = recommendationVCPUAmount(op.GetValue()) + if err != nil { + return 0, err } } } - return 0 + if amounts && count == 0 { + return 0, fmt.Errorf("recommendation resource amounts contain no VCPU") + } + return count, nil } -// extractVCPUCountFromRecommendation extracts the recommended vCPU count from a -// GCP Commitment Recommender response (issue #1022 C1). -// -// The Recommender encodes the commitment resource amounts in two places: -// - Operation.Value: a structpb.Value whose numeric value is the amount, with -// Operation.Path indicating the resource type (e.g. "/resources/0/amount"). -// Operations with ResourceType "compute.googleapis.com/Commitment" and a path -// containing "amount" carry the VCPU count; the sibling MEMORY amount is -// extracted by extractMemoryMBFromRecommendation. -// - RecommendationContent.Overview: a JSON struct with a "numericValue" field. -// -// We prefer the operation-value path because it is structured and unambiguous. -// If no VCPU operation is found we fall back to the overview's numericValue. -func extractVCPUCountFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) { - if gcpRec.Content == nil { - return +func recommendationAmountType(op *recommenderpb.Operation) (string, error) { + if op.GetResourceType() != "compute.googleapis.com/Commitment" { + return "", nil } - - if count := vcpuCountFromOperationGroups(gcpRec.Content); count > 0 { - rec.Count = count - return + switch { + case strings.EqualFold(op.GetAction(), "add"), strings.EqualFold(op.GetAction(), "replace"): + default: + return "", nil + } + selector, ok := strings.CutPrefix(op.GetPath(), "/resources/") + if !ok { + return "", nil + } + selector, ok = strings.CutSuffix(selector, "/amount") + if !ok { + return "", nil } + if selector != "*" { + index, err := strconv.ParseUint(selector, 10, 64) + if err != nil || strconv.FormatUint(index, 10) != selector { + return "", fmt.Errorf("invalid resource selector %q", selector) + } + } + kind := op.GetPathFilters()["/resources/"+selector+"/type"].GetStringValue() + switch { + case strings.EqualFold(kind, computepb.ResourceCommitment_MEMORY.String()), strings.EqualFold(kind, "MEMORY_MB"): + return computepb.ResourceCommitment_MEMORY.String(), nil + case kind == computepb.ResourceCommitment_VCPU.String(), kind == computepb.ResourceCommitment_ACCELERATOR.String(), kind == computepb.ResourceCommitment_LOCAL_SSD.String(): + return kind, nil + default: + return "", fmt.Errorf("unknown or absent commitment resource type %q for %s", kind, op.GetPath()) + } +} - // Fallback: overview numericValue (used by older recommender versions). - if gcpRec.Content.GetOverview() != nil { - if nv := gcpRec.Content.GetOverview().GetFields()["numericValue"]; nv != nil { - if count := nv.GetNumberValue(); count > 0 { - rec.Count = int(count) - } +func recommendationVCPUAmount(value *structpb.Value) (int, error) { + var amount int64 + switch v := value.GetKind().(type) { + case *structpb.Value_StringValue: + parsed, err := strconv.ParseInt(v.StringValue, 10, 64) + if err != nil { + return 0, fmt.Errorf("invalid VCPU amount %q: %w", v.StringValue, err) + } + amount = parsed + case *structpb.Value_NumberValue: + // Larger JSON numbers may have lost integer precision before reaching this parser. + const maxExactInteger = 1<<53 - 1 + if math.IsNaN(v.NumberValue) || v.NumberValue <= 0 || v.NumberValue > maxExactInteger || math.Trunc(v.NumberValue) != v.NumberValue { + return 0, fmt.Errorf("VCPU amount must be a positive exact integer") } + amount = int64(v.NumberValue) + default: + return 0, fmt.Errorf("VCPU amount must be a decimal string or number") + } + if amount <= 0 || amount > math.MaxInt { + return 0, fmt.Errorf("VCPU amount must be positive and fit int") } + return int(amount), nil } // memoryMBFromOperationGroups walks the commitment operation groups and returns diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 39ad85e..9bf2242 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "errors" + "math" + "strconv" "testing" "cloud.google.com/go/compute/apiv1/computepb" @@ -977,7 +979,8 @@ func TestComputeEngineClient_ConvertGCPRecommendation(t *testing.T) { }, } - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + rec, err := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + require.NoError(t, err) require.NotNil(t, rec) assert.Equal(t, common.ProviderGCP, rec.Provider) assert.Equal(t, common.ServiceCompute, rec.Service) @@ -1037,19 +1040,7 @@ func TestComputeEngineClient_GetRecommendations_PageCapFires(t *testing.T) { require.Error(t, err, "page cap must surface an error when the iterator never terminates") } -// realisticCUDRecommendation builds a realistic GCP Commitment Recommender payload -// for a 4-vCPU n1-standard-4 commitment in us-central1. The operation groups have -// three ops: -// 1. A machine-type op whose resource path ends in the machine type (n1-standard-4) -- -// used by extractResourceTypeFromRecommendation to set rec.ResourceType. -// 2. A VCPU commitment resource op whose numeric Value is the VCPU count -- -// used by extractVCPUCountFromRecommendation to set rec.Count = 4. -// 3. A MEMORY commitment resource op whose numeric Value is 6144 MB -- -// used by extractMemoryMBFromRecommendation to set rec.Details.MemoryGB = 6. -// Using 6144 MB (1536 MB/vCPU) intentionally tests a non-4096-ratio case to -// confirm the value is read from the payload rather than computed from a ratio. -// -// This mirrors the GCP CUD Recommender format (issue #1022 C1 + memory fix). +// Synthetic fixture for conversion and insert tests; not a captured service response. func realisticCUDRecommendation() *recommenderpb.Recommendation { vcpuVal, _ := structpb.NewValue(4.0) memVal, _ := structpb.NewValue(6144.0) // 6144 MB = 6 GB; ratio is 1536 MB/vCPU, NOT 4096 @@ -1083,13 +1074,12 @@ func realisticCUDRecommendation() *recommenderpb.Recommendation { { Operations: []*recommenderpb.Operation{ { - // VCPU op: carries the vCPU count as a numeric value. - // extractVCPUCountFromRecommendation reads this and sets rec.Count = 4. Action: "add", ResourceType: "compute.googleapis.com/Commitment", Resource: "//compute.googleapis.com/projects/test/regions/us-central1/commitments/cud-001", Path: "/resources/0/amount", PathValue: &recommenderpb.Operation_Value{Value: vcpuVal}, + PathFilters: map[string]*structpb.Value{"/resources/0/type": structpb.NewStringValue("VCPU")}, }, { // MEMORY op: carries the memory amount in MB as a numeric value. @@ -1176,7 +1166,8 @@ func TestConverterToInsert_CountNonZero_VCPUAmountSet(t *testing.T) { client.SetBillingService(mockBillingWithCommitment()) gcpRec := realisticCUDRecommendation() // 4 vCPU, 6144 MB (from payload) - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + rec, err := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + require.NoError(t, err) require.NotNil(t, rec) // C1: Count must be > 0 so that buildInsertRequest produces non-zero VCPU Amount. @@ -1230,7 +1221,8 @@ func TestConverterFillsPricingForScorer(t *testing.T) { client.SetBillingService(mockBillingWithCommitment()) gcpRec := realisticCUDRecommendation() - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + rec, err := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + require.NoError(t, err) require.NotNil(t, rec) // C2: SavingsPercentage must be > 0 so a MinSavingsPct filter doesn't silently drop the rec. @@ -1338,7 +1330,8 @@ func TestConvertGCPRecommendation_EmptyParamsDefaultsToMonthly(t *testing.T) { } // Empty params: no caller-supplied PaymentOption. - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + rec, err := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + require.NoError(t, err) require.NotNil(t, rec) assert.Equal(t, "monthly", rec.PaymentOption, "GCP CUDs have no upfront option; empty PaymentOption must default to \"monthly\" (10-M5)") @@ -1353,7 +1346,8 @@ func TestConvertGCPRecommendation_ParamPaymentOptionRespected(t *testing.T) { gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} params := common.RecommendationParams{PaymentOption: "monthly"} - rec := client.convertGCPRecommendation(ctx, gcpRec, params) + rec, err := client.convertGCPRecommendation(ctx, gcpRec, params) + require.NoError(t, err) require.NotNil(t, rec) assert.Equal(t, "monthly", rec.PaymentOption) } @@ -1473,13 +1467,102 @@ func TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly(t *test for _, input := range []string{"upfront", "all-upfront", "UPFRONT", "partial-upfront"} { params := common.RecommendationParams{PaymentOption: input} - rec := client.convertGCPRecommendation(ctx, gcpRec, params) + rec, err := client.convertGCPRecommendation(ctx, gcpRec, params) + require.NoError(t, err) require.NotNil(t, rec) assert.Equal(t, "monthly", rec.PaymentOption, "GCP CUDs are monthly-only; input %q must be forced to \"monthly\"", input) } } +func TestVCPURecommendationRequiresExplicitResourceIdentity(t *testing.T) { + cases := []struct { + name string + change func(*recommenderpb.Operation) + want int + wantErr bool + }{ + {name: "indexed cpu", want: 4}, + {name: "wildcard cpu", want: 4, change: func(op *recommenderpb.Operation) { + op.Path = "/resources/*/amount" + op.PathFilters = map[string]*structpb.Value{"/resources/*/type": structpb.NewStringValue("VCPU")} + }}, + {name: "mixed case replace", want: 4, change: func(op *recommenderpb.Operation) { op.Action = "RePlAcE" }}, + {name: "string cpu", want: 4, change: func(op *recommenderpb.Operation) { + op.PathValue = &recommenderpb.Operation_Value{Value: structpb.NewStringValue("4")} + }}, + {name: "missing filter", wantErr: true, change: func(op *recommenderpb.Operation) { op.PathFilters = nil }}, + {name: "wrong selector", wantErr: true, change: func(op *recommenderpb.Operation) { op.Path = "/resources/1/amount" }}, + {name: "invalid selector", wantErr: true, change: func(op *recommenderpb.Operation) { op.Path = "/resources/cpu/amount" }}, + {name: "unknown type", wantErr: true, change: func(op *recommenderpb.Operation) { + op.PathFilters["/resources/0/type"] = structpb.NewStringValue("UNKNOWN") + }}, + {name: "numeric type", wantErr: true, change: func(op *recommenderpb.Operation) { op.PathFilters["/resources/0/type"] = structpb.NewNumberValue(4) }}, + {name: "accelerator only", wantErr: true, change: func(op *recommenderpb.Operation) { + op.PathFilters["/resources/0/type"] = structpb.NewStringValue("ACCELERATOR") + }}, + {name: "SSD only", wantErr: true, change: func(op *recommenderpb.Operation) { + op.PathFilters["/resources/0/type"] = structpb.NewStringValue("LOCAL_SSD") + }}, + {name: "memory only", wantErr: true, change: func(op *recommenderpb.Operation) { + op.PathFilters["/resources/0/type"] = structpb.NewStringValue("MEMORY") + }}, + {name: "resource substring", change: func(op *recommenderpb.Operation) { op.ResourceType = "unrelated/Commitment" }}, + {name: "path substring", change: func(op *recommenderpb.Operation) { op.Path = "/resources/0/amountOther" }}, + {name: "test precondition", change: func(op *recommenderpb.Operation) { op.Action = "test" }}, + {name: "remove", change: func(op *recommenderpb.Operation) { op.Action = "remove" }}, + {name: "copy", change: func(op *recommenderpb.Operation) { op.Action = "copy" }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + op := &recommenderpb.Operation{ + Action: "ADD", ResourceType: "compute.googleapis.com/Commitment", + Path: "/resources/0/amount", PathFilters: map[string]*structpb.Value{"/resources/0/type": structpb.NewStringValue("VCPU")}, + PathValue: &recommenderpb.Operation_Value{Value: structpb.NewNumberValue(4)}, + } + if tc.change != nil { + tc.change(op) + } + input := &recommenderpb.Recommendation{Content: &recommenderpb.RecommendationContent{ + OperationGroups: []*recommenderpb.OperationGroup{{Operations: []*recommenderpb.Operation{op}}}, + Overview: &structpb.Struct{Fields: map[string]*structpb.Value{"numericValue": structpb.NewNumberValue(999)}}, + }} + rec, err := (&Client{}).convertGCPRecommendation(context.Background(), input, common.RecommendationParams{}) + if tc.wantErr { + require.ErrorContains(t, err, "VCPU") + assert.Nil(t, rec) + return + } + require.NoError(t, err) + require.NotNil(t, rec) + assert.Equal(t, tc.want, rec.Count) + }) + } + input := commitmentOnlyCUDRecommendation() + group := input.Content.OperationGroups[0] + group.Operations = append(group.Operations, group.Operations[0]) + rec, err := (&Client{}).convertGCPRecommendation(context.Background(), input, common.RecommendationParams{}) + require.ErrorContains(t, err, "multiple VCPU") + assert.Nil(t, rec) +} + +func TestRecommendationVCPUAmountRejectsInvalidQuantities(t *testing.T) { + for _, value := range []*structpb.Value{ + nil, structpb.NewNullValue(), structpb.NewBoolValue(true), structpb.NewStructValue(&structpb.Struct{}), + structpb.NewListValue(&structpb.ListValue{}), structpb.NewNumberValue(0), structpb.NewNumberValue(-1), + structpb.NewNumberValue(1.5), structpb.NewNumberValue(math.NaN()), structpb.NewNumberValue(math.Inf(1)), + structpb.NewNumberValue(math.Inf(-1)), structpb.NewNumberValue(1 << 53), structpb.NewNumberValue(float64(math.MaxInt)), + structpb.NewStringValue(""), structpb.NewStringValue("0"), structpb.NewStringValue("-1"), + structpb.NewStringValue("1.5"), structpb.NewStringValue("1e3"), structpb.NewStringValue("9223372036854775808"), + } { + _, err := recommendationVCPUAmount(value) + require.Error(t, err, "value %v", value) + } + count, err := recommendationVCPUAmount(structpb.NewStringValue(strconv.FormatInt(int64(math.MaxInt), 10))) + require.NoError(t, err) + assert.Equal(t, math.MaxInt, count) +} + // TestIsMemoryAmountOp_MatchesBothSpellings asserts that the inbound Recommender // memory-op detector skips both the canonical "MEMORY" and legacy "MEMORY_MB" // path_filter spellings, so the VCPU extractor never mistakes the memory sibling @@ -1596,18 +1679,21 @@ func TestConvertGCPRecommendation_PropagatesParamsTerm(t *testing.T) { gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} // 3yr must be propagated. - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "3yr"}) + rec, err := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "3yr"}) + require.NoError(t, err) require.NotNil(t, rec) assert.Equal(t, "3yr", rec.Term, "params.Term=3yr must be propagated to rec.Term (H-3 fix); pre-fix code hardcoded 1yr") // 1yr explicit must be propagated. - rec = client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "1yr"}) + rec, err = client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "1yr"}) + require.NoError(t, err) require.NotNil(t, rec) assert.Equal(t, "1yr", rec.Term) // Empty term must default to "1yr". - rec = client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + rec, err = client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + require.NoError(t, err) require.NotNil(t, rec) assert.Equal(t, "1yr", rec.Term, "empty params.Term must default to 1yr") @@ -1625,7 +1711,8 @@ func TestConvertGCPRecommendation_RejectsUnknownTerm(t *testing.T) { gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} - rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "5yr"}) + rec, err := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "5yr"}) + require.NoError(t, err) assert.Nil(t, rec, "convertGCPRecommendation must return nil for unrecognized term (not silently default to 12 months)") } @@ -1980,6 +2067,7 @@ func commitmentOnlyCUDRecommendation() *recommenderpb.Recommendation { Resource: commitmentResource, Path: "/resources/0/amount", PathValue: &recommenderpb.Operation_Value{Value: vcpuVal}, + PathFilters: map[string]*structpb.Value{"/resources/0/type": structpb.NewStringValue("VCPU")}, }, { Action: "add", @@ -2029,7 +2117,8 @@ func TestPurchaseCommitment_CommitmentOnlyRecommendationRefuses(t *testing.T) { client, err := NewClient(ctx, "test-project", "us-central1") require.NoError(t, err) - converted := client.convertGCPRecommendation(ctx, commitmentOnlyCUDRecommendation(), common.RecommendationParams{Term: "1yr"}) + converted, err := client.convertGCPRecommendation(ctx, commitmentOnlyCUDRecommendation(), common.RecommendationParams{Term: "1yr"}) + require.NoError(t, err) require.NotNil(t, converted, "the recommendation stays visible; only the purchase is refused") require.Empty(t, converted.ResourceType, "a commitment-only payload must not yield a machine type (issue #1538)") From 40636e7c5f4ad0500d782772c1f00b0f12b653fb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 30 Sep 2026 02:56:32 +0200 Subject: [PATCH 2/2] fix(gcp): enforce recommendation budget at server page boundaries Count SDK page fetches instead of recommendations so buffered items and dismissed rows do not exhaust the page budget. Include empty pages and reject continuation beyond page 20 without returning partial results. Exercise exact item and page boundaries, empty and repeated tokens, cancellation, and API failures through the actual SDK and public provider. --- .../recommendations_pagination_sdk_test.go | 131 ++++++++++++++++++ .../gcp/services/computeengine/client.go | 21 ++- .../gcp/services/computeengine/client_test.go | 29 +--- 3 files changed, 149 insertions(+), 32 deletions(-) create mode 100644 providers/gcp/recommendations_pagination_sdk_test.go diff --git a/providers/gcp/recommendations_pagination_sdk_test.go b/providers/gcp/recommendations_pagination_sdk_test.go new file mode 100644 index 0000000..70c9967 --- /dev/null +++ b/providers/gcp/recommendations_pagination_sdk_test.go @@ -0,0 +1,131 @@ +package gcp + +import ( + "context" + "errors" + "fmt" + "net" + "net/http" + "net/http/httptest" + "strconv" + "sync/atomic" + "testing" + "time" + + "cloud.google.com/go/recommender/apiv1/recommenderpb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/option" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/status" + "google.golang.org/grpc/test/bufconn" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func TestComputeRecommendationsSDKPageBudget(t *testing.T) { + for _, tc := range []struct { + name string + pages, items, dismissed, wantCalls int + repeat, cancel, denied, cap bool + }{ + {name: "exactly20items", pages: 1, items: 20, wantCalls: 1}, + {name: "moreThan20items", pages: 1, items: 21, wantCalls: 1}, + {name: "dismissedDoNotConsumeBudget", pages: 1, items: 2, dismissed: 25, wantCalls: 1}, + {name: "exactly20pages", pages: 20, items: 2, wantCalls: 20}, + {name: "over20pages", pages: 21, items: 2, wantCalls: 20, cap: true}, + {name: "empty20pages", pages: 20, wantCalls: 20}, + {name: "emptyOver20pages", pages: 21, wantCalls: 20, cap: true}, + {name: "repeatedEmptyToken", pages: 21, repeat: true, wantCalls: 20, cap: true}, + {name: "cancelSecondPage", pages: 2, items: 2, cancel: true, wantCalls: 2}, + {name: "deniedSecondPage", pages: 2, items: 2, denied: true, wantCalls: 2}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + regions := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "/compute/v1/projects/recommendation-project/regions", r.URL.Path) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"items":[{"name":"us-central1","status":"UP"}]}`) + })) + defer regions.Close() + listener := bufconn.Listen(1024 * 1024) + defer listener.Close() + server := grpc.NewServer() + var calls atomic.Int32 + recommenderpb.RegisterRecommenderServer(server, &recommendationSDKServer{list: func(_ context.Context, req *recommenderpb.ListRecommendationsRequest) (*recommenderpb.ListRecommendationsResponse, error) { + page := int(calls.Add(1)) + assert.Equal(t, "projects/recommendation-project/locations/us-central1/recommenders/google.compute.commitment.UsageCommitmentRecommender", req.GetParent()) + expectedToken := "" + if page > 1 { + expectedToken = strconv.Itoa(page) + } + if page > 1 && tc.repeat { + expectedToken = "repeat" + } + assert.Equal(t, expectedToken, req.GetPageToken()) + if page == 2 && tc.cancel { + cancel() + return nil, status.Error(codes.Canceled, "canceled during second page") + } + if page == 2 && tc.denied { + return nil, status.Error(codes.PermissionDenied, "second page denied") + } + response := &recommenderpb.ListRecommendationsResponse{} + for i := 0; i < tc.items+tc.dismissed; i++ { + state := recommenderpb.RecommendationStateInfo_ACTIVE + if i < tc.dismissed { + state = recommenderpb.RecommendationStateInfo_DISMISSED + } + rec := sdkCostRecommendation(10, state) + rec.Name = fmt.Sprintf("page-%d-item-%d", page, i) + rec.Content = sdkResourceAmounts([]string{"VCPU", "MEMORY"}, true, "us-central1") + response.Recommendations = append(response.Recommendations, rec) + } + if page < tc.pages { + response.NextPageToken = strconv.Itoa(page + 1) + } + if tc.repeat { + response.NextPageToken = "repeat" + } + // Bound a broken implementation without hiding its extra fetches. + if page > 21 { + return nil, status.Error(codes.InvalidArgument, "test page safety bound") + } + return response, nil + }}) + serveDone := make(chan error, 1) + go func() { serveDone <- server.Serve(listener) }() + defer func() { server.Stop(); assert.NoError(t, <-serveDone) }() + provider := NewProviderWithProject(ctx, "recommendation-project", option.WithoutAuthentication(), option.WithEndpoint(regions.URL), + option.WithGRPCDialOption(grpc.WithTransportCredentials(insecure.NewCredentials())), + option.WithGRPCDialOption(grpc.WithContextDialer(func(ctx context.Context, _ string) (net.Conn, error) { return listener.DialContext(ctx) }))) + client, err := provider.GetRecommendationsClient(ctx) + require.NoError(t, err) + recs, err := client.GetRecommendationsForService(ctx, common.ServiceCompute) + assert.Equal(t, tc.wantCalls, int(calls.Load())) + switch { + case tc.cap: + require.ErrorContains(t, err, "page cap (20 pages)") + assert.Nil(t, recs) + case tc.cancel: + require.Error(t, err) + assert.True(t, errors.Is(err, context.Canceled) || status.Code(err) == codes.Canceled, "cancellation cause: %v", err) + assert.Nil(t, recs) + case tc.denied: + require.Error(t, err) + assert.Equal(t, codes.PermissionDenied, status.Code(err)) + assert.Nil(t, recs) + default: + require.NoError(t, err) + require.Len(t, recs, tc.pages*tc.items) + for _, rec := range recs { + assert.Equal(t, 4, rec.Count) + assert.Equal(t, common.ComputeDetails{MemoryGB: 6}, rec.Details) + } + } + }) + } +} diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 89740bd..c28f2e0 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -348,7 +348,22 @@ type realRecommenderClient struct { } func (r *realRecommenderClient) ListRecommendations(ctx context.Context, req *recommenderpb.ListRecommendationsRequest) RecommenderIterator { - return &realRecommenderIterator{it: r.client.ListRecommendations(ctx, req)} + it := r.client.ListRecommendations(ctx, req) + fetch := it.InternalFetch + pages := 0 + // The SDK's unstable fetch hook is needed to count empty server pages, + // which Next skips internally. SDK boundary tests guard this dependency. + it.InternalFetch = func(size int, token string) ([]*recommenderpb.Recommendation, string, error) { + if err := ctx.Err(); err != nil { + return nil, "", err + } + if pages >= maxRecsPages { + return nil, "", fmt.Errorf("computeengine: GetRecommendations page cap (%d pages) reached", maxRecsPages) + } + pages++ + return fetch(size, token) + } + return &realRecommenderIterator{it: it} } func (r *realRecommenderClient) Close() error { @@ -401,7 +416,7 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio } it := recClient.ListRecommendations(ctx, req) - for pageIdx := 0; pageIdx < maxRecsPages; pageIdx++ { + for { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("context canceled during pagination: %w", err) } @@ -433,8 +448,6 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio recommendations = append(recommendations, *converted) } } - - return nil, fmt.Errorf("computeengine: GetRecommendations iteration cap (%d items) reached", maxRecsPages) } // GetExistingCommitments retrieves existing Compute Engine CUDs. diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 9bf2242..26a56bc 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -997,22 +997,6 @@ func TestComputeEngineClient_ConvertGCPRecommendation(t *testing.T) { assert.Nil(t, rec.RecurringMonthlyCost) } -// infiniteRecommenderIterator never signals iterator.Done, used to exercise -// the ctx-cancel guard and the maxRecsPages budget cap. -type infiniteRecommenderIterator struct{} - -func (i *infiniteRecommenderIterator) Next() (*recommenderpb.Recommendation, error) { - return &recommenderpb.Recommendation{}, nil -} - -type infiniteRecommenderClient struct{} - -func (c *infiniteRecommenderClient) ListRecommendations(_ context.Context, _ *recommenderpb.ListRecommendationsRequest) RecommenderIterator { - return &infiniteRecommenderIterator{} -} - -func (c *infiniteRecommenderClient) Close() error { return nil } - // TestComputeEngineClient_GetRecommendations_CtxCancelReturnsError asserts // that a canceled context is treated as a terminal stop and returns an error // rather than silently producing a partial result set @@ -1023,23 +1007,12 @@ func TestComputeEngineClient_GetRecommendations_CtxCancelReturnsError(t *testing client, err := NewClient(context.Background(), "test-project", "us-central1") require.NoError(t, err) - client.SetRecommenderClient(&infiniteRecommenderClient{}) + client.SetRecommenderClient(&MockRecommenderClient{}) _, err = client.GetRecommendations(ctx, &common.RecommendationParams{}) require.ErrorIs(t, err, context.Canceled, "canceled context must surface an error, not a partial result set") } -// TestComputeEngineClient_GetRecommendations_PageCapFires asserts that the -// iteration budget terminates an infinite iterator rather than looping forever. -func TestComputeEngineClient_GetRecommendations_PageCapFires(t *testing.T) { - client, err := NewClient(context.Background(), "test-project", "us-central1") - require.NoError(t, err) - client.SetRecommenderClient(&infiniteRecommenderClient{}) - - _, err = client.GetRecommendations(context.Background(), &common.RecommendationParams{}) - require.Error(t, err, "page cap must surface an error when the iterator never terminates") -} - // Synthetic fixture for conversion and insert tests; not a captured service response. func realisticCUDRecommendation() *recommenderpb.Recommendation { vcpuVal, _ := structpb.NewValue(4.0)