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

Libbeat: Do not overwrite agent.*, ecs.version, and host.name #14407

Merged
merged 3 commits into from
Dec 2, 2019

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented Nov 7, 2019

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.name which is used by the Kibana SIEM app to identify hosts. This PR changes Libbeat to not overwrite host.name and 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. DeepUpdateNoOverwrite alongside the existing MapStr.DeepUpdate and an overwrite parameter to the add_fields processor (but does not expose it).

The affected fields that will no longer be overwritten if they already exist are:

  1. agent.ephemeral_id
  2. agent.hostname
  3. agent.id
  4. agent.type
  5. agent.version
  6. ecs.version
  7. host.name

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.

@cwurm cwurm requested review from urso and andrewkroh November 7, 2019 15:04
@cwurm cwurm requested review from a team as code owners November 8, 2019 00:39
@cwurm cwurm requested review from a team and removed request for a team November 8, 2019 00:39
Copy link
Contributor

@kvch kvch left a comment

Choose a reason for hiding this comment

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

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)
}

@urso
Copy link

urso commented Nov 12, 2019

+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. DeepUpdate(m, common.NoOverwrite). Given DeepUpdate is a quite commonly used API (potentially by community beats) I'd prefer to keep it stable => new function/method. Alternative name to DeepUpdateWithoutOverwrite might be DeepUpdateNoOverwrites.

leehinman added a commit that referenced this pull request Nov 20, 2019
* Set host.name to computername

 - set host.name to computer name for windows events and sysmon
 - Add info about libbeat #14407 dependency

Fixes #13706
leehinman added a commit to leehinman/beats that referenced this pull request Nov 21, 2019
* 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)
leehinman added a commit that referenced this pull request Nov 21, 2019
* Set host.name to computername

 - set host.name to computer name for windows events and sysmon
 - Add info about libbeat #14407 dependency

Fixes #13706

(cherry picked from commit da6dd9d)
libbeat/common/mapstr.go Outdated Show resolved Hide resolved
libbeat/common/mapstr.go Outdated Show resolved Hide resolved
@cwurm
Copy link
Contributor Author

cwurm commented Nov 25, 2019

@kvch @urso Makes sense, I've changed to a new DeepUpdateNoOverwrite function. Let me know if that works.

Copy link
Contributor

@kvch kvch left a comment

Choose a reason for hiding this comment

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

LGTM. Please update the section in the PR description about DeepUpdate.

@cwurm
Copy link
Contributor Author

cwurm commented Nov 26, 2019

@kvch Thanks, done. Do you mind formally approving the PR?

@cwurm cwurm merged commit c115bb1 into elastic:master Dec 2, 2019
@cwurm cwurm deleted the libbeat_do_not_overwrite branch December 2, 2019 13:55
cwurm pushed a commit to cwurm/beats that referenced this pull request Dec 2, 2019
…c#14407)

Do not forcibly overwrite agent.*, ecs.version, and host.name in Libbeat when they are already set.

(cherry picked from commit c115bb1)
@cwurm cwurm added the v7.6.0 label Dec 2, 2019
cwurm pushed a commit that referenced this pull request Dec 5, 2019
#14879)

Do not forcibly overwrite agent.*, ecs.version, and host.name in Libbeat when they are already set.

(cherry picked from commit c115bb1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants