cmd/tailscale/cli: add bind-address and bind-port flags to netcheck command (#18621)

Add more explicit `--bind-address` and `--bind-port` flags to the `tailscale netcheck` CLI to give users control over UDP probes' source IP and UDP port.

This was already supported in a less documented manner via the` TS_DEBUG_NETCHECK_UDP_BIND` environment variable. The environment variable reference is preserved and used as a fallback value in the absence of these new CLI flags.

Updates tailscale/corp#36833

Signed-off-by: Amal Bansode <amal@tailscale.com>

cmd/tailscale/cli/netcheck.go

@@ -10,7 +10,9 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"math"
 	"net/http"
+	"net/netip"
 	"sort"
 	"strings"
 	"time"
@@ -26,6 +28,7 @@ import (
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/eventbus"
+	"tailscale.com/util/set"
 
 	// The "netcheck" command also wants the portmapper linked.
 	//
@@ -41,19 +44,25 @@ var netcheckCmd = &ffcli.Command{
 	ShortUsage: "tailscale netcheck",
 	ShortHelp:  "Print an analysis of local network conditions",
 	Exec:       runNetcheck,
-	FlagSet: (func() *flag.FlagSet {
-		fs := newFlagSet("netcheck")
-		fs.StringVar(&netcheckArgs.format, "format", "", `output format; empty (for human-readable), "json" or "json-line"`)
-		fs.DurationVar(&netcheckArgs.every, "every", 0, "if non-zero, do an incremental report with the given frequency")
-		fs.BoolVar(&netcheckArgs.verbose, "verbose", false, "verbose logs")
-		return fs
-	})(),
+	FlagSet:    netcheckFlagSet,
 }
 
+var netcheckFlagSet = func() *flag.FlagSet {
+	fs := newFlagSet("netcheck")
+	fs.StringVar(&netcheckArgs.format, "format", "", `output format; empty (for human-readable), "json" or "json-line"`)
+	fs.DurationVar(&netcheckArgs.every, "every", 0, "if non-zero, do an incremental report with the given frequency")
+	fs.BoolVar(&netcheckArgs.verbose, "verbose", false, "verbose logs")
+	fs.StringVar(&netcheckArgs.bindAddress, "bind-address", "", "send and receive connectivity probes using this locally bound IP address; default: OS-assigned")
+	fs.IntVar(&netcheckArgs.bindPort, "bind-port", 0, "send and receive connectivity probes using this UDP port; default: OS-assigned")
+	return fs
+}()
+
 var netcheckArgs struct {
-	format  string
-	every   time.Duration
-	verbose bool
+	format      string
+	every       time.Duration
+	verbose     bool
+	bindAddress string
+	bindPort    int
 }
 
 func runNetcheck(ctx context.Context, args []string) error {
@@ -73,6 +82,11 @@ func runNetcheck(ctx context.Context, args []string) error {
 		defer pm.Close()
 	}
 
+	flagsProvided := set.Set[string]{}
+	netcheckFlagSet.Visit(func(f *flag.Flag) {
+		flagsProvided.Add(f.Name)
+	})
+
 	c := &netcheck.Client{
 		NetMon:      netMon,
 		PortMapper:  pm,
@@ -89,7 +103,17 @@ func runNetcheck(ctx context.Context, args []string) error {
 		fmt.Fprintln(Stderr, "# Warning: this JSON format is not yet considered a stable interface")
 	}
 
-	if err := c.Standalone(ctx, envknob.String("TS_DEBUG_NETCHECK_UDP_BIND")); err != nil {
+	bind, err := createNetcheckBindString(
+		netcheckArgs.bindAddress,
+		flagsProvided.Contains("bind-address"),
+		netcheckArgs.bindPort,
+		flagsProvided.Contains("bind-port"),
+		envknob.String("TS_DEBUG_NETCHECK_UDP_BIND"))
+	if err != nil {
+		return err
+	}
+
+	if err := c.Standalone(ctx, bind); err != nil {
 		fmt.Fprintln(Stderr, "netcheck: UDP test failure:", err)
 	}
 
@@ -265,3 +289,44 @@ func prodDERPMap(ctx context.Context, httpc *http.Client) (*tailcfg.DERPMap, err
 	}
 	return &derpMap, nil
 }
+
+// createNetcheckBindString determines the netcheck socket bind "address:port" string based
+// on the CLI args and environment variable values used to invoke the netcheck CLI.
+// Arguments cliAddressIsSet and cliPortIsSet explicitly indicate whether the
+// corresponding cliAddress and cliPort were set in CLI args, instead of relying
+// on in-band sentinel values.
+func createNetcheckBindString(cliAddress string, cliAddressIsSet bool, cliPort int, cliPortIsSet bool, envBind string) (string, error) {
+	// Default to port number 0 but overwrite with a valid CLI value, if set.
+	var port uint16 = 0
+	if cliPortIsSet {
+		// 0 is valid, results in OS picking port.
+		if cliPort >= 0 && cliPort <= math.MaxUint16 {
+			port = uint16(cliPort)
+		} else {
+			return "", fmt.Errorf("invalid bind port number: %d", cliPort)
+		}
+	}
+
+	// Use CLI address, if set.
+	if cliAddressIsSet {
+		addr, err := netip.ParseAddr(cliAddress)
+		if err != nil {
+			return "", fmt.Errorf("invalid bind address: %q", cliAddress)
+		}
+		return netip.AddrPortFrom(addr, port).String(), nil
+	} else {
+		// No CLI address set, but port is set.
+		if cliPortIsSet {
+			return fmt.Sprintf(":%d", port), nil
+		}
+	}
+
+	// Fall back to the environment variable.
+	// Intentionally skipping input validation here to avoid breaking legacy usage method.
+	if envBind != "" {
+		return envBind, nil
+	}
+
+	// OS picks both address and port.
+	return ":0", nil
+}

cmd/tailscale/cli/netcheck_test.go

@@ -0,0 +1,108 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+package cli
+
+import (
+	"testing"
+)
+
+func TestCreateBindStr(t *testing.T) {
+	// Test all combinations of CLI arg address, CLI arg port, and env var string
+	// as inputs to create netcheck bind string.
+	tests := []struct {
+		name            string
+		cliAddress      string
+		cliAddressIsSet bool
+		cliPort         int
+		cliPortIsSet    bool
+		envBind         string
+		want            string
+		wantError       string
+	}{
+		{
+			name: "noAddr-noPort-noEnv",
+			want: ":0",
+		},
+		{
+			name:            "yesAddrv4-noPort-noEnv",
+			cliAddress:      "100.123.123.123",
+			cliAddressIsSet: true,
+			want:            "100.123.123.123:0",
+		},
+		{
+			name:            "yesAddrv6-noPort-noEnv",
+			cliAddress:      "dead::beef",
+			cliAddressIsSet: true,
+			want:            "[dead::beef]:0",
+		},
+		{
+			name:            "yesAddr-yesPort-noEnv",
+			cliAddress:      "100.123.123.123",
+			cliAddressIsSet: true,
+			cliPort:         456,
+			cliPortIsSet:    true,
+			want:            "100.123.123.123:456",
+		},
+		{
+			name:            "yesAddr-yesPort-yesEnv",
+			cliAddress:      "100.123.123.123",
+			cliAddressIsSet: true,
+			cliPort:         456,
+			cliPortIsSet:    true,
+			envBind:         "55.55.55.55:789",
+			want:            "100.123.123.123:456",
+		},
+		{
+			name:         "noAddr-yesPort-noEnv",
+			cliPort:      456,
+			cliPortIsSet: true,
+			want:         ":456",
+		},
+		{
+			name:         "noAddr-yesPort-yesEnv",
+			cliPort:      456,
+			cliPortIsSet: true,
+			envBind:      "55.55.55.55:789",
+			want:         ":456",
+		},
+		{
+			name:    "noAddr-noPort-yesEnv",
+			envBind: "55.55.55.55:789",
+			want:    "55.55.55.55:789",
+		},
+		{
+			name:            "badAddr-noPort-noEnv-1",
+			cliAddress:      "678.678.678.678",
+			cliAddressIsSet: true,
+			wantError:       `invalid bind address: "678.678.678.678"`,
+		},
+		{
+			name:            "badAddr-noPort-noEnv-2",
+			cliAddress:      "lorem ipsum",
+			cliAddressIsSet: true,
+			wantError:       `invalid bind address: "lorem ipsum"`,
+		},
+		{
+			name:         "noAddr-badPort-noEnv",
+			cliPort:      -1,
+			cliPortIsSet: true,
+			wantError:    "invalid bind port number: -1",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, gotErr := createNetcheckBindString(tt.cliAddress, tt.cliAddressIsSet, tt.cliPort, tt.cliPortIsSet, tt.envBind)
+			var gotErrStr string
+			if gotErr != nil {
+				gotErrStr = gotErr.Error()
+			}
+			if gotErrStr != tt.wantError {
+				t.Errorf("got error %q; want error %q", gotErrStr, tt.wantError)
+			}
+			if got != tt.want {
+				t.Errorf("got result %q; want result %q", got, tt.want)
+			}
+		})
+	}
+}