Skip to content

Commit 7269ac1

Browse files
danwinshipdanwinship
committed
Don't read back veth MAC; we already know it's based on the IP
1 parent 9709c73 commit 7269ac1

File tree

3 files changed

+61
-51
lines changed

3 files changed

+61
-51
lines changed

‎pkg/network/node/ovscontroller.go‎

Lines changed: 36 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -226,40 +226,46 @@ func (oc *ovsController) ensureOvsPort(hostVeth, sandboxID string) (int, error)
226226
return oc.ovs.AddPort(hostVeth, -1, "external-ids=sandbox="+sandboxID)
227227
}
228228

229-
func (oc *ovsController) setupPodFlows(ofport int, podIP, podMAC string, vnid uint32) error {
229+
func (oc *ovsController) setupPodFlows(ofport int, podIP net.IP, vnid uint32) error {
230230
otx := oc.ovs.NewTransaction()
231231

232+
ipstr := podIP.String()
233+
podIP = podIP.To4()
234+
ipmac := fmt.Sprintf("00:00:%02x:%02x:%02x:%02x/00:00:ff:ff:ff:ff", podIP[0], podIP[1], podIP[2], podIP[3])
235+
232236
// ARP/IP traffic from container
233-
otx.AddFlow("table=20, priority=100, in_port=%d, arp, nw_src=%s, arp_sha=%s, actions=load:%d->NXM_NX_REG0[], goto_table:21", ofport, podIP, podMAC, vnid)
234-
otx.AddFlow("table=20, priority=100, in_port=%d, ip, nw_src=%s, actions=load:%d->NXM_NX_REG0[], goto_table:21", ofport, podIP, vnid)
237+
otx.AddFlow("table=20, priority=100, in_port=%d, arp, nw_src=%s, arp_sha=%s, actions=load:%d->NXM_NX_REG0[], goto_table:21", ofport, ipstr, ipmac, vnid)
238+
otx.AddFlow("table=20, priority=100, in_port=%d, ip, nw_src=%s, actions=load:%d->NXM_NX_REG0[], goto_table:21", ofport, ipstr, vnid)
235239
if oc.useConnTrack {
236-
otx.AddFlow("table=25, priority=100, ip, nw_src=%s, actions=load:%d->NXM_NX_REG0[], goto_table:30", podIP, vnid)
240+
otx.AddFlow("table=25, priority=100, ip, nw_src=%s, actions=load:%d->NXM_NX_REG0[], goto_table:30", ipstr, vnid)
237241
}
238242

239243
// ARP request/response to container (not isolated)
240-
otx.AddFlow("table=40, priority=100, arp, nw_dst=%s, actions=output:%d", podIP, ofport)
244+
otx.AddFlow("table=40, priority=100, arp, nw_dst=%s, actions=output:%d", ipstr, ofport)
241245

242246
// IP traffic to container
243-
otx.AddFlow("table=70, priority=100, ip, nw_dst=%s, actions=load:%d->NXM_NX_REG1[], load:%d->NXM_NX_REG2[], goto_table:80", podIP, vnid, ofport)
247+
otx.AddFlow("table=70, priority=100, ip, nw_dst=%s, actions=load:%d->NXM_NX_REG1[], load:%d->NXM_NX_REG2[], goto_table:80", ipstr, vnid, ofport)
244248

245249
return otx.EndTransaction()
246250
}
247251

248-
func (oc *ovsController) cleanupPodFlows(podIP string) error {
252+
func (oc *ovsController) cleanupPodFlows(podIP net.IP) error {
253+
ipstr := podIP.String()
254+
249255
otx := oc.ovs.NewTransaction()
250-
otx.DeleteFlows("ip, nw_dst=%s", podIP)
251-
otx.DeleteFlows("ip, nw_src=%s", podIP)
252-
otx.DeleteFlows("arp, nw_dst=%s", podIP)
253-
otx.DeleteFlows("arp, nw_src=%s", podIP)
256+
otx.DeleteFlows("ip, nw_dst=%s", ipstr)
257+
otx.DeleteFlows("ip, nw_src=%s", ipstr)
258+
otx.DeleteFlows("arp, nw_dst=%s", ipstr)
259+
otx.DeleteFlows("arp, nw_src=%s", ipstr)
254260
return otx.EndTransaction()
255261
}
256262

257-
func (oc *ovsController) SetUpPod(hostVeth, podIP, podMAC, sandboxID string, vnid uint32) (int, error) {
263+
func (oc *ovsController) SetUpPod(sandboxID, hostVeth string, podIP net.IP, vnid uint32) (int, error) {
258264
ofport, err := oc.ensureOvsPort(hostVeth, sandboxID)
259265
if err != nil {
260266
return -1, err
261267
}
262-
return ofport, oc.setupPodFlows(ofport, podIP, podMAC, vnid)
268+
return ofport, oc.setupPodFlows(ofport, podIP, vnid)
263269
}
264270

265271
// Returned list can also be used for port names
@@ -322,64 +328,58 @@ func (oc *ovsController) SetPodBandwidth(hostVeth, sandboxID string, ingressBPS,
322328
return nil
323329
}
324330

325-
func (oc *ovsController) getPodDetailsBySandboxID(sandboxID string) (int, string, string, error) {
331+
func (oc *ovsController) getPodDetailsBySandboxID(sandboxID string) (int, net.IP, error) {
326332
strports, err := oc.ovs.Find("interface", "ofport", "external-ids:sandbox="+sandboxID)
327333
if err != nil {
328-
return 0, "", "", err
334+
return 0, nil, err
329335
} else if len(strports) == 0 {
330-
return 0, "", "", fmt.Errorf("failed to find pod details from OVS flows")
336+
return 0, nil, fmt.Errorf("failed to find pod details from OVS flows")
331337
} else if len(strports) > 1 {
332-
return 0, "", "", fmt.Errorf("found multiple ofports for sandbox ID %q: %#v", sandboxID, strports)
338+
return 0, nil, fmt.Errorf("found multiple ofports for sandbox ID %q: %#v", sandboxID, strports)
333339
}
334340
ofport, err := strconv.Atoi(strports[0])
335341
if err != nil {
336-
return 0, "", "", fmt.Errorf("could not parse ofport %q: %v", strports[0], err)
342+
return 0, nil, fmt.Errorf("could not parse ofport %q: %v", strports[0], err)
337343
}
338344

339345
flows, err := oc.ovs.DumpFlows("table=20,arp,in_port=%d", ofport)
340346
if err != nil {
341-
return 0, "", "", err
347+
return 0, nil, err
342348
} else if len(flows) != 1 {
343-
return 0, "", "", fmt.Errorf("could not find correct OVS flows for port %d", ofport)
349+
return 0, nil, fmt.Errorf("could not find correct OVS flows for port %d", ofport)
344350
}
345351

346352
parsed, err := ovs.ParseFlow(ovs.ParseForDump, flows[0])
347353
if err != nil {
348-
return 0, "", "", err
354+
return 0, nil, err
349355
}
350356

351-
macField, macOk := parsed.FindField("arp_sha")
352357
ipField, ipOk := parsed.FindField("arp_spa")
353-
if !macOk || !ipOk {
354-
return 0, "", "", fmt.Errorf("failed to parse OVS flows for sandbox ID %q", sandboxID)
355-
}
356-
357-
if _, err := net.ParseMAC(macField.Value); err != nil {
358-
return 0, "", "", fmt.Errorf("failed to parse arp_sha %q: %v", macField.Value, err)
358+
if !ipOk {
359+
return 0, nil, fmt.Errorf("failed to parse OVS flows for sandbox ID %q", sandboxID)
359360
}
360-
podMAC := macField.Value
361-
if net.ParseIP(ipField.Value) == nil {
362-
return 0, "", "", fmt.Errorf("failed to parse arp_spa %q", ipField.Value)
361+
podIP := net.ParseIP(ipField.Value)
362+
if podIP == nil {
363+
return 0, nil, fmt.Errorf("failed to parse arp_spa %q", ipField.Value)
363364
}
364-
podIP := ipField.Value
365365

366-
return ofport, podIP, podMAC, nil
366+
return ofport, podIP, nil
367367
}
368368

369369
func (oc *ovsController) UpdatePod(sandboxID string, vnid uint32) error {
370-
ofport, podIP, podMAC, err := oc.getPodDetailsBySandboxID(sandboxID)
370+
ofport, podIP, err := oc.getPodDetailsBySandboxID(sandboxID)
371371
if err != nil {
372372
return err
373373
}
374374
err = oc.cleanupPodFlows(podIP)
375375
if err != nil {
376376
return err
377377
}
378-
return oc.setupPodFlows(ofport, podIP, podMAC, vnid)
378+
return oc.setupPodFlows(ofport, podIP, vnid)
379379
}
380380

381381
func (oc *ovsController) TearDownPod(sandboxID string) error {
382-
_, podIP, _, err := oc.getPodDetailsBySandboxID(sandboxID)
382+
_, podIP, err := oc.getPodDetailsBySandboxID(sandboxID)
383383
if err != nil {
384384
// OVS flows related to sandboxID not found
385385
// Nothing needs to be done in that case

‎pkg/network/node/ovscontroller_test.go‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package node
44

55
import (
66
"fmt"
7+
"net"
78
"reflect"
89
"sort"
910
"strings"
@@ -14,6 +15,8 @@ import (
1415

1516
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1617
kapi "k8s.io/kubernetes/pkg/apis/core"
18+
19+
"github.com/containernetworking/plugins/pkg/utils/hwaddr"
1720
)
1821

1922
func setupOVSController(t *testing.T) (ovs.Interface, *ovsController, []string) {
@@ -225,7 +228,7 @@ func TestOVSPod(t *testing.T) {
225228
ovsif, oc, origFlows := setupOVSController(t)
226229

227230
// Add
228-
ofport, err := oc.SetUpPod("veth1", "10.128.0.2", "11:22:33:44:55:66", sandboxID, 42)
231+
ofport, err := oc.SetUpPod(sandboxID, "veth1", net.ParseIP("10.128.0.2"), 42)
229232
if err != nil {
230233
t.Fatalf("Unexpected error adding pod rules: %v", err)
231234
}
@@ -237,7 +240,7 @@ func TestOVSPod(t *testing.T) {
237240
err = assertFlowChanges(origFlows, flows,
238241
flowChange{
239242
kind: flowAdded,
240-
match: []string{"table=20", fmt.Sprintf("in_port=%d", ofport), "arp", "10.128.0.2", "11:22:33:44:55:66"},
243+
match: []string{"table=20", fmt.Sprintf("in_port=%d", ofport), "arp", "10.128.0.2", "00:00:0a:80:00:02/00:00:ff:ff:ff:ff"},
241244
},
242245
flowChange{
243246
kind: flowAdded,
@@ -275,7 +278,7 @@ func TestOVSPod(t *testing.T) {
275278
err = assertFlowChanges(origFlows, flows,
276279
flowChange{
277280
kind: flowAdded,
278-
match: []string{"table=20", fmt.Sprintf("in_port=%d", ofport), "arp", "10.128.0.2", "11:22:33:44:55:66"},
281+
match: []string{"table=20", fmt.Sprintf("in_port=%d", ofport), "arp", "10.128.0.2", "00:00:0a:80:00:02/00:00:ff:ff:ff:ff"},
279282
},
280283
flowChange{
281284
kind: flowAdded,
@@ -320,26 +323,24 @@ func TestGetPodDetails(t *testing.T) {
320323
type testcase struct {
321324
sandboxID string
322325
ip string
323-
mac string
324326
errStr string
325327
}
326328

327329
testcases := []testcase{
328330
{
329331
sandboxID: sandboxID,
330332
ip: "10.130.0.2",
331-
mac: "4a:77:32:e4:ab:9d",
332333
},
333334
}
334335

335336
for _, tc := range testcases {
336337
_, oc, _ := setupOVSController(t)
337-
tcOFPort, err := oc.SetUpPod("veth1", tc.ip, tc.mac, tc.sandboxID, 42)
338+
tcOFPort, err := oc.SetUpPod(tc.sandboxID, "veth1", net.ParseIP(tc.ip), 42)
338339
if err != nil {
339340
t.Fatalf("Unexpected error adding pod rules: %v", err)
340341
}
341342

342-
ofport, ip, mac, err := oc.getPodDetailsBySandboxID(tc.sandboxID)
343+
ofport, ip, err := oc.getPodDetailsBySandboxID(tc.sandboxID)
343344
if err != nil {
344345
if tc.errStr != "" {
345346
if !strings.Contains(err.Error(), tc.errStr) {
@@ -354,11 +355,8 @@ func TestGetPodDetails(t *testing.T) {
354355
if ofport != tcOFPort {
355356
t.Fatalf("unexpected ofport %d (expected %d)", ofport, tcOFPort)
356357
}
357-
if ip != tc.ip {
358-
t.Fatalf("unexpected ip %q (expected %q)", ip, tc.ip)
359-
}
360-
if mac != tc.mac {
361-
t.Fatalf("unexpected mac %q (expected %q)", mac, tc.mac)
358+
if ip.String() != tc.ip {
359+
t.Fatalf("unexpected ip %q (expected %q)", ip.String(), tc.ip)
362360
}
363361
}
364362
}
@@ -1050,3 +1048,16 @@ func TestSyncVNIDRules(t *testing.T) {
10501048
}
10511049
}
10521050
}
1051+
1052+
// Ensure that CNI's IP-addressed-based MAC addresses use the IP in the way we expect
1053+
func TestSetHWAddrByIP(t *testing.T) {
1054+
ip := net.ParseIP("1.2.3.4")
1055+
hwAddr, err := hwaddr.GenerateHardwareAddr4(ip, hwaddr.PrivateMACPrefix)
1056+
if err != nil {
1057+
t.Fatalf("unexpected error: %v", err)
1058+
}
1059+
expectedHWAddr := net.HardwareAddr(append(hwaddr.PrivateMACPrefix, ip.To4()...))
1060+
if !reflect.DeepEqual(hwAddr, expectedHWAddr) {
1061+
t.Fatalf("hwaddr.GenerateHardwareAddr4 changed behavior! (%#v != %#v)", hwAddr, expectedHWAddr)
1062+
}
1063+
}

‎pkg/network/node/pod.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -538,7 +538,7 @@ func (m *podManager) setup(req *cniserver.PodRequest) (cnitypes.Result, *running
538538
}
539539
}
540540

541-
var hostVethName, contVethMac string
541+
var hostVethName string
542542
err = ns.WithNetNSPath(req.Netns, func(hostNS ns.NetNS) error {
543543
hostVeth, contVeth, err := ip.SetupVeth(podInterfaceName, int(m.mtu), hostNS)
544544
if err != nil {
@@ -587,7 +587,6 @@ func (m *podManager) setup(req *cniserver.PodRequest) (cnitypes.Result, *running
587587
}
588588

589589
hostVethName = hostVeth.Name
590-
contVethMac = contVeth.HardwareAddr.String()
591590
return nil
592591
})
593592
if err != nil {
@@ -603,7 +602,7 @@ func (m *podManager) setup(req *cniserver.PodRequest) (cnitypes.Result, *running
603602
return nil, nil, err
604603
}
605604

606-
ofport, err := m.ovs.SetUpPod(hostVethName, podIP.String(), contVethMac, req.SandboxID, vnid)
605+
ofport, err := m.ovs.SetUpPod(req.SandboxID, hostVethName, podIP, vnid)
607606
if err != nil {
608607
return nil, nil, err
609608
}

0 commit comments

Comments
 (0)