cleanup(sinsp)!: remove mesos and k8s filterchecks and move some k8s filtechecks under container.

[Andreagit97]

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area libsinsp

/area tests

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

This PR removes the deprecated filerchecks of mesos and k8s. Some k8s filterchecks are extracted from the container enrichment engine so they will survive but under another name. More precisely:

• "k8s.pod.name"
• "k8s.pod.id"
• "k8s.pod.label"
• "k8s.pod.labels"
• "k8s.ns.name"

are now moved under:

• "container.k8s.pod.name"
• "container.k8s.pod.id"
• "container.k8s.pod.label"
• "container.k8s.pod.labels"
• "container.k8s.ns.name"

Since this is a BREAKING CHANGE because the name of these filter checks is changed, I took the opportunity to use the right flags for the new filter checks, more in detail:

• "container.k8s.pod.label" now requires the more intuitive [ ] syntax, so container.k8s.pod.label[foo], if an argument is not provided, we throw an exception
• "container.k8s.pod.labels" is a List so we can use in and intersects operators

Please note: There were 2 additional fields shared with the old k8s enrichment engine:

• "k8s.pod.ip"
• "k8s.pod.cni.json"

These fields are only extracted by the container runtime and we already have them as container.ip and container.cni.json so we can just remove the k8s. ones and keep what we already have!

Which issue(s) this PR fixes:

Special notes for your reviewer:

BREAKING CHANGE: remove deprecated filter checks k8s and mesos

Does this PR introduce a user-facing change?:

cleanup(sinsp)!: remove `mesos` and `k8s` filterchecks and move some `k8s` filterchecks under `container.`

[FedeDP]

Out of curiosity, what are remaining usages of m_metaevt now?

[Andreagit97]

this is a good question, i have no idea 😖

[jasondellaluce]

It's now only in place get_procs_cpu_from_driver. I'd love to investigate turning that into an async event queue and call it a day.

[FedeDP]

/approve

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP

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,FedeDP]

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

[poiana]

LGTM label has been added.

Git tree hash: 595d2e9ee78a64bb253c99fa0cd3a9a1e4b86715

[incertum]

It would be great if we could chat about such major breaking production changes as a team beforehand.

For example, I'm not completely sold on the idea of breaking our naming convention. Totally fine to list them under container fields, but I think keeping the old names could work just as well. Or we keep the old k8s filtercheck category (my personal preference).

These fields are pretty delicate for real-world production, so we need to be more careful compared to some other low-impact refactors. I would suggest to focus on removing mesos and the old k8s client in this PR. We can hold off on these particular changes until we've had a chance to discuss it further. Especially rushing this into the code base a few days before the next libs release appears to not be in the best interest of the project. For example I wasn't even aware of these renaming plans, I thought we just wanted to remove the old components.

I would be very curious to hear what the other maintainers are recommending.
@falcosecurity/libs-maintainers @falcosecurity/falco-maintainers @falcosecurity/rules-maintainers

/hold

[Andreagit97]

I chose this approach for a technical constraint we have today: plugins cannot redefine filter checks already defined in libsinsp. This is exactly the case we have here with the new k8s plugin, we cannot redefine k8s.* fields until they exist in libsinsp. The idea was to create as few breaking changes as possible, users that today have rules with k8s.* fields should only enable the plugin without changing their rules...
Moreover, this split finally allows a clear separation between container* fields and k8s* fields. We spent some months telling users that if they only need simple pod information (e.g. pod.name, pod.id, ...) they can just use the container runtime instead of the k8s enrichment engine. With this solution, users will clearly understand who provides what. If they want extra k8s fields, they need to enable the k8s plugin, otherwise, they can simply use the container engine.
Unfortunately, this solution forces us to create new container fields, and yes this is a breaking change, but I'm not sure we have many ways to avoid at least a breaking change here :/
To be honest I didn't think it was a surprise, we talked about this issue in one of the last maintainer calls, we discussed the possibility of reimplementing some container fields with the new plugin (hoping for better perf), and me or Jason exposed this technical limitation we have. This is the last step of the k8s/mesos cleanup introduced here #1478 (comment), quoting directly the description of the PR:

This removes all the files related to the implementation of the clients, plus all the code related to them in other classes (such as sinsp and filterchecks). This preserves the k8s.* and mesos.* filterchecks classes, which will be cleaned up in a future PR due to some additional challenges specific to them.

BTW I agree that we could have been more explicit, sorry for that... I'm happy to hear ideas/suggestions/concerns of other maintainers

[incertum]

Focusing my response on the end user interface / result. I understand the technical constraints, we can discuss them more after we have decided.

From my perspective (considering today's k8s fields) the k8splugin is not needed for most production use cases as namespace and pod name is sufficient for the most part, so we should keep in mind that perhaps the "no k8splugin" use case may be the more common one. This assessment can change if the k8splugin will be extended in the future with more valuable fields for incident response and workload attribution. In addition, for example having the k8s fields without many of the container fields is not ideal from an incident response perspective either. Therefore, if adopters use the k8splugin they will continue to rely on the container engine.

I agree wrt the confusion around the -k flag. I believe many adopters unnecessarily ran the -k overhead when in fact they wouldn't have needed it.

Some more thoughts:

• Keep old k8s. filter check for the k8s fields we extract from the container runtime socket (also do not remove the k8s.pod.ip and k8s.pod.cni.json) -> the filtercheck strings are a crucial part of our (unfortunately undocumented) end user contract (we talked about this in the past wrt proc.exepath), I think we truly never renamed any of them AFAIK
• Since we introduce the new k8splugin and adopters will need to make conscious decisions to use them we could instead have a new k8splugin filtercheck category, but idk about semi duplicate fields like k8splugin.ns.name ... seems not right either
• Overloading the old names like today is likely the way to go. Today we do not support different sources for k8.ns.name, it's extracted from the container runtime socket only. Therefore, if the end user wants the k8.ns.name extracted from the plugin and not from the container runtime socket, we could have an override config? But I am not sure about that at all. I would prefer to hide the complexity from the end user -- a solution that populates the k8s fields in the most performant and robust manner is what we should do. For example if you run the k8splugin the namespace etc should be populated by "whoever is first" the container engine or plugin. The end user may truly not care about where the k8s fields come from, but will care A LOT about not breaking production + established incident response runbooks and robustness.
• By the way, perhaps we can explicitly state the default origin of the k8s fields in the fields description similar to how we added information about buffer sizes to some of the proc fields
• Lastly I am aware of the complexities involved because of the technical constraints, but we have always found a way, so why should it be different now, we can make it work

[incertum]

/hold until we have reached a consensus and have also consulted with adopters. This is crucial as this change could potentially disrupt entire production deployments and incident response workflows. Is it worth it?

[leogr]

Hey

This week, I've tried to get feedback from as many people as possible and analyze the problem from different angles. I've now wrapped up my conclusion in two main points (see below) with recommended action items. My proposal almost aligns with @incertum points, and since it will avoid significant disruption for our users, I think we can move forward soon.

I would like to unblock this shortly since it and the plugin implementation address a long-standing pain for some users. If we reach a consensus soon, we can include this PR in libs 0.14 (as initially planned) and release it in 1 or 2 weeks from now. This will ensure the delivery of the K8s support enhancement in Falco 0.37 (and also free a lot of maintainer cycles that will be able to work on other fronts).
🙏

Finally, I would like to thank @Andreagit97 and @alacuku for their superb job on this workstream and the detailed deep dive we had in our chats 🚀

---

Don't remove fields

I agree that fields should never be removed or moved. Removing (or renaming) fields can cause significant breaking changes for our adopters. This is particularly critical for Falco output consumers who rely on these field names, especially those using the Falco json_output option. Removing or renaming fields could lead to major disruptions or, worse, silent failures in systems that depend on specific field names for data routing and processing. Such changes might break integrations with tools like fluentd, disrupt analytics in Elasticsearch, and cause alterations in data series in Loki/Prometheus, resulting in significant operational challenges (thanks to @Issif, which helped me understand some use cases).

However, maintaining all fields indefinitely is not sustainable. That's why we introduced the concept of deprecating fields. A deprecated field is:

• Hidden from documentation
• Triggers a warning when Falco starts if used in any loaded rules
• Always returns a nil value

Still, we must be careful to use field deprecation and always consider its impact.

For instance, we were confident that mesos related fields were not widely used since their implementation had been broken for years without complaints. Similarly, in this development cycle, we've deprecated other fields for comparable reasons, which was deemed acceptable (see PR #1503).

Based on these considerations, I propose the following:

• All container.* fields should remain as they are
• All k8s.pod.* and k8s.ns.name fields should also be kept unchanged
• All other k8s fields should be marked as deprecated (this is a reasonable step since these fields are mainly used with the -k/-K option, and there's a strong indication that they are not widely used, partly because many of these extended fields have limited usability)
• All mesos related fields should remain deprecated without being removed

Acceptability of Contextual Aliases

Field aliasing in Falco should be sparingly used, but it can be appropriate when it adds semantic value rather than just being a redundant feature. For instance, container.ip and k8s.pod.ip yielding the same value is justified as they represent the same concept in their respective contexts (a container's IP in a pod matches the pod's IP). However, this should be true only when the event happens in a Kubernetes environment. It does not make sense to get a value from k8s.pod.ip when the event comes from a container that is not running in Kubernetes.

Proposed Actions:

• Maintain the current aliasing of container.ip with k8s.pod.ip and container.cni.json with k8s.pod.cni.json.
• Adjust k8s.pod.ip and k8s.pod.cni.json to return an empty value when not in a Pod context, emphasizing the contextual relevance of these fields. This fix could be part of this or a subsequent PR (I'd prefer not to block this PR just for this point if it is problematic; I opened k8s.* fields must be populated only if event originate from within a Kubernetes environment #1532 to track this).

This may also be the case for those fields defined by the plugin. For instance, k8s.pod.name, sourced from the container runtime, indicates the Pod name as defined locally by the kubelet. In contrast, a hypothetical field named k8sres.pod.name, gathered from the API server via a plugin, may refer to the resource name in the cluster. Both fields will almost always return the same value. Still, they might be valued at different stages of the Pod's lifecycle (and we will able to document this more precisely), or, in extreme cases, they may be differ (think about a malfunction or if someone maliciously tampers the container runtime data locally).

Therefore, I recommend:

• Keeping k8s fields separate from those provided by plugins
• Not implementing a fallback mechanism between k8s and plugin-provided fields ( our experience suggests such mechanisms can be unreliable, and distinct field definitions offer more dependable data)
• For plugin plugin-defined fields, use a prefix that represents the data. For example, k8sres or something similar will represent cluster resources.

[leogr]

👍

[incertum]

+1 SGTM @leogr

[Andreagit97]

Finally, I would like to thank @Andreagit97 and @alacuku for their superb job on this workstream and the detailed deep dive we had in our chats 🚀

Thank you! I have to admit that in our previous discussions, we probably underrated the entity of this breaking change, the next time we need to involve more people in the design from the very beginning, in this way, we can collect early feedback.

Don't remove fields

I agree that fields should never be removed or moved. Removing (or renaming) fields can cause significant breaking changes for our adopters. This is particularly critical for Falco output consumers who rely on these field names, especially those using the Falco json_output option. Removing or renaming fields could lead to major disruptions or, worse, silent failures in systems that depend on specific field names for data routing and processing.

Yeah, this is something we took into consideration when we proposed the initial design and we all seem aware of it, but I understand that now that is time to choose, this change can scare us...I still think that in the long term, the proposed solution could improve the lives of our users but yeah I understand that there are more conservative ways. I just hope to not create further confusion with new fields introduced by the plugin, clarity is a key point IMO.

Based on these considerations, I propose the following:

• All container.* fields should remain as they are
• All k8s.pod.* and k8s.ns.name fields should also be kept unchanged
• All other k8s fields should be marked as deprecated (this is a reasonable step since these fields are mainly used with the -k/-K option, and there's a strong indication that they are not widely used, partly because many of these extended fields have limited usability)
• All mesos related fields should remain deprecated without being removed

As I said before I'm not 100% sure this is the best solution for our users in the long term, but I won't fight for that if you all think this is the right way to go

Acceptability of Contextual Aliases

Field aliasing in Falco should be sparingly used, but it can be appropriate when it adds semantic value rather than just being a redundant feature. For instance, container.ip and k8s.pod.ip yielding the same value is justified as they represent the same concept in their respective contexts (a container's IP in a pod matches the pod's IP). However, this should be true only when the event happens in a Kubernetes environment. It does not make sense to get a value from k8s.pod.ip when the event comes from a container that is not running in Kubernetes.

Proposed Actions:

• Maintain the current aliasing of container.ip with k8s.pod.ip and container.cni.json with k8s.pod.cni.json.

I have to be honest, IMO with container.ip we can satisfy almost all use cases so I see this as a little bit redundant but again it seems like the majority thinks this is the right thing to do, so we can keep it 👍

Therefore, I recommend:

• Keeping k8s fields separate from those provided by plugins
• Not implementing a fallback mechanism between k8s and plugin-provided fields ( our experience suggests such mechanisms can be unreliable, and distinct field definitions offer more dependable data)
• For plugin plugin-defined fields, use a prefix that represents the data. For example, k8sres or something similar will represent cluster resources.

Yeah, as I told you in private, I don't love the k8sres prefix, but I cannot think of something better to explain to our users this is yet another way to extract the same fields, so we can go for it if we are all onboard