fix: skip isRdma mutation for vfio-pci with InfiniBand linkType#1039
fix: skip isRdma mutation for vfio-pci with InfiniBand linkType#1039SchSeba wants to merge 1 commit intok8snetworkplumbingwg:masterfrom
Conversation
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Summary of ChangesHello @SchSeba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the mutating webhook that incorrectly applied Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a critical bug where the mutating webhook incorrectly applied the isRdma: true patch to policies with vfio-pci device types and InfiniBand link types, leading to validation failures. The introduced conditional check for deviceType before applying the isRdma patch correctly prevents this conflict, and comprehensive unit tests for mutateSriovNetworkNodePolicy significantly improve test coverage. However, a potential Denial of Service vulnerability was identified in the new code due to an unsafe type assertion on the 'spec' field. If a malformed or incomplete resource is submitted, the webhook could panic, affecting the availability of the admission process.
pkg/webhook/mutate.go
Outdated
| if str, ok := spec.(map[string]interface{})["linkType"].(string); ok && strings.EqualFold(str, constants.LinkTypeIB) { | ||
| log.Log.V(2).Info("mutateSriovNetworkNodePolicy(): set isRdma to true for policy since ib link type is detected", "policy-name", name) | ||
| patchs = append(patchs, InfiniBandIsRdmaPatch) | ||
| devType, _ := spec.(map[string]interface{})["deviceType"].(string) |
There was a problem hiding this comment.
The code performs a type assertion spec.(map[string]interface{}) without checking if spec is nil or of the expected type. If a user submits a SriovNetworkNodePolicy without a spec field, the webhook will panic. This can be used to cause a Denial of Service (DoS) for the admission controller. While this pattern exists elsewhere in the file, it is introduced here in the new code. It is recommended to use the comma-ok idiom for type assertions to safely handle cases where the 'spec' field might be missing or malformed.
| devType, _ := spec.(map[string]interface{})["deviceType"].(string) | |
| specMap, ok := spec.(map[string]interface{}) | |
| if !ok { | |
| return &reviewResponse, nil | |
| } | |
| devType, _ := specMap["deviceType"].(string) |
SchSeba
force-pushed
the
support_vfio_ib_in_webhook
branch
from
e57ce51 to
cc0f2e2
Compare
The mutating webhook unconditionally set isRdma to true for policies with InfiniBand link type, even when deviceType was vfio-pci. This caused the validating webhook to reject the resource since deviceType vfio-pci conflicts with isRdma: true. Check the deviceType before applying the InfiniBand isRdma patch and skip it when deviceType is vfio-pci. Add unit tests for mutateSriovNetworkNodePolicy covering default values, InfiniBand with various device types, Ethernet, and preservation of user-specified fields. Signed-off-by: Sebastian Sch <sebassch@gmail.com>
SchSeba
force-pushed
the
support_vfio_ib_in_webhook
branch
from
cc0f2e2 to
936ef00
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves an issue where the mutating webhook incorrectly set isRdma: true for vfio-pci devices with an InfiniBand link type. The fix is correct and appropriately checks the deviceType before applying the mutation. The addition of a comprehensive unit test suite for mutateSriovNetworkNodePolicy is an excellent improvement that significantly enhances the robustness of this component. I have one minor suggestion to improve the explicitness of the logic in mutate.go.
| devType, _ := specMap["deviceType"].(string) | ||
| if !strings.EqualFold(devType, constants.DeviceTypeVfioPci) { | ||
| log.Log.V(2).Info("mutateSriovNetworkNodePolicy(): set isRdma to true for policy since ib link type is detected", "policy-name", name) | ||
| patchs = append(patchs, InfiniBandIsRdmaPatch) | ||
| } |
There was a problem hiding this comment.
This logic can be made more robust and explicit by checking the ok value from the type assertion. This avoids relying on the zero value of a string to achieve the correct behavior when deviceType is not specified. The following suggestion refactors this block to be more idiomatic and clear.
if devType, ok := specMap["deviceType"].(string); !ok || !strings.EqualFold(devType, constants.DeviceTypeVfioPci) {
log.Log.V(2).Info("mutateSriovNetworkNodePolicy(): set isRdma to true for policy since ib link type is detected", "policy-name", name)
patchs = append(patchs, InfiniBandIsRdmaPatch)
}
Pull Request Test Coverage Report for Build 22179241629Details
💛 - Coveralls |
2 participants
The mutating webhook unconditionally set isRdma to true for policies with InfiniBand link type, even when deviceType was vfio-pci. This caused the validating webhook to reject the resource since deviceType vfio-pci conflicts with isRdma: true.
Check the deviceType before applying the InfiniBand isRdma patch and skip it when deviceType is vfio-pci.
Add unit tests for mutateSriovNetworkNodePolicy covering default values, InfiniBand with various device types, Ethernet, and preservation of user-specified fields.