Add claude interactive google service account creation during cluster creation #105
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
|
Hi @ngopalak-redhat. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
ngopalak-redhat
changed the title
|
/ok-t-test |
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ngopalak-redhat, stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
WalkthroughDocumentation update to the OpenShift cluster creation guide that adds a comprehensive GCP Service Account Setup workflow (Step 5.2a) with interactive user prompts, creation commands, and configuration steps for new or existing service accounts, including IAM permissions and credential handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
plugins/openshift/commands/create-cluster.md (1)
264-367: Add guidance on credential management and cleanup after cluster creation.The GCP Service Account Setup section does not address credential cleanup, key rotation, or lifecycle management best practices.
Consider adding a subsection (e.g., "Credential Cleanup and Security") after Step 5.2a that covers:
**Credential Cleanup & Security** After cluster creation is complete, consider: 1. **Rotate or delete the service account key** if it's no longer needed: ```bash gcloud iam service-accounts keys delete KEY_ID --iam-account="$SERVICE_ACCOUNT_EMAIL"
Secure the GOOGLE_APPLICATION_CREDENTIALS file:
- Never commit to version control
- Use
.gitignore: Add~/.gcp/to prevent accidental commits- Set file permissions to
0600(already done above)Review and remove service account roles if using a temporary account:
gcloud projects remove-iam-policy-binding "$PROJECT_ID" \ --member="serviceAccount:$SERVICE_ACCOUNT_EMAIL" \ --role="ROLE_NAME"Delete the service account if no longer needed:
gcloud iam service-accounts delete "$SERVICE_ACCOUNT_EMAIL"</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2157c1918b22a3408e063759e19a40d6d8287725 and 59008bc1beafb0b13bfc10377fbbd0f5f2a80153. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `plugins/openshift/commands/create-cluster.md` (3 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>plugins/openshift/commands/create-cluster.md</summary> 273-273: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 284-284: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>plugins/openshift/commands/create-cluster.md (1)</summary><blockquote> `58-58`: **LGTM on GCP prerequisites update and Step 5.2a reference.** The updated prerequisite language (line 58) and reference to Step 5.2a (line 245) correctly guide users to the new guided service account setup workflow. These integrations are clear and well-placed. Also applies to: 245-245 </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
|
||
| **Ask the user**: "Do you want to use an existing service account JSON file or create a new one?" | ||
|
|
||
| **Option 1: Use Existing Service Account** |
There was a problem hiding this comment.
Fix markdown linting violations: use headings instead of emphasis.
Lines 273 and 284 use bold emphasis (**text**) where markdown headings should be used, violating MD036.
Apply this diff to convert emphasis to proper markdown headings:
-**Ask the user**: "Do you want to use an existing service account JSON file or create a new one?"
+### Ask the user: "Do you want to use an existing service account JSON file or create a new one?"
-**Option 1: Use Existing Service Account**
+### Option 1: Use Existing Service AccountAlso apply the same fix at line 284:
-**Option 2: Create New Service Account**
+### Option 2: Create New Service AccountAlso applies to: 284-284
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
273-273: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In plugins/openshift/commands/create-cluster.md around lines 273 and 284, bold
emphasis is being used where markdown headings are required (MD036); replace the
bolded lines (e.g., "**Option 1: Use Existing Service Account**") with the
appropriate heading syntax (e.g., use "#" or "##" depending on document
structure) for each occurrence so they become proper headings, and ensure
heading level is consistent with surrounding sections.
| 4. **Create the service account**: | ||
| ```bash | ||
| echo "Creating service account: $SERVICE_ACCOUNT_NAME" | ||
| gcloud iam service-accounts create "$SERVICE_ACCOUNT_NAME" --display-name="$SERVICE_ACCOUNT_NAME" | ||
| ``` | ||
|
|
||
| 5. **Extract service account details**: | ||
| ```bash | ||
| # Get service account information | ||
| SERVICE_ACCOUNT_JSON="$(gcloud iam service-accounts list --format json | jq -r '.[] | select(.name | match("/\(env.SERVICE_ACCOUNT_NAME)@"))')" | ||
| SERVICE_ACCOUNT_EMAIL="$(jq -r .email <<< "$SERVICE_ACCOUNT_JSON")" | ||
| PROJECT_ID="$(jq -r .projectId <<< "$SERVICE_ACCOUNT_JSON")" | ||
|
|
||
| echo "Service Account Email: $SERVICE_ACCOUNT_EMAIL" | ||
| echo "Project ID: $PROJECT_ID" | ||
| ``` | ||
|
|
||
| 6. **Grant required permissions**: | ||
| ```bash | ||
| echo "Granting IAM roles to service account..." | ||
|
|
||
| while IFS= read -r ROLE_TO_ADD ; do | ||
| echo "Adding role: $ROLE_TO_ADD" | ||
| gcloud projects add-iam-policy-binding "$PROJECT_ID" \ | ||
| --condition="None" \ | ||
| --member="serviceAccount:$SERVICE_ACCOUNT_EMAIL" \ | ||
| --role="$ROLE_TO_ADD" | ||
| done << 'END_OF_ROLES' | ||
| roles/compute.admin | ||
| roles/iam.securityAdmin | ||
| roles/iam.serviceAccountAdmin | ||
| roles/iam.serviceAccountKeyAdmin | ||
| roles/iam.serviceAccountUser | ||
| roles/storage.admin | ||
| roles/dns.admin | ||
| roles/compute.loadBalancerAdmin | ||
| roles/iam.roleAdmin | ||
| END_OF_ROLES | ||
| echo "All roles granted successfully." | ||
| ``` |
There was a problem hiding this comment.
Add error handling and validation after service account creation and role grants.
The service account creation (line 310) and IAM role grants (lines 328–344) lack error checking. If these commands fail, subsequent operations will fail silently or with confusing errors.
Add error checks and validation:
4. **Create the service account**:
```bash
echo "Creating service account: $SERVICE_ACCOUNT_NAME"
+ if gcloud iam service-accounts create "$SERVICE_ACCOUNT_NAME" --display-name="$SERVICE_ACCOUNT_NAME"; then
+ echo "Service account created successfully."
+ else
+ echo "Error: Failed to create service account. Exiting."
+ exit 1
+ fi
- gcloud iam service-accounts create "$SERVICE_ACCOUNT_NAME" --display-name="$SERVICE_ACCOUNT_NAME"
```Also add validation after granting roles (after line 346):
echo "All roles granted successfully."
+
+ # Verify roles were assigned
+ ASSIGNED_ROLES="$(gcloud projects get-iam-policy "$PROJECT_ID" --flatten='bindings[].members' --format='value(bindings.role)' --filter="bindings.members:serviceAccount:$SERVICE_ACCOUNT_EMAIL")"
+ if [ -z "$ASSIGNED_ROLES" ]; then
+ echo "Warning: Could not verify role assignment. Please check manually."
+ fi🤖 Prompt for AI Agents
In plugins/openshift/commands/create-cluster.md around lines 307 to 347, the
service account creation and IAM role grant steps lack error handling and
validation; update the script so the gcloud iam service-accounts create call is
tested for success and exits with a clear error message on failure, verify
SERVICE_ACCOUNT_JSON, SERVICE_ACCOUNT_EMAIL and PROJECT_ID are non-empty after
extraction (exit with error if not), and wrap each gcloud projects
add-iam-policy-binding invocation to check its exit status (log failures, stop
and exit non-zero if any role grant fails, or accumulate failed roles and report
them before exiting); ensure all error messages are descriptive and return
non-zero status so downstream steps don't run on silent failure.
| SERVICE_ACCOUNT_JSON="$(gcloud iam service-accounts list --format json | jq -r '.[] | select(.name | match("/\(env.SERVICE_ACCOUNT_NAME)@"))')" | ||
| SERVICE_ACCOUNT_EMAIL="$(jq -r .email <<< "$SERVICE_ACCOUNT_JSON")" | ||
| PROJECT_ID="$(jq -r .projectId <<< "$SERVICE_ACCOUNT_JSON")" | ||
|
|
||
| echo "Service Account Email: $SERVICE_ACCOUNT_EMAIL" | ||
| echo "Project ID: $PROJECT_ID" | ||
| ``` |
There was a problem hiding this comment.
Fix jq filter syntax for extracting service account details.
The jq filter on line 316 uses incorrect syntax for environment variable interpolation: \(env.SERVICE_ACCOUNT_NAME) will not work as intended. The jq env object requires a different syntax, and this approach is fragile.
Simplify by extracting the account after storing the email address, or use a direct approach:
- # Get service account information
- SERVICE_ACCOUNT_JSON="$(gcloud iam service-accounts list --format json | jq -r '.[] | select(.name | match("/\(env.SERVICE_ACCOUNT_NAME)@"))')"
- SERVICE_ACCOUNT_EMAIL="$(jq -r .email <<< "$SERVICE_ACCOUNT_JSON")"
- PROJECT_ID="$(jq -r .projectId <<< "$SERVICE_ACCOUNT_JSON")"
+ # Get service account email directly using gcloud
+ SERVICE_ACCOUNT_EMAIL="$(gcloud iam service-accounts list --format='value(email)' --filter='displayName:*'"$SERVICE_ACCOUNT_NAME"'*')"
+ if [ -z "$SERVICE_ACCOUNT_EMAIL" ]; then
+ echo "Error: Failed to retrieve service account email. Exiting."
+ exit 1
+ fi
+
+ # Get project ID from gcloud config
+ PROJECT_ID="$(gcloud config get-value project)"
+ if [ -z "$PROJECT_ID" ]; then
+ echo "Error: Project ID not set in gcloud config. Exiting."
+ exit 1
+ fiCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugins/openshift/commands/create-cluster.md around lines 316–322, the jq
filter currently attempts to interpolate the environment variable using
incorrect syntax; replace that fragile regex approach with a robust selection by
comparing the .email field to the SERVICE_ACCOUNT_NAME environment variable (or
better, use gcloud's --filter to return the specific service account), then
extract .email and .projectId from that selected object; ensure you set
SERVICE_ACCOUNT_EMAIL and PROJECT_ID from that selected JSON result so
interpolation is correct and reliable.
| 7. **Create and download service account key**: | ||
| ```bash | ||
| KEY_FILE="${HOME}/.gcp/${SERVICE_ACCOUNT_NAME}-key.json" | ||
| mkdir -p "$(dirname "$KEY_FILE")" | ||
| echo "Creating service account key..." | ||
| gcloud iam service-accounts keys create "$KEY_FILE" \ | ||
| --iam-account="$SERVICE_ACCOUNT_EMAIL" | ||
| echo "Service account key saved to: $KEY_FILE" | ||
| ``` | ||
| 8. **Set environment variable**: | ||
| ```bash | ||
| export GOOGLE_APPLICATION_CREDENTIALS="$KEY_FILE" | ||
| echo "GOOGLE_APPLICATION_CREDENTIALS set to: $KEY_FILE" | ||
| ``` | ||
There was a problem hiding this comment.
Add error handling for key file creation and secure permissions.
The service account key creation (lines 355–356) and environment variable setup lack error checking. Additionally, the generated key file should have restrictive permissions.
Add error handling and secure file permissions:
7. **Create and download service account key**:
```bash
KEY_FILE="${HOME}/.gcp/${SERVICE_ACCOUNT_NAME}-key.json"
mkdir -p "$(dirname "$KEY_FILE")"
echo "Creating service account key..."
+ if gcloud iam service-accounts keys create "$KEY_FILE" \
+ --iam-account="$SERVICE_ACCOUNT_EMAIL"; then
+ # Secure the key file with restrictive permissions
+ chmod 600 "$KEY_FILE"
+ echo "Service account key saved to: $KEY_FILE (permissions: 0600)"
+ else
+ echo "Error: Failed to create service account key. Exiting."
+ exit 1
+ fi
- gcloud iam service-accounts keys create "$KEY_FILE" \
- --iam-account="$SERVICE_ACCOUNT_EMAIL"
-
- echo "Service account key saved to: $KEY_FILE"
```🤖 Prompt for AI Agents
In plugins/openshift/commands/create-cluster.md around lines 349 to 366, the
gcloud service-account key creation and subsequent steps lack error handling and
do not secure the key file; update the snippet so the gcloud key creation is
executed inside a conditional that checks for success, on success run chmod 600
on the generated KEY_FILE and echo the saved path and permissions, on failure
print an error and exit with non-zero status; keep the export
GOOGLE_APPLICATION_CREDENTIALS step but only set it after verifying the key was
created and permissions set.
2 participants
What this PR does / why we need it:
Adds the recommended service account and roles to be created in GCP for cluster creation.
As mentioned earlier in the document this process of cluster creation is the last resort. ClusterBot and other methods are preferred. It help teams needing special hardware and visibility into the nodes on the cluster.
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist: