Skip to content

Commit

Permalink
Fix tap MTU setting
Browse files Browse the repository at this point in the history 
MTU for the tap interface was not being set properly, causing also
wrong MTU for the bridge. The problem was observed e.g. when using
Flannel on a cluster running on top of OpenStack.
  • Loading branch information
Ivan Shvedunov committed Jan 22, 2019
1 parent 14f6027 commit 0e7fbfc
Show file tree
Hide file tree
Showing 7 changed files with 155 additions and 28 deletions.
4 changes: 2 additions & 2 deletions pkg/nettools/calico_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func TestCalicoDetection(t *testing.T) {
},
} {
t.Run(tc.name, func(t *testing.T) {
withFakeCNIVeth(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
withFakeCNIVeth(t, defaultMTU, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
for _, r := range tc.routes {
if r.Scope == SCOPE_LINK {
r.LinkIndex = origContVeth.Attrs().Index
Expand All @@ -148,7 +148,7 @@ func TestCalicoDetection(t *testing.T) {

func TestFixCalicoNetworking(t *testing.T) {
withDummyNetworkNamespace(t, func(dummyNS ns.NetNS, dummyInfo *cnicurrent.Result) {
withFakeCNIVeth(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
withFakeCNIVeth(t, defaultMTU, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
addCalicoRoutes(t, origContVeth)
info, err := ExtractLinkInfo(origContVeth, contNS.Path())
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions pkg/nettools/nettools.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import (
)

const (
defaultMTU = 1500
tapInterfaceNameTemplate = "tap%d"
containerBridgeNameTemplate = "br%d"
loopbackInterfaceName = "lo"
Expand Down
47 changes: 33 additions & 14 deletions pkg/nettools/nettools_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,9 +259,9 @@ func setupLink(hwAddrAsText string, link netlink.Link) netlink.Link {
return link
}

func withFakeCNIVeth(t *testing.T, toRun func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link)) {
func withFakeCNIVeth(t *testing.T, mtu int, toRun func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link)) {
withHostAndContNS(t, func(hostNS, contNS ns.NetNS) {
origHostVeth, origContVeth, err := CreateEscapeVethPair(contNS, "eth0", 1500)
origHostVeth, origContVeth, err := CreateEscapeVethPair(contNS, "eth0", mtu)
if err != nil {
log.Panicf("failed to create veth pair: %v", err)
}
Expand All @@ -282,8 +282,8 @@ func withFakeCNIVeth(t *testing.T, toRun func(hostNS, contNS ns.NetNS, origHostV
})
}

func withFakeCNIVethAndGateway(t *testing.T, toRun func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link)) {
withFakeCNIVeth(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
func withFakeCNIVethAndGateway(t *testing.T, mtu int, toRun func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link)) {
withFakeCNIVeth(t, mtu, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
addTestRoute(t, &netlink.Route{
Gw: parseAddr("10.1.90.1/24").IPNet.IP,
Scope: SCOPE_UNIVERSE,
Expand All @@ -308,7 +308,7 @@ func verifyNoAddressAndRoutes(t *testing.T, link netlink.Link) {
}

func TestFindVeth(t *testing.T) {
withFakeCNIVethAndGateway(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
withFakeCNIVethAndGateway(t, defaultMTU, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
allLinks, err := netlink.LinkList()
if err != nil {
log.Panicf("LinkList() failed: %v", err)
Expand All @@ -325,7 +325,7 @@ func TestFindVeth(t *testing.T) {
}

func TestStripLink(t *testing.T) {
withFakeCNIVethAndGateway(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
withFakeCNIVethAndGateway(t, defaultMTU, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
if err := StripLink(origContVeth); err != nil {
log.Panicf("StripLink() failed: %v", err)
}
Expand All @@ -334,7 +334,7 @@ func TestStripLink(t *testing.T) {
}

func TestExtractLinkInfo(t *testing.T) {
withFakeCNIVethAndGateway(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
withFakeCNIVethAndGateway(t, defaultMTU, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
info, err := ExtractLinkInfo(origContVeth, contNS.Path())
if err != nil {
log.Panicf("failed to grab interface info: %v", err)
Expand All @@ -347,7 +347,7 @@ func TestExtractLinkInfo(t *testing.T) {
})
}

func verifyContainerSideNetwork(t *testing.T, origContVeth netlink.Link, contNsPath string, hostNS ns.NetNS) {
func verifyContainerSideNetwork(t *testing.T, origContVeth netlink.Link, contNsPath string, hostNS ns.NetNS, mtu int) {
allLinks, err := netlink.LinkList()
if err != nil {
log.Panicf("error listing links: %v", err)
Expand Down Expand Up @@ -375,6 +375,9 @@ func verifyContainerSideNetwork(t *testing.T, origContVeth netlink.Link, contNsP
if reflect.DeepEqual(origContVeth.Attrs().HardwareAddr, origHwAddr) {
t.Errorf("cni veth hardware address didn't change")
}
if origContVeth.Attrs().MTU != mtu {
t.Errorf("bad veth MTU: %d instead of %d", origContVeth.Attrs().MTU, mtu)
}

verifyNoAddressAndRoutes(t, origContVeth)

Expand All @@ -384,6 +387,9 @@ func verifyContainerSideNetwork(t *testing.T, origContVeth netlink.Link, contNsP
if tap.Type() != "tun" {
t.Errorf("tap0 interface must have type tun, but has %q instead", tap.Type())
}
if tap.Attrs().MTU != mtu {
t.Errorf("bad tap MTU: %d instead of %d", tap.Attrs().MTU, mtu)
}

addrs, err := netlink.AddrList(bridge, FAMILY_V4)
if err != nil {
Expand All @@ -395,20 +401,33 @@ func verifyContainerSideNetwork(t *testing.T, origContVeth netlink.Link, contNsP
} else if addrs[0].String() != expectedAddr {
t.Errorf("bad br0 address %q (expected %q)", addrs[0].String(), expectedAddr)
}

if bridge.Attrs().MTU != mtu {
t.Errorf("bad bridge MTU: %d instead of %d", bridge.Attrs().MTU, mtu)
}
}

func TestSetUpContainerSideNetworkWithInfo(t *testing.T) {
withFakeCNIVethAndGateway(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
withFakeCNIVethAndGateway(t, defaultMTU, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
if err := StripLink(origContVeth); err != nil {
log.Panicf("StripLink() failed: %v", err)
}
verifyContainerSideNetwork(t, origContVeth, contNS.Path(), hostNS, defaultMTU)
})
}

func TestSetUpContainerSideNetworkMTU(t *testing.T) {
withFakeCNIVethAndGateway(t, 9000, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
if err := StripLink(origContVeth); err != nil {
log.Panicf("StripLink() failed: %v", err)
}
verifyContainerSideNetwork(t, origContVeth, contNS.Path(), hostNS)
verifyContainerSideNetwork(t, origContVeth, contNS.Path(), hostNS, 9000)
})
}

func TestLoopbackInterface(t *testing.T) {
withFakeCNIVethAndGateway(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
verifyContainerSideNetwork(t, origContVeth, contNS.Path(), hostNS)
withFakeCNIVethAndGateway(t, defaultMTU, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
verifyContainerSideNetwork(t, origContVeth, contNS.Path(), hostNS, defaultMTU)
if out, err := exec.Command("ping", "-c", "1", "127.0.0.1").CombinedOutput(); err != nil {
log.Panicf("ping 127.0.0.1 failed:\n%s", out)
}
Expand Down Expand Up @@ -483,7 +502,7 @@ func verifyVethHaveConfiguration(t *testing.T, info *cnicurrent.Result) {
}

func TestTeardownContainerSideNetwork(t *testing.T) {
withFakeCNIVethAndGateway(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
withFakeCNIVethAndGateway(t, defaultMTU, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
if err := StripLink(origContVeth); err != nil {
log.Panicf("StripLink() failed: %v", err)
}
Expand Down Expand Up @@ -516,7 +535,7 @@ func TestTeardownContainerSideNetwork(t *testing.T) {
}

func TestFindingLinkByAddress(t *testing.T) {
withFakeCNIVeth(t, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
withFakeCNIVeth(t, defaultMTU, func(hostNS, contNS ns.NetNS, origHostVeth, origContVeth netlink.Link) {
expectedInfo := expectedExtractedLinkInfo(contNS.Path())
allLinks, err := netlink.LinkList()
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions pkg/nettools/tap_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,5 +83,10 @@ func CreateTAP(devName string, mtu int) (netlink.Link, error) {
return nil, fmt.Errorf("failed to set %q up: %v", devName, err)
}

// NOTE: link mtu in LinkAttrs above is actually ignored
if err := netlink.LinkSetMTU(tap, mtu); err != nil {
return nil, fmt.Errorf("LinkSetMTU(): %v", err)
}

return tap, nil
}
15 changes: 12 additions & 3 deletions tests/network/fake_cni_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ import (
"github.com/Mirantis/virtlet/pkg/utils"
)

const (
defaultMTU = 1500
)

// FakeCNIVethPair represents a veth pair created by the fake CNI
type FakeCNIVethPair struct {
HostSide netlink.Link
Expand All @@ -46,6 +50,7 @@ type fakeCNIEntry struct {
added bool
removed bool
useBadResult bool
mtu int
}

func (e *fakeCNIEntry) addSandboxToNetwork(ifaceIndex int) error {
Expand All @@ -61,7 +66,7 @@ func (e *fakeCNIEntry) addSandboxToNetwork(ifaceIndex int) error {
var vp FakeCNIVethPair
if err := e.hostNS.Do(func(ns.NetNS) error {
var err error
vp.HostSide, vp.ContSide, err = nettools.CreateEscapeVethPair(e.contNS, iface.Name, 1500)
vp.HostSide, vp.ContSide, err = nettools.CreateEscapeVethPair(e.contNS, iface.Name, e.mtu)
return err
}); err != nil {
return fmt.Errorf("failed to create escape veth pair: %v", err)
Expand Down Expand Up @@ -161,19 +166,23 @@ func NewFakeCNIClient() *FakeCNIClient {
}
}

func (c *FakeCNIClient) ExpectPod(podId, podName, podNS string, info *cnicurrent.Result, hostNS ns.NetNS, extraRoutes map[int][]netlink.Route) {
func (c *FakeCNIClient) ExpectPod(podId, podName, podNS string, info *cnicurrent.Result, hostNS ns.NetNS, extraRoutes map[int][]netlink.Route, mtu int) {
if mtu == 0 {
mtu = defaultMTU
}
c.entries[podKey(podId, podName, podNS)] = &fakeCNIEntry{
podId: podId,
podName: podName,
podNS: podNS,
info: info,
hostNS: hostNS,
extraRoutes: extraRoutes,
mtu: mtu,
}
}

func (c *FakeCNIClient) ExpectDummyPod(info *cnicurrent.Result, hostNS ns.NetNS, extraRoutes map[int][]netlink.Route) {
c.ExpectPod(c.DummyPodId, "", "", info, hostNS, extraRoutes)
c.ExpectPod(c.DummyPodId, "", "", info, hostNS, extraRoutes, 0)
}

func (c *FakeCNIClient) GetDummyNetwork() (*cnicurrent.Result, string, error) {
Expand Down
54 changes: 54 additions & 0 deletions tests/network/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,60 @@ func addAddress(t *testing.T, netNS ns.NetNS, link netlink.Link, addr string) {
}
}

func getBridgeFromMember(t *testing.T, netNS ns.NetNS, link netlink.Link) netlink.Link {
var bridge netlink.Link
if err := netNS.Do(func(ns.NetNS) (err error) {
link, err = netlink.LinkByName(link.Attrs().Name)
if err != nil {
return fmt.Errorf("LinkByName(): %v", err)
}
masterIndex := link.Attrs().MasterIndex
if masterIndex == 0 {
return fmt.Errorf("link %q doesn't have master", link.Attrs().Name)
}
bridge, err = netlink.LinkByIndex(masterIndex)
if err != nil {
return fmt.Errorf("LinkByIndex(): %v", err)
}
return nil
}); err != nil {
t.Fatalf("getBridgeFromMember(): %v", err)
}
return bridge
}

func verifyMTU(t *testing.T, netNS ns.NetNS, link netlink.Link, expectedMTU int) {
if link.Type() != "bridge" {
link = getBridgeFromMember(t, netNS, link)
}
if err := netNS.Do(func(ns.NetNS) (err error) {
link, err = netlink.LinkByName(link.Attrs().Name)
if err != nil {
return fmt.Errorf("LinkByName(): %v", err)
}
if link.Type() != "bridge" {
return fmt.Errorf("doVerifyMTU(): %q: bridge expected", link.Attrs().Name)
}
index := link.Attrs().Index
links, err := netlink.LinkList()
if err != nil {
return fmt.Errorf("LinkList(): %v", err)
}
for _, link := range links {
attrs := link.Attrs()
if attrs.MasterIndex != index {
continue
}
if attrs.MTU != expectedMTU {
t.Errorf("bad MTU for link %q: %d instead of %d", attrs.Name, attrs.MTU, expectedMTU)
}
}
return nil
}); err != nil {
t.Fatalf("verifying mtu failed: %v", err)
}
}

// tapConnector copies frames between tap interfaces. It returns
// a channel that should be closed to stop copying and close
// the tap devices
Expand Down
57 changes: 48 additions & 9 deletions tests/network/vm_network_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,12 @@ func sampleCNIResult() *cnicurrent.Result {
}

type vmNetworkTester struct {
t *testing.T
linkCount int
hostNS, contNS, clientNS ns.NetNS
clientTapLinks []netlink.Link
dhcpClientTaps []*os.File
g *NetTestGroup
t *testing.T
linkCount int
hostNS, clientNS ns.NetNS
clientTapLinks []netlink.Link
dhcpClientTaps []*os.File
g *NetTestGroup
}

func newVMNetworkTester(t *testing.T, linkCount int) *vmNetworkTester {
Expand Down Expand Up @@ -291,9 +291,9 @@ type tapFDSourceTester struct {
c *tapmanager.FDClient
}

func newTapFDSourceTester(t *testing.T, podId string, info *cnicurrent.Result, hostNS ns.NetNS, extraRoutes map[int][]netlink.Route) *tapFDSourceTester {
func newTapFDSourceTester(t *testing.T, podId string, info *cnicurrent.Result, hostNS ns.NetNS, extraRoutes map[int][]netlink.Route, mtu int) *tapFDSourceTester {
cniClient := NewFakeCNIClient()
cniClient.ExpectPod(podId, samplePodName, samplePodNS, info, hostNS, extraRoutes)
cniClient.ExpectPod(podId, samplePodName, samplePodNS, info, hostNS, extraRoutes, mtu)

tmpDir, err := ioutil.TempDir("", "pass-fd-test")
if err != nil {
Expand Down Expand Up @@ -415,6 +415,8 @@ func TestTapFDSource(t *testing.T) {
outerAddrs []string
// clientAddrs specifies per-interface VM IPs to ping
clientAddrs []string
// mtu specifies MTU for the host interface
mtu int
}{
{
name: "single cni",
Expand All @@ -428,6 +430,7 @@ func TestTapFDSource(t *testing.T) {
"new_network_number='10.1.90.0'",
"new_subnet_mask='255.255.255.0'",
"tap0: offered 10.1.90.5 from 169.254.254.2",
"new_interface_mtu='1500'",
},
},
interfaceDesc: []tapmanager.InterfaceDescription{
Expand All @@ -441,6 +444,33 @@ func TestTapFDSource(t *testing.T) {
outerAddrs: outerAddrs,
clientAddrs: clientAddrs,
},
{
name: "single cni with MTU",
interfaceCount: 1,
info: sampleCNIResult(),
tcpdumpStopOn: "10.1.90.1.4243 > 10.1.90.5.4242: UDP",
dhcpExpectedSubstrings: [][]string{
{
"new_classless_static_routes='10.10.42.0/24 10.1.90.90 0.0.0.0/0 10.1.90.1'",
"new_ip_address='10.1.90.5'",
"new_network_number='10.1.90.0'",
"new_subnet_mask='255.255.255.0'",
"tap0: offered 10.1.90.5 from 169.254.254.2",
"new_interface_mtu='9000'",
},
},
interfaceDesc: []tapmanager.InterfaceDescription{
{
Type: network.InterfaceTypeTap,
HardwareAddr: mustParseMAC(clientMacAddrs[0]),
FdIndex: 0,
PCIAddress: "",
},
},
outerAddrs: outerAddrs,
clientAddrs: clientAddrs,
mtu: 9000,
},
{
name: "multiple cnis",
interfaceCount: 2,
Expand Down Expand Up @@ -949,7 +979,7 @@ func TestTapFDSource(t *testing.T) {
vnt := newVMNetworkTester(t, tc.interfaceCount)
defer vnt.teardown()

tst := newTapFDSourceTester(t, podId, tc.info, vnt.hostNS, tc.extraRoutes)
tst := newTapFDSourceTester(t, podId, tc.info, vnt.hostNS, tc.extraRoutes, tc.mtu)
defer tst.teardown()
c := tst.setupServerAndConnectToFDServer()
if tc.dummyInfo != nil {
Expand Down Expand Up @@ -1048,7 +1078,15 @@ func TestTapFDSource(t *testing.T) {
verifyNoDiff(t, "interfaceDesc", tc.interfaceDesc, interfaceDesc)
}

contNS, err := ns.GetNS(csn.NsPath)
if err != nil {
t.Fatalf("GetNS(): %v", err)
}

for n, veth := range veths {
if tc.mtu != 0 {
verifyMTU(t, contNS, veth.ContSide, tc.mtu)
}
addAddress(t, vnt.hostNS, veth.HostSide, tc.outerAddrs[n])
}

Expand Down Expand Up @@ -1079,3 +1117,4 @@ func TestTapFDSource(t *testing.T) {

// TODO: test DNS handling
// TODO: test SR-IOV (by making a fake sysfs dir)
// TODO: mtu option over DHCP

0 comments on commit 0e7fbfc

Please sign in to comment.