Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: validation during merging node_listen configuration #4881

Merged
merged 3 commits into from
Aug 27, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions apisix/cli/file.lua
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,28 @@ local function tinyyaml_type(t)
end


local function path_is_multi_type(path, type_val)
if str_sub(path, 1, 14) == "nginx_config->" and
(type_val == "number" or type_val == "string") then
return true
end

if path == "apisix->node_listen" and type_val == "number" then
return true
end

if path == "apisix->ssl->listen_port" and type_val == "number" then
return true
end

if path == "apisix->ssl->listen" and type_val == "number" then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what apisix->ssl->listen it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tzssangglass The config apisix.ssl.listen will be used in another PR (#4856). This will not affect the current code, do I remove it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think remove it is better and add it to the PRs that need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I have removed it, please retest. @tzssangglass

return true
end

return false
end


local function merge_conf(base, new_tab, ppath)
ppath = ppath or ""

Expand Down Expand Up @@ -143,12 +165,11 @@ local function merge_conf(base, new_tab, ppath)
if base[key] == nil then
base[key] = val
elseif type(base[key]) ~= type_val then
if (ppath == "nginx_config" or str_sub(ppath, 1, 14) == "nginx_config->") and
(type_val == "number" or type_val == "string")
then
local path = ppath == "" and key or ppath .. "->" .. key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use JSON schema to check it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@membphis The old code has been the schema like ppath .. "->" .. key,and it used to print log. If change this to JSON,many test cases could be affected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, got it. thx


if path_is_multi_type(path, type_val) then
base[key] = val
else
local path = ppath == "" and key or ppath .. "->" .. key
return nil, "failed to merge, path[" .. path .. "] expect: " ..
type(base[key]) .. ", but got: " .. type_val
end
Expand Down
4 changes: 3 additions & 1 deletion conf/config-default.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
#

apisix:
node_listen: 9080 # APISIX listening port
# node_listen: 9080 # APISIX listening port
node_listen: # This style support multiple ports
- 9080
enable_admin: true
enable_admin_cors: true # Admin API support CORS response headers.
enable_debug: false
Expand Down