From f9e56a9467eb637bfcc50396712a7c4b80ea748d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 30 Nov 2023 11:52:35 -0800 Subject: [PATCH] [TLS certificate provider] remove pollset_set from cert provider API (#35013) We have no cert providers today that actually use iomgr polling, so this API is not actually used anywhere. And even if we wanted to add a cert provider that did use iomgr polling, I don't think it would work correctly, because we are plumbing the pollset_set linkage only for XdsCredentials, not for normal TlsCredentials. Closes #35013 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35013 from markdroth:cert_provider_remove_pollset_set 383cd02ffb6405a77006a97601bb73606e254067 PiperOrigin-RevId: 586743891 --- .../client_channel/lb_policy/xds/cds.cc | 57 +++++-------------- src/core/ext/xds/certificate_provider_store.h | 5 -- .../tls/grpc_tls_certificate_provider.h | 3 - 3 files changed, 14 insertions(+), 51 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc index 7e42338d70d..ea42f22e7c0 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc @@ -199,21 +199,18 @@ class CdsLb : public LoadBalancingPolicy { // The root of the tree is config_->cluster(). std::map watchers_; - // TODO(roth, yashkt): These are here because we need to handle - // pollset_set linkage as clusters are added or removed from the - // XdsCertificateProvider. However, in the aggregate cluster case, - // there may be multiple clusters in the same cert provider, and we're - // only tracking the cert providers for the most recent underlying - // cluster here. I think this is a bug that could cause us to starve - // the underlying cert providers of polling. However, it is not - // actually causing any problem in practice today, because (a) we have - // no cert provider impl that relies on gRPC's polling and (b) - // probably no one is actually configuring an aggregate cluster with - // different cert providers in different underlying clusters. - // Hopefully, this problem won't be an issue in practice until after - // the EventEngine migration is done, at which point the need for - // handling pollset_set linkage will go away, and these fields can - // simply be removed. + // TODO(roth, yashkt): These are here because XdsCertificateProvider + // does not store the actual underlying cert providers, it stores only + // their distributors, so we need to hold a ref to the cert providers + // here. However, in the aggregate cluster case, there may be multiple + // clusters in the same cert provider, and we're only tracking the cert + // providers for the most recent underlying cluster here. This is + // clearly a bug, and I think it will cause us to stop getting updates + // for all but one of the cert providers in the aggregate cluster + // case. Need to figure out the right way to fix this -- I don't + // think we want to store another map here, so ideally, we should just + // have XdsCertificateProvider actually hold the refs to the cert + // providers instead of just the distributors. RefCountedPtr root_certificate_provider_; RefCountedPtr identity_certificate_provider_; @@ -610,20 +607,7 @@ absl::Status CdsLb::UpdateXdsCertificateProvider( root_provider_instance_name, "\" not recognized.")); } } - if (root_certificate_provider_ != new_root_provider) { - if (root_certificate_provider_ != nullptr && - root_certificate_provider_->interested_parties() != nullptr) { - grpc_pollset_set_del_pollset_set( - interested_parties(), - root_certificate_provider_->interested_parties()); - } - if (new_root_provider != nullptr && - new_root_provider->interested_parties() != nullptr) { - grpc_pollset_set_add_pollset_set(interested_parties(), - new_root_provider->interested_parties()); - } - root_certificate_provider_ = std::move(new_root_provider); - } + root_certificate_provider_ = std::move(new_root_provider); xds_certificate_provider_->UpdateRootCertNameAndDistributor( cluster_name, root_provider_cert_name, root_certificate_provider_ == nullptr @@ -647,20 +631,7 @@ absl::Status CdsLb::UpdateXdsCertificateProvider( identity_provider_instance_name, "\" not recognized.")); } } - if (identity_certificate_provider_ != new_identity_provider) { - if (identity_certificate_provider_ != nullptr && - identity_certificate_provider_->interested_parties() != nullptr) { - grpc_pollset_set_del_pollset_set( - interested_parties(), - identity_certificate_provider_->interested_parties()); - } - if (new_identity_provider != nullptr && - new_identity_provider->interested_parties() != nullptr) { - grpc_pollset_set_add_pollset_set( - interested_parties(), new_identity_provider->interested_parties()); - } - identity_certificate_provider_ = std::move(new_identity_provider); - } + identity_certificate_provider_ = std::move(new_identity_provider); xds_certificate_provider_->UpdateIdentityCertNameAndDistributor( cluster_name, identity_provider_cert_name, identity_certificate_provider_ == nullptr diff --git a/src/core/ext/xds/certificate_provider_store.h b/src/core/ext/xds/certificate_provider_store.h index 24b172ac32d..aba287f9789 100644 --- a/src/core/ext/xds/certificate_provider_store.h +++ b/src/core/ext/xds/certificate_provider_store.h @@ -36,7 +36,6 @@ #include "src/core/lib/gprpp/sync.h" #include "src/core/lib/gprpp/unique_type_name.h" #include "src/core/lib/gprpp/validation_errors.h" -#include "src/core/lib/iomgr/iomgr_fwd.h" #include "src/core/lib/json/json.h" #include "src/core/lib/json/json_args.h" #include "src/core/lib/json/json_object_loader.h" @@ -96,10 +95,6 @@ class CertificateProviderStore return certificate_provider_->distributor(); } - grpc_pollset_set* interested_parties() const override { - return certificate_provider_->interested_parties(); - } - int CompareImpl(const grpc_tls_certificate_provider* other) const override { // TODO(yashykt): This should probably delegate to the `Compare` method of // the wrapped certificate_provider_ object. diff --git a/src/core/lib/security/credentials/tls/grpc_tls_certificate_provider.h b/src/core/lib/security/credentials/tls/grpc_tls_certificate_provider.h index 22ccf7ebd74..d9f2527a33c 100644 --- a/src/core/lib/security/credentials/tls/grpc_tls_certificate_provider.h +++ b/src/core/lib/security/credentials/tls/grpc_tls_certificate_provider.h @@ -39,7 +39,6 @@ #include "src/core/lib/gprpp/sync.h" #include "src/core/lib/gprpp/thd.h" #include "src/core/lib/gprpp/unique_type_name.h" -#include "src/core/lib/iomgr/iomgr_fwd.h" #include "src/core/lib/security/credentials/tls/grpc_tls_certificate_distributor.h" #include "src/core/lib/security/security_connector/ssl_utils.h" @@ -55,8 +54,6 @@ struct grpc_tls_certificate_provider : public grpc_core::RefCounted { public: - virtual grpc_pollset_set* interested_parties() const { return nullptr; } - virtual grpc_core::RefCountedPtr distributor() const = 0;