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 also realized the logic in `reload_nginx` was flawed.
It was only checking whether a worker process was running, rather than verifying
that the old worker (by PID) had exited. Since NGINX spawns a new worker and
sends a SIGQUIT to the old one during reloads, our previous check was resulting
in false positives. 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/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,
@@ -788,17 +803,22 @@ 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)
+                    return _worker_pid not in pids
+
+                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

@@ -1,5 +1,6 @@
 import json
 from pathlib import Path
+import unittest
 
 from .. import case, formats
 
@@ -8,15 +9,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 +31,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;

test/cases/smoke/test_smoke.py

@@ -6,7 +6,7 @@
 class TestSmoke(case.TestCase):
 
     def test_smoke(self):
-        conf_path = Path(__file__).parent / './conf/nginx.conf'
+        conf_path = Path(__file__).parent / "./conf/nginx.conf"
         conf_text = conf_path.read_text()
         status, log_lines = self.orch.nginx_replace_config(
             conf_text, conf_path.name)
@@ -17,7 +17,7 @@ def test_minimal_config(self):
         does not crash on module load, per the bug reported in
         <https://github.com/DataDog/nginx-datadog/pull/17>.
         """
-        conf_path = Path(__file__).parent / './conf/minimal.conf'
+        conf_path = Path(__file__).parent / "./conf/minimal.conf"
         conf_text = conf_path.read_text()
         status, log_lines = self.orch.nginx_replace_config(
             conf_text, conf_path.name)