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

[Meta] Verify efficient usage of createPointInTimeFinder #203017

Open
kc13greiner opened this issue Dec 4, 2024 · 8 comments
Open

[Meta] Verify efficient usage of createPointInTimeFinder #203017

kc13greiner opened this issue Dec 4, 2024 · 8 comments
Labels
Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@kc13greiner
Copy link
Contributor

kc13greiner commented Dec 4, 2024 •
edited by TinaHeiligers
Loading

Summary

Using the createPointInTimeFinder() to load an unbounded amount of saved objects and keeping all of these in memory for analysis is not an efficient usage of this function. This inefficient usage can lead to unforeseen issues.

Example:

let allCases = [];
for await (const { saved_objects: cases } of finder.find()) {
  allCases.push(cases); // DONT persist all results into memory
}
const casesWithComments = countCasesWithComments(allCases);

Mitigation

To mitigate this, plugins should consider one of the following approaches (with the top items being more ideal solutions):

  1. Push as much data processing / computation to Elasticsearch as possible by using ES aggregations instead of having to load all data into Kibana. In some cases this might require changing how data is modelled to make this possible.
  2. Where ES aggregations aren't possible, keep the memory footprint as low as possible through a combination of the following:
    1. Only persist the aggregated result, not the raw results 
      let casesWithComments = 0;
for await (const { saved_objects: cases } of finder.find()) {
  casesWithComments += countCasesWithComments(cases); // DO only persist the aggregated result, only one page of cases results are kept in memory at a time
}
    2. Consider limiting the amount results as much as possible by applying a filter or only retrieving an upper bound of documents and using sort to retrieve the most relevant docs first
    3. Only retrieve the fields absolutely necessary
    4. Ensure that you have validation in place to apply sensible limits to how large a document can become. If a title field can contain 10mb of text then the above mitigations might not be useful when a page of 1000 documents are retrieved.
    5. (For telemetry) move from a pull model to a push model E.g. Instead of loading and iterating over all cases to count how many of them have adopted a new feature, increment a ui counter or create an EBT event every time a user enables the new feature.

Audit your code

Please analyze your team's owned files that contain usages of createPointInTimeFinder() as listed in the table below.

If you're are loading all results into memory please create a separate issue to remediate and link it in the provided column

If you're not loading all results into memory, please update the Verified column to Yes

Code Line # Line # Line # Team Verified? Issue (if needed)
x-pack/plugins/observability_solution/synthetics/server/routes/settings/params/params.ts 58 @elastic/obs-ux-management-team No
x-pack/plugins/observability_solution/synthetics/server/saved_objects/synthetics_monitor/get_all_monitors.ts 37 @elastic/obs-ux-management-team No
x-pack/plugins/observability_solution/synthetics/server/synthetics_service/project_monitor/project_monitor_formatter.ts 250 345 @elastic/obs-ux-management-team No
x-pack/plugins/observability_solution/synthetics/server/synthetics_service/synthetics_monitor/synthetics_monitor_client.ts 341 @elastic/obs-ux-management-team No
x-pack/plugins/observability_solution/synthetics/server/synthetics_service/synthetics_service.ts 294 586 @elastic/obs-ux-management-team No
x-pack/plugins/observability_solution/slo/server/lib/collectors/fetcher.ts 13 @elastic/obs-ux-management-team No
src/plugins/data_views/server/register_index_pattern_usage_collection.ts 95 @elastic/kibana-data-discovery No
src/plugins/data_views/server/deprecations/scripted_fields.ts 26 @elastic/kibana-data-discovery No
src/plugins/ftr_apis/server/routes/kbn_client_so/clean.ts 33 @elastic/kibana-core Yes
src/plugins/kibana_usage_collection/server/collectors/application_usage/telemetry_application_usage_collector.ts 72 110 @elastic/kibana-core Yes
src/plugins/kibana_usage_collection/server/collectors/application_usage/rollups/total.ts 32 55 @elastic/kibana-core Yes
src/plugins/kibana_usage_collection/server/collectors/common/counters.ts 34 @elastic/kibana-core Yes
src/plugins/kibana_usage_collection/server/collectors/ui_metric/telemetry_ui_metric_collector.ts 52 @elastic/kibana-core Yes
packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_client.ts 378 393 @elastic/kibana-core No
packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts 530 541 @elastic/kibana-core No
packages/core/saved-objects/core-saved-objects-api-server-internal/src/saved_objects_client.ts 181 185 @elastic/kibana-core No
packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts 548 @elastic/kibana-core No
packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/find_legacy_url_aliases.ts 38 @elastic/kibana-core No
packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.ts 48 @elastic/kibana-core No
packages/core/saved-objects/core-saved-objects-import-export-server-internal/src/export/saved_objects_exporter.ts 159 @elastic/kibana-core No
x-pack/plugins/alerting/server/application/backfill/methods/schedule/schedule_backfill.ts 119 @elastic/response-ops No
x-pack/plugins/alerting/server/application/rule/methods/bulk_delete/bulk_delete_rules.ts 147 @elastic/response-ops No
x-pack/plugins/alerting/server/application/rule/methods/bulk_disable/bulk_disable_rules.ts 142 @elastic/response-ops No
x-pack/plugins/alerting/server/application/rule/methods/bulk_edit/bulk_edit_rules.ts 288 @elastic/response-ops No
x-pack/plugins/alerting/server/application/rule/methods/bulk_enable/bulk_enable_rules.ts 162 @elastic/response-ops No
x-pack/plugins/alerting/server/backfill_client/backfill_client.ts 247 @elastic/response-ops No
x-pack/plugins/alerting/server/usage/lib/get_telemetry_from_kibana.ts 518 @elastic/response-ops No
x-pack/plugins/cases/server/saved_object_types/import_export/export.ts 110 @elastic/response-ops No
x-pack/plugins/cases/server/services/attachments/operations/get.ts 109 157 363 @elastic/response-ops No
x-pack/plugins/cases/server/services/user_actions/index.ts 562 @elastic/response-ops No
x-pack/plugins/cases/server/services/user_actions/operations/find.ts 213 @elastic/response-ops No
x-pack/plugins/fleet/server/services/epm/packages/cleanup.ts 80 @elastic/fleet No
x-pack/plugins/fleet/server/services/security/message_signing_service.ts 226 @elastic/fleet No
x-pack/plugins/fleet/server/services/security/uninstall_token_service/index.ts 385 470 @elastic/fleet No
x-pack/plugins/fleet/server/services/spaces/enable_space_awareness.ts 102 @elastic/fleet No
x-pack/plugins/lists/server/services/exception_lists/find_exception_list_items_point_in_time_finder.ts 99 @elastic/security-detection-engine No
x-pack/plugins/lists/server/services/exception_lists/find_exception_list_point_in_time_finder.ts 80 @elastic/security-detection-engine No
x-pack/plugins/lists/server/services/exception_lists/find_value_list_exception_list_items_point_in_time_finder.ts 80 @elastic/security-detection-engine No
x-pack/plugins/saved_objects_tagging/server/routes/lib/get_connection_count.ts 29 @elastic/appex-sharedux No
x-pack/plugins/saved_objects_tagging/server/services/tags/tags_client.ts 52 @elastic/appex-sharedux No
x-pack/plugins/security_solution/server/usage/queries/get_case_comments.ts 38 @elastic/security-data-analytics No
x-pack/plugins/security_solution/server/usage/queries/get_detection_rules.ts 62 @elastic/security-data-analytics No
x-pack/plugins/security_solution/server/usage/queries/legacy_get_rule_actions.ts 46 @elastic/security-data-analytics No
x-pack/plugins/encrypted_saved_objects/server/mocks.ts 39 @elastic/kibana-security No
x-pack/plugins/encrypted_saved_objects/server/saved_objects/index.ts 56 133 @elastic/kibana-security No
x-pack/plugins/spaces/server/spaces_client/spaces_client.ts 201 @elastic/kibana-security No
x-pack/plugins/watcher/server/routes/api/indices/register_get_index_patterns_route.ts 24 @elastic/kibana
@kc13greiner kc13greiner added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Dec 4, 2024
@TinaHeiligers
Copy link
Contributor

@kc13greiner what's the priority on auditing and mitigating?

@kc13greiner
Copy link
Contributor Author

@kc13greiner what's the priority on auditing and mitigating?

Heya @TinaHeiligers , I would appreciate the audit portion as soon as teams are able! Priority on mitigating/fixes will depend on other factors.

@shahzad31
Copy link
Contributor

@kc13greiner what's the alternative?

@TinaHeiligers TinaHeiligers mentioned this issue Dec 5, 2024
Closed
@kc13greiner
Copy link
Contributor Author

Heya @shahzad31 ! Mitigation ideas have been added above (Thanks, @rudolf !). There may be other solutions to fix this bad pattern, but we leave that up to the individual team

@cnasikas
Copy link
Member

Only retrieve the fields absolutely necessary

I recall some issues with ZDT and model versions returning fewer fields from ES, but I cannot find a related issue. @rudolf, do you have any ideas, or am I imaging this 🙂 ?

@rudolf
Copy link
Contributor

rudolf commented Dec 11, 2024

@cnasikas you're right, if you retrieve a subset of fields then we can't safely migrate the document https://github.com/elastic/kibana/blob/main/packages/core/saved-objects/core-saved-objects-api-server/src/apis/find.ts#L72-L79

I think the impact is limited to if you have a model version with a data_backfill change then you can't immediately use those fields in the same version in the fields argument and you'll have to do two releases.

There is also an edge case with encrypted saved objects where if you include an encrypted field in the fields argument but don't include all the AAD fields it would fail to encrypt and strip away those fields from the response. #200049

@jeramysoucy
Copy link
Contributor

@kc13greiner I took a look at our use cases and I think we're good.

  • x-pack/plugins/encrypted_saved_objects/server/mocks.ts | 39: this just a mock that returns the core mock
  • x-pack/plugins/encrypted_saved_objects/server/saved_objects/index.ts | 56 | 133: first mention is just a comment, and the actual use case is a sort of functional wrapper that uses a pmap when processing the results from the PIT finder.
  • x-pack/plugins/spaces/server/spaces_client/spaces_client.ts | 201: this just wraps the create PIT finder call and returns finder to the caller.

@afharo afharo mentioned this issue Dec 18, 2024
Closed
@TinaHeiligers
Copy link
Contributor

@kc13greiner Core's using aggregations for telemetry collection, so we're good there. The other instances are the base type and implementation of PIT find and our FTR SO client used in testing.
I've updated the table in the main description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@rudolf @shahzad31 @cnasikas @TinaHeiligers @kc13greiner @jeramysoucy