upstream_rbac: use stats_prefix string to scope filter stats by fl0Lec · Pull Request #45987 · envoyproxy/envoy

fl0Lec · 2026-07-06T07:44:48Z

Commit Message:
upstream_rbac: use stats_prefix string to scope filter stats

Risk Level:
Low

Testing:
Existing unit tests. Integration test coverage TBD.

Docs Changes:
N/A

Release Notes:
Included

Platform Specific Features:
N/A

API Considerations:
N/A — no API changes.

Context

As noted in the review of #45666, the correct pattern for HTTP filter stats scoping is:

The FactoryContext::scope() carries an empty prefix
The stats_prefix string passed to createFilterFactoryFromProto carries the parent's namespace (e.g. http.ingress. for connection manager filters, cluster.X. for cluster upstream filters)
Filters concatenate stats_prefix with their own metric names to produce fully-qualified stat paths

Problem

Two inconsistencies existed:

Router upstream filter chain (router.cc): passed context.scope().prefix() (empty, since the filter-chain scope has no prefix of its own) to FilterChainHelper, so upstream filters received an empty stats_prefix — no context prefix at all.
Cluster upstream filter chain (upstream_impl.cc): passed the cluster scope's prefix string (e.g. cluster.X.) as stats_prefix, while simultaneously passing a scope that already carried the same cluster.X. prefix. Any filter following the standard pattern would produce cluster.X.cluster.X.my_stat — a double-prefixed name. The upstream RBAC filter worked around this by ignoring stats_prefix entirely (EMPTY_STRING), relying solely on the scope prefix.

Fix

router.cc: pass stat_prefix (converted to string, e.g. http.ingress.) as the stats_prefix to FilterChainHelper. Scope is unchanged.
upstream_impl.cc: pass an empty string as stats_prefix since the upstream context scope already carries cluster.<name>.. Filters that emit stats relative to the scope get the right prefix without the string.
upstream_rbac/config.cc: use the received stats_prefix instead of EMPTY_STRING.

Result

Context	Scope prefix	stats_prefix string	RBAC stat
Router upstream	`""`	`http.ingress.`	`http.ingress.rbac.allowed`
Cluster upstream	`cluster.X.`	`""`	`cluster.X.rbac.allowed`

The stats_prefix string passed to createFilterFactoryFromProto carries the parent's namespace (e.g. "http.ingress." for router upstream filters). Upstream filters should use this string to qualify their own stats rather than relying on the scope's prefix, which follows the same convention used by downstream HTTP filters. Changes: - router.cc: pass stat_prefix (converted to string) to FilterChainHelper instead of the scope's empty prefix, so router upstream filters receive a non-empty stats_prefix matching the connection manager's stat prefix. - upstream_impl.cc: pass an empty stats_prefix to FilterChainHelper since the upstream_context_ scope already carries the "cluster.<name>." prefix; passing the same prefix as both a scope prefix and the stats_prefix string would cause filters to emit double-prefixed names. - upstream_rbac/config.cc: use the received stats_prefix instead of EMPTY_STRING so RBAC counters land under the parent's namespace. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

repokitteh-read-only · 2026-07-06T07:44:55Z

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: #45987 was opened by fl0Lec.

see: more, trace.

Add two unit tests to upstream_rbac_filter_test.cc verifying that: - a non-empty stats_prefix (router context) scopes RBAC stats under "http.<stat_prefix>.rbac.*" - an empty stats_prefix with a pre-scoped cluster scope produces "cluster.<name>.rbac.*" and not the double-prefixed variant Add one integration test to upstream_rbac_integration_test.cc that exercises the full Envoy stack and asserts the denied counter exists under "cluster.cluster_0.rbac.denied" while the double-prefixed "cluster.cluster_0.cluster.cluster_0.rbac.denied" remains absent. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

… check Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

FilterChainHelper stores stats_prefix_ as a const std::string&. Passing a string literal ("") creates a temporary whose lifetime ends after the FilterChainHelper constructor returns, leaving stats_prefix_ dangling. ASAN detects this as stack-use-after-scope in generateStats(). Use a named local variable instead, matching the pattern used in router.cc. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

fl0Lec · 2026-07-07T13:07:01Z

/retest

fl0Lec · 2026-07-09T10:53:04Z

cc: @wbpcode as you requested the change on #45666

wbpcode · 2026-07-12T13:44:14Z

-      std::string prefix = stats_scope_->symbolTable().toString(stats_scope_->prefix());
+      // The scope passed via upstream_context_ already carries the "cluster.<name>." prefix,
+      // so pass an empty stats_prefix string. Upstream filters qualify their own stats by
+      // prepending the stats_prefix string to their metric names; passing the scope prefix
+      // again here would produce double-prefixed names (e.g. "cluster.X.cluster.X.rbac.").
+      // Use a named variable: FilterChainHelper stores stats_prefix by reference, so a temporary
+      // (e.g. "") would dangle after the constructor returns.
+      const std::string empty_stats_prefix;
      Http::FilterChainHelper<Server::Configuration::UpstreamFactoryContext,
                              Server::Configuration::UpstreamHttpFilterConfigFactory>
          helper(*http_filter_config_provider_manager_, upstream_context_.serverFactoryContext(),
                 factory_context.serverFactoryContext().clusterManager(), upstream_context_,
-                 prefix);
+                 empty_stats_prefix);


I inclined to unify them by following way:

create a new scope for cluster with empty prefix if there is HTTP filters. Use the cluster.<cluster_name> as the prefix.

All behavior change should be guard by a runtime flag.

BTW, I knew this is fix, but still a behavior change also, it would be better to have a runtime flag.

wbpcode

Thanks for this contribution. I guess now we are in the correct direction. :)

/wait

…r's scope approach Address wbpcode's review feedback: - In upstream_impl.cc: use the server root scope (empty prefix) as the filter context scope and pass "cluster.<name>." explicitly as stats_prefix, mirroring the router case. The old code (scope prefix string re-passed as stats_prefix) is preserved under the legacy branch of the runtime flag. - In router.cc: gate the stat_prefix vs scope.prefix() choice behind the same flag. - Runtime flag: envoy.reloadable_features.upstream_http_filters_correct_stats_prefix (default true). - Tests: add unit tests for all four combinations (new/legacy × router/cluster) and an integration test for the legacy double-prefix behavior with the flag disabled. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

repokitteh-read-only · 2026-07-13T07:45:45Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #45987 was synchronize by fl0Lec.

see: more, trace.

…oid if/else duplication Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…ng root_scope_ctx Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…refix and ECDS lifetime upstream_context_ now always uses serverScope() (root scope, empty prefix) instead of stats_scope_ (which carries "cluster.<name>."). The explicit stats_prefix "cluster.<name>." is passed to FilterChainHelper directly, so upstream HTTP filter stats land under "cluster.<name>.rbac.*" without double-prefixing. This also fixes the ECDS segfault: using a long-lived member as the factory context avoids the dangling reference caused by any local UpstreamFactoryContextImpl being captured beyond the constructor's lifetime. Remove now-obsolete tests for double-prefix and legacy behaviour that no longer exist. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…tats_prefix contract Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…ment Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…etwork filter stat scoping Using serverScope() for upstream_context_ broke network filters that call context.scope() directly (e.g. UpstreamIdleTimeoutVerifierFilter): their stats landed at root scope instead of under 'cluster.<name>.'. Fix by keeping upstream_context_ on stats_scope_ (which carries the cluster prefix) and passing EMPTY_STRING as stats_prefix to FilterChainHelper for HTTP filters — the scope's own prefix already scopes the stats correctly. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…erConfigFactory Add a stats_prefix parameter to createFilterFactoryFromProto on NamedUpstreamNetworkFilterConfigFactory, mirroring the upstream HTTP filter API. This allows upstream_impl.cc to pass 'cluster.<name>.' explicitly so network filters can emit stats under the cluster namespace even when upstream_context_ uses root scope. A 2-arg convenience overload is kept for the ECDS dynamic path which hasn't been updated to thread stats_prefix through yet (TODO in UpstreamNetworkDynamicFilterConfigProviderImpl). Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…erride The 2-arg convenience overload on NamedUpstreamNetworkFilterConfigFactory already handles the base class call in NetworkDynamicFilterConfigProviderImplBase. The override in config_discovery_impl.h was doing exactly the same thing. Move the TODO to filter_config.h where the actual gap lives. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…re with default delegation The 2-arg createFilterFactoryFromProto remains PURE so all existing implementations continue to compile without any change. The new 3-arg overload (with stats_prefix) is non-pure virtual and defaults to calling the 2-arg form, allowing callers like upstream_impl to pass an explicit "cluster.<name>." prefix while backward-compatible filters (including reverse_tunnel) need not be touched. Reverts the reverse_tunnel signature change from the previous commit. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…flag The switch of upstream_context_ from the cluster-scoped stats_scope_ to the server root scope changes emitted stat names for existing upstream HTTP filters (e.g. admission_control), which combine context.scope() with the stats_prefix and were therefore double-prefixed as "cluster.<name>.cluster.<name>.*". Gate the switch behind the same envoy.reloadable_features.upstream_http_filters_correct_stats_prefix flag used by the router path so the change is reversible: when disabled, upstream_context_ keeps stats_scope_ and the legacy stringified scope prefix is passed as stats_prefix, preserving existing stat names exactly. Also hoist the duplicated absl::StrCat("cluster.", name_, ".") into a single cluster_stats_prefix local shared by the network and HTTP filter chains, and omit the unused stats_prefix parameter name in the default 3-arg overload. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

…ilters Replaces the shared-context + network-filter stats_prefix overload approach with two factory contexts on ClusterInfoImpl: - upstream_context_ stays scoped to stats_scope_ ("cluster.<name>."), used by upstream network filters which scope stats via context.scope() and have no stats_prefix param. - http_upstream_context_ is scoped to the server root scope when envoy.reloadable_features.upstream_http_filters_correct_stats_prefix is enabled (and to stats_scope_ otherwise), used by the upstream HTTP filter chain, which receives an explicit "cluster.<name>." stats_prefix via FilterChainHelper. This gives upstream HTTP filters the router pattern (root scope + explicit stat prefix) without adding a stats_prefix overload to NamedUpstreamNetworkFilterConfigFactory. That overload caused -Woverloaded-virtual (-Werror) in every subclass overriding only the 2-arg form (reverse_tunnel and several test doubles). NamedUpstreamNetworkFilterConfigFactory is back to a single 2-arg pure virtual, reverse_tunnel is untouched, and the idle_timeout test filter is restored to its original form. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

fl0Lec · 2026-07-14T14:27:01Z

/retest

Simplify the upstream HTTP filter stat scoping: reuse the single cluster-scoped upstream_context_ for both the network and HTTP filter chains, and pass an empty stats_prefix to the HTTP chain when the correct-stats-prefix flag is enabled. upstream_context_ is already scoped to "cluster.<name>.", so an empty stats_prefix yields correctly namespaced "cluster.<name>.*" stats without the previous repeated prefix. When the flag is disabled, the legacy stringified scope prefix is passed, preserving existing (repeated-prefix) stat names. This drops the separate http_upstream_context_ / empty-prefix scope members in favor of the existing context, keeping the change minimal. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

fl0Lec · 2026-07-15T08:04:31Z

/retest

fl0Lec · 2026-07-15T12:46:12Z

Thanks for the review @wbpcode. I reworked the upstream HTTP filter stat scoping; here's where it landed and the reasoning, since it differs a bit from your suggestion.

On your suggestion (new empty-prefix scope + cluster. as the explicit prefix), the catch is that network and HTTP filters share the same UpstreamFactoryContext, and network filters have no stats_prefix parameter, so to give the HTTP chain a different (root/empty) scope than the network chain, I had to either:

add a stats_prefix overload to NamedUpstreamNetworkFilterConfigFactory — which tripped -Woverloaded-virtual under -Werror in every subclass that overrides only the 2-arg form (reverse_tunnel + several test doubles)
introduce a second UpstreamFactoryContextImpl plus a cluster-owned empty-prefix scope member

Both add moving parts for output that's identical to just emptying the stats_prefix. And because upstream_context_'s scope already exposes the correctly-sanitized cluster.., reusing it (rather than rebuilding the prefix string) keeps the sanitized name for free, with no new scope/context and no stat-lifetime question.

I'm happy to switch to the explicit empty-scope version if you'd prefer it for consistency with the router pattern, just wanted to flag that it produces the same stats at the cost of the extra context (or the network-filter overload).

…ats-prefix # Conflicts: # source/common/runtime/runtime_features.cc Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

fl0Lec · 2026-07-21T13:00:09Z

/retest

paul-r-gall · 2026-07-21T14:27:17Z

ping @wbpcode

wbpcode · 2026-07-24T13:36:35Z

I am OK to go ahead with current version. We could get the correct output stats at both versions anyway. We can defer that to future if necessary.

The RBAC filter now consumes the parent stats_prefix, which is a behavior change, so gate it behind envoy.reloadable_features.upstream_http_filters_correct_stats_prefix. When the flag is disabled, fall back to the previous empty prefix. Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

fl0Lec added 4 commits July 6, 2026 10:04

upstream_rbac_integration_test: use waitForCounter for stat existence…

7cecd7e

… check Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

upstream_rbac_filter_test: fix clang-format on EXPECT_FALSE line

50dd71a

Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

fl0Lec marked this pull request as ready for review July 8, 2026 08:42

fl0Lec requested a review from yanavlasov as a code owner July 8, 2026 08:42

fl0Lec mentioned this pull request Jul 8, 2026

router: scope upstream HTTP filter chain to the connection manager stat prefix #45666

Closed

ggreenway assigned wbpcode Jul 9, 2026

wbpcode reviewed Jul 12, 2026

View reviewed changes

Comment thread source/common/router/router.cc Outdated

repokitteh-read-only Bot added the waiting label Jul 12, 2026

repokitteh-read-only Bot removed the waiting label Jul 13, 2026

fl0Lec added 7 commits July 13, 2026 07:53

upstream_rbac: shorten changelog and refactor FilterChainHelper to av…

66dfbab

…oid if/else duplication Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

upstream_impl: simplify filter context selection by always constructi…

ffcf470

…ng root_scope_ctx Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

upstream_rbac: update stale comment in config.cc to reflect correct s…

b932576

…tats_prefix contract Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

upstream_impl: drop misleading router reference from stats_prefix com…

1364d81

…ment Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

fl0Lec mentioned this pull request Jul 13, 2026

upstream_network_filter: add stats_prefix to NamedUpstreamNetworkFilterConfigFactory DataDog/envoy#37

Merged

fl0Lec added 3 commits July 13, 2026 15:01

wbpcode reviewed Jul 14, 2026

View reviewed changes

Comment thread source/common/upstream/upstream_impl.cc Outdated

Comment thread source/common/upstream/upstream_impl.cc Outdated

fl0Lec requested a review from wbpcode July 16, 2026 11:59

Merge remote-tracking branch 'upstream/main' into fl/upstream-rbac-st…

12e3a18

…ats-prefix # Conflicts: # source/common/runtime/runtime_features.cc Signed-off-by: Florent Lecoultre <florent.lecoultre@datadoghq.com>

wbpcode approved these changes Jul 24, 2026

View reviewed changes

wbpcode requested changes Jul 24, 2026

View reviewed changes

Comment thread source/extensions/filters/http/upstream_rbac/config.cc Outdated

fl0Lec requested a review from wbpcode July 24, 2026 14:57

Uh oh!

Conversation

fl0Lec commented Jul 6, 2026

Context

Problem

Fix

Result

Uh oh!

repokitteh-read-only Bot commented Jul 6, 2026

Uh oh!

fl0Lec commented Jul 7, 2026

Uh oh!

fl0Lec commented Jul 9, 2026

Uh oh!

wbpcode Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Jul 13, 2026

Uh oh!

Uh oh!

Uh oh!

fl0Lec commented Jul 14, 2026

Uh oh!

fl0Lec commented Jul 15, 2026

Uh oh!

fl0Lec commented Jul 15, 2026

Uh oh!

fl0Lec commented Jul 21, 2026

Uh oh!

paul-r-gall commented Jul 21, 2026

Uh oh!

wbpcode commented Jul 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants