Merge pull request #40236 from mikedanese/cert-csr

Automatic merge from submit-queue (batch tested with PRs 40345, 38183, 40236, 40861, 40900)

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).

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

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",

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
 }

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
 }

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")
 	}

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{