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

add_process_metadata processor adds container id even if process metadata not accessible #19767

Merged
merged 6 commits into from
Aug 4, 2020

Conversation

jtinkus
Copy link
Contributor

@jtinkus jtinkus commented Jul 9, 2020

  • Enhancement

What does this PR do?

Changed add_process_metadata processor to get both process metadata and container id and not error out if only one is available.

Why is it important?

If container is non-privileged, then process metadata for external processes is not fully readable (no access to /proc/pid/exe and /proc/pid/cwd) and code errors out before even trying to get container id. Same time container id is still accessible in /proc/pid/cgroup file. Now process metadata is skipped for such processes, but container id is still added.

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
  • [] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Minor fix, probably no need for doc or changelog.

Author's Checklist

  • [ ]

How to test this PR locally

Run AuditBeat in non-privileged container in k8s cluster.

Configure add_process_metadata processor in yaml file, for example:

- add_process_metadata:
     match_pids: [process.ppid]
     target: process.parent
     cgroup_regex: \/.+\/.+\/.+\/([0-9a-f]{64}).*

This should add process.parent.container.id field to event even if other process metadata for given ppid is not accessible due non-privileged container rights. In such case there is no other process metadata added except container.id.

Related issues

Use cases

Screenshots

Logs

@elasticmachine
Copy link
Collaborator

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?

1 similar comment
@elasticmachine
Copy link
Collaborator

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?

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jul 9, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 9, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [ChrsMark commented: jenkins run the tests please]

  • Start Time: 2020-08-03T12:53:27.196+0000

  • Duration: 66 min 17 sec

Test stats 🧪

Test Results
Failed 0
Passed 14469
Skipped 1373
Total 15842

@jtinkus
Copy link
Contributor Author

jtinkus commented Jul 9, 2020

@exekias proposal for minor fix in the code you have reviewed before.

@exekias exekias added enhancement review Team:Platforms Label for the Integrations - Platforms team labels Jul 9, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jul 9, 2020
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thank you so much for contributing 🎉 ! I left a question, could you also please add a CHANGELOG entry?

@jsoriano This is one of the places where privileged mode was needed, note anymore if this goes in 🙂

@jtinkus jtinkus requested a review from exekias August 3, 2020 12:19
@ChrsMark ChrsMark self-requested a review August 3, 2020 12:46
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

LGTM, extra thanks for the unit tests!

@ChrsMark
Copy link
Member

ChrsMark commented Aug 3, 2020

jenkins run the tests please

@jtinkus
Copy link
Contributor Author

jtinkus commented Aug 4, 2020

Seems that PR is now waiting approval from @exekias

@ChrsMark ChrsMark added test-plan Add this PR to be manual test plan and removed test-plan Add this PR to be manual test plan labels Aug 4, 2020
@ChrsMark
Copy link
Member

ChrsMark commented Aug 4, 2020

@exekias will be out for some days so I'm merging this one to avoid waiting.
Thank you @jtinkus for contributing this! Could you also share your manual testing process on the description of this PR (How to test this PR locally)?

@ChrsMark ChrsMark merged commit 99191e9 into elastic:master Aug 4, 2020
ChrsMark pushed a commit to ChrsMark/beats that referenced this pull request Aug 4, 2020
ChrsMark added a commit that referenced this pull request Aug 4, 2020
…ner id even if process metadata not accessible (#20417)

* add_process_metadata processor adds container id even if process metadata not accessible (#19767)


(cherry picked from commit 99191e9)

* Update CHANGELOG.next.asciidoc

Co-authored-by: jtinkus <35308202+jtinkus@users.noreply.github.com>
v1v added a commit to v1v/beats that referenced this pull request Aug 6, 2020
…ne-2.0

* upstream/master:
  [docs] Promote ingest management to beta (elastic#20295)
  Upgrade elasticsearch client library used in tests (elastic#20405)
  Disable logging when pulling on python integration tests (elastic#20397)
  Remove pillow from testing requirements.txt (elastic#20407)
  [Filebeat][ATP Module]Setting user agent field required by the API (elastic#20440)
  [Ingest Manager] Send datastreams fields (elastic#20402)
  Add event.ingested to all Filebeat modules (elastic#20386)
  [Elastic Agent] Fix agent control socket path to always be less than 107 characters (elastic#20426)
  Improve cgroup_regex docs with examples (elastic#20425)
  Makes `metrics` config option required in app_insights (elastic#20406)
  Ensure install scripts only install if needed (elastic#20349)
  Update container name for the azure filesets (elastic#19899)
  Group same timestamp metrics values in app_insights metricset (elastic#20403)
  add_process_metadata processor adds container id even if process metadata not accessible (elastic#19767)
  Support "cluster" scope in Metricbeat elasticsearch module (elastic#18547)
  [Filebeat][SophosXG Module] Renaming module and fileset (elastic#20396)
  Update Suricata dashboards (elastic#20394)
  [Elastic Agent] Improve version, restart, enroll CLI commands (elastic#20359)
  Prepare home directories for docker images in a different stage (elastic#20356)
v1v added a commit to v1v/beats that referenced this pull request Aug 6, 2020
…allation

* upstream/master: (23 commits)
  [docs] Promote ingest management to beta (elastic#20295)
  Upgrade elasticsearch client library used in tests (elastic#20405)
  Disable logging when pulling on python integration tests (elastic#20397)
  Remove pillow from testing requirements.txt (elastic#20407)
  [Filebeat][ATP Module]Setting user agent field required by the API (elastic#20440)
  [Ingest Manager] Send datastreams fields (elastic#20402)
  Add event.ingested to all Filebeat modules (elastic#20386)
  [Elastic Agent] Fix agent control socket path to always be less than 107 characters (elastic#20426)
  Improve cgroup_regex docs with examples (elastic#20425)
  Makes `metrics` config option required in app_insights (elastic#20406)
  Ensure install scripts only install if needed (elastic#20349)
  Update container name for the azure filesets (elastic#19899)
  Group same timestamp metrics values in app_insights metricset (elastic#20403)
  add_process_metadata processor adds container id even if process metadata not accessible (elastic#19767)
  Support "cluster" scope in Metricbeat elasticsearch module (elastic#18547)
  [Filebeat][SophosXG Module] Renaming module and fileset (elastic#20396)
  Update Suricata dashboards (elastic#20394)
  [Elastic Agent] Improve version, restart, enroll CLI commands (elastic#20359)
  Prepare home directories for docker images in a different stage (elastic#20356)
  New multiline mode in Filebeat: while_pattern (elastic#19662)
  ...
@andresrc andresrc added the test-plan-added This PR has been added to the test plan label Oct 3, 2020
melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020
@zube zube bot removed the [zube]: Done label Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement review Team:Platforms Label for the Integrations - Platforms team test-plan Add this PR to be manual test plan test-plan-added This PR has been added to the test plan v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants