From a0ccfdcd2324428c381518b19602cc7e58c532b7 Mon Sep 17 00:00:00 2001 From: Melissa Kilby Date: Mon, 26 Feb 2024 18:44:29 +0000 Subject: [PATCH] cleanup: revert any changes to parse_container_json_evt + ensure existing state updates in lookup_sync are preserved Signed-off-by: Melissa Kilby --- userspace/libsinsp/container_engine/cri.cpp | 4 ++ userspace/libsinsp/parsers.cpp | 45 +++++++-------------- 2 files changed, 18 insertions(+), 31 deletions(-) diff --git a/userspace/libsinsp/container_engine/cri.cpp b/userspace/libsinsp/container_engine/cri.cpp index 0a80afb6e7d..8da0af93d81 100644 --- a/userspace/libsinsp/container_engine/cri.cpp +++ b/userspace/libsinsp/container_engine/cri.cpp @@ -271,8 +271,12 @@ bool cri::resolve(sinsp_threadinfo *tinfo, bool query_os_for_missing_info) * disabling async lookups. */ result.set_lookup_status(sinsp_container_lookup::state::STARTED); + // note: The cache should not have SUCCESSFUL as lookup status at this point, else `parse_container_json_evt` would wrongly exit early. cache->replace_container(std::make_shared(result)); } + // note: On the other hand `parse_container_json_evt` expects SUCCESSFUL as lookup state for the incoming container event / + // the not yet cached container, this was previously done within `lookup_sync`. + result.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL); } if (done) diff --git a/userspace/libsinsp/parsers.cpp b/userspace/libsinsp/parsers.cpp index bb57f194758..d2d423b367d 100644 --- a/userspace/libsinsp/parsers.cpp +++ b/userspace/libsinsp/parsers.cpp @@ -5078,11 +5078,22 @@ void sinsp_parser::parse_container_json_evt(sinsp_evt *evt) const Json::Value& lookup_state = container["lookup_state"]; if(check_json_val_is_convertible(lookup_state, Json::uintValue, "lookup_state")) { + container_info->set_lookup_status(static_cast(lookup_state.asUInt())); + switch(container_info->get_lookup_status()) + { + case sinsp_container_lookup::state::STARTED: + case sinsp_container_lookup::state::SUCCESSFUL: + case sinsp_container_lookup::state::FAILED: + break; + default: + container_info->set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL); + } + // state == STARTED doesn't make sense in a scap file // as there's no actual lookup that would ever finish if(!evt->get_tinfo_ref() && container_info->get_lookup_status() == sinsp_container_lookup::state::STARTED) { - SINSP_DEBUG("Rewriting lookup_state == STARTED from scap file to FAILED for container %s", + SINSP_DEBUG("Rewriting lookup_state = STARTED from scap file to FAILED for container %s", container_info->m_id.c_str()); container_info->set_lookup_status(sinsp_container_lookup::state::FAILED); } @@ -5221,41 +5232,13 @@ void sinsp_parser::parse_container_json_evt(sinsp_evt *evt) } } - evt->set_tinfo_ref(container_info->get_tinfo(m_inspector)); - evt->set_tinfo(evt->get_tinfo_ref().get()); - container_info->set_lookup_status(static_cast(lookup_state.asUInt())); - switch(container_info->get_lookup_status()) - { - // Preserve case where lookup_state == STARTED from scap file was set to FAILED - // or otherwise set to false. - case sinsp_container_lookup::state::FAILED: - break; - default: - { - // note: The container image is the most crucial field from a security incident response perspective. - // We aim to raise the bar for successful container lookups. Conversely, pod sandboxes do not include - // a container image in the API response. - if (!container_info->m_image.empty() || container_info->is_pod_sandbox()) - { - // Marked as SUCCESSFUL if and only if the container image field present or - // if it's a pod sandbox container - container_info->set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL); - } else - { - container_info->set_lookup_status(sinsp_container_lookup::state::FAILED); - } - } - } - - // note: Important, only set event filtering flag after all final state updates are done if(!container_info->is_successful()) { SINSP_DEBUG("Filtering container event for failed lookup of %s (but calling callbacks anyway)", container_info->m_id.c_str()); evt->set_filtered_out(true); } - - // `add_container` also triggers the callbacks, therefore always continue even if the - // lookup_status was set to FAILED + evt->set_tinfo_ref(container_info->get_tinfo(m_inspector)); + evt->set_tinfo(evt->get_tinfo_ref().get()); m_inspector->m_container_manager.add_container(container_info, evt->get_thread_info(true)); /* SINSP_STR_DEBUG("Container\n-------\nID:" + container_info.m_id +