Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
github-actions[bot] commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-3767693988 This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
github-actions[bot] closed pull request #12187: feat: adds support for nginx variables in service_name param URL: https://github.com/apache/apisix/pull/12187 -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
github-actions[bot] commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-3681351174 This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
membphis commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-3434762323 Anyone is welcome to leave a comment on this PR and give feedback on whether you think this feature is necessary -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-3410971891 Hello everyone! I'd like to reopen the discussion, as the PR is currently in its final form, but there's no approval or restriction on merging it into master. Is there any active discussion about this feature, or can we rely on the community's reactions in this thread as a sign of support? -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
membphis commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-3148880794 > We can consider Nginx as an unsafe web server because it allows us to enable such features out of the box. I believe it's the user's choice how to use such a feature, so its presence is much better than its absence. I don't know a good way to balance between safe and convenient for this new feature. I love this PR, but we need to consider the `unsafe` thing. More discussion and suggestions from the community are needed, we may get another good way to resolve the issue -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r224777 ## apisix/upstream.lua: ## @@ -305,7 +305,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") Review Comment: I added logging service_name. This format is ok? -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on PR #12187:
URL: https://github.com/apache/apisix/pull/12187#issuecomment-3143880073
> This new feature is not safe, we can wait for more voice from the
community.
We can consider Nginx as an unsafe web server because it allows us to enable
such features out of the box. I believe it's the user's choice how to use such
a feature, so its presence is much better than its absence.
> For me, `one upstream` work for `one service`, much safer.
It's safer, but it's not convenient when you have hundreds of microservices
with simple domain/service mapping. The "safe" method requires you to manually
copy and paste configurations for every such service, which results in a
massive configuration that negatively affects performance and maintainability.
> Here is a bad example:
Actually, it's a very good example because this is how the feature works.
You create a single configuration with simple mapping. For additional security,
you can further refine the configuration and extract the service name not
directly from the header, but from a special map that serves as an allow list
for services.
Example:
```nginx
map $http_host $service {
hostnames;
default ""; # or some dummy upstream that returns a more valuable
response
service-a.* service_a;
service-b.* service_b;
}
```
```sh
$ curl http://127.0.0.1:9180/apisix/admin/routes/1 -H "X-API-KEY:
$admin_key" -X PUT -i -d '
{
"uri": "/*",
"upstream": {
"service_name": "${service}",
"discovery_type": "eureka"
}
}'
```
--
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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
membphis commented on PR #12187:
URL: https://github.com/apache/apisix/pull/12187#issuecomment-3142919026
This new feature is not safe, we can wait for more voice from the community.
For me, `one upstream` work for `one service`, much safer.
Here is a bad example:
```
# bad example, which may cause APISIX unsafe, allow user to access any
service of K8s or eureka
$ curl http://127.0.0.1:9180/apisix/admin/routes/1 -H "X-API-KEY:
$admin_key" -X PUT -i -d '
{
"uri": "/*",
"upstream": {
"service_name": "${http_service}",
"discovery_type": "eureka"
}
}'
```
I sugguest: need to wait for more voice from community, don't merge this PR
soon
--
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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
nic-6443 commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2247054300 ## apisix/upstream.lua: ## @@ -305,7 +305,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") Review Comment: please add `service_name` to this error log, since `service_name` is no longer a fixed value, we need to know the specific `service_name` when there is a `no valid upstream node`. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2221910687 ## apisix/core/utils.lua: ## @@ -461,5 +461,4 @@ function _M.check_tls_bool(fields, conf, plugin_name) end end - Review Comment: Should I return an empty string so that the file has two empty lines at the end? Maybe this is the code style of the project and I accidentally broke it? -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Baoyuantop commented on code in PR #12187:
URL: https://github.com/apache/apisix/pull/12187#discussion_r2221687434
##
docs/en/latest/discovery.md:
##
@@ -216,6 +216,35 @@ Server: APISIX web server
{"node":{"value":{"uri":"\/user\/*","upstream": {"service_name":
"USER-SERVICE", "type": "roundrobin", "discovery_type":
"eureka"}},"createdIndex":61925,"key":"\/apisix\/routes\/1","modifiedIndex":61925}}
```
+You can also use variables in the service name. For example, to route requests
based on the host header using nginx map:
Review Comment:
Need to modify the Chinese document synchronously.
--
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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Baoyuantop commented on code in PR #12187:
URL: https://github.com/apache/apisix/pull/12187#discussion_r2221663611
##
apisix/core/utils.lua:
##
@@ -461,5 +461,4 @@ function _M.check_tls_bool(fields, conf, plugin_name)
end
end
-
Review Comment:
This change is not necessary.
##
docs/en/latest/discovery.md:
##
@@ -216,6 +216,35 @@ Server: APISIX web server
{"node":{"value":{"uri":"\/user\/*","upstream": {"service_name":
"USER-SERVICE", "type": "roundrobin", "discovery_type":
"eureka"}},"createdIndex":61925,"key":"\/apisix\/routes\/1","modifiedIndex":61925}}
```
+You can also use variables in the service name. For example, to route requests
based on the host header using nginx map:
+
+First, configure the map in your nginx configuration:
+
+```nginx
+map $http_host $backend {
+hostnames;
+default service_a;
+x.domain.local service_x;
+y.domain.local service_y;
+}
+```
+
+Then use the mapped variable in your route configuration:
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/1 -H "X-API-KEY: $admin_key"
-X PUT -i -d '
+{
+"uri": "/*",
+"upstream": {
+"service_name": "${backend}",
+"type": "roundrobin",
+"discovery_type": "eureka"
+}
+}'
+```
+
+In this example, requests to `x.domain.local` will be routed to the service
named "service_a", while requests to `y.domain.local` will be routed to
"service_b".
Review Comment:
Should this be service_x and service_y ?
##
t/discovery/consul.t:
##
@@ -26,7 +26,6 @@ add_block_preprocessor(sub {
my ($block) = @_;
my $http_config = $block->http_config // <<_EOC_;
-
Review Comment:
Unnecessary changes
--
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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-3101445312 > Hi @undying, please fix the failed CI Finally fixed -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Baoyuantop commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-3030329963 Hi @undying, please fix the failed CI -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on code in PR #12187:
URL: https://github.com/apache/apisix/pull/12187#discussion_r2137220282
##
apisix/core/utils.lua:
##
@@ -337,6 +337,24 @@ do
end
end
-- Resolve ngx.var in the given string
+-- @function core.utils.resolve_var
+-- @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"
Review Comment:
I've checked that [pat
regex](https://github.com/apache/apisix/blob/master/apisix/core/utils.lua#L298C19-L298C45)
is [working this way](https://regex101.com/r/LJgPIo/1).
But anyway, I deleted the documentation based on Baoyuantop's recommendation.
--
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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
bzp2010 commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-2949291415 I'm honestly not sure how we should test it. You can choose any kind of service discovery provider that is easy to utilize and add tests there. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
bzp2010 commented on code in PR #12187:
URL: https://github.com/apache/apisix/pull/12187#discussion_r2132207021
##
apisix/core/utils.lua:
##
@@ -337,6 +337,24 @@ do
end
end
-- Resolve ngx.var in the given string
+-- @function core.utils.resolve_var
+-- @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"
+--
+-- -- Usage in APISIX context:
+-- local service_name = utils.resolve_var(up_conf.service_name, api_ctx.var)
Review Comment:
Remove it and the IDE will tell us the reference without having to document
it.
##
apisix/core/utils.lua:
##
@@ -337,6 +337,24 @@ do
end
end
-- Resolve ngx.var in the given string
+-- @function core.utils.resolve_var
+-- @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"
Review Comment:
Have you already confirmed this? I mean, do the escaping.
--
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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
bzp2010 commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2132199958 ## apisix/core/utils.lua: ## @@ -337,6 +337,24 @@ do end end -- Resolve ngx.var in the given string +-- @function core.utils.resolve_var Review Comment: I think we can keep it, but we need to make sure it's correct. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2131665803 ## apisix/core/utils.lua: ## @@ -337,6 +337,24 @@ do end end -- Resolve ngx.var in the given string +-- @function core.utils.resolve_var Review Comment: 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. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Baoyuantop commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2131378084 ## apisix/core/utils.lua: ## @@ -337,6 +337,24 @@ do end end -- Resolve ngx.var in the given string +-- @function core.utils.resolve_var Review Comment: Are these comments necessary? -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2129210171 ## apisix/upstream.lua: ## @@ -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, "service_name is empty: " .. (err or "nil") Review Comment: Changed the error message for suggested one -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2129208627 ## t/core/utils.t: ## @@ -393,3 +393,5 @@ res:nil res:5 res:12 res:7 + + Review Comment: It made sense solely for launching Github Actions Fixed. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
bzp2010 commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2125563602 ## apisix/upstream.lua: ## @@ -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, "service_name is empty: " .. (err or "nil") Review Comment: It is best to use a meaningful fallback value, such as `resolve_var resolves to empty string`. ## t/core/utils.t: ## @@ -393,3 +393,5 @@ res:nil res:5 res:12 res:7 + + Review Comment: This doesn't seem to make sense? Don't modify the code style, which causes Code Lint to fail. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on code in PR #12187:
URL: https://github.com/apache/apisix/pull/12187#discussion_r2124667723
##
apisix/core/utils.lua:
##
@@ -461,5 +461,35 @@ function _M.check_tls_bool(fields, conf, plugin_name)
end
end
+---
+-- Checks if a string is an nginx variable.
+-- An nginx variable starts with the '$' character.
+--
+-- @function core.utils.is_nginx_variable
+-- @tparam string str The string to check
+-- @treturn boolean true if the string starts with '$', false otherwise
+-- @usage
+-- local utils = require("apisix.core.utils")
+--
+-- -- Usage examples:
+-- local is_var = utils.is_nginx_variable("$host") -- true
+-- local is_var = utils.is_nginx_variable("host") -- false
+-- local is_var = utils.is_nginx_variable("${host}") -- true
+-- local is_var = utils.is_nginx_variable("\\$host") -- false
+--
+-- -- Usage in APISIX context:
+-- if utils.is_nginx_variable(up_conf.service_name) then
+-- -- Handle as nginx variable
+-- else
+-- -- Handle as regular service name
+-- end
+function _M.is_nginx_variable(str)
+if not str or type(str) ~= "string" then
+return false
+end
+
+-- Check if the string starts with '$' and it's not escaped
+return str:sub(1, 1) == "$" and (str:sub(1, 2) ~= "\\$")
+end
Review Comment:
My main idea was to avoid an unnecessary call to `resolve_var` if the string
is not actually a variable. Well, as if it were superfluous, since
`resolve_var` has such a check and an early exit, so optimization is
unnecessary.
I changed it to use `resolve_var` directly, thanks for the help.
--
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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2124362620 ## apisix/upstream.lua: ## @@ -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) Review Comment: > Just a reminder that you don't check the third return value of resolve_var. It indicates that several variables in the template have been replaced. I'm not sure that I need to check that. User may construct service name from multiple variables and this is fine (for me at least). But thanks for noticing this. > If your template contains only one variable and its value is empty, service name will be replaced with an empty string. I'm not sure if this is what you expect. Good point, thank you! I'll fix with a check that value is not an empty string. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
bzp2010 commented on code in PR #12187: URL: https://github.com/apache/apisix/pull/12187#discussion_r2123492465 ## apisix/upstream.lua: ## @@ -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) Review Comment: Just a reminder that you don't check the third return value of resolve_var. It indicates that several variables in the template have been replaced. If your template contains only one variable and its value is empty, service name will be replaced with an empty string. I'm not sure if this is what you expect. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
bzp2010 commented on code in PR #12187:
URL: https://github.com/apache/apisix/pull/12187#discussion_r2118840015
##
apisix/core/utils.lua:
##
@@ -461,5 +461,35 @@ function _M.check_tls_bool(fields, conf, plugin_name)
end
end
+---
+-- Checks if a string is an nginx variable.
+-- An nginx variable starts with the '$' character.
+--
+-- @function core.utils.is_nginx_variable
+-- @tparam string str The string to check
+-- @treturn boolean true if the string starts with '$', false otherwise
+-- @usage
+-- local utils = require("apisix.core.utils")
+--
+-- -- Usage examples:
+-- local is_var = utils.is_nginx_variable("$host") -- true
+-- local is_var = utils.is_nginx_variable("host") -- false
+-- local is_var = utils.is_nginx_variable("${host}") -- true
+-- local is_var = utils.is_nginx_variable("\\$host") -- false
+--
+-- -- Usage in APISIX context:
+-- if utils.is_nginx_variable(up_conf.service_name) then
+-- -- Handle as nginx variable
+-- else
+-- -- Handle as regular service name
+-- end
+function _M.is_nginx_variable(str)
+if not str or type(str) ~= "string" then
+return false
+end
+
+-- Check if the string starts with '$' and it's not escaped
+return str:sub(1, 1) == "$" and (str:sub(1, 2) ~= "\\$")
+end
Review Comment:
Does it really work? That's part of what the `resolve_var` function does.
You can just use `resolve_var` directly, and it resolves the variable as
best it can. When any variable is not found or does not exist, the variable is
replaced with an empty string.
Also, `resolve_var` returns the number of variables it replaces; if it
returns 0, you can choose to leave the template string as it is (i.e., no
replacement).
Using a separate function to determine if it is a variable is neither useful
nor necessary. Your implementation assumes that variable strings must begin
with `$`, but `resolve_var` can handle more clearances than you assume, such as
`TMP_${VAR1}_${VAR2}`.
Other than that, your function just checks if the function is a string
starting with `$` (by the way `core.string` provides `has_prefix` and
`has_suffix` functions directly), it doesn't check if the variable key is a
real nginx variable at all, and in fact it can't be checked, a variable that
doesn't exist is indistinguishable from a variable with a value of nil.
Therefore the naming of `is_nginx_variable` is inappropriate.
--
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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-2921733609 Okay, it seems that everything is built successfully, hope it can be merged now. -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
undying commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-2916943362 [One of the steps in GitHub Actions still failing](https://github.com/apache/apisix/actions/runs/15294764947/job/43029963306?pr=12187), and it seems to me that this is not related to the code that I changed. Judging by [other PRs where the same step falls](https://github.com/apache/apisix/pull/12244/commits/4a8f7990872f6836c4b1f91b22a0710cfa8fe010), that's the way it is. Am I right and is it about GitHub Actions or is it about the code? -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Baoyuantop commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-2889409140 > > Can you describe it in more detail? > > I mean, there are a lot of GitHub actions in the repository, including running tests inside the build step. I tried to run this step locally to make debugging locally faster, but I couldn't, because the project has too many dependencies. > > I thought maybe you have some tools for running tests locally? How did you try? What were the mistakes you encountered? Here's a document to refer to https://apisix.apache.org/docs/apisix/internal/testing-framework/ -- 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]
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
moonming commented on PR #12187: URL: https://github.com/apache/apisix/pull/12187#issuecomment-2846296963 @undying please fix CI, thx -- 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]
