From 9c76ad6bd5115edcd3b26d26277289f6d3784c4b Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 7 Jun 2018 13:48:12 -0400 Subject: [PATCH] Don't allow multiple egress IPs for the same namespace on the same node --- pkg/network/node/egressip.go | 53 +++++++++++++++++++------------ pkg/network/node/egressip_test.go | 39 ++++++++++++++++++++++- 2 files changed, 70 insertions(+), 22 deletions(-) diff --git a/pkg/network/node/egressip.go b/pkg/network/node/egressip.go index 791e80e21a27..b010e2107de9 100644 --- a/pkg/network/node/egressip.go +++ b/pkg/network/node/egressip.go @@ -133,11 +133,7 @@ func (eip *egressIPWatcher) egressIPChanged(eg *egressIPInfo) { func (eip *egressIPWatcher) addNode(egressIP string, node *nodeEgress) { eg := eip.ensureEgressIPInfo(egressIP) - if len(eg.nodes) != 0 { - utilruntime.HandleError(fmt.Errorf("Multiple nodes claiming EgressIP %q (nodes %q, %q)", eg.ip, node.nodeIP, eg.nodes[0].nodeIP)) - } eg.nodes = append(eg.nodes, node) - eip.egressIPChanged(eg) } @@ -158,11 +154,7 @@ func (eip *egressIPWatcher) deleteNode(egressIP string, node *nodeEgress) { func (eip *egressIPWatcher) addNamespace(egressIP string, ns *namespaceEgress) { eg := eip.ensureEgressIPInfo(egressIP) - if len(eg.namespaces) != 0 { - utilruntime.HandleError(fmt.Errorf("Multiple namespaces claiming EgressIP %q (NetIDs %d, %d)", eg.ip, ns.vnid, eg.namespaces[0].vnid)) - } eg.namespaces = append(eg.namespaces, ns) - eip.egressIPChanged(eg) } @@ -290,9 +282,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[ns] = true + // 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() } @@ -301,9 +295,32 @@ func (eip *egressIPWatcher) deleteNamespaceEgress(vnid uint32) { eip.updateNamespaceEgress(vnid, nil) } +func (eip *egressIPWatcher) egressIPActive(eg *egressIPInfo) (bool, error) { + if len(eg.nodes) == 0 || len(eg.namespaces) == 0 { + return false, nil + } + if len(eg.nodes) > 1 { + return false, fmt.Errorf("Multiple nodes (%s, %s) claiming EgressIP %s", eg.nodes[0].nodeIP, eg.nodes[1].nodeIP, eg.ip) + } + if len(eg.namespaces) > 1 { + return false, fmt.Errorf("Multiple namespaces (%d, %d) claiming EgressIP %s", eg.namespaces[0].vnid, eg.namespaces[1].vnid, eg.ip) + } + for _, ip := range eg.namespaces[0].requestedIPs { + eg2 := eip.egressIPs[ip] + if eg2 != eg && len(eg2.nodes) == 1 && eg2.nodes[0] == eg.nodes[0] { + return false, fmt.Errorf("Multiple EgressIPs (%s, %s) for VNID %d on node %s", eg.ip, eg2.ip, eg.namespaces[0].vnid, eg.nodes[0].nodeIP) + } + } + return true, nil +} + func (eip *egressIPWatcher) syncEgressIPs() { for eg := range eip.changedEgressIPs { - eip.syncEgressNodeState(eg) + active, err := eip.egressIPActive(eg) + if err != nil { + utilruntime.HandleError(err) + } + eip.syncEgressNodeState(eg, active) } eip.changedEgressIPs = make(map[*egressIPInfo]bool) @@ -316,12 +333,8 @@ func (eip *egressIPWatcher) syncEgressIPs() { eip.changedNamespaces = make(map[*namespaceEgress]bool) } -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) - if egressIPActive && eg.assignedNodeIP != eg.nodes[0].nodeIP { +func (eip *egressIPWatcher) syncEgressNodeState(eg *egressIPInfo, active bool) { + if active && 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 eg.assignedIPTablesMark = getMarkForVNID(eg.namespaces[0].vnid, eip.masqueradeBit) @@ -331,7 +344,7 @@ func (eip *egressIPWatcher) syncEgressNodeState(eg *egressIPInfo) { eg.assignedNodeIP = "" } } - } else if !egressIPActive && eg.assignedNodeIP != "" { + } else if !active && eg.assignedNodeIP != "" { glog.V(4).Infof("Removing egress IP %s from node %s", eg.ip, eg.assignedNodeIP) if eg.assignedNodeIP == eip.localIP { if err := eip.releaseEgressIP(eg.ip, eg.assignedIPTablesMark); err != nil { @@ -340,8 +353,6 @@ func (eip *egressIPWatcher) syncEgressNodeState(eg *egressIPInfo) { } eg.assignedNodeIP = "" eg.assignedIPTablesMark = "" - } else if !egressIPActive { - glog.V(4).Infof("Egress IP %s is not assignable (%d namespaces, %d nodes)", eg.ip, len(eg.namespaces), len(eg.nodes)) } } diff --git a/pkg/network/node/egressip_test.go b/pkg/network/node/egressip_test.go index bb9bbf57d96a..9a9c8e9b2139 100644 --- a/pkg/network/node/egressip_test.go +++ b/pkg/network/node/egressip_test.go @@ -352,6 +352,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) { // Now assigning that IP to a node should switch OVS to use that since it's first in the list 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}, ) @@ -361,6 +365,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) { // Swapping the order in the NetNamespace should swap back eip.updateNamespaceEgress(42, []string{"172.17.0.100", "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"}, ) @@ -370,6 +378,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 = 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"}, ) @@ -379,6 +391,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) { // Removing the remaining egress IP should now kill the namespace eip.updateNodeEgress("172.17.0.3", "", nil) + err = assertNoNetlinkChanges(eip) + if err != nil { + t.Fatalf("%v", err) + } err = assertOVSChanges(eip, &flows, egressOVSChange{vnid: 42, egress: Dropped}, ) @@ -389,6 +405,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 = assertNetlinkChange(eip, "claim 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"}, ) @@ -397,8 +417,13 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) { } // Assigning either the used or the unused Egress IP to another namespace should - // break this namespace + // break this namespace (and assigning the local egress IP to another namespace + // should cause the IP to be released). 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}, @@ -408,6 +433,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}, @@ -417,6 +446,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) { } eip.updateNamespaceEgress(44, []string{"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: Dropped}, egressOVSChange{vnid: 44, egress: Dropped}, @@ -426,6 +459,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) { } eip.deleteNamespaceEgress(44) + err = assertNetlinkChange(eip, "claim 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"}, egressOVSChange{vnid: 44, egress: Normal},