From 9d8c32dbee64f74b68c83a1442071ebacbd7c52c Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Mon, 1 Mar 2021 12:23:06 -0500 Subject: [PATCH 01/33] server: add --enable-core-dump flag This ensures Envoy can core dump when the dumpability bit might have been unset (e.g.: running inside a container with fewer capabilities than the ones Envoy itself has). Fixes #15242. Signed-off-by: Raul Gutierrez Segales --- api/envoy/admin/v3/server_info.proto | 5 ++++- api/envoy/admin/v4alpha/server_info.proto | 5 ++++- docs/root/version_history/current.rst | 1 + generated_api_shadow/envoy/admin/v3/server_info.proto | 5 ++++- .../envoy/admin/v4alpha/server_info.proto | 5 ++++- include/envoy/server/options.h | 5 +++++ source/server/options_impl.cc | 2 ++ source/server/options_impl.h | 2 ++ source/server/server.cc | 9 +++++++++ test/mocks/server/options.cc | 1 + test/mocks/server/options.h | 2 ++ test/server/options_impl_test.cc | 1 + test/server/server_test.cc | 7 +++++++ 13 files changed, 46 insertions(+), 4 deletions(-) diff --git a/api/envoy/admin/v3/server_info.proto b/api/envoy/admin/v3/server_info.proto index 08fb8dceedd43..108cb8a115487 100644 --- a/api/envoy/admin/v3/server_info.proto +++ b/api/envoy/admin/v3/server_info.proto @@ -58,7 +58,7 @@ message ServerInfo { config.core.v3.Node node = 7; } -// [#next-free-field: 37] +// [#next-free-field: 38] message CommandLineOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.admin.v2alpha.CommandLineOptions"; @@ -189,4 +189,7 @@ message CommandLineOptions { // See :option:`--socket-mode` for details. uint32 socket_mode = 36; + + // See :option:`--enable-core-dump` for details. + bool enable_core_dump = 37; } diff --git a/api/envoy/admin/v4alpha/server_info.proto b/api/envoy/admin/v4alpha/server_info.proto index 9d4db6942f880..18e59c92b0eff 100644 --- a/api/envoy/admin/v4alpha/server_info.proto +++ b/api/envoy/admin/v4alpha/server_info.proto @@ -58,7 +58,7 @@ message ServerInfo { config.core.v4alpha.Node node = 7; } -// [#next-free-field: 37] +// [#next-free-field: 38] message CommandLineOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.admin.v3.CommandLineOptions"; @@ -188,4 +188,7 @@ message CommandLineOptions { // See :option:`--socket-mode` for details. uint32 socket_mode = 36; + + // See :option:`--enable-core-dump` for details. + bool enable_core_dump = 37; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 2d984f50e3a26..c4a77e0bdf8a9 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -111,6 +111,7 @@ New Features * route config: added :ref:`allow_post field ` for allowing POST payload as raw TCP. * route config: added :ref:`max_direct_response_body_size_bytes ` to set maximum :ref:`direct response body ` size in bytes. If not specified the default remains 4096 bytes. * server: added *fips_mode* to :ref:`server compilation settings ` related statistic. +* server: added `--enable-core-dump` flag to enable core dumps via prctl. * tcp_proxy: add support for converting raw TCP streams into HTTP/1.1 CONNECT requests. See :ref:`upgrade documentation ` for details. * tcp_proxy: added a :ref:`use_post field ` for using HTTP POST to proxy TCP streams. * tcp_proxy: added a :ref:`headers_to_add field ` for setting additional headers to the HTTP requests for TCP proxing. diff --git a/generated_api_shadow/envoy/admin/v3/server_info.proto b/generated_api_shadow/envoy/admin/v3/server_info.proto index c7a19453b882e..7257453bc7259 100644 --- a/generated_api_shadow/envoy/admin/v3/server_info.proto +++ b/generated_api_shadow/envoy/admin/v3/server_info.proto @@ -59,7 +59,7 @@ message ServerInfo { config.core.v3.Node node = 7; } -// [#next-free-field: 37] +// [#next-free-field: 38] message CommandLineOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.admin.v2alpha.CommandLineOptions"; @@ -189,6 +189,9 @@ message CommandLineOptions { // See :option:`--socket-mode` for details. uint32 socket_mode = 36; + // See :option:`--enable-core-dump` for details. + bool enable_core_dump = 37; + uint64 hidden_envoy_deprecated_max_stats = 20 [deprecated = true, (envoy.annotations.disallowed_by_default) = true]; diff --git a/generated_api_shadow/envoy/admin/v4alpha/server_info.proto b/generated_api_shadow/envoy/admin/v4alpha/server_info.proto index 9d4db6942f880..18e59c92b0eff 100644 --- a/generated_api_shadow/envoy/admin/v4alpha/server_info.proto +++ b/generated_api_shadow/envoy/admin/v4alpha/server_info.proto @@ -58,7 +58,7 @@ message ServerInfo { config.core.v4alpha.Node node = 7; } -// [#next-free-field: 37] +// [#next-free-field: 38] message CommandLineOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.admin.v3.CommandLineOptions"; @@ -188,4 +188,7 @@ message CommandLineOptions { // See :option:`--socket-mode` for details. uint32 socket_mode = 36; + + // See :option:`--enable-core-dump` for details. + bool enable_core_dump = 37; } diff --git a/include/envoy/server/options.h b/include/envoy/server/options.h index 9b3709af846b7..05729bbe0181d 100644 --- a/include/envoy/server/options.h +++ b/include/envoy/server/options.h @@ -234,6 +234,11 @@ class Options { */ virtual bool mutexTracingEnabled() const PURE; + /** + * @return bool indicating whether core dumps have been enabled. + */ + virtual bool coreDumpEnabled() const PURE; + /** * @return bool indicating whether cpuset size should determine the number of worker threads. */ diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index d4623d5a41412..370e460694be9 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -156,6 +156,7 @@ OptionsImpl::OptionsImpl(std::vector args, TCLAP::ValueArg socket_mode("", "socket-mode", "Socket file permission", false, "600", "string", cmd); + TCLAP::SwitchArg enable_core_dump("", "enable-core-dump", "Enable core dumps", cmd, false); cmd.setExceptionHandling(false); try { @@ -177,6 +178,7 @@ OptionsImpl::OptionsImpl(std::vector args, hot_restart_disabled_ = disable_hot_restart.getValue(); mutex_tracing_enabled_ = enable_mutex_tracing.getValue(); + core_dump_enabled_ = enable_core_dump.getValue(); cpuset_threads_ = cpuset_threads.getValue(); diff --git a/source/server/options_impl.h b/source/server/options_impl.h index 85897ac7d8227..370480c755f75 100644 --- a/source/server/options_impl.h +++ b/source/server/options_impl.h @@ -146,6 +146,7 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable disabled_extensions_; uint32_t count_; diff --git a/source/server/server.cc b/source/server/server.cc index 674c84393fe1d..3b9e21ff7d966 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -1,5 +1,7 @@ #include "server/server.h" +#include + #include #include #include @@ -90,6 +92,13 @@ InstanceImpl::InstanceImpl( grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), server_contexts_(*this) { + +#ifdef PR_SET_DUMPABLE + if (options.coreDumpEnabled()) { + prctl(PR_SET_DUMPABLE, 1); + } +#endif + try { if (!options.logPath().empty()) { try { diff --git a/test/mocks/server/options.cc b/test/mocks/server/options.cc index 354628ee65c75..09409e2772c53 100644 --- a/test/mocks/server/options.cc +++ b/test/mocks/server/options.cc @@ -38,6 +38,7 @@ MockOptions::MockOptions(const std::string& config_path) : config_path_(config_p ON_CALL(*this, hotRestartDisabled()).WillByDefault(ReturnPointee(&hot_restart_disabled_)); ON_CALL(*this, signalHandlingEnabled()).WillByDefault(ReturnPointee(&signal_handling_enabled_)); ON_CALL(*this, mutexTracingEnabled()).WillByDefault(ReturnPointee(&mutex_tracing_enabled_)); + ON_CALL(*this, coreDumpEnabled()).WillByDefault(ReturnPointee(&core_dump_enabled_)); ON_CALL(*this, cpusetThreadsEnabled()).WillByDefault(ReturnPointee(&cpuset_threads_enabled_)); ON_CALL(*this, disabledExtensions()).WillByDefault(ReturnRef(disabled_extensions_)); ON_CALL(*this, toCommandLineOptions()).WillByDefault(Invoke([] { diff --git a/test/mocks/server/options.h b/test/mocks/server/options.h index d28fd47b143ae..9bc6e721ea68f 100644 --- a/test/mocks/server/options.h +++ b/test/mocks/server/options.h @@ -47,6 +47,7 @@ class MockOptions : public Options { MOCK_METHOD(bool, hotRestartDisabled, (), (const)); MOCK_METHOD(bool, signalHandlingEnabled, (), (const)); MOCK_METHOD(bool, mutexTracingEnabled, (), (const)); + MOCK_METHOD(bool, coreDumpEnabled, (), (const)); MOCK_METHOD(bool, cpusetThreadsEnabled, (), (const)); MOCK_METHOD(const std::vector&, disabledExtensions, (), (const)); MOCK_METHOD(Server::CommandLineOptionsPtr, toCommandLineOptions, (), (const)); @@ -71,6 +72,7 @@ class MockOptions : public Options { bool hot_restart_disabled_{}; bool signal_handling_enabled_{true}; bool mutex_tracing_enabled_{}; + bool core_dump_enabled_{}; bool cpuset_threads_enabled_{}; std::vector disabled_extensions_; std::string socket_path_; diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index 8b5019ee01f3b..9ed6289c323e7 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -243,6 +243,7 @@ TEST_F(OptionsImplTest, SetAll) { EXPECT_EQ(options->serviceZone(), command_line_options->service_zone()); EXPECT_EQ(options->hotRestartDisabled(), command_line_options->disable_hot_restart()); EXPECT_EQ(options->mutexTracingEnabled(), command_line_options->enable_mutex_tracing()); + EXPECT_EQ(options->coreDumpEnabled(), command_line_options->enable_core_dump()); EXPECT_EQ(options->cpusetThreadsEnabled(), command_line_options->cpuset_threads()); EXPECT_EQ(options->socketPath(), command_line_options->socket_path()); EXPECT_EQ(options->socketMode(), command_line_options->socket_mode()); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 681bb9cf068e0..142fac58a03fd 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1334,6 +1334,13 @@ TEST_P(ServerInstanceImplTest, MutexContentionEnabled) { EXPECT_NO_THROW(initialize("test/server/test_data/server/empty_bootstrap.yaml")); } +TEST_P(ServerInstanceImplTest, CoreDumpEnabled) { + options_.service_cluster_name_ = "some_cluster_name"; + options_.service_node_name_ = "some_node_name"; + options_.core_dump_enabled_ = true; + EXPECT_NO_THROW(initialize("test/server/test_data/server/empty_bootstrap.yaml")); +} + TEST_P(ServerInstanceImplTest, NoHttpTracing) { options_.service_cluster_name_ = "some_cluster_name"; options_.service_node_name_ = "some_node_name"; From 82e1cebd68f1800052ae305b3712c136f65aefcb Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Mon, 1 Mar 2021 12:43:30 -0500 Subject: [PATCH 02/33] Fix docs Signed-off-by: Raul Gutierrez Segales --- docs/root/operations/cli.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/root/operations/cli.rst b/docs/root/operations/cli.rst index 4f7129a1b4dfc..9470904aa3d41 100644 --- a/docs/root/operations/cli.rst +++ b/docs/root/operations/cli.rst @@ -350,3 +350,9 @@ following are the command line options that Envoy supports. * build mode - either ``RELEASE`` or ``DEBUG``, * TLS library - either ``BoringSSL`` or ``BoringSSL-FIPS``. + +.. option:: --enable-core-dump + + *(optional)* This flag enables core dumps. This is useful for container environments when using + capabilities, given that when Envoy has more capabilities than its base environment core dumping will + be disabled by the kernel. Using this flag ensures it remains enabled. From 906aaa5c20455935e696e4e545e1fdaa2079c410 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Mon, 1 Mar 2021 14:13:57 -0500 Subject: [PATCH 03/33] Fix windows Signed-off-by: Raul Gutierrez Segales --- source/server/server.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/server/server.cc b/source/server/server.cc index 3b9e21ff7d966..233bdc2238a5e 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -1,6 +1,8 @@ #include "server/server.h" +#ifndef WIN32 #include +#endif #include #include From 1b85c69e3f49653020cf934c548957c5257c58ce Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Mon, 1 Mar 2021 16:04:16 -0500 Subject: [PATCH 04/33] Skip prctl for OS X too Signed-off-by: Raul Gutierrez Segales --- source/server/server.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/server.cc b/source/server/server.cc index 233bdc2238a5e..70e542da79977 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -1,6 +1,6 @@ #include "server/server.h" -#ifndef WIN32 +#if !defined(WIN32) && !defined(__APPLE__) #include #endif From 57e810f65a94db431987276f0c4012de64952705 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Mon, 1 Mar 2021 16:39:36 -0500 Subject: [PATCH 05/33] Check prctl's return value and log accordingly Signed-off-by: Raul Gutierrez Segales --- source/server/server.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/server/server.cc b/source/server/server.cc index 70e542da79977..26200eb026cbf 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -97,7 +97,9 @@ InstanceImpl::InstanceImpl( #ifdef PR_SET_DUMPABLE if (options.coreDumpEnabled()) { - prctl(PR_SET_DUMPABLE, 1); + if (prctl(PR_SET_DUMPABLE, 1) == -1) { + ENVOY_LOG(error, "prctl call to enable core dumps failed"); + } } #endif From 09d58c0dd25faef0d88947f3a35126518550c3fd Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 2 Mar 2021 11:23:28 -0500 Subject: [PATCH 06/33] Move code to platform specific files * fix/improve cli doc * introduce linux/platform_impl.cc for Linux specific prctl calls * enabling core dump now happens in MainCommonBase instead of InstanceImpl, because we need access to PlatformImpl * also, move PlatformImpl from MainCommon to MainCommonBase * added some bazel helpers to distinguish Linux from the rest of Posix Signed-off-by: Raul Gutierrez Segales --- bazel/envoy_library.bzl | 32 +++++++++++++++++++++++++++++++ docs/root/operations/cli.rst | 7 ++++--- source/exe/BUILD | 15 +++++++++++++-- source/exe/linux/platform_impl.cc | 22 +++++++++++++++++++++ source/exe/main_common.cc | 21 +++++++++++--------- source/exe/main_common.h | 5 +---- source/exe/platform_impl.h | 1 + source/exe/posix/platform_impl.cc | 2 ++ source/exe/win32/platform_impl.cc | 2 ++ source/server/server.cc | 13 ------------- test/exe/main_common_test.cc | 13 ++++++------- 11 files changed, 95 insertions(+), 38 deletions(-) create mode 100644 source/exe/linux/platform_impl.cc diff --git a/bazel/envoy_library.bzl b/bazel/envoy_library.bzl index 0131f3c132c75..5aee3c22d0569 100644 --- a/bazel/envoy_library.bzl +++ b/bazel/envoy_library.bzl @@ -218,6 +218,38 @@ def envoy_cc_posix_library(name, srcs = [], hdrs = [], **kargs): **kargs ) +# Used to specify a library that only builds on POSIX excluding Linux +def envoy_cc_posix_without_linux_library(name, srcs = [], hdrs = [], **kargs): + envoy_cc_library( + name = name + "_posix", + srcs = select({ + "@envoy//bazel:windows_x86_64": [], + "@envoy//bazel:linux": [], + "//conditions:default": srcs, + }), + hdrs = select({ + "@envoy//bazel:windows_x86_64": [], + "@envoy//bazel:linux": [], + "//conditions:default": hdrs, + }), + **kargs + ) + +# Used to specify a library that only builds on Linux +def envoy_cc_linux_library(name, srcs = [], hdrs = [], **kargs): + envoy_cc_library( + name = name + "_linux", + srcs = select({ + "@envoy//bazel:linux": srcs, + "//conditions:default": [], + }), + hdrs = select({ + "@envoy//bazel:linux": hdrs, + "//conditions:default": [], + }), + **kargs + ) + # Used to specify a library that only builds on Windows def envoy_cc_win32_library(name, srcs = [], hdrs = [], **kargs): envoy_cc_library( diff --git a/docs/root/operations/cli.rst b/docs/root/operations/cli.rst index 9470904aa3d41..fd9c3be5ebc63 100644 --- a/docs/root/operations/cli.rst +++ b/docs/root/operations/cli.rst @@ -353,6 +353,7 @@ following are the command line options that Envoy supports. .. option:: --enable-core-dump - *(optional)* This flag enables core dumps. This is useful for container environments when using - capabilities, given that when Envoy has more capabilities than its base environment core dumping will - be disabled by the kernel. Using this flag ensures it remains enabled. + *(optional)* This flag is intended for Linux-based systems and it's a no-op for all other platforms. + It enables core dumps by invoking `prctl `_ using the + PR_SET_DUMPABLE option. This is useful for container environments when using capabilities, given that when + Envoy has more capabilities than its base environment core dumping will be disabled by the kernel. diff --git a/source/exe/BUILD b/source/exe/BUILD index 1b8bc64bbcb54..673ed75400114 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -2,8 +2,9 @@ load( "//bazel:envoy_build_system.bzl", "envoy_cc_binary", "envoy_cc_library", + "envoy_cc_linux_library", "envoy_cc_platform_dep", - "envoy_cc_posix_library", + "envoy_cc_posix_without_linux_library", "envoy_cc_win32_library", "envoy_package", ) @@ -153,7 +154,7 @@ envoy_cc_library( ], ) -envoy_cc_posix_library( +envoy_cc_posix_without_linux_library( name = "platform_impl_lib", srcs = ["posix/platform_impl.cc"], deps = [ @@ -163,6 +164,16 @@ envoy_cc_posix_library( ], ) +envoy_cc_linux_library( + name = "platform_impl_lib", + srcs = ["linux/platform_impl.cc"], + deps = [ + ":platform_header_lib", + "//source/common/common:thread_lib", + "//source/common/filesystem:filesystem_lib", + ], +) + envoy_cc_win32_library( name = "platform_impl_lib", srcs = ["win32/platform_impl.cc"], diff --git a/source/exe/linux/platform_impl.cc b/source/exe/linux/platform_impl.cc new file mode 100644 index 0000000000000..60e2ae80914ab --- /dev/null +++ b/source/exe/linux/platform_impl.cc @@ -0,0 +1,22 @@ +#if !defined(__linux__) +#error "Linux platform file is part of non-Linux build." +#endif + +#include + +#include "common/common/thread_impl.h" +#include "common/filesystem/filesystem_impl.h" + +#include "exe/platform_impl.h" + +namespace Envoy { + +PlatformImpl::PlatformImpl() + : thread_factory_(std::make_unique()), + file_system_(std::make_unique()) {} + +PlatformImpl::~PlatformImpl() = default; + +bool PlatformImpl::enableCoreDump() { return prctl(PR_SET_DUMPABLE, 1) != -1; } + +} // namespace Envoy diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 3ac535e192811..aa69ea87d0678 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -46,15 +46,19 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem ListenerHooks& listener_hooks, Server::ComponentFactory& component_factory, std::unique_ptr&& random_generator, - Thread::ThreadFactory& thread_factory, - Filesystem::Instance& file_system, std::unique_ptr process_context) - : options_(options), component_factory_(component_factory), thread_factory_(thread_factory), - file_system_(file_system), stats_allocator_(symbol_table_) { + : options_(options), component_factory_(component_factory), stats_allocator_(symbol_table_) { // Process the option to disable extensions as early as possible, // before we do any configuration loading. OptionsImpl::disableExtensions(options.disabledExtensions()); + // Enable core dumps as early as possible. + if (options_.coreDumpEnabled()) { + if (!platform_impl_.enableCoreDump()) { + ENVOY_LOG_MISC(error, "failed to enable core dump"); + } + } + switch (options_.mode()) { case Server::Mode::InitOnly: case Server::Mode::Serve: { @@ -79,7 +83,7 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem server_ = std::make_unique( *init_manager_, options_, time_system, local_address, listener_hooks, *restarter_, *stats_store_, access_log_lock, component_factory, std::move(random_generator), *tls_, - thread_factory_, file_system_, std::move(process_context)); + platform_impl_.threadFactory(), platform_impl_.fileSystem(), std::move(process_context)); break; } @@ -163,8 +167,8 @@ bool MainCommonBase::run() { return true; case Server::Mode::Validate: { auto local_address = Network::Utility::getLocalAddress(options_.localAddressIpVersion()); - return Server::validateConfig(options_, local_address, component_factory_, thread_factory_, - file_system_); + return Server::validateConfig(options_, local_address, component_factory_, + platform_impl_.threadFactory(), platform_impl_.fileSystem()); } case Server::Mode::InitOnly: PERF_DUMP(); @@ -188,8 +192,7 @@ void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string MainCommon::MainCommon(int argc, const char* const* argv) : options_(argc, argv, &MainCommon::hotRestartVersion, spdlog::level::info), base_(options_, real_time_system_, default_listener_hooks_, prod_component_factory_, - std::make_unique(), platform_impl_.threadFactory(), - platform_impl_.fileSystem(), nullptr) {} + std::make_unique(), nullptr) {} std::string MainCommon::hotRestartVersion(bool hot_restart_enabled) { #ifdef ENVOY_HOT_RESTART diff --git a/source/exe/main_common.h b/source/exe/main_common.h index 0bc9430ebf2c5..bf3139cff5eba 100644 --- a/source/exe/main_common.h +++ b/source/exe/main_common.h @@ -39,7 +39,6 @@ class MainCommonBase { MainCommonBase(const Server::Options& options, Event::TimeSystem& time_system, ListenerHooks& listener_hooks, Server::ComponentFactory& component_factory, std::unique_ptr&& random_generator, - Thread::ThreadFactory& thread_factory, Filesystem::Instance& file_system, std::unique_ptr process_context); bool run(); @@ -66,6 +65,7 @@ class MainCommonBase { const AdminRequestFn& handler); protected: + PlatformImpl platform_impl_; ProcessWide process_wide_; // Process-wide state setup/teardown (excluding grpc). // We instantiate this class regardless of ENVOY_GOOGLE_GRPC, to avoid having // an ifdef in a header file exposed in a C++ library. It is too easy to have @@ -73,8 +73,6 @@ class MainCommonBase { Grpc::GoogleGrpcContext google_grpc_context_; const Envoy::Server::Options& options_; Server::ComponentFactory& component_factory_; - Thread::ThreadFactory& thread_factory_; - Filesystem::Instance& file_system_; Stats::SymbolTableImpl symbol_table_; Stats::AllocatorImpl stats_allocator_; @@ -144,7 +142,6 @@ class MainCommon { Envoy::TerminateHandler log_on_terminate_; #endif - PlatformImpl platform_impl_; Envoy::OptionsImpl options_; Event::RealTimeSystem real_time_system_; DefaultListenerHooks default_listener_hooks_; diff --git a/source/exe/platform_impl.h b/source/exe/platform_impl.h index 6e1ec22ce872a..86238e33bc0f8 100644 --- a/source/exe/platform_impl.h +++ b/source/exe/platform_impl.h @@ -11,6 +11,7 @@ class PlatformImpl { ~PlatformImpl(); Thread::ThreadFactory& threadFactory() { return *thread_factory_; } Filesystem::Instance& fileSystem() { return *file_system_; } + bool enableCoreDump(); private: Thread::ThreadFactoryPtr thread_factory_; diff --git a/source/exe/posix/platform_impl.cc b/source/exe/posix/platform_impl.cc index 8fc227724bee5..730280c4bd960 100644 --- a/source/exe/posix/platform_impl.cc +++ b/source/exe/posix/platform_impl.cc @@ -11,4 +11,6 @@ PlatformImpl::PlatformImpl() PlatformImpl::~PlatformImpl() = default; +bool PlatformImpl::enableCoreDump() { return false; } + } // namespace Envoy diff --git a/source/exe/win32/platform_impl.cc b/source/exe/win32/platform_impl.cc index 52107ae546fe4..4b10bece2bbf2 100644 --- a/source/exe/win32/platform_impl.cc +++ b/source/exe/win32/platform_impl.cc @@ -58,4 +58,6 @@ PlatformImpl::PlatformImpl() PlatformImpl::~PlatformImpl() { ::WSACleanup(); } +bool PlatformImpl::enableCoreDump() { return false; } + } // namespace Envoy diff --git a/source/server/server.cc b/source/server/server.cc index 26200eb026cbf..674c84393fe1d 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -1,9 +1,5 @@ #include "server/server.h" -#if !defined(WIN32) && !defined(__APPLE__) -#include -#endif - #include #include #include @@ -94,15 +90,6 @@ InstanceImpl::InstanceImpl( grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), server_contexts_(*this) { - -#ifdef PR_SET_DUMPABLE - if (options.coreDumpEnabled()) { - if (prctl(PR_SET_DUMPABLE, 1) == -1) { - ENVOY_LOG(error, "prctl call to enable core dumps failed"); - } - } -#endif - try { if (!options.logPath().empty()) { try { diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 8428cf6b43fe3..aa3dac84b621d 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -120,7 +120,6 @@ TEST_P(MainCommonTest, ConstructWritesBasePathId) { // Test that an in-use base id triggers a retry and that we eventually give up. TEST_P(MainCommonTest, RetryDynamicBaseIdFails) { #ifdef ENVOY_HOT_RESTART - PlatformImpl platform; Event::TestRealTimeSystem real_time_system; DefaultListenerHooks default_listener_hooks; ProdComponentFactory prod_component_factory; @@ -132,7 +131,7 @@ TEST_P(MainCommonTest, RetryDynamicBaseIdFails) { OptionsImpl first_options(first_args, &MainCommon::hotRestartVersion, spdlog::level::info); MainCommonBase first(first_options, real_time_system, default_listener_hooks, prod_component_factory, std::make_unique(), - platform.threadFactory(), platform.fileSystem(), nullptr); + nullptr); const std::string base_id_str = TestEnvironment::readFileToStringForTest(base_id_path); uint32_t base_id; @@ -145,11 +144,11 @@ TEST_P(MainCommonTest, RetryDynamicBaseIdFails) { std::vector({"envoy-static", "--use-dynamic-base-id", "-c", config_file_}); OptionsImpl second_options(second_args, &MainCommon::hotRestartVersion, spdlog::level::info); - EXPECT_THROW_WITH_MESSAGE( - MainCommonBase(second_options, real_time_system, default_listener_hooks, - prod_component_factory, std::unique_ptr{mock_rng}, - platform.threadFactory(), platform.fileSystem(), nullptr), - EnvoyException, "unable to select a dynamic base id"); + EXPECT_THROW_WITH_MESSAGE(MainCommonBase(second_options, real_time_system, default_listener_hooks, + prod_component_factory, + std::unique_ptr{mock_rng}, + nullptr), + EnvoyException, "unable to select a dynamic base id"); #endif } From aa46afcbb2fc8c33897bcb51566c85f915577b7e Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 2 Mar 2021 12:07:54 -0500 Subject: [PATCH 07/33] Fix bazel helpers Signed-off-by: Raul Gutierrez Segales --- bazel/envoy_build_system.bzl | 14 ++++++++++++++ source/exe/BUILD | 4 ++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/bazel/envoy_build_system.bzl b/bazel/envoy_build_system.bzl index e33386ed5b98b..a9bc9639332c5 100644 --- a/bazel/envoy_build_system.bzl +++ b/bazel/envoy_build_system.bzl @@ -8,7 +8,9 @@ load( _envoy_basic_cc_library = "envoy_basic_cc_library", _envoy_cc_extension = "envoy_cc_extension", _envoy_cc_library = "envoy_cc_library", + _envoy_cc_linux_library = "envoy_cc_linux_library", _envoy_cc_posix_library = "envoy_cc_posix_library", + _envoy_cc_posix_without_linux_library = "envoy_cc_posix_without_linux_library", _envoy_cc_win32_library = "envoy_cc_win32_library", _envoy_include_prefix = "envoy_include_prefix", _envoy_proto_library = "envoy_proto_library", @@ -145,6 +147,16 @@ def envoy_cc_platform_dep(name): "//conditions:default": [name + "_posix"], }) +# Used to select a dependency that has different implementations on Linux vs rest of POSIX vs Windows. +# The platform-specific implementations should be specified with envoy_cc_linux_library, +# envoy_cc_posix_without_library and envoy_cc_win32_library respectively +def envoy_cc_platform_specific_dep(name): + return select({ + "@envoy//bazel:windows_x86_64": [name + "_win32"], + "@envoy//bazel:linux": [name + "_linux"], + "//conditions:default": [name + "_posix"], + }) + # Envoy proto descriptor targets should be specified with this function. # This is used for testing only. def envoy_proto_descriptor(name, out, srcs = [], external_deps = []): @@ -202,7 +214,9 @@ envoy_cc_binary = _envoy_cc_binary envoy_basic_cc_library = _envoy_basic_cc_library envoy_cc_extension = _envoy_cc_extension envoy_cc_library = _envoy_cc_library +envoy_cc_linux_library = _envoy_cc_linux_library envoy_cc_posix_library = _envoy_cc_posix_library +envoy_cc_posix_without_linux_library = _envoy_cc_posix_without_linux_library envoy_cc_win32_library = _envoy_cc_win32_library envoy_include_prefix = _envoy_include_prefix envoy_proto_library = _envoy_proto_library diff --git a/source/exe/BUILD b/source/exe/BUILD index 673ed75400114..0dca0d67354cd 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -3,7 +3,7 @@ load( "envoy_cc_binary", "envoy_cc_library", "envoy_cc_linux_library", - "envoy_cc_platform_dep", + "envoy_cc_platform_specific_dep", "envoy_cc_posix_without_linux_library", "envoy_cc_win32_library", "envoy_package", @@ -142,7 +142,7 @@ envoy_cc_library( envoy_cc_library( name = "platform_impl_lib", deps = [":platform_header_lib"] + - envoy_cc_platform_dep("platform_impl_lib"), + envoy_cc_platform_specific_dep("platform_impl_lib"), ) envoy_cc_library( From 193bae6ed84075a7ae30a418e13c97a9fd48a78c Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 2 Mar 2021 12:54:42 -0500 Subject: [PATCH 08/33] Fix selectors Signed-off-by: Raul Gutierrez Segales --- bazel/envoy_library.bzl | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bazel/envoy_library.bzl b/bazel/envoy_library.bzl index 5aee3c22d0569..0319adf40f1a9 100644 --- a/bazel/envoy_library.bzl +++ b/bazel/envoy_library.bzl @@ -240,11 +240,17 @@ def envoy_cc_linux_library(name, srcs = [], hdrs = [], **kargs): envoy_cc_library( name = name + "_linux", srcs = select({ - "@envoy//bazel:linux": srcs, + "@envoy//bazel:linux_x86_64": srcs, + "@envoy//bazel:linux_aarch64": srcs, + "@envoy//bazel:linux_ppc": srcs, + "@envoy//bazel:linux_mips64": srcs, "//conditions:default": [], }), hdrs = select({ - "@envoy//bazel:linux": hdrs, + "@envoy//bazel:linux_x86_64": hdrs, + "@envoy//bazel:linux_aarch64": hdrs, + "@envoy//bazel:linux_ppc": hdrs, + "@envoy//bazel:linux_mips64": hdrs, "//conditions:default": [], }), **kargs From 28967293e6184f797fbac22e5a455f04026c7dad Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 2 Mar 2021 13:00:09 -0500 Subject: [PATCH 09/33] Update envoy_cc_posix_without_linux_library Signed-off-by: Raul Gutierrez Segales --- bazel/envoy_library.bzl | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bazel/envoy_library.bzl b/bazel/envoy_library.bzl index 0319adf40f1a9..af5e34f2decf1 100644 --- a/bazel/envoy_library.bzl +++ b/bazel/envoy_library.bzl @@ -224,12 +224,18 @@ def envoy_cc_posix_without_linux_library(name, srcs = [], hdrs = [], **kargs): name = name + "_posix", srcs = select({ "@envoy//bazel:windows_x86_64": [], - "@envoy//bazel:linux": [], + "@envoy//bazel:linux_x86_64": [], + "@envoy//bazel:linux_aarch64": [], + "@envoy//bazel:linux_ppc": [], + "@envoy//bazel:linux_mips64": [], "//conditions:default": srcs, }), hdrs = select({ "@envoy//bazel:windows_x86_64": [], - "@envoy//bazel:linux": [], + "@envoy//bazel:linux_x86_64": [], + "@envoy//bazel:linux_aarch64": [], + "@envoy//bazel:linux_ppc": [], + "@envoy//bazel:linux_mips64": [], "//conditions:default": hdrs, }), **kargs From 7c831e3c5609fa5ecec42004729bb9d13fa1fd4a Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 2 Mar 2021 14:26:11 -0500 Subject: [PATCH 10/33] Fix CI Remove outer calls to envoy_cc_platform_dep, which means we cannot properly pick the Linux platform. Signed-off-by: Raul Gutierrez Segales --- test/extensions/common/redis/BUILD | 3 ++- tools/BUILD | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/extensions/common/redis/BUILD b/test/extensions/common/redis/BUILD index 96ae92a2a8613..83e2d561229ce 100644 --- a/test/extensions/common/redis/BUILD +++ b/test/extensions/common/redis/BUILD @@ -30,6 +30,7 @@ envoy_extension_cc_test( deps = [ "//source/common/common:lock_guard_lib", "//source/common/common:thread_lib", + "//source/exe:platform_impl_lib", "//source/extensions/common/redis:cluster_refresh_manager_lib", "//test/extensions/filters/network/common/redis:redis_mocks", "//test/extensions/filters/network/redis_proxy:redis_mocks", @@ -37,5 +38,5 @@ envoy_extension_cc_test( "//test/mocks/upstream:priority_set_mocks", "//test/test_common:simulated_time_system_lib", "//test/test_common:utility_lib", - ] + envoy_cc_platform_dep("//source/exe:platform_impl_lib"), + ], ) diff --git a/tools/BUILD b/tools/BUILD index d9dec1be786be..9d51dc642a64a 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -44,7 +44,8 @@ envoy_cc_binary( "//source/common/protobuf:utility_lib", "//source/common/common:random_generator_lib", "//source/common/stats:isolated_store_lib", + "//source/exe:platform_impl_lib", "//test/test_common:test_version_linkstamp", "@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto", - ] + envoy_cc_platform_dep("//source/exe:platform_impl_lib"), + ], ) From d120e3e9c453d745437e6c107e00ae845ee00bee Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 2 Mar 2021 14:36:39 -0500 Subject: [PATCH 11/33] Info log when prctl succeeds Signed-off-by: Raul Gutierrez Segales --- source/exe/main_common.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index aa69ea87d0678..3bef2b5c7a794 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -54,7 +54,9 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem // Enable core dumps as early as possible. if (options_.coreDumpEnabled()) { - if (!platform_impl_.enableCoreDump()) { + if (platform_impl_.enableCoreDump()) { + ENVOY_LOG_MISC(info, "core dump enabled"); + } else { ENVOY_LOG_MISC(error, "failed to enable core dump"); } } From fd4f178fb8184f81b8f3f19ba3c3e9b3c0846f30 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 2 Mar 2021 15:37:12 -0500 Subject: [PATCH 12/33] Fix format Signed-off-by: Raul Gutierrez Segales --- test/extensions/common/redis/BUILD | 1 - tools/BUILD | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/test/extensions/common/redis/BUILD b/test/extensions/common/redis/BUILD index 83e2d561229ce..4b54046b37900 100644 --- a/test/extensions/common/redis/BUILD +++ b/test/extensions/common/redis/BUILD @@ -1,6 +1,5 @@ load( "//bazel:envoy_build_system.bzl", - "envoy_cc_platform_dep", "envoy_cc_test_library", "envoy_package", ) diff --git a/tools/BUILD b/tools/BUILD index 9d51dc642a64a..5d1d9bde640da 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -2,7 +2,6 @@ load("@rules_python//python:defs.bzl", "py_library") load( "//bazel:envoy_build_system.bzl", "envoy_cc_binary", - "envoy_cc_platform_dep", "envoy_package", "envoy_py_test_binary", ) @@ -40,9 +39,9 @@ envoy_cc_binary( deps = [ "//source/common/api:api_lib", "//source/common/common:assert_lib", + "//source/common/common:random_generator_lib", "//source/common/protobuf:message_validator_lib", "//source/common/protobuf:utility_lib", - "//source/common/common:random_generator_lib", "//source/common/stats:isolated_store_lib", "//source/exe:platform_impl_lib", "//test/test_common:test_version_linkstamp", From 61a490c20fd3031311639c5ae7ccbb87f7a7d2b6 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 2 Mar 2021 15:49:32 -0500 Subject: [PATCH 13/33] Improve changelog Signed-off-by: Raul Gutierrez Segales --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c4a77e0bdf8a9..da6a144914174 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -111,7 +111,7 @@ New Features * route config: added :ref:`allow_post field ` for allowing POST payload as raw TCP. * route config: added :ref:`max_direct_response_body_size_bytes ` to set maximum :ref:`direct response body ` size in bytes. If not specified the default remains 4096 bytes. * server: added *fips_mode* to :ref:`server compilation settings ` related statistic. -* server: added `--enable-core-dump` flag to enable core dumps via prctl. +* server: added `--enable-core-dump` flag to enable core dumps via prctl (Linux-based systems only). * tcp_proxy: add support for converting raw TCP streams into HTTP/1.1 CONNECT requests. See :ref:`upgrade documentation ` for details. * tcp_proxy: added a :ref:`use_post field ` for using HTTP POST to proxy TCP streams. * tcp_proxy: added a :ref:`headers_to_add field ` for setting additional headers to the HTTP requests for TCP proxing. From 2276fce01fee934b7b2e0a65d7af3926cd209a7a Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 2 Mar 2021 21:03:00 -0500 Subject: [PATCH 14/33] Add test for enableCoreDump() This should fix the coverage issue and catch any regressions if our kernel friends were to ever break prctl(). Signed-off-by: Raul Gutierrez Segales --- test/exe/BUILD | 9 +++++++++ test/exe/platform_impl_test.cc | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 test/exe/platform_impl_test.cc diff --git a/test/exe/BUILD b/test/exe/BUILD index 49caa21fef2c6..bfc97ccc22690 100644 --- a/test/exe/BUILD +++ b/test/exe/BUILD @@ -115,3 +115,12 @@ envoy_cc_test( "//test/test_common:environment_lib", ], ) + +envoy_cc_test( + name = "platform_impl_test", + srcs = ["platform_impl_test.cc"], + deps = [ + "//source/exe:platform_impl_lib", + "//test/mocks/runtime:runtime_mocks", + ], +) diff --git a/test/exe/platform_impl_test.cc b/test/exe/platform_impl_test.cc new file mode 100644 index 0000000000000..0d3fdc541f13a --- /dev/null +++ b/test/exe/platform_impl_test.cc @@ -0,0 +1,21 @@ +#include "exe/platform_impl.h" + +#include "test/mocks/common.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { + +TEST(PlatformImpl, Basic) { + PlatformImpl platform; + +#if !defined(WIN32) && !defined(__APPLE__) + EXPECT_EQ(true, platform.enableCoreDump()); +#else + EXPECT_EQ(false, platform.enableCoreDump()); +#endif +} + +} // namespace Envoy From e6284903e6217e6733abf873ee21ff34c6b2ad15 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 3 Mar 2021 08:18:36 -0500 Subject: [PATCH 15/33] Add test to exercise --enable-core-dump Signed-off-by: Raul Gutierrez Segales --- test/exe/main_common_test.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index aa3dac84b621d..10da67de54d84 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -117,6 +117,12 @@ TEST_P(MainCommonTest, ConstructWritesBasePathId) { #endif } +// Exercise enabling core dump. +TEST_P(MainCommonTest, EnableCoreDump) { + addArg("--enable-core-dump"); + VERBOSE_EXPECT_NO_THROW(MainCommon main_common(argc(), argv())); +} + // Test that an in-use base id triggers a retry and that we eventually give up. TEST_P(MainCommonTest, RetryDynamicBaseIdFails) { #ifdef ENVOY_HOT_RESTART From 224f5ff85c19a9e2f80348ad8ab9020beb64e099 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 3 Mar 2021 08:21:07 -0500 Subject: [PATCH 16/33] Reduce branching to avoid coverage issues Signed-off-by: Raul Gutierrez Segales --- source/exe/main_common.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 3bef2b5c7a794..76ace563bb0a2 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -54,11 +54,7 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem // Enable core dumps as early as possible. if (options_.coreDumpEnabled()) { - if (platform_impl_.enableCoreDump()) { - ENVOY_LOG_MISC(info, "core dump enabled"); - } else { - ENVOY_LOG_MISC(error, "failed to enable core dump"); - } + ENVOY_LOG_MISC(info, "core dump enabled: {}", platform_impl_.enableCoreDump()); } switch (options_.mode()) { From ee968ed5a47dd0d700d2b54b5f5a1bf9ba5f2749 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 3 Mar 2021 08:27:34 -0500 Subject: [PATCH 17/33] Decouple logging call from enabling call Signed-off-by: Raul Gutierrez Segales --- source/exe/main_common.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 76ace563bb0a2..f81557257dc83 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -54,7 +54,8 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem // Enable core dumps as early as possible. if (options_.coreDumpEnabled()) { - ENVOY_LOG_MISC(info, "core dump enabled: {}", platform_impl_.enableCoreDump()); + auto ret = platform_impl_.enableCoreDump(); + ENVOY_LOG_MISC(info, "core dump enabled: {}", ret); } switch (options_.mode()) { From ff2e246b37bd460961a4ee74075a701aac14b7fe Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 3 Mar 2021 20:33:38 -0500 Subject: [PATCH 18/33] Link to cmdline flag Signed-off-by: Raul Gutierrez Segales --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 32898247a886e..95440458f89c1 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -121,7 +121,7 @@ New Features * route config: added :ref:`allow_post field ` for allowing POST payload as raw TCP. * route config: added :ref:`max_direct_response_body_size_bytes ` to set maximum :ref:`direct response body ` size in bytes. If not specified the default remains 4096 bytes. * server: added *fips_mode* to :ref:`server compilation settings ` related statistic. -* server: added `--enable-core-dump` flag to enable core dumps via prctl (Linux-based systems only). +* server: added :option:`--enable-core-dump` flag to enable core dumps via prctl (Linux-based systems only). * tcp_proxy: add support for converting raw TCP streams into HTTP/1.1 CONNECT requests. See :ref:`upgrade documentation ` for details. * tcp_proxy: added a :ref:`use_post field ` for using HTTP POST to proxy TCP streams. * tcp_proxy: added a :ref:`headers_to_add field ` for setting additional headers to the HTTP requests for TCP proxing. From 9c5ae2e6d7392dac8b0233a764dc12371e3d24a0 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 4 Mar 2021 13:42:40 -0500 Subject: [PATCH 19/33] Split info vs warn depending on enableCoreDump()'s return Signed-off-by: Raul Gutierrez Segales --- source/exe/main_common.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index f81557257dc83..21e071284de9e 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -55,7 +55,11 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem // Enable core dumps as early as possible. if (options_.coreDumpEnabled()) { auto ret = platform_impl_.enableCoreDump(); - ENVOY_LOG_MISC(info, "core dump enabled: {}", ret); + if (ret) { + ENVOY_LOG_MISC(info, "core dump enabled: {}", ret); + } else { + ENVOY_LOG_MISC(warn, "core dump enabled: {}", ret); + } } switch (options_.mode()) { From 129d19c97449c2d4b5431bc6cf937312e2af95e1 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 4 Mar 2021 14:33:53 -0500 Subject: [PATCH 20/33] Fix logging Signed-off-by: Raul Gutierrez Segales --- source/exe/main_common.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 21e071284de9e..5b4afb2b52966 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -54,11 +54,11 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem // Enable core dumps as early as possible. if (options_.coreDumpEnabled()) { - auto ret = platform_impl_.enableCoreDump(); + const auto ret = platform_impl_.enableCoreDump(); if (ret) { - ENVOY_LOG_MISC(info, "core dump enabled: {}", ret); + ENVOY_LOG_MISC(info, "core dump enabled"); } else { - ENVOY_LOG_MISC(warn, "core dump enabled: {}", ret); + ENVOY_LOG_MISC(warn, "failed to enable core dump"); } } From 73300934af5704d9d91b83d7e3021b94880cee6a Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 4 Mar 2021 15:29:49 -0500 Subject: [PATCH 21/33] Test enableCoreDump() fails path Signed-off-by: Raul Gutierrez Segales --- source/exe/main_common.cc | 13 ++++++++----- source/exe/main_common.h | 3 ++- source/exe/platform_impl.h | 2 +- test/exe/main_common_test.cc | 28 ++++++++++++++++++++++++---- 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 5b4afb2b52966..e24be86832ec7 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -45,16 +45,18 @@ Runtime::LoaderPtr ProdComponentFactory::createRuntime(Server::Instance& server, MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem& time_system, ListenerHooks& listener_hooks, Server::ComponentFactory& component_factory, + std::unique_ptr platform_impl, std::unique_ptr&& random_generator, std::unique_ptr process_context) - : options_(options), component_factory_(component_factory), stats_allocator_(symbol_table_) { + : platform_impl_(platform_impl), options_(options), component_factory_(component_factory), + stats_allocator_(symbol_table_) { // Process the option to disable extensions as early as possible, // before we do any configuration loading. OptionsImpl::disableExtensions(options.disabledExtensions()); // Enable core dumps as early as possible. if (options_.coreDumpEnabled()) { - const auto ret = platform_impl_.enableCoreDump(); + const auto ret = platform_impl_->enableCoreDump(); if (ret) { ENVOY_LOG_MISC(info, "core dump enabled"); } else { @@ -86,7 +88,7 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem server_ = std::make_unique( *init_manager_, options_, time_system, local_address, listener_hooks, *restarter_, *stats_store_, access_log_lock, component_factory, std::move(random_generator), *tls_, - platform_impl_.threadFactory(), platform_impl_.fileSystem(), std::move(process_context)); + platform_impl_->threadFactory(), platform_impl_->fileSystem(), std::move(process_context)); break; } @@ -171,7 +173,7 @@ bool MainCommonBase::run() { case Server::Mode::Validate: { auto local_address = Network::Utility::getLocalAddress(options_.localAddressIpVersion()); return Server::validateConfig(options_, local_address, component_factory_, - platform_impl_.threadFactory(), platform_impl_.fileSystem()); + platform_impl_->threadFactory(), platform_impl_->fileSystem()); } case Server::Mode::InitOnly: PERF_DUMP(); @@ -195,7 +197,8 @@ void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string MainCommon::MainCommon(int argc, const char* const* argv) : options_(argc, argv, &MainCommon::hotRestartVersion, spdlog::level::info), base_(options_, real_time_system_, default_listener_hooks_, prod_component_factory_, - std::make_unique(), nullptr) {} + std::make_unique(), std::make_unique(), + nullptr) {} std::string MainCommon::hotRestartVersion(bool hot_restart_enabled) { #ifdef ENVOY_HOT_RESTART diff --git a/source/exe/main_common.h b/source/exe/main_common.h index bf3139cff5eba..ec36502098c04 100644 --- a/source/exe/main_common.h +++ b/source/exe/main_common.h @@ -38,6 +38,7 @@ class MainCommonBase { // destructed. MainCommonBase(const Server::Options& options, Event::TimeSystem& time_system, ListenerHooks& listener_hooks, Server::ComponentFactory& component_factory, + std::shared_ptr platform_impl, std::unique_ptr&& random_generator, std::unique_ptr process_context); @@ -65,7 +66,7 @@ class MainCommonBase { const AdminRequestFn& handler); protected: - PlatformImpl platform_impl_; + std::unique_ptr platform_impl_; ProcessWide process_wide_; // Process-wide state setup/teardown (excluding grpc). // We instantiate this class regardless of ENVOY_GOOGLE_GRPC, to avoid having // an ifdef in a header file exposed in a C++ library. It is too easy to have diff --git a/source/exe/platform_impl.h b/source/exe/platform_impl.h index 86238e33bc0f8..e18e72731ea2a 100644 --- a/source/exe/platform_impl.h +++ b/source/exe/platform_impl.h @@ -11,7 +11,7 @@ class PlatformImpl { ~PlatformImpl(); Thread::ThreadFactory& threadFactory() { return *thread_factory_; } Filesystem::Instance& fileSystem() { return *file_system_; } - bool enableCoreDump(); + virtual bool enableCoreDump(); private: Thread::ThreadFactoryPtr thread_factory_; diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 10da67de54d84..090b11a8e6766 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -117,12 +117,32 @@ TEST_P(MainCommonTest, ConstructWritesBasePathId) { #endif } -// Exercise enabling core dump. +// Exercise enabling core dump and succeeding. +// Note: this test will call the real prctl(), which is what we want. TEST_P(MainCommonTest, EnableCoreDump) { addArg("--enable-core-dump"); VERBOSE_EXPECT_NO_THROW(MainCommon main_common(argc(), argv())); } +class MockPlatformImpl : public PlatformImpl { + bool enableCoreDump() override { return false; } +} + +// Exercise enabling core dump and failing. +// Note: this simulates a failed prctl() call. +TEST_P(MainCommonTest, EnableCoreDumpFails) { + Event::TestRealTimeSystem real_time_system; + DefaultListenerHooks default_listener_hooks; + ProdComponentFactory prod_component_factory; + + const auto args = std::vector( + {"envoy-static", "--use-dynamic-base-id", "-c", config_file_.c_str(), "--enable-core-dump"}); + OptionsImpl options(args, &MainCommon::hotRestartVersion, spdlog::level::info); + MainCommonBase first(options, real_time_system, default_listener_hooks, prod_component_factory, + std::make_unique(), + std::make_unique(), nullptr); +} + // Test that an in-use base id triggers a retry and that we eventually give up. TEST_P(MainCommonTest, RetryDynamicBaseIdFails) { #ifdef ENVOY_HOT_RESTART @@ -136,8 +156,8 @@ TEST_P(MainCommonTest, RetryDynamicBaseIdFails) { config_file_, "--base-id-path", base_id_path}); OptionsImpl first_options(first_args, &MainCommon::hotRestartVersion, spdlog::level::info); MainCommonBase first(first_options, real_time_system, default_listener_hooks, - prod_component_factory, std::make_unique(), - nullptr); + prod_component_factory, std::make_unique(), + std::make_unique(), nullptr); const std::string base_id_str = TestEnvironment::readFileToStringForTest(base_id_path); uint32_t base_id; @@ -151,7 +171,7 @@ TEST_P(MainCommonTest, RetryDynamicBaseIdFails) { OptionsImpl second_options(second_args, &MainCommon::hotRestartVersion, spdlog::level::info); EXPECT_THROW_WITH_MESSAGE(MainCommonBase(second_options, real_time_system, default_listener_hooks, - prod_component_factory, + prod_component_factory, std::make_unique(), std::unique_ptr{mock_rng}, nullptr), EnvoyException, "unable to select a dynamic base id"); From d0532e22c67302fde94a9e183200bbefa9137ccb Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 4 Mar 2021 16:40:23 -0500 Subject: [PATCH 22/33] unique_ptr Signed-off-by: Raul Gutierrez Segales --- source/exe/main_common.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/exe/main_common.h b/source/exe/main_common.h index ec36502098c04..d970b3dd8047d 100644 --- a/source/exe/main_common.h +++ b/source/exe/main_common.h @@ -38,7 +38,7 @@ class MainCommonBase { // destructed. MainCommonBase(const Server::Options& options, Event::TimeSystem& time_system, ListenerHooks& listener_hooks, Server::ComponentFactory& component_factory, - std::shared_ptr platform_impl, + std::unique_ptr platform_impl, std::unique_ptr&& random_generator, std::unique_ptr process_context); From 641e72b21e810078f5f101bcef48b6bc849b8f67 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 4 Mar 2021 16:52:38 -0500 Subject: [PATCH 23/33] Improve test * check that enableCoreDump() was called * check the logs for the expected warn entry Signed-off-by: Raul Gutierrez Segales --- test/exe/main_common_test.cc | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 604ff80c02b72..f25f23e2a4087 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -136,7 +136,15 @@ TEST_P(MainCommonTest, EnableCoreDump) { } class MockPlatformImpl : public PlatformImpl { - bool enableCoreDump() override { return false; } +public: + MockPlatformImpl(bool& called) : called_(called) {} + bool enableCoreDump() override { + called_ = true; + return false; + } + +private: + bool called_{}; } // Exercise enabling core dump and failing. @@ -149,9 +157,20 @@ TEST_P(MainCommonTest, EnableCoreDumpFails) { const auto args = std::vector( {"envoy-static", "--use-dynamic-base-id", "-c", config_file_.c_str(), "--enable-core-dump"}); OptionsImpl options(args, &MainCommon::hotRestartVersion, spdlog::level::info); - MainCommonBase first(options, real_time_system, default_listener_hooks, prod_component_factory, - std::make_unique(), - std::make_unique(), nullptr); + + bool enable_core_dump_called; + auto test = [&]() { + MainCommonBase first(options, real_time_system, default_listener_hooks, prod_component_factory, + std::make_unique(enable_core_dump_called), + std::make_unique(), nullptr); + }; + + EXPECT_NO_THROW(test()); + EXPECT_TRUE(enable_core_dump_called); + + enable_core_dump_called = false; + EXPECT_LOG_CONTAINS("warn", "failed to enable core dump", test()); + EXPECT_TRUE(enable_core_dump_called); } // Test that an in-use base id triggers a retry and that we eventually give up. From e6abf4b3463a254a70c2d72ddcd375e1d5064c5f Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 4 Mar 2021 17:38:49 -0500 Subject: [PATCH 24/33] Fixes Signed-off-by: Raul Gutierrez Segales --- source/exe/main_common.cc | 4 ++-- source/exe/platform_impl.h | 2 +- test/exe/main_common_test.cc | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 8b2b7e781dde8..57d0396cef5ad 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -48,8 +48,8 @@ MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem std::unique_ptr platform_impl, std::unique_ptr&& random_generator, std::unique_ptr process_context) - : platform_impl_(platform_impl), options_(options), component_factory_(component_factory), - stats_allocator_(symbol_table_) { + : platform_impl_(std::move(platform_impl)), options_(options), + component_factory_(component_factory), stats_allocator_(symbol_table_) { // Process the option to disable extensions as early as possible, // before we do any configuration loading. OptionsImpl::disableExtensions(options.disabledExtensions()); diff --git a/source/exe/platform_impl.h b/source/exe/platform_impl.h index e18e72731ea2a..bb7120aed828d 100644 --- a/source/exe/platform_impl.h +++ b/source/exe/platform_impl.h @@ -8,7 +8,7 @@ namespace Envoy { class PlatformImpl { public: PlatformImpl(); - ~PlatformImpl(); + virtual ~PlatformImpl(); Thread::ThreadFactory& threadFactory() { return *thread_factory_; } Filesystem::Instance& fileSystem() { return *file_system_; } virtual bool enableCoreDump(); diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index f25f23e2a4087..28a8c0acd6431 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -145,7 +145,7 @@ class MockPlatformImpl : public PlatformImpl { private: bool called_{}; -} +}; // Exercise enabling core dump and failing. // Note: this simulates a failed prctl() call. From bdefe7a1342d2d837536af443c8800524f885315 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 4 Mar 2021 18:37:19 -0500 Subject: [PATCH 25/33] Fix format Signed-off-by: Raul Gutierrez Segales --- test/exe/main_common_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 28a8c0acd6431..e6f9d643af7a1 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -129,7 +129,7 @@ TEST_P(MainCommonTest, ConstructWritesBasePathId) { } // Exercise enabling core dump and succeeding. -// Note: this test will call the real prctl(), which is what we want. +// Note: this test will call the real system call, which is what we want. TEST_P(MainCommonTest, EnableCoreDump) { addArg("--enable-core-dump"); VERBOSE_EXPECT_NO_THROW(MainCommon main_common(argc(), argv())); From e9128ddd2b1540981c9d2483cf131ae7491e716c Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 4 Mar 2021 19:03:23 -0500 Subject: [PATCH 26/33] Drop the per architecture match, since it's not needed This probably is enough and we don't need to additionally use `config_settings_group` to group things further. Signed-off-by: Raul Gutierrez Segales --- bazel/envoy_library.bzl | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/bazel/envoy_library.bzl b/bazel/envoy_library.bzl index d81bfb635e4be..6e46c598d47b9 100644 --- a/bazel/envoy_library.bzl +++ b/bazel/envoy_library.bzl @@ -225,18 +225,12 @@ def envoy_cc_posix_without_linux_library(name, srcs = [], hdrs = [], **kargs): name = name + "_posix", srcs = select({ "@envoy//bazel:windows_x86_64": [], - "@envoy//bazel:linux_x86_64": [], - "@envoy//bazel:linux_aarch64": [], - "@envoy//bazel:linux_ppc": [], - "@envoy//bazel:linux_mips64": [], + "@envoy//bazel:linux": [], "//conditions:default": srcs, }), hdrs = select({ "@envoy//bazel:windows_x86_64": [], - "@envoy//bazel:linux_x86_64": [], - "@envoy//bazel:linux_aarch64": [], - "@envoy//bazel:linux_ppc": [], - "@envoy//bazel:linux_mips64": [], + "@envoy//bazel:linux": [], "//conditions:default": hdrs, }), **kargs @@ -247,17 +241,11 @@ def envoy_cc_linux_library(name, srcs = [], hdrs = [], **kargs): envoy_cc_library( name = name + "_linux", srcs = select({ - "@envoy//bazel:linux_x86_64": srcs, - "@envoy//bazel:linux_aarch64": srcs, - "@envoy//bazel:linux_ppc": srcs, - "@envoy//bazel:linux_mips64": srcs, + "@envoy//bazel:linux": srcs, "//conditions:default": [], }), hdrs = select({ - "@envoy//bazel:linux_x86_64": hdrs, - "@envoy//bazel:linux_aarch64": hdrs, - "@envoy//bazel:linux_ppc": hdrs, - "@envoy//bazel:linux_mips64": hdrs, + "@envoy//bazel:linux": hdrs, "//conditions:default": [], }), **kargs From 21da012cbd7b8124d8a4c9dae125d495a83aefc5 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 4 Mar 2021 20:29:44 -0500 Subject: [PATCH 27/33] Fix format Signed-off-by: Raul Gutierrez Segales --- test/exe/main_common_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index e6f9d643af7a1..75cb923795e8b 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -148,7 +148,6 @@ class MockPlatformImpl : public PlatformImpl { }; // Exercise enabling core dump and failing. -// Note: this simulates a failed prctl() call. TEST_P(MainCommonTest, EnableCoreDumpFails) { Event::TestRealTimeSystem real_time_system; DefaultListenerHooks default_listener_hooks; From 583bf9e442aac571c5171da8794e25f72b6c0127 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Fri, 5 Mar 2021 07:33:59 -0500 Subject: [PATCH 28/33] Fix Signed-off-by: Raul Gutierrez Segales --- source/exe/main_common.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 57d0396cef5ad..052ab0ecda8ba 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -197,8 +197,8 @@ void MainCommonBase::adminRequest(absl::string_view path_and_query, absl::string MainCommon::MainCommon(const std::vector& args) : options_(args, &MainCommon::hotRestartVersion, spdlog::level::info), base_(options_, real_time_system_, default_listener_hooks_, prod_component_factory_, - std::make_unique(), platform_impl_.threadFactory(), - platform_impl_.fileSystem(), nullptr) {} + std::make_unique(), std::make_unique(), + nullptr) {} MainCommon::MainCommon(int argc, const char* const* argv) : options_(argc, argv, &MainCommon::hotRestartVersion, spdlog::level::info), From 938e4dd2e3dca605143756da6444007033916e39 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Fri, 5 Mar 2021 09:20:37 -0500 Subject: [PATCH 29/33] Fix test Keep ref instead of copying. Signed-off-by: Raul Gutierrez Segales --- test/exe/main_common_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 75cb923795e8b..18e9b19c8764a 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -144,7 +144,7 @@ class MockPlatformImpl : public PlatformImpl { } private: - bool called_{}; + bool& called_; }; // Exercise enabling core dump and failing. From fcb514c12f2ff7800fefba55cb3480588414e609 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Fri, 5 Mar 2021 09:31:40 -0500 Subject: [PATCH 30/33] prctl() will only work on Linux, so use an ifdef Signed-off-by: Raul Gutierrez Segales --- test/exe/main_common_test.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 18e9b19c8764a..e932099ab7eed 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -131,8 +131,11 @@ TEST_P(MainCommonTest, ConstructWritesBasePathId) { // Exercise enabling core dump and succeeding. // Note: this test will call the real system call, which is what we want. TEST_P(MainCommonTest, EnableCoreDump) { +#ifdef __linux__ addArg("--enable-core-dump"); - VERBOSE_EXPECT_NO_THROW(MainCommon main_common(argc(), argv())); + auto test = [&]() { MainCommon main_common(argc(), argv()); }; + EXPECT_LOG_CONTAINS("info", "core dump enabled", test()); +#endif } class MockPlatformImpl : public PlatformImpl { From 37f19aa914562c7bc790685a2869f0d8b99e362b Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Fri, 5 Mar 2021 09:33:31 -0500 Subject: [PATCH 31/33] Be more consistent in the use of ifdefs Signed-off-by: Raul Gutierrez Segales --- test/exe/platform_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/exe/platform_impl_test.cc b/test/exe/platform_impl_test.cc index 0d3fdc541f13a..955a55710eb84 100644 --- a/test/exe/platform_impl_test.cc +++ b/test/exe/platform_impl_test.cc @@ -11,7 +11,7 @@ namespace Envoy { TEST(PlatformImpl, Basic) { PlatformImpl platform; -#if !defined(WIN32) && !defined(__APPLE__) +#ifdef __linux__ EXPECT_EQ(true, platform.enableCoreDump()); #else EXPECT_EQ(false, platform.enableCoreDump()); From b0026f4a8628fcf5a9ed3972389f60c34e3306c9 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Fri, 5 Mar 2021 11:04:18 -0500 Subject: [PATCH 32/33] Fix clang tidy Signed-off-by: Raul Gutierrez Segales --- test/exe/main_common_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index e932099ab7eed..43cc3360bab84 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -157,7 +157,7 @@ TEST_P(MainCommonTest, EnableCoreDumpFails) { ProdComponentFactory prod_component_factory; const auto args = std::vector( - {"envoy-static", "--use-dynamic-base-id", "-c", config_file_.c_str(), "--enable-core-dump"}); + {"envoy-static", "--use-dynamic-base-id", "-c", config_file_, "--enable-core-dump"}); OptionsImpl options(args, &MainCommon::hotRestartVersion, spdlog::level::info); bool enable_core_dump_called; From 0790c5e39637405f4eebb2c4a2ae657e96d8f7fd Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Fri, 5 Mar 2021 13:20:53 -0500 Subject: [PATCH 33/33] Use a proper mock method Signed-off-by: Raul Gutierrez Segales --- test/exe/main_common_test.cc | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 43cc3360bab84..672c05b9282bb 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -140,14 +140,7 @@ TEST_P(MainCommonTest, EnableCoreDump) { class MockPlatformImpl : public PlatformImpl { public: - MockPlatformImpl(bool& called) : called_(called) {} - bool enableCoreDump() override { - called_ = true; - return false; - } - -private: - bool& called_; + MOCK_METHOD(bool, enableCoreDump, ()); }; // Exercise enabling core dump and failing. @@ -160,19 +153,16 @@ TEST_P(MainCommonTest, EnableCoreDumpFails) { {"envoy-static", "--use-dynamic-base-id", "-c", config_file_, "--enable-core-dump"}); OptionsImpl options(args, &MainCommon::hotRestartVersion, spdlog::level::info); - bool enable_core_dump_called; auto test = [&]() { + auto* platform_impl = new NiceMock(); + EXPECT_CALL(*platform_impl, enableCoreDump()).WillOnce(Return(false)); MainCommonBase first(options, real_time_system, default_listener_hooks, prod_component_factory, - std::make_unique(enable_core_dump_called), + std::unique_ptr{platform_impl}, std::make_unique(), nullptr); }; EXPECT_NO_THROW(test()); - EXPECT_TRUE(enable_core_dump_called); - - enable_core_dump_called = false; EXPECT_LOG_CONTAINS("warn", "failed to enable core dump", test()); - EXPECT_TRUE(enable_core_dump_called); } // Test that an in-use base id triggers a retry and that we eventually give up.