diff --git a/infra/gcp/ensure-organization.sh b/infra/gcp/ensure-organization.sh index f928fed572e..f43e7809565 100755 --- a/infra/gcp/ensure-organization.sh +++ b/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" diff --git a/infra/gcp/lib_iam.sh b/infra/gcp/lib_iam.sh index 083c97b954a..d6d6c9a0d08 100755 --- a/infra/gcp/lib_iam.sh +++ b/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 } diff --git a/infra/gcp/lib_util.sh b/infra/gcp/lib_util.sh index 41ba211dac8..5418b4f5e1b 100755 --- a/infra/gcp/lib_util.sh +++ b/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 +} diff --git a/infra/gcp/roles/audit.viewer.yaml b/infra/gcp/roles/audit.viewer.yaml index 5a381c9a4ff..11dade1ae85 100644 --- a/infra/gcp/roles/audit.viewer.yaml +++ b/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