[filestream] Add native file identity. #14351
Conversation
belimawr
added
the
Team:Elastic-Agent-Data-Plane
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
| - version: "1.2.0" | ||
| changes: | ||
| - description: Add the native file identity | ||
| type: bugfix |
There was a problem hiding this comment.
Is it a bug because it should have had native file identity since the beginning?
There was a problem hiding this comment.
It's a bug because disabling fingerprint is broken in 9.x. Actually in 9.x disabling the fingerprint only removes the offset and length settings, without actually changing the file identity 🤦♂️.
Another way to see this is: we released a breaking change in 9.0 and did not update the integration to correctly support it.
| {{#if fingerprint }} | ||
| {{#unless file_identity_native }} | ||
| file_identity.fingerprint: ~ | ||
| prospector.scanner.fingerprint.enabled: true | ||
| prospector.scanner.fingerprint.offset: {{ fingerprint_offset }} | ||
| prospector.scanner.fingerprint.length: {{ fingerprint_length }} | ||
| {{/unless}} | ||
| {{/if}} | ||
|
|
||
| {{#if file_identity_native }} | ||
| {{#unless fingerprint }} | ||
| file_identity.native: ~ | ||
| prospector.scanner.fingerprint.enabled: false | ||
| {{/unless}} | ||
| {{/if}} |
There was a problem hiding this comment.
[Blocker]
I'm not sure it's behaving as expected. Better it isn't cleat to me what is the expected behaviour:
I played the user and enabled both, the result is none on the policy. Is it expected?
I think we could either define a precedence so when both are enabled it picks one, or if both are enabled produce an error.The error could be achieved by creating a invalid config. I'm not sure it's a good idea, but if we cannot do validation in the UI, it's a way to surface the error to the user.
[Blocker-ish suggestion|Question]
I believe there is a great change users will mix both file identity. Perhaps there is a option to use a drop down and select one or the other?
I know there are additional configuration to each one of them, but better at least ensure only one file identity is chosen.
[Suggestion]
As there isn't really an hierarchic between the elements in the advanced config, perhaps it could be achieved with a prefix in the name, like:
File Identity: Fingerprint: enabled
File Identity: Fingerprint: offsetFile Identity: Native: enabled
There was a problem hiding this comment.
I played the user and enabled both, the result is none on the policy. Is it expected?
Yes, this is the expected result. Because both settings cannot be enabled at the same time and there is no way in Fleet UI to prevent this/show an error, I opted by ignoring both and keep the integration working rather than having the Filestream input fail to start.
I think we could either define a precedence so when both are enabled it picks one (...)
That's the behaviour now, well if both are enabled, then Filestream default setting is used. Which is fingerprint for 9.x and Native for 8.x/7.x
(...) or if both are enabled produce an error. The error could be achieved by creating a invalid config. I'm not sure it's a good idea, but if we cannot do validation in the UI, it's a way to surface the error to the user.
Because both define the same key in the YAML and Fleet UI renders the YAML, the error is quite hard to understand, see the screenshots.
Error screenshots
Error pop-up at the bottom of the screen:
Clicking in see full error brings up the other pop-up
Regardless of how it is going to look in Fleet UI, I don't think this is a very helpful error.
I believe there is a great change users will mix both file identity. Perhaps there is a option to use a drop down and select one or the other?
I've looked into something like that in the past (probably when I was resurrecting this integration) and I didn't find any good solution. As far as I know there is no drop-down option.
I know there are additional configuration to each one of them, but better at least ensure only one file identity is chosen.
The native file identity does not have any extra configuration, only the fingerprint has extra ones.
I added the single prefix:
Screenshot
There was a problem hiding this comment.
Endpoint/Defend has it:
if you're in a rush, I could approve it, but if it can be done, I believe we'd avoid a lot of issues down the line
There was a problem hiding this comment.
It would be nice to have this merged by tomorrow, but I can wait a bit while exploring this option.
💚 Build Succeeded
History
cc @belimawr |
|
|
Package filestream - 1.2.0 containing this change is available at https://epr.elastic.co/package/filestream/1.2.0/ |
Proposed commit message
This commit adds the native file identity to the filestream package
Checklist
changelog.ymlfile.I have verified that any added dashboard complies with Kibana's Dashboard good practices## Author's Checklist## How to test this PR locallyelastic-packageecho "foo" > /tmp/log.log## Related issuesScreenshots