Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proto: re-implement MessageUtil::hash function to consistently hash Any recursively #8231

Merged
merged 7 commits into from
Sep 20, 2019
Merged

proto: re-implement MessageUtil::hash function to consistently hash Any recursively #8231

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
57972f1
proto: define a hash function can consistently hash Any and use in se…
lizan Sep 13, 2019
14c2213
link protobuf issue
lizan Sep 13, 2019
366e2ee
spell
lizan Sep 17, 2019
7b289ec
use TextFormat directly
lizan Sep 18, 2019
795c9be
negative test
lizan Sep 18, 2019
a20fca1
unify hash functions
lizan Sep 20, 2019
ee690d0
Merge remote-tracking branch 'upstream/master' into stable_hash_any
lizan Sep 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions source/common/protobuf/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,16 @@ class MessageUtil {
return HashUtil::xxHash64(text);
}

/**
* A hash function that stable hashes known Any in the message. This is much slower than
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems incredibly fragile. Shouldn't the above function handle this correctly? Is there a protobuf issue tracking this that we can link to / TODO?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this seems pretty bad due to the expectation that debug string is stable in any way. I feel it might be one of the least bad things for now though. We might want to move away from using any form of protobuf hashing in the future, instead opting to only consider explicit UUIDs or versions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I updated the implementation to use TextFormat directly with more settings to TextFormat::Printer explicitly, as well as more test to validate map as well. Does this look better now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but TBH, I still think we should take a long term (beyond this PR) think and see if we can distance ourselves from proto hashing. There have been too many bugs here and proto is unlikely to standardize a hash due to the implications on serialization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I'm not sure how the developer is supposed to know which of these to use. We need a holistic solution that avoids this problem entirely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry to interrupt) I think using the resource version should take preference over hashing. There may be cases when the control plan wants to trigger a reload for a config that is structurally the same (e.g. references some modified file or resource by name).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 @kyessenov Both SGTM, filed #8301 for further discussion, it is beyond the scope of this PR.

@mattklein123 how do you want this PR land? I can make this as default hash and use everywhere (solves potential issue in other places like RDS, but slightly less performant), or leave as is to at least solve SDS issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal feeling is that we should use a single hash that works in all cases, even if it performs worse, until we see perf traces that prove that we shouldn't do that. @htuch any opinion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, single hash function sounds best. If we can simplify and then only add in the necessary complexity for performance reasons, that would be ideal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and updated title/description.

* hash above, should be used when known Any is in there.
* See https://github.com/protocolbuffers/protobuf/issues/5731 for more context, we use
* ShortDebugString because it does parse and serialize Anys as debug string recursively.
*/
static std::size_t anyStableHash(const Protobuf::Message& message) {
return HashUtil::xxHash64(message.ShortDebugString());
}

static void loadFromJson(const std::string& json, Protobuf::Message& message,
ProtobufMessage::ValidationVisitor& validation_visitor);
static void loadFromJson(const std::string& json, ProtobufWkt::Struct& message);
Expand Down
2 changes: 1 addition & 1 deletion source/common/secret/secret_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class SecretManagerImpl : public SecretManager {
const std::string& config_name,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
const std::string map_key =
absl::StrCat(MessageUtil::hash(sds_config_source), ".", config_name);
absl::StrCat(MessageUtil::anyStableHash(sds_config_source), ".", config_name);

std::shared_ptr<SecretType> secret_provider = dynamic_secret_providers_[map_key].lock();
if (!secret_provider) {
Expand Down
1 change: 1 addition & 0 deletions test/common/secret/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ envoy_cc_test(
"//test/test_common:registry_lib",
"//test/test_common:simulated_time_system_lib",
"//test/test_common:utility_lib",
"@envoy_api//envoy/config/grpc_credential/v2alpha:file_based_metadata_cc",
],
)

Expand Down
86 changes: 86 additions & 0 deletions test/common/secret/secret_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
#include "envoy/admin/v2alpha/config_dump.pb.h"
#include "envoy/api/v2/auth/cert.pb.h"
#include "envoy/common/exception.h"
#include "envoy/config/grpc_credential/v2alpha/file_based_metadata.pb.h"

#include "common/common/base64.h"
#include "common/common/logger.h"
#include "common/secret/sds_api.h"
#include "common/secret/secret_manager_impl.h"
Expand Down Expand Up @@ -165,6 +167,90 @@ name: "abc.com"
"Secret type not implemented");
}

// Validate that secret manager deduplicates dynamic TLS certificate secret provider.
// Regression test of https://github.com/envoyproxy/envoy/issues/5744
TEST_F(SecretManagerImplTest, DeduplicateDynamicTlsCertificateSecretProvider) {
Server::MockInstance server;
std::unique_ptr<SecretManager> secret_manager(new SecretManagerImpl(config_tracker_));

NiceMock<Server::Configuration::MockTransportSocketFactoryContext> secret_context;

NiceMock<LocalInfo::MockLocalInfo> local_info;
NiceMock<Event::MockDispatcher> dispatcher;
NiceMock<Runtime::MockRandomGenerator> random;
Stats::IsolatedStoreImpl stats;
NiceMock<Init::MockManager> init_manager;
NiceMock<Init::ExpectableWatcherImpl> init_watcher;
Init::TargetHandlePtr init_target_handle;
EXPECT_CALL(init_manager, add(_))
.WillRepeatedly(Invoke([&init_target_handle](const Init::Target& target) {
init_target_handle = target.createHandle("test");
}));
EXPECT_CALL(secret_context, stats()).WillRepeatedly(ReturnRef(stats));
EXPECT_CALL(secret_context, initManager()).WillRepeatedly(Return(&init_manager));
EXPECT_CALL(secret_context, dispatcher()).WillRepeatedly(ReturnRef(dispatcher));
EXPECT_CALL(secret_context, localInfo()).WillRepeatedly(ReturnRef(local_info));

envoy::api::v2::core::ConfigSource config_source;
TestUtility::loadFromYaml(R"(
api_config_source:
api_type: GRPC
grpc_services:
- google_grpc:
call_credentials:
- from_plugin:
name: envoy.grpc_credentials.file_based_metadata
typed_config:
"@type": type.googleapis.com/envoy.config.grpc_credential.v2alpha.FileBasedMetadataConfig
stat_prefix: sdsstat
credentials_factory_name: envoy.grpc_credentials.file_based_metadata
)",
config_source);
config_source.mutable_api_config_source()
->mutable_grpc_services(0)
->mutable_google_grpc()
->mutable_call_credentials(0)
->mutable_from_plugin()
->mutable_typed_config()
->set_value(Base64::decode("CjUKMy92YXIvcnVuL3NlY3JldHMva3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3Vud"
"C90b2tlbhILeC10b2tlbi1iaW4="));
auto secret_provider1 =
secret_manager->findOrCreateTlsCertificateProvider(config_source, "abc.com", secret_context);

// The base64 encoded proto binary is identical to the one above, but in different field order.
// It is also identical to the YAML below.
config_source.mutable_api_config_source()
->mutable_grpc_services(0)
->mutable_google_grpc()
->mutable_call_credentials(0)
->mutable_from_plugin()
->mutable_typed_config()
->set_value(Base64::decode("Egt4LXRva2VuLWJpbgo1CjMvdmFyL3J1bi9zZWNyZXRzL2t1YmVybmV0ZXMuaW8vc"
"2VydmljZWFjY291bnQvdG9rZW4="));
auto secret_provider2 =
secret_manager->findOrCreateTlsCertificateProvider(config_source, "abc.com", secret_context);

envoy::config::grpc_credential::v2alpha::FileBasedMetadataConfig file_based_metadata_config;
TestUtility::loadFromYaml(R"(
header_key: x-token-bin
secret_data:
filename: "/var/run/secrets/kubernetes.io/serviceaccount/token"
)",
file_based_metadata_config);
config_source.mutable_api_config_source()
->mutable_grpc_services(0)
->mutable_google_grpc()
->mutable_call_credentials(0)
->mutable_from_plugin()
->mutable_typed_config()
->PackFrom(file_based_metadata_config);
auto secret_provider3 =
secret_manager->findOrCreateTlsCertificateProvider(config_source, "abc.com", secret_context);

EXPECT_EQ(secret_provider1, secret_provider2);
EXPECT_EQ(secret_provider2, secret_provider3);
}

TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) {
Server::MockInstance server;
std::unique_ptr<SecretManager> secret_manager(new SecretManagerImpl(config_tracker_));
Expand Down