From f232d4554a5a80d311f4b7b730eeea8a80944138 Mon Sep 17 00:00:00 2001 From: Marwan Sulaiman Date: Mon, 23 Oct 2023 12:29:00 -0400 Subject: [PATCH] cmd/tailscale: translate old serve commands to new ones This PR fixes the isLegacyInvocation to better catch serve and funnel legacy commands. In addition, it now also returns a string that translates the old command into the new one so that users can have an easier transition story. Updates #8489 Signed-off-by: Marwan Sulaiman --- cmd/tailscale/cli/serve_v2.go | 144 +++++++++++++++++++++++------ cmd/tailscale/cli/serve_v2_test.go | 116 +++++++++++++++++++---- 2 files changed, 217 insertions(+), 43 deletions(-) diff --git a/cmd/tailscale/cli/serve_v2.go b/cmd/tailscale/cli/serve_v2.go index 26ce1f29c..993a4feea 100644 --- a/cmd/tailscale/cli/serve_v2.go +++ b/cmd/tailscale/cli/serve_v2.go @@ -153,19 +153,30 @@ func newServeV2Command(e *serveEnv, subcmd serveMode) *ffcli.Command { } func validateArgs(subcmd serveMode, args []string) error { - switch len(args) { - case 0: - return flag.ErrHelp - case 1, 2: - if isLegacyInvocation(subcmd, args) { - fmt.Fprintf(os.Stderr, "error: the CLI for serve and funnel has changed.") - fmt.Fprintf(os.Stderr, "Please see https://tailscale.com/kb/1242/tailscale-serve for more information.") - return errHelp + if translation, ok := isLegacyInvocation(subcmd, args); ok { + fmt.Fprint(os.Stderr, "Error: the CLI for serve and funnel has changed.") + if translation != "" { + fmt.Fprint(os.Stderr, " You can run the following command instead:\n") + fmt.Fprintf(os.Stderr, "\t- %s\n", translation) } - default: - fmt.Fprintf(os.Stderr, "error: invalid number of arguments (%d)", len(args)) + fmt.Fprint(os.Stderr, "\nPlease see https://tailscale.com/kb/1242/tailscale-serve for more information.\n") return errHelp } + if len(args) == 0 { + return flag.ErrHelp + } + if len(args) > 2 { + fmt.Fprintf(os.Stderr, "Error: invalid number of arguments (%d)\n", len(args)) + return errHelp + } + turnOff := args[len(args)-1] == "off" + if len(args) == 2 && !turnOff { + fmt.Fprintln(os.Stderr, "Error: invalid argument format") + return errHelp + } + + // Given the two checks above, we can assume there + // are only 1 or 2 arguments which is valid. return nil } @@ -656,18 +667,98 @@ func srvTypeAndPortFromFlags(e *serveEnv) (srvType serveType, srvPort uint16, er return srvType, srvPort, nil } -func isLegacyInvocation(subcmd serveMode, args []string) bool { - if subcmd == serve && len(args) == 2 { - prefixes := []string{"http", "https", "tcp", "tls-terminated-tcp"} +// isLegacyInvocation helps transition customers who have been using the beta +// CLI to the newer API by returning a translation from the old command to the new command. +// The second result is a boolean that only returns true if the given arguments is a valid +// legacy invocation. If the given args are in the old format but are not valid, it will +// return false and expects the new code path has enough validations to reject the request. +func isLegacyInvocation(subcmd serveMode, args []string) (string, bool) { + if subcmd == funnel { + if len(args) != 2 { + return "", false + } + _, err := strconv.ParseUint(args[0], 10, 16) + return "", err == nil && (args[1] == "on" || args[1] == "off") + } + turnOff := len(args) > 1 && args[len(args)-1] == "off" + if turnOff { + args = args[:len(args)-1] + } + if len(args) == 0 { + return "", false + } + + srcType, srcPortStr, found := strings.Cut(args[0], ":") + if !found { + if srcType == "https" && srcPortStr == "" { + // Default https port to 443. + srcPortStr = "443" + } else if srcType == "http" && srcPortStr == "" { + // Default http port to 80. + srcPortStr = "80" + } else { + return "", false + } + } - for _, prefix := range prefixes { - if strings.HasPrefix(args[0], prefix) { - return true - } + var wantLength int + switch srcType { + case "https", "http": + wantLength = 3 + case "tcp", "tls-terminated-tcp": + wantLength = 2 + default: + // return non-legacy, and let new code handle validation. + return "", false + } + // The length is either exactlly the same as in "https / " + // or target is omitted as in "https / off" where omit the off at + // the top. + if len(args) != wantLength && !(turnOff && len(args) == wantLength-1) { + return "", false + } + + cmd := []string{"tailscale", "serve", "--bg"} + switch srcType { + case "https": + // In the new code, we default to https:443, + // so we don't need to pass the flag explicitly. + if srcPortStr != "443" { + cmd = append(cmd, fmt.Sprintf("--https %s", srcPortStr)) } + case "http": + cmd = append(cmd, fmt.Sprintf("--http %s", srcPortStr)) + case "tcp", "tls-terminated-tcp": + cmd = append(cmd, fmt.Sprintf("--%s %s", srcType, srcPortStr)) } - return false + var mount string + if srcType == "https" || srcType == "http" { + mount = args[1] + if _, err := cleanMountPoint(mount); err != nil { + return "", false + } + if mount != "/" { + cmd = append(cmd, "--set-path "+mount) + } + } + + // If there's no "off" there must always be a target destination. + // If there is "off", target is optional so check if it exists + // first before appending it. + hasTarget := !turnOff || (turnOff && len(args) == wantLength) + if hasTarget { + dest := args[len(args)-1] + if strings.Contains(dest, " ") { + dest = strconv.Quote(dest) + } + cmd = append(cmd, dest) + } + if turnOff { + cmd = append(cmd, "off") + } + + return strings.Join(cmd, " "), true } // removeWebServe removes a web handler from the serve config @@ -768,7 +859,7 @@ func (e *serveEnv) removeTCPServe(sc *ipn.ServeConfig, src uint16) error { // - https-insecure://localhost:3000 // - https-insecure://localhost:3000/foo func expandProxyTargetDev(target string, supportedSchemes []string, defaultScheme string) (string, error) { - var host = "127.0.0.1" + const host = "127.0.0.1" // support target being a port number if port, err := strconv.ParseUint(target, 10, 16); err == nil { @@ -791,20 +882,21 @@ func expandProxyTargetDev(target string, supportedSchemes []string, defaultSchem return "", fmt.Errorf("must be a URL starting with one of the supported schemes: %v", supportedSchemes) } - // validate the port - port, err := strconv.ParseUint(u.Port(), 10, 16) - if err != nil || port == 0 { - return "", fmt.Errorf("invalid port %q", u.Port()) - } - // validate the host. switch u.Hostname() { case "localhost", "127.0.0.1": - u.Host = fmt.Sprintf("%s:%d", host, port) default: return "", errors.New("only localhost or 127.0.0.1 proxies are currently supported") } + // validate the port + port, err := strconv.ParseUint(u.Port(), 10, 16) + if err != nil || port == 0 { + return "", fmt.Errorf("invalid port %q", u.Port()) + } + + u.Host = fmt.Sprintf("%s:%d", host, port) + return u.String(), nil } diff --git a/cmd/tailscale/cli/serve_v2_test.go b/cmd/tailscale/cli/serve_v2_test.go index 028e66cd2..3c474bcbb 100644 --- a/cmd/tailscale/cli/serve_v2_test.go +++ b/cmd/tailscale/cli/serve_v2_test.go @@ -11,6 +11,7 @@ import ( "path/filepath" "reflect" "runtime" + "strconv" "strings" "testing" @@ -1188,29 +1189,110 @@ func unindent(s string) string { func TestIsLegacyInvocation(t *testing.T) { tests := []struct { - subcmd serveMode - args []string - expected bool + subcmd serveMode + args []string + expected bool + translation string }{ - {subcmd: serve, args: []string{"https", "localhost:3000"}, expected: true}, - {subcmd: serve, args: []string{"https:8443", "localhost:3000"}, expected: true}, - {subcmd: serve, args: []string{"http", "localhost:3000"}, expected: true}, - {subcmd: serve, args: []string{"http:80", "localhost:3000"}, expected: true}, - {subcmd: serve, args: []string{"tcp:2222", "tcp://localhost:22"}, expected: true}, - {subcmd: serve, args: []string{"tls-terminated-tcp:443", "tcp://localhost:80"}, expected: true}, + { + subcmd: serve, + args: []string{"https", "/", "localhost:3000"}, + expected: true, + translation: "tailscale serve --bg localhost:3000", + }, + { + subcmd: serve, + args: []string{"https", "/", "localhost:3000", "off"}, + expected: true, + translation: "tailscale serve --bg localhost:3000 off", + }, + { + subcmd: serve, + args: []string{"https", "/", "off"}, + expected: true, + translation: "tailscale serve --bg off", + }, + { + subcmd: serve, + args: []string{"https:4545", "/foo", "off"}, + expected: true, + translation: "tailscale serve --bg --https 4545 --set-path /foo off", + }, + { + subcmd: serve, + args: []string{"https:4545", "/foo", "localhost:9090", "off"}, + expected: true, + translation: "tailscale serve --bg --https 4545 --set-path /foo localhost:9090 off", + }, + { + subcmd: serve, + args: []string{"https:8443", "/", "localhost:3000"}, + expected: true, + translation: "tailscale serve --bg --https 8443 localhost:3000", + }, + { + subcmd: serve, + args: []string{"http", "/", "localhost:3000"}, + expected: true, + translation: "tailscale serve --bg --http 80 localhost:3000", + }, + { + subcmd: serve, + args: []string{"http:80", "/", "localhost:3000"}, + expected: true, + translation: "tailscale serve --bg --http 80 localhost:3000", + }, + { + subcmd: serve, + args: []string{"https:10000", "/motd.txt", `text:Hello, world!`}, + expected: true, + translation: `tailscale serve --bg --https 10000 --set-path /motd.txt "text:Hello, world!"`, + }, + { + subcmd: serve, + args: []string{"tcp:2222", "tcp://localhost:22"}, + expected: true, + translation: "tailscale serve --bg --tcp 2222 tcp://localhost:22", + }, + { + subcmd: serve, + args: []string{"tls-terminated-tcp:443", "tcp://localhost:80"}, + expected: true, + translation: "tailscale serve --bg --tls-terminated-tcp 443 tcp://localhost:80", + }, + { + subcmd: funnel, + args: []string{"443", "on"}, + expected: true, + }, + { + subcmd: funnel, + args: []string{"443", "off"}, + expected: true, + }, - // false - {subcmd: serve, args: []string{"3000"}, expected: false}, - {subcmd: serve, args: []string{"localhost:3000"}, expected: false}, + { + subcmd: serve, + args: []string{"3000"}, + expected: false, + }, + { + subcmd: serve, + args: []string{"localhost:3000"}, + expected: false, + }, } - for _, tt := range tests { - args := strings.Join(tt.args, " ") - t.Run(fmt.Sprintf("%v %s", infoMap[tt.subcmd].Name, args), func(t *testing.T) { - actual := isLegacyInvocation(tt.subcmd, tt.args) + for idx, tt := range tests { + t.Run(strconv.Itoa(idx), func(t *testing.T) { + gotTranslation, actual := isLegacyInvocation(tt.subcmd, tt.args) if actual != tt.expected { - t.Errorf("Got: %v; expected: %v", actual, tt.expected) + t.Fatalf("got: %v; expected: %v", actual, tt.expected) + } + + if gotTranslation != tt.translation { + t.Fatalf("expected translaction to be %q but got %q", tt.translation, gotTranslation) } }) }