From 9979fed61678e466e6af239d9875494d51e22156 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 07:42:47 +0200 Subject: [PATCH 1/2] fix(gcp): skip a recommendation with a bad amount and cap string VCPU amounts A malformed VCPU or MEMORY amount on one Recommender row aborted GetRecommendations and dropped every other row. Such a row is now skipped with a warning, like the AWS and Azure paths. Structural payload errors (unknown resource type, missing VCPU, duplicate amounts) still abort. recommendationAmount now caps string amounts at 2^53-1 for VCPU as well as MEMORY, so the MEMORY-only check is removed. Closes #237 --- .../gcp/services/computeengine/client.go | 41 ++++++++++++------ .../gcp/services/computeengine/client_test.go | 43 ++++++++++++++++++- 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 9e8a013..53405a0 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -448,7 +448,7 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio continue } - converted, err := c.convertGCPRecommendation(ctx, rec, params) + converted, err := c.convertOrSkipBadAmount(ctx, rec, params) if err != nil { return nil, fmt.Errorf("computeengine: recommendation %q: %w", rec.GetName(), err) } @@ -458,6 +458,19 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio } } +// convertOrSkipBadAmount converts rec, returning nil with a logged warning when +// its amount is malformed so one bad row does not discard the rest of the +// project's recommendations (same policy as the AWS and Azure paths). +// Structural payload errors are still returned and abort the batch. +func (c *Client) convertOrSkipBadAmount(ctx context.Context, rec *recommenderpb.Recommendation, params common.RecommendationParams) (*common.Recommendation, error) { + converted, err := c.convertGCPRecommendation(ctx, rec, params) + if errors.Is(err, errBadAmount) { + log.Printf("computeengine: skipping recommendation %q: %v", rec.GetName(), err) + return nil, nil + } + return converted, err +} + // GetExistingCommitments retrieves existing Compute Engine CUDs. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { svc, err := c.createCommitmentsService(ctx) @@ -1417,37 +1430,45 @@ func recommendationVCPUAmount(value *structpb.Value) (int, error) { return 0, err } if amount > math.MaxInt { - return 0, fmt.Errorf("VCPU amount must be positive and fit int") + return 0, fmt.Errorf("%w: VCPU amount must fit int", errBadAmount) } return int(amount), nil } +// errBadAmount marks a malformed or out-of-range resource amount, which makes +// GetRecommendations skip that one recommendation instead of failing the batch. +var errBadAmount = errors.New("bad recommendation amount") + // maxExactAmount is the largest integer a float64 (JSON number, or the // ComputeDetails.MemoryGB downstream) represents exactly. const maxExactAmount = 1<<53 - 1 // recommendationAmount parses a resource amount that the Recommender may -// carry as a decimal int64 string or as an exact positive JSON number (at most -// 2^53-1, beyond which a JSON number has already lost precision). +// carry as a decimal int64 string or as an exact positive JSON number. Both +// forms are capped at 2^53-1: beyond it a JSON number has already lost +// precision, and ComputeDetails.MemoryGB (float64) cannot hold a larger value. func recommendationAmount(value *structpb.Value, name string) (int64, 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 %s amount %q: %w", name, v.StringValue, err) + return 0, fmt.Errorf("%w: invalid %s amount %q: %v", errBadAmount, name, v.StringValue, err) + } + if parsed > maxExactAmount { + return 0, fmt.Errorf("%w: %s amount must be at most 2^53-1", errBadAmount, name) } amount = parsed case *structpb.Value_NumberValue: if math.IsNaN(v.NumberValue) || v.NumberValue > maxExactAmount || math.Trunc(v.NumberValue) != v.NumberValue { - return 0, fmt.Errorf("%s amount must be a positive exact integer", name) + return 0, fmt.Errorf("%w: %s amount must be a positive exact integer", errBadAmount, name) } amount = int64(v.NumberValue) default: - return 0, fmt.Errorf("%s amount must be a decimal string or number", name) + return 0, fmt.Errorf("%w: %s amount must be a decimal string or number", errBadAmount, name) } if amount <= 0 { - return 0, fmt.Errorf("%s amount must be positive", name) + return 0, fmt.Errorf("%w: %s amount must be positive", errBadAmount, name) } return amount, nil } @@ -1477,10 +1498,6 @@ func memoryMBFromOperationGroups(content *recommenderpb.RecommendationContent) ( if err != nil { return 0, err } - // MemoryGB is a float64, so larger string amounts would not survive the round trip. - if memMB > maxExactAmount { - return 0, fmt.Errorf("MEMORY amount must be at most 2^53-1") - } } } return memMB, nil diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index c66c4f8..433fe1a 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -1704,13 +1704,14 @@ func TestRecommendationVCPUAmountRejectsInvalidQuantities(t *testing.T) { 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"), + structpb.NewStringValue("9007199254740992"), structpb.NewStringValue(strconv.FormatInt(int64(math.MaxInt), 10)), } { _, err := recommendationVCPUAmount(value) require.Error(t, err, "value %v", value) } - count, err := recommendationVCPUAmount(structpb.NewStringValue(strconv.FormatInt(int64(math.MaxInt), 10))) + count, err := recommendationVCPUAmount(structpb.NewStringValue("9007199254740991")) require.NoError(t, err) - assert.Equal(t, math.MaxInt, count) + assert.Equal(t, 1<<53-1, count) } func TestConvertGCPRecommendationMemoryAmountRepresentations(t *testing.T) { @@ -2417,3 +2418,41 @@ func TestGetRecommendationsStringMemoryAmountEndToEnd(t *testing.T) { require.NoError(t, err) assert.Equal(t, int64(6144), amount) } + +func badAmountRecommendation(name, vcpu, memory string) *recommenderpb.Recommendation { + rec := commitmentOnlyCUDRecommendation() + rec.Name = name + rec.StateInfo = &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE} + ops := rec.Content.OperationGroups[0].Operations + ops[0].PathValue = &recommenderpb.Operation_Value{Value: structpb.NewStringValue(vcpu)} + ops[1].PathValue = &recommenderpb.Operation_Value{Value: structpb.NewStringValue(memory)} + return rec +} + +func TestGetRecommendations_SkipsBadAmountRowKeepsOthers(t *testing.T) { + cases := []struct{ name, vcpu, memory string }{ + {"malformed vcpu", "abc", "6144"}, + {"malformed memory", "4", "6Gi"}, + {"oversized vcpu string", "9007199254740992", "6144"}, + {"oversized memory string", "4", "9223372036854775807"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + client.SetRecommenderClient(&MockRecommenderClient{iterator: &MockRecommenderIterator{ + recommendations: []*recommenderpb.Recommendation{ + badAmountRecommendation("good-before", "4", "6144"), + badAmountRecommendation("bad", tc.vcpu, tc.memory), + badAmountRecommendation("good-after", "8", "12288"), + }, + }}) + + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) + require.NoError(t, err) + require.Len(t, recs, 2) + assert.Equal(t, 4, recs[0].Count) + assert.Equal(t, 8, recs[1].Count) + }) + } +} From 97460a1f470bcedef953f5d4d8973320c98b66ad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 07:48:33 +0200 Subject: [PATCH 2/2] test(gcp): pin that structural recommendation errors still abort the batch Wrap the ParseInt cause with a second %w for errorlint. --- .../gcp/services/computeengine/client.go | 2 +- .../gcp/services/computeengine/client_test.go | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 53405a0..4fabe98 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -1453,7 +1453,7 @@ func recommendationAmount(value *structpb.Value, name string) (int64, error) { case *structpb.Value_StringValue: parsed, err := strconv.ParseInt(v.StringValue, 10, 64) if err != nil { - return 0, fmt.Errorf("%w: invalid %s amount %q: %v", errBadAmount, name, v.StringValue, err) + return 0, fmt.Errorf("%w: invalid %s amount %q: %w", errBadAmount, name, v.StringValue, err) } if parsed > maxExactAmount { return 0, fmt.Errorf("%w: %s amount must be at most 2^53-1", errBadAmount, name) diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 433fe1a..3b8dec1 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -2456,3 +2456,33 @@ func TestGetRecommendations_SkipsBadAmountRowKeepsOthers(t *testing.T) { }) } } + +func TestGetRecommendations_StructuralErrorsStillAbort(t *testing.T) { + cases := map[string]func(*recommenderpb.Recommendation){ + "duplicate vcpu": func(r *recommenderpb.Recommendation) { + g := r.Content.OperationGroups[0] + g.Operations = append(g.Operations, g.Operations[0]) + }, + "unknown resource type": func(r *recommenderpb.Recommendation) { + r.Content.OperationGroups[0].Operations[1].PathFilters["/resources/1/type"] = structpb.NewStringValue("BOGUS") + }, + "missing vcpu": func(r *recommenderpb.Recommendation) { + r.Content.OperationGroups[0].Operations = r.Content.OperationGroups[0].Operations[1:] + }, + } + for name, mutate := range cases { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + bad := badAmountRecommendation("bad", "4", "6144") + mutate(bad) + client.SetRecommenderClient(&MockRecommenderClient{iterator: &MockRecommenderIterator{ + recommendations: []*recommenderpb.Recommendation{badAmountRecommendation("good", "4", "6144"), bad}, + }}) + + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) + require.Error(t, err) + assert.Nil(t, recs) + }) + } +}