Fix audit/unenroll calls when agent runs fleet-server #6085

michel-laterman · 2024-11-19T19:27:27Z

What does this PR do?

Call audit/unenroll (during uninstall) before components are stopped if the elastic-agent is running a fleet-server instance.

Why is it important?

Agents running fleet-server instances would always get an error because fleet-server would be stopped by the time the notify call is attempted.

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

How to test this PR locally

Enroll an agent in a policy with the fleet-server integration and uninstall the agent

Related issues

Closes elastic-agent installed as Fleet server remains stuck in uninstalling with retry errors for over 4-5 minutes. #5752

michel-laterman · 2024-11-21T19:55:11Z

internal/pkg/agent/install/uninstall.go

+		if cfg.Fleet.Client.Transport != nil {
+			cfg.Fleet.Client.Transport.TLS.VerificationMode = tlscommon.VerifyCertificate
+		}
+		cfg.Fleet.Client.Hosts = []string{cfg.Fleet.Client.Host}


I'm not sure what's going on here; but Client.Hosts had the remote (cloud) fleet-server address, and Client.Host was the internal port.
I'm going to take some time tomorrow to verify that our normal agent operations still use the internal port when the agent is running fleet-server.

agent does connect to localhost:8221; but strangely enough fleet-server does not populate the server.address attribute in HTTP logs; i'll open a fleet-server PR to fix this
pr here: elastic/fleet-server#4142

cfg.Fleet.Client.Hosts = []string{cfg.Fleet.Client.Host} is a bit weird. This is perhaps something else that could be encapsulated into the output of an RPC call to the running agent service since agent itself must know that these represent two different things.

We should avoid using an RPC call because we don't want to introduce the requirement that an agent is running for the uninstall to succeed.

What I think is happening here:

elastic agent sets Host from the enroll command

in the policy_change handler elastic-agent uses only Hosts field coming from Fleet and tries to reset the other fields.

I guess that the merged result of that is not quite right in this scenario and we end up with the discrepancy between Host and Hosts

From discussion with @nchaulet at the time, it was determined that Fleet will send back only Hosts field while the separate Host, Protocol, Path is only handled by agent when initializing config to store command line args. This refactor has been done in context of mTLS from policy support PR as an attempt to clean up the policy change handler.

I filed an issue to have a look and possible remote Host, Protocol and Path fields from the agent config at the time but maybe this needs further evaluation/discussion taking into account fleet bootstrap scenarios

Can we link to that issue and add this explanation to the code? This is going to be really not obvious to someone reading this code without the context of this PR.

elasticmachine · 2024-11-21T19:55:57Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

internal/pkg/agent/install/uninstall.go

pierrehilbert · 2024-11-26T10:08:03Z

It's a detail for now but we should probably also plan to backport this to 8.17

cmacknz · 2024-11-26T20:06:48Z

This needs a regression test :)

testing/integration/fleetserver_test.go

internal/pkg/agent/install/uninstall.go

testing/integration/install_test.go

internal/pkg/agent/install/uninstall.go

pchila

Tested on Linux with and without the fix and it solves the issue with audit/unenroll when running a local Fleet server

I left a small comment for the Host/Hosts discrepancy we have trying to give more context, not sure if we want to follow that up with a dedicated issue

mergify · 2024-12-11T21:12:40Z

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 fix-audit-unenroll-fleet-server upstream/fix-audit-unenroll-fleet-server
git merge upstream/main
git push upstream fix-audit-unenroll-fleet-server

cmacknz

Tried it with an unprivileged agent on my Mac and it worked this time. Thanks for the follow up and additional test coverage!

❯ sudo elastic-development-agent uninstall
Elastic Agent will be uninstalled from your system at /Library/Elastic/Agent-Development. Do you want to continue? [Y/n]:y
[=== ] Done  [2s]
Elastic Agent has been uninstalled.

…eet-server

swiatekm

LGTM

michel-laterman · 2024-12-13T00:20:04Z

@cmacknz @ycombinator, can one of you please force this in once buildkite completes? we have integration test coverage but not unit tests

michel-laterman · 2024-12-13T19:12:06Z

buildkite test this

elastic-sonarqube · 2024-12-17T19:31:16Z

Quality Gate failed

Failed conditions
3.1% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

cmacknz · 2024-12-17T20:57:26Z

Force merging now that CI has passed.

* Fix audit/unenroll calls when agent runs fleet-server * Fix integration test * Increase output for failed test * move notification attempt * move notification attempt * audit/uninstall will call localhost with certonly tls * Fix linter * remove tls setting * Add PR link to changelog * stop checking if we need to notify in an error is encountered * Shorten non-fatal error messages to prevent output issues in terminals * Don't load agentinfo, use ID value from config instead * Add unprivilged tests to audit unenroll integration tests * Add comments for setting hosts (cherry picked from commit f321d8a) # Conflicts: # internal/pkg/agent/install/uninstall.go

* Fix audit/unenroll calls when agent runs fleet-server * Fix integration test * Increase output for failed test * move notification attempt * move notification attempt * audit/uninstall will call localhost with certonly tls * Fix linter * remove tls setting * Add PR link to changelog * stop checking if we need to notify in an error is encountered * Shorten non-fatal error messages to prevent output issues in terminals * Don't load agentinfo, use ID value from config instead * Add unprivilged tests to audit unenroll integration tests * Add comments for setting hosts (cherry picked from commit f321d8a)

* Fix audit/unenroll calls when agent runs fleet-server * Fix integration test * Increase output for failed test * move notification attempt * move notification attempt * audit/uninstall will call localhost with certonly tls * Fix linter * remove tls setting * Add PR link to changelog * stop checking if we need to notify in an error is encountered * Shorten non-fatal error messages to prevent output issues in terminals * Don't load agentinfo, use ID value from config instead * Add unprivilged tests to audit unenroll integration tests * Add comments for setting hosts (cherry picked from commit f321d8a) # Conflicts: # internal/pkg/agent/install/uninstall.go

…-server (#6365) * Fix audit/unenroll calls when agent runs fleet-server (#6085) * Fix audit/unenroll calls when agent runs fleet-server * Fix integration test * Increase output for failed test * move notification attempt * move notification attempt * audit/uninstall will call localhost with certonly tls * Fix linter * remove tls setting * Add PR link to changelog * stop checking if we need to notify in an error is encountered * Shorten non-fatal error messages to prevent output issues in terminals * Don't load agentinfo, use ID value from config instead * Add unprivilged tests to audit unenroll integration tests * Add comments for setting hosts (cherry picked from commit f321d8a) # Conflicts: # internal/pkg/agent/install/uninstall.go * Fix merge conflict --------- Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> Co-authored-by: michel-laterman <michel.laterman@elastic.co>

* Fix audit/unenroll calls when agent runs fleet-server * Fix integration test * Increase output for failed test * move notification attempt * move notification attempt * audit/uninstall will call localhost with certonly tls * Fix linter * remove tls setting * Add PR link to changelog * stop checking if we need to notify in an error is encountered * Shorten non-fatal error messages to prevent output issues in terminals * Don't load agentinfo, use ID value from config instead * Add unprivilged tests to audit unenroll integration tests * Add comments for setting hosts (cherry picked from commit f321d8a) Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com>

…-server (#6363) * Fix audit/unenroll calls when agent runs fleet-server (#6085) * Fix audit/unenroll calls when agent runs fleet-server * Fix integration test * Increase output for failed test * move notification attempt * move notification attempt * audit/uninstall will call localhost with certonly tls * Fix linter * remove tls setting * Add PR link to changelog * stop checking if we need to notify in an error is encountered * Shorten non-fatal error messages to prevent output issues in terminals * Don't load agentinfo, use ID value from config instead * Add unprivilged tests to audit unenroll integration tests * Add comments for setting hosts (cherry picked from commit f321d8a) # Conflicts: # internal/pkg/agent/install/uninstall.go * Fix merge conflict --------- Co-authored-by: Michel Laterman <82832767+michel-laterman@users.noreply.github.com> Co-authored-by: michel-laterman <michel.laterman@elastic.co>

Fix audit/unenroll calls when agent runs fleet-server

4fbee98

michel-laterman added bug Something isn't working Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify labels Nov 19, 2024

mergify bot assigned michel-laterman Nov 19, 2024

michel-laterman added 3 commits November 20, 2024 10:16

Fix integration test

8b036d7

Increase output for failed test

1bcf7f7

move notification attempt

9950ed5

michel-laterman force-pushed the fix-audit-unenroll-fleet-server branch from dce24e2 to 9950ed5 Compare November 20, 2024 21:21

michel-laterman and others added 3 commits November 21, 2024 09:14

move notification attempt

1b0f34c

Merge branch 'main' into fix-audit-unenroll-fleet-server

d8ef38e

audit/uninstall will call localhost with certonly tls

c349511

michel-laterman commented Nov 21, 2024

View reviewed changes

michel-laterman marked this pull request as ready for review November 21, 2024 19:55

michel-laterman requested a review from a team as a code owner November 21, 2024 19:55

michel-laterman requested review from swiatekm and pchila November 21, 2024 19:55

Fix linter

9da083e

swiatekm reviewed Nov 25, 2024

View reviewed changes

internal/pkg/agent/install/uninstall.go Outdated Show resolved Hide resolved

internal/pkg/agent/install/uninstall.go Outdated Show resolved Hide resolved

remove tls setting

3710448

cmacknz reviewed Nov 29, 2024

View reviewed changes

testing/integration/fleetserver_test.go Outdated Show resolved Hide resolved

michel-laterman added the backport-8.17 Automated backport with mergify label Nov 29, 2024

Add PR link to changelog

7d68d68

cmacknz reviewed Nov 29, 2024

View reviewed changes

internal/pkg/agent/install/uninstall.go Show resolved Hide resolved

cmacknz reviewed Nov 29, 2024

View reviewed changes

testing/integration/install_test.go Outdated Show resolved Hide resolved

cmacknz reviewed Nov 29, 2024

View reviewed changes

internal/pkg/agent/install/uninstall.go Outdated Show resolved Hide resolved

michel-laterman and others added 2 commits December 9, 2024 16:35

Add unprivilged tests to audit unenroll integration tests

933d6c8

Merge branch 'main' into fix-audit-unenroll-fleet-server

8596b93

pchila approved these changes Dec 11, 2024

View reviewed changes

cmacknz approved these changes Dec 11, 2024

View reviewed changes

michel-laterman added 2 commits December 11, 2024 15:16

Add comments for setting hosts

5913866

Merge remote-tracking branch 'origin/main' into fix-audit-unenroll-fl…

71c9cf5

…eet-server

michel-laterman enabled auto-merge (squash) December 11, 2024 23:25

swiatekm approved these changes Dec 12, 2024

View reviewed changes

michel-laterman added 3 commits December 12, 2024 12:56

Merge branch 'main' into fix-audit-unenroll-fleet-server

5b3fdbf

Merge branch 'main' into fix-audit-unenroll-fleet-server

8cc461b

Merge branch 'main' into fix-audit-unenroll-fleet-server

60ba11c

ycombinator disabled auto-merge December 13, 2024 00:23

Merge branch 'main' into fix-audit-unenroll-fleet-server

6615db5

michel-laterman added 2 commits December 16, 2024 14:49

Merge branch 'main' into fix-audit-unenroll-fleet-server

f01bc6a

Merge branch 'main' into fix-audit-unenroll-fleet-server

0ad9069

cmacknz merged commit f321d8a into elastic:main Dec 17, 2024
12 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix audit/unenroll calls when agent runs fleet-server #6085

Fix audit/unenroll calls when agent runs fleet-server #6085

michel-laterman commented Nov 19, 2024

michel-laterman Nov 21, 2024

michel-laterman Nov 22, 2024 •

edited

Loading

cmacknz Nov 29, 2024

michel-laterman Dec 2, 2024

pchila Dec 11, 2024 •

edited

Loading

cmacknz Dec 11, 2024

elasticmachine commented Nov 21, 2024

pierrehilbert commented Nov 26, 2024

cmacknz commented Nov 26, 2024

pchila left a comment •

edited

Loading

mergify bot commented Dec 11, 2024

cmacknz left a comment

swiatekm left a comment

michel-laterman commented Dec 13, 2024

michel-laterman commented Dec 13, 2024

elastic-sonarqube bot commented Dec 17, 2024

cmacknz commented Dec 17, 2024

Fix audit/unenroll calls when agent runs fleet-server #6085

Fix audit/unenroll calls when agent runs fleet-server #6085

Conversation

michel-laterman commented Nov 19, 2024

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

michel-laterman Nov 21, 2024

Choose a reason for hiding this comment

michel-laterman Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

cmacknz Nov 29, 2024

Choose a reason for hiding this comment

michel-laterman Dec 2, 2024

Choose a reason for hiding this comment

pchila Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

cmacknz Dec 11, 2024

Choose a reason for hiding this comment

elasticmachine commented Nov 21, 2024

pierrehilbert commented Nov 26, 2024

cmacknz commented Nov 26, 2024

pchila left a comment • edited Loading

Choose a reason for hiding this comment

mergify bot commented Dec 11, 2024

cmacknz left a comment

Choose a reason for hiding this comment

swiatekm left a comment

Choose a reason for hiding this comment

michel-laterman commented Dec 13, 2024

michel-laterman commented Dec 13, 2024

elastic-sonarqube bot commented Dec 17, 2024

Quality Gate failed

cmacknz commented Dec 17, 2024

michel-laterman Nov 22, 2024 •

edited

Loading

pchila Dec 11, 2024 •

edited

Loading

pchila left a comment •

edited

Loading