OCPNODE-4518: Block runc on RHEL 10 via OSImageURL stream class inspection - #6238
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesBootstrap and render controllers now share image inspection dependencies, synchronize image-related informer caches, and select RHEL10 runc validation based on either OS image URLs or OS image streams. Constructors, command wiring, constants, tests, and bootstrap fixtures were updated accordingly. OS image inspection and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant RenderController
participant StreamClassInspector
Bootstrap->>StreamClassInspector: create stream-class inspector
Bootstrap->>RenderController: RunBootstrap with inspector
RenderController->>StreamClassInspector: inspect OSImageURL when needed
StreamClassInspector-->>RenderController: return stream class
RenderController-->>Bootstrap: return validation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
2338c29 to
dc5c6d7
Compare
![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/controller/bootstrap/bootstrap.go`:
- Around line 366-369: The base OS stream-class lookup in bootstrap is still
using the original pullSecret, which can miss the merged IRI credentials and
fail auth on no-registry/InternalReleaseImage installs. Update the call in Run
to pass the merged pull secret bytes used earlier after the IRI merge, and make
sure getBaseStreamClass (and any downstream inspection path it uses) receives
those newer credentials instead of the stale secret.
🪄 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: Enterprise
Run ID: f54f4a98-8a95-4960-a70f-30f1425ce5b8
📒 Files selected for processing (8)
cmd/machine-config-controller/start.gopkg/controller/bootstrap/bootstrap.gopkg/controller/common/constants.gopkg/controller/common/images.gopkg/controller/render/render_controller.gopkg/controller/render/render_controller_test.gopkg/osimagestream/inspector.gotest/e2e-bootstrap/bootstrap_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/controller/common/images.go
- pkg/controller/common/constants.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/machine-config-controller/start.go
- test/e2e-bootstrap/bootstrap_test.go
- pkg/osimagestream/inspector.go
- pkg/controller/render/render_controller_test.go
- pkg/controller/render/render_controller.go
dc5c6d7 to
f79c206
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
f79c206 to
1a01a98
Compare
bitoku
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bitoku: This pull request references OCPNODE-4518 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. |
a654f87 to
7b85ae2
Compare
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/controller/render/render_controller_test.go`:
- Around line 1481-1485: The fake lister helper newFakeMCLister currently
ignores the error from indexer.Add, which can silently skip malformed
MachineConfig fixtures and hide bad test state. Update the helper to handle the
Add failure explicitly by either returning an error from newFakeMCLister or
switching it to take testing.T and failing the test immediately, so cache-based
tests cannot proceed with incomplete fixtures.
🪄 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: Enterprise
Run ID: 670ef7f7-6325-4548-9912-9fd6bd766880
📒 Files selected for processing (7)
cmd/machine-config-controller/start.gopkg/controller/bootstrap/bootstrap.gopkg/controller/common/constants.gopkg/controller/render/render_controller.gopkg/controller/render/render_controller_test.gopkg/osimagestream/inspector.gotest/e2e-bootstrap/bootstrap_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/controller/common/constants.go
- cmd/machine-config-controller/start.go
- test/e2e-bootstrap/bootstrap_test.go
- pkg/osimagestream/inspector.go
- pkg/controller/bootstrap/bootstrap.go
- pkg/controller/render/render_controller.go
|
/pipeline required |
|
Scheduling tests matching the |
7b85ae2 to
e9b2c17
Compare
| // decide whether runc should be blocked on RHEL 10. | ||
| // TODO(OCP 5.3): remove this once runc is removed. | ||
| var baseStreamClass string | ||
| if osImageStream == nil && cconfig.Spec.BaseOSContainerImage != "" { |
There was a problem hiding this comment.
The FG is now GAed and this condition triggers only in Hypershift, that's a complete separate topic (they have their own implementation). Given that for standalone the osImageStream variable will be always populated, isn't it simpler to trust it exists?
There was a problem hiding this comment.
Ah, this is my mistake. Thank you!
I wanted to check the stream class only when the OSImageURL overrides OSImageStream.
634b255 to
c00ffd9
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
@pablintino Can you take another look? |
pablintino
left a comment
pablintino
left a comment
There was a problem hiding this comment.
This change is to me a bit concerning. Despite the idea/strategy, in general, is IMHO good, the implementation has some details that are worth to consider, and at least, ack it has downsides and/or shadows:
- This change introduces a new function to parse the streams of a dedicated function in the osimagestream package that creates a new parsing path. That's fine, but the function has zero tests. The API stile and the nature of the functions make them look like a kludge.
- This change makes the MCC now perform OS Image Stream related parsing (clearly showed by the need to add all of those new listers). We have thought of moving the OS Image Stream logic to the controller a few times (there's a stale Jira card) for other reasons and maybe this is another one, or the one we need to do it. It would significantly simplify this change.
- The caching mechanism is clumsy. I would prefer to avoid exposing internal details (like a variable that acts as a barrier to run a piece of logic like both constants do). We have been recently talkinga about cahing for OS Image Streams discovery if we end up integrating it with PIS. This change will benefit from something like that too without the need of using an annotation for such of an internal thing.
| // - The OSImageURL was overridden (the override may target a different | ||
| // stream than the pool's default, so the stream-based check was skipped | ||
| // in generateRenderedMachineConfig). | ||
| // TODO(OCP 5.3): Remove when runc is removed. |
There was a problem hiding this comment.
These TODOs are worth a Jira card to make sure we don't loose track of this removal.
|
Thank you @pablintino for the review.
I'll implement them for sure.
Do we have any idea how to do them? I'm happy to discuss them if that helps. Meanwhile I'll try implementing new codes based on your feed back, on top of the current commit. I hope it helps us understand the best way to implement this. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
openshift-ci
Bot
added
the
approved
|
Thank you! @asahay19 is a QE for this feature. She'll be working on the manual testing and e2e. |
I am working on testing this PR. Will update the results soon on this PR. Thanks ! |
openshift-ci
Bot
added
the
needs-rebase
Rename validateNoRuncOnRHEL10 to validateNoRuncOnRHEL10FromOSImageStream and validateNoRuncFromOSImageURL to validateNoRuncOnRHEL10FromOSImageURL to distinguish the two validation paths more clearly. Assisted-by: Claude Code <https://claude.com/claude-code>
Convert from a *Controller method to a standalone function that accepts an osimagestream.StreamClassInspector parameter, making it testable without a full controller instance. Assisted-by: Claude Code <https://claude.com/claude-code>
…achineConfig The stream-based runc check was embedded inside generateRenderedMachineConfig, while the URL-based check lived in the callers. Move the stream check to syncGeneratedMachineConfig and RunBootstrap so both runc validations are colocated at the same level. Assisted-by: Claude Code <https://claude.com/claude-code>
The helper added indirection without reducing duplication — each call site now carries the detection + error inline, and validateNoRuncOnRHEL10FromOSImageURL documents why it checks runc before inspecting the image (no network access needed). Assisted-by: Claude Code <https://claude.com/claude-code>
Only call validateNoRuncOnRHEL10FromOSImageURL when OSImageURL is overridden; skip both checks when neither override nor stream exists. Assisted-by: Claude Code <https://claude.com/claude-code>
Derive a context.Context from the controller's stopCh and thread it through to StreamClassInspector.InspectStreamClass so the 30-second image-inspection timeout is cancelled immediately on shutdown instead of blocking the sync loop. Assisted-by: Claude Code <https://claude.com/claude-code>
Storing a context in a struct is a Go antipattern — thread it as a parameter through the sync call chain instead. Assisted-by: Claude Code <https://claude.com/claude-code>
c8424d1 to
08f5b4e
Compare
openshift-ci
Bot
removed
lgtm
|
/verified by @asahay19 I have verified that via OSImageURL stream , gurad is getting triggered and upgrade is blocked when default container runtime is runc. Full test logs has been documented here: https://redhat.atlassian.net/browse/OCPNODE-4614 |
openshift-ci-robot
added
the
verified
|
@asahay19: 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asahay19, bitoku, 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 |
|
/retest-required |
|
/test bootstrap-unit |
|
@bitoku: 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. |
4 participants
Assisted-by: Claude Code https://claude.com/claude-code
- What I did
When OSImageStream is not available, detect RHEL 10 by inspecting the container image's io.openshift.os.streamclass label from the OSImageURL. This complements the OSImageStream-based check (commit 50a5088) by covering the OSImageURL path.
- How to verify it
manual test and e2e test
- Description for the changelog
Added a block mechanism when OSImageURL is RHEL 10 based and runc is used.
Summary by CodeRabbit
New Features
Bug Fixes