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) + } + }) + } +}