use hash func that generates deterministic result for protobuf (#5814)

This PR uses hash func that generates deterministic result for proto.

Background -
When integrate envoy SDS with Istio, we found envoy sends out multiple requests for same sdsconfig(details in #5744);

After some debugging, we found the issue describes in [protocolbuffers/protobuf#5668](protocolbuffers/protobuf#5668).

Signed-off-by: Quanjie Lin <quanlin@google.com>

source/common/config/config_provider_impl.h

@@ -196,7 +196,7 @@ class ConfigSubscriptionInstanceBase : public Init::Target,
   }
 
 protected:
-  ConfigSubscriptionInstanceBase(const std::string& name, const std::string& manager_identifier,
+  ConfigSubscriptionInstanceBase(const std::string& name, const uint64_t manager_identifier,
                                  ConfigProviderManagerImplBase& config_provider_manager,
                                  TimeSource& time_source, const SystemTime& last_updated,
                                  const LocalInfo::LocalInfo& local_info)
@@ -222,7 +222,7 @@ class ConfigSubscriptionInstanceBase : public Init::Target,
   const std::string name_;
   std::function<void()> initialize_callback_;
   std::unordered_set<MutableConfigProviderImplBase*> mutable_config_providers_;
-  const std::string manager_identifier_;
+  const uint64_t manager_identifier_;
   ConfigProviderManagerImplBase& config_provider_manager_;
   TimeSource& time_source_;
   SystemTime last_updated_;
@@ -346,7 +346,7 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl
   using ConfigProviderMap = std::unordered_map<ConfigProviderInstanceType,
                                                std::unique_ptr<ConfigProviderSet>, EnumClassHash>;
   using ConfigSubscriptionMap =
-      std::unordered_map<std::string, std::weak_ptr<ConfigSubscriptionInstanceBase>>;
+      std::unordered_map<uint64_t, std::weak_ptr<ConfigSubscriptionInstanceBase>>;
 
   ConfigProviderManagerImplBase(Server::Admin& admin, const std::string& config_name);
 
@@ -369,15 +369,15 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl
    * @return std::shared_ptr<T> an existing (if a match is found) or newly allocated subscription.
    */
   template <typename T>
-  std::shared_ptr<T> getSubscription(
-      const Protobuf::Message& config_source_proto, Init::Manager& init_manager,
-      const std::function<ConfigSubscriptionInstanceBaseSharedPtr(
-          const std::string&, ConfigProviderManagerImplBase&)>& subscription_factory_fn) {
+  std::shared_ptr<T>
+  getSubscription(const Protobuf::Message& config_source_proto, Init::Manager& init_manager,
+                  const std::function<ConfigSubscriptionInstanceBaseSharedPtr(
+                      const uint64_t, ConfigProviderManagerImplBase&)>& subscription_factory_fn) {
     static_assert(std::is_base_of<ConfigSubscriptionInstanceBase, T>::value,
                   "T must be a subclass of ConfigSubscriptionInstanceBase");
 
     ConfigSubscriptionInstanceBaseSharedPtr subscription;
-    const std::string manager_identifier = config_source_proto.SerializeAsString();
+    const uint64_t manager_identifier = MessageUtil::hash(config_source_proto);
 
     auto it = config_subscriptions_.find(manager_identifier);
     if (it == config_subscriptions_.end()) {
@@ -401,12 +401,12 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl
   }
 
 private:
-  void bindSubscription(const std::string& manager_identifier,
+  void bindSubscription(const uint64_t manager_identifier,
                         ConfigSubscriptionInstanceBaseSharedPtr& subscription) {
     config_subscriptions_.insert({manager_identifier, subscription});
   }
 
-  void unbindSubscription(const std::string& manager_identifier) {
+  void unbindSubscription(const uint64_t manager_identifier) {
     config_subscriptions_.erase(manager_identifier);
   }
 

source/common/router/rds_impl.cc

@@ -56,7 +56,7 @@ StaticRouteConfigProviderImpl::~StaticRouteConfigProviderImpl() {
 // initialization needs to be fixed.
 RdsRouteConfigSubscription::RdsRouteConfigSubscription(
     const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
-    const std::string& manager_identifier, Server::Configuration::FactoryContext& factory_context,
+    const uint64_t manager_identifier, Server::Configuration::FactoryContext& factory_context,
     const std::string& stat_prefix,
     Envoy::Router::RouteConfigProviderManagerImpl& route_config_provider_manager)
     : route_config_name_(rds.route_config_name()),
@@ -194,9 +194,7 @@ Router::RouteConfigProviderPtr RouteConfigProviderManagerImpl::createRdsRouteCon
     Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix) {
 
   // RdsRouteConfigSubscriptions are unique based on their serialized RDS config.
-  // TODO(htuch): Full serialization here gives large IDs, could get away with a
-  // strong hash instead.
-  const std::string manager_identifier = rds.SerializeAsString();
+  const uint64_t manager_identifier = MessageUtil::hash(rds);
 
   RdsRouteConfigSubscriptionSharedPtr subscription;
 

source/common/router/rds_impl.h

@@ -121,7 +121,7 @@ class RdsRouteConfigSubscription
 
   RdsRouteConfigSubscription(
       const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
-      const std::string& manager_identifier, Server::Configuration::FactoryContext& factory_context,
+      const uint64_t manager_identifier, Server::Configuration::FactoryContext& factory_context,
       const std::string& stat_prefix,
       RouteConfigProviderManagerImpl& route_config_provider_manager);
 
@@ -134,7 +134,7 @@ class RdsRouteConfigSubscription
   Stats::ScopePtr scope_;
   RdsStats stats_;
   RouteConfigProviderManagerImpl& route_config_provider_manager_;
-  const std::string manager_identifier_;
+  const uint64_t manager_identifier_;
   TimeSource& time_source_;
   SystemTime last_updated_;
   absl::optional<LastConfigInfo> config_info_;
@@ -202,7 +202,7 @@ class RouteConfigProviderManagerImpl : public RouteConfigProviderManager,
   // TODO(jsedgwick) These two members are prime candidates for the owned-entry list/map
   // as in ConfigTracker. I.e. the ProviderImpls would have an EntryOwner for these lists
   // Then the lifetime management stuff is centralized and opaque.
-  std::unordered_map<std::string, std::weak_ptr<RdsRouteConfigSubscription>>
+  std::unordered_map<uint64_t, std::weak_ptr<RdsRouteConfigSubscription>>
       route_config_subscriptions_;
   std::unordered_set<RouteConfigProvider*> static_route_config_providers_;
   Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_;

source/common/secret/secret_manager_impl.h

@@ -50,7 +50,8 @@ class SecretManagerImpl : public SecretManager {
     findOrCreate(const envoy::api::v2::core::ConfigSource& sds_config_source,
                  const std::string& config_name,
                  Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
-      const std::string map_key = sds_config_source.SerializeAsString() + config_name;
+      const std::string map_key =
+          absl::StrCat(MessageUtil::hash(sds_config_source), ".", config_name);
 
       std::shared_ptr<SecretType> secret_provider = dynamic_secret_providers_[map_key].lock();
       if (!secret_provider) {

test/common/config/config_provider_impl_test.cc

@@ -42,7 +42,7 @@ class DummyConfigSubscription
     : public ConfigSubscriptionInstanceBase,
       Envoy::Config::SubscriptionCallbacks<test::common::config::DummyConfig> {
 public:
-  DummyConfigSubscription(const std::string& manager_identifier,
+  DummyConfigSubscription(const uint64_t manager_identifier,
                           Server::Configuration::FactoryContext& factory_context,
                           DummyConfigProviderManager& config_provider_manager);
 
@@ -162,7 +162,7 @@ class DummyConfigProviderManager : public ConfigProviderManagerImplBase {
                                             const std::string&) override {
     DummyConfigSubscriptionSharedPtr subscription = getSubscription<DummyConfigSubscription>(
         config_source_proto, factory_context.initManager(),
-        [&factory_context](const std::string& manager_identifier,
+        [&factory_context](const uint64_t manager_identifier,
                            ConfigProviderManagerImplBase& config_provider_manager)
             -> ConfigSubscriptionInstanceBaseSharedPtr {
           return std::make_shared<DummyConfigSubscription>(
@@ -199,7 +199,7 @@ StaticDummyConfigProvider::StaticDummyConfigProvider(
       config_(std::make_shared<DummyConfig>(config_proto)), config_proto_(config_proto) {}
 
 DummyConfigSubscription::DummyConfigSubscription(
-    const std::string& manager_identifier, Server::Configuration::FactoryContext& factory_context,
+    const uint64_t manager_identifier, Server::Configuration::FactoryContext& factory_context,
     DummyConfigProviderManager& config_provider_manager)
     : ConfigSubscriptionInstanceBase(
           "DummyDS", manager_identifier, config_provider_manager, factory_context.timeSource(),

tools/check_format.py

@@ -42,6 +42,10 @@
 # Files in these paths can use std::get_time
 GET_TIME_WHITELIST = ('./test/test_common/utility.cc')
 
+# Files in these paths can use MessageLite::SerializeAsString
+SERIALIZE_AS_STRING_WHITELIST = ('./test/common/protobuf/utility_test.cc',
+                                 './test/common/grpc/codec_test.cc')
+
 CLANG_FORMAT_PATH = os.getenv("CLANG_FORMAT", "clang-format-7")
 BUILDIFIER_PATH = os.getenv("BUILDIFIER_BIN", "$GOPATH/bin/buildifier")
 ENVOY_BUILD_FIXER_PATH = os.path.join(
@@ -223,6 +227,10 @@ def whitelistedForGetTime(file_path):
   return file_path in GET_TIME_WHITELIST
 
 
+def whitelistedForSerializeAsString(file_path):
+  return file_path in SERIALIZE_AS_STRING_WHITELIST
+
+
 def findSubstringAndReturnError(pattern, file_path, error_message):
   with open(file_path) as f:
     text = f.read()
@@ -414,6 +422,12 @@ def checkSourceLine(line, file_path, reportError):
   if file_path != './test/test_common/test_base.h' and (' testing::Test ' in line or
                                                         ' testing::TestWithParam' in line):
     reportError("Derive test classes from TestBase in test/test_common/test_base.h")
+  if not whitelistedForSerializeAsString(file_path) and 'SerializeAsString' in line:
+    # The MessageLite::SerializeAsString doesn't generate deterministic serialization,
+    # use MessageUtil::hash instead.
+    reportError(
+        "Don't use MessageLite::SerializeAsString for generating deterministic serialization, use MessageUtil::hash instead."
+    )
 
 
 def checkBuildLine(line, file_path, reportError):

tools/check_format_test_helper.py

@@ -202,6 +202,9 @@ def checkFileExpectingOK(filename):
   errors += checkUnfixableError("elvis_operator.cc", "Don't use the '?:' operator")
   errors += checkUnfixableError("testing_test.cc",
                                 "Don't use 'using testing::Test;, elaborate the type instead")
+  errors += checkUnfixableError(
+      "serialize_as_string.cc",
+      "Don't use MessageLite::SerializeAsString for generating deterministic serialization")
   errors += checkUnfixableError(
       "version_history.rst",
       "Version history line malformed. Does not match VERSION_HISTORY_NEW_LINE_REGEX in "

tools/testdata/check_format/serialize_as_string.cc

@@ -0,0 +1,8 @@
+namespace Envoy {
+
+void use_serialize_as_string() {
+  google::protobuf::FieldMask mask;
+  const std::string key = mask.SerializeAsString();
+}
+
+} // namespace Envoy