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 and store universe domain
GCPClient->>GCPServices: initialize services with auth options and universe domain
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)Dockerfile.localTraceback (most recent call last): go.modTraceback (most recent call last): 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.
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.
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
| @@ -1,4 +1,4 @@ | |||
| FROM golang:1.21 AS builder | |||
| FROM golang:1.26 AS builder | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
Pin the non-Red Hat builder image by digest.
golang:1.26 is mutable, so local builds can consume unreviewed image contents over time. Replace it with golang:1.26@sha256:<verified-digest>. This repeats the previously withdrawn finding, but the current path instruction explicitly requires digest pinning.
🧰 Tools
🪛 Trivy (0.72.0)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 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 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.
Source: Path instructions
| golang.org/x/time v0.15.0 | ||
| google.golang.org/api v0.288.0 | ||
| google.golang.org/genproto v0.0.0-20260319201613-d00831a3d3e7 | ||
| google.golang.org/grpc v1.82.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
osv-scanner scan source -r .Repository: openshift/cloud-credential-operator
Length of output: 2760
🌐 Web query:
GHSA-hrxh-6v49-42gf google.golang.org/grpc v1.82.0 OSV
💡 Result:
GHSA-hrxh-6v49-42gf is a high-severity security advisory affecting the google.golang.org/grpc library [1][2]. It covers multiple vulnerabilities identified in the gRPC-Go implementation, specifically within the xDS RBAC authorization engine and the HTTP/2 transport server [2]. The vulnerabilities addressed are: 1. xDS RBAC Authorization Bypass (Fail-Open): Occurs when translating xDS RBAC policies containing Metadata or RequestedServerName fields [2]. 2. HTTP/2 Rapid Reset Denial of Service (DoS) Bypass: Involves a bypass of existing HTTP/2 Rapid Reset mitigations during client-initiated stream resets, leading to high CPU consumption [2]. 3. xDS RBAC Engine Server Panic: A DoS vulnerability triggered when parsing crafted xDS RBAC policies that include NOT rules around unsupported fields [2]. All affected versions of google.golang.org/grpc prior to 1.82.1 are vulnerable [3][1]. The issue is resolved in version 1.82.1, and users are advised to upgrade to this version or later to mitigate these risks [2][4].
Citations:
- 1: GHSA-hrxh-6v49-42gf
- 2: GHSA-hrxh-6v49-42gf
- 3: GHSA-hrxh-6v49-42gf
- 4: https://www.kodemsecurity.com/cve-archive/ghsa-hrxh-6v49-42gf
Upgrade google.golang.org/grpc to 1.82.1 or later. v1.82.0 is affected by GHSA-hrxh-6v49-42gf; bump to a patched release and update go.sum.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 58-58: google.golang.org/grpc 1.82.0: gRPC-Go: xDS RBAC and HTTP/2 Vulnerabilities
🤖 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 `@go.mod` at 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.
Sources: Path instructions, Linters/SAST tools
|
@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