Skip to content

Commit

Permalink
Fix handling of dhcp classless routes
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Ivan Shvedunov committed Oct 16, 2018
1 parent 4d925c7 commit 57e4f6b
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 84 deletions.
26 changes: 20 additions & 6 deletions pkg/dhcp/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"bytes"
"errors"
"fmt"
"io"
"net"
"strings"

Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
185 changes: 117 additions & 68 deletions tests/network/dhcp_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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'",
Expand All @@ -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()
}
15 changes: 5 additions & 10 deletions tests/network/vm_network_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
})
Expand Down Expand Up @@ -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",
},
Expand Down Expand Up @@ -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",
},
Expand Down Expand Up @@ -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",
},
Expand Down Expand Up @@ -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",
},
Expand Down

0 comments on commit 57e4f6b

Please sign in to comment.