fix(test): rework reload_nginx

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,
@@ -788,17 +803,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;