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

feat(process-monitor): Detect new containers #213

Merged
merged 6 commits into from
Nov 13, 2023

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented Oct 6, 2023

Enhance the process monitor with an ability to detect when a
container runtime creates a new mount namespace, which we can consider
as a creation of a new container.

Achieve that by:

  • Registering the inodes of container runtime binaries we want to
    track in the user-space, saving them in a BPF map.
  • In BPF, every time a process is being executed using the runtime
    binary, checking whether the PID namespace was changed.

@vadorovsky vadorovsky marked this pull request as ready for review October 30, 2023 10:40
Enhance the process monitor with an ability to detect when a
container runtime creates a new PID namespace, which we can consider
as a creation of a new container.

Achieve that by:

* Registering the inodes of container runtime binaries we want to
  track in the user-space, saving them in a BPF map.
* In BPF, every time a process is being executed using the runtime
  binary, checking whether the PID namespace was changed.
PID namespace can be omitted by passing `--pid=host` to Docker or
podman. Checking mount namespace is more reliable.
* Don't show namespaces and `is_new_container` bool.
* Show container info, but only if a process is containerized.
Comment on lines 212 to 213
#[validatron(skip)]
container: Option<ContainerInfo>,
Copy link
Member

Choose a reason for hiding this comment

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

this can be removed from here as it already present in the header. same thing for the fork event

Copy link
Member Author

@vadorovsky vadorovsky Nov 6, 2023

Choose a reason for hiding this comment

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

If I remove it there, I won't be able to use it here belowi, in the impl Format for Payload block:

https://github.com/vadorovsky/pulsar/blob/8133055d2b99f0bf2637ce828850163c973a6e60/crates/pulsar-core/src/event.rs#L300-L307

There is no access to the header in this place.

Copy link
Member Author

@vadorovsky vadorovsky Nov 6, 2023

Choose a reason for hiding this comment

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

However, I can remove is_new_container, it's not needed in payloads.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I removed all namespace and container-related stuff from payloads, it's only in header now. And container details are getting formatted from header now.

@@ -196,12 +199,18 @@ pub enum Payload {
Fork {
ppid: i32,
namespaces: Namespaces,
Copy link
Member

Choose a reason for hiding this comment

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

probably Namespaces should be included in the header via process_tracker and here should be a skip(validatron). in this way the only reason of having it in the fork event is an optimization to avoid multiple readings from /proc . what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

same thing for exec event

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here

@vadorovsky vadorovsky merged commit edc2124 into exein-io:main Nov 13, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants