You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "nic-6443 (via GitHub)" <gi...@apache.org> on 2026/04/23 09:25:27 UTC
[PR] fix(stream/traffic-split): handle upstream_id set by plugin in stream context [apisix]
nic-6443 opened a new pull request, #13285:
URL: https://github.com/apache/apisix/pull/13285
When the traffic-split plugin selects an upstream via `upstream_id`, it sets `api_ctx.upstream_id`. In the HTTP path this is handled in `handle_upstream()` which re-fetches the upstream and updates `api_ctx.matched_upstream`. The stream path was missing this handling.
`set_by_route()` only checks `api_ctx.upstream_conf` (set when using inline upstream objects), but doesn't handle `api_ctx.upstream_id`. So even when the plugin selected a different upstream by ID, the route's original upstream was still used.
The fix adds a check after `plugin.run_plugin("preread")`: if `api_ctx.upstream_id` is set, fetch that upstream and update `api_ctx.matched_upstream` before calling `set_upstream()`.
Also adds a test case that configures a stream route with a traffic-split rule using `upstream_id` and verifies traffic is correctly forwarded to the referenced upstream.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(stream/traffic-split): handle upstream_id set by plugin in stream context [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on PR #13285:
URL: https://github.com/apache/apisix/pull/13285#issuecomment-4304128686
Fixed the nginx.conf parse error. The issue was that TEST 5 had both `--- config` (custom HTTP location for admin setup) and `--- stream_enable`. The test framework's preprocessors run in LIFO order, so Stream.pm runs last — it sees `stream_config` is defined (set by APISIX.pm) and appends a `location = /t { sock:connect(...) }` block after APISIX.pm's already large server config, producing a malformed nginx.conf. Since TEST 5 is just making HTTP admin API calls (create/delete routes), it doesn't actually need the stream server at all — removing `--- stream_enable` from that test fixes the issue.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(stream/traffic-split): handle upstream_id set by plugin in stream context [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #13285:
URL: https://github.com/apache/apisix/pull/13285#discussion_r3129801706
##########
apisix/init.lua:
##########
@@ -1347,6 +1347,15 @@ function _M.stream_preread_phase()
plugin.run_plugin("preread", plugins, api_ctx)
+ if api_ctx.upstream_id then
+ local new_upstream = apisix_upstream.get_by_id(api_ctx.upstream_id)
+ if not new_upstream then
+ core.log.error("failed to get upstream by upstream_id: ", api_ctx.upstream_id)
Review Comment:
Good point — `get_by_id()` already logs the error at line 552 (`core.log.error("failed to find upstream by id: ", up_id)`). Removed the duplicate log in the latest commit.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(stream/traffic-split): handle upstream_id set by plugin in stream context [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #13285:
URL: https://github.com/apache/apisix/pull/13285#discussion_r3131264145
##########
apisix/init.lua:
##########
@@ -1347,6 +1347,15 @@ function _M.stream_preread_phase()
plugin.run_plugin("preread", plugins, api_ctx)
+ if api_ctx.upstream_id then
+ local new_upstream = apisix_upstream.get_by_id(api_ctx.upstream_id)
+ if not new_upstream then
+ core.log.error("failed to get upstream by upstream_id: ", api_ctx.upstream_id)
Review Comment:
Already addressed — the duplicate `core.log.error` was removed in the previous commit. The `get_by_id()` function at line 552 already logs the error, so no additional logging needed here.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(stream/traffic-split): handle upstream_id set by plugin in stream context [apisix]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13285:
URL: https://github.com/apache/apisix/pull/13285#discussion_r3129758346
##########
apisix/init.lua:
##########
@@ -1347,6 +1347,15 @@ function _M.stream_preread_phase()
plugin.run_plugin("preread", plugins, api_ctx)
+ if api_ctx.upstream_id then
+ local new_upstream = apisix_upstream.get_by_id(api_ctx.upstream_id)
+ if not new_upstream then
+ core.log.error("failed to get upstream by upstream_id: ", api_ctx.upstream_id)
Review Comment:
`apisix_upstream.get_by_id()` already logs an error when the upstream is missing. Logging again here will produce duplicate error entries for the same failure; consider removing this extra `core.log.error(...)` (or change it to `core.log.debug` if you need additional context) and rely on `get_by_id`'s log.
```suggestion
```
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(stream/traffic-split): handle upstream_id set by plugin in stream context [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 merged PR #13285:
URL: https://github.com/apache/apisix/pull/13285
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(stream/traffic-split): handle upstream_id set by plugin in stream context [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on PR #13285:
URL: https://github.com/apache/apisix/pull/13285#issuecomment-4303631193
Applied same fix as #13284 TEST 5: added `--- stream_enable` to the route setup test to avoid stream→HTTP nginx restart after TEST 4's 10 concurrent connections.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org