-
Notifications
You must be signed in to change notification settings - Fork 513
[sentinel_one] Enhance ECS Mappings and Unify Field Structures across all Data Streams #15931
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
base: main
Are you sure you want to change the base?
[sentinel_one] Enhance ECS Mappings and Unify Field Structures across all Data Streams #15931
Conversation
🚀 Benchmarks reportTo see the full report comment with |
💚 Build Succeeded
|
kcreddy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpascale43 can you confirm if this requires a dashboard changes as well?
| - rename: | ||
| field: json.data.threatClassificationSource | ||
| target_field: sentinel_one.activity.data.threat.classification.source | ||
| ignore_missing: true | ||
| - set: | ||
| field: sentinel_one.threat_classification.source | ||
| copy_from: sentinel_one.activity.data.threat.classification.source | ||
| ignore_empty_value: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that we are copying same value into 2 different fields with this change:
sentinel_one.activity.data.threat.classification.source and sentinel_one.threat_classification.source
Same with: sentinel_one.activity.site.name, and sentinel_one.site.name and several others.
Is this to avoid breaking-change?
It is mentioned here that this is worthy to be considered as a breaking-change, with updates to dashboards. @cpascale43 can you confirm if its still the case?
If its going to be a breaking-change, then we should ideally remove the previous fields sentinel_one.activity.data.threat.classification.source, sentinel_one.activity.site.name, etc. unless preserve_duplicate_custom_fields is true.
I checked the pre-built detection rules for sentinel_one and there is no effect there if its a breaking-change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, only a few fields are currently being copied into the sentinel_one.* namespace. We decided to keep the existing fields to avoid introducing breaking changes that could impact existing users.
I’ll wait for @cpascale43's suggestion on this. If we can treat it as a breaking change, we can proceed with removing the old fields and updating the dashboards if needed.
| cursor: | ||
| last_create_at: | ||
| value: '[[.last_event.createdAt]]' | ||
| ignore_empty_value: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this to changelog and commit message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added here to avoid CI failures since the related PR hadn’t been merged yet. As that PR has been merged today, this change will disappear once I sync the latest updates.
| - name: account | ||
| type: group | ||
| fields: | ||
| - name: name | ||
| type: keyword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you confirm if site.id and site.name is not available in all data streams as per API reference, or if we just ignored adding to the pipeline and hence ignored here as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had ignored this since it’s not present in the pipeline neither in the test samples. I’ll review the API documentation again to verify whether these fields exist in the data streams and will update both the pipeline and this if needed.
Proposed commit message
Checklist
changelog.ymlfile.How to test this PR locally
Related Issue