certsyncpod: Swap secret/cm directories atomically

tchap · tchap · commit 60f05a8ffd5e · 2025-09-10T14:45:33.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,7 @@ 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))
 		for filename := range configMap.Data {
 			fullFilename := filepath.Join(contentDir, filename)
 
@@ -152,27 +152,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, writeFiles(&realFS, c.eventRecorder, configMap.Namespace, configMap.Name, "configmap", contentDir, data, 0644)...)
 	}
 
 	klog.Infof("Syncing secrets: %v", c.secrets)
@@ -220,7 +200,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 +237,80 @@ 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, writeFiles(&realFS, c.eventRecorder, secret.Namespace, secret.Name, "secret", contentDir, data, 0600)...)
+	}
+
+	return utilerrors.NewAggregate(errors)
+}
+
+type fileSystem struct {
+	MkdirAll              func(path string, perm os.FileMode) error
+	MkdirTemp             func(dir, pattern string) (string, error)
+	RemoveAll             func(path string) error
+	WriteFile             func(name string, data []byte, perm os.FileMode) error
+	SwapDirectoriesAtomic func(dirA, dirB string) error
+}
+
+var realFS = fileSystem{
+	MkdirAll:              os.MkdirAll,
+	MkdirTemp:             os.MkdirTemp,
+	RemoveAll:             os.RemoveAll,
+	WriteFile:             os.WriteFile,
+	SwapDirectoriesAtomic: staticpod.SwapDirectoriesAtomic,
+}
+
+func writeFiles[C string | []byte](
+	fs *fileSystem, eventRecorder events.Recorder,
+	objectNamespace, objectName, kind string,
+	contentDir string, files map[string]C, filePerm os.FileMode,
+) []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 := fs.MkdirAll(contentDir, 0755); err != nil && !os.IsExist(err) {
+		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 := fs.MkdirTemp(filepath.Dir(contentDir), filepath.Base(contentDir)+"-*")
+	if err != nil {
+		eventRecorder.Warningf("CertificateUpdateFailed",
+			"Failed to create source %s directory for %s/%s: %v", kind, objectNamespace, objectName, err)
+		errors = append(errors, err)
+		return errors
+	}
+	defer fs.RemoveAll(srcDir)
+
+	// Populate the tmp directory with files.
+	for filename, content := range files {
+		fullFilename := filepath.Join(srcDir, filename)
+		klog.Infof("Writing %s manifest %q ...", kind, fullFilename)
+
+		if err := fs.WriteFile(fullFilename, []byte(content), filePerm); err != nil {
+			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 := fs.SwapDirectoriesAtomic(contentDir, srcDir); err != nil {
+		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)
+	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,204 @@
+package certsyncpod
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+
+	"github.com/openshift/library-go/pkg/operator/events/eventstesting"
+)
+
+func TestWriteFiles(t *testing.T) {
+	const (
+		objectNamespace = "default"
+		objectName      = "server_cert"
+	)
+
+	newRealFS := func() *fileSystem {
+		fs := realFS
+		return &fs
+	}
+
+	checkDirectoryContents := func(t *testing.T, contentDir string, files map[string][]byte) {
+		// 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 {
+			info, err := entry.Info()
+			if err != nil {
+				t.Fatalf("Failed to read file information for %q: %v", entry.Name(), err)
+			}
+			if perm := info.Mode().Perm(); perm != 0600 {
+				t.Errorf("Unexpected file permissions for %q: %v", entry.Name(), perm)
+			}
+
+			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, files) {
+			t.Errorf("Unexpected directory content:\n%s\n", cmp.Diff(files, writtenData))
+		}
+	}
+
+	ensureParentDirectoryClean := func(t *testing.T, contentDir string) {
+		// 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())
+			}
+		}
+	}
+
+	checkOnSuccess := func(t *testing.T, contentDir string, directoryData, objectData map[string][]byte, errs []error) {
+		if len(errs) > 0 {
+			t.Fatalf("Unexpected errors when writing new data object: %v", errs)
+		}
+
+		checkDirectoryContents(t, contentDir, objectData)
+		ensureParentDirectoryClean(t, contentDir)
+	}
+
+	checkOnError := func(t *testing.T, contentDir string, directoryData, objectData map[string][]byte, errs []error) {
+		if len(errs) == 0 {
+			t.Error("Expected some errors, got none")
+		}
+
+		checkDirectoryContents(t, contentDir, directoryData)
+		ensureParentDirectoryClean(t, contentDir)
+	}
+
+	type testCase struct {
+		name          string
+		newFS         func() *fileSystem
+		directoryData map[string][]byte
+		objectData    map[string][]byte
+		checkState    func(t *testing.T, contentDir string, directoryData, objectData map[string][]byte, errs []error)
+	}
+
+	errorTestCase := func(name string, newFS func() *fileSystem) testCase {
+		return testCase{
+			name:  name,
+			newFS: newFS,
+			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"),
+			},
+			checkState: checkOnError,
+		}
+	}
+
+	testCases := []testCase{
+		{
+			name:          "create target directory",
+			newFS:         newRealFS,
+			directoryData: nil,
+			objectData: map[string][]byte{
+				"tls.crt": []byte("TLS cert"),
+				"tls.key": []byte("TLS key"),
+			},
+			checkState: checkOnSuccess,
+		},
+		{
+			name:  "update target directory change file content",
+			newFS: newRealFS,
+			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"),
+			},
+			checkState: checkOnSuccess,
+		},
+		{
+			name:  "update target directory change filenames",
+			newFS: newRealFS,
+			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"),
+			},
+			checkState: checkOnSuccess,
+		},
+		errorTestCase("directory unchanged on failed to create object directory", func() *fileSystem {
+			fs := newRealFS()
+			fs.MkdirAll = func(path string, perm os.FileMode) error {
+				return errors.New("nuked")
+			}
+			return fs
+		}),
+		errorTestCase("directory unchanged on failed to create temporary directory", func() *fileSystem {
+			fs := newRealFS()
+			fs.MkdirTemp = func(dir, pattern string) (string, error) {
+				return "", errors.New("nuked")
+			}
+			return fs
+		}),
+		errorTestCase("directory unchanged on failed to write file", func() *fileSystem {
+			fs := newRealFS()
+			fs.WriteFile = func(path string, data []byte, perm os.FileMode) error {
+				return errors.New("nuked")
+			}
+			return fs
+		}),
+		errorTestCase("directory unchanged on failed to swap directories", func() *fileSystem {
+			fs := newRealFS()
+			fs.SwapDirectoriesAtomic = func(dirA, dirB string) error {
+				return errors.New("nuked")
+			}
+			return fs
+		}),
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			recorder := eventstesting.NewTestingEventRecorder(t)
+
+			// 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, 0600); err != nil {
+						t.Fatalf("Failed to populate file %q: %v", targetPath, err)
+					}
+				}
+			}
+
+			// Replace with the object data.
+			errs := writeFiles(tc.newFS(), recorder, objectNamespace, objectName, "secret",
+				contentDir, tc.objectData, 0600)
+
+			// Check the resulting state.
+			tc.checkState(t, contentDir, tc.directoryData, tc.objectData, errs)
+		})
+	}
+
+}
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,16 @@ func writeTemporaryFile(content []byte, filePerms os.FileMode, fullFilename stri
 	}
 	return tmpfile.Name(), nil
 }
+
+// SwapDirectoriesAtomic can be used to swap two directories atomically.
+//
+// This function requires absolute paths and will return an error if that's not the case.
+func SwapDirectoriesAtomic(dirA, dirB string) error {
+	if !filepath.IsAbs(dirA) {
+		return fmt.Errorf("not an absolute path: %q", dirA)
+	}
+	if !filepath.IsAbs(dirB) {
+		return fmt.Errorf("not an absolute path: %q", dirB)
+	}
+	return unix.Renameat2(0, dirA, 0, dirB, unix.RENAME_EXCHANGE)
+}
diff --git a/pkg/operator/staticpod/file_utils_test.go b/pkg/operator/staticpod/file_utils_test.go
