diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 5c36c24f156..3fefae1797b 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -31,6 +31,7 @@ for general contribution guidelines. - [pfreixes](https://github.com/pfreixes), Skyscanner Ltd - [ran-su](https://github.com/ran-su), Google LLC - [sanjaypujare](https://github.com/sanjaypujare), Google LLC +- [sastryvp](https://github.com/sastryvp), Google LLC - [sergiitk](https://github.com/sergiitk), Google LLC - [soheilhy](https://github.com/soheilhy), Google LLC - [stanley-cheung](https://github.com/stanley-cheung), Google LLC diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl index e3aeb05464d..7b41b29566a 100644 --- a/bazel/experiments.bzl +++ b/bazel/experiments.bzl @@ -28,6 +28,7 @@ EXPERIMENT_ENABLES = { "http2_stats_fix": "http2_stats_fix", "keepalive_fix": "keepalive_fix", "keepalive_server_fix": "keepalive_server_fix", + "max_pings_wo_data_throttle": "max_pings_wo_data_throttle", "monitoring_experiment": "monitoring_experiment", "multiping": "multiping", "peer_state_based_framing": "peer_state_based_framing", @@ -57,10 +58,16 @@ EXPERIMENTS = { "dbg": { }, "off": { + "core_end2end_test": [ + "event_engine_client", + ], "endpoint_test": [ "tcp_frame_size_tuning", "tcp_rcv_lowat", ], + "event_engine_client_test": [ + "event_engine_client", + ], "flow_control_test": [ "multiping", "peer_state_based_framing", @@ -78,15 +85,11 @@ EXPERIMENTS = { "event_engine_dns", ], "core_end2end_test": [ - "event_engine_client", "event_engine_listener", ], "cpp_lb_end2end_test": [ "pick_first_new", ], - "event_engine_client_test": [ - "event_engine_client", - ], "event_engine_listener_test": [ "event_engine_listener", ], diff --git a/bazel/grpc_deps.bzl b/bazel/grpc_deps.bzl index d0099ab6163..f3920abb2ef 100644 --- a/bazel/grpc_deps.bzl +++ b/bazel/grpc_deps.bzl @@ -406,10 +406,10 @@ def grpc_deps(): if "bazel_gazelle" not in native.existing_rules(): http_archive( name = "bazel_gazelle", - sha256 = "de69a09dc70417580aabf20a28619bb3ef60d038470c7cf8442fafcf627c21cb", + sha256 = "d76bf7a60fd8b050444090dfa2837a4eaf9829e1165618ee35dceca5cbdf58d5", urls = [ - "https://mirror.bazel.build/github.com/bazelbuild/bazel-gazelle/releases/download/v0.24.0/bazel-gazelle-v0.24.0.tar.gz", - "https://github.com/bazelbuild/bazel-gazelle/releases/download/v0.24.0/bazel-gazelle-v0.24.0.tar.gz", + "https://mirror.bazel.build/github.com/bazelbuild/bazel-gazelle/releases/download/v0.37.0/bazel-gazelle-v0.37.0.tar.gz", + "https://github.com/bazelbuild/bazel-gazelle/releases/download/v0.37.0/bazel-gazelle-v0.37.0.tar.gz", ], ) diff --git a/bazel/grpc_extra_deps.bzl b/bazel/grpc_extra_deps.bzl index eb3c3fc29c5..a370fe44966 100644 --- a/bazel/grpc_extra_deps.bzl +++ b/bazel/grpc_extra_deps.bzl @@ -52,7 +52,7 @@ def grpc_extra_deps(ignore_version_differences = False): api_dependencies() go_rules_dependencies() - go_register_toolchains(version = "1.18") + go_register_toolchains(version = "1.20") gazelle_dependencies() # Pull-in the go 3rd party dependencies for protoc_gen_validate, which is diff --git a/doc/grpc_xds_features.md b/doc/grpc_xds_features.md index b6f33ea59fe..16ac26214bd 100644 --- a/doc/grpc_xds_features.md +++ b/doc/grpc_xds_features.md @@ -77,5 +77,5 @@ Aggregate Cluster Behavior Fixes | [A75](https://github.com/grpc/proposal/blob/m Pick First | [A62](https://github.com/grpc/proposal/blob/master/A62-pick-first.md) | v1.58.0 | v1.58.1 | v1.56.0 | | [StringMatcher for Header Matching](https://github.com/envoyproxy/envoy/blob/3fe4b8d335fa339ef6f17325c8d31f87ade7bb1a/api/envoy/config/route/v3/route_components.proto#L2280) | [A63](https://github.com/grpc/proposal/blob/master/A63-xds-string-matcher-in-header-matching.md) | v1.56.0 | v1.53.0 | v1.56.0 | v1.9.0 | LRS Custom Metrics Support | [A64](https://github.com/grpc/proposal/blob/master/A64-lrs-custom-metrics.md) | v1.54.0 | | | | -mTLS Credentials in xDS Bootstrap File | [A65](https://github.com/grpc/proposal/blob/master/A65-xds-mtls-creds-in-bootstrap.md) | v1.57.0 | | v1.61.0 | | +mTLS Credentials in xDS Bootstrap File | [A65](https://github.com/grpc/proposal/blob/master/A65-xds-mtls-creds-in-bootstrap.md) | v1.65.0 | | v1.61.0 | | Stateful Session Affinity | [A55](https://github.com/grpc/proposal/blob/master/A55-xds-stateful-session-affinity.md), [A60](https://github.com/grpc/proposal/blob/master/A60-xds-stateful-session-affinity-weighted-clusters.md), [A75](https://github.com/grpc/proposal/blob/master/A75-xds-aggregate-cluster-behavior-fixes.md) | v1.61.0 | | | | diff --git a/include/grpc/impl/channel_arg_names.h b/include/grpc/impl/channel_arg_names.h index 663e9620a34..efd380b8681 100644 --- a/include/grpc/impl/channel_arg_names.h +++ b/include/grpc/impl/channel_arg_names.h @@ -111,9 +111,11 @@ "grpc.server_max_unrequested_time_in_server" /** Channel arg to override the http2 :scheme header */ #define GRPC_ARG_HTTP2_SCHEME "grpc.http2_scheme" -/** How many pings can the client send before needing to send a - data/header frame? (0 indicates that an infinite number of - pings can be sent without sending a data frame or header frame) */ +/** How many pings can the client send before needing to send a data/header + frame? (0 indicates that an infinite number of pings can be sent without + sending a data frame or header frame). + If experiment "max_pings_wo_data_throttle" is enabled, instead of pings being + completely blocked, they are throttled. */ #define GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA \ "grpc.http2.max_pings_without_data" /** How many misbehaving pings the server can bear before sending goaway and diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.cc b/src/core/ext/transport/chttp2/server/chttp2_server.cc index 3388e234fd9..77645c06871 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.cc +++ b/src/core/ext/transport/chttp2/server/chttp2_server.cc @@ -474,23 +474,18 @@ void Chttp2ServerListener::ActiveConnection::HandshakingState::OnHandshakeDone( // handshaker may have handed off the connection to some external // code, so we can just clean up here without creating a transport. if (args->endpoint != nullptr) { - Transport* transport = - grpc_create_chttp2_transport(args->args, args->endpoint, false); + RefCountedPtr transport = + grpc_create_chttp2_transport(args->args, args->endpoint, false) + ->Ref(); grpc_error_handle channel_init_err = self->connection_->listener_->server_->SetupTransport( - transport, self->accepting_pollset_, args->args, - grpc_chttp2_transport_get_socket_node(transport)); + transport.get(), self->accepting_pollset_, args->args, + grpc_chttp2_transport_get_socket_node(transport.get())); if (channel_init_err.ok()) { // Use notify_on_receive_settings callback to enforce the // handshake deadline. - // Note: The reinterpret_cast<>s here are safe, because - // grpc_chttp2_transport is a C-style extension of - // Transport, so this is morally equivalent of a - // static_cast<> to a derived class. - // TODO(roth): Change to static_cast<> when we C++-ify the - // transport API. self->connection_->transport_ = - reinterpret_cast(transport)->Ref(); + DownCast(transport.get())->Ref(); self->Ref().release(); // Held by OnReceiveSettings(). GRPC_CLOSURE_INIT(&self->on_receive_settings_, OnReceiveSettings, self, grpc_schedule_on_exec_ctx); @@ -520,9 +515,9 @@ void Chttp2ServerListener::ActiveConnection::HandshakingState::OnHandshakeDone( grpc_schedule_on_exec_ctx_); cleanup_connection = true; } - grpc_chttp2_transport_start_reading(transport, args->read_buffer, - &self->on_receive_settings_, - on_close); + grpc_chttp2_transport_start_reading( + transport.get(), args->read_buffer, &self->on_receive_settings_, + on_close); self->timer_handle_ = self->connection_->event_engine_->RunAfter( self->deadline_ - Timestamp::Now(), [self = self->Ref()]() mutable { diff --git a/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc b/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc index fed8ca20371..143b08a67c7 100644 --- a/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc +++ b/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc @@ -35,16 +35,18 @@ namespace grpc_core { namespace { -int g_default_max_pings_without_data = 2; +int g_default_max_pings_without_data_sent = 2; +constexpr Duration kThrottleIntervalWithoutDataSent = Duration::Minutes(1); absl::optional g_default_max_inflight_pings; } // namespace Chttp2PingRatePolicy::Chttp2PingRatePolicy(const ChannelArgs& args, bool is_client) - : max_pings_without_data_( + : max_pings_without_data_sent_( is_client - ? std::max(0, args.GetInt(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA) - .value_or(g_default_max_pings_without_data)) + ? std::max(0, + args.GetInt(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA) + .value_or(g_default_max_pings_without_data_sent)) : 0), // Configuration via channel arg dominates, otherwise if the multiping // experiment is enabled we use 100, otherwise 1. @@ -54,18 +56,15 @@ Chttp2PingRatePolicy::Chttp2PingRatePolicy(const ChannelArgs& args, IsMultipingEnabled() ? 100 : 1)))) {} void Chttp2PingRatePolicy::SetDefaults(const ChannelArgs& args) { - g_default_max_pings_without_data = + g_default_max_pings_without_data_sent = std::max(0, args.GetInt(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA) - .value_or(g_default_max_pings_without_data)); + .value_or(g_default_max_pings_without_data_sent)); g_default_max_inflight_pings = args.GetInt(GRPC_ARG_HTTP2_MAX_INFLIGHT_PINGS); } Chttp2PingRatePolicy::RequestSendPingResult Chttp2PingRatePolicy::RequestSendPing(Duration next_allowed_ping_interval, size_t inflight_pings) const { - if (max_pings_without_data_ != 0 && pings_before_data_required_ == 0) { - return TooManyRecentPings{}; - } if (max_inflight_pings_ > 0 && inflight_pings > static_cast(max_inflight_pings_)) { return TooManyRecentPings{}; @@ -77,12 +76,29 @@ Chttp2PingRatePolicy::RequestSendPing(Duration next_allowed_ping_interval, return TooSoon{next_allowed_ping_interval, last_ping_sent_time_, next_allowed_ping - now}; } + // Throttle pings to 1 minute if we haven't sent any data recently + if (max_pings_without_data_sent_ != 0 && + pings_before_data_sending_required_ == 0) { + if (IsMaxPingsWoDataThrottleEnabled()) { + const Timestamp next_allowed_ping = + last_ping_sent_time_ + kThrottleIntervalWithoutDataSent; + if (next_allowed_ping > now) { + return TooSoon{kThrottleIntervalWithoutDataSent, last_ping_sent_time_, + next_allowed_ping - now}; + } + } else { + return TooManyRecentPings{}; + } + } + return SendGranted{}; } void Chttp2PingRatePolicy::SentPing() { last_ping_sent_time_ = Timestamp::Now(); - if (pings_before_data_required_) --pings_before_data_required_; + if (pings_before_data_sending_required_ > 0) { + --pings_before_data_sending_required_; + } } void Chttp2PingRatePolicy::ReceivedDataFrame() { @@ -90,13 +106,13 @@ void Chttp2PingRatePolicy::ReceivedDataFrame() { } void Chttp2PingRatePolicy::ResetPingsBeforeDataRequired() { - pings_before_data_required_ = max_pings_without_data_; + pings_before_data_sending_required_ = max_pings_without_data_sent_; } std::string Chttp2PingRatePolicy::GetDebugString() const { return absl::StrCat( - "max_pings_without_data: ", max_pings_without_data_, - ", pings_before_data_required: ", pings_before_data_required_, + "max_pings_without_data: ", max_pings_without_data_sent_, + ", pings_before_data_required: ", pings_before_data_sending_required_, ", last_ping_sent_time_: ", last_ping_sent_time_.ToString()); } diff --git a/src/core/ext/transport/chttp2/transport/ping_rate_policy.h b/src/core/ext/transport/chttp2/transport/ping_rate_policy.h index b9501686d95..6dab7a361bc 100644 --- a/src/core/ext/transport/chttp2/transport/ping_rate_policy.h +++ b/src/core/ext/transport/chttp2/transport/ping_rate_policy.h @@ -70,13 +70,14 @@ class Chttp2PingRatePolicy { void ReceivedDataFrame(); std::string GetDebugString() const; - int TestOnlyMaxPingsWithoutData() const { return max_pings_without_data_; } + int TestOnlyMaxPingsWithoutData() const { + return max_pings_without_data_sent_; + } private: - const int max_pings_without_data_; + const int max_pings_without_data_sent_; const int max_inflight_pings_; - // No pings allowed before receiving a header or data frame. - int pings_before_data_required_ = 0; + int pings_before_data_sending_required_ = 0; Timestamp last_ping_sent_time_ = Timestamp::InfPast(); }; diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc index 468884ec583..b4ba5bdba89 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -59,6 +59,11 @@ const char* const description_keepalive_server_fix = "Allows overriding keepalive_permit_without_calls for servers. Refer " "https://github.com/grpc/grpc/pull/33917 for more information."; const char* const additional_constraints_keepalive_server_fix = "{}"; +const char* const description_max_pings_wo_data_throttle = + "Experiment to throttle pings to a period of 1 min when " + "GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of " + "completely blocking)."; +const char* const additional_constraints_max_pings_wo_data_throttle = "{}"; const char* const description_monitoring_experiment = "Placeholder experiment to prove/disprove our monitoring is working"; const char* const additional_constraints_monitoring_experiment = "{}"; @@ -157,6 +162,9 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_keepalive_fix, nullptr, 0, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, additional_constraints_keepalive_server_fix, nullptr, 0, false, false}, + {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle, + additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false, + true}, {"monitoring_experiment", description_monitoring_experiment, additional_constraints_monitoring_experiment, nullptr, 0, true, true}, {"multiping", description_multiping, additional_constraints_multiping, @@ -241,6 +249,11 @@ const char* const description_keepalive_server_fix = "Allows overriding keepalive_permit_without_calls for servers. Refer " "https://github.com/grpc/grpc/pull/33917 for more information."; const char* const additional_constraints_keepalive_server_fix = "{}"; +const char* const description_max_pings_wo_data_throttle = + "Experiment to throttle pings to a period of 1 min when " + "GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of " + "completely blocking)."; +const char* const additional_constraints_max_pings_wo_data_throttle = "{}"; const char* const description_monitoring_experiment = "Placeholder experiment to prove/disprove our monitoring is working"; const char* const additional_constraints_monitoring_experiment = "{}"; @@ -326,7 +339,7 @@ const ExperimentMetadata g_experiment_metadata[] = { {"client_privacy", description_client_privacy, additional_constraints_client_privacy, nullptr, 0, false, false}, {"event_engine_client", description_event_engine_client, - additional_constraints_event_engine_client, nullptr, 0, true, true}, + additional_constraints_event_engine_client, nullptr, 0, false, true}, {"event_engine_dns", description_event_engine_dns, additional_constraints_event_engine_dns, nullptr, 0, true, false}, {"event_engine_listener", description_event_engine_listener, @@ -339,6 +352,9 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_keepalive_fix, nullptr, 0, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, additional_constraints_keepalive_server_fix, nullptr, 0, false, false}, + {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle, + additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false, + true}, {"monitoring_experiment", description_monitoring_experiment, additional_constraints_monitoring_experiment, nullptr, 0, true, true}, {"multiping", description_multiping, additional_constraints_multiping, @@ -423,6 +439,11 @@ const char* const description_keepalive_server_fix = "Allows overriding keepalive_permit_without_calls for servers. Refer " "https://github.com/grpc/grpc/pull/33917 for more information."; const char* const additional_constraints_keepalive_server_fix = "{}"; +const char* const description_max_pings_wo_data_throttle = + "Experiment to throttle pings to a period of 1 min when " + "GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of " + "completely blocking)."; +const char* const additional_constraints_max_pings_wo_data_throttle = "{}"; const char* const description_monitoring_experiment = "Placeholder experiment to prove/disprove our monitoring is working"; const char* const additional_constraints_monitoring_experiment = "{}"; @@ -521,6 +542,9 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_keepalive_fix, nullptr, 0, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, additional_constraints_keepalive_server_fix, nullptr, 0, false, false}, + {"max_pings_wo_data_throttle", description_max_pings_wo_data_throttle, + additional_constraints_max_pings_wo_data_throttle, nullptr, 0, false, + true}, {"monitoring_experiment", description_monitoring_experiment, additional_constraints_monitoring_experiment, nullptr, 0, true, true}, {"multiping", description_multiping, additional_constraints_multiping, diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index 9e3ba9148ba..50dc88d8269 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -70,6 +70,7 @@ inline bool IsFreeLargeAllocatorEnabled() { return false; } inline bool IsHttp2StatsFixEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } +inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } inline bool IsMultipingEnabled() { return false; } @@ -97,8 +98,7 @@ inline bool IsCallStatusOverrideOnCancellationEnabled() { return true; } inline bool IsCallV3Enabled() { return false; } inline bool IsCanaryClientPrivacyEnabled() { return false; } inline bool IsClientPrivacyEnabled() { return false; } -#define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_CLIENT -inline bool IsEventEngineClientEnabled() { return true; } +inline bool IsEventEngineClientEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_DNS inline bool IsEventEngineDnsEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_LISTENER @@ -108,6 +108,7 @@ inline bool IsFreeLargeAllocatorEnabled() { return false; } inline bool IsHttp2StatsFixEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } +inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } inline bool IsMultipingEnabled() { return false; } @@ -145,6 +146,7 @@ inline bool IsFreeLargeAllocatorEnabled() { return false; } inline bool IsHttp2StatsFixEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } +inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } inline bool IsMultipingEnabled() { return false; } @@ -181,6 +183,7 @@ enum ExperimentIds { kExperimentIdHttp2StatsFix, kExperimentIdKeepaliveFix, kExperimentIdKeepaliveServerFix, + kExperimentIdMaxPingsWoDataThrottle, kExperimentIdMonitoringExperiment, kExperimentIdMultiping, kExperimentIdPeerStateBasedFraming, @@ -243,6 +246,10 @@ inline bool IsKeepaliveFixEnabled() { inline bool IsKeepaliveServerFixEnabled() { return IsExperimentEnabled(kExperimentIdKeepaliveServerFix); } +#define GRPC_EXPERIMENT_IS_INCLUDED_MAX_PINGS_WO_DATA_THROTTLE +inline bool IsMaxPingsWoDataThrottleEnabled() { + return IsExperimentEnabled(kExperimentIdMaxPingsWoDataThrottle); +} #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return IsExperimentEnabled(kExperimentIdMonitoringExperiment); diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index 4d1a27e4d3c..f26fc02f27e 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -120,6 +120,14 @@ owner: yashkt@google.com test_tags: [] allow_in_fuzzing_config: false +- name: max_pings_wo_data_throttle + description: + Experiment to throttle pings to a period of 1 min when + GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA limit has reached (instead of + completely blocking). + expiry: 2024/12/31 + owner: yashkt@google.com + test_tags: [] - name: monitoring_experiment description: Placeholder experiment to prove/disprove our monitoring is working expiry: never-ever diff --git a/src/core/lib/experiments/rollouts.yaml b/src/core/lib/experiments/rollouts.yaml index 7187f8c2952..fe679a5718e 100644 --- a/src/core/lib/experiments/rollouts.yaml +++ b/src/core/lib/experiments/rollouts.yaml @@ -58,7 +58,7 @@ # not tested on iOS at all ios: broken posix: false - windows: true + windows: false - name: event_engine_dns default: # not tested on iOS at all @@ -79,6 +79,8 @@ default: false - name: keepalive_server_fix default: false +- name: max_pings_wo_data_throttle + default: false - name: monitoring_experiment default: true - name: peer_state_based_framing diff --git a/src/core/lib/security/credentials/channel_creds_registry_init.cc b/src/core/lib/security/credentials/channel_creds_registry_init.cc index 9b7a0e04372..832daba1284 100644 --- a/src/core/lib/security/credentials/channel_creds_registry_init.cc +++ b/src/core/lib/security/credentials/channel_creds_registry_init.cc @@ -96,6 +96,8 @@ class TlsChannelCredsFactory : public ChannelCredsFactory<> { } options->set_watch_root_cert(!config->ca_certificate_file().empty()); options->set_watch_identity_pair(!config->certificate_file().empty()); + options->set_certificate_verifier( + MakeRefCounted()); return MakeRefCounted(std::move(options)); } diff --git a/test/core/end2end/tests/bad_ping.cc b/test/core/end2end/tests/bad_ping.cc index 1aa247e46e3..51a799549b2 100644 --- a/test/core/end2end/tests/bad_ping.cc +++ b/test/core/end2end/tests/bad_ping.cc @@ -87,6 +87,9 @@ CORE_END2END_TEST(RetryHttp2Test, BadPing) { // Try sending more pings than server allows, but server should be fine because // max_pings_without_data should limit pings sent out on wire. CORE_END2END_TEST(RetryHttp2Test, PingsWithoutData) { + if (IsMaxPingsWoDataThrottleEnabled()) { + GTEST_SKIP() << "pings are not limited if this experiment is enabled"; + } // Only allow MAX_PING_STRIKES pings without data (DATA/HEADERS/WINDOW_UPDATE) // so that the transport will throttle the excess pings. InitClient(ChannelArgs() diff --git a/test/core/transport/chttp2/ping_rate_policy_test.cc b/test/core/transport/chttp2/ping_rate_policy_test.cc index be443468bc9..46ddd0501fd 100644 --- a/test/core/transport/chttp2/ping_rate_policy_test.cc +++ b/test/core/transport/chttp2/ping_rate_policy_test.cc @@ -19,6 +19,8 @@ #include "gtest/gtest.h" +#include "src/core/lib/experiments/experiments.h" + namespace grpc_core { namespace { @@ -47,6 +49,10 @@ TEST(PingRatePolicy, ServerCanSendAtStart) { } TEST(PingRatePolicy, ClientBlockedUntilDataSent) { + if (IsMaxPingsWoDataThrottleEnabled()) { + GTEST_SKIP() + << "Pings are not blocked if max_pings_wo_data_throttle is enabled."; + } Chttp2PingRatePolicy policy{ChannelArgs(), true}; EXPECT_EQ(policy.RequestSendPing(Duration::Milliseconds(10), 0), TooManyRecentPings()); @@ -59,6 +65,35 @@ TEST(PingRatePolicy, ClientBlockedUntilDataSent) { EXPECT_EQ(policy.RequestSendPing(Duration::Zero(), 0), TooManyRecentPings()); } +TEST(PingRatePolicy, ClientThrottledUntilDataSent) { + if (!IsMaxPingsWoDataThrottleEnabled()) { + GTEST_SKIP() + << "Throttling behavior is enabled with max_pings_wo_data_throttle."; + } + Chttp2PingRatePolicy policy{ChannelArgs(), true}; + // First ping is allowed. + EXPECT_EQ(policy.RequestSendPing(Duration::Milliseconds(10), 0), + SendGranted()); + policy.SentPing(); + // Second ping is throttled since no data has been sent. + auto result = policy.RequestSendPing(Duration::Zero(), 0); + EXPECT_TRUE(absl::holds_alternative(result)); + EXPECT_EQ(absl::get(result).wait, + Duration::Minutes(1)); + policy.ResetPingsBeforeDataRequired(); + // After resetting pings before data required (data sent), we can send pings + // without being throttled. + EXPECT_EQ(policy.RequestSendPing(Duration::Zero(), 0), SendGranted()); + policy.SentPing(); + EXPECT_EQ(policy.RequestSendPing(Duration::Zero(), 0), SendGranted()); + policy.SentPing(); + // After reaching limit, we are throttled again. + result = policy.RequestSendPing(Duration::Zero(), 0); + EXPECT_TRUE(absl::holds_alternative(result)); + EXPECT_EQ(absl::get(result).wait, + Duration::Minutes(1)); +} + TEST(PingRatePolicy, RateThrottlingWorks) { Chttp2PingRatePolicy policy{ChannelArgs(), false}; // Observe that we can fail if we send in a tight loop diff --git a/test/cpp/end2end/xds/BUILD b/test/cpp/end2end/xds/BUILD index 15afd38eb26..2a66bca252c 100644 --- a/test/cpp/end2end/xds/BUILD +++ b/test/cpp/end2end/xds/BUILD @@ -216,6 +216,11 @@ grpc_cc_test( name = "xds_core_end2end_test", size = "large", srcs = ["xds_core_end2end_test.cc"], + data = [ + "//src/core/tsi/test_creds:ca.pem", + "//src/core/tsi/test_creds:server1.key", + "//src/core/tsi/test_creds:server1.pem", + ], external_deps = [ "gtest", ], diff --git a/test/cpp/end2end/xds/xds_core_end2end_test.cc b/test/cpp/end2end/xds/xds_core_end2end_test.cc index abc1d20cf06..6b8fcd43482 100644 --- a/test/cpp/end2end/xds/xds_core_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_core_end2end_test.cc @@ -160,6 +160,34 @@ TEST_P(XdsClientTest, XdsStreamErrorPropagation) { ::testing::HasSubstr("(node ID:xds_end2end_test)")); } +// +// XdsServerTlsTest: xDS server using TlsCreds +// + +class XdsServerTlsTest : public XdsEnd2endTest { + protected: + void SetUp() override { + InitClient(MakeBootstrapBuilder().SetXdsChannelCredentials( + "tls", absl::StrCat("{\"ca_certificate_file\": \"", + kCaCertPath, "\"}")), + /*lb_expected_authority=*/"", + /*xds_resource_does_not_exist_timeout_ms=*/0, + /*balancer_authority_override=*/"foo.test.google.fr"); + } +}; + +INSTANTIATE_TEST_SUITE_P( + XdsTest, XdsServerTlsTest, + ::testing::Values(XdsTestType().set_xds_server_uses_tls_creds(true)), + &XdsTestType::Name); + +TEST_P(XdsServerTlsTest, Basic) { + CreateAndStartBackends(1); + EdsResourceArgs args({{"locality0", CreateEndpointsForBackends()}}); + balancer_->ads_service()->SetEdsResource(BuildEdsResource(args)); + CheckRpcSendOk(DEBUG_LOCATION); +} + // // GlobalXdsClientTest - tests that need to run with a global XdsClient // (this is the default in production) diff --git a/test/cpp/end2end/xds/xds_end2end_test_lib.cc b/test/cpp/end2end/xds/xds_end2end_test_lib.cc index 08d305c2a8a..f9e884650fe 100644 --- a/test/cpp/end2end/xds/xds_end2end_test_lib.cc +++ b/test/cpp/end2end/xds/xds_end2end_test_lib.cc @@ -318,6 +318,26 @@ XdsEnd2endTest::BalancerServerThread::BalancerServerThread( }, debug_label)) {} +std::shared_ptr +XdsEnd2endTest::BalancerServerThread::Credentials() { + if (GetParam().xds_server_uses_tls_creds()) { + std::string identity_cert = + grpc_core::testing::GetFileContents(kServerCertPath); + std::string private_key = + grpc_core::testing::GetFileContents(kServerKeyPath); + std::vector identity_key_cert_pairs = { + {private_key, identity_cert}}; + auto certificate_provider = + std::make_shared( + identity_key_cert_pairs); + grpc::experimental::TlsServerCredentialsOptions options( + certificate_provider); + options.watch_identity_key_cert_pairs(); + return grpc::experimental::TlsServerCredentials(options); + } + return ServerThread::Credentials(); +} + void XdsEnd2endTest::BalancerServerThread::RegisterAllServices( ServerBuilder* builder) { builder->RegisterService(ads_service_.get()); @@ -490,7 +510,8 @@ std::vector XdsEnd2endTest::GetBackendPorts(size_t start_index, void XdsEnd2endTest::InitClient(absl::optional builder, std::string lb_expected_authority, - int xds_resource_does_not_exist_timeout_ms) { + int xds_resource_does_not_exist_timeout_ms, + std::string balancer_authority_override) { if (!builder.has_value()) { builder = MakeBootstrapBuilder(); } @@ -509,6 +530,11 @@ void XdsEnd2endTest::InitClient(absl::optional builder, const_cast(GRPC_ARG_FAKE_SECURITY_EXPECTED_TARGETS), const_cast(lb_expected_authority.c_str()))); } + if (!balancer_authority_override.empty()) { + xds_channel_args_to_add_.emplace_back(grpc_channel_arg_string_create( + const_cast(GRPC_ARG_DEFAULT_AUTHORITY), + const_cast(balancer_authority_override.c_str()))); + } xds_channel_args_.num_args = xds_channel_args_to_add_.size(); xds_channel_args_.args = xds_channel_args_to_add_.data(); bootstrap_ = builder->Build(); diff --git a/test/cpp/end2end/xds/xds_end2end_test_lib.h b/test/cpp/end2end/xds/xds_end2end_test_lib.h index 7ac0474a9f4..c16aa278d1b 100644 --- a/test/cpp/end2end/xds/xds_end2end_test_lib.h +++ b/test/cpp/end2end/xds/xds_end2end_test_lib.h @@ -115,6 +115,11 @@ class XdsTestType { return *this; } + XdsTestType& set_xds_server_uses_tls_creds(bool value) { + xds_server_uses_tls_creds_ = value; + return *this; + } + bool enable_load_reporting() const { return enable_load_reporting_; } bool enable_rds_testing() const { return enable_rds_testing_; } bool use_xds_credentials() const { return use_xds_credentials_; } @@ -130,6 +135,7 @@ class XdsTestType { rbac_audit_condition() const { return rbac_audit_condition_; } + bool xds_server_uses_tls_creds() const { return xds_server_uses_tls_creds_; } std::string AsString() const { std::string retval = "V3"; @@ -158,6 +164,9 @@ class XdsTestType { RBAC_AuditLoggingOptions_AuditCondition_Name( rbac_audit_condition_)); } + if (xds_server_uses_tls_creds_) { + retval += "XdsServerUsesTlsCreds"; + } return retval; } @@ -178,6 +187,7 @@ class XdsTestType { ::envoy::config::rbac::v3::RBAC_AuditLoggingOptions_AuditCondition rbac_audit_condition_ = ::envoy::config::rbac::v3:: RBAC_AuditLoggingOptions_AuditCondition_NONE; + bool xds_server_uses_tls_creds_ = false; }; // A base class for xDS end-to-end tests. @@ -412,6 +422,11 @@ class XdsEnd2endTest : public ::testing::TestWithParam, AdsServiceImpl* ads_service() { return ads_service_.get(); } LrsServiceImpl* lrs_service() { return lrs_service_.get(); } + // If XdsTestType::xds_server_uses_tls_creds() is true, + // returns TlsServerCredentials. + // Otherwise, returns fake credentials. + std::shared_ptr Credentials() override; + private: const char* Type() override { return "Balancer"; } void RegisterAllServices(ServerBuilder* builder) override; @@ -578,7 +593,8 @@ class XdsEnd2endTest : public ::testing::TestWithParam, // All tests must call this exactly once at the start of the test. void InitClient(absl::optional builder = absl::nullopt, std::string lb_expected_authority = "", - int xds_resource_does_not_exist_timeout_ms = 0); + int xds_resource_does_not_exist_timeout_ms = 0, + std::string balancer_authority_override = ""); XdsBootstrapBuilder MakeBootstrapBuilder() { return XdsBootstrapBuilder().SetServers( diff --git a/test/cpp/end2end/xds/xds_utils.cc b/test/cpp/end2end/xds/xds_utils.cc index f70ca9b9ccd..6a45d81b8a0 100644 --- a/test/cpp/end2end/xds/xds_utils.cc +++ b/test/cpp/end2end/xds/xds_utils.cc @@ -84,7 +84,7 @@ std::string XdsBootstrapBuilder::MakeXdsServersText( " \"server_uri\": \"\",\n" " \"channel_creds\": [\n" " {\n" - " \"type\": \"\"\n" + " \"type\": \"\"\n" " }\n" " ],\n" " \"server_features\": []\n" @@ -99,6 +99,11 @@ std::string XdsBootstrapBuilder::MakeXdsServersText( kXdsServerTemplate, {{"", server_uri}, {"", xds_channel_creds_type_}, + {"", + xds_channel_creds_config_.empty() + ? "" + : absl::StrCat(",\n \"config\": ", + xds_channel_creds_config_)}, {"", absl::StrJoin(server_features, ", ")}})); } return absl::StrCat(" \"xds_servers\": [\n", diff --git a/test/cpp/end2end/xds/xds_utils.h b/test/cpp/end2end/xds/xds_utils.h index 10b552ea62c..6fb0d33b341 100644 --- a/test/cpp/end2end/xds/xds_utils.h +++ b/test/cpp/end2end/xds/xds_utils.h @@ -43,8 +43,10 @@ class XdsBootstrapBuilder { servers_ = std::vector(servers.begin(), servers.end()); return *this; } - XdsBootstrapBuilder& SetXdsChannelCredentials(const std::string& type) { + XdsBootstrapBuilder& SetXdsChannelCredentials( + const std::string& type, const std::string& config = "") { xds_channel_creds_type_ = type; + xds_channel_creds_config_ = config; return *this; } XdsBootstrapBuilder& SetClientDefaultListenerResourceNameTemplate( @@ -100,6 +102,7 @@ class XdsBootstrapBuilder { bool ignore_resource_deletion_ = false; std::vector servers_; std::string xds_channel_creds_type_ = "fake"; + std::string xds_channel_creds_config_; std::string client_default_listener_resource_name_template_; std::map plugins_; std::map authorities_; diff --git a/tools/internal_ci/helper_scripts/requirements.linux_perf.txt b/tools/internal_ci/helper_scripts/requirements.linux_perf.txt index 57e1a0c48c1..f1c954d5b39 100644 --- a/tools/internal_ci/helper_scripts/requirements.linux_perf.txt +++ b/tools/internal_ci/helper_scripts/requirements.linux_perf.txt @@ -1,5 +1,5 @@ cryptography==3.4.6 PyJWT==2.0.1 -requests==2.31.0 +requests==2.32.2 scipy==1.5.4 tabulate==0.8.9 diff --git a/tools/internal_ci/helper_scripts/requirements.macos.txt b/tools/internal_ci/helper_scripts/requirements.macos.txt index 190bf328634..59a759b0ce2 100644 --- a/tools/internal_ci/helper_scripts/requirements.macos.txt +++ b/tools/internal_ci/helper_scripts/requirements.macos.txt @@ -3,4 +3,4 @@ cryptography==3.4.6 PyJWT==2.0.1 pyOpenSSL==20.0.1 PyYAML==6.0 -requests==2.31.0 +requests==2.32.2