{ipn,cmd/tailscale/cli}: move ServeConfig mutation logic to ipn/serve

Moving logic that manipulates a ServeConfig into recievers on the
ServeConfig in the ipn package. This is setup work to allow the
web client and cli to both utilize these shared functions to edit
the serve config.

Any logic specific to flag parsing or validation is left untouched
in the cli command. The web client will similarly manage its
validation of user's requested changes. If validation logic becomes
similar-enough, we can make a serve util for shared functionality,
which likely does not make sense in ipn.

Updates #10261

Signed-off-by: Sonia Appasamy <sonia@tailscale.com>

cmd/tailscale/cli/funnel.go

@@ -15,7 +15,6 @@ import (
 	"github.com/peterbourgon/ff/v3/ffcli"
 	"tailscale.com/ipn"
 	"tailscale.com/tailcfg"
-	"tailscale.com/util/mak"
 )
 
 var funnelCmd = func() *ffcli.Command {
@@ -114,15 +113,8 @@ func (e *serveEnv) runFunnel(ctx context.Context, args []string) error {
 		// Nothing to do.
 		return nil
 	}
-	if on {
+	sc.SetFunnel(dnsName, port, on)
-		mak.Set(&sc.AllowFunnel, hp, true)
+
-	} else {
-		delete(sc.AllowFunnel, hp)
-		// clear map mostly for testing
-		if len(sc.AllowFunnel) == 0 {
-			sc.AllowFunnel = nil
-		}
-	}
 	if err := e.lc.SetServeConfig(ctx, sc); err != nil {
 		return err
 	}

cmd/tailscale/cli/serve_legacy.go

@@ -27,7 +27,6 @@ import (
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
-	"tailscale.com/util/mak"
 	"tailscale.com/version"
 )
 
@@ -357,35 +356,12 @@ func (e *serveEnv) handleWebServe(ctx context.Context, srvPort uint16, useTLS bo
 	if err != nil {
 		return err
 	}
-	hp := ipn.HostPort(net.JoinHostPort(dnsName, strconv.Itoa(int(srvPort))))
-
 	if sc.IsTCPForwardingOnPort(srvPort) {
 		fmt.Fprintf(os.Stderr, "error: cannot serve web; already serving TCP\n")
 		return errHelp
 	}
 
-	mak.Set(&sc.TCP, srvPort, &ipn.TCPPortHandler{HTTPS: useTLS, HTTP: !useTLS})
+	sc.SetWebHandler(h, dnsName, srvPort, mount, useTLS)
-
-	if _, ok := sc.Web[hp]; !ok {
-		mak.Set(&sc.Web, hp, new(ipn.WebServerConfig))
-	}
-	mak.Set(&sc.Web[hp].Handlers, mount, h)
-
-	for k, v := range sc.Web[hp].Handlers {
-		if v == h {
-			continue
-		}
-		// If the new mount point ends in / and another mount point
-		// shares the same prefix, remove the other handler.
-		// (e.g. /foo/ overwrites /foo)
-		// The opposite example is also handled.
-		m1 := strings.TrimSuffix(mount, "/")
-		m2 := strings.TrimSuffix(k, "/")
-		if m1 == m2 {
-			delete(sc.Web[hp].Handlers, k)
-			continue
-		}
-	}
 
 	if !reflect.DeepEqual(cursc, sc) {
 		if err := e.lc.SetServeConfig(ctx, sc); err != nil {
@@ -444,19 +420,7 @@ func (e *serveEnv) handleWebServeRemove(ctx context.Context, srvPort uint16, mou
 	if !sc.WebHandlerExists(hp, mount) {
 		return errors.New("error: handler does not exist")
 	}
-	// delete existing handler, then cascade delete if empty
+	sc.RemoveWebHandler(dnsName, srvPort, []string{mount}, false)
-	delete(sc.Web[hp].Handlers, mount)
-	if len(sc.Web[hp].Handlers) == 0 {
-		delete(sc.Web, hp)
-		delete(sc.TCP, srvPort)
-	}
-	// clear empty maps mostly for testing
-	if len(sc.Web) == 0 {
-		sc.Web = nil
-	}
-	if len(sc.TCP) == 0 {
-		sc.TCP = nil
-	}
 	if err := e.lc.SetServeConfig(ctx, sc); err != nil {
 		return err
 	}
@@ -592,15 +556,12 @@ func (e *serveEnv) handleTCPServe(ctx context.Context, srcType string, srcPort u
 		return fmt.Errorf("cannot serve TCP; already serving web on %d", srcPort)
 	}
 
-	mak.Set(&sc.TCP, srcPort, &ipn.TCPPortHandler{TCPForward: fwdAddr})
-
 	dnsName, err := e.getSelfDNSName(ctx)
 	if err != nil {
 		return err
 	}
-	if terminateTLS {
+
-		sc.TCP[srcPort].TerminateTLS = dnsName
+	sc.SetTCPForwarding(srcPort, fwdAddr, terminateTLS, dnsName)
-	}
 
 	if !reflect.DeepEqual(cursc, sc) {
 		if err := e.lc.SetServeConfig(ctx, sc); err != nil {
@@ -626,11 +587,7 @@ func (e *serveEnv) handleTCPServeRemove(ctx context.Context, src uint16) error {
 		return fmt.Errorf("unable to remove; serving web, not TCP forwarding on serve port %d", src)
 	}
 	if ph := sc.GetTCPPortHandler(src); ph != nil {
-		delete(sc.TCP, src)
+		sc.RemoveTCPForwarding(src)
-		// clear map mostly for testing
-		if len(sc.TCP) == 0 {
-			sc.TCP = nil
-		}
 		return e.lc.SetServeConfig(ctx, sc)
 	}
 	return errors.New("error: serve config does not exist")

cmd/tailscale/cli/serve_v2.go

@@ -528,29 +528,7 @@ func (e *serveEnv) applyWebServe(sc *ipn.ServeConfig, dnsName string, srvPort ui
 		return errors.New("cannot serve web; already serving TCP")
 	}
 
-	mak.Set(&sc.TCP, srvPort, &ipn.TCPPortHandler{HTTPS: useTLS, HTTP: !useTLS})
+	sc.SetWebHandler(h, dnsName, srvPort, mount, useTLS)
-
-	hp := ipn.HostPort(net.JoinHostPort(dnsName, strconv.Itoa(int(srvPort))))
-	if _, ok := sc.Web[hp]; !ok {
-		mak.Set(&sc.Web, hp, new(ipn.WebServerConfig))
-	}
-	mak.Set(&sc.Web[hp].Handlers, mount, h)
-
-	// TODO: handle multiple web handlers from foreground mode
-	for k, v := range sc.Web[hp].Handlers {
-		if v == h {
-			continue
-		}
-		// If the new mount point ends in / and another mount point
-		// shares the same prefix, remove the other handler.
-		// (e.g. /foo/ overwrites /foo)
-		// The opposite example is also handled.
-		m1 := strings.TrimSuffix(mount, "/")
-		m2 := strings.TrimSuffix(k, "/")
-		if m1 == m2 {
-			delete(sc.Web[hp].Handlers, k)
-		}
-	}
 
 	return nil
 }
@@ -581,11 +559,7 @@ func (e *serveEnv) applyTCPServe(sc *ipn.ServeConfig, dnsName string, srcType se
 		return fmt.Errorf("cannot serve TCP; already serving web on %d", srcPort)
 	}
 
-	mak.Set(&sc.TCP, srcPort, &ipn.TCPPortHandler{TCPForward: dstURL.Host})
+	sc.SetTCPForwarding(srcPort, dstURL.Host, terminateTLS, dnsName)
-
-	if terminateTLS {
-		sc.TCP[srcPort].TerminateTLS = dnsName
-	}
 
 	return nil
 }
@@ -599,14 +573,10 @@ func (e *serveEnv) applyFunnel(sc *ipn.ServeConfig, dnsName string, srvPort uint
 		sc = new(ipn.ServeConfig)
 	}
 
-	// TODO: should ensure there is no other conflicting funnel
+	if _, exists := sc.AllowFunnel[hp]; exists && !allowFunnel {
-	// TODO: add error handling for if toggling for existing sc
+		fmt.Fprintf(e.stderr(), "Removing Funnel for %s:%s\n", dnsName, hp)
-	if allowFunnel {
-		mak.Set(&sc.AllowFunnel, hp, true)
-	} else if _, exists := sc.AllowFunnel[hp]; exists {
-		fmt.Fprintf(e.stderr(), "Removing Funnel for %s\n", hp)
-		delete(sc.AllowFunnel, hp)
 	}
+	sc.SetFunnel(dnsName, srvPort, allowFunnel)
 }
 
 // unsetServe removes the serve config for the given serve port.
@@ -795,34 +765,7 @@ func (e *serveEnv) removeWebServe(sc *ipn.ServeConfig, dnsName string, srvPort u
 		}
 	}
 
-	// delete existing handler, then cascade delete if empty
+	sc.RemoveWebHandler(dnsName, srvPort, mounts, true)
-	for _, m := range mounts {
-		delete(sc.Web[hp].Handlers, m)
-	}
-	if len(sc.Web[hp].Handlers) == 0 {
-		delete(sc.Web, hp)
-		delete(sc.AllowFunnel, hp)
-		delete(sc.TCP, srvPort)
-	}
-
-	// clear empty maps mostly for testing
-	if len(sc.Web) == 0 {
-		sc.Web = nil
-	}
-
-	if len(sc.TCP) == 0 {
-		sc.TCP = nil
-	}
-
-	// disable funnel if no remaining mounts exist for the serve port
-	if sc.Web == nil && sc.TCP == nil {
-		delete(sc.AllowFunnel, hp)
-	}
-
-	if len(sc.AllowFunnel) == 0 {
-		sc.AllowFunnel = nil
-	}
-
 	return nil
 }
 
@@ -838,11 +781,7 @@ func (e *serveEnv) removeTCPServe(sc *ipn.ServeConfig, src uint16) error {
 	if sc.IsServingWeb(src) {
 		return fmt.Errorf("unable to remove; serving web, not TCP forwarding on serve port %d", src)
 	}
-	delete(sc.TCP, src)
+	sc.RemoveTCPForwarding(src)
-	// clear map mostly for testing
-	if len(sc.TCP) == 0 {
-		sc.TCP = nil
-	}
 	return nil
 }
 

ipn/serve.go

@@ -15,6 +15,7 @@ import (
 
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
+	"tailscale.com/util/mak"
 )
 
 // ServeConfigKey returns a StateKey that stores the
@@ -253,6 +254,111 @@ func (sc *ServeConfig) FindConfig(port uint16) (*ServeConfig, bool) {
 	return nil, false
 }
 
+// SetWebHandler sets the given HTTPHandler at the specified host, port,
+// and mount in the serve config. sc.TCP is also updated to reflect web
+// serving usage of the given port.
+func (sc *ServeConfig) SetWebHandler(handler *HTTPHandler, host string, port uint16, mount string, useTLS bool) {
+	if sc == nil {
+		sc = new(ServeConfig)
+	}
+	mak.Set(&sc.TCP, port, &TCPPortHandler{HTTPS: useTLS, HTTP: !useTLS})
+
+	hp := HostPort(net.JoinHostPort(host, strconv.Itoa(int(port))))
+	if _, ok := sc.Web[hp]; !ok {
+		mak.Set(&sc.Web, hp, new(WebServerConfig))
+	}
+	mak.Set(&sc.Web[hp].Handlers, mount, handler)
+
+	// TODO(tylersmalley): handle multiple web handlers from foreground mode
+	for k, v := range sc.Web[hp].Handlers {
+		if v == handler {
+			continue
+		}
+		// If the new mount point ends in / and another mount point
+		// shares the same prefix, remove the other handler.
+		// (e.g. /foo/ overwrites /foo)
+		// The opposite example is also handled.
+		m1 := strings.TrimSuffix(mount, "/")
+		m2 := strings.TrimSuffix(k, "/")
+		if m1 == m2 {
+			delete(sc.Web[hp].Handlers, k)
+		}
+	}
+}
+
+// SetTCPForwarding sets the fwdAddr (IP:port form) to which to forward
+// connections from the given port. If terminateTLS is true, TLS connections
+// are terminated with only the given host name permitted before passing them
+// to the fwdAddr.
+func (sc *ServeConfig) SetTCPForwarding(port uint16, fwdAddr string, terminateTLS bool, host string) {
+	if sc == nil {
+		sc = new(ServeConfig)
+	}
+	mak.Set(&sc.TCP, port, &TCPPortHandler{TCPForward: fwdAddr})
+	if terminateTLS {
+		sc.TCP[port].TerminateTLS = host
+	}
+}
+
+// SetFunnel sets the sc.AllowFunnel value for the given host and port.
+func (sc *ServeConfig) SetFunnel(host string, port uint16, setOn bool) {
+	if sc == nil {
+		sc = new(ServeConfig)
+	}
+	hp := HostPort(net.JoinHostPort(host, strconv.Itoa(int(port))))
+
+	// TODO(tylersmalley): should ensure there is no other conflicting funnel
+	// TODO(tylersmalley): add error handling for if toggling for existing sc
+	if setOn {
+		mak.Set(&sc.AllowFunnel, hp, true)
+	} else if _, exists := sc.AllowFunnel[hp]; exists {
+		delete(sc.AllowFunnel, hp)
+		// Clear map mostly for testing.
+		if len(sc.AllowFunnel) == 0 {
+			sc.AllowFunnel = nil
+		}
+	}
+}
+
+// RemoveWebHandler deletes the web handlers at all of the given mount points
+// for the provided host and port in the serve config. If cleanupFunnel is
+// true, this also removes the funnel value for this port if no handlers remain.
+func (sc *ServeConfig) RemoveWebHandler(host string, port uint16, mounts []string, cleanupFunnel bool) {
+	hp := HostPort(net.JoinHostPort(host, strconv.Itoa(int(port))))
+
+	// Delete existing handler, then cascade delete if empty.
+	for _, m := range mounts {
+		delete(sc.Web[hp].Handlers, m)
+	}
+	if len(sc.Web[hp].Handlers) == 0 {
+		delete(sc.Web, hp)
+		delete(sc.TCP, port)
+		if cleanupFunnel {
+			delete(sc.AllowFunnel, hp) // disable funnel if no mounts remain for the port
+		}
+	}
+
+	// Clear empty maps, mostly for testing.
+	if len(sc.Web) == 0 {
+		sc.Web = nil
+	}
+	if len(sc.TCP) == 0 {
+		sc.TCP = nil
+	}
+	if len(sc.AllowFunnel) == 0 {
+		sc.AllowFunnel = nil
+	}
+}
+
+// RemoveTCPForwarding deletes the TCP forwarding configuration for the given
+// port from the serve config.
+func (sc *ServeConfig) RemoveTCPForwarding(port uint16) {
+	delete(sc.TCP, port)
+	if len(sc.TCP) == 0 {
+		sc.TCP = nil
+	}
+}
+
 // IsFunnelOn reports whether if ServeConfig is currently allowing funnel
 // traffic for any host:port.
 //