GCP-430: Wire GCP WIF credentials for CNCC in HyperShift HCP mode#2915
GCP-430: Wire GCP WIF credentials for CNCC in HyperShift HCP mode#2915apahim wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@apahim: This pull request references GCP-430 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 "4.22.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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds conditional GCP credentials injection and sets the cloud-token minter's token audience to "openshift"; implements validation and rendering of a GCP credentials filename into HyperShift CNCC, and adds tests verifying these behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 🔧 golangci-lint (2.5.0)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v0.8.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.38.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17231 characters] ... ired in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20250922181213-ec3ebc5fd46b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.34.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.34.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
|
@apahim: This pull request references GCP-430 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 "4.22.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. |
|
@apahim: This pull request references GCP-430 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 "4.22.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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: apahim 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 |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
![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/network/cloud_network_test.go (1)
34-66: Extract shared render-data fixture to reduce duplication.Both tests build nearly identical
render.MakeRenderData()payloads. A helper fixture will keep future template-key updates consistent.Refactor sketch
+func makeManagedControllerRenderData() render.RenderData { + data := render.MakeRenderData() + data.Data["ReleaseVersion"] = "4.18.0" + data.Data["PlatformType"] = "GCP" + data.Data["PlatformRegion"] = "us-central1" + data.Data["PlatformTypeAWS"] = "AWS" + data.Data["PlatformTypeAzure"] = "Azure" + data.Data["PlatformTypeGCP"] = "GCP" + data.Data["CloudNetworkConfigControllerImage"] = "test-image" + data.Data["KubernetesServiceURL"] = "https://localhost:6443" + data.Data["ExternalControlPlane"] = true + data.Data["PlatformAzureEnvironment"] = "" + data.Data["PlatformAWSCAPath"] = "" + data.Data["PlatformAPIURL"] = "" + data.Data["CLIImage"] = "cli-image" + data.Data["TokenMinterImage"] = "token-minter-image" + data.Data["TokenAudience"] = "https://issuer.example.com" + data.Data["ManagementClusterName"] = "test-cluster" + data.Data["HostedClusterNamespace"] = "test-ns" + data.Data["ReleaseImage"] = "release-image" + data.Data["HCPNodeSelector"] = map[string]string{} + data.Data["HCPLabels"] = map[string]string{} + data.Data["HCPTolerations"] = []string{} + data.Data["RunAsUser"] = "" + data.Data["PriorityClass"] = "" + data.Data["HTTP_PROXY"] = "" + data.Data["HTTPS_PROXY"] = "" + data.Data["NO_PROXY"] = "" + data.Data["AzureManagedCertDirectory"] = "" + data.Data["AzureManagedCredsPath"] = "" + data.Data["AzureManagedSecretProviderClass"] = "" + return data +}Also applies to: 106-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/network/cloud_network_test.go` around lines 34 - 66, The test duplicated a large render.MakeRenderData() payload in cloud_network_test.go (the blocks starting at the shown diff and again at lines ~106-137); refactor by extracting a shared helper (e.g., newTestRenderData or makeRenderDataFixture) that returns render.MakeRenderData() with all required keys set (ReleaseVersion, PlatformType, PlatformRegion, PlatformTypeAWS/Azure/GCP, CloudNetworkConfigControllerImage, KubernetesServiceURL, ExternalControlPlane, PlatformAzureEnvironment, PlatformAWSCAPath, PlatformAPIURL, CLIImage, TokenMinterImage, TokenAudience, ManagementClusterName, HostedClusterNamespace, ReleaseImage, HCPNodeSelector, HCPLabels, HCPTolerations, RunAsUser, PriorityClass, HTTP_PROXY, HTTPS_PROXY, NO_PROXY, AzureManagedCertDirectory, AzureManagedCredsPath, AzureManagedSecretProviderClass, GCPCredentialsPath) and have both tests call that helper (passing tc.gcpCredentialsPath when needed) to remove duplication and keep future template-key updates in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/network/cloud_network.go`:
- Around line 113-117: Validate and constrain the GCP_CNCC_CREDENTIALS_FILE
value to a filename before joining: read it via os.Getenv into gcpCredsFile,
compute base := filepath.Base(gcpCredsFile) and only use
filepath.Join("/etc/secret/cloudprovider", base) when base equals the original
gcpCredsFile and base is non-empty (otherwise treat it as invalid and set
data.Data["GCPCredentialsPath"] = "" or handle error); update the assignment to
data.Data["GCPCredentialsPath"] accordingly so absolute paths or values with
directories cannot escape the intended directory.
---
Nitpick comments:
In `@pkg/network/cloud_network_test.go`:
- Around line 34-66: The test duplicated a large render.MakeRenderData() payload
in cloud_network_test.go (the blocks starting at the shown diff and again at
lines ~106-137); refactor by extracting a shared helper (e.g., newTestRenderData
or makeRenderDataFixture) that returns render.MakeRenderData() with all required
keys set (ReleaseVersion, PlatformType, PlatformRegion,
PlatformTypeAWS/Azure/GCP, CloudNetworkConfigControllerImage,
KubernetesServiceURL, ExternalControlPlane, PlatformAzureEnvironment,
PlatformAWSCAPath, PlatformAPIURL, CLIImage, TokenMinterImage, TokenAudience,
ManagementClusterName, HostedClusterNamespace, ReleaseImage, HCPNodeSelector,
HCPLabels, HCPTolerations, RunAsUser, PriorityClass, HTTP_PROXY, HTTPS_PROXY,
NO_PROXY, AzureManagedCertDirectory, AzureManagedCredsPath,
AzureManagedSecretProviderClass, GCPCredentialsPath) and have both tests call
that helper (passing tc.gcpCredentialsPath when needed) to remove duplication
and keep future template-key updates in one place.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
bindata/cloud-network-config-controller/managed/controller.yamlpkg/network/cloud_network.gopkg/network/cloud_network_test.go
openshift-ci
bot
added
the
do-not-merge/work-in-progress
- Read GCP_CNCC_CREDENTIALS_FILE env var (set by the Control Plane Operator via the HyperShift PR #7824) and set GOOGLE_APPLICATION_CREDENTIALS on the CNCC container so that CNCC's auto-detection (cloud-network-config-controller PR openshift#206) picks up the WIF external_account credential. - Validate GCP_CNCC_CREDENTIALS_FILE is a plain filename to prevent path traversal. - Add --token-audience=openshift to the cloud-token minter so GCP STS can validate the projected service-account token against the OIDC provider's allowed audience. - Add unit tests for template rendering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@apahim: This pull request references GCP-430 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 "4.22.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
bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest ci/prow/hypershift-e2e-aks |
|
/retest-required |
3 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest-required |
|
/retest-required |
|
@apahim: 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. |
| return nil, errors.Errorf("invalid GCP_CNCC_CREDENTIALS_FILE %q: must be a filename", gcpCredsFile) | ||
| } else { | ||
| data.Data["GCPCredentialsPath"] = filepath.Join("/etc/secret/cloudprovider", gcpCredsFile) | ||
| } |
There was a problem hiding this comment.
So... if the feature is configured, hypershift passes the literal string "application_default_credentials.json" to CNO, which then prepends the literal string "/etc/secret/cloudprovider/" and passes it to CNCC, which uses it as a filename.
This would make sense if hypershift knew the credentials filename but not the directory it was in, and CNO knew the directory but not the filename. But I'm guessing that's not the case.
Where do the creds come from? Who writes them there?
If HyperShift actually knows for sure what the credentials filename is (and nobody else does know for sure) then it should pass the whole path to CNO, which should pass it to CNCC.
If HyperShift doesn't know where the creds are, but only knows whether or not it wants CNCC to use them, then it should just pass a true/false value to CNO, and then likewise, CNO can either pass the full pathname or a true/false value to CNCC.
Also, it seems like it would be better to configure CNCC via a command-line argument...
There was a problem hiding this comment.
Hypershift creates the secret cloud-network-config-controller-creds containing the WIF credential JSON under the key application_default_credentials.json
Here specifically:
But that's becuase the secret name is hardcoded in CNO:
Then CNO mounts it in /etc/secret/cloudprovider:
Then we have to create the env var for CNO with the filename:
The filename-only approach follows the existing Azure pattern at line 110, where MANAGED_AZURE_HCP_CREDENTIALS_FILE_PATH similarly passes a filename that CNO combines with a mount path (/var/run/secrets/azure).
Each component owns its part of the path: HyperShift controls the secret key name, CNO controls the mount point.
The env var also doubles as a feature flag - empty means GCP WIF is not configured.
Passing the full path would couple HyperShift to CNO's mount point; passing a boolean would require hardcoding the filename in CNO, coupling it to HyperShift's secret key choice. The current split keeps both sides independent.
Regarding configuring CNCC: the env var set on the CNCC container is GOOGLE_APPLICATION_CREDENTIALS, which is a standard GCP SDK convention. The client libraries read it automatically to locate credentials... It's not an arbitrary env var, so a CLI argument would add an extra layer that just sets this env var anyway.
4 participants
Summary by CodeRabbit
New Features
Tests