From 70656d08050195d6c382c5d9a82310832c603ed2 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 7 Oct 2020 17:46:24 -0700 Subject: [PATCH 01/12] Sanitize cronet_status.cc - missing new line --- src/core/ext/transport/cronet/transport/cronet_status.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_status.cc b/src/core/ext/transport/cronet/transport/cronet_status.cc index c128572528e..f904aee2c3e 100644 --- a/src/core/ext/transport/cronet/transport/cronet_status.cc +++ b/src/core/ext/transport/cronet/transport/cronet_status.cc @@ -490,4 +490,4 @@ const char* cronet_net_error_as_string(cronet_net_error_code net_error) { return "CRONET_NET_ERROR_DNS_SECURE_RESOLVER_HOSTNAME_RESOLUTION_FAILED"; } return "UNAVAILABLE."; -} \ No newline at end of file +} From 6c53881b7d2e1bd41f2de0fa5bc2cc902adeff45 Mon Sep 17 00:00:00 2001 From: Alex Merry Date: Thu, 8 Oct 2020 17:44:41 +0100 Subject: [PATCH 02/12] Fix undefined behavior when Python plugin is given no args Split() was incrementing the iterator past the end iterator when given an empty string, which is undefined behaviour, and triggered an assert when compiled with debug iterators in MSVC. --- src/compiler/python_generator_helpers.h | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/compiler/python_generator_helpers.h b/src/compiler/python_generator_helpers.h index a52e920d82a..4bdc9383d2c 100644 --- a/src/compiler/python_generator_helpers.h +++ b/src/compiler/python_generator_helpers.h @@ -138,11 +138,20 @@ StringVector get_all_comments(const DescriptorType* descriptor) { inline void Split(const std::string& s, char delim, std::vector* append_to) { - auto current = s.begin(); - while (current <= s.end()) { - auto next = std::find(current, s.end(), delim); - append_to->emplace_back(current, next); - current = next + 1; + if (s.empty()) { + // splitting an empty string logically produces a single-element list + append_to->emplace_back(); + } else { + auto current = s.begin(); + while (current < s.end()) { + const auto next = std::find(current, s.end(), delim); + append_to->emplace_back(current, next); + current = next; + if (current != s.end()) { + // it was the delimiter - need to be at the start of the next entry + ++current; + } + } } } From 15228596ed7ee9c9ac17012c7c4c0de17d473161 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 9 Oct 2020 08:14:20 -0700 Subject: [PATCH 03/12] Don't keep address attributes on the resulting subchannels. --- .../filters/client_channel/client_channel.cc | 32 ++----------------- .../lb_policy/subchannel_list.h | 6 ++-- .../filters/client_channel/server_address.h | 4 --- .../client_channel/subchannel_interface.h | 14 ++------ 4 files changed, 9 insertions(+), 47 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 9fcb045527d..c497bc4f216 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -882,9 +882,6 @@ class CallData { // ChannelData::SubchannelWrapper // -using ServerAddressAttributeMap = - std::map>; - // This class is a wrapper for Subchannel that hides details of the // channel's implementation (such as the health check service name and // connected subchannel) from the LB policy API. @@ -896,13 +893,11 @@ using ServerAddressAttributeMap = class ChannelData::SubchannelWrapper : public SubchannelInterface { public: SubchannelWrapper(ChannelData* chand, Subchannel* subchannel, - grpc_core::UniquePtr health_check_service_name, - ServerAddressAttributeMap attributes) + grpc_core::UniquePtr health_check_service_name) : SubchannelInterface(&grpc_client_channel_routing_trace), chand_(chand), subchannel_(subchannel), - health_check_service_name_(std::move(health_check_service_name)), - attributes_(std::move(attributes)) { + health_check_service_name_(std::move(health_check_service_name)) { if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) { gpr_log(GPR_INFO, "chand=%p: creating subchannel wrapper %p for subchannel %p", @@ -984,13 +979,6 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface { return subchannel_->channel_args(); } - const ServerAddress::AttributeInterface* GetAttribute( - const char* key) const override { - auto it = attributes_.find(key); - if (it == attributes_.end()) return nullptr; - return it->second.get(); - } - void ThrottleKeepaliveTime(int new_keepalive_time) { subchannel_->ThrottleKeepaliveTime(new_keepalive_time); } @@ -1188,7 +1176,6 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface { ChannelData* chand_; Subchannel* subchannel_; grpc_core::UniquePtr health_check_service_name_; - ServerAddressAttributeMap attributes_; // Maps from the address of the watcher passed to us by the LB policy // to the address of the WrapperWatcher that we passed to the underlying // subchannel. This is needed so that when the LB policy calls @@ -1363,18 +1350,6 @@ class ChannelData::ConnectivityWatcherRemover { // ChannelData::ClientChannelControlHelper // -} // namespace - -// Allows accessing the attributes from a ServerAddress. -class ChannelServerAddressPeer { - public: - static ServerAddressAttributeMap GetAttributes(ServerAddress* address) { - return std::move(address->attributes_); - } -}; - -namespace { - class ChannelData::ClientChannelControlHelper : public LoadBalancingPolicy::ChannelControlHelper { public: @@ -1426,8 +1401,7 @@ class ChannelData::ClientChannelControlHelper subchannel->ThrottleKeepaliveTime(chand_->keepalive_time_); // Create and return wrapper for the subchannel. return MakeRefCounted( - chand_, subchannel, std::move(health_check_service_name), - ChannelServerAddressPeer::GetAttributes(&address)); + chand_, subchannel, std::move(health_check_service_name)); } void UpdateState( diff --git a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h index e6d0b546f77..c1ed5e213cd 100644 --- a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h +++ b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h @@ -363,9 +363,9 @@ SubchannelList::SubchannelList( } subchannels_.reserve(addresses.size()); // Create a subchannel for each address. - for (const ServerAddress& address : addresses) { + for (ServerAddress& address : addresses) { RefCountedPtr subchannel = - helper->CreateSubchannel(std::move(address), args); + helper->CreateSubchannel(address, args); if (subchannel == nullptr) { // Subchannel could not be created. if (GRPC_TRACE_FLAG_ENABLED(*tracer_)) { @@ -383,7 +383,7 @@ SubchannelList::SubchannelList( tracer_->name(), policy_, this, subchannels_.size(), subchannel.get(), address.ToString().c_str()); } - subchannels_.emplace_back(this, address, std::move(subchannel)); + subchannels_.emplace_back(this, std::move(address), std::move(subchannel)); } } diff --git a/src/core/ext/filters/client_channel/server_address.h b/src/core/ext/filters/client_channel/server_address.h index ddcf530d3c8..7a188a0ce45 100644 --- a/src/core/ext/filters/client_channel/server_address.h +++ b/src/core/ext/filters/client_channel/server_address.h @@ -97,10 +97,6 @@ class ServerAddress { std::string ToString() const; private: - // Allows the channel to access the attributes without knowing the keys. - // (We intentionally do not allow LB policies to do this.) - friend class ChannelServerAddressPeer; - grpc_resolved_address address_; grpc_channel_args* args_; std::map> attributes_; diff --git a/src/core/ext/filters/client_channel/subchannel_interface.h b/src/core/ext/filters/client_channel/subchannel_interface.h index ccaca4ef08f..f78581d83a6 100644 --- a/src/core/ext/filters/client_channel/subchannel_interface.h +++ b/src/core/ext/filters/client_channel/subchannel_interface.h @@ -21,9 +21,10 @@ #include -#include "src/core/ext/filters/client_channel/server_address.h" +#include +#include + #include "src/core/lib/gprpp/ref_counted.h" -#include "src/core/lib/gprpp/ref_counted_ptr.h" namespace grpc_core { @@ -88,11 +89,6 @@ class SubchannelInterface : public RefCounted { // TODO(roth): Need a better non-grpc-specific abstraction here. virtual const grpc_channel_args* channel_args() = 0; - - // Allows accessing the attributes associated with the address for - // this subchannel. - virtual const ServerAddress::AttributeInterface* GetAttribute( - const char* key) const = 0; }; // A class that delegates to another subchannel, to be used in cases @@ -124,10 +120,6 @@ class DelegatingSubchannel : public SubchannelInterface { const grpc_channel_args* channel_args() override { return wrapped_subchannel_->channel_args(); } - const ServerAddress::AttributeInterface* GetAttribute( - const char* key) const override { - return wrapped_subchannel_->GetAttribute(key); - } private: RefCountedPtr wrapped_subchannel_; From 33f3e60288ce503adf94996e50f8935814669187 Mon Sep 17 00:00:00 2001 From: yulin-liang Date: Mon, 12 Oct 2020 10:26:52 -0700 Subject: [PATCH 04/12] Increasing the number of available threads for iOS --- src/core/lib/gpr/cpu_iphone.cc | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/core/lib/gpr/cpu_iphone.cc b/src/core/lib/gpr/cpu_iphone.cc index 2847e03ba58..94a724de711 100644 --- a/src/core/lib/gpr/cpu_iphone.cc +++ b/src/core/lib/gpr/cpu_iphone.cc @@ -22,8 +22,16 @@ #ifdef GPR_CPU_IPHONE -/* Probably 2 instead of 1, but see comment on gpr_cpu_current_cpu. */ -unsigned gpr_cpu_num_cores(void) { return 1; } +#include + +unsigned gpr_cpu_num_cores(void) { + size_t len; + unsigned int ncpu; + len = sizeof(ncpu); + sysctlbyname("hw.ncpu", &ncpu, &len, NULL, 0); + + return ncpu; +} /* Most code that's using this is using it to shard across work queues. So unless profiling shows it's a problem or there appears a way to detect the From a6954fe39eb7fa51820410f5b0ce3d9c5ca7139d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 12 Oct 2020 14:14:52 -0700 Subject: [PATCH 05/12] Use a single instance of the drop and locality stats objects. --- .../client_channel/lb_policy/xds/eds_drop.cc | 2 +- src/core/ext/xds/xds_client.cc | 146 +++++++++++------- src/core/ext/xds/xds_client.h | 12 +- src/core/ext/xds/xds_client_stats.cc | 41 ++++- test/cpp/end2end/xds_end2end_test.cc | 8 - 5 files changed, 135 insertions(+), 74 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/eds_drop.cc b/src/core/ext/filters/client_channel/lb_policy/xds/eds_drop.cc index e5c8f4dfaad..41af02a4499 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/eds_drop.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/eds_drop.cc @@ -207,7 +207,7 @@ EdsDropLb::EdsDropLb(RefCountedPtr xds_client, Args args) EdsDropLb::~EdsDropLb() { if (GRPC_TRACE_FLAG_ENABLED(grpc_eds_drop_lb_trace)) { - gpr_log(GPR_INFO, "[eds_drop_lb %p] destroying xds LB policy", this); + gpr_log(GPR_INFO, "[eds_drop_lb %p] destroying eds_drop LB policy", this); } } diff --git a/src/core/ext/xds/xds_client.cc b/src/core/ext/xds/xds_client.cc index 6d448641b03..ad49714a938 100644 --- a/src/core/ext/xds/xds_client.cc +++ b/src/core/ext/xds/xds_client.cc @@ -336,7 +336,7 @@ class XdsClient::ChannelState::LrsCallState void ScheduleNextReportLocked(); static void OnNextReportTimer(void* arg, grpc_error* error); bool OnNextReportTimerLocked(grpc_error* error); - void SendReportLocked(); + bool SendReportLocked(); static void OnReportDone(void* arg, grpc_error* error); bool OnReportDoneLocked(grpc_error* error); @@ -1287,8 +1287,7 @@ bool XdsClient::ChannelState::LrsCallState::Reporter::OnNextReportTimerLocked( GRPC_ERROR_UNREF(error); return true; } - SendReportLocked(); - return false; + return SendReportLocked(); } namespace { @@ -1307,7 +1306,7 @@ bool LoadReportCountersAreZero(const XdsApi::ClusterLoadReportMap& snapshot) { } // namespace -void XdsClient::ChannelState::LrsCallState::Reporter::SendReportLocked() { +bool XdsClient::ChannelState::LrsCallState::Reporter::SendReportLocked() { // Construct snapshot from all reported stats. XdsApi::ClusterLoadReportMap snapshot = xds_client()->BuildLoadReportSnapshotLocked(parent_->send_all_clusters_, @@ -1317,8 +1316,12 @@ void XdsClient::ChannelState::LrsCallState::Reporter::SendReportLocked() { const bool old_val = last_report_counters_were_zero_; last_report_counters_were_zero_ = LoadReportCountersAreZero(snapshot); if (old_val && last_report_counters_were_zero_) { + if (xds_client()->load_report_map_.empty()) { + parent_->chand()->StopLrsCall(); + return true; + } ScheduleNextReportLocked(); - return; + return false; } // Create a request that contains the snapshot. grpc_slice request_payload_slice = @@ -1339,6 +1342,7 @@ void XdsClient::ChannelState::LrsCallState::Reporter::SendReportLocked() { xds_client(), this, call_error); GPR_ASSERT(GRPC_CALL_OK == call_error); } + return false; } void XdsClient::ChannelState::LrsCallState::Reporter::OnReportDone( @@ -1982,10 +1986,22 @@ RefCountedPtr XdsClient::AddClusterDropStats( auto it = load_report_map_ .emplace(std::make_pair(std::move(key), LoadReportState())) .first; - auto cluster_drop_stats = MakeRefCounted( - Ref(DEBUG_LOCATION, "DropStats"), lrs_server, - it->first.first /*cluster_name*/, it->first.second /*eds_service_name*/); - it->second.drop_stats.insert(cluster_drop_stats.get()); + LoadReportState& load_report_state = it->second; + RefCountedPtr cluster_drop_stats; + if (load_report_state.drop_stats != nullptr) { + cluster_drop_stats = load_report_state.drop_stats->RefIfNonZero(); + } + if (cluster_drop_stats == nullptr) { + if (load_report_state.drop_stats != nullptr) { + load_report_state.deleted_drop_stats += + load_report_state.drop_stats->GetSnapshotAndReset(); + } + cluster_drop_stats = MakeRefCounted( + Ref(DEBUG_LOCATION, "DropStats"), lrs_server, + it->first.first /*cluster_name*/, + it->first.second /*eds_service_name*/); + load_report_state.drop_stats = cluster_drop_stats.get(); + } chand_->MaybeStartLrsCall(); return cluster_drop_stats; } @@ -1995,19 +2011,18 @@ void XdsClient::RemoveClusterDropStats( absl::string_view eds_service_name, XdsClusterDropStats* cluster_drop_stats) { MutexLock lock(&mu_); - auto load_report_it = load_report_map_.find( - std::make_pair(std::string(cluster_name), std::string(eds_service_name))); - if (load_report_it == load_report_map_.end()) return; - LoadReportState& load_report_state = load_report_it->second; // TODO(roth): When we add support for direct federation, use the // server name specified in lrs_server. - auto it = load_report_state.drop_stats.find(cluster_drop_stats); - if (it != load_report_state.drop_stats.end()) { - // Record final drop stats in deleted_drop_stats, which will be + auto it = load_report_map_.find( + std::make_pair(std::string(cluster_name), std::string(eds_service_name))); + if (it == load_report_map_.end()) return; + LoadReportState& load_report_state = it->second; + if (load_report_state.drop_stats == cluster_drop_stats) { + // Record final snapshot in deleted_drop_stats, which will be // added to the next load report. - auto dropped_requests = cluster_drop_stats->GetSnapshotAndReset(); - load_report_state.deleted_drop_stats += dropped_requests; - load_report_state.drop_stats.erase(it); + load_report_state.deleted_drop_stats += + load_report_state.drop_stats->GetSnapshotAndReset(); + load_report_state.drop_stats = nullptr; } } @@ -2026,12 +2041,24 @@ RefCountedPtr XdsClient::AddClusterLocalityStats( auto it = load_report_map_ .emplace(std::make_pair(std::move(key), LoadReportState())) .first; - auto cluster_locality_stats = MakeRefCounted( - Ref(DEBUG_LOCATION, "LocalityStats"), lrs_server, - it->first.first /*cluster_name*/, it->first.second /*eds_service_name*/, - locality); - it->second.locality_stats[std::move(locality)].locality_stats.insert( - cluster_locality_stats.get()); + LoadReportState& load_report_state = it->second; + LoadReportState::LocalityState& locality_state = + load_report_state.locality_stats[locality]; + RefCountedPtr cluster_locality_stats; + if (locality_state.locality_stats != nullptr) { + cluster_locality_stats = locality_state.locality_stats->RefIfNonZero(); + } + if (cluster_locality_stats == nullptr) { + if (locality_state.locality_stats != nullptr) { + locality_state.deleted_locality_stats += + locality_state.locality_stats->GetSnapshotAndReset(); + } + cluster_locality_stats = MakeRefCounted( + Ref(DEBUG_LOCATION, "LocalityStats"), lrs_server, + it->first.first /*cluster_name*/, it->first.second /*eds_service_name*/, + std::move(locality)); + locality_state.locality_stats = cluster_locality_stats.get(); + } chand_->MaybeStartLrsCall(); return cluster_locality_stats; } @@ -2042,22 +2069,21 @@ void XdsClient::RemoveClusterLocalityStats( const RefCountedPtr& locality, XdsClusterLocalityStats* cluster_locality_stats) { MutexLock lock(&mu_); - auto load_report_it = load_report_map_.find( - std::make_pair(std::string(cluster_name), std::string(eds_service_name))); - if (load_report_it == load_report_map_.end()) return; - LoadReportState& load_report_state = load_report_it->second; // TODO(roth): When we add support for direct federation, use the // server name specified in lrs_server. + auto it = load_report_map_.find( + std::make_pair(std::string(cluster_name), std::string(eds_service_name))); + if (it == load_report_map_.end()) return; + LoadReportState& load_report_state = it->second; auto locality_it = load_report_state.locality_stats.find(locality); if (locality_it == load_report_state.locality_stats.end()) return; - auto& locality_set = locality_it->second.locality_stats; - auto it = locality_set.find(cluster_locality_stats); - if (it != locality_set.end()) { + LoadReportState::LocalityState& locality_state = locality_it->second; + if (locality_state.locality_stats == cluster_locality_stats) { // Record final snapshot in deleted_locality_stats, which will be // added to the next load report. - locality_it->second.deleted_locality_stats.emplace_back( - cluster_locality_stats->GetSnapshotAndReset()); - locality_set.erase(it); + locality_state.deleted_locality_stats += + locality_state.locality_stats->GetSnapshotAndReset(); + locality_state.locality_stats = nullptr; } } @@ -2098,6 +2124,9 @@ void XdsClient::NotifyOnErrorLocked(grpc_error* error) { XdsApi::ClusterLoadReportMap XdsClient::BuildLoadReportSnapshotLocked( bool send_all_clusters, const std::set& clusters) { + if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { + gpr_log(GPR_INFO, "[xds_client %p] start building load report", this); + } XdsApi::ClusterLoadReportMap snapshot_map; for (auto load_report_it = load_report_map_.begin(); load_report_it != load_report_map_.end();) { @@ -2116,9 +2145,15 @@ XdsApi::ClusterLoadReportMap XdsClient::BuildLoadReportSnapshotLocked( XdsApi::ClusterLoadReport snapshot; // Aggregate drop stats. snapshot.dropped_requests = std::move(load_report.deleted_drop_stats); - for (auto& drop_stats : load_report.drop_stats) { - auto dropped_requests = drop_stats->GetSnapshotAndReset(); - snapshot.dropped_requests += dropped_requests; + if (load_report.drop_stats != nullptr) { + snapshot.dropped_requests += + load_report.drop_stats->GetSnapshotAndReset(); + if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { + gpr_log(GPR_INFO, + "[xds_client %p] cluster=%s eds_service_name=%s drop_stats=%p", + this, cluster_key.first.c_str(), cluster_key.second.c_str(), + load_report.drop_stats); + } } // Aggregate locality stats. for (auto it = load_report.locality_stats.begin(); @@ -2127,34 +2162,39 @@ XdsApi::ClusterLoadReportMap XdsClient::BuildLoadReportSnapshotLocked( auto& locality_state = it->second; XdsClusterLocalityStats::Snapshot& locality_snapshot = snapshot.locality_stats[locality_name]; - for (auto& locality_stats : locality_state.locality_stats) { - locality_snapshot += locality_stats->GetSnapshotAndReset(); - } - // Add final snapshots from recently deleted locality stats objects. - for (auto& deleted_locality_stats : - locality_state.deleted_locality_stats) { - locality_snapshot += deleted_locality_stats; + locality_snapshot = std::move(locality_state.deleted_locality_stats); + if (locality_state.locality_stats != nullptr) { + locality_snapshot += + locality_state.locality_stats->GetSnapshotAndReset(); + if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { + gpr_log(GPR_INFO, + "[xds_client %p] cluster=%s eds_service_name=%s " + "locality=%s locality_stats=%p", + this, cluster_key.first.c_str(), cluster_key.second.c_str(), + locality_name->AsHumanReadableString().c_str(), + locality_state.locality_stats); + } } - locality_state.deleted_locality_stats.clear(); // If the only thing left in this entry was final snapshots from // deleted locality stats objects, remove the entry. - if (locality_state.locality_stats.empty()) { + if (locality_state.locality_stats == nullptr) { it = load_report.locality_stats.erase(it); } else { ++it; } } + // Compute load report interval. + const grpc_millis now = ExecCtx::Get()->Now(); + snapshot.load_report_interval = now - load_report.last_report_time; + load_report.last_report_time = now; + // Record snapshot. if (record_stats) { - // Compute load report interval. - const grpc_millis now = ExecCtx::Get()->Now(); - snapshot.load_report_interval = now - load_report.last_report_time; - load_report.last_report_time = now; - // Record snapshot. snapshot_map[cluster_key] = std::move(snapshot); } // If the only thing left in this entry was final snapshots from // deleted stats objects, remove the entry. - if (load_report.locality_stats.empty() && load_report.drop_stats.empty()) { + if (load_report.locality_stats.empty() && + load_report.drop_stats == nullptr) { load_report_it = load_report_map_.erase(load_report_it); } else { ++load_report_it; diff --git a/src/core/ext/xds/xds_client.h b/src/core/ext/xds/xds_client.h index 3e74c2c38dc..9e222fbb536 100644 --- a/src/core/ext/xds/xds_client.h +++ b/src/core/ext/xds/xds_client.h @@ -39,7 +39,7 @@ namespace grpc_core { -extern TraceFlag xds_client_trace; +extern TraceFlag grpc_xds_client_trace; class XdsClient : public DualRefCounted { public: @@ -267,17 +267,13 @@ class XdsClient : public DualRefCounted { absl::optional update; }; - // TODO(roth): Change this to store exactly one instance of - // XdsClusterDropStats and exactly one instance of - // XdsClusterLocalityStats per locality. We can return multiple refs - // to the same object instead of registering multiple objects. struct LoadReportState { struct LocalityState { - std::set locality_stats; - std::vector deleted_locality_stats; + XdsClusterLocalityStats* locality_stats = nullptr; + XdsClusterLocalityStats::Snapshot deleted_locality_stats; }; - std::set drop_stats; + XdsClusterDropStats* drop_stats = nullptr; XdsClusterDropStats::Snapshot deleted_drop_stats; std::map, LocalityState, XdsLocalityName::Less> diff --git a/src/core/ext/xds/xds_client_stats.cc b/src/core/ext/xds/xds_client_stats.cc index ba29ec085f6..06cd95c4fc0 100644 --- a/src/core/ext/xds/xds_client_stats.cc +++ b/src/core/ext/xds/xds_client_stats.cc @@ -45,12 +45,27 @@ XdsClusterDropStats::XdsClusterDropStats(RefCountedPtr xds_client, absl::string_view lrs_server_name, absl::string_view cluster_name, absl::string_view eds_service_name) - : xds_client_(std::move(xds_client)), + : RefCounted(&grpc_xds_client_trace), + xds_client_(std::move(xds_client)), lrs_server_name_(lrs_server_name), cluster_name_(cluster_name), - eds_service_name_(eds_service_name) {} + eds_service_name_(eds_service_name) { + if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { + gpr_log(GPR_INFO, "[xds_client %p] created drop stats %p for {%s, %s, %s}", + xds_client_.get(), this, std::string(lrs_server_name_).c_str(), + std::string(cluster_name_).c_str(), + std::string(eds_service_name_).c_str()); + } +} XdsClusterDropStats::~XdsClusterDropStats() { + if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { + gpr_log(GPR_INFO, + "[xds_client %p] destroying drop stats %p for {%s, %s, %s}", + xds_client_.get(), this, std::string(lrs_server_name_).c_str(), + std::string(cluster_name_).c_str(), + std::string(eds_service_name_).c_str()); + } xds_client_->RemoveClusterDropStats(lrs_server_name_, cluster_name_, eds_service_name_, this); xds_client_.reset(DEBUG_LOCATION, "DropStats"); @@ -81,13 +96,31 @@ XdsClusterLocalityStats::XdsClusterLocalityStats( RefCountedPtr xds_client, absl::string_view lrs_server_name, absl::string_view cluster_name, absl::string_view eds_service_name, RefCountedPtr name) - : xds_client_(std::move(xds_client)), + : RefCounted(&grpc_xds_client_trace), + xds_client_(std::move(xds_client)), lrs_server_name_(lrs_server_name), cluster_name_(cluster_name), eds_service_name_(eds_service_name), - name_(std::move(name)) {} + name_(std::move(name)) { + if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { + gpr_log(GPR_INFO, + "[xds_client %p] created locality stats %p for {%s, %s, %s, %s}", + xds_client_.get(), this, std::string(lrs_server_name_).c_str(), + std::string(cluster_name_).c_str(), + std::string(eds_service_name_).c_str(), + name_->AsHumanReadableString().c_str()); + } +} XdsClusterLocalityStats::~XdsClusterLocalityStats() { + if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { + gpr_log(GPR_INFO, + "[xds_client %p] destroying locality stats %p for {%s, %s, %s, %s}", + xds_client_.get(), this, std::string(lrs_server_name_).c_str(), + std::string(cluster_name_).c_str(), + std::string(eds_service_name_).c_str(), + name_->AsHumanReadableString().c_str()); + } xds_client_->RemoveClusterLocalityStats(lrs_server_name_, cluster_name_, eds_service_name_, name_, this); xds_client_.reset(DEBUG_LOCATION, "LocalityStats"); diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index 744d7aac3fd..2108e3eaaa3 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -5347,14 +5347,6 @@ TEST_P(BalancerUpdateTest, DeadUpdate) { << balancers_[2]->ads_service()->eds_response_state().error_message; } -// The re-resolution tests are deferred because they rely on the fallback mode, -// which hasn't been supported. - -// TODO(juanlishen): Add TEST_P(BalancerUpdateTest, ReresolveDeadBackend). - -// TODO(juanlishen): Add TEST_P(UpdatesWithClientLoadReportingTest, -// ReresolveDeadBalancer) - class ClientLoadReportingTest : public XdsEnd2endTest { public: ClientLoadReportingTest() : XdsEnd2endTest(4, 1, 3) {} From 6564b3de8387f38788f436b35c5e73e997d8662d Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 12 Oct 2020 14:52:59 -0700 Subject: [PATCH 06/12] Update grpc-java to 1.32.2 in the interop_matrix --- tools/interop_matrix/client_matrix.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/interop_matrix/client_matrix.py b/tools/interop_matrix/client_matrix.py index 876cbfb97d8..2694e4f1c1f 100644 --- a/tools/interop_matrix/client_matrix.py +++ b/tools/interop_matrix/client_matrix.py @@ -224,7 +224,7 @@ LANG_RELEASE_MATRIX = { ('v1.29.0', ReleaseInfo()), ('v1.30.2', ReleaseInfo()), ('v1.31.1', ReleaseInfo()), - ('v1.32.1', ReleaseInfo()), + ('v1.32.2', ReleaseInfo()), ]), 'python': OrderedDict([ From 2077c3696dabeb20da003ad95126a95932d5091f Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Mon, 12 Oct 2020 16:05:52 -0700 Subject: [PATCH 07/12] Update transport socket name to envoy.transport_sockets.tls --- src/core/ext/xds/xds_api.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index 612a7ac5ecc..6233391cd0f 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -1971,7 +1971,7 @@ grpc_error* CdsResponseParse( if (transport_socket != nullptr) { absl::string_view name = UpbStringToAbsl( envoy_config_core_v3_TransportSocket_name(transport_socket)); - if (name == "tls") { + if (name == "envoy.transport_sockets.tls") { auto* typed_config = envoy_config_core_v3_TransportSocket_typed_config(transport_socket); if (typed_config != nullptr) { From 0cf672d42e93144cda884b57830e4232d1385311 Mon Sep 17 00:00:00 2001 From: Chuan Ren Date: Mon, 21 Sep 2020 15:34:38 -0700 Subject: [PATCH 08/12] Add implementation of base external account credentials --- BUILD | 2 + BUILD.gn | 2 + CMakeLists.txt | 1 + Makefile | 2 + build_autogenerated.yaml | 2 + config.m4 | 2 + config.w32 | 2 + gRPC-C++.podspec | 2 + gRPC-Core.podspec | 3 + grpc.gemspec | 2 + grpc.gyp | 1 + package.xml | 2 + .../external/external_account_credentials.cc | 311 ++++++++++++++++++ .../external/external_account_credentials.h | 119 +++++++ src/core/lib/security/util/json_util.h | 1 + src/python/grpcio/grpc_core_dependencies.py | 1 + test/core/security/credentials_test.cc | 307 +++++++++++++++++ tools/doxygen/Doxyfile.c++.internal | 2 + tools/doxygen/Doxyfile.core.internal | 2 + 19 files changed, 766 insertions(+) create mode 100644 src/core/lib/security/credentials/external/external_account_credentials.cc create mode 100644 src/core/lib/security/credentials/external/external_account_credentials.h diff --git a/BUILD b/BUILD index 5ec511c504e..22421b0ed02 100644 --- a/BUILD +++ b/BUILD @@ -1777,6 +1777,7 @@ grpc_cc_library( "src/core/lib/security/credentials/composite/composite_credentials.cc", "src/core/lib/security/credentials/credentials.cc", "src/core/lib/security/credentials/credentials_metadata.cc", + "src/core/lib/security/credentials/external/external_account_credentials.cc", "src/core/lib/security/credentials/fake/fake_credentials.cc", "src/core/lib/security/credentials/google_default/credentials_generic.cc", "src/core/lib/security/credentials/google_default/google_default_credentials.cc", @@ -1815,6 +1816,7 @@ grpc_cc_library( "src/core/lib/security/credentials/alts/alts_credentials.h", "src/core/lib/security/credentials/composite/composite_credentials.h", "src/core/lib/security/credentials/credentials.h", + "src/core/lib/security/credentials/external/external_account_credentials.h", "src/core/lib/security/credentials/fake/fake_credentials.h", "src/core/lib/security/credentials/google_default/google_default_credentials.h", "src/core/lib/security/credentials/iam/iam_credentials.h", diff --git a/BUILD.gn b/BUILD.gn index 56d77fc6700..88f738f0319 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -812,6 +812,8 @@ config("grpc_config") { "src/core/lib/security/credentials/credentials.cc", "src/core/lib/security/credentials/credentials.h", "src/core/lib/security/credentials/credentials_metadata.cc", + "src/core/lib/security/credentials/external/external_account_credentials.cc", + "src/core/lib/security/credentials/external/external_account_credentials.h", "src/core/lib/security/credentials/fake/fake_credentials.cc", "src/core/lib/security/credentials/fake/fake_credentials.h", "src/core/lib/security/credentials/google_default/credentials_generic.cc", diff --git a/CMakeLists.txt b/CMakeLists.txt index 3b63d02f1e3..63aaeaa1a4c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1735,6 +1735,7 @@ add_library(grpc src/core/lib/security/credentials/composite/composite_credentials.cc src/core/lib/security/credentials/credentials.cc src/core/lib/security/credentials/credentials_metadata.cc + src/core/lib/security/credentials/external/external_account_credentials.cc src/core/lib/security/credentials/fake/fake_credentials.cc src/core/lib/security/credentials/google_default/credentials_generic.cc src/core/lib/security/credentials/google_default/google_default_credentials.cc diff --git a/Makefile b/Makefile index afeb11e1f3a..76d8d915fde 100644 --- a/Makefile +++ b/Makefile @@ -2338,6 +2338,7 @@ LIBGRPC_SRC = \ src/core/lib/security/credentials/composite/composite_credentials.cc \ src/core/lib/security/credentials/credentials.cc \ src/core/lib/security/credentials/credentials_metadata.cc \ + src/core/lib/security/credentials/external/external_account_credentials.cc \ src/core/lib/security/credentials/fake/fake_credentials.cc \ src/core/lib/security/credentials/google_default/credentials_generic.cc \ src/core/lib/security/credentials/google_default/google_default_credentials.cc \ @@ -4804,6 +4805,7 @@ src/core/lib/security/credentials/alts/grpc_alts_credentials_server_options.cc: src/core/lib/security/credentials/composite/composite_credentials.cc: $(OPENSSL_DEP) src/core/lib/security/credentials/credentials.cc: $(OPENSSL_DEP) src/core/lib/security/credentials/credentials_metadata.cc: $(OPENSSL_DEP) +src/core/lib/security/credentials/external/external_account_credentials.cc: $(OPENSSL_DEP) src/core/lib/security/credentials/fake/fake_credentials.cc: $(OPENSSL_DEP) src/core/lib/security/credentials/google_default/credentials_generic.cc: $(OPENSSL_DEP) src/core/lib/security/credentials/google_default/google_default_credentials.cc: $(OPENSSL_DEP) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index f62b8017c15..62d5aa22d5d 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -661,6 +661,7 @@ libs: - src/core/lib/security/credentials/alts/grpc_alts_credentials_options.h - src/core/lib/security/credentials/composite/composite_credentials.h - src/core/lib/security/credentials/credentials.h + - src/core/lib/security/credentials/external/external_account_credentials.h - src/core/lib/security/credentials/fake/fake_credentials.h - src/core/lib/security/credentials/google_default/google_default_credentials.h - src/core/lib/security/credentials/iam/iam_credentials.h @@ -1077,6 +1078,7 @@ libs: - src/core/lib/security/credentials/composite/composite_credentials.cc - src/core/lib/security/credentials/credentials.cc - src/core/lib/security/credentials/credentials_metadata.cc + - src/core/lib/security/credentials/external/external_account_credentials.cc - src/core/lib/security/credentials/fake/fake_credentials.cc - src/core/lib/security/credentials/google_default/credentials_generic.cc - src/core/lib/security/credentials/google_default/google_default_credentials.cc diff --git a/config.m4 b/config.m4 index 3e74b3090af..d4c86f727a3 100644 --- a/config.m4 +++ b/config.m4 @@ -407,6 +407,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/security/credentials/composite/composite_credentials.cc \ src/core/lib/security/credentials/credentials.cc \ src/core/lib/security/credentials/credentials_metadata.cc \ + src/core/lib/security/credentials/external/external_account_credentials.cc \ src/core/lib/security/credentials/fake/fake_credentials.cc \ src/core/lib/security/credentials/google_default/credentials_generic.cc \ src/core/lib/security/credentials/google_default/google_default_credentials.cc \ @@ -979,6 +980,7 @@ if test "$PHP_GRPC" != "no"; then PHP_ADD_BUILD_DIR($ext_builddir/src/core/lib/security/credentials) PHP_ADD_BUILD_DIR($ext_builddir/src/core/lib/security/credentials/alts) PHP_ADD_BUILD_DIR($ext_builddir/src/core/lib/security/credentials/composite) + PHP_ADD_BUILD_DIR($ext_builddir/src/core/lib/security/credentials/external) PHP_ADD_BUILD_DIR($ext_builddir/src/core/lib/security/credentials/fake) PHP_ADD_BUILD_DIR($ext_builddir/src/core/lib/security/credentials/google_default) PHP_ADD_BUILD_DIR($ext_builddir/src/core/lib/security/credentials/iam) diff --git a/config.w32 b/config.w32 index 98ab01a799c..c6798364404 100644 --- a/config.w32 +++ b/config.w32 @@ -374,6 +374,7 @@ if (PHP_GRPC != "no") { "src\\core\\lib\\security\\credentials\\composite\\composite_credentials.cc " + "src\\core\\lib\\security\\credentials\\credentials.cc " + "src\\core\\lib\\security\\credentials\\credentials_metadata.cc " + + "src\\core\\lib\\security\\credentials\\external\\external_account_credentials.cc " + "src\\core\\lib\\security\\credentials\\fake\\fake_credentials.cc " + "src\\core\\lib\\security\\credentials\\google_default\\credentials_generic.cc " + "src\\core\\lib\\security\\credentials\\google_default\\google_default_credentials.cc " + @@ -1021,6 +1022,7 @@ if (PHP_GRPC != "no") { FSO.CreateFolder(base_dir+"\\ext\\grpc\\src\\core\\lib\\security\\credentials"); FSO.CreateFolder(base_dir+"\\ext\\grpc\\src\\core\\lib\\security\\credentials\\alts"); FSO.CreateFolder(base_dir+"\\ext\\grpc\\src\\core\\lib\\security\\credentials\\composite"); + FSO.CreateFolder(base_dir+"\\ext\\grpc\\src\\core\\lib\\security\\credentials\\external"); FSO.CreateFolder(base_dir+"\\ext\\grpc\\src\\core\\lib\\security\\credentials\\fake"); FSO.CreateFolder(base_dir+"\\ext\\grpc\\src\\core\\lib\\security\\credentials\\google_default"); FSO.CreateFolder(base_dir+"\\ext\\grpc\\src\\core\\lib\\security\\credentials\\iam"); diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 37efd951d34..291219934fd 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -524,6 +524,7 @@ Pod::Spec.new do |s| 'src/core/lib/security/credentials/alts/grpc_alts_credentials_options.h', 'src/core/lib/security/credentials/composite/composite_credentials.h', 'src/core/lib/security/credentials/credentials.h', + 'src/core/lib/security/credentials/external/external_account_credentials.h', 'src/core/lib/security/credentials/fake/fake_credentials.h', 'src/core/lib/security/credentials/google_default/google_default_credentials.h', 'src/core/lib/security/credentials/iam/iam_credentials.h', @@ -1018,6 +1019,7 @@ Pod::Spec.new do |s| 'src/core/lib/security/credentials/alts/grpc_alts_credentials_options.h', 'src/core/lib/security/credentials/composite/composite_credentials.h', 'src/core/lib/security/credentials/credentials.h', + 'src/core/lib/security/credentials/external/external_account_credentials.h', 'src/core/lib/security/credentials/fake/fake_credentials.h', 'src/core/lib/security/credentials/google_default/google_default_credentials.h', 'src/core/lib/security/credentials/iam/iam_credentials.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index bd418e8f858..abde8269a40 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -867,6 +867,8 @@ Pod::Spec.new do |s| 'src/core/lib/security/credentials/credentials.cc', 'src/core/lib/security/credentials/credentials.h', 'src/core/lib/security/credentials/credentials_metadata.cc', + 'src/core/lib/security/credentials/external/external_account_credentials.cc', + 'src/core/lib/security/credentials/external/external_account_credentials.h', 'src/core/lib/security/credentials/fake/fake_credentials.cc', 'src/core/lib/security/credentials/fake/fake_credentials.h', 'src/core/lib/security/credentials/google_default/credentials_generic.cc', @@ -1429,6 +1431,7 @@ Pod::Spec.new do |s| 'src/core/lib/security/credentials/alts/grpc_alts_credentials_options.h', 'src/core/lib/security/credentials/composite/composite_credentials.h', 'src/core/lib/security/credentials/credentials.h', + 'src/core/lib/security/credentials/external/external_account_credentials.h', 'src/core/lib/security/credentials/fake/fake_credentials.h', 'src/core/lib/security/credentials/google_default/google_default_credentials.h', 'src/core/lib/security/credentials/iam/iam_credentials.h', diff --git a/grpc.gemspec b/grpc.gemspec index 7d434c02d98..fd1e52db547 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -785,6 +785,8 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/security/credentials/credentials.cc ) s.files += %w( src/core/lib/security/credentials/credentials.h ) s.files += %w( src/core/lib/security/credentials/credentials_metadata.cc ) + s.files += %w( src/core/lib/security/credentials/external/external_account_credentials.cc ) + s.files += %w( src/core/lib/security/credentials/external/external_account_credentials.h ) s.files += %w( src/core/lib/security/credentials/fake/fake_credentials.cc ) s.files += %w( src/core/lib/security/credentials/fake/fake_credentials.h ) s.files += %w( src/core/lib/security/credentials/google_default/credentials_generic.cc ) diff --git a/grpc.gyp b/grpc.gyp index 56e41d761f1..896590a2091 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -771,6 +771,7 @@ 'src/core/lib/security/credentials/composite/composite_credentials.cc', 'src/core/lib/security/credentials/credentials.cc', 'src/core/lib/security/credentials/credentials_metadata.cc', + 'src/core/lib/security/credentials/external/external_account_credentials.cc', 'src/core/lib/security/credentials/fake/fake_credentials.cc', 'src/core/lib/security/credentials/google_default/credentials_generic.cc', 'src/core/lib/security/credentials/google_default/google_default_credentials.cc', diff --git a/package.xml b/package.xml index ed80ad00315..714925a5f61 100644 --- a/package.xml +++ b/package.xml @@ -765,6 +765,8 @@ + + diff --git a/src/core/lib/security/credentials/external/external_account_credentials.cc b/src/core/lib/security/credentials/external/external_account_credentials.cc new file mode 100644 index 00000000000..599b718655f --- /dev/null +++ b/src/core/lib/security/credentials/external/external_account_credentials.cc @@ -0,0 +1,311 @@ +// +// Copyright 2020 gRPC authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#include + +#include "src/core/lib/security/credentials/external/external_account_credentials.h" + +#include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" +#include "absl/time/clock.h" +#include "absl/time/time.h" + +#include "src/core/lib/http/parser.h" +#include "src/core/lib/security/util/json_util.h" +#include "src/core/lib/slice/b64.h" + +#define EXTERNAL_ACCOUNT_CREDENTIALS_GRANT_TYPE \ + "urn:ietf:params:oauth:grant-type:token-exchange" +#define EXTERNAL_ACCOUNT_CREDENTIALS_REQUESTED_TOKEN_TYPE \ + "urn:ietf:params:oauth:token-type:access_token" +#define GOOGLE_CLOUD_PLATFORM_DEFAULT_SCOPE \ + "https://www.googleapis.com/auth/cloud-platform" + +namespace grpc_core { + +ExternalAccountCredentials::ExternalAccountCredentials( + ExternalAccountCredentialsOptions options, std::vector scopes) + : options_(std::move(options)) { + if (scopes.empty()) { + scopes.push_back(GOOGLE_CLOUD_PLATFORM_DEFAULT_SCOPE); + } + scopes_ = std::move(scopes); +} + +ExternalAccountCredentials::~ExternalAccountCredentials() {} + +std::string ExternalAccountCredentials::debug_string() { + return absl::StrFormat("ExternalAccountCredentials{Audience:%s,%s}", + options_.audience, + grpc_oauth2_token_fetcher_credentials::debug_string()); +} + +// The token fetching flow: +// 1. Retrieve subject token - Subclass's RetrieveSubjectToken() gets called +// and the subject token is received in OnRetrieveSubjectTokenInternal(). +// 2. Exchange token - ExchangeToken() gets called with the +// subject token from #1. Receive the response in OnExchangeTokenInternal(). +// 3. (Optional) Impersonate service account - ImpersenateServiceAccount() gets +// called with the access token of the response from #2. Get an impersonated +// access token in OnImpersenateServiceAccountInternal(). +// 4. Finish token fetch - Return back the response that contains an access +// token in FinishTokenFetch(). +// TODO(chuanr): Avoid starting the remaining requests if the channel gets shut +// down. +void ExternalAccountCredentials::fetch_oauth2( + grpc_credentials_metadata_request* metadata_req, + grpc_httpcli_context* httpcli_context, grpc_polling_entity* pollent, + grpc_iomgr_cb_func response_cb, grpc_millis deadline) { + GPR_ASSERT(ctx_ == nullptr); + ctx_ = new HTTPRequestContext(httpcli_context, pollent, deadline); + metadata_req_ = metadata_req; + response_cb_ = response_cb; + auto cb = [this](std::string token, grpc_error* error) { + OnRetrieveSubjectTokenInternal(token, error); + }; + RetrieveSubjectToken(ctx_, options_, cb); +} + +void ExternalAccountCredentials::OnRetrieveSubjectTokenInternal( + absl::string_view subject_token, grpc_error* error) { + if (error != GRPC_ERROR_NONE) { + FinishTokenFetch(error); + } else { + ExchangeToken(subject_token); + } +} + +void ExternalAccountCredentials::ExchangeToken( + absl::string_view subject_token) { + grpc_uri* uri = grpc_uri_parse(options_.token_url, false); + if (uri == nullptr) { + FinishTokenFetch(GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrFormat("Invalid token url: %s.", options_.token_url).c_str())); + return; + } + grpc_httpcli_request request; + memset(&request, 0, sizeof(grpc_httpcli_request)); + request.host = const_cast(uri->authority); + request.http.path = gpr_strdup(uri->path); + grpc_http_header* headers = nullptr; + if (!options_.client_id.empty() && !options_.client_secret.empty()) { + request.http.hdr_count = 2; + headers = static_cast( + gpr_malloc(sizeof(grpc_http_header) * request.http.hdr_count)); + headers[0].key = gpr_strdup("Content-Type"); + headers[0].value = gpr_strdup("application/x-www-form-urlencoded"); + std::string raw_cred = + absl::StrFormat("%s:%s", options_.client_id, options_.client_secret); + char* encoded_cred = + grpc_base64_encode(raw_cred.c_str(), raw_cred.length(), 0, 0); + std::string str = absl::StrFormat("Basic %s", std::string(encoded_cred)); + headers[1].key = gpr_strdup("Authorization"); + headers[1].value = gpr_strdup(str.c_str()); + gpr_free(encoded_cred); + } else { + request.http.hdr_count = 1; + headers = static_cast( + gpr_malloc(sizeof(grpc_http_header) * request.http.hdr_count)); + headers[0].key = gpr_strdup("Content-Type"); + headers[0].value = gpr_strdup("application/x-www-form-urlencoded"); + } + request.http.hdrs = headers; + request.handshaker = (strcmp(uri->scheme, "https") == 0) + ? &grpc_httpcli_ssl + : &grpc_httpcli_plaintext; + std::vector body_parts; + body_parts.push_back(absl::StrFormat("%s=%s", "audience", options_.audience)); + body_parts.push_back(absl::StrFormat( + "%s=%s", "grant_type", EXTERNAL_ACCOUNT_CREDENTIALS_GRANT_TYPE)); + body_parts.push_back( + absl::StrFormat("%s=%s", "requested_token_type", + EXTERNAL_ACCOUNT_CREDENTIALS_REQUESTED_TOKEN_TYPE)); + body_parts.push_back(absl::StrFormat("%s=%s", "subject_token_type", + options_.subject_token_type)); + body_parts.push_back( + absl::StrFormat("%s=%s", "subject_token", subject_token)); + std::string scope = GOOGLE_CLOUD_PLATFORM_DEFAULT_SCOPE; + if (options_.service_account_impersonation_url.empty()) { + scope = absl::StrJoin(scopes_, " "); + } + body_parts.push_back(absl::StrFormat("%s=%s", "scope", scope)); + std::string body = absl::StrJoin(body_parts, "&"); + grpc_resource_quota* resource_quota = + grpc_resource_quota_create("external_account_credentials"); + grpc_http_response_destroy(&ctx_->response); + ctx_->response = {}; + GRPC_CLOSURE_INIT(&ctx_->closure, OnExchangeToken, this, nullptr); + grpc_httpcli_post(ctx_->httpcli_context, ctx_->pollent, resource_quota, + &request, body.c_str(), body.size(), ctx_->deadline, + &ctx_->closure, &ctx_->response); + grpc_resource_quota_unref_internal(resource_quota); + grpc_http_request_destroy(&request.http); + grpc_uri_destroy(uri); +} + +void ExternalAccountCredentials::OnExchangeToken(void* arg, grpc_error* error) { + ExternalAccountCredentials* self = + static_cast(arg); + self->OnExchangeTokenInternal(GRPC_ERROR_REF(error)); +} + +void ExternalAccountCredentials::OnExchangeTokenInternal(grpc_error* error) { + if (error != GRPC_ERROR_NONE) { + FinishTokenFetch(error); + } else { + if (options_.service_account_impersonation_url.empty()) { + metadata_req_->response = ctx_->response; + metadata_req_->response.body = gpr_strdup(ctx_->response.body); + FinishTokenFetch(GRPC_ERROR_NONE); + } else { + ImpersenateServiceAccount(); + } + } +} + +void ExternalAccountCredentials::ImpersenateServiceAccount() { + grpc_error* error = GRPC_ERROR_NONE; + absl::string_view response_body(ctx_->response.body, + ctx_->response.body_length); + Json json = Json::Parse(response_body, &error); + if (error != GRPC_ERROR_NONE || json.type() != Json::Type::OBJECT) { + FinishTokenFetch(GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + "Invalid token exchange response.", &error, 1)); + GRPC_ERROR_UNREF(error); + return; + } + auto it = json.object_value().find("access_token"); + if (it == json.object_value().end() || + it->second.type() != Json::Type::STRING) { + FinishTokenFetch(GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrFormat("Missing or invalid access_token in %s.", response_body) + .c_str())); + return; + } + std::string access_token = it->second.string_value(); + grpc_uri* uri = + grpc_uri_parse(options_.service_account_impersonation_url, false); + if (uri == nullptr) { + FinishTokenFetch(GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrFormat("Invalid service account impersonation url: %s.", + options_.service_account_impersonation_url) + .c_str())); + return; + } + grpc_httpcli_request request; + memset(&request, 0, sizeof(grpc_httpcli_request)); + request.host = const_cast(uri->authority); + request.http.path = gpr_strdup(uri->path); + request.http.hdr_count = 2; + grpc_http_header* headers = static_cast( + gpr_malloc(sizeof(grpc_http_header) * request.http.hdr_count)); + headers[0].key = gpr_strdup("Content-Type"); + headers[0].value = gpr_strdup("application/x-www-form-urlencoded"); + std::string str = absl::StrFormat("Bearer %s", access_token); + headers[1].key = gpr_strdup("Authorization"); + headers[1].value = gpr_strdup(str.c_str()); + request.http.hdrs = headers; + request.handshaker = (strcmp(uri->scheme, "https") == 0) + ? &grpc_httpcli_ssl + : &grpc_httpcli_plaintext; + std::string scope = absl::StrJoin(scopes_, " "); + std::string body = absl::StrFormat("%s=%s", "scope", scope); + grpc_resource_quota* resource_quota = + grpc_resource_quota_create("external_account_credentials"); + grpc_http_response_destroy(&ctx_->response); + ctx_->response = {}; + GRPC_CLOSURE_INIT(&ctx_->closure, OnImpersenateServiceAccount, this, nullptr); + grpc_httpcli_post(ctx_->httpcli_context, ctx_->pollent, resource_quota, + &request, body.c_str(), body.size(), ctx_->deadline, + &ctx_->closure, &ctx_->response); + grpc_resource_quota_unref_internal(resource_quota); + grpc_http_request_destroy(&request.http); + grpc_uri_destroy(uri); +} + +void ExternalAccountCredentials::OnImpersenateServiceAccount( + void* arg, grpc_error* error) { + ExternalAccountCredentials* self = + static_cast(arg); + self->OnImpersenateServiceAccountInternal(GRPC_ERROR_REF(error)); +} + +void ExternalAccountCredentials::OnImpersenateServiceAccountInternal( + grpc_error* error) { + if (error != GRPC_ERROR_NONE) { + FinishTokenFetch(error); + return; + } + absl::string_view response_body(ctx_->response.body, + ctx_->response.body_length); + Json json = Json::Parse(response_body, &error); + if (error != GRPC_ERROR_NONE || json.type() != Json::Type::OBJECT) { + FinishTokenFetch(GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + "Invalid service account impersonation response.", &error, 1)); + GRPC_ERROR_UNREF(error); + return; + } + auto it = json.object_value().find("accessToken"); + if (it == json.object_value().end() || + it->second.type() != Json::Type::STRING) { + FinishTokenFetch(GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrFormat("Missing or invalid accessToken in %s.", response_body) + .c_str())); + return; + } + std::string access_token = it->second.string_value(); + it = json.object_value().find("expireTime"); + if (it == json.object_value().end() || + it->second.type() != Json::Type::STRING) { + FinishTokenFetch(GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrFormat("Missing or invalid expireTime in %s.", response_body) + .c_str())); + return; + } + std::string expire_time = it->second.string_value(); + absl::Time t; + if (!absl::ParseTime(absl::RFC3339_full, expire_time, &t, nullptr)) { + FinishTokenFetch(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "Invalid expire time of service account impersonation response.")); + return; + } + int expire_in = (t - absl::Now()) / absl::Seconds(1); + std::string body = absl::StrFormat( + "{\"access_token\":\"%s\",\"expires_in\":%d,\"token_type\":\"Bearer\"}", + access_token, expire_in); + metadata_req_->response = ctx_->response; + metadata_req_->response.body = gpr_strdup(body.c_str()); + metadata_req_->response.body_length = body.length(); + FinishTokenFetch(GRPC_ERROR_NONE); +} + +void ExternalAccountCredentials::FinishTokenFetch(grpc_error* error) { + GRPC_LOG_IF_ERROR("Fetch external account credentials access token", + GRPC_ERROR_REF(error)); + // Move object state into local variables. + auto* cb = response_cb_; + response_cb_ = nullptr; + auto* metadata_req = metadata_req_; + metadata_req_ = nullptr; + auto* ctx = ctx_; + ctx_ = nullptr; + // Invoke the callback. + cb(metadata_req, error); + // Delete context. + delete ctx; + GRPC_ERROR_UNREF(error); +} + +} // namespace grpc_core diff --git a/src/core/lib/security/credentials/external/external_account_credentials.h b/src/core/lib/security/credentials/external/external_account_credentials.h new file mode 100644 index 00000000000..96a02232739 --- /dev/null +++ b/src/core/lib/security/credentials/external/external_account_credentials.h @@ -0,0 +1,119 @@ +// +// Copyright 2020 gRPC authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +#ifndef GRPC_CORE_LIB_SECURITY_CREDENTIALS_EXTERNAL_EXTERNAL_ACCOUNT_CREDENTIALS_H +#define GRPC_CORE_LIB_SECURITY_CREDENTIALS_EXTERNAL_EXTERNAL_ACCOUNT_CREDENTIALS_H + +#include + +#include +#include + +#include "src/core/lib/json/json.h" +#include "src/core/lib/security/credentials/oauth2/oauth2_credentials.h" + +namespace grpc_core { + +// Base external account credentials. The base class implements common logic for +// exchanging external account credentials for GCP access token to authorize +// requests to GCP APIs. The specific logic of retrieving subject token is +// implemented in subclasses. +class ExternalAccountCredentials + : public grpc_oauth2_token_fetcher_credentials { + public: + // External account credentials json interface. + struct ExternalAccountCredentialsOptions { + std::string type; + std::string audience; + std::string subject_token_type; + std::string service_account_impersonation_url; + std::string token_url; + std::string token_info_url; + Json credential_source; + std::string quota_project_id; + std::string client_id; + std::string client_secret; + }; + + ExternalAccountCredentials(ExternalAccountCredentialsOptions options, + std::vector scopes); + ~ExternalAccountCredentials() override; + std::string debug_string() override; + + protected: + // This is a helper struct to pass information between multiple callback based + // asynchronous calls. + struct HTTPRequestContext { + HTTPRequestContext(grpc_httpcli_context* httpcli_context, + grpc_polling_entity* pollent, grpc_millis deadline) + : httpcli_context(httpcli_context), + pollent(pollent), + deadline(deadline) {} + ~HTTPRequestContext() { grpc_http_response_destroy(&response); } + + // Contextual parameters passed from + // grpc_oauth2_token_fetcher_credentials::fetch_oauth2(). + grpc_httpcli_context* httpcli_context; + grpc_polling_entity* pollent; + grpc_millis deadline; + + // Reusable token fetch http response and closure. + grpc_closure closure; + grpc_http_response response; + }; + + // Subclasses of base external account credentials need to override this + // method to implement the specific subject token retrieval logic. + // Once the subject token is ready, subclasses need to invoke + // the callback function (cb) to pass the subject token (or error) + // back. + virtual void RetrieveSubjectToken( + const HTTPRequestContext* ctx, + const ExternalAccountCredentialsOptions& options, + std::function cb) = 0; + + private: + // This method implements the common token fetch logic and it will be called + // when grpc_oauth2_token_fetcher_credentials request a new access token. + void fetch_oauth2(grpc_credentials_metadata_request* req, + grpc_httpcli_context* httpcli_context, + grpc_polling_entity* pollent, grpc_iomgr_cb_func cb, + grpc_millis deadline) override; + + void OnRetrieveSubjectTokenInternal(absl::string_view subject_token, + grpc_error* error); + + void ExchangeToken(absl::string_view subject_token); + static void OnExchangeToken(void* arg, grpc_error* error); + void OnExchangeTokenInternal(grpc_error* error); + + void ImpersenateServiceAccount(); + static void OnImpersenateServiceAccount(void* arg, grpc_error* error); + void OnImpersenateServiceAccountInternal(grpc_error* error); + + void FinishTokenFetch(grpc_error* error); + + ExternalAccountCredentialsOptions options_; + std::vector scopes_; + + HTTPRequestContext* ctx_ = nullptr; + grpc_credentials_metadata_request* metadata_req_ = nullptr; + grpc_iomgr_cb_func response_cb_ = nullptr; +}; + +} // namespace grpc_core + +#endif // GRPC_CORE_LIB_SECURITY_CREDENTIALS_EXTERNAL_EXTERNAL_ACCOUNT_CREDENTIALS_H diff --git a/src/core/lib/security/util/json_util.h b/src/core/lib/security/util/json_util.h index 42f7005e00e..cde9ca97ffd 100644 --- a/src/core/lib/security/util/json_util.h +++ b/src/core/lib/security/util/json_util.h @@ -30,6 +30,7 @@ #define GRPC_AUTH_JSON_TYPE_INVALID "invalid" #define GRPC_AUTH_JSON_TYPE_SERVICE_ACCOUNT "service_account" #define GRPC_AUTH_JSON_TYPE_AUTHORIZED_USER "authorized_user" +#define GRPC_AUTH_JSON_TYPE_EXTERNAL_ACCOUNT "external_account" // Gets a child property from a json node. const char* grpc_json_get_string_property(const grpc_core::Json& json, diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 5b2b78dcfb5..63a76598a01 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -383,6 +383,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/security/credentials/composite/composite_credentials.cc', 'src/core/lib/security/credentials/credentials.cc', 'src/core/lib/security/credentials/credentials_metadata.cc', + 'src/core/lib/security/credentials/external/external_account_credentials.cc', 'src/core/lib/security/credentials/fake/fake_credentials.cc', 'src/core/lib/security/credentials/google_default/credentials_generic.cc', 'src/core/lib/security/credentials/google_default/google_default_credentials.cc', diff --git a/test/core/security/credentials_test.cc b/test/core/security/credentials_test.cc index 22f6cdb7573..32c00179acf 100644 --- a/test/core/security/credentials_test.cc +++ b/test/core/security/credentials_test.cc @@ -43,6 +43,7 @@ #include "src/core/lib/http/httpcli.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/security/credentials/composite/composite_credentials.h" +#include "src/core/lib/security/credentials/external/external_account_credentials.h" #include "src/core/lib/security/credentials/fake/fake_credentials.h" #include "src/core/lib/security/credentials/google_default/google_default_credentials.h" #include "src/core/lib/security/credentials/jwt/jwt_credentials.h" @@ -135,6 +136,16 @@ static const char test_sts_endpoint_url[] = static const char test_method[] = "ThisIsNotAMethod"; +static const char valid_external_account_creds_token_exchange_response[] = + "{\"access_token\":\"token_exchange_access_token\"," + " \"expires_in\":3599," + " \"token_type\":\"Bearer\"}"; + +static const char + valid_external_account_creds_service_account_impersonation_response[] = + "{\"accessToken\":\"service_account_impersonation_access_token\"," + " \"expireTime\":\"2050-01-01T00:00:00Z\"}"; + /* -- Global state flags. -- */ static bool g_test_is_on_gce = false; @@ -1881,6 +1892,297 @@ static void test_auth_metadata_context(void) { } } +static void validate_external_account_creds_token_exchage_request( + const grpc_httpcli_request* request, const char* body, size_t body_size, + bool expect_actor_token) { + // Check that the body is constructed properly. + GPR_ASSERT(body != nullptr); + GPR_ASSERT(body_size != 0); + GPR_ASSERT(request->handshaker == &grpc_httpcli_ssl); + std::string get_url_equivalent = + absl::StrFormat("%s?%s", "https://foo.com:5555/token", body); + grpc_uri* uri = grpc_uri_parse(get_url_equivalent.c_str(), false); + GPR_ASSERT(strcmp(grpc_uri_get_query_arg(uri, "audience"), "audience") == 0); + GPR_ASSERT(strcmp(grpc_uri_get_query_arg(uri, "grant_type"), + "urn:ietf:params:oauth:grant-type:token-exchange") == 0); + GPR_ASSERT(strcmp(grpc_uri_get_query_arg(uri, "requested_token_type"), + "urn:ietf:params:oauth:token-type:access_token") == 0); + GPR_ASSERT(strcmp(grpc_uri_get_query_arg(uri, "subject_token"), + "test_subject_token") == 0); + GPR_ASSERT(strcmp(grpc_uri_get_query_arg(uri, "subject_token_type"), + "subject_token_type") == 0); + GPR_ASSERT(strcmp(grpc_uri_get_query_arg(uri, "scope"), + "https://www.googleapis.com/auth/cloud-platform") == 0); + grpc_uri_destroy(uri); + + // Check the rest of the request. + GPR_ASSERT(strcmp(request->host, "foo.com:5555") == 0); + GPR_ASSERT(strcmp(request->http.path, "/token") == 0); + GPR_ASSERT(request->http.hdr_count == 2); + GPR_ASSERT(strcmp(request->http.hdrs[0].key, "Content-Type") == 0); + GPR_ASSERT(strcmp(request->http.hdrs[0].value, + "application/x-www-form-urlencoded") == 0); + GPR_ASSERT(strcmp(request->http.hdrs[1].key, "Authorization") == 0); + GPR_ASSERT(strcmp(request->http.hdrs[1].value, + "Basic Y2xpZW50X2lkOmNsaWVudF9zZWNyZXQ=") == 0); +} + +static void +validate_external_account_creds_service_account_impersonation_request( + const grpc_httpcli_request* request, const char* body, size_t body_size, + bool expect_actor_token) { + // Check that the body is constructed properly. + GPR_ASSERT(body != nullptr); + GPR_ASSERT(body_size != 0); + GPR_ASSERT(request->handshaker == &grpc_httpcli_ssl); + GPR_ASSERT(strcmp(body, "scope=scope_1 scope_2") == 0); + + // Check the rest of the request. + GPR_ASSERT(strcmp(request->host, "foo.com:5555") == 0); + GPR_ASSERT(strcmp(request->http.path, "/service_account_impersonation") == 0); + GPR_ASSERT(request->http.hdr_count == 2); + GPR_ASSERT(strcmp(request->http.hdrs[0].key, "Content-Type") == 0); + GPR_ASSERT(strcmp(request->http.hdrs[0].value, + "application/x-www-form-urlencoded") == 0); + GPR_ASSERT(strcmp(request->http.hdrs[1].key, "Authorization") == 0); + GPR_ASSERT(strcmp(request->http.hdrs[1].value, + "Bearer token_exchange_access_token") == 0); +} + +static int external_account_creds_httpcli_post_success( + const grpc_httpcli_request* request, const char* body, size_t body_size, + grpc_millis /*deadline*/, grpc_closure* on_done, + grpc_httpcli_response* response) { + if (strcmp(request->http.path, "/token") == 0) { + validate_external_account_creds_token_exchage_request(request, body, + body_size, true); + *response = http_response( + 200, valid_external_account_creds_token_exchange_response); + } else if (strcmp(request->http.path, "/service_account_impersonation") == + 0) { + validate_external_account_creds_service_account_impersonation_request( + request, body, body_size, true); + *response = http_response( + 200, + valid_external_account_creds_service_account_impersonation_response); + } + grpc_core::ExecCtx::Run(DEBUG_LOCATION, on_done, GRPC_ERROR_NONE); + return 1; +} + +static int +external_account_creds_httpcli_post_failure_token_exchange_response_missing_access_token( + const grpc_httpcli_request* request, const char* body, size_t body_size, + grpc_millis /*deadline*/, grpc_closure* on_done, + grpc_httpcli_response* response) { + if (strcmp(request->http.path, "/token") == 0) { + *response = http_response(200, + "{\"not_access_token\":\"not_access_token\"," + "\"expires_in\":3599," + " \"token_type\":\"Bearer\"}"); + } else if (strcmp(request->http.path, "/service_account_impersonation") == + 0) { + *response = http_response( + 200, + valid_external_account_creds_service_account_impersonation_response); + } + grpc_core::ExecCtx::Run(DEBUG_LOCATION, on_done, GRPC_ERROR_NONE); + return 1; +} + +// The subclass of ExternalAccountCredentials for testing. +// ExternalAccountCredentials is an abstract class so we can't directly test +// against it. +class TestExternalAccountCredentials final + : public grpc_core::ExternalAccountCredentials { + public: + TestExternalAccountCredentials(ExternalAccountCredentialsOptions options, + std::vector scopes) + : ExternalAccountCredentials(std::move(options), std::move(scopes)) {} + + protected: + void RetrieveSubjectToken(const HTTPRequestContext* ctx, + const ExternalAccountCredentialsOptions& options, + std::function cb) { + cb("test_subject_token", GRPC_ERROR_NONE); + } +}; + +static void test_external_account_creds_success(void) { + expected_md emd[] = {{"authorization", "Bearer token_exchange_access_token"}}; + grpc_core::ExecCtx exec_ctx; + grpc_auth_metadata_context auth_md_ctx = {test_service_url, test_method, + nullptr, nullptr}; + grpc_core::Json credential_source(""); + TestExternalAccountCredentials::ExternalAccountCredentialsOptions options = { + "external_account", // type; + "audience", // audience; + "subject_token_type", // subject_token_type; + "", // service_account_impersonation_url; + "https://foo.com:5555/token", // token_url; + "https://foo.com:5555/token_info", // token_info_url; + credential_source, // credential_source; + "quota_project_id", // quota_project_id; + "client_id", // client_id; + "client_secret", // client_secret; + }; + TestExternalAccountCredentials creds(options, {}); + /* Check security level. */ + GPR_ASSERT(creds.min_security_level() == GRPC_PRIVACY_AND_INTEGRITY); + /* First request: http put should be called. */ + request_metadata_state* state = + make_request_metadata_state(GRPC_ERROR_NONE, emd, GPR_ARRAY_SIZE(emd)); + grpc_httpcli_set_override(httpcli_get_should_not_be_called, + external_account_creds_httpcli_post_success); + run_request_metadata_test(&creds, auth_md_ctx, state); + grpc_core::ExecCtx::Get()->Flush(); + /* Second request: the cached token should be served directly. */ + state = + make_request_metadata_state(GRPC_ERROR_NONE, emd, GPR_ARRAY_SIZE(emd)); + grpc_httpcli_set_override(httpcli_get_should_not_be_called, + httpcli_post_should_not_be_called); + run_request_metadata_test(&creds, auth_md_ctx, state); + grpc_core::ExecCtx::Get()->Flush(); + grpc_httpcli_set_override(nullptr, nullptr); +} + +static void +test_external_account_creds_success_with_service_account_impersonation(void) { + expected_md emd[] = { + {"authorization", "Bearer service_account_impersonation_access_token"}}; + grpc_core::ExecCtx exec_ctx; + grpc_auth_metadata_context auth_md_ctx = {test_service_url, test_method, + nullptr, nullptr}; + grpc_core::Json credential_source(""); + TestExternalAccountCredentials::ExternalAccountCredentialsOptions options = { + "external_account", // type; + "audience", // audience; + "subject_token_type", // subject_token_type; + "https://foo.com:5555/service_account_impersonation", // service_account_impersonation_url; + "https://foo.com:5555/token", // token_url; + "https://foo.com:5555/token_info", // token_info_url; + credential_source, // credential_source; + "quota_project_id", // quota_project_id; + "client_id", // client_id; + "client_secret", // client_secret; + }; + TestExternalAccountCredentials creds(options, {"scope_1", "scope_2"}); + /* Check security level. */ + GPR_ASSERT(creds.min_security_level() == GRPC_PRIVACY_AND_INTEGRITY); + /* First request: http put should be called. */ + request_metadata_state* state = + make_request_metadata_state(GRPC_ERROR_NONE, emd, GPR_ARRAY_SIZE(emd)); + grpc_httpcli_set_override(httpcli_get_should_not_be_called, + external_account_creds_httpcli_post_success); + run_request_metadata_test(&creds, auth_md_ctx, state); + grpc_core::ExecCtx::Get()->Flush(); + grpc_httpcli_set_override(nullptr, nullptr); +} + +static void test_external_account_creds_faiure_invalid_token_url(void) { + grpc_core::ExecCtx exec_ctx; + grpc_auth_metadata_context auth_md_ctx = {test_service_url, test_method, + nullptr, nullptr}; + grpc_core::Json credential_source(""); + TestExternalAccountCredentials::ExternalAccountCredentialsOptions options = { + "external_account", // type; + "audience", // audience; + "subject_token_type", // subject_token_type; + "https://foo.com:5555/service_account_impersonation", // service_account_impersonation_url; + "invalid_token_url", // token_url; + "https://foo.com:5555/token_info", // token_info_url; + credential_source, // credential_source; + "quota_project_id", // quota_project_id; + "client_id", // client_id; + "client_secret", // client_secret; + }; + TestExternalAccountCredentials creds(options, {}); + grpc_httpcli_set_override(httpcli_get_should_not_be_called, + httpcli_post_should_not_be_called); + grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "Invalid token url: invalid_token_url."); + grpc_error* expected_error = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + "Error occurred when fetching oauth2 token.", &error, 1); + request_metadata_state* state = + make_request_metadata_state(expected_error, nullptr, 0); + run_request_metadata_test(&creds, auth_md_ctx, state); + GRPC_ERROR_UNREF(error); + grpc_core::ExecCtx::Get()->Flush(); + grpc_httpcli_set_override(nullptr, nullptr); +} + +static void +test_external_account_creds_faiure_invalid_service_account_impersonation_url( + void) { + grpc_core::ExecCtx exec_ctx; + grpc_auth_metadata_context auth_md_ctx = {test_service_url, test_method, + nullptr, nullptr}; + grpc_core::Json credential_source(""); + TestExternalAccountCredentials::ExternalAccountCredentialsOptions options = { + "external_account", // type; + "audience", // audience; + "subject_token_type", // subject_token_type; + "invalid_service_account_impersonation_url", // service_account_impersonation_url; + "https://foo.com:5555/token", // token_url; + "https://foo.com:5555/token_info", // token_info_url; + credential_source, // credential_source; + "quota_project_id", // quota_project_id; + "client_id", // client_id; + "client_secret", // client_secret; + }; + TestExternalAccountCredentials creds(options, {}); + grpc_httpcli_set_override(httpcli_get_should_not_be_called, + external_account_creds_httpcli_post_success); + grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "Invalid service account impersonation url: " + "invalid_service_account_impersonation_url."); + grpc_error* expected_error = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + "Error occurred when fetching oauth2 token.", &error, 1); + request_metadata_state* state = + make_request_metadata_state(expected_error, nullptr, 0); + run_request_metadata_test(&creds, auth_md_ctx, state); + GRPC_ERROR_UNREF(error); + grpc_core::ExecCtx::Get()->Flush(); + grpc_httpcli_set_override(nullptr, nullptr); +} + +static void +test_external_account_creds_faiure_token_exchange_response_missing_access_token( + void) { + grpc_core::ExecCtx exec_ctx; + grpc_auth_metadata_context auth_md_ctx = {test_service_url, test_method, + nullptr, nullptr}; + grpc_core::Json credential_source(""); + TestExternalAccountCredentials::ExternalAccountCredentialsOptions options = { + "external_account", // type; + "audience", // audience; + "subject_token_type", // subject_token_type; + "https://foo.com:5555/service_account_impersonation", // service_account_impersonation_url; + "https://foo.com:5555/token", // token_url; + "https://foo.com:5555/token_info", // token_info_url; + credential_source, // credential_source; + "quota_project_id", // quota_project_id; + "client_id", // client_id; + "client_secret", // client_secret; + }; + TestExternalAccountCredentials creds(options, {}); + grpc_httpcli_set_override( + httpcli_get_should_not_be_called, + external_account_creds_httpcli_post_failure_token_exchange_response_missing_access_token); + grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "Missing or invalid access_token in " + "{\"not_access_token\":\"not_access_token\",\"expires_in\":3599,\"token_" + "type\":\"Bearer\"}."); + grpc_error* expected_error = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + "Error occurred when fetching oauth2 token.", &error, 1); + request_metadata_state* state = + make_request_metadata_state(expected_error, nullptr, 0); + run_request_metadata_test(&creds, auth_md_ctx, state); + GRPC_ERROR_UNREF(error); + grpc_core::ExecCtx::Get()->Flush(); + grpc_httpcli_set_override(nullptr, nullptr); +} + int main(int argc, char** argv) { grpc::testing::TestEnvironment env(argc, argv); grpc_init(); @@ -1925,6 +2227,11 @@ int main(int argc, char** argv) { test_get_well_known_google_credentials_file_path(); test_channel_creds_duplicate_without_call_creds(); test_auth_metadata_context(); + test_external_account_creds_success(); + test_external_account_creds_success_with_service_account_impersonation(); + test_external_account_creds_faiure_invalid_token_url(); + test_external_account_creds_faiure_invalid_service_account_impersonation_url(); + test_external_account_creds_faiure_token_exchange_response_missing_access_token(); grpc_shutdown(); return 0; } diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index b645161be23..cb6201ebf03 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1734,6 +1734,8 @@ src/core/lib/security/credentials/composite/composite_credentials.h \ src/core/lib/security/credentials/credentials.cc \ src/core/lib/security/credentials/credentials.h \ src/core/lib/security/credentials/credentials_metadata.cc \ +src/core/lib/security/credentials/external/external_account_credentials.cc \ +src/core/lib/security/credentials/external/external_account_credentials.h \ src/core/lib/security/credentials/fake/fake_credentials.cc \ src/core/lib/security/credentials/fake/fake_credentials.h \ src/core/lib/security/credentials/google_default/credentials_generic.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index f4afb777a81..4080267059f 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -1561,6 +1561,8 @@ src/core/lib/security/credentials/composite/composite_credentials.h \ src/core/lib/security/credentials/credentials.cc \ src/core/lib/security/credentials/credentials.h \ src/core/lib/security/credentials/credentials_metadata.cc \ +src/core/lib/security/credentials/external/external_account_credentials.cc \ +src/core/lib/security/credentials/external/external_account_credentials.h \ src/core/lib/security/credentials/fake/fake_credentials.cc \ src/core/lib/security/credentials/fake/fake_credentials.h \ src/core/lib/security/credentials/google_default/credentials_generic.cc \ From 5a25a427b5af333cf72c9f5615c8f5d044ad4d30 Mon Sep 17 00:00:00 2001 From: Donna Dionne Date: Tue, 13 Oct 2020 10:04:13 -0700 Subject: [PATCH 09/12] Protecting the circuit breaking (drop action) using environment variable. All the logic of refcounting are left but will not result in any actions. Test added to test protected case. --- .../client_channel/lb_policy/xds/eds.cc | 32 ++++++--- test/cpp/end2end/xds_end2end_test.cc | 66 +++++++++++++++++++ 2 files changed, 90 insertions(+), 8 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc index 5fae55a93dc..6afdce4d5cc 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc @@ -36,6 +36,7 @@ #include "src/core/ext/xds/xds_client.h" #include "src/core/ext/xds/xds_client_stats.h" #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/gpr/env.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" @@ -56,6 +57,17 @@ constexpr char kEds[] = "eds_experimental"; const char* kXdsLocalityNameAttributeKey = "xds_locality_name"; +// TODO (donnadionne): Check to see if circuit breaking is enabled, this will be +// removed once circuit breaking feature is fully integrated and enabled by +// default. +bool XdsCircuitBreakingEnabled() { + char* value = gpr_getenv("GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING"); + bool parsed_value; + bool parse_succeeded = gpr_parse_bool_value(value, &parsed_value); + gpr_free(value); + return parse_succeeded && parsed_value; +} + // Config for EDS LB policy. class EdsLbConfig : public LoadBalancingPolicy::Config { public: @@ -206,6 +218,7 @@ class EdsLb : public LoadBalancingPolicy { RefCountedPtr drop_config_; RefCountedPtr drop_stats_; RefCountedPtr child_picker_; + bool xds_circuit_breaking_enabled_; uint32_t max_concurrent_requests_; }; @@ -309,6 +322,7 @@ EdsLb::EdsPicker::EdsPicker(RefCountedPtr eds_policy) : eds_policy_(std::move(eds_policy)), drop_stats_(eds_policy_->drop_stats_), child_picker_(eds_policy_->child_picker_), + xds_circuit_breaking_enabled_(XdsCircuitBreakingEnabled()), max_concurrent_requests_( eds_policy_->config_->max_concurrent_requests()) { if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_eds_trace)) { @@ -318,16 +332,18 @@ EdsLb::EdsPicker::EdsPicker(RefCountedPtr eds_policy) } EdsLb::PickResult EdsLb::EdsPicker::Pick(PickArgs args) { - // Check and see if we exceeded the max concurrent requests count. uint32_t current = eds_policy_->concurrent_requests_.FetchAdd(1); - if (current >= max_concurrent_requests_) { - eds_policy_->concurrent_requests_.FetchSub(1); - if (drop_stats_ != nullptr) { - drop_stats_->AddUncategorizedDrops(); + if (xds_circuit_breaking_enabled_) { + // Check and see if we exceeded the max concurrent requests count. + if (current >= max_concurrent_requests_) { + eds_policy_->concurrent_requests_.FetchSub(1); + if (drop_stats_ != nullptr) { + drop_stats_->AddUncategorizedDrops(); + } + PickResult result; + result.type = PickResult::PICK_COMPLETE; + return result; } - PickResult result; - result.type = PickResult::PICK_COMPLETE; - return result; } // If we're not dropping the call, we should always have a child picker. if (child_picker_ == nullptr) { // Should never happen. diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index f355c413323..4310bd4bf4f 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -2290,6 +2290,7 @@ TEST_P(XdsResolverOnlyTest, CircuitBreaking) { Status status_; }; + gpr_setenv("GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING", "true"); constexpr size_t kMaxConcurrentRequests = 10; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); @@ -2332,6 +2333,71 @@ TEST_P(XdsResolverOnlyTest, CircuitBreaking) { // Make sure RPCs go to the correct backend: EXPECT_EQ(kMaxConcurrentRequests + 1, backends_[0]->backend_service()->request_count()); + gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING"); +} + +TEST_P(XdsResolverOnlyTest, CircuitBreakingDisabled) { + class TestRpc { + public: + TestRpc() {} + + void StartRpc(grpc::testing::EchoTestService::Stub* stub) { + sender_thread_ = std::thread([this, stub]() { + EchoResponse response; + EchoRequest request; + request.mutable_param()->set_client_cancel_after_us(1 * 1000 * 1000); + request.set_message(kRequestMessage); + status_ = stub->Echo(&context_, request, &response); + }); + } + + void CancelRpc() { + context_.TryCancel(); + sender_thread_.join(); + } + + private: + std::thread sender_thread_; + ClientContext context_; + Status status_; + }; + + constexpr size_t kMaxConcurrentRequests = 10; + SetNextResolution({}); + SetNextResolutionForLbChannelAllBalancers(); + // Populate new EDS resources. + AdsServiceImpl::EdsResourceArgs args({ + {"locality0", GetBackendPorts(0, 1)}, + }); + balancers_[0]->ads_service()->SetEdsResource( + AdsServiceImpl::BuildEdsResource(args)); + // Update CDS resource to set max concurrent request. + CircuitBreakers circuit_breaks; + Cluster cluster = balancers_[0]->ads_service()->default_cluster(); + auto* threshold = cluster.mutable_circuit_breakers()->add_thresholds(); + threshold->set_priority(RoutingPriority::DEFAULT); + threshold->mutable_max_requests()->set_value(kMaxConcurrentRequests); + balancers_[0]->ads_service()->SetCdsResource(cluster); + // Send exactly max_concurrent_requests long RPCs. + TestRpc rpcs[kMaxConcurrentRequests]; + for (size_t i = 0; i < kMaxConcurrentRequests; ++i) { + rpcs[i].StartRpc(stub_.get()); + } + // Wait for all RPCs to be in flight. + while (backends_[0]->backend_service()->RpcsWaitingForClientCancel() < + kMaxConcurrentRequests) { + gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), + gpr_time_from_micros(1 * 1000, GPR_TIMESPAN))); + } + // Sending a RPC now should not fail as circuit breaking is disabled. + Status status = SendRpc(); + EXPECT_TRUE(status.ok()); + for (size_t i = 0; i < kMaxConcurrentRequests; ++i) { + rpcs[i].CancelRpc(); + } + // Make sure RPCs go to the correct backend: + EXPECT_EQ(kMaxConcurrentRequests + 1, + backends_[0]->backend_service()->request_count()); } TEST_P(XdsResolverOnlyTest, MultipleChannelsShareXdsClient) { From 844d59935649010a8df69df4296eda7d8dd23202 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 13 Oct 2020 11:47:43 -0700 Subject: [PATCH 10/12] Improve ref-count tracing. --- .../filters/client_channel/client_channel.cc | 5 +- .../health/health_check_client.cc | 5 +- .../ext/filters/client_channel/lb_policy.cc | 6 +- .../client_channel/lb_policy/grpclb/grpclb.cc | 4 +- .../lb_policy/subchannel_list.h | 3 +- .../ext/filters/client_channel/resolver.cc | 4 +- .../ext/filters/client_channel/subchannel.cc | 5 +- .../client_channel/subchannel_interface.h | 5 +- .../subchannel_pool_interface.h | 6 +- .../chttp2/transport/chttp2_transport.cc | 4 +- src/core/ext/xds/xds_client.cc | 16 +++- src/core/ext/xds/xds_client_stats.cc | 8 +- src/core/lib/gprpp/dual_ref_counted.h | 91 +++++++++---------- src/core/lib/gprpp/orphanable.h | 10 +- src/core/lib/gprpp/ref_counted.h | 86 ++++++++---------- src/core/lib/iomgr/ev_epollex_linux.cc | 5 +- src/core/lib/iomgr/tcp_posix.cc | 3 +- .../lib/security/context/security_context.h | 4 +- .../security_connector/security_connector.h | 4 +- src/core/lib/transport/transport.cc | 4 +- test/core/gprpp/dual_ref_counted_test.cc | 6 +- test/core/gprpp/orphanable_test.cc | 7 +- test/core/gprpp/ref_counted_ptr_test.cc | 8 +- test/core/gprpp/ref_counted_test.cc | 8 +- 24 files changed, 160 insertions(+), 147 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index c497bc4f216..7292184bf71 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -894,7 +894,10 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface { public: SubchannelWrapper(ChannelData* chand, Subchannel* subchannel, grpc_core::UniquePtr health_check_service_name) - : SubchannelInterface(&grpc_client_channel_routing_trace), + : SubchannelInterface( + GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace) + ? "SubchannelWrapper" + : nullptr), chand_(chand), subchannel_(subchannel), health_check_service_name_(std::move(health_check_service_name)) { diff --git a/src/core/ext/filters/client_channel/health/health_check_client.cc b/src/core/ext/filters/client_channel/health/health_check_client.cc index 60a2ebb4bfa..4e49ed651ee 100644 --- a/src/core/ext/filters/client_channel/health/health_check_client.cc +++ b/src/core/ext/filters/client_channel/health/health_check_client.cc @@ -51,7 +51,10 @@ HealthCheckClient::HealthCheckClient( grpc_pollset_set* interested_parties, RefCountedPtr channelz_node, RefCountedPtr watcher) - : InternallyRefCounted(&grpc_health_check_client_trace), + : InternallyRefCounted( + GRPC_TRACE_FLAG_ENABLED(grpc_health_check_client_trace) + ? "HealthCheckClient" + : nullptr), service_name_(service_name), connected_subchannel_(std::move(connected_subchannel)), interested_parties_(interested_parties), diff --git a/src/core/ext/filters/client_channel/lb_policy.cc b/src/core/ext/filters/client_channel/lb_policy.cc index 6c639ef86ce..53c61eecd0c 100644 --- a/src/core/ext/filters/client_channel/lb_policy.cc +++ b/src/core/ext/filters/client_channel/lb_policy.cc @@ -32,7 +32,11 @@ DebugOnlyTraceFlag grpc_trace_lb_policy_refcount(false, "lb_policy_refcount"); // LoadBalancingPolicy::LoadBalancingPolicy(Args args, intptr_t initial_refcount) - : InternallyRefCounted(&grpc_trace_lb_policy_refcount, initial_refcount), + : InternallyRefCounted( + GRPC_TRACE_FLAG_ENABLED(grpc_trace_lb_policy_refcount) + ? "LoadBalancingPolicy" + : nullptr, + initial_refcount), work_serializer_(std::move(args.work_serializer)), interested_parties_(grpc_pollset_set_create()), channel_control_helper_(std::move(args.channel_control_helper)) {} diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 421feb8c162..d8397c6e74c 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -745,7 +745,9 @@ void GrpcLb::Helper::AddTraceEvent(TraceSeverity severity, GrpcLb::BalancerCallState::BalancerCallState( RefCountedPtr parent_grpclb_policy) - : InternallyRefCounted(&grpc_lb_glb_trace), + : InternallyRefCounted( + GRPC_TRACE_FLAG_ENABLED(grpc_lb_glb_trace) ? "BalancerCallState" + : nullptr), grpclb_policy_(std::move(parent_grpclb_policy)) { GPR_ASSERT(grpclb_policy_ != nullptr); GPR_ASSERT(!grpclb_policy()->shutting_down_); diff --git a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h index c1ed5e213cd..839c788cdce 100644 --- a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h +++ b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h @@ -353,7 +353,8 @@ SubchannelList::SubchannelList( LoadBalancingPolicy* policy, TraceFlag* tracer, ServerAddressList addresses, LoadBalancingPolicy::ChannelControlHelper* helper, const grpc_channel_args& args) - : InternallyRefCounted(tracer), + : InternallyRefCounted( + GRPC_TRACE_FLAG_ENABLED(*tracer) ? "SubchannelList" : nullptr), policy_(policy), tracer_(tracer) { if (GRPC_TRACE_FLAG_ENABLED(*tracer_)) { diff --git a/src/core/ext/filters/client_channel/resolver.cc b/src/core/ext/filters/client_channel/resolver.cc index 4a76a7d7bdc..8831db29c17 100644 --- a/src/core/ext/filters/client_channel/resolver.cc +++ b/src/core/ext/filters/client_channel/resolver.cc @@ -31,7 +31,9 @@ namespace grpc_core { Resolver::Resolver(std::shared_ptr work_serializer, std::unique_ptr result_handler) - : InternallyRefCounted(&grpc_trace_resolver_refcount), + : InternallyRefCounted(GRPC_TRACE_FLAG_ENABLED(grpc_trace_resolver_refcount) + ? "Resolver" + : nullptr), work_serializer_(std::move(work_serializer)), result_handler_(std::move(result_handler)) {} diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index 677e929d0fa..b188736071c 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -87,7 +87,10 @@ DebugOnlyTraceFlag grpc_trace_subchannel_refcount(false, "subchannel_refcount"); ConnectedSubchannel::ConnectedSubchannel( grpc_channel_stack* channel_stack, const grpc_channel_args* args, RefCountedPtr channelz_subchannel) - : RefCounted(&grpc_trace_subchannel_refcount), + : RefCounted( + GRPC_TRACE_FLAG_ENABLED(grpc_trace_subchannel_refcount) + ? "ConnectedSubchannel" + : nullptr), channel_stack_(channel_stack), args_(grpc_channel_args_copy(args)), channelz_subchannel_(std::move(channelz_subchannel)) {} diff --git a/src/core/ext/filters/client_channel/subchannel_interface.h b/src/core/ext/filters/client_channel/subchannel_interface.h index f78581d83a6..48d7708d85c 100644 --- a/src/core/ext/filters/client_channel/subchannel_interface.h +++ b/src/core/ext/filters/client_channel/subchannel_interface.h @@ -46,9 +46,8 @@ class SubchannelInterface : public RefCounted { virtual grpc_pollset_set* interested_parties() = 0; }; - template - explicit SubchannelInterface(TraceFlagT* trace_flag = nullptr) - : RefCounted(trace_flag) {} + explicit SubchannelInterface(const char* trace = nullptr) + : RefCounted(trace) {} virtual ~SubchannelInterface() = default; diff --git a/src/core/ext/filters/client_channel/subchannel_pool_interface.h b/src/core/ext/filters/client_channel/subchannel_pool_interface.h index 2fe5fa3fd05..9c48f61d399 100644 --- a/src/core/ext/filters/client_channel/subchannel_pool_interface.h +++ b/src/core/ext/filters/client_channel/subchannel_pool_interface.h @@ -23,6 +23,7 @@ #include "src/core/lib/avl/avl.h" #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/ref_counted.h" namespace grpc_core { @@ -62,7 +63,10 @@ class SubchannelKey { // shut down safely. See https://github.com/grpc/grpc/issues/12560. class SubchannelPoolInterface : public RefCounted { public: - SubchannelPoolInterface() : RefCounted(&grpc_subchannel_pool_trace) {} + SubchannelPoolInterface() + : RefCounted(GRPC_TRACE_FLAG_ENABLED(grpc_subchannel_pool_trace) + ? "SubchannelPoolInterface" + : nullptr) {} virtual ~SubchannelPoolInterface() {} // Registers a subchannel against a key. Returns the subchannel registered diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 67e0e8505f0..88346ed8f8e 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -434,7 +434,9 @@ static void init_keepalive_pings_if_enabled(grpc_chttp2_transport* t) { grpc_chttp2_transport::grpc_chttp2_transport( const grpc_channel_args* channel_args, grpc_endpoint* ep, bool is_client, grpc_resource_user* resource_user) - : refs(1, &grpc_trace_chttp2_refcount), + : refs(1, GRPC_TRACE_FLAG_ENABLED(grpc_trace_chttp2_refcount) + ? "chttp2_refcount" + : nullptr), ep(ep), peer_string(grpc_endpoint_get_peer(ep)), resource_user(resource_user), diff --git a/src/core/ext/xds/xds_client.cc b/src/core/ext/xds/xds_client.cc index ad49714a938..003729ee722 100644 --- a/src/core/ext/xds/xds_client.cc +++ b/src/core/ext/xds/xds_client.cc @@ -433,7 +433,9 @@ class XdsClient::ChannelState::StateWatcher XdsClient::ChannelState::ChannelState(WeakRefCountedPtr xds_client, grpc_channel* channel) - : InternallyRefCounted(&grpc_xds_client_trace), + : InternallyRefCounted( + GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace) ? "ChannelState" + : nullptr), xds_client_(std::move(xds_client)), channel_(channel) { GPR_ASSERT(channel_ != nullptr); @@ -634,7 +636,9 @@ void XdsClient::ChannelState::RetryableCall::OnRetryTimerLocked( XdsClient::ChannelState::AdsCallState::AdsCallState( RefCountedPtr> parent) - : InternallyRefCounted(&grpc_xds_client_trace), + : InternallyRefCounted( + GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace) ? "AdsCallState" + : nullptr), parent_(std::move(parent)) { // Init the ADS call. Note that the call will progress every time there's // activity in xds_client()->interested_parties_, which is comprised of @@ -1385,7 +1389,9 @@ bool XdsClient::ChannelState::LrsCallState::Reporter::OnReportDoneLocked( XdsClient::ChannelState::LrsCallState::LrsCallState( RefCountedPtr> parent) - : InternallyRefCounted(&grpc_xds_client_trace), + : InternallyRefCounted( + GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace) ? "LrsCallState" + : nullptr), parent_(std::move(parent)) { // Init the LRS call. Note that the call will progress every time there's // activity in xds_client()->interested_parties_, which is comprised of @@ -1737,7 +1743,9 @@ grpc_channel* CreateXdsChannel(const XdsBootstrap& bootstrap, } // namespace XdsClient::XdsClient(grpc_error** error) - : DualRefCounted(&grpc_xds_client_trace), + : DualRefCounted(GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace) + ? "XdsClient" + : nullptr), request_timeout_(GetRequestTimeout()), interested_parties_(grpc_pollset_set_create()), bootstrap_( diff --git a/src/core/ext/xds/xds_client_stats.cc b/src/core/ext/xds/xds_client_stats.cc index 06cd95c4fc0..72cae4dfd40 100644 --- a/src/core/ext/xds/xds_client_stats.cc +++ b/src/core/ext/xds/xds_client_stats.cc @@ -45,7 +45,9 @@ XdsClusterDropStats::XdsClusterDropStats(RefCountedPtr xds_client, absl::string_view lrs_server_name, absl::string_view cluster_name, absl::string_view eds_service_name) - : RefCounted(&grpc_xds_client_trace), + : RefCounted(GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace) + ? "XdsClusterDropStats" + : nullptr), xds_client_(std::move(xds_client)), lrs_server_name_(lrs_server_name), cluster_name_(cluster_name), @@ -96,7 +98,9 @@ XdsClusterLocalityStats::XdsClusterLocalityStats( RefCountedPtr xds_client, absl::string_view lrs_server_name, absl::string_view cluster_name, absl::string_view eds_service_name, RefCountedPtr name) - : RefCounted(&grpc_xds_client_trace), + : RefCounted(GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace) + ? "XdsClusterLocalityStats" + : nullptr), xds_client_(std::move(xds_client)), lrs_server_name_(lrs_server_name), cluster_name_(cluster_name), diff --git a/src/core/lib/gprpp/dual_ref_counted.h b/src/core/lib/gprpp/dual_ref_counted.h index 66935bbdea1..25d8625bf64 100644 --- a/src/core/lib/gprpp/dual_ref_counted.h +++ b/src/core/lib/gprpp/dual_ref_counted.h @@ -27,7 +27,6 @@ #include #include -#include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/atomic.h" #include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/orphanable.h" @@ -72,10 +71,9 @@ class DualRefCounted : public Orphanable { const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); #ifndef NDEBUG const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { - gpr_log(GPR_INFO, "%s:%p unref %d -> %d, weak_ref %d -> %d", - trace_flag_->name(), this, strong_refs, strong_refs - 1, - weak_refs, weak_refs + 1); + if (trace_ != nullptr) { + gpr_log(GPR_INFO, "%s:%p unref %d -> %d, weak_ref %d -> %d", trace_, this, + strong_refs, strong_refs - 1, weak_refs, weak_refs + 1); } GPR_ASSERT(strong_refs > 0); #endif @@ -91,10 +89,10 @@ class DualRefCounted : public Orphanable { const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); #ifndef NDEBUG const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { + if (trace_ != nullptr) { gpr_log(GPR_INFO, "%s:%p %s:%d unref %d -> %d, weak_ref %d -> %d) %s", - trace_flag_->name(), this, location.file(), location.line(), - strong_refs, strong_refs - 1, weak_refs, weak_refs + 1, reason); + trace_, this, location.file(), location.line(), strong_refs, + strong_refs - 1, weak_refs, weak_refs + 1, reason); } GPR_ASSERT(strong_refs > 0); #else @@ -115,10 +113,9 @@ class DualRefCounted : public Orphanable { const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); #ifndef NDEBUG const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { + if (trace_ != nullptr) { gpr_log(GPR_INFO, "%s:%p ref_if_non_zero %d -> %d (weak_refs=%d)", - trace_flag_->name(), this, strong_refs, strong_refs + 1, - weak_refs); + trace_, this, strong_refs, strong_refs + 1, weak_refs); } #endif if (strong_refs == 0) return nullptr; @@ -135,11 +132,11 @@ class DualRefCounted : public Orphanable { const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); #ifndef NDEBUG const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { + if (trace_ != nullptr) { gpr_log(GPR_INFO, "%s:%p %s:%d ref_if_non_zero %d -> %d (weak_refs=%d) %s", - trace_flag_->name(), this, location.file(), location.line(), - strong_refs, strong_refs + 1, weak_refs, reason); + trace_, this, location.file(), location.line(), strong_refs, + strong_refs + 1, weak_refs, reason); } #else // Avoid unused-parameter warnings for debug-only parameters @@ -167,17 +164,18 @@ class DualRefCounted : public Orphanable { void WeakUnref() { #ifndef NDEBUG // Grab a copy of the trace flag before the atomic change, since we - // can't safely access it afterwards if we're going to be freed. - auto* trace_flag = trace_flag_; + // will no longer be holding a ref afterwards and therefore can't + // safely access it, since another thread might free us in the interim. + const char* trace = trace_; #endif const uint64_t prev_ref_pair = refs_.FetchSub(MakeRefPair(0, 1), MemoryOrder::ACQ_REL); const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); #ifndef NDEBUG const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); - if (trace_flag != nullptr && trace_flag->enabled()) { - gpr_log(GPR_INFO, "%s:%p weak_unref %d -> %d (refs=%d)", - trace_flag->name(), this, weak_refs, weak_refs - 1, strong_refs); + if (trace != nullptr) { + gpr_log(GPR_INFO, "%s:%p weak_unref %d -> %d (refs=%d)", trace, this, + weak_refs, weak_refs - 1, strong_refs); } GPR_ASSERT(weak_refs > 0); #endif @@ -188,18 +186,19 @@ class DualRefCounted : public Orphanable { void WeakUnref(const DebugLocation& location, const char* reason) { #ifndef NDEBUG // Grab a copy of the trace flag before the atomic change, since we - // can't safely access it afterwards if we're going to be freed. - auto* trace_flag = trace_flag_; + // will no longer be holding a ref afterwards and therefore can't + // safely access it, since another thread might free us in the interim. + const char* trace = trace_; #endif const uint64_t prev_ref_pair = refs_.FetchSub(MakeRefPair(0, 1), MemoryOrder::ACQ_REL); const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); #ifndef NDEBUG const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); - if (trace_flag != nullptr && trace_flag->enabled()) { - gpr_log(GPR_INFO, "%s:%p %s:%d weak_unref %d -> %d (refs=%d) %s", - trace_flag->name(), this, location.file(), location.line(), - weak_refs, weak_refs - 1, strong_refs, reason); + if (trace_ != nullptr) { + gpr_log(GPR_INFO, "%s:%p %s:%d weak_unref %d -> %d (refs=%d) %s", trace, + this, location.file(), location.line(), weak_refs, weak_refs - 1, + strong_refs, reason); } GPR_ASSERT(weak_refs > 0); #else @@ -217,21 +216,18 @@ class DualRefCounted : public Orphanable { DualRefCounted& operator=(const DualRefCounted&) = delete; protected: - // TraceFlagT is defined to accept both DebugOnlyTraceFlag and TraceFlag. - // Note: RefCount tracing is only enabled on debug builds, even when a - // TraceFlag is used. - template + // Note: Tracing is a no-op in non-debug builds. explicit DualRefCounted( - TraceFlagT* + const char* #ifndef NDEBUG // Leave unnamed if NDEBUG to avoid unused parameter warning - trace_flag + trace #endif = nullptr, int32_t initial_refcount = 1) : #ifndef NDEBUG - trace_flag_(trace_flag), + trace_(trace), #endif refs_(MakeRefPair(initial_refcount, 0)) { } @@ -262,10 +258,9 @@ class DualRefCounted : public Orphanable { const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); GPR_ASSERT(strong_refs != 0); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { - gpr_log(GPR_INFO, "%s:%p ref %d -> %d; (weak_refs=%d)", - trace_flag_->name(), this, strong_refs, strong_refs + 1, - weak_refs); + if (trace_ != nullptr) { + gpr_log(GPR_INFO, "%s:%p ref %d -> %d; (weak_refs=%d)", trace_, this, + strong_refs, strong_refs + 1, weak_refs); } #else refs_.FetchAdd(MakeRefPair(1, 0), MemoryOrder::RELAXED); @@ -278,10 +273,10 @@ class DualRefCounted : public Orphanable { const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); GPR_ASSERT(strong_refs != 0); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { - gpr_log(GPR_INFO, "%s:%p %s:%d ref %d -> %d (weak_refs=%d) %s", - trace_flag_->name(), this, location.file(), location.line(), - strong_refs, strong_refs + 1, weak_refs, reason); + if (trace_ != nullptr) { + gpr_log(GPR_INFO, "%s:%p %s:%d ref %d -> %d (weak_refs=%d) %s", trace_, + this, location.file(), location.line(), strong_refs, + strong_refs + 1, weak_refs, reason); } #else // Use conditionally-important parameters @@ -297,9 +292,9 @@ class DualRefCounted : public Orphanable { refs_.FetchAdd(MakeRefPair(0, 1), MemoryOrder::RELAXED); const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { - gpr_log(GPR_INFO, "%s:%p weak_ref %d -> %d; (refs=%d)", - trace_flag_->name(), this, weak_refs, weak_refs + 1, strong_refs); + if (trace_ != nullptr) { + gpr_log(GPR_INFO, "%s:%p weak_ref %d -> %d; (refs=%d)", trace_, this, + weak_refs, weak_refs + 1, strong_refs); } #else refs_.FetchAdd(MakeRefPair(0, 1), MemoryOrder::RELAXED); @@ -312,10 +307,10 @@ class DualRefCounted : public Orphanable { refs_.FetchAdd(MakeRefPair(0, 1), MemoryOrder::RELAXED); const uint32_t strong_refs = GetStrongRefs(prev_ref_pair); const uint32_t weak_refs = GetWeakRefs(prev_ref_pair); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { - gpr_log(GPR_INFO, "%s:%p %s:%d weak_ref %d -> %d (refs=%d) %s", - trace_flag_->name(), this, location.file(), location.line(), - weak_refs, weak_refs + 1, strong_refs, reason); + if (trace_ != nullptr) { + gpr_log(GPR_INFO, "%s:%p %s:%d weak_ref %d -> %d (refs=%d) %s", trace_, + this, location.file(), location.line(), weak_refs, weak_refs + 1, + strong_refs, reason); } #else // Use conditionally-important parameters @@ -326,7 +321,7 @@ class DualRefCounted : public Orphanable { } #ifndef NDEBUG - TraceFlag* trace_flag_; + const char* trace_; #endif Atomic refs_; }; diff --git a/src/core/lib/gprpp/orphanable.h b/src/core/lib/gprpp/orphanable.h index 79542de6dd1..4336538ac0b 100644 --- a/src/core/lib/gprpp/orphanable.h +++ b/src/core/lib/gprpp/orphanable.h @@ -27,7 +27,6 @@ #include #include -#include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/memory.h" #include "src/core/lib/gprpp/ref_counted.h" @@ -85,13 +84,10 @@ class InternallyRefCounted : public Orphanable { template friend class RefCountedPtr; - // TraceFlagT is defined to accept both DebugOnlyTraceFlag and TraceFlag. - // Note: RefCount tracing is only enabled on debug builds, even when a - // TraceFlag is used. - template - explicit InternallyRefCounted(TraceFlagT* trace_flag = nullptr, + // Note: Tracing is a no-op on non-debug builds. + explicit InternallyRefCounted(const char* trace = nullptr, intptr_t initial_refcount = 1) - : refs_(initial_refcount, trace_flag) {} + : refs_(initial_refcount, trace) {} virtual ~InternallyRefCounted() = default; RefCountedPtr Ref() GRPC_MUST_USE_RESULT { diff --git a/src/core/lib/gprpp/ref_counted.h b/src/core/lib/gprpp/ref_counted.h index b69ba312312..bb5e18020c7 100644 --- a/src/core/lib/gprpp/ref_counted.h +++ b/src/core/lib/gprpp/ref_counted.h @@ -29,7 +29,6 @@ #include #include -#include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/atomic.h" #include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/memory.h" @@ -51,21 +50,19 @@ class RefCount { // `init` is the initial refcount stored in this object. // - // TraceFlagT is defined to accept both DebugOnlyTraceFlag and TraceFlag. - // Note: RefCount tracing is only enabled on debug builds, even when a - // TraceFlag is used. - template - constexpr explicit RefCount( + // `trace` is a string to be logged with trace events; if null, no + // trace logging will be done. Tracing is a no-op in non-debug builds. + explicit RefCount( Value init = 1, - TraceFlagT* + const char* #ifndef NDEBUG // Leave unnamed if NDEBUG to avoid unused parameter warning - trace_flag + trace #endif = nullptr) : #ifndef NDEBUG - trace_flag_(trace_flag), + trace_(trace), #endif value_(init) { } @@ -74,9 +71,9 @@ class RefCount { void Ref(Value n = 1) { #ifndef NDEBUG const Value prior = value_.FetchAdd(n, MemoryOrder::RELAXED); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { - gpr_log(GPR_INFO, "%s:%p ref %" PRIdPTR " -> %" PRIdPTR, - trace_flag_->name(), this, prior, prior + n); + if (trace_ != nullptr) { + gpr_log(GPR_INFO, "%s:%p ref %" PRIdPTR " -> %" PRIdPTR, trace_, this, + prior, prior + n); } #else value_.FetchAdd(n, MemoryOrder::RELAXED); @@ -85,16 +82,15 @@ class RefCount { void Ref(const DebugLocation& location, const char* reason, Value n = 1) { #ifndef NDEBUG const Value prior = value_.FetchAdd(n, MemoryOrder::RELAXED); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { + if (trace_ != nullptr) { gpr_log(GPR_INFO, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s", - trace_flag_->name(), this, location.file(), location.line(), - prior, prior + n, reason); + trace_, this, location.file(), location.line(), prior, prior + n, + reason); } #else // Use conditionally-important parameters (void)location; (void)reason; - value_.FetchAdd(n, MemoryOrder::RELAXED); #endif } @@ -103,9 +99,9 @@ class RefCount { void RefNonZero() { #ifndef NDEBUG const Value prior = value_.FetchAdd(1, MemoryOrder::RELAXED); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { - gpr_log(GPR_INFO, "%s:%p ref %" PRIdPTR " -> %" PRIdPTR, - trace_flag_->name(), this, prior, prior + 1); + if (trace_ != nullptr) { + gpr_log(GPR_INFO, "%s:%p ref %" PRIdPTR " -> %" PRIdPTR, trace_, this, + prior, prior + 1); } assert(prior > 0); #else @@ -115,10 +111,10 @@ class RefCount { void RefNonZero(const DebugLocation& location, const char* reason) { #ifndef NDEBUG const Value prior = value_.FetchAdd(1, MemoryOrder::RELAXED); - if (trace_flag_ != nullptr && trace_flag_->enabled()) { + if (trace_ != nullptr) { gpr_log(GPR_INFO, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s", - trace_flag_->name(), this, location.file(), location.line(), - prior, prior + 1, reason); + trace_, this, location.file(), location.line(), prior, prior + 1, + reason); } assert(prior > 0); #else @@ -131,23 +127,22 @@ class RefCount { bool RefIfNonZero() { #ifndef NDEBUG - if (trace_flag_ != nullptr && trace_flag_->enabled()) { + if (trace_ != nullptr) { const Value prior = get(); gpr_log(GPR_INFO, "%s:%p ref_if_non_zero %" PRIdPTR " -> %" PRIdPTR, - trace_flag_->name(), this, prior, prior + 1); + trace_, this, prior, prior + 1); } #endif return value_.IncrementIfNonzero(); } bool RefIfNonZero(const DebugLocation& location, const char* reason) { #ifndef NDEBUG - if (trace_flag_ != nullptr && trace_flag_->enabled()) { + if (trace_ != nullptr) { const Value prior = get(); gpr_log(GPR_INFO, - "%s:%p %s:%d ref_if_non_zero " - "%" PRIdPTR " -> %" PRIdPTR " %s", - trace_flag_->name(), this, location.file(), location.line(), - prior, prior + 1, reason); + "%s:%p %s:%d ref_if_non_zero %" PRIdPTR " -> %" PRIdPTR " %s", + trace_, this, location.file(), location.line(), prior, prior + 1, + reason); } #endif // Avoid unused-parameter warnings for debug-only parameters @@ -160,14 +155,15 @@ class RefCount { bool Unref() { #ifndef NDEBUG // Grab a copy of the trace flag before the atomic change, since we - // can't safely access it afterwards if we're going to be freed. - auto* trace_flag = trace_flag_; + // will no longer be holding a ref afterwards and therefore can't + // safely access it, since another thread might free us in the interim. + auto* trace = trace_; #endif const Value prior = value_.FetchSub(1, MemoryOrder::ACQ_REL); #ifndef NDEBUG - if (trace_flag != nullptr && trace_flag->enabled()) { - gpr_log(GPR_INFO, "%s:%p unref %" PRIdPTR " -> %" PRIdPTR, - trace_flag->name(), this, prior, prior - 1); + if (trace != nullptr) { + gpr_log(GPR_INFO, "%s:%p unref %" PRIdPTR " -> %" PRIdPTR, trace, this, + prior, prior - 1); } GPR_DEBUG_ASSERT(prior > 0); #endif @@ -176,15 +172,16 @@ class RefCount { bool Unref(const DebugLocation& location, const char* reason) { #ifndef NDEBUG // Grab a copy of the trace flag before the atomic change, since we - // can't safely access it afterwards if we're going to be freed. - auto* trace_flag = trace_flag_; + // will no longer be holding a ref afterwards and therefore can't + // safely access it, since another thread might free us in the interim. + auto* trace = trace_; #endif const Value prior = value_.FetchSub(1, MemoryOrder::ACQ_REL); #ifndef NDEBUG - if (trace_flag != nullptr && trace_flag->enabled()) { + if (trace != nullptr) { gpr_log(GPR_INFO, "%s:%p %s:%d unref %" PRIdPTR " -> %" PRIdPTR " %s", - trace_flag->name(), this, location.file(), location.line(), prior, - prior - 1, reason); + trace, this, location.file(), location.line(), prior, prior - 1, + reason); } GPR_DEBUG_ASSERT(prior > 0); #else @@ -199,7 +196,7 @@ class RefCount { Value get() const { return value_.Load(MemoryOrder::RELAXED); } #ifndef NDEBUG - TraceFlag* trace_flag_; + const char* trace_; #endif Atomic value_; }; @@ -315,13 +312,10 @@ class RefCounted : public Impl { RefCounted& operator=(const RefCounted&) = delete; protected: - // TraceFlagT is defined to accept both DebugOnlyTraceFlag and TraceFlag. - // Note: RefCount tracing is only enabled on debug builds, even when a - // TraceFlag is used. - template - explicit RefCounted(TraceFlagT* trace_flag = nullptr, + // Note: Tracing is a no-op on non-debug builds. + explicit RefCounted(const char* trace = nullptr, intptr_t initial_refcount = 1) - : refs_(initial_refcount, trace_flag) {} + : refs_(initial_refcount, trace) {} private: // Allow RefCountedPtr<> to access IncrementRefCount(). diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index 055251f7525..61179291358 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -591,7 +591,10 @@ static grpc_error* pollable_create(pollable_type type, pollable** p) { } (*p)->type = type; - new (&(*p)->refs) grpc_core::RefCount(1, &grpc_trace_pollable_refcount); + new (&(*p)->refs) grpc_core::RefCount( + 1, GRPC_TRACE_FLAG_ENABLED(grpc_trace_pollable_refcount) + ? "pollable_refcount" + : nullptr); gpr_mu_init(&(*p)->mu); (*p)->epfd = epfd; (*p)->owner_fd = nullptr; diff --git a/src/core/lib/iomgr/tcp_posix.cc b/src/core/lib/iomgr/tcp_posix.cc index 3c5703460fc..2d4624a790b 100644 --- a/src/core/lib/iomgr/tcp_posix.cc +++ b/src/core/lib/iomgr/tcp_posix.cc @@ -1798,7 +1798,8 @@ grpc_endpoint* grpc_tcp_create(grpc_fd* em_fd, #endif } /* paired with unref in grpc_tcp_destroy */ - new (&tcp->refcount) grpc_core::RefCount(1, &grpc_tcp_trace); + new (&tcp->refcount) grpc_core::RefCount( + 1, GRPC_TRACE_FLAG_ENABLED(grpc_tcp_trace) ? "tcp" : nullptr); gpr_atm_no_barrier_store(&tcp->shutdown_count, 0); tcp->em_fd = em_fd; grpc_slice_buffer_init(&tcp->last_read_buffer); diff --git a/src/core/lib/security/context/security_context.h b/src/core/lib/security/context/security_context.h index b1991089ae5..6ff4856e6bd 100644 --- a/src/core/lib/security/context/security_context.h +++ b/src/core/lib/security/context/security_context.h @@ -54,7 +54,9 @@ struct grpc_auth_context grpc_core::RefCountedPtr chained) : grpc_core::RefCounted( - &grpc_trace_auth_context_refcount), + GRPC_TRACE_FLAG_ENABLED(grpc_trace_auth_context_refcount) + ? "auth_context_refcount" + : nullptr), chained_(std::move(chained)) { if (chained_ != nullptr) { peer_identity_property_name_ = chained_->peer_identity_property_name_; diff --git a/src/core/lib/security/security_connector/security_connector.h b/src/core/lib/security/security_connector/security_connector.h index 2ddd9db7a3c..c3e264f30f9 100644 --- a/src/core/lib/security/security_connector/security_connector.h +++ b/src/core/lib/security/security_connector/security_connector.h @@ -49,7 +49,9 @@ class grpc_security_connector public: explicit grpc_security_connector(const char* url_scheme) : grpc_core::RefCounted( - &grpc_trace_security_connector_refcount), + GRPC_TRACE_FLAG_ENABLED(grpc_trace_security_connector_refcount) + ? "security_connector_refcount" + : nullptr), url_scheme_(url_scheme) {} virtual ~grpc_security_connector() = default; diff --git a/src/core/lib/transport/transport.cc b/src/core/lib/transport/transport.cc index 99a32980aa6..057c69bbbc9 100644 --- a/src/core/lib/transport/transport.cc +++ b/src/core/lib/transport/transport.cc @@ -91,7 +91,9 @@ void grpc_stream_ref_init(grpc_stream_refcount* refcount, int /*initial_refs*/, #endif GRPC_CLOSURE_INIT(&refcount->destroy, cb, cb_arg, grpc_schedule_on_exec_ctx); - new (&refcount->refs) grpc_core::RefCount(1, &grpc_trace_stream_refcount); + new (&refcount->refs) grpc_core::RefCount( + 1, GRPC_TRACE_FLAG_ENABLED(grpc_trace_stream_refcount) ? "stream_refcount" + : nullptr); new (&refcount->slice_refcount) grpc_slice_refcount( grpc_slice_refcount::Type::REGULAR, &refcount->refs, slice_stream_destroy, refcount, &refcount->slice_refcount); diff --git a/test/core/gprpp/dual_ref_counted_test.cc b/test/core/gprpp/dual_ref_counted_test.cc index ac3e5d96061..3dd39ff1e4f 100644 --- a/test/core/gprpp/dual_ref_counted_test.cc +++ b/test/core/gprpp/dual_ref_counted_test.cc @@ -72,13 +72,9 @@ TEST(DualRefCounted, RefIfNonZero) { foo->WeakUnref(); } -// Note: We use DebugOnlyTraceFlag instead of TraceFlag to ensure that -// things build properly in both debug and non-debug cases. -DebugOnlyTraceFlag foo_tracer(true, "foo"); - class FooWithTracing : public DualRefCounted { public: - FooWithTracing() : DualRefCounted(&foo_tracer) {} + FooWithTracing() : DualRefCounted("FooWithTracing") {} ~FooWithTracing() { GPR_ASSERT(shutting_down_); } void Orphan() override { shutting_down_ = true; } diff --git a/test/core/gprpp/orphanable_test.cc b/test/core/gprpp/orphanable_test.cc index 341b753548a..9fd4af4a36c 100644 --- a/test/core/gprpp/orphanable_test.cc +++ b/test/core/gprpp/orphanable_test.cc @@ -79,15 +79,10 @@ TEST(OrphanablePtr, InternallyRefCounted) { bar->FinishWork(); } -// Note: We use DebugOnlyTraceFlag instead of TraceFlag to ensure that -// things build properly in both debug and non-debug cases. -DebugOnlyTraceFlag baz_tracer(true, "baz"); - class Baz : public InternallyRefCounted { public: Baz() : Baz(0) {} - explicit Baz(int value) - : InternallyRefCounted(&baz_tracer), value_(value) {} + explicit Baz(int value) : InternallyRefCounted("Baz"), value_(value) {} void Orphan() override { Unref(); } int value() const { return value_; } diff --git a/test/core/gprpp/ref_counted_ptr_test.cc b/test/core/gprpp/ref_counted_ptr_test.cc index 3907b21dc1a..28adda9ae3a 100644 --- a/test/core/gprpp/ref_counted_ptr_test.cc +++ b/test/core/gprpp/ref_counted_ptr_test.cc @@ -180,11 +180,9 @@ TEST(MakeRefCounted, Args) { EXPECT_EQ(3, foo->value()); } -TraceFlag foo_tracer(true, "foo"); - class FooWithTracing : public RefCounted { public: - FooWithTracing() : RefCounted(&foo_tracer) {} + FooWithTracing() : RefCounted("FooWithTracing") {} }; TEST(RefCountedPtr, RefCountedWithTracing) { @@ -416,11 +414,9 @@ TEST(WeakRefCountedPtr, Swap) { EXPECT_EQ(bar_strong.get(), bar3.get()); } -TraceFlag bar_tracer(true, "bar"); - class BarWithTracing : public DualRefCounted { public: - BarWithTracing() : DualRefCounted(&bar_tracer) {} + BarWithTracing() : DualRefCounted("BarWithTracing") {} ~BarWithTracing() { GPR_ASSERT(shutting_down_); } diff --git a/test/core/gprpp/ref_counted_test.cc b/test/core/gprpp/ref_counted_test.cc index c978f020295..fefe90b0caa 100644 --- a/test/core/gprpp/ref_counted_test.cc +++ b/test/core/gprpp/ref_counted_test.cc @@ -126,13 +126,9 @@ TEST(RefCountedNonPolymorphic, ExtraRef) { foo->Unref(); } -// Note: We use DebugOnlyTraceFlag instead of TraceFlag to ensure that -// things build properly in both debug and non-debug cases. -DebugOnlyTraceFlag foo_tracer(true, "foo"); - class FooWithTracing : public RefCounted { public: - FooWithTracing() : RefCounted(&foo_tracer) {} + FooWithTracing() : RefCounted("Foo") {} }; TEST(RefCountedWithTracing, Basic) { @@ -150,7 +146,7 @@ TEST(RefCountedWithTracing, Basic) { class FooNonPolymorphicWithTracing : public RefCounted { public: - FooNonPolymorphicWithTracing() : RefCounted(&foo_tracer) {} + FooNonPolymorphicWithTracing() : RefCounted("FooNonPolymorphicWithTracing") {} }; TEST(RefCountedNonPolymorphicWithTracing, Basic) { From fff44c05c60220b625ad4e09ba50034a4d3df4eb Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 14 Oct 2020 09:44:44 -0700 Subject: [PATCH 11/12] Change service_config_test to use gmock ContainsRegex() matcher. --- .../client_channel/service_config_test.cc | 441 +++++++++--------- 1 file changed, 229 insertions(+), 212 deletions(-) diff --git a/test/core/client_channel/service_config_test.cc b/test/core/client_channel/service_config_test.cc index 2c3ed355bd7..abdb43fefa9 100644 --- a/test/core/client_channel/service_config_test.cc +++ b/test/core/client_channel/service_config_test.cc @@ -16,10 +16,9 @@ * */ -#include - #include "absl/strings/str_cat.h" +#include #include #include @@ -143,13 +142,6 @@ class ErrorParser : public ServiceConfigParser::Parser { static const char* GlobalError() { return "ErrorParser : globalError"; } }; -void VerifyRegexMatch(grpc_error* error, const std::regex& regex) { - std::smatch match; - std::string error_str = grpc_error_string(error); - EXPECT_TRUE(std::regex_search(error_str, match, regex)) << error_str; - GRPC_ERROR_UNREF(error); -} - class ServiceConfigTest : public ::testing::Test { protected: void SetUp() override { @@ -168,8 +160,9 @@ TEST_F(ServiceConfigTest, ErrorCheck1) { const char* test_json = ""; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex(std::string("JSON parse error")); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex("JSON parse error")); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, BasicTest1) { @@ -204,12 +197,13 @@ TEST_F(ServiceConfigTest, ErrorDuplicateMethodConfigNames) { "]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - std::string("Service config parsing error.*referenced_errors" - ".*Method Params.*referenced_errors" - ".*methodConfig.*referenced_errors" - ".*multiple method configs with same name")); - VerifyRegexMatch(error, regex); + EXPECT_THAT( + grpc_error_string(error), + ::testing::ContainsRegex("Service config parsing error.*referenced_errors" + ".*Method Params.*referenced_errors" + ".*methodConfig.*referenced_errors" + ".*multiple method configs with same name")); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, ErrorDuplicateMethodConfigNamesWithNullMethod) { @@ -220,12 +214,13 @@ TEST_F(ServiceConfigTest, ErrorDuplicateMethodConfigNamesWithNullMethod) { "]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - std::string("Service config parsing error.*referenced_errors" - ".*Method Params.*referenced_errors" - ".*methodConfig.*referenced_errors" - ".*multiple method configs with same name")); - VerifyRegexMatch(error, regex); + EXPECT_THAT( + grpc_error_string(error), + ::testing::ContainsRegex("Service config parsing error.*referenced_errors" + ".*Method Params.*referenced_errors" + ".*methodConfig.*referenced_errors" + ".*multiple method configs with same name")); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, ErrorDuplicateMethodConfigNamesWithEmptyMethod) { @@ -236,12 +231,13 @@ TEST_F(ServiceConfigTest, ErrorDuplicateMethodConfigNamesWithEmptyMethod) { "]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - std::string("Service config parsing error.*referenced_errors" - ".*Method Params.*referenced_errors" - ".*methodConfig.*referenced_errors" - ".*multiple method configs with same name")); - VerifyRegexMatch(error, regex); + EXPECT_THAT( + grpc_error_string(error), + ::testing::ContainsRegex("Service config parsing error.*referenced_errors" + ".*Method Params.*referenced_errors" + ".*methodConfig.*referenced_errors" + ".*multiple method configs with same name")); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, ErrorDuplicateDefaultMethodConfigs) { @@ -252,12 +248,13 @@ TEST_F(ServiceConfigTest, ErrorDuplicateDefaultMethodConfigs) { "]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - std::string("Service config parsing error.*referenced_errors" - ".*Method Params.*referenced_errors" - ".*methodConfig.*referenced_errors" - ".*multiple default method configs")); - VerifyRegexMatch(error, regex); + EXPECT_THAT( + grpc_error_string(error), + ::testing::ContainsRegex("Service config parsing error.*referenced_errors" + ".*Method Params.*referenced_errors" + ".*methodConfig.*referenced_errors" + ".*multiple default method configs")); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, ErrorDuplicateDefaultMethodConfigsWithNullService) { @@ -268,12 +265,13 @@ TEST_F(ServiceConfigTest, ErrorDuplicateDefaultMethodConfigsWithNullService) { "]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - std::string("Service config parsing error.*referenced_errors" - ".*Method Params.*referenced_errors" - ".*methodConfig.*referenced_errors" - ".*multiple default method configs")); - VerifyRegexMatch(error, regex); + EXPECT_THAT( + grpc_error_string(error), + ::testing::ContainsRegex("Service config parsing error.*referenced_errors" + ".*Method Params.*referenced_errors" + ".*methodConfig.*referenced_errors" + ".*multiple default method configs")); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, ErrorDuplicateDefaultMethodConfigsWithEmptyService) { @@ -284,12 +282,13 @@ TEST_F(ServiceConfigTest, ErrorDuplicateDefaultMethodConfigsWithEmptyService) { "]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - std::string("Service config parsing error.*referenced_errors" - ".*Method Params.*referenced_errors" - ".*methodConfig.*referenced_errors" - ".*multiple default method configs")); - VerifyRegexMatch(error, regex); + EXPECT_THAT( + grpc_error_string(error), + ::testing::ContainsRegex("Service config parsing error.*referenced_errors" + ".*Method Params.*referenced_errors" + ".*methodConfig.*referenced_errors" + ".*multiple default method configs")); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, ValidMethodConfig) { @@ -338,22 +337,24 @@ TEST_F(ServiceConfigTest, Parser1ErrorInvalidType) { const char* test_json = "{\"global_param\":\"5\"}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - absl::StrCat("Service config parsing error.*referenced_errors.*" - "Global Params.*referenced_errors.*", - TestParser1::InvalidTypeErrorMessage())); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex(absl::StrCat( + "Service config parsing error.*referenced_errors.*" + "Global Params.*referenced_errors.*", + TestParser1::InvalidTypeErrorMessage()))); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, Parser1ErrorInvalidValue) { const char* test_json = "{\"global_param\":-5}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - absl::StrCat("Service config parsing error.*referenced_errors.*" - "Global Params.*referenced_errors.*", - TestParser1::InvalidValueErrorMessage())); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex(absl::StrCat( + "Service config parsing error.*referenced_errors.*" + "Global Params.*referenced_errors.*", + TestParser1::InvalidValueErrorMessage()))); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, Parser2BasicTest) { @@ -393,12 +394,13 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidType) { "\"method_param\":\"5\"}]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - absl::StrCat("Service config parsing error.*referenced_errors\":\\[.*" - "Method Params.*referenced_errors.*methodConfig.*" - "referenced_errors.*", - TestParser2::InvalidTypeErrorMessage())); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex(absl::StrCat( + "Service config parsing error.*referenced_errors\":\\[.*" + "Method Params.*referenced_errors.*methodConfig.*" + "referenced_errors.*", + TestParser2::InvalidTypeErrorMessage()))); + GRPC_ERROR_UNREF(error); } TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) { @@ -407,12 +409,13 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) { "\"method_param\":-5}]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - absl::StrCat("Service config parsing error.*referenced_errors\":\\[.*" - "Method Params.*referenced_errors.*methodConfig.*" - "referenced_errors.*", - TestParser2::InvalidValueErrorMessage())); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex(absl::StrCat( + "Service config parsing error.*referenced_errors\":\\[.*" + "Method Params.*referenced_errors.*methodConfig.*" + "referenced_errors.*", + TestParser2::InvalidValueErrorMessage()))); + GRPC_ERROR_UNREF(error); } // Test parsing with ErrorParsers which always add errors @@ -434,24 +437,29 @@ TEST_F(ErroredParsersScopingTest, GlobalParams) { const char* test_json = "{}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex(absl::StrCat( - "Service config parsing error.*referenced_errors\":\\[.*" - "Global Params.*referenced_errors.*", - ErrorParser::GlobalError(), ".*", ErrorParser::GlobalError())); - VerifyRegexMatch(error, regex); + EXPECT_THAT( + grpc_error_string(error), + ::testing::ContainsRegex(absl::StrCat( + "Service config parsing error.*referenced_errors\":\\[.*" + "Global Params.*referenced_errors.*", + ErrorParser::GlobalError(), ".*", ErrorParser::GlobalError()))); + GRPC_ERROR_UNREF(error); } TEST_F(ErroredParsersScopingTest, MethodParams) { const char* test_json = "{\"methodConfig\": [{}]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex(absl::StrCat( - "Service config parsing error.*referenced_errors\":\\[.*" - "Global Params.*referenced_errors.*", - ErrorParser::GlobalError(), ".*", ErrorParser::GlobalError(), - ".*Method Params.*referenced_errors.*methodConfig.*referenced_errors.*", - ErrorParser::MethodError(), ".*", ErrorParser::MethodError())); - VerifyRegexMatch(error, regex); + EXPECT_THAT( + grpc_error_string(error), + ::testing::ContainsRegex(absl::StrCat( + "Service config parsing error.*referenced_errors\":\\[.*" + "Global Params.*referenced_errors.*", + ErrorParser::GlobalError(), ".*", ErrorParser::GlobalError(), + ".*Method Params.*referenced_errors.*methodConfig.*" + "referenced_errors.*", + ErrorParser::MethodError(), ".*", ErrorParser::MethodError()))); + GRPC_ERROR_UNREF(error); } class ClientChannelParserTest : public ::testing::Test { @@ -527,13 +535,14 @@ TEST_F(ClientChannelParserTest, UnknownLoadBalancingConfig) { const char* test_json = "{\"loadBalancingConfig\": [{\"unknown\":{}}]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Global Params.*referenced_errors.*" - "Client channel global parser.*referenced_errors.*" - "field:loadBalancingConfig.*referenced_errors.*" - "No known policies in list: unknown"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Global Params.*referenced_errors.*" + "Client channel global parser.*referenced_errors.*" + "field:loadBalancingConfig.*referenced_errors.*" + "No known policies in list: unknown")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, InvalidGrpclbLoadBalancingConfig) { @@ -544,15 +553,16 @@ TEST_F(ClientChannelParserTest, InvalidGrpclbLoadBalancingConfig) { "]}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Global Params.*referenced_errors.*" - "Client channel global parser.*referenced_errors.*" - "field:loadBalancingConfig.*referenced_errors.*" - "GrpcLb Parser.*referenced_errors.*" - "field:childPolicy.*referenced_errors.*" - "type should be array"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Global Params.*referenced_errors.*" + "Client channel global parser.*referenced_errors.*" + "field:loadBalancingConfig.*referenced_errors.*" + "GrpcLb Parser.*referenced_errors.*" + "field:childPolicy.*referenced_errors.*" + "type should be array")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, ValidLoadBalancingPolicy) { @@ -581,25 +591,27 @@ TEST_F(ClientChannelParserTest, UnknownLoadBalancingPolicy) { const char* test_json = "{\"loadBalancingPolicy\":\"unknown\"}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Global Params.*referenced_errors.*" - "Client channel global parser.*referenced_errors.*" - "field:loadBalancingPolicy error:Unknown lb policy"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Global Params.*referenced_errors.*" + "Client channel global parser.*referenced_errors.*" + "field:loadBalancingPolicy error:Unknown lb policy")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, LoadBalancingPolicyXdsNotAllowed) { const char* test_json = "{\"loadBalancingPolicy\":\"eds_experimental\"}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Global Params.*referenced_errors.*" - "Client channel global parser.*referenced_errors.*" - "field:loadBalancingPolicy error:eds_experimental requires " - "a config. Please use loadBalancingConfig instead."); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Global Params.*referenced_errors.*" + "Client channel global parser.*referenced_errors.*" + "field:loadBalancingPolicy error:eds_experimental requires " + "a config. Please use loadBalancingConfig instead.")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, ValidRetryThrottling) { @@ -630,13 +642,14 @@ TEST_F(ClientChannelParserTest, RetryThrottlingMissingFields) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Global Params.*referenced_errors.*" - "Client channel global parser.*referenced_errors.*" - "field:retryThrottling field:maxTokens error:Not found.*" - "field:retryThrottling field:tokenRatio error:Not found"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Global Params.*referenced_errors.*" + "Client channel global parser.*referenced_errors.*" + "field:retryThrottling field:maxTokens error:Not found.*" + "field:retryThrottling field:tokenRatio error:Not found")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, InvalidRetryThrottlingNegativeMaxTokens) { @@ -649,13 +662,14 @@ TEST_F(ClientChannelParserTest, InvalidRetryThrottlingNegativeMaxTokens) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Global Params.*referenced_errors.*" - "Client channel global parser.*referenced_errors.*" - "field:retryThrottling field:maxTokens error:should " - "be greater than zero"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Global Params.*referenced_errors.*" + "Client channel global parser.*referenced_errors.*" + "field:retryThrottling field:maxTokens error:should " + "be greater than zero")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, InvalidRetryThrottlingInvalidTokenRatio) { @@ -668,13 +682,14 @@ TEST_F(ClientChannelParserTest, InvalidRetryThrottlingInvalidTokenRatio) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Global Params.*referenced_errors.*" - "Client channel global parser.*referenced_errors.*" - "field:retryThrottling field:tokenRatio " - "error:Failed parsing"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Global Params.*referenced_errors.*" + "Client channel global parser.*referenced_errors.*" + "field:retryThrottling field:tokenRatio " + "error:Failed parsing")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, ValidTimeout) { @@ -712,13 +727,14 @@ TEST_F(ClientChannelParserTest, InvalidTimeout) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Method Params.*referenced_errors.*" - "methodConfig.*referenced_errors.*" - "Client channel parser.*referenced_errors.*" - "field:timeout error:Failed parsing"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Method Params.*referenced_errors.*" + "methodConfig.*referenced_errors.*" + "Client channel parser.*referenced_errors.*" + "field:timeout error:Failed parsing")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, ValidWaitForReady) { @@ -762,13 +778,14 @@ TEST_F(ClientChannelParserTest, InvalidWaitForReady) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Method Params.*referenced_errors.*" - "methodConfig.*referenced_errors.*" - "Client channel parser.*referenced_errors.*" - "field:waitForReady error:Type should be true/false"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Method Params.*referenced_errors.*" + "methodConfig.*referenced_errors.*" + "Client channel parser.*referenced_errors.*" + "field:waitForReady error:Type should be true/false")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, ValidRetryPolicy) { @@ -823,14 +840,15 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyMaxAttempts) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Method Params.*referenced_errors.*" - "methodConfig.*referenced_errors.*" - "Client channel parser.*referenced_errors.*" - "retryPolicy.*referenced_errors.*" - "field:maxAttempts error:should be at least 2"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Method Params.*referenced_errors.*" + "methodConfig.*referenced_errors.*" + "Client channel parser.*referenced_errors.*" + "retryPolicy.*referenced_errors.*" + "field:maxAttempts error:should be at least 2")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, InvalidRetryPolicyInitialBackoff) { @@ -851,14 +869,15 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyInitialBackoff) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Method Params.*referenced_errors.*" - "methodConfig.*referenced_errors.*" - "Client channel parser.*referenced_errors.*" - "retryPolicy.*referenced_errors.*" - "field:initialBackoff error:Failed to parse"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Method Params.*referenced_errors.*" + "methodConfig.*referenced_errors.*" + "Client channel parser.*referenced_errors.*" + "retryPolicy.*referenced_errors.*" + "field:initialBackoff error:Failed to parse")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, InvalidRetryPolicyMaxBackoff) { @@ -879,14 +898,15 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyMaxBackoff) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Method Params.*referenced_errors.*" - "methodConfig.*referenced_errors.*" - "Client channel parser.*referenced_errors.*" - "retryPolicy.*referenced_errors.*" - "field:maxBackoff error:failed to parse"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Method Params.*referenced_errors.*" + "methodConfig.*referenced_errors.*" + "Client channel parser.*referenced_errors.*" + "retryPolicy.*referenced_errors.*" + "field:maxBackoff error:failed to parse")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, InvalidRetryPolicyBackoffMultiplier) { @@ -907,14 +927,15 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyBackoffMultiplier) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Method Params.*referenced_errors.*" - "methodConfig.*referenced_errors.*" - "Client channel parser.*referenced_errors.*" - "retryPolicy.*referenced_errors.*" - "field:backoffMultiplier error:should be of type number"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Method Params.*referenced_errors.*" + "methodConfig.*referenced_errors.*" + "Client channel parser.*referenced_errors.*" + "retryPolicy.*referenced_errors.*" + "field:backoffMultiplier error:should be of type number")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, InvalidRetryPolicyRetryableStatusCodes) { @@ -935,14 +956,15 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyRetryableStatusCodes) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Method Params.*referenced_errors.*" - "methodConfig.*referenced_errors.*" - "Client channel parser.*referenced_errors.*" - "retryPolicy.*referenced_errors.*" - "field:retryableStatusCodes error:should be non-empty"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Method Params.*referenced_errors.*" + "methodConfig.*referenced_errors.*" + "Client channel parser.*referenced_errors.*" + "retryPolicy.*referenced_errors.*" + "field:retryableStatusCodes error:should be non-empty")); + GRPC_ERROR_UNREF(error); } TEST_F(ClientChannelParserTest, ValidHealthCheck) { @@ -975,10 +997,11 @@ TEST_F(ClientChannelParserTest, InvalidHealthCheckMultipleEntries) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "JSON parsing failed.*referenced_errors.*" - "duplicate key \"healthCheckConfig\" at index 104"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "JSON parsing failed.*referenced_errors.*" + "duplicate key \"healthCheckConfig\" at index 104")); + GRPC_ERROR_UNREF(error); } class MessageSizeParserTest : public ::testing::Test { @@ -1028,13 +1051,14 @@ TEST_F(MessageSizeParserTest, InvalidMaxRequestMessageBytes) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Method Params.*referenced_errors.*" - "methodConfig.*referenced_errors.*" - "Message size parser.*referenced_errors.*" - "field:maxRequestMessageBytes error:should be non-negative"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Method Params.*referenced_errors.*" + "methodConfig.*referenced_errors.*" + "Message size parser.*referenced_errors.*" + "field:maxRequestMessageBytes error:should be non-negative")); + GRPC_ERROR_UNREF(error); } TEST_F(MessageSizeParserTest, InvalidMaxResponseMessageBytes) { @@ -1049,28 +1073,21 @@ TEST_F(MessageSizeParserTest, InvalidMaxResponseMessageBytes) { "}"; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(nullptr, test_json, &error); - std::regex regex( - "Service config parsing error.*referenced_errors.*" - "Method Params.*referenced_errors.*" - "methodConfig.*referenced_errors.*" - "Message size parser.*referenced_errors.*" - "field:maxResponseMessageBytes error:should be of type " - "number"); - VerifyRegexMatch(error, regex); + EXPECT_THAT(grpc_error_string(error), + ::testing::ContainsRegex( + "Service config parsing error.*referenced_errors.*" + "Method Params.*referenced_errors.*" + "methodConfig.*referenced_errors.*" + "Message size parser.*referenced_errors.*" + "field:maxResponseMessageBytes error:should be of type " + "number")); + GRPC_ERROR_UNREF(error); } } // namespace testing } // namespace grpc_core int main(int argc, char** argv) { -// Regexes don't work in old libstdc++ versions, so just skip testing in those -// cases -#if defined(__GLIBCXX__) && (__GLIBCXX__ <= 20150623) - gpr_log(GPR_ERROR, - "Skipping service_config_test since std::regex is not supported on " - "this system."); - return 0; -#endif ::testing::InitGoogleTest(&argc, argv); grpc::testing::TestEnvironment env(argc, argv); grpc_init(); From 51997e36d0ee96ccda805c13b656379152f28e25 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 14 Oct 2020 08:39:13 -0700 Subject: [PATCH 12/12] Remove unnecessary dependencies for xds_cluster_manager LB policy. --- BUILD | 26 +++++-------------- .../lb_policy/xds/xds_cluster_manager.cc | 25 +++++------------- 2 files changed, 14 insertions(+), 37 deletions(-) diff --git a/BUILD b/BUILD index 120aad21a06..9dd669e735b 100644 --- a/BUILD +++ b/BUILD @@ -1305,23 +1305,6 @@ grpc_cc_library( ], ) -grpc_cc_library( - name = "grpc_xds_api_header", - hdrs = [ - "src/core/ext/xds/xds_api.h", - "src/core/ext/xds/xds_bootstrap.h", - "src/core/ext/xds/xds_client_stats.h", - ], - external_deps = [ - "upb_lib", - "re2", - ], - language = "c++", - deps = [ - "grpc_base", - ], -) - grpc_cc_library( name = "grpc_xds_client", srcs = [ @@ -1331,8 +1314,15 @@ grpc_cc_library( "src/core/ext/xds/xds_client_stats.cc", ], hdrs = [ + "src/core/ext/xds/xds_api.h", + "src/core/ext/xds/xds_bootstrap.h", "src/core/ext/xds/xds_channel_args.h", "src/core/ext/xds/xds_client.h", + "src/core/ext/xds/xds_client_stats.h", + ], + external_deps = [ + "upb_lib", + "re2", ], language = "c++", deps = [ @@ -1341,7 +1331,6 @@ grpc_cc_library( "grpc_client_channel", "grpc_google_mesh_ca_certificate_provider_factory", "grpc_secure", - "grpc_xds_api_header", ], ) @@ -1422,7 +1411,6 @@ grpc_cc_library( "grpc_base", "grpc_client_channel", "grpc_resolver_xds_header", - "grpc_xds_api_header", ], ) diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc index 6f7279bff05..4363c86c10b 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc @@ -16,18 +16,13 @@ #include -#include -#include -#include +#include +#include +#include -#include "absl/container/inlined_vector.h" -#include "absl/strings/match.h" -#include "absl/strings/numbers.h" +#include "absl/status/status.h" #include "absl/strings/str_cat.h" -#include "absl/strings/str_join.h" -#include "absl/strings/str_split.h" #include "absl/strings/string_view.h" -#include "re2/re2.h" #include @@ -36,7 +31,6 @@ #include "src/core/ext/filters/client_channel/lb_policy_factory.h" #include "src/core/ext/filters/client_channel/lb_policy_registry.h" #include "src/core/ext/filters/client_channel/resolver/xds/xds_resolver.h" -#include "src/core/ext/xds/xds_api.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/orphanable.h" @@ -109,17 +103,13 @@ class XdsClusterManagerLb : public LoadBalancingPolicy { // It is required that the keys of cluster_map have to live at least as long // as the ClusterPicker instance. - ClusterPicker(ClusterMap cluster_map, - RefCountedPtr config) - : cluster_map_(std::move(cluster_map)), config_(std::move(config)) {} + explicit ClusterPicker(ClusterMap cluster_map) + : cluster_map_(std::move(cluster_map)) {} PickResult Pick(PickArgs args) override; private: ClusterMap cluster_map_; - // Take a reference to config so that we can use - // XdsApi::RdsUpdate::RdsRoute::Matchers from it. - RefCountedPtr config_; }; // Each ClusterChild holds a ref to its parent XdsClusterManagerLb. @@ -368,8 +358,7 @@ void XdsClusterManagerLb::UpdateStateLocked() { Ref(DEBUG_LOCATION, "QueuePicker"))); } } - picker = - absl::make_unique(std::move(cluster_map), config_); + picker = absl::make_unique(std::move(cluster_map)); break; } case GRPC_CHANNEL_CONNECTING: