From 2e5daf48beb4f9c298d139ff53dfbd346b643180 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 16:24:35 +0200 Subject: [PATCH] fix(gcp): provision Recommender list permission in the setup wizard The wizard granted only Compute Viewer and a custom role with compute.commitments.create, so a newly configured identity could not call the project-scoped UsageCommitmentRecommender. Add a second single-permission custom role (cudlyRecommendationReader: recommender.usageCommitmentRecommendations.list), ensure both roles before the first setIamPolicy write, print the recommender.googleapis.com prerequisite, and document the permission contract and project vs billing scope. Closes #2124 Co-Authored-By: Claude Sonnet 5.5 --- cmd/configure_gcp.go | 44 ++++++++++++++++++++++++----------- cmd/configure_gcp_iam_test.go | 44 +++++++++++++++++++++++++++++------ docs/cli/cloud-setup.md | 31 ++++++++++++++++++++---- 3 files changed, 95 insertions(+), 24 deletions(-) diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index ac9616e59..c16ddbd43 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -780,9 +780,21 @@ func gcpStepCreateServiceAccount(ctx context.Context, reader *bufio.Reader, proj return saEmail, nil } -const gcpPurchaserRoleID = "cudlyCommitmentPurchaser" +// gcpCustomRole is a single-permission custom role the wizard provisions. +type gcpCustomRole struct { + id string + title string + permission string +} + +var ( + gcpPurchaserRole = gcpCustomRole{id: "cudlyCommitmentPurchaser", title: "CUDly Commitment Purchaser", permission: "compute.commitments.create"} + // gcpRecommendationReaderRole covers the project-scoped + // google.compute.commitment.UsageCommitmentRecommender list call. + gcpRecommendationReaderRole = gcpCustomRole{id: "cudlyRecommendationReader", title: "CUDly Recommendation Reader", permission: "recommender.usageCommitmentRecommendations.list"} +) -func ensureGCPPurchaserRole(ctx context.Context, projectID string) (string, error) { +func ensureGCPCustomRole(ctx context.Context, projectID string, spec gcpCustomRole) (string, error) { ctx, cancel := context.WithTimeout(ctx, gcpSDKCallTimeout) defer cancel() opt, err := newGCPAPIOption(ctx) @@ -794,21 +806,21 @@ func ensureGCPPurchaserRole(ctx context.Context, projectID string) (string, erro return "", fmt.Errorf("failed to create IAM client: %w", err) } parent := "projects/" + projectID - name := parent + "/roles/" + gcpPurchaserRoleID + name := parent + "/roles/" + spec.id + want := []string{spec.permission} role, err := svc.Projects.Roles.Get(name).Context(ctx).Do() var apiErr *googleapi.Error if errors.As(err, &apiErr) && apiErr.Code == 404 { role, err = svc.Projects.Roles.Create(parent, &iamv1.CreateRoleRequest{ - RoleId: gcpPurchaserRoleID, - Role: &iamv1.Role{Title: "CUDly Commitment Purchaser", Stage: "GA", - IncludedPermissions: []string{"compute.commitments.create"}}, + RoleId: spec.id, + Role: &iamv1.Role{Title: spec.title, Stage: "GA", IncludedPermissions: want}, }).Context(ctx).Do() } if err != nil { return "", fmt.Errorf("failed to provision custom role %s; review the role and setup permissions before retrying: %w", name, err) } - if role.Name != name || role.Deleted || role.Stage == "DISABLED" || !slices.Equal(role.IncludedPermissions, []string{"compute.commitments.create"}) { - return "", fmt.Errorf("unsafe custom role %s: expected an enabled role containing only compute.commitments.create; review it before retrying", name) + if role.Name != name || role.Deleted || role.Stage == "DISABLED" || !slices.Equal(role.IncludedPermissions, want) { + return "", fmt.Errorf("unsafe custom role %s: expected an enabled role containing only %s; review it before retrying", name, spec.permission) } return name, nil } @@ -821,7 +833,9 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm fmt.Println("-----------------------") fmt.Println("Grant the required roles to the service account.") fmt.Println() - fmt.Printf("[R]un, [S]kip? (creates or validates %s with compute.commitments.create, then grants it and roles/compute.viewer to %s on project %s via SDK) ", gcpPurchaserRoleID, saEmail, projectID) + fmt.Println("Prerequisite: enable recommender.googleapis.com in the service account's project (this step does not enable it).") + fmt.Printf("[R]un, [S]kip? (creates or validates %s with %s and %s with %s, then grants both and roles/compute.viewer to %s on project %s via SDK) ", + gcpPurchaserRole.id, gcpPurchaserRole.permission, gcpRecommendationReaderRole.id, gcpRecommendationReaderRole.permission, saEmail, projectID) choice, err := reader.ReadString('\n') if err != nil { @@ -829,11 +843,15 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm } switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": - role, roleErr := ensureGCPPurchaserRole(ctx, projectID) - if roleErr != nil { - return roleErr + grants := []string{"roles/compute.viewer"} + for _, spec := range []gcpCustomRole{gcpPurchaserRole, gcpRecommendationReaderRole} { + role, roleErr := ensureGCPCustomRole(ctx, projectID, spec) + if roleErr != nil { + return roleErr + } + grants = append(grants, role) } - for _, grant := range []string{"roles/compute.viewer", role} { + for _, grant := range grants { if grantErr := grantGCPIAMRole(ctx, projectID, member, grant); grantErr != nil { return grantErr } diff --git a/cmd/configure_gcp_iam_test.go b/cmd/configure_gcp_iam_test.go index 01dedf145..54d9fdd26 100644 --- a/cmd/configure_gcp_iam_test.go +++ b/cmd/configure_gcp_iam_test.go @@ -26,7 +26,7 @@ func TestGCPStepGrantRole(t *testing.T) { runGCPIAMFixture(t, scenario) return } - for _, scenario := range []string{"create", "empty", "reuse", "extra-permission", "deleted", "disabled", "wrong-name", "bad-created", "forbidden", "write-error", "skip", "conditional-other", "conditional-member", "already-unconditional", "existing-admin", "coordinator", "coordinator-error", "coordinator-conflict", "coordinator-second-write"} { + for _, scenario := range []string{"create", "empty", "reuse", "extra-permission", "deleted", "disabled", "wrong-name", "bad-created", "forbidden", "write-error", "skip", "conditional-other", "conditional-member", "already-unconditional", "existing-admin", "coordinator", "coordinator-error", "coordinator-conflict", "coordinator-second-write", "reader-create", "reader-extra-permission", "reader-deleted", "reader-wrong-name"} { t.Run(scenario, func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -43,8 +43,19 @@ func runGCPIAMFixture(t *testing.T, scenario string) { const roleName = "projects/fixture-project/roles/cudlyCommitmentPurchaser" const member = "serviceAccount:cudly-service-account@fixture-project.iam.gserviceaccount.com" role := &iam.Role{Name: roleName, Stage: "GA", IncludedPermissions: []string{"compute.commitments.create"}} + const readerRoleName = "projects/fixture-project/roles/cudlyRecommendationReader" + reader := &iam.Role{Name: readerRoleName, Stage: "GA", IncludedPermissions: []string{"recommender.usageCommitmentRecommendations.list"}} wantError := "" switch scenario { + case "reader-extra-permission": + reader.IncludedPermissions = append(reader.IncludedPermissions, "recommender.usageCommitmentRecommendations.get") + wantError = "unsafe custom role" + case "reader-deleted": + reader.Deleted = true + wantError = "unsafe custom role" + case "reader-wrong-name": + reader.Name = "projects/fixture-project/roles/other" + wantError = "unsafe custom role" case "extra-permission", "bad-created": role.IncludedPermissions = append(role.IncludedPermissions, "compute.instances.delete") wantError = "unsafe custom role" @@ -79,7 +90,7 @@ func runGCPIAMFixture(t *testing.T, scenario string) { } var writes []*crm.SetIamPolicyRequest var creates []*iam.CreateRoleRequest - var roleReads, keyCalls int + var roleReads, readerReads, keyCalls int var mu sync.Mutex server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { mu.Lock() @@ -88,6 +99,13 @@ func runGCPIAMFixture(t *testing.T, scenario string) { switch { case r.URL.Path == "/token": _, _ = io.WriteString(w, `{"access_token":"fixture","token_type":"Bearer","expires_in":3600}`) + case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/roles/cudlyRecommendationReader"): + readerReads++ + if scenario == "reader-create" { + http.Error(w, `{"error":{"code":404,"message":"missing"}}`, http.StatusNotFound) + return + } + _ = json.NewEncoder(w).Encode(reader) case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/roles/cudlyCommitmentPurchaser"): roleReads++ switch scenario { @@ -106,6 +124,10 @@ func runGCPIAMFixture(t *testing.T, scenario string) { http.Error(w, "conflict", http.StatusConflict) return } + if request.RoleId == "cudlyRecommendationReader" { + _ = json.NewEncoder(w).Encode(reader) + return + } _ = json.NewEncoder(w).Encode(role) case strings.HasSuffix(r.URL.Path, ":getIamPolicy"): var request crm.GetIamPolicyRequest @@ -191,7 +213,7 @@ func runGCPIAMFixture(t *testing.T, scenario string) { require.Zero(t, keyCalls, "role setup must never mint a key or call unexpected endpoints") wantCreates := 0 switch scenario { - case "create", "empty", "bad-created", "coordinator", "coordinator-conflict": + case "create", "empty", "bad-created", "coordinator", "coordinator-conflict", "reader-create": wantCreates = 1 } require.Len(t, creates, wantCreates) @@ -201,9 +223,17 @@ func runGCPIAMFixture(t *testing.T, scenario string) { return } require.Equal(t, 1, roleReads) + if strings.HasPrefix(scenario, "reader-") || wantError == "" || scenario == "write-error" || scenario == "coordinator" { + require.Equal(t, 1, readerReads, "reader role must be ensured before any setIamPolicy write") + } for _, create := range creates { - require.Equal(t, "cudlyCommitmentPurchaser", create.RoleId) require.Empty(t, create.Role.Name) + if scenario == "reader-create" { + require.Equal(t, "cudlyRecommendationReader", create.RoleId) + require.Equal(t, []string{"recommender.usageCommitmentRecommendations.list"}, create.Role.IncludedPermissions) + continue + } + require.Equal(t, "cudlyCommitmentPurchaser", create.RoleId) require.Equal(t, []string{"compute.commitments.create"}, create.Role.IncludedPermissions) } if scenario == "coordinator-second-write" { @@ -220,9 +250,9 @@ func runGCPIAMFixture(t *testing.T, scenario string) { require.Len(t, writes, 1) return } - wantWrites := 2 + wantWrites := 3 if scenario == "already-unconditional" { - wantWrites = 1 + wantWrites = 2 } require.Len(t, writes, wantWrites) granted := map[string]bool{} @@ -235,7 +265,7 @@ func runGCPIAMFixture(t *testing.T, scenario string) { } } } - expected := map[string]bool{"roles/compute.viewer": true, roleName: true} + expected := map[string]bool{"roles/compute.viewer": true, roleName: true, readerRoleName: true} if scenario == "existing-admin" { expected["roles/compute.admin"] = true } diff --git a/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index d0200e13b..3ff847d1c 100644 --- a/docs/cli/cloud-setup.md +++ b/docs/cli/cloud-setup.md @@ -138,17 +138,40 @@ The Service Account needs the following roles: |------|---------| | `roles/compute.viewer` | Read Compute Engine resources and commitment operations | | `projects/PROJECT_ID/roles/cudlyCommitmentPurchaser` | Purchase commitments with only `compute.commitments.create` | +| `projects/PROJECT_ID/roles/cudlyRecommendationReader` | List Compute commitment recommendations with only `recommender.usageCommitmentRecommendations.list` | The setup operator needs `iam.roles.get`, `iam.roles.create`, `resourcemanager.projects.getIamPolicy`, and `resourcemanager.projects.setIamPolicy` for this step. These setup permissions are not granted to the Service Account. -An existing custom role must have exactly the purchase permission and be enabled; -the wizard refuses incompatible roles rather than changing them. It also refuses +An existing custom role must have exactly its one permission and be enabled (not +disabled and not soft-deleted); the wizard refuses incompatible roles rather +than changing them. It also refuses to widen an existing conditional-only grant for this Service Account. Rerunning the wizard does not remove broad grants from older installations. -Recommendation access requires additional Recommender permissions; neither -role above provides them. + +#### Recommendation access + +Compute Engine commitment recommendations are read from the project-scoped +resource `projects/PROJECT_ID/locations/REGION/recommenders/google.compute.commitment.UsageCommitmentRecommender`. +That list call needs `recommender.usageCommitmentRecommendations.list` on the +project, which the wizard grants through `cudlyRecommendationReader`. The +Recommender API (`recommender.googleapis.com`) must be enabled in the Service +Account's project; the wizard prints this prerequisite but does not enable it. + +Scope notes: + +- Project scope: the grant above is a project binding. Nothing is granted on a + billing account, folder or organization, because the pinned client never + queries those parents. Billing-account roles such as `roles/billing.viewer` + are not needed for this call. +- Other services: the GCP provider also queries Cloud SQL and Memorystore + recommenders (`google.cloudsql.instance.PerformanceRecommender`, + `google.memorystore.redis.PerformanceRecommender`). The wizard provisions + nothing for them; those calls return a permission error, which is reported + as a warning for that service while other services continue. + +Rerunning the wizard does not remove broad grants from older installations. ### Migrating legacy Compute Admin grants