Only rotate certificates in the background

pkg/kubelet/kubelet.go

@@ -774,7 +774,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
 		kubeDeps.TLSOptions.Config.GetCertificate = func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
 			cert := klet.serverCertificateManager.Current()
 			if cert == nil {
-				return nil, fmt.Errorf("no certificate available")
+				return nil, fmt.Errorf("no serving certificate available for the kubelet")
 			}
 			return cert, nil
 		}

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

@@ -141,7 +141,6 @@ type manager struct {
 	certStore                Store
 	certAccessLock           sync.RWMutex
 	cert                     *tls.Certificate
-	rotationDeadline         time.Time
 	forceRotation            bool
 	certificateExpiration    Gauge
 	serverHealth             bool
@@ -208,36 +207,26 @@ func (m *manager) SetCertificateSigningRequestClient(certSigningRequestClient ce
 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. This will happen on the cluster master, where the kubelet is
-	// responsible for bootstrapping the pods of the master components.
+	// client.
 	if m.certSigningRequestClient == nil {
 		glog.V(2).Infof("Certificate rotation is not enabled, no connection to the apiserver.")
 		return
 	}
 
 	glog.V(2).Infof("Certificate rotation is enabled.")
 
-	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() {
-		glog.V(1).Infof("shouldRotate() is true, forcing immediate rotation")
-		if _, err := m.rotateCerts(); err != nil {
-			utilruntime.HandleError(fmt.Errorf("Could not rotate certificates: %v", err))
-		}
-	}
-	backoff := wait.Backoff{
-		Duration: 2 * time.Second,
-		Factor:   2,
-		Jitter:   0.1,
-		Steps:    5,
-	}
 	go wait.Forever(func() {
-		sleepInterval := m.rotationDeadline.Sub(time.Now())
-		glog.V(2).Infof("Waiting %v for next certificate rotation", sleepInterval)
-		time.Sleep(sleepInterval)
+		deadline := m.nextRotationDeadline()
+		if sleepInterval := deadline.Sub(time.Now()); sleepInterval > 0 {
+			glog.V(2).Infof("Waiting %v for next certificate rotation", sleepInterval)
+			time.Sleep(sleepInterval)
+		}
+		backoff := wait.Backoff{
+			Duration: 2 * time.Second,
+			Factor:   2,
+			Jitter:   0.1,
+			Steps:    5,
+		}
 		if err := wait.ExponentialBackoff(backoff, m.rotateCerts); err != nil {
 			utilruntime.HandleError(fmt.Errorf("Reached backoff limit, still unable to rotate certs: %v", err))
 			wait.PollInfinite(32*time.Second, m.rotateCerts)
@@ -252,11 +241,14 @@ func getCurrentCertificateOrBootstrap(
 
 	currentCert, err := store.Current()
 	if err == nil {
-		return currentCert, false, nil
-	}
-
-	if _, ok := err.(*NoCertKeyError); !ok {
-		return nil, false, err
+		// if the current cert is expired, fall back to the bootstrap cert
+		if currentCert.Leaf != nil && time.Now().Before(currentCert.Leaf.NotAfter) {
+			return currentCert, false, nil
+		}
+	} else {
+		if _, ok := err.(*NoCertKeyError); !ok {
+			return nil, false, err
+		}
 	}
 
 	if bootstrapCertificatePEM == nil || bootstrapKeyPEM == nil {
@@ -279,20 +271,6 @@ func getCurrentCertificateOrBootstrap(
 	return &bootstrapCert, true, nil
 }
 
-// shouldRotate looks at how close the current certificate is to expiring and
-// decides if it is time to rotate or not.
-func (m *manager) shouldRotate() bool {
-	m.certAccessLock.RLock()
-	defer m.certAccessLock.RUnlock()
-	if m.cert == nil {
-		return true
-	}
-	if m.forceRotation {
-		return true
-	}
-	return time.Now().After(m.rotationDeadline)
-}
-
 // 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.
@@ -349,30 +327,34 @@ func (m *manager) rotateCerts() (bool, error) {
 	}
 
 	m.updateCached(cert)
-	m.setRotationDeadline()
-	m.forceRotation = false
 	return true, nil
 }
 
-// setRotationDeadline sets a cached value for the threshold at which the
+// nextRotationDeadline returns a value for the threshold at which the
 // current certificate should be rotated, 80%+/-10% of the expiration of the
 // certificate.
-func (m *manager) setRotationDeadline() {
+func (m *manager) nextRotationDeadline() time.Time {
+	// forceRotation is not protected by locks
+	if m.forceRotation {
+		m.forceRotation = false
+		return time.Now()
+	}
+
 	m.certAccessLock.RLock()
 	defer m.certAccessLock.RUnlock()
 	if m.cert == nil {
-		m.rotationDeadline = time.Now()
-		return
+		return time.Now()
 	}
 
 	notAfter := m.cert.Leaf.NotAfter
 	totalDuration := float64(notAfter.Sub(m.cert.Leaf.NotBefore))
+	deadline := m.cert.Leaf.NotBefore.Add(jitteryDuration(totalDuration))
 
-	m.rotationDeadline = m.cert.Leaf.NotBefore.Add(jitteryDuration(totalDuration))
-	glog.V(2).Infof("Certificate expiration is %v, rotation deadline is %v", notAfter, m.rotationDeadline)
+	glog.V(2).Infof("Certificate expiration is %v, rotation deadline is %v", notAfter, deadline)
 	if m.certificateExpiration != nil {
 		m.certificateExpiration.Set(float64(notAfter.Unix()))
 	}
+	return deadline
 }
 
 // jitteryDuration uses some jitter to set the rotation threshold so each node

staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go

@@ -146,46 +146,6 @@ func TestNewManagerNoRotation(t *testing.T) {
 	}
 }
 
-func TestShouldRotate(t *testing.T) {
-	now := time.Now()
-	tests := []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},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			m := manager{
-				cert: &tls.Certificate{
-					Leaf: &x509.Certificate{
-						NotBefore: test.notBefore,
-						NotAfter:  test.notAfter,
-					},
-				},
-				template: &x509.CertificateRequest{},
-				usages:   []certificates.KeyUsage{},
-			}
-			m.setRotationDeadline()
-			if m.shouldRotate() != test.shouldRotate {
-				t.Errorf("Time %v, a certificate issued for (%v, %v) should rotate should be %t.",
-					now,
-					m.cert.Leaf.NotBefore,
-					m.cert.Leaf.NotAfter,
-					test.shouldRotate)
-			}
-		})
-	}
-}
-
 type gaugeMock struct {
 	calls     int
 	lastValue float64
@@ -233,13 +193,13 @@ func TestSetRotationDeadline(t *testing.T) {
 			jitteryDuration = func(float64) time.Duration { return time.Duration(float64(tc.notAfter.Sub(tc.notBefore)) * 0.7) }
 			lowerBound := tc.notBefore.Add(time.Duration(float64(tc.notAfter.Sub(tc.notBefore)) * 0.7))
 
-			m.setRotationDeadline()
+			deadline := m.nextRotationDeadline()
 
-			if !m.rotationDeadline.Equal(lowerBound) {
+			if !deadline.Equal(lowerBound) {
 				t.Errorf("For notBefore %v, notAfter %v, the rotationDeadline %v should be %v.",
 					tc.notBefore,
 					tc.notAfter,
-					m.rotationDeadline,
+					deadline,
 					lowerBound)
 			}
 			if g.calls != 1 {
@@ -321,7 +281,7 @@ func TestNewManagerBootstrap(t *testing.T) {
 	}
 	if m, ok := cm.(*manager); !ok {
 		t.Errorf("Expected a '*manager' from 'NewManager'")
-	} else if !m.shouldRotate() {
+	} else if !m.forceRotation {
 		t.Errorf("Expected rotation should happen during bootstrap, but it won't.")
 	}
 }
@@ -360,9 +320,8 @@ func TestNewManagerNoBootstrap(t *testing.T) {
 	if m, ok := cm.(*manager); !ok {
 		t.Errorf("Expected a '*manager' from 'NewManager'")
 	} else {
-		m.setRotationDeadline()
-		if m.shouldRotate() {
-			t.Errorf("Expected rotation should happen during bootstrap, but it won't.")
+		if m.forceRotation {
+			t.Errorf("Expected rotation should not happen during bootstrap, but it won't.")
 		}
 	}
 }
@@ -515,8 +474,7 @@ func TestInitializeCertificateSigningRequestClient(t *testing.T) {
 			if m, ok := certificateManager.(*manager); !ok {
 				t.Errorf("Expected a '*manager' from 'NewManager'")
 			} else {
-				m.setRotationDeadline()
-				if m.shouldRotate() {
+				if m.forceRotation {
 					if success, err := m.rotateCerts(); !success {
 						t.Errorf("Got failure from 'rotateCerts', wanted success.")
 					} else if err != nil {
@@ -614,8 +572,7 @@ func TestInitializeOtherRESTClients(t *testing.T) {
 			if m, ok := certificateManager.(*manager); !ok {
 				t.Errorf("Expected a '*manager' from 'NewManager'")
 			} else {
-				m.setRotationDeadline()
-				if m.shouldRotate() {
+				if m.forceRotation {
 					success, err := certificateManager.(*manager).rotateCerts()
 					if err != nil {
 						t.Errorf("Got error %v, expected none.", err)