From 998ab49bd38a45c953b54cf0d5796e96ee89741c Mon Sep 17 00:00:00 2001 From: Inseok Lee Date: Fri, 13 Sep 2024 10:23:06 +0900 Subject: [PATCH 01/12] redis: Support eval_ro, evalsha_ro --- .../network/common/redis/supported_commands.h | 3 +- .../clusters/redis/redis_cluster_lb_test.cc | 25 +++++++++++ .../redis_proxy/command_splitter_impl_test.cc | 44 +++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/network/common/redis/supported_commands.h b/source/extensions/filters/network/common/redis/supported_commands.h index 2bb6e77398fbb..5325d2e494973 100644 --- a/source/extensions/filters/network/common/redis/supported_commands.h +++ b/source/extensions/filters/network/common/redis/supported_commands.h @@ -54,7 +54,8 @@ struct SupportedCommands { * @return commands which hash on the fourth argument */ static const absl::flat_hash_set& evalCommands() { - CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set, "eval", "evalsha"); + CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set, "eval", "evalsha", "eval_ro", + "evalsha_ro"); } /** diff --git a/test/extensions/clusters/redis/redis_cluster_lb_test.cc b/test/extensions/clusters/redis/redis_cluster_lb_test.cc index 3bd635f646d2a..f46f43cec953c 100644 --- a/test/extensions/clusters/redis/redis_cluster_lb_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_lb_test.cc @@ -604,6 +604,31 @@ TEST_F(RedisLoadBalancerContextImplTest, EnforceHashTag) { EXPECT_EQ(NetworkFilters::Common::Redis::Client::ReadPolicy::Primary, context2.readPolicy()); } +TEST_F(RedisLoadBalancerContextImplTest, ReadOnlyCommand) { + std::vector eval_ro_foo(4); + eval_ro_foo[0].type(NetworkFilters::Common::Redis::RespType::BulkString); + eval_ro_foo[0].asString() = "eval_ro"; + eval_ro_foo[1].type(NetworkFilters::Common::Redis::RespType::BulkString); + eval_ro_foo[1].asString() = "return {KEYS[1]}"; + eval_ro_foo[2].type(NetworkFilters::Common::Redis::RespType::BulkString); + eval_ro_foo[2].asString() = "foo"; + eval_ro_foo[3].type(NetworkFilters::Common::Redis::RespType::BulkString); + eval_ro_foo[3].asString() = "0"; + + NetworkFilters::Common::Redis::RespValue eval_ro_request; + eval_ro_request.type(NetworkFilters::Common::Redis::RespType::Array); + eval_ro_request.asArray().swap(eval_ro_foo); + + RedisLoadBalancerContextImpl context1( + "foo", true, true, eval_ro_request, + NetworkFilters::Common::Redis::Client::ReadPolicy::PreferReplica); + + EXPECT_EQ(absl::optional(44950), context1.computeHashKey()); + EXPECT_EQ(true, context1.isReadCommand()); + EXPECT_EQ(NetworkFilters::Common::Redis::Client::ReadPolicy::PreferReplica, + context1.readPolicy()); +} + } // namespace Redis } // namespace Clusters } // namespace Extensions diff --git a/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc b/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc index a30ad3706e32d..8b1ed47de8f4f 100644 --- a/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc @@ -529,6 +529,50 @@ TEST_F(RedisSingleServerRequestTest, EvalShaSuccess) { store_.counter(fmt::format("redis.foo.command.{}.success", lower_command)).value()); }; +TEST_F(RedisSingleServerRequestTest, EvalRoSuccess) { + InSequence s; + + Common::Redis::RespValuePtr request{new Common::Redis::RespValue()}; + makeBulkStringArray(*request, {"eval_ro", "return {ARGV[1]}", "1", "key", "arg"}); + makeRequest("key", std::move(request)); + EXPECT_NE(nullptr, handle_); + + std::string lower_command = absl::AsciiStrToLower("eval_ro"); + + time_system_.setMonotonicTime(std::chrono::milliseconds(10)); + EXPECT_CALL(store_, deliverHistogramToSinks( + Property(&Stats::Metric::name, + fmt::format("redis.foo.command.{}.latency", lower_command)), + 10)); + respond(); + + EXPECT_EQ(1UL, store_.counter(fmt::format("redis.foo.command.{}.total", lower_command)).value()); + EXPECT_EQ(1UL, + store_.counter(fmt::format("redis.foo.command.{}.success", lower_command)).value()); +}; + +TEST_F(RedisSingleServerRequestTest, EvalShaRoSuccess) { + InSequence s; + + Common::Redis::RespValuePtr request{new Common::Redis::RespValue()}; + makeBulkStringArray(*request, {"EVALSHA_RO", "return {ARGV[1]}", "1", "keykey", "arg"}); + makeRequest("keykey", std::move(request)); + EXPECT_NE(nullptr, handle_); + + std::string lower_command = absl::AsciiStrToLower("evalsha_ro"); + + time_system_.setMonotonicTime(std::chrono::milliseconds(10)); + EXPECT_CALL(store_, deliverHistogramToSinks( + Property(&Stats::Metric::name, + fmt::format("redis.foo.command.{}.latency", lower_command)), + 10)); + respond(); + + EXPECT_EQ(1UL, store_.counter(fmt::format("redis.foo.command.{}.total", lower_command)).value()); + EXPECT_EQ(1UL, + store_.counter(fmt::format("redis.foo.command.{}.success", lower_command)).value()); +}; + TEST_F(RedisSingleServerRequestTest, EvalWrongNumberOfArgs) { InSequence s; From 836197fcc0d2e3ba38a1abda05797642c3446b74 Mon Sep 17 00:00:00 2001 From: Doogie Min Date: Sat, 2 Nov 2024 02:32:31 +0900 Subject: [PATCH 02/12] use custom header for tracing Include code formatting improvements for consistent style in trace test files. --- .../opentelemetry/span_context_extractor.h | 4 +- .../tracers/opentelemetry/tracer.cc | 4 +- .../always_on_sampler_integration_test.cc | 25 +++++++----- .../dynatrace_sampler_integration_test.cc | 25 +++++++----- .../span_context_extractor_test.cc | 38 +++++++++++-------- 5 files changed, 56 insertions(+), 40 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/span_context_extractor.h b/source/extensions/tracers/opentelemetry/span_context_extractor.h index dffeb6218c921..517a9700bafaf 100644 --- a/source/extensions/tracers/opentelemetry/span_context_extractor.h +++ b/source/extensions/tracers/opentelemetry/span_context_extractor.h @@ -15,8 +15,8 @@ namespace OpenTelemetry { class OpenTelemetryConstantValues { public: - const Tracing::TraceContextHandler TRACE_PARENT{"traceparent"}; - const Tracing::TraceContextHandler TRACE_STATE{"tracestate"}; + const Tracing::TraceContextHandler TRACE_PARENT{"x-sendbird-traceparent"}; + const Tracing::TraceContextHandler TRACE_STATE{"x-sendbird-tracestate"}; }; using OpenTelemetryConstants = ConstSingleton; diff --git a/source/extensions/tracers/opentelemetry/tracer.cc b/source/extensions/tracers/opentelemetry/tracer.cc index 9017f360e2fbe..66f9003cfcea3 100644 --- a/source/extensions/tracers/opentelemetry/tracer.cc +++ b/source/extensions/tracers/opentelemetry/tracer.cc @@ -27,11 +27,11 @@ using opentelemetry::proto::collector::trace::v1::ExportTraceServiceRequest; namespace { const Tracing::TraceContextHandler& traceParentHeader() { - CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "traceparent"); + CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "x-sendbird-traceparent"); } const Tracing::TraceContextHandler& traceStateHeader() { - CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "tracestate"); + CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "x-sendbird-tracestate"); } void callSampler(SamplerSharedPtr sampler, const StreamInfo::StreamInfo& stream_info, diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_integration_test.cc b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_integration_test.cc index 0fe665b9c9c80..5f5efbeddf1bd 100644 --- a/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_integration_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/always_on_sampler_integration_test.cc @@ -59,9 +59,12 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, AlwaysOnSamplerIntegrationTest, // Sends a request with traceparent and tracestate header. TEST_P(AlwaysOnSamplerIntegrationTest, TestWithTraceparentAndTracestate) { - Http::TestRequestHeaderMapImpl request_headers{ - {":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}, - {":authority", "host"}, {"tracestate", "key=value"}, {"traceparent", TRACEPARENT_VALUE}}; + Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-sendbird-tracestate", "key=value"}, + {"x-sendbird-traceparent", TRACEPARENT_VALUE}}; auto response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); @@ -71,14 +74,14 @@ TEST_P(AlwaysOnSamplerIntegrationTest, TestWithTraceparentAndTracestate) { // traceparent should be set: traceid should be re-used, span id should be different absl::string_view traceparent_value = upstream_request_->headers() - .get(Http::LowerCaseString("traceparent"))[0] + .get(Http::LowerCaseString("x-sendbird-traceparent"))[0] ->value() .getStringView(); EXPECT_TRUE(absl::StartsWith(traceparent_value, TRACEPARENT_VALUE_START)); EXPECT_NE(TRACEPARENT_VALUE, traceparent_value); // tracestate should be forwarded absl::string_view tracestate_value = upstream_request_->headers() - .get(Http::LowerCaseString("tracestate"))[0] + .get(Http::LowerCaseString("x-sendbird-tracestate"))[0] ->value() .getStringView(); EXPECT_EQ("key=value", tracestate_value); @@ -90,7 +93,7 @@ TEST_P(AlwaysOnSamplerIntegrationTest, TestWithTraceparentOnly) { {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}, - {"traceparent", TRACEPARENT_VALUE}}; + {"x-sendbird-traceparent", TRACEPARENT_VALUE}}; auto response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); ASSERT_TRUE(response->waitForEndStream()); @@ -99,14 +102,14 @@ TEST_P(AlwaysOnSamplerIntegrationTest, TestWithTraceparentOnly) { // traceparent should be set: traceid should be re-used, span id should be different absl::string_view traceparent_value = upstream_request_->headers() - .get(Http::LowerCaseString("traceparent"))[0] + .get(Http::LowerCaseString("x-sendbird-traceparent"))[0] ->value() .getStringView(); EXPECT_TRUE(absl::StartsWith(traceparent_value, TRACEPARENT_VALUE_START)); EXPECT_NE(TRACEPARENT_VALUE, traceparent_value); // OTLP tracer adds an empty tracestate absl::string_view tracestate_value = upstream_request_->headers() - .get(Http::LowerCaseString("tracestate"))[0] + .get(Http::LowerCaseString("x-sendbird-tracestate"))[0] ->value() .getStringView(); EXPECT_EQ("", tracestate_value); @@ -125,11 +128,13 @@ TEST_P(AlwaysOnSamplerIntegrationTest, TestWithoutTraceparentAndTracestate) { // traceparent will be added, trace_id and span_id will be generated, so there is nothing we can // assert - EXPECT_EQ(upstream_request_->headers().get(::Envoy::Http::LowerCaseString("traceparent")).size(), + EXPECT_EQ(upstream_request_->headers() + .get(::Envoy::Http::LowerCaseString("x-sendbird-traceparent")) + .size(), 1); // OTLP tracer adds an empty tracestate absl::string_view tracestate_value = upstream_request_->headers() - .get(Http::LowerCaseString("tracestate"))[0] + .get(Http::LowerCaseString("x-sendbird-tracestate"))[0] ->value() .getStringView(); EXPECT_EQ("", tracestate_value); diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_integration_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_integration_test.cc index a3887eaf5ab4e..c2dee1f01ffad 100644 --- a/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_integration_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_integration_test.cc @@ -61,9 +61,12 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DynatraceSamplerIntegrationTest, // Sends a request with traceparent and tracestate header. TEST_P(DynatraceSamplerIntegrationTest, TestWithTraceparentAndTracestate) { // tracestate does not contain a Dynatrace tag - Http::TestRequestHeaderMapImpl request_headers{ - {":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}, - {":authority", "host"}, {"tracestate", "key=value"}, {"traceparent", TRACEPARENT_VALUE}}; + Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-sendbird-tracestate", "key=value"}, + {"x-sendbird-traceparent", TRACEPARENT_VALUE}}; auto response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); @@ -73,14 +76,14 @@ TEST_P(DynatraceSamplerIntegrationTest, TestWithTraceparentAndTracestate) { // traceparent should be set: traceid should be re-used, span id should be different absl::string_view traceparent_value = upstream_request_->headers() - .get(Http::LowerCaseString("traceparent"))[0] + .get(Http::LowerCaseString("x-sendbird-traceparent"))[0] ->value() .getStringView(); EXPECT_TRUE(absl::StartsWith(traceparent_value, TRACEPARENT_VALUE_START)); EXPECT_NE(TRACEPARENT_VALUE, traceparent_value); // Dynatrace tracestate should be added to existing tracestate absl::string_view tracestate_value = upstream_request_->headers() - .get(Http::LowerCaseString("tracestate"))[0] + .get(Http::LowerCaseString("x-sendbird-tracestate"))[0] ->value() .getStringView(); // use StartsWith because path-info (last element in trace state) contains a random value @@ -96,7 +99,7 @@ TEST_P(DynatraceSamplerIntegrationTest, TestWithTraceparentOnly) { {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}, - {"traceparent", TRACEPARENT_VALUE}}; + {"x-sendbird-traceparent", TRACEPARENT_VALUE}}; auto response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); ASSERT_TRUE(response->waitForEndStream()); @@ -105,14 +108,14 @@ TEST_P(DynatraceSamplerIntegrationTest, TestWithTraceparentOnly) { // traceparent should be set: traceid should be re-used, span id should be different absl::string_view traceparent_value = upstream_request_->headers() - .get(Http::LowerCaseString("traceparent"))[0] + .get(Http::LowerCaseString("x-sendbird-traceparent"))[0] ->value() .getStringView(); EXPECT_TRUE(absl::StartsWith(traceparent_value, TRACEPARENT_VALUE_START)); EXPECT_NE(TRACEPARENT_VALUE, traceparent_value); // Dynatrace tag should be added to tracestate absl::string_view tracestate_value = upstream_request_->headers() - .get(Http::LowerCaseString("tracestate"))[0] + .get(Http::LowerCaseString("x-sendbird-tracestate"))[0] ->value() .getStringView(); // use StartsWith because path-info (last element in trace state contains a random value) @@ -133,11 +136,13 @@ TEST_P(DynatraceSamplerIntegrationTest, TestWithoutTraceparentAndTracestate) { // traceparent will be added, trace_id and span_id will be generated, so there is nothing we can // assert - EXPECT_EQ(upstream_request_->headers().get(::Envoy::Http::LowerCaseString("traceparent")).size(), + EXPECT_EQ(upstream_request_->headers() + .get(::Envoy::Http::LowerCaseString("x-sendbird-traceparent")) + .size(), 1); // Dynatrace tag should be added to tracestate absl::string_view tracestate_value = upstream_request_->headers() - .get(Http::LowerCaseString("tracestate"))[0] + .get(Http::LowerCaseString("x-sendbird-tracestate"))[0] ->value() .getStringView(); EXPECT_TRUE(absl::StartsWith(tracestate_value, "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;")) diff --git a/test/extensions/tracers/opentelemetry/span_context_extractor_test.cc b/test/extensions/tracers/opentelemetry/span_context_extractor_test.cc index 5515d760d6bd9..31aacae6b2557 100644 --- a/test/extensions/tracers/opentelemetry/span_context_extractor_test.cc +++ b/test/extensions/tracers/opentelemetry/span_context_extractor_test.cc @@ -23,7 +23,8 @@ constexpr absl::string_view trace_flags = "01"; TEST(SpanContextExtractorTest, ExtractSpanContext) { Tracing::TestTraceContextImpl request_headers{ - {"traceparent", fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}}; + {"x-sendbird-traceparent", + fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}}; SpanContextExtractor span_context_extractor(request_headers); absl::StatusOr span_context = span_context_extractor.extractSpanContext(); @@ -38,7 +39,7 @@ TEST(SpanContextExtractorTest, ExtractSpanContext) { TEST(SpanContextExtractorTest, ExtractSpanContextNotSampled) { const std::string trace_flags_unsampled{"00"}; Tracing::TestTraceContextImpl request_headers{ - {"traceparent", + {"x-sendbird-traceparent", fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags_unsampled)}}; SpanContextExtractor span_context_extractor(request_headers); absl::StatusOr span_context = span_context_extractor.extractSpanContext(); @@ -62,7 +63,8 @@ TEST(SpanContextExtractorTest, ThrowsExceptionWithoutHeader) { TEST(SpanContextExtractorTest, ThrowsExceptionWithTooLongHeader) { Tracing::TestTraceContextImpl request_headers{ - {"traceparent", fmt::format("000{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}}; + {"x-sendbird-traceparent", + fmt::format("000{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}}; SpanContextExtractor span_context_extractor(request_headers); absl::StatusOr span_context = span_context_extractor.extractSpanContext(); @@ -73,7 +75,7 @@ TEST(SpanContextExtractorTest, ThrowsExceptionWithTooLongHeader) { TEST(SpanContextExtractorTest, ThrowsExceptionWithTooShortHeader) { Tracing::TestTraceContextImpl request_headers{ - {"traceparent", fmt::format("{}-{}-{}", trace_id, parent_id, trace_flags)}}; + {"x-sendbird-traceparent", fmt::format("{}-{}-{}", trace_id, parent_id, trace_flags)}}; SpanContextExtractor span_context_extractor(request_headers); absl::StatusOr span_context = span_context_extractor.extractSpanContext(); @@ -84,7 +86,8 @@ TEST(SpanContextExtractorTest, ThrowsExceptionWithTooShortHeader) { TEST(SpanContextExtractorTest, ThrowsExceptionWithInvalidHyphenation) { Tracing::TestTraceContextImpl request_headers{ - {"traceparent", fmt::format("{}{}-{}-{}", version, trace_id, parent_id, trace_flags)}}; + {"x-sendbird-traceparent", + fmt::format("{}{}-{}-{}", version, trace_id, parent_id, trace_flags)}}; SpanContextExtractor span_context_extractor(request_headers); absl::StatusOr span_context = span_context_extractor.extractSpanContext(); @@ -97,7 +100,7 @@ TEST(SpanContextExtractorTest, ThrowsExceptionWithInvalidSizes) { const std::string invalid_version{"0"}; const std::string invalid_trace_flags{"001"}; Tracing::TestTraceContextImpl request_headers{ - {"traceparent", + {"x-sendbird-traceparent", fmt::format("{}-{}-{}-{}", invalid_version, trace_id, parent_id, invalid_trace_flags)}}; SpanContextExtractor span_context_extractor(request_headers); @@ -110,7 +113,7 @@ TEST(SpanContextExtractorTest, ThrowsExceptionWithInvalidSizes) { TEST(SpanContextExtractorTest, ThrowsExceptionWithInvalidHex) { const std::string invalid_version{"ZZ"}; Tracing::TestTraceContextImpl request_headers{ - {"traceparent", + {"x-sendbird-traceparent", fmt::format("{}-{}-{}-{}", invalid_version, trace_id, parent_id, trace_flags)}}; SpanContextExtractor span_context_extractor(request_headers); @@ -123,7 +126,7 @@ TEST(SpanContextExtractorTest, ThrowsExceptionWithInvalidHex) { TEST(SpanContextExtractorTest, ThrowsExceptionWithAllZeroTraceId) { const std::string invalid_trace_id{"00000000000000000000000000000000"}; Tracing::TestTraceContextImpl request_headers{ - {"traceparent", + {"x-sendbird-traceparent", fmt::format("{}-{}-{}-{}", version, invalid_trace_id, parent_id, trace_flags)}}; SpanContextExtractor span_context_extractor(request_headers); @@ -136,7 +139,7 @@ TEST(SpanContextExtractorTest, ThrowsExceptionWithAllZeroTraceId) { TEST(SpanContextExtractorTest, ThrowsExceptionWithAllZeroParentId) { const std::string invalid_parent_id{"0000000000000000"}; Tracing::TestTraceContextImpl request_headers{ - {"traceparent", + {"x-sendbird-traceparent", fmt::format("{}-{}-{}-{}", version, trace_id, invalid_parent_id, trace_flags)}}; SpanContextExtractor span_context_extractor(request_headers); @@ -148,7 +151,8 @@ TEST(SpanContextExtractorTest, ThrowsExceptionWithAllZeroParentId) { TEST(SpanContextExtractorTest, ExtractSpanContextWithEmptyTracestate) { Tracing::TestTraceContextImpl request_headers{ - {"traceparent", fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}}; + {"x-sendbird-traceparent", + fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}}; SpanContextExtractor span_context_extractor(request_headers); absl::StatusOr span_context = span_context_extractor.extractSpanContext(); @@ -158,8 +162,9 @@ TEST(SpanContextExtractorTest, ExtractSpanContextWithEmptyTracestate) { TEST(SpanContextExtractorTest, ExtractSpanContextWithTracestate) { Tracing::TestTraceContextImpl request_headers{ - {"traceparent", fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}, - {"tracestate", "sample-tracestate"}}; + {"x-sendbird-traceparent", + fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}, + {"x-sendbird-tracestate", "sample-tracestate"}}; SpanContextExtractor span_context_extractor(request_headers); absl::StatusOr span_context = span_context_extractor.extractSpanContext(); @@ -168,7 +173,7 @@ TEST(SpanContextExtractorTest, ExtractSpanContextWithTracestate) { } TEST(SpanContextExtractorTest, IgnoreTracestateWithoutTraceparent) { - Tracing::TestTraceContextImpl request_headers{{"tracestate", "sample-tracestate"}}; + Tracing::TestTraceContextImpl request_headers{{"x-sendbird-tracestate", "sample-tracestate"}}; SpanContextExtractor span_context_extractor(request_headers); absl::StatusOr span_context = span_context_extractor.extractSpanContext(); @@ -178,9 +183,10 @@ TEST(SpanContextExtractorTest, IgnoreTracestateWithoutTraceparent) { TEST(SpanContextExtractorTest, ExtractSpanContextWithMultipleTracestateEntries) { Http::TestRequestHeaderMapImpl request_headers{ - {"traceparent", fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}, - {"tracestate", "sample-tracestate"}, - {"tracestate", "sample-tracestate-2"}}; + {"x-sendbird-traceparent", + fmt::format("{}-{}-{}-{}", version, trace_id, parent_id, trace_flags)}, + {"x-sendbird-tracestate", "sample-tracestate"}, + {"x-sendbird-tracestate", "sample-tracestate-2"}}; Tracing::HttpTraceContext trace_context(request_headers); SpanContextExtractor span_context_extractor(trace_context); absl::StatusOr span_context = span_context_extractor.extractSpanContext(); From c8d79910dac3d6ce7bdc610da76214b8510f484f Mon Sep 17 00:00:00 2001 From: Gavin Jeong Date: Wed, 3 Sep 2025 01:17:19 +0900 Subject: [PATCH 03/12] Add QUIC Keylog Support with SSLKEYLOGFILE and TLS Context Integration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit introduces QUIC/HTTP3 keylog functionality in Envoy, enabling generation of NSS Key Log Format files for Wireshark and other debugging tools. - Keylog callback registration in OnNewSslCtx() - Implementation of EnvoyQuicProofSource::setupQuicKeylogCallback() and quicKeylogCallback() - TLS context–based keylog configuration with per–filter chain caching and thread safety - Address filtering via local/remote IP lists - Fallback to SSLKEYLOGFILE environment variable for compatibility with existing workflows - QuicKeylogBridge integration with Envoy’s existing TLS keylog infrastructure - RawBufferSocket fallback fix in QuicServerTransportSocketFactory::createDownstreamTransportSocket() - Comprehensive unit tests including edge cases Signed-off-by: Chanhun Jeong --- source/common/quic/BUILD | 1 + source/common/quic/envoy_quic_proof_source.cc | 177 +++++++++++++++++- source/common/quic/envoy_quic_proof_source.h | 48 ++++- .../quic_server_transport_socket_factory.h | 10 +- .../quic/envoy_quic_proof_source_test.cc | 176 +++++++++++++++++ 5 files changed, 407 insertions(+), 5 deletions(-) diff --git a/source/common/quic/BUILD b/source/common/quic/BUILD index 3b176bbcf4888..169bc36a24fa7 100644 --- a/source/common/quic/BUILD +++ b/source/common/quic/BUILD @@ -525,6 +525,7 @@ envoy_cc_library( "//envoy/server:transport_socket_config_interface", "//envoy/ssl:context_config_interface", "//source/common/common:assert_lib", + "//source/common/network:raw_buffer_socket_lib", "//source/common/network:transport_socket_options_lib", "//source/common/tls:server_context_config_lib", "//source/common/tls:server_context_lib", diff --git a/source/common/quic/envoy_quic_proof_source.cc b/source/common/quic/envoy_quic_proof_source.cc index 04be05c68f311..bb166c52e7a99 100644 --- a/source/common/quic/envoy_quic_proof_source.cc +++ b/source/common/quic/envoy_quic_proof_source.cc @@ -2,6 +2,9 @@ #include +#include +#include + #include "envoy/ssl/tls_certificate_config.h" #include "source/common/quic/cert_compression.h" @@ -9,6 +12,8 @@ #include "source/common/quic/quic_io_handle_wrapper.h" #include "source/common/runtime/runtime_features.h" #include "source/common/stream_info/stream_info_impl.h" +#include "source/common/tls/context_config_impl.h" +#include "source/common/network/utility.h" #include "openssl/bytestring.h" #include "quiche/quic/core/crypto/certificate_view.h" @@ -29,7 +34,7 @@ EnvoyQuicProofSource::GetCertChain(const quic::QuicSocketAddress& server_address return nullptr; } - return getTlsCertAndFilterChain(*res, hostname, cert_matched_sni).cert_; + return getTlsCertAndFilterChain(*res, hostname, cert_matched_sni, server_address, client_address).cert_; } void EnvoyQuicProofSource::signPayload( @@ -44,7 +49,7 @@ void EnvoyQuicProofSource::signPayload( } CertWithFilterChain res = - getTlsCertAndFilterChain(*data, hostname, nullptr /* cert_matched_sni */); + getTlsCertAndFilterChain(*data, hostname, nullptr /* cert_matched_sni */, server_address, client_address); if (res.private_key_ == nullptr) { ENVOY_LOG(warn, "No matching filter chain found for handshake."); callback->Run(false, "", nullptr); @@ -74,13 +79,26 @@ void EnvoyQuicProofSource::signPayload( EnvoyQuicProofSource::CertWithFilterChain EnvoyQuicProofSource::getTlsCertAndFilterChain(const TransportSocketFactoryWithFilterChain& data, const std::string& hostname, - bool* cert_matched_sni) { + bool* cert_matched_sni, + const quic::QuicSocketAddress& server_address, + const quic::QuicSocketAddress& client_address) { auto [cert, key] = data.transport_socket_factory_.getTlsCertificateAndKey(hostname, cert_matched_sni); if (cert == nullptr || key == nullptr) { ENVOY_LOG(warn, "No certificate is configured in transport socket config."); return {}; } + + // Cache the keylog configuration and connection info for this filter chain + try { + const auto& context_config = data.transport_socket_factory_.getContextConfig(); + storeKeylogInfo(data.filter_chain_, + std::shared_ptr(&context_config, [](const Ssl::ContextConfig*){}), + server_address, client_address); + } catch (const std::exception& e) { + ENVOY_LOG(debug, "Failed to cache keylog info for filter chain: {}", e.what()); + } + return {std::move(cert), std::move(key), data.filter_chain_}; } @@ -117,6 +135,159 @@ void EnvoyQuicProofSource::updateFilterChainManager( void EnvoyQuicProofSource::OnNewSslCtx(SSL_CTX* ssl_ctx) { CertCompression::registerSslContext(ssl_ctx); + + // Try to set up keylog callback for QUIC SSL contexts + setupQuicKeylogCallback(ssl_ctx); +} + +void EnvoyQuicProofSource::setupQuicKeylogCallback(SSL_CTX* ssl_ctx) { + // Store reference to this proof source in SSL_CTX for use in keylog callback + SSL_CTX_set_app_data(ssl_ctx, this); + + // Set up the keylog callback - the actual keylog configuration will be + // determined per-connection in the callback based on the filter chain + SSL_CTX_set_keylog_callback(ssl_ctx, quicKeylogCallback); +} + +// Helper function to convert Envoy address to QUICHE address +quic::QuicSocketAddress envoyAddressToQuicAddress(const Network::Address::Instance& envoy_addr) { + if (envoy_addr.type() == Network::Address::Type::Ip) { + const auto& ip_addr = *envoy_addr.ip(); + quiche::QuicheIpAddress quiche_addr; + if (quiche_addr.FromString(ip_addr.addressAsString())) { + return quic::QuicSocketAddress(quic::QuicIpAddress(quiche_addr), ip_addr.port()); + } + } + // Return any address for non-IP addresses + return quic::QuicSocketAddress(); +} + +// Static keylog callback for QUIC SSL contexts +void EnvoyQuicProofSource::quicKeylogCallback(const SSL* ssl, const char* line) { + ASSERT(ssl != nullptr); + + // Get the proof source instance from SSL_CTX + auto* proof_source = + static_cast(SSL_CTX_get_app_data(SSL_get_SSL_CTX(ssl))); + ASSERT(proof_source != nullptr); + + ENVOY_LOG(debug, "QUIC keylog callback invoked for line: {}", line); + + // Try to find keylog configuration from cached filter chain information + // We iterate through all cached filter chains to find one with keylog configuration + bool keylog_written = false; + { + absl::MutexLock lock(&proof_source->keylog_cache_mutex_); + for (const auto& entry : proof_source->keylog_config_cache_) { + const auto& keylog_info = entry.second; + if (keylog_info.config) { + try { + // Convert QUIC addresses back to Envoy addresses for the bridge + std::string server_addr_str = absl::StrCat( + keylog_info.server_address.host().ToString(), ":", + keylog_info.server_address.port()); + std::string client_addr_str = absl::StrCat( + keylog_info.client_address.host().ToString(), ":", + keylog_info.client_address.port()); + + Network::Address::InstanceConstSharedPtr local_addr = + Network::Utility::parseInternetAddressAndPortNoThrow(server_addr_str); + Network::Address::InstanceConstSharedPtr remote_addr = + Network::Utility::parseInternetAddressAndPortNoThrow(client_addr_str); + + if (local_addr && remote_addr) { + QuicKeylogBridge::writeKeylog(*keylog_info.config, *local_addr, *remote_addr, line); + keylog_written = true; + ENVOY_LOG(debug, "QUIC keylog written using cached configuration"); + break; // Successfully handled by built-in system + } + } catch (const std::exception& e) { + ENVOY_LOG(debug, "Failed to write keylog using cached config: {}", e.what()); + } + } + } + } + + if (keylog_written) { + return; + } + + // Fallback: Use environment variable for backward compatibility + const char* keylog_path = std::getenv("SSLKEYLOGFILE"); + if (keylog_path != nullptr) { + std::ofstream keylog_file(keylog_path, std::ios::app); + if (keylog_file.is_open()) { + keylog_file << line << "\n"; + keylog_file.close(); + ENVOY_LOG(debug, "QUIC keylog written to {}: {}", keylog_path, line); + } + } +} + +void EnvoyQuicProofSource::QuicKeylogBridge::writeKeylog( + const Ssl::ContextConfig& config, + const Network::Address::Instance& local_addr, + const Network::Address::Instance& remote_addr, + const char* line) { + + const std::string& keylog_path = config.tlsKeyLogPath(); + if (keylog_path.empty()) { + return; + } + + // Check address filtering + const auto& local_ip_list = config.tlsKeyLogLocal(); + const auto& remote_ip_list = config.tlsKeyLogRemote(); + + bool local_match = (local_ip_list.getIpListSize() == 0 || local_ip_list.contains(local_addr)); + bool remote_match = (remote_ip_list.getIpListSize() == 0 || remote_ip_list.contains(remote_addr)); + + if (!local_match || !remote_match) { + ENVOY_LOG(debug, "QUIC keylog filtered out by address match (local={}, remote={})", + local_match, remote_match); + return; + } + + // Use access log manager to write keylog + try { + auto& access_log_manager = config.accessLogManager(); + auto file_or_error = access_log_manager.createAccessLog( + Filesystem::FilePathAndType{Filesystem::DestinationType::File, keylog_path}); + + if (file_or_error.ok()) { + auto keylog_file = file_or_error.value(); + keylog_file->write(absl::StrCat(line, "\n")); + ENVOY_LOG(debug, "QUIC keylog written via bridge to {}: {}", keylog_path, line); + } else { + ENVOY_LOG(warn, "Failed to create keylog file {}: {}", keylog_path, + file_or_error.status().message()); + } + } catch (const std::exception& e) { + ENVOY_LOG(warn, "Failed to write QUIC keylog: {}", e.what()); + } +} + +// Get SSL socket index for storing transport socket callbacks +int EnvoyQuicProofSource::sslSocketIndex() { + static int ssl_socket_index = SSL_get_ex_new_index(0, nullptr, nullptr, nullptr, nullptr); + return ssl_socket_index; +} + +void EnvoyQuicProofSource::storeKeylogInfo(const Network::FilterChain& filter_chain, + std::shared_ptr config, + const quic::QuicSocketAddress& server_address, + const quic::QuicSocketAddress& client_address) const { + absl::MutexLock lock(&keylog_cache_mutex_); + keylog_config_cache_[&filter_chain] = KeylogInfo{std::move(config), server_address, client_address}; +} + +absl::optional EnvoyQuicProofSource::getKeylogInfo(const Network::FilterChain& filter_chain) const { + absl::MutexLock lock(&keylog_cache_mutex_); + auto it = keylog_config_cache_.find(&filter_chain); + if (it != keylog_config_cache_.end()) { + return it->second; + } + return absl::nullopt; } } // namespace Quic diff --git a/source/common/quic/envoy_quic_proof_source.h b/source/common/quic/envoy_quic_proof_source.h index 6a9bb62ee255f..a521953c13682 100644 --- a/source/common/quic/envoy_quic_proof_source.h +++ b/source/common/quic/envoy_quic_proof_source.h @@ -1,15 +1,30 @@ #pragma once +#include + +#include "envoy/ssl/context_config.h" + +#include "source/common/common/thread.h" #include "source/common/quic/envoy_quic_proof_source_base.h" #include "source/common/quic/quic_server_transport_socket_factory.h" #include "source/server/listener_stats.h" +#include "absl/synchronization/mutex.h" +#include "absl/types/optional.h" +#include "quiche/quic/platform/api/quic_socket_address.h" + namespace Envoy { namespace Quic { // A ProofSource implementation which supplies a proof instance with certs from filter chain. class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase { public: + // Cache for keylog configurations by filter chain + struct KeylogInfo { + std::shared_ptr config; + quic::QuicSocketAddress server_address; + quic::QuicSocketAddress client_address; + }; EnvoyQuicProofSource(Network::Socket& listen_socket, Network::FilterChainManager& filter_chain_manager, Server::ListenerStats& listener_stats, TimeSource& time_source) @@ -27,6 +42,15 @@ class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase { void updateFilterChainManager(Network::FilterChainManager& filter_chain_manager); + // Bridge interface for QUIC-TLS keylog integration + class QuicKeylogBridge { + public: + static void writeKeylog(const Ssl::ContextConfig& config, + const Network::Address::Instance& local_addr, + const Network::Address::Instance& remote_addr, + const char* line); + }; + protected: // quic::ProofSource void signPayload(const quic::QuicSocketAddress& server_address, @@ -47,17 +71,39 @@ class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase { }; CertWithFilterChain getTlsCertAndFilterChain(const TransportSocketFactoryWithFilterChain& data, - const std::string& hostname, bool* cert_matched_sni); + const std::string& hostname, bool* cert_matched_sni, + const quic::QuicSocketAddress& server_address, + const quic::QuicSocketAddress& client_address); absl::optional getTransportSocketAndFilterChain(const quic::QuicSocketAddress& server_address, const quic::QuicSocketAddress& client_address, const std::string& hostname); + void setupQuicKeylogCallback(SSL_CTX* ssl_ctx); + + // Static callback function for QUIC keylog + static void quicKeylogCallback(const SSL* ssl, const char* line); + + // Get SSL socket index for storing transport socket callbacks + static int sslSocketIndex(); + + // Store keylog configuration and connection info for a filter chain + void storeKeylogInfo(const Network::FilterChain& filter_chain, + std::shared_ptr config, + const quic::QuicSocketAddress& server_address, + const quic::QuicSocketAddress& client_address) const; + + // Get cached keylog information for a filter chain + absl::optional getKeylogInfo(const Network::FilterChain& filter_chain) const; + Network::Socket& listen_socket_; Network::FilterChainManager* filter_chain_manager_{nullptr}; Server::ListenerStats& listener_stats_; TimeSource& time_source_; + + mutable absl::Mutex keylog_cache_mutex_; + mutable std::unordered_map keylog_config_cache_ ABSL_GUARDED_BY(keylog_cache_mutex_); }; } // namespace Quic diff --git a/source/common/quic/quic_server_transport_socket_factory.h b/source/common/quic/quic_server_transport_socket_factory.h index 85aaf45a7e5d5..48f5b365e09f9 100644 --- a/source/common/quic/quic_server_transport_socket_factory.h +++ b/source/common/quic/quic_server_transport_socket_factory.h @@ -7,6 +7,7 @@ #include "envoy/ssl/handshaker.h" #include "source/common/common/assert.h" +#include "source/common/network/raw_buffer_socket.h" #include "source/common/network/transport_socket_options_impl.h" #include "source/common/quic/quic_transport_socket_factory.h" #include "source/common/tls/server_ssl_socket.h" @@ -25,8 +26,12 @@ class QuicServerTransportSocketFactory : public Network::DownstreamTransportSock ~QuicServerTransportSocketFactory() override; // Network::DownstreamTransportSocketFactory + // QUIC uses a different transport socket mechanism, but some code paths may call this + // Return a raw buffer socket as a safe fallback Network::TransportSocketPtr createDownstreamTransportSocket() const override { - PANIC("not implemented"); + ENVOY_LOG(warn, "createDownstreamTransportSocket called on QUIC transport socket factory. " + "This should not happen in normal QUIC operation."); + return std::make_unique(); } bool implementsSecureTransport() const override { return true; } @@ -38,6 +43,9 @@ class QuicServerTransportSocketFactory : public Network::DownstreamTransportSock bool earlyDataEnabled() const { return enable_early_data_; } + // Access the TLS context configuration (for keylog integration) + const Ssl::ServerContextConfig& getContextConfig() const { return *config_; } + protected: QuicServerTransportSocketFactory(bool enable_early_data, Stats::Scope& store, Ssl::ServerContextConfigPtr config, diff --git a/test/common/quic/envoy_quic_proof_source_test.cc b/test/common/quic/envoy_quic_proof_source_test.cc index a51dfa36722b8..af4b43647a3c7 100644 --- a/test/common/quic/envoy_quic_proof_source_test.cc +++ b/test/common/quic/envoy_quic_proof_source_test.cc @@ -1,3 +1,7 @@ +#include + +#include +#include #include #include #include @@ -13,6 +17,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/server/server_factory_context.h" #include "test/mocks/ssl/mocks.h" +#include "test/test_common/network_utility.h" #include "test/test_common/test_runtime.h" #include "gmock/gmock.h" @@ -347,5 +352,176 @@ TEST_F(EnvoyQuicProofSourceTest, ComputeSignatureFailNoFilterChain) { std::make_unique(false, filter_chain_, signature)); } +// Test keylog functionality +TEST_F(EnvoyQuicProofSourceTest, TestKeylogFunctionality) { + // Test that OnNewSslCtx sets up keylog callback correctly + bssl::UniquePtr ssl_ctx(SSL_CTX_new(TLS_method())); + ASSERT_NE(ssl_ctx, nullptr); + + // Call OnNewSslCtx which should set up the keylog callback + proof_source_.OnNewSslCtx(ssl_ctx.get()); + + // Verify that the proof source was stored in SSL_CTX app data + void* app_data = SSL_CTX_get_app_data(ssl_ctx.get()); + EXPECT_EQ(app_data, static_cast(&proof_source_)); + + // Verify that keylog callback was set + void (*callback)(const SSL*, const char*) = SSL_CTX_get_keylog_callback(ssl_ctx.get()); + EXPECT_NE(callback, nullptr); +} + +// Test keylog callback registration +TEST_F(EnvoyQuicProofSourceTest, TestKeylogCallbackRegistration) { + // Create SSL_CTX and setup keylog + bssl::UniquePtr ssl_ctx(SSL_CTX_new(TLS_method())); + proof_source_.OnNewSslCtx(ssl_ctx.get()); + + // Verify that keylog callback is registered + void (*callback)(const SSL*, const char*) = SSL_CTX_get_keylog_callback(ssl_ctx.get()); + EXPECT_NE(callback, nullptr); + + // Verify that app data points to our proof source + void* app_data = SSL_CTX_get_app_data(ssl_ctx.get()); + EXPECT_EQ(app_data, static_cast(&proof_source_)); +} + +// Test keylog file writing with environment variable +TEST_F(EnvoyQuicProofSourceTest, TestKeylogFileWriting) { + // Create a temporary file for keylog output + std::string temp_file = "/tmp/test_keylog_" + std::to_string(getpid()) + ".txt"; + + // Set SSLKEYLOGFILE environment variable + setenv("SSLKEYLOGFILE", temp_file.c_str(), 1); + + // Create SSL_CTX and setup keylog + bssl::UniquePtr ssl_ctx(SSL_CTX_new(TLS_method())); + proof_source_.OnNewSslCtx(ssl_ctx.get()); + + // Create SSL connection + bssl::UniquePtr ssl(SSL_new(ssl_ctx.get())); + + // Get the keylog callback and call it to test functionality + void (*callback)(const SSL*, const char*) = SSL_CTX_get_keylog_callback(ssl_ctx.get()); + ASSERT_NE(callback, nullptr); + + // Call the callback with test data + const char* test_line = "CLIENT_RANDOM 0123456789abcdef test_key_material"; + callback(ssl.get(), test_line); + + // Verify the keylog was written to file + std::ifstream keylog_file(temp_file); + ASSERT_TRUE(keylog_file.is_open()); + std::string line; + ASSERT_TRUE(std::getline(keylog_file, line)); + EXPECT_EQ(line, test_line); + keylog_file.close(); + + // Clean up + unlink(temp_file.c_str()); + unsetenv("SSLKEYLOGFILE"); +} + +// Test keylog callback without environment variable +TEST_F(EnvoyQuicProofSourceTest, TestKeylogCallbackWithoutEnvironmentVariable) { + // Ensure SSLKEYLOGFILE is not set + unsetenv("SSLKEYLOGFILE"); + + // Create SSL_CTX and setup keylog + bssl::UniquePtr ssl_ctx(SSL_CTX_new(TLS_method())); + proof_source_.OnNewSslCtx(ssl_ctx.get()); + + // Verify that keylog callback is still registered (even without env var) + void (*callback)(const SSL*, const char*) = SSL_CTX_get_keylog_callback(ssl_ctx.get()); + EXPECT_NE(callback, nullptr); + + // Create SSL connection and test that callback doesn't crash without env var + bssl::UniquePtr ssl(SSL_new(ssl_ctx.get())); + + // Call the callback - it should not crash even without SSLKEYLOGFILE set + const char* test_line = "CLIENT_RANDOM 0123456789abcdef test_key_material"; + EXPECT_NO_THROW(callback(ssl.get(), test_line)); +} + +// Test QUIC keylog bridge functionality +TEST_F(EnvoyQuicProofSourceTest, TestQuicKeylogBridge) { + // Create a mock context config with keylog configuration + NiceMock mock_config; + NiceMock mock_access_log_manager; + auto mock_access_log_file = std::make_shared>(); + + std::string keylog_path = "/tmp/test_bridge_keylog_" + std::to_string(getpid()) + ".txt"; + + // Setup mock expectations + EXPECT_CALL(mock_config, tlsKeyLogPath()) + .WillRepeatedly(ReturnRef(keylog_path)); + + Network::Address::IpList empty_ip_list; + EXPECT_CALL(mock_config, tlsKeyLogLocal()) + .WillRepeatedly(ReturnRef(empty_ip_list)); + EXPECT_CALL(mock_config, tlsKeyLogRemote()) + .WillRepeatedly(ReturnRef(empty_ip_list)); + + EXPECT_CALL(mock_config, accessLogManager()) + .WillRepeatedly(ReturnRef(mock_access_log_manager)); + + EXPECT_CALL(mock_access_log_manager, createAccessLog(_)) + .WillOnce(Return(absl::StatusOr(mock_access_log_file))); + + EXPECT_CALL(*mock_access_log_file, write(_)) + .Times(1); + + // Create test addresses + auto local_addr = Network::Test::getCanonicalLoopbackAddress(Network::Address::IpVersion::v4); + auto remote_addr = Network::Test::getCanonicalLoopbackAddress(Network::Address::IpVersion::v4); + + // Test the bridge functionality + const char* test_line = "CLIENT_RANDOM 123456789 ABCDEF"; + EnvoyQuicProofSource::QuicKeylogBridge::writeKeylog(mock_config, *local_addr, *remote_addr, test_line); +} + +// Test the complete keylog callback flow including SSL context setup +TEST_F(EnvoyQuicProofSourceTest, TestKeylogCallbackWithSslContext) { + // Create an SSL context to test the callback registration + bssl::UniquePtr ssl_ctx(SSL_CTX_new(TLS_method())); + ASSERT_NE(ssl_ctx, nullptr); + + // Use OnNewSslCtx which calls setupQuicKeylogCallback internally + proof_source_.OnNewSslCtx(ssl_ctx.get()); + + // Create an SSL connection + bssl::UniquePtr ssl(SSL_new(ssl_ctx.get())); + ASSERT_NE(ssl, nullptr); + + // Verify that the keylog callback is set + auto callback = SSL_CTX_get_keylog_callback(ssl_ctx.get()); + EXPECT_NE(callback, nullptr); + + // Verify that the proof source is stored as app data + auto stored_proof_source = SSL_CTX_get_app_data(ssl_ctx.get()); + EXPECT_EQ(stored_proof_source, &proof_source_); + + // Test calling the callback - it should handle the case where transport socket callbacks are not available + const char* test_line = "CLIENT_RANDOM 0123456789abcdef test_key_material"; + + // Set up environment variable for fallback test + std::string keylog_path = "/tmp/test_callback_keylog_" + std::to_string(getpid()) + ".txt"; + setenv("SSLKEYLOGFILE", keylog_path.c_str(), 1); + + EXPECT_NO_THROW(callback(ssl.get(), test_line)); + + // Check that the keylog was written via environment variable fallback + std::ifstream keylog_file(keylog_path); + EXPECT_TRUE(keylog_file.good()); + if (keylog_file.good()) { + std::string line; + std::getline(keylog_file, line); + EXPECT_EQ(line, test_line); + } + + // Clean up + unsetenv("SSLKEYLOGFILE"); + unlink(keylog_path.c_str()); +} + } // namespace Quic } // namespace Envoy From 7b6861d16e05b65ebcc2152e02f43ff387e60f40 Mon Sep 17 00:00:00 2001 From: Chanhun Jeong Date: Wed, 1 Oct 2025 09:52:29 +0900 Subject: [PATCH 04/12] redis: Fix race conditions and use-after-free issues in cluster destruction This commit combines multiple fixes for Redis cluster stability: - Fix race conditions in cluster destruction by capturing is_destroying_ flag - Add comprehensive null checks to prevent segfaults during cluster destruction - Use local shared_ptr copies to prevent race conditions - Use shared_from_this() to keep RedisDiscoverySession alive during timer callbacks - Fix use-after-free by using session-owned flag instead of parent reference These fixes ensure safe cleanup of Redis clusters and prevent crashes during cluster removal and timer callback execution. --- .../clusters/redis/redis_cluster.cc | 185 ++++++++++++++---- .../extensions/clusters/redis/redis_cluster.h | 30 +++ 2 files changed, 176 insertions(+), 39 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 5c475c8322b6a..e29e23dd5f704 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -69,11 +69,12 @@ RedisCluster::RedisCluster( info(), context.serverFactoryContext().api())), auth_password_(NetworkFilters::RedisProxy::ProtocolOptionsConfigImpl::authPassword( info(), context.serverFactoryContext().api())), - cluster_name_(cluster.name()), refresh_manager_(Common::Redis::getClusterRefreshManager( - context.serverFactoryContext().singletonManager(), - context.serverFactoryContext().mainThreadDispatcher(), - context.serverFactoryContext().clusterManager(), - context.serverFactoryContext().api().timeSource())), + cluster_name_(cluster.name()), + refresh_manager_(Common::Redis::getClusterRefreshManager( + context.serverFactoryContext().singletonManager(), + context.serverFactoryContext().mainThreadDispatcher(), + context.serverFactoryContext().clusterManager(), + context.serverFactoryContext().api().timeSource())), registration_handle_(nullptr) { const auto& locality_lb_endpoints = load_assignment_.endpoints(); for (const auto& locality_lb_endpoint : locality_lb_endpoints) { @@ -85,21 +86,39 @@ RedisCluster::RedisCluster( } // Register the cluster callback using weak_ptr to avoid use-after-free + // Also capture a pointer to is_destroying_ to check destruction state std::weak_ptr weak_session = redis_discovery_session_; + std::atomic* is_destroying_ptr = &is_destroying_; registration_handle_ = refresh_manager_->registerCluster( cluster_name_, redirect_refresh_interval_, redirect_refresh_threshold_, - failure_refresh_threshold_, host_degraded_refresh_threshold_, [weak_session]() { + failure_refresh_threshold_, host_degraded_refresh_threshold_, + [weak_session, is_destroying_ptr]() { + // Check if cluster is being destroyed first + if (is_destroying_ptr->load(std::memory_order_acquire)) { + return; + } // Try to lock the weak pointer to ensure the session is still alive auto session = weak_session.lock(); if (session && session->resolve_timer_) { session->resolve_timer_->enableTimer(std::chrono::milliseconds(0)); } }); + + // Initialize the session after construction is complete so it can use shared_from_this() + redis_discovery_session_->initialize(); } RedisCluster::~RedisCluster() { // Set flag to prevent any callbacks from executing during destruction - is_destroying_.store(true); + // Use memory_order_release to ensure this write is visible to callbacks + is_destroying_.store(true, std::memory_order_release); + + // CRITICAL: Set the session's parent_destroyed_ flag BEFORE resetting the session. + // This allows callbacks with shared_from_this() to safely check if parent is destroyed + // without accessing the parent object itself (which may be destroyed). + if (redis_discovery_session_) { + redis_discovery_session_->parent_destroyed_.store(true, std::memory_order_release); + } // Reset redis_discovery_session_ before other members are destroyed // to ensure any pending callbacks from refresh_manager_ don't access it. @@ -109,6 +128,11 @@ RedisCluster::~RedisCluster() { // Also clear DNS discovery targets to prevent their callbacks from // accessing the destroyed cluster. dns_discovery_resolve_targets_.clear(); + + // Reset the registration handle LAST to ensure no new callbacks are scheduled + // while we're cleaning up. Any callbacks already scheduled will check is_destroying_ + // and return early. + registration_handle_.reset(); } void RedisCluster::startPreInit() { @@ -228,6 +252,10 @@ RedisCluster::DnsDiscoveryResolveTarget::~DnsDiscoveryResolveTarget() { void RedisCluster::DnsDiscoveryResolveTarget::startResolveDns() { ENVOY_LOG(trace, "starting async DNS resolution for {}", dns_address_); + if (parent_.is_destroying_.load(std::memory_order_acquire) || !parent_.dns_resolver_) { + return; + } + active_query_ = parent_.dns_resolver_->resolve( dns_address_, parent_.dns_lookup_family_, [this](Network::DnsResolver::ResolutionStatus status, absl::string_view, @@ -235,26 +263,34 @@ void RedisCluster::DnsDiscoveryResolveTarget::startResolveDns() { active_query_ = nullptr; ENVOY_LOG(trace, "async DNS resolution complete for {}", dns_address_); if (status == Network::DnsResolver::ResolutionStatus::Failure || response.empty()) { - if (status == Network::DnsResolver::ResolutionStatus::Failure) { - parent_.info_->configUpdateStats().update_failure_.inc(); - } else { - parent_.info_->configUpdateStats().update_empty_.inc(); + auto info = parent_.info_; + if (info) { + if (status == Network::DnsResolver::ResolutionStatus::Failure) { + info->configUpdateStats().update_failure_.inc(); + } else { + info->configUpdateStats().update_empty_.inc(); + } } if (!resolve_timer_) { - resolve_timer_ = parent_.dispatcher_.createTimer([this]() -> void { - // Check if the parent cluster is being destroyed - if (parent_.is_destroying_.load()) { - return; - } - startResolveDns(); - }); + resolve_timer_ = + parent_.dispatcher_.createTimer([this]() -> void { + // Check if the parent cluster is being destroyed + if (parent_.is_destroying_.load()) { + return; + } + startResolveDns(); + }); } // if the initial dns resolved to empty, we'll skip the redis discovery phase and // treat it as an empty cluster. parent_.onPreInitComplete(); resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); } else { + // Check if the parent cluster is being destroyed + if (parent_.is_destroying_.load(std::memory_order_acquire)) { + return; + } // Once the DNS resolve the initial set of addresses, call startResolveRedis on // the RedisDiscoverySession. The RedisDiscoverySession will using the "cluster // slots" command for service discovery and slot allocation. All subsequent @@ -271,18 +307,23 @@ RedisCluster::RedisDiscoverySession::RedisDiscoverySession( Envoy::Extensions::Clusters::Redis::RedisCluster& parent, NetworkFilters::Common::Redis::Client::ClientFactory& client_factory) : parent_(parent), dispatcher_(parent.dispatcher_), - resolve_timer_(parent.dispatcher_.createTimer([this]() -> void { - // Check if the parent cluster is being destroyed - if (parent_.is_destroying_.load()) { - return; - } - startResolveRedis(); - })), + resolve_timer_(nullptr), client_factory_(client_factory), buffer_timeout_(0), redis_command_stats_( NetworkFilters::Common::Redis::RedisCommandStats::createRedisCommandStats( parent_.info()->statsScope().symbolTable())) {} +void RedisCluster::RedisDiscoverySession::initialize() { + // Create timer with shared_from_this() to keep session alive during callbacks + auto self = shared_from_this(); + resolve_timer_ = dispatcher_.createTimer([self]() -> void { + if (!self->isParentAlive()) { + return; + } + self->startResolveRedis(); + }); +} + // Convert the cluster slot IP/Port response to an address, return null if the response // does not match the expected type. Network::Address::InstanceConstSharedPtr @@ -310,6 +351,10 @@ RedisCluster::RedisDiscoverySession::~RedisDiscoverySession() { void RedisCluster::RedisDiscoveryClient::onEvent(Network::ConnectionEvent event) { if (event == Network::ConnectionEvent::RemoteClose || event == Network::ConnectionEvent::LocalClose) { + // Check if the parent cluster is being destroyed + if (parent_.parent_.is_destroying_.load(std::memory_order_acquire)) { + return; + } auto client_to_delete = parent_.client_map_.find(host_); ASSERT(client_to_delete != parent_.client_map_.end()); parent_.dispatcher_.deferredDelete(std::move(client_to_delete->second->client_)); @@ -330,11 +375,17 @@ void RedisCluster::RedisDiscoverySession::registerDiscoveryAddress( } void RedisCluster::RedisDiscoverySession::startResolveRedis() { - parent_.info_->configUpdateStats().update_attempt_.inc(); + // Use helper to safely get parent info (returns nullptr if parent is being destroyed) + auto info = parentInfo(); + if (!info) { + return; + } + + info->configUpdateStats().update_attempt_.inc(); // If a resolution is currently in progress, skip it. if (current_request_) { ENVOY_LOG(debug, "redis cluster slot request is already in progress for '{}'", - parent_.info_->name()); + info ? info->name() : "unknown"); return; } @@ -356,23 +407,33 @@ void RedisCluster::RedisDiscoverySession::startResolveRedis() { if (!client) { client = std::make_unique(*this); client->host_ = current_host_address_; + // Get parent info again in case parent was destroyed between checks + auto parent_info = parentInfo(); + if (!parent_info) { + return; + } // absl::nullopt here disables AWS IAM authentication in redis client which is not supported by // redis cluster implementation client->client_ = client_factory_.create( - host, dispatcher_, shared_from_this(), redis_command_stats_, parent_.info()->statsScope(), + host, dispatcher_, shared_from_this(), redis_command_stats_, parent_info->statsScope(), parent_.auth_username_, parent_.auth_password_, false, absl::nullopt, absl::nullopt); client->client_->addConnectionCallbacks(*client); } - ENVOY_LOG(debug, "executing redis cluster slot request for '{}'", parent_.info_->name()); + ENVOY_LOG(debug, "executing redis cluster slot request for '{}'", + info ? info->name() : "unknown"); current_request_ = client->client_->makeRequest(ClusterSlotsRequest::instance_, *this); } void RedisCluster::RedisDiscoverySession::updateDnsStats( Network::DnsResolver::ResolutionStatus status, bool empty_response) { + auto info = parentInfo(); + if (!info) { + return; + } if (status == Network::DnsResolver::ResolutionStatus::Failure) { - parent_.info_->configUpdateStats().update_failure_.inc(); + info->configUpdateStats().update_failure_.inc(); } else if (empty_response) { - parent_.info_->configUpdateStats().update_empty_.inc(); + info->configUpdateStats().update_empty_.inc(); } } @@ -393,6 +454,10 @@ void RedisCluster::RedisDiscoverySession::updateDnsStats( void RedisCluster::RedisDiscoverySession::resolveClusterHostnames( ClusterSlotsSharedPtr&& slots, std::shared_ptr hostname_resolution_required_cnt) { + if (!isParentAlive() || !parent_.dns_resolver_) { + return; + } + for (uint64_t slot_idx = 0; slot_idx < slots->size(); slot_idx++) { auto& slot = (*slots)[slot_idx]; if (slot.primary() == nullptr) { @@ -404,6 +469,10 @@ void RedisCluster::RedisDiscoverySession::resolveClusterHostnames( [this, slot_idx, slots, hostname_resolution_required_cnt]( Network::DnsResolver::ResolutionStatus status, absl::string_view, std::list&& response) -> void { + if (!isParentAlive()) { + return; + } + auto& slot = (*slots)[slot_idx]; ENVOY_LOG( debug, @@ -451,6 +520,10 @@ void RedisCluster::RedisDiscoverySession::resolveClusterHostnames( void RedisCluster::RedisDiscoverySession::resolveReplicas( ClusterSlotsSharedPtr slots, std::size_t index, std::shared_ptr hostname_resolution_required_cnt) { + if (!isParentAlive() || !parent_.dns_resolver_) { + return; + } + auto& slot = (*slots)[index]; if (slot.replicas_to_resolve_.empty()) { if (*hostname_resolution_required_cnt == 0) { @@ -467,6 +540,11 @@ void RedisCluster::RedisDiscoverySession::resolveReplicas( [this, index, slots, replica_idx, hostname_resolution_required_cnt]( Network::DnsResolver::ResolutionStatus status, absl::string_view, std::list&& response) -> void { + // Check if the parent cluster is being destroyed before accessing any parent members + if (parent_.is_destroying_.load(std::memory_order_acquire)) { + return; + } + auto& slot = (*slots)[index]; auto& replica = slot.replicas_to_resolve_[replica_idx]; ENVOY_LOG(debug, "async DNS resolution complete for replica address {}", replica.first); @@ -495,13 +573,24 @@ void RedisCluster::RedisDiscoverySession::resolveReplicas( void RedisCluster::RedisDiscoverySession::finishClusterHostnameResolution( ClusterSlotsSharedPtr slots) { + if (!isParentAlive()) { + return; + } parent_.onClusterSlotUpdate(std::move(slots)); - resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + if (resolve_timer_) { + resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + } } void RedisCluster::RedisDiscoverySession::onResponse( NetworkFilters::Common::Redis::RespValuePtr&& value) { - ENVOY_LOG(debug, "redis cluster slot request for '{}' succeeded", parent_.info_->name()); + auto info = parentInfo(); + if (!info) { + current_request_ = nullptr; + return; + } + ENVOY_LOG(debug, "redis cluster slot request for '{}' succeeded", + info ? info->name() : "unknown"); current_request_ = nullptr; const uint32_t SlotRangeStart = 0; @@ -597,7 +686,9 @@ void RedisCluster::RedisDiscoverySession::onResponse( } else { // All slots addresses were represented by IP/Port pairs. parent_.onClusterSlotUpdate(std::move(cluster_slots)); - resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + if (resolve_timer_) { + resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + } } } @@ -627,20 +718,36 @@ bool RedisCluster::RedisDiscoverySession::validateCluster( void RedisCluster::RedisDiscoverySession::onUnexpectedResponse( const NetworkFilters::Common::Redis::RespValuePtr& value) { + // Check if the parent cluster is being destroyed before accessing any parent members + auto info = parentInfo(); + if (!info) { + return; + } + ENVOY_LOG(warn, "Unexpected response to cluster slot command: {}", value->toString()); - this->parent_.info_->configUpdateStats().update_failure_.inc(); - resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + info->configUpdateStats().update_failure_.inc(); + if (resolve_timer_) { + resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + } } void RedisCluster::RedisDiscoverySession::onFailure() { - ENVOY_LOG(debug, "redis cluster slot request for '{}' failed", parent_.info_->name()); current_request_ = nullptr; + + auto info = parentInfo(); + if (!info) { + return; + } + + ENVOY_LOG(debug, "redis cluster slot request for '{}' failed", info->name()); if (!current_host_address_.empty()) { auto client_to_delete = client_map_.find(current_host_address_); client_to_delete->second->client_->close(); } - parent_.info()->configUpdateStats().update_failure_.inc(); - resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + info->configUpdateStats().update_failure_.inc(); + if (resolve_timer_) { + resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + } } RedisCluster::ClusterSlotsRequest RedisCluster::ClusterSlotsRequest::instance_; diff --git a/source/extensions/clusters/redis/redis_cluster.h b/source/extensions/clusters/redis/redis_cluster.h index 1b12f347ddf88..0a4b8fe8d29e9 100644 --- a/source/extensions/clusters/redis/redis_cluster.h +++ b/source/extensions/clusters/redis/redis_cluster.h @@ -221,6 +221,9 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { ~RedisDiscoverySession() override; + // Initialize timer - must be called after construction since it uses shared_from_this() + void initialize(); + void registerDiscoveryAddress(std::list&& response, const uint32_t port); // Start discovery against a random host from existing hosts @@ -266,6 +269,28 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { void finishClusterHostnameResolution(ClusterSlotsSharedPtr slots); void updateDnsStats(Network::DnsResolver::ResolutionStatus status, bool empty_response); + private: + friend class RedisCluster; + friend struct RedisCluster::DnsDiscoveryResolveTarget; + friend struct RedisDiscoveryClient; + // Thread-safe check if parent cluster is being destroyed. + // Returns true if it's safe to proceed with parent operations. + // NOTE: We check our own flag instead of parent_.is_destroying_ because + // parent_ is a reference that becomes dangling after parent is destroyed. + bool isParentAlive() const { + return !parent_destroyed_.load(std::memory_order_acquire); + } + + // Thread-safe accessor for parent cluster info. + // Returns nullptr if parent is being destroyed or info is not available. + // This encapsulates the safety checks needed when accessing parent state from callbacks. + Upstream::ClusterInfoConstSharedPtr parentInfo() const { + if (!isParentAlive()) { + return nullptr; + } + return parent_.info_; + } + RedisCluster& parent_; Event::Dispatcher& dispatcher_; std::string current_host_address_; @@ -278,6 +303,11 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { NetworkFilters::Common::Redis::Client::ClientFactory& client_factory_; const std::chrono::milliseconds buffer_timeout_; NetworkFilters::Common::Redis::RedisCommandStatsSharedPtr redis_command_stats_; + + // Flag set by parent's destructor to signal that parent is being destroyed. + // Callbacks check this flag (owned by session) instead of accessing parent's flag + // to avoid use-after-free when parent is destroyed but callbacks are still queued. + std::atomic parent_destroyed_{false}; }; Upstream::ClusterManager& cluster_manager_; From 50d7d358b309a30a396c518dd5ae52286ba686af Mon Sep 17 00:00:00 2001 From: Jack Park Date: Thu, 6 Nov 2025 16:13:03 -0800 Subject: [PATCH 05/12] Add pubsub commands --- .../arch_overview/other_protocols/redis.rst | 4 +++ .../network/common/redis/supported_commands.h | 31 ++++++++++--------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/docs/root/intro/arch_overview/other_protocols/redis.rst b/docs/root/intro/arch_overview/other_protocols/redis.rst index 42f851309a653..b2b573d19135c 100644 --- a/docs/root/intro/arch_overview/other_protocols/redis.rst +++ b/docs/root/intro/arch_overview/other_protocols/redis.rst @@ -228,6 +228,7 @@ For details on each command's usage see the official SISMEMBER, Set SMEMBERS, Set SPOP, Set + SPUBLISH Pubsub SRANDMEMBER, Set SREM, Set SCAN, Generic @@ -238,8 +239,11 @@ For details on each command's usage see the official SINTERSTORE, Set SMISMEMBER, Set SMOVE, Set + SSUBSCRIBE Pubsub + SUBSCRIBE, Pubsub SUNION, Set SUNIONSTORE, Set + SUNSUBSCRIBE Pubsub WATCH, String UNWATCH, String ZADD, Sorted Set diff --git a/source/extensions/filters/network/common/redis/supported_commands.h b/source/extensions/filters/network/common/redis/supported_commands.h index 5325d2e494973..a2681f26e3fd6 100644 --- a/source/extensions/filters/network/common/redis/supported_commands.h +++ b/source/extensions/filters/network/common/redis/supported_commands.h @@ -30,16 +30,17 @@ struct SupportedCommands { "lpop", "lpush", "lpushx", "lrange", "lrem", "lset", "ltrim", "persist", "pexpire", "pexpireat", "pfadd", "pfcount", "psetex", "pttl", "publish", "restore", "rpop", "rpush", "rpushx", "sadd", "scard", "set", "setbit", "setex", "setnx", "setrange", "sismember", - "smembers", "spop", "srandmember", "srem", "sscan", "strlen", "ttl", "type", "xack", "xadd", - "xautoclaim", "xclaim", "xdel", "xlen", "xpending", "xrange", "xrevrange", "xtrim", "zadd", - "zcard", "zcount", "zincrby", "zlexcount", "zpopmin", "zpopmax", "zrange", "zrangebylex", - "zrangebyscore", "zrank", "zrem", "zremrangebylex", "zremrangebyrank", "zremrangebyscore", - "zrevrange", "zrevrangebylex", "zrevrangebyscore", "zrevrank", "zscan", "zscore", "copy", - "rpoplpush", "smove", "sunion", "sdiff", "sinter", "sinterstore", "zunionstore", - "zinterstore", "pfmerge", "georadius", "georadiusbymember", "rename", "sort", "sort_ro", - "zmscore", "sdiffstore", "msetnx", "substr", "zrangestore", "zunion", "zdiff", - "sunionstore", "smismember", "hrandfield", "geosearchstore", "zdiffstore", "zinter", - "zrandmember", "bitop", "lpos", "renamenx"); + "smembers", "spop", "spublish", "srandmember", "srem", "sscan", "ssubscribe", "strlen", + "subscribe", "sunsubscribe", "ttl", "type", "watch", "xack", "xadd", "xautoclaim", "xclaim", + "xdel", "xlen", "xpending", "xrange", "xrevrange", "xtrim", "zadd", "zcard", "zcount", + "zincrby", "zlexcount", "zpopmin", "zpopmax", "zrange", "zrangebylex", "zrangebyscore", + "zrank", "zrem", "zremrangebylex", "zremrangebyrank", "zremrangebyscore", "zrevrange", + "zrevrangebylex", "zrevrangebyscore", "zrevrank", "zscan", "zscore", "copy", "rpoplpush", + "smove", "sunion", "sdiff", "sinter", "sinterstore", "zunionstore", "zinterstore", "pfmerge", + "georadius", "georadiusbymember", "rename", "sort", "sort_ro", "zmscore", "sdiffstore", + "msetnx", "substr", "zrangestore", "zunion", "zdiff", "sunionstore", "smismember", + "hrandfield", "geosearchstore", "zdiffstore", "zinter", "zrandmember", "bitop", "lpos", + "renamenx"); } /** @@ -143,11 +144,11 @@ struct SupportedCommands { "hincrbyfloat", "hmset", "hset", "hsetnx", "incr", "incrby", "incrbyfloat", "linsert", "lmove", "lpop", "lpush", "lpushx", "lrem", "lset", "ltrim", "mset", "multi", "persist", "pexpire", "pexpireat", "pfadd", "psetex", "restore", "rpop", "rpush", "rpushx", "sadd", - "set", "setbit", "setex", "setnx", "setrange", "spop", "srem", "zadd", "zincrby", "touch", - "zpopmin", "zpopmax", "zrem", "zremrangebylex", "zremrangebyrank", "zremrangebyscore", - "unlink", "copy", "rpoplpush", "smove", "sinterstore", "zunionstore", "zinterstore", - "pfmerge", "georadius", "georadiusbymember", "rename", "sort", "sdiffstore", "msetnx", - "zrangestore", "sunionstore", "geosearchstore", "zdiffstore", "bitop", "renamenx"); + "set", "setbit", "setex", "setnx", "setrange", "spop", "spublish", "srem", "zadd", "zincrby", + "touch", "zpopmin", "zpopmax", "zrem", "zremrangebylex", "zremrangebyrank", + "zremrangebyscore", "unlink", "copy", "rpoplpush", "smove", "sinterstore", "zunionstore", + "zinterstore", "pfmerge", "georadius", "georadiusbymember", "rename", "sort", "sdiffstore", + "msetnx", "zrangestore", "sunionstore", "geosearchstore", "zdiffstore", "bitop", "renamenx"); } static bool isReadCommand(const std::string& command) { From f5f97b5ea491a6f58ef420afa863289f49cab925 Mon Sep 17 00:00:00 2001 From: ku524 Date: Mon, 5 Jan 2026 11:22:18 +0900 Subject: [PATCH 06/12] [CPLAT-8445] Change trace_id pattern (UUIDv4->UUIDv7) --- .../tracers/opentelemetry/tracer.cc | 33 ++++++++++-- .../extensions/tracers/opentelemetry/tracer.h | 13 +++++ .../opentelemetry_tracer_impl_test.cc | 53 +++++++++++-------- 3 files changed, 73 insertions(+), 26 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/tracer.cc b/source/extensions/tracers/opentelemetry/tracer.cc index 66f9003cfcea3..f796cb277208c 100644 --- a/source/extensions/tracers/opentelemetry/tracer.cc +++ b/source/extensions/tracers/opentelemetry/tracer.cc @@ -5,6 +5,7 @@ #include "envoy/config/trace/v3/opentelemetry.pb.h" +#include "source/common/common/assert.h" #include "source/common/common/empty_string.h" #include "source/common/common/hex.h" #include "source/common/tracing/common_values.h" @@ -266,6 +267,34 @@ void Tracer::sendSpan(::opentelemetry::proto::trace::v1::Span& span) { } } +std::string Tracer::generateTraceIdV7() { + // UUIDv7 bit layout (RFC 9562): + // High 64 bits: [timestamp_ms (48)] [version (4)] [rand_a (12)] + // Low 64 bits: [variant (2)] [rand_b (62)] + + // Get current Unix timestamp in milliseconds. + const uint64_t timestamp_ms = static_cast( + std::chrono::duration_cast( + time_source_.systemTime().time_since_epoch()) + .count()); + // Timestamp must fit in 48 bits (valid until year ~10889). + ASSERT((timestamp_ms >> 48) == 0); + + // Generate random bits. + const uint64_t rand_a = random_.random() & 0x0FFFULL; // 12 bits + const uint64_t rand_b = random_.random() & 0x3FFFFFFFFFFFFFFFULL; // 62 bits + + // Assemble high 64 bits: [timestamp_ms(48)][version=7(4)][rand_a(12)] + constexpr uint64_t kVersion7 = 0x7ULL; + const uint64_t trace_id_high = (timestamp_ms << 16) | (kVersion7 << 12) | rand_a; + + // Assemble low 64 bits: [variant=2(2)][rand_b(62)] + constexpr uint64_t kVariantRfc4122 = 0x2ULL; + const uint64_t trace_id_low = (kVariantRfc4122 << 62) | rand_b; + + return absl::StrCat(Hex::uint64ToHex(trace_id_high), Hex::uint64ToHex(trace_id_low)); +} + Tracing::SpanPtr Tracer::startSpan(const std::string& operation_name, const StreamInfo::StreamInfo& stream_info, SystemTime start_time, Tracing::Decision tracing_decision, @@ -279,9 +308,7 @@ Tracing::SpanPtr Tracer::startSpan(const std::string& operation_name, // Create an Tracers::OpenTelemetry::Span class that will contain the OTel span. auto new_span = std::make_unique(operation_name, stream_info, start_time, time_source_, *this, span_kind, use_local_decision); - uint64_t trace_id_high = random_.random(); - uint64_t trace_id = random_.random(); - new_span->setTraceId(absl::StrCat(Hex::uint64ToHex(trace_id_high), Hex::uint64ToHex(trace_id))); + new_span->setTraceId(generateTraceIdV7()); uint64_t span_id = random_.random(); new_span->setId(Hex::uint64ToHex(span_id)); if (sampler_) { diff --git a/source/extensions/tracers/opentelemetry/tracer.h b/source/extensions/tracers/opentelemetry/tracer.h index 1f6e10ccf63ba..17cf51d7cfc45 100644 --- a/source/extensions/tracers/opentelemetry/tracer.h +++ b/source/extensions/tracers/opentelemetry/tracer.h @@ -65,6 +65,19 @@ class Tracer : Logger::Loggable { * Removes all spans from the span buffer and sends them to the collector. */ void flushSpans(); + /** + * Generates a 128-bit trace ID following UUIDv7 format (RFC 9562). + * + * UUIDv7 layout (128 bits): + * [0-47] 48-bit Unix timestamp (milliseconds) + * [48-51] 4-bit version (0111 = 7) + * [52-63] 12-bit rand_a + * [64-65] 2-bit variant (10) + * [66-127] 62-bit rand_b + * + * @return 32-character hex string (128-bit trace ID). + */ + std::string generateTraceIdV7(); OpenTelemetryTraceExporterPtr exporter_; Envoy::TimeSource& time_source_; diff --git a/test/extensions/tracers/opentelemetry/opentelemetry_tracer_impl_test.cc b/test/extensions/tracers/opentelemetry/opentelemetry_tracer_impl_test.cc index 628a73283724c..b22b7be9c5d5a 100644 --- a/test/extensions/tracers/opentelemetry/opentelemetry_tracer_impl_test.cc +++ b/test/extensions/tracers/opentelemetry/opentelemetry_tracer_impl_test.cc @@ -252,18 +252,20 @@ TEST_F(OpenTelemetryDriverTest, GenerateSpanContextWithoutHeadersTest) { Tracing::TestTraceContextImpl request_headers{ {":authority", "test.com"}, {":path", "/"}, {":method", "GET"}}; - // Mock the random call for generating trace and span IDs so we can check it later. - const uint64_t trace_id_high = 1; - const uint64_t trace_id_low = 2; + // Fix timestamp for deterministic UUIDv7 trace_id generation. + time_system_.setSystemTime(std::chrono::milliseconds(0)); + + // Mock the random calls for generating trace_id (UUIDv7: rand_a, rand_b) and span_id. + const uint64_t rand_a = 1; + const uint64_t rand_b = 2; const uint64_t new_span_id = 3; NiceMock& mock_random_generator_ = context_.server_factory_context_.api_.random_; - // The tracer should generate three random numbers for the trace high, trace low, and span id. { InSequence s; - EXPECT_CALL(mock_random_generator_, random()).WillOnce(Return(trace_id_high)); - EXPECT_CALL(mock_random_generator_, random()).WillOnce(Return(trace_id_low)); + EXPECT_CALL(mock_random_generator_, random()).WillOnce(Return(rand_a)); + EXPECT_CALL(mock_random_generator_, random()).WillOnce(Return(rand_b)); EXPECT_CALL(mock_random_generator_, random()).WillOnce(Return(new_span_id)); } @@ -278,8 +280,9 @@ TEST_F(OpenTelemetryDriverTest, GenerateSpanContextWithoutHeadersTest) { // Ends in 01 because span should be sampled. See // https://w3c.github.io/trace-context/#trace-flags. + // trace_id is UUIDv7: timestamp(0) + version(7) + rand_a(1) | variant(2) + rand_b(2) EXPECT_EQ(sampled_entry.has_value(), true); - EXPECT_EQ(sampled_entry.value(), "00-00000000000000010000000000000002-0000000000000003-01"); + EXPECT_EQ(sampled_entry.value(), "00-00000000000070018000000000000002-0000000000000003-01"); } // Verifies a span it not created when an invalid traceparent header is received @@ -606,6 +609,8 @@ TEST_F(OpenTelemetryDriverTest, ExportOTLPSpanWithAttributes) { setupValidDriver(); Tracing::TestTraceContextImpl request_headers{ {":authority", "test.com"}, {":path", "/"}, {":method", "GET"}}; + // Fix timestamp for deterministic UUIDv7 trace_id generation. + time_system_.setSystemTime(std::chrono::milliseconds(0)); NiceMock& mock_random_generator_ = context_.server_factory_context_.api_.random_; int64_t generated_int = 1; @@ -660,12 +665,11 @@ TEST_F(OpenTelemetryDriverTest, ExportOTLPSpanWithAttributes) { TestUtility::loadFromYaml(fmt::format(request_yaml, envoy_version, timestamp_ns, timestamp_ns), request_proto); - std::string generated_int_hex = Hex::uint64ToHex(generated_int); auto* expected_span = request_proto.mutable_resource_spans(0)->mutable_scope_spans(0)->mutable_spans(0); - expected_span->set_trace_id( - absl::HexStringToBytes(absl::StrCat(generated_int_hex, generated_int_hex))); - expected_span->set_span_id(absl::HexStringToBytes(absl::StrCat(generated_int_hex))); + // UUIDv7 trace_id: timestamp(0) + version(7) + rand_a(1) | variant(2) + rand_b(1) + expected_span->set_trace_id(absl::HexStringToBytes("00000000000070018000000000000001")); + expected_span->set_span_id(absl::HexStringToBytes(Hex::uint64ToHex(generated_int))); EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.opentelemetry.min_flush_spans", 5U)) .Times(1) @@ -682,6 +686,8 @@ TEST_F(OpenTelemetryDriverTest, ExportOTLPSpanWithAttributesAndStatus) { setupValidDriver(); Tracing::TestTraceContextImpl request_headers{ {":authority", "test.com"}, {":path", "/"}, {":method", "GET"}}; + // Fix timestamp for deterministic UUIDv7 trace_id generation. + time_system_.setSystemTime(std::chrono::milliseconds(0)); NiceMock& mock_random_generator_ = context_.server_factory_context_.api_.random_; int64_t generated_int = 1; @@ -742,12 +748,11 @@ TEST_F(OpenTelemetryDriverTest, ExportOTLPSpanWithAttributesAndStatus) { TestUtility::loadFromYaml(fmt::format(request_yaml, envoy_version, timestamp_ns, timestamp_ns), request_proto); - std::string generated_int_hex = Hex::uint64ToHex(generated_int); auto* expected_span = request_proto.mutable_resource_spans(0)->mutable_scope_spans(0)->mutable_spans(0); - expected_span->set_trace_id( - absl::HexStringToBytes(absl::StrCat(generated_int_hex, generated_int_hex))); - expected_span->set_span_id(absl::HexStringToBytes(absl::StrCat(generated_int_hex))); + // UUIDv7 trace_id: timestamp(0) + version(7) + rand_a(1) | variant(2) + rand_b(1) + expected_span->set_trace_id(absl::HexStringToBytes("00000000000070018000000000000001")); + expected_span->set_span_id(absl::HexStringToBytes(Hex::uint64ToHex(generated_int))); EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.opentelemetry.min_flush_spans", 5U)) .Times(1) @@ -764,6 +769,8 @@ TEST_F(OpenTelemetryDriverTest, ExportOTLPGRPCSpanWithAttributesAndStatus) { setupValidDriver(); Tracing::TestTraceContextImpl request_headers{ {":authority", "test.com"}, {":path", "/"}, {":method", "GET"}}; + // Fix timestamp for deterministic UUIDv7 trace_id generation. + time_system_.setSystemTime(std::chrono::milliseconds(0)); NiceMock& mock_random_generator_ = context_.server_factory_context_.api_.random_; int64_t generated_int = 1; @@ -832,12 +839,11 @@ TEST_F(OpenTelemetryDriverTest, ExportOTLPGRPCSpanWithAttributesAndStatus) { TestUtility::loadFromYaml(fmt::format(request_yaml, envoy_version, timestamp_ns, timestamp_ns), request_proto); - std::string generated_int_hex = Hex::uint64ToHex(generated_int); auto* expected_span = request_proto.mutable_resource_spans(0)->mutable_scope_spans(0)->mutable_spans(0); - expected_span->set_trace_id( - absl::HexStringToBytes(absl::StrCat(generated_int_hex, generated_int_hex))); - expected_span->set_span_id(absl::HexStringToBytes(absl::StrCat(generated_int_hex))); + // UUIDv7 trace_id: timestamp(0) + version(7) + rand_a(1) | variant(2) + rand_b(1) + expected_span->set_trace_id(absl::HexStringToBytes("00000000000070018000000000000001")); + expected_span->set_span_id(absl::HexStringToBytes(Hex::uint64ToHex(generated_int))); EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.opentelemetry.min_flush_spans", 5U)) .Times(1) @@ -947,6 +953,8 @@ TEST_F(OpenTelemetryDriverTest, ExportSpanWithCustomServiceName) { Tracing::TestTraceContextImpl request_headers{ {":authority", "test.com"}, {":path", "/"}, {":method", "GET"}}; + // Fix timestamp for deterministic UUIDv7 trace_id generation. + time_system_.setSystemTime(std::chrono::milliseconds(0)); NiceMock& mock_random_generator_ = context_.server_factory_context_.api_.random_; int64_t generated_int = 1; @@ -986,12 +994,11 @@ TEST_F(OpenTelemetryDriverTest, ExportSpanWithCustomServiceName) { TestUtility::loadFromYaml(fmt::format(request_yaml, envoy_version, timestamp_ns, timestamp_ns), request_proto); - std::string generated_int_hex = Hex::uint64ToHex(generated_int); auto* expected_span = request_proto.mutable_resource_spans(0)->mutable_scope_spans(0)->mutable_spans(0); - expected_span->set_trace_id( - absl::HexStringToBytes(absl::StrCat(generated_int_hex, generated_int_hex))); - expected_span->set_span_id(absl::HexStringToBytes(absl::StrCat(generated_int_hex))); + // UUIDv7 trace_id: timestamp(0) + version(7) + rand_a(1) | variant(2) + rand_b(1) + expected_span->set_trace_id(absl::HexStringToBytes("00000000000070018000000000000001")); + expected_span->set_span_id(absl::HexStringToBytes(Hex::uint64ToHex(generated_int))); EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.opentelemetry.min_flush_spans", 5U)) .Times(1) From 7deed7ca24d0be75451e0463704a7eaea42b1751 Mon Sep 17 00:00:00 2001 From: ku524 Date: Thu, 15 Jan 2026 01:00:01 +0900 Subject: [PATCH 07/12] [CPLAT-8549] Add TRACE_SAMPLED access log formatter Add TraceSampledFormatter that uses Envoy's internal tracing decision (stream_info.traceReason()) This approach works correctly at trace origin points (e.g., Istio Ingress Gateway) where no incoming traceparent header exists. Usage: %TRACE_SAMPLED% in access log format Returns: "true" if traced, "false" otherwise Co-Authored-By: Claude Opus 4.5 --- .../formatter/http_specific_formatter.cc | 36 +++++++++++++++ .../formatter/http_specific_formatter.h | 14 ++++++ .../formatter/substitution_formatter_test.cc | 44 +++++++++++++++++++ 3 files changed, 94 insertions(+) diff --git a/source/common/formatter/http_specific_formatter.cc b/source/common/formatter/http_specific_formatter.cc index d924af5626e7a..849831f52ebbb 100644 --- a/source/common/formatter/http_specific_formatter.cc +++ b/source/common/formatter/http_specific_formatter.cc @@ -182,6 +182,37 @@ TraceIDFormatter::formatWithContext(const HttpFormatterContext& context, return trace_id; } +namespace { +// Determines if the request is being traced based on stream info. +// This is equivalent to TracerUtility::shouldTraceRequest but inlined +// to avoid dependency on tracer_lib. +bool isTraced(const StreamInfo::StreamInfo& stream_info) { + if (stream_info.healthCheck()) { + return false; + } + switch (stream_info.traceReason()) { + case Tracing::Reason::ClientForced: + case Tracing::Reason::ServiceForced: + case Tracing::Reason::Sampling: + return true; + default: + return false; + } +} +} // namespace + +absl::optional +TraceSampledFormatter::formatWithContext(const HttpFormatterContext&, + const StreamInfo::StreamInfo& stream_info) const { + return isTraced(stream_info) ? "true" : "false"; +} + +Protobuf::Value +TraceSampledFormatter::formatValueWithContext(const HttpFormatterContext&, + const StreamInfo::StreamInfo& stream_info) const { + return ValueUtil::stringValue(isTraced(stream_info) ? "true" : "false"); +} + GrpcStatusFormatter::Format GrpcStatusFormatter::parseFormat(absl::string_view format) { if (format.empty() || format == "CAMEL_STRING") { return GrpcStatusFormatter::CamelString; @@ -454,6 +485,11 @@ BuiltInHttpCommandParser::getKnownFormatters() { [](absl::string_view, absl::optional) { return std::make_unique(); }}}, + {"TRACE_SAMPLED", + {CommandSyntaxChecker::COMMAND_ONLY, + [](absl::string_view, absl::optional) { + return std::make_unique(); + }}}, {"QUERY_PARAM", {CommandSyntaxChecker::PARAMS_REQUIRED | CommandSyntaxChecker::LENGTH_ALLOWED, [](absl::string_view format, absl::optional max_length) { diff --git a/source/common/formatter/http_specific_formatter.h b/source/common/formatter/http_specific_formatter.h index f0ba656f3ed64..3e694da54c9dc 100644 --- a/source/common/formatter/http_specific_formatter.h +++ b/source/common/formatter/http_specific_formatter.h @@ -149,6 +149,20 @@ class TraceIDFormatter : public FormatterProvider { const StreamInfo::StreamInfo& stream_info) const override; }; +/** + * FormatterProvider for trace sampled status. + * Uses Envoy's internal tracing decision (stream_info.traceReason()). + * Works at trace origin (e.g., Istio Ingress Gateway) where no incoming traceparent header exists. + */ +class TraceSampledFormatter : public FormatterProvider { +public: + absl::optional + formatWithContext(const HttpFormatterContext& context, + const StreamInfo::StreamInfo& stream_info) const override; + Protobuf::Value formatValueWithContext(const HttpFormatterContext& context, + const StreamInfo::StreamInfo& stream_info) const override; +}; + class GrpcStatusFormatter : public FormatterProvider, HeaderFormatter { public: enum Format { diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index b7d5f6247bd25..c86a53b4df60e 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -2900,6 +2900,50 @@ TEST(SubstitutionFormatterTest, TraceIDFormatter) { } } +TEST(SubstitutionFormatterTest, TraceSampledFormatter) { + StreamInfo::MockStreamInfo stream_info; + Http::TestRequestHeaderMapImpl request_header{}; + Http::TestResponseHeaderMapImpl response_header{}; + Http::TestResponseTrailerMapImpl response_trailer{}; + std::string body; + HttpFormatterContext formatter_context(&request_header, &response_header, &response_trailer, + body); + TraceSampledFormatter formatter{}; + + // Test traced=true cases: Sampling, ClientForced, ServiceForced + { + EXPECT_CALL(stream_info, healthCheck()).WillRepeatedly(Return(false)); + EXPECT_CALL(stream_info, traceReason()).WillRepeatedly(Return(Tracing::Reason::Sampling)); + EXPECT_EQ("true", formatter.formatWithContext(formatter_context, stream_info)); + EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info), + ProtoEq(ValueUtil::stringValue("true"))); + } + { + EXPECT_CALL(stream_info, healthCheck()).WillOnce(Return(false)); + EXPECT_CALL(stream_info, traceReason()).WillOnce(Return(Tracing::Reason::ClientForced)); + EXPECT_EQ("true", formatter.formatWithContext(formatter_context, stream_info)); + } + { + EXPECT_CALL(stream_info, healthCheck()).WillOnce(Return(false)); + EXPECT_CALL(stream_info, traceReason()).WillOnce(Return(Tracing::Reason::ServiceForced)); + EXPECT_EQ("true", formatter.formatWithContext(formatter_context, stream_info)); + } + + // Test traced=false cases: NotTraceable, HealthCheck + { + EXPECT_CALL(stream_info, healthCheck()).WillRepeatedly(Return(false)); + EXPECT_CALL(stream_info, traceReason()).WillRepeatedly(Return(Tracing::Reason::NotTraceable)); + EXPECT_EQ("false", formatter.formatWithContext(formatter_context, stream_info)); + EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info), + ProtoEq(ValueUtil::stringValue("false"))); + } + { + // HealthCheck requests should return false regardless of traceReason + EXPECT_CALL(stream_info, healthCheck()).WillOnce(Return(true)); + EXPECT_EQ("false", formatter.formatWithContext(formatter_context, stream_info)); + } +} + /** * Populate a metadata object with the following test data: * "com.test": {"test_key":"test_value","test_obj":{"inner_key":"inner_value"}} From c2b2700bb57fc8b5c0e86a3c2d35774f7164f839 Mon Sep 17 00:00:00 2001 From: ku524 Date: Thu, 15 Jan 2026 17:49:31 +0900 Subject: [PATCH 08/12] [CPLAT-8549] Change request_id format (UUIDv4->custom UUIDv7) --- source/extensions/request_id/uuid/BUILD | 1 + source/extensions/request_id/uuid/config.cc | 79 ++++++- source/extensions/request_id/uuid/config.h | 34 ++- source/server/admin/admin.cc | 2 +- test/extensions/request_id/uuid/BUILD | 1 + .../extensions/request_id/uuid/config_test.cc | 200 ++++++++++++------ 6 files changed, 238 insertions(+), 79 deletions(-) diff --git a/source/extensions/request_id/uuid/BUILD b/source/extensions/request_id/uuid/BUILD index 7e2fd0ffb3b8c..9243aac3b277e 100644 --- a/source/extensions/request_id/uuid/BUILD +++ b/source/extensions/request_id/uuid/BUILD @@ -20,6 +20,7 @@ envoy_cc_extension( deps = [ "//envoy/http:request_id_extension_interface", "//envoy/server:request_id_extension_config_interface", + "//source/common/common:hex_lib", "//source/common/common:random_generator_lib", "//source/common/stream_info:stream_id_provider_lib", "@envoy_api//envoy/extensions/request_id/uuid/v3:pkg_cc_proto", diff --git a/source/extensions/request_id/uuid/config.cc b/source/extensions/request_id/uuid/config.cc index be75b13b763e4..3c274befb44be 100644 --- a/source/extensions/request_id/uuid/config.cc +++ b/source/extensions/request_id/uuid/config.cc @@ -1,12 +1,17 @@ #include "source/extensions/request_id/uuid/config.h" +#include + #include "envoy/http/header_map.h" #include "envoy/tracing/tracer.h" +#include "source/common/common/hex.h" #include "source/common/common/random_generator.h" #include "source/common/common/utility.h" #include "source/common/stream_info/stream_id_provider_impl.h" +#include "absl/strings/str_cat.h" + namespace Envoy { namespace Extensions { namespace RequestId { @@ -17,7 +22,7 @@ void UUIDRequestIDExtension::set(Http::RequestHeaderMap& request_headers, bool e // No request ID then set new one anyway. if (request_id_header == nullptr || request_id_header->value().empty()) { - request_headers.setRequestId(random_.uuid()); + request_headers.setRequestId(generateUuidV7()); return; } @@ -32,7 +37,7 @@ void UUIDRequestIDExtension::set(Http::RequestHeaderMap& request_headers, bool e if (!keep_external_id) { // If we are not keeping external request ID, then set new one anyway. - request_headers.setRequestId(random_.uuid()); + request_headers.setRequestId(generateUuidV7()); return; } @@ -64,12 +69,15 @@ UUIDRequestIDExtension::getInteger(const Http::RequestHeaderMap& request_headers return absl::nullopt; } const std::string uuid(request_headers.getRequestIdValue()); - if (uuid.length() < 8) { + // UUID format: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx (36 chars) + // For UUIDv7: first 8 chars contain timestamp, use last 8 hex chars (rand_b) for randomness. + // Position 28-35 contains the last 8 hex digits. + if (uuid.length() < 36) { return absl::nullopt; } uint64_t value; - if (!StringUtil::atoull(uuid.substr(0, 8).c_str(), value, 16)) { + if (!StringUtil::atoull(uuid.substr(28, 8).c_str(), value, 16)) { return absl::nullopt; } @@ -95,6 +103,8 @@ UUIDRequestIDExtension::getTraceReason(const Http::RequestHeaderMap& request_hea return Tracing::Reason::Sampling; case TRACE_CLIENT: return Tracing::Reason::ClientForced; + case NO_TRACE_V4: // UUIDv4 freshly generated (version bit '4') + case NO_TRACE_V7: // UUIDv7 freshly generated (version bit '7') default: return Tracing::Reason::NotTraceable; } @@ -122,7 +132,15 @@ void UUIDRequestIDExtension::setTraceReason(Http::RequestHeaderMap& request_head uuid[TRACE_BYTE_POSITION] = TRACE_SAMPLED; break; case Tracing::Reason::NotTraceable: - uuid[TRACE_BYTE_POSITION] = NO_TRACE; + // Preserve the original version bit ('4' for UUIDv4, '7' for UUIDv7). + // Only reset if currently set to a trace reason marker. + if (uuid[TRACE_BYTE_POSITION] == TRACE_SAMPLED || + uuid[TRACE_BYTE_POSITION] == TRACE_FORCED || + uuid[TRACE_BYTE_POSITION] == TRACE_CLIENT) { + // Default to UUIDv7 version bit when clearing trace reason. + uuid[TRACE_BYTE_POSITION] = NO_TRACE_V7; + } + // If already NO_TRACE_V4 or NO_TRACE_V7, leave unchanged. break; default: break; @@ -130,6 +148,57 @@ void UUIDRequestIDExtension::setTraceReason(Http::RequestHeaderMap& request_head request_headers.setRequestId(uuid); } +std::string UUIDRequestIDExtension::generateUuidV7() { + // Modified UUIDv7: 상위 4비트를 0xF 마커로 설정하여 trace_id와 구분 + // request_id: fxxxxxxx-xxxx-7xxx-yxxx-xxxxxxxxxxxx ('f'로 시작) + // trace_id: 0xxxxxxx-xxxx-7xxx-yxxx-xxxxxxxxxxxx ('0'으로 시작) + // + // Bit layout: + // [0-3] 4-bit marker (1111 = 0xF for request_id) + // [4-47] 44-bit Unix timestamp in milliseconds (~557 years until ~2527 AD) + // [48-51] 4-bit version (0111 = 7) + // [52-63] 12-bit rand_a + // [64-65] 2-bit variant (10 = RFC 4122) + // [66-127] 62-bit rand_b + // + // Result format: fxxxxxxx-xxxx-7xxx-yxxx-xxxxxxxxxxxx + // where y is one of [8, 9, a, b] (variant bits). + + const uint64_t timestamp_ms = static_cast( + std::chrono::duration_cast( + time_source_.systemTime().time_since_epoch()) + .count()); + + // 실제 timestamp는 44비트만 사용 (상위 4비트는 마커용) + // 44비트로 약 557년 표현 가능 (서기 ~2527년까지) + const uint64_t timestamp_44bit = timestamp_ms & 0x0FFFFFFFFFFFULL; + + // 상위 4비트에 0xF 마커 설정 → request_id가 'f'로 시작하게 됨 + constexpr uint64_t kRequestIdMarker = 0xFULL; + const uint64_t marked_timestamp = (kRequestIdMarker << 44) | timestamp_44bit; + + // rand_a: 12 bits of randomness for sub-millisecond ordering + const uint64_t rand_a = random_.random() & 0x0FFFULL; + // rand_b: 62 bits of randomness (2 bits reserved for variant) + const uint64_t rand_b = random_.random() & 0x3FFFFFFFFFFFFFFFULL; + + constexpr uint64_t kVersion7 = 0x7ULL; + constexpr uint64_t kVariantRfc4122 = 0x2ULL; + + // Build high 64 bits: marked_timestamp (48) | version (4) | rand_a (12) + const uint64_t uuid_high = (marked_timestamp << 16) | (kVersion7 << 12) | rand_a; + // Build low 64 bits: variant (2) | rand_b (62) + const uint64_t uuid_low = (kVariantRfc4122 << 62) | rand_b; + + // Convert to hex strings (16 chars each, zero-padded) + std::string high_hex = Hex::uint64ToHex(uuid_high); + std::string low_hex = Hex::uint64ToHex(uuid_low); + + // Format: fxxxxxxx-xxxx-7xxx-yxxx-xxxxxxxxxxxx + return absl::StrCat(high_hex.substr(0, 8), "-", high_hex.substr(8, 4), "-", + high_hex.substr(12, 4), "-", low_hex.substr(0, 4), "-", low_hex.substr(4, 12)); +} + REGISTER_FACTORY(UUIDRequestIDExtensionFactory, Server::Configuration::RequestIDExtensionFactory); } // namespace RequestId diff --git a/source/extensions/request_id/uuid/config.h b/source/extensions/request_id/uuid/config.h index 92c1ea86dc2a5..c2f2ed7812acf 100644 --- a/source/extensions/request_id/uuid/config.h +++ b/source/extensions/request_id/uuid/config.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/common/time.h" #include "envoy/extensions/request_id/uuid/v3/uuid.pb.h" #include "envoy/extensions/request_id/uuid/v3/uuid.pb.validate.h" #include "envoy/http/request_id_extension.h" @@ -14,15 +15,16 @@ namespace RequestId { class UUIDRequestIDExtension : public Envoy::Http::RequestIDExtension { public: UUIDRequestIDExtension(const envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig& config, - Random::RandomGenerator& random) - : random_(random), + Random::RandomGenerator& random, TimeSource& time_source) + : random_(random), time_source_(time_source), pack_trace_reason_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, pack_trace_reason, true)), use_request_id_for_trace_sampling_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, use_request_id_for_trace_sampling, true)) {} - static Envoy::Http::RequestIDExtensionSharedPtr defaultInstance(Random::RandomGenerator& random) { + static Envoy::Http::RequestIDExtensionSharedPtr + defaultInstance(Random::RandomGenerator& random, TimeSource& time_source) { return std::make_shared( - envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), random); + envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), random, time_source); } bool packTraceReason() { return pack_trace_reason_; } @@ -42,7 +44,22 @@ class UUIDRequestIDExtension : public Envoy::Http::RequestIDExtension { bool useRequestIdForTraceSampling() const override { return use_request_id_for_trace_sampling_; } private: + /** + * Generates a 128-bit request ID following UUIDv7 format (RFC 9562). + * + * UUIDv7 layout (128 bits): + * [0-47] 48-bit Unix timestamp (milliseconds) + * [48-51] 4-bit version (0111 = 7) + * [52-63] 12-bit rand_a + * [64-65] 2-bit variant (10) + * [66-127] 62-bit rand_b + * + * @return 36-character UUID string with hyphens. + */ + std::string generateUuidV7(); + Envoy::Random::RandomGenerator& random_; + Envoy::TimeSource& time_source_; const bool pack_trace_reason_; const bool use_request_id_for_trace_sampling_; @@ -61,8 +78,10 @@ class UUIDRequestIDExtension : public Envoy::Http::RequestIDExtension { // one modified because of client trace. static const char TRACE_CLIENT = 'b'; - // Initial value for freshly generated uuid4. - static const char NO_TRACE = '4'; + // Initial value for freshly generated UUIDv4. + static const char NO_TRACE_V4 = '4'; + // Initial value for freshly generated UUIDv7. + static const char NO_TRACE_V7 = '7'; }; // Factory for the UUID request ID extension. @@ -79,7 +98,8 @@ class UUIDRequestIDExtensionFactory : public Server::Configuration::RequestIDExt MessageUtil::downcastAndValidate< const envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig&>( config, context.messageValidationVisitor()), - context.serverFactoryContext().api().randomGenerator()); + context.serverFactoryContext().api().randomGenerator(), + context.serverFactoryContext().api().timeSource()); } }; diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index d4b308c760d18..87d0a4f46f6b5 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -110,7 +110,7 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server, : server_(server), listener_info_(std::make_shared()), factory_context_(server, listener_info_), request_id_extension_(Extensions::RequestId::UUIDRequestIDExtension::defaultInstance( - server_.api().randomGenerator())), + server_.api().randomGenerator(), server_.api().timeSource())), profile_path_(profile_path), stats_(Http::ConnectionManagerImpl::generateStats( "http.admin.", *server_.stats().rootScope())), null_overload_manager_(server.threadLocal(), false), diff --git a/test/extensions/request_id/uuid/BUILD b/test/extensions/request_id/uuid/BUILD index a139870f1828a..029b3cf91d184 100644 --- a/test/extensions/request_id/uuid/BUILD +++ b/test/extensions/request_id/uuid/BUILD @@ -18,5 +18,6 @@ envoy_cc_test( "//source/extensions/request_id/uuid:config", "//test/mocks/runtime:runtime_mocks", "//test/mocks/stream_info:stream_info_mocks", + "//test/test_common:simulated_time_system_lib", ], ) diff --git a/test/extensions/request_id/uuid/config_test.cc b/test/extensions/request_id/uuid/config_test.cc index 277519addee8d..054ab1f5699e9 100644 --- a/test/extensions/request_id/uuid/config_test.cc +++ b/test/extensions/request_id/uuid/config_test.cc @@ -5,6 +5,7 @@ #include "test/mocks/common.h" #include "test/mocks/stream_info/mocks.h" +#include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -16,24 +17,30 @@ namespace Extensions { namespace RequestId { TEST(UUIDRequestIDExtensionTest, SetRequestID) { - testing::StrictMock random; + testing::NiceMock random; + Event::SimulatedTimeSystem time_system; UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), - random); + random, time_system); { // edge_request: true, keep_external_id: false. Http::TestRequestHeaderMapImpl request_headers; - // Without request ID. - EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id")); + // Without request ID. generateUuidV7() calls random.random() twice. uuid_utils.set(request_headers, true, false); - EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId)); + std::string first_request_id = std::string(request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(36, first_request_id.length()); + EXPECT_EQ('f', first_request_id[0]); // UUIDv7 request_id marker + EXPECT_EQ('7', first_request_id[14]); // UUIDv7 version // With request ID. Previous one will be overwritten. - EXPECT_CALL(random, uuid()).WillOnce(Return("second-request-id")); uuid_utils.set(request_headers, true, false); - EXPECT_EQ("second-request-id", request_headers.get_(Http::Headers::get().RequestId)); + std::string second_request_id = + std::string(request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(36, second_request_id.length()); + EXPECT_EQ('f', second_request_id[0]); + EXPECT_EQ('7', second_request_id[14]); } { @@ -41,15 +48,14 @@ TEST(UUIDRequestIDExtensionTest, SetRequestID) { Http::TestRequestHeaderMapImpl request_headers; - // Without request ID. - EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id")); + // Without request ID. generateUuidV7() calls random.random() twice. uuid_utils.set(request_headers, true, true); - EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId)); + std::string first_request_id = std::string(request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(36, first_request_id.length()); // With request ID. Previous one will be kept. - EXPECT_CALL(random, uuid()).Times(0); uuid_utils.set(request_headers, true, true); - EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(first_request_id, request_headers.get_(Http::Headers::get().RequestId)); } { @@ -57,15 +63,14 @@ TEST(UUIDRequestIDExtensionTest, SetRequestID) { Http::TestRequestHeaderMapImpl request_headers; - // Without request ID. - EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id")); + // Without request ID. generateUuidV7() calls random.random() twice. uuid_utils.set(request_headers, false, false); - EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId)); + std::string first_request_id = std::string(request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(36, first_request_id.length()); // With request ID. Previous one will be kept. - EXPECT_CALL(random, uuid()).Times(0); uuid_utils.set(request_headers, false, false); - EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(first_request_id, request_headers.get_(Http::Headers::get().RequestId)); } { @@ -73,32 +78,34 @@ TEST(UUIDRequestIDExtensionTest, SetRequestID) { Http::TestRequestHeaderMapImpl request_headers; - // Without request ID. - EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id")); + // Without request ID. generateUuidV7() calls random.random() twice. uuid_utils.set(request_headers, false, true); - EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId)); + std::string first_request_id = std::string(request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(36, first_request_id.length()); // With request ID. Previous one will be kept. - EXPECT_CALL(random, uuid()).Times(0); uuid_utils.set(request_headers, false, true); - EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(first_request_id, request_headers.get_(Http::Headers::get().RequestId)); } } TEST(UUIDRequestIDExtensionTest, SetRequestIDWhenEmpty) { - testing::StrictMock random; + testing::NiceMock random; + Event::SimulatedTimeSystem time_system; UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), - random); + random, time_system); { // Request ID not set. Http::TestRequestHeaderMapImpl request_headers; - // A new request ID will be set. - EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id")); + // A new request ID will be set. generateUuidV7() calls random.random() twice. uuid_utils.set(request_headers, false, true); - EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId)); + std::string first_request_id = std::string(request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(36, first_request_id.length()); + EXPECT_EQ('f', first_request_id[0]); // UUIDv7 request_id marker + EXPECT_EQ('7', first_request_id[14]); // UUIDv7 version } { @@ -109,10 +116,12 @@ TEST(UUIDRequestIDExtensionTest, SetRequestIDWhenEmpty) { "", }}; - // A new request ID will be set. - EXPECT_CALL(random, uuid()).WillOnce(Return("first-request-id")); + // A new request ID will be set. generateUuidV7() calls random.random() twice. uuid_utils.set(request_headers, false, true); - EXPECT_EQ("first-request-id", request_headers.get_(Http::Headers::get().RequestId)); + std::string first_request_id = std::string(request_headers.get_(Http::Headers::get().RequestId)); + EXPECT_EQ(36, first_request_id.length()); + EXPECT_EQ('f', first_request_id[0]); + EXPECT_EQ('7', first_request_id[14]); } { @@ -124,7 +133,6 @@ TEST(UUIDRequestIDExtensionTest, SetRequestIDWhenEmpty) { }}; // The request ID will be kept. - EXPECT_CALL(random, uuid()).Times(0); uuid_utils.set(request_headers, false, true); EXPECT_EQ("some-request-id", request_headers.get_(Http::Headers::get().RequestId)); } @@ -132,8 +140,9 @@ TEST(UUIDRequestIDExtensionTest, SetRequestIDWhenEmpty) { TEST(UUIDRequestIDExtensionTest, ClearExternalTraceReason) { testing::NiceMock random; + Event::SimulatedTimeSystem time_system; UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), - random); + random, time_system); std::string uuid_with_trace_reason = random.uuid_; @@ -147,9 +156,8 @@ TEST(UUIDRequestIDExtensionTest, ClearExternalTraceReason) { }}; std::string expected_uuid_with_trace_reason = uuid_with_trace_reason; - expected_uuid_with_trace_reason[14] = '4'; // 'b' means NO_TRACE. + expected_uuid_with_trace_reason[14] = '7'; // Clear to NO_TRACE (UUIDv7 version bit). - EXPECT_CALL(random, uuid()).Times(0); uuid_utils.set(request_headers, true, true); // External request ID will be kept but the trace reason will be cleared. @@ -157,9 +165,10 @@ TEST(UUIDRequestIDExtensionTest, ClearExternalTraceReason) { } TEST(UUIDRequestIDExtensionTest, PreserveRequestIDInResponse) { - testing::StrictMock random; + testing::NiceMock random; + Event::SimulatedTimeSystem time_system; UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), - random); + random, time_system); Http::TestRequestHeaderMapImpl request_headers; Http::TestResponseHeaderMapImpl response_headers; @@ -181,63 +190,77 @@ TEST(UUIDRequestIDExtensionTest, PreserveRequestIDInResponse) { } TEST(UUIDRequestIDExtensionTest, GetRequestIdAndModRequestIDBy) { - Random::RandomGeneratorImpl random; + testing::NiceMock random; + Event::SimulatedTimeSystem time_system; UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), - random); + random, time_system); Http::TestRequestHeaderMapImpl request_headers; EXPECT_FALSE(uuid_utils.get(request_headers)); EXPECT_FALSE(uuid_utils.getInteger(request_headers).has_value()); + // UUID too short (< 36 chars). request_headers.setRequestId("fffffff"); EXPECT_EQ("fffffff", uuid_utils.get(request_headers).value()); EXPECT_FALSE(uuid_utils.getInteger(request_headers).has_value()); - request_headers.setRequestId("fffffffz-0012-0110-00ff-0c00400600ff"); - EXPECT_EQ("fffffffz-0012-0110-00ff-0c00400600ff", uuid_utils.get(request_headers).value()); + // UUID with invalid hex char 'z' in the last 8 chars (position 28-35). + request_headers.setRequestId("fffffffz-0012-0110-00ff-0c00400600fz"); + EXPECT_EQ("fffffffz-0012-0110-00ff-0c00400600fz", uuid_utils.get(request_headers).value()); EXPECT_FALSE(uuid_utils.getInteger(request_headers).has_value()); + // getInteger() now uses last 8 hex chars (position 28-35). + // UUID: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx + // 0 8 13 18 23 28 35 + // ^^^^^^^^ (last 8 hex chars) + + // "00000000-0000-0000-0000-000000000000" -> last 8 chars = "00000000" = 0 request_headers.setRequestId("00000000-0000-0000-0000-000000000000"); EXPECT_EQ("00000000-0000-0000-0000-000000000000", uuid_utils.get(request_headers).value()); EXPECT_EQ(0, uuid_utils.getInteger(request_headers).value()); - request_headers.setRequestId("00000001-0000-0000-0000-000000000000"); - EXPECT_EQ("00000001-0000-0000-0000-000000000000", uuid_utils.get(request_headers).value()); + // "00000001-0000-0000-0000-000000000001" -> last 8 chars = "00000001" = 1 + request_headers.setRequestId("00000001-0000-0000-0000-000000000001"); + EXPECT_EQ("00000001-0000-0000-0000-000000000001", uuid_utils.get(request_headers).value()); EXPECT_EQ(1, uuid_utils.getInteger(request_headers).value()); - request_headers.setRequestId("0000000f-0000-0000-0000-00000000000a"); - EXPECT_EQ("0000000f-0000-0000-0000-00000000000a", uuid_utils.get(request_headers).value()); - EXPECT_EQ(15, uuid_utils.getInteger(request_headers).value()); + // "0000000f-0000-0000-0000-0000000000ff" -> last 8 chars = "000000ff" = 255 + request_headers.setRequestId("0000000f-0000-0000-0000-0000000000ff"); + EXPECT_EQ("0000000f-0000-0000-0000-0000000000ff", uuid_utils.get(request_headers).value()); + EXPECT_EQ(255, uuid_utils.getInteger(request_headers).value()); request_headers.setRequestId(""); EXPECT_EQ("", uuid_utils.get(request_headers).value()); EXPECT_FALSE(uuid_utils.getInteger(request_headers).has_value()); - request_headers.setRequestId("000000ff-0000-0000-0000-000000000000"); - EXPECT_EQ("000000ff-0000-0000-0000-000000000000", uuid_utils.get(request_headers).value()); - EXPECT_EQ(55, uuid_utils.getInteger(request_headers).value() % 100); - - request_headers.setRequestId("000000ff-0000-0000-0000-000000000000"); - EXPECT_EQ("000000ff-0000-0000-0000-000000000000", uuid_utils.get(request_headers).value()); - EXPECT_EQ(255, uuid_utils.getInteger(request_headers).value()); + // "000000ff-0000-0000-0000-000012345678" -> last 8 chars = "12345678" = 0x12345678 + request_headers.setRequestId("000000ff-0000-0000-0000-000012345678"); + EXPECT_EQ("000000ff-0000-0000-0000-000012345678", uuid_utils.get(request_headers).value()); + EXPECT_EQ(0x12345678, uuid_utils.getInteger(request_headers).value()); + // "a0090100-0012-0110-00ff-0c00400600ff" -> last 8 chars = "400600ff" = 0x400600ff request_headers.setRequestId("a0090100-0012-0110-00ff-0c00400600ff"); EXPECT_EQ("a0090100-0012-0110-00ff-0c00400600ff", uuid_utils.get(request_headers).value()); - EXPECT_EQ(8, uuid_utils.getInteger(request_headers).value() % 137); + EXPECT_EQ(0x400600ff, uuid_utils.getInteger(request_headers).value()); - request_headers.setRequestId("ffffffff-0012-0110-00ff-0c00400600ff"); - EXPECT_EQ("ffffffff-0012-0110-00ff-0c00400600ff", uuid_utils.get(request_headers).value()); - EXPECT_EQ(95, uuid_utils.getInteger(request_headers).value() % 100); + // "ffffffff-0012-0110-00ff-0c00ffffffff" -> last 8 chars = "ffffffff" = 0xffffffff + request_headers.setRequestId("ffffffff-0012-0110-00ff-0c00ffffffff"); + EXPECT_EQ("ffffffff-0012-0110-00ff-0c00ffffffff", uuid_utils.get(request_headers).value()); + EXPECT_EQ(0xffffffff, uuid_utils.getInteger(request_headers).value()); + // Test modulo operations for sampling distribution. + // "ffffffff-0012-0110-00ff-0c00400600ff" -> last 8 chars = "400600ff" = 0x400600ff = 1073873151 request_headers.setRequestId("ffffffff-0012-0110-00ff-0c00400600ff"); EXPECT_EQ("ffffffff-0012-0110-00ff-0c00400600ff", uuid_utils.get(request_headers).value()); - EXPECT_EQ(7295, uuid_utils.getInteger(request_headers).value() % 10000); + EXPECT_EQ(1073873151 % 100, uuid_utils.getInteger(request_headers).value() % 100); + EXPECT_EQ(1073873151 % 10000, uuid_utils.getInteger(request_headers).value() % 10000); } TEST(UUIDRequestIDExtensionTest, RequestIDModDistribution) { Random::RandomGeneratorImpl random; + Event::SimulatedTimeSystem time_system; UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), - random); + random, time_system); Http::TestRequestHeaderMapImpl request_headers; const int mod = 100; @@ -246,13 +269,15 @@ TEST(UUIDRequestIDExtensionTest, RequestIDModDistribution) { int interesting_samples = 0; for (int i = 0; i < 500000; ++i) { - std::string uuid = random.uuid(); + // Generate UUIDv7 via uuid_utils.set() instead of random.uuid(). + uuid_utils.set(request_headers, true, false); + std::string uuid = std::string(request_headers.getRequestIdValue()); const char c = uuid[19]; - ASSERT_TRUE(uuid[14] == '4'); // UUID version 4 (random) + ASSERT_TRUE(uuid[0] == 'f'); // Request ID marker + ASSERT_TRUE(uuid[14] == '7'); // UUID version 7 ASSERT_TRUE(c == '8' || c == '9' || c == 'a' || c == 'b'); // UUID variant 1 (RFC4122) - request_headers.setRequestId(uuid); const uint64_t value = uuid_utils.getInteger(request_headers).value() % mod; if (value < required_percentage) { @@ -266,18 +291,25 @@ TEST(UUIDRequestIDExtensionTest, RequestIDModDistribution) { TEST(UUIDRequestIDExtensionTest, DISABLED_benchmark) { Random::RandomGeneratorImpl random; + Event::SimulatedTimeSystem time_system; + UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), + random, time_system); + Http::TestRequestHeaderMapImpl request_headers; for (int i = 0; i < 100000000; ++i) { - random.uuid(); + uuid_utils.set(request_headers, true, false); } } TEST(UUIDRequestIDExtensionTest, SetTraceStatus) { Random::RandomGeneratorImpl random; + Event::SimulatedTimeSystem time_system; UUIDRequestIDExtension uuid_utils(envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), - random); + random, time_system); Http::TestRequestHeaderMapImpl request_headers; - request_headers.setRequestId(random.uuid()); + + // Generate UUIDv7 via uuid_utils.set() instead of random.uuid(). + uuid_utils.set(request_headers, true, false); EXPECT_EQ(Tracing::Reason::NotTraceable, uuid_utils.getTraceReason(request_headers)); @@ -301,11 +333,15 @@ TEST(UUIDRequestIDExtensionTest, SetTraceStatus) { TEST(UUIDRequestIDExtensionTest, SetTraceStatusPackingDisabled) { Random::RandomGeneratorImpl random; + Event::SimulatedTimeSystem time_system; envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig config; config.mutable_pack_trace_reason()->set_value(false); - UUIDRequestIDExtension uuid_utils(config, random); + UUIDRequestIDExtension uuid_utils(config, random, time_system); - std::string uuid_with_trace_reason = random.uuid(); + // Generate UUIDv7 and manually set trace reason marker. + Http::TestRequestHeaderMapImpl temp_headers; + uuid_utils.set(temp_headers, true, false); + std::string uuid_with_trace_reason = std::string(temp_headers.getRequestIdValue()); uuid_with_trace_reason[14] = 'b'; // 'b' means TRACE_CLIENT. Http::TestRequestHeaderMapImpl request_headers; @@ -319,6 +355,38 @@ TEST(UUIDRequestIDExtensionTest, SetTraceStatusPackingDisabled) { EXPECT_EQ(uuid_with_trace_reason, request_headers.getRequestIdValue()); } +TEST(UUIDRequestIDExtensionTest, GenerateUuidV7Format) { + testing::NiceMock random; + Event::SimulatedTimeSystem time_system; + UUIDRequestIDExtension uuid_utils( + envoy::extensions::request_id::uuid::v3::UuidRequestIdConfig(), random, time_system); + + Http::TestRequestHeaderMapImpl request_headers; + + EXPECT_CALL(random, random()).Times(2).WillRepeatedly(Return(0x123456789ABCDEFULL)); + uuid_utils.set(request_headers, true, false); + + std::string uuid = std::string(request_headers.getRequestIdValue()); + + // Verify UUID length. + EXPECT_EQ(36, uuid.length()); + + // Verify marker: request_id starts with 'f'. + EXPECT_EQ('f', uuid[0]); + + // Verify version: position 14 = '7' (UUIDv7). + EXPECT_EQ('7', uuid[14]); + + // Verify variant: position 19 = '8', '9', 'a', or 'b' (RFC 4122). + EXPECT_TRUE(uuid[19] == '8' || uuid[19] == '9' || uuid[19] == 'a' || uuid[19] == 'b'); + + // Verify UUID format: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx + EXPECT_EQ('-', uuid[8]); + EXPECT_EQ('-', uuid[13]); + EXPECT_EQ('-', uuid[18]); + EXPECT_EQ('-', uuid[23]); +} + } // namespace RequestId } // namespace Extensions } // namespace Envoy From 85bcd320fea9bbc78fa5dff148b7afe1477ea4f6 Mon Sep 17 00:00:00 2001 From: Gavin Jeong Date: Fri, 16 Jan 2026 13:51:15 +0900 Subject: [PATCH 09/12] Add per-shard Redis proxy metrics Add per-shard statistics for Redis proxy to track hot shard usage: - enable_per_shard_stats: Emits per-shard request counters - upstream_rq_total: Total requests to each shard - upstream_rq_success: Successful requests - upstream_rq_failure: Failed requests - upstream_rq_active: Active requests (gauge) - enable_per_shard_latency_stats: Emits latency histogram - upstream_rq_time: Request latency in microseconds All metrics are scoped under: cluster..shard..* Per-shard command-level stats are also recorded when enable_command_stats is enabled alongside the per-shard options. Note: These options may significantly increase metric cardinality in large clusters. Use with caution in production environments. --- .../network/redis_proxy/v3/redis_proxy.proto | 33 +++- .../extensions/clusters/redis/redis_cluster.h | 2 + .../filters/network/common/redis/client.h | 10 ++ .../network/common/redis/client_impl.cc | 4 +- .../network/common/redis/client_impl.h | 4 + .../network/redis_proxy/conn_pool_impl.cc | 157 +++++++++++++++++- .../network/redis_proxy/conn_pool_impl.h | 43 ++++- .../extensions/health_checkers/redis/redis.h | 2 + 8 files changed, 249 insertions(+), 6 deletions(-) diff --git a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index 40cc2858dfbc8..401a843c77193 100644 --- a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -33,7 +33,7 @@ message RedisProxy { "envoy.config.filter.network.redis_proxy.v2.RedisProxy"; // Redis connection pool settings. - // [#next-free-field: 11] + // [#next-free-field: 13] message ConnPoolSettings { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.network.redis_proxy.v2.RedisProxy.ConnPoolSettings"; @@ -134,6 +134,37 @@ message RedisProxy { // storm to busy redis server. This config is a protection to rate limit reconnection rate. // If not set, there will be no rate limiting on the reconnection. ConnectionRateLimit connection_rate_limit = 10; + + // Enable per-shard statistics for tracking hot shard usage. When enabled, the following + // statistics will be emitted per upstream host (shard): + // + // * ``upstream_rq_total``: Total requests to this shard + // * ``upstream_rq_success``: Successful requests to this shard + // * ``upstream_rq_failure``: Failed requests to this shard + // * ``upstream_rq_active``: Active requests to this shard (gauge) + // + // The statistics will be emitted under the scope: + // ``cluster..shard..*`` + // + // .. note:: + // Enabling this option may significantly increase metric cardinality in large clusters + // with many shards. Use with caution in production environments. + bool enable_per_shard_stats = 11; + + // Enable per-shard latency histogram for tracking request latency per upstream host (shard). + // When enabled, the following histogram will be emitted per shard: + // + // * ``upstream_rq_time``: Request latency histogram in microseconds + // + // The histogram will be emitted under the scope: + // ``cluster..shard..upstream_rq_time`` + // + // This option requires ``enable_per_shard_stats`` to be enabled. + // + // .. note:: + // Enabling this option may significantly increase metric cardinality in large clusters + // with many shards. Use with caution in production environments. + bool enable_per_shard_latency_stats = 12; } message PrefixRoutes { diff --git a/source/extensions/clusters/redis/redis_cluster.h b/source/extensions/clusters/redis/redis_cluster.h index 0a4b8fe8d29e9..23539995c39ca 100644 --- a/source/extensions/clusters/redis/redis_cluster.h +++ b/source/extensions/clusters/redis/redis_cluster.h @@ -241,6 +241,8 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { std::chrono::milliseconds bufferFlushTimeoutInMs() const override { return buffer_timeout_; } uint32_t maxUpstreamUnknownConnections() const override { return 0; } bool enableCommandStats() const override { return true; } + bool enablePerShardStats() const override { return false; } // Not needed for discovery + bool enablePerShardLatencyStats() const override { return false; } // Not needed for discovery bool connectionRateLimitEnabled() const override { return false; } uint32_t connectionRateLimitPerSec() const override { return 0; } // For any readPolicy other than Primary, the RedisClientFactory will send a READONLY command diff --git a/source/extensions/filters/network/common/redis/client.h b/source/extensions/filters/network/common/redis/client.h index 6879a12b12d4b..a1fc16347f73d 100644 --- a/source/extensions/filters/network/common/redis/client.h +++ b/source/extensions/filters/network/common/redis/client.h @@ -183,6 +183,16 @@ class Config { */ virtual bool enableCommandStats() const PURE; + /** + * @return when enabled, per-shard statistics will be recorded for tracking hot shard usage. + */ + virtual bool enablePerShardStats() const PURE; + + /** + * @return when enabled, per-shard latency histograms will be recorded. + */ + virtual bool enablePerShardLatencyStats() const PURE; + /** * @return the read policy the proxy should use. */ diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index 92a9f183c11e3..83940ca3453fc 100644 --- a/source/extensions/filters/network/common/redis/client_impl.cc +++ b/source/extensions/filters/network/common/redis/client_impl.cc @@ -41,7 +41,9 @@ ConfigImpl::ConfigImpl( // as the buffer is flushed on each request immediately. max_upstream_unknown_connections_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_upstream_unknown_connections, 100)), - enable_command_stats_(config.enable_command_stats()) { + enable_command_stats_(config.enable_command_stats()), + enable_per_shard_stats_(config.enable_per_shard_stats()), + enable_per_shard_latency_stats_(config.enable_per_shard_latency_stats()) { switch (config.read_policy()) { PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; case envoy::extensions::filters::network::redis_proxy::v3::RedisProxy::ConnPoolSettings::MASTER: diff --git a/source/extensions/filters/network/common/redis/client_impl.h b/source/extensions/filters/network/common/redis/client_impl.h index a535084033948..01970b5ebee7c 100644 --- a/source/extensions/filters/network/common/redis/client_impl.h +++ b/source/extensions/filters/network/common/redis/client_impl.h @@ -54,6 +54,8 @@ class ConfigImpl : public Config { return max_upstream_unknown_connections_; } bool enableCommandStats() const override { return enable_command_stats_; } + bool enablePerShardStats() const override { return enable_per_shard_stats_; } + bool enablePerShardLatencyStats() const override { return enable_per_shard_latency_stats_; } ReadPolicy readPolicy() const override { return read_policy_; } bool connectionRateLimitEnabled() const override { return connection_rate_limit_enabled_; } uint32_t connectionRateLimitPerSec() const override { return connection_rate_limit_per_sec_; } @@ -66,6 +68,8 @@ class ConfigImpl : public Config { const std::chrono::milliseconds buffer_flush_timeout_; const uint32_t max_upstream_unknown_connections_; const bool enable_command_stats_; + const bool enable_per_shard_stats_; + const bool enable_per_shard_latency_stats_; ReadPolicy read_policy_; bool connection_rate_limit_enabled_; uint32_t connection_rate_limit_per_sec_; diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc index 76eabcc5cf103..979887ea11ebc 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc @@ -269,6 +269,52 @@ void InstanceImpl::ThreadLocalPool::drainClients() { } } +RedisShardStatsSharedPtr +InstanceImpl::ThreadLocalPool::getOrCreateShardStats(const std::string& host_address) { + auto it = shard_stats_map_.find(host_address); + if (it != shard_stats_map_.end()) { + return it->second.stats_; + } + + // Create a sanitized stat name from the host address (replace ':' and '.' with '_') + std::string stat_name = host_address; + std::replace(stat_name.begin(), stat_name.end(), ':', '_'); + std::replace(stat_name.begin(), stat_name.end(), '.', '_'); + + Stats::ScopeSharedPtr shard_scope = stats_scope_->createScope(fmt::format("shard.{}.", stat_name)); + auto shard_stats = std::make_shared(RedisShardStats{ + REDIS_SHARD_STATS(POOL_COUNTER(*shard_scope), POOL_GAUGE(*shard_scope))}); + // Store both scope and stats to keep the scope alive + shard_stats_map_[host_address] = ShardStatsEntry{shard_scope, shard_stats}; + return shard_stats; +} + +Stats::ScopeSharedPtr +InstanceImpl::ThreadLocalPool::getShardScope(const std::string& host_address) { + auto it = shard_stats_map_.find(host_address); + if (it != shard_stats_map_.end()) { + return it->second.scope_; + } + return nullptr; +} + +Stats::Histogram* +InstanceImpl::ThreadLocalPool::getOrCreateShardLatencyHistogram(const std::string& host_address) { + auto it = shard_stats_map_.find(host_address); + if (it == shard_stats_map_.end()) { + // Create shard stats entry first if it doesn't exist + getOrCreateShardStats(host_address); + it = shard_stats_map_.find(host_address); + } + + if (it->second.latency_histogram_ == nullptr) { + // Create the histogram if it doesn't exist + it->second.latency_histogram_ = &it->second.scope_->histogramFromString( + "upstream_rq_time", Stats::Histogram::Unit::Microseconds); + } + return it->second.latency_histogram_; +} + InstanceImpl::ThreadLocalActiveClientPtr& InstanceImpl::ThreadLocalPool::threadLocalActiveClient(Upstream::HostConstSharedPtr host) { TokenBucketPtr& rate_limiter = cx_rate_limiter_map_[host]; @@ -455,7 +501,22 @@ InstanceImpl::ThreadLocalPool::makeRequestToHost(Upstream::HostConstSharedPtr& h } } - pending_requests_.emplace_back(*this, std::move(request), callbacks, host); + // Get or create per-shard stats for tracking hot shard usage (if enabled) + RedisShardStatsSharedPtr shard_stats = nullptr; + Stats::ScopeSharedPtr shard_scope = nullptr; + Stats::Histogram* latency_histogram = nullptr; + if (config_->enablePerShardStats()) { + const std::string host_address = host->address()->asString(); + shard_stats = getOrCreateShardStats(host_address); + shard_scope = getShardScope(host_address); + } + if (config_->enablePerShardLatencyStats()) { + const std::string host_address = host->address()->asString(); + latency_histogram = getOrCreateShardLatencyHistogram(host_address); + } + + pending_requests_.emplace_back(*this, std::move(request), callbacks, host, shard_stats, + shard_scope, latency_histogram); PendingRequest& pending_request = pending_requests_.back(); if (!transaction.active_) { @@ -518,9 +579,30 @@ void InstanceImpl::ThreadLocalActiveClient::onEvent(Network::ConnectionEvent eve InstanceImpl::PendingRequest::PendingRequest(InstanceImpl::ThreadLocalPool& parent, RespVariant&& incoming_request, PoolCallbacks& pool_callbacks, - Upstream::HostConstSharedPtr& host) + Upstream::HostConstSharedPtr& host, + RedisShardStatsSharedPtr shard_stats, + Stats::ScopeSharedPtr shard_scope, + Stats::Histogram* latency_histogram) : parent_(parent), incoming_request_(std::move(incoming_request)), - pool_callbacks_(pool_callbacks), host_(host) {} + pool_callbacks_(pool_callbacks), host_(host), shard_stats_(std::move(shard_stats)), + shard_scope_(std::move(shard_scope)), latency_histogram_(latency_histogram), + start_time_(parent.dispatcher_.timeSource().monotonicTime()) { + // Track per-shard request metrics and command stats + if (shard_stats_) { + shard_stats_->upstream_rq_total_.inc(); + shard_stats_->upstream_rq_active_.inc(); + } + // Extract and track command name for per-shard command stats + if (shard_scope_ && parent_.config_->enableCommandStats()) { + command_ = parent_.redis_command_stats_->getCommandFromRequest(getRequest(incoming_request_)); + parent_.redis_command_stats_->updateStatsTotal(*shard_scope_, command_); + // Create per-shard per-command latency timer when both command stats and per-shard latency are enabled + if (parent_.config_->enablePerShardLatencyStats()) { + command_latency_timer_ = parent_.redis_command_stats_->createCommandTimer( + *shard_scope_, command_, parent_.dispatcher_.timeSource()); + } + } +} InstanceImpl::PendingRequest::~PendingRequest() { cache_load_handle_.reset(); @@ -528,6 +610,15 @@ InstanceImpl::PendingRequest::~PendingRequest() { if (request_handler_) { request_handler_->cancel(); request_handler_ = nullptr; + // Update per-shard stats - treat cancellation as failure + if (shard_stats_) { + shard_stats_->upstream_rq_active_.dec(); + shard_stats_->upstream_rq_failure_.inc(); + } + // Update per-shard command stats (failure due to cancellation) + if (shard_scope_ && parent_.config_->enableCommandStats()) { + parent_.redis_command_stats_->updateStats(*shard_scope_, command_, false); + } // If we have to cancel the request on the client, then we'll treat this as failure for pool // callback pool_callbacks_.onFailure(); @@ -536,12 +627,52 @@ InstanceImpl::PendingRequest::~PendingRequest() { void InstanceImpl::PendingRequest::onResponse(Common::Redis::RespValuePtr&& response) { request_handler_ = nullptr; + // Update per-shard stats + if (shard_stats_) { + shard_stats_->upstream_rq_active_.dec(); + shard_stats_->upstream_rq_success_.inc(); + } + // Update per-shard command stats (success) + if (shard_scope_ && parent_.config_->enableCommandStats()) { + parent_.redis_command_stats_->updateStats(*shard_scope_, command_, true); + } + // Record per-shard latency histogram + if (latency_histogram_ != nullptr) { + const auto end_time = parent_.dispatcher_.timeSource().monotonicTime(); + const auto latency_us = std::chrono::duration_cast( + end_time - start_time_).count(); + latency_histogram_->recordValue(latency_us); + } + // Complete per-shard per-command latency timer + if (command_latency_timer_) { + command_latency_timer_->complete(); + } pool_callbacks_.onResponse(std::move(response)); parent_.onRequestCompleted(); } void InstanceImpl::PendingRequest::onFailure() { request_handler_ = nullptr; + // Update per-shard stats + if (shard_stats_) { + shard_stats_->upstream_rq_active_.dec(); + shard_stats_->upstream_rq_failure_.inc(); + } + // Update per-shard command stats (failure) + if (shard_scope_ && parent_.config_->enableCommandStats()) { + parent_.redis_command_stats_->updateStats(*shard_scope_, command_, false); + } + // Record per-shard latency histogram (even for failures) + if (latency_histogram_ != nullptr) { + const auto end_time = parent_.dispatcher_.timeSource().monotonicTime(); + const auto latency_us = std::chrono::duration_cast( + end_time - start_time_).count(); + latency_histogram_->recordValue(latency_us); + } + // Complete per-shard per-command latency timer + if (command_latency_timer_) { + command_latency_timer_->complete(); + } pool_callbacks_.onFailure(); parent_.refresh_manager_->onFailure(parent_.cluster_name_); parent_.onRequestCompleted(); @@ -640,6 +771,26 @@ void InstanceImpl::PendingRequest::doRedirection(Common::Redis::RespValuePtr&& v void InstanceImpl::PendingRequest::cancel() { request_handler_->cancel(); request_handler_ = nullptr; + // Update per-shard stats - treat cancellation as failure + if (shard_stats_) { + shard_stats_->upstream_rq_active_.dec(); + shard_stats_->upstream_rq_failure_.inc(); + } + // Update per-shard command stats (failure due to cancellation) + if (shard_scope_ && parent_.config_->enableCommandStats()) { + parent_.redis_command_stats_->updateStats(*shard_scope_, command_, false); + } + // Record per-shard latency histogram (even for cancellations) + if (latency_histogram_ != nullptr) { + const auto end_time = parent_.dispatcher_.timeSource().monotonicTime(); + const auto latency_us = std::chrono::duration_cast( + end_time - start_time_).count(); + latency_histogram_->recordValue(latency_us); + } + // Complete per-shard per-command latency timer + if (command_latency_timer_) { + command_latency_timer_->complete(); + } parent_.onRequestCompleted(); } diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h index b8f520777ae34..0c720dc1865cf 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h @@ -7,8 +7,10 @@ #include #include +#include "envoy/common/time.h" #include "envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.pb.h" #include "envoy/stats/stats_macros.h" +#include "envoy/stats/timespan.h" #include "envoy/thread_local/thread_local.h" #include "envoy/upstream/cluster_manager.h" @@ -49,6 +51,32 @@ struct RedisClusterStats { REDIS_CLUSTER_STATS(GENERATE_COUNTER_STRUCT) }; +/** + * Per-shard statistics for tracking hot shard usage. + * These metrics help identify which shards are receiving more traffic. + */ +#define REDIS_SHARD_STATS(COUNTER, GAUGE) \ + COUNTER(upstream_rq_total) \ + COUNTER(upstream_rq_success) \ + COUNTER(upstream_rq_failure) \ + GAUGE(upstream_rq_active, Accumulate) + +struct RedisShardStats { + REDIS_SHARD_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) +}; + +using RedisShardStatsSharedPtr = std::shared_ptr; + +/** + * Struct to hold per-shard stats and the scope they belong to. + * The scope must be kept alive for the stats to remain valid. + */ +struct ShardStatsEntry { + Stats::ScopeSharedPtr scope_; + RedisShardStatsSharedPtr stats_; + Stats::Histogram* latency_histogram_{nullptr}; // Per-shard latency histogram (optional) +}; + class DoNothingPoolCallbacks : public PoolCallbacks { public: void onResponse(Common::Redis::RespValuePtr&&) override {}; @@ -119,7 +147,9 @@ class InstanceImpl : public Instance, public std::enable_shared_from_this { PendingRequest(ThreadLocalPool& parent, RespVariant&& incoming_request, - PoolCallbacks& pool_callbacks, Upstream::HostConstSharedPtr& host); + PoolCallbacks& pool_callbacks, Upstream::HostConstSharedPtr& host, + RedisShardStatsSharedPtr shard_stats, Stats::ScopeSharedPtr shard_scope, + Stats::Histogram* latency_histogram); ~PendingRequest() override; // Common::Redis::Client::ClientCallbacks @@ -148,6 +178,12 @@ class InstanceImpl : public Instance, public std::enable_shared_from_this& hosts_added); void onHostsRemoved(const std::vector& hosts_removed); void drainClients(); + RedisShardStatsSharedPtr getOrCreateShardStats(const std::string& host_address); + Stats::ScopeSharedPtr getShardScope(const std::string& host_address); + Stats::Histogram* getOrCreateShardLatencyHistogram(const std::string& host_address); // Upstream::ClusterUpdateCallbacks void onClusterAddOrUpdate(absl::string_view cluster_name, @@ -222,6 +261,8 @@ class InstanceImpl : public Instance, public std::enable_shared_from_this aws_iam_authenticator_; absl::optional aws_iam_config_; + // Per-shard stats map keyed by host address (e.g., "10.0.0.1:6379") + absl::node_hash_map shard_stats_map_; }; const std::string cluster_name_; diff --git a/source/extensions/health_checkers/redis/redis.h b/source/extensions/health_checkers/redis/redis.h index 8668904b26447..3be9d26cff47f 100644 --- a/source/extensions/health_checkers/redis/redis.h +++ b/source/extensions/health_checkers/redis/redis.h @@ -95,6 +95,8 @@ class RedisHealthChecker : public Upstream::HealthCheckerImplBase { uint32_t maxUpstreamUnknownConnections() const override { return 0; } bool enableCommandStats() const override { return false; } + bool enablePerShardStats() const override { return false; } // Not needed for health checks + bool enablePerShardLatencyStats() const override { return false; } // Not needed for health checks bool connectionRateLimitEnabled() const override { return false; } uint32_t connectionRateLimitPerSec() const override { return 0; } From a71d3fb4c436930cfa4757af8c8c30378dd4ba40 Mon Sep 17 00:00:00 2001 From: Doogie Min Date: Tue, 23 Dec 2025 16:24:42 +0900 Subject: [PATCH 10/12] redis: Add zone-aware routing support for Redis Cluster proxy This change implements zone-aware routing for Redis Cluster, allowing read requests to be routed to replicas in the same availability zone as the client. Key changes: - Add enable_zone_discovery config option to redis_cluster.proto - Add az_affinity and az_affinity_replicas_and_primary read policies - Implement INFO command-based zone discovery during cluster slot updates - Store zone info in host locality for standard Envoy locality handling - RedisShard groups replicas by zone for efficient zone-aware routing Zone Discovery Flow: 1. CLUSTER SLOTS response triggers zone discovery when enabled 2. INFO command sent to each unique node to get availability_zone 3. Zones stored in host->locality().zone() when hosts are created 4. RedisShard reads zone from host locality, groups replicas by zone Read Policies: - AzAffinity: local replicas -> any replica -> primary - AzAffinityReplicasAndPrimary: local replicas -> local primary -> any replica -> primary Note: This feature currently works with Valkey only. Valkey exposes availability_zone in its INFO response. Standard Redis does not support this field. Signed-off-by: Doogie Min --- .../clusters/redis/v3/redis_cluster.proto | 12 +- .../network/redis_proxy/v3/redis_proxy.proto | 17 ++ changelogs/current.yaml | 8 + .../clusters/redis/redis_cluster.cc | 242 +++++++++++++++- .../extensions/clusters/redis/redis_cluster.h | 84 +++++- .../clusters/redis/redis_cluster_lb.cc | 54 +++- .../clusters/redis/redis_cluster_lb.h | 59 +++- .../filters/network/common/redis/client.h | 12 +- .../network/common/redis/client_impl.cc | 8 + .../filters/network/redis_proxy/config.cc | 5 +- .../network/redis_proxy/conn_pool_impl.cc | 17 +- .../network/redis_proxy/conn_pool_impl.h | 7 +- .../clusters/redis/redis_cluster_lb_test.cc | 262 +++++++++++++++++- 13 files changed, 749 insertions(+), 38 deletions(-) diff --git a/api/envoy/extensions/clusters/redis/v3/redis_cluster.proto b/api/envoy/extensions/clusters/redis/v3/redis_cluster.proto index ba8f434d6c74a..91afac08f34fc 100644 --- a/api/envoy/extensions/clusters/redis/v3/redis_cluster.proto +++ b/api/envoy/extensions/clusters/redis/v3/redis_cluster.proto @@ -54,7 +54,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // redirect_refresh_threshold: 10 // [#extension: envoy.clusters.redis] -// [#next-free-field: 7] +// [#next-free-field: 8] message RedisClusterConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.config.cluster.redis.RedisClusterConfig"; @@ -83,4 +83,14 @@ message RedisClusterConfig { // If not set, this defaults to 0, which disables the topology refresh due to degraded or // unhealthy host. uint32 host_degraded_refresh_threshold = 6; + + // Enable zone discovery via INFO command. When enabled, the cluster will + // send INFO command to each node to discover its availability_zone field, + // which is then used for zone-aware routing. + // + // Note: This feature currently works with Valkey only. Valkey exposes + // availability_zone in its INFO response. Standard Redis does not support this field. + // + // If not set, this defaults to false. + google.protobuf.BoolValue enable_zone_discovery = 7; } diff --git a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index 401a843c77193..0f9e43397e6f9 100644 --- a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -60,6 +60,23 @@ message RedisProxy { // Read from any node of the cluster. A random node is selected among the primary and // replicas, healthy nodes have precedent over unhealthy nodes. ANY = 4; + + // Read from replicas in the same availability zone as the Envoy proxy. If no replicas + // are available in the same zone, fall back to any replica. If no replicas are available + // at all, fall back to the primary. + // + // Note: Zone discovery currently works with Valkey only. Valkey exposes availability_zone + // in its INFO response. Standard Redis does not support this field. + // + // The client zone is determined from Envoy's node.locality.zone. + AZ_AFFINITY = 5; + + // Similar to AZ_AFFINITY, but also considers the primary node for same-zone routing. + // Priority order: replicas in same zone -> primary in same zone -> any replica -> primary. + // This is useful when reducing cross-zone traffic is more important than read distribution. + // + // Note: Zone discovery currently works with Valkey only. + AZ_AFFINITY_REPLICAS_AND_PRIMARY = 6; } // Per-operation timeout in milliseconds. The timer starts when the first diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9ecf0d6e48ce5..dd6dc555952d9 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -13,5 +13,13 @@ removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` new_features: +- area: redis + change: | + Added zone-aware routing support for Redis Cluster proxy. New read policies + :ref:`AZ_AFFINITY ` + and :ref:`AZ_AFFINITY_REPLICAS_AND_PRIMARY ` + route read requests to replicas in the same availability zone. Zone discovery is enabled via + :ref:`enable_zone_discovery `. + Note: This feature currently works with Valkey only. deprecated: diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index e29e23dd5f704..34a22f1b94465 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -24,6 +24,28 @@ absl::StatusOr> RedisCluster::RedisHost return ret; } +absl::StatusOr> +RedisCluster::RedisHost::create(Upstream::ClusterInfoConstSharedPtr cluster, + const std::string& hostname, + Network::Address::InstanceConstSharedPtr address, + RedisCluster& parent, bool primary, const std::string& zone) { + absl::Status creation_status = absl::OkStatus(); + auto ret = std::unique_ptr(new RedisCluster::RedisHost( + cluster, hostname, address, parent, primary, zone, creation_status)); + RETURN_IF_NOT_OK(creation_status); + return ret; +} + +std::shared_ptr +RedisCluster::RedisHost::makeLocalityWithZone( + const envoy::config::core::v3::Locality& base_locality, const std::string& zone) { + auto locality = std::make_shared(base_locality); + if (!zone.empty()) { + locality->set_zone(zone); + } + return locality; +} + absl::StatusOr> RedisCluster::create( const envoy::config::cluster::v3::Cluster& cluster, const envoy::extensions::clusters::redis::v3::RedisClusterConfig& redis_cluster, @@ -75,7 +97,9 @@ RedisCluster::RedisCluster( context.serverFactoryContext().mainThreadDispatcher(), context.serverFactoryContext().clusterManager(), context.serverFactoryContext().api().timeSource())), - registration_handle_(nullptr) { + registration_handle_(nullptr), + enable_zone_discovery_( + PROTOBUF_GET_WRAPPED_OR_DEFAULT(redis_cluster, enable_zone_discovery, false)) { const auto& locality_lb_endpoints = load_assignment_.endpoints(); for (const auto& locality_lb_endpoint : locality_lb_endpoints) { for (const auto& lb_endpoint : locality_lb_endpoint.lb_endpoints()) { @@ -162,21 +186,32 @@ void RedisCluster::updateAllHosts(const Upstream::HostVector& hosts_added, hosts_added, hosts_removed, absl::nullopt, absl::nullopt, absl::nullopt); } -void RedisCluster::onClusterSlotUpdate(ClusterSlotsSharedPtr&& slots) { +void RedisCluster::onClusterSlotUpdate(ClusterSlotsSharedPtr&& slots, + const HostZoneMap& host_zone_map) { Upstream::HostVector new_hosts; absl::flat_hash_set all_new_hosts; + // Helper to look up zone from map + auto get_zone = [&host_zone_map](const std::string& address) -> std::string { + auto it = host_zone_map.find(address); + return (it != host_zone_map.end()) ? it->second : ""; + }; + for (const ClusterSlot& slot : *slots) { - if (all_new_hosts.count(slot.primary()->asString()) == 0) { + const std::string primary_address = slot.primary()->asString(); + if (all_new_hosts.count(primary_address) == 0) { + // Pass zone from map to set host's locality.zone new_hosts.emplace_back(THROW_OR_RETURN_VALUE( - RedisHost::create(info(), "", slot.primary(), *this, true), std::unique_ptr)); - all_new_hosts.emplace(slot.primary()->asString()); + RedisHost::create(info(), "", slot.primary(), *this, true, get_zone(primary_address)), + std::unique_ptr)); + all_new_hosts.emplace(primary_address); } for (auto const& replica : slot.replicas()) { if (all_new_hosts.count(replica.first) == 0) { - new_hosts.emplace_back( - THROW_OR_RETURN_VALUE(RedisHost::create(info(), "", replica.second, *this, false), - std::unique_ptr)); + // Pass zone from map to set host's locality.zone + new_hosts.emplace_back(THROW_OR_RETURN_VALUE( + RedisHost::create(info(), "", replica.second, *this, false, get_zone(replica.first)), + std::unique_ptr)); all_new_hosts.emplace(replica.first); } } @@ -576,9 +611,13 @@ void RedisCluster::RedisDiscoverySession::finishClusterHostnameResolution( if (!isParentAlive()) { return; } - parent_.onClusterSlotUpdate(std::move(slots)); - if (resolve_timer_) { - resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + if (parent_.enable_zone_discovery_) { + startZoneDiscovery(std::move(slots)); + } else { + parent_.onClusterSlotUpdate(std::move(slots)); + if (resolve_timer_) { + resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + } } } @@ -685,9 +724,13 @@ void RedisCluster::RedisDiscoverySession::onResponse( resolveClusterHostnames(std::move(cluster_slots), hostname_resolution_required_cnt); } else { // All slots addresses were represented by IP/Port pairs. - parent_.onClusterSlotUpdate(std::move(cluster_slots)); - if (resolve_timer_) { - resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + if (parent_.enable_zone_discovery_) { + startZoneDiscovery(std::move(cluster_slots)); + } else { + parent_.onClusterSlotUpdate(std::move(cluster_slots)); + if (resolve_timer_) { + resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + } } } } @@ -750,7 +793,178 @@ void RedisCluster::RedisDiscoverySession::onFailure() { } } +// ZoneDiscoveryCallback implementation +void RedisCluster::ZoneDiscoveryCallback::onResponse( + NetworkFilters::Common::Redis::RespValuePtr&& value) { + parent_.onZoneResponse(address_, is_primary_, std::move(value)); +} + +void RedisCluster::ZoneDiscoveryCallback::onFailure() { + parent_.onZoneDiscoveryFailure(address_, is_primary_); +} + +// Zone discovery methods +void RedisCluster::RedisDiscoverySession::startZoneDiscovery(ClusterSlotsSharedPtr slots) { + ENVOY_LOG(debug, "starting zone discovery for '{}' with {} slots", parent_.info_->name(), + slots->size()); + + // Collect unique node addresses + absl::flat_hash_set unique_addresses; + absl::flat_hash_map address_is_primary; // track if address is primary + + for (const ClusterSlot& slot : *slots) { + if (slot.primary()) { + const std::string& addr = slot.primary()->asString(); + if (unique_addresses.find(addr) == unique_addresses.end()) { + unique_addresses.insert(addr); + address_is_primary[addr] = true; + } + } + for (const auto& replica : slot.replicas()) { + if (unique_addresses.find(replica.first) == unique_addresses.end()) { + unique_addresses.insert(replica.first); + address_is_primary[replica.first] = false; + } + } + } + + if (unique_addresses.empty()) { + ENVOY_LOG(debug, "no nodes to discover zones for"); + parent_.onClusterSlotUpdate(std::move(slots)); + resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); + return; + } + + pending_zone_discovery_slots_ = std::move(slots); + pending_zone_requests_.store(unique_addresses.size()); + + // Send INFO command to each unique node + for (const auto& addr : unique_addresses) { + bool is_primary = address_is_primary[addr]; + + // Find existing client or create new one + RedisDiscoveryClientPtr& client = client_map_[addr]; + if (!client) { + // Need to create a new host and client for this address + auto address = Network::Utility::parseInternetAddressAndPortNoThrow(addr, false); + if (!address) { + ENVOY_LOG(warn, "failed to parse address {} for zone discovery", addr); + onZoneDiscoveryFailure(addr, is_primary); + continue; + } + + auto host_or_error = RedisHost::create(parent_.info(), "", address, parent_, is_primary); + if (!host_or_error.ok()) { + ENVOY_LOG(warn, "failed to create host for zone discovery: {}", addr); + onZoneDiscoveryFailure(addr, is_primary); + continue; + } + + // Convert unique_ptr to shared_ptr for the client factory + Upstream::HostSharedPtr host = std::move(*host_or_error); + + client = std::make_unique(*this); + client->host_ = addr; + client->client_ = client_factory_.create( + host, dispatcher_, shared_from_this(), redis_command_stats_, parent_.info()->statsScope(), + parent_.auth_username_, parent_.auth_password_, false, absl::nullopt, absl::nullopt); + client->client_->addConnectionCallbacks(*client); + } + + // Create callback for this zone request + auto callback = std::make_unique(*this, addr, is_primary); + zone_callbacks_[addr] = std::move(callback); + + ENVOY_LOG(debug, "sending INFO command to {} for zone discovery", addr); + auto* request = client->client_->makeRequest(InfoRequest::instance_, *zone_callbacks_[addr]); + if (request) { + zone_requests_[addr] = request; + } else { + ENVOY_LOG(warn, "failed to send INFO command to {}", addr); + onZoneDiscoveryFailure(addr, is_primary); + } + } +} + +void RedisCluster::RedisDiscoverySession::onZoneResponse( + const std::string& address, bool /*is_primary*/, + NetworkFilters::Common::Redis::RespValuePtr&& value) { + ENVOY_LOG(debug, "received zone discovery response from {}", address); + + // Remove from tracking + zone_requests_.erase(address); + zone_callbacks_.erase(address); + + if (value->type() == NetworkFilters::Common::Redis::RespType::BulkString) { + std::string zone = parseAvailabilityZone(value->asString()); + if (!zone.empty()) { + discovered_zones_[address] = zone; + ENVOY_LOG(debug, "discovered zone '{}' for node {}", zone, address); + } + } else { + ENVOY_LOG(warn, "unexpected INFO response type from {}: {}", address, + static_cast(value->type())); + } + + uint32_t remaining = pending_zone_requests_.fetch_sub(1) - 1; + if (remaining == 0) { + finishZoneDiscovery(); + } +} + +void RedisCluster::RedisDiscoverySession::onZoneDiscoveryFailure(const std::string& address, + bool /*is_primary*/) { + ENVOY_LOG(warn, "zone discovery failed for node {}", address); + + // Remove from tracking + zone_requests_.erase(address); + zone_callbacks_.erase(address); + + // Continue even on failure - just won't have zone info for this node + uint32_t remaining = pending_zone_requests_.fetch_sub(1) - 1; + if (remaining == 0) { + finishZoneDiscovery(); + } +} + +void RedisCluster::RedisDiscoverySession::finishZoneDiscovery() { + ENVOY_LOG(debug, "zone discovery complete for '{}'", parent_.info_->name()); + + if (pending_zone_discovery_slots_) { + parent_.onClusterSlotUpdate(std::move(pending_zone_discovery_slots_), discovered_zones_); + pending_zone_discovery_slots_ = nullptr; + discovered_zones_.clear(); // Clear after use + } + resolve_timer_->enableTimer(parent_.cluster_refresh_rate_); +} + +// Parse availability_zone from Valkey INFO response. +// Valkey INFO response format: +// # Server +// valkey_version:8.0.0 +// ... +// availability_zone:us-east-1a +// ... +// Note: Standard Redis does not expose availability_zone. +std::string +RedisCluster::RedisDiscoverySession::parseAvailabilityZone(const std::string& info_response) { + const std::string key = "availability_zone:"; + size_t pos = info_response.find(key); + if (pos == std::string::npos) { + return ""; + } + + pos += key.size(); + size_t end = info_response.find_first_of("\r\n", pos); + if (end == std::string::npos) { + end = info_response.size(); + } + + return info_response.substr(pos, end - pos); +} + RedisCluster::ClusterSlotsRequest RedisCluster::ClusterSlotsRequest::instance_; +RedisCluster::InfoRequest RedisCluster::InfoRequest::instance_; absl::StatusOr> RedisClusterFactory::createClusterWithConfig( diff --git a/source/extensions/clusters/redis/redis_cluster.h b/source/extensions/clusters/redis/redis_cluster.h index 23539995c39ca..3e636a366773c 100644 --- a/source/extensions/clusters/redis/redis_cluster.h +++ b/source/extensions/clusters/redis/redis_cluster.h @@ -113,6 +113,20 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { static ClusterSlotsRequest instance_; }; + // INFO command request for zone discovery + struct InfoRequest : public Extensions::NetworkFilters::Common::Redis::RespValue { + public: + InfoRequest() { + type(Extensions::NetworkFilters::Common::Redis::RespType::Array); + std::vector values(1); + values[0].type(NetworkFilters::Common::Redis::RespType::BulkString); + values[0].asString() = "INFO"; + asArray().swap(values); + } + + static InfoRequest instance_; + }; + InitializePhase initializePhase() const override { return InitializePhase::Primary; } /// TimeSource& timeSource() const { return time_source_; } @@ -134,7 +148,7 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { void updateAllHosts(const Upstream::HostVector& hosts_added, const Upstream::HostVector& hosts_removed, uint32_t priority); - void onClusterSlotUpdate(ClusterSlotsSharedPtr&&); + void onClusterSlotUpdate(ClusterSlotsSharedPtr&&, const HostZoneMap& host_zone_map = {}); void reloadHealthyHostsHelper(const Upstream::HostSharedPtr& host) override; @@ -151,11 +165,19 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { // A redis node in the Redis cluster. class RedisHost : public Upstream::HostImpl { public: + // Factory method without zone (for backward compatibility) static absl::StatusOr> create(Upstream::ClusterInfoConstSharedPtr cluster, const std::string& hostname, Network::Address::InstanceConstSharedPtr address, RedisCluster& parent, bool primary); + // Factory method with zone parameter - sets locality.zone on the host + static absl::StatusOr> + create(Upstream::ClusterInfoConstSharedPtr cluster, const std::string& hostname, + Network::Address::InstanceConstSharedPtr address, RedisCluster& parent, bool primary, + const std::string& zone); + protected: + // Constructor without zone RedisHost(Upstream::ClusterInfoConstSharedPtr cluster, const std::string& hostname, Network::Address::InstanceConstSharedPtr address, RedisCluster& parent, bool primary, absl::Status& creation_status) @@ -171,9 +193,29 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { parent.localityLbEndpoint().priority(), parent.lbEndpoint().health_status()), primary_(primary) {} + // Constructor with zone - creates locality with zone set + RedisHost(Upstream::ClusterInfoConstSharedPtr cluster, const std::string& hostname, + Network::Address::InstanceConstSharedPtr address, RedisCluster& parent, bool primary, + const std::string& zone, absl::Status& creation_status) + : Upstream::HostImpl( + creation_status, cluster, hostname, address, + std::make_shared(parent.lbEndpoint().metadata()), + std::make_shared( + parent.localityLbEndpoint().metadata()), + parent.lbEndpoint().load_balancing_weight().value(), + makeLocalityWithZone(parent.localityLbEndpoint().locality(), zone), + parent.lbEndpoint().endpoint().health_check_config(), + parent.localityLbEndpoint().priority(), parent.lbEndpoint().health_status()), + primary_(primary) {} + bool isPrimary() const { return primary_; } private: + // Helper to create Locality proto with zone set + static std::shared_ptr + makeLocalityWithZone(const envoy::config::core::v3::Locality& base_locality, + const std::string& zone); + const bool primary_; }; @@ -212,6 +254,26 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { using RedisDiscoveryClientPtr = std::unique_ptr; + // Callback handler for zone discovery INFO requests + struct ZoneDiscoveryCallback + : public Extensions::NetworkFilters::Common::Redis::Client::ClientCallbacks { + ZoneDiscoveryCallback(RedisDiscoverySession& parent, const std::string& address, + bool is_primary) + : parent_(parent), address_(address), is_primary_(is_primary) {} + + // Extensions::NetworkFilters::Common::Redis::Client::ClientCallbacks + void onResponse(NetworkFilters::Common::Redis::RespValuePtr&& value) override; + void onFailure() override; + void onRedirection(NetworkFilters::Common::Redis::RespValuePtr&&, const std::string&, + bool) override {} + + RedisDiscoverySession& parent_; + const std::string address_; + const bool is_primary_; + }; + + using ZoneDiscoveryCallbackPtr = std::unique_ptr; + struct RedisDiscoverySession : public Extensions::NetworkFilters::Common::Redis::Client::Config, public Extensions::NetworkFilters::Common::Redis::Client::ClientCallbacks, @@ -229,6 +291,14 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { // Start discovery against a random host from existing hosts void startResolveRedis(); + // Zone discovery methods + void startZoneDiscovery(ClusterSlotsSharedPtr slots); + void onZoneResponse(const std::string& address, bool is_primary, + NetworkFilters::Common::Redis::RespValuePtr&& value); + void onZoneDiscoveryFailure(const std::string& address, bool is_primary); + void finishZoneDiscovery(); + static std::string parseAvailabilityZone(const std::string& info_response); + // Extensions::NetworkFilters::Common::Redis::Client::Config bool disableOutlierEvents() const override { return true; } std::chrono::milliseconds opTimeout() const override { @@ -305,11 +375,20 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { NetworkFilters::Common::Redis::Client::ClientFactory& client_factory_; const std::chrono::milliseconds buffer_timeout_; NetworkFilters::Common::Redis::RedisCommandStatsSharedPtr redis_command_stats_; - + // Flag set by parent's destructor to signal that parent is being destroyed. // Callbacks check this flag (owned by session) instead of accessing parent's flag // to avoid use-after-free when parent is destroyed but callbacks are still queued. std::atomic parent_destroyed_{false}; + + // Zone discovery state + ClusterSlotsSharedPtr pending_zone_discovery_slots_; + std::atomic pending_zone_requests_{0}; + absl::node_hash_map zone_callbacks_; + absl::node_hash_map + zone_requests_; + HostZoneMap discovered_zones_; // address -> zone mapping from INFO responses }; Upstream::ClusterManager& cluster_manager_; @@ -336,6 +415,7 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { const std::string cluster_name_; const Common::Redis::ClusterRefreshManagerSharedPtr refresh_manager_; Common::Redis::ClusterRefreshManager::HandlePtr registration_handle_; + const bool enable_zone_discovery_; // Flag to prevent callbacks during destruction std::atomic is_destroying_{false}; diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index cd63e5d05f8d7..c7ceb146098c7 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -124,6 +124,7 @@ Upstream::HostConstSharedPtr chooseRandomHost(const Upstream::HostSetImpl& host_ return nullptr; } } + } // namespace Upstream::HostSelectionResponse @@ -174,6 +175,50 @@ RedisClusterLoadBalancerFactory::RedisClusterLoadBalancer::chooseHost( } case NetworkFilters::Common::Redis::Client::ReadPolicy::Any: return chooseRandomHost(shard->allHosts(), random_); + case NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinity: { + // Spread read requests between replicas in the same AZ in round robin. + // Falls back to other replicas or primary if needed. + const std::string& client_zone = redis_context->clientZone(); + if (!client_zone.empty()) { + const auto* local_replicas = shard->replicasInZone(client_zone); + if (local_replicas) { + auto host = chooseRandomHost(*local_replicas, random_); + if (host) { + return host; + } + } + } + // Fall back to any replica, then primary + if (!shard->replicas().hosts().empty()) { + return chooseRandomHost(shard->replicas(), random_); + } + return shard->primary(); + } + case NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinityReplicasAndPrimary: { + // Spread read requests among nodes within the client's AZ in round robin, + // prioritizing: local replicas → local primary → any replica → primary. + const std::string& client_zone = redis_context->clientZone(); + if (!client_zone.empty()) { + // Try local replicas first + const auto* local_replicas = shard->replicasInZone(client_zone); + if (local_replicas) { + auto host = chooseRandomHost(*local_replicas, random_); + if (host) { + return host; + } + } + // Try local primary + if (shard->primaryZone() == client_zone && + shard->primary()->coarseHealth() == Upstream::Host::Health::Healthy) { + return shard->primary(); + } + } + // Fall back to any replica, then primary + if (!shard->replicas().hosts().empty()) { + return chooseRandomHost(shard->replicas(), random_); + } + return shard->primary(); + } } } return shard->primary(); @@ -201,10 +246,10 @@ bool RedisLoadBalancerContextImpl::isReadRequest( RedisLoadBalancerContextImpl::RedisLoadBalancerContextImpl( const std::string& key, bool enabled_hashtagging, bool is_redis_cluster, const NetworkFilters::Common::Redis::RespValue& request, - NetworkFilters::Common::Redis::Client::ReadPolicy read_policy) + NetworkFilters::Common::Redis::Client::ReadPolicy read_policy, const std::string& client_zone) : hash_key_(is_redis_cluster ? Crc16::crc16(hashtag(key, true)) : MurmurHash::murmurHash2(hashtag(key, enabled_hashtagging))), - is_read_(isReadRequest(request)), read_policy_(read_policy) {} + is_read_(isReadRequest(request)), read_policy_(read_policy), client_zone_(client_zone) {} // Inspired by the redis-cluster hashtagging algorithm // https://redis.io/topics/cluster-spec#keys-hash-tags @@ -227,8 +272,9 @@ absl::string_view RedisLoadBalancerContextImpl::hashtag(absl::string_view v, boo } RedisSpecifyShardContextImpl::RedisSpecifyShardContextImpl( uint64_t shard_index, const NetworkFilters::Common::Redis::RespValue& request, - NetworkFilters::Common::Redis::Client::ReadPolicy read_policy) - : RedisLoadBalancerContextImpl(std::to_string(shard_index), true, true, request, read_policy), + NetworkFilters::Common::Redis::Client::ReadPolicy read_policy, const std::string& client_zone) + : RedisLoadBalancerContextImpl(std::to_string(shard_index), true, true, request, read_policy, + client_zone), shard_index_(shard_index) {} } // namespace Redis diff --git a/source/extensions/clusters/redis/redis_cluster_lb.h b/source/extensions/clusters/redis/redis_cluster_lb.h index c07e758cf26b6..1a5f465107807 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.h +++ b/source/extensions/clusters/redis/redis_cluster_lb.h @@ -16,6 +16,7 @@ #include "source/extensions/filters/network/common/redis/supported_commands.h" #include "absl/container/btree_map.h" +#include "absl/container/flat_hash_map.h" #include "absl/synchronization/mutex.h" namespace Envoy { @@ -66,12 +67,21 @@ class ClusterSlot { using ClusterSlotsPtr = std::unique_ptr>; using ClusterSlotsSharedPtr = std::shared_ptr>; +// Map from host address to zone (used during zone discovery) +using HostZoneMap = absl::flat_hash_map; + class RedisLoadBalancerContext { public: virtual ~RedisLoadBalancerContext() = default; virtual bool isReadCommand() const PURE; virtual NetworkFilters::Common::Redis::Client::ReadPolicy readPolicy() const PURE; + + /** + * @return the client zone for zone-aware routing. + * Returns empty string if zone-aware routing is not configured. + */ + virtual const std::string& clientZone() const PURE; }; class RedisLoadBalancerContextImpl : public RedisLoadBalancerContext, @@ -87,12 +97,14 @@ class RedisLoadBalancerContextImpl : public RedisLoadBalancerContext, * will be hashed using crc16. * @param request specify the Redis request. * @param read_policy specify the read policy. + * @param client_zone specify the client zone for zone-aware routing. */ RedisLoadBalancerContextImpl(const std::string& key, bool enabled_hashtagging, bool is_redis_cluster, const NetworkFilters::Common::Redis::RespValue& request, NetworkFilters::Common::Redis::Client::ReadPolicy read_policy = - NetworkFilters::Common::Redis::Client::ReadPolicy::Primary); + NetworkFilters::Common::Redis::Client::ReadPolicy::Primary, + const std::string& client_zone = ""); // Upstream::LoadBalancerContextBase absl::optional computeHashKey() override { return hash_key_; } @@ -103,6 +115,8 @@ class RedisLoadBalancerContextImpl : public RedisLoadBalancerContext, return read_policy_; } + const std::string& clientZone() const override { return client_zone_; } + private: absl::string_view hashtag(absl::string_view v, bool enabled); @@ -111,6 +125,7 @@ class RedisLoadBalancerContextImpl : public RedisLoadBalancerContext, const absl::optional hash_key_; const bool is_read_; const NetworkFilters::Common::Redis::Client::ReadPolicy read_policy_; + const std::string client_zone_; }; class RedisSpecifyShardContextImpl : public RedisLoadBalancerContextImpl { @@ -120,11 +135,13 @@ class RedisSpecifyShardContextImpl : public RedisLoadBalancerContextImpl { * @param shard_index specify the shard index for the Redis request. * @param request specify the Redis request. * @param read_policy specify the read policy. + * @param client_zone specify the client zone for zone-aware routing. */ RedisSpecifyShardContextImpl(uint64_t shard_index, const NetworkFilters::Common::Redis::RespValue& request, NetworkFilters::Common::Redis::Client::ReadPolicy read_policy = - NetworkFilters::Common::Redis::Client::ReadPolicy::Primary); + NetworkFilters::Common::Redis::Client::ReadPolicy::Primary, + const std::string& client_zone = ""); // Upstream::LoadBalancerContextBase absl::optional computeHashKey() override { return shard_index_; } @@ -140,7 +157,7 @@ class ClusterSlotUpdateCallBack { /** * Callback when cluster slot is updated * @param slots provides the updated cluster slots. - * @param all_hosts provides the updated hosts. + * @param all_hosts provides the updated hosts (with zone info already set in host locality). * @return indicate if the cluster slot is updated or not. */ virtual bool onClusterSlotUpdate(ClusterSlotsSharedPtr&& slots, @@ -174,24 +191,60 @@ class RedisClusterLoadBalancerFactory : public ClusterSlotUpdateCallBack, private: class RedisShard { public: + // Constructor derives zone information from host localities RedisShard(Upstream::HostConstSharedPtr primary, Upstream::HostVectorConstSharedPtr replicas, Upstream::HostVectorConstSharedPtr all_hosts, Random::RandomGenerator& random) : primary_(std::move(primary)) { + // Derive primary zone from host's locality + primary_zone_ = primary_->locality().zone(); + replicas_.updateHosts(Upstream::HostSetImpl::partitionHosts( std::move(replicas), Upstream::HostsPerLocalityImpl::empty()), nullptr, {}, {}, random.random()); all_hosts_.updateHosts(Upstream::HostSetImpl::partitionHosts( std::move(all_hosts), Upstream::HostsPerLocalityImpl::empty()), nullptr, {}, {}, random.random()); + + // Group replicas by zone from host localities for efficient zone-aware routing + absl::flat_hash_map zone_hosts; + for (const auto& host : replicas_.hosts()) { + const std::string& zone = host->locality().zone(); + if (!zone.empty()) { + zone_hosts[zone].push_back(host); + } + } + // Convert each zone's hosts to HostSetImpl + for (auto& [zone, hosts] : zone_hosts) { + auto host_set = std::make_unique(0, absl::nullopt, absl::nullopt); + auto hosts_ptr = std::make_shared(std::move(hosts)); + host_set->updateHosts(Upstream::HostSetImpl::partitionHosts( + std::move(hosts_ptr), Upstream::HostsPerLocalityImpl::empty()), + nullptr, {}, {}, random.random()); + replicas_by_zone_[zone] = std::move(host_set); + } } const Upstream::HostConstSharedPtr primary() const { return primary_; } const Upstream::HostSetImpl& replicas() const { return replicas_; } const Upstream::HostSetImpl& allHosts() const { return all_hosts_; } + // Zone information for zone-aware routing + const std::string& primaryZone() const { return primary_zone_; } + + // Get replicas in a specific zone. Returns nullptr if no replicas in that zone. + const Upstream::HostSetImpl* replicasInZone(const std::string& zone) const { + if (zone.empty()) { + return nullptr; + } + auto it = replicas_by_zone_.find(zone); + return (it != replicas_by_zone_.end()) ? it->second.get() : nullptr; + } + private: const Upstream::HostConstSharedPtr primary_; Upstream::HostSetImpl replicas_{0, absl::nullopt, absl::nullopt}; Upstream::HostSetImpl all_hosts_{0, absl::nullopt, absl::nullopt}; + std::string primary_zone_; + absl::flat_hash_map> replicas_by_zone_; }; using RedisShardSharedPtr = std::shared_ptr; diff --git a/source/extensions/filters/network/common/redis/client.h b/source/extensions/filters/network/common/redis/client.h index a1fc16347f73d..b90212ea63d44 100644 --- a/source/extensions/filters/network/common/redis/client.h +++ b/source/extensions/filters/network/common/redis/client.h @@ -117,7 +117,17 @@ using ClientPtr = std::unique_ptr; /** * Read policy to use for Redis cluster. */ -enum class ReadPolicy { Primary, PreferPrimary, Replica, PreferReplica, Any }; +enum class ReadPolicy { + Primary, + PreferPrimary, + Replica, + PreferReplica, + Any, + // Zone-aware routing: prefer replicas in same AZ, fallback to any replica, then primary + AzAffinity, + // Zone-aware routing: prefer replicas in same AZ, then primary in same AZ, then any + AzAffinityReplicasAndPrimary +}; /** * Configuration for a redis connection pool. diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index 83940ca3453fc..74b4042fcd636 100644 --- a/source/extensions/filters/network/common/redis/client_impl.cc +++ b/source/extensions/filters/network/common/redis/client_impl.cc @@ -63,6 +63,14 @@ ConfigImpl::ConfigImpl( case envoy::extensions::filters::network::redis_proxy::v3::RedisProxy::ConnPoolSettings::ANY: read_policy_ = ReadPolicy::Any; break; + case envoy::extensions::filters::network::redis_proxy::v3::RedisProxy::ConnPoolSettings:: + AZ_AFFINITY: + read_policy_ = ReadPolicy::AzAffinity; + break; + case envoy::extensions::filters::network::redis_proxy::v3::RedisProxy::ConnPoolSettings:: + AZ_AFFINITY_REPLICAS_AND_PRIMARY: + read_policy_ = ReadPolicy::AzAffinityReplicasAndPrimary; + break; } if (config.has_connection_rate_limit()) { diff --git a/source/extensions/filters/network/redis_proxy/config.cc b/source/extensions/filters/network/redis_proxy/config.cc index c5481540f4954..ce39330a7c22c 100644 --- a/source/extensions/filters/network/redis_proxy/config.cc +++ b/source/extensions/filters/network/redis_proxy/config.cc @@ -102,11 +102,14 @@ Network::FilterFactoryCb RedisProxyFilterConfigFactory::createFilterFactoryFromP Stats::ScopeSharedPtr stats_scope = context.scope().createScope(fmt::format("cluster.{}.redis_cluster", cluster)); + // Get zone from LocalInfo for fallback when client_zone not explicitly configured + const std::string& local_zone = server_context.localInfo().zoneName(); auto conn_pool_ptr = std::make_shared( cluster, server_context.clusterManager(), Common::Redis::Client::ClientFactoryImpl::instance_, server_context.threadLocal(), proto_config.settings(), server_context.api(), std::move(stats_scope), redis_command_stats, - refresh_manager, filter_config->dns_cache_, aws_iam_config, aws_iam_authenticator); + refresh_manager, filter_config->dns_cache_, aws_iam_config, aws_iam_authenticator, + local_zone); conn_pool_ptr->init(); upstreams.emplace(cluster, conn_pool_ptr); } diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc index 979887ea11ebc..6e3f7cff6fcf3 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc @@ -53,13 +53,15 @@ InstanceImpl::InstanceImpl( const Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr& dns_cache, absl::optional aws_iam_config, absl::optional - aws_iam_authenticator) + aws_iam_authenticator, + const std::string& local_zone) : cluster_name_(cluster_name), cm_(cm), client_factory_(client_factory), tls_(tls.allocateSlot()), config_(new Common::Redis::Client::ConfigImpl(config)), api_(api), stats_scope_(std::move(stats_scope)), redis_command_stats_(redis_command_stats), redis_cluster_stats_{REDIS_CLUSTER_STATS(POOL_COUNTER(*stats_scope_))}, refresh_manager_(std::move(refresh_manager)), dns_cache_(dns_cache), - aws_iam_authenticator_(aws_iam_authenticator), aws_iam_config_(aws_iam_config) {} + aws_iam_authenticator_(aws_iam_authenticator), aws_iam_config_(aws_iam_config), + local_zone_(local_zone) {} void InstanceImpl::init() { // Note: `this` and `cluster_name` have a a lifetime of the filter. @@ -120,7 +122,7 @@ InstanceImpl::ThreadLocalPool::ThreadLocalPool( stats_scope_(parent->stats_scope_), redis_command_stats_(parent->redis_command_stats_), redis_cluster_stats_(parent->redis_cluster_stats_), refresh_manager_(parent->refresh_manager_), aws_iam_authenticator_(aws_iam_authenticator), - aws_iam_config_(aws_iam_config) { + aws_iam_config_(aws_iam_config), client_zone_(parent->localZone()) { cluster_update_handle_ = parent->cm_.addThreadLocalClusterUpdateCallbacks(*this); Upstream::ThreadLocalCluster* cluster = parent->cm_.getThreadLocalCluster(cluster_name_); @@ -352,7 +354,7 @@ uint16_t InstanceImpl::ThreadLocalPool::shardSize() { unique_hosts.reserve(Envoy::Extensions::Clusters::Redis::MaxSlot); for (uint16_t size = 0; size < Envoy::Extensions::Clusters::Redis::MaxSlot; size++) { Clusters::Redis::RedisSpecifyShardContextImpl lb_context( - size, request, Common::Redis::Client::ReadPolicy::Primary); + size, request, Common::Redis::Client::ReadPolicy::Primary, client_zone_); Upstream::HostConstSharedPtr host = Upstream::LoadBalancer::onlyAllowSynchronousHostSelection( cluster_->loadBalancer().chooseHost(&lb_context)); if (!host) { @@ -375,8 +377,8 @@ InstanceImpl::ThreadLocalPool::makeRequest(const std::string& key, RespVariant&& Clusters::Redis::RedisLoadBalancerContextImpl lb_context( key, config_->enableHashtagging(), is_redis_cluster_, getRequest(request), - transaction.active_ ? Common::Redis::Client::ReadPolicy::Primary : config_->readPolicy()); - + transaction.active_ ? Common::Redis::Client::ReadPolicy::Primary : config_->readPolicy(), + client_zone_); Upstream::HostConstSharedPtr host = Upstream::LoadBalancer::onlyAllowSynchronousHostSelection( cluster_->loadBalancer().chooseHost(&lb_context)); if (!host) { @@ -399,7 +401,8 @@ InstanceImpl::ThreadLocalPool::makeRequestToShard(uint16_t shard_index, RespVari Clusters::Redis::RedisSpecifyShardContextImpl lb_context( shard_index, getRequest(request), - transaction.active_ ? Common::Redis::Client::ReadPolicy::Primary : config_->readPolicy()); + transaction.active_ ? Common::Redis::Client::ReadPolicy::Primary : config_->readPolicy(), + client_zone_); Upstream::HostConstSharedPtr host = Upstream::LoadBalancer::onlyAllowSynchronousHostSelection( cluster_->loadBalancer().chooseHost(&lb_context)); diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h index 0c720dc1865cf..7ada14fbe97af 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h @@ -96,7 +96,8 @@ class InstanceImpl : public Instance, public std::enable_shared_from_this aws_iam_config, absl::optional - aws_iam_authenticator); + aws_iam_authenticator, + const std::string& local_zone = ""); uint16_t shardSize() override; // RedisProxy::ConnPool::Instance Common::Redis::Client::PoolRequest* @@ -263,8 +264,11 @@ class InstanceImpl : public Instance, public std::enable_shared_from_this aws_iam_config_; // Per-shard stats map keyed by host address (e.g., "10.0.0.1:6379") absl::node_hash_map shard_stats_map_; + std::string client_zone_; // Zone from node.locality.zone }; + const std::string& localZone() const { return local_zone_; } + const std::string cluster_name_; Upstream::ClusterManager& cm_; Common::Redis::Client::ClientFactory& client_factory_; @@ -279,6 +283,7 @@ class InstanceImpl : public Instance, public std::enable_shared_from_this aws_iam_authenticator_; absl::optional aws_iam_config_; + const std::string local_zone_; // Zone from node.locality.zone }; } // namespace ConnPool diff --git a/test/extensions/clusters/redis/redis_cluster_lb_test.cc b/test/extensions/clusters/redis/redis_cluster_lb_test.cc index f46f43cec953c..bc90cb1dbf7a0 100644 --- a/test/extensions/clusters/redis/redis_cluster_lb_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_lb_test.cc @@ -20,8 +20,10 @@ class TestLoadBalancerContext : public RedisLoadBalancerContext, public Upstream::LoadBalancerContextBase { public: TestLoadBalancerContext(uint64_t hash_key, bool is_read, - NetworkFilters::Common::Redis::Client::ReadPolicy read_policy) - : hash_key_(hash_key), is_read_(is_read), read_policy_(read_policy) {} + NetworkFilters::Common::Redis::Client::ReadPolicy read_policy, + const std::string& client_zone = "") + : hash_key_(hash_key), is_read_(is_read), read_policy_(read_policy), + client_zone_(client_zone) {} TestLoadBalancerContext(absl::optional hash) : hash_key_(hash) {} @@ -32,16 +34,25 @@ class TestLoadBalancerContext : public RedisLoadBalancerContext, NetworkFilters::Common::Redis::Client::ReadPolicy readPolicy() const override { return read_policy_; }; + const std::string& clientZone() const override { return client_zone_; } absl::optional hash_key_; bool is_read_; NetworkFilters::Common::Redis::Client::ReadPolicy read_policy_; + std::string client_zone_; }; class RedisClusterLoadBalancerTest : public Event::TestUsingSimulatedTime, public testing::Test { public: RedisClusterLoadBalancerTest() = default; + // Helper to create a host with locality zone set + Upstream::HostSharedPtr makeHostWithZone(const std::string& url, const std::string& zone) { + envoy::config::core::v3::Locality locality; + locality.set_zone(zone); + return Upstream::makeTestHost(info_, url, locality); + } + void init() { factory_ = std::make_shared(random_); lb_ = std::make_unique(factory_); @@ -53,11 +64,12 @@ class RedisClusterLoadBalancerTest : public Event::TestUsingSimulatedTime, publi const std::vector>& expected_assignments, bool read_command = false, NetworkFilters::Common::Redis::Client::ReadPolicy read_policy = - NetworkFilters::Common::Redis::Client::ReadPolicy::Primary) { + NetworkFilters::Common::Redis::Client::ReadPolicy::Primary, + const std::string& client_zone = "") { Upstream::LoadBalancerPtr lb = lb_->factory()->create(lb_params_); for (auto& assignment : expected_assignments) { - TestLoadBalancerContext context(assignment.first, read_command, read_policy); + TestLoadBalancerContext context(assignment.first, read_command, read_policy, client_zone); auto host = lb->chooseHost(&context).host; EXPECT_FALSE(host == nullptr); EXPECT_EQ(hosts[assignment.second]->address()->asString(), host->address()->asString()); @@ -629,6 +641,248 @@ TEST_F(RedisLoadBalancerContextImplTest, ReadOnlyCommand) { context1.readPolicy()); } +TEST_F(RedisLoadBalancerContextImplTest, ClientZone) { + std::vector get_foo(2); + get_foo[0].type(NetworkFilters::Common::Redis::RespType::BulkString); + get_foo[0].asString() = "get"; + get_foo[1].type(NetworkFilters::Common::Redis::RespType::BulkString); + get_foo[1].asString() = "foo"; + + NetworkFilters::Common::Redis::RespValue get_request; + get_request.type(NetworkFilters::Common::Redis::RespType::Array); + get_request.asArray().swap(get_foo); + + // Test with client zone specified + RedisLoadBalancerContextImpl context1( + "foo", true, true, get_request, NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinity, + "us-east-1a"); + + EXPECT_EQ("us-east-1a", context1.clientZone()); + EXPECT_EQ(NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinity, context1.readPolicy()); + + // Test with empty client zone + RedisLoadBalancerContextImpl context2( + "foo", true, true, get_request, + NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinityReplicasAndPrimary); + + EXPECT_EQ("", context2.clientZone()); + EXPECT_EQ(NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinityReplicasAndPrimary, + context2.readPolicy()); +} + +// Tests for AZ_AFFINITY read policy with replicas in the same zone +TEST_F(RedisClusterLoadBalancerTest, AzAffinityWithLocalReplica) { + // Setup: primary in zone-a, replica in zone-a (same as client) + // Hosts must have locality zones set - RedisShard reads zone from host->locality().zone() + Upstream::HostVector hosts{ + makeHostWithZone("tcp://127.0.0.1:90", "zone-a"), // primary, zone-a + makeHostWithZone("tcp://127.0.0.1:91", "zone-b"), // primary, zone-b + makeHostWithZone("tcp://127.0.0.2:90", "zone-a"), // replica for slot 0, zone-a + makeHostWithZone("tcp://127.0.0.2:91", "zone-b"), // replica for slot 1, zone-b + }; + + ClusterSlotsPtr slots = std::make_unique>(std::vector{ + ClusterSlot(0, 8000, hosts[0]->address()), + ClusterSlot(8001, 16383, hosts[1]->address()), + }); + slots->at(0).addReplica(hosts[2]->address()); + slots->at(1).addReplica(hosts[3]->address()); + + Upstream::HostMap all_hosts; + std::transform(hosts.begin(), hosts.end(), std::inserter(all_hosts, all_hosts.end()), makePair); + init(); + factory_->onClusterSlotUpdate(std::move(slots), all_hosts); + + // Client is in zone-a, should prefer replica in zone-a for slot 0 + const std::vector> expected_assignments = { + {0, 2}, // slot 0: replica in zone-a + {8001, 3}, // slot 1: replica in zone-b (no local replica, fall back to any replica) + }; + validateAssignment(hosts, expected_assignments, true, + NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinity, "zone-a"); +} + +// Tests for AZ_AFFINITY when no local replica exists - should fall back to any replica +TEST_F(RedisClusterLoadBalancerTest, AzAffinityNoLocalReplica) { + // Hosts must have locality zones set - RedisShard reads zone from host->locality().zone() + Upstream::HostVector hosts{ + makeHostWithZone("tcp://127.0.0.1:90", "zone-a"), // primary, zone-a + makeHostWithZone("tcp://127.0.0.2:90", "zone-b"), // replica, zone-b + }; + + ClusterSlotsPtr slots = std::make_unique>(std::vector{ + ClusterSlot(0, 16383, hosts[0]->address()), + }); + slots->at(0).addReplica(hosts[1]->address()); + + Upstream::HostMap all_hosts; + std::transform(hosts.begin(), hosts.end(), std::inserter(all_hosts, all_hosts.end()), makePair); + init(); + factory_->onClusterSlotUpdate(std::move(slots), all_hosts); + + // Client is in zone-c (no local replica), should fall back to any replica + const std::vector> expected_assignments = { + {0, 1}, // falls back to replica in zone-b + }; + validateAssignment(hosts, expected_assignments, true, + NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinity, "zone-c"); +} + +// Tests for AZ_AFFINITY when no replica exists - should fall back to primary +TEST_F(RedisClusterLoadBalancerTest, AzAffinityNoReplica) { + // Hosts must have locality zones set - RedisShard reads zone from host->locality().zone() + Upstream::HostVector hosts{ + makeHostWithZone("tcp://127.0.0.1:90", "zone-a"), // primary only + }; + + ClusterSlotsPtr slots = std::make_unique>(std::vector{ + ClusterSlot(0, 16383, hosts[0]->address()), + }); + + Upstream::HostMap all_hosts; + std::transform(hosts.begin(), hosts.end(), std::inserter(all_hosts, all_hosts.end()), makePair); + init(); + factory_->onClusterSlotUpdate(std::move(slots), all_hosts); + + // No replicas, should fall back to primary + const std::vector> expected_assignments = { + {0, 0}, // falls back to primary + }; + validateAssignment(hosts, expected_assignments, true, + NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinity, "zone-a"); +} + +// Tests for AZ_AFFINITY_REPLICAS_AND_PRIMARY with local replica +TEST_F(RedisClusterLoadBalancerTest, AzAffinityReplicasAndPrimaryWithLocalReplica) { + // Hosts must have locality zones set - RedisShard reads zone from host->locality().zone() + Upstream::HostVector hosts{ + makeHostWithZone("tcp://127.0.0.1:90", "zone-a"), // primary, zone-a + makeHostWithZone("tcp://127.0.0.2:90", "zone-a"), // replica, zone-a (same as client) + }; + + ClusterSlotsPtr slots = std::make_unique>(std::vector{ + ClusterSlot(0, 16383, hosts[0]->address()), + }); + slots->at(0).addReplica(hosts[1]->address()); + + Upstream::HostMap all_hosts; + std::transform(hosts.begin(), hosts.end(), std::inserter(all_hosts, all_hosts.end()), makePair); + init(); + factory_->onClusterSlotUpdate(std::move(slots), all_hosts); + + // Client is in zone-a, should prefer replica in zone-a + const std::vector> expected_assignments = { + {0, 1}, // replica in zone-a + }; + validateAssignment( + hosts, expected_assignments, true, + NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinityReplicasAndPrimary, "zone-a"); +} + +// Tests for AZ_AFFINITY_REPLICAS_AND_PRIMARY - prefer local primary when no local replica +TEST_F(RedisClusterLoadBalancerTest, AzAffinityReplicasAndPrimaryLocalPrimary) { + // Hosts must have locality zones set - RedisShard reads zone from host->locality().zone() + Upstream::HostVector hosts{ + makeHostWithZone("tcp://127.0.0.1:90", "zone-a"), // primary, zone-a (same as client) + makeHostWithZone("tcp://127.0.0.2:90", "zone-b"), // replica, zone-b + }; + + ClusterSlotsPtr slots = std::make_unique>(std::vector{ + ClusterSlot(0, 16383, hosts[0]->address()), + }); + slots->at(0).addReplica(hosts[1]->address()); + + Upstream::HostMap all_hosts; + std::transform(hosts.begin(), hosts.end(), std::inserter(all_hosts, all_hosts.end()), makePair); + init(); + factory_->onClusterSlotUpdate(std::move(slots), all_hosts); + + // Client is in zone-a, no local replica, but primary is in zone-a + const std::vector> expected_assignments = { + {0, 0}, // primary in zone-a (no local replica, but local primary) + }; + validateAssignment( + hosts, expected_assignments, true, + NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinityReplicasAndPrimary, "zone-a"); +} + +// Tests for AZ_AFFINITY_REPLICAS_AND_PRIMARY - fall back to any replica when no local hosts +TEST_F(RedisClusterLoadBalancerTest, AzAffinityReplicasAndPrimaryFallbackToReplica) { + // Hosts must have locality zones set - RedisShard reads zone from host->locality().zone() + Upstream::HostVector hosts{ + makeHostWithZone("tcp://127.0.0.1:90", "zone-a"), // primary, zone-a + makeHostWithZone("tcp://127.0.0.2:90", "zone-b"), // replica, zone-b + }; + + ClusterSlotsPtr slots = std::make_unique>(std::vector{ + ClusterSlot(0, 16383, hosts[0]->address()), + }); + slots->at(0).addReplica(hosts[1]->address()); + + Upstream::HostMap all_hosts; + std::transform(hosts.begin(), hosts.end(), std::inserter(all_hosts, all_hosts.end()), makePair); + init(); + factory_->onClusterSlotUpdate(std::move(slots), all_hosts); + + // Client is in zone-c (no local hosts), should fall back to any replica + const std::vector> expected_assignments = { + {0, 1}, // falls back to replica + }; + validateAssignment( + hosts, expected_assignments, true, + NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinityReplicasAndPrimary, "zone-c"); +} + +// Tests for AZ_AFFINITY_REPLICAS_AND_PRIMARY - fall back to primary when no replicas +TEST_F(RedisClusterLoadBalancerTest, AzAffinityReplicasAndPrimaryNoReplica) { + // Hosts must have locality zones set - RedisShard reads zone from host->locality().zone() + Upstream::HostVector hosts{ + makeHostWithZone("tcp://127.0.0.1:90", "zone-a"), // primary only, zone-a + }; + + ClusterSlotsPtr slots = std::make_unique>(std::vector{ + ClusterSlot(0, 16383, hosts[0]->address()), + }); + + Upstream::HostMap all_hosts; + std::transform(hosts.begin(), hosts.end(), std::inserter(all_hosts, all_hosts.end()), makePair); + init(); + factory_->onClusterSlotUpdate(std::move(slots), all_hosts); + + // No replicas, should fall back to primary + const std::vector> expected_assignments = { + {0, 0}, // falls back to primary + }; + validateAssignment( + hosts, expected_assignments, true, + NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinityReplicasAndPrimary, "zone-c"); +} + +// Tests for AZ_AFFINITY with empty client zone - should behave like PreferReplica +TEST_F(RedisClusterLoadBalancerTest, AzAffinityEmptyClientZone) { + Upstream::HostVector hosts{ + Upstream::makeTestHost(info_, "tcp://127.0.0.1:90"), // primary + Upstream::makeTestHost(info_, "tcp://127.0.0.2:90"), // replica + }; + + ClusterSlotsPtr slots = std::make_unique>(std::vector{ + ClusterSlot(0, 16383, hosts[0]->address()), + }); + slots->at(0).addReplica(hosts[1]->address()); + + Upstream::HostMap all_hosts; + std::transform(hosts.begin(), hosts.end(), std::inserter(all_hosts, all_hosts.end()), makePair); + init(); + factory_->onClusterSlotUpdate(std::move(slots), all_hosts); + + // Empty client zone, should fall back to any replica + const std::vector> expected_assignments = { + {0, 1}, // any replica + }; + validateAssignment(hosts, expected_assignments, true, + NetworkFilters::Common::Redis::Client::ReadPolicy::AzAffinity, ""); +} + } // namespace Redis } // namespace Clusters } // namespace Extensions From 964046c26413b00cd56182666c11d4abde2d5863 Mon Sep 17 00:00:00 2001 From: Gavin Jeong Date: Fri, 16 Jan 2026 14:10:20 +0900 Subject: [PATCH 11/12] tls: Add certificate compression support (RFC 8879) Add TLS certificate compression with brotli, zstd, and zlib algorithms. This reduces TLS handshake size, especially beneficial for QUIC where the ServerHello needs to fit in the initial response. Key changes: - Move cert_compression from quic/ to tls/ for shared use - Add brotli and zstd algorithms alongside existing zlib - Add compression stats: ssl.certificate_compression..* - Add runtime flag (default: disabled) for safe rollout - Fix SSL_CTX app_data crash risk for QUIC by using SSL_CTX_get_ex_new_index() Runtime guard: envoy.reloadable_features.tls_support_certificate_compression Cherry-picked from upstream PR #42690 (not yet merged). --- changelogs/current.yaml | 6 + docs/root/_include/ssl_stats.rst | 3 + source/common/quic/BUILD | 7 +- source/common/quic/active_quic_listener.cc | 3 +- source/common/quic/cert_compression.cc | 119 ------- source/common/quic/cert_compression.h | 33 +- source/common/quic/envoy_quic_proof_source.cc | 30 +- source/common/quic/envoy_quic_proof_source.h | 7 +- ...nvoy_quic_proof_source_factory_interface.h | 4 +- source/common/runtime/runtime_features.cc | 2 + source/common/tls/BUILD | 27 ++ source/common/tls/cert_compression.cc | 325 ++++++++++++++++++ source/common/tls/cert_compression.h | 59 ++++ source/common/tls/context_impl.cc | 13 + source/common/tls/context_impl.h | 5 +- source/common/tls/ssl_ctx_stats_provider.h | 24 ++ source/common/tls/stats.cc | 4 + source/common/tls/stats.h | 14 + .../envoy_quic_proof_source_factory_impl.cc | 4 +- .../envoy_quic_proof_source_factory_impl.h | 3 +- test/common/quic/cert_compression_test.cc | 25 +- .../quic/envoy_quic_proof_source_test.cc | 3 +- test/common/tls/BUILD | 10 + test/common/tls/cert_compression_test.cc | 260 ++++++++++++++ test/common/tls/ssl_socket_test.cc | 53 +++ .../pending_proof_source_factory_impl.cc | 10 +- .../pending_proof_source_factory_impl.h | 3 +- .../integration/quic_http_integration_test.cc | 6 +- tools/spelling/spelling_dictionary.txt | 2 + 29 files changed, 880 insertions(+), 184 deletions(-) delete mode 100644 source/common/quic/cert_compression.cc create mode 100644 source/common/tls/cert_compression.cc create mode 100644 source/common/tls/cert_compression.h create mode 100644 source/common/tls/ssl_ctx_stats_provider.h create mode 100644 test/common/tls/cert_compression_test.cc diff --git a/changelogs/current.yaml b/changelogs/current.yaml index dd6dc555952d9..0bfd24670fa74 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -21,5 +21,11 @@ new_features: route read requests to replicas in the same availability zone. Zone discovery is enabled via :ref:`enable_zone_discovery `. Note: This feature currently works with Valkey only. +- area: tls + change: | + Added TLS certificate compression support (RFC 8879) with brotli, zstd, and zlib algorithms. + Compression reduces TLS handshake size, especially beneficial for QUIC where the ServerHello + needs to fit in the initial response. Enable via runtime guard + ``envoy.reloadable_features.tls_support_certificate_compression`` (defaults to ``false``). deprecated: diff --git a/docs/root/_include/ssl_stats.rst b/docs/root/_include/ssl_stats.rst index fa2f1e0a16bc3..b5033b50c8e80 100644 --- a/docs/root/_include/ssl_stats.rst +++ b/docs/root/_include/ssl_stats.rst @@ -19,3 +19,6 @@ sigalgs., Counter, Total successful TLS connections that used signature algorithm versions., Counter, Total successful TLS connections that used protocol version was_key_usage_invalid, Counter, Total successful TLS connections that used an `invalid keyUsage extension `_. (This is not available in BoringSSL FIPS yet due to `issue #28246 `_) + certificate_compression..compressed, Counter, Total certificates compressed using algorithm (brotli/zstd/zlib). Requires runtime flag ``envoy.reloadable_features.tls_support_certificate_compression``. + certificate_compression..total_uncompressed_bytes, Counter, Total bytes of certificates before compression using algorithm + certificate_compression..total_compressed_bytes, Counter, Total bytes of certificates after compression using algorithm diff --git a/source/common/quic/BUILD b/source/common/quic/BUILD index 169bc36a24fa7..0383acbae8a1a 100644 --- a/source/common/quic/BUILD +++ b/source/common/quic/BUILD @@ -705,13 +705,8 @@ envoy_cc_library( envoy_cc_library( name = "cert_compression_lib", - srcs = envoy_select_enable_http3(["cert_compression.cc"]), hdrs = envoy_select_enable_http3(["cert_compression.h"]), - external_deps = ["ssl"], deps = envoy_select_enable_http3([ - "//bazel/foreign_cc:zlib", - "//source/common/common:assert_lib", - "//source/common/common:logger_lib", - "//source/common/runtime:runtime_lib", + "//source/common/tls:cert_compression_lib", ]), ) diff --git a/source/common/quic/active_quic_listener.cc b/source/common/quic/active_quic_listener.cc index 697e9d29f6739..69ba3f8cd8368 100644 --- a/source/common/quic/active_quic_listener.cc +++ b/source/common/quic/active_quic_listener.cc @@ -65,7 +65,8 @@ ActiveQuicListener::ActiveQuicListener( absl::string_view(reinterpret_cast(random_seed_), sizeof(random_seed_)), quic::QuicRandom::GetInstance(), proof_source_factory.createQuicProofSource( - listen_socket_, listener_config.filterChainManager(), stats_, dispatcher.timeSource()), + listen_socket_, listener_config.filterChainManager(), stats_, dispatcher.timeSource(), + listener_config.listenerScope()), quic::KeyExchangeSource::Default()); auto connection_helper = std::make_unique(dispatcher_); crypto_config_->AddDefaultConfig(random, connection_helper->GetClock(), diff --git a/source/common/quic/cert_compression.cc b/source/common/quic/cert_compression.cc deleted file mode 100644 index d3caecd417c66..0000000000000 --- a/source/common/quic/cert_compression.cc +++ /dev/null @@ -1,119 +0,0 @@ -#include "source/common/quic/cert_compression.h" - -#include "source/common/common/assert.h" -#include "source/common/runtime/runtime_features.h" - -#include "openssl/tls1.h" - -#define ZLIB_CONST -#include "zlib.h" - -namespace Envoy { -namespace Quic { - -namespace { - -class ScopedZStream { -public: - using CleanupFunc = int (*)(z_stream*); - - ScopedZStream(z_stream& z, CleanupFunc cleanup) : z_(z), cleanup_(cleanup) {} - ~ScopedZStream() { cleanup_(&z_); } - -private: - z_stream& z_; - CleanupFunc cleanup_; -}; - -} // namespace - -void CertCompression::registerSslContext(SSL_CTX* ssl_ctx) { - auto ret = SSL_CTX_add_cert_compression_alg(ssl_ctx, TLSEXT_cert_compression_zlib, compressZlib, - decompressZlib); - ASSERT(ret == 1); -} - -int CertCompression::compressZlib(SSL*, CBB* out, const uint8_t* in, size_t in_len) { - - z_stream z = {}; - int rv = deflateInit(&z, Z_DEFAULT_COMPRESSION); - if (rv != Z_OK) { - IS_ENVOY_BUG(fmt::format("Cert compression failure in deflateInit: {}", rv)); - return FAILURE; - } - - ScopedZStream deleter(z, deflateEnd); - - const auto upper_bound = deflateBound(&z, in_len); - - uint8_t* out_buf = nullptr; - if (!CBB_reserve(out, &out_buf, upper_bound)) { - IS_ENVOY_BUG(fmt::format("Cert compression failure in allocating output CBB buffer of size {}", - upper_bound)); - return FAILURE; - } - - z.next_in = in; - z.avail_in = in_len; - z.next_out = out_buf; - z.avail_out = upper_bound; - - rv = deflate(&z, Z_FINISH); - if (rv != Z_STREAM_END) { - IS_ENVOY_BUG(fmt::format( - "Cert compression failure in deflate: {}, z.total_out {}, in_len {}, z.avail_in {}", rv, - z.avail_in, in_len, z.avail_in)); - return FAILURE; - } - - if (!CBB_did_write(out, z.total_out)) { - IS_ENVOY_BUG("CBB_did_write failed"); - return FAILURE; - } - - ENVOY_LOG(trace, "Cert compression successful"); - - return SUCCESS; -} - -int CertCompression::decompressZlib(SSL*, CRYPTO_BUFFER** out, size_t uncompressed_len, - const uint8_t* in, size_t in_len) { - z_stream z = {}; - int rv = inflateInit(&z); - if (rv != Z_OK) { - IS_ENVOY_BUG(fmt::format("Cert decompression failure in inflateInit: {}", rv)); - return FAILURE; - } - - ScopedZStream deleter(z, inflateEnd); - - z.next_in = in; - z.avail_in = in_len; - bssl::UniquePtr decompressed_data( - CRYPTO_BUFFER_alloc(&z.next_out, uncompressed_len)); - z.avail_out = uncompressed_len; - - rv = inflate(&z, Z_FINISH); - if (rv != Z_STREAM_END) { - ENVOY_LOG_PERIODIC(error, std::chrono::seconds(10), - "Cert decompression failure in inflate, possibly caused by invalid " - "compressed cert from peer: {}, z.total_out {}, uncompressed_len {}", - rv, z.total_out, uncompressed_len); - return FAILURE; - } - - if (z.total_out != uncompressed_len) { - ENVOY_LOG_PERIODIC(error, std::chrono::seconds(10), - "Decompression length did not match peer provided uncompressed length, " - "caused by either invalid peer handshake data or decompression error."); - return FAILURE; - } - - ENVOY_LOG(trace, "Cert decompression successful"); - - *out = decompressed_data.release(); - return SUCCESS; -} - -} // namespace Quic -} // namespace Envoy diff --git a/source/common/quic/cert_compression.h b/source/common/quic/cert_compression.h index 65be63cb8fde0..f49947ef256c6 100644 --- a/source/common/quic/cert_compression.h +++ b/source/common/quic/cert_compression.h @@ -1,30 +1,29 @@ #pragma once -#include "source/common/common/logger.h" - -#include "openssl/ssl.h" +#include "source/common/runtime/runtime_features.h" +#include "source/common/tls/cert_compression.h" namespace Envoy { namespace Quic { -/** - * Support for certificate compression and decompression in QUIC TLS handshakes. This often - * needed for the ServerHello to fit in the initial response and not need an additional round trip - * between client and server. - */ +// QUIC wrapper for TLS certificate compression. class CertCompression : protected Logger::Loggable { public: - // Registers compression and decompression functions on `ssl_ctx` if enabled. - static void registerSslContext(SSL_CTX* ssl_ctx); + using Algorithm = Extensions::TransportSockets::Tls::CertCompression::Algorithm; - // Callbacks for `SSL_CTX_add_cert_compression_alg`. - static int compressZlib(SSL* ssl, CBB* out, const uint8_t* in, size_t in_len); - static int decompressZlib(SSL*, CRYPTO_BUFFER** out, size_t uncompressed_len, const uint8_t* in, - size_t in_len); + static void registerSslContext(SSL_CTX* ssl_ctx, Stats::Scope* scope = nullptr) { + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.tls_support_certificate_compression")) { + Extensions::TransportSockets::Tls::CertCompression::registerAlgorithms( + ssl_ctx, {Algorithm::Brotli, Algorithm::Zstd, Algorithm::Zlib}, scope); + } else { + Extensions::TransportSockets::Tls::CertCompression::registerAlgorithms( + ssl_ctx, {Algorithm::Zlib}, scope); + } + } - // Defined return values for callbacks from `SSL_CTX_add_cert_compression_alg`. - static constexpr int SUCCESS = 1; - static constexpr int FAILURE = 0; + static constexpr int SUCCESS = Extensions::TransportSockets::Tls::CertCompression::SUCCESS; + static constexpr int FAILURE = Extensions::TransportSockets::Tls::CertCompression::FAILURE; }; } // namespace Quic diff --git a/source/common/quic/envoy_quic_proof_source.cc b/source/common/quic/envoy_quic_proof_source.cc index bb166c52e7a99..6a77ba1fa9a2e 100644 --- a/source/common/quic/envoy_quic_proof_source.cc +++ b/source/common/quic/envoy_quic_proof_source.cc @@ -134,7 +134,7 @@ void EnvoyQuicProofSource::updateFilterChainManager( } void EnvoyQuicProofSource::OnNewSslCtx(SSL_CTX* ssl_ctx) { - CertCompression::registerSslContext(ssl_ctx); + CertCompression::registerSslContext(ssl_ctx, &stats_scope_); // Try to set up keylog callback for QUIC SSL contexts setupQuicKeylogCallback(ssl_ctx); @@ -184,15 +184,15 @@ void EnvoyQuicProofSource::quicKeylogCallback(const SSL* ssl, const char* line) try { // Convert QUIC addresses back to Envoy addresses for the bridge std::string server_addr_str = absl::StrCat( - keylog_info.server_address.host().ToString(), ":", + keylog_info.server_address.host().ToString(), ":", keylog_info.server_address.port()); std::string client_addr_str = absl::StrCat( - keylog_info.client_address.host().ToString(), ":", + keylog_info.client_address.host().ToString(), ":", keylog_info.client_address.port()); - - Network::Address::InstanceConstSharedPtr local_addr = + + Network::Address::InstanceConstSharedPtr local_addr = Network::Utility::parseInternetAddressAndPortNoThrow(server_addr_str); - Network::Address::InstanceConstSharedPtr remote_addr = + Network::Address::InstanceConstSharedPtr remote_addr = Network::Utility::parseInternetAddressAndPortNoThrow(client_addr_str); if (local_addr && remote_addr) { @@ -225,11 +225,11 @@ void EnvoyQuicProofSource::quicKeylogCallback(const SSL* ssl, const char* line) } void EnvoyQuicProofSource::QuicKeylogBridge::writeKeylog( - const Ssl::ContextConfig& config, + const Ssl::ContextConfig& config, const Network::Address::Instance& local_addr, - const Network::Address::Instance& remote_addr, + const Network::Address::Instance& remote_addr, const char* line) { - + const std::string& keylog_path = config.tlsKeyLogPath(); if (keylog_path.empty()) { return; @@ -238,12 +238,12 @@ void EnvoyQuicProofSource::QuicKeylogBridge::writeKeylog( // Check address filtering const auto& local_ip_list = config.tlsKeyLogLocal(); const auto& remote_ip_list = config.tlsKeyLogRemote(); - + bool local_match = (local_ip_list.getIpListSize() == 0 || local_ip_list.contains(local_addr)); bool remote_match = (remote_ip_list.getIpListSize() == 0 || remote_ip_list.contains(remote_addr)); - + if (!local_match || !remote_match) { - ENVOY_LOG(debug, "QUIC keylog filtered out by address match (local={}, remote={})", + ENVOY_LOG(debug, "QUIC keylog filtered out by address match (local={}, remote={})", local_match, remote_match); return; } @@ -253,13 +253,13 @@ void EnvoyQuicProofSource::QuicKeylogBridge::writeKeylog( auto& access_log_manager = config.accessLogManager(); auto file_or_error = access_log_manager.createAccessLog( Filesystem::FilePathAndType{Filesystem::DestinationType::File, keylog_path}); - + if (file_or_error.ok()) { auto keylog_file = file_or_error.value(); keylog_file->write(absl::StrCat(line, "\n")); ENVOY_LOG(debug, "QUIC keylog written via bridge to {}: {}", keylog_path, line); } else { - ENVOY_LOG(warn, "Failed to create keylog file {}: {}", keylog_path, + ENVOY_LOG(warn, "Failed to create keylog file {}: {}", keylog_path, file_or_error.status().message()); } } catch (const std::exception& e) { @@ -273,7 +273,7 @@ int EnvoyQuicProofSource::sslSocketIndex() { return ssl_socket_index; } -void EnvoyQuicProofSource::storeKeylogInfo(const Network::FilterChain& filter_chain, +void EnvoyQuicProofSource::storeKeylogInfo(const Network::FilterChain& filter_chain, std::shared_ptr config, const quic::QuicSocketAddress& server_address, const quic::QuicSocketAddress& client_address) const { diff --git a/source/common/quic/envoy_quic_proof_source.h b/source/common/quic/envoy_quic_proof_source.h index a521953c13682..cc4f5ac6ba917 100644 --- a/source/common/quic/envoy_quic_proof_source.h +++ b/source/common/quic/envoy_quic_proof_source.h @@ -3,6 +3,7 @@ #include #include "envoy/ssl/context_config.h" +#include "envoy/stats/scope.h" #include "source/common/common/thread.h" #include "source/common/quic/envoy_quic_proof_source_base.h" @@ -27,9 +28,10 @@ class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase { }; EnvoyQuicProofSource(Network::Socket& listen_socket, Network::FilterChainManager& filter_chain_manager, - Server::ListenerStats& listener_stats, TimeSource& time_source) + Server::ListenerStats& listener_stats, TimeSource& time_source, + Stats::Scope& stats_scope) : listen_socket_(listen_socket), filter_chain_manager_(&filter_chain_manager), - listener_stats_(listener_stats), time_source_(time_source) {} + listener_stats_(listener_stats), time_source_(time_source), stats_scope_(stats_scope) {} ~EnvoyQuicProofSource() override = default; @@ -101,6 +103,7 @@ class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase { Network::FilterChainManager* filter_chain_manager_{nullptr}; Server::ListenerStats& listener_stats_; TimeSource& time_source_; + Stats::Scope& stats_scope_; mutable absl::Mutex keylog_cache_mutex_; mutable std::unordered_map keylog_config_cache_ ABSL_GUARDED_BY(keylog_cache_mutex_); diff --git a/source/common/quic/envoy_quic_proof_source_factory_interface.h b/source/common/quic/envoy_quic_proof_source_factory_interface.h index 9de1df10f8f85..b34f4018c2b0e 100644 --- a/source/common/quic/envoy_quic_proof_source_factory_interface.h +++ b/source/common/quic/envoy_quic_proof_source_factory_interface.h @@ -3,6 +3,7 @@ #include "envoy/config/typed_config.h" #include "envoy/network/filter.h" #include "envoy/network/socket.h" +#include "envoy/stats/scope.h" #include "source/server/listener_stats.h" @@ -21,7 +22,8 @@ class EnvoyQuicProofSourceFactoryInterface : public Config::TypedFactory { virtual std::unique_ptr createQuicProofSource(Network::Socket& listen_socket, Network::FilterChainManager& filter_chain_manager, - Server::ListenerStats& listener_stats, TimeSource& time_source) PURE; + Server::ListenerStats& listener_stats, TimeSource& time_source, + Stats::Scope& stats_scope) PURE; }; } // namespace Quic diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 9d0156175754d..116b508bd0911 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -114,6 +114,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_always_use_v6); FALSE_RUNTIME_GUARD(envoy_restart_features_upstream_http_filters_with_tcp_proxy); // TODO(danzh) false deprecate it once QUICHE has its own enable/disable flag. FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_reject_all); +// TODO: flip to true after sufficient testing of TLS certificate compression. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_tls_support_certificate_compression); // TODO(#10646) change to true when UHV is sufficiently tested // For more information about Universal Header Validation, please see // https://github.com/envoyproxy/envoy/issues/10646 diff --git a/source/common/tls/BUILD b/source/common/tls/BUILD index b4981836f9e9f..041fe390c0f9e 100644 --- a/source/common/tls/BUILD +++ b/source/common/tls/BUILD @@ -182,6 +182,8 @@ envoy_cc_library( # TLS is core functionality. visibility = ["//visibility:public"], deps = [ + ":cert_compression_lib", + ":ssl_ctx_stats_provider_lib", ":stats_lib", ":utility_lib", "//envoy/ssl:context_config_interface", @@ -259,3 +261,28 @@ envoy_cc_library( "//source/common/network:address_lib", ], ) + +envoy_cc_library( + name = "ssl_ctx_stats_provider_lib", + hdrs = ["ssl_ctx_stats_provider.h"], + deps = [ + ":stats_lib", + ], +) + +envoy_cc_library( + name = "cert_compression_lib", + srcs = ["cert_compression.cc"], + hdrs = ["cert_compression.h"], + external_deps = ["ssl"], + deps = [ + ":ssl_ctx_stats_provider_lib", + "//envoy/ssl:context_config_interface", + "//source/common/common:assert_lib", + "//source/common/common:logger_lib", + "//bazel/foreign_cc:zlib", + "//bazel/foreign_cc:zstd", + "@org_brotli//:brotlidec", + "@org_brotli//:brotlienc", + ], +) diff --git a/source/common/tls/cert_compression.cc b/source/common/tls/cert_compression.cc new file mode 100644 index 0000000000000..c57129682b672 --- /dev/null +++ b/source/common/tls/cert_compression.cc @@ -0,0 +1,325 @@ +#include "source/common/tls/cert_compression.h" + +#include "source/common/common/assert.h" +#include "source/common/common/macros.h" +#include "source/common/tls/stats.h" + +#include "brotli/decode.h" +#include "brotli/encode.h" +#include "openssl/tls1.h" +#include "zstd.h" + +#define ZLIB_CONST +#include "zlib.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { + +namespace { + +class ScopedZStream { +public: + using CleanupFunc = int (*)(z_stream*); + + ScopedZStream(z_stream& z, CleanupFunc cleanup) : z_(z), cleanup_(cleanup) {} + ~ScopedZStream() { cleanup_(&z_); } + +private: + z_stream& z_; + CleanupFunc cleanup_; +}; + +// ex_data index for Stats::Scope* - avoids conflict with QUICHE own SSL_CTX usage. +int getScopeExDataIndex() { + static int index = SSL_CTX_get_ex_new_index(0, nullptr, nullptr, nullptr, nullptr); + return index; +} + +void recordCompressedCertSize(SSL* ssl, size_t uncompressed_size, size_t compressed_size, + const std::string& algo) { + if (ssl == nullptr) { + return; + } + SSL_CTX* ssl_ctx = SSL_get_SSL_CTX(ssl); + if (ssl_ctx == nullptr) { + return; + } + auto* scope = static_cast(SSL_CTX_get_ex_data(ssl_ctx, getScopeExDataIndex())); + if (scope != nullptr) { + const std::string prefix = "ssl.certificate_compression." + algo + "."; + CertCompressionStats stats = generateCertCompressionStats(*scope, prefix); + stats.compressed_.inc(); + stats.total_uncompressed_bytes_.add(uncompressed_size); + stats.total_compressed_bytes_.add(compressed_size); + } +} + +} // namespace + +void CertCompression::registerBrotli(SSL_CTX* ssl_ctx) { + auto ret = SSL_CTX_add_cert_compression_alg(ssl_ctx, TLSEXT_cert_compression_brotli, + compressBrotli, decompressBrotli); + ASSERT(ret == 1); +} + +void CertCompression::registerZstd(SSL_CTX* ssl_ctx) { + auto ret = SSL_CTX_add_cert_compression_alg(ssl_ctx, TLSEXT_cert_compression_zstd, compressZstd, + decompressZstd); + ASSERT(ret == 1); +} + +void CertCompression::registerZlib(SSL_CTX* ssl_ctx) { + auto ret = SSL_CTX_add_cert_compression_alg(ssl_ctx, TLSEXT_cert_compression_zlib, compressZlib, + decompressZlib); + ASSERT(ret == 1); +} + +void CertCompression::registerAlgorithms(SSL_CTX* ssl_ctx, const std::vector& algorithms, + Stats::Scope* scope) { + if (scope != nullptr) { + SSL_CTX_set_ex_data(ssl_ctx, getScopeExDataIndex(), scope); + } + for (const auto& algo : algorithms) { + switch (algo) { + case Algorithm::Brotli: + registerBrotli(ssl_ctx); + break; + case Algorithm::Zstd: + registerZstd(ssl_ctx); + break; + case Algorithm::Zlib: + registerZlib(ssl_ctx); + break; + } + } +} + +int CertCompression::compressBrotli(SSL* ssl, CBB* out, const uint8_t* in, size_t in_len) { + size_t encoded_size = BrotliEncoderMaxCompressedSize(in_len); + if (encoded_size == 0) { + IS_ENVOY_BUG("BrotliEncoderMaxCompressedSize returned 0"); + return FAILURE; + } + + uint8_t* out_buf = nullptr; + if (!CBB_reserve(out, &out_buf, encoded_size)) { + IS_ENVOY_BUG(fmt::format("Cert compression failure in allocating output CBB buffer of size {}", + encoded_size)); + return FAILURE; + } + + if (BrotliEncoderCompress(BROTLI_DEFAULT_QUALITY, BROTLI_DEFAULT_WINDOW, BROTLI_MODE_GENERIC, + in_len, in, &encoded_size, out_buf) != BROTLI_TRUE) { + IS_ENVOY_BUG("Cert compression failure in BrotliEncoderCompress"); + return FAILURE; + } + + if (!CBB_did_write(out, encoded_size)) { + IS_ENVOY_BUG("CBB_did_write failed"); + return FAILURE; + } + + recordCompressedCertSize(ssl, in_len, encoded_size, "brotli"); + ENVOY_LOG(trace, "Cert brotli compression successful"); + return SUCCESS; +} + +int CertCompression::decompressBrotli(SSL*, CRYPTO_BUFFER** out, size_t uncompressed_len, + const uint8_t* in, size_t in_len) { + uint8_t* out_buf = nullptr; + bssl::UniquePtr decompressed_data(CRYPTO_BUFFER_alloc(&out_buf, uncompressed_len)); + if (!decompressed_data) { + IS_ENVOY_BUG("Failed to allocate CRYPTO_BUFFER for brotli decompression"); + return FAILURE; + } + + size_t decoded_size = uncompressed_len; + BrotliDecoderResult result = BrotliDecoderDecompress(in_len, in, &decoded_size, out_buf); + + if (result != BROTLI_DECODER_RESULT_SUCCESS) { + ENVOY_LOG_PERIODIC(error, std::chrono::seconds(10), + "Cert brotli decompression failure, possibly caused by invalid " + "compressed cert from peer: result={}, decoded_size={}, uncompressed_len={}", + static_cast(result), decoded_size, uncompressed_len); + return FAILURE; + } + + if (decoded_size != uncompressed_len) { + ENVOY_LOG_PERIODIC(error, std::chrono::seconds(10), + "Brotli decompression length did not match peer provided uncompressed " + "length, caused by either invalid peer handshake data or decompression " + "error: decoded_size={}, uncompressed_len={}", + decoded_size, uncompressed_len); + return FAILURE; + } + + ENVOY_LOG(trace, "Cert brotli decompression successful"); + *out = decompressed_data.release(); + return SUCCESS; +} + +int CertCompression::compressZstd(SSL* ssl, CBB* out, const uint8_t* in, size_t in_len) { + size_t const max_size = ZSTD_compressBound(in_len); + if (max_size == 0) { + IS_ENVOY_BUG("ZSTD_compressBound returned 0"); + return FAILURE; + } + + uint8_t* out_buf = nullptr; + if (!CBB_reserve(out, &out_buf, max_size)) { + IS_ENVOY_BUG(fmt::format("Cert compression failure in allocating output CBB buffer of size {}", + max_size)); + return FAILURE; + } + + size_t const compressed_size = ZSTD_compress(out_buf, max_size, in, in_len, ZSTD_CLEVEL_DEFAULT); + if (ZSTD_isError(compressed_size)) { + IS_ENVOY_BUG( + fmt::format("Cert zstd compression failure: {}", ZSTD_getErrorName(compressed_size))); + return FAILURE; + } + + if (!CBB_did_write(out, compressed_size)) { + IS_ENVOY_BUG("CBB_did_write failed"); + return FAILURE; + } + + recordCompressedCertSize(ssl, in_len, compressed_size, "zstd"); + ENVOY_LOG(trace, "Cert zstd compression successful"); + return SUCCESS; +} + +int CertCompression::decompressZstd(SSL*, CRYPTO_BUFFER** out, size_t uncompressed_len, + const uint8_t* in, size_t in_len) { + uint8_t* out_buf = nullptr; + bssl::UniquePtr decompressed_data(CRYPTO_BUFFER_alloc(&out_buf, uncompressed_len)); + if (!decompressed_data) { + IS_ENVOY_BUG("Failed to allocate CRYPTO_BUFFER for zstd decompression"); + return FAILURE; + } + + size_t const decompressed_size = ZSTD_decompress(out_buf, uncompressed_len, in, in_len); + if (ZSTD_isError(decompressed_size)) { + ENVOY_LOG_PERIODIC(error, std::chrono::seconds(10), + "Cert zstd decompression failure, possibly caused by invalid " + "compressed cert from peer: {}, uncompressed_len={}", + ZSTD_getErrorName(decompressed_size), uncompressed_len); + return FAILURE; + } + + if (decompressed_size != uncompressed_len) { + ENVOY_LOG_PERIODIC(error, std::chrono::seconds(10), + "Zstd decompression length did not match peer provided uncompressed " + "length, caused by either invalid peer handshake data or decompression " + "error: decompressed_size={}, uncompressed_len={}", + decompressed_size, uncompressed_len); + return FAILURE; + } + + ENVOY_LOG(trace, "Cert zstd decompression successful"); + *out = decompressed_data.release(); + return SUCCESS; +} + +int CertCompression::compressZlib(SSL* ssl, CBB* out, const uint8_t* in, size_t in_len) { + z_stream z = {}; + // The deflateInit macro from zlib.h contains an old-style cast, so we need to suppress the + // warning for this call. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wold-style-cast" + int rv = deflateInit(&z, Z_DEFAULT_COMPRESSION); +#pragma GCC diagnostic pop + if (rv != Z_OK) { + IS_ENVOY_BUG(fmt::format("Cert compression failure in deflateInit: {}", rv)); + return FAILURE; + } + + ScopedZStream deleter(z, deflateEnd); + + const auto upper_bound = deflateBound(&z, in_len); + + uint8_t* out_buf = nullptr; + if (!CBB_reserve(out, &out_buf, upper_bound)) { + IS_ENVOY_BUG(fmt::format("Cert compression failure in allocating output CBB buffer of size {}", + upper_bound)); + return FAILURE; + } + + z.next_in = in; + z.avail_in = in_len; + z.next_out = out_buf; + z.avail_out = upper_bound; + + rv = deflate(&z, Z_FINISH); + if (rv != Z_STREAM_END) { + IS_ENVOY_BUG(fmt::format( + "Cert compression failure in deflate: {}, z.total_out {}, in_len {}, z.avail_in {}", rv, + z.avail_in, in_len, z.avail_in)); + return FAILURE; + } + + if (!CBB_did_write(out, z.total_out)) { + IS_ENVOY_BUG("CBB_did_write failed"); + return FAILURE; + } + + recordCompressedCertSize(ssl, in_len, z.total_out, "zlib"); + ENVOY_LOG(trace, "Cert zlib compression successful"); + return SUCCESS; +} + +int CertCompression::decompressZlib(SSL*, CRYPTO_BUFFER** out, size_t uncompressed_len, + const uint8_t* in, size_t in_len) { + z_stream z = {}; + // The inflateInit macro from zlib.h contains an old-style cast, so we need to suppress the + // warning for this call. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wold-style-cast" + int rv = inflateInit(&z); +#pragma GCC diagnostic pop + if (rv != Z_OK) { + IS_ENVOY_BUG(fmt::format("Cert decompression failure in inflateInit: {}", rv)); + return FAILURE; + } + + ScopedZStream deleter(z, inflateEnd); + + z.next_in = in; + z.avail_in = in_len; + bssl::UniquePtr decompressed_data( + CRYPTO_BUFFER_alloc(&z.next_out, uncompressed_len)); + if (!decompressed_data) { + IS_ENVOY_BUG("Failed to allocate CRYPTO_BUFFER for zlib decompression"); + return FAILURE; + } + z.avail_out = uncompressed_len; + + rv = inflate(&z, Z_FINISH); + if (rv != Z_STREAM_END) { + ENVOY_LOG_PERIODIC(error, std::chrono::seconds(10), + "Cert zlib decompression failure, possibly caused by invalid " + "compressed cert from peer: {}, z.total_out {}, uncompressed_len {}", + rv, z.total_out, uncompressed_len); + return FAILURE; + } + + if (z.total_out != uncompressed_len) { + ENVOY_LOG_PERIODIC(error, std::chrono::seconds(10), + "Zlib decompression length did not match peer provided uncompressed " + "length, caused by either invalid peer handshake data or decompression " + "error: z.total_out={}, uncompressed_len={}", + z.total_out, uncompressed_len); + return FAILURE; + } + + ENVOY_LOG(trace, "Cert zlib decompression successful"); + *out = decompressed_data.release(); + return SUCCESS; +} + +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/source/common/tls/cert_compression.h b/source/common/tls/cert_compression.h new file mode 100644 index 0000000000000..14a7033b5fe29 --- /dev/null +++ b/source/common/tls/cert_compression.h @@ -0,0 +1,59 @@ +#pragma once + +#include + +#include "envoy/stats/scope.h" + +#include "source/common/common/logger.h" + +#include "openssl/ssl.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { + +// RFC 8879 Certificate Compression Algorithm IDs. +// TLSEXT_cert_compression_zlib (1) and TLSEXT_cert_compression_brotli (2) are defined in BoringSSL. +// Define zstd (3) locally as BoringSSL does not define it. +#ifndef TLSEXT_cert_compression_zstd +#define TLSEXT_cert_compression_zstd 3 +#endif + +// RFC 8879 TLS Certificate Compression. +class CertCompression : protected Logger::Loggable { +public: + enum class Algorithm : uint16_t { + Zlib = 1, + Brotli = 2, + Zstd = 3, + }; + + // Registers algorithms in the order provided (first = highest priority). + static void registerAlgorithms(SSL_CTX* ssl_ctx, const std::vector& algorithms, + Stats::Scope* scope = nullptr); + + static void registerBrotli(SSL_CTX* ssl_ctx); + static void registerZstd(SSL_CTX* ssl_ctx); + static void registerZlib(SSL_CTX* ssl_ctx); + + static int compressBrotli(SSL* ssl, CBB* out, const uint8_t* in, size_t in_len); + static int decompressBrotli(SSL* ssl, CRYPTO_BUFFER** out, size_t uncompressed_len, + const uint8_t* in, size_t in_len); + + static int compressZstd(SSL* ssl, CBB* out, const uint8_t* in, size_t in_len); + static int decompressZstd(SSL* ssl, CRYPTO_BUFFER** out, size_t uncompressed_len, + const uint8_t* in, size_t in_len); + + static int compressZlib(SSL* ssl, CBB* out, const uint8_t* in, size_t in_len); + static int decompressZlib(SSL* ssl, CRYPTO_BUFFER** out, size_t uncompressed_len, + const uint8_t* in, size_t in_len); + + static constexpr int SUCCESS = 1; + static constexpr int FAILURE = 0; +}; + +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/source/common/tls/context_impl.cc b/source/common/tls/context_impl.cc index e32186ddb4c44..b6abcd0a5fef7 100644 --- a/source/common/tls/context_impl.cc +++ b/source/common/tls/context_impl.cc @@ -26,6 +26,7 @@ #include "source/common/protobuf/utility.h" #include "source/common/runtime/runtime_features.h" #include "source/common/stats/utility.h" +#include "source/common/tls/cert_compression.h" #include "source/common/tls/cert_validator/factory.h" #include "source/common/tls/stats.h" #include "source/common/tls/utility.h" @@ -162,6 +163,18 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c return; } } + + // Register certificate compression algorithms to reduce TLS handshake size (RFC 8879). + // Only register if the runtime feature flag is enabled (default: disabled). + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.tls_support_certificate_compression")) { + // Priority: brotli > zstd > zlib (brotli generally provides best compression for certs) + CertCompression::registerAlgorithms(ctx.ssl_ctx_.get(), + {CertCompression::Algorithm::Brotli, + CertCompression::Algorithm::Zstd, + CertCompression::Algorithm::Zlib}, + &scope); + } } auto verify_mode_or_error = cert_validator_->initializeSslContexts( diff --git a/source/common/tls/context_impl.h b/source/common/tls/context_impl.h index 64ace4fd02bf7..c15d1ced89a6c 100644 --- a/source/common/tls/context_impl.h +++ b/source/common/tls/context_impl.h @@ -21,6 +21,7 @@ #include "source/common/stats/symbol_table.h" #include "source/common/tls/cert_validator/cert_validator.h" #include "source/common/tls/context_manager_impl.h" +#include "source/common/tls/ssl_ctx_stats_provider.h" #include "source/common/tls/stats.h" #include "absl/synchronization/mutex.h" @@ -80,6 +81,7 @@ namespace TransportSockets { namespace Tls { class ContextImpl : public virtual Envoy::Ssl::Context, + public SslCtxStatsProvider, protected Logger::Loggable { public: virtual absl::StatusOr> @@ -92,7 +94,8 @@ class ContextImpl : public virtual Envoy::Ssl::Context, */ void logHandshake(SSL* ssl) const; - SslStats& stats() { return stats_; } + SslStats& stats() override { return stats_; } + Stats::Scope& statsScope() override { return scope_; } /** * The global SSL-library index used for storing a pointer to the SslExtendedSocketInfo diff --git a/source/common/tls/ssl_ctx_stats_provider.h b/source/common/tls/ssl_ctx_stats_provider.h new file mode 100644 index 0000000000000..132061e4cd1c6 --- /dev/null +++ b/source/common/tls/ssl_ctx_stats_provider.h @@ -0,0 +1,24 @@ +#pragma once + +#include "envoy/stats/scope.h" + +#include "source/common/tls/stats.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { + +// Interface for providing SSL stats from SSL_CTX app_data. +// This avoids circular dependency between cert_compression_lib and context_lib. +class SslCtxStatsProvider { +public: + virtual ~SslCtxStatsProvider() = default; + virtual SslStats& stats() = 0; + virtual Stats::Scope& statsScope() = 0; +}; + +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/source/common/tls/stats.cc b/source/common/tls/stats.cc index 3840ea17b99bb..3cb404c19c6ee 100644 --- a/source/common/tls/stats.cc +++ b/source/common/tls/stats.cc @@ -22,6 +22,10 @@ Stats::Gauge& createCertificateExpirationGauge(Stats::Scope& scope, const std::s Stats::Gauge::ImportMode::NeverImport); } +CertCompressionStats generateCertCompressionStats(Stats::Scope& scope, const std::string& prefix) { + return CertCompressionStats{CERT_COMPRESSION_STATS(POOL_COUNTER_PREFIX(scope, prefix))}; +} + } // namespace Tls } // namespace TransportSockets } // namespace Extensions diff --git a/source/common/tls/stats.h b/source/common/tls/stats.h index cb7112c86c844..c4a0dc23b0567 100644 --- a/source/common/tls/stats.h +++ b/source/common/tls/stats.h @@ -34,6 +34,20 @@ SslStats generateSslStats(Stats::Scope& store); Stats::Gauge& createCertificateExpirationGauge(Stats::Scope& scope, const std::string& cert_name); +/** + * Certificate compression stats per algorithm. @see stats_macros.h + */ +#define CERT_COMPRESSION_STATS(COUNTER) \ + COUNTER(compressed) \ + COUNTER(total_uncompressed_bytes) \ + COUNTER(total_compressed_bytes) + +struct CertCompressionStats { + CERT_COMPRESSION_STATS(GENERATE_COUNTER_STRUCT) +}; + +CertCompressionStats generateCertCompressionStats(Stats::Scope& scope, const std::string& prefix); + } // namespace Tls } // namespace TransportSockets } // namespace Extensions diff --git a/source/extensions/quic/proof_source/envoy_quic_proof_source_factory_impl.cc b/source/extensions/quic/proof_source/envoy_quic_proof_source_factory_impl.cc index b82d9b82a4d1b..ec508368d8afa 100644 --- a/source/extensions/quic/proof_source/envoy_quic_proof_source_factory_impl.cc +++ b/source/extensions/quic/proof_source/envoy_quic_proof_source_factory_impl.cc @@ -5,9 +5,9 @@ namespace Quic { std::unique_ptr EnvoyQuicProofSourceFactoryImpl::createQuicProofSource( Network::Socket& listen_socket, Network::FilterChainManager& filter_chain_manager, - Server::ListenerStats& listener_stats, TimeSource& time_source) { + Server::ListenerStats& listener_stats, TimeSource& time_source, Stats::Scope& stats_scope) { return std::make_unique(listen_socket, filter_chain_manager, listener_stats, - time_source); + time_source, stats_scope); } REGISTER_FACTORY(EnvoyQuicProofSourceFactoryImpl, EnvoyQuicProofSourceFactoryInterface); diff --git a/source/extensions/quic/proof_source/envoy_quic_proof_source_factory_impl.h b/source/extensions/quic/proof_source/envoy_quic_proof_source_factory_impl.h index 4e3b2a8663092..85438522f9d96 100644 --- a/source/extensions/quic/proof_source/envoy_quic_proof_source_factory_impl.h +++ b/source/extensions/quic/proof_source/envoy_quic_proof_source_factory_impl.h @@ -21,7 +21,8 @@ class EnvoyQuicProofSourceFactoryImpl : public EnvoyQuicProofSourceFactoryInterf std::unique_ptr createQuicProofSource(Network::Socket& listen_socket, Network::FilterChainManager& filter_chain_manager, - Server::ListenerStats& listener_stats, TimeSource& time_source) override; + Server::ListenerStats& listener_stats, TimeSource& time_source, + Stats::Scope& stats_scope) override; }; DECLARE_FACTORY(EnvoyQuicProofSourceFactoryImpl); diff --git a/test/common/quic/cert_compression_test.cc b/test/common/quic/cert_compression_test.cc index 767b13df1bf32..73ad5edc06c9e 100644 --- a/test/common/quic/cert_compression_test.cc +++ b/test/common/quic/cert_compression_test.cc @@ -1,4 +1,5 @@ #include "source/common/quic/cert_compression.h" +#include "source/common/tls/cert_compression.h" #include "test/test_common/logging.h" @@ -7,16 +8,17 @@ namespace Envoy { namespace Quic { +using TlsCertCompression = Extensions::TransportSockets::Tls::CertCompression; + TEST(CertCompressionZlibTest, DecompressBadData) { EXPECT_LOG_CONTAINS( "error", - "Cert decompression failure in inflate, possibly caused by invalid compressed cert from peer", - { + "Cert zlib decompression failure, possibly caused by invalid compressed cert from peer", { CRYPTO_BUFFER* out = nullptr; const uint8_t bad_compressed_data = 1; - EXPECT_EQ(CertCompression::FAILURE, - CertCompression::decompressZlib(nullptr, &out, 100, &bad_compressed_data, - sizeof(bad_compressed_data))); + EXPECT_EQ(TlsCertCompression::FAILURE, + TlsCertCompression::decompressZlib(nullptr, &out, 100, &bad_compressed_data, + sizeof(bad_compressed_data))); }); } @@ -25,18 +27,19 @@ TEST(CertCompressionZlibTest, DecompressBadLength) { constexpr size_t uncompressed_len = 6; bssl::ScopedCBB compressed; ASSERT_EQ(1, CBB_init(compressed.get(), 0)); - ASSERT_EQ(CertCompression::SUCCESS, - CertCompression::compressZlib(nullptr, compressed.get(), the_data, uncompressed_len)); + ASSERT_EQ( + TlsCertCompression::SUCCESS, + TlsCertCompression::compressZlib(nullptr, compressed.get(), the_data, uncompressed_len)); const auto compressed_len = CBB_len(compressed.get()); EXPECT_NE(0, compressed_len); EXPECT_LOG_CONTAINS("error", - "Decompression length did not match peer provided uncompressed length, " - "caused by either invalid peer handshake data or decompression error.", + "Zlib decompression length did not match peer provided uncompressed length, " + "caused by either invalid peer handshake data or decompression error", { CRYPTO_BUFFER* out = nullptr; - EXPECT_EQ(CertCompression::FAILURE, - CertCompression::decompressZlib( + EXPECT_EQ(TlsCertCompression::FAILURE, + TlsCertCompression::decompressZlib( nullptr, &out, uncompressed_len + 1 /* intentionally incorrect */, CBB_data(compressed.get()), compressed_len)); diff --git a/test/common/quic/envoy_quic_proof_source_test.cc b/test/common/quic/envoy_quic_proof_source_test.cc index af4b43647a3c7..51c8ca875b274 100644 --- a/test/common/quic/envoy_quic_proof_source_test.cc +++ b/test/common/quic/envoy_quic_proof_source_test.cc @@ -161,7 +161,8 @@ class EnvoyQuicProofSourceTest : public ::testing::Test { listener_stats_({ALL_LISTENER_STATS(POOL_COUNTER(listener_config_.listenerScope()), POOL_GAUGE(listener_config_.listenerScope()), POOL_HISTOGRAM(listener_config_.listenerScope()))}), - proof_source_(listen_socket_, filter_chain_manager_, listener_stats_, time_system_) { + proof_source_(listen_socket_, filter_chain_manager_, listener_stats_, time_system_, + listener_config_.listenerScope()) { EXPECT_CALL(*mock_context_config_, setSecretUpdateCallback(_)) .Times(testing::AtLeast(1u)) .WillRepeatedly(SaveArg<0>(&secret_update_callback_)); diff --git a/test/common/tls/BUILD b/test/common/tls/BUILD index bd5867e46ee10..ebe4d334fcedd 100644 --- a/test/common/tls/BUILD +++ b/test/common/tls/BUILD @@ -293,6 +293,16 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "cert_compression_test", + srcs = ["cert_compression_test.cc"], + external_deps = ["ssl"], + deps = [ + "//source/common/tls:cert_compression_lib", + "//test/test_common:logging_lib", + ], +) + envoy_cc_benchmark_binary( name = "tls_throughput_benchmark", srcs = ["tls_throughput_benchmark.cc"], diff --git a/test/common/tls/cert_compression_test.cc b/test/common/tls/cert_compression_test.cc new file mode 100644 index 0000000000000..d7231d2cfe432 --- /dev/null +++ b/test/common/tls/cert_compression_test.cc @@ -0,0 +1,260 @@ +#include "source/common/tls/cert_compression.h" + +#include "test/test_common/logging.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { + +// Test data for round-trip compression tests +constexpr uint8_t kTestData[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}; +constexpr size_t kTestDataLen = sizeof(kTestData); + +// +// Brotli Tests +// + +TEST(CertCompressionBrotliTest, RoundTrip) { + // Compress + bssl::ScopedCBB compressed; + ASSERT_EQ(1, CBB_init(compressed.get(), 0)); + EXPECT_EQ(CertCompression::SUCCESS, + CertCompression::compressBrotli(nullptr, compressed.get(), kTestData, kTestDataLen)); + const auto compressed_len = CBB_len(compressed.get()); + EXPECT_GT(compressed_len, 0u); + + // Decompress + CRYPTO_BUFFER* out = nullptr; + EXPECT_EQ(CertCompression::SUCCESS, + CertCompression::decompressBrotli(nullptr, &out, kTestDataLen, + CBB_data(compressed.get()), compressed_len)); + ASSERT_NE(nullptr, out); + bssl::UniquePtr out_ptr(out); + + // Verify + EXPECT_EQ(kTestDataLen, CRYPTO_BUFFER_len(out)); + EXPECT_EQ(0, memcmp(kTestData, CRYPTO_BUFFER_data(out), kTestDataLen)); +} + +TEST(CertCompressionBrotliTest, DecompressBadData) { + EXPECT_LOG_CONTAINS( + "error", + "Cert brotli decompression failure, possibly caused by invalid compressed cert from peer", { + CRYPTO_BUFFER* out = nullptr; + const uint8_t bad_compressed_data = 1; + EXPECT_EQ(CertCompression::FAILURE, + CertCompression::decompressBrotli(nullptr, &out, 100, &bad_compressed_data, + sizeof(bad_compressed_data))); + }); +} + +TEST(CertCompressionBrotliTest, DecompressBadLength) { + bssl::ScopedCBB compressed; + ASSERT_EQ(1, CBB_init(compressed.get(), 0)); + ASSERT_EQ(CertCompression::SUCCESS, + CertCompression::compressBrotli(nullptr, compressed.get(), kTestData, kTestDataLen)); + const auto compressed_len = CBB_len(compressed.get()); + EXPECT_GT(compressed_len, 0u); + + EXPECT_LOG_CONTAINS( + "error", "Brotli decompression length did not match peer provided uncompressed length", { + CRYPTO_BUFFER* out = nullptr; + EXPECT_EQ(CertCompression::FAILURE, + CertCompression::decompressBrotli(nullptr, &out, + kTestDataLen + 1 /* intentionally incorrect */, + CBB_data(compressed.get()), compressed_len)); + }); +} + +// +// Zstd Tests +// + +TEST(CertCompressionZstdTest, RoundTrip) { + // Compress + bssl::ScopedCBB compressed; + ASSERT_EQ(1, CBB_init(compressed.get(), 0)); + EXPECT_EQ(CertCompression::SUCCESS, + CertCompression::compressZstd(nullptr, compressed.get(), kTestData, kTestDataLen)); + const auto compressed_len = CBB_len(compressed.get()); + EXPECT_GT(compressed_len, 0u); + + // Decompress + CRYPTO_BUFFER* out = nullptr; + EXPECT_EQ(CertCompression::SUCCESS, + CertCompression::decompressZstd(nullptr, &out, kTestDataLen, CBB_data(compressed.get()), + compressed_len)); + ASSERT_NE(nullptr, out); + bssl::UniquePtr out_ptr(out); + + // Verify + EXPECT_EQ(kTestDataLen, CRYPTO_BUFFER_len(out)); + EXPECT_EQ(0, memcmp(kTestData, CRYPTO_BUFFER_data(out), kTestDataLen)); +} + +TEST(CertCompressionZstdTest, DecompressBadData) { + EXPECT_LOG_CONTAINS( + "error", + "Cert zstd decompression failure, possibly caused by invalid compressed cert from peer", { + CRYPTO_BUFFER* out = nullptr; + const uint8_t bad_compressed_data = 1; + EXPECT_EQ(CertCompression::FAILURE, + CertCompression::decompressZstd(nullptr, &out, 100, &bad_compressed_data, + sizeof(bad_compressed_data))); + }); +} + +TEST(CertCompressionZstdTest, DecompressBadLength) { + bssl::ScopedCBB compressed; + ASSERT_EQ(1, CBB_init(compressed.get(), 0)); + ASSERT_EQ(CertCompression::SUCCESS, + CertCompression::compressZstd(nullptr, compressed.get(), kTestData, kTestDataLen)); + const auto compressed_len = CBB_len(compressed.get()); + EXPECT_GT(compressed_len, 0u); + + EXPECT_LOG_CONTAINS( + "error", "Zstd decompression length did not match peer provided uncompressed length", { + CRYPTO_BUFFER* out = nullptr; + EXPECT_EQ(CertCompression::FAILURE, + CertCompression::decompressZstd(nullptr, &out, + kTestDataLen + 1 /* intentionally incorrect */, + CBB_data(compressed.get()), compressed_len)); + }); +} + +// +// Zlib Tests +// + +TEST(CertCompressionZlibTest, RoundTrip) { + // Compress + bssl::ScopedCBB compressed; + ASSERT_EQ(1, CBB_init(compressed.get(), 0)); + EXPECT_EQ(CertCompression::SUCCESS, + CertCompression::compressZlib(nullptr, compressed.get(), kTestData, kTestDataLen)); + const auto compressed_len = CBB_len(compressed.get()); + EXPECT_GT(compressed_len, 0u); + + // Decompress + CRYPTO_BUFFER* out = nullptr; + EXPECT_EQ(CertCompression::SUCCESS, + CertCompression::decompressZlib(nullptr, &out, kTestDataLen, CBB_data(compressed.get()), + compressed_len)); + ASSERT_NE(nullptr, out); + bssl::UniquePtr out_ptr(out); + + // Verify + EXPECT_EQ(kTestDataLen, CRYPTO_BUFFER_len(out)); + EXPECT_EQ(0, memcmp(kTestData, CRYPTO_BUFFER_data(out), kTestDataLen)); +} + +TEST(CertCompressionZlibTest, DecompressBadData) { + EXPECT_LOG_CONTAINS( + "error", + "Cert zlib decompression failure, possibly caused by invalid compressed cert from peer", { + CRYPTO_BUFFER* out = nullptr; + const uint8_t bad_compressed_data = 1; + EXPECT_EQ(CertCompression::FAILURE, + CertCompression::decompressZlib(nullptr, &out, 100, &bad_compressed_data, + sizeof(bad_compressed_data))); + }); +} + +TEST(CertCompressionZlibTest, DecompressBadLength) { + bssl::ScopedCBB compressed; + ASSERT_EQ(1, CBB_init(compressed.get(), 0)); + ASSERT_EQ(CertCompression::SUCCESS, + CertCompression::compressZlib(nullptr, compressed.get(), kTestData, kTestDataLen)); + const auto compressed_len = CBB_len(compressed.get()); + EXPECT_GT(compressed_len, 0u); + + EXPECT_LOG_CONTAINS("error", + "Zlib decompression length did not match peer provided uncompressed " + "length, caused by either invalid peer handshake data or decompression " + "error", + { + CRYPTO_BUFFER* out = nullptr; + EXPECT_EQ(CertCompression::FAILURE, + CertCompression::decompressZlib( + nullptr, &out, kTestDataLen + 1 /* intentionally incorrect */, + CBB_data(compressed.get()), compressed_len)); + }); +} + +// +// Stats Recording Tests +// These tests verify that compression succeeds even when SSL context is null +// (stats recording is safely skipped in this case) +// + +TEST(CertCompressionStatsTest, CompressBrotliWithNullSslRecordsNoStats) { + // Verify compression succeeds and doesn't crash when SSL is null + // (stats recording is safely skipped) + bssl::ScopedCBB compressed; + ASSERT_EQ(1, CBB_init(compressed.get(), 0)); + EXPECT_EQ(CertCompression::SUCCESS, + CertCompression::compressBrotli(nullptr, compressed.get(), kTestData, kTestDataLen)); + EXPECT_GT(CBB_len(compressed.get()), 0u); +} + +TEST(CertCompressionStatsTest, CompressZstdWithNullSslRecordsNoStats) { + bssl::ScopedCBB compressed; + ASSERT_EQ(1, CBB_init(compressed.get(), 0)); + EXPECT_EQ(CertCompression::SUCCESS, + CertCompression::compressZstd(nullptr, compressed.get(), kTestData, kTestDataLen)); + EXPECT_GT(CBB_len(compressed.get()), 0u); +} + +TEST(CertCompressionStatsTest, CompressZlibWithNullSslRecordsNoStats) { + bssl::ScopedCBB compressed; + ASSERT_EQ(1, CBB_init(compressed.get(), 0)); + EXPECT_EQ(CertCompression::SUCCESS, + CertCompression::compressZlib(nullptr, compressed.get(), kTestData, kTestDataLen)); + EXPECT_GT(CBB_len(compressed.get()), 0u); +} + +// +// Registration Tests +// These tests verify that the compression algorithms can be registered with SSL_CTX +// + +class CertCompressionRegistrationTest : public testing::Test { +protected: + void SetUp() override { + ssl_ctx_.reset(SSL_CTX_new(TLS_method())); + ASSERT_NE(nullptr, ssl_ctx_.get()); + } + + bssl::UniquePtr ssl_ctx_; +}; + +TEST_F(CertCompressionRegistrationTest, RegisterBrotli) { + // Verify brotli registration succeeds without crashing + EXPECT_NO_THROW(CertCompression::registerBrotli(ssl_ctx_.get())); +} + +TEST_F(CertCompressionRegistrationTest, RegisterZstd) { + // Verify zstd registration succeeds without crashing + EXPECT_NO_THROW(CertCompression::registerZstd(ssl_ctx_.get())); +} + +TEST_F(CertCompressionRegistrationTest, RegisterZlib) { + // Verify zlib registration succeeds without crashing + EXPECT_NO_THROW(CertCompression::registerZlib(ssl_ctx_.get())); +} + +TEST_F(CertCompressionRegistrationTest, RegisterAllAlgorithms) { + // Verify all algorithms can be registered on the same context + // Order matters: brotli > zstd > zlib (by priority) + EXPECT_NO_THROW(CertCompression::registerBrotli(ssl_ctx_.get())); + EXPECT_NO_THROW(CertCompression::registerZstd(ssl_ctx_.get())); + EXPECT_NO_THROW(CertCompression::registerZlib(ssl_ctx_.get())); +} + +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/test/common/tls/ssl_socket_test.cc b/test/common/tls/ssl_socket_test.cc index f9a69cc9fdd42..76b2d05fd7b04 100644 --- a/test/common/tls/ssl_socket_test.cc +++ b/test/common/tls/ssl_socket_test.cc @@ -7914,6 +7914,59 @@ TEST_P(SslSocketTest, RsaKeyUsageVerificationEnforcementOn) { testUtilV2(test_options); } +// Test that TLS handshakes succeed when certificate compression is enabled via runtime flag. +// This verifies the certificate compression feature (RFC 8879) integration with brotli, zstd, +// and zlib algorithms when the runtime flag is enabled. +TEST_P(SslSocketTest, CertificateCompressionEnabled) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.tls_support_certificate_compression", "true"}}); + + envoy::config::listener::v3::Listener listener; + envoy::config::listener::v3::FilterChain* filter_chain = listener.add_filter_chains(); + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext server_tls_context; + envoy::extensions::transport_sockets::tls::v3::TlsCertificate* server_cert = + server_tls_context.mutable_common_tls_context()->add_tls_certificates(); + server_cert->mutable_certificate_chain()->set_filename( + TestEnvironment::substitute("{{ test_rundir }}/test/common/tls/test_data/san_dns_cert.pem")); + server_cert->mutable_private_key()->set_filename( + TestEnvironment::substitute("{{ test_rundir }}/test/common/tls/test_data/san_dns_key.pem")); + + updateFilterChain(server_tls_context, *filter_chain); + + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext client_tls_context; + + // TLS handshake should succeed with compression algorithms registered. + TestUtilOptionsV2 test_options(listener, client_tls_context, /*expect_success=*/true, version_); + testUtilV2(test_options); +} + +// Test that TLS handshakes succeed with certificate compression disabled (default behavior). +// This verifies backward compatibility when the runtime flag is disabled. +TEST_P(SslSocketTest, CertificateCompressionDisabled) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.tls_support_certificate_compression", "false"}}); + + envoy::config::listener::v3::Listener listener; + envoy::config::listener::v3::FilterChain* filter_chain = listener.add_filter_chains(); + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext server_tls_context; + envoy::extensions::transport_sockets::tls::v3::TlsCertificate* server_cert = + server_tls_context.mutable_common_tls_context()->add_tls_certificates(); + server_cert->mutable_certificate_chain()->set_filename( + TestEnvironment::substitute("{{ test_rundir }}/test/common/tls/test_data/san_dns_cert.pem")); + server_cert->mutable_private_key()->set_filename( + TestEnvironment::substitute("{{ test_rundir }}/test/common/tls/test_data/san_dns_key.pem")); + + updateFilterChain(server_tls_context, *filter_chain); + + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext client_tls_context; + + // TLS handshake should succeed without compression algorithms (backward compatibility). + TestUtilOptionsV2 test_options(listener, client_tls_context, /*expect_success=*/true, version_); + testUtilV2(test_options); +} + } // namespace Tls } // namespace TransportSockets } // namespace Extensions diff --git a/test/extensions/quic/proof_source/pending_proof_source_factory_impl.cc b/test/extensions/quic/proof_source/pending_proof_source_factory_impl.cc index 9dd472518eb34..7887f9c7aa636 100644 --- a/test/extensions/quic/proof_source/pending_proof_source_factory_impl.cc +++ b/test/extensions/quic/proof_source/pending_proof_source_factory_impl.cc @@ -9,8 +9,10 @@ class PendingProofSource : public EnvoyQuicProofSource { public: PendingProofSource(Network::Socket& listen_socket, Network::FilterChainManager& filter_chain_manager, - Server::ListenerStats& listener_stats, TimeSource& time_source) - : EnvoyQuicProofSource(listen_socket, filter_chain_manager, listener_stats, time_source) {} + Server::ListenerStats& listener_stats, TimeSource& time_source, + Stats::Scope& stats_scope) + : EnvoyQuicProofSource(listen_socket, filter_chain_manager, listener_stats, time_source, + stats_scope) {} protected: void signPayload(const quic::QuicSocketAddress& /*server_address*/, @@ -28,9 +30,9 @@ class PendingProofSource : public EnvoyQuicProofSource { std::unique_ptr PendingProofSourceFactoryImpl::createQuicProofSource( Network::Socket& listen_socket, Network::FilterChainManager& filter_chain_manager, - Server::ListenerStats& listener_stats, TimeSource& time_source) { + Server::ListenerStats& listener_stats, TimeSource& time_source, Stats::Scope& stats_scope) { return std::make_unique(listen_socket, filter_chain_manager, listener_stats, - time_source); + time_source, stats_scope); } REGISTER_FACTORY(PendingProofSourceFactoryImpl, EnvoyQuicProofSourceFactoryInterface); diff --git a/test/extensions/quic/proof_source/pending_proof_source_factory_impl.h b/test/extensions/quic/proof_source/pending_proof_source_factory_impl.h index 0643b371c51fe..abd86c03dc8b7 100644 --- a/test/extensions/quic/proof_source/pending_proof_source_factory_impl.h +++ b/test/extensions/quic/proof_source/pending_proof_source_factory_impl.h @@ -21,7 +21,8 @@ class PendingProofSourceFactoryImpl : public EnvoyQuicProofSourceFactoryInterfac std::unique_ptr createQuicProofSource(Network::Socket& listen_socket, Network::FilterChainManager& filter_chain_manager, - Server::ListenerStats& listener_stats, TimeSource& time_source) override; + Server::ListenerStats& listener_stats, TimeSource& time_source, + Stats::Scope& stats_scope) override; }; DECLARE_FACTORY(PendingProofSourceFactoryImpl); diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index 3882cd9665c07..9b325748001ad 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -94,11 +94,13 @@ TEST_P(QuicHttpIntegrationTest, RuntimeEnableDraft29) { } TEST_P(QuicHttpIntegrationTest, CertCompressionEnabled) { + config_helper_.addRuntimeOverride("envoy.reloadable_features.tls_support_certificate_compression", + "true"); initialize(); EXPECT_LOG_CONTAINS_ALL_OF( - Envoy::ExpectedLogMessages( - {{"trace", "Cert compression successful"}, {"trace", "Cert decompression successful"}}), + Envoy::ExpectedLogMessages({{"trace", "Cert brotli compression successful"}, + {"trace", "Cert brotli decompression successful"}}), { testRouterHeaderOnlyRequestAndResponse(); }); } diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index b279330f19d17..df26bb2b767a2 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -393,6 +393,7 @@ Prereq QDCOUNT QPACK QUIC +QUICHE QoS qat qatzip @@ -480,6 +481,7 @@ TE TFO TID TLS +TLSEXT TLSv TLV TMPDIR From 0711a41cf71b72490fb98606ab31c231f3bff398 Mon Sep 17 00:00:00 2001 From: Gavin Jeong Date: Fri, 16 Jan 2026 19:51:41 +0900 Subject: [PATCH 12/12] fix: Dereference shared_ptr for locality in RedisHost constructor The HostImpl constructor expects a const reference, not a shared_ptr. --- source/extensions/clusters/redis/redis_cluster.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/clusters/redis/redis_cluster.h b/source/extensions/clusters/redis/redis_cluster.h index 3e636a366773c..d4786dc88b7ff 100644 --- a/source/extensions/clusters/redis/redis_cluster.h +++ b/source/extensions/clusters/redis/redis_cluster.h @@ -203,7 +203,7 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { std::make_shared( parent.localityLbEndpoint().metadata()), parent.lbEndpoint().load_balancing_weight().value(), - makeLocalityWithZone(parent.localityLbEndpoint().locality(), zone), + *makeLocalityWithZone(parent.localityLbEndpoint().locality(), zone), parent.lbEndpoint().endpoint().health_check_config(), parent.localityLbEndpoint().priority(), parent.lbEndpoint().health_status()), primary_(primary) {}