Fix etchosts.Update to not target hosts with given hostname as prefix

[thieman]

Fixes issue #603.

[thieman]

@mavenugo Could I bother you for a review here? Thanks in advance!

[mavenugo]

@mrjana is the best person for this.

[thieman]

@mrjana Could this be reviewed, please? It's (hopefully) a small fix for what is a very annoying bug if you happen to have containers named in such a way that you trigger it.

[calavera]

@thieman is it just me or your test doesn't cover the issue? I cannot find any trailing spaces in the hosts you're using as examples.

[thieman]

The trailing space is not necessary, the issue is that any entry in the hosts file of format <IP> <prefix> will be updated, regardless of what is after the prefix. So triggering an update on a host called myHost will erroneously update both of these lines:

1.1.1.1 myHost
2.2.2.2 myHostWhichHasATotallyDifferentNameButStartsWithmyHost

The test case I provided will fail if you back out my change to etchosts.go, so it's covering the issue as far as I can tell. Unless I'm misunderstanding you?

edit: To state it another way, my change only matches the prefix if there is whitespace after the prefix or a literal period.

# This line gets matched because there is a newline immediately after myHost
1.1.1.1 myHost
# This line does NOT get matched because the character after myHost is not whitespace or a period
2.2.2.2 myHostWhichHasATotallyDifferentNameButStartsWithmyHost

If you'd prefer, we could add another line like this:

# This would get matched because the next character after myHost is a space
3.3.3.3 myHost myHost.domain
# This would also get matched because the next character is a period
3.3.3.3 myHost.domain
# This line would NOT get matched because the character after myHost is still not whitespace or a period
4.4.4.4 myHostWhichHasATotallyDifferentNameButStartsWithmyHost myHostWhichHasATotallyDifferentNameButStartsWithmyHost.mydomain

[thieman]

@calavera Did that make sense?

@mavenugo @mrjana I'm still waiting for a review on this 6 weeks later, is there anyone else we can bring in if you're both busy? Would love to get this in for 1.10, it's a very annoying bug.

[mavenugo]

@thieman sorry about the delay. Since /etc/hosts based SD is being replaced by DNS for 1.10, I am not too sure if this PR is relevant. But again, more than reviewing this change, it is important to make sure this doesn't cause any regression and hence it is better to get it reviewed by @mrjana @sanimej or @aboch who has dealt with related issues in the past.

@aboch @sanimej can u also pls help review this ?

[thieman]

I wasn't aware that we were moving to a new approach for 1.10, that's very interesting to learn. 😄 Thanks for the update.

[mavenugo]

@thieman #767 is the proposal.

[aboch]

LGTM

[sanimej]

@thieman Change in the Update function LGTM. I think the pattern in Delete function also needs to be changed.. its "\S*\t%s\n". If you use the --link container_name format without the alias then the /etc/hosts file entry will be "IP \t container_name \s container_id". And above pattern will not match. Looks like we need to match for \n or space.

Change in this PR is still needed with the DNS based service discovery @mavenugo mentioned. Because for default bridge with --link we will continue to use /etc/hosts file. Thanks for addressing this issue.

[thieman]

I'm taking a look into this now, though it looks like that Delete function was changed recently, so the bug may have been fixed then. http://github.com/docker/libnetwork/commit/9536e8e1

[thieman]

@sanimej That commit did indeed fix the problem, since it is now checking based on the suffix of the line rather than the prefix. I went ahead and added an additional regression test around that to codify the correct behavior in the test suite. Should be good for another review when you have time. 😄

edit: Actually, if what you said here was true...

If you use the --link container_name format without the alias then the /etc/hosts file entry will be "IP \t container_name \s container_id"

Then I don't believe the current Delete function is handling that case at all, since it's looking for \t container_name at the end of the string. If that's the case, I think it's probably a separate issue from what this PR is trying to do, but an issue nonetheless.

[thieman]

Starting to grok this a bit better...since Delete is working off of actual Record structs rather than plain strings of aliases, I think the delete will still work as long as the Record is well-formed since the "Hosts" in your example would be container_name \s container_id. So I think we are good here.

[thieman]

@sanimej Given that the Delete functionality seems fine, is this good for merge now?

[thieman]

@sanimej @mavenugo @aboch Hearing rumblings on Twitter that 1.10 is about to go into code freeze, any chance we can get this simple, tested, LGTM'd fix merged so it might make it in?

[mavenugo]

@thieman thanks for the reminder. yes. 1.10 code freeze is coming up.
@sanimej can you please give your comment asap ?

[sanimej]

Please add a brief note about the test rather than referring to the issue number here.

Can you also squash the two commits into one.

[sanimej]

@thieman We can merge this if you can take care of the minor comments. Sorry about the delay.

[thieman]

@sanimej Updated and rebased on latest master, thanks!

[sanimej]

LGTM

[mavenugo]

LGTM.