Skip to content

feat: adds support for nginx variables in service_name param #12187

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
19 changes: 18 additions & 1 deletion apisix/core/utils.lua
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,24 @@ do
end
end
-- Resolve ngx.var in the given string
-- @function core.utils.resolve_var
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these comments necessary?

Copy link
Author

Choose a reason for hiding this comment

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

I added comments from the approach in order to generate documentation for the methods later.
If there are no such plans, I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep it, but we need to make sure it's correct.

-- @tparam string tpl The template string to resolve variables in
-- @tparam table ctx The context table containing variables
-- @tparam function escaper Optional function to escape resolved values
-- @treturn string The resolved string
-- @treturn string|nil Error message if any
-- @treturn number Number of variables replaced
-- @usage
-- local utils = require("apisix.core.utils")
--
-- -- Usage examples:
-- local res = utils.resolve_var("$host", ctx.var) -- "example.com"
-- local res = utils.resolve_var("${host}", ctx.var) -- "example.com"
-- local res = utils.resolve_var("TMP_${VAR1}_${VAR2}", ctx.var) -- "TMP_value1_value2"
-- local res = utils.resolve_var("\\$host", ctx.var) -- "$host"
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you already confirmed this? I mean, do the escaping.

--
-- -- Usage in APISIX context:
-- local service_name = utils.resolve_var(up_conf.service_name, api_ctx.var)
Comment on lines +356 to +357
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove it and the IDE will tell us the reference without having to document it.

_M.resolve_var = resolve_var


Expand Down Expand Up @@ -461,5 +479,4 @@ function _M.check_tls_bool(fields, conf, plugin_name)
end
end


return _M
7 changes: 6 additions & 1 deletion apisix/upstream.lua
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,12 @@ function _M.set_by_route(route, api_ctx)
return 503, err
end

local new_nodes, err = dis.nodes(up_conf.service_name, up_conf.discovery_args)
local service_name, err = core.utils.resolve_var(up_conf.service_name, api_ctx.var)
if not service_name or service_name == "" then
return 503, "resolve_var resolves to empty string: " .. (err or "nil")
end

local new_nodes, err = dis.nodes(service_name, up_conf.discovery_args)
if not new_nodes then
return HTTP_CODE_UPSTREAM_UNAVAILABLE, "no valid upstream node: " .. (err or "nil")
end
Expand Down
Loading