Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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 updates the IAM roles for the Cloud Build service account and simplifies the documentation for creating triggers. Key changes include adding roles/iam.serviceAccountUser at the project level and removing roles/cloudbuild.workerpools.use. Feedback points out a high-security risk with project-wide service account impersonation, the potential loss of private pool support, and the removal of idempotency checks in the documentation. Additionally, a numbering error was introduced in the markdown file.
| "roles/run.developer", | ||
| "roles/serviceusage.serviceUsageConsumer", | ||
| "roles/storage.admin", | ||
| "roles/iam.serviceAccountUser", |
There was a problem hiding this comment.
Granting roles/iam.serviceAccountUser at the project level is a significant security risk. This allows the service account to act as any service account within the project, violating the principle of least privilege and potentially leading to privilege escalation. If the service account needs to impersonate other identities (e.g., for Cloud Run deployments), this role should be granted specifically on those target service accounts rather than at the project level.
| gcbSARoles := []string{ | ||
| "roles/artifactregistry.writer", | ||
| "roles/cloudbuild.builds.editor", | ||
| "roles/cloudbuild.workerpools.use", |
There was a problem hiding this comment.
Removing roles/cloudbuild.workerpools.use will prevent the service account from using Cloud Build private pools. If this capability is intended to be supported by the MCP server, this role should be retained.
| 1. **Check for Service Account**: Check if a dedicated service account for Cloud Build (e.g., `cloud-build-runner@<PROJECT_ID>.iam.gserviceaccount.com`) already exists. | ||
| 2. **Create Service Account (if needed)**: If no dedicated service account exists, create one using `gcloud iam service-accounts create`. | ||
| 3. **Required Roles (Ensured Automatically)**: The `create_build_trigger` MCP tool will **automatically ensure** the necessary roles (e.g., `roles/logging.logWriter`, `roles/artifactregistry.writer`, `roles/developerconnect.tokenAccessor`, `roles/run.developer`, `roles/storage.admin`, `roles/serviceusage.serviceUsageConsumer`, `roles/cloudbuild.builds.editor`, `roles/cloudbuild.workerpools.use`, and specific IAM delegation on the default Compute SA) are granted to the relevant service accounts. You **do not** need to grant these permissions manually. | ||
| 1. **Create Service Account**: Create one using `gcloud iam service-accounts create`. |
There was a problem hiding this comment.
The removal of the Check for Service Account step makes this instruction non-idempotent, which contradicts the Core Principle: Idempotency defined in section 2 of this document. The agent should be instructed to verify if the service account already exists before attempting to create it to avoid errors.
| 2. **Create Service Account (if needed)**: If no dedicated service account exists, create one using `gcloud iam service-accounts create`. | ||
| 3. **Required Roles (Ensured Automatically)**: The `create_build_trigger` MCP tool will **automatically ensure** the necessary roles (e.g., `roles/logging.logWriter`, `roles/artifactregistry.writer`, `roles/developerconnect.tokenAccessor`, `roles/run.developer`, `roles/storage.admin`, `roles/serviceusage.serviceUsageConsumer`, `roles/cloudbuild.builds.editor`, `roles/cloudbuild.workerpools.use`, and specific IAM delegation on the default Compute SA) are granted to the relevant service accounts. You **do not** need to grant these permissions manually. | ||
| 1. **Create Service Account**: Create one using `gcloud iam service-accounts create`. | ||
| 3. **Required Roles (Ensured Automatically)**: Using the `create_build_trigger` MCP tool will **automatically ensure** the necessary roles are granted to the given service account. You **do not** need to grant these permissions manually. |
There was a problem hiding this comment.
The list numbering is inconsistent, skipping from '1' to '3'. Please update the numbering to be sequential.
yeshwanth1993
force-pushed
the
cloud-build-fix
branch
from
872e49a to
105a0d7
Compare
1 participant
Fixes #<issue_number_goes_here>