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

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,
+	}
+}

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=<true|false>, 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)

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)
+			}
+		})
+	}
+}