AlinsRan commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r3540804818
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -232,9 +253,24 @@ end
local function create_tracer_obj(conf, plugin_info)
if plugin_info.trace_id_source == "x-request-id" then
- id_generator.new_ids = function()
- local trace_id = core.request.headers()["x-request-id"] or
ngx_var.request_id
- return trace_id, id_generator.new_span_id()
+ if not id_generator._wrapped then
+ local _original_new_ids = id_generator.new_ids
+
+ id_generator.new_ids = function()
+ local trace_id = core.request.headers()["x-request-id"]
+ or ngx_var.request_id
+
+ trace_id = trace_id and string_lower(trace_id)
Review Comment:
Good catch — fixed. `new_ids` now only uses the header when it is a plain
string (`type(trace_id) == "string"`), so a duplicated `X-Request-Id` (which
`get_headers()` returns as a table) falls through to the default generator
instead of reaching `string_lower`. Added a duplicated-header regression test.
##########
t/plugin/opentelemetry.t:
##########
@@ -488,3 +489,143 @@ opentracing
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/.*opentelemetry-lua.*/
+
+
+
+=== TEST 23: recreate route for invalid x-request-id test
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "name": "route_name",
+ "plugins": {
+ "opentelemetry": {
+ "sampler": {
+ "name": "always_on"
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/opentracing"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+
+
+
+=== TEST 24: invalid x-request-id should not crash
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-446655440000
+--- wait: 2
+--- response_body
+opentracing
+--- no_error_log
+[error]
+
+
+
+=== TEST 25: invalid x-request-id should still generate a valid trace
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-446655440000
+--- wait: 2
+--- exec
+tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
+--- response_body eval
+qr/"traceId"\s*:\s*"[0-9a-f]{32}"/i
+
+
+
+=== TEST 26: all-zero x-request-id should not be used as trace id
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 00000000000000000000000000000000
+--- wait: 2
+--- exec
+tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
+--- response_body eval
+qr/"traceId"\s*:\s*"[0-9a-f]{32}"/i
Review Comment:
Right, that assertion could not fail. Split the request and exec into
separate blocks, and the all-zero case now asserts `(?!0{32})[0-9a-f]{32}`, so
it fails if the zero id is ever reused.
##########
t/plugin/opentelemetry.t:
##########
@@ -488,3 +489,143 @@ opentracing
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/.*opentelemetry-lua.*/
+
+
+
+=== TEST 23: recreate route for invalid x-request-id test
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "name": "route_name",
+ "plugins": {
+ "opentelemetry": {
+ "sampler": {
+ "name": "always_on"
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/opentracing"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+
+
+
+=== TEST 24: invalid x-request-id should not crash
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-446655440000
+--- wait: 2
+--- response_body
+opentracing
+--- no_error_log
+[error]
+
+
+
+=== TEST 25: invalid x-request-id should still generate a valid trace
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-446655440000
+--- wait: 2
+--- exec
+tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
+--- response_body eval
+qr/"traceId"\s*:\s*"[0-9a-f]{32}"/i
+
+
+
+=== TEST 26: all-zero x-request-id should not be used as trace id
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 00000000000000000000000000000000
+--- wait: 2
+--- exec
+tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
+--- response_body eval
+qr/"traceId"\s*:\s*"[0-9a-f]{32}"/i
+
+
+
+=== TEST 27: uppercase x-request-id should still generate a valid trace id
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550E8400-E29B-41D4-A716-446655440000
Review Comment:
Fixed — the uppercase test now sends a real 32-char hex
(`550E8400E29B41D4A716446655440000`) and asserts the exact lowercased trace id,
so the case-normalization branch is actually exercised.
##########
apisix/plugins/opentelemetry.lua:
##########
@@ -231,9 +249,17 @@ end
local function create_tracer_obj(conf, plugin_info)
if plugin_info.trace_id_source == "x-request-id" then
+ local _original_new_ids = id_generator.new_ids
+
id_generator.new_ids = function()
- local trace_id = core.request.headers()["x-request-id"] or
ngx_var.request_id
- return trace_id, id_generator.new_span_id()
+ local trace_id = core.request.headers()["x-request-id"]
+ or ngx_var.request_id
+
+ if is_valid_trace_id(trace_id) then
+ return trace_id, id_generator.new_span_id()
+ end
+
+ return _original_new_ids()
Review Comment:
Addressed: the default generator is now captured once at module load
(`original_new_ids`) instead of inside the closure, so re-creating the tracer
no longer stacks wrappers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
