client: fix connection-errors being shadowed by API version errors#47440
Merged
neersighted merged 4 commits intomoby:masterfrom Feb 28, 2024
Merged
client: fix connection-errors being shadowed by API version errors#47440neersighted merged 4 commits intomoby:masterfrom
neersighted merged 4 commits intomoby:masterfrom
Conversation
Member
Author
|
Ah! I somewhat expected this one on Windows; I guess either need to use a const ( |
Member
Author
|
Hm... |
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
…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 <[email protected]>
1c8ae55 to
6aea26b
Compare
Member
Author
|
This one's green now, but definitely appreciate a good look at my changes; @neersighted @cpuguy83 @vvoland (unfortunately, had to go down the rabbit-hole on changing the error, otherwise |
neersighted
approved these changes
Feb 28, 2024
This was referenced Feb 29, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
docker image prune --all --forceis failing in Version 25.0.1 docker/cli#4852client: fix TestPingWithError
This test was added in 27ef09a (#33827), 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:
This patch updates the test to remove the response.
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
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.
client: fix connection-errors being shadowed by API version mismatch errors
Commit e690724 (#46463) 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:
With this patch applied:
- Description for the changelog
- Fix a regression that caused API socket connection failures to report an API version negotiation failure instead.- A picture of a cute animal (not mandatory but encouraged)