Merge pull request #71173 from smarterclayton/revert_bootstrap

Revert "Make bootstrap client cert loading part of rotation"

Kubernetes-commit: 8996fc1639f20c17b4b321c184a734f8894b736a

util/certificate/certificate_manager.go

@@ -48,10 +48,11 @@ var certificateWaitBackoff = wait.Backoff{Duration: 30 * time.Second, Steps: 4,
 // manager. In the background it communicates with the API server to get new
 // certificates for certificates about to expire.
 type Manager interface {
+	// CertificateSigningRequestClient sets the client interface that is used for
+	// signing new certificates generated as part of rotation.
+	SetCertificateSigningRequestClient(certificatesclient.CertificateSigningRequestInterface) error
 	// Start the API server status sync loop.
 	Start()
-	// Stop the cert manager loop.
-	Stop()
 	// Current returns the currently selected certificate from the
 	// certificate manager, as well as the associated certificate and key data
 	// in PEM format.
@@ -66,11 +67,11 @@ type Manager interface {
 
 // Config is the set of configuration parameters available for a new Manager.
 type Config struct {
-	// ClientFn will be used to create a client for
+	// CertificateSigningRequestClient will be used for signing new certificate
-	// signing new certificate requests generated when a key rotation occurs.
+	// requests generated when a key rotation occurs. It must be set either at
-	// It must be set at initialization. The function will never be invoked
+	// initialization or by using CertificateSigningRequestClient before
-	// in parallel. It is passed the current client certificate if one exists.
+	// Manager.Start() is called.
-	ClientFn CSRClientFunc
+	CertificateSigningRequestClient certificatesclient.CertificateSigningRequestInterface
 	// Template is the CertificateRequest that will be used as a template for
 	// generating certificate signing requests for all new keys generated as
 	// part of rotation. It follows the same rules as the template parameter of
@@ -140,34 +141,21 @@ type Gauge interface {
 // NoCertKeyError indicates there is no cert/key currently available.
 type NoCertKeyError string
 
-// CSRClientFunc returns a new client for requesting CSRs. It passes the
-// current certificate if one is available and valid.
-type CSRClientFunc func(current *tls.Certificate) (certificatesclient.CertificateSigningRequestInterface, error)
-
 func (e *NoCertKeyError) Error() string { return string(*e) }
 
 type manager struct {
-	getTemplate     func() *x509.CertificateRequest
+	certSigningRequestClient certificatesclient.CertificateSigningRequestInterface
-	lastRequestLock sync.Mutex
+	getTemplate              func() *x509.CertificateRequest
-	lastRequest     *x509.CertificateRequest
+	lastRequestLock          sync.Mutex
-	dynamicTemplate bool
+	lastRequest              *x509.CertificateRequest
-	usages          []certificates.KeyUsage
+	dynamicTemplate          bool
-	forceRotation   bool
+	usages                   []certificates.KeyUsage
-
+	certStore                Store
-	certStore Store
+	certAccessLock           sync.RWMutex
-
+	cert                     *tls.Certificate
-	certificateExpiration Gauge
+	forceRotation            bool
-
+	certificateExpiration    Gauge
-	// the following variables must only be accessed under certAccessLock
+	serverHealth             bool
-	certAccessLock sync.RWMutex
-	cert           *tls.Certificate
-	serverHealth   bool
-
-	// the clientFn must only be accessed under the clientAccessLock
-	clientAccessLock sync.Mutex
-	clientFn         CSRClientFunc
-	stopCh           chan struct{}
-	stopped          bool
 }
 
 // NewManager returns a new certificate manager. A certificate manager is
@@ -188,15 +176,14 @@ func NewManager(config *Config) (Manager, error) {
 	}
 
 	m := manager{
-		stopCh:                make(chan struct{}),
+		certSigningRequestClient: config.CertificateSigningRequestClient,
-		clientFn:              config.ClientFn,
+		getTemplate:              getTemplate,
-		getTemplate:           getTemplate,
+		dynamicTemplate:          config.GetTemplate != nil,
-		dynamicTemplate:       config.GetTemplate != nil,
+		usages:                   config.Usages,
-		usages:                config.Usages,
+		certStore:                config.CertificateStore,
-		certStore:             config.CertificateStore,
+		cert:                     cert,
-		cert:                  cert,
+		forceRotation:            forceRotation,
-		forceRotation:         forceRotation,
+		certificateExpiration:    config.CertificateExpiration,
-		certificateExpiration: config.CertificateExpiration,
 	}
 
 	return &m, nil
@@ -205,14 +192,10 @@ func NewManager(config *Config) (Manager, error) {
 // Current returns the currently selected certificate from the certificate
 // manager. This can be nil if the manager was initialized without a
 // certificate and has not yet received one from the
-// CertificateSigningRequestClient, or if the current cert has expired.
+// CertificateSigningRequestClient.
 func (m *manager) Current() *tls.Certificate {
 	m.certAccessLock.RLock()
 	defer m.certAccessLock.RUnlock()
-	if m.cert != nil && m.cert.Leaf != nil && time.Now().After(m.cert.Leaf.NotAfter) {
-		klog.V(2).Infof("Current certificate is expired.")
-		return nil
-	}
 	return m.cert
 }
 
@@ -224,15 +207,18 @@ func (m *manager) ServerHealthy() bool {
 	return m.serverHealth
 }
 
-// Stop terminates the manager.
+// SetCertificateSigningRequestClient sets the client interface that is used
-func (m *manager) Stop() {
+// for signing new certificates generated as part of rotation. It must be
-	m.clientAccessLock.Lock()
+// called before Start() and can not be used to change the
-	defer m.clientAccessLock.Unlock()
+// CertificateSigningRequestClient that has already been set. This method is to
-	if m.stopped {
+// support the one specific scenario where the CertificateSigningRequestClient
-		return
+// uses the CertificateManager.
+func (m *manager) SetCertificateSigningRequestClient(certSigningRequestClient certificatesclient.CertificateSigningRequestInterface) error {
+	if m.certSigningRequestClient == nil {
+		m.certSigningRequestClient = certSigningRequestClient
+		return nil
 	}
-	close(m.stopCh)
+	return fmt.Errorf("property CertificateSigningRequestClient is already set")
-	m.stopped = true
 }
 
 // Start will start the background work of rotating the certificates.
@@ -240,7 +226,7 @@ func (m *manager) Start() {
 	// Certificate rotation depends on access to the API server certificate
 	// signing API, so don't start the certificate manager if we don't have a
 	// client.
-	if m.clientFn == nil {
+	if m.certSigningRequestClient == nil {
 		klog.V(2).Infof("Certificate rotation is not enabled, no connection to the apiserver.")
 		return
 	}
@@ -248,7 +234,7 @@ func (m *manager) Start() {
 	klog.V(2).Infof("Certificate rotation is enabled.")
 
 	templateChanged := make(chan struct{})
-	go wait.Until(func() {
+	go wait.Forever(func() {
 		deadline := m.nextRotationDeadline()
 		if sleepInterval := deadline.Sub(time.Now()); sleepInterval > 0 {
 			klog.V(2).Infof("Waiting %v for next certificate rotation", sleepInterval)
@@ -283,17 +269,17 @@ func (m *manager) Start() {
 			utilruntime.HandleError(fmt.Errorf("Reached backoff limit, still unable to rotate certs: %v", err))
 			wait.PollInfinite(32*time.Second, m.rotateCerts)
 		}
-	}, time.Second, m.stopCh)
+	}, time.Second)
 
 	if m.dynamicTemplate {
-		go wait.Until(func() {
+		go wait.Forever(func() {
 			// check if the current template matches what we last requested
 			if !m.certSatisfiesTemplate() && !reflect.DeepEqual(m.getLastRequest(), m.getTemplate()) {
 				// if the template is different, queue up an interrupt of the rotation deadline loop.
 				// if we've requested a CSR that matches the new template by the time the interrupt is handled, the interrupt is disregarded.
 				templateChanged <- struct{}{}
 			}
-		}, time.Second, m.stopCh)
+		}, time.Second)
 	}
 }
 
@@ -341,26 +327,11 @@ func getCurrentCertificateOrBootstrap(
 	return &bootstrapCert, true, nil
 }
 
-func (m *manager) getClient() (certificatesclient.CertificateSigningRequestInterface, error) {
-	current := m.Current()
-	m.clientAccessLock.Lock()
-	defer m.clientAccessLock.Unlock()
-	return m.clientFn(current)
-}
-
-// RotateCerts is exposed for testing only and is not a part of the public interface.
-// Returns true if it changed the cert, false otherwise. Error is only returned in
-// exceptional cases.
-func (m *manager) RotateCerts() (bool, error) {
-	return m.rotateCerts()
-}
-
 // rotateCerts attempts to request a client cert from the server, wait a reasonable
 // period of time for it to be signed, and then update the cert on disk. If it cannot
 // retrieve a cert, it will return false. It will only return error in exceptional cases.
 // This method also keeps track of "server health" by interpreting the responses it gets
 // from the server on the various calls it makes.
-// TODO: return errors, have callers handle and log them correctly
 func (m *manager) rotateCerts() (bool, error) {
 	klog.V(2).Infof("Rotating certificates")
 
@@ -370,16 +341,9 @@ func (m *manager) rotateCerts() (bool, error) {
 		return false, nil
 	}
 
-	// request the client each time
-	client, err := m.getClient()
-	if err != nil {
-		utilruntime.HandleError(fmt.Errorf("Unable to load a client to request certificates: %v", err))
-		return false, nil
-	}
-
 	// Call the Certificate Signing Request API to get a certificate for the
 	// new private key.
-	req, err := csr.RequestCertificate(client, csrPEM, "", m.usages, privateKey)
+	req, err := csr.RequestCertificate(m.certSigningRequestClient, csrPEM, "", m.usages, privateKey)
 	if err != nil {
 		utilruntime.HandleError(fmt.Errorf("Failed while requesting a signed certificate from the master: %v", err))
 		return false, m.updateServerError(err)
@@ -395,7 +359,7 @@ func (m *manager) rotateCerts() (bool, error) {
 	var crtPEM []byte
 	watchDuration := time.Minute
 	if err := wait.ExponentialBackoff(certificateWaitBackoff, func() (bool, error) {
-		data, err := csr.WaitForCertificate(client, req, watchDuration)
+		data, err := csr.WaitForCertificate(m.certSigningRequestClient, req, watchDuration)
 		switch {
 		case err == nil:
 			crtPEM = data

util/certificate/certificate_manager_test.go

@@ -60,23 +60,6 @@ iQIgZX08DA8VfvcA5/Xj1Zjdey9FVY6POLXen6RPiabE97UCICp6eUW7ht+2jjar
 e35EltCRCjoejRHTuN9TC0uCoVipAiAXaJIx/Q47vGwiw6Y8KXsNU6y54gTbOSxX
 54LzHNk/+Q==
 -----END RSA PRIVATE KEY-----`)
-var expiredStoreCertData = newCertificateData(`-----BEGIN CERTIFICATE-----
-MIIBFzCBwgIJALhygXnxXmN1MA0GCSqGSIb3DQEBCwUAMBMxETAPBgNVBAMMCGhv
-c3QtMTIzMB4XDTE4MTEwNDIzNTc1NFoXDTE4MTEwNTIzNTc1NFowEzERMA8GA1UE
-AwwIaG9zdC0xMjMwXDANBgkqhkiG9w0BAQEFAANLADBIAkEAtBMa7NWpv3BVlKTC
-PGO/LEsguKqWHBtKzweMY2CVtAL1rQm913huhxF9w+ai76KQ3MHK5IVnLJjYYA5M
-zP2H5QIDAQABMA0GCSqGSIb3DQEBCwUAA0EAN2DPFUtCzqnidL+5nh+46Sk6dkMI
-T5DD11UuuIjZusKvThsHKVCIsyJ2bDo7cTbI+/nklLRP+FcC2wESFUgXbA==
------END CERTIFICATE-----`, `-----BEGIN RSA PRIVATE KEY-----
-MIIBUwIBADANBgkqhkiG9w0BAQEFAASCAT0wggE5AgEAAkEAtBMa7NWpv3BVlKTC
-PGO/LEsguKqWHBtKzweMY2CVtAL1rQm913huhxF9w+ai76KQ3MHK5IVnLJjYYA5M
-zP2H5QIDAQABAkAS9BfXab3OKpK3bIgNNyp+DQJKrZnTJ4Q+OjsqkpXvNltPJosf
-G8GsiKu/vAt4HGqI3eU77NvRI+mL4MnHRmXBAiEA3qM4FAtKSRBbcJzPxxLEUSwg
-XSCcosCktbkXvpYrS30CIQDPDxgqlwDEJQ0uKuHkZI38/SPWWqfUmkecwlbpXABK
-iQIgZX08DA8VfvcA5/Xj1Zjdey9FVY6POLXen6RPiabE97UCICp6eUW7ht+2jjar
-e35EltCRCjoejRHTuN9TC0uCoVipAiAXaJIx/Q47vGwiw6Y8KXsNU6y54gTbOSxX
-54LzHNk/+Q==
------END RSA PRIVATE KEY-----`)
 var bootstrapCertData = newCertificateData(
 	`-----BEGIN CERTIFICATE-----
 MIICRzCCAfGgAwIBAgIJANXr+UzRFq4TMA0GCSqGSIb3DQEBCwUAMH4xCzAJBgNV
@@ -405,8 +388,8 @@ func TestRotateCertCreateCSRError(t *testing.T) {
 		},
 		getTemplate: func() *x509.CertificateRequest { return &x509.CertificateRequest{} },
 		usages:      []certificates.KeyUsage{},
-		clientFn: func(_ *tls.Certificate) (certificatesclient.CertificateSigningRequestInterface, error) {
+		certSigningRequestClient: fakeClient{
-			return fakeClient{failureType: createError}, nil
+			failureType: createError,
 		},
 	}
 
@@ -428,8 +411,8 @@ func TestRotateCertWaitingForResultError(t *testing.T) {
 		},
 		getTemplate: func() *x509.CertificateRequest { return &x509.CertificateRequest{} },
 		usages:      []certificates.KeyUsage{},
-		clientFn: func(_ *tls.Certificate) (certificatesclient.CertificateSigningRequestInterface, error) {
+		certSigningRequestClient: fakeClient{
-			return fakeClient{failureType: watchError}, nil
+			failureType: watchError,
 		},
 	}
 
@@ -615,14 +598,6 @@ func TestInitializeCertificateSigningRequestClient(t *testing.T) {
 			expectedCertBeforeStart: storeCertData,
 			expectedCertAfterStart:  storeCertData,
 		},
-		{
-			description:             "Current certificate expired, no bootstrap certificate",
-			storeCert:               expiredStoreCertData,
-			bootstrapCert:           nilCertificate,
-			apiCert:                 apiServerCertData,
-			expectedCertBeforeStart: nil,
-			expectedCertAfterStart:  apiServerCertData,
-		},
 	}
 
 	for _, tc := range testCases {
@@ -646,25 +621,19 @@ func TestInitializeCertificateSigningRequestClient(t *testing.T) {
 				CertificateStore:        certificateStore,
 				BootstrapCertificatePEM: tc.bootstrapCert.certificatePEM,
 				BootstrapKeyPEM:         tc.bootstrapCert.keyPEM,
-				ClientFn: func(_ *tls.Certificate) (certificatesclient.CertificateSigningRequestInterface, error) {
-					return &fakeClient{
-						certificatePEM: tc.apiCert.certificatePEM,
-					}, nil
-				},
 			})
 			if err != nil {
 				t.Errorf("Got %v, wanted no error.", err)
 			}
 
 			certificate := certificateManager.Current()
-			if tc.expectedCertBeforeStart == nil {
+			if !certificatesEqual(certificate, tc.expectedCertBeforeStart.certificate) {
-				if certificate != nil {
+				t.Errorf("Got %v, wanted %v", certificateString(certificate), certificateString(tc.expectedCertBeforeStart.certificate))
-					t.Errorf("Expected certificate to be nil, was %s", certificate.Leaf.NotAfter)
+			}
-				}
+			if err := certificateManager.SetCertificateSigningRequestClient(&fakeClient{
-			} else {
+				certificatePEM: tc.apiCert.certificatePEM,
-				if !certificatesEqual(certificate, tc.expectedCertBeforeStart.certificate) {
+			}); err != nil {
-					t.Errorf("Got %v, wanted %v", certificateString(certificate), certificateString(tc.expectedCertBeforeStart.certificate))
+				t.Errorf("Got error %v, expected none.", err)
-				}
 			}
 
 			if m, ok := certificateManager.(*manager); !ok {
@@ -680,12 +649,6 @@ func TestInitializeCertificateSigningRequestClient(t *testing.T) {
 			}
 
 			certificate = certificateManager.Current()
-			if tc.expectedCertAfterStart == nil {
-				if certificate != nil {
-					t.Errorf("Expected certificate to be nil, was %s", certificate.Leaf.NotAfter)
-				}
-				return
-			}
 			if !certificatesEqual(certificate, tc.expectedCertAfterStart.certificate) {
 				t.Errorf("Got %v, wanted %v", certificateString(certificate), certificateString(tc.expectedCertAfterStart.certificate))
 			}
@@ -758,10 +721,8 @@ func TestInitializeOtherRESTClients(t *testing.T) {
 				CertificateStore:        certificateStore,
 				BootstrapCertificatePEM: tc.bootstrapCert.certificatePEM,
 				BootstrapKeyPEM:         tc.bootstrapCert.keyPEM,
-				ClientFn: func(_ *tls.Certificate) (certificatesclient.CertificateSigningRequestInterface, error) {
+				CertificateSigningRequestClient: &fakeClient{
-					return &fakeClient{
+					certificatePEM: tc.apiCert.certificatePEM,
-						certificatePEM: tc.apiCert.certificatePEM,
-					}, nil
 				},
 			})
 			if err != nil {
@@ -912,12 +873,10 @@ func TestServerHealth(t *testing.T) {
 				CertificateStore:        certificateStore,
 				BootstrapCertificatePEM: tc.bootstrapCert.certificatePEM,
 				BootstrapKeyPEM:         tc.bootstrapCert.keyPEM,
-				ClientFn: func(_ *tls.Certificate) (certificatesclient.CertificateSigningRequestInterface, error) {
+				CertificateSigningRequestClient: &fakeClient{
-					return &fakeClient{
+					certificatePEM: tc.apiCert.certificatePEM,
-						certificatePEM: tc.apiCert.certificatePEM,
+					failureType:    tc.failureType,
-						failureType:    tc.failureType,
+					err:            tc.clientErr,
-						err:            tc.clientErr,
-					}, nil
 				},
 			})
 			if err != nil {