From bc937da20ebeb038c550d01644548b32babaf8b2 Mon Sep 17 00:00:00 2001 From: Enrique Llorente Date: Thu, 4 Jul 2024 16:39:01 +0200 Subject: [PATCH 1/7] config: Add 'role' field to config Signed-off-by: Enrique Llorente --- pkg/config/types.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/config/types.go b/pkg/config/types.go index 9b12ec5a..3f983ef6 100644 --- a/pkg/config/types.go +++ b/pkg/config/types.go @@ -5,9 +5,16 @@ import ( "fmt" ) +type NetworkRole string + +const ( + NetworkRolePrimary NetworkRole = "primary" +) + type RelevantConfig struct { - Name string `json:"name"` - AllowPersistentIPs bool `json:"allowPersistentIPs,omitempty"` + Name string `json:"name"` + AllowPersistentIPs bool `json:"allowPersistentIPs,omitempty"` + Role NetworkRole `json:"role,omitempty"` } func NewConfig(nadSpec string) (*RelevantConfig, error) { From 6ba3618649cb0df734a5bf44cda28ec5574edbea Mon Sep 17 00:00:00 2001 From: Enrique Llorente Date: Thu, 4 Jul 2024 16:40:10 +0200 Subject: [PATCH 2/7] udn: Add FindPrimaryNetwork helper Signed-off-by: Enrique Llorente --- pkg/udn/primary_network.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 pkg/udn/primary_network.go diff --git a/pkg/udn/primary_network.go b/pkg/udn/primary_network.go new file mode 100644 index 00000000..d581e959 --- /dev/null +++ b/pkg/udn/primary_network.go @@ -0,0 +1,33 @@ +package udn + +import ( + "context" + "fmt" + + v1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" + + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kubevirt/ipam-extensions/pkg/config" +) + +func FindPrimaryNetwork(ctx context.Context, + cli client.Client, + namespace string) (*v1.NetworkAttachmentDefinition, error) { + nadList := v1.NetworkAttachmentDefinitionList{} + if err := cli.List(ctx, &nadList, client.InNamespace(namespace)); err != nil { + return nil, fmt.Errorf("failed listing nads for pod namespace %q", namespace) + } + + for _, nad := range nadList.Items { + netConfig, err := config.NewConfig(nad.Spec.Config) + if err != nil { + return nil, fmt.Errorf("failed to extract the relevant NAD information") + } + if netConfig.Role == config.NetworkRolePrimary { + return ptr.To(nad), nil + } + } + return nil, nil +} From 3b5970bd564f128e0767fa6f07fdfba60bff491e Mon Sep 17 00:00:00 2001 From: Enrique Llorente Date: Thu, 4 Jul 2024 16:50:23 +0200 Subject: [PATCH 3/7] webhook: Check first vm annotation Signed-off-by: Enrique Llorente --- pkg/ipamclaimswebhook/podmutator.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/ipamclaimswebhook/podmutator.go b/pkg/ipamclaimswebhook/podmutator.go index a9b2d3eb..7f2a3112 100644 --- a/pkg/ipamclaimswebhook/podmutator.go +++ b/pkg/ipamclaimswebhook/podmutator.go @@ -65,6 +65,14 @@ func (a *IPAMClaimsValet) Handle(ctx context.Context, request admission.Request) return admission.Errored(http.StatusBadRequest, err) } + vmName, hasVMAnnotation := pod.Annotations["kubevirt.io/domain"] + if !hasVMAnnotation { + log.Info( + "does not have the kubevirt VM annotation", + ) + return admission.Allowed("not a VM") + } + log.Info("webhook handling event") networkSelectionElements, err := netutils.ParsePodNetworkAnnotation(pod) if err != nil { @@ -112,15 +120,6 @@ func (a *IPAMClaimsValet) Handle(ctx context.Context, request admission.Request) "NAD", nadName, "network", pluginConfig.Name, ) - vmName, hasVMAnnotation := pod.Annotations["kubevirt.io/domain"] - if !hasVMAnnotation { - log.Info( - "does not have the kubevirt VM annotation", - "NAD", nadName, - "network", pluginConfig.Name, - ) - return admission.Allowed("not a VM") - } vmKey := types.NamespacedName{Namespace: pod.Namespace, Name: vmName} vmi := &virtv1.VirtualMachineInstance{} From fba614fd3f860f46d7568d4c6c7605e32b924eaa Mon Sep 17 00:00:00 2001 From: Enrique Llorente Date: Thu, 4 Jul 2024 16:57:47 +0200 Subject: [PATCH 4/7] types: Add OVN annotation Signed-off-by: Enrique Llorente --- pkg/config/types.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/config/types.go b/pkg/config/types.go index 3f983ef6..fc9fdc98 100644 --- a/pkg/config/types.go +++ b/pkg/config/types.go @@ -11,6 +11,8 @@ const ( NetworkRolePrimary NetworkRole = "primary" ) +const OVNPrimaryNetworkIPAMClaimAnnotation = "k8s.ovn.org/ovn-udn-ipamclaim-reference" + type RelevantConfig struct { Name string `json:"name"` AllowPersistentIPs bool `json:"allowPersistentIPs,omitempty"` From 71dfd6c34d261bfd75a28923685252c8d76910f5 Mon Sep 17 00:00:00 2001 From: Enrique Llorente Date: Thu, 4 Jul 2024 17:28:36 +0200 Subject: [PATCH 5/7] ipamclaim: Add key composer Signed-off-by: Enrique Llorente --- pkg/claims/claims.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/claims/claims.go b/pkg/claims/claims.go index 8c8a5370..699ecbf2 100644 --- a/pkg/claims/claims.go +++ b/pkg/claims/claims.go @@ -41,3 +41,7 @@ func OwnedByVMLabel(vmiName string) client.MatchingLabels { virtv1.VirtualMachineLabel: vmiName, } } + +func ComposeKey(vmName, networkName string) string { + return fmt.Sprintf("%s.%s", vmName, networkName) +} From d008090d67b68af7626467f0a31715f367ba8df2 Mon Sep 17 00:00:00 2001 From: Enrique Llorente Date: Thu, 4 Jul 2024 17:08:09 +0200 Subject: [PATCH 6/7] webhook: Annotate with primary network ipamclaim Signed-off-by: Enrique Llorente --- pkg/ipamclaimswebhook/podmutator.go | 238 +++++++++++++++-------- pkg/ipamclaimswebhook/podmutator_test.go | 121 +++++++++--- 2 files changed, 251 insertions(+), 108 deletions(-) diff --git a/pkg/ipamclaimswebhook/podmutator.go b/pkg/ipamclaimswebhook/podmutator.go index 7f2a3112..9745bcf4 100644 --- a/pkg/ipamclaimswebhook/podmutator.go +++ b/pkg/ipamclaimswebhook/podmutator.go @@ -39,7 +39,9 @@ import ( virtv1 "kubevirt.io/api/core/v1" + "github.com/kubevirt/ipam-extensions/pkg/claims" "github.com/kubevirt/ipam-extensions/pkg/config" + "github.com/kubevirt/ipam-extensions/pkg/udn" ) // +kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=fail,groups="",resources=pods,verbs=create;update,versions=v1,name=ipam-claims.k8s.cni.cncf.io,admissionReviewVersions=v1,sideEffects=None @@ -65,6 +67,8 @@ func (a *IPAMClaimsValet) Handle(ctx context.Context, request admission.Request) return admission.Errored(http.StatusBadRequest, err) } + log.Info("webhook handling event") + vmName, hasVMAnnotation := pod.Annotations["kubevirt.io/domain"] if !hasVMAnnotation { log.Info( @@ -73,93 +77,43 @@ func (a *IPAMClaimsValet) Handle(ctx context.Context, request admission.Request) return admission.Allowed("not a VM") } - log.Info("webhook handling event") networkSelectionElements, err := netutils.ParsePodNetworkAnnotation(pod) if err != nil { var goodTypeOfError *v1.NoK8sNetworkError - if errors.As(err, &goodTypeOfError) { - return admission.Allowed("no secondary networks requested") + if !errors.As(err, &goodTypeOfError) { + return admission.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse pod network selection elements")) } - return admission.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse pod network selection elements")) } - var ( - hasChangedNetworkSelectionElements bool - podNetworkSelectionElements = make([]v1.NetworkSelectionElement, 0, len(networkSelectionElements)) - ) - for _, networkSelectionElement := range networkSelectionElements { - nadName := types.NamespacedName{ - Namespace: networkSelectionElement.Namespace, - Name: networkSelectionElement.Name, - }.String() - log.Info( - "iterating network selection elements", - "NAD", nadName, - ) - nadKey := types.NamespacedName{ - Namespace: networkSelectionElement.Namespace, - Name: networkSelectionElement.Name, - } - - nad := v1.NetworkAttachmentDefinition{} - if err := a.Client.Get(context.Background(), nadKey, &nad); err != nil { - if k8serrors.IsNotFound(err) { - return admission.Allowed("NAD not found, will hang on scheduler") - } - return admission.Errored(http.StatusInternalServerError, err) + var newPod *corev1.Pod + hasChangedNetworkSelectionElements, err := + ensureIPAMClaimRefAtNetworkSelectionElements(ctx, a.Client, pod, vmName, networkSelectionElements) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } + if hasChangedNetworkSelectionElements { + if newPod == nil { + newPod = pod.DeepCopy() } - - pluginConfig, err := config.NewConfig(nad.Spec.Config) - if err != nil { + if err := updatePodSelectionElements(newPod, networkSelectionElements); err != nil { return admission.Errored(http.StatusInternalServerError, err) } - - if pluginConfig.AllowPersistentIPs { - log.Info( - "will request persistent IPs", - "NAD", nadName, - "network", pluginConfig.Name, - ) - - vmKey := types.NamespacedName{Namespace: pod.Namespace, Name: vmName} - vmi := &virtv1.VirtualMachineInstance{} - if err := a.Client.Get(context.Background(), vmKey, vmi); err != nil { - return admission.Errored(http.StatusInternalServerError, err) - } - - vmiNets := vmiSecondaryNetworks(vmi) - networkName, foundNetworkName := vmiNets[nadKey.String()] - if !foundNetworkName { - log.Info( - "network name not found", - "NAD", nadName, - "network", networkName, - ) - podNetworkSelectionElements = append(podNetworkSelectionElements, *networkSelectionElement) - continue - } - - networkSelectionElement.IPAMClaimReference = fmt.Sprintf("%s.%s", vmName, networkName) - log.Info( - "requesting claim", - "NAD", nadName, - "network", pluginConfig.Name, - "claim", networkSelectionElement.IPAMClaimReference, - ) - podNetworkSelectionElements = append(podNetworkSelectionElements, *networkSelectionElement) - hasChangedNetworkSelectionElements = true - continue - } - podNetworkSelectionElements = append(podNetworkSelectionElements, *networkSelectionElement) } - if len(podNetworkSelectionElements) > 0 { - newPod, err := podWithUpdatedSelectionElements(pod, podNetworkSelectionElements) - if err != nil { - return admission.Errored(http.StatusInternalServerError, err) + newPrimaryNetworkIPAMClaimName, err := findNewPrimaryNetworkIPAMClaimName(ctx, a.Client, pod, vmName) + if err != nil { + return admission.Errored(http.StatusInternalServerError, + fmt.Errorf("failed looking for primary user defined IPAMClaim name: %v", err)) + } + if newPrimaryNetworkIPAMClaimName != "" { + if newPod == nil { + newPod = pod.DeepCopy() } + updatePodWithOVNPrimaryNetworkIPAMClaimAnnotation(newPod, newPrimaryNetworkIPAMClaimName) + } - if reflect.DeepEqual(newPod, pod) || !hasChangedNetworkSelectionElements { + if newPod != nil { + if reflect.DeepEqual(newPod, pod) { return admission.Allowed("mutation not needed") } @@ -171,6 +125,7 @@ func (a *IPAMClaimsValet) Handle(ctx context.Context, request admission.Request) log.Info("new pod annotations", "pod", newPod.Annotations) return admission.PatchResponseFromRaw(request.Object.Raw, marshaledPod) } + return admission.Allowed("carry on") } @@ -195,12 +150,137 @@ func vmiSecondaryNetworks(vmi *virtv1.VirtualMachineInstance) map[string]string return indexedSecondaryNetworks } -func podWithUpdatedSelectionElements(pod *corev1.Pod, networks []v1.NetworkSelectionElement) (*corev1.Pod, error) { - newPod := pod.DeepCopy() +func updatePodSelectionElements(pod *corev1.Pod, networks []*v1.NetworkSelectionElement) error { newNets, err := json.Marshal(networks) if err != nil { - return nil, err + return err + } + pod.Annotations[v1.NetworkAttachmentAnnot] = string(newNets) + return nil +} + +func updatePodWithOVNPrimaryNetworkIPAMClaimAnnotation(pod *corev1.Pod, primaryNetworkIPAMClaimName string) { + pod.Annotations[config.OVNPrimaryNetworkIPAMClaimAnnotation] = primaryNetworkIPAMClaimName +} + +func ensureIPAMClaimRefAtNetworkSelectionElements(ctx context.Context, + cli client.Client, pod *corev1.Pod, vmName string, + networkSelectionElements []*v1.NetworkSelectionElement) (changed bool, err error) { + log := logf.FromContext(ctx) + hasChangedNetworkSelectionElements := false + for i, networkSelectionElement := range networkSelectionElements { + nadName := fmt.Sprintf("%s/%s", networkSelectionElement.Namespace, networkSelectionElement.Name) + log.Info( + "iterating network selection elements", + "NAD", nadName, + ) + nadKey := types.NamespacedName{ + Namespace: networkSelectionElement.Namespace, + Name: networkSelectionElement.Name, + } + + nad := v1.NetworkAttachmentDefinition{} + if err := cli.Get(context.Background(), nadKey, &nad); err != nil { + if k8serrors.IsNotFound(err) { + log.Info("NAD not found, will hang on scheduler", "NAD", nadName) + return false, nil + } + return false, err + } + + pluginConfig, err := config.NewConfig(nad.Spec.Config) + if err != nil { + return false, err + } + + if !pluginConfig.AllowPersistentIPs { + continue + } + + log.Info( + "will request persistent IPs", + "NAD", nadName, + "network", pluginConfig.Name, + ) + + vmKey := types.NamespacedName{Namespace: pod.Namespace, Name: vmName} + vmi := &virtv1.VirtualMachineInstance{} + if err := cli.Get(context.Background(), vmKey, vmi); err != nil { + return false, err + } + + vmiNets := vmiSecondaryNetworks(vmi) + networkName, foundNetworkName := vmiNets[nadKey.String()] + if !foundNetworkName { + log.Info( + "network name not found", + "NAD", nadName, + "network", networkName, + ) + continue + } + + networkSelectionElements[i].IPAMClaimReference = claims.ComposeKey(vmName, networkName) + log.Info( + "requesting claim", + "NAD", nadName, + "network", pluginConfig.Name, + "claim", networkSelectionElement.IPAMClaimReference, + ) + hasChangedNetworkSelectionElements = true + continue + } + return hasChangedNetworkSelectionElements, nil +} + +func findNewPrimaryNetworkIPAMClaimName(ctx context.Context, + cli client.Client, pod *corev1.Pod, vmName string) (string, error) { + log := logf.FromContext(ctx) + if pod.Annotations[config.OVNPrimaryNetworkIPAMClaimAnnotation] != "" { + return "", nil + } + primaryNetworkNAD, err := udn.FindPrimaryNetwork(ctx, cli, pod.Namespace) + if err != nil { + return "", err + } + if primaryNetworkNAD == nil { + return "", nil + } + pluginConfig, err := config.NewConfig(primaryNetworkNAD.Spec.Config) + if err != nil { + return "", err + } + + if !pluginConfig.AllowPersistentIPs { + return "", nil + } + + log.Info( + "will request primary network persistent IPs", + "NAD", client.ObjectKeyFromObject(primaryNetworkNAD), + "network", pluginConfig.Name, + ) + vmKey := types.NamespacedName{Namespace: pod.Namespace, Name: vmName} + vmi := &virtv1.VirtualMachineInstance{} + if err := cli.Get(context.Background(), vmKey, vmi); err != nil { + return "", err + } + + networkName := vmiPodNetworkName(vmi) + if networkName == "" { + log.Info("vmi has no pod network primary UDN ipam claim will not be created", "vmi", vmKey.String()) + return "", nil + } + + return claims.ComposeKey(vmi.Name, networkName), nil +} + +// returns the name of the kubevirt VM pod network +func vmiPodNetworkName(vmi *virtv1.VirtualMachineInstance) string { + for _, network := range vmi.Spec.Networks { + if network.Pod != nil { + return network.Name + } } - newPod.Annotations[v1.NetworkAttachmentAnnot] = string(newNets) - return newPod, nil + return "" } diff --git a/pkg/ipamclaimswebhook/podmutator_test.go b/pkg/ipamclaimswebhook/podmutator_test.go index b204fa9c..f4afe612 100644 --- a/pkg/ipamclaimswebhook/podmutator_test.go +++ b/pkg/ipamclaimswebhook/podmutator_test.go @@ -36,7 +36,7 @@ import ( type testConfig struct { inputVM *virtv1.VirtualMachine inputVMI *virtv1.VirtualMachineInstance - inputNAD *nadv1.NetworkAttachmentDefinition + inputNADs []*nadv1.NetworkAttachmentDefinition inputPod *corev1.Pod expectedAdmissionResponse admission.Response } @@ -86,8 +86,8 @@ var _ = Describe("KubeVirt IPAM launcher pod mutato machine", Serial, func() { initialObjects = append(initialObjects, config.inputVMI) } - if config.inputNAD != nil { - initialObjects = append(initialObjects, config.inputNAD) + for _, nad := range config.inputNADs { + initialObjects = append(initialObjects, nad) } ctrlOptions := controllerruntime.Options{ @@ -111,25 +111,81 @@ var _ = Describe("KubeVirt IPAM launcher pod mutato machine", Serial, func() { Equal(config.expectedAdmissionResponse), ) }, - Entry("pod not requesting secondary attachments is accepted", testConfig{ + Entry("pod not beloging to a VM and not requesting secondary "+ + "attachments and no primary user defined network is accepted", testConfig{ inputVM: dummyVM(nadName), inputVMI: dummyVMI(nadName), - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, inputPod: &corev1.Pod{}, expectedAdmissionResponse: admission.Response{ AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: true, Result: &metav1.Status{ - Message: "no secondary networks requested", + Message: "not a VM", Code: http.StatusOK, }, }, }, }), - Entry("vm launcher pod with an attachment to a network with persistent IPs enabled requests an IPAMClaim", testConfig{ + Entry("vm launcher pod with an attachment to a primary and secondary user "+ + "defined network with persistent IPs enabled requests an IPAMClaim", testConfig{ inputVM: dummyVM(nadName), inputVMI: dummyVMI(nadName), - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + dummyPrimaryNetworkNAD(nadName), + }, + inputPod: dummyPodForVM(nadName, vmName), + expectedAdmissionResponse: admission.Response{ + AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: true, + PatchType: &patchType, + }, + Patches: []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/metadata/annotations/k8s.ovn.org~1ovn-udn-ipamclaim-reference", + Value: "vm1.podnet", + }, + { + Operation: "replace", + Path: "/metadata/annotations/k8s.v1.cni.cncf.io~1networks", + Value: "[{\"name\":\"supadupanet\",\"namespace\":\"ns1\",\"ipam-claim-reference\":\"vm1.randomnet\"}]", + }, + }, + }, + }), + Entry("vm launcher pod with with primary user defined network defined "+ + "at namespace with persistent IPs enabled requests an IPAMClaim", testConfig{ + inputVM: dummyVM(nadName), + inputVMI: dummyVMI(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyPrimaryNetworkNAD(nadName), + }, + inputPod: dummyPodForVM("" /*without network selection element*/, vmName), + expectedAdmissionResponse: admission.Response{ + AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: true, + PatchType: &patchType, + }, + Patches: []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/metadata/annotations/k8s.ovn.org~1ovn-udn-ipamclaim-reference", + Value: "vm1.podnet", + }, + }, + }, + }), + Entry("vm launcher pod with an attachment to a secondary user defined "+ + "network with persistent IPs enabled requests an IPAMClaim", testConfig{ + inputVM: dummyVM(nadName), + inputVMI: dummyVMI(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, inputPod: dummyPodForVM(nadName, vmName), expectedAdmissionResponse: admission.Response{ AdmissionResponse: admissionv1.AdmissionResponse{ @@ -148,20 +204,24 @@ var _ = Describe("KubeVirt IPAM launcher pod mutato machine", Serial, func() { Entry("vm launcher pod with an attachment to a network *without* persistentIPs is accepted", testConfig{ inputVM: dummyVM(nadName), inputVMI: dummyVMI(nadName), - inputNAD: dummyNADWithoutPersistentIPs(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNADWithoutPersistentIPs(nadName), + }, inputPod: dummyPodForVM(nadName, vmName), expectedAdmissionResponse: admission.Response{ AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: true, Result: &metav1.Status{ - Message: "mutation not needed", + Message: "carry on", Code: http.StatusOK, }, }, }, }), Entry("pod not belonging to a VM with an attachment to a network with persistent IPs enabled is accepted", testConfig{ - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, inputPod: dummyPod(nadName), expectedAdmissionResponse: admission.Response{ AdmissionResponse: admissionv1.AdmissionResponse{ @@ -176,8 +236,10 @@ var _ = Describe("KubeVirt IPAM launcher pod mutato machine", Serial, func() { Entry("pod requesting an attachment via a NAD with an invalid configuration throws a BAD REQUEST", testConfig{ inputVM: dummyVM(nadName), inputVMI: dummyVMI(nadName), - inputNAD: dummyNAD(nadName), - inputPod: pod("{not json}", nil), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, + inputPod: dummyPodForVM("{not json}", vmName), expectedAdmissionResponse: admission.Response{ AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: false, @@ -196,14 +258,16 @@ var _ = Describe("KubeVirt IPAM launcher pod mutato machine", Serial, func() { AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: true, Result: &metav1.Status{ - Message: "NAD not found, will hang on scheduler", + Message: "carry on", Code: http.StatusOK, }, }, }, }), Entry("launcher pod whose VMI is not found throws a server error", testConfig{ - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, inputPod: dummyPodForVM(nadName, vmName), expectedAdmissionResponse: admission.Response{ AdmissionResponse: admissionv1.AdmissionResponse{ @@ -261,7 +325,7 @@ func dummyVMISpec(nadName string) virtv1.VirtualMachineInstanceSpec { } } -func dummyNAD(nadName string) *nadv1.NetworkAttachmentDefinition { +func dummyNADWithConfig(nadName string, config string) *nadv1.NetworkAttachmentDefinition { namespaceAndName := strings.Split(nadName, "/") return &nadv1.NetworkAttachmentDefinition{ ObjectMeta: metav1.ObjectMeta{ @@ -269,22 +333,20 @@ func dummyNAD(nadName string) *nadv1.NetworkAttachmentDefinition { Name: namespaceAndName[1], }, Spec: nadv1.NetworkAttachmentDefinitionSpec{ - Config: `{"name": "goodnet", "allowPersistentIPs": true}`, + Config: config, }, } } +func dummyNAD(nadName string) *nadv1.NetworkAttachmentDefinition { + return dummyNADWithConfig(nadName, `{"name": "goodnet", "allowPersistentIPs": true}`) +} + +func dummyPrimaryNetworkNAD(nadName string) *nadv1.NetworkAttachmentDefinition { + return dummyNADWithConfig(nadName+"primary", `{"name": "primarynet", "role": "primary", "allowPersistentIPs": true}`) +} func dummyNADWithoutPersistentIPs(nadName string) *nadv1.NetworkAttachmentDefinition { - namespaceAndName := strings.Split(nadName, "/") - return &nadv1.NetworkAttachmentDefinition{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: namespaceAndName[0], - Name: namespaceAndName[1], - }, - Spec: nadv1.NetworkAttachmentDefinitionSpec{ - Config: `{"name": "goodnet"}`, - }, - } + return dummyNADWithConfig(nadName, `{"name": "goodnet"}`) } func podAdmissionRequest(pod *corev1.Pod) admission.Request { @@ -312,8 +374,9 @@ func dummyPod(nadName string) *corev1.Pod { } func pod(nadName string, annotations map[string]string) *corev1.Pod { - baseAnnotations := map[string]string{ - nadv1.NetworkAttachmentAnnot: nadName, + baseAnnotations := map[string]string{} + if nadName != "" { + baseAnnotations[nadv1.NetworkAttachmentAnnot] = nadName } for k, v := range annotations { baseAnnotations[k] = v From bdabec2434bf54c98e8296ed783585d6f407892d Mon Sep 17 00:00:00 2001 From: Enrique Llorente Date: Thu, 4 Jul 2024 17:41:42 +0200 Subject: [PATCH 7/7] vm, controller: Create IPAMClaim for primary Signed-off-by: Enrique Llorente --- pkg/vminetworkscontroller/vmi_controller.go | 89 ++++++++++++------- .../vmi_controller_test.go | 85 +++++++++++++----- 2 files changed, 120 insertions(+), 54 deletions(-) diff --git a/pkg/vminetworkscontroller/vmi_controller.go b/pkg/vminetworkscontroller/vmi_controller.go index 3681b4f3..6069f060 100644 --- a/pkg/vminetworkscontroller/vmi_controller.go +++ b/pkg/vminetworkscontroller/vmi_controller.go @@ -27,6 +27,7 @@ import ( "github.com/kubevirt/ipam-extensions/pkg/claims" "github.com/kubevirt/ipam-extensions/pkg/config" + "github.com/kubevirt/ipam-extensions/pkg/udn" ) // VirtualMachineInstanceReconciler reconciles a VirtualMachineInstance object @@ -84,7 +85,7 @@ func (r *VirtualMachineInstanceReconciler) Reconcile( ownerInfo := ownerReferenceFor(vmi, vm) for logicalNetworkName, netConfigName := range vmiNetworks { - claimKey := fmt.Sprintf("%s.%s", vmi.Name, logicalNetworkName) + claimKey := claims.ComposeKey(vmi.Name, logicalNetworkName) ipamClaim := &ipamclaimsapi.IPAMClaim{ ObjectMeta: controllerruntime.ObjectMeta{ Name: claimKey, @@ -158,44 +159,68 @@ func (r *VirtualMachineInstanceReconciler) vmiNetworksClaimingIPAM( ) (map[string]string, error) { vmiNets := make(map[string]string) for _, net := range vmi.Spec.Networks { - if net.Pod != nil { - continue - } - if net.Multus != nil && !net.Multus.Default { - nadName := net.Multus.NetworkName - namespace := vmi.Namespace - namespaceAndName := strings.Split(nadName, "/") - if len(namespaceAndName) == 2 { - namespace = namespaceAndName[0] - nadName = namespaceAndName[1] + if err := r.ensureVMINetworksWithSecondaryUDN(ctx, vmi.Namespace, net, vmiNets); err != nil { + return nil, err } - - contextWithTimeout, cancel := context.WithTimeout(ctx, time.Second) - defer cancel() - nad := &nadv1.NetworkAttachmentDefinition{} - if err := r.Client.Get( - contextWithTimeout, - apitypes.NamespacedName{Namespace: namespace, Name: nadName}, - nad, - ); err != nil { - if apierrors.IsNotFound(err) { - return nil, err - } + } else if net.Pod != nil { + if err := r.ensureVMINetworksWithPrimaryUDN(ctx, vmi.Namespace, net, vmiNets); err != nil { + return nil, err } + } + } - nadConfig, err := config.NewConfig(nad.Spec.Config) - if err != nil { - r.Log.Error(err, "failed extracting the relevant NAD configuration", "NAD name", nadName) - return nil, fmt.Errorf("failed to extract the relevant NAD information") - } + return vmiNets, nil +} - if nadConfig.AllowPersistentIPs { - vmiNets[net.Name] = nadConfig.Name - } +func (r *VirtualMachineInstanceReconciler) ensureVMINetworksWithSecondaryUDN(ctx context.Context, + namespace string, network virtv1.Network, vmiNets map[string]string) error { + nadName := network.Multus.NetworkName + namespaceAndName := strings.Split(nadName, "/") + if len(namespaceAndName) == 2 { + namespace = namespaceAndName[0] + nadName = namespaceAndName[1] + } + + contextWithTimeout, cancel := context.WithTimeout(ctx, time.Second) + defer cancel() + nad := &nadv1.NetworkAttachmentDefinition{} + if err := r.Client.Get( + contextWithTimeout, + apitypes.NamespacedName{Namespace: namespace, Name: nadName}, + nad, + ); err != nil { + if apierrors.IsNotFound(err) { + return err } } - return vmiNets, nil + return r.ensureVMINetworkWithUDN(network, nad, vmiNets) +} + +func (r *VirtualMachineInstanceReconciler) ensureVMINetworksWithPrimaryUDN(ctx context.Context, + namespace string, network virtv1.Network, vmiNets map[string]string) error { + primaryNetworkNAD, err := udn.FindPrimaryNetwork(ctx, r.Client, namespace) + if err != nil { + return err + } + if primaryNetworkNAD == nil { + return nil + } + return r.ensureVMINetworkWithUDN(network, primaryNetworkNAD, vmiNets) +} + +func (r *VirtualMachineInstanceReconciler) ensureVMINetworkWithUDN(network virtv1.Network, + nad *nadv1.NetworkAttachmentDefinition, vmiNets map[string]string) error { + nadConfig, err := config.NewConfig(nad.Spec.Config) + if err != nil { + r.Log.Error(err, "failed extracting the relevant NAD configuration", "NAD name", nad.Name) + return fmt.Errorf("failed to extract the relevant NAD information") + } + + if nadConfig.AllowPersistentIPs { + vmiNets[network.Name] = nadConfig.Name + } + return nil } func shouldCleanFinalizers(vmi *virtv1.VirtualMachineInstance, vm *virtv1.VirtualMachine) bool { diff --git a/pkg/vminetworkscontroller/vmi_controller_test.go b/pkg/vminetworkscontroller/vmi_controller_test.go index d9af04c2..35ffc2ea 100644 --- a/pkg/vminetworkscontroller/vmi_controller_test.go +++ b/pkg/vminetworkscontroller/vmi_controller_test.go @@ -45,7 +45,7 @@ var ( type testConfig struct { inputVM *virtv1.VirtualMachine inputVMI *virtv1.VirtualMachineInstance - inputNAD *nadv1.NetworkAttachmentDefinition + inputNADs []*nadv1.NetworkAttachmentDefinition existingIPAMClaim *ipamclaimsapi.IPAMClaim expectedError error expectedResponse reconcile.Result @@ -94,8 +94,8 @@ var _ = Describe("VMI IPAM controller", Serial, func() { initialObjects = append(initialObjects, config.inputVMI) } - if config.inputNAD != nil { - initialObjects = append(initialObjects, config.inputNAD) + for _, nad := range config.inputNADs { + initialObjects = append(initialObjects, nad) } if config.existingIPAMClaim != nil { @@ -140,10 +140,13 @@ var _ = Describe("VMI IPAM controller", Serial, func() { Expect(ipamClaimsCleaner(ipamClaimList.Items...)).To(ConsistOf(config.expectedIPAMClaims)) } }, - Entry("when the VM has an associated VMI pointing to an existing NAD", testConfig{ - inputVM: dummyVM(dummyVMISpec(nadName)), - inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + Entry("when the VM has an associated VMI pointing to an existing NAD with a primary network at namespace", testConfig{ + inputVM: dummyVM(dummyVMISpec(nadName)), + inputVMI: dummyVMI(dummyVMISpec(nadName)), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + dummyPrimaryNetworkNAD(nadName), + }, expectedResponse: reconcile.Result{}, expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{ { @@ -162,19 +165,39 @@ var _ = Describe("VMI IPAM controller", Serial, func() { }, Spec: ipamclaimsapi.IPAMClaimSpec{Network: "goodnet"}, }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s.%s", vmName, "podnet"), + Namespace: namespace, + Finalizers: []string{claims.KubevirtVMFinalizer}, + Labels: claims.OwnedByVMLabel(vmName), + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "kubevirt.io/v1", + Kind: "VirtualMachine", + Name: vmName, + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true)}, + }, + }, + Spec: ipamclaimsapi.IPAMClaimSpec{Network: "primarynet"}, + }, }, }), Entry("when the VM has an associated VMI pointing to an existing NAD but as multus default network", testConfig{ - inputVM: dummyVM(dummyVMIWithMultusDefaultNetworkSpec(nadName)), - inputVMI: dummyVMI(dummyVMIWithMultusDefaultNetworkSpec(nadName)), - inputNAD: dummyNAD(nadName), + inputVM: dummyVM(dummyVMIWithMultusDefaultNetworkSpec(nadName)), + inputVMI: dummyVMI(dummyVMIWithMultusDefaultNetworkSpec(nadName)), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, expectedResponse: reconcile.Result{}, expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{}, }), Entry("when the VM has an associated VMI pointing to an existing NAD with an improper config", testConfig{ - inputVM: dummyVM(dummyVMISpec(nadName)), - inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNADWrongFormat(nadName), + inputVM: dummyVM(dummyVMISpec(nadName)), + inputVMI: dummyVMI(dummyVMISpec(nadName)), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNADWrongFormat(nadName), + }, expectedError: fmt.Errorf("failed to extract the relevant NAD information"), }), Entry("the associated VMI exists but points to a NAD that doesn't exist", testConfig{ @@ -267,8 +290,10 @@ var _ = Describe("VMI IPAM controller", Serial, func() { }, }), Entry("standalone VMI which is marked for deletion, with active pods, should keep IPAMClaims finalizers", testConfig{ - inputVMI: dummyMarkedForDeletionVMIWithActivePods(nadName), - inputNAD: dummyNAD(nadName), + inputVMI: dummyMarkedForDeletionVMIWithActivePods(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, expectedResponse: reconcile.Result{}, existingIPAMClaim: &ipamclaimsapi.IPAMClaim{ ObjectMeta: metav1.ObjectMeta{ @@ -331,7 +356,9 @@ var _ = Describe("VMI IPAM controller", Serial, func() { Entry("everything is OK but there's already an IPAMClaim with this name", testConfig{ inputVM: dummyVM(dummyVMISpec(nadName)), inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, existingIPAMClaim: &ipamclaimsapi.IPAMClaim{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s.%s", vmName, "randomnet"), @@ -344,7 +371,9 @@ var _ = Describe("VMI IPAM controller", Serial, func() { Entry("found an existing IPAMClaim for the same VM", testConfig{ inputVM: decorateVMWithUID(dummyUID, dummyVM(dummyVMISpec(nadName))), inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, existingIPAMClaim: &ipamclaimsapi.IPAMClaim{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s.%s", vmName, "randomnet"), @@ -386,7 +415,9 @@ var _ = Describe("VMI IPAM controller", Serial, func() { Entry("found an existing IPAMClaim for a VM with same name but different UID", testConfig{ inputVM: decorateVMWithUID(dummyUID, dummyVM(dummyVMISpec(nadName))), inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, existingIPAMClaim: &ipamclaimsapi.IPAMClaim{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("%s.%s", vmName, "randomnet"), @@ -407,8 +438,10 @@ var _ = Describe("VMI IPAM controller", Serial, func() { expectedError: fmt.Errorf("failed since it found an existing IPAMClaim for \"vm1.randomnet\""), }), Entry("a lonesome VMI (with no corresponding VM) is a valid migration use-case", testConfig{ - inputVMI: dummyVMI(dummyVMISpec(nadName)), - inputNAD: dummyNAD(nadName), + inputVMI: dummyVMI(dummyVMISpec(nadName)), + inputNADs: []*nadv1.NetworkAttachmentDefinition{ + dummyNAD(nadName), + }, expectedResponse: reconcile.Result{}, expectedIPAMClaims: []ipamclaimsapi.IPAMClaim{ { @@ -499,7 +532,7 @@ func dummyVMIWithMultusDefaultNetworkSpec(nadName string) virtv1.VirtualMachineI } } -func dummyNAD(nadName string) *nadv1.NetworkAttachmentDefinition { +func dummyNADWithConfig(nadName string, config string) *nadv1.NetworkAttachmentDefinition { namespaceAndName := strings.Split(nadName, "/") return &nadv1.NetworkAttachmentDefinition{ ObjectMeta: metav1.ObjectMeta{ @@ -507,11 +540,19 @@ func dummyNAD(nadName string) *nadv1.NetworkAttachmentDefinition { Name: namespaceAndName[1], }, Spec: nadv1.NetworkAttachmentDefinitionSpec{ - Config: `{"name": "goodnet", "allowPersistentIPs": true}`, + Config: config, }, } } +func dummyNAD(nadName string) *nadv1.NetworkAttachmentDefinition { + return dummyNADWithConfig(nadName, `{"name": "goodnet", "allowPersistentIPs": true}`) +} + +func dummyPrimaryNetworkNAD(nadName string) *nadv1.NetworkAttachmentDefinition { + return dummyNADWithConfig(nadName+"primary", `{"name": "primarynet", "role": "primary", "allowPersistentIPs": true}`) +} + func dummyNADWrongFormat(nadName string) *nadv1.NetworkAttachmentDefinition { namespaceAndName := strings.Split(nadName, "/") return &nadv1.NetworkAttachmentDefinition{