Merge pull request #46154 from jcbsmpsn/improve-certificate-manager-waiting

pkg/kubelet/certificate/certificate_manager.go

@@ -122,6 +122,7 @@ type manager struct {
 	certStore                Store
 	certAccessLock           sync.RWMutex
 	cert                     *tls.Certificate
+	rotationDeadline         time.Time
 	forceRotation            bool
 }
 
@@ -186,16 +187,28 @@ func (m *manager) Start() {
 
 	glog.V(2).Infof("Certificate rotation is enabled.")
 
-	err := m.rotateCerts()
-	if err != nil {
-		glog.Errorf("Could not rotate certificates: %v", err)
+	m.setRotationDeadline()
+
+	// Synchronously request a certificate before entering the background
+	// loop to allow bootstrap scenarios, where the certificate manager
+	// doesn't have a certificate at all yet.
+	if m.shouldRotate() {
+		_, err := m.rotateCerts()
+		if err != nil {
+			glog.Errorf("Could not rotate certificates: %v", err)
+		}
+	}
+	backoff := wait.Backoff{
+		Duration: 2 * time.Second,
+		Factor:   2,
+		Jitter:   0.1,
+		Steps:    7,
 	}
 	go wait.Forever(func() {
-		for range time.Tick(syncPeriod) {
-			err := m.rotateCerts()
-			if err != nil {
-				glog.Errorf("Could not rotate certificates: %v", err)
-			}
+		time.Sleep(m.rotationDeadline.Sub(time.Now()))
+		if err := wait.ExponentialBackoff(backoff, m.rotateCerts); err != nil {
+			glog.Errorf("Reached backoff limit, still unable to rotate certs: %v", err)
+			wait.PollInfinite(128*time.Second, m.rotateCerts)
 		}
 	}, 0)
 }
@@ -242,47 +255,61 @@ func (m *manager) shouldRotate() bool {
 	if m.cert == nil {
 		return true
 	}
-
-	notAfter := m.cert.Leaf.NotAfter
-	totalDuration := float64(notAfter.Sub(m.cert.Leaf.NotBefore))
-
-	// Use some jitter to set the rotation threshold so each node will rotate
-	// at approximately 70-90% of the total lifetime of the certificate.  With
-	// jitter, if a number of nodes are added to a cluster at approximately the
-	// same time (such as cluster creation time), they won't all try to rotate
-	// certificates at the same time for the rest of the life of the cluster.
-	jitteryDuration := wait.Jitter(time.Duration(totalDuration), 0.2) - time.Duration(totalDuration*0.3)
-
-	rotationThreshold := m.cert.Leaf.NotBefore.Add(jitteryDuration)
-	passedThreshold := time.Now().After(rotationThreshold)
-	return m.forceRotation || passedThreshold
-}
-
-func (m *manager) rotateCerts() error {
-	if !m.shouldRotate() {
-		return nil
+	if m.forceRotation {
+		return true
 	}
+	return time.Now().After(m.rotationDeadline)
+}
 
+func (m *manager) rotateCerts() (bool, error) {
 	csrPEM, keyPEM, err := m.generateCSR()
 	if err != nil {
-		return err
+		glog.Errorf("Unable to generate a certificate signing request: %v", err)
+		return false, nil
 	}
 
 	// Call the Certificate Signing Request API to get a certificate for the
 	// new private key.
 	crtPEM, err := requestCertificate(m.certSigningRequestClient, csrPEM, m.usages)
 	if err != nil {
-		return fmt.Errorf("unable to get a new key signed: %v", err)
+		glog.Errorf("Failed while requesting a signed certificate from the master: %v", err)
+		return false, nil
 	}
 
 	cert, err := m.certStore.Update(crtPEM, keyPEM)
 	if err != nil {
-		return fmt.Errorf("unable to store the new cert/key pair: %v", err)
+		glog.Errorf("Unable to store the new cert/key pair: %v", err)
+		return false, nil
 	}
 
 	m.updateCached(cert)
+	m.setRotationDeadline()
 	m.forceRotation = false
-	return nil
+	return true, nil
+}
+
+// setRotationDeadline sets a cached value for the threshold at which the
+// current certificate should be rotated, 80%+/-10% of the expiration of the
+// certificate.
+func (m *manager) setRotationDeadline() {
+	m.certAccessLock.RLock()
+	defer m.certAccessLock.RUnlock()
+	if m.cert == nil {
+		m.rotationDeadline = time.Now()
+		return
+	}
+
+	notAfter := m.cert.Leaf.NotAfter
+	totalDuration := float64(notAfter.Sub(m.cert.Leaf.NotBefore))
+
+	// Use some jitter to set the rotation threshold so each node will rotate
+	// at approximately 70-90% of the total lifetime of the certificate.  With
+	// jitter, if a number of nodes are added to a cluster at approximately the
+	// same time (such as cluster creation time), they won't all try to rotate
+	// certificates at the same time for the rest of the life of the cluster.
+	jitteryDuration := wait.Jitter(time.Duration(totalDuration), 0.2) - time.Duration(totalDuration*0.3)
+
+	m.rotationDeadline = m.cert.Leaf.NotBefore.Add(jitteryDuration)
 }
 
 func (m *manager) updateCached(cert *tls.Certificate) {

pkg/kubelet/certificate/certificate_manager_test.go

@@ -164,13 +164,14 @@ func TestShouldRotate(t *testing.T) {
 			m := manager{
 				cert: &tls.Certificate{
 					Leaf: &x509.Certificate{
-						NotAfter:  test.notAfter,
 						NotBefore: test.notBefore,
+						NotAfter:  test.notAfter,
 					},
 				},
 				template: &x509.CertificateRequest{},
 				usages:   []certificates.KeyUsage{},
 			}
+			m.setRotationDeadline()
 			if m.shouldRotate() != test.shouldRotate {
 				t.Errorf("For time %v, a certificate issued for (%v, %v) should rotate should be %t.",
 					now,
@@ -182,6 +183,53 @@ func TestShouldRotate(t *testing.T) {
 	}
 }
 
+func TestSetRotationDeadline(t *testing.T) {
+	now := time.Now()
+	testCases := []struct {
+		name         string
+		notBefore    time.Time
+		notAfter     time.Time
+		shouldRotate bool
+	}{
+		{"just issued, still good", now.Add(-1 * time.Hour), now.Add(99 * time.Hour), false},
+		{"half way expired, still good", now.Add(-24 * time.Hour), now.Add(24 * time.Hour), false},
+		{"mostly expired, still good", now.Add(-69 * time.Hour), now.Add(31 * time.Hour), false},
+		{"just about expired, should rotate", now.Add(-91 * time.Hour), now.Add(9 * time.Hour), true},
+		{"nearly expired, should rotate", now.Add(-99 * time.Hour), now.Add(1 * time.Hour), true},
+		{"already expired, should rotate", now.Add(-10 * time.Hour), now.Add(-1 * time.Hour), true},
+		{"long duration", now.Add(-6 * 30 * 24 * time.Hour), now.Add(6 * 30 * 24 * time.Hour), true},
+		{"short duration", now.Add(-30 * time.Second), now.Add(30 * time.Second), true},
+	}
+
+	for _, tc := range testCases {
+		for i := 0; i < 1000; i++ {
+			t.Run(tc.name, func(t *testing.T) {
+				m := manager{
+					cert: &tls.Certificate{
+						Leaf: &x509.Certificate{
+							NotBefore: tc.notBefore,
+							NotAfter:  tc.notAfter,
+						},
+					},
+					template: &x509.CertificateRequest{},
+					usages:   []certificates.KeyUsage{},
+				}
+				m.setRotationDeadline()
+				lowerBound := tc.notBefore.Add(time.Duration(float64(tc.notAfter.Sub(tc.notBefore)) * 0.7))
+				upperBound := tc.notBefore.Add(time.Duration(float64(tc.notAfter.Sub(tc.notBefore)) * 0.9))
+				if m.rotationDeadline.Before(lowerBound) || m.rotationDeadline.After(upperBound) {
+					t.Errorf("For notBefore %v, notAfter %v, the rotationDeadline %v should be between %v and %v.",
+						tc.notBefore,
+						tc.notAfter,
+						m.rotationDeadline,
+						lowerBound,
+						upperBound)
+				}
+			})
+		}
+	}
+}
+
 func TestRotateCertCreateCSRError(t *testing.T) {
 	now := time.Now()
 	m := manager{
@@ -198,8 +246,10 @@ func TestRotateCertCreateCSRError(t *testing.T) {
 		},
 	}
 
-	if err := m.rotateCerts(); err == nil {
-		t.Errorf("Expected an error from 'rotateCerts'.")
+	if success, err := m.rotateCerts(); success {
+		t.Errorf("Got success from 'rotateCerts', wanted failure")
+	} else if err != nil {
+		t.Errorf("Got error %v from 'rotateCerts', wanted no error.", err)
 	}
 }
 
@@ -219,8 +269,10 @@ func TestRotateCertWaitingForResultError(t *testing.T) {
 		},
 	}
 
-	if err := m.rotateCerts(); err == nil {
-		t.Errorf("Expected an error receiving results from the CSR request but nothing was received.")
+	if success, err := m.rotateCerts(); success {
+		t.Errorf("Got success from 'rotateCerts', wanted failure.")
+	} else if err != nil {
+		t.Errorf("Got error %v from 'rotateCerts', wanted no error.", err)
 	}
 }
 
@@ -286,8 +338,11 @@ func TestNewManagerNoBootstrap(t *testing.T) {
 
 	if m, ok := cm.(*manager); !ok {
 		t.Errorf("Expected a '*manager' from 'NewManager'")
-	} else if m.shouldRotate() {
-		t.Errorf("Expected rotation should happen during bootstrap, but it won't.")
+	} else {
+		m.setRotationDeadline()
+		if m.shouldRotate() {
+			t.Errorf("Expected rotation should happen during bootstrap, but it won't.")
+		}
 	}
 }
 
@@ -436,8 +491,17 @@ func TestInitializeCertificateSigningRequestClient(t *testing.T) {
 				t.Errorf("Got error %v, expected none.", err)
 			}
 
-			if err := certificateManager.(*manager).rotateCerts(); err != nil {
-				t.Errorf("Got error %v, expected none.", err)
+			if m, ok := certificateManager.(*manager); !ok {
+				t.Errorf("Expected a '*manager' from 'NewManager'")
+			} else {
+				m.setRotationDeadline()
+				if m.shouldRotate() {
+					if success, err := m.rotateCerts(); !success {
+						t.Errorf("Got failure from 'rotateCerts', wanted success.")
+					} else if err != nil {
+						t.Errorf("Got error %v, expected none.", err)
+					}
+				}
 			}
 
 			certificate = certificateManager.Current()
@@ -526,8 +590,17 @@ func TestInitializeOtherRESTClients(t *testing.T) {
 				t.Errorf("Got %v, wanted %v", certificateString(certificate), certificateString(tc.expectedCertBeforeStart.certificate))
 			}
 
-			if err := certificateManager.(*manager).rotateCerts(); err != nil {
-				t.Errorf("Got error %v, expected none.", err)
+			if m, ok := certificateManager.(*manager); !ok {
+				t.Errorf("Expected a '*manager' from 'NewManager'")
+			} else {
+				m.setRotationDeadline()
+				if m.shouldRotate() {
+					if success, err := certificateManager.(*manager).rotateCerts(); !success {
+						t.Errorf("Got failure from 'rotateCerts', expected success")
+					} else if err != nil {
+						t.Errorf("Got error %v, expected none.", err)
+					}
+				}
 			}
 
 			certificate = certificateManager.Current()