Re: [PR] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
Baoyuantop merged PR #12794: URL: https://github.com/apache/apisix/pull/12794 -- 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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
yixianOu commented on code in PR #12794: URL: https://github.com/apache/apisix/pull/12794#discussion_r2626149196 ## t/xrpc/pingpong.t: ## @@ -779,3 +779,49 @@ qr/connect to \S+ while prereading client data/ connect to 127.0.0.3:1995 while prereading client data connect to 127.0.0.1:1995 while prereading client data --- stream_conf_enable + + + +=== TEST 22: cleanup Review Comment: I have deleted this cleanup step. I originally added it because I thought it was the cause of the Redis test failure at that time. -- 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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
yixianOu commented on code in PR #12794: URL: https://github.com/apache/apisix/pull/12794#discussion_r2626149196 ## t/xrpc/pingpong.t: ## @@ -779,3 +779,49 @@ qr/connect to \S+ while prereading client data/ connect to 127.0.0.3:1995 while prereading client data connect to 127.0.0.1:1995 while prereading client data --- stream_conf_enable + + + +=== TEST 22: cleanup Review Comment: I have deleted this cleanup, I added it because I thought it was the cause of the redis test failure. -- 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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
yixianOu commented on code in PR #12794:
URL: https://github.com/apache/apisix/pull/12794#discussion_r2626144470
##
t/xrpc/pingpong.t:
##
@@ -510,9 +510,9 @@ call pingpong's log, ctx unfinished: false
}
}
)
-if code >= 300 then
+if code ~= 400 then
ngx.status = code
-ngx.say(body)
+ngx.say("expected 400 for invalid superior_id, got " .. code
.. ": " .. body)
Review Comment:
Before adding this validation feature, the test expected the invalid routing
rule to be added successfully.
However, after the Lua code changes, the validation logic now expects this
rule to fail to be added, so this test needs to be modified.
--
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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
Baoyuantop commented on code in PR #12794:
URL: https://github.com/apache/apisix/pull/12794#discussion_r2625851797
##
t/xrpc/pingpong.t:
##
@@ -510,9 +510,9 @@ call pingpong's log, ctx unfinished: false
}
}
)
-if code >= 300 then
+if code ~= 400 then
ngx.status = code
-ngx.say(body)
+ngx.say("expected 400 for invalid superior_id, got " .. code
.. ": " .. body)
Review Comment:
Could you explain why this part was modified?
##
t/xrpc/pingpong.t:
##
@@ -779,3 +779,49 @@ qr/connect to \S+ while prereading client data/
connect to 127.0.0.3:1995 while prereading client data
connect to 127.0.0.1:1995 while prereading client data
--- stream_conf_enable
+
+
+
+=== TEST 22: cleanup
Review Comment:
I think this cleaning is unnecessary.
--
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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
yixianOu commented on PR #12794: URL: https://github.com/apache/apisix/pull/12794#issuecomment-3654493248 - **CI log breakdown**: 1st run: TEST 20 failed, 23 passed Retry run: All 23 passed but process exited with code 1 - Could this be a flaky test issue? Likely culprits: - etcd connection timeout (saw "connection refused" in logs) - Race conditions from network latency - Test env init timing problems - The files I modified: `apisix/admin/stream_routes.lua`, `t/admin/stream-routes-subordinate.t`, `t/xrpc/pingpong.t` None of these touch Kubernetes discovery—so wouldn’t the K8s test failure be unrelated to my 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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
Baoyuantop commented on PR #12794: URL: https://github.com/apache/apisix/pull/12794#issuecomment-3654425858 Hi @yixianOu, without any logical changes, we cannot make tests pass by modifying the test code. All test-related modifications in a PR must revolve around the functionality of the PR itself; irrelevant modifications should not be included. I addressed a portion of the issues in CI at https://github.com/apache/apisix/pull/12805. Have you merged that part of the code? -- 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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
yixianOu commented on PR #12794:
URL: https://github.com/apache/apisix/pull/12794#issuecomment-3654406655
During the last CI test run, I noticed that the `jwe-decrypt.t` test failed.
The reason was that the `Authorization` field in the response headers had a
value of `"Authorization": ["hello"]`.
```
# Failed test 't/plugin/jwe-decrypt.t TEST 26: verify in upstream header
- response_body_like - response is expected ({
# "headers": { "Authorization": [ "hello" ], "Host": [ "localhost"
# ], "X-Forwarded-For": [ "127.0.0.1" ], "X-Forwarded-Host": [
# "localhost" ], "X-Forwarded-Port": [ "1984" ],
# "X-Forwarded-Proto": [ "http" ], "X-Real-Ip": [ "127.0.0.1" ] }
# })'
# at /usr/local/share/perl/5.38.2/Test/Nginx/Socket.pm line 1706.
# '{
# "headers": {
# "Authorization": [
# "hello"
# ],
# "Host": [
# "localhost"
# ],
# "X-Forwarded-For": [
# "127.0.0.1"
# ],
# "X-Forwarded-Host": [
# "localhost"
# ],
# "X-Forwarded-Port": [
# "1984"
# ],
# "X-Forwarded-Proto": [
# "http"
# ],
# "X-Real-Ip": [
# "127.0.0.1"
# ]
# }
# }
# '
# doesn't match '(?^s:.*"Authorization": "hello".*
# )'
# Looks like you failed 2 tests of 154.
# [08:26:06] t/plugin/jwe-decrypt.t ..
# Dubious, test returned 2 (wstat 512, 0x200)
# Failed 2/154 subtests
```
Therefore, I modified the expected response in `jwe-decrypt.t` to
`.*"Authorization":\s*\[\s*"hello"\s*\].*`.
This change allowed the test to pass successfully on my local machine:
```
╭───╮
│ [08:16:40] t/plugin/jwe-decrypt.t .. ok12507 ms ( 0.03 usr 0.00 sys +
1.91 │
│ cusr 0.63 csys = 2.57 CPU)
│
│ [08:16:40]
│
│ All tests successful.
│
│ Files=1, Tests=154, 13 wallclock secs ( 0.06 usr 0.01 sys + 1.91 cusr
0.63 │
│ csys = 2.61 CPU)
│
│ Result: PASS
│
│
│
╰───╯
```
However, in today's CI test run, `jwe-decrypt.t` failed again. This time,
the `Authorization` field in the response headers was `"Authorization":
"hello"`.
Should we revert the expected response back to `.*"Authorization":
"hello".*`?
@Baoyuantop
--
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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
yixianOu commented on PR #12794:
URL: https://github.com/apache/apisix/pull/12794#issuecomment-3649202506
1) jwe-decrypt.t
- Change: Updated TEST 26 assertion to match upstream Authorization header
when echoed as an array, i.e., allowing "Authorization": ["hello"].
- Reason: httpbin’s /headers returns headers as arrays; the previous
string-only regex caused false test failures. This makes the test compatible
with the actual upstream response format.
2) stream-routes-subordinate.t
- Change: Switched failure checks to decode the admin API JSON and assert on
the error_msg field (e.g., “failed to fetch stream routes[999]”, “protocol
mismatch”, “can not delete this stream route”).
- Reason: Using structured error_msg ensures precise, stable validation,
avoiding brittle raw-string matching and aligning tests with the API’s
canonical error payload format.
3) ai-request-rewrite2.t
- Change: removed manual cleanup steps embedded in the tests.
- Reason: Reduce flakiness and dependency on manual teardown.
CI test failure occurred in the job:
`build (ubuntu-latest, linux_apisix_current_luarocks_in_customed_nginx)`
Error log details:
```txt
Error: 2025/12/12 06:10:14 [error] 95223#95223: *1 [lua]
config_etcd.lua:602: load_full_data(): failed to check item data of
[/apisix/routes] err:object matches none of the required: ["plugins","uri"] or
["upstream","uri"] or ["upstream_id","uri"] or ["service_id","uri"] or
["plugins","uris"] or ["upstream","uris"] or ["upstream_id","uris"] or
["service_id","uris"] or ["script","uri"] or ["script","uris"] ,val:
{"priority":0,"status":1,"uri":"/2"}, context: init_worker_by_lua*
Root cause analysis:
The failure is triggered by an invalid route object persisted in etcd:
{"priority":0,"status":1,"uri":"/2"}.
This route object fails schema validation because it lacks mandatory field
combinations — per the routing schema requirements, at least one of [plugins,
upstream, upstream_id, service_id, script] must be paired with either uri or
uris.
The schema check failure in load_full_data() throws the "object matches none
of the required" error, causing the worker initialization process to terminate
immediately and resulting in CI pipeline failure.
But I can't find where this dirty record come from.
--
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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
Baoyuantop commented on code in PR #12794:
URL: https://github.com/apache/apisix/pull/12794#discussion_r2613133645
##
t/plugin/ai-request-rewrite2.t:
##
@@ -286,3 +286,24 @@ passed
qr/missing request body/
--- response_body
passed
+
+
+
+=== TEST 5: cleanup
Review Comment:
We fixed some unstable tests in https://github.com/apache/apisix/pull/12805,
and I believe this code was unnecessary.
##
t/admin/stream-routes-subordinate.t:
##
@@ -0,0 +1,260 @@
+#
+# 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);
+no_long_string();
+no_root_location();
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+if (!$block->extra_yaml_config) {
+my $extra_yaml_config = <<_EOC_;
+xrpc:
+ protocols:
+- name: redis
+- name: dubbo
+_EOC_
+$block->set_value("extra_yaml_config", $extra_yaml_config);
+}
+
+$block;
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: create superior route
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/stream_routes/1',
+ngx.HTTP_PUT,
+[[{
+"protocol": {"name": "redis"},
+"upstream": {
+"nodes": {"127.0.0.1:6379": 1},
+"type": "roundrobin"
+}
+}]]
+)
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+--- request
+GET /t
+--- response_body
+passed
+
+
+
+=== TEST 2: create subordinate route with valid superior_id
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/stream_routes/2',
+ngx.HTTP_PUT,
+[[{
+"protocol": {
+"name": "redis",
+"superior_id": "1"
+},
+"upstream": {
+"nodes": {"127.0.0.1:6380": 1},
+"type": "roundrobin"
+}
+}]]
+)
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+--- request
+GET /t
+--- response_body
+passed
+
+
+
+=== TEST 3: superior_id not exist (should fail)
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/stream_routes/3',
+ngx.HTTP_PUT,
+[[{
+"protocol": {"name": "redis", "superior_id": "999"},
+"upstream": {
+"nodes": {"127.0.0.1:6381": 1},
+"type": "roundrobin"
+}
+}]]
+)
+if code ~= 400 then
+ngx.say("failed: expected 400, got ", code)
+return
+end
+if not body or not string.find(body, "failed to fetch stream
routes[999]", 1, true) then
+ngx.say("failed: unexpected body: ", body)
+return
+end
+ngx.say("passed")
+}
+}
+--- request
+GET /t
+--- response_body
+passed
+
+
+
+=== TEST 4: protocol mismatch (should fail)
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code = t('/apisix/admin/stream_routes/4',
+ngx.HTTP_PUT,
+[[{
+"protocol": {"name": "dubbo"},
+"upstream": {
+"nodes": {"127.0.0.1:20880": 1},
+"type": "roundrobin"
+}
+}]]
+)
+
+local code, body = t('/apisix/admin/stream_routes/5',
+ngx.HTTP_PUT,
+[[{
+"protocol": {"name": "redis", "superior_id": "4"},
+"upstream": {
+
Re: [PR] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
yixianOu commented on PR #12794: URL: https://github.com/apache/apisix/pull/12794#issuecomment-3632241519 `httpbin.org:80 ` Is this external service reliable? My local development container performs tests, this service sometimes times out, sometimes returns 503, sometimes it can be accessed normally and passes the test, I don't know if this is my local network problem or this external service problem. -- 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] feat: Add dependency protocol checking and deletion checking for stream routing [apisix]
yixianOu commented on PR #12794: URL: https://github.com/apache/apisix/pull/12794#issuecomment-3627109230 Modified `t/xrpc/pingpong.t` to expect a 400 error code when creating the route with `superior_id = 1`. This aligns the test with the new validation logic. Added cleanup test to avoid residual configuration in etcd affecting subsequent redis tests. For mcp-bridge testing, I don't know how to fix 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]
