From c71e8db05834a3858238d016043663b97d8559c9 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 7 Apr 2024 18:17:25 -0700 Subject: [PATCH] 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 {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 --- cmd/tailscale/cli/cli.go | 16 +++++++++++++ cmd/tailscale/cli/debug.go | 10 ++++---- cmd/tailscale/cli/exitnode.go | 3 +-- cmd/tailscale/cli/funnel.go | 5 ++-- cmd/tailscale/cli/network-lock.go | 17 ++++++-------- cmd/tailscale/cli/serve_legacy.go | 32 +++++++++++++------------- cmd/tailscale/cli/serve_legacy_test.go | 13 +++++++++++ cmd/tailscale/cli/serve_v2.go | 4 ++-- cmd/tailscale/cli/switch.go | 2 +- cmd/tailscale/cli/version.go | 3 +-- cmd/tailscale/cli/whois.go | 3 +-- cmd/tailscale/depaware.txt | 2 +- 12 files changed, 66 insertions(+), 44 deletions(-) diff --git a/cmd/tailscale/cli/cli.go b/cmd/tailscale/cli/cli.go index 19f6c0763..4c2807053 100644 --- a/cmd/tailscale/cli/cli.go +++ b/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 +} diff --git a/cmd/tailscale/cli/debug.go b/cmd/tailscale/cli/debug.go index 4c59614fd..92ae7bb0b 100644 --- a/cmd/tailscale/cli/debug.go +++ b/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 } diff --git a/cmd/tailscale/cli/exitnode.go b/cmd/tailscale/cli/exitnode.go index 370c9207e..4df208e0d 100644 --- a/cmd/tailscale/cli/exitnode.go +++ b/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 { diff --git a/cmd/tailscale/cli/funnel.go b/cmd/tailscale/cli/funnel.go index 3383c4e7b..39f4af745 100644 --- a/cmd/tailscale/cli/funnel.go +++ b/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") } } diff --git a/cmd/tailscale/cli/network-lock.go b/cmd/tailscale/cli/network-lock.go index b1d4ed978..c3ef5c149 100644 --- a/cmd/tailscale/cli/network-lock.go +++ b/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 } diff --git a/cmd/tailscale/cli/serve_legacy.go b/cmd/tailscale/cli/serve_legacy.go index d0e0e8c7a..77dc3888f 100644 --- a/cmd/tailscale/cli/serve_legacy.go +++ b/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:, https:, tcp: or tls-terminated-tcp:\n\n", srcType) + fmt.Fprintf(Stderr, "error: invalid serve type %q\n", srcType) + fmt.Fprint(Stderr, "must be one of: http:, https:, tcp: or tls-terminated-tcp:\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 } } diff --git a/cmd/tailscale/cli/serve_legacy_test.go b/cmd/tailscale/cli/serve_legacy_test.go index 5b70e6748..2eb982ca0 100644 --- a/cmd/tailscale/cli/serve_legacy_test.go +++ b/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 { diff --git a/cmd/tailscale/cli/serve_v2.go b/cmd/tailscale/cli/serve_v2.go index 3af7ef88d..aabe48217 100644 --- a/cmd/tailscale/cli/serve_v2.go +++ b/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 } diff --git a/cmd/tailscale/cli/switch.go b/cmd/tailscale/cli/switch.go index 857b88d8b..f0bda7350 100644 --- a/cmd/tailscale/cli/switch.go +++ b/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")) diff --git a/cmd/tailscale/cli/version.go b/cmd/tailscale/cli/version.go index 411c76a2a..18c7526fa 100644 --- a/cmd/tailscale/cli/version.go +++ b/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) } diff --git a/cmd/tailscale/cli/whois.go b/cmd/tailscale/cli/whois.go index 3a3a9eda6..d0af73817 100644 --- a/cmd/tailscale/cli/whois.go +++ b/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) diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 24ca03520..103f2048d 100644 --- a/cmd/tailscale/depaware.txt +++ b/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