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

sarapprover: remove self node cert #62471

Merged
merged 1 commit into from Apr 13, 2018

Conversation

@mikedanese
Copy link
Member

mikedanese commented Apr 12, 2018

The functionality to bootstrap node certificates is ready but is blocked by a separable issue discussed in: kubernetes/community#1982. The functionality could be useful for power users who want to write their own approvers if the feature could be promoted to beta. In it's current state this feature doesn't help anybody.

I propose that we remove automated approval of node serving certificates for now and work towards getting the node functionality to beta.

cc @awly @kubernetes/sig-auth-pr-reviews

Remove alpha functionality that allowed the controller manager to approve kubelet server certificates.
@mikedanese

This comment has been minimized.

Copy link
Member Author

mikedanese commented Apr 12, 2018

/retest

@mikedanese mikedanese force-pushed the mikedanese:certs2 branch from 1f3fe3c to d553f3c Apr 12, 2018

@rtripat

This comment has been minimized.

Copy link
Contributor

rtripat commented Apr 12, 2018

@mikedanese Will the kubelet continue to have the ability to request a server certificate as an alpha feature?

@ericchiang

This comment has been minimized.

Copy link
Member

ericchiang commented Apr 12, 2018

+1 from me

@@ -201,28 +192,3 @@ func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.Cert
}
return true
}

This comment has been minimized.

@rtripat

rtripat Apr 12, 2018

Contributor

Unrelated to this change but what do you think about adding debug logging in the recognizer's about the exact condition which led to CSR not being approved?

This comment has been minimized.

@mikedanese

mikedanese Apr 12, 2018

Author Member

Events are better for exposing reasons to end users. Would this be for certs that are recognized but the subject access review is denied?

This comment has been minimized.

@rtripat

rtripat Apr 12, 2018

Contributor

Not so much for the SAR since I think the result of that request will show up in API server logs at appropriate verbosity.

If we can have greater visibility via logs or events into which condition in the recognizer failed to approve the CSR that will be great.

This is more useful when someone generates their own bootstrap token and wants to debug why their CSR isn't being approved. Otherwise, the controller leaves the CSR in pending without any trace in the logs. Example: username of token should match the CommonName in CSR like system:node:$NodeName

This comment has been minimized.

@mikedanese

mikedanese Apr 13, 2018

Author Member

Debug logging might be ok but not all CSRs are expected to be approved by this controller. Unrecognized CSRs are expected under normal circumstances.

@mikedanese

This comment has been minimized.

Copy link
Member Author

mikedanese commented Apr 12, 2018

@mikedanese Will the kubelet continue to have the ability to request a server certificate as an alpha feature?

The motivation for this change is that the kubelet piece can cease to be alpha, and become beta.

func TestRecognizers(t *testing.T) {
goodCases := []func(b *csrBuilder){
func(b *csrBuilder) {
},
}

testRecognizer(t, goodCases, isNodeClientCert, true)
testRecognizer(t, goodCases, isSelfNodeClientCert, true)

This comment has been minimized.

@liggitt

liggitt Apr 13, 2018

Member

did you mean to remove the self client cert cases?

This comment has been minimized.

@mikedanese

mikedanese Apr 13, 2018

Author Member

Good catch

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 13, 2018

question on the removed client cert cases, lgtm otherwise

sarapprover: remove self node cert
The functionality to bootstrap node certificates is ready but is blocked
by a seperable issue discussed in:
kubernetes/community#1982. The functionality
could be useful for power users who want to write their own approvers if
the feature could be promoted to beta. In it's current state this
feature doesn't help anybody.

I propose that we remove automated approval of node serving certificates
for now and work towards getting the node functionality to beta.

@mikedanese mikedanese force-pushed the mikedanese:certs2 branch from d553f3c to 7665f15 Apr 13, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Apr 13, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 13, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mikedanese

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-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Apr 13, 2018

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

@k8s-github-robot k8s-github-robot merged commit eca4d03 into kubernetes:master Apr 13, 2018

15 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation mikedanese 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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@mikedanese mikedanese deleted the mikedanese:certs2 branch Apr 13, 2018

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.