From fc5d3df56c3daa3a1157724fe0deead5ec9a3b5d Mon Sep 17 00:00:00 2001 From: jordigilh Date: Thu, 8 Jun 2023 13:49:57 -0400 Subject: [PATCH 1/6] APB Controllers: use single loop level driven model The controllers for APB rely on pod, namespace, and APB CRD events. Previously each handler would do some configuration. This changes the model to use the single loop level driven design. Now: - The namespace handler will search for any policies that apply to it and queue them to the APB handler. - The pod handler will search for if any policies use this pod as a pod GW and then queue them to the APB handler. - APB handler modified to use keys in its workqueue rather than objects. Objects are fetched at sync time for the most current version. - Legacy egress gw code still handles wiring served pods based on a shared cache by APB controller. Co-authored-by: jordigilh Signed-off-by: jordigilh Signed-off-by: Tim Rozet --- .../apbroute/external_controller.go | 66 +-- .../apbroute/external_controller_namespace.go | 150 ++---- .../external_controller_namespace_test.go | 21 +- .../apbroute/external_controller_pod.go | 411 ++-------------- .../apbroute/external_controller_pod_test.go | 16 +- .../apbroute/external_controller_policy.go | 448 +++++++++++++----- .../external_controller_policy_test.go | 102 +++- .../apbroute/external_controller_test.go | 1 - .../controller/apbroute/master_controller.go | 198 ++------ .../controller/apbroute/node_controller.go | 141 +----- 10 files changed, 606 insertions(+), 948 deletions(-) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller.go b/go-controller/pkg/ovn/controller/apbroute/external_controller.go index 96db4767356..d1693c47f60 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller.go @@ -2,6 +2,7 @@ package apbroute import ( "fmt" + "strings" "sync" adminpolicybasedrouteapi "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1" @@ -17,6 +18,14 @@ import ( type gatewayInfoList []*gatewayInfo +func (g gatewayInfoList) String() string { + ret := []string{} + for _, i := range g { + ret = append(ret, i.String()) + } + return strings.Join(ret, ", ") +} + // HasBFDEnabled returns the BFD value for the given IP stored in the gatewayInfoList when found. // It also returns a boolean that indicates if the IP was found and an error // An IP can only have one BFD value. @@ -82,6 +91,10 @@ type gatewayInfo struct { BFDEnabled bool } +func (g *gatewayInfo) String() string { + return fmt.Sprintf("BFDEnabled: %t, Gateways: %+v", g.BFDEnabled, g.Gateways.items) +} + func newGatewayInfo(items sets.Set[string], bfdEnabled bool) *gatewayInfo { return &gatewayInfo{Gateways: &syncSet{items: items, mux: &sync.Mutex{}}, BFDEnabled: bfdEnabled} } @@ -93,16 +106,7 @@ type syncSet struct { // Equal compares two gatewayInfo instances and returns true if all the gateway IPs are equal, regardless of the order, as well as the BFDEnabled field value. func (g *gatewayInfo) Equal(g2 *gatewayInfo) bool { - return g.BFDEnabled == g2.BFDEnabled && g.Gateways.Equal(g2.Gateways) -} - -func (g *syncSet) Equal(s2 *syncSet) bool { - s2.mux.Lock() - s2items := s2.items.Clone() - s2.mux.Unlock() - g.mux.Lock() - defer g.mux.Unlock() - return g.items.Equal(s2items) + return g.BFDEnabled == g2.BFDEnabled && len(g.Gateways.Difference(g2.Gateways.items.Clone())) == 0 } func (g *syncSet) Has(ip string) bool { @@ -145,7 +149,6 @@ type namespaceInfo struct { Policies sets.Set[string] StaticGateways gatewayInfoList DynamicGateways map[ktypes.NamespacedName]*gatewayInfo - markForDelete bool } func newNamespaceInfo() *namespaceInfo { @@ -157,8 +160,8 @@ func newNamespaceInfo() *namespaceInfo { } type routeInfo struct { - policy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute - markedForDelete bool + policy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute + markedForDeletion bool } type ExternalRouteInfo struct { @@ -222,19 +225,20 @@ func newExternalPolicyManager( } // getRoutePolicyFromCache retrieves the cached value of the policy if it exists in the cache, as well as locking the key in case it exists. -func (m *externalPolicyManager) getRoutePolicyFromCache(policyName string) (adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, bool, bool) { +func (m *externalPolicyManager) getRoutePolicyFromCache(policyName string) (*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, bool, bool) { var ( - policy adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute + policy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute found, markedForDeletion bool ) _ = m.routePolicySyncCache.DoWithLock(policyName, func(policyName string) error { + klog.Infof("Getting route %s", policyName) ri, f := m.routePolicySyncCache.Load(policyName) if !f { return nil } found = f - policy = *ri.policy - markedForDeletion = ri.markedForDelete + markedForDeletion = ri.markedForDeletion + policy = ri.policy.DeepCopy() return nil }) return policy, found, markedForDeletion @@ -247,7 +251,7 @@ func (m *externalPolicyManager) storeRoutePolicyInCache(policyInfo *adminpolicyb m.routePolicySyncCache.LoadOrStore(policyName, &routeInfo{policy: policyInfo}) return nil } - if ri.markedForDelete { + if ri.markedForDeletion { return fmt.Errorf("attempting to store policy %s that is in the process of being deleted", policyInfo.Name) } ri.policy = policyInfo @@ -258,7 +262,7 @@ func (m *externalPolicyManager) storeRoutePolicyInCache(policyInfo *adminpolicyb func (m *externalPolicyManager) deleteRoutePolicyFromCache(policyName string) error { return m.routePolicySyncCache.DoWithLock(policyName, func(policyName string) error { ri, found := m.routePolicySyncCache.Load(policyName) - if found && !ri.markedForDelete { + if found && !ri.markedForDeletion { return fmt.Errorf("attempting to delete route policy %s from cache before it has been marked for deletion", policyName) } m.routePolicySyncCache.Delete(policyName) @@ -278,7 +282,7 @@ func (m *externalPolicyManager) getAndMarkRoutePolicyForDeletionInCache(policyNa if !found { return nil } - ri.markedForDelete = true + ri.markedForDeletion = true exists = true routePolicy = *ri.policy return nil @@ -296,26 +300,8 @@ func (m *externalPolicyManager) getNamespaceInfoFromCache(namespaceName string) return nsInfo, true } -func (m *externalPolicyManager) getAndMarkForDeleteNamespaceInfoFromCache(namespaceName string) (*namespaceInfo, bool) { - nsInfo, ok := m.getNamespaceInfoFromCache(namespaceName) - if !ok { - return nil, false - } - nsInfo.markForDelete = true - return nsInfo, ok -} - -func (m *externalPolicyManager) deleteNamespaceInfoInCache(namespaceName string) { - nsInfo, ok := m.namespaceInfoSyncCache.Load(namespaceName) - if !ok { - klog.Warningf("Failed to retrieve namespace %s for deletion", namespaceName) - return - } - if !nsInfo.markForDelete { - klog.Warningf("Attempting to delete namespace %s when it has not been marked for deletion", namespaceName) - return - } - m.namespaceInfoSyncCache.Delete(namespaceName) +func (m *externalPolicyManager) getAllNamespacesNamesInCache() []string { + return m.namespaceInfoSyncCache.GetKeys() } func (m *externalPolicyManager) unlockNamespaceInfoCache(namespaceName string) { diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go index 04e067faf16..4bdf831480a 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go @@ -1,90 +1,62 @@ package apbroute import ( - "fmt" - adminpolicybasedrouteapi "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - ktypes "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" ) -// processAddNamespace takes in a namespace and applies the policies that are applicable to the namespace, previously stored in the cacheInfo object argument. -// The logic goes through all the policies and applies the gateway IPs derived from the static and dynamic hop to all the pods in the namespace. -// Lastly, it updates the cacheInfo to contain the static and dynamic gateway IPs generated from the previous action to keep track of the gateway IPs applied in the namespace. -func (m *externalPolicyManager) processAddNamespace(new *v1.Namespace, cacheInfo *namespaceInfo) error { - staticGateways, dynamicGateways, err := m.aggregateNamespaceInfo(cacheInfo.Policies) - if err != nil { - return err - } - cacheInfo.StaticGateways = staticGateways - cacheInfo.DynamicGateways = dynamicGateways - return nil -} - -// processDeleteNamespace processes a delete namespace event by ensuring that no pod is still running in that namespace before deleting the namespace info cache. It also -// marks the namespace for deletion so that if a new pod event appears targeting the namespace, the operation will be rejected. -func (m *externalPolicyManager) processDeleteNamespace(namespaceName string) error { - _, found := m.getAndMarkForDeleteNamespaceInfoFromCache(namespaceName) - if !found { - // namespace is not a recipient for policies - return nil - } - defer m.unlockNamespaceInfoCache(namespaceName) - podsInNs, err := m.podLister.Pods(namespaceName).List(labels.Everything()) - if err != nil { - return err - } - if len(podsInNs) != 0 { - klog.Infof("Attempting to delete namespace %s with resources still attached to it. Retrying...", namespaceName) - return fmt.Errorf("unable to delete namespace %s with resources still attached to it", namespaceName) - } - m.deleteNamespaceInfoInCache(namespaceName) - return nil -} - -// processUpdateNamespace takes in a namespace name, current policies applied to the namespace, policies that are now expected to be applied to the namespace and the cache info -// that contains all the current gateway IPs and policies for that namespace. It follows this logic: -// * Calculate the difference between current and expected policies and proceed to remove the gateway IPs from the policies that are no longer applicable to this namespace -// * Calculate the difference between the expected and current ones to determine the new policies to be applied and proceed to apply them. -// * Update the cache info with the new list of policies, as well as the static and dynamic gateway IPs derived from executing the previous logic. -func (m *externalPolicyManager) processUpdateNamespace(namespaceName string, currentPolicies, newPolicies sets.Set[string], cacheInfo *namespaceInfo) error { - - // some differences apply, let's figure out if previous policies have been removed first - policiesNotValid := currentPolicies.Difference(newPolicies) - // iterate through the policies that no longer apply to this namespace - for policyName := range policiesNotValid { - err := m.removePolicyFromNamespaceWithName(namespaceName, policyName, cacheInfo) +func (m *externalPolicyManager) syncNamespace(namespace *v1.Namespace, routeQueue workqueue.RateLimitingInterface) error { + klog.V(5).Infof("APB processing namespace: %s", namespace) + + keysToBeQueued := sets.New[string]() + + // get a copy of all policies at this time and see if they include this namespace + policyKeys := m.routePolicySyncCache.GetKeys() + + for _, policyName := range policyKeys { + err := m.routePolicySyncCache.DoWithLock(policyName, + func(key string) error { + ri, found := m.routePolicySyncCache.Load(policyName) + if found { + targetNsList, err := m.listNamespacesBySelector(&ri.policy.Spec.From.NamespaceSelector) + if err != nil { + return err + } + for _, targetNS := range targetNsList { + if targetNS.Name == namespace.Name { + keysToBeQueued.Insert(policyName) + } + } + } + return nil + }) if err != nil { return err } } - // policies that now apply to this namespace - newPoliciesDiff := newPolicies.Difference(currentPolicies) - for policyName := range newPoliciesDiff { - policy, found, markedForDeletion := m.getRoutePolicyFromCache(policyName) - if !found { - return fmt.Errorf("failed to find external route policy %s in cache", policyName) - } - if markedForDeletion { - klog.Infof("Skipping route policy %s as it has been marked for deletion", policyName) - continue - } - err := m.applyPolicyToNamespace(namespaceName, &policy, cacheInfo) - if err != nil { - return err + // check if this namespace is being tracked by policy in its namespace cache + cacheInfo, found := m.getNamespaceInfoFromCache(namespace.Name) + if found { + for policyName := range cacheInfo.Policies { + keysToBeQueued.Insert(policyName) } + m.unlockNamespaceInfoCache(namespace.Name) + } + + for policyKey := range keysToBeQueued { + klog.V(5).Infof("APB queuing policy: %s for namespace: %s", policyKey, namespace) + routeQueue.Add(policyKey) } - // at least one policy apply, let's update the cache - cacheInfo.Policies = newPolicies - return nil + return nil } +// must be called with lock on namespaceInfo cache func (m *externalPolicyManager) applyPolicyToNamespace(namespaceName string, policy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, cacheInfo *namespaceInfo) error { processedPolicy, err := m.processExternalRoutePolicy(policy) @@ -98,15 +70,8 @@ func (m *externalPolicyManager) applyPolicyToNamespace(namespaceName string, pol return nil } -func (m *externalPolicyManager) removePolicyFromNamespaceWithName(targetNamespace, policyName string, cacheInfo *namespaceInfo) error { - policy, err := m.routeLister.Get(policyName) - if err != nil { - return err - } - return m.removePolicyFromNamespace(targetNamespace, policy, cacheInfo) -} +// must be called with lock on namespaceInfo cache func (m *externalPolicyManager) removePolicyFromNamespace(targetNamespace string, policy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, cacheInfo *namespaceInfo) error { - processedPolicy, err := m.processExternalRoutePolicy(policy) if err != nil { return err @@ -115,7 +80,13 @@ func (m *externalPolicyManager) removePolicyFromNamespace(targetNamespace string if err != nil { return err } + + klog.Infof("Deleting APB policy %s in namespace cache %s", policy.Name, targetNamespace) cacheInfo.Policies = cacheInfo.Policies.Delete(policy.Name) + if len(cacheInfo.Policies) == 0 { + m.namespaceInfoSyncCache.Delete(targetNamespace) + } + return nil } @@ -131,32 +102,3 @@ func (m *externalPolicyManager) listNamespacesBySelector(selector *metav1.LabelS return ns, nil } - -func (m *externalPolicyManager) aggregateNamespaceInfo(policies sets.Set[string]) (gatewayInfoList, map[ktypes.NamespacedName]*gatewayInfo, error) { - - static := gatewayInfoList{} - dynamic := make(map[ktypes.NamespacedName]*gatewayInfo) - for policyName := range policies { - externalPolicy, err := m.routeLister.Get(policyName) - if err != nil { - klog.Warningf("Unable to find route policy %s:%+v", policyName, err) - continue - } - processedPolicy, err := m.processExternalRoutePolicy(externalPolicy) - if err != nil { - return nil, nil, err - } - var duplicated sets.Set[string] - static, duplicated, err = static.Insert(processedPolicy.staticGateways...) - if err != nil { - return nil, nil, err - } - if duplicated.Len() > 0 { - klog.Warningf("Found duplicated gateway IP(s) %+s in policy(s) %+s", sets.List(duplicated), sets.List(policies)) - } - for podName, gatewayInfo := range processedPolicy.dynamicGateways { - dynamic[podName] = gatewayInfo - } - } - return static, dynamic, nil -} diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace_test.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace_test.go index 73ceb65a6a9..2cc55b3a962 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace_test.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace_test.go @@ -100,7 +100,6 @@ func listNamespaceInfo() []string { func deepCopyNamespaceInfo(source, destination *namespaceInfo) { destination.Policies = sets.New(source.Policies.UnsortedList()...) destination.StaticGateways = gatewayInfoList{} - destination.markForDelete = source.markForDelete for _, gwInfo := range source.StaticGateways { destination.StaticGateways, _, err = destination.StaticGateways.Insert(&gatewayInfo{ Gateways: &syncSet{ @@ -311,7 +310,6 @@ var _ = Describe("OVN External Gateway namespace", func() { Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) Eventually(func() bool { return len(getNamespaceInfo(namespaceTest.Name).DynamicGateways) > 0 }, 5).Should(BeTrue()) deleteNamespace(namespaceTest.Name, fakeClient) - Eventually(func() bool { return getNamespaceInfo(namespaceTest.Name).markForDelete }, 5).Should(BeTrue()) deletePod(targetPod, fakeClient) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(0)) }) @@ -324,26 +322,20 @@ var _ = Describe("OVN External Gateway namespace", func() { } initController([]runtime.Object{namespaceDefault, namespaceTest, targetPod, podGW}, []runtime.Object{dynamicPolicy}) - + createNamespace(namespaceTest2) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) Eventually(func() bool { return len(getNamespaceInfo(namespaceTest.Name).DynamicGateways) > 0 }, 5).Should(BeTrue()) By("delete the namespace whilst a pod still remains") deleteNamespace(namespaceTest.Name, fakeClient) - Eventually(func() bool { return getNamespaceInfo(namespaceTest.Name).markForDelete }, 5).Should(BeTrue()) By("create the namespace test again while it is being deleted") createNamespace(namespaceTest) By("delete the remaining pod in the namespace to proceed on deleting the namespace itself") deletePod(targetPod, fakeClient) - Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) - // The new namespace should not be marked for deletion and should contain the same dynamic gateways - Eventually(func() bool { + Eventually(func() []string { return listNamespaceInfo() }, time.Hour).Should(HaveLen(1)) + Eventually(func() int { nsInfo := getNamespaceInfo(namespaceTest.Name) - if nsInfo != nil { - return nsInfo.markForDelete - } - return true - }, time.Hour).Should(BeFalse()) - Eventually(func() bool { return len(getNamespaceInfo(namespaceTest.Name).DynamicGateways) > 0 }, time.Hour).Should(BeTrue()) + return len(nsInfo.DynamicGateways) + }, 5).Should(Equal(1)) }) }) @@ -385,8 +377,7 @@ var _ = Describe("OVN External Gateway namespace", func() { DynamicGateways: make(map[ktypes.NamespacedName]*gatewayInfo)}, cmpOpts...)) updateNamespaceLabel(namespaceTest.Name, dynamicPolicyTest2.Spec.From.NamespaceSelector.MatchLabels, fakeClient) - Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal(newNamespaceInfo())) + Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(0)) }) It("validates that a namespace changes its policies when its labels are changed to match a different policy, resulting in the later on being the only policy applied to the namespace", func() { diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go index c122f53f602..8db67e57490 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go @@ -4,362 +4,41 @@ import ( "encoding/json" "fmt" "net" - "strings" nettypes "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" adminpolicybasedrouteapi "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1" v1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/labels" ktypes "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + corev1listers "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" utilnet "k8s.io/utils/net" ) -// processAddPod covers 2 scenarios: -// 1) The pod is an external gateway, in which case it needs to propagate its IP to a set of pods in the cluster. -// Determining which namespaces to update is determined by matching the pod's namespace and label selector against -// all the existing Admin Policy Based External route CRs. It's a reverse lookup: -// -// pod GW -> dynamic hop -> APB External Route CR -> target namespaces (label selector in the CR's `From`` field) -> pods in namespace -// -// 2) The pod belongs to a namespace impacted by at least one APB External Route CR, in which case its logical routes need to be -// updated to reflect the external routes. -// -// A pod can only be either an external gateway or a consumer of an external route policy. -func (m *externalPolicyManager) processAddPod(newPod *v1.Pod) error { +func (m *externalPolicyManager) syncPod(pod *v1.Pod, podLister corev1listers.PodLister, routeQueue workqueue.RateLimitingInterface) error { - // the pod can either be a gateway pod or a standard pod that requires no processing from the external controller. - // to determine either way, find out which matching dynamic hops include this pod. If none applies, then this is - // a standard pod and all is needed is to update it's logical routes to include all the external gateways, if they exist. - podPolicies, err := m.findMatchingDynamicPolicies(newPod) - if err != nil { - return err - } - if len(podPolicies) > 0 { - // this is a gateway pod - pnames := []string{} - for _, p := range podPolicies { - pnames = append(pnames, p.Name) - } - klog.Infof("Adding pod gateway %s/%s for policy %s", newPod.Namespace, newPod.Name, strings.Join(pnames, ",")) - return m.applyPodGWPolicies(newPod, podPolicies) - } - cacheInfo, found := m.getNamespaceInfoFromCache(newPod.Namespace) - if !found || (found && cacheInfo.Policies.Len() == 0) { - // this is a standard pod and there are no external gateway policies applicable to the pod's namespace. Nothing to do - if !found { - return nil - } - m.unlockNamespaceInfoCache(newPod.Namespace) - return nil - } - defer m.unlockNamespaceInfoCache(newPod.Namespace) - if cacheInfo.markForDelete { - klog.Infof("Attempting to add a pod to namespace %s when it has been marked for deletion", newPod.Namespace) - return fmt.Errorf("failed to add external gateways to pod %[1]s/%[2]s when namespace %[1]s has been marked for deletion", newPod.Namespace, newPod.Name) - } - // there are external gateway policies applicable to the pod's namespace. - klog.Infof("Applying policies to new pod %s/%s %+v", newPod.Namespace, newPod.Name, cacheInfo.Policies) - return m.applyGatewayInfoToPod(newPod, cacheInfo.StaticGateways, cacheInfo.DynamicGateways) -} - -func (m *externalPolicyManager) applyPodGWPolicies(pod *v1.Pod, externalRoutePolicies []*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { - for _, erp := range externalRoutePolicies { - err := m.applyPodGWPolicy(pod, erp) - if err != nil { - return err - } - } - return nil -} - -func (m *externalPolicyManager) applyPodGWPolicy(pod *v1.Pod, externalRoutePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { - klog.Infof("Processing policy %s for pod %s/%s", externalRoutePolicy.Name, pod.Namespace, pod.Name) - routePolicy, err := m.getRoutePolicyForPodGateway(pod, externalRoutePolicy) - if err != nil { - return err - } - // update all namespaces targeted by this pod's policy to include the new pod IP as their external GW - err = m.applyProcessedPolicy(externalRoutePolicy.Name, routePolicy) - if err != nil { - return err - } - gwInfoMap, err := m.aggregateDynamicRouteGatewayInformation(pod, routePolicy) - if err != nil { - return err - } - key := ktypes.NamespacedName{Namespace: pod.Namespace, Name: pod.Name} - // update the namespace information for each targeted namespace to reflect the gateway IPs that handle external traffic - for ns, gwInfo := range gwInfoMap { - cacheInfo, found := m.getNamespaceInfoFromCache(ns) - if !found { - klog.Warningf("Attempting to update the dynamic gateway information for pod %s in a namespace that does not exist %s", pod.Name, ns) - continue - } - // update the gwInfo in the namespace cache - cacheInfo.DynamicGateways[key] = gwInfo - m.unlockNamespaceInfoCache(ns) - if err != nil { - return err - } - } - return nil -} - -func (m *externalPolicyManager) removePodGatewayFromNamespace(nsName string, podNamespacedName ktypes.NamespacedName) error { - // retrieve the gateway information from the impacted namespace's cache - cacheInfo, found := m.getNamespaceInfoFromCache(nsName) - if !found { - klog.Warningf("Attempting to remove pod gateway %s/%s from a namespace that does not exist %s", podNamespacedName.Namespace, podNamespacedName.Name, nsName) - return nil - } - defer m.unlockNamespaceInfoCache(nsName) - - gwInfo, found := cacheInfo.DynamicGateways[podNamespacedName] - if !found { - klog.Warningf("Pod %s/%s not found in dynamic cacheInfo for namespace %s", podNamespacedName.Namespace, podNamespacedName.Name, nsName) - return nil - } - annotatedGWIPs, err := m.calculateAnnotatedNamespaceGatewayIPsForNamespace(nsName) - if err != nil { - return err - } - // it is safe to pass the current policies and not to expect the pod IP in the coexisting list of IPs since the pod will no longer match the dynamic hop selectors in any of the policies - coexistingIPs, err := m.retrieveDynamicGatewayIPsForPolicies(cacheInfo.Policies) - if err != nil { - return err - } - coexistingIPs = coexistingIPs.Union(annotatedGWIPs) - // Filter out the IPs that are not in coexisting. Those IPs are to be deleted. - invalidGWIPs := gwInfo.Gateways.Difference(coexistingIPs) - // Filter out the IPs from the coexisting list that are to be kept by calculating the difference between the coexising and those IPs that are to be deleted and not coexisting at the same time. - ipsToKeep := coexistingIPs.Difference(invalidGWIPs) - klog.Infof("Coexisting %s, invalid %s, ipsToKeep %s", strings.Join(sets.List(coexistingIPs), ","), strings.Join(sets.List(invalidGWIPs), ","), strings.Join(sets.List(ipsToKeep), ",")) - err = m.netClient.deleteGatewayIPs(nsName, invalidGWIPs, ipsToKeep) - if err != nil { - return err - } - gwInfo.Gateways.Delete(invalidGWIPs.UnsortedList()...) - if len(gwInfo.Gateways.UnsortedList()) == 0 { - // remove pod from namespace cache - delete(cacheInfo.DynamicGateways, podNamespacedName) - } - return nil -} - -func (m *externalPolicyManager) addPodGatewayToNamespace(podNamespacedName ktypes.NamespacedName, namespaceName string, processedPolicies []*routePolicy) error { - // the pod's gatewayInfo is unique to a namespace as the networkName field can differ depending on the policy definition of that field - // so we retrieve the correct one for the given target namespace from the pre-processed policies. It uses - // the target namespace and the key (pod_namespace,pod_name) as keys. - gatewayInfo, err := m.findGatewayInfoForPodInTargetNamespace(podNamespacedName, namespaceName, processedPolicies) - if err != nil { - return err - } - // use the pod's gatewayInfo to update the logical routes for all the pod's in the target namespace - err = m.addGWRoutesForNamespace(namespaceName, gatewayInfoList{gatewayInfo}) - if err != nil { - return err - } - cacheInfo, found := m.getNamespaceInfoFromCache(namespaceName) - defer m.unlockNamespaceInfoCache(namespaceName) - if !found { - cacheInfo = m.newNamespaceInfoInCache(namespaceName) - } - // add pod gateway information to the namespace cache - cacheInfo.DynamicGateways[podNamespacedName] = gatewayInfo - return nil -} - -// processUpdatePod takes in an updated gateway pod and the list of old namespaces where the pod was used as egress gateway and proceeds as follows -// - Finds the matching policies that apply to the pod based on the dynamic hop pod and namespace selectors. If the labels in the pod have not changed, the policies will match to the existing one. -// - Based on the policies that use the pod IP as gateway, determine the namespaces where the pod IP will be used as egress gateway. If the namespaces match, return without error -// - Remove the pod IP as egress gateway from the namespaces that are no longer impacted by the pod. This is determined by calculating the difference between the old namespaces and the new ones based on the policies -// applicable to the updated pod. -// - Add the pod IP as egress gateway to the namespaces that are now being impacted by the changes in the pod. -func (m *externalPolicyManager) processUpdatePod(updatedPod *v1.Pod, oldTargetNs sets.Set[string]) error { - - // find the policies that apply to this new pod. Unless there are changes to the labels, they should be identical. - newPodPolicies, err := m.findMatchingDynamicPolicies(updatedPod) - if err != nil { - return err - } - key := ktypes.NamespacedName{Namespace: updatedPod.Namespace, Name: updatedPod.Name} - // aggregate the expected target namespaces based on the new pod's labels and current policies - // if the labels have not changed, the new targeted namespaces and the old ones should be identical - newTargetNs, err := m.aggregateTargetNamespacesByPolicies(key, newPodPolicies) - if err != nil { - return err - } - if oldTargetNs.Equal(newTargetNs) { - // targeting the same namespaces. Nothing to do - return nil - } - // the pods have changed and they don't target the same sets of namespaces, delete its reference on the ones that don't apply - // and add to the new ones, if necessary - nsToRemove := oldTargetNs.Difference(newTargetNs) - if len(nsToRemove) > 0 { - klog.Infof("Removing pod gateway %s/%s from namespace(s): %s", updatedPod.Namespace, updatedPod.Name, strings.Join(sets.List(nsToRemove), ",")) - } - // retrieve the gateway information for the pod - for ns := range nsToRemove { - err = m.removePodGatewayFromNamespace(ns, ktypes.NamespacedName{Namespace: updatedPod.Namespace, Name: updatedPod.Name}) - if err != nil { - return err - } - } - - // pre-process the policies so we can apply them this process extracts from the CR the contents of the policies - // into an internal structure that contains the static and dynamic hops information. - pp, err := m.processExternalRoutePolicies(newPodPolicies) - if err != nil { - return err - } - - nsToAdd := newTargetNs.Difference(oldTargetNs) - if len(nsToAdd) > 0 { - klog.Infof("Adding pod gateway %s/%s to namespace(s): %s", updatedPod.Namespace, updatedPod.Name, strings.Join(sets.List(nsToAdd), ",")) - } - for ns := range nsToAdd { - err = m.addPodGatewayToNamespace(ktypes.NamespacedName{Namespace: updatedPod.Namespace, Name: updatedPod.Name}, ns, pp) - if err != nil { - return err - } - } - - return nil -} - -func (m *externalPolicyManager) aggregateTargetNamespacesByPolicies(podName ktypes.NamespacedName, externalRoutePolicies []*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) (sets.Set[string], error) { - targetNamespaces := sets.New[string]() - for _, erp := range externalRoutePolicies { - namespaces, err := m.listNamespacesBySelector(&erp.Spec.From.NamespaceSelector) - if err != nil { - return nil, err - } - for _, ns := range namespaces { - if targetNamespaces.Has(ns.Name) { - klog.Warningf("External gateway pod %s targets namespace %s more than once", podName.Namespace, podName.Name) - continue - } - targetNamespaces = targetNamespaces.Insert(ns.Name) - } - } - return targetNamespaces, nil -} - -func (m *externalPolicyManager) findGatewayInfoForPodInTargetNamespace(key ktypes.NamespacedName, targetNamespace string, processedPolicies []*routePolicy) (*gatewayInfo, error) { - for _, p := range processedPolicies { - namespaces, err := m.listNamespacesBySelector(p.targetNamespacesSelector) - if err != nil { - return nil, err - } - for _, targetNs := range namespaces { - if targetNs.Name == targetNamespace { - return p.dynamicGateways[key], nil - } - } - } - return nil, fmt.Errorf("gateway information for pod %s/%s not found", key.Namespace, key.Name) -} - -// processDeletePod removes the gateway IP derived from the pod. The IP is then removed from all the pods found in the namespaces by the -// network client (north bound as logical static route or in conntrack). -func (m *externalPolicyManager) processDeletePod(pod *v1.Pod, namespaces sets.Set[string]) error { - err := m.deletePodGatewayInNamespaces(pod, namespaces) - if err != nil { + _, err := podLister.Pods(pod.Namespace).Get(pod.Name) + if err != nil && !apierrors.IsNotFound(err) { return err } - return nil -} - -func (m *externalPolicyManager) deletePodGatewayInNamespaces(pod *v1.Pod, targetNamespaces sets.Set[string]) error { - - for nsName := range targetNamespaces { - err := m.deletePodGatewayInNamespace(pod, nsName) - if err != nil { - return err - } - } - return nil -} -func (m *externalPolicyManager) deletePodGatewayInNamespace(pod *v1.Pod, targetNamespace string) error { - - key := ktypes.NamespacedName{Namespace: pod.Namespace, Name: pod.Name} - cacheInfo, found := m.getNamespaceInfoFromCache(targetNamespace) - if !found { - klog.Warningf("Attempting to delete pod gateway %s/%s from a namespace that does not exist %s", pod.Namespace, pod.Name, targetNamespace) - return nil - } - defer m.unlockNamespaceInfoCache(targetNamespace) - gwInfo, ok := cacheInfo.DynamicGateways[key] - if !ok { - return fmt.Errorf("unable to find cached pod %s/%s external gateway information in namespace %s", pod.Namespace, pod.Name, targetNamespace) - } - annotatedGWIPs, err := m.calculateAnnotatedNamespaceGatewayIPsForNamespace(targetNamespace) - if err != nil { - return err + // Only queues policies affected by gateway pods + policies, pErr := m.listPoliciesInNamespacesUsingPodGateway(ktypes.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}) + if pErr != nil { + return pErr } - coexistingIPs, err := m.retrieveDynamicGatewayIPsForPolicies(cacheInfo.Policies) - if err != nil { - return err - } - coexistingIPs = coexistingIPs.Union(annotatedGWIPs) - // Filter out the IPs that are not in coexisting. Those IPs are to be deleted. - invalidGWIPs := gwInfo.Gateways.Difference(coexistingIPs) - // Filter out the IPs from the coexisting list that are to be kept by calculating the difference between the coexising and those IPs that are to be deleted and not coexisting at the same time. - ipsToKeep := coexistingIPs.Difference(invalidGWIPs) - klog.Infof("Coexisting %s, invalid %s, ipsToKeep %s", strings.Join(sets.List(coexistingIPs), ","), strings.Join(sets.List(invalidGWIPs), ","), strings.Join(sets.List(ipsToKeep), ",")) - err = m.netClient.deleteGatewayIPs(targetNamespace, invalidGWIPs, ipsToKeep) - if err != nil { - return err + klog.Infof("Processing gateway pod %s/%s with matching policies %+v", pod.Namespace, pod.Name, policies.UnsortedList()) + for policyName := range policies { + klog.V(2).InfoS("Queueing policy %s", policyName) + routeQueue.Add(policyName) } - gwInfo.Gateways.Delete(invalidGWIPs.UnsortedList()...) - if cacheInfo.DynamicGateways[key].Gateways.Len() == 0 { - klog.Infof("Deleting gateway info of gateway pod %s targeting namespace %s", key, targetNamespace) - delete(cacheInfo.DynamicGateways, key) - } - return nil -} -// processAddPodRoutes applies the policies associated to the pod's namespace to the pod logical route -func (m *externalPolicyManager) applyGatewayInfoToPod(newPod *v1.Pod, static gatewayInfoList, dynamic map[ktypes.NamespacedName]*gatewayInfo) error { - err := m.netClient.addGatewayIPs(newPod, static) - if err != nil { - return err - } - for _, egress := range dynamic { - err := m.netClient.addGatewayIPs(newPod, gatewayInfoList{egress}) - if err != nil { - return err - } - } return nil } -// getRoutePolicyForPodGateway iterates through the dynamic hops of a given external route policy spec to determine the pod's GW information. -// Note that a pod can match multiple policies with different configuration at the same time, with the condition -// that the pod can only target the same namespace once at most. That's a 1-1 pod to namespace match. -func (m *externalPolicyManager) getRoutePolicyForPodGateway(newPod *v1.Pod, externalRoutePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) (*routePolicy, error) { - - key := ktypes.NamespacedName{Namespace: newPod.Namespace, Name: newPod.Name} - - pp, err := m.processExternalRoutePolicy(externalRoutePolicy) - if err != nil { - return nil, err - } - if _, ok := pp.dynamicGateways[key]; !ok { - return nil, fmt.Errorf("pod %s not found while processing dynamic hops", key) - } - // store only the information needed - return &routePolicy{ - targetNamespacesSelector: pp.targetNamespacesSelector, - dynamicGateways: map[ktypes.NamespacedName]*gatewayInfo{key: pp.dynamicGateways[key]}, - }, nil - -} - func getExGwPodIPs(gatewayPod *v1.Pod, networkName string) (sets.Set[string], error) { if networkName != "" { return getMultusIPsFromNetworkName(gatewayPod, networkName) @@ -405,8 +84,33 @@ func getMultusIPsFromNetworkName(pod *v1.Pod, networkName string) (sets.Set[stri return nil, fmt.Errorf("unable to find multus network %s in pod %s/%s", networkName, pod.Namespace, pod.Name) } -func (m *externalPolicyManager) filterNamespacesUsingPodGateway(key ktypes.NamespacedName) sets.Set[string] { - namespaces := sets.New[string]() +func (m *externalPolicyManager) listPoliciesUsingPodGateway(key ktypes.NamespacedName) ([]*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, error) { + + ret := make([]*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, 0) + policies, err := m.routeLister.List(labels.Everything()) + if err != nil { + return nil, err + } + for _, p := range policies { + klog.Infof("Checking for policy %s to have pod %s", p.Name, key) + pp, err := m.processExternalRoutePolicy(p) + if err != nil { + return nil, err + } + if _, found := pp.dynamicGateways[key]; found { + klog.Infof("Policy %s has pod %s", p.Name, key) + ret = append(ret, p) + } + + } + return ret, nil +} + +func (m *externalPolicyManager) listPoliciesInNamespacesUsingPodGateway(key ktypes.NamespacedName) (sets.Set[string], error) { + policies := sets.New[string]() + // iterate through all current namespaces that contain the pod. This is needed in case the pod is deleted from an existing namespace, in which case + // if we iterated applying the namespace selector in the policies, we would miss the fact that a pod was part of a namespace that is no longer + // and we'd miss updating that namespace and removing the pod through the reconciliation of the policy in that namespace. nsList := m.listNamespaceInfoCache() for _, namespaceName := range nsList { cacheInfo, found := m.getNamespaceInfoFromCache(namespaceName) @@ -414,36 +118,17 @@ func (m *externalPolicyManager) filterNamespacesUsingPodGateway(key ktypes.Names continue } if _, ok := cacheInfo.DynamicGateways[key]; ok { - namespaces = namespaces.Insert(namespaceName) + policies = policies.Union(cacheInfo.Policies) } m.unlockNamespaceInfoCache(namespaceName) } - return namespaces -} - -func (m *externalPolicyManager) listPodsInNamespaceWithSelector(namespace string, selector *metav1.LabelSelector) ([]*v1.Pod, error) { - - s, err := metav1.LabelSelectorAsSelector(selector) + // list all namespaces that match the policy, for those new namespaces where the pod now applies + p, err := m.listPoliciesUsingPodGateway(key) if err != nil { return nil, err } - return m.podLister.Pods(namespace).List(s) -} - -func containsNamespaceInSlice(nss []*v1.Namespace, podNs string) bool { - for _, ns := range nss { - if ns.Name == podNs { - return true - } - } - return false -} - -func containsPodInSlice(pods []*v1.Pod, podName string) bool { - for _, pod := range pods { - if pod.Name == podName { - return true - } + for _, policy := range p { + policies.Insert(policy.Name) } - return false + return policies, nil } diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod_test.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod_test.go index 8ac708a7e7c..10cf9ca11bf 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod_test.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod_test.go @@ -204,7 +204,7 @@ var _ = Describe("OVN External Gateway policy", func() { Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), + {Namespace: pod1.Namespace, Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), }, })) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(Equal( @@ -212,12 +212,12 @@ var _ = Describe("OVN External Gateway policy", func() { Policies: sets.New(dynamicPolicyTest2.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), + {Namespace: pod1.Namespace, Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), }, })) deletePod(pod1, fakeClient) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(2)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5, 1).Should(Equal( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, @@ -344,8 +344,8 @@ var _ = Describe("OVN External Gateway policy", func() { Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), - {Namespace: "default", Name: pod3.Name}: newGatewayInfo(sets.New(pod3.Status.PodIPs[0].IP), false), + {Namespace: pod2.Namespace, Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), + {Namespace: pod3.Namespace, Name: pod3.Name}: newGatewayInfo(sets.New(pod3.Status.PodIPs[0].IP), false), }, })) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(Equal( @@ -360,7 +360,7 @@ var _ = Describe("OVN External Gateway policy", func() { Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod3.Name}: newGatewayInfo(sets.New(pod3.Status.PodIPs[0].IP), false), + {Namespace: pod3.Namespace, Name: pod3.Name}: newGatewayInfo(sets.New(pod3.Status.PodIPs[0].IP), false), }, })) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(Equal( @@ -368,7 +368,7 @@ var _ = Describe("OVN External Gateway policy", func() { Policies: sets.New(dynamicPolicyForTest2Only.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), + {Namespace: pod2.Namespace, Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), }, })) }) @@ -386,7 +386,7 @@ var _ = Describe("OVN External Gateway policy", func() { }, })) updatePodLabels(pod1, map[string]string{}, fakeClient) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5, 1).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go index 0164208a257..cb3aa605206 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go @@ -6,39 +6,258 @@ import ( "reflect" "strings" - v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" ktypes "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" adminpolicybasedrouteapi "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util" ) -// processAddPolicy takes in a new policy and applies it. To do that, it aggregates the IPs from the static hops and retrieves the IPs from the pods resulting from applying the +func (m *externalPolicyManager) syncRoutePolicy(policyName string, routeQueue workqueue.RateLimitingInterface) (*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, error) { + klog.Infof("Processing sync for APB %s", policyName) + routePolicy, err := m.routeLister.Get(policyName) + if err != nil && !apierrors.IsNotFound(err) { + return nil, err + } + if apierrors.IsNotFound(err) || !routePolicy.DeletionTimestamp.IsZero() { + // DELETE use case + klog.Infof("Deleting policy %s", policyName) + err = m.processDeletePolicy(policyName) + if err != nil { + return nil, fmt.Errorf("failed to delete Admin Policy Based External Route %s:%w", policyName, err) + } + return nil, nil + } + currentPolicy, found, _ := m.getRoutePolicyFromCache(routePolicy.Name) + if !found { + // ADD use case + klog.Infof("Adding policy %s", routePolicy.Name) + err := m.processAddPolicy(routePolicy) + if err != nil { + return routePolicy, fmt.Errorf("failed to create Admin Policy Based External Route %s:%w", routePolicy.Name, err) + } + return routePolicy, nil + } + + if reflect.DeepEqual(currentPolicy.Spec, routePolicy.Spec) { + // Reconcile changes to namespace or pod + klog.Infof("Reconciling policy %s with updates to namespace or pods", routePolicy.Name) + err := m.reconcilePolicyWithNamespacesAndPods(currentPolicy) + if err != nil { + return routePolicy, fmt.Errorf("failed to create Admin Policy Based External Route %s:%w", routePolicy.Name, err) + } + return routePolicy, nil + } + // UPDATE policy use case + klog.Infof("Updating policy %s", routePolicy.Name) + err = m.processUpdatePolicy(currentPolicy, routePolicy) + if err != nil { + return routePolicy, fmt.Errorf("failed to update Admin Policy Based External Route %s:%w", routePolicy.Name, err) + } + + return routePolicy, nil +} + +// processAddPolicy takes in an existing policy and reconciles it. To do that, it aggregates the IPs from the static hops and retrieves the IPs from the pods resulting from applying the // namespace and pod selectors in the dynamic hops. // The last step is to store the new policy in the route policy cache so that it can be used in the future to compare against changes in its spec. -func (m *externalPolicyManager) processAddPolicy(routePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) (*routePolicy, error) { +func (m *externalPolicyManager) processAddPolicy(routePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { // it's a new policy processedPolicies, err := m.processExternalRoutePolicy(routePolicy) if err != nil { - return nil, err + return err } err = m.applyProcessedPolicy(routePolicy.Name, processedPolicies) if err != nil { - return nil, err + return err } err = m.storeRoutePolicyInCache(routePolicy) if err != nil { - return nil, err + return err } klog.Infof("Added Admin Policy Based External Route %s", routePolicy.Name) - return processedPolicies, nil + return nil +} + +// listNamespacesWithPolicy returns a slice containing the names of the namespaces where a given policy is applied to. +func (m *externalPolicyManager) listNamespacesWithPolicy(policyName string) (sets.Set[string], error) { + nss := m.getAllNamespacesNamesInCache() + ret := sets.New[string]() + for _, ns := range nss { + if m.hasPolicyInNamespace(policyName, ns) { + ret.Insert(ns) + } + } + return ret, nil +} + +func (m *externalPolicyManager) hasPolicyInNamespace(policyName, namespaceName string) bool { + + nsInfo, found := m.getNamespaceInfoFromCache(namespaceName) + if !found { + klog.Infof("Namespace %s not found while consolidating namespaces using policy %s", namespaceName, policyName) + return false + } + defer m.unlockNamespaceInfoCache(namespaceName) + return nsInfo.Policies.Has(policyName) +} + +// locks the namespace cache and executes discrepancyFunc +func (m *externalPolicyManager) processPolicyDiscrepancyInNamespace(nsName string, routePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, + discrepancyFunc func(string, *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, *namespaceInfo) error) error { + cacheInfo, found := m.getNamespaceInfoFromCache(nsName) + if !found { + klog.Infof("Namespace %s not found in cache, creating", nsName) + cacheInfo = m.newNamespaceInfoInCache(nsName) + } + defer m.unlockNamespaceInfoCache(nsName) + return discrepancyFunc(nsName, routePolicy, cacheInfo) +} + +func (m *externalPolicyManager) reconcilePolicyWithNamespacesAndPods(routePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { + + nss, err := m.listNamespacesBySelector(&routePolicy.Spec.From.NamespaceSelector) + if err != nil { + return err + } + targetNs := sets.New[string]() + for _, ns := range nss { + targetNs.Insert(ns.Name) + } + + klog.Infof("Reconciling namespaces %+v for policy %s", strings.Join(targetNs.UnsortedList(), ", "), routePolicy.Name) + // reconcile Namespaces + currentNs, err := m.listNamespacesWithPolicy(routePolicy.Name) + if err != nil { + return err + } + klog.Infof("List of existing namespaces with policy %s: %s", routePolicy.Name, strings.Join(currentNs.UnsortedList(), ", ")) + if len(currentNs.Difference(targetNs)) > 0 { + klog.Infof("Removing APB policy %s in namespaces %+v", routePolicy.Name, currentNs.Difference(targetNs)) + } + // namespaces where the policy should not be applied anymore. + for nsName := range currentNs.Difference(targetNs) { + err = m.processPolicyDiscrepancyInNamespace(nsName, routePolicy, m.removePolicyFromNamespace) + if err != nil { + return err + } + } + if len(targetNs.Difference(currentNs)) > 0 { + klog.Infof("Adding policy %s to namespaces %+v", routePolicy.Name, targetNs.Difference(currentNs)) + } + // namespaces where the policy should now be applied + for nsName := range targetNs.Difference(currentNs) { + err = m.processPolicyDiscrepancyInNamespace(nsName, routePolicy, m.applyPolicyToNamespace) + if err != nil { + return err + } + } + klog.Infof("Reconciling policy %s against matching namespaces %+v", routePolicy.Name, targetNs.Intersection(currentNs)) + // namespaces where the policy still applies. In this case validate the dynamic hops + for nsName := range targetNs.Intersection(currentNs) { + err = m.processReconciliationWithNamespace(nsName) + if err != nil { + return err + } + } + return nil +} + +func (m *externalPolicyManager) processReconciliationWithNamespace(nsName string) error { + cacheInfo, found := m.getNamespaceInfoFromCache(nsName) + if !found { + klog.Infof("Namespace %s not found in cache", nsName) + return nil + } + klog.Infof("Retrieving cache for ns %s: +%v dynamic", nsName, cacheInfo.DynamicGateways) + defer m.unlockNamespaceInfoCache(nsName) + policies := make([]*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, 0) + for policy := range cacheInfo.Policies { + cachedPolicy, found, markedForDeletion := m.getRoutePolicyFromCache(policy) + if !found { + klog.Infof("Policy %s not found while calculating all policies in a namespace", policy) + continue + } + if markedForDeletion { + klog.Warningf("Attempting to add or update route policy %s when it has been marked for deletion. Skipping...", policy) + continue + } + policies = append(policies, cachedPolicy) + } + processedPolicies, err := m.processExternalRoutePolicies(policies) + if err != nil { + return err + } + // remove pod gw IPs that are no longer valid + allProcessedGWIPs := map[ktypes.NamespacedName]*gatewayInfo{} + // Consolidate all dynamic gateway IPs from the policies into a single map + for _, pp := range processedPolicies { + for k, v := range pp.dynamicGateways { + klog.Infof("Processing %+v", k) + allProcessedGWIPs[k] = v + } + } + newGateways, invalidGWIPs, ipsToKeep := m.calculateDynamicGateways(allProcessedGWIPs, cacheInfo.DynamicGateways) + klog.Infof("[%s] Deleting gateway IPs %+v, keeping %+v", nsName, invalidGWIPs, ipsToKeep) + // delete all invalid GW IP references in the NorthBoundDB (master controller) or conntrack (node_controller) + err = m.netClient.deleteGatewayIPs(nsName, invalidGWIPs, ipsToKeep) + if err != nil { + return err + } + // proceed to add the GW IPs again + for _, gatewayInfo := range newGateways { + klog.Infof("[%s] Adding gateway IPs %+v", nsName, gatewayInfo) + err = m.addGWRoutesForNamespace(nsName, gatewayInfoList{gatewayInfo}) + if err != nil { + return err + } + } + cacheInfo.DynamicGateways = newGateways + return nil +} + +func (m *externalPolicyManager) calculateDynamicGateways(allProcessedGWIPs, cachedDynamicGWInfo map[ktypes.NamespacedName]*gatewayInfo) (map[ktypes.NamespacedName]*gatewayInfo, sets.Set[string], sets.Set[string]) { + klog.Infof("Processed policies: %+v", allProcessedGWIPs) + // In order to delete the invalid GWs, the logic has to collect all the valid GW IPs as well as the invalid ones. + // This is due to implementation requirements by the network clients: for the NB interaction (master controller), only the invalid GWs is needed + // but when interacting with the conntrack (node controller), it requires to use only the valid GWs due to a white listing approach + // (delete any entry that does not reference any of these IPs) when deleting its entries. + ipsToKeep := sets.New[string]() + invalidGWIPs := sets.New[string]() + // this map will be used to store all valid gateway info references as they are processed in the next two loop. + newGateways := map[ktypes.NamespacedName]*gatewayInfo{} + for k, v1 := range allProcessedGWIPs { + v2, ok := cachedDynamicGWInfo[k] + if ok && !v1.Equal(v2) { + // podGW not found or its gatewayInfo does not match, remove it + klog.Infof("PP to cacheInfo: invalid GW IP %+v compared to %+v", v2, v1) + invalidGWIPs.Insert(v2.Gateways.UnsortedList()...) + continue + } + // store th gatewayInfo in the map + klog.Infof("Storing %s when removing pod GWs ", k) + newGateways[k] = v1 + ipsToKeep.Insert(v1.Gateways.UnsortedList()...) + } + + // Compare all elements in the cacheInfo map against the consolidated map: those that are not in the consolidated map are to be deleted. + // The previous loop covers for the pods that exist in both maps but contain different gateway infos, and thus to be deleted + for k, v := range cachedDynamicGWInfo { + if _, ok := allProcessedGWIPs[k]; !ok { + // IP not found in the processed policies, it means the pod gateway information is no longer applicable + klog.Infof("CacheInfo-> GW IP %+v not found in processed policies", k) + invalidGWIPs.Insert(v.Gateways.UnsortedList()...) + continue + } + } + return newGateways, invalidGWIPs, ipsToKeep } // applyProcessedPolicy takes in a route policy and applies it to each of the namespaces defined in the namespaces selector in the route policy. @@ -50,16 +269,24 @@ func (m *externalPolicyManager) applyProcessedPolicy(policyName string, routePol return err } for _, ns := range targetNs { - // ensure namespace still exists before processing it - _, err := m.namespaceLister.Get(ns.Name) - if apierrors.IsNotFound(err) { - // namespace no longer exists - continue - } cacheInfo, found := m.getNamespaceInfoFromCache(ns.Name) if !found { cacheInfo = m.newNamespaceInfoInCache(ns.Name) } + // ensure namespace still exists before processing it + namespace, err := m.namespaceLister.Get(ns.Name) + if err != nil && !apierrors.IsNotFound(err) { + return err + } + // if the namespace no longer exists or it is being deleted then skip it + if apierrors.IsNotFound(err) || !namespace.DeletionTimestamp.IsZero() { + if !found { + // remove it as we are responsible for creating it. + m.namespaceInfoSyncCache.Delete(ns.Name) + } + m.unlockNamespaceInfoCache(ns.Name) + continue + } err = m.applyProcessedPolicyToNamespace(ns.Name, policyName, routePolicy, cacheInfo) m.unlockNamespaceInfoCache(ns.Name) if err != nil { @@ -76,31 +303,36 @@ func (m *externalPolicyManager) processDeletePolicy(policyName string) error { // mark the policy for deletion. // if it's already marked continue processing the delete action as this could be a retry attempt from a previous failed delete run. // if it's no longer in the cache, return nil + klog.Infof("Getting route %s and marking it for deletion", policyName) routePolicy, found := m.getAndMarkRoutePolicyForDeletionInCache(policyName) if !found { + klog.Infof("Policy %s not found", policyName) return nil } - targetNs, err := m.listNamespacesBySelector(&routePolicy.Spec.From.NamespaceSelector) - if err != nil { - return err - } - for _, ns := range targetNs { - cacheInfo, found := m.getNamespaceInfoFromCache(ns.Name) + for _, ns := range m.getAllNamespacesNamesInCache() { + cacheInfo, found := m.getNamespaceInfoFromCache(ns) if !found { - klog.Warningf("Attempting to delete policy %s from a namespace that does not exist %s", routePolicy.Name, ns.Name) + klog.Infof("Attempting to delete policy %s from a namespace that does not exist %s", routePolicy.Name, ns) continue } - err = m.removePolicyFromNamespace(ns.Name, &routePolicy, cacheInfo) - m.unlockNamespaceInfoCache(ns.Name) + var err error + if cacheInfo.Policies.Has(routePolicy.Name) { + err = m.removePolicyFromNamespace(ns, &routePolicy, cacheInfo) + } + m.unlockNamespaceInfoCache(ns) if err != nil { return err } } - err = m.deleteRoutePolicyFromCache(routePolicy.Name) + klog.Infof("Proceeding to delete route %s from cache", routePolicy.Name) + err := m.deleteRoutePolicyFromCache(routePolicy.Name) if err != nil { return err } klog.Infof("Deleted Admin Policy Based External Route %s", routePolicy.Name) + // Requeue policy after delete to reconcile any namespaces that still reference it. + // This scenario can happen when a routine retrieves the policy reference before it is marked for deletion. + // routeQueue.Add(&routePolicy) return nil } @@ -110,6 +342,9 @@ func (m *externalPolicyManager) processDeletePolicy(policyName string) error { func (m *externalPolicyManager) calculateAnnotatedNamespaceGatewayIPsForNamespace(targetNamespace string) (sets.Set[string], error) { namespace, err := m.namespaceLister.Get(targetNamespace) if err != nil { + if apierrors.IsNotFound(err) { + return sets.New[string](), nil + } return nil, err } @@ -166,32 +401,42 @@ func (m *externalPolicyManager) calculateAnnotatedPodGatewayIPsForNamespace(targ // found in at least a legacy annotation or another policy impacting the namespace, then the IP is not removed from the cache or the network resource (north bound or conntrack) func (m *externalPolicyManager) deletePolicyInNamespace(namespaceName, policyName string, routePolicy *routePolicy, cacheInfo *namespaceInfo) error { coexistingPolicies := cacheInfo.Policies.Clone().Delete(policyName) - annotatedGWIPs, err := m.calculateAnnotatedNamespaceGatewayIPsForNamespace(namespaceName) - if err != nil { - return err - } - coexistingIPs, err := m.retrieveStaticGatewayIPsForPolicies(coexistingPolicies) - if err != nil { - return err - } // don't care if the route is flagged for deletion, delete any gw IPs related to the policy policy, found, _ := m.getRoutePolicyFromCache(policyName) if !found { return fmt.Errorf("policy %s not found", policyName) } - pp, err := m.processExternalRoutePolicy(&policy) + pp, err := m.processExternalRoutePolicy(policy) + if err != nil { + return err + } + + // Static Hops + annotatedGWIPs, err := m.calculateAnnotatedNamespaceGatewayIPsForNamespace(namespaceName) + if err != nil { + return err + } + coexistingIPs, err := m.retrieveStaticGatewayIPsForPolicies(coexistingPolicies) if err != nil { return err } static := sets.New[string]() + // consolidate all IPs from the static hops of the policy for _, gatewayInfo := range pp.staticGateways { static = static.Insert(gatewayInfo.Gateways.UnsortedList()...) } + // delete the subset of IPs that are meant to be deleted from the static hops defined by the policy for _, gwInfo := range routePolicy.staticGateways { static = static.Delete(gwInfo.Gateways.UnsortedList()...) } + // Consolidate all IPs to keep: + // * IPs from other policies that target the namespace. + // * Annotated IPs coming from the legacy gateway API. + // * Remaining IPs from the policy after removing those IPs that are no longer applicable to the policy. This can happen when a policy has changed its spec and now a subset of the old IPs are + // no longer valid based on the new spec. This set contains only the subset of original IPs that are still valid. + // coexistingIPs = coexistingIPs.Union(annotatedGWIPs).Union(static) for _, gwInfo := range routePolicy.staticGateways { @@ -200,6 +445,9 @@ func (m *externalPolicyManager) deletePolicyInNamespace(namespaceName, policyNam // Filter out the IPs from the coexisting list that are to be kept by calculating the difference between the coexising and those IPs that are to be deleted and not coexisting at the same time. ipsToKeep := coexistingIPs.Difference(invalidGWIPs) klog.Infof("Coexisting %s, invalid %s, ipsToKeep %s", strings.Join(sets.List(coexistingIPs), ","), strings.Join(sets.List(invalidGWIPs), ","), strings.Join(sets.List(ipsToKeep), ",")) + if len(invalidGWIPs) == 0 { + continue + } err := m.netClient.deleteGatewayIPs(namespaceName, invalidGWIPs, ipsToKeep) if err != nil { return err @@ -211,6 +459,7 @@ func (m *externalPolicyManager) deletePolicyInNamespace(namespaceName, policyNam gwInfo.Gateways.Delete(invalidGWIPs.UnsortedList()...) } + // Dynamic Hops annotatedGWIPs, err = m.calculateAnnotatedPodGatewayIPsForNamespace(namespaceName) if err != nil { return err @@ -228,23 +477,34 @@ func (m *externalPolicyManager) deletePolicyInNamespace(namespaceName, policyNam for _, gwInfo := range routePolicy.dynamicGateways { dynamic = dynamic.Delete(gwInfo.Gateways.UnsortedList()...) } + // Consolidate all IPs to keep: + // * IPs from other policies that target the namespace. + // * Annotated IPs coming from the legacy gateway API. + // * Remaining IPs from the policy after removing those IPs that are no longer applicable to the policy. This can happen when a policy has changed its spec and now a subset of the old IPs are + // no longer valid based on the new spec. This set contains only the subset of original IPs that are still valid. + // coexistingIPs = coexistingIPs.Union(annotatedGWIPs).Union(dynamic) - for pod, gwInfo := range routePolicy.dynamicGateways { + for gwPodNamespacedName, gwInfo := range routePolicy.dynamicGateways { // Filter out the IPs that are not in coexisting. Those IPs are to be deleted. invalidGWIPs := gwInfo.Gateways.Difference(coexistingIPs) // Filter out the IPs from the coexisting list that are to be kept by calculating the difference between the coexising and those IPs that are to be deleted and not coexisting at the same time. ipsToKeep := coexistingIPs.Difference(invalidGWIPs) klog.Infof("Coexisting %s, invalid %s, ipsToKeep %s", strings.Join(sets.List(coexistingIPs), ","), strings.Join(sets.List(invalidGWIPs), ","), strings.Join(sets.List(ipsToKeep), ",")) + if len(invalidGWIPs) == 0 { + continue + } err := m.netClient.deleteGatewayIPs(namespaceName, invalidGWIPs, ipsToKeep) if err != nil { return err } if gwInfo.Gateways.Difference(invalidGWIPs).Len() == 0 { // delete cached information for the pod gateway - delete(cacheInfo.DynamicGateways, pod) + klog.Infof("Deleting cache entry for dynamic hop %s", gwPodNamespacedName) + delete(cacheInfo.DynamicGateways, gwPodNamespacedName) continue } + klog.Infof("Deleting dynamic hop IPs in gateway info %s", strings.Join(sets.List(invalidGWIPs), ",")) gwInfo.Gateways.Delete(invalidGWIPs.UnsortedList()...) } return nil @@ -275,6 +535,7 @@ func (m *externalPolicyManager) applyProcessedPolicyToNamespace(namespaceName, p cacheInfo.DynamicGateways[pod] = newGatewayInfo(info.Gateways.items, info.BFDEnabled) } + klog.Infof("Adding policy %s to namespace %s", policyName, namespaceName) cacheInfo.Policies = cacheInfo.Policies.Insert(policyName) return nil } @@ -284,28 +545,28 @@ func (m *externalPolicyManager) applyProcessedPolicyToNamespace(namespaceName, p // * Remove the static and dynamic hop entries in the namespaces impacted by the current version of the policy that are in the current policy but not in the updated version. // * Apply the static and dynamic hop entries in the namespaces impacted by the updated version of the policy that are in the updated version but not in the current version. // * Store the updated policy in the route policy cache. -func (m *externalPolicyManager) processUpdatePolicy(currentPolicy, updatedPolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) (*routePolicy, error) { +func (m *externalPolicyManager) processUpdatePolicy(currentPolicy, updatedPolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { klog.Infof("Processing update for Admin Policy Based External Route '%s'", currentPolicy.Name) // To update the policies, first we'll process the diff between old and new and remove the discrepancies that are not found in the new object. // Afterwards, we'll process the diff between the new and the old and apply the new policies not found in the old policy, ensuring that we are not reduplicating the gatewayInfo. err := m.removeDiscrepanciesInRoutePolicy(currentPolicy, updatedPolicy) if err != nil { - return nil, err + return err } // At this point we have removed all the aspects of the current policy that no longer applies. Next step is to apply the parts of the new policy that are not in the current one. err = m.applyUpdatesInRoutePolicy(currentPolicy, updatedPolicy) if err != nil { - return nil, err + return err } // update the cache to ensure it reflects the latest copy - err = m.storeRoutePolicyInCache(updatedPolicy) - if err != nil { - return nil, err - } - klog.Infof("Updated Admin Policy Based External Route %s", currentPolicy.Name) - return m.processExternalRoutePolicy(updatedPolicy) + return m.storeRoutePolicyInCache(updatedPolicy) + // if err != nil { + // return err + // } + // klog.Infof("Updated Admin Policy Based External Route %s", currentPolicy.Name) + // return m.processExternalRoutePolicy(updatedPolicy) } func (m *externalPolicyManager) applyUpdatesInRoutePolicy(currentPolicy, newPolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { @@ -364,6 +625,7 @@ func (m *externalPolicyManager) removeDiscrepanciesInRoutePolicy(currentPolicy, if err != nil { return err } + klog.Infof("Removing discrepancies between current and updated policy: %+v, %+v, %+v", unmatchingNamespaces, unmatchingStaticHops, unmatchingDynamicHops) // delete the namespaces where this policy no longer applies for unmatchNs := range unmatchingNamespaces { cacheInfo, found := m.getNamespaceInfoFromCache(unmatchNs) @@ -372,11 +634,10 @@ func (m *externalPolicyManager) removeDiscrepanciesInRoutePolicy(currentPolicy, continue } err := m.removePolicyFromNamespace(unmatchNs, currentPolicy, cacheInfo) + m.unlockNamespaceInfoCache(unmatchNs) if err != nil { - m.unlockNamespaceInfoCache(unmatchNs) return err } - m.unlockNamespaceInfoCache(unmatchNs) } // delete the hops that no longer apply from all the current policy's applicable namespaces @@ -487,11 +748,16 @@ func (m *externalPolicyManager) processExternalRoutePolicy(policy *adminpolicyba if err != nil { errors = append(errors, err) } - + if len(staticGWInfo) > 0 { + klog.Infof("Found static hops for policy %s:%+v", policy.Name, staticGWInfo) + } dynamicGWInfo, err := m.processDynamicHopsGatewayInformation(policy.Spec.NextHops.DynamicHops) if err != nil { errors = append(errors, err) } + if len(dynamicGWInfo) > 0 { + klog.Infof("Found dynamic hops for policy %s:%+v", policy.Name, dynamicGWInfo) + } if len(errors) > 0 { return nil, kerrors.NewAggregate(errors) } @@ -515,84 +781,6 @@ func (m *externalPolicyManager) processExternalRoutePolicies(externalRoutePolici return routePolicies, nil } -func (m *externalPolicyManager) findMatchingDynamicPolicies(pod *v1.Pod) ([]*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, error) { - var routePolicies []*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute - crs, err := m.routeLister.List(labels.Everything()) - if err != nil { - return nil, err - } - for _, cr := range crs { - policySpec := adminpolicybasedrouteapi.AdminPolicyBasedExternalRouteSpec{ - From: cr.Spec.From, - NextHops: adminpolicybasedrouteapi.ExternalNextHops{DynamicHops: []*adminpolicybasedrouteapi.DynamicHop{}}} - for _, dp := range cr.Spec.NextHops.DynamicHops { - nss, err := m.listNamespacesBySelector(dp.NamespaceSelector) - if err != nil { - return nil, err - } - if !containsNamespaceInSlice(nss, pod.Namespace) { - continue - } - nsPods, err := m.listPodsInNamespaceWithSelector(pod.Namespace, &dp.PodSelector) - if err != nil { - return nil, err - } - if containsPodInSlice(nsPods, pod.Name) { - // add only the hop information that intersects with the pod - policySpec.NextHops.DynamicHops = append(policySpec.NextHops.DynamicHops, dp) - } - } - if len(policySpec.NextHops.DynamicHops) > 0 { - routePolicies = append(routePolicies, &adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute{ - ObjectMeta: metav1.ObjectMeta{ - Name: cr.Name, - }, - Spec: policySpec, - }) - } - - } - return routePolicies, nil -} - -func (m *externalPolicyManager) getPoliciesForNamespace(namespaceName string) (sets.Set[string], error) { - matches := sets.New[string]() - policies, err := m.routeLister.List(labels.Everything()) - if err != nil { - return nil, err - } - - for _, policy := range policies { - targetNamespaces, err := m.listNamespacesBySelector(&policy.Spec.From.NamespaceSelector) - if err != nil { - return nil, err - } - for _, ns := range targetNamespaces { - if namespaceName == ns.Name { - matches = matches.Insert(policy.Name) - } - } - } - - return matches, nil -} - -func (m *externalPolicyManager) aggregateDynamicRouteGatewayInformation(pod *v1.Pod, routePolicy *routePolicy) (map[string]*gatewayInfo, error) { - key := ktypes.NamespacedName{Namespace: pod.Namespace, Name: pod.Name} - gwInfoMap := make(map[string]*gatewayInfo) - targetNs, err := m.listNamespacesBySelector(routePolicy.targetNamespacesSelector) - if err != nil { - return nil, err - } - for _, ns := range targetNs { - if _, ok := gwInfoMap[ns.Name]; ok { - return nil, fmt.Errorf("duplicated target namespace '%s ' while processing external policies for pod %s/%s", ns.Name, pod.Namespace, pod.Name) - } - gwInfoMap[ns.Name] = routePolicy.dynamicGateways[key] - } - return gwInfoMap, nil -} - // calculatePolicyDifferences determines the differences between two policies in terms of namespaces where the policy applies, and the differences in static and dynamic hops. // The return values are the namespaces, static hops and dynamic hops that are in the first policy but not in the second instance. func (m *externalPolicyManager) calculatePolicyDifferences(policy1, policy2 *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) (sets.Set[string], []*adminpolicybasedrouteapi.StaticHop, []*adminpolicybasedrouteapi.DynamicHop, error) { @@ -683,9 +871,11 @@ func (m *externalPolicyManager) retrieveDynamicGatewayIPsForPolicies(coexistingP coexistingDynamicIPs := sets.New[string]() for name := range coexistingPolicies { - policy, found, _ := m.getRoutePolicyFromCache(name) + policy, found, markedForDeletion := m.getRoutePolicyFromCache(name) if !found { - klog.Warningf("Unable to find route policy %s in cache", name) + return nil, fmt.Errorf("failed to find external route policy %s in cache", name) + } + if markedForDeletion { continue } pp, err := m.processDynamicHopsGatewayInformation(policy.Spec.NextHops.DynamicHops) @@ -705,9 +895,11 @@ func (m *externalPolicyManager) retrieveStaticGatewayIPsForPolicies(policies set coexistingStaticIPs := sets.New[string]() for name := range policies { - policy, found, _ := m.getRoutePolicyFromCache(name) + policy, found, markedForDeletion := m.getRoutePolicyFromCache(name) if !found { - klog.Warningf("Unable to find route policy %s in cache", name) + return nil, fmt.Errorf("unable to find route policy %s in cache", name) + } + if markedForDeletion { continue } pp, err := m.processStaticHopsGatewayInformation(policy.Spec.NextHops.StaticHops) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy_test.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy_test.go index 093774eb122..deb77e501bc 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy_test.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy_test.go @@ -2,12 +2,12 @@ package apbroute import ( "context" - . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/klog/v2" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes/fake" @@ -349,7 +349,7 @@ var _ = Describe("OVN External Gateway policy", func() { Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(2)) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) deletePolicy(staticPolicy.Name, fakeRouteClient) - Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) + Eventually(func() []string { return listRoutePolicyInCache() }, 5, 1).Should(HaveLen(1)) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) @@ -375,15 +375,15 @@ var _ = Describe("OVN External Gateway policy", func() { It("validates that an overlapping IP from another policy will not be deleted when one of the overlaping policies is deleted", func() { initController([]runtime.Object{namespaceTest, namespaceDefault, pod1}, []runtime.Object{staticPolicy, duplicatedStatic, dynamicPolicy, duplicatedDynamic}) + Eventually(func() []string { return listRoutePolicyInCache() }, 5, 1).Should(HaveLen(4)) - Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(4)) deletePolicy(staticPolicy.Name, fakeRouteClient) deletePolicy(dynamicPolicy.Name, fakeRouteClient) - Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(2)) + Eventually(func() []string { return listRoutePolicyInCache() }, 5, 1).Should(HaveLen(2)) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) - }, 5).Should(BeComparableTo( + }, 5, 1).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(duplicatedStatic.Name, duplicatedDynamic.Name), StaticGateways: gatewayInfoList{ @@ -419,6 +419,7 @@ var _ = Describe("OVN External Gateway policy", func() { p.Spec.From.NamespaceSelector = v1.LabelSelector{MatchLabels: namespaceTest2.Labels} p.Generation++ lastUpdate := p.Status.LastTransitionTime + klog.Info("TROZET BEFORE UPDATE") _, err = fakeRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Update(context.Background(), p, v1.UpdateOptions{}) Expect(err).NotTo(HaveOccurred()) Eventually(func() v1.Time { @@ -426,9 +427,7 @@ var _ = Describe("OVN External Gateway policy", func() { Expect(err).NotTo(HaveOccurred()) return p.Status.LastTransitionTime }).Should(Not(Equal(lastUpdate))) - Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(2)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal(newNamespaceInfo())) - + Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(BeComparableTo(expected, cmpOpts...)) @@ -509,7 +508,7 @@ var _ = Describe("OVN External Gateway policy", func() { Policies: sets.New(singlePodDynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), + {Namespace: pod1.Namespace, Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), }}, cmpOpts...)) @@ -533,7 +532,7 @@ var _ = Describe("OVN External Gateway policy", func() { Policies: sets.New(singlePodDynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), + {Namespace: pod2.Namespace, Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), }}, cmpOpts...)) @@ -664,5 +663,88 @@ var _ = Describe("OVN External Gateway policy", func() { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo(expected, cmpOpts...)) }) + + It("validates that changing the BFD setting in a static hop will trigger an update", func() { + initController([]runtime.Object{namespaceDefault, namespaceTest}, []runtime.Object{staticPolicy}) + Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) + Eventually(listNamespaceInfo(), 5).Should(HaveLen(1)) + + Eventually(func() *namespaceInfo { + return getNamespaceInfo(namespaceTest.Name) + }, 5).Should(BeComparableTo( + &namespaceInfo{ + Policies: sets.New(staticPolicy.Name), + StaticGateways: gatewayInfoList{ + newGatewayInfo(sets.New(staticHopGWIP), false), + }, + DynamicGateways: map[types.NamespacedName]*gatewayInfo{}}, + cmpOpts...)) + + By("set BDF to true in the static hop") + p, err := fakeRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.Background(), staticPolicy.Name, v1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + p.Spec.NextHops.StaticHops[0].BFDEnabled = true + p.Generation++ + lastUpdate := p.Status.LastTransitionTime + _, err = fakeRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Update(context.Background(), p, v1.UpdateOptions{}) + Expect(err).NotTo(HaveOccurred()) + Eventually(func() v1.Time { + p, err = fakeRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.TODO(), staticPolicy.Name, v1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + return p.Status.LastTransitionTime + }).Should(Not(Equal(lastUpdate))) + + Eventually(func() *namespaceInfo { + return getNamespaceInfo(namespaceTest.Name) + }, 5).Should(BeComparableTo( + &namespaceInfo{ + Policies: sets.New(staticPolicy.Name), + StaticGateways: gatewayInfoList{ + newGatewayInfo(sets.New(staticHopGWIP), true), + }, + DynamicGateways: map[types.NamespacedName]*gatewayInfo{}}, + cmpOpts...)) + + }) + + It("validates that changing the BFD setting in a dynamic hop will trigger an update", func() { + initController([]runtime.Object{namespaceDefault, namespaceTest, pod1}, []runtime.Object{dynamicPolicy}) + Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) + Eventually(listNamespaceInfo(), 5).Should(HaveLen(1)) + Eventually(func() *namespaceInfo { + return getNamespaceInfo(namespaceTest.Name) + }, 5).Should(BeComparableTo( + &namespaceInfo{ + Policies: sets.New(dynamicPolicy.Name), + StaticGateways: gatewayInfoList{}, + DynamicGateways: map[types.NamespacedName]*gatewayInfo{ + {Namespace: pod1.Namespace, Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), + }}, + cmpOpts...)) + By("set BDF to true in the dynamic hop") + p, err := fakeRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.Background(), dynamicPolicy.Name, v1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + p.Spec.NextHops.DynamicHops[0].BFDEnabled = true + p.Generation++ + lastUpdate := p.Status.LastTransitionTime + _, err = fakeRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Update(context.Background(), p, v1.UpdateOptions{}) + Expect(err).NotTo(HaveOccurred()) + Eventually(func() v1.Time { + p, err = fakeRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.TODO(), dynamicPolicy.Name, v1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + return p.Status.LastTransitionTime + }).Should(Not(Equal(lastUpdate))) + Eventually(func() *namespaceInfo { + return getNamespaceInfo(namespaceTest.Name) + }, 5).Should(BeComparableTo( + &namespaceInfo{ + Policies: sets.New(dynamicPolicy.Name), + StaticGateways: gatewayInfoList{}, + DynamicGateways: map[types.NamespacedName]*gatewayInfo{ + {Namespace: pod1.Namespace, Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), true), + }}, + cmpOpts...)) + + }) }) }) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_test.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_test.go index 77256fd8d07..6251979d082 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_test.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_test.go @@ -14,7 +14,6 @@ var ( // Default comparable options to bypass comparing the mux object and define a less function to sort the slices. cmpOpts = []cmp.Option{ cmpopts.IgnoreFields(syncSet{}, "mux"), - cmpopts.IgnoreFields(namespaceInfo{}, "markForDelete"), cmpopts.SortSlices(func(x, y interface{}) bool { s1, ok1 := x.(*gatewayInfo) s2, ok2 := y.(*gatewayInfo) diff --git a/go-controller/pkg/ovn/controller/apbroute/master_controller.go b/go-controller/pkg/ovn/controller/apbroute/master_controller.go index ad024218ee5..47186479ca9 100644 --- a/go-controller/pkg/ovn/controller/apbroute/master_controller.go +++ b/go-controller/pkg/ovn/controller/apbroute/master_controller.go @@ -202,7 +202,6 @@ func (c *ExternalGatewayMasterController) Run(threadiness int) { klog.Infof("Repairing Admin Policy Based External Route Services") c.repair() - startWg := &sync.WaitGroup{} wg := &sync.WaitGroup{} for i := 0; i < threadiness; i++ { for _, workerFn := range []func(*sync.WaitGroup){ @@ -213,10 +212,8 @@ func (c *ExternalGatewayMasterController) Run(threadiness int) { // detects namespace changes and applies polices that match the namespace selector in the `From` policy field c.runNamespaceWorker, } { - startWg.Add(1) wg.Add(1) go func(fn func(*sync.WaitGroup)) { - startWg.Done() defer wg.Done() wait.Until(func() { fn(wg) @@ -224,7 +221,6 @@ func (c *ExternalGatewayMasterController) Run(threadiness int) { }(workerFn) } } - startWg.Wait() // wait until we're told to stop <-c.stopCh @@ -245,87 +241,47 @@ func (c *ExternalGatewayMasterController) processNextPolicyWorkItem(wg *sync.Wai wg.Add(1) defer wg.Done() - obj, shutdown := c.routeQueue.Get() + key, shutdown := c.routeQueue.Get() if shutdown { return false } - defer c.routeQueue.Done(obj) + defer c.routeQueue.Done(key) - item := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) - klog.Infof("Processing policy %s", item.Name) - err := c.syncRoutePolicy(item) + klog.Infof("Processing policy %s", key) + policy, err := c.mgr.syncRoutePolicy(key.(string), c.routeQueue) if err != nil { - if c.routeQueue.NumRequeues(item) < maxRetries { + klog.Errorf("Failed to sync APB policy %s: %v", key, err) + } + // capture the error from processing the sync in the statuses message field + err = c.updateStatusAPBExternalRoute(policy, err) + if err != nil { + if c.routeQueue.NumRequeues(key) < maxRetries { klog.V(2).InfoS("Error found while processing policy: %w", err) - c.routeQueue.AddRateLimited(item) + c.routeQueue.AddRateLimited(key) return true } - klog.Warningf("Dropping policy %q out of the queue: %w", item.Name, err) + klog.Warningf("Dropping policy %q out of the queue: %w", key, err) utilruntime.HandleError(err) } - c.routeQueue.Forget(obj) + c.routeQueue.Forget(key) return true } -func (c *ExternalGatewayMasterController) syncRoutePolicy(routePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { - _, err := c.routeLister.Get(routePolicy.Name) - if err != nil && !apierrors.IsNotFound(err) { - return err - } - if apierrors.IsNotFound(err) { - // DELETE use case - klog.Infof("Deleting policy %s", routePolicy.Name) - err = c.mgr.processDeletePolicy(routePolicy.Name) - if err != nil { - return fmt.Errorf("failed to delete Admin Policy Based External Route %s:%w", routePolicy.Name, err) - } - klog.Infof("Policy %s deleted", routePolicy.Name) - return nil - } - currentPolicy, found, markedForDeletion := c.mgr.getRoutePolicyFromCache(routePolicy.Name) - if markedForDeletion { - klog.Warningf("Attempting to add or update route policy %s when it has been marked for deletion. Skipping...", routePolicy.Name) - return nil - } - if !found { - // ADD use case - klog.Infof("Adding policy %s", routePolicy.Name) - pp, err := c.mgr.processAddPolicy(routePolicy) - newErr := c.updateStatusAPBExternalRoute(routePolicy.Name, pp, err) - if err != nil { - return fmt.Errorf("failed to create Admin Policy Based External Route %s:%w", routePolicy.Name, err) - } - if newErr != nil { - return fmt.Errorf("failed to update status in Admin Policy Based External Route %s:%w", routePolicy.Name, newErr) - } - return nil - } - // UPDATE use case - klog.Infof("Updating policy %s", routePolicy.Name) - pp, err := c.mgr.processUpdatePolicy(¤tPolicy, routePolicy) - newErr := c.updateStatusAPBExternalRoute(routePolicy.Name, pp, err) - if err != nil { - return fmt.Errorf("failed to update Admin Policy Based External Route %s:%w", routePolicy.Name, err) - } - if newErr != nil { - return fmt.Errorf("failed to update status in Admin Policy Based External Route %s:%w", routePolicy.Name, newErr) - } - return nil -} - func (c *ExternalGatewayMasterController) onPolicyAdd(obj interface{}) { - policy, ok := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) + _, ok := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) if !ok { utilruntime.HandleError(fmt.Errorf("expecting %T but received %T", &adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute{}, obj)) return } - if policy == nil { - utilruntime.HandleError(errors.New("invalid Admin Policy Based External Route provided to onPolicyAdd()")) + key, err := cache.MetaNamespaceKeyFunc(obj) + if err != nil { + utilruntime.HandleError(fmt.Errorf("couldn't get key for object %+v: %v", obj, err)) return } - c.routeQueue.Add(policy) + klog.V(4).Infof("Adding policy %s", key) + c.routeQueue.Add(key) } func (c *ExternalGatewayMasterController) onPolicyUpdate(oldObj, newObj interface{}) { @@ -348,26 +304,33 @@ func (c *ExternalGatewayMasterController) onPolicyUpdate(oldObj, newObj interfac return } - c.routeQueue.Add(newObj) + key, err := cache.MetaNamespaceKeyFunc(newObj) + if err != nil { + utilruntime.HandleError(fmt.Errorf("couldn't get key for object %+v: %v", newObj, err)) + } + c.routeQueue.Add(key) } func (c *ExternalGatewayMasterController) onPolicyDelete(obj interface{}) { - policy, ok := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) + _, ok := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) if !ok { tombstone, ok := obj.(cache.DeletedFinalStateUnknown) if !ok { utilruntime.HandleError(fmt.Errorf("couldn't get object from tomstone %#v", obj)) return } - policy, ok = tombstone.Obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) + _, ok = tombstone.Obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) if !ok { utilruntime.HandleError(fmt.Errorf("tombstone contained object that is not an Admin Policy Based External Route %#v", tombstone.Obj)) return } } - if policy != nil { - c.routeQueue.Add(policy) + key, err := cache.MetaNamespaceKeyFunc(obj) + if err != nil { + utilruntime.HandleError(fmt.Errorf("couldn't get key for object %+v: %v", obj, err)) + return } + c.routeQueue.Add(key) } func (c *ExternalGatewayMasterController) onNamespaceAdd(obj interface{}) { @@ -441,7 +404,7 @@ func (c *ExternalGatewayMasterController) processNextNamespaceWorkItem(wg *sync. defer c.namespaceQueue.Done(obj) - err := c.syncNamespace(obj.(*v1.Namespace)) + err := c.mgr.syncNamespace(obj.(*v1.Namespace), c.routeQueue) if err != nil { if c.namespaceQueue.NumRequeues(obj) < maxRetries { klog.V(2).InfoS("Error found while processing namespace %s:%w", obj.(*v1.Namespace), err) @@ -455,53 +418,6 @@ func (c *ExternalGatewayMasterController) processNextNamespaceWorkItem(wg *sync. return true } -func (c *ExternalGatewayMasterController) syncNamespace(namespace *v1.Namespace) error { - _, err := c.namespaceLister.Get(namespace.Name) - if err != nil && !apierrors.IsNotFound(err) { - return err - } - if apierrors.IsNotFound(err) || !namespace.DeletionTimestamp.IsZero() { - // DELETE use case - klog.Infof("Deleting namespace reference %s", namespace.Name) - return c.mgr.processDeleteNamespace(namespace.Name) - } - matches, err := c.mgr.getPoliciesForNamespace(namespace.Name) - if err != nil { - return err - } - cacheInfo, found := c.mgr.getNamespaceInfoFromCache(namespace.Name) - if !found && len(matches) == 0 { - // it's not a namespace being cached already and it is not a target for policies, nothing to do - return nil - } - - defer c.mgr.unlockNamespaceInfoCache(namespace.Name) - if found && cacheInfo.markForDelete { - // namespace exists and has been marked for deletion, this means there should be an event to complete deleting the namespace. - // wait for the namespace to be deleted before recreating it in the cache. - return fmt.Errorf("cannot add namespace %s because it is currently being deleted", namespace.Name) - } - - if !found { - // ADD use case - // new namespace or namespace updated its labels and now match a routing policy - cacheInfo = c.mgr.newNamespaceInfoInCache(namespace.Name) - cacheInfo.Policies = matches - return c.mgr.processAddNamespace(namespace, cacheInfo) - } - - if !cacheInfo.Policies.Equal(matches) { - // UPDATE use case - // policies differ, need to reconcile them - err = c.mgr.processUpdateNamespace(namespace.Name, cacheInfo.Policies, matches, cacheInfo) - if err != nil { - return err - } - return nil - } - return nil -} - func (c *ExternalGatewayMasterController) onPodAdd(obj interface{}) { pod, ok := obj.(*v1.Pod) if !ok { @@ -580,7 +496,7 @@ func (c *ExternalGatewayMasterController) processNextPodWorkItem(wg *sync.WaitGr defer c.podQueue.Done(obj) p := obj.(*v1.Pod) - err := c.syncPod(p) + err := c.mgr.syncPod(p, c.podLister, c.routeQueue) if err != nil { if c.podQueue.NumRequeues(obj) < maxRetries { klog.V(2).InfoS("Error found while processing pod %s/%s:%w", p.Namespace, p.Name, err) @@ -595,37 +511,15 @@ func (c *ExternalGatewayMasterController) processNextPodWorkItem(wg *sync.WaitGr return true } -func (c *ExternalGatewayMasterController) syncPod(pod *v1.Pod) error { - - _, err := c.podLister.Pods(pod.Namespace).Get(pod.Name) - if err != nil && !apierrors.IsNotFound(err) { - return err - } - namespaces := c.mgr.filterNamespacesUsingPodGateway(ktypes.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}) - klog.Infof("Processing pod %s/%s", pod.Namespace, pod.Name) - if apierrors.IsNotFound(err) || !pod.DeletionTimestamp.IsZero() { - // DELETE case - if namespaces.Len() == 0 { - // nothing to do, this pod is not a gateway pod - return nil - } - klog.Infof("Deleting pod gateway %s/%s", pod.Namespace, pod.Name) - return c.mgr.processDeletePod(pod, namespaces) - } - if namespaces.Len() == 0 { - // ADD case: new pod or existing pod that is not a gateway pod and could now be one. - klog.Infof("Adding pod %s/%s", pod.Namespace, pod.Name) - return c.mgr.processAddPod(pod) +// updateStatusAPBExternalRoute updates the CR with the current status of the CR instance, including errors captured while processing the CR during its lifetime +func (c *ExternalGatewayMasterController) updateStatusAPBExternalRoute(externalRoutePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, processedError error) error { + if externalRoutePolicy == nil { + // policy doesnt exist anymore, nothing to do + return nil } - // UPDATE case - klog.Infof("Updating pod gateway %s/%s", pod.Namespace, pod.Name) - return c.mgr.processUpdatePod(pod, namespaces) -} -func (c *ExternalGatewayMasterController) updateStatusAPBExternalRoute(routeName string, processedPolicy *routePolicy, processedError error) error { - - routePolicy, err := c.apbRoutePolicyClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.TODO(), routeName, metav1.GetOptions{}) - if apierrors.IsNotFound(err) { + processedPolicy, err := c.mgr.processExternalRoutePolicy(externalRoutePolicy) + if err != nil { return err } @@ -638,12 +532,20 @@ func (c *ExternalGatewayMasterController) updateStatusAPBExternalRoute(routeName gwIPs = gwIPs.Insert(dynamic.Gateways.UnsortedList()...) } } + // retrieve the policy for update + routePolicy, err := c.apbRoutePolicyClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.TODO(), externalRoutePolicy.Name, metav1.GetOptions{}) + if err != nil && apierrors.IsNotFound(err) { + return processedError + } + if err != nil { + return err + } updateStatus(routePolicy, strings.Join(sets.List(gwIPs), ","), processedError) _, err = c.apbRoutePolicyClient.K8sV1().AdminPolicyBasedExternalRoutes().UpdateStatus(context.TODO(), routePolicy, metav1.UpdateOptions{}) if !apierrors.IsNotFound(err) { return err } - return nil + return processedError } func (c *ExternalGatewayMasterController) GetDynamicGatewayIPsForTargetNamespace(namespaceName string) (sets.Set[string], error) { @@ -663,5 +565,5 @@ func updateStatus(route *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, route.Status.LastTransitionTime = metav1.Time{Time: time.Now()} route.Status.Status = adminpolicybasedrouteapi.SuccessStatus route.Status.Messages = append(route.Status.Messages, fmt.Sprintf("Configured external gateway IPs: %s", gwIPs)) - klog.Infof("Updating Admin Policy Based External Route %s with Status: %s, Message: %s", route.Name, route.Status.Status, route.Status.Messages[len(route.Status.Messages)-1]) + // klog.Infof("Updating Admin Policy Based External Route %s with Status: %s, Message: %s", route.Name, route.Status.Status, route.Status.Messages[len(route.Status.Messages)-1]) } diff --git a/go-controller/pkg/ovn/controller/apbroute/node_controller.go b/go-controller/pkg/ovn/controller/apbroute/node_controller.go index 332853ab5ff..e7f0b29ff52 100644 --- a/go-controller/pkg/ovn/controller/apbroute/node_controller.go +++ b/go-controller/pkg/ovn/controller/apbroute/node_controller.go @@ -8,7 +8,6 @@ import ( "time" v1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" ktypes "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" @@ -265,68 +264,28 @@ func (c *ExternalGatewayNodeController) processNextPolicyWorkItem(wg *sync.WaitG wg.Add(1) defer wg.Done() - obj, shutdown := c.routeQueue.Get() - + key, shutdown := c.routeQueue.Get() if shutdown { return false } + defer c.routeQueue.Done(key) - defer c.routeQueue.Done(obj) - - item := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) - klog.Infof("Processing policy %s", item.Name) - err := c.syncRoutePolicy(item) + item := key.(string) + klog.Infof("Processing policy %s", key) + _, err := c.mgr.syncRoutePolicy(key.(string), c.routeQueue) if err != nil { if c.routeQueue.NumRequeues(item) < maxRetries { - klog.V(2).InfoS("Error found while processing policy: %v", err.Error()) + klog.V(2).InfoS("Error found while processing policy: %s, error: %v", key, err.Error()) c.routeQueue.AddRateLimited(item) return true } - klog.Warningf("Dropping policy %q out of the queue: %v", item.Name, err) + klog.Warningf("Dropping policy %q out of the queue: %v", key, err) utilruntime.HandleError(err) } - c.routeQueue.Forget(obj) + c.routeQueue.Forget(key) return true } -func (c *ExternalGatewayNodeController) syncRoutePolicy(routePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { - _, err := c.routeLister.Get(routePolicy.Name) - if err != nil && !apierrors.IsNotFound(err) { - return err - } - if apierrors.IsNotFound(err) { - // DELETE use case - klog.Infof("Deleting policy %s", routePolicy.Name) - err := c.mgr.processDeletePolicy(routePolicy.Name) - if err != nil { - return fmt.Errorf("failed to delete Admin Policy Based External Route %s:%w", routePolicy.Name, err) - } - klog.Infof("Policy %s deleted", routePolicy.Name) - return nil - } - currentPolicy, found, markedForDeletion := c.mgr.getRoutePolicyFromCache(routePolicy.Name) - if markedForDeletion { - klog.Warningf("Attempting to add or update route policy %s when it has been marked for deletion. Skipping...", routePolicy.Name) - return nil - } - if !found { - // ADD use case - klog.Infof("Adding policy %s", routePolicy.Name) - _, err := c.mgr.processAddPolicy(routePolicy) - if err != nil { - return fmt.Errorf("failed to create Admin Policy Based External Route %s:%w", routePolicy.Name, err) - } - return nil - } - // UPDATE use case - klog.Infof("Updating policy %s", routePolicy.Name) - _, err = c.mgr.processUpdatePolicy(¤tPolicy, routePolicy) - if err != nil { - return fmt.Errorf("failed to update Admin Policy Based External Route %s:%w", routePolicy.Name, err) - } - return nil -} - func (c *ExternalGatewayNodeController) onPolicyAdd(obj interface{}) { policy, ok := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) if !ok { @@ -360,7 +319,6 @@ func (c *ExternalGatewayNodeController) onPolicyUpdate(oldObj, newObj interface{ !newRoutePolicy.GetDeletionTimestamp().IsZero() { return } - c.routeQueue.Add(newObj) } @@ -394,14 +352,12 @@ func (c *ExternalGatewayNodeController) processNextNamespaceWorkItem(wg *sync.Wa defer wg.Done() obj, shutdown := c.namespaceQueue.Get() - if shutdown { return false } - defer c.namespaceQueue.Done(obj) - err := c.syncNamespace(obj.(*v1.Namespace)) + err := c.mgr.syncNamespace(obj.(*v1.Namespace), c.routeQueue) if err != nil { if c.namespaceQueue.NumRequeues(obj) < maxRetries { klog.V(2).InfoS("Error found while processing namespace %s:%w", obj.(*v1.Namespace), err) @@ -415,54 +371,6 @@ func (c *ExternalGatewayNodeController) processNextNamespaceWorkItem(wg *sync.Wa return true } -func (c *ExternalGatewayNodeController) syncNamespace(namespace *v1.Namespace) error { - _, err := c.namespaceLister.Get(namespace.Name) - if err != nil && !apierrors.IsNotFound(err) { - return err - } - if apierrors.IsNotFound(err) || !namespace.DeletionTimestamp.IsZero() { - // DELETE use case - klog.Infof("Deleting namespace reference %s", namespace.Name) - return c.mgr.processDeleteNamespace(namespace.Name) - } - matches, err := c.mgr.getPoliciesForNamespace(namespace.Name) - if err != nil { - return err - } - cacheInfo, found := c.mgr.getNamespaceInfoFromCache(namespace.Name) - if !found && len(matches) == 0 { - // it's not a namespace being cached already and it is not a target for policies, nothing to do - return nil - } - - defer c.mgr.unlockNamespaceInfoCache(namespace.Name) - if found && cacheInfo.markForDelete { - // namespace exists and has been marked for deletion, this means there should be an event to complete deleting the namespace. - // wait for the namespace to be deleted before recreating it in the cache. - return fmt.Errorf("cannot add namespace %s because it is currently being deleted", namespace.Name) - } - - if !found { - // ADD use case - // new namespace or namespace updated its labels and now match a routing policy - cacheInfo = c.mgr.newNamespaceInfoInCache(namespace.Name) - cacheInfo.Policies = matches - return c.mgr.processAddNamespace(namespace, cacheInfo) - } - - if !cacheInfo.Policies.Equal(matches) { - // UPDATE use case - // policies differ, need to reconcile them - err = c.mgr.processUpdateNamespace(namespace.Name, cacheInfo.Policies, matches, cacheInfo) - if err != nil { - return err - } - return nil - } - return nil - -} - func (c *ExternalGatewayNodeController) onPodAdd(obj interface{}) { pod, ok := obj.(*v1.Pod) if !ok { @@ -534,15 +442,13 @@ func (c *ExternalGatewayNodeController) processNextPodWorkItem(wg *sync.WaitGrou defer wg.Done() obj, shutdown := c.podQueue.Get() - if shutdown { return false } - defer c.podQueue.Done(obj) p := obj.(*v1.Pod) - err := c.syncPod(p) + err := c.mgr.syncPod(p, c.podLister, c.routeQueue) if err != nil { if c.podQueue.NumRequeues(obj) < maxRetries { klog.V(2).InfoS("Error found while processing pod %s/%s:%w", p.Namespace, p.Name, err) @@ -557,33 +463,6 @@ func (c *ExternalGatewayNodeController) processNextPodWorkItem(wg *sync.WaitGrou return true } -func (c *ExternalGatewayNodeController) syncPod(pod *v1.Pod) error { - - _, err := c.podLister.Pods(pod.Namespace).Get(pod.Name) - if err != nil && !apierrors.IsNotFound(err) { - return err - } - namespaces := c.mgr.filterNamespacesUsingPodGateway(ktypes.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}) - klog.Infof("Processing pod reference %s/%s", pod.Namespace, pod.Name) - if apierrors.IsNotFound(err) || !pod.DeletionTimestamp.IsZero() { - // DELETE case - if namespaces.Len() == 0 { - // nothing to do, this pod is not a gateway pod - return nil - } - klog.Infof("Deleting pod gateway %s/%s", pod.Namespace, pod.Name) - return c.mgr.processDeletePod(pod, namespaces) - } - if namespaces.Len() == 0 { - // ADD case: new pod or existing pod that is not a gateway pod and could now be one. - klog.Infof("Adding pod reference %s/%s", pod.Namespace, pod.Name) - return c.mgr.processAddPod(pod) - } - // UPDATE case - klog.Infof("Updating pod gateway %s/%s", pod.Namespace, pod.Name) - return c.mgr.processUpdatePod(pod, namespaces) -} - func (c *ExternalGatewayNodeController) GetAdminPolicyBasedExternalRouteIPsForTargetNamespace(namespaceName string) (sets.Set[string], error) { gwIPs, err := c.mgr.getDynamicGatewayIPsForTargetNamespace(namespaceName) if err != nil { From 70161d4f0a6f8545fcfdd2029c6509b534865499 Mon Sep 17 00:00:00 2001 From: jordigilh Date: Mon, 19 Jun 2023 09:55:42 -0400 Subject: [PATCH 2/6] Consolidate gateway IPs from static and dynamic hops before deleting them and update log levels. Signed-off-by: jordigilh --- .../apbroute/external_controller.go | 1 - .../apbroute/external_controller_namespace.go | 10 +- .../apbroute/external_controller_pod.go | 12 +- .../apbroute/external_controller_pod_test.go | 98 +++++++-------- .../apbroute/external_controller_policy.go | 113 ++++++++---------- .../external_controller_policy_test.go | 23 ++-- .../controller/apbroute/master_controller.go | 16 ++- .../ovn/controller/apbroute/network_client.go | 6 +- .../controller/apbroute/node_controller.go | 16 +-- .../pkg/ovn/controller/apbroute/repair.go | 13 +- 10 files changed, 148 insertions(+), 160 deletions(-) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller.go b/go-controller/pkg/ovn/controller/apbroute/external_controller.go index d1693c47f60..357674b698b 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller.go @@ -231,7 +231,6 @@ func (m *externalPolicyManager) getRoutePolicyFromCache(policyName string) (*adm found, markedForDeletion bool ) _ = m.routePolicySyncCache.DoWithLock(policyName, func(policyName string) error { - klog.Infof("Getting route %s", policyName) ri, f := m.routePolicySyncCache.Load(policyName) if !f { return nil diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go index 4bdf831480a..7e1d1c3f5b5 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go @@ -14,7 +14,7 @@ func (m *externalPolicyManager) syncNamespace(namespace *v1.Namespace, routeQueu keysToBeQueued := sets.New[string]() - // get a copy of all policies at this time and see if they include this namespace + // Get a copy of all policies at this time and see if they include this namespace policyKeys := m.routePolicySyncCache.GetKeys() for _, policyName := range policyKeys { @@ -39,7 +39,7 @@ func (m *externalPolicyManager) syncNamespace(namespace *v1.Namespace, routeQueu } } - // check if this namespace is being tracked by policy in its namespace cache + // Check if this namespace is being tracked by policy in its namespace cache cacheInfo, found := m.getNamespaceInfoFromCache(namespace.Name) if found { for policyName := range cacheInfo.Policies { @@ -56,7 +56,7 @@ func (m *externalPolicyManager) syncNamespace(namespace *v1.Namespace, routeQueu return nil } -// must be called with lock on namespaceInfo cache +// Must be called with lock on namespaceInfo cache func (m *externalPolicyManager) applyPolicyToNamespace(namespaceName string, policy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, cacheInfo *namespaceInfo) error { processedPolicy, err := m.processExternalRoutePolicy(policy) @@ -70,7 +70,7 @@ func (m *externalPolicyManager) applyPolicyToNamespace(namespaceName string, pol return nil } -// must be called with lock on namespaceInfo cache +// Must be called with lock on namespaceInfo cache func (m *externalPolicyManager) removePolicyFromNamespace(targetNamespace string, policy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, cacheInfo *namespaceInfo) error { processedPolicy, err := m.processExternalRoutePolicy(policy) if err != nil { @@ -81,7 +81,7 @@ func (m *externalPolicyManager) removePolicyFromNamespace(targetNamespace string return err } - klog.Infof("Deleting APB policy %s in namespace cache %s", policy.Name, targetNamespace) + klog.V(4).InfoS("Deleting APB policy %s in namespace cache %s", policy.Name, targetNamespace) cacheInfo.Policies = cacheInfo.Policies.Delete(policy.Name) if len(cacheInfo.Policies) == 0 { m.namespaceInfoSyncCache.Delete(targetNamespace) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go index 8db67e57490..4a9966c31e8 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go @@ -30,9 +30,9 @@ func (m *externalPolicyManager) syncPod(pod *v1.Pod, podLister corev1listers.Pod if pErr != nil { return pErr } - klog.Infof("Processing gateway pod %s/%s with matching policies %+v", pod.Namespace, pod.Name, policies.UnsortedList()) + klog.V(4).InfoS("Processing gateway pod %s/%s with matching policies %+v", pod.Namespace, pod.Name, policies.UnsortedList()) for policyName := range policies { - klog.V(2).InfoS("Queueing policy %s", policyName) + klog.V(5).InfoS("Queueing policy %s", policyName) routeQueue.Add(policyName) } @@ -92,13 +92,13 @@ func (m *externalPolicyManager) listPoliciesUsingPodGateway(key ktypes.Namespace return nil, err } for _, p := range policies { - klog.Infof("Checking for policy %s to have pod %s", p.Name, key) + klog.V(5).InfoS("Checking for policy %s to have pod %s", p.Name, key) pp, err := m.processExternalRoutePolicy(p) if err != nil { return nil, err } if _, found := pp.dynamicGateways[key]; found { - klog.Infof("Policy %s has pod %s", p.Name, key) + klog.V(5).InfoS("Policy %s has pod %s", p.Name, key) ret = append(ret, p) } @@ -108,7 +108,7 @@ func (m *externalPolicyManager) listPoliciesUsingPodGateway(key ktypes.Namespace func (m *externalPolicyManager) listPoliciesInNamespacesUsingPodGateway(key ktypes.NamespacedName) (sets.Set[string], error) { policies := sets.New[string]() - // iterate through all current namespaces that contain the pod. This is needed in case the pod is deleted from an existing namespace, in which case + // Iterate through all current namespaces that contain the pod. This is needed in case the pod is deleted from an existing namespace, in which case // if we iterated applying the namespace selector in the policies, we would miss the fact that a pod was part of a namespace that is no longer // and we'd miss updating that namespace and removing the pod through the reconciliation of the policy in that namespace. nsList := m.listNamespaceInfoCache() @@ -122,7 +122,7 @@ func (m *externalPolicyManager) listPoliciesInNamespacesUsingPodGateway(key ktyp } m.unlockNamespaceInfoCache(namespaceName) } - // list all namespaces that match the policy, for those new namespaces where the pod now applies + // List all namespaces that match the policy, for those new namespaces where the pod now applies p, err := m.listPoliciesUsingPodGateway(key) if err != nil { return nil, err diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod_test.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod_test.go index 10cf9ca11bf..b4b984c882c 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod_test.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod_test.go @@ -199,36 +199,36 @@ var _ = Describe("OVN External Gateway policy", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, namespaceTest2, pod1}, []runtime.Object{dynamicPolicyTest2, dynamicPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(2)) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(2)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ {Namespace: pod1.Namespace, Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), }, - })) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(Equal( + }, cmpOpts...)) + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicyTest2.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ {Namespace: pod1.Namespace, Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), }, - })) + }, cmpOpts...)) deletePod(pod1, fakeClient) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(2)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5, 1).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5, 1).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{}, - })) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(Equal( + }, cmpOpts...)) + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicyTest2.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{}, - })) + }, cmpOpts...)) }) It("deletes a pod that does not match any policy", func() { @@ -243,12 +243,12 @@ var _ = Describe("OVN External Gateway policy", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, pod1}, []runtime.Object{noMatchPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(noMatchPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{}, - })) + }, cmpOpts...)) deletePod(pod1, fakeClient) Eventually(func() bool { _, err := fakeClient.CoreV1().Pods(pod1.Namespace).Get(context.Background(), pod1.Name, v1.GetOptions{}) @@ -256,12 +256,12 @@ var _ = Describe("OVN External Gateway policy", func() { }).Should(BeTrue()) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(noMatchPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{}, - })) + }, cmpOpts...)) }) It("deletes a pod gateway that is one of two pods that matches two policies to the same target namespace", func() { @@ -270,14 +270,14 @@ var _ = Describe("OVN External Gateway policy", func() { Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) deletePod(pod1, fakeClient) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(overlappingPolicy.Name, dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), + {Namespace: pod2.Namespace, Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), }, - })) + }, cmpOpts...)) }) }) @@ -287,21 +287,21 @@ var _ = Describe("OVN External Gateway policy", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, unmatchPod}, []runtime.Object{dynamicPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{}, - })) + }, cmpOpts...)) updatePodLabels(unmatchPod, pod1.Labels, fakeClient) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: unmatchPod.Name}: newGatewayInfo(sets.New(unmatchPod.Status.PodIPs[0].IP), false), + {Namespace: unmatchPod.Namespace, Name: unmatchPod.Name}: newGatewayInfo(sets.New(unmatchPod.Status.PodIPs[0].IP), false), }, - })) + }, cmpOpts...)) }) It("updates an existing pod gateway to match a new policy that targets the same namespace", func() { @@ -309,37 +309,37 @@ var _ = Describe("OVN External Gateway policy", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, pod2}, []runtime.Object{overlappingPolicy, dynamicPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(2)) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name, overlappingPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), + {Namespace: pod2.Namespace, Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), }, - })) + }, cmpOpts...)) updatePodLabels(pod2, map[string]string{"duplicated": "true"}, fakeClient) - // wait for 2 second to ensure that the pod changed have been reconciled. We are doing this because the outcome of the change should not impact the list of dynamic IPs and + // Wait for 2 second to ensure that the pod changed have been reconciled. We are doing this because the outcome of the change should not impact the list of dynamic IPs and // there is no way to know which of the policies apply specifically to the pod. Eventually(func() bool { p, err := fakeClient.CoreV1().Pods(pod2.Namespace).Get(context.TODO(), pod2.Name, v1.GetOptions{}) Expect(err).NotTo(HaveOccurred()) return reflect.DeepEqual(p.Labels, map[string]string{"duplicated": "true"}) }, 2, 2).Should(BeTrue()) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name, overlappingPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), + {Namespace: pod2.Namespace, Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), }, - })) + }, cmpOpts...)) }) It("updates an existing pod gateway to match a new policy that targets a different namespace", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, namespaceTest2, pod2, pod3}, []runtime.Object{dynamicPolicyForTest2Only, dynamicPolicy}) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(2)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, @@ -347,36 +347,36 @@ var _ = Describe("OVN External Gateway policy", func() { {Namespace: pod2.Namespace, Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), {Namespace: pod3.Namespace, Name: pod3.Name}: newGatewayInfo(sets.New(pod3.Status.PodIPs[0].IP), false), }, - })) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(Equal( + }, cmpOpts...)) + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicyForTest2Only.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{}, - })) + }, cmpOpts...)) updatePodLabels(pod2, map[string]string{"duplicated": "true"}, fakeClient) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ {Namespace: pod3.Namespace, Name: pod3.Name}: newGatewayInfo(sets.New(pod3.Status.PodIPs[0].IP), false), }, - })) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(Equal( + }, cmpOpts...)) + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicyForTest2Only.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ {Namespace: pod2.Namespace, Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), }, - })) + }, cmpOpts...)) }) It("updates an existing pod gateway to match no policies", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, pod1, pod2}, []runtime.Object{dynamicPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, time.Minute).Should(HaveLen(1)) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(1)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, @@ -384,52 +384,52 @@ var _ = Describe("OVN External Gateway policy", func() { {Namespace: "default", Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), {Namespace: "default", Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), }, - })) + }, cmpOpts...)) updatePodLabels(pod1, map[string]string{}, fakeClient) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ {Namespace: "default", Name: pod2.Name}: newGatewayInfo(sets.New(pod2.Status.PodIPs[0].IP), false), }, - })) + }, cmpOpts...)) }) It("updates a pod to match a policy to a single namespace", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, namespaceTest2, pod1}, []runtime.Object{dynamicPolicyForTest2Only, dynamicPolicy}) Eventually(func() []string { return listNamespaceInfo() }, 5).Should(HaveLen(2)) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), + {Namespace: pod1.Namespace, Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), }, - })) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(Equal( + }, cmpOpts...)) + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicyForTest2Only.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ - {Namespace: "default", Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), + {Namespace: pod1.Namespace, Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), }, - })) + }, cmpOpts...)) updatePodLabels(pod1, map[string]string{"key": "pod"}, fakeClient) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(Equal( + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicy.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{ {Namespace: "default", Name: pod1.Name}: newGatewayInfo(sets.New(pod1.Status.PodIPs[0].IP), false), }, - })) - Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(Equal( + }, cmpOpts...)) + Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest2.Name) }, 5).Should(BeComparableTo( &namespaceInfo{ Policies: sets.New(dynamicPolicyForTest2Only.Name), StaticGateways: gatewayInfoList{}, DynamicGateways: map[types.NamespacedName]*gatewayInfo{}, - })) + }, cmpOpts...)) }) }) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go index cb3aa605206..e1a08061c33 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go @@ -20,14 +20,14 @@ import ( ) func (m *externalPolicyManager) syncRoutePolicy(policyName string, routeQueue workqueue.RateLimitingInterface) (*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, error) { - klog.Infof("Processing sync for APB %s", policyName) + klog.V(5).InfoS("Processing sync for APB %s", policyName) routePolicy, err := m.routeLister.Get(policyName) if err != nil && !apierrors.IsNotFound(err) { return nil, err } if apierrors.IsNotFound(err) || !routePolicy.DeletionTimestamp.IsZero() { // DELETE use case - klog.Infof("Deleting policy %s", policyName) + klog.V(4).InfoS("Deleting policy %s", policyName) err = m.processDeletePolicy(policyName) if err != nil { return nil, fmt.Errorf("failed to delete Admin Policy Based External Route %s:%w", policyName, err) @@ -37,7 +37,7 @@ func (m *externalPolicyManager) syncRoutePolicy(policyName string, routeQueue wo currentPolicy, found, _ := m.getRoutePolicyFromCache(routePolicy.Name) if !found { // ADD use case - klog.Infof("Adding policy %s", routePolicy.Name) + klog.V(4).InfoS("Adding policy %s", routePolicy.Name) err := m.processAddPolicy(routePolicy) if err != nil { return routePolicy, fmt.Errorf("failed to create Admin Policy Based External Route %s:%w", routePolicy.Name, err) @@ -47,7 +47,7 @@ func (m *externalPolicyManager) syncRoutePolicy(policyName string, routeQueue wo if reflect.DeepEqual(currentPolicy.Spec, routePolicy.Spec) { // Reconcile changes to namespace or pod - klog.Infof("Reconciling policy %s with updates to namespace or pods", routePolicy.Name) + klog.V(5).InfoS("Reconciling policy %s with updates to namespace or pods", routePolicy.Name) err := m.reconcilePolicyWithNamespacesAndPods(currentPolicy) if err != nil { return routePolicy, fmt.Errorf("failed to create Admin Policy Based External Route %s:%w", routePolicy.Name, err) @@ -55,7 +55,7 @@ func (m *externalPolicyManager) syncRoutePolicy(policyName string, routeQueue wo return routePolicy, nil } // UPDATE policy use case - klog.Infof("Updating policy %s", routePolicy.Name) + klog.V(4).InfoS("Updating policy %s", routePolicy.Name) err = m.processUpdatePolicy(currentPolicy, routePolicy) if err != nil { return routePolicy, fmt.Errorf("failed to update Admin Policy Based External Route %s:%w", routePolicy.Name, err) @@ -66,7 +66,6 @@ func (m *externalPolicyManager) syncRoutePolicy(policyName string, routeQueue wo // processAddPolicy takes in an existing policy and reconciles it. To do that, it aggregates the IPs from the static hops and retrieves the IPs from the pods resulting from applying the // namespace and pod selectors in the dynamic hops. -// The last step is to store the new policy in the route policy cache so that it can be used in the future to compare against changes in its spec. func (m *externalPolicyManager) processAddPolicy(routePolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { // it's a new policy @@ -102,7 +101,7 @@ func (m *externalPolicyManager) hasPolicyInNamespace(policyName, namespaceName s nsInfo, found := m.getNamespaceInfoFromCache(namespaceName) if !found { - klog.Infof("Namespace %s not found while consolidating namespaces using policy %s", namespaceName, policyName) + klog.V(5).InfoS("Namespace %s not found while consolidating namespaces using policy %s", namespaceName, policyName) return false } defer m.unlockNamespaceInfoCache(namespaceName) @@ -114,7 +113,7 @@ func (m *externalPolicyManager) processPolicyDiscrepancyInNamespace(nsName strin discrepancyFunc func(string, *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, *namespaceInfo) error) error { cacheInfo, found := m.getNamespaceInfoFromCache(nsName) if !found { - klog.Infof("Namespace %s not found in cache, creating", nsName) + klog.V(5).InfoS("Namespace %s not found in cache, creating", nsName) cacheInfo = m.newNamespaceInfoInCache(nsName) } defer m.unlockNamespaceInfoCache(nsName) @@ -132,15 +131,15 @@ func (m *externalPolicyManager) reconcilePolicyWithNamespacesAndPods(routePolicy targetNs.Insert(ns.Name) } - klog.Infof("Reconciling namespaces %+v for policy %s", strings.Join(targetNs.UnsortedList(), ", "), routePolicy.Name) + klog.V(4).InfoS("Reconciling namespaces %+v for policy %s", strings.Join(targetNs.UnsortedList(), ", "), routePolicy.Name) // reconcile Namespaces currentNs, err := m.listNamespacesWithPolicy(routePolicy.Name) if err != nil { return err } - klog.Infof("List of existing namespaces with policy %s: %s", routePolicy.Name, strings.Join(currentNs.UnsortedList(), ", ")) + klog.V(5).Infof("List of existing namespaces with policy %s: %s", routePolicy.Name, strings.Join(currentNs.UnsortedList(), ", ")) if len(currentNs.Difference(targetNs)) > 0 { - klog.Infof("Removing APB policy %s in namespaces %+v", routePolicy.Name, currentNs.Difference(targetNs)) + klog.V(4).Infof("Removing APB policy %s in namespaces %+v", routePolicy.Name, currentNs.Difference(targetNs)) } // namespaces where the policy should not be applied anymore. for nsName := range currentNs.Difference(targetNs) { @@ -150,7 +149,7 @@ func (m *externalPolicyManager) reconcilePolicyWithNamespacesAndPods(routePolicy } } if len(targetNs.Difference(currentNs)) > 0 { - klog.Infof("Adding policy %s to namespaces %+v", routePolicy.Name, targetNs.Difference(currentNs)) + klog.V(4).Infof("Adding policy %s to namespaces %+v", routePolicy.Name, targetNs.Difference(currentNs)) } // namespaces where the policy should now be applied for nsName := range targetNs.Difference(currentNs) { @@ -159,7 +158,7 @@ func (m *externalPolicyManager) reconcilePolicyWithNamespacesAndPods(routePolicy return err } } - klog.Infof("Reconciling policy %s against matching namespaces %+v", routePolicy.Name, targetNs.Intersection(currentNs)) + klog.V(4).Infof("Reconciling policy %s against matching namespaces %+v", routePolicy.Name, targetNs.Intersection(currentNs)) // namespaces where the policy still applies. In this case validate the dynamic hops for nsName := range targetNs.Intersection(currentNs) { err = m.processReconciliationWithNamespace(nsName) @@ -173,16 +172,15 @@ func (m *externalPolicyManager) reconcilePolicyWithNamespacesAndPods(routePolicy func (m *externalPolicyManager) processReconciliationWithNamespace(nsName string) error { cacheInfo, found := m.getNamespaceInfoFromCache(nsName) if !found { - klog.Infof("Namespace %s not found in cache", nsName) + klog.V(5).InfoS("Namespace %s not found in cache", nsName) return nil } - klog.Infof("Retrieving cache for ns %s: +%v dynamic", nsName, cacheInfo.DynamicGateways) defer m.unlockNamespaceInfoCache(nsName) policies := make([]*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, 0) for policy := range cacheInfo.Policies { cachedPolicy, found, markedForDeletion := m.getRoutePolicyFromCache(policy) if !found { - klog.Infof("Policy %s not found while calculating all policies in a namespace", policy) + klog.V(5).InfoS("Policy %s not found while calculating all policies in a namespace", policy) continue } if markedForDeletion { @@ -200,12 +198,10 @@ func (m *externalPolicyManager) processReconciliationWithNamespace(nsName string // Consolidate all dynamic gateway IPs from the policies into a single map for _, pp := range processedPolicies { for k, v := range pp.dynamicGateways { - klog.Infof("Processing %+v", k) allProcessedGWIPs[k] = v } } newGateways, invalidGWIPs, ipsToKeep := m.calculateDynamicGateways(allProcessedGWIPs, cacheInfo.DynamicGateways) - klog.Infof("[%s] Deleting gateway IPs %+v, keeping %+v", nsName, invalidGWIPs, ipsToKeep) // delete all invalid GW IP references in the NorthBoundDB (master controller) or conntrack (node_controller) err = m.netClient.deleteGatewayIPs(nsName, invalidGWIPs, ipsToKeep) if err != nil { @@ -213,7 +209,6 @@ func (m *externalPolicyManager) processReconciliationWithNamespace(nsName string } // proceed to add the GW IPs again for _, gatewayInfo := range newGateways { - klog.Infof("[%s] Adding gateway IPs %+v", nsName, gatewayInfo) err = m.addGWRoutesForNamespace(nsName, gatewayInfoList{gatewayInfo}) if err != nil { return err @@ -224,7 +219,7 @@ func (m *externalPolicyManager) processReconciliationWithNamespace(nsName string } func (m *externalPolicyManager) calculateDynamicGateways(allProcessedGWIPs, cachedDynamicGWInfo map[ktypes.NamespacedName]*gatewayInfo) (map[ktypes.NamespacedName]*gatewayInfo, sets.Set[string], sets.Set[string]) { - klog.Infof("Processed policies: %+v", allProcessedGWIPs) + klog.V(5).InfoS("Processed policies: %+v", allProcessedGWIPs) // In order to delete the invalid GWs, the logic has to collect all the valid GW IPs as well as the invalid ones. // This is due to implementation requirements by the network clients: for the NB interaction (master controller), only the invalid GWs is needed // but when interacting with the conntrack (node controller), it requires to use only the valid GWs due to a white listing approach @@ -237,12 +232,12 @@ func (m *externalPolicyManager) calculateDynamicGateways(allProcessedGWIPs, cach v2, ok := cachedDynamicGWInfo[k] if ok && !v1.Equal(v2) { // podGW not found or its gatewayInfo does not match, remove it - klog.Infof("PP to cacheInfo: invalid GW IP %+v compared to %+v", v2, v1) + klog.V(5).InfoS("PP to cacheInfo: invalid GW IP %+v compared to %+v", v2, v1) invalidGWIPs.Insert(v2.Gateways.UnsortedList()...) continue } // store th gatewayInfo in the map - klog.Infof("Storing %s when removing pod GWs ", k) + klog.V(5).InfoS("Storing %s when removing pod GWs ", k) newGateways[k] = v1 ipsToKeep.Insert(v1.Gateways.UnsortedList()...) } @@ -252,7 +247,7 @@ func (m *externalPolicyManager) calculateDynamicGateways(allProcessedGWIPs, cach for k, v := range cachedDynamicGWInfo { if _, ok := allProcessedGWIPs[k]; !ok { // IP not found in the processed policies, it means the pod gateway information is no longer applicable - klog.Infof("CacheInfo-> GW IP %+v not found in processed policies", k) + klog.V(5).InfoS("CacheInfo-> GW IP %+v not found in processed policies", k) invalidGWIPs.Insert(v.Gateways.UnsortedList()...) continue } @@ -303,16 +298,16 @@ func (m *externalPolicyManager) processDeletePolicy(policyName string) error { // mark the policy for deletion. // if it's already marked continue processing the delete action as this could be a retry attempt from a previous failed delete run. // if it's no longer in the cache, return nil - klog.Infof("Getting route %s and marking it for deletion", policyName) + klog.V(5).InfoS("Getting route %s and marking it for deletion", policyName) routePolicy, found := m.getAndMarkRoutePolicyForDeletionInCache(policyName) if !found { - klog.Infof("Policy %s not found", policyName) + klog.V(5).InfoS("Policy %s not found", policyName) return nil } for _, ns := range m.getAllNamespacesNamesInCache() { cacheInfo, found := m.getNamespaceInfoFromCache(ns) if !found { - klog.Infof("Attempting to delete policy %s from a namespace that does not exist %s", routePolicy.Name, ns) + klog.V(5).InfoS("Attempting to delete policy %s from a namespace that does not exist %s", routePolicy.Name, ns) continue } var err error @@ -324,15 +319,12 @@ func (m *externalPolicyManager) processDeletePolicy(policyName string) error { return err } } - klog.Infof("Proceeding to delete route %s from cache", routePolicy.Name) + klog.V(5).InfoS("Proceeding to delete route %s from cache", routePolicy.Name) err := m.deleteRoutePolicyFromCache(routePolicy.Name) if err != nil { return err } - klog.Infof("Deleted Admin Policy Based External Route %s", routePolicy.Name) - // Requeue policy after delete to reconcile any namespaces that still reference it. - // This scenario can happen when a routine retrieves the policy reference before it is marked for deletion. - // routeQueue.Add(&routePolicy) + klog.V(4).InfoS("Deleted Admin Policy Based External Route %s", routePolicy.Name) return nil } @@ -407,6 +399,9 @@ func (m *externalPolicyManager) deletePolicyInNamespace(namespaceName, policyNam if !found { return fmt.Errorf("policy %s not found", policyName) } + allGWIPsToDelete := sets.New[string]() + allGWIPsToKeep := sets.New[string]() + pp, err := m.processExternalRoutePolicy(policy) if err != nil { return err @@ -444,14 +439,13 @@ func (m *externalPolicyManager) deletePolicyInNamespace(namespaceName, policyNam invalidGWIPs := gwInfo.Gateways.Difference(coexistingIPs) // Filter out the IPs from the coexisting list that are to be kept by calculating the difference between the coexising and those IPs that are to be deleted and not coexisting at the same time. ipsToKeep := coexistingIPs.Difference(invalidGWIPs) - klog.Infof("Coexisting %s, invalid %s, ipsToKeep %s", strings.Join(sets.List(coexistingIPs), ","), strings.Join(sets.List(invalidGWIPs), ","), strings.Join(sets.List(ipsToKeep), ",")) + klog.V(4).InfoS("Coexisting %s, invalid %s, ipsToKeep %s", strings.Join(sets.List(coexistingIPs), ","), strings.Join(sets.List(invalidGWIPs), ","), strings.Join(sets.List(ipsToKeep), ",")) if len(invalidGWIPs) == 0 { continue } - err := m.netClient.deleteGatewayIPs(namespaceName, invalidGWIPs, ipsToKeep) - if err != nil { - return err - } + allGWIPsToDelete = allGWIPsToDelete.Union(invalidGWIPs) + allGWIPsToKeep = allGWIPsToKeep.Union(ipsToKeep) + if gwInfo.Gateways.Difference(invalidGWIPs).Len() == 0 { cacheInfo.StaticGateways = cacheInfo.StaticGateways.Delete(gwInfo) continue @@ -490,24 +484,26 @@ func (m *externalPolicyManager) deletePolicyInNamespace(namespaceName, policyNam invalidGWIPs := gwInfo.Gateways.Difference(coexistingIPs) // Filter out the IPs from the coexisting list that are to be kept by calculating the difference between the coexising and those IPs that are to be deleted and not coexisting at the same time. ipsToKeep := coexistingIPs.Difference(invalidGWIPs) - klog.Infof("Coexisting %s, invalid %s, ipsToKeep %s", strings.Join(sets.List(coexistingIPs), ","), strings.Join(sets.List(invalidGWIPs), ","), strings.Join(sets.List(ipsToKeep), ",")) + klog.V(4).InfoS("Coexisting %s, invalid %s, ipsToKeep %s", strings.Join(sets.List(coexistingIPs), ","), strings.Join(sets.List(invalidGWIPs), ","), strings.Join(sets.List(ipsToKeep), ",")) if len(invalidGWIPs) == 0 { continue } - err := m.netClient.deleteGatewayIPs(namespaceName, invalidGWIPs, ipsToKeep) - if err != nil { - return err - } + allGWIPsToDelete = allGWIPsToDelete.Union(invalidGWIPs) + allGWIPsToKeep = allGWIPsToKeep.Union(ipsToKeep) + if gwInfo.Gateways.Difference(invalidGWIPs).Len() == 0 { // delete cached information for the pod gateway - klog.Infof("Deleting cache entry for dynamic hop %s", gwPodNamespacedName) + klog.V(5).InfoS("Deleting cache entry for dynamic hop %s", gwPodNamespacedName) delete(cacheInfo.DynamicGateways, gwPodNamespacedName) continue } - klog.Infof("Deleting dynamic hop IPs in gateway info %s", strings.Join(sets.List(invalidGWIPs), ",")) + klog.V(4).InfoS("Deleting dynamic hop IPs in gateway info %s", strings.Join(sets.List(invalidGWIPs), ",")) gwInfo.Gateways.Delete(invalidGWIPs.UnsortedList()...) } - return nil + // Processing IPs + // Delete them in bulk since the conntrack is using a white list approach (IPs in AllGWIPsToKeep) to filter out the entries that need to be removed. + // This is not a non-issue for the north bound DB since the client uses the allGWIPsToDelete set to remove the IPs. + return m.netClient.deleteGatewayIPs(namespaceName, allGWIPsToDelete, allGWIPsToKeep) } // applyProcessedPolicyToNamespace applies the gateway IPs derived from the processed policy to a namespace and updates the cache information for the namespace. @@ -535,7 +531,7 @@ func (m *externalPolicyManager) applyProcessedPolicyToNamespace(namespaceName, p cacheInfo.DynamicGateways[pod] = newGatewayInfo(info.Gateways.items, info.BFDEnabled) } - klog.Infof("Adding policy %s to namespace %s", policyName, namespaceName) + klog.V(4).InfoS("Adding policy %s to namespace %s", policyName, namespaceName) cacheInfo.Policies = cacheInfo.Policies.Insert(policyName) return nil } @@ -546,7 +542,7 @@ func (m *externalPolicyManager) applyProcessedPolicyToNamespace(namespaceName, p // * Apply the static and dynamic hop entries in the namespaces impacted by the updated version of the policy that are in the updated version but not in the current version. // * Store the updated policy in the route policy cache. func (m *externalPolicyManager) processUpdatePolicy(currentPolicy, updatedPolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { - klog.Infof("Processing update for Admin Policy Based External Route '%s'", currentPolicy.Name) + klog.V(5).InfoS("Processing update for Admin Policy Based External Route '%s'", currentPolicy.Name) // To update the policies, first we'll process the diff between old and new and remove the discrepancies that are not found in the new object. // Afterwards, we'll process the diff between the new and the old and apply the new policies not found in the old policy, ensuring that we are not reduplicating the gatewayInfo. @@ -562,11 +558,6 @@ func (m *externalPolicyManager) processUpdatePolicy(currentPolicy, updatedPolicy // update the cache to ensure it reflects the latest copy return m.storeRoutePolicyInCache(updatedPolicy) - // if err != nil { - // return err - // } - // klog.Infof("Updated Admin Policy Based External Route %s", currentPolicy.Name) - // return m.processExternalRoutePolicy(updatedPolicy) } func (m *externalPolicyManager) applyUpdatesInRoutePolicy(currentPolicy, newPolicy *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) error { @@ -596,14 +587,14 @@ func (m *externalPolicyManager) applyUpdatesInRoutePolicy(currentPolicy, newPoli if err != nil { return err } - // retrieve all new namespaces + // Retrieve all new namespaces nsList, err := m.listNamespacesBySelector(&newPolicy.Spec.From.NamespaceSelector) if err != nil { return err } for _, ns := range nsList { if additionalNamespaces.Has(ns.Name) { - // policy has already been fully applied to this namespace by the previous operation + // Policy has already been fully applied to this namespace by the previous operation continue } cacheInfo, found := m.getNamespaceInfoFromCache(ns.Name) @@ -625,8 +616,8 @@ func (m *externalPolicyManager) removeDiscrepanciesInRoutePolicy(currentPolicy, if err != nil { return err } - klog.Infof("Removing discrepancies between current and updated policy: %+v, %+v, %+v", unmatchingNamespaces, unmatchingStaticHops, unmatchingDynamicHops) - // delete the namespaces where this policy no longer applies + klog.V(4).InfoS("Removing discrepancies between current and updated policy: namespaces %+v, static hop IPs %+v, dynamic hop IPs %+v", unmatchingNamespaces, unmatchingStaticHops, unmatchingDynamicHops) + // Delete the namespaces where this policy no longer applies for unmatchNs := range unmatchingNamespaces { cacheInfo, found := m.getNamespaceInfoFromCache(unmatchNs) if !found { @@ -640,7 +631,7 @@ func (m *externalPolicyManager) removeDiscrepanciesInRoutePolicy(currentPolicy, } } - // delete the hops that no longer apply from all the current policy's applicable namespaces + // Delete the hops that no longer apply from all the current policy's applicable namespaces processedStaticHops, err := m.processStaticHopsGatewayInformation(unmatchingStaticHops) if err != nil { return err @@ -649,14 +640,14 @@ func (m *externalPolicyManager) removeDiscrepanciesInRoutePolicy(currentPolicy, if err != nil { return err } - // retrieve all current namespaces + // Retrieve all current namespaces nsList, err := m.listNamespacesBySelector(¤tPolicy.Spec.From.NamespaceSelector) if err != nil { return err } for _, ns := range nsList { if unmatchingNamespaces.Has(ns.Name) { - // policy has already been deleted in this namespace by the previous operation + // Policy has already been deleted in this namespace by the previous operation continue } cacheInfo, found := m.getNamespaceInfoFromCache(ns.Name) @@ -723,7 +714,7 @@ func (m *externalPolicyManager) processDynamicHopsGatewayInformation(hops []*adm if err != nil { return nil, err } - // if we found any gateways then we need to update current pods routing in the relevant namespace + // If we found any gateways then we need to update current pods routing in the relevant namespace if len(foundGws) == 0 { klog.Warningf("No valid gateway IPs found for requested external gateway pod %s/%s", pod.Namespace, pod.Name) continue @@ -749,14 +740,14 @@ func (m *externalPolicyManager) processExternalRoutePolicy(policy *adminpolicyba errors = append(errors, err) } if len(staticGWInfo) > 0 { - klog.Infof("Found static hops for policy %s:%+v", policy.Name, staticGWInfo) + klog.V(5).InfoS("Found static hops for policy %s:%+v", policy.Name, staticGWInfo) } dynamicGWInfo, err := m.processDynamicHopsGatewayInformation(policy.Spec.NextHops.DynamicHops) if err != nil { errors = append(errors, err) } if len(dynamicGWInfo) > 0 { - klog.Infof("Found dynamic hops for policy %s:%+v", policy.Name, dynamicGWInfo) + klog.V(5).InfoS("Found dynamic hops for policy %s:%+v", policy.Name, dynamicGWInfo) } if len(errors) > 0 { return nil, kerrors.NewAggregate(errors) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy_test.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy_test.go index deb77e501bc..9a72a92ef68 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy_test.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy_test.go @@ -2,6 +2,7 @@ package apbroute import ( "context" + . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -206,9 +207,9 @@ var _ = Describe("OVN External Gateway policy", func() { Eventually(func(g Gomega) { p, found, _ := externalController.mgr.getRoutePolicyFromCache(multipleMatchPolicy.Name) g.Expect(found).To(BeTrue()) - g.Expect(p.Spec).To(BeEquivalentTo(multipleMatchPolicy.Spec)) + g.Expect(p.Spec).To(BeComparableTo(multipleMatchPolicy.Spec, cmpOpts...)) }, 5).Should(Succeed()) - Eventually(listNamespaceInfo(), 5).Should(HaveLen(3)) + Eventually(listNamespaceInfo, 5).Should(HaveLen(3)) expected := &namespaceInfo{ Policies: sets.New(multipleMatchPolicy.Name), StaticGateways: gatewayInfoList{newGatewayInfo(sets.New(staticHopGWIP), false)}, @@ -230,7 +231,7 @@ var _ = Describe("OVN External Gateway policy", func() { initController([]runtime.Object{namespaceTest2, namespaceDefault, pod1}, []runtime.Object{dynamicPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) - Eventually(listNamespaceInfo(), 5).Should(HaveLen(0)) + Eventually(listNamespaceInfo, 5).Should(HaveLen(0)) }) It("registers a new policy with multiple dynamic and static GWs and bfd enabled on all gateways", func() { @@ -245,7 +246,7 @@ var _ = Describe("OVN External Gateway policy", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, pod1, pod2, pod3, pod4, pod5, pod6}, []runtime.Object{staticMultiIPPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) - Eventually(listNamespaceInfo(), 5).Should(HaveLen(1)) + Eventually(listNamespaceInfo, 5).Should(HaveLen(1)) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) @@ -275,7 +276,7 @@ var _ = Describe("OVN External Gateway policy", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, pod1}, []runtime.Object{staticPolicy, dynamicPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5). Should(HaveLen(2)) - Eventually(listNamespaceInfo(), 5).Should(HaveLen(1)) + Eventually(listNamespaceInfo, 5).Should(HaveLen(1)) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( @@ -307,7 +308,7 @@ var _ = Describe("OVN External Gateway policy", func() { Eventually(func() []string { return listRoutePolicyInCache() }, 5). Should(HaveLen(4)) - Eventually(listNamespaceInfo(), 5).Should(HaveLen(1)) + Eventually(listNamespaceInfo, 5).Should(HaveLen(1)) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) @@ -548,7 +549,7 @@ var _ = Describe("OVN External Gateway policy", func() { initController([]runtime.Object{namespaceDefault, namespaceTest}, []runtime.Object{staticMultiIPPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) - Eventually(listNamespaceInfo(), 5).Should(HaveLen(1)) + Eventually(listNamespaceInfo, 5).Should(HaveLen(1)) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) @@ -615,7 +616,7 @@ var _ = Describe("OVN External Gateway policy", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, pod1, pod2, pod3, pod4, pod5, pod6}, []runtime.Object{staticMultiIPPolicy, staticPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(2)) - Eventually(listNamespaceInfo(), 5).Should(HaveLen(1)) + Eventually(listNamespaceInfo, 5).Should(HaveLen(1)) expected := &namespaceInfo{ Policies: sets.New(staticMultiIPPolicy.Name, staticPolicy.Name), StaticGateways: gatewayInfoList{ @@ -652,7 +653,7 @@ var _ = Describe("OVN External Gateway policy", func() { Expect(err).NotTo(HaveOccurred()) Eventually(func() v1.Time { - // retrieve the CR and ensure the last update timestamp is different before comparing it against the slice. + // Retrieve the CR and ensure the last update timestamp is different before comparing it against the slice. p, err = fakeRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.TODO(), staticMultiIPPolicy.Name, v1.GetOptions{}) Expect(err).NotTo(HaveOccurred()) return p.Status.LastTransitionTime @@ -667,7 +668,7 @@ var _ = Describe("OVN External Gateway policy", func() { It("validates that changing the BFD setting in a static hop will trigger an update", func() { initController([]runtime.Object{namespaceDefault, namespaceTest}, []runtime.Object{staticPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) - Eventually(listNamespaceInfo(), 5).Should(HaveLen(1)) + Eventually(listNamespaceInfo, 5).Should(HaveLen(1)) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) @@ -710,7 +711,7 @@ var _ = Describe("OVN External Gateway policy", func() { It("validates that changing the BFD setting in a dynamic hop will trigger an update", func() { initController([]runtime.Object{namespaceDefault, namespaceTest, pod1}, []runtime.Object{dynamicPolicy}) Eventually(func() []string { return listRoutePolicyInCache() }, 5).Should(HaveLen(1)) - Eventually(listNamespaceInfo(), 5).Should(HaveLen(1)) + Eventually(listNamespaceInfo, 5).Should(HaveLen(1)) Eventually(func() *namespaceInfo { return getNamespaceInfo(namespaceTest.Name) }, 5).Should(BeComparableTo( diff --git a/go-controller/pkg/ovn/controller/apbroute/master_controller.go b/go-controller/pkg/ovn/controller/apbroute/master_controller.go index 47186479ca9..26e2836492b 100644 --- a/go-controller/pkg/ovn/controller/apbroute/master_controller.go +++ b/go-controller/pkg/ovn/controller/apbroute/master_controller.go @@ -48,8 +48,6 @@ type ExternalGatewayMasterController struct { stopCh <-chan struct{} // route policies - - // routerInformer v1apbinformer.AdminPolicyBasedExternalRouteInformer routeLister adminpolicybasedroutelisters.AdminPolicyBasedExternalRouteLister routeSynced cache.InformerSynced routeQueue workqueue.RateLimitingInterface @@ -175,7 +173,7 @@ func NewExternalMasterController( func (c *ExternalGatewayMasterController) Run(threadiness int) { defer utilruntime.HandleCrash() - klog.Infof("Starting Admin Policy Based Route Controller") + klog.V(4).InfoS("Starting Admin Policy Based Route Controller") c.routePolicyInformer.Start(c.stopCh) @@ -199,7 +197,7 @@ func (c *ExternalGatewayMasterController) Run(threadiness int) { } syncWg.Wait() - klog.Infof("Repairing Admin Policy Based External Route Services") + klog.V(4).InfoS("Repairing Admin Policy Based External Route Services") c.repair() wg := &sync.WaitGroup{} @@ -249,7 +247,7 @@ func (c *ExternalGatewayMasterController) processNextPolicyWorkItem(wg *sync.Wai defer c.routeQueue.Done(key) - klog.Infof("Processing policy %s", key) + klog.V(4).InfoS("Processing policy %s", key) policy, err := c.mgr.syncRoutePolicy(key.(string), c.routeQueue) if err != nil { klog.Errorf("Failed to sync APB policy %s: %v", key, err) @@ -258,7 +256,7 @@ func (c *ExternalGatewayMasterController) processNextPolicyWorkItem(wg *sync.Wai err = c.updateStatusAPBExternalRoute(policy, err) if err != nil { if c.routeQueue.NumRequeues(key) < maxRetries { - klog.V(2).InfoS("Error found while processing policy: %w", err) + klog.V(4).InfoS("Error found while processing policy: %w", err) c.routeQueue.AddRateLimited(key) return true } @@ -407,7 +405,7 @@ func (c *ExternalGatewayMasterController) processNextNamespaceWorkItem(wg *sync. err := c.mgr.syncNamespace(obj.(*v1.Namespace), c.routeQueue) if err != nil { if c.namespaceQueue.NumRequeues(obj) < maxRetries { - klog.V(2).InfoS("Error found while processing namespace %s:%w", obj.(*v1.Namespace), err) + klog.V(4).InfoS("Error found while processing namespace %s:%w", obj.(*v1.Namespace), err) c.namespaceQueue.AddRateLimited(obj) return true } @@ -499,7 +497,7 @@ func (c *ExternalGatewayMasterController) processNextPodWorkItem(wg *sync.WaitGr err := c.mgr.syncPod(p, c.podLister, c.routeQueue) if err != nil { if c.podQueue.NumRequeues(obj) < maxRetries { - klog.V(2).InfoS("Error found while processing pod %s/%s:%w", p.Namespace, p.Name, err) + klog.V(4).InfoS("Error found while processing pod %s/%s:%w", p.Namespace, p.Name, err) c.podQueue.AddRateLimited(obj) return true } @@ -565,5 +563,5 @@ func updateStatus(route *adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute, route.Status.LastTransitionTime = metav1.Time{Time: time.Now()} route.Status.Status = adminpolicybasedrouteapi.SuccessStatus route.Status.Messages = append(route.Status.Messages, fmt.Sprintf("Configured external gateway IPs: %s", gwIPs)) - // klog.Infof("Updating Admin Policy Based External Route %s with Status: %s, Message: %s", route.Name, route.Status.Status, route.Status.Messages[len(route.Status.Messages)-1]) + klog.V(4).InfoS("Updating Admin Policy Based External Route %s with Status: %s, Message: %s", route.Name, route.Status.Status, route.Status.Messages[len(route.Status.Messages)-1]) } diff --git a/go-controller/pkg/ovn/controller/apbroute/network_client.go b/go-controller/pkg/ovn/controller/apbroute/network_client.go index b63362200be..db69e04c8be 100644 --- a/go-controller/pkg/ovn/controller/apbroute/network_client.go +++ b/go-controller/pkg/ovn/controller/apbroute/network_client.go @@ -256,7 +256,7 @@ func (nb *northBoundClient) addGWRoutesForPod(gateways []*gatewayInfo, podIfAddr routesAdded := 0 portPrefix, err := nb.extSwitchPrefix(node) if err != nil { - klog.Infof("Failed to find ext switch prefix for %s %v", node, err) + klog.Warningf("Failed to find ext switch prefix for %s %v", node, err) return err } @@ -442,7 +442,7 @@ func (nb *northBoundClient) updateExternalGWInfoCacheForPodIPWithGatewayIP(podIP portPrefix, err := nb.extSwitchPrefix(nodeName) if err != nil { - klog.Infof("Failed to find ext switch prefix for %s %v", nodeName, err) + klog.Warningf("Failed to find ext switch prefix for %s %v", nodeName, err) return err } if bfdEnabled { @@ -727,7 +727,7 @@ func (c *conntrackClient) deleteGatewayIPs(namespaceName string, _, toBeKept set var wg sync.WaitGroup wg.Add(len(toBeKept)) validMACs := sync.Map{} - klog.Infof("Keeping conntrack entries in namespace %s with gateway IPs %s", namespaceName, strings.Join(sets.List(toBeKept), ",")) + klog.V(4).InfoS("Keeping conntrack entries in namespace %s with gateway IPs %s", namespaceName, strings.Join(sets.List(toBeKept), ",")) for gwIP := range toBeKept { go func(gwIP string) { defer wg.Done() diff --git a/go-controller/pkg/ovn/controller/apbroute/node_controller.go b/go-controller/pkg/ovn/controller/apbroute/node_controller.go index e7f0b29ff52..2f99986b4aa 100644 --- a/go-controller/pkg/ovn/controller/apbroute/node_controller.go +++ b/go-controller/pkg/ovn/controller/apbroute/node_controller.go @@ -134,25 +134,25 @@ func NewExternalNodeController( func (c *ExternalGatewayNodeController) Run(threadiness int) { defer utilruntime.HandleCrash() - klog.Infof("Starting Admin Policy Based Route Node Controller") + klog.V(4).InfoS("Starting Admin Policy Based Route Node Controller") c.routePolicyInformer.Start(c.stopCh) if !cache.WaitForNamedCacheSync("apbexternalroutenamespaces", c.stopCh, c.namespaceSynced) { utilruntime.HandleError(fmt.Errorf("timed out waiting for caches to sync")) - klog.Infof("Synchronization failed") + klog.V(4).InfoS("Synchronization failed") return } if !cache.WaitForNamedCacheSync("apbexternalroutepods", c.stopCh, c.podSynced) { utilruntime.HandleError(fmt.Errorf("timed out waiting for caches to sync")) - klog.Infof("Synchronization failed") + klog.V(4).InfoS("Synchronization failed") return } if !cache.WaitForNamedCacheSync("adminpolicybasedexternalroutes", c.stopCh, c.routeSynced) { utilruntime.HandleError(fmt.Errorf("timed out waiting for caches to sync")) - klog.Infof("Synchronization failed") + klog.V(4).InfoS("Synchronization failed") return } @@ -271,11 +271,11 @@ func (c *ExternalGatewayNodeController) processNextPolicyWorkItem(wg *sync.WaitG defer c.routeQueue.Done(key) item := key.(string) - klog.Infof("Processing policy %s", key) + klog.V(4).InfoS("Processing policy %s", key) _, err := c.mgr.syncRoutePolicy(key.(string), c.routeQueue) if err != nil { if c.routeQueue.NumRequeues(item) < maxRetries { - klog.V(2).InfoS("Error found while processing policy: %s, error: %v", key, err.Error()) + klog.V(4).InfoS("Error found while processing policy: %s, error: %v", key, err.Error()) c.routeQueue.AddRateLimited(item) return true } @@ -360,7 +360,7 @@ func (c *ExternalGatewayNodeController) processNextNamespaceWorkItem(wg *sync.Wa err := c.mgr.syncNamespace(obj.(*v1.Namespace), c.routeQueue) if err != nil { if c.namespaceQueue.NumRequeues(obj) < maxRetries { - klog.V(2).InfoS("Error found while processing namespace %s:%w", obj.(*v1.Namespace), err) + klog.V(4).InfoS("Error found while processing namespace %s:%w", obj.(*v1.Namespace), err) c.namespaceQueue.AddRateLimited(obj) return true } @@ -451,7 +451,7 @@ func (c *ExternalGatewayNodeController) processNextPodWorkItem(wg *sync.WaitGrou err := c.mgr.syncPod(p, c.podLister, c.routeQueue) if err != nil { if c.podQueue.NumRequeues(obj) < maxRetries { - klog.V(2).InfoS("Error found while processing pod %s/%s:%w", p.Namespace, p.Name, err) + klog.V(4).InfoS("Error found while processing pod %s/%s:%w", p.Namespace, p.Name, err) c.podQueue.AddRateLimited(obj) return true } diff --git a/go-controller/pkg/ovn/controller/apbroute/repair.go b/go-controller/pkg/ovn/controller/apbroute/repair.go index fec467f6262..195c2783d49 100644 --- a/go-controller/pkg/ovn/controller/apbroute/repair.go +++ b/go-controller/pkg/ovn/controller/apbroute/repair.go @@ -27,7 +27,7 @@ type managedGWIPs struct { func (c *ExternalGatewayMasterController) repair() { start := time.Now() defer func() { - klog.Infof("Syncing exgw routes took %v", time.Since(start)) + klog.V(4).InfoS("Syncing exgw routes took %v", time.Since(start)) }() // migration from LGW to SGW mode @@ -109,19 +109,18 @@ func (c *ExternalGatewayMasterController) repair() { } } - klog.Infof("OVN ECMP route cache is: %+v", ovnRouteCache) - klog.Infof("Cluster ECMP route cache is: %+v", policyGWIPsMap) + klog.V(4).InfoS("OVN ECMP route cache is: %+v", ovnRouteCache) + klog.V(4).InfoS("Cluster ECMP route cache is: %+v", policyGWIPsMap) // iterate through ovn routes and remove any stale entries for podIP, ovnRoutes := range ovnRouteCache { podHasAnyECMPRoutes := false for _, ovnRoute := range ovnRoutes { if !ovnRoute.shouldExist { - klog.Infof("Found stale exgw ecmp route, podIP: %s, nexthop: %s, router: %s", + klog.V(4).InfoS("Found stale exgw ecmp route, podIP: %s, nexthop: %s, router: %s", podIP, ovnRoute.nextHop, ovnRoute.router) lrsr := nbdb.LogicalRouterStaticRoute{UUID: ovnRoute.uuid} err := c.nbClient.deleteLogicalRouterStaticRoutes(ovnRoute.router, &lrsr) - // err := if err != nil { klog.Errorf("Error deleting static route %s from router %s: %v", ovnRoute.uuid, ovnRoute.router, err) } @@ -133,7 +132,7 @@ func (c *ExternalGatewayMasterController) repair() { // the default bridge to a new secondary bridge (or vice versa) prefix, err := c.nbClient.extSwitchPrefix(node) if err != nil { - // we shouldn't continue in this case, because we cant be sure this is a route we want to remove + // We shouldn't continue in this case, because we cant be sure this is a route we want to remove klog.Errorf("Cannot sync exgw bfd: %+v, unable to determine exgw switch prefix: %v", ovnRoute, err) } else { @@ -188,7 +187,7 @@ func (c *ExternalGatewayMasterController) buildExternalIPGatewaysFromPolicyRules return nil, err } for _, nsPod := range nsPods { - // ignore completed pods, host networked pods, pods not scheduled + // Ignore completed pods, host networked pods, pods not scheduled if util.PodWantsHostNetwork(nsPod) || util.PodCompleted(nsPod) || !util.PodScheduled(nsPod) { continue } From 127c20c1be5d1b0ea19a9eb3df81ab6a14d12407 Mon Sep 17 00:00:00 2001 From: jordigilh Date: Thu, 22 Jun 2023 13:35:23 -0400 Subject: [PATCH 3/6] APB node controller: Store policy as key in the queue Signed-off-by: jordigilh --- .../controller/apbroute/master_controller.go | 2 +- .../controller/apbroute/node_controller.go | 40 ++++++++++++------- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/go-controller/pkg/ovn/controller/apbroute/master_controller.go b/go-controller/pkg/ovn/controller/apbroute/master_controller.go index 26e2836492b..0a07cc2c757 100644 --- a/go-controller/pkg/ovn/controller/apbroute/master_controller.go +++ b/go-controller/pkg/ovn/controller/apbroute/master_controller.go @@ -256,7 +256,7 @@ func (c *ExternalGatewayMasterController) processNextPolicyWorkItem(wg *sync.Wai err = c.updateStatusAPBExternalRoute(policy, err) if err != nil { if c.routeQueue.NumRequeues(key) < maxRetries { - klog.V(4).InfoS("Error found while processing policy: %w", err) + klog.V(4).InfoS("Error found while processing policy %s: %w", key, err) c.routeQueue.AddRateLimited(key) return true } diff --git a/go-controller/pkg/ovn/controller/apbroute/node_controller.go b/go-controller/pkg/ovn/controller/apbroute/node_controller.go index 2f99986b4aa..60e21a8ac27 100644 --- a/go-controller/pkg/ovn/controller/apbroute/node_controller.go +++ b/go-controller/pkg/ovn/controller/apbroute/node_controller.go @@ -270,16 +270,18 @@ func (c *ExternalGatewayNodeController) processNextPolicyWorkItem(wg *sync.WaitG } defer c.routeQueue.Done(key) - item := key.(string) klog.V(4).InfoS("Processing policy %s", key) _, err := c.mgr.syncRoutePolicy(key.(string), c.routeQueue) if err != nil { - if c.routeQueue.NumRequeues(item) < maxRetries { - klog.V(4).InfoS("Error found while processing policy: %s, error: %v", key, err.Error()) - c.routeQueue.AddRateLimited(item) + klog.Errorf("Failed to sync APB policy %s: %v", key, err) + } + if err != nil { + if c.routeQueue.NumRequeues(key) < maxRetries { + klog.V(4).InfoS("Error found while processing policy %s: %w", key, err) + c.routeQueue.AddRateLimited(key) return true } - klog.Warningf("Dropping policy %q out of the queue: %v", key, err) + klog.Warningf("Dropping policy %q out of the queue: %w", key, err) utilruntime.HandleError(err) } c.routeQueue.Forget(key) @@ -287,16 +289,18 @@ func (c *ExternalGatewayNodeController) processNextPolicyWorkItem(wg *sync.WaitG } func (c *ExternalGatewayNodeController) onPolicyAdd(obj interface{}) { - policy, ok := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) + _, ok := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) if !ok { utilruntime.HandleError(fmt.Errorf("expecting %T but received %T", &adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute{}, obj)) return } - if policy == nil { - utilruntime.HandleError(errors.New("invalid Admin Policy Based External Route provided to onPolicyAdd()")) + key, err := cache.MetaNamespaceKeyFunc(obj) + if err != nil { + utilruntime.HandleError(fmt.Errorf("couldn't get key for object %+v: %v", obj, err)) return } - c.routeQueue.Add(policy) + klog.V(4).Infof("Adding policy %s", key) + c.routeQueue.Add(key) } func (c *ExternalGatewayNodeController) onPolicyUpdate(oldObj, newObj interface{}) { @@ -319,26 +323,34 @@ func (c *ExternalGatewayNodeController) onPolicyUpdate(oldObj, newObj interface{ !newRoutePolicy.GetDeletionTimestamp().IsZero() { return } - c.routeQueue.Add(newObj) + + key, err := cache.MetaNamespaceKeyFunc(newObj) + if err != nil { + utilruntime.HandleError(fmt.Errorf("couldn't get key for object %+v: %v", newObj, err)) + } + c.routeQueue.Add(key) } func (c *ExternalGatewayNodeController) onPolicyDelete(obj interface{}) { - policy, ok := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) + _, ok := obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) if !ok { tombstone, ok := obj.(cache.DeletedFinalStateUnknown) if !ok { utilruntime.HandleError(fmt.Errorf("couldn't get object from tomstone %#v", obj)) return } - policy, ok = tombstone.Obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) + _, ok = tombstone.Obj.(*adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute) if !ok { utilruntime.HandleError(fmt.Errorf("tombstone contained object that is not an Admin Policy Based External Route %#v", tombstone.Obj)) return } } - if policy != nil { - c.routeQueue.Add(policy) + key, err := cache.MetaNamespaceKeyFunc(obj) + if err != nil { + utilruntime.HandleError(fmt.Errorf("couldn't get key for object %+v: %v", obj, err)) + return } + c.routeQueue.Add(key) } func (c *ExternalGatewayNodeController) runNamespaceWorker(wg *sync.WaitGroup) { From 9ebefc4d9a1997ec1729799f9314215e60ac60f5 Mon Sep 17 00:00:00 2001 From: jordigilh Date: Thu, 22 Jun 2023 15:46:48 -0400 Subject: [PATCH 4/6] Add check to validate if an event to a namespace impacts a namespace selector in a dynamic hop and queue the impacted policy Signed-off-by: jordigilh --- .../apbroute/external_controller_namespace.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go index 7e1d1c3f5b5..ee7bc241d47 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go @@ -31,6 +31,17 @@ func (m *externalPolicyManager) syncNamespace(namespace *v1.Namespace, routeQueu keysToBeQueued.Insert(policyName) } } + for _, hop := range ri.policy.Spec.NextHops.DynamicHops { + gwNs, err := m.listNamespacesBySelector(hop.NamespaceSelector) + if err != nil { + return err + } + for _, ns := range gwNs { + if ns.Name == namespace.Name { + keysToBeQueued.Insert(policyName) + } + } + } } return nil }) From b7ac83a8f6fcc4362fd1326e43ba056b9c5bdf82 Mon Sep 17 00:00:00 2001 From: jordigilh Date: Mon, 26 Jun 2023 11:55:47 +0200 Subject: [PATCH 5/6] Add static hops when a new pod is added to a target namespace Signed-off-by: jordigilh --- .../apbroute/external_controller_pod.go | 9 + .../apbroute/external_controller_policy.go | 67 +++++++- .../pkg/ovn/external_gateway_test.go | 162 +++++++++++++++++- 3 files changed, 228 insertions(+), 10 deletions(-) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go index 4a9966c31e8..9e7cebe2d57 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_pod.go @@ -30,6 +30,15 @@ func (m *externalPolicyManager) syncPod(pod *v1.Pod, podLister corev1listers.Pod if pErr != nil { return pErr } + if policies.Len() == 0 { + // this is not a gateway pod + cacheInfo, found := m.getNamespaceInfoFromCache(pod.Namespace) + if found { + // its namespace is a target for policies. + policies = policies.Union(cacheInfo.Policies) + m.unlockNamespaceInfoCache(pod.Namespace) + } + } klog.V(4).InfoS("Processing gateway pod %s/%s with matching policies %+v", pod.Namespace, pod.Name, policies.UnsortedList()) for policyName := range policies { klog.V(5).InfoS("Queueing policy %s", policyName) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go index e1a08061c33..b359c588959 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_policy.go @@ -193,7 +193,6 @@ func (m *externalPolicyManager) processReconciliationWithNamespace(nsName string if err != nil { return err } - // remove pod gw IPs that are no longer valid allProcessedGWIPs := map[ktypes.NamespacedName]*gatewayInfo{} // Consolidate all dynamic gateway IPs from the policies into a single map for _, pp := range processedPolicies { @@ -201,32 +200,83 @@ func (m *externalPolicyManager) processReconciliationWithNamespace(nsName string allProcessedGWIPs[k] = v } } - newGateways, invalidGWIPs, ipsToKeep := m.calculateDynamicGateways(allProcessedGWIPs, cacheInfo.DynamicGateways) + newDynamicGateways, invalidGWIPs, ipsToKeep := m.calculateDynamicGateways(allProcessedGWIPs, cacheInfo.DynamicGateways) + // Consolidate all static gateway IPs from the policies into a single set + allStaticGWIPs := make(gatewayInfoList, 0) + for _, pp := range processedPolicies { + allStaticGWIPs = append(allStaticGWIPs, pp.staticGateways...) + } + newStaticGateways, invalidStaticGWIPs, staticIPsToKeep := m.calculateStaticGateways(allStaticGWIPs, cacheInfo.StaticGateways) + // delete all invalid GW IP references in the NorthBoundDB (master controller) or conntrack (node_controller) - err = m.netClient.deleteGatewayIPs(nsName, invalidGWIPs, ipsToKeep) + // provide all valid IPs, static as well as dynamic, since conntrack is using a white listed approach when deleting entries + err = m.netClient.deleteGatewayIPs(nsName, invalidGWIPs.Union(invalidStaticGWIPs), ipsToKeep.Union(staticIPsToKeep)) if err != nil { return err } - // proceed to add the GW IPs again - for _, gatewayInfo := range newGateways { + + // proceed to add the dynamic GW IPs + for _, gatewayInfo := range newDynamicGateways { err = m.addGWRoutesForNamespace(nsName, gatewayInfoList{gatewayInfo}) if err != nil { return err } } - cacheInfo.DynamicGateways = newGateways + // add the static GW IPs + err = m.addGWRoutesForNamespace(nsName, newStaticGateways) + if err != nil { + return err + } + // Update namespace cacheInfo with the new static and dynamic gateways + cacheInfo.StaticGateways = newStaticGateways + cacheInfo.DynamicGateways = newDynamicGateways return nil } +func (m *externalPolicyManager) calculateStaticGateways(allProcessedGWIPs, cachedStaticGWInfo gatewayInfoList) (gatewayInfoList, sets.Set[string], sets.Set[string]) { + klog.V(5).InfoS("Processed static policies: %+v", allProcessedGWIPs) + ipsToKeep := sets.New[string]() + invalidGWIPs := sets.New[string]() + newGateways := make(gatewayInfoList, 0) + + for _, gwInfo := range allProcessedGWIPs { + info := newGatewayInfo(sets.New[string](), gwInfo.BFDEnabled) + for ip := range gwInfo.Gateways.items { + if !cachedStaticGWInfo.HasIP(ip) { + klog.V(5).InfoS("PP to cacheInfo: static GW IP %s not found in namespace cache ", ip) + invalidGWIPs.Insert(ip) + continue + } + ipsToKeep.Insert(ip) + info.Gateways.items.Insert(ip) + } + if info.Gateways.items.Len() > 0 { + newGateways = append(newGateways, info) + } + } + + // Compare all elements in the cacheInfo map against the consolidated map: those that are not in the consolidated map are to be deleted. + // The previous loop covers for the static IPs that exist in both slices but contain different gateway infos, and thus to be deleted + for _, cachedInfo := range cachedStaticGWInfo { + for ip := range cachedInfo.Gateways.items { + if !allProcessedGWIPs.HasIP(ip) { + klog.V(5).InfoS("CacheInfo-> static GW IP %v not found in processed policies", ip) + invalidGWIPs.Insert(ip) + } + } + } + return newGateways, invalidGWIPs, ipsToKeep +} + func (m *externalPolicyManager) calculateDynamicGateways(allProcessedGWIPs, cachedDynamicGWInfo map[ktypes.NamespacedName]*gatewayInfo) (map[ktypes.NamespacedName]*gatewayInfo, sets.Set[string], sets.Set[string]) { - klog.V(5).InfoS("Processed policies: %+v", allProcessedGWIPs) + klog.V(5).InfoS("Processed dynamic policies: %+v", allProcessedGWIPs) // In order to delete the invalid GWs, the logic has to collect all the valid GW IPs as well as the invalid ones. // This is due to implementation requirements by the network clients: for the NB interaction (master controller), only the invalid GWs is needed // but when interacting with the conntrack (node controller), it requires to use only the valid GWs due to a white listing approach // (delete any entry that does not reference any of these IPs) when deleting its entries. ipsToKeep := sets.New[string]() invalidGWIPs := sets.New[string]() - // this map will be used to store all valid gateway info references as they are processed in the next two loop. + // this map will be used to store all valid gateway info references as they are processed in the next two loops. newGateways := map[ktypes.NamespacedName]*gatewayInfo{} for k, v1 := range allProcessedGWIPs { v2, ok := cachedDynamicGWInfo[k] @@ -249,7 +299,6 @@ func (m *externalPolicyManager) calculateDynamicGateways(allProcessedGWIPs, cach // IP not found in the processed policies, it means the pod gateway information is no longer applicable klog.V(5).InfoS("CacheInfo-> GW IP %+v not found in processed policies", k) invalidGWIPs.Insert(v.Gateways.UnsortedList()...) - continue } } return newGateways, invalidGWIPs, ipsToKeep diff --git a/go-controller/pkg/ovn/external_gateway_test.go b/go-controller/pkg/ovn/external_gateway_test.go index d8347466be3..8fd261b972f 100644 --- a/go-controller/pkg/ovn/external_gateway_test.go +++ b/go-controller/pkg/ovn/external_gateway_test.go @@ -202,6 +202,151 @@ var _ = ginkgo.Describe("OVN Egress Gateway Operations", func() { }, })) + table.DescribeTable("reconciles an new pod with namespace single exgw static GW after policy is created", func(bfd bool, finalNB []libovsdbtest.TestData) { + app.Action = func(ctx *cli.Context) error { + + namespaceT := *newNamespace(namespaceName) + + t := newTPod( + "node1", + "10.128.1.0/24", + "10.128.1.2", + "10.128.1.1", + "myPod", + "10.128.1.3", + "0a:58:0a:80:01:03", + namespaceT.Name, + ) + + fakeOvn.startWithDBSetup( + libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: "node1", + Name: "node1", + }, + &nbdb.LogicalRouter{ + UUID: "GR_node1-UUID", + Name: "GR_node1", + }, + }, + }, + &v1.NamespaceList{ + Items: []v1.Namespace{ + namespaceT, + }, + }, + &adminpolicybasedrouteapi.AdminPolicyBasedExternalRouteList{ + Items: []adminpolicybasedrouteapi.AdminPolicyBasedExternalRoute{ + newPolicy("policy", &metav1.LabelSelector{MatchLabels: map[string]string{"name": namespaceName}}, sets.NewString("9.0.0.1"), bfd, nil, nil, bfd, ""), + }, + }, + ) + + t.populateLogicalSwitchCache(fakeOvn, getLogicalSwitchUUID(fakeOvn.controller.nbClient, "node1")) + + injectNode(fakeOvn) + err := fakeOvn.controller.WatchNamespaces() + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + err = fakeOvn.controller.WatchPods() + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + fakeOvn.RunAPBExternalPolicyController() + ginkgo.By("Waiting for the policy to be processed") + gomega.Eventually(func() bool { + p, err := fakeOvn.fakeClient.AdminPolicyRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.Background(), "policy", metav1.GetOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + return !p.Status.LastTransitionTime.IsZero() + }).Should(gomega.BeTrue()) + ginkgo.By("Adding the target pod") + _, err = fakeOvn.fakeClient.KubeClient.CoreV1().Pods(t.namespace).Create(context.Background(), newPod(t.namespace, t.podName, t.nodeName, t.podIP), metav1.CreateOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + ginkgo.By("Validating the north bound DB has been updated with the new static route to the target pod") + gomega.Eventually(func() string { return getPodAnnotations(fakeOvn.fakeClient.KubeClient, t.namespace, t.podName) }, 2).Should(gomega.MatchJSON(t.getAnnotationsJson())) + gomega.Eventually(fakeOvn.nbClient).Should(libovsdbtest.HaveData(finalNB)) + return nil + } + + err := app.Run([]string{app.Name}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + }, table.Entry("No BFD", false, []libovsdbtest.TestData{ + &nbdb.LogicalSwitchPort{ + UUID: "lsp1", + Addresses: []string{"0a:58:0a:80:01:03 10.128.1.3"}, + ExternalIDs: map[string]string{ + "pod": "true", + "namespace": namespaceName, + }, + Name: "namespace1_myPod", + Options: map[string]string{ + "iface-id-ver": "myPod", + "requested-chassis": "node1", + }, + PortSecurity: []string{"0a:58:0a:80:01:03 10.128.1.3"}, + }, + &nbdb.LogicalSwitch{ + UUID: "node1", + Name: "node1", + Ports: []string{"lsp1"}, + }, + &nbdb.LogicalRouterStaticRoute{ + UUID: "static-route-1-UUID", + IPPrefix: "10.128.1.3/32", + Nexthop: "9.0.0.1", + Policy: &nbdb.LogicalRouterStaticRoutePolicySrcIP, + OutputPort: &logicalRouterPort, + Options: map[string]string{ + "ecmp_symmetric_reply": "true", + }, + }, + &nbdb.LogicalRouter{ + UUID: "GR_node1-UUID", + Name: "GR_node1", + StaticRoutes: []string{"static-route-1-UUID"}, + }, + }), + table.Entry("BFD Enabled", true, []libovsdbtest.TestData{ + &nbdb.LogicalSwitchPort{ + UUID: "lsp1", + Addresses: []string{"0a:58:0a:80:01:03 10.128.1.3"}, + ExternalIDs: map[string]string{ + "pod": "true", + "namespace": namespaceName, + }, + Name: "namespace1_myPod", + Options: map[string]string{ + "iface-id-ver": "myPod", + "requested-chassis": "node1", + }, + PortSecurity: []string{"0a:58:0a:80:01:03 10.128.1.3"}, + }, + &nbdb.LogicalSwitch{ + UUID: "node1", + Name: "node1", + Ports: []string{"lsp1"}, + }, + &nbdb.BFD{ + UUID: bfd1NamedUUID, + DstIP: "9.0.0.1", + LogicalPort: "rtoe-GR_node1", + }, + &nbdb.LogicalRouterStaticRoute{ + UUID: "static-route-1-UUID", + IPPrefix: "10.128.1.3/32", + Nexthop: "9.0.0.1", + BFD: &bfd1NamedUUID, + Policy: &nbdb.LogicalRouterStaticRoutePolicySrcIP, + OutputPort: &logicalRouterPort, + Options: map[string]string{ + "ecmp_symmetric_reply": "true", + }, + }, + &nbdb.LogicalRouter{ + UUID: "GR_node1-UUID", + Name: "GR_node1", + StaticRoutes: []string{"static-route-1-UUID"}, + }, + })) + table.DescribeTable("reconciles an new pod with namespace single exgw static gateway already set with pod event first", func(bfd bool, finalNB []libovsdbtest.TestData) { app.Action = func(ctx *cli.Context) error { @@ -1112,6 +1257,11 @@ var _ = ginkgo.Describe("OVN Egress Gateway Operations", func() { err = fakeOvn.controller.WatchPods() gomega.Expect(err).NotTo(gomega.HaveOccurred()) fakeOvn.RunAPBExternalPolicyController() + gomega.Eventually(func() bool { + p, err := fakeOvn.fakeClient.AdminPolicyRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.Background(), "policy", metav1.GetOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + return !p.Status.LastTransitionTime.IsZero() + }).Should(gomega.BeTrue()) _, err = fakeOvn.fakeClient.KubeClient.CoreV1().Pods(t.namespace).Create(context.TODO(), newPod(t.namespace, t.podName, t.nodeName, t.podIP), metav1.CreateOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) @@ -1417,7 +1567,11 @@ var _ = ginkgo.Describe("OVN Egress Gateway Operations", func() { err = fakeOvn.controller.WatchPods() gomega.Expect(err).NotTo(gomega.HaveOccurred()) fakeOvn.RunAPBExternalPolicyController() - + gomega.Eventually(func() bool { + p, err := fakeOvn.fakeClient.AdminPolicyRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.Background(), "policy", metav1.GetOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + return !p.Status.LastTransitionTime.IsZero() + }).Should(gomega.BeTrue()) _, err = fakeOvn.fakeClient.KubeClient.CoreV1().Pods(t.namespace).Create(context.TODO(), newPod(t.namespace, t.podName, t.nodeName, t.podIP), metav1.CreateOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) @@ -2081,6 +2235,12 @@ var _ = ginkgo.Describe("OVN Egress Gateway Operations", func() { "") _, err = fakeOvn.fakeClient.AdminPolicyRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Create(context.Background(), &p, metav1.CreateOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) + ginkgo.By("Waiting for the policy to be processed") + gomega.Eventually(func() bool { + policy, err := fakeOvn.fakeClient.AdminPolicyRouteClient.K8sV1().AdminPolicyBasedExternalRoutes().Get(context.Background(), p.Name, metav1.GetOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + return !policy.Status.LastTransitionTime.IsZero() + }).Should(gomega.BeTrue()) tempNB := []libovsdbtest.TestData{ &nbdb.LogicalRouter{ From 462815dab7e871e7326d4c9f3c2390d09ccbcab6 Mon Sep 17 00:00:00 2001 From: jordigilh Date: Wed, 28 Jun 2023 13:29:30 +0200 Subject: [PATCH 6/6] APB: Use label selector to match against namespace labels Signed-off-by: jordigilh --- .../apbroute/external_controller_namespace.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go index ee7bc241d47..d763e475920 100644 --- a/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go +++ b/go-controller/pkg/ovn/controller/apbroute/external_controller_namespace.go @@ -4,6 +4,7 @@ import ( adminpolicybasedrouteapi "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/adminpolicybasedroute/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" @@ -22,14 +23,10 @@ func (m *externalPolicyManager) syncNamespace(namespace *v1.Namespace, routeQueu func(key string) error { ri, found := m.routePolicySyncCache.Load(policyName) if found { - targetNsList, err := m.listNamespacesBySelector(&ri.policy.Spec.From.NamespaceSelector) - if err != nil { - return err - } - for _, targetNS := range targetNsList { - if targetNS.Name == namespace.Name { - keysToBeQueued.Insert(policyName) - } + nsSel, _ := metav1.LabelSelectorAsSelector(&ri.policy.Spec.From.NamespaceSelector) + if nsSel.Matches(labels.Set(namespace.Labels)) { + keysToBeQueued.Insert(policyName) + return nil } for _, hop := range ri.policy.Spec.NextHops.DynamicHops { gwNs, err := m.listNamespacesBySelector(hop.NamespaceSelector) @@ -39,6 +36,7 @@ func (m *externalPolicyManager) syncNamespace(namespace *v1.Namespace, routeQueu for _, ns := range gwNs { if ns.Name == namespace.Name { keysToBeQueued.Insert(policyName) + return nil } } }