cmd/tailscale/cli: stop spamming os.Stdout/os.Stderr in tests

After:

    bradfitz@book1pro tailscale.com % ./tool/go test -c ./cmd/tailscale/cli
    bradfitz@book1pro tailscale.com % ./cli.test
    bradfitz@book1pro tailscale.com %

Before:

    bradfitz@book1pro tailscale.com % ./tool/go test -c ./cmd/tailscale/cli
    bradfitz@book1pro tailscale.com % ./cli.test

    Warning: funnel=on for foo.test.ts.net:443, but no serve config
             run: `tailscale serve --help` to see how to configure handlers

    Warning: funnel=on for foo.test.ts.net:443, but no serve config
             run: `tailscale serve --help` to see how to configure handlers
    USAGE
      funnel <serve-port> {on|off}
      funnel status [--json]

    Funnel allows you to publish a 'tailscale serve'
    server publicly, open to the entire internet.

    Turning off Funnel only turns off serving to the internet.
    It does not affect serving to your tailnet.

    SUBCOMMANDS
      status  show current serve/funnel status
    error: path must be absolute

    error: invalid TCP source "localhost:5432": missing port in address

    error: invalid TCP source "tcp://somehost:5432"
    must be one of: localhost or 127.0.0.1

    tcp://somehost:5432error: invalid TCP source "tcp://somehost:0"
    must be one of: localhost or 127.0.0.1

    tcp://somehost:0error: invalid TCP source "tcp://somehost:65536"
    must be one of: localhost or 127.0.0.1

    tcp://somehost:65536error: path must be absolute

    error: cannot serve web; already serving TCP

    You don't have permission to enable this feature.

This also moves the color handling up to a generic spot so it's
not just one subcommand doing it itself. See
https://github.com/tailscale/tailscale/issues/11626#issuecomment-2041795129

Fixes #11643
Updates #11626

Change-Id: I3a49e659dcbce491f4a2cb784be20bab53f72303
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/tailscale/cli/cli.go

@@ -19,6 +19,8 @@ import (
 	"sync"
 	"text/tabwriter"
 
+	"github.com/mattn/go-colorable"
+	"github.com/mattn/go-isatty"
 	"github.com/peterbourgon/ff/v3/ffcli"
 	"tailscale.com/client/tailscale"
 	"tailscale.com/envknob"
@@ -287,3 +289,17 @@ func countFlags(fs *flag.FlagSet) (n int) {
 	fs.VisitAll(func(*flag.Flag) { n++ })
 	return n
 }
+
+// colorableOutput returns a colorable writer if stdout is a terminal (not, say,
+// redirected to a file or pipe), the Stdout writer is os.Stdout (we're not
+// embedding the CLI in wasm or a mobile app), and NO_COLOR is not set (see
+// https://no-color.org/). If any of those is not the case, ok is false
+// and w is Stdout.
+func colorableOutput() (w io.Writer, ok bool) {
+	if Stdout != os.Stdout ||
+		os.Getenv("NO_COLOR") != "" ||
+		!isatty.IsTerminal(os.Stdout.Fd()) {
+		return Stdout, false
+	}
+	return colorable.NewColorableStdout(), true
+}

cmd/tailscale/cli/debug.go

@@ -453,7 +453,7 @@ func runWatchIPN(ctx context.Context, args []string) error {
 		return err
 	}
 	defer watcher.Close()
-	fmt.Fprintf(os.Stderr, "Connected.\n")
+	fmt.Fprintf(Stderr, "Connected.\n")
 	for seen := 0; watchIPNArgs.count == 0 || seen < watchIPNArgs.count; seen++ {
 		n, err := watcher.Next()
 		if err != nil {
@@ -563,7 +563,7 @@ func runStat(ctx context.Context, args []string) error {
 func runHostinfo(ctx context.Context, args []string) error {
 	hi := hostinfo.New()
 	j, _ := json.MarshalIndent(hi, "", "  ")
-	os.Stdout.Write(j)
+	Stdout.Write(j)
 	return nil
 }
 
@@ -716,7 +716,7 @@ var ts2021Args struct {
 }
 
 func runTS2021(ctx context.Context, args []string) error {
-	log.SetOutput(os.Stdout)
+	log.SetOutput(Stdout)
 	log.SetFlags(log.Ltime | log.Lmicroseconds)
 
 	keysURL := "https://" + ts2021Args.host + "/key?v=" + strconv.Itoa(ts2021Args.version)
@@ -885,7 +885,7 @@ func runCapture(ctx context.Context, args []string) error {
 
 	switch captureArgs.outFile {
 	case "-":
-		fmt.Fprintln(os.Stderr, "Press Ctrl-C to stop the capture.")
+		fmt.Fprintln(Stderr, "Press Ctrl-C to stop the capture.")
 		_, err = io.Copy(os.Stdout, stream)
 		return err
 	case "":
@@ -911,7 +911,7 @@ func runCapture(ctx context.Context, args []string) error {
 		return err
 	}
 	defer f.Close()
-	fmt.Fprintln(os.Stderr, "Press Ctrl-C to stop the capture.")
+	fmt.Fprintln(Stderr, "Press Ctrl-C to stop the capture.")
 	_, err = io.Copy(f, stream)
 	return err
 }

cmd/tailscale/cli/exitnode.go

@@ -9,7 +9,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"os"
 	"slices"
 	"strings"
 	"text/tabwriter"
@@ -121,7 +120,7 @@ func runExitNodeList(ctx context.Context, args []string) error {
 		return fmt.Errorf("no exit nodes found for %q", exitNodeArgs.filter)
 	}
 
-	w := tabwriter.NewWriter(os.Stdout, 10, 5, 5, ' ', 0)
+	w := tabwriter.NewWriter(Stdout, 10, 5, 5, ' ', 0)
 	defer w.Flush()
 	fmt.Fprintf(w, "\n %s\t%s\t%s\t%s\t%s\t", "IP", "HOSTNAME", "COUNTRY", "CITY", "STATUS")
 	for _, country := range filteredPeers.Countries {

cmd/tailscale/cli/funnel.go

@@ -8,7 +8,6 @@ import (
 	"flag"
 	"fmt"
 	"net"
-	"os"
 	"strconv"
 	"strings"
 
@@ -169,10 +168,10 @@ func printFunnelWarning(sc *ipn.ServeConfig) {
 		p, _ := strconv.ParseUint(portStr, 10, 16)
 		if _, ok := sc.TCP[uint16(p)]; !ok {
 			warn = true
-			fmt.Fprintf(os.Stderr, "\nWarning: funnel=on for %s, but no serve config\n", hp)
+			fmt.Fprintf(Stderr, "\nWarning: funnel=on for %s, but no serve config\n", hp)
 		}
 	}
 	if warn {
-		fmt.Fprintf(os.Stderr, "         run: `tailscale serve --help` to see how to configure handlers\n")
+		fmt.Fprintf(Stderr, "         run: `tailscale serve --help` to see how to configure handlers\n")
 	}
 }

cmd/tailscale/cli/network-lock.go

@@ -17,8 +17,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/mattn/go-colorable"
-	"github.com/mattn/go-isatty"
 	"github.com/peterbourgon/ff/v3/ffcli"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tka"
@@ -471,10 +469,10 @@ func runNetworkLockSign(ctx context.Context, args []string) error {
 	// Provide a better help message for when someone clicks through the signing flow
 	// on the wrong device.
 	if err != nil && strings.Contains(err.Error(), "this node is not trusted by network lock") {
-		fmt.Fprintln(os.Stderr, "Error: Signing is not available on this device because it does not have a trusted tailnet lock key.")
-		fmt.Fprintln(os.Stderr)
-		fmt.Fprintln(os.Stderr, "Try again on a signing device instead. Tailnet admins can see signing devices on the admin panel.")
-		fmt.Fprintln(os.Stderr)
+		fmt.Fprintln(Stderr, "Error: Signing is not available on this device because it does not have a trusted tailnet lock key.")
+		fmt.Fprintln(Stderr)
+		fmt.Fprintln(Stderr, "Try again on a signing device instead. Tailnet admins can see signing devices on the admin panel.")
+		fmt.Fprintln(Stderr)
 	}
 	return err
 }
@@ -643,20 +641,19 @@ func runNetworkLockLog(ctx context.Context, args []string) error {
 		return fixTailscaledConnectError(err)
 	}
 	if nlLogArgs.json {
-		enc := json.NewEncoder(os.Stdout)
+		enc := json.NewEncoder(Stdout)
 		enc.SetIndent("", "  ")
 		return enc.Encode(updates)
 	}
 
-	useColor := isatty.IsTerminal(os.Stdout.Fd())
+	out, useColor := colorableOutput()
 
-	stdOut := colorable.NewColorableStdout()
 	for _, update := range updates {
 		stanza, err := nlDescribeUpdate(update, useColor)
 		if err != nil {
 			return err
 		}
-		fmt.Fprintln(stdOut, stanza)
+		fmt.Fprintln(out, stanza)
 	}
 	return nil
 }

cmd/tailscale/cli/serve_legacy.go

@@ -197,7 +197,7 @@ func (e *serveEnv) getLocalClientStatusWithoutPeers(ctx context.Context) (*ipnst
 	}
 	description, ok := isRunningOrStarting(st)
 	if !ok {
-		fmt.Fprintf(os.Stderr, "%s\n", description)
+		fmt.Fprintf(Stderr, "%s\n", description)
 		os.Exit(1)
 	}
 	if st.Self == nil {
@@ -251,7 +251,7 @@ func (e *serveEnv) runServe(ctx context.Context, args []string) error {
 	turnOff := "off" == args[len(args)-1]
 
 	if len(args) < 2 || ((srcType == "https" || srcType == "http") && !turnOff && len(args) < 3) {
-		fmt.Fprintf(os.Stderr, "error: invalid number of arguments\n\n")
+		fmt.Fprintf(Stderr, "error: invalid number of arguments\n\n")
 		return errHelp
 	}
 
@@ -290,8 +290,8 @@ func (e *serveEnv) runServe(ctx context.Context, args []string) error {
 		}
 		return e.handleTCPServe(ctx, srcType, srcPort, args[1])
 	default:
-		fmt.Fprintf(os.Stderr, "error: invalid serve type %q\n", srcType)
-		fmt.Fprint(os.Stderr, "must be one of: http:<port>, https:<port>, tcp:<port> or tls-terminated-tcp:<port>\n\n", srcType)
+		fmt.Fprintf(Stderr, "error: invalid serve type %q\n", srcType)
+		fmt.Fprint(Stderr, "must be one of: http:<port>, https:<port>, tcp:<port> or tls-terminated-tcp:<port>\n\n", srcType)
 		return errHelp
 	}
 }
@@ -327,13 +327,13 @@ func (e *serveEnv) handleWebServe(ctx context.Context, srvPort uint16, useTLS bo
 			return fmt.Errorf("path serving is not supported if sandboxed on macOS")
 		}
 		if !filepath.IsAbs(source) {
-			fmt.Fprintf(os.Stderr, "error: path must be absolute\n\n")
+			fmt.Fprintf(Stderr, "error: path must be absolute\n\n")
 			return errHelp
 		}
 		source = filepath.Clean(source)
 		fi, err := os.Stat(source)
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "error: invalid path: %v\n\n", err)
+			fmt.Fprintf(Stderr, "error: invalid path: %v\n\n", err)
 			return errHelp
 		}
 		if fi.IsDir() && !strings.HasSuffix(mount, "/") {
@@ -357,7 +357,7 @@ func (e *serveEnv) handleWebServe(ctx context.Context, srvPort uint16, useTLS bo
 		return err
 	}
 	if sc.IsTCPForwardingOnPort(srvPort) {
-		fmt.Fprintf(os.Stderr, "error: cannot serve web; already serving TCP\n")
+		fmt.Fprintf(Stderr, "error: cannot serve web; already serving TCP\n")
 		return errHelp
 	}
 
@@ -512,18 +512,18 @@ func (e *serveEnv) handleTCPServe(ctx context.Context, srcType string, srcPort u
 	case "tls-terminated-tcp":
 		terminateTLS = true
 	default:
-		fmt.Fprintf(os.Stderr, "error: invalid TCP source %q\n\n", dest)
+		fmt.Fprintf(Stderr, "error: invalid TCP source %q\n\n", dest)
 		return errHelp
 	}
 
 	dstURL, err := url.Parse(dest)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "error: invalid TCP source %q: %v\n\n", dest, err)
+		fmt.Fprintf(Stderr, "error: invalid TCP source %q: %v\n\n", dest, err)
 		return errHelp
 	}
 	host, dstPortStr, err := net.SplitHostPort(dstURL.Host)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "error: invalid TCP source %q: %v\n\n", dest, err)
+		fmt.Fprintf(Stderr, "error: invalid TCP source %q: %v\n\n", dest, err)
 		return errHelp
 	}
 
@@ -531,13 +531,13 @@ func (e *serveEnv) handleTCPServe(ctx context.Context, srcType string, srcPort u
 	case "localhost", "127.0.0.1":
 		// ok
 	default:
-		fmt.Fprintf(os.Stderr, "error: invalid TCP source %q\n", dest)
-		fmt.Fprint(os.Stderr, "must be one of: localhost or 127.0.0.1\n\n", dest)
+		fmt.Fprintf(Stderr, "error: invalid TCP source %q\n", dest)
+		fmt.Fprint(Stderr, "must be one of: localhost or 127.0.0.1\n\n", dest)
 		return errHelp
 	}
 
 	if p, err := strconv.ParseUint(dstPortStr, 10, 16); p == 0 || err != nil {
-		fmt.Fprintf(os.Stderr, "error: invalid port %q\n\n", dstPortStr)
+		fmt.Fprintf(Stderr, "error: invalid port %q\n\n", dstPortStr)
 		return errHelp
 	}
 
@@ -804,10 +804,10 @@ func (e *serveEnv) enableFeatureInteractive(ctx context.Context, feature string,
 		return nil // already enabled
 	}
 	if info.Text != "" {
-		fmt.Fprintln(os.Stdout, "\n"+info.Text)
+		fmt.Fprintln(Stdout, "\n"+info.Text)
 	}
 	if info.URL != "" {
-		fmt.Fprintln(os.Stdout, "\n         "+info.URL+"\n")
+		fmt.Fprintln(Stdout, "\n         "+info.URL+"\n")
 	}
 	if !info.ShouldWait {
 		e.lc.IncrementCounter(ctx, fmt.Sprintf("%s_not_awaiting_enablement", feature), 1)
@@ -852,7 +852,7 @@ func (e *serveEnv) enableFeatureInteractive(ctx context.Context, feature string,
 			}
 			if gotAll {
 				e.lc.IncrementCounter(ctx, fmt.Sprintf("%s_enabled", feature), 1)
-				fmt.Fprintln(os.Stdout, "Success.")
+				fmt.Fprintln(Stdout, "Success.")
 				return nil
 			}
 		}

cmd/tailscale/cli/serve_legacy_test.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"flag"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -54,6 +55,9 @@ func TestCleanMountPoint(t *testing.T) {
 }
 
 func TestServeConfigMutations(t *testing.T) {
+	tstest.Replace(t, &Stderr, io.Discard)
+	tstest.Replace(t, &Stdout, io.Discard)
+
 	// Stateful mutations, starting from an empty config.
 	type step struct {
 		command []string                       // serve args; nil means no command to run (only reset)
@@ -706,6 +710,7 @@ func TestServeConfigMutations(t *testing.T) {
 			lc:          lc,
 			testFlagOut: &flagOut,
 			testStdout:  &stdout,
+			testStderr:  io.Discard,
 		}
 		lastCount := lc.setCount
 		var cmd *ffcli.Command
@@ -717,6 +722,10 @@ func TestServeConfigMutations(t *testing.T) {
 			cmd = newServeLegacyCommand(e)
 			args = st.command
 		}
+		if cmd.FlagSet == nil {
+			cmd.FlagSet = flag.NewFlagSet(cmd.Name, flag.ContinueOnError)
+			cmd.FlagSet.SetOutput(Stdout)
+		}
 		err := cmd.ParseAndRun(context.Background(), args)
 		if flagOut.Len() > 0 {
 			t.Logf("flag package output: %q", flagOut.Bytes())
@@ -750,6 +759,9 @@ func TestServeConfigMutations(t *testing.T) {
 }
 
 func TestVerifyFunnelEnabled(t *testing.T) {
+	tstest.Replace(t, &Stderr, io.Discard)
+	tstest.Replace(t, &Stdout, io.Discard)
+
 	lc := &fakeLocalServeClient{}
 	var stdout bytes.Buffer
 	var flagOut bytes.Buffer
@@ -757,6 +769,7 @@ func TestVerifyFunnelEnabled(t *testing.T) {
 		lc:          lc,
 		testFlagOut: &flagOut,
 		testStdout:  &stdout,
+		testStderr:  io.Discard,
 	}
 
 	tests := []struct {

cmd/tailscale/cli/serve_v2.go

@@ -823,12 +823,12 @@ func (e *serveEnv) stdout() io.Writer {
 	if e.testStdout != nil {
 		return e.testStdout
 	}
-	return os.Stdout
+	return Stdout
 }
 
 func (e *serveEnv) stderr() io.Writer {
 	if e.testStderr != nil {
 		return e.testStderr
 	}
-	return os.Stderr
+	return Stderr
 }

cmd/tailscale/cli/switch.go

@@ -48,7 +48,7 @@ func listProfiles(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
-	tw := tabwriter.NewWriter(os.Stdout, 2, 2, 2, ' ', 0)
+	tw := tabwriter.NewWriter(Stdout, 2, 2, 2, ' ', 0)
 	defer tw.Flush()
 	printRow := func(vals ...string) {
 		fmt.Fprintln(tw, strings.Join(vals, "\t"))

cmd/tailscale/cli/version.go

@@ -8,7 +8,6 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
-	"os"
 
 	"github.com/peterbourgon/ff/v3/ffcli"
 	"tailscale.com/clientupdate"
@@ -70,7 +69,7 @@ func runVersion(ctx context.Context, args []string) error {
 			Meta:     m,
 			Upstream: upstreamVer,
 		}
-		e := json.NewEncoder(os.Stdout)
+		e := json.NewEncoder(Stdout)
 		e.SetIndent("", "\t")
 		return e.Encode(out)
 	}

cmd/tailscale/cli/whois.go

@@ -9,7 +9,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"os"
 	"strings"
 	"text/tabwriter"
 
@@ -53,7 +52,7 @@ func runWhoIs(ctx context.Context, args []string) error {
 		return nil
 	}
 
-	w := tabwriter.NewWriter(os.Stdout, 10, 5, 5, ' ', 0)
+	w := tabwriter.NewWriter(Stdout, 10, 5, 5, ' ', 0)
 	fmt.Fprintf(w, "Machine:\n")
 	fmt.Fprintf(w, "  Name:\t%s\n", strings.TrimSuffix(who.Node.Name, "."))
 	fmt.Fprintf(w, "  ID:\t%s\n", who.Node.StableID)

cmd/tailscale/depaware.txt

@@ -27,7 +27,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
    L    github.com/jsimonetti/rtnetlink/internal/unix                from github.com/jsimonetti/rtnetlink
         github.com/kballard/go-shellquote                            from tailscale.com/cmd/tailscale/cli
      💣 github.com/mattn/go-colorable                                from tailscale.com/cmd/tailscale/cli
-     💣 github.com/mattn/go-isatty                                   from github.com/mattn/go-colorable+
+     💣 github.com/mattn/go-isatty                                   from tailscale.com/cmd/tailscale/cli+
    L 💣 github.com/mdlayher/netlink                                  from github.com/google/nftables+
    L 💣 github.com/mdlayher/netlink/nlenc                            from github.com/jsimonetti/rtnetlink+
    L    github.com/mdlayher/netlink/nltest                           from github.com/google/nftables