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

[Auditbeat] Socket metricset #8834

Merged
merged 38 commits into from
Nov 30, 2018

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented Oct 30, 2018

This sockets metricset is mostly based on Metricbeat's socket metricset. It uses the same code in a number of places:

  1. Netlink to get a list of sockets from the kernel (code refactored into a new NetlinkSession)
  2. Enrichment by username (UserCache), direction (ListenerTable), and process (ProcTable).

Otherwise, it pretty much follows the conventions of the existing metricsets in the system module.

There are a few things we might want to do in the future:

  1. I'll be opening a PR after this for collecting user information in the host metricset. With a list of users available over there we might want to access that instead of doing new lookups here.
  2. Similarly, we already have process information in the processes metricset, so we might want to use that instead of relying on ProcTable. We'll still have to scan /proc to find the PID owning a socket though.
  3. Some sockets are not owned by a user process, and so their inode does not appear in /proc (e.g. NFS). At least some can be owned by RPC services, and I have a working prototype for enriching such sockets with information about the RPC service that owns them. It's a bit tricky to get a list of RPC services in a good way so we might not want to do it now. I'll open an issue after this.

metricbeat/module/system/socket/netlink.go Outdated Show resolved Hide resolved
metricbeat/module/system/socket/netlink.go Outdated Show resolved Hide resolved
metricbeat/module/system/socket/netlink.go Outdated Show resolved Hide resolved
@cwurm cwurm added in progress Pull request is currently in progress. and removed review labels Nov 6, 2018
Copy link
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.

I have a few comments about leveraging ECS more, mostly :-)

x-pack/auditbeat/module/system/sockets/_meta/data.json Outdated Show resolved Hide resolved
x-pack/auditbeat/module/system/sockets/_meta/data.json Outdated Show resolved Hide resolved
x-pack/auditbeat/module/system/sockets/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/auditbeat/module/system/sockets/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/auditbeat/module/system/sockets/_meta/data.json Outdated Show resolved Hide resolved
x-pack/auditbeat/module/system/sockets/sockets_test.go Outdated Show resolved Hide resolved
@cwurm cwurm force-pushed the sockets_metricset branch from 490b15d to 4da95d8 Compare November 15, 2018 20:16
@cwurm cwurm changed the title [Auditbeat] Sockets metricset [Auditbeat] Socket metricset Nov 15, 2018
@cwurm cwurm mentioned this pull request Nov 16, 2018
21 tasks
@cwurm cwurm force-pushed the sockets_metricset branch from d1a182b to b117a37 Compare November 21, 2018 15:59
@cwurm
Copy link
Contributor Author

cwurm commented Nov 21, 2018

Should be ready for another review.

High-level changes since the last:

  • To be able to reuse some of the code from Metricbeat's system/socket I moved listeners.go and ptable.go to metricbeat/helper/.
  • Fields should now be ECS compliant (including using network/source/destination)
  • Regular state reporting.

@cwurm cwurm added review and removed in progress Pull request is currently in progress. labels Nov 21, 2018
Copy link
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.

Left one question I wasn't sure about, but otherwise LGTM.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good and I'm glad you were able to reuse some Metricbeat parts.

I'm curious how strongly others feel about the utility of persisting state to disk for this metricset. And I think we need some changes to more closely match ECS.

x-pack/auditbeat/module/system/socket/socket.go Outdated Show resolved Hide resolved
@cwurm
Copy link
Contributor Author

cwurm commented Nov 30, 2018

Changes:

  • removed TCP state
  • changed entirely to top-level ECS fields
  • removed storing state timestamp to disk, and changed default state.period to 1h

@andrewkroh Let me know if this looks good, please.

Copy link
Member

@andrewkroh andrewkroh 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 merged commit 0e529b2 into elastic:feature-auditbeat-host Nov 30, 2018
@cwurm
Copy link
Contributor Author

cwurm commented Nov 30, 2018

Adding network.type to fields.ecs.yml opened pandora's box, with all fields.asciidoc needing an update. Something I think we should rather do in master - so for now I've merged this after backing out the change to fields.ecs.yml and catching the "field not documented" exception in the socket system test. To be removed when the field is in - I'll create a follow-up issue.

@webmat
Copy link
Contributor

webmat commented Dec 12, 2018

@cwurm Yes, when I did a big push on Filebeat modules, the MO was to add the fields as locally as possible (e.g. module's overall _meta/fields.yml, or Filebeat level).

Fully defining all fields in libbeat has some issues.

One of which is that there used to be only a few common fields. So each Beat would list them right alongside their local field definitions in the documentation, and it wasn't an issue.

When introducing ECS' 100+ fields, this means the Beat fields are drowned in common field definitions 😆 It's fine that they're documented explicitly in each Beat, but it should be made clear which are common fields and which are specific fields.

@ruflin is working on improving documentation generation and other wide ranging issues like this.

@ruflin
Copy link
Contributor

ruflin commented Dec 13, 2018

@cwurm If you are targeting 6.x, please don't add all ECS fields and only the ones you need ;-)

@cwurm
Copy link
Contributor Author

cwurm commented Dec 14, 2018

@ruflin Which fields did you have in mind?

This will go into master first, and then be backported to 6.x. I assume we might have to adjust some fields for the backport.

@ruflin
Copy link
Contributor

ruflin commented Dec 14, 2018

@cwurm Can you ping me on the backport? I'm worried that some fields might conflict with fields we have set as alias in 6.x (hopefully not).

cwurm pushed a commit to cwurm/beats that referenced this pull request Dec 16, 2018
Collects information about open sockets (Linux only).

Uses netlink to query for all currently open sockets. Sends information about all sockets on start, and periodically as determined by `state.period`. Otherwise, sends only newly opened or closed sockets. The sockets are enriched with process and user information.
@cwurm
Copy link
Contributor Author

cwurm commented Dec 17, 2018

@ruflin Backport PR is open: #9581

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