You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "undying (via GitHub)" <gi...@apache.org> on 2025/04/29 14:26:24 UTC
[PR] Upstreams variable support [apisix]
undying opened a new pull request, #12187:
URL: https://github.com/apache/apisix/pull/12187
### Description
Added support for Nginx variables in the `service_name` parameter for service discovery. This allows dynamic service name resolution based on Nginx variables such as `$host`, `$http_host`, and others, working with all service discovery implementations (Consul, Eureka, Nacos, etc.).
### Detailed Description
#### Problem
The current implementation of service discovery in APISIX does not support using Nginx variables in the `service_name` parameter. This limits configuration flexibility as the service name must be hardcoded in the configuration, regardless of which service discovery implementation is used.
#### Solution
Added support for Nginx variables in the `service_name` parameter at the upstream level. This change enables dynamic service name resolution across all service discovery implementations. For example:
#### Usage Example
1. Define a variable in Nginx configuration:
```nginx
map $http_host $backend {
default echo;
"~([^.]+).domain.local" $1;
}
```
2. Define routes
```yaml
routes:
-
uri: "/*"
host: "*.domain.local"
upstream:
service_name: $backend # Nginx variable
type: "roundrobin"
discovery_type: "consul" # or any other discovery type
```
2. Use the variable in route configuration with any service discovery:
```yaml
routes:
-
uri: "/*"
host: "*.domain.local"
upstream:
service_name: $backend
type: "roundrobin"
discovery_type: "consul" # or "eureka", "nacos", etc.
```
In this example, if a request comes to `service1.domain.local`, the `$backend` variable will be set to `service1`, and APISIX will look for a service with this name in the configured service discovery system.
#### Backward Compatibility
The change is fully backward compatible because:
1. Existing configurations will continue to work without changes
2. The new functionality is only activated when using Nginx variables in `service_name`
3. The change is implemented at the upstream level, making it available to all service discovery implementations
---
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
undying commented on code in PR #12187:
URL: https://github.com/apache/apisix/pull/12187#discussion_r2247744447
##########
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "undying (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "membphis (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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 dev@apisix.apache.org 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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: adds support for nginx variables in service_name param [apisix]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org