Change docker context to default in MacOS #19046

vyasgun · 2023-06-29T10:29:00Z

podman-mac-helper maps requests coming to /var/run/docker.sock (or the default context) to the podman socket. Docker context should be set to default for this to work. Added the code to change context to default during machine start in case of MacOS.

Fixes: #16548

Does this PR introduce a user-facing change?

No

NONE

openshift-ci · 2023-06-29T10:29:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vyasgun
Once this PR has been reviewed and has the lgtm label, please assign jwhonce for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrothberg

@Luap99 @n1hility PTAL

vrothberg · 2023-06-29T12:52:31Z

cmd/podman-mac-helper/service.go

+	// Run the command
+	err = cmd.Run()
+	if err != nil && !errors.Is(err, exec.ErrNotFound) {
+		fmt.Print(fail)


Question: I am largely unfamiliar with this helper but it seems strange to suppress the errors.

What happens if errors.Is(err, exec.ErrNotFound)? Is it intentional to return success?

I added this check because docker binary not existing in any of the paths might not be considered an error but on second thought, maybe it shouldn't be suppressed.

I don't see how this can even work, this is a privileged helper AFAICT so you would execute docker as root here and not in the correct user context. If this is really need AFAICT it must happen in pkg/machine/qemu were it connects to the claim helper. cc @n1hility

Right we should avoid running this as root. It needs to be done outside of the privileged helper, and ran on behalf of the user's account. It should live in the podman machine docker sock network setup logic

So is it a feasible solution to redirect$HOME/.docker/run/docker.sock as well in the podman-mac-helper service?

I think the approach of changing the context is still the right one, just we should do that in podman machine start (as well as a future on the fly dynamic command if added) instead. (see my other comment here: #19046 (comment))

Luap99 · 2023-06-30T08:47:19Z

Why do we touch any docker config at all? Redirecting a socket is one thing but actively changing docker config on each startup without user consent seems like a bad idea?

Reading #16548 this asks for redirecting the $HOME/.docker/run/docker.sock socket as well. I don't see how calling the this docker command changes anything?

But my biggest problem is that there is not user documentation about how this helper works. The package installer installs this service by default which means users will have no clue about what is happening if we start chaning the docker context behind there backs. There are no docs about this tool so users are just left on its own to figure out why we broke his docker install.

vyasgun · 2023-07-03T05:37:54Z

@Luap99 thanks for the review. The main issue is that the user doesn't want to run into an error while using docker commands but wants them to be directed to podman. This is resolved by changing the context to default as podman-mac-helper is already linking the default docker socket and the podman socket.

n1hility · 2023-07-11T13:11:56Z

Reading #16548 this asks for redirecting the $HOME/.docker/run/docker.sock socket as well. I don't see how calling the this docker command changes anything?

Right IMO we should not mess with $HOME/.docker/run/docker.sock as it would make a running docker unusable (e.g. no ability to copy between the two), and likely bother users.

Why do we touch any docker config at all? Redirecting a socket is one thing but actively changing docker config on each startup without user consent seems like a bad idea?

The underlying problem is that if users installed one particular old version of docker desktop, it generated config pointing to the non-common global location (There are other conditions that can happen, but in the default and most common case the context points to the default /var/run/docker.sock). So I suggested that if the user is starting podman machine, and we are already claiming the socket, they likely intend this to work, so we could just "fix" the context. A context of default works on both Docker and Podman machine (unless they did a custom setup that disabled /var/rundocker.sock) We probably should have a flag that can disable taking it back in case it does get annoying, but IMO its most likely what people want.

vyasgun · 2023-07-12T09:28:14Z

@n1hility That sounds good to me. I will update the PR accordingly.

flouthoc · 2023-07-12T10:15:44Z

cmd/podman/machine/start.go

@@ -79,7 +83,23 @@ func start(_ *cobra.Command, args []string) error {
 	if err := vm.Start(vmName, startOpts); err != nil {
 		return err
 	}
+	if err := changeDockerContext(); err != nil {


If there is a agreement on this solution, there are few point which I think should be done:

This should be only done for users who want this, maybe additional hidden flag ? Then podman-desktop can use this hidden flag by default --change-docker-context ? or something like that.

After that should this be only done if docker desktop exists on host and has a version >=4.13 ? Not for everybody since problem only persists for new users ?

If there is a agreement on this solution, there are few point which I think should be done:

This should be only done for users who want this, maybe additional hidden flag ? Then podman-desktop can use this hidden flag by default --change-docker-context ? or something like that.

I agree that we at least need a way to limit/disable the behavior, not sure on opt-in vs opt-out. There is also a few other ways we could go about this:

podman machine start detects and warns but does not fix. This wouldn't help podman desktop though, so it would likely mean that desktop would need to have its own detection. One benefit of that is that podman desktop could have a pop-up to ask if the context should be corrected

Instead of doing this in start(), we move the check/fix to the respective installers (mac + brew). The advantage here is that 90+% of the time this is a one-time fix, so this would save wasted CPU cycles on likely unnecessary checks

We don't address it all, and instead Podman Desktop handles this.

After that should this be only done if docker desktop exists on host and has a version >=4.13 ? Not for everybody since problem only persists for new users ?

Basically everyone is affected that ever installed 4.13.0 is broken forever until they change it back. 4.13.1 and onward works fine on fresh installs. The other way this can happen is an advanced install where the symlink is disabled.

In short the only way we can detect it is to examine the current context (either parsing the file or via the docker CLI)

I don't understand the suggestion about moving the check/fix from start to installers. As Docker Desktop changes the context to $HOME/.docker/run/docker.sock each time it starts, it seems more convenient to me to have the context change command executed when the user starts the Podman machine.

In all the versions I looked at this setting is only set during install / first launch. On all current versions, if you go with the defaults the docker cli will use /var/run/docker.sock (which symlinks to the user home sock), If you ever installed 4.13.0 though, it would have written a different current context value to the ~/.docker/config.json, which sticks around until it's removed or changed. The other scenario which sets the context is if you pick Advanced settings and you disable /var/run/docker sock, at which point it will also write a context value to config.json.

n1hility · 2023-07-13T03:10:50Z

cmd/podman/machine/start.go

+	}
+	path := os.Getenv("PATH")
+	path = strings.Join([]string{path, "/usr/bin", "/usr/local/bin"}, ":")
+	os.Setenv("PATH", path)


Instead of changing the PATH globally for the whole process (which can cause side-effects on other areas of the code-base), a better approach would be to search a list of paths, which also has the added benefit to verify that docker binary exists before invoking it. Once found, the fully qualified joined absolute path could be used to invoke the specific location without needing a PATH change.

I made the suggested change

n1hility · 2023-07-13T03:14:14Z

@vyasgun Thanks for working on this!

Luap99 · 2023-07-13T08:09:53Z

cmd/podman/machine/start.go

+	pathList := strings.Split(os.Getenv("PATH"), ":")
+	var dockerBinaryPath string
+	for _, path := range pathList {
+		dockerPath := filepath.Join(path, "docker")
+		if info, err := os.Stat(dockerPath); err == nil && info.Mode().IsRegular() {
+			dockerBinaryPath = dockerPath
+			break
+		}
+	}
+	if dockerBinaryPath == "" {
+		return fmt.Errorf("docker binary not found")
+	}
+
+	cmd := exec.Command(dockerBinaryPath, "context", "use", "default")


I don't understand this? Why do we not directly execute docker, exec.Command() will already search PATH.

This will prevent the binary not found error as we will only invoke it when we find it in the path.

cmd/podman/machine/start.go

Fixes: containers#1654 Signed-off-by: vyasgun <vyasgun20@gmail.com>

github-actions · 2023-09-09T00:06:24Z

A friendly reminder that this PR had no activity for 30 days.

rhatdan · 2023-09-09T08:10:27Z

@vyasgun whats up with this PR?

vrothberg

Friendly ping. What should we do with the PR?

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels Jun 29, 2023

vyasgun force-pushed the pr/mac-docker-sock branch 2 times, most recently from 04366c5 to 1bd8e4c Compare June 29, 2023 12:11

vrothberg reviewed Jun 29, 2023

View reviewed changes

vyasgun force-pushed the pr/mac-docker-sock branch from 1bd8e4c to df2372a Compare July 12, 2023 10:03

flouthoc reviewed Jul 12, 2023

View reviewed changes

vyasgun force-pushed the pr/mac-docker-sock branch from df2372a to 7f97614 Compare July 12, 2023 10:32

vyasgun requested a review from n1hility July 12, 2023 14:06

n1hility reviewed Jul 13, 2023

View reviewed changes

vyasgun force-pushed the pr/mac-docker-sock branch from 7f97614 to b5d287a Compare July 13, 2023 07:36

Luap99 reviewed Jul 13, 2023

View reviewed changes

Change docker context to default in MacOS during machine start

4a5582b

Fixes: containers#1654 Signed-off-by: vyasgun <vyasgun20@gmail.com>

vyasgun force-pushed the pr/mac-docker-sock branch from b5d287a to 4a5582b Compare August 8, 2023 08:17

github-actions bot added the stale-pr label Sep 9, 2023

github-actions bot removed the stale-pr label Sep 11, 2023

vrothberg reviewed Oct 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change docker context to default in MacOS #19046

Change docker context to default in MacOS #19046

vyasgun commented Jun 29, 2023 •

edited

Loading

openshift-ci bot commented Jun 29, 2023

vrothberg left a comment

vrothberg Jun 29, 2023

vyasgun Jun 29, 2023

Luap99 Jun 30, 2023

n1hility Jul 11, 2023

vyasgun Jul 11, 2023

n1hility Jul 11, 2023

Luap99 commented Jun 30, 2023

vyasgun commented Jul 3, 2023

n1hility commented Jul 11, 2023

vyasgun commented Jul 12, 2023

flouthoc Jul 12, 2023

n1hility Jul 12, 2023 •

edited

Loading

vyasgun Jul 13, 2023

n1hility Jul 16, 2023

n1hility Jul 13, 2023

vyasgun Jul 13, 2023

n1hility commented Jul 13, 2023

Luap99 Jul 13, 2023

vyasgun Aug 8, 2023

github-actions bot commented Sep 9, 2023

rhatdan commented Sep 9, 2023

vrothberg left a comment

Change docker context to default in MacOS #19046

Are you sure you want to change the base?

Change docker context to default in MacOS #19046

Conversation

vyasgun commented Jun 29, 2023 • edited Loading

Does this PR introduce a user-facing change?

openshift-ci bot commented Jun 29, 2023

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 commented Jun 30, 2023

vyasgun commented Jul 3, 2023

n1hility commented Jul 11, 2023

vyasgun commented Jul 12, 2023

Choose a reason for hiding this comment

n1hility Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1hility commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 9, 2023

rhatdan commented Sep 9, 2023

vrothberg left a comment

Choose a reason for hiding this comment

vyasgun commented Jun 29, 2023 •

edited

Loading

n1hility Jul 12, 2023 •

edited

Loading