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

Do not add IP to name records for aliases #2299

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

olljanat
Copy link
Contributor

@olljanat olljanat commented Nov 10, 2018

- What I did
Do not add IP to name records for aliases so IP to name resolution will always reply container name.

- How I did it
Send ipMapUpdate = false to addSvcRecords function so it will not add IP to name records https://github.com/docker/libnetwork/blob/09e39d99d5c769cb7d167058504581d1cfc779e3/network.go#L1422-L1427

- How to verify it
This can be easily tested with commands:

docker network create --internal --attachable --driver overlay --scope swarm foo-network
docker run -it --name bar-container --rm --network foo-network nicolaka/netshoot
for i in {1..1000}
do
  nslookup $(hostname -i) | grep "arpa" | grep -v "bar-container"
done

without this one about 100-150 / 1000 tests fails.

Fixes moby/moby#34882

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>
@olljanat
Copy link
Contributor Author

@fcrisciani I can see that this code have been added by your PR #1796

What are you thoughts is this correct way to fix the issue?

@fcrisciani
Copy link

@olljanat in general I would say that the answer is not "wrong" in the sense that if you try to resolve the reverse translation answer you will get the same IP as a result.

Looking at the patch I have to see that the attachable container case is not going to be broken with your suggested change. Anyway I would like to dig more on the real use case and what is being broken by this behavior to better think to a proper solution

@olljanat
Copy link
Contributor Author

olljanat commented Nov 12, 2018

@fcrisciani I noticed this issue when I was looking for that if it is possible to modify Redis Sentinel to store hostnames to config files instead of IPs which would make it working with static IPs and would avoid needed for: moby/moby#24170

Here is example config from my lab which is updated by Redis Sentinel

sentinel myid c73d00854dd76658fabc38199394d23d02890533
sentinel deny-scripts-reconfig yes
sentinel resolve-hostnames yes
sentinel monitor redis-cluster redis1.redis 6379 2
sentinel down-after-milliseconds redis-cluster 5000

# Generated by CONFIG REWRITE
port 26379
dir "/data"
maxclients 4064
protected-mode no
sentinel failover-timeout redis-cluster 10000
sentinel config-epoch redis-cluster 0
sentinel leader-epoch redis-cluster 0
sentinel known-replica redis-cluster redis3.redis 6379
sentinel known-replica redis-cluster redis2.redis 6379
sentinel known-sentinel redis-cluster sentinel2.redis 26379 e01aa25470f3464e22bfa312e3d2afdfdb026f88
sentinel known-sentinel redis-cluster sentinel3.redis 26379 69f4033663862814f0dcbc90932fc4eb38f26b1a
sentinel current-epoch 0

and without this change time to time there was container id instead of these redisX.redis or sentinelX.redis hostnames which makes is breaking when container is re-created by swarm and container id changes.

Internally Redis uses IPs for all connections so that why I need reverse DNS for resolve hostnames for those auto discovered cluster nodes.

Reason why I'm looking for this is that currently only way make Redis Sentinel working is create overlay network with manually specified subnet and join standalone containers with static IPs to it.

@TincaTibo
Copy link

TincaTibo commented Nov 13, 2018

@fcrisciani Hi, I have this issue on a distributed system analyser I'm working on: I'm capturing network communication between the containes in the Swarm to analyse and troubleshoot system behavior.
I need to be able to associate the IP to a name for better machine and human processing like:

  • identifying the service that talks
  • aggregating flows from all replicas of the same service.

Having the IP constantly changing name when reverse resolving it is a pain. I overcame it by settings rules of acceptance over the name resolution result, but this is an ugly hack.

@cgaspar-deshaw
Copy link

I ran into this as HBase requires forward/reverse name lookups to match. This fixes the bug that makes HBase unusable in a docker container on an encrypted overlay network.

@olljanat
Copy link
Contributor Author

@fcrisciani did you got enough explanations why this is needed?

These are bit rare use cases and that why most users have been able live without this fix but at least I cannot image any use case where current approach would be needed so can we merge this one?

Copy link

@fcrisciani fcrisciani left a comment

Choose a reason for hiding this comment

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

LGTM

I dig a bit into the logic and the attachable containers seems to still be working fine because if they have a name that will match with the container name, else if a name is not specified the container name is set to the short version of the container ID that is also the same as the only alias that is set.

@fcrisciani fcrisciani merged commit 309de09 into moby:master Nov 29, 2018
@fcrisciani
Copy link

Thanks @olljanat for the contribution!
Only one comment on leveraging the task name given by swarmkit, there is no real guarantee from swarmkit side that if you scale up a service from 1 to 10 replicas and then scale it back down to 1 replica that the only task remaining is the one with name bla.1.redis. Like the .1. is just a number that keeps the task name unique but not a deterministic way to know how many replicas of a service are present.

@olljanat
Copy link
Contributor Author

@fcrisciani good point but that is not most probably issue at least on my use cases because I'm planning to use MaxReplicas=1 setting ( moby/moby#37940 ) with these services so I can use --hostname="{{.Service.Name}}" switch with them.

@olljanat olljanat deleted the ip-to-hostname-fix branch November 29, 2018 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants