Populate typed metadata in proxy protocol filter by default

[nezdolik]

Commit Message: Additional Description: Populate typed metadata in proxy protocol filter by default (with runtime flag) together with untyped metadata. Continue sanitizing non utf8 chars in untyped metadata. Risk Level: Testing: Unit tests Docs Changes: Release Notes: TBD Platform Specific Features: NA Runtime guard: envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener Fixes #32718

[repokitteh-read-only[bot]]

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc). CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/). envoyproxy/api-shepherds assignee is @htuch CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/33146 was opened by nezdolik.

see: more, trace.

[nezdolik]

cc @yanavlasov @ggreenway

[soulxu]

/wait for update

[nezdolik]

@htuch I was not familiar with dynamic typed metadata, looks like the implementation would have been simpler with dynamic typed metadata. @ggreenway @yanavlasov any thoughts/preferences which one should be used?

[htuch]

https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/data_sharing_between_filters.html#filter-state-sharing provides the guidance on this. I think it's not a readable doc these days and could benefit from a table summary and more actionable guidance. Probably I would default to using filter state for this if there was no need to consume in places that filter state doesn't work.

[alhuan]

:wave: Potentially dumb question (I'm not really an Envoy expert) but aren't there several filters that could legitimately use this output that have access to metadata but not filter state? Some examples are routing in HTTP Connection Manager (https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-routematch) and ext_authz (https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_authz/v3/ext_authz.proto#envoy-v3-api-field-extensions-filters-http-ext-authz-v3-extauthz-metadata-context-namespaces) which both seem like significant usecases for this feature. Is filter state accessible there?

Also quick note that the runtime feature flag is misspelled: use_typed_metadata_in_proxy_protol_listener misspells protocol as protol.

Thanks!

[htuch]

Yeah, you're probably right @alhuan. Let's go with this as is.

[htuch]

@nezdolik looks like a legit CI failure from this PR.

[nezdolik]

Just realized as well that old behavior should remain unchanged, e.g. when flag is false and string values are used we should still sanitize them. Otherwise this will revert the security fix that was rolled out before.

[htuch]

+1, any existing behavior should just move under runtime guard, no semantic changes in this PR.

[nezdolik]

@ggreenway would you mind taking a look?

[alhuan]

@nezdolik Is this fix still planned for inclusion in the next minor release of Envoy? I hope this isn't a rude question to ask; I just noticed there hasn't seemed to be activity on this PR for a couple weeks and was wondering if it was still in-progress or if someone else could help out. Thank you!

[nezdolik]

/retest

[nezdolik]

/retest

[nezdolik]

@ggreenway @yanavlasov this is ready for another review.

[nezdolik]

/retest

[phlax]

codeql fail is ubuntu ppa server down (which seems to be happening with increasing frequency)

[nezdolik]

tsan flake in unrelated test QuicHttpIntegrationTests/QuicHttpIntegrationTest.UsesPreferredAddressDNAT/IPv6

[nezdolik]

/retest

[nezdolik]

@yanavlasov could you please take another look

[ggreenway]

@htuch can you review this on behalf of @envoyproxy/api-shepherds ?

[nezdolik]

@ggreenway is this patch fine to merge now?