Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2366148383
##
docs/en/latest/discovery/nacos.md:
##
@@ -278,3 +279,52 @@ The formatted response as below:
}
}
```
+
+ Metadata filtering
+
+APISIX supports filtering service instances based on metadata. When a route is
configured with metadata conditions, only service instances whose metadata
contains all the key-value pairs specified in the route's `metadata`
configuration will be selected. The metadata values in the route configuration
are arrays, and a service instance matches if its metadata value equals any
value in the corresponding array.
+
+Example: If a service instance has metadata `{lane: "a", env: "prod", version:
"1.0"}`, it will match routes configured with metadata `{lane: ["a"]}` or
`{lane: ["a"], env: ["prod"]}`, but not routes configured with `{lane: ["b"]}`
or `{lane: ["a"], region: ["us"]}`.
+
+Example of routing a request with metadata filtering:
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/5 -H "X-API-KEY: $admin_key"
-X PUT -i -d '
+{
+"uri": "/nacosWithMetadata/*",
+"upstream": {
+"service_name": "APISIX-NACOS",
+"type": "roundrobin",
+"discovery_type": "nacos",
+"discovery_args": {
+ "metadata": {
+"version": ["v1"]
Review Comment:
I considered your advice. The `string:string` structure is sufficient to
handle this scenario. Consul, Nacos, and Eureka support string:string metadata,
Kubernetes supports labels, and DNS supports TXT records. I'm not familiar with
TARS.
Additionally, if you plan to support string[] in the future, you could
implement a union type like `string|string[]`.
##
apisix/discovery/nacos/init.lua:
##
@@ -54,6 +56,25 @@ local function get_key(namespace_id, group_name,
service_name)
return namespace_id .. '.' .. group_name .. '.' .. service_name
end
+
+local function metadata_contains(host_metadata, route_metadata)
+if not host_metadata or not next(host_metadata) then
+return false
+end
+
+for k, v in pairs(route_metadata) do
+if type(v) ~= "string" then
Review Comment:
Yes, this check is redundant.
##
apisix/discovery/nacos/init.lua:
##
@@ -54,6 +55,23 @@ local function get_key(namespace_id, group_name,
service_name)
return namespace_id .. '.' .. group_name .. '.' .. service_name
end
+
+local function metadata_contains(host_metadata, route_metadata)
+if not route_metadata or not next(route_metadata) then
+return true
+end
+if not host_metadata or not next(host_metadata) then
+return false
+end
+
+for k, v in pairs(route_metadata) do
+if host_metadata[k] ~= v then
+return false
+end
+end
+return true
+end
Review Comment:
Ok, I'll. Should I put the separate file in `apisix/utils`?
##
apisix/discovery/nacos/init.lua:
##
@@ -54,6 +55,23 @@ local function get_key(namespace_id, group_name,
service_name)
return namespace_id .. '.' .. group_name .. '.' .. service_name
end
+
+local function metadata_contains(host_metadata, route_metadata)
+if not route_metadata or not next(route_metadata) then
+return true
+end
+if not host_metadata or not next(host_metadata) then
+return false
+end
+
+for k, v in pairs(route_metadata) do
+if host_metadata[k] ~= v then
+return false
+end
+end
+return true
+end
Review Comment:
@SkyeYoung Where should I put the separate file?
##
t/core/schema_def.t:
##
@@ -255,64 +255,173 @@ passed
=== TEST 5: validate IPv6 address format in upstream nodes
--- config
-location /t {
Review Comment:
The indent here is wrong. But I shouldn't change it in this PR. I'll revert
it.
##
ci/pod/nacos/service/Dockerfile:
##
@@ -25,8 +25,8 @@ ENV GROUP=${GROUP:-DEFAULT_GROUP}
ADD
https://raw.githubusercontent.com/api7/nacos-test-service/main/spring-nacos-1.0-SNAPSHOT.jar
/app.jar
Review Comment:
Not related to this PR.
##
apisix/discovery/nacos/init.lua:
##
@@ -329,6 +344,13 @@ local function fetch_from_host(base_uri, username,
password, services)
port = host.port,
weight = host.weight or default_weight,
}
+if host.metadata ~= nil then
+if type(host.metadata) == 'table' then
+node.metadata = host.metadata
+else
+log.error('nacos host metadata is not a table: ',
host.metadata)
+end
Review Comment:
I fixed this by clearing the metadata field before returning the nodes.
##
apisix/discovery/nacos/init.lua:
##
@@ -54,6 +55,23 @@ local function get_key(namespace_id, group_name,
service_name)
return nam
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-4149582411 > Hi @beardnick, thank you for the Nacos metadata filtering support (#12392)! > > With 16 reviews, this is a well-discussed and important feature for canary/lane-based routing. Metadata filtering is essential for many production deployment patterns. > > **To confirm merge readiness**: > > 1. Are all 16 review comments addressed? > 2. Is the metadata filtering API consistent with how we plan to implement it for other discovery backends (e.g., Consul in #12448)? > 3. Tests and documentation appear present — please confirm everything passes on latest master. > > I'd like to prioritize this alongside #12448 (Consul) for a consistent metadata filtering experience. Thank you for your persistence through the review process! 👏 1. Only this one left, I need some guidance here—where should I place the extracted function? https://github.com/user-attachments/assets/dce29ba5-a35a-4a13-9761-523514d44474"; /> 2. No, there's a key difference: in my PR, `env` is a string, whereas in #12448 , `env` is an array. 3. I confirmed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Copilot commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2917016146
##
apisix/discovery/nacos/init.lua:
##
@@ -408,6 +430,19 @@ function _M.nodes(service_name, discovery_args)
return nil
end
local nodes = core.json.decode(value)
+
+-- Apply metadata filtering if specified
+local required_metadata = discovery_args and discovery_args.metadata
+if required_metadata and next(required_metadata) then
+local filtered_nodes = {}
+for _, node in ipairs(nodes) do
+if metadata_contains(node.metadata, required_metadata) then
+core.table.insert(filtered_nodes, node)
+end
+end
+return filtered_nodes
+end
Review Comment:
`_M.nodes()` always `core.json.decode()`s from shared dict, and when
metadata filtering is enabled it allocates a new `filtered_nodes` array each
call. Given `compare_upstream_node()`’s fast path (`old_t == new_t`) and its
reference-based comparison for `metadata`, consider adding a small per-worker
cache keyed by (namespace, group, service, raw_json_value, required_metadata)
so repeated requests can reuse the same nodes table when nothing changed. This
prevents unnecessary upstream refresh work and keeps node versions stable.
##
apisix/discovery/nacos/init.lua:
##
@@ -329,6 +344,13 @@ local function fetch_from_host(base_uri, username,
password, services)
port = host.port,
weight = host.weight or default_weight,
}
+if host.metadata ~= nil then
+if type(host.metadata) == 'table' then
+node.metadata = host.metadata
+else
+log.error('nacos host metadata is not a table: ',
host.metadata)
+end
Review Comment:
`fetch_from_host` now persists `host.metadata` into each returned node.
Because `nacos_dict` stores nodes as JSON and `_M.nodes()` decodes it on every
call, the `metadata` table will be a fresh Lua table each time.
`upstream_util.compare_upstream_node()` compares `metadata` by reference (not
deep value), so this will make APISIX treat upstream nodes as changed on every
request (nodes_ver bumps, frequent resource updates, and potential log spam).
To avoid this, either (a) don’t include `metadata` in the nodes table returned
to upstream (keep it only for filtering and strip it before returning), or (b)
cache the decoded nodes table per service key/value so the same table (and
metadata sub-tables) are reused when unchanged.
```suggestion
if host.metadata ~= nil and type(host.metadata) ~= 'table'
then
log.error('nacos host metadata is not a table: ',
host.metadata)
```
##
ci/pod/nacos/service/Dockerfile:
##
@@ -25,8 +25,8 @@ ENV GROUP=${GROUP:-DEFAULT_GROUP}
ADD
https://raw.githubusercontent.com/api7/nacos-test-service/main/spring-nacos-1.0-SNAPSHOT.jar
/app.jar
Review Comment:
The `ADD` of the remote JAR from
`https://raw.githubusercontent.com/api7/nacos-test-service/main/spring-nacos-1.0-SNAPSHOT.jar`
introduces a supply-chain risk: the image build and runtime depend on code
fetched from a mutable third-party branch without any integrity pinning or
verification. If that repository or the `main` branch is compromised, a
malicious JAR could be served and then executed inside your CI/test environment
with access to any secrets or network resources available to this container. To
harden this, pin the artifact to an immutable reference (e.g., a specific
commit hash or release URL) or vendor the JAR into this repository and verify
its integrity before execution.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Copilot commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2734667915
##
apisix/discovery/nacos/init.lua:
##
@@ -21,7 +21,9 @@ local http = require('resty.http')
local core = require('apisix.core')
local ipairs = ipairs
local pairs = pairs
+local next = next
local type = type
+local math = math
Review Comment:
The local variable 'math' on line 26 is not used anywhere in the code. It's
only needed on line 27 to extract 'math.random' into the 'math_random'
variable. Since 'math' is a built-in Lua global, it doesn't need to be cached
unless it's used multiple times. Consider removing line 26 as it's redundant.
```suggestion
```
##
ci/pod/nacos/service/entrypoint.sh:
##
@@ -0,0 +1,43 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# Build Java command with proper environment variable expansion
+JAVA_ARGS=(
+"-Djava.security.egd=file:/dev/./urandom"
+"-jar"
+"/app.jar"
+"--suffix.num=${SUFFIX_NUM}"
+"--spring.cloud.nacos.discovery.server-addr=${NACOS_ADDR}"
+"--spring.application.name=${SERVICE_NAME}"
+"--spring.cloud.nacos.discovery.group=${GROUP}"
+"--spring.cloud.nacos.discovery.namespace=${NAMESPACE}"
+)
+
+# Add metadata dynamically for all METADATA_* environment variables
+for var in $(env | grep '^METADATA_' | cut -d= -f1); do
+# Convert METADATA_LANE to lane, METADATA_ENV to env, etc.
+metadata_key=$(echo "${var#METADATA_}" | tr '[:upper:]' '[:lower:]')
+metadata_value=$(eval echo \$${var})
Review Comment:
The use of `eval` on environment-derived values in `metadata_value=$(eval
echo \$${var})` allows arbitrary shell command injection if a `METADATA_*`
environment variable contains shell metacharacters or command substitutions. An
attacker who can set these environment variables for this container can execute
arbitrary commands in the container context when the entrypoint runs. Avoid
using `eval` and unquoted expansions here; instead, read the environment
variable value via safe parameter expansion without re-interpreting its
contents as shell code.
```suggestion
metadata_value=${!var}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3808400913 @Baoyuantop Hi, already updated, please take a look again. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Baoyuantop commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3659684111 Hi @beardnick, please check those comments -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
nic-6443 commented on code in PR #12445: URL: https://github.com/apache/apisix/pull/12445#discussion_r2601366980 ## apisix/discovery/nacos/init.lua: ## @@ -54,6 +56,20 @@ local function get_key(namespace_id, group_name, service_name) return namespace_id .. '.' .. group_name .. '.' .. service_name end + +local function metadata_contains(node_metadata, route_metadata) Review Comment: ```suggestion local function metadata_contains(node_metadata, required_metadata) ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
nic-6443 commented on code in PR #12445: URL: https://github.com/apache/apisix/pull/12445#discussion_r2601366437 ## apisix/discovery/nacos/init.lua: ## @@ -315,10 +331,12 @@ local function fetch_full_registry(premature) local key = get_key(namespace_id, group_name, service_info.service_name) service_names[key] = true for _, host in ipairs(data.hosts) do +assert(host.metadata == nil or type(host.metadata) == "table") Review Comment: `assert` will directly cause the program to panic and should be replaced with an error log to record illegal data. The program needs to consider various exceptional situations. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3580625252 @SkyeYoung @bzp2010 @nic-6443 @Revolyssup @AlinsRan @membphis The failed tests appear to be unrelated to my PR. Could you continue reviewing 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3561418903 > Hi @beardnick, could you merge the code from the main branch again to make CI pass? Done -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Baoyuantop commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3561100758 Hi @beardnick, could you merge the code from the main branch again to make CI pass? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3519394032 @Baoyuantop @SkyeYoung The failed tests appear to be unrelated to my PR. Could you continue reviewing 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Baoyuantop commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2501423447
##
t/core/schema_def.t:
##
@@ -255,64 +255,173 @@ passed
=== TEST 5: validate IPv6 address format in upstream nodes
--- config
-location /t {
Review Comment:
Why was this part changed?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3496324049 @Baoyuantop @SkyeYoung ping -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Baoyuantop commented on code in PR #12445: URL: https://github.com/apache/apisix/pull/12445#discussion_r2467763335 ## apisix/discovery/nacos/init.lua: ## @@ -54,6 +56,25 @@ local function get_key(namespace_id, group_name, service_name) return namespace_id .. '.' .. group_name .. '.' .. service_name end + +local function metadata_contains(host_metadata, route_metadata) Review Comment: host_metadata and node.metadata are mixed, it is recommended to unify them as node_metadata ## apisix/discovery/nacos/init.lua: ## @@ -54,6 +56,25 @@ local function get_key(namespace_id, group_name, service_name) return namespace_id .. '.' .. group_name .. '.' .. service_name end + +local function metadata_contains(host_metadata, route_metadata) +if not host_metadata or not next(host_metadata) then +return false +end + +for k, v in pairs(route_metadata) do +if type(v) ~= "string" then Review Comment: Can we handle these checks at schema level? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Baoyuantop commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2331934894
##
docs/en/latest/discovery/nacos.md:
##
@@ -278,3 +279,52 @@ The formatted response as below:
}
}
```
+
+ Metadata filtering
+
+APISIX supports filtering service instances based on metadata. When a route is
configured with metadata conditions, only service instances whose metadata
contains all the key-value pairs specified in the route's `metadata`
configuration will be selected. The metadata values in the route configuration
are arrays, and a service instance matches if its metadata value equals any
value in the corresponding array.
+
+Example: If a service instance has metadata `{lane: "a", env: "prod", version:
"1.0"}`, it will match routes configured with metadata `{lane: ["a"]}` or
`{lane: ["a"], env: ["prod"]}`, but not routes configured with `{lane: ["b"]}`
or `{lane: ["a"], region: ["us"]}`.
+
+Example of routing a request with metadata filtering:
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/5 -H "X-API-KEY: $admin_key"
-X PUT -i -d '
+{
+"uri": "/nacosWithMetadata/*",
+"upstream": {
+"service_name": "APISIX-NACOS",
+"type": "roundrobin",
+"discovery_type": "nacos",
+"discovery_args": {
+ "metadata": {
+"version": ["v1"]
Review Comment:
Hi @beardnick, please take a look.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Baoyuantop commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2321118197
##
docs/en/latest/discovery/nacos.md:
##
@@ -278,3 +279,52 @@ The formatted response as below:
}
}
```
+
+ Metadata filtering
+
+APISIX supports filtering service instances based on metadata. When a route is
configured with metadata conditions, only service instances whose metadata
contains all the key-value pairs specified in the route's `metadata`
configuration will be selected. The metadata values in the route configuration
are arrays, and a service instance matches if its metadata value equals any
value in the corresponding array.
+
+Example: If a service instance has metadata `{lane: "a", env: "prod", version:
"1.0"}`, it will match routes configured with metadata `{lane: ["a"]}` or
`{lane: ["a"], env: ["prod"]}`, but not routes configured with `{lane: ["b"]}`
or `{lane: ["a"], region: ["us"]}`.
+
+Example of routing a request with metadata filtering:
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/5 -H "X-API-KEY: $admin_key"
-X PUT -i -d '
+{
+"uri": "/nacosWithMetadata/*",
+"upstream": {
+"service_name": "APISIX-NACOS",
+"type": "roundrobin",
+"discovery_type": "nacos",
+"discovery_args": {
+ "metadata": {
+"version": ["v1"]
Review Comment:
After careful consideration, I still feel that using an array here will
increase complexity, and there is no such requirement at present. Since the
metadata data comes from nacos, I think we should keep it consistent with the
metadata format of nacos and use a simple key:value pair.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3209937479 @SkyeYoung Please rerun the failed tests. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
SkyeYoung commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2281094276
##
docs/en/latest/discovery/nacos.md:
##
@@ -278,3 +279,52 @@ The formatted response as below:
}
}
```
+
+ Metadata filtering
+
+APISIX supports filtering service instances based on metadata. When a route is
configured with metadata conditions, only service instances whose metadata
contains all the key-value pairs specified in the route's `metadata`
configuration will be selected.
+
+Example: If a service instance has metadata `{lane: "a", env: "prod", version:
"1.0"}`, it will match routes configured with metadata `{lane: "a"}` or `{lane:
"a", env: "prod"}`, but not routes configured with `{lane: "b"}` or `{lane:
"a", region: "us"}`.
+
+Example of routing a request with metadata filtering:
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/5 -H "X-API-KEY: $admin_key"
-X PUT -i -d '
+{
+"uri": "/nacosWithMetadata/*",
+"upstream": {
+"service_name": "APISIX-NACOS",
+"type": "roundrobin",
+"discovery_type": "nacos",
+"discovery_args": {
+ "metadata": {
+"version": "v1"
+ }
+}
+}
+}'
+```
+
+This route will only route traffic to service instances that have the metadata
field `version` set to `v1`.
+
+For multiple metadata criteria:
+
+```shell
+$ curl http://127.0.0.1:9180/apisix/admin/routes/6 -H "X-API-KEY: $admin_key"
-X PUT -i -d '
+{
+"uri": "/nacosWithMultipleMetadata/*",
+"upstream": {
+"service_name": "APISIX-NACOS",
+"type": "roundrobin",
+"discovery_type": "nacos",
+"discovery_args": {
+ "metadata": {
+"lane": "a",
+"env": "prod"
+ }
+}
+}
+}'
+```
Review Comment:
These examples should also be updated according to 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
SkyeYoung commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2281092685
##
docs/en/latest/discovery/nacos.md:
##
@@ -132,6 +132,7 @@ $ curl http://127.0.0.1:9180/apisix/admin/stream_routes/1
-H "X-API-KEY: $admin_
| | -- | --- | --- | - |
|
| namespace_id | string | optional| public | | This parameter is
used to specify the namespace of the corresponding service |
| group_name | string | optional| DEFAULT_GROUP | | This
parameter is used to specify the group of the corresponding service |
+| metadata | object | optional| {} | | Filter service
instances by metadata using containment matching |
Review Comment:
It seems the default value isn't `{}` now, but is being filtered out
directly. Please remove these characters.
##
docs/zh/latest/discovery/nacos.md:
##
@@ -132,6 +132,7 @@ $ curl http://127.0.0.1:9180/apisix/admin/stream_routes/1
-H "X-API-KEY: $admin_
| | -- | --- | --- | - |
|
| namespace_id | string | 可选| public | | 服务所在的命名空间 |
| group_name | string | 可选| DEFAULT_GROUP | | 服务所在的组 |
+| metadata | object | 可选| {} | | 使用包含匹配方式根据元数据过滤服务实例 |
Review Comment:
ditto
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
SkyeYoung commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2281089720
##
apisix/discovery/nacos/init.lua:
##
@@ -319,6 +347,7 @@ local function fetch_full_registry(premature)
host = host.ip,
port = host.port,
weight = host.weight or default_weight,
+metadata = host.metadata,
Review Comment:
Are you sure `metadata` is a `table`? Please assert.
##
apisix/schema_def.lua:
##
@@ -488,7 +488,17 @@ local upstream_schema = {
description = "group name",
type = "string",
},
-}
+metadata = {
+description = "metadata for filtering service instances",
+type = "object",
+additionalProperties = {
+type = "array",
+items = {
+type = "string"
+}
+}
Review Comment:
Should we ensure `uniqueItems=true`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3177451271 > @beardnick Please fix the errors reported in the CI. The failed tests seem unrelated to my PR. Could you please rerun them? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
SkyeYoung commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3173440559 @beardnick Please fix the errors reported in the 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3166608365 > @beardnick Are you planning to implement this in this PR, or in another one? > > If you want to implement it in other PRs, then this PR is ready for review. In this PR. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
SkyeYoung commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3166287675 @beardnick Are you planning to implement this in this PR, or in another one? If you want to implement it in other PRs, then this PR is ready for review. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
beardnick commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3157229362 > After checking #12392 and #12464, I have a question: > > I haven't seen any discussions that should be implemented as single-value matching or multi-value matching? > > #12464 Propose: > > > Currently, someone in Nacos is also implementing the same function #12392 #12445, but only supports single value matching. However, we hope to support multi-value matching. > > This is because user requests often do not have any tags, so if we want to allow multiple instances with different metadata to provide services together, we must register a special metadata to group them, but this grouping cannot be changed dynamically. > > I think it makes sense. > > But I am not an expert in this field, so what do you think? Certainly, that sounds very reasonable. I’m happy to support multi-value matching. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
SkyeYoung commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3148878495 @beardnick ping -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
SkyeYoung commented on code in PR #12445: URL: https://github.com/apache/apisix/pull/12445#discussion_r2244489111 ## apisix/discovery/nacos/init.lua: ## @@ -54,6 +55,23 @@ local function get_key(namespace_id, group_name, service_name) return namespace_id .. '.' .. group_name .. '.' .. service_name end + +local function metadata_contains(host_metadata, route_metadata) +if not route_metadata or not next(route_metadata) then +return true +end +if not host_metadata or not next(host_metadata) then +return false +end + +for k, v in pairs(route_metadata) do +if host_metadata[k] ~= v then +return false +end +end +return true Review Comment: It seems that there is a problem with the logic here: https://github.com/user-attachments/assets/0257ee6e-20b9-4f93-bda3-77d97bac8eb7"; /> `route_metadata` will not be nil or empty -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
SkyeYoung commented on code in PR #12445:
URL: https://github.com/apache/apisix/pull/12445#discussion_r2241982959
##
apisix/discovery/nacos/init.lua:
##
@@ -355,6 +374,19 @@ function _M.nodes(service_name, discovery_args)
return nil
end
local nodes = core.json.decode(value)
+
+-- Apply metadata filtering if specified
+local route_metadata = discovery_args and discovery_args.metadata
+if route_metadata and next(route_metadata) then
+local filtered_nodes = {}
+for _, node in ipairs(nodes) do
+if metadata_contains(node.metadata, route_metadata) then
+core.table.insert(filtered_nodes, node)
+end
+end
+return filtered_nodes
+end
+
Review Comment:
ditto
##
apisix/discovery/nacos/init.lua:
##
@@ -54,6 +55,23 @@ local function get_key(namespace_id, group_name,
service_name)
return namespace_id .. '.' .. group_name .. '.' .. service_name
end
+
+local function metadata_contains(host_metadata, route_metadata)
+if not route_metadata or not next(route_metadata) then
+return true
+end
+if not host_metadata or not next(host_metadata) then
+return false
+end
+
+for k, v in pairs(route_metadata) do
+if host_metadata[k] ~= v then
+return false
+end
+end
+return true
+end
Review Comment:
Can u abstract this func into a separate file for reuse?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
jizhuozhi commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3101148756 > I don't think it's a duplicate at the moment. Using metadata to filter nacos services is a very useful feature. For more information, you can check the related issue. Got it, thanks for you reply :) This is used to filter when pulling the service list from service discovery to avoid pulling a large number of useless instances. We also have similar demands when using Consul. I will try to add the following. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Baoyuantop commented on code in PR #12445: URL: https://github.com/apache/apisix/pull/12445#discussion_r2218119593 ## apisix/discovery/nacos/init.lua: ## @@ -54,6 +55,23 @@ local function get_key(namespace_id, group_name, service_name) return namespace_id .. '.' .. group_name .. '.' .. service_name end + +local function metadata_contains(host_metadata, route_metadata) Review Comment: Could you explain what these two parameters mean? ## t/discovery/nacos.t: ## @@ -1066,3 +1066,124 @@ GET /t --- response_body server 1 server 4 + + + +=== TEST 27: get APISIX-NACOS info from NACOS - metadata filtering lane=a (only server1) +--- yaml_config eval: $::yaml_config +--- apisix_yaml +routes: + - +uri: /hello +upstream: + service_name: APISIX-NACOS + discovery_type: nacos + type: roundrobin + discovery_args: +metadata: + lane: "a" +#END +--- pipelined_requests eval +[ +"GET /hello", +"GET /hello", +"GET /hello", +"GET /hello", +"GET /hello", +] +--- response_body_like eval +[ +qr/server 1/, +qr/server 1/, +qr/server 1/, +qr/server 1/, +qr/server 1/, +] + + + +=== TEST 28: get APISIX-NACOS info from NACOS - metadata filtering lane=b (only server2) Review Comment: This case is somewhat repetitive, I think the configuration can be changed to ``` discovery_args: metadata: ``` Test the behavior of metadata being empty. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
Baoyuantop commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3095073292 > There seems to be duplicate work #12448 (coincidentally, we have similar demands). According to my understanding of apisix, all routes share the same service discovery results, so it is not suitable to filter in the discovery. I don't think it's a duplicate at the moment. Using metadata to filter nacos services is a very useful feature. For more information, you can check the related 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(nacos): add metadata filtering support to nacos discovery [apisix]
jizhuozhi commented on PR #12445: URL: https://github.com/apache/apisix/pull/12445#issuecomment-3095061852 There seems to be duplicate work https://github.com/apache/apisix/pull/12448 (coincidentally, we have similar demands). According to my understanding of apisix, all routes share the same service discovery results, so it is not suitable to filter in the discovery. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
