Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fleet] Make Fleet agents write APIs space aware #185040

Closed
14 tasks done
nchaulet opened this issue Jun 7, 2024 · 1 comment · Fixed by #191277
Closed
14 tasks done

[Fleet] Make Fleet agents write APIs space aware #185040

nchaulet opened this issue Jun 7, 2024 · 1 comment · Fixed by #191277
Assignees
Labels
QA:Needs Validation Issue needs to be validated by QA Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@nchaulet
Copy link
Member

nchaulet commented Jun 7, 2024

Description

Related to https://github.com/elastic/ingest-dev/issues/2893
Similar to #184869

As part of making Fleet space aware we should make the Fleet agent APIs space aware (the read API should already be done in #184869)

Details

When the useSpaceAwareness feature flag is enabled we shoud do:

Access validation

For every API we should ensure the user only access agent in the current space, this mean:

  • For individual agent actions we should validate the agent is the current space (if soClient.getCurrentNamespace() we are in the default space, we should validate namespaces:['default'] or not namespaces:*
  • For action using kuery we should add the following namespaces:['default'] or not namespaces:* to the provided kuery (there is an helper _joinFilters that could help to do so.

The saved object client used in those API should be scoped to the current space (if we need an internal client we could use appContextService.getInternalUserSOClientForSpaceId(spaceId?), this should ensure we access related object like agent policy in the same space only.

Write namespaces for related action

All the .fleet-actions and .fleet-action-results should be created with the namespaces: [currentSpaceId] property

APIs

Agent Action

  • POST /agents/{agentId}/reassign
  • POST /agents/{agentId}/unenroll
  • POST /agents/{agentId}/request_diagnostics
  • POST /agents/{agentId}/upgrade

Agents Bulk API

  • POST /agents/bulk_reassign
  • POST /agents/bulk_request_diagnostics
  • POST /agents/bulk_unenroll
  • POST /agents/bulk_update_agent_tags
  • POST /agents/bulk_upgrade

Agent Misc

  • DELETE /agents/{agentId}
  • PUT /agents/{agentId}

Agent Actions

  • POST /agents/{agentId}/actions/{actionId}/cancel
  • GET /agents/action_status
  • POST /agents/{agentId}/actions

Implementation details/guideline

With a scoped saved objet client space could be retrieved with the soClient.getCurrentNamespace()

To write API integration test for this, I created a new tests suite with the space awarness flag enabled in x-pack/test/fleet_api_integration/apis/space_awarness/

@nchaulet nchaulet added Team:Fleet Team label for Observability Data Collection Fleet team Team:Elastic-Agent-Control-Plane labels Jun 7, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

jillguyonnet added a commit that referenced this issue Jul 25, 2024
## 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

1. Enroll an agent in the default space.
2. Create a custom space and enroll an agent in it.
3. 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.
4. Same test from the custom space.
5. 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.
6. Same test from the custom space.
7. 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

- [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

---------

Co-authored-by: Elastic Machine <[email protected]>
jillguyonnet added a commit that referenced this issue Jul 26, 2024
## Summary

I found a small bug while working on
#185040: when agent policies are
created, there should be a root-level `namespaces` property, which is
currently missing.

`GET .fleet-policies/_mapping` contains a `namespaces` property with
`keyword` type that was added in
https://github.com/elastic/elasticsearch.

Note: I was looking into removing the existing `data.namespaces`
property, however I don't see any issues with it. It is coming from
[here](https://github.com/nchaulet/kibana/blob/f77e4d243fca87a87eeae1409f27876cc7ea0836/x-pack/plugins/fleet/server/services/agent_policy.ts#L1140),
i.e. the `data` property is generated from the full agent policy which
already has a `namespaces` property.

### 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
jillguyonnet added a commit that referenced this issue Aug 2, 2024
## 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

1. 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.
2. 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

- [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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
@kpollich kpollich added QA:Needs Validation Issue needs to be validated by QA and removed Team:Elastic-Agent-Control-Plane labels Aug 13, 2024
jillguyonnet added a commit that referenced this issue Aug 19, 2024
## Summary

Relates to #185040

This PR makes the following Fleet agents API space aware (behind
`useSpaceAwareness` feature flag):
* `POST /agents/{agentId}/reassign`
* `POST /agents/{agentId}/upgrade`
* `POST /agents/bulk_reassign`
* `POST /agents/bulk_upgrade`
* `POST /agents/{agentId}/actions/{actionId}/cancel`

While working on that last endpoint, I noticed and fixed an error in the
documentation.

### 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: Nicolas Chaulet <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
jillguyonnet added a commit that referenced this issue Aug 30, 2024
## 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA:Needs Validation Issue needs to be validated by QA Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants