fix(opentelemetry): validate x-request-id before using it as trace_id by prasunsrivastav123-lang · Pull Request #12990 · apache/apisix

prasunsrivastav123-lang · 2026-02-09T13:48:43Z

Description

This PR fixes a crash in the OpenTelemetry plugin when trace_id_source is set to x-request-id and the request ID is not a valid 32-hex trace ID (for example, a UUID).
The plugin now validates the header and gracefully falls back to the default trace ID generator instead of passing invalid values to the OpenTelemetry encoder.

Fixes #12976
Fixes #12976

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

prasunsrivastav123-lang · 2026-02-10T19:16:24Z

@indrekj @markokocic @huacnlee pls review this pr

Baoyuantop · 2026-02-11T01:42:57Z

Hi @prasunsrivastav123-lang, please add a test case for this fix.

prasunsrivastav123-lang · 2026-02-12T08:51:17Z

hey @Baoyuantop i added a test case pls review this pr thank you !

Baoyuantop · 2026-02-13T06:17:43Z

Hi @prasunsrivastav123-lang, please fix failed CI

prasunsrivastav123-lang · 2026-02-13T06:32:03Z

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 .

Copilot

Pull request overview

This PR fixes a crash in the OpenTelemetry plugin when trace_id_source is set to x-request-id and the request ID is not a valid 32-character hexadecimal trace ID (such as a UUID with hyphens). The plugin now validates the X-Request-Id header and gracefully falls back to the default trace ID generator instead of passing invalid values to the OpenTelemetry encoder, which was causing a Lua runtime error.

Changes:

Added is_valid_trace_id() function to validate that trace IDs are exactly 32 hexadecimal characters
Modified create_tracer_obj() to validate the X-Request-Id header before using it as a trace_id, with fallback to the original generator for invalid values
Added test case (TEST 21) to verify that invalid X-Request-Id headers (UUID format) don't cause crashes

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
apisix/plugins/opentelemetry.lua	Added validation function and modified trace ID generation logic to validate and fallback for invalid request IDs
t/plugin/opentelemetry.t	Added test case to verify invalid X-Request-Id doesn't crash the plugin

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T07:55:00Z

+local function is_valid_trace_id(trace_id)
+    if not trace_id or #trace_id ~= 32 then
+        return false
+    end


The validation function should reject trace IDs that are all zeros (00000000000000000000000000000000) according to the W3C Trace Context specification. Currently, the function only checks length and hex format, but an all-zeros trace_id is invalid and should be rejected to ensure proper fallback to the default generator.

Suggested change

end

end

-- W3C Trace Context: trace-id of all zeros is invalid

if trace_id == "00000000000000000000000000000000" then

return false

end

Copilot · 2026-02-13T07:55:00Z

+   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


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.

Copilot · 2026-02-13T07:55:01Z

+   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


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.

Suggested change

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

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

Copilot · 2026-02-13T07:55:01Z

+--- response_body
+opentracing
+--- no_error_log
+[error]


The test verifies that an invalid X-Request-Id (UUID format) doesn't cause a crash, but it doesn't verify that the plugin correctly falls back to generating a valid trace ID. Consider adding a check in a follow-up test to verify that the trace appears in the OpenTelemetry collector logs with a valid trace_id, similar to TEST 16 which checks the log output.

Suggested change

[error]

[error]

=== TEST 22: invalid x-request-id should still generate a trace_id

--- request

GET /opentracing

--- more_headers

X-Request-Id: 550e8400-e29b-41d4-a716-446655440000

--- wait: 2

--- grep_error_log eval

qr/trace_id/

--- grep_error_log_out

trace_id

- Add is_valid_trace_id() to validate 32-char hex trace IDs - Reject all-zero trace IDs per W3C spec - Prevent multiple id_generator overrides - Add tests for invalid X-Request-Id fallback behavior Signed-off-by: Prasun Srivastav <prasunsrivastav123@gmail.com>

prasunsrivastav123-lang · 2026-02-13T18:29:18Z

@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!

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-14T06:34:22Z

+   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


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.

Copilot · 2026-02-14T06:34:22Z

+   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


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".

prasunsrivastav123-lang · 2026-02-24T11:17:56Z

@Baoyuantop i just Removed the global flag and simplified the id_generator override as suggested.
Could you please recheck? Thanks!

prasunsrivastav123-lang · 2026-03-04T17:57:41Z

@Baoyuantop can you review this

Baoyuantop · 2026-03-06T00:19:38Z

Hi @prasunsrivastav123-lang, please fix failed CI

prasunsrivastav123-lang · 2026-03-06T05:08:42Z

Hi @Baoyuantop , I’ve fixed the CI issue by reindexing the tests and pushed the update. Could you please take another look? Thanks!

moonming

Hi @prasunsrivastav123-lang, thank you for fixing the x-request-id validation in OpenTelemetry (#12976)!

Validating the x-request-id format before using it as a trace_id is the right approach — invalid formats could cause issues with tracing backends.

The implementation with a fallback to the default generator looks clean. Could you confirm that the tests cover:

Valid 32-character hex trace IDs
Invalid formats (too short, non-hex characters, empty string)
Missing x-request-id header (should use default generator)

If all tests pass, this should be ready. Thank you!

prasunsrivastav123-lang · 2026-03-16T18:31:34Z

@moonming @Baoyuantop Thanks! Yes, I’ve added tests covering valid 32-character hex IDs, multiple invalid formats (UUID, all-zero, malformed), and the missing header fallback case. All tests are passing now. Please take another look.

prasunsrivastav123-lang · 2026-03-27T18:00:27Z

@moonming if there need any other changes?

AlinsRan · 2026-04-02T00:58:15Z

+        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()


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.

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

Hi @prasunsrivastav123-lang, has this comment been resolved?

Baoyuantop · 2026-04-09T07:27:17Z

Hi @prasunsrivastav123-lang, there's a conflict in a code file. Could you help me resolve it?

prasunsrivastav123-lang · 2026-04-09T08:12:25Z

sure 😃

prasunsrivastav123-lang · 2026-04-10T16:13:13Z

@Baoyuantop is it ok now ?

Baoyuantop · 2026-04-13T08:52:35Z

Hi @prasunsrivastav123-lang, please fix the failed CI

…st coverage

prasunsrivastav123-lang · 2026-04-14T19:13:40Z

@Baoyuantop Updated based on feedback. Added full validation and test coverage.

- Fix mixed indentation (0/3/4/8 spaces) in is_valid_trace_id() and create_tracer_obj() to use consistent 4-space indentation - Restore master's original TEST 20-22 (additional_attributes with numeric nginx variables) that were corrupted during merge - Renumber PR's new test cases as TEST 23-31 to avoid duplicate test numbers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Baoyuantop · 2026-04-15T02:39:54Z

Hi @prasunsrivastav123-lang, there were some formatting issues when the code was merged; I’ve corrected them.

Replace direct access to Lua global 'string' with a local alias 'string_lower' to satisfy APISIX lint rules. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

prasunsrivastav123-lang · 2026-04-16T18:08:37Z

@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.

fix(opentelemetry): validate x-request-id before using it as trace_id

9828d17

dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Feb 9, 2026

test(opentelemetry): add test for invalid x-request-id handling

3f93a20

dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Feb 12, 2026

Baoyuantop added the wait for update wait for the author's response in this issue/PR label Feb 13, 2026

test(opentelemetry): add test for invalid x-request-id handling

28e8dbc

github-actions Bot added the user responded label Feb 13, 2026

Baoyuantop requested a review from Copilot February 13, 2026 07:49

Baoyuantop added awaiting review and removed wait for update wait for the author's response in this issue/PR user responded labels Feb 13, 2026

Baoyuantop added this to ⚡️ Apache APISIX Roadmap Feb 13, 2026

Baoyuantop moved this to 👀 In review in ⚡️ Apache APISIX Roadmap Feb 13, 2026

Copilot started reviewing on behalf of Baoyuantop February 13, 2026 07:49 View session

Baoyuantop assigned prasunsrivastav123-lang Feb 13, 2026

Copilot AI reviewed Feb 13, 2026

View reviewed changes

dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Feb 13, 2026

Baoyuantop requested a review from Copilot February 14, 2026 06:29

Copilot started reviewing on behalf of Baoyuantop February 14, 2026 06:29 View session

Copilot AI reviewed Feb 14, 2026

View reviewed changes

Baoyuantop reviewed Feb 24, 2026

View reviewed changes

Comment thread apisix/plugins/opentelemetry.lua Outdated

Comment thread apisix/plugins/opentelemetry.lua Outdated

fix: simplify id_generator override and remove global flag

66a9196

Baoyuantop previously approved these changes Mar 6, 2026

View reviewed changes

test: reindex opentelemetry tests to fix CI

d21f243

prasunsrivastav123-lang dismissed Baoyuantop’s stale review via d21f243 March 6, 2026 05:07

moonming requested changes Mar 16, 2026

View reviewed changes

test: extend x-request-id validation coverage

9e52b7a

test: add coverage for valid and invalid x-request-id handling

b45f437

Baoyuantop previously approved these changes Mar 18, 2026

View reviewed changes

Baoyuantop requested a review from moonming March 18, 2026 07:41

Baoyuantop requested a review from membphis April 1, 2026 09:32

AlinsRan reviewed Apr 2, 2026

View reviewed changes

Merge branch 'master' into fix-otel-invalid-trace-id

c7fc02a

prasunsrivastav123-lang dismissed Baoyuantop’s stale review via c7fc02a April 10, 2026 16:12

refactor(opentelemetry): make trace_id validation pure and improve te…

5b734d7

…st coverage

fix(lint): use local string_lower instead of global string.lower

076bad4

Replace direct access to Lua global 'string' with a local alias 'string_lower' to satisfy APISIX lint rules. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

-    end
+    end
+    -- W3C Trace Context: trace-id of all zeros is invalid
+    if trace_id == "00000000000000000000000000000000" then
+        return false
+    end

-[error]
+[error]
+=== TEST 22: invalid x-request-id should still generate a trace_id
+--- request
+GET /opentracing
+--- more_headers
+X-Request-Id: 550e8400-e29b-41d4-a716-446655440000
+--- wait: 2
+--- grep_error_log eval
+qr/trace_id/
+--- grep_error_log_out
+trace_id

Conversation

prasunsrivastav123-lang commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

prasunsrivastav123-lang commented Feb 10, 2026

Uh oh!

Baoyuantop commented Feb 11, 2026

Uh oh!

prasunsrivastav123-lang commented Feb 12, 2026

Uh oh!

Baoyuantop commented Feb 13, 2026

Uh oh!

prasunsrivastav123-lang commented Feb 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

prasunsrivastav123-lang commented Feb 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prasunsrivastav123-lang commented Feb 24, 2026

Uh oh!

prasunsrivastav123-lang commented Mar 4, 2026

Uh oh!

Baoyuantop commented Mar 6, 2026

Uh oh!

prasunsrivastav123-lang commented Mar 6, 2026

Uh oh!

moonming left a comment

Choose a reason for hiding this comment

Uh oh!

prasunsrivastav123-lang commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prasunsrivastav123-lang commented Mar 27, 2026

Uh oh!

AlinsRan Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prasunsrivastav123-lang Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Baoyuantop Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Baoyuantop commented Apr 9, 2026

Uh oh!

prasunsrivastav123-lang commented Apr 9, 2026

Uh oh!

prasunsrivastav123-lang commented Apr 10, 2026

prasunsrivastav123-lang commented Feb 9, 2026 •

edited

Loading

prasunsrivastav123-lang commented Mar 16, 2026 •

edited

Loading

AlinsRan Apr 2, 2026 •

edited

Loading