[kubernetes] Fix CiliumNetworkPolicy endpointSelector for multi-node RWX volumes#2227
Conversation
mattia-eleuteri
requested review from
IvanHunters,
androndo,
kvaps,
lexfrei,
lllamnyp and
sircthulhu
as code owners
dosubot
bot
added
size:M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughControllerPublishVolume now builds endpointSelectors dynamically from VM names extracted from ownerReferences. OwnerReference additions/removals trigger rebuilding the selector and updating the CiliumNetworkPolicy via new helper functions Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller
participant Builder as buildEndpointSelector
participant K8sAPI as Kubernetes API (CiliumNetworkPolicy)
participant VM as VM ownerReferences
Controller->>VM: read ownerReferences
Controller->>Builder: vmNamesFromOwnerRefs(ownerReferences)
Builder-->>Controller: endpointSelector (matchExpressions for VM names)
Controller->>K8sAPI: create/update CiliumNetworkPolicy with selector
note right of K8sAPI: On ownerReference change,\nController repeats flow to rebuild selector
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
Summary of ChangesHello, 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 resolves a bug in the kubevirt-csi-driver that prevented NFS-backed RWX volumes from being correctly mounted on multiple VMs due to an improperly configured CiliumNetworkPolicy. The changes ensure that the endpoint selector is dynamically updated to include all relevant VMs, allowing for successful NFS egress and mounts. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Activity
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
|
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
mattia-eleuteri
force-pushed
the
fix/cnp-endpoint-selector-rwx
branch
from
9a78c81 to
81230e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go (1)
428-445:⚠️ Potential issue | 🟠 MajorAlways reconcile
endpointSelector, even when ownerRef already exists.At Line 431, the early return skips selector rebuild. This leaves previously broken CNPs (already containing multiple ownerRefs but stale single-VM selector) unreconciled on retries for the same VM.
🔧 Proposed fix
func (w *WrappedControllerService) addCNPOwnerReference(ctx context.Context, namespace, cnpName string, ownerRef map[string]interface{}) error { return retry.RetryOnConflict(retry.DefaultRetry, func() error { existing, err := w.dynamicClient.Resource(ciliumNetworkPolicyGVR).Namespace(namespace).Get(ctx, cnpName, metav1.GetOptions{}) if err != nil { return status.Errorf(codes.Internal, "failed to get CiliumNetworkPolicy %s: %v", cnpName, err) } ownerRefs, _, _ := unstructured.NestedSlice(existing.Object, "metadata", "ownerReferences") uid, _, _ := unstructured.NestedString(ownerRef, "uid") + alreadyPresent := false for _, ref := range ownerRefs { if refMap, ok := ref.(map[string]interface{}); ok { if refMap["uid"] == uid { - return nil // already present + alreadyPresent = true + break } } } - ownerRefs = append(ownerRefs, ownerRef) - if err := unstructured.SetNestedSlice(existing.Object, ownerRefs, "metadata", "ownerReferences"); err != nil { - return status.Errorf(codes.Internal, "failed to set ownerReferences: %v", err) + if !alreadyPresent { + ownerRefs = append(ownerRefs, ownerRef) + if err := unstructured.SetNestedSlice(existing.Object, ownerRefs, "metadata", "ownerReferences"); err != nil { + return status.Errorf(codes.Internal, "failed to set ownerReferences: %v", err) + } } // Rebuild endpointSelector to include all VMs selector := buildEndpointSelector(vmNamesFromOwnerRefs(ownerRefs)) if err := unstructured.SetNestedField(existing.Object, selector, "spec", "endpointSelector"); err != nil { return status.Errorf(codes.Internal, "failed to set endpointSelector: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go` around lines 428 - 445, The early return when detecting an existing ownerRef prevents rebuilding the endpointSelector; change the logic in the ownerRefs loop so that you do not return immediately when refMap["uid"] == uid but instead skip appending the duplicate ownerRef while continuing to rebuild the selector and update the object: keep the ownerRefs slice deduping behavior (do not append if already present), then always call buildEndpointSelector(vmNamesFromOwnerRefs(ownerRefs)) and unstructured.SetNestedField(existing.Object, selector, "spec", "endpointSelector") (and the existing SetNestedSlice call only when ownerRefs changed) so stale single-VM selectors are reconciled even when the ownerRef was already present.
🧹 Nitpick comments (1)
packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go (1)
525-536: Consider deduplicating VM names extracted from ownerReferences.If owner refs ever contain duplicate VM names (e.g., stale refs with different UIDs),
valuescan include duplicates. Deduping here keeps selectors minimal and deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go` around lines 525 - 536, The vmNamesFromOwnerRefs function may return duplicate VM names when ownerReferences contain repeated entries; change vmNamesFromOwnerRefs to deduplicate before returning by using a seen set (e.g., map[string]struct{}) while iterating ownerRefs and only append names not yet seen, so the returned slice from vmNamesFromOwnerRefs contains unique, deterministic VM names for selector construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go`:
- Around line 428-445: The early return when detecting an existing ownerRef
prevents rebuilding the endpointSelector; change the logic in the ownerRefs loop
so that you do not return immediately when refMap["uid"] == uid but instead skip
appending the duplicate ownerRef while continuing to rebuild the selector and
update the object: keep the ownerRefs slice deduping behavior (do not append if
already present), then always call
buildEndpointSelector(vmNamesFromOwnerRefs(ownerRefs)) and
unstructured.SetNestedField(existing.Object, selector, "spec",
"endpointSelector") (and the existing SetNestedSlice call only when ownerRefs
changed) so stale single-VM selectors are reconciled even when the ownerRef was
already present.
---
Nitpick comments:
In `@packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go`:
- Around line 525-536: The vmNamesFromOwnerRefs function may return duplicate VM
names when ownerReferences contain repeated entries; change vmNamesFromOwnerRefs
to deduplicate before returning by using a seen set (e.g., map[string]struct{})
while iterating ownerRefs and only append names not yet seen, so the returned
slice from vmNamesFromOwnerRefs contains unique, deterministic VM names for
selector construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d414e48a-54a4-446c-9941-6ec98ef269c3
📒 Files selected for processing (1)
packages/apps/kubernetes/images/kubevirt-csi-driver/controller.go
mattia-eleuteri
changed the title
…RWX volumes When an NFS-backed RWX volume is published to multiple VMs, the CiliumNetworkPolicy egress rule only allowed traffic from the first VM. The endpointSelector.matchLabels was set once on creation and never broadened, causing NFS mounts to hang on all nodes except the first. Switch from matchLabels to matchExpressions (operator: In) so the selector can list multiple VM names. Rebuild the selector whenever ownerReferences are added or removed. Signed-off-by: mattia-eleuteri <mattia@hidora.io>
mattia-eleuteri
force-pushed
the
fix/cnp-endpoint-selector-rwx
branch
from
81230e4 to
cc5ec0b
Compare
kvaps
added
backport
|
Successfully created backport PR for |
|
Successfully created backport PR for |
2 participants
What this PR does
When an NFS-backed RWX volume is published to multiple VMs, the
CiliumNetworkPolicyendpointSelector.matchLabelsonly included the first VM. SubsequentControllerPublishVolumecalls addedownerReferencesbut never broadened the selector, causing Cilium to block NFS egress — mounts hang on all nodes except the first.This PR switches from
matchLabelstomatchExpressions(operator: In) so the selector can list multiple VM names, and rebuilds it whenever ownerReferences are added or removed.Release note
Summary by CodeRabbit
New Features
Improvements