From 954ed2179ff60d201e602796e7cda343e477e1b1 Mon Sep 17 00:00:00 2001 From: Periyasamy Palanisamy Date: Fri, 13 Sep 2024 10:39:31 +0200 Subject: [PATCH] Fix util.PodNadNames for primary networks The NAD name retrieval in the util.PodNadNames function is currently based on NetworkSelectionElement which doesn't work for primary networks. So enhancing this method to work for primary networks as well. It would benefit in getting podIPs and NAD names associated with pod's user defined primary network. Signed-off-by: Periyasamy Palanisamy --- .../pkg/ovn/base_network_controller.go | 24 +------ .../pkg/ovn/base_network_controller_policy.go | 11 +--- go-controller/pkg/util/pod_annotation.go | 62 ++++++++++--------- 3 files changed, 37 insertions(+), 60 deletions(-) diff --git a/go-controller/pkg/ovn/base_network_controller.go b/go-controller/pkg/ovn/base_network_controller.go index 03d250619e..d73a7f317a 100644 --- a/go-controller/pkg/ovn/base_network_controller.go +++ b/go-controller/pkg/ovn/base_network_controller.go @@ -702,30 +702,12 @@ func (bnc *BaseNetworkController) doesNetworkRequireIPAM() bool { return util.DoesNetworkRequireIPAM(bnc.NetInfo) } -func (bnc *BaseNetworkController) getPodNADNames(pod *kapi.Pod) ([]string, error) { +func (bnc *BaseNetworkController) getPodNADNames(pod *kapi.Pod) []string { if !bnc.IsSecondary() { - return []string{types.DefaultNetworkName}, nil - } - // util.PodNadNames is based on the network selection element so it wouldn't work for primary - // network. So retrieve NAD name from the network which is serving pod's namespace. - if bnc.IsPrimaryNetwork() { - // We couldn't rely on bnc.NetInfo because this may span across different namespaces. - // Hence retrieve active network object from pod namespace and then NADs from this object. - namespacePrimaryNetwork, err := bnc.getActiveNetworkForNamespace(pod.Namespace) - if err != nil { - return nil, err - } - if namespacePrimaryNetwork.GetNetworkName() != bnc.NetInfo.GetNetworkName() { - return nil, fmt.Errorf("pod %s/%s active network %s mismatch", pod.Namespace, pod.Name, namespacePrimaryNetwork.GetNetworkName()) - } - nadList := namespacePrimaryNetwork.GetNADs() - if len(nadList) != 1 { - return nil, fmt.Errorf("expected one NAD in %s network, got: %d: %v", namespacePrimaryNetwork.GetNetworkName(), len(nadList), nadList) - } - return nadList, nil + return []string{types.DefaultNetworkName} } podNadNames, _ := util.PodNadNames(pod, bnc.NetInfo) - return podNadNames, nil + return podNadNames } func (bnc *BaseNetworkController) getClusterPortGroupDbIDs(base string) *libovsdbops.DbObjectIDs { diff --git a/go-controller/pkg/ovn/base_network_controller_policy.go b/go-controller/pkg/ovn/base_network_controller_policy.go index ea78d68050..f8ab8e53c3 100644 --- a/go-controller/pkg/ovn/base_network_controller_policy.go +++ b/go-controller/pkg/ovn/base_network_controller_policy.go @@ -597,10 +597,7 @@ func (bnc *BaseNetworkController) getNewLocalPolicyPorts(np *networkPolicy, continue } - nadNames, err := bnc.getPodNADNames(pod) - if err != nil { - errs = append(errs, fmt.Errorf("unable to get NAD names for pod %s/%s: %v", pod.Namespace, pod.Name, err)) - } + nadNames := bnc.getPodNADNames(pod) for _, nadName := range nadNames { logicalPortName := bnc.GetLogicalPortName(pod, nadName) if _, ok := np.localPods.Load(logicalPortName); ok { @@ -649,11 +646,7 @@ func (bnc *BaseNetworkController) getExistingLocalPolicyPorts(np *networkPolicy, for _, obj := range objs { pod := obj.(*kapi.Pod) - var nadNames []string - nadNames, err = bnc.getPodNADNames(pod) - if err != nil { - return - } + nadNames := bnc.getPodNADNames(pod) for _, nadName := range nadNames { logicalPortName := bnc.GetLogicalPortName(pod, nadName) loadedPortUUID, ok := np.localPods.Load(logicalPortName) diff --git a/go-controller/pkg/util/pod_annotation.go b/go-controller/pkg/util/pod_annotation.go index 478123e6a7..48ca49f00a 100644 --- a/go-controller/pkg/util/pod_annotation.go +++ b/go-controller/pkg/util/pod_annotation.go @@ -14,6 +14,7 @@ import ( v1 "k8s.io/api/core/v1" listers "k8s.io/client-go/listers/core/v1" + "k8s.io/klog/v2" utilnet "k8s.io/utils/net" ) @@ -303,38 +304,14 @@ func GetPodCIDRsWithFullMask(pod *v1.Pod, nInfo NetInfo) ([]*net.IPNet, error) { return ips, nil } -// GetPodIPsOfNetwork returns the pod's IP addresses. -// If the network is default type, then retrieve it from Pod Status IPs. -// If the network is secondary and also used as primary network for the pod, then retrieve -// IPs directly from "k8s.ovn.org/pod-networks" pod annotation. -// If the network is only secondary, then fetch pod NAD name using NetworkSelectionElement -// and then retrieve IPs from k8s.ovn.org/pod-networks annotation for the network. -// This function is intended to also return IPs for HostNetwork and other non-OVN-IPAM-ed pods. +// GetPodIPsOfNetwork returns the pod's IP addresses, first from the OVN annotation +// and then falling back to the Pod Status IPs. This function is intended to +// also return IPs for HostNetwork and other non-OVN-IPAM-ed pods. func GetPodIPsOfNetwork(pod *v1.Pod, nInfo NetInfo) ([]net.IP, error) { - if !nInfo.IsSecondary() || PodWantsHostNetwork(pod) { - return DefaultNetworkPodIPs(pod) - } - if nInfo.IsPrimaryNetwork() { - // The nInfo might belongs to a network which may be serving multiple namespaces. - // Hence find out the NAD associated with pod namespace and retrieve IPs from it. - nadList := nInfo.GetNADs() - for _, nad := range nadList { - userNet, err := UnmarshalPodAnnotation(pod.Annotations, nad) - if err != nil && IsAnnotationNotSetError(err) { - // NAD belongs to same network, but serving different namespace. so skip processing it. - continue - } else if err != nil { - return nil, err - } - ips := make([]net.IP, 0, len(userNet.IPs)) - for _, netIP := range userNet.IPs { - ips = append(ips, netIP.IP) - } - return ips, nil - } - return []net.IP{}, nil + if nInfo.IsSecondary() { + return SecondaryNetworkPodIPs(pod, nInfo) } - return SecondaryNetworkPodIPs(pod, nInfo) + return DefaultNetworkPodIPs(pod) } func DefaultNetworkPodIPs(pod *v1.Pod) ([]net.IP, error) { @@ -380,7 +357,32 @@ func SecondaryNetworkPodIPs(pod *v1.Pod, networkInfo NetInfo) ([]net.IP, error) return ips, nil } +func GetPrimaryNetworkNADNameForPodFromNetInfo(pod *v1.Pod, netinfo NetInfo) (string, error) { + podNetworks, err := UnmarshalPodAnnotationAllNetworks(pod.Annotations) + if err != nil { + return "", err + } + for _, nadName := range netinfo.GetNADs() { + if podNetwork, ok := podNetworks[nadName]; ok && podNetwork.Role == types.NetworkRolePrimary { + return nadName, nil + } + } + klog.Warningf("NAD is not found on the pod %s/%s for the network %s", pod.Namespace, pod.Name, netinfo.GetNetworkName()) + return "", nil +} + func PodNadNames(pod *v1.Pod, netinfo NetInfo) ([]string, error) { + // When passed netInfo is a primary network, then retrieve NAD name from network which + // is serving pod's namespace. We can't directly rely on NAD name from passed netInfo + // object as it may span across different namespaces. Hence retrieve active network + // object from pod's namespace and then retrieve NAD names from this object. + if netinfo.IsPrimaryNetwork() { + nadName, err := GetPrimaryNetworkNADNameForPodFromNetInfo(pod, netinfo) + if err != nil { + return nil, err + } + return []string{nadName}, nil + } on, networkMap, err := GetPodNADToNetworkMapping(pod, netinfo) // skip pods that are not on this network if err != nil {