Routemgr error handling (#1073)

In case the route management feature is not supported 
then do not create unnecessary firewall and manager instances. 
This can happen if the nftables nor iptables is not available on the host OS.

- Move the error handling to upper layer
- Remove fake, useless implementations of interfaces
- Update go-iptables because In Docker the old version can not 
determine well the path of executable file
- update lib to 0.70
This commit is contained in:
Zoltan Papp 2023-08-12 11:42:36 +02:00 committed by GitHub
parent 2dec016201
commit 0f0c7ec2ed
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 79 additions and 88 deletions

View File

@ -27,14 +27,19 @@ func genKey(format string, input string) string {
} }
// NewFirewall if supported, returns an iptables manager, otherwise returns a nftables manager // NewFirewall if supported, returns an iptables manager, otherwise returns a nftables manager
func NewFirewall(parentCTX context.Context) firewallManager { func NewFirewall(parentCTX context.Context) (firewallManager, error) {
manager, err := newNFTablesManager(parentCTX) manager, err := newNFTablesManager(parentCTX)
if err == nil { if err == nil {
log.Debugf("nftables firewall manager will be used") log.Debugf("nftables firewall manager will be used")
return manager return manager, nil
} }
log.Debugf("fallback to iptables firewall manager: %s", err) fMgr, err := newIptablesManager(parentCTX)
return newIptablesManager(parentCTX) if err != nil {
log.Debugf("failed to initialize iptables for root mgr: %s", err)
return nil, err
}
log.Debugf("iptables firewall manager will be used")
return fMgr, nil
} }
func getInPair(pair routerPair) routerPair { func getInPair(pair routerPair) routerPair {

View File

@ -3,24 +3,12 @@
package routemanager package routemanager
import "context" import (
"context"
"fmt"
)
type unimplementedFirewall struct{} // NewFirewall returns a nil manager
func NewFirewall(context.Context) (firewallManager, error) {
func (unimplementedFirewall) RestoreOrCreateContainers() error { return nil, fmt.Errorf("firewall not supported on this OS")
return nil
}
func (unimplementedFirewall) InsertRoutingRules(pair routerPair) error {
return nil
}
func (unimplementedFirewall) RemoveRoutingRules(pair routerPair) error {
return nil
}
func (unimplementedFirewall) CleanRoutingRules() {
}
// NewFirewall returns an unimplemented Firewall manager
func NewFirewall(parentCtx context.Context) firewallManager {
return unimplementedFirewall{}
} }

View File

@ -49,14 +49,12 @@ type iptablesManager struct {
mux sync.Mutex mux sync.Mutex
} }
func newIptablesManager(parentCtx context.Context) *iptablesManager { func newIptablesManager(parentCtx context.Context) (*iptablesManager, error) {
ctx, cancel := context.WithCancel(parentCtx)
ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4)
if err != nil { if err != nil {
log.Debugf("failed to initialize iptables for ipv4: %s", err) return nil, err
} else if !isIptablesClientAvailable(ipv4Client) { } else if !isIptablesClientAvailable(ipv4Client) {
log.Infof("iptables is missing for ipv4") return nil, fmt.Errorf("iptables is missing for ipv4")
ipv4Client = nil
} }
ipv6Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv6) ipv6Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv6)
if err != nil { if err != nil {
@ -66,13 +64,14 @@ func newIptablesManager(parentCtx context.Context) *iptablesManager {
ipv6Client = nil ipv6Client = nil
} }
ctx, cancel := context.WithCancel(parentCtx)
return &iptablesManager{ return &iptablesManager{
ctx: ctx, ctx: ctx,
stop: cancel, stop: cancel,
ipv4Client: ipv4Client, ipv4Client: ipv4Client,
ipv6Client: ipv6Client, ipv6Client: ipv6Client,
rules: make(map[string]map[string][]string), rules: make(map[string]map[string][]string),
} }, nil
} }
// CleanRoutingRules cleans existing iptables resources that we created by the agent // CleanRoutingRules cleans existing iptables resources that we created by the agent
@ -395,6 +394,10 @@ func (i *iptablesManager) insertRoutingRule(keyFormat, table, chain, jump string
ipVersion = ipv6 ipVersion = ipv6
} }
if iptablesClient == nil {
return fmt.Errorf("unable to insert iptables routing rules. Iptables client is not initialized")
}
ruleKey := genKey(keyFormat, pair.ID) ruleKey := genKey(keyFormat, pair.ID)
rule := genRuleSpec(jump, ruleKey, pair.source, pair.destination) rule := genRuleSpec(jump, ruleKey, pair.source, pair.destination)
existingRule, found := i.rules[ipVersion][ruleKey] existingRule, found := i.rules[ipVersion][ruleKey]
@ -459,6 +462,10 @@ func (i *iptablesManager) removeRoutingRule(keyFormat, table, chain string, pair
ipVersion = ipv6 ipVersion = ipv6
} }
if iptablesClient == nil {
return fmt.Errorf("unable to remove iptables routing rules. Iptables client is not initialized")
}
ruleKey := genKey(keyFormat, pair.ID) ruleKey := genKey(keyFormat, pair.ID)
existingRule, found := i.rules[ipVersion][ruleKey] existingRule, found := i.rules[ipVersion][ruleKey]
if found { if found {

View File

@ -16,7 +16,7 @@ func TestIptablesManager_RestoreOrCreateContainers(t *testing.T) {
t.SkipNow() t.SkipNow()
} }
manager := newIptablesManager(context.TODO()) manager, _ := newIptablesManager(context.TODO())
defer manager.CleanRoutingRules() defer manager.CleanRoutingRules()

View File

@ -27,7 +27,7 @@ type DefaultManager struct {
stop context.CancelFunc stop context.CancelFunc
mux sync.Mutex mux sync.Mutex
clientNetworks map[string]*clientNetwork clientNetworks map[string]*clientNetwork
serverRouter *serverRouter serverRouter serverRouter
statusRecorder *peer.Status statusRecorder *peer.Status
wgInterface *iface.WGIface wgInterface *iface.WGIface
pubKey string pubKey string
@ -36,13 +36,17 @@ type DefaultManager struct {
// NewManager returns a new route manager // NewManager returns a new route manager
func NewManager(ctx context.Context, pubKey string, wgInterface *iface.WGIface, statusRecorder *peer.Status, initialRoutes []*route.Route) *DefaultManager { func NewManager(ctx context.Context, pubKey string, wgInterface *iface.WGIface, statusRecorder *peer.Status, initialRoutes []*route.Route) *DefaultManager {
mCTX, cancel := context.WithCancel(ctx) serverRouter, err := newServerRouter(ctx, wgInterface)
if err != nil {
log.Errorf("server router is not supported: %s", err)
}
mCTX, cancel := context.WithCancel(ctx)
dm := &DefaultManager{ dm := &DefaultManager{
ctx: mCTX, ctx: mCTX,
stop: cancel, stop: cancel,
clientNetworks: make(map[string]*clientNetwork), clientNetworks: make(map[string]*clientNetwork),
serverRouter: newServerRouter(ctx, wgInterface), serverRouter: serverRouter,
statusRecorder: statusRecorder, statusRecorder: statusRecorder,
wgInterface: wgInterface, wgInterface: wgInterface,
pubKey: pubKey, pubKey: pubKey,
@ -59,7 +63,9 @@ func NewManager(ctx context.Context, pubKey string, wgInterface *iface.WGIface,
// Stop stops the manager watchers and clean firewall rules // Stop stops the manager watchers and clean firewall rules
func (m *DefaultManager) Stop() { func (m *DefaultManager) Stop() {
m.stop() m.stop()
if m.serverRouter != nil {
m.serverRouter.cleanUp() m.serverRouter.cleanUp()
}
m.ctx = nil m.ctx = nil
} }
@ -77,10 +83,13 @@ func (m *DefaultManager) UpdateRoutes(updateSerial uint64, newRoutes []*route.Ro
m.updateClientNetworks(updateSerial, newClientRoutesIDMap) m.updateClientNetworks(updateSerial, newClientRoutesIDMap)
m.notifier.onNewRoutes(newClientRoutesIDMap) m.notifier.onNewRoutes(newClientRoutesIDMap)
if m.serverRouter != nil {
err := m.serverRouter.updateRoutes(newServerRoutesMap) err := m.serverRouter.updateRoutes(newServerRoutesMap)
if err != nil { if err != nil {
return err return err
} }
}
return nil return nil
} }

View File

@ -30,7 +30,6 @@ func TestManagerUpdateRoutes(t *testing.T) {
inputInitRoutes []*route.Route inputInitRoutes []*route.Route
inputRoutes []*route.Route inputRoutes []*route.Route
inputSerial uint64 inputSerial uint64
shouldCheckServerRoutes bool
serverRoutesExpected int serverRoutesExpected int
clientNetworkWatchersExpected int clientNetworkWatchersExpected int
}{ }{
@ -87,7 +86,6 @@ func TestManagerUpdateRoutes(t *testing.T) {
}, },
}, },
inputSerial: 1, inputSerial: 1,
shouldCheckServerRoutes: runtime.GOOS == "linux",
serverRoutesExpected: 2, serverRoutesExpected: 2,
clientNetworkWatchersExpected: 0, clientNetworkWatchersExpected: 0,
}, },
@ -116,7 +114,6 @@ func TestManagerUpdateRoutes(t *testing.T) {
}, },
}, },
inputSerial: 1, inputSerial: 1,
shouldCheckServerRoutes: runtime.GOOS == "linux",
serverRoutesExpected: 1, serverRoutesExpected: 1,
clientNetworkWatchersExpected: 1, clientNetworkWatchersExpected: 1,
}, },
@ -174,25 +171,6 @@ func TestManagerUpdateRoutes(t *testing.T) {
inputSerial: 1, inputSerial: 1,
clientNetworkWatchersExpected: 0, clientNetworkWatchersExpected: 0,
}, },
{
name: "No Server Routes Should Be Added To Non Linux",
inputRoutes: []*route.Route{
{
ID: "a",
NetID: "routeA",
Peer: localPeerKey,
Network: netip.MustParsePrefix("1.2.3.4/32"),
NetworkType: route.IPv4Network,
Metric: 9999,
Masquerade: false,
Enabled: true,
},
},
inputSerial: 1,
shouldCheckServerRoutes: runtime.GOOS != "linux",
serverRoutesExpected: 0,
clientNetworkWatchersExpected: 0,
},
{ {
name: "Remove 1 Client Route", name: "Remove 1 Client Route",
inputInitRoutes: []*route.Route{ inputInitRoutes: []*route.Route{
@ -335,7 +313,6 @@ func TestManagerUpdateRoutes(t *testing.T) {
}, },
inputRoutes: []*route.Route{}, inputRoutes: []*route.Route{},
inputSerial: 1, inputSerial: 1,
shouldCheckServerRoutes: true,
serverRoutesExpected: 0, serverRoutesExpected: 0,
clientNetworkWatchersExpected: 0, clientNetworkWatchersExpected: 0,
}, },
@ -384,7 +361,6 @@ func TestManagerUpdateRoutes(t *testing.T) {
}, },
}, },
inputSerial: 1, inputSerial: 1,
shouldCheckServerRoutes: runtime.GOOS == "linux",
serverRoutesExpected: 2, serverRoutesExpected: 2,
clientNetworkWatchersExpected: 1, clientNetworkWatchersExpected: 1,
}, },
@ -419,8 +395,9 @@ func TestManagerUpdateRoutes(t *testing.T) {
require.Len(t, routeManager.clientNetworks, testCase.clientNetworkWatchersExpected, "client networks size should match") require.Len(t, routeManager.clientNetworks, testCase.clientNetworkWatchersExpected, "client networks size should match")
if testCase.shouldCheckServerRoutes { if runtime.GOOS == "linux" {
require.Len(t, routeManager.serverRouter.routes, testCase.serverRoutesExpected, "server networks size should match") sr := routeManager.serverRouter.(*defaultServerRouter)
require.Len(t, sr.routes, testCase.serverRoutesExpected, "server networks size should match")
} }
}) })
} }

View File

@ -0,0 +1,9 @@
package routemanager
import "github.com/netbirdio/netbird/route"
type serverRouter interface {
updateRoutes(map[string]*route.Route) error
removeFromServerNetwork(*route.Route) error
cleanUp()
}

View File

@ -2,20 +2,11 @@ package routemanager
import ( import (
"context" "context"
"fmt"
"github.com/netbirdio/netbird/iface" "github.com/netbirdio/netbird/iface"
"github.com/netbirdio/netbird/route"
) )
type serverRouter struct { func newServerRouter(context.Context, *iface.WGIface) (serverRouter, error) {
return nil, fmt.Errorf("server route not supported on this os")
} }
func newServerRouter(ctx context.Context, wgInterface *iface.WGIface) *serverRouter {
return &serverRouter{}
}
func (r *serverRouter) updateRoutes(routesMap map[string]*route.Route) error {
return nil
}
func (r *serverRouter) cleanUp() {}

View File

@ -13,7 +13,7 @@ import (
"github.com/netbirdio/netbird/route" "github.com/netbirdio/netbird/route"
) )
type serverRouter struct { type defaultServerRouter struct {
mux sync.Mutex mux sync.Mutex
ctx context.Context ctx context.Context
routes map[string]*route.Route routes map[string]*route.Route
@ -21,16 +21,21 @@ type serverRouter struct {
wgInterface *iface.WGIface wgInterface *iface.WGIface
} }
func newServerRouter(ctx context.Context, wgInterface *iface.WGIface) *serverRouter { func newServerRouter(ctx context.Context, wgInterface *iface.WGIface) (serverRouter, error) {
return &serverRouter{ firewall, err := NewFirewall(ctx)
ctx: ctx, if err != nil {
routes: make(map[string]*route.Route), return nil, err
firewall: NewFirewall(ctx),
wgInterface: wgInterface,
}
} }
func (m *serverRouter) updateRoutes(routesMap map[string]*route.Route) error { return &defaultServerRouter{
ctx: ctx,
routes: make(map[string]*route.Route),
firewall: firewall,
wgInterface: wgInterface,
}, nil
}
func (m *defaultServerRouter) updateRoutes(routesMap map[string]*route.Route) error {
serverRoutesToRemove := make([]string, 0) serverRoutesToRemove := make([]string, 0)
if len(routesMap) > 0 { if len(routesMap) > 0 {
@ -81,7 +86,7 @@ func (m *serverRouter) updateRoutes(routesMap map[string]*route.Route) error {
return nil return nil
} }
func (m *serverRouter) removeFromServerNetwork(route *route.Route) error { func (m *defaultServerRouter) removeFromServerNetwork(route *route.Route) error {
select { select {
case <-m.ctx.Done(): case <-m.ctx.Done():
log.Infof("not removing from server network because context is done") log.Infof("not removing from server network because context is done")
@ -98,7 +103,7 @@ func (m *serverRouter) removeFromServerNetwork(route *route.Route) error {
} }
} }
func (m *serverRouter) addToServerNetwork(route *route.Route) error { func (m *defaultServerRouter) addToServerNetwork(route *route.Route) error {
select { select {
case <-m.ctx.Done(): case <-m.ctx.Done():
log.Infof("not adding to server network because context is done") log.Infof("not adding to server network because context is done")
@ -115,6 +120,6 @@ func (m *serverRouter) addToServerNetwork(route *route.Route) error {
} }
} }
func (m *serverRouter) cleanUp() { func (m *defaultServerRouter) cleanUp() {
m.firewall.CleanRoutingRules() m.firewall.CleanRoutingRules()
} }

View File

@ -20,7 +20,7 @@ func InterfaceFilter(disallowList []string) func(string) bool {
for _, s := range disallowList { for _, s := range disallowList {
if strings.HasPrefix(iFace, s) { if strings.HasPrefix(iFace, s) {
log.Debugf("ignoring interface %s - it is not allowed", iFace) log.Tracef("ignoring interface %s - it is not allowed", iFace)
return false return false
} }
} }

2
go.mod
View File

@ -31,7 +31,7 @@ require (
fyne.io/fyne/v2 v2.1.4 fyne.io/fyne/v2 v2.1.4
github.com/c-robinson/iplib v1.0.3 github.com/c-robinson/iplib v1.0.3
github.com/cilium/ebpf v0.10.0 github.com/cilium/ebpf v0.10.0
github.com/coreos/go-iptables v0.6.0 github.com/coreos/go-iptables v0.7.0
github.com/creack/pty v1.1.18 github.com/creack/pty v1.1.18
github.com/eko/gocache/v3 v3.1.1 github.com/eko/gocache/v3 v3.1.1
github.com/getlantern/systray v1.2.1 github.com/getlantern/systray v1.2.1

4
go.sum
View File

@ -131,8 +131,8 @@ github.com/containerd/typeurl v1.0.2/go.mod h1:9trJWW2sRlGub4wZJRTW83VtbOLS6hwcD
github.com/coocood/freecache v1.2.1 h1:/v1CqMq45NFH9mp/Pt142reundeBM0dVUD3osQBeu/U= github.com/coocood/freecache v1.2.1 h1:/v1CqMq45NFH9mp/Pt142reundeBM0dVUD3osQBeu/U=
github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk=
github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE=
github.com/coreos/go-iptables v0.6.0 h1:is9qnZMPYjLd8LYqmm/qlE+wwEgJIkTYdhV3rfZo4jk= github.com/coreos/go-iptables v0.7.0 h1:XWM3V+MPRr5/q51NuWSgU0fqMad64Zyxs8ZUoMsamr8=
github.com/coreos/go-iptables v0.6.0/go.mod h1:Qe8Bv2Xik5FyTXwgIbLAnv2sWSBmvWdFETJConOQ//Q= github.com/coreos/go-iptables v0.7.0/go.mod h1:Qe8Bv2Xik5FyTXwgIbLAnv2sWSBmvWdFETJConOQ//Q=
github.com/coreos/go-semver v0.2.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk= github.com/coreos/go-semver v0.2.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk=
github.com/coreos/go-systemd v0.0.0-20190321100706-95778dfbb74e/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= github.com/coreos/go-systemd v0.0.0-20190321100706-95778dfbb74e/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4=
github.com/coreos/go-systemd/v22 v22.0.0/go.mod h1:xO0FLkIi5MaZafQlIrOotqXZ90ih+1atmu1JpKERPPk= github.com/coreos/go-systemd/v22 v22.0.0/go.mod h1:xO0FLkIi5MaZafQlIrOotqXZ90ih+1atmu1JpKERPPk=