config: allow unknown fields flag (take 2)

source/common/config/utility.h

@@ -52,7 +52,10 @@ class Utility {
     Protobuf::RepeatedPtrField<ResourceType> typed_resources;
     for (const auto& resource : response.resources()) {
       auto* typed_resource = typed_resources.Add();
-      resource.UnpackTo(typed_resource);
+      if (!resource.UnpackTo(typed_resource)) {
+        throw EnvoyException("Unable to unpack " + resource.DebugString());
+      }
+      MessageUtil::checkUnknownFields(*typed_resource);
     }
     return typed_resources;
   }

source/common/protobuf/utility.cc

@@ -48,9 +48,18 @@ ProtoValidationException::ProtoValidationException(const std::string& validation
   ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what());
 }
 
+ProtoUnknownFieldsMode MessageUtil::proto_unknown_fields = ProtoUnknownFieldsMode::Strict;
+
 void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message) {
+  MessageUtil::loadFromJsonEx(json, message, ProtoUnknownFieldsMode::Strict);
+}
+
+void MessageUtil::loadFromJsonEx(const std::string& json, Protobuf::Message& message,
+                                 ProtoUnknownFieldsMode proto_unknown_fields) {
   Protobuf::util::JsonParseOptions options;
-  options.ignore_unknown_fields = true;
+  if (proto_unknown_fields == ProtoUnknownFieldsMode::Allow) {
+    options.ignore_unknown_fields = true;
+  }
   const auto status = Protobuf::util::JsonStringToMessage(json, &message, options);
   if (!status.ok()) {
     throw EnvoyException("Unable to parse JSON as proto (" + status.ToString() + "): " + json);
@@ -68,6 +77,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa
   if (StringUtil::endsWith(path, ".pb")) {
     // Attempt to parse the binary format.
     if (message.ParseFromString(contents)) {
+      MessageUtil::checkUnknownFields(message);
       return;
     }
     throw EnvoyException("Unable to parse file \"" + path + "\" as a binary protobuf (type " +
@@ -121,7 +131,7 @@ void MessageUtil::jsonConvert(const Protobuf::Message& source, Protobuf::Message
     throw EnvoyException(fmt::format("Unable to convert protobuf message to JSON string: {} {}",
                                      status.ToString(), source.DebugString()));
   }
-  MessageUtil::loadFromJson(json, dest);
+  MessageUtil::loadFromJsonEx(json, dest, MessageUtil::proto_unknown_fields);
 }
 
 ProtobufWkt::Struct MessageUtil::keyValueStruct(const std::string& key, const std::string& value) {

source/common/protobuf/utility.h

@@ -133,6 +133,8 @@ class ProtoValidationException : public EnvoyException {
   ProtoValidationException(const std::string& validation_error, const Protobuf::Message& message);
 };
 
+enum class ProtoUnknownFieldsMode { Strict, Allow };
+
 class MessageUtil {
 public:
   // std::hash
@@ -158,7 +160,19 @@ class MessageUtil {
     return HashUtil::xxHash64(text);
   }
 
+  static ProtoUnknownFieldsMode proto_unknown_fields;
+
+  static void checkUnknownFields(const Protobuf::Message& message) {
+    if (MessageUtil::proto_unknown_fields == ProtoUnknownFieldsMode::Strict &&
+        !message.GetReflection()->GetUnknownFields(message).empty()) {
+      throw EnvoyException("Protobuf message (type " + message.GetTypeName() +
+                           ") has unknown fields");
+    }
+  }
+
   static void loadFromJson(const std::string& json, Protobuf::Message& message);
+  static void loadFromJsonEx(const std::string& json, Protobuf::Message& message,
+                             ProtoUnknownFieldsMode proto_unknown_fields);
   static void loadFromYaml(const std::string& yaml, Protobuf::Message& message);
   static void loadFromFile(const std::string& path, Protobuf::Message& message);
 
@@ -214,6 +228,7 @@ class MessageUtil {
     if (!message.UnpackTo(&typed_message)) {
       throw EnvoyException("Unable to unpack " + message.DebugString());
     }
+    checkUnknownFields(typed_message);
     return typed_message;
   };
 

source/server/BUILD

@@ -156,6 +156,7 @@ envoy_cc_library(
         "//include/envoy/stats:stats_interface",
         "//source/common/common:macros",
         "//source/common/common:version_lib",
+        "//source/common/protobuf:utility_lib",
         "//source/common/stats:stats_lib",
     ],
 )

source/server/options_impl.cc

@@ -9,6 +9,7 @@
 #include "common/common/logger.h"
 #include "common/common/macros.h"
 #include "common/common/version.h"
+#include "common/protobuf/utility.h"
 
 #include "spdlog/spdlog.h"
 #include "tclap/CmdLine.h"
@@ -64,6 +65,9 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv,
   TCLAP::SwitchArg allow_v1_config("", "allow-deprecated-v1-api", "allow use of legacy v1 config",
                                    cmd, false);
 
+  TCLAP::SwitchArg allow_unknown_fields("", "allow-unknown-fields",
+                                        "allow unknown fields in the configuration", cmd, false);
+
   TCLAP::ValueArg<std::string> admin_address_path("", "admin-address-path", "Admin address path",
                                                   false, "", "string", cmd);
   TCLAP::ValueArg<std::string> local_address_ip_version("", "local-address-ip-version",
@@ -184,6 +188,9 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv,
   config_path_ = config_path.getValue();
   config_yaml_ = config_yaml.getValue();
   v2_config_only_ = !allow_v1_config.getValue();
+  if (allow_unknown_fields.getValue()) {
+    MessageUtil::proto_unknown_fields = ProtoUnknownFieldsMode::Allow;
+  }
   admin_address_path_ = admin_address_path.getValue();
   log_path_ = log_path.getValue();
   restart_epoch_ = restart_epoch.getValue();

test/common/config/utility_test.cc

@@ -46,6 +46,16 @@ TEST(UtilityTest, GetTypedResources) {
   EXPECT_EQ("1", typed_resources[1].cluster_name());
 }
 
+TEST(UtilityTest, GetTypedResourcesWrongType) {
+  envoy::api::v2::DiscoveryResponse response;
+  envoy::api::v2::ClusterLoadAssignment load_assignment_0;
+  load_assignment_0.set_cluster_name("0");
+  response.add_resources()->PackFrom(load_assignment_0);
+
+  EXPECT_THROW_WITH_REGEX(Utility::getTypedResources<envoy::api::v2::Listener>(response),
+                          EnvoyException, "Unable to unpack .*");
+}
+
 TEST(UtilityTest, ComputeHashedVersion) {
   EXPECT_EQ("hash_2e1472b57af294d1", Utility::computeHashedVersion("{}").first);
   EXPECT_EQ("hash_33bf00a859c4ba3f", Utility::computeHashedVersion("foo").first);

test/common/protobuf/utility_test.cc

@@ -54,6 +54,17 @@ TEST(UtilityTest, LoadBinaryProtoFromFile) {
   EXPECT_TRUE(TestUtility::protoEqual(bootstrap, proto_from_file));
 }
 
+TEST(UtilityTest, LoadBinaryProtoUnknownFieldFromFile) {
+  ProtobufWkt::Duration source_duration;
+  source_duration.set_seconds(42);
+  const std::string filename =
+      TestEnvironment::writeStringToFileForTest("proto.pb", source_duration.SerializeAsString());
+  envoy::config::bootstrap::v2::Bootstrap proto_from_file;
+  EXPECT_THROW_WITH_MESSAGE(
+      MessageUtil::loadFromFile(filename, proto_from_file), EnvoyException,
+      "Protobuf message (type envoy.config.bootstrap.v2.Bootstrap) has unknown fields");
+}
+
 TEST(UtilityTest, LoadTextProtoFromFile) {
   envoy::config::bootstrap::v2::Bootstrap bootstrap;
   bootstrap.mutable_cluster_manager()
@@ -212,6 +223,25 @@ TEST(UtilityTest, HashedValueStdHash) {
   EXPECT_NE(set.find(hv3), set.end());
 }
 
+TEST(UtilityTest, AnyConvertWrongType) {
+  ProtobufWkt::Duration source_duration;
+  source_duration.set_seconds(42);
+  ProtobufWkt::Any source_any;
+  source_any.PackFrom(source_duration);
+  EXPECT_THROW_WITH_REGEX(MessageUtil::anyConvert<ProtobufWkt::Timestamp>(source_any),
+                          EnvoyException, "Unable to unpack .*");
+}
+
+TEST(UtilityTest, AnyConvertWrongFields) {
+  const ProtobufWkt::Struct obj = MessageUtil::keyValueStruct("test_key", "test_value");
+  ProtobufWkt::Any source_any;
+  source_any.PackFrom(obj);
+  source_any.set_type_url("type.google.com/google.protobuf.Timestamp");
+  EXPECT_THROW_WITH_MESSAGE(MessageUtil::anyConvert<ProtobufWkt::Timestamp>(source_any),
+                            EnvoyException,
+                            "Protobuf message (type google.protobuf.Timestamp) has unknown fields");
+}
+
 TEST(UtilityTest, JsonConvertSuccess) {
   ProtobufWkt::Duration source_duration;
   source_duration.set_seconds(42);
@@ -221,9 +251,11 @@ TEST(UtilityTest, JsonConvertSuccess) {
 }
 
 TEST(UtilityTest, JsonConvertUnknownFieldSuccess) {
+  MessageUtil::proto_unknown_fields = ProtoUnknownFieldsMode::Allow;
   const ProtobufWkt::Struct obj = MessageUtil::keyValueStruct("test_key", "test_value");
   envoy::config::bootstrap::v2::Bootstrap bootstrap;
   EXPECT_NO_THROW(MessageUtil::jsonConvert(obj, bootstrap));
+  MessageUtil::proto_unknown_fields = ProtoUnknownFieldsMode::Strict;
 }
 
 TEST(UtilityTest, JsonConvertFail) {

test/common/router/config_impl_test.cc

@@ -4490,6 +4490,9 @@ class PerFilterConfigsTest : public testing::Test {
                                        Server::Configuration::FactoryContext&) override {
       NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
     }
+    ProtobufTypes::MessagePtr createEmptyRouteConfigProto() override {
+      return ProtobufTypes::MessagePtr{new ProtobufWkt::Timestamp()};
+    }
   };
 
   void checkEach(const std::string& yaml, uint32_t expected_entry, uint32_t expected_route,
@@ -4563,7 +4566,7 @@ name: foo
     routes:
       - match: { prefix: "/" }
         route: { cluster: baz }
-    per_filter_config: { test.default.filter: { unknown_key: 123} }
+    per_filter_config: { test.default.filter: { seconds: 123} }
 )EOF";
 
   checkNoPerFilterConfig(yaml);

test/config/utility.cc

@@ -89,7 +89,6 @@ const std::string ConfigHelper::DEFAULT_HEALTH_CHECK_FILTER =
 name: envoy.health_check
 config:
     pass_through_mode: false
-    endpoint: /healthcheck
 )EOF";
 
 const std::string ConfigHelper::DEFAULT_SQUASH_FILTER =

test/extensions/filters/network/thrift_proxy/integration_test.cc

@@ -46,12 +46,7 @@ class ThriftConnManagerIntegrationTest
             route_config:
               name: "routes"
               routes:
-                - match:
-                    method_name: "execute"
-                  route:
-                    cluster: "cluster_0"
-                - match:
-                    method_name: "poke"
+                - match: {}
                   route:
                     cluster: "cluster_0"
       )EOF";

test/integration/http_integration.cc

@@ -409,7 +409,6 @@ void HttpIntegrationTest::testComputedHealthCheck() {
 name: envoy.health_check
 config:
     pass_through_mode: false
-    endpoint: /healthcheck
     cluster_min_healthy_percentages:
         example_cluster_name: { value: 75 }
 )EOF");

tools/protodoc/protodoc.bzl

@@ -78,7 +78,6 @@ def _proto_doc_aspect_impl(target, ctx):
     return [OutputGroupInfo(rst = transitive_outputs)]
 
 proto_doc_aspect = aspect(
-    implementation = _proto_doc_aspect_impl,
     attr_aspects = ["deps"],
     attrs = {
         "_protoc": attr.label(
@@ -92,4 +91,5 @@ proto_doc_aspect = aspect(
             cfg = "host",
         ),
     },
+    implementation = _proto_doc_aspect_impl,
 )