istio · qiwzhang · Jun 21, 2018
diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h
@@ -8,6 +8,17 @@
 namespace Envoy {
 namespace Secret {
 
+  class DynamicSecretProvider {
+  public:
+    virtual ~SecretProvider() {}
+
+    virtual const Ssl::TlsCertificateConfigSharedPtr secret() const PURE;
+
+    virtual void addUpdateCallback(SecretCallbacks& callback) PURE;
+    virtual void removeUpdateCallback(SecretCallbacks& callback) PURE;
+  }
+  typedef std::shared_ptr<DynamicSecretProvider> DynamicSecretProviderSharedPtr;
+
 /**
  * A manager for static and dynamic secrets.
  */
@@ -21,16 +32,15 @@ class SecretManager {
    * @param secret a protobuf message of envoy::api::v2::auth::Secret.
    * @throw an EnvoyException if the secret is invalid or not supported.
    */
-  virtual void addOrUpdateSecret(const std::string& config_source_hash,
-                                 const envoy::api::v2::auth::Secret& secret) PURE;
+  virtual void addStaticSecret(const envoy::api::v2::auth::Secret& secret) PURE;
 
   /**
    * @param sds_config_source_hash hash string of normalized config source.
    * @param name a name of the Ssl::TlsCertificateConfig.
    * @return the TlsCertificate secret. Returns nullptr if the secret is not found.
    */
   virtual const Ssl::TlsCertificateConfigSharedPtr
-  findTlsCertificate(const std::string& config_source_hash, const std::string& name) const PURE;
+  findStaticTlsCertificate(const std::string& name) const PURE;
 
   /**
    * Add or update SDS config source. SecretManager starts downloading secrets from registered
@@ -40,19 +50,9 @@ class SecretManager {
    * @param config_name a name that uniquely refers to the SDS config source.
    * @return a hash string of normalized config source.
    */
-  virtual std::string addOrUpdateSdsService(const envoy::api::v2::core::ConfigSource& config_source,
-                                            std::string config_name) PURE;
+  virtual DynamicSecretProviderSharedPtr createDynamicSecretProvider(const envoy::api::v2::core::ConfigSource& config_source,
+						     std::string config_name) PURE;
 
-  /**
-   * Register callback function which is to be invoked on secret update.
-   *
-   * @param config_source_hash Hash code of ConfigSource.
-   * @param secret_name name of the secret.
-   * @param callback SecretCallbacks class.
-   */
-  virtual void registerTlsCertificateConfigCallbacks(const std::string& config_source_hash,
-                                                     const std::string& secret_name,
-                                                     SecretCallbacks& callback) PURE;
 };
 
 } // namespace Secret

diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc
@@ -34,15 +34,30 @@ void SdsApi::initialize(std::function<void()> callback) {
 }
 
 void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) {
-  for (const auto& resource : resources) {
-    switch (resource.type_case()) {
-    case envoy::api::v2::auth::Secret::kTlsCertificate:
-      server_.secretManager().addOrUpdateSecret(sds_config_source_hash_, resource);
-      break;
-    case envoy::api::v2::auth::Secret::kSessionTicketKeys:
-      NOT_IMPLEMENTED;
-    default:
-      throw EnvoyException("sds: invalid configuration");
+  if (resources.empty()) {
+    ENVOY_LOG(debug, "Missing RouteConfiguration for {} in onConfigUpdate()", route_config_name_);
+    runInitializeCallbackIfAny();
+    return;
+  }
+  if (resources.size() != 1) {
+    throw EnvoyException(fmt::format("Unexpected RDS resource length: {}", resources.size()));
+  }
+  const auto& secret = resources[0];
+  MessageUtil::validate(secret);
+  // TODO(PiotrSikora): Remove this hack once fixed internally.
+  if (!(secret.name() == sds_config_name_)) {
+    throw EnvoyException(fmt::format("Unexpected RDS configuration (expecting {}): {}",
+				     sds_config_name_, secret.name()));
+  }
+  const uint64_t new_hash = MessageUtil::hash(secret);
+  if (new_hash != secret_hash_) 
+    if (secret.type_case() == envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate) {
+      tls_certificate_secret_ =
+        std::make_shared<Ssl::TlsCertificateConfigImpl>(secret.tls_certificate());
+
+      for (auto cb : update_callbakcs_) {
+	cb->onAddOrUpdateSecret();
+      }
     }
   }
 
@@ -62,4 +77,4 @@ void SdsApi::runInitializeCallbackIfAny() {
 }
 
 } // namespace Secret
-} // namespace Envoy
+} // namespace Envoy
diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h
@@ -13,7 +13,9 @@ namespace Secret {
 /**
  * SDS API implementation that fetches secrets from SDS server via Subscription.
  */
-class SdsApi : public Init::Target, Config::SubscriptionCallbacks<envoy::api::v2::auth::Secret> {
+class SdsApi : public Init::Target,
+  public DynamicSecretProvider,
+  public Config::SubscriptionCallbacks<envoy::api::v2::auth::Secret> {
 public:
   SdsApi(Server::Instance& server, const envoy::api::v2::core::ConfigSource& sds_config,
          std::string sds_config_hash, std::string sds_config_name);
@@ -28,6 +30,17 @@ class SdsApi : public Init::Target, Config::SubscriptionCallbacks<envoy::api::v2
     return MessageUtil::anyConvert<envoy::api::v2::auth::Secret>(resource).name();
   }
 
+  // DynamicSecretProvider
+  const Ssl::TlsCertificateConfigSharedPtr secret() const override {
+    return tls_certificate_secrets_;
+  }
+  void addUpdateCallback(SecretCallbacks& callback) override {
+    update_callbacks_.push_back(*callback);
+  }
+  void removeUpdateCallback(SecretCallbacks& callback) override {
+    update_callbacks_.remove(*callback);
+  }
+
 private:
   void runInitializeCallbackIfAny();
 
@@ -37,9 +50,13 @@ class SdsApi : public Init::Target, Config::SubscriptionCallbacks<envoy::api::v2
   std::unique_ptr<Config::Subscription<envoy::api::v2::auth::Secret>> subscription_;
   std::function<void()> initialize_callback_;
   std::string sds_config_name_;
+
+  std::string secret_hash_;
+  Ssl::TlsCertificateConfigSharedPtr tls_certificate_secrets_;
+  std::list<SecretCallbacks*> update_callbacks_;
 };
 
 typedef std::unique_ptr<SdsApi> SdsApiPtr;
 
 } // namespace Secret
-} // namespace Envoy
+} // namespace Envoy
diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc
@@ -8,54 +8,30 @@
 namespace Envoy {
 namespace Secret {
 
-void SecretManagerImpl::addOrUpdateSecret(const std::string& config_source_hash,
-                                          const envoy::api::v2::auth::Secret& secret) {
+void SecretManagerImpl::addStaticSecret( const envoy::api::v2::auth::Secret& secret) {
   switch (secret.type_case()) {
   case envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate: {
     std::unique_lock<std::shared_timed_mutex> lhs(tls_certificate_secrets_mutex_);
     auto tls_certificate_secret =
         std::make_shared<Ssl::TlsCertificateConfigImpl>(secret.tls_certificate());
-    tls_certificate_secrets_[config_source_hash][secret.name()] = tls_certificate_secret;
+    tls_certificate_secrets_[secret.name()] = tls_certificate_secret;
 
     if (config_source_hash.empty()) {
       return;
     }
-
-    std::string secret_name = secret.name();
-    server_.dispatcher().post([this, config_source_hash, secret_name, secret]() {
-      std::unique_lock<std::shared_timed_mutex> lhs(tls_certificate_secret_update_callbacks_mutex_);
-      auto config_source_it = tls_certificate_secret_update_callbacks_.find(config_source_hash);
-      if (config_source_it != tls_certificate_secret_update_callbacks_.end()) {
-        auto callback_it = config_source_it->second.find(secret_name);
-        if (callback_it != config_source_it->second.end()) {
-          if (callback_it->second.first == nullptr ||
-              !callback_it->second.first->equalTo(*tls_certificate_secret)) {
-            for (auto& callback : callback_it->second.second) {
-              callback->onAddOrUpdateSecret();
-            }
-            callback_it->second.first = secret;
-          }
-        }
-      }
-    });
   } break;
   default:
     throw EnvoyException("Secret type not implemented");
   }
 }
 
 const Ssl::TlsCertificateConfigSharedPtr
-SecretManagerImpl::findTlsCertificate(const std::string& config_source_hash,
+SecretManagerImpl::findStaticTlsCertificate(const std::string& config_source_hash,
                                       const std::string& name) const {
   std::shared_lock<std::shared_timed_mutex> lhs(tls_certificate_secrets_mutex_);
 
-  auto config_source_it = tls_certificate_secrets_.find(config_source_hash);
-  if (config_source_it == tls_certificate_secrets_.end()) {
-    return nullptr;
-  }
-
-  auto secret = config_source_it->second.find(name);
-  return (secret != config_source_it->second.end()) ? secret->second : nullptr;
+  auto secret = tls_certificate_secrets_.find(name);
+  return (secret != tls_certificate_secrets_.end()) ? secret->second : nullptr;
 }
 
 std::string SecretManagerImpl::addOrUpdateSdsService(
@@ -64,36 +40,13 @@ std::string SecretManagerImpl::addOrUpdateSdsService(
 
   auto hash = SecretManagerUtil::configSourceHash(sds_config_source);
   std::string sds_apis_key = hash + config_name;
-  if (sds_apis_.find(sds_apis_key) != sds_apis_.end()) {
-    return hash;
-  }
-
-  sds_apis_[sds_apis_key] = std::make_unique<SdsApi>(server_, sds_config_source, hash, config_name);
-
-  return hash;
-}
-
-void SecretManagerImpl::registerTlsCertificateConfigCallbacks(const std::string& config_source_hash,
-                                                              const std::string& secret_name,
-                                                              SecretCallbacks& callback) {
-  auto secret = findTlsCertificate(config_source_hash, secret_name);
-
-  std::unique_lock<std::shared_timed_mutex> lhs(tls_certificate_secret_update_callbacks_mutex_);
-
-  auto config_source_it = tls_certificate_secret_update_callbacks_.find(config_source_hash);
-  if (config_source_it == tls_certificate_secret_update_callbacks_.end()) {
-    tls_certificate_secret_update_callbacks_[config_source_hash][secret_name] = {secret,
-                                                                                 {&callback}};
-    return;
-  }
-
-  auto name_it = config_source_it->second.find(secret_name);
-  if (name_it == config_source_it->second.end()) {
-    config_source_it->second[secret_name] = {secret, {&callback}};
-    return;
+  auto sds_api = sds_apis_[sds_apis_key].lock();
+  if (!sds_api) {
+    sds_api = std::make_shared<SdsApi>(server_, sds_config_source, hash, config_name);
+    sds_apis_[sds_apis_key] = sds_api;
   }
 
-  name_it->second.second.push_back(&callback);
+  return sds_api;
 }
 
 } // namespace Secret

diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h
@@ -17,30 +17,27 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable<Logger::Id::ups
 public:
   SecretManagerImpl(Server::Instance& server) : server_(server) {}
 
-  void addOrUpdateSecret(const std::string& config_source_hash,
-                         const envoy::api::v2::auth::Secret& secret) override;
+  void addStaticSecret(const envoy::api::v2::auth::Secret& secret) override;
   const Ssl::TlsCertificateConfigSharedPtr
-  findTlsCertificate(const std::string& config_source_hash, const std::string& name) const override;
+  findStaticTlsCertificate(const std::string& name) const override;
 
   std::string addOrUpdateSdsService(const envoy::api::v2::core::ConfigSource& config_source,
                                     std::string config_name) override;
 
-  void registerTlsCertificateConfigCallbacks(const std::string& config_source_hash,
-                                             const std::string& secret_name,
-                                             SecretCallbacks& callback) override;
+  virtual DynamicSecretProviderSharedPtr createDynamicSecretProvider(const envoy::api::v2::core::ConfigSource& config_source,
+						     std::string config_name) override;
 
 private:
   Server::Instance& server_;
 
   // map hash code of SDS config source and SdsApi object.
-  std::unordered_map<std::string, SdsApiPtr> sds_apis_;
+  std::unordered_map<std::string, std::weak_ptr<DynamicSecretProvider>> sds_apis_;
   mutable std::shared_timed_mutex sds_api_mutex_;
 
   // Manages pairs of name and Ssl::TlsCertificateConfig grouped by SDS config source hash.
   // If SDS config source hash is empty, it is a static secret.
-  std::unordered_map<std::string,
-                     std::unordered_map<std::string, Ssl::TlsCertificateConfigSharedPtr>>
-      tls_certificate_secrets_;
+  std::unordered_map<std::string, Ssl::TlsCertificateConfigSharedPtr>
+    tls_certificate_secrets_;
   mutable std::shared_timed_mutex tls_certificate_secrets_mutex_;
 
   // callback functions for secret update