OSAC-1156: Add ImplementationStrategy field to SecurityGroup spec#278
Conversation
|
@ori-amizur: This pull request references OSAC-1156 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 task 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. |
|
Warning Review limit reached
More reviews will be available in 54 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (6)
WalkthroughSecurityGroup now reads ChangesSecurityGroup Implementation Strategy Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Risk Assessment & Security Notes
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
ori-amizur
force-pushed
the
OSAC-1156
branch
2 times, most recently
from
e174501 to
48e9720
Compare
ori-amizur
changed the title
SecurityGroup now reads its implementation strategy from spec.implementationStrategy (set by the fulfillment-service), falling back to "network_policy" when unset. This follows the PublicIPPool pattern and keeps the policy decision in the fulfillment-service rather than hardcoding it in the operator. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/securitygroup_controller.go (1)
92-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop obsolete
virtualnetworksRBAC permission from SecurityGroup controller.
handleUpdatenow resolves strategy fromSecurityGroup.spec, so this controller no longer needsvirtualnetworksread access. Keeping it broadens permissions unnecessarily.Suggested change
-// +kubebuilder:rbac:groups=osac.openshift.io,resources=virtualnetworks,verbs=get;list;watch🤖 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 `@internal/controller/securitygroup_controller.go` at line 92, Remove the obsolete RBAC rule granting read access to virtualnetworks from the SecurityGroup controller by deleting the kubebuilder comment line "// +kubebuilder:rbac:groups=osac.openshift.io,resources=virtualnetworks,verbs=get;list;watch" in internal/controller/securitygroup_controller.go; this controller now resolves strategy from SecurityGroup.spec in handleUpdate, so ensure only necessary RBAC annotations remain and run codegen/manifest regeneration if applicable.
🤖 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/securitygroup_types.go`:
- Around line 79-83: spec.implementationStrategy is free-form and must be
constrained before reaching provisioning; update the CRD field in
securitygroup_types.go (ImplementationStrategy) to add a kubebuilder validation
allow-list or strict pattern (e.g. +kubebuilder:validation:Enum=<comma-separated
allowed tokens> or
+kubebuilder:validation:Pattern="^network_policy$|^other_allowed$") that
includes at least "network_policy" and any other controller-supported strategy
names, so invalid tokens are rejected at schema validation time.
---
Outside diff comments:
In `@internal/controller/securitygroup_controller.go`:
- Line 92: Remove the obsolete RBAC rule granting read access to virtualnetworks
from the SecurityGroup controller by deleting the kubebuilder comment line "//
+kubebuilder:rbac:groups=osac.openshift.io,resources=virtualnetworks,verbs=get;list;watch"
in internal/controller/securitygroup_controller.go; this controller now resolves
strategy from SecurityGroup.spec in handleUpdate, so ensure only necessary RBAC
annotations remain and run codegen/manifest regeneration if applicable.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 907f69b1-e2b6-48f5-9c83-41733a870464
📒 Files selected for processing (5)
api/v1alpha1/securitygroup_types.goconfig/crd/bases/osac.openshift.io_securitygroups.yamlinternal/controller/constants_common.gointernal/controller/securitygroup_controller.gointernal/controller/securitygroup_controller_test.go
| // ImplementationStrategy determines the backend used to enforce security rules. | ||
| // Set by the fulfillment-service; defaults to network_policy when empty. | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:Type=string | ||
| ImplementationStrategy string `json:"implementationStrategy,omitempty"` |
There was a problem hiding this comment.
Constrain implementationStrategy with an allow-list/pattern before it reaches provisioning paths.
spec.implementationStrategy is currently free-form. Since this value is propagated to controller annotations and consumed downstream, add schema-level validation (enum or strict pattern) to prevent unsafe/unexpected strategy tokens.
Suggested change
// ImplementationStrategy determines the backend used to enforce security rules.
// Set by the fulfillment-service; defaults to network_policy when empty.
// +kubebuilder:validation:Optional
// +kubebuilder:validation:Type=string
+ // +kubebuilder:validation:Pattern=`^[a-z0-9]([a-z0-9_-]*[a-z0-9])?$`
ImplementationStrategy string `json:"implementationStrategy,omitempty"`As per coding guidelines, “Validate at trust boundaries with allow-lists, not deny-lists.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ImplementationStrategy determines the backend used to enforce security rules. | |
| // Set by the fulfillment-service; defaults to network_policy when empty. | |
| // +kubebuilder:validation:Optional | |
| // +kubebuilder:validation:Type=string | |
| ImplementationStrategy string `json:"implementationStrategy,omitempty"` | |
| // ImplementationStrategy determines the backend used to enforce security rules. | |
| // Set by the fulfillment-service; defaults to network_policy when empty. | |
| // +kubebuilder:validation:Optional | |
| // +kubebuilder:validation:Type=string | |
| // +kubebuilder:validation:Pattern=`^[a-z0-9]([a-z0-9_-]*[a-z0-9])?$` | |
| ImplementationStrategy string `json:"implementationStrategy,omitempty"` |
🤖 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 `@api/v1alpha1/securitygroup_types.go` around lines 79 - 83,
spec.implementationStrategy is free-form and must be constrained before reaching
provisioning; update the CRD field in securitygroup_types.go
(ImplementationStrategy) to add a kubebuilder validation allow-list or strict
pattern (e.g. +kubebuilder:validation:Enum=<comma-separated allowed tokens> or
+kubebuilder:validation:Pattern="^network_policy$|^other_allowed$") that
includes at least "network_policy" and any other controller-supported strategy
names, so invalid tokens are rejected at schema validation time.
Source: Coding guidelines
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, ori-amizur 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 |
|
/override /ci/prow/e2e-vmaas |
|
@ori-amizur: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-vmaas |
|
@ori-amizur: Overrode contexts on behalf of ori-amizur: ci/prow/e2e-vmaas 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 kubernetes-sigs/prow repository. |
3 participants
SecurityGroup now reads its implementation strategy from
spec.implementationStrategy (set by the fulfillment-service),
falling back to "network_policy" when unset. This follows the
PublicIPPool pattern and keeps the policy decision in the
fulfillment-service rather than hardcoding it in the operator.
Summary by CodeRabbit
New Features
implementationStrategyfield to specify how security policies are enforced. When not explicitly set,network_policyis used as the default enforcement mechanism.Refactor