Cleanup `enabled: false` saved objects mappings #149102

rudolf · 2023-01-18T10:27:52Z

Summary

Clean up saved object mappings to reduce the field count and make mappings more future-proof.

This PR attempts to remove all enabled: false mappings. It's not possible to toggle the enabled property so it becomes impossible to ever query over a disabled field. type: 'object', enabled: 'false' also provides worse validation since it's possible to index a string into such an object. So using dynamic: false, properties: {} provides better validation and is more future-proof allowing us to query over these fields should the need arise.

Since these fields can't be queried over it should be safe to remove these mappings but please review and/or test to ensure there's no regressions. If your plugin every relied on the lack of validation for being able to ingest a string field into an type: 'object', enabled: 'false' field this PR would now cause failures.

Related: https://github.com/elastic/dev/issues/2189

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

rudolf · 2023-01-18T14:21:56Z

x-pack/plugins/fleet/server/saved_objects/index.ts

-              },
-            },
-          },
+          type: 'nested', // TODO Review: If we're not searching `nested` isn't necessary but it's not possible to change the type later


@elastic/fleet

Nested mappings is rather "expensive" because it creates a document for each object in the array. It also enforces other limits like 50 nested fields per index and only 10000 objects per array.
https://www.elastic.co/guide/en/elasticsearch/reference/current/nested.html#_limits_on_nested_mappings_and_objects

Given that we previously had enabled: false we were never querying these fields. By extension we wouldn't need the advantages of the nested type either. Which makes me lean towards removing it.

BUT it's not possible to change the type later on (it's an incompatible mapping change) so if we have any reason to believe we'd need to run nested queries on the inputs in the future it would be better keeping it.

@rudolf - Thanks for the ping. I can't imagine we'd need to run nested queries on this inputs field, so I'd be in favor of just biting the bullet on this mapping change to just dynamic: false + properties: {} like the others now.

elasticmachine · 2023-01-18T15:40:11Z

Pinging @elastic/fleet (Team:Fleet)

lukasolson

DataDiscovery changes LGTM

nreese · 2023-01-18T18:39:53Z

x-pack/plugins/maps/server/saved_objects/setup_saved_objects.ts

@@ -61,23 +61,6 @@ export function setupSavedObjects(
      );
    },
  });
-


Thanks, this resolves #139347 for maps.

Question, will this impact upgrades? If a cluster has set this value in previous versions, will the cluster be able to upgrade? We have seen some SDHs regarding upgrade failures for maps-telemetry and clusters that have previously set this value.

By adding it to the REMOVED_TYPES list in packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/unused_types.ts any remaining maps-telemetry documents will be deleted upon the next upgrade. So it won't impact upgrades.

cnasikas

ResponseOps changes LTGM. Code review only.

elasticmachine · 2023-01-19T10:50:57Z

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

tomsonpl · 2023-01-30T12:03:14Z

Osquery tests should be passing now if you update the branch. Let's see then, thanks!

tomsonpl

Defend Workflows changes LGTM ✅

drewdaemon

Code review. The typings give me confidence that a string isn't being inserted into any of the changed properties in saved queries or saved search sessions.

rylnd

The params on our legacy actions SO was always an object and these objects are now deprecated, so LGTM.

kibana-ci · 2023-02-09T11:14:13Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: a977e92

Failed CI Steps

FTR Configs #5

Test Failures

[job] [logs] FTR Configs #5 / dashboard custom time range by reference can add a custom time range to panel
[job] [logs] FTR Configs #5 / dashboard custom time range by reference can remove a custom time range from a panel
[job] [logs] FTR Configs #5 / dashboard custom time range by value can remove a custom time range from a panel
[job] [logs] FTR Configs #5 / dashboard custom time range embeddable that does not support time should not show custom time picker in flyout

Metrics [docs]

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id	before	after	diff
`ingest-package-policies`	38	17	-21

History

💔 Build #106578 failed 8d158be
💛 Build #103949 was flaky 3dd40d7
💔 Build #103761 failed d165f61
💔 Build #103657 failed 647c639
💔 Build #103304 failed c8335bf

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

cc @rudolf

…appings (#154473) **Related to:** elastic/security-team#6268 (internal) ## Summary For each of our Saved Object types, we must: 1. Remove any SO field mappings with `index: false` (or `enabled: false`, although a first pass was done in #149102) from our SO `mappings` declarations 2. Audit and remove any _unused_ SO fields to minimize our footprint This PR addresses these two requirements for this `security-rule` saved object type (prebuilt rule asset). ## Details Specifically, the PR removes the `name` field from the mappings because: - We don't filter, sort, search, or aggregate by it. - We might need to do it in the future for our prebuilt rule upgrade/installation workflows, but for now we're going to implement filtering, sorting, and pagination on the client side, thus there's no need for this mapping server-side. <img width="1295" alt="Screenshot 2023-04-05 at 15 19 10" src="https://user-images.githubusercontent.com/7359339/230094740-706a9a78-fec3-469e-a4ad-e8b7d7309c78.png"> Also, we may need to add more fields to this mapping in the future to implement further improvements for the prebuilt rule installation, upgrade, or deprecation workflows. ### Checklist - [ ] [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] The unit test for SO mapping hashes has been updated. - [ ] More tests will be added as part of #148176 and #148192

rudolf added 2 commits January 18, 2023 13:24

Remove unused type maps-telemetry

19e63eb

Remove enabled:false mappings

46d8c13

rudolf force-pushed the cleanup-so-mappings branch from 84787b5 to 46d8c13 Compare January 18, 2023 12:24

rudolf commented Jan 18, 2023

View reviewed changes

rudolf changed the title ~~Cleanup saved objects mappings~~ Cleanup enabled: false saved objects mappings Jan 18, 2023

rudolf marked this pull request as ready for review January 18, 2023 14:27

rudolf requested review from a team as code owners January 18, 2023 14:27

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jan 18, 2023

lukasolson approved these changes Jan 18, 2023

View reviewed changes

nreese reviewed Jan 18, 2023

View reviewed changes

nreese mentioned this pull request Jan 18, 2023

[Maps] Remove maps-telemetry SO registration #139347

Closed

cnasikas approved these changes Jan 19, 2023

View reviewed changes

rudolf added 3 commits January 19, 2023 11:02

Merge branch 'main' into cleanup-so-mappings

e2b1316

Remove nested type from ingest-package-policies mappings

fa6339a

Remove type: object, enabled: false mappings pass 2

1aa1e94

rudolf requested review from a team as code owners January 19, 2023 10:10

rudolf requested review from patrykkopycinski and paul-tavares January 19, 2023 10:10

rudolf added 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 Feature:Migrations labels Jan 19, 2023

Merge branch 'main' into cleanup-so-mappings

d165f61

tomsonpl approved these changes Jan 30, 2023

View reviewed changes

drewdaemon approved these changes Jan 30, 2023

View reviewed changes

Merge branch 'main' into cleanup-so-mappings

3dd40d7

mikecote mentioned this pull request Jan 31, 2023

Add UUID to RuleAction #148038

Merged

patrykkopycinski approved these changes Feb 2, 2023

View reviewed changes

afharo assigned rudolf Feb 6, 2023

TinaHeiligers mentioned this pull request Feb 6, 2023

[Fleet] Add ability to show FQDN of agents #150239

Merged

Merge branch 'main' into cleanup-so-mappings

8d158be

rudolf enabled auto-merge (squash) February 8, 2023 18:33

paul-tavares approved these changes Feb 8, 2023

View reviewed changes

rylnd approved these changes Feb 8, 2023

View reviewed changes

Merge branch 'main' into cleanup-so-mappings

a977e92

rudolf requested a review from a team as a code owner February 9, 2023 09:36

rudolf merged commit 111d9d1 into main Feb 9, 2023

rudolf deleted the cleanup-so-mappings branch February 9, 2023 11:14

kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Feb 9, 2023

banderror mentioned this pull request Apr 5, 2023

[Security Solution] Remove the name field from the security-rule SO mappings #154473

Merged

3 tasks

rudolf added the Epic:ScaleMigrations Scale upgrade migrations to millions of saved objects label Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup `enabled: false` saved objects mappings #149102

Cleanup `enabled: false` saved objects mappings #149102

rudolf commented Jan 18, 2023 •

edited

Loading

rudolf Jan 18, 2023

kpollich Jan 18, 2023

elasticmachine commented Jan 18, 2023

lukasolson left a comment

nreese Jan 18, 2023 •

edited

Loading

rudolf Jan 19, 2023

cnasikas left a comment

elasticmachine commented Jan 19, 2023

tomsonpl commented Jan 30, 2023

tomsonpl left a comment

drewdaemon left a comment

rylnd left a comment

kibana-ci commented Feb 9, 2023

@@ @@ -61,23 +61,6 @@ export function setupSavedObjects( @@
                     );
                   },
                 });

Cleanup enabled: false saved objects mappings #149102

Cleanup enabled: false saved objects mappings #149102

Conversation

rudolf commented Jan 18, 2023 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

rudolf Jan 18, 2023

Choose a reason for hiding this comment

kpollich Jan 18, 2023

Choose a reason for hiding this comment

elasticmachine commented Jan 18, 2023

lukasolson left a comment

Choose a reason for hiding this comment

nreese Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

rudolf Jan 19, 2023

Choose a reason for hiding this comment

cnasikas left a comment

Choose a reason for hiding this comment

elasticmachine commented Jan 19, 2023

tomsonpl commented Jan 30, 2023

tomsonpl left a comment

Choose a reason for hiding this comment

drewdaemon left a comment

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 9, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Saved Objects .kibana field count

History

Cleanup `enabled: false` saved objects mappings #149102

Cleanup `enabled: false` saved objects mappings #149102

rudolf commented Jan 18, 2023 •

edited

Loading

nreese Jan 18, 2023 •

edited

Loading