[Fleet] RBAC - Make read agent actions space aware #189519

jillguyonnet · 2024-07-30T14:22:43Z

Summary

Relates to #185040

This PR makes the following Fleet agents API space aware (behind useSpaceAwareness feature flag):

GET /agents/action_status
POST /agents/{agentId}/actions

I have already started work on POST /agents/{agentId}/actions/{actionId}/cancel but I think it would best grouped with POST /agents/{agentId}/upgrade and POST /agents/bulk_upgrade in a separate PR.

Details

GET /agents/action_status

⚠️ I have implemented the following logic in the action status service:

If the useSpaceAwareness feature flag is disabled, there is no change.
In the default space, actions with namespaces: ['default'] and those with no namespaces property are returned.
In a custom space, only actions with namespaces: ['spaceName'] are returned.

This is to ensure older actions with no namespaces property are not lost. Feedback on this approach would be awesome.

NB: only tag update agent actions and agent policy update actions have a namespaces property at the moment.

POST /agents/{agentId}/actions

If the useSpaceAwareness feature flag is enabled, the action fails if the agent is not in the current space.
The namespaces property is populated when the action is created.

Other

This PR also fixes an issue with setting namespaces in agent actions for tags update in the default space (this is because I didn't realise soClient.getCurrentNamespace() returns undefined in the default space).

⚠️ I also modified the isAgentInNamespace helper to return true in the default space for agents with no explicitly set namespaces.

Finally, this PR introduces the following helpers:

getCurrentNamespace(soClient): this helper returns the string default instead of undefined in the default space, which seems to be the behaviour we want most of the time.
addNamespaceFilteringToQuery: this helper extends the ES queries used in the action status service to conditionally add filtering by namespace as described above. It should be reusable for other endpoints as well.
The isAgentInNamespace and agentsKueryNamespaceFilter were moved into the fleet/server/services/spaces folder where other space-related helpers live.

Testing

In order to test GET /agents/action_status, the best would be to have a custom space and create a mix of agent and agent policy actions across the default and the custom spaces, for instance:
- Agent policy updates (change the policy description)
- Update agent tags (creates agent actions with set namespaces)
- Unenroll agents (creates agent actions with no namespaces property)
  Then check the output of GET /agents/action_status from the Dev Tools and the UI (agent activity flyout):
- Agent policy actions should only be listed in the relevant space.
- Update agent tags actions should only be listed in the relevant space.
- Other actions should only be listed in the default space.
Test POST /agents/{agentId}/actions from the Dev Tools with any action type, e.g. "UNENROLL":
- If the agent is not in the current space, it should return not found.
- If the agent is in the current space, it should create an action with the correct namespaces property.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed

obltmachine · 2024-07-30T14:22:58Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

jillguyonnet · 2024-07-30T14:46:35Z

/ci

elasticmachine · 2024-07-31T15:27:12Z

Pinging @elastic/fleet (Team:Fleet)

nchaulet · 2024-07-31T19:28:06Z

x-pack/plugins/fleet/server/routes/agent/actions_handlers.ts

@@ -39,6 +40,7 @@ export const postNewAgentActionHandlerBuilder = function (
        created_at: new Date().toISOString(),
        ...newAgentAction,
        agents: [agent.id],


Should we validate the agent is in the current namespace?

I believe the call to actionsService.getAgent already handles that. I've added an integration test to cover that case.

Just checked what is done in actionsService.getAgent and it seems there is no namespace validation there.

Please correct me if I'm wrong :)

actionsService.getAgent is defined here

the postNewAgentActionHandlerBuilder maps it to AgentService.getAgentById here

and getAgentById contains namespace validation here

I've tried it manually as well and I'm seeing 404 for agent ids in other spaces. Am I missing anything?

Looks like I missed that change in getAgentById 👍

kibana-ci · 2024-08-02T11:27:00Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: bd58978

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #3 / Space awareness actions POST /agents/{agentId}/actions should create an action with set namespace in the default space
[job] [logs] Fleet Cypress Tests #4 / View agents list Agent status filter should filter on healthy (16 result)
[job] [logs] Fleet Cypress Tests #4 / View agents list Agent status filter should filter on healthy and unhealthy
[job] [logs] Fleet Cypress Tests #4 / View agents list Bulk actions should allow to bulk upgrade agents and cancel that upgrade

Metrics [docs]

✅ unchanged

History

💔 Build #224705 failed 611b9f45ca1d7833c7c32f5c18b31c4934dc5588

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jillguyonnet

nchaulet

LGTM 🚀

## Summary Closes #185040 Followup to: #188507 #189519 #190069 This PR contains the last required changed for making Fleet agents write APIs space aware: * Implement space awareness in the following endpoints: * `POST /agents/{agentId}/unenroll` * `POST /agents/{agentId}/request_diagnostics` * `POST /agents/bulk_unenroll` * `POST /agents/bulk_request_diagnostics` * Fix a bug in `POST /agents/bulk_update_agent_tags` where the space id was not passed. * Simply the setting of the `namespaces` property when creating agent actions when the space id is known. * Rename `currentNamespace` to `currentSpaceId` where appropriate (see comment below). * Add API integration tests and consolidate existing ones. ~⚠️ At the time of writing, I would like there to be more tests covering bulk query processing in batches, which are currently lacking. I have experienced difficulties getting those tests to pass consistently.~ Filed [followup issue](#191643) for those. ### A note on terminology As pointed out in #191083 (comment), it seems that the terms "namespace" and "space id" are occasionally used interchangeably in some parts of the codebase to refer to a Kibana space. For instance, documents in Fleet indices (agents, agent policies, agent actions...) [possess a `namespaces` property](elastic/elasticsearch#108363) to track the spaces they belong to. The current space id is also returned using the Saved Object client's `getCurrentNamespace` function. However, "namespace" is also a datastream property. In the Agent policy settings UI, the "Spaces" property (which will be linked to the saved object's `namespaces` property) is above the "Default namespace" property, which relates to the integration's data streams: <img width="1916" alt="Screenshot 2024-08-26 at 14 51 10" src="https://github.com/user-attachments/assets/fe2a0948-3387-4a93-96dc-90fc5cf1a683"> This should not be a source of major issues, but is best clarified for future reference. In this PR, I've replaced some occurrences of `namespace` with `spaceId` where appropriate to try to maximise the use of the latter. ### Testing * This PR should be put through the Flaky Test Runner prior to merging (I will kick the job). * Manual testing should also be performed for the new endpoints mentioned above. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed --------- Co-authored-by: Elastic Machine <[email protected]>

jillguyonnet self-assigned this Jul 30, 2024

jillguyonnet added 4 commits July 31, 2024 17:24

[Fleet] RBAC - Make read agent actions space aware

f187403

Add space check to POST action

eaf54c3

Linting

ddca3b9

Add API integration tests

3e64318

jillguyonnet force-pushed the fleet/185040-rbac-agents-write-api-2 branch from a1264cd to 3e64318 Compare July 31, 2024 15:24

jillguyonnet added Team:Fleet Team label for Observability Data Collection Fleet team release_note:skip Skip the PR/issue when compiling release notes labels Jul 31, 2024

jillguyonnet requested a review from nchaulet July 31, 2024 15:27

jillguyonnet marked this pull request as ready for review July 31, 2024 15:27

jillguyonnet requested a review from a team as a code owner July 31, 2024 15:27

nchaulet reviewed Jul 31, 2024

View reviewed changes

Add integration tests for POST new action

bd58978

nchaulet approved these changes Aug 2, 2024

View reviewed changes

jillguyonnet merged commit 718f6c3 into elastic:main Aug 2, 2024
26 checks passed

jillguyonnet deleted the fleet/185040-rbac-agents-write-api-2 branch August 2, 2024 13:04

kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Aug 2, 2024

jillguyonnet mentioned this pull request Aug 26, 2024

[Fleet] RBAC - Make agents write APIs space aware - 4/4 #191277

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] RBAC - Make read agent actions space aware #189519

[Fleet] RBAC - Make read agent actions space aware #189519

jillguyonnet commented Jul 30, 2024 •

edited

Loading

obltmachine commented Jul 30, 2024

jillguyonnet commented Jul 30, 2024

elasticmachine commented Jul 31, 2024

nchaulet Jul 31, 2024

jillguyonnet Aug 2, 2024

nchaulet Aug 2, 2024

jillguyonnet Aug 2, 2024

nchaulet Aug 2, 2024

kibana-ci commented Aug 2, 2024

nchaulet left a comment

[Fleet] RBAC - Make read agent actions space aware #189519

[Fleet] RBAC - Make read agent actions space aware #189519

Conversation

jillguyonnet commented Jul 30, 2024 • edited Loading

Summary

Details

GET /agents/action_status

POST /agents/{agentId}/actions

Other

Testing

Checklist

obltmachine commented Jul 30, 2024

🤖 GitHub comments

jillguyonnet commented Jul 30, 2024

elasticmachine commented Jul 31, 2024

nchaulet Jul 31, 2024

Choose a reason for hiding this comment

jillguyonnet Aug 2, 2024

Choose a reason for hiding this comment

nchaulet Aug 2, 2024

Choose a reason for hiding this comment

jillguyonnet Aug 2, 2024

Choose a reason for hiding this comment

nchaulet Aug 2, 2024

Choose a reason for hiding this comment

kibana-ci commented Aug 2, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

nchaulet left a comment

Choose a reason for hiding this comment

jillguyonnet commented Jul 30, 2024 •

edited

Loading