feat: Remove tmpdir from updater (#591)

* starting on changing installDir

* fix and add tests

* fix gosec issues

* add license

* fix formatting

* remove command line option from collector

* remove redundant parameters, rename copyFiles functions

.gitignore

@@ -20,7 +20,7 @@ windows/plugins
 opentelemetry-java-contrib-jmx-metrics.jar
 VERSION.txt
 release_deps
-tmp
+/tmp
 
 # OpAmp Files
 collector.yaml

opamp/observiq/updater_manager_others.go

@@ -48,12 +48,9 @@ func newUpdaterManager(defaultLogger *zap.Logger, tmpPath string) updaterManager
 // While waiting for Updater, it should kill the collector and we should never execute any code past running it
 func (m othersUpdaterManager) StartAndMonitorUpdater() error {
 	updaterPath := filepath.Join(m.tmpPath, updaterDir, updaterName)
-	absTmpPath, err := filepath.Abs(m.tmpPath)
-	if err != nil {
-		return fmt.Errorf("failed to get absolute path of tmp dir: %w", err)
-	}
+
 	//#nosec G204 -- paths are not determined via user input
-	cmd := exec.Command(updaterPath, "--tmpdir", absTmpPath)
+	cmd := exec.Command(updaterPath)
 
 	// We need to set the processor group id to something different so that at least on mac, when the
 	// collector dies the updater won't die as well

opamp/observiq/updater_manager_windows.go

@@ -48,12 +48,9 @@ func newUpdaterManager(defaultLogger *zap.Logger, tmpPath string) updaterManager
 // While waiting for Updater, it should kill the collector and we should never execute any code past running it
 func (m windowsUpdaterManager) StartAndMonitorUpdater() error {
 	updaterPath := filepath.Join(m.tmpPath, updaterDir, updaterName)
-	absTmpPath, err := filepath.Abs(m.tmpPath)
-	if err != nil {
-		return fmt.Errorf("failed to get absolute path of tmp dir: %w", err)
-	}
+
 	//#nosec G204 -- paths are not determined via user input
-	cmd := exec.Command(updaterPath, "--tmpdir", absTmpPath)
+	cmd := exec.Command(updaterPath)
 
 	// Start does not block
 	if err := cmd.Start(); err != nil {

updater/cmd/updater/main.go

@@ -19,7 +19,6 @@ import (
 	"errors"
 	"fmt"
 	"log"
-	"os"
 	"time"
 
 	"github.com/observiq/observiq-otel-collector/packagestate"
@@ -36,7 +35,6 @@ import (
 
 func main() {
 	var showVersion = pflag.BoolP("version", "v", false, "Prints the version of the updater and exits, if specified.")
-	var tmpDir = pflag.String("tmpdir", "", "Temporary directory for artifacts. Parent of the 'rollback' directory.")
 	pflag.Parse()
 
 	if *showVersion {
@@ -46,12 +44,6 @@ func main() {
 		return
 	}
 
-	if *tmpDir == "" {
-		log.Println("The --tmpdir flag must be specified!")
-		pflag.PrintDefaults()
-		os.Exit(1)
-	}
-
 	// We can't create the zap logger yet, because we don't know the install dir, which is needed
 	// to create the logger. So we pass a Nop logger here.
 	installDir, err := path.InstallDir(zap.NewNop())
@@ -65,17 +57,17 @@ func main() {
 	}
 
 	// Create a monitor and load the package status file
-	monitor, err := state.NewCollectorMonitor(logger)
+	monitor, err := state.NewCollectorMonitor(logger, installDir)
 	if err != nil {
 		logger.Fatal("Failed to create monitor", zap.Error(err))
 	}
 
-	installer, err := install.NewInstaller(logger, *tmpDir)
+	installer, err := install.NewInstaller(logger, installDir)
 	if err != nil {
 		logger.Fatal("Failed to create installer", zap.Error(err))
 	}
 
-	rb, err := rollback.NewRollbacker(logger, *tmpDir)
+	rb, err := rollback.NewRollbacker(logger, installDir)
 	if err != nil {
 		logger.Fatal("Failed to create rollbacker", zap.Error(err))
 	}

updater/internal/action/file_action.go

@@ -42,10 +42,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. tmpDir is specified for rollback purposes.
+// fromPathRel into toPath. installDir 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, tmpDir string) (*CopyFileAction, error) {
+func NewCopyFileAction(logger *zap.Logger, fromPathRel, toPath, installDir string) (*CopyFileAction, error) {
 	fileExists := true
 	_, err := os.Stat(toPath)
 	switch {
@@ -60,7 +60,7 @@ func NewCopyFileAction(logger *zap.Logger, fromPathRel, toPath, tmpDir string) (
 		ToPath:      toPath,
 		// The file will be created if it doesn't already exist
 		FileCreated: !fileExists,
-		backupDir:   path.BackupDirFromTempDir(tmpDir),
+		backupDir:   path.BackupDir(installDir),
 		logger:      logger.Named("copy-file-action"),
 	}, nil
 }

updater/internal/action/file_action_test.go

@@ -26,40 +26,40 @@ import (
 func TestNewCopyFileAction(t *testing.T) {
 	t.Run("out file does not exist", func(t *testing.T) {
 		scratchDir := t.TempDir()
-		testTempDir := filepath.Join("testdata", "copyfileaction")
+		testInstallDir := filepath.Join("testdata", "copyfileaction")
 		outFile := filepath.Join(scratchDir, "test.txt")
-		inFile := filepath.Join(testTempDir, "latest", "test.txt")
+		inFile := filepath.Join(testInstallDir, "latest", "test.txt")
 
-		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testTempDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testInstallDir)
 		require.NoError(t, err)
 
 		require.Equal(t, &CopyFileAction{
 			FromPathRel: inFile,
 			ToPath:      outFile,
 			FileCreated: true,
-			backupDir:   filepath.Join(testTempDir, "rollback"),
+			backupDir:   filepath.Join(testInstallDir, "tmp", "rollback"),
 			logger:      a.logger,
 		}, a)
 	})
 
 	t.Run("out file exists", func(t *testing.T) {
 		scratchDir := t.TempDir()
-		testTempDir := filepath.Join("testdata", "copyfileaction")
+		testInstallDir := filepath.Join("testdata", "copyfileaction")
 		outFile := filepath.Join(scratchDir, "test.txt")
-		inFile := filepath.Join(testTempDir, "latest", "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, testTempDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testInstallDir)
 		require.NoError(t, err)
 
 		require.Equal(t, &CopyFileAction{
 			FromPathRel: inFile,
 			ToPath:      outFile,
 			FileCreated: false,
-			backupDir:   filepath.Join(testTempDir, "rollback"),
+			backupDir:   filepath.Join(testInstallDir, "tmp", "rollback"),
 			logger:      a.logger,
 		}, a)
 	})
@@ -68,11 +68,11 @@ func TestNewCopyFileAction(t *testing.T) {
 func TestCopyFileActionRollback(t *testing.T) {
 	t.Run("deletes out file if it does not exist", func(t *testing.T) {
 		scratchDir := t.TempDir()
-		testTempDir := filepath.Join("testdata", "copyfileaction")
+		testInstallDir := filepath.Join("testdata", "copyfileaction")
 		outFile := filepath.Join(scratchDir, "test.txt")
-		inFile := filepath.Join(testTempDir, "latest", "test.txt")
+		inFile := filepath.Join(testInstallDir, "tmp", "latest", "test.txt")
 
-		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testTempDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testInstallDir)
 		require.NoError(t, err)
 
 		inBytes, err := os.ReadFile(inFile)
@@ -89,19 +89,19 @@ func TestCopyFileActionRollback(t *testing.T) {
 
 	t.Run("Rolls back out file when it exists", func(t *testing.T) {
 		scratchDir := t.TempDir()
-		testTempDir := filepath.Join("testdata", "copyfileaction")
+		testInstallDir := filepath.Join("testdata", "copyfileaction")
 		outFile := filepath.Join(scratchDir, "test.txt")
 		inFileRel := "test.txt"
-		inFile := filepath.Join(testTempDir, "latest", inFileRel)
-		originalFile := filepath.Join(testTempDir, "rollback", "test.txt")
+		inFile := filepath.Join(testInstallDir, "tmp", "latest", inFileRel)
+		originalFile := filepath.Join(testInstallDir, "tmp", "rollback", "test.txt")
 
 		originalBytes, err := os.ReadFile(originalFile)
 		require.NoError(t, err)
 
 		err = os.WriteFile(outFile, originalBytes, 0600)
 		require.NoError(t, err)
 
-		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFileRel, outFile, testTempDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFileRel, outFile, testInstallDir)
 		require.NoError(t, err)
 
 		// Overwrite original file with latest file
@@ -124,10 +124,10 @@ func TestCopyFileActionRollback(t *testing.T) {
 
 	t.Run("Fails if backup file doesn't exist", func(t *testing.T) {
 		scratchDir := t.TempDir()
-		testTempDir := filepath.Join("testdata", "copyfileaction")
+		testInstallDir := filepath.Join("testdata", "copyfileaction")
 		outFile := filepath.Join(scratchDir, "test.txt")
-		inFile := filepath.Join(testTempDir, "latest", "not_in_backup.txt")
-		originalFile := filepath.Join(testTempDir, "rollback", "test.txt")
+		inFile := filepath.Join(testInstallDir, "tmp", "latest", "not_in_backup.txt")
+		originalFile := filepath.Join(testInstallDir, "tmp", "rollback", "test.txt")
 
 		// The latest file exists in the directory already, but for some reason is not copied to backup
 		originalBytes, err := os.ReadFile(originalFile)
@@ -136,7 +136,7 @@ func TestCopyFileActionRollback(t *testing.T) {
 		err = os.WriteFile(outFile, originalBytes, 0600)
 		require.NoError(t, err)
 
-		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testTempDir)
+		a, err := NewCopyFileAction(zaptest.NewLogger(t), inFile, outFile, testInstallDir)
 		require.NoError(t, err)
 
 		// Overwrite original file with latest file

updater/internal/action/service_action.go

@@ -66,13 +66,13 @@ type ServiceUpdateAction struct {
 var _ RollbackableAction = (*ServiceUpdateAction)(nil)
 
 // NewServiceUpdateAction creates a new ServiceUpdateAction
-func NewServiceUpdateAction(logger *zap.Logger, tmpDir string) *ServiceUpdateAction {
+func NewServiceUpdateAction(logger *zap.Logger, installDir string) *ServiceUpdateAction {
 	namedLogger := logger.Named("service-update-action")
 	return &ServiceUpdateAction{
 		backupSvc: service.NewService(
 			namedLogger,
 			"", // latestDir doesn't matter here
-			service.WithServiceFile(path.BackupServiceFile(path.ServiceFileDir(path.BackupDirFromTempDir(tmpDir)))),
+			service.WithServiceFile(path.BackupServiceFile(installDir)),
 		),
 	}
 }

updater/internal/install/install.go

@@ -33,26 +33,18 @@ import (
 type Installer struct {
 	latestDir  string
 	installDir string
-	tmpDir     string
 	svc        service.Service
 	logger     *zap.Logger
 }
 
 // NewInstaller returns a new instance of an Installer.
-func NewInstaller(logger *zap.Logger, tmpDir string) (*Installer, error) {
+func NewInstaller(logger *zap.Logger, installDir string) (*Installer, error) {
 	namedLogger := logger.Named("installer")
 
-	latestDir := path.LatestDirFromTempDir(tmpDir)
-	installDirPath, err := path.InstallDir(namedLogger.Named("install-dir"))
-	if err != nil {
-		return nil, fmt.Errorf("failed to determine install dir: %w", err)
-	}
-
 	return &Installer{
-		latestDir:  latestDir,
-		svc:        service.NewService(namedLogger, latestDir),
-		installDir: installDirPath,
-		tmpDir:     tmpDir,
+		latestDir:  path.LatestDir(installDir),
+		svc:        service.NewService(namedLogger, installDir),
+		installDir: installDir,
 		logger:     namedLogger,
 	}, nil
 }
@@ -69,7 +61,7 @@ func (i Installer) Install(rb rollback.ActionAppender) error {
 
 	// install files that go to installDirPath to their correct location,
 	// excluding any config files (logging.yaml, config.yaml, manager.yaml)
-	if err := copyFiles(i.logger, i.latestDir, i.installDir, i.tmpDir, rb); err != nil {
+	if err := installFiles(i.logger, i.latestDir, i.installDir, rb); err != nil {
 		return fmt.Errorf("failed to install new files: %w", err)
 	}
 	i.logger.Debug("Install artifacts copied")
@@ -78,7 +70,7 @@ func (i Installer) Install(rb rollback.ActionAppender) error {
 	if err := i.svc.Update(); err != nil {
 		return fmt.Errorf("failed to update service: %w", err)
 	}
-	rb.AppendAction(action.NewServiceUpdateAction(i.logger, i.tmpDir))
+	rb.AppendAction(action.NewServiceUpdateAction(i.logger, i.installDir))
 	i.logger.Debug("Updated service configuration")
 
 	// Start service
@@ -91,9 +83,9 @@ func (i Installer) Install(rb rollback.ActionAppender) error {
 	return nil
 }
 
-// copyFiles moves the file tree rooted at latestDirPath to installDirPath,
+// installFiles moves the file tree rooted at inputPath to installDir,
 // skipping configuration files. Appends CopyFileAction-s to the Rollbacker as it copies file.
-func copyFiles(logger *zap.Logger, inputPath, outputPath, tmpDir string, rb rollback.ActionAppender) error {
+func installFiles(logger *zap.Logger, inputPath, installDir string, rb rollback.ActionAppender) error {
 	err := filepath.WalkDir(inputPath, func(inPath string, d fs.DirEntry, err error) error {
 		switch {
 		case err != nil:
@@ -116,7 +108,7 @@ func copyFiles(logger *zap.Logger, inputPath, outputPath, tmpDir string, rb roll
 
 		// use the relative path to get the outPath (where we should write the file), and
 		// to get the out directory (which we will create if it does not exist).
-		outPath := filepath.Join(outputPath, relPath)
+		outPath := filepath.Join(installDir, relPath)
 		outDir := filepath.Dir(outPath)
 
 		if err := os.MkdirAll(outDir, 0750); err != nil {
@@ -125,7 +117,7 @@ func copyFiles(logger *zap.Logger, inputPath, outputPath, tmpDir string, rb roll
 
 		// We create the action record here, because we want to record whether the file exists or not before
 		// we open the file (which will end up creating the file).
-		cfa, err := action.NewCopyFileAction(logger, relPath, outPath, tmpDir)
+		cfa, err := action.NewCopyFileAction(logger, relPath, outPath, installDir)
 		if err != nil {
 			return fmt.Errorf("failed to create copy file action: %w", err)
 		}

updater/internal/install/install_test.go

@@ -81,7 +81,7 @@ func TestInstallArtifacts(t *testing.T) {
 			installer.logger,
 			filepath.Join("test.txt"),
 			filepath.Join(installer.installDir, "test.txt"),
-			installer.tmpDir,
+			installer.installDir,
 		)
 		require.NoError(t, err)
 		copyTestTxtAction.FileCreated = true
@@ -90,7 +90,7 @@ func TestInstallArtifacts(t *testing.T) {
 			installer.logger,
 			filepath.Join("test-folder", "another-test.txt"),
 			filepath.Join(installer.installDir, "test-folder", "another-test.txt"),
-			installer.tmpDir,
+			installer.installDir,
 		)
 		require.NoError(t, err)
 		copyNestedTestTxtAction.FileCreated = true
@@ -99,7 +99,7 @@ func TestInstallArtifacts(t *testing.T) {
 			action.NewServiceStopAction(svc),
 			copyNestedTestTxtAction,
 			copyTestTxtAction,
-			action.NewServiceUpdateAction(installer.logger, installer.tmpDir),
+			action.NewServiceUpdateAction(installer.logger, installer.installDir),
 			action.NewServiceStartAction(svc),
 		}, actions)
 	})
@@ -147,7 +147,7 @@ func TestInstallArtifacts(t *testing.T) {
 			installer.logger,
 			filepath.Join("test.txt"),
 			filepath.Join(installer.installDir, "test.txt"),
-			installer.tmpDir,
+			installer.installDir,
 		)
 		require.NoError(t, err)
 		copyTestTxtAction.FileCreated = true
@@ -156,7 +156,7 @@ func TestInstallArtifacts(t *testing.T) {
 			installer.logger,
 			filepath.Join("test-folder", "another-test.txt"),
 			filepath.Join(installer.installDir, "test-folder", "another-test.txt"),
-			installer.tmpDir,
+			installer.installDir,
 		)
 		require.NoError(t, err)
 		copyNestedTestTxtAction.FileCreated = true
@@ -196,7 +196,7 @@ func TestInstallArtifacts(t *testing.T) {
 			installer.logger,
 			filepath.Join("test.txt"),
 			filepath.Join(installer.installDir, "test.txt"),
-			installer.tmpDir,
+			installer.installDir,
 		)
 		require.NoError(t, err)
 		copyTestTxtAction.FileCreated = true
@@ -205,7 +205,7 @@ func TestInstallArtifacts(t *testing.T) {
 			installer.logger,
 			filepath.Join("test-folder", "another-test.txt"),
 			filepath.Join(installer.installDir, "test-folder", "another-test.txt"),
-			installer.tmpDir,
+			installer.installDir,
 		)
 		require.NoError(t, err)
 		copyNestedTestTxtAction.FileCreated = true
@@ -214,7 +214,7 @@ func TestInstallArtifacts(t *testing.T) {
 			action.NewServiceStopAction(svc),
 			copyNestedTestTxtAction,
 			copyTestTxtAction,
-			action.NewServiceUpdateAction(installer.logger, installer.tmpDir),
+			action.NewServiceUpdateAction(installer.logger, installer.installDir),
 		}, actions)
 	})