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.

Sign up for GitHub

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

Jump to bottom

[Fleet] Fix space_ids validation and spaces UI #191083

Merged
merged 8 commits into from
Aug 23, 2024
Merged

[Fleet] Fix space_ids validation and spaces UI #191083

merged 8 commits into from
Aug 23, 2024

Conversation

nchaulet
Copy link
Member

Summary

That PR do a few fixes following the introduction of multiple spaces in #189387

  • Fix the UI to reflex we support multiple spaces and not just one
  • Fix the API validation to support an empty space_ids array (an empty space_ids array is handled as the field is not provided)

UI Changes

Screenshot 2024-08-22 at 9 36 37 AM

@nchaulet nchaulet added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team labels Aug 22, 2024
@nchaulet nchaulet self-assigned this Aug 22, 2024
@nchaulet nchaulet requested a review from a team as a code owner August 22, 2024 13:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@nchaulet nchaulet requested a review from jillguyonnet August 22, 2024 13:39
@obltmachine
Copy link

🤖 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
Copy link
Contributor

The changes here look good. I have a few questions that may not be directly related to this change:

  1. UI: the space selection component doesn't seem to reflect which spaces the policy is already part of. For example in the screenshots below, the policy is visible in all the spaces but you wouldn't know from the UI:
Screenshot 2024-08-22 at 16 08 09 Screenshot 2024-08-22 at 16 08 21

  1. It looks like the API allows adding spaces, but not removing?

  2. Looking at the policy settings, I'm wondering if there is an existing confusion between spaceId and namespace (at least in the PRs related to [Fleet] Make Fleet agents write APIs space aware  #185040). It looks like these are two different concepts, but for example:

    • In the example policy above, the namespace property is default while spaceIds would be an array.
    • The space aware agent actions generated from changes related to [Fleet] Make Fleet agents write APIs space aware  #185040 contain a namespace property which is really the space the command originated from.

Does the above look right to you or do we need to clarify the terminology?

@nchaulet
Copy link
Member Author

nchaulet commented Aug 22, 2024 •
edited
Loading

@jillguyonnet When testing did you enabled space awareness with curl -u elastic:changeme -XPOST "http://localhost:5601/internal/fleet/enable_space_awareness" -H "kbn-xsrf: reporting" -H 'elastic-api-version: 1' ?

The UI is only working when the feature is enabled, I just pushed a change to show that UI only when it's enabled and not only based on the feature flag useSpaceAwareness

It looks like the API allows adding spaces, but not removing?

It should work for removing space too, as long you keep at least one space, I just fixed the UI to not allow to clear that field

Yes the terminology is kind of confusing between kibana space and datastream namespace. I think we should try to use space ID (it's how it's done in the rest of Kibana) when we talk about kibana space and namespace for datastream namespaces. To add a little more to that confusion the space ID is stored in a field call namespaces in the saved object or .fleet-* indices

@jillguyonnet
Copy link
Contributor

Thanks for the clarifications @nchaulet! Yes, it seems I reset my settings prior to testing your branch and forgot to re-enable space awareness, sorry about that.

It all looks good. One thing I've noticed that might be worthwhile considering is that if you remove a space from that same space (e.g. remove 'Default' while in the Default space), you then hit an error because the UI tries to navigate to the policy page:
Screenshot 2024-08-22 at 17 06 44

Thanks for the input on naming. I will take that into account in my upcoming PR and ping you to look at the change.

@nchaulet
Copy link
Member Author

nchaulet commented Aug 22, 2024 •
edited
Loading

Thanks for the clarifications @nchaulet! Yes, it seems I reset my settings prior to testing your branch and forgot to re-enable space awareness, sorry about that.

No worries, it allows to catch a bug on not using the right source of truth for the feature being enabled :)

It all looks good. One thing I've noticed that might be worthwhile considering is that if you remove a space from that same space (e.g. remove 'Default' while in the Default space), you then hit an error because the UI tries to navigate to the policy page:

That a good catch we could probably redirect to another space here, I will create a follow up issue

@nchaulet nchaulet enabled auto-merge (squash) August 22, 2024 15:31
@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@jillguyonnet jillguyonnet mentioned this pull request Aug 23, 2024
Closed
@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@nchaulet nchaulet merged commit 8f7ea3c into elastic:main Aug 23, 2024
22 checks passed
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 1.8MB 1.8MB -41.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/test-suites-xpack 708 709 +1

Total ESLint disabled count

id before after diff
@kbn/test-suites-xpack 731 732 +1

History

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

cc @nchaulet

@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Aug 23, 2024
@jillguyonnet jillguyonnet mentioned this pull request Aug 26, 2024
Merged
2 tasks
jillguyonnet added a commit that referenced this pull request Aug 30, 2024
@jillguyonnet @elasticmachine
[Fleet] RBAC - Make agents write APIs space aware - 4/4 (#191277)
3ba243d 
## 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
Reviewers

@jillguyonnet jillguyonnet jillguyonnet approved these changes

Assignees

@nchaulet nchaulet

Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nchaulet @elasticmachine @obltmachine @jillguyonnet @kibana-ci @kibanamachine