Fix filemarker creation process, update error message on uninstall (#…

…4172) (#4307) * move install file marker create, rename methods, change error message * change error message * change error message * remove calls to InstallMarkerExists * fix windows helper * fix windows, again * Revert "remove calls to InstallMarkerExists" This reverts commit 400bb0d. * change install file locaton, revert removal of filepath.Dir * fix wording * move IsUpgradeable, remove duplication * add tests * add mostly useless test * linter * tinkering with CI * skip supervisor test on windows * tinkering with the linter * remove experiment * refactor install to make adding tests easier * linter * add more tests... * still fixing tests * sonarQube * fix windows tests, fight sonarqube * tinker with file marker create * move install path * fix bad merge * still cleaning up merge * add tests * add tests, docs * fix tests * don't check mode bits * move command, fix permissions * SonarQube... * refactor dir setup to make Sonarqube happy * add changelog * Update changelog/fragments/1707951532-change-install-marker-creation.yaml Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> (cherry picked from commit 65e2c30) Co-authored-by: Alex K <8418476+fearful-symmetry@users.noreply.github.com>
elastic · Feb 22, 2024 · c9bdd24 · c9bdd24
1 parent 5dca10d
commit c9bdd24
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 12 deletions.
diff --git a/changelog/fragments/1707951532-change-install-marker-creation.yaml b/changelog/fragments/1707951532-change-install-marker-creation.yaml
@@ -0,0 +1,32 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: feature
+
+# Change summary; a 80ish characters long description of the change.
+summary: change-install-marker-creation
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
+description: Create the .installed marker earlier on in the install process, allowing the use of `elastic-agent uninstall` to cleanup if the install fails.
+
+# Affected component; a word indicating the component this changeset affects.
+component: install
+
+# PR URL; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+pr: https://github.com/elastic/elastic-agent/pull/4172
+
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+issue: https://github.com/elastic/elastic-agent/issues/4051
diff --git a/internal/pkg/agent/install/install.go b/internal/pkg/agent/install/install.go
@@ -102,13 +102,9 @@ func Install(cfgFile, topPath string, unprivileged bool, log *logp.Logger, pt *p
 		}
 	}
 
-	// ensure parent directory exists
-	err = os.MkdirAll(filepath.Dir(topPath), 0755)
+	err = setupInstallPath(topPath, ownership)
 	if err != nil {
-		return utils.FileOwner{}, errors.New(
-			err,
-			fmt.Sprintf("failed to create installation parent directory (%s)", filepath.Dir(topPath)),
-			errors.M("directory", filepath.Dir(topPath)))
+		return utils.FileOwner{}, fmt.Errorf("error setting up install path: %w", err)
 	}
 
 	manifest, err := readPackageManifest(dir)
@@ -173,11 +169,6 @@ func Install(cfgFile, topPath string, unprivileged bool, log *logp.Logger, pt *p
 		}
 	}
 
-	// create the install marker
-	if err := CreateInstallMarker(topPath, ownership); err != nil {
-		return utils.FileOwner{}, fmt.Errorf("failed to create install marker: %w", err)
-	}
-
 	// post install (per platform)
 	err = postInstall(topPath)
 	if err != nil {
@@ -216,6 +207,27 @@ func Install(cfgFile, topPath string, unprivileged bool, log *logp.Logger, pt *p
 	return ownership, nil
 }
 
+// setup the basic topPath, and the .installed file
+func setupInstallPath(topPath string, ownership utils.FileOwner) error {
+	// ensure parent directory exists
+	err := os.MkdirAll(filepath.Dir(topPath), 0755)
+	if err != nil {
+		return errors.New(err, fmt.Sprintf("failed to create installation parent directory (%s)", filepath.Dir(topPath)), errors.M("directory", filepath.Dir(topPath)))
+	}
+
+	// create Agent/ directory with more locked-down permissions
+	err = os.MkdirAll(topPath, 0750)
+	if err != nil {
+		return errors.New(err, fmt.Sprintf("failed to create top path (%s)", topPath), errors.M("directory", topPath))
+	}
+
+	// create the install marker
+	if err := CreateInstallMarker(topPath, ownership); err != nil {
+		return fmt.Errorf("failed to create install marker: %w", err)
+	}
+	return nil
+}
+
 func readPackageManifest(extractedPackageDir string) (*v1.PackageManifest, error) {
 	manifestFilePath := filepath.Join(extractedPackageDir, v1.ManifestFileName)
 	manifestFile, err := os.Open(manifestFilePath)
@@ -473,10 +485,14 @@ func hasAllSSDs(block ghw.BlockInfo) bool {
 	return true
 }
 
+// CreateInstallMarker creates a `.installed` file at the given install path,
+// and then calls fixInstallMarkerPermissions to set the ownership provided by `ownership`
 func CreateInstallMarker(topPath string, ownership utils.FileOwner) error {
 	markerFilePath := filepath.Join(topPath, paths.MarkerFileName)
-	if _, err := os.Create(markerFilePath); err != nil {
+	handle, err := os.Create(markerFilePath)
+	if err != nil {
 		return err
 	}
+	_ = handle.Close()
 	return fixInstallMarkerPermissions(markerFilePath, ownership)
 }
diff --git a/internal/pkg/agent/install/install_test.go b/internal/pkg/agent/install/install_test.go
@@ -14,8 +14,10 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
 	"github.com/elastic/elastic-agent/internal/pkg/cli"
 	v1 "github.com/elastic/elastic-agent/pkg/api/v1"
+	"github.com/elastic/elastic-agent/pkg/utils"
 )
 
 func TestHasAllSSDs(t *testing.T) {
@@ -185,3 +187,10 @@ func TestCopyFiles(t *testing.T) {
 	}
 
 }
+
+func TestSetupInstallPath(t *testing.T) {
+	tmpdir := t.TempDir()
+	err := setupInstallPath(tmpdir, utils.CurrentFileOwner())
+	require.NoError(t, err)
+	require.FileExists(t, filepath.Join(tmpdir, paths.MarkerFileName))
+}