Merge upstream pr 688 (falcosecurity#121)

* fix(container_engine): Only return on success or all retries failed

Instead of always returning a result on the first attempt, only return
results on success or when all retries have failed.

This prevents spurious "container" events for incomplete results.

This is especially important when both docker and cri are enabled,
when both must be tried due to the cgroup pattern overlapping, but
only one actually holds the container.

Signed-off-by: Mark Stemm <[email protected]>

* Log a warning when empty container infos are returned

When empty container infos are passed up due to all attempts failing,
log a warining. This will help highlight cases when the communication
with the container runtime isn't working properly.

Signed-off-by: Mark Stemm <[email protected]>

* Add debug log to note when a lookup is async or sync

The "async_xxx" refers to the code that performs the lookup (we used
to have a separate "docker" engine, but it's been removed.

To make it more clear about whether a lookup is synchronous or
asynchronous, add a debug log.

Signed-off-by: Mark Stemm <[email protected]>

* Use bundled valijson for "regular" build

valijson doesn't really have an ubuntu package, so it can't be
preinstalled. Use the bundled valijson instead.

* Add RE2 to container used for builds + tests

This way it will be present when building with
-DUSE_BUNDLED_DEPS=False

Signed-off-by: Mark Stemm <[email protected]>

.github/workflows/ci.yaml

@@ -11,7 +11,7 @@ jobs:
         flavor:  [ regular, bundled-deps, with-chisels, minimal ]
         include:
           - flavor: regular
-            build-args: '-DBUILD_BPF=On -DUSE_BUNDLED_DEPS=False'
+            build-args: '-DBUILD_BPF=On -DUSE_BUNDLED_DEPS=False -DUSE_BUNDLED_VALIJSON=True'
           - flavor: bundled-deps
             build-args: '-DBUILD_BPF=On -DUSE_BUNDLED_DEPS=True'
           - flavor: with-chisels
@@ -50,6 +50,7 @@ jobs:
             protobuf-compiler-grpc \
             libgtest-dev \
             libprotobuf-dev \
+            libre2-dev \
             linux-headers-$(uname -r) \
             && apt-get clean
         env:

userspace/libsinsp/container.cpp

@@ -341,6 +341,20 @@ void sinsp_container_manager::notify_new_container(const sinsp_container_info& c
 		}
 		else
 		{
+			// We don't log any warning when the inspector
+			// is doing its initial scan from /proc + any
+			// container lookups. Those don't have
+			// retries.
+			if(!container_info.is_successful() && m_inspector->m_inited)
+			{
+				// This means that the container
+				// engine made multiple attempts to
+				// look up the info and all attempts
+				// failed. Log that as a warning.
+				g_logger.format(sinsp_logger::SEV_WARNING,
+						"notify_new_container (%s): Saving empty container info after repeated failed lookups",
+						container_info.m_id.c_str());
+			}
 			add_container(std::make_shared<sinsp_container_info>(container_info), tinfo);
 		}
 		return;

userspace/libsinsp/container_engine/container_async_source.tpp

@@ -101,15 +101,22 @@ void container_async_source<key_type>::run_impl()
 
 		lookup_sync(key, res);
 
-		// For security reasons we store the value regardless of the lookup status on the
-		// first attempt, so we can track the container activity even without its metadata.
-		// For subsequent attempts we store it only if successful.
-		if(res.m_lookup.first_attempt() || res.m_lookup.is_successful())
+		if(!res.m_lookup.should_retry())
 		{
+			// Either the fetch was successful or the
+			// maximum number of retries have occurred.
+			if(!res.m_lookup.is_successful())
+			{
+				g_logger.format(sinsp_logger::SEV_DEBUG,
+						"%s_async (%s): Could not look up container info after %u retries",
+						name(),
+						container_id(key).c_str(),
+						res.m_lookup.retry_no());
+			}
+
 			this->store_value(key, res);
 		}
-
-		if(res.m_lookup.should_retry())
+		else
 		{
 			// Make a new attempt
 			res.m_lookup.attempt_increment();

userspace/libsinsp/container_engine/docker/base.cpp

@@ -65,10 +65,16 @@ void docker_base::parse_docker(const docker_lookup_request& request, container_c
 	bool done;
 	if (cache->async_allowed())
 	{
+		g_logger.format(sinsp_logger::SEV_DEBUG,
+				"docker_async (%s): Starting asynchronous lookup",
+				request.container_id.c_str());
 		done = m_docker_info_source->lookup(request, result);
 	}
 	else
 	{
+		g_logger.format(sinsp_logger::SEV_DEBUG,
+				"docker_async (%s): Starting synchronous lookup",
+				request.container_id.c_str());
 		done = m_docker_info_source->lookup_sync(request, result);
 	}
 	if (done)