From 9107b5eadf92c5a3ed5702f03f87ae9e05fd6e89 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Thu, 26 Oct 2023 10:38:08 -0700 Subject: [PATCH] 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 Signed-off-by: Maisem Ali --- cmd/tailscale/cli/funnel.go | 19 ++++++------------- cmd/tailscale/cli/serve_legacy.go | 18 ++++++++++++++++++ cmd/tailscale/cli/serve_legacy_test.go | 8 ++------ cmd/tailscale/cli/serve_v2.go | 11 +++++------ 4 files changed, 31 insertions(+), 25 deletions(-) diff --git a/cmd/tailscale/cli/funnel.go b/cmd/tailscale/cli/funnel.go index 6086646fe..17d362993 100644 --- a/cmd/tailscale/cli/funnel.go +++ b/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 { - hasFunnelAttrs := func(selfNode *ipnstate.PeerStatus) bool { - return selfNode.HasCap(tailcfg.CapabilityHTTPS) && selfNode.HasCap(tailcfg.NodeAttrFunnel) - } - if hasFunnelAttrs(st.Self) { - return nil // already enabled - } +func (e *serveEnv) verifyFunnelEnabled(ctx context.Context, port uint16) error { enableErr := e.enableFeatureInteractive(ctx, "funnel", tailcfg.CapabilityHTTPS, tailcfg.NodeAttrFunnel) st, statusErr := e.getLocalClientStatusWithoutPeers(ctx) // get updated status; interactive flow may block switch { diff --git a/cmd/tailscale/cli/serve_legacy.go b/cmd/tailscale/cli/serve_legacy.go index a1803c6dd..e6e18669d 100644 --- a/cmd/tailscale/cli/serve_legacy.go +++ b/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 diff --git a/cmd/tailscale/cli/serve_legacy_test.go b/cmd/tailscale/cli/serve_legacy_test.go index f77106ddd..58139ecc2 100644 --- a/cmd/tailscale/cli/serve_legacy_test.go +++ b/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() diff --git a/cmd/tailscale/cli/serve_v2.go b/cmd/tailscale/cli/serve_v2.go index 76677a299..0e0c37d84 100644 --- a/cmd/tailscale/cli/serve_v2.go +++ b/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