Merge pull request #19926 from danwinship/egress-ip-duplicates

Fix a bug when a namespace has two egress IPs on same node

Author: openshift-merge-robot

pkg/network/node/egressip.go

@@ -271,9 +271,11 @@ func (eip *egressIPWatcher) updateNamespaceEgress(vnid uint32, egressIPs []strin
 		eip.deleteNamespace(ip, ns)
 	}
 
-	// Make sure we update OVS even if nothing was added or removed; the order might
-	// have changed
-	eip.changedNamespaces = append(eip.changedNamespaces, ns)
+	// Even IPs that weren't added/removed need to be considered "changed", to
+	// ensure we correctly process reorderings, duplicates added/removed, etc.
+	for _, ip := range newRequestedIPs.Intersection(oldRequestedIPs).UnsortedList() {
+		eip.egressIPChanged(eip.egressIPs[ip])
+	}
 
 	eip.syncEgressIPs()
 }
@@ -310,8 +312,8 @@ func (eip *egressIPWatcher) syncEgressIPs() {
 func (eip *egressIPWatcher) syncEgressNodeState(eg *egressIPInfo) {
 	// The egressIPInfo should have an assigned node IP if and only if the
 	// egress IP is active (ie, it is assigned to exactly 1 node and exactly
-	// 1 namespace).
-	egressIPActive := (len(eg.nodes) == 1 && len(eg.namespaces) == 1)
+	// 1 namespace, and it's the first one for its namespace).
+	egressIPActive := (len(eg.nodes) == 1 && len(eg.namespaces) == 1 && eg.ip == eg.namespaces[0].requestedIPs[0])
 	if egressIPActive && eg.assignedNodeIP != eg.nodes[0].nodeIP {
 		glog.V(4).Infof("Assigning egress IP %s to node %s", eg.ip, eg.nodes[0].nodeIP)
 		eg.assignedNodeIP = eg.nodes[0].nodeIP

pkg/network/node/egressip_test.go

@@ -355,6 +355,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
 
 	// Assigning that IP to a node should now make it work
 	eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.101"})
+	err = assertNetlinkChange(eip, "claim 172.17.0.101")
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
 	err = assertOVSChanges(eip, &flows,
 		egressOVSChange{vnid: 42, egress: Local},
 	)
@@ -364,6 +368,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
 
 	// Swapping the order in the NetNamespace should bring the original Egress IP back
 	eip.updateNamespaceEgress(42, []string{"172.17.0.100", "172.17.0.101"})
+	err = assertNetlinkChange(eip, "release 172.17.0.101")
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
 	err = assertOVSChanges(eip, &flows,
 		egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
 	)
@@ -373,6 +381,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
 
 	// Removing the inactive egress IP from its node should have no effect
 	eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.200"})
+	err = assertNoNetlinkChanges(eip)
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
 	err = assertOVSChanges(eip, &flows,
 		egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
 	)
@@ -392,6 +404,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
 	// Now add the egress IPs back...
 	eip.updateNodeEgress("172.17.0.3", []string{"172.17.0.100"})
 	eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.101"})
+	err = assertNoNetlinkChanges(eip)
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
 	err = assertOVSChanges(eip, &flows,
 		egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
 	)
@@ -402,6 +418,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
 	// Assigning either the used or the unused Egress IP to another namespace should
 	// break this namespace
 	eip.updateNamespaceEgress(43, []string{"172.17.0.100"})
+	err = assertNoNetlinkChanges(eip)
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
 	err = assertOVSChanges(eip, &flows,
 		egressOVSChange{vnid: 42, egress: Dropped},
 		egressOVSChange{vnid: 43, egress: Dropped},
@@ -411,6 +431,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
 	}
 
 	eip.deleteNamespaceEgress(43)
+	err = assertNoNetlinkChanges(eip)
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
 	err = assertOVSChanges(eip, &flows,
 		egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
 		egressOVSChange{vnid: 43, egress: Normal},
@@ -420,6 +444,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
 	}
 
 	eip.updateNamespaceEgress(44, []string{"172.17.0.101"})
+	err = assertNoNetlinkChanges(eip)
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
 	err = assertOVSChanges(eip, &flows,
 		egressOVSChange{vnid: 42, egress: Dropped},
 		egressOVSChange{vnid: 44, egress: Dropped},
@@ -429,6 +457,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
 	}
 
 	eip.deleteNamespaceEgress(44)
+	err = assertNoNetlinkChanges(eip)
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
 	err = assertOVSChanges(eip, &flows,
 		egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
 		egressOVSChange{vnid: 44, egress: Normal},