Updater properly installs and rollbacks JMX Jar. (#600)

Also making sure that we use the backed up file permissions when
rolling back a file that no longer exists in the install directory

updater/internal/action/file_action.go

@@ -21,7 +21,6 @@ import (
 	"path/filepath"
 
 	"github.com/observiq/observiq-otel-collector/updater/internal/file"
-	"github.com/observiq/observiq-otel-collector/updater/internal/path"
 	"go.uber.org/zap"
 )
 
@@ -42,10 +41,10 @@ var _ RollbackableAction = (*CopyFileAction)(nil)
 var _ fmt.Stringer = (*CopyFileAction)(nil)
 
 // NewCopyFileAction creates a new CopyFileAction that indicates a file was copied from
-// fromPathRel into toPath. installDir is specified for rollback purposes.
+// fromPathRel into toPath. backupDir is specified for rollback purposes.
 // NOTE: This action MUST be created BEFORE the action actually takes place; This allows
 // for previous existence of the file to be recorded.
-func NewCopyFileAction(logger *zap.Logger, fromPathRel, toPath, installDir string) (*CopyFileAction, error) {
+func NewCopyFileAction(logger *zap.Logger, fromPathRel, toPath, backupDir string) (*CopyFileAction, error) {
 	fileExists := true
 	_, err := os.Stat(toPath)
 	switch {
@@ -60,7 +59,7 @@ func NewCopyFileAction(logger *zap.Logger, fromPathRel, toPath, installDir strin
 		ToPath:      toPath,
 		// The file will be created if it doesn't already exist
 		FileCreated: !fileExists,
-		backupDir:   path.BackupDir(installDir),
+		backupDir:   backupDir,
 		logger:      logger.Named("copy-file-action"),
 	}, nil
 }
@@ -76,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); err != nil {
+	if err := file.CopyFile(c.logger.Named("copy-file"), backupFilePath, c.ToPath, true, true); err != nil {
 		return fmt.Errorf("failed to copy file: %w", err)
 	}
 

updater/internal/action/file_action_test.go

@@ -19,6 +19,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/observiq/observiq-otel-collector/updater/internal/path"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zaptest"
 )
@@ -27,39 +28,41 @@ func TestNewCopyFileAction(t *testing.T) {
 	t.Run("out file does not exist", func(t *testing.T) {
 		scratchDir := t.TempDir()
 		testInstallDir := filepath.Join("testdata", "copyfileaction")
+		backupDir := path.BackupDir(testInstallDir)
 		outFile := filepath.Join(scratchDir, "test.txt")
 		inFile := filepath.Join(testInstallDir, "latest", "test.txt")
 
-		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testInstallDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, backupDir)
 		require.NoError(t, err)
 
 		require.Equal(t, &CopyFileAction{
 			FromPathRel: inFile,
 			ToPath:      outFile,
 			FileCreated: true,
-			backupDir:   filepath.Join(testInstallDir, "tmp", "rollback"),
+			backupDir:   backupDir,
 			logger:      a.logger,
 		}, a)
 	})
 
 	t.Run("out file exists", func(t *testing.T) {
 		scratchDir := t.TempDir()
 		testInstallDir := filepath.Join("testdata", "copyfileaction")
+		backupDir := path.BackupDir(testInstallDir)
 		outFile := filepath.Join(scratchDir, "test.txt")
 		inFile := filepath.Join(testInstallDir, "latest", "test.txt")
 
 		f, err := os.Create(outFile)
 		require.NoError(t, err)
 		require.NoError(t, f.Close())
 
-		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testInstallDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, backupDir)
 		require.NoError(t, err)
 
 		require.Equal(t, &CopyFileAction{
 			FromPathRel: inFile,
 			ToPath:      outFile,
 			FileCreated: false,
-			backupDir:   filepath.Join(testInstallDir, "tmp", "rollback"),
+			backupDir:   backupDir,
 			logger:      a.logger,
 		}, a)
 	})
@@ -69,10 +72,11 @@ func TestCopyFileActionRollback(t *testing.T) {
 	t.Run("deletes out file if it does not exist", func(t *testing.T) {
 		scratchDir := t.TempDir()
 		testInstallDir := filepath.Join("testdata", "copyfileaction")
+		backupDir := path.BackupDir(testInstallDir)
 		outFile := filepath.Join(scratchDir, "test.txt")
 		inFile := filepath.Join(testInstallDir, "tmp", "latest", "test.txt")
 
-		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testInstallDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, backupDir)
 		require.NoError(t, err)
 
 		inBytes, err := os.ReadFile(inFile)
@@ -90,6 +94,7 @@ func TestCopyFileActionRollback(t *testing.T) {
 	t.Run("Rolls back out file when it exists", func(t *testing.T) {
 		scratchDir := t.TempDir()
 		testInstallDir := filepath.Join("testdata", "copyfileaction")
+		backupDir := path.BackupDir(testInstallDir)
 		outFile := filepath.Join(scratchDir, "test.txt")
 		inFileRel := "test.txt"
 		inFile := filepath.Join(testInstallDir, "tmp", "latest", inFileRel)
@@ -101,7 +106,7 @@ func TestCopyFileActionRollback(t *testing.T) {
 		err = os.WriteFile(outFile, originalBytes, 0600)
 		require.NoError(t, err)
 
-		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFileRel, outFile, testInstallDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFileRel, outFile, backupDir)
 		require.NoError(t, err)
 
 		// Overwrite original file with latest file
@@ -125,6 +130,7 @@ func TestCopyFileActionRollback(t *testing.T) {
 	t.Run("Fails if backup file doesn't exist", func(t *testing.T) {
 		scratchDir := t.TempDir()
 		testInstallDir := filepath.Join("testdata", "copyfileaction")
+		backupDir := path.BackupDir(testInstallDir)
 		outFile := filepath.Join(scratchDir, "test.txt")
 		inFile := filepath.Join(testInstallDir, "tmp", "latest", "not_in_backup.txt")
 		originalFile := filepath.Join(testInstallDir, "tmp", "rollback", "test.txt")
@@ -136,7 +142,7 @@ func TestCopyFileActionRollback(t *testing.T) {
 		err = os.WriteFile(outFile, originalBytes, 0600)
 		require.NoError(t, err)
 
-		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testInstallDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, backupDir)
 		require.NoError(t, err)
 
 		// Overwrite original file with latest file

updater/internal/file/file.go

@@ -26,7 +26,7 @@ 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) error {
+func CopyFile(logger *zap.Logger, pathIn, pathOut string, overwrite bool, useInFilePermBackup bool) error {
 	pathInClean := filepath.Clean(pathIn)
 
 	// Open the input file for reading.
@@ -43,21 +43,41 @@ 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
 
-		// Save old file's permissions and delete it first if it exists
-		fileOutInfo, _ := os.Stat(pathOutClean)
-		if fileOutInfo != nil {
-			fileMode = fileOutInfo.Mode()
+		// Try to save old 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 {
+			logger.Debug("Failed to remove output file", zap.Error(err))
 		}
-		_ = os.Remove(pathOutClean)
 	} else {
 		// 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()
+		if err != nil {
+			return fmt.Errorf("failed to retrive fileinfo for input file: %w", err)
+		}
+
+		fileMode = inFileInfo.Mode()
 	}
 
 	// Open the output file, creating it if it does not exist and truncating it.

updater/internal/file/file_test.go

@@ -32,7 +32,7 @@ func TestCopyFile(t *testing.T) {
 		inFile := filepath.Join("testdata", "test.txt")
 		outFile := filepath.Join(tmpDir, "test.txt")
 
-		err := CopyFile(zaptest.NewLogger(t), inFile, outFile, true)
+		err := CopyFile(zaptest.NewLogger(t), inFile, outFile, true, false)
 		require.NoError(t, err)
 		require.FileExists(t, outFile)
 
@@ -52,6 +52,34 @@ 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()
 
@@ -64,19 +92,22 @@ 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, true)
+		fioOrig, err := os.Stat(outFile)
+		require.NoError(t, err)
+
+		err = CopyFile(zaptest.NewLogger(t), inFile, outFile, true, false)
 		require.NoError(t, err)
 		require.FileExists(t, outFile)
 
 		contentsOut, err := os.ReadFile(outFile)
 		require.NoError(t, err)
 		require.Equal(t, contentsIn, contentsOut)
 
-		fi, err := os.Stat(outFile)
+		fio, 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, fs.FileMode(0640), fi.Mode())
+			require.Equal(t, fioOrig.Mode(), fio.Mode())
 		}
 	})
 
@@ -86,7 +117,7 @@ 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)
+		err := CopyFile(zaptest.NewLogger(t), inFile, outFile, true, false)
 		require.ErrorContains(t, err, "failed to open input file")
 		require.NoFileExists(t, outFile)
 	})
@@ -100,7 +131,7 @@ 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)
+		err = CopyFile(zaptest.NewLogger(t), inFile, outFile, true, false)
 		require.ErrorContains(t, err, "failed to open input file")
 		require.FileExists(t, outFile)
 
@@ -118,7 +149,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)
+		err = CopyFile(zaptest.NewLogger(t), inFile, outFile, false, false)
 		require.ErrorContains(t, err, "failed to open output file")
 		require.FileExists(t, outFile)
 
@@ -140,7 +171,7 @@ func TestCopyFile(t *testing.T) {
 		inFile := filepath.Join("testdata", "test.txt")
 		outFile := filepath.Join(tmpDir, "test.txt")
 
-		err := CopyFile(zaptest.NewLogger(t), inFile, outFile, false)
+		err := CopyFile(zaptest.NewLogger(t), inFile, outFile, false, false)
 		require.NoError(t, err)
 		require.FileExists(t, outFile)
 
@@ -152,11 +183,13 @@ func TestCopyFile(t *testing.T) {
 
 		require.Equal(t, contentsIn, contentsOut)
 
-		fi, err := os.Stat(outFile)
+		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, fs.FileMode(0600), fi.Mode())
+			require.Equal(t, fii.Mode(), fio.Mode())
 		}
 	})
 }