Merge pull request #25120 from markdroth/xds_security_aggregate_cluster_fix

change xDS certificate provider to contain a map by cluster

src/core/ext/filters/client_channel/lb_policy/xds/cds.cc

@@ -270,6 +270,17 @@ void CdsLb::ShutdownLocked() {
     }
     xds_client_.reset(DEBUG_LOCATION, "CdsLb");
   }
+  if (xds_certificate_provider_ != nullptr) {
+    // Unregister root and identity distributors from xDS provider, so
+    // that we don't leak memory.
+    // TODO(yashkt): This should not be necessary.  Consider changing
+    // the provider API to use DualRefCounted<> instead of RefCounted<>,
+    // so that the xDS provider can use weak refs internally.
+    xds_certificate_provider_->UpdateRootCertNameAndDistributor(
+        config_->cluster(), "", nullptr);
+    xds_certificate_provider_->UpdateIdentityCertNameAndDistributor(
+        config_->cluster(), "", nullptr);
+  }
   grpc_channel_args_destroy(args_);
   args_ = nullptr;
 }
@@ -453,18 +464,16 @@ grpc_error* CdsLb::UpdateXdsCertificateProvider(
     xds_certificate_provider_ = nullptr;
     return GRPC_ERROR_NONE;
   }
+  if (xds_certificate_provider_ == nullptr) {
+    xds_certificate_provider_ = MakeRefCounted<XdsCertificateProvider>();
+  }
+  // Configure root cert.
   absl::string_view root_provider_instance_name =
       cluster_data.common_tls_context.combined_validation_context
           .validation_context_certificate_provider_instance.instance_name;
   absl::string_view root_provider_cert_name =
       cluster_data.common_tls_context.combined_validation_context
           .validation_context_certificate_provider_instance.certificate_name;
-  absl::string_view identity_provider_instance_name =
-      cluster_data.common_tls_context
-          .tls_certificate_certificate_provider_instance.instance_name;
-  absl::string_view identity_provider_cert_name =
-      cluster_data.common_tls_context
-          .tls_certificate_certificate_provider_instance.certificate_name;
   RefCountedPtr<XdsCertificateProvider> new_root_provider;
   if (!root_provider_instance_name.empty()) {
     new_root_provider =
@@ -491,6 +500,18 @@ grpc_error* CdsLb::UpdateXdsCertificateProvider(
     }
     root_certificate_provider_ = std::move(new_root_provider);
   }
+  xds_certificate_provider_->UpdateRootCertNameAndDistributor(
+      config_->cluster(), root_provider_cert_name,
+      root_certificate_provider_ == nullptr
+          ? nullptr
+          : root_certificate_provider_->distributor());
+  // Configure identity cert.
+  absl::string_view identity_provider_instance_name =
+      cluster_data.common_tls_context
+          .tls_certificate_certificate_provider_instance.instance_name;
+  absl::string_view identity_provider_cert_name =
+      cluster_data.common_tls_context
+          .tls_certificate_certificate_provider_instance.certificate_name;
   RefCountedPtr<XdsCertificateProvider> new_identity_provider;
   if (!identity_provider_instance_name.empty()) {
     new_identity_provider =
@@ -517,53 +538,17 @@ grpc_error* CdsLb::UpdateXdsCertificateProvider(
     }
     identity_certificate_provider_ = std::move(new_identity_provider);
   }
+  xds_certificate_provider_->UpdateIdentityCertNameAndDistributor(
+      config_->cluster(), identity_provider_cert_name,
+      identity_certificate_provider_ == nullptr
+          ? nullptr
+          : identity_certificate_provider_->distributor());
+  // Configure SAN matchers.
   const std::vector<XdsApi::StringMatcher>& match_subject_alt_names =
       cluster_data.common_tls_context.combined_validation_context
           .default_validation_context.match_subject_alt_names;
-  if (!root_provider_instance_name.empty() &&
+  xds_certificate_provider_->UpdateSubjectAlternativeNameMatchers(
-      !identity_provider_instance_name.empty()) {
+      config_->cluster(), match_subject_alt_names);
-    // Using mTLS configuration
-    if (xds_certificate_provider_ != nullptr &&
-        xds_certificate_provider_->ProvidesRootCerts() &&
-        xds_certificate_provider_->ProvidesIdentityCerts()) {
-      xds_certificate_provider_->UpdateRootCertNameAndDistributor(
-          root_provider_cert_name, root_certificate_provider_->distributor());
-      xds_certificate_provider_->UpdateIdentityCertNameAndDistributor(
-          identity_provider_cert_name,
-          identity_certificate_provider_->distributor());
-      xds_certificate_provider_->UpdateSubjectAlternativeNameMatchers(
-          match_subject_alt_names);
-    } else {
-      // Existing xDS certificate provider does not have mTLS configuration.
-      // Create new certificate provider so that new subchannel connectors are
-      // created.
-      xds_certificate_provider_ = MakeRefCounted<XdsCertificateProvider>(
-          root_provider_cert_name, root_certificate_provider_->distributor(),
-          identity_provider_cert_name,
-          identity_certificate_provider_->distributor(),
-          match_subject_alt_names);
-    }
-  } else if (!root_provider_instance_name.empty()) {
-    // Using TLS configuration
-    if (xds_certificate_provider_ != nullptr &&
-        xds_certificate_provider_->ProvidesRootCerts() &&
-        !xds_certificate_provider_->ProvidesIdentityCerts()) {
-      xds_certificate_provider_->UpdateRootCertNameAndDistributor(
-          root_provider_cert_name, root_certificate_provider_->distributor());
-      xds_certificate_provider_->UpdateSubjectAlternativeNameMatchers(
-          match_subject_alt_names);
-    } else {
-      // Existing xDS certificate provider does not have TLS configuration.
-      // Create new certificate provider so that new subchannel connectors are
-      // created.
-      xds_certificate_provider_ = MakeRefCounted<XdsCertificateProvider>(
-          root_provider_cert_name, root_certificate_provider_->distributor(),
-          "", nullptr, match_subject_alt_names);
-    }
-  } else {
-    // No configuration provided.
-    xds_certificate_provider_ = nullptr;
-  }
   return GRPC_ERROR_NONE;
 }
 

src/core/ext/xds/xds_certificate_provider.cc

@@ -37,15 +37,16 @@ class RootCertificatesWatcher
   // presently, the watcher is immediately deleted when
   // CancelTlsCertificatesWatch() is called, but that can potentially change in
   // the future.
-  explicit RootCertificatesWatcher(
+  RootCertificatesWatcher(
-      RefCountedPtr<grpc_tls_certificate_distributor> parent)
+      RefCountedPtr<grpc_tls_certificate_distributor> parent,
-      : parent_(std::move(parent)) {}
+      std::string cert_name)
+      : parent_(std::move(parent)), cert_name_(std::move(cert_name)) {}
 
   void OnCertificatesChanged(absl::optional<absl::string_view> root_certs,
                              absl::optional<PemKeyCertPairList>
                              /* key_cert_pairs */) override {
     if (root_certs.has_value()) {
-      parent_->SetKeyMaterials("", std::string(root_certs.value()),
+      parent_->SetKeyMaterials(cert_name_, std::string(root_certs.value()),
                                absl::nullopt);
     }
   }
@@ -53,7 +54,7 @@ class RootCertificatesWatcher
   void OnError(grpc_error* root_cert_error,
                grpc_error* identity_cert_error) override {
     if (root_cert_error != GRPC_ERROR_NONE) {
-      parent_->SetErrorForCert("", root_cert_error /* pass the ref */,
+      parent_->SetErrorForCert(cert_name_, root_cert_error /* pass the ref */,
                                absl::nullopt);
     }
     GRPC_ERROR_UNREF(identity_cert_error);
@@ -61,6 +62,7 @@ class RootCertificatesWatcher
 
  private:
   RefCountedPtr<grpc_tls_certificate_distributor> parent_;
+  std::string cert_name_;
 };
 
 class IdentityCertificatesWatcher
@@ -71,22 +73,23 @@ class IdentityCertificatesWatcher
   // presently, the watcher is immediately deleted when
   // CancelTlsCertificatesWatch() is called, but that can potentially change in
   // the future.
-  explicit IdentityCertificatesWatcher(
+  IdentityCertificatesWatcher(
-      RefCountedPtr<grpc_tls_certificate_distributor> parent)
+      RefCountedPtr<grpc_tls_certificate_distributor> parent,
-      : parent_(std::move(parent)) {}
+      std::string cert_name)
+      : parent_(std::move(parent)), cert_name_(std::move(cert_name)) {}
 
   void OnCertificatesChanged(
       absl::optional<absl::string_view> /* root_certs */,
       absl::optional<PemKeyCertPairList> key_cert_pairs) override {
     if (key_cert_pairs.has_value()) {
-      parent_->SetKeyMaterials("", absl::nullopt, key_cert_pairs);
+      parent_->SetKeyMaterials(cert_name_, absl::nullopt, key_cert_pairs);
     }
   }
 
   void OnError(grpc_error* root_cert_error,
                grpc_error* identity_cert_error) override {
     if (identity_cert_error != GRPC_ERROR_NONE) {
-      parent_->SetErrorForCert("", absl::nullopt,
+      parent_->SetErrorForCert(cert_name_, absl::nullopt,
                                identity_cert_error /* pass the ref */);
     }
     GRPC_ERROR_UNREF(root_cert_error);
@@ -94,34 +97,35 @@ class IdentityCertificatesWatcher
 
  private:
   RefCountedPtr<grpc_tls_certificate_distributor> parent_;
+  std::string cert_name_;
 };
 
 }  // namespace
 
-XdsCertificateProvider::XdsCertificateProvider(
+//
-    absl::string_view root_cert_name,
+// XdsCertificateProvider::ClusterCertificateState
-    RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor,
+//
-    absl::string_view identity_cert_name,
+
-    RefCountedPtr<grpc_tls_certificate_distributor> identity_cert_distributor,
+XdsCertificateProvider::ClusterCertificateState::~ClusterCertificateState() {
-    std::vector<XdsApi::StringMatcher> san_matchers)
+  if (root_cert_watcher_ != nullptr) {
-    : root_cert_name_(root_cert_name),
+    root_cert_distributor_->CancelTlsCertificatesWatch(root_cert_watcher_);
-      identity_cert_name_(identity_cert_name),
+  }
-      root_cert_distributor_(std::move(root_cert_distributor)),
+  if (identity_cert_watcher_ != nullptr) {
-      identity_cert_distributor_(std::move(identity_cert_distributor)),
+    identity_cert_distributor_->CancelTlsCertificatesWatch(
-      san_matchers_(std::move(san_matchers)),
+        identity_cert_watcher_);
-      distributor_(MakeRefCounted<grpc_tls_certificate_distributor>()) {
+  }
-  distributor_->SetWatchStatusCallback(
-      absl::bind_front(&XdsCertificateProvider::WatchStatusCallback, this));
 }
 
-XdsCertificateProvider::~XdsCertificateProvider() {
+bool XdsCertificateProvider::ClusterCertificateState::IsSafeToRemove() const {
-  distributor_->SetWatchStatusCallback(nullptr);
+  return !watching_root_certs_ && !watching_identity_certs_ &&
+         root_cert_distributor_ == nullptr &&
+         identity_cert_distributor_ == nullptr;
 }
 
-void XdsCertificateProvider::UpdateRootCertNameAndDistributor(
+void XdsCertificateProvider::ClusterCertificateState::
-    absl::string_view root_cert_name,
+    UpdateRootCertNameAndDistributor(
-    RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor) {
+        const std::string& cert_name, absl::string_view root_cert_name,
-  MutexLock lock(&mu_);
+        RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor) {
   if (root_cert_name_ == root_cert_name &&
       root_cert_distributor_ == root_cert_distributor) {
     return;
@@ -133,10 +137,10 @@ void XdsCertificateProvider::UpdateRootCertNameAndDistributor(
       root_cert_distributor_->CancelTlsCertificatesWatch(root_cert_watcher_);
     }
     if (root_cert_distributor != nullptr) {
-      UpdateRootCertWatcher(root_cert_distributor.get());
+      UpdateRootCertWatcher(cert_name, root_cert_distributor.get());
     } else {
       root_cert_watcher_ = nullptr;
-      distributor_->SetErrorForCert(
+      xds_certificate_provider_->distributor_->SetErrorForCert(
           "",
           GRPC_ERROR_CREATE_FROM_STATIC_STRING(
               "No certificate provider available for root certificates"),
@@ -147,10 +151,11 @@ void XdsCertificateProvider::UpdateRootCertNameAndDistributor(
   root_cert_distributor_ = std::move(root_cert_distributor);
 }
 
-void XdsCertificateProvider::UpdateIdentityCertNameAndDistributor(
+void XdsCertificateProvider::ClusterCertificateState::
-    absl::string_view identity_cert_name,
+    UpdateIdentityCertNameAndDistributor(
-    RefCountedPtr<grpc_tls_certificate_distributor> identity_cert_distributor) {
+        const std::string& cert_name, absl::string_view identity_cert_name,
-  MutexLock lock(&mu_);
+        RefCountedPtr<grpc_tls_certificate_distributor>
+            identity_cert_distributor) {
   if (identity_cert_name_ == identity_cert_name &&
       identity_cert_distributor_ == identity_cert_distributor) {
     return;
@@ -163,10 +168,10 @@ void XdsCertificateProvider::UpdateIdentityCertNameAndDistributor(
           identity_cert_watcher_);
     }
     if (identity_cert_distributor != nullptr) {
-      UpdateIdentityCertWatcher(identity_cert_distributor.get());
+      UpdateIdentityCertWatcher(cert_name, identity_cert_distributor.get());
     } else {
       identity_cert_watcher_ = nullptr;
-      distributor_->SetErrorForCert(
+      xds_certificate_provider_->distributor_->SetErrorForCert(
           "", absl::nullopt,
           GRPC_ERROR_CREATE_FROM_STATIC_STRING(
               "No certificate provider available for identity certificates"));
@@ -176,42 +181,45 @@ void XdsCertificateProvider::UpdateIdentityCertNameAndDistributor(
   identity_cert_distributor_ = std::move(identity_cert_distributor);
 }
 
-void XdsCertificateProvider::UpdateSubjectAlternativeNameMatchers(
+void XdsCertificateProvider::ClusterCertificateState::UpdateRootCertWatcher(
-    std::vector<XdsApi::StringMatcher> matchers) {
+    const std::string& cert_name,
-  MutexLock lock(&san_matchers_mu_);
+    grpc_tls_certificate_distributor* root_cert_distributor) {
-  san_matchers_ = std::move(matchers);
+  auto watcher = absl::make_unique<RootCertificatesWatcher>(
+      xds_certificate_provider_->distributor_, cert_name);
+  root_cert_watcher_ = watcher.get();
+  root_cert_distributor->WatchTlsCertificates(std::move(watcher),
+                                              root_cert_name_, absl::nullopt);
 }
 
-void XdsCertificateProvider::WatchStatusCallback(std::string cert_name,
+void XdsCertificateProvider::ClusterCertificateState::UpdateIdentityCertWatcher(
-                                                 bool root_being_watched,
+    const std::string& cert_name,
-                                                 bool identity_being_watched) {
+    grpc_tls_certificate_distributor* identity_cert_distributor) {
+  auto watcher = absl::make_unique<IdentityCertificatesWatcher>(
+      xds_certificate_provider_->distributor_, cert_name);
+  identity_cert_watcher_ = watcher.get();
+  identity_cert_distributor->WatchTlsCertificates(
+      std::move(watcher), absl::nullopt, identity_cert_name_);
+}
+
+void XdsCertificateProvider::ClusterCertificateState::WatchStatusCallback(
+    const std::string& cert_name, bool root_being_watched,
+    bool identity_being_watched) {
   // We aren't specially handling the case where root_cert_distributor is same
   // as identity_cert_distributor. Always using two separate watchers
   // irrespective of the fact results in a straightforward design, and using a
   // single watcher does not seem to provide any benefit other than cutting down
   // on the number of callbacks.
-  MutexLock lock(&mu_);
-  if (!cert_name.empty()) {
-    grpc_error* error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(
-        absl::StrCat("Illegal certificate name: \'", cert_name,
-                     "\'. Should be empty.")
-            .c_str());
-    distributor_->SetErrorForCert(cert_name, GRPC_ERROR_REF(error),
-                                  GRPC_ERROR_REF(error));
-    GRPC_ERROR_UNREF(error);
-    return;
-  }
   if (root_being_watched && !watching_root_certs_) {
     // We need to start watching root certs.
     watching_root_certs_ = true;
     if (root_cert_distributor_ == nullptr) {
-      distributor_->SetErrorForCert(
+      xds_certificate_provider_->distributor_->SetErrorForCert(
-          "",
+          cert_name,
           GRPC_ERROR_CREATE_FROM_STATIC_STRING(
               "No certificate provider available for root certificates"),
           absl::nullopt);
     } else {
-      UpdateRootCertWatcher(root_cert_distributor_.get());
+      UpdateRootCertWatcher(cert_name, root_cert_distributor_.get());
     }
   } else if (!root_being_watched && watching_root_certs_) {
     // We need to cancel root certs watch.
@@ -225,12 +233,12 @@ void XdsCertificateProvider::WatchStatusCallback(std::string cert_name,
   if (identity_being_watched && !watching_identity_certs_) {
     watching_identity_certs_ = true;
     if (identity_cert_distributor_ == nullptr) {
-      distributor_->SetErrorForCert(
+      xds_certificate_provider_->distributor_->SetErrorForCert(
-          "", absl::nullopt,
+          cert_name, absl::nullopt,
           GRPC_ERROR_CREATE_FROM_STATIC_STRING(
               "No certificate provider available for identity certificates"));
     } else {
-      UpdateIdentityCertWatcher(identity_cert_distributor_.get());
+      UpdateIdentityCertWatcher(cert_name, identity_cert_distributor_.get());
     }
   } else if (!identity_being_watched && watching_identity_certs_) {
     watching_identity_certs_ = false;
@@ -243,20 +251,102 @@ void XdsCertificateProvider::WatchStatusCallback(std::string cert_name,
   }
 }
 
-void XdsCertificateProvider::UpdateRootCertWatcher(
+//
-    grpc_tls_certificate_distributor* root_cert_distributor) {
+// XdsCertificateProvider
-  auto watcher = absl::make_unique<RootCertificatesWatcher>(distributor());
+//
-  root_cert_watcher_ = watcher.get();
+
-  root_cert_distributor->WatchTlsCertificates(std::move(watcher),
+XdsCertificateProvider::XdsCertificateProvider()
-                                              root_cert_name_, absl::nullopt);
+    : distributor_(MakeRefCounted<grpc_tls_certificate_distributor>()) {
+  distributor_->SetWatchStatusCallback(
+      absl::bind_front(&XdsCertificateProvider::WatchStatusCallback, this));
 }
 
-void XdsCertificateProvider::UpdateIdentityCertWatcher(
+XdsCertificateProvider::~XdsCertificateProvider() {
-    grpc_tls_certificate_distributor* identity_cert_distributor) {
+  distributor_->SetWatchStatusCallback(nullptr);
-  auto watcher = absl::make_unique<IdentityCertificatesWatcher>(distributor());
+}
-  identity_cert_watcher_ = watcher.get();
+
-  identity_cert_distributor->WatchTlsCertificates(
+bool XdsCertificateProvider::ProvidesRootCerts(const std::string& cert_name) {
-      std::move(watcher), absl::nullopt, identity_cert_name_);
+  MutexLock lock(&mu_);
+  auto it = certificate_state_map_.find(cert_name);
+  if (it == certificate_state_map_.end()) return false;
+  return it->second->ProvidesRootCerts();
+}
+
+void XdsCertificateProvider::UpdateRootCertNameAndDistributor(
+    const std::string& cert_name, absl::string_view root_cert_name,
+    RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor) {
+  MutexLock lock(&mu_);
+  auto it = certificate_state_map_.find(cert_name);
+  if (it == certificate_state_map_.end()) {
+    it = certificate_state_map_
+             .emplace(cert_name,
+                      absl::make_unique<ClusterCertificateState>(Ref()))
+             .first;
+  }
+  it->second->UpdateRootCertNameAndDistributor(cert_name, root_cert_name,
+                                               root_cert_distributor);
+  // Delete unused entries.
+  if (it->second->IsSafeToRemove()) certificate_state_map_.erase(it);
+}
+
+bool XdsCertificateProvider::ProvidesIdentityCerts(
+    const std::string& cert_name) {
+  MutexLock lock(&mu_);
+  auto it = certificate_state_map_.find(cert_name);
+  if (it == certificate_state_map_.end()) return false;
+  return it->second->ProvidesIdentityCerts();
+}
+
+void XdsCertificateProvider::UpdateIdentityCertNameAndDistributor(
+    const std::string& cert_name, absl::string_view identity_cert_name,
+    RefCountedPtr<grpc_tls_certificate_distributor> identity_cert_distributor) {
+  MutexLock lock(&mu_);
+  auto it = certificate_state_map_.find(cert_name);
+  if (it == certificate_state_map_.end()) {
+    it = certificate_state_map_
+             .emplace(cert_name,
+                      absl::make_unique<ClusterCertificateState>(Ref()))
+             .first;
+  }
+  it->second->UpdateIdentityCertNameAndDistributor(
+      cert_name, identity_cert_name, identity_cert_distributor);
+  // Delete unused entries.
+  if (it->second->IsSafeToRemove()) certificate_state_map_.erase(it);
+}
+
+std::vector<XdsApi::StringMatcher> XdsCertificateProvider::GetSanMatchers(
+    const std::string& cluster) {
+  MutexLock lock(&san_matchers_mu_);
+  auto it = san_matcher_map_.find(cluster);
+  if (it == san_matcher_map_.end()) return {};
+  return it->second;
+}
+
+void XdsCertificateProvider::UpdateSubjectAlternativeNameMatchers(
+    const std::string& cluster, std::vector<XdsApi::StringMatcher> matchers) {
+  MutexLock lock(&san_matchers_mu_);
+  if (matchers.empty()) {
+    san_matcher_map_.erase(cluster);
+  } else {
+    san_matcher_map_[cluster] = std::move(matchers);
+  }
+}
+
+void XdsCertificateProvider::WatchStatusCallback(std::string cert_name,
+                                                 bool root_being_watched,
+                                                 bool identity_being_watched) {
+  MutexLock lock(&mu_);
+  auto it = certificate_state_map_.find(cert_name);
+  if (it == certificate_state_map_.end()) {
+    it = certificate_state_map_
+             .emplace(cert_name,
+                      absl::make_unique<ClusterCertificateState>(Ref()))
+             .first;
+  }
+  it->second->WatchStatusCallback(cert_name, root_being_watched,
+                                  identity_being_watched);
+  // Delete unused entries.
+  if (it->second->IsSafeToRemove()) certificate_state_map_.erase(it);
 }
 
 namespace {

src/core/ext/xds/xds_certificate_provider.h

@@ -31,44 +31,28 @@ namespace grpc_core {
 
 class XdsCertificateProvider : public grpc_tls_certificate_provider {
  public:
-  XdsCertificateProvider(
+  XdsCertificateProvider();
-      absl::string_view root_cert_name,
-      RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor,
-      absl::string_view identity_cert_name,
-      RefCountedPtr<grpc_tls_certificate_distributor> identity_cert_distributor,
-      std::vector<XdsApi::StringMatcher> san_matchers);
-
   ~XdsCertificateProvider() override;
 
-  void UpdateRootCertNameAndDistributor(
-      absl::string_view root_cert_name,
-      RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor);
-  void UpdateIdentityCertNameAndDistributor(
-      absl::string_view identity_cert_name,
-      RefCountedPtr<grpc_tls_certificate_distributor>
-          identity_cert_distributor);
-  void UpdateSubjectAlternativeNameMatchers(
-      std::vector<XdsApi::StringMatcher> matchers);
-
   grpc_core::RefCountedPtr<grpc_tls_certificate_distributor> distributor()
       const override {
     return distributor_;
   }
 
-  bool ProvidesRootCerts() {
+  bool ProvidesRootCerts(const std::string& cert_name);
-    MutexLock lock(&mu_);
+  void UpdateRootCertNameAndDistributor(
-    return root_cert_distributor_ != nullptr;
+      const std::string& cert_name, absl::string_view root_cert_name,
-  }
+      RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor);
 
-  bool ProvidesIdentityCerts() {
+  bool ProvidesIdentityCerts(const std::string& cert_name);
-    MutexLock lock(&mu_);
+  void UpdateIdentityCertNameAndDistributor(
-    return identity_cert_distributor_ != nullptr;
+      const std::string& cert_name, absl::string_view identity_cert_name,
-  }
+      RefCountedPtr<grpc_tls_certificate_distributor>
+          identity_cert_distributor);
 
-  std::vector<XdsApi::StringMatcher> subject_alternative_name_matchers() {
+  std::vector<XdsApi::StringMatcher> GetSanMatchers(const std::string& cluster);
-    MutexLock lock(&san_matchers_mu_);
+  void UpdateSubjectAlternativeNameMatchers(
-    return san_matchers_;
+      const std::string& cluster, std::vector<XdsApi::StringMatcher> matchers);
-  }
 
   grpc_arg MakeChannelArg() const;
 
@@ -76,14 +60,63 @@ class XdsCertificateProvider : public grpc_tls_certificate_provider {
       const grpc_channel_args* args);
 
  private:
+  class ClusterCertificateState {
+   public:
+    explicit ClusterCertificateState(
+        RefCountedPtr<XdsCertificateProvider> xds_certificate_provider)
+        : xds_certificate_provider_(std::move(xds_certificate_provider)) {}
+
+    ~ClusterCertificateState();
+
+    // Returns true if the certs aren't being watched and there are no
+    // distributors configured.
+    bool IsSafeToRemove() const;
+
+    bool ProvidesRootCerts() const { return root_cert_distributor_ != nullptr; }
+    bool ProvidesIdentityCerts() const {
+      return identity_cert_distributor_ != nullptr;
+    }
+
+    void UpdateRootCertNameAndDistributor(
+        const std::string& cert_name, absl::string_view root_cert_name,
+        RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor);
+    void UpdateIdentityCertNameAndDistributor(
+        const std::string& cert_name, absl::string_view identity_cert_name,
+        RefCountedPtr<grpc_tls_certificate_distributor>
+            identity_cert_distributor);
+
+    void UpdateRootCertWatcher(
+        const std::string& cert_name,
+        grpc_tls_certificate_distributor* root_cert_distributor);
+    void UpdateIdentityCertWatcher(
+        const std::string& cert_name,
+        grpc_tls_certificate_distributor* identity_cert_distributor);
+
+    void WatchStatusCallback(const std::string& cert_name,
+                             bool root_being_watched,
+                             bool identity_being_watched);
+
+   private:
+    RefCountedPtr<XdsCertificateProvider> xds_certificate_provider_;
+    bool watching_root_certs_ = false;
+    bool watching_identity_certs_ = false;
+    std::string root_cert_name_;
+    std::string identity_cert_name_;
+    RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor_;
+    RefCountedPtr<grpc_tls_certificate_distributor> identity_cert_distributor_;
+    grpc_tls_certificate_distributor::TlsCertificatesWatcherInterface*
+        root_cert_watcher_ = nullptr;
+    grpc_tls_certificate_distributor::TlsCertificatesWatcherInterface*
+        identity_cert_watcher_ = nullptr;
+  };
+
   void WatchStatusCallback(std::string cert_name, bool root_being_watched,
                            bool identity_being_watched);
-  void UpdateRootCertWatcher(
-      grpc_tls_certificate_distributor* root_cert_distributor);
-  void UpdateIdentityCertWatcher(
-      grpc_tls_certificate_distributor* identity_cert_distributor);
 
   Mutex mu_;
+  std::map<std::string /*cert_name*/, std::unique_ptr<ClusterCertificateState>>
+      certificate_state_map_;
+
   // Use a separate mutex for san_matchers_ to avoid deadlocks since
   // san_matchers_ needs to be accessed when a handshake is being done and we
   // run into a possible deadlock scenario if using the same mutex. The mutex
@@ -93,18 +126,10 @@ class XdsCertificateProvider : public grpc_tls_certificate_provider {
   // -> HandshakeManager::Add() -> SecurityHandshaker::DoHandshake() ->
   // subject_alternative_names_matchers()
   Mutex san_matchers_mu_;
-  bool watching_root_certs_ = false;
+  std::map<std::string /*cluster_name*/, std::vector<XdsApi::StringMatcher>>
-  bool watching_identity_certs_ = false;
+      san_matcher_map_;
-  std::string root_cert_name_;
+
-  std::string identity_cert_name_;
-  RefCountedPtr<grpc_tls_certificate_distributor> root_cert_distributor_;
-  RefCountedPtr<grpc_tls_certificate_distributor> identity_cert_distributor_;
-  std::vector<XdsApi::StringMatcher> san_matchers_;
   RefCountedPtr<grpc_tls_certificate_distributor> distributor_;
-  grpc_tls_certificate_distributor::TlsCertificatesWatcherInterface*
-      root_cert_watcher_ = nullptr;
-  grpc_tls_certificate_distributor::TlsCertificatesWatcherInterface*
-      identity_cert_watcher_ = nullptr;
 };
 
 }  // namespace grpc_core

src/core/lib/security/credentials/xds/xds_credentials.cc

@@ -20,6 +20,7 @@
 
 #include "src/core/lib/security/credentials/xds/xds_credentials.h"
 
+#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h"
 #include "src/core/ext/xds/xds_certificate_provider.h"
 #include "src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h"
 #include "src/core/lib/security/credentials/tls/tls_credentials.h"
@@ -60,32 +61,44 @@ bool XdsVerifySubjectAlternativeNames(
   return false;
 }
 
-int ServerAuthCheckSchedule(void* config_user_data,
+class ServerAuthCheck {
-                            grpc_tls_server_authorization_check_arg* arg) {
+ public:
-  XdsCertificateProvider* xds_certificate_provider =
+  ServerAuthCheck(
-      static_cast<XdsCertificateProvider*>(config_user_data);
+      RefCountedPtr<XdsCertificateProvider> xds_certificate_provider,
-  if (XdsVerifySubjectAlternativeNames(
+      std::string cluster_name)
-          arg->subject_alternative_names, arg->subject_alternative_names_size,
+      : xds_certificate_provider_(std::move(xds_certificate_provider)),
-          xds_certificate_provider->subject_alternative_name_matchers())) {
+        cluster_name_(std::move(cluster_name)) {}
-    arg->success = 1;
+
-    arg->status = GRPC_STATUS_OK;
+  static int Schedule(void* config_user_data,
-  } else {
+                      grpc_tls_server_authorization_check_arg* arg) {
-    arg->success = 0;
+    return static_cast<ServerAuthCheck*>(config_user_data)->ScheduleImpl(arg);
-    arg->status = GRPC_STATUS_UNAUTHENTICATED;
-    if (arg->error_details) {
-      arg->error_details->set_error_details(
-          "SANs from certificate did not match SANs from xDS control plane");
-    }
   }
 
-  return 0; /* synchronous check */
+  static void Destroy(void* config_user_data) {
-}
+    delete static_cast<ServerAuthCheck*>(config_user_data);
+  }
 
-void ServerAuthCheckDestroy(void* config_user_data) {
+ private:
-  XdsCertificateProvider* xds_certificate_provider =
+  int ScheduleImpl(grpc_tls_server_authorization_check_arg* arg) {
-      static_cast<XdsCertificateProvider*>(config_user_data);
+    if (XdsVerifySubjectAlternativeNames(
-  xds_certificate_provider->Unref();
+            arg->subject_alternative_names, arg->subject_alternative_names_size,
-}
+            xds_certificate_provider_->GetSanMatchers(cluster_name_))) {
+      arg->success = 1;
+      arg->status = GRPC_STATUS_OK;
+    } else {
+      arg->success = 0;
+      arg->status = GRPC_STATUS_UNAUTHENTICATED;
+      if (arg->error_details) {
+        arg->error_details->set_error_details(
+            "SANs from certificate did not match SANs from xDS control plane");
+      }
+    }
+    return 0; /* synchronous check */
+  }
+
+  RefCountedPtr<XdsCertificateProvider> xds_certificate_provider_;
+  std::string cluster_name_;
+};
 
 }  // namespace
 
@@ -105,49 +118,79 @@ RefCountedPtr<grpc_channel_security_connector>
 XdsCredentials::create_security_connector(
     RefCountedPtr<grpc_call_credentials> call_creds, const char* target_name,
     const grpc_channel_args* args, grpc_channel_args** new_args) {
-  auto xds_certificate_provider =
+  struct ChannelArgsDeleter {
-      XdsCertificateProvider::GetFromChannelArgs(args);
+    const grpc_channel_args* args;
+    bool owned;
+    ~ChannelArgsDeleter() {
+      if (owned) grpc_channel_args_destroy(args);
+    }
+  };
+  ChannelArgsDeleter temp_args{args, false};
   // TODO(yashykt): This arg will no longer need to be added after b/173119596
   // is fixed.
   grpc_arg override_arg = grpc_channel_arg_string_create(
       const_cast<char*>(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG),
       const_cast<char*>(target_name));
   const char* override_arg_name = GRPC_SSL_TARGET_NAME_OVERRIDE_ARG;
-  const grpc_channel_args* temp_args = args;
   if (grpc_channel_args_find(args, override_arg_name) == nullptr) {
-    temp_args = grpc_channel_args_copy_and_add_and_remove(
+    temp_args.args = grpc_channel_args_copy_and_add_and_remove(
         args, &override_arg_name, 1, &override_arg, 1);
+    temp_args.owned = true;
   }
   RefCountedPtr<grpc_channel_security_connector> security_connector;
+  auto xds_certificate_provider =
+      XdsCertificateProvider::GetFromChannelArgs(args);
   if (xds_certificate_provider != nullptr) {
-    auto tls_credentials_options =
+    std::string cluster_name =
-        MakeRefCounted<grpc_tls_credentials_options>();
+        grpc_channel_args_find_string(args, GRPC_ARG_XDS_CLUSTER_NAME);
-    tls_credentials_options->set_certificate_provider(xds_certificate_provider);
+    GPR_ASSERT(cluster_name.data() != nullptr);
-    if (xds_certificate_provider->ProvidesRootCerts()) {
+    const bool watch_root =
-      tls_credentials_options->set_watch_root_cert(true);
+        xds_certificate_provider->ProvidesRootCerts(cluster_name);
-    }
+    const bool watch_identity =
-    if (xds_certificate_provider->ProvidesIdentityCerts()) {
+        xds_certificate_provider->ProvidesIdentityCerts(cluster_name);
-      tls_credentials_options->set_watch_identity_pair(true);
+    if (watch_root || watch_identity) {
+      auto tls_credentials_options =
+          MakeRefCounted<grpc_tls_credentials_options>();
+      tls_credentials_options->set_certificate_provider(
+          xds_certificate_provider);
+      if (watch_root) {
+        tls_credentials_options->set_watch_root_cert(true);
+        tls_credentials_options->set_root_cert_name(cluster_name);
+      }
+      if (watch_identity) {
+        tls_credentials_options->set_watch_identity_pair(true);
+        tls_credentials_options->set_identity_cert_name(cluster_name);
+      }
+      tls_credentials_options->set_server_verification_option(
+          GRPC_TLS_SKIP_HOSTNAME_VERIFICATION);
+      auto* server_auth_check = new ServerAuthCheck(xds_certificate_provider,
+                                                    std::move(cluster_name));
+      tls_credentials_options->set_server_authorization_check_config(
+          MakeRefCounted<grpc_tls_server_authorization_check_config>(
+              server_auth_check, ServerAuthCheck::Schedule, nullptr,
+              ServerAuthCheck::Destroy));
+      // TODO(yashkt): Creating a new TlsCreds object each time we create a
+      // security connector means that the security connector's cmp() method
+      // returns unequal for each instance, which means that every time an LB
+      // policy updates, all the subchannels will be recreated.  This is
+      // going to lead to a lot of connection churn.  Instead, we should
+      // either (a) change the TLS security connector's cmp() method to be
+      // smarter somehow, so that it compares unequal only when the
+      // tls_credentials_options have changed, or (b) cache the TlsCreds
+      // objects in the XdsCredentials object so that we can reuse the
+      // same one when creating new security connectors, swapping out the
+      // TlsCreds object only when the tls_credentials_options change.
+      // Option (a) would probably be better, although it may require some
+      // structural changes to the security connector API.
+      auto tls_credentials =
+          MakeRefCounted<TlsCredentials>(std::move(tls_credentials_options));
+      return tls_credentials->create_security_connector(
+          std::move(call_creds), target_name, temp_args.args, new_args);
     }
-    tls_credentials_options->set_server_verification_option(
-        GRPC_TLS_SKIP_HOSTNAME_VERIFICATION);
-    tls_credentials_options->set_server_authorization_check_config(
-        MakeRefCounted<grpc_tls_server_authorization_check_config>(
-            xds_certificate_provider->Ref().release(), ServerAuthCheckSchedule,
-            nullptr, ServerAuthCheckDestroy));
-    auto tls_credentials =
-        MakeRefCounted<TlsCredentials>(std::move(tls_credentials_options));
-    security_connector = tls_credentials->create_security_connector(
-        std::move(call_creds), target_name, temp_args, new_args);
-  } else {
-    GPR_ASSERT(fallback_credentials_ != nullptr);
-    security_connector = fallback_credentials_->create_security_connector(
-        std::move(call_creds), target_name, temp_args, new_args);
-  }
-  if (temp_args != args) {
-    grpc_channel_args_destroy(temp_args);
   }
-  return security_connector;
+  GPR_ASSERT(fallback_credentials_ != nullptr);
+  return fallback_credentials_->create_security_connector(
+      std::move(call_creds), target_name, temp_args.args, new_args);
 }
 
 //

test/core/xds/xds_certificate_provider_test.cc

@@ -105,8 +105,10 @@ TEST(
       MakeRefCounted<grpc_tls_certificate_distributor>();
   auto identity_cert_distributor =
       MakeRefCounted<grpc_tls_certificate_distributor>();
-  XdsCertificateProvider provider("root", root_cert_distributor, "identity",
+  XdsCertificateProvider provider;
-                                  identity_cert_distributor, {});
+  provider.UpdateRootCertNameAndDistributor("", "root", root_cert_distributor);
+  provider.UpdateIdentityCertNameAndDistributor("", "identity",
+                                                identity_cert_distributor);
   auto* watcher = new TestCertificatesWatcher;
   provider.distributor()->WatchTlsCertificates(
       std::unique_ptr<TestCertificatesWatcher>(watcher), "", "");
@@ -174,8 +176,10 @@ TEST(XdsCertificateProviderTest,
       MakeRefCounted<grpc_tls_certificate_distributor>();
   auto identity_cert_distributor =
       MakeRefCounted<grpc_tls_certificate_distributor>();
-  XdsCertificateProvider provider("test", root_cert_distributor, "test",
+  XdsCertificateProvider provider;
-                                  identity_cert_distributor, {});
+  provider.UpdateRootCertNameAndDistributor("", "test", root_cert_distributor);
+  provider.UpdateIdentityCertNameAndDistributor("", "test",
+                                                identity_cert_distributor);
   auto* watcher = new TestCertificatesWatcher;
   provider.distributor()->WatchTlsCertificates(
       std::unique_ptr<TestCertificatesWatcher>(watcher), "", "");
@@ -242,8 +246,9 @@ TEST(XdsCertificateProviderTest,
 TEST(XdsCertificateProviderTest,
      RootCertDistributorSameAsIdentityCertDistributorDifferentCertNames) {
   auto distributor = MakeRefCounted<grpc_tls_certificate_distributor>();
-  XdsCertificateProvider provider("root", distributor, "identity", distributor,
+  XdsCertificateProvider provider;
-                                  {});
+  provider.UpdateRootCertNameAndDistributor("", "root", distributor);
+  provider.UpdateIdentityCertNameAndDistributor("", "identity", distributor);
   auto* watcher = new TestCertificatesWatcher;
   provider.distributor()->WatchTlsCertificates(
       std::unique_ptr<TestCertificatesWatcher>(watcher), "", "");
@@ -306,7 +311,9 @@ TEST(XdsCertificateProviderTest,
 TEST(XdsCertificateProviderTest,
      RootCertDistributorSameAsIdentityCertDistributorSameCertNames) {
   auto distributor = MakeRefCounted<grpc_tls_certificate_distributor>();
-  XdsCertificateProvider provider("", distributor, "", distributor, {});
+  XdsCertificateProvider provider;
+  provider.UpdateRootCertNameAndDistributor("", "", distributor);
+  provider.UpdateIdentityCertNameAndDistributor("", "", distributor);
   auto* watcher = new TestCertificatesWatcher;
   provider.distributor()->WatchTlsCertificates(
       std::unique_ptr<TestCertificatesWatcher>(watcher), "", "");
@@ -369,7 +376,7 @@ TEST(XdsCertificateProviderTest,
 TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) {
   auto distributor = MakeRefCounted<grpc_tls_certificate_distributor>();
   distributor->SetKeyMaterials("", kRootCert1, MakeKeyCertPairsType1());
-  XdsCertificateProvider provider("", nullptr, "", nullptr, {});
+  XdsCertificateProvider provider;
   auto* watcher = new TestCertificatesWatcher;
   provider.distributor()->WatchTlsCertificates(
       std::unique_ptr<TestCertificatesWatcher>(watcher), "", "");
@@ -384,7 +391,7 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) {
       ::testing::HasSubstr(
           "No certificate provider available for identity certificates"));
   // Update root cert distributor.
-  provider.UpdateRootCertNameAndDistributor("", distributor);
+  provider.UpdateRootCertNameAndDistributor("", "", distributor);
   EXPECT_EQ(watcher->root_certs(), kRootCert1);
   EXPECT_EQ(watcher->key_cert_pairs(), absl::nullopt);
   EXPECT_EQ(watcher->root_cert_error(), GRPC_ERROR_NONE);
@@ -393,7 +400,7 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) {
       ::testing::HasSubstr(
           "No certificate provider available for identity certificates"));
   // Update identity cert distributor
-  provider.UpdateIdentityCertNameAndDistributor("", distributor);
+  provider.UpdateIdentityCertNameAndDistributor("", "", distributor);
   EXPECT_EQ(watcher->root_certs(), kRootCert1);
   EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType1());
   EXPECT_EQ(watcher->root_cert_error(), GRPC_ERROR_NONE);
@@ -421,7 +428,7 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) {
   EXPECT_EQ(watcher->root_cert_error(), GRPC_ERROR_NONE);
   EXPECT_EQ(watcher->identity_cert_error(), GRPC_ERROR_NONE);
   // Remove root cert provider
-  provider.UpdateRootCertNameAndDistributor("", nullptr);
+  provider.UpdateRootCertNameAndDistributor("", "", nullptr);
   distributor->SetKeyMaterials("", kRootCert2, MakeKeyCertPairsType2());
   EXPECT_EQ(watcher->root_certs(), kRootCert1);  // not updated
   EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType2());
@@ -430,7 +437,7 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) {
                   "No certificate provider available for root certificates"));
   EXPECT_EQ(watcher->identity_cert_error(), GRPC_ERROR_NONE);
   // Remove identity cert provider too
-  provider.UpdateIdentityCertNameAndDistributor("", nullptr);
+  provider.UpdateIdentityCertNameAndDistributor("", "", nullptr);
   distributor->SetKeyMaterials("", kRootCert1, MakeKeyCertPairsType1());
   EXPECT_EQ(watcher->root_certs(), kRootCert1);
   EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType2());  // not updated
@@ -442,8 +449,8 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) {
       ::testing::HasSubstr(
           "No certificate provider available for identity certificates"));
   // Change certificate names being watched, without any certificate updates.
-  provider.UpdateRootCertNameAndDistributor("root", distributor);
+  provider.UpdateRootCertNameAndDistributor("", "root", distributor);
-  provider.UpdateIdentityCertNameAndDistributor("identity", distributor);
+  provider.UpdateIdentityCertNameAndDistributor("", "identity", distributor);
   EXPECT_EQ(watcher->root_certs(), kRootCert1);
   EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType2());
   EXPECT_THAT(grpc_error_string(watcher->root_cert_error()),
@@ -467,16 +474,16 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) {
       MakeRefCounted<grpc_tls_certificate_distributor>();
   auto identity_cert_distributor =
       MakeRefCounted<grpc_tls_certificate_distributor>();
-  provider.UpdateRootCertNameAndDistributor("root", root_cert_distributor);
+  provider.UpdateRootCertNameAndDistributor("", "root", root_cert_distributor);
-  provider.UpdateIdentityCertNameAndDistributor("identity",
+  provider.UpdateIdentityCertNameAndDistributor("", "identity",
                                                 identity_cert_distributor);
   EXPECT_EQ(watcher->root_certs(), kRootCert2);
   EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType1());
   EXPECT_EQ(watcher->root_cert_error(), GRPC_ERROR_NONE);
   EXPECT_EQ(watcher->identity_cert_error(), GRPC_ERROR_NONE);
   // Change certificate names without any certificate updates.
-  provider.UpdateRootCertNameAndDistributor("test", root_cert_distributor);
+  provider.UpdateRootCertNameAndDistributor("", "test", root_cert_distributor);
-  provider.UpdateIdentityCertNameAndDistributor("test",
+  provider.UpdateIdentityCertNameAndDistributor("", "test",
                                                 identity_cert_distributor);
   EXPECT_EQ(watcher->root_certs(), kRootCert2);
   EXPECT_EQ(watcher->key_cert_pairs(), MakeKeyCertPairsType1());
@@ -493,15 +500,70 @@ TEST(XdsCertificateProviderTest, SwapOutDistributorsMultipleTimes) {
   EXPECT_EQ(watcher->identity_cert_error(), GRPC_ERROR_NONE);
 }
 
-TEST(XdsCertificateProviderTest, CertificateNameNotEmpty) {
+TEST(XdsCertificateProviderTest, MultipleCertNames) {
-  XdsCertificateProvider provider("", nullptr, "", nullptr, {});
+  XdsCertificateProvider provider;
+  // Start watch for "test1".  There are no underlying distributors for
+  // that cert name, so it will return an error.
+  auto* watcher1 = new TestCertificatesWatcher;
+  provider.distributor()->WatchTlsCertificates(
+      std::unique_ptr<TestCertificatesWatcher>(watcher1), "test1", "test1");
+  EXPECT_EQ(watcher1->root_certs(), absl::nullopt);
+  EXPECT_EQ(watcher1->key_cert_pairs(), absl::nullopt);
+  EXPECT_THAT(grpc_error_string(watcher1->root_cert_error()),
+              ::testing::HasSubstr(
+                  "No certificate provider available for root certificates"));
+  EXPECT_THAT(
+      grpc_error_string(watcher1->identity_cert_error()),
+      ::testing::HasSubstr(
+          "No certificate provider available for identity certificates"));
+  // Add distributor for "test1".  This will return data to the watcher.
+  auto cert_distributor1 = MakeRefCounted<grpc_tls_certificate_distributor>();
+  cert_distributor1->SetKeyMaterials("root", kRootCert1, absl::nullopt);
+  cert_distributor1->SetKeyMaterials("identity", absl::nullopt,
+                                     MakeKeyCertPairsType1());
+  provider.UpdateRootCertNameAndDistributor("test1", "root", cert_distributor1);
+  provider.UpdateIdentityCertNameAndDistributor("test1", "identity",
+                                                cert_distributor1);
+  EXPECT_EQ(watcher1->root_certs(), kRootCert1);
+  EXPECT_EQ(watcher1->key_cert_pairs(), MakeKeyCertPairsType1());
+  EXPECT_EQ(watcher1->root_cert_error(), GRPC_ERROR_NONE);
+  EXPECT_EQ(watcher1->identity_cert_error(), GRPC_ERROR_NONE);
+  // Add distributor for "test2".
+  auto cert_distributor2 = MakeRefCounted<grpc_tls_certificate_distributor>();
+  cert_distributor2->SetKeyMaterials("root2", kRootCert2, absl::nullopt);
+  cert_distributor2->SetKeyMaterials("identity2", absl::nullopt,
+                                     MakeKeyCertPairsType2());
+  provider.UpdateRootCertNameAndDistributor("test2", "root2",
+                                            cert_distributor2);
+  provider.UpdateIdentityCertNameAndDistributor("test2", "identity2",
+                                                cert_distributor2);
+  // Add watcher for "test2".  This one should return data immediately.
+  auto* watcher2 = new TestCertificatesWatcher;
+  provider.distributor()->WatchTlsCertificates(
+      std::unique_ptr<TestCertificatesWatcher>(watcher2), "test2", "test2");
+  EXPECT_EQ(watcher2->root_certs(), kRootCert2);
+  EXPECT_EQ(watcher2->key_cert_pairs(), MakeKeyCertPairsType2());
+  EXPECT_EQ(watcher2->root_cert_error(), GRPC_ERROR_NONE);
+  EXPECT_EQ(watcher2->identity_cert_error(), GRPC_ERROR_NONE);
+  // The presence of "test2" should not affect "test1".
+  EXPECT_EQ(watcher1->root_certs(), kRootCert1);
+  EXPECT_EQ(watcher1->key_cert_pairs(), MakeKeyCertPairsType1());
+  EXPECT_EQ(watcher1->root_cert_error(), GRPC_ERROR_NONE);
+  EXPECT_EQ(watcher1->identity_cert_error(), GRPC_ERROR_NONE);
+}
+
+TEST(XdsCertificateProviderTest, UnknownCertName) {
+  XdsCertificateProvider provider;
   auto* watcher = new TestCertificatesWatcher;
   provider.distributor()->WatchTlsCertificates(
       std::unique_ptr<TestCertificatesWatcher>(watcher), "test", "test");
   EXPECT_THAT(grpc_error_string(watcher->root_cert_error()),
-              ::testing::HasSubstr("Illegal certificate name: \'test\'"));
+              ::testing::HasSubstr(
-  EXPECT_THAT(grpc_error_string(watcher->identity_cert_error()),
+                  "No certificate provider available for root certificates"));
-              ::testing::HasSubstr("Illegal certificate name: \'test\'"));
+  EXPECT_THAT(
+      grpc_error_string(watcher->identity_cert_error()),
+      ::testing::HasSubstr(
+          "No certificate provider available for identity certificates"));
 }
 
 }  // namespace

test/cpp/end2end/xds_end2end_test.cc

@@ -5453,7 +5453,7 @@ class XdsSecurityTest : public BasicTest {
         }
       }
     }
-    EXPECT_TRUE(num_tries < kRetryCount);
+    EXPECT_LT(num_tries, kRetryCount);
   }
 
   std::string root_cert_;