From 925810d00b0d3095a8e67fd4e04e0f597ed188bb Mon Sep 17 00:00:00 2001 From: Quanjie Lin <32855694+quanjielin@users.noreply.github.com> Date: Fri, 8 Feb 2019 13:39:30 -0800 Subject: [PATCH] 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](https://github.com/protocolbuffers/protobuf/issues/5668). Signed-off-by: Quanjie Lin --- source/common/config/config_provider_impl.h | 20 +++++++++---------- source/common/router/rds_impl.cc | 6 ++---- source/common/router/rds_impl.h | 6 +++--- source/common/secret/secret_manager_impl.h | 3 ++- .../config/config_provider_impl_test.cc | 6 +++--- tools/check_format.py | 14 +++++++++++++ tools/check_format_test_helper.py | 3 +++ .../check_format/serialize_as_string.cc | 8 ++++++++ 8 files changed, 45 insertions(+), 21 deletions(-) create mode 100644 tools/testdata/check_format/serialize_as_string.cc diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index 884cbcaf2be3..6a53795481f1 100644 --- a/source/common/config/config_provider_impl.h +++ b/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 initialize_callback_; std::unordered_set 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, EnumClassHash>; using ConfigSubscriptionMap = - std::unordered_map>; + std::unordered_map>; ConfigProviderManagerImplBase(Server::Admin& admin, const std::string& config_name); @@ -369,15 +369,15 @@ class ConfigProviderManagerImplBase : public ConfigProviderManager, public Singl * @return std::shared_ptr an existing (if a match is found) or newly allocated subscription. */ template - std::shared_ptr getSubscription( - const Protobuf::Message& config_source_proto, Init::Manager& init_manager, - const std::function& subscription_factory_fn) { + std::shared_ptr + getSubscription(const Protobuf::Message& config_source_proto, Init::Manager& init_manager, + const std::function& subscription_factory_fn) { static_assert(std::is_base_of::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); } diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 9427ef60025d..1eb5528a27c8 100644 --- a/source/common/router/rds_impl.cc +++ b/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; diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 12918f6ffe34..4498eb99bb10 100644 --- a/source/common/router/rds_impl.h +++ b/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 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::unordered_map> route_config_subscriptions_; std::unordered_set static_route_config_providers_; Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_; diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 7c00c7ea98a5..aa7ee6e4b215 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/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 secret_provider = dynamic_secret_providers_[map_key].lock(); if (!secret_provider) { diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index 1f355769cda5..1d94dbf4b239 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -42,7 +42,7 @@ class DummyConfigSubscription : public ConfigSubscriptionInstanceBase, Envoy::Config::SubscriptionCallbacks { 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( 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( @@ -199,7 +199,7 @@ StaticDummyConfigProvider::StaticDummyConfigProvider( config_(std::make_shared(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(), diff --git a/tools/check_format.py b/tools/check_format.py index bfd9c9061aae..e9e759426321 100755 --- a/tools/check_format.py +++ b/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): diff --git a/tools/check_format_test_helper.py b/tools/check_format_test_helper.py index e09015566fdc..001a3191c6b7 100755 --- a/tools/check_format_test_helper.py +++ b/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 " diff --git a/tools/testdata/check_format/serialize_as_string.cc b/tools/testdata/check_format/serialize_as_string.cc new file mode 100644 index 000000000000..705bf5dae77f --- /dev/null +++ b/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