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

Add kubeadm init upload encrypted certs phase #73907

Merged
merged 1 commit into from Feb 20, 2019

Conversation

@yagonobre
Copy link
Member

yagonobre commented Feb 11, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1374
Ref: kubernetes/kubeadm#1373

Special notes for your reviewer:
I'm sending this WIP for a first review.

TODO:

  • Encryption stuffs
  • RBAC
  • Tests
  • Kubeadm init output make output human readable

Does this PR introduce a user-facing change?:

kubeadm: Allow to upload certificates required to join a new control-plane to kubeadm-certs secret using the flag `--experimental-upload-certs` on `init` or upload-certs phase.

/assign @fabriziopandini
/priority important-soon

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Feb 11, 2019

/cc

@k8s-ci-robot k8s-ci-robot requested a review from neolit123 Feb 11, 2019

@yagonobre yagonobre force-pushed the yagonobre:init-upload-certs branch from 53a40c6 to abb2c44 Feb 11, 2019

@fabriziopandini
Copy link
Contributor

fabriziopandini left a comment

@yagonobre thanks for sharing this WIP! it is awesome to see this moving on
I left some comment, but the overall approach looks great!

Show resolved Hide resolved cmd/kubeadm/app/cmd/init.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/init.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/phases/init/uploadcerts.go
Show resolved Hide resolved cmd/kubeadm/app/cmd/phases/init/uploadcerts.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/phases/init/uploadcerts.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/phases/init/uploadcerts.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/phases/uploadcerts/uploadcerts.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/phases/uploadcerts/uploadcerts.go Outdated
@k8s-ci-robot

This comment was marked as resolved.

Copy link
Contributor

k8s-ci-robot commented Feb 12, 2019

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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.

@yagonobre yagonobre force-pushed the yagonobre:init-upload-certs branch from 9db8e3c to f3ea584 Feb 12, 2019

@yagonobre yagonobre force-pushed the yagonobre:init-upload-certs branch from fb27446 to 8563b7f Feb 12, 2019

@fabriziopandini
Copy link
Contributor

fabriziopandini left a comment

@yagonobre thanks for this new version
I left some minor comments, but IMO this is already in really good shape.
I will ping people on slack for getting more eyes on this PR

Show resolved Hide resolved cmd/kubeadm/app/cmd/init.go
Show resolved Hide resolved cmd/kubeadm/app/cmd/util/join.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/init.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/phases/uploadcerts/uploadcerts.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/phases/uploadcerts/uploadcerts.go
Show resolved Hide resolved cmd/kubeadm/app/phases/uploadcerts/uploadcerts.go
Show resolved Hide resolved cmd/kubeadm/app/util/crypto/crypto.go
Show resolved Hide resolved cmd/kubeadm/app/util/crypto/crypto.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/util/crypto/crypto.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/util/crypto/crypto.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/init.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/phases/init/uploadcerts.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/phases/uploadcerts/uploadcerts.go Outdated

@yagonobre yagonobre force-pushed the yagonobre:init-upload-certs branch from 47e2647 to 09b09ea Feb 18, 2019

@fabriziopandini
Copy link
Contributor

fabriziopandini left a comment

@yagonobre
Only 1 minor to properly address jason comment and then ok for me!
/approve

Show resolved Hide resolved cmd/kubeadm/app/cmd/init.go

@yagonobre yagonobre force-pushed the yagonobre:init-upload-certs branch from 05763d4 to 1247be7 Feb 19, 2019

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels Feb 19, 2019

@yagonobre

This comment has been minimized.

Copy link
Member Author

yagonobre commented Feb 19, 2019

Should I add the token reference to rbac rules? So it can be deleted by token gc.

@yagonobre yagonobre force-pushed the yagonobre:init-upload-certs branch from e553160 to a6b114f Feb 19, 2019

testutil "k8s.io/kubernetes/cmd/kubeadm/test"
)

func TestUploadCerts(t *testing.T) {

This comment has been minimized.

@yagonobre

yagonobre Feb 19, 2019

Author Member

I still working on tests.

@yagonobre yagonobre force-pushed the yagonobre:init-upload-certs branch from a6b114f to 79fd5f2 Feb 19, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

fabriziopandini commented Feb 19, 2019

@yagonobre

Should I add the token reference to rbac rules? So it can be deleted by token gc.

IMO it is not necessary. If the RBAC rules outlive the kubeadm-certs secrets no warm can be done

@yagonobre

This comment has been minimized.

Copy link
Member Author

yagonobre commented Feb 20, 2019

/test pull-kubernetes-integration

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

fabriziopandini commented Feb 20, 2019

@yagonobre
Considering this PR is getting big and that @ereslibre is waiting for consolidating the join part, if it is ok for you, I propose to remove WIP, get this merged and follow up with the missing unit tests in a separated PR
wdyt?

@yagonobre yagonobre changed the title [WIP] Add kubeadm init upload encrypted certs phase Add kubeadm init upload encrypted certs phase Feb 20, 2019

@yagonobre

This comment has been minimized.

Copy link
Member Author

yagonobre commented Feb 20, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

fabriziopandini commented Feb 20, 2019

All the comments seem addressed, and as agreed with @yagonobre additional unit test will follow up in a separated PR
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 20, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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 merged commit b4a2b63 into kubernetes:master Feb 20, 2019

17 checks passed

cla/linuxfoundation yagonobre 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-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Context retired without replacement.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped
tide In merge pool.
Details

@yagonobre yagonobre deleted the yagonobre:init-upload-certs branch Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.