Skip to content

Commit 8581458

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #18547 from danwinship/egressip-add-delete-add-fix
Automatic merge from submit-queue (batch tested with PRs 18498, 18544, 18479, 18547). Fix cleanup of auto egress IPs when deleting a NetNamespace When deleting a NetNamespace, we weren't cleaning up all of the state correctly because we tried to make use of a struct field after its value had been cleared, meaning that complicated sequences of egress IP operations could result in junk rules getting added to OVS. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1543786
2 parents 6bd4094 + e23eb28 commit 8581458

File tree

2 files changed

+57
-2
lines changed

2 files changed

+57
-2
lines changed

‎pkg/network/node/egressip.go

+3-2
Original file line numberDiff line numberDiff line change
@@ -298,8 +298,9 @@ func (eip *egressIPWatcher) deleteNamespaceEgress(vnid uint32) {
298298

299299
if ns.assignedIP != "" {
300300
ns.requestedIP = ""
301-
eip.deleteEgressIP(ns.assignedIP)
302-
delete(eip.namespacesByEgressIP, ns.assignedIP)
301+
egressIP := ns.assignedIP
302+
eip.deleteEgressIP(egressIP)
303+
delete(eip.namespacesByEgressIP, egressIP)
303304
}
304305
delete(eip.namespacesByVNID, vnid)
305306
}

‎pkg/network/node/egressip_test.go

+54
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,10 @@ func TestEgressIP(t *testing.T) {
4040
eip.deleteNamespaceEgress(42)
4141
eip.deleteNamespaceEgress(43)
4242

43+
if len(eip.nodesByNodeIP) != 0 || len(eip.nodesByEgressIP) != 0 || len(eip.namespacesByVNID) != 0 || len(eip.namespacesByEgressIP) != 0 {
44+
t.Fatalf("Unexpected eip state: %#v", eip)
45+
}
46+
4347
// No namespaces use egress yet, so should be no changes
4448
err := assertNoNetlinkChanges(eip)
4549
if err != nil {
@@ -74,6 +78,11 @@ func TestEgressIP(t *testing.T) {
7478
t.Fatalf("Unexpected flow changes: %v", err)
7579
}
7680

81+
ns42 := eip.namespacesByVNID[42]
82+
if ns42 == nil || eip.namespacesByEgressIP["172.17.0.100"] != ns42 || eip.nodesByEgressIP["172.17.0.100"] != nil {
83+
t.Fatalf("Unexpected eip state: %#v", eip)
84+
}
85+
7786
eip.updateNodeEgress("172.17.0.3", []string{"172.17.0.100"})
7887
err = assertNoNetlinkChanges(eip)
7988
if err != nil {
@@ -94,6 +103,11 @@ func TestEgressIP(t *testing.T) {
94103
}
95104
origFlows = flows
96105

106+
node3 := eip.nodesByNodeIP["172.17.0.3"]
107+
if node3 == nil || eip.namespacesByEgressIP["172.17.0.100"] != ns42 || eip.nodesByEgressIP["172.17.0.100"] != node3 {
108+
t.Fatalf("Unexpected eip state: %#v", eip)
109+
}
110+
97111
// Assign HostSubnet.EgressIP first, then NetNamespace.EgressIP, with a remote EgressIP
98112
eip.updateNodeEgress("172.17.0.3", []string{"172.17.0.101", "172.17.0.100"})
99113
err = assertNoNetlinkChanges(eip)
@@ -109,6 +123,10 @@ func TestEgressIP(t *testing.T) {
109123
t.Fatalf("Unexpected flow changes: %v", err)
110124
}
111125

126+
if eip.nodesByEgressIP["172.17.0.100"] != node3 || eip.nodesByEgressIP["172.17.0.101"] != node3 {
127+
t.Fatalf("Unexpected eip state: %#v", eip)
128+
}
129+
112130
eip.updateNamespaceEgress(43, "172.17.0.101")
113131
err = assertNoNetlinkChanges(eip)
114132
if err != nil {
@@ -129,6 +147,11 @@ func TestEgressIP(t *testing.T) {
129147
}
130148
origFlows = flows
131149

150+
ns43 := eip.namespacesByVNID[43]
151+
if ns43 == nil || eip.namespacesByEgressIP["172.17.0.101"] != ns43 || eip.nodesByEgressIP["172.17.0.101"] != node3 {
152+
t.Fatalf("Unexpected eip state: %#v", eip)
153+
}
154+
132155
// Assign NetNamespace.EgressIP first, then HostSubnet.EgressIP, with a local EgressIP
133156
eip.updateNamespaceEgress(44, "172.17.0.102")
134157
err = assertNoNetlinkChanges(eip)
@@ -149,6 +172,11 @@ func TestEgressIP(t *testing.T) {
149172
t.Fatalf("Unexpected flow changes: %v", err)
150173
}
151174

175+
ns44 := eip.namespacesByVNID[44]
176+
if ns44 == nil || eip.namespacesByEgressIP["172.17.0.102"] != ns44 || eip.nodesByEgressIP["172.17.0.102"] != nil {
177+
t.Fatalf("Unexpected eip state: %#v", eip)
178+
}
179+
152180
eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.102"})
153181
err = assertNetlinkChange(eip, "claim 172.17.0.102")
154182
if err != nil {
@@ -169,6 +197,11 @@ func TestEgressIP(t *testing.T) {
169197
}
170198
origFlows = flows
171199

200+
node4 := eip.nodesByNodeIP["172.17.0.4"]
201+
if node4 == nil || eip.nodesByEgressIP["172.17.0.102"] != node4 {
202+
t.Fatalf("Unexpected eip state: %#v", eip)
203+
}
204+
172205
// Assign HostSubnet.EgressIP first, then NetNamespace.EgressIP, with a local EgressIP
173206
eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.102", "172.17.0.103"})
174207
err = assertNoNetlinkChanges(eip)
@@ -184,6 +217,10 @@ func TestEgressIP(t *testing.T) {
184217
t.Fatalf("Unexpected flow changes: %v", err)
185218
}
186219

220+
if eip.nodesByEgressIP["172.17.0.102"] != node4 || eip.nodesByEgressIP["172.17.0.103"] != node4 {
221+
t.Fatalf("Unexpected eip state: %#v", eip)
222+
}
223+
187224
eip.updateNamespaceEgress(45, "172.17.0.103")
188225
err = assertNetlinkChange(eip, "claim 172.17.0.103")
189226
if err != nil {
@@ -204,6 +241,11 @@ func TestEgressIP(t *testing.T) {
204241
}
205242
origFlows = flows
206243

244+
ns45 := eip.namespacesByVNID[45]
245+
if ns45 == nil || eip.namespacesByEgressIP["172.17.0.103"] != ns45 || eip.nodesByEgressIP["172.17.0.103"] != node4 {
246+
t.Fatalf("Unexpected eip state: %#v", eip)
247+
}
248+
207249
// Drop namespace EgressIP
208250
eip.deleteNamespaceEgress(44)
209251
err = assertNetlinkChange(eip, "release 172.17.0.102")
@@ -225,6 +267,10 @@ func TestEgressIP(t *testing.T) {
225267
}
226268
origFlows = flows
227269

270+
if eip.namespacesByVNID[44] != nil || eip.namespacesByEgressIP["172.17.0.102"] != nil || eip.nodesByEgressIP["172.17.0.102"] != node4 {
271+
t.Fatalf("Unexpected eip state: %#v", eip)
272+
}
273+
228274
// Drop remote node EgressIP
229275
eip.updateNodeEgress("172.17.0.3", []string{"172.17.0.100"})
230276
err = assertNoNetlinkChanges(eip)
@@ -250,6 +296,10 @@ func TestEgressIP(t *testing.T) {
250296
}
251297
origFlows = flows
252298

299+
if eip.namespacesByVNID[43] != ns43 || eip.namespacesByEgressIP["172.17.0.101"] != ns43 || eip.nodesByEgressIP["172.17.0.101"] != nil {
300+
t.Fatalf("Unexpected eip state: %#v", eip)
301+
}
302+
253303
// Drop local node EgressIP
254304
eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.102"})
255305
err = assertNetlinkChange(eip, "release 172.17.0.103")
@@ -275,6 +325,10 @@ func TestEgressIP(t *testing.T) {
275325
}
276326
origFlows = flows
277327

328+
if eip.namespacesByVNID[45] != ns45 || eip.namespacesByEgressIP["172.17.0.103"] != ns45 || eip.nodesByEgressIP["172.17.0.103"] != nil {
329+
t.Fatalf("Unexpected eip state: %#v", eip)
330+
}
331+
278332
// Trying to assign node IP as egress IP should fail. (It will log an error but this test doesn't notice that.)
279333
eip.updateNodeEgress("172.17.0.4", []string{"172.17.0.4", "172.17.0.102"})
280334
err = assertNoNetlinkChanges(eip)

0 commit comments

Comments
 (0)