update test

pkg/kubelet/certificate/kubelet.go

@@ -272,10 +272,11 @@ func (m *kubeletServerCertificateDynamicFileManager) Enqueue() {
 	cert, err := tls.X509KeyPair(certContent, keyContent)
 	if err != nil {
 		klog.ErrorS(err, "invalid certificate and key pair from file", "certFile", m.certFile, "keyFile", m.keyFile)
-	} else {
-		m.currentTLSCertificate.Store(&cert)
-		klog.InfoS("loaded certificate and key pair in kubelet server certificate manager", "certFile", m.certFile, "keyFile", m.keyFile)
+		return
 	}
+	m.currentTLSCertificate.Store(&cert)
+	klog.V(4).InfoS("loaded certificate and key pair in kubelet server certificate manager", "certFile", m.certFile, "keyFile", m.keyFile)
+
 }
 
 // Current returns the last valid certificate key pair loaded from files.

pkg/kubelet/certificate/kubelet_test.go

@@ -18,6 +18,8 @@ package certificate
 
 import (
 	"bytes"
+	"context"
+	"fmt"
 	"net"
 	"os"
 	"path/filepath"
@@ -26,6 +28,7 @@ import (
 	"time"
 
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/util/cert"
 	netutils "k8s.io/utils/net"
 )
@@ -106,14 +109,6 @@ func TestAddressesToHostnamesAndIPs(t *testing.T) {
 	}
 }
 
-func atomicWriteFile(name string, data []byte, perm os.FileMode) error {
-	tmp := name + ".tmp"
-	if err := os.WriteFile(tmp, data, perm); err != nil {
-		return err
-	}
-	return os.Rename(tmp, name)
-}
-
 func removeThenCreate(name string, data []byte, perm os.FileMode) error {
 	if err := os.Remove(name); err != nil {
 		if !os.IsNotExist(err) {
@@ -123,69 +118,96 @@ func removeThenCreate(name string, data []byte, perm os.FileMode) error {
 	return os.WriteFile(name, data, perm)
 }
 
-func createCertAndKeyFiles(certPath, keyPath string, useRename bool) error {
+func createCertAndKeyFiles(certDir string) (string, string, error) {
 	cert, key, err := cert.GenerateSelfSignedCertKey("k8s.io", nil, nil)
 	if err != nil {
-		return err
+		return "", "", nil
+	}
+
+	certPath := filepath.Join(certDir, "kubelet.cert")
+	keyPath := filepath.Join(certDir, "kubelet.key")
+	if err := removeThenCreate(certPath, cert, os.FileMode(0644)); err != nil {
+		return "", "", err
 	}
 
-	createFile := removeThenCreate
-	if useRename {
-		createFile = atomicWriteFile
+	if err := removeThenCreate(keyPath, key, os.FileMode(0600)); err != nil {
+		return "", "", err
 	}
 
-	if err := createFile(certPath, cert, os.FileMode(0644)); err != nil {
-		return err
+	return certPath, keyPath, nil
+}
+
+// createCertAndKeyFilesUsingRename creates cert and key files under a parent dir `identity` as
+// <certDir>/identity/kubelet.cert, <certDir>/identity/kubelet.key
+func createCertAndKeyFilesUsingRename(certDir string) (string, string, error) {
+	cert, key, err := cert.GenerateSelfSignedCertKey("k8s.io", nil, nil)
+	if err != nil {
+		return "", "", nil
 	}
 
-	if err := createFile(keyPath, key, os.FileMode(0600)); err != nil {
-		return err
+	var certKeyPathFn = func(dataDir string) (string, string, string) {
+		outputDir := filepath.Join(certDir, dataDir)
+		return outputDir, filepath.Join(outputDir, "kubelet.cert"), filepath.Join(outputDir, "kubelet.key")
 	}
-	return nil
+
+	writeDir, writeCertPath, writeKeyPath := certKeyPathFn("identity.tmp")
+
+	if err := os.Mkdir(writeDir, 0777); err != nil {
+		return "", "", err
+	}
+
+	if err := removeThenCreate(writeCertPath, cert, os.FileMode(0644)); err != nil {
+		return "", "", err
+	}
+
+	if err := removeThenCreate(writeKeyPath, key, os.FileMode(0600)); err != nil {
+		return "", "", err
+	}
+
+	targetDir, certPath, keyPath := certKeyPathFn("identity")
+	if err := os.RemoveAll(targetDir); err != nil {
+		if !os.IsNotExist(err) {
+			return "", "", err
+		}
+	}
+	if err := os.Rename(writeDir, targetDir); err != nil {
+		return "", "", err
+	}
+
+	return certPath, keyPath, nil
 }
 
 func TestKubeletServerCertificateFromFiles(t *testing.T) {
+	// test two common ways of certificate file updates:
+	// 1. native one, delete and write the cert and key files directly
+	// 2. create the cert and key files under a child dir and perform dir rename during update
 	tests := []struct {
-		name           string
-		useAtomicWrite bool
+		name      string
+		useRename bool
 	}{
 		{
-			name:           "use atomic write",
-			useAtomicWrite: true,
+			name:      "rename cert dir",
+			useRename: true,
 		},
 		{
-			name:           "remove and create",
-			useAtomicWrite: false,
+			name:      "remove and create",
+			useRename: false,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			certDir := t.TempDir()
-			rotateCertErrs := make(chan error, 10)
-			defer func() {
-				if len(rotateCertErrs) != 0 {
-					t.Errorf("got errors when rotating certificate files in the test")
-				}
-				close(rotateCertErrs)
-			}()
-
-			certPath := filepath.Join(certDir, "kubelet.cert")
-			keyPath := filepath.Join(certDir, "kubelet.key")
+			createFn := createCertAndKeyFiles
+			if tt.useRename {
+				createFn = createCertAndKeyFilesUsingRename
+			}
 
-			err := createCertAndKeyFiles(certPath, keyPath, tt.useAtomicWrite)
+			certDir := t.TempDir()
+			certPath, keyPath, err := createFn(certDir)
 			if err != nil {
 				t.Fatalf("Unable to setup cert files: %v", err)
 			}
 
-			// simulate certificate files update in the background
-			go func() {
-				time.Sleep(1 * time.Second)
-				if err := createCertAndKeyFiles(certPath, keyPath, tt.useAtomicWrite); err != nil {
-					rotateCertErrs <- err
-				}
-			}()
-
 			m, err := NewKubeletServerCertificateDynamicFileManager(certPath, keyPath)
 			if err != nil {
 				t.Fatalf("Unable to create certificte provider: %v", err)
@@ -207,18 +229,32 @@ func TestKubeletServerCertificateFromFiles(t *testing.T) {
 				t.Errorf("expected the same loaded certificate object when there is no cert file change, got different")
 			}
 
-			time.Sleep(2 * time.Second)
-			c3 := m.Current()
-			if c3 == nil {
-				t.Errorf("failed to provide valid certificate after file update")
-			} else if bytes.Equal(c.Certificate[0], c3.Certificate[0]) {
-				t.Errorf("failed to provide the updated certificate")
+			// simulate certificate files update in the background
+			if _, _, err := createFn(certDir); err != nil {
+				t.Fatalf("got errors when rotating certificate files in the test: %v", err)
+			}
+
+			checkCondition := func(_ context.Context) (bool, error) {
+				c3 := m.Current()
+				if c3 == nil {
+					return false, fmt.Errorf("expected valid certificate regardless of file changes, but got nil")
+				}
+				if bytes.Equal(c.Certificate[0], c3.Certificate[0]) {
+					t.Logf("loaded certificate is not updated")
+					return false, nil
+				}
+				return true, nil
+			}
+
+			if err := wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, 2*time.Second, true, checkCondition); err != nil {
+				t.Errorf("failed to provide the updated certificate: %v", err)
 			}
 
 			if err = os.Remove(certPath); err != nil {
 				t.Errorf("could not delete file in order to perform test")
 			}
 
+			time.Sleep(1 * time.Second)
 			if m.Current() == nil {
 				t.Errorf("expected the manager still provide cached content when certificate file was not available")
 			}