fix(test): rework reload_nginx (#197)

* fix(test): rework `reload_nginx`

This change comes after a deep investigation into why certain jobs were
failing when upgrading to the latest dd-trace-cpp version.

The root cause turned out to be increased contention in the default curl HTTP
client due to telemetry refactoring. This caused NGINX to take significantly
longer to shut down, which in turn led to timeouts during reload_nginx.

While investigating, I've updated the logic to explicitly wait for the old worker
PID to terminate and confirm the new worker is up before proceeding.

Additionally, while fixing this, I encountered issues with some AppSec tests.
These tests were intentionally causing the worker to fail during initialization
(e.g. by using invalid config paths), but this left NGINX in a broken state for
subsequent tests. To resolve this, the tests for
`datadog_appsec_http_blocked_template_json`,
`datadog_appsec_http_blocked_template_html`, and
`datadog_appsec_ruleset_file` now validate the existence of their required files
during the configuration process instead of at worker startup.

Author: dmehala

CMakeLists.txt

@@ -137,6 +137,7 @@ target_compile_definitions(ngx_http_datadog_objs PUBLIC DD_NGINX_FLAVOR="${NGINX
 target_sources(ngx_http_datadog_objs
   PRIVATE
     src/common/variable.cpp
+    src/common/directives.cpp
     src/array_util.cpp
     src/datadog_conf.cpp
     src/datadog_conf_handler.cpp

src/common/directives.cpp

@@ -0,0 +1,20 @@
+#include "common/directives.h"
+
+#include <cassert>
+#include <filesystem>
+
+#include "string_util.h"
+
+namespace datadog::common {
+
+char *check_file_exists(ngx_conf_t *cf, void *post, void *data) {
+  assert(data != nullptr);
+  ngx_str_t *s = (ngx_str_t *)data;
+  if (!std::filesystem::exists(nginx::to_string_view(*s))) {
+    ngx_conf_log_error(NGX_LOG_ERR, cf, 0, "Failed to open file: \"%V\"", s);
+    return static_cast<char *>(NGX_CONF_ERROR);
+  }
+  return NGX_CONF_OK;
+}
+
+}  // namespace datadog::common

src/common/directives.h

@@ -0,0 +1,19 @@
+#pragma once
+
+extern "C" {
+#include <ngx_core.h>
+}
+
+namespace datadog::common {
+
+/// Checks if the file specified in the configuration exists.
+///
+/// This function is typically used as a post-processing callback for NGINX
+/// configuration It verifies that the file specified in a configuration
+/// directive actually exists on the filesystem.
+char *check_file_exists(ngx_conf_t *cf, void *post, void *data);
+
+/// Post handler for checking a filepath exists.
+static ngx_conf_post_t ngx_conf_post_file_exists = {check_file_exists};
+
+}  // namespace datadog::common

src/security/directives.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include "common/directives.h"
 #include "datadog_conf.h"
 #include "datadog_directive.h"
 
@@ -36,7 +37,7 @@ constexpr directive appsec_directives[] = {
         ngx_conf_set_str_slot,
         NGX_HTTP_MAIN_CONF_OFFSET,
         offsetof(datadog_main_conf_t, appsec_ruleset_file),
-        nullptr,
+        &datadog::common::ngx_conf_post_file_exists,
     },
 
     {
@@ -45,7 +46,7 @@ constexpr directive appsec_directives[] = {
         ngx_conf_set_str_slot,
         NGX_HTTP_MAIN_CONF_OFFSET,
         offsetof(datadog_main_conf_t, appsec_http_blocked_template_json),
-        nullptr,
+        &datadog::common::ngx_conf_post_file_exists,
     },
 
     {
@@ -54,7 +55,7 @@ constexpr directive appsec_directives[] = {
         ngx_conf_set_str_slot,
         NGX_HTTP_MAIN_CONF_OFFSET,
         offsetof(datadog_main_conf_t, appsec_http_blocked_template_html),
-        nullptr,
+        &datadog::common::ngx_conf_post_file_exists,
     },
 
     {

test/cases/orchestration.py

@@ -29,6 +29,21 @@ def quit_signal_handler(signum, frame):
     faulthandler.dump_traceback()
 
 
+def wait_until(predicate_func, timeout_seconds):
+    """Wait until `predicate_func` is True."""
+    before = time.monotonic()
+    while True:
+        if predicate_func():
+            break
+
+        now = time.monotonic()
+        if now - before >= timeout_seconds:
+            raise Exception(
+                f"{timeout_seconds} seconds timeout exceeded while waiting for nginx workers to stop.  {now - before} seconds elapsed."
+            )
+        time.sleep(0.5)
+
+
 signal.signal(signal.SIGQUIT, quit_signal_handler)
 
 # Since we override the environment variables of child processes,
@@ -816,17 +831,24 @@ def reload_nginx(self, wait_for_workers_to_terminate=True):
             # The polling interval was chosen based on a system where:
             # - nginx_worker_pids ran in ~0.05 seconds
             # - the workers terminated after ~6 seconds
-            poll_period_seconds = 0.5
-            timeout_seconds = 10
-            before = time.monotonic()
-            while old_worker_pids & nginx_worker_pids(nginx_container,
-                                                      self.verbose):
-                now = time.monotonic()
-                if now - before >= timeout_seconds:
-                    raise Exception(
-                        f"{timeout_seconds} seconds timeout exceeded while waiting for nginx workers to stop.  {now - before} seconds elapsed."
-                    )
-                time.sleep(poll_period_seconds)
+            def old_worker_stops(worker_pid):
+                _worker_pid = worker_pid
+
+                def check():
+                    pids = nginx_worker_pids(nginx_container, self.verbose)
+                    res = _worker_pid.intersection(pids)
+                    # print(f"_worker_pid={_worker_pid}, pids={pids}, cond={res}")
+                    return len(res) == 0
+
+                return check
+
+            wait_until(old_worker_stops(old_worker_pids), timeout_seconds=10)
+
+            def new_worker_starts():
+                pids = nginx_worker_pids(nginx_container, self.verbose)
+                return len(pids) == 1
+
+            wait_until(new_worker_starts, timeout_seconds=10)
 
     def nginx_replace_config(self, nginx_conf_text, file_name):
         """Replace nginx's config and reload nginx.

test/cases/sec_config/test_sec_config.py

@@ -8,15 +8,20 @@ class TestSecConfig(case.TestCase):
     requires_waf = True
 
     def apply_config(self, conf_name):
-        conf_path = Path(__file__).parent / f'./conf/http_{conf_name}.conf'
+        conf_path = Path(__file__).parent / f"./conf/http_{conf_name}.conf"
         conf_text = conf_path.read_text()
         status, log_lines = self.orch.nginx_replace_config(
             conf_text, conf_path.name)
         self.assertEqual(0, status, log_lines)
 
+    def _test_config(self, conf_name):
+        conf_path = Path(__file__).parent / f"./conf/http_{conf_name}.conf"
+        conf_text = conf_path.read_text()
+        return self.orch.nginx_replace_config(conf_text, conf_path.name)
+
     def get_appsec_data(self):
         self.orch.reload_nginx()
-        log_lines = self.orch.sync_service('agent')
+        log_lines = self.orch.sync_service("agent")
         entries = [
             entry for entry in (formats.parse_trace(line)
                                 for line in log_lines) if entry is not None
@@ -25,141 +30,140 @@ def get_appsec_data(self):
         for entry in entries:
             for trace in entry:
                 for span in trace:
-                    if span.get('meta', {}).get('_dd.appsec.json'):
-                        return json.loads(span['meta']['_dd.appsec.json'])
-        self.failureException('No _dd.appsec.json found in traces')
+                    if span.get("meta", {}).get("_dd.appsec.json"):
+                        return json.loads(span["meta"]["_dd.appsec.json"])
+        self.failureException("No _dd.appsec.json found in traces")
 
     def test_custom_templates(self):
-        templ_json_path = Path(__file__).parent / './conf/templ.json'
-        templ_html_path = Path(__file__).parent / './conf/templ.html'
-        self.orch.nginx_replace_file('/tmp/templ.json',
+        templ_json_path = Path(__file__).parent / "./conf/templ.json"
+        templ_html_path = Path(__file__).parent / "./conf/templ.html"
+        self.orch.nginx_replace_file("/tmp/templ.json",
                                      templ_json_path.read_text())
-        self.orch.nginx_replace_file('/tmp/templ.html',
+        self.orch.nginx_replace_file("/tmp/templ.html",
                                      templ_html_path.read_text())
 
-        self.apply_config('custom_blocking_templates')
+        self.apply_config("custom_blocking_templates")
 
         headers = {
-            'User-Agent': 'dd-test-scanner-log-block',
-            'Accept': 'text/html'
+            "User-Agent": "dd-test-scanner-log-block",
+            "Accept": "text/html"
         }
         status, headers, body = self.orch.send_nginx_http_request(
-            '/http', 80, headers)
+            "/http", 80, headers)
         self.assertEqual(status, 403)
         # find content-type header:
         ct = next((v for k, v in headers if k.lower() == "content-type"), None)
-        self.assertEqual(ct, 'text/html;charset=utf-8')
-        self.assertTrue('My custom blocking response' in body)
+        self.assertEqual(ct, "text/html;charset=utf-8")
+        self.assertTrue("My custom blocking response" in body)
 
         headers = {
-            'User-Agent': 'dd-test-scanner-log-block',
-            'Accept': 'text/json'
+            "User-Agent": "dd-test-scanner-log-block",
+            "Accept": "text/json"
         }
         status, headers, body = self.orch.send_nginx_http_request(
-            '/http', 80, headers)
+            "/http", 80, headers)
         self.assertEqual(status, 403)
         ct = next((v for k, v in headers if k.lower() == "content-type"), None)
-        self.assertEqual(ct, 'application/json')
+        self.assertEqual(ct, "application/json")
         self.assertEqual(
             body,
             '{"error": "blocked", "details": "my custom json response"}\n')
 
     def test_appsec_fully_disabled(self):
-        self.apply_config('appsec_fully_disabled')
+        self.apply_config("appsec_fully_disabled")
 
         headers = {
-            'User-Agent': 'dd-test-scanner-log-block',
-            'Accept': 'text/json'
+            "User-Agent": "dd-test-scanner-log-block",
+            "Accept": "text/json"
         }
-        status, _, _ = self.orch.send_nginx_http_request('/', 80, headers)
+        status, _, _ = self.orch.send_nginx_http_request("/", 80, headers)
         self.assertEqual(status, 200)
 
     def test_bad_custom_template(self):
-        self.apply_config('bad_template_file')
-
-        msg = self.orch.wait_for_log_message(
-            'nginx',
-            '.*Initialising security library failed.*',
-            timeout_secs=5)
+        # We can't afford to shutdown workers
+        status, log_lines = self._test_config("bad_template_file")
+        self.assertNotEqual(0, status, log_lines)
         self.assertTrue(
-            'Failed to open file: /file/that/does/not/exist' in msg)
+            any('Failed to open file: "/file/that/does/not/exist"' in line
+                for line in log_lines))
 
     def test_bad_rules_file(self):
-        self.apply_config('bad_rules_file')
-
-        msg = self.orch.wait_for_log_message(
-            'nginx',
-            '.*Initialising security library failed.*',
-            timeout_secs=5)
-        self.assertTrue('Failed to open file: /bad/rules/file' in msg)
+        status, log_lines = self._test_config("bad_rules_file")
+        self.assertNotEqual(0, status, log_lines)
+        self.assertTrue(
+            any('Failed to open file: "/bad/rules/file' in line
+                for line in log_lines))
 
     def test_bad_pool_name(self):
-        conf_path = Path(__file__).parent / 'conf/http_bad_thread_pool.conf'
-        conf_text = conf_path.read_text()
-        status, log_lines = self.orch.nginx_replace_config(
-            conf_text, conf_path.name)
+        status, log_lines = self._test_config("bad_thread_pool")
         self.assertNotEqual(0, status, log_lines)
 
         self.assertTrue(
             any('datadog_waf_thread_pool_name: "bad_thread_pool" not found' in
                 line for line in log_lines))
 
     def test_multiple_pools(self):
-        self.apply_config('multiple_thread_pools')
+        self.apply_config("multiple_thread_pools")
 
-        headers = {'User-Agent': 'dd-test-scanner-log-block'}
+        headers = {"User-Agent": "dd-test-scanner-log-block"}
         status, _, _ = self.orch.send_nginx_http_request(
-            '/http/a', 80, headers)
+            "/http/a", 80, headers)
         self.assertEqual(status, 403)
 
-        headers = {'User-Agent': 'dd-test-scanner-log-block'}
+        headers = {"User-Agent": "dd-test-scanner-log-block"}
         status, _, _ = self.orch.send_nginx_http_request(
-            '/local/', 80, headers)
+            "/local/", 80, headers)
         self.assertEqual(status, 403)
 
-        headers = {'User-Agent': 'dd-test-scanner-log-block'}
+        headers = {"User-Agent": "dd-test-scanner-log-block"}
         status, _, _ = self.orch.send_nginx_http_request(
-            '/unmonitored/index.html', 80, headers)
+            "/unmonitored/index.html", 80, headers)
         self.assertEqual(status, 200)
 
     def test_custom_obfuscation(self):
-        waf_path = Path(__file__).parent / './conf/waf.json'
+        waf_path = Path(__file__).parent / "./conf/waf.json"
         waf_text = waf_path.read_text()
-        self.orch.nginx_replace_file('/tmp/waf.json', waf_text)
+        self.orch.nginx_replace_file("/tmp/waf.json", waf_text)
 
-        self.apply_config('custom_obfuscation')
+        self.apply_config("custom_obfuscation")
 
         # Redaction by key
         # datadog_appsec_obfuscation_key_regex my.special.key;
         status, _, _ = self.orch.send_nginx_http_request(
-            '/http/?my_special_key=matched+value', 80)
+            "/http/?my_special_key=matched+value", 80)
         appsec_data = self.get_appsec_data()
         self.assertEqual(
-            appsec_data['triggers'][0]['rule_matches'][0]['parameters'][0]
-            ['value'], '<Redacted>')
+            appsec_data["triggers"][0]["rule_matches"][0]["parameters"][0]
+            ["value"],
+            "<Redacted>",
+        )
 
         # Redaction by value
         # datadog_appsec_obfuscation_value_regex \Az.*;
         status, _, _ = self.orch.send_nginx_http_request(
-            '/http/?the+key=z_matched+value', 80)
+            "/http/?the+key=z_matched+value", 80)
         appsec_data = self.get_appsec_data()
         self.assertEqual(
-            appsec_data['triggers'][0]['rule_matches'][0]['parameters'][0]
-            ['value'], '<Redacted>')
+            appsec_data["triggers"][0]["rule_matches"][0]["parameters"][0]
+            ["value"],
+            "<Redacted>",
+        )
 
     def test_no_obfuscation(self):
-        waf_path = Path(__file__).parent / './conf/waf.json'
+        waf_path = Path(__file__).parent / "./conf/waf.json"
         waf_text = waf_path.read_text()
-        self.orch.nginx_replace_file('/tmp/waf.json', waf_text)
+        self.orch.nginx_replace_file("/tmp/waf.json", waf_text)
 
-        self.apply_config('no_obfuscation')
+        self.apply_config("no_obfuscation")
 
-        self.orch.sync_service('agent')
+        self.orch.sync_service("agent")
 
         # No redaction by key
         status, _, _ = self.orch.send_nginx_http_request(
-            '/http/?password=matched+value', 80)
+            "/http/?password=matched+value", 80)
         appsec_data = self.get_appsec_data()
         self.assertEqual(
-            appsec_data['triggers'][0]['rule_matches'][0]['parameters'][0]
-            ['value'], 'matched value')
+            appsec_data["triggers"][0]["rule_matches"][0]["parameters"][0]
+            ["value"],
+            "matched value",
+        )

test/cases/smoke/conf/minimal.conf

@@ -3,7 +3,7 @@
 # nginx config, and "index.html".
 load_module /datadog-tests/ngx_http_datadog_module.so;
 
-worker_processes 0;
+worker_processes 1;
 
 events {
     worker_connections  1024;

test/cases/smoke/conf/nginx.conf

@@ -3,7 +3,7 @@
 # nginx config, and "index.html".
 load_module /datadog-tests/ngx_http_datadog_module.so;
 
-worker_processes 0;
+worker_processes 1;
 
 events {
     worker_connections  1024;