Skip to content

Refactor SecretManagerImpl to templatize findOrCreate method. #4448

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

Merged
htuch merged 3 commits into from
Sep 18, 2018
Merged

Refactor SecretManagerImpl to templatize findOrCreate method. #4448

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bfda816
Refactor SecretManagerImpl to templatize findOrCreate method.
JimmyCYJ Sep 18, 2018
30c6bda
Revise per comments.
JimmyCYJ Sep 18, 2018
cc8b2de
Revise per comments.
JimmyCYJ Sep 18, 2018
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: 7 additions & 3 deletions source/common/secret/sds_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,18 @@ class SdsApi : public Init::Target,
Cleanup clean_up_;
};

typedef std::shared_ptr<SdsApi> SdsApiSharedPtr;
class TlsCertificateSdsApi;
class CertificateValidationContextSdsApi;
typedef std::shared_ptr<TlsCertificateSdsApi> TlsCertificateSdsApiSharedPtr;
typedef std::shared_ptr<CertificateValidationContextSdsApi>
CertificateValidationContextSdsApiSharedPtr;

/**
* TlsCertificateSdsApi implementation maintains and updates dynamic TLS certificate secrets.
*/
class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider {
public:
static SdsApiSharedPtr
static TlsCertificateSdsApiSharedPtr
create(Server::Configuration::TransportSocketFactoryContext& secret_provider_context,
const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name,
std::function<void()> destructor_cb) {
Expand Down Expand Up @@ -119,7 +123,7 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider
class CertificateValidationContextSdsApi : public SdsApi,
public CertificateValidationContextConfigProvider {
public:
static SdsApiSharedPtr
static CertificateValidationContextSdsApiSharedPtr
create(Server::Configuration::TransportSocketFactoryContext& secret_provider_context,
const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name,
std::function<void()> destructor_cb) {
Expand Down
46 changes: 12 additions & 34 deletions source/common/secret/secret_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,59 +64,37 @@ SecretManagerImpl::createInlineCertificateValidationContextProvider(
certificate_validation_context);
}

void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) {
ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key);

auto num_deleted = dynamic_secret_providers_.erase(map_key);
ASSERT(num_deleted == 1, "");
}

SdsApiSharedPtr SecretManagerImpl::findOrCreate(
const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
std::function<SdsApiSharedPtr(std::function<void()> unregister_secret_provider)> create_fn) {
const std::string map_key = sds_config_source.SerializeAsString() + config_name;

SdsApiSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock();
if (!secret_provider) {
// SdsApi is owned by ListenerImpl and ClusterInfo which are destroyed before
// SecretManagerImpl. It is safe to invoke this callback at the destructor of SdsApi.
std::function<void()> unregister_secret_provider = [map_key, this]() {
removeDynamicSecretProvider(map_key);
};

secret_provider = create_fn(unregister_secret_provider);
dynamic_secret_providers_[map_key] = secret_provider;
}
return secret_provider;
}

TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider(
const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
auto create_fn = [&secret_provider_context, &sds_config_source, &config_name](
std::function<void()> unregister_secret_provider) -> SdsApiSharedPtr {
auto create_fn =
[&secret_provider_context, &sds_config_source, &config_name](
std::function<void()> unregister_secret_provider) -> TlsCertificateSdsApiSharedPtr {
ASSERT(secret_provider_context.initManager() != nullptr);
return TlsCertificateSdsApi::create(secret_provider_context, sds_config_source, config_name,
unregister_secret_provider);
};
SdsApiSharedPtr secret_provider = findOrCreate(sds_config_source, config_name, create_fn);
TlsCertificateSdsApiSharedPtr secret_provider =
certificate_providers_.findOrCreate(sds_config_source, config_name, create_fn);

return std::dynamic_pointer_cast<TlsCertificateConfigProvider>(secret_provider);
return secret_provider;
}

CertificateValidationContextConfigProviderSharedPtr
SecretManagerImpl::findOrCreateCertificateValidationContextProvider(
const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
auto create_fn = [&secret_provider_context, &sds_config_source, &config_name](
std::function<void()> unregister_secret_provider) -> SdsApiSharedPtr {
auto create_fn = [&secret_provider_context, &sds_config_source,
&config_name](std::function<void()> unregister_secret_provider)
-> CertificateValidationContextSdsApiSharedPtr {
ASSERT(secret_provider_context.initManager() != nullptr);
return CertificateValidationContextSdsApi::create(secret_provider_context, sds_config_source,
config_name, unregister_secret_provider);
};
SdsApiSharedPtr secret_provider = findOrCreate(sds_config_source, config_name, create_fn);
CertificateValidationContextSdsApiSharedPtr secret_provider =
validation_context_providers_.findOrCreate(sds_config_source, config_name, create_fn);

return std::dynamic_pointer_cast<CertificateValidationContextConfigProvider>(secret_provider);
return secret_provider;
}

} // namespace Secret
Expand Down
43 changes: 36 additions & 7 deletions source/common/secret/secret_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,40 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable<Logger::Id::sec
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override;

private:
// Removes dynamic secret provider which has been deleted.
void removeDynamicSecretProvider(const std::string& map_key);
// Finds or creates SdsApi object.
SdsApiSharedPtr findOrCreate(
const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
std::function<SdsApiSharedPtr(std::function<void()> unregister_secret_provider)> create_fn);
template <class SecretType> class DynamicSecretProviders {
public:
// Finds or creates SdsApi object.
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) {

@qiwzhang qiwzhang Sep 18, 2018

Copy link
Copy Markdown
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.

@JimmyCYJ JimmyCYJ Sep 18, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done. Thanks.

const std::string map_key = sds_config_source.SerializeAsString() + config_name;

std::shared_ptr<SecretType> secret_provider = dynamic_secret_providers_[map_key].lock();
if (!secret_provider) {
// SdsApi is owned by ListenerImpl and ClusterInfo which are destroyed before
// SecretManagerImpl. It is safe to invoke this callback at the destructor of SdsApi.
std::function<void()> unregister_secret_provider = [map_key, this]() {
removeDynamicSecretProvider(map_key);
};

secret_provider = create_fn(unregister_secret_provider);
dynamic_secret_providers_[map_key] = secret_provider;
}
return secret_provider;
}

private:
// Removes dynamic secret provider which has been deleted.
void removeDynamicSecretProvider(const std::string& map_key) {
ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key);

auto num_deleted = dynamic_secret_providers_.erase(map_key);
ASSERT(num_deleted == 1, "");
}

std::unordered_map<std::string, std::weak_ptr<SecretType>> dynamic_secret_providers_;
};

// Manages pairs of secret name and TlsCertificateConfigProviderSharedPtr.
std::unordered_map<std::string, TlsCertificateConfigProviderSharedPtr>
Expand All @@ -58,7 +86,8 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable<Logger::Id::sec
static_certificate_validation_context_providers_;

// map hash code of SDS config source and SdsApi object.
std::unordered_map<std::string, std::weak_ptr<SdsApi>> dynamic_secret_providers_;
DynamicSecretProviders<TlsCertificateSdsApi> certificate_providers_;
DynamicSecretProviders<CertificateValidationContextSdsApi> validation_context_providers_;
};

} // namespace Secret
Expand Down