Merge branch 'main' into becca/tuf-rollout-cleanup

pkg/autoupdate/tuf/autoupdate.go

@@ -51,7 +51,6 @@ type librarian interface {
 	Available(binary autoupdatableBinary, targetFilename string) bool
 	AddToLibrary(binary autoupdatableBinary, currentVersion string, targetFilename string, targetMetadata data.TargetFileMeta) error
 	TidyLibrary(binary autoupdatableBinary, currentVersion string)
-	Close() error
 }
 
 type querier interface {
@@ -218,9 +217,6 @@ func (ta *TufAutoupdater) Execute() (err error) {
 }
 
 func (ta *TufAutoupdater) Interrupt(_ error) {
-	if err := ta.libraryManager.Close(); err != nil {
-		level.Debug(ta.logger).Log("msg", "could not close library on interrupt", "err", err)
-	}
 	ta.interrupt <- struct{}{}
 }
 

pkg/autoupdate/tuf/autoupdate_test.go

@@ -206,10 +206,8 @@ func TestExecute_osquerydUpdate(t *testing.T) {
 	require.Contains(t, logLines[len(logLines)-1], "restarted binary after update")
 
 	// The autoupdater won't stop after an osqueryd download, so interrupt it and let it shut down
-	mockLibraryManager.On("Close").Return(nil)
 	autoupdater.Interrupt(errors.New("test error"))
 	time.Sleep(1 * time.Second)
-	mockLibraryManager.AssertExpectations(t)
 }
 
 func TestExecute_withInitialDelay(t *testing.T) {
@@ -246,7 +244,6 @@ func TestExecute_withInitialDelay(t *testing.T) {
 
 	// Expect that we interrupt
 	mockLibraryManager := NewMocklibrarian(t)
-	mockLibraryManager.On("Close").Return(nil)
 	autoupdater.libraryManager = mockLibraryManager
 
 	// Let the autoupdater run for less than the initial delay
@@ -357,7 +354,6 @@ func Test_storeError(t *testing.T) {
 	// We only expect TidyLibrary to run for osqueryd, since we can't get the current running version
 	// for launcher in tests.
 	mockLibraryManager.On("TidyLibrary", binaryOsqueryd, mock.Anything).Return().Once()
-	mockLibraryManager.On("Close").Return(nil)
 
 	// Set the check interval to something short so we can accumulate some errors
 	autoupdater.checkInterval = 1 * time.Second

pkg/autoupdate/tuf/library_manager.go

@@ -31,7 +31,6 @@ type updateLibraryManager struct {
 	mirrorUrl    string // dl.kolide.co
 	mirrorClient *http.Client
 	baseDir      string
-	stagingDir   string
 	lock         *libraryLock
 	logger       log.Logger
 }
@@ -50,13 +49,6 @@ func newUpdateLibraryManager(mirrorUrl string, mirrorClient *http.Client, baseDi
 		return nil, fmt.Errorf("could not make base directory for updates library: %w", err)
 	}
 
-	// Create the directory for staging updates
-	stagingDir, err := agent.MkdirTemp("staged-updates")
-	if err != nil {
-		return nil, fmt.Errorf("could not make staged updates directory: %w", err)
-	}
-	ulm.stagingDir = stagingDir
-
 	// Create the update library
 	for _, binary := range binaries {
 		if err := os.MkdirAll(updatesDirectory(binary, baseDir), 0755); err != nil {
@@ -67,20 +59,6 @@ func newUpdateLibraryManager(mirrorUrl string, mirrorClient *http.Client, baseDi
 	return &ulm, nil
 }
 
-// Close cleans up the temporary staging directory
-func (ulm *updateLibraryManager) Close() error {
-	// Acquire lock to ensure we aren't interrupting an ongoing operation
-	for _, binary := range binaries {
-		ulm.lock.Lock(binary)
-		defer ulm.lock.Unlock(binary)
-	}
-
-	if err := os.RemoveAll(ulm.stagingDir); err != nil {
-		return fmt.Errorf("could not remove staging dir %s: %w", ulm.stagingDir, err)
-	}
-	return nil
-}
-
 // updatesDirectory returns the update library location for the given binary.
 func updatesDirectory(binary autoupdatableBinary, baseUpdateDirectory string) string {
 	return filepath.Join(baseUpdateDirectory, string(binary))
@@ -115,14 +93,19 @@ func (ulm *updateLibraryManager) AddToLibrary(binary autoupdatableBinary, curren
 		return nil
 	}
 
-	// Remove downloaded archives after update, regardless of success -- this will run before the unlock
-	defer ulm.tidyStagedUpdates(binary)
-
 	stagedUpdatePath, err := ulm.stageAndVerifyUpdate(binary, targetFilename, targetMetadata)
 	if err != nil {
 		return fmt.Errorf("could not stage update: %w", err)
 	}
 
+	// Remove downloaded archives after update, regardless of success -- this will run before the unlock
+	defer func() {
+		dirToRemove := filepath.Dir(stagedUpdatePath)
+		if err := os.RemoveAll(dirToRemove); err != nil {
+			level.Debug(ulm.logger).Log("msg", "could not remove temp staging directory", "err", err, "directory", dirToRemove)
+		}
+	}()
+
 	if err := ulm.moveVerifiedUpdate(binary, targetFilename, stagedUpdatePath); err != nil {
 		return fmt.Errorf("could not move verified update: %w", err)
 	}
@@ -133,7 +116,11 @@ func (ulm *updateLibraryManager) AddToLibrary(binary autoupdatableBinary, curren
 // stageAndVerifyUpdate downloads the update indicated by `targetFilename` and verifies it against
 // the given, validated local metadata.
 func (ulm *updateLibraryManager) stageAndVerifyUpdate(binary autoupdatableBinary, targetFilename string, localTargetMetadata data.TargetFileMeta) (string, error) {
-	stagedUpdatePath := filepath.Join(ulm.stagingDir, targetFilename)
+	stagingDir, err := agent.MkdirTemp(fmt.Sprintf("staged-updates-%s", versionFromTarget(binary, targetFilename)))
+	if err != nil {
+		return "", fmt.Errorf("could not create temporary directory for downloading target: %w", err)
+	}
+	stagedUpdatePath := filepath.Join(stagingDir, targetFilename)
 
 	// Request download from mirror
 	downloadPath := path.Join("/", "kolide", string(binary), runtime.GOOS, PlatformArch(), targetFilename)
@@ -178,9 +165,9 @@ func (ulm *updateLibraryManager) stageAndVerifyUpdate(binary autoupdatableBinary
 // Finally, it moves the update to its correct versioned location in the update library for the given binary.
 func (ulm *updateLibraryManager) moveVerifiedUpdate(binary autoupdatableBinary, targetFilename string, stagedUpdate string) error {
 	targetVersion := versionFromTarget(binary, targetFilename)
-	stagedVersionedDirectory := filepath.Join(ulm.stagingDir, targetVersion)
-	if err := os.MkdirAll(stagedVersionedDirectory, 0755); err != nil {
-		return fmt.Errorf("could not create directory %s for untarring and validating new update: %w", stagedVersionedDirectory, err)
+	stagedVersionedDirectory, err := agent.MkdirTemp(targetVersion)
+	if err != nil {
+		return fmt.Errorf("could not create temporary directory for untarring and validating new update: %w", err)
 	}
 	defer func() {
 		// In case of error, clean up the staged version
@@ -224,39 +211,16 @@ func (ulm *updateLibraryManager) removeUpdate(binary autoupdatableBinary, binary
 	}
 }
 
-// TidyLibrary removes unneeded files from the staged updates and updates directories.
+// TidyLibrary reviews all updates in the library for the binary and removes any old versions
+// that are no longer needed. It will always preserve the current running binary, and then the
+// two most recent valid versions. It will remove versions it cannot validate.
 func (ulm *updateLibraryManager) TidyLibrary(binary autoupdatableBinary, currentVersion string) {
 	// Acquire lock for modifying the library
 	ulm.lock.Lock(binary)
 	defer ulm.lock.Unlock(binary)
 
-	// First, remove old staged archives
-	ulm.tidyStagedUpdates(binary)
-
 	// Remove any updates we no longer need
-	ulm.tidyUpdateLibrary(binary, currentVersion)
-}
-
-// tidyStagedUpdates removes all old archives from the staged updates directory.
-func (ulm *updateLibraryManager) tidyStagedUpdates(binary autoupdatableBinary) {
-	matches, err := filepath.Glob(filepath.Join(ulm.stagingDir, "*"))
-	if err != nil {
-		level.Debug(ulm.logger).Log("msg", "could not glob for staged updates to tidy updates library", "err", err)
-		return
-	}
-
-	for _, match := range matches {
-		if err := os.RemoveAll(match); err != nil {
-			level.Debug(ulm.logger).Log("msg", "could not remove staged update when tidying update library", "file", match, "err", err)
-		}
-	}
-}
-
-// tidyUpdateLibrary reviews all updates in the library for the binary and removes any old versions
-// that are no longer needed. It will always preserve the current running binary, and then the
-// two most recent valid versions. It will remove versions it cannot validate.
-func (ulm *updateLibraryManager) tidyUpdateLibrary(binary autoupdatableBinary, currentRunningVersion string) {
-	if currentRunningVersion == "" {
+	if currentVersion == "" {
 		level.Debug(ulm.logger).Log("msg", "cannot tidy update library without knowing current running version")
 		return
 	}
@@ -282,7 +246,7 @@ func (ulm *updateLibraryManager) tidyUpdateLibrary(binary autoupdatableBinary, c
 	nonCurrentlyRunningVersionsKept := 0
 	for i := len(versionsInLibrary) - 1; i >= 0; i -= 1 {
 		// Always keep the current running executable
-		if versionsInLibrary[i] == currentRunningVersion {
+		if versionsInLibrary[i] == currentVersion {
 			continue
 		}
 

pkg/autoupdate/tuf/library_manager_test.go

@@ -30,38 +30,15 @@ func Test_newUpdateLibraryManager(t *testing.T) {
 	require.NoError(t, err, "could not stat base dir")
 	require.True(t, baseDir.IsDir(), "base dir is not a directory")
 
-	stagedDownloadDir, err := os.Stat(testLibraryManager.stagingDir)
-	require.NoError(t, err, "could not stat staged osqueryd download dir")
-	require.True(t, stagedDownloadDir.IsDir(), "staged osqueryd download dir is not a directory")
-
-	osquerydDownloadDir, err := os.Stat(filepath.Join(testBaseDir, "osqueryd"))
+	osquerydDownloadDir, err := os.Stat(updatesDirectory("osqueryd", testLibraryManager.baseDir))
 	require.NoError(t, err, "could not stat osqueryd download dir")
 	require.True(t, osquerydDownloadDir.IsDir(), "osqueryd download dir is not a directory")
 
-	launcherDownloadDir, err := os.Stat(filepath.Join(testBaseDir, "launcher"))
+	launcherDownloadDir, err := os.Stat(updatesDirectory("launcher", testLibraryManager.baseDir))
 	require.NoError(t, err, "could not stat launcher download dir")
 	require.True(t, launcherDownloadDir.IsDir(), "launcher download dir is not a directory")
 }
 
-func TestClose(t *testing.T) {
-	t.Parallel()
-
-	testBaseDir := filepath.Join(t.TempDir(), "updates")
-	testLibraryManager, err := newUpdateLibraryManager("", nil, testBaseDir, log.NewNopLogger())
-	require.NoError(t, err, "unexpected error creating new update library manager")
-
-	stagedDownloadDir, err := os.Stat(testLibraryManager.stagingDir)
-	require.NoError(t, err, "could not stat staged osqueryd download dir")
-	require.True(t, stagedDownloadDir.IsDir(), "staged osqueryd download dir is not a directory")
-
-	// Close the library
-	require.NoError(t, testLibraryManager.Close(), "expected no error on closing library")
-
-	// Confirm staged download directory is gone
-	_, err = os.Stat(testLibraryManager.stagingDir)
-	require.True(t, os.IsNotExist(err), "expected staged download dir to be removed on Close")
-}
-
 func Test_pathToTargetVersionExecutable(t *testing.T) {
 	t.Parallel()
 
@@ -171,11 +148,6 @@ func TestAddToLibrary(t *testing.T) {
 			executableInfo, err := os.Stat(executableLocation(filepath.Join(updatesDirectory(tt.binary, testBaseDir), testReleaseVersion), tt.binary))
 			require.NoError(t, err, "checking that downloaded update includes executable")
 			require.False(t, executableInfo.IsDir())
-
-			// Confirm the staging directory is empty
-			matches, err := filepath.Glob(filepath.Join(testLibraryManager.stagingDir, "*"))
-			require.NoError(t, err, "checking that staging dir was cleaned")
-			require.Equal(t, 0, len(matches), "unexpected files found in staged updates directory: %+v", matches)
 		})
 	}
 }
@@ -198,7 +170,6 @@ func TestAddToLibrary_alreadyRunning(t *testing.T) {
 				mirrorClient: http.DefaultClient,
 				logger:       log.NewNopLogger(),
 				baseDir:      testBaseDir,
-				stagingDir:   t.TempDir(),
 				lock:         newLibraryLock(),
 			}
 
@@ -238,7 +209,6 @@ func TestAddToLibrary_alreadyAdded(t *testing.T) {
 				mirrorClient: http.DefaultClient,
 				logger:       log.NewNopLogger(),
 				baseDir:      testBaseDir,
-				stagingDir:   t.TempDir(),
 				lock:         newLibraryLock(),
 			}
 
@@ -328,11 +298,6 @@ func TestAddToLibrary_verifyStagedUpdate_handlesInvalidFiles(t *testing.T) {
 			// Request download
 			require.Error(t, testLibraryManager.AddToLibrary(tt.binary, "", tt.targetFile, tt.targetMeta), "expected error when library manager downloads invalid file")
 
-			// Confirm the update was removed after download
-			downloadMatches, err := filepath.Glob(filepath.Join(testLibraryManager.stagingDir, "*"))
-			require.NoError(t, err, "checking that staging dir did not have any downloads")
-			require.Equal(t, 0, len(downloadMatches), "unexpected files found in staged updates directory: %+v", downloadMatches)
-
 			// Confirm the update was not added to the library
 			updateMatches, err := filepath.Glob(filepath.Join(updatesDirectory(tt.binary, testBaseDir), "*"))
 			require.NoError(t, err, "checking that updates directory does not contain any updates")
@@ -498,22 +463,11 @@ func TestTidyLibrary(t *testing.T) {
 				// Set up test library manager
 				testBaseDir := t.TempDir()
 				testLibraryManager := &updateLibraryManager{
-					logger:     log.NewNopLogger(),
-					baseDir:    testBaseDir,
-					stagingDir: t.TempDir(),
-					lock:       newLibraryLock(),
+					logger:  log.NewNopLogger(),
+					baseDir: testBaseDir,
+					lock:    newLibraryLock(),
 				}
 
-				// Make a file in the staged updates directory
-				f1, err := os.Create(filepath.Join(testLibraryManager.stagingDir, fmt.Sprintf("%s-1.2.3.tar.gz", binary)))
-				require.NoError(t, err, "creating fake download file")
-				f1.Close()
-
-				// Confirm we made the staging files
-				stagingMatches, err := filepath.Glob(filepath.Join(testLibraryManager.stagingDir, "*"))
-				require.NoError(t, err, "could not glob for files in staged download dir")
-				require.Equal(t, 1, len(stagingMatches))
-
 				// Set up existing versions for test
 				for existingVersion, isExecutable := range tt.existingVersions {
 					executablePath := executableLocation(filepath.Join(updatesDirectory(binary, testBaseDir), existingVersion), binary)
@@ -538,13 +492,6 @@ func TestTidyLibrary(t *testing.T) {
 				// Tidy the library
 				testLibraryManager.TidyLibrary(binary, tt.currentlyRunningVersion)
 
-				// Confirm the staging directory was tidied up
-				_, err = os.Stat(testLibraryManager.stagingDir)
-				require.NoError(t, err, "could not stat staged download dir")
-				matchesAfter, err := filepath.Glob(filepath.Join(testLibraryManager.stagingDir, "*"))
-				require.NoError(t, err, "could not glob for files in staged download dir")
-				require.Equal(t, 0, len(matchesAfter))
-
 				// Confirm that the versions we expect are still there
 				for _, expectedPreservedVersion := range tt.expectedPreservedVersions {
 					info, err := os.Stat(filepath.Join(updatesDirectory(binary, testBaseDir), expectedPreservedVersion))