cmd/tailscale/cli: use status before doing interactive feature query

We were inconsistent whether we checked if the feature was already
enabled which we could do cheaply using the locally available status.
We would do the checks fine if we were turning on funnel, but not serve.

This moves the cap checks down into enableFeatureInteractive so that
are always run.

Updates #9984

Co-authored-by: Tyler Smalley <tyler@tailscale.com>
Signed-off-by: Maisem Ali <maisem@tailscale.com>

cmd/tailscale/cli/funnel.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/peterbourgon/ff/v3/ffcli"
 	"tailscale.com/ipn"
-	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
 	"tailscale.com/util/mak"
 )
@@ -88,10 +87,6 @@ func (e *serveEnv) runFunnel(ctx context.Context, args []string) error {
 	if sc == nil {
 		sc = new(ipn.ServeConfig)
 	}
-	st, err := e.getLocalClientStatusWithoutPeers(ctx)
-	if err != nil {
-		return fmt.Errorf("getting client status: %w", err)
-	}
 
 	port64, err := strconv.ParseUint(args[0], 10, 16)
 	if err != nil {
@@ -103,11 +98,15 @@ func (e *serveEnv) runFunnel(ctx context.Context, args []string) error {
 		// 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 {
+		if err := e.verifyFunnelEnabled(ctx, port); err != nil {
 			return err
 		}
 	}
 
+	st, err := e.getLocalClientStatusWithoutPeers(ctx)
+	if err != nil {
+		return fmt.Errorf("getting client status: %w", err)
+	}
 	dnsName := strings.TrimSuffix(st.Self.DNSName, ".")
 	hp := ipn.HostPort(dnsName + ":" + strconv.Itoa(int(port)))
 	if on == sc.AllowFunnel[hp] {
@@ -141,13 +140,7 @@ func (e *serveEnv) runFunnel(ctx context.Context, args []string) error {
 // If an error is reported, the CLI should stop execution and return the error.
 //
 // verifyFunnelEnabled may refresh the local state and modify the st input.
-func (e *serveEnv) verifyFunnelEnabled(ctx context.Context, st *ipnstate.Status, port uint16) error {
+func (e *serveEnv) verifyFunnelEnabled(ctx context.Context, port uint16) error {
-	hasFunnelAttrs := func(selfNode *ipnstate.PeerStatus) bool {
-		return selfNode.HasCap(tailcfg.CapabilityHTTPS) && selfNode.HasCap(tailcfg.NodeAttrFunnel)
-	}
-	if hasFunnelAttrs(st.Self) {
-		return nil // already enabled
-	}
 	enableErr := e.enableFeatureInteractive(ctx, "funnel", tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel)
 	st, statusErr := e.getLocalClientStatusWithoutPeers(ctx) // get updated status; interactive flow may block
 	switch {

cmd/tailscale/cli/serve_legacy.go

@@ -818,6 +818,24 @@ func parseServePort(s string) (uint16, error) {
 // 2023-08-09: The only valid feature values are "serve" and "funnel".
 // This can be moved to some CLI lib when expanded past serve/funnel.
 func (e *serveEnv) enableFeatureInteractive(ctx context.Context, feature string, caps ...tailcfg.NodeCapability) (err error) {
+	st, err := e.getLocalClientStatusWithoutPeers(ctx)
+	if err != nil {
+		return fmt.Errorf("getting client status: %w", err)
+	}
+	if st.Self == nil {
+		return errors.New("no self node")
+	}
+	hasCaps := func() bool {
+		for _, c := range caps {
+			if !st.Self.HasCap(c) {
+				return false
+			}
+		}
+		return true
+	}
+	if hasCaps() {
+		return nil // already enabled
+	}
 	info, err := e.lc.QueryFeature(ctx, feature)
 	if err != nil {
 		return err

cmd/tailscale/cli/serve_legacy_test.go

@@ -786,7 +786,7 @@ func TestVerifyFunnelEnabled(t *testing.T) {
 		{
 			name:                 "fallback-flow-enabled",
 			queryFeatureResponse: mockQueryFeatureResponse{resp: nil, err: errors.New("not-allowed")},
-			caps:                 []tailcfg.NodeCapability{tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel},
+			caps:                 []tailcfg.NodeCapability{tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel, "https://tailscale.com/cap/funnel-ports?ports=80,443,8080-8090"},
 			wantErr:              "", // no error, success
 		},
 		{
@@ -811,10 +811,6 @@ func TestVerifyFunnelEnabled(t *testing.T) {
 				defer func() { fakeStatus.Self.Capabilities = oldCaps }() // reset after test
 				fakeStatus.Self.Capabilities = tt.caps
 			}
-			st, err := e.getLocalClientStatusWithoutPeers(ctx)
-			if err != nil {
-				t.Fatal(err)
-			}
 
 			defer func() {
 				r := recover()
@@ -826,7 +822,7 @@ func TestVerifyFunnelEnabled(t *testing.T) {
 					t.Errorf("wrong panic; got=%s, want=%s", gotPanic, tt.wantPanic)
 				}
 			}()
-			gotErr := e.verifyFunnelEnabled(ctx, st, 443)
+			gotErr := e.verifyFunnelEnabled(ctx, 443)
 			var got string
 			if gotErr != nil {
 				got = gotErr.Error()

cmd/tailscale/cli/serve_v2.go

@@ -213,15 +213,10 @@ func (e *serveEnv) runServeCombined(subcmd serveMode) execFunc {
 		ctx, cancel := signal.NotifyContext(ctx, os.Interrupt)
 		defer cancel()
 
-		st, err := e.getLocalClientStatusWithoutPeers(ctx)
-		if err != nil {
-			return fmt.Errorf("getting client status: %w", err)
-		}
-
 		funnel := subcmd == funnel
 		if funnel {
 			// verify node has funnel capabilities
-			if err := e.verifyFunnelEnabled(ctx, st, 443); err != nil {
+			if err := e.verifyFunnelEnabled(ctx, 443); err != nil {
 				return err
 			}
 		}
@@ -246,6 +241,10 @@ func (e *serveEnv) runServeCombined(subcmd serveMode) execFunc {
 		if sc == nil {
 			sc = new(ipn.ServeConfig)
 		}
+		st, err := e.getLocalClientStatusWithoutPeers(ctx)
+		if err != nil {
+			return fmt.Errorf("getting client status: %w", err)
+		}
 		dnsName := strings.TrimSuffix(st.Self.DNSName, ".")
 
 		// set parent serve config to always be persisted