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.

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

Better defaults for --docker-host. #1338

Open
matejvasek opened this issue Dec 1, 2021 · 22 comments · May be fixed by #1910
Open

Better defaults for --docker-host. #1338

matejvasek opened this issue Dec 1, 2021 · 22 comments · May be fixed by #1910
Labels
help wanted Need some extra hands to get this done. status/ready Issue ready to be worked on. type/enhancement Issue that requests a new feature or improvement.
Milestone

Comments

@matejvasek
Copy link
Contributor

matejvasek commented Dec 1, 2021

Description

When running pack against docker socket at non-standard location using the DOCKER_HOST environment variable,
the user also has to set the --docker-host flag so export container knows how to access the daemon.

Right now it defaults to unix://var/run/docker.sock which is not always right.

The value of the flag can be determined if docker host is either unix socket or ssh.
For unix socket the mapping is identity.
For ssh we can acutally ask remote what is the value of the DOCKER_HOST there.

For TCP there is no way that I know of; user simply must know what to enter.
The mapping could be possibly identity but there are possible problems when when accessing localhost:
the export container sees itself as localhost not the host machine running the daemon (unless --network=host).

@matejvasek matejvasek added status/triage Issue or PR that requires contributor attention. type/enhancement Issue that requests a new feature or improvement. labels Dec 1, 2021
@jylin
Copy link

jylin commented Dec 14, 2021

+1

With docker rootless, I currently use "--docker-host inherit", which works, but it would be good if this were not needed.

Related to this request: can "pack" detect docker host from context if DOCKER_HOST is not set? See: https://docs.docker.com/engine/security/rootless/#client. Edit: I'll create another issue for that.

@natalieparellano natalieparellano added status/discussion-needed Issue or PR that requires in-depth discussion. and removed status/triage Issue or PR that requires contributor attention. labels Mar 15, 2022
@jylin
Copy link

jylin commented Jul 3, 2022

Another problem: pack seems to escape "@" when set in DOCKER_HOST, so something like "DOCKER_HOST=ssh://user@host" doesn't work, even using "--docker-host inherit".

@jijeesh
Copy link

jijeesh commented Jul 22, 2022

Document Support for podman v2 buildpacks/docs#341

ssh-add -k "$HOME/.ssh/podman-machine-default"
podman system connection default podman-machine-default-root

login into the root user

ssh root@localhost -p 58780

create link to docker.sock

ln -s /run/podman/podman.sock /var/run/docker.sock

then export

export DOCKER_HOST=ssh://root@localhost:58780

@jjbustamante
Copy link
Member

Relates to #1830

@jjbustamante jjbustamante added status/ready Issue ready to be worked on. and removed status/discussion-needed Issue or PR that requires in-depth discussion. labels Aug 17, 2023
@jjbustamante jjbustamante added this to the 0.31.0 milestone Aug 17, 2023
@till
Copy link

till commented Sep 12, 2023

I stumbled across this today as I used another container for dockerd and after googling a lot and reading code I think this is a (currently) bug, as the documentation states that:

You just need to set the DOCKER_HOST environment and most applications will pick it up (pack is one of them).

It seems like, parts of pack (as in downloading images), seem to recognise DOCKER_HOST already, while specifically in pack build, it's a bit mangled. I think this code should be amended in order to make the podman example work.

The other option would be to amend the documentation to tell people to use --docker-host=inherit.

Is there a downside to using DOCKER_HOST straight up?

@jjbustamante
Copy link
Member

Hi @till

It seems like, parts of pack (as in downloading images), seem to recognise DOCKER_HOST already, while specifically in pack build, it's a bit mangled. I think this code should be amended in order to make the podman example work.

The other option would be to amend the documentation to tell people to use --docker-host=inherit.

I think we put it in the docs:

Screenshot 2023-09-12 at 10 47 19 AM

And this ticket was created and linked in In the future, this may be automatically detected.

@till
Copy link

till commented Sep 12, 2023

Couldn't find it, apologies.

Do you have any pointers what you'd like to see in a PR? I can try to whip something up.

@till
Copy link

till commented Sep 12, 2023

I may have overlooked this also because the wording is somewhat wrong in the docs. This is not (just) related to a socket connection, but overall using DOCKER_HOST with environment variables..

For reference, I set DOCKER_HOST=tcp://some.service.svc.cluster.local:2375 so I can run docker/podman in another pod, etc..

Anyhow, if anyone has any pointers, I would make a PR that uses DOCKER_HOST when it's set. Doesn't change/correct/adjust the value, etc.. Just uses it transparently. Would that be acceptable?

@natalieparellano
Copy link
Member

@till this should be fine, when passed inherit we just read the value from DOCKER_HOST anyway:

		if dockerHost == "inherit" {
			dockerHost = os.Getenv("DOCKER_HOST")
		}

It probably makes sense for DOCKER_HOST to be the default value when no --docker-host is provided, but the end user should be able to override it in case the value is not correct.

@matejvasek
Copy link
Contributor Author

matejvasek commented Sep 13, 2023

The inherit was deliberately chosen not to be default just to be sure backward compatibility is not broken.

@matejvasek
Copy link
Contributor Author

matejvasek commented Sep 13, 2023

@till
The thing is:
The DOCKER_HOST envvar is the address of docker socket from local machine (where you run pack/docker) point of view.
The --docker-host flag indicates socket path from point of view of the machine that is actually running daemon and which is mounted into build containers.

There are cases when this is one single machine, for instance your are running podman on Linux.
However there are also cases when this is not true, for example when using podman-machine (VM) on macOS.

@matejvasek
Copy link
Contributor Author

matejvasek commented Sep 13, 2023

@till Another example: imagine you have one Linux machine with docker running there. From point of view of that machine the docker socket is unix:///var/run/docker.sock. Then imaging you have a Windows machine that is connecting to that Linux machine via TCP. From point of view of that machine the docker socket is tcp://example.local:2375. You most definitely do not do not want --docker-host=inherit which would be equivalent to --docker-host=tcp://example.local:2375. What is best here is to leave it alone and default to unix:///var/run/docker.sock.

You most definitely do not do not want --docker-host=inherit which would be equivalent to --docker-host=tcp://example.local:2375

To elaborate:

  • While your Windows machine may correctly resolve example.local to IP of your Linux machine this may not be true about the build container.
  • That TCP connection may be TLS protected and build container may not have crypto secrets that are in Windows machine.

@matejvasek
Copy link
Contributor Author

matejvasek commented Sep 13, 2023

tl;dr we cannot universally assume --docker-host=inherit will just work.

@matejvasek
Copy link
Contributor Author

matejvasek commented Sep 13, 2023

Another example:
DOCKER_HOST=ssh://user@server -- again we do not want --docker-host=inherit the builder container does not have password or ssh keys to access it.

till added a commit to till/pack that referenced this issue Sep 19, 2023
@till till linked a pull request Sep 19, 2023 that will close this issue
2 tasks
till added a commit to till/pack that referenced this issue Sep 19, 2023
related: buildpacks#1338
Signed-off-by: till <till@php.net>
@till
Copy link

till commented Sep 19, 2023

Another example: DOCKER_HOST=ssh://user@server -- again we do not want --docker-host=inherit the builder container does not have password or ssh keys to access it.

I am not sure what you are guarding against. And why wouldn't the builder have access? Wouldn't that be up for the user to sort out? Or unset DOCKER_HOST in case it's not what they want? Or what would you do here?

I think if it's set, it should be used as is as that is behaviour most people expect and that is also mentioned in blog posts (e.g. regarding podman).

I made a PR as a pass, let me know if you have thoughts. It also falls back to the original behaviour, at least so I hope.

@matejvasek
Copy link
Contributor Author

matejvasek commented Sep 19, 2023

Another example: DOCKER_HOST=ssh://user@server -- again we do not want --docker-host=inherit the builder container does not have password or ssh keys to access it.

I am not sure what you are guarding against. And why wouldn't the builder have access? Wouldn't that be up for the user to sort out? Or unset DOCKER_HOST in case it's not what they want? Or what would you do here?

I think if it's set, it should be used as is as that is behaviour most people expect and that is also mentioned in blog posts (e.g. regarding podman).

I made a PR as a pass, let me know if you have thoughts. It also falls back to the original behaviour, at least so I hope.

@till I am not strongly against --docker-host=inherit being default.
I am merely pointing out the complexities of setting --docker-host.

At least with ssh protocol I am quite convinced you do not want inherit, the builder container most definitely doesn't have credentials (unless somehow complicatedly mounted, if somebody is skilled enough to do this they may as well know how to set up --docker-host) .

Also with tcp you might have problem because of a) client cert authentication, b) hostname resolution. For instance if docker host is tcp://localhost:1234 then again build container won't be able to access it because localhost from build container PoV is the container itself not the machine running daemon (the machine where you run pack).

With unix socket it is more likely you want inherit, still there are scenarios where this is an issue. For instance on macOS with podman desktop the podman will somehow expose the docker socket out of the VM but the path is different than path it the VM, so again inherit won't work.

So by using defaulting --docker-host=inherit we fix issues for some users but possibly break it for some other.
Question is what user group is bigger.

@till
Copy link

till commented Sep 19, 2023

@matejvasek thanks for elaborating and clarifying, I think I understand what you mean.

I am trying to make it behave how it is generally expected for a new user, vs. what someone needs who runs pack on a k8s cluster (though I am in that bucket).

My TCP (non tls) example already works with the proper DOCKER_HOST.

So I realize changing this may be a BC break, but pack is at 0.y.z. So I hope that's acceptable?

I think this could be further improved by adding more variables to the build (such as the location of the TLS certs), or potentially providing an ssh config:
https://github.com/moby/moby/blob/1a7969545d73537545645f5cd2c79b7a77e7d39f/client/options.go#L33

I have to admit, I haven't looked into the internals how docker/cli connects to an ssh host, as in, where it's getting configuration from. I have only used the socket and http(s) so far.

@matejvasek
Copy link
Contributor Author

@till I think we could conditionally default to DOCKER_HOST:

  • With ssh never default.
  • With tcp default only when host in not localhost (localhost, 127.0.0.1, ::1) and DOCKER_CERT_PATH,DOCKER_TLS_VERIFY are not set.
  • With unix default only on Linux, do not default on macOS or Windows.

till added a commit to till/pack that referenced this issue Sep 20, 2023
related: buildpacks#1338
Signed-off-by: till <till@php.net>
@matejvasek
Copy link
Contributor Author

@till even in your specific case DOCKER_HOST=tcp://some.service.svc.cluster.local:2375 --docker-host=inherit is not necessary what you want. It does work just fine but it is sub-optimal -- you drag all traffic via k8s service.

What you could do instead:

pack --network=host --docker-host=tcp://localhost:2375 <rest of args here>

@till
Copy link

till commented Sep 20, 2023

No, the dind is running per user in a tenant/namespace with resource quotas applied. Also to ensure the cache doesn't work across boundaries etc.. I definitely do not want host network.

I used a service to ensure it's predictable between updates and longer running than the pod with pack. This also allows for simultaneous builds, etc..

@matejvasek
Copy link
Contributor Author

There is one more way to avoid setting --docker-host. It is possible to use --publish flag then the image is directly pushed to registry (but it is not then present in podman storage).

@cmoulliard
Copy link

cmoulliard commented Nov 19, 2024

On macos using podman rootless, the way to go is to do:

~/code/redhat-buildpacks/buildpack-test on main •
// using fish
❯ set -gx DOCKER_HOST unix:///$HOME/.local/share/containers/podman/machine/podman.sock
// using bash
❯ export DOCKER_HOST="unix:///$HOME/.local/share/containers/podman/machine/podman.sock"

❯ pack build quay.io/snowdrop/my-quarkus-app -B paketocommunity/builder-ubi-base:latest --docker-host inherit
49c69080bafa: Already exists 
acab5d95f367: Already exists 
...

but then we will got another error

===> ANALYZING
Running the analyzer on OS linux from image pack.local/lifecycle/6b717a7465756e6a6e70:latest with:
Container Settings:
  Args: /cnb/lifecycle/analyzer -log-level debug -daemon -run /layers/run.toml -run-image index.docker.io/paketocommunity/run-ubi-base:latest -launch-cache /launch-cache quay.io/snowdrop/my-quarkus-app
  System Envs: CNB_USER_ID=1002 CNB_GROUP_ID=1000 CNB_PLATFORM_API=0.13
  Image: pack.local/lifecycle/6b717a7465756e6a6e70:latest
  User: root
  Labels: map[author:pack]
Host Settings:
  Binds: /Users/cmoullia/.local/share/containers/podman/machine/podman.sock:/var/run/docker.sock pack-cache-snowdrop_my-quarkus-app_latest-699f2436a984.launch:/launch-cache pack-layers-bpdycwqiaz:/layers pack-app-naddxdkbvn:/workspace
  Network Mode: pack.local-network-657a77737a7762786a6d
ERROR: failed to build: executing lifecycle: failed to create 'analyzer' container: Error response from daemon: container create: statfs /Users/cmoullia/.local/share/containers/podman/machine/podman.sock: operation not supported

@natalieparellano natalieparellano added the help wanted Need some extra hands to get this done. label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Need some extra hands to get this done. status/ready Issue ready to be worked on. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants