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.

Sign up for GitHub

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

Jump to bottom

Certificate signing controller for TLS bootstrap (alpha) #25764

Merged
merged 3 commits into from
Jul 20, 2016
Merged

Certificate signing controller for TLS bootstrap (alpha) #25764

merged 3 commits into from
Jul 20, 2016
+17,107 −520

Conversation

gtank
Copy link

@gtank gtank commented May 17, 2016 •
edited
Loading

The controller handles generating and signing certificates when a CertificateSigningRequest has the "Approved" condition. Uses cfssl to support a wide set of possible keys and algorithms. Depends on PR #25562, only the last two commits are relevant to this PR.

cc @mikedanese

Analytics

@k8s-bot
Copy link

k8s-bot commented May 17, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments
@k8s-bot
Copy link

k8s-bot commented May 17, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented May 17, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 17, 2016
@krousey krousey assigned mikedanese and unassigned krousey May 18, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2016
@gtank gtank mentioned this pull request May 24, 2016
Merged
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 2, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

1 similar comment
@k8s-bot
Copy link

k8s-bot commented Jun 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@gtank gtank force-pushed the certificates-api-controller-v3 branch from 5c795c2 to e846e0b Compare June 27, 2016 23:20
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/old-docs labels Jun 27, 2016
@mikedanese
Copy link
Member

@gtank please rebase.

@gtank gtank force-pushed the certificates-api-controller-v3 branch from e846e0b to 966eebd Compare June 28, 2016 22:26
@k8s-github-robot k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/new-api labels Jun 28, 2016
@gtank gtank force-pushed the certificates-api-controller-v3 branch 2 times, most recently from ad54760 to 9feff4e Compare June 30, 2016 00:32
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 30, 2016
@gtank gtank mentioned this pull request Jun 30, 2016
Closed
@gtank gtank force-pushed the certificates-api-controller-v3 branch from 9feff4e to 164dfdc Compare July 1, 2016 00:04
@euank
Copy link
Contributor

euank commented Jul 1, 2016

@k8s-bot test this issue: #27693

@gtank
Copy link
Author

gtank commented Jul 1, 2016

@mikedanese found the flake!

mikedanese
mikedanese reviewed Jul 2, 2016
View reviewed changes
cmd/kube-controller-manager/app/controllermanager.go
)
if err != nil {
glog.Errorf("Failed to start certificate controller: %v", err)
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Remove the else. https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

Copy link
Author

Choose a reason for hiding this comment

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

How else should this be done? It can't return or exit here, and shouldn't start the control loop if the signer failed to initialize. I can change the conditionals around to reduce indentation, but the control flow will be the same.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, misread, I withdraw comment

Copy link
Member

Choose a reason for hiding this comment

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

Why is it not glog.Fatalf though?

Copy link
Author

Choose a reason for hiding this comment

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

It isn't fatal to Kubernetes; there are a lot ways you could accomplish this task aside from running the signing controller.

@mikedanese
Copy link
Member

Looks good.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2016
@gtank gtank force-pushed the certificates-api-controller-v3 branch from 164dfdc to 902b9fa Compare July 19, 2016 18:26
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2016
@mikedanese
Copy link
Member

mikedanese commented Jul 19, 2016 •
edited
Loading

LGTM. Todo:

  1. kubelet integration
  2. e2e tests
  3. kubectl integration

cc @kubernetes/sig-cluster-lifecycle

@mikedanese mikedanese added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 19, 2016
@k8s-bot
Copy link

k8s-bot commented Jul 20, 2016

GCE e2e build/test passed for commit 902b9fa.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit a279673 into kubernetes:master Jul 20, 2016
@philips philips mentioned this pull request Jul 22, 2016
Closed
@timothysc timothysc added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Jul 26, 2016
@timothysc
Copy link
Member

/cc @dgoodwin

@MikeSpreitzer MikeSpreitzer mentioned this pull request Sep 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mikedanese mikedanese

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@gtank @k8s-bot @mikedanese @euank @k8s-github-robot @timothysc @justinsb @krousey @googlebot