CORS-4544: WIF Support on GCD - #1073
Open
patrickdillon wants to merge 4 commits into
Open
Conversation
Bring in GCP universe domain fields.
go mod tidy && go mod vendor
Adds support for including a universe domain in ccoctl generated GCP WIF credentials, which enables authentication in Google Cloud Dedicated, GCP's sovereign cloud offerings. The universe domain is determined by the credentials provided to ccoctl.
Contributor
|
@patrickdillon: This pull request references CORS-4544 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. |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughGCP provisioning now retrieves and propagates the universe domain. Workload identity, service-account, and STS configurations use domain-specific endpoints. Custom domains add ChangesGCP universe domain support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant createAllCmd
participant GCPClient
participant createServiceAccounts
participant createWorkloadIdentityProvider
createAllCmd->>GCPClient: Get universe domain
createAllCmd->>createServiceAccounts: Pass universe domain
createServiceAccounts-->>createServiceAccounts: Render domain-specific credentials
createAllCmd->>createWorkloadIdentityProvider: Pass universe domain
createWorkloadIdentityProvider-->>createWorkloadIdentityProvider: Render storage issuer URL
sequenceDiagram
participant STSSynchronization
participant InfrastructureStatus
participant syncSTSSecret
participant CredentialTemplate
STSSynchronization->>InfrastructureStatus: Read universe domain
InfrastructureStatus-->>STSSynchronization: Return domain or default
STSSynchronization->>syncSTSSecret: Pass universe domain
syncSTSSecret->>CredentialTemplate: Render STS and IAM endpoints
CredentialTemplate-->>syncSTSSecret: Return credential JSON
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cmd/provisioning/gcp/create_service_accounts.go (1)
213-215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate the universe domain in generate-only mode.
The generate-only path invokes
create-cred-configwithoutuniverseDomain. That command defaults togoogleapis.com, so custom-universe installations receive invalid credentials. Generate the JSON with the same domain-aware template or use another supported domain-aware path. Add a custom-domain dry-run test.🤖 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/cmd/provisioning/gcp/create_service_accounts.go` around lines 213 - 215, Update the generate-only flow around generateCredentialsConfigScript and create-cred-config to pass the configured universe domain through the same domain-aware command template used by normal provisioning, rather than relying on the googleapis.com default. Ensure custom-universe installations generate valid credentials, and add a dry-run test covering a custom universe domain.
🧹 Nitpick comments (1)
pkg/cmd/provisioning/gcp/create_service_accounts_test.go (1)
207-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd observable custom-domain coverage. The changed tests only pass
googleapis.com, so they do not validate the new feature.
pkg/cmd/provisioning/gcp/create_service_accounts_test.go#L207-L207: decode the generated secret for a custom domain and assert domain-specific STS and IAM Credentials URLs plusuniverse_domain.pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go#L209-L209: assert the public-bucket discovery document uses a custom-domain issuer.pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go#L419-L419: assert the pool-JWK provider uses a custom-domain issuer and defaults correctly for an empty domain.🤖 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/cmd/provisioning/gcp/create_service_accounts_test.go` at line 207, Expand the custom-domain test coverage across all listed sites: in pkg/cmd/provisioning/gcp/create_service_accounts_test.go:207-207, decode the generated secret from createServiceAccounts and assert domain-specific STS and IAM Credentials URLs plus universe_domain; in pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go:209-209, assert the public-bucket discovery document uses a custom-domain issuer; and at :419-419, assert the pool-JWK provider uses a custom-domain issuer while an empty domain falls back to the default.
🤖 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 `@pkg/cmd/provisioning/gcp/create_service_accounts.go`:
- Around line 337-341: Update universeDomainArgs in
pkg/cmd/provisioning/gcp/create_service_accounts.go:337-341 to validate the
domain against an explicit allow-list, canonicalize it, and JSON-encode it
before interpolating credential content. Apply the same validation and
canonicalization in pkg/gcp/actuator/actuator.go:338-342 before rendering
credential secrets; both sites require direct changes, and preserve
googleapis.com as the default.
In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider.go`:
- Around line 104-106: Normalize an empty universeDomain to the existing
googleapis.com default before constructing issuerURL in
createWorkloadIdentityProvider, matching the service-account path. Add or update
a test covering an empty universe domain and verify the issuer URL uses
https://storage.googleapis.com/<bucket>.
In `@pkg/gcp/actuator/actuator.go`:
- Around line 349-353: The getUniverseDomain method must propagate the error
from utils.GetInfrastructure instead of returning googleapis.com, allowing
reconciliation to retry and preserve sovereign configuration. Update its
signature and callers to return or handle the error, while retaining the
googleapis.com fallback only when Infrastructure loads successfully without a
universe-domain value.
---
Outside diff comments:
In `@pkg/cmd/provisioning/gcp/create_service_accounts.go`:
- Around line 213-215: Update the generate-only flow around
generateCredentialsConfigScript and create-cred-config to pass the configured
universe domain through the same domain-aware command template used by normal
provisioning, rather than relying on the googleapis.com default. Ensure
custom-universe installations generate valid credentials, and add a dry-run test
covering a custom universe domain.
---
Nitpick comments:
In `@pkg/cmd/provisioning/gcp/create_service_accounts_test.go`:
- Line 207: Expand the custom-domain test coverage across all listed sites: in
pkg/cmd/provisioning/gcp/create_service_accounts_test.go:207-207, decode the
generated secret from createServiceAccounts and assert domain-specific STS and
IAM Credentials URLs plus universe_domain; in
pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go:209-209,
assert the public-bucket discovery document uses a custom-domain issuer; and at
:419-419, assert the pool-JWK provider uses a custom-domain issuer while an
empty domain falls back to the default.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 28cbb504-02f1-4eb2-ae39-05b79d82adb5
⛔ Files ignored due to path filters (37)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_25_kube-controller-manager_01_kubecontrollermanagers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_25_kube-scheduler_01_kubeschedulers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (7)
go.modpkg/cmd/provisioning/gcp/create_all.gopkg/cmd/provisioning/gcp/create_service_accounts.gopkg/cmd/provisioning/gcp/create_service_accounts_test.gopkg/cmd/provisioning/gcp/create_workload_identity_provider.gopkg/cmd/provisioning/gcp/create_workload_identity_provider_test.gopkg/gcp/actuator/actuator.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1073 +/- ##
==========================================
- Coverage 47.21% 47.16% -0.06%
==========================================
Files 97 97
Lines 12650 12675 +25
==========================================
+ Hits 5973 5978 +5
- Misses 6017 6036 +19
- Partials 660 661 +1
🚀 New features to boost your workflow:
|
Member
Member
|
/retest-required |
Member
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-gcp-ovn-upgrade |
Contributor
|
@tthvo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b937e100-91f8-11f1-8241-321e69913d89-0 |
The cloud-credential operator, when running in WIF mode, syncs or recreates the credentials secrets on each sync. This commit ensures that the secret contains the correct universe domain when running in a non-default universe domain.
patrickdillon
force-pushed
the
ccoctl-universe-domain-v2
branch
from
August 7, 2026 15:30
58c230c to
fa48238
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go (1)
123-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for a non-default universe domain.
The new test checks only the empty-domain fallback to
googleapis.com. The pool-JWK-file test also passes"googleapis.com". These cases do not verify that a configured GCD domain reaches the issuer URL.Add a test case with a non-default
universeDomainand asserthttps://storage.<domain>/<bucket>.As per the PR objective, this cohort adds configurable GCP universe-domain support.
Also applies to: 451-451
🤖 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/cmd/provisioning/gcp/create_workload_identity_provider_test.go` around lines 123 - 153, Add a table-driven test case alongside “Empty universe domain defaults to googleapis.com” that supplies a non-default universeDomain, such as an alternate GCD domain, and verifies the generated discovery document issuer uses https://storage.<domain>/<bucket>. Ensure the case exercises the existing setup and generateOnly flow while preserving the default-domain assertion.
🤖 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 `@pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go`:
- Line 241: In the successful-case branch of the test around
createWorkloadIdentityProvider, add require.NoError(t, err) before invoking
test.verify. Ensure every case where test.expectError is false validates the
returned error before verification.
---
Nitpick comments:
In `@pkg/cmd/provisioning/gcp/create_workload_identity_provider_test.go`:
- Around line 123-153: Add a table-driven test case alongside “Empty universe
domain defaults to googleapis.com” that supplies a non-default universeDomain,
such as an alternate GCD domain, and verifies the generated discovery document
issuer uses https://storage.<domain>/<bucket>. Ensure the case exercises the
existing setup and generateOnly flow while preserving the default-domain
assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 71b298fd-6584-449b-abbc-2efae4dde1b2
📒 Files selected for processing (3)
pkg/cmd/provisioning/gcp/create_workload_identity_provider.gopkg/cmd/provisioning/gcp/create_workload_identity_provider_test.gopkg/gcp/actuator/actuator.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cmd/provisioning/gcp/create_workload_identity_provider.go
- pkg/gcp/actuator/actuator.go
| } | ||
| testPublicKeyPath := filepath.Join(tempDirName, testPublicKeyFile) | ||
| err := createWorkloadIdentityProvider(context.TODO(), mockGCPClient, testInfraName, testRegionName, testProject, testName, testPublicKeyPath, tempDirName, keyStorageMethod, test.generateOnly) | ||
| err := createWorkloadIdentityProvider(context.TODO(), mockGCPClient, testInfraName, testRegionName, testProject, testName, testPublicKeyPath, tempDirName, keyStorageMethod, test.generateOnly, test.universeDomain) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the returned error for successful cases.
When test.expectError is false, the test calls test.verify without require.NoError(t, err). Add that assertion before verification.
As per path instructions: “Never ignore error returns” for Go code.
🤖 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/cmd/provisioning/gcp/create_workload_identity_provider_test.go` at line
241, In the successful-case branch of the test around
createWorkloadIdentityProvider, add require.NoError(t, err) before invoking
test.verify. Ensure every case where test.expectError is false validates the
returned error before verification.
Source: Path instructions
Member
There was a problem hiding this comment.
Yea, it would be nice to add require.NoError(t, err) before proceeding to verifying. Though, it's an existing test-only issue, and shouldn't block the PR from merging.
There was a problem hiding this comment.
@tthvo, acknowledged. The missing require.NoError(t, err) is a valid test improvement, but it is pre-existing and non-blocking for #1073.
Would you like me to create a GitHub follow-up issue for this test cleanup?
You are interacting with an AI system.
Contributor
|
@patrickdillon: The following test 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. |
Member
|
/test e2e-gcp |
Member
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-gcp-ovn-upgrade |
Contributor
|
@tthvo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a397aaf0-929e-11f1-8a41-5e6abeffbbf0-0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for using WIF on GCD, Google's Sovereign Cloud offering. The main concern is plumbing support for universe domain through the generated credentials.
Summary by CodeRabbit
New Features
Bug Fixes
googleapis.comwhen no custom domain is configured.