client: minor cleanup, linting- and bug-fixes#47881
Conversation
client/request.go
Outdated
| if f, elevatedErr := os.Open(`\\.\PHYSICALDRIVE0`); elevatedErr == nil { | ||
| _ = f.Close() | ||
| err = errors.Wrap(err, "in the default daemon configuration on Windows, the docker client must be run with elevated privileges to connect") | ||
| } else { | ||
| f.Close() | ||
| err = errors.Wrap(err, "this error may indicate that the docker daemon is not running") |
There was a problem hiding this comment.
I'm actually wondering if the condition as a whole is flipped, but I don't have a Windows machine to try;
From reading the code, I THINK an error would be returned if the use is NOT privileged; in that case, opening \\.\PHYSICALDRIVE0 would fail, and the "try running with elevated privileges" would be the error to show.
If that does not fail, and the most likely reason for the original error was that the daemon was not running.
All that said, I wonder if this whole code should be handled elsewhere; after all, we should know how the client is trying to connect, so I assume we can check there if the user has privileges to open the named pipe?
There was a problem hiding this comment.
Looking at the 'How to verify' section in #39588 (comment), I'm pretty sure you're right. The code doesn't match the intended behavior.
Once you replace elevatedErr == nil with elevatedErr != nil, f.Close() is located in the right branch. Maybe worth fixing that instead?
There was a problem hiding this comment.
Yup! Thanks for confirming my suspicion; I updated the PR with that change; PTAL
This comment was marked as resolved.
This comment was marked as resolved.
thaJeztah
force-pushed
the
client_cleanups
branch
from
ea3a5df to
d797384
Compare
Make this code slightly more idiomatic, and make it clear in what cases we don't return an actual response, but an empty / default struct. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
commit 1a5dafb improved the error messages produced by adding a check if the client is using as an elevated user. For this, it attempts to open `\\.\PHYSICALDRIVE0`. However, it looks like closing the file landed in the wrong branch of the condition, so the file-handle would not be closed when the os.Open succeeded. Looking further into this check, it appears the conditions were reversed; if the check _fails_, it means the user is not running with elevated permissions, but the check would use elevatedErr == nil. Fix both by changing the condition to `elevatedErr != nil`. While at it, also changing the string to use a string-literal, to reduce the amount of escaping needed. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
client_cleanups
branch
from
d797384 to
e6f41e2
Compare
|
Interesting failure in setup-buildx-action; https://github.com/moby/moby/actions/runs/9347176230/job/25723674583?pr=47881 |
thaJeztah
added
kind/bugfix
2 participants
client: Client.NetworkInspectWithRaw: minor cleanup
Make this code slightly more idiomatic, and make it clear in what cases
we don't return an actual response, but an empty / default struct.
client: ensureReaderClosed: make linters happier
client: Client.doRequest: fix closing filehandle and reversed errors
commit 1a5dafb (#39588) improved the error messages
produced by adding a check if the client is using as an elevated user. For
this, it attempts to open
\\.\PHYSICALDRIVE0.However, it looks like closing the file landed in the wrong branch of the
condition, so the file-handle would not be closed when the os.Open succeeded.
Looking further into this check, it appears the conditions were reversed;
if the check fails, it means the user is not running with elevated
permissions, but the check would use elevatedErr == nil.
Fix both by changing the condition to
elevatedErr != nil.While at it, also changing the string to use a string-literal, to reduce
the amount of escaping needed.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)