From 3dfb43e117af99b68f6c43b75b86ce7c6a4a2f21 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Sat, 19 Oct 2024 12:43:26 +0000 Subject: [PATCH] chore(netlink): debug log ip rule commands in netlink instead of routing package --- internal/netlink/helpers_test.go | 8 +++ internal/netlink/interfaces.go | 1 + internal/netlink/rule.go | 41 ++++++++++++++ internal/netlink/rule_test.go | 50 +++++++++++++++++ internal/routing/logger.go | 2 - internal/routing/logger_mock_test.go | 82 ---------------------------- internal/routing/rules.go | 36 ------------ internal/routing/rules_test.go | 61 --------------------- 8 files changed, 100 insertions(+), 181 deletions(-) create mode 100644 internal/netlink/helpers_test.go create mode 100644 internal/netlink/rule_test.go delete mode 100644 internal/routing/logger_mock_test.go diff --git a/internal/netlink/helpers_test.go b/internal/netlink/helpers_test.go new file mode 100644 index 00000000..f8a565a2 --- /dev/null +++ b/internal/netlink/helpers_test.go @@ -0,0 +1,8 @@ +package netlink + +import "net/netip" + +func makeNetipPrefix(n byte) netip.Prefix { + const bits = 24 + return netip.PrefixFrom(netip.AddrFrom4([4]byte{n, n, n, 0}), bits) +} diff --git a/internal/netlink/interfaces.go b/internal/netlink/interfaces.go index 89403978..7e9e9ac1 100644 --- a/internal/netlink/interfaces.go +++ b/internal/netlink/interfaces.go @@ -3,6 +3,7 @@ package netlink import "github.com/qdm12/log" type DebugLogger interface { + Debug(message string) Debugf(format string, args ...any) Patch(options ...log.Option) } diff --git a/internal/netlink/rule.go b/internal/netlink/rule.go index e52b25e7..8da2d910 100644 --- a/internal/netlink/rule.go +++ b/internal/netlink/rule.go @@ -3,6 +3,8 @@ package netlink import ( + "fmt" + "github.com/vishvananda/netlink" ) @@ -17,6 +19,15 @@ func NewRule() Rule { } func (n *NetLink) RuleList(family int) (rules []Rule, err error) { + switch family { + case FamilyAll: + n.debugLogger.Debug("ip -4 rule list") + n.debugLogger.Debug("ip -6 rule list") + case FamilyV4: + n.debugLogger.Debug("ip -4 rule list") + case FamilyV6: + n.debugLogger.Debug("ip -6 rule list") + } netlinkRules, err := netlink.RuleList(family) if err != nil { return nil, err @@ -30,11 +41,13 @@ func (n *NetLink) RuleList(family int) (rules []Rule, err error) { } func (n *NetLink) RuleAdd(rule Rule) error { + n.debugLogger.Debug(ruleDbgMsg(true, rule)) netlinkRule := ruleToNetlinkRule(rule) return netlink.RuleAdd(&netlinkRule) } func (n *NetLink) RuleDel(rule Rule) error { + n.debugLogger.Debug(ruleDbgMsg(false, rule)) netlinkRule := ruleToNetlinkRule(rule) return netlink.RuleDel(&netlinkRule) } @@ -62,3 +75,31 @@ func netlinkRuleToRule(netlinkRule netlink.Rule) (rule Rule) { Invert: netlinkRule.Invert, } } + +func ruleDbgMsg(add bool, rule Rule) (debugMessage string) { + debugMessage = "ip rule" + + if add { + debugMessage += " add" + } else { + debugMessage += " del" + } + + if rule.Src.IsValid() { + debugMessage += " from " + rule.Src.String() + } + + if rule.Dst.IsValid() { + debugMessage += " to " + rule.Dst.String() + } + + if rule.Table != 0 { + debugMessage += " lookup " + fmt.Sprint(rule.Table) + } + + if rule.Priority != -1 { + debugMessage += " pref " + fmt.Sprint(rule.Priority) + } + + return debugMessage +} diff --git a/internal/netlink/rule_test.go b/internal/netlink/rule_test.go new file mode 100644 index 00000000..bfec7951 --- /dev/null +++ b/internal/netlink/rule_test.go @@ -0,0 +1,50 @@ +package netlink + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_ruleDbgMsg(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + add bool + rule Rule + dbgMsg string + }{ + "default values": { + dbgMsg: "ip rule del pref 0", + }, + "add rule": { + add: true, + rule: Rule{ + Src: makeNetipPrefix(1), + Dst: makeNetipPrefix(2), + Table: 100, + Priority: 101, + }, + dbgMsg: "ip rule add from 1.1.1.0/24 to 2.2.2.0/24 lookup 100 pref 101", + }, + "del rule": { + rule: Rule{ + Src: makeNetipPrefix(1), + Dst: makeNetipPrefix(2), + Table: 100, + Priority: 101, + }, + dbgMsg: "ip rule del from 1.1.1.0/24 to 2.2.2.0/24 lookup 100 pref 101", + }, + } + + for name, testCase := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + dbgMsg := ruleDbgMsg(testCase.add, testCase.rule) + + assert.Equal(t, testCase.dbgMsg, dbgMsg) + }) + } +} diff --git a/internal/routing/logger.go b/internal/routing/logger.go index 327ee580..3992d117 100644 --- a/internal/routing/logger.go +++ b/internal/routing/logger.go @@ -1,7 +1,5 @@ package routing -//go:generate mockgen -destination=logger_mock_test.go -package routing . Logger - type Logger interface { Debug(s string) Info(s string) diff --git a/internal/routing/logger_mock_test.go b/internal/routing/logger_mock_test.go deleted file mode 100644 index 17f3b1ec..00000000 --- a/internal/routing/logger_mock_test.go +++ /dev/null @@ -1,82 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/qdm12/gluetun/internal/routing (interfaces: Logger) - -// Package routing is a generated GoMock package. -package routing - -import ( - reflect "reflect" - - gomock "github.com/golang/mock/gomock" -) - -// MockLogger is a mock of Logger interface. -type MockLogger struct { - ctrl *gomock.Controller - recorder *MockLoggerMockRecorder -} - -// MockLoggerMockRecorder is the mock recorder for MockLogger. -type MockLoggerMockRecorder struct { - mock *MockLogger -} - -// NewMockLogger creates a new mock instance. -func NewMockLogger(ctrl *gomock.Controller) *MockLogger { - mock := &MockLogger{ctrl: ctrl} - mock.recorder = &MockLoggerMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockLogger) EXPECT() *MockLoggerMockRecorder { - return m.recorder -} - -// Debug mocks base method. -func (m *MockLogger) Debug(arg0 string) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "Debug", arg0) -} - -// Debug indicates an expected call of Debug. -func (mr *MockLoggerMockRecorder) Debug(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Debug", reflect.TypeOf((*MockLogger)(nil).Debug), arg0) -} - -// Error mocks base method. -func (m *MockLogger) Error(arg0 string) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "Error", arg0) -} - -// Error indicates an expected call of Error. -func (mr *MockLoggerMockRecorder) Error(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Error", reflect.TypeOf((*MockLogger)(nil).Error), arg0) -} - -// Info mocks base method. -func (m *MockLogger) Info(arg0 string) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "Info", arg0) -} - -// Info indicates an expected call of Info. -func (mr *MockLoggerMockRecorder) Info(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Info", reflect.TypeOf((*MockLogger)(nil).Info), arg0) -} - -// Warn mocks base method. -func (m *MockLogger) Warn(arg0 string) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "Warn", arg0) -} - -// Warn indicates an expected call of Warn. -func (mr *MockLoggerMockRecorder) Warn(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Warn", reflect.TypeOf((*MockLogger)(nil).Warn), arg0) -} diff --git a/internal/routing/rules.go b/internal/routing/rules.go index 4e3b6d03..c4fc9c10 100644 --- a/internal/routing/rules.go +++ b/internal/routing/rules.go @@ -8,9 +8,6 @@ import ( ) func (r *Routing) addIPRule(src, dst netip.Prefix, table, priority int) error { - const add = true - r.logger.Debug(ruleDbgMsg(add, src, dst, table, priority)) - rule := netlink.NewRule() rule.Src = src rule.Dst = dst @@ -35,9 +32,6 @@ func (r *Routing) addIPRule(src, dst netip.Prefix, table, priority int) error { } func (r *Routing) deleteIPRule(src, dst netip.Prefix, table, priority int) error { - const add = false - r.logger.Debug(ruleDbgMsg(add, src, dst, table, priority)) - rule := netlink.NewRule() rule.Src = src rule.Dst = dst @@ -59,36 +53,6 @@ func (r *Routing) deleteIPRule(src, dst netip.Prefix, table, priority int) error return nil } -func ruleDbgMsg(add bool, src, dst netip.Prefix, - table, priority int, -) (debugMessage string) { - debugMessage = "ip rule" - - if add { - debugMessage += " add" - } else { - debugMessage += " del" - } - - if src.IsValid() { - debugMessage += " from " + src.String() - } - - if dst.IsValid() { - debugMessage += " to " + dst.String() - } - - if table != 0 { - debugMessage += " lookup " + fmt.Sprint(table) - } - - if priority != -1 { - debugMessage += " pref " + fmt.Sprint(priority) - } - - return debugMessage -} - func rulesAreEqual(a, b netlink.Rule) bool { return ipPrefixesAreEqual(a.Src, b.Src) && ipPrefixesAreEqual(a.Dst, b.Dst) && diff --git a/internal/routing/rules_test.go b/internal/routing/rules_test.go index f139d0d6..1982e4b3 100644 --- a/internal/routing/rules_test.go +++ b/internal/routing/rules_test.go @@ -48,13 +48,11 @@ func Test_Routing_addIPRule(t *testing.T) { dst netip.Prefix table int priority int - dbgMsg string ruleList ruleListCall ruleAdd ruleAddCall err error }{ "list error": { - dbgMsg: "ip rule add pref 0", ruleList: ruleListCall{ err: errDummy, }, @@ -65,7 +63,6 @@ func Test_Routing_addIPRule(t *testing.T) { dst: makeNetipPrefix(2), table: 99, priority: 99, - dbgMsg: "ip rule add from 1.1.1.0/24 to 2.2.2.0/24 lookup 99 pref 99", ruleList: ruleListCall{ rules: []netlink.Rule{ makeIPRule(makeNetipPrefix(2), makeNetipPrefix(2), 99, 99), @@ -78,7 +75,6 @@ func Test_Routing_addIPRule(t *testing.T) { dst: makeNetipPrefix(2), table: 99, priority: 99, - dbgMsg: "ip rule add from 1.1.1.0/24 to 2.2.2.0/24 lookup 99 pref 99", ruleAdd: ruleAddCall{ expected: true, ruleToAdd: makeIPRule(makeNetipPrefix(1), makeNetipPrefix(2), 99, 99), @@ -91,7 +87,6 @@ func Test_Routing_addIPRule(t *testing.T) { dst: makeNetipPrefix(2), table: 99, priority: 99, - dbgMsg: "ip rule add from 1.1.1.0/24 to 2.2.2.0/24 lookup 99 pref 99", ruleList: ruleListCall{ rules: []netlink.Rule{ makeIPRule(makeNetipPrefix(2), makeNetipPrefix(2), 99, 99), @@ -110,9 +105,6 @@ func Test_Routing_addIPRule(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) - logger := NewMockLogger(ctrl) - logger.EXPECT().Debug(testCase.dbgMsg) - netLinker := NewMockNetLinker(ctrl) netLinker.EXPECT().RuleList(netlink.FamilyAll). Return(testCase.ruleList.rules, testCase.ruleList.err) @@ -122,7 +114,6 @@ func Test_Routing_addIPRule(t *testing.T) { } r := Routing{ - logger: logger, netLinker: netLinker, } @@ -160,13 +151,11 @@ func Test_Routing_deleteIPRule(t *testing.T) { dst netip.Prefix table int priority int - dbgMsg string ruleList ruleListCall ruleDel ruleDelCall err error }{ "list error": { - dbgMsg: "ip rule del pref 0", ruleList: ruleListCall{ err: errDummy, }, @@ -177,7 +166,6 @@ func Test_Routing_deleteIPRule(t *testing.T) { dst: makeNetipPrefix(2), table: 99, priority: 99, - dbgMsg: "ip rule del from 1.1.1.0/24 to 2.2.2.0/24 lookup 99 pref 99", ruleList: ruleListCall{ rules: []netlink.Rule{ makeIPRule(makeNetipPrefix(1), makeNetipPrefix(2), 99, 99), @@ -195,7 +183,6 @@ func Test_Routing_deleteIPRule(t *testing.T) { dst: makeNetipPrefix(2), table: 99, priority: 99, - dbgMsg: "ip rule del from 1.1.1.0/24 to 2.2.2.0/24 lookup 99 pref 99", ruleList: ruleListCall{ rules: []netlink.Rule{ makeIPRule(makeNetipPrefix(2), makeNetipPrefix(2), 99, 99), @@ -212,7 +199,6 @@ func Test_Routing_deleteIPRule(t *testing.T) { dst: makeNetipPrefix(2), table: 99, priority: 99, - dbgMsg: "ip rule del from 1.1.1.0/24 to 2.2.2.0/24 lookup 99 pref 99", ruleList: ruleListCall{ rules: []netlink.Rule{ makeIPRule(makeNetipPrefix(2), makeNetipPrefix(2), 99, 99), @@ -227,9 +213,6 @@ func Test_Routing_deleteIPRule(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) - logger := NewMockLogger(ctrl) - logger.EXPECT().Debug(testCase.dbgMsg) - netLinker := NewMockNetLinker(ctrl) netLinker.EXPECT().RuleList(netlink.FamilyAll). Return(testCase.ruleList.rules, testCase.ruleList.err) @@ -239,7 +222,6 @@ func Test_Routing_deleteIPRule(t *testing.T) { } r := Routing{ - logger: logger, netLinker: netLinker, } @@ -256,49 +238,6 @@ func Test_Routing_deleteIPRule(t *testing.T) { } } -func Test_ruleDbgMsg(t *testing.T) { - t.Parallel() - - testCases := map[string]struct { - add bool - src netip.Prefix - dst netip.Prefix - table int - priority int - dbgMsg string - }{ - "default values": { - dbgMsg: "ip rule del pref 0", - }, - "add rule": { - add: true, - src: makeNetipPrefix(1), - dst: makeNetipPrefix(2), - table: 100, - priority: 101, - dbgMsg: "ip rule add from 1.1.1.0/24 to 2.2.2.0/24 lookup 100 pref 101", - }, - "del rule": { - src: makeNetipPrefix(1), - dst: makeNetipPrefix(2), - table: 100, - priority: 101, - dbgMsg: "ip rule del from 1.1.1.0/24 to 2.2.2.0/24 lookup 100 pref 101", - }, - } - - for name, testCase := range testCases { - t.Run(name, func(t *testing.T) { - t.Parallel() - - dbgMsg := ruleDbgMsg(testCase.add, testCase.src, - testCase.dst, testCase.table, testCase.priority) - - assert.Equal(t, testCase.dbgMsg, dbgMsg) - }) - } -} - func Test_rulesAreEqual(t *testing.T) { t.Parallel()