search

envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.96k stars 4.8k forks source link

envoy.features.fail_on_any_deprecated_feature Runtime Key Doesn't Prevent Envoy Startup #16270

Open brodmerkle opened 3 years ago

brodmerkle commented 3 years ago

While upgrading to Envoy 1.18.2 I tried to use the new envoy.features.fail_on_any_deprecated_feature runtime key to prevent Envoy from starting up at all if one or more deprecated fields are found to be in use. However, even after adding this runtime key, Envoy continued to start up even when logging deprecation warnings - in my case due to having an access_log_path defined under admin, which is newly deprecated in 1.18. This was unexpected, since the Envoy 1.18.0 changelog includes this entry:

config: add envoy.features.fail_on_any_deprecated_feature runtime key, which matches the behaviour of compile-time flag ENVOY_DISABLE_DEPRECATED_FEATURES, i.e. use of deprecated fields will cause a crash.

To narrow down the problem and rule out any misconfiguration on my part, I attempted a much simpler test case by purely pulling down envoyproxy/envoy:v1.18.2 and making minor edits to its /etc/envoy/envoy.yaml, then trying to run envoy against this updated configuration to see if I could get envoy.features.fail_on_any_deprecated_feature to work in this context. I could not, so the simplified test case here is just to swap out the following lines in /etc/envoy/envoy.yaml...

admin:
  address:
    socket_address:
      protocol: TCP
      address: 0.0.0.0
      port_value: 9901

...with this larger config that utilizes the now-deprecated access_log_path field and enables the envoy.features.fail_on_any_deprecated_feature runtime key:

admin:
  access_log_path: /dev/stdout
  address:
    socket_address:
      protocol: TCP
      address: 0.0.0.0
      port_value: 9901
layered_runtime:
  layers:
  - name: static-layer
    static_layer:
      envoy.features.fail_on_any_deprecated_feature: true

To execute these steps directly against the envoy:v1.18.2 image we can use these commands (not passing "--bootstrap-version 3" prevents Envoy from spotting the deprecated fields at all):

docker pull envoyproxy/envoy:v1.18.2
docker exec --entrypoint /bin/bash -it envoyproxy/envoy:v1.18.2
  apt-get update && apt-get install -y vim.tiny # To help us make a quick edit for the test.
  vi /etc/envoy/envoy.yaml  # Make the edit defined above.
  envoy --bootstrap-version 3 -c /etc/envoy/envoy.yaml # Observe deprecation warnings, but Envoy starts up.

The actual deprecation warning logged is the one we'd expect if we'd never turned on envoy.features.fail_on_any_deprecated_feature at all:

[2021-05-02 17:27:39.585][385][warning][misc] [source/common/protobuf/message_validator_impl.cc:21] Deprecated field: type envoy.config.bootstrap.v3.Admin Using deprecated option 'envoy.config.bootstrap.v3.Admin.access_log_path' from file bootstrap.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/version_history/version_history for details. If continued use of this field is absolutely necessary, see https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime#using-runtime-overrides-for-deprecated-features for how to apply a temporary and highly discouraged override.

Looking in PR history, it certainly appears that https://github.com/envoyproxy/envoy/pull/14749 introduced support for this new runtime key with the expected semantics, and this code is indeed present in 1.18.2, specifically at https://github.com/envoyproxy/envoy/blob/v1.18.2/source/common/protobuf/utility.cc#L210.

Is this expected behavior when the envoy.features.fail_on_any_deprecated_feature is set to true?

mattklein123 commented 3 years ago

My suspicion is that unfortunately there is an init order issue and the deprecated field in question is actually parsed prior to runtime being initialized. This prevents the failure. This feature probably should have been a CLI option or we need to do some type of double pass over the bootstrap to look for deprecated fields and whether we need to fail to start. cc @htuch @alyssawilk who might have further ideas.

htuch commented 3 years ago

Yeah, I think this assessment is correct. We need to have already parsed the entire bootstrap and validated prior to the availability of bootstrap based runtime fields.