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 <irbe@tailscale.com>

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

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=

util/linuxfw/iptables_runner.go

@@ -7,6 +7,7 @@ package linuxfw
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"net/netip"
 	"os"
@@ -20,6 +21,14 @@ import (
 	"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
 		}

util/linuxfw/iptables_runner_test.go

@@ -13,6 +13,12 @@ import (
 	"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()

util/linuxfw/linuxfw.go

@@ -11,7 +11,6 @@ import (
 	"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