diff --git a/pkg/kubelet/certificate/kubelet.go b/pkg/kubelet/certificate/kubelet.go index 69c022ef7fbcf..086016af40e99 100644 --- a/pkg/kubelet/certificate/kubelet.go +++ b/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. diff --git a/pkg/kubelet/certificate/kubelet_test.go b/pkg/kubelet/certificate/kubelet_test.go index a74818b04b303..c4474139e0d73 100644 --- a/pkg/kubelet/certificate/kubelet_test.go +++ b/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 +// /identity/kubelet.cert, /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") }