Copilot commented on code in PR #12448: URL: https://github.com/apache/apisix/pull/12448#discussion_r2917012002
########## docs/en/latest/discovery/consul.md: ########## @@ -232,6 +232,61 @@ $ curl http://127.0.0.1:9180/apisix/admin/stream_routes/1 -H "X-API-KEY: $admin_ }' ``` +### discovery_args + +| Name | Type | Requirement | Default | Valid | Description | +|----------------| ------ | ----------- | ------- | ----- | ------------------------------------------------------------ | +| metadata | object | optional | {} | | Filter service instances by metadata using containment matching | Review Comment: The markdown table under `### discovery_args` starts rows with `||`, which renders as an extra empty column in most markdown parsers. It should start with a single leading `|` for each row (header, separator, and data rows). ########## docs/en/latest/discovery/consul.md: ########## @@ -232,6 +232,61 @@ $ curl http://127.0.0.1:9180/apisix/admin/stream_routes/1 -H "X-API-KEY: $admin_ }' ``` +### discovery_args + +| Name | Type | Requirement | Default | Valid | Description | +|----------------| ------ | ----------- | ------- | ----- | ------------------------------------------------------------ | +| metadata | object | optional | {} | | Filter service instances by metadata using containment matching | + +#### Metadata filtering + +APISIX supports filtering service instances based on metadata. When a route is configured with metadata conditions, only service instances whose metadata matched with roles 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", "b"], env: "prod"}`, but not routes configured with `{lane: ["c"]}` or `{lane: "a", region: "us"}`. Review Comment: The docs/examples here use `discovery_args.metadata` but the wording says “route’s `metadata` configuration”, which is misleading (this is upstream `discovery_args`). Also, the example `env: "prod"` is a string, while the schema expects an array of strings for every key (eg `env: ["prod"]`). Please align the wording and examples with the actual config shape. ```suggestion APISIX supports filtering service instances based on metadata. When an upstream is configured with metadata conditions via `discovery_args.metadata`, only service instances whose metadata matches the values specified in `discovery_args.metadata` will be selected. Example: If a service instance has metadata `{lane: "a", env: "prod", version: "1.0"}`, it will match upstreams configured with `discovery_args.metadata` `{lane: ["a"]}` or `{lane: ["a", "b"], env: ["prod"]}`, but not upstreams configured with `{lane: ["c"]}` or `{lane: ["a"], region: ["us"]}`. ``` ########## apisix/utils/discovery.lua: ########## @@ -0,0 +1,66 @@ +-- +-- 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. +-- +local core = require("apisix.core") +local ipairs = ipairs +local pairs = pairs + +local _M = {} + +local function do_metadata_match(node, expected_metadata) + local metadata = node.metadata + -- because metadata has already been checked in nodes_metadata, + -- there is at least one role, if there is no metadata in node, it's must not matched Review Comment: Comment grammar reads awkwardly (“it's must not matched”). Please correct the wording so the intent is clear. ```suggestion -- there is at least one role, so if a node has no metadata, it cannot match ``` ########## apisix/utils/discovery.lua: ########## @@ -0,0 +1,66 @@ +-- +-- 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. +-- +local core = require("apisix.core") +local ipairs = ipairs +local pairs = pairs + +local _M = {} + +local function do_metadata_match(node, expected_metadata) + local metadata = node.metadata + -- because metadata has already been checked in nodes_metadata, + -- there is at least one role, if there is no metadata in node, it's must not matched + if not metadata then + return false + end + for key, values in pairs(expected_metadata) do + local matched = false + for _, value in ipairs(values) do + if metadata[key] == value then + matched = true + break + end + end + if not matched then + return false + end + end + return true +end + +local function nodes_metadata(nodes, metadata) + if not nodes then + return nil + end + + -- fast path: there is not metadata roles, all nodes are available, + -- and make a guarantee for do_metadata_match: at least one role + if not metadata then + return nodes + end Review Comment: `nodes_metadata` treats an empty table (`{}`) as an active filter, which causes an unnecessary allocation + per-request table churn while still matching all nodes (because `pairs({})` is empty). Consider treating empty metadata as “no filter” (eg `if not metadata or next(metadata) == nil then return nodes end`) and adjust the comment that claims “at least one role” accordingly. ########## apisix/schema_def.lua: ########## @@ -493,6 +493,18 @@ local upstream_schema = { description = "group name", type = "string", }, + metadata = { + description = "metadata for filtering service instances", Review Comment: This change introduces `discovery_args.metadata` on the generic upstream schema, but only Consul appears to apply it in `dis.nodes(...)` right now. Consider clarifying in the schema description (or docs) that this field is currently only effective for specific discovery types (eg `consul`) to avoid configs being accepted but ignored for other discovery backends. ```suggestion description = "metadata for filtering service instances, only effective for specific discovery types (e.g., consul)", ``` ########## apisix/schema_def.lua: ########## @@ -493,6 +493,18 @@ local upstream_schema = { description = "group name", type = "string", }, + metadata = { + description = "metadata for filtering service instances", + type = "object", + additionalProperties = { + type = "array", + items = { + description = "candidate metadata value", + type = "string", + }, Review Comment: The new `discovery_args.metadata` schema allows empty arrays for a key (eg `{"version": []}`), which will silently filter out *all* nodes at runtime. If that’s not intentional, add `minItems = 1` (and potentially `minProperties = 1` at the object level) so invalid filters are rejected at validation time. ```suggestion type = "object", minProperties = 1, additionalProperties = { type = "array", items = { description = "candidate metadata value", type = "string", }, minItems = 1, ``` ########## docs/en/latest/discovery/consul.md: ########## @@ -232,6 +232,61 @@ $ curl http://127.0.0.1:9180/apisix/admin/stream_routes/1 -H "X-API-KEY: $admin_ }' ``` +### discovery_args + +| Name | Type | Requirement | Default | Valid | Description | +|----------------| ------ | ----------- | ------- | ----- | ------------------------------------------------------------ | +| metadata | object | optional | {} | | Filter service instances by metadata using containment matching | + +#### Metadata filtering + +APISIX supports filtering service instances based on metadata. When a route is configured with metadata conditions, only service instances whose metadata matched with roles 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", "b"], env: "prod"}`, but not routes configured with `{lane: ["c"]}` 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": "/consulWithMetadata/*", + "upstream": { + "service_name": "APISIX-CONSUL", + "type": "roundrobin", + "discovery_type": "consul", + "discovery_args": { + "metadata": { + "version": ["v1", "v2"] + } + } + } +}' +``` + +This route will only route traffic to service instances that have the metadata field `version` set to `v1` or `v2`. + +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": "/consulWithMultipleMetadata/*", + "upstream": { + "service_name": "APISIX-CONSUL", + "type": "roundrobin", + "discovery_type": "consul", + "discovery_args": { + "metadata": { + "lane": ["a"], + "env": ["prod"] + } + } + } +}' +``` + +This route will only route traffic to service instances that have both `lane: "a"` and `env: "prod"` in their metadata. + You could find more usage in the `apisix/t/discovery/stream/consul.t` file. Review Comment: The reference path `apisix/t/discovery/stream/consul.t` doesn’t exist in the repo; the tests live under `t/discovery/stream/consul.t`. Update this link/path so readers can actually find the example. ```suggestion You could find more usage in the `t/discovery/stream/consul.t` file. ``` ########## apisix/discovery/consul/init.lua: ########## @@ -527,12 +536,23 @@ function _M.connect(premature, consul_server, retry_delay) end -- not store duplicate service IDs. local service_id = svc_address .. ":" .. svc_port + -- ensure that metadata is an accessible table, + -- avoid `null` returned by cjson + if metadata == cjson_null then + metadata = nil + elseif type(metadata) ~= "table" then + log.error("service ", service_id, + " has invalid metadata, use nil as default: ", + json_delay_encode(metadata)) + metadata = nil + end if not nodes_uniq[service_id] then -- add node to nodes table core.table.insert(nodes, { host = svc_address, port = tonumber(svc_port), weight = default_weight, + metadata = metadata }) Review Comment: The PR description mentions “respect its `weight` if available (aligned with Eureka)”, but nodes are still created with `weight = default_weight` unconditionally. If Consul service meta is expected to carry a weight value (eg `metadata.weight`), it should be applied here (and removed from `metadata` like Eureka does) or the description/docs should be updated to avoid implying weight support. -- 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]
