metadata: supported to access specific item of ListValue by the MetadataKey

[yusofg2]

Commit Message:
The grpc extraction filter always writes dynamic metadata as a ListValue, even for singular scalar fields. The ratelimit filter's MetaDataAction calls .string_value() on whatever Value it finds; calling .string_value() on a ListValue returns "" (wrong oneof arm), causing populateDescriptor() to return false and silently disabling rate limiting for every gRPC request.

Add a ValueType enum to RequestFieldValueDisposition (LIST = default, STRING = opt-in). When STRING is set, the filter promotes list_value().values(0) to a StringValue before writing to metadata, making the value consumable by the ratelimit filter's metadata descriptor action without a Lua bridge.

LIST remains the default for backward compatibility. STRING is only correct when every segment of the field path is non-repeated; the proto comment documents the repeated-field undercounting hazard explicitly.

Co-Authored-By: Claude Sonnet 4.6 (1M context) noreply@anthropic.com

Additional Description:

gRPC Field Extraction ↔ Rate Limit Filter Incompatibility

envoy.filters.http.grpc_field_extraction and envoy.filters.http.ratelimit are
incompatible by default. When a gRPC field is extracted and the ratelimit filter
is configured to use it as a descriptor, rate limiting is silently disabled for every
request — the ratelimit service is never called.

This is not a configuration error. It is a type mismatch baked into both filters.

---

Root Cause

What grpc_field_extraction writes

The extraction filter uses the proto_field_extraction library's ExtractValue(),
which always returns a Protobuf::Value with kind = kListValue on success —
even for singular scalar fields. For a field tenant_id: "acme" the metadata entry
written to envoy.filters.http.grpc_field_extraction is:

{ "tenant_id": ["acme"] }   // ListValue wrapping a single StringValue element

What ratelimit reads

MetaDataAction::populateDescriptor in
source/common/router/router_ratelimit.cc:241:

const std::string metadata_string_value =
    Envoy::Config::Metadata::metadataValue(metadata_source, metadata_key_).string_value();

if (!metadata_string_value.empty()) {
    descriptor_entry = {descriptor_key_, metadata_string_value};
    return true;
} else if (metadata_string_value.empty() && !default_value_.empty()) {
    descriptor_entry = {descriptor_key_, default_value_};
    return true;
}
return skip_if_absent_;   // defaults to false

metadataValue(...).string_value() calls the protobuf string_value() accessor
on a ListValue. Protobuf Value is a oneof — calling the wrong arm returns the
zero value, which for strings is "".

The failure chain

Step	What happens
1	grpc_field_extraction writes ListValue { values: ["acme"] } to dynamic metadata
2	MetaDataAction::populateDescriptor calls .string_value() on the ListValue
3	Protobuf returns "" (wrong oneof arm)
4	metadata_string_value is ""
5	Falls through to default_value_ check — also empty (no default configured)
6	Returns skip_if_absent_ which is false by default
7	populateDescriptor returns false — the entire rate limit check is aborted
8	The ratelimit service is never called; the request passes through without limit

The outcome is worse than "rate limiting doesn't fire." With skip_if_absent: false
(the default), rate limiting is completely disabled for every affected request.
With skip_if_absent: true the descriptor is silently skipped, which is less bad
but still means the field has no effect on rate limiting.

---

Options Considered

Option A — value_type: STRING in grpc_field_extraction (implemented in this PR)

Add a value_type enum field to RequestFieldValueDisposition in the filter's
proto. When set to STRING, the filter promotes the first ListValue element to
a StringValue before writing to metadata. The ratelimit filter then reads a
StringValue and .string_value() returns the correct value.

Config:

request_field_extractions:
  tenant_id:
    value_type: STRING    # opt-in; LIST is the default

What gets written to metadata:

{ "tenant_id": "acme" }   // StringValue — ratelimit reads this correctly

Compatibility matrix:

Consumer	LIST (default)	STRING (opt-in)
ratelimit metadata descriptor	❌ silently broken	✅ works
ext_authz forwarding	✅	✅
rbac policy matching	✅	✅
Lua scripts	✅	✅
Access logs %DYNAMIC_METADATA%	JSON array	bare string
WASM extensions	✅	✅

Limitations:

• STRING mode takes only values(0), silently discarding all subsequent elements.
  This is correct for singular scalar fields (tenant_id, key.display_name) but
  causes rate-limit undercounting for repeated fields: a request touching N values
  is charged only against the first value's bucket.
• The safe rule: value_type: STRING is correct if and only if every segment of the
  field path is singular (non-repeated) in the proto definition.
• If both the full list (for ext_authz) and a string (for ratelimit) are needed
  from the same field, a Lua bridge is required — see Option C.

Code locations:

• Proto definition: api/envoy/extensions/filters/http/grpc_field_extraction/v3/config.proto
• Implementation: source/extensions/filters/http/grpc_field_extraction/extractor_impl.cc
• Header (parallel map): source/extensions/filters/http/grpc_field_extraction/extractor_impl.h
• Tests: test/extensions/filters/http/grpc_field_extraction/filter_test.cc

---

Option B — Fix MetaDataAction in the ratelimit filter to unwrap ListValue

Modify router_ratelimit.cc:242 to check kind_case() and unwrap values(0) when
the value is a ListValue:

// hypothetical change
const Protobuf::Value& raw = Envoy::Config::Metadata::metadataValue(...);
const std::string metadata_string_value =
    raw.kind_case() == Protobuf::Value::kListValue && raw.list_value().values_size() > 0
        ? raw.list_value().values(0).string_value()
        : raw.string_value();

Would it break anything?
In practice, no. Any existing consumer hitting a ListValue from a metadata field
is already receiving "" and their rate limiting is already silently broken. There
is nothing to regress.

Problems with this approach:

• Implicit behavior change. An operator reading
  metadata: { descriptor_key: tenant_id } sees no signal that list-unwrapping
  is happening. The extraction filter has the proto context; the ratelimit filter
  does not.
• Mixes concerns. The ratelimit filter becomes implicitly aware of gRPC
  extraction semantics. The extraction filter is the right place to own value-type
  decisions because it knows whether the field is singular or repeated.
• Still has the repeated-field undercounting problem — taking values(0)
  silently drops the rest, same as Option A, but now without even a config signal
  to warn the operator.

---

Option C — Ratelimit filter fan-out over ListValue elements

A more ambitious extension: when MetaDataAction sees a ListValue, emit one
RateLimitDescriptor per element. For a request with tenant_ids: ["a", "b", "c"],
three descriptors would be submitted, charging each bucket.

This would solve the repeated-field undercounting problem that both A and B
cannot address. But:

• It changes the number of descriptors emitted per request — a breaking change to
  the ratelimit service wire protocol (lyft/ratelimit expects a specific number of
  descriptors per call).
• The ratelimit service itself would need changes to handle fan-out correctly
  (deduplicate hits, etc.).
• Much larger scope; not achievable within the extraction filter.

Verdict: Maybe a long-term solution for repeated-field rate limiting, but a
different and much larger feature. Out of scope for the current spike.

---

Option D — Lua bridge for dual-consumer use case

When the same extracted field must be consumed by both the ratelimit filter
(needs StringValue) and another filter (needs the full ListValue):

1. Extract with value_type: LIST (full list preserved).
2. Insert a Lua filter after grpc_field_extraction that reads the list and writes
   a separate string key for the ratelimit filter:

local meta = request_handle:streamInfo():dynamicMetadata()
local ns = "envoy.filters.http.grpc_field_extraction"
local extracted = meta:get(ns)
if extracted and extracted["tenant_id"] then
  local list = extracted["tenant_id"]
  if #list > 0 then
    request_handle:streamInfo():dynamicMetadata():set(
      "ratelimit.extracted", "tenant_id", list[1])
  end
end

The ratelimit descriptor then points at namespace ratelimit.extracted instead of
envoy.filters.http.grpc_field_extraction.

When this is needed: Only when the same field must feed both ratelimit
(string) and ext_authz / RBAC / access logs (full list). For pure GRLS pipelines,
value_type: STRING (Option A) is sufficient.

---

Field Path Compatibility Reference

Field path	Path is safe for STRING?	Notes
tenant_id	✅	Top-level singular scalar
key.display_name	✅	All segments singular
key.name	✅	All segments singular
supported_types.string	✅	Singular nested message, singular terminal field
repeated_supported_types.string	❌	Terminal field is repeated
keys.name	❌	Intermediate keys is repeated
key.tags	❌	Terminal field is repeated

Safe rule: value_type: STRING is correct iff every segment of the dot-separated
path is declared singular (not repeated) in the proto definition.

---

Risk Level: Low

Testing:
To validate the fix end-to-end, a local test environment was assembled with the patched Envoy binary running as a Docker container fronting a gRPC echo service, with a Lyft ratelimit (GRLS) instance backed by Redis. A test script
drives gRPC requests through the full filter chain — grpc_field_extraction (with value_type: STRING) followed by the ratelimit filter's metadata descriptor action — with no Lua filter present. The script verifies that early requests
succeed and that once the configured limit is reached the ratelimit filter correctly rejects subsequent requests, confirming that the extracted field value reached GRLS and was acted on. A secondary check queries Redis directly to
confirm GRLS wrote a counter key for the descriptor, ruling out any false-positive from failure_mode_deny: false silently passing requests through. As a negative control, the same test was run against an unpatched Envoy with the
default LIST extraction mode, where all requests succeed, GRLS receives no ShouldRateLimit calls, and Redis remains empty — confirming the incompatibility is the default behavior and the fix is what enables the integration.

Docs Changes: N/A
Release Notes: Added changelog fragment.
Platform Specific Features: None

[repokitteh-read-only]

Hi @yusofg2, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #45948 was opened by yusofg2.

see: more, trace.

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #45948 was opened by yusofg2.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #45948 was ready_for_review by yusofg2.

see: more, trace.

[wbpcode]

Thanks for this contribution. I guess this two different parts to optimize:

• extraction: I inclined to keep the value that https://github.com/grpc-ecosystem/proto-field-extraction lib extracted. If we want to support simple value (at non-list format), we'd better to create PR at upstream.
• access: this part is what we can control in our code base. We could enhance the message MetadataKey to support to access list value. Then, we can let the rate limit action access the string value from a list (I can take a check, this should be pretty simple).

/wait

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).

🐱

Caused by: #45948 was synchronize by yusofg2.

see: more, trace.

[yusofg2]

Thanks for this contribution. I guess this two different parts to optimize:

• extraction: I inclined to keep the value that https://github.com/grpc-ecosystem/proto-field-extraction lib extracted. If we want to support simple value (at non-list format), we'd better to create PR at upstream.
• access: this part is what we can control in our code base. We could enhance the message MetadataKey to support to access list value. Then, we can let the rate limit action access the string value from a list (I can take a check, this should be pretty simple).

/wait

Thanks for the feedback @wbpcode! I've implemented your suggested approach - adding index support to PathSegment so the metadata access layer can unwrap list values.

What Changed

MetadataKey now supports list element access:

message PathSegment {
  oneof segment {
    string key = 1;
    uint32 index = 2;  // NEW: access ListValue elements
  }
}

Usage example (ratelimit consuming grpc_field_extraction):

metadata_key:
  key: envoy.filters.http.grpc_field_extraction
  path:
  - key: tenant_id
  - index: 0  # unwrap the ListValue

Implementation details:

• Added PathSegment type enum (Key/Index) with bounds checking
• Out-of-bounds index returns empty Value (same as missing key)
• Debug logging for troubleshooting
• Comprehensive unit tests (100% coverage)
• Documentation added to grpc_field_extraction proto explaining the ListValue behavior

All grpc_field_extraction code changes reverted. Committed as 48e52cb1ad.

---

Optional Enhancement: Auto-unwrap Single-Element Lists

One ergonomic concern: every grpc_field_extraction consumer must add - index: 0 for singular fields. We could auto-unwrap single-element ListValues at path traversal end:

// In structValue() after path traversal:
if (val->kind_case() == Protobuf::Value::kListValue && val->list_value().values_size() == 1) {
  return val->list_value().values(0);  // Auto-unwrap
}

Pros:

• Ergonomic - singular fields don't need explicit - index: 0
• Handles the common case (90%+ of grpc_field_extraction usage)

Cons:

• Affects ALL metadata consumers, not just grpc_field_extraction - any filter that writes single-element ListValues would be auto-unwrapped
• Semantic change - "single-element ListValue is never a valid terminal value"
• Could break existing configs that intentionally access ListValue wrapper
• Only helps single-element lists (not actual repeated fields)

Happy to add this if you think the ergonomics justify the magic. Otherwise, current implementation is ready to merge.

[paul-r-gall]

ping @wbpcode for review

[wbpcode]

Don't change this v2 API.

[yusofg2]

Good catch — reverted. v2 is FROZEN, so it shouldn't gain the index field. v3 already has it, which is what the new MetadataKey code path uses.

[wbpcode]

I don't think this type_ make sense. If non-empty key is there, we should treat it as key, or then try the index.

[yusofg2]

Agreed — removed PathSegment::Type entirely. Segment kind is now inferred from !key_.empty(). This is safe because the v3 proto's key oneof arm has (validate.rules).string = {min_bytes: 1}, so a key segment can never legitimately be empty.

[wbpcode]

Thanks for the update. I added some comments to this PR.

[wbpcode]

I guess we should enhance this method to support const std::vector<PathSegment>& path?

And for the unused dead code, like

const Protobuf::Value& Metadata::structValue(const Protobuf::Struct& struct_value,
                                             const std::vector<std::string>& path)

Please do a clean up.

[yusofg2]

Done — structValue(Struct, vector<string>) now converts each string into a key-only PathSegment and delegates to structValue(Struct, vector<PathSegment>), so there's no more duplicated struct-walking logic. I kept the vector<string> overload's public signature since it's still used directly by transform.cc (and its tests) — but it's now a thin wrapper rather than a parallel implementation.

[wbpcode]

Thanks for the update. Sorry for the regression of the review comment!

[wbpcode]

This may may the code patch who calling structValue(Struct, vector<string>) be super slow. my initially thought is to refactor all call sites to use new MetadataKey rather then vector<string> and then remove the legacy vector<string> related code.

[wbpcode]

But fine, let's revert this change and keep the previous version. We can do the clean up at follow up and needn't to make everything perfect in this PR.

[yusofg2]

Reverted — restored the original standalone structValue(Struct, vector<string>) implementation, so the hot call sites keep their zero-allocation walk and there's no perf regression. Agreed on deferring the real cleanup (migrating callers to MetadataKey and removing the legacy vector<string> path) to a follow-up PR.

[wbpcode]

/wait

[yusofg2]

@wbpcode I addressed the comments, please take a look. Thank you!

[wbpcode]

LGTM. Thanks.

[wbpcode]

Seems the CI failures are real

[yusofg2]

Seems the CI failures are real

You're right, @wbpcode — thanks. Root cause: MetadataKey::path_ changed type from std::vector<std::string> to std::vector<PathSegment>, but addSelectedHostKey() in the override_host load balancer was still indexing path_ as strings, which broke the config_test build and cascaded to all checks. Fixed in 470141207d (2 lines, using PathSegment::key_ since that write-path only ever addresses struct fields). Verified the previously-uncompiled target builds clean locally.

Could you re-authorize the CI run when you get a chance? It's stuck on the authorize gate for the new commit. Thanks!