-
Notifications
You must be signed in to change notification settings - Fork 293
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
Implement new build flag --docker-host #988
Conversation
Using TCP via |
This is also of interest for #966. |
Docker doc also mentions |
975687a
to
ce5145e
Compare
Codecov Report
@@ Coverage Diff @@
## main #988 +/- ##
==========================================
+ Coverage 80.35% 80.38% +0.03%
==========================================
Files 130 130
Lines 7964 7976 +12
==========================================
+ Hits 6399 6411 +12
Misses 1147 1147
Partials 418 418
Flags with carried forward coverage won't be shown. Click here to find out more. |
ce5145e
to
b820137
Compare
Hi @matejvasek, thanks for this PR. Just wanted to give you an update and heads up. Given the holiday's it'll probably take a little bit longer to get through the process. I'm setting up an environment to test this. I have a few comments I'll add to the PR but it's great to see this feature finally coming in. Adding some context for others, |
Update: So I ran into a case where this conflicts with the use of Docker Desktop. Currently the mount The proposed solution in this PR currently adds complexity that isn't quite a drop-in replacement. For example, if you use a I do wonder if this solution could be optionally applied via detecting the host daemon process (docker desktop vs podman) or (less preferable) by using an optional flag/configuration element. Overall, I think it's going in the right direction but we'll want to make sure it's not disruptive out of the gate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! I have a few reservations about the PR as is:
- Tests added for these cases would help the robustness
- We'd probably want to thread the logger through the provider, so we don't have to write directly to
errorWriter
I'm happy to take care of both of those in a polish PR, but I do want to clarify the specific logical questions I had – notably, which OS's we think we need to set host
(so we can have an appropriate comment clarifying what we're doing, and setting provider.ctrConf.Env
even for non linux
environments.
Definitely hoping to get this in in the next few days!
@jromero I know that's problematic, on Linux this can be solved by using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarize my thinking, I love the idea of being able to specify an alternative to the default bind-mounted socket. I'm a bit concerned with this implementation right now because it's very unencrypted-TCP-on-Linux oriented and breaks the currently working, TCP-over-mTLS which is platform agnostic and much safer for general use.
I wonder, what if this were done with pack build
option like --docker-host=
which, if specified, would always disable the default binding and sets a "universal" docker host for pack
and lifecycle
, supporting values like:
Use-case 1: localhost-over-unencrypted-TCP
Just like pack's current behavior (respect DOCKER_HOST
) but also copy it to down to lifecycle
export DOCKER_HOST=tcp://localhost:2375
pack build my-app --docker-host=inherit --network=host
# equivalent
pack build my-app --docker-host=tcp://localhost:2375 --network=host
Use-case 2: Non-standard socket location
This would bind-mount anything with a prefix of unix:
(or npipe:
for Windows) and use it as pack
and lifecycle
's DOCKER_HOST
(though maybe using the standard socket path in the container). This should enable rootless docker (which as of last year, did not work due to the hard-coded host socket path)
pack build my-app --docker-host=unix:///run/user/1001/docker.sock
Use-case 3: mTLS-secured daemon
Parse any TCP URI for optional cert options (syntax could be tweaked but a cert paths would also set DOCKER_TLS_VERIFY
and DOCKER_CERT_PATH
for lifecycle). More details on use-case.
pack build my-app --docker-host=tcp://my-secure-host:2376?tlscertpath=/docker-certs --volume $HOME/.docker:/docker-certs
We could start with just Use-case 1 which could expand into the other cases with future contributions.
Thanks for the feedback. I hope I'll get back to this next week. |
Hey @matejvasek ! Just wanted to check on in this – this is something we're definitely interested in enabling, and it would be great to get this done. Do you think you'll have some time for this over the next few weeks? If not, I know @micahyoung had mentioned that he had some ideas for doing it as well. |
4591339
to
87a9460
Compare
@dfreilich sorry for not getting to this earlier, I had to fix some |
docker-host
flag
ae711d6
to
5cc674e
Compare
internal/commands/build.go
Outdated
@@ -143,6 +145,13 @@ func buildCommandFlags(cmd *cobra.Command, buildFlags *BuildFlags, cfg config.Co | |||
cmd.Flags().StringArrayVar(&buildFlags.EnvFiles, "env-file", []string{}, "Build-time environment variables file\nOne variable per line, of the form 'VAR=VALUE' or 'VAR'\nWhen using latter value-less form, value will be taken from current\n environment at the time this command is executed\nNOTE: These are NOT available at image runtime.\"") | |||
cmd.Flags().StringVar(&buildFlags.Network, "network", "", "Connect detect and build containers to network") | |||
cmd.Flags().BoolVar(&buildFlags.Publish, "publish", false, "Publish to registry") | |||
cmd.Flags().StringVar(&buildFlags.DockerHost, "docker-host", "inherit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, but I realize I missed one aspect of the workflow in my earlier comment, that the previous behavior needs to be the new default (namely, when --docker-host
is not provided, don't pass through DOCKER_HOST
env vars into the container and always using a bind mount). Unfortunately, inherit
isn't sufficient for a backward compatible default.
I was able to fix this in your PR by setting the default value for the flag to ""
instead of "inherit"
. I'd be happy with this change for now and if needs any further cleanup, I can do another pass when I add in the TLS behavior.
@micahyoung this is odd |
@micahyoung could you please try restarting the action? |
I'll have to squash the commits anyway. |
Thank you for the quick implementation and changes. Looking forward to using the feature. It appears the last run went green as well. |
0eecc6e
to
f414655
Compare
squashed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for doing this, this looks really good, and really close! I'm sorry for sending this back, just a few small things I'd like to see added to this.
@dfreilich PTAL |
b748222
to
89fa1e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you so much for this, this ended up being considerably more complex than we initially thought, and I appreciate all the work that went into this!
@micahyoung I'll give you time to check it out as well (if you have the time).
Introducing new build flag: --docker-host. The purpose of the flag is to expose custom docker deamon to build container. This is useful when running docker daemon on non standard location e.g. "tcp://localhost:1234", or if resulting image should be stored in different daemon tha you are running locally. Signed-off-by: Matej Vasek <[email protected]>
89fa1e5
to
be9b1b9
Compare
docker-host
flag
@micahyoung what is your use-case for this? Are you pushing images into remote daemon? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great contribution, many thanks!
Yeah, I am using Docker Desktop Windows daemons (and recently Docker Desktop MacOS/ARM64 daemons) on local/remote VMs mostly, with the daemon API either unencrypted or secured with mTLS, but running My current workflow, before this feature, was based on swapping around the
With the functionality of this feature, I should be able to now do this from the command line:
But overall, this feature is a great step to making the implicit |
Summary
Introducing new build flag: --docker-host.
The purpose of the flag is to expose custom docker deamon to build
container. This is useful when running docker daemon on non standard
location e.g. "tcp://localhost:1234", or if resulting image should be
stored in different daemon than you are running locally.