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

[Security Solution] Remove index false from artifact saved objects mappings #155204

Merged
merged 12 commits into from
Apr 25, 2023

Conversation

kevinlog
Copy link
Contributor

@kevinlog kevinlog commented Apr 18, 2023

Updates the mappings for the artifact saved objects

This effort is part of https://github.com/elastic/security-team/issues/6268 and https://github.com/elastic/dev/issues/2189

@kevinlog kevinlog added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.8.0 labels Apr 18, 2023
@kevinlog kevinlog requested a review from a team as a code owner April 18, 2023 21:22
@kevinlog kevinlog requested review from pzl and tomsonpl April 18, 2023 21:22
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

@kevinlog kevinlog requested review from paul-tavares and dasansol92 and removed request for pzl and tomsonpl April 18, 2023 21:23
@kevinlog kevinlog requested a review from a team as a code owner April 18, 2023 22:11
@rylnd
Copy link
Contributor

rylnd commented Apr 18, 2023

@kevinlog if none of these fields were being indexed previously, I suspect that mapping most of them is unnecessary. Ideally, all of these fields would be removed from the mapping declaration entirely, and reflected in a schema elsewhere, but the tricky part of this issue is determining which fields are actively used, or may be used in the future (since the whole point is to prevent any non-additive changes in the future).

For AET we're effectively removing all the index: false fields from each SO and seeing if anything breaks (both through automated tests, smoke tests, and domain knowledge of those SOs (😅)).

@kevinlog
Copy link
Contributor Author

@rylnd

Ideally, all of these fields would be removed from the mapping declaration entirely, and reflected in a schema elsewhere, but the tricky part of this issue is determining which fields are actively used, or may be used in the future (since the whole point is to prevent any non-additive changes in the future).

Makes sense. I want @dasansol92 and @paul-tavares to take a look first before removing anything else. I ran a smoke test and it looked OK. Tests are running now.

It looks like we have these represented in a schema here: https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/common/endpoint/schema/common.ts#L10

@@ -14,36 +14,31 @@ export const exceptionsArtifactSavedObjectType = ArtifactConstants.SAVED_OBJECT_
export const manifestSavedObjectType = ManifestConstants.SAVED_OBJECT_TYPE;

export const exceptionsArtifactSavedObjectMappings: SavedObjectsType['mappings'] = {
dynamic: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

So this SO type (endpoint:user-artifact) is actually one we no longer use and have an issue open to remove it (team issue 6214), so whatever changes you want to do here, you can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paul-tavares I went ahead and removed the SO here: 19d06c7

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

@kevinlog kevinlog requested a review from a team as a code owner April 19, 2023 19:16
properties: {
created: {
type: 'date',
index: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see an explanation in the comments/PR so I'll ask here: why are we adding mappings for these fields, when they previously weren't needed? These mappings can be added at any point in the future, but this is our last opportunity to remove anything unnecessarily indexed (or specified to not be indexed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rylnd apologies - I was doing some smoke testing locally with the fields removed first. All looks OK, so I pushed up the changes.

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

thats Awesome - Thanks @kevinlog for removing that type.

@kevinlog kevinlog requested a review from TinaHeiligers April 20, 2023 22:47
@kevinlog
Copy link
Contributor Author

Hi @TinaHeiligers - could I get one more review from kibana-core on this? I made some changes involving removing an SO since your initial review. Apologies for the changes, this went through a couple rounds of discussion with the team.

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.

The updated code LGTM.
Thanks for cleaning up!

@kevinlog kevinlog requested a review from a team as a code owner April 25, 2023 11:32
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Changes in saved_object_api_integration/common/fixtures/es_archiver/saved_objects/spaces/mappings.json LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 396 399 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 476 479 +3
total +5

History

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

@kevinlog kevinlog merged commit 2fad86a into elastic:main Apr 25, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 25, 2023
@kevinlog kevinlog deleted the task/remove-enabled-index-false-fields branch April 25, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants