From bf0b6c5f8ae6150ef26dab4256a4e88efdbe3da0 Mon Sep 17 00:00:00 2001
From: Maycon Santos <mlsmaycon@gmail.com>
Date: Fri, 12 Apr 2024 18:44:01 +0200
Subject: [PATCH 1/5] Delete route from system only if added by the client
This adds a flags to check if the addVPNRoute executed without issues before removing a route from the system
---
client/internal/routemanager/client.go | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/client/internal/routemanager/client.go b/client/internal/routemanager/client.go
index d41ed422b81..eb9f6adea1e 100644
--- a/client/internal/routemanager/client.go
+++ b/client/internal/routemanager/client.go
@@ -39,6 +39,7 @@ type clientNetwork struct {
chosenRoute *route.Route
network netip.Prefix
updateSerial uint64
+ systemUpdated bool
}
func newClientNetworkWatcher(ctx context.Context, wgInterface *iface.WGIface, statusRecorder *peer.Status, network netip.Prefix) *clientNetwork {
@@ -215,8 +216,11 @@ func (c *clientNetwork) removeRouteFromWireguardPeer(peerKey string) error {
func (c *clientNetwork) removeRouteFromPeerAndSystem() error {
if c.chosenRoute != nil {
- if err := removeVPNRoute(c.network, c.wgInterface.Name()); err != nil {
- return fmt.Errorf("remove route %s from system, err: %v", c.network, err)
+ if c.systemUpdated {
+ if err := removeVPNRoute(c.network, c.wgInterface.Name()); err != nil {
+ return fmt.Errorf("remove route %s from system, err: %v", c.network, err)
+ }
+ c.systemUpdated = false
}
if err := c.removeRouteFromWireguardPeer(c.chosenRoute.Peer); err != nil {
@@ -260,6 +264,8 @@ func (c *clientNetwork) recalculateRouteAndUpdatePeerAndSystem() error {
return fmt.Errorf("route %s couldn't be added for peer %s, err: %v",
c.network.String(), c.wgInterface.Address().IP.String(), err)
}
+
+ c.systemUpdated = true
}
c.chosenRoute = c.routes[chosen]
From c4f17771e356d3a8859556db7c5baf7225e32844 Mon Sep 17 00:00:00 2001
From: Maycon Santos <mlsmaycon@gmail.com>
Date: Fri, 12 Apr 2024 18:44:48 +0200
Subject: [PATCH 2/5] return error instead of log
---
client/internal/routemanager/systemops.go | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/client/internal/routemanager/systemops.go b/client/internal/routemanager/systemops.go
index 1ee54b746d8..ad7edd366ef 100644
--- a/client/internal/routemanager/systemops.go
+++ b/client/internal/routemanager/systemops.go
@@ -256,8 +256,7 @@ func addNonExistingRoute(prefix netip.Prefix, intf string) error {
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
+ return fmt.Errorf("Skipping adding a new route for network %s because it already exists", prefix)
}
ok, err = isSubRange(prefix)
From 56450e1b4fc9fcddfff369e2aeea4594cc50d9c2 Mon Sep 17 00:00:00 2001
From: Maycon Santos <mlsmaycon@gmail.com>
Date: Sun, 14 Apr 2024 16:39:30 +0200
Subject: [PATCH 3/5] Update client/internal/routemanager/systemops.go
Co-authored-by: Viktor Liu <viktor@netbird.io>
---
client/internal/routemanager/systemops.go | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/client/internal/routemanager/systemops.go b/client/internal/routemanager/systemops.go
index ad7edd366ef..6c14fb77501 100644
--- a/client/internal/routemanager/systemops.go
+++ b/client/internal/routemanager/systemops.go
@@ -256,7 +256,7 @@ func addNonExistingRoute(prefix netip.Prefix, intf string) error {
return fmt.Errorf("exists in route table: %w", err)
}
if ok {
- return fmt.Errorf("Skipping adding a new route for network %s because it already exists", prefix)
+ return fmt.Errorf("skipping adding a new route for network %s because it already exists", prefix)
}
ok, err = isSubRange(prefix)
From c4ca7ef1b15842646b7e4086c288e7b0170cb64f Mon Sep 17 00:00:00 2001
From: Maycon Santos <mlsmaycon@gmail.com>
Date: Sun, 14 Apr 2024 16:43:56 +0200
Subject: [PATCH 4/5] add debug log
---
client/internal/routemanager/client.go | 2 ++
1 file changed, 2 insertions(+)
diff --git a/client/internal/routemanager/client.go b/client/internal/routemanager/client.go
index eb9f6adea1e..28a862a9258 100644
--- a/client/internal/routemanager/client.go
+++ b/client/internal/routemanager/client.go
@@ -221,6 +221,8 @@ func (c *clientNetwork) removeRouteFromPeerAndSystem() error {
return fmt.Errorf("remove route %s from system, err: %v", c.network, err)
}
c.systemUpdated = false
+ } else {
+ log.Debugf("system is not updated for network %s, skiping removal from system route table", c.network)
}
if err := c.removeRouteFromWireguardPeer(c.chosenRoute.Peer); err != nil {
From 64faeb5fae46c4053b64242577778c3381fee754 Mon Sep 17 00:00:00 2001
From: Maycon Santos <mlsmaycon@gmail.com>
Date: Sun, 14 Apr 2024 22:45:01 +0200
Subject: [PATCH 5/5] fix tests
---
.../internal/routemanager/systemops_test.go | 60 ++++++++-----------
1 file changed, 25 insertions(+), 35 deletions(-)
diff --git a/client/internal/routemanager/systemops_test.go b/client/internal/routemanager/systemops_test.go
index 9f906c06fbe..887f43418bb 100644
--- a/client/internal/routemanager/systemops_test.go
+++ b/client/internal/routemanager/systemops_test.go
@@ -3,18 +3,14 @@
package routemanager
import (
- "bytes"
"context"
"fmt"
"net"
"net/netip"
- "os"
"runtime"
- "strings"
"testing"
"github.com/pion/transport/v3/stdnet"
- log "github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.zx2c4.com/wireguard/wgctrl/wgtypes"
@@ -41,8 +37,8 @@ func TestAddRemoveRoutes(t *testing.T) {
shouldBeRemoved: true,
},
{
- name: "Should Not Add Or Remove Route 127.0.0.1/32",
- prefix: netip.MustParsePrefix("127.0.0.1/32"),
+ name: "Should Not Add Or Remove Route 127.0.1.1/32",
+ prefix: netip.MustParsePrefix("127.0.1.1/32"),
shouldRouteToWireguard: false,
shouldBeRemoved: false,
},
@@ -140,10 +136,10 @@ func TestGetNextHop(t *testing.T) {
}
func TestAddExistAndRemoveRoute(t *testing.T) {
- defaultGateway, _, err := getNextHop(netip.MustParseAddr("0.0.0.0"))
+ defaultGateway, _, errGW := getNextHop(netip.MustParseAddr("0.0.0.0"))
t.Log("defaultGateway: ", defaultGateway)
- if err != nil {
- t.Fatal("shouldn't return error when fetching the gateway: ", err)
+ if errGW != nil {
+ t.Fatal("shouldn't return error when fetching the gateway: ", errGW)
}
testCases := []struct {
name string
@@ -159,7 +155,7 @@ func TestAddExistAndRemoveRoute(t *testing.T) {
{
name: "Should Not Add Route if overlaps with default gateway",
prefix: netip.MustParsePrefix(defaultGateway.String() + "/31"),
- shouldAddRoute: false,
+ shouldAddRoute: true,
},
{
name: "Should Add Route if bigger network exists",
@@ -182,11 +178,6 @@ func TestAddExistAndRemoveRoute(t *testing.T) {
}
for n, testCase := range testCases {
- var buf bytes.Buffer
- log.SetOutput(&buf)
- defer func() {
- log.SetOutput(os.Stderr)
- }()
t.Run(testCase.name, func(t *testing.T) {
peerPrivateKey, _ := wgtypes.GeneratePrivateKey()
newNet, err := stdnet.NewNet()
@@ -202,34 +193,33 @@ func TestAddExistAndRemoveRoute(t *testing.T) {
// Prepare the environment
if testCase.preExistingPrefix.IsValid() {
- err := genericAddVPNRoute(testCase.preExistingPrefix, wgInterface.Name())
+ err = genericAddVPNRoute(testCase.preExistingPrefix, wgInterface.Name())
require.NoError(t, err, "should not return err when adding pre-existing route")
+ ok, err := existsInRouteTable(testCase.preExistingPrefix)
+ require.NoError(t, err, "should not return err")
+ require.True(t, ok, "route should exist")
}
// Add the route
err = genericAddVPNRoute(testCase.prefix, wgInterface.Name())
- require.NoError(t, err, "should not return err when adding route")
-
- if testCase.shouldAddRoute {
- // test if route exists after adding
- ok, err := existsInRouteTable(testCase.prefix)
- require.NoError(t, err, "should not return err")
- require.True(t, ok, "route should exist")
-
- // remove route again if added
- err = genericRemoveVPNRoute(testCase.prefix, wgInterface.Name())
- require.NoError(t, err, "should not return err")
+ if !testCase.shouldAddRoute {
+ require.Error(t, err, "should return err when adding existing route")
+ return
}
-
- // 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)
+ require.NoError(t, err, "should not return err when adding route")
+ // test if route exists after adding
ok, err := existsInRouteTable(testCase.prefix)
- t.Log("Buffer string: ", buf.String())
require.NoError(t, err, "should not return err")
+ require.True(t, ok, "route should exist")
- if !strings.Contains(buf.String(), "because it already exists") {
- require.False(t, ok, "route should not exist")
- }
+ // remove route again if added
+ err = genericRemoveVPNRoute(testCase.prefix, wgInterface.Name())
+ require.NoError(t, err, "should not return err")
+
+ // test if route was removed
+ ok, err = existsInRouteTable(testCase.prefix)
+ require.NoError(t, err, "should not return err")
+ require.False(t, ok, "route should not exist")
})
}
}
@@ -355,7 +345,7 @@ func setupTestEnv(t *testing.T) {
// 10.0.0.0/8 route exists in main table and vpn table
err = addVPNRoute(netip.MustParsePrefix("10.0.0.0/8"), wgIface.Name())
- require.NoError(t, err, "addVPNRoute should not return err")
+ require.Error(t, err, "addVPNRoute should return err")
t.Cleanup(func() {
err = removeVPNRoute(netip.MustParsePrefix("10.0.0.0/8"), wgIface.Name())
assert.NoError(t, err, "removeVPNRoute should not return err")