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

[crowdstrike] Expose FDR cache options for more flexibility #9063

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

marc-gr
Copy link
Contributor

@marc-gr marc-gr commented Feb 6, 2024

Proposed commit message

Exposes cache options for the FDR data stream.

Using the cache leads to more memory consumption, by exposing this users can tweak how cache is evicted to limit memory usage if needed.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

@marc-gr marc-gr added enhancement New feature or request Integration:Crowdstrike Team:Security-Service Integrations Security Service Integrations Team labels Feb 6, 2024
@marc-gr marc-gr requested a review from efd6 February 6, 2024 09:15
@marc-gr marc-gr requested a review from a team as a code owner February 6, 2024 09:15
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@marc-gr marc-gr force-pushed the improve_crowdstrike_cache_options branch from b40269e to f195c59 Compare February 6, 2024 09:16
@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

Comment on lines 67 to 74
- name: metadata_cache_capacity
required: true
show_user: true
title: Metadata cache capacity
description: The maximum amount of metadata objects to cache. Operations that would cause the capacity to be exceeded will result in evictions of the oldest elements. The capacity should not be lower than the number of elements that are expected to be referenced when processing the input as evicted elements are lost. Values at or below zero indicate no limit.
type: text
multi: false
default: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's a good idea to expose this. If it is exposed, the FOOTGUN sign needs to be a lot bigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in, unexpectedly evicting objects, or?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. If the user sets this too low, they will have holes in their enrichment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't this also happen after the TTL expires?

Copy link
Contributor

@efd6 efd6 Feb 7, 2024

Choose a reason for hiding this comment

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

All to puts will call the store's evictExpired method. This will do TTL-base expiry, but if the capacity of the store is positive there will be enforced eviction tighter than the TTL until the number of elements is within capacity. This means that earlier-seen elements will be absent from the cache and so we'll have an enrichment blind spot.

The logic for this is here https://github.com/elastic/beats/blob/7546ae1ceefa19c9a4886ab012fa661eaeaccf4b/libbeat/processors/cache/mem_store.go#L174

and here https://github.com/elastic/beats/blob/7546ae1ceefa19c9a4886ab012fa661eaeaccf4b/libbeat/processors/cache/mem_store.go#L198-L205

But overall, yes, but we have a clear understanding of the dynamics of TTL; we can know definitively how long the refresh cycle is because it is configured, but the numbers of entities in the aidmaster/user_info are a non-configured function of the system that is being monitored. In principle they can be known and accounted for, but in practice, I can see situation where the integration is configured in one setting, the system gets bigger and then metadata is not applied in confusing ways. I'd prefer to reduce the likelihood of that.

@mbudge
Copy link

mbudge commented Feb 8, 2024

Just a reminder user enrichment doesn't work #8742

@marc-gr
Copy link
Contributor Author

marc-gr commented Feb 13, 2024

@efd6 I added a warning to the capacity setting and set it hidden by default. If you still think there is no reason to expose it in any context, I am not against removing it.

@elasticmachine
Copy link

💚 Build Succeeded

History

Copy link

Quality Gate passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No Coverage information No data about Coverage
0.0% 0.0% Duplication on New Code

See analysis details on SonarQube

@marc-gr marc-gr merged commit 92900f6 into elastic:main Feb 13, 2024
5 checks passed
@marc-gr marc-gr deleted the improve_crowdstrike_cache_options branch February 13, 2024 10:32
@elasticmachine
Copy link

Package crowdstrike - 1.29.0 containing this change is available at https://epr.elastic.co/search?package=crowdstrike

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:Crowdstrike Team:Security-Service Integrations Security Service Integrations Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants