Support IPv6 #508

drewkerrigan · 2017-09-30T01:07:27Z

Get DC/OS with IPv6

Start a new DC/OS cluster with this image:

wget https://downloads.dcos.io/dcos/testing/pull/1842/dcos_generate_config.sh

Replace existing Mesos-DNS

On each master, update Mesos-DNS config:

In /opt/mesosphere/etc/mesos-dns.json:

Change "IPSources": ["host", "netinfo"], -> "IPSources": ["netinfo", "host"],

Then get the new mesos-dns with the following commands:

cd /opt/mesosphere/packages/mesos-dns--600da87080b7634f2380594499004a7ff0b34662/bin
mv mesos-dns mesos-dns.bak
curl -O https://959-26509179-gh.circle-artifacts.com/0/tmp/circle-artifacts.Dv9WDHl/mesos-dns--linux-amd64 --output mesos-dns--linux-amd64
mv mesos-dns--linux-amd64 mesos-dns
chmod 755 mesos-dns
systemctl restart dcos-mesos-dns.service

Create a test app on `dcos6` network

{
    "id": "/ipv6-nginx",
    "cmd": "nginx -g 'daemon off;'",
    "instances": 1,
    "cpus": 0.1,
    "mem": 32,
    "container": {
        "type": "DOCKER",
        "docker": {
            "image": "nginx:stable"
        },
        "portMappings": [
            {
                "containerPort": 80,
                "protocol": "tcp",
                "name": "web"
            }
        ]
    },
    "requirePorts": false,
    "networks": [
        {
            "name": "dcos6",
            "mode": "container"
        }
    ],
    "healthChecks": [],
    "fetch": [],
    "constraints": []
}

Test the changes

dig ipv6-nginx.marathon.mesos ANY

dig ipv6-nginx.marathon.mesos AAAA

dig _ipv6-nginx._tcp.marathon.mesos SRV

curl leader.mesos:8123/v1/hosts/ipv6-nginx.marathon.mesos

curl leader.mesos:8123/v1/services/_ipv6-nginx._tcp.marathon.mesos

curl leader.mesos:8123/v1/enumerate

urbanserj

LGTM

jdef

Completed an initial, quick pass over the non-test changes. Will take another spin through later.

ipv4 is preferred in several places to ipv6 - why? should this preference instead be controlled by config param?

also, what's up with the flapping mesos-go deps?

i initially thought to review each commit individually but ended up just reviewing the "merged" commit stream. in the future it would be great to have a more well-structured commit stream to simplify the review process

jdef · 2017-10-05T15:51:07Z

records/generator.go

+			}
+			if ipv4 != nil {
+				rg.insertRR(a, ipv4.String(), A)
+				// Prefer ipv4 for slaveIP


why is this a preference? can you elaborate here?

Valid point - I can instead make SlaveIPs a map of lists of strings so that we can later add records for both ipv4 and ipv6 for .slave.mesos recs

jdef · 2017-10-05T15:53:51Z

records/generator.go

 			srv := net.JoinHostPort(a, slave.PID.Port)
 			rg.insertRR("_slave._tcp."+domain+".", srv, SRV)
 		} else {
-			logging.VeryVerbose.Printf("string '%q' for slave with id %q is not a valid IP address", address, slave.ID)
-			address = labels.DomainFrag(address, labels.Sep, spec)
+			logging.VeryVerbose.Printf("string '%q' for slave with id %q is not a valid IP address", slave.PID.Host, slave.ID)


you didn't do this, but since you're changing the line: please remove the single-quotes wrapping the %q

jdef · 2017-10-05T16:11:29Z

records/generator.go

+	} else if ip.To16() != nil {
+		return AAAA
+	}
+	return A


should this panic instead? it's unlikely that we'd ever end up here but I hate silent failures

I agree, my only concern is that there are a bunch of tests with master "ip addresses" like 6, or bob, etc. I assume the tests are just flawed and I can make them into valid ipv4 addresses to pass the tests, but I'm not sure if there's a valid use case for masters with non-ip hostnames in master pids / addresses. Should we maybe just not create records and error log for those cases? I'm also fine with panic.

jdef · 2017-10-05T16:12:03Z

records/generator.go

+	if parsedIP := net.ParseIP(ip); parsedIP != nil {
+		return rrsKindForIP(parsedIP)
+	}
+	return A


panic? otherwise, what are the cases where returning A makes sense here?

See above comment.

jdef · 2017-10-05T16:12:24Z

records/generator.go

+// hostToIPs attempts to parse hostname into an ip. If that doesn't work it will perform a
+// lookup and try to find one ipv4 and one ipv6 in the results.
+func hostToIPs(hostname string) (ipv4, ipv6 net.IP, ok bool) {
+	ok = false


ok = false in superfluous, please remove

jdef · 2017-10-05T16:13:20Z

records/generator.go

+			ipv4 = t4.IP
+			ok = true
+		} else {
+			logging.Error.Printf("cannot translate hostname %q into an ip4 address", hostname)


why do we log this for ipv4 but not for ipv6? should this be verbose?

jdef · 2017-10-05T16:14:33Z

records/generator.go

+			}
+		}
+	}
+	return ipv4, ipv6


just return here w/o args is sufficient

jdef · 2017-10-05T16:14:54Z

records/generator.go

+			ok = true
+		}
+	}
+	return ipv4, ipv6, ok


just return here w/o args is sufficient

jdef · 2017-10-05T16:18:37Z

resolver/resolver.go

@@ -389,6 +415,14 @@ func (res *Resolver) handleSRV(rs *records.RecordGenerator, name string, m, r *d
 			}
 			m.Extra = append(m.Extra, aRR)
 			added[host] = struct{}{}
+		} else if aaaa, ok := rs.AAAAs.First(host); ok {


why only append these if ipv4 isn't found? why can't we return both? (or is that not kosher?)

You're correct, this was an oversight

jdef · 2017-10-05T16:21:40Z

resolver/resolver.go

@@ -718,6 +760,8 @@ func (res *Resolver) RestService(req *restful.Request, resp *restful.Response) {
 		var ip string
 		if r, ok := rs.As.First(host); ok {
 			ip = r
+		} else if r, ok := rs.AAAAs.First(host); ok {


why is ipv4 preferred?

Another oversight, should have both.

drewkerrigan · 2017-10-05T20:56:35Z

@jdef PTAL.

re: "ipv4 is preferred in several places to ipv6 - why?"

This was mainly due to some flawed assumptions I made. After taking another go at it, I realized we can and should use both when available.

re: "also, what's up with the flapping mesos-go deps?"

I initially thought that I would need an updated mesos-go ([WIP] Support masters on ipv6 mesos/mesos-go#328 has an updated upid package) but later decided to just copy a subset of upid into records/state/upid with the fixed Parse function. We should eventually get mesos-go updated though. I also noticed you already had another PR to update mesos-go to the new api/v0 (Upgrade to latest mesos-go #496).

re: "in the future it would be great to have a more well-structured commit stream to simplify the review process"

I can restructure commits before merge or sooner if you'd like - no excuses here :-)

jdef

better, thanks. left a few more comments.

also, mind updating the docs to indicate that ipv6 is now supported?

jdef · 2017-10-09T14:09:00Z

records/generator.go

+			rg.insertTaskRR(arec+".slave"+tail, sIP.String(), rrsKindForIP(sIP), enumTask)
+			rg.insertTaskRR(canonical+".slave"+tail, sIP.String(), rrsKindForIP(sIP), enumTask)
+		} else {
+			// ack: slave IP may not be an actual IP if labels.DomainFrag was used.


good catch. this is suspect. if not debugging/fixing this issue here, should probably create an issue to track

Created #509 and I'll reference from the comment.

jdef · 2017-10-09T14:14:07Z

records/state/state_test.go

@@ -166,8 +166,8 @@ func statuses(st ...Status) taskOpt {
 	}
 }

-func slaveIP(ip string) taskOpt {
-	return func(t *Task) { t.SlaveIP = ip }
+func slaveIPs(ip string) taskOpt {


suggestion: func slaveIPs(ip ...string) taskOpt { ... }

jdef · 2017-10-09T14:26:11Z

resolver/resolver.go

-			if err != nil {
-				errs.Add(err)
-				continue
+		if _, aFound := aAdded[host]; !aFound {


is it worth using different priorities for ipv4 records vs ipv6 records? via http://ipv6friday.org/blog/2012/01/ipv6-and-dns/

An IPv4 client should check the lowest priority first and find out that there’s no available host in that priority and switch to the second priority. An IPv6 client finds a host in the lowest priority. A dual stack client will also find the IPv6 host first and stick to that. DNS SRV records are for you to define how you want to be reached.

Only the SRV records themselves have the Priority field, so this would only make sense if we were creating different SRV hostname records for ipv4 and ipv6 which we aren't currently. The code you commented on is only referring to the additional section. So in the example:

;; QUESTION SECTION: ;_ipv6-nginx._tcp.marathon.mesos. IN SRV ;; ANSWER SECTION: _ipv6-nginx._tcp.marathon.mesos. 60 IN SRV 0 0 80 ipv6-nginx-6ijia-s1.marathon.mesos. ;; ADDITIONAL SECTION: ipv6-nginx-6ijia-s1.marathon.mesos. 60 IN A 12.0.2.2 ipv6-nginx-6ijia-s1.marathon.mesos. 60 IN AAAA fd01:b::2:8000:2

Where there are both IPv4 and IPv6 records for a single host (ipv6-nginx-6ijia-s1.marathon.mesos.), it would be up to the client to decide which they prefer. Priority would only apply if we were doing something like ipv6-nginx-6ijia-s1.marathon.mesos. for A and ipv6-nginx-6ijia-s1.preferred.marathon.mesos. for AAAA, and then we would have 2 separate SRV records for _ipv6-nginx._tcp.marathon.mesos..

You're right, thanks for clarifying.

drewkerrigan · 2017-10-09T21:22:20Z

@jdef PTAL, I made some minor updates to the docs and addressed the other comments.

drewkerrigan · 2017-10-10T15:49:21Z

@jdef @urbanserj ping - updated the description with steps to test it out if you're interested.

jdef

LGTM

Is there a plan to write some DC/OS ipv6 integration tests that will exercise these changes?

drewkerrigan · 2017-10-10T19:19:47Z

@jdef yes we'll add integration tests with the related DC/OS PR

drewkerrigan added 7 commits September 21, 2017 14:55

Updating to latest mesos-go

6440c0b

initial ipv6 record changes

2f1c007

checkpoint, fixing tests

e02532c

Cleanup

a200301

add tests, copy upid parse for now

300a5a3

fix config test

5f9c0eb

fixing ci tests

07c9fed

drewkerrigan requested review from jdef, asridharan, GoelDeepak and urbanserj October 2, 2017 20:25

urbanserj approved these changes Oct 2, 2017

View reviewed changes

drewkerrigan added 3 commits October 4, 2017 15:32

use ipv4 and ipv6 from task status

1263841

update resolver for AAAA

dc9c56a

revert mesos-go update

dc70510

jdef reviewed Oct 5, 2017

View reviewed changes

cleanup logic, add multiple ips for slaves

ea54da1

jdef reviewed Oct 9, 2017

View reviewed changes

Update docs, address pr comments

33877b6

Fix slaveIPs helper

e4dfc16

jdef approved these changes Oct 10, 2017

View reviewed changes

drewkerrigan merged commit be53925 into master Oct 10, 2017

GoelDeepak deleted the ack/ipv6 branch February 10, 2018 02:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support IPv6 #508

Support IPv6 #508

drewkerrigan commented Sep 30, 2017 •

edited

Loading

urbanserj left a comment

jdef left a comment

jdef Oct 5, 2017

drewkerrigan Oct 5, 2017

jdef Oct 5, 2017

jdef Oct 5, 2017

drewkerrigan Oct 5, 2017

jdef Oct 5, 2017

drewkerrigan Oct 5, 2017

jdef Oct 5, 2017

jdef Oct 5, 2017

jdef Oct 5, 2017

jdef Oct 5, 2017

jdef Oct 5, 2017

drewkerrigan Oct 5, 2017

jdef Oct 5, 2017

drewkerrigan Oct 5, 2017

drewkerrigan commented Oct 5, 2017 •

edited

Loading

jdef left a comment

jdef Oct 9, 2017

drewkerrigan Oct 9, 2017

jdef Oct 9, 2017

jdef Oct 9, 2017

drewkerrigan Oct 9, 2017

jdef Oct 10, 2017

drewkerrigan commented Oct 9, 2017

drewkerrigan commented Oct 10, 2017

jdef left a comment

drewkerrigan commented Oct 10, 2017

Support IPv6 #508

Support IPv6 #508

Conversation

drewkerrigan commented Sep 30, 2017 • edited Loading

Get DC/OS with IPv6

Replace existing Mesos-DNS

Create a test app on dcos6 network

Test the changes

urbanserj left a comment

Choose a reason for hiding this comment

jdef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewkerrigan commented Oct 5, 2017 • edited Loading

jdef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewkerrigan commented Oct 9, 2017

drewkerrigan commented Oct 10, 2017

jdef left a comment

Choose a reason for hiding this comment

drewkerrigan commented Oct 10, 2017

drewkerrigan commented Sep 30, 2017 •

edited

Loading

Create a test app on `dcos6` network

drewkerrigan commented Oct 5, 2017 •

edited

Loading