Fix kubeadm etcd manifests to use brackets around IPv6 addrs #70633

leblancd · 2018-11-05T01:26:45Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Kubeadm init with an IPv6-only configuration is currently failing to bring up a cluster. In this failure condition, kubeadm is waiting forever for the control plane to come up. This failure is described in kubernetes/kubeadm Issue # 1212. The root cause is that kubeadm is generating an erroneous etcd.yaml manifest that has entries of the form:

   - --advertise-client-urls=https://fd00:20::2:2379

And this is causing the etcd server to crash.
For IPv6 addresses, this URL should use square brackets around the IPv6 address, e.g.:

   - --advertise-client-urls=https://[fd00:20::2]:2379

The fix is to use net.JoinHostPort() to generate URLs such as the above.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm # 1212

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
NONE

NONE

leblancd · 2018-11-05T01:29:10Z

/area ipv6

rosti

@leblancd thanks for your contribution! Looks OK to me.

One tiny nit though. It looks to me, that we have too many times code in the spirit of:

fmt.Sprintf("https://%s", net.JoinHostPort(cfg.APIEndpoint.AdvertiseAddress, strconv.Itoa(kubeadmconstants.EtcdListenClientPort)))

Perhaps we should add an utility function to k8s.io/kubernetes/cmd/kubeadm/app/util/etcd that takes AdvertiseAddress as an argument?

rosti · 2018-11-05T11:26:17Z

/ok-to-test

pmichali

/lgtm Looks like that'll resolve the regeression.

leblancd · 2018-11-05T11:53:51Z

@rosti - I can create a utility function for generating the URLs. This would need to be passed the port as well as the IP address / host. Any preference for name, e.g. etcdutil.URL() or etcdutil.genURL()?
BTW, looks like a github outage is causing test failures?

pmichali

BTW, I see there is a TestGetEtcdCommand() that at least tests some of the URLs generated. Could you add some test cases there, for IPv6? Can you check the other areas to see if there are existing tests that could be adapted to test IPv6 variants?

It looks like there are only two ports used. It would be useful to have a pair of helper methods to generate the URL strings, and use them, to reduce duplication.

rosti · 2018-11-05T13:02:46Z

@leblancd you can add a couple of tiny utility funcs in there, named GetClientURL and GetPeerURL, so that we keep code more understandable and not have to pass the port constants.

leblancd · 2018-11-05T15:37:28Z

@rosti - Roger that, I understand your earlier comment now, thanks.

neolit123

@leblancd thank you for this bug fix.
the _test.go files accompanying the touched files would need some updates too.
i.e. we need to add test coverage for IPv6 where applicable.

neolit123 · 2018-11-05T20:37:30Z

/priority important-longterm

neolit123 · 2018-11-05T20:46:12Z

@pmichali your PR here is newer and seems to be conflicting with this one on the kubeadm side.

@pmichali and @leblancd please resolve which PR is going to touch kubeadm. we can make this the kubeadm change and the other one the kubelet change.
thanks.

leblancd · 2018-11-05T23:13:28Z

@neolit123 - @pmichali 's PR #70659 is temporarily including my changes because his fix depends on my fix being in place. This change (PR #70633) should be considered the fix for IPv6 errors in kubeadm etcd.yaml .

leblancd · 2018-11-05T23:15:23Z

@neolit123 , @pmichali - I've added unit test cases for local.go and etcd.go.

neolit123 · 2018-11-05T23:43:20Z

thanks for the update.
/retest

timothysc

I'd like one of the intel folks to review in the broader context, b/c they've done most of the IPv6 work.

timothysc · 2018-11-06T01:06:47Z

cmd/kubeadm/app/util/etcd/etcd.go

I don't think this really needs to live here, but I don't think it's blocking for me.

/approve
/assign @kad
/cc @bart0sh

I'd do simpler "https://" + net.JoinHostPort(...) instead of calling that function, but this not critical.

@leblancd please just remove the utility as per the above comments then LGTM.
thanks!

k8s-ci-robot · 2018-11-06T01:07:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leblancd, timothysc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [timothysc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

When 'kubeadm init ...' is used with an IPv6 kubeadm configuration, kubeadm currently generates an etcd.yaml manifest that uses IP:port combinatins where the IP is an IPv6 address, but it is not enclosed in square brackets, e.g.: - --advertise-client-urls=https://fd00:20::2:2379 For IPv6 advertise addresses, this should be of the form: - --advertise-client-urls=https://[fd00:20::2]:2379 The lack of brackets around IPv6 addresses in cases like this is causing failures to bring up IPv6-only clusters with Kubeadm as described in kubernetes/kubeadm Issues kubernetes#1212. This format error is fixed by using net.JoinHostPort() to generate URLs as shown above. Fixes kubernetes/kubeadm Issue kubernetes#1212

rosti

Thank you @leblancd ! Looks good!

rosti · 2018-11-19T10:48:44Z

cmd/kubeadm/app/util/etcd/etcd.go

+// GetClientURL creates an HTTPS URL that uses the configured advertise
+// address and client port for the API controller
+func GetClientURL(cfg *kubeadmapi.InitConfiguration) string {
+	return "https://" + net.JoinHostPort(cfg.LocalAPIEndpoint.AdvertiseAddress, strconv.Itoa(constants.EtcdListenClientPort))


Could have used GetClientURLByIP here, but it's not bad the way it is either.

rosti · 2018-11-19T10:50:28Z

/test pull-kubernetes-e2e-gce-100-performance

kad · 2018-11-20T14:59:43Z

/lgtm

k8s-ci-robot requested review from luxas and timothysc November 5, 2018 01:26

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2018

k8s-ci-robot added area/ipv6 release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 5, 2018

rosti reviewed Nov 5, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 5, 2018

pmichali approved these changes Nov 5, 2018

View reviewed changes

pmichali reviewed Nov 5, 2018

View reviewed changes

neolit123 reviewed Nov 5, 2018

View reviewed changes

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 5, 2018

leblancd force-pushed the kubeadm_etcd_v6_fix branch from 9b63f43 to d28e564 Compare November 5, 2018 23:02

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 5, 2018

leblancd force-pushed the kubeadm_etcd_v6_fix branch from d28e564 to b3821fb Compare November 5, 2018 23:20

timothysc approved these changes Nov 6, 2018

View reviewed changes

k8s-ci-robot assigned kad Nov 6, 2018

k8s-ci-robot requested a review from bart0sh November 6, 2018 01:07

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 6, 2018

leblancd force-pushed the kubeadm_etcd_v6_fix branch from b3821fb to 9988771 Compare November 19, 2018 02:50

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2018

rosti approved these changes Nov 19, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 20, 2018

timothysc added this to the v1.13 milestone Nov 20, 2018

timothysc added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Nov 20, 2018

k8s-ci-robot merged commit f8983a8 into kubernetes:master Nov 20, 2018

pmichali mentioned this pull request Nov 24, 2018

kubelet: get IP based on service network IP mode for dual-stack support. #70659

Closed

abasitt mentioned this pull request Jan 6, 2023

clustermesh-api server failing to init in IPv6 single stack cluster cilium/cilium#22952

Closed

2 tasks

Fix kubeadm etcd manifests to use brackets around IPv6 addrs #70633

Fix kubeadm etcd manifests to use brackets around IPv6 addrs #70633

Uh oh!

Conversation

leblancd commented Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leblancd commented Nov 5, 2018

Uh oh!

rosti left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosti commented Nov 5, 2018

Uh oh!

pmichali left a comment

Choose a reason for hiding this comment

Uh oh!

leblancd commented Nov 5, 2018

Uh oh!

pmichali left a comment

Choose a reason for hiding this comment

Uh oh!

rosti commented Nov 5, 2018

Uh oh!

leblancd commented Nov 5, 2018

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

neolit123 commented Nov 5, 2018

Uh oh!

neolit123 commented Nov 5, 2018

Uh oh!

leblancd commented Nov 5, 2018

Uh oh!

leblancd commented Nov 5, 2018

Uh oh!

neolit123 commented Nov 5, 2018

Uh oh!

timothysc left a comment

Choose a reason for hiding this comment

Uh oh!

timothysc Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

kad Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

neolit123 Nov 7, 2018

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 6, 2018

Uh oh!

rosti left a comment

Choose a reason for hiding this comment

Uh oh!

rosti Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

rosti commented Nov 19, 2018

Uh oh!

kad commented Nov 20, 2018

Uh oh!

Uh oh!

leblancd commented Nov 5, 2018 •

edited

Loading

rosti left a comment •

edited

Loading