From 8b018a5091b5d59adb3d2e2457e12415a39418b5 Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Tue, 2 Aug 2022 12:40:46 -0400 Subject: [PATCH 1/9] break CopyFile into separate functions --- updater/internal/action/file_action.go | 2 +- updater/internal/file/file.go | 91 ++++++++++++++-------- updater/internal/install/install.go | 4 +- updater/internal/rollback/rollback.go | 4 +- updater/internal/service/service_darwin.go | 2 +- 5 files changed, 63 insertions(+), 40 deletions(-) diff --git a/updater/internal/action/file_action.go b/updater/internal/action/file_action.go index ddf548656..9f65de006 100644 --- a/updater/internal/action/file_action.go +++ b/updater/internal/action/file_action.go @@ -75,7 +75,7 @@ func (c CopyFileAction) Rollback() error { // join the relative path to the backup directory to get the location of the backup path backupFilePath := filepath.Join(c.backupDir, c.FromPathRel) - if err := file.CopyFile(c.logger.Named("copy-file"), backupFilePath, c.ToPath, true, true); err != nil { + if err := file.CopyFileRollback(c.logger.Named("copy-file"), backupFilePath, c.ToPath); err != nil { return fmt.Errorf("failed to copy file: %w", err) } diff --git a/updater/internal/file/file.go b/updater/internal/file/file.go index e27d64ea9..f9bf6c643 100644 --- a/updater/internal/file/file.go +++ b/updater/internal/file/file.go @@ -15,6 +15,7 @@ package file import ( + "errors" "fmt" "io" "io/fs" @@ -25,64 +26,87 @@ import ( ) // CopyFile copies the file from pathIn to pathOut. -// If the file does not exist, it is created. If the file does exist, it is truncated before writing. -func CopyFile(logger *zap.Logger, pathIn, pathOut string, overwrite bool, useInFilePermBackup bool) error { - pathInClean := filepath.Clean(pathIn) - - // Open the input file for reading. - inFile, err := os.Open(pathInClean) - if err != nil { - return fmt.Errorf("failed to open input file: %w", err) - } - defer func() { - err := inFile.Close() - if err != nil { - logger.Info("Failed to close input file", zap.Error(err)) - } - }() +// if overwrite is true, and the file does not exist, it is created. If the file does exist, it is deleted before writing. +func CopyFile(logger *zap.Logger, pathIn, pathOut string, overwrite bool) error { - pathOutClean := filepath.Clean(pathOut) fileMode := fs.FileMode(0600) flags := os.O_CREATE | os.O_WRONLY if overwrite { - // If we are OK to overwrite, we will truncate the file on open - flags |= os.O_TRUNC - - // Try to save old file's permissions + pathOutClean := filepath.Clean(pathOut) + // Try to save existing file's permissions outFileInfo, _ := os.Stat(pathOutClean) if outFileInfo != nil { fileMode = outFileInfo.Mode() - } else if useInFilePermBackup { - // Use the new file's permissions as a backup and don't fail on error (best chance for rollback) - inFileInfo, err := inFile.Stat() - switch { - case err != nil: - logger.Error("failed to retrieve fileinfo for input file", zap.Error(err)) - case inFileInfo != nil: - fileMode = inFileInfo.Mode() - } } // Remove old file to prevent issues with mac - if err = os.Remove(pathOutClean); err != nil { + if err := os.Remove(pathOutClean); err != nil { logger.Debug("Failed to remove output file", zap.Error(err)) } } else { + pathInClean := filepath.Clean(pathIn) // This flag will make OpenFile error if the file already exists flags |= os.O_EXCL // Use the new file's permissions and fail if there's an issue (want to fail for backup) - inFileInfo, err := inFile.Stat() + inFileInfo, err := os.Stat(pathInClean) if err != nil { - return fmt.Errorf("failed to retrive fileinfo for input file: %w", err) + return fmt.Errorf("failed to retrieve fileinfo for input file: %w", err) } fileMode = inFileInfo.Mode() } + return copyFileInternal(logger, pathIn, pathOut, flags, fileMode) +} + +// CopyFileRollback copies the file to the file from pathIn to pathOut, preserving the input file's mode if possible +// Used to perform a rollback +func CopyFileRollback(logger *zap.Logger, pathIn, pathOut string) error { + + // Default to 0600 if we can't determine the input file's mode + fileMode := fs.FileMode(0600) + pathInClean := filepath.Clean(pathIn) + // Use the backup file's permissions as a backup and don't fail on error (best chance for rollback) + inFileInfo, err := os.Stat(pathInClean) + switch { + case errors.Is(err, os.ErrNotExist): + return fmt.Errorf("input file does not exist: %w", err) + case err != nil: + logger.Error("failed to retrieve fileinfo for input file", zap.Error(err)) + default: + fileMode = inFileInfo.Mode() + } + + pathOutClean := filepath.Clean(pathOut) + // Remove old file to prevent issues with mac + if err = os.Remove(pathOutClean); err != nil { + logger.Debug("Failed to remove output file", zap.Error(err)) + } + + return copyFileInternal(logger, pathIn, pathOut, os.O_CREATE|os.O_WRONLY, fileMode) +} + +// copyFileInternal copies the file at pathIn to pathOut, using the provided flags and file mode +func copyFileInternal(logger *zap.Logger, pathIn, pathOut string, outFlags int, outMode fs.FileMode) error { + pathInClean := filepath.Clean(pathIn) + + // Open the input file for reading. + inFile, err := os.Open(pathInClean) + if err != nil { + return fmt.Errorf("failed to open input file: %w", err) + } + defer func() { + err := inFile.Close() + if err != nil { + logger.Info("Failed to close input file", zap.Error(err)) + } + }() + + pathOutClean := filepath.Clean(pathOut) // Open the output file, creating it if it does not exist and truncating it. //#nosec G304 -- out file is cleaned; this is a general purpose copy function - outFile, err := os.OpenFile(pathOutClean, flags, fileMode) + outFile, err := os.OpenFile(pathOutClean, outFlags, outMode) if err != nil { return fmt.Errorf("failed to open output file: %w", err) } @@ -97,6 +121,5 @@ func CopyFile(logger *zap.Logger, pathIn, pathOut string, overwrite bool, useInF if _, err := io.Copy(outFile, inFile); err != nil { return fmt.Errorf("failed to copy file: %w", err) } - return nil } diff --git a/updater/internal/install/install.go b/updater/internal/install/install.go index cb1b57662..bd19def82 100644 --- a/updater/internal/install/install.go +++ b/updater/internal/install/install.go @@ -134,7 +134,7 @@ func installFiles(logger *zap.Logger, inputPath, installDir, backupDir string, r // and we will want to roll that back if that is the case. rb.AppendAction(cfa) - if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, true, false); err != nil { + if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, true); err != nil { return fmt.Errorf("failed to copy file: %w", err) } @@ -195,7 +195,7 @@ func installFile(logger *zap.Logger, inPath, installDirPath, backupDirPath strin // and we will want to roll that back if that is the case. rb.AppendAction(cfa) - if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, true, false); err != nil { + if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, true); err != nil { return fmt.Errorf("failed to copy file: %w", err) } diff --git a/updater/internal/rollback/rollback.go b/updater/internal/rollback/rollback.go index c21fa1ede..df6d8cab7 100644 --- a/updater/internal/rollback/rollback.go +++ b/updater/internal/rollback/rollback.go @@ -159,7 +159,7 @@ func backupFiles(logger *zap.Logger, installDir, outputPath string) error { } // Fail if copying the input file to the output file would fail - if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, false, false); err != nil { + if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, false); err != nil { return fmt.Errorf("failed to copy file: %w", err) } @@ -187,7 +187,7 @@ func backupFile(logger *zap.Logger, inPath, outputDirPath string) error { } // Fail if copying the input file to the output file would fail - if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, false, false); err != nil { + if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, false); err != nil { return fmt.Errorf("failed to copy file: %w", err) } diff --git a/updater/internal/service/service_darwin.go b/updater/internal/service/service_darwin.go index badd8d9b3..908b97ef7 100644 --- a/updater/internal/service/service_darwin.go +++ b/updater/internal/service/service_darwin.go @@ -140,7 +140,7 @@ func (d darwinService) Update() error { } func (d darwinService) Backup() error { - if err := file.CopyFile(d.logger.Named("copy-file"), d.installedServiceFilePath, path.BackupServiceFile(d.installDir), false, false); err != nil { + if err := file.CopyFile(d.logger.Named("copy-file"), d.installedServiceFilePath, path.BackupServiceFile(d.installDir), false); err != nil { return fmt.Errorf("failed to copy service file: %w", err) } From c261962728e2a818ab556900fa53728c3e15bf7b Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Tue, 2 Aug 2022 12:54:50 -0400 Subject: [PATCH 2/9] break overwrite flag into two functions --- updater/internal/file/file.go | 54 +++++++++++----------- updater/internal/install/install.go | 4 +- updater/internal/rollback/rollback.go | 4 +- updater/internal/service/service_darwin.go | 2 +- updater/internal/service/service_linux.go | 2 +- 5 files changed, 33 insertions(+), 33 deletions(-) diff --git a/updater/internal/file/file.go b/updater/internal/file/file.go index f9bf6c643..5723c1c2a 100644 --- a/updater/internal/file/file.go +++ b/updater/internal/file/file.go @@ -26,44 +26,44 @@ import ( ) // CopyFile copies the file from pathIn to pathOut. -// if overwrite is true, and the file does not exist, it is created. If the file does exist, it is deleted before writing. -func CopyFile(logger *zap.Logger, pathIn, pathOut string, overwrite bool) error { - +// The file is created if it does not exist. If the file does exist, it is removed, then re-written, preserving the file's mode. +func CopyFileOverwrite(logger *zap.Logger, pathIn, pathOut string) error { fileMode := fs.FileMode(0600) - flags := os.O_CREATE | os.O_WRONLY - if overwrite { - pathOutClean := filepath.Clean(pathOut) - // Try to save existing file's permissions - outFileInfo, _ := os.Stat(pathOutClean) - if outFileInfo != nil { - fileMode = outFileInfo.Mode() - } + pathOutClean := filepath.Clean(pathOut) - // Remove old file to prevent issues with mac - if err := os.Remove(pathOutClean); err != nil { - logger.Debug("Failed to remove output file", zap.Error(err)) - } - } else { - pathInClean := filepath.Clean(pathIn) - // This flag will make OpenFile error if the file already exists - flags |= os.O_EXCL + // Try to save existing file's permissions + outFileInfo, _ := os.Stat(pathOutClean) + if outFileInfo != nil { + fileMode = outFileInfo.Mode() + } - // Use the new file's permissions and fail if there's an issue (want to fail for backup) - inFileInfo, err := os.Stat(pathInClean) - if err != nil { - return fmt.Errorf("failed to retrieve fileinfo for input file: %w", err) - } + // Remove old file to prevent issues with mac + if err := os.Remove(pathOutClean); err != nil { + logger.Debug("Failed to remove output file", zap.Error(err)) + } - fileMode = inFileInfo.Mode() + return copyFileInternal(logger, pathIn, pathOut, os.O_CREATE|os.O_WRONLY, fileMode) +} + +// CopyFileNoOverwrite copies the file from pathIn to pathOut, preserving the input files mode. +// If the output file already exists, this function returns an error. +func CopyFileNoOverwrite(logger *zap.Logger, pathIn, pathOut string) error { + pathInClean := filepath.Clean(pathIn) + + // Use the new file's permissions and fail if there's an issue (want to fail for backup) + inFileInfo, err := os.Stat(pathInClean) + if err != nil { + return fmt.Errorf("failed to retrieve fileinfo for input file: %w", err) } - return copyFileInternal(logger, pathIn, pathOut, flags, fileMode) + // the os.O_EXCL flag will make OpenFile error if the file already exists + return copyFileInternal(logger, pathIn, pathOut, os.O_EXCL|os.O_CREATE|os.O_WRONLY, inFileInfo.Mode()) + } // CopyFileRollback copies the file to the file from pathIn to pathOut, preserving the input file's mode if possible // Used to perform a rollback func CopyFileRollback(logger *zap.Logger, pathIn, pathOut string) error { - // Default to 0600 if we can't determine the input file's mode fileMode := fs.FileMode(0600) pathInClean := filepath.Clean(pathIn) diff --git a/updater/internal/install/install.go b/updater/internal/install/install.go index bd19def82..c016a2a55 100644 --- a/updater/internal/install/install.go +++ b/updater/internal/install/install.go @@ -134,7 +134,7 @@ func installFiles(logger *zap.Logger, inputPath, installDir, backupDir string, r // and we will want to roll that back if that is the case. rb.AppendAction(cfa) - if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, true); err != nil { + if err := file.CopyFileOverwrite(logger.Named("copy-file"), inPath, outPath); err != nil { return fmt.Errorf("failed to copy file: %w", err) } @@ -195,7 +195,7 @@ func installFile(logger *zap.Logger, inPath, installDirPath, backupDirPath strin // and we will want to roll that back if that is the case. rb.AppendAction(cfa) - if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, true); err != nil { + if err := file.CopyFileOverwrite(logger.Named("copy-file"), inPath, outPath); err != nil { return fmt.Errorf("failed to copy file: %w", err) } diff --git a/updater/internal/rollback/rollback.go b/updater/internal/rollback/rollback.go index df6d8cab7..9fe544c9f 100644 --- a/updater/internal/rollback/rollback.go +++ b/updater/internal/rollback/rollback.go @@ -159,7 +159,7 @@ func backupFiles(logger *zap.Logger, installDir, outputPath string) error { } // Fail if copying the input file to the output file would fail - if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, false); err != nil { + if err := file.CopyFileNoOverwrite(logger.Named("copy-file"), inPath, outPath); err != nil { return fmt.Errorf("failed to copy file: %w", err) } @@ -187,7 +187,7 @@ func backupFile(logger *zap.Logger, inPath, outputDirPath string) error { } // Fail if copying the input file to the output file would fail - if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, false); err != nil { + if err := file.CopyFileNoOverwrite(logger.Named("copy-file"), inPath, outPath); err != nil { return fmt.Errorf("failed to copy file: %w", err) } diff --git a/updater/internal/service/service_darwin.go b/updater/internal/service/service_darwin.go index 908b97ef7..3b07bbb6b 100644 --- a/updater/internal/service/service_darwin.go +++ b/updater/internal/service/service_darwin.go @@ -140,7 +140,7 @@ func (d darwinService) Update() error { } func (d darwinService) Backup() error { - if err := file.CopyFile(d.logger.Named("copy-file"), d.installedServiceFilePath, path.BackupServiceFile(d.installDir), false); err != nil { + if err := file.CopyFileNoOverwrite(d.logger.Named("copy-file"), d.installedServiceFilePath, path.BackupServiceFile(d.installDir)); err != nil { return fmt.Errorf("failed to copy service file: %w", err) } diff --git a/updater/internal/service/service_linux.go b/updater/internal/service/service_linux.go index cd271e7ff..452e1d47a 100644 --- a/updater/internal/service/service_linux.go +++ b/updater/internal/service/service_linux.go @@ -165,7 +165,7 @@ func (l linuxService) Update() error { } func (l linuxService) Backup() error { - if err := file.CopyFile(l.logger.Named("copy-file"), l.installedServiceFilePath, path.BackupServiceFile(l.installDir), false, false); err != nil { + if err := file.CopyFileNoOverwrite(l.logger.Named("copy-file"), l.installedServiceFilePath, path.BackupServiceFile(l.installDir), false, false); err != nil { return fmt.Errorf("failed to copy service file: %w", err) } From 225496bda8b10b158e7383ebec82dcf89f5e241e Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Tue, 2 Aug 2022 12:56:16 -0400 Subject: [PATCH 3/9] fix comment for CopyFileOverwrite --- updater/internal/file/file.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/updater/internal/file/file.go b/updater/internal/file/file.go index 5723c1c2a..c9c02e738 100644 --- a/updater/internal/file/file.go +++ b/updater/internal/file/file.go @@ -25,7 +25,7 @@ import ( "go.uber.org/zap" ) -// CopyFile copies the file from pathIn to pathOut. +// CopyFileOverwrite copies the file from pathIn to pathOut. // The file is created if it does not exist. If the file does exist, it is removed, then re-written, preserving the file's mode. func CopyFileOverwrite(logger *zap.Logger, pathIn, pathOut string) error { fileMode := fs.FileMode(0600) From 74e9d2a8f0721796e77586283ddfbe391be8210c Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Tue, 2 Aug 2022 13:24:50 -0400 Subject: [PATCH 4/9] small tweaks --- updater/internal/file/file.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/updater/internal/file/file.go b/updater/internal/file/file.go index c9c02e738..37efa74d6 100644 --- a/updater/internal/file/file.go +++ b/updater/internal/file/file.go @@ -26,7 +26,8 @@ import ( ) // CopyFileOverwrite copies the file from pathIn to pathOut. -// The file is created if it does not exist. If the file does exist, it is removed, then re-written, preserving the file's mode. +// The output file is created if it does not exist. +// If the output file does exist, it is removed, then written from the input file, preserving the output file's mode. func CopyFileOverwrite(logger *zap.Logger, pathIn, pathOut string) error { fileMode := fs.FileMode(0600) pathOutClean := filepath.Clean(pathOut) @@ -45,7 +46,7 @@ func CopyFileOverwrite(logger *zap.Logger, pathIn, pathOut string) error { return copyFileInternal(logger, pathIn, pathOut, os.O_CREATE|os.O_WRONLY, fileMode) } -// CopyFileNoOverwrite copies the file from pathIn to pathOut, preserving the input files mode. +// CopyFileNoOverwrite copies the file from pathIn to pathOut, preserving the input file's mode. // If the output file already exists, this function returns an error. func CopyFileNoOverwrite(logger *zap.Logger, pathIn, pathOut string) error { pathInClean := filepath.Clean(pathIn) @@ -58,7 +59,6 @@ func CopyFileNoOverwrite(logger *zap.Logger, pathIn, pathOut string) error { // the os.O_EXCL flag will make OpenFile error if the file already exists return copyFileInternal(logger, pathIn, pathOut, os.O_EXCL|os.O_CREATE|os.O_WRONLY, inFileInfo.Mode()) - } // CopyFileRollback copies the file to the file from pathIn to pathOut, preserving the input file's mode if possible @@ -99,7 +99,7 @@ func copyFileInternal(logger *zap.Logger, pathIn, pathOut string, outFlags int, defer func() { err := inFile.Close() if err != nil { - logger.Info("Failed to close input file", zap.Error(err)) + logger.Error("Failed to close input file", zap.Error(err)) } }() @@ -113,7 +113,7 @@ func copyFileInternal(logger *zap.Logger, pathIn, pathOut string, outFlags int, defer func() { err := outFile.Close() if err != nil { - logger.Info("Failed to close output file", zap.Error(err)) + logger.Error("Failed to close output file", zap.Error(err)) } }() From bedb323126c09f19b663ae70caf8711465089d98 Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Tue, 2 Aug 2022 13:45:29 -0400 Subject: [PATCH 5/9] tests for file package --- updater/internal/file/file.go | 10 +++ updater/internal/file/file_test.go | 104 ++++++++++++++++++----------- 2 files changed, 75 insertions(+), 39 deletions(-) diff --git a/updater/internal/file/file.go b/updater/internal/file/file.go index 37efa74d6..cee6295cb 100644 --- a/updater/internal/file/file.go +++ b/updater/internal/file/file.go @@ -38,6 +38,12 @@ func CopyFileOverwrite(logger *zap.Logger, pathIn, pathOut string) error { fileMode = outFileInfo.Mode() } + pathInClean := filepath.Clean(pathIn) + // If the input file cannot be opened for some reason, do NOT delete the file + if _, err := os.Stat(pathInClean); err != nil { + return fmt.Errorf("failed to stat input file: %w", err) + } + // Remove old file to prevent issues with mac if err := os.Remove(pathOutClean); err != nil { logger.Debug("Failed to remove output file", zap.Error(err)) @@ -73,6 +79,10 @@ func CopyFileRollback(logger *zap.Logger, pathIn, pathOut string) error { case errors.Is(err, os.ErrNotExist): return fmt.Errorf("input file does not exist: %w", err) case err != nil: + // TODO: Should this actually error out? + // If we can't STAT the input file, what are the chances we can open for reading? + // we are going to delete the file, so we might be better off bailing here and leaving the + // newer file instead of deleting it and failing to copy the input file logger.Error("failed to retrieve fileinfo for input file", zap.Error(err)) default: fileMode = inFileInfo.Mode() diff --git a/updater/internal/file/file_test.go b/updater/internal/file/file_test.go index 12dda94a6..33957e79a 100644 --- a/updater/internal/file/file_test.go +++ b/updater/internal/file/file_test.go @@ -25,14 +25,14 @@ import ( "go.uber.org/zap/zaptest" ) -func TestCopyFile(t *testing.T) { +func TestCopyFileOverwrite(t *testing.T) { t.Run("Copies file when output does not exist", func(t *testing.T) { tmpDir := t.TempDir() inFile := filepath.Join("testdata", "test.txt") outFile := filepath.Join(tmpDir, "test.txt") - err := CopyFile(zaptest.NewLogger(t), inFile, outFile, true, false) + err := CopyFileOverwrite(zaptest.NewLogger(t), inFile, outFile) require.NoError(t, err) require.FileExists(t, outFile) @@ -52,34 +52,6 @@ func TestCopyFile(t *testing.T) { } }) - t.Run("Copies file when output does not exist and uses new permissions when argument set", func(t *testing.T) { - tmpDir := t.TempDir() - - inFile := filepath.Join("testdata", "test.txt") - outFile := filepath.Join(tmpDir, "test.txt") - - err := CopyFile(zaptest.NewLogger(t), inFile, outFile, true, true) - require.NoError(t, err) - require.FileExists(t, outFile) - - contentsIn, err := os.ReadFile(inFile) - require.NoError(t, err) - - contentsOut, err := os.ReadFile(outFile) - require.NoError(t, err) - - require.Equal(t, contentsIn, contentsOut) - - fio, err := os.Stat(outFile) - require.NoError(t, err) - fii, err := os.Stat(outFile) - require.NoError(t, err) - // file mode on windows acts unlike unix, we'll only check for this on linux/darwin - if runtime.GOOS != "windows" { - require.Equal(t, fii.Mode(), fio.Mode()) - } - }) - t.Run("Copies file when output already exists", func(t *testing.T) { tmpDir := t.TempDir() @@ -95,7 +67,7 @@ func TestCopyFile(t *testing.T) { fioOrig, err := os.Stat(outFile) require.NoError(t, err) - err = CopyFile(zaptest.NewLogger(t), inFile, outFile, true, false) + err = CopyFileOverwrite(zaptest.NewLogger(t), inFile, outFile) require.NoError(t, err) require.FileExists(t, outFile) @@ -117,8 +89,8 @@ func TestCopyFile(t *testing.T) { inFile := filepath.Join("testdata", "does-not-exist.txt") outFile := filepath.Join(tmpDir, "test.txt") - err := CopyFile(zaptest.NewLogger(t), inFile, outFile, true, false) - require.ErrorContains(t, err, "failed to open input file") + err := CopyFileOverwrite(zaptest.NewLogger(t), inFile, outFile) + require.ErrorContains(t, err, "failed to stat input file") require.NoFileExists(t, outFile) }) @@ -131,16 +103,46 @@ func TestCopyFile(t *testing.T) { err := os.WriteFile(outFile, []byte("This is a file that already exists"), 0600) require.NoError(t, err) - err = CopyFile(zaptest.NewLogger(t), inFile, outFile, true, false) - require.ErrorContains(t, err, "failed to open input file") + err = CopyFileOverwrite(zaptest.NewLogger(t), inFile, outFile) + require.ErrorContains(t, err, "failed to stat input file") require.FileExists(t, outFile) contentsOut, err := os.ReadFile(outFile) require.NoError(t, err) require.Equal(t, []byte("This is a file that already exists"), contentsOut) }) +} - t.Run("Fails to overwrite the output file if 'overwrite' false", func(t *testing.T) { +func TestCopyFileRollback(t *testing.T) { + t.Run("Copies file when output does not exist", func(t *testing.T) { + tmpDir := t.TempDir() + + inFile := filepath.Join("testdata", "test.txt") + outFile := filepath.Join(tmpDir, "test.txt") + + err := CopyFileNoOverwrite(zaptest.NewLogger(t), inFile, outFile) + require.NoError(t, err) + require.FileExists(t, outFile) + + contentsIn, err := os.ReadFile(inFile) + require.NoError(t, err) + + contentsOut, err := os.ReadFile(outFile) + require.NoError(t, err) + + require.Equal(t, contentsIn, contentsOut) + + fio, err := os.Stat(outFile) + require.NoError(t, err) + fii, err := os.Stat(outFile) + require.NoError(t, err) + // file mode on windows acts unlike unix, we'll only check for this on linux/darwin + if runtime.GOOS != "windows" { + require.Equal(t, fii.Mode(), fio.Mode()) + } + }) + + t.Run("Fails to overwrite the output file", func(t *testing.T) { tmpDir := t.TempDir() inFile := filepath.Join("testdata", "test.txt") @@ -149,7 +151,7 @@ func TestCopyFile(t *testing.T) { err := os.WriteFile(outFile, []byte("This is a file that already exists"), 0640) require.NoError(t, err) - err = CopyFile(zaptest.NewLogger(t), inFile, outFile, false, false) + err = CopyFileNoOverwrite(zaptest.NewLogger(t), inFile, outFile) require.ErrorContains(t, err, "failed to open output file") require.FileExists(t, outFile) @@ -165,13 +167,26 @@ func TestCopyFile(t *testing.T) { } }) - t.Run("Copies file when output does not exist when 'overwrite' is false", func(t *testing.T) { + t.Run("Fails when input file does not exist", func(t *testing.T) { + tmpDir := t.TempDir() + + inFile := filepath.Join("testdata", "does-not-exist.txt") + outFile := filepath.Join(tmpDir, "test.txt") + + err := CopyFileNoOverwrite(zaptest.NewLogger(t), inFile, outFile) + require.ErrorContains(t, err, "failed to retrieve fileinfo for input file") + require.NoFileExists(t, outFile) + }) +} + +func TestCopyFileNoOverwrite(t *testing.T) { + t.Run("Copies file when output does not exist and uses inFile's permissions", func(t *testing.T) { tmpDir := t.TempDir() inFile := filepath.Join("testdata", "test.txt") outFile := filepath.Join(tmpDir, "test.txt") - err := CopyFile(zaptest.NewLogger(t), inFile, outFile, false, false) + err := CopyFileRollback(zaptest.NewLogger(t), inFile, outFile) require.NoError(t, err) require.FileExists(t, outFile) @@ -192,4 +207,15 @@ func TestCopyFile(t *testing.T) { require.Equal(t, fii.Mode(), fio.Mode()) } }) + + t.Run("Fails when input file does not exist", func(t *testing.T) { + tmpDir := t.TempDir() + + inFile := filepath.Join("testdata", "does-not-exist.txt") + outFile := filepath.Join(tmpDir, "test.txt") + + err := CopyFileRollback(zaptest.NewLogger(t), inFile, outFile) + require.ErrorContains(t, err, "input file does not exist") + require.NoFileExists(t, outFile) + }) } From 9137ff7d6f9f12529df287399214c59432bb1d2b Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Tue, 2 Aug 2022 13:58:19 -0400 Subject: [PATCH 6/9] fix linux build --- updater/internal/service/service_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/updater/internal/service/service_linux.go b/updater/internal/service/service_linux.go index 452e1d47a..c8804270f 100644 --- a/updater/internal/service/service_linux.go +++ b/updater/internal/service/service_linux.go @@ -165,7 +165,7 @@ func (l linuxService) Update() error { } func (l linuxService) Backup() error { - if err := file.CopyFileNoOverwrite(l.logger.Named("copy-file"), l.installedServiceFilePath, path.BackupServiceFile(l.installDir), false, false); err != nil { + if err := file.CopyFileNoOverwrite(l.logger.Named("copy-file"), l.installedServiceFilePath, path.BackupServiceFile(l.installDir)); err != nil { return fmt.Errorf("failed to copy service file: %w", err) } From 306ee375badc44232783432df3aaec651b1b0c84 Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Wed, 3 Aug 2022 11:21:22 -0400 Subject: [PATCH 7/9] remove todo --- updater/internal/file/file.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/updater/internal/file/file.go b/updater/internal/file/file.go index cee6295cb..10a2f5abe 100644 --- a/updater/internal/file/file.go +++ b/updater/internal/file/file.go @@ -79,10 +79,6 @@ func CopyFileRollback(logger *zap.Logger, pathIn, pathOut string) error { case errors.Is(err, os.ErrNotExist): return fmt.Errorf("input file does not exist: %w", err) case err != nil: - // TODO: Should this actually error out? - // If we can't STAT the input file, what are the chances we can open for reading? - // we are going to delete the file, so we might be better off bailing here and leaving the - // newer file instead of deleting it and failing to copy the input file logger.Error("failed to retrieve fileinfo for input file", zap.Error(err)) default: fileMode = inFileInfo.Mode() From 2605533c67b37cfaf27b4b985259867887d6321a Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Wed, 3 Aug 2022 11:22:54 -0400 Subject: [PATCH 8/9] explain why we continue even on error. --- updater/internal/file/file.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/updater/internal/file/file.go b/updater/internal/file/file.go index 10a2f5abe..11bb78f21 100644 --- a/updater/internal/file/file.go +++ b/updater/internal/file/file.go @@ -79,6 +79,8 @@ func CopyFileRollback(logger *zap.Logger, pathIn, pathOut string) error { case errors.Is(err, os.ErrNotExist): return fmt.Errorf("input file does not exist: %w", err) case err != nil: + // Even though we failed to stat, we'll continue in this case to give the best chance + // of rolling back successfully. logger.Error("failed to retrieve fileinfo for input file", zap.Error(err)) default: fileMode = inFileInfo.Mode() From 93c48590179fcb323684f187fd35a7c892ee02e8 Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Wed, 3 Aug 2022 11:34:32 -0400 Subject: [PATCH 9/9] empty commit for testing