[Fleet] RBAC - Make agents write APIs space aware #188507

jillguyonnet · 2024-07-17T08:18:53Z

Summary

Relates to #185040

This PR makes the following Fleet agents API space aware:

PUT /agents/{agentId}
DELETE /agents/{agentId}
POST /agents/bulk_update_agent_tags

Actions created from POST /agents/bulk_update_agent_tags have the namespaces property populated with the current space.

I am opening this PR with a few endpoints to get early feedback and make this more agile. Other endpoints will be implemented in a followup PR.

Testing

Enroll an agent in the default space.
Create a custom space and enroll an agent in it.
From the default space, test the PUT /agents/{agentId} and DELETE /agents/{agentId} endpoints and check that the request fails for the agent in the custom space.
Same test from the custom space.
From the default space, test the POST /agents/bulk_update_agent_tags with all agents ids and check that only the agents in the default space get updated.
Same test from the custom space.
Review the actions created from the bulk tag updates (the easiest way is GET .fleet-actions/_search) and ensure the namespaces property is correct.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

obltmachine · 2024-07-17T08:19:08Z

🤖 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-18T15:44:37Z

/ci

elasticmachine · 2024-07-18T17:23:39Z

Pinging @elastic/fleet (Team:Fleet)

nchaulet · 2024-07-18T19:52:10Z

Overall it look good, I think the bulk_update_agent_tags may be a little more complex as it create .fleet-actions and .fleet-action-results here and we want to populate the namespaces field here, let me know if you think it's something that could be part of that PR or should have a follow up PR for that.

x-pack/plugins/fleet/server/services/agents/namespace.ts

jillguyonnet · 2024-07-19T09:52:07Z

@elasticmachine merge upstream

elasticmachine · 2024-07-19T11:06:14Z

💔 Build Failed

Buildkite Build
Commit: c4b034f

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #15 / Create renders correctly with required and defaultValue
[job] [logs] FTR Configs #43 / Space awareness agents POST /agents/bulkUpdateAgentTags should only update tags of agents in the same space when passing a list of agent ids
[job] [logs] FTR Configs #43 / Space awareness agents POST /agents/bulkUpdateAgentTags should only update tags of agents in the same space when passing a list of agent ids

Metrics [docs]

✅ unchanged

History

cc @jillguyonnet

jillguyonnet · 2024-07-22T09:20:38Z

@elasticmachine merge upstream

jillguyonnet · 2024-07-22T15:02:11Z

Hi @nchaulet thank you for your comment! In the spirit of keeping this PR small, I thought it could definitely be worthwhile adding a namespace field to the action and actions results documents. One thing I'm not sure of now that I've pushed this... is it correct to only assign the current namespace (the one the action was issued from) to actions? Agent can have multiple spaces, but I thought for actions only that one should be relevant.

In other words, if agent1 is in spaces A and B and I bulk update tags from space A, is it correct that the associated action has namespace: A? This would mean the action is not visible from space B, hence my doubt.

EDIT: never mind, I see the mapping already contains namespaces. I'll update the commit.

jillguyonnet · 2024-07-23T08:03:12Z

/ci

jillguyonnet · 2024-07-23T09:58:49Z

/ci

jillguyonnet · 2024-07-23T12:43:19Z

x-pack/plugins/fleet/server/services/agents/update_agent_tags_action_runner.ts

@@ -149,6 +149,9 @@ export async function updateTagsBatch(
  const versionConflictCount = res.version_conflicts ?? 0;
  const versionConflictIds = isLastRetry ? getUuidArray(versionConflictCount) : [];

+  const currentNameSpace = soClient.getCurrentNamespace();
+  const namespaces = currentNameSpace ? [currentNameSpace] : [];


@nchaulet Can you ensure this meets the expectations?

Yes that perfect 👍

nchaulet

Tested locally and it work as expected 🚀 one last thing I see we need to address is to put this behind the useSpaceAwareness flag so it do not impact user with the flag disabled

jillguyonnet · 2024-07-23T13:40:03Z

Tested locally and it work as expected 🚀 one last thing I see we need to address is to put this behind the useSpaceAwareness flag so it do not impact user with the flag disabled

Awesome, thanks! And right, I'll make that change right away 😅

jillguyonnet · 2024-07-23T14:14:02Z

@nchaulet Can I check one thing? GET /agents/{agentId} seems to be space-aware without the useSpaceAwareness flag (just tested on main). Is that expected?

For instance GET /agents/{agentId} from space A where agentId belongs to an agent in space B yields 404.

GET /agents isn't space aware without the flag though.

nchaulet · 2024-07-23T14:35:27Z

@nchaulet Can I check one thing? GET /agents/{agentId} seems to be space-aware without the useSpaceAwareness flag (just tested on main). Is that expected?

No this seems an issue, I will do a PR to address that

jillguyonnet · 2024-07-23T14:36:33Z

No this seems an issue, I will do a PR to address that

I can fix that as part of this, it would be pretty easy (as the handler uses the same utility function as the update and delete handlers to check the space).

nchaulet · 2024-07-23T14:40:14Z

I can fix that as part of this, it would be pretty easy (as the handler uses the same utility function as the update and delete handlers to check the space).

I think it's worth being fixed in his own PR so it could be backported to 8.15

jillguyonnet · 2024-07-23T14:43:48Z

I pushed a commit to take care of this PR (it also fixes GET /agents/{agentId} although that is not intentional). ~~Assuming it's OK, let me know if I should hold off until your fix is merged as there may be merge conflicts.~~
Edit: will take care of conflict in this PR 👍

…nts-write-api

criamico · 2024-07-24T13:33:59Z

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

+    const agent = await fleetContext.agentClient.asCurrentUser.getAgent(request.params.agentId);
+    if (!isAgentInNamespace(agent, coreContext.savedObjects.client.getCurrentNamespace())) {
+      throw new FleetNotFoundError(`${agent.id} not found in namespace`);
+    }


nit: I would be tempted to move these three line to a separate function, something like findAgentInNamespace, since it's repeated three times in the same file. What do you think?

nchaulet

🚀

criamico

Thanks for doing the last changes! LGTM! 🚢

kibana-ci · 2024-07-24T15:42:28Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 616d3bf

Failed CI Steps

FTR Configs #102

Test Failures

[job] [logs] FTR Configs #102 / Screenshots - serverless observability UI response ops docs observability cases Observability case settings case settings screenshots

Metrics [docs]

✅ unchanged

History

💛 Build #223397 was flaky 2e3e2cb
💔 Build #223242 failed ff98d7a
💛 Build #223156 was flaky 82d8158
💔 Build #223129 failed 5d00614

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

cc @jillguyonnet

## 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 <elasticmachine@users.noreply.github.com>

## Summary Closes elastic#185040 Followup to: elastic#188507 elastic#189519 elastic#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](elastic#191643) for those. ### A note on terminology As pointed out in elastic#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 <elasticmachine@users.noreply.github.com>

jillguyonnet self-assigned this Jul 17, 2024

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 17, 2024

[Fleet] RBAC - Make agents write APIs space aware

55389b2

jillguyonnet force-pushed the fleet/185040-rbac-agents-write-api branch from 6982047 to 55389b2 Compare July 17, 2024 08:30

Add bulk update agent tags endpoint

828a0af

jillguyonnet requested a review from nchaulet July 18, 2024 15:44

Add API integration test for bulk update tags

95cd473

jillguyonnet marked this pull request as ready for review July 18, 2024 17:23

jillguyonnet requested a review from a team as a code owner July 18, 2024 17:23

criamico reviewed Jul 19, 2024

View reviewed changes

x-pack/plugins/fleet/server/services/agents/namespace.ts Show resolved Hide resolved

Add unit tests for agent namespace helpers

6c11b42

Merge branch 'main' into fleet/185040-rbac-agents-write-api

c4b034f

jillguyonnet marked this pull request as draft July 22, 2024 09:20

elasticmachine and others added 2 commits July 22, 2024 19:20

Merge branch 'main' into fleet/185040-rbac-agents-write-api

7c00dde

Add namespace property to actions and action results

5d00614

jillguyonnet added 2 commits July 23, 2024 11:30

Change namespace to namespaces

6a87d09

Fix integration tests

82d8158

jillguyonnet marked this pull request as ready for review July 23, 2024 12:39

jillguyonnet commented Jul 23, 2024

View reviewed changes

nchaulet reviewed Jul 23, 2024

View reviewed changes

Put behind feature flag

ff98d7a

nchaulet mentioned this pull request Jul 23, 2024

[Fleet] Fix get one agent when feature flag disabled #188953

Merged

jillguyonnet added 2 commits July 24, 2024 08:57

Merge remote-tracking branch 'origin/main' into fleet/185040-rbac-age…

80ff19f

…nts-write-api

Fix unit tests

2e3e2cb

criamico reviewed Jul 24, 2024

View reviewed changes

nchaulet approved these changes Jul 24, 2024

View reviewed changes

Add small utility function

616d3bf

criamico approved these changes Jul 24, 2024

View reviewed changes

jillguyonnet merged commit 9e71c7e into elastic:main Jul 25, 2024
23 checks passed

jillguyonnet deleted the fleet/185040-rbac-agents-write-api branch July 25, 2024 07:36

kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Jul 25, 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 agents write APIs space aware #188507

[Fleet] RBAC - Make agents write APIs space aware #188507

jillguyonnet commented Jul 17, 2024 •

edited

Loading

obltmachine commented Jul 17, 2024

jillguyonnet commented Jul 18, 2024

elasticmachine commented Jul 18, 2024

nchaulet commented Jul 18, 2024

jillguyonnet commented Jul 19, 2024

elasticmachine commented Jul 19, 2024 •

edited

Loading

jillguyonnet commented Jul 22, 2024

jillguyonnet commented Jul 22, 2024 •

edited

Loading

jillguyonnet commented Jul 23, 2024

jillguyonnet commented Jul 23, 2024

jillguyonnet Jul 23, 2024

nchaulet Jul 23, 2024

nchaulet left a comment

jillguyonnet commented Jul 23, 2024 •

edited

Loading

jillguyonnet commented Jul 23, 2024

nchaulet commented Jul 23, 2024

jillguyonnet commented Jul 23, 2024

nchaulet commented Jul 23, 2024

jillguyonnet commented Jul 23, 2024 •

edited

Loading

criamico Jul 24, 2024

nchaulet left a comment

criamico left a comment

kibana-ci commented Jul 24, 2024

[Fleet] RBAC - Make agents write APIs space aware #188507

[Fleet] RBAC - Make agents write APIs space aware #188507

Conversation

jillguyonnet commented Jul 17, 2024 • edited Loading

Summary

Testing

Checklist

obltmachine commented Jul 17, 2024

🤖 GitHub comments

jillguyonnet commented Jul 18, 2024

elasticmachine commented Jul 18, 2024

nchaulet commented Jul 18, 2024

jillguyonnet commented Jul 19, 2024

elasticmachine commented Jul 19, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

History

jillguyonnet commented Jul 22, 2024

jillguyonnet commented Jul 22, 2024 • edited Loading

jillguyonnet commented Jul 23, 2024

jillguyonnet commented Jul 23, 2024

jillguyonnet Jul 23, 2024

Choose a reason for hiding this comment

nchaulet Jul 23, 2024

Choose a reason for hiding this comment

nchaulet left a comment

Choose a reason for hiding this comment

jillguyonnet commented Jul 23, 2024 • edited Loading

jillguyonnet commented Jul 23, 2024

nchaulet commented Jul 23, 2024

jillguyonnet commented Jul 23, 2024

nchaulet commented Jul 23, 2024

jillguyonnet commented Jul 23, 2024 • edited Loading

criamico Jul 24, 2024

Choose a reason for hiding this comment

nchaulet left a comment

Choose a reason for hiding this comment

criamico left a comment

Choose a reason for hiding this comment

kibana-ci commented Jul 24, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

jillguyonnet commented Jul 17, 2024 •

edited

Loading

elasticmachine commented Jul 19, 2024 •

edited

Loading

jillguyonnet commented Jul 22, 2024 •

edited

Loading

jillguyonnet commented Jul 23, 2024 •

edited

Loading

jillguyonnet commented Jul 23, 2024 •

edited

Loading