Skip to content

The option --dns and --net=host should not be mutually exclusive.#22408

Merged
vdemeester merged 2 commits into
moby:masterfrom
yongtang:21976-allow-dns-and-net-host
May 25, 2016
Merged

The option --dns and --net=host should not be mutually exclusive.#22408
vdemeester merged 2 commits into
moby:masterfrom
yongtang:21976-allow-dns-and-net-host

Conversation

@yongtang
Copy link
Copy Markdown
Member

@yongtang yongtang commented Apr 29, 2016 •
edited
Loading

- What I did

This fix tries to address the issue raised in #21976 and allows the options of --add-host, --dns, --dns-search, --dns-opt and --net=host to work at the same time.

- How I did it

The validation functions for options has been updated.

- How to verify it

The documentation has been updated and additional tests have been added to cover this change.

- Description for the changelog

Allows --add-host, --dns, --dns-search and --dns-opt to be used with --net=host at the same time when container runs.

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #21976.

Signed-off-by: Yong Tang yong.tang.github@outlook.com
@yongtang yongtang mentioned this pull request Apr 29, 2016
Closed
@yongtang yongtang force-pushed the 21976-allow-dns-and-net-host branch from e2b5709 to 8b2e785 Compare April 29, 2016 13:31
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Apr 29, 2016

ping @mavenugo @estesp
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented May 5, 2016

ping @mavenugo @estesp wdyt?
@yongtang yongtang force-pushed the 21976-allow-dns-and-net-host branch from 8b2e785 to f384ba8 Compare May 6, 2016 05:17
@mavenugo
Copy link
Copy Markdown
Contributor

mavenugo commented May 6, 2016

There seems to be some historical context that I may be missing when it comes to supporting --dns with --net=host. But @mrjana should know if there are any gotchas that I may be missing.
@icecrime
Copy link
Copy Markdown
Contributor

icecrime commented May 11, 2016

Ping @mrjana: WDYT?
@mrjana
Copy link
Copy Markdown
Contributor

mrjana commented May 16, 2016

+1 on the design. LGTM
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented May 16, 2016

moving to code review. @yongtang this needs a rebase now, can you rebase?
@yongtang yongtang force-pushed the 21976-allow-dns-and-net-host branch from f384ba8 to ce99698 Compare May 17, 2016 03:45
@yongtang
Copy link
Copy Markdown
Member Author

yongtang commented May 17, 2016

Thanks @mrjana @icecrime @thaJeztah @mavenugo for the review. I rebased the Pull Request. Please let me know if there are any issues.
cpuguy83
cpuguy83 reviewed May 17, 2016
View reviewed changes
Comment thread docs/reference/run.md Outdated
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 May 17, 2016

Choose a reason for hiding this comment

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

Is there a reason we wouldn't support --add-host and --dns-search as well?
@estesp
Copy link
Copy Markdown
Contributor

estesp commented May 18, 2016

@mavenugo I think the historical restriction was that in net=host mode, the resolv.conf link was readonly direct to /etc/resolv.conf on the host. Now that it is a bind mount of the copy in container metadata, it is already editable (in both net = host and "normal" modes), and therefore the restriction really isn't required.

I agree with @cpuguy83 that if we are removing this restriction, then any option which modifies resolv.conf or /etc/hosts might as well be allowed as well for consistency. The documentation should be clear that this is not modifying the host copies of these files, and maybe even a warning that this is slightly opposed to the concept of sharing the "host's networking configuration/stack". But given the use cases, I'm ok with allowing the capability.
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented May 18, 2016

Thanks @estesp, and agreed on proper documentation as well (to prevent confusion)
@yongtang
Copy link
Copy Markdown
Member Author

yongtang commented May 19, 2016

Thanks @cpuguy83 @estesp and @thaJeztah for the review and suggestions. I will update the PR to address the issues raised.
@thaJeztah thaJeztah mentioned this pull request May 20, 2016
Closed
@yongtang yongtang force-pushed the 21976-allow-dns-and-net-host branch from ce99698 to 9a28f67 Compare May 21, 2016 18:51
@yongtang
Copy link
Copy Markdown
Member Author

yongtang commented May 21, 2016

@thaJeztah @cpuguy83 @estesp @icecrime @mavenugo I updated the pull request so that now --add-host, --dns, --dns-search and --dns-opt could be used with --net=host at the same time. The documentation and tests have also be updated. Please let me know if there are any issues.
@yongtang yongtang force-pushed the 21976-allow-dns-and-net-host branch from bb5533e to 3dab32c Compare May 21, 2016 21:17
@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented May 23, 2016

LGTM
1 similar comment
@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented May 23, 2016

LGTM
@vdemeester
Copy link
Copy Markdown
Member

vdemeester commented May 24, 2016

docs LGTM 👼
/cc @thaJeztah @SvenDowideit
thaJeztah
thaJeztah reviewed May 24, 2016
View reviewed changes
Comment thread docs/reference/run.md Outdated
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah May 24, 2016

Choose a reason for hiding this comment

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

Perhaps something like;

Similar to `--hostname`, the `--add-host`, `--dns`, `--dns-search`, and
`--dns-opt` options can be used in `host` network mode. These options update
`/etc/hosts` or `/etc/resolv.conf` inside the container. No change are made to
`/etc/hosts` and `/etc/resolv.conf` on the host.
@yongtang yongtang force-pushed the 21976-allow-dns-and-net-host branch from 3dab32c to f9a6214 Compare May 24, 2016 14:12
@yongtang
Copy link
Copy Markdown
Member Author

yongtang commented May 24, 2016

Thanks @thaJeztah, the documentation has been updated.
@thaJeztah thaJeztah added this to the 1.12.0 milestone May 24, 2016
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented May 24, 2016

LGTM, thanks @yongtang
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented May 24, 2016

oh, so sorry @yongtang, looks like it needs a rebase 😢
yongtang added 2 commits May 24, 2016 16:03
@yongtang
The option --dns, --dns-search, --dns-opt and --net=host should not b…
23821fe 
…e mutually exclusive.

This fix tries to address the issue raised in moby#21976 and allows
the options of `--dns`, `--dns-search`, `--dns-opt` and `--net=host`
to work at the same time.

The documentation has been updated and additional tests have been
added to cover this change.

This fix fixes moby#21976.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
The option --add-host and --net=host should not be mutually exclusive.
90bd41a 
This fix tries to address the issue raised in moby#21976 and allows
the options of `--add-host` and `--net=host` to work at the same time.

The documentation has been updated and additional tests have been
added to cover this change.

This fix fixes moby#21976.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang force-pushed the 21976-allow-dns-and-net-host branch from f9a6214 to 90bd41a Compare May 25, 2016 02:25
@yongtang
Copy link
Copy Markdown
Member Author

yongtang commented May 25, 2016

Thanks @thaJeztah the PR has been rebased.
@vdemeester vdemeester merged commit 9f5a2c6 into moby:master May 25, 2016
@bboreham bboreham mentioned this pull request May 25, 2016
Closed
@yongtang yongtang deleted the 21976-allow-dns-and-net-host branch May 25, 2016 09:27
@ketzacoatl
Copy link
Copy Markdown

ketzacoatl commented May 25, 2016

TY, TY, TY, to all involved!
@denibertovic denibertovic mentioned this pull request Jun 3, 2016
Closed
@nick4fake nick4fake mentioned this pull request Jun 4, 2016
Closed
@thaJeztah thaJeztah mentioned this pull request Jun 17, 2016
Merged
@vefimova vefimova mentioned this pull request Jul 21, 2016
Closed
@vefimova vefimova mentioned this pull request Jul 29, 2016
Merged
@samuelkarp samuelkarp mentioned this pull request Dec 9, 2016
Closed
@codablock codablock mentioned this pull request Dec 21, 2016
Merged
@cmluciano cmluciano mentioned this pull request Jan 23, 2017
Merged
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Mar 1, 2017
Merge pull request #29378 from vefimova/docker_resolv
d5ff694 
Automatic merge from submit-queue

Re-writing of the resolv.conf file generated by docker

Fixes #17406 

Docker 1.12 will contain feature "The option --dns and --net=host should not be mutually exclusive" (moby/moby#22408)
This patch adds optional support for this ability in kubelet (for now in case of "hostNetwork: true" set all dns settings are ignored if any).
To enable feature use newly added kubelet flag: --allow-dns-for-hostnet=true
@Random-Liu Random-Liu mentioned this pull request Jun 7, 2018
Merged
@Gufran Gufran mentioned this pull request Apr 22, 2020
Closed
This was referenced Dec 6, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment