config: handling deprecated enum values (#8281)

Handling deprecated and deprecated-default enum values as part of our config checking. Risk Level: Medium Testing: new unit tests Docs Changes: updated runtime docs Release Notes: reverted #8207 Fixes #8253 Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Sep 24, 2019 · 7d8e9de · 7d8e9de
1 parent 1f7f90f
commit 7d8e9de
Show file tree

Hide file tree

Showing 8 changed files with 141 additions and 7 deletions.
diff --git a/docs/root/configuration/operations/runtime.rst b/docs/root/configuration/operations/runtime.rst
@@ -238,7 +238,8 @@ In the second phase the message and filename will be added to
 :repo:`runtime_features.cc <source/common/runtime/runtime_features.cc>`
 and use of that configuration field will cause the config to be rejected by default. 
 This fail-by-default mode can be overridden in runtime configuration by setting
-envoy.deprecated_features.filename.proto:fieldname to true. For example, for a deprecated field
+envoy.deprecated_features.filename.proto:fieldname or envoy.deprecated_features.filename.proto:enum_value
+to true. For example, for a deprecated field
 ``Foo.Bar.Eep`` in ``baz.proto`` set ``envoy.deprecated_features.baz.proto:Eep`` to
 ``true``. Use of this override is **strongly discouraged**.
 Fatal-by-default configuration indicates that the removal of the old code paths is imminent. It is

diff --git a/docs/root/intro/deprecated.rst b/docs/root/intro/deprecated.rst
@@ -38,9 +38,7 @@ Version 1.12.0 (pending)
   <envoy_api_msg_config.filter.network.http_connection_manager.v2.HttpConnectionManager>`
   has been deprecated in favor of the `traffic_direction` field in
   :ref:`Listener <envoy_api_msg_Listener>`. The latter takes priority if
-  specified. Note that the default value `INGRESS` of the `operation_name`
-  field is not detected as being set, so the deprecation warning is not
-  triggered for it.
+  specified.
 
 Version 1.11.0 (July 11, 2019)
 ==============================

diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc
@@ -179,6 +179,51 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa
   }
 }
 
+void checkForDeprecatedNonRepeatedEnumValue(const Protobuf::Message& message,
+                                            absl::string_view filename,
+                                            const Protobuf::FieldDescriptor* field,
+                                            const Protobuf::Reflection* reflection,
+                                            Runtime::Loader* runtime) {
+  // Repeated fields will be handled by recursion in checkForUnexpectedFields.
+  if (field->is_repeated() || field->cpp_type() != Protobuf::FieldDescriptor::CPPTYPE_ENUM) {
+    return;
+  }
+
+  bool default_value = !reflection->HasField(message, field);
+
+  const Protobuf::EnumValueDescriptor* enum_value_descriptor = reflection->GetEnum(message, field);
+  if (!enum_value_descriptor->options().deprecated()) {
+    return;
+  }
+  std::string err = fmt::format(
+      "Using {}deprecated value {} for enum '{}' from file {}. This enum value will be removed "
+      "from Envoy soon{}. Please see https://www.envoyproxy.io/docs/envoy/latest/intro/deprecated "
+      "for details.",
+      (default_value ? "the default now-" : ""), enum_value_descriptor->name(), field->full_name(),
+      filename, (default_value ? " so a non-default value must now be explicitly set" : ""));
+#ifdef ENVOY_DISABLE_DEPRECATED_FEATURES
+  bool warn_only = false;
+#else
+  bool warn_only = true;
+#endif
+
+  if (runtime && !runtime->snapshot().deprecatedFeatureEnabled(absl::StrCat(
+                     "envoy.deprecated_features.", filename, ":", enum_value_descriptor->name()))) {
+    warn_only = false;
+  }
+
+  if (warn_only) {
+    ENVOY_LOG_MISC(warn, "{}", err);
+  } else {
+    const char fatal_error[] =
+        " 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.";
+    throw ProtoValidationException(err + fatal_error, message);
+  }
+}
+
 void MessageUtil::checkForUnexpectedFields(const Protobuf::Message& message,
                                            ProtobufMessage::ValidationVisitor& validation_visitor,
                                            Runtime::Loader* runtime) {
@@ -199,7 +244,12 @@ void MessageUtil::checkForUnexpectedFields(const Protobuf::Message& message,
   const Protobuf::Descriptor* descriptor = message.GetDescriptor();
   const Protobuf::Reflection* reflection = message.GetReflection();
   for (int i = 0; i < descriptor->field_count(); ++i) {
-    const auto* field = descriptor->field(i);
+    const Protobuf::FieldDescriptor* field = descriptor->field(i);
+    absl::string_view filename = filenameFromPath(field->file()->name());
+
+    // Before we check to see if the field is in use, see if there's a
+    // deprecated default enum value.
+    checkForDeprecatedNonRepeatedEnumValue(message, filename, field, reflection, runtime);
 
     // If this field is not in use, continue.
     if ((field->is_repeated() && reflection->FieldSize(message, field) == 0) ||
@@ -212,7 +262,6 @@ void MessageUtil::checkForUnexpectedFields(const Protobuf::Message& message,
 #else
     bool warn_only = true;
 #endif
-    absl::string_view filename = filenameFromPath(field->file()->name());
     // Allow runtime to be null both to not crash if this is called before server initialization,
     // and so proto validation works in context where runtime singleton is not set up (e.g.
     // standalone config validation utilities)

diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc
@@ -679,6 +679,48 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RepeatedMessageDeprecated))
                       checkForDeprecation(base));
 }
 
+// Check that deprecated enum values trigger for default values
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(EnumValuesDeprecatedDefault)) {
+  envoy::test::deprecation_test::Base base;
+  base.mutable_enum_container();
+
+  EXPECT_LOG_CONTAINS(
+      "warning",
+      "Using the default now-deprecated value DEPRECATED_DEFAULT for enum "
+      "'envoy.test.deprecation_test.Base.InnerMessageWithDeprecationEnum.deprecated_enum' from "
+      "file deprecated.proto. This enum value will be removed from Envoy soon so a non-default "
+      "value must now be explicitly set.",
+      checkForDeprecation(base));
+}
+
+// Check that deprecated enum values trigger for non-default values
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(EnumValuesDeprecated)) {
+  envoy::test::deprecation_test::Base base;
+  base.mutable_enum_container()->set_deprecated_enum(
+      envoy::test::deprecation_test::Base::DEPRECATED_NOT_DEFAULT);
+
+  EXPECT_LOG_CONTAINS(
+      "warning",
+      "Using deprecated value DEPRECATED_NOT_DEFAULT for enum "
+      "'envoy.test.deprecation_test.Base.InnerMessageWithDeprecationEnum.deprecated_enum' "
+      "from file deprecated.proto. This enum value will be removed from Envoy soon.",
+      checkForDeprecation(base));
+}
+
+// Make sure the runtime overrides for protos work, by checking the non-fatal to
+// fatal option.
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RuntimeOverrideEnumDefault)) {
+  envoy::test::deprecation_test::Base base;
+  base.mutable_enum_container();
+
+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.deprecated_features.deprecated.proto:DEPRECATED_DEFAULT", "false"}});
+
+  // Make sure this is set up right.
+  EXPECT_THROW_WITH_REGEX(checkForDeprecation(base), ProtoValidationException,
+                          "Using the default now-deprecated value DEPRECATED_DEFAULT");
+}
+
 class TimestampUtilTest : public testing::Test, public ::testing::WithParamInterface<int64_t> {};
 
 TEST_P(TimestampUtilTest, SystemClockToTimestampTest) {

diff --git a/test/extensions/tracers/zipkin/config_test.cc b/test/extensions/tracers/zipkin/config_test.cc
@@ -26,6 +26,7 @@ TEST(ZipkinTracerConfigTest, ZipkinHttpTracer) {
     config:
       collector_cluster: fake_cluster
       collector_endpoint: /api/v1/spans
+      collector_endpoint_version: HTTP_JSON
   )EOF";
 
   envoy::config::trace::v2::Tracing configuration;

diff --git a/test/integration/proxy_proto_integration_test.cc b/test/integration/proxy_proto_integration_test.cc
@@ -89,7 +89,7 @@ TEST_P(ProxyProtoIntegrationTest, RouterProxyUnknownLongRequestAndResponseWithBo
   testRouterRequestAndResponseWithBody(1024, 512, false, &creator);
 }
 
-TEST_P(ProxyProtoIntegrationTest, OriginalDst) {
+TEST_P(ProxyProtoIntegrationTest, DEPRECATED_FEATURE_TEST(OriginalDst)) {
   // Change the cluster to an original destination cluster. An original destination cluster
   // ignores the configured hosts, and instead uses the restored destination address from the
   // incoming (server) connection as the destination address for the outgoing (client) connection.
@@ -119,4 +119,34 @@ TEST_P(ProxyProtoIntegrationTest, OriginalDst) {
   testRouterRequestAndResponseWithBody(1024, 512, false, &creator);
 }
 
+TEST_P(ProxyProtoIntegrationTest, ClusterProvided) {
+  // Change the cluster to an original destination cluster. An original destination cluster
+  // ignores the configured hosts, and instead uses the restored destination address from the
+  // incoming (server) connection as the destination address for the outgoing (client) connection.
+  config_helper_.addConfigModifier([&](envoy::config::bootstrap::v2::Bootstrap& bootstrap) -> void {
+    auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0);
+    cluster->mutable_hosts()->Clear();
+    cluster->set_type(envoy::api::v2::Cluster::ORIGINAL_DST);
+    cluster->set_lb_policy(envoy::api::v2::Cluster::CLUSTER_PROVIDED);
+  });
+
+  ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr {
+    Network::ClientConnectionPtr conn = makeClientConnection(lookupPort("http"));
+    // Create proxy protocol line that has the fake upstream address as the destination address.
+    // This address will become the "restored" address for the server connection and will
+    // be used as the destination address by the original destination cluster.
+    std::string proxyLine = fmt::format(
+        "PROXY {} {} 65535 {}\r\n",
+        GetParam() == Network::Address::IpVersion::v4 ? "TCP4 1.2.3.4" : "TCP6 1:2:3::4",
+        Network::Test::getLoopbackAddressString(GetParam()),
+        fake_upstreams_[0]->localAddress()->ip()->port());
+
+    Buffer::OwnedImpl buf(proxyLine);
+    conn->write(buf, false);
+    return conn;
+  };
+
+  testRouterRequestAndResponseWithBody(1024, 512, false, &creator);
+}
+
 } // namespace Envoy
diff --git a/test/proto/deprecated.proto b/test/proto/deprecated.proto
@@ -15,4 +15,16 @@ message Base {
   InnerMessage not_deprecated_message = 5;
   repeated InnerMessage repeated_message = 6;
   repeated InnerMessage deprecated_repeated_message = 7 [deprecated = true];
+
+  // For deprecated enum value testing, stick the enum in a container, to avoid
+  // the default instantiation of Base having a deprecated-by-default value.
+  enum DeprecationEnum {
+    DEPRECATED_DEFAULT = 0 [deprecated = true];
+    NOT_DEPRECATED = 1;
+    DEPRECATED_NOT_DEFAULT = 2 [deprecated = true];
+  }
+  message InnerMessageWithDeprecationEnum {
+    DeprecationEnum deprecated_enum = 1;
+  }
+  InnerMessageWithDeprecationEnum enum_container = 8;
 }
diff --git a/test/server/zipkin_tracing.yaml b/test/server/zipkin_tracing.yaml
@@ -8,3 +8,4 @@ tracing:
     config:
       collector_cluster: zipkin
       collector_endpoint: "/api/v1/spans"
+      collector_endpoint_version: HTTP_JSON