Merge pull request #16659 from danwinship/auto-egress-ip-fixes

Automatic merge from submit-queue (batch tested with PRs 16625, 16659).

auto egress IP fixes

Some fixes to the auto egress IP code

api/swagger-spec/oapi-v1.json

@@ -25756,8 +25756,7 @@
     "required": [
      "host",
      "hostIP",
-     "subnet",
-     "egressIPs"
+     "subnet"
     ],
     "properties": {
      "kind": {
@@ -26916,8 +26915,7 @@
     "description": "NetNamespace describes a single isolated network. When using the redhat/openshift-ovs-multitenant plugin, every Namespace will have a corresponding NetNamespace object with the same name. (When using redhat/openshift-ovs-subnet, NetNamespaces are not used.)",
     "required": [
      "netname",
-     "netid",
-     "egressIPs"
+     "netid"
     ],
     "properties": {
      "kind": {

api/swagger-spec/openshift-openapi-spec.json

@@ -90978,8 +90978,7 @@
     "required": [
      "host",
      "hostIP",
-     "subnet",
-     "egressIPs"
+     "subnet"
     ],
     "properties": {
      "apiVersion": {

pkg/network/apis/network/v1/types.go

@@ -70,7 +70,8 @@ type HostSubnet struct {
 	Subnet string `json:"subnet" protobuf:"bytes,4,opt,name=subnet"`
 
 	// EgressIPs is the list of automatic egress IP addresses currently hosted by this node
-	EgressIPs []string `json:"egressIPs" protobuf:"bytes,5,rep,name=egressIPs"`
+	// +optional
+	EgressIPs []string `json:"egressIPs,omitempty" protobuf:"bytes,5,rep,name=egressIPs"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
@@ -103,7 +104,7 @@ type NetNamespace struct {
 
 	// EgressIPs is a list of reserved IPs that will be used as the source for external traffic coming from pods in this namespace. (If empty, external traffic will be masqueraded to Node IPs.)
 	// +optional
-	EgressIPs []string `json:"egressIPs" protobuf:"bytes,4,rep,name=egressIPs"`
+	EgressIPs []string `json:"egressIPs,omitempty" protobuf:"bytes,4,rep,name=egressIPs"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

pkg/network/apis/network/v1/zz_generated.conversion.go

@@ -287,11 +287,7 @@ func autoConvert_network_HostSubnet_To_v1_HostSubnet(in *network.HostSubnet, out
 	out.Host = in.Host
 	out.HostIP = in.HostIP
 	out.Subnet = in.Subnet
-	if in.EgressIPs == nil {
-		out.EgressIPs = make([]string, 0)
-	} else {
-		out.EgressIPs = *(*[]string)(unsafe.Pointer(&in.EgressIPs))
-	}
+	out.EgressIPs = *(*[]string)(unsafe.Pointer(&in.EgressIPs))
 	return nil
 }
 
@@ -343,11 +339,7 @@ func autoConvert_network_NetNamespace_To_v1_NetNamespace(in *network.NetNamespac
 	out.ObjectMeta = in.ObjectMeta
 	out.NetName = in.NetName
 	out.NetID = in.NetID
-	if in.EgressIPs == nil {
-		out.EgressIPs = make([]string, 0)
-	} else {
-		out.EgressIPs = *(*[]string)(unsafe.Pointer(&in.EgressIPs))
-	}
+	out.EgressIPs = *(*[]string)(unsafe.Pointer(&in.EgressIPs))
 	return nil
 }
 

pkg/network/node/egressip.go

@@ -97,12 +97,12 @@ func (eip *egressIPWatcher) watchHostSubnets() {
 			egressIPs = hs.EgressIPs
 		}
 
-		eip.updateNode(hs.HostIP, egressIPs)
+		eip.updateNodeEgress(hs.HostIP, egressIPs)
 		return nil
 	})
 }
 
-func (eip *egressIPWatcher) updateNode(nodeIP string, nodeEgressIPs []string) {
+func (eip *egressIPWatcher) updateNodeEgress(nodeIP string, nodeEgressIPs []string) {
 	eip.Lock()
 	defer eip.Unlock()
 
@@ -121,6 +121,11 @@ func (eip *egressIPWatcher) updateNode(nodeIP string, nodeEgressIPs []string) {
 
 	// Process new EgressIPs
 	for _, ip := range node.egressIPs.Difference(oldEgressIPs).UnsortedList() {
+		if oldNode := eip.nodesByEgressIP[ip]; oldNode != nil {
+			glog.Errorf("Multiple nodes claiming EgressIP %q (nodes %q, %q)", ip, node.nodeIP, oldNode.nodeIP)
+			continue
+		}
+
 		eip.nodesByEgressIP[ip] = node
 		hex := ipToHex(ip)
 		claimedNodeIP := nodeIP
@@ -172,31 +177,34 @@ func (eip *egressIPWatcher) watchNetNamespaces() {
 	common.RunEventQueue(eip.networkClient.Network().RESTClient(), common.NetNamespaces, func(delta cache.Delta) error {
 		netns := delta.Object.(*networkapi.NetNamespace)
 
-		var egressIP string
 		if delta.Type != cache.Deleted && len(netns.EgressIPs) != 0 {
-			egressIP = netns.EgressIPs[0]
+			if len(netns.EgressIPs) > 1 {
+				glog.Warningf("Ignoring extra EgressIPs (%v) in NetNamespace %q", netns.EgressIPs[1:], netns.Name)
+			}
+			eip.updateNamespaceEgress(netns.NetID, netns.EgressIPs[0])
+		} else {
+			eip.deleteNamespaceEgress(netns.NetID)
 		}
-
-		eip.updateNamespace(netns.NetID, egressIP)
 		return nil
 	})
 }
 
-func (eip *egressIPWatcher) updateNamespace(vnid uint32, egressIP string) {
+func (eip *egressIPWatcher) updateNamespaceEgress(vnid uint32, egressIP string) {
 	eip.Lock()
 	defer eip.Unlock()
 
 	ns := eip.namespacesByVNID[vnid]
 	if ns == nil {
-		if egressIP == "" {
-			return
-		}
 		ns = &namespaceEgress{vnid: vnid}
 		eip.namespacesByVNID[vnid] = ns
 	}
 	if ns.claimedIP == egressIP {
 		return
 	}
+	if oldNS := eip.namespacesByEgressIP[egressIP]; oldNS != nil {
+		glog.Errorf("Multiple NetNamespaces claiming EgressIP %q (NetIDs %d, %d)", egressIP, ns.vnid, oldNS.vnid)
+		return
+	}
 
 	if ns.claimedIP != "" {
 		delete(eip.namespacesByEgressIP, ns.claimedIP)
@@ -210,12 +218,27 @@ func (eip *egressIPWatcher) updateNamespace(vnid uint32, egressIP string) {
 		ns.nodeIP = node.nodeIP
 	}
 
-	egressHex := ""
-	if egressIP != "" {
-		egressHex = ipToHex(egressIP)
+	err := eip.oc.UpdateNamespaceEgressRules(ns.vnid, ns.nodeIP, ipToHex(egressIP))
+	if err != nil {
+		glog.Errorf("Error updating Namespace egress rules: %v", err)
+	}
+}
+
+func (eip *egressIPWatcher) deleteNamespaceEgress(vnid uint32) {
+	eip.Lock()
+	defer eip.Unlock()
+
+	ns := eip.namespacesByVNID[vnid]
+	if ns == nil {
+		return
+	}
+
+	if ns.claimedIP != "" {
+		delete(eip.namespacesByEgressIP, ns.claimedIP)
 	}
+	delete(eip.namespacesByVNID, vnid)
 
-	err := eip.oc.UpdateNamespaceEgressRules(ns.vnid, ns.nodeIP, egressHex)
+	err := eip.oc.UpdateNamespaceEgressRules(ns.vnid, "", "")
 	if err != nil {
 		glog.Errorf("Error updating Namespace egress rules: %v", err)
 	}
@@ -236,7 +259,8 @@ func (eip *egressIPWatcher) claimEgressIP(egressIP, egressHex string) error {
 		for _, link := range links {
 			addrs, err := netlink.AddrList(link, syscall.AF_INET)
 			if err != nil {
-				return fmt.Errorf("could not get addresses of interface %q while adding egress IP: %v", link.Attrs().Name, err)
+				glog.Warningf("Could not get addresses of interface %q while trying to find egress interface: %v", link.Attrs().Name, err)
+				continue
 			}
 
 			for _, addr := range addrs {

pkg/network/node/egressip_test.go

@@ -34,10 +34,10 @@ func TestEgressIP(t *testing.T) {
 	eip := newEgressIPWatcher("172.17.0.4", oc)
 	eip.testModeChan = make(chan string, 10)
 
-	eip.updateNode("172.17.0.3", []string{})
-	eip.updateNode("172.17.0.4", []string{})
-	eip.updateNamespace(42, "")
-	eip.updateNamespace(43, "")
+	eip.updateNodeEgress("172.17.0.3", []string{})
+	eip.updateNodeEgress("172.17.0.4", []string{})
+	eip.deleteNamespaceEgress(42)
+	eip.deleteNamespaceEgress(43)
 
 	// No namespaces use egress yet, so should be no changes
 	err := assertNoNetlinkChanges(eip)
@@ -54,7 +54,7 @@ func TestEgressIP(t *testing.T) {
 	}
 
 	// Assign NetNamespace.EgressIP first, then HostSubnet.EgressIP, with a remote EgressIP
-	eip.updateNamespace(42, "172.17.0.100")
+	eip.updateNamespaceEgress(42, "172.17.0.100")
 	err = assertNoNetlinkChanges(eip)
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -73,7 +73,7 @@ func TestEgressIP(t *testing.T) {
 		t.Fatalf("Unexpected flow changes: %v", err)
 	}
 
-	eip.updateNode("172.17.0.3", []string{"172.17.0.100"})
+	eip.updateNodeEgress("172.17.0.3", []string{"172.17.0.100"})
 	err = assertNoNetlinkChanges(eip)
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -94,7 +94,7 @@ func TestEgressIP(t *testing.T) {
 	origFlows = flows
 
 	// Assign HostSubnet.EgressIP first, then NetNamespace.EgressIP, with a remote EgressIP
-	eip.updateNode("172.17.0.3", []string{"172.17.0.101", "172.17.0.100"})
+	eip.updateNodeEgress("172.17.0.3", []string{"172.17.0.101", "172.17.0.100"})
 	err = assertNoNetlinkChanges(eip)
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -108,7 +108,7 @@ func TestEgressIP(t *testing.T) {
 		t.Fatalf("Unexpected flow changes: %v", err)
 	}
 
-	eip.updateNamespace(43, "172.17.0.101")
+	eip.updateNamespaceEgress(43, "172.17.0.101")
 	err = assertNoNetlinkChanges(eip)
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -129,7 +129,7 @@ func TestEgressIP(t *testing.T) {
 	origFlows = flows
 
 	// Assign NetNamespace.EgressIP first, then HostSubnet.EgressIP, with a local EgressIP
-	eip.updateNamespace(44, "172.17.0.102")
+	eip.updateNamespaceEgress(44, "172.17.0.102")
 	err = assertNoNetlinkChanges(eip)
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -148,7 +148,7 @@ func TestEgressIP(t *testing.T) {
 		t.Fatalf("Unexpected flow changes: %v", err)
 	}
 
-	eip.updateNode("172.17.0.4", []string{"172.17.0.102"})
+	eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.102"})
 	err = assertNetlinkChange(eip, "claim 172.17.0.102")
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -169,7 +169,7 @@ func TestEgressIP(t *testing.T) {
 	origFlows = flows
 
 	// Assign HostSubnet.EgressIP first, then NetNamespace.EgressIP, with a local EgressIP
-	eip.updateNode("172.17.0.4", []string{"172.17.0.102", "172.17.0.103"})
+	eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.102", "172.17.0.103"})
 	err = assertNetlinkChange(eip, "claim 172.17.0.103")
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -183,7 +183,7 @@ func TestEgressIP(t *testing.T) {
 		t.Fatalf("Unexpected flow changes: %v", err)
 	}
 
-	eip.updateNamespace(45, "172.17.0.103")
+	eip.updateNamespaceEgress(45, "172.17.0.103")
 	err = assertNoNetlinkChanges(eip)
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -204,7 +204,7 @@ func TestEgressIP(t *testing.T) {
 	origFlows = flows
 
 	// Drop namespace EgressIP
-	eip.updateNamespace(44, "")
+	eip.deleteNamespaceEgress(44)
 	err = assertNoNetlinkChanges(eip)
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -225,7 +225,7 @@ func TestEgressIP(t *testing.T) {
 	origFlows = flows
 
 	// Drop remote node EgressIP
-	eip.updateNode("172.17.0.3", []string{"172.17.0.100"})
+	eip.updateNodeEgress("172.17.0.3", []string{"172.17.0.100"})
 	err = assertNoNetlinkChanges(eip)
 	if err != nil {
 		t.Fatalf("%v", err)
@@ -250,7 +250,7 @@ func TestEgressIP(t *testing.T) {
 	origFlows = flows
 
 	// Drop local node EgressIP
-	eip.updateNode("172.17.0.4", []string{"172.17.0.102"})
+	eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.102"})
 	err = assertNetlinkChange(eip, "release 172.17.0.103")
 	if err != nil {
 		t.Fatalf("%v", err)

pkg/network/node/multitenant.go

@@ -33,6 +33,10 @@ func (mp *multiTenantPlugin) Name() string {
 	return network.MultiTenantPluginName
 }
 
+func (mp *multiTenantPlugin) SupportsVNIDs() bool {
+	return true
+}
+
 func (mp *multiTenantPlugin) Start(node *OsdnNode) error {
 	mp.node = node
 	mp.vnids = newNodeVNIDMap(mp, node.networkClient)
@@ -47,10 +51,6 @@ func (mp *multiTenantPlugin) Start(node *OsdnNode) error {
 		return err
 	}
 
-	if err := mp.node.SetupEgressNetworkPolicy(); err != nil {
-		return err
-	}
-
 	return nil
 }
 

pkg/network/node/networkpolicy.go

@@ -72,6 +72,10 @@ func (np *networkPolicyPlugin) Name() string {
 	return network.NetworkPolicyPluginName
 }
 
+func (np *networkPolicyPlugin) SupportsVNIDs() bool {
+	return true
+}
+
 func (np *networkPolicyPlugin) Start(node *OsdnNode) error {
 	np.node = node
 	np.kubeInformers = node.kubeInformers
@@ -92,9 +96,6 @@ func (np *networkPolicyPlugin) Start(node *OsdnNode) error {
 	if err := np.initNamespaces(); err != nil {
 		return err
 	}
-	if err := np.node.SetupEgressNetworkPolicy(); err != nil {
-		return err
-	}
 
 	np.watchNamespaces()
 	np.watchPods()

pkg/network/node/node.go

@@ -51,6 +51,7 @@ const (
 type osdnPolicy interface {
 	Name() string
 	Start(node *OsdnNode) error
+	SupportsVNIDs() bool
 
 	AddNetNamespace(netns *networkapi.NetNamespace)
 	UpdateNetNamespace(netns *networkapi.NetNamespace, oldNetID uint32)
@@ -337,12 +338,17 @@ func (node *OsdnNode) Start() error {
 	if err != nil {
 		return err
 	}
-	if err = node.egressIP.Start(node.networkClient, nodeIPTables); err != nil {
-		return err
-	}
 	if err = node.policy.Start(node); err != nil {
 		return err
 	}
+	if node.policy.SupportsVNIDs() {
+		if err := node.SetupEgressNetworkPolicy(); err != nil {
+			return err
+		}
+		if err = node.egressIP.Start(node.networkClient, nodeIPTables); err != nil {
+			return err
+		}
+	}
 	if !node.useConnTrack {
 		node.watchServices()
 	}

pkg/network/node/singletenant.go

@@ -17,6 +17,10 @@ func (sp *singleTenantPlugin) Name() string {
 	return network.SingleTenantPluginName
 }
 
+func (sp *singleTenantPlugin) SupportsVNIDs() bool {
+	return false
+}
+
 func (sp *singleTenantPlugin) Start(node *OsdnNode) error {
 	otx := node.oc.NewTransaction()
 	otx.AddFlow("table=80, priority=200, actions=output:NXM_NX_REG2[]")