Remove org-level role/binding cleanup, improve lib_iam output

infra/gcp/ensure-organization.sh

@@ -43,19 +43,7 @@ org_roles=(
     iam.serviceAccountLister
 )
 
-old_org_roles=(
-    StorageBucketLister
-)
-
-# TODO(https://github.com/kubernetes/k8s.io/issues/1659): obviated by organization.admin, remove when bindings gone
-old_org_admin_roles=(
-    roles/billing.user
-    roles/iam.organizationRoleAdmin
-    roles/resourcemanager.organizationAdmin
-    roles/resourcemanager.projectCreator
-    roles/resourcemanager.projectDeleter
-    roles/servicemanagement.quotaAdmin
-)
+old_org_roles=()
 
 color 6 "Ensuring organization custom roles exist"
 (
@@ -90,11 +78,7 @@ color 6 "Ensuring organization IAM bindings exist"
 
 color 6 "Ensuring removed organization IAM bindings do not exist"
 (
-    # TODO(spiffxp): remove this once the old bindings are confirmed gone
-    for role in "${old_org_admin_roles[@]}"; do
-        ensure_removed_org_role_binding "user:thockin@google.com" "${role}"
-        ensure_removed_org_role_binding "user:davanum@gmail.com" "${role}"
-    done
+    color 6 "No bindings to remove"
 ) 2>&1 | indent
 
 color 6 "Ensuring removed organization custom roles do not exist"

infra/gcp/lib_iam.sh

@@ -241,8 +241,7 @@ function _ensure_custom_iam_role_from_file() {
     <"${file}" sed -e "s|^name: ${name}|name: ${full_name}|" >"${ready}"
     gcloud iam roles "${verb}" ${scope_flag} "${name}" --file "${ready}" | yq -Y 'del(.etag)' > "${after}"
 
-    # if they differ, ignore the error
-    diff -u "${before}" "${after}" || true
+    diff_colorized "${before}" "${after}"
 }
 
 # Ensure that custom IAM role exists in scope and in sync with definition in file
@@ -272,7 +271,7 @@ function _ensure_removed_custom_iam_role() {
     local before="${tmp_dir}/iam-bind.before.txt"
 
     # gcloud iam roles delete errors if role doesn't exist, so confirm it does
-    if ! gcloud iam roles describe ${scope_flag} ${name} --format="value(deleted)" > "${before}"; then
+    if ! gcloud iam roles describe ${scope_flag} ${name} --format="value(deleted)" > "${before}" 2>/dev/null; then
         # not found, or can't see... no point in continuing
         return
     fi
@@ -284,6 +283,19 @@ function _ensure_removed_custom_iam_role() {
     gcloud iam roles delete ${scope_flag} "${name}"
 }
 
+# Format gcloud $resource get-iam-policy output to produce list of:
+# - member: user:foo@example.com
+#   role: roles/foo.bar
+# ... sorted by member, for ease of diffing
+# (couldn't find a gcloud --flatten --format incantation to do this)
+function _format_iam_policy() {
+  # shellcheck disable=SC2016
+  # $r is a jq variable, not a bash expression
+  yq -Y '.bindings 
+    | map(.role as $r | .members | map({member: ., role: $r})) 
+    | flatten | sort_by(.member)'
+}
+
 # Ensure that IAM binding is present for resource
 # Arguments:
 #   $1:  The resource type (e.g. "projects", "organizations", "secrets" )
@@ -304,18 +316,22 @@ function _ensure_resource_role_binding() {
     local before="${tmp_dir}/iam-bind.before.yaml"
     local after="${tmp_dir}/iam-bind.after.yaml"
 
-    # gcloud add-iam-policy-binding will not error on adding a duplicate binding
-    # TODO: that said, it is annoying to see lots of "updated iam policy for X" when nothing changed,
-    #       so consider avoiding the call
-    gcloud "${resource}" get-iam-policy "${id}" | yq -Y 'del(.etag)' > "${before}"
-    # add the binding
-    gcloud \
-        "${resource}" add-iam-policy-binding "${id}" \
-        --member "${principal}" \
-        --role "${role}" | \
-        yq -Y 'del(.etag)' > "${after}"
-    # if they differ, ignore the error
-    diff -u "${before}" "${after}" || true
+    gcloud "${resource}" get-iam-policy "${id}" | _format_iam_policy >"${before}"
+
+    # `gcloud add-iam-policy-binding` is idempotent, 
+    # but avoid calling if we can, to reduce output noise
+    if ! <"${before}" yq --exit-status \
+        ".[] | select(contains({role: \"${role}\", member: \"${principal}\"}))" \
+        >/dev/null; then
+
+        gcloud \
+            "${resource}" add-iam-policy-binding "${id}" \
+            --member "${principal}" \
+            --role "${role}" \
+        | _format_iam_policy >"${after}"
+
+        diff_colorized "${before}" "${after}"
+    fi
 }
 
 # Ensure that IAM binding has been removed from resource
@@ -335,14 +351,23 @@ function _ensure_removed_resource_role_binding() {
     local principal="${3}"
     local role="${4}"
 
-    # gcloud remove-iam-policy-binding errors if binding doesn't exist, so confirm it does
-    if gcloud "${resource}" get-iam-policy "${id}" \
-        --flatten="bindings[].members" \
-        --format='value(bindings.role)' \
-        --filter="bindings.members='${principal}' AND bindings.role='${role}'" | grep -q "${role}"; then
+    local before="${tmp_dir}/iam-bind.before.txt"
+    local after="${tmp_dir}/iam-bind.after.txt"
+
+    gcloud "${resource}" get-iam-policy "${id}" | _format_iam_policy >"${before}"
+
+    # `gcloud remove-iam-policy-binding` errors if binding doesn't exist,
+    #  so avoid calling if we can, to reduce output noise
+    if <"${before}" yq --exit-status \
+      ".[] | select(contains({role: \"${role}\", member: \"${principal}\"}))" \
+        >/dev/null; then
+
         gcloud \
             "${resource}" remove-iam-policy-binding "${id}" \
             --member "${principal}" \
-            --role "${role}"
+            --role "${role}" \
+        | _format_iam_policy >"${after}"
+
+        diff_colorized "${before}" "${after}"
     fi
 }

infra/gcp/lib_util.sh

@@ -107,3 +107,8 @@ function join_by() {
   # and $@ returns array which doesn't respect IFS
   echo "$*"
 }
+
+# use git-diff for color output, strip patch header
+function diff_colorized() {
+  git --no-pager diff --color --no-prefix --no-index "$@" | tail -n+5
+}

infra/gcp/roles/audit.viewer.yaml

@@ -545,15 +545,27 @@ includedPermissions:
   - deploymentmanager.typeProviders.list
   - deploymentmanager.types.list
   - dialogflow.agents.list
+  - dialogflow.answerrecords.list
+  - dialogflow.callMatchers.list
   - dialogflow.contexts.list
+  - dialogflow.conversationDatasets.list
+  - dialogflow.conversationModels.list
+  - dialogflow.conversationProfiles.list
+  - dialogflow.conversations.list
   - dialogflow.documents.list
   - dialogflow.entityTypes.list
   - dialogflow.environments.list
   - dialogflow.flows.list
   - dialogflow.intents.list
   - dialogflow.knowledgeBases.list
+  - dialogflow.messages.list
+  - dialogflow.modelEvaluations.list
   - dialogflow.pages.list
+  - dialogflow.participants.list
+  - dialogflow.phoneNumberOrders.list
+  - dialogflow.phoneNumbers.list
   - dialogflow.sessionEntityTypes.list
+  - dialogflow.smartMessagingEntries.list
   - dialogflow.transitionRouteGroups.list
   - dialogflow.versions.list
   - dialogflow.webhooks.list