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

allow /etc/hosts /etc/resolv.conf and /etc/hostname to be changed #5129

Merged
merged 3 commits into from
Aug 20, 2014

Conversation

vieux
Copy link
Contributor

@vieux vieux commented Apr 9, 2014

Fixes #2267
related: #11950
My attempt to fix #2267

@vieux
Copy link
Contributor Author

vieux commented Apr 10, 2014

@jpetazzo
Copy link
Contributor

So, this creates a "private" hosts and resolv.conf file for each container, right?

@vieux
Copy link
Contributor Author

vieux commented Apr 10, 2014

hosts it was already the case, but was mounted in ro
resolv.conf it was already the case if you host's one was 127.0.0.1, but was mounted in ro

@vieux
Copy link
Contributor Author

vieux commented Apr 14, 2014

@jpetazzo
Copy link
Contributor

+1.

Question (out of curiosity rather than anything else): if I change those files in my container, will that be committed to the image, or are they excluded anyway?

@vieux
Copy link
Contributor Author

vieux commented Apr 14, 2014

No, the changes won't be commited.

@westurner
Copy link

Looking forward to this. Thanks!

@vieux
Copy link
Contributor Author

vieux commented Apr 17, 2014

rebased, @creack @crosbymichael @shykes @unclejack please review

@vieux
Copy link
Contributor Author

vieux commented Apr 17, 2014

@alexlarsson do you have any input ?

@alexlarsson
Copy link
Contributor

I think this is kind of weird. First of all, even if the file is rw bind-mounted you can't do a proper atomic replace operation on it (write to tmpfile, rename over target). Secondly, if we copy the file like that we will never update the resolve file if the host changes to a different network, so, e.g. on a roaming laptop containers will stop properly resolving hostnames.

@vieux
Copy link
Contributor Author

vieux commented Apr 18, 2014

@alexlarsson

  1. we bind rw a copy, so I guess we don't care about the atomic operation ?
  2. It was already the case if you have 127.0.0.1 on your host. Simpky use -dns 8.8.8.8 and you're good, no ?

@alexlarsson
Copy link
Contributor

@vieux I understand that we're already broken wrt this with the current system, but I think we want to either the "read-only forward of host dns, with updates" model, or "get initial state but then let the container modify it" model. This patch doesn't do either, since the natural way to modify these files is with a write-to-tmp-rename-over-target atomic operation, which will fail with the rw bind mount.

@vieux
Copy link
Contributor Author

vieux commented May 13, 2014

@shykes any input on this ?

@vieux vieux modified the milestones: 1.1, 1.0 May 15, 2014
@crosbymichael crosbymichael removed this from the 1.0 milestone May 19, 2014
@shykes
Copy link
Contributor

shykes commented May 29, 2014

I am definitely in favor of making /etc/hosts writeable. (Just got bitten by the problem today while trying to debug calls to a remote api in my app).

@vieux would you mind giving a high-level description of you implement it, expected behavior etc?

Thanks

@vieux
Copy link
Contributor Author

vieux commented May 29, 2014

@shykes I rebased so you can test it.

It's quite simple, in daemon/volumes.go we mount /etc/resolv.conf /etc/hosts and /etc/hostname as read write, you can edit the files, but as it's in the init layer, they will never be committed / showed in the diff

It works because we already created a local copy of /etc/hosts and /etc/hostname for each container. So we can bind mount rw without issue.
In daemon/container.go I updated to we never bind mount from the host /etc/resolv.conf but always from a local copy, like we do for /etc/hosts and /etc/hostname

ping @unclejack @crosbymichael

@crosbymichael
Copy link
Contributor

Because we always copy this into the container what do we do when ppl want to commit the changes in a build? Still not allow that like that PR does and require users apps to modify these files at runtime? Since then are runtime options and I don't think it makes sense to make these changes at build time.

@kiorky
Copy link
Contributor

kiorky commented May 30, 2014

@crosbymichael @vieux yes both of those files need to be writable at any moment of the container lifecycle.

@vieux
Copy link
Contributor Author

vieux commented Jun 20, 2014

rebased any news on this @crosbymichael @unclejack

@unclejack
Copy link
Contributor

@vieux Could you add a cli integration test to ensure we don't commit these, please?

@SvenDowideit
Copy link
Contributor

can we modify the info at docs/source/articles/networking.md to cover this change please. This is another thing that experienced users will love to see in their CHANGELOG

vieux and others added 3 commits August 19, 2014 17:06
Docker-DCO-1.1-Signed-off-by: Victor Vieux <victor.vieux@docker.com> (github: vieux)
Docker-DCO-1.1-Signed-off-by: Victor Vieux <vieux@docker.com> (github: vieux)
Signed-off-by: Victor Vieux <vieux@docker.com>
@SvenDowideit
Copy link
Contributor

Doc LGTM

vieux added a commit that referenced this pull request Aug 20, 2014
allow /etc/hosts /etc/resolv.conf and /etc/hostname to be changed
@vieux vieux merged commit 94fbcea into moby:master Aug 20, 2014
@vieux vieux deleted the host_resolv branch August 20, 2014 16:02
@phemmer
Copy link
Contributor

phemmer commented Aug 27, 2014

Was all happy to use this feature, and I've already found an issue :-(
Updating /etc/hosts should be done atomically, otherwise you can get resolve errors in the middle of swapping it out. Simply overwriting the file isn't safe as when the file is opened, it is first truncated, and thus empty.
The best (only?) way to do this is to create /etc/hosts.new, and then do a rename (rename is atomic).
However this does not work because /etc/hosts is a bind mount, thus you can't use rename to swap it out.

Is there any reason it was not made such that /etc/hosts is a normal file, and is instead removed when the image is created?
Should this be created as a new issue, or would it not even be considered?

@LK4D4
Copy link
Contributor

LK4D4 commented Sep 3, 2014

ping @vieux @erikh
WDYT about making /etc/hosts normal file? It also allows our links make atomic changes to /etc/hosts. Now I and @unclejack have problems with TestHostsLinkedContainerUpdate and possibly because of this.

@erikh
Copy link
Contributor

erikh commented Sep 3, 2014

We’re making them in a locked container that isn’t running yet atm (this will change with links rewrite). I’m not sure this is going to result in anything.

That said, no objections otherwise.

On Sep 3, 2014, at 1:16 PM, Alexandr Morozov notifications@github.com wrote:

ping @vieux @erikh
WDYT about making /etc/hosts normal file? It also allows our links make atomic changes to /etc/hosts. Now I and @unclejack have problems with TestHostsLinkedContainerUpdate and possibly because of this.


Reply to this email directly or view it on GitHub.

@LK4D4
Copy link
Contributor

LK4D4 commented Sep 3, 2014

Personally I'm +1 to do this atomically. Empty and missing /etc/hosts can lead to nasty things.

@LK4D4
Copy link
Contributor

LK4D4 commented Sep 4, 2014

@phemmer Is it make sense to make syscall.Flock before changing? I just not very familiar with such things.

@phemmer
Copy link
Contributor

phemmer commented Sep 4, 2014

@LK4D4 As an alternative to rename()? It wouldn't hurt, but I doubt glibc attempts a shared lock before trying to use it, so it wouldn't help there.

It might help multiple apps trying to update the file at the same time. But I doubt many apps try to grab a lock before updating it.
If you're thinking about using a lock when the docker daemon updates it so that it doesn't conflict with processes inside the container, I don't see any harm in doing so. But my question would be, will the lock be present within the container?

@jpetazzo
Copy link
Contributor

Well, you could update the file atomically by:

  • writing \n# and the rest of the host entry
  • replace the # with a space

... But that makes me cringe a little bit. At that point it feels better to have a proper resolver, probably DNS-based. My 2c...

@phemmer
Copy link
Contributor

phemmer commented Sep 22, 2014

a write() call is atomic as long as the data all fits within a single write() (is less than PIPE_BUF), so that's not an issue. The issue is when you need to remove or modify an existing entry within the file, as then you have to rewrite the whole file.

@jpetazzo
Copy link
Contributor

Oh right, silly me, on files, it should work just fine.

To remove an entry, ... well we could just comment it out (by overwriting the first character of the line with a #). But... at the end of the day, it will be pretty creepy. I mean, that would be like a small specialized filesystem in /etc/hosts, dealing with fragmentation and all. Brrr.

SemanticBeeng added a commit to SemanticBeeng/ansible-inside-docker-demo that referenced this pull request Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow customization of /etc/hosts, /etc/resolv.conf, etc. in containers