Merge pull request #6022 from thaJeztah/connhelper_cleanups_step1

cli/connhelper: cleanups and test improvements

Author: thaJeztah

cli/connhelper/commandconn/commandconn.go

@@ -28,7 +28,6 @@ import (
 	"syscall"
 	"time"
 
-	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
 
@@ -149,7 +148,7 @@ func (c *commandConn) handleEOF(err error) error {
 			c.stderrMu.Lock()
 			stderr := c.stderr.String()
 			c.stderrMu.Unlock()
-			return errors.Errorf("command %v did not exit after %v: stderr=%q", c.cmd.Args, err, stderr)
+			return fmt.Errorf("command %v did not exit after %v: stderr=%q", c.cmd.Args, err, stderr)
 		}
 	}
 
@@ -159,7 +158,7 @@ func (c *commandConn) handleEOF(err error) error {
 	c.stderrMu.Lock()
 	stderr := c.stderr.String()
 	c.stderrMu.Unlock()
-	return errors.Errorf("command %v has exited with %v, make sure the URL is valid, and Docker 18.09 or later is installed on the remote host: stderr=%s", c.cmd.Args, werr, stderr)
+	return fmt.Errorf("command %v has exited with %v, make sure the URL is valid, and Docker 18.09 or later is installed on the remote host: stderr=%s", c.cmd.Args, werr, stderr)
 }
 
 func ignorableCloseError(err error) bool {

cli/connhelper/connhelper.go

@@ -3,13 +3,13 @@ package connhelper
 
 import (
 	"context"
+	"fmt"
 	"net"
 	"net/url"
 	"strings"
 
 	"github.com/docker/cli/cli/connhelper/commandconn"
 	"github.com/docker/cli/cli/connhelper/ssh"
-	"github.com/pkg/errors"
 )
 
 // ConnectionHelper allows to connect to a remote host with custom stream provider binary.
@@ -43,7 +43,7 @@ func getConnectionHelper(daemonURL string, sshFlags []string) (*ConnectionHelper
 	if u.Scheme == "ssh" {
 		sp, err := ssh.ParseURL(daemonURL)
 		if err != nil {
-			return nil, errors.Wrap(err, "ssh host connection is not valid")
+			return nil, fmt.Errorf("ssh host connection is not valid: %w", err)
 		}
 		sshFlags = addSSHTimeout(sshFlags)
 		sshFlags = disablePseudoTerminalAllocation(sshFlags)

cli/connhelper/ssh/ssh.go

@@ -2,19 +2,36 @@
 package ssh
 
 import (
+	"errors"
+	"fmt"
 	"net/url"
-
-	"github.com/pkg/errors"
 )
 
-// ParseURL parses URL
+// ParseURL creates a [Spec] from the given ssh URL. It returns an error if
+// the URL is using the wrong scheme, contains fragments, query-parameters,
+// or contains a password.
 func ParseURL(daemonURL string) (*Spec, error) {
 	u, err := url.Parse(daemonURL)
 	if err != nil {
-		return nil, err
+		var urlErr *url.Error
+		if errors.As(err, &urlErr) {
+			err = urlErr.Unwrap()
+		}
+		return nil, fmt.Errorf("invalid ssh URL: %w", err)
+	}
+	s, err := newSpec(u)
+	if err != nil {
+		return nil, fmt.Errorf("invalid ssh URL: %w", err)
+	}
+	return s, nil
+}
+
+func newSpec(u *url.URL) (*Spec, error) {
+	if u.Scheme == "" {
+		return nil, errors.New("no scheme provided")
 	}
 	if u.Scheme != "ssh" {
-		return nil, errors.Errorf("expected scheme ssh, got %q", u.Scheme)
+		return nil, errors.New("incorrect scheme: " + u.Scheme)
 	}
 
 	var sp Spec
@@ -27,17 +44,18 @@ func ParseURL(daemonURL string) (*Spec, error) {
 	}
 	sp.Host = u.Hostname()
 	if sp.Host == "" {
-		return nil, errors.Errorf("no host specified")
+		return nil, errors.New("hostname is empty")
 	}
 	sp.Port = u.Port()
 	sp.Path = u.Path
 	if u.RawQuery != "" {
-		return nil, errors.Errorf("extra query after the host: %q", u.RawQuery)
+		return nil, fmt.Errorf("query parameters are not allowed: %q", u.RawQuery)
 	}
 	if u.Fragment != "" {
-		return nil, errors.Errorf("extra fragment after the host: %q", u.Fragment)
+		return nil, fmt.Errorf("fragments are not allowed: %q", u.Fragment)
 	}
-	return &sp, err
+
+	return &sp, nil
 }
 
 // Spec of SSH URL

cli/connhelper/ssh/ssh_test.go

@@ -9,59 +9,132 @@ import (
 
 func TestParseURL(t *testing.T) {
 	testCases := []struct {
+		doc           string
 		url           string
+		remoteCommand []string
 		expectedArgs  []string
 		expectedError string
+		expectedSpec  Spec
 	}{
 		{
-			url: "ssh://foo",
+			doc: "bare ssh URL",
+			url: "ssh://example.com",
 			expectedArgs: []string{
-				"--", "foo",
+				"--", "example.com",
+			},
+			expectedSpec: Spec{
+				Host: "example.com",
 			},
 		},
 		{
-			url: "ssh://me@foo:10022",
+			doc: "bare ssh URL and remote command",
+			url: "ssh://example.com",
+			remoteCommand: []string{
+				"docker", "system", "dial-stdio",
+			},
 			expectedArgs: []string{
-				"-l", "me",
-				"-p", "10022",
-				"--", "foo",
+				"--", "example.com",
+				"docker", "system", "dial-stdio",
+			},
+			expectedSpec: Spec{
+				Host: "example.com",
 			},
 		},
 		{
-			url:           "ssh://me:passw0rd@foo",
-			expectedError: "plain-text password is not supported",
+			doc: "ssh URL with path and remote command and flag",
+			url: "ssh://example.com/var/run/docker.sock",
+			remoteCommand: []string{
+				"docker", "--host", "unix:///var/run/docker.sock", "system", "dial-stdio",
+			},
+			expectedArgs: []string{
+				"--", "example.com",
+				"docker", "--host", "unix:///var/run/docker.sock", "system", "dial-stdio",
+			},
+			expectedSpec: Spec{
+				Host: "example.com",
+				Path: "/var/run/docker.sock",
+			},
 		},
 		{
-			url: "ssh://foo/bar",
+			doc: "ssh URL with username and port",
+			url: "ssh://me@example.com:10022",
 			expectedArgs: []string{
-				"--", "foo",
+				"-l", "me",
+				"-p", "10022",
+				"--", "example.com",
+			},
+			expectedSpec: Spec{
+				User: "me",
+				Host: "example.com",
+				Port: "10022",
 			},
 		},
 		{
-			url:           "ssh://foo?bar",
-			expectedError: `extra query after the host: "bar"`,
+			doc: "ssh URL with username, port, and path",
+			url: "ssh://me@example.com:10022/var/run/docker.sock",
+			expectedArgs: []string{
+				"-l", "me",
+				"-p", "10022",
+				"--", "example.com",
+			},
+			expectedSpec: Spec{
+				User: "me",
+				Host: "example.com",
+				Port: "10022",
+				Path: "/var/run/docker.sock",
+			},
+		},
+		{
+			doc:           "malformed URL",
+			url:           "malformed %%url",
+			expectedError: `invalid ssh URL: invalid URL escape "%%u"`,
+		},
+		{
+			doc:           "URL missing scheme",
+			url:           "no-scheme.example.com",
+			expectedError: "invalid ssh URL: no scheme provided",
 		},
 		{
-			url:           "ssh://foo#bar",
-			expectedError: `extra fragment after the host: "bar"`,
+			doc:           "invalid URL with password",
+			url:           "ssh://me:passw0rd@example.com",
+			expectedError: "invalid ssh URL: plain-text password is not supported",
 		},
 		{
+			doc:           "invalid URL with query parameter",
+			url:           "ssh://example.com?foo=bar&bar=baz",
+			expectedError: `invalid ssh URL: query parameters are not allowed: "foo=bar&bar=baz"`,
+		},
+		{
+			doc:           "invalid URL with fragment",
+			url:           "ssh://example.com#bar",
+			expectedError: `invalid ssh URL: fragments are not allowed: "bar"`,
+		},
+		{
+			doc:           "invalid URL without hostname",
 			url:           "ssh://",
-			expectedError: "no host specified",
+			expectedError: "invalid ssh URL: hostname is empty",
+		},
+		{
+			url:           "ssh:///no-hostname",
+			expectedError: "invalid ssh URL: hostname is empty",
 		},
 		{
-			url:           "foo://bar",
-			expectedError: `expected scheme ssh, got "foo"`,
+			doc:           "invalid URL with unsupported scheme",
+			url:           "https://example.com",
+			expectedError: `invalid ssh URL: incorrect scheme: https`,
 		},
 	}
 	for _, tc := range testCases {
-		t.Run(tc.url, func(t *testing.T) {
+		t.Run(tc.doc, func(t *testing.T) {
 			sp, err := ParseURL(tc.url)
 			if tc.expectedError == "" {
 				assert.NilError(t, err)
-				assert.Check(t, is.DeepEqual(tc.expectedArgs, sp.Args()))
+				actualArgs := sp.Args(tc.remoteCommand...)
+				assert.Check(t, is.DeepEqual(actualArgs, tc.expectedArgs))
+				assert.Check(t, is.Equal(*sp, tc.expectedSpec))
 			} else {
-				assert.ErrorContains(t, err, tc.expectedError)
+				assert.Check(t, is.Error(err, tc.expectedError))
+				assert.Check(t, is.Nil(sp))
 			}
 		})
 	}