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

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

Merged
merged 2 commits into from Feb 22, 2018

Conversation

@stevesloka
Contributor

stevesloka commented Feb 2, 2018

This adds a new flag (--apiserver-advertise-dns-address) to kubeadm which is used in node kubelet.confg to point to API server allowing users to define a DNS entry instead of an IP address.

Fixes kubernetes/kubeadm#411

Adds new flag `--apiserver-advertise-dns-address` which is used in node kubelet.confg to point to API server

// @timothysc @craigtracey

@timothysc

This comment has been minimized.

Member

timothysc commented Feb 3, 2018

@@ -145,6 +145,7 @@ func getAddonsSubCommands() []*cobra.Command {
if properties.use == "all" || properties.use == "kube-proxy" {
cmd.Flags().StringVar(&cfg.API.AdvertiseAddress, "apiserver-advertise-address", cfg.API.AdvertiseAddress, `The IP address or DNS name the API server is accessible on`)
cmd.Flags().StringVar(&cfg.API.AdvertiseDNSAddress, "apiserver-advertise-dns-address", cfg.API.AdvertiseDNSAddress, `The DNS name the API server is accessible on`)

This comment has been minimized.

@timothysc

timothysc Feb 3, 2018

Member

I think I've turned a blind eye to some of the flag proliferation, this seems awfully redundant.

/cc @errordeveloper

This comment has been minimized.

@kapilt

kapilt Feb 7, 2018

part of the issue is the previous advertiseaddress was actually a listen address not an advertise.

@timothysc timothysc self-assigned this Feb 3, 2018

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Feb 3, 2018

Need a way to allow users to specify dns name for API server as well as the ip address in the event the API server host machine has multiple.

There are the San certs, however multiple can be passed so which one would you choose.

@jamiehannaford

This comment has been minimized.

Member

jamiehannaford commented Feb 4, 2018

Any reason why we can't just use --apiserver-cert-extra-sans? We'd also need to make sure the bootstrap config files are using the DNS name provided by join cmd (rather than IP which seems to currently be the case unless I'm mistaken).

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Feb 5, 2018

I started there when I began thinking through this PR to use --apiserver-cert-extra-sans, but you can pass multiple sans, so which one should I use? We could have a convention that the first one passed is the one to use, but thought that might be more confusing.

@craigtracey

I think this is a pretty important change, and this will really help us close this gap. Thanks, @stevesloka!

@@ -57,7 +57,7 @@ func SetupMasterConfigurationFile(t *testing.T, tmpdir string, cfg *kubeadmapi.M
kind: MasterConfiguration
certificatesDir: {{.CertificatesDir}}
api:
advertiseAddress: {{.API.AdvertiseAddress}}
advertiseAddress: {{.API.AdvertiseDNSAddress}}

This comment has been minimized.

@craigtracey

craigtracey Feb 6, 2018

Contributor

Is this change intentional? AdvertiseAddress should not be the same as AdvertiseDNSAddress

This comment has been minimized.

@stevesloka

stevesloka Feb 6, 2018

Contributor

Nope that's a mistake, thanks for catching.

AdvertiseAddress string `json:"advertiseAddress"`
// AdvertiseDNSAddress sets the DNS address for the API server to advertise.
AdvertiseDNSAddress string `json:"advertiseDNSAddress"`

This comment has been minimized.

@craigtracey

craigtracey Feb 6, 2018

Contributor

I find this naming a bit confusing as I don't think anything is being "advertised." While the kubelet uses the terminology of advertiseAddress to specify which IP the server is listening on, this new argument is being used only to indicate where a node should register itself. Perhaps a more descriptive argument would be controlPlaneEndpoint?

This comment has been minimized.

@stevesloka

stevesloka Feb 6, 2018

Contributor

I think controlPlane make sense, I'll move to that, take out the args, and do a verify it works via the config.

@timothysc

This comment has been minimized.

Member

timothysc commented Feb 6, 2018

per sig conversation, we should leverage the config object to allow for updates and try not to proliferate cmd line flags b/c anything we do will have to be maintained post GA.

@stevesloka stevesloka changed the title from Add new flag for api-server-dns-name / use for client kubelet.conf to Add configuration item to allow kubeadm join to use a dns name pointing to control plane Feb 7, 2018

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Feb 7, 2018

@timothysc @craigtracey I updated to only use the config, also rebased and pushed.

@@ -143,7 +143,7 @@ func getControlPlaneSubCommands(outDir, defaultKubernetesVersion string) []*cobr
cmd.Flags().StringVar(&cfg.KubernetesVersion, "kubernetes-version", cfg.KubernetesVersion, `Choose a specific Kubernetes version for the control plane`)
if properties.use == "all" || properties.use == "apiserver" {
cmd.Flags().StringVar(&cfg.API.AdvertiseAddress, "apiserver-advertise-address", cfg.API.AdvertiseAddress, "The IP address or DNS name the API server is accessible on")
cmd.Flags().StringVar(&cfg.API.AdvertiseAddress, "apiserver-advertise-address", cfg.API.AdvertiseAddress, "The IP address of the API server is accessible on")

This comment has been minimized.

@timothysc

timothysc Feb 9, 2018

Member

So I need to look through history here. I think other changes that crib'd hostIP from self-hosting affected this. Sorry it's taking a while but we want the original intent to be preserved. @kad if you have a moment could you dig the change, otherwise i'll have to search for it tomorrow.

This comment has been minimized.

@craigtracey

craigtracey Feb 9, 2018

Contributor

This is actually a defect. The kube-apiserver only accepts an IP for --advertise-address.
https://kubernetes.io/docs/reference/generated/kube-apiserver/

I believe what @stevesloka has here is correct.

This comment has been minimized.

@discordianfish

discordianfish Apr 12, 2018

Contributor

Just for reference: Yes this can only be an IP and #56956 was just merged to validate this.

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Feb 13, 2018

Does anyone have time this week to review? Curious if the implementation is acceptable or if anyone has any additional comments.

@timothysc

This comment has been minimized.

Member

timothysc commented Feb 13, 2018

/approve

@kubernetes/sig-cluster-lifecycle-pr-reviews - For transparency I'd prefer someone non-heptio to lgtm

@xiangpengzhao

a couple of concerns. otherwise LGTM.

@@ -548,5 +548,10 @@ func getAltNames(cfg *kubeadmapi.MasterConfiguration) (*certutil.AltNames, error
}
}
// add api server dns advertise address
if len(cfg.API.ControlPlaneEndpoint) > 0 {

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 13, 2018

Member

Shouldn't we also validate that ControlPlaneEndpoint is a proper DNS subdomain, i.e., validation.IsDNS1123Subdomain(ControlPlaneEndpoint)?

This comment has been minimized.

@stevesloka

stevesloka Feb 14, 2018

Contributor

I added this and also covered with tests.

return "", fmt.Errorf("error parsing address %s", cfg.API.AdvertiseAddress)
var masterIP string
if len(cfg.API.ControlPlaneEndpoint) > 0 {
masterIP = cfg.API.ControlPlaneEndpoint

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 13, 2018

Member

validation.IsDNS1123Subdomain(ControlPlaneEndpoint) here as well, and if the validation fails, error out.

var masterIP string
if len(cfg.API.ControlPlaneEndpoint) > 0 {
masterIP = cfg.API.ControlPlaneEndpoint
} else {

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 13, 2018

Member

I'm also wondering what if when both cfg.API.AdvertiseAddress and cfg.API.ControlPlaneEndpoint are configured. The current change only returns cfg.API.ControlPlaneEndpoint as master IP. But this seems ok, anyway.

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Feb 13, 2018

/ok-to-test

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Feb 14, 2018

/test pull-kubernetes-bazel-test

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Feb 15, 2018

@errordeveloper I got those tests fixed up, looks like the build is good now.

@kad

small nit: may I suggest to split autogenerated changes to separate commit ?

if len(cfg.API.ControlPlaneEndpoint) > 0 {
errs := validation.IsDNS1123Subdomain(cfg.API.ControlPlaneEndpoint)
if len(errs) > 0 {
return "", fmt.Errorf("error parsing `ControlPlaneEndpoint` to valid dns subdomain")

This comment has been minimized.

@kad

kad Feb 15, 2018

Member

Maybe here we want to provide to user more information about error ? e.g. what is wrong with domain provided.

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Feb 15, 2018

@kad I spit the commits into 2, one for code and the other for autogen bazel deps. Also, I have the validation of the DNS address return the errors from validation method, and updated the error message to show those errors which get returned.

@kad

This comment has been minimized.

Member

kad commented Feb 15, 2018

@stevesloka Thanks. Changes in cmd/kubeadm/app/apis/kubeadm/v1alpha1/zz_generated.conversion.go seems to be still in first commit. The rest looks good for me.

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Feb 15, 2018

@kad I've moved the zz_generated over to the 2nd commit

@stevesloka

This comment has been minimized.

Contributor

stevesloka commented Feb 20, 2018

Does anyone have any additional comments on this PR?

@timothysc

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 22, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 22, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 22, 2018

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 22, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 22, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit d7cadf5 into kubernetes:master Feb 22, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation stevesloka authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@stevesloka stevesloka deleted the stevesloka:apiServerDNS branch Feb 22, 2018

k8s-ci-robot added a commit to kubernetes/website that referenced this pull request Apr 12, 2018

Add controlPlaneEndpoint to config file reference (#7909)
First-time kubeadm user here: The v1.10.0 docs indicate that config file use is experimental, but judging from the comments on kubernetes/kubernetes#59288 it looks like new features are beginning to be supported only in the config file. controlPlaneEndpoint is super useful for us and while I suspect the kubeadm init docs will ultimately be re-written to emphasize the config file options rather than flags (with appropriate examples and descriptions as the flags have now), it would've saved us some time today if we'd seen controlPlaneEndpoint in the config file example.

discordianfish added a commit to itskoko/kubecfn that referenced this pull request Apr 12, 2018

Upgrade kubeadm to v1.10.1-beta.0, drop cm-patcher
This switches kubeadm to use the new ControlPlaneEndpoint options
introduced in kubernetes/kubernetes#59288

zacharysarah added a commit to kubernetes/website that referenced this pull request Apr 16, 2018

Add controlPlaneEndpoint to config file reference (#7909)
First-time kubeadm user here: The v1.10.0 docs indicate that config file use is experimental, but judging from the comments on kubernetes/kubernetes#59288 it looks like new features are beginning to be supported only in the config file. controlPlaneEndpoint is super useful for us and while I suspect the kubeadm init docs will ultimately be re-written to emphasize the config file options rather than flags (with appropriate examples and descriptions as the flags have now), it would've saved us some time today if we'd seen controlPlaneEndpoint in the config file example.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment