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

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) {
+	if translation, ok := isLegacyInvocation(subcmd, args); ok {
-	case 0:
+		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)
+		}
+		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
-	case 1, 2:
+	}
-		if isLegacyInvocation(subcmd, args) {
+	if len(args) > 2 {
-			fmt.Fprintf(os.Stderr, "error: the CLI for serve and funnel has changed.")
+		fmt.Fprintf(os.Stderr, "Error: invalid number of arguments (%d)\n", len(args))
-			fmt.Fprintf(os.Stderr, "Please see https://tailscale.com/kb/1242/tailscale-serve for more information.")
 		return errHelp
 	}
-	default:
+	turnOff := args[len(args)-1] == "off"
-		fmt.Fprintf(os.Stderr, "error: invalid number of arguments (%d)", len(args))
+	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 {
+// isLegacyInvocation helps transition customers who have been using the beta
-	if subcmd == serve && len(args) == 2 {
+// CLI to the newer API by returning a translation from the old command to the new command.
-		prefixes := []string{"http", "https", "tcp", "tls-terminated-tcp"}
+// 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
+		}
+	}
+
+	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 / <target>"
+	// 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))
+	}
 
-		for _, prefix := range prefixes {
+	var mount string
-			if strings.HasPrefix(args[0], prefix) {
+	if srcType == "https" || srcType == "http" {
-				return true
+		mount = args[1]
+		if _, err := cleanMountPoint(mount); err != nil {
+			return "", false
 		}
+		if mount != "/" {
+			cmd = append(cmd, "--set-path "+mount)
 		}
 	}
 
-	return false
+	// 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,19 +882,20 @@ 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 host.
+	switch u.Hostname() {
+	case "localhost", "127.0.0.1":
+	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())
 	}
 
-	// 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")
-	}
 
 	return u.String(), nil
 }

cmd/tailscale/cli/serve_v2_test.go

@@ -11,6 +11,7 @@ import (
 	"path/filepath"
 	"reflect"
 	"runtime"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -1191,26 +1192,107 @@ func TestIsLegacyInvocation(t *testing.T) {
 		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,
-		{subcmd: serve, args: []string{"http", "localhost:3000"}, expected: true},
+			args:        []string{"https", "/", "localhost:3000"},
-		{subcmd: serve, args: []string{"http:80", "localhost:3000"}, expected: true},
+			expected:    true,
-		{subcmd: serve, args: []string{"tcp:2222", "tcp://localhost:22"}, expected: true},
+			translation: "tailscale serve --bg localhost:3000",
-		{subcmd: serve, args: []string{"tls-terminated-tcp:443", "tcp://localhost:80"}, expected: true},
+		},
+		{
+			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,
-		{subcmd: serve, args: []string{"localhost:3000"}, expected: false},
+			args:     []string{"3000"},
+			expected: false,
+		},
+		{
+			subcmd:   serve,
+			args:     []string{"localhost:3000"},
+			expected: false,
+		},
 	}
 
-	for _, tt := range tests {
+	for idx, tt := range tests {
-		args := strings.Join(tt.args, " ")
+		t.Run(strconv.Itoa(idx), func(t *testing.T) {
-		t.Run(fmt.Sprintf("%v %s", infoMap[tt.subcmd].Name, args), func(t *testing.T) {
+			gotTranslation, actual := isLegacyInvocation(tt.subcmd, tt.args)
-			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)
 			}
 		})
 	}