-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
base: master
Are you sure you want to change the base?
Conversation
@undying please fix CI, thx |
I've pushed the linter fix, hope that will help. |
I don't get it. 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 comment was marked as resolved.
This comment was marked as resolved.
Okay, it seems that everything is built successfully, hope it can be merged now. |
* use resolve_var directly * adds resolve_var documentation * removes is_nginx_variable function and tests
…x into upstreams_variable_support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need add doc and test case
apisix/core/utils.lua
Outdated
@@ -337,6 +337,24 @@ do | |||
end | |||
end | |||
-- Resolve ngx.var in the given string | |||
-- @function core.utils.resolve_var |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these comments necessary?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
apisix/core/utils.lua
Outdated
-- 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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked that pat regex is working this way.
But anyway, I deleted the documentation based on Baoyuantop's recommendation.
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. |
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
In this example, if a request comes to
service1.domain.local
, the$backend
variable will be set toservice1
, 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:
service_name
Checklist