From 738d2422b6a0ebd34efbf2f27441638b921bfffb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 14:37:08 +0100 Subject: [PATCH 1/2] fix(gcp): list only live commitments so the item cap counts live CUDs GetExistingCommitments listed every regional commitment, including EXPIRED and CANCELLED ones, and errors once more than 50 items come back. A region with a long history would hit that cap every time, which makes a fail-closed recent-commitment check block it permanently. The list request now sends the filter "(status != EXPIRED) (status != CANCELLED)", so the cap applies to live CUDs only. The only in-repo caller, the duplicate checker, already drops those states, so its behavior does not change. The test fake ignores Filter, so the tests assert the exact request sent. Server-side filtering by the real API is unverified. Another test keeps the cap error for more than 50 live items. Part of LeanerCloud/cloud-commitments-platform#682 Co-Authored-By: Claude Sonnet 5.5 --- .../gcp/services/computeengine/client.go | 6 ++++ .../gcp/services/computeengine/client_test.go | 33 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index b4415225..8bc51869 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -27,6 +27,7 @@ import ( "google.golang.org/api/option" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/structpb" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -43,6 +44,10 @@ const maxRecsPages = 20 // maxCommitmentsPages caps GCP committed-use discount iteration. const maxCommitmentsPages = 50 +// liveCommitmentsFilter drops EXPIRED and CANCELLED commitments server-side so +// the item cap counts live CUDs, not a region's full history (AIP-160 syntax). +const liveCommitmentsFilter = "(status != EXPIRED) (status != CANCELLED)" + // maxMachineTypeItems caps GCP machine types iteration (one item per Next() call). const maxMachineTypeItems = 20 @@ -487,6 +492,7 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen req := &computepb.ListRegionCommitmentsRequest{ Project: c.projectID, Region: c.region, + Filter: proto.String(liveCommitmentsFilter), } return c.collectCommitments(ctx, svc, req) diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index f673b0e4..fea26e32 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -177,9 +177,11 @@ type MockCommitmentsService struct { commitments []*computepb.Commitment getCommitment *computepb.Commitment getErr error + lastListReq *computepb.ListRegionCommitmentsRequest // captured for assertions } func (m *MockCommitmentsService) List(ctx context.Context, req *computepb.ListRegionCommitmentsRequest) CommitmentsIterator { + m.lastListReq = req return &MockCommitmentsIterator{commitments: m.commitments, err: m.listErr} } @@ -409,6 +411,37 @@ func TestComputeEngineClient_GetExistingCommitments_WithMock(t *testing.T) { assert.False(t, commitments[0].StartDate.IsZero()) } +// The mock ignores Filter, so this proves only what the client sends. Whether +// the real API honours the filter is unverified (no live GCP call was made). +func TestComputeEngineClient_GetExistingCommitments_ListsOnlyLive(t *testing.T) { + t.Parallel() + client, _ := NewClient("test-project", "us-central1") + svc := &MockCommitmentsService{commitments: []*computepb.Commitment{recentCUD()}} + client.SetCommitmentsService(svc) + + _, err := client.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.NotNil(t, svc.lastListReq) + assert.Equal(t, "(status != EXPIRED) (status != CANCELLED)", svc.lastListReq.GetFilter()) + assert.Equal(t, "test-project", svc.lastListReq.GetProject()) + assert.Equal(t, "us-central1", svc.lastListReq.GetRegion()) +} + +func TestComputeEngineClient_GetExistingCommitments_ItemCapStillErrors(t *testing.T) { + t.Parallel() + client, _ := NewClient("test-project", "us-central1") + cuds := make([]*computepb.Commitment, 0, maxCommitmentsPages+1) + for i := 0; i <= maxCommitmentsPages; i++ { + cuds = append(cuds, recentCUD()) + } + client.SetCommitmentsService(&MockCommitmentsService{commitments: cuds}) + + got, err := client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Nil(t, got) + assert.Contains(t, err.Error(), "iteration cap") +} + func TestComputeEngineClient_GetExistingCommitments_Error(t *testing.T) { t.Parallel() ctx := context.Background() From e8879d377c381c3ab795a5bb363fdfc1d763f929 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:03:41 +0100 Subject: [PATCH 2/2] fix(gcp): build the live-commitment filter from the SDK enum and allow exactly 50 items The filter string held the status names as literals, which the misspell lint flags, and could drift from the API. It is now built from the computepb Commitment status enum values, and the test asserts the same. The item cap was checked before reading the next item, so a list of exactly 50 commitments errored as if the cap had been passed. The check now runs after the end-of-list check, so 50 items succeed and a 51st still errors. A boundary test covers exactly 50 items; it failed with the old order. Part of LeanerCloud/cloud-commitments-platform#682 Co-Authored-By: Claude Opus 5.5 --- .../gcp/services/computeengine/client.go | 15 ++++++++------ .../gcp/services/computeengine/client_test.go | 20 +++++++++++++++++-- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 8bc51869..f03aae93 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -44,9 +44,11 @@ const maxRecsPages = 20 // maxCommitmentsPages caps GCP committed-use discount iteration. const maxCommitmentsPages = 50 -// liveCommitmentsFilter drops EXPIRED and CANCELLED commitments server-side so -// the item cap counts live CUDs, not a region's full history (AIP-160 syntax). -const liveCommitmentsFilter = "(status != EXPIRED) (status != CANCELLED)" +// liveCommitmentsFilter drops expired and canceled commitments server-side so +// the item cap counts live commitments only. It is built from the SDK status +// enum so the status names cannot drift from what the API returns. +var liveCommitmentsFilter = fmt.Sprintf("(status != %s) (status != %s)", + computepb.Commitment_EXPIRED.String(), computepb.Commitment_CANCELLED.String()) //nolint:misspell // SDK identifier // maxMachineTypeItems caps GCP machine types iteration (one item per Next() call). const maxMachineTypeItems = 20 @@ -522,9 +524,6 @@ func (c *Client) collectCommitments(ctx context.Context, svc CommitmentsService, if err := ctx.Err(); err != nil { return nil, fmt.Errorf("context canceled during pagination: %w", err) } - if pageIdx >= maxCommitmentsPages { - return nil, fmt.Errorf("computeengine: GetExistingCommitments iteration cap (%d items) reached", maxCommitmentsPages) - } commitment, err := it.Next() if errors.Is(err, iterator.Done) { break @@ -532,6 +531,10 @@ func (c *Client) collectCommitments(ctx context.Context, svc CommitmentsService, if err != nil { return nil, fmt.Errorf("failed to list commitments: %w", err) } + // Checked after Done so exactly maxCommitmentsPages items still succeed. + if pageIdx >= maxCommitmentsPages { + return nil, fmt.Errorf("computeengine: GetExistingCommitments iteration cap (%d items) reached", maxCommitmentsPages) + } com, err := c.convertGCPCommitmentToCommon(commitment) if err != nil { diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index fea26e32..12d09093 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -412,7 +412,7 @@ func TestComputeEngineClient_GetExistingCommitments_WithMock(t *testing.T) { } // The mock ignores Filter, so this proves only what the client sends. Whether -// the real API honours the filter is unverified (no live GCP call was made). +// the real API honors the filter is unverified (no live GCP call was made). func TestComputeEngineClient_GetExistingCommitments_ListsOnlyLive(t *testing.T) { t.Parallel() client, _ := NewClient("test-project", "us-central1") @@ -422,7 +422,8 @@ func TestComputeEngineClient_GetExistingCommitments_ListsOnlyLive(t *testing.T) _, err := client.GetExistingCommitments(context.Background()) require.NoError(t, err) require.NotNil(t, svc.lastListReq) - assert.Equal(t, "(status != EXPIRED) (status != CANCELLED)", svc.lastListReq.GetFilter()) + want := "(status != " + computepb.Commitment_EXPIRED.String() + ") (status != " + computepb.Commitment_CANCELLED.String() + ")" //nolint:misspell // SDK identifier + assert.Equal(t, want, svc.lastListReq.GetFilter()) assert.Equal(t, "test-project", svc.lastListReq.GetProject()) assert.Equal(t, "us-central1", svc.lastListReq.GetRegion()) } @@ -442,6 +443,21 @@ func TestComputeEngineClient_GetExistingCommitments_ItemCapStillErrors(t *testin assert.Contains(t, err.Error(), "iteration cap") } +// Exactly maxCommitmentsPages items is still a complete list, not a cap hit. +func TestComputeEngineClient_GetExistingCommitments_ExactlyAtCapSucceeds(t *testing.T) { + t.Parallel() + client, _ := NewClient("test-project", "us-central1") + cuds := make([]*computepb.Commitment, 0, maxCommitmentsPages) + for i := 0; i < maxCommitmentsPages; i++ { + cuds = append(cuds, recentCUD()) + } + client.SetCommitmentsService(&MockCommitmentsService{commitments: cuds}) + + got, err := client.GetExistingCommitments(context.Background()) + require.NoError(t, err) + assert.Len(t, got, maxCommitmentsPages) +} + func TestComputeEngineClient_GetExistingCommitments_Error(t *testing.T) { t.Parallel() ctx := context.Background()