From 9ac4f37999dd6dc37d221c4bf321c68ec2d60b50 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 23:18:49 +0100 Subject: [PATCH] fix(gcp): narrow runtime SA recommender access to a single-permission custom role The Cloud Run runtime SA held the predefined roles/recommender.viewer, which grants get/list on every recommender and insight type in the project, while the GKE workload SA (which runs the same scheduled recommendation collector) had no Recommender grant at all. The pinned cloud-commitments-go GCP provider only calls ListRecommendations on projects//locations//recommenders/google.compute.commitment.UsageCommitmentRecommender for commitment recommendations. Replace the broad grant in both modules with a project custom role cudlyRecommendationReader containing only recommender.usageCommitmentRecommendations.list, the same contract the CLI setup wizard provisions (cloud-commitments-cli#2124). Creating the role relies on roles/iam.roleAdmin, which ci-cd-permissions already grants to the deploy SA; there is no fallback to the predefined role. Drop the stale recommender_viewer import from gcp-import-dev-state.sh and add a static test asserting both runtime SAs get exactly that one permission, project-scoped, and no predefined roles/recommender.* role. Closes #788 Co-Authored-By: Claude Opus 5.5 --- scripts/gcp-import-dev-state.sh | 4 - .../modules/compute/gcp/cloud-run/main.tf | 32 ++++- terraform/modules/compute/gcp/gke/main.tf | 25 ++++ .../gcp/runtime_recommender_iam_test.go | 115 ++++++++++++++++++ 4 files changed, 168 insertions(+), 8 deletions(-) create mode 100644 terraform/modules/compute/gcp/runtime_recommender_iam_test.go diff --git a/scripts/gcp-import-dev-state.sh b/scripts/gcp-import-dev-state.sh index 700673564..c5cbc61d7 100755 --- a/scripts/gcp-import-dev-state.sh +++ b/scripts/gcp-import-dev-state.sh @@ -346,10 +346,6 @@ if [ -n "$SA_CLOUDRUN_EXISTS" ]; then import_if_missing \ "module.compute_cloud_run[0].google_project_iam_member.compute_commitment_admin" \ "${PROJECT} roles/compute.admin ${SA_MEMBER}" - - import_if_missing \ - "module.compute_cloud_run[0].google_project_iam_member.recommender_viewer" \ - "${PROJECT} roles/recommender.viewer ${SA_MEMBER}" fi # Scheduler service account + job diff --git a/terraform/modules/compute/gcp/cloud-run/main.tf b/terraform/modules/compute/gcp/cloud-run/main.tf index 76e115953..7b27b6605 100644 --- a/terraform/modules/compute/gcp/cloud-run/main.tf +++ b/terraform/modules/compute/gcp/cloud-run/main.tf @@ -487,11 +487,35 @@ resource "google_project_iam_member" "compute_commitment_writer" { member = "serviceAccount:${google_service_account.cloud_run.email}" } -# Recommender Viewer: read access to the Recommender API (commitment -# recommendations, cost insights) for surfacing optimization suggestions. -resource "google_project_iam_member" "recommender_viewer" { +# Recommendation read access: custom project role cudlyRecommendationReader +# granting only recommender.usageCommitmentRecommendations.list, the one +# permission the collector needs to list +# projects//locations//recommenders/ +# google.compute.commitment.UsageCommitmentRecommender (cloud-commitments-go +# providers/gcp computeengine client). Replaces roles/recommender.viewer, which +# granted get/list on every recommender and insight type in the project +# (issue #788). Same role ID and permission as the CLI setup wizard +# (cloud-commitments-cli#2124). The Cloud SQL / Memorystore Performance +# recommender calls are not covered; they 403 and are logged as a per-service +# warning without failing the collection. +# +# Like cudlyCommitmentWriter, this needs roles/iam.roleAdmin on the deployer +# SA (terraform/environments/gcp/ci-cd-permissions); there is deliberately no +# fallback to the predefined role. +resource "google_project_iam_custom_role" "cudly_recommendation_reader" { + project = var.project_id + role_id = "cudlyRecommendationReader" + title = "CUDly Recommendation Reader" + description = "Minimum permission for CUDly to list Compute Engine committed use discount recommendations." + permissions = [ + "recommender.usageCommitmentRecommendations.list", + ] + stage = "GA" +} + +resource "google_project_iam_member" "recommendation_reader" { project = var.project_id - role = "roles/recommender.viewer" + role = google_project_iam_custom_role.cudly_recommendation_reader.id member = "serviceAccount:${google_service_account.cloud_run.email}" } diff --git a/terraform/modules/compute/gcp/gke/main.tf b/terraform/modules/compute/gcp/gke/main.tf index 4857b89e6..bce59bbd9 100644 --- a/terraform/modules/compute/gcp/gke/main.tf +++ b/terraform/modules/compute/gcp/gke/main.tf @@ -301,6 +301,31 @@ resource "google_service_account_iam_member" "workload_identity" { depends_on = [google_container_cluster.main] } +# Recommendation read access for the workload SA: the GKE deployment runs the +# same collector as Cloud Run (google_cloud_scheduler_job.recommendations +# below), and a GCP account in application_default auth mode collects with +# this SA. Same narrow custom role as compute/gcp/cloud-run: only +# recommender.usageCommitmentRecommendations.list, never the predefined +# roles/recommender.viewer (issue #788). The two modules are mutually exclusive +# per environment (var.compute_platform), so the shared role ID cannot clash. +# Needs roles/iam.roleAdmin on the deployer SA (ci-cd-permissions). +resource "google_project_iam_custom_role" "cudly_recommendation_reader" { + project = var.project_id + role_id = "cudlyRecommendationReader" + title = "CUDly Recommendation Reader" + description = "Minimum permission for CUDly to list Compute Engine committed use discount recommendations." + permissions = [ + "recommender.usageCommitmentRecommendations.list", + ] + stage = "GA" +} + +resource "google_project_iam_member" "recommendation_reader" { + project = var.project_id + role = google_project_iam_custom_role.cudly_recommendation_reader.id + member = "serviceAccount:${google_service_account.workload.email}" +} + # ============================================== # Kubernetes Resources (CONDITIONAL) # ============================================== diff --git a/terraform/modules/compute/gcp/runtime_recommender_iam_test.go b/terraform/modules/compute/gcp/runtime_recommender_iam_test.go new file mode 100644 index 000000000..38eaaf2c8 --- /dev/null +++ b/terraform/modules/compute/gcp/runtime_recommender_iam_test.go @@ -0,0 +1,115 @@ +// Regression test for issue #788: the Cloud Run runtime SA held the broad +// predefined roles/recommender.viewer (get/list on every recommender and +// insight type in the project), while the GKE workload SA had no Recommender +// grant at all. The runtime only lists Compute Engine committed-use +// recommendations through the pinned cloud-commitments-go GCP provider, so +// both runtime SAs must instead hold a project custom role containing exactly +// recommender.usageCommitmentRecommendations.list (the contract the CLI setup +// wizard provisions as cudlyRecommendationReader, cloud-commitments-cli#2124). +package gcp_test + +import ( + "os" + "regexp" + "slices" + "strings" + "testing" +) + +const ( + recommendationReaderRoleID = "cudlyRecommendationReader" + recommendationReaderPermission = "recommender.usageCommitmentRecommendations.list" +) + +var ( + customRoleBlockRe = regexp.MustCompile(`(?s)resource\s+"google_project_iam_custom_role"\s+"([a-z0-9_]+)"\s*\{(.*?)\n\}`) + roleIDRe = regexp.MustCompile(`(?m)^\s*role_id\s*=\s*"([^"]+)"`) + permissionsRe = regexp.MustCompile(`(?s)permissions\s*=\s*\[(.*?)\]`) + quotedRe = regexp.MustCompile(`"([^"]*)"`) + iamMemberBlockRe = regexp.MustCompile(`(?s)resource\s+"google_project_iam_member"\s+"[a-z0-9_]+"\s*\{(.*?)\n\}`) + // Any predefined Recommender role (roles/recommender.viewer, .admin, + // .projectCudViewer, ...) on a non-comment line. + predefinedRecommenderRe = regexp.MustCompile(`roles/recommender\.`) +) + +// codeLines drops whole-line comments so explanatory prose that names the +// old role is not mistaken for a grant. +func codeLines(src string) string { + var out []string + for _, line := range strings.Split(src, "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "#") || strings.HasPrefix(trimmed, "//") { + continue + } + out = append(out, line) + } + return strings.Join(out, "\n") +} + +func TestRuntimeSARecommenderGrantIsNarrow(t *testing.T) { + for _, tc := range []struct { + file string + sa string // runtime service account resource name in the module + }{ + {file: "cloud-run/main.tf", sa: "google_service_account.cloud_run.email"}, + {file: "gke/main.tf", sa: "google_service_account.workload.email"}, + } { + t.Run(tc.file, func(t *testing.T) { + data, err := os.ReadFile(tc.file) + if err != nil { + t.Fatalf("reading %s: %v", tc.file, err) + } + code := codeLines(string(data)) + + if loc := predefinedRecommenderRe.FindStringIndex(code); loc != nil { + t.Errorf("%s must not grant a predefined roles/recommender.* role to the runtime SA (issue #788); found %q", tc.file, code[loc[0]:min(loc[0]+40, len(code))]) + } + + var roleResource string + for _, m := range customRoleBlockRe.FindAllStringSubmatch(code, -1) { + id := roleIDRe.FindStringSubmatch(m[2]) + if id == nil || id[1] != recommendationReaderRoleID { + continue + } + if roleResource != "" { + t.Fatalf("%s defines %s more than once", tc.file, recommendationReaderRoleID) + } + roleResource = m[1] + perms := permissionsRe.FindStringSubmatch(m[2]) + if perms == nil { + t.Fatalf("%s: custom role %s has no permissions list", tc.file, recommendationReaderRoleID) + } + quoted := quotedRe.FindAllStringSubmatch(perms[1], -1) + got := make([]string, 0, len(quoted)) + for _, q := range quoted { + got = append(got, q[1]) + } + if want := []string{recommendationReaderPermission}; !slices.Equal(got, want) { + t.Errorf("%s: custom role %s permissions = %v, want exactly %v", tc.file, recommendationReaderRoleID, got, want) + } + } + if roleResource == "" { + t.Fatalf("%s must define google_project_iam_custom_role with role_id %q (issue #788)", tc.file, recommendationReaderRoleID) + } + + roleRef := regexp.MustCompile(`(?m)^\s*role\s*=\s*google_project_iam_custom_role\.` + regexp.QuoteMeta(roleResource) + `\.id\s*$`) + memberRef := regexp.MustCompile(`(?m)^\s*member\s*=\s*"serviceAccount:\$\{` + regexp.QuoteMeta(tc.sa) + `\}"\s*$`) + projectRef := regexp.MustCompile(`(?m)^\s*project\s*=\s*var\.project_id\s*$`) + bound := 0 + for _, m := range iamMemberBlockRe.FindAllStringSubmatch(code, -1) { + if roleRef.MatchString(m[1]) { + bound++ + if !memberRef.MatchString(m[1]) { + t.Errorf("%s: the %s binding must target the runtime SA %s", tc.file, recommendationReaderRoleID, tc.sa) + } + if !projectRef.MatchString(m[1]) { + t.Errorf("%s: the %s binding must be project-scoped (project = var.project_id)", tc.file, recommendationReaderRoleID) + } + } + } + if bound != 1 { + t.Errorf("%s: want exactly one google_project_iam_member binding %s, got %d", tc.file, recommendationReaderRoleID, bound) + } + }) + } +}