ipn/ipnlocal: move Ping method from IPN bus to LocalBackend (HTTP)

Change-Id: I61759f1dae8d9d446353db54c8b1e13bfffb3287
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

client/tailscale/localclient.go

@@ -28,6 +28,7 @@ import (
 	"time"
 
 	"go4.org/mem"
+	"inet.af/netaddr"
 	"tailscale.com/client/tailscale/apitype"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnstate"
@@ -662,6 +663,23 @@ func (lc *LocalClient) ExpandSNIName(ctx context.Context, name string) (fqdn str
 	return "", false
 }
 
+// Ping sends a ping of the provided type to the provided IP and waits
+// for its response.
+func (lc *LocalClient) Ping(ctx context.Context, ip netaddr.IP, pingtype tailcfg.PingType) (*ipnstate.PingResult, error) {
+	v := url.Values{}
+	v.Set("ip", ip.String())
+	v.Set("type", string(pingtype))
+	body, err := lc.send(ctx, "POST", "/localapi/v0/ping?"+v.Encode(), 200, nil)
+	if err != nil {
+		return nil, fmt.Errorf("error %w: %s", err, body)
+	}
+	pr := new(ipnstate.PingResult)
+	if err := json.Unmarshal(body, pr); err != nil {
+		return nil, err
+	}
+	return pr, nil
+}
+
 // tailscaledConnectHint gives a little thing about why tailscaled (or
 // platform equivalent) is not answering localapi connections.
 //

cmd/tailscale/cli/ping.go

@@ -16,7 +16,7 @@ import (
 	"time"
 
 	"github.com/peterbourgon/ff/v3/ffcli"
-	"tailscale.com/ipn"
+	"inet.af/netaddr"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
 )
@@ -87,24 +87,10 @@ func runPing(ctx context.Context, args []string) error {
 		os.Exit(1)
 	}
 
-	c, bc, ctx, cancel := connect(ctx)
-	defer cancel()
-
 	if len(args) != 1 || args[0] == "" {
 		return errors.New("usage: ping <hostname-or-IP>")
 	}
 	var ip string
-	prc := make(chan *ipnstate.PingResult, 1)
-	bc.SetNotifyCallback(func(n ipn.Notify) {
-		if n.ErrMessage != nil {
-			fatalf("Notify.ErrMessage: %v", *n.ErrMessage)
-		}
-		if pr := n.PingResult; pr != nil && pr.IP == ip {
-			prc <- pr
-		}
-	})
-	pumpErr := make(chan error, 1)
-	go func() { pumpErr <- pump(ctx, bc, c) }()
 
 	hostOrIP := args[0]
 	ip, self, err := tailscaleIPFromArg(ctx, hostOrIP)
@@ -124,48 +110,47 @@ func runPing(ctx context.Context, args []string) error {
 	anyPong := false
 	for {
 		n++
-		bc.Ping(ip, pingType())
-		timer := time.NewTimer(pingArgs.timeout)
-		select {
-		case <-timer.C:
-			printf("timeout waiting for ping reply\n")
-		case err := <-pumpErr:
-			return err
-		case pr := <-prc:
-			timer.Stop()
-			if pr.Err != "" {
-				if pr.IsLocalIP {
-					outln(pr.Err)
-					return nil
-				}
-				return errors.New(pr.Err)
-			}
-			latency := time.Duration(pr.LatencySeconds * float64(time.Second)).Round(time.Millisecond)
-			via := pr.Endpoint
-			if pr.DERPRegionID != 0 {
-				via = fmt.Sprintf("DERP(%s)", pr.DERPRegionCode)
+		ctx, cancel := context.WithTimeout(ctx, pingArgs.timeout)
+		pr, err := localClient.Ping(ctx, netaddr.MustParseIP(ip), pingType())
+		cancel()
+		if err != nil {
+			if errors.Is(err, context.DeadlineExceeded) {
+				printf("ping %q timed out\n", ip)
+				continue
 			}
-			if via == "" {
-				// TODO(bradfitz): populate the rest of ipnstate.PingResult for TSMP queries?
-				// For now just say which protocol it used.
-				via = string(pingType())
-			}
-			anyPong = true
-			extra := ""
-			if pr.PeerAPIPort != 0 {
-				extra = fmt.Sprintf(", %d", pr.PeerAPIPort)
-			}
-			printf("pong from %s (%s%s) via %v in %v\n", pr.NodeName, pr.NodeIP, extra, via, latency)
-			if pingArgs.tsmp || pingArgs.icmp {
-				return nil
-			}
-			if pr.Endpoint != "" && pingArgs.untilDirect {
+			return err
+		}
+		if pr.Err != "" {
+			if pr.IsLocalIP {
+				outln(pr.Err)
 				return nil
 			}
-			time.Sleep(time.Second)
-		case <-ctx.Done():
-			return ctx.Err()
+			return errors.New(pr.Err)
+		}
+		latency := time.Duration(pr.LatencySeconds * float64(time.Second)).Round(time.Millisecond)
+		via := pr.Endpoint
+		if pr.DERPRegionID != 0 {
+			via = fmt.Sprintf("DERP(%s)", pr.DERPRegionCode)
+		}
+		if via == "" {
+			// TODO(bradfitz): populate the rest of ipnstate.PingResult for TSMP queries?
+			// For now just say which protocol it used.
+			via = string(pingType())
 		}
+		anyPong = true
+		extra := ""
+		if pr.PeerAPIPort != 0 {
+			extra = fmt.Sprintf(", %d", pr.PeerAPIPort)
+		}
+		printf("pong from %s (%s%s) via %v in %v\n", pr.NodeName, pr.NodeIP, extra, via, latency)
+		if pingArgs.tsmp || pingArgs.icmp {
+			return nil
+		}
+		if pr.Endpoint != "" && pingArgs.untilDirect {
+			return nil
+		}
+		time.Sleep(time.Second)
+
 		if n == pingArgs.num {
 			if !anyPong {
 				return errors.New("no reply")

ipn/backend.go

@@ -65,14 +65,13 @@ type Notify struct {
 	// For State InUseOtherUser, ErrMessage is not critical and just contains the details.
 	ErrMessage *string
 
-	LoginFinished *empty.Message       // non-nil when/if the login process succeeded
-	State         *State               // if non-nil, the new or current IPN state
-	Prefs         *Prefs               // if non-nil, the new or current preferences
-	NetMap        *netmap.NetworkMap   // if non-nil, the new or current netmap
-	Engine        *EngineStatus        // if non-nil, the new or urrent wireguard stats
-	BrowseToURL   *string              // if non-nil, UI should open a browser right now
-	BackendLogID  *string              // if non-nil, the public logtail ID used by backend
-	PingResult    *ipnstate.PingResult // if non-nil, a ping response arrived
+	LoginFinished *empty.Message     // non-nil when/if the login process succeeded
+	State         *State             // if non-nil, the new or current IPN state
+	Prefs         *Prefs             // if non-nil, the new or current preferences
+	NetMap        *netmap.NetworkMap // if non-nil, the new or current netmap
+	Engine        *EngineStatus      // if non-nil, the new or urrent wireguard stats
+	BrowseToURL   *string            // if non-nil, UI should open a browser right now
+	BackendLogID  *string            // if non-nil, the public logtail ID used by backend
 
 	// FilesWaiting if non-nil means that files are buffered in
 	// the Tailscale daemon and ready for local transfer to the
@@ -122,9 +121,6 @@ func (n Notify) String() string {
 	if n.BackendLogID != nil {
 		sb.WriteString("BackendLogID ")
 	}
-	if n.PingResult != nil {
-		fmt.Fprintf(&sb, "ping=%v ", *n.PingResult)
-	}
 	if n.FilesWaiting != nil {
 		sb.WriteString("FilesWaiting ")
 	}
@@ -245,8 +241,4 @@ type Backend interface {
 	// counts. Connection events are emitted automatically without
 	// polling.
 	RequestEngineStatus()
-	// Ping attempts to start connecting to the given IP and sends a Notify
-	// with its PingResult. If the host is down, there might never
-	// be a PingResult sent. The cmd/tailscale CLI client adds a timeout.
-	Ping(ip string, pingType tailcfg.PingType)
 }

ipn/fake_test.go

@@ -5,7 +5,6 @@
 package ipn
 
 import (
-	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/netmap"
 )
@@ -99,9 +98,3 @@ func (b *FakeBackend) RequestEngineStatus() {
 		b.notify(Notify{Engine: &EngineStatus{}})
 	}
 }
-
-func (b *FakeBackend) Ping(ip string, pingType tailcfg.PingType) {
-	if b.notify != nil {
-		b.notify(Notify{PingResult: &ipnstate.PingResult{}})
-	}
-}

ipn/ipnlocal/local.go

@@ -1698,15 +1698,20 @@ func (b *LocalBackend) StartLoginInteractive() {
 	}
 }
 
-func (b *LocalBackend) Ping(ipStr string, pingType tailcfg.PingType) {
-	ip, err := netaddr.ParseIP(ipStr)
-	if err != nil {
-		b.logf("ignoring Ping request to invalid IP %q", ipStr)
-		return
-	}
+func (b *LocalBackend) Ping(ctx context.Context, ip netaddr.IP, pingType tailcfg.PingType) (*ipnstate.PingResult, error) {
+	ch := make(chan *ipnstate.PingResult, 1)
 	b.e.Ping(ip, pingType, func(pr *ipnstate.PingResult) {
-		b.send(ipn.Notify{PingResult: pr})
+		select {
+		case ch <- pr:
+		default:
+		}
 	})
+	select {
+	case pr := <-ch:
+		return pr, nil
+	case <-ctx.Done():
+		return nil, ctx.Err()
+	}
 }
 
 // parseWgStatusLocked returns an EngineStatus based on s.

ipn/localapi/localapi.go

@@ -111,6 +111,8 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		h.serveLogout(w, r)
 	case "/localapi/v0/prefs":
 		h.servePrefs(w, r)
+	case "/localapi/v0/ping":
+		h.servePing(w, r)
 	case "/localapi/v0/check-prefs":
 		h.serveCheckPrefs(w, r)
 	case "/localapi/v0/check-ip-forwarding":
@@ -625,6 +627,36 @@ func (h *Handler) serveSetExpirySooner(w http.ResponseWriter, r *http.Request) {
 	io.WriteString(w, "done\n")
 }
 
+func (h *Handler) servePing(w http.ResponseWriter, r *http.Request) {
+	ctx := r.Context()
+	if r.Method != "POST" {
+		http.Error(w, "want POST", 400)
+		return
+	}
+	ipStr := r.FormValue("ip")
+	if ipStr == "" {
+		http.Error(w, "missing 'ip' parameter", 400)
+		return
+	}
+	ip, err := netaddr.ParseIP(ipStr)
+	if err != nil {
+		http.Error(w, "invalid IP", 400)
+		return
+	}
+	pingTypeStr := r.FormValue("type")
+	if ipStr == "" {
+		http.Error(w, "missing 'type' parameter", 400)
+		return
+	}
+	res, err := h.b.Ping(ctx, ip, tailcfg.PingType(pingTypeStr))
+	if err != nil {
+		writeErrorJSON(w, err)
+		return
+	}
+	w.Header().Set("Content-Type", "application/json")
+	json.NewEncoder(w).Encode(res)
+}
+
 func (h *Handler) serveDial(w http.ResponseWriter, r *http.Request) {
 	if r.Method != "POST" {
 		http.Error(w, "POST required", http.StatusMethodNotAllowed)

ipn/message.go

@@ -51,11 +51,6 @@ type SetPrefsArgs struct {
 	New *Prefs
 }
 
-type PingArgs struct {
-	IP   string
-	Type tailcfg.PingType
-}
-
 // Command is a command message that is JSON encoded and sent by a
 // frontend to a backend.
 type Command struct {
@@ -78,7 +73,6 @@ type Command struct {
 	SetPrefs              *SetPrefsArgs
 	RequestEngineStatus   *NoArgs
 	RequestStatus         *NoArgs
-	Ping                  *PingArgs
 }
 
 type BackendServer struct {
@@ -170,9 +164,6 @@ func (bs *BackendServer) GotCommand(ctx context.Context, cmd *Command) error {
 	if c := cmd.RequestEngineStatus; c != nil {
 		bs.b.RequestEngineStatus()
 		return nil
-	} else if c := cmd.Ping; c != nil {
-		bs.b.Ping(c.IP, tailcfg.PingType(c.Type))
-		return nil
 	}
 
 	if IsReadonlyContext(ctx) {
@@ -311,13 +302,6 @@ func (bc *BackendClient) RequestStatus() {
 	bc.send(Command{AllowVersionSkew: true, RequestStatus: &NoArgs{}})
 }
 
-func (bc *BackendClient) Ping(ip string, pingType tailcfg.PingType) {
-	bc.send(Command{Ping: &PingArgs{
-		IP:   ip,
-		Type: pingType,
-	}})
-}
-
 // MaxMessageSize is the maximum message size, in bytes.
 const MaxMessageSize = 10 << 20