client: doRequest: make sure we return a connection-error

thaJeztah · thaJeztah · commit 913478b42826 · 2024-02-23T15:13:22.000+01:00
This function has various errors that are returned when failing to make a
connection (due to permission issues, TLS mis-configuration, or failing to
resolve the TCP address).

The errConnectionFailed error is currently used as a special case when
processing Ping responses. The current code did not consistently treat
connection errors, and because of that could either absorb the error,
or process the empty response.

Signed-off-by: Sebastiaan van Stijn &lt;github@gone.nl&gt;
diff --git a/client/errors.go b/client/errors.go
@@ -11,15 +11,16 @@ import (
 
 // errConnectionFailed implements an error returned when connection failed.
 type errConnectionFailed struct {
-	host string
+	error
 }
 
 // Error returns a string representation of an errConnectionFailed
-func (err errConnectionFailed) Error() string {
-	if err.host == "" {
-		return "Cannot connect to the Docker daemon. Is the docker daemon running on this host?"
-	}
-	return fmt.Sprintf("Cannot connect to the Docker daemon at %s. Is the docker daemon running?", err.host)
+func (e errConnectionFailed) Error() string {
+	return e.error.Error()
+}
+
+func (e errConnectionFailed) Unwrap() error {
+	return e.error
 }
 
 // IsErrConnectionFailed returns true if the error is caused by connection failed.
@@ -29,7 +30,13 @@ func IsErrConnectionFailed(err error) bool {
 
 // ErrorConnectionFailed returns an error with host in the error message when connection to docker daemon failed.
 func ErrorConnectionFailed(host string) error {
-	return errConnectionFailed{host: host}
+	var err error
+	if host == "" {
+		err = fmt.Errorf("Cannot connect to the Docker daemon. Is the docker daemon running on this host?")
+	} else {
+		err = fmt.Errorf("Cannot connect to the Docker daemon at %s. Is the docker daemon running?", host)
+	}
+	return errConnectionFailed{error: err}
 }
 
 // IsErrNotFound returns true if the error is a NotFound error, which is returned
diff --git a/client/request.go b/client/request.go
@@ -134,17 +134,18 @@ func (cli *Client) sendRequest(ctx context.Context, method, path string, query u
 	return resp, errdefs.FromStatusCode(err, resp.statusCode)
 }
 
+// FIXME(thaJeztah): Should this actually return a serverResp when a connection error occurred?
 func (cli *Client) doRequest(req *http.Request) (serverResponse, error) {
 	serverResp := serverResponse{statusCode: -1, reqURL: req.URL}
 
 	resp, err := cli.client.Do(req)
 	if err != nil {
 		if cli.scheme != "https" && strings.Contains(err.Error(), "malformed HTTP response") {
-			return serverResp, fmt.Errorf("%v.\n* Are you trying to connect to a TLS-enabled daemon without TLS?", err)
+			return serverResp, errConnectionFailed{fmt.Errorf("%v.\n* Are you trying to connect to a TLS-enabled daemon without TLS?", err)}
 		}
 
 		if cli.scheme == "https" && strings.Contains(err.Error(), "bad certificate") {
-			return serverResp, errors.Wrap(err, "the server probably has client authentication (--tlsverify) enabled; check your TLS client certification settings")
+			return serverResp, errConnectionFailed{errors.Wrap(err, "the server probably has client authentication (--tlsverify) enabled; check your TLS client certification settings")}
 		}
 
 		// Don't decorate context sentinel errors; users may be comparing to
@@ -156,12 +157,13 @@ func (cli *Client) doRequest(req *http.Request) (serverResponse, error) {
 		if uErr, ok := err.(*url.Error); ok {
 			if nErr, ok := uErr.Err.(*net.OpError); ok {
 				if os.IsPermission(nErr.Err) {
-					return serverResp, errors.Wrapf(err, "permission denied while trying to connect to the Docker daemon socket at %v", cli.host)
+					return serverResp, errConnectionFailed{errors.Wrapf(err, "permission denied while trying to connect to the Docker daemon socket at %v", cli.host)}
 				}
 			}
 		}
 
 		if nErr, ok := err.(net.Error); ok {
+			// FIXME(thaJeztah): any net.Error should be considered a connection error (but we should include the original error)?
 			if nErr.Timeout() {
 				return serverResp, ErrorConnectionFailed(cli.host)
 			}
@@ -190,7 +192,7 @@ func (cli *Client) doRequest(req *http.Request) (serverResponse, error) {
 			}
 		}
 
-		return serverResp, errors.Wrap(err, "error during connect")
+		return serverResp, errConnectionFailed{errors.Wrap(err, "error during connect")}
 	}
 
 	if resp != nil {

Original file line number	Diff line number	Diff line change
`@@ -134,17 +134,18 @@ func (cli *Client) sendRequest(ctx context.Context, method, path string, query u`
`134`	`134`	`return resp, errdefs.FromStatusCode(err, resp.statusCode)`
`135`	`135`	`}`
`136`	`136`
	`137`	`+// FIXME(thaJeztah): Should this actually return a serverResp when a connection error occurred?`
`137`	`138`	`func (cli Client) doRequest(req http.Request) (serverResponse, error) {`
`138`	`139`	`serverResp := serverResponse{statusCode: -1, reqURL: req.URL}`
`139`	`140`
`140`	`141`	`resp, err := cli.client.Do(req)`
`141`	`142`	`if err != nil {`
`142`	`143`	`if cli.scheme != "https" && strings.Contains(err.Error(), "malformed HTTP response") {`
`143`		`- return serverResp, fmt.Errorf("%v.\n* Are you trying to connect to a TLS-enabled daemon without TLS?", err)`
	`144`	`+ return serverResp, errConnectionFailed{fmt.Errorf("%v.\n* Are you trying to connect to a TLS-enabled daemon without TLS?", err)}`
`144`	`145`	`}`
`145`	`146`
`146`	`147`	`if cli.scheme == "https" && strings.Contains(err.Error(), "bad certificate") {`
`147`		`- return serverResp, errors.Wrap(err, "the server probably has client authentication (--tlsverify) enabled; check your TLS client certification settings")`
	`148`	`+ return serverResp, errConnectionFailed{errors.Wrap(err, "the server probably has client authentication (--tlsverify) enabled; check your TLS client certification settings")}`
`148`	`149`	`}`
`149`	`150`
`150`	`151`	`// Don't decorate context sentinel errors; users may be comparing to`
`@@ -156,12 +157,13 @@ func (cli Client) doRequest(req http.Request) (serverResponse, error) {`
`156`	`157`	`if uErr, ok := err.(*url.Error); ok {`
`157`	`158`	`if nErr, ok := uErr.Err.(*net.OpError); ok {`
`158`	`159`	`if os.IsPermission(nErr.Err) {`
`159`		`- return serverResp, errors.Wrapf(err, "permission denied while trying to connect to the Docker daemon socket at %v", cli.host)`
	`160`	`+ return serverResp, errConnectionFailed{errors.Wrapf(err, "permission denied while trying to connect to the Docker daemon socket at %v", cli.host)}`
`160`	`161`	`}`
`161`	`162`	`}`
`162`	`163`	`}`
`163`	`164`
`164`	`165`	`if nErr, ok := err.(net.Error); ok {`
	`166`	`+ // FIXME(thaJeztah): any net.Error should be considered a connection error (but we should include the original error)?`
`165`	`167`	`if nErr.Timeout() {`
`166`	`168`	`return serverResp, ErrorConnectionFailed(cli.host)`
`167`	`169`	`}`
`@@ -190,7 +192,7 @@ func (cli Client) doRequest(req http.Request) (serverResponse, error) {`
`190`	`192`	`}`
`191`	`193`	`}`
`192`	`194`
`193`		`- return serverResp, errors.Wrap(err, "error during connect")`
	`195`	`+ return serverResp, errConnectionFailed{errors.Wrap(err, "error during connect")}`
`194`	`196`	`}`
`195`	`197`
`196`	`198`	`if resp != nil {`