From 5b670eb3a5f1749a655692d97a2e7086c78d1580 Mon Sep 17 00:00:00 2001 From: Irbe Krumina Date: Fri, 30 May 2025 11:30:03 +0100 Subject: [PATCH] cmd/containerboot: allow setting --accept-dns via TS_EXTRA_ARGS again (#16129) In 1.84 we made 'tailscale set'/'tailscale up' error out if duplicate command line flags are passed. This broke some container configurations as we have two env vars that can be used to set --accept-dns flag: - TS_ACCEPT_DNS- specifically for --accept-dns - TS_EXTRA_ARGS- accepts any arbitrary 'tailscale up'/'tailscale set' flag. We default TS_ACCEPT_DNS to false (to make the container behaviour more declarative), which with the new restrictive CLI behaviour resulted in failure for users who had set --accept-dns via TS_EXTRA_ARGS as the flag would be provided twice. This PR re-instates the previous behaviour by checking if TS_EXTRA_ARGS contains --accept-dns flag and if so using its value to override TS_ACCEPT_DNS. Updates tailscale/tailscale#16108 Signed-off-by: Irbe Krumina --- cmd/containerboot/main_test.go | 248 ++++++++++++++++++----------- cmd/containerboot/settings.go | 57 +++++++ cmd/containerboot/settings_test.go | 108 +++++++++++++ 3 files changed, 322 insertions(+), 91 deletions(-) create mode 100644 cmd/containerboot/settings_test.go diff --git a/cmd/containerboot/main_test.go b/cmd/containerboot/main_test.go index a0ccce3dd..c7293c77a 100644 --- a/cmd/containerboot/main_test.go +++ b/cmd/containerboot/main_test.go @@ -41,97 +41,6 @@ import ( "tailscale.com/types/ptr" ) -// testEnv represents the environment needed for a single sub-test so that tests -// can run in parallel. -type testEnv struct { - kube *kubeServer // Fake kube server. - lapi *localAPI // Local TS API server. - d string // Temp dir for the specific test. - argFile string // File with commands test_tailscale{,d}.sh were invoked with. - runningSockPath string // Path to the running tailscaled socket. - localAddrPort int // Port for the containerboot HTTP server. - healthAddrPort int // Port for the (deprecated) containerboot health server. -} - -func newTestEnv(t *testing.T) testEnv { - d := t.TempDir() - - lapi := localAPI{FSRoot: d} - if err := lapi.Start(); err != nil { - t.Fatal(err) - } - t.Cleanup(lapi.Close) - - kube := kubeServer{FSRoot: d} - kube.Start(t) - t.Cleanup(kube.Close) - - tailscaledConf := &ipn.ConfigVAlpha{AuthKey: ptr.To("foo"), Version: "alpha0"} - serveConf := ipn.ServeConfig{TCP: map[uint16]*ipn.TCPPortHandler{80: {HTTP: true}}} - egressCfg := egressSvcConfig("foo", "foo.tailnetxyz.ts.net") - - dirs := []string{ - "var/lib", - "usr/bin", - "tmp", - "dev/net", - "proc/sys/net/ipv4", - "proc/sys/net/ipv6/conf/all", - "etc/tailscaled", - } - for _, path := range dirs { - if err := os.MkdirAll(filepath.Join(d, path), 0700); err != nil { - t.Fatal(err) - } - } - files := map[string][]byte{ - "usr/bin/tailscaled": fakeTailscaled, - "usr/bin/tailscale": fakeTailscale, - "usr/bin/iptables": fakeTailscale, - "usr/bin/ip6tables": fakeTailscale, - "dev/net/tun": []byte(""), - "proc/sys/net/ipv4/ip_forward": []byte("0"), - "proc/sys/net/ipv6/conf/all/forwarding": []byte("0"), - "etc/tailscaled/cap-95.hujson": mustJSON(t, tailscaledConf), - "etc/tailscaled/serve-config.json": mustJSON(t, serveConf), - filepath.Join("etc/tailscaled/", egressservices.KeyEgressServices): mustJSON(t, egressCfg), - filepath.Join("etc/tailscaled/", egressservices.KeyHEPPings): []byte("4"), - } - for path, content := range files { - // Making everything executable is a little weird, but the - // stuff that doesn't need to be executable doesn't care if we - // do make it executable. - if err := os.WriteFile(filepath.Join(d, path), content, 0700); err != nil { - t.Fatal(err) - } - } - - argFile := filepath.Join(d, "args") - runningSockPath := filepath.Join(d, "tmp/tailscaled.sock") - var localAddrPort, healthAddrPort int - for _, p := range []*int{&localAddrPort, &healthAddrPort} { - ln, err := net.Listen("tcp", ":0") - if err != nil { - t.Fatalf("Failed to open listener: %v", err) - } - if err := ln.Close(); err != nil { - t.Fatalf("Failed to close listener: %v", err) - } - port := ln.Addr().(*net.TCPAddr).Port - *p = port - } - - return testEnv{ - kube: &kube, - lapi: &lapi, - d: d, - argFile: argFile, - runningSockPath: runningSockPath, - localAddrPort: localAddrPort, - healthAddrPort: healthAddrPort, - } -} - func TestContainerBoot(t *testing.T) { boot := filepath.Join(t.TempDir(), "containerboot") if err := exec.Command("go", "build", "-ldflags", "-X main.testSleepDuration=1ms", "-o", boot, "tailscale.com/cmd/containerboot").Run(); err != nil { @@ -515,6 +424,37 @@ func TestContainerBoot(t *testing.T) { }, } }, + "auth_key_once_extra_args_override_dns": func(env *testEnv) testCase { + return testCase{ + Env: map[string]string{ + "TS_AUTHKEY": "tskey-key", + "TS_AUTH_ONCE": "true", + "TS_ACCEPT_DNS": "false", + "TS_EXTRA_ARGS": "--accept-dns", + }, + Phases: []phase{ + { + WantCmds: []string{ + "/usr/bin/tailscaled --socket=/tmp/tailscaled.sock --state=mem: --statedir=/tmp --tun=userspace-networking", + }, + }, + { + Notify: &ipn.Notify{ + State: ptr.To(ipn.NeedsLogin), + }, + WantCmds: []string{ + "/usr/bin/tailscale --socket=/tmp/tailscaled.sock up --accept-dns=true --authkey=tskey-key", + }, + }, + { + Notify: runningNotify, + WantCmds: []string{ + "/usr/bin/tailscale --socket=/tmp/tailscaled.sock set --accept-dns=true", + }, + }, + }, + } + }, "kube_storage": func(env *testEnv) testCase { return testCase{ Env: map[string]string{ @@ -766,6 +706,41 @@ func TestContainerBoot(t *testing.T) { }, } }, + "extra_args_accept_dns": func(env *testEnv) testCase { + return testCase{ + Env: map[string]string{ + "TS_EXTRA_ARGS": "--accept-dns", + }, + Phases: []phase{ + { + WantCmds: []string{ + "/usr/bin/tailscaled --socket=/tmp/tailscaled.sock --state=mem: --statedir=/tmp --tun=userspace-networking", + "/usr/bin/tailscale --socket=/tmp/tailscaled.sock up --accept-dns=true", + }, + }, { + Notify: runningNotify, + }, + }, + } + }, + "extra_args_accept_dns_overrides_env_var": func(env *testEnv) testCase { + return testCase{ + Env: map[string]string{ + "TS_ACCEPT_DNS": "true", // Overridden by TS_EXTRA_ARGS. + "TS_EXTRA_ARGS": "--accept-dns=false", + }, + Phases: []phase{ + { + WantCmds: []string{ + "/usr/bin/tailscaled --socket=/tmp/tailscaled.sock --state=mem: --statedir=/tmp --tun=userspace-networking", + "/usr/bin/tailscale --socket=/tmp/tailscaled.sock up --accept-dns=false", + }, + }, { + Notify: runningNotify, + }, + }, + } + }, "hostname": func(env *testEnv) testCase { return testCase{ Env: map[string]string{ @@ -1604,3 +1579,94 @@ func egressSvcConfig(name, fqdn string) egressservices.Configs { }, } } + +// testEnv represents the environment needed for a single sub-test so that tests +// can run in parallel. +type testEnv struct { + kube *kubeServer // Fake kube server. + lapi *localAPI // Local TS API server. + d string // Temp dir for the specific test. + argFile string // File with commands test_tailscale{,d}.sh were invoked with. + runningSockPath string // Path to the running tailscaled socket. + localAddrPort int // Port for the containerboot HTTP server. + healthAddrPort int // Port for the (deprecated) containerboot health server. +} + +func newTestEnv(t *testing.T) testEnv { + d := t.TempDir() + + lapi := localAPI{FSRoot: d} + if err := lapi.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(lapi.Close) + + kube := kubeServer{FSRoot: d} + kube.Start(t) + t.Cleanup(kube.Close) + + tailscaledConf := &ipn.ConfigVAlpha{AuthKey: ptr.To("foo"), Version: "alpha0"} + serveConf := ipn.ServeConfig{TCP: map[uint16]*ipn.TCPPortHandler{80: {HTTP: true}}} + egressCfg := egressSvcConfig("foo", "foo.tailnetxyz.ts.net") + + dirs := []string{ + "var/lib", + "usr/bin", + "tmp", + "dev/net", + "proc/sys/net/ipv4", + "proc/sys/net/ipv6/conf/all", + "etc/tailscaled", + } + for _, path := range dirs { + if err := os.MkdirAll(filepath.Join(d, path), 0700); err != nil { + t.Fatal(err) + } + } + files := map[string][]byte{ + "usr/bin/tailscaled": fakeTailscaled, + "usr/bin/tailscale": fakeTailscale, + "usr/bin/iptables": fakeTailscale, + "usr/bin/ip6tables": fakeTailscale, + "dev/net/tun": []byte(""), + "proc/sys/net/ipv4/ip_forward": []byte("0"), + "proc/sys/net/ipv6/conf/all/forwarding": []byte("0"), + "etc/tailscaled/cap-95.hujson": mustJSON(t, tailscaledConf), + "etc/tailscaled/serve-config.json": mustJSON(t, serveConf), + filepath.Join("etc/tailscaled/", egressservices.KeyEgressServices): mustJSON(t, egressCfg), + filepath.Join("etc/tailscaled/", egressservices.KeyHEPPings): []byte("4"), + } + for path, content := range files { + // Making everything executable is a little weird, but the + // stuff that doesn't need to be executable doesn't care if we + // do make it executable. + if err := os.WriteFile(filepath.Join(d, path), content, 0700); err != nil { + t.Fatal(err) + } + } + + argFile := filepath.Join(d, "args") + runningSockPath := filepath.Join(d, "tmp/tailscaled.sock") + var localAddrPort, healthAddrPort int + for _, p := range []*int{&localAddrPort, &healthAddrPort} { + ln, err := net.Listen("tcp", ":0") + if err != nil { + t.Fatalf("Failed to open listener: %v", err) + } + if err := ln.Close(); err != nil { + t.Fatalf("Failed to close listener: %v", err) + } + port := ln.Addr().(*net.TCPAddr).Port + *p = port + } + + return testEnv{ + kube: &kube, + lapi: &lapi, + d: d, + argFile: argFile, + runningSockPath: runningSockPath, + localAddrPort: localAddrPort, + healthAddrPort: healthAddrPort, + } +} diff --git a/cmd/containerboot/settings.go b/cmd/containerboot/settings.go index 0ac9c828e..5a8be9036 100644 --- a/cmd/containerboot/settings.go +++ b/cmd/containerboot/settings.go @@ -147,12 +147,69 @@ func configFromEnv() (*settings, error) { } } + // See https://github.com/tailscale/tailscale/issues/16108 for context- we + // do this to preserve the previous behaviour where --accept-dns could be + // set either via TS_ACCEPT_DNS or TS_EXTRA_ARGS. + acceptDNS := cfg.AcceptDNS != nil && *cfg.AcceptDNS + tsExtraArgs, acceptDNSNew := parseAcceptDNS(cfg.ExtraArgs, acceptDNS) + cfg.ExtraArgs = tsExtraArgs + if acceptDNS != acceptDNSNew { + cfg.AcceptDNS = &acceptDNSNew + } + if err := cfg.validate(); err != nil { return nil, fmt.Errorf("invalid configuration: %v", err) } return cfg, nil } +// parseAcceptDNS parses any values for Tailscale --accept-dns flag set via +// TS_ACCEPT_DNS and TS_EXTRA_ARGS env vars. If TS_EXTRA_ARGS contains +// --accept-dns flag, override the acceptDNS value with the one from +// TS_EXTRA_ARGS. +// The value of extraArgs can be empty string or one or more whitespace-separate +// key value pairs for 'tailscale up' command. The value for boolean flags can +// be omitted (default to true). +func parseAcceptDNS(extraArgs string, acceptDNS bool) (string, bool) { + if !strings.Contains(extraArgs, "--accept-dns") { + return extraArgs, acceptDNS + } + // TODO(irbekrm): we should validate that TS_EXTRA_ARGS contains legit + // 'tailscale up' flag values separated by whitespace. + argsArr := strings.Fields(extraArgs) + i := -1 + for key, val := range argsArr { + if strings.HasPrefix(val, "--accept-dns") { + i = key + break + } + } + if i == -1 { + return extraArgs, acceptDNS + } + a := strings.TrimSpace(argsArr[i]) + var acceptDNSFromExtraArgsS string + keyval := strings.Split(a, "=") + if len(keyval) == 2 { + acceptDNSFromExtraArgsS = keyval[1] + } else if len(keyval) == 1 && keyval[0] == "--accept-dns" { + // If the arg is just --accept-dns, we assume it means true. + acceptDNSFromExtraArgsS = "true" + } else { + log.Printf("TS_EXTRA_ARGS contains --accept-dns, but it is not in the expected format --accept-dns=, ignoring it") + return extraArgs, acceptDNS + } + acceptDNSFromExtraArgs, err := strconv.ParseBool(acceptDNSFromExtraArgsS) + if err != nil { + log.Printf("TS_EXTRA_ARGS contains --accept-dns=%q, which is not a valid boolean value, ignoring it", acceptDNSFromExtraArgsS) + return extraArgs, acceptDNS + } + if acceptDNSFromExtraArgs != acceptDNS { + log.Printf("TS_EXTRA_ARGS contains --accept-dns=%v, which overrides TS_ACCEPT_DNS=%v", acceptDNSFromExtraArgs, acceptDNS) + } + return strings.Join(append(argsArr[:i], argsArr[i+1:]...), " "), acceptDNSFromExtraArgs +} + func (s *settings) validate() error { if s.TailscaledConfigFilePath != "" { dir, file := path.Split(s.TailscaledConfigFilePath) diff --git a/cmd/containerboot/settings_test.go b/cmd/containerboot/settings_test.go new file mode 100644 index 000000000..dbec066c9 --- /dev/null +++ b/cmd/containerboot/settings_test.go @@ -0,0 +1,108 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +//go:build linux + +package main + +import "testing" + +func Test_parseAcceptDNS(t *testing.T) { + tests := []struct { + name string + extraArgs string + acceptDNS bool + wantExtraArgs string + wantAcceptDNS bool + }{ + { + name: "false_extra_args_unset", + extraArgs: "", + wantExtraArgs: "", + wantAcceptDNS: false, + }, + { + name: "false_unrelated_args_set", + extraArgs: "--accept-routes=true --advertise-routes=10.0.0.1/32", + wantExtraArgs: "--accept-routes=true --advertise-routes=10.0.0.1/32", + wantAcceptDNS: false, + }, + { + name: "true_extra_args_unset", + extraArgs: "", + acceptDNS: true, + wantExtraArgs: "", + wantAcceptDNS: true, + }, + { + name: "true_unrelated_args_set", + acceptDNS: true, + extraArgs: "--accept-routes=true --advertise-routes=10.0.0.1/32", + wantExtraArgs: "--accept-routes=true --advertise-routes=10.0.0.1/32", + wantAcceptDNS: true, + }, + { + name: "false_extra_args_set_to_false", + extraArgs: "--accept-dns=false", + wantExtraArgs: "", + wantAcceptDNS: false, + }, + { + name: "false_extra_args_set_to_true", + extraArgs: "--accept-dns=true", + wantExtraArgs: "", + wantAcceptDNS: true, + }, + { + name: "true_extra_args_set_to_false", + extraArgs: "--accept-dns=false", + acceptDNS: true, + wantExtraArgs: "", + wantAcceptDNS: false, + }, + { + name: "true_extra_args_set_to_true", + extraArgs: "--accept-dns=true", + acceptDNS: true, + wantExtraArgs: "", + wantAcceptDNS: true, + }, + { + name: "false_extra_args_set_to_true_implicitly", + extraArgs: "--accept-dns", + wantExtraArgs: "", + wantAcceptDNS: true, + }, + { + name: "false_extra_args_set_to_true_implicitly_with_unrelated_args", + extraArgs: "--accept-dns --accept-routes --advertise-routes=10.0.0.1/32", + wantExtraArgs: "--accept-routes --advertise-routes=10.0.0.1/32", + wantAcceptDNS: true, + }, + { + name: "false_extra_args_set_to_true_implicitly_surrounded_with_unrelated_args", + extraArgs: "--accept-routes --accept-dns --advertise-routes=10.0.0.1/32", + wantExtraArgs: "--accept-routes --advertise-routes=10.0.0.1/32", + wantAcceptDNS: true, + }, + { + name: "true_extra_args_set_to_false_with_unrelated_args", + extraArgs: "--accept-routes --accept-dns=false", + acceptDNS: true, + wantExtraArgs: "--accept-routes", + wantAcceptDNS: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotExtraArgs, gotAcceptDNS := parseAcceptDNS(tt.extraArgs, tt.acceptDNS) + if gotExtraArgs != tt.wantExtraArgs { + t.Errorf("parseAcceptDNS() gotExtraArgs = %v, want %v", gotExtraArgs, tt.wantExtraArgs) + } + if gotAcceptDNS != tt.wantAcceptDNS { + t.Errorf("parseAcceptDNS() gotAcceptDNS = %v, want %v", gotAcceptDNS, tt.wantAcceptDNS) + } + }) + } +}