Fix(gcloud): Correct service account email format and improve creation #177
Conversation
Summary of ChangesHello @cjac, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the reliability and correctness of service account management within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes significant improvements to the service account creation and configuration process, particularly for domain-scoped projects. The changes to correctly format the service account email and replace gcloud ... describe with gcloud ... list are great for reliability. The introduction of a retry loop for IAM role binding is an excellent addition to handle propagation delays.
My review includes a few suggestions:
- There are a couple of critical issues where the script doesn't exit on failure as intended. The
exit_handleris called, but it doesn't terminate the script. This should be changed toexit 1. - I've also included some suggestions to improve shell script style and performance by using built-in parameter expansion and arithmetic commands.
gcloud/lib/shared-functions.sh
Outdated
| --description="Service account for use with cluster ${CLUSTER_NAME}" \ | ||
| --display-name="${SA_NAME}"; then | ||
| echo "ERROR: Failed to create service account ${SA_NAME}." | ||
| exit_handler |
There was a problem hiding this comment.
Calling exit_handler here does not terminate the script. Since trap exit_handler EXIT is set, you should call exit 1 to terminate the script on error. The trap will automatically invoke exit_handler.
| exit_handler | |
| exit 1 |
gcloud/lib/shared-functions.sh
Outdated
| sleep ${SLEEP_TIME} | ||
| else | ||
| echo "Failed to bind ${role} to ${GSA} after ${MAX_RETRIES} attempts." | ||
| exit_handler |
There was a problem hiding this comment.
Calling exit_handler here does not terminate the script. Since trap exit_handler EXIT is set, you should call exit 1 to terminate the script on error. The trap will automatically invoke exit_handler.
| exit_handler | |
| exit 1 |
| DOMAIN=$(echo "${PROJECT_ID}" | cut -d':' -f1) | ||
| PROJECT_NAME=$(echo "${PROJECT_ID}" | cut -d':' -f2) |
There was a problem hiding this comment.
For better performance and to avoid forking external processes, you can use shell parameter expansion instead of echo and cut to parse the PROJECT_ID.
| DOMAIN=$(echo "${PROJECT_ID}" | cut -d':' -f1) | |
| PROJECT_NAME=$(echo "${PROJECT_ID}" | cut -d':' -f2) | |
| DOMAIN="${PROJECT_ID%:*}" | |
| PROJECT_NAME="${PROJECT_ID#*:}" |
| gcloud projects add-iam-policy-binding "${PROJECT_ID}" \ | ||
| --member="serviceAccount:${GSA}" \ | ||
| --role=roles/compute.viewer | ||
| RETRY_COUNT=$((RETRY_COUNT + 1)) |
There was a problem hiding this comment.
You can use the more concise C-style arithmetic command ((RETRY_COUNT++)) to increment the counter.
| RETRY_COUNT=$((RETRY_COUNT + 1)) | |
| ((RETRY_COUNT++)) |
This commit addresses issues in the create-dpgce script related to
service account handling, particularly for domain-scoped projects.
- lib/env.sh:
- Correctly formats the service account email (GSA) for domain-scoped
projects (e.g., c9h.org:project-id) by including the domain
in the email address (e.g., sa-name@project-id.c9h.org.iam.gserviceaccount.com).
- lib/shared-functions.sh:
- Replaced `gcloud iam service-accounts describe` with `list --filter`
for a more reliable existence check, as describe was failing to find
existing service accounts.
- Added a retry loop for `gcloud projects add-iam-policy-binding`
to handle potential IAM propagation delays after service account
creation.
- Ensured the script exits if role bindings fail after multiple retries.
- Cleaned up role binding logic into a loop.
These changes ensure the script can reliably create and configure the
necessary service account and its IAM roles, unblocking cluster creation.
cjac
force-pushed
the
scoped-service-accounts-2025-09-30
branch
from
4b65f6d to
bf02e6a
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces valuable improvements to the service account creation process, particularly for domain-scoped projects and by adding robust retry logic for IAM role bindings. The code is cleaner and more resilient. However, I've identified a critical issue in the new error handling logic where the script would not exit upon failure, potentially leading to an inconsistent state. I've also included a suggestion to improve script performance and readability by using shell built-ins.
| --description="Service account for use with cluster ${CLUSTER_NAME}" \ | ||
| --display-name="${SA_NAME}"; then | ||
| echo "ERROR: Failed to create service account ${SA_NAME}." | ||
| exit 1 |
There was a problem hiding this comment.
Calling exit_handler directly here will only clean up temporary files but will not terminate the script, causing it to proceed even after the service account creation failed. The script should exit with a non-zero status code. The trap exit_handler EXIT at the top of the script will ensure exit_handler is called upon exit.
| exit 1 | |
| exit 1 |
| sleep ${SLEEP_TIME} | ||
| else | ||
| echo "Failed to bind ${role} to ${GSA} after ${MAX_RETRIES} attempts." | ||
| exit 1 |
There was a problem hiding this comment.
Similar to the service account creation failure, calling exit_handler directly here will not terminate the script. The script should exit with a non-zero status code to indicate failure after all retries for role binding have been exhausted. The configured trap will handle the cleanup.
| exit 1 | |
| exit 1 |
| DOMAIN=$(echo "${PROJECT_ID}" | cut -d':' -f1) | ||
| PROJECT_NAME=$(echo "${PROJECT_ID}" | cut -d':' -f2) |
There was a problem hiding this comment.
For better performance and to avoid forking external processes, you can use shell parameter expansion to parse the PROJECT_ID instead of echo and cut.
| DOMAIN=$(echo "${PROJECT_ID}" | cut -d':' -f1) | |
| PROJECT_NAME=$(echo "${PROJECT_ID}" | cut -d':' -f2) | |
| DOMAIN="${PROJECT_ID%%:*}" | |
| PROJECT_NAME="${PROJECT_ID#*:}" |
1 participant
This commit addresses issues in the create-dpgce script related to service account handling, particularly for domain-scoped projects.
lib/env.sh:
lib/shared-functions.sh:
gcloud iam service-accounts describewithlist --filterfor a more reliable existence check, as describe was failing to find existing service accounts.gcloud projects add-iam-policy-bindingto handle potential IAM propagation delays after service account creation.These changes ensure the script can reliably create and configure the necessary service account and its IAM roles, unblocking cluster creation.