-
Notifications
You must be signed in to change notification settings - Fork 169
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
[WIP] new(libsinsp): extract pod_uid
from cgroups
#1377
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
Copyright (C) 2023 The Falco Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
|
||
*/ | ||
|
||
#pragma once | ||
|
||
#define RGX_POD "(pod[a-z0-9]{8}[-_][a-z0-9]{4}[-_][a-z0-9]{4}[-_][a-z0-9]{4}[-_][a-z0-9]{12})" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't this live closer to the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* | ||
Copyright (C) 2023 The Falco Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
|
||
*/ | ||
|
||
/* K8s filterchecks are not defined in the minimal build so we need this ifdef */ | ||
#if !defined(CYGWING_AGENT) && !defined(MINIMAL_BUILD) && !defined(__EMSCRIPTEN__) | ||
#include <test/helpers/threads_helpers.h> | ||
TEST_F(sinsp_with_test_input, K8S_FILTER_pod_id) | ||
{ | ||
add_default_init_thread(); | ||
open_inspector(); | ||
|
||
int64_t p1_tid = 2; | ||
int64_t p1_pid = 2; | ||
int64_t p1_ptid = INIT_TID; | ||
int64_t p1_vtid = 1; | ||
int64_t p1_vpid = 1; | ||
|
||
uint64_t not_relevant_64 = 0; | ||
uint32_t not_relevant_32 = 0; | ||
|
||
// cgroupfs driver format | ||
std::vector<std::string> cgroups1 = { | ||
"cpuset=/kubepods/besteffort/pod05869489-8c7f-45dc-9abd-1b1620787bb1/691e0ffb65010b2b611f3a15b7f76c48466192e673e156f38bd2f8e25acd6bbc", | ||
}; | ||
std::string cgroupsv = test_utils::to_null_delimited(cgroups1); | ||
scap_const_sized_buffer empty_bytebuf = {/*.buf =*/ nullptr, /*.size =*/ 0}; | ||
auto evt = add_event_advance_ts(increasing_ts(), p1_tid, PPME_SYSCALL_CLONE_20_X, 21, 0, "init", empty_bytebuf, p1_tid, p1_pid, p1_ptid, "", not_relevant_64, not_relevant_64, not_relevant_64, not_relevant_32, not_relevant_32, not_relevant_32, "init", scap_const_sized_buffer{cgroupsv.data(), cgroupsv.size()}, 0, not_relevant_32, not_relevant_32, p1_vtid, p1_vpid); | ||
|
||
ASSERT_EQ(get_field_as_string(evt, "k8s.pod.id"), "05869489-8c7f-45dc-9abd-1b1620787bb1"); | ||
} | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -31,6 +31,9 @@ limitations under the License. | |||
#include "tracer_emitter.h" | ||||
#endif | ||||
|
||||
#include <re2/re2.h> | ||||
#include "pod_regex.h" | ||||
|
||||
constexpr static const char* s_thread_table_name = "threads"; | ||||
|
||||
extern sinsp_evttables g_infotables; | ||||
|
@@ -74,6 +77,7 @@ sinsp_threadinfo::sinsp_threadinfo(sinsp* inspector, std::shared_ptr<libsinsp::s | |||
// m_loginuser | ||||
// m_group | ||||
define_static_field(this, m_container_id, "container_id"); | ||||
define_static_field(this, m_pod_uid, "pod_uid"); | ||||
// m_flags | ||||
define_static_field(this, m_fdlimit, "fd_limit"); | ||||
// m_cap_permitted | ||||
|
@@ -823,6 +827,42 @@ sinsp_threadinfo* sinsp_threadinfo::get_parent_thread() | |||
return m_inspector->get_thread_ref(m_ptid, false).get(); | ||||
} | ||||
|
||||
static re2::RE2 pattern(RGX_POD, re2::RE2::POSIX); | ||||
|
||||
void sinsp_threadinfo::set_pod_uid() | ||||
{ | ||||
m_pod_uid = ""; | ||||
|
||||
// Set pod_uid to `""` if: | ||||
// 1. We don't have cgroups for this thread. | ||||
// 2. We are not in a container. | ||||
if(this->cgroups().empty() || !this->is_in_pid_namespace()) | ||||
{ | ||||
return; | ||||
} | ||||
|
||||
// If `this->cgroups()` is not empty we should have at least the first element | ||||
// An example of `this->cgroups()[0].second` layout is: | ||||
// /kubelet.slice/kubelet-kubepods.slice/kubelet-kubepods-pod1b011081_6e8e_4839_b225_4685eae5fd59.slice/cri-containerd-2f92446a3fbfd0b7a73457b45e96c75a25c5e44e7b1bcec165712b906551c261.scope | ||||
if(!re2::RE2::PartialMatch(this->cgroups()[0].second, pattern, &m_pod_uid)) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't this be done by a container engine thread as an async event? Just thinking out loud. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👋 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
String search for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@Molter73 Just for curiosity, how do you associate the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||||
{ | ||||
return; | ||||
} | ||||
|
||||
// Here `m_pod_uid` could have 2 possible layouts: | ||||
// - (driver cgroup) pod05869489-8c7f-45dc-9abd-1b1620787bb1 | ||||
// - (driver systemd) pod05869489_8c7f_45dc_9abd_1b1620787bb1 | ||||
|
||||
// remove the "pod" prefix from `m_pod_uid` | ||||
m_pod_uid.erase(0, 3); | ||||
|
||||
// convert `_` into `-` if we are in `systemd` notation | ||||
std::replace(m_pod_uid.begin(), m_pod_uid.end(), '_', '-'); | ||||
|
||||
// The final `pod_uid` layout is: | ||||
// 05869489-8c7f-45dc-9abd-1b1620787bb1 | ||||
} | ||||
|
||||
sinsp_fdinfo_t* sinsp_threadinfo::add_fd(int64_t fd, sinsp_fdinfo_t *fdinfo) | ||||
{ | ||||
sinsp_fdtable* fd_table_ptr = get_fd_table(); | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed so that unit-test-libsinsp is able to correctly link
re2
since it is now needed.