OCPBUGS-83401: add RootCA cert to the sysContextBuilder certs#5861
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@andfasano: This pull request references Jira Issue OCPBUGS-83401, 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@andfasano: This pull request references Jira Issue OCPBUGS-83401, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (afasano@redhat.com), skipping review request. 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. |
WalkthroughCertificate-bundle construction now treats Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test ? |
![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 the current code and only fix it if needed.
Inline comments:
In `@pkg/imageutils/sys_context.go`:
- Around line 231-235: The Root CA append (checking
controllerConfig.Spec.RootCAData and writing into certBundle) is currently
nested inside the AdditionalTrustBundle conditional so RootCAData is skipped
when AdditionalTrustBundle is empty; move the RootCAData handling out of the
outer if that checks controllerConfig.Spec.AdditionalTrustBundle so that the
block that checks len(controllerConfig.Spec.RootCAData) and calls
certBundle.Write(...) and certBundle.WriteString("\n") runs independently
(always executed when RootCAData is present), ensuring
controllerConfig.Spec.RootCAData is always appended to certBundle even if
AdditionalTrustBundle is empty.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 679c7a90-9973-4ee7-89d3-55fc453c56eb
📒 Files selected for processing (1)
pkg/imageutils/sys_context.go
|
/test e2e-agent-compact-ipv4-iso-no-registry |
andfasano
changed the title
|
/testwith openshift/release/4.22/e2e-agent-iso-no-registry-conformance-techpreview |
|
/jira refresh |
|
@andfasano: This pull request references Jira Issue OCPBUGS-83401, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (afasano@redhat.com), skipping review request. 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. |
|
/testwith openshift/release/4.22/e2e-agent-iso-no-registry-conformance-techpreview |
andfasano
changed the title
|
/retest |
|
/test ? |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
/testwith openshift/release/4.22/nightly/e2e-agent-iso-no-registry-conformance-techpreview |
|
/testwith openshift/release/main/nightly/e2e-agent-iso-no-registry-conformance-techpreview |
|
/lgtm |
|
Scheduling tests matching the |
andfasano
force-pushed
the
iri-add-support-syscontextbuilder
branch
from
f1916db to
7aeb7b8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/imageutils/sys_context_test.go (1)
185-198: Strengthen this rootCA case to verify bundle contents, not just cert-dir creation.At Line 196-197, this case currently proves only that cert handling is enabled. It does not verify the PR’s core behavior:
RootCADatabeing written intoca-bundle.crt. Please add assertions thatca-bundle.crtexists and contains the configured root CA bytes.Proposed test hardening
import ( + "os" "path/filepath" "testing" @@ // Check certs expectations if tc.expectCerts { assert.NotEmpty(t, sysCtx.SysContext.DockerPerHostCertDirPath, "DockerPerHostCertDirPath should not be empty") assert.DirExists(t, sysCtx.SysContext.DockerPerHostCertDirPath, "Certs directory should exist") + + // When RootCAData is provided, ensure merged bundle is materialized and contains it. + if tc.controllerConfig != nil && len(tc.controllerConfig.Spec.RootCAData) > 0 { + bundlePath := filepath.Join(sysCtx.SysContext.DockerPerHostCertDirPath, "ca-bundle.crt") + assert.FileExists(t, bundlePath, "Merged CA bundle should exist") + bundleData, readErr := os.ReadFile(bundlePath) + require.NoError(t, readErr, "Reading merged CA bundle should not fail") + assert.Contains(t, string(bundleData), string(tc.controllerConfig.Spec.RootCAData), "Merged CA bundle should contain RootCAData") + } } else { assert.Empty(t, sysCtx.SysContext.DockerPerHostCertDirPath, "DockerPerHostCertDirPath should be empty") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/imageutils/sys_context_test.go` around lines 185 - 198, The test case for "WithControllerConfig only - just rootCA" currently only checks that a temp cert dir was created; update the test to also assert that the ca-bundle.crt file was created inside the generated cert directory and that its contents equal the configured ControllerConfig.Spec.RootCAData bytes; locate the test case in pkg/imageutils/sys_context_test.go (the table entry where controllerConfig is set to &mcfgv1.ControllerConfig{Spec: mcfgv1.ControllerConfigSpec{RootCAData: []byte(...)}}) and add assertions that the path "<certDir>/ca-bundle.crt" exists and that reading that file returns the exact RootCAData provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/imageutils/sys_context_test.go`:
- Around line 185-198: The test case for "WithControllerConfig only - just
rootCA" currently only checks that a temp cert dir was created; update the test
to also assert that the ca-bundle.crt file was created inside the generated cert
directory and that its contents equal the configured
ControllerConfig.Spec.RootCAData bytes; locate the test case in
pkg/imageutils/sys_context_test.go (the table entry where controllerConfig is
set to &mcfgv1.ControllerConfig{Spec: mcfgv1.ControllerConfigSpec{RootCAData:
[]byte(...)}}) and add assertions that the path "<certDir>/ca-bundle.crt" exists
and that reading that file returns the exact RootCAData provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cfcbb889-8763-4668-9e1e-1493c69b39fc
📒 Files selected for processing (2)
pkg/imageutils/sys_context.gopkg/imageutils/sys_context_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/imageutils/sys_context.go
|
/lgtm |
|
Scheduling tests matching the |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, bfournie, pablintino 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
|
/verified by @andfasano |
openshift-ci-robot
added
the
verified
|
@andfasano: This PR has been marked as verified by 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. |
|
/test e2e-aws-ovn-upgrade |
|
/test e2e-aws-ovn-upgrade @pablintino any chance to override this job? It looks permafailing for unrelated reasons |
|
@andfasano: 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. |
|
@andfasano: Jira Issue OCPBUGS-83401: All pull requests linked via external trackers have merged:
Jira Issue OCPBUGS-83401 has been moved to the MODIFIED state. 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. |
|
Fix included in release 5.0.0-0.nightly-2026-04-24-121336 |
5 participants
- What I did
Added the rootCA cert to the certs built by sysContextBuilder. This is required to support the
OSStreamsfeature during the bootstrap when theNoRegistryClusterInstallfeature is enabled (and thus the OCP release payload is stored internally in the IRI registries, all of them signed by using the rootCA cert)- How to verify it
Run the iso-no-registry CI job
- Description for the changelog
Summary by CodeRabbit