From 57e4f6bd6eb92b7d66c219140caf669bb35ecfbd Mon Sep 17 00:00:00 2001 From: Ivan Shvedunov Date: Mon, 15 Oct 2018 18:05:49 +0300 Subject: [PATCH] Fix handling of dhcp classless routes When there are any classless routes passed from the DHCP server, some DHCP client implementations ignore the router setting. In order to work around this issue, we use the classless routes option instead of the router option in case if there are any options except for the default one. --- pkg/dhcp/server.go | 26 ++++- tests/network/dhcp_test.go | 185 +++++++++++++++++++------------ tests/network/vm_network_test.go | 15 +-- 3 files changed, 142 insertions(+), 84 deletions(-) diff --git a/pkg/dhcp/server.go b/pkg/dhcp/server.go index 5a7fcdba7..b285b5c9a 100644 --- a/pkg/dhcp/server.go +++ b/pkg/dhcp/server.go @@ -23,6 +23,7 @@ import ( "bytes" "errors" "fmt" + "io" "net" "strings" @@ -304,14 +305,18 @@ func (s *Server) getStaticRoutes(ip net.IPNet) (router, routes []byte, err error continue } } - b.Write(toDestinationDescriptor(route.Dst)) - if gw != nil { - b.Write(gw) - } else { - b.Write([]byte{0, 0, 0, 0}) - } + writeRoute(&b, route.Dst, gw) } + // XXX: classless routes apparently cause a problem: + // https://askubuntu.com/questions/767002/dhcp-connection-does-not-set-default-gateway-automatically + if b.Len() != 0 && router != nil { + writeRoute(&b, net.IPNet{ + IP: net.IPv4zero, + Mask: net.IPv4Mask(0, 0, 0, 0), + }, router) + router = nil + } routes = b.Bytes() return } @@ -327,6 +332,15 @@ func toDestinationDescriptor(network net.IPNet) []byte { ) } +func writeRoute(w io.Writer, dst net.IPNet, gw net.IP) { + w.Write(toDestinationDescriptor(dst)) + if gw != nil { + w.Write(gw) + } else { + w.Write([]byte{0, 0, 0, 0}) + } +} + func widthOfMaskToSignificantOctets(mask int) int { return (mask + 7) / 8 } diff --git a/tests/network/dhcp_test.go b/tests/network/dhcp_test.go index 9eca156ec..f82698a25 100644 --- a/tests/network/dhcp_test.go +++ b/tests/network/dhcp_test.go @@ -31,15 +31,15 @@ import ( "github.com/Mirantis/virtlet/pkg/network" ) -type dhcpTestCase struct { - csn network.ContainerSideNetwork - expectedSubstrings []string -} - func TestDhcpServer(t *testing.T) { clientMac, _ := net.ParseMAC(clientMacAddrs[0]) - testCases := []*dhcpTestCase{ + for _, tc := range []struct { + name string + csn network.ContainerSideNetwork + expectedSubstrings []string + }{ { + name: "with classless routes", csn: network.ContainerSideNetwork{ Result: &cnicurrent.Result{ Interfaces: []*cnicurrent.Interface{ @@ -87,7 +87,62 @@ func TestDhcpServer(t *testing.T) { }, expectedSubstrings: []string{ "new_broadcast_address='10.1.90.255'", - "new_classless_static_routes='10.10.42.0/24 10.1.90.90'", + "new_classless_static_routes='10.10.42.0/24 10.1.90.90 0.0.0.0/0 10.1.90.1'", + "new_dhcp_lease_time='86400'", + "new_dhcp_rebinding_time='64800'", + "new_dhcp_renewal_time='43200'", + "new_dhcp_server_identifier='169.254.254.2'", + "new_domain_name_servers='8.8.8.8'", + "new_ip_address='10.1.90.5'", + "new_interface_mtu='9000'", + "new_network_number='10.1.90.0'", + "new_subnet_cidr='24'", + "new_subnet_mask='255.255.255.0'", + "veth0: offered 10.1.90.5 from 169.254.254.2", + }, + }, + { + name: "without classless routes", + csn: network.ContainerSideNetwork{ + Result: &cnicurrent.Result{ + Interfaces: []*cnicurrent.Interface{ + { + Name: "eth0", + Mac: clientMacAddrs[0], + // Sandbox is clientNS dependent + // so it must be set in runtime + }, + }, + IPs: []*cnicurrent.IPConfig{ + { + Version: "4", + Interface: 0, + Address: net.IPNet{ + IP: net.IP{10, 1, 90, 5}, + Mask: net.IPMask{255, 255, 255, 0}, + }, + Gateway: net.IP{10, 1, 90, 1}, + }, + }, + Routes: []*cnitypes.Route{ + { + Dst: net.IPNet{ + IP: net.IP{0, 0, 0, 0}, + Mask: net.IPMask{0, 0, 0, 0}, + }, + GW: net.IP{10, 1, 90, 1}, + }, + }, + }, + Interfaces: []*network.InterfaceDescription{ + { + HardwareAddr: clientMac, + MTU: 9000, + }, + }, + }, + expectedSubstrings: []string{ + "new_broadcast_address='10.1.90.255'", "new_dhcp_lease_time='86400'", "new_dhcp_rebinding_time='64800'", "new_dhcp_renewal_time='43200'", @@ -103,66 +158,60 @@ func TestDhcpServer(t *testing.T) { }, }, // TODO: add dns test case here + } { + t.Run(tc.name, func(t *testing.T) { + serverNS, err := ns.NewNS() + if err != nil { + t.Fatalf("Failed to create ns for dhcp server: %v", err) + } + defer serverNS.Close() + clientNS, err := ns.NewNS() + if err != nil { + t.Fatalf("Failed to create ns for dhcp client: %v", err) + } + defer clientNS.Close() + + // Sandbox is clientNS dependent so it needs to be set there on all interfaces + for _, iface := range tc.csn.Result.Interfaces { + iface.Sandbox = clientNS.Path() + } + + var clientVeth, serverVeth netlink.Link + if err := serverNS.Do(func(ns.NetNS) error { + serverVeth, clientVeth, err = nettools.CreateEscapeVethPair(clientNS, "veth0", 1500) + if err != nil { + return fmt.Errorf("failed to create escape veth pair: %v", err) + } + addr, err := netlink.ParseAddr("169.254.254.2/24") + if err != nil { + return fmt.Errorf("failed to parse dhcp interface address: %v", err) + } + if err = netlink.AddrAdd(serverVeth, addr); err != nil { + return fmt.Errorf("failed to add addr for serverVeth: %v", err) + } + + return nil + }); err != nil { + t.Fatal(err) + } + + if err := clientNS.Do(func(ns.NetNS) error { + mac, _ := net.ParseMAC(clientMacAddrs[0]) + if err = nettools.SetHardwareAddr(clientVeth, mac); err != nil { + return fmt.Errorf("can't set MAC address on the client interface: %v", err) + } + + return nil + }); err != nil { + t.Fatal(err) + } + + g := NewNetTestGroup(t, 15*time.Second) + defer g.Stop() + g.Add(serverNS, NewDhcpServerTester(&tc.csn)) + + g.Add(clientNS, NewDhcpClient("veth0", tc.expectedSubstrings)) + g.Wait() + }) } - - for _, testCase := range testCases { - // TODO: use subtests https://golang.org/pkg/testing/#hdr-Subtests_and_Sub_benchmarks - // (need newer Go) - runDhcpTestCase(t, testCase) - } -} - -func runDhcpTestCase(t *testing.T, testCase *dhcpTestCase) { - serverNS, err := ns.NewNS() - if err != nil { - t.Fatalf("Failed to create ns for dhcp server: %v", err) - } - defer serverNS.Close() - clientNS, err := ns.NewNS() - if err != nil { - t.Fatalf("Failed to create ns for dhcp client: %v", err) - } - defer clientNS.Close() - - // Sandbox is clientNS dependent so it needs to be set there on all interfaces - for _, iface := range testCase.csn.Result.Interfaces { - iface.Sandbox = clientNS.Path() - } - - var clientVeth, serverVeth netlink.Link - if err := serverNS.Do(func(ns.NetNS) error { - serverVeth, clientVeth, err = nettools.CreateEscapeVethPair(clientNS, "veth0", 1500) - if err != nil { - return fmt.Errorf("failed to create escape veth pair: %v", err) - } - addr, err := netlink.ParseAddr("169.254.254.2/24") - if err != nil { - return fmt.Errorf("failed to parse dhcp interface address: %v", err) - } - if err = netlink.AddrAdd(serverVeth, addr); err != nil { - return fmt.Errorf("failed to add addr for serverVeth: %v", err) - } - - return nil - }); err != nil { - t.Fatal(err) - } - - if err := clientNS.Do(func(ns.NetNS) error { - mac, _ := net.ParseMAC(clientMacAddrs[0]) - if err = nettools.SetHardwareAddr(clientVeth, mac); err != nil { - return fmt.Errorf("can't set MAC address on the client interface: %v", err) - } - - return nil - }); err != nil { - t.Fatal(err) - } - - g := NewNetTestGroup(t, 15*time.Second) - defer g.Stop() - g.Add(serverNS, NewDhcpServerTester(&testCase.csn)) - - g.Add(clientNS, NewDhcpClient("veth0", testCase.expectedSubstrings)) - g.Wait() } diff --git a/tests/network/vm_network_test.go b/tests/network/vm_network_test.go index ab687bae4..75369d673 100644 --- a/tests/network/vm_network_test.go +++ b/tests/network/vm_network_test.go @@ -271,10 +271,9 @@ func TestVmNetwork(t *testing.T) { vnt.addTcpdump(hostVeth, "10.1.90.1.4243 > 10.1.90.5.4242: UDP", "BOOTP/DHCP") vnt.g.Add(contNS, NewDhcpServerTester(csn)) vnt.verifyDhcp("tap0", []string{ - "new_classless_static_routes='10.10.42.0/24 10.1.90.90'", + "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_routers='10.1.90.1'", "new_subnet_mask='255.255.255.0'", "tap0: offered 10.1.90.5 from 169.254.254.2", }) @@ -424,10 +423,9 @@ func TestTapFDSource(t *testing.T) { 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'", + "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_routers='10.1.90.1'", "new_subnet_mask='255.255.255.0'", "tap0: offered 10.1.90.5 from 169.254.254.2", }, @@ -498,10 +496,9 @@ func TestTapFDSource(t *testing.T) { 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'", + "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_routers='10.1.90.1'", "new_subnet_mask='255.255.255.0'", "tap0: offered 10.1.90.5 from 169.254.254.2", }, @@ -633,10 +630,9 @@ func TestTapFDSource(t *testing.T) { 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'", + "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_routers='10.1.90.1'", "new_subnet_mask='255.255.255.0'", "tap0: offered 10.1.90.5 from 169.254.254.2", }, @@ -671,10 +667,9 @@ func TestTapFDSource(t *testing.T) { 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'", + "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_routers='10.1.90.1'", "new_subnet_mask='255.255.255.0'", "tap0: offered 10.1.90.5 from 169.254.254.2", },