CORS-4516: GCP: Configure cloud provider to use mounted creds - #10733
CORS-4516: GCP: Configure cloud provider to use mounted creds#10733patrickdillon wants to merge 1 commit into
Conversation
By setting the token-url to nil, we cause cloud-provider-gcp to use the mounted credentials provisioned by the credentialsrequest.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/hold
📝 WalkthroughWalkthroughChangesGCP token URL propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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
🤖 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/asset/manifests/cloudproviderconfig.go`:
- Around line 208-231: Add tests around CloudProviderConfig.Generate that derive
tokenURL from mocked session credentials: verify the default “googleapis.com”
universe omits token-url, while a non-default universe domain renders token-url
as nil. Avoid passing tokenURL directly so the credential-derived selection path
is exercised.
🪄 Autofix (Beta)
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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 520c927b-3f6f-43d7-a4c3-1bcd01624544
📒 Files selected for processing (3)
pkg/asset/manifests/cloudproviderconfig.gopkg/asset/manifests/gcp/cloudproviderconfig.gopkg/asset/manifests/gcp/cloudproviderconfig_test.go
| // TODO(padillon): The universe domain comparison can be removed (always set token-url = nil) | ||
| // when we want to switch all installs to use the credentialsrequest. Or, when | ||
| // https://github.com/kubernetes/cloud-provider-gcp/pull/1261 merges, we can remove this | ||
| // entirely from the cloud config. | ||
| var tokenURL string | ||
| session, err := gcpic.GetSession(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get GCP session: %w", err) | ||
| } | ||
| ud, err := session.Credentials.GetUniverseDomain() | ||
| if err != nil { | ||
| return fmt.Errorf("could not get GCP universe domain: %w", err) | ||
| } | ||
| if ud != "" && ud != "googleapis.com" { | ||
| tokenURL = "nil" | ||
| } | ||
|
|
||
| gcpConfig, err := gcpmanifests.CloudProviderConfig( | ||
| clusterID.InfraID, | ||
| installConfig.Config.GCP.ProjectID, | ||
| subnet, | ||
| installConfig.Config.GCP.NetworkProjectID, | ||
| firewallManagement, | ||
| tokenURL, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add generation-path coverage for universe-domain selection.
The renderer test passes "nil" directly, so it does not exercise credential-derived selection. Add CloudProviderConfig.Generate coverage for googleapis.com omitting token-url and a non-default universe domain rendering token-url = nil.
🤖 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/asset/manifests/cloudproviderconfig.go` around lines 208 - 231, Add tests
around CloudProviderConfig.Generate that derive tokenURL from mocked session
credentials: verify the default “googleapis.com” universe omits token-url, while
a non-default universe domain renders token-url as nil. Avoid passing tokenURL
directly so the credential-derived selection path is exercised.
Source: Coding guidelines
|
/cc @rochacbruno |
|
/hold cancel Dependency has merged |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
@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. |
patrickdillon
changed the title
|
@patrickdillon: This pull request references CORS-4516 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
3 participants
By setting the token-url to nil, we cause cloud-provider-gcp to use the mounted credentials provisioned by the credentialsrequest.
/hold
Depends on openshift/cluster-cloud-controller-manager-operator#493
We might want to just do this for all GCP installs. We could then dump some of the roles from the service account or even make it optional (for users that don't need artifact registry).
Summary by CodeRabbit
New Features
Bug Fixes