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

[Deprecations] Logs Sources settings in all spaces #203042

Merged

Conversation

afharo
Copy link
Member

@afharo afharo commented Dec 5, 2024

Summary

Resolves #202649.

Logs Sources deprecations now check all spaces for the deprecated configuration:
From the any space (including those not having any deprecations):
image

image

How to test the deprecation

Open the "Logs settings" and select Indices/Data view deprecated options:
image


The automated mitigation API PUT /api/logs_shared/deprecations/migrate_log_view_settings is also fixed to loop through all the spaces to resolve the issue.


In order to achieve this piece of work, I had to extend the current Saved Objects client to expose 1 new method:

  1. asScopedToNamespace: Returns a new client scoped to the specified Space ID.

Checklist

Identify risks

  • As highlighted by the @legrego, the new API lists all available Spaces, but it doesn't limit it by access to features inside that Space (i.e.: it doesn't list all Spaces the user has access to the o11y features only). This may lead to noise in the audit logs if abused.
    • Potential mitigation is to document the API to be used with caution. As it's mostly intended for deprecations and migrations.

@afharo afharo self-assigned this Dec 5, 2024
@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Team:obs-ux-logs Observability Logs User Experience Team labels Dec 5, 2024
@afharo afharo marked this pull request as ready for review December 5, 2024 16:00
@afharo afharo requested review from a team as code owners December 5, 2024 16:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

@afharo afharo force-pushed the deprecations/log-sources-check-in-all-spaces branch from 08bf159 to acf77fd Compare December 5, 2024 16:06
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Dec 5, 2024
Copy link
Contributor

github-actions bot commented Dec 5, 2024

🤖 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!)


/**
* Given a list of namespace strings, returns a subset that the user is authorized to search in.
* If a wildcard '*' is used, it is expanded to an explicit list of namespace strings.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make it super clear that the explicit list of namespace strings only includes the namespaces the current user has access to othewise consumers might incorrectly interpret that as a list of all namespaces.

Suggested change
* If a wildcard '*' is used, it is expanded to an explicit list of namespace strings.
* If a wildcard '*' is used, it is expanded to an explicit list of namespace strings the current user has access to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it's worth noting that this will include any namespace the user has ANY access to.

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok at a glance. The new APIs need tests, especially given that they're public and carry risks. It'd be helpful to point out the risks associated with using these APIs.
Initial code review only. I'll re-review once concerns are addressed.

getSearchableNamespaces: (namespaces: string[] | undefined) => Promise<string[]>;

/**
* Returns a new Saved Objects client scoped to the new namespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Returns a new Saved Objects client scoped to the new namespace.
* Returns a new Saved Objects client scoped to the current user and current namespace.

We're not creating a namespace, only a client that is scoped to one

getSearchableNamespaces: (namespaces: string[] | undefined) => Promise<string[]>;

/**
* Returns a new Saved Objects repository scoped to the new namespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Returns a new Saved Objects repository scoped to the new namespace.
* Returns a new Saved Objects repository scoped to a specific namespace.

@@ -25,4 +25,9 @@ export interface ISavedObjectsSpacesExtension {
* If a wildcard '*' is used, it is expanded to an explicit list of namespace strings.
*/
getSearchableNamespaces: (namespaces: string[] | undefined) => Promise<string[]>;
/**
* Returns a new Saved Objects Spaces Extension scoped to the new namespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Returns a new Saved Objects Spaces Extension scoped to the new namespace.
* Returns a new Saved Objects Spaces Extension scoped to a specific namespace.

export const getLogSourcesSettingDeprecationInfo = async (
params: LogSourcesSettingDeprecationParams
): Promise<DeprecationsDetails[]> => {
const allAvailableSpaces = await params.context.savedObjectsClient.getSearchableNamespaces(['*']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires end users to actively enable Log sources in each space.
We need that documented.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't fully understand this comment. We're reading a SO, and it's perfectly fine if the SO doesn't exist. Why would we need Log sources to be enabled in each space for this logic to work?

@jeramysoucy jeramysoucy self-requested a review December 9, 2024 13:16
Copy link
Contributor

@jeramysoucy jeramysoucy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@afharo afharo requested a review from a team as a code owner December 12, 2024 07:33
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-pack/plugins/reporting/server/deprecations/migrate_existing_indices_ilm_policy.test.ts lgtm

@Kerry350 Kerry350 self-requested a review December 13, 2024 16:16
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 16, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: 339a047
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-203042-339a047ee815

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-deprecations-server 13 14 +1
@kbn/core-deprecations-server-internal 3 4 +1
total +2
Unknown metric groups

API count

id before after diff
@kbn/core-deprecations-server 16 17 +1
@kbn/core-deprecations-server-internal 4 5 +1
@kbn/core-saved-objects-api-server 357 361 +4
@kbn/core-saved-objects-server 564 566 +2
total +8

History

cc @afharo

afharo added a commit that referenced this pull request Dec 16, 2024
## Summary

#203042 highlighted these APIs not having comments.


### Checklist


- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 16, 2024
## Summary

elastic#203042 highlighted these APIs not having comments.

### Checklist

- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials

(cherry picked from commit 25b171d)
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for improving this 👌

@afharo afharo merged commit 2ed3442 into elastic:main Dec 16, 2024
9 checks passed
@afharo afharo deleted the deprecations/log-sources-check-in-all-spaces branch December 16, 2024 12:40
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12352858418

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- [Inference Connector] Changed UI/UX due to the new RFC for the _inference/_service (#203363)

Manual backport

To create the backport manually run:

node scripts/backport --pr 203042

Questions ?

Please refer to the Backport tool documentation

afharo added a commit to afharo/kibana that referenced this pull request Dec 16, 2024
(cherry picked from commit 2ed3442)

# Conflicts:
#	x-pack/platform/plugins/private/translations/translations/zh-CN.json
@afharo
Copy link
Member Author

afharo commented Dec 16, 2024

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

afharo added a commit that referenced this pull request Dec 16, 2024
…204476)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Deprecations] Logs Sources settings in all spaces
(#203042)](#203042)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Alejandro Fernández
Haro","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-16T12:40:56Z","message":"[Deprecations]
Logs Sources settings in all spaces
(#203042)","sha":"2ed34427c056e935f77c6dded03afc11f82d301c","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","Team:Security","release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-logs"],"number":203042,"url":"https://github.com/elastic/kibana/pull/203042","mergeCommit":{"message":"[Deprecations]
Logs Sources settings in all spaces
(#203042)","sha":"2ed34427c056e935f77c6dded03afc11f82d301c"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/203042","number":203042,"mergeCommit":{"message":"[Deprecations]
Logs Sources settings in all spaces
(#203042)","sha":"2ed34427c056e935f77c6dded03afc11f82d301c"}}]}]
BACKPORT-->
JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Dec 19, 2024
## Summary

elastic#203042 highlighted these APIs not having comments.


### Checklist


- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:obs-ux-logs Observability Logs User Experience Team Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UA] Check all spaces in getLogSourcesSettingDeprecationInfo
7 participants