nic-6443 commented on code in PR #13220: URL: https://github.com/apache/apisix/pull/13220#discussion_r3076987018
########## 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: Fixed. Added a nil guard — `check_duplicate` now returns `false` early when identifier is nil, so resources without `id` skip duplicate checking 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` key in standalone config is handled differently — it uses `apisix.admin.plugins` which has its own checker that validates the whole array. The nil identifier issue is now fixed with the guard added above. The `plugins` entries use `name` not `id`, and since `item.id` will be nil, duplicate checking is safely skipped. ########## 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: Added TEST 14 — validates routes and services without `id` field, confirming no nil crash and a 200 response. ########## 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: Already fixed in the previous commit — `local ipairs = ipairs` is back on line 18. -- 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]
