[Unprivileged] Adjust integration tests to use unprivileged by default

[blakerouse]

What does this PR do?

Adjusts all the integration tests to use --unprivileged by default. This is done in a way that makes sure that anytime install is used its installed with --unprivileged unless the install is explicit that it should be installed privileged.

The upgrade tests use a mechanism to determine if its possible to install and upgrade in unprivileged mode it will. For the standalone upgrade scenarios both privileged and unprivileged with upgrade is also enabled.

Why is it important?

Provides the best coverage for --unprivileged which is more restrictive in how Elastic Agent runs. If it can run in --unprivileged it can most certainly run privileged.

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation
• [ ] I have made corresponding change to the default configuration files
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] I have added an entry in ./changelog/fragments using the changelog tool
• I have added an integration test or an E2E test

Related issues

• Closes All applicable integration tests should run with an unpriveledged agent installation #3736

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b non-root-more-testing upstream/non-root-more-testing
git merge upstream/main
git push upstream non-root-more-testing

[elasticmachine]

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

[blakerouse]

All integration tests pass! Only issue is with the Windows 10 test, that is affecting every PR.

[cmacknz]

Mostly looks good, a few comments that are almost nits but that I think make the test report and logs easier to understand.

[cmacknz]

Reading:

installOpts := atesting.InstallOpts{
		NonInteractive: true,
		Force:          true,
		Unprivileged:   atesting.NewBool(false),
	}

Should we just invert the name so we don't have to use a pointer type here? I'd rather use Privileged: true as the exception and have it not exactly match the argument name then have to deal with using NewBool.

[blakerouse]

Because the InstallOpts will conditional change IsUnprivileged depending on the operating system that is being tested, the code needs it to be a tristate boolean.

• Unset
• True
• False

In the case that it was never set it will we set to true on Linux and false on Windows (for now). If specifically set then that value is used instead. If we didn't use a tristate then the default of false versus setting false explicitly would be unknown so on Linux it would always be changed to true even on a test where false should be forced. Even if we flip this logic by renaming the variable Privileged it results in the same issue.

I also really prefer it to match the command line arguments, all other fields match, don't think we should deviant now.

[cmacknz]

Thanks, I don't feel that strongly about it and there's a technical reason for it to be like this. I'd be in favour of removing it if it turns out we don't need it once all the platforms support unprivileged.

[blakerouse]

All tests pass except for the Windows 10 unit tests. This is showing that unprivileged on Linux, is ready to start being used!

[pierrehilbert]

@blakerouse FYI #4123

[amitkanfer]

🚀

[elastic-sonarqube]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

[jlind23]

@pierrehilbert @cmacknz Are all integrations tests run in both mode on every PRs?

[cmacknz]

No, the ones where we know there are differences or that are important are run in both modes (install, upgrade, etc).

The philosophy is that if it works in unprivileged mode it will work as root, so many things don't need to be tested twice. Running without privileges is harder to get right because it is sensitive to directories and file permissions where as running as root is not.

[jlind23]

@blakerouse could we please make sure tests names also contains an indicator about whether or not they are running as root?

[blakerouse]

@jlind23 That is already done. A test that is testing specific unprivileged vs privileged has it in the name of the test. If the test by default uses unprivileged then it is not in the name, because on Linux it will be unprivileged and then on Windows it will be privileged (at least currently, until Windows support unprivileged). Once unprivileged becomes the default it seems redundant to repeat it everywhere, it would be better to just call out the privileged mode.

[jlind23]

@blakerouse thanks for the prompt answer, happy to see this already covered 👍🏼

[pierrehilbert]

@blakerouse shouldn't we have 8.13.0-SNAPSHOT here too?