Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
AlinsRan merged PR #12990: URL: https://github.com/apache/apisix/pull/12990 -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
membphis commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4923709476 P2 follow-up: the current commit history still contains AI co-author trailers such as `Co-authored-by: Copilot ...`. Before merge, please clean those trailers from the final commit history (for example via squash/rewrite), so the project history does not carry AI attribution markers. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
AlinsRan commented on code in PR #12990: URL: https://github.com/apache/apisix/pull/12990#discussion_r3548188924 ## apisix/plugins/opentelemetry.lua: ## @@ -230,12 +250,23 @@ 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 plugin_info.trace_id_source == "x-request-id" + and not id_generator_overridden then +id_generator.new_ids = function() + +local header_trace_id = core.request.headers()["x-request-id"] +or ngx_var.request_id + +if is_valid_trace_id(header_trace_id) then +return header_trace_id, id_generator.new_span_id() end + +-- fallback to default generator for invalid values (e.g. UUID) +return original_new_ids() end +id_generator_overridden = true +end Review Comment: Fixed. `trace_id_source` lives in plugin metadata (global), so there is no per-route mode to preserve — `create_tracer_obj` now resets `id_generator.new_ids` to the captured default first and only wraps it for the `x-request-id` mode. The lrucache is keyed on `metadata.modifiedIndex`, so switching back to `random` rebuilds the tracer and takes effect without a worker restart. Added a regression test that a valid `X-Request-Id` is ignored once `trace_id_source` is `random`. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
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-44665544
+--- 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-44665544
+--- 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:
+--- 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-44665544
+--- wait: 2
+--- response_body
+opentracing
+--- no_error_log
+[error]
+
+
+
+=== TEST 25: invalid x-request-id should still generate a valid trace
+--
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
AlinsRan commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r3535159917
##
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-44665544
+--- 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-44665544
+--- 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:
+--- 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-44665544
Review Comment:
fixed.
--
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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
AlinsRan commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r3535159917
##
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-44665544
+--- 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-44665544
+--- 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:
+--- 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-44665544
Review Comment:
fixed.
--
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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
nic-6443 commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r3393077871
##
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-44665544
+--- 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-44665544
+--- 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:
+--- 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:
This assertion can't fail: the all-zero id itself matches `[0-9a-f]{32}`, so
the test passes even if the validation it's supposed to verify is removed. It
also matches whatever span an *earlier* test exported, since `tail -n 1` on the
collector file is shared state across blocks. Something like
`qr/"traceId"\s*:\s*"(?!0{32})[0-9a-f]{32}"/` would at least pin down the
rejection here; the same vacuous-match concern applies to TESTs 25 and 28-31.
Unrelated nit while in this file: the first hunk is a whitespace-only change
before TEST 20 (4 blank lines between blocks instead of 3) — please drop it.
##
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-44665544
+--- 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-44665544
+--- 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:
+--- 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 /opent
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
nic-6443 commented on code in PR #12990: URL: https://github.com/apache/apisix/pull/12990#discussion_r3393077315 ## 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: A duplicated `X-Request-Id` header still crashes this: `core.request.headers()` is `ngx.req.get_headers()`, which returns a Lua table when the same header appears more than once, so `string_lower(trace_id)` throws `bad argument`. And since plugin phase handlers are not pcall-wrapped, that error now fails the request with a 500 on every such request (before this patch the same input crashed asynchronously in the export timer). Given the whole point of this fix is to validate untrusted header input, it should survive this case too — a `type(trace_id) ~= "string"` check (e.g. at the top of `is_valid_trace_id`, before any `string_lower`) would cover it. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4262361841 @Baoyuantop Thanks for fixing the formatting! I’ve updated the implementation to ensure trace_id validation is handled cleanly: - Only valid 32-character hex trace IDs are used - Invalid formats (non-hex, incorrect length, all-zero) fall back to the default generator - Missing x-request-id also correctly falls back Test cases have been added to cover all these scenarios. Please let me know if any further changes are needed. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Baoyuantop commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4248737446 Hi @prasunsrivastav123-lang, there were some formatting issues when the code was merged; I’ve corrected them. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4246486752 @Baoyuantop Updated based on feedback. Added full validation and test coverage. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Baoyuantop commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4235095993 Hi @prasunsrivastav123-lang, please fix the failed CI -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Baoyuantop commented on code in PR #12990: URL: https://github.com/apache/apisix/pull/12990#discussion_r3071856252 ## 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: Hi @prasunsrivastav123-lang, has this comment been resolved? -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4225143392 @Baoyuantop is it ok now ? -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4212646533 sure 😃 -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Baoyuantop commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4212343078 Hi @prasunsrivastav123-lang, there's a conflict in a code file. Could you help me resolve it? -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on code in PR #12990: URL: https://github.com/apache/apisix/pull/12990#discussion_r3030325293 ## 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: You're right — the current implementation wraps `id_generator.new_ids` multiple times when the tracer is recreated, which breaks idempotency and can lead to stacked wrappers over time. My intention was to validate the x-request-id and fallback safely, but I see that the current approach needs adjustment to avoid repeated wrapping. I’ll update the implementation to ensure the override happens only once (idempotent behavior) and push a fix shortly -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
AlinsRan commented on code in PR #12990: URL: https://github.com/apache/apisix/pull/12990#discussion_r3025359907 ## 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: The lrucache has a 24-hour TTL After expiry, create_tracer_obj is called again. At that point `id_generator.new_ids` is already the closure from the previous call, so `_original_new_ids` captures the already-overridden function. Each 24h expiry adds another wrapper layer. The pre-PR code was idempotent (it overwrote the same closure). This PR makes it strictly worse. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
AlinsRan commented on code in PR #12990: URL: https://github.com/apache/apisix/pull/12990#discussion_r3025359907 ## 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: The lrucache has a 24-hour TTL (line 54). After expiry, create_tracer_obj is called again. At that point id_generator.new_ids is already the closure from the previous call, so _original_new_ids captures the already-overridden function. Each 24h expiry adds another wrapper layer — unbounded nesting over days/weeks in production. The pre-PR code was idempotent (it overwrote the same closure). This PR makes it strictly worse. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4144317239 @moonming if there need any other changes? -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4069755134 @moonming I've added additional tests covering empty, non-hex, malformed, and missing x-request-id values to ensure fallback to the default generator. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4009605221 Hi @Baoyuantop , I’ve fixed the CI issue by reindexing the tests and pushed the update. Could you please take another look? Thanks! -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Baoyuantop commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-4008642844 Hi @prasunsrivastav123-lang, please fix failed CI -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-3999194942 @Baoyuantop can you review this -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-3951000675 @Baoyuantop i just Removed the global flag and simplified the id_generator override as suggested. Could you please recheck? Thanks! -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Baoyuantop commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r2844273557
##
apisix/plugins/opentelemetry.lua:
##
@@ -29,6 +29,8 @@ local exporter_client_new =
require("opentelemetry.trace.exporter.http_client").
local otlp_exporter_new = require("opentelemetry.trace.exporter.otlp").new
local batch_span_processor_new =
require("opentelemetry.trace.batch_span_processor").new
local id_generator = require("opentelemetry.trace.id_generator")
+local original_new_ids = id_generator.new_ids
+local id_generator_overridden = false
Review Comment:
The `id_generator_overridden` flag, once set to true, is never reset. If an
administrator changes `trace_id_source` back to `random` from `x-request-id`,
all worker processes will continue to attempt to extract trace IDs from
`x-request-id` because `id_generator.new_ids` has been replaced with a custom
function and will never be restored. This could lead to a silent bug where
configuration changes don't take effect (although this issue existed in the
original code, the new implementation has solidified it through a global flag).
The global flag is redundant. `original_new_ids` already captures the
original function reference during module loading; multiple executions of
`id_generator.new_ids = function()...end` simply replace the function reference
and do not introduce nested wrapping issues. Introducing the global flag solves
a problem that doesn't actually exist, but introduces a real bug.
In fact, the correct and simplest fix is to remove
`id_generator_overridden` and the module-level `original_new_ids`, and directly
capture and verify the original function reference within the existing `if`
block:
```
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
if is_valid_trace_id(trace_id) then
return trace_id, id_generator.new_span_id()
end
return _original_new_ids()
end
end
```
##
apisix/plugins/opentelemetry.lua:
##
@@ -230,12 +250,23 @@ 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 plugin_info.trace_id_source == "x-request-id"
Review Comment:
Please fix the code format
--
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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-3902298449 @Baoyuantop I’ve addressed the review comments and updated the tests accordingly. Could you please take another look? -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Copilot commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r2807109060
##
apisix/plugins/opentelemetry.lua:
##
@@ -230,12 +250,23 @@ 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 plugin_info.trace_id_source == "x-request-id"
+ and not id_generator_overridden then
+id_generator.new_ids = function()
+
+local header_trace_id = core.request.headers()["x-request-id"]
+or ngx_var.request_id
+
+if is_valid_trace_id(header_trace_id) then
+return header_trace_id, id_generator.new_span_id()
end
+
+-- fallback to default generator for invalid values (e.g. UUID)
+return original_new_ids()
end
+id_generator_overridden = true
+end
Review Comment:
The indentation is inconsistent in this block. Lines 253-254 have leading
spaces instead of consistent indentation, and line 256 appears to have extra
trailing spaces. This should be corrected to match the surrounding code style.
##
apisix/plugins/opentelemetry.lua:
##
@@ -230,12 +250,23 @@ 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 plugin_info.trace_id_source == "x-request-id"
+ and not id_generator_overridden then
+id_generator.new_ids = function()
+
+local header_trace_id = core.request.headers()["x-request-id"]
+or ngx_var.request_id
+
+if is_valid_trace_id(header_trace_id) then
+return header_trace_id, id_generator.new_span_id()
end
+
+-- fallback to default generator for invalid values (e.g. UUID)
+return original_new_ids()
end
+id_generator_overridden = true
+end
Review Comment:
The id_generator override is applied globally and permanently once
trace_id_source is set to "x-request-id". If the plugin metadata is later
updated to set trace_id_source to "random", the override persists and all
requests will still go through the x-request-id validation logic before falling
back to the original generator. This is inefficient and not the intended
behavior. Consider either: 1) checking the current trace_id_source value inside
the overridden function and calling original_new_ids directly when it's
"random", or 2) implementing a mechanism to restore the original
id_generator.new_ids when the metadata changes back to "random".
##
t/plugin/opentelemetry.t:
##
@@ -434,3 +434,65 @@ HEAD /specific_status
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/.*\/specific_status.*/
+
+
+
+=== TEST 20: 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,
+[[{
+"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 21: invalid x-request-id should not crash
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-44665544
+--- wait: 2
+--- response_body
+opentracing
+--- no_error_log
+[error]
+
+
+
+=== TEST 22: invalid x-request-id should still generate a valid trace
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-44665544
+--- wait: 2
+--- exec
+tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
+--- response_body eval
+qr/"traceId":"[0-9a-f]{32}"/
Review Comment:
While the test covers the main scenario (UUID with hyphens), it would be
beneficial to add test cases for additional edge cases such as: 1) an all-zero
trace ID () which should be rejected per W3C
spec, 2) uppercase hex characters to verify case handling, and 3) trace
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-3898691012 @Baoyuantop Addressed the review feedback: Added validation for all-zero trace IDs Prevented multiple id_generator overrides Added a test to verify valid fallback trace generation Kindly requesting a re-review. Thanks! -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Copilot commented on code in PR #12990:
URL: https://github.com/apache/apisix/pull/12990#discussion_r2802805095
##
apisix/plugins/opentelemetry.lua:
##
@@ -230,12 +237,22 @@ 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 plugin_info.trace_id_source == "x-request-id" then
+ local original_new_ids = id_generator.new_ids
+
+id_generator.new_ids = function()
+local header_trace_id = core.request.headers()["x-request-id"]
+or ngx_var.request_id
+
+if is_valid_trace_id(header_trace_id) then
+return header_trace_id, id_generator.new_span_id()
end
+
+-- fallback to default generator for invalid values (e.g. UUID)
+return original_new_ids()
end
Review Comment:
The original_new_ids variable is saved inside the if block, which means it's
saved every time create_tracer_obj is called when trace_id_source is
'x-request-id'. After the first call, id_generator.new_ids has already been
overridden, so subsequent calls (e.g., after cache expiration after 24 hours)
would save the already-overridden function, creating nested function wrappers.
To fix this, save the original id_generator.new_ids once at module load time
(outside the function), and check if it's already been overridden before
overriding again, or restore it when trace_id_source changes.
##
apisix/plugins/opentelemetry.lua:
##
@@ -230,12 +237,22 @@ 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 plugin_info.trace_id_source == "x-request-id" then
+ local original_new_ids = id_generator.new_ids
+
+id_generator.new_ids = function()
+local header_trace_id = core.request.headers()["x-request-id"]
+or ngx_var.request_id
+
+if is_valid_trace_id(header_trace_id) then
+return header_trace_id, id_generator.new_span_id()
end
+
+-- fallback to default generator for invalid values (e.g. UUID)
+return original_new_ids()
end
+end
Review Comment:
Inconsistent indentation in the if statement and local variable declaration.
The if statement uses 3 spaces while the local statement uses 2 spaces.
According to the .editorconfig, the codebase uses space indentation
consistently. Both lines should be indented with 4 spaces to match the function
body indentation.
```suggestion
if plugin_info.trace_id_source == "x-request-id" then
local original_new_ids = id_generator.new_ids
id_generator.new_ids = function()
local header_trace_id = core.request.headers()["x-request-id"]
or ngx_var.request_id
if is_valid_trace_id(header_trace_id) then
return header_trace_id, id_generator.new_span_id()
end
-- fallback to default generator for invalid values (e.g. UUID)
return original_new_ids()
end
end
```
##
t/plugin/opentelemetry.t:
##
@@ -434,3 +434,52 @@ HEAD /specific_status
tail -n 1 ci/pod/otelcol-contrib/data-otlp.json
--- response_body eval
qr/.*\/specific_status.*/
+
+
+
+=== TEST 20: 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,
+[[{
+"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 21: invalid x-request-id should not crash
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-44665544
+--- wait: 2
+--- response_body
+opentracing
+--- no_error_log
+[error]
Review Comment:
The test veri
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-3895168577 Hi @Baoyuantop I’ve added the requested test case to cover the invalid X-Request-Id scenario and ensured the route is properly recreated to avoid upstream issues.if any adjustment need pls tell . -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Baoyuantop commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-3895126913 Hi @prasunsrivastav123-lang, please fix failed CI -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-3889560885 hey @Baoyuantop i added a test case pls review this pr thank you ! -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
Baoyuantop commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-3881657664 Hi @prasunsrivastav123-lang, please add a test case for this fix. -- 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]
Re: [PR] fix(opentelemetry): validate x-request-id before using it as trace_id [apisix]
prasunsrivastav123-lang commented on PR #12990: URL: https://github.com/apache/apisix/pull/12990#issuecomment-3880186642 @indrekj @markokocic @huacnlee pls review this pr -- 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]
