Fix the certificate rotation threshold and add jitter.

pkg/kubelet/certificate/certificate_manager.go

@@ -75,7 +75,6 @@ type manager struct {
 	certStore                Store
 	certAccessLock           sync.RWMutex
 	cert                     *tls.Certificate
-	shouldRotatePercent      uint
 }
 
 // NewManager returns a new certificate manager. A certificate manager is
@@ -85,25 +84,19 @@ func NewManager(
 	certSigningRequestClient certificatesclient.CertificateSigningRequestInterface,
 	template *x509.CertificateRequest,
 	usages []certificates.KeyUsage,
-	certificateStore Store,
+	certificateStore Store) (Manager, error) {
-	certRotationPercent uint) (Manager, error) {
 
 	cert, err := certificateStore.Current()
 	if err != nil {
 		return nil, err
 	}
 
-	if certRotationPercent > 100 {
-		certRotationPercent = 100
-	}
-
 	m := manager{
 		certSigningRequestClient: certSigningRequestClient,
 		template:                 template,
 		usages:                   usages,
 		certStore:                certificateStore,
 		cert:                     cert,
-		shouldRotatePercent:      certRotationPercent,
 	}
 
 	return &m, nil
@@ -129,15 +122,10 @@ func (m *manager) GetCertificate(clientHello *tls.ClientHelloInfo) (*tls.Certifi
 
 // Start will start the background work of rotating the certificates.
 func (m *manager) Start() {
-	if m.shouldRotatePercent < 1 {
-		glog.V(2).Infof("Certificate rotation is not enabled.")
-		return
-	}
-
 	// 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 master, where the kubelet is responsible
+	// client. This will happen on the cluster master, where the kubelet is
-	// for bootstrapping the pods of the master components.
+	// responsible for bootstrapping the pods of the master components.
 	if m.certSigningRequestClient == nil {
 		glog.V(2).Infof("Certificate rotation is not enabled, no connection to the apiserver.")
 		return
@@ -160,9 +148,17 @@ func (m *manager) shouldRotate() bool {
 	m.certAccessLock.RLock()
 	defer m.certAccessLock.RUnlock()
 	notAfter := m.cert.Leaf.NotAfter
-	total := notAfter.Sub(m.cert.Leaf.NotBefore)
+	totalDuration := float64(notAfter.Sub(m.cert.Leaf.NotBefore))
-	remaining := notAfter.Sub(time.Now())
+
-	return remaining < 0 || uint(remaining*100/total) < m.shouldRotatePercent
+	// 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)
+	return time.Now().After(rotationThreshold)
 }
 
 func (m *manager) rotateCerts() error {

pkg/kubelet/certificate/certificate_manager_test.go

@@ -78,18 +78,6 @@ O1eRCsCGPAnUCviFgNeH15ug+6N54DTTR6ZV/TTV64FDOcsox9nrhYcmH9sYuITi
 -----END CERTIFICATE-----`
 )
 
-func TestNewManagerNoRotation(t *testing.T) {
-	cert, err := tls.X509KeyPair([]byte(certificateData), []byte(privateKeyData))
-	if err != nil {
-		t.Fatalf("Unable to initialize a certificate: %v", err)
-	}
-
-	store := &fakeStore{cert: &cert}
-	if _, err := NewManager(nil, &x509.CertificateRequest{}, []certificates.KeyUsage{}, store, 0); err != nil {
-		t.Fatalf("Failed to initialize the certificate manager: %v", err)
-	}
-}
-
 func TestShouldRotate(t *testing.T) {
 	now := time.Now()
 	tests := []struct {
@@ -98,12 +86,16 @@ func TestShouldRotate(t *testing.T) {
 		notAfter     time.Time
 		shouldRotate bool
 	}{
-		{"half way", now.Add(-24 * time.Hour), now.Add(24 * time.Hour), false},
+		{"just issued, still good", now.Add(-1 * time.Hour), now.Add(99 * time.Hour), false},
-		{"nearly there", now.Add(-100 * time.Hour), now.Add(1 * time.Hour), true},
+		{"half way expired, still good", now.Add(-24 * time.Hour), now.Add(24 * time.Hour), false},
-		{"just started", now.Add(-1 * time.Hour), now.Add(100 * 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{
@@ -113,17 +105,15 @@ func TestShouldRotate(t *testing.T) {
 				},
 				template: &x509.CertificateRequest{},
 				usages:   []certificates.KeyUsage{},
-			shouldRotatePercent: 10,
 			}
-
 			if m.shouldRotate() != test.shouldRotate {
-			t.Errorf("For test case %s, time %v, a certificate issued for (%v, %v) should rotate should be %t.",
+				t.Errorf("For time %v, a certificate issued for (%v, %v) should rotate should be %t.",
-				test.name,
 					now,
 					m.cert.Leaf.NotBefore,
 					m.cert.Leaf.NotAfter,
 					test.shouldRotate)
 			}
+		})
 	}
 }