Make prospector.scanner.fingerprint configurable #12274
Conversation
MichaelKatsoulis
requested review from
gizas,
constanca-m and
tetianakravchenko
andrewkroh
added
Integration:kubernetes
🚀 Benchmarks reportTo see the full report comment with |
| - name: fingerprint | ||
| title: Fingerprint settings | ||
| show_user: false | ||
| description: Configuration options for prospector.scanner.fingerprint. By default fingerprint is enabled, offset is 0 and length 1024. |
There was a problem hiding this comment.
maybe instead of listing defaults in description, would be better to add a link to the doc instead - https://www.elastic.co/guide/en/beats/filebeat/current/filebeat-input-filestream.html#filebeat-input-filestream-scan-fingerprint
| multi: false | ||
| required: true | ||
| show_user: false | ||
| description: If prospector.scanner.fingerprint.enabled is set to false, file_identity fingerprint must be switched off. If it set to true, file_identity fingerprint must be switched on. |
There was a problem hiding this comment.
I think those conditions might be quite confusing, wondering if suggested option wouldn't be easier
| file_identity.fingerprint: ~ | ||
| {{/if}} |
There was a problem hiding this comment.
To avoid conditions in fields description, we could use smth like below instead:
{{#if useFingerprint}}
file_identity.fingerprint: ~
prospector:
scanner:
fingerprint:
enabled: true
{{ fingerprintConfig }}
{{/if}}
(not tested)
fingerprintConfig variable then would be:
offset: 0
length: 1024
There was a problem hiding this comment.
I thought about this but then it would be confusing to the users as to what options exactly they can set in fingerprintConfig yaml. For example they can set offset and length but not enabled. And if they set enabled as well then there will be an error.
Maybe theenabled could also be part of the available configuration options, to avoid errors, but it won't have any effect as it will be under the {{#if useFingerprint}}
Like this:
{{#if useFingerprint}}
file_identity.fingerprint: ~
prospector:
scanner:
{{ fingerprint }}
{{/if}}
There was a problem hiding this comment.
@tetianakravchenko check the new commit
There was a problem hiding this comment.
Maybe theenabled could also be part of the available configuration options, to avoid errors, but it won't have any effect as it will be under the {{#if useFingerprint}}
but does it change much from the previous option? I mean that we still allow config like:
useFingerprint: true
fingerprint:
enabled: false
that is not supported, no?
As an option in useFingerprint description can be mentioned that prospector.scanner.fingerprint will be enabled automatically, for configuring additional prospector.scanner.fingerprint parameters please use Fingerprint settings. WDYT?
Another reason to put prospector.scanner.fingerprint to the template - now we allow to configure all prospector.scanner.*, not just the prospector.scanner.fingerprint is it expected behavior?
There was a problem hiding this comment.
Another reason to put prospector.scanner.fingerprint to the template - now we allow to configure all prospector.scanner.*, not just the prospector.scanner.fingerprint is it expected behavior?
In theory they could configure all prospector.scanner.* but that was not the idea. The idea was to configure fingerprint only. It was just more clear if they also have to set the fingerprint key.
We could change that and go one level deeper.
useFingerprint: true
fingerprint:
enabled: false
That is not supported but can never happened as if the set useFingerprint false, then the prospector.scanner.fingerprint won't be include in the final config. If we do not include the enabled parameter, then it is error prone. First because users do not know exactly what are the options and second because if the add the enabled for whatever reason there will be an error about key duplication.
I suggest we just do that:
{{#if useFingerprint}}
file_identity.fingerprint: ~
prospector:
scanner:
fingerprint:
{{ fingerprintConfig }}
{{/if}}
There was a problem hiding this comment.
That is not supported but can never happened as if the set useFingerprint false, then the prospector.scanner.fingerprint won't be include in the final config.
why? it can happen
useFingerprint: true, but variable
fingerprintYaml is:
enabled: false
If we do not include the enabled parameter, then it is error prone.
Yes, it makes sense, I understand your point
I suggest we just do that:
{{#if useFingerprint}} file_identity.fingerprint: ~ prospector: scanner: fingerprint: {{ fingerprintConfig }} {{/if}}
👍 yes, I think it looks better, just to prevent unexpected scanners configuration.
To the default value I would add a comment:
enabled: true # This must be set to `true`
offset: 0
length: 1024
or smth like that, so to make it explicitly said. wdyt?
There was a problem hiding this comment.
if enabled is set to false but the file_identity.fingerprint is configured then there is an error. But I will add this comment to be more clear
There was a problem hiding this comment.
Comment added in b04da0a
💚 Build Succeeded
History
|
|
|
Package kubernetes - 1.70.0 containing this change is available at https://epr.elastic.co/package/kubernetes/1.70.0/ |
* Make prospector.scanner.fingerprint configurable --------- Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
* Make prospector.scanner.fingerprint configurable --------- Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
Proposed commit message
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots