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

Restore bootstrap in the background with fix to preserve kubeadm behavior #71174

Merged
merged 2 commits into from Dec 3, 2018

Conversation

@smarterclayton
Copy link
Contributor

smarterclayton commented Nov 17, 2018

This restores the revert of #69890 due to a failure in kubeadm.

fixes #68686

kubeadm depends on being able to give the kubelet a high powered cert as --kubeconfig on the master and expects the kubelet to rotate that cert. For now, preserve the behavior, even though this doesn't improve security on the masters (being able to read root owned files off the master is not a critical part of our threat model). Add comments and a test to ensure this is clearly required.

When a kubelet is using --bootstrap-kubeconfig and certificate rotation, it no longer waits for bootstrap to succeed before launching static pods.
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Nov 17, 2018

/test pull-kubernetes-e2e-kubeadm-gce

@smarterclayton smarterclayton force-pushed the smarterclayton:debug_kubeadm branch from 0890f02 to 50603d2 Nov 17, 2018

@smarterclayton smarterclayton force-pushed the smarterclayton:debug_kubeadm branch from 50603d2 to 5c5c0ab Nov 17, 2018

@smarterclayton smarterclayton force-pushed the smarterclayton:debug_kubeadm branch 2 times, most recently from 956b8cd to c0e2418 Nov 17, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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

@smarterclayton smarterclayton changed the title DO NOT MERGE - Add debug output to kubeadm start Restore bootstrap in the background with fix to preserve kubeadm behavior Nov 18, 2018

@neolit123
Copy link
Member

neolit123 left a comment

@smarterclayton
btw pull-kubernetes-e2e-kubeadm-gce — Skipped, not sure if it was actually skipped.
were you able to create a kubeadm setup locally for testing purposes?

// The desired configuration for bootstrapping is to use a bootstrap kubeconfig and to have
// the kubeconfig file be managed by this process. For backwards compatibility with kubeadm,
// which provides a high powered kubeconfig on the master with cert/key data, we must
// bootstrap the cert manager with the contents of the initial client config.

This comment has been minimized.

@neolit123

neolit123 Nov 19, 2018

Member

thanks a lot working on this and the detailed comments!
do you think that kubeadm should move away from the high-powered kubeconfig model?

to me it feels like we are adding a workaround for something that kubeadm is doing.

will cc @luxas here, although not sure if he will have the time to take a look.

This comment has been minimized.

@smarterclayton

smarterclayton Nov 19, 2018

Author Contributor

For back compat we need to continue to support it. I think kubeadm should be able to switch to setting bootstrap-kubeconfig equal to their high powered cert on masters, because all root file access on masters is powerful. Then we could remove the need to support this case.

This comment has been minimized.

@neolit123

neolit123 Nov 19, 2018

Member

we briefly discussed this today with @fabriziopandini and he had a proposal.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 19, 2018

while there are kubeadm related topics for this PR, sig-node / sig-auth should give the final LGTM.

@mikedanese

This comment has been minimized.

Copy link
Member

mikedanese commented Nov 19, 2018

Thanks. Looking...

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Nov 19, 2018

/assign @fabriziopandini @liztio

please review ^

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Nov 19, 2018

You can see all kubeadm runs with https://gubernator.k8s.io/builds/kubernetes-jenkins/pr-logs/pull/71174/pull-kubernetes-e2e-kubeadm-gce/

The job won't trigger until my PR to test-infra merges (to detect changes to bootstrap code). Also, because skip_report: true on the job, you can't see the status from the link (you have to look at the gubernator logs)

/test pull-kubernetes-e2e-kubeadm-gce

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 19, 2018

The job won't trigger until my PR to test-infra merges

pinged sig-testing about it.

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Nov 19, 2018

/test pull-kubernetes-bazel-build

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 19, 2018

/test pull-kubernetes-e2e-kubeadm-gce

kubernetes/test-infra#10177 merged.

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Nov 20, 2018

What's the risk of holding on this till 1.14?

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 20, 2018

This fixes some long-broken self-hosting flows and improves kubelet startup, but is not critical for last minute 1.13 merge. Moving to 1.14 and dropping severity. Will merge when master opens and get a release of soak time.

/milestone v1.14

@jennybuckley

This comment has been minimized.

Copy link
Contributor

jennybuckley commented Nov 26, 2018

/cc @yliaog

@k8s-ci-robot k8s-ci-robot requested a review from yliaog Nov 26, 2018

@luxas

This comment has been minimized.

Copy link
Member

luxas commented Nov 27, 2018

Also just wanna point out that in v1.14 we should strive to not need the "workaround" for how kubeadm sets this up, but instead merge this PR and a kubeadm patch so kubeadm isn't blocked on that a kubelet with no clientcert won't start static pods.

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Nov 30, 2018

We ready to merge this?

@luxas once kubeadm is changed, I can submit the PR that removes the workaround for the old code path (although it's enough of a part of our API that we might need a formal deprecation period)

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 30, 2018

+1 from me

@luxas

luxas approved these changes Dec 3, 2018

Copy link
Member

luxas left a comment

I haven't reviewed this in detail, but I'm happy to proceed with this.
Re-adding the LGTM and putting this in the queue, and we'll follow this up in kubeadm.
/lgtm

@luxas

This comment has been minimized.

Copy link
Member

luxas commented Dec 3, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit dc9261b into kubernetes:master Dec 3, 2018

18 checks passed

cla/linuxfoundation smarterclayton 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-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce 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 Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment