Merge pull request #3960 from tssurya/efw-add-2ndary-nodeIPs

EFW: Add support for secondary nodeIPs

go-controller/pkg/ovn/base_event_handler.go

@@ -105,7 +105,7 @@ func (h *baseNetworkControllerEventHandler) areResourcesEqual(objType reflect.Ty
 			return false, fmt.Errorf("could not cast obj2 of type %T to *kapi.Node", obj2)
 		}
 		if reflect.DeepEqual(oldNode.Labels, newNode.Labels) &&
-			reflect.DeepEqual(oldNode.Status.Addresses, newNode.Status.Addresses) {
+			!util.NodeHostCIDRsAnnotationChanged(oldNode, newNode) {
 			return true, nil
 		}
 		return false, nil

go-controller/pkg/ovn/controller/services/node_tracker.go

@@ -12,7 +12,6 @@ import (
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
 
 	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/util/sets"
 	coreinformers "k8s.io/client-go/informers/core/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
@@ -251,14 +250,13 @@ func (nt *nodeTracker) updateNode(node *v1.Node) {
 		}
 		chassisID = gwConf.ChassisID
 	}
-	hostAddresses, err := util.ParseNodeHostCIDRsDropNetMask(node)
+	hostAddresses, err := util.GetNodeHostAddrs(node)
 	if err != nil {
 		klog.Warningf("Failed to get node host CIDRs for [%s]: %s", node.Name, err.Error())
-		hostAddresses = sets.New[string]()
 	}
 
 	hostAddressesIPs := make([]net.IP, 0, len(hostAddresses))
-	for _, ipStr := range hostAddresses.UnsortedList() {
+	for _, ipStr := range hostAddresses {
 		ip := net.ParseIP(ipStr)
 		hostAddressesIPs = append(hostAddressesIPs, ip)
 	}

go-controller/pkg/ovn/egressfirewall.go

@@ -113,11 +113,11 @@ func (oc *DefaultNetworkController) newEgressFirewallRule(rawEgressFirewallRule
 			return efr, fmt.Errorf("unable to query nodes for egress firewall: %w", err)
 		}
 		for _, node := range nodes {
-			for _, addr := range node.Status.Addresses {
-				if addr.Type == kapi.NodeInternalIP {
-					efr.to.nodeAddrs.Insert(addr.Address)
-				}
+			hostAddresses, err := util.GetNodeHostAddrs(node)
+			if err != nil {
+				return efr, fmt.Errorf("unable to get node host CIDRs for egress firewall, node: %s: %w", node.Name, err)
 			}
+			efr.to.nodeAddrs.Insert(hostAddresses...)
 		}
 	}
 	efr.ports = rawEgressFirewallRule.Ports
@@ -396,7 +396,9 @@ func (oc *DefaultNetworkController) addEgressFirewallRules(ef *egressFirewall, p
 			action = nbdb.ACLActionDrop
 		}
 		if len(rule.to.nodeAddrs) > 0 {
-			for addr := range rule.to.nodeAddrs {
+			for _, addr := range sets.List(rule.to.nodeAddrs) {
+				// ideally we don't care about sorting this list, but this is being done to ensure Unit Test consistency
+				// and its not like this nodeAddrs can be super large per node EFW rule to cause scale issues
 				if utilnet.IsIPv6String(addr) {
 					matchTargets = append(matchTargets, matchTarget{matchKindV6CIDR, addr, false})
 				} else {
@@ -731,29 +733,23 @@ func (oc *DefaultNetworkController) getEgressFirewallACLDbIDs(namespace string,
 		})
 }
 
-func getNodeInternalAddrsToString(node *kapi.Node) []string {
-	v4, v6 := util.GetNodeInternalAddrs(node)
-	var addrs []string
-	for _, addr := range []net.IP{v4, v6} {
-		if addr != nil {
-			addrs = append(addrs, addr.String())
-		}
-	}
-
-	return addrs
-}
-
 func (oc *DefaultNetworkController) updateEgressFirewallForNode(oldNode, newNode *kapi.Node) error {
 
 	var addressesToAdd []string
 	var addressesToRemove []string
-
+	var err error
 	if oldNode != nil {
-		addressesToRemove = getNodeInternalAddrsToString(oldNode)
+		addressesToRemove, err = util.GetNodeHostAddrs(oldNode)
+		if err != nil {
+			return err
+		}
 	}
 
 	if newNode != nil {
-		addressesToAdd = getNodeInternalAddrsToString(newNode)
+		addressesToAdd, err = util.GetNodeHostAddrs(newNode)
+		if err != nil {
+			return err
+		}
 	}
 
 	// cycle through egress firewalls and check if any match this node's labels

go-controller/pkg/ovn/egressfirewall_test.go

@@ -518,6 +518,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 				var err error
 				nodeName := "node1"
 				nodeIP := "9.9.9.9"
+				nodeIP2 := "11.11.11.11"
+				nodeIP3 := "fc00:f853:ccd:e793::2"
+				config.IPv4Mode = true
+				config.IPv6Mode = true
 
 				app.Action = func(ctx *cli.Context) error {
 					namespace1 := *newNamespace("namespace1")
@@ -538,9 +542,9 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 							{
 								ObjectMeta: metav1.ObjectMeta{
 									Name: nodeName,
-								},
-								Status: v1.NodeStatus{
-									Addresses: []v1.NodeAddress{{Type: v1.NodeInternalIP, Address: nodeIP}},
+									Annotations: map[string]string{
+										util.OVNNodeHostCIDRs: fmt.Sprintf("[\"%s/24\",\"%s/24\",\"%s/64\"]", nodeIP, nodeIP2, nodeIP3),
+									},
 								},
 							},
 						})
@@ -562,7 +566,8 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 					gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
 					expectedDatabaseState := getEFExpectedDb(initialData,
-						fakeOVN, namespace1.Name, fmt.Sprintf("(ip4.dst == %s)", nodeIP), "", nbdb.ACLActionAllow)
+						fakeOVN, namespace1.Name,
+						fmt.Sprintf("(ip4.dst == %s || ip4.dst == %s || ip6.dst == %s)", nodeIP2, nodeIP, nodeIP3), "", nbdb.ACLActionAllow)
 					gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))
 
 					ginkgo.By("Updating a node to not match nodeSelector on Egress Firewall")
@@ -777,19 +782,21 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 			})
 			for _, ipMode := range []string{"IPv4", "IPv6"} {
 				ginkgo.It(fmt.Sprintf("configures egress firewall correctly with node selector, gateway mode: %s, IP mode: %s", gwMode, ipMode), func() {
-					nodeIP := "10.10.10.1"
-					nodeIP6 := "fc00:f853:ccd:e793::2"
+					nodeIP4CIDR := "10.10.10.1/24"
+					nodeIP, _, _ := net.ParseCIDR(nodeIP4CIDR)
+					nodeIP6CIDR := "fc00:f853:ccd:e793::2/64"
+					nodeIP6, _, _ := net.ParseCIDR(nodeIP6CIDR)
 					config.Gateway.Mode = gwMode
-					var nodeAddr v1.NodeAddress
+					var nodeCIDR string
 					if ipMode == "IPv4" {
 						config.IPv4Mode = true
 						config.IPv6Mode = false
-						nodeAddr = v1.NodeAddress{v1.NodeInternalIP, nodeIP}
+						nodeCIDR = nodeIP4CIDR
 
 					} else {
 						config.IPv4Mode = false
 						config.IPv6Mode = true
-						nodeAddr = v1.NodeAddress{v1.NodeInternalIP, nodeIP6}
+						nodeCIDR = nodeIP6CIDR
 					}
 					app.Action = func(ctx *cli.Context) error {
 						labelKey := "name"
@@ -806,14 +813,11 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						})
 						mdata := newObjectMeta(node1Name, "")
 						mdata.Labels = map[string]string{labelKey: labelValue}
+						mdata.Annotations = map[string]string{util.OVNNodeHostCIDRs: fmt.Sprintf("[\"%s\"]", nodeCIDR)}
 
 						startOvnWithNodes(dbSetup, []v1.Namespace{namespace1}, []egressfirewallapi.EgressFirewall{*egressFirewall},
 							[]v1.Node{
 								{
-									Status: v1.NodeStatus{
-										Phase:     v1.NodeRunning,
-										Addresses: []v1.NodeAddress{nodeAddr},
-									},
 									ObjectMeta: mdata,
 								},
 							})
@@ -1051,19 +1055,16 @@ var _ = ginkgo.Describe("OVN test basic functions", func() {
 		fakeOVN = NewFakeOVN(false)
 		a := newObjectMeta(node1Name, "")
 		a.Labels = nodeLabel
-		node1 := v1.Node{
-			Status: v1.NodeStatus{
-				Phase:     v1.NodeRunning,
-				Addresses: []v1.NodeAddress{{Type: v1.NodeInternalIP, Address: node1Addr}},
-			},
-			ObjectMeta: a,
+		a.Annotations = map[string]string{
+			util.OVNNodeHostCIDRs: fmt.Sprintf("[\"%s/24\"]", node1Addr),
+		}
+		node1 := v1.Node{ObjectMeta: a}
+		b := newObjectMeta(node2Name, "")
+		b.Annotations = map[string]string{
+			util.OVNNodeHostCIDRs: fmt.Sprintf("[\"%s/24\"]", node2Addr),
 		}
 		node2 := v1.Node{
-			Status: v1.NodeStatus{
-				Phase:     v1.NodeRunning,
-				Addresses: []v1.NodeAddress{{Type: v1.NodeInternalIP, Address: node2Addr}},
-			},
-			ObjectMeta: newObjectMeta(node2Name, ""),
+			ObjectMeta: b,
 		}
 		fakeOVN.startWithDBSetup(dbSetup, &v1.NodeList{Items: []v1.Node{node1, node2}})
 	})
@@ -1412,7 +1413,7 @@ var _ = ginkgo.Describe("OVN test basic functions", func() {
 				output: egressFirewallRule{
 					id:     1,
 					access: egressfirewallapi.EgressFirewallRuleAllow,
-					to:     destination{nodeAddrs: sets.New("9.9.9.9", "10.10.10.10"), nodeSelector: &metav1.LabelSelector{}},
+					to:     destination{nodeAddrs: sets.New("10.10.10.10", "9.9.9.9"), nodeSelector: &metav1.LabelSelector{}},
 				},
 			},
 			// match one node

go-controller/pkg/ovn/hybrid_test.go

@@ -411,7 +411,7 @@ var _ = ginkgo.Describe("Hybrid SDN Master Operations", func() {
 			}, 2).Should(gomega.HaveKeyWithValue(hotypes.HybridOverlayDRIP, nodeHOIP))
 
 			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs)
+			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs.UnsortedList())
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
 			var clusterSubnets []*net.IPNet
@@ -673,7 +673,7 @@ var _ = ginkgo.Describe("Hybrid SDN Master Operations", func() {
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 			setupClusterController(clusterController, expectedClusterLBGroup.UUID, expectedSwitchLBGroup.UUID, expectedRouterLBGroup.UUID, expectedNodeSwitch.UUID, node1.Name)
 
-			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs)
+			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs.UnsortedList())
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
 			//assuming all the pods have finished processing
@@ -863,7 +863,7 @@ var _ = ginkgo.Describe("Hybrid SDN Master Operations", func() {
 			}, 2).Should(gomega.HaveKeyWithValue(hotypes.HybridOverlayDRMAC, nodeHOMAC))
 
 			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs)
+			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs.UnsortedList())
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
 			var clusterSubnets []*net.IPNet
@@ -1152,7 +1152,7 @@ var _ = ginkgo.Describe("Hybrid SDN Master Operations", func() {
 
 			//ensure hybrid overlay elements have been added
 			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs)
+			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs.UnsortedList())
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
 			gomega.Eventually(func() ([]*nbdb.LogicalRouterStaticRoute, error) {
@@ -1345,7 +1345,7 @@ var _ = ginkgo.Describe("Hybrid SDN Master Operations", func() {
 			}, 2).Should(gomega.HaveKeyWithValue(hotypes.HybridOverlayDRMAC, nodeHOMAC))
 
 			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs)
+			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs.UnsortedList())
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
 			// switch the node to a ovn node
@@ -1518,7 +1518,7 @@ var _ = ginkgo.Describe("Hybrid SDN Master Operations", func() {
 			}, 2).Should(gomega.HaveKeyWithValue(hotypes.HybridOverlayDRMAC, nodeHOMAC))
 
 			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs)
+			err = clusterController.syncGatewayLogicalNetwork(updatedNode, l3GatewayConfig, []*net.IPNet{subnet}, hostAddrs.UnsortedList())
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
 			var clusterSubnets []*net.IPNet

go-controller/pkg/ovn/master.go

@@ -298,7 +298,7 @@ func (oc *DefaultNetworkController) syncNodeManagementPort(node *kapi.Node, host
 }
 
 func (oc *DefaultNetworkController) syncGatewayLogicalNetwork(node *kapi.Node, l3GatewayConfig *util.L3GatewayConfig,
-	hostSubnets []*net.IPNet, hostAddrs sets.Set[string]) error {
+	hostSubnets []*net.IPNet, hostAddrs []string) error {
 	var err error
 	var gwLRPIPs, clusterSubnets []*net.IPNet
 	for _, clusterSubnet := range config.Default.ClusterSubnets {
@@ -324,7 +324,7 @@ func (oc *DefaultNetworkController) syncGatewayLogicalNetwork(node *kapi.Node, l
 		if err != nil {
 			return err
 		}
-		relevantHostIPs, err := util.MatchAllIPStringFamily(utilnet.IsIPv6(hostIfAddr.IP), sets.List(hostAddrs))
+		relevantHostIPs, err := util.MatchAllIPStringFamily(utilnet.IsIPv6(hostIfAddr.IP), hostAddrs)
 		if err != nil && err != util.ErrorNoIP {
 			return err
 		}

go-controller/pkg/ovn/master_test.go

@@ -1095,7 +1095,7 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 			clusterSubnets := startFakeController(oc, wg)
 
 			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			err = oc.syncGatewayLogicalNetwork(&testNode, l3GatewayConfig, []*net.IPNet{subnet}, sets.New(node1.NodeIP))
+			err = oc.syncGatewayLogicalNetwork(&testNode, l3GatewayConfig, []*net.IPNet{subnet}, []string{node1.NodeIP})
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 			retry.InitRetryObjWithAdd(testNode, testNode.Name, oc.retryNodes)
 			gomega.Expect(retry.RetryObjsLen(oc.retryNodes)).To(gomega.Equal(1))
@@ -1153,7 +1153,7 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 			err = oc.syncNodeManagementPort(node, []*net.IPNet{subnet})
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			err = oc.syncGatewayLogicalNetwork(node, l3GatewayConfig, []*net.IPNet{subnet}, sets.New(node1.NodeIP))
+			err = oc.syncGatewayLogicalNetwork(node, l3GatewayConfig, []*net.IPNet{subnet}, []string{node1.NodeIP})
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 			ginkgo.By("Stale route should have been removed")
 
@@ -1184,7 +1184,7 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 			clusterSubnets := startFakeController(oc, wg)
 
 			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			err = oc.syncGatewayLogicalNetwork(&testNode, l3GatewayConfig, []*net.IPNet{subnet}, sets.New(node1.NodeIP))
+			err = oc.syncGatewayLogicalNetwork(&testNode, l3GatewayConfig, []*net.IPNet{subnet}, []string{node1.NodeIP})
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
 			skipSnat := false
@@ -1252,7 +1252,7 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 
 			// ensure the stale SNAT's are cleaned up
 			gomega.Expect(oc.StartServiceController(wg, false)).To(gomega.Succeed())
-			err = oc.syncGatewayLogicalNetwork(&testNode, l3GatewayConfig, []*net.IPNet{subnet}, sets.New(node1.NodeIP))
+			err = oc.syncGatewayLogicalNetwork(&testNode, l3GatewayConfig, []*net.IPNet{subnet}, []string{node1.NodeIP})
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
 			expectedNBDatabaseState = append(expectedNBDatabaseState,
@@ -1401,10 +1401,10 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 			startFakeController(oc, wg)
 
 			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			nodeHostAddrs := sets.New[string]()
+			nodeHostAddrs := []string{}
 			for _, nodeHostCIDR := range nodeHostCIDRs.UnsortedList() {
 				ip, _, _ := net.ParseCIDR(nodeHostCIDR)
-				nodeHostAddrs.Insert(ip.String())
+				nodeHostAddrs = append(nodeHostAddrs, ip.String())
 			}
 			err = oc.syncGatewayLogicalNetwork(&testNode, l3GatewayConfig, []*net.IPNet{subnet}, nodeHostAddrs)
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
@@ -1460,10 +1460,10 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 			startFakeController(oc, wg)
 
 			subnet := ovntest.MustParseIPNet(node1.NodeSubnet)
-			nodeHostAddrs := sets.New[string]()
+			nodeHostAddrs := []string{}
 			for _, nodeHostCIDR := range nodeHostCIDRs.UnsortedList() {
 				ip, _, _ := net.ParseCIDR(nodeHostCIDR)
-				nodeHostAddrs.Insert(ip.String())
+				nodeHostAddrs = append(nodeHostAddrs, ip.String())
 			}
 			err = oc.syncGatewayLogicalNetwork(&testNode, l3GatewayConfig, []*net.IPNet{subnet}, nodeHostAddrs)
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())

go-controller/pkg/ovn/ovn.go

@@ -23,7 +23,6 @@ import (
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
 
 	kapi "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/kubernetes/scheme"
 	listers "k8s.io/client-go/listers/core/v1"
 	ref "k8s.io/client-go/tools/reference"
@@ -374,9 +373,9 @@ func (oc *DefaultNetworkController) syncNodeGateway(node *kapi.Node, hostSubnets
 			return fmt.Errorf("error cleaning up gateway for node %s: %v", node.Name, err)
 		}
 	} else if hostSubnets != nil {
-		var hostAddrs sets.Set[string]
+		var hostAddrs []string
 		if config.Gateway.Mode == config.GatewayModeShared {
-			hostAddrs, err = util.ParseNodeHostCIDRsDropNetMask(node)
+			hostAddrs, err = util.GetNodeHostAddrs(node)
 			if err != nil && !util.IsAnnotationNotSetError(err) {
 				return fmt.Errorf("failed to get host CIDRs for node: %s: %v", node.Name, err)
 			}

go-controller/pkg/util/node_annotations.go

@@ -750,6 +750,16 @@ func ParseNodeHostCIDRsDropNetMask(node *kapi.Node) (sets.Set[string], error) {
 	return sets.New(cfg...), nil
 }
 
+// GetNodeHostAddrs returns the parsed Host CIDR annotation of the given node
+// as an array of strings. If the annotation is not set, then we return empty list.
+func GetNodeHostAddrs(node *kapi.Node) ([]string, error) {
+	hostAddresses, err := ParseNodeHostCIDRsDropNetMask(node)
+	if err != nil && !IsAnnotationNotSetError(err) {
+		return nil, fmt.Errorf("failed to get node host CIDRs for %s: %s", node.Name, err.Error())
+	}
+	return hostAddresses.UnsortedList(), nil
+}
+
 func ParseNodeHostCIDRsExcludeOVNNetworks(node *kapi.Node) ([]string, error) {
 	networks, err := ParseNodeHostCIDRsList(node)
 	if err != nil {