add validation webhooks for netfilter nic selector#998
add validation webhooks for netfilter nic selector#998SchSeba wants to merge 1 commit intok8snetworkplumbingwg:masterfrom
Conversation
Signed-off-by: Sebastian Sch <sebassch@gmail.com>
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
WalkthroughAdded NetFilter-specific validation rules to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)pkg/webhook/validate.go (2)
pkg/webhook/validate_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (7)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/webhook/validate_test.go (1)
1364-1426: Test combines NetFilter with PfNames, violating mutual exclusivity validation rules.The test
TestValidatePolicyForNodeStateWithValidVFAndNetFilteruses bothNetFilterandPfNamestogether, which violates the static validation rule enforced instaticValidateSriovNetworkNodePolicythat prohibits using NetFilter with vendor, deviceID, pfNames, or rootDevices. The test passes only because it callsvalidatePolicyForNodeState(dynamic validation) directly, bypassing static validation. In practice, such policies would be rejected at the static validation stage before reaching dynamic validation.This test should be removed or updated to align with the documented mutual exclusivity requirement.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/webhook/validate.gopkg/webhook/validate_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/webhook/validate.go (2)
api/v1/sriovnetworknodepolicy_types.go (1)
Bridge(87-90)pkg/consts/constants.go (1)
LinkTypeETH(51-51)
pkg/webhook/validate_test.go (1)
api/v1/sriovnetworknodepolicy_types.go (5)
SriovNetworkNicSelector(70-84)SriovNetworkNodePolicy(148-154)SriovNetworkNodePolicySpec(27-68)Bridge(87-90)OVSConfig(98-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-coverage
- GitHub Check: test
- GitHub Check: build
- GitHub Check: Golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
pkg/webhook/validate.go (2)
156-176: LGTM! NetFilter validations are well-structured and comprehensive.The validation logic correctly enforces:
- Mutual exclusivity between NetFilter and other nicSelector fields
- Prohibition of EswitchMode when NetFilter is set
- Prohibition of Bridge configuration with NetFilter
- LinkType restriction to "eth"/"ETH" (case-insensitive)
The placement before other nicSelector validations is appropriate, and error messages are clear.
152-154: Existing NetFilter-only workflows are properly tested and will continue to work.Comprehensive test coverage in
pkg/webhook/validate_test.goconfirms that policies using NetFilter as the sole nicSelector parameter pass validation (e.g.,TestStaticValidateSriovNetworkNodePolicyWithNetFilterOnly,TestValidatePolicyForNodeStateWithValidNetFilter). Additionally, the validation enforces that NetFilter is mutually exclusive with other nicSelector fields—mixing NetFilter with vendor, deviceID, pfNames, or rootDevices correctly fails validation with appropriate error messages, as verified by test cases at lines 1193-1248.pkg/webhook/validate_test.go (5)
1193-1249: LGTM! Comprehensive test coverage for mutual exclusivity.The table-driven test approach effectively validates that NetFilter is mutually exclusive with Vendor, DeviceID, PfNames, and RootDevices. The error message validation ensures consistency with the implementation.
1251-1267: LGTM! EswitchMode validation is properly tested.The test correctly verifies that combining NetFilter with EswitchMode is rejected.
1269-1285: LGTM! Bridge configuration validation is properly tested.The test correctly verifies that Bridge configuration cannot be used with NetFilter.
1287-1345: LGTM! Excellent coverage of LinkType validation including case sensitivity.The test cases comprehensively validate:
- Rejection of "ib"/"IB" LinkType values
- Acceptance of "eth"/"ETH" LinkType values (case-insensitive)
- Acceptance of empty LinkType
This matches the case-insensitive validation logic using
strings.EqualFoldin the implementation.
1347-1362: LGTM! Validates that NetFilter can be used alone.This test confirms that NetFilter is a valid standalone nicSelector option.
|
Hi @adrianchiris @zeeke let me know if we can merge this one please |
| return false, fmt.Errorf("nicSelector fields vendor, deviceID, pfNames, and rootDevices are not allowed when netFilter is specified") | ||
| } | ||
| // 2. do not support changing the EswitchMode when NetFilter is specified | ||
| if cr.Spec.EswitchMode != "" { |
There was a problem hiding this comment.
netfilter is when we use VMs right ? so we cant really configure sriov
There was a problem hiding this comment.
right that is why we don't want to allow this
| return false, fmt.Errorf("bridge configuration is not supported when netFilter is specified") | ||
| } | ||
| // 4. LinkType only "eth", "ETH" allowed when NetFilter is specified | ||
| if cr.Spec.LinkType != "" && !strings.EqualFold(cr.Spec.LinkType, consts.LinkTypeETH) { |
There was a problem hiding this comment.
why do we limit this just to eth ?
There was a problem hiding this comment.
because that is what we tested right now and there is a request.
if someone will test and request IB device we can later remove this validation
SchSeba
left a comment
SchSeba
left a comment
There was a problem hiding this comment.
Thanks for the comments @adrianchiris I answer on both
| return false, fmt.Errorf("nicSelector fields vendor, deviceID, pfNames, and rootDevices are not allowed when netFilter is specified") | ||
| } | ||
| // 2. do not support changing the EswitchMode when NetFilter is specified | ||
| if cr.Spec.EswitchMode != "" { |
There was a problem hiding this comment.
right that is why we don't want to allow this
| return false, fmt.Errorf("bridge configuration is not supported when netFilter is specified") | ||
| } | ||
| // 4. LinkType only "eth", "ETH" allowed when NetFilter is specified | ||
| if cr.Spec.LinkType != "" && !strings.EqualFold(cr.Spec.LinkType, consts.LinkTypeETH) { |
There was a problem hiding this comment.
because that is what we tested right now and there is a request.
if someone will test and request IB device we can later remove this validation
3 participants
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.