apisix icon indicating copy to clipboard operation
apisix copied to clipboard

feat: adds support for nginx variables in service_name param

Open undying opened this issue 7 months ago • 15 comments

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:
map $http_host $backend {
  default echo;
  "~([^.]+).domain.local" $1;
}
  1. Define routes
routes:
  -
    uri: "/*"
    host: "*.domain.local"
    upstream:
      service_name: $backend  # Nginx variable
      type: "roundrobin"
      discovery_type: "consul"  # or any other discovery type
  1. Use the variable in route configuration with any service discovery:
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 first)

undying avatar Apr 29 '25 14:04 undying

@undying please fix CI, thx

moonming avatar May 02 '25 04:05 moonming

I've pushed the linter fix, hope that will help. Btw, how do you run tests locally to have a fast response loop while developing plugins?

undying avatar May 15 '25 13:05 undying

I've pushed the linter fix, hope that will help. Btw, how do you run tests locally to have a fast response loop while developing plugins?

I don't get it. Can you describe it in more detail?

Baoyuantop avatar May 16 '25 02:05 Baoyuantop

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?

undying avatar May 16 '25 10:05 undying

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/

Baoyuantop avatar May 19 '25 02:05 Baoyuantop

One of the steps in GitHub Actions still failing, 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, that's the way it is.

Am I right and is it about GitHub Actions or is it about the code?

undying avatar May 28 '25 16:05 undying

Okay, it seems that everything is built successfully, hope it can be merged now.

undying avatar May 30 '25 09:05 undying

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.

bzp2010 avatar Jun 06 '25 13:06 bzp2010

Hi @undying, please fix the failed CI

Baoyuantop avatar Jul 03 '25 02:07 Baoyuantop

Hi @undying, please fix the failed CI

Finally fixed

undying avatar Jul 22 '25 07:07 undying

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

membphis avatar Aug 01 '25 06:08 membphis

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:

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"
    }
}'

undying avatar Aug 01 '25 09:08 undying

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

membphis avatar Aug 04 '25 01:08 membphis

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?

undying avatar Oct 16 '25 13:10 undying

Anyone is welcome to leave a comment on this PR and give feedback on whether you think this feature is necessary

membphis avatar Oct 23 '25 01:10 membphis