enrich container id from process id by BrookHF · Pull Request #15947 · elastic/beats

BrookHF · 2020-01-29T23:52:46Z

Type of change

Enhancement

What does this PR do?

This PR enables add process metadata to enrich container id in Kubernetes environment by cgroup using process id.

Why is it important?

Include important metadata of the container id in the Kubernetes environment, which beats don't have this ability yet.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works

Author's Checklist

[ ]

How to test this PR locally

Deploy Auditbeat in minikube with the include_cid set to true. Deploy an sshd server in minikube. After the Auditbeat is running, ssh into the sshd server, which will trigger an Auditbeat event. See if the event coming from the sshd container is enriched with container id cid

Related issues

Closes #14967

Use cases

Screenshots

Logs

elasticmachine · 2020-01-29T23:52:48Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2020-01-29T23:52:48Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

houndci-bot · 2020-01-29T23:52:49Z

libbeat/processors/add_process_metadata/test_cid_provider.go

don't use an underscore in package name

houndci-bot · 2020-01-29T23:52:50Z

libbeat/processors/add_process_metadata/gosiger_cid_provider.go

don't use an underscore in package name

andrewkroh

Thanks for the contribution. I have a few suggestions for config and testing.

andrewkroh · 2020-02-19T15:59:33Z

libbeat/processors/add_process_metadata/add_process_metadata.go

This needs to be configurable. Let's do it the same way as add_docker_metadata.

beats/libbeat/processors/add_docker_metadata/config.go

Line 37 in 24c1e59

HostFS string `config:"system.hostfs"` // Specifies the mount point of the host’s filesystem for use in monitoring a host from within a container.

andrewkroh · 2020-02-19T16:15:55Z

libbeat/processors/add_process_metadata/add_process_metadata.go

This needs to be configurable too. I'd prefer to have this be search list so that it works out of the box with multiple tools (/kubepods, /docker). I don't know how the other tools name their cgroups, but if we can cover them too this would be great.

libbeat/processors/add_process_metadata/config.go

andrewkroh · 2020-02-20T05:32:04Z

libbeat/processors/add_process_metadata/gosiger_cid_provider.go

I see there is a mock cidProvider implementation for testing the processor. But that leaves this part untested. I recommend mocking out the cgroup.ProcessCgroupPaths function in order to get test coverage of this provider. add_docker_metadata does this with:

beats/libbeat/processors/add_docker_metadata/add_docker_metadata.go

Lines 49 to 51 in bc71069

// processGroupPaths returns the cgroups associated with a process. This enables

// unit testing by allowing us to stub the OS interface.

var processCgroupPaths = cgroup.ProcessCgroupPaths

cla-checker-service · 2020-02-25T21:26:53Z

💚 CLA has been signed

BrookHF · 2020-02-25T21:30:45Z

Updated configuration, test, and ECS field

BrookHF · 2020-02-25T23:14:24Z

Also, I signed the CLA, but it still shows not signed

andrewkroh

Thanks for making those updates. I have just a few minor suggestions on the updates.

andrewkroh · 2020-02-26T03:58:01Z

libbeat/processors/add_process_metadata/add_process_metadata.go

Suggested change

// wantContainerID = false

andrewkroh · 2020-02-26T04:07:31Z

libbeat/processors/add_process_metadata/add_process_metadata.go

I recommend leaving this as nil when container.id is not in the mappings. Then adjust the enrich function accordingly. This will save it from doing the work when it's disabled.

Thanks for the suggestion!
Instead of making the cidProvider nil when it is disabled, I passed a dummy function to the cidProvider which also avoids the use of cgroup.ProcessCgroupPaths to see the cgroup of the process.
The reason for that is I felt that the use of nil is a code smell, as we could not extend nil in the future. Also, the extra steps introduced by the dummy function will probably be optimized away by the compiler.

andrewkroh · 2020-02-26T04:08:56Z

libbeat/processors/add_process_metadata/docs/add_process_metadata.asciidoc

Suggested change

directory of the host `/`. HostPath is the path where /proc reside. For different

directory of the host `/`. This is the path where `/proc` is mounted. For different

andrewkroh · 2020-02-26T04:09:34Z

libbeat/processors/add_process_metadata/docs/add_process_metadata.asciidoc

Suggested change

runtime configuration of Kubernetes or docker, the host_path can be set to

runtime configuration of Kubernetes or Docker the `host_path` can be set to

andrewkroh · 2020-02-26T04:09:45Z

libbeat/processors/add_process_metadata/docs/add_process_metadata.asciidoc

Suggested change

overwrite the default `host_path`

overwrite the default.

andrewkroh · 2020-02-26T04:12:02Z

libbeat/processors/add_process_metadata/docs/add_process_metadata.asciidoc

Suggested change

`/kubepods` and `/docker`. CgroupPrefix is the prefix where the container id is

`/kubepods` and `/docker`. This is the prefix where the container ID is

andrewkroh · 2020-02-26T04:12:28Z

libbeat/processors/add_process_metadata/docs/add_process_metadata.asciidoc

Suggested change

inside cgroup. or different runtime configuration of Kubernetes or docker, the

inside cgroup. For different runtime configuration of Kubernetes or Docker the

andrewkroh · 2020-02-26T04:12:54Z

libbeat/processors/add_process_metadata/docs/add_process_metadata.asciidoc

Suggested change

cgroup_prefixes can be set to overwrite the default `cgroup_prefixes`

`cgroup_prefixes` can be set to overwrite the defaults.

BrookHF · 2020-02-26T22:17:17Z

I recommend leaving this as nil when container.id is not in the mappings. Then adjust the enrich function accordingly. This will save it from doing the work when it's disabled.

Thanks for the suggestion!
Instead of making the cidProvider nil when it is disabled, I passed a dummy function to the cidProvider which also avoids the use of cgroup.ProcessCgroupPaths to see the cgroup of the process.
The reason for that is I felt that the use of nil is a code smell, as we could not extend nil in the future. Also, the extra steps introduced by the dummy function will probably be optimized away by the compiler.

andrewkroh · 2020-02-27T20:07:27Z

libbeat/processors/add_process_metadata/add_process_metadata.go

Suggested change

andrewkroh · 2020-02-27T20:24:20Z

libbeat/processors/add_process_metadata/add_process_metadata.go

Suggested change

dummyProcessCgroupPaths := func(_ string, pid int) (map[string]string, error) {

dummyProcessCgroupPaths := func(_ string, _ int) (map[string]string, error) {

andrewkroh · 2020-02-27T20:25:13Z

libbeat/processors/add_process_metadata/add_process_metadata.go

Suggested change

cidProvider = newCidProvider(config.HostPath, []string{}, dummyProcessCgroupPaths)

cidProvider = newCidProvider(config.HostPath, nil, dummyProcessCgroupPaths)

https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

andrewkroh · 2020-02-27T20:27:38Z

libbeat/processors/add_process_metadata/add_process_metadata.go

Suggested change

// add container.id into meta for mapping to pickup

// enrichContainerID adds container.id into meta for mapping to pickup.

I like to follow godoc style even for unexported types/methods.

andrewkroh · 2020-02-27T20:29:05Z

libbeat/processors/add_process_metadata/config.go

Suggested change

IncludeCid bool `config:"include_cid"`

andrewkroh · 2020-02-27T20:29:30Z

libbeat/processors/add_process_metadata/gosiger_cid_provider.go

Suggested change

andrewkroh · 2020-02-27T20:29:46Z

libbeat/processors/add_process_metadata/gosiger_cid_provider.go

Suggested change

andrewkroh · 2020-02-27T20:37:17Z

libbeat/processors/add_process_metadata/add_process_metadata.go

The reason for that is I felt that the use of nil is a code smell, as we could not extend nil in the future. Also, the extra steps introduced by the dummy function will probably be optimized away by the compiler.

I still think that leaving this cidProvider as nil is simpler when the feature is disabled. It doesn't leave anything to chance with compiler optimizations.

I'm not sure what you meant by "as we could not extend nil in the future".

Just a small style different, I changed it to use nil.

Trying to explain "as we could not extend nil in the future".

Put the cidProvider to nil, in the case of container.id is disabled, Then there has to be a nil check in the enrich function. If there is another case, there has to be another type of check for what cidProvider.

Whereas, by changing the behavior of the cidProvider so that when container.id is disabled it will not add container id. If there is another case, just pass an instance of cidProvider which behaved differently, then there is not any change required in the enrich.

Furthermore, say if there is any change required in the runtime. It would be possible to pass a different instance of cidProvider to the processor determined by some conditions.

It is not specifically required in this PR, but just thought it could leave more flexibility

why not just overwrite/expand existing provider instead of creating another one? Some functionalities might still go for regular provider and e.g. try to read /proc/$some_pid/exe instead $host_path/proc/$some_pid/exe

andrewkroh · 2020-02-27T20:39:16Z

libbeat/processors/add_process_metadata/add_process_metadata.go

I don't think this needs to pass back the meta reference. Just have it return the error.

andrewkroh · 2020-03-02T15:57:05Z

jenkins, test this please

andrewkroh

LGTM. Just two minor nits to cleanup.

libbeat/processors/add_process_metadata/add_process_metadata.go

exekias

Waiting for CI

exekias · 2020-03-24T20:27:17Z

jenkins test this please

exekias · 2020-03-25T09:24:27Z

Thank you so much for your contribution!!

* enrich container id from process id (cherry picked from commit 3cb957d)

* enrich container id from process id (cherry picked from commit 3cb957d) Co-authored-by: Fang He <fangbrookhe@gmail.com>

* enrich container id from process id (cherry picked from commit 3cb957d)

* enrich container id from process id (cherry picked from commit 3cb957d) Co-authored-by: Fang He <fangbrookhe@gmail.com>

* enrich container id from process id (cherry picked from commit 3aaf027) Co-authored-by: Fang He <fangbrookhe@gmail.com>

houndci-bot reviewed Jan 29, 2020

View reviewed changes

jsoriano added Team:Platforms Label for the Integrations - Platforms team review [zube]: Inbox labels Feb 6, 2020

andresrc added [zube]: In Review and removed [zube]: Inbox labels Feb 7, 2020

zube bot added [zube]: Inbox [zube]: In Review and removed [zube]: In Review [zube]: Inbox labels Feb 7, 2020

andrewkroh requested changes Feb 19, 2020

View reviewed changes

andrewkroh reviewed Feb 20, 2020

View reviewed changes

BrookHF requested a review from andrewkroh February 25, 2020 21:28

andrewkroh requested changes Feb 26, 2020

View reviewed changes

BrookHF requested a review from andrewkroh February 26, 2020 22:02

andrewkroh reviewed Feb 27, 2020

View reviewed changes

BrookHF requested a review from andrewkroh February 28, 2020 00:24

exekias mentioned this pull request Feb 28, 2020

Adding pid matcher for add_kubernetes_metadata processor. #16549

Closed

8 tasks

andrewkroh approved these changes Mar 2, 2020

View reviewed changes

libbeat/processors/add_process_metadata/add_process_metadata.go Outdated Show resolved Hide resolved

libbeat/processors/add_process_metadata/add_process_metadata.go Outdated Show resolved Hide resolved

add CHANGELOG, change config

998ae92

BrookHF force-pushed the cid_from_pid branch from d5b32c1 to 998ae92 Compare March 19, 2020 19:22

BrookHF requested a review from exekias March 19, 2020 20:45

Merge branch 'master' into cid_from_pid

5f79ddf

exekias approved these changes Mar 24, 2020

View reviewed changes

exekias merged commit 3cb957d into elastic:master Mar 25, 2020

zube bot added [zube]: Done and removed [zube]: In Review labels Mar 25, 2020

exekias pushed a commit to exekias/beats that referenced this pull request Mar 25, 2020

enrich container id from process id (elastic#15947)

5328640

* enrich container id from process id (cherry picked from commit 3cb957d)

exekias mentioned this pull request Mar 25, 2020

Cherry-pick #15947 to 7.x: enrich container id from process id #17235

Merged

5 tasks

exekias added the v7.7.0 label Mar 25, 2020

exekias pushed a commit that referenced this pull request Mar 25, 2020

enrich container id from process id (#15947) (#17235)

507feea

* enrich container id from process id (cherry picked from commit 3cb957d) Co-authored-by: Fang He <fangbrookhe@gmail.com>

exekias pushed a commit to exekias/beats that referenced this pull request Mar 25, 2020

enrich container id from process id (elastic#15947)

a340258

* enrich container id from process id (cherry picked from commit 3cb957d)

exekias mentioned this pull request Mar 25, 2020

Cherry-pick #15947 to 7.7: enrich container id from process id #17244

Merged

5 tasks

exekias pushed a commit that referenced this pull request Mar 25, 2020

enrich container id from process id (#15947) (#17244)

2206baf

* enrich container id from process id (cherry picked from commit 3cb957d) Co-authored-by: Fang He <fangbrookhe@gmail.com>

exekias added the test-plan Add this PR to be manual test plan label Mar 26, 2020

andresrc added the test-plan-added This PR has been added to the test plan label Mar 27, 2020

This was referenced Mar 31, 2020

add_process_metadata: container id: Do not cache whole cgroup file for each process #17351

Merged

Add regex based cid extractor to add_process_metadata #17360

Merged

This was referenced Mar 31, 2020

Cherry-pick #17351 to 7.7: add_process_metadata: container id: Do not cache whole cgroup file for each process #17361

Merged

Cherry-pick #17351 to 7.x: add_process_metadata: container id: Do not cache whole cgroup file for each process #17362

Merged

andresrc removed the [zube]: Done label Mar 31, 2020

jsoriano mentioned this pull request Apr 2, 2020

Add auditd example with Auditbeat in kubernetes manifests #17431

Merged

exekias mentioned this pull request Apr 3, 2020

Cherry-pick #17360 to 7.x: Add regex based cid extractor to add_process_metadata #17462

Merged

5 tasks

This was referenced Apr 6, 2020

Cherry-pick #17431 to 7.x: Add auditd example with Auditbeat in kubernetes manifests #17545

Merged

Cherry-pick #17431 to 7.7: Add auditd example with Auditbeat in kubernetes manifests #17546

Merged

david-kow mentioned this pull request Jun 24, 2020

Remove auditbeat config bit that is only supported on 7.7+ elastic/cloud-on-k8s#3313

Merged

leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023

enrich container id from process id (elastic#15947) (elastic#17244)

faac586

* enrich container id from process id (cherry picked from commit 3aaf027) Co-authored-by: Fang He <fangbrookhe@gmail.com>

	// processGroupPaths returns the cgroups associated with a process. This enables
	// unit testing by allowing us to stub the OS interface.
	var processCgroupPaths = cgroup.ProcessCgroupPaths

	directory of the host `/`. HostPath is the path where /proc reside. For different
	directory of the host `/`. This is the path where `/proc` is mounted. For different

	runtime configuration of Kubernetes or docker, the host_path can be set to
	runtime configuration of Kubernetes or Docker the `host_path` can be set to

	`/kubepods` and `/docker`. CgroupPrefix is the prefix where the container id is
	`/kubepods` and `/docker`. This is the prefix where the container ID is

	inside cgroup. or different runtime configuration of Kubernetes or docker, the
	inside cgroup. For different runtime configuration of Kubernetes or Docker the

	cgroup_prefixes can be set to overwrite the default `cgroup_prefixes`
	`cgroup_prefixes` can be set to overwrite the defaults.

	dummyProcessCgroupPaths := func(_ string, pid int) (map[string]string, error) {
	dummyProcessCgroupPaths := func(_ string, _ int) (map[string]string, error) {

	cidProvider = newCidProvider(config.HostPath, []string{}, dummyProcessCgroupPaths)
	cidProvider = newCidProvider(config.HostPath, nil, dummyProcessCgroupPaths)

	// add container.id into meta for mapping to pickup
	// enrichContainerID adds container.id into meta for mapping to pickup.

Conversation

BrookHF commented Jan 29, 2020 • edited by andresrc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Uh oh!

elasticmachine commented Jan 29, 2020

Uh oh!

elasticmachine commented Jan 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cla-checker-service bot commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrookHF commented Feb 25, 2020

Uh oh!

BrookHF commented Feb 25, 2020

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrookHF commented Feb 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrookHF commented Jan 29, 2020 •

edited by andresrc

Loading

cla-checker-service bot commented Feb 25, 2020 •

edited

Loading