Skip to content

Commit 9c76ad6

Browse files
danwinshipdanwinship
committed
Don't allow multiple egress IPs for the same namespace on the same node
1 parent fa00719 commit 9c76ad6

File tree

2 files changed

+70
-22
lines changed

2 files changed

+70
-22
lines changed

‎pkg/network/node/egressip.go

Lines changed: 32 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -133,11 +133,7 @@ func (eip *egressIPWatcher) egressIPChanged(eg *egressIPInfo) {
133133

134134
func (eip *egressIPWatcher) addNode(egressIP string, node *nodeEgress) {
135135
eg := eip.ensureEgressIPInfo(egressIP)
136-
if len(eg.nodes) != 0 {
137-
utilruntime.HandleError(fmt.Errorf("Multiple nodes claiming EgressIP %q (nodes %q, %q)", eg.ip, node.nodeIP, eg.nodes[0].nodeIP))
138-
}
139136
eg.nodes = append(eg.nodes, node)
140-
141137
eip.egressIPChanged(eg)
142138
}
143139

@@ -158,11 +154,7 @@ func (eip *egressIPWatcher) deleteNode(egressIP string, node *nodeEgress) {
158154

159155
func (eip *egressIPWatcher) addNamespace(egressIP string, ns *namespaceEgress) {
160156
eg := eip.ensureEgressIPInfo(egressIP)
161-
if len(eg.namespaces) != 0 {
162-
utilruntime.HandleError(fmt.Errorf("Multiple namespaces claiming EgressIP %q (NetIDs %d, %d)", eg.ip, ns.vnid, eg.namespaces[0].vnid))
163-
}
164157
eg.namespaces = append(eg.namespaces, ns)
165-
166158
eip.egressIPChanged(eg)
167159
}
168160

@@ -290,9 +282,11 @@ func (eip *egressIPWatcher) updateNamespaceEgress(vnid uint32, egressIPs []strin
290282
eip.deleteNamespace(ip, ns)
291283
}
292284

293-
// Make sure we update OVS even if nothing was added or removed; the order might
294-
// have changed
295-
eip.changedNamespaces[ns] = true
285+
// Even IPs that weren't added/removed need to be considered "changed", to
286+
// ensure we correctly process reorderings, duplicates added/removed, etc.
287+
for _, ip := range newRequestedIPs.Intersection(oldRequestedIPs).UnsortedList() {
288+
eip.egressIPChanged(eip.egressIPs[ip])
289+
}
296290

297291
eip.syncEgressIPs()
298292
}
@@ -301,9 +295,32 @@ func (eip *egressIPWatcher) deleteNamespaceEgress(vnid uint32) {
301295
eip.updateNamespaceEgress(vnid, nil)
302296
}
303297

298+
func (eip *egressIPWatcher) egressIPActive(eg *egressIPInfo) (bool, error) {
299+
if len(eg.nodes) == 0 || len(eg.namespaces) == 0 {
300+
return false, nil
301+
}
302+
if len(eg.nodes) > 1 {
303+
return false, fmt.Errorf("Multiple nodes (%s, %s) claiming EgressIP %s", eg.nodes[0].nodeIP, eg.nodes[1].nodeIP, eg.ip)
304+
}
305+
if len(eg.namespaces) > 1 {
306+
return false, fmt.Errorf("Multiple namespaces (%d, %d) claiming EgressIP %s", eg.namespaces[0].vnid, eg.namespaces[1].vnid, eg.ip)
307+
}
308+
for _, ip := range eg.namespaces[0].requestedIPs {
309+
eg2 := eip.egressIPs[ip]
310+
if eg2 != eg && len(eg2.nodes) == 1 && eg2.nodes[0] == eg.nodes[0] {
311+
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)
312+
}
313+
}
314+
return true, nil
315+
}
316+
304317
func (eip *egressIPWatcher) syncEgressIPs() {
305318
for eg := range eip.changedEgressIPs {
306-
eip.syncEgressNodeState(eg)
319+
active, err := eip.egressIPActive(eg)
320+
if err != nil {
321+
utilruntime.HandleError(err)
322+
}
323+
eip.syncEgressNodeState(eg, active)
307324
}
308325
eip.changedEgressIPs = make(map[*egressIPInfo]bool)
309326

@@ -316,12 +333,8 @@ func (eip *egressIPWatcher) syncEgressIPs() {
316333
eip.changedNamespaces = make(map[*namespaceEgress]bool)
317334
}
318335

319-
func (eip *egressIPWatcher) syncEgressNodeState(eg *egressIPInfo) {
320-
// The egressIPInfo should have an assigned node IP if and only if the
321-
// egress IP is active (ie, it is assigned to exactly 1 node and exactly
322-
// 1 namespace).
323-
egressIPActive := (len(eg.nodes) == 1 && len(eg.namespaces) == 1)
324-
if egressIPActive && eg.assignedNodeIP != eg.nodes[0].nodeIP {
336+
func (eip *egressIPWatcher) syncEgressNodeState(eg *egressIPInfo, active bool) {
337+
if active && eg.assignedNodeIP != eg.nodes[0].nodeIP {
325338
glog.V(4).Infof("Assigning egress IP %s to node %s", eg.ip, eg.nodes[0].nodeIP)
326339
eg.assignedNodeIP = eg.nodes[0].nodeIP
327340
eg.assignedIPTablesMark = getMarkForVNID(eg.namespaces[0].vnid, eip.masqueradeBit)
@@ -331,7 +344,7 @@ func (eip *egressIPWatcher) syncEgressNodeState(eg *egressIPInfo) {
331344
eg.assignedNodeIP = ""
332345
}
333346
}
334-
} else if !egressIPActive && eg.assignedNodeIP != "" {
347+
} else if !active && eg.assignedNodeIP != "" {
335348
glog.V(4).Infof("Removing egress IP %s from node %s", eg.ip, eg.assignedNodeIP)
336349
if eg.assignedNodeIP == eip.localIP {
337350
if err := eip.releaseEgressIP(eg.ip, eg.assignedIPTablesMark); err != nil {
@@ -340,8 +353,6 @@ func (eip *egressIPWatcher) syncEgressNodeState(eg *egressIPInfo) {
340353
}
341354
eg.assignedNodeIP = ""
342355
eg.assignedIPTablesMark = ""
343-
} else if !egressIPActive {
344-
glog.V(4).Infof("Egress IP %s is not assignable (%d namespaces, %d nodes)", eg.ip, len(eg.namespaces), len(eg.nodes))
345356
}
346357
}
347358

‎pkg/network/node/egressip_test.go

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
352352

353353
// Now assigning that IP to a node should switch OVS to use that since it's first in the list
354354
eip.updateNodeEgress("172.17.0.4", "", []string{"172.17.0.101"})
355+
err = assertNetlinkChange(eip, "claim 172.17.0.101")
356+
if err != nil {
357+
t.Fatalf("%v", err)
358+
}
355359
err = assertOVSChanges(eip, &flows,
356360
egressOVSChange{vnid: 42, egress: Local},
357361
)
@@ -361,6 +365,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
361365

362366
// Swapping the order in the NetNamespace should swap back
363367
eip.updateNamespaceEgress(42, []string{"172.17.0.100", "172.17.0.101"})
368+
err = assertNoNetlinkChanges(eip)
369+
if err != nil {
370+
t.Fatalf("%v", err)
371+
}
364372
err = assertOVSChanges(eip, &flows,
365373
egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
366374
)
@@ -370,6 +378,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
370378

371379
// Removing the inactive egress IP from its node should have no effect
372380
eip.updateNodeEgress("172.17.0.4", "", []string{"172.17.0.200"})
381+
err = assertNetlinkChange(eip, "release 172.17.0.101")
382+
if err != nil {
383+
t.Fatalf("%v", err)
384+
}
373385
err = assertOVSChanges(eip, &flows,
374386
egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
375387
)
@@ -379,6 +391,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
379391

380392
// Removing the remaining egress IP should now kill the namespace
381393
eip.updateNodeEgress("172.17.0.3", "", nil)
394+
err = assertNoNetlinkChanges(eip)
395+
if err != nil {
396+
t.Fatalf("%v", err)
397+
}
382398
err = assertOVSChanges(eip, &flows,
383399
egressOVSChange{vnid: 42, egress: Dropped},
384400
)
@@ -389,6 +405,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
389405
// Now add the egress IPs back...
390406
eip.updateNodeEgress("172.17.0.3", "", []string{"172.17.0.100"})
391407
eip.updateNodeEgress("172.17.0.4", "", []string{"172.17.0.101"})
408+
err = assertNetlinkChange(eip, "claim 172.17.0.101")
409+
if err != nil {
410+
t.Fatalf("%v", err)
411+
}
392412
err = assertOVSChanges(eip, &flows,
393413
egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
394414
)
@@ -397,8 +417,13 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
397417
}
398418

399419
// Assigning either the used or the unused Egress IP to another namespace should
400-
// break this namespace
420+
// break this namespace (and assigning the local egress IP to another namespace
421+
// should cause the IP to be released).
401422
eip.updateNamespaceEgress(43, []string{"172.17.0.100"})
423+
err = assertNoNetlinkChanges(eip)
424+
if err != nil {
425+
t.Fatalf("%v", err)
426+
}
402427
err = assertOVSChanges(eip, &flows,
403428
egressOVSChange{vnid: 42, egress: Dropped},
404429
egressOVSChange{vnid: 43, egress: Dropped},
@@ -408,6 +433,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
408433
}
409434

410435
eip.deleteNamespaceEgress(43)
436+
err = assertNoNetlinkChanges(eip)
437+
if err != nil {
438+
t.Fatalf("%v", err)
439+
}
411440
err = assertOVSChanges(eip, &flows,
412441
egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
413442
egressOVSChange{vnid: 43, egress: Normal},
@@ -417,6 +446,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
417446
}
418447

419448
eip.updateNamespaceEgress(44, []string{"172.17.0.101"})
449+
err = assertNetlinkChange(eip, "release 172.17.0.101")
450+
if err != nil {
451+
t.Fatalf("%v", err)
452+
}
420453
err = assertOVSChanges(eip, &flows,
421454
egressOVSChange{vnid: 42, egress: Dropped},
422455
egressOVSChange{vnid: 44, egress: Dropped},
@@ -426,6 +459,10 @@ func TestMultipleNamespaceEgressIPs(t *testing.T) {
426459
}
427460

428461
eip.deleteNamespaceEgress(44)
462+
err = assertNetlinkChange(eip, "claim 172.17.0.101")
463+
if err != nil {
464+
t.Fatalf("%v", err)
465+
}
429466
err = assertOVSChanges(eip, &flows,
430467
egressOVSChange{vnid: 42, egress: Remote, remote: "172.17.0.3"},
431468
egressOVSChange{vnid: 44, egress: Normal},

0 commit comments

Comments
 (0)