cmd/tailscale: reduce duplicate calls to LocalBackend

This PR ensures calls to the LocalBackend are not happening
multiples times and ensures the set/unset methods are
only manipulating the serve config

Updates #8489

Signed-off-by: Marwan Sulaiman <marwan@tailscale.com>

cmd/tailscale/cli/serve_dev.go

@@ -203,27 +203,24 @@ func (e *serveEnv) runServeCombined(subcmd serveMode) execFunc {
 			return errHelp
 		}
 
-		if turnOff {
+		sc, err := e.lc.GetServeConfig(ctx)
-			err := e.unsetServe(ctx, srvType, srvPort, mount)
 		if err != nil {
-				fmt.Fprintf(os.Stderr, "error: %v\n\n", err)
+			return fmt.Errorf("error getting serve config: %w", err)
-				return errHelp
-			}
-			return nil
 		}
 
-		err = e.setServe(ctx, st, srvType, srvPort, mount, target, funnel)
+		// nil if no config
-		if err != nil {
+		if sc == nil {
-			fmt.Fprintf(os.Stderr, "error: %v\n\n", err)
+			sc = new(ipn.ServeConfig)
-			return errHelp
 		}
+		dnsName := strings.TrimSuffix(st.Self.DNSName, ".")
 
-		return nil
+		// set parent serve config to always be persisted
-	}
+		// at the top level, but a nested config might be
-}
+		// the one that gets manipulated depending on
+		// foreground or background.
+		parentSC := sc
 
-func (e *serveEnv) setServe(ctx context.Context, st *ipnstate.Status, srvType string, srvPort uint16, mount string, target string, allowFunnel bool) error {
+		if !turnOff && srvType == "https" {
-	if srvType == "https" {
 			// Running serve with https requires that the tailnet has enabled
 			// https cert provisioning. Send users through an interactive flow
 			// to enable this if not already done.
@@ -233,38 +230,18 @@ func (e *serveEnv) setServe(ctx context.Context, st *ipnstate.Status, srvType st
 			// on, enableFeatureInteractive will error. For now, we hide that
 			// error and maintain the previous behavior (prior to 2023-08-15)
 			// of letting them edit the serve config before enabling certs.
-		e.enableFeatureInteractive(ctx, "serve", func(caps []string) bool {
+			if err := e.enableFeatureInteractive(ctx, "serve", func(caps []string) bool {
 				return slices.Contains(caps, tailcfg.CapabilityHTTPS)
-		})
+			}); err != nil {
-	}
+				return fmt.Errorf("error enabling https feature: %w", err)
-
+			}
-	// get serve config
-	sc, err := e.lc.GetServeConfig(ctx)
-	if err != nil {
-		return err
-	}
-
-	// nil if no config
-	if sc == nil {
-		sc = new(ipn.ServeConfig)
-	}
-
-	// set parent serve config to always be persisted
-	// at the top level, but a nested config might be
-	// the one that gets manipulated depending on
-	// foreground or background.
-	parentSC := sc
-
-	dnsName, err := e.getSelfDNSName(ctx)
-	if err != nil {
-		return err
 		}
 
+		var watcher *tailscale.IPNBusWatcher
+		if !e.bg && !turnOff {
 			// if foreground mode, create a WatchIPNBus session
 			// and use the nested config for all following operations
 			// TODO(marwan-at-work): nested-config validations should happen here or previous to this point.
-	var watcher *tailscale.IPNBusWatcher
-	if !e.bg {
 			watcher, err = e.lc.WatchIPNBus(ctx, ipn.NotifyInitialState)
 			if err != nil {
 				return err
@@ -282,44 +259,25 @@ func (e *serveEnv) setServe(ctx context.Context, st *ipnstate.Status, srvType st
 			sc = fsc
 		}
 
-	// update serve config based on the type
+		var msg string
-	switch srvType {
+		if turnOff {
-	case "https", "http":
+			err = e.unsetServe(sc, dnsName, srvType, srvPort, mount)
-		mount, err := cleanMountPoint(mount)
+		} else {
-		if err != nil {
+			err = e.setServe(sc, st, dnsName, srvType, srvPort, mount, target, funnel)
-			return fmt.Errorf("failed to clean the mount point: %w", err)
+			msg = e.messageForPort(sc, st, dnsName, srvPort)
 		}
-		useTLS := srvType == "https"
-		err = e.applyWebServe(sc, dnsName, srvPort, useTLS, mount, target)
 		if err != nil {
-			return fmt.Errorf("failed apply web serve: %w", err)
+			fmt.Fprintf(os.Stderr, "error: %v\n\n", err)
-		}
+			return errHelp
-	case "tcp", "tls-terminated-tcp":
-		err = e.applyTCPServe(sc, dnsName, srvType, srvPort, target)
-		if err != nil {
-			return fmt.Errorf("failed to apply TCP serve: %w", err)
-		}
-	default:
-		return fmt.Errorf("invalid type %q", srvType)
 		}
 
-	// update the serve config based on if funnel is enabled
-	e.applyFunnel(sc, dnsName, srvPort, allowFunnel)
-
-	// persist the serve config changes
 		if err := e.lc.SetServeConfig(ctx, parentSC); err != nil {
 			return err
 		}
 
-	// notify the user of the change
+		fmt.Fprintln(os.Stderr, msg)
-	m, err := e.messageForPort(ctx, sc, st, dnsName, srvPort)
-	if err != nil {
-		return err
-	}
 
-	fmt.Fprintln(os.Stderr, m)
+		if watcher != nil {
-
-	if !e.bg {
 			for {
 				_, err = watcher.Next()
 				if err != nil {
@@ -333,8 +291,37 @@ func (e *serveEnv) setServe(ctx context.Context, st *ipnstate.Status, srvType st
 
 		return nil
 	}
+}
 
-func (e *serveEnv) messageForPort(ctx context.Context, sc *ipn.ServeConfig, st *ipnstate.Status, dnsName string, srvPort uint16) (string, error) {
+func (e *serveEnv) setServe(sc *ipn.ServeConfig, st *ipnstate.Status, dnsName, srvType string, srvPort uint16, mount string, target string, allowFunnel bool) error {
+	// update serve config based on the type
+	switch srvType {
+	case "https", "http":
+		mount, err := cleanMountPoint(mount)
+		if err != nil {
+			return fmt.Errorf("failed to clean the mount point: %w", err)
+		}
+		useTLS := srvType == "https"
+		err = e.applyWebServe(sc, dnsName, srvPort, useTLS, mount, target)
+		if err != nil {
+			return fmt.Errorf("failed apply web serve: %w", err)
+		}
+	case "tcp", "tls-terminated-tcp":
+		err := e.applyTCPServe(sc, dnsName, srvType, srvPort, target)
+		if err != nil {
+			return fmt.Errorf("failed to apply TCP serve: %w", err)
+		}
+	default:
+		return fmt.Errorf("invalid type %q", srvType)
+	}
+
+	// update the serve config based on if funnel is enabled
+	e.applyFunnel(sc, dnsName, srvPort, allowFunnel)
+
+	return nil
+}
+
+func (e *serveEnv) messageForPort(sc *ipn.ServeConfig, st *ipnstate.Status, dnsName string, srvPort uint16) string {
 	var output strings.Builder
 
 	hp := ipn.HostPort(net.JoinHostPort(dnsName, strconv.Itoa(int(srvPort))))
@@ -409,7 +396,7 @@ func (e *serveEnv) messageForPort(ctx context.Context, sc *ipn.ServeConfig, st *
 		// TODO(marwan-at-work): give the user more context on their foreground process.
 	}
 
-	return output.String(), nil
+	return output.String() + "\n"
 }
 
 func (e *serveEnv) applyWebServe(sc *ipn.ServeConfig, dnsName string, srvPort uint16, useTLS bool, mount, target string) error {
@@ -550,14 +537,14 @@ func (e *serveEnv) applyFunnel(sc *ipn.ServeConfig, dnsName string, srvPort uint
 
 // TODO(tylersmalley) Refactor into setServe so handleWebServeFunnelRemove and handleTCPServeRemove.
 // apply serve config changes and we print a status message.
-func (e *serveEnv) unsetServe(ctx context.Context, srvType string, srvPort uint16, mount string) error {
+func (e *serveEnv) unsetServe(sc *ipn.ServeConfig, dnsName string, srvType string, srvPort uint16, mount string) error {
 	switch srvType {
 	case "https", "http":
 		mount, err := cleanMountPoint(mount)
 		if err != nil {
 			return fmt.Errorf("failed to clean the mount point: %w", err)
 		}
-		err = e.handleWebServeFunnelRemove(ctx, srvPort, mount)
+		err = e.handleWebServeFunnelRemove(sc, dnsName, srvPort, mount)
 		if err != nil {
 			return err
 		}
@@ -565,7 +552,7 @@ func (e *serveEnv) unsetServe(ctx context.Context, srvType string, srvPort uint1
 		return nil
 	case "tcp", "tls-terminated-tcp":
 		// TODO(tylersmalley) should remove funnel
-		return e.removeTCPServe(ctx, srvPort)
+		return e.removeTCPServe(sc, srvPort)
 	default:
 		return fmt.Errorf("invalid type %q", srvType)
 	}
@@ -624,18 +611,10 @@ func checkLegacyServeInvocation(subcmd serveMode, args []string) error {
 // The srvPort argument is the serving port and the mount argument is
 // the mount point or registered path to remove.
 // TODO(tylersmalley): fork of handleWebServeRemove, return name once dev work is merged
-func (e *serveEnv) handleWebServeFunnelRemove(ctx context.Context, srvPort uint16, mount string) error {
+func (e *serveEnv) handleWebServeFunnelRemove(sc *ipn.ServeConfig, dnsName string, srvPort uint16, mount string) error {
-	sc, err := e.lc.GetServeConfig(ctx)
-	if err != nil {
-		return err
-	}
 	if sc == nil {
 		return errors.New("error: serve config does not exist")
 	}
-	dnsName, err := e.getSelfDNSName(ctx)
-	if err != nil {
-		return err
-	}
 	if sc.IsTCPForwardingOnPort(srvPort) {
 		return errors.New("cannot remove web handler; currently serving TCP")
 	}
@@ -662,34 +641,25 @@ func (e *serveEnv) handleWebServeFunnelRemove(ctx context.Context, srvPort uint1
 		delete(sc.AllowFunnel, hp)
 	}
 
-	if err := e.lc.SetServeConfig(ctx, sc); err != nil {
-		return err
-	}
-
 	return nil
 }
 
 // removeTCPServe removes the TCP forwarding configuration for the
 // given srvPort, or serving port.
-func (e *serveEnv) removeTCPServe(ctx context.Context, src uint16) error {
+func (e *serveEnv) removeTCPServe(sc *ipn.ServeConfig, src uint16) error {
-	cursc, err := e.lc.GetServeConfig(ctx)
-	if err != nil {
-		return err
-	}
-	sc := cursc.Clone() // nil if no config
 	if sc == nil {
-		sc = new(ipn.ServeConfig)
+		return nil
+	}
+	if sc.GetTCPPortHandler(src) == nil {
+		return errors.New("error: serve config does not exist")
 	}
 	if sc.IsServingWeb(src) {
 		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)
 	// clear map mostly for testing
 	if len(sc.TCP) == 0 {
 		sc.TCP = nil
 	}
-		return e.lc.SetServeConfig(ctx, sc)
+	return nil
-	}
-	return errors.New("error: serve config does not exist")
 }