-
Notifications
You must be signed in to change notification settings - Fork 0
fix(gcp): remove the project from the number call #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
98bb6d9
0451d58
ff42f77
69a7705
9f8e63a
4526404
be61256
40e9ea7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -887,7 +887,11 @@ func (c *LiveGCFClient) GetProjectNumber(ctx context.Context, projectID string) | |
| reqURL := fmt.Sprintf("https://cloudresourcemanager.googleapis.com/v1/projects/%s", | ||
| url.PathEscape(projectID)) | ||
|
|
||
| resp, err := c.Client.DoRequest(ctx, http.MethodGet, reqURL, "") | ||
| // CRM is a global API — omit x-goog-user-project to avoid requiring | ||
| // the API to be enabled on the target project. | ||
| noQuotaClient := *c.Client | ||
| noQuotaClient.QuotaProject = "" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [critical] compilation error The diff sets noQuotaClient.QuotaProject but gcp.Client (defined in internal/gcp/client.go) has no QuotaProject field. The struct contains only httpClient *http.Client and tokenFunc. This code will not compile. Suggested fix: Add a QuotaProject string field to gcp.Client and update DoRequest to set the x-goog-user-project header when non-empty, or take a different approach. |
||
| resp, err := noQuotaClient.DoRequest(ctx, http.MethodGet, reqURL, "") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [critical] logic error / no-op mechanism Even if QuotaProject were added to gcp.Client, DoRequest never reads it or sets the x-goog-user-project HTTP header. Clearing the field would have no effect on the outgoing request. Suggested fix: Modify DoRequest to conditionally set x-goog-user-project based on the QuotaProject field value. |
||
| if err != nil { | ||
| return "", fmt.Errorf("looking up project number: %w", err) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # QualityFlow Tests — GH-11 | ||
|
|
||
| Generated by the QualityFlow pipeline. | ||
|
|
||
| | Directory | Count | Framework | | ||
| |-----------|-------|-----------| | ||
| | `go/` | 1 files | Go | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,232 @@ | ||
| //go:build e2e | ||
|
|
||
| package gcf | ||
|
|
||
| // GH-11 End-to-End Tests: Remove Quota Project from GCP Project Number Lookup | ||
| // | ||
| // These tests validate the behavioral change in GetProjectNumber where the | ||
| // x-goog-user-project header is omitted to avoid requiring the Cloud Resource | ||
| // Manager API to be enabled on the target project. | ||
| // | ||
| // STD Reference: outputs/std/GH-11/GH-11_test_description.yaml | ||
| // STP Reference: outputs/stp/GH-11/GH-11_test_plan.md | ||
| // | ||
| // Compilation prerequisite: gcp.Client must include a QuotaProject string | ||
| // field. See STP Known Limitations (I.2) and Entry Criteria (II.4). | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "sync" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TS-GH-11-001: GetProjectNumber succeeds without quota project header. | ||
| // | ||
| // Validates the core behavioral change of GH-11: the CRM API request must | ||
| // omit the x-goog-user-project header so customers don't need to enable the | ||
| // Cloud Resource Manager API on their target project. | ||
| func TestGetProjectNumber_SuccessWithoutQuotaHeader(t *testing.T) { | ||
| var capturedHeaders http.Header | ||
|
|
||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| capturedHeaders = r.Header.Clone() | ||
| w.WriteHeader(http.StatusOK) | ||
| json.NewEncoder(w).Encode(map[string]string{"projectNumber": "123456789"}) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| client := newTestClient(srv) | ||
|
|
||
| projectNumber, err := client.GetProjectNumber(context.Background(), "my-project") | ||
| require.NoError(t, err, "GetProjectNumber should succeed for a valid project ID") | ||
|
|
||
| assert.Equal(t, "123456789", projectNumber, | ||
| "GetProjectNumber should return the correct project number from the API response") | ||
|
|
||
| assert.Empty(t, capturedHeaders.Get("x-goog-user-project"), | ||
| "Request to CRM API must not include x-goog-user-project header — "+ | ||
| "this is the core fix of GH-11") | ||
| } | ||
|
|
||
| // TS-GH-11-002: Original gcp.Client is not mutated after GetProjectNumber call. | ||
| // | ||
| // Verifies that the value copy of *c.Client isolates the QuotaProject | ||
| // mutation. If the original client is mutated, subsequent API calls from | ||
| // the same client could behave unexpectedly. | ||
| func TestGetProjectNumber_OriginalClientNotMutated(t *testing.T) { | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| json.NewEncoder(w).Encode(map[string]string{"projectNumber": "123456789"}) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| client := newTestClient(srv) | ||
| // Set a known QuotaProject value before the call. | ||
| client.Client.QuotaProject = "original-project-id" | ||
|
|
||
| _, err := client.GetProjectNumber(context.Background(), "test-project") | ||
| require.NoError(t, err, "GetProjectNumber should complete without error") | ||
|
|
||
| assert.Equal(t, "original-project-id", client.Client.QuotaProject, | ||
| "GetProjectNumber must not mutate the original client's QuotaProject field — "+ | ||
| "it should create a value copy with QuotaProject cleared") | ||
| } | ||
|
|
||
| // TS-GH-11-003: GetProjectNumber returns error for HTTP 403 Forbidden. | ||
| // | ||
| // Validates that permission errors from the CRM API are surfaced clearly | ||
| // to help operators diagnose IAM issues during FullSend deployment. | ||
| func TestGetProjectNumber_Forbidden(t *testing.T) { | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| w.WriteHeader(http.StatusForbidden) | ||
| fmt.Fprintln(w, `{"error":{"message":"denied"}}`) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| client := newTestClient(srv) | ||
|
|
||
| _, err := client.GetProjectNumber(context.Background(), "proj") | ||
| require.Error(t, err, "GetProjectNumber should return an error for a 403 response") | ||
|
|
||
| assert.Contains(t, err.Error(), "unexpected status 403", | ||
| "Error message should include the HTTP status code for diagnostics") | ||
| } | ||
|
|
||
| // TS-GH-11-004: GetProjectNumber returns error for empty project number response. | ||
| // | ||
| // Validates that an empty projectNumber in a 200 response is detected | ||
| // and reported, rather than propagating an empty string downstream. | ||
| func TestGetProjectNumber_EmptyProjectNumber(t *testing.T) { | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| json.NewEncoder(w).Encode(map[string]string{"projectNumber": ""}) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| client := newTestClient(srv) | ||
|
|
||
| _, err := client.GetProjectNumber(context.Background(), "proj") | ||
| require.Error(t, err, "GetProjectNumber should return an error when projectNumber is empty") | ||
|
|
||
| assert.Contains(t, err.Error(), "empty project number", | ||
| "Error message should describe the empty project number condition") | ||
| } | ||
|
|
||
| // TS-GH-11-005: provisionSelfManaged workflow succeeds with modified GetProjectNumber. | ||
| // | ||
| // Validates the integration between the modified GetProjectNumber and its | ||
| // caller (provisionSelfManaged). The project number returned by GetProjectNumber | ||
| // is used for subsequent WIF pool/provider operations. | ||
| func TestProvisionSelfManaged_SuccessWithProjectNumber(t *testing.T) { | ||
| fake := newFakeGCFClient() | ||
| fake.projectNumber = "123456789" | ||
| // Configure GetFunction to return nil first (not found), then active after deploy. | ||
| fake.functionInfoAfterCreate = &FunctionInfo{ | ||
| State: "ACTIVE", | ||
| URI: "https://my-func-abc.run.app", | ||
| Region: "us-central1", | ||
| } | ||
|
|
||
| srcDir := fakeFunctionSourceDir(t) | ||
|
|
||
| p := newTestProvisioner(Config{ | ||
| ProjectID: "test-project", | ||
| Region: "us-central1", | ||
| GitHubOrgs: []string{"my-org"}, | ||
| AgentPEMs: singleRolePEMs(), | ||
| AgentAppIDs: singleRoleAppIDs(), | ||
| FunctionSourceDir: srcDir, | ||
| }, fake) | ||
|
|
||
| result, err := p.provisionSelfManaged(context.Background()) | ||
| require.NoError(t, err, "provisionSelfManaged should succeed when GetProjectNumber returns a valid number") | ||
|
|
||
| assert.Contains(t, result, "FULLSEND_MINT_URL", | ||
| "Result map should contain FULLSEND_MINT_URL key") | ||
| assert.NotEmpty(t, result["FULLSEND_MINT_URL"], | ||
| "FULLSEND_MINT_URL should not be empty") | ||
|
|
||
| // Verify GetProjectNumber was called. | ||
| assert.Contains(t, fake.calls, "GetProjectNumber", | ||
| "GetProjectNumber should be called during provisioning") | ||
| } | ||
|
|
||
| // TS-GH-11-006: provisionSelfManaged fails gracefully when GetProjectNumber errors. | ||
| // | ||
| // Validates fail-fast behavior: if the project number cannot be obtained, | ||
| // no downstream GCP operations (WIF pool creation, function deployment) | ||
| // should be attempted. | ||
| func TestProvisionSelfManaged_FailsOnProjectNumberError(t *testing.T) { | ||
| fake := newFakeGCFClient() | ||
| fake.errs["GetProjectNumber"] = fmt.Errorf("permission denied") | ||
|
|
||
| srcDir := fakeFunctionSourceDir(t) | ||
|
|
||
| p := newTestProvisioner(Config{ | ||
| ProjectID: "test-project", | ||
| Region: "us-central1", | ||
| GitHubOrgs: []string{"my-org"}, | ||
| AgentPEMs: singleRolePEMs(), | ||
| AgentAppIDs: singleRoleAppIDs(), | ||
| FunctionSourceDir: srcDir, | ||
| }, fake) | ||
|
|
||
| _, err := p.provisionSelfManaged(context.Background()) | ||
| require.Error(t, err, "provisionSelfManaged should fail when GetProjectNumber returns an error") | ||
|
|
||
| // Verify no downstream operations were attempted after the failure. | ||
| for _, call := range fake.calls { | ||
| switch call { | ||
| case "GetProjectNumber": | ||
| // Expected — this is the call that failed. | ||
| default: | ||
| t.Errorf("Unexpected downstream call %q after GetProjectNumber failure — "+ | ||
| "fail-fast behavior violated", call) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // TS-GH-11-007: Client value copy does not share mutable QuotaProject state. | ||
| // | ||
| // Validates that concurrent GetProjectNumber calls from the same client | ||
| // create independent copies and do not interfere with each other. Run with | ||
| // `go test -race` to detect data races in the copy mechanism. | ||
| func TestGetProjectNumber_ConcurrentClientIsolation(t *testing.T) { | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| json.NewEncoder(w).Encode(map[string]string{"projectNumber": "123456789"}) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| client := newTestClient(srv) | ||
| client.Client.QuotaProject = "shared-project" | ||
|
|
||
| const goroutines = 10 | ||
| var wg sync.WaitGroup | ||
| errs := make([]error, goroutines) | ||
|
|
||
| wg.Add(goroutines) | ||
| for i := 0; i < goroutines; i++ { | ||
| go func(idx int) { | ||
| defer wg.Done() | ||
| _, errs[idx] = client.GetProjectNumber(context.Background(), fmt.Sprintf("project-%d", idx)) | ||
| }(i) | ||
| } | ||
| wg.Wait() | ||
|
|
||
| // All goroutines should complete without error. | ||
| for i, err := range errs { | ||
| assert.NoError(t, err, "Goroutine %d should complete without error", i) | ||
| } | ||
|
|
||
| // Original client state must be preserved after all concurrent calls. | ||
| assert.Equal(t, "shared-project", client.Client.QuotaProject, | ||
| "Original client QuotaProject must not be mutated by concurrent GetProjectNumber calls") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] struct-copy-pattern
The shallow copy pattern is unconventional in this file. Consider adding explicit support in gcp.Client for quota-project-free requests.
Suggested fix: Add a dedicated method or option to gcp.Client for quota-project-free requests.