-
Notifications
You must be signed in to change notification settings - Fork 265
OCPBUGS-60670: Add ValidatingAdmissionPolicy for EgressIP #2837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdds two ValidatingAdmissionPolicy and two ValidatingAdmissionPolicyBinding resources in a YAML file to restrict the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arghosh93 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 |
|
@arghosh93: This pull request references Jira Issue OCPBUGS-60670, 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 |
|
@arghosh93: This pull request references Jira Issue OCPBUGS-60670, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
bindata/network/ovn-kubernetes/common/egressip-admission-policy.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bindata/network/ovn-kubernetes/common/egressip-admission-policy.yaml
🔇 Additional comments (2)
bindata/network/ovn-kubernetes/common/egressip-admission-policy.yaml (2)
23-39: CREATE policy validation logic is correct.The expression
!has(object.metadata.annotations) || !("k8s.ovn.org/egressip-mark" in object.metadata.annotations)correctly denies creation only when the egressip-mark annotation is explicitly set in the request, while allowing all other CREATE operations. This aligns with the objective to prevent unauthorized creation with the managed annotation.
41-56: Bindings are correctly structured.Both ValidatingAdmissionPolicyBinding resources properly reference their policies and apply cluster-wide Deny enforcement without exceptions. This is appropriate for governing a system-managed annotation.
bindata/network/ovn-kubernetes/common/egressip-admission-policy.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
bindata/network/ovn-kubernetes/common/egressip-admission-policy.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bindata/network/ovn-kubernetes/common/egressip-admission-policy.yaml
🔇 Additional comments (1)
bindata/network/ovn-kubernetes/common/egressip-admission-policy.yaml (1)
8-10: Verify UPDATE policy doesn't allow modification of existing annotations.The
matchConditiononly triggers when the annotation is being added to an EgressIP (present in new object, absent in old). If a regular user attempts to modify an existing annotation value, the condition won't match, and the policy won't apply. Confirm whether this is intentional or if the policy should also prevent modification of pre-existing annotations by non-system users.
8c93e4e to
8808de0
Compare
|
/retest-required |
8808de0 to
10bcd65
Compare
10bcd65 to
c228ecb
Compare
This commit is to add couple of ValidatingAdmissionPolicy to take care of following conditions: - k8s.ovn.org/egressip-mark annotation should not be added while creating an EgressIP. - A regular user should not be able to add k8s.ovn.org/egressip-mark annotation. Only a system user is allowed to do so. Signed-off-by: Arnab Ghosh <arnabghosh89@gmail.com>
c228ecb to
deb1808
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
|
@arghosh93: 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. |
|
/retest-required |
This commit is to add couple of ValidatingAdmissionPolicy to take care of following conditions: