Conversation
![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 Cloud Build setup to grant the Cloud Build Service Agent impersonation permissions on the user-provided service account and removes the unused roles/cloudbuild.workerpools.use role. Feedback identifies a critical API incompatibility where project-level IAM methods are incorrectly applied to service account resources, and suggests removing the serviceAccount: prefix from the returned string to ensure compatibility with the Cloud Build API. Additionally, the service account validation logic is noted as being too restrictive for project IDs containing dots or colons.
| return "", fmt.Errorf("unable to add iam.serviceAccountUser role to Cloud Build P4SA %s on SA %s err: %w", gcbP4sa, r, err) | ||
| } | ||
|
|
||
| return resolvedSA, nil |
There was a problem hiding this comment.
The function currently returns resolvedSA, which includes the serviceAccount: prefix. However, the Cloud Build API's serviceAccount field for triggers expects either the service account email address or its full resource path, not the IAM member string format. Providing the prefix will likely cause the subsequent CreateBuildTrigger call to fail with an invalid argument error.
| return resolvedSA, nil | |
| return strings.TrimPrefix(resolvedSA, "serviceAccount:"), nil |
| {"valid sa", "serviceAccount:test-sa@project.iam.gserviceaccount.com", true}, | ||
| {"valid sa with dashes", "serviceAccount:my-sa-123@my-project-456.iam.gserviceaccount.com", true}, | ||
| {"missing prefix", "test-sa@project.iam.gserviceaccount.com", false}, | ||
| {"wrong prefix", "user:test-sa@project.iam.gserviceaccount.com", false}, | ||
| {"missing domain", "serviceAccount:test-sa@project", false}, | ||
| {"wrong domain", "serviceAccount:test-sa@project.com", false}, |
There was a problem hiding this comment.
The validation logic for service accounts (and these corresponding tests) appears to be too restrictive. Google Cloud project IDs can contain dots and colons (e.g., for domain-scoped projects like example.com:my-project). The current regex [a-z0-9-]+ used in the implementation will reject valid service accounts from such projects. Consider updating the regex to support these valid project ID formats.
2 participants
Fixes #<issue_number_goes_here>