From add62af7c6031d174e5e5747025441d0e5c856d4 Mon Sep 17 00:00:00 2001 From: Irbe Krumina Date: Tue, 23 Apr 2024 21:08:18 +0100 Subject: [PATCH] util/linuxfw,go.{mod,sum}: don't log errors when deleting non-existant chains and rules (#11852) This PR bumps iptables to a newer version that has a function to detect 'NotExists' errors and uses that function to determine whether errors received on iptables rule and chain clean up are because the rule/chain does not exist- if so don't log the error. Updates corp#19336 Signed-off-by: Irbe Krumina --- go.mod | 2 +- go.sum | 4 ++-- util/linuxfw/iptables_runner.go | 31 ++++++++++++++-------------- util/linuxfw/iptables_runner_test.go | 6 ++++++ util/linuxfw/linuxfw.go | 21 ------------------- 5 files changed, 24 insertions(+), 40 deletions(-) diff --git a/go.mod b/go.mod index 303296c3b..fb0e42bb5 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.11.64 github.com/aws/aws-sdk-go-v2/service/s3 v1.33.0 github.com/aws/aws-sdk-go-v2/service/ssm v1.44.7 - github.com/coreos/go-iptables v0.7.0 + github.com/coreos/go-iptables v0.7.1-0.20240112124308-65c67c9f46e6 github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf github.com/coreos/go-systemd/v22 v22.5.0 github.com/creack/pty v1.1.21 diff --git a/go.sum b/go.sum index dd310c7ea..f5e907b85 100644 --- a/go.sum +++ b/go.sum @@ -213,8 +213,8 @@ github.com/cncf/udpa/go v0.0.0-20200629203442-efcf912fb354/go.mod h1:WmhPx2Nbnht github.com/cncf/udpa/go v0.0.0-20201120205902-5459f2c99403/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk= github.com/containerd/stargz-snapshotter/estargz v0.15.1 h1:eXJjw9RbkLFgioVaTG+G/ZW/0kEe2oEKCdS/ZxIyoCU= github.com/containerd/stargz-snapshotter/estargz v0.15.1/go.mod h1:gr2RNwukQ/S9Nv33Lt6UC7xEx58C+LHRdoqbEKjz1Kk= -github.com/coreos/go-iptables v0.7.0 h1:XWM3V+MPRr5/q51NuWSgU0fqMad64Zyxs8ZUoMsamr8= -github.com/coreos/go-iptables v0.7.0/go.mod h1:Qe8Bv2Xik5FyTXwgIbLAnv2sWSBmvWdFETJConOQ//Q= +github.com/coreos/go-iptables v0.7.1-0.20240112124308-65c67c9f46e6 h1:8h5+bWd7R6AYUslN6c6iuZWTKsKxUFDlpnmilO6R2n0= +github.com/coreos/go-iptables v0.7.1-0.20240112124308-65c67c9f46e6/go.mod h1:Qe8Bv2Xik5FyTXwgIbLAnv2sWSBmvWdFETJConOQ//Q= github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf h1:iW4rZ826su+pqaw19uhpSCzhj44qo35pNgKFGqzDKkU= github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= github.com/coreos/go-systemd/v22 v22.5.0 h1:RrqgGjYQKalulkV8NGVIfkXQf6YYmOyiJKk8iXXhfZs= diff --git a/util/linuxfw/iptables_runner.go b/util/linuxfw/iptables_runner.go index 1c296a25a..237bc0870 100644 --- a/util/linuxfw/iptables_runner.go +++ b/util/linuxfw/iptables_runner.go @@ -7,6 +7,7 @@ import ( "bytes" + "errors" "fmt" "net/netip" "os" @@ -20,6 +21,14 @@ "tailscale.com/util/multierr" ) +// isNotExistError needs to be overridden in tests that rely on distinguishing +// this error, because we don't have a good way how to create a new +// iptables.Error of that type. +var isNotExistError = func(err error) bool { + var e *iptables.Error + return errors.As(err, &e) && e.IsNotExist() +} + type iptablesInterface interface { // Adding this interface for testing purposes so we can mock out // the iptables library, in reality this is a wrapper to *iptables.IPTables. @@ -156,10 +165,6 @@ func (i *iptablesRunner) HasIPV6NAT() bool { return i.v6NATAvailable } -func isErrChainNotExist(err error) bool { - return errCode(err) == 1 -} - // getIPTByAddr returns the iptablesInterface with correct IP family // that we will be using for the given address. func (i *iptablesRunner) getIPTByAddr(addr netip.Addr) iptablesInterface { @@ -261,7 +266,7 @@ func (i *iptablesRunner) AddChains() error { // If the chain already exists, it is a no-op. create := func(ipt iptablesInterface, table, chain string) error { err := ipt.ClearChain(table, chain) - if isErrChainNotExist(err) { + if isNotExistError(err) { // nonexistent chain. let's create it! return ipt.NewChain(table, chain) } @@ -377,7 +382,7 @@ func (i *iptablesRunner) DNATNonTailscaleTraffic(tun string, dst netip.Addr) err // originDst to the backend dsts. Traffic will be load balanced using round robin. func (i *iptablesRunner) DNATWithLoadBalancer(origDst netip.Addr, dsts []netip.Addr) error { table := i.getIPTByAddr(dsts[0]) - if err := table.ClearChain("nat", "PREROUTING"); err != nil && !isErrChainNotExist(err) { + if err := table.ClearChain("nat", "PREROUTING"); err != nil && !isNotExistError(err) { // If clearing the PREROUTING chain fails, fail the whole operation. This // rule is currently only used in Kubernetes containers where a // failed container gets restarted which should hopefully fix things. @@ -454,7 +459,7 @@ func (i *iptablesRunner) DelChains() error { func (i *iptablesRunner) DelBase() error { del := func(ipt iptablesInterface, table, chain string) error { if err := ipt.ClearChain(table, chain); err != nil { - if isErrChainNotExist(err) { + if isNotExistError(err) { // nonexistent chain. That's fine, since it's // the desired state anyway. return nil @@ -602,14 +607,8 @@ func IPTablesCleanUp(logf logger.Logf) { func delTSHook(ipt iptablesInterface, table, chain string, logf logger.Logf) error { tsChain := tsChain(chain) args := []string{"-j", tsChain} - if err := ipt.Delete(table, chain, args...); err != nil { - // TODO(apenwarr): check for errCode(1) here. - // Unfortunately the error code from the iptables - // module resists unwrapping, unlike with other - // calls. So we have to assume if Delete fails, - // it's because there is no such rule. - logf("deleting %v in %s/%s: %v", args, table, chain, err) - return nil + if err := ipt.Delete(table, chain, args...); err != nil && !isNotExistError(err) { + return fmt.Errorf("deleting %v in %s/%s: %v", args, table, chain, err) } return nil } @@ -618,7 +617,7 @@ func delTSHook(ipt iptablesInterface, table, chain string, logf logger.Logf) err // since the desired state is already achieved. otherwise, it returns an error. func delChain(ipt iptablesInterface, table, chain string) error { if err := ipt.ClearChain(table, chain); err != nil { - if isErrChainNotExist(err) { + if isNotExistError(err) { // nonexistent chain. nothing to do. return nil } diff --git a/util/linuxfw/iptables_runner_test.go b/util/linuxfw/iptables_runner_test.go index e678ddd6d..2363e4ed3 100644 --- a/util/linuxfw/iptables_runner_test.go +++ b/util/linuxfw/iptables_runner_test.go @@ -13,6 +13,12 @@ "tailscale.com/net/tsaddr" ) +var testIsNotExistErr = "exitcode:1" + +func init() { + isNotExistError = func(e error) bool { return e.Error() == testIsNotExistErr } +} + func TestAddAndDeleteChains(t *testing.T) { iptr := NewFakeIPTablesRunner() err := iptr.AddChains() diff --git a/util/linuxfw/linuxfw.go b/util/linuxfw/linuxfw.go index e4b1929cf..0f47328f9 100644 --- a/util/linuxfw/linuxfw.go +++ b/util/linuxfw/linuxfw.go @@ -11,7 +11,6 @@ "errors" "fmt" "os" - "os/exec" "strconv" "strings" @@ -105,26 +104,6 @@ func getTailscaleSubnetRouteMark() []byte { return []byte{0x00, 0x04, 0x00, 0x00} } -// errCode extracts and returns the process exit code from err, or -// zero if err is nil. -func errCode(err error) int { - if err == nil { - return 0 - } - var e *exec.ExitError - if ok := errors.As(err, &e); ok { - return e.ExitCode() - } - s := err.Error() - if strings.HasPrefix(s, "exitcode:") { - code, err := strconv.Atoi(s[9:]) - if err == nil { - return code - } - } - return -42 -} - // checkIPv6 checks whether the system appears to have a working IPv6 // network stack. It returns an error explaining what looks wrong or // missing. It does not check that IPv6 is currently functional or