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

* 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>

Author: fearful-symmetry

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

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

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