From 2e2c0b221a04d05edc148ec11af2b7b22972c4de Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Mon, 28 Sep 2026 05:47:12 +0200
Subject: [PATCH 1/5] sec(iac/gcp): scope gcp-sa-impersonation to
 least-privilege commitment roles

The bundle granted the impersonated target SA two broad project-scope roles:
roles/commerceorgpolicy.commitmentAdmin (organization-scoped) and
roles/billing.viewer (billing-account-scoped). Neither can actually be
granted via google_project_iam_member at project scope; both 400 on apply,
matching the failure mode the cloud-run module and the sibling gcp-target
bundle already document for these exact roles.

Replace both with the same pattern the gcp-target bundle and the
terraform/modules/compute/gcp/cloud-run module use: a project-scoped custom
role (compute.commitments.create/update) plus the built-in
roles/compute.viewer for read access. There is no billing-read path in this
bundle to scope roles/billing.viewer to, so it is dropped.

Regression test: iac/gcp_sa_impersonation_test.go asserts the two GCP
onboarding bundles (gcp-sa-impersonation, gcp-target) grant an equivalent
commitment-permission set, and that gcp-sa-impersonation's main.tf never
grants the two invalid project-scope roles again. Confirmed it fails against
the pre-fix main.tf/variables.tf (git stash of the fix) and passes after.

Closes #129
---
 .../gcp-sa-impersonation/terraform/main.tf    | 39 ++++++--
 .../terraform/variables.tf                    | 29 ++++++
 iac/gcp_sa_impersonation_test.go              | 94 +++++++++++++++++++
 3 files changed, 155 insertions(+), 7 deletions(-)
 create mode 100644 iac/gcp_sa_impersonation_test.go

diff --git a/iac/federation/gcp-sa-impersonation/terraform/main.tf b/iac/federation/gcp-sa-impersonation/terraform/main.tf
index 83dccd8b2..edb39e511 100644
--- a/iac/federation/gcp-sa-impersonation/terraform/main.tf
+++ b/iac/federation/gcp-sa-impersonation/terraform/main.tf
@@ -21,17 +21,42 @@ resource "google_service_account_iam_member" "cudly_impersonate" {
 }
 
 # Grant the target SA permissions needed to manage CUDs/commitments.
-# commitmentAdmin: create/manage committed-use discounts.
-# billing.viewer:  read billing data (costs, invoices). Read-only —
-#                  billing.user is unnecessarily broad for CUDly's use case.
-resource "google_project_iam_member" "cudly_commitment_admin" {
+#
+# roles/commerceorgpolicy.commitmentAdmin and roles/billing.viewer are
+# organization- and billing-account-scoped respectively; granting either at
+# project scope via google_project_iam_member 400s (same failure mode
+# terraform/modules/compute/gcp/cloud-run/main.tf:484 documents for
+# billing.viewer, and federation/gcp-target/terraform/variables.tf:198-201
+# documents for commitmentAdmin). Neither can work here.
+#
+# Instead grant exactly what CUDly needs, mirroring the sibling
+# gcp-target bundle (iac/federation/gcp-target/terraform/main.tf) so the
+# two onboarding paths converge on the same permission set: a custom role
+# holding only the two commitment-write permissions, plus the built-in
+# compute.viewer role for read access (regions/zones/machineTypes/commitments
+# .list/.get). There is no billing-read path in this bundle (no
+# billing_account_id input to scope roles/billing.viewer to), so it is
+# dropped rather than reintroduced broken.
+resource "google_project_iam_custom_role" "cudly" {
   project = var.project_id
-  role    = "roles/commerceorgpolicy.commitmentAdmin"
+  role_id = var.custom_role_id
+  title   = "CUDly Commitment Writer"
+  # Description matches terraform/modules/compute/gcp/cloud-run and
+  # federation/gcp-target exactly so applies from any of the three don't
+  # stomp each other.
+  description = "Minimum permissions for CUDly to purchase and update committed use discounts."
+  permissions = var.custom_role_permissions
+  stage       = "GA"
+}
+
+resource "google_project_iam_member" "cudly_custom" {
+  project = var.project_id
+  role    = google_project_iam_custom_role.cudly.id
   member  = "serviceAccount:${var.service_account_email}"
 }
 
-resource "google_project_iam_member" "cudly_billing_viewer" {
+resource "google_project_iam_member" "cudly_compute_viewer" {
   project = var.project_id
-  role    = "roles/billing.viewer"
+  role    = "roles/compute.viewer"
   member  = "serviceAccount:${var.service_account_email}"
 }
diff --git a/iac/federation/gcp-sa-impersonation/terraform/variables.tf b/iac/federation/gcp-sa-impersonation/terraform/variables.tf
index 728cd2295..095657cd5 100644
--- a/iac/federation/gcp-sa-impersonation/terraform/variables.tf
+++ b/iac/federation/gcp-sa-impersonation/terraform/variables.tf
@@ -13,6 +13,35 @@ variable "source_service_account" {
   type        = string
 }
 
+variable "custom_role_id" {
+  description = <<-EOT
+    Project-scoped custom role ID Terraform creates and binds to
+    var.service_account_email. The role carries the minimum permissions
+    required to purchase and manage Compute Engine CUDs on behalf of CUDly.
+    Matches the default in federation/gcp-target/terraform so applies from
+    either bundle stay idempotent against the same project.
+  EOT
+  type        = string
+  default     = "cudlyCommitmentWriter"
+}
+
+variable "custom_role_permissions" {
+  description = <<-EOT
+    Permissions bundled into the custom role granted to
+    var.service_account_email. Defaults match the definition in
+    terraform/modules/compute/gcp/cloud-run and federation/gcp-target, so all
+    three stay in lockstep and none can stomp another's role on apply.
+
+    Read-side permissions (regions/zones/machineTypes/commitments.list/.get)
+    come from roles/compute.viewer, granted separately in main.tf.
+  EOT
+  type        = list(string)
+  default = [
+    "compute.commitments.create",
+    "compute.commitments.update",
+  ]
+}
+
 variable "cudly_api_url" {
   description = "CUDly API base URL for automatic account registration. Leave empty to skip registration."
   type        = string
diff --git a/iac/gcp_sa_impersonation_test.go b/iac/gcp_sa_impersonation_test.go
new file mode 100644
index 000000000..f0b8b0b3c
--- /dev/null
+++ b/iac/gcp_sa_impersonation_test.go
@@ -0,0 +1,94 @@
+package iac
+
+import (
+	"regexp"
+	"slices"
+	"strings"
+	"testing"
+)
+
+// These tests assert that the two GCP onboarding bundles (gcp-sa-impersonation
+// and gcp-target) grant an equivalent permission set to the CUDly service
+// account, so they cannot drift back into granting different, organization-
+// or billing-account-scoped roles at project scope (#129).
+
+const (
+	gcpSAImpersonationMainTF = "federation/gcp-sa-impersonation/terraform/main.tf"
+	gcpSAImpersonationVarsTF = "federation/gcp-sa-impersonation/terraform/variables.tf"
+	gcpTargetVarsTF          = "federation/gcp-target/terraform/variables.tf"
+)
+
+func readModuleFile(t *testing.T, path string) string {
+	t.Helper()
+	raw, err := Modules.ReadFile(path)
+	if err != nil {
+		t.Fatalf("read %s: %v", path, err)
+	}
+	return string(raw)
+}
+
+// extractCustomRolePermissions pulls the quoted string literals out of the
+// `variable "custom_role_permissions" { ... default = [ ... ] }` block in a
+// federation module's variables.tf.
+func extractCustomRolePermissions(t *testing.T, tf, path string) []string {
+	t.Helper()
+	varRe := regexp.MustCompile(`(?s)variable "custom_role_permissions" \{.*?default = \[(.*?)\]`)
+	m := varRe.FindStringSubmatch(tf)
+	if m == nil {
+		t.Fatalf("%s: could not find variable \"custom_role_permissions\" default block", path)
+	}
+	litRe := regexp.MustCompile(`"([^"]+)"`)
+	lits := litRe.FindAllStringSubmatch(m[1], -1)
+	perms := make([]string, 0, len(lits))
+	for _, l := range lits {
+		perms = append(perms, l[1])
+	}
+	if len(perms) == 0 {
+		t.Fatalf("%s: custom_role_permissions default has no permissions", path)
+	}
+	return perms
+}
+
+// TestGCPBundlesGrantEquivalentCommitmentPermissions guards #129: the
+// sa-impersonation and gcp-target bundles must converge on the same
+// commitment-write permission set rather than one granting a broader,
+// project-scope-invalid role.
+func TestGCPBundlesGrantEquivalentCommitmentPermissions(t *testing.T) {
+	saImpersonationPerms := extractCustomRolePermissions(t, readModuleFile(t, gcpSAImpersonationVarsTF), gcpSAImpersonationVarsTF)
+	targetPerms := extractCustomRolePermissions(t, readModuleFile(t, gcpTargetVarsTF), gcpTargetVarsTF)
+
+	sortedSA := slices.Clone(saImpersonationPerms)
+	slices.Sort(sortedSA)
+	sortedTarget := slices.Clone(targetPerms)
+	slices.Sort(sortedTarget)
+
+	if !slices.Equal(sortedSA, sortedTarget) {
+		t.Errorf("gcp-sa-impersonation custom_role_permissions %v != gcp-target custom_role_permissions %v; the two onboarding paths must grant the same permission set",
+			sortedSA, sortedTarget)
+	}
+}
+
+// TestGCPSAImpersonationDoesNotGrantInvalidProjectScopeRoles guards the two
+// broken grants #129 reported: roles/commerceorgpolicy.commitmentAdmin is
+// organization-scoped and roles/billing.viewer is billing-account-scoped, so
+// granting either via google_project_iam_member (project scope) 400s. Neither
+// may reappear in the bundle's main.tf.
+func TestGCPSAImpersonationDoesNotGrantInvalidProjectScopeRoles(t *testing.T) {
+	tf := readModuleFile(t, gcpSAImpersonationMainTF)
+
+	// Match an actual `role = "roles/..."` assignment, not prose (this file's
+	// own comments name both roles to explain why they were removed).
+	if regexp.MustCompile(`role\s*=\s*"roles/commerceorgpolicy\.commitmentAdmin"`).MatchString(tf) {
+		t.Errorf("%s: grants roles/commerceorgpolicy.commitmentAdmin, which is organization-scoped and 400s at project scope", gcpSAImpersonationMainTF)
+	}
+	if regexp.MustCompile(`role\s*=\s*"roles/billing\.viewer"`).MatchString(tf) {
+		t.Errorf("%s: grants roles/billing.viewer, which is billing-account-scoped and 400s at project scope (see terraform/modules/compute/gcp/cloud-run/main.tf for the correct google_billing_account_iam_member shape if a billing read path is ever added here)", gcpSAImpersonationMainTF)
+	}
+
+	if !strings.Contains(tf, `resource "google_project_iam_custom_role" "cudly"`) {
+		t.Errorf("%s: expected a google_project_iam_custom_role granting the minimum commitment-write permissions", gcpSAImpersonationMainTF)
+	}
+	if !strings.Contains(tf, `role    = "roles/compute.viewer"`) {
+		t.Errorf("%s: expected roles/compute.viewer for read access (regions/zones/machineTypes/commitments.list/.get)", gcpSAImpersonationMainTF)
+	}
+}

From fb03b6b44ad71fdbf814a2cee526c642345d32da Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Mon, 28 Sep 2026 06:15:55 +0200
Subject: [PATCH 2/5] fix(iac/gcp): let gcp-sa-impersonation skip creating a
 colliding custom role

CodeRabbit flagged a real apply-time collision on PR #377: if a project is
already onboarded through federation/gcp-target, or already self-hosts
CUDly via terraform/modules/compute/gcp/cloud-run, that call site already
created a project custom role with the same role_id
(cudlyCommitmentWriter). Applying gcp-sa-impersonation against the same
project then tries to create it again and 409s, since each bundle applies
from its own Terraform state with nothing to reconcile against.

Add var.create_custom_role (default true, preserving current behavior).
When false, the bundle binds var.service_account_email to the
already-existing role at "projects/${var.project_id}/roles/${var.custom_role_id}"
instead of creating it, so the same project can be onboarded through more
than one of the three call sites that define this role.
---
 .../gcp-sa-impersonation/terraform/main.tf    | 20 ++++++++++++++++++-
 .../terraform/variables.tf                    | 14 +++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/iac/federation/gcp-sa-impersonation/terraform/main.tf b/iac/federation/gcp-sa-impersonation/terraform/main.tf
index edb39e511..2386363e3 100644
--- a/iac/federation/gcp-sa-impersonation/terraform/main.tf
+++ b/iac/federation/gcp-sa-impersonation/terraform/main.tf
@@ -37,7 +37,17 @@ resource "google_service_account_iam_member" "cudly_impersonate" {
 # .list/.get). There is no billing-read path in this bundle (no
 # billing_account_id input to scope roles/billing.viewer to), so it is
 # dropped rather than reintroduced broken.
+#
+# var.create_custom_role gates creation: if the same project was already
+# onboarded through federation/gcp-target (or self-hosts CUDly via
+# terraform/modules/compute/gcp/cloud-run), that call site already created a
+# role with the same var.custom_role_id, and creating it again here 409s
+# (custom role IDs are unique per project, and each bundle applies from its
+# own Terraform state, so there's no shared state to reconcile against).
+# Default true preserves this bundle's own behavior for the common case
+# where it's the only onboarding path applied to the project.
 resource "google_project_iam_custom_role" "cudly" {
+  count   = var.create_custom_role ? 1 : 0
   project = var.project_id
   role_id = var.custom_role_id
   title   = "CUDly Commitment Writer"
@@ -49,9 +59,17 @@ resource "google_project_iam_custom_role" "cudly" {
   stage       = "GA"
 }
 
+locals {
+  # Reference the role this bundle created, or (when var.create_custom_role
+  # is false) the identically-shaped role another call site already created
+  # in this project. google_project_iam_custom_role.id renders to exactly
+  # this format.
+  custom_role_id = var.create_custom_role ? google_project_iam_custom_role.cudly[0].id : "projects/${var.project_id}/roles/${var.custom_role_id}"
+}
+
 resource "google_project_iam_member" "cudly_custom" {
   project = var.project_id
-  role    = google_project_iam_custom_role.cudly.id
+  role    = local.custom_role_id
   member  = "serviceAccount:${var.service_account_email}"
 }
 
diff --git a/iac/federation/gcp-sa-impersonation/terraform/variables.tf b/iac/federation/gcp-sa-impersonation/terraform/variables.tf
index 095657cd5..b01a2950d 100644
--- a/iac/federation/gcp-sa-impersonation/terraform/variables.tf
+++ b/iac/federation/gcp-sa-impersonation/terraform/variables.tf
@@ -13,6 +13,20 @@ variable "source_service_account" {
   type        = string
 }
 
+variable "create_custom_role" {
+  description = <<-EOT
+    Whether to create the var.custom_role_id custom role in this project.
+    Set to false if the project was already onboarded through
+    federation/gcp-target, or already self-hosts CUDly via
+    terraform/modules/compute/gcp/cloud-run, since either of those call
+    sites already creates a role with the same ID and re-creating it here
+    409s. Default true is correct when this bundle is the only onboarding
+    path applied to the project.
+  EOT
+  type        = bool
+  default     = true
+}
+
 variable "custom_role_id" {
   description = <<-EOT
     Project-scoped custom role ID Terraform creates and binds to

From fd1025052ffd1a36d11e2edad4519efc0e5caeba Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Mon, 28 Sep 2026 06:36:13 +0200
Subject: [PATCH 3/5] docs(iac/gcp): clarify create_custom_role only applies to
 an existing role

CodeRabbit review on PR #377: gcp-target only creates its custom role when
it also creates its own service account (var.service_account_email left
empty). The prior wording of create_custom_role's description implied
setting it to false was always safe whenever the project had used
gcp-target, which is not true when gcp-target reused a pre-provisioned SA.
Spell out the precondition (verify the role exists first) and name exactly
when each of the two other call sites actually creates it.
---
 .../gcp-sa-impersonation/terraform/variables.tf | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/iac/federation/gcp-sa-impersonation/terraform/variables.tf b/iac/federation/gcp-sa-impersonation/terraform/variables.tf
index b01a2950d..0dad0b0d0 100644
--- a/iac/federation/gcp-sa-impersonation/terraform/variables.tf
+++ b/iac/federation/gcp-sa-impersonation/terraform/variables.tf
@@ -16,12 +16,17 @@ variable "source_service_account" {
 variable "create_custom_role" {
   description = <<-EOT
     Whether to create the var.custom_role_id custom role in this project.
-    Set to false if the project was already onboarded through
-    federation/gcp-target, or already self-hosts CUDly via
-    terraform/modules/compute/gcp/cloud-run, since either of those call
-    sites already creates a role with the same ID and re-creating it here
-    409s. Default true is correct when this bundle is the only onboarding
-    path applied to the project.
+    Default true is correct when this bundle is the only onboarding path
+    applied to the project.
+
+    Set to false ONLY after confirming the role already exists (e.g. `gcloud
+    iam roles describe var.custom_role_id --project var.project_id`):
+    either terraform/modules/compute/gcp/cloud-run (self-hosted CUDly,
+    unconditional) or federation/gcp-target (only when it created its own
+    service account, i.e. its var.service_account_email was left empty) can
+    have already created a role with the same ID in this project. If
+    neither applies, the role does not exist and setting this to false
+    makes the binding below target a missing role, which fails the apply.
   EOT
   type        = bool
   default     = true

From c0243e61d1ac8ccec14257e17ffddece6bc17463 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Mon, 28 Sep 2026 10:55:26 +0200
Subject: [PATCH 4/5] fix(iac/gcp): grant recommender.viewer, fix the
 sa-impersonation CLI too

Independent review of PR #377 (head fd102505) found two gaps in the #129
fix:

1. Both GCP onboarding bundles (gcp-sa-impersonation and gcp-target)
   granted roles/compute.viewer but not roles/recommender.viewer.
   providers/gcp/services/computeengine.GetRecommendations calls the GCP
   Recommender API (google.billing.CostInsight.commitmentRecommender); a
   403 there is caught by providers/gcp/recommendations.go and only
   warn-logged, never returned as an error, so an account onboarded
   through either bundle would apply successfully but silently return
   zero GCP recommendations forever. Add roles/recommender.viewer to
   both:
   - gcp-sa-impersonation: new built_in_project_roles variable (mirrors
     gcp-target's service_account_project_roles), granted via a
     google_project_iam_member for_each instead of the single
     compute.viewer resource.
   - gcp-target: added to service_account_project_roles' default.
   iac/gcp_sa_impersonation_test.go gained
   TestGCPBundlesGrantEquivalentBuiltInRoles asserting the two lists stay
   equal and both contain recommender.viewer.

2. internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl (the CLI
   equivalent of the same bundle, served by handler_federation.go for
   format=cli downloads) still bound the exact two broken roles
   (commerceorgpolicy.commitmentAdmin, billing.viewer) this PR's
   Terraform fix removed. Ported the same fix: create the
   cudlyCommitmentWriter custom role via `gcloud iam roles create`, then
   bind it plus roles/compute.viewer and roles/recommender.viewer.
   internal/iacfiles/gcp_sa_impersonation_cli_test.go (new) runs the
   rendered script against a recording gcloud stub and asserts on the
   actual invocations, not just the template text.

Verification:
- terraform fmt -check -diff, terraform init -backend=false && validate,
  tflint: clean for both iac/federation/gcp-sa-impersonation/terraform
  and iac/federation/gcp-target/terraform.
- GOWORK=off GOTOOLCHAIN=go1.26.6 go test ./iac/... ./internal/iacfiles/...
  -v: 87/87 pass.
- go vet ./iac/... ./internal/iacfiles/...: clean.
- go test ./internal/api/... -run Federation -short: 83/83 pass.
- Confirmed both new regression tests fail on the pre-fix code (git stash
  of each file) and pass after.
---
 .../gcp-sa-impersonation/terraform/main.tf    | 18 ++++-
 .../terraform/variables.tf                    | 21 +++++
 .../gcp-target/terraform/variables.tf         |  8 ++
 iac/gcp_sa_impersonation_test.go              | 76 ++++++++++++-------
 .../iacfiles/gcp_sa_impersonation_cli_test.go | 75 ++++++++++++++++++
 .../gcp-sa-impersonation-cli.sh.tmpl          | 19 ++++-
 6 files changed, 185 insertions(+), 32 deletions(-)
 create mode 100644 internal/iacfiles/gcp_sa_impersonation_cli_test.go

diff --git a/iac/federation/gcp-sa-impersonation/terraform/main.tf b/iac/federation/gcp-sa-impersonation/terraform/main.tf
index 2386363e3..2af3276a1 100644
--- a/iac/federation/gcp-sa-impersonation/terraform/main.tf
+++ b/iac/federation/gcp-sa-impersonation/terraform/main.tf
@@ -73,8 +73,18 @@ resource "google_project_iam_member" "cudly_custom" {
   member  = "serviceAccount:${var.service_account_email}"
 }
 
-resource "google_project_iam_member" "cudly_compute_viewer" {
-  project = var.project_id
-  role    = "roles/compute.viewer"
-  member  = "serviceAccount:${var.service_account_email}"
+# Built-in read-side roles, mirroring federation/gcp-target's
+# service_account_project_roles: roles/compute.viewer (regions/zones/
+# machineTypes/commitments.list/.get) and roles/recommender.viewer. The
+# latter is required by the collection pipeline
+# (providers/gcp/services/computeengine.GetRecommendations calls the GCP
+# Recommender API, "google.billing.CostInsight.commitmentRecommender");
+# without it, every recommendation call 403s and is caught and warn-logged
+# by providers/gcp/recommendations.go, so an onboarded account would
+# silently surface zero GCP recommendations rather than erroring.
+resource "google_project_iam_member" "cudly_built_in" {
+  for_each = toset(var.built_in_project_roles)
+  project  = var.project_id
+  role     = each.value
+  member   = "serviceAccount:${var.service_account_email}"
 }
diff --git a/iac/federation/gcp-sa-impersonation/terraform/variables.tf b/iac/federation/gcp-sa-impersonation/terraform/variables.tf
index 0dad0b0d0..e781e953a 100644
--- a/iac/federation/gcp-sa-impersonation/terraform/variables.tf
+++ b/iac/federation/gcp-sa-impersonation/terraform/variables.tf
@@ -61,6 +61,27 @@ variable "custom_role_permissions" {
   ]
 }
 
+variable "built_in_project_roles" {
+  description = <<-EOT
+    Built-in project-scoped roles granted to var.service_account_email.
+    Defaults match federation/gcp-target's service_account_project_roles, so
+    both onboarding paths grant the same read-side access:
+
+    - roles/compute.viewer: regions/zones/machineTypes/commitments.list/.get.
+    - roles/recommender.viewer: required by the collection pipeline
+      (providers/gcp/services/computeengine.GetRecommendations calls the GCP
+      Recommender API). Missing it doesn't fail the apply or any API call
+      visibly -- providers/gcp/recommendations.go catches the resulting 403
+      and only warn-logs it, so the account onboards successfully but
+      silently returns zero GCP recommendations.
+  EOT
+  type        = list(string)
+  default = [
+    "roles/compute.viewer",
+    "roles/recommender.viewer",
+  ]
+}
+
 variable "cudly_api_url" {
   description = "CUDly API base URL for automatic account registration. Leave empty to skip registration."
   type        = string
diff --git a/iac/federation/gcp-target/terraform/variables.tf b/iac/federation/gcp-target/terraform/variables.tf
index 8dbd99fc8..017558769 100644
--- a/iac/federation/gcp-target/terraform/variables.tf
+++ b/iac/federation/gcp-target/terraform/variables.tf
@@ -195,6 +195,13 @@ variable "service_account_project_roles" {
     the two modules' definitions of cudlyCommitmentWriter identical
     so one doesn't stomp the other on apply.
 
+    roles/recommender.viewer is required by the collection pipeline
+    (providers/gcp/services/computeengine.GetRecommendations calls the GCP
+    Recommender API). Missing it doesn't fail the apply or any API call
+    visibly -- providers/gcp/recommendations.go catches the resulting 403
+    and only warn-logs it, so the account onboards successfully but
+    silently returns zero GCP recommendations (#129).
+
     Note: GCP's built-in commitment/billing roles (e.g.
     roles/commerceorgpolicy.commitmentAdmin, roles/billing.viewer) are
     organization- or billing-account-scoped and will 400 if granted at
@@ -204,6 +211,7 @@ variable "service_account_project_roles" {
   type        = list(string)
   default = [
     "roles/compute.viewer",
+    "roles/recommender.viewer",
   ]
 }
 
diff --git a/iac/gcp_sa_impersonation_test.go b/iac/gcp_sa_impersonation_test.go
index f0b8b0b3c..473253e4f 100644
--- a/iac/gcp_sa_impersonation_test.go
+++ b/iac/gcp_sa_impersonation_test.go
@@ -3,14 +3,18 @@ package iac
 import (
 	"regexp"
 	"slices"
-	"strings"
 	"testing"
 )
 
 // These tests assert that the two GCP onboarding bundles (gcp-sa-impersonation
 // and gcp-target) grant an equivalent permission set to the CUDly service
 // account, so they cannot drift back into granting different, organization-
-// or billing-account-scoped roles at project scope (#129).
+// or billing-account-scoped roles at project scope (#129), and that neither
+// silently drops a built-in role the other grants (also #129: missing
+// roles/recommender.viewer doesn't fail the apply or any visible API call --
+// providers/gcp/recommendations.go catches the resulting 403 and only
+// warn-logs it, so an onboarded account silently returns zero GCP
+// recommendations).
 
 const (
 	gcpSAImpersonationMainTF = "federation/gcp-sa-impersonation/terraform/main.tf"
@@ -27,44 +31,62 @@ func readModuleFile(t *testing.T, path string) string {
 	return string(raw)
 }
 
-// extractCustomRolePermissions pulls the quoted string literals out of the
-// `variable "custom_role_permissions" { ... default = [ ... ] }` block in a
-// federation module's variables.tf.
-func extractCustomRolePermissions(t *testing.T, tf, path string) []string {
+// extractVarDefaultList pulls the quoted string literals out of a
+// `variable "<name>" { ... default = [ ... ] ... }` block in a federation
+// module's variables.tf.
+func extractVarDefaultList(t *testing.T, tf, varName, path string) []string {
 	t.Helper()
-	varRe := regexp.MustCompile(`(?s)variable "custom_role_permissions" \{.*?default = \[(.*?)\]`)
+	varRe := regexp.MustCompile(`(?s)variable "` + regexp.QuoteMeta(varName) + `" \{.*?default = \[(.*?)\]`)
 	m := varRe.FindStringSubmatch(tf)
 	if m == nil {
-		t.Fatalf("%s: could not find variable \"custom_role_permissions\" default block", path)
+		t.Fatalf("%s: could not find variable %q default block", path, varName)
 	}
 	litRe := regexp.MustCompile(`"([^"]+)"`)
 	lits := litRe.FindAllStringSubmatch(m[1], -1)
-	perms := make([]string, 0, len(lits))
+	values := make([]string, 0, len(lits))
 	for _, l := range lits {
-		perms = append(perms, l[1])
+		values = append(values, l[1])
 	}
-	if len(perms) == 0 {
-		t.Fatalf("%s: custom_role_permissions default has no permissions", path)
+	if len(values) == 0 {
+		t.Fatalf("%s: variable %q default has no values", path, varName)
+	}
+	return values
+}
+
+func assertEquivalentLists(t *testing.T, a, b []string, aName, bName string) {
+	t.Helper()
+	sortedA := slices.Clone(a)
+	slices.Sort(sortedA)
+	sortedB := slices.Clone(b)
+	slices.Sort(sortedB)
+	if !slices.Equal(sortedA, sortedB) {
+		t.Errorf("%s %v != %s %v; the two onboarding paths must grant the same permission set", aName, sortedA, bName, sortedB)
 	}
-	return perms
 }
 
 // TestGCPBundlesGrantEquivalentCommitmentPermissions guards #129: the
 // sa-impersonation and gcp-target bundles must converge on the same
-// commitment-write permission set rather than one granting a broader,
-// project-scope-invalid role.
+// commitment-write custom-role permission set rather than one granting a
+// broader, project-scope-invalid role.
 func TestGCPBundlesGrantEquivalentCommitmentPermissions(t *testing.T) {
-	saImpersonationPerms := extractCustomRolePermissions(t, readModuleFile(t, gcpSAImpersonationVarsTF), gcpSAImpersonationVarsTF)
-	targetPerms := extractCustomRolePermissions(t, readModuleFile(t, gcpTargetVarsTF), gcpTargetVarsTF)
+	saPerms := extractVarDefaultList(t, readModuleFile(t, gcpSAImpersonationVarsTF), "custom_role_permissions", gcpSAImpersonationVarsTF)
+	targetPerms := extractVarDefaultList(t, readModuleFile(t, gcpTargetVarsTF), "custom_role_permissions", gcpTargetVarsTF)
+	assertEquivalentLists(t, saPerms, targetPerms, "gcp-sa-impersonation custom_role_permissions", "gcp-target custom_role_permissions")
+}
 
-	sortedSA := slices.Clone(saImpersonationPerms)
-	slices.Sort(sortedSA)
-	sortedTarget := slices.Clone(targetPerms)
-	slices.Sort(sortedTarget)
+// TestGCPBundlesGrantEquivalentBuiltInRoles guards the same drift for the
+// built-in project-scoped roles (roles/compute.viewer,
+// roles/recommender.viewer): gcp-sa-impersonation's built_in_project_roles
+// must match gcp-target's service_account_project_roles.
+func TestGCPBundlesGrantEquivalentBuiltInRoles(t *testing.T) {
+	saRoles := extractVarDefaultList(t, readModuleFile(t, gcpSAImpersonationVarsTF), "built_in_project_roles", gcpSAImpersonationVarsTF)
+	targetRoles := extractVarDefaultList(t, readModuleFile(t, gcpTargetVarsTF), "service_account_project_roles", gcpTargetVarsTF)
+	assertEquivalentLists(t, saRoles, targetRoles, "gcp-sa-impersonation built_in_project_roles", "gcp-target service_account_project_roles")
 
-	if !slices.Equal(sortedSA, sortedTarget) {
-		t.Errorf("gcp-sa-impersonation custom_role_permissions %v != gcp-target custom_role_permissions %v; the two onboarding paths must grant the same permission set",
-			sortedSA, sortedTarget)
+	for _, want := range []string{"roles/compute.viewer", "roles/recommender.viewer"} {
+		if !slices.Contains(saRoles, want) {
+			t.Errorf("gcp-sa-impersonation built_in_project_roles missing %s", want)
+		}
 	}
 }
 
@@ -85,10 +107,10 @@ func TestGCPSAImpersonationDoesNotGrantInvalidProjectScopeRoles(t *testing.T) {
 		t.Errorf("%s: grants roles/billing.viewer, which is billing-account-scoped and 400s at project scope (see terraform/modules/compute/gcp/cloud-run/main.tf for the correct google_billing_account_iam_member shape if a billing read path is ever added here)", gcpSAImpersonationMainTF)
 	}
 
-	if !strings.Contains(tf, `resource "google_project_iam_custom_role" "cudly"`) {
+	if !regexp.MustCompile(`resource "google_project_iam_custom_role" "cudly" \{`).MatchString(tf) {
 		t.Errorf("%s: expected a google_project_iam_custom_role granting the minimum commitment-write permissions", gcpSAImpersonationMainTF)
 	}
-	if !strings.Contains(tf, `role    = "roles/compute.viewer"`) {
-		t.Errorf("%s: expected roles/compute.viewer for read access (regions/zones/machineTypes/commitments.list/.get)", gcpSAImpersonationMainTF)
+	if !regexp.MustCompile(`resource "google_project_iam_member" "cudly_built_in" \{`).MatchString(tf) {
+		t.Errorf("%s: expected a google_project_iam_member granting var.built_in_project_roles for read access", gcpSAImpersonationMainTF)
 	}
 }
diff --git a/internal/iacfiles/gcp_sa_impersonation_cli_test.go b/internal/iacfiles/gcp_sa_impersonation_cli_test.go
new file mode 100644
index 000000000..d60d3d05f
--- /dev/null
+++ b/internal/iacfiles/gcp_sa_impersonation_cli_test.go
@@ -0,0 +1,75 @@
+package iacfiles
+
+import (
+	"strings"
+	"testing"
+)
+
+// Regression test for #129: gcp-sa-impersonation-cli.sh.tmpl (the CLI
+// equivalent of iac/federation/gcp-sa-impersonation/terraform) granted
+// roles/commerceorgpolicy.commitmentAdmin (organization-scoped) and
+// roles/billing.viewer (billing-account-scoped) at project scope, which
+// gcloud rejects. This runs the rendered script against a recording `gcloud`
+// stub and asserts the actual invocations it makes, not just the template
+// text, so a broken template (e.g. a role name reverted, or the custom-role
+// creation step dropped) is caught even if a `mustContain` check on the raw
+// text would still pass.
+
+// simpleGcloudStubScript returns a stand-in `gcloud` executable that logs
+// every invocation (newline-folded) to logPath and exits 0, never contacting
+// GCP. Unlike gcloudStubScript (templates_gcp_wif_test.go), this script
+// makes no pool/provider describe/create calls that need state modeling, so
+// a plain recorder is enough.
+func simpleGcloudStubScript(logPath string) string {
+	return "#!/usr/bin/env bash\n" +
+		"args=\"$*\"\n" +
+		"printf '%s\\n' \"${args//$'\\n'/ }\" >> '" + logPath + "'\n"
+}
+
+func TestGCPSAImpersonationCLI_GrantsLeastPrivilegeRoles(t *testing.T) {
+	data := baseData()
+	// Cleared so the auto-register block (curl to a real URL) is not
+	// rendered; this test is about the IAM role-granting calls only.
+	data.CUDlyAPIURL = ""
+	data.ProjectID = "target-project"
+	data.ServiceAccountEmail = "cudly@target-project.iam.gserviceaccount.com"
+	rendered := renderCLITemplate(t, "templates/gcp-sa-impersonation-cli.sh.tmpl", data)
+
+	exitCode, _, stderr, calls := runRenderedScript(t, "gcp-sa-impersonation-cli.sh", rendered,
+		func(logPath string) map[string]string {
+			return map[string]string{"gcloud": simpleGcloudStubScript(logPath)}
+		},
+		map[string]string{"SOURCE_SERVICE_ACCOUNT": "cudly-host@source-project.iam.gserviceaccount.com"},
+	)
+	if exitCode != 0 {
+		t.Fatalf("rendered script exited %d, stderr: %s\ncalls: %v", exitCode, stderr, calls)
+	}
+
+	joined := strings.Join(calls, "\n")
+
+	for _, forbidden := range []string{
+		"roles/commerceorgpolicy.commitmentAdmin",
+		"roles/billing.viewer",
+	} {
+		if strings.Contains(joined, forbidden) {
+			t.Errorf("rendered script still grants %s (org/billing-account-scoped; 400s at project scope, #129); calls:\n%s", forbidden, joined)
+		}
+	}
+
+	if !strings.Contains(joined, "iam roles create cudlyCommitmentWriter") {
+		t.Errorf("rendered script must create the cudlyCommitmentWriter custom role; calls:\n%s", joined)
+	}
+	if !strings.Contains(joined, "--permissions=compute.commitments.create,compute.commitments.update") {
+		t.Errorf("rendered script's custom role must hold exactly compute.commitments.create/update; calls:\n%s", joined)
+	}
+
+	for _, wantRole := range []string{
+		"--role=projects/target-project/roles/cudlyCommitmentWriter",
+		"--role=roles/compute.viewer",
+		"--role=roles/recommender.viewer",
+	} {
+		if !strings.Contains(joined, wantRole) {
+			t.Errorf("rendered script must bind %s to the service account; calls:\n%s", wantRole, joined)
+		}
+	}
+}
diff --git a/internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl b/internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl
index ece3e3c2f..988564663 100644
--- a/internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl
+++ b/internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl
@@ -24,8 +24,25 @@ gcloud iam service-accounts create "${SA_NAME}" \
   --display-name="CUDly target SA" \
   || echo "  (service account may already exist)"
 
+# roles/commerceorgpolicy.commitmentAdmin (organization-scoped) and
+# roles/billing.viewer (billing-account-scoped) both fail at project scope
+# (#129). Grant exactly what CUDly needs instead: a project custom role
+# holding only the two commitment-write permissions, plus the built-in
+# roles/compute.viewer and roles/recommender.viewer for read access. This
+# mirrors iac/federation/gcp-sa-impersonation/terraform/main.tf so the
+# Terraform and CLI onboarding paths grant the same permission set.
+CUSTOM_ROLE_ID="cudlyCommitmentWriter"
+echo "Creating custom role ${CUSTOM_ROLE_ID} (if missing)..."
+gcloud iam roles create "${CUSTOM_ROLE_ID}" \
+  --project="${PROJECT_ID}" \
+  --title="CUDly Commitment Writer" \
+  --description="Minimum permissions for CUDly to purchase and update committed use discounts." \
+  --permissions=compute.commitments.create,compute.commitments.update \
+  --stage=GA \
+  || echo "  (custom role may already exist)"
+
 echo "Granting required roles to ${SERVICE_ACCOUNT_EMAIL}..."
-for role in roles/commerceorgpolicy.commitmentAdmin roles/billing.viewer; do
+for role in "projects/${PROJECT_ID}/roles/${CUSTOM_ROLE_ID}" roles/compute.viewer roles/recommender.viewer; do
   gcloud projects add-iam-policy-binding "${PROJECT_ID}" \
     --member="serviceAccount:${SERVICE_ACCOUNT_EMAIL}" \
     --role="${role}" \

From 1376e559f20a2f735b71be4cd818abb0e9de4b49 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Mon, 28 Sep 2026 13:20:01 +0200
Subject: [PATCH 5/5] fix(iac/gcp): fail loud on sa-impersonation CLI role
 create/bind errors

Independent review of PR #377 (head c0243e61) found two gaps in
gcp-sa-impersonation-cli.sh.tmpl:

1. `gcloud iam roles create ... || echo "(may already exist)"` treated
   every failure as "already exists", including a role soft-deleted
   within the last 7 days, a permission-denied error, or an existing role
   with a different (e.g. pre-#129) permission set. The binding loop had
   the same swallow-and-continue shape. A real failure could reach
   "=== Done ===" with the commitment-write role never actually bound.

   Fixed: on a failed create, `gcloud iam roles describe` distinguishes
   a genuine error (propagates under `set -e`) from an existing role. A
   soft-deleted role is undeleted first; either way its permissions are
   converged with `gcloud iam roles update` rather than trusted as-is.
   The binding loop's `|| echo` is gone -- add-iam-policy-binding is
   already idempotent, so a failure there is real and must propagate.

2. The recording `gcloud` stub in gcp_sa_impersonation_cli_test.go always
   exited 0, so the create-fails/describe/undelete/update/binding-failure
   branches above were never exercised. Added
   roleCreateGcloudStubScript, mode-switched via
   STUB_ROLE_CREATE_MODE / STUB_FAIL_BINDING_ROLE, and three tests:
   - TestGCPSAImpersonationCLI_SoftDeletedRole_UndeletesConvergesAndBinds
   - TestGCPSAImpersonationCLI_RoleDescribeFails_ExitsNonZeroWithoutBinding
   - TestGCPSAImpersonationCLI_BindingFailure_ExitsNonZero

Verification:
- GOWORK=off GOTOOLCHAIN=go1.26.6 go test ./internal/iacfiles/... -v:
  60/60 pass (4/4 in this file).
- go vet ./internal/iacfiles/...: clean. gofmt -l: no output.
- shellcheck against the rendered (placeholder-substituted) script: clean.
- Confirmed all three new tests fail against the pre-fix template
  (git stash of the .tmpl file) and pass after.

On the recommender-ID nit: verified against the pinned
github.com/LeanerCloud/cloud-commitments-go/providers/gcp module
(providers/gcp/services/computeengine/client.go:394) -- the actual
string used is "google.billing.CostInsight.commitmentRecommender", not
"google.compute.commitment.UsageCommitmentRecommender". The existing
comment in main.tf is accurate to the pinned library and is unchanged.
---
 .../iacfiles/gcp_sa_impersonation_cli_test.go | 136 ++++++++++++++++++
 .../gcp-sa-impersonation-cli.sh.tmpl          |  43 +++++-
 2 files changed, 172 insertions(+), 7 deletions(-)

diff --git a/internal/iacfiles/gcp_sa_impersonation_cli_test.go b/internal/iacfiles/gcp_sa_impersonation_cli_test.go
index d60d3d05f..9095f62f9 100644
--- a/internal/iacfiles/gcp_sa_impersonation_cli_test.go
+++ b/internal/iacfiles/gcp_sa_impersonation_cli_test.go
@@ -73,3 +73,139 @@ func TestGCPSAImpersonationCLI_GrantsLeastPrivilegeRoles(t *testing.T) {
 		}
 	}
 }
+
+// roleCreateGcloudStubScript is a recording `gcloud` stub that models the
+// `iam roles create|describe|undelete|update` and
+// `projects add-iam-policy-binding` calls the CLI template makes, so the
+// three failure-mode tests below can exercise branches
+// TestGCPSAImpersonationCLI_GrantsLeastPrivilegeRoles's always-succeeds
+// stub never reaches:
+//
+//   - STUB_ROLE_CREATE_MODE=deleted: `iam roles create` fails, `describe`
+//     reports the role as soft-deleted -- the script must undelete it and
+//     still converge permissions and bind.
+//   - STUB_ROLE_CREATE_MODE=describe_fails: `iam roles create` fails and
+//     `describe` also fails (role genuinely doesn't exist / permission
+//     denied) -- the script must exit non-zero and never reach the
+//     binding loop.
+//   - STUB_FAIL_BINDING_ROLE=<role>: `projects add-iam-policy-binding`
+//     fails for that one role -- the script must exit non-zero rather
+//     than swallow it and print "=== Done ===".
+func roleCreateGcloudStubScript(logPath string) string {
+	return `#!/usr/bin/env bash
+args="$*"
+printf '%s\n' "${args//$'\n'/ }" >> '` + logPath + `'
+
+case "$1 $2 $3" in
+  "iam roles create")
+    case "${STUB_ROLE_CREATE_MODE:-}" in
+      deleted|describe_fails) exit 1 ;;
+      *) exit 0 ;;
+    esac
+    ;;
+  "iam roles describe")
+    case "${STUB_ROLE_CREATE_MODE:-}" in
+      deleted) echo "True"; exit 0 ;;
+      describe_fails) echo "gcloud: role not found" >&2; exit 1 ;;
+      *) echo "False"; exit 0 ;;
+    esac
+    ;;
+  "iam roles undelete") exit 0 ;;
+  "iam roles update") exit 0 ;;
+esac
+
+if [ "$1 $2" = "projects add-iam-policy-binding" ]; then
+  for a in "$@"; do
+    case "$a" in
+      --role=*)
+        if [ "${a#--role=}" = "${STUB_FAIL_BINDING_ROLE:-}" ]; then
+          exit 1
+        fi
+        ;;
+    esac
+  done
+  exit 0
+fi
+
+exit 0
+`
+}
+
+func renderGCPSAImpersonationScript(t *testing.T) string {
+	t.Helper()
+	data := baseData()
+	// Cleared so the auto-register block (curl to a real URL) is not
+	// rendered; these tests are about the IAM role-granting calls only.
+	data.CUDlyAPIURL = ""
+	data.ProjectID = "target-project"
+	data.ServiceAccountEmail = "cudly@target-project.iam.gserviceaccount.com"
+	return renderCLITemplate(t, "templates/gcp-sa-impersonation-cli.sh.tmpl", data)
+}
+
+func TestGCPSAImpersonationCLI_SoftDeletedRole_UndeletesConvergesAndBinds(t *testing.T) {
+	rendered := renderGCPSAImpersonationScript(t)
+	exitCode, _, stderr, calls := runRenderedScript(t, "gcp-sa-impersonation-cli.sh", rendered,
+		func(logPath string) map[string]string {
+			return map[string]string{"gcloud": roleCreateGcloudStubScript(logPath)}
+		},
+		map[string]string{
+			"SOURCE_SERVICE_ACCOUNT": "cudly-host@source-project.iam.gserviceaccount.com",
+			"STUB_ROLE_CREATE_MODE":  "deleted",
+		},
+	)
+	if exitCode != 0 {
+		t.Fatalf("rendered script exited %d, stderr: %s\ncalls: %v", exitCode, stderr, calls)
+	}
+	joined := strings.Join(calls, "\n")
+
+	if !strings.Contains(joined, "iam roles undelete cudlyCommitmentWriter") {
+		t.Errorf("a soft-deleted role must be undeleted before use; calls:\n%s", joined)
+	}
+	if !strings.Contains(joined, "iam roles update cudlyCommitmentWriter") {
+		t.Errorf("an existing role's permissions must be converged (gcloud iam roles update), not trusted as-is; calls:\n%s", joined)
+	}
+	for _, wantRole := range []string{
+		"--role=projects/target-project/roles/cudlyCommitmentWriter",
+		"--role=roles/compute.viewer",
+		"--role=roles/recommender.viewer",
+	} {
+		if !strings.Contains(joined, wantRole) {
+			t.Errorf("recovering from a soft-deleted role must still bind %s; calls:\n%s", wantRole, joined)
+		}
+	}
+}
+
+func TestGCPSAImpersonationCLI_RoleDescribeFails_ExitsNonZeroWithoutBinding(t *testing.T) {
+	rendered := renderGCPSAImpersonationScript(t)
+	exitCode, _, _, calls := runRenderedScript(t, "gcp-sa-impersonation-cli.sh", rendered,
+		func(logPath string) map[string]string {
+			return map[string]string{"gcloud": roleCreateGcloudStubScript(logPath)}
+		},
+		map[string]string{
+			"SOURCE_SERVICE_ACCOUNT": "cudly-host@source-project.iam.gserviceaccount.com",
+			"STUB_ROLE_CREATE_MODE":  "describe_fails",
+		},
+	)
+	if exitCode == 0 {
+		t.Fatalf("rendered script must exit non-zero when both role create and role describe fail; calls:\n%s", strings.Join(calls, "\n"))
+	}
+	if joined := strings.Join(calls, "\n"); strings.Contains(joined, "add-iam-policy-binding") {
+		t.Errorf("a role create+describe failure must not fall through to binding roles it never confirmed exist; calls:\n%s", joined)
+	}
+}
+
+func TestGCPSAImpersonationCLI_BindingFailure_ExitsNonZero(t *testing.T) {
+	rendered := renderGCPSAImpersonationScript(t)
+	exitCode, _, _, calls := runRenderedScript(t, "gcp-sa-impersonation-cli.sh", rendered,
+		func(logPath string) map[string]string {
+			return map[string]string{"gcloud": roleCreateGcloudStubScript(logPath)}
+		},
+		map[string]string{
+			"SOURCE_SERVICE_ACCOUNT": "cudly-host@source-project.iam.gserviceaccount.com",
+			"STUB_FAIL_BINDING_ROLE": "roles/recommender.viewer",
+		},
+	)
+	if exitCode == 0 {
+		t.Fatalf("rendered script must exit non-zero when a role binding fails, not print \"=== Done ===\" over a partially-granted service account; calls:\n%s", strings.Join(calls, "\n"))
+	}
+}
diff --git a/internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl b/internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl
index 988564663..f61988828 100644
--- a/internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl
+++ b/internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl
@@ -32,23 +32,52 @@ gcloud iam service-accounts create "${SA_NAME}" \
 # mirrors iac/federation/gcp-sa-impersonation/terraform/main.tf so the
 # Terraform and CLI onboarding paths grant the same permission set.
 CUSTOM_ROLE_ID="cudlyCommitmentWriter"
+CUSTOM_ROLE_TITLE="CUDly Commitment Writer"
+CUSTOM_ROLE_DESCRIPTION="Minimum permissions for CUDly to purchase and update committed use discounts."
+CUSTOM_ROLE_PERMISSIONS="compute.commitments.create,compute.commitments.update"
+
 echo "Creating custom role ${CUSTOM_ROLE_ID} (if missing)..."
-gcloud iam roles create "${CUSTOM_ROLE_ID}" \
+if ! gcloud iam roles create "${CUSTOM_ROLE_ID}" \
   --project="${PROJECT_ID}" \
-  --title="CUDly Commitment Writer" \
-  --description="Minimum permissions for CUDly to purchase and update committed use discounts." \
-  --permissions=compute.commitments.create,compute.commitments.update \
-  --stage=GA \
-  || echo "  (custom role may already exist)"
+  --title="${CUSTOM_ROLE_TITLE}" \
+  --description="${CUSTOM_ROLE_DESCRIPTION}" \
+  --permissions="${CUSTOM_ROLE_PERMISSIONS}" \
+  --stage=GA; then
+  # Create fails whether the role already exists, was soft-deleted, or the
+  # call hit a real error (permission denied, quota, ...). Distinguish them
+  # with `describe` instead of assuming "already exists": a real failure
+  # here must propagate (set -e), not be swallowed and leave the role
+  # unbound below.
+  echo "  role create failed; checking whether ${CUSTOM_ROLE_ID} already exists..."
+  IS_DELETED=$(gcloud iam roles describe "${CUSTOM_ROLE_ID}" --project="${PROJECT_ID}" --format='value(deleted)')
+  if [ "${IS_DELETED}" = "True" ]; then
+    echo "  role was soft-deleted; undeleting..."
+    gcloud iam roles undelete "${CUSTOM_ROLE_ID}" --project="${PROJECT_ID}"
+  fi
+  # The role may already exist with a different (e.g. stale, pre-#129)
+  # permission set; converge it unconditionally rather than trusting
+  # whatever is already there.
+  echo "  role exists; converging its permissions..."
+  gcloud iam roles update "${CUSTOM_ROLE_ID}" \
+    --project="${PROJECT_ID}" \
+    --title="${CUSTOM_ROLE_TITLE}" \
+    --description="${CUSTOM_ROLE_DESCRIPTION}" \
+    --permissions="${CUSTOM_ROLE_PERMISSIONS}" \
+    --stage=GA
+fi
 
 echo "Granting required roles to ${SERVICE_ACCOUNT_EMAIL}..."
+# add-iam-policy-binding is idempotent (re-adding an existing binding is a
+# no-op success), so a failure here is real and must propagate (set -e),
+# not be swallowed -- otherwise the script can print "=== Done ===" with a
+# required role never actually bound.
 for role in "projects/${PROJECT_ID}/roles/${CUSTOM_ROLE_ID}" roles/compute.viewer roles/recommender.viewer; do
   gcloud projects add-iam-policy-binding "${PROJECT_ID}" \
     --member="serviceAccount:${SERVICE_ACCOUNT_EMAIL}" \
     --role="${role}" \
     --condition=None \
     --quiet \
-    --output=none || echo "  (role ${role} may already be bound or unavailable)"
+    --output=none
 done
 
 echo "Granting token creator to source SA ${SOURCE_SERVICE_ACCOUNT}..."