kubeadm: Implementing the kubeconfig phase fully #49419

fabriziopandini · 2017-07-21T21:52:40Z

What this PR does / why we need it:
This contains implementation of kubeconfig phases in kubeadm, which is part of the wider effort of implementing phases in kubeadm, previously in alpha stage.

The original proposal for this activity can be found here and related comments.

Kubeadm phase implementation checklist is defined here

Common implementation guidelines and principles for all phases are defined here

This PR implements:

kubeadm phase kubeconfig
- kubeadm phase kubeconfig all
- kubeadm phase kubeconfig admin
- kubeadm phase kubeconfig kubelet
- kubeadm phase kubeconfig scheduler
- kubeadm phase kubeconfig controller-manager
- kubeadm phase kubeconfig user

Which issue this PR fixes:
kubernetes/kubeadm#350

Special notes for your reviewer:

This PR implements the second phases of kubeadm init; implementation of this PR follow the same approach already used for #48196 (cert phases).

Please note that:

the API - phase\kubeconfig.go - is now totally free by any UX concerns, and implements only the core logic for kubeconfig generation.
the UX - cmd\phase\kubeconfig.go - now takes charge of UX commands and kubeadm own's rules for kubeconfig files in /etc/kubernetes folder (e.g. create only if not already exists)
The PR includes also a fix for a regression on a unit test for phase certs introduced by Add node-name flag to init phase #48594 and few minor code changes in phase certs introduced to avoid code duplication between the two phases.

k8s-ci-robot · 2017-07-21T21:52:47Z

Hi @fabriziopandini. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

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.

fabriziopandini · 2017-07-22T14:13:58Z

This PR will fix also kubernetes/kubeadm#350; edited command for easier reference

luxas · 2017-07-24T08:51:03Z

/ok-to-test
/release-note

luxas

On a high level:

Please don't move useful pkg functions like writing a kubeconfig file if not exists. That's useful generally
Enhance the struct that describes the generation
Move over the map[string]buildStruct to cmd and call the generic fn from there

I'd like this PR to touch way less code. It can be done ;)

luxas · 2017-07-24T08:52:56Z

cmd/kubeadm/app/cmd/phases/certs_test.go

I think this will make the tempdir to be deleted immediately after this function exits, which is not what we want

In any case, I'd explicitely call defer os.RemoveAll after the createTmpDir call

luxas · 2017-07-24T08:54:54Z

cmd/kubeadm/app/cmd/phases/kubeconfig.go

what about getKubeConfigSubCommands?

luxas · 2017-07-24T08:56:36Z

cmd/kubeadm/app/cmd/phases/kubeconfig.go

Please add a note: Please note that this should *only* be used for bootstrapping purposes. After your control plane is up, you should request all kubelet credentials from the CSR API

luxas · 2017-07-24T08:57:13Z

cmd/kubeadm/app/cmd/phases/kubeconfig.go

with an alias controller-manager possibly?

luxas · 2017-07-24T08:57:30Z

cmd/kubeadm/app/cmd/phases/kubeconfig.go

nit: the Controller Manager

luxas · 2017-07-26T10:52:54Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

Please make a ClientCertAuth struct with the unique fields there: Organization and CAKey

luxas · 2017-07-26T10:53:37Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

Use structs as pointers instead to detect auth method

luxas · 2017-07-26T10:55:32Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

I'd propose to just move this map to cmd and consume it there.
Also keep most of the code as-is here in this pkg.
The only thing that has to change is that and the BuildConfigProperties struct organization

luxas · 2017-07-26T10:57:00Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

I think making a new function for all special cases is not necessary. We have the pattern, let's express what we want with a struct instead and pass it to a generic function GetKubeConfigBytesFromSpec or similar
Then just move the default generation profiles into cmd

luxas · 2017-07-26T10:59:27Z

cmd/kubeadm/app/cmd/phases/kubeconfig.go

Can you make this a wrapped function so that you can set this as cmdFunc: createDefaultKubeConfigFiles("all|api-server|etc.")?

fabriziopandini · 2017-08-05T14:51:52Z

@luxas, this version includes all your comments - including also what we agreed during the follow up call - except the one about ipv6, because #48288 is not merged yet.
Looking forward for a new round of feedbacks! 😄

luxas

Thank you a bunch @fabriziopandini!
I left a couple of nits here, but none of them block this PR
It's better to merge this now and reiterate on small one-line changes than to hold this and its dependencies up for such trivial things.

I downloaded this and tested it out manually and it works well. Thank you for improving our test coverage as part of this btw ;)

/lgtm

luxas · 2017-08-05T15:08:19Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

-	MakeClientCerts bool
+// clientCertAuth struct holds info required to build a client certificate to provide authentication info in a kubeconfig object
+type clientCertAuth struct {
+	CaKey         *rsa.PrivateKey


luxas · 2017-08-05T15:08:32Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

-// /etc/kubernetes/{admin,kubelet}.conf exist but not certs => certs will be generated and conflict with the kubeconfig files => error
+// kubeConfigSpec struct holds info required to build a KubeConfig object
+type kubeConfigSpec struct {
+	CaCert         *x509.Certificate


nit: CACert

luxas · 2017-08-05T15:10:33Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

-		kubeConfigFilePath := filepath.Join(outDir, filename)
-		err = writeKubeconfigToDiskIfNotExists(kubeConfigFilePath, kubeconfig)
+		// writes the KubeConfig to disk if it not exists
+		err = createKubeConfigFileIfNotExists(outDir, kubeConfigFileName, config)


nit: inline this in the if clause: if err := foo(); err != nil {. Reduces scope, which is good

luxas · 2017-08-05T15:11:08Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

+		kubeadmconstants.KubeletKubeConfigFileName: {
+			CaCert:     caCert,
+			APIServer:  cfg.GetMasterEndpoint(),
+			ClientName: fmt.Sprintf("system:node:%s", cfg.NodeName),


I'd love to find a helper func in an auth pkg for this some day.

luxas · 2017-08-05T15:11:51Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

-		}
-		return kubeconfigutil.CreateWithCerts(
-			config.APIServer,
+	// If this kubeconfing should use token


nit: kubeconfig

luxas · 2017-08-05T19:39:30Z

cmd/kubeadm/app/cmd/phases/kubeconfig.go

+		}
+		cmd.Flags().StringVar(&cfg.CertificatesDir, "cert-dir", cfg.CertificatesDir, "The path where to save and store the certificates")
+		cmd.Flags().StringVar(&cfg.API.AdvertiseAddress, "apiserver-advertise-address", cfg.API.AdvertiseAddress, "The IP address the API Server will advertise it's listening on. 0.0.0.0 means the default network interface's address.")
+		cmd.Flags().Int32Var(&cfg.API.BindPort, "apiserver-bind-port", cfg.API.BindPort, "Port for the API Server to bind to")


nit: apiserver-port
nit: The port the API server is accessible on

luxas · 2017-08-05T19:40:35Z

cmd/kubeadm/app/cmd/phases/kubeconfig.go

+		cmd.Flags().StringVar(&cfg.API.AdvertiseAddress, "apiserver-advertise-address", cfg.API.AdvertiseAddress, "The IP address the API Server will advertise it's listening on. 0.0.0.0 means the default network interface's address.")
+		cmd.Flags().Int32Var(&cfg.API.BindPort, "apiserver-bind-port", cfg.API.BindPort, "Port for the API Server to bind to")
+		if properties.use == "all" || properties.use == "kubelet" {
+			cmd.Flags().StringVar(&cfg.NodeName, "node-name", cfg.NodeName, `Specify the node name`)


nit: The node name that the kubelet client cert should use

luxas · 2017-08-05T19:41:30Z

cmd/kubeadm/app/cmd/phases/kubeconfig.go

+			cmd.Flags().StringVar(&cfg.NodeName, "node-name", cfg.NodeName, `Specify the node name`)
+		}
+		if properties.use == "user" {
+			cmd.Flags().StringVar(&token, "token", token, "The path to the directory where the certificates are.")


nit: The token that should be used as the authentication mechanism for this kubeconfig

luxas · 2017-08-05T19:42:20Z

cmd/kubeadm/app/cmd/phases/kubeconfig.go

+		}
+		if properties.use == "user" {
+			cmd.Flags().StringVar(&token, "token", token, "The path to the directory where the certificates are.")
+			cmd.Flags().StringVar(&clientName, "client-name", clientName, "The name of the client for which the KubeConfig file will be generated.")


nit: The name of the KubeConfig user that will be created. Will also be used as the CN if client certs are created.

luxas · 2017-08-05T19:42:49Z

cmd/kubeadm/app/cmd/phases/kubeconfig_test.go

+	kubeconfigtestutil "k8s.io/kubernetes/cmd/kubeadm/test/kubeconfig"
+)
+
+func TestKubeConfigCSubCommandsHasFlags(t *testing.T) {


nit: TestKubeConfigSubCommandsHasFlags?

luxas · 2017-08-05T19:47:52Z

/retest

k8s-github-robot · 2017-08-05T19:48:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, luxas

Associated issue: 267

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

luxas · 2017-08-05T22:15:22Z

/retest

k8s-github-robot · 2017-08-05T22:45:27Z

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

k8s-github-robot · 2017-08-05T23:34:42Z

Automatic merge from submit-queue

fabriziopandini · 2017-08-06T18:31:18Z

Thanks @luxas! 🎉 As agreed I move to phase "controlplane" and then I will come back on this comments and on refactoring phase cert!

@fabriziopandini

…plane2 Automatic merge from submit-queue kubeadm: Implementing the controlplane phase **What this PR does / why we need it:** This contains implementation of controlplane phases in kubeadm, which is part of the wider effort of implementing phases in kubeadm, previously in alpha stage. The original proposal for this activity can be found [here](https://github.com/kubernetes/kubeadm/pull/156/files) and related comments. Kubeadm phase implementation checklist is defined [here](kubernetes/kubeadm#267) Common implementation guidelines and principles for all phases are defined [here](https://docs.google.com/document/d/1VQMyFIVMfRGQPP3oCUpfjiWtOr3pLxp4g7cP-hXQFXc/edit?usp=sharing) This PR implements: - [x] kubeadm phase controlplane: wip by @fabriziopandini - [x] kubeadm phase controlplane all - [x] kubeadm phase controlplane etcd - [x] kubeadm phase controlplane apiserver - [x] kubeadm phase controlplane scheduler - [x] kubeadm phase controlplane controller-manager **Which issue this PR fixes:** kubernetes/kubeadm#349 **Special notes for your reviewer:** This PR implements the same approach of #49419, thus minimising rework/impacts on existing codebase.

@luxas

…leanups Automatic merge from submit-queue (batch tested with PRs 50692, 50727) kubeadm: Small cleanups from the phases refactoring **What this PR does / why we need it**: Small cleanups on kubeadm phases **Which issue this PR fixes**: fixes pending comments in [#49419](#49419) fixes [#376](kubernetes/kubeadm#376) **Special notes for your reviewer**: cc @luxas

@luxas

Automatic merge from submit-queue (batch tested with PRs 50904, 50691) Make Kubeadm phase certs codebase consistent with other phases **What this PR does / why we need it:** This PR is a refactoring of [#48196](#48196), that makes this part of kubeadm more consistent with other parts of kubeadm recently changed, e.g. [controlplane & etcd phase #50302](#50302) and [kubeconfig phase #49419](#49419) **Which issue this PR fixes:** none **Special notes for your reviewer:** cc @luxas

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 21, 2017

k8s-github-robot assigned lukemarsden and timothysc Jul 21, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Jul 21, 2017

fabriziopandini mentioned this pull request Jul 21, 2017

Kubeadm phase implementation checklist kubernetes/kubeadm#267

Closed

38 tasks

luxas assigned luxas and unassigned lukemarsden Jul 24, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note-label-needed labels Jul 24, 2017

timothysc added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Jul 24, 2017

timothysc added this to the v1.8 milestone Jul 24, 2017

timothysc added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jul 24, 2017

luxas reviewed Jul 26, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2017

fully implement kubeadm-phase-kubeconfig

f9f91bf

fabriziopandini force-pushed the kubeadm-phase-kubeconfig2 branch from cf0bbdb to f9f91bf Compare August 5, 2017 14:45

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2017

luxas approved these changes Aug 5, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2017

k8s-github-robot merged commit 48415c0 into kubernetes:master Aug 5, 2017

fabriziopandini deleted the kubeadm-phase-kubeconfig2 branch August 6, 2017 20:17

fabriziopandini mentioned this pull request Aug 8, 2017

kubeadm: Implementing the controlplane phase #50302

Merged

6 tasks

This was referenced Aug 15, 2017

Make Kubeadm phase certs codebase consistent with other phases #50691

Merged

kubeadm: Small cleanups from the phases refactoring #50692

Merged

luxas mentioned this pull request Aug 18, 2017

Make the kubeconfig phase invokable from the kubeadm phase CLI kubernetes/kubeadm#350

Closed

carlory mentioned this pull request Apr 27, 2025

kubeadm: use testing.TempDir and delete helper function #131489

Merged

kubeadm: Implementing the kubeconfig phase fully #49419

kubeadm: Implementing the kubeconfig phase fully #49419

Uh oh!

Conversation

fabriziopandini commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 21, 2017

Uh oh!

fabriziopandini commented Jul 22, 2017

Uh oh!

luxas commented Jul 24, 2017

Uh oh!

luxas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabriziopandini commented Aug 5, 2017

Uh oh!

luxas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luxas commented Aug 5, 2017

Uh oh!

k8s-github-robot commented Aug 5, 2017

Uh oh!

luxas commented Aug 5, 2017

Uh oh!

k8s-github-robot commented Aug 5, 2017

Uh oh!

k8s-github-robot commented Aug 5, 2017

Uh oh!

fabriziopandini commented Aug 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

fabriziopandini commented Jul 21, 2017 •

edited

Loading