From efc1feedc9df924fd13b250fb8151f04604eb1fb Mon Sep 17 00:00:00 2001 From: David Anderson Date: Sun, 10 May 2020 22:13:09 +0000 Subject: [PATCH] wgengine/router: include more information when iptables ops fail. The iptables package we use doesn't include command output, so we're left with guessing what went wrong most of the time. This will at least narrow things down to which operation failed. Signed-off-by: David Anderson --- wgengine/router/router_linux.go | 61 +++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/wgengine/router/router_linux.go b/wgengine/router/router_linux.go index fb84c42f5..52fa4ce2c 100644 --- a/wgengine/router/router_linux.go +++ b/wgengine/router/router_linux.go @@ -343,10 +343,10 @@ func (r *linuxRouter) delRoute(cidr netaddr.IPPrefix) error { // adding the route fails. func (r *linuxRouter) addSubnetRule(cidr netaddr.IPPrefix) error { if err := r.ipt4.Insert("filter", "ts-forward", 1, "-i", r.tunname, "-d", normalizeCIDR(cidr), "-j", "MARK", "--set-mark", tailscaleSubnetRouteMark); err != nil { - return err + return fmt.Errorf("adding subnet mark rule for %q: %v", cidr, err) } if err := r.ipt4.Insert("filter", "ts-forward", 1, "-o", r.tunname, "-s", normalizeCIDR(cidr), "-j", "ACCEPT"); err != nil { - return err + return fmt.Errorf("adding subnet forward rule for %q: %v", cidr, err) } return nil } @@ -356,10 +356,10 @@ func (r *linuxRouter) addSubnetRule(cidr netaddr.IPPrefix) error { // fails. func (r *linuxRouter) delSubnetRule(cidr netaddr.IPPrefix) error { if err := r.ipt4.Delete("filter", "ts-forward", "-i", r.tunname, "-d", normalizeCIDR(cidr), "-j", "MARK", "--set-mark", tailscaleSubnetRouteMark); err != nil { - return err + return fmt.Errorf("deleting subnet mark rule for %q: %v", cidr, err) } if err := r.ipt4.Delete("filter", "ts-forward", "-o", r.tunname, "-s", normalizeCIDR(cidr), "-j", "ACCEPT"); err != nil { - return err + return fmt.Errorf("deleting subnet forward rule for %q: %v", cidr, err) } return nil } @@ -411,11 +411,11 @@ func (r *linuxRouter) deleteLegacyNetfilter() error { del := func(table, chain string, args ...string) error { exists, err := r.ipt4.Exists(table, chain, args...) if err != nil { - return err + return fmt.Errorf("checking for %v in %s/%s: %v", args, table, chain, err) } if exists { if err := r.ipt4.Delete(table, chain, args...); err != nil { - return err + return fmt.Errorf("deleting %v in %s/%s: %v", args, table, chain, err) } } return nil @@ -437,24 +437,25 @@ func (r *linuxRouter) delNetfilter4() error { del := func(table, chain string) error { tsChain := "ts-" + strings.ToLower(chain) - exists, err := r.ipt4.Exists(table, chain, "-j", tsChain) + args := []string{"-j", tsChain} + exists, err := r.ipt4.Exists(table, chain, args...) if err != nil { - return err + return fmt.Errorf("checking for %v in %s/%s: %v", args, table, chain, err) } if exists { if err := r.ipt4.Delete(table, chain, "-j", tsChain); err != nil { - return err + return fmt.Errorf("deleting %v in %s/%s: %v", args, table, chain, err) } } chains, err := r.ipt4.ListChains(table) if err != nil { - return err + return fmt.Errorf("listing iptables chains: %v", err) } for _, chain := range chains { if chain == tsChain { if err := r.ipt4.DeleteChain(table, tsChain); err != nil { - return err + return fmt.Errorf("deleting %s/%s: %v", table, tsChain, err) } break } @@ -493,7 +494,7 @@ func (r *linuxRouter) addBaseNetfilter4() error { chains, err := r.ipt4.ListChains(table) if err != nil { - return err + return fmt.Errorf("listing iptables chains: %v", err) } found := false for _, chain := range chains { @@ -508,16 +509,19 @@ func (r *linuxRouter) addBaseNetfilter4() error { err = r.ipt4.NewChain(table, tsChain) } if err != nil { - return err + return fmt.Errorf("setting up %s/%s: %v", table, tsChain, err) } args := []string{"-j", tsChain} exists, err := r.ipt4.Exists(table, chain, args...) if err != nil { - return err + return fmt.Errorf("checking for %v in %s/%s: %v", args, table, chain, err) + } + if exists { + return nil } - if !exists { - return r.ipt4.Insert(table, chain, 1, args...) + if err := r.ipt4.Insert(table, chain, 1, args...); err != nil { + return fmt.Errorf("adding %v in %s/%s: %v", args, table, chain, err) } return nil } @@ -537,11 +541,13 @@ func (r *linuxRouter) addBaseNetfilter4() error { // // Note, this will definitely break nodes that end up using the // CGNAT range for other purposes :(. - if err := r.ipt4.Append("filter", "ts-input", "!", "-i", r.tunname, "-s", chromeOSVMRange, "-m", "comment", "--comment", "ChromeOS VM connectivity", "-j", "RETURN"); err != nil { - return err + args := []string{"!", "-i", r.tunname, "-s", chromeOSVMRange, "-m", "comment", "--comment", "ChromeOS VM connectivity", "-j", "RETURN"} + if err := r.ipt4.Append("filter", "ts-input", args...); err != nil { + return fmt.Errorf("adding %v in filter/ts-input: %v", args, err) } - if err := r.ipt4.Append("filter", "ts-input", "!", "-i", r.tunname, "-s", "100.64.0.0/10", "-j", "DROP"); err != nil { - return err + args = []string{"!", "-i", r.tunname, "-s", "100.64.0.0/10", "-j", "DROP"} + if err := r.ipt4.Append("filter", "ts-input", args...); err != nil { + return fmt.Errorf("adding %v in filter/ts-input: %v", args, err) } // Forward and masquerade packets that have the Tailscale subnet @@ -557,15 +563,18 @@ func (r *linuxRouter) addBaseNetfilter4() error { // destination IP in filter/FORWARD, and set a packet mark that // nat/POSTROUTING can use to effectively run that same test // again. - if err := r.ipt4.Append("filter", "ts-forward", "-m", "mark", "--mark", tailscaleSubnetRouteMark, "-j", "ACCEPT"); err != nil { - return err + args = []string{"-m", "mark", "--mark", tailscaleSubnetRouteMark, "-j", "ACCEPT"} + if err := r.ipt4.Append("filter", "ts-forward", args...); err != nil { + return fmt.Errorf("adding %v in filter/ts-forward: %v", args, err) } - if err := r.ipt4.Append("filter", "ts-forward", "-i", r.tunname, "-j", "DROP"); err != nil { - return err + args = []string{"-i", r.tunname, "-j", "DROP"} + if err := r.ipt4.Append("filter", "ts-forward", args...); err != nil { + return fmt.Errorf("adding %v in filter/ts-forward: %v", args, err) } // TODO(danderson): this should be optional. - if err := r.ipt4.Append("nat", "ts-postrouting", "-m", "mark", "--mark", tailscaleSubnetRouteMark, "-j", "MASQUERADE"); err != nil { - return err + args = []string{"-m", "mark", "--mark", tailscaleSubnetRouteMark, "-j", "MASQUERADE"} + if err := r.ipt4.Append("nat", "ts-postrouting", args...); err != nil { + return fmt.Errorf("adding %v in nat/ts-postrouting: %v", args, err) } return nil