Adding pid matcher for add_kubernetes_metadata processor. by jtinkus · Pull Request #16549 · elastic/beats

jtinkus · 2020-02-25T10:52:40Z

Enhancement

What does this PR do?

Adding new matcher for add_kubernetes_metadata processor. Matcher takes pid from event and extracts container id from cgroup file based on pid.

Why is it important?

Enables Kubernetes metadata matching if there is no other suitable data in event for current matchers, for example matching AuditBeat events.

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

Caching, time to live and cache cleanup delay
Overall code and style (completely new to go)
Seems I do not know how to add label to PR :)

How to test this PR locally

Run AuditBeat on kubernetes worker node or in container with add_kubernetes_metadata processor enabled, configuring it to use container indexer and pid matcher:
processors:

- add_kubernetes_metadata:
    host: aks-agentpool-41193738-0
    kube_config: /root/.kube/config
    default_indexers.enabled: false
    default_matchers.enabled: false
    matchers:
      - pid:
          # optional override for matcher regex, defaults to:
          # matcher_regex: ^\d+:\w+:\/.+\/.+\/.+\/([0-9a-f]{64})
    indexers:
      - container:

Related issues

Use cases

Screenshots

Logs

elasticmachine · 2020-02-25T10:52:42Z

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-02-25T10:52:42Z

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?

cla-checker-service · 2020-02-25T10:52:43Z

💚 CLA has been signed

…cher

into kube_meta_pid_matcher

jtinkus · 2020-02-28T09:51:51Z

Tests on master seem to fail in general....

exekias · 2020-02-28T10:00:20Z

Thank you for opening @jtinkus. Process info enrichment is a great feature to have!

Just recently we got a different PR that adds the container id to processes metadata: #15947

With that PR in, it should be possible to enrich processes with k8s metadata by just matching that field. How do you feel about that? I'm wondering if we should only introduce that PR to keep a single way of doing this kind of enrichment.

jtinkus · 2020-02-28T11:00:38Z

Hello! Skimmed over the code. Seems similar thing. Configurable regex leaves more flexibility IMHO. And in first glance did not see falling back to ppid when cgroup file for pid not found. But you are more familiar with whole product, so leaving it up to you :) We need k8s metadata matched and actually we are in hurry with that... So please keep me posted! With best regards, -- Jako

…

On Fri, Feb 28, 2020 at 12:00 PM Carlos Pérez-Aradros Herce < ***@***.***> wrote: Thank you for opening @jtinkus <https://github.com/jtinkus>. Process info enrichment is a great feature to have! Just recently we got a different PR that adds the container id to processes metadata: #15947 <#15947> With that PR in, it should be possible to enrich processes with k8s metadata by just matching that field. How do you feel about that? I'm wondering if we should only introduce that PR to keep a single way of doing this kind of enrichment. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16549?email_source=notifications&email_token=AINMFKWA7T5O7U3OQNOIEZDRFDODLA5CNFSM4K3E6VL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENH6WKY#issuecomment-592440107>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AINMFKUEJUPKBWNOKF2SCHLRFDODLANCNFSM4K3E6VLQ> .

-- Jako

jtinkus · 2020-03-10T08:50:43Z

Hello!

Seems that there are problems with other PR you mentioned.

Also seems that we are trying to solve a bit different problems. Enrichment of process metadata vs simple matching of kubernetes metadata. In our case going with this other PR would mean using add_process_metadata processor first and then add_kubernetes_metadata processor. This would mean overhead of collecting other process metadata where only container id is needed.

Also in my testing it seems rather often that process is already ended and container id is obtianed based on ppid. That makes % of getting cid successfully a bit higher too.

@exekias, WDYT, maybe it would be OK to have such a small bit of duplication finally, as those PR-s try to solve different problem in my opinion and getting cid from cgroup is not small and isolated functionality in other PR...

With best regards,

danmx · 2020-03-10T09:38:49Z

libbeat/processors/add_kubernetes_metadata/matchers.go

+
+// For easy stubbing file read in unit tests
+var readCgroupFile = func(pid int) ([]byte, error) {
+	return ioutil.ReadFile(fmt.Sprintf("/proc/%d/cgroup", pid))


A feature from other PR to support custom host fs mounts to access /proc (e.g. /hostfs/proc) would be nice. It'd drop reliance on running in host's PID namespace

OK, will add that too.

danmx · 2020-03-10T10:13:30Z

libbeat/processors/add_kubernetes_metadata/matchers.go

+// NewPidMatcher initializes and returns a PidMatcher
+func NewPidMatcher(cfg common.Config) (Matcher, error) {
+	config := struct {
+		RegexField string `config:"matcher_regex"`


Since k8s is following a principle of /prefix/class/podID/cID wouldn't string manipulation be more performant?
e.g.:

func getCID(cgroup string) (string, error) { if i := strings.LastIndex(cgroup, "/"); i >= 0 { return cgroup[i+1:], nil } return "", fmt.Errorf("Can't extract container ID, cgroup: %s", cgroup) }

Actually I do not know about performance, i'm sure regex is more flexible.
For example this another PR used prefixes from conf instead to choose right line from cgroups returned by gosigar...

would you prefer using gosigar and prfixes like other PR does?

As far as I understand gosigar will get you the content of /proc/$pid/cgroup.

https://github.com/elastic/beats/blob/master/libbeat/processors/add_docker_metadata/add_docker_metadata.go#L259

you use prefixes for detecting if given process is using e.g. /docker or /kubepods

check my PR to add_docker_metadata: #16926

Yes, i understand how it works. gosigar gives map of cgroup strings, but i'm not able to assess performance difference over that current regex solution where i search first regex match over whole cgroup file content...

If you know or feel that iterating over that string map searching matching prefix and then splitting string is faster, i'm happy to change.

Just a thought, this kubernetes meta plugin should be used only with kubernetes, so i guess that docker specific setup is not relevant here...

All together I'd like to end up with good solution :) Sadly i'm completely new with go and also beats, just solving a problem for related project my work depends on. So all of your advice is more than appreciated.

Thank you!!!

only time I'm iterating over whole cgroup map is when there is no container ID otherwise only the first entry is checked. I'll read a little bit more about cgroups and containers because I don't know if there is a possibility that it can run in a container with only subset of cgroups created

https://medium.com/@dgryski/speeding-up-regexp-matching-with-ragel-4727f1c16027

Here is something about regex performance....

danmx · 2020-03-10T10:23:40Z

libbeat/processors/add_kubernetes_metadata/matchers.go

+
+// MetadataIndex returns index for matching metadata indexed based on container id
+func (p *PidMatcher) MetadataIndex(event common.MapStr) string {
+	val, err := event.GetValue("process.pid")


Specifying which field contains process ID would give more freedom to the user to specify if you want to look at ppid or pid

Please explain more. I'm expecting pid and ppid to be in ratrher concrete place in event. Do you mean that instead of trying with ppid when pid not successful you'd like to see configurable option to try with ppid only?

both, with hardcoded process.pid and/or process.ppid people can't inspect e.g. system.process.pid.
For example add_docker_metadata allows you to pick which field to inspect for a process id

OK. Will make them fields configurable.

libbeat/processors/add_kubernetes_metadata/matchers.go

…cher

jtinkus · 2020-03-11T15:22:38Z

libbeat/processors/add_kubernetes_metadata/matchers.go

+// MetadataIndex returns index for matching metadata indexed based on container id
+func (p *PidMatcher) MetadataIndex(event common.MapStr) string {
+	// As we are post processing here, actual process may be already exited.
+	// So trying to find the container id by the parent process id first, hoping parent process lives a bit longer


Here I changed the order. Ppid is used first. Now i have feeling, that falling back to pid just in case is actually useless, meaning that if cgroup file for ppid is already gone, then child process is closed too, right?

So if i'm not mistaken, then using only ppid for extracting container id should be enough.

what if ppid belongs to the container runtime?

right. so in rare cases this pid part is still useful.

jtinkus · 2020-03-31T09:56:36Z

Alternative PR went to master.

Pid matcher for add_kubernetes_metadata.

2c7703e

jtinkus changed the title ~~Pid matcher for add_kubernetes_metadata.~~ [libbeat] Adding pid matcher for add_kubernetes_metadata processor. Feb 25, 2020

jtinkus changed the title ~~[libbeat] Adding pid matcher for add_kubernetes_metadata processor.~~ Adding pid matcher for add_kubernetes_metadata processor. Feb 26, 2020

Jako Tinkus added 2 commits February 26, 2020 09:39

Changelog update.

01a269a

Merge remote-tracking branch 'upstream/master' into kube_meta_pid_mat…

f4cffdb

…cher

ChrsMark added enhancement Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team labels Feb 26, 2020

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

jtinkus and others added 3 commits February 28, 2020 08:41

Merge branch 'master' into kube_meta_pid_matcher

1dbb875

Merge remote-tracking branch 'upstream/master' into kube_meta_pid_mat…

1d07a16

…cher

Merge branch 'kube_meta_pid_matcher' of https://github.com/jtinkus/beats

abb43de

into kube_meta_pid_matcher

danmx reviewed Mar 10, 2020

View reviewed changes

Jako Tinkus added 3 commits March 11, 2020 14:29

Add host_path, pid and ppid as configurable fields.

8bd6210

Merge remote-tracking branch 'upstream/master' into kube_meta_pid_mat…

db7365e

…cher

Move debug info to better place

97c651d

jtinkus commented Mar 11, 2020

View reviewed changes

exekias mentioned this pull request Mar 11, 2020

enrich container id from process id #15947

Merged

5 tasks

jtinkus closed this Mar 31, 2020

zube bot removed the [zube]: In Review label Mar 31, 2020

zube bot added the [zube]: Done label Mar 31, 2020

jtinkus mentioned this pull request Mar 31, 2020

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

Merged

6 tasks

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

Conversation

jtinkus commented Feb 25, 2020 • edited by andresrc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Feb 25, 2020

Uh oh!

elasticmachine commented Feb 25, 2020

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!

jtinkus commented Feb 28, 2020

Uh oh!

exekias commented Feb 28, 2020

Uh oh!

jtinkus commented Feb 28, 2020 via email

Uh oh!

jtinkus commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmx Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtinkus commented Mar 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jtinkus commented Feb 25, 2020 •

edited by andresrc

Loading

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

edited

Loading

jtinkus commented Mar 10, 2020 •

edited

Loading

danmx Mar 11, 2020 •

edited

Loading