certsyncpod: Swap secret/cm directories atomically

tchap · tchap · commit a389cbd8f55b · 2025-09-10T11:12:29.000+02:00
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.
diff --git a/pkg/operator/staticpod/certsyncpod/certsync_controller.go b/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
 }
diff --git a/pkg/operator/staticpod/certsyncpod/certsync_controller_test.go b/pkg/operator/staticpod/certsyncpod/certsync_controller_test.go
@@ -0,0 +1,116 @@
+package certsyncpod
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/openshift/library-go/pkg/operator/events/eventstesting"
+)
+
+func TestCertSyncController_WriteData(t *testing.T) {
+	const (
+		objectNamespace = "default"
+		objectName      = "server_cert"
+	)
+
+	testCases := []struct {
+		name          string
+		directoryData map[string][]byte
+		objectData    map[string][]byte
+	}{
+		{
+			name:          "create target directory",
+			directoryData: nil,
+			objectData: map[string][]byte{
+				"tls.crt": []byte("TLS cert"),
+				"tls.key": []byte("TLS key"),
+			},
+		},
+		{
+			name: "update target directory change file content",
+			directoryData: map[string][]byte{
+				"tls.crt": []byte("TLS cert"),
+				"tls.key": []byte("TLS key"),
+			},
+			objectData: map[string][]byte{
+				"tls.crt": []byte("rotated TLS cert"),
+				"tls.key": []byte("rotated TLS key"),
+			},
+		},
+		{
+			name: "update target directory change filenames",
+			directoryData: map[string][]byte{
+				"tls.crt": []byte("TLS cert"),
+				"tls.key": []byte("TLS key"),
+			},
+			objectData: map[string][]byte{
+				"api.crt": []byte("rotated TLS cert"),
+				"api.key": []byte("rotated TLS key"),
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// Init the controller.
+			recorder := eventstesting.NewTestingEventRecorder(t)
+			controller := &CertSyncController{
+				eventRecorder: recorder,
+			}
+
+			// Write the current directory contents.
+			contentDir := filepath.Join(t.TempDir(), "secrets", objectName)
+			if tc.directoryData != nil {
+				if err := os.MkdirAll(contentDir, 0755); err != nil {
+					t.Fatalf("Failed to create content directory %q: %v", contentDir, err)
+				}
+
+				for filename, content := range tc.directoryData {
+					targetPath := filepath.Join(contentDir, filename)
+					if err := os.WriteFile(targetPath, content, 0644); err != nil {
+						t.Fatalf("Failed to populate file %q: %v", targetPath, err)
+					}
+				}
+			}
+
+			// Replace with the object data.
+			errs := controller.writeData(objectNamespace, objectName, "secret", contentDir, tc.objectData)
+			if len(errs) > 0 {
+				t.Fatalf("Unexpected errors when writing new data object: %v", errs)
+			}
+
+			// Ensure the content directory is in sync.
+			entries, err := os.ReadDir(contentDir)
+			if err != nil {
+				t.Fatalf("Failed to read directory %q: %v", contentDir, err)
+			}
+			writtenData := make(map[string][]byte, len(entries))
+			for _, entry := range entries {
+				content, err := os.ReadFile(filepath.Join(contentDir, entry.Name()))
+				if err != nil {
+					t.Fatalf("Failed to read file %q: %v", entry.Name(), err)
+				}
+				writtenData[entry.Name()] = content
+			}
+			if !cmp.Equal(writtenData, tc.objectData) {
+				t.Errorf("Unexpected content directory content:\n%s\n", cmp.Diff(tc.objectData, writtenData))
+			}
+
+			// Make sure there are no leftovers in the parent directory.
+			parentDir := filepath.Dir(contentDir)
+			parentEntries, err := os.ReadDir(parentDir)
+			if err != nil {
+				t.Fatalf("Failed to read directory %q: %v", parentDir, err)
+			}
+			if n := len(parentEntries); n != 1 {
+				t.Errorf("Unexpected number of entries in directory %q: %d", parentDir, n)
+				for _, entry := range parentEntries {
+					t.Logf("Parent directory entry: %q", entry.Name())
+				}
+			}
+		})
+	}
+
+}
diff --git a/pkg/operator/staticpod/file_utils.go b/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)
+}
diff --git a/pkg/operator/staticpod/file_utils_test.go b/pkg/operator/staticpod/file_utils_test.go
@@ -0,0 +1,106 @@
+package staticpod
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestSwapDirectoriesAtomic(t *testing.T) {
+	expectNoError := func(t *testing.T, err error) {
+		t.Helper()
+		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))
+		})
+	}
+}
