From 2e956713deed73822db6b82a37aaced91a05bea3 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Thu, 21 Dec 2023 14:55:14 -0600 Subject: [PATCH] safesocket: remove ConnectionStrategy (#10662) This type seems to be a migration shim for TCP tailscaled sockets (instead of unix/windows pipes). The `port` field was never set, so it was effectively used as a string (`path` field). Remove the whole type and simplify call sites to pass the socket path directly to `safesocket.Connect`. Updates #cleanup Signed-off-by: Andrew Lytvynov --- client/tailscale/localclient.go | 3 +- logpolicy/logpolicy.go | 2 +- safesocket/basic_test.go | 3 +- safesocket/pipe_windows.go | 4 +-- safesocket/pipe_windows_test.go | 3 +- safesocket/safesocket.go | 48 ++------------------------ safesocket/safesocket_js.go | 2 +- safesocket/safesocket_plan9.go | 6 ++-- safesocket/unixsocket.go | 4 +-- tstest/integration/integration_test.go | 3 +- 10 files changed, 16 insertions(+), 62 deletions(-) diff --git a/client/tailscale/localclient.go b/client/tailscale/localclient.go index 7b99fc61e..cb8670d8b 100644 --- a/client/tailscale/localclient.go +++ b/client/tailscale/localclient.go @@ -102,8 +102,7 @@ func (lc *LocalClient) defaultDialer(ctx context.Context, network, addr string) return d.DialContext(ctx, "tcp", "127.0.0.1:"+strconv.Itoa(port)) } } - s := safesocket.DefaultConnectionStrategy(lc.socket()) - return safesocket.Connect(s) + return safesocket.Connect(lc.socket()) } // DoLocalRequest makes an HTTP request to the local machine's Tailscale daemon. diff --git a/logpolicy/logpolicy.go b/logpolicy/logpolicy.go index 7d22362ae..2c41c4664 100644 --- a/logpolicy/logpolicy.go +++ b/logpolicy/logpolicy.go @@ -714,7 +714,7 @@ func dialContext(ctx context.Context, netw, addr string, netMon *netmon.Monitor, } if version.IsWindowsGUI() && strings.HasPrefix(netw, "tcp") { - if c, err := safesocket.Connect(safesocket.DefaultConnectionStrategy("")); err == nil { + if c, err := safesocket.Connect(""); err == nil { fmt.Fprintf(c, "CONNECT %s HTTP/1.0\r\n\r\n", addr) br := bufio.NewReader(c) res, err := http.ReadResponse(br, nil) diff --git a/safesocket/basic_test.go b/safesocket/basic_test.go index fca1c3f09..31c54ac7b 100644 --- a/safesocket/basic_test.go +++ b/safesocket/basic_test.go @@ -57,8 +57,7 @@ func TestBasics(t *testing.T) { }() go func() { - s := DefaultConnectionStrategy(sock) - c, err := Connect(s) + c, err := Connect(sock) if err != nil { errs <- err return diff --git a/safesocket/pipe_windows.go b/safesocket/pipe_windows.go index 1259b7834..dd6d521b6 100644 --- a/safesocket/pipe_windows.go +++ b/safesocket/pipe_windows.go @@ -17,13 +17,13 @@ "golang.org/x/sys/windows" ) -func connect(s *ConnectionStrategy) (net.Conn, error) { +func connect(path string) (net.Conn, error) { dl := time.Now().Add(20 * time.Second) ctx, cancel := context.WithDeadline(context.Background(), dl) defer cancel() // We use the identification impersonation level so that tailscaled may // obtain information about our token for access control purposes. - return winio.DialPipeAccessImpLevel(ctx, s.path, windows.GENERIC_READ|windows.GENERIC_WRITE, winio.PipeImpLevelIdentification) + return winio.DialPipeAccessImpLevel(ctx, path, windows.GENERIC_READ|windows.GENERIC_WRITE, winio.PipeImpLevelIdentification) } func setFlags(network, address string, c syscall.RawConn) error { diff --git a/safesocket/pipe_windows_test.go b/safesocket/pipe_windows_test.go index c6f8eb393..2080d1934 100644 --- a/safesocket/pipe_windows_test.go +++ b/safesocket/pipe_windows_test.go @@ -80,8 +80,7 @@ func TestExpectedWindowsTypes(t *testing.T) { }() go func() { - s := DefaultConnectionStrategy(sock) - c, err := Connect(s) + c, err := Connect(sock) if err != nil { errs <- err return diff --git a/safesocket/safesocket.go b/safesocket/safesocket.go index 43936f6d5..379b934ca 100644 --- a/safesocket/safesocket.go +++ b/safesocket/safesocket.go @@ -52,52 +52,10 @@ func tailscaledStillStarting() bool { return tailscaledProcExists() } -// A ConnectionStrategy is a plan for how to connect to tailscaled or equivalent -// (e.g. IPNExtension on macOS). -// -// This is a struct because prior to Tailscale 1.34.0 it was more complicated -// and there were multiple protocols that could be used. See LocalClient's -// dialer for what happens in practice these days (2022-11-28). -// -// TODO(bradfitz): we can remove this struct now and revert this package closer -// to its original smaller API. -type ConnectionStrategy struct { - path string // unix socket path - port uint16 // TCP port - - // Longer term, a ConnectionStrategy should be an ordered list of things to attempt, - // with just the information required to connection for each. - // - // We have at least these cases to consider (see issue 3530): - // - // tailscale sandbox | tailscaled sandbox | OS | connection - // ------------------|--------------------|---------|----------- - // no | no | unix* | unix socket *includes tailscaled on darwin - // no | no | Windows | TCP/port - // no | no | wasm | memconn - // no | Network Extension | macOS | TCP/port/token, port/token from lsof - // no | System Extension | macOS | TCP/port/token, port/token from lsof - // yes | Network Extension | macOS | TCP/port/token, port/token from readdir - // yes | System Extension | macOS | TCP/port/token, port/token from readdir - // - // Note e.g. that port is only relevant as an input to Connect on Windows, - // that path is not relevant to Windows, and that neither matters to wasm. -} - -// DefaultConnectionStrategy returns a default connection strategy. -// The default strategy is to attempt to connect in as many ways as possible. -// It uses path as the unix socket path, when applicable, -// and defaults to WindowsLocalPort for the TCP port when applicable. -// It falls back to auto-discovery across sandbox boundaries on macOS. -// TODO: maybe take no arguments, since path is irrelevant on Windows? Discussion in PR 3499. -func DefaultConnectionStrategy(path string) *ConnectionStrategy { - return &ConnectionStrategy{path: path} -} - -// Connect connects to tailscaled using s -func Connect(s *ConnectionStrategy) (net.Conn, error) { +// Connect connects to tailscaled using a unix socket or named pipe. +func Connect(path string) (net.Conn, error) { for { - c, err := connect(s) + c, err := connect(path) if err != nil && tailscaledStillStarting() { time.Sleep(250 * time.Millisecond) continue diff --git a/safesocket/safesocket_js.go b/safesocket/safesocket_js.go index d29be95ed..396938355 100644 --- a/safesocket/safesocket_js.go +++ b/safesocket/safesocket_js.go @@ -15,6 +15,6 @@ func listen(path string) (net.Listener, error) { return memconn.Listen("memu", memName) } -func connect(_ *ConnectionStrategy) (net.Conn, error) { +func connect(_ string) (net.Conn, error) { return memconn.Dial("memu", memName) } diff --git a/safesocket/safesocket_plan9.go b/safesocket/safesocket_plan9.go index 445963394..32095bbd2 100644 --- a/safesocket/safesocket_plan9.go +++ b/safesocket/safesocket_plan9.go @@ -85,13 +85,13 @@ func (fc plan9FileConn) SetWriteDeadline(t time.Time) error { return syscall.EPLAN9 } -func connect(s *ConnectionStrategy) (net.Conn, error) { - f, err := os.OpenFile(s.path, os.O_RDWR, 0666) +func connect(path string) (net.Conn, error) { + f, err := os.OpenFile(path, os.O_RDWR, 0666) if err != nil { return nil, err } - return plan9FileConn{name: s.path, file: f}, nil + return plan9FileConn{name: path, file: f}, nil } // Create an entry in /srv, open a pipe, write the diff --git a/safesocket/unixsocket.go b/safesocket/unixsocket.go index 0a0dd485c..8a7162408 100644 --- a/safesocket/unixsocket.go +++ b/safesocket/unixsocket.go @@ -16,11 +16,11 @@ "runtime" ) -func connect(s *ConnectionStrategy) (net.Conn, error) { +func connect(path string) (net.Conn, error) { if runtime.GOOS == "js" { return nil, errors.New("safesocket.Connect not yet implemented on js/wasm") } - return net.Dial("unix", s.path) + return net.Dial("unix", path) } func listen(path string) (net.Listener, error) { diff --git a/tstest/integration/integration_test.go b/tstest/integration/integration_test.go index 23cd1059b..f724d2a4e 100644 --- a/tstest/integration/integration_test.go +++ b/tstest/integration/integration_test.go @@ -1226,9 +1226,8 @@ func (n *testNode) Ping(otherNode *testNode) error { // over its localhost IPC mechanism. (Unix socket, etc) func (n *testNode) AwaitListening() { t := n.env.t - s := safesocket.DefaultConnectionStrategy(n.sockFile) if err := tstest.WaitFor(20*time.Second, func() (err error) { - c, err := safesocket.Connect(s) + c, err := safesocket.Connect(n.sockFile) if err == nil { c.Close() }