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
- Read GCP_CNCC_CREDENTIALS_FILE env var (set by CPO) 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. - 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>
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. |
WalkthroughThis change adds GCP Workload Identity Federation support to the Cloud Network Config Controller. The manifest is updated to include token audience configuration and conditional Google credentials environment setup, while the implementation handles reading GCP credentials from environment variables and rendering them into deployments. 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
| gcpCredsFile := os.Getenv("GCP_CNCC_CREDENTIALS_FILE") | ||
| if gcpCredsFile != "" { | ||
| data.Data["GCPCredentialsPath"] = filepath.Join("/etc/secret/cloudprovider", gcpCredsFile) | ||
| } else { | ||
| data.Data["GCPCredentialsPath"] = "" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file and verify the code context
cat -n pkg/network/cloud_network.go | head -130 | tail -30Repository: openshift/cluster-network-operator
Length of output: 2070
🏁 Script executed:
#!/bin/bash
# Verify all producers/consumers of GCP_CNCC_CREDENTIALS_FILE in-repo
rg -n -C3 'GCP_CNCC_CREDENTIALS_FILE|GCPCredentialsPath'Repository: openshift/cluster-network-operator
Length of output: 4821
🏁 Script executed:
#!/bin/bash
# Check filepath.Join behavior to understand the vulnerability
# This tests if filepath.Join escapes directory with absolute/traversal paths
python3 << 'EOF'
import os
from pathlib import Path
# Test cases for filepath.Join behavior in Go
test_cases = [
("/etc/secret/cloudprovider", "normal.txt"),
("/etc/secret/cloudprovider", "/etc/passwd"),
("/etc/secret/cloudprovider", "../../etc/passwd"),
("/etc/secret/cloudprovider", "./normal.txt"),
]
print("Go filepath.Join() behavior simulation (path.Join in Go):")
for base, rel in test_cases:
# Note: Go's filepath.Join uses / separator and works similarly to os.path.join
result = os.path.join(base, rel)
print(f" filepath.Join('{base}', '{rel}') => '{result}'")
if result.startswith(base):
print(f" ✓ Contained within {base}")
else:
print(f" ✗ ESCAPED {base}")
EOFRepository: openshift/cluster-network-operator
Length of output: 718
🏁 Script executed:
#!/bin/bash
# Search for documentation, comments, or configuration of GCP_CNCC_CREDENTIALS_FILE
rg -n -C2 'GCP_CNCC_CREDENTIALS_FILE' --type-list | head -50
find . -name '*.md' -o -name '*.yaml' -o -name '*.yml' | xargs grep -l 'GCP_CNCC_CREDENTIALS_FILE' 2>/dev/nullRepository: openshift/cluster-network-operator
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Check if there are any validation wrappers around environment variable usage
rg -B5 -A5 'os.Getenv.*CREDENTIALS' pkg/network/ | head -100Repository: openshift/cluster-network-operator
Length of output: 1229
Constrain GCP_CNCC_CREDENTIALS_FILE to a filename before joining.
Using filepath.Join("/etc/secret/cloudprovider", gcpCredsFile) without validation allows absolute paths to escape the intended directory. For example, if gcpCredsFile is /etc/passwd, the result becomes /etc/passwd instead of a path under /etc/secret/cloudprovider. Add a basename validation to reject non-filename values.
Suggested fix
// GCP WIF credential path for HCP deployments.
gcpCredsFile := os.Getenv("GCP_CNCC_CREDENTIALS_FILE")
- if gcpCredsFile != "" {
+ if gcpCredsFile == "" {
+ data.Data["GCPCredentialsPath"] = ""
+ } else if filepath.Base(gcpCredsFile) != gcpCredsFile {
+ 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)
- } else {
- data.Data["GCPCredentialsPath"] = ""
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/network/cloud_network.go` around lines 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.
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@apahim: 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. |
| data.Data["AzureManagedCredsPath"] = filepath.Join(azureCertPath, os.Getenv("MANAGED_AZURE_HCP_CREDENTIALS_FILE_PATH")) | ||
| data.Data["AzureManagedSecretProviderClass"] = os.Getenv("ARO_HCP_SECRET_PROVIDER_CLASS") | ||
| // GCP WIF credential path for HCP deployments. | ||
| gcpCredsFile := os.Getenv("GCP_CNCC_CREDENTIALS_FILE") |
There was a problem hiding this comment.
In the commit message you say that GCP_CNCC_CREDENTIALS_FILE is set by CPO. What's CPO? You're setting this env var in this other PR, right? https://github.com/openshift/hypershift/pull/7824/changes
| data.Data["AzureManagedCertDirectory"] = "" | ||
| data.Data["AzureManagedCredsPath"] = "" | ||
| data.Data["AzureManagedSecretProviderClass"] = "" | ||
| data.Data["GCPCredentialsPath"] = "" |
There was a problem hiding this comment.
You can probably use a helper function to set up data.Data for your tests
| } | ||
| yamlStr := string(yaml) | ||
|
|
||
| hasGoogleAppCreds := strings.Contains(yamlStr, "GOOGLE_APPLICATION_CREDENTIALS") |
There was a problem hiding this comment.
Here and below, every time you call strings.Contains, you're just looking for a substring in the entire yaml. I think it's more appropriate to actually parse the object and look for exactly the parameters you're interested in and check their value. For example, existing unit tests use uns.NestedSlice and uns.NestedString to do that.
3 participants
Summary by CodeRabbit
New Features
Tests