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/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..c28f2e0 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" @@ -346,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 { @@ -399,16 +416,13 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio } it := recClient.ListRecommendations(ctx, req) - for pageIdx := 0; ; pageIdx++ { + for { 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 +440,14 @@ 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 } // GetExistingCommitments retrieves existing Compute Engine CUDs. @@ -1095,7 +1110,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 +1129,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 +1155,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 +1177,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 +1296,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 +1311,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..26a56bc 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) @@ -994,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 @@ -1020,36 +1007,13 @@ 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") -} - -// 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 +1047,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 +1139,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 +1194,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 +1303,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 +1319,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 +1440,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 +1652,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 +1684,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 +2040,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 +2090,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)")