MG-183: Add namespace restrictions for must-gather jobs#371
Draft
neha037 wants to merge 3 commits into
Draft
Conversation
This commit implements the namespace restrictions feature to prevent must-gather jobs from creating or modifying resources in protected system namespaces (openshift-*, kube-*, hypershift-*). Changes: - API: Add NamespaceRestrictions field to MustGatherSpec with policy-based configuration - API: Add EnforcedRestrictions and ObservedGeneration to MustGatherStatus - Controller: Implement namespace restriction computation logic with Default and Override policies - Controller: Update status with enforced restrictions during reconciliation - Tests: Add comprehensive unit tests for restriction pattern matching - CRD: Generate updated manifest with new API fields and validation The feature supports two policies: - Default: Merges user-provided patterns with platform defaults - Override: Uses only user-provided patterns (allows removing defaults) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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. |
|
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 (4)
💤 Files with no reviewable changes (1)
WalkthroughThis PR adds namespace restriction configuration to the MustGather API, computes enforced restrictions in the controller, publishes them in status, extends the CRD schema, and adds unit tests for restriction logic. ChangesMustGather namespace restriction flow
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 10❌ Failed checks (10 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: neha037 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 |
Contributor
Author
🔍 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #371 +/- ##
==========================================
+ Coverage 82.32% 82.86% +0.54%
==========================================
Files 8 9 +1
Lines 905 934 +29
==========================================
+ Hits 745 774 +29
Misses 148 148
Partials 12 12
🚀 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
Contributor
|
@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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements the namespace restrictions feature to prevent must-gather jobs from creating or modifying resources in protected system namespaces (openshift-, kube-, hypershift-*).
Changes
API Types
NamespaceRestrictionsfield toMustGatherSpecwith policy-based configurationRestrictionPolicyenum type with valuesDefaultandOverrideEnforcedRestrictionsfield toMustGatherStatusto show actual applied restrictionsObservedGenerationfield toMustGatherStatusto track reconciliationController Logic
Testing
Documentation
Design
The feature follows OpenShift API conventions:
RestrictionPolicy) instead of boolean for better extensibilityTesting
Unit Tests
go test ./controllers/mustgather/... -vAll tests pass including new tests for namespace restrictions logic.
Integration
The restriction computation integrates with the existing controller reconciliation loop and updates status appropriately.
Jira
Fixes: https://issues.redhat.com/browse/MG-183
Related
🤖 Generated with Claude Code
Summary by CodeRabbit
namespaceRestrictionsconfiguration withpolicy(Default/Override) and a list of restricted namespace/wildcard patterns.enforcedRestrictions) and the latest observed generation (observedGeneration).