diff --git a/VERSION.txt b/VERSION.txt index 99742f6f2431d..6fef6c580852c 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -1 +1 @@ -1.34.6-dev +1.34.6 diff --git a/changelogs/1.32.11.yaml b/changelogs/1.32.11.yaml new file mode 100644 index 0000000000000..935f1c45a820e --- /dev/null +++ b/changelogs/1.32.11.yaml @@ -0,0 +1,7 @@ +date: September 2, 2025 + +bug_fixes: +- area: oauth2 + change: | + Fixed an issue where cookies prefixed with ``__Secure-`` or ``__Host-`` were not receiving a + Secure attribute (`CVE-2025-55162 `_). diff --git a/changelogs/1.33.8.yaml b/changelogs/1.33.8.yaml new file mode 100644 index 0000000000000..ec53a9b62b5cd --- /dev/null +++ b/changelogs/1.33.8.yaml @@ -0,0 +1,7 @@ +date: September 2, 2025 + +bug_fixes: +- area: oauth2 + change: | + Fixed an issue where cookies prefixed with ``__Secure-`` or ``__Host-`` were not receiving a + Secure attribute. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index a7bce22d88548..58aba7edbc15c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -1,13 +1,6 @@ -date: Pending - -behavior_changes: -# *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* - -minor_behavior_changes: -# *Changes that may cause incompatibilities for some users, but should not for most* +date: September 2, 2025 bug_fixes: -# *Changes expected to improve the state of the world and are unlikely to have negative effects* - area: oauth2 change: | Fixed an issue where cookies prefixed with ``__Secure-`` or ``__Host-`` were not receiving a @@ -16,10 +9,3 @@ bug_fixes: change: | Fixed an UAF in DNS cache that can occur when the Host header is modified between the Dynamic Forwarding and Router filters. - -removed_config_or_runtime: -# *Normally occurs at the end of the* :ref:`deprecation period ` - -new_features: - -deprecated: diff --git a/docs/inventories/v1.32/objects.inv b/docs/inventories/v1.32/objects.inv index 767ecc6c090aa..1f2821c4608ff 100644 Binary files a/docs/inventories/v1.32/objects.inv and b/docs/inventories/v1.32/objects.inv differ diff --git a/docs/inventories/v1.33/objects.inv b/docs/inventories/v1.33/objects.inv index da58252da22e3..216ece34b1cc9 100644 Binary files a/docs/inventories/v1.33/objects.inv and b/docs/inventories/v1.33/objects.inv differ diff --git a/docs/inventories/v1.34/objects.inv b/docs/inventories/v1.34/objects.inv index 48ccb98994b12..e754781f866ce 100644 Binary files a/docs/inventories/v1.34/objects.inv and b/docs/inventories/v1.34/objects.inv differ diff --git a/docs/versions.yaml b/docs/versions.yaml index 791b39a2bb9c6..bce16b48ea561 100644 --- a/docs/versions.yaml +++ b/docs/versions.yaml @@ -25,6 +25,6 @@ "1.29": 1.29.12 "1.30": 1.30.11 "1.31": 1.31.10 -"1.32": 1.32.10 -"1.33": 1.33.7 -"1.34": 1.34.4 +"1.32": 1.32.11 +"1.33": 1.33.8 +"1.34": 1.34.5 diff --git a/source/common/quic/BUILD b/source/common/quic/BUILD index c44d4a489aa80..8c1a4b54eae07 100644 --- a/source/common/quic/BUILD +++ b/source/common/quic/BUILD @@ -512,6 +512,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/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 7081b14a88bb6..1f440f274ae1d 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -74,13 +74,10 @@ RedisCluster::RedisCluster( cluster_name_(cluster.name()), refresh_manager_(Common::Redis::getClusterRefreshManager( context.serverFactoryContext().singletonManager(), - context.serverFactoryContext().mainThreadDispatcher(), context.clusterManager(), + context.serverFactoryContext().mainThreadDispatcher(), + context.serverFactoryContext().clusterManager(), context.serverFactoryContext().api().timeSource())), - registration_handle_(refresh_manager_->registerCluster( - cluster_name_, redirect_refresh_interval_, redirect_refresh_threshold_, - failure_refresh_threshold_, host_degraded_refresh_threshold_, [&]() { - redis_discovery_session_->resolve_timer_->enableTimer(std::chrono::milliseconds(0)); - })) { + registration_handle_(nullptr) { 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()) { @@ -89,6 +86,55 @@ RedisCluster::RedisCluster( *this, host.socket_address().address(), host.socket_address().port_value())); } } + + // 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, 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 + // 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. + // This matches the approach in PR #39625. + redis_discovery_session_.reset(); + + // 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() { @@ -201,7 +247,7 @@ RedisCluster::DnsDiscoveryResolveTarget::~DnsDiscoveryResolveTarget() { active_query_->cancel(Network::ActiveDnsQuery::CancelReason::QueryAbandoned); } // Disable timer for mock tests. - if (resolve_timer_) { + if (resolve_timer_ && resolve_timer_->enabled()) { resolve_timer_->disableTimer(); } } @@ -209,6 +255,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, @@ -216,21 +266,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 { startResolveDns(); }); + 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 @@ -247,12 +310,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 { 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 @@ -280,6 +354,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_)); @@ -300,11 +378,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; } @@ -327,21 +411,31 @@ 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; + } client->client_ = client_factory_.create(host, dispatcher_, shared_from_this(), - redis_command_stats_, parent_.info()->statsScope(), + redis_command_stats_, parent_info->statsScope(), parent_.auth_username_, parent_.auth_password_, false); 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(); } } @@ -362,6 +456,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) { @@ -373,6 +471,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, @@ -420,6 +522,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) { @@ -436,6 +542,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); @@ -464,13 +575,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; @@ -566,7 +688,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_); + } } } @@ -596,20 +720,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 50ada2a61abde..8530fc7c06b15 100644 --- a/source/extensions/clusters/redis/redis_cluster.h +++ b/source/extensions/clusters/redis/redis_cluster.h @@ -90,6 +90,7 @@ namespace Redis { class RedisCluster : public Upstream::BaseDynamicClusterImpl { public: + ~RedisCluster(); static absl::StatusOr> create(const envoy::config::cluster::v3::Cluster& cluster, const envoy::extensions::clusters::redis::v3::RedisClusterConfig& redis_cluster, @@ -222,6 +223,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 @@ -267,6 +271,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_; @@ -279,6 +305,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_; @@ -304,7 +335,10 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { const std::string auth_password_; const std::string cluster_name_; const Common::Redis::ClusterRefreshManagerSharedPtr refresh_manager_; - const Common::Redis::ClusterRefreshManager::HandlePtr registration_handle_; + Common::Redis::ClusterRefreshManager::HandlePtr registration_handle_; + + // Flag to prevent callbacks during destruction + std::atomic is_destroying_{false}; }; class RedisClusterFactory : public Upstream::ConfigurableClusterFactoryBase< diff --git a/source/extensions/filters/network/common/redis/supported_commands.h b/source/extensions/filters/network/common/redis/supported_commands.h index cdbb850247248..521b5913b30dc 100644 --- a/source/extensions/filters/network/common/redis/supported_commands.h +++ b/source/extensions/filters/network/common/redis/supported_commands.h @@ -49,7 +49,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/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 c18c23569dddc..186e913b12ab4 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/common/quic/envoy_quic_proof_source_test.cc b/test/common/quic/envoy_quic_proof_source_test.cc index 2abe458fafad2..3d9d9be185a49 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" @@ -344,5 +349,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 diff --git a/test/extensions/clusters/redis/BUILD b/test/extensions/clusters/redis/BUILD index 6f0a607172659..32dfc5229938f 100644 --- a/test/extensions/clusters/redis/BUILD +++ b/test/extensions/clusters/redis/BUILD @@ -31,6 +31,7 @@ envoy_extension_cc_test( "//source/server:transport_socket_config_lib", "//test/common/upstream:utility_lib", "//test/extensions/clusters/redis:redis_cluster_mocks", + "//test/extensions/common/redis:mocks_lib", "//test/extensions/filters/network/common/redis:redis_mocks", "//test/extensions/filters/network/common/redis:test_utils_lib", "//test/extensions/filters/network/redis_proxy:redis_mocks", diff --git a/test/extensions/clusters/redis/redis_cluster_lb_test.cc b/test/extensions/clusters/redis/redis_cluster_lb_test.cc index 4a970ae1e359f..0590c24c46c62 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/clusters/redis/redis_cluster_test.cc b/test/extensions/clusters/redis/redis_cluster_test.cc index 98fb8b371fceb..f4dd0686be8bc 100644 --- a/test/extensions/clusters/redis/redis_cluster_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_test.cc @@ -18,6 +18,7 @@ #include "test/common/upstream/utility.h" #include "test/extensions/clusters/redis/mocks.h" +#include "test/extensions/common/redis/mocks.h" #include "test/extensions/filters/network/common/redis/mocks.h" #include "test/mocks/common.h" #include "test/mocks/protobuf/mocks.h" @@ -1486,6 +1487,44 @@ TEST_F(RedisClusterTest, HostRemovalAfterHcFail) { */ } +// Test that verifies cluster destruction does not cause segfault when refresh manager +// triggers callback after cluster is destroyed. This reproduces the issue from #38585. +TEST_F(RedisClusterTest, NoSegfaultOnClusterDestructionWithPendingCallback) { + // This test verifies that destroying the cluster properly cleans up resources + // and doesn't cause a segfault. The key protection is in the destructor that + // sets is_destroying_ flag and cleans up the redis_discovery_session_. + + // Create the cluster with basic configuration + setupFromV3Yaml(BasicConfig); + const std::list resolved_addresses{"127.0.0.1"}; + expectResolveDiscovery(Network::DnsLookupFamily::V4Only, "foo.bar.com", resolved_addresses); + expectRedisResolve(true); + + cluster_->initialize([&]() { + initialized_.ready(); + return absl::OkStatus(); + }); + + EXPECT_CALL(membership_updated_, ready()); + EXPECT_CALL(initialized_, ready()); + EXPECT_CALL(*cluster_callback_, onClusterSlotUpdate(_, _)); + std::bitset single_slot_primary(0xfff); + std::bitset no_replica(0); + expectClusterSlotResponse(createResponse(single_slot_primary, no_replica)); + expectHealthyHosts(std::list({"127.0.0.1:22120"})); + + // Now destroy the cluster. With the fix in place (destructor setting is_destroying_ + // and resetting redis_discovery_session_), this should not crash. + // Without the fix, accessing resolve_timer_ after destruction would segfault. + cluster_.reset(); + + // If we reach here without crashing, the test passes. + // The fix ensures that: + // 1. The destructor sets is_destroying_ = true + // 2. The destructor resets redis_discovery_session_ + // 3. Timer callbacks check is_destroying_ before accessing cluster members +} + } // 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 a0f4d1ef65017..e7df19c022fe6 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; 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 051a21b6846f5..a187d52d140f6 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 b87f984768ebe..ae13d846c7f8c 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();