From 6f590175e1bbf20250624c5e56c52050cde7ee38 Mon Sep 17 00:00:00 2001 From: Brandon Johnson Date: Wed, 27 Jul 2022 16:33:58 -0400 Subject: [PATCH] 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 | 2 +- opamp/observiq/updater_manager_others.go | 7 +- opamp/observiq/updater_manager_windows.go | 7 +- updater/cmd/updater/main.go | 14 +--- updater/internal/action/file_action.go | 6 +- updater/internal/action/file_action_test.go | 38 ++++----- .../action/mocks/rollbackable_action.go | 2 +- updater/internal/action/service_action.go | 4 +- .../{ => tmp}/latest/not_in_backup.txt | 0 .../copyfileaction/{ => tmp}/latest/test.txt | 0 .../{ => tmp}/rollback/test.txt | 0 updater/internal/install/install.go | 28 +++---- updater/internal/install/install_test.go | 16 ++-- updater/internal/path/path.go | 42 +++++----- updater/internal/path/path_test.go | 21 ++--- updater/internal/path/path_windows.go | 6 +- updater/internal/path/path_windows_test.go | 80 +++++++++++++++++++ .../rollback/mocks/action_appender.go | 2 +- updater/internal/rollback/rollback.go | 22 +++-- updater/internal/rollback/rollback_test.go | 9 +-- updater/internal/service/mocks/service.go | 12 +-- updater/internal/service/service.go | 4 +- updater/internal/service/service_darwin.go | 8 +- .../internal/service/service_darwin_test.go | 26 +++--- updater/internal/service/service_linux.go | 10 ++- .../internal/service/service_linux_test.go | 27 ++++--- updater/internal/service/service_windows.go | 26 ++---- .../internal/service/service_windows_test.go | 62 +++----------- updater/internal/state/monitor.go | 9 +-- 29 files changed, 253 insertions(+), 237 deletions(-) rename updater/internal/action/testdata/copyfileaction/{ => tmp}/latest/not_in_backup.txt (100%) rename updater/internal/action/testdata/copyfileaction/{ => tmp}/latest/test.txt (100%) rename updater/internal/action/testdata/copyfileaction/{ => tmp}/rollback/test.txt (100%) create mode 100644 updater/internal/path/path_windows_test.go diff --git a/.gitignore b/.gitignore index 37133164a..c23c8c5b7 100644 --- a/.gitignore +++ b/.gitignore @@ -20,7 +20,7 @@ windows/plugins opentelemetry-java-contrib-jmx-metrics.jar VERSION.txt release_deps -tmp +/tmp # OpAmp Files collector.yaml diff --git a/opamp/observiq/updater_manager_others.go b/opamp/observiq/updater_manager_others.go index 6d91dce01..5c13ad287 100644 --- a/opamp/observiq/updater_manager_others.go +++ b/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 diff --git a/opamp/observiq/updater_manager_windows.go b/opamp/observiq/updater_manager_windows.go index 33f3e1230..bf90b575d 100644 --- a/opamp/observiq/updater_manager_windows.go +++ b/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 { diff --git a/updater/cmd/updater/main.go b/updater/cmd/updater/main.go index 9d5204101..80aacfefc 100644 --- a/updater/cmd/updater/main.go +++ b/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)) } diff --git a/updater/internal/action/file_action.go b/updater/internal/action/file_action.go index abf367ab9..90c95df3c 100644 --- a/updater/internal/action/file_action.go +++ b/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 } diff --git a/updater/internal/action/file_action_test.go b/updater/internal/action/file_action_test.go index 080b875d5..119b50cc7 100644 --- a/updater/internal/action/file_action_test.go +++ b/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,11 +89,11 @@ 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) @@ -101,7 +101,7 @@ func TestCopyFileActionRollback(t *testing.T) { 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 diff --git a/updater/internal/action/mocks/rollbackable_action.go b/updater/internal/action/mocks/rollbackable_action.go index 5ccf66995..43904bbfe 100644 --- a/updater/internal/action/mocks/rollbackable_action.go +++ b/updater/internal/action/mocks/rollbackable_action.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.13.1. DO NOT EDIT. +// Code generated by mockery v2.14.0. DO NOT EDIT. package mocks diff --git a/updater/internal/action/service_action.go b/updater/internal/action/service_action.go index c45f10cac..8781df5da 100644 --- a/updater/internal/action/service_action.go +++ b/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)), ), } } diff --git a/updater/internal/action/testdata/copyfileaction/latest/not_in_backup.txt b/updater/internal/action/testdata/copyfileaction/tmp/latest/not_in_backup.txt similarity index 100% rename from updater/internal/action/testdata/copyfileaction/latest/not_in_backup.txt rename to updater/internal/action/testdata/copyfileaction/tmp/latest/not_in_backup.txt diff --git a/updater/internal/action/testdata/copyfileaction/latest/test.txt b/updater/internal/action/testdata/copyfileaction/tmp/latest/test.txt similarity index 100% rename from updater/internal/action/testdata/copyfileaction/latest/test.txt rename to updater/internal/action/testdata/copyfileaction/tmp/latest/test.txt diff --git a/updater/internal/action/testdata/copyfileaction/rollback/test.txt b/updater/internal/action/testdata/copyfileaction/tmp/rollback/test.txt similarity index 100% rename from updater/internal/action/testdata/copyfileaction/rollback/test.txt rename to updater/internal/action/testdata/copyfileaction/tmp/rollback/test.txt diff --git a/updater/internal/install/install.go b/updater/internal/install/install.go index a6f21271c..4e607fc20 100644 --- a/updater/internal/install/install.go +++ b/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) } diff --git a/updater/internal/install/install_test.go b/updater/internal/install/install_test.go index ee158f3e4..e3322d42a 100644 --- a/updater/internal/install/install_test.go +++ b/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) }) diff --git a/updater/internal/path/path.go b/updater/internal/path/path.go index 510a065ea..509a76040 100644 --- a/updater/internal/path/path.go +++ b/updater/internal/path/path.go @@ -16,38 +16,32 @@ package path import "path/filepath" -const ( - latestDirFragment = "latest" - rollbackDirFragment = "rollback" - serviceFileDirFragment = "install" - serviceFileBackupFilename = "backup.service" - logDirFragment = "log" - logFile = "updater.log" -) - -// LatestDirFromTempDir gets the path to the "latest" dir, where the new artifacts are, -// from the temporary directory -func LatestDirFromTempDir(tmpDir string) string { - return filepath.Join(tmpDir, latestDirFragment) +// TempDir gets the path to the "tmp" dir, used for staging updates & backups +func TempDir(installDir string) string { + return filepath.Join(installDir, "tmp") } -// BackupDirFromTempDir gets the path to the "rollback" dir, where current artifacts are backed up, -// from the temporary directory -func BackupDirFromTempDir(tmpDir string) string { - return filepath.Join(tmpDir, rollbackDirFragment) +// LatestDir gets the path to the "latest" dir, where the new artifacts are unpacked. +func LatestDir(installDir string) string { + return filepath.Join(TempDir(installDir), "latest") } -// ServiceFileDir gets the directory of the service file definitions from the install dir -func ServiceFileDir(installBaseDir string) string { - return filepath.Join(installBaseDir, serviceFileDirFragment) +// BackupDir gets the path to the "rollback" dir, where current artifacts are backed up. +func BackupDir(installDir string) string { + return filepath.Join(TempDir(installDir), "rollback") } -// BackupServiceFile returns the full path to the backup service file from the service file directory path -func BackupServiceFile(serviceFileDir string) string { - return filepath.Join(serviceFileDir, serviceFileBackupFilename) +// ServiceFileDir gets the directory of the service file definitions +func ServiceFileDir(installDir string) string { + return filepath.Join(installDir, "install") +} + +// BackupServiceFile returns the full path to the backup service file +func BackupServiceFile(installDir string) string { + return filepath.Join(BackupDir(installDir), "backup.service") } // LogFile returns the full path to the log file for the updater func LogFile(installDir string) string { - return filepath.Join(installDir, logDirFragment, logFile) + return filepath.Join(installDir, "log", "updater.log") } diff --git a/updater/internal/path/path_test.go b/updater/internal/path/path_test.go index e6c0fe08b..538c11e97 100644 --- a/updater/internal/path/path_test.go +++ b/updater/internal/path/path_test.go @@ -21,25 +21,26 @@ import ( "github.com/stretchr/testify/require" ) -func TestLatestDirFromTempDir(t *testing.T) { - require.Equal(t, filepath.Join("tmp", "latest"), LatestDirFromTempDir("tmp")) +func TestTempDir(t *testing.T) { + require.Equal(t, filepath.Join("install", "tmp"), TempDir("install")) } -func TestBackupDirFromTempDir(t *testing.T) { - require.Equal(t, filepath.Join("tmp", "rollback"), BackupDirFromTempDir("tmp")) +func TestLatestDir(t *testing.T) { + require.Equal(t, filepath.Join("install", "tmp", "latest"), LatestDir("install")) +} + +func TestBackupDir(t *testing.T) { + require.Equal(t, filepath.Join("install", "tmp", "rollback"), BackupDir("install")) } func TestServiceFileDir(t *testing.T) { - installDir := filepath.Join("tmp", "rollback") - require.Equal(t, filepath.Join(installDir, "install"), ServiceFileDir(installDir)) + require.Equal(t, filepath.Join("install", "install"), ServiceFileDir("install")) } func TestBackupServiceFile(t *testing.T) { - serviceFileDir := filepath.Join("tmp", "rollback", "install") - require.Equal(t, filepath.Join(serviceFileDir, "backup.service"), BackupServiceFile(serviceFileDir)) + require.Equal(t, filepath.Join("install", "tmp", "rollback", "backup.service"), BackupServiceFile("install")) } func TestLogFile(t *testing.T) { - installDir := filepath.Join("install") - require.Equal(t, filepath.Join(installDir, "log", "updater.log"), LogFile(installDir)) + require.Equal(t, filepath.Join("install", "log", "updater.log"), LogFile("install")) } diff --git a/updater/internal/path/path_windows.go b/updater/internal/path/path_windows.go index 55009405f..f0787def1 100644 --- a/updater/internal/path/path_windows.go +++ b/updater/internal/path/path_windows.go @@ -23,8 +23,8 @@ import ( const defaultProductName = "observIQ Distro for OpenTelemetry Collector" -// InstallDirFromRegistry gets the installation dir of the given product from the Windows Registry -func InstallDirFromRegistry(logger *zap.Logger, productName string) (string, error) { +// installDirFromRegistry gets the installation dir of the given product from the Windows Registry +func installDirFromRegistry(logger *zap.Logger, productName string) (string, error) { // this key is created when installing using the MSI installer keyPath := fmt.Sprintf(`Software\Microsoft\Windows\CurrentVersion\Uninstall\%s`, productName) key, err := registry.OpenKey(registry.LOCAL_MACHINE, keyPath, registry.READ) @@ -49,5 +49,5 @@ func InstallDirFromRegistry(logger *zap.Logger, productName string) (string, err // InstallDir returns the filepath to the install directory func InstallDir(logger *zap.Logger) (string, error) { - return InstallDirFromRegistry(logger, defaultProductName) + return installDirFromRegistry(logger, defaultProductName) } diff --git a/updater/internal/path/path_windows_test.go b/updater/internal/path/path_windows_test.go new file mode 100644 index 000000000..330fab3dd --- /dev/null +++ b/updater/internal/path/path_windows_test.go @@ -0,0 +1,80 @@ +// Copyright observIQ, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build windows && integration + +package path + +import ( + "fmt" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" + "golang.org/x/sys/windows/registry" +) + +func TestInstallDirFromRegistry(t *testing.T) { + t.Run("Successfully grabs install dir from registry", func(t *testing.T) { + productName := "default-product-name" + installDir, err := filepath.Abs("C:/temp") + require.NoError(t, err) + + defer deleteInstallDirRegistryKey(t, productName) + createInstallDirRegistryKey(t, productName, installDir) + + dir, err := installDirFromRegistry(zaptest.NewLogger(t), productName) + require.NoError(t, err) + require.Equal(t, installDir+`\`, dir) + }) + + t.Run("Registry key does not exist", func(t *testing.T) { + productName := "default-product-name" + + _, err := installDirFromRegistry(zaptest.NewLogger(t), productName) + require.ErrorContains(t, err, "failed to open registry key") + }) +} + +func deleteInstallDirRegistryKey(t *testing.T, productName string) { + t.Helper() + + keyPath := fmt.Sprintf(`Software\Microsoft\Windows\CurrentVersion\Uninstall\%s`, productName) + key, err := registry.OpenKey(registry.LOCAL_MACHINE, keyPath, registry.WRITE) + if err != nil { + // Key may not exist, assume that's why we couldn't open it + return + } + defer key.Close() + + err = registry.DeleteKey(key, "") + require.NoError(t, err) +} + +func createInstallDirRegistryKey(t *testing.T, productName, installDir string) { + t.Helper() + + installDir, err := filepath.Abs(installDir) + require.NoError(t, err) + installDir += `\` + + keyPath := fmt.Sprintf(`Software\Microsoft\Windows\CurrentVersion\Uninstall\%s`, productName) + key, _, err := registry.CreateKey(registry.LOCAL_MACHINE, keyPath, registry.WRITE) + require.NoError(t, err) + defer key.Close() + + err = key.SetStringValue("InstallLocation", installDir) + require.NoError(t, err) +} diff --git a/updater/internal/rollback/mocks/action_appender.go b/updater/internal/rollback/mocks/action_appender.go index bc6226cef..fc25f4bfa 100644 --- a/updater/internal/rollback/mocks/action_appender.go +++ b/updater/internal/rollback/mocks/action_appender.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.13.1. DO NOT EDIT. +// Code generated by mockery v2.14.0. DO NOT EDIT. package mocks diff --git a/updater/internal/rollback/rollback.go b/updater/internal/rollback/rollback.go index e14e71d09..8724ba551 100644 --- a/updater/internal/rollback/rollback.go +++ b/updater/internal/rollback/rollback.go @@ -40,13 +40,12 @@ type Rollbacker struct { originalSvc service.Service backupDir string installDir string - tmpDir string actions []action.RollbackableAction logger *zap.Logger } // NewRollbacker returns a new Rollbacker -func NewRollbacker(logger *zap.Logger, tmpDir string) (*Rollbacker, error) { +func NewRollbacker(logger *zap.Logger, installDir string) (*Rollbacker, error) { namedLogger := logger.Named("rollbacker") installDir, err := path.InstallDir(namedLogger.Named("install-dir")) @@ -55,11 +54,10 @@ func NewRollbacker(logger *zap.Logger, tmpDir string) (*Rollbacker, error) { } return &Rollbacker{ - backupDir: path.BackupDirFromTempDir(tmpDir), + backupDir: path.BackupDir(installDir), installDir: installDir, - tmpDir: tmpDir, logger: namedLogger, - originalSvc: service.NewService(namedLogger, path.LatestDirFromTempDir(tmpDir)), + originalSvc: service.NewService(namedLogger, installDir), }, nil } @@ -77,12 +75,12 @@ func (r Rollbacker) Backup() error { } // Copy all the files in the install directory to the backup directory - if err := copyFiles(r.logger, r.installDir, r.backupDir, r.tmpDir); err != nil { + if err := backupFiles(r.logger, r.installDir, r.backupDir); err != nil { return fmt.Errorf("failed to copy files to backup dir: %w", err) } // Backup the service configuration so we can reload it in case of rollback - if err := r.originalSvc.Backup(path.ServiceFileDir(r.backupDir)); err != nil { + if err := r.originalSvc.Backup(); err != nil { return fmt.Errorf("failed to backup service configuration: %w", err) } @@ -103,14 +101,14 @@ func (r Rollbacker) Rollback() { } } -// copyFiles copies files from inputPath to output path, skipping tmpDir. -func copyFiles(logger *zap.Logger, inputPath, outputPath, tmpDir string) error { - absTmpDir, err := filepath.Abs(tmpDir) +// backupFiles copies files from installDir to output path, skipping tmpDir. +func backupFiles(logger *zap.Logger, installDir, outputPath string) error { + absTmpDir, err := filepath.Abs(path.TempDir(installDir)) if err != nil { return fmt.Errorf("failed to get absolute path for temporary directory: %w", err) } - err = filepath.WalkDir(inputPath, func(inPath string, d fs.DirEntry, err error) error { + err = filepath.WalkDir(installDir, func(inPath string, d fs.DirEntry, err error) error { fullPath, absErr := filepath.Abs(inPath) if absErr != nil { @@ -132,7 +130,7 @@ func copyFiles(logger *zap.Logger, inputPath, outputPath, tmpDir string) error { // We want the path relative to the directory we are walking in order to calculate where the file should be // mirrored in the output directory. - relPath, err := filepath.Rel(inputPath, inPath) + relPath, err := filepath.Rel(installDir, inPath) if err != nil { return err } diff --git a/updater/internal/rollback/rollback_test.go b/updater/internal/rollback/rollback_test.go index d3a293800..384c61359 100644 --- a/updater/internal/rollback/rollback_test.go +++ b/updater/internal/rollback/rollback_test.go @@ -35,13 +35,12 @@ func TestRollbackerBackup(t *testing.T) { installDir := filepath.Join("testdata", "rollbacker") svc := service_mocks.NewService(t) - svc.On("Backup", filepath.Join(outDir, "install")).Return(nil) + svc.On("Backup").Return(nil) rb := &Rollbacker{ originalSvc: svc, backupDir: outDir, installDir: installDir, - tmpDir: filepath.Join(installDir, "tmp-dir"), logger: zaptest.NewLogger(t), } @@ -58,13 +57,12 @@ func TestRollbackerBackup(t *testing.T) { installDir := filepath.Join("testdata", "rollbacker") svc := service_mocks.NewService(t) - svc.On("Backup", filepath.Join(outDir, "install")).Return(fmt.Errorf("invalid permissions")) + svc.On("Backup").Return(fmt.Errorf("invalid permissions")) rb := &Rollbacker{ originalSvc: svc, backupDir: outDir, installDir: installDir, - tmpDir: filepath.Join(installDir, "tmp-dir"), logger: zaptest.NewLogger(t), } @@ -78,7 +76,7 @@ func TestRollbackerBackup(t *testing.T) { leftoverFile := filepath.Join(outDir, "leftover-file.txt") svc := service_mocks.NewService(t) - svc.On("Backup", filepath.Join(outDir, "install")).Return(nil) + svc.On("Backup").Return(nil) err := os.WriteFile(leftoverFile, []byte("leftover file"), 0600) require.NoError(t, err) @@ -87,7 +85,6 @@ func TestRollbackerBackup(t *testing.T) { originalSvc: svc, backupDir: outDir, installDir: installDir, - tmpDir: filepath.Join(installDir, "tmp-dir"), logger: zaptest.NewLogger(t), } diff --git a/updater/internal/service/mocks/service.go b/updater/internal/service/mocks/service.go index c00ed0921..c2810171a 100644 --- a/updater/internal/service/mocks/service.go +++ b/updater/internal/service/mocks/service.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.13.1. DO NOT EDIT. +// Code generated by mockery v2.14.0. DO NOT EDIT. package mocks @@ -9,13 +9,13 @@ type Service struct { mock.Mock } -// Backup provides a mock function with given fields: outDir -func (_m *Service) Backup(outDir string) error { - ret := _m.Called(outDir) +// Backup provides a mock function with given fields: +func (_m *Service) Backup() error { + ret := _m.Called() var r0 error - if rf, ok := ret.Get(0).(func(string) error); ok { - r0 = rf(outDir) + if rf, ok := ret.Get(0).(func() error); ok { + r0 = rf() } else { r0 = ret.Error(0) } diff --git a/updater/internal/service/service.go b/updater/internal/service/service.go index ae4a1e491..fef849be6 100644 --- a/updater/internal/service/service.go +++ b/updater/internal/service/service.go @@ -32,8 +32,8 @@ type Service interface { // Updates the old service configuration to the new one Update() error - // Backup backs the current service configuration to the given directory - Backup(outDir string) error + // Backup backs the current service configuration + Backup() error } // replaceInstallDir replaces "[INSTALLDIR]" with the given installDir string. diff --git a/updater/internal/service/service_darwin.go b/updater/internal/service/service_darwin.go index d05ea6b28..908b97ef7 100644 --- a/updater/internal/service/service_darwin.go +++ b/updater/internal/service/service_darwin.go @@ -42,9 +42,9 @@ func WithServiceFile(svcFilePath string) Option { } // NewService returns an instance of the Service interface for managing the observiq-otel-collector service on the current OS. -func NewService(logger *zap.Logger, latestPath string, opts ...Option) Service { +func NewService(logger *zap.Logger, installDir string, opts ...Option) Service { darwinSvc := &darwinService{ - newServiceFilePath: filepath.Join(path.ServiceFileDir(latestPath), "com.observiq.collector.plist"), + newServiceFilePath: filepath.Join(path.ServiceFileDir(installDir), "com.observiq.collector.plist"), installedServiceFilePath: darwinServiceFilePath, installDir: path.DarwinInstallDir, logger: logger.Named("darwin-service"), @@ -139,8 +139,8 @@ func (d darwinService) Update() error { return nil } -func (d darwinService) Backup(outDir string) error { - if err := file.CopyFile(d.logger.Named("copy-file"), d.installedServiceFilePath, path.BackupServiceFile(outDir), false); err != nil { +func (d darwinService) Backup() error { + 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) } diff --git a/updater/internal/service/service_darwin_test.go b/updater/internal/service/service_darwin_test.go index 06b088d54..9778e3573 100644 --- a/updater/internal/service/service_darwin_test.go +++ b/updater/internal/service/service_darwin_test.go @@ -179,9 +179,13 @@ func TestDarwinServiceInstall(t *testing.T) { serviceFileContents, err := os.ReadFile(newServiceFile) require.NoError(t, err) + installDir := t.TempDir() + require.NoError(t, os.MkdirAll(path.BackupDir(installDir), 0775)) + d := &darwinService{ newServiceFilePath: newServiceFile, installedServiceFilePath: installedServicePath, + installDir: installDir, logger: zaptest.NewLogger(t), } @@ -194,12 +198,11 @@ func TestDarwinServiceInstall(t *testing.T) { require.NoError(t, d.Stop()) - backupServiceDir := t.TempDir() - err = d.Backup(backupServiceDir) + err = d.Backup() require.NoError(t, err) - require.FileExists(t, path.BackupServiceFile(backupServiceDir)) + require.FileExists(t, path.BackupServiceFile(installDir)) - backupServiceContents, err := os.ReadFile(path.BackupServiceFile(backupServiceDir)) + backupServiceContents, err := os.ReadFile(path.BackupServiceFile(installDir)) require.Equal(t, serviceFileContents, backupServiceContents) require.NoError(t, d.uninstall()) @@ -211,15 +214,17 @@ func TestDarwinServiceInstall(t *testing.T) { uninstallService(t, installedServicePath) newServiceFile := filepath.Join("testdata", "darwin-service.plist") + installDir := t.TempDir() + require.NoError(t, os.MkdirAll(path.BackupDir(installDir), 0775)) d := &darwinService{ newServiceFilePath: newServiceFile, installedServiceFilePath: installedServicePath, + installDir: installDir, logger: zaptest.NewLogger(t), } - backupServiceDir := t.TempDir() - err := d.Backup(backupServiceDir) + err := d.Backup() require.ErrorContains(t, err, "failed to copy service file") }) @@ -230,10 +235,14 @@ func TestDarwinServiceInstall(t *testing.T) { newServiceFile := filepath.Join("testdata", "darwin-service.plist") + installDir := t.TempDir() + require.NoError(t, os.MkdirAll(path.BackupDir(installDir), 0775)) + d := &darwinService{ newServiceFilePath: newServiceFile, installedServiceFilePath: installedServicePath, logger: zaptest.NewLogger(t), + installDir: installDir, } err := d.install() @@ -245,12 +254,11 @@ func TestDarwinServiceInstall(t *testing.T) { require.NoError(t, d.Stop()) - backupServiceDir := t.TempDir() // Write the backup file before creating it; Backup should // not ever overwrite an existing file - os.WriteFile(path.BackupServiceFile(backupServiceDir), []byte("file exists"), 0600) + os.WriteFile(path.BackupServiceFile(installDir), []byte("file exists"), 0600) - err = d.Backup(backupServiceDir) + err = d.Backup() require.ErrorContains(t, err, "failed to copy service file") }) } diff --git a/updater/internal/service/service_linux.go b/updater/internal/service/service_linux.go index 7b9e8425b..2538f79d0 100644 --- a/updater/internal/service/service_linux.go +++ b/updater/internal/service/service_linux.go @@ -43,11 +43,12 @@ func WithServiceFile(svcFilePath string) Option { } // NewService returns an instance of the Service interface for managing the observiq-otel-collector service on the current OS. -func NewService(logger *zap.Logger, latestPath string, opts ...Option) Service { +func NewService(logger *zap.Logger, installDir string, opts ...Option) Service { linuxSvc := &linuxService{ - newServiceFilePath: filepath.Join(path.ServiceFileDir(latestPath), "observiq-otel-collector.service"), + newServiceFilePath: filepath.Join(path.ServiceFileDir(installDir), "observiq-otel-collector.service"), serviceName: linuxServiceName, installedServiceFilePath: linuxServiceFilePath, + installDir: installDir, logger: logger.Named("linux-service"), } @@ -65,6 +66,7 @@ type linuxService struct { serviceName string // installedServiceFilePath is the file path to the installed unit file installedServiceFilePath string + installDir string logger *zap.Logger } @@ -162,8 +164,8 @@ func (l linuxService) Update() error { return nil } -func (l linuxService) Backup(outDir string) error { - if err := file.CopyFile(l.logger.Named("copy-file"), l.installedServiceFilePath, path.BackupServiceFile(outDir), false); err != nil { +func (l linuxService) Backup() error { + if err := file.CopyFile(l.logger.Named("copy-file"), l.installedServiceFilePath, path.BackupServiceFile(l.installDir), false); err != nil { return fmt.Errorf("failed to copy service file: %w", err) } diff --git a/updater/internal/service/service_linux_test.go b/updater/internal/service/service_linux_test.go index 937638f1d..7bc8f6922 100644 --- a/updater/internal/service/service_linux_test.go +++ b/updater/internal/service/service_linux_test.go @@ -178,10 +178,14 @@ func TestLinuxServiceInstall(t *testing.T) { serviceFileContents, err := os.ReadFile(newServiceFile) require.NoError(t, err) + installDir := t.TempDir() + require.NoError(t, os.MkdirAll(path.BackupDir(installDir), 0775)) + d := &linuxService{ newServiceFilePath: newServiceFile, installedServiceFilePath: installedServicePath, serviceName: "linux-service", + installDir: installDir, logger: zaptest.NewLogger(t), } @@ -192,12 +196,11 @@ func TestLinuxServiceInstall(t *testing.T) { // We want to check that the service was actually loaded requireServiceLoadedStatus(t, true) - backupServiceDir := t.TempDir() - err = d.Backup(backupServiceDir) + err = d.Backup() require.NoError(t, err) - require.FileExists(t, path.BackupServiceFile(backupServiceDir)) + require.FileExists(t, path.BackupServiceFile(installDir)) - backupServiceContents, err := os.ReadFile(path.BackupServiceFile(backupServiceDir)) + backupServiceContents, err := os.ReadFile(path.BackupServiceFile(installDir)) require.Equal(t, serviceFileContents, backupServiceContents) require.NoError(t, d.uninstall()) @@ -209,15 +212,18 @@ func TestLinuxServiceInstall(t *testing.T) { newServiceFile := filepath.Join("testdata", "linux-service.service") + installDir := t.TempDir() + require.NoError(t, os.MkdirAll(path.BackupDir(installDir), 0775)) + d := &linuxService{ newServiceFilePath: newServiceFile, installedServiceFilePath: installedServicePath, serviceName: "linux-service", + installDir: installDir, logger: zaptest.NewLogger(t), } - backupServiceDir := t.TempDir() - err := d.Backup(backupServiceDir) + err := d.Backup() require.ErrorContains(t, err, "failed to copy service file") }) @@ -227,10 +233,14 @@ func TestLinuxServiceInstall(t *testing.T) { newServiceFile := filepath.Join("testdata", "linux-service.service") + installDir := t.TempDir() + require.NoError(t, os.MkdirAll(path.BackupDir(installDir), 0775)) + d := &linuxService{ newServiceFilePath: newServiceFile, installedServiceFilePath: installedServicePath, serviceName: "linux-service", + installDir: installDir, logger: zaptest.NewLogger(t), } @@ -241,12 +251,11 @@ func TestLinuxServiceInstall(t *testing.T) { // We want to check that the service was actually loaded requireServiceLoadedStatus(t, true) - backupServiceDir := t.TempDir() // Write the backup file before creating it; Backup should // not ever overwrite an existing file - os.WriteFile(path.BackupServiceFile(backupServiceDir), []byte("file exists"), 0600) + os.WriteFile(path.BackupServiceFile(installDir), []byte("file exists"), 0600) - err = d.Backup(backupServiceDir) + err = d.Backup() require.ErrorContains(t, err, "failed to copy service file") }) } diff --git a/updater/internal/service/service_windows.go b/updater/internal/service/service_windows.go index fb6e7f09d..a79f8815a 100644 --- a/updater/internal/service/service_windows.go +++ b/updater/internal/service/service_windows.go @@ -51,9 +51,9 @@ func WithServiceFile(svcFilePath string) Option { } // NewService returns an instance of the Service interface for managing the observiq-otel-collector service on the current OS. -func NewService(logger *zap.Logger, latestPath string, opts ...Option) Service { +func NewService(logger *zap.Logger, installDir string, opts ...Option) Service { winSvc := &windowsService{ - newServiceFilePath: filepath.Join(path.ServiceFileDir(latestPath), "windows_service.json"), + newServiceFilePath: filepath.Join(path.ServiceFileDir(installDir), "windows_service.json"), serviceName: defaultServiceName, productName: defaultProductName, logger: logger.Named("windows-service"), @@ -73,6 +73,7 @@ type windowsService struct { serviceName string // productName is the name of the installed product productName string + installDir string logger *zap.Logger } @@ -126,14 +127,8 @@ func (w windowsService) install() error { return fmt.Errorf("failed to read service config: %w", err) } - // fetch the install directory so that we can determine the binary path that we need to execute - iDir, err := path.InstallDirFromRegistry(w.logger.Named("install-dir"), w.productName) - if err != nil { - return fmt.Errorf("failed to get install dir: %w", err) - } - // expand the arguments to be properly formatted (expand [INSTALLDIR], clean '"' to be '"') - expandArguments(wsc, iDir) + expandArguments(wsc, w.installDir) // Split the arguments; Arguments are "shell-like", in that they may contain spaces, and can be quoted to indicate that. splitArgs, err := shellquote.Split(wsc.Service.Arguments) @@ -155,7 +150,7 @@ func (w windowsService) install() error { // Create the service using the service manager. s, err := m.CreateService(w.serviceName, - filepath.Join(iDir, wsc.Path), + filepath.Join(w.installDir, wsc.Path), mgr.Config{ Description: wsc.Service.Description, DisplayName: wsc.Service.DisplayName, @@ -233,7 +228,7 @@ func (w windowsService) Update() error { return nil } -func (w windowsService) Backup(outDir string) error { +func (w windowsService) Backup() error { wsc, err := w.currentServiceConfig() if err != nil { @@ -247,7 +242,7 @@ func (w windowsService) Backup(outDir string) error { } // Open with O_EXCL to fail if the file already exists - f, err := os.OpenFile(path.BackupServiceFile(outDir), os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600) + f, err := os.OpenFile(path.BackupServiceFile(w.installDir), os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600) if err != nil { return fmt.Errorf("failed to create backup service file: %w", err) } @@ -335,13 +330,8 @@ func (w windowsService) currentServiceConfig() (*windowsServiceConfig, error) { return nil, fmt.Errorf("failed to split service BinaryPathName: %w", err) } - iDir, err := path.InstallDirFromRegistry(w.logger.Named("install-dir"), w.productName) - if err != nil { - return nil, fmt.Errorf("failed to get install dir: %w", err) - } - // In the original config, the Path is the main binary path, relative to the install directory. - binaryPath, err := filepath.Rel(iDir, fullBinaryPath) + binaryPath, err := filepath.Rel(w.installDir, fullBinaryPath) if err != nil { return nil, fmt.Errorf("could not find service exe relative to install dir: %w", err) } diff --git a/updater/internal/service/service_windows_test.go b/updater/internal/service/service_windows_test.go index 11fb64b75..a2f49f4ab 100644 --- a/updater/internal/service/service_windows_test.go +++ b/updater/internal/service/service_windows_test.go @@ -27,7 +27,6 @@ import ( "time" "go.uber.org/zap/zaptest" - "golang.org/x/sys/windows/registry" "github.com/observiq/observiq-otel-collector/updater/internal/path" "github.com/stretchr/testify/assert" @@ -50,13 +49,12 @@ func TestWindowsServiceInstall(t *testing.T) { compileProgram(t, serviceGoFile, testServiceProgram) defer uninstallService(t) - createInstallDirRegistryKey(t, testProductName, tempDir) - defer deleteInstallDirRegistryKey(t, testProductName) w := &windowsService{ newServiceFilePath: serviceJSON, serviceName: "windows-service", productName: testProductName, + installDir: tempDir, logger: zaptest.NewLogger(t), } @@ -100,13 +98,12 @@ func TestWindowsServiceInstall(t *testing.T) { compileProgram(t, serviceGoFile, testServiceProgram) defer uninstallService(t) - createInstallDirRegistryKey(t, testProductName, tempDir) - defer deleteInstallDirRegistryKey(t, testProductName) w := &windowsService{ newServiceFilePath: serviceJSON, serviceName: "windows-service", productName: testProductName, + installDir: tempDir, logger: zaptest.NewLogger(t), } @@ -149,13 +146,12 @@ func TestWindowsServiceInstall(t *testing.T) { compileProgram(t, serviceGoFile, testServiceProgram) defer uninstallService(t) - createInstallDirRegistryKey(t, testProductName, tempDir) - defer deleteInstallDirRegistryKey(t, testProductName) w := &windowsService{ newServiceFilePath: serviceJSON, serviceName: "windows-service", productName: testProductName, + installDir: tempDir, logger: zaptest.NewLogger(t), } @@ -195,13 +191,12 @@ func TestWindowsServiceInstall(t *testing.T) { compileProgram(t, serviceGoFile, testServiceProgram) defer uninstallService(t) - createInstallDirRegistryKey(t, testProductName, tempDir) - defer deleteInstallDirRegistryKey(t, testProductName) w := &windowsService{ newServiceFilePath: filepath.Join(tempDir, "not-a-valid-service.json"), serviceName: "windows-service", productName: testProductName, + installDir: tempDir, logger: zaptest.NewLogger(t), } @@ -219,6 +214,7 @@ func TestWindowsServiceInstall(t *testing.T) { w := &windowsService{ newServiceFilePath: serviceJSON, serviceName: "windows-service", + installDir: tempDir, productName: testProductName, } @@ -237,6 +233,7 @@ func TestWindowsServiceInstall(t *testing.T) { newServiceFilePath: serviceJSON, serviceName: "windows-service", productName: testProductName, + installDir: tempDir, logger: zaptest.NewLogger(t), } @@ -254,6 +251,7 @@ func TestWindowsServiceInstall(t *testing.T) { newServiceFilePath: serviceJSON, serviceName: "windows-service", productName: testProductName, + installDir: tempDir, logger: zaptest.NewLogger(t), } @@ -263,11 +261,9 @@ func TestWindowsServiceInstall(t *testing.T) { t.Run("Test backup works", func(t *testing.T) { tempDir := t.TempDir() - installDir := filepath.Join(tempDir, "install directory") - backupDir := filepath.Join(tempDir, "backup") - - require.NoError(t, os.MkdirAll(installDir, 0775)) - require.NoError(t, os.MkdirAll(backupDir, 0775)) + installDir, err := filepath.Abs(filepath.Join(tempDir, "install directory")) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(path.BackupDir(installDir), 0775)) testProductName := "Test Product" @@ -280,13 +276,12 @@ func TestWindowsServiceInstall(t *testing.T) { compileProgram(t, serviceGoFile, testServiceProgram) defer uninstallService(t) - createInstallDirRegistryKey(t, testProductName, installDir) - defer deleteInstallDirRegistryKey(t, testProductName) w := &windowsService{ newServiceFilePath: serviceJSON, serviceName: "windows-service", productName: testProductName, + installDir: installDir, logger: zaptest.NewLogger(t), } @@ -312,10 +307,10 @@ func TestWindowsServiceInstall(t *testing.T) { // Take a backup; Assert the backup makes sense. // It will not be the same as the original service file due to expansion of INSTALLDIR // which is OK and expected. - err = w.Backup(backupDir) + err = w.Backup() require.NoError(t, err) - backupSvcFile := path.BackupServiceFile(backupDir) + backupSvcFile := path.BackupServiceFile(installDir) svcCfg, err := readWindowsServiceConfig(backupSvcFile) require.NoError(t, err) @@ -489,37 +484,6 @@ func writeServiceFile(t *testing.T, outPath, inPath, serviceGoPath string) { require.NoError(t, err) } -func deleteInstallDirRegistryKey(t *testing.T, productName string) { - t.Helper() - - keyPath := fmt.Sprintf(`Software\Microsoft\Windows\CurrentVersion\Uninstall\%s`, productName) - key, err := registry.OpenKey(registry.LOCAL_MACHINE, keyPath, registry.WRITE) - if err != nil { - // Key may not exist, assume that's why we couldn't open it - return - } - defer key.Close() - - err = registry.DeleteKey(key, "") - require.NoError(t, err) -} - -func createInstallDirRegistryKey(t *testing.T, productName, installDir string) { - t.Helper() - - installDir, err := filepath.Abs(installDir) - require.NoError(t, err) - installDir += `\` - - keyPath := fmt.Sprintf(`Software\Microsoft\Windows\CurrentVersion\Uninstall\%s`, productName) - key, _, err := registry.CreateKey(registry.LOCAL_MACHINE, keyPath, registry.WRITE) - require.NoError(t, err) - defer key.Close() - - err = key.SetStringValue("InstallLocation", installDir) - require.NoError(t, err) -} - func compileProgram(t *testing.T, inPath, outPath string) { t.Helper() diff --git a/updater/internal/state/monitor.go b/updater/internal/state/monitor.go index d99b9897d..834535b81 100644 --- a/updater/internal/state/monitor.go +++ b/updater/internal/state/monitor.go @@ -23,7 +23,6 @@ import ( "time" "github.com/observiq/observiq-otel-collector/packagestate" - "github.com/observiq/observiq-otel-collector/updater/internal/path" "github.com/open-telemetry/opamp-go/protobufs" "go.uber.org/zap" ) @@ -51,13 +50,8 @@ type CollectorMonitor struct { } // NewCollectorMonitor create a new Monitor specifically for the collector -func NewCollectorMonitor(logger *zap.Logger) (Monitor, error) { +func NewCollectorMonitor(logger *zap.Logger, installDir string) (Monitor, error) { namedLogger := logger.Named("collector-monitor") - // Get install directory - installDir, err := path.InstallDir(namedLogger.Named("install-dir")) - if err != nil { - return nil, fmt.Errorf("failed to determine install directory: %w", err) - } // Create a collector monitor packageStatusPath := filepath.Join(installDir, packagestate.DefaultFileName) @@ -66,6 +60,7 @@ func NewCollectorMonitor(logger *zap.Logger) (Monitor, error) { } // Load the current status to ensure the package status file exists + var err error collectorMonitor.currentStatus, err = collectorMonitor.stateManager.LoadStatuses() if err != nil { return nil, fmt.Errorf("failed to load package statues: %w", err)