Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Makes sure JMX jar is installed and rolled back properly #600

Merged
merged 1 commit into from
Jul 29, 2022
Merged

fix: Makes sure JMX jar is installed and rolled back properly #600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions updater/internal/action/file_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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)
}

Expand Down
20 changes: 13 additions & 7 deletions updater/internal/action/file_action_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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)
})
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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")
Expand All @@ -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
Expand Down
34 changes: 27 additions & 7 deletions updater/internal/file/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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()
}
}
Comment on lines +51 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is right, but does it make sense to always prefer the destination file's permissions, even when useInFilePermBackup is true? The logic just feels off to me for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah...that's a good point. If the old file exists there's really no reason we won't get it's permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. it's all a little confusing for sure. ultimately we have 3 cases where we use this copy method.

  1. Backup - We want to preserve the original file's permissions on the backed up file
  2. Install - We want use the existing file's permissions. If there is no existing file, we are going to use 0600 and hope for the best (this might cause issues if we have executables that we add in the future).
  3. Rollback - This is a little more ambiguous. If the file currently exists, it probably doesn't matter that much if we use the existing file's permissions or the backed up file's permissions. If the file does NOT exist, then we definitely want to use the backed up file's permissions (and not the default 0600).


// 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()
Comment on lines +73 to +80
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we always want to use the input files's mode in this case, or only when the useInFilePermBackup flag is true?
I guess we only use this on backup, so I guess overwrite = false currently implies useInFilePermBackup = true.

}

// Open the output file, creating it if it does not exist and truncating it.
Expand Down
53 changes: 43 additions & 10 deletions updater/internal/file/file_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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()

Expand All @@ -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())
}
})

Expand All @@ -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)
})
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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())
}
})
}
Loading