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.

Sign up for GitHub

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(libsinsp/container_info): change default / init lookup state to FAILED #1707

Merged
merged 3 commits into from
Apr 16, 2024
Merged

feat(libsinsp/container_info): change default / init lookup state to FAILED #1707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1eb59b1
feat(libsinsp/container_info): change default / init lookup state to …
incertum Feb 24, 2024
b033ecc
update(tests): define explicit SUCESSFUL container lookup status
incertum Mar 11, 2024
a68fce9
fix(container_engine): explicitely set lookup state for all engines
incertum Mar 12, 2024
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
1 change: 1 addition & 0 deletions userspace/libsinsp/container_engine/bpm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ bool bpm::resolve(sinsp_threadinfo *tinfo, bool query_os_for_missing_info)
if(container_cache().should_lookup(container_info.m_id, CT_BPM))
{
container_info.m_name = container_info.m_id;
container_info.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering whether it would be better to set the SUCCESSFUL lookup status inside
container_cache().notify_new_container(): it requires less changes and it "feels" better, ie: since we are notifying the new container, we mark the container as successfully looked up.

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels less error prone too (like: a new engine gets introduced and we forgot to add the set_lookup_state call).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except some container engines (e.g. libvirt_lxc) add the container to the cache and already expect the lookup status to be SUCCESSFUL, so that notify_new_container() doesn't do anything.

As stated more at the beginning of the PR this change is just an initial improvement so that the lookup state is not SUCCESSFUL by default, which makes not a lot of sense and can backfire even more. We have this issue (#1708) we wanted to work on after Falco 0.38.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, i completely overlooked that. Then we are good to go!

container_cache().add_container(std::make_shared<sinsp_container_info>(container_info), tinfo);
container_cache().notify_new_container(container_info, tinfo);
}
Expand Down
1 change: 1 addition & 0 deletions userspace/libsinsp/container_engine/docker/base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ docker_base::resolve_impl(sinsp_threadinfo *tinfo, const docker_lookup_request&
auto container = sinsp_container_info();
container.m_type = request.container_type;
container.m_id = request.container_id;
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CRI and docker mostly handled the lookup state correctly, except here it was necessary to add.

cache->notify_new_container(container, tinfo);
return true;
}
Expand Down
1 change: 1 addition & 0 deletions userspace/libsinsp/container_engine/libvirt_lxc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ bool libvirt_lxc::resolve(sinsp_threadinfo *tinfo, bool query_os_for_missing_inf
if(container_cache().should_lookup(container.m_id, CT_LIBVIRT_LXC))
{
container.m_name = container.m_id;
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
container_cache().add_container(std::make_shared<sinsp_container_info>(container), tinfo);
container_cache().notify_new_container(container, tinfo);
}
Expand Down
1 change: 1 addition & 0 deletions userspace/libsinsp/container_engine/lxc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ bool lxc::resolve(sinsp_threadinfo *tinfo, bool query_os_for_missing_info)
if (container_cache().should_lookup(container.m_id, CT_LXC))
{
container.m_name = container.m_id;
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
container_cache().add_container(std::make_shared<sinsp_container_info>(container), tinfo);
container_cache().notify_new_container(container, tinfo);
}
Expand Down
1 change: 1 addition & 0 deletions userspace/libsinsp/container_engine/mesos.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ bool libsinsp::container_engine::mesos::resolve(sinsp_threadinfo* tinfo, bool qu
if(container_cache().should_lookup(container.m_id, CT_MESOS))
{
container.m_name = container.m_id;
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
container_cache().add_container(std::make_shared<sinsp_container_info>(container), tinfo);
container_cache().notify_new_container(container, tinfo);
}
Expand Down
1 change: 1 addition & 0 deletions userspace/libsinsp/container_engine/rkt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ bool rkt::rkt::resolve(sinsp_threadinfo* tinfo, bool query_os_for_missing_info)

if (have_rkt)
{
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
cache->add_container(std::make_shared<sinsp_container_info>(container), tinfo);
cache->notify_new_container(container, tinfo);
return true;
Expand Down
4 changes: 2 additions & 2 deletions userspace/libsinsp/container_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class sinsp_container_lookup
sinsp_container_lookup(short max_retry = 3, short max_delay_ms = 500):
m_max_retry(max_retry),
m_max_delay_ms(max_delay_ms),
m_state(state::SUCCESSFUL),
m_state(state::FAILED),
m_retry(0)
{
assert(max_retry >= 0);
Expand Down Expand Up @@ -132,7 +132,7 @@ class sinsp_container_lookup
private:
short m_max_retry;
short m_max_delay_ms;
state m_state = state::SUCCESSFUL;
state m_state = state::FAILED;
short m_retry;
};

Expand Down
2 changes: 2 additions & 0 deletions userspace/libsinsp/test/container_engine/container_parser_cri_containerd.ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,7 @@ TEST_F(sinsp_with_test_input, container_parser_cri_containerd)
"blkio=/k8s.io/3ad7b26ded6d8e7b23da7d48fe889434573036c27ae5a74837233de441c3601e",
"memory=/k8s.io/3ad7b26ded6d8e7b23da7d48fe889434573036c27ae5a74837233de441c3601e"};
std::string cgroupsv = test_utils::to_null_delimited(cgroups);
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
std::string container_json = m_inspector.m_container_manager.container_to_json(container);
add_event_advance_ts(increasing_ts(), parent_tid, PPME_SYSCALL_CLONE_20_E, 0);
add_event_advance_ts(increasing_ts(), parent_tid, PPME_SYSCALL_CLONE_20_X, 20, child_tid, "bash", empty_bytebuf, (uint64_t)1, (uint64_t)1, (uint64_t)0, "", (uint64_t)0, (uint64_t)0, (uint64_t)0, (uint32_t)12088, (uint32_t)7208, (uint32_t)0, "bash", scap_const_sized_buffer{cgroupsv.data(), cgroupsv.size()}, (uint32_t)(PPM_CL_CLONE_CHILD_CLEARTID | PPM_CL_CLONE_CHILD_SETTID | PPM_CL_CLONE_NEWPID | PPM_CL_CHILD_IN_PIDNS), (uint32_t)1000, (uint32_t)1000, (uint64_t)parent_tid, (uint64_t)parent_pid);
Expand Down Expand Up @@ -782,6 +783,7 @@ TEST_F(sinsp_with_test_input, container_parser_cri_containerd_sandbox_container)
"blkio=/k8s.io/63060edc2d3aa803ab559f2393776b151f99fc5b05035b21db66b3b62246ad6a",
"memory=/k8s.io/63060edc2d3aa803ab559f2393776b151f99fc5b05035b21db66b3b62246ad6a"};
std::string cgroupsv = test_utils::to_null_delimited(cgroups);
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
std::string container_json = m_inspector.m_container_manager.container_to_json(container);
add_event_advance_ts(increasing_ts(), parent_tid, PPME_SYSCALL_CLONE_20_E, 0);
add_event_advance_ts(increasing_ts(), parent_tid, PPME_SYSCALL_CLONE_20_X, 20, child_tid, "bash", empty_bytebuf, (uint64_t)1, (uint64_t)1, (uint64_t)0, "", (uint64_t)0, (uint64_t)0, (uint64_t)0, (uint32_t)12088, (uint32_t)7208, (uint32_t)0, "bash", scap_const_sized_buffer{cgroupsv.data(), cgroupsv.size()}, (uint32_t)(PPM_CL_CLONE_CHILD_CLEARTID | PPM_CL_CLONE_CHILD_SETTID | PPM_CL_CLONE_NEWPID | PPM_CL_CHILD_IN_PIDNS), (uint32_t)1000, (uint32_t)1000, (uint64_t)parent_tid, (uint64_t)parent_pid);
Expand Down
2 changes: 2 additions & 0 deletions userspace/libsinsp/test/container_engine/container_parser_cri_crio.ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,7 @@ TEST_F(sinsp_with_test_input, container_parser_cri_crio)
"pids=/pod_123.slice/pod_123-456.slice/crio-49ecc282021562c567a8159ef424a06cdd8637efdca5953de9794eafe29adcad.scope",
"misc=/pod_123.slice/pod_123-456.slice/crio-49ecc282021562c567a8159ef424a06cdd8637efdca5953de9794eafe29adcad.scope"};
std::string cgroupsv = test_utils::to_null_delimited(cgroups);
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
std::string container_json = m_inspector.m_container_manager.container_to_json(container);
add_event_advance_ts(increasing_ts(), parent_tid, PPME_SYSCALL_CLONE_20_E, 0);
add_event_advance_ts(increasing_ts(), parent_tid, PPME_SYSCALL_CLONE_20_X, 20, child_tid, "bash", empty_bytebuf, (uint64_t)1, (uint64_t)1, (uint64_t)0, "", (uint64_t)0, (uint64_t)0, (uint64_t)0, (uint32_t)12088, (uint32_t)7208, (uint32_t)0, "bash", scap_const_sized_buffer{cgroupsv.data(), cgroupsv.size()}, (uint32_t)(PPM_CL_CLONE_CHILD_CLEARTID | PPM_CL_CLONE_CHILD_SETTID | PPM_CL_CLONE_NEWPID | PPM_CL_CHILD_IN_PIDNS), (uint32_t)1000, (uint32_t)1000, (uint64_t)parent_tid, (uint64_t)parent_pid);
Expand Down Expand Up @@ -745,6 +746,7 @@ TEST_F(sinsp_with_test_input, container_parser_cri_crio_sandbox_container)
"pids=/pod_123.slice/pod_123-456.slice/crio-1f04600dc6949359da68eee5fe7c4069706a567c07d1ef89fe3bbfdeac7a6dca.scope",
"misc=/pod_123.slice/pod_123-456.slice/crio-1f04600dc6949359da68eee5fe7c4069706a567c07d1ef89fe3bbfdeac7a6dca.scope"};
std::string cgroupsv = test_utils::to_null_delimited(cgroups);
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
std::string container_json = m_inspector.m_container_manager.container_to_json(container);
add_event_advance_ts(increasing_ts(), parent_tid, PPME_SYSCALL_CLONE_20_E, 0);
add_event_advance_ts(increasing_ts(), parent_tid, PPME_SYSCALL_CLONE_20_X, 20, child_tid, "bash", empty_bytebuf, (uint64_t)1, (uint64_t)1, (uint64_t)0, "", (uint64_t)0, (uint64_t)0, (uint64_t)0, (uint32_t)12088, (uint32_t)7208, (uint32_t)0, "bash", scap_const_sized_buffer{cgroupsv.data(), cgroupsv.size()}, (uint32_t)(PPM_CL_CLONE_CHILD_CLEARTID | PPM_CL_CLONE_CHILD_SETTID | PPM_CL_CLONE_NEWPID | PPM_CL_CHILD_IN_PIDNS), (uint32_t)1000, (uint32_t)1000, (uint64_t)parent_tid, (uint64_t)parent_pid);
Expand Down
9 changes: 7 additions & 2 deletions userspace/libsinsp/test/events_plugin.ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,13 @@ TEST_F(sinsp_with_test_input, event_sources)
ASSERT_FALSE(field_has_value(evt, "evt.asynctype"));

// metaevents have the "syscall" event source
evt = add_event_advance_ts(increasing_ts(), 1, PPME_CONTAINER_JSON_E, 1, "{\"value\": 1}");
ASSERT_EQ(evt->get_type(), PPME_CONTAINER_JSON_E);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was it intentional to use the deprecated event type PPME_CONTAINER_JSON_E? Changed it to PPME_CONTAINER_JSON_2_E

Copy link
Member

Choose a reason for hiding this comment

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

cc @FedeDP

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't write those tests, i don't know why they used PPME_CONTAINER_JSON_E. Most probably it was just an oversight.

std::shared_ptr<sinsp_container_info> container = std::make_shared<sinsp_container_info>();
container->m_type = CT_CONTAINERD;
container->m_id = "3ad7b26ded6d";
container->set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
std::string container_json = m_inspector.m_container_manager.container_to_json(*container);
evt = add_event_advance_ts(increasing_ts(), -1, PPME_CONTAINER_JSON_2_E, 1, container_json.c_str());
ASSERT_EQ(evt->get_type(), PPME_CONTAINER_JSON_2_E);
ASSERT_EQ(evt->get_source_idx(), syscall_source_idx);
ASSERT_EQ(std::string(evt->get_source_name()), syscall_source_name);
ASSERT_EQ(get_field_as_string(evt, "evt.source"), syscall_source_name);
Expand Down
Loading