Avoid uninstalling and re-installing service components on policy change

[ycombinator]

What does this PR do?

This PR identifies Service Runtime components with only their input type; the output ID is not longer used.

Why is it important?

Service Runtime components are intended to be kept running (via a service) for as long as possible. We should only install/start or uninstall/stop them if they are being explicitly added or removed, respectively, from the component model. If only their configuration is being updated, we should keep the component running.

If a component's ID changes between the last and current component models, Elastic Agent will ask the component's service to uninstall and then reinstall itself. Prior to this PR, service components' ID were determined by their input type and output ID. Therefore, if a service component's output were changed, it would cause the service to be uninstall and then reinstalled. This is undesirable behavior, as services should be kept running as long as possible.

With the changes in this PR, we no longer consider the output ID when generating service components' IDs. If a service component's output is changed, it's ID remains the same between the last and current component models. Elastic Agent does not uninstall and reinstall the component's service but simply passes the configuration change to it (which it was doing prior to this PR anyway).

Checklist

• I have read and understood the pull request guidelines of this project.
• 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

Disruptive User Impact

None.

How to test this PR locally

Policy reassign does not uninstall/reinstall Endpoint

1. Using the Fleet UI, create three Agent policies:
   • default: containing only the system integration
   • tp-es: containing the Elastic Defend integration, with tamper protection enabled, and using the Elasticsearch output.
   • tp-ls: containing the Elastic Defend integration, with tamper protection enabled, and using the Logstash output. Note that you will need to create the Logstash output in Fleet > Settings.
2. Enroll an Elastic Agent in the tp-es policy and verify the agent is healthy and shipping data.
3. Assign the Agent to the tp-ls policy.
4. Check the Agent logs and make sure the Endpoint component is not being uninstalled and reinstalled. Concretely, check that there is no log entry for uninstall endpoint service.
5. Check the Endpoint logs (located under /opt/Elastic/Endpoint/state/log/ on Linux) and make sure that Endpoint has connected to Logstash (or has attempted to and failed if there is no actual Logstash endpoint listening).

Removing Endpoint from policy uninstalls Endpoint

1. Assign the Agent to the default policy.
2. Check the Agent logs and make sure the Endpoint component is stopped and uninstalled. Concretely, check that there is a log entry for stopping endpoint service runtime, followed by uninstall endpoint service, followed by Stopped: endpoint service runtime.

Related issues

• Resolves Cannot successfully change output type or name in tamper protected agent polices that contain Elastic Defend #11266

Questions to ask yourself

• How are we going to support this in production?
• How are we going to measure its adoption?
• How are we going to debug this?
• What are the metrics I should take care of?
• ...

[mergify]

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-./d./d is the label that automatically backports to the 8./d branch. /d is the digit
• backport-active-all is the label that automatically backports to all active branches.
• backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
• backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

[elasticmachine]

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

[cmacknz]

Dropping the output ID makes sense to me, but I think if you do a policy assignment the input ID also changes, which would cause an uninstall and reinstall.

Does the policy reassignment case work properly for tamper protected agents? Or is this only handling the case where the output is changed within the same policy?

[ycombinator]

Dropping the output ID makes sense to me, but I think if you do a policy assignment the input ID also changes, which would cause an uninstall and reinstall.

As discussed in today's meeting, the component ID uses the input type (not input ID) so, with the change in this PR, we should still prevent an uninstall and reinstall.

[ycombinator]

Does the policy reassignment case work properly for tamper protected agents? Or is this only handling the case where the output is changed within the same policy?

I will test this along with the upgrade scenario.

Yes, it does. I forgot that that's how I'd tested this PR to begin with. I even added manual testing steps to the PR's description. 🤦

I will test the upgrade scenario next.

[ycombinator]

I will test the upgrade scenario next.

Okay, so upgrading works in that nothing is broken after upgrade. Here's the output of elastic-agent status before, during, and after the upgrade:

Before

┌─ fleet
│  └─ status: (HEALTHY) Connected
└─ elastic-agent
   ├─ status: (HEALTHY) Running
   ├─ info
   │  ├─ id: a4d2bed3-5c18-42d0-bc55-cc838f2a43a6
   │  ├─ version: 9.2.0-SNAPSHOT
   │  └─ commit: 07d5c3a34e486f81130d7177678c547944dbb84c
   └─ endpoint-default
      ├─ status: (HEALTHY) Healthy: communicating with endpoint service
      ├─ endpoint-default
      │  ├─ status: (HEALTHY) Applied policy 'defend' (Defend policy rev. 1. Agent policy rev. 21.)
      │  └─ type: OUTPUT
      └─ endpoint-default-a92a4f1c-8773-43ce-8e97-062fc2cebb1f
         ├─ status: (HEALTHY) Applied policy 'defend' (Defend policy rev. 1. Agent policy rev. 21.)
         └─ type: INPUT

During

┌─ fleet
│  └─ status: (STARTING)
├─ elastic-agent
│  ├─ status: (HEALTHY) Running
│  ├─ info
│  │  ├─ id: 6c6af8b5-7e88-42d0-8d71-70afbcfa39f1
│  │  ├─ version: 9.3.0
│  │  └─ commit: 7026d24f297cdcd6826da98ec3c39ea7f0c59c19
│  └─ endpoint
│     ├─ status: (HEALTHY) Healthy: communicating with endpoint service
│     ├─ endpoint
│     │  ├─ status: (HEALTHY) Applied policy 'defend' (Defend policy rev. 1. Agent policy rev. 21.)
│     │  └─ type: OUTPUT
│     └─ endpoint-a92a4f1c-8773-43ce-8e97-062fc2cebb1f
│        ├─ status: (HEALTHY) Applied policy 'defend' (Defend policy rev. 1. Agent policy rev. 21.)
│        └─ type: INPUT
└─ upgrade_details
   ├─ target_version: 9.3.0
   ├─ state: UPG_WATCHING
   ├─ action_id: 349b43f0-973b-4b40-ab21-ee082debcfaf
   └─ metadata

After

┌─ fleet
│  └─ status: (HEALTHY) Connected
└─ elastic-agent
   ├─ status: (HEALTHY) Running
   ├─ info
   │  ├─ id: 6c6af8b5-7e88-42d0-8d71-70afbcfa39f1
   │  ├─ version: 9.3.0
   │  └─ commit: 7026d24f297cdcd6826da98ec3c39ea7f0c59c19
   └─ endpoint
      ├─ status: (HEALTHY) Healthy: communicating with endpoint service
      ├─ endpoint
      │  ├─ status: (HEALTHY) Applied policy 'defend' (Defend policy rev. 1. Agent policy rev. 21.)
      │  └─ type: OUTPUT
      └─ endpoint-a92a4f1c-8773-43ce-8e97-062fc2cebb1f
         ├─ status: (HEALTHY) Applied policy 'defend' (Defend policy rev. 1. Agent policy rev. 21.)
         └─ type: INPUT```

Notice that the ID of the Endpoint component after the upgrade changes to `endpoint`.  Specifically, the ID no longer has the output ID, `default`, as the suffix.

[cmacknz]

What's the right additional testing to add for this? It looks correct by inspection, but I think we should add something proving it does what we think.

If we could get a simulated policy reassignment (input id: key change) and output change as a test of just the component model that's probably the lightest thing we could do.

If policy reassignment always fails without this change, that's a test case that is missing from the endpoint integration tests, so we could add that. I also don't think we have a test that does an upgrade with defend installed but that is even heavier, though we have had lots of problems around this discovered in the field so maybe worth it.

[ycombinator]

If we could get a simulated policy reassignment (input id: key change) and output change as a test of just the component model that's probably the lightest thing we could do.

Added a unit test case for input ID change to TestComponentUpdateDiff in cf89151.

A unit test that tests just changing the output already exists in TestComponentUpdateDiff:

elastic-agent/internal/pkg/agent/application/coordinator/coordinator_test.go

Line 208 in eb1cd7f

name: "just-change-output",

But I added additional assertions for that test case to make sure no components were added, removed, or updated in the component model as a result of such a change: 4e352a7#diff-21de8fb82a5ebccaa0ca3afb0cb8bfbe10d5b8f920021785c2d485ee35a3556cR225-R227

[ycombinator]

If policy reassignment always fails without this change, that's a test case that is missing from the endpoint integration tests, so we could add that.

Added in 8a1a8c3

[ycombinator]

I also don't think we have a test that does an upgrade with defend installed...
Looks like we do have some integration tests around upgrading Endpoint:

elastic-agent/testing/integration/ess/endpoint_security_test.go

Line 65 in eb1cd7f

func TestUpgradeAgentWithTamperProtectedEndpoint_DEB(t *testing.T) {

elastic-agent/testing/integration/ess/endpoint_security_test.go

Line 99 in eb1cd7f

func TestUpgradeAgentWithTamperProtectedEndpoint_RPM(t *testing.T) {

[ycombinator]

@cmacknz Thanks for the review. I see you added the backport-9.3 label on this PR. Being a bug fix, should we backport it to all active branches?

[elasticmachine]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 284dcb8

Failed CI Steps

• Unit tests - Windows 2016
• Ubuntu:2404:amd64:sudo

History

• 💔 Build #32772 failed f70ec1f
• 💔 Build #32652 failed aa3188b
• 💔 Build #32608 failed e2c6373
• 💔 Build #32552 failed cdb0764
• 💔 Build #32541 failed ea763b6

cc @ycombinator

[cmacknz]

@cmacknz Thanks for the review. I see you added the backport-9.3 label on this PR. Being a bug fix, should we backport it to all active branches?

I think we need to release this in 9.3 first, and if there are no unintended problems once we get through the 9.3 testing cycle we can backport.

The difference between 9.3 and 9.2+9.1 is time to release, if we had backported to the already released minors this would have released very quickly after merge with no soak time.

[ycombinator]

I think we need to release this in 9.3 first, and if there are no unintended problems once we get through the 9.3 testing cycle we can backport.

@cmacknz I think we're good to backport this PR now?

[cmacknz]

Possibly, 9.3.0 has not been available for that long yet. Unless someone asks us to backport this I would leave it in 9.3 only to be conservative.