diff --git a/pkg/kubelet/certificate/certificate_manager.go b/pkg/kubelet/certificate/certificate_manager.go index ada732e9be2..d02ef4f3468 100644 --- a/pkg/kubelet/certificate/certificate_manager.go +++ b/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,6 +255,49 @@ func (m *manager) shouldRotate() bool { if m.cert == nil { return true } + 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 { + 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 { + 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 { + glog.Errorf("Unable to store the new cert/key pair: %v", err) + return false, nil + } + + m.updateCached(cert) + m.setRotationDeadline() + m.forceRotation = false + 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)) @@ -253,36 +309,7 @@ func (m *manager) shouldRotate() bool { // 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 - } - - csrPEM, keyPEM, err := m.generateCSR() - if err != nil { - return err - } - - // 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) - } - - cert, err := m.certStore.Update(crtPEM, keyPEM) - if err != nil { - return fmt.Errorf("unable to store the new cert/key pair: %v", err) - } - - m.updateCached(cert) - m.forceRotation = false - return nil + m.rotationDeadline = m.cert.Leaf.NotBefore.Add(jitteryDuration) } func (m *manager) updateCached(cert *tls.Certificate) { diff --git a/pkg/kubelet/certificate/certificate_manager_test.go b/pkg/kubelet/certificate/certificate_manager_test.go index f3980ccab29..a9fcf7b2dcc 100644 --- a/pkg/kubelet/certificate/certificate_manager_test.go +++ b/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()