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

StefanKurek · 2022-07-28T22:20:54Z

Proposed Change

Ensures JMX jar is both installed and rolled back properly
Also making sure that we use the backed up file permissions when
rolling back a file that no longer exists in the install directory
Updated some tests that were extremely hard to debug.

Checklist

Changes are tested
CI has passed

cpheps

A lot of this is over my head. I left comments from a code standpoint of what made sense. I'm not quite sure about all the file ops.

updater/internal/file/file.go

updater/internal/install/install.go

cpheps

Tested these changes on all 3 OSes and they work.

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

BinaryFissionGames · 2022-07-29T14:02:23Z

updater/internal/file/file.go

+		// 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()
+			}
+		}


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.

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

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

Backup - We want to preserve the original file's permissions on the backed up file

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).

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).

BinaryFissionGames · 2022-07-29T14:10:37Z

updater/internal/file/file.go

+
+		// 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()


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.

BinaryFissionGames · 2022-07-29T14:17:57Z

updater/internal/rollback/rollback.go

+		if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, false, false); err != nil {
 			return fmt.Errorf("failed to copy file: %w", err)
 		}


I know this doesn't matter (see above comment about useInFilePermBackup = true and overwrite = false), but I'd expect useInFilePermBackup to be set to true here when backing up a file.

BinaryFissionGames · 2022-07-29T14:18:14Z

updater/internal/rollback/rollback.go

+	}
+
+	// Fail if copying the input file to the output file would fail
+	if err := file.CopyFile(logger.Named("copy-file"), inPath, outPath, false, false); err != nil {


Same here, i'd assume the last parameter to be true.

BinaryFissionGames · 2022-07-29T14:18:41Z

updater/internal/service/service_darwin.go

@@ -140,7 +140,7 @@ func (d darwinService) Update() error {
 }

 func (d darwinService) Backup() error {
-	if err := file.CopyFile(d.logger.Named("copy-file"), d.installedServiceFilePath, path.BackupServiceFile(d.installDir), false); err != nil {
+	if err := file.CopyFile(d.logger.Named("copy-file"), d.installedServiceFilePath, path.BackupServiceFile(d.installDir), false, false); err != nil {


Same here, I'd expect the last parameter to be true.

BinaryFissionGames · 2022-07-29T14:18:55Z

updater/internal/service/service_linux.go

@@ -165,7 +165,7 @@ func (l linuxService) Update() error {
 }

 func (l linuxService) Backup() error {
-	if err := file.CopyFile(l.logger.Named("copy-file"), l.installedServiceFilePath, path.BackupServiceFile(l.installDir), false); err != nil {
+	if err := file.CopyFile(l.logger.Named("copy-file"), l.installedServiceFilePath, path.BackupServiceFile(l.installDir), false, false); err != nil {


Same here, I'd expect the last parameter to be true.

BinaryFissionGames

Talked through the logic, makes sense, might want to come back for a refactor on this later.

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

* feat: Add windows service definition to archive (#515) * feat: Create Updater artifact (#529) * add new binary to everything but windows * add to windows msi * add version flag to updater * build using combined target * fix manual build * add license header * break updater into separate module * add new module to dependabot * copy version pkg to new updater internal pkg To not have a dependency on the root module * Workaround for securego/gosec#501 * feat: Add tarball download + unarchiving to updater (#538) * Add download and content-hash verification to updater * add a couple more tests for some edge cases * lint * gosec errors * fix defer f.close properly * fix tests on windows * more windows specific testing * fix final test failure on windows * more line-ending test fixes * feat: Added OpAMP PackageStatuses functionality & basic response to PackagesAvailable (#550) * Added OpAMP PackageStatuses functionality & basic response to PackagesAvailable * Add new data model for marshal/unmarshaling OpAMP package statuses. * feat: Add ability to install unpacked artifacts in updater (#562) * start artifact install logic * fix uninstall service step * add tests for windows service manager * remove kardiano/service dependency * check filepath with spaces * more tests, hook up to main * naming * add licenses * gosec fixes * linux gosec + some lint issues * linter * fix formatting of windows service test * actually fix formatting * guard linux/win service tests behind tag * run tests as sudo on linux * fix inverted conditional * split updater integration tests into separate target * refactor package for better encapsulation * update darwin service to load/unload for start/stop * fix installDir for windows after rename * test replaceInstallDir * add license to service_test.go * fix make target phony * add some comments * add start of readme * add a (very basic) readme * use switch instead of multiple ifs * Add comments to moveFiles * fix failing darwin test * Moved code to download, verify, and extract OpAMP package file from updater to collector (#565) * OpAmp Package Update Glue (#567) Switched PackageStatuses yaml to a JSON file to prevent partial reads by Updater. Removed excess fields for package status. We should be able to communicate with available status and error message. If just started an install, will prevent another PackagesAvailable message from starting another install. If OpAMP client errors out at any point, sets the status to failed with an error message (if possible) in the JSON file. This will allow the updater to quickly shut down the collector and start up the rollback one (which will then send the message to BindPlane). On BindPlane connect, will check if the status is installing. If so, will check if Server version matches current version. Based on this will either set status to success or fail and write to JSON file for BindPlane to notice. It should only try to send a message immediately to BindPlane if it was a success. * Moved package install function to goroutine * Add mutex for updatingClient flag in client (#570) * Created packagestate module (#579) * Broke package status objects into their own file Signed-off-by: Corbin Phelps <[email protected]> * Updated main module to reference packagestate module Signed-off-by: Corbin Phelps <[email protected]> * Fixed licsense check for new module Signed-off-by: Corbin Phelps <[email protected]> * Created interface and mocks for package state manager Signed-off-by: Corbin Phelps <[email protected]> * Changed PackageStateProvider to use interface of StateManager Signed-off-by: Corbin Phelps <[email protected]> * Fixed up linux test for package state manager Signed-off-by: Corbin Phelps <[email protected]> * feat: Updater rollback (#584) * start rollback * more wip * more tests * add licenses, more testing * split out action stuff to separate package, more testing Needed to do this due to circular deps in mocks * move service test data * fix up darwin tests * Fix linux service to fit new service interface * fix windows service (todo: tests) * add windows backup test * fix service action pointin to wrong file * Logic for Updater to monitor Collector Status (#581) * Added default file name into package state to be accessed by updater Signed-off-by: Corbin Phelps <[email protected]> * Added logic to monitor status of collector from updater Signed-off-by: Corbin Phelps <[email protected]> * Added tests and fixuped some ci-checks issues Signed-off-by: Corbin Phelps <[email protected]> * Ran make add-license Signed-off-by: Corbin Phelps <[email protected]> * Added mocks for updater state monitor Signed-off-by: Corbin Phelps <[email protected]> * Pre-PR fixups Signed-off-by: Corbin Phelps <[email protected]> * Modified monitor state logic to be more flexible on errors Signed-off-by: Corbin Phelps <[email protected]> * fix revive linting errors * fix windows gosec error * update gosec to ignore test program * refactor CopyFile to allow failure on overwrite * refactor file action to take relative dir * add interface enforcement to actions * add nosec to open func * split windows service backup function into a few functions Co-authored-by: Corbin Phelps <[email protected]> * feat: Updater logging (#589) * add zap logging * add log level flag * add license headers * lint fixes * remove unimplemented comment * skip NewLogger test on windows * remove ability to specify level * remove rotation * remove copyFiles receiver * remove stringer implementation * remove previous log file on logger creation * tidy go mod * re-add stringer for copy file action * feat: Collector starts up Updater (#590) * Adds ability to start Updater and monitor it for failure * Fixes new collector erroring on execution after it is copied * Added KillMode=process to the linux service file in order to orphan the updater * Added disconnection flag to avoid failure messages in graceful shutdown * Added linux service file to tarball Co-authored-by: Corbin Phelps <[email protected]> * Fixed go.sum Signed-off-by: Corbin Phelps <[email protected]> * 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 * Fixed name of package updater looks at (#592) Signed-off-by: Corbin Phelps <[email protected]> * feat: Copy updater executable to CWD of collector before executing (#594) * move updater to CWD before running * fix darwin, add test * fix windows + windows tests * make tests parallel for updater manager * gosec * fix: Windows updater log fix (#595) * Added os specific log path Signed-off-by: Corbin Phelps <[email protected]> * make tests run on windows * make fmt, fix function redefinitions * reduce diff * add license Co-authored-by: Corbin Phelps <[email protected]> * feat: Updater cleans up temporary directory (#596) * remove tmpdir on rollback or update success * remove temp directory in failure scenarios * comment why we use a noop logger for failure * move installer creation to where it's actually used * fix redundant calls to removeTmpDir * fix: Pass install dir into service (#598) * pass install dir into service * pass install dir to service update action service * 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 * feat: Harden collector shutdown while updating (#597) * change service timeouts * update non-windows with new timeout * fix windows test * stop the service before rollback * fix install tests * fix: If the collector detects an error updating, clean temporary directory (#601) * Have the collector clean artifacts if update fails early * fix client tests * Updated Makefile & GitHub Action workflow so Updater binary has license scans (#604) * Fixes tmp dir for update to have 0700 permissions (#609) * fix(updater): Do Update in place for windows service (#605) * do Update in place for windows service * add a few comments * feat: Refactor updater main (#608) * refactor main; tests WIP * add tests for Updater * fix lint * add license * rename installer and rollbacker to avoid confusion w/ interface * final debug log to info log * empty commit for testing * fix(updater): Enable debug logs (#613) * feat: Refactor Updater's file package (#611) * break CopyFile into separate functions * break overwrite flag into two functions * fix comment for CopyFileOverwrite * small tweaks * tests for file package * fix linux build * remove todo * explain why we continue even on error. * empty commit for testing * Added better logging/messaging around collector package updating (#614) Co-authored-by: Brandon Johnson <[email protected]> Co-authored-by: Brandon Johnson <[email protected]> Co-authored-by: Corbin Phelps <[email protected]>

StefanKurek requested review from a team, armstrmi and dmikolay and removed request for a team July 28, 2022 22:20

StefanKurek force-pushed the fix/updater_jmx_jar_handling branch from d9a8fce to 3a3ef70 Compare July 28, 2022 22:41

cpheps reviewed Jul 29, 2022

View reviewed changes

updater/internal/file/file.go Outdated Show resolved Hide resolved

updater/internal/file/file.go Outdated Show resolved Hide resolved

updater/internal/install/install.go Outdated Show resolved Hide resolved

StefanKurek force-pushed the fix/updater_jmx_jar_handling branch 2 times, most recently from a2d7927 to 617d979 Compare July 29, 2022 00:45

cpheps approved these changes Jul 29, 2022

View reviewed changes

cpheps force-pushed the opamp-upgrade branch from f3c4c2c to 25887d1 Compare July 29, 2022 12:05

Updater properly installs and rollbacks JMX Jar.

8f74c39

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

StefanKurek force-pushed the fix/updater_jmx_jar_handling branch from 617d979 to 8f74c39 Compare July 29, 2022 13:42

BinaryFissionGames reviewed Jul 29, 2022

View reviewed changes

BinaryFissionGames self-requested a review July 29, 2022 15:05

BinaryFissionGames approved these changes Jul 29, 2022

View reviewed changes

StefanKurek merged commit 7545862 into opamp-upgrade Jul 29, 2022

StefanKurek deleted the fix/updater_jmx_jar_handling branch July 29, 2022 15:10

cpheps pushed a commit that referenced this pull request Jul 29, 2022

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

0d168f7

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

cpheps pushed a commit that referenced this pull request Aug 1, 2022

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

1038072

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

StefanKurek added a commit that referenced this pull request Aug 4, 2022

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

33857be

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

BinaryFissionGames pushed a commit that referenced this pull request Aug 4, 2022

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

133ed52

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

StefanKurek commented Jul 28, 2022

cpheps left a comment

cpheps left a comment

BinaryFissionGames Jul 29, 2022

cpheps Jul 29, 2022

StefanKurek Jul 29, 2022

BinaryFissionGames Jul 29, 2022

BinaryFissionGames Jul 29, 2022

BinaryFissionGames Jul 29, 2022

BinaryFissionGames Jul 29, 2022

BinaryFissionGames Jul 29, 2022

BinaryFissionGames left a comment

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

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

Conversation

StefanKurek commented Jul 28, 2022

Proposed Change

Checklist

cpheps left a comment

Choose a reason for hiding this comment

cpheps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BinaryFissionGames left a comment

Choose a reason for hiding this comment