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

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ endif()
project(falcosecurity-libs)

option(USE_BUNDLED_DEPS "Enable bundled dependencies instead of using the system ones" ON)
option(MINIMAL_BUILD "Produce a minimal build with only the essential features (no kubernetes, no mesos, no marathon and no container metadata)" OFF)
option(MINIMAL_BUILD "Produce a minimal build with only the essential features (no container metadata)" OFF)
Copy link
Member

Choose a reason for hiding this comment

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

👍

option(MUSL_OPTIMIZED_BUILD "Enable if you want a musl optimized build" OFF)
option(USE_BUNDLED_DRIVER "Use the driver/ subdirectory in the build process (only available in Linux)" ON)
option(ENABLE_DRIVERS_TESTS "Enable driver tests (bpf, kernel module, modern bpf)" OFF)
Expand Down
2 changes: 0 additions & 2 deletions userspace/libsinsp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,6 @@ set(SINSP_SOURCES
sinsp_filtercheck_fspath.cpp
sinsp_filtercheck_gen_event.cpp
sinsp_filtercheck_group.cpp
sinsp_filtercheck_k8s.cpp
sinsp_filtercheck_mesos.cpp
sinsp_filtercheck_rawstring.cpp
sinsp_filtercheck_reference.cpp
sinsp_filtercheck_syslog.cpp
Expand Down
2 changes: 0 additions & 2 deletions userspace/libsinsp/filter_check_list.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,6 @@ sinsp_filter_check_list::sinsp_filter_check_list()
add_filter_check(new sinsp_filter_check_syslog());
add_filter_check(new sinsp_filter_check_utils());
add_filter_check(new sinsp_filter_check_fdlist());
add_filter_check(new sinsp_filter_check_k8s());
add_filter_check(new sinsp_filter_check_mesos());
add_filter_check(new sinsp_filter_check_tracer());
add_filter_check(new sinsp_filter_check_evtin());
}
Expand Down
2 changes: 0 additions & 2 deletions userspace/libsinsp/filterchecks.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ limitations under the License.
#include "sinsp_filtercheck_fspath.h"
#include "sinsp_filtercheck_gen_event.h"
#include "sinsp_filtercheck_group.h"
#include "sinsp_filtercheck_k8s.h"
#include "sinsp_filtercheck_mesos.h"
#include "sinsp_filtercheck_rawstring.h"
#include "sinsp_filtercheck_reference.h"
#include "sinsp_filtercheck_syslog.h"
Expand Down
4 changes: 1 addition & 3 deletions userspace/libsinsp/sinsp.h
Original file line number Diff line number Diff line change
Expand Up @@ -1205,7 +1205,7 @@ VISIBILITY_PRIVATE
std::vector<sinsp_protodecoder*> m_decoders_reset_list;

//
// meta event management for other sources like k8s, mesos.
// meta event management for other sources.
//
sinsp_evt* m_metaevt;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a good question, i have no idea 😖

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

meta_event_callback m_meta_event_callback;
Expand Down Expand Up @@ -1360,8 +1360,6 @@ VISIBILITY_PRIVATE
friend class sinsp_worker;
friend class curses_textbox;
friend class sinsp_filter_check_fd;
friend class sinsp_filter_check_k8s;
friend class sinsp_filter_check_mesos;
friend class sinsp_filter_check_evtin;
friend class sinsp_baseliner;
friend class sinsp_memory_dumper;
Expand Down
190 changes: 175 additions & 15 deletions userspace/libsinsp/sinsp_filtercheck_container.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,27 @@ using namespace std;
return (uint8_t*) (x).c_str(); \
} while(0)

// We use a macro to avoid a performance overhead, even using an inline function
// we are not sure the compiler will inline it
#define CHECK_LABEL_PRESENCE(x) \
if(is_host || !container_info) \
{ \
return nullptr; \
} \
else \
{ \
auto label = container_info->m_labels.find(x); \
if(label != container_info->m_labels.end()) \
{ \
m_tstr = label->second; \
RETURN_EXTRACT_STRING(m_tstr); \
} \
else \
{ \
return nullptr; \
} \
}

static const filtercheck_field_info sinsp_filter_check_container_fields[] =
{
{PT_CHARBUF, EPF_NONE, PF_NA, "container.id", "Container ID", "The truncated container id (first 12 characters)."},
Expand All @@ -57,6 +78,11 @@ static const filtercheck_field_info sinsp_filter_check_container_fields[] =
{PT_RELTIME, EPF_NONE, PF_DEC, "container.duration", "Number of nanoseconds since container.start_ts", "Number of nanoseconds since container.start_ts."},
{PT_CHARBUF, EPF_NONE, PF_NA, "container.ip", "Container ip address", "The container's / pod's primary ip address as retrieved from the container engine. Only ipv4 addresses are tracked. Consider container.cni.json (CRI use case) for logging ip addresses for each network interface."},
{PT_CHARBUF, EPF_NONE, PF_NA, "container.cni.json", "Container's / pod's CNI result json", "The container's / pod's CNI result field from the respective pod status info. It contains ip addresses for each network interface exposed as unparsed escaped JSON string. Supported for CRI container engine (containerd, cri-o runtimes), optimized for containerd (some non-critical JSON keys removed). Useful for tracking ips (ipv4 and ipv6, dual-stack support) for each network interface (multi-interface support)."},
{PT_CHARBUF, EPF_NONE, PF_NA, "container.k8s.pod.name", "Pod Name", "When the container belongs to a Kubernetes pod it provides the pod name."},
Copy link
Contributor

@incertum incertum Nov 18, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{PT_CHARBUF, EPF_NONE, PF_NA, "container.k8s.pod.id", "Pod ID", "When the container belongs to a Kubernetes pod it provides the pod id."},
{PT_CHARBUF, EPF_ARG_REQUIRED, PF_NA, "container.k8s.pod.label", "Pod Label", "When the container belongs to a Kubernetes pod it provides a specific pod label. 'container.k8s.pod.label[foo]'."},
{PT_CHARBUF, EPF_IS_LIST, PF_NA, "container.k8s.pod.labels", "Pod Labels", "When the container belongs to a Kubernetes pod it provides the pod comma-separated key/value labels. E.g. '(foo1:bar1,foo2:bar2)'."},
{PT_CHARBUF, EPF_NONE, PF_NA, "container.k8s.ns.name", "Pod Namespace Name", "When the container belongs to a Kubernetes pod it provides the pod namespace name."},
};

sinsp_filter_check_container::sinsp_filter_check_container()
Expand All @@ -73,34 +99,47 @@ sinsp_filter_check* sinsp_filter_check_container::allocate_new()
return (sinsp_filter_check*) new sinsp_filter_check_container();
}

int32_t sinsp_filter_check_container::extract_arg(const string &val, size_t basepos)
int32_t sinsp_filter_check_container::extract_arg(const std::string& val, size_t basepos, container_arg_type type)
{
size_t start = val.find_first_of('[', basepos);
if(start == string::npos)
{
throw sinsp_exception("filter syntax error: " + val);
throw sinsp_exception("the field '" + val + "' requires an argument but '[' is not found");
}

size_t end = val.find_first_of(']', start);
if(end == string::npos)
{
throw sinsp_exception("filter syntax error: " + val);
throw sinsp_exception("the field '" + val + "' requires an argument but ']' is not found");
}

string numstr = val.substr(start + 1, end-start-1);
try
{
m_argid = sinsp_numparser::parsed32(numstr);
}
catch (const sinsp_exception& e)
string str = val.substr(start + 1, end-start-1);

if(type == container_arg_type::TYPE_S32)
{
if(strstr(e.what(), "is not a valid number") == NULL)
try
{
throw;
m_argid = sinsp_numparser::parsed32(str);
}
catch (const sinsp_exception& e)
{
if(strstr(e.what(), "is not a valid number") == NULL)
{
throw;
}

m_argid = -1;
m_argstr = numstr;
m_argid = -1;
m_argstr = str;
}
}
else if(type == container_arg_type::TYPE_STRING)
{
m_argstr = str;
}
else
{
ASSERT(false);
throw sinsp_exception("argument type unknown during 'sinsp_filter_check_container::extract_arg'");
}

return end+1;
Expand Down Expand Up @@ -149,15 +188,25 @@ int32_t sinsp_filter_check_container::parse_field_name(const char* str, bool all
}
m_field = &m_info.m_fields[m_field_id];

res = extract_arg(val, basepos);
res = extract_arg(val, basepos, container_arg_type::TYPE_S32);
}
else if (val.find("container.mount") == 0 &&
val[basepos-1] != 's')
{
m_field_id = TYPE_CONTAINER_MOUNT;
m_field = &m_info.m_fields[m_field_id];

res = extract_arg(val, basepos-1);
res = extract_arg(val, basepos-1, container_arg_type::TYPE_S32);
}
// We need to check that is `pod.label` and not `pod.labels`
else if(val.find("container.k8s.pod.label") == 0 &&
val[sizeof("container.k8s.pod.label")-1] != 's')
{
m_field_id = TYPE_CONTAINER_K8S_POD_LABEL;
m_field = &m_info.m_fields[m_field_id];

// We don't need to check the return value, if no arg was specified we throw an exception
res = extract_arg(val, sizeof("container.k8s.pod.label")-1, container_arg_type::TYPE_STRING);
}
else
{
Expand Down Expand Up @@ -574,10 +623,121 @@ uint8_t* sinsp_filter_check_container::extract(sinsp_evt *evt, OUT uint32_t* len
RETURN_EXTRACT_STRING(container_info->m_pod_cniresult);
}
break;
case TYPE_CONTAINER_K8S_POD_NAME:
CHECK_LABEL_PRESENCE("io.kubernetes.pod.name");
break;
case TYPE_CONTAINER_K8S_POD_ID:
CHECK_LABEL_PRESENCE("io.kubernetes.pod.uid");
break;
case TYPE_CONTAINER_K8S_POD_LABEL:
if(is_host || !container_info)
{
return nullptr;
}
else
{
auto label = container_info->m_labels.find("io.kubernetes.sandbox.id");
if(label == container_info->m_labels.end())
{
return nullptr;
}

std::string sandbox_container_id = label->second;
if(sandbox_container_id.size() > 12)
{
sandbox_container_id.resize(12);
}
const auto sandbox_container_info = m_inspector->m_container_manager.get_container(sandbox_container_id);
if(!sandbox_container_info || sandbox_container_info->m_labels.empty())
{
return nullptr;
}

label = sandbox_container_info->m_labels.find(m_argstr);
if(label == sandbox_container_info->m_labels.end())
{
return nullptr;
}
m_tstr = label->second;
RETURN_EXTRACT_STRING(m_tstr);
}
break;
case TYPE_CONTAINER_K8S_NS_NAME:
CHECK_LABEL_PRESENCE("io.kubernetes.pod.namespace");
break;
default:
ASSERT(false);
break;
}

return NULL;
}

bool sinsp_filter_check_container::extract(sinsp_evt* evt, OUT std::vector<extract_value_t>& values,
bool sanitize_strings)
{
values.clear();

// `TYPE_CONTAINER_K8S_POD_LABELS` has the `EPF_IS_LIST` flag.
if(m_field_id == TYPE_CONTAINER_K8S_POD_LABELS)
{
sinsp_threadinfo* tinfo = evt->get_thread_info();
// Without the container id we can do nothing
if(tinfo == nullptr || tinfo->m_container_id.empty())
{
return false;
}

auto container_info = m_inspector->m_container_manager.get_container(tinfo->m_container_id);

if(container_info == nullptr)
{
return false;
}

// We need the label `io.kubernetes.sandbox.id` to extract pod labels.
auto sandbox_label = container_info->m_labels.find("io.kubernetes.sandbox.id");
if(sandbox_label == container_info->m_labels.end())
{
return false;
}

std::string sandbox_container_id = sandbox_label->second;
if(sandbox_container_id.size() > 12)
{
sandbox_container_id.resize(12);
}
const auto sandbox_container_info =
m_inspector->m_container_manager.get_container(sandbox_container_id);

if(sandbox_container_info == nullptr || sandbox_container_info->m_labels.empty())
{
return false;
}

// Clean a possible not empty storage
m_labels_storage.clear();
for(const auto& label : sandbox_container_info->m_labels)
{
// Exclude annotations and internal labels
if(label.first.find("annotation.") == 0 || label.first.find("io.kubernetes.") == 0)
{
continue;
}

// We use a storage because these strings should survive until the next filtercheck call.
m_labels_storage.emplace_back(label.first + ":" + label.second);
}

for(const auto& label : m_labels_storage)
{
extract_value_t val;
val.ptr = (uint8_t*)label.c_str();
val.len = label.size();
values.emplace_back(val);
}
return true;
}

return sinsp_filter_check::extract(evt, values, sanitize_strings);
}
17 changes: 16 additions & 1 deletion userspace/libsinsp/sinsp_filtercheck_container.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,32 @@ class sinsp_filter_check_container : public sinsp_filter_check
TYPE_CONTAINER_DURATION,
TYPE_CONTAINER_IP_ADDR,
TYPE_CONTAINER_CNIRESULT,
TYPE_CONTAINER_K8S_POD_NAME,
TYPE_CONTAINER_K8S_POD_ID,
TYPE_CONTAINER_K8S_POD_LABEL,
TYPE_CONTAINER_K8S_POD_LABELS,
TYPE_CONTAINER_K8S_NS_NAME,
};

enum container_arg_type {
TYPE_S32,
TYPE_STRING
};

sinsp_filter_check_container();
sinsp_filter_check* allocate_new();
uint8_t* extract(sinsp_evt *evt, OUT uint32_t* len, bool sanitize_strings = true);
// This is needed to extract arguments with `EPF_IS_LIST` flags
bool extract(sinsp_evt *evt, OUT std::vector<extract_value_t>& values, bool sanitize_strings = true);

const std::string &get_argstr();
private:
int32_t parse_field_name(const char* str, bool alloc_state, bool needed_for_filtering);
int32_t extract_arg(const std::string& val, size_t basename);
int32_t extract_arg(const std::string& val, size_t basepos, container_arg_type type);

// This vector is used to save the concatenated labels (`key:value`)
// between invocations of `TYPE_CONTAINER_K8S_POD_LABELS` filtercheck.
std::vector<std::string> m_labels_storage;
std::string m_tstr;
uint32_t m_u32val;
int32_t m_argid;
Expand Down
Loading