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

Make DNS records and queries case-insensitive #2394

Merged
merged 1 commit into from
Jun 20, 2019

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Jun 14, 2019

RFC434 states that DNS Servers should be case insensitive
This commit makes sure that all DNS queries will be translated
to lower ASCII characters and all svcRecords will be saved in
lower case to abide by the RFC

Fixes - https://github.com/moby/moby/issues/21169

Signed-off-by: Arko Dasgupta [email protected]

@selansen
Copy link
Collaborator

@arkodg as per your comment on moby, it will be a problem if we create container Foo and foo and attach in the same network. so we need to make sure it doesn't break existing functionality.

Below is what the test I tried on 19.03

 docker run -itd --network=nw1 --name=COnt1 busybox
 docker container ls | grep -i cont1
d32c30ffa045        busybox                                  "sh"                     37 seconds ago       Up 35 seconds                                                                                            COnt1
4d8a9cd286f4        busybox                                  "sh"                     About a minute ago   Up About a minute                                                                                        cont1```


Moby comment:
=============
Make sure we return a err.Conflict if two containers with different names but same case-insensitive names try to connect to the same network (eg. containers FOO and foo connecting to network bar should NOT be permitted) . 

@thaJeztah
Copy link
Member

Should this take some round-robin approach if multiple containers share the same hostname (in different case), but prioritise "exact match" over "match, but different casing"?

I think that was the proposal in; moby/moby#28689 (comment)

  1. For non-colliding names, let's just return the case insensitive match.
  2. For colliding names, favor the one that matches case.
  3. For full collision, return both records pseudo-randomly.

Oh, could you remove Fixes - moby/moby#21169 from the commit-message? (or change to "addresses" / "relates to")

@arkodg
Copy link
Contributor Author

arkodg commented Jun 17, 2019

@thaJeztah the round-robin approach introduces non deterministic behavior so I would not want to implement that

@selansen
Copy link
Collaborator

@thaJeztah the round-robin approach introduces non deterministic behavior so I would not want to implement that

Agree. Thats will cause wired behavior.

@arkodg arkodg force-pushed the dns-lookup-case-insensitive branch from be98208 to 91601e1 Compare June 18, 2019 01:14
@thaJeztah
Copy link
Member

True, this can be ambiguous, but round-robin is currently supported, and definitely has valid use-cases;

example 1: "blue(ish) / green(ish) deployments"

mkdir case-sense && cd case-sense

docker network create -d overlay --attachable myoverlay
cat > docker-compose.yml -<<'EOF'
version: "3.5"
services:
  web:
    image: nginx:alpine
networks:
  default:
    external: true
    name: myoverlay
EOF
docker stack deploy -c docker-compose.yml stackblue
docker stack deploy -c docker-compose.yml stackgreen

docker run --rm --network myoverlay alpine nslookup web
nslookup: can't resolve '(null)': Name does not resolve

Name:      web
Address 1: 10.0.1.10
Address 2: 10.0.1.13

Example 2: docker compose with aliases and scaling

cat > docker-compose.yml -<<'EOF'
version: "3.5"
services:
  web:
    image: nginx:alpine
    networks:
      default:
        aliases:
         - foobar

networks:
  default:
    external: true
    name: myoverlay
EOF
docker-compose --project-name=example up -d --scale web=3

docker run --rm --network myoverlay alpine nslookup foobar
nslookup: can't resolve '(null)': Name does not resolve

Name:      foobar
Address 1: 10.0.1.20 example_web_1.myoverlay
Address 2: 10.0.1.21 example_web_2.myoverlay
Address 3: 10.0.1.22 example_web_3.myoverlay

@thaJeztah
Copy link
Member

Oh, looks like you made a typo in your commit message; RFC434 should be RFC4343

@thaJeztah
Copy link
Member

Reading the RFC to check if case should be preserved, which seems to be the case (RFC4343 section 4);

While ASCII label comparisons are case insensitive, [STD13] says case
MUST be preserved on output and preserved when convenient on input.

Also of note; RFC4034, section 6.2;

all uppercase US-ASCII letters in the owner name of the RR are
replaced by the corresponding lowercase US-ASCII letters;

Which might be different than tolower (if non-US-ASCII characters are present); wondering if some form of strings.ToLowerSpecial() would be needed (during resolve)) otherwise strings.EqualFold could be used

@arkodg
Copy link
Contributor Author

arkodg commented Jun 18, 2019

With the current changes this is the o/p

docker network create bar
docker run -d --name FOO --net bar nginx:alpine
docker run -d --name foo --net bar nginx:alpine

docker run -it --rm --net bar alpine nslookup foo

nslookup: can't resolve '(null)': Name does not resolve

Name:      foo
Address 1: 172.20.0.2 foo.bar
Address 2: 172.20.0.3 foo.bar

I guess what you're saying is we need to preserve the name for FOO.bar, but the way we build the DNS name is an implementation detail in libnetwork right ?

    RFC434 states that DNS Servers should be case insensitive
    This commit makes sure that all DNS queries will be translated
    to lower ASCII characters and all svcRecords will be saved in
    lower case to abide by the RFC

    Relates to moby/moby#21169

Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg force-pushed the dns-lookup-case-insensitive branch from 91601e1 to 49b1a51 Compare June 19, 2019 18:24
@arkodg
Copy link
Contributor Author

arkodg commented Jun 19, 2019

PTAL @thaJeztah @selansen

with the latest changes the behavior looks like the following

docker network create bar
docker run -d --name FOO --net bar nginx:alpine
docker run -d --name foo --net bar nginx:alpine

docker run -it --rm --net bar alpine nslookup fOo

nslookup: can't resolve '(null)': Name does not resolve

Name:      fOo
Address 1: 172.20.0.3 foo.bar
Address 2: 172.20.0.2 FOO.bar

@selansen
Copy link
Collaborator

This looks good @arkodg

Copy link
Collaborator

@selansen selansen left a comment

Choose a reason for hiding this comment

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

LGTM

@euanh euanh merged commit 19e372a into moby:master Jun 20, 2019
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jun 25, 2019
full diffs:

- moby/libnetwork@fc5a7d9...62a13ae
- vishvananda/netlink@b2de5d1...v1.0.0
- vishvananda/netns@604eaf1...13995c7

notable changes in libnetwork:

- moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0
- moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule
  - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER
  - addresses moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules
  - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway
- moby/libnetwork#2394 Make DNS records and queries case-insensitive
  - addresses moby#28689 Embedded DNS is case-sensitive
  - addresses moby#21169 hostnames with new networking are case-sensitive

Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jun 27, 2019
full diffs:

- moby/libnetwork@fc5a7d9...62a13ae
- vishvananda/netlink@b2de5d1...v1.0.0
- vishvananda/netns@604eaf1...13995c7

notable changes in libnetwork:

- moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0
- moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule
  - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER
  - addresses moby/moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules
  - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway
- moby/libnetwork#2394 Make DNS records and queries case-insensitive
  - addresses moby/moby#28689 Embedded DNS is case-sensitive
  - addresses moby/moby#21169 hostnames with new networking are case-sensitive

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: 344b093258fcb2195fa393081e5224a6c766c798
Component: engine
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Sep 16, 2019
full diffs:

- moby/libnetwork@fc5a7d9...62a13ae
- vishvananda/netlink@b2de5d1...v1.0.0
- vishvananda/netns@604eaf1...13995c7

notable changes in libnetwork:

- moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0
- moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule
  - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER
  - addresses moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules
  - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway
- moby/libnetwork#2394 Make DNS records and queries case-insensitive
  - addresses moby#28689 Embedded DNS is case-sensitive
  - addresses moby#21169 hostnames with new networking are case-sensitive

Signed-off-by: Sebastiaan van Stijn <[email protected]>
(cherry picked from commit 344b093)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Sep 17, 2019
full diffs:

- moby/libnetwork@fc5a7d9...62a13ae
- vishvananda/netlink@b2de5d1...v1.0.0
- vishvananda/netns@604eaf1...13995c7

notable changes in libnetwork:

- moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0
- moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule
  - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER
  - addresses moby/moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules
  - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway
- moby/libnetwork#2394 Make DNS records and queries case-insensitive
  - addresses moby/moby#28689 Embedded DNS is case-sensitive
  - addresses moby/moby#21169 hostnames with new networking are case-sensitive

Signed-off-by: Sebastiaan van Stijn <[email protected]>
(cherry picked from commit 344b093258fcb2195fa393081e5224a6c766c798)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: f3e1aff81df959e9178433b77e7f3364c22aee59
Component: engine
burnMyDread pushed a commit to burnMyDread/moby that referenced this pull request Oct 21, 2019
full diffs:

- moby/libnetwork@fc5a7d9...62a13ae
- vishvananda/netlink@b2de5d1...v1.0.0
- vishvananda/netns@604eaf1...13995c7

notable changes in libnetwork:

- moby/libnetwork#2366 Bump vishvananda/netlink to 1.0.0
- moby/libnetwork#2339 controller: Check if IPTables is enabled for arrangeUserFilterRule
  - addresses moby/libnetwork#2158 dockerd when run with --iptables=false modifies iptables by adding DOCKER-USER
  - addresses moby#35777 With iptables=false dockerd still creates DOCKER-USER chain and rules
  - addresses docker/for-linux#136 dockerd --iptables=false adds DOCKER-USER chain and modify FORWARD chain anyway
- moby/libnetwork#2394 Make DNS records and queries case-insensitive
  - addresses moby#28689 Embedded DNS is case-sensitive
  - addresses moby#21169 hostnames with new networking are case-sensitive

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: zach <[email protected]>
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