Merge pull request #18322 from smarterclayton/bootstrap

Automatic merge from submit-queue.

Write Kubelet flags as an option on openshift start node to support moving directly to kubelet

Instead of having openshift start node bootstrap, prepare to move to directly invoking the kubelet by having a flag on `openshift start node` called `--write-flags` that generates the arguments to invoke the kubelet for a given `--config`. Instead of calling `openshift start node` to do bootstrapping, we'd instead invoke the --write-flags path and call the kubelet directly.  The generated node-config on the system would have the correct bootstrap settings.

Would require us to move to dynamic config in the kubelet or to add a secondary loop to pull down the latest kube-config. That's probably acceptable.

Also contains a set of changes that allow certificate rotation to happen completely in the background, rather than blocking the kubelet startup. This allows us to keep bootstrapping the node from the master, but to still launch static pods in the bacgkround (right now we can't launch static pods while bootstrapping because bootstrapping is happening *before* the kubelet pod sync loop runs).  In this model, master containers as static pods will not require any node changes to make work (so master nodes wouldn't be different from other nodes).  I'm going to clean this up and propose upstream.

Note that this path would *not* require --runonce mode, which is very good because it's effectively unsupported.

@deads2k we're block on static pod for kubelet until we sort out the path forward. I don't want to have two separate types of node config, and I think this is probably the best position in the long run (all nodes bootstrap and have static pod config, nodes background loop waiting for bootstrapping and reject requests that require client/server connections until bootstrapping completes).

Origin-commit: a5d2ee081b9430a35e1a083b9077a0639d2aa10e


Kubernetes-commit: e6c9e9036872fde82dea33f209ff8a33b44ea8d9

Author: k8s-publishing-bot

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

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)