Remove add gateway logic

Author: lixmal

client/internal/routemanager/systemops/systemops_generic.go

@@ -106,47 +106,6 @@ func (r *SysOps) cleanupRefCounter(stateManager *statemanager.Manager) error {
 	return nil
 }
 
-// TODO: fix: for default our wg address now appears as the default gw
-func (r *SysOps) addRouteForCurrentDefaultGateway(prefix netip.Prefix) error {
-	addr := netip.IPv4Unspecified()
-	if prefix.Addr().Is6() {
-		addr = netip.IPv6Unspecified()
-	}
-
-	nexthop, err := GetNextHop(addr)
-	if err != nil && !errors.Is(err, vars.ErrRouteNotFound) {
-		return fmt.Errorf("get existing route gateway: %s", err)
-	}
-
-	if !prefix.Contains(nexthop.IP) {
-		log.Debugf("Skipping adding a new route for gateway %s because it is not in the network %s", nexthop.IP, prefix)
-		return nil
-	}
-
-	gatewayPrefix := netip.PrefixFrom(nexthop.IP, 32)
-	if nexthop.IP.Is6() {
-		gatewayPrefix = netip.PrefixFrom(nexthop.IP, 128)
-	}
-
-	ok, err := existsInRouteTable(gatewayPrefix)
-	if err != nil {
-		return fmt.Errorf("unable to check if there is an existing route for gateway %s. error: %s", gatewayPrefix, err)
-	}
-
-	if ok {
-		log.Debugf("Skipping adding a new route for gateway %s because it already exists", gatewayPrefix)
-		return nil
-	}
-
-	nexthop, err = GetNextHop(nexthop.IP)
-	if err != nil && !errors.Is(err, vars.ErrRouteNotFound) {
-		return fmt.Errorf("unable to get the next hop for the default gateway address. error: %s", err)
-	}
-
-	log.Debugf("Adding a new route for gateway %s with next hop %s", gatewayPrefix, nexthop.IP)
-	return r.addToRouteTable(gatewayPrefix, nexthop)
-}
-
 // addRouteToNonVPNIntf adds a new route to the routing table for the given prefix and returns the next hop and interface.
 // If the next hop or interface is pointing to the VPN interface, it will return the initial values.
 func (r *SysOps) addRouteToNonVPNIntf(prefix netip.Prefix, vpnIntf iface.WGIface, initialNextHop Nexthop) (Nexthop, error) {
@@ -271,32 +230,7 @@ func (r *SysOps) genericAddVPNRoute(prefix netip.Prefix, intf *net.Interface) er
 		return nil
 	}
 
-	return r.addNonExistingRoute(prefix, intf)
-}
-
-// addNonExistingRoute adds a new route to the vpn interface if it doesn't exist in the current routing table
-func (r *SysOps) addNonExistingRoute(prefix netip.Prefix, intf *net.Interface) error {
-	ok, err := existsInRouteTable(prefix)
-	if err != nil {
-		return fmt.Errorf("exists in route table: %w", err)
-	}
-	if ok {
-		log.Warnf("Skipping adding a new route for network %s because it already exists", prefix)
-		return nil
-	}
-
-	ok, err = isSubRange(prefix)
-	if err != nil {
-		return fmt.Errorf("sub range: %w", err)
-	}
-
-	if ok {
-		if err := r.addRouteForCurrentDefaultGateway(prefix); err != nil {
-			log.Warnf("Unable to add route for current default gateway route. Will proceed without it. error: %s", err)
-		}
-	}
-
-	return r.addToRouteTable(prefix, Nexthop{netip.Addr{}, intf})
+	return r.addToRouteTable(prefix, nextHop)
 }
 
 // genericRemoveVPNRoute removes the route from the vpn interface. If a default prefix is given,
@@ -457,32 +391,6 @@ func ipToAddr(ip net.IP, intf *net.Interface) (netip.Addr, error) {
 	return addr.Unmap(), nil
 }
 
-func existsInRouteTable(prefix netip.Prefix) (bool, error) {
-	routes, err := GetRoutesFromTable()
-	if err != nil {
-		return false, fmt.Errorf("get routes from table: %w", err)
-	}
-	for _, tableRoute := range routes {
-		if tableRoute == prefix {
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
-func isSubRange(prefix netip.Prefix) (bool, error) {
-	routes, err := GetRoutesFromTable()
-	if err != nil {
-		return false, fmt.Errorf("get routes from table: %w", err)
-	}
-	for _, tableRoute := range routes {
-		if tableRoute.Bits() > vars.MinRangeBits && tableRoute.Contains(prefix.Addr()) && tableRoute.Bits() < prefix.Bits() {
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
 // IsAddrRouted checks if the candidate address would route to the vpn, in which case it returns true and the matched prefix.
 func IsAddrRouted(addr netip.Addr, vpnRoutes []netip.Prefix) (bool, netip.Prefix) {
 	localRoutes, err := hasSeparateRouting()

client/internal/routemanager/systemops/systemops_generic_test.go

@@ -10,7 +10,6 @@ import (
 	"net/netip"
 	"os"
 	"runtime"
-	"strings"
 	"testing"
 
 	"github.com/pion/transport/v3/stdnet"
@@ -165,25 +164,20 @@ func TestGetNextHop(t *testing.T) {
 func TestAddExistAndRemoveRoute(t *testing.T) {
 	defaultNexthop, err := GetNextHop(netip.MustParseAddr("0.0.0.0"))
 	t.Log("defaultNexthop: ", defaultNexthop)
-	if err != nil {
-		t.Fatal("shouldn't return error when fetching the gateway: ", err)
-	}
+	require.NoError(t, err, "shouldn't return error when fetching the gateway")
+
 	testCases := []struct {
 		name              string
 		prefix            netip.Prefix
 		preExistingPrefix netip.Prefix
 		shouldAddRoute    bool
+		expectError       bool
 	}{
 		{
 			name:           "Should Add And Remove random Route",
 			prefix:         netip.MustParsePrefix("99.99.99.99/32"),
 			shouldAddRoute: true,
 		},
-		{
-			name:           "Should Not Add Route if overlaps with default gateway",
-			prefix:         netip.MustParsePrefix(defaultNexthop.IP.String() + "/31"),
-			shouldAddRoute: false,
-		},
 		{
 			name:              "Should Add Route if bigger network exists",
 			prefix:            netip.MustParsePrefix("100.100.100.0/24"),
@@ -197,10 +191,10 @@ func TestAddExistAndRemoveRoute(t *testing.T) {
 			shouldAddRoute:    true,
 		},
 		{
-			name:              "Should Not Add Route if same network exists",
+			name:              "Should Error on duplicate route",
 			prefix:            netip.MustParsePrefix("100.100.0.0/16"),
 			preExistingPrefix: netip.MustParsePrefix("100.100.0.0/16"),
-			shouldAddRoute:    false,
+			expectError:       true,
 		},
 	}
 
@@ -217,9 +211,8 @@ func TestAddExistAndRemoveRoute(t *testing.T) {
 
 			peerPrivateKey, _ := wgtypes.GeneratePrivateKey()
 			newNet, err := stdnet.NewNet()
-			if err != nil {
-				t.Fatal(err)
-			}
+			require.NoError(t, err, "should create new net")
+
 			opts := iface.WGIFaceOpts{
 				IFaceName:    fmt.Sprintf("utun53%d", n),
 				Address:      "100.65.75.2/24",
@@ -249,6 +242,12 @@ func TestAddExistAndRemoveRoute(t *testing.T) {
 
 			// Add the route
 			err = r.AddVPNRoute(testCase.prefix, intf)
+
+			if testCase.expectError {
+				require.Error(t, err, "should return error")
+				return
+			}
+
 			require.NoError(t, err, "should not return err when adding route")
 
 			if testCase.shouldAddRoute {
@@ -260,97 +259,16 @@ func TestAddExistAndRemoveRoute(t *testing.T) {
 				// remove route again if added
 				err = r.RemoveVPNRoute(testCase.prefix, intf)
 				require.NoError(t, err, "should not return err")
-			}
-
-			// route should either not have been added or should have been removed
-			// In case of already existing route, it should not have been added (but still exist)
-			ok, err := existsInRouteTable(testCase.prefix)
-			t.Log("Buffer string: ", buf.String())
-			require.NoError(t, err, "should not return err")
 
-			if !strings.Contains(buf.String(), "because it already exists") {
-				require.False(t, ok, "route should not exist")
+				// route should be removed
+				ok, err = existsInRouteTable(testCase.prefix)
+				require.NoError(t, err, "should not return err")
+				require.False(t, ok, "route should not exist after removal")
 			}
 		})
 	}
 }
 
-func TestIsSubRange(t *testing.T) {
-	addresses, err := net.InterfaceAddrs()
-	if err != nil {
-		t.Fatal("shouldn't return error when fetching interface addresses: ", err)
-	}
-
-	var subRangeAddressPrefixes []netip.Prefix
-	var nonSubRangeAddressPrefixes []netip.Prefix
-	for _, address := range addresses {
-		p := netip.MustParsePrefix(address.String())
-		if !p.Addr().IsLoopback() && p.Addr().Is4() && p.Bits() < 32 {
-			p2 := netip.PrefixFrom(p.Masked().Addr(), p.Bits()+1)
-			subRangeAddressPrefixes = append(subRangeAddressPrefixes, p2)
-			nonSubRangeAddressPrefixes = append(nonSubRangeAddressPrefixes, p.Masked())
-		}
-	}
-
-	for _, prefix := range subRangeAddressPrefixes {
-		isSubRangePrefix, err := isSubRange(prefix)
-		if err != nil {
-			t.Fatal("shouldn't return error when checking if address is sub-range: ", err)
-		}
-		if !isSubRangePrefix {
-			t.Fatalf("address %s should be sub-range of an existing route in the table", prefix)
-		}
-	}
-
-	for _, prefix := range nonSubRangeAddressPrefixes {
-		isSubRangePrefix, err := isSubRange(prefix)
-		if err != nil {
-			t.Fatal("shouldn't return error when checking if address is sub-range: ", err)
-		}
-		if isSubRangePrefix {
-			t.Fatalf("address %s should not be sub-range of an existing route in the table", prefix)
-		}
-	}
-}
-
-func TestExistsInRouteTable(t *testing.T) {
-	addresses, err := net.InterfaceAddrs()
-	if err != nil {
-		t.Fatal("shouldn't return error when fetching interface addresses: ", err)
-	}
-
-	var addressPrefixes []netip.Prefix
-	for _, address := range addresses {
-		p := netip.MustParsePrefix(address.String())
-
-		switch {
-		case p.Addr().Is6():
-			continue
-		// Windows sometimes has hidden interface link local addrs that don't turn up on any interface
-		case runtime.GOOS == "windows" && p.Addr().IsLinkLocalUnicast():
-			continue
-		// Linux loopback 127/8 is in the local table, not in the main table and always takes precedence
-		case runtime.GOOS == "linux" && p.Addr().IsLoopback():
-			continue
-		// FreeBSD loopback 127/8 is not added to the routing table
-		case runtime.GOOS == "freebsd" && p.Addr().IsLoopback():
-			continue
-		default:
-			addressPrefixes = append(addressPrefixes, p.Masked())
-		}
-	}
-
-	for _, prefix := range addressPrefixes {
-		exists, err := existsInRouteTable(prefix)
-		if err != nil {
-			t.Fatal("shouldn't return error when checking if address exists in route table: ", err)
-		}
-		if !exists {
-			t.Fatalf("address %s should exist in route table", prefix)
-		}
-	}
-}
-
 func createWGInterface(t *testing.T, interfaceName, ipAddressCIDR string, listenPort int) *iface.WGIface {
 	t.Helper()
 
@@ -565,3 +483,16 @@ func TestIsVpnRoute(t *testing.T) {
 		})
 	}
 }
+
+func existsInRouteTable(prefix netip.Prefix) (bool, error) {
+	routes, err := GetRoutesFromTable()
+	if err != nil {
+		return false, fmt.Errorf("get routes from table: %w", err)
+	}
+	for _, tableRoute := range routes {
+		if tableRoute == prefix {
+			return true, nil
+		}
+	}
+	return false, nil
+}

client/internal/routemanager/systemops/systemops_linux.go

@@ -247,7 +247,7 @@ func addRoute(prefix netip.Prefix, nexthop Nexthop, tableID int) error {
 		return fmt.Errorf("add gateway and device: %w", err)
 	}
 
-	if err := netlink.RouteAdd(route); err != nil && !errors.Is(err, syscall.EEXIST) && !isOpErr(err) {
+	if err := netlink.RouteAdd(route); err != nil && !isOpErr(err) {
 		return fmt.Errorf("netlink add route: %w", err)
 	}
 
@@ -270,7 +270,7 @@ func addUnreachableRoute(prefix netip.Prefix, tableID int) error {
 		Dst:    ipNet,
 	}
 
-	if err := netlink.RouteAdd(route); err != nil && !errors.Is(err, syscall.EEXIST) && !isOpErr(err) {
+	if err := netlink.RouteAdd(route); err != nil && !isOpErr(err) {
 		return fmt.Errorf("netlink add unreachable route: %w", err)
 	}
 

client/internal/routemanager/systemops/systemops_windows.go

@@ -492,7 +492,7 @@ func cancelMibChangeNotify2(handle windows.Handle) error {
 }
 
 // GetRoutesFromTable returns the current routing table from with prefixes only.
-// It ccaches the result for 2 seconds to avoid blocking the caller.
+// It caches the result for 2 seconds to avoid blocking the caller.
 func GetRoutesFromTable() ([]netip.Prefix, error) {
 	mux.Lock()
 	defer mux.Unlock()