cleanup: revert any changes to parse_container_json_evt + ensure exis…

…ting state updates in lookup_sync are preserved Signed-off-by: Melissa Kilby <[email protected]>
falcosecurity · Feb 26, 2024 · a0ccfdc · a0ccfdc
1 parent 763469f
commit a0ccfdc
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 31 deletions.
diff --git 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<sinsp_container_info>(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
@@ -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<sinsp_container_lookup::state>(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<sinsp_container_lookup::state>(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 +