From e34351f7159ec1da0722d7cbde59fdcb61bee01f Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Fri, 20 Jan 2017 11:42:44 -0800 Subject: [PATCH] refactor approver and signer interfaces to be consisten w.r.t. apiserver interaction This makes it so that only the controller loop talks to the API server directly. The signatures for Sign and Approve also become more consistent, while allowing the Signer to report conditions (which it wasn't able to do before). --- .../app/certificates.go | 2 +- pkg/controller/certificates/BUILD | 1 - .../certificates/certificate_controller.go | 24 +++++++++++++------ pkg/controller/certificates/cfssl_signer.go | 10 ++++++-- .../certificates/cfssl_signer_test.go | 3 ++- pkg/controller/certificates/groupapprove.go | 7 ++---- 6 files changed, 30 insertions(+), 17 deletions(-) diff --git a/cmd/kube-controller-manager/app/certificates.go b/cmd/kube-controller-manager/app/certificates.go index 760d1efd91b..58d7f265304 100644 --- a/cmd/kube-controller-manager/app/certificates.go +++ b/cmd/kube-controller-manager/app/certificates.go @@ -38,7 +38,7 @@ func startCSRController(ctx ControllerContext) (bool, error) { resyncPeriod, ctx.Options.ClusterSigningCertFile, ctx.Options.ClusterSigningKeyFile, - certcontroller.NewGroupApprover(c.Certificates().CertificateSigningRequests(), ctx.Options.ApproveAllKubeletCSRsForGroup), + certcontroller.NewGroupApprover(ctx.Options.ApproveAllKubeletCSRsForGroup), ) if err != nil { // TODO this is failing consistently in test-cmd and local-up-cluster.sh. Fix them and make it consistent with all others which diff --git a/pkg/controller/certificates/BUILD b/pkg/controller/certificates/BUILD index 54df3e450ca..6113f7815b8 100644 --- a/pkg/controller/certificates/BUILD +++ b/pkg/controller/certificates/BUILD @@ -21,7 +21,6 @@ go_library( deps = [ "//pkg/apis/certificates/v1beta1:go_default_library", "//pkg/client/clientset_generated/clientset:go_default_library", - "//pkg/client/clientset_generated/clientset/typed/certificates/v1beta1:go_default_library", "//pkg/client/legacylisters:go_default_library", "//pkg/controller:go_default_library", "//vendor:github.com/cloudflare/cfssl/config", diff --git a/pkg/controller/certificates/certificate_controller.go b/pkg/controller/certificates/certificate_controller.go index 94de94f37b4..f30e3f98faf 100644 --- a/pkg/controller/certificates/certificate_controller.go +++ b/pkg/controller/certificates/certificate_controller.go @@ -37,12 +37,16 @@ import ( "github.com/golang/glog" ) +// err returned from these interfaces should indicate utter failure that +// should be retried. "Buisness logic" errors should be indicated by adding +// a condition to the CSRs status, not by returning an error. + type AutoApprover interface { AutoApprove(csr *certificates.CertificateSigningRequest) (*certificates.CertificateSigningRequest, error) } type Signer interface { - Sign(csr *certificates.CertificateSigningRequest) ([]byte, error) + Sign(csr *certificates.CertificateSigningRequest) (*certificates.CertificateSigningRequest, error) } type CertificateController struct { @@ -197,6 +201,10 @@ func (cc *CertificateController) maybeSignCertificate(key string) error { if err != nil { return fmt.Errorf("error auto approving csr: %v", err) } + _, err = cc.kubeClient.Certificates().CertificateSigningRequests().UpdateApproval(csr) + if err != nil { + return fmt.Errorf("error updating approval for csr: %v", err) + } } // At this point, the controller needs to: @@ -204,14 +212,16 @@ func (cc *CertificateController) maybeSignCertificate(key string) error { // 2. Generate a signed certificate // 3. Update the Status subresource - if csr.Status.Certificate == nil && IsCertificateRequestApproved(csr) { - certBytes, err := cc.signer.Sign(csr) + if cc.signer != nil && csr.Status.Certificate == nil && IsCertificateRequestApproved(csr) { + csr, err := cc.signer.Sign(csr) if err != nil { - return err + return fmt.Errorf("error auto signing csr: %v", err) + } + _, err = cc.kubeClient.Certificates().CertificateSigningRequests().UpdateStatus(csr) + if err != nil { + return fmt.Errorf("error updating signature for csr: %v", err) } - csr.Status.Certificate = certBytes } - _, err = cc.kubeClient.Certificates().CertificateSigningRequests().UpdateStatus(csr) - return err + return nil } diff --git a/pkg/controller/certificates/cfssl_signer.go b/pkg/controller/certificates/cfssl_signer.go index 1670a4cb9ac..35cf4f9d23f 100644 --- a/pkg/controller/certificates/cfssl_signer.go +++ b/pkg/controller/certificates/cfssl_signer.go @@ -77,7 +77,7 @@ func NewCFSSLSigner(caFile, caKeyFile string) (*CFSSLSigner, error) { }, nil } -func (cs *CFSSLSigner) Sign(csr *certificates.CertificateSigningRequest) ([]byte, error) { +func (cs *CFSSLSigner) Sign(csr *certificates.CertificateSigningRequest) (*certificates.CertificateSigningRequest, error) { var usages []string for _, usage := range csr.Spec.Usages { usages = append(usages, string(usage)) @@ -93,7 +93,13 @@ func (cs *CFSSLSigner) Sign(csr *certificates.CertificateSigningRequest) ([]byte if err != nil { return nil, err } - return s.Sign(signer.SignRequest{ + + csr.Status.Certificate, err = s.Sign(signer.SignRequest{ Request: string(csr.Spec.Request), }) + if err != nil { + return nil, err + } + + return csr, nil } diff --git a/pkg/controller/certificates/cfssl_signer_test.go b/pkg/controller/certificates/cfssl_signer_test.go index 81d95b9edaf..56300f69e0f 100644 --- a/pkg/controller/certificates/cfssl_signer_test.go +++ b/pkg/controller/certificates/cfssl_signer_test.go @@ -49,10 +49,11 @@ func TestSigner(t *testing.T) { }, } - certData, err := s.Sign(csr) + csr, err = s.Sign(csr) if err != nil { t.Fatalf("failed to sign CSR: %v", err) } + certData := csr.Status.Certificate if len(certData) == 0 { t.Fatalf("expected a certificate after signing") } diff --git a/pkg/controller/certificates/groupapprove.go b/pkg/controller/certificates/groupapprove.go index b2a04585c6b..2dabb5a04d4 100644 --- a/pkg/controller/certificates/groupapprove.go +++ b/pkg/controller/certificates/groupapprove.go @@ -23,19 +23,16 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" certificates "k8s.io/kubernetes/pkg/apis/certificates/v1beta1" - clientcertificates "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/certificates/v1beta1" ) // groupApprover implements AutoApprover for signing Kubelet certificates. type groupApprover struct { - client clientcertificates.CertificateSigningRequestInterface approveAllKubeletCSRsForGroup string } // NewGroupApprover creates an approver that accepts any CSR requests where the subject group contains approveAllKubeletCSRsForGroup. -func NewGroupApprover(client clientcertificates.CertificateSigningRequestInterface, approveAllKubeletCSRsForGroup string) AutoApprover { +func NewGroupApprover(approveAllKubeletCSRsForGroup string) AutoApprover { return &groupApprover{ - client: client, approveAllKubeletCSRsForGroup: approveAllKubeletCSRsForGroup, } } @@ -84,7 +81,7 @@ func (cc *groupApprover) AutoApprove(csr *certificates.CertificateSigningRequest Reason: "AutoApproved", Message: "Auto approving of all kubelet CSRs is enabled on the controller manager", }) - return cc.client.UpdateApproval(csr) + return csr, nil } var kubeletClientUsages = []certificates.KeyUsage{