Skip to content
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

[kubeadm] Add support for clusterName in config file. #60852

Merged
merged 4 commits into from
Apr 12, 2018

Conversation

karan
Copy link
Contributor

@karan karan commented Mar 6, 2018

What this PR does / why we need it: Adds a --cluster-name arg to kubeadm init.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
See kubernetes-retired/kube-deploy#636
Code inspired by #52470

Special notes for your reviewer:

Release note:

Adds --cluster-name to kubeadm init for specifying the cluster name in kubeconfig.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 6, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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 Mar 6, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2018
@karan
Copy link
Contributor Author

karan commented Mar 6, 2018

/retest

@karan
Copy link
Contributor Author

karan commented Mar 6, 2018

/retest

@karan
Copy link
Contributor Author

karan commented Mar 6, 2018

Tests seem to be failing with merge conflicts - my branch and this PR are up-to-date so not sure why.

$ co master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

karangoel at karangoel-macbookpro in ~/.gvm/pkgsets/go1.9.2/global/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases on master (go1.9.2)
$ git sync
Already up to date.
Everything up-to-date

karangoel at karangoel-macbookpro in ~/.gvm/pkgsets/go1.9.2/global/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases on master (go1.9.2)
$ co -
Switched to branch 'kubeadm-cluster-name'

karangoel at karangoel-macbookpro in ~/.gvm/pkgsets/go1.9.2/global/src/k8s.io/kubernetes/cmd/kubeadm/app/cmd/phases on kubeadm-cluster-name (go1.9.2)
$ git rebase origin/master
Current branch kubeadm-cluster-name is up to date.

@karan
Copy link
Contributor Author

karan commented Mar 6, 2018

/assign @krousey

@krousey
Copy link
Contributor

krousey commented Mar 6, 2018

@karan
I assume you have a remote defined called upstream that points to github.com/kubernetes/kubernetes (git remotes list). Rebase like this:

$ git fetch upstream
$ git checkout kubeadm-cluster-name
$ git rebase upstream/master

@karan
Copy link
Contributor Author

karan commented Mar 6, 2018

Correct, upstream is k8s/k8s.

$ co master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

$ git fetch upstream

$ co -
Switched to branch 'kubeadm-cluster-name'

$ git rebase upstream/master
Current branch kubeadm-cluster-name is up to date.

@krousey
Copy link
Contributor

krousey commented Mar 6, 2018

I see build errors:

      # k8s.io/kubernetes/cmd/kubeadm/app/phases/certs
cmd/kubeadm/app/phases/certs/certs.go:353:63: not enough arguments in call to pkiutil.NewCertificateAuthority
	have ()
	want (string)
# k8s.io/kubernetes/cmd/kubeadm/test/certs
cmd/kubeadm/test/certs/util.go:31:55: not enough arguments in call to pkiutil.NewCertificateAuthority
	have ()

Seems like you didn't update the logic that generates ETCD certs as well.

@karan karan force-pushed the kubeadm-cluster-name branch 4 times, most recently from 14a9d56 to 836696d Compare March 6, 2018 22:32
@karan
Copy link
Contributor Author

karan commented Mar 6, 2018

With the latest changes, this builds locally now.

$ bazel build //cmd/kubeadm/...:all
INFO: Analysed 177 targets (1 packages loaded).
INFO: Found 177 targets...
INFO: Elapsed time: 65.023s, Critical Path: 41.15s
INFO: Build completed successfully, 22 total actions

@karan karan changed the title Add cluster-name to kubeadm init Add --cluster-name to kubeadm init Mar 6, 2018
@karan karan force-pushed the kubeadm-cluster-name branch 2 times, most recently from ba345d7 to ff14e48 Compare March 7, 2018 00:39
@karan
Copy link
Contributor Author

karan commented Mar 7, 2018

/assign luxas

Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use-case / motivation behind this?
I'm very hesitant against adding more flags to kubeadm init, in fact I think we've declared a freeze there. I could maybe see it going into the config file, but I don't want to over-engineer that either. We have phases for a reason, can the use case here be done via that API instead? In other words, is this an advanced use-case enough to justify going phases route vs the init one?

/assign @timothysc

@karan
Copy link
Contributor Author

karan commented Mar 22, 2018

The use case here is to be able to generate a kubeconfig that references the cluster with a name other than "kubernetes" so we can have multiple clusters in the same config file. The motivation is to allow the cluster-api tooling to manage multiple clusters, by referencing names rather than IP addresses.

This PR is not urgent so we can wait until the freeze.

Copy link
Contributor

@xiangpengzhao xiangpengzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a couple of nits. otherwise LGTM. However, @luxas concern sounds reasonable.

@@ -224,6 +227,8 @@ type NodeConfiguration struct {
Token string `json:"token"`
// CRISocket is used to retrieve container runtime info.
CRISocket string `json:"criSocket,omitempty"`
// ClusterName is the name for the cluster in kubeconfig.
ClusterName string `json:"clusterName"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be an optional field, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. made it so

@@ -107,7 +107,7 @@ var (

// NewCmdInit returns "kubeadm init" command.
func NewCmdInit(out io.Writer) *cobra.Command {
cfg := &kubeadmapiext.MasterConfiguration{}
cfg := &kubeadmapiext.MasterConfiguration{ClusterName: kubeadmapiext.DefaultClusterName}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ClusterName: kubeadmapiext.DefaultClusterName needed to be specified here? I think it will be defaulted to the value by legacyscheme.Scheme.Default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Did not know that's what legacyscheme did. Thanks

@@ -101,7 +101,7 @@ var (

// NewCmdJoin returns "kubeadm join" command.
func NewCmdJoin(out io.Writer) *cobra.Command {
cfg := &kubeadmapiext.NodeConfiguration{}
cfg := &kubeadmapiext.NodeConfiguration{ClusterName: kubeadmapiext.DefaultClusterName}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2018
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 11, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2018
@karan karan changed the title Add --cluster-name to kubeadm init [kubeadm] Add support for clusterName in config file. Apr 12, 2018
@karan
Copy link
Contributor Author

karan commented Apr 12, 2018

This is now not using a flag but config file.

Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for scoping this down.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karan, 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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 58178, 62491, 60852). If you want to cherry-pick this change to another branch, please follow the instructions here.

@jorhett
Copy link

jorhett commented Feb 19, 2019

So I am concerned with how this PR played out. @luxas said

I'm very hesitant against adding more flags to kubeadm init, in fact I think we've declared a freeze there. I could maybe see it going into the config file,

As of the current version of Kubernetes 10 months later, there doesn't appear to be any documentation for the config file. The only thing said here: https://kubernetes.io/docs/reference/setup-tools/kubeadm/kubeadm-init/#config-file is

Caution: The config file is still considered beta and may change in future versions.

And worse, the man page which does show the config file option continues to say:

--config string Path to kubeadm config file. WARNING: Usage of a configuration file is experimental.

So it this experimental, or beta? Those would indicate very different levels of support. And either way, there's no clear documentation of what the config file should look like. Is this what it refers to? https://godoc.org/k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1#hdr-Kubeadm_init_configuration_types

While I much prefer config files to command line arguments, and agree this was the right approach, it seems we need to button this up a bit more with consistent documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants