feat: add configurable field-level restrictions for container and pod overrides#1653
feat: add configurable field-level restrictions for container and pod overrides#1653tolusha wants to merge 4 commits into
Conversation
tolusha
requested review from
akurinnoy,
btjd,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tolusha 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 |
|
Warning Review limit reached
More reviews will be available in 3 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces a configurable ChangesConfigurable Override Field Restriction
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over Reconcile,ApplyContainerOverrides: Container override path
Reconcile->>GetKubeContainersFromDevfile: GetRestrictedContainerOverrideFields
GetKubeContainersFromDevfile->>ApplyContainerOverrides: restrictedFields
ApplyContainerOverrides->>restrictContainerOverride: override, restrictedFields
restrictContainerOverride-->>ApplyContainerOverrides: error or nil
end
rect rgba(60, 179, 113, 0.5)
Note over ProvisionStorage,GetVolumesFromOverrides: Pod override and storage path
ProvisionStorage->>rewriteContainerVolumeMounts: GetRestrictedPodOverrideFields
rewriteContainerVolumeMounts->>GetVolumesFromOverrides: workspace, restrictedFields
GetVolumesFromOverrides->>restrictPodOverride: &override.Spec, restrictedFields
restrictPodOverride-->>ProvisionStorage: error or nil
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~130 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
rohanKanojia
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return err | ||
| } | ||
| } | ||
| if err := rules.checkString("terminationMessagePath", &override.TerminationMessagePath); err != nil { |
There was a problem hiding this comment.
For container_restrictions.go and pod_restriction.go, I am a bit concerned because we are calling checkString(<field>) for so many container/pod fields.
Have you considered iterating over the fields in DeniedContainerOverrideFields and DeniedPodOverrideFields and checking if those fields exist in the container/pod instead?
Please correct me if I'm wrong but it seems like this PR is checking this the other way around (going over all possible container/pod fields, and checking DeniedContainerOverrideFields/DeniedPodOverrideFields)
There was a problem hiding this comment.
Have you considered iterating over the fields in DeniedContainerOverrideFields and DeniedPodOverrideFields and checking if those fields exist in the container/pod instead?
It is a way complicated
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… overrides Signed-off-by: Anatolii Bazko <abazko@redhat.com>
tolusha
force-pushed
the
overriderestrictions
branch
from
fd4d54f to
73a202d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… overrides Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1653 +/- ##
==========================================
+ Coverage 37.17% 40.52% +3.34%
==========================================
Files 168 171 +3
Lines 14761 15621 +860
==========================================
+ Hits 5488 6330 +842
+ Misses 8921 8906 -15
- Partials 352 385 +33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… overrides Signed-off-by: Anatolii Bazko <abazko@redhat.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… overrides Signed-off-by: Anatolii Bazko <abazko@redhat.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Code Review Summary
Overall Assessment: Solid implementation of a security-critical feature with excellent test coverage. The restriction system is well-architected with proper defense-in-depth.
Key Highlights:
✅ 2700+ lines of comprehensive test coverage
✅ Security-first Kubernetes defaults aligned with Pod Security Standards
✅ Clean FieldRestriction abstraction with typed check methods
✅ Platform awareness (K8s vs OpenShift defaults)
✅ Defense-in-depth with always-restricted + configurable fields
Primary Concern: Breaking behavioral change on Kubernetes requires release notes and migration guide before merge (see inline comment on defaults.go).
The inline comments below provide specific suggestions for improvements.
| RestrictedPodOverrideFields: []string{}, | ||
| } | ||
| defaultKubernetesOverrideConfig = &v1alpha1.OverrideConfig{ | ||
| RestrictedContainerOverrideFields: []string{ |
There was a problem hiding this comment.
Breaking change: Release notes and migration guide required
The new Kubernetes default restrictions will cause existing DevWorkspaces using security-sensitive overrides to fail after operator upgrade. Any workspace with securityContext.privileged=true, hostNetwork=true, or volumes.hostPath will fail to start.
Required before merge:
- Release notes documenting this breaking change
- Migration guide with two paths:
- Set
restrictedContainerOverrideFields: []andrestrictedPodOverrideFields: []in DevWorkspaceOperatorConfig to restore pre-upgrade behavior - Remove restricted overrides from DevWorkspaces before upgrade
- Set
- Consider logging a warning when a workspace fails due to new default restrictions
| return checkContainerSecurityContext(override.SecurityContext, remaining, restriction) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Can we add a log message in the cases where the root/remaining is not matched by the case statements?
| return checkContainerResourceClaims(resources.Claims, remaining, restriction) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Can we add a log message in the cases where the root/remaining is not matched by the case statements?
dkwon17
left a comment
dkwon17
left a comment
There was a problem hiding this comment.
Tested and LGTM, just a comment
2 participants
What does this PR do?
This PR adds a configurable restriction system for
container-overridesandpod-overridesDevWorkspace attributes. Cluster admins can define deny rules in DevWorkspaceOperatorConfig to block specific fields (e.g.hostNetwork, securityContext.privileged) or specific values (e.g. restartPolicy=Always) from being set via overrides. On Kubernetes, security-sensitive defaults are denied out of the box — such as privileged containers,
running as root, host networking, and hostPath volumes — to match the restrictions that OpenShift enforces natively via SCCs
What issues does this PR fix or reference?
N/A
Is it tested? How?
Create a DevWorkspace with
container-overrides, for instance:PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Release Notes
New Features
container-overridesandpod-overrides.DevWorkspaceOperatorConfig.spec.config.overrides, withrestrictedContainerOverrideFieldsandrestrictedPodOverrideFields.Documentation