Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[25.0 backport] client: fix connection-errors being shadowed by API version errors #47470

Merged
merged 4 commits into from
Feb 29, 2024
Merged

[25.0 backport] client: fix connection-errors being shadowed by API version errors #47470

merged 4 commits into from
Feb 29, 2024

Commits on Feb 29, 2024

  1. client: fix TestPingWithError 

    This test was added in 27ef09a, which changed
the Ping handling to ignore internal server errors. That case is tested in
TestPingFail, which verifies that we accept the Ping response if a 500
status code was received.

The TestPingWithError test was added to verify behavior if a protocol
(connection) error occurred; however the mock-client returned both a
response, and an error; the error returned would only happen if a connection
error occurred, which means that the server would not provide a reply.

Running the test also shows that returning a response is unexpected, and
ignored:

    === RUN   TestPingWithError
    2024/02/23 14:16:49 RoundTripper returned a response & error; ignoring response
    2024/02/23 14:16:49 RoundTripper returned a response & error; ignoring response
    --- PASS: TestPingWithError (0.00s)
    PASS

This patch updates the test to remove the response.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 349abc6)
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
    @thaJeztah @neersighted
    thaJeztah authored and neersighted committed Feb 29, 2024
    Configuration menu
    Copy the full SHA
    200a2c3 View commit details
    Browse the repository at this point in the history

  2. client: NegotiateAPIVersion: do not ignore (connection) errors from Ping 

    NegotiateAPIVersion was ignoring errors returned by Ping. The intent here
was to handle API responses from a daemon that may be in an unhealthy state,
however this case is already handled by Ping itself.

Ping only returns an error when either failing to connect to the API (daemon
not running or permissions errors), or when failing to parse the API response.

Neither of those should be ignored in this code, or considered a successful
"ping", so update the code to return

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 901b905)
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
    @thaJeztah @neersighted
    thaJeztah authored and neersighted committed Feb 29, 2024
    Configuration menu
    Copy the full SHA
    3ec1946 View commit details
    Browse the repository at this point in the history

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

    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 <github@gone.nl>
(cherry picked from commit 913478b)
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
    @thaJeztah @neersighted
    thaJeztah authored and neersighted committed Feb 29, 2024
    Configuration menu
    Copy the full SHA
    012bfd3 View commit details
    Browse the repository at this point in the history

  4. client: fix connection-errors being shadowed by API version mismatch … 

    …errors

Commit e690724 applied a fix for situations
where the client was configured with API-version negotiation, but did not yet
negotiate a version.

However, the checkVersion() function that was implemented copied the semantics
of cli.NegotiateAPIVersion, which ignored connection failures with the
assumption that connection errors would still surface further down.

However, when using the result of a failed negotiation for NewVersionError,
an API version mismatch error would be produced, masking the actual connection
error.

This patch changes the signature of checkVersion to return unexpected errors,
including failures to connect to the API.

Before this patch:

    docker -H unix:///no/such/socket.sock secret ls
    "secret list" requires API version 1.25, but the Docker daemon API version is 1.24

With this patch applied:

    docker -H unix:///no/such/socket.sock secret ls
    Cannot connect to the Docker daemon at unix:///no/such/socket.sock. Is the docker daemon running?

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 6aea26b)
Conflicts: client/image_list.go
    client/image_list_test.go
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
    @thaJeztah @neersighted
    thaJeztah authored and neersighted committed Feb 29, 2024
    Configuration menu
    Copy the full SHA
    94137f6 View commit details
    Browse the repository at this point in the history