Copilot commented on code in PR #12826:
URL: https://github.com/apache/apisix/pull/12826#discussion_r2901487672
##########
apisix/http/service.lua:
##########
@@ -52,6 +52,19 @@ local function filter(service)
apisix_upstream.filter_upstream(service.value.upstream, service)
+ if type(pre_service_or_size) == "number" or not obj then
+ return
+ end
+
Review Comment:
`filter()` returns immediately for full reloads (`type(pre_service_or_size)
== "number"`). With incremental routing, service changes can affect computed
route hosts (routes using `service_id` inherit `service.hosts` during router
build). If services are reloaded via `need_reload` (compaction/restart),
`service_version` will change but `need_create_radixtree` will remain false, so
the router may not be rebuilt and can keep stale service-host-derived matching.
Consider marking `apisix.router.need_create_radixtree = true` on service full
reload (and/or providing a reliable end-of-reload signal similar to routes).
```suggestion
if type(pre_service_or_size) == "number" then
-- full reload of services; router may need rebuild because routes
-- inheriting service.hosts can be affected
local ar = require("apisix.router")
ar.need_create_radixtree = true
core.log.info("full services reload, rebuild radixtree")
return
end
if not obj then
return
end
```
##########
apisix/router.lua:
##########
@@ -17,33 +17,130 @@
local require = require
local http_route = require("apisix.http.route")
local apisix_upstream = require("apisix.upstream")
-local core = require("apisix.core")
+local core = require("apisix.core")
local set_plugins_meta_parent =
require("apisix.plugin").set_plugins_meta_parent
local str_lower = string.lower
-local ipairs = ipairs
+local ipairs = ipairs
+local process = require("ngx.process")
+
local _M = {version = 0.3}
-local function filter(route)
+_M.need_create_radixtree = true
+
+
+local function sync_tb_create(sync_tb, route)
+ if not sync_tb[route.value.id] then
+ sync_tb[route.value.id] = {op = "create", cur_route = route}
+ elseif sync_tb[route.value.id]["op"] == "delete" then
+ sync_tb[route.value.id] = {op = "update", cur_route = route,
+ last_route =
sync_tb[route.value.id]["last_route"]}
+ end
+end
+
+
+local function sync_tb_delete(sync_tb, route)
+ local key = route.value.id
+ if not sync_tb[key] then
+ sync_tb[key] = {op = "delete", last_route = route}
+ elseif sync_tb[key]["op"] == "create" then
+ sync_tb[key] = nil
+ elseif sync_tb[key]["op"] == "update" then
+ sync_tb[key] = {op = "delete", last_route = sync_tb[key]["last_route"]}
+ end
+end
+
+
+local function sync_tb_update(sync_tb, pre_route, route)
+
+ if not sync_tb[route.value.id] then
+ sync_tb[route.value.id] = {op = "update", last_route = pre_route,
cur_route = route}
+ elseif sync_tb[route.value.id]["op"] == "update" then
+ sync_tb[route.value.id] = {
+ op = "update",
+ last_route = sync_tb[route.value.id]["last_route"],
+ cur_route = route
+ }
+ elseif sync_tb[route.value.id]["op"] == "create" then
+ sync_tb[route.value.id] = {op = "create", cur_route = route}
+ end
+end
+
+
+local function filter(route, pre_route_or_size, obj)
route.orig_modifiedIndex = route.modifiedIndex
route.has_domain = false
- if not route.value then
+
+ if route.value then
+ set_plugins_meta_parent(route.value.plugins, route)
+ if route.value.host then
+ route.value.host = str_lower(route.value.host)
+ elseif route.value.hosts then
+ for i, v in ipairs(route.value.hosts) do
+ route.value.hosts[i] = str_lower(v)
+ end
+ end
+
+ apisix_upstream.filter_upstream(route.value.upstream, route)
+ end
+
+ if not obj then
+ return
+ end
+ --save sync route and operation type into a map
+ if type(pre_route_or_size) == "number" then
+ if pre_route_or_size == #obj.values then
+ _M.need_create_radixtree = true
+ end
+ return
+ end
+
+ -- to not store changes in privileged agent
+ if process.type() == "privileged agent" then
return
end
- set_plugins_meta_parent(route.value.plugins, route)
+ local sync_tb = _M.sync_tb
+ if pre_route_or_size then
+ if route.value then
+ --update route
+ core.log.notice("update routes watched from etcd into radixtree. ",
+ core.json.delay_encode(route, true))
Review Comment:
These `core.log.notice(... delay_encode(route, true))` statements will emit
the full route object (including plugins) at NOTICE level on every watched
change. For frequent route updates this can significantly increase log
volume/CPU and may expose sensitive plugin config in logs. Consider lowering to
`debug` or logging only the route id/op (and maybe key fields) instead of the
full JSON.
##########
apisix/http/router/radixtree_uri.lua:
##########
@@ -27,14 +29,122 @@ local _M = {version = 0.2}
local uri_routes = {}
local uri_router
+function _M.incremental_operate_radixtree(router, routes, with_parameter)
+ local op, route, last_route, err
+ local cur_tmp, last_tmp
+ local router_opts = {
+ no_param_match = with_parameter ~= true
+ }
+
+ event.push(event.CONST.BUILD_ROUTER, routes)
+ for k, v in pairs(ar.sync_tb) do
+ op = ar.sync_tb[k]["op"]
+ route = ar.sync_tb[k]["cur_route"]
+ last_route = ar.sync_tb[k]["last_route"]
+ cur_tmp = {}
+ last_tmp = {}
+
+ if route and route.value then
+ local status = core.table.try_read_attr(route, "value", "status")
+ if status and status == 0 then
+ goto CONTINUE
+ end
+
+ local filter_fun, err
+ if route.value.filter_func then
+ filter_fun, err = loadstring(
+ "return " .. route.value.filter_func,
+ "router#" .. route.value.id
+ )
+ if not filter_fun then
+ core.log.error("failed to load filter function: ", err,
+ " route id", route.value.id)
+ return
Review Comment:
On `filter_func` load failure, the code logs and `return`s from
`incremental_operate_radixtree`, aborting all remaining pending sync operations
and leaving `ar.sync_tb` uncleared. The full rebuild path skips only the bad
route; incremental should similarly skip/mark-rebuild and still clear the
current sync entry to avoid getting stuck on the same failure forever.
```suggestion
goto CONTINUE
```
##########
apisix/http/router/radixtree_uri.lua:
##########
@@ -27,14 +29,122 @@ local _M = {version = 0.2}
local uri_routes = {}
local uri_router
+function _M.incremental_operate_radixtree(router, routes, with_parameter)
+ local op, route, last_route, err
+ local cur_tmp, last_tmp
+ local router_opts = {
+ no_param_match = with_parameter ~= true
+ }
+
+ event.push(event.CONST.BUILD_ROUTER, routes)
Review Comment:
The incremental router update logic in this module is a behavior change
(create/update/delete without full rebuild) but there are no tests covering it.
Please add Test::Nginx cases that exercise route create/update/delete via the
Admin API and verify correct matching (including host matching via
`service_id`, status enable/disable, and parameterized routes for the
`with_parameter` variant).
##########
apisix/router.lua:
##########
@@ -17,33 +17,130 @@
local require = require
local http_route = require("apisix.http.route")
local apisix_upstream = require("apisix.upstream")
-local core = require("apisix.core")
+local core = require("apisix.core")
local set_plugins_meta_parent =
require("apisix.plugin").set_plugins_meta_parent
local str_lower = string.lower
-local ipairs = ipairs
+local ipairs = ipairs
+local process = require("ngx.process")
+
local _M = {version = 0.3}
-local function filter(route)
+_M.need_create_radixtree = true
+
+
+local function sync_tb_create(sync_tb, route)
+ if not sync_tb[route.value.id] then
+ sync_tb[route.value.id] = {op = "create", cur_route = route}
+ elseif sync_tb[route.value.id]["op"] == "delete" then
+ sync_tb[route.value.id] = {op = "update", cur_route = route,
+ last_route =
sync_tb[route.value.id]["last_route"]}
+ end
+end
+
+
+local function sync_tb_delete(sync_tb, route)
+ local key = route.value.id
+ if not sync_tb[key] then
+ sync_tb[key] = {op = "delete", last_route = route}
+ elseif sync_tb[key]["op"] == "create" then
+ sync_tb[key] = nil
+ elseif sync_tb[key]["op"] == "update" then
+ sync_tb[key] = {op = "delete", last_route = sync_tb[key]["last_route"]}
+ end
+end
+
+
+local function sync_tb_update(sync_tb, pre_route, route)
+
+ if not sync_tb[route.value.id] then
+ sync_tb[route.value.id] = {op = "update", last_route = pre_route,
cur_route = route}
+ elseif sync_tb[route.value.id]["op"] == "update" then
+ sync_tb[route.value.id] = {
+ op = "update",
+ last_route = sync_tb[route.value.id]["last_route"],
+ cur_route = route
+ }
+ elseif sync_tb[route.value.id]["op"] == "create" then
+ sync_tb[route.value.id] = {op = "create", cur_route = route}
+ end
+end
+
+
+local function filter(route, pre_route_or_size, obj)
route.orig_modifiedIndex = route.modifiedIndex
route.has_domain = false
- if not route.value then
+
+ if route.value then
+ set_plugins_meta_parent(route.value.plugins, route)
+ if route.value.host then
+ route.value.host = str_lower(route.value.host)
+ elseif route.value.hosts then
+ for i, v in ipairs(route.value.hosts) do
+ route.value.hosts[i] = str_lower(v)
+ end
+ end
+
+ apisix_upstream.filter_upstream(route.value.upstream, route)
+ end
+
+ if not obj then
+ return
+ end
+ --save sync route and operation type into a map
+ if type(pre_route_or_size) == "number" then
+ if pre_route_or_size == #obj.values then
+ _M.need_create_radixtree = true
+ end
Review Comment:
`load_full_data()` passes a numeric second arg (`#values`) into this filter.
The current `pre_route_or_size == #obj.values` check can fail when some items
are invalid and skipped (so `#obj.values < #values`), meaning
`need_create_radixtree` may never be set on a full reload. That can leave the
worker using a stale router after `need_reload` reloads config. Consider
setting `need_create_radixtree` unconditionally on full reload (or using a more
reliable end-of-reload signal than comparing counts).
```suggestion
-- full data reload: always rebuild radixtree after load_full_data,
-- since some routes may be invalid and skipped, making #obj.values
-- smaller than the original size passed in as pre_route_or_size
_M.need_create_radixtree = true
```
##########
apisix/http/router/radixtree_uri.lua:
##########
@@ -27,14 +29,122 @@ local _M = {version = 0.2}
local uri_routes = {}
local uri_router
+function _M.incremental_operate_radixtree(router, routes, with_parameter)
+ local op, route, last_route, err
+ local cur_tmp, last_tmp
+ local router_opts = {
+ no_param_match = with_parameter ~= true
+ }
+
+ event.push(event.CONST.BUILD_ROUTER, routes)
+ for k, v in pairs(ar.sync_tb) do
+ op = ar.sync_tb[k]["op"]
+ route = ar.sync_tb[k]["cur_route"]
+ last_route = ar.sync_tb[k]["last_route"]
+ cur_tmp = {}
+ last_tmp = {}
+
+ if route and route.value then
+ local status = core.table.try_read_attr(route, "value", "status")
+ if status and status == 0 then
Review Comment:
In `incremental_operate_radixtree`, when a route is disabled (`status == 0`)
the code does `goto CONTINUE`, but the `::CONTINUE::` label is *after*
`ar.sync_tb[k] = nil`. This leaves the sync entry in `ar.sync_tb` forever, so
every subsequent request will keep re-processing (and skipping) the same entry.
Clear the entry before jumping (or move the cleanup before the label).
```suggestion
if status and status == 0 then
-- clear sync entry for disabled route before skipping
further processing
ar.sync_tb[k] = nil
```
##########
apisix/http/router/radixtree_uri.lua:
##########
@@ -27,14 +29,122 @@ local _M = {version = 0.2}
local uri_routes = {}
local uri_router
+function _M.incremental_operate_radixtree(router, routes, with_parameter)
+ local op, route, last_route, err
+ local cur_tmp, last_tmp
+ local router_opts = {
+ no_param_match = with_parameter ~= true
+ }
+
+ event.push(event.CONST.BUILD_ROUTER, routes)
+ for k, v in pairs(ar.sync_tb) do
+ op = ar.sync_tb[k]["op"]
+ route = ar.sync_tb[k]["cur_route"]
+ last_route = ar.sync_tb[k]["last_route"]
+ cur_tmp = {}
+ last_tmp = {}
+
+ if route and route.value then
+ local status = core.table.try_read_attr(route, "value", "status")
+ if status and status == 0 then
+ goto CONTINUE
+ end
+
+ local filter_fun, err
+ if route.value.filter_func then
+ filter_fun, err = loadstring(
+ "return " .. route.value.filter_func,
+ "router#" .. route.value.id
+ )
+ if not filter_fun then
+ core.log.error("failed to load filter function: ", err,
+ " route id", route.value.id)
+ return
+ end
+
+ filter_fun = filter_fun()
+ end
+
+ cur_tmp = {
+ id = route.value.id,
+ paths = route.value.uris or route.value.uri,
+ methods = route.value.methods,
+ priority = route.value.priority,
+ hosts = route.value.hosts or route.value.host,
+ remote_addrs = route.value.remote_addrs or
route.value.remote_addr,
Review Comment:
Incremental route building sets `hosts = route.value.hosts or
route.value.host`, but the full router build (`create_radixtree_uri_router`)
also derives hosts from the referenced `service_id` when the route itself has
no host/hosts. Without matching that logic here, incremental
create/update/delete can change host-matching semantics (and may fail to find
the existing route to update/delete). Compute `hosts` the same way as the full
build (including `service_fetch(service_id)` fallback).
##########
apisix/router.lua:
##########
@@ -17,33 +17,130 @@
local require = require
local http_route = require("apisix.http.route")
local apisix_upstream = require("apisix.upstream")
-local core = require("apisix.core")
+local core = require("apisix.core")
local set_plugins_meta_parent =
require("apisix.plugin").set_plugins_meta_parent
local str_lower = string.lower
-local ipairs = ipairs
+local ipairs = ipairs
+local process = require("ngx.process")
+
local _M = {version = 0.3}
-local function filter(route)
+_M.need_create_radixtree = true
+
+
+local function sync_tb_create(sync_tb, route)
+ if not sync_tb[route.value.id] then
+ sync_tb[route.value.id] = {op = "create", cur_route = route}
+ elseif sync_tb[route.value.id]["op"] == "delete" then
+ sync_tb[route.value.id] = {op = "update", cur_route = route,
+ last_route =
sync_tb[route.value.id]["last_route"]}
+ end
+end
+
+
+local function sync_tb_delete(sync_tb, route)
+ local key = route.value.id
+ if not sync_tb[key] then
+ sync_tb[key] = {op = "delete", last_route = route}
+ elseif sync_tb[key]["op"] == "create" then
+ sync_tb[key] = nil
+ elseif sync_tb[key]["op"] == "update" then
+ sync_tb[key] = {op = "delete", last_route = sync_tb[key]["last_route"]}
+ end
+end
+
+
+local function sync_tb_update(sync_tb, pre_route, route)
+
+ if not sync_tb[route.value.id] then
+ sync_tb[route.value.id] = {op = "update", last_route = pre_route,
cur_route = route}
+ elseif sync_tb[route.value.id]["op"] == "update" then
+ sync_tb[route.value.id] = {
+ op = "update",
+ last_route = sync_tb[route.value.id]["last_route"],
+ cur_route = route
+ }
+ elseif sync_tb[route.value.id]["op"] == "create" then
+ sync_tb[route.value.id] = {op = "create", cur_route = route}
+ end
+end
+
+
+local function filter(route, pre_route_or_size, obj)
route.orig_modifiedIndex = route.modifiedIndex
route.has_domain = false
- if not route.value then
+
+ if route.value then
+ set_plugins_meta_parent(route.value.plugins, route)
+ if route.value.host then
+ route.value.host = str_lower(route.value.host)
+ elseif route.value.hosts then
+ for i, v in ipairs(route.value.hosts) do
+ route.value.hosts[i] = str_lower(v)
+ end
+ end
+
+ apisix_upstream.filter_upstream(route.value.upstream, route)
+ end
+
+ if not obj then
+ return
+ end
+ --save sync route and operation type into a map
+ if type(pre_route_or_size) == "number" then
+ if pre_route_or_size == #obj.values then
+ _M.need_create_radixtree = true
+ end
+ return
+ end
+
+ -- to not store changes in privileged agent
+ if process.type() == "privileged agent" then
return
end
- set_plugins_meta_parent(route.value.plugins, route)
+ local sync_tb = _M.sync_tb
+ if pre_route_or_size then
+ if route.value then
+ --update route
+ core.log.notice("update routes watched from etcd into radixtree. ",
+ core.json.delay_encode(route, true))
+ local pre_status = pre_route_or_size.value.status
+ local status = route.value.status
+
+ -- sync according status
+ if pre_status == 0 and status == 1 then
+ sync_tb_create(sync_tb, route)
+ elseif pre_status == 1 and status == 0 then
+ sync_tb_delete(sync_tb, pre_route_or_size)
+ elseif pre_status == 1 and status == 1 then
+ sync_tb_update(sync_tb, pre_route_or_size, route)
Review Comment:
The sync logic treats `status` as always `0/1`, but APISIX historically
treats missing `status` as enabled (many existing objects omit it, and schema
validation does not apply defaults). Here, a nil `status` won't match any
branch and may prevent create/update/delete from being recorded (and create
uses `if route.value.status == 1` only). Normalize missing `status` to `1` (for
both current and previous route) before computing the operation.
--
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]
