cmd/tailscale/cli, ipn: move serve CLI funcs on to ServeConfig (#6401)

Signed-off-by: Shayne Sweeney <shayne@tailscale.com>

cmd/tailscale/cli/serve.go

@@ -114,6 +114,19 @@ func newServeCommand(e *serveEnv) *ffcli.Command {
 	}
 }
 
+func (e *serveEnv) newFlags(name string, setup func(fs *flag.FlagSet)) *flag.FlagSet {
+	onError, out := flag.ExitOnError, Stderr
+	if e.testFlagOut != nil {
+		onError, out = flag.ContinueOnError, e.testFlagOut
+	}
+	fs := flag.NewFlagSet(name, onError)
+	fs.SetOutput(out)
+	if setup != nil {
+		setup(fs)
+	}
+	return fs
+}
+
 // serveEnv is the environment the serve command runs within. All I/O should be
 // done via serveEnv methods so that it can be faked out for tests.
 //
@@ -133,6 +146,9 @@ type serveEnv struct {
 	testStdout               io.Writer
 }
 
+// getSelfDNSName returns the DNS name of the current node.
+// The trailing dot is removed.
+// Returns an error if local client status fails.
 func (e *serveEnv) getSelfDNSName(ctx context.Context) (string, error) {
 	st, err := e.getLocalClientStatus(ctx)
 	if err != nil {
@@ -141,6 +157,10 @@ func (e *serveEnv) getSelfDNSName(ctx context.Context) (string, error) {
 	return strings.TrimSuffix(st.Self.DNSName, "."), nil
 }
 
+// getLocalClientStatus calls LocalClient.Status, checks if
+// Status is ready.
+// Returns error if unable to reach tailscaled or if self node is nil.
+// Exits if status is not running or starting.
 func (e *serveEnv) getLocalClientStatus(ctx context.Context) (*ipnstate.Status, error) {
 	if e.testGetLocalClientStatus != nil {
 		return e.testGetLocalClientStatus(ctx)
@@ -160,19 +180,6 @@ func (e *serveEnv) getLocalClientStatus(ctx context.Context) (*ipnstate.Status,
 	return st, nil
 }
 
-func (e *serveEnv) newFlags(name string, setup func(fs *flag.FlagSet)) *flag.FlagSet {
-	onError, out := flag.ExitOnError, Stderr
-	if e.testFlagOut != nil {
-		onError, out = flag.ContinueOnError, e.testFlagOut
-	}
-	fs := flag.NewFlagSet(name, onError)
-	fs.SetOutput(out)
-	if setup != nil {
-		setup(fs)
-	}
-	return fs
-}
-
 func (e *serveEnv) getServeConfig(ctx context.Context) (*ipn.ServeConfig, error) {
 	if e.testGetServeConfig != nil {
 		return e.testGetServeConfig(ctx)
@@ -187,13 +194,6 @@ func (e *serveEnv) setServeConfig(ctx context.Context, c *ipn.ServeConfig) error
 	return localClient.SetServeConfig(ctx, c)
 }
 
-func (e *serveEnv) stdout() io.Writer {
-	if e.testStdout != nil {
-		return e.testStdout
-	}
-	return os.Stdout
-}
-
 // validateServePort returns --serve-port flag value,
 // or an error if the port is not a valid port to serve on.
 func (e *serveEnv) validateServePort() (port uint16, err error) {
@@ -221,12 +221,6 @@ func (e *serveEnv) runServe(ctx context.Context, args []string) error {
 		return flag.ErrHelp
 	}
 
-	srvPort, err := e.validateServePort()
-	if err != nil {
-		return err
-	}
-	srvPortStr := strconv.Itoa(int(srvPort))
-
 	// Undocumented debug command (not using ffcli subcommands) to set raw
 	// configs from stdin for now (2022-11-13).
 	if len(args) == 1 && args[0] == "set-raw" {
@@ -246,6 +240,12 @@ func (e *serveEnv) runServe(ctx context.Context, args []string) error {
 		return flag.ErrHelp
 	}
 
+	srvPort, err := e.validateServePort()
+	if err != nil {
+		return err
+	}
+	srvPortStr := strconv.Itoa(int(srvPort))
+
 	mount, err := cleanMountPoint(args[0])
 	if err != nil {
 		return err
@@ -305,7 +305,7 @@ func (e *serveEnv) runServe(ctx context.Context, args []string) error {
 	}
 	hp := ipn.HostPort(net.JoinHostPort(dnsName, srvPortStr))
 
-	if isTCPForwardingOnPort(sc, srvPort) {
+	if sc.IsTCPForwardingOnPort(srvPort) {
 		fmt.Fprintf(os.Stderr, "error: cannot serve web; already serving TCP\n")
 		return flag.ErrHelp
 	}
@@ -359,11 +359,11 @@ func (e *serveEnv) handleWebServeRemove(ctx context.Context, mount string) error
 	if err != nil {
 		return err
 	}
-	if isTCPForwardingOnPort(sc, srvPort) {
+	if sc.IsTCPForwardingOnPort(srvPort) {
 		return errors.New("cannot remove web handler; currently serving TCP")
 	}
 	hp := ipn.HostPort(net.JoinHostPort(dnsName, srvPortStr))
-	if !httpHandlerExists(sc, hp, mount) {
+	if !sc.WebHandlerExists(hp, mount) {
 		return errors.New("error: serve config does not exist")
 	}
 	// delete existing handler, then cascade delete if empty
@@ -385,18 +385,6 @@ func (e *serveEnv) handleWebServeRemove(ctx context.Context, mount string) error
 	return nil
 }
 
-func httpHandlerExists(sc *ipn.ServeConfig, hp ipn.HostPort, mount string) bool {
-	h := getHTTPHandler(sc, hp, mount)
-	return h != nil
-}
-
-func getHTTPHandler(sc *ipn.ServeConfig, hp ipn.HostPort, mount string) *ipn.HTTPHandler {
-	if sc != nil && sc.Web[hp] != nil {
-		return sc.Web[hp].Handlers[mount]
-	}
-	return nil
-}
-
 func cleanMountPoint(mount string) (string, error) {
 	if mount == "" {
 		return "", errors.New("mount point cannot be empty")
@@ -447,39 +435,6 @@ func expandProxyTarget(target string) (string, error) {
 	return url, nil
 }
 
-// isTCPForwardingAny checks if any TCP port is being forwarded.
-func isTCPForwardingAny(sc *ipn.ServeConfig) bool {
-	if sc == nil || len(sc.TCP) == 0 {
-		return false
-	}
-	for _, h := range sc.TCP {
-		if h.TCPForward != "" {
-			return true
-		}
-	}
-	return false
-}
-
-// isTCPForwardingOnPort checks serve config to see if
-// we're specifically forwarding TCP on the given port.
-func isTCPForwardingOnPort(sc *ipn.ServeConfig, port uint16) bool {
-	if sc == nil || sc.TCP[port] == nil {
-		return false
-	}
-	return !sc.TCP[port].HTTPS
-}
-
-// isServingWeb checks serve config to see if
-// we're serving a web handler on the given port.
-func isServingWeb(sc *ipn.ServeConfig, port uint16) bool {
-	if sc == nil || sc.Web == nil || sc.TCP == nil ||
-		sc.TCP[port] == nil || sc.TCP[port].HTTPS == false {
-		// not listening on port
-		return false
-	}
-	return true
-}
-
 func allNumeric(s string) bool {
 	for i := 0; i < len(s); i++ {
 		if s[i] < '0' || s[i] > '9' {
@@ -516,7 +471,7 @@ func (e *serveEnv) runServeStatus(ctx context.Context, args []string) error {
 	if err != nil {
 		return err
 	}
-	if isTCPForwardingAny(sc) {
+	if sc.IsTCPForwardingAny() {
 		if err := printTCPStatusTree(ctx, sc, st); err != nil {
 			return err
 		}
@@ -540,6 +495,13 @@ func (e *serveEnv) runServeStatus(ctx context.Context, args []string) error {
 	return nil
 }
 
+func (e *serveEnv) stdout() io.Writer {
+	if e.testStdout != nil {
+		return e.testStdout
+	}
+	return os.Stdout
+}
+
 func printTCPStatusTree(ctx context.Context, sc *ipn.ServeConfig, st *ipnstate.Status) error {
 	dnsName := strings.TrimSuffix(st.Self.DNSName, ".")
 	for p, h := range sc.TCP {
@@ -552,7 +514,7 @@ func printTCPStatusTree(ctx context.Context, sc *ipn.ServeConfig, st *ipnstate.S
 			tlsStatus = "TLS terminated"
 		}
 		fStatus := "tailnet only"
-		if isFunnelOn(sc, hp) {
+		if sc.IsFunnelOn(hp) {
 			fStatus = "Funnel on"
 		}
 		printf("|-- tcp://%s (%s, %s)\n", hp, tlsStatus, fStatus)
@@ -570,7 +532,7 @@ func printWebStatusTree(sc *ipn.ServeConfig, hp ipn.HostPort) {
 		return
 	}
 	fStatus := "tailnet only"
-	if isFunnelOn(sc, hp) {
+	if sc.IsFunnelOn(hp) {
 		fStatus = "Funnel on"
 	}
 	host, portStr, _ := net.SplitHostPort(string(hp))
@@ -649,12 +611,15 @@ func (e *serveEnv) runServeTCP(ctx context.Context, args []string) error {
 
 	fwdAddr := "127.0.0.1:" + portStr
 
-	if e.remove {
+	if sc.IsServingWeb(srvPort) {
-		if isServingWeb(sc, srvPort) {
+		if e.remove {
-			return errors.New("cannot remove TCP port; currently serving web")
+			return fmt.Errorf("unable to remove; serving web, not TCP forwarding on serve port %d", srvPort)
 		}
-		if sc.TCP != nil && sc.TCP[srvPort] != nil &&
+		return fmt.Errorf("cannot serve TCP; already serving web on %d", srvPort)
-			sc.TCP[srvPort].TCPForward == fwdAddr {
+	}
+
+	if e.remove {
+		if ph := sc.GetTCPPortHandler(srvPort); ph != nil && ph.TCPForward == fwdAddr {
 			delete(sc.TCP, srvPort)
 			// clear map mostly for testing
 			if len(sc.TCP) == 0 {
@@ -662,15 +627,9 @@ func (e *serveEnv) runServeTCP(ctx context.Context, args []string) error {
 			}
 			return e.setServeConfig(ctx, sc)
 		}
-
 		return errors.New("error: serve config does not exist")
 	}
 
-	if isServingWeb(sc, srvPort) {
-		fmt.Fprintf(os.Stderr, "error: cannot serve TCP; already serving Web\n\n")
-		return flag.ErrHelp
-	}
-
 	mak.Set(&sc.TCP, srvPort, &ipn.TCPPortHandler{TCPForward: fwdAddr})
 
 	dnsName, err := e.getSelfDNSName(ctx)
@@ -716,6 +675,9 @@ func (e *serveEnv) runServeFunnel(ctx context.Context, args []string) error {
 	if err != nil {
 		return err
 	}
+	if sc == nil {
+		sc = new(ipn.ServeConfig)
+	}
 	st, err := e.getLocalClientStatus(ctx)
 	if err != nil {
 		return fmt.Errorf("getting client status: %w", err)
@@ -725,14 +687,11 @@ func (e *serveEnv) runServeFunnel(ctx context.Context, args []string) error {
 	}
 	dnsName := strings.TrimSuffix(st.Self.DNSName, ".")
 	hp := ipn.HostPort(dnsName + ":" + srvPortStr)
-	if on && sc != nil && sc.AllowFunnel[hp] ||
+	isFun := sc.IsFunnelOn(hp)
-		!on && (sc == nil || !sc.AllowFunnel[hp]) {
+	if on && isFun || !on && !isFun {
 		// Nothing to do.
 		return nil
 	}
-	if sc == nil {
-		sc = &ipn.ServeConfig{}
-	}
 	if on {
 		mak.Set(&sc.AllowFunnel, hp, true)
 	} else {
@@ -747,7 +706,3 @@ func (e *serveEnv) runServeFunnel(ctx context.Context, args []string) error {
 	}
 	return nil
 }
-
-func isFunnelOn(sc *ipn.ServeConfig, hp ipn.HostPort) bool {
-	return sc != nil && sc.AllowFunnel[hp]
-}

cmd/tailscale/cli/serve_test.go

@@ -577,9 +577,9 @@ type step struct {
 			},
 		},
 	})
-	add(step{ // try to start a tcp forwarder on the same port
+	add(step{ // try to start a tcp forwarder on the same serve port (443 default)
 		command: cmd("tcp 5432"),
-		wantErr: exactErr(flag.ErrHelp, "flag.ErrHelp"),
+		wantErr: anyErr(),
 	})
 
 	// And now run the steps above.

ipn/serve.go

@@ -0,0 +1,135 @@
+// Copyright (c) 2022 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package ipn
+
+// ServeConfigKey returns a StateKey that stores the
+// JSON-encoded ServeConfig for a config profile.
+func ServeConfigKey(profileID ProfileID) StateKey {
+	return StateKey("_serve/" + profileID)
+}
+
+// ServeConfig is the JSON type stored in the StateStore for
+// StateKey "_serve/$PROFILE_ID" as returned by ServeConfigKey.
+type ServeConfig struct {
+	// TCP are the list of TCP port numbers that tailscaled should handle for
+	// the Tailscale IP addresses. (not subnet routers, etc)
+	TCP map[uint16]*TCPPortHandler `json:",omitempty"`
+
+	// Web maps from "$SNI_NAME:$PORT" to a set of HTTP handlers
+	// keyed by mount point ("/", "/foo", etc)
+	Web map[HostPort]*WebServerConfig `json:",omitempty"`
+
+	// AllowFunnel is the set of SNI:port values for which funnel
+	// traffic is allowed, from trusted ingress peers.
+	AllowFunnel map[HostPort]bool `json:",omitempty"`
+}
+
+// HostPort is an SNI name and port number, joined by a colon.
+// There is no implicit port 443. It must contain a colon.
+type HostPort string
+
+// WebServerConfig describes a web server's configuration.
+type WebServerConfig struct {
+	Handlers map[string]*HTTPHandler // mountPoint => handler
+}
+
+// TCPPortHandler describes what to do when handling a TCP
+// connection.
+type TCPPortHandler struct {
+	// HTTPS, if true, means that tailscaled should handle this connection as an
+	// HTTPS request as configured by ServeConfig.Web.
+	//
+	// It is mutually exclusive with TCPForward.
+	HTTPS bool `json:",omitempty"`
+
+	// TCPForward is the IP:port to forward TCP connections to.
+	// Whether or not TLS is terminated by tailscaled depends on
+	// TerminateTLS.
+	//
+	// It is mutually exclusive with HTTPS.
+	TCPForward string `json:",omitempty"`
+
+	// TerminateTLS, if non-empty, means that tailscaled should terminate the
+	// TLS connections before forwarding them to TCPForward, permitting only the
+	// SNI name with this value. It is only used if TCPForward is non-empty.
+	// (the HTTPS mode uses ServeConfig.Web)
+	TerminateTLS string `json:",omitempty"`
+}
+
+// HTTPHandler is either a path or a proxy to serve.
+type HTTPHandler struct {
+	// Exactly one of the following may be set.
+
+	Path  string `json:",omitempty"` // absolute path to directory or file to serve
+	Proxy string `json:",omitempty"` // http://localhost:3000/, localhost:3030, 3030
+
+	Text string `json:",omitempty"` // plaintext to serve (primarily for testing)
+
+	// TODO(bradfitz): bool to not enumerate directories? TTL on mapping for
+	// temporary ones? Error codes? Redirects?
+}
+
+// WebHandlerExists checks if the ServeConfig Web handler exists for
+// the given host:port and mount point.
+func (sc *ServeConfig) WebHandlerExists(hp HostPort, mount string) bool {
+	h := sc.GetWebHandler(hp, mount)
+	return h != nil
+}
+
+// GetWebHandler returns the HTTPHandler for the given host:port and mount point.
+// Returns nil if the handler does not exist.
+func (sc *ServeConfig) GetWebHandler(hp HostPort, mount string) *HTTPHandler {
+	if sc.Web[hp] != nil {
+		return sc.Web[hp].Handlers[mount]
+	}
+	return nil
+}
+
+// GetTCPPortHandler returns the TCPPortHandler for the given port.
+// If the port is not configured, nil is returned.
+func (sc *ServeConfig) GetTCPPortHandler(port uint16) *TCPPortHandler {
+	return sc.TCP[port]
+}
+
+// IsTCPForwardingAny checks if ServeConfig is currently forwarding
+// in TCPForward mode on any port.
+// This is exclusive of Web/HTTPS serving.
+func (sc *ServeConfig) IsTCPForwardingAny() bool {
+	if len(sc.TCP) == 0 {
+		return false
+	}
+	for _, h := range sc.TCP {
+		if h.TCPForward != "" {
+			return true
+		}
+	}
+	return false
+}
+
+// IsTCPForwardingOnPort checks if ServeConfig is currently forwarding
+// in TCPForward mode on the given port.
+// This is exclusive of Web/HTTPS serving.
+func (sc *ServeConfig) IsTCPForwardingOnPort(port uint16) bool {
+	if sc.TCP[port] == nil {
+		return false
+	}
+	return !sc.TCP[port].HTTPS
+}
+
+// IsServingWeb checks if ServeConfig is currently serving
+// Web/HTTPS on the given port.
+// This is exclusive of TCPForwarding.
+func (sc *ServeConfig) IsServingWeb(port uint16) bool {
+	if sc.TCP[port] == nil {
+		return false
+	}
+	return sc.TCP[port].HTTPS
+}
+
+// IsFunnelOn checks if ServeConfig is currently allowing
+// funnel traffic on for the given host:port.
+func (sc *ServeConfig) IsFunnelOn(hp HostPort) bool {
+	return sc.AllowFunnel[hp]
+}

ipn/store.go

@@ -90,70 +90,3 @@ func ReadStoreInt(store StateStore, id StateKey) (int64, error) {
 func PutStoreInt(store StateStore, id StateKey, val int64) error {
 	return store.WriteState(id, fmt.Appendf(nil, "%d", val))
 }
-
-// ServeConfigKey returns a StateKey that stores the
-// JSON-encoded ServeConfig for a config profile.
-func ServeConfigKey(profileID ProfileID) StateKey {
-	return StateKey("_serve/" + profileID)
-}
-
-// ServeConfig is the JSON type stored in the StateStore for
-// StateKey "_serve/$PROFILE_ID" as returned by ServeConfigKey.
-type ServeConfig struct {
-	// TCP are the list of TCP port numbers that tailscaled should handle for
-	// the Tailscale IP addresses. (not subnet routers, etc)
-	TCP map[uint16]*TCPPortHandler `json:",omitempty"`
-
-	// Web maps from "$SNI_NAME:$PORT" to a set of HTTP handlers
-	// keyed by mount point ("/", "/foo", etc)
-	Web map[HostPort]*WebServerConfig `json:",omitempty"`
-
-	// AllowFunnel is the set of SNI:port values for which funnel
-	// traffic is allowed, from trusted ingress peers.
-	AllowFunnel map[HostPort]bool `json:",omitempty"`
-}
-
-// HostPort is an SNI name and port number, joined by a colon.
-// There is no implicit port 443. It must contain a colon.
-type HostPort string
-
-// WebServerConfig describes a web server's configuration.
-type WebServerConfig struct {
-	Handlers map[string]*HTTPHandler // mountPoint => handler
-}
-
-// TCPPortHandler describes what to do when handling a TCP
-// connection.
-type TCPPortHandler struct {
-	// HTTPS, if true, means that tailscaled should handle this connection as an
-	// HTTPS request as configured by ServeConfig.Web.
-	//
-	// It is mutually exclusive with TCPForward.
-	HTTPS bool `json:",omitempty"`
-
-	// TCPForward is the IP:port to forward TCP connections to.
-	// Whether or not TLS is terminated by tailscaled depends on
-	// TerminateTLS.
-	//
-	// It is mutually exclusive with HTTPS.
-	TCPForward string `json:",omitempty"`
-
-	// TerminateTLS, if non-empty, means that tailscaled should terminate the
-	// TLS connections before forwarding them to TCPForward, permitting only the
-	// SNI name with this value. It is only used if TCPForward is non-empty.
-	// (the HTTPS mode uses ServeConfig.Web)
-	TerminateTLS string `json:",omitempty"`
-}
-
-// HTTPHandler is either a path or a proxy to serve.
-type HTTPHandler struct {
-	// Exactly one of the following may be set.
-
-	Path  string `json:",omitempty"` // absolute path to directory or file to serve
-	Proxy string `json:",omitempty"` // http://localhost:3000/, localhost:3030, 3030
-
-	Text string `json:",omitempty"` // plaintext to serve (primarily for testing)
-
-	// TODO(bradfitz): bool to not enumerate directories? TTL on mapping for
-	// temporary ones? Error codes? Redirects?
-}