[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 383cd02ffb
PiperOrigin-RevId: 586743891
pull/35134/head
Mark D. Roth 1 year ago committed by Copybara-Service
parent f356ef47e6
commit f9e56a9467
  1. 53
      src/core/ext/filters/client_channel/lb_policy/xds/cds.cc
  2. 5
      src/core/ext/xds/certificate_provider_store.h
  3. 3
      src/core/lib/security/credentials/tls/grpc_tls_certificate_provider.h

@ -199,21 +199,18 @@ class CdsLb : public LoadBalancingPolicy {
// The root of the tree is config_->cluster(). // The root of the tree is config_->cluster().
std::map<std::string, WatcherState> watchers_; std::map<std::string, WatcherState> watchers_;
// TODO(roth, yashkt): These are here because we need to handle // TODO(roth, yashkt): These are here because XdsCertificateProvider
// pollset_set linkage as clusters are added or removed from the // does not store the actual underlying cert providers, it stores only
// XdsCertificateProvider. However, in the aggregate cluster case, // their distributors, so we need to hold a ref to the cert providers
// there may be multiple clusters in the same cert provider, and we're // here. However, in the aggregate cluster case, there may be multiple
// only tracking the cert providers for the most recent underlying // clusters in the same cert provider, and we're only tracking the cert
// cluster here. I think this is a bug that could cause us to starve // providers for the most recent underlying cluster here. This is
// the underlying cert providers of polling. However, it is not // clearly a bug, and I think it will cause us to stop getting updates
// actually causing any problem in practice today, because (a) we have // for all but one of the cert providers in the aggregate cluster
// no cert provider impl that relies on gRPC's polling and (b) // case. Need to figure out the right way to fix this -- I don't
// probably no one is actually configuring an aggregate cluster with // think we want to store another map here, so ideally, we should just
// different cert providers in different underlying clusters. // have XdsCertificateProvider actually hold the refs to the cert
// Hopefully, this problem won't be an issue in practice until after // providers instead of just the distributors.
// 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.
RefCountedPtr<grpc_tls_certificate_provider> root_certificate_provider_; RefCountedPtr<grpc_tls_certificate_provider> root_certificate_provider_;
RefCountedPtr<grpc_tls_certificate_provider> identity_certificate_provider_; RefCountedPtr<grpc_tls_certificate_provider> identity_certificate_provider_;
@ -610,20 +607,7 @@ absl::Status CdsLb::UpdateXdsCertificateProvider(
root_provider_instance_name, "\" not recognized.")); 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( xds_certificate_provider_->UpdateRootCertNameAndDistributor(
cluster_name, root_provider_cert_name, cluster_name, root_provider_cert_name,
root_certificate_provider_ == nullptr root_certificate_provider_ == nullptr
@ -647,20 +631,7 @@ absl::Status CdsLb::UpdateXdsCertificateProvider(
identity_provider_instance_name, "\" not recognized.")); 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( xds_certificate_provider_->UpdateIdentityCertNameAndDistributor(
cluster_name, identity_provider_cert_name, cluster_name, identity_provider_cert_name,
identity_certificate_provider_ == nullptr identity_certificate_provider_ == nullptr

@ -36,7 +36,6 @@
#include "src/core/lib/gprpp/sync.h" #include "src/core/lib/gprpp/sync.h"
#include "src/core/lib/gprpp/unique_type_name.h" #include "src/core/lib/gprpp/unique_type_name.h"
#include "src/core/lib/gprpp/validation_errors.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.h"
#include "src/core/lib/json/json_args.h" #include "src/core/lib/json/json_args.h"
#include "src/core/lib/json/json_object_loader.h" #include "src/core/lib/json/json_object_loader.h"
@ -96,10 +95,6 @@ class CertificateProviderStore
return certificate_provider_->distributor(); 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 { int CompareImpl(const grpc_tls_certificate_provider* other) const override {
// TODO(yashykt): This should probably delegate to the `Compare` method of // TODO(yashykt): This should probably delegate to the `Compare` method of
// the wrapped certificate_provider_ object. // the wrapped certificate_provider_ object.

@ -39,7 +39,6 @@
#include "src/core/lib/gprpp/sync.h" #include "src/core/lib/gprpp/sync.h"
#include "src/core/lib/gprpp/thd.h" #include "src/core/lib/gprpp/thd.h"
#include "src/core/lib/gprpp/unique_type_name.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/credentials/tls/grpc_tls_certificate_distributor.h"
#include "src/core/lib/security/security_connector/ssl_utils.h" #include "src/core/lib/security/security_connector/ssl_utils.h"
@ -55,8 +54,6 @@
struct grpc_tls_certificate_provider struct grpc_tls_certificate_provider
: public grpc_core::RefCounted<grpc_tls_certificate_provider> { : public grpc_core::RefCounted<grpc_tls_certificate_provider> {
public: public:
virtual grpc_pollset_set* interested_parties() const { return nullptr; }
virtual grpc_core::RefCountedPtr<grpc_tls_certificate_distributor> virtual grpc_core::RefCountedPtr<grpc_tls_certificate_distributor>
distributor() const = 0; distributor() const = 0;

Loading…
Cancel
Save