diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index b4415225..f03aae93 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,12 @@ const maxRecsPages = 20 // maxCommitmentsPages caps GCP committed-use discount iteration. const maxCommitmentsPages = 50 +// 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 @@ -487,6 +494,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) @@ -516,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 @@ -526,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 f673b0e4..12d09093 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,53 @@ 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 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") + svc := &MockCommitmentsService{commitments: []*computepb.Commitment{recentCUD()}} + client.SetCommitmentsService(svc) + + _, err := client.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.NotNil(t, svc.lastListReq) + 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()) +} + +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") +} + +// 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()