[WIP] new(libsinsp): extract pod_uid from cgroups

[Andreagit97]

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area libsinsp

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

This PR allows us to extract the pod_uid directly from the cgroups paths obtained from our drivers (kmod/bpf).

Which issue(s) this PR fixes:

This is necessary to unblock falcosecurity/falco#2973

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Andreagit97]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[FedeDP]

This is needed so that unit-test-libsinsp is able to correctly link re2 since it is now needed.

[Molter73]

Are we sure we want to add a regex match in the hot path for processes? What type of performance hit do we expect from this? Would you mind providing an easy way to disable this check at runtime just in case? Maybe since it looks like heavily related to k8s, could we move it somewhere in the k8s specific code?

[Andreagit97]

Good point! Let's say the alternative would be to loop over some prefixes like in the container engine

libs/userspace/libsinsp/runc.cpp

Line 41 in 3d1f481

bool match_one_container_id(const std::string &cgroup, const std::string &prefix, const std::string &suffix, std::string &container_id)

but not sure this would be more efficient 🤔

I'm ok with disabling it at runtime, but not sure what is the best way to disable it (cmake?some flags provided to sinsp?) Any ideas?

[FedeDP]

Couldn't this be done by a container engine thread as an async event? Just thinking out loud.

[Molter73]

I don't think this needs to be in the process hot path at all, at least thinking from my use case, we have a separate process that gets this information from the kube api, so this are just compute cycles being wasted. Since it's for k8s, I insist it might be better placed somewhere closer to the k8s specific code, but I'm not familiar enough with that code to suggest where to place it.

As for how to enable it at runtime, a simple bool m_pod_uid_enabled flag and a setter alongside the set_pod_uid definition is more than enough IMO, set_pod_uid would just check that flag before doing anything else and return if it's set to false.

[incertum]

👋 happy to help as I have been sifting through the container engine on multiple occasions.

First, yes plz no additional regexes on the hot path.

A few pointers:

I usually use crictl to spin up test pods as that allows for easy testing of both containerd and crio. Example inspection outputs of both the pod / sandbox or the container show that if it's a sandbox the container id is the same as the pod / sandbox id.

INSPECT POD / SANDBOX

sudo crictl pods
sudo crictl inspectp 95120af54b0d1


crio

 "io.kubernetes.cri-o.SandboxID": "95120af54b0d102cc259e906f9aea0423ea7a5dd87ae458567f93cd453b944b1",
 "io.kubernetes.cri-o.ContainerID": "95120af54b0d102cc259e906f9aea0423ea7a5dd87ae458567f93cd453b944b1",

containerd

"status": {
    "id": "c28089bf708270613d27c409a4aaa5679537e8cbe54993ee3749038811cc8d97",

"io.kubernetes.cri.container-type": "sandbox",
"io.kubernetes.cri.sandbox-id": "c28089bf708270613d27c409a4aaa5679537e8cbe54993ee3749038811cc8d97",


INSPECT CONTAINER

sudo crictl ps
sudo crictl inspect 84d2897a399b0

crio 

"io.kubernetes.cri-o.SandboxID": "95120af54b0d102cc259e906f9aea0423ea7a5dd87ae458567f93cd453b944b1",
"io.kubernetes.cri-o.ContainerID": "84d2897a399b0026499e93f490f47304818eb77868b23634f397a50bcedadb77",

String search for m_is_pod_sandbox in the code base and I think that will direct you to the relevant code blocks to make minor adjustments. Basically perhaps it's just about returning the existing container id already extracted from the current cgroups parsing logic if it's a sandbox (without needing the API call against the container runtime socket to know if it's a sandbox or container). And since I haven't looked myself I don't know what all needs to be adjusted to get this, just suspecting it could be easier and less intrusive in the container engine code base (echoing @FedeDP and @Molter73 feedback).

[FedeDP]

I don't think this is much heavier than lots of things already happening in the main thread after all. I see the use case and it makes sense IMO.
I'd only enable this behavior (ie: the regex match) when either the k8s client is enabled or the new plugin is enabled, so that people that don't use it don't pay the (small) extra cost.

[Andreagit97]

I don't think this needs to be in the process hot path at all, at least thinking from my use case, we have a separate process that gets this information from the kube api, so this are just compute cycles being wasted

@Molter73 Just for curiosity, how do you associate the pod_uid extracted from the kube API to a sinsp thread?

[Molter73]

I don't think this needs to be in the process hot path at all, at least thinking from my use case, we have a separate process that gets this information from the kube api, so this are just compute cycles being wasted. Since it's for k8s, I insist it might be better placed somewhere closer to the k8s specific code, but I'm not familiar enough with that code to suggest where to place it.

Hi @Molter73 , we are writing a plugin to handle the kubernetes metadata. But we need to associate a process/thread with those metadata. The best is to leverage the pod UID and extract it from the cgroup path. The cgroup path is already there, just need to extract the pod UID substring from it. We can consider moving the extraction of the pod UID to the plugin code but performances would be impacted too much since the extraction would be done each time we need to associate a syscall to its k8s metadata. Keep in mind that plugin logic would block all the processing on the sinsp side.

Well, that's all fine, but we can still probably move it to the container engine. Even if most of the heavy lifting by container engines is done asynchronously, getting the container ID is done in line by the resolve members, so by making the check there you can limit the search to engines that actually make sense and you could potentially use the already matched container ID in some cases as @incertum suggests. At the very least, if it's going to be a costly operation, an enable flag for adopters to opt out would be appreciated.

I don't think this needs to be in the process hot path at all, at least thinking from my use case, we have a separate process that gets this information from the kube api, so this are just compute cycles being wasted

@Molter73 Just for curiosity, how do you associate the pod_uid extracted from the kube API to a sinsp thread?

Have to admit I'm not 100% sure how this process works, it's a completely separate process from the one I usually work on, but basically we inform data about the process and the container ID it belongs to and the other process does its thing to get information about the resource the container belongs to from the API. I'd have to dig deeper to understand it better, but at that point the data is no longer a sinsp_thread.

[incertum]

@alacuku thanks for adding more context around why you would want this outside the container engine. It was not clear to me before you shared this additional context. I was wondering why we actually cannot use the container engine API calls to get the sandbox id like we currently do as the id itself has not too much value without the pod name anyways. But now I see the use case. Just double checking: The new k8s client definitely and absolutely would only work with the sandbox id and we couldn't pivot from container id to sandbox id like we for example do today with the container runtime API calls?

Yes the tree would need to be formed - it would still be more effective in the container engine portions, but as you explained the ultimate goal would be to bypass the container engine.

Question:

How urgent is this patch now? Are we planning the k8s client refactor for Falco 0.37? What are the performance improvement hopes compared to extracting the most critical information from the container runtime socket today? Are we just planning to replace the mechanisms of getting the k8s fields we support today or are we planning to extend capabilities?

For example, would support for extracting info from for example https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#mutatingadmissionwebhook be possible with the new framework down the road? That would get me definitely "hooked" and in favor.

[incertum]

@Andreagit97 and @alacuku re to what was mentioned above Omar opened a new issue falcosecurity/falco#2895. It would propose adding some similar (not necessarily regex), but string checks on the hot path.

@Andreagit97 you mentioned you wanted to explore more options, curious to hear more. I liked to hear that the refactor could potentially be more performant and more reliable than the current container engine. In that case we could also win in terms of using the k8s client to already gather container image and other fields we currently only fetch from the container runtime socket.

[Molter73]

Why can't this live closer to the set_pod_uid definition in threadinfo.h? A full header file for a single define seems overkill.

[Andreagit97]

We will evaluate other possible solutions

[Andreagit97]

this is now integrated into the new plugin, we can close this