cli/serve: funnel interactive enablement flow tweaks

1. Add metrics to funnel flow.
2. Stop blocking users from turning off funnels when no longer in
   their node capabilities.
3. Rename LocalClient.IncrementMetric to IncrementCounter to better
   callout its usage is only for counter clientmetrics.

Updates tailscale/corp#10577

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

client/tailscale/localclient.go

@@ -259,13 +259,12 @@ func (lc *LocalClient) DaemonMetrics(ctx context.Context) ([]byte, error) {
 	return lc.get200(ctx, "/localapi/v0/metrics")
 }
 
-// IncrementMetric increments the value of a Tailscale daemon's metric by
+// IncrementCounter increments the value of a Tailscale daemon's counter
-// the given delta. If the metric has yet to exist, a new counter metric is
+// metric by the given delta. If the metric has yet to exist, a new counter
-// created and initialized to delta.
+// metric is created and initialized to delta.
 //
-// IncrementMetric only supports counter metrics and non-negative delta values.
+// IncrementCounter does not support gauge metrics or negative delta values.
-// Gauge metrics are unsupported.
+func (lc *LocalClient) IncrementCounter(ctx context.Context, name string, delta int) error {
-func (lc *LocalClient) IncrementMetric(ctx context.Context, name string, delta int) error {
 	type metricUpdate struct {
 		Name  string `json:"name"`
 		Type  string `json:"type"`

cmd/tailscale/cli/funnel.go

@@ -94,8 +94,13 @@ func (e *serveEnv) runFunnel(ctx context.Context, args []string) error {
 	}
 	port := uint16(port64)
 
-	if err := e.verifyFunnelEnabled(ctx, st, port); err != nil {
+	if on {
-		return err
+		// Don't block from turning off existing Funnel if
+		// network configuration/capabilities have changed.
+		// Only block from starting new Funnels.
+		if err := e.verifyFunnelEnabled(ctx, st, port); err != nil {
+			return err
+		}
 	}
 
 	dnsName := strings.TrimSuffix(st.Self.DNSName, ".")
@@ -176,7 +181,7 @@ func printFunnelWarning(sc *ipn.ServeConfig) {
 		p, _ := strconv.ParseUint(portStr, 10, 16)
 		if _, ok := sc.TCP[uint16(p)]; !ok {
 			warn = true
-			fmt.Fprintf(os.Stderr, "Warning: funnel=on for %s, but no serve config\n", hp)
+			fmt.Fprintf(os.Stderr, "\nWarning: funnel=on for %s, but no serve config\n", hp)
 		}
 	}
 	if warn {

cmd/tailscale/cli/serve.go

@@ -133,6 +133,7 @@ type localServeClient interface {
 	SetServeConfig(context.Context, *ipn.ServeConfig) error
 	QueryFeature(ctx context.Context, feature string) (*tailcfg.QueryFeatureResponse, error)
 	WatchIPNBus(ctx context.Context, mask ipn.NotifyWatchOpt) (*tailscale.IPNBusWatcher, error)
+	IncrementCounter(ctx context.Context, name string, delta int) error
 }
 
 // serveEnv is the environment the serve command runs within. All I/O should be
@@ -796,17 +797,19 @@ func (e *serveEnv) enableFeatureInteractive(ctx context.Context, feature string,
 		return nil // already enabled
 	}
 	if info.Text != "" {
-		fmt.Fprintln(os.Stdout, info.Text)
+		fmt.Fprintln(os.Stdout, "\n"+info.Text)
 	}
 	if info.URL != "" {
-		fmt.Fprintln(os.Stdout, "\n         "+info.URL)
+		fmt.Fprintln(os.Stdout, "\n         "+info.URL+"\n")
 	}
 	if !info.ShouldWait {
+		e.lc.IncrementCounter(ctx, fmt.Sprintf("%s_not_awaiting_enablement", feature), 1)
 		// The feature has not been enabled yet,
 		// but the CLI should not block on user action.
 		// Once info.Text is printed, exit the CLI.
 		os.Exit(0)
 	}
+	e.lc.IncrementCounter(ctx, fmt.Sprintf("%s_awaiting_enablement", feature), 1)
 	// Block until feature is enabled.
 	watchCtx, cancelWatch := context.WithCancel(ctx)
 	defer cancelWatch()
@@ -816,6 +819,7 @@ func (e *serveEnv) enableFeatureInteractive(ctx context.Context, feature string,
 		// don't block. We still present the URL in the CLI,
 		// then close the process. Swallow the error.
 		log.Fatalf("lost connection to tailscaled: %v", err)
+		e.lc.IncrementCounter(ctx, fmt.Sprintf("%s_enablement_lost_connection", feature), 1)
 		return err
 	}
 	defer watcher.Close()
@@ -826,11 +830,13 @@ func (e *serveEnv) enableFeatureInteractive(ctx context.Context, feature string,
 			// Let the user finish enablement then rerun their
 			// command themselves.
 			log.Fatalf("lost connection to tailscaled: %v", err)
+			e.lc.IncrementCounter(ctx, fmt.Sprintf("%s_enablement_lost_connection", feature), 1)
 			return err
 		}
 		if nm := n.NetMap; nm != nil && nm.SelfNode != nil {
 			if hasRequiredCapabilities(nm.SelfNode.Capabilities) {
-				fmt.Fprintln(os.Stdout, "\nSuccess.")
+				e.lc.IncrementCounter(ctx, fmt.Sprintf("%s_enabled", feature), 1)
+				fmt.Fprintln(os.Stdout, "Success.")
 				return nil
 			}
 		}

cmd/tailscale/cli/serve_test.go

@@ -893,7 +893,11 @@ func (lc *fakeLocalServeClient) QueryFeature(ctx context.Context, feature string
 }
 
 func (lc *fakeLocalServeClient) WatchIPNBus(ctx context.Context, mask ipn.NotifyWatchOpt) (*tailscale.IPNBusWatcher, error) {
-	return nil, nil
+	return nil, nil // unused in tests
+}
+
+func (lc *fakeLocalServeClient) IncrementCounter(ctx context.Context, name string, delta int) error {
+	return nil // unused in tests
 }
 
 // exactError returns an error checker that wants exactly the provided want error.