MG-183: Add namespace restrictions for must-gather jobs - #371
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@neha037: This pull request references MG-183 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. |
|
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:
WalkthroughMustGather reconciliation now rejects resources in namespaces prefixed with ChangesNamespace validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MustGatherReconciler
participant isRestrictedNamespace
participant setValidationFailureStatus
participant JobProcessing
MustGatherReconciler->>isRestrictedNamespace: Check namespace prefixes
isRestrictedNamespace-->>MustGatherReconciler: Return restriction result
MustGatherReconciler->>setValidationFailureStatus: Record ValidationNamespace failure
MustGatherReconciler-->>JobProcessing: Return before Job processing
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Code Review ReportReviewer: OAPE Principal Engineer ✅ Logic VerificationJira Intent Met: YES
API Design Quality:
Edge Cases Handled:
📊 Review ModulesModule A: Golang Logic & Safety
Code Quality Highlights:
Module B: Bash ScriptsN/A - No bash scripts modified Module C: Operator Metadata (OLM)
Module D: Build Consistency
Module E: Context-Adaptive Review
💪 Strengths
📝 Recommendations for Future WorkThese are not blocking for this PR:
🎯 SummaryThis is a well-executed implementation of the namespace restrictions feature that:
No blocking issues found. ✅ Review performed by OAPE automated code review system |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
controllers/mustgather/namespace_restrictions_test.go (1)
27-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests validate Go-level prefix matching only, not the CRD's regex validation.
These tests confirm
computeEnforcedRestrictions/matchesPatternwork correctly at the Go level for patterns like"kube-*". However, since the CRD'sPatternregex (seeapi/v1alpha1/mustgather_types.goline 133) actually rejects these same patterns at admission time, none of the current tests would have caught that regression — the unit tests bypass API-server schema validation entirely. Consider adding a schema-level test (e.g., validating thePatternregex directly against the documented example strings) to guard against this class of bug going forward.🤖 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 `@controllers/mustgather/namespace_restrictions_test.go` around lines 27 - 152, Add a schema-level test for namespace restriction patterns, because TestComputeEnforcedRestrictions only checks Go helper behavior and misses CRD admission validation. Extend the mustgather restriction test coverage around computeEnforcedRestrictions/matchesPattern by validating the Pattern regex used in mustgather_types.go against the documented examples like "kube-*", "hypershift-*", and "openshift-*", so a future regex mismatch is caught before API-server admission. Focus the new test on the regex/schema contract rather than the in-memory merging logic.
🤖 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 `@api/v1alpha1/mustgather_types.go`:
- Around line 121-135: The RestrictedNamespaces validation currently rejects the
documented suffix wildcard patterns because the items regex in MustGatherSpec
does not allow a hyphen immediately before the trailing asterisk. Update the
validation on RestrictedNamespaces so suffix patterns like openshift-*, kube-*,
and hypershift-* are accepted while still preserving the existing namespace-name
constraints for non-wildcard values. Keep the change localized to the
MustGatherSpec field tags and ensure the revised pattern still enforces
suffix-only wildcards and the existing length limits.
- Around line 144-152: Remove the type-level enum validation from
RestrictionPolicy and keep the enum only on the Policy field in
mustgather_types.go. The issue is that RestrictionPolicy’s Kubebuilder
annotation is being merged into the CRD via allOf, which blocks the allowed
empty string value for Policy. Update the RestrictionPolicy declaration so it
only documents the type, and leave the field-level validation on Policy as the
source of truth for Default, Override, and "".
In `@controllers/mustgather/mustgather_controller.go`:
- Around line 153-156: The create-success path in mustgather_controller.go
computes enforcedRestrictions in the reconcile flow but never writes it to
instance.Status, so new MustGather objects return with empty status fields.
Update the MustGather reconcile logic around computeEnforcedRestrictions and the
ManageSuccess path to persist the computed enforcedRestrictions and set
Status.ObservedGeneration on instance before returning, keeping the status
consistent without waiting for a later reconcile; use the existing
updateStatus/handleJobCompletion pattern as reference points in
MustGatherReconciler.
In `@controllers/mustgather/namespace_restrictions.go`:
- Around line 109-138: The namespace restriction helpers in
matchesRestrictedPattern and matchesPattern only compute and store restricted
namespaces, but the reconcile flow still does not enforce them at runtime.
Update the controller logic that uses these helpers so protected namespaces are
actually blocked from write access during reconciliation or job generation; if
enforcement is not being wired yet, rename enforcedRestrictions to
computedRestrictions to reflect that it is only a derived status value.
---
Nitpick comments:
In `@controllers/mustgather/namespace_restrictions_test.go`:
- Around line 27-152: Add a schema-level test for namespace restriction
patterns, because TestComputeEnforcedRestrictions only checks Go helper behavior
and misses CRD admission validation. Extend the mustgather restriction test
coverage around computeEnforcedRestrictions/matchesPattern by validating the
Pattern regex used in mustgather_types.go against the documented examples like
"kube-*", "hypershift-*", and "openshift-*", so a future regex mismatch is
caught before API-server admission. Focus the new test on the regex/schema
contract rather than the in-memory merging logic.
🪄 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: 34a587b4-223e-4dff-be67-2de125c3c079
⛔ Files ignored due to path filters (1)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (5)
api/v1alpha1/mustgather_types.gocontrollers/mustgather/mustgather_controller.gocontrollers/mustgather/namespace_restrictions.gocontrollers/mustgather/namespace_restrictions_test.godeploy/crds/operator.openshift.io_mustgathers.yaml
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #371 +/- ##
==========================================
- Coverage 82.57% 78.44% -4.13%
==========================================
Files 9 10 +1
Lines 918 965 +47
==========================================
- Hits 758 757 -1
- Misses 148 197 +49
+ Partials 12 11 -1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/mustgather/mustgather_controller.go (1)
153-173: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh top-level status on validation failures
controllers/mustgather/mustgather_controller.go:153-173
setValidationFailureStatusupdates the condition’sObservedGeneration, but it leavesinstance.Status.EnforcedRestrictionsandinstance.Status.ObservedGenerationunchanged. Repeated validation failures will keep reporting the last successful values; update those fields before writing the failure status too.🤖 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 `@controllers/mustgather/mustgather_controller.go` around lines 153 - 173, The validation-failure path in mustgather_controller.go only updates the condition via setValidationFailureStatus, but it leaves the top-level status fields stale. In the reconcile block that rejects the operator service account usage, update instance.Status.EnforcedRestrictions with the freshly computed enforcedRestrictions and set instance.Status.ObservedGeneration to the current generation before calling setValidationFailureStatus. Keep the changes localized to the validation branch around the saName/operatorNamespace check so repeated failures report the latest status.
🤖 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.
Outside diff comments:
In `@controllers/mustgather/mustgather_controller.go`:
- Around line 153-173: The validation-failure path in mustgather_controller.go
only updates the condition via setValidationFailureStatus, but it leaves the
top-level status fields stale. In the reconcile block that rejects the operator
service account usage, update instance.Status.EnforcedRestrictions with the
freshly computed enforcedRestrictions and set instance.Status.ObservedGeneration
to the current generation before calling setValidationFailureStatus. Keep the
changes localized to the validation branch around the saName/operatorNamespace
check so repeated failures report the latest status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5c1d504-0f33-4a09-a871-b1b4fb2eb926
📒 Files selected for processing (4)
api/v1alpha1/mustgather_types.goapi/v1alpha1/mustgather_types_test.gocontrollers/mustgather/mustgather_controller.godeploy/crds/operator.openshift.io_mustgathers.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/crds/operator.openshift.io_mustgathers.yaml
- api/v1alpha1/mustgather_types.go
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controllers/mustgather/mustgather_controller_test.go (1)
817-903: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent assertion depth across new namespace test cases.
The
kube_rejected/hypershift_rejectedcases (Lines 817-847, 848-878) don't assertCompletedor Job absence like theopenshift_rejectedcase does, andreconcile_allowed_namespace_default_succeeds(Lines 879-903) doesn't assert a Job was actually created. Consider aligning all four cases to the same assertion depth for consistency.🤖 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 `@controllers/mustgather/mustgather_controller_test.go` around lines 817 - 903, The namespace reconciliation tests have inconsistent post-reconciliation verification. Update the postTestChecks for reconcile_restricted_namespace_kube_rejected and reconcile_restricted_namespace_hypershift_rejected to also verify the MustGather is Completed and no Job exists, matching the openshift_rejected case; update reconcile_allowed_namespace_default_succeeds to verify successful completion and that the expected Job was created.
🤖 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 `@controllers/mustgather/mustgather_controller_test.go`:
- Around line 817-903: The namespace reconciliation tests have inconsistent
post-reconciliation verification. Update the postTestChecks for
reconcile_restricted_namespace_kube_rejected and
reconcile_restricted_namespace_hypershift_rejected to also verify the MustGather
is Completed and no Job exists, matching the openshift_rejected case; update
reconcile_allowed_namespace_default_succeeds to verify successful completion and
that the expected Job was created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b779a15e-7042-4fe3-bc1f-70aa337e94be
📒 Files selected for processing (3)
controllers/mustgather/constant.gocontrollers/mustgather/mustgather_controller.gocontrollers/mustgather/mustgather_controller_test.go
|
Let's also revisit this through ValidatingAdmissionPolicy too, as it seems like we can restrict such ops at creation stage itself eg. Seems that ValidatingAdmissionPolicy in Kubernetes is stable as of k8s v1.30 and given we're targeting this feature for OpenShift 5.0+ should not possess a risk. |
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Up for review |
|
+1 on the ValidatingAdmissionPolicy approach /approve |
openshift-ci
Bot
added
the
approved
| ginkgo.BeforeAll(func() { | ||
| ginkgo.By("Creating ValidatingAdmissionPolicy for namespace restrictions") | ||
| failPolicy := admissionregistrationv1.Fail | ||
| vap := &admissionregistrationv1.ValidatingAdmissionPolicy{ |
There was a problem hiding this comment.
are we leaning towards a user-centric validation opt-in?
i.e. here there's an explicit need for a cluster admin to put such a validation policy on the cluster. Otherwise, by default the cluster would just let users create MustGather in the restricted namespaces anyway. Thoughts?
There was a problem hiding this comment.
let's also re-evaluate if it makes sense to put this directly on the bundle as an OLM manifest (if at all possible AND viable to enforce).
There was a problem hiding this comment.
Hey @swghosh , no, the validation was not a opt-in but the test were design this way as on a standard OLM install, the restriction won't be active by default as OLM does not support support ValidatingAdmissionPolicy or ValidatingAdmissionPolicyBinding as bundle manifest objects.
Currently looking into asset embedding ( baking the VAP yaml files into the operator binary ) to resolve this issue. Using openshift/cluster-kube-descheduler-operator for reference.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
# Conflicts: # controllers/mustgather/mustgather_controller.go
|
/retest |
|
@neha037: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neha037, swghosh 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 |
4 participants
Summary
Adds a ValidatingAdmissionPolicy (VAP) that blocks creation of
MustGathercustom resources in restricted infrastructure namespaces:openshift-*,kube-*, andhypershift-*.This uses Kubernetes-native admission control via ValidatingAdmissionPolicy with CEL expressions, rather than RBAC restrictions or a custom admission webhook. VAP was chosen because:
failurePolicy: Failensures requests are denied if the policy engine is unavailablematchConstraintstargets onlymustgathersresources onCREATE, leaving all other resources and operations unaffectedChanges
ValidatingAdmissionPolicy (
deploy/50_...yaml,deploy/51_...yaml)ValidatingAdmissionPolicywith 3 CEL validation expressions, one per restricted prefix (openshift-,kube-,hypershift-), using!object.metadata.namespace.startsWith(...)ValidatingAdmissionPolicyBindingwithvalidationActions: [Deny]failurePolicy: Fail(fail-closed)operator.openshift.io/v1alpha1mustgathersonCREATEonlyController cleanup (
controllers/mustgather/mustgather_controller.go)setValidationFailureStatusreturn pattern — removed redundant error unpackingExample rejection
Error messages per prefix:
openshift-*: "MustGather resources cannot be created in openshift-* namespaces."kube-*: "MustGather resources cannot be created in kube-* namespaces."hypershift-*: "MustGather resources cannot be created in hypershift-* namespaces."Testing
E2E Tests (252 lines added)
New
"Namespace Restriction Tests"Ginkgo context with:BeforeAllcreates a probe MustGather inopenshift-monitoring; if the VAP does not enforce (cluster lacks TechPreview gate), the entire context is skipped gracefullyForbiddenrejection inopenshift-monitoring,kube-system, andhypershift-e2e-testopenshift(no hyphen, sostartsWith('openshift-')does not match)AfterAllremoves the VAP, binding, and any probe CRs; hypershift test namespace is cleaned up viadeferDesign Document
MG-183 Design Document
Jira
Fixes: https://issues.redhat.com/browse/MG-183