Merge pull request #2074 from tssurya/OCPBUGS-29599

OCPBUGS-29599: Update HostNetworkNamespace address_set for remote zone nodes

go-controller/pkg/ovn/base_network_controller.go

@@ -253,7 +253,7 @@ func (bnc *BaseNetworkController) createOvnClusterRouter() (*nbdb.LogicalRouter,
 }
 
 // syncNodeClusterRouterPort ensures a node's LS to the cluster router's LRP is created.
-// NOTE: We could have created the router port in ensureNodeLogicalNetwork() instead of here,
+// NOTE: We could have created the router port in createNodeLogicalSwitch() instead of here,
 // but chassis ID is not available at that moment. We need the chassis ID to set the
 // gateway-chassis, which in effect pins the logical switch to the current node in OVN.
 // Otherwise, ovn-controller will flood-fill unrelated datapaths unnecessarily, causing scale

go-controller/pkg/ovn/default_network_controller.go

@@ -33,6 +33,7 @@ import (
 
 	kapi "k8s.io/api/core/v1"
 	knet "k8s.io/api/networking/v1"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/wait"
 	corev1listers "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
@@ -121,6 +122,7 @@ type DefaultNetworkController struct {
 	nodeClusterRouterPortFailed sync.Map
 	hybridOverlayFailed         sync.Map
 	syncZoneICFailed            sync.Map
+	syncHostNetAddrSetFailed    sync.Map
 
 	// variable to determine if all pods present on the node during startup have been processed
 	// updated atomically
@@ -747,6 +749,7 @@ func (h *defaultNetworkControllerEventHandler) AddResource(obj interface{}, from
 				}
 			}
 		}
+		var aggregatedErrors []error
 		if h.oc.isLocalZoneNode(node) {
 			var nodeParams *nodeSyncs
 			if fromRetryLoop {
@@ -770,13 +773,19 @@ func (h *defaultNetworkControllerEventHandler) AddResource(obj interface{}, from
 			if err = h.oc.addUpdateLocalNodeEvent(node, nodeParams); err != nil {
 				klog.Infof("Node add failed for %s, will try again later: %v",
 					node.Name, err)
-				return err
+				aggregatedErrors = append(aggregatedErrors, err)
 			}
 		} else {
 			if err = h.oc.addUpdateRemoteNodeEvent(node, config.OVNKubernetesFeature.EnableInterconnect); err != nil {
-				return err
+				aggregatedErrors = append(aggregatedErrors, err)
 			}
 		}
+		if err = h.oc.addIPToHostNetworkNamespaceAddrSet(node); err != nil {
+			klog.Errorf("Failed to add node IPs to %s address_set: %v", config.Kubernetes.HostNetworkNamespace, err)
+			h.oc.syncHostNetAddrSetFailed.Store(node.Name, true)
+			aggregatedErrors = append(aggregatedErrors, err)
+		}
+		return kerrors.NewAggregate(aggregatedErrors)
 
 	case factory.EgressFirewallType:
 		egressFirewall := obj.(*egressfirewall.EgressFirewall).DeepCopy()
@@ -898,6 +907,7 @@ func (h *defaultNetworkControllerEventHandler) UpdateResource(oldObj, newObj int
 		newNodeIsLocalZoneNode := h.oc.isLocalZoneNode(newNode)
 		zoneClusterChanged := h.oc.nodeZoneClusterChanged(oldNode, newNode, newNodeIsLocalZoneNode)
 		nodeSubnetChanged := nodeSubnetChanged(oldNode, newNode)
+		var aggregatedErrors []error
 		if newNodeIsLocalZoneNode {
 			var nodeSyncsParam *nodeSyncs
 			if h.oc.isLocalZoneNode(oldNode) {
@@ -928,7 +938,9 @@ func (h *defaultNetworkControllerEventHandler) UpdateResource(oldObj, newObj int
 				nodeSyncsParam = &nodeSyncs{true, true, true, true, true, config.OVNKubernetesFeature.EnableInterconnect}
 			}
 
-			return h.oc.addUpdateLocalNodeEvent(newNode, nodeSyncsParam)
+			if err := h.oc.addUpdateLocalNodeEvent(newNode, nodeSyncsParam); err != nil {
+				aggregatedErrors = append(aggregatedErrors, err)
+			}
 		} else {
 			_, syncZoneIC := h.oc.syncZoneICFailed.Load(newNode.Name)
 
@@ -940,8 +952,20 @@ func (h *defaultNetworkControllerEventHandler) UpdateResource(oldObj, newObj int
 				klog.Infof("Node %s in remote zone %s needs interconnect zone sync up. Zone cluster changed: %v",
 					newNode.Name, util.GetNodeZone(newNode), zoneClusterChanged)
 			}
-			return h.oc.addUpdateRemoteNodeEvent(newNode, syncZoneIC)
+			if err := h.oc.addUpdateRemoteNodeEvent(newNode, syncZoneIC); err != nil {
+				aggregatedErrors = append(aggregatedErrors, err)
+			}
+		}
+		_, syncHostNetAddrSet := h.oc.syncHostNetAddrSetFailed.Load(newNode.Name)
+		if syncHostNetAddrSet {
+			if err := h.oc.addIPToHostNetworkNamespaceAddrSet(newNode); err != nil {
+				klog.Errorf("Failed to add node IPs to %s address_set: %v", config.Kubernetes.HostNetworkNamespace, err)
+				aggregatedErrors = append(aggregatedErrors, err)
+			} else {
+				h.oc.syncHostNetAddrSetFailed.Delete(newNode.Name)
+			}
 		}
+		return kerrors.NewAggregate(aggregatedErrors)
 
 	case factory.EgressIPType:
 		oldEIP := oldObj.(*egressipv1.EgressIP)

go-controller/pkg/ovn/master.go

@@ -336,47 +336,6 @@ func (oc *DefaultNetworkController) syncGatewayLogicalNetwork(node *kapi.Node, l
 	return err
 }
 
-func (oc *DefaultNetworkController) ensureNodeLogicalNetwork(node *kapi.Node, hostSubnets []*net.IPNet) error {
-	var hostNetworkPolicyIPs []net.IP
-
-	for _, hostSubnet := range hostSubnets {
-		mgmtIfAddr := util.GetNodeManagementIfAddr(hostSubnet)
-		hostNetworkPolicyIPs = append(hostNetworkPolicyIPs, mgmtIfAddr.IP)
-	}
-
-	// also add the join switch IPs for this node - needed in shared gateway mode
-	// Note: join switch IPs for each node are generated by cluster manager and
-	// stored in the node annotation
-	lrpIPs, err := util.ParseNodeGatewayRouterLRPAddrs(node)
-	if err != nil {
-		return fmt.Errorf("failed to get join switch port IP address for node %s: %v", node.Name, err)
-	}
-
-	for _, lrpIP := range lrpIPs {
-		hostNetworkPolicyIPs = append(hostNetworkPolicyIPs, lrpIP.IP)
-	}
-
-	// add the host network IPs for this node to host network namespace's address set
-	if err = func() error {
-		hostNetworkNamespace := config.Kubernetes.HostNetworkNamespace
-		if hostNetworkNamespace != "" {
-			nsInfo, nsUnlock, err := oc.ensureNamespaceLocked(hostNetworkNamespace, true, nil)
-			if err != nil {
-				return fmt.Errorf("failed to ensure namespace locked: %v", err)
-			}
-			defer nsUnlock()
-			if err = nsInfo.addressSet.AddIPs(hostNetworkPolicyIPs); err != nil {
-				return err
-			}
-		}
-		return nil
-	}(); err != nil {
-		return err
-	}
-
-	return oc.createNodeLogicalSwitch(node.Name, hostSubnets, oc.clusterLoadBalancerGroupUUID, oc.switchLoadBalancerGroupUUID)
-}
-
 func (oc *DefaultNetworkController) addNode(node *kapi.Node) ([]*net.IPNet, error) {
 	// Node subnet for the default network is allocated by cluster manager.
 	// Make sure that the node is allocated with the subnet before proceeding
@@ -410,7 +369,7 @@ func (oc *DefaultNetworkController) addNode(node *kapi.Node) ([]*net.IPNet, erro
 	// subsequent operation in addNode() fails, oc.lsManager.DeleteNode(node.Name)
 	// needs to be done, otherwise, this node's IPAM will be overwritten and the
 	// same IP could be allocated to multiple Pods scheduled on this node.
-	err = oc.ensureNodeLogicalNetwork(node, hostSubnets)
+	err = oc.createNodeLogicalSwitch(node.Name, hostSubnets, oc.clusterLoadBalancerGroupUUID, oc.switchLoadBalancerGroupUUID)
 	if err != nil {
 		return nil, err
 	}
@@ -927,12 +886,20 @@ func (oc *DefaultNetworkController) deleteOVNNodeEvent(node *kapi.Node) error {
 		oc.syncZoneICFailed.Delete(node.Name)
 	}
 
+	// Remove management port IP and node's gateway-router-lrp-ifaddr
+	// from address_set specific to HostNetworkNamespace
+	if err := oc.delIPFromHostNetworkNamespaceAddrSet(node); err != nil {
+		return fmt.Errorf("failed to delete IPs from %s address_set: %v",
+			config.Kubernetes.HostNetworkNamespace, err)
+	}
+
 	oc.lsManager.DeleteSwitch(node.Name)
 	oc.addNodeFailed.Delete(node.Name)
 	oc.mgmtPortFailed.Delete(node.Name)
 	oc.gatewaysFailed.Delete(node.Name)
 	oc.nodeClusterRouterPortFailed.Delete(node.Name)
 	oc.localZoneNodes.Delete(node.Name)
+	oc.syncHostNetAddrSetFailed.Delete(node.Name)
 
 	return nil
 }
@@ -1021,3 +988,67 @@ func (oc *DefaultNetworkController) deleteHoNodeEvent(node *kapi.Node) error {
 	}
 	return nil
 }
+
+// addIPToHostNetworkNamespaceAddrSet adds management port IP and node's
+// gateway-router-lrp-ifaddr to address_set created for HostNetworkNamespace.
+// This function gets called from both AddResource & UpdateResource to add IPs
+// to address_set for both local and remote zone nodes.
+func (oc *DefaultNetworkController) addIPToHostNetworkNamespaceAddrSet(node *kapi.Node) error {
+	var hostNetworkPolicyIPs []net.IP
+
+	hostNetworkPolicyIPs, err := oc.getHostNamespaceAddressesForNode(node)
+	if err != nil {
+		return fmt.Errorf("error parsing annotation for node %s: %v", node.Name, err)
+	}
+
+	// add the host network IPs for this node to host network namespace's address set
+	if err = func() error {
+		hostNetworkNamespace := config.Kubernetes.HostNetworkNamespace
+		if hostNetworkNamespace != "" {
+			nsInfo, nsUnlock, err := oc.ensureNamespaceLocked(hostNetworkNamespace, true, nil)
+			if err != nil {
+				return fmt.Errorf("failed to ensure namespace locked: %v", err)
+			}
+			defer nsUnlock()
+			if err = nsInfo.addressSet.AddIPs(hostNetworkPolicyIPs); err != nil {
+				return err
+			}
+		}
+		return nil
+	}(); err != nil {
+		return err
+	}
+	return nil
+}
+
+// delIPFromHostNetworkNamespaceAddrSet removes management port IP and node's
+// gateway-router-lrp-ifaddr from address_set created for HostNetworkNamespace.
+// This function gets called from deleteOVNNodeEvent to remove IPs from address_set
+// for both local and remote zone nodes
+func (oc *DefaultNetworkController) delIPFromHostNetworkNamespaceAddrSet(node *kapi.Node) error {
+	var hostNetworkPolicyIPs []net.IP
+
+	hostNetworkPolicyIPs, err := oc.getHostNamespaceAddressesForNode(node)
+	if err != nil {
+		return fmt.Errorf("error parsing annotation for node %s: %v", node.Name, err)
+	}
+
+	// delete host network IPs for this node from host network namespace's address set
+	if err = func() error {
+		hostNetworkNamespace := config.Kubernetes.HostNetworkNamespace
+		if hostNetworkNamespace != "" {
+			nsInfo, nsUnlock, err := oc.ensureNamespaceLocked(hostNetworkNamespace, true, nil)
+			if err != nil {
+				return fmt.Errorf("failed to ensure namespace locked: %v", err)
+			}
+			defer nsUnlock()
+			if err = nsInfo.addressSet.DeleteIPs(hostNetworkPolicyIPs); err != nil {
+				return err
+			}
+		}
+		return nil
+	}(); err != nil {
+		return err
+	}
+	return nil
+}

go-controller/pkg/ovn/master_test.go

@@ -939,6 +939,7 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 		expectedNBDatabaseState        []libovsdbtest.TestData
 		l3GatewayConfig                *util.L3GatewayConfig
 		nodeHostCIDRs                  sets.Set[string]
+		fakeOvn                        *FakeOVN
 	)
 
 	const (
@@ -951,6 +952,7 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 	ginkgo.BeforeEach(func() {
 		// Restore global default values before each testcase
 		gomega.Expect(config.PrepareTestConfig()).To(gomega.Succeed())
+		fakeOvn = NewFakeOVN(true)
 
 		app = cli.NewApp()
 		app.Name = "test"
@@ -1533,8 +1535,7 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 				oc.retryNodes.ResourceHandler.AddResource(
 					&testNode, false)).To(
 				gomega.MatchError(
-					"nodeAdd: error adding node \"node1\": could not find \"k8s.ovn.org/node-subnets\" annotation"))
-
+					"[nodeAdd: error adding node \"node1\": could not find \"k8s.ovn.org/node-subnets\" annotation, error parsing annotation for node node1: could not find \"k8s.ovn.org/node-subnets\" annotation]"))
 			ginkgo.By("labeling the node to a hybrid overlay node")
 			testNode.Labels = nodeNoHostSubnetAnnotation()
 			_, err = fakeClient.KubeClient.CoreV1().Nodes().Update(context.TODO(), &testNode, metav1.UpdateOptions{})
@@ -1678,6 +1679,104 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 		})
 		gomega.Expect(err).NotTo(gomega.HaveOccurred())
 	})
+
+	ginkgo.It("verify IPs in host-network-namespace address_set after remote zone node add or delete", func() {
+		app.Action = func(ctx *cli.Context) error {
+			_, err := config.InitConfig(ctx, nil, nil)
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+			config.OVNKubernetesFeature.EnableInterconnect = true
+			config.Kubernetes.HostNetworkNamespace = "ovn-host-network"
+
+			// add the transit switch port bindings on behalf of ovn-northd so that
+			// the node gets added successfully. This is required for test simulation
+			// and not required in real world scenario.
+			pb := &sbdb.PortBinding{
+				LogicalPort: "tstor-node1",
+				Datapath:    "tstor-node1",
+				TunnelKey:   5,
+			}
+			dbSetup.SBData = append(
+				dbSetup.SBData,
+				pb,
+			)
+
+			// transit_switch is required to be able to start OVN with an empty
+			// NodeList and to avoid calling ensureTransitSwitch which expects
+			// a list of nodes. This is required for test simulation and not
+			// required in real world scenario.
+			ts := &nbdb.LogicalSwitch{
+				Name: "transit_switch",
+			}
+			dbSetup.NBData = append(
+				dbSetup.NBData,
+				ts,
+			)
+
+			// https://github.com/ovn-org/ovn-kubernetes/blob/master/go-controller/pkg/ovn/namespace.go#L312
+			// adds management port and gateway router LRP IP to HostNetworkNamespace address set whenever
+			// the namespace gets created. Hence starting OVN with an empty NodeList and adding the node
+			// after the namespace is created successfully
+			fakeOvn.startWithDBSetup(dbSetup,
+				&v1.NamespaceList{
+					Items: []v1.Namespace{*newNamespace(config.Kubernetes.HostNetworkNamespace)},
+				},
+				&v1.NodeList{
+					Items: []v1.Node{},
+				},
+			)
+
+			// Required to make sure a remote zone node is being added
+			fakeOvn.controller.zone = "node1"
+
+			gomega.Expect(fakeOvn.controller.WatchNamespaces()).To(gomega.Succeed(), "Namespace should be created fine")
+			err = fakeOvn.controller.WatchNodes()
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+			// Check whether the address_set is empty after HostNetworkNamespace addition
+			fakeOvn.asf.EventuallyExpectEmptyAddressSetExist(config.Kubernetes.HostNetworkNamespace)
+
+			// Set annotations required to create remote zone node
+			newNodeSubnet := "10.1.2.0/24"
+			transitSwitchSubnet := "100.88.0.3/16"
+			testNode.Annotations["k8s.ovn.org/node-subnets"] = fmt.Sprintf("{\"default\":[\"%s\"]}", newNodeSubnet)
+			testNode.Annotations["k8s.ovn.org/node-chassis-id"] = "2"
+			testNode.Annotations["k8s.ovn.org/node-transit-switch-port-ifaddr"] = fmt.Sprintf("{\"ipv4\":\"%s\"}", transitSwitchSubnet)
+			testNode.Annotations["k8s.ovn.org/zone-name"] = "foo"
+			updatedNode, err := fakeOvn.fakeClient.KubeClient.CoreV1().Nodes().Create(context.TODO(), &testNode, metav1.CreateOptions{})
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+			// Check whether node IPs have been added to HostNetworkNamespace
+			// address_set or not
+			var ips []string
+			hostSubnets, err := util.ParseNodeHostSubnetAnnotation(updatedNode, types.DefaultNetworkName)
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+			mgmt_ips := util.GetNodeManagementIfAddr(hostSubnets[0])
+			ips = append(ips, mgmt_ips.IP.String())
+			lrpips, err := util.ParseNodeGatewayRouterLRPAddrs(updatedNode)
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+			lrpip, _, _ := net.ParseCIDR(lrpips[0].String())
+			ips = append(ips, lrpip.String())
+
+			fakeOvn.asf.EventuallyExpectAddressSetWithIPs(config.Kubernetes.HostNetworkNamespace, ips)
+
+			// Delete the node and check whether the address_set is empty or not
+			err = fakeOvn.fakeClient.KubeClient.CoreV1().Nodes().Delete(context.TODO(), testNode.Name, metav1.DeleteOptions{})
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+			var emptyAddress []string
+			fakeOvn.asf.EventuallyExpectAddressSetWithIPs(config.Kubernetes.HostNetworkNamespace, emptyAddress)
+
+			return nil
+		}
+
+		err := app.Run([]string{
+			app.Name,
+			"-cluster-subnets=" + clusterCIDR,
+			"--init-gateways",
+			"--nodeport",
+		})
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
+	})
 })
 
 func nodeNoHostSubnetAnnotation() map[string]string {