Libbeat: Do not overwrite agent.*, ecs.version, and host.name#14407
Libbeat: Do not overwrite agent.*, ecs.version, and host.name#14407cwurm merged 3 commits intoelastic:masterfrom
Conversation
8cdbef3 to
5f6390b
Compare
kvch
left a comment
There was a problem hiding this comment.
I have a suggestion to eliminate the cryptic true or false as a parameter to DeepUpdate and keep the API of MapStr a "cleaner". This alternative implementation would also reduce the changes required in the codebase.
// add a new private function which handles updating values
// and makes a decision regarding overwriting values
func (m MapStr) deepUpdateMap(d MapStr, overwrite bool) {
// body of the current DeepUpdate
}
func (m MapStr) DeepUpdate(d MapStr) {
m.deepUpdateMap(d, true)
}
func (m MapStr) DeepUpdateWithoutOverwrite(d MapStr) {
m.deepUpdateMap(d, false)
}|
+1 on not introducing booleans as arguments to select behavior in public APIs. If we want to introduce some indicator in the public API it should be an enum so it is clear what will happen at the call side e.g. |
* Set host.name to computername - set host.name to computer name for windows events and sysmon - Add info about libbeat elastic#14407 dependency Fixes elastic#13706 (cherry picked from commit da6dd9d)
5f6390b to
7ddc18a
Compare
kvch
left a comment
There was a problem hiding this comment.
LGTM. Please update the section in the PR description about DeepUpdate.
3dc70ab to
55da2f0
Compare
|
@kvch Thanks, done. Do you mind formally approving the PR? |
55da2f0 to
525cd44
Compare
Addresses part of #13920 (comment).
Libbeat currently sets a few fields in every event, with no option to turn it off, or to at least not overwrite existing values.
This is a problem when receiving forwarded events (see #13920 for details). The field I'm most concerned about is
host.namewhich is used by the Kibana SIEM app to identify hosts. This PR changes Libbeat to not overwritehost.nameand a few other fields when they are already set (see list of fields below). This is technically a breaking change, though I think in almost all cases the current behavior is not what users would expect, and it is creating problems in the wild (eg #13706, discuss #1, discuss #2) - so I would like to make it in 7.x.A bit more detail on the implementation: Adds a new function
MapStr. DeepUpdateNoOverwritealongside the existingMapStr.DeepUpdateand anoverwriteparameter to theadd_fieldsprocessor (but does not expose it).The affected fields that will no longer be overwritten if they already exist are:
If we do not want to change the behavior for all these fields we could also refactor the code to only not overwrite
host.name- but I think it makes sense to not overwrite any of these fields.