validate AdvertiseAddress in kubeadm init and other case #56956

Lion-Wei · 2017-12-08T08:16:15Z

What this PR does / why we need it:

When using kubeadm init --apiserver-advertise-address=****, apiserver-advertise-address` can only be ipv4 or ipv6 address, if people use domain name in this field, will not use it and silently get hostIP.

Add a warning in this case.

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#590

Special notes for your reviewer:

Release note:

NONE

Lion-Wei · 2017-12-08T08:17:40Z

/sig kubeadm
/cc @luxas
@discordianfish

cblecker · 2017-12-08T18:47:11Z

/ok-to-test

fabriziopandini · 2017-12-10T07:13:09Z

@Lion-Wei, thanks for this PR!
Before reviewing it, I was wandering if, instead of giving users an advice that kubeadm used the hostIP instead of the given dns name, we can:

improve --apiserver-advertise-address in order to handle dns names as well (if this is a use case we want to support)
or
block the init sequence before it starts if a dns name is passed to --apiserver-advertise-address (basically raise an error in validation phase)

What do you think about?
/cc @luxas

Lion-Wei · 2017-12-11T01:43:40Z

@fabriziopandini Thanks for replying. I'm not sure if make --apiserver-advertise-address accept dns name is a good idea, it's lots work, and I don't know weather there are other consideration, maybe need some discuss.
Add validation for --apiserver-advertise-address is a good idea for now, I think. There don't have any validate tight now, you can pass any string to this flag.

discordianfish · 2017-12-11T10:13:44Z

@Lion-Wei I'm not completely sure where apiserver-advertise-address is used. I know it's used in the kube-proxy config where supporting a DNS name should be trivial and is required to connect to a ELB on AWS since they don't have static IPs. The apiserver-advertise-address might be used in other place where it's harder to support this. If that's the case, maybe we should have a different flag for 'everything it can use a DNS name'?

discordianfish · 2017-12-13T10:27:15Z

So this is a requirement for HA and probably blocking kubernetes/kubeadm#261 /cc @luxas

petergardfjall · 2017-12-14T06:55:33Z

@discordianfish : you're refering to kubernetes/kubeadm#261, right?

discordianfish · 2017-12-15T11:03:34Z

@petergardfjall Oops, right. Thanks, fixed it!

fabriziopandini · 2017-12-15T14:58:54Z

@Lion-Wei @discordianfish
I propose to discuss this at the next sig-meeting

Even if the advertise address is used in few parts of the kubeadm codebase, in the majority of cases no change are required because the value is used as a string.
However there are two tricky cases that should be addressed:

ipv4 or ipv6 tests
api server probe address

Lion-Wei · 2017-12-16T02:36:20Z

@fabriziopandini Thanks.

fabriziopandini · 2017-12-19T17:33:52Z

@Lion-Wei as discussed in SIG-meeting today, we should go for raising a validation error if a dns name is passed to --apiserver-advertise-address; the apiserver-advertise-address should be a valid ipv4 or ipv6 address.

If the user should use dns names he can always use APIServerCertSANs.

Thanks again for your availability to change the PR in this direction; if you need additional info or help in changing the PR feel free to ping me on slack.

discordianfish · 2017-12-19T18:00:28Z

@fabriziopandini Should I open a new issue for a option to set the kube-proxy endpoint?

Lion-Wei · 2017-12-20T02:11:16Z

@fabriziopandini Thanks for your attention. I will do that ASAP.
@discordianfish I'm not very familiar with APIServerCertSANs, I will look into it, to see if this flag can solve the problem.

discordianfish · 2017-12-21T11:12:03Z

To clarify this:

--apiserver-advertise-address is the bind address. So it needs to be an IP on some interface or 0.0.0.0 to listen on all. Validating this was decided 👍
APIServerCertSANs specifies for which names the certificates generated are valid for. I'm already using this and it works fine 👍
TBD new flag would specify where kube-proxy is suppose to connect to (and this should support DNS names to use a ELB on AWS for example). Without this it's not possible to just run kubeadm init on every new master in a multi master setup, since it will always put in the IP of the last provisioned server in the kube-proxy configmap. 👎

fabriziopandini · 2017-12-21T12:09:43Z

@discordianfish please open a issue for the the kube-proxy endpoint in https://github.com/kubernetes/kubeadm/issues

Lion-Wei · 2017-12-22T09:05:50Z

@fabriziopandini , I found it's little complicated. Because we will set cfg.API.AdvertiseAddress to default in SetInitDynamicDefaults before pre-flight check. And need this args for http proxy check and other thins.
I think maybe use should check this args in SetInitDynamicDefaults part

fabriziopandini · 2017-12-24T10:55:46Z

@Lion-Wei
I think that checking this arg in SetInitDynamicDefaults is acceptable (by the way we are already doing this for KubernetesVersion); please check that this apply to all the commands with --advetise-address in input.

fabriziopandini

Thanks!

One final question to be addressed: it is really necessary to validate parameters inside each phases or it is enough to implement the validation in SetInitDynamicDefaults (which is execute before each phases)?

fabriziopandini · 2017-12-27T14:37:42Z

cmd/kubeadm/app/phases/addons/proxy/proxy.go

@@ -51,6 +52,10 @@ func EnsureProxyAddon(cfg *kubeadmapi.MasterConfiguration, client clientset.Inte
 		return fmt.Errorf("error when creating kube-proxy service account: %v", err)
 	}

+	// validate cfg.API.AdvertiseAddress.
+	if err := validation.ValidateApiserverAdvertiseAddress(cfg.API.AdvertiseAddress); err != nil {


The addon phase is called by three commands: init, upgrade, and phase addon; all this command executes
SetInitDynamicDefaults (that executes a first validation of the apiserver) before invoking EnsureProxyAddon (that execute a second validation)
If possible, please remove the second validation; the same apply to other phases as well

That't true, thanks, sorry didn't notice. : (
Then I guess there don't have other case to validate this flag.

@fabriziopandini Besides, need I add unit test of this case? Seems like masterconfig.go don't have any unit test.

Lion-Wei · 2018-02-08T10:34:24Z

@fabriziopandini , kindly ping, need another /lgtm. : )

fabriziopandini · 2018-02-11T19:19:26Z

/lgtm

cblecker · 2018-02-13T17:32:13Z

ping @krousey @luxas @timothysc for approval

Lion-Wei · 2018-03-26T03:05:54Z

Friendly ping @fabriziopandini @krousey . Since this has been ready for a while.

fabriziopandini · 2018-03-26T15:53:03Z

@Lion-Wei sorry I missed the lgtm removed
/lgtm
/CC @kubernetes/sig-cluster-lifecycle-pr-reviews for approval

cblecker · 2018-03-26T16:58:21Z

/test pull-kubernetes-typecheck

Lion-Wei · 2018-03-31T09:51:03Z

/test all

Lion-Wei · 2018-04-09T09:28:18Z

friendly ping @jbeda @timothysc ,looking forward for approve. : )

timothysc · 2018-04-11T15:28:34Z

/test all

timothysc

/approve

k8s-ci-robot · 2018-04-11T15:29:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, Lion-Wei, 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

k8s-ci-robot · 2018-04-11T16:20:56Z

@Lion-Wei: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gke	eeaa524e29b445c9aa6b4db55a7491d7a6d53139	link	`/test pull-kubernetes-e2e-gke`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fejta-bot · 2018-04-11T18:32:07Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-04-11T20:38:44Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-04-11T21:33:30Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested a review from luxas December 8, 2017 08:17

k8s-github-robot assigned krousey and fabriziopandini Dec 8, 2017

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 Dec 8, 2017

discordianfish mentioned this pull request Dec 21, 2017

Create new flag to specify apiserver address for kube-proxy kubernetes/kubeadm#609

Closed

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 25, 2017

fabriziopandini reviewed Dec 27, 2017

View reviewed changes

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2018

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

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

Add validation of apiserver-advertise-address

a0acc28

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 24, 2018

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2018

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 26, 2018

cblecker assigned jbeda and timothysc Mar 31, 2018

timothysc approved these changes Apr 11, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2018

k8s-github-robot merged commit c8cded5 into kubernetes:master Apr 11, 2018

discordianfish mentioned this pull request Apr 12, 2018

Add configuration item to allow kubeadm join to use a dns name pointing to control plane #59288

Merged

validate AdvertiseAddress in kubeadm init and other case #56956

validate AdvertiseAddress in kubeadm init and other case #56956

Conversation

Lion-Wei commented Dec 8, 2017

Lion-Wei commented Dec 8, 2017

cblecker commented Dec 8, 2017

fabriziopandini commented Dec 10, 2017

Lion-Wei commented Dec 11, 2017

discordianfish commented Dec 11, 2017

discordianfish commented Dec 13, 2017 • edited Loading

petergardfjall commented Dec 14, 2017

discordianfish commented Dec 15, 2017

fabriziopandini commented Dec 15, 2017

Lion-Wei commented Dec 16, 2017 • edited Loading

fabriziopandini commented Dec 19, 2017

discordianfish commented Dec 19, 2017

Lion-Wei commented Dec 20, 2017

discordianfish commented Dec 21, 2017 • edited Loading

fabriziopandini commented Dec 21, 2017

Lion-Wei commented Dec 22, 2017

fabriziopandini commented Dec 24, 2017

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Lion-Wei Dec 28, 2017

Choose a reason for hiding this comment

Lion-Wei Dec 28, 2017

Choose a reason for hiding this comment

Lion-Wei commented Feb 8, 2018

fabriziopandini commented Feb 11, 2018

cblecker commented Feb 13, 2018

Lion-Wei commented Mar 26, 2018

fabriziopandini commented Mar 26, 2018

cblecker commented Mar 26, 2018

Lion-Wei commented Mar 31, 2018

Lion-Wei commented Apr 9, 2018

timothysc commented Apr 11, 2018

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 11, 2018

k8s-ci-robot commented Apr 11, 2018 • edited Loading

fejta-bot commented Apr 11, 2018

k8s-github-robot commented Apr 11, 2018

k8s-github-robot commented Apr 11, 2018

discordianfish commented Dec 13, 2017 •

edited

Loading

Lion-Wei commented Dec 16, 2017 •

edited

Loading

discordianfish commented Dec 21, 2017 •

edited

Loading

fabriziopandini Dec 27, 2017 •

edited

Loading

k8s-ci-robot commented Apr 11, 2018 •

edited

Loading