-
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
Finally fixed |
apisix/upstream.lua
Outdated
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") |
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.
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
.
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 logging service_name. This format is ok?
This new feature is not safe, we can wait for more voice from the community. For me, Here is a bad example:
I sugguest: need to wait for more voice from community, don't merge this PR soon |
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.
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.
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: map $http_host $service {
hostnames;
default ""; # or some dummy upstream that returns a more valuable response
service-a.* service_a;
service-b.* service_b;
} $ 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"
}
}' |
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 |
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