From 2c25cd7bcf655772647b5ed80fd2011e0708b3a4 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 23 Jan 2024 08:45:17 -0800 Subject: [PATCH 1/3] [cds LB] obtain dynamic subscription even if cluster is present in XdsConfig (#35627) This ensures that if a cluster is used both in the RouteConfig and via a ClusterSpecifierPlugin, and then the entry in the RouteConfig goes away, we won't unsubscribe and then resubscribe to the CDS resource. Closes #35627 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35627 from markdroth:xds_rls_dynamic_subscription_fix c78afaf6cefbab33a81f9a6e1a2937518a4226c8 PiperOrigin-RevId: 600801766 --- .../client_channel/lb_policy/xds/cds.cc | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc index 85ff24abd34ae..0d51f391db60d 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc @@ -294,6 +294,23 @@ absl::Status CdsLb::UpdateLocked(UpdateArgs args) { } else { GPR_ASSERT(cluster_name_ == new_config->cluster()); } + // Start dynamic subscription if needed. + if (new_config->is_dynamic() && subscription_ == nullptr) { + if (GRPC_TRACE_FLAG_ENABLED(grpc_cds_lb_trace)) { + gpr_log(GPR_INFO, + "[cdslb %p] obtaining dynamic subscription for cluster %s", this, + cluster_name_.c_str()); + } + auto* dependency_mgr = args.args.GetObject(); + if (dependency_mgr == nullptr) { + // Should never happen. + absl::Status status = + absl::InternalError("xDS dependency mgr not passed to CDS LB policy"); + ReportTransientFailure(status); + return status; + } + subscription_ = dependency_mgr->GetClusterSubscription(cluster_name_); + } // Get xDS config. auto new_xds_config = args.args.GetObjectRef(); if (new_xds_config == nullptr) { @@ -307,27 +324,13 @@ absl::Status CdsLb::UpdateLocked(UpdateArgs args) { if (it == new_xds_config->clusters.end()) { // Cluster not present. if (new_config->is_dynamic()) { - // This is a dynamic cluster. Subscribe to it if not yet subscribed. - if (subscription_ == nullptr) { - auto* dependency_mgr = args.args.GetObject(); - if (dependency_mgr == nullptr) { - // Should never happen. - absl::Status status = absl::InternalError( - "xDS dependency mgr not passed to CDS LB policy"); - ReportTransientFailure(status); - return status; - } - subscription_ = dependency_mgr->GetClusterSubscription(cluster_name_); - // Stay in CONNECTING until we get an update that has the cluster. - return absl::OkStatus(); - } // If we are already subscribed, it's possible that we just // recently subscribed but another update came through before we // got the new cluster, in which case it will still be missing. if (GRPC_TRACE_FLAG_ENABLED(grpc_cds_lb_trace)) { gpr_log(GPR_INFO, "[cdslb %p] xDS config has no entry for dynamic cluster %s, " - "ignoring update", + "waiting for subsequent update", this, cluster_name_.c_str()); } // Stay in CONNECTING until we get an update that has the cluster. From 1a20f21b6b75a16cd5521d5a12e2334fa4235491 Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Tue, 23 Jan 2024 09:18:06 -0800 Subject: [PATCH 2/3] Fixed multi-line comments warning (#35351) Adding more stuff on top of https://github.com/grpc/grpc/pull/35127 Closes #35351 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35351 from veblush:fix-line 1b25e5df492d9ad84dbbadd9c7406c90d38ab176 PiperOrigin-RevId: 600810979 --- src/core/lib/gpr/posix/sync.cc | 4 ++-- src/core/lib/gpr/windows/sync.cc | 4 ++-- src/core/lib/iomgr/grpc_if_nametoindex_posix.cc | 4 ++-- src/core/lib/iomgr/grpc_if_nametoindex_unsupported.cc | 4 ++-- test/core/security/credentials_test.cc | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/core/lib/gpr/posix/sync.cc b/src/core/lib/gpr/posix/sync.cc index c3cf035ba4d76..4e57b2c61812b 100644 --- a/src/core/lib/gpr/posix/sync.cc +++ b/src/core/lib/gpr/posix/sync.cc @@ -155,5 +155,5 @@ void gpr_once_init(gpr_once* once, void (*init_function)(void)) { GPR_ASSERT(pthread_once(once, init_function) == 0); } -#endif // defined(GPR_POSIX_SYNC) && !defined(GPR_ABSEIL_SYNC) && \ - // !defined(GPR_CUSTOM_SYNC) +#endif // defined(GPR_POSIX_SYNC) && !defined(GPR_ABSEIL_SYNC) && + // !defined(GPR_CUSTOM_SYNC) diff --git a/src/core/lib/gpr/windows/sync.cc b/src/core/lib/gpr/windows/sync.cc index f06c73efb040e..2fb6748f89b90 100644 --- a/src/core/lib/gpr/windows/sync.cc +++ b/src/core/lib/gpr/windows/sync.cc @@ -118,5 +118,5 @@ void gpr_once_init(gpr_once* once, void (*init_function)(void)) { InitOnceExecuteOnce(once, run_once_func, &arg, &phony); } -#endif // defined(GPR_WINDOWS) && !defined(GPR_ABSEIL_SYNC) && \ - // !defined(GPR_CUSTOM_SYNC) +#endif // defined(GPR_WINDOWS) && !defined(GPR_ABSEIL_SYNC) && + // !defined(GPR_CUSTOM_SYNC) diff --git a/src/core/lib/iomgr/grpc_if_nametoindex_posix.cc b/src/core/lib/iomgr/grpc_if_nametoindex_posix.cc index fe1ada035928c..2b3bc5177d185 100644 --- a/src/core/lib/iomgr/grpc_if_nametoindex_posix.cc +++ b/src/core/lib/iomgr/grpc_if_nametoindex_posix.cc @@ -39,5 +39,5 @@ uint32_t grpc_if_nametoindex(char* name) { return out; } -#endif // GRPC_IF_NAMETOINDEX == 1 && \ - // defined(GRPC_POSIX_SOCKET_IF_NAMETOINDEX) +#endif // GRPC_IF_NAMETOINDEX == 1 && + // defined(GRPC_POSIX_SOCKET_IF_NAMETOINDEX) diff --git a/src/core/lib/iomgr/grpc_if_nametoindex_unsupported.cc b/src/core/lib/iomgr/grpc_if_nametoindex_unsupported.cc index 5555cad6d1b9d..317fb56d058d1 100644 --- a/src/core/lib/iomgr/grpc_if_nametoindex_unsupported.cc +++ b/src/core/lib/iomgr/grpc_if_nametoindex_unsupported.cc @@ -35,5 +35,5 @@ uint32_t grpc_if_nametoindex(char* name) { return 0; } -#endif // GRPC_IF_NAMETOINDEX == 0 || \ - // !defined(GRPC_POSIX_SOCKET_IF_NAMETOINDEX) +#endif // GRPC_IF_NAMETOINDEX == 0 || + // !defined(GRPC_POSIX_SOCKET_IF_NAMETOINDEX) diff --git a/test/core/security/credentials_test.cc b/test/core/security/credentials_test.cc index ad58ccd394f67..855ede9859de8 100644 --- a/test/core/security/credentials_test.cc +++ b/test/core/security/credentials_test.cc @@ -1944,8 +1944,8 @@ TEST(CredentialsTest, TestGetWellKnownGoogleCredentialsFilePath) { // so we set it to some fake value restore_home_env = true; SetEnv("HOME", "/fake/home/for/bazel"); -#endif // defined(GRPC_BAZEL_BUILD) && (defined(GPR_POSIX_ENV) || \ - // defined(GPR_LINUX_ENV)) +#endif // defined(GRPC_BAZEL_BUILD) && (defined(GPR_POSIX_ENV) || + // defined(GPR_LINUX_ENV)) std::string path = grpc_get_well_known_google_credentials_file_path(); GPR_ASSERT(!path.empty()); #if defined(GPR_POSIX_ENV) || defined(GPR_LINUX_ENV) From 9c9405903665d8ed2c31f854ddb72cbed3a1f315 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 23 Jan 2024 10:06:28 -0800 Subject: [PATCH 3/3] [handshaker API] remove some unused legacy cruft (#35628) Closes #35628 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35628 from markdroth:handshaker_cruft_cleanup 8fad55cf42ca9ddaf16a727c037789900403a1ba PiperOrigin-RevId: 600826375 --- src/core/lib/security/transport/security_handshaker.cc | 8 -------- src/core/lib/security/transport/security_handshaker.h | 6 ------ src/core/lib/transport/handshaker.cc | 8 -------- src/core/lib/transport/handshaker.h | 7 ------- 4 files changed, 29 deletions(-) diff --git a/src/core/lib/security/transport/security_handshaker.cc b/src/core/lib/security/transport/security_handshaker.cc index 20784dcf3be31..885b35fc9d25e 100644 --- a/src/core/lib/security/transport/security_handshaker.cc +++ b/src/core/lib/security/transport/security_handshaker.cc @@ -671,11 +671,3 @@ void SecurityRegisterHandshakerFactories(CoreConfiguration::Builder* builder) { } } // namespace grpc_core - -grpc_handshaker* grpc_security_handshaker_create( - tsi_handshaker* handshaker, grpc_security_connector* connector, - const grpc_channel_args* args) { - return SecurityHandshakerCreate(handshaker, connector, - grpc_core::ChannelArgs::FromC(args)) - .release(); -} diff --git a/src/core/lib/security/transport/security_handshaker.h b/src/core/lib/security/transport/security_handshaker.h index 659df96a5b2af..f6f5d42a4621f 100644 --- a/src/core/lib/security/transport/security_handshaker.h +++ b/src/core/lib/security/transport/security_handshaker.h @@ -42,10 +42,4 @@ void SecurityRegisterHandshakerFactories(CoreConfiguration::Builder*); } // namespace grpc_core -// TODO(arjunroy): This is transitional to account for the new handshaker API -// and will eventually be removed entirely. -grpc_handshaker* grpc_security_handshaker_create( - tsi_handshaker* handshaker, grpc_security_connector* connector, - const grpc_channel_args* args); - #endif // GRPC_SRC_CORE_LIB_SECURITY_TRANSPORT_SECURITY_HANDSHAKER_H diff --git a/src/core/lib/transport/handshaker.cc b/src/core/lib/transport/handshaker.cc index ccf55608a5534..9f459c5deb01a 100644 --- a/src/core/lib/transport/handshaker.cc +++ b/src/core/lib/transport/handshaker.cc @@ -227,11 +227,3 @@ void HandshakeManager::DoHandshake(grpc_endpoint* endpoint, } } // namespace grpc_core - -void grpc_handshake_manager_add(grpc_handshake_manager* mgr, - grpc_handshaker* handshaker) { - // This is a transition method to aid the API change for handshakers. - grpc_core::RefCountedPtr refd_hs( - static_cast(handshaker)); - mgr->Add(refd_hs); -} diff --git a/src/core/lib/transport/handshaker.h b/src/core/lib/transport/handshaker.h index a01adc626853b..7d8b66da361b3 100644 --- a/src/core/lib/transport/handshaker.h +++ b/src/core/lib/transport/handshaker.h @@ -162,11 +162,4 @@ class HandshakeManager : public RefCounted { } // namespace grpc_core -// TODO(arjunroy): These are transitional to account for the new handshaker API -// and will eventually be removed entirely. -typedef grpc_core::HandshakeManager grpc_handshake_manager; -typedef grpc_core::Handshaker grpc_handshaker; -void grpc_handshake_manager_add(grpc_handshake_manager* mgr, - grpc_handshaker* handshaker); - #endif // GRPC_SRC_CORE_LIB_TRANSPORT_HANDSHAKER_H