From 125c710835ad7d099a8b3fbaab38939ebcc59c95 Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Fri, 29 Aug 2025 10:28:01 -0700 Subject: [PATCH 1/5] Add regression tests for auth subrequest behavior from v1.6.2 --- test/cases/auth_requests/README.md | 3 + test/cases/auth_requests/__init__.py | 0 test/cases/auth_requests/conf/http.conf | 38 +++++ .../cases/auth_requests/test_auth_requests.py | 138 ++++++++++++++++++ 4 files changed, 179 insertions(+) create mode 100644 test/cases/auth_requests/README.md create mode 100644 test/cases/auth_requests/__init__.py create mode 100644 test/cases/auth_requests/conf/http.conf create mode 100644 test/cases/auth_requests/test_auth_requests.py diff --git a/test/cases/auth_requests/README.md b/test/cases/auth_requests/README.md new file mode 100644 index 00000000..727fd61b --- /dev/null +++ b/test/cases/auth_requests/README.md @@ -0,0 +1,3 @@ +NGINX can authenticate requests based on a subrequest to an external server, which is done by configuring 'auth_request ' for the protected locations. These tests ensure that our tracing corrrectly handles this configuration. + +IMPORTANT: The NGINX configuration under test sets 'log_subrequest on;' to ensure that we create a span for the auth subrequest, which introduces a child span inside the root nginx span. \ No newline at end of file diff --git a/test/cases/auth_requests/__init__.py b/test/cases/auth_requests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/test/cases/auth_requests/conf/http.conf b/test/cases/auth_requests/conf/http.conf new file mode 100644 index 00000000..807e0db4 --- /dev/null +++ b/test/cases/auth_requests/conf/http.conf @@ -0,0 +1,38 @@ +# "/datadog-tests" is a directory created by the docker build +# of the nginx test image. It contains the module, the +# nginx config, and "index.html". +load_module /datadog-tests/ngx_http_datadog_module.so; + +events { + worker_connections 1024; +} + +http { + datadog_agent_url http://agent:8126; + + log_subrequest on; + + server { + listen 80; + + location /http-no-auth { + proxy_pass http://http:8080; + } + + location /http { + set $auth_token $http_x_token; + auth_request /auth; + + proxy_pass http://http:8080; + } + + location =/auth { + internal; + proxy_pass http://http:8080/auth; + proxy_pass_request_body off; + proxy_pass_request_headers off; + proxy_set_header Content-Length ""; + proxy_set_header Authorization $auth_token; + } + } +} diff --git a/test/cases/auth_requests/test_auth_requests.py b/test/cases/auth_requests/test_auth_requests.py new file mode 100644 index 00000000..a973d058 --- /dev/null +++ b/test/cases/auth_requests/test_auth_requests.py @@ -0,0 +1,138 @@ +from .. import case +from .. import formats + +import json +from pathlib import Path + + +class TestAuthRequests(case.TestCase): + + def test_no_auth_request_is_successful(self): + conf_path = Path(__file__).parent / "./conf/http.conf" + conf_text = conf_path.read_text() + status, log_lines = self.orch.nginx_replace_config( + conf_text, conf_path.name) + self.assertEqual(status, 0, log_lines) + + self.orch.sync_service('agent') + + path = '/http-no-auth' + status, _, body = self.orch.send_nginx_http_request(path) + self.assertEqual(status, 200) + response = json.loads(body) + self.assertEqual(response["service"], "http") + + headers = response["headers"] + trace_id = headers["x-datadog-trace-id"] + span_id = headers["x-datadog-parent-id"] + + self.orch.reload_nginx() + log_lines = self.orch.sync_service('agent') + + # Expect that nginx sent a trace containing two spans: one for the + # request, and another for the auth subrequest. + nginx_sent_a_trace = False + for line in log_lines: + trace = formats.parse_trace(line) + if trace is None: + # not a trace + continue + for chunk in trace: + first, *rest = chunk + if first['service'] == 'nginx': + nginx_sent_a_trace = True + self.assertEqual(0, len(rest), chunk) + self.assertEqual(first['meta']['http.url'], f'http://nginx{path}', first) + self.assertEqual(first['meta']['nginx.location'], path, first) + + self.assertEqual(str(first['trace_id']), str(trace_id), trace) + self.assertEqual(str(first['span_id']), str(span_id), trace) + + self.assertTrue(nginx_sent_a_trace, log_lines) + + def test_auth_request_with_auth_token_is_successful(self): + conf_path = Path(__file__).parent / "./conf/http.conf" + conf_text = conf_path.read_text() + status, log_lines = self.orch.nginx_replace_config( + conf_text, conf_path.name) + self.assertEqual(status, 0, log_lines) + + self.orch.sync_service('agent') + + status, _, body = self.orch.send_nginx_http_request( + '/http', 80, headers={'x-token': 'mysecret'}) + self.assertEqual(status, 200) + response = json.loads(body) + self.assertEqual(response["service"], "http") + + headers = response["headers"] + trace_id = headers["x-datadog-trace-id"] + span_id = headers["x-datadog-parent-id"] + + self.orch.reload_nginx() + log_lines = self.orch.sync_service('agent') + + # Expect that nginx sent a trace containing two spans: one for the + # request, and another for the auth subrequest. + nginx_sent_a_trace = False + for line in log_lines: + trace = formats.parse_trace(line) + if trace is None: + # not a trace + continue + for chunk in trace: + first, *rest = chunk + if first['service'] == 'nginx': + nginx_sent_a_trace = True + self.assertEqual(1, len(rest), chunk) + self.assertEqual(first['meta']['http.url'], 'http://nginx/http', first) + self.assertEqual(first['meta']['nginx.location'], '/http', first) + + # Assert that the subrequest was traced + self.assertEqual('nginx', rest[0]['service'], rest[0]) + self.assertEqual(rest[0]['meta']['http.url'], 'http://nginx/http', rest[0]) + self.assertEqual(rest[0]['meta']['nginx.location'], '/auth', rest[0]) + + # Assert existing behavior that the trace ID and span ID + # match the trace ID and span ID of the nginx subrequest. + self.assertEqual(str(rest[0]['trace_id']), str(trace_id), trace) + self.assertEqual(str(rest[0]['span_id']), str(span_id), trace) + + self.assertTrue(nginx_sent_a_trace, log_lines) + + def test_auth_request_without_auth_token_is_not_successful(self): + conf_path = Path(__file__).parent / "./conf/http.conf" + conf_text = conf_path.read_text() + status, log_lines = self.orch.nginx_replace_config( + conf_text, conf_path.name) + self.assertEqual(status, 0, log_lines) + + self.orch.sync_service('agent') + + status, _, _ = self.orch.send_nginx_http_request('/http') + self.assertEqual(status, 401) + + self.orch.reload_nginx() + log_lines = self.orch.sync_service('agent') + + # Expect that nginx sent a trace containing two spans: one for the + # request, and another for the auth subrequest. + nginx_sent_a_trace = False + for line in log_lines: + trace = formats.parse_trace(line) + if trace is None: + # not a trace + continue + for chunk in trace: + first, *rest = chunk + if first['service'] == 'nginx': + nginx_sent_a_trace = True + self.assertEqual(1, len(rest), chunk) + self.assertEqual(first['meta']['http.url'], 'http://nginx/http', first) + self.assertEqual(first['meta']['nginx.location'], '/http', first) + + self.assertEqual('nginx', rest[0]['service'], rest[0]) + self.assertEqual(rest[0]['meta']['http.url'], 'http://nginx/http', rest[0]) + self.assertEqual(rest[0]['meta']['nginx.location'], '/auth', rest[0]) + + self.assertTrue(nginx_sent_a_trace, log_lines) From 608f5f0c6550b742aa9579582ec0bc5b251e56ff Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Fri, 29 Aug 2025 10:39:06 -0700 Subject: [PATCH 2/5] Provide bug fix for auth subrequests that are being traced: When getting the span information to inject into the request (and for the WAF security context), we end up with 2 spans in this scenario, so do not assert that there's only one span and grab the last span to use for the context. This restores the the distributed tracing behavior from v1.6.2 --- src/datadog_context.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/datadog_context.cpp b/src/datadog_context.cpp index e8c39386..e7f1a6c1 100644 --- a/src/datadog_context.cpp +++ b/src/datadog_context.cpp @@ -343,7 +343,7 @@ ngx_int_t DatadogContext::on_precontent_phase(ngx_http_request_t *request) { // inject headers in the precontent phase into the request headers // These headers will be copied by ngx_http_proxy_create_request on the // content phase into the outgoing request headers (probably) - RequestTracing &trace = single_trace(); + RequestTracing &trace = traces_.back(); dd::Span &span = trace.active_span(); span.set_tag("span.kind", "client"); From 24039f943f4c2dcd6488bf47e6297d53058d54fa Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Fri, 29 Aug 2025 10:45:40 -0700 Subject: [PATCH 3/5] Breaking change: When subrequests are logged (and we generate traces for them), change the behavior to inject the original root span's context into requests and security context, instead of the subrequest span. --- src/datadog_context.cpp | 2 +- test/cases/auth_requests/test_auth_requests.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/datadog_context.cpp b/src/datadog_context.cpp index e7f1a6c1..13cc4b42 100644 --- a/src/datadog_context.cpp +++ b/src/datadog_context.cpp @@ -343,7 +343,7 @@ ngx_int_t DatadogContext::on_precontent_phase(ngx_http_request_t *request) { // inject headers in the precontent phase into the request headers // These headers will be copied by ngx_http_proxy_create_request on the // content phase into the outgoing request headers (probably) - RequestTracing &trace = traces_.back(); + RequestTracing &trace = traces_.front(); dd::Span &span = trace.active_span(); span.set_tag("span.kind", "client"); diff --git a/test/cases/auth_requests/test_auth_requests.py b/test/cases/auth_requests/test_auth_requests.py index a973d058..80d69611 100644 --- a/test/cases/auth_requests/test_auth_requests.py +++ b/test/cases/auth_requests/test_auth_requests.py @@ -95,8 +95,8 @@ def test_auth_request_with_auth_token_is_successful(self): # Assert existing behavior that the trace ID and span ID # match the trace ID and span ID of the nginx subrequest. - self.assertEqual(str(rest[0]['trace_id']), str(trace_id), trace) - self.assertEqual(str(rest[0]['span_id']), str(span_id), trace) + self.assertEqual(str(first['trace_id']), str(trace_id), trace) + self.assertEqual(str(first['span_id']), str(span_id), trace) self.assertTrue(nginx_sent_a_trace, log_lines) From 9887888ccccc78c540d43163ec9f1231811b4042 Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Fri, 29 Aug 2025 11:15:15 -0700 Subject: [PATCH 4/5] Run formatter on Python code --- .../cases/auth_requests/test_auth_requests.py | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/test/cases/auth_requests/test_auth_requests.py b/test/cases/auth_requests/test_auth_requests.py index 80d69611..2cd5bb77 100644 --- a/test/cases/auth_requests/test_auth_requests.py +++ b/test/cases/auth_requests/test_auth_requests.py @@ -42,11 +42,15 @@ def test_no_auth_request_is_successful(self): if first['service'] == 'nginx': nginx_sent_a_trace = True self.assertEqual(0, len(rest), chunk) - self.assertEqual(first['meta']['http.url'], f'http://nginx{path}', first) - self.assertEqual(first['meta']['nginx.location'], path, first) + self.assertEqual(first['meta']['http.url'], + f'http://nginx{path}', first) + self.assertEqual(first['meta']['nginx.location'], path, + first) - self.assertEqual(str(first['trace_id']), str(trace_id), trace) - self.assertEqual(str(first['span_id']), str(span_id), trace) + self.assertEqual(str(first['trace_id']), str(trace_id), + trace) + self.assertEqual(str(first['span_id']), str(span_id), + trace) self.assertTrue(nginx_sent_a_trace, log_lines) @@ -59,8 +63,9 @@ def test_auth_request_with_auth_token_is_successful(self): self.orch.sync_service('agent') + path = '/http' status, _, body = self.orch.send_nginx_http_request( - '/http', 80, headers={'x-token': 'mysecret'}) + path, 80, headers={'x-token': 'mysecret'}) self.assertEqual(status, 200) response = json.loads(body) self.assertEqual(response["service"], "http") @@ -85,18 +90,24 @@ def test_auth_request_with_auth_token_is_successful(self): if first['service'] == 'nginx': nginx_sent_a_trace = True self.assertEqual(1, len(rest), chunk) - self.assertEqual(first['meta']['http.url'], 'http://nginx/http', first) - self.assertEqual(first['meta']['nginx.location'], '/http', first) + self.assertEqual(first['meta']['http.url'], + f'http://nginx{path}', first) + self.assertEqual(first['meta']['nginx.location'], path, + first) # Assert that the subrequest was traced self.assertEqual('nginx', rest[0]['service'], rest[0]) - self.assertEqual(rest[0]['meta']['http.url'], 'http://nginx/http', rest[0]) - self.assertEqual(rest[0]['meta']['nginx.location'], '/auth', rest[0]) + self.assertEqual(rest[0]['meta']['http.url'], + f'http://nginx{path}', rest[0]) + self.assertEqual(rest[0]['meta']['nginx.location'], + '/auth', rest[0]) # Assert existing behavior that the trace ID and span ID # match the trace ID and span ID of the nginx subrequest. - self.assertEqual(str(first['trace_id']), str(trace_id), trace) - self.assertEqual(str(first['span_id']), str(span_id), trace) + self.assertEqual(str(first['trace_id']), str(trace_id), + trace) + self.assertEqual(str(first['span_id']), str(span_id), + trace) self.assertTrue(nginx_sent_a_trace, log_lines) @@ -109,7 +120,8 @@ def test_auth_request_without_auth_token_is_not_successful(self): self.orch.sync_service('agent') - status, _, _ = self.orch.send_nginx_http_request('/http') + path = '/http' + status, _, _ = self.orch.send_nginx_http_request(path) self.assertEqual(status, 401) self.orch.reload_nginx() @@ -128,11 +140,15 @@ def test_auth_request_without_auth_token_is_not_successful(self): if first['service'] == 'nginx': nginx_sent_a_trace = True self.assertEqual(1, len(rest), chunk) - self.assertEqual(first['meta']['http.url'], 'http://nginx/http', first) - self.assertEqual(first['meta']['nginx.location'], '/http', first) + self.assertEqual(first['meta']['http.url'], + f'http://nginx/{path}', first) + self.assertEqual(first['meta']['nginx.location'], path, + first) self.assertEqual('nginx', rest[0]['service'], rest[0]) - self.assertEqual(rest[0]['meta']['http.url'], 'http://nginx/http', rest[0]) - self.assertEqual(rest[0]['meta']['nginx.location'], '/auth', rest[0]) + self.assertEqual(rest[0]['meta']['http.url'], + 'http://nginx/http', rest[0]) + self.assertEqual(rest[0]['meta']['nginx.location'], + '/auth', rest[0]) self.assertTrue(nginx_sent_a_trace, log_lines) From a00f0926a26269602de24e5649588ff5dbe65657 Mon Sep 17 00:00:00 2001 From: Zach Montoya Date: Tue, 2 Sep 2025 09:03:21 -0700 Subject: [PATCH 5/5] Fix bad assert in test case --- test/cases/auth_requests/test_auth_requests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cases/auth_requests/test_auth_requests.py b/test/cases/auth_requests/test_auth_requests.py index 2cd5bb77..0244c2d5 100644 --- a/test/cases/auth_requests/test_auth_requests.py +++ b/test/cases/auth_requests/test_auth_requests.py @@ -141,7 +141,7 @@ def test_auth_request_without_auth_token_is_not_successful(self): nginx_sent_a_trace = True self.assertEqual(1, len(rest), chunk) self.assertEqual(first['meta']['http.url'], - f'http://nginx/{path}', first) + f'http://nginx{path}', first) self.assertEqual(first['meta']['nginx.location'], path, first)