Envoy: listener: deprecate use_proxy_proto

[tbarrella]

Title: Check all filter chains before injecting proxy proto listener filter deprecate use_proxy_proto

Description: See discussion here

[lambdai]

Background
use proxy proto is currently per filter chain config. The current behavior is to assume all filter chains in the same listener has the same value: either all true or all false.

It's doesn't make much sense if the owning listener has filter chain request proxy proto while other filter chain don't.

We should deprecate this field in FilterChain, WDTY?

CC @mattklein123 @htuch

[mattklein123]

Yes, we should deprecate it, but I'm more concerned about whether we block it or not right now. Do we have a check that it's all or none? If not can we add it behind a runtime guard?

[tbarrella]

Incidentally, it seems this behavior has been around for a while, it looks like since #1471 if I'm interpreting the code right

[lambdai]

+1 for runtime guard and add warning message

[ggreenway]

I'd say don't change the behavior, deprecate it, and make sure docs show how to add it to the listener filters explicitly.

[mattklein123]

My concern is the existing behavior makes absolutely no sense. How can it work at all? I'm fine with whatever approach we want, definitely deprecation, but I'm also potentially in favor of warn/fail on mismatch behind a runtime flag.

[ggreenway]

I agree it makes no sense, but changing it has the risk of breaking someone's config that currently works (even if it doesn't make sense). And if we're deprecating it, the risk doesn't seem worth it. If we weren't going to deprecate it, I'd say definitely fix it (with runtime guard).

[mattklein123]

OK sounds good. I'm fine either way, but let's definitely do the deprecation.

[tbarrella]

I have a few questions about how to deprecate something (referring to the breaking change policy)

Deprecators must implement a conversion from the deprecated configuration to the latest vNalpha (with the deprecated field) that Envoy uses internally.

What does this mean in this case? For examples that use use_proxy_proto like this it seems like I just need to update them to instead use

listener_filters:
- name: envoy.filters.listener.proxy_protocol
  typed_config:
    "@type": type.googleapis.com/envoy.extensions.filters.listener.proxy_protocol.v3.ProxyProtocol

But must there also be some code/tool that manipulates configuration for people?

The PR author deprecating the old configuration is responsible for updating all tests and canonical configuration, or guarding them with the DEPRECATED_FEATURE_TEST() macro.

Is this true for fuzzing tests as well? Referring to this test case

For adding [deprecated = true] to proto definitions, would this only be for v4alpha (there's also v3 and v2)?

[lambdai]

For adding [deprecated = true] to proto definitions, would this only be for v4alpha (there's also v3 and v2)?

You only need to add for v3 and run tools/proto_format/proto_format.sh fix to sync to other versions.

Not fully sure about the fuzzer CC @asraa

[htuch]

+1 on deprecate. The less magic in the filter chains the better IMHO (as well as existing behavior being broken).

[mattklein123]

I think we should a) deprecate and also b) warn on mismatch (not all or nothing).

I think this will give us good coverage.