Skip to content

[Auditbeat] Add message field to system module#9483

Merged
cwurm merged 10 commits intoelastic:feature-auditbeat-hostfrom
cwurm:message_everywhere_squashed
Dec 12, 2018
Merged

[Auditbeat] Add message field to system module#9483
cwurm merged 10 commits intoelastic:feature-auditbeat-hostfrom
cwurm:message_everywhere_squashed

Conversation

@cwurm
Copy link
Copy Markdown
Contributor

@cwurm cwurm commented Dec 11, 2018

This adds a top-level message field to the host, process, socket, and user metricsets.

Some example messages:

  • host: Ubuntu host ubuntu-bionic (IP: 10.0.2.15) is up for 0 days, 5 hours, 11 minutes
  • process: Process zsh (PID: 2363) is RUNNING
  • socket: Inbound socket (10.0.2.2:52002 -> 10.0.2.15:22) OPEN by process sshd (PID: 2293) and user root (UID: 0)
  • user: Existing user elastic (UID: 1002, Groups: elastic,docker)

@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/secops

@cwurm cwurm force-pushed the message_everywhere_squashed branch from f8b2403 to df08708 Compare December 11, 2018 17:59
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These could have been a method on the eventAction type, like Render() or something like that. Just an idea.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree in general. In this case I prefer this since the value is only ever used right after being filled. A method would separate this flow, and probably to different parts of the file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Functions like this should get some unit tests. Fine with adding them later in this case, but normally we'd expect them with the PR.

@tsg
Copy link
Copy Markdown
Contributor

tsg commented Dec 11, 2018

Some unit tests should be added at least for:

  • hostMessage
  • fmtDuration
  • processMessage
  • socketMessage
  • userMessage

Copy link
Copy Markdown
Contributor

@tsg tsg left a comment

Choose a reason for hiding this comment

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

Good from my side.

Copy link
Copy Markdown
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM overall.

Two things I'd like to bring up:

  • Can you use "LISTENING" instead of "OPEN", for socket message? Seems more in line with the system terminology
  • Is it possible to report previous and new host ID, when it changes?

This makes me realize that integration tests will be a challenge. The current state of having many values change around because it's a different run of the VM will make it hard to spot legit differences that are expected or that should be investigated.

Perhaps we could separate the tests in two. Test how the data/events are acquired and do high level checks on them (field names, regexes on values), then have unit tests that start from static states, and ensure they are processed correctly, with very precise diffs like we have right now. This kind of assumes we can split these tests via an intermediate state, however...

But that's for later. Only my two bullet points above need discussion. I'm good if we move forward despite my points, however.

Copy link
Copy Markdown
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

@cwurm cwurm force-pushed the message_everywhere_squashed branch from 31aaa3d to 48d5ac7 Compare December 12, 2018 21:44
@cwurm cwurm merged commit b28a9fb into elastic:feature-auditbeat-host Dec 12, 2018
cwurm pushed a commit to cwurm/beats that referenced this pull request Dec 16, 2018
This adds a top-level `message` field to the `host`, `process`, `socket`, and `user` metricsets.
@cwurm cwurm mentioned this pull request Dec 18, 2018
21 tasks
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.

6 participants