CORS-4524: Enable GCP custom universe domain support for CCO - #1068
CORS-4524: Enable GCP custom universe domain support for CCO#1068patrickdillon wants to merge 6 commits into
Conversation
Sets the universe domain when initializing GCP clients in order to support running in environments with non-default universe domains such as Google Cloud Dedicated, GCP's sovereign cloud offering. CCO will always use JSON-based creds, either service account with key or WIF. We must utilize the WithAuthCredentialsJSON function because it will use self-signed JWTs rather than oauth token exchange which fails in GCD. The authentication falls back to WithCredentials, as ccoctl can authenticate via the metadata server (obtaining credentials from the service account attached to a VM).
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request references CORS-4524 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe change preserves credential JSON types, propagates GCP universe domains through clients, formats service-account emails for domain-scoped projects, updates related mocks and tests, and refreshes the local Go builder and dependencies. ChangesGCP credential and domain support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CredentialLoader
participant GCPClient
participant GCPServices
CredentialLoader->>GCPClient: parse credential type and create credentials
GCPClient->>GCPClient: derive universe domain and assemble auth options
GCPClient->>GCPServices: initialize services with auth options
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ 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: 1
🧹 Nitpick comments (1)
pkg/gcp/actuator/serviceaccount.go (1)
159-165: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for domain-scoped project IDs.
Add table-driven assertions for both
projectandeu0:openshift, including the full IAM resource name. The supplied test change only models the conventional format.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gcp/actuator/serviceaccount.go` around lines 159 - 165, Extend the tests covering the service-account email/resource-name construction to use table-driven cases for both conventional project ID “project” and domain-scoped ID “eu0:openshift”. Assert the complete expected IAM resource name for each case, including the reversed “openshift.eu0” email domain produced by the project-name formatting logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile.local`:
- Line 1: Pin the Docker builder image in the FROM declaration for the builder
stage by replacing the mutable golang:1.26 tag with its immutable registry
digest, while preserving the golang 1.26 image and the builder stage name.
---
Nitpick comments:
In `@pkg/gcp/actuator/serviceaccount.go`:
- Around line 159-165: Extend the tests covering the service-account
email/resource-name construction to use table-driven cases for both conventional
project ID “project” and domain-scoped ID “eu0:openshift”. Assert the complete
expected IAM resource name for each case, including the reversed “openshift.eu0”
email domain produced by the project-name formatting logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1068 +/- ##
==========================================
- Coverage 47.30% 47.21% -0.09%
==========================================
Files 97 97
Lines 12614 12650 +36
==========================================
+ Hits 5967 5973 +6
- Misses 5987 6017 +30
Partials 660 660
🚀 New features to boost your workflow:
|
|
Added a link in the description to openshift/enhancements#1977 |
60f65c8 to
b651e8d
Compare
openshift-ci
Bot
added
the
needs-rebase
|
ug screwed up the rebase |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
In some cases, such as Google Dedicated Cloud, service accounts take on a different format. When the project has the format eu0:PROJECT_ID, the service account has the format: serviceAccount:SERVICE_ACCOUNT_NAME@PROJECT_ID.eu0.iam.gserviceaccount.com This commit adds some simnple handling for this case.
b651e8d to
13a4d15
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/test ? |
|
/test e2e-gcp |
|
/test e2e-gcp e2e-gcp-manual-oidc Regression testing only |
|
/testwith openshift/installer/main/e2e-gcd-ovn-private-techpreview #1068 openshift/cluster-cloud-controller-manager-operator#493 openshift/installer#10687 |
Oops I meant to include cloud-provider-gcp, not the CCCMO PR. Will need to retry |
|
/testwith openshift/installer/main/e2e-gcd-ovn-private-techpreview #1068 openshift/cloud-provider-gcp#131 openshift/installer#10687 Maybe we have enough quota for two runs... |
|
/testwith openshift/installer/main/e2e-gcd-ovn-private-techpreview #1068 openshift/cloud-provider-gcp#131 openshift/installer#10687 |
|
/testwith openshift/installer/main/e2e-gcd-ovn-private-techpreview #1068 openshift/cloud-provider-gcp#131 openshift/installer#10706 |
|
ok, the most recent multi-pr looks like it is making it into the install process. This doesn't include all PRs, so install will not succeed (I will try that again soon), but verification for this PR would look like master nodes getting scheduled and credentialsrequests getting filled |
Updates from CredentialsFromJSON -> CredentialsFromJSONWithType to avoid using the deprecated function. This change is a no-op, only intended to avoid deprecation warnings. The new function takes a credential type parameter, which can be used to limit accepted credentials types, which is useful for validating credentials provided by external third parties. For OpenShift, cluster credentials are coming from first party users running clusters in their project, so we do not need to limit which credentials are accepted; therefore we just pass the type through from the credential to the function.
13a4d15 to
5def0d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile.local`:
- Line 1: Update the builder image reference in the Dockerfile’s FROM
instruction to pin golang:1.26 using the verified sha256 digest, preserving the
existing builder stage name.
In `@go.mod`:
- Line 58: Update the google.golang.org/grpc dependency from v1.82.0 to v1.82.1
or a later patched release, then regenerate go.sum so its checksums match the
upgraded module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Bump google api packages so we have most up-to-date authentication libraries.
go mod tidy && go mod vendor
Needed for new GCP packages. Makes local consistent with Dockerfile.
5def0d7 to
a6a3575
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Dockerfile.local (1)
1-1: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the non-Red Hat builder image by digest.
golang:1.26is mutable. Replace it with a verified digest-pinned reference while retaining the1.26tag.Proposed fix
-FROM golang:1.26 AS builder +FROM golang:1.26@sha256:<verified-digest> AS builder🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile.local` at line 1, Update the Dockerfile builder image reference in the FROM instruction to retain the golang:1.26 tag while appending a verified immutable image digest. Do not change the builder stage name or use an unverified digest.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Dockerfile.local`:
- Line 1: Update the Dockerfile builder image reference in the FROM instruction
to retain the golang:1.26 tag while appending a verified immutable image digest.
Do not change the builder stage name or use an unverified digest.
|
This functionality is exercised by this multi-pr /testwith run /verified by multi-pr test |
openshift-ci-robot
added
the
verified
|
@patrickdillon: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@patrickdillon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
4 participants
This PR adds plumbing to set universe domain on GCP clients as well as updating assumptions on service account name formatting. Doing this enables openshift operation on Google Cloud Dedicated, GCP's sovereign cloud offering.
Additionally, this PR updates the relevant packages to move off of deprecated functions that were being used for authentication.
For more details, check the relevant commit messages.
For testing, we can check GCP e2e for any regressions. For testing GCD, once all the operator PRs are open, which should happen very soon, we can use
/testwithto test in the environment.Due to the priority of this, we intend to backport this functionality to 4.22.
See openshift/enhancements#1977 for more details.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores