Skip to content

Refactor SecretManagerImpl to templatize findOrCreate method.#4448

Merged
htuch merged 3 commits intoenvoyproxy:masterfrom
istio:templatize_findOrCreate
Sep 18, 2018
Merged

Refactor SecretManagerImpl to templatize findOrCreate method.#4448
htuch merged 3 commits intoenvoyproxy:masterfrom
istio:templatize_findOrCreate

Conversation

@JimmyCYJ
Copy link
Member

Signed-off-by: JimmyCYJ jimmychen.0102@gmail.com

Description: According to comment in PR #4355, we want to avoid std::dynamic_pointer_cast in SecretManagerImpl. This PR creates a template class and moves findOrCreate method into the template class.
Risk Level: Low
Testing: Existing unit tests and integration tests.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
@JimmyCYJ
Copy link
Member Author

cc @qiwzhang @lizan

std::shared_ptr<SecretType> findOrCreate(
const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
std::function<std::shared_ptr<SecretType>(std::function<void()> unregister_secret_provider)>
create_fn) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not to pass in create_fn. The creation can be done in this template class.

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. Thanks.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
};
TlsCertificateSdsApiSharedPtr secret_provider =
certificate_providers_.findOrCreate(sds_config_source, config_name, create_fn);
certificate_providers_.findOrCreate(sds_config_source, config_name, secret_provider_context);
Copy link
Contributor

Choose a reason for hiding this comment

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

not need a separate variable

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.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
Copy link
Contributor

@qiwzhang qiwzhang left a comment

Choose a reason for hiding this comment

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

LGTM

@htuch htuch self-assigned this Sep 18, 2018
@JimmyCYJ
Copy link
Member Author

Thanks a lot for reviewing this PR.

@htuch htuch merged commit 7f899f5 into envoyproxy:master Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants