Skip to content

Commit 2f29592

Browse files
danwinshipdanwinship
committed
Don't read back veth MAC; we already know it's based on the IP
1 parent ddb2b45 commit 2f29592

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
@@ -227,40 +227,46 @@ func (oc *ovsController) ensureOvsPort(hostVeth, sandboxID string) (int, error)
227227
return oc.ovs.AddPort(hostVeth, -1, "external-ids=sandbox="+sandboxID)
228228
}
229229

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

233+
ipstr := podIP.String()
234+
podIP = podIP.To4()
235+
ipmac := fmt.Sprintf("00:00:%02x:%02x:%02x:%02x/00:00:ff:ff:ff:ff", podIP[0], podIP[1], podIP[2], podIP[3])
236+
233237
// ARP/IP traffic from container
234-
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)
235-
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)
238+
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)
239+
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)
236240
if oc.useConnTrack {
237-
otx.AddFlow("table=25, priority=100, ip, nw_src=%s, actions=load:%d->NXM_NX_REG0[], goto_table:30", podIP, vnid)
241+
otx.AddFlow("table=25, priority=100, ip, nw_src=%s, actions=load:%d->NXM_NX_REG0[], goto_table:30", ipstr, vnid)
238242
}
239243

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

243247
// IP traffic to container
244-
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)
248+
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)
245249

246250
return otx.EndTransaction()
247251
}
248252

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

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

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

326-
func (oc *ovsController) getPodDetailsBySandboxID(sandboxID string) (int, string, string, error) {
332+
func (oc *ovsController) getPodDetailsBySandboxID(sandboxID string) (int, net.IP, error) {
327333
strports, err := oc.ovs.Find("interface", "ofport", "external-ids:sandbox="+sandboxID)
328334
if err != nil {
329-
return 0, "", "", err
335+
return 0, nil, err
330336
} else if len(strports) == 0 {
331-
return 0, "", "", fmt.Errorf("failed to find pod details from OVS flows")
337+
return 0, nil, fmt.Errorf("failed to find pod details from OVS flows")
332338
} else if len(strports) > 1 {
333-
return 0, "", "", fmt.Errorf("found multiple ofports for sandbox ID %q: %#v", sandboxID, strports)
339+
return 0, nil, fmt.Errorf("found multiple ofports for sandbox ID %q: %#v", sandboxID, strports)
334340
}
335341
ofport, err := strconv.Atoi(strports[0])
336342
if err != nil {
337-
return 0, "", "", fmt.Errorf("could not parse ofport %q: %v", strports[0], err)
343+
return 0, nil, fmt.Errorf("could not parse ofport %q: %v", strports[0], err)
338344
}
339345

340346
flows, err := oc.ovs.DumpFlows("table=20,arp,in_port=%d", ofport)
341347
if err != nil {
342-
return 0, "", "", err
348+
return 0, nil, err
343349
} else if len(flows) != 1 {
344-
return 0, "", "", fmt.Errorf("could not find correct OVS flows for port %d", ofport)
350+
return 0, nil, fmt.Errorf("could not find correct OVS flows for port %d", ofport)
345351
}
346352

347353
parsed, err := ovs.ParseFlow(ovs.ParseForDump, flows[0])
348354
if err != nil {
349-
return 0, "", "", err
355+
return 0, nil, err
350356
}
351357

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

367-
return ofport, podIP, podMAC, nil
367+
return ofport, podIP, nil
368368
}
369369

370370
func (oc *ovsController) UpdatePod(sandboxID string, vnid uint32) error {
371-
ofport, podIP, podMAC, err := oc.getPodDetailsBySandboxID(sandboxID)
371+
ofport, podIP, err := oc.getPodDetailsBySandboxID(sandboxID)
372372
if err != nil {
373373
return err
374374
}
375375
err = oc.cleanupPodFlows(podIP)
376376
if err != nil {
377377
return err
378378
}
379-
return oc.setupPodFlows(ofport, podIP, podMAC, vnid)
379+
return oc.setupPodFlows(ofport, podIP, vnid)
380380
}
381381

382382
func (oc *ovsController) TearDownPod(sandboxID string) error {
383-
_, podIP, _, err := oc.getPodDetailsBySandboxID(sandboxID)
383+
_, podIP, err := oc.getPodDetailsBySandboxID(sandboxID)
384384
if err != nil {
385385
// OVS flows related to sandboxID not found
386386
// 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
@@ -539,7 +539,7 @@ func (m *podManager) setup(req *cniserver.PodRequest) (cnitypes.Result, *running
539539
}
540540
}
541541

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

590590
hostVethName = hostVeth.Name
591-
contVethMac = contVeth.HardwareAddr.String()
592591
return nil
593592
})
594593
if err != nil {
@@ -604,7 +603,7 @@ func (m *podManager) setup(req *cniserver.PodRequest) (cnitypes.Result, *running
604603
return nil, nil, err
605604
}
606605

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

0 commit comments

Comments
 (0)