Skip to content

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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

undying
Copy link

@undying undying commented Apr 29, 2025

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)

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Apr 29, 2025
@undying undying changed the title Upstreams variable support feat: adds support for nginx variables in service_name param Apr 29, 2025
@moonming
Copy link
Member

moonming commented May 2, 2025

@undying please fix CI, thx

@Baoyuantop Baoyuantop added the wait for update wait for the author's response in this issue/PR label May 7, 2025
@undying
Copy link
Author

undying commented May 15, 2025

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?

@Baoyuantop
Copy link
Contributor

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?

@undying
Copy link
Author

undying commented May 16, 2025

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?

@Baoyuantop
Copy link
Contributor

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/

@undying

This comment was marked as resolved.

@undying
Copy link
Author

undying commented May 30, 2025

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

@Baoyuantop Baoyuantop self-requested a review May 30, 2025 09:51
undying added 2 commits June 2, 2025 15:16
* use resolve_var directly
* adds resolve_var documentation
* removes is_nginx_variable function and tests
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jun 2, 2025
Copy link
Contributor

@Baoyuantop Baoyuantop left a 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

@@ -337,6 +337,24 @@ do
end
end
-- Resolve ngx.var in the given string
-- @function core.utils.resolve_var
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these comments necessary?

Copy link
Author

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.

Copy link
Contributor

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.

-- 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"
Copy link
Contributor

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.

Copy link
Author

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.

@bzp2010
Copy link
Contributor

bzp2010 commented Jun 6, 2025

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.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jun 9, 2025
@Baoyuantop Baoyuantop removed wait for update wait for the author's response in this issue/PR user responded labels Jun 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants