From 0052830c64abeeb267fd9f017b46f5ecf3eb0e87 Mon Sep 17 00:00:00 2001 From: Sonia Appasamy Date: Tue, 15 Aug 2023 00:07:51 -0400 Subject: [PATCH] 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 --- client/tailscale/localclient.go | 11 +++++------ cmd/tailscale/cli/funnel.go | 11 ++++++++--- cmd/tailscale/cli/serve.go | 12 +++++++++--- cmd/tailscale/cli/serve_test.go | 6 +++++- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/client/tailscale/localclient.go b/client/tailscale/localclient.go index 150eed0ed..fde465bac 100644 --- a/client/tailscale/localclient.go +++ b/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 -// the given delta. If the metric has yet to exist, a new counter metric is -// created and initialized to delta. +// IncrementCounter increments the value of a Tailscale daemon's counter +// metric by the given delta. If the metric has yet to exist, a new counter +// metric is created and initialized to delta. // -// IncrementMetric only supports counter metrics and non-negative delta values. -// Gauge metrics are unsupported. -func (lc *LocalClient) IncrementMetric(ctx context.Context, name string, delta int) error { +// IncrementCounter does not support gauge metrics or negative delta values. +func (lc *LocalClient) IncrementCounter(ctx context.Context, name string, delta int) error { type metricUpdate struct { Name string `json:"name"` Type string `json:"type"` diff --git a/cmd/tailscale/cli/funnel.go b/cmd/tailscale/cli/funnel.go index b27390c1c..cf6ae15de 100644 --- a/cmd/tailscale/cli/funnel.go +++ b/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 { - return err + if on { + // 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 { diff --git a/cmd/tailscale/cli/serve.go b/cmd/tailscale/cli/serve.go index 4688012dd..8979a90ed 100644 --- a/cmd/tailscale/cli/serve.go +++ b/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 } } diff --git a/cmd/tailscale/cli/serve_test.go b/cmd/tailscale/cli/serve_test.go index 88e7aeb87..cae6b2bee 100644 --- a/cmd/tailscale/cli/serve_test.go +++ b/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.