Various configs use http2_protocol_options deprecated field

[davinci26]

Various configs use (see front-envoy-*.yaml and others):

http2_protocol_options: {} which is deprecated instead of

    typed_extension_protocol_options:
      envoy.extensions.upstreams.http.v3.HttpProtocolOptions:
        "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions
        explicit_http_config:
          http2_protocol_options: {}

[davinci26]

I identified the following configs:

examples/cache/front-envoy.yaml
examples/cors/backend/front-envoy.yaml
examples/cors/frontend/front-envoy.yaml
examples/csrf/crosssite/front-envoy.yaml
examples/csrf/samesite/front-envoy.yaml
 examples/dynamic-config-cp/envoy.yaml
 examples/dynamic-config-fs/configs/cds.yaml
examples/ext_authz/config/grpc-service/v3.yaml
examples/ext_authz/config/opa-service/v3.yaml
examples/front-proxy/front-envoy.yaml
examples/grpc-bridge/server/envoy-proxy.yaml
examples/jaeger-native-tracing/front-envoy-jaeger.yaml
examples/jaeger-native-tracing/service1-envoy-jaeger.yaml
examples/jaeger-tracing/front-envoy-jaeger.yaml
examples/jaeger-tracing/service1-envoy-jaeger.yaml
examples/load-reporting-service/service-envoy-w-lrs.yaml
examples/skywalking-tracing/front-envoy-skywalking.yaml
examples/skywalking-tracing/service1-envoy-skywalking.yaml
examples/zipkin-tracing/front-envoy-zipkin.yaml
examples/zipkin-tracing/service1-envoy-zipkin.yaml

[phlax]

@davinci26 what is the default ?

im wondering if we can just remove them - the newer way seems complicated to just set an empty {}

[mattklein123]

cc @alyssawilk ^. How did this pass CI in the first place? I thought we force fixing all deprecated configs?

[alyssawilk]

setting http2 protocol options in the cluster config is deprecated
https://github.com/envoyproxy/envoy/blob/main/api/envoy/config/cluster/v3/cluster.proto#L798
but setting http2 protocol options in the http config is not.
https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/upstreams/http/v3/http_protocol_options.proto#L69

If you take a look at the config you pasted it's using the new way of doing things

typed_extension_protocol_options:
  envoy.extensions.upstreams.http.v3.HttpProtocolOptions:

Sorry the name overlap is confusing!

[davinci26]

I probably got confused somewhere when I was reading the examples\cors\frontend\front-envoy.yaml

  clusters:
  - name: frontend_service
    connect_timeout: 0.25s
    type: STRICT_DNS
    lb_policy: ROUND_ROBIN
    http2_protocol_options: {}
    load_assignment:
      cluster_name: frontend_service
      endpoints:
      - lb_endpoints:
        - endpoint:
            address:
              socket_address:
                address: frontendservice
                port_value: 8000

[alyssawilk]

hm, yeah, that's way definitely concerning.

bazel test --define deprecated_features=disabled //test/config_test:example_configs_test should be failing for bad configs and I checked that file is definitely run by the test.

[davinci26]

I think that this test is loading the configs as v2 in some cases but I don't understand why. I observed that in #15461 which adds a new property in bootstrap proto and one test in example_configs_test is failing with:

[2021-03-18 20:00:29.912][29392][info][config] [source/server/configuration_impl.cc:128] loading tracing configuration
[2021-03-18 20:00:29.912][29392][info][config] [source/server/configuration_impl.cc:88] loading 0 static secret(s)
[2021-03-18 20:00:29.912][29392][info][config] [source/server/configuration_impl.cc:94] loading 2 cluster(s)
[2021-03-18 20:00:29.930][29392][info][config] [source/server/configuration_impl.cc:98] loading 1 listener(s)
[2021-03-18 20:00:29.941][29392][info][tracing] [source/common/tracing/http_tracer_manager_impl.cc:41] instantiating a new tracer: envoy.tracers.zipkin
[2021-03-18 20:00:29.948][29392][info][config] [source/server/configuration_impl.cc:110] loading stats configuration
[2021-03-18 20:00:29.977][29392][info][misc] [test/config_test/config_test.cc:192] testing C:/_eb/execroot/envoy/_tmp/ca647c4567690a3a5ae380df8b1a7823/test/config_test/zipkin-tracing_service2-envoy-zipkin.yaml as yaml.
[2021-03-18 20:00:30.132][29392][info][config] [source/server/configuration_impl.cc:128] loading tracing configuration
[2021-03-18 20:00:30.132][29392][info][config] [source/server/configuration_impl.cc:88] loading 0 static secret(s)
[2021-03-18 20:00:30.132][29392][info][config] [source/server/configuration_impl.cc:94] loading 2 cluster(s)
[2021-03-18 20:00:30.149][29392][info][config] [source/server/configuration_impl.cc:98] loading 1 listener(s)
[2021-03-18 20:00:30.160][29392][info][tracing] [source/common/tracing/http_tracer_manager_impl.cc:41] instantiating a new tracer: envoy.tracers.zipkin
[2021-03-18 20:00:30.166][29392][info][config] [source/server/configuration_impl.cc:110] loading stats configuration
unknown file: error: C++ exception with description "Protobuf message (type envoy.config.bootstrap.v2.Admin with unknown field set {5}) has unknown fields" thrown in the test body.

[phlax]

I think that this test is loading the configs as v2 in some cases but I don't understand why.

i hit this before it loads as v2 and then tries to upgrade iirc #13817

[davinci26]

it loads as v2 and then tries to upgrade

That would explain what I am seeing. The configs above can be loaded as v2 and in v2 the features are not deprecated so no update takes place and the test passes.

When I add a new property that is forced to load it as v3 and then I see:

unknown file: error: Uninteresting mock function call - returning directly.
    Function call: onDeprecatedField("type envoy.config.cluster.v3.Cluster Using deprecated option 'envoy.config.cluster.v3.Cluster.http2_protocol_options' from file cluster.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/version_history/version_history for details.", true)
Stack trace:
  00007FF6B560FC8B: testing::internal::GoogleTestFailureReporter::ReportFailure
  00007FF6B139D1FA: testing::internal::Expect
  00007FF6B56126A1: testing::internal::ReportUninterestingCall
  00007FF6B5610667: testing::internal::UntypedFunctionMockerBase::UntypedInvokeWith
  00007FF6B1884944: testing::internal::FunctionMocker<void __cdecl(std::basic_string_view<char,std::char_traits<char> >,bool)>::Invoke
  00007FF6B1885E9F: Envoy::ProtobufMessage::MockValidationVisitor::onDeprecatedField
  00007FF6B3C555EE: Envoy::`anonymous namespace'::deprecatedFieldHelper
  00007FF6B3C65245: Envoy::`anonymous namespace'::UnexpectedFieldProtoVisitor::onField
  00007FF6B3D0051F: Envoy::ProtobufMessage::traverseMessage
  00007FF6B3D005A9: Envoy::ProtobufMessage::traverseMessage
  00007FF6B3D005F1: Envoy::ProtobufMessage::traverseMessage
  00007FF6B3C5202C: Envoy::MessageUtil::checkForUnexpectedFields
  00007FF6B1522EC7: Envoy::MessageUtil::validate<envoy::config::bootstrap::v3::Bootstrap>
  00007FF6B14FC233: Envoy::Server::InstanceUtil::loadBootstrapConfig
  00007FF6B138E92D: Envoy::ConfigTest::ConfigTest::ConfigTest
  00007FF6B13687E5: Envoy::ConfigTest::run
  00007FF6B13352C3: Envoy::ExampleConfigsTest_All_Test::TestBody
  00007FF6B55E885C: testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,void>
  00007FF6B55E85F9: testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void>
  00007FF6B55D1C6A: testing::Test::Run
  00007FF6B55D27DB: testing::TestInfo::Run
... Google Test internal frames ...

[alyssawilk]

Does this get solved when we remove v2 in a few weeks? Losing testing of config migration will be a huge pain for maintainers. cc @adisuissa

[mattklein123]

IIRC this will be solved when we lose v2, because the config will still be tested but there will be no fallback, so I think as long as we fail on deprecated fields we are OK? cc @htuch

[davinci26]

I think as long as we fail on deprecated fields we are OK?

Based on what I have observed, when v2 is deleted configs with deprecated properties will fail

[alyssawilk]

Might make the v2 removal more arduous if our "fail on deprecated config" tests are now "upgrade or fail" but I guess that's on whoever does the v2 removal. Has anyone signed on for that?

[davinci26]

With my PR here #15461, I have added in all the configs a property that is v3 only and as part of this process I saw the config tests failing.

I have implicitly forced the configs to load as v3 with the PR above and I removed the deprecated property on #15563. I am not sure if there will be more configs failing due to the above.

[alyssawilk]

For sure it'll help to have the example configs cleaned up - thanks for tackling those :-)

[htuch]

Might make the v2 removal more arduous if our "fail on deprecated config" tests are now "upgrade or fail" but I guess that's on whoever does the v2 removal. Has anyone signed on for that?

I'm going to coordinate and oversee the v2 removal, I don't anticipate writing every PR. Can you elaborate on what the pain point here is? The current plan is to incrementally remove v2 features and their deprecated tests, we have support for existing v2 deprecated tests with an internal-only runtime flag.

[alyssawilk]

The problem is that somewhere along the line the v2 conversation code started compensating for the CI flags which verified we removed deprecated fields, so we lost CI verification that when folks deprecated fields, they migrated test and example config over to the new.
It just means instead of folks deprecating configs doing the work, whoever does v2 removal will have to do so (minus the bits @davinci26 just caught and fixed)

[htuch]

Ack, yes, I think this is the case. Another item for #10943

[mattklein123]

I think this is fixed so we can just assume v2 deletion will improve the overall situation.