kubeadm: Remove cluster name from JoinConfiguration #70757

ereslibre · 2018-11-07T19:48:09Z

What type of PR is this?
/kind cleanup

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

Special notes for your reviewer:
None

Does this PR introduce a user-facing change?:

NONE

timothysc · 2018-11-07T19:56:48Z

/ok-to-test

neolit123 · 2018-11-07T20:06:48Z

/priority important-soon

timothysc

Same comment about failed conversion test.

timothysc · 2018-11-07T20:09:24Z

/assign @fabriziopandini @luxas @timothysc

cmd/kubeadm/app/discovery/discovery.go

ereslibre · 2018-11-07T20:52:09Z

I will add the conversion test tomorrow.

fabriziopandini

@ereslibre thanks for this PR!

if I got this right, JoinConfiguration.ClusterName is used only to set the cluster name in the config map that gets returned from Discovery, and you are proposing to replace it with a constant.

I'm fine with this, even if I would use existing const DefaultClusterName instead of creating a new const & I will remove parameter passing this const along the chain of calls.

Now, what should be decided is if it is fine to keep the above constant as a clusterName when the above config will be saved into the bootstrap-kubelet.conf or if we want to replace the cluster name with ClusterConfiguration.ClusterName before saving bootstrap-kubelet.conf.

In other words:

Discovery --> TLSBootstrap kubeconfig with clusterName = Constants --> bootstrap-kubelet.conf

or

Discovery --> TLSBootstrap kubeconfig with clusterName = Constants --> Replace cluster name with ClusterConfiguration.ClusterName --> bootstrap-kubelet.conf

@ereslibre @luxas @timothysc @neolit123 opinions?

neolit123 · 2018-11-07T23:33:18Z

Discovery --> TLSBootstrap kubeconfig with clusterName = Constants --> Replace cluster name with ClusterConfiguration.ClusterName --> bootstrap-kubelet.conf

+1 for this.
no strong opinions, but if we go for the other option it should be DefaultClusterName at least.

ereslibre · 2018-11-08T08:51:05Z

Discovery --> TLSBootstrap kubeconfig with clusterName = Constants --> Replace cluster name with ClusterConfiguration.ClusterName --> bootstrap-kubelet.conf

+1 for this, will update the PR today.

cmd/kubeadm/app/apis/kubeadm/v1alpha3/conversion.go

cmd/kubeadm/app/cmd/join.go

cmd/kubeadm/app/constants/constants.go

fabriziopandini

@ereslibre
This version introduced a double second discovery, that is something we should avoid.
Could you kindly check if the approach I'm suggesting in comments is feasible for you?

cmd/kubeadm/app/apis/kubeadm/v1alpha3/defaults.go

fabriziopandini · 2018-11-08T17:53:14Z

cmd/kubeadm/app/cmd/join.go

@@ -619,6 +619,12 @@ func fetchInitConfigurationFromJoinConfiguration(cfg *kubeadmapi.JoinConfigurati
 		return nil, nil, err
 	}

+	// Retrieve the final KubeConfig file with the cluster name discovered after fetching the cluster configuration


I don't think we should do discovery twice.
Instead I think we should change the TLS kubeconfig in place or create a new kubeconfig file from TLS kubeconfig but with a different cluster name (if you need prior art let me know)

This will allow to clean up the Discovery.For chain of calls from the clusterName field

I changed the approach to generate a different one locally. Not resolving the conversation until you agree that is the best way.

ereslibre · 2018-11-08T20:05:17Z

/retest

luxas

LGTM apart from the comment

luxas · 2018-11-08T20:39:48Z

cmd/kubeadm/app/cmd/join.go

@@ -619,6 +619,16 @@ func fetchInitConfigurationFromJoinConfiguration(cfg *kubeadmapi.JoinConfigurati
 		return nil, nil, err
 	}

+	// Create the final KubeConfig file with the cluster name discovered after fetching the cluster configuration
+	clusterinfo := kubeconfigutil.GetClusterFromKubeConfig(tlsBootstrapCfg)
+	tlsBootstrapCfg = kubeconfigutil.CreateWithToken(


This doesn't work, as the client cert returned from TLS bootstrap would be overridden with the tls bootstrap token.
The way to do this is:

Extract clusterinfo (like you do)

Delete the existing cluster key and value in the .Clusters map

Replace the current context's .Cluster value

Add clusterinfo back to the .Clusters map with the new initConfiguration.ClusterName key

@luxas Out of curiosity, in the case you are mentioning, this is where discovery.For is supposed to return? (so len(cfg.Discovery.TLSBootstrapToken) == 0):

kubernetes/cmd/kubeadm/app/discovery/discovery.go

Lines 45 to 47 in 91d6d75

if len(cfg.Discovery.TLSBootstrapToken) == 0 {

return config, nil

}

ereslibre · 2018-11-08T22:36:21Z

PR rebased.

neolit123 · 2018-11-08T23:32:05Z

/retest

timothysc

/approve

k8s-ci-robot · 2018-11-09T00:19:42Z

[APPROVALNOTIFIER] This PR is APPROVED

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

fabriziopandini · 2018-11-09T15:57:09Z

@ereslibre thanks!
/lgtm
cleanup of clean up the Discovery.For chain of calls from the clusterName field can be addressed in another PR.

ereslibre · 2018-11-09T19:37:27Z

/retest

k8s-ci-robot requested review from chuckha and timothysc November 7, 2018 19:48

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 7, 2018

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 7, 2018

timothysc reviewed Nov 7, 2018

View reviewed changes

k8s-ci-robot assigned fabriziopandini, luxas and timothysc Nov 7, 2018

neolit123 reviewed Nov 7, 2018

View reviewed changes

cmd/kubeadm/app/discovery/discovery.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 7, 2018

fabriziopandini reviewed Nov 7, 2018

View reviewed changes

neolit123 reviewed Nov 8, 2018

View reviewed changes

cmd/kubeadm/app/apis/kubeadm/v1alpha3/conversion.go Show resolved Hide resolved

cmd/kubeadm/app/cmd/join.go Outdated Show resolved Hide resolved

cmd/kubeadm/app/constants/constants.go Outdated Show resolved Hide resolved

ereslibre mentioned this pull request Nov 8, 2018

kubeadm: Remove feature gates from JoinConfiguration #70755

Merged

fabriziopandini suggested changes Nov 8, 2018

View reviewed changes

luxas reviewed Nov 8, 2018

View reviewed changes

kubeadm: Remove cluster name from JoinConfiguration

47b4e04

timothysc approved these changes Nov 9, 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 Nov 9, 2018

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

k8s-ci-robot merged commit c3d05c8 into kubernetes:master Nov 9, 2018

ereslibre deleted the remove-cluster-name-from-join-configuration branch November 9, 2018 20:47

ereslibre mentioned this pull request Jan 27, 2019

REQUEST: New membership for ereslibre kubernetes/org#421

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Remove cluster name from JoinConfiguration #70757

kubeadm: Remove cluster name from JoinConfiguration #70757

ereslibre commented Nov 7, 2018

timothysc commented Nov 7, 2018

neolit123 commented Nov 7, 2018

timothysc left a comment

timothysc commented Nov 7, 2018

ereslibre commented Nov 7, 2018

fabriziopandini left a comment

neolit123 commented Nov 7, 2018 •

edited

ereslibre commented Nov 8, 2018

fabriziopandini left a comment

fabriziopandini Nov 8, 2018

ereslibre Nov 8, 2018

ereslibre commented Nov 8, 2018

luxas left a comment

luxas Nov 8, 2018

ereslibre Nov 8, 2018

ereslibre commented Nov 8, 2018

neolit123 commented Nov 8, 2018

timothysc left a comment

k8s-ci-robot commented Nov 9, 2018

fabriziopandini commented Nov 9, 2018

ereslibre commented Nov 9, 2018

	if len(cfg.Discovery.TLSBootstrapToken) == 0 {
	return config, nil
	}

kubeadm: Remove cluster name from JoinConfiguration #70757

kubeadm: Remove cluster name from JoinConfiguration #70757

Conversation

ereslibre commented Nov 7, 2018

timothysc commented Nov 7, 2018

neolit123 commented Nov 7, 2018

timothysc left a comment

Choose a reason for hiding this comment

timothysc commented Nov 7, 2018

ereslibre commented Nov 7, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

neolit123 commented Nov 7, 2018 • edited

ereslibre commented Nov 8, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Nov 8, 2018

Choose a reason for hiding this comment

ereslibre Nov 8, 2018

Choose a reason for hiding this comment

ereslibre commented Nov 8, 2018

luxas left a comment

Choose a reason for hiding this comment

luxas Nov 8, 2018

Choose a reason for hiding this comment

ereslibre Nov 8, 2018

Choose a reason for hiding this comment

ereslibre commented Nov 8, 2018

neolit123 commented Nov 8, 2018

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 9, 2018

fabriziopandini commented Nov 9, 2018

ereslibre commented Nov 9, 2018

neolit123 commented Nov 7, 2018 •

edited