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

Add --add-host for docker build #30383

Merged
merged 1 commit into from Feb 27, 2017

Conversation

@TDAbboud
Contributor

TDAbboud commented Jan 23, 2017

Signed-off-by: Tony Abboud tdabboud@hotmail.com

- What I did
Added the --add-host flag to the docker build command. This flag allows you to add extra hosts to a containers /etc/hosts file upon building an image.

- How I did it
I added another flag to the build handler and added server side code to accept and set the extra hosts when the /build endpoint is hit.

- How to verify it
I have added an integration test TestBuildWithExtraHost which adds hosts whose ip is set to the localhost, but then it tries to ping based off of the hostname that was set.

- Description for the changelog
Add the --add-host flag to docker build to add entries to a containers /etc/hosts file when building an image

Implements #30096

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 31, 2017

Contributor

Can't this be achieved with ARG or whatever?
ping @docker/core-engine-maintainers

Contributor

LK4D4 commented Jan 31, 2017

Can't this be achieved with ARG or whatever?
ping @docker/core-engine-maintainers

@thaJeztah thaJeztah added this to backlog in maintainers-session Feb 2, 2017

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Feb 7, 2017

Contributor

@LK4D4 can you elaborate? Are you imagining some kind of "cat $ARG >> /etc/hosts" type of thing appearing in the Dockerfile?

Contributor

duglin commented Feb 7, 2017

@LK4D4 can you elaborate? Are you imagining some kind of "cat $ARG >> /etc/hosts" type of thing appearing in the Dockerfile?

Show outdated Hide outdated cli/command/image/build.go Outdated
buildImageSuccessfully(c, name,
withBuildFlags(
"--add-host", "foo:127.0.0.1",
"--add-host", "bar:127.0.0.1",

This comment has been minimized.

@duglin

duglin Feb 7, 2017

Contributor

I know we're in the design phase and this is premature, but if I don't ask now I'll probably forget....
can you add some negative tests? For example --add-host myhost: or --add-host myhost and ones with bad IPs. And ones with ipv6

@duglin

duglin Feb 7, 2017

Contributor

I know we're in the design phase and this is premature, but if I don't ask now I'll probably forget....
can you add some negative tests? For example --add-host myhost: or --add-host myhost and ones with bad IPs. And ones with ipv6

This comment has been minimized.

@TDAbboud

TDAbboud Feb 9, 2017

Contributor

That makes total sense. I was thinking about this when I wrote the initial test. I will add some more to this PR and push it again.

@TDAbboud

TDAbboud Feb 9, 2017

Contributor

That makes total sense. I was thinking about this when I wrote the initial test. I will add some more to this PR and push it again.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Feb 7, 2017

Contributor

In your testing, does the /etc/hosts file get saved in the resulting image?

Contributor

duglin commented Feb 7, 2017

In your testing, does the /etc/hosts file get saved in the resulting image?

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Feb 7, 2017

Contributor

as long as the changes are not saved in the resulting image, this SGTM

Contributor

duglin commented Feb 7, 2017

as long as the changes are not saved in the resulting image, this SGTM

@TDAbboud

This comment has been minimized.

Show comment
Hide comment
@TDAbboud

TDAbboud Feb 9, 2017

Contributor

The /etc/hosts file is not saved in the resulting image. I actually expressed concern about this in #30096

Contributor

TDAbboud commented Feb 9, 2017

The /etc/hosts file is not saved in the resulting image. I actually expressed concern about this in #30096

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 9, 2017

Member

We discussed this in the maintainers meeting, and although we still think setting up a DNS for these purposes, we already implemented --network for docker build, so let's go ahead and do the same for --add-host

Member

thaJeztah commented Feb 9, 2017

We discussed this in the maintainers meeting, and although we still think setting up a DNS for these purposes, we already implemented --network for docker build, so let's go ahead and do the same for --add-host

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Feb 13, 2017

Member

@TDAbboud needs a rebase 👼

Member

vdemeester commented Feb 13, 2017

@TDAbboud needs a rebase 👼

@TDAbboud

This comment has been minimized.

Show comment
Hide comment
@TDAbboud

TDAbboud Feb 14, 2017

Contributor

@vdemeester I just re-based and pushed the branch again. I also added another test for some invalid extra hosts.
There currently is no test for a valid IPv6 address, as I was not sure how exactly to implement this. The default /etc/hosts file already contains entries for localhost i.e. ::1 localhost so adding another entry will not work. I think it would be good to test this case as well though, if anyone has a good idea on how to test it.

Contributor

TDAbboud commented Feb 14, 2017

@vdemeester I just re-based and pushed the branch again. I also added another test for some invalid extra hosts.
There currently is no test for a valid IPv6 address, as I was not sure how exactly to implement this. The default /etc/hosts file already contains entries for localhost i.e. ::1 localhost so adding another entry will not work. I think it would be good to test this case as well though, if anyone has a good idea on how to test it.

@vdemeester

Small nit on the API changes, but otherwise LGTM 🦁

Show outdated Hide outdated docs/api/version-history.md Outdated
@cpuguy83

LGTM

We are currently having issues with CI, so we need to make sure to wait for CI to run against this PR.

Thanks!

Add --add-host for docker build
Signed-off-by: Tony Abboud <tdabboud@hotmail.com>
@TDAbboud

This comment has been minimized.

Show comment
Hide comment
@TDAbboud

TDAbboud Feb 23, 2017

Contributor

Just pinging to verify all tests have passed and the branch has been rebased

Contributor

TDAbboud commented Feb 23, 2017

Just pinging to verify all tests have passed and the branch has been rebased

@thaJeztah

LGTM, thanks!

@thaJeztah thaJeztah merged commit a64ea37 into moby:master Feb 27, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30975 has succeeded
Details
janky Jenkins build Docker-PRs 39588 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 10652 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.04.0 milestone Feb 27, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 27, 2017

Member

/cc @albers @sdurrheimer for completion scripts

Member

thaJeztah commented Feb 27, 2017

/cc @albers @sdurrheimer for completion scripts

@arehmandev

This comment has been minimized.

Show comment
Hide comment
@arehmandev

arehmandev Jul 12, 2017

Can we confirm this is working? I have just recently tried:

docker build --add-host=docker:10.180.0.1 -t abs
docker exec -it "containerid" sh
cat /etc/hosts - it doesn't appear there

arehmandev commented Jul 12, 2017

Can we confirm this is working? I have just recently tried:

docker build --add-host=docker:10.180.0.1 -t abs
docker exec -it "containerid" sh
cat /etc/hosts - it doesn't appear there

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 12, 2017

Member

the host that's added with this flag is only used during the build; it deliberately should not persist in the image

Member

thaJeztah commented Jul 12, 2017

the host that's added with this flag is only used during the build; it deliberately should not persist in the image

@arehmandev

This comment has been minimized.

Show comment
Hide comment
@arehmandev

arehmandev Jul 12, 2017

Is this really as intended? I mean other than at runtime, how can you persist host entries to the image?

  • perhaps the best way to approach this would be something like "HOSTENTRY" in the Dockerfile

arehmandev commented Jul 12, 2017

Is this really as intended? I mean other than at runtime, how can you persist host entries to the image?

  • perhaps the best way to approach this would be something like "HOSTENTRY" in the Dockerfile
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 12, 2017

Contributor

@arehmandev You don't. These are generally host specific settings and are not persisted into the image.

Contributor

cpuguy83 commented Jul 12, 2017

@arehmandev You don't. These are generally host specific settings and are not persisted into the image.

@arehmandev

This comment has been minimized.

Show comment
Hide comment
@arehmandev

arehmandev Jul 12, 2017

So I guess its up to the container orchestration system now to plug in these values. Cool thanks for clarification

arehmandev commented Jul 12, 2017

So I guess its up to the container orchestration system now to plug in these values. Cool thanks for clarification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment