OCPBUGS-44637: node-joiner: configure proxy CA certificate before image pulls#10532
OCPBUGS-44637: node-joiner: configure proxy CA certificate before image pulls#10532rwsu wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rwsu: This pull request references Jira Issue OCPBUGS-44637, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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/invalid-bug
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughNew logic runs during add-node command startup to read the cluster Proxy, fetch a referenced trusted CA ConfigMap from openshift-config, and—when a CA bundle is present—concatenate it with the system CA bundle into proxy-ca-bundle.crt and set SSL_CERT_FILE to that path. ChangesProxy CA Trust Setup
Sequence DiagramsequenceDiagram
actor AddNodesCommand as NewAddNodesCommand
participant Setup as setupProxyCACert
participant REST as REST Config Builder
participant ConfigClient as OpenShift Config Client
participant K8sClient as Kubernetes Client
participant ConfigMap as ConfigMap (openshift-config)
participant FS as Filesystem
AddNodesCommand->>Setup: setupProxyCACert(directory, kubeConfig)
Setup->>REST: build REST config (kubeconfig or in-cluster)
REST-->>Setup: REST config
Setup->>ConfigClient: create OpenShift config client
Setup->>K8sClient: create Kubernetes client
Setup->>ConfigClient: Get Proxies/cluster
alt Proxy exists and TrustedCA.name set
ConfigClient->>ConfigMap: Get ConfigMap in openshift-config
alt ConfigMap found and has ca-bundle.crt
ConfigMap-->>Setup: ca-bundle.crt
Setup->>FS: read system CA bundle (optional)
FS-->>Setup: system CA content (may be empty)
Setup->>FS: write concatenated proxy-ca-bundle.crt
FS-->>Setup: path to proxy-ca-bundle.crt
Setup->>Setup: set SSL_CERT_FILE to path
else ConfigMap missing or missing key
ConfigMap-->>Setup: NotFound or no ca-bundle.crt => no-op
end
else Proxy missing or TrustedCA not set
ConfigClient-->>Setup: no-op
end
Setup-->>AddNodesCommand: return (nil or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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.12.1)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/nodejoiner/addnodes.go (1)
139-145: 💤 Low valueConsider ensuring a newline separator between the system CA and proxy CA bundles.
If the system CA bundle file doesn't end with a newline, the concatenation will produce malformed PEM where the proxy CA's
-----BEGIN CERTIFICATE-----immediately follows the system CA's-----END CERTIFICATE-----on the same line. While RHEL/CoreOS bundles typically end with newlines, defensive handling would prevent subtle TLS failures.Proposed fix
// Read the system CA bundle so public registries remain trusted alongside the proxy CA. systemCerts, err := os.ReadFile(systemCACertBundle) if err != nil && !os.IsNotExist(err) { return fmt.Errorf("cannot read system CA bundle: %w", err) } - combined := append(systemCerts, []byte(proxyCACert)...) + combined := systemCerts + if len(combined) > 0 && combined[len(combined)-1] != '\n' { + combined = append(combined, '\n') + } + combined = append(combined, []byte(proxyCACert)...)🤖 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/nodejoiner/addnodes.go` around lines 139 - 145, When building the combined CA bundle after reading systemCerts and proxyCACert, ensure there is a newline separator so PEM blocks don't run together; modify the logic around systemCerts/combined (the os.ReadFile(systemCACertBundle) handling and the combined := append(...) step) to detect if systemCerts ends with a newline (or is empty) and if not append a single '\n' before concatenating proxyCACert, so the final combined PEM always has a newline boundary between the system CA bundle and proxy CA.
🤖 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.
Nitpick comments:
In `@pkg/nodejoiner/addnodes.go`:
- Around line 139-145: When building the combined CA bundle after reading
systemCerts and proxyCACert, ensure there is a newline separator so PEM blocks
don't run together; modify the logic around systemCerts/combined (the
os.ReadFile(systemCACertBundle) handling and the combined := append(...) step)
to detect if systemCerts ends with a newline (or is empty) and if not append a
single '\n' before concatenating proxyCACert, so the final combined PEM always
has a newline boundary between the system CA bundle and proxy CA.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1058115-4d0d-497a-a248-8f09a866f032
📒 Files selected for processing (2)
pkg/nodejoiner/addnodes.gopkg/nodejoiner/addnodes_test.go
…ge pulls When a cluster proxy is configured with a self-signed certificate, the node-joiner pod fails to pull images through the proxy because the proxy CA is not trusted by the pod's TLS stack. setupProxyCACert now runs before the asset graph. It reads proxy/cluster to check for a trusted CA bundle, fetches the named ConfigMap from openshift-config, concatenates the cert with the system CA bundle, and sets SSL_CERT_FILE so that all subsequent Go TLS connections and oc subprocess calls trust both the proxy CA and public registry CAs. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@rwsu: all tests passed! 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@rwsu: This pull request references Jira Issue OCPBUGS-44637, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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: GitHub didn't allow me to request PR reviews from the following users: zniu1011. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
2 participants
When a cluster proxy is configured with a self-signed certificate, the node-joiner pod fails to pull images through the proxy because the proxy CA is not trusted by the pod's TLS stack.
setupProxyCACert now runs before the asset graph. It reads proxy/cluster to check for a trusted CA bundle, fetches the named ConfigMap from openshift-config, concatenates the cert with the system CA bundle, and sets SSL_CERT_FILE so that all subsequent Go TLS connections and oc subprocess calls trust both the proxy CA and public registry CAs.
Assisted-by: Claude Sonnet 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features
Tests