Copilot commented on code in PR #13220: URL: https://github.com/apache/apisix/pull/13220#discussion_r3076950830
########## apisix/admin/config_validate.lua: ########## @@ -0,0 +1,237 @@ +-- +-- 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. +-- + +--- Batch configuration validation module. +-- Validates APISIX declarative configurations (routes, services, consumers, etc.) +-- including resource-level JSON Schema and plugin check_schema() advanced validation. +-- Used by both standalone mode and etcd mode via POST /apisix/admin/configs/validate. + +local type = type +local pairs = pairs +local ipairs = ipairs +local tostring = tostring +local pcall = pcall +local str_find = string.find +local str_sub = string.sub +local table_insert = table.insert +local yaml = require("lyaml") +local core = require("apisix.core") +local tbl_deepcopy = require("apisix.core.table").deepcopy +local constants = require("apisix.constants") + +local _M = {} + +local resources = { + routes = require("apisix.admin.routes"), + services = require("apisix.admin.services"), + upstreams = require("apisix.admin.upstreams"), + consumers = require("apisix.admin.consumers"), + credentials = require("apisix.admin.credentials"), + schema = require("apisix.admin.schema"), + ssls = require("apisix.admin.ssl"), + plugins = require("apisix.admin.plugins"), + protos = require("apisix.admin.proto"), + global_rules = require("apisix.admin.global_rules"), + stream_routes = require("apisix.admin.stream_routes"), + plugin_metadata = require("apisix.admin.plugin_metadata"), + plugin_configs = require("apisix.admin.plugin_config"), + consumer_groups = require("apisix.admin.consumer_group"), + secrets = require("apisix.admin.secrets"), +} + +local CONF_VERSION_KEY_SUFFIX = "_conf_version" +local ALL_RESOURCE_KEYS = {} +for dir in pairs(constants.HTTP_ETCD_DIRECTORY) do + local key = str_sub(dir, 2) + ALL_RESOURCE_KEYS[key] = key .. CONF_VERSION_KEY_SUFFIX +end +for dir in pairs(constants.STREAM_ETCD_DIRECTORY) do + local key = str_sub(dir, 2) + ALL_RESOURCE_KEYS[key] = key .. CONF_VERSION_KEY_SUFFIX +end + + +local function check_duplicate(item, key, id_set) + local identifier, identifier_type + if key == "consumers" then + identifier = item.id or item.username + identifier_type = item.id and "credential id" or "username" + else + identifier = item.id + identifier_type = "id" + end + + if id_set[identifier] then + return true, "found duplicate " .. identifier_type .. " " .. identifier .. " in " .. key + end + id_set[identifier] = true + return false Review Comment: `check_duplicate()` indexes `id_set[identifier]` where `identifier` can be `nil` for resource types whose schema doesn't require `id` (e.g., routes/services in declarative config). In Lua this raises `table index is nil`, turning an otherwise-valid payload into a 500. Please guard against missing identifiers (skip duplicate checking when there is no stable key, or derive one per resource type) so validation always returns a 4xx with a useful error instead of crashing. ########## apisix/admin/config_validate.lua: ########## @@ -0,0 +1,237 @@ +-- +-- 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. +-- + +--- Batch configuration validation module. +-- Validates APISIX declarative configurations (routes, services, consumers, etc.) +-- including resource-level JSON Schema and plugin check_schema() advanced validation. +-- Used by both standalone mode and etcd mode via POST /apisix/admin/configs/validate. + +local type = type +local pairs = pairs +local ipairs = ipairs +local tostring = tostring +local pcall = pcall +local str_find = string.find +local str_sub = string.sub +local table_insert = table.insert +local yaml = require("lyaml") +local core = require("apisix.core") +local tbl_deepcopy = require("apisix.core.table").deepcopy +local constants = require("apisix.constants") + +local _M = {} + +local resources = { + routes = require("apisix.admin.routes"), + services = require("apisix.admin.services"), + upstreams = require("apisix.admin.upstreams"), + consumers = require("apisix.admin.consumers"), + credentials = require("apisix.admin.credentials"), + schema = require("apisix.admin.schema"), + ssls = require("apisix.admin.ssl"), + plugins = require("apisix.admin.plugins"), + protos = require("apisix.admin.proto"), + global_rules = require("apisix.admin.global_rules"), + stream_routes = require("apisix.admin.stream_routes"), + plugin_metadata = require("apisix.admin.plugin_metadata"), + plugin_configs = require("apisix.admin.plugin_config"), + consumer_groups = require("apisix.admin.consumer_group"), + secrets = require("apisix.admin.secrets"), +} + +local CONF_VERSION_KEY_SUFFIX = "_conf_version" +local ALL_RESOURCE_KEYS = {} +for dir in pairs(constants.HTTP_ETCD_DIRECTORY) do + local key = str_sub(dir, 2) + ALL_RESOURCE_KEYS[key] = key .. CONF_VERSION_KEY_SUFFIX +end +for dir in pairs(constants.STREAM_ETCD_DIRECTORY) do + local key = str_sub(dir, 2) + ALL_RESOURCE_KEYS[key] = key .. CONF_VERSION_KEY_SUFFIX +end + + +local function check_duplicate(item, key, id_set) + local identifier, identifier_type + if key == "consumers" then + identifier = item.id or item.username + identifier_type = item.id and "credential id" or "username" + else + identifier = item.id + identifier_type = "id" + end + + if id_set[identifier] then + return true, "found duplicate " .. identifier_type .. " " .. identifier .. " in " .. key + end + id_set[identifier] = true + return false +end + + +local function check_conf(checker, schema, item, typ) + if not checker then + return true + end + local str_id = tostring(item.id) + if typ == "consumers" and + core.string.find(str_id, "/credentials/") then + local credential_checker = resources.credentials.checker + local credential_schema = resources.credentials.schema + return credential_checker(item.id, item, false, credential_schema, { + skip_references_check = true, + }) + end + + local secret_type + if typ == "secrets" then + local idx = str_find(str_id or "", "/") + if not idx then + return false, { + error_msg = "invalid secret id: " .. (str_id or "") + } + end + secret_type = str_sub(str_id, 1, idx - 1) + end + return checker(item.id, item, false, schema, { + secret_type = secret_type, + skip_references_check = true, + }) +end + + +function _M.validate_configuration(req_body, collect_all_errors) + local is_valid = true + local validation_results = {} + + for key, conf_version_key in pairs(ALL_RESOURCE_KEYS) do + local items = req_body[key] + local resource = resources[key] or {} + + -- Validate conf_version_key if present + local new_conf_version = req_body[conf_version_key] + if new_conf_version and type(new_conf_version) ~= "number" then + if not collect_all_errors then + return false, conf_version_key .. " must be a number" + end + is_valid = false + table_insert(validation_results, { + resource_type = key, + error = conf_version_key .. " must be a number, got " .. type(new_conf_version) + }) + end + + if items and #items > 0 then + local item_schema = resource.schema + local item_checker = resource.checker + local id_set = {} + + for index, item in ipairs(items) do + local item_temp = tbl_deepcopy(item) + local valid, err = check_conf(item_checker, item_schema, item_temp, key) Review Comment: The `plugins` section in declarative config uses `name` (see `core.schema.plugins`) and `apisix.admin.plugins` doesn't provide a per-item `schema/checker`. As written, `plugins` entries won't be schema-validated, and duplicate checking falls back to `item.id` (nil) which can trigger the `table index is nil` crash. Consider special-casing `key == "plugins"` to validate the whole array against `core.schema.plugins` and use `item.name` for duplicate detection/error messages. ########## apisix/admin/config_validate.lua: ########## @@ -0,0 +1,237 @@ +-- +-- 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. +-- + +--- Batch configuration validation module. +-- Validates APISIX declarative configurations (routes, services, consumers, etc.) +-- including resource-level JSON Schema and plugin check_schema() advanced validation. +-- Used by both standalone mode and etcd mode via POST /apisix/admin/configs/validate. + +local type = type +local pairs = pairs +local ipairs = ipairs +local tostring = tostring +local pcall = pcall +local str_find = string.find +local str_sub = string.sub +local table_insert = table.insert +local yaml = require("lyaml") +local core = require("apisix.core") +local tbl_deepcopy = require("apisix.core.table").deepcopy +local constants = require("apisix.constants") + +local _M = {} + +local resources = { + routes = require("apisix.admin.routes"), + services = require("apisix.admin.services"), + upstreams = require("apisix.admin.upstreams"), + consumers = require("apisix.admin.consumers"), + credentials = require("apisix.admin.credentials"), + schema = require("apisix.admin.schema"), + ssls = require("apisix.admin.ssl"), + plugins = require("apisix.admin.plugins"), + protos = require("apisix.admin.proto"), + global_rules = require("apisix.admin.global_rules"), + stream_routes = require("apisix.admin.stream_routes"), + plugin_metadata = require("apisix.admin.plugin_metadata"), + plugin_configs = require("apisix.admin.plugin_config"), + consumer_groups = require("apisix.admin.consumer_group"), + secrets = require("apisix.admin.secrets"), +} + +local CONF_VERSION_KEY_SUFFIX = "_conf_version" +local ALL_RESOURCE_KEYS = {} +for dir in pairs(constants.HTTP_ETCD_DIRECTORY) do + local key = str_sub(dir, 2) + ALL_RESOURCE_KEYS[key] = key .. CONF_VERSION_KEY_SUFFIX +end +for dir in pairs(constants.STREAM_ETCD_DIRECTORY) do + local key = str_sub(dir, 2) + ALL_RESOURCE_KEYS[key] = key .. CONF_VERSION_KEY_SUFFIX +end + + +local function check_duplicate(item, key, id_set) + local identifier, identifier_type + if key == "consumers" then + identifier = item.id or item.username + identifier_type = item.id and "credential id" or "username" + else + identifier = item.id + identifier_type = "id" + end + + if id_set[identifier] then + return true, "found duplicate " .. identifier_type .. " " .. identifier .. " in " .. key + end + id_set[identifier] = true + return false +end + + +local function check_conf(checker, schema, item, typ) + if not checker then + return true + end + local str_id = tostring(item.id) + if typ == "consumers" and + core.string.find(str_id, "/credentials/") then + local credential_checker = resources.credentials.checker + local credential_schema = resources.credentials.schema + return credential_checker(item.id, item, false, credential_schema, { + skip_references_check = true, + }) + end + + local secret_type + if typ == "secrets" then + local idx = str_find(str_id or "", "/") + if not idx then + return false, { + error_msg = "invalid secret id: " .. (str_id or "") + } + end + secret_type = str_sub(str_id, 1, idx - 1) + end + return checker(item.id, item, false, schema, { + secret_type = secret_type, + skip_references_check = true, + }) +end + + +function _M.validate_configuration(req_body, collect_all_errors) + local is_valid = true + local validation_results = {} + + for key, conf_version_key in pairs(ALL_RESOURCE_KEYS) do + local items = req_body[key] + local resource = resources[key] or {} + + -- Validate conf_version_key if present + local new_conf_version = req_body[conf_version_key] + if new_conf_version and type(new_conf_version) ~= "number" then + if not collect_all_errors then + return false, conf_version_key .. " must be a number" + end + is_valid = false + table_insert(validation_results, { + resource_type = key, + error = conf_version_key .. " must be a number, got " .. type(new_conf_version) + }) + end + + if items and #items > 0 then + local item_schema = resource.schema + local item_checker = resource.checker + local id_set = {} + + for index, item in ipairs(items) do + local item_temp = tbl_deepcopy(item) + local valid, err = check_conf(item_checker, item_schema, item_temp, key) + if not valid then + local err_prefix = "invalid " .. key .. " at index " .. (index - 1) .. ", err: " + local err_msg = type(err) == "table" and err.error_msg or err + local error_msg = err_prefix .. err_msg + + if not collect_all_errors then + return false, error_msg + end + is_valid = false + table_insert(validation_results, { + resource_type = key, + index = index - 1, + error = error_msg + }) + end + + -- check for duplicate IDs + local duplicated, dup_err = check_duplicate(item, key, id_set) + if duplicated then + if not collect_all_errors then + return false, dup_err + end + is_valid = false + table_insert(validation_results, { + resource_type = key, + index = index - 1, + error = dup_err + }) + end + end + end + end + + if collect_all_errors then + return is_valid, validation_results + end + + return is_valid, nil +end + + +function _M.validate(ctx) + local content_type = core.request.header(nil, "content-type") or "application/json" + local req_body, err = core.request.get_body() + if err then + return core.response.exit(400, {error_msg = "invalid request body: " .. err}) + end + + if not req_body or #req_body <= 0 then + return core.response.exit(400, {error_msg = "invalid request body: empty request body"}) + end Review Comment: This endpoint reads the request body via `core.request.get_body()` without a max-size limit, while other Admin API handlers in `apisix/admin/init.lua` use `MAX_REQ_BODY` to fail fast on oversized payloads. Since `/configs/validate` is explicitly meant for batch configs, it's easy to hit large bodies; consider adding a bounded read (either accept `max_body` as a parameter from the caller, or use a shared constant) to avoid excessive memory use / unexpected 413-like behavior differences. ########## apisix/admin/init.lua: ########## @@ -423,6 +424,12 @@ local function standalone_run() end +local function validate_configs() + set_ctx_and_check_token() + return config_validate.validate() Review Comment: `config_validate.validate` is defined as `validate(ctx)` but this handler calls it without passing the API ctx. It works today because `ctx` is unused, but this coupling is brittle (any future use of `ctx` will see `nil` in etcd mode). Consider either removing the `ctx` parameter from `config_validate.validate` or passing `ngx.ctx.api_ctx` here for consistency with the standalone call site. ```suggestion return config_validate.validate(ngx.ctx.api_ctx) ``` ########## apisix/admin/standalone.lua: ########## @@ -15,37 +15,20 @@ -- local type = type local pairs = pairs -local ipairs = ipairs local str_lower = string.lower -local str_find = string.find -local str_sub = string.sub -local tostring = tostring local ngx = ngx -local pcall = pcall local ngx_time = ngx.time local get_method = ngx.req.get_method Review Comment: `ipairs` is still used later in this file (e.g., in `patch_schema()`), but the local alias was removed. This falls back to the global `ipairs`, which is inconsistent with the rest of the module’s localized stdlib usage and slightly increases per-request overhead. Consider reintroducing `local ipairs = ipairs` (or stop localizing stdlib consistently throughout the file). ########## t/admin/config-validate.t: ########## @@ -0,0 +1,426 @@ +# +# 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(); +no_shuffle(); +log_level("warn"); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!$block->request) { + $block->set_value("request", "GET /t"); + } +}); + +run_tests; + +__DATA__ + +=== TEST 1: validate configs - success with valid route +--- config +location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/configs/validate', + ngx.HTTP_POST, + [[{ + "routes": [ + { + "id": "r1", + "uri": "/test", + "upstream": { + "nodes": {"127.0.0.1:1980": 1}, + "type": "roundrobin" + } + } + ] + }]] + ) Review Comment: The new test suite doesn't exercise declarative configs where resources omit `id` (common for routes/services, e.g., the shipped `conf/apisix.yaml` route example). Adding a test case that posts a valid route without `id` (and ideally a `plugins` list entry that uses `name`) would prevent regressions and would catch the current nil-identifier crash in duplicate checking. -- 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]
