wgengine/router: fix Windows route sorting that caused de-dup to not work (#727)

Updates #725

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

wgengine/router/ifconfig_windows.go

@@ -315,15 +315,7 @@ func configureInterface(cfg *Config, tun *tun.NativeTun) error {
 		return err
 	}
 
-	sort.Slice(routes, func(i, j int) bool {
+	sort.Slice(routes, func(i, j int) bool { return routeLess(&routes[i], &routes[j]) })
-		return (bytes.Compare(routes[i].Destination.IP, routes[j].Destination.IP) == -1 ||
-			// Narrower masks first
-			bytes.Compare(routes[i].Destination.Mask, routes[j].Destination.Mask) == 1 ||
-			// No nexthop before non-empty nexthop
-			bytes.Compare(routes[i].NextHop, routes[j].NextHop) == -1 ||
-			// Lower metrics first
-			routes[i].Metric < routes[j].Metric)
-	})
 
 	deduplicatedRoutes := []*winipcfg.RouteData{}
 	for i := 0; i < len(routes); i++ {
@@ -387,3 +379,25 @@ func configureInterface(cfg *Config, tun *tun.NativeTun) error {
 
 	return errAcc
 }
+
+// routeLess reports whether ri should sort before rj.
+// The actual sort order doesn't appear to matter. The caller just
+// wants them sorted to be able to de-dup.
+func routeLess(ri, rj *winipcfg.RouteData) bool {
+	if v := bytes.Compare(ri.Destination.IP, rj.Destination.IP); v != 0 {
+		return v == -1
+	}
+	if v := bytes.Compare(ri.Destination.Mask, rj.Destination.Mask); v != 0 {
+		// Narrower masks first
+		return v == 1
+	}
+	if ri.Metric != rj.Metric {
+		// Lower metrics first
+		return ri.Metric < rj.Metric
+	}
+	if v := bytes.Compare(ri.NextHop, rj.NextHop); v != 0 {
+		// No nexthop before non-empty nexthop.
+		return v == -1
+	}
+	return false
+}

wgengine/router/ifconfig_windows_test.go

@@ -0,0 +1,97 @@
+// Copyright (c) 2020 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package router
+
+import (
+	"math/rand"
+	"net"
+	"testing"
+
+	winipcfg "github.com/tailscale/winipcfg-go"
+	"inet.af/netaddr"
+)
+
+func randIP() net.IP {
+	b := byte(rand.Intn(3))
+	return net.IP{b, b, b, b}
+}
+
+func randRouteData() *winipcfg.RouteData {
+	return &winipcfg.RouteData{
+		Destination: net.IPNet{
+			IP:   randIP(),
+			Mask: net.CIDRMask(rand.Intn(3)+1, 32),
+		},
+		NextHop: randIP(),
+		Metric:  uint32(rand.Intn(3)),
+	}
+}
+
+func TestRouteLess(t *testing.T) {
+	type D = winipcfg.RouteData
+	ipnet := func(s string) net.IPNet {
+		ipp, err := netaddr.ParseIPPrefix(s)
+		if err != nil {
+			t.Fatalf("error parsing test data %q: %v", s, err)
+		}
+		return *ipp.IPNet()
+	}
+
+	tests := []struct {
+		ri, rj *winipcfg.RouteData
+		want   bool
+	}{
+		{
+			ri:   &D{Metric: 1},
+			rj:   &D{Metric: 2},
+			want: true,
+		},
+		{
+			ri:   &D{Destination: ipnet("1.1.0.0/16"), Metric: 2},
+			rj:   &D{Destination: ipnet("2.2.0.0/16"), Metric: 1},
+			want: true,
+		},
+		{
+			ri:   &D{Destination: ipnet("1.1.0.0/16"), Metric: 1},
+			rj:   &D{Destination: ipnet("2.2.0.0/16"), Metric: 1},
+			want: true,
+		},
+		{
+			ri:   &D{Destination: ipnet("1.1.0.0/32"), Metric: 2},
+			rj:   &D{Destination: ipnet("1.1.0.0/16"), Metric: 1},
+			want: true,
+		},
+		{
+			ri:   &D{Destination: ipnet("1.1.0.0/32"), Metric: 1},
+			rj:   &D{Destination: ipnet("1.1.0.0/16"), Metric: 1},
+			want: true,
+		},
+		{
+			ri:   &D{Destination: ipnet("1.1.0.0/16"), Metric: 1, NextHop: net.ParseIP("3.3.3.3")},
+			rj:   &D{Destination: ipnet("1.1.0.0/16"), Metric: 1, NextHop: net.ParseIP("4.4.4.4")},
+			want: true,
+		},
+	}
+	for i, tt := range tests {
+		got := routeLess(tt.ri, tt.rj)
+		if got != tt.want {
+			t.Errorf("%v. less = %v; want %v", i, got, tt.want)
+		}
+		back := routeLess(tt.rj, tt.ri)
+		if back && got {
+			t.Errorf("%v. less both ways", i)
+		}
+	}
+}
+
+func TestRouteLessConsistent(t *testing.T) {
+	for i := 0; i < 10000; i++ {
+		ri := randRouteData()
+		rj := randRouteData()
+		if routeLess(ri, rj) && routeLess(rj, ri) {
+			t.Fatalf("both compare less to each other:\n\t%#v\nand\n\t%#v", ri, rj)
+		}
+	}
+}