From 1e604b075a37c7501022dba8fbc588755e80026a Mon Sep 17 00:00:00 2001 From: Ondra Kupka Date: Wed, 1 Oct 2025 12:31:59 +0200 Subject: [PATCH 1/8] Add atomicdir package pkg/operator/staticpod/internal/atomicdir contains internal helpers for performing atomic operations on directories. This patch contains only a standalone swap function, which will be subsequenty used for synchronizing directory with the given state. --- pkg/operator/staticpod/installerpod/cmd.go | 3 +- .../internal/atomicdir/swap_linux.go | 22 ++ .../internal/atomicdir/swap_linux_test.go | 251 ++++++++++++++++++ .../internal/atomicdir/swap_other.go | 10 + 4 files changed, 285 insertions(+), 1 deletion(-) create mode 100644 pkg/operator/staticpod/internal/atomicdir/swap_linux.go create mode 100644 pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go create mode 100644 pkg/operator/staticpod/internal/atomicdir/swap_other.go diff --git a/pkg/operator/staticpod/installerpod/cmd.go b/pkg/operator/staticpod/installerpod/cmd.go index 5d065b40fc..31afadff86 100644 --- a/pkg/operator/staticpod/installerpod/cmd.go +++ b/pkg/operator/staticpod/installerpod/cmd.go @@ -3,7 +3,6 @@ package installerpod import ( "context" "fmt" - "k8s.io/utils/clock" "os" "path" "sort" @@ -11,6 +10,8 @@ import ( "strings" "time" + "k8s.io/utils/clock" + "k8s.io/apimachinery/pkg/util/wait" "github.com/blang/semver/v4" diff --git a/pkg/operator/staticpod/internal/atomicdir/swap_linux.go b/pkg/operator/staticpod/internal/atomicdir/swap_linux.go new file mode 100644 index 0000000000..9ce912af76 --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/swap_linux.go @@ -0,0 +1,22 @@ +//go:build linux + +package atomicdir + +import ( + "golang.org/x/sys/unix" +) + +// swap can be used to exchange two directories atomically. +func swap(firstDir, secondDir string) error { + // Renameat2 can be used to exchange two directories atomically when RENAME_EXCHANGE flag is specified. + // The paths to be exchanged can be specified in multiple ways: + // + // * You can specify a file descriptor and a relative path to that descriptor. + // * You can specify an absolute path, in which case the file descriptor is ignored. + // + // We use AT_FDCWD special file descriptor so that when any of the paths is relative, + // it's relative to the current working directory. + // + // For more details, see `man renameat2` as that is the associated C library function. + return unix.Renameat2(unix.AT_FDCWD, firstDir, unix.AT_FDCWD, secondDir, unix.RENAME_EXCHANGE) +} diff --git a/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go b/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go new file mode 100644 index 0000000000..ef4370180e --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go @@ -0,0 +1,251 @@ +//go:build linux + +package atomicdir + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "github.com/google/go-cmp/cmp" + + "k8s.io/apimachinery/pkg/util/sets" +) + +func TestSwap(t *testing.T) { + stateFirst := directoryState{ + "1.txt": { + Content: []byte("hello 1 world"), + Perm: 0600, + }, + "2.txt": { + Content: []byte("hello 2 world"), + Perm: 0400, + }, + } + stateSecond := directoryState{ + "a.txt": { + Content: []byte("hello a world"), + Perm: 0600, + }, + } + stateEmpty := directoryState{} + + expectNoError := func(t *testing.T, err error) { + t.Helper() + if err != nil { + t.Fatalf("Expected no error, got %v", err) + } + } + + checkSuccess := func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, err error) { + t.Helper() + expectNoError(t, err) + + // Make sure the contents are swapped. + stateFirst.CheckDirectoryMatches(t, pathSecond) + stateSecond.CheckDirectoryMatches(t, pathFirst) + } + + testCases := []struct { + name string + setup func(t *testing.T, tmpDir string) (pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState) + checkResult func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, err error) + }{ + { + name: "success with absolute paths", + setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateFirst.Write(t, pathFirst) + stateSecond.Write(t, pathSecond) + + return pathFirst, stateFirst, pathSecond, stateSecond + }, + checkResult: checkSuccess, + }, + { + name: "success with the first path relative", + setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateFirst.Write(t, pathFirst) + stateSecond.Write(t, pathSecond) + + cwd, err := os.Getwd() + expectNoError(t, err) + + relFirst, err := filepath.Rel(cwd, pathFirst) + expectNoError(t, err) + + return relFirst, stateFirst, pathSecond, stateSecond + }, + checkResult: checkSuccess, + }, + { + name: "success with the second path relative", + setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateFirst.Write(t, pathFirst) + stateSecond.Write(t, pathSecond) + + cwd, err := os.Getwd() + expectNoError(t, err) + + relSecond, err := filepath.Rel(cwd, pathSecond) + expectNoError(t, err) + + return pathFirst, stateFirst, relSecond, stateSecond + }, + checkResult: checkSuccess, + }, + { + name: "success with an empty directory", + setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateFirst.Write(t, pathFirst) + stateEmpty.Write(t, pathSecond) + + return pathFirst, stateFirst, pathSecond, stateEmpty + }, + checkResult: checkSuccess, + }, + { + name: "success with both directories empty", + setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateEmpty.Write(t, pathFirst) + stateEmpty.Write(t, pathSecond) + + return pathFirst, stateEmpty, pathSecond, stateEmpty + }, + checkResult: checkSuccess, + }, + { + name: "error with the first directory not existing", + setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + expectNoError(t, os.Mkdir(pathSecond, 0755)) + + return pathFirst, stateEmpty, pathSecond, stateEmpty + }, + checkResult: func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, err error) { + if !os.IsNotExist(err) { + t.Errorf("Expected a directory not exists error, got %v", err) + } + }, + }, + { + name: "error with the second directory not existing", + setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + expectNoError(t, os.Mkdir(pathFirst, 0755)) + + return pathFirst, stateEmpty, pathSecond, stateEmpty + }, + checkResult: func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, err error) { + if !os.IsNotExist(err) { + t.Errorf("Expected a directory not exists error, got %v", err) + } + }, + }, + { + name: "error with no directory existing", + setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + return pathFirst, stateEmpty, pathSecond, stateEmpty + }, + checkResult: func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, 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) { + pathFirst, stateFirst, pathSecond, stateSecond := tc.setup(t, t.TempDir()) + tc.checkResult(t, pathFirst, stateFirst, pathSecond, stateSecond, swap(pathFirst, pathSecond)) + }) + } +} + +type fileState struct { + Content []byte + Perm os.FileMode +} + +type directoryState map[string]fileState + +func (dir directoryState) Write(t *testing.T, path string) { + if err := os.MkdirAll(path, 0755); err != nil && !os.IsExist(err) { + t.Fatalf("Failed to create directory %q: %v", path, err) + } + + for filename, state := range dir { + fullFilename := filepath.Join(path, filename) + if err := os.WriteFile(fullFilename, state.Content, state.Perm); err != nil { + t.Fatalf("Failed to write file %q: %v", fullFilename, err) + } + } +} + +func (dir directoryState) CheckDirectoryMatches(t *testing.T, path string) { + entries, err := os.ReadDir(path) + if err != nil { + t.Fatalf("Failed to read directory %q: %v", path, err) + } + + expectedFiles := sets.KeySet(dir) + for _, entry := range entries { + // Mark the file as visited. + expectedFiles.Delete(entry.Name()) + + // Get the expected state. + state, ok := dir[entry.Name()] + if !ok { + t.Errorf("Directory %q contains unexpected file %q", path, entry.Name()) + continue + } + + // Check permissions. + info, err := entry.Info() + if err != nil { + t.Errorf("Failed to stat file %q: %v", entry.Name(), err) + continue + } + + if info.Mode() != state.Perm { + t.Errorf("Unexpected permissions on file %q: expected %v, got %v", entry.Name(), state.Perm, info.Mode()) + } + + // Check file content. + content, err := os.ReadFile(filepath.Join(path, entry.Name())) + if err != nil { + t.Errorf("Failed to read file %q: %v", entry.Name(), err) + continue + } + if !bytes.Equal(state.Content, content) { + t.Errorf("Unexpected content in file %q:\n%v", entry.Name(), cmp.Diff(string(state.Content), string(content))) + } + } + if expectedFiles.Len() != 0 { + t.Errorf("Some expected files were not found in directory %q: %s", path, expectedFiles.UnsortedList()) + } +} diff --git a/pkg/operator/staticpod/internal/atomicdir/swap_other.go b/pkg/operator/staticpod/internal/atomicdir/swap_other.go new file mode 100644 index 0000000000..55c54891c8 --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/swap_other.go @@ -0,0 +1,10 @@ +//go:build !linux + +package atomicdir + +// swap can be used to exchange two directories atomically. +// +// This function is only implemented for Linux and returns an error on other platforms. +func swap(firstDir, secondDir string) error { + return errors.New("swap is not supported on this platform") +} From 43445d06ff36e0612e0cfed4da7113eb113a703d Mon Sep 17 00:00:00 2001 From: Lukasz Szaszkiewicz Date: Thu, 2 Oct 2025 09:30:05 +0200 Subject: [PATCH 2/8] operator/staticpod/internal/atomicdir/swap_other: add missing import --- pkg/operator/staticpod/internal/atomicdir/swap_other.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/operator/staticpod/internal/atomicdir/swap_other.go b/pkg/operator/staticpod/internal/atomicdir/swap_other.go index 55c54891c8..6f5c0fed9e 100644 --- a/pkg/operator/staticpod/internal/atomicdir/swap_other.go +++ b/pkg/operator/staticpod/internal/atomicdir/swap_other.go @@ -2,6 +2,8 @@ package atomicdir +import "errors" + // swap can be used to exchange two directories atomically. // // This function is only implemented for Linux and returns an error on other platforms. From 56e39a38903287c29408786524c4c409965fa7bb Mon Sep 17 00:00:00 2001 From: Ondra Kupka Date: Wed, 1 Oct 2025 16:02:20 +0200 Subject: [PATCH 3/8] Add atomicdir.Sync function The function can be used to atomically sync a directory with the desired state. This uses atomicdir.swap implemented earlier. --- .../staticpod/internal/atomicdir/sync.go | 86 ++++ .../internal/atomicdir/sync_linux_test.go | 434 ++++++++++++++++++ 2 files changed, 520 insertions(+) create mode 100644 pkg/operator/staticpod/internal/atomicdir/sync.go create mode 100644 pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go diff --git a/pkg/operator/staticpod/internal/atomicdir/sync.go b/pkg/operator/staticpod/internal/atomicdir/sync.go new file mode 100644 index 0000000000..d24a5eb04d --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/sync.go @@ -0,0 +1,86 @@ +package atomicdir + +import ( + "fmt" + "os" + "path/filepath" + + "k8s.io/klog/v2" +) + +// Sync can be used to atomically synchronize target directory with the given file content map. +// This is done by populating a staging directory, then atomically swapping it with the target directory. +// This effectively means that any extra files in the target directory are pruned. +// +// The staging directory needs to be explicitly specified. It is initially created using os.MkdirAll with targetDirPerm. +// It is then populated using files with filePerm. Once the atomic swap is performed, the staging directory +// (which is now the original target directory) is removed. +func Sync(targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string][]byte, filePerm os.FileMode) error { + return sync(&realFS, targetDir, targetDirPerm, stagingDir, files, filePerm) +} + +type fileSystem struct { + MkdirAll func(path string, perm os.FileMode) error + RemoveAll func(path string) error + WriteFile func(name string, data []byte, perm os.FileMode) error + SwapDirectories func(dirA, dirB string) error +} + +var realFS = fileSystem{ + MkdirAll: os.MkdirAll, + RemoveAll: os.RemoveAll, + WriteFile: os.WriteFile, + SwapDirectories: swap, +} + +// sync prepares a tmp directory and writes all files into that directory. +// Then it atomically swap the tmp directory for the target one. +// This is currently implemented as really atomically swapping directories. +// +// The same goal of atomic swap could be implemented using symlinks much like AtomicWriter does in +// https://github.com/kubernetes/kubernetes/blob/v1.34.0/pkg/volume/util/atomic_writer.go#L58 +// The reason we don't do that is that we already have a directory populated and watched that needs to we swapped. +// In other words, it's for compatibility reasons. And if we were to migrate to the symlink approach, +// we would anyway need to atomically turn the current data directory into a symlink. +// This would all just increase complexity and require atomic swap on the OS level anyway. +func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string][]byte, filePerm os.FileMode) (retErr error) { + klog.Infof("Ensuring target directory %q exists ...", targetDir) + if err := fs.MkdirAll(targetDir, targetDirPerm); err != nil { + return fmt.Errorf("failed creating target directory: %w", err) + } + + klog.Infof("Creating staging directory to swap for %q ...", targetDir) + if err := fs.MkdirAll(stagingDir, targetDirPerm); err != nil { + return fmt.Errorf("failed creating staging directory: %w", err) + } + defer func() { + if err := fs.RemoveAll(stagingDir); err != nil { + if retErr != nil { + retErr = fmt.Errorf("failed removing staging directory %q: %w; previous error: %w", stagingDir, err, retErr) + return + } + retErr = fmt.Errorf("failed removing staging directory %q: %w", stagingDir, err) + } + }() + + for filename, content := range files { + // Make sure filename is a plain filename, not a path. + // This also ensures the staging directory cannot be escaped. + if filename != filepath.Base(filename) { + return fmt.Errorf("filename cannot be a path: %q", filename) + } + + fullFilename := filepath.Join(stagingDir, filename) + klog.Infof("Writing file %q ...", fullFilename) + + if err := fs.WriteFile(fullFilename, content, filePerm); err != nil { + return fmt.Errorf("failed writing %q: %w", fullFilename, err) + } + } + + klog.Infof("Atomically swapping target directory %q with staging directory %q ...", targetDir, stagingDir) + if err := fs.SwapDirectories(targetDir, stagingDir); err != nil { + return fmt.Errorf("failed swapping target directory %q with staging directory %q: %w", targetDir, stagingDir, err) + } + return +} diff --git a/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go new file mode 100644 index 0000000000..195ed1a5dc --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go @@ -0,0 +1,434 @@ +//go:build linux + +package atomicdir + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestSync(t *testing.T) { + newRealFS := func() *fileSystem { + fs := realFS + return &fs + } + + type testCase struct { + name string + // newFS is the main mocking factory for the test run. + newFS func() *fileSystem + // existingFiles is used to populate the target directory state before testing. + // An empty map will cause the directory to be created, a nil map will cause no directory to be created. + existingFiles map[string][]byte + // filesToSync will be synchronized into the target directory. + filesToSync map[string][]byte + // expectedFiles contains the files that are expected to be in the target directory after sync is called. + expectedFiles map[string][]byte + // expectSyncError check the return value from Sync. + expectSyncError bool + // expectLingeringStagingDirectory can be set to true to expect the temporary directory not to be removed. + expectLingeringStagingDirectory bool + } + + errorTestCase := func(name string, newFS func() *fileSystem) testCase { + return testCase{ + name: name, + newFS: newFS, + existingFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + filesToSync: map[string][]byte{ + "api.crt": []byte("rotated TLS cert"), + "api.key": []byte("rotated TLS key"), + }, + expectedFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + expectSyncError: true, + } + } + + testCases := []testCase{ + { + name: "target directory does not exist", + newFS: newRealFS, + existingFiles: nil, + filesToSync: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + expectedFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + }, + { + name: "target directory is empty", + newFS: newRealFS, + existingFiles: map[string][]byte{}, + filesToSync: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + expectedFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + }, + { + name: "target directory already synchronized", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + filesToSync: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + expectedFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + }, + { + name: "change file contents preserving the filenames", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + filesToSync: map[string][]byte{ + "tls.crt": []byte("rotated TLS cert"), + "tls.key": []byte("rotated TLS key"), + }, + expectedFiles: map[string][]byte{ + "tls.crt": []byte("rotated TLS cert"), + "tls.key": []byte("rotated TLS key"), + }, + }, + { + name: "change filenames preserving the file contents", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + filesToSync: map[string][]byte{ + "api.crt": []byte("TLS cert"), + "api.key": []byte("TLS key"), + }, + expectedFiles: map[string][]byte{ + "api.crt": []byte("TLS cert"), + "api.key": []byte("TLS key"), + }, + }, + { + name: "change filenames and file contents", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + filesToSync: map[string][]byte{ + "api.crt": []byte("rotated TLS cert"), + "api.key": []byte("rotated TLS key"), + }, + expectedFiles: map[string][]byte{ + "api.crt": []byte("rotated TLS cert"), + "api.key": []byte("rotated TLS key"), + }, + }, + { + name: "replace a single file content", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "1.txt": []byte("1"), + "2.txt": []byte("2"), + }, + filesToSync: map[string][]byte{ + "1.txt": []byte("1"), + "2.txt": []byte("3"), + }, + expectedFiles: map[string][]byte{ + "1.txt": []byte("1"), + "2.txt": []byte("3"), + }, + }, + { + name: "replace a single file", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "1.txt": []byte("1"), + "2.txt": []byte("2"), + }, + filesToSync: map[string][]byte{ + "1.txt": []byte("1"), + "3.txt": []byte("3"), + }, + expectedFiles: map[string][]byte{ + "1.txt": []byte("1"), + "3.txt": []byte("3"), + }, + }, + { + name: "rename a single file", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "1.txt": []byte("1"), + "2.txt": []byte("2"), + }, + filesToSync: map[string][]byte{ + "1.txt": []byte("1"), + "3.txt": []byte("2"), + }, + expectedFiles: map[string][]byte{ + "1.txt": []byte("1"), + "3.txt": []byte("2"), + }, + }, + { + name: "add new files", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + filesToSync: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + "another_tls.crt": []byte("another TLS cert"), + "another_tls.key": []byte("another TLS key"), + }, + expectedFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + "another_tls.crt": []byte("another TLS cert"), + "another_tls.key": []byte("another TLS key"), + }, + }, + { + name: "delete a single file", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "1.txt": []byte("1"), + "2.txt": []byte("2"), + }, + filesToSync: map[string][]byte{ + "1.txt": []byte("1"), + }, + expectedFiles: map[string][]byte{ + "1.txt": []byte("1"), + }, + }, + { + name: "delete all files", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "1.txt": []byte("1"), + "2.txt": []byte("2"), + }, + filesToSync: map[string][]byte{}, + expectedFiles: map[string][]byte{}, + }, + errorTestCase("directory unchanged on failed to create object directory", func() *fileSystem { + fs := newRealFS() + mkdirAll := fs.MkdirAll + fs.MkdirAll = func(path string, perm os.FileMode) error { + // Fail on the content dir. + if !strings.Contains(path, "/staging/") { + return errors.New("nuked") + } + return mkdirAll(path, perm) + } + return fs + }), + errorTestCase("directory unchanged on failed to create staging directory", func() *fileSystem { + fs := newRealFS() + mkdirAll := fs.MkdirAll + fs.MkdirAll = func(path string, perm os.FileMode) error { + // Fail on the staging dir. + if strings.Contains(path, "/staging/") { + return errors.New("nuked") + } + return mkdirAll(path, perm) + } + return fs + }), + errorTestCase("directory unchanged on failed to write the first file", func() *fileSystem { + fs := newRealFS() + fs.WriteFile = failToWriteNth(fs.WriteFile, 0) + return fs + }), + errorTestCase("directory unchanged on failed to write the second file", func() *fileSystem { + fs := newRealFS() + fs.WriteFile = failToWriteNth(fs.WriteFile, 1) + return fs + }), + errorTestCase("directory unchanged on failed to swap directories", func() *fileSystem { + fs := newRealFS() + fs.SwapDirectories = func(dirA, dirB string) error { + return errors.New("nuked") + } + return fs + }), + { + name: "directory synchronized then failing to remove temporary directory", + newFS: func() *fileSystem { + fs := newRealFS() + fs.RemoveAll = func(path string) error { + return errors.New("nuked") + } + return fs + }, + existingFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + filesToSync: map[string][]byte{ + "api.crt": []byte("rotated TLS cert"), + "api.key": []byte("rotated TLS key"), + }, + expectedFiles: map[string][]byte{ + "api.crt": []byte("rotated TLS cert"), + "api.key": []byte("rotated TLS key"), + }, + expectSyncError: true, + expectLingeringStagingDirectory: true, + }, + { + name: "invalid filename specified (relative path)", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + filesToSync: map[string][]byte{ + // This fails even though the actual resolved path is just "api.crt". + // We simply do not handle paths in any way, we expect filenames. + "home/../api.crt": []byte("rotated TLS cert"), + "api.key": []byte("rotated TLS key"), + }, + expectedFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + expectSyncError: true, + }, + { + name: "invalid filename specified (absolute path)", + newFS: newRealFS, + existingFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + filesToSync: map[string][]byte{ + "/api.crt": []byte("rotated TLS cert"), + "api.key": []byte("rotated TLS key"), + }, + expectedFiles: map[string][]byte{ + "tls.crt": []byte("TLS cert"), + "tls.key": []byte("TLS key"), + }, + expectSyncError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Write the current directory contents. + contentDir := filepath.Join(t.TempDir(), "secrets", "tls-cert") + if tc.existingFiles != nil { + if err := os.MkdirAll(contentDir, 0700); err != nil { + t.Fatalf("Failed to create content directory %q: %v", contentDir, err) + } + + for filename, content := range tc.existingFiles { + 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. + stagingDir := filepath.Join(t.TempDir(), "staging", "secrets", "tls-cert") + err := sync(tc.newFS(), contentDir, 0700, stagingDir, tc.filesToSync, 0600) + + // Check the resulting state. + checkDirectoryContents(t, contentDir, 0700, tc.expectedFiles, 0600) + + if (err != nil) != tc.expectSyncError { + t.Errorf("Expected error from sync = %v, got %v", tc.expectSyncError, err) + } + + if !tc.expectLingeringStagingDirectory { + // Note that staging/secrets is still there, though. Which is fine. + ensureDirectoryNotFound(t, stagingDir) + } + }) + } +} + +type writeFileFunc func(path string, data []byte, perm os.FileMode) error + +func failToWriteNth(writeFile writeFileFunc, n int) writeFileFunc { + var c int + return func(path string, data []byte, perm os.FileMode) error { + i := c + c++ + if i == n { + return errors.New("nuked") + } + return writeFile(path, data, perm) + } +} + +func checkDirectoryContents(t *testing.T, contentDir string, contentDirPerm os.FileMode, files map[string][]byte, filePerm os.FileMode) { + // Ensure the content directory permissions match. + stat, err := os.Stat(contentDir) + if err != nil { + t.Fatalf("Failed to stat %q: %v", contentDir, err) + } + if perm := stat.Mode().Perm(); perm != contentDirPerm { + t.Errorf("Permissions mismatch detected for %q: expected %v, got %v", contentDir, contentDirPerm, perm) + } + + // 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 != filePerm { + 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)) + } +} + +func ensureDirectoryNotFound(t *testing.T, path string) { + if _, stat := os.Stat(path); !os.IsNotExist(stat) { + t.Errorf("Directory %q should not exist", path) + } +} From 9e3c63391d451d2dc54adcf9c576d3e609323d68 Mon Sep 17 00:00:00 2001 From: Ondra Kupka Date: Wed, 15 Oct 2025 13:59:34 +0200 Subject: [PATCH 4/8] atomicdir: Set permission for each file separately --- .../internal/atomicdir/swap_linux_test.go | 97 +++-- .../staticpod/internal/atomicdir/sync.go | 16 +- .../internal/atomicdir/sync_linux_test.go | 337 ++++++++---------- 3 files changed, 201 insertions(+), 249 deletions(-) diff --git a/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go b/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go index ef4370180e..b2c0467eda 100644 --- a/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go +++ b/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go @@ -3,14 +3,11 @@ package atomicdir import ( - "bytes" "os" "path/filepath" "testing" "github.com/google/go-cmp/cmp" - - "k8s.io/apimachinery/pkg/util/sets" ) func TestSwap(t *testing.T) { @@ -44,8 +41,8 @@ func TestSwap(t *testing.T) { expectNoError(t, err) // Make sure the contents are swapped. - stateFirst.CheckDirectoryMatches(t, pathSecond) - stateSecond.CheckDirectoryMatches(t, pathFirst) + stateFirst.CheckDirectoryMatches(t, pathSecond, 0755) + stateSecond.CheckDirectoryMatches(t, pathFirst, 0755) } testCases := []struct { @@ -59,8 +56,8 @@ func TestSwap(t *testing.T) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") - stateFirst.Write(t, pathFirst) - stateSecond.Write(t, pathSecond) + stateFirst.Write(t, pathFirst, 0755) + stateSecond.Write(t, pathSecond, 0755) return pathFirst, stateFirst, pathSecond, stateSecond }, @@ -72,8 +69,8 @@ func TestSwap(t *testing.T) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") - stateFirst.Write(t, pathFirst) - stateSecond.Write(t, pathSecond) + stateFirst.Write(t, pathFirst, 0755) + stateSecond.Write(t, pathSecond, 0755) cwd, err := os.Getwd() expectNoError(t, err) @@ -91,8 +88,8 @@ func TestSwap(t *testing.T) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") - stateFirst.Write(t, pathFirst) - stateSecond.Write(t, pathSecond) + stateFirst.Write(t, pathFirst, 0755) + stateSecond.Write(t, pathSecond, 0755) cwd, err := os.Getwd() expectNoError(t, err) @@ -110,8 +107,8 @@ func TestSwap(t *testing.T) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") - stateFirst.Write(t, pathFirst) - stateEmpty.Write(t, pathSecond) + stateFirst.Write(t, pathFirst, 0755) + stateEmpty.Write(t, pathSecond, 0755) return pathFirst, stateFirst, pathSecond, stateEmpty }, @@ -123,8 +120,8 @@ func TestSwap(t *testing.T) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") - stateEmpty.Write(t, pathFirst) - stateEmpty.Write(t, pathSecond) + stateEmpty.Write(t, pathFirst, 0755) + stateEmpty.Write(t, pathSecond, 0755) return pathFirst, stateEmpty, pathSecond, stateEmpty }, @@ -186,66 +183,62 @@ func TestSwap(t *testing.T) { } } -type fileState struct { - Content []byte - Perm os.FileMode -} - -type directoryState map[string]fileState +type directoryState map[string]File -func (dir directoryState) Write(t *testing.T, path string) { - if err := os.MkdirAll(path, 0755); err != nil && !os.IsExist(err) { - t.Fatalf("Failed to create directory %q: %v", path, err) +func (dir directoryState) Write(tb testing.TB, dirPath string, dirPerm os.FileMode) { + if err := os.MkdirAll(dirPath, dirPerm); err != nil { + tb.Fatalf("Failed to create directory %q: %v", dirPath, err) } for filename, state := range dir { - fullFilename := filepath.Join(path, filename) + fullFilename := filepath.Join(dirPath, filename) if err := os.WriteFile(fullFilename, state.Content, state.Perm); err != nil { - t.Fatalf("Failed to write file %q: %v", fullFilename, err) + tb.Fatalf("Failed to write file %q: %v", fullFilename, err) } } } -func (dir directoryState) CheckDirectoryMatches(t *testing.T, path string) { - entries, err := os.ReadDir(path) +func (dir directoryState) CheckDirectoryMatches(tb testing.TB, dirPath string, dirPerm os.FileMode) { + // Ensure the directory permissions match. + stat, err := os.Stat(dirPath) if err != nil { - t.Fatalf("Failed to read directory %q: %v", path, err) + tb.Fatalf("Failed to stat %q: %v", dirPath, err) + } + if perm := stat.Mode().Perm(); perm != dirPerm { + tb.Errorf("Permissions mismatch detected for %q: expected %v, got %v", dirPath, dirPerm, perm) } - expectedFiles := sets.KeySet(dir) - for _, entry := range entries { - // Mark the file as visited. - expectedFiles.Delete(entry.Name()) + // Ensure all files are in sync. + entries, err := os.ReadDir(dirPath) + if err != nil { + tb.Fatalf("Failed to read directory %q: %v", dirPath, err) + } - // Get the expected state. - state, ok := dir[entry.Name()] - if !ok { - t.Errorf("Directory %q contains unexpected file %q", path, entry.Name()) - continue - } + actualState := make(directoryState, len(entries)) + for _, entry := range entries { + filePath := filepath.Join(dirPath, entry.Name()) - // Check permissions. info, err := entry.Info() if err != nil { - t.Errorf("Failed to stat file %q: %v", entry.Name(), err) - continue + tb.Fatalf("Failed to stat %q: %v", filePath, err) } - if info.Mode() != state.Perm { - t.Errorf("Unexpected permissions on file %q: expected %v, got %v", entry.Name(), state.Perm, info.Mode()) + if info.IsDir() { + tb.Errorf("Unexpected directory detected: %q", filePath) + continue } - // Check file content. - content, err := os.ReadFile(filepath.Join(path, entry.Name())) + content, err := os.ReadFile(filePath) if err != nil { - t.Errorf("Failed to read file %q: %v", entry.Name(), err) - continue + tb.Fatalf("Failed to read %q: %v", filePath, err) } - if !bytes.Equal(state.Content, content) { - t.Errorf("Unexpected content in file %q:\n%v", entry.Name(), cmp.Diff(string(state.Content), string(content))) + + actualState[entry.Name()] = File{ + Content: content, + Perm: info.Mode(), } } - if expectedFiles.Len() != 0 { - t.Errorf("Some expected files were not found in directory %q: %s", path, expectedFiles.UnsortedList()) + if !cmp.Equal(dir, actualState) { + tb.Errorf("Unexpected directory content for %q:\n%s\n", dirPath, cmp.Diff(dir, actualState)) } } diff --git a/pkg/operator/staticpod/internal/atomicdir/sync.go b/pkg/operator/staticpod/internal/atomicdir/sync.go index d24a5eb04d..e54f0a2345 100644 --- a/pkg/operator/staticpod/internal/atomicdir/sync.go +++ b/pkg/operator/staticpod/internal/atomicdir/sync.go @@ -8,6 +8,12 @@ import ( "k8s.io/klog/v2" ) +// File represents file content together with the associated permissions. +type File struct { + Content []byte + Perm os.FileMode +} + // Sync can be used to atomically synchronize target directory with the given file content map. // This is done by populating a staging directory, then atomically swapping it with the target directory. // This effectively means that any extra files in the target directory are pruned. @@ -15,8 +21,8 @@ import ( // The staging directory needs to be explicitly specified. It is initially created using os.MkdirAll with targetDirPerm. // It is then populated using files with filePerm. Once the atomic swap is performed, the staging directory // (which is now the original target directory) is removed. -func Sync(targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string][]byte, filePerm os.FileMode) error { - return sync(&realFS, targetDir, targetDirPerm, stagingDir, files, filePerm) +func Sync(targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]File) error { + return sync(&realFS, targetDir, targetDirPerm, stagingDir, files) } type fileSystem struct { @@ -43,7 +49,7 @@ var realFS = fileSystem{ // In other words, it's for compatibility reasons. And if we were to migrate to the symlink approach, // we would anyway need to atomically turn the current data directory into a symlink. // This would all just increase complexity and require atomic swap on the OS level anyway. -func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string][]byte, filePerm os.FileMode) (retErr error) { +func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]File) (retErr error) { klog.Infof("Ensuring target directory %q exists ...", targetDir) if err := fs.MkdirAll(targetDir, targetDirPerm); err != nil { return fmt.Errorf("failed creating target directory: %w", err) @@ -63,7 +69,7 @@ func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDi } }() - for filename, content := range files { + for filename, file := range files { // Make sure filename is a plain filename, not a path. // This also ensures the staging directory cannot be escaped. if filename != filepath.Base(filename) { @@ -73,7 +79,7 @@ func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDi fullFilename := filepath.Join(stagingDir, filename) klog.Infof("Writing file %q ...", fullFilename) - if err := fs.WriteFile(fullFilename, content, filePerm); err != nil { + if err := fs.WriteFile(fullFilename, file.Content, file.Perm); err != nil { return fmt.Errorf("failed writing %q: %w", fullFilename, err) } } diff --git a/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go index 195ed1a5dc..62eaecc1fc 100644 --- a/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go +++ b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go @@ -8,8 +8,6 @@ import ( "path/filepath" "strings" "testing" - - "github.com/google/go-cmp/cmp" ) func TestSync(t *testing.T) { @@ -24,11 +22,11 @@ func TestSync(t *testing.T) { newFS func() *fileSystem // existingFiles is used to populate the target directory state before testing. // An empty map will cause the directory to be created, a nil map will cause no directory to be created. - existingFiles map[string][]byte + existingFiles map[string]File // filesToSync will be synchronized into the target directory. - filesToSync map[string][]byte + filesToSync map[string]File // expectedFiles contains the files that are expected to be in the target directory after sync is called. - expectedFiles map[string][]byte + expectedFiles map[string]File // expectSyncError check the return value from Sync. expectSyncError bool // expectLingeringStagingDirectory can be set to true to expect the temporary directory not to be removed. @@ -39,17 +37,17 @@ func TestSync(t *testing.T) { return testCase{ name: name, newFS: newFS, - existingFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + existingFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "api.crt": []byte("rotated TLS cert"), - "api.key": []byte("rotated TLS key"), + filesToSync: map[string]File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + expectedFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, expectSyncError: true, } @@ -60,183 +58,183 @@ func TestSync(t *testing.T) { name: "target directory does not exist", newFS: newRealFS, existingFiles: nil, - filesToSync: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + filesToSync: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + expectedFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, }, { name: "target directory is empty", newFS: newRealFS, - existingFiles: map[string][]byte{}, - filesToSync: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + existingFiles: map[string]File{}, + filesToSync: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + expectedFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, }, { name: "target directory already synchronized", newFS: newRealFS, - existingFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + existingFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + filesToSync: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + expectedFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, }, { name: "change file contents preserving the filenames", newFS: newRealFS, - existingFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + existingFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "tls.crt": []byte("rotated TLS cert"), - "tls.key": []byte("rotated TLS key"), + filesToSync: map[string]File{ + "tls.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "tls.crt": []byte("rotated TLS cert"), - "tls.key": []byte("rotated TLS key"), + expectedFiles: map[string]File{ + "tls.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, }, { name: "change filenames preserving the file contents", newFS: newRealFS, - existingFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + existingFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "api.crt": []byte("TLS cert"), - "api.key": []byte("TLS key"), + filesToSync: map[string]File{ + "api.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "api.crt": []byte("TLS cert"), - "api.key": []byte("TLS key"), + expectedFiles: map[string]File{ + "api.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("TLS key"), Perm: 0600}, }, }, { name: "change filenames and file contents", newFS: newRealFS, - existingFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + existingFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "api.crt": []byte("rotated TLS cert"), - "api.key": []byte("rotated TLS key"), + filesToSync: map[string]File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "api.crt": []byte("rotated TLS cert"), - "api.key": []byte("rotated TLS key"), + expectedFiles: map[string]File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, }, { name: "replace a single file content", newFS: newRealFS, - existingFiles: map[string][]byte{ - "1.txt": []byte("1"), - "2.txt": []byte("2"), + existingFiles: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "1.txt": []byte("1"), - "2.txt": []byte("3"), + filesToSync: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("3"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "1.txt": []byte("1"), - "2.txt": []byte("3"), + expectedFiles: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("3"), Perm: 0600}, }, }, { name: "replace a single file", newFS: newRealFS, - existingFiles: map[string][]byte{ - "1.txt": []byte("1"), - "2.txt": []byte("2"), + existingFiles: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "1.txt": []byte("1"), - "3.txt": []byte("3"), + filesToSync: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "3.txt": {Content: []byte("3"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "1.txt": []byte("1"), - "3.txt": []byte("3"), + expectedFiles: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "3.txt": {Content: []byte("3"), Perm: 0600}, }, }, { name: "rename a single file", newFS: newRealFS, - existingFiles: map[string][]byte{ - "1.txt": []byte("1"), - "2.txt": []byte("2"), + existingFiles: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "1.txt": []byte("1"), - "3.txt": []byte("2"), + filesToSync: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "3.txt": {Content: []byte("2"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "1.txt": []byte("1"), - "3.txt": []byte("2"), + expectedFiles: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "3.txt": {Content: []byte("2"), Perm: 0600}, }, }, { name: "add new files", newFS: newRealFS, - existingFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), - }, - filesToSync: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), - "another_tls.crt": []byte("another TLS cert"), - "another_tls.key": []byte("another TLS key"), - }, - expectedFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), - "another_tls.crt": []byte("another TLS cert"), - "another_tls.key": []byte("another TLS key"), + existingFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + "another_tls.crt": {Content: []byte("another TLS cert"), Perm: 0600}, + "another_tls.key": {Content: []byte("another TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + "another_tls.crt": {Content: []byte("another TLS cert"), Perm: 0600}, + "another_tls.key": {Content: []byte("another TLS key"), Perm: 0600}, }, }, { name: "delete a single file", newFS: newRealFS, - existingFiles: map[string][]byte{ - "1.txt": []byte("1"), - "2.txt": []byte("2"), + existingFiles: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "1.txt": []byte("1"), + filesToSync: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "1.txt": []byte("1"), + expectedFiles: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, }, }, { name: "delete all files", newFS: newRealFS, - existingFiles: map[string][]byte{ - "1.txt": []byte("1"), - "2.txt": []byte("2"), + existingFiles: map[string]File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string][]byte{}, - expectedFiles: map[string][]byte{}, + filesToSync: map[string]File{}, + expectedFiles: map[string]File{}, }, errorTestCase("directory unchanged on failed to create object directory", func() *fileSystem { fs := newRealFS() @@ -288,17 +286,17 @@ func TestSync(t *testing.T) { } return fs }, - existingFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + existingFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "api.crt": []byte("rotated TLS cert"), - "api.key": []byte("rotated TLS key"), + filesToSync: map[string]File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "api.crt": []byte("rotated TLS cert"), - "api.key": []byte("rotated TLS key"), + expectedFiles: map[string]File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, expectSyncError: true, expectLingeringStagingDirectory: true, @@ -306,36 +304,36 @@ func TestSync(t *testing.T) { { name: "invalid filename specified (relative path)", newFS: newRealFS, - existingFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + existingFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string][]byte{ + filesToSync: map[string]File{ // This fails even though the actual resolved path is just "api.crt". // We simply do not handle paths in any way, we expect filenames. - "home/../api.crt": []byte("rotated TLS cert"), - "api.key": []byte("rotated TLS key"), + "home/../api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + expectedFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, expectSyncError: true, }, { name: "invalid filename specified (absolute path)", newFS: newRealFS, - existingFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + existingFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string][]byte{ - "/api.crt": []byte("rotated TLS cert"), - "api.key": []byte("rotated TLS key"), + filesToSync: map[string]File{ + "/api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string][]byte{ - "tls.crt": []byte("TLS cert"), - "tls.key": []byte("TLS key"), + expectedFiles: map[string]File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, expectSyncError: true, }, @@ -346,24 +344,15 @@ func TestSync(t *testing.T) { // Write the current directory contents. contentDir := filepath.Join(t.TempDir(), "secrets", "tls-cert") if tc.existingFiles != nil { - if err := os.MkdirAll(contentDir, 0700); err != nil { - t.Fatalf("Failed to create content directory %q: %v", contentDir, err) - } - - for filename, content := range tc.existingFiles { - targetPath := filepath.Join(contentDir, filename) - if err := os.WriteFile(targetPath, content, 0600); err != nil { - t.Fatalf("Failed to populate file %q: %v", targetPath, err) - } - } + directoryState(tc.existingFiles).Write(t, contentDir, 0755) } // Replace with the object data. stagingDir := filepath.Join(t.TempDir(), "staging", "secrets", "tls-cert") - err := sync(tc.newFS(), contentDir, 0700, stagingDir, tc.filesToSync, 0600) + err := sync(tc.newFS(), contentDir, 0755, stagingDir, tc.filesToSync) // Check the resulting state. - checkDirectoryContents(t, contentDir, 0700, tc.expectedFiles, 0600) + directoryState(tc.expectedFiles).CheckDirectoryMatches(t, contentDir, 0755) if (err != nil) != tc.expectSyncError { t.Errorf("Expected error from sync = %v, got %v", tc.expectSyncError, err) @@ -391,42 +380,6 @@ func failToWriteNth(writeFile writeFileFunc, n int) writeFileFunc { } } -func checkDirectoryContents(t *testing.T, contentDir string, contentDirPerm os.FileMode, files map[string][]byte, filePerm os.FileMode) { - // Ensure the content directory permissions match. - stat, err := os.Stat(contentDir) - if err != nil { - t.Fatalf("Failed to stat %q: %v", contentDir, err) - } - if perm := stat.Mode().Perm(); perm != contentDirPerm { - t.Errorf("Permissions mismatch detected for %q: expected %v, got %v", contentDir, contentDirPerm, perm) - } - - // 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 != filePerm { - 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)) - } -} - func ensureDirectoryNotFound(t *testing.T, path string) { if _, stat := os.Stat(path); !os.IsNotExist(stat) { t.Errorf("Directory %q should not exist", path) From 0210cf008cacfa866e1b4256fbc4b42ca22a6093 Mon Sep 17 00:00:00 2001 From: Ondra Kupka Date: Mon, 20 Oct 2025 10:31:09 +0200 Subject: [PATCH 5/8] atomicdir: Split testing utils into a helper pkg --- .../internal/atomicdir/swap_linux_test.go | 97 +++------------- .../staticpod/internal/atomicdir/sync.go | 12 +- .../internal/atomicdir/sync_linux_test.go | 109 +++++++++--------- .../atomicdir/testing/directory_state.go | 74 ++++++++++++ .../internal/atomicdir/types/file.go | 10 ++ 5 files changed, 164 insertions(+), 138 deletions(-) create mode 100644 pkg/operator/staticpod/internal/atomicdir/testing/directory_state.go create mode 100644 pkg/operator/staticpod/internal/atomicdir/types/file.go diff --git a/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go b/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go index b2c0467eda..a477b2fa45 100644 --- a/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go +++ b/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go @@ -7,11 +7,11 @@ import ( "path/filepath" "testing" - "github.com/google/go-cmp/cmp" + adtesting "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/testing" ) func TestSwap(t *testing.T) { - stateFirst := directoryState{ + stateFirst := adtesting.DirectoryState{ "1.txt": { Content: []byte("hello 1 world"), Perm: 0600, @@ -21,13 +21,13 @@ func TestSwap(t *testing.T) { Perm: 0400, }, } - stateSecond := directoryState{ + stateSecond := adtesting.DirectoryState{ "a.txt": { Content: []byte("hello a world"), Perm: 0600, }, } - stateEmpty := directoryState{} + stateEmpty := adtesting.DirectoryState{} expectNoError := func(t *testing.T, err error) { t.Helper() @@ -36,7 +36,7 @@ func TestSwap(t *testing.T) { } } - checkSuccess := func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, err error) { + checkSuccess := func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) { t.Helper() expectNoError(t, err) @@ -47,12 +47,12 @@ func TestSwap(t *testing.T) { testCases := []struct { name string - setup func(t *testing.T, tmpDir string) (pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState) - checkResult func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, err error) + setup func(t *testing.T, tmpDir string) (pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState) + checkResult func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) }{ { name: "success with absolute paths", - setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") @@ -65,7 +65,7 @@ func TestSwap(t *testing.T) { }, { name: "success with the first path relative", - setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") @@ -84,7 +84,7 @@ func TestSwap(t *testing.T) { }, { name: "success with the second path relative", - setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") @@ -103,7 +103,7 @@ func TestSwap(t *testing.T) { }, { name: "success with an empty directory", - setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") @@ -116,7 +116,7 @@ func TestSwap(t *testing.T) { }, { name: "success with both directories empty", - setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") @@ -129,7 +129,7 @@ func TestSwap(t *testing.T) { }, { name: "error with the first directory not existing", - setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") @@ -137,7 +137,7 @@ func TestSwap(t *testing.T) { return pathFirst, stateEmpty, pathSecond, stateEmpty }, - checkResult: func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, err error) { + checkResult: func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) { if !os.IsNotExist(err) { t.Errorf("Expected a directory not exists error, got %v", err) } @@ -145,7 +145,7 @@ func TestSwap(t *testing.T) { }, { name: "error with the second directory not existing", - setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") @@ -153,7 +153,7 @@ func TestSwap(t *testing.T) { return pathFirst, stateEmpty, pathSecond, stateEmpty }, - checkResult: func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, err error) { + checkResult: func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) { if !os.IsNotExist(err) { t.Errorf("Expected a directory not exists error, got %v", err) } @@ -161,13 +161,13 @@ func TestSwap(t *testing.T) { }, { name: "error with no directory existing", - setup: func(t *testing.T, tmpDir string) (string, directoryState, string, directoryState) { + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { pathFirst := filepath.Join(tmpDir, "first") pathSecond := filepath.Join(tmpDir, "second") return pathFirst, stateEmpty, pathSecond, stateEmpty }, - checkResult: func(t *testing.T, pathFirst string, stateFirst directoryState, pathSecond string, stateSecond directoryState, err error) { + checkResult: func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) { if !os.IsNotExist(err) { t.Errorf("Expected a directory not exists error, got %v", err) } @@ -177,68 +177,9 @@ func TestSwap(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() pathFirst, stateFirst, pathSecond, stateSecond := tc.setup(t, t.TempDir()) tc.checkResult(t, pathFirst, stateFirst, pathSecond, stateSecond, swap(pathFirst, pathSecond)) }) } } - -type directoryState map[string]File - -func (dir directoryState) Write(tb testing.TB, dirPath string, dirPerm os.FileMode) { - if err := os.MkdirAll(dirPath, dirPerm); err != nil { - tb.Fatalf("Failed to create directory %q: %v", dirPath, err) - } - - for filename, state := range dir { - fullFilename := filepath.Join(dirPath, filename) - if err := os.WriteFile(fullFilename, state.Content, state.Perm); err != nil { - tb.Fatalf("Failed to write file %q: %v", fullFilename, err) - } - } -} - -func (dir directoryState) CheckDirectoryMatches(tb testing.TB, dirPath string, dirPerm os.FileMode) { - // Ensure the directory permissions match. - stat, err := os.Stat(dirPath) - if err != nil { - tb.Fatalf("Failed to stat %q: %v", dirPath, err) - } - if perm := stat.Mode().Perm(); perm != dirPerm { - tb.Errorf("Permissions mismatch detected for %q: expected %v, got %v", dirPath, dirPerm, perm) - } - - // Ensure all files are in sync. - entries, err := os.ReadDir(dirPath) - if err != nil { - tb.Fatalf("Failed to read directory %q: %v", dirPath, err) - } - - actualState := make(directoryState, len(entries)) - for _, entry := range entries { - filePath := filepath.Join(dirPath, entry.Name()) - - info, err := entry.Info() - if err != nil { - tb.Fatalf("Failed to stat %q: %v", filePath, err) - } - - if info.IsDir() { - tb.Errorf("Unexpected directory detected: %q", filePath) - continue - } - - content, err := os.ReadFile(filePath) - if err != nil { - tb.Fatalf("Failed to read %q: %v", filePath, err) - } - - actualState[entry.Name()] = File{ - Content: content, - Perm: info.Mode(), - } - } - if !cmp.Equal(dir, actualState) { - tb.Errorf("Unexpected directory content for %q:\n%s\n", dirPath, cmp.Diff(dir, actualState)) - } -} diff --git a/pkg/operator/staticpod/internal/atomicdir/sync.go b/pkg/operator/staticpod/internal/atomicdir/sync.go index e54f0a2345..a56c961a33 100644 --- a/pkg/operator/staticpod/internal/atomicdir/sync.go +++ b/pkg/operator/staticpod/internal/atomicdir/sync.go @@ -6,13 +6,9 @@ import ( "path/filepath" "k8s.io/klog/v2" -) -// File represents file content together with the associated permissions. -type File struct { - Content []byte - Perm os.FileMode -} + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" +) // Sync can be used to atomically synchronize target directory with the given file content map. // This is done by populating a staging directory, then atomically swapping it with the target directory. @@ -21,7 +17,7 @@ type File struct { // The staging directory needs to be explicitly specified. It is initially created using os.MkdirAll with targetDirPerm. // It is then populated using files with filePerm. Once the atomic swap is performed, the staging directory // (which is now the original target directory) is removed. -func Sync(targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]File) error { +func Sync(targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]types.File) error { return sync(&realFS, targetDir, targetDirPerm, stagingDir, files) } @@ -49,7 +45,7 @@ var realFS = fileSystem{ // In other words, it's for compatibility reasons. And if we were to migrate to the symlink approach, // we would anyway need to atomically turn the current data directory into a symlink. // This would all just increase complexity and require atomic swap on the OS level anyway. -func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]File) (retErr error) { +func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]types.File) (retErr error) { klog.Infof("Ensuring target directory %q exists ...", targetDir) if err := fs.MkdirAll(targetDir, targetDirPerm); err != nil { return fmt.Errorf("failed creating target directory: %w", err) diff --git a/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go index 62eaecc1fc..30e8c89ecd 100644 --- a/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go +++ b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go @@ -8,6 +8,9 @@ import ( "path/filepath" "strings" "testing" + + adtesting "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/testing" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" ) func TestSync(t *testing.T) { @@ -22,11 +25,11 @@ func TestSync(t *testing.T) { newFS func() *fileSystem // existingFiles is used to populate the target directory state before testing. // An empty map will cause the directory to be created, a nil map will cause no directory to be created. - existingFiles map[string]File + existingFiles map[string]types.File // filesToSync will be synchronized into the target directory. - filesToSync map[string]File + filesToSync map[string]types.File // expectedFiles contains the files that are expected to be in the target directory after sync is called. - expectedFiles map[string]File + expectedFiles map[string]types.File // expectSyncError check the return value from Sync. expectSyncError bool // expectLingeringStagingDirectory can be set to true to expect the temporary directory not to be removed. @@ -37,15 +40,15 @@ func TestSync(t *testing.T) { return testCase{ name: name, newFS: newFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, @@ -58,11 +61,11 @@ func TestSync(t *testing.T) { name: "target directory does not exist", newFS: newRealFS, existingFiles: nil, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, @@ -70,12 +73,12 @@ func TestSync(t *testing.T) { { name: "target directory is empty", newFS: newRealFS, - existingFiles: map[string]File{}, - filesToSync: map[string]File{ + existingFiles: map[string]types.File{}, + filesToSync: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, @@ -83,15 +86,15 @@ func TestSync(t *testing.T) { { name: "target directory already synchronized", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, @@ -99,15 +102,15 @@ func TestSync(t *testing.T) { { name: "change file contents preserving the filenames", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "tls.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "tls.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, @@ -115,15 +118,15 @@ func TestSync(t *testing.T) { { name: "change filenames preserving the file contents", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "api.crt": {Content: []byte("TLS cert"), Perm: 0600}, "api.key": {Content: []byte("TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "api.crt": {Content: []byte("TLS cert"), Perm: 0600}, "api.key": {Content: []byte("TLS key"), Perm: 0600}, }, @@ -131,15 +134,15 @@ func TestSync(t *testing.T) { { name: "change filenames and file contents", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, @@ -147,15 +150,15 @@ func TestSync(t *testing.T) { { name: "replace a single file content", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "2.txt": {Content: []byte("3"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "2.txt": {Content: []byte("3"), Perm: 0600}, }, @@ -163,15 +166,15 @@ func TestSync(t *testing.T) { { name: "replace a single file", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "3.txt": {Content: []byte("3"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "3.txt": {Content: []byte("3"), Perm: 0600}, }, @@ -179,15 +182,15 @@ func TestSync(t *testing.T) { { name: "rename a single file", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "3.txt": {Content: []byte("2"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "3.txt": {Content: []byte("2"), Perm: 0600}, }, @@ -195,17 +198,17 @@ func TestSync(t *testing.T) { { name: "add new files", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, "another_tls.crt": {Content: []byte("another TLS cert"), Perm: 0600}, "another_tls.key": {Content: []byte("another TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, "another_tls.crt": {Content: []byte("another TLS cert"), Perm: 0600}, @@ -215,26 +218,26 @@ func TestSync(t *testing.T) { { name: "delete a single file", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, }, }, { name: "delete all files", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "1.txt": {Content: []byte("1"), Perm: 0600}, "2.txt": {Content: []byte("2"), Perm: 0600}, }, - filesToSync: map[string]File{}, - expectedFiles: map[string]File{}, + filesToSync: map[string]types.File{}, + expectedFiles: map[string]types.File{}, }, errorTestCase("directory unchanged on failed to create object directory", func() *fileSystem { fs := newRealFS() @@ -286,15 +289,15 @@ func TestSync(t *testing.T) { } return fs }, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, @@ -304,17 +307,17 @@ func TestSync(t *testing.T) { { name: "invalid filename specified (relative path)", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ // This fails even though the actual resolved path is just "api.crt". // We simply do not handle paths in any way, we expect filenames. "home/../api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, @@ -323,15 +326,15 @@ func TestSync(t *testing.T) { { name: "invalid filename specified (absolute path)", newFS: newRealFS, - existingFiles: map[string]File{ + existingFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, - filesToSync: map[string]File{ + filesToSync: map[string]types.File{ "/api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, }, - expectedFiles: map[string]File{ + expectedFiles: map[string]types.File{ "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, "tls.key": {Content: []byte("TLS key"), Perm: 0600}, }, @@ -341,10 +344,12 @@ func TestSync(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() + // Write the current directory contents. contentDir := filepath.Join(t.TempDir(), "secrets", "tls-cert") if tc.existingFiles != nil { - directoryState(tc.existingFiles).Write(t, contentDir, 0755) + adtesting.DirectoryState(tc.existingFiles).Write(t, contentDir, 0755) } // Replace with the object data. @@ -352,7 +357,7 @@ func TestSync(t *testing.T) { err := sync(tc.newFS(), contentDir, 0755, stagingDir, tc.filesToSync) // Check the resulting state. - directoryState(tc.expectedFiles).CheckDirectoryMatches(t, contentDir, 0755) + adtesting.DirectoryState(tc.expectedFiles).CheckDirectoryMatches(t, contentDir, 0755) if (err != nil) != tc.expectSyncError { t.Errorf("Expected error from sync = %v, got %v", tc.expectSyncError, err) diff --git a/pkg/operator/staticpod/internal/atomicdir/testing/directory_state.go b/pkg/operator/staticpod/internal/atomicdir/testing/directory_state.go new file mode 100644 index 0000000000..9312157383 --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/testing/directory_state.go @@ -0,0 +1,74 @@ +package testing + +import ( + "os" + "path/filepath" + "testing" + + "github.com/google/go-cmp/cmp" + + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" +) + +// DirectoryState can be used both bootstrap and check a directory state. +type DirectoryState map[string]types.File + +// Write writes the given state into the given directory. +func (dir DirectoryState) Write(tb testing.TB, dirPath string, dirPerm os.FileMode) { + if err := os.MkdirAll(dirPath, dirPerm); err != nil { + tb.Fatalf("Failed to create directory %q: %v", dirPath, err) + } + + for filename, state := range dir { + fullFilename := filepath.Join(dirPath, filename) + if err := os.WriteFile(fullFilename, state.Content, state.Perm); err != nil { + tb.Fatalf("Failed to write file %q: %v", fullFilename, err) + } + } +} + +// CheckDirectoryMatches checks the given directory against the given state. +func (dir DirectoryState) CheckDirectoryMatches(tb testing.TB, dirPath string, dirPerm os.FileMode) { + // Ensure the directory permissions match. + stat, err := os.Stat(dirPath) + if err != nil { + tb.Fatalf("Failed to stat %q: %v", dirPath, err) + } + if perm := stat.Mode().Perm(); perm != dirPerm { + tb.Errorf("Permissions mismatch detected for %q: expected %v, got %v", dirPath, dirPerm, perm) + } + + // Ensure all files are in sync. + entries, err := os.ReadDir(dirPath) + if err != nil { + tb.Fatalf("Failed to read directory %q: %v", dirPath, err) + } + + actualState := make(DirectoryState, len(entries)) + for _, entry := range entries { + filePath := filepath.Join(dirPath, entry.Name()) + + info, err := entry.Info() + if err != nil { + tb.Fatalf("Failed to stat %q: %v", filePath, err) + } + + if info.IsDir() { + tb.Errorf("Unexpected directory detected: %q", filePath) + continue + } + + content, err := os.ReadFile(filePath) + if err != nil { + tb.Fatalf("Failed to read %q: %v", filePath, err) + } + + actualState[entry.Name()] = types.File{ + Content: content, + Perm: info.Mode(), + } + } + if !cmp.Equal(dir, actualState) { + tb.Errorf("Unexpected directory content for %q:\n%s\n", dirPath, cmp.Diff(dir, actualState)) + } +} diff --git a/pkg/operator/staticpod/internal/atomicdir/types/file.go b/pkg/operator/staticpod/internal/atomicdir/types/file.go new file mode 100644 index 0000000000..f99f41c57c --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/types/file.go @@ -0,0 +1,10 @@ +// Package types exists to avoid import cycles as it's imported by both atomicdir and atomicdir/testing. +package types + +import "os" + +// File represents file content together with the associated permissions. +type File struct { + Content []byte + Perm os.FileMode +} From 95d966c9b9815d955da5a28f47667bee302959b9 Mon Sep 17 00:00:00 2001 From: Ondra Kupka Date: Thu, 16 Oct 2025 14:01:54 +0200 Subject: [PATCH 6/8] certsync+installer: Write object dirs atomically Use atomicdir.Sync to write target secret/configmap directories to be synchronized with the relevant objects. Added unit tests, but the coverage is not complete. Particularly filesystem operations failing are not being tested. --- .../certsyncpod/certsync_controller.go | 130 +-- .../certsync_controller_linux_test.go | 780 ++++++++++++++++++ pkg/operator/staticpod/installerpod/cmd.go | 103 ++- .../staticpod/installerpod/cmd_test.go | 90 +- .../staticpod/internal/atomicdir/sync.go | 4 +- 5 files changed, 970 insertions(+), 137 deletions(-) create mode 100644 pkg/operator/staticpod/certsyncpod/certsync_controller_linux_test.go diff --git a/pkg/operator/staticpod/certsyncpod/certsync_controller.go b/pkg/operator/staticpod/certsyncpod/certsync_controller.go index 111776d994..6911454be0 100644 --- a/pkg/operator/staticpod/certsyncpod/certsync_controller.go +++ b/pkg/operator/staticpod/certsyncpod/certsync_controller.go @@ -2,10 +2,12 @@ package certsyncpod import ( "context" + "fmt" "os" "path/filepath" "reflect" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilerrors "k8s.io/apimachinery/pkg/util/errors" @@ -17,17 +19,19 @@ import ( "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/staticpod" "github.com/openshift/library-go/pkg/operator/staticpod/controller/installer" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir" ) +const stagingDirUID = "cert-sync" + type CertSyncController struct { destinationDir string namespace string configMaps []installer.UnrevisionedResource secrets []installer.UnrevisionedResource - configmapGetter corev1interface.ConfigMapInterface + configMapGetter corev1interface.ConfigMapInterface configMapLister v1.ConfigMapLister secretGetter corev1interface.SecretInterface secretLister v1.SecretLister @@ -42,10 +46,10 @@ func NewCertSyncController(targetDir, targetNamespace string, configmaps, secret secrets: secrets, eventRecorder: eventRecorder.WithComponentSuffix("cert-sync-controller"), - configmapGetter: kubeClient.CoreV1().ConfigMaps(targetNamespace), + configMapGetter: kubeClient.CoreV1().ConfigMaps(targetNamespace), configMapLister: informers.Core().V1().ConfigMaps().Lister(), - secretLister: informers.Core().V1().Secrets().Lister(), secretGetter: kubeClient.CoreV1().Secrets(targetNamespace), + secretLister: informers.Core().V1().Secrets().Lister(), } return factory.New(). @@ -60,15 +64,12 @@ func NewCertSyncController(targetDir, targetNamespace string, configmaps, secret ) } -func getConfigMapDir(targetDir, configMapName string) string { - return filepath.Join(targetDir, "configmaps", configMapName) -} - -func getSecretDir(targetDir, secretName string) string { - return filepath.Join(targetDir, "secrets", secretName) -} - func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + if err := os.RemoveAll(getStagingDir(c.destinationDir)); err != nil { + c.eventRecorder.Warningf("CertificateUpdateFailed", fmt.Sprintf("Failed to prune staging directory: %v", err)) + return err + } + errors := []error{} klog.Infof("Syncing configmaps: %v", c.configMaps) @@ -80,7 +81,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue case apierrors.IsNotFound(err) && cm.Optional: - configMapFile := getConfigMapDir(c.destinationDir, cm.Name) + configMapFile := getConfigMapTargetDir(c.destinationDir, cm.Name) if _, err := os.Stat(configMapFile); os.IsNotExist(err) { // if the configmap file does not exist, there is no work to do, so skip making any live check and just return. // if the configmap actually exists in the API, we'll eventually see it on the watch. @@ -88,7 +89,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte } // Check with the live call it is really missing - configMap, err = c.configmapGetter.Get(ctx, cm.Name, metav1.GetOptions{}) + configMap, err = c.configMapGetter.Get(ctx, cm.Name, metav1.GetOptions{}) if err == nil { klog.Infof("Caches are stale. They don't see configmap '%s/%s', yet it is present", configMap.Namespace, configMap.Name) // We will get re-queued when we observe the change @@ -113,9 +114,10 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue } - contentDir := getConfigMapDir(c.destinationDir, cm.Name) + contentDir := getConfigMapTargetDir(c.destinationDir, cm.Name) + stagingDir := getConfigMapStagingDir(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) @@ -138,7 +140,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte klog.V(2).Infof("Syncing updated configmap '%s/%s'.", configMap.Namespace, configMap.Name) // We need to do a live get here so we don't overwrite a newer file with one from a stale cache - configMap, err = c.configmapGetter.Get(ctx, configMap.Name, metav1.GetOptions{}) + configMap, err = c.configMapGetter.Get(ctx, configMap.Name, metav1.GetOptions{}) if err != nil { // Even if the error is not exists we will act on it when caches catch up c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed getting configmap: %s/%s: %v", c.namespace, cm.Name, err) @@ -152,27 +154,11 @@ 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 - } + files := make(map[string][]byte, len(configMap.Data)) + for k, v := range configMap.Data { + files[k] = []byte(v) } - c.eventRecorder.Eventf("CertificateUpdated", "Wrote updated configmap: %s/%s", configMap.Namespace, configMap.Name) + errors = append(errors, syncDirectory(c.eventRecorder, "configmap", configMap.ObjectMeta, contentDir, 0755, stagingDir, files, 0644)) } klog.Infof("Syncing secrets: %v", c.secrets) @@ -184,7 +170,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue case apierrors.IsNotFound(err) && s.Optional: - secretFile := getSecretDir(c.destinationDir, s.Name) + secretFile := getSecretTargetDir(c.destinationDir, s.Name) if _, err := os.Stat(secretFile); os.IsNotExist(err) { // if the secret file does not exist, there is no work to do, so skip making any live check and just return. // if the secret actually exists in the API, we'll eventually see it on the watch. @@ -218,9 +204,10 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue } - contentDir := getSecretDir(c.destinationDir, s.Name) + contentDir := getSecretTargetDir(c.destinationDir, s.Name) + stagingDir := getSecretStagingDir(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 +244,50 @@ 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, 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 - } + errors = append(errors, syncDirectory(c.eventRecorder, "secret", secret.ObjectMeta, contentDir, 0755, stagingDir, secret.Data, 0600)) + } + return utilerrors.NewAggregate(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 - } +func syncDirectory( + eventRecorder events.Recorder, + typeName string, o metav1.ObjectMeta, + targetDir string, targetDirPerm os.FileMode, stagingDir string, + fileContents map[string][]byte, filePerm os.FileMode, +) error { + files := make(map[string]types.File, len(fileContents)) + for filename, content := range fileContents { + files[filename] = types.File{ + Content: content, + Perm: filePerm, } - c.eventRecorder.Eventf("CertificateUpdated", "Wrote updated secret: %s/%s", secret.Namespace, secret.Name) } - return utilerrors.NewAggregate(errors) + if err := atomicdir.Sync(targetDir, targetDirPerm, stagingDir, files); err != nil { + err = fmt.Errorf("failed to sync %s %s/%s (directory %q): %w", typeName, o.Name, o.Namespace, targetDir, err) + eventRecorder.Warning("CertificateUpdateFailed", err.Error()) + return err + } + eventRecorder.Eventf("CertificateUpdated", "Wrote updated %s: %s/%s", typeName, o.Namespace, o.Name) + return nil +} + +func getStagingDir(targetDir string) string { + return filepath.Join(targetDir, "staging", stagingDirUID) +} + +func getConfigMapTargetDir(targetDir, configMapName string) string { + return filepath.Join(targetDir, "configmaps", configMapName) +} + +func getConfigMapStagingDir(targetDir, configMapName string) string { + return filepath.Join(getStagingDir(targetDir), "configmaps", configMapName) +} + +func getSecretTargetDir(targetDir, secretName string) string { + return filepath.Join(targetDir, "secrets", secretName) +} + +func getSecretStagingDir(targetDir, secretName string) string { + return filepath.Join(getStagingDir(targetDir), "secrets", secretName) } diff --git a/pkg/operator/staticpod/certsyncpod/certsync_controller_linux_test.go b/pkg/operator/staticpod/certsyncpod/certsync_controller_linux_test.go new file mode 100644 index 0000000000..f4611d7fde --- /dev/null +++ b/pkg/operator/staticpod/certsyncpod/certsync_controller_linux_test.go @@ -0,0 +1,780 @@ +//go:build linux + +package certsyncpod + +import ( + "bytes" + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "fmt" + "math/big" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/apiserver/pkg/server/dynamiccertificates" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes/fake" + ktesting "k8s.io/client-go/testing" + clocktesting "k8s.io/utils/clock/testing" + + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/staticpod/controller/installer" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir" + adtesting "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/testing" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" +) + +const testingNamespace = "test-namespace" + +// TestCertSyncController_Sync tests the sync method in various scenarios. +func TestCertSyncController_Sync(t *testing.T) { + testCases := []struct { + name string + configMapsToSync []installer.UnrevisionedResource + configMapObjects []*corev1.ConfigMap + configMapGetErrors map[string]error + secretsToSync []installer.UnrevisionedResource + secretObjects []*corev1.Secret + secretGetErrors map[string]error + // Keys are paths relative to the controller destination directory. + existingDirectories map[string]adtesting.DirectoryState + expectedError bool + expectedEventTypes []string + // Keys are paths relative to the controller destination directory. + expectedDirectories map[string]adtesting.DirectoryState + }{ + { + name: "create first configmap", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config"}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config", map[string]string{ + "config.yaml": "test-config-content", + }), + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("test-config-content"), + Perm: 0644, + }, + }, + }, + }, + { + name: "add another configmap", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config-1"}, + {Name: "test-config-2"}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config-1", map[string]string{ + "config.yaml": "test-config-content-1", + }), + createConfigMap("test-config-2", map[string]string{ + "config.yaml": "test-config-content-2", + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config-1": { + "config.yaml": { + Content: []byte("test-config-content-1"), + Perm: 0644, + }, + }, + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config-1": { + "config.yaml": { + Content: []byte("test-config-content-1"), + Perm: 0644, + }, + }, + "configmaps/test-config-2": { + "config.yaml": { + Content: []byte("test-config-content-2"), + Perm: 0644, + }, + }, + }, + }, + { + name: "update existing configmap", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config", Optional: false}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config", map[string]string{ + "config.yaml": "updated-config-content", + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("test-config-content"), + Perm: 0644, + }, + }, + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("updated-config-content"), + Perm: 0644, + }, + }, + }, + }, + { + name: "succeed when optional configmap missing", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "optional-config", Optional: true}, + }, + }, + { + name: "fail when required configmap missing", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "required-config", Optional: false}, + }, + expectedError: true, + }, + { + name: "remove directory when optional configmap missing", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "optional-config", Optional: true}, + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/optional-config": { + "config.yaml": { + Content: []byte("optional-config-content"), + Perm: 0644, + }, + }, + }, + expectedEventTypes: []string{"CertificateRemoved"}, + }, + { + name: "configmap unchanged on get error", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "error-config"}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("error-config", map[string]string{ + "config.yaml": "updated-config-content", + }), + }, + configMapGetErrors: map[string]error{ + "error-config": apierrors.NewInternalError(fmt.Errorf("API server error")), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/error-config": { + "config.yaml": { + Content: []byte("error-config-content"), + Perm: 0644, + }, + }, + }, + expectedError: true, + expectedEventTypes: []string{"CertificateUpdateFailed"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/error-config": { + "config.yaml": { + Content: []byte("error-config-content"), + Perm: 0644, + }, + }, + }, + }, + + { + name: "create first secret", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret"}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret", map[string][]byte{ + "tls.crt": []byte("test-cert-content"), + "tls.key": []byte("test-key-content"), + }), + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + }, + { + name: "add another secret", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret-1"}, + {Name: "test-secret-2"}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret-1", map[string][]byte{ + "tls-1.crt": []byte("test-cert-content-1"), + "tls-1.key": []byte("test-key-content-1"), + }), + createSecret("test-secret-2", map[string][]byte{ + "tls-2.crt": []byte("test-cert-content-2"), + "tls-2.key": []byte("test-key-content-2"), + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret-1": { + "tls-1.crt": { + Content: []byte("test-cert-content-1"), + Perm: 0600, + }, + "tls-1.key": { + Content: []byte("test-key-content-1"), + Perm: 0600, + }, + }, + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret-1": { + "tls-1.crt": { + Content: []byte("test-cert-content-1"), + Perm: 0600, + }, + "tls-1.key": { + Content: []byte("test-key-content-1"), + Perm: 0600, + }, + }, + "secrets/test-secret-2": { + "tls-2.crt": { + Content: []byte("test-cert-content-2"), + Perm: 0600, + }, + "tls-2.key": { + Content: []byte("test-key-content-2"), + Perm: 0600, + }, + }, + }, + }, + { + name: "update existing secret", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret", Optional: true}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret", map[string][]byte{ + "tls.crt": []byte("updated-cert-content"), + "tls.key": []byte("updated-key-content"), + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret": { + "tls.crt": { + Content: []byte("updated-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("updated-key-content"), + Perm: 0600, + }, + }, + }, + }, + { + name: "succeed when optional secret missing", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "optional-secret", Optional: true}, + }, + }, + { + name: "fail when required secret missing", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "required-secret", Optional: false}, + }, + expectedError: true, + }, + { + name: "remove directory when optional secret missing", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "optional-secret", Optional: true}, + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "secrets/optional-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + expectedEventTypes: []string{"CertificateRemoved"}, + }, + { + name: "secret unchanged on get error", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "error-secret"}, + }, + secretObjects: []*corev1.Secret{ + createSecret("error-secret", map[string][]byte{ + "token": []byte("updated-secret-content"), + }), + }, + secretGetErrors: map[string]error{ + "error-secret": apierrors.NewInternalError(fmt.Errorf("API server error")), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "secrets/error-secret": { + "token": { + Content: []byte("error-config-content"), + Perm: 0600, + }, + }, + }, + expectedError: true, + expectedEventTypes: []string{"CertificateUpdateFailed"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "secrets/error-secret": { + "token": { + Content: []byte("error-config-content"), + Perm: 0600, + }, + }, + }, + }, + + { + name: "create multiple configmaps and secrets", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config-1", Optional: false}, + {Name: "test-config-2", Optional: true}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config-1", map[string]string{ + "app.yaml": "test-config-content-1", + }), + createConfigMap("test-config-2", map[string]string{ + "opt.yaml": "test-config-content-2", + }), + }, + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret-1", Optional: false}, + {Name: "test-secret-2", Optional: true}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret-1", map[string][]byte{ + "token": []byte("test-secret-content-1"), + }), + createSecret("test-secret-2", map[string][]byte{ + "key": []byte("test-secret-content-2"), + }), + }, + expectedEventTypes: []string{"CertificateUpdated", "CertificateUpdated", "CertificateUpdated", "CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config-1": { + "app.yaml": { + Content: []byte("test-config-content-1"), + Perm: 0644, + }, + }, + "configmaps/test-config-2": { + "opt.yaml": { + Content: []byte("test-config-content-2"), + Perm: 0644, + }, + }, + "secrets/test-secret-1": { + "token": { + Content: []byte("test-secret-content-1"), + Perm: 0600, + }, + }, + "secrets/test-secret-2": { + "key": { + Content: []byte("test-secret-content-2"), + Perm: 0600, + }, + }, + }, + }, + { + name: "already synchronized", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config"}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config", map[string]string{ + "config.yaml": "test-config-content", + }), + }, + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret"}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret", map[string][]byte{ + "tls.crt": []byte("test-cert-content"), + "tls.key": []byte("test-key-content"), + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("test-config-content"), + Perm: 0644, + }, + }, + "secrets/test-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + expectedEventTypes: nil, // No events when no changes. + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("test-config-content"), + Perm: 0644, + }, + }, + "secrets/test-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + controller, eventRecorder, stopCh := createController(t.TempDir(), + tc.configMapsToSync, tc.configMapObjects, tc.configMapGetErrors, + tc.secretsToSync, tc.secretObjects, tc.secretGetErrors, + ) + defer close(stopCh) + + for path, state := range tc.existingDirectories { + targetPath := filepath.Join(controller.destinationDir, path) + state.Write(t, targetPath, 0755) + } + + syncCtx := factory.NewSyncContext("CertSyncController", eventRecorder) + err := controller.sync(context.Background(), syncCtx) + if err != nil { + t.Log("sync returned an error:", err) + } + + if tc.expectedError && err == nil { + t.Errorf("Expected error but got none") + } else if !tc.expectedError && err != nil { + t.Errorf("Unexpected error: %v", err) + } + + verifyEvents(t, eventRecorder, tc.expectedEventTypes) + + // Check filesystem state. We need to gather all paths to know which are extra. + extraPaths := sets.NewString() + filepath.Walk(controller.destinationDir, func(path string, info os.FileInfo, err error) error { + if !info.IsDir() || + path == controller.destinationDir || + strings.HasSuffix(path, "/staging") || + strings.HasSuffix(path, "/staging/cert-sync") || + strings.HasSuffix(path, "/staging/cert-sync/secrets") || + strings.HasSuffix(path, "/staging/cert-sync/configmaps") || + path == filepath.Join(controller.destinationDir, "configmaps") || + path == filepath.Join(controller.destinationDir, "secrets") { + + return nil + } + + extraPaths.Insert(path) + return nil + }) + for path, state := range tc.expectedDirectories { + targetPath := filepath.Join(controller.destinationDir, path) + state.CheckDirectoryMatches(t, targetPath, 0755) + extraPaths.Delete(targetPath) + } + if extraPaths.Len() > 0 { + t.Errorf("Directories that should not be there detected: %v", extraPaths.List()) + } + }) + } +} + +func createController( + destinationDir string, + configMapsToSync []installer.UnrevisionedResource, + configMapObjects []*corev1.ConfigMap, + configMapGetErrors map[string]error, + secretsToSync []installer.UnrevisionedResource, + secretObjects []*corev1.Secret, + secretGetErrors map[string]error, +) (*CertSyncController, events.Recorder, chan struct{}) { + kubeObjects := make([]runtime.Object, 0) + for _, cm := range configMapObjects { + kubeObjects = append(kubeObjects, cm) + } + for _, secret := range secretObjects { + kubeObjects = append(kubeObjects, secret) + } + kubeClient := fake.NewClientset(kubeObjects...) + + if configMapGetErrors != nil { + kubeClient.PrependReactor("get", "configmaps", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) { + getAction := action.(ktesting.GetAction) + if err, exists := configMapGetErrors[getAction.GetName()]; exists { + return true, nil, err + } + return false, nil, nil + }) + } + + if secretGetErrors != nil { + kubeClient.PrependReactor("get", "secrets", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) { + getAction := action.(ktesting.GetAction) + if err, exists := secretGetErrors[getAction.GetName()]; exists { + return true, nil, err + } + return false, nil, nil + }) + } + + informers := informers.NewSharedInformerFactoryWithOptions(kubeClient, 1*time.Hour) + eventRecorder := events.NewInMemoryRecorder("CertSyncController", clocktesting.NewFakeClock(time.Now())) + + controller := &CertSyncController{ + destinationDir: destinationDir, + namespace: testingNamespace, + configMaps: configMapsToSync, + secrets: secretsToSync, + configMapGetter: kubeClient.CoreV1().ConfigMaps(testingNamespace), + configMapLister: informers.Core().V1().ConfigMaps().Lister(), + secretGetter: kubeClient.CoreV1().Secrets(testingNamespace), + secretLister: informers.Core().V1().Secrets().Lister(), + eventRecorder: eventRecorder, + } + + stopCh := make(chan struct{}) + informers.Start(stopCh) + informers.WaitForCacheSync(stopCh) + + return controller, eventRecorder, stopCh +} + +func verifyEvents(t *testing.T, eventRecorder events.Recorder, expectedEventTypes []string) { + var gotEventTypes []string + for _, event := range eventRecorder.(events.InMemoryRecorder).Events() { + gotEventTypes = append(gotEventTypes, event.Reason) + } + + if !cmp.Equal(gotEventTypes, expectedEventTypes) { + t.Errorf("Unexpected event types (-want, +got): \n%v", cmp.Diff(expectedEventTypes, gotEventTypes)) + } +} + +func createConfigMap(name string, data map[string]string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testingNamespace, + }, + Data: data, + } +} + +func createSecret(name string, data map[string][]byte) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testingNamespace, + }, + Data: data, + } +} + +// TestDynamicCertificates makes sure the receiving side of certificate synchronization works as expected. +// It reads and watches the certificates being synchronized in the same way as e.g. kube-apiserver, +// the very same libraries are being used. +func TestDynamicCertificates(t *testing.T) { + const typeName = "secret" + om := metav1.ObjectMeta{ + Namespace: "openshift-kube-apiserver", + Name: "s1", + } + + // Generate all necessary keypairs. + tlsCert, tlsKey := generateKeypair(t) + tlsCertUpdated, tlsKeyUpdated := generateKeypair(t) + + // Write the keypair into a secret directory. + secretDir := filepath.Join(t.TempDir(), "secrets", om.Name) + stagingDir := filepath.Join(t.TempDir(), "staging", stagingDirUID, "secrets", om.Name) + certFile := filepath.Join(secretDir, "tls.crt") + keyFile := filepath.Join(secretDir, "tls.key") + + if err := os.MkdirAll(secretDir, 0700); err != nil { + t.Fatalf("Failed to create secret directory %q: %v", secretDir, err) + } + if err := os.WriteFile(certFile, tlsCert, 0600); err != nil { + t.Fatalf("Failed to write TLS certificate into %q: %v", certFile, err) + } + if err := os.WriteFile(keyFile, tlsKey, 0600); err != nil { + t.Fatalf("Failed to write TLS key into %q: %v", keyFile, err) + } + + // Start the watcher. + // This reads the keypair synchronously so the initial state is loaded here. + dc, err := dynamiccertificates.NewDynamicServingContentFromFiles("localhost TLS", certFile, keyFile) + if err != nil { + t.Fatalf("Failed to init dynamic certificate: %v", err) + } + + // Check the initial keypair is loaded. + cert, key := dc.CurrentCertKeyContent() + if !bytes.Equal(cert, tlsCert) || !bytes.Equal(key, tlsKey) { + t.Fatal("Unexpected initial keypair loaded") + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + dc.Run(ctx, 1) + }() + defer wg.Wait() + defer cancel() + + // Poll until update detected. + files := map[string]types.File{ + "tls.crt": {Content: tlsCertUpdated, Perm: 0600}, + "tls.key": {Content: tlsKeyUpdated, Perm: 0600}, + } + err = wait.PollUntilContextCancel(ctx, 250*time.Millisecond, true, func(ctx context.Context) (bool, error) { + // Replace the secret directory. + if err := atomicdir.Sync(secretDir, 0700, stagingDir, files); err != nil { + t.Errorf("Failed to write files: %v", err) + return false, err + } + + // Check the loaded content matches. + // This is most probably updated based on write in a previous Poll invocation. + cert, key := dc.CurrentCertKeyContent() + return bytes.Equal(cert, tlsCertUpdated) && bytes.Equal(key, tlsKeyUpdated), nil + }) + if err != nil { + t.Fatalf("Failed to wait for dynamic certificate: %v", err) + } +} + +// generateKeypair returns (cert, key). +func generateKeypair(t *testing.T) ([]byte, []byte) { + t.Helper() + + privateKey, err := ecdsa.GenerateKey(elliptic.P224(), rand.Reader) + if err != nil { + t.Fatalf("Failed to generate TLS key: %v", err) + } + + notBefore := time.Now() + notAfter := notBefore.Add(1 * time.Hour) + + serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128) + serialNumber, err := rand.Int(rand.Reader, serialNumberLimit) + if err != nil { + t.Fatalf("Failed to generate serial number for TLS keypair: %v", err) + } + + template := x509.Certificate{ + SerialNumber: serialNumber, + Subject: pkix.Name{ + Organization: []string{"Example Org"}, + }, + NotBefore: notBefore, + NotAfter: notAfter, + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + DNSNames: []string{"example.com"}, + } + + publicKeyBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &privateKey.PublicKey, privateKey) + if err != nil { + t.Fatalf("Failed to create TLS certificate: %v", err) + } + + var certOut bytes.Buffer + if err := pem.Encode(&certOut, &pem.Block{Type: "CERTIFICATE", Bytes: publicKeyBytes}); err != nil { + t.Fatalf("Failed to write certificate PEM: %v", err) + } + + privateKeyBytes, err := x509.MarshalPKCS8PrivateKey(privateKey) + if err != nil { + t.Fatalf("Unable to marshal private key: %v", err) + } + + var keyOut bytes.Buffer + if err := pem.Encode(&keyOut, &pem.Block{Type: "PRIVATE KEY", Bytes: privateKeyBytes}); err != nil { + t.Fatalf("Failed to write certificate PEM: %v", err) + } + + return certOut.Bytes(), keyOut.Bytes() +} diff --git a/pkg/operator/staticpod/installerpod/cmd.go b/pkg/operator/staticpod/installerpod/cmd.go index 31afadff86..35424a56f5 100644 --- a/pkg/operator/staticpod/installerpod/cmd.go +++ b/pkg/operator/staticpod/installerpod/cmd.go @@ -5,38 +5,40 @@ import ( "fmt" "os" "path" + "path/filepath" "sort" "strconv" "strings" "time" - "k8s.io/utils/clock" - - "k8s.io/apimachinery/pkg/util/wait" - "github.com/blang/semver/v4" "github.com/davecgh/go-spew/spew" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" "github.com/spf13/cobra" "github.com/spf13/pflag" - "k8s.io/klog/v2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/uuid" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/server" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "k8s.io/klog/v2" + "k8s.io/utils/clock" "github.com/openshift/library-go/pkg/config/client" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/resource/resourceread" "github.com/openshift/library-go/pkg/operator/resource/retry" - "github.com/openshift/library-go/pkg/operator/staticpod" "github.com/openshift/library-go/pkg/operator/staticpod/internal" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir" "github.com/openshift/library-go/pkg/operator/staticpod/internal/flock" ) +const stagingDirUID = "installer" + type InstallOptions struct { // TODO replace with genericclioptions KubeConfig string @@ -219,8 +221,10 @@ func (o *InstallOptions) kubeletVersion(ctx context.Context) (string, error) { func (o *InstallOptions) copySecretsAndConfigMaps(ctx context.Context, resourceDir string, secretNames, optionalSecretNames, configNames, optionalConfigNames sets.Set[string], prefixed bool) error { - klog.Infof("Creating target resource directory %q ...", resourceDir) - if err := os.MkdirAll(resourceDir, 0755); err != nil && !os.IsExist(err) { + + stagingDirBase := getStagingDir(resourceDir) + klog.Infof("Pruning staging directory %q ...", stagingDirBase) + if err := os.RemoveAll(stagingDirBase); err != nil { return err } @@ -258,34 +262,43 @@ func (o *InstallOptions) copySecretsAndConfigMaps(ctx context.Context, resourceD if prefixed { secretBaseName = o.prefixFor(secret.Name) } - contentDir := path.Join(resourceDir, "secrets", secretBaseName) - klog.Infof("Creating directory %q ...", contentDir) - if err := os.MkdirAll(contentDir, 0755); err != nil { - return err - } - for filename, content := range secret.Data { - if err := writeSecret(content, path.Join(contentDir, filename)); err != nil { - return err + + contentDir := getSecretTargetDir(resourceDir, secretBaseName) + stagingDir := getSecretStagingDir(resourceDir, secretBaseName) + + files := make(map[string]types.File, len(secret.Data)) + for name, content := range secret.Data { + files[name] = types.File{ + Content: content, + Perm: getFilePermissionsSecret(name), } } + + if err := atomicdir.Sync(contentDir, 0755, stagingDir, files); err != nil { + return fmt.Errorf("failed to sync secret %s/%s (directory %q): %w", secret.Namespace, secret.Name, contentDir, err) + } } for _, configmap := range configs { configMapBaseName := configmap.Name if prefixed { configMapBaseName = o.prefixFor(configmap.Name) } - contentDir := path.Join(resourceDir, "configmaps", configMapBaseName) - klog.Infof("Creating directory %q ...", contentDir) - if err := os.MkdirAll(contentDir, 0755); err != nil { - return err - } - for filename, content := range configmap.Data { - if err := writeConfig([]byte(content), path.Join(contentDir, filename)); err != nil { - return err + + contentDir := getConfigMapTargetDir(resourceDir, configMapBaseName) + stagingDir := getConfigMapStagingDir(resourceDir, configMapBaseName) + + files := make(map[string]types.File, len(configmap.Data)) + for name, content := range configmap.Data { + files[name] = types.File{ + Content: []byte(content), + Perm: getFilePermissionsConfigMap(name), } } - } + if err := atomicdir.Sync(contentDir, 0755, stagingDir, files); err != nil { + return fmt.Errorf("failed to sync configmap %s/%s (directory %q): %w", configmap.Namespace, configmap.Name, contentDir, err) + } + } return nil } @@ -626,22 +639,36 @@ func (o *InstallOptions) writePod(rawPodBytes []byte, manifestFileName, resource return nil } -func writeConfig(content []byte, fullFilename string) error { - klog.Infof("Writing config file %q ...", fullFilename) +func getStagingDir(targetDir string) string { + return filepath.Join(targetDir, "staging", stagingDirUID) +} - filePerms := os.FileMode(0600) - if strings.HasSuffix(fullFilename, ".sh") { - filePerms = 0755 - } - return staticpod.WriteFileAtomic(content, filePerms, fullFilename) +func getConfigMapTargetDir(targetDir, configMapName string) string { + return filepath.Join(targetDir, "configmaps", configMapName) } -func writeSecret(content []byte, fullFilename string) error { - klog.Infof("Writing secret manifest %q ...", fullFilename) +func getConfigMapStagingDir(targetDir, configMapName string) string { + return filepath.Join(getStagingDir(targetDir), "configmaps", configMapName) +} + +func getSecretTargetDir(targetDir, secretName string) string { + return filepath.Join(targetDir, "secrets", secretName) +} + +func getSecretStagingDir(targetDir, secretName string) string { + return filepath.Join(getStagingDir(targetDir), "secrets", secretName) +} + +func getFilePermissionsConfigMap(filename string) os.FileMode { + if strings.HasSuffix(filename, ".sh") { + return 0755 + } + return 0600 +} - filePerms := os.FileMode(0600) - if strings.HasSuffix(fullFilename, ".sh") { - filePerms = 0700 +func getFilePermissionsSecret(filename string) os.FileMode { + if strings.HasSuffix(filename, ".sh") { + return 0700 } - return staticpod.WriteFileAtomic(content, filePerms, fullFilename) + return 0600 } diff --git a/pkg/operator/staticpod/installerpod/cmd_test.go b/pkg/operator/staticpod/installerpod/cmd_test.go index 5e6201b47b..cd046920ec 100644 --- a/pkg/operator/staticpod/installerpod/cmd_test.go +++ b/pkg/operator/staticpod/installerpod/cmd_test.go @@ -4,11 +4,11 @@ import ( "context" "os" "path" - "reflect" "strings" "testing" "time" + "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -51,8 +51,8 @@ func TestCopyContent(t *testing.T) { Revision: "006", Namespace: "some-ns", PodConfigMapNamePrefix: "kube-apiserver-pod", - SecretNamePrefixes: []string{"first", "second"}, - ConfigMapNamePrefixes: []string{"alpha", "bravo"}, + SecretNamePrefixes: []string{"first", "second", "third"}, + ConfigMapNamePrefixes: []string{"alpha", "bravo", "delta"}, }, client: func() *fake.Clientset { return fake.NewSimpleClientset( @@ -70,6 +70,12 @@ func TestCopyContent(t *testing.T) { "dos-B.crt": []byte("dos"), }, }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "some-ns", Name: "third-006"}, + Data: map[string][]byte{ + "run-third.sh": []byte("echo third"), + }, + }, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Namespace: "some-ns", Name: "alpha-006"}, Data: map[string]string{ @@ -90,23 +96,31 @@ func TestCopyContent(t *testing.T) { "pod.yaml": podYaml, }, }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: "some-ns", Name: "delta-006"}, + Data: map[string]string{ + "run-delta.sh": "echo delta", + }, + }, ) }, expected: func(t *testing.T, resourceDir, podDir string) { - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "one-A.crt"), "one") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "two-A.crt"), "two") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "uno-B.crt"), "uno") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "dos-B.crt"), "dos") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "apple-A.crt"), "apple") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "banana-A.crt"), "banana") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "manzana-B.crt"), "manzana") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "platano-B.crt"), "platano") + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "one-A.crt"), "one", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "two-A.crt"), "two", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "uno-B.crt"), "uno", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "dos-B.crt"), "dos", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "run-third.sh"), "echo third", 0700) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "apple-A.crt"), "apple", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "banana-A.crt"), "banana", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "manzana-B.crt"), "manzana", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "platano-B.crt"), "platano", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "delta", "run-delta.sh"), "echo delta", 0755) checkFileContentMatchesPod(t, path.Join(resourceDir, "kube-apiserver-pod-006", "kube-apiserver-pod.yaml"), podYaml) checkFileContentMatchesPod(t, path.Join(podDir, "kube-apiserver-pod.yaml"), podYaml) }, }, { - name: "optional-secrets-confmaps", + name: "optional-secrets-configmaps", o: InstallOptions{ Revision: "006", Namespace: "some-ns", @@ -169,18 +183,18 @@ func TestCopyContent(t *testing.T) { ) }, expected: func(t *testing.T, resourceDir, podDir string) { - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "one-A.crt"), "one") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "two-A.crt"), "two") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "uno-B.crt"), "uno") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "dos-B.crt"), "dos") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "tres-C.crt"), "tres") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "cuatro-C.crt"), "cuatro") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "apple-A.crt"), "apple") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "banana-A.crt"), "banana") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "manzana-B.crt"), "manzana") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "platano-B.crt"), "platano") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "charlie", "apple-C.crt"), "apple") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "charlie", "banana-C.crt"), "banana") + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "one-A.crt"), "one", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "two-A.crt"), "two", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "uno-B.crt"), "uno", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "dos-B.crt"), "dos", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "tres-C.crt"), "tres", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "cuatro-C.crt"), "cuatro", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "apple-A.crt"), "apple", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "banana-A.crt"), "banana", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "manzana-B.crt"), "manzana", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "platano-B.crt"), "platano", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "charlie", "apple-C.crt"), "apple", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "charlie", "banana-C.crt"), "banana", 0600) checkFileContentMatchesPod(t, path.Join(resourceDir, "kube-apiserver-pod-006", "kube-apiserver-pod.yaml"), podYaml) checkFileContentMatchesPod(t, path.Join(podDir, "kube-apiserver-pod.yaml"), podYaml) }, @@ -215,13 +229,7 @@ func TestCopyContent(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - testDir, err := os.MkdirTemp("", "copy-content-test") - if err != nil { - t.Fatal(err) - } - defer func() { - os.Remove(testDir) - }() + testDir := t.TempDir() o := test.o o.KubeClient = test.client() @@ -230,7 +238,7 @@ func TestCopyContent(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - err = o.copyContent(ctx) + err := o.copyContent(ctx) switch { case err == nil && len(test.expectedErr) == 0: case err != nil && len(test.expectedErr) == 0: @@ -258,15 +266,25 @@ func TestKubeletVersion(t *testing.T) { } } -func checkFileContent(t *testing.T, file, expected string) { - actual, err := os.ReadFile(file) +func checkFileContent(t *testing.T, file, expected string, expectedPerm os.FileMode) { + actualBytes, err := os.ReadFile(file) if err != nil { t.Error(err) return } + actual := string(actualBytes) + + stat, err := os.Stat(file) + if err != nil { + t.Errorf("Failed to stat %q: %v", file, err) + return + } + if gotPerm := stat.Mode().Perm(); gotPerm != expectedPerm { + t.Errorf("File permissions mismatch for %q: expected %v, got %v", file, gotPerm, expectedPerm) + } - if !reflect.DeepEqual(expected, string(actual)) { - t.Errorf("%q: expected %q, got %q", file, expected, string(actual)) + if !cmp.Equal(expected, actual) { + t.Errorf("File content mismatch for %q:\n%s", file, cmp.Diff(expected, actual)) } } diff --git a/pkg/operator/staticpod/internal/atomicdir/sync.go b/pkg/operator/staticpod/internal/atomicdir/sync.go index a56c961a33..0c2cc3dfff 100644 --- a/pkg/operator/staticpod/internal/atomicdir/sync.go +++ b/pkg/operator/staticpod/internal/atomicdir/sync.go @@ -51,7 +51,7 @@ func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDi return fmt.Errorf("failed creating target directory: %w", err) } - klog.Infof("Creating staging directory to swap for %q ...", targetDir) + klog.Infof("Creating staging directory %q ...", stagingDir) if err := fs.MkdirAll(stagingDir, targetDirPerm); err != nil { return fmt.Errorf("failed creating staging directory: %w", err) } @@ -80,7 +80,7 @@ func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDi } } - klog.Infof("Atomically swapping target directory %q with staging directory %q ...", targetDir, stagingDir) + klog.Infof("Atomically swapping staging directory %q with target directory %q ...", stagingDir, targetDir) if err := fs.SwapDirectories(targetDir, stagingDir); err != nil { return fmt.Errorf("failed swapping target directory %q with staging directory %q: %w", targetDir, stagingDir, err) } From a1cfead5234a786b3f84de0a177bb9e081884447 Mon Sep 17 00:00:00 2001 From: Luis Sanchez Date: Thu, 30 Apr 2026 08:17:26 -0400 Subject: [PATCH 7/8] atomicdir: fsync files and directories for crash durability atomicdir.Sync writes files to a staging directory, atomically swaps it with the target directory via renameat2(RENAME_EXCHANGE), then deletes the old data. Without fsync, file data lives only in the kernel page cache. On ungraceful shutdown the journal replays the swap and deletion (metadata), but the file data was never flushed, leaving truncated or empty files. Introduce an fsutil package with WriteFileFsync (write + fsync file + fsync parent directory) and Fsync (fsync a path) primitives. Use WriteFileFsync for all file writes so each file is individually durable, and fsync both parent directories after the swap to persist which inode each directory name points to. --- .../staticpod/internal/atomicdir/sync.go | 48 +++++--- .../internal/atomicdir/sync_linux_test.go | 62 +++++++++- .../staticpod/internal/fsutil/fsutil.go | 33 +++++ .../staticpod/internal/fsutil/fsutil_test.go | 113 ++++++++++++++++++ 4 files changed, 236 insertions(+), 20 deletions(-) create mode 100644 pkg/operator/staticpod/internal/fsutil/fsutil.go create mode 100644 pkg/operator/staticpod/internal/fsutil/fsutil_test.go diff --git a/pkg/operator/staticpod/internal/atomicdir/sync.go b/pkg/operator/staticpod/internal/atomicdir/sync.go index 0c2cc3dfff..779eb21288 100644 --- a/pkg/operator/staticpod/internal/atomicdir/sync.go +++ b/pkg/operator/staticpod/internal/atomicdir/sync.go @@ -8,15 +8,14 @@ import ( "k8s.io/klog/v2" "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/fsutil" ) -// Sync can be used to atomically synchronize target directory with the given file content map. -// This is done by populating a staging directory, then atomically swapping it with the target directory. -// This effectively means that any extra files in the target directory are pruned. -// -// The staging directory needs to be explicitly specified. It is initially created using os.MkdirAll with targetDirPerm. -// It is then populated using files with filePerm. Once the atomic swap is performed, the staging directory -// (which is now the original target directory) is removed. +// Sync atomically and durably replaces the contents of targetDir with the given files. +// It writes files to a staging directory with fsync, then atomically swaps it with +// the target directory via renameat2(RENAME_EXCHANGE), fsyncing parent directories +// to ensure the swap is persisted. Extra files in targetDir are pruned. +// The old target directory (now at stagingDir) is removed after the swap. func Sync(targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]types.File) error { return sync(&realFS, targetDir, targetDirPerm, stagingDir, files) } @@ -24,27 +23,29 @@ func Sync(targetDir string, targetDirPerm os.FileMode, stagingDir string, files type fileSystem struct { MkdirAll func(path string, perm os.FileMode) error RemoveAll func(path string) error - WriteFile func(name string, data []byte, perm os.FileMode) error + WriteFileFsync func(name string, data []byte, perm os.FileMode) error SwapDirectories func(dirA, dirB string) error + Fsync func(name string) error } var realFS = fileSystem{ MkdirAll: os.MkdirAll, RemoveAll: os.RemoveAll, - WriteFile: os.WriteFile, + WriteFileFsync: fsutil.WriteFileFsync, SwapDirectories: swap, + Fsync: fsutil.Fsync, } -// sync prepares a tmp directory and writes all files into that directory. -// Then it atomically swap the tmp directory for the target one. -// This is currently implemented as really atomically swapping directories. +// sync writes files into the staging directory, then durably swaps it with the target. +// Each file write is individually fsynced (including its parent directory entry) via +// fs.WriteFileFsync. After the swap, parent directories are fsynced to persist the exchange. // -// The same goal of atomic swap could be implemented using symlinks much like AtomicWriter does in +// Note: the upstream Kubernetes AtomicWriter uses symlinks for atomic updates but does +// not fsync, leaving file data in the page cache with no crash durability guarantee: // https://github.com/kubernetes/kubernetes/blob/v1.34.0/pkg/volume/util/atomic_writer.go#L58 -// The reason we don't do that is that we already have a directory populated and watched that needs to we swapped. -// In other words, it's for compatibility reasons. And if we were to migrate to the symlink approach, -// we would anyway need to atomically turn the current data directory into a symlink. -// This would all just increase complexity and require atomic swap on the OS level anyway. +// We use renameat2(RENAME_EXCHANGE) instead of symlinks because we need to swap an +// existing directory that is already being watched. Migrating to symlinks would still +// require an atomic swap at the OS level, adding complexity for no benefit. func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]types.File) (retErr error) { klog.Infof("Ensuring target directory %q exists ...", targetDir) if err := fs.MkdirAll(targetDir, targetDirPerm); err != nil { @@ -75,7 +76,7 @@ func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDi fullFilename := filepath.Join(stagingDir, filename) klog.Infof("Writing file %q ...", fullFilename) - if err := fs.WriteFile(fullFilename, file.Content, file.Perm); err != nil { + if err := fs.WriteFileFsync(fullFilename, file.Content, file.Perm); err != nil { return fmt.Errorf("failed writing %q: %w", fullFilename, err) } } @@ -84,5 +85,16 @@ func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDi if err := fs.SwapDirectories(targetDir, stagingDir); err != nil { return fmt.Errorf("failed swapping target directory %q with staging directory %q: %w", targetDir, stagingDir, err) } + + // fsync parent directories to ensure the swap is durable on disk. + // renameat2(RENAME_EXCHANGE) modifies parent directory entries, so the + // parents must be fsynced to persist which inode each name points to. + if err := fs.Fsync(filepath.Dir(targetDir)); err != nil { + return fmt.Errorf("failed syncing parent directory of %q: %w", targetDir, err) + } + if err := fs.Fsync(filepath.Dir(stagingDir)); err != nil { + return fmt.Errorf("failed syncing parent directory of %q: %w", stagingDir, err) + } + return } diff --git a/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go index 30e8c89ecd..24f1a9542a 100644 --- a/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go +++ b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go @@ -265,12 +265,12 @@ func TestSync(t *testing.T) { }), errorTestCase("directory unchanged on failed to write the first file", func() *fileSystem { fs := newRealFS() - fs.WriteFile = failToWriteNth(fs.WriteFile, 0) + fs.WriteFileFsync = failToWriteNth(fs.WriteFileFsync, 0) return fs }), errorTestCase("directory unchanged on failed to write the second file", func() *fileSystem { fs := newRealFS() - fs.WriteFile = failToWriteNth(fs.WriteFile, 1) + fs.WriteFileFsync = failToWriteNth(fs.WriteFileFsync, 1) return fs }), errorTestCase("directory unchanged on failed to swap directories", func() *fileSystem { @@ -280,6 +280,64 @@ func TestSync(t *testing.T) { } return fs }), + { + name: "directory synchronized on failed to sync parent of target directory", + newFS: func() *fileSystem { + fs := newRealFS() + origFsync := realFS.Fsync + var fsyncCount int + fs.Fsync = func(name string) error { + fsyncCount++ + if fsyncCount == 1 { + return errors.New("nuked") + } + return origFsync(name) + } + return fs + }, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectSyncError: true, + }, + { + name: "directory synchronized on failed to sync parent of staging directory", + newFS: func() *fileSystem { + fs := newRealFS() + origFsync := realFS.Fsync + var fsyncCount int + fs.Fsync = func(name string) error { + fsyncCount++ + if fsyncCount == 2 { + return errors.New("nuked") + } + return origFsync(name) + } + return fs + }, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectSyncError: true, + }, { name: "directory synchronized then failing to remove temporary directory", newFS: func() *fileSystem { diff --git a/pkg/operator/staticpod/internal/fsutil/fsutil.go b/pkg/operator/staticpod/internal/fsutil/fsutil.go new file mode 100644 index 0000000000..5b205f52db --- /dev/null +++ b/pkg/operator/staticpod/internal/fsutil/fsutil.go @@ -0,0 +1,33 @@ +package fsutil + +import ( + "os" + "path/filepath" +) + +// Fsync fsyncs a file or directory to ensure it is durable on disk. +func Fsync(name string) error { + f, err := os.Open(name) + if err != nil { + return err + } + syncErr := f.Sync() + // close(2) can surface errors not caught by fsync(2), e.g. on NFS. + closeErr := f.Close() + if syncErr != nil { + return syncErr + } + return closeErr +} + +// WriteFileFsync writes data to a file and fsyncs both the file and its +// parent directory to ensure the write is durable on disk. +func WriteFileFsync(name string, data []byte, perm os.FileMode) error { + if err := os.WriteFile(name, data, perm); err != nil { + return err + } + if err := Fsync(name); err != nil { + return err + } + return Fsync(filepath.Dir(name)) +} diff --git a/pkg/operator/staticpod/internal/fsutil/fsutil_test.go b/pkg/operator/staticpod/internal/fsutil/fsutil_test.go new file mode 100644 index 0000000000..f21d3b91d1 --- /dev/null +++ b/pkg/operator/staticpod/internal/fsutil/fsutil_test.go @@ -0,0 +1,113 @@ +package fsutil + +import ( + "os" + "path/filepath" + "testing" +) + +func TestWriteFileFsync(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T) string + data []byte + perm os.FileMode + expectError bool + }{ + { + name: "writes file with correct content and permissions", + setup: func(t *testing.T) string { + return filepath.Join(t.TempDir(), "test.txt") + }, + data: []byte("hello world"), + perm: 0600, + }, + { + name: "creates file in nonexistent directory fails", + setup: func(t *testing.T) string { + return filepath.Join(t.TempDir(), "nodir", "test.txt") + }, + data: []byte("hello"), + perm: 0600, + expectError: true, + }, + { + name: "overwrites existing file", + setup: func(t *testing.T) string { + p := filepath.Join(t.TempDir(), "test.txt") + if err := os.WriteFile(p, []byte("old content"), 0600); err != nil { + t.Fatal(err) + } + return p + }, + data: []byte("new content"), + perm: 0600, + }, + { + name: "writes empty file", + setup: func(t *testing.T) string { + return filepath.Join(t.TempDir(), "empty.txt") + }, + data: []byte{}, + perm: 0644, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + path := tc.setup(t) + err := WriteFileFsync(path, tc.data, tc.perm) + + if tc.expectError { + if err == nil { + t.Fatal("expected error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read back file: %v", err) + } + if string(got) != string(tc.data) { + t.Errorf("content mismatch: got %q, want %q", got, tc.data) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("failed to stat file: %v", err) + } + if info.Mode().Perm() != tc.perm { + t.Errorf("permission mismatch: got %o, want %o", info.Mode().Perm(), tc.perm) + } + }) + } +} + +func TestFsync(t *testing.T) { + t.Run("syncs existing file", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "test.txt") + if err := os.WriteFile(path, []byte("data"), 0600); err != nil { + t.Fatal(err) + } + if err := Fsync(path); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("syncs existing directory", func(t *testing.T) { + dir := t.TempDir() + if err := Fsync(dir); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("fails on nonexistent path", func(t *testing.T) { + if err := Fsync(filepath.Join(t.TempDir(), "nonexistent")); err == nil { + t.Fatal("expected error, got nil") + } + }) +} From 700c01c9e3dd896e1a9149a6d7842c0d6bb07908 Mon Sep 17 00:00:00 2001 From: Luis Sanchez Date: Thu, 30 Apr 2026 08:17:34 -0400 Subject: [PATCH 8/8] installerpod: fsync pod manifest writes for crash durability writePod uses bare os.WriteFile plus a delete-then-write pattern for kubelet manifests. On ungraceful shutdown, the delete is journaled but the new file data may not have reached disk, leaving the manifest missing. Replace os.WriteFile with fsutil.WriteFileFsync, which writes, fsyncs the file, and fsyncs the parent directory in a single call, ensuring both the resource copy and the kubelet manifest are durable before the function returns. --- pkg/operator/staticpod/installerpod/cmd.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/operator/staticpod/installerpod/cmd.go b/pkg/operator/staticpod/installerpod/cmd.go index 35424a56f5..68666ff196 100644 --- a/pkg/operator/staticpod/installerpod/cmd.go +++ b/pkg/operator/staticpod/installerpod/cmd.go @@ -35,6 +35,7 @@ import ( "github.com/openshift/library-go/pkg/operator/staticpod/internal" "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir" "github.com/openshift/library-go/pkg/operator/staticpod/internal/flock" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/fsutil" ) const stagingDirUID = "installer" @@ -622,8 +623,8 @@ func (o *InstallOptions) writePod(rawPodBytes []byte, manifestFileName, resource // Write secrets, config maps and pod to disk // This does not need timeout, instead we should fail hard when we are not able to write. klog.Infof("Writing pod manifest %q ...", path.Join(resourceDir, manifestFileName)) - if err := os.WriteFile(path.Join(resourceDir, manifestFileName), []byte(finalPodBytes), 0600); err != nil { - return err + if err := fsutil.WriteFileFsync(path.Join(resourceDir, manifestFileName), []byte(finalPodBytes), 0600); err != nil { + return fmt.Errorf("failed writing %q: %w", path.Join(resourceDir, manifestFileName), err) } // remove the existing file to ensure kubelet gets "create" event from inotify watchers @@ -633,8 +634,8 @@ func (o *InstallOptions) writePod(rawPodBytes []byte, manifestFileName, resource return err } klog.Infof("Writing static pod manifest %q ...\n%s", path.Join(o.PodManifestDir, manifestFileName), finalPodBytes) - if err := os.WriteFile(path.Join(o.PodManifestDir, manifestFileName), []byte(finalPodBytes), 0600); err != nil { - return err + if err := fsutil.WriteFileFsync(path.Join(o.PodManifestDir, manifestFileName), []byte(finalPodBytes), 0600); err != nil { + return fmt.Errorf("failed writing %q: %w", path.Join(o.PodManifestDir, manifestFileName), err) } return nil }