Re: [PR] fix(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
nic-6443 merged PR #13232: URL: https://github.com/apache/apisix/pull/13232 -- 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(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
janiussyafiq commented on code in PR #13232:
URL: https://github.com/apache/apisix/pull/13232#discussion_r3182897173
##
t/plugin/opentelemetry6.t:
##
@@ -248,3 +241,56 @@ opentracing
end
}
}
+
+
+
+=== TEST 7: clear file
+--- exec
+echo '' > ci/pod/otelcol-contrib/data-otlp.json
+--- response_body eval
+qr//
+
+
+
+=== TEST 8: trigger two concurrent HTTP/2 requests on the same TLS connection
+--- init_by_lua_block
+require "resty.core"
+apisix = require("apisix")
+core = require("apisix.core")
+apisix.http_init()
+
+local utils = require("apisix.core.utils")
+utils.dns_parse = function (domain)
+if domain == "test1.com" then
+return {address = "127.0.0.2"}
+end
+error("unknown domain: " .. domain)
+end
+--- exec
+curl -sk --http2 --parallel --resolve "test.com:1994:127.0.0.1"
https://test.com:1994/opentracing https://test.com:1994/opentracing
Review Comment:
fixed
##
t/lib/test_otel.lua:
##
@@ -132,4 +132,50 @@ function _M.verify_tree(filepath, expected_tree)
end
+function _M.verify_isolated_traces(filepath, root_name, count)
+local spans_by_id, err = parse_spans(filepath)
+if not spans_by_id then
+return false, err
+end
+
+local traces = {}
+for _, span in pairs(spans_by_id) do
+if not traces[span.traceId] then
+traces[span.traceId] = {}
+end
+table.insert(traces[span.traceId], span.name)
+end
+
+local matching = {}
+for trace_id, names in pairs(traces) do
+for _, name in ipairs(names) do
+if name == root_name then
+table.insert(matching, { id = trace_id, names = names })
+break
+end
+end
+end
+
+if #matching ~= count then
+return false, string.format(
+"expected %d traces with span '%s', got %d",
+count, root_name, #matching)
+end
+
+for _, trace in ipairs(matching) do
+local seen = {}
+for _, name in ipairs(trace.names) do
+if seen[name] then
+return false, string.format(
+"trace %s has duplicate span '%s': cross-stream
contamination detected",
+trace.id, name)
+end
+seen[name] = true
+end
+end
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(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
shreemaan-abhishek commented on code in PR #13232:
URL: https://github.com/apache/apisix/pull/13232#discussion_r3151415605
##
t/node/tracer.t:
##
@@ -0,0 +1,105 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('debug');
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+if (!$block->extra_yaml_config) {
+my $extra_yaml_config = <<_EOC_;
+apisix:
+tracing: true
+_EOC_
+$block->set_value("extra_yaml_config", $extra_yaml_config);
+}
+
+if (!$block->request) {
+$block->set_value("request", "GET /t");
+}
+
+if (!defined $block->response_body) {
+$block->set_value("response_body", "passed\n");
+}
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: set SSL cert for test.com
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin")
+local ssl_cert = t.read_file("t/certs/apisix.crt")
+local ssl_key = t.read_file("t/certs/apisix.key")
+local core = require("apisix.core")
+local data = {cert = ssl_cert, key = ssl_key, sni = "test.com"}
+local code, body = t.test('/apisix/admin/ssls/1',
+ngx.HTTP_PUT,
+core.json.encode(data),
+[[{
+"value": {
+"sni": "test.com"
+},
+"key": "/apisix/ssls/1"
+}]]
+)
+ngx.status = code
+ngx.say(body)
+}
+}
+
+
+
+=== TEST 2: set route
+--- 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,
+[[{
+"upstream": {
+"nodes": {
+"127.0.0.1:1980": 1
+},
+"type": "roundrobin"
+},
+"uri": "/opentracing"
+}]]
+)
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+
+
+
+=== TEST 3: consecutive HTTPS keepalive requests do not crash when tracing is
enabled
+--- exec
+curl -s -k https://test.com:1994/opentracing https://test.com:1994/opentracing
Review Comment:
test.com is already used in several places in the CI, `test.com` resolution
is guaranteed by coredns setup.
--
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(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
Copilot commented on code in PR #13232:
URL: https://github.com/apache/apisix/pull/13232#discussion_r3151143517
##
t/plugin/opentelemetry6.t:
##
@@ -248,3 +241,56 @@ opentracing
end
}
}
+
+
+
+=== TEST 7: clear file
+--- exec
+echo '' > ci/pod/otelcol-contrib/data-otlp.json
+--- response_body eval
+qr//
+
+
+
+=== TEST 8: trigger two concurrent HTTP/2 requests on the same TLS connection
+--- init_by_lua_block
+require "resty.core"
+apisix = require("apisix")
+core = require("apisix.core")
+apisix.http_init()
+
+local utils = require("apisix.core.utils")
+utils.dns_parse = function (domain)
+if domain == "test1.com" then
+return {address = "127.0.0.2"}
+end
+error("unknown domain: " .. domain)
+end
+--- exec
+curl -sk --http2 --parallel --resolve "test.com:1994:127.0.0.1"
https://test.com:1994/opentracing https://test.com:1994/opentracing
Review Comment:
Using `curl --parallel` makes the test nondeterministic: (1) responses
written to stdout can be interleaved/concatenated in ways that vary by curl
version/platform, and (2) curl may open more than one HTTP/2 connection under
parallel scheduling, undermining the goal of 'same TLS connection' validation.
For a stable regression, prefer a single curl invocation without `--parallel`
(still reuses the same HTTP/2 connection sequentially) or write outputs to
separate files (`--output`) and assert their contents independently.
##
t/node/tracer.t:
##
@@ -0,0 +1,105 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('debug');
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+if (!$block->extra_yaml_config) {
+my $extra_yaml_config = <<_EOC_;
+apisix:
+tracing: true
+_EOC_
+$block->set_value("extra_yaml_config", $extra_yaml_config);
+}
+
+if (!$block->request) {
+$block->set_value("request", "GET /t");
+}
+
+if (!defined $block->response_body) {
+$block->set_value("response_body", "passed\n");
+}
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: set SSL cert for test.com
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin")
+local ssl_cert = t.read_file("t/certs/apisix.crt")
+local ssl_key = t.read_file("t/certs/apisix.key")
+local core = require("apisix.core")
+local data = {cert = ssl_cert, key = ssl_key, sni = "test.com"}
+local code, body = t.test('/apisix/admin/ssls/1',
+ngx.HTTP_PUT,
+core.json.encode(data),
+[[{
+"value": {
+"sni": "test.com"
+},
+"key": "/apisix/ssls/1"
+}]]
+)
+ngx.status = code
+ngx.say(body)
+}
+}
+
+
+
+=== TEST 2: set route
+--- 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,
+[[{
+"upstream": {
+"nodes": {
+"127.0.0.1:1980": 1
+},
+"type": "roundrobin"
+},
+"uri": "/opentracing"
+}]]
+)
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+
+
+
+=== TEST 3: consecutive HTTPS keepalive requests do not crash when tracing is
enabled
+--- exec
+curl -s -k https://test.com:1994/opentracing https://test.com:1994/opentracing
Review Comment:
This test relies on `test.com` resolving in the test environment. Many CI
environments won’t have DNS/hosts entries for `test.com`, making the test
flaky. Use `--resolve \"test.com:1994:127.0.0.1\"` (or the project-standard
mechanism used in other tests) to ensure the request targets the local APISIX
instance deterministically.
``
Re: [PR] fix(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
janiussyafiq commented on PR #13232: URL: https://github.com/apache/apisix/pull/13232#issuecomment-4306533741 Thanks for the thorough and detailed explanation @shreemaan-abhishek, and I totally agree with your view and have already implemented the fix based on your suggestion. The changes made: - `tracer.start()` now uses `rawget(ctx, "tracing")` instead of `ctx.tracing`, bypassing the `__index` metatable chain so each per-request ctx gets its own fresh tracing table - `tracer.release()` now uses `rawset(ctx, "tracing", nil)` to clear the per-request ctx's own field directly - `tracer.release(ngx_ctx)` is now explicitly called at all exit points of `ssl_client_hello_phase` in `init.lua` to prevent the SSL-phase tracing table from leaking for the lifetime of the TLS connection As you mentioned, one consequence of this fix is that the SSL-handshake span is now its own standalone trace rather than being stitched into the HTTP request's trace. This also means `ssl_client_hello_phase` no longer appears in the OTel exported spans — since the SSL-phase tracing table is released at the end of `ssl_client_hello_phase`, the OTel plugin (which runs in the HTTP log phase) never has access to it. As a result, the existing `ssl_client_hello_phase` child span assertion in `opentelemetry6.t` TEST 6 has been removed to reflect this new behaviour. cc @nic-6443 For the tests, I have added: - `t/node/tracer.t`: consecutive HTTPS keepalive - `t/plugin/opentelemetry6.t`: concurrent HTTP/2 span isolation test using a new `verify_isolated_traces` helper in `t/lib/test_otel.lua` that verifies each stream produces its own separate trace with no duplicate spans (cross-stream contamination) -- 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(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
nic-6443 commented on PR #13232: URL: https://github.com/apache/apisix/pull/13232#issuecomment-4301621322 I have carefully read the problem analysis and fix suggestions provided by @shreemaan-abhishek, and I agree with his views. It is the correct approach to separate the tracers for SSL and HTTP, because the same SSL connection can be shared by multiple HTTP requests, and forcibly linking them together will lead to data disorder issues. -- 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(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
shreemaan-abhishek commented on PR #13232:
URL: https://github.com/apache/apisix/pull/13232#issuecomment-4301494151
### `ctx.tracing = nil` in `release()` is a no-op on a request-phase ctx
@hachi029 already demonstrated this empirically. The per-request `ngx.ctx`
in an HTTP phase is created as a fresh empty table whose `__index` metatable
points at the **connection-scoped** SSL-phase ctx. On the same TLS connection
this means three distinct Lua tables are in play:
- The **SSL-phase ctx table**: lives on the TLS connection, populated once
during the handshake, shared with every HTTP request on that connection via the
metatable link below.
- **Request 1's per-request ctx table**: a fresh empty table created on
first `ngx.ctx` access in request 1, whose metatable is the SSL-phase ctx table.
- **Request 2's per-request ctx table**: another fresh empty table created
on first `ngx.ctx` access in request 2, whose metatable is also the SSL-phase
ctx table.
`tracer.start()` runs in `ssl_client_hello_phase` (apisix/init.lua:209),
where `ctx` is the SSL-phase ctx table directly, so it writes `tracing` into
the SSL-phase ctx table. In every later HTTP phase, reading `ctx.tracing` on a
per-request ctx finds no own field called `tracing` and falls through `__index`
to the SSL-phase ctx table's `tracing` field. Writing `ctx.tracing = nil` on a
per-request ctx only affects that per-request ctx; the SSL-phase ctx table has
no `__newindex` metamethod, so its `tracing` field is untouched and the next
read on any per-request ctx still returns the same tracing table.
This matches @hachi029's repro: the logged table address `0x7ff77b5a1060` is
identical in the SSL phase, both concurrent HTTP/2 streams, and the log phase,
and `ngx.ctx.tracing = nil` followed by a re-read returns the same table.
So line 88 (`ctx.tracing = nil`) is dead code under the metatable
inheritance that OpenResty actually uses. The crash is prevented purely by the
`not tracing.spans` guard on line 42, and it only happens to work because
`tablepool.fetch` returns the just-released tracing table back (LIFO),
effectively re-fusing it onto the SSL-phase ctx's `tracing` field.
### Proposal: sever the inheritance with `rawget` / `rawset`
Both issues disappear if `tracer.start` and `tracer.release` operate on the
per-request ctx directly, bypassing the metatable:
```lua
function _M.start(ctx, name, kind)
...
local tracing = rawget(ctx, "tracing")
if not tracing then
tracing = tablepool.fetch("tracing", 0, 8)
tracing.spans = tablepool.fetch("tracing_spans", 20, 0)
rawset(ctx, "tracing", tracing)
end
...
end
function _M.release(ctx)
local tracing = rawget(ctx, "tracing")
if not tracing then
return
end
for _, sp in ipairs(tracing.spans) do
sp:release()
end
tablepool.release("tracing_spans", tracing.spans)
tablepool.release("tracing", tracing)
rawset(ctx, "tracing", nil)
end
```
Why this works:
- In the SSL phase, `ctx` is the SSL-phase ctx table directly; `rawget` on
it is identical to the regular field read. The SSL-handshake span continues to
be recorded on the SSL-phase ctx table's `tracing` field as before.
- In every HTTP phase, `rawget(ctx, "tracing")` returns `nil` on first
access because the per-request ctx has no own `tracing` key and `rawget` does
not follow `__index`. Each request initialises its own fresh tracing table on
its own per-request ctx. No cross-stream sharing, no stale-pointer crash, no
double-release.
- The `not tracing.spans` defensive check is no longer needed `rawget`
guarantees each phase only ever sees its own tracing table, which is either
`nil` or fully initialised.
### CAVEAT: Release the SSL-phase tracing table explicitly
With `rawget` in place, the tracing table on the SSL-phase ctx (set during
`ssl_client_hello_phase`) is no longer picked up or released by any HTTP
request phase, so it would leak for the lifetime of the TLS connection. It
should be released at the end of `ssl_client_hello_phase` in `apisix/init.lua`,
immediately after `span:finish(ngx_ctx)`:
```lua
span:finish(ngx_ctx)
tracer.release(ngx_ctx)
```
This is, in effect, what's happening implicitly today the SSL-phase tracing
table is being released by request 1's log phase, which is exactly the coupling
that causes #13200. Making it explicit and SSL-scoped is the correct shape.
One consequence: the SSL-handshake span becomes its own standalone trace
rather than being stitched into the HTTP request's trace. Stitching across SSL
and HTTP requires propagating only the *span context* (traceparent IDs), not
the live tracing object a separate, larger change.
TEST 4 currently only asserts the response body, which isn't sensitive to
ei
Re: [PR] fix(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
janiussyafiq commented on PR #13232:
URL: https://github.com/apache/apisix/pull/13232#issuecomment-4297774564
> We recently encountered an similar issue in our online environment
involving ngx.ctx during the SSL phase and request processing phases. Here is
an example to demonstrate the problem:
>
> nginx.conf:
>
> ```nginx
>ssl_certificate_by_lua_block {
> ngx.ctx.tracing={}
> ngx.log(ngx.WARN, "id: ", tostring(ngx.ctx.tracing))
> }
> location /slow {
> content_by_lua_block {
> ngx.log(ngx.WARN, "id(slow): ", tostring(ngx.ctx.tracing))
> table.insert(ngx.ctx.tracing, "slow_1")
> ngx.sleep(1)
> table.insert(ngx.ctx.tracing, "slow_2")
> ngx.say("slow")
> }
> }
> location /fast {
> content_by_lua_block {
> ngx.log(ngx.WARN, "id(fast): ", tostring(ngx.ctx.tracing))
> table.insert(ngx.ctx.tracing, "fast_1")
> ngx.say("fast")
> }
> }
> log_by_lua_block {
> ngx.log(ngx.WARN, "data: ", table.concat(ngx.ctx.tracing, ", "),
", id:", tostring(ngx.ctx.tracing))
> ngx.ctx.tracing=nil
> ngx.log(ngx.WARN, "nil?(after set to nil): ", ngx.ctx.tracing==nil)
> ngx.log(ngx.WARN, "data(after set to nil): ",
table.concat(ngx.ctx.tracing, ", "))
> }
> ```
>
> Run the command bellow to process two requests concurrently within the
same connection:
>
> ```
> curl -Z -k --http2 https://127.0.0.1:8443/slow https://127.0.0.1:8443/fast
> ```
>
> Error Log Output:(Prefix timestamps and metadata removed)
>
> ```
> ssl_certificate_by_lua(nginx.conf:119):3: id: table: 0x7ff77b5a1060,
context: ssl_certificate_by_lua*, client: 127.0.0.1, server: 0.0.0.0:8443
> content_by_lua(nginx.conf:131):2: id(slow): table: 0x7ff77b5a1060, client:
127.0.0.1, server: localhost, request: "GET /slow HTTP/2.0", host:
"127.0.0.1:8443"
> content_by_lua(nginx.conf:141):2: id(fast): table: 0x7ff77b5a1060, client:
127.0.0.1, server: localhost, request: "GET /fast HTTP/2.0", host:
"127.0.0.1:8443"
> log_by_lua(nginx.conf:123):2: data: slow_1, fast_1, id:table:
0x7ff77b5a1060 while logging request, client: 127.0.0.1, server: localhost,
request: "GET /fast HTTP/2.0", host: "127.0.0.1:8443"
> log_by_lua(nginx.conf:123):4: nil?(after set to nil): false while logging
request, client: 127.0.0.1, server: localhost, request: "GET /fast HTTP/2.0",
host: "127.0.0.1:8443"
> log_by_lua(nginx.conf:123):5: data(after set to nil): slow_1, fast_1 while
logging request, client: 127.0.0.1, server: localhost, request: "GET /fast
HTTP/2.0", host: "127.0.0.1:8443"
> log_by_lua(nginx.conf:123):2: data: slow_1, fast_1, slow_2, id:table:
0x7ff77b5a1060 while logging request, client: 127.0.0.1, server: localhost,
request: "GET /slow HTTP/2.0", host: "127.0.0.1:8443"
> log_by_lua(nginx.conf:123):4: nil?(after set to nil): false while logging
request, client: 127.0.0.1, server: localhost, request: "GET /slow HTTP/2.0",
host: "127.0.0.1:8443"
> log_by_lua(nginx.conf:123):5: data(after set to nil): slow_1, fast_1,
slow_2 while logging request, client: 127.0.0.1, server: localhost, request:
"GET /slow HTTP/2.0", host: "127.0.0.1:8443"
> ```
>
> Key Observations from the Logs:
>
> * ngx.ctx.tracing is shared across different requests on the same
connection, leading to data interference between requests.
> * Setting ngx.ctx.tracing = nil does not achieve the expected result; the
value persists.
>
> Based on the implementation relate to ngx.ctx
[get_ctx_table](https://github.com/openresty/lua-resty-core/blob/master/lib/resty/core/ctx.lua#L67),
the ngx.ctx accessed during the SSL phase acts as the metatable for the
ngx.ctx accessed during the request processing phase.
>
> In the example above:
>
> * The value retrieved from ngx.ctx.tracing is always the one initialized
during the SSL phase.
> * Setting ngx.ctx.tracing to nil in the request phase does nothing because
the request-level ngx.ctx table does not actually contain the tracing key
itself. When access it again, Lua continues to look up the key along the
metatable and finding the value stored in the SSL-phase ngx.ctx.
>
> In this specific issue, the PR fixes the crash, the tracing data still
interference across concurrent requests.
WDYT @Baoyuantop @membphis @nic-6443 ? I can confirm that this PR solves the
crash issue, but I am not sure how to write test case to confirm no data
interference between both requests. One idea is to check span using
opentelemetry but ig we can do it in separate PR? i'm working on a solution
right now just want to have ur guys opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to
Re: [PR] fix(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
hachi029 commented on PR #13232:
URL: https://github.com/apache/apisix/pull/13232#issuecomment-4295761677
> > Does this fix apply to HTTP/2 in cases where the second HTTP request
starts before the first one ends, causing the two requests to share the same
ctx.tracing?
>
>
https://github.com/apache/apisix/pull/13232/changes#diff-1b0efc84c955bbecaf03cb73a6b7c72f8cac0b1045c1d069fc01fa390d2c31daR109-R114
>
> i've added the test case here to address your issue. after doing some
digging, i found that in http2 requests, ctx.tracing won't be shared across
different requests since each request would init a new one. hence could u
elaborate more on your issue or if there is any reproduction steps?
> > Does this fix apply to HTTP/2 in cases where the second HTTP request
starts before the first one ends, causing the two requests to share the same
ctx.tracing?
>
>
https://github.com/apache/apisix/pull/13232/changes#diff-1b0efc84c955bbecaf03cb73a6b7c72f8cac0b1045c1d069fc01fa390d2c31daR109-R114
>
> i've added the test case here to address your issue. after doing some
digging, i found that in http2 requests, ctx.tracing won't be shared across
different requests since each request would init a new one. hence could u
elaborate more on your issue or if there is any reproduction steps?
We recently encountered an similar issue in our online environment involving
ngx.ctx during the SSL phase and request processing phases. Here is an example
to demonstrate the problem:
nginx.conf:
```nginx
ssl_certificate_by_lua_block {
ngx.ctx.tracing={}
ngx.log(ngx.WARN, "id: ", tostring(ngx.ctx.tracing))
}
log_by_lua_block {
ngx.log(ngx.WARN, "data: ", table.concat(ngx.ctx.tracing, ", "), ",
id:", tostring(ngx.ctx.tracing))
ngx.ctx.tracing=nil
ngx.log(ngx.WARN, "nil?(after set to nil): ", ngx.ctx.tracing==nil)
ngx.log(ngx.WARN, "data(after set to nil): ",
table.concat(ngx.ctx.tracing, ", "))
}
location /slow {
content_by_lua_block {
ngx.log(ngx.WARN, "id(slow): ", tostring(ngx.ctx.tracing))
table.insert(ngx.ctx.tracing, "slow_1")
ngx.sleep(1)
table.insert(ngx.ctx.tracing, "slow_2")
ngx.say("slow")
}
}
location /fast {
content_by_lua_block {
ngx.log(ngx.WARN, "id(fast): ", tostring(ngx.ctx.tracing))
table.insert(ngx.ctx.tracing, "fast_1")
ngx.say("fast")
}
}
```
Run the command bellow to process two requests concurrently within the same
connection:
```
curl -Z -k https://127.0.0.1:8443/slow https://127.0.0.1:8443/fast
```
Error Log Output:(Prefix timestamps and metadata removed)
```
ssl_certificate_by_lua(nginx.conf:119):3: id: table: 0x7ff77b5a1060,
context: ssl_certificate_by_lua*, client: 127.0.0.1, server: 0.0.0.0:8443
content_by_lua(nginx.conf:131):2: id(slow): table: 0x7ff77b5a1060, client:
127.0.0.1, server: localhost, request: "GET /slow HTTP/2.0", host:
"127.0.0.1:8443"
content_by_lua(nginx.conf:141):2: id(fast): table: 0x7ff77b5a1060, client:
127.0.0.1, server: localhost, request: "GET /fast HTTP/2.0", host:
"127.0.0.1:8443"
log_by_lua(nginx.conf:123):2: data: slow_1, fast_1, id:table: 0x7ff77b5a1060
while logging request, client: 127.0.0.1, server: localhost, request: "GET
/fast HTTP/2.0", host: "127.0.0.1:8443"
log_by_lua(nginx.conf:123):4: nil?(after set to nil): false while logging
request, client: 127.0.0.1, server: localhost, request: "GET /fast HTTP/2.0",
host: "127.0.0.1:8443"
log_by_lua(nginx.conf:123):5: data(after set to nil): slow_1, fast_1 while
logging request, client: 127.0.0.1, server: localhost, request: "GET /fast
HTTP/2.0", host: "127.0.0.1:8443"
log_by_lua(nginx.conf:123):2: data: slow_1, fast_1, slow_2, id:table:
0x7ff77b5a1060 while logging request, client: 127.0.0.1, server: localhost,
request: "GET /slow HTTP/2.0", host: "127.0.0.1:8443"
log_by_lua(nginx.conf:123):4: nil?(after set to nil): false while logging
request, client: 127.0.0.1, server: localhost, request: "GET /slow HTTP/2.0",
host: "127.0.0.1:8443"
log_by_lua(nginx.conf:123):5: data(after set to nil): slow_1, fast_1, slow_2
while logging request, client: 127.0.0.1, server: localhost, request: "GET
/slow HTTP/2.0", host: "127.0.0.1:8443"
```
Key Observations from the Logs:
- ngx.ctx.tracing is shared across different requests on the same
connection, leading to data interference between requests.
- Setting ngx.ctx.tracing = nil does not achieve the expected result; the
value persists.
Based on the implementation relate to ngx.ctx
[get_ctx_table](https://github.com/openresty/lua-resty-core/blob/master/lib/resty/core/ctx.lua#L67),
the ngx.ctx accessed during the SSL phase acts a
Re: [PR] fix(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
Baoyuantop commented on PR #13232: URL: https://github.com/apache/apisix/pull/13232#issuecomment-4294090912 The PR is on the right track; 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(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
janiussyafiq commented on PR #13232: URL: https://github.com/apache/apisix/pull/13232#issuecomment-4289443550 > Does this fix apply to HTTP/2 in cases where the second HTTP request starts before the first one ends, causing the two requests to share the same ctx.tracing? https://github.com/apache/apisix/pull/13232/changes#diff-1b0efc84c955bbecaf03cb73a6b7c72f8cac0b1045c1d069fc01fa390d2c31daR109-R114 i've added the test case here to address your issue. after doing some digging, i found that in http2 requests, ctx.tracing won't be shared across different requests since each request would init a new one. hence could u elaborate more on your issue or if there is any reproduction steps? -- 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(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
janiussyafiq commented on code in PR #13232:
URL: https://github.com/apache/apisix/pull/13232#discussion_r3118235653
##
apisix/tracer.lua:
##
@@ -77,10 +77,13 @@ function _M.release(ctx)
return
end
-for _, sp in ipairs(tracing.spans) do
-sp:release()
+local spans = tracing.spans
+if spans then
+for _, sp in ipairs(spans) do
+sp:release()
+end
+tablepool.release("tracing_spans", spans)
end
-tablepool.release("tracing_spans", tracing.spans)
tablepool.release("tracing", tracing)
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(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
hachi029 commented on PR #13232: URL: https://github.com/apache/apisix/pull/13232#issuecomment-4285992173 Does this fix apply to HTTP/2 in cases where the second HTTP request starts before the first one ends, causing the two requests to share the same ctx.tracing? -- 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(tracer): prevent stale ctx.tracing crash on HTTPS keepalive connections [apisix]
Copilot commented on code in PR #13232:
URL: https://github.com/apache/apisix/pull/13232#discussion_r3114682665
##
apisix/tracer.lua:
##
@@ -77,10 +77,13 @@ function _M.release(ctx)
return
end
-for _, sp in ipairs(tracing.spans) do
-sp:release()
+local spans = tracing.spans
+if spans then
+for _, sp in ipairs(spans) do
+sp:release()
+end
+tablepool.release("tracing_spans", spans)
end
-tablepool.release("tracing_spans", tracing.spans)
tablepool.release("tracing", tracing)
Review Comment:
After releasing `tracing` back to `lua-tablepool`, `ctx.tracing` is still
left pointing at a table that may be immediately reused by another request.
This can cause cross-request trace contamination (or double-release if
`release()` is called again) because the stale reference remains truthy and may
later contain unrelated data if the pool hands the table out again. Please
clear the request context reference (e.g., set `ctx.tracing = nil`) once the
pool releases have completed.
```suggestion
tablepool.release("tracing", tracing)
ctx.tracing = nil
```
--
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]
