diff --git a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc index f35715c37ad98..c5bc8c1f2c499 100644 --- a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc @@ -23,28 +23,32 @@ HttpGrpcAccessLog::ThreadLocalLogger::ThreadLocalLogger( GrpcCommon::GrpcAccessLoggerSharedPtr logger) : logger_(std::move(logger)) {} -HttpGrpcAccessLog::HttpGrpcAccessLog( - AccessLog::FilterPtr&& filter, - envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig config, - ThreadLocal::SlotAllocator& tls, GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache, - Stats::Scope& scope) - : Common::ImplBase(std::move(filter)), scope_(scope), config_(std::move(config)), +HttpGrpcAccessLog::HttpGrpcAccessLog(AccessLog::FilterPtr&& filter, + const HttpGrpcAccessLogConfig config, + ThreadLocal::SlotAllocator& tls, + GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache, + Stats::Scope& scope) + : Common::ImplBase(std::move(filter)), scope_(scope), + config_(std::make_shared(std::move(config))), tls_slot_(tls.allocateSlot()), access_logger_cache_(std::move(access_logger_cache)) { - for (const auto& header : config_.additional_request_headers_to_log()) { + for (const auto& header : config_->additional_request_headers_to_log()) { request_headers_to_log_.emplace_back(header); } - for (const auto& header : config_.additional_response_headers_to_log()) { + for (const auto& header : config_->additional_response_headers_to_log()) { response_headers_to_log_.emplace_back(header); } - for (const auto& header : config_.additional_response_trailers_to_log()) { + for (const auto& header : config_->additional_response_trailers_to_log()) { response_trailers_to_log_.emplace_back(header); } - Envoy::Config::Utility::checkTransportVersion(config_.common_config()); - tls_slot_->set([this](Event::Dispatcher&) { - return std::make_shared(access_logger_cache_->getOrCreateLogger( - config_.common_config(), Common::GrpcAccessLoggerType::HTTP, scope_)); + Envoy::Config::Utility::checkTransportVersion(config_->common_config()); + // Note that &scope might have died by the time when this callback is called on each thread. + // This is supposed to be fixed by https://github.com/envoyproxy/envoy/issues/18066. + tls_slot_->set([config = config_, access_logger_cache = access_logger_cache_, + &scope = scope_](Event::Dispatcher&) { + return std::make_shared(access_logger_cache->getOrCreateLogger( + config->common_config(), Common::GrpcAccessLoggerType::HTTP, scope)); }); } @@ -56,7 +60,7 @@ void HttpGrpcAccessLog::emitLog(const Http::RequestHeaderMap& request_headers, // TODO(mattklein123): Populate sample_rate field. envoy::data::accesslog::v3::HTTPAccessLogEntry log_entry; GrpcCommon::Utility::extractCommonAccessLogProperties(*log_entry.mutable_common_properties(), - stream_info, config_.common_config()); + stream_info, config_->common_config()); if (stream_info.protocol()) { switch (stream_info.protocol().value()) { diff --git a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.h b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.h index d403596f8decc..25da566a10355 100644 --- a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.h +++ b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.h @@ -21,13 +21,15 @@ namespace HttpGrpc { // TODO(mattklein123): Stats +using envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig; +using HttpGrpcAccessLogConfigConstSharedPtr = std::shared_ptr; + /** * Access log Instance that streams HTTP logs over gRPC. */ class HttpGrpcAccessLog : public Common::ImplBase { public: - HttpGrpcAccessLog(AccessLog::FilterPtr&& filter, - envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig config, + HttpGrpcAccessLog(AccessLog::FilterPtr&& filter, const HttpGrpcAccessLogConfig config, ThreadLocal::SlotAllocator& tls, GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache, Stats::Scope& scope); @@ -49,7 +51,7 @@ class HttpGrpcAccessLog : public Common::ImplBase { const StreamInfo::StreamInfo& stream_info) override; Stats::Scope& scope_; - const envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig config_; + const HttpGrpcAccessLogConfigConstSharedPtr config_; const ThreadLocal::SlotPtr tls_slot_; const GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache_; std::vector request_headers_to_log_; diff --git a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc index 7fbcee911d5bc..63eb08d4b277f 100644 --- a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc @@ -17,17 +17,21 @@ namespace TcpGrpc { TcpGrpcAccessLog::ThreadLocalLogger::ThreadLocalLogger(GrpcCommon::GrpcAccessLoggerSharedPtr logger) : logger_(std::move(logger)) {} -TcpGrpcAccessLog::TcpGrpcAccessLog( - AccessLog::FilterPtr&& filter, - envoy::extensions::access_loggers::grpc::v3::TcpGrpcAccessLogConfig config, - ThreadLocal::SlotAllocator& tls, GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache, - Stats::Scope& scope) - : Common::ImplBase(std::move(filter)), scope_(scope), config_(std::move(config)), +TcpGrpcAccessLog::TcpGrpcAccessLog(AccessLog::FilterPtr&& filter, + const TcpGrpcAccessLogConfig config, + ThreadLocal::SlotAllocator& tls, + GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache, + Stats::Scope& scope) + : Common::ImplBase(std::move(filter)), scope_(scope), + config_(std::make_shared(std::move(config))), tls_slot_(tls.allocateSlot()), access_logger_cache_(std::move(access_logger_cache)) { - Config::Utility::checkTransportVersion(config_.common_config()); - tls_slot_->set([this](Event::Dispatcher&) { - return std::make_shared(access_logger_cache_->getOrCreateLogger( - config_.common_config(), Common::GrpcAccessLoggerType::TCP, scope_)); + Config::Utility::checkTransportVersion(config_->common_config()); + // Note that &scope might have died by the time when this callback is called on each thread. + // This is supposed to be fixed by https://github.com/envoyproxy/envoy/issues/18066. + tls_slot_->set([config = config_, access_logger_cache = access_logger_cache_, + &scope = scope_](Event::Dispatcher&) { + return std::make_shared(access_logger_cache->getOrCreateLogger( + config->common_config(), Common::GrpcAccessLoggerType::TCP, scope)); }); } @@ -37,7 +41,7 @@ void TcpGrpcAccessLog::emitLog(const Http::RequestHeaderMap&, const Http::Respon // Common log properties. envoy::data::accesslog::v3::TCPAccessLogEntry log_entry; GrpcCommon::Utility::extractCommonAccessLogProperties(*log_entry.mutable_common_properties(), - stream_info, config_.common_config()); + stream_info, config_->common_config()); envoy::data::accesslog::v3::ConnectionProperties& connection_properties = *log_entry.mutable_connection_properties(); diff --git a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h index fba13f16d6c6d..a0b3842a49df0 100644 --- a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h +++ b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h @@ -20,13 +20,15 @@ namespace TcpGrpc { // TODO(mattklein123): Stats +using envoy::extensions::access_loggers::grpc::v3::TcpGrpcAccessLogConfig; +using TcpGrpcAccessLogConfigConstSharedPtr = std::shared_ptr; + /** * Access log Instance that streams TCP logs over gRPC. */ class TcpGrpcAccessLog : public Common::ImplBase { public: - TcpGrpcAccessLog(AccessLog::FilterPtr&& filter, - envoy::extensions::access_loggers::grpc::v3::TcpGrpcAccessLogConfig config, + TcpGrpcAccessLog(AccessLog::FilterPtr&& filter, const TcpGrpcAccessLogConfig config, ThreadLocal::SlotAllocator& tls, GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache, Stats::Scope& scope); @@ -48,7 +50,7 @@ class TcpGrpcAccessLog : public Common::ImplBase { const StreamInfo::StreamInfo& stream_info) override; Stats::Scope& scope_; - const envoy::extensions::access_loggers::grpc::v3::TcpGrpcAccessLogConfig config_; + const TcpGrpcAccessLogConfigConstSharedPtr config_; const ThreadLocal::SlotPtr tls_slot_; const GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache_; }; diff --git a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc index 8c78f302f5612..61946a9ed4b6c 100644 --- a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc +++ b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc @@ -48,6 +48,36 @@ class MockGrpcAccessLoggerCache : public GrpcCommon::GrpcAccessLoggerCache { Common::GrpcAccessLoggerType logger_type, Stats::Scope& scope)); }; +// Test for the issue described in https://github.com/envoyproxy/envoy/pull/18081 +TEST(HttpGrpcAccessLog, TlsLifetimeCheck) { + NiceMock tls; + Stats::IsolatedStoreImpl scope; + std::shared_ptr logger_cache{new MockGrpcAccessLoggerCache()}; + tls.defer_data_ = true; + { + AccessLog::MockFilter* filter{new NiceMock()}; + envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig config; + config.mutable_common_config()->set_transport_api_version( + envoy::config::core::v3::ApiVersion::V3); + EXPECT_CALL(*logger_cache, getOrCreateLogger(_, _, _)) + .WillOnce([](const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& + common_config, + Common::GrpcAccessLoggerType type, Stats::Scope&) { + // This is a part of the actual getOrCreateLogger code path and shouldn't crash. + std::make_pair(MessageUtil::hash(common_config), type); + return nullptr; + }); + // Set tls callback in the HttpGrpcAccessLog constructor, + // but it is not called yet since we have defer_data_ = true. + const auto access_log = std::make_unique(AccessLog::FilterPtr{filter}, + config, tls, logger_cache, scope); + // Intentionally make access_log die earlier in this scope to simulate the situation where the + // creator has been deleted yet the tls callback is not called yet. + } + // Verify the tls callback does not crash since it captures the env with proper lifetime. + tls.call(); +} + class HttpGrpcAccessLogTest : public testing::Test { public: void init() { diff --git a/test/extensions/access_loggers/grpc/tcp_config_test.cc b/test/extensions/access_loggers/grpc/tcp_config_test.cc index 9889c337de476..7a2c5f50b2004 100644 --- a/test/extensions/access_loggers/grpc/tcp_config_test.cc +++ b/test/extensions/access_loggers/grpc/tcp_config_test.cc @@ -76,6 +76,44 @@ TEST_F(TcpGrpcAccessLogConfigTest, Ok) { run("good_cluster"); } // Wrong configuration with invalid clusters. TEST_F(TcpGrpcAccessLogConfigTest, InvalidCluster) { run("invalid"); } +class MockGrpcAccessLoggerCache : public GrpcCommon::GrpcAccessLoggerCache { +public: + // GrpcAccessLoggerCache + MOCK_METHOD(GrpcCommon::GrpcAccessLoggerSharedPtr, getOrCreateLogger, + (const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config, + Common::GrpcAccessLoggerType logger_type, Stats::Scope& scope)); +}; + +// Test for the issue described in https://github.com/envoyproxy/envoy/pull/18081 +TEST(TcpGrpcAccessLog, TlsLifetimeCheck) { + NiceMock tls; + Stats::IsolatedStoreImpl scope; + std::shared_ptr logger_cache{new MockGrpcAccessLoggerCache()}; + tls.defer_data_ = true; + { + AccessLog::MockFilter* filter{new NiceMock()}; + envoy::extensions::access_loggers::grpc::v3::TcpGrpcAccessLogConfig config; + config.mutable_common_config()->set_transport_api_version( + envoy::config::core::v3::ApiVersion::V3); + EXPECT_CALL(*logger_cache, getOrCreateLogger(_, _, _)) + .WillOnce([](const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& + common_config, + Common::GrpcAccessLoggerType type, Stats::Scope&) { + // This is a part of the actual getOrCreateLogger code path and shouldn't crash. + std::make_pair(MessageUtil::hash(common_config), type); + return nullptr; + }); + // Set tls callback in the TcpGrpcAccessLog constructor, + // but it is not called yet since we have defer_data_ = true. + const auto access_log = std::make_unique(AccessLog::FilterPtr{filter}, config, + tls, logger_cache, scope); + // Intentionally make access_log die earlier in this scope to simulate the situation where the + // creator has been deleted yet the tls callback is not called yet. + } + // Verify the tls callback does not crash since it captures the env with proper lifetime. + tls.call(); +} + } // namespace } // namespace TcpGrpc } // namespace AccessLoggers