certsyncpod: Swap secret/cm directories atomically

Currently it can happen that cert-syncer replaces some of the
secret/configmap files successfully and then fails. This can lead to
problems when these are e.g. TLS cert/key files and the directory gets
inconsistent. This may seem transient, but when cert-syncer is
terminated in the middle, it can later fail to start as the whole
kube-apiserver gets into a crash loop.

This introduces a new staticpod.SwapDirectoriesAtomic, which uses
unix.Renameat2 with RENAME_EXCHANGE flag set.

Author: tchap

pkg/operator/staticpod/certsyncpod/certsync_controller.go

@@ -115,7 +115,8 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte
 
 		contentDir := getConfigMapDir(c.destinationDir, cm.Name)
 
-		data := map[string]string{}
+		data := make(map[string]string, len(configMap.Data))
+		dataBytes := make(map[string][]byte, len(configMap.Data))
 		for filename := range configMap.Data {
 			fullFilename := filepath.Join(contentDir, filename)
 
@@ -128,6 +129,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte
 			}
 
 			data[filename] = string(existingContent)
+			dataBytes[filename] = existingContent
 		}
 
 		// Check if cached configmap differs
@@ -152,27 +154,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte
 			continue
 		}
 
-		klog.Infof("Creating directory %q ...", contentDir)
-		if err := os.MkdirAll(contentDir, 0755); err != nil && !os.IsExist(err) {
-			c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed creating directory for configmap: %s/%s: %v", configMap.Namespace, configMap.Name, err)
-			errors = append(errors, err)
-			continue
-		}
-		for filename, content := range configMap.Data {
-			fullFilename := filepath.Join(contentDir, filename)
-			// if the existing is the same, do nothing
-			if reflect.DeepEqual(data[fullFilename], content) {
-				continue
-			}
-
-			klog.Infof("Writing configmap manifest %q ...", fullFilename)
-			if err := staticpod.WriteFileAtomic([]byte(content), 0644, fullFilename); err != nil {
-				c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed writing file for configmap: %s/%s: %v", configMap.Namespace, configMap.Name, err)
-				errors = append(errors, err)
-				continue
-			}
-		}
-		c.eventRecorder.Eventf("CertificateUpdated", "Wrote updated configmap: %s/%s", configMap.Namespace, configMap.Name)
+		errors = append(errors, c.writeData(configMap.Namespace, configMap.Name, "configmap", contentDir, dataBytes)...)
 	}
 
 	klog.Infof("Syncing secrets: %v", c.secrets)
@@ -220,7 +202,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte
 
 		contentDir := getSecretDir(c.destinationDir, s.Name)
 
-		data := map[string][]byte{}
+		data := make(map[string][]byte, len(secret.Data))
 		for filename := range secret.Data {
 			fullFilename := filepath.Join(contentDir, filename)
 
@@ -257,29 +239,64 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte
 			continue
 		}
 
-		klog.Infof("Creating directory %q ...", contentDir)
-		if err := os.MkdirAll(contentDir, 0755); err != nil && !os.IsExist(err) {
-			c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed creating directory for secret: %s/%s: %v", secret.Namespace, secret.Name, err)
+		errors = append(errors, c.writeData(secret.Namespace, secret.Name, "secret", contentDir, data)...)
+	}
+
+	return utilerrors.NewAggregate(errors)
+}
+
+func (c *CertSyncController) writeData(
+	objectNamespace, objectName, kind string,
+	contentDir string, data map[string][]byte,
+) []error {
+	var errors []error
+
+	// We are going to atomically swap the new data directory for the old one.
+	// In case the target directory does not exist, create it so that the directory not existing is not a special case.
+	klog.Infof("Ensuring directory %q exists ...", contentDir)
+	if err := os.MkdirAll(contentDir, 0755); err != nil && !os.IsExist(err) {
+		c.eventRecorder.Warningf("CertificateUpdateFailed",
+			"Failed creating content directory for %s: %s/%s: %v", kind, objectNamespace, objectName, err)
+		errors = append(errors, err)
+		return errors
+	}
+
+	// Create a tmp source directory to be swapped.
+	srcDir, err := os.MkdirTemp(filepath.Dir(contentDir), filepath.Base(contentDir)+"-*")
+	if err != nil {
+		c.eventRecorder.Warningf("CertificateUpdateFailed",
+			"Failed to create source %s directory for %s/%s: %v", kind, objectNamespace, objectName, err)
+		errors = append(errors, err)
+		return errors
+	}
+	defer os.RemoveAll(srcDir)
+
+	// Populate the tmp directory with files.
+	for filename, content := range data {
+		// TODO: Fix permissions
+		fullFilename := filepath.Join(srcDir, filename)
+		klog.Infof("Writing %s manifest %q ...", kind, fullFilename)
+
+		if err := os.WriteFile(fullFilename, content, 0600); err != nil {
+			c.eventRecorder.Warningf("CertificateUpdateFailed",
+				"Failed writing file for %s: %s/%s: %v", kind, objectNamespace, objectName, err)
 			errors = append(errors, err)
 			continue
 		}
-		for filename, content := range secret.Data {
-			// TODO fix permissions
-			fullFilename := filepath.Join(contentDir, filename)
-			// if the existing is the same, do nothing
-			if reflect.DeepEqual(data[fullFilename], content) {
-				continue
-			}
+	}
+	if len(errors) > 0 {
+		return errors
+	}
 
-			klog.Infof("Writing secret manifest %q ...", fullFilename)
-			if err := staticpod.WriteFileAtomic(content, 0600, fullFilename); err != nil {
-				c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed writing file for secret: %s/%s: %v", secret.Namespace, secret.Name, err)
-				errors = append(errors, err)
-				continue
-			}
-		}
-		c.eventRecorder.Eventf("CertificateUpdated", "Wrote updated secret: %s/%s", secret.Namespace, secret.Name)
+	// Swap directories atomically.
+	if err := staticpod.SwapDirectoriesAtomic(contentDir, srcDir); err != nil {
+		c.eventRecorder.Warningf("CertificateUpdateFailed",
+			"Failed to enable new %s directory for %s/%s: %v", kind, objectNamespace, objectName, err)
+		errors = append(errors, err)
+		return errors
 	}
 
-	return utilerrors.NewAggregate(errors)
+	c.eventRecorder.Eventf("CertificateUpdated",
+		"Wrote updated %s: %s/%s", kind, objectNamespace, objectName)
+	return nil
 }

pkg/operator/staticpod/file_utils.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+
+	"golang.org/x/sys/unix"
 )
 
 func WriteFileAtomic(content []byte, filePerms os.FileMode, fullFilename string) error {
@@ -31,3 +33,20 @@ func writeTemporaryFile(content []byte, filePerms os.FileMode, fullFilename stri
 	}
 	return tmpfile.Name(), nil
 }
+
+// SwapDirectoriesAtomic can be used to swap two directories atomically.
+func SwapDirectoriesAtomic(dirA, dirB string) error {
+	fdA, err := unix.Open(dirA, unix.O_DIRECTORY, 0)
+	if err != nil {
+		return err
+	}
+	defer unix.Close(fdA)
+
+	fdB, err := unix.Open(dirB, unix.O_DIRECTORY, 0)
+	if err != nil {
+		return err
+	}
+	defer unix.Close(fdB)
+
+	return unix.Renameat2(fdA, dirA, fdB, dirB, unix.RENAME_EXCHANGE)
+}

pkg/operator/staticpod/file_utils_test.go

@@ -0,0 +1,105 @@
+package staticpod
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestSwapDirectoriesAtomic(t *testing.T) {
+	expectNoError := func(t *testing.T, err error) {
+		if err != nil {
+			t.Fatalf("Expected no error, got %v", err)
+		}
+	}
+
+	testCases := []struct {
+		name        string
+		setup       func(t *testing.T, tmpDir string) (dirA, dirB string)
+		checkResult func(t *testing.T, dirA, dirB string, err error)
+	}{
+		{
+			name: "both directories exist",
+			setup: func(t *testing.T, tmpDir string) (string, string) {
+				dirA := filepath.Join(tmpDir, "a")
+				dirB := filepath.Join(tmpDir, "b")
+
+				if err := os.Mkdir(dirA, 0755); err != nil {
+					t.Fatalf("Failed to create directory %s: %v", dirA, err)
+				}
+				if err := os.Mkdir(dirB, 0755); err != nil {
+					t.Fatalf("Failed to create directory %s: %v", dirB, err)
+				}
+
+				fileA, err := os.Create(filepath.Join(dirA, "1.txt"))
+				expectNoError(t, err)
+				defer fileA.Close()
+
+				fileB, err := os.Create(filepath.Join(dirB, "2.txt"))
+				expectNoError(t, err)
+				defer fileB.Close()
+
+				return dirA, dirB
+			},
+			checkResult: func(t *testing.T, dirA, dirB string, err error) {
+				expectNoError(t, err)
+
+				// Make sure the contents are swapped.
+				fileA, err := os.Open(filepath.Join(dirA, "2.txt"))
+				if err != nil {
+					t.Errorf("Expected directory %q to contain file 2.txt: %v", dirA, err)
+				}
+				defer fileA.Close()
+
+				fileB, err := os.Open(filepath.Join(dirB, "1.txt"))
+				if err != nil {
+					t.Errorf("Expected directory %q to contain file 1.txt: %v", dirB, err)
+				}
+				defer fileB.Close()
+			},
+		},
+		{
+			name: "directory A does not exist",
+			setup: func(t *testing.T, tmpDir string) (string, string) {
+				dirA := filepath.Join(tmpDir, "a")
+				dirB := filepath.Join(tmpDir, "b")
+
+				if err := os.Mkdir(dirB, 0755); err != nil {
+					t.Fatalf("Failed to create directory %s: %v", dirB, err)
+				}
+
+				return dirA, dirB
+			},
+			checkResult: func(t *testing.T, dirA, dirB string, err error) {
+				if !os.IsNotExist(err) {
+					t.Errorf("Expected a directory not exists error, got %v", err)
+				}
+			},
+		},
+		{
+			name: "directory B does not exist",
+			setup: func(t *testing.T, tmpDir string) (string, string) {
+				dirA := filepath.Join(tmpDir, "a")
+				dirB := filepath.Join(tmpDir, "b")
+
+				if err := os.Mkdir(dirA, 0755); err != nil {
+					t.Fatalf("Failed to create directory %s: %v", dirA, err)
+				}
+
+				return dirA, dirB
+			},
+			checkResult: func(t *testing.T, dirA, dirB string, err error) {
+				if !os.IsNotExist(err) {
+					t.Errorf("Expected a directory not exists error, got %v", err)
+				}
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			dirA, dirB := tc.setup(t, t.TempDir())
+			tc.checkResult(t, dirA, dirB, SwapDirectoriesAtomic(dirA, dirB))
+		})
+	}
+}