Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

Use Docker CLI socket from home #2171

Merged
merged 2 commits into from
Jul 22, 2022

Conversation

p1-0tr
Copy link
Contributor

@p1-0tr p1-0tr commented Jul 8, 2022

What I did

We should not rely on having a global path for the Docker CLI socket. On
macOS this forces Docker Desktop to access directories which require
raised privileges. Whereas on Linux we do not create sockets in that
location at all, currently. So look for the Docker CLI socket in the
User's home directory.

Related issue

(not mandatory) A picture of a cute animal, if possible in relation with what you did


func init() {
// Attempt to retrieve the Docker CLI socket for the current user.
if home, err := os.UserHomeDir(); err == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to take into account situations where HOME isn't set? I know the docker cli uses that information, and falls back to looking up the user's config; https://github.com/moby/moby/blob/686be57d0a6e514c0cddb2f3ac9cbb3cbef87f5f/pkg/homedir/homedir_unix.go#L25-L33

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point :) thanks


/*
Copyright 2020 Docker Compose CLI authors
Copyright 2020, 2022 Docker Compose CLI authors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the validation currently doesn't allow for the years to differ between files, so all should have the same years

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah :( I even did a quick google for some more decent copyright header checker, but couldn't find one which does the right thing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In https://github.com/docker/cli-docs-tool/, we've been trying Google addlicense, which seemed to work (a bit more "loosely" in checking); see docker/cli-docs-tool#4

@p1-0tr p1-0tr force-pushed the ps-docker-cli-sock-from-home branch 2 times, most recently from 17bfc3c to 4d4b6cf Compare July 8, 2022 14:04
@p1-0tr p1-0tr requested a review from thaJeztah July 8, 2022 14:23
@p1-0tr p1-0tr force-pushed the ps-docker-cli-sock-from-home branch from 4d4b6cf to 718f596 Compare July 13, 2022 07:38
.gitignore Outdated
@@ -1,3 +1,24 @@
bin/
dist/
/.vscode/

## VIM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t all that be in your global gitignore on your machine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that will work as well. Putting those in project-specific configs is force of habit for me.

@mat007 mat007 requested a review from glours July 22, 2022 07:38
@p1-0tr p1-0tr force-pushed the ps-docker-cli-sock-from-home branch from 718f596 to 1798796 Compare July 22, 2022 09:08
@@ -30,6 +31,10 @@ var (
socket = `\\.\pipe\docker_cli`
)

func init() {
overrideSocket() // nop, unless built for e2e testing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean

Suggested change
overrideSocket() // nop, unless built for e2e testing
overrideSocket() // no-op, unless built for e2e testing

or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nop is the mnemonic for no-op in most assemblers (and tends to be used as a neologism for "do nothing"), but yeah that would be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

@mat007 mat007 requested a review from djs55 July 22, 2022 09:19
Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, I'm just waiting for an approval from someone of the Pos team to merge it

We should not rely on having a global path for the Docker CLI socket. On
macOS this forces Docker Desktop to access directories which require
raised privileges. Whereas on Linux we do not create sockets in that
location at all, currently. So look for the Docker CLI socket in the
User's home directory.

Signed-off-by: Piotr Stankiewicz <piotr.stankiewicz@docker.com>
Signed-off-by: Piotr Stankiewicz <piotr.stankiewicz@docker.com>
@p1-0tr p1-0tr force-pushed the ps-docker-cli-sock-from-home branch from 1798796 to 19ac0ad Compare July 22, 2022 09:31
@p1-0tr p1-0tr requested a review from mat007 July 22, 2022 09:34
func init() {
// Attempt to retrieve the Docker CLI socket for the current user.
if home := homedir.Get(); home != "" {
socket = filepath.Join(home, "/Library/Containers/com.docker.docker/Data/docker-cli.sock")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use a shorter Linux-style path in $HOME/.docker/desktop/docker-cli.sock even on Darwin because we've had some problems where the maximum path in the sockaddr is 104 characters (even shorter than Linux's 108 characters). We work around this in some of our binaries by calling os.Chdir("Library/Containers/com.docker.docker") and then using a relative path... but I suspect this trick doesn't work for the CLI because changing the current directory probably breaks something.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is a tough one.

The best thing I can think of from the top of my head is:

pushd ...
connect with relative path
popd

but not sure that would be safe to do.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reflecting on it a bit, I'm not as concerned as I was because the socket is best-effort (and it will work probably 99% of the time). So I'm happy with it as-is.

@glours glours merged commit 471888f into docker-archive:main Jul 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants