-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
use hash func that generates deterministic result for protobuf #5814
Changes from 5 commits
3299ceb
b7957ec
cffd4e9
3396c2d
ed28a13
1d79741
7ce7966
077ba0d
8a7bb58
eb10a74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = | ||
std::to_string(MessageUtil::hash(sds_config_source)) + config_name; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will a number started config_name potentially conflicts with other hashes? i.e. a config hash There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potentially, yes, though I'm not sure how well do we guard against that in rest of the codebase. @quanjielin could you add a separator? e.g.
Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that will be good enough There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
std::shared_ptr<SecretType> secret_provider = dynamic_secret_providers_[map_key].lock(); | ||
if (!secret_provider) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just use uint64_t for map_key type and change type elsewhere, no need convert to string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since resourcename is also needed to construct mapkey, are you suggesting using something like
MessageUtil::hash(sds_config_source+config_name)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry I meant rds_impl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.