From cfe39038b3a73303a89144b3f48521166c5da4cc Mon Sep 17 00:00:00 2001 From: ohadvano Date: Tue, 9 May 2023 16:56:44 +0300 Subject: [PATCH 01/30] add API option Signed-off-by: ohadvano --- api/envoy/config/bootstrap/v3/bootstrap.proto | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index 6b230536cd86e..a37ee564de3b0 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -41,7 +41,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // ` for more detail. // Bootstrap :ref:`configuration overview `. -// [#next-free-field: 38] +// [#next-free-field: 39] message Bootstrap { option (udpa.annotations.versioning).previous_message_type = "envoy.config.bootstrap.v2.Bootstrap"; @@ -101,6 +101,15 @@ message Bootstrap { core.v3.ApiConfigSource ads_config = 3; } + message ApplicationLogFormat { + oneof log_format { + // Flush application logs in JSON format. If set, it will override Envoy's --log-format + // CLI option. The configured JSON struct can support all the format tags specified in + // the --log-format section, in :ref:`operations CLI `. + google.protobuf.Struct json_format = 1; + } + } + reserved 10, 11; reserved "runtime"; @@ -360,6 +369,8 @@ message Bootstrap { // Envoy only supports ListenerManager for this field and Envoy Mobile // supports ApiListenerManager. core.v3.TypedExtensionConfig listener_manager = 37; + + ApplicationLogFormat application_log_format = 38; } // Administration interface :ref:`operations documentation From e848556a908a30d879982619c343264c66058155 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 11 May 2023 10:38:17 +0300 Subject: [PATCH 02/30] initial implementation to set JSON format from struct Signed-off-by: ohadvano --- source/common/common/logger.cc | 17 +++++++++++++++++ source/common/common/logger.h | 6 ++++++ source/server/server.cc | 5 +++++ 3 files changed, 28 insertions(+) diff --git a/source/common/common/logger.cc b/source/common/common/logger.cc index 9b02e67496605..07260c8ff0c4d 100644 --- a/source/common/common/logger.cc +++ b/source/common/common/logger.cc @@ -6,6 +6,8 @@ #include #include +#include "envoy/common/exception.h" + #include "source/common/common/json_escape_string.h" #include "source/common/common/lock_guard.h" @@ -254,6 +256,21 @@ void Registry::setLogFormat(const std::string& log_format) { } } +void Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { + Protobuf::util::JsonPrintOptions json_options; + json_options.preserve_proto_field_names = true; + json_options.always_print_primitive_fields = true; + + std::string format_as_json; + if (auto status = + Protobuf::util::MessageToJsonString(log_format_struct, &format_as_json, json_options); + !status.ok()) { + throw EnvoyException("failed to set log format as JSON string from struct"); + } + + setLogFormat(format_as_json); +} + Logger* Registry::logger(const std::string& log_name) { Logger* logger_to_return = nullptr; for (Logger& logger : loggers()) { diff --git a/source/common/common/logger.h b/source/common/common/logger.h index 7d7b86f29f2cf..d752dd2d915bd 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -15,6 +15,7 @@ #include "source/common/common/logger_impl.h" #include "source/common/common/macros.h" #include "source/common/common/non_copyable.h" +#include "source/common/protobuf/protobuf.h" #include "absl/container/flat_hash_map.h" #include "absl/strings/string_view.h" @@ -353,6 +354,11 @@ class Registry { */ static void setLogFormat(const std::string& log_format); + /** + * Sets the log format from a struct as a JSON string. + */ + static void setJsonLogFormat(const Protobuf::Message& log_format_struct); + /** * @return std::vector& the installed loggers. */ diff --git a/source/server/server.cc b/source/server/server.cc index 1e2feade47456..2d36fb47a26e1 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -425,6 +425,11 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add messageValidationContext().staticValidationVisitor(), *api_); bootstrap_config_update_time_ = time_source_.systemTime(); + if (bootstrap_.has_application_log_format() && + bootstrap_.application_log_format().has_json_format()) { + Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); + } + #ifdef ENVOY_PERFETTO perfetto::TracingInitArgs args; // Include in-process events only. From ea9dca4ee7c8afbe7889543f3dfab1d81d0b5c07 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 11 May 2023 10:53:02 +0300 Subject: [PATCH 03/30] add message to error Signed-off-by: ohadvano --- source/common/common/BUILD | 1 + source/common/common/logger.cc | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 6f0f7a885ee8c..43cf861af2fd0 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -211,6 +211,7 @@ envoy_cc_library( ":lock_guard_lib", ":macros", ":non_copyable", + "//source/common/protobuf:protobuf", ] + select({ "//bazel:android_logger": ["logger_impl_lib_android"], "//conditions:default": ["logger_impl_lib_standard"], diff --git a/source/common/common/logger.cc b/source/common/common/logger.cc index 07260c8ff0c4d..9cbb6c71847d0 100644 --- a/source/common/common/logger.cc +++ b/source/common/common/logger.cc @@ -265,7 +265,8 @@ void Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { if (auto status = Protobuf::util::MessageToJsonString(log_format_struct, &format_as_json, json_options); !status.ok()) { - throw EnvoyException("failed to set log format as JSON string from struct"); + throw EnvoyException( + fmt::format("failed to set log format as JSON string from struct: {}", status.ToString())); } setLogFormat(format_as_json); From 83366a05c173581aecba3f6155a17e4cfd2721eb Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 11 May 2023 12:48:58 +0300 Subject: [PATCH 04/30] update docs Signed-off-by: ohadvano --- api/envoy/config/bootstrap/v3/bootstrap.proto | 10 +++++--- .../observability/application_logging.rst | 23 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index a37ee564de3b0..e9a6cf8a08684 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -103,9 +103,11 @@ message Bootstrap { message ApplicationLogFormat { oneof log_format { - // Flush application logs in JSON format. If set, it will override Envoy's --log-format - // CLI option. The configured JSON struct can support all the format tags specified in - // the --log-format section, in :ref:`operations CLI `. + option (validate.required) = true; + + // Flush application logs in JSON format. The configured JSON struct can + // support all the format tags specified in the --log-format section, + // in :ref:`operations CLI `. google.protobuf.Struct json_format = 1; } } @@ -370,6 +372,8 @@ message Bootstrap { // supports ApiListenerManager. core.v3.TypedExtensionConfig listener_manager = 37; + // Optional field to set the application logs format. If this field is set, it will override + // the default log format, or the format that is specified by the CLI option ``--log-foramt``. ApplicationLogFormat application_log_format = 38; } diff --git a/docs/root/configuration/observability/application_logging.rst b/docs/root/configuration/observability/application_logging.rst index 7434d2cd5791c..7603680e4e7cd 100644 --- a/docs/root/configuration/observability/application_logging.rst +++ b/docs/root/configuration/observability/application_logging.rst @@ -23,3 +23,26 @@ with the following :ref:`command line options `: * The ``--log-level`` flag can be set to control the log severity logged to Stackdriver. `Reference documentation `_ for Stackdriver on GKE. + +Printing logs in JSON format +---------------------------- + +It is possible to use the bootstrap config :ref:`json_format ` +to print the logs in custom JSON format. The json format struct can support all the format flags that are specified in :ref:`command line options `. Example: + +.. code-block:: yaml + + application_log_format: + json_format: + Timestamp: "%Y-%m-%dT%T.%F" + ThreadId: "%t" + SourceLine: "%s:%#" + Level: "%l" + Message: "%_" + FixedValue: "SomeFixedValue" + +.. note:: + The JSON log format will be applied only after the bootstrap config initialization. + Therefore, some of the logs will be printed in the default log format, or the one that is specified by the CLI option ``--log-format``. + For full JSON application logs experience, it is possible to set ``--log-format`` with JSON format, so both logs before bootstrap initialization and after, are written in JSON format. + For example: ``--log-format '{"Timestamp":"%Y-%m-%dT%T.%F","ThreadId":"%t","SourceLine":"%s:%#","Level":"%l","Message":"%_"}'. From 610f00175a991e98b911e8fd4d98446cd3aed7ed Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 11 May 2023 14:29:23 +0300 Subject: [PATCH 05/30] fix doc Signed-off-by: ohadvano --- .../configuration/observability/application_logging.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/root/configuration/observability/application_logging.rst b/docs/root/configuration/observability/application_logging.rst index 7603680e4e7cd..ecd0ca1113ee1 100644 --- a/docs/root/configuration/observability/application_logging.rst +++ b/docs/root/configuration/observability/application_logging.rst @@ -42,7 +42,8 @@ to print the logs in custom JSON format. The json format struct can support all FixedValue: "SomeFixedValue" .. note:: - The JSON log format will be applied only after the bootstrap config initialization. - Therefore, some of the logs will be printed in the default log format, or the one that is specified by the CLI option ``--log-format``. - For full JSON application logs experience, it is possible to set ``--log-format`` with JSON format, so both logs before bootstrap initialization and after, are written in JSON format. - For example: ``--log-format '{"Timestamp":"%Y-%m-%dT%T.%F","ThreadId":"%t","SourceLine":"%s:%#","Level":"%l","Message":"%_"}'. + The JSON log format will be applied only after the bootstrap config initialization. + Therefore, some of the logs will be printed in the default log format, or the one that is specified by the CLI option ``--log-format``. + For full JSON application logs experience, it is possible to set ``--log-format`` with JSON format, + so both logs before bootstrap initialization and after, are written in JSON format. + For example: ``--log-format '{"Timestamp":"%Y-%m-%dT%T.%F","ThreadId":"%t","SourceLine":"%s:%#","Level":"%l","Message":"%_"}'``. \ No newline at end of file From 3608affc259a69fa866c05514d5bfc276ef369ab Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 11 May 2023 14:36:30 +0300 Subject: [PATCH 06/30] end of doc newline Signed-off-by: ohadvano --- docs/root/configuration/observability/application_logging.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/configuration/observability/application_logging.rst b/docs/root/configuration/observability/application_logging.rst index ecd0ca1113ee1..dc048fa0ea480 100644 --- a/docs/root/configuration/observability/application_logging.rst +++ b/docs/root/configuration/observability/application_logging.rst @@ -46,4 +46,4 @@ to print the logs in custom JSON format. The json format struct can support all Therefore, some of the logs will be printed in the default log format, or the one that is specified by the CLI option ``--log-format``. For full JSON application logs experience, it is possible to set ``--log-format`` with JSON format, so both logs before bootstrap initialization and after, are written in JSON format. - For example: ``--log-format '{"Timestamp":"%Y-%m-%dT%T.%F","ThreadId":"%t","SourceLine":"%s:%#","Level":"%l","Message":"%_"}'``. \ No newline at end of file + For example: ``--log-format '{"Timestamp":"%Y-%m-%dT%T.%F","ThreadId":"%t","SourceLine":"%s:%#","Level":"%l","Message":"%_"}'``. From a45bccdda4ef2e8859f56bdcfc3882c7834e4c3b Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 11 May 2023 16:49:28 +0300 Subject: [PATCH 07/30] fix docs, move status check to server Signed-off-by: ohadvano --- api/envoy/config/bootstrap/v3/bootstrap.proto | 6 +++--- source/common/common/logger.cc | 15 ++++++--------- source/common/common/logger.h | 2 +- source/server/server.cc | 7 ++++++- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index e9a6cf8a08684..bd8a378a699f5 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -106,8 +106,8 @@ message Bootstrap { option (validate.required) = true; // Flush application logs in JSON format. The configured JSON struct can - // support all the format tags specified in the --log-format section, - // in :ref:`operations CLI `. + // support all the format tags specified in the in :option:`--log-format` + // command line option section. google.protobuf.Struct json_format = 1; } } @@ -373,7 +373,7 @@ message Bootstrap { core.v3.TypedExtensionConfig listener_manager = 37; // Optional field to set the application logs format. If this field is set, it will override - // the default log format, or the format that is specified by the CLI option ``--log-foramt``. + // the default log format, or the format that is specified by the :option:`--log-format` command line option. ApplicationLogFormat application_log_format = 38; } diff --git a/source/common/common/logger.cc b/source/common/common/logger.cc index 9cbb6c71847d0..ac5bf1afe0469 100644 --- a/source/common/common/logger.cc +++ b/source/common/common/logger.cc @@ -6,8 +6,6 @@ #include #include -#include "envoy/common/exception.h" - #include "source/common/common/json_escape_string.h" #include "source/common/common/lock_guard.h" @@ -256,20 +254,19 @@ void Registry::setLogFormat(const std::string& log_format) { } } -void Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { +ProtobufUtil::Status Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { Protobuf::util::JsonPrintOptions json_options; json_options.preserve_proto_field_names = true; json_options.always_print_primitive_fields = true; std::string format_as_json; - if (auto status = - Protobuf::util::MessageToJsonString(log_format_struct, &format_as_json, json_options); - !status.ok()) { - throw EnvoyException( - fmt::format("failed to set log format as JSON string from struct: {}", status.ToString())); + auto status = + Protobuf::util::MessageToJsonString(log_format_struct, &format_as_json, json_options); + if (status.ok()) { + setLogFormat(format_as_json); } - setLogFormat(format_as_json); + return status; } Logger* Registry::logger(const std::string& log_name) { diff --git a/source/common/common/logger.h b/source/common/common/logger.h index d752dd2d915bd..23c96cb896123 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -357,7 +357,7 @@ class Registry { /** * Sets the log format from a struct as a JSON string. */ - static void setJsonLogFormat(const Protobuf::Message& log_format_struct); + static ProtobufUtil::Status setJsonLogFormat(const Protobuf::Message& log_format_struct); /** * @return std::vector& the installed loggers. diff --git a/source/server/server.cc b/source/server/server.cc index 2d36fb47a26e1..b60b9ed74a41a 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -427,7 +427,12 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add if (bootstrap_.has_application_log_format() && bootstrap_.application_log_format().has_json_format()) { - Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); + auto status = + Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); + if (!status.ok()) { + throw EnvoyException(fmt::format("failed to set log format as JSON string from struct: {}", + status.ToString())); + } } #ifdef ENVOY_PERFETTO From 51c692f83749db2b46469700dc6534d11b52c910 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 11 May 2023 17:07:49 +0300 Subject: [PATCH 08/30] noop Signed-off-by: ohadvano --- source/server/server.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/server/server.cc b/source/server/server.cc index b60b9ed74a41a..9342c68f7faac 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -842,6 +842,7 @@ void InstanceImpl::startWorkers() { }); } + Runtime::LoaderPtr InstanceUtil::createRuntime(Instance& server, Server::Configuration::Initial& config) { #ifdef ENVOY_ENABLE_YAML From ff884e08106f9e1b3eb9a45ce213e802d587ed5e Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 11 May 2023 17:07:56 +0300 Subject: [PATCH 09/30] noop Signed-off-by: ohadvano --- source/server/server.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/server/server.cc b/source/server/server.cc index 9342c68f7faac..b60b9ed74a41a 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -842,7 +842,6 @@ void InstanceImpl::startWorkers() { }); } - Runtime::LoaderPtr InstanceUtil::createRuntime(Instance& server, Server::Configuration::Initial& config) { #ifdef ENVOY_ENABLE_YAML From 5e31eac4c643ba3c83a1792799df712616907891 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 11 May 2023 17:16:34 +0300 Subject: [PATCH 10/30] add application log to config validation Signed-off-by: ohadvano --- source/server/config_validation/server.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index c836e4cd1d1ae..c79024b046e5c 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -85,6 +85,16 @@ void ValidationInstance::initialize(const Options& options, InstanceUtil::loadBootstrapConfig(bootstrap_, options, messageValidationContext().staticValidationVisitor(), *api_); + if (bootstrap_.has_application_log_format() && + bootstrap_.application_log_format().has_json_format()) { + auto status = + Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); + if (!status.ok()) { + throw EnvoyException(fmt::format("failed to set log format as JSON string from struct: {}", + status.ToString())); + } + } + // Inject regex engine to singleton. Regex::EnginePtr regex_engine = createRegexEngine( bootstrap_, messageValidationContext().staticValidationVisitor(), serverFactoryContext()); From 56e80d1c121d27011900623ee7912a996419e282 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 12 May 2023 00:42:38 +0300 Subject: [PATCH 11/30] add tests Signed-off-by: ohadvano --- test/common/common/logger_test.cc | 68 +++++++++++++++++++ test/server/config_validation/server_test.cc | 58 ++++++++++++++-- .../test_data/json_application_logs.yaml | 10 +++ test/server/server_test.cc | 23 +++++++ .../server/json_application_log.yaml | 10 +++ 5 files changed, 163 insertions(+), 6 deletions(-) create mode 100644 test/server/config_validation/test_data/json_application_logs.yaml create mode 100644 test/server/test_data/server/json_application_log.yaml diff --git a/test/common/common/logger_test.cc b/test/common/common/logger_test.cc index b026eddc6419b..3be4d427fda8c 100644 --- a/test/common/common/logger_test.cc +++ b/test/common/common/logger_test.cc @@ -270,6 +270,74 @@ TEST(LoggerTest, LogWithLogDetails) { ENVOY_LOG_MISC(info, "hello"); } +TEST(LoggerTest, TestJsonFormatError) { + ProtobufWkt::Any log_struct; + log_struct.set_type_url("type.googleapis.com/bad.type.url"); + log_struct.set_value("asdf"); + + // This scenario shouldn't happen in production, the test is added mainly for coverage. + auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_FALSE(status.ok()); +} + +TEST(LoggerTest, TestJsonFormatEmptyStruct) { + ProtobufWkt::Struct log_struct; + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + + auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_TRUE(status.ok()); + + MockLogSink sink(Envoy::Logger::Registry::getSink()); + EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_EQ(msg, "{}\n"); + EXPECT_EQ(log.logger_name, "misc"); + })); + + ENVOY_LOG_MISC(info, "hello"); +} + +TEST(LoggerTest, TestJsonFormatNullField) { + ProtobufWkt::Struct log_struct; + (*log_struct.mutable_fields())["Message"].set_string_value("%v"); + (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + + auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_TRUE(status.ok()); + + MockLogSink sink(Envoy::Logger::Registry::getSink()); + EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto&) { + EXPECT_THAT(msg, HasSubstr("\"Message\":\"hello\"")); + EXPECT_THAT(msg, HasSubstr("\"NullField\":null")); + })); + + ENVOY_LOG_MISC(info, "hello"); +} + +TEST(LoggerTest, TestJsonFormat) { + ProtobufWkt::Struct log_struct; + (*log_struct.mutable_fields())["Level"].set_string_value("%l"); + (*log_struct.mutable_fields())["Message"].set_string_value("%v"); + (*log_struct.mutable_fields())["FixedValue"].set_string_value("Fixed"); + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + + auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_TRUE(status.ok()); + + MockLogSink sink(Envoy::Logger::Registry::getSink()); + EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_THAT(msg, HasSubstr("\"Level\":\"info\"")); + EXPECT_THAT(msg, HasSubstr("\"Message\":\"hello\"")); + EXPECT_THAT(msg, HasSubstr("\"FixedValue\":\"Fixed\"")); + EXPECT_EQ(msg[0], '{'); + EXPECT_EQ(msg[msg.size() - 2], '}'); + + EXPECT_EQ(log.logger_name, "misc"); + })); + + ENVOY_LOG_MISC(info, "hello"); +} + } // namespace } // namespace Logger } // namespace Envoy diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index d52e83202b07d..86f4f55afc74e 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -73,14 +73,24 @@ class RuntimeFeatureValidationServerTest : public ValidationServerTest { static const std::vector getAllConfigFiles() { setupTestDirectory(); + return {"runtime_config.yaml"}; + } +}; - auto files = TestUtility::listFiles(ValidationServerTest::directory_, false); +class JsonApplicationLogsValidationServerTest : public ValidationServerTest { +public: + static void SetUpTestSuite() { // NOLINT(readability-identifier-naming) + setupTestDirectory(); + } - // Strip directory part. options_ adds it for each test. - for (auto& file : files) { - file = file.substr(directory_.length() + 1); - } - return files; + static void setupTestDirectory() { + directory_ = + TestEnvironment::runfilesDirectory("envoy/test/server/config_validation/test_data/"); + } + + static const std::vector getAllConfigFiles() { + setupTestDirectory(); + return {"json_application_logs.yaml"}; } }; @@ -206,6 +216,42 @@ INSTANTIATE_TEST_SUITE_P( AllConfigs, RuntimeFeatureValidationServerTest, ::testing::ValuesIn(RuntimeFeatureValidationServerTest::getAllConfigFiles())); +struct MockLogSink : Logger::SinkDelegate { + MockLogSink(Logger::DelegatingLogSinkSharedPtr log_sink) : Logger::SinkDelegate(log_sink) { + setDelegate(); + } + ~MockLogSink() override { restoreDelegate(); } + + MOCK_METHOD(void, log, (absl::string_view, const spdlog::details::log_msg&)); + MOCK_METHOD(void, logWithStableName, + (absl::string_view, absl::string_view, absl::string_view, absl::string_view)); + void flush() override {} +}; + +TEST_P(JsonApplicationLogsValidationServerTest, ValidateJsonApplicationLogs) { + Thread::MutexBasicLockable access_log_lock; + Stats::IsolatedStoreImpl stats_store; + DangerousDeprecatedTestTime time_system; + ValidationInstance server(options_, time_system.timeSystem(), + Network::Address::InstanceConstSharedPtr(), stats_store, + access_log_lock, component_factory_, Thread::threadFactoryForTest(), + Filesystem::fileSystemForTest()); + + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + MockLogSink sink(Envoy::Logger::Registry::getSink()); + EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_EQ(msg, "{\"MessageFromProto\":\"hello\"}\n"); + EXPECT_EQ(log.logger_name, "misc"); + })); + + ENVOY_LOG_MISC(info, "hello"); + server.shutdown(); +} + +INSTANTIATE_TEST_SUITE_P( + AllConfigs, JsonApplicationLogsValidationServerTest, + ::testing::ValuesIn(JsonApplicationLogsValidationServerTest::getAllConfigFiles())); + } // namespace } // namespace Server } // namespace Envoy diff --git a/test/server/config_validation/test_data/json_application_logs.yaml b/test/server/config_validation/test_data/json_application_logs.yaml new file mode 100644 index 0000000000000..132cf9d06716f --- /dev/null +++ b/test/server/config_validation/test_data/json_application_logs.yaml @@ -0,0 +1,10 @@ +--- +application_log_format: + json_format: + MessageFromProto: "%v" + +admin: + address: + socket_address: + address: 0.0.0.0 + port_value: 9000 diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 59cbd75f3644f..f3ec78b9ba9ed 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1624,6 +1624,29 @@ TEST_P(ServerInstanceImplTest, AdminAccessLogFilter) { EXPECT_NO_THROW(initialize("test/server/test_data/server/access_log_filter_bootstrap.yaml")); } +struct MockLogSink : Logger::SinkDelegate { + MockLogSink(Logger::DelegatingLogSinkSharedPtr log_sink) : Logger::SinkDelegate(log_sink) { setDelegate(); } + ~MockLogSink() override { restoreDelegate(); } + + MOCK_METHOD(void, log, (absl::string_view, const spdlog::details::log_msg&)); + MOCK_METHOD(void, logWithStableName, + (absl::string_view, absl::string_view, absl::string_view, absl::string_view)); + void flush() override {} +}; + +TEST_P(ServerInstanceImplTest, JsonApplicationLog) { + EXPECT_NO_THROW(initialize("test/server/test_data/server/json_application_log.yaml")); + + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + MockLogSink sink(Envoy::Logger::Registry::getSink()); + EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_EQ(msg, "{\"MessageFromProto\":\"hello\"}\n"); + EXPECT_EQ(log.logger_name, "misc"); + })); + + ENVOY_LOG_MISC(info, "hello"); +} + } // namespace } // namespace Server } // namespace Envoy diff --git a/test/server/test_data/server/json_application_log.yaml b/test/server/test_data/server/json_application_log.yaml new file mode 100644 index 0000000000000..132cf9d06716f --- /dev/null +++ b/test/server/test_data/server/json_application_log.yaml @@ -0,0 +1,10 @@ +--- +application_log_format: + json_format: + MessageFromProto: "%v" + +admin: + address: + socket_address: + address: 0.0.0.0 + port_value: 9000 From 8002a8400a5e91a8bbb18c24db728cd5fa66cd77 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 12 May 2023 00:58:39 +0300 Subject: [PATCH 12/30] move MockLogSink to common mocks Signed-off-by: ohadvano --- test/common/common/logger_test.cc | 12 +----------- test/mocks/common.h | 12 ++++++++++++ test/server/config_validation/server_test.cc | 13 +------------ test/server/server_test.cc | 11 +---------- 4 files changed, 15 insertions(+), 33 deletions(-) diff --git a/test/common/common/logger_test.cc b/test/common/common/logger_test.cc index 3be4d427fda8c..1667af669d77e 100644 --- a/test/common/common/logger_test.cc +++ b/test/common/common/logger_test.cc @@ -4,6 +4,7 @@ #include "source/common/common/json_escape_string.h" #include "source/common/common/logger.h" +#include "test/mocks/common.h" #include "test/test_common/environment.h" #include "gmock/gmock.h" @@ -176,17 +177,6 @@ TEST_P(LoggerCustomFlagsTest, LogMessageAsJsonStringEscaped) { "StreamAggregatedResources gRPC config stream closed: 14, connection error: desc = " "\\\"transport: Error while dialing dial tcp [::1]:15012: connect: connection refused\\\""); } - -struct MockLogSink : SinkDelegate { - MockLogSink(DelegatingLogSinkSharedPtr log_sink) : SinkDelegate(log_sink) { setDelegate(); } - ~MockLogSink() override { restoreDelegate(); } - - MOCK_METHOD(void, log, (absl::string_view, const spdlog::details::log_msg&)); - MOCK_METHOD(void, logWithStableName, - (absl::string_view, absl::string_view, absl::string_view, absl::string_view)); - void flush() override {} -}; - class NamedLogTest : public Loggable, public testing::Test {}; TEST_F(NamedLogTest, NamedLogsAreSentToSink) { diff --git a/test/mocks/common.h b/test/mocks/common.h index 058a7736af69f..1e496030e4f9b 100644 --- a/test/mocks/common.h +++ b/test/mocks/common.h @@ -143,4 +143,16 @@ class MockKeyValueStoreFactory : public KeyValueStoreFactory { std::string name() const override { return "mock_key_value_store_factory"; } }; +struct MockLogSink : Logger::SinkDelegate { + MockLogSink(Logger::DelegatingLogSinkSharedPtr log_sink) : Logger::SinkDelegate(log_sink) { + setDelegate(); + } + ~MockLogSink() override { restoreDelegate(); } + + MOCK_METHOD(void, log, (absl::string_view, const spdlog::details::log_msg&)); + MOCK_METHOD(void, logWithStableName, + (absl::string_view, absl::string_view, absl::string_view, absl::string_view)); + void flush() override {} +}; + } // namespace Envoy diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index 86f4f55afc74e..43c2b91595c88 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -6,6 +6,7 @@ #include "source/server/config_validation/server.h" #include "test/integration/server.h" +#include "test/mocks/common.h" #include "test/mocks/network/mocks.h" #include "test/mocks/server/options.h" #include "test/mocks/stats/mocks.h" @@ -216,18 +217,6 @@ INSTANTIATE_TEST_SUITE_P( AllConfigs, RuntimeFeatureValidationServerTest, ::testing::ValuesIn(RuntimeFeatureValidationServerTest::getAllConfigFiles())); -struct MockLogSink : Logger::SinkDelegate { - MockLogSink(Logger::DelegatingLogSinkSharedPtr log_sink) : Logger::SinkDelegate(log_sink) { - setDelegate(); - } - ~MockLogSink() override { restoreDelegate(); } - - MOCK_METHOD(void, log, (absl::string_view, const spdlog::details::log_msg&)); - MOCK_METHOD(void, logWithStableName, - (absl::string_view, absl::string_view, absl::string_view, absl::string_view)); - void flush() override {} -}; - TEST_P(JsonApplicationLogsValidationServerTest, ValidateJsonApplicationLogs) { Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; diff --git a/test/server/server_test.cc b/test/server/server_test.cc index f3ec78b9ba9ed..d063002c6e1a0 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -20,6 +20,7 @@ #include "test/common/stats/stat_test_utility.h" #include "test/config/v2_link_hacks.h" #include "test/integration/server.h" +#include "test/mocks/common.h" #include "test/mocks/server/bootstrap_extension_factory.h" #include "test/mocks/server/fatal_action_factory.h" #include "test/mocks/server/hot_restart.h" @@ -1624,16 +1625,6 @@ TEST_P(ServerInstanceImplTest, AdminAccessLogFilter) { EXPECT_NO_THROW(initialize("test/server/test_data/server/access_log_filter_bootstrap.yaml")); } -struct MockLogSink : Logger::SinkDelegate { - MockLogSink(Logger::DelegatingLogSinkSharedPtr log_sink) : Logger::SinkDelegate(log_sink) { setDelegate(); } - ~MockLogSink() override { restoreDelegate(); } - - MOCK_METHOD(void, log, (absl::string_view, const spdlog::details::log_msg&)); - MOCK_METHOD(void, logWithStableName, - (absl::string_view, absl::string_view, absl::string_view, absl::string_view)); - void flush() override {} -}; - TEST_P(ServerInstanceImplTest, JsonApplicationLog) { EXPECT_NO_THROW(initialize("test/server/test_data/server/json_application_log.yaml")); From fcde3b584339d790e9468a91fd0804c009a076c2 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 12 May 2023 11:25:11 +0300 Subject: [PATCH 13/30] reduce coverage percentage Signed-off-by: ohadvano --- test/per_file_coverage.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index c3c94ccd8be37..1d76643a247c1 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -75,7 +75,7 @@ declare -a KNOWN_LOW_COVERAGE=( "source/server:93.8" # flaky: be careful adjusting. See https://github.com/envoyproxy/envoy/issues/15239 "source/server/admin:profiler-lib:83" "source/extensions/load_balancing_policies/common:94" # Death tests don't report LCOV -"source/server/config_validation:88.2" +"source/server/config_validation:87.3" "source/extensions/health_checkers:95.9" "source/extensions/health_checkers/http:93.8" "source/extensions/health_checkers/grpc:92.0" From 8906ffc5b2b15541395170df7a84a9ddf9d0a0d5 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 12 May 2023 11:41:32 +0300 Subject: [PATCH 14/30] change test check Signed-off-by: ohadvano --- test/server/config_validation/server_test.cc | 2 +- test/server/server_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index 43c2b91595c88..4a00296cf3c64 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -229,7 +229,7 @@ TEST_P(JsonApplicationLogsValidationServerTest, ValidateJsonApplicationLogs) { Envoy::Logger::Registry::setLogLevel(spdlog::level::info); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { - EXPECT_EQ(msg, "{\"MessageFromProto\":\"hello\"}\n"); + EXPECT_THAT(msg, HasSubstr("{\"MessageFromProto\":\"hello\"}")); EXPECT_EQ(log.logger_name, "misc"); })); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index d063002c6e1a0..9c6c5dcff920e 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1631,7 +1631,7 @@ TEST_P(ServerInstanceImplTest, JsonApplicationLog) { Envoy::Logger::Registry::setLogLevel(spdlog::level::info); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { - EXPECT_EQ(msg, "{\"MessageFromProto\":\"hello\"}\n"); + EXPECT_THAT(msg, HasSubstr("{\"MessageFromProto\":\"hello\"}")); EXPECT_EQ(log.logger_name, "misc"); })); From 9e498dfd3af85dc0b258c61f5e3a7a64d9a6dc87 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 12 May 2023 11:44:47 +0300 Subject: [PATCH 15/30] add using Signed-off-by: ohadvano --- test/server/config_validation/server_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index 4a00296cf3c64..8278e77420a56 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -15,6 +15,8 @@ #include "test/test_common/registry.h" #include "test/test_common/test_time.h" +using testing::HasSubstr; + namespace Envoy { namespace Server { namespace { From 4a2495ad3554e3cab9110bc835e7bd04788f94b9 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 12 May 2023 12:27:26 +0300 Subject: [PATCH 16/30] fix test Signed-off-by: ohadvano --- test/common/common/logger_test.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/common/common/logger_test.cc b/test/common/common/logger_test.cc index 1667af669d77e..e02aca85a4fd2 100644 --- a/test/common/common/logger_test.cc +++ b/test/common/common/logger_test.cc @@ -279,7 +279,7 @@ TEST(LoggerTest, TestJsonFormatEmptyStruct) { MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { - EXPECT_EQ(msg, "{}\n"); + EXPECT_THAT(msg, HasSubstr("{}")); EXPECT_EQ(log.logger_name, "misc"); })); @@ -319,9 +319,6 @@ TEST(LoggerTest, TestJsonFormat) { EXPECT_THAT(msg, HasSubstr("\"Level\":\"info\"")); EXPECT_THAT(msg, HasSubstr("\"Message\":\"hello\"")); EXPECT_THAT(msg, HasSubstr("\"FixedValue\":\"Fixed\"")); - EXPECT_EQ(msg[0], '{'); - EXPECT_EQ(msg[msg.size() - 2], '}'); - EXPECT_EQ(log.logger_name, "misc"); })); From 8bd5c49d81ccde6def1e36ca7d2acc46c5d8e8bb Mon Sep 17 00:00:00 2001 From: ohadvano Date: Wed, 17 May 2023 01:01:56 +0300 Subject: [PATCH 17/30] give CLI option precedence and add tests Signed-off-by: ohadvano --- api/envoy/config/bootstrap/v3/bootstrap.proto | 3 ++- .../observability/application_logging.rst | 6 +---- envoy/server/options.h | 5 ++++ source/common/common/logger.cc | 12 ++++++---- source/common/common/logger.h | 2 +- source/server/config_validation/server.cc | 9 ++----- source/server/options_impl.cc | 1 + source/server/options_impl.h | 7 +++++- source/server/server.cc | 9 ++----- test/common/common/logger_test.cc | 17 +++++-------- test/mocks/server/options.h | 1 + test/per_file_coverage.sh | 2 +- test/server/config_validation/server_test.cc | 24 +++++++++++++++++++ test/server/options_impl_test.cc | 5 ++++ test/server/server_test.cc | 15 ++++++++++++ 15 files changed, 80 insertions(+), 38 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index bd8a378a699f5..a57a21a203abc 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -373,7 +373,8 @@ message Bootstrap { core.v3.TypedExtensionConfig listener_manager = 37; // Optional field to set the application logs format. If this field is set, it will override - // the default log format, or the format that is specified by the :option:`--log-format` command line option. + // the default log format. In case the :option:`--log-format` command line option is used, + // it will override ``application_log_format`` configurations. ApplicationLogFormat application_log_format = 38; } diff --git a/docs/root/configuration/observability/application_logging.rst b/docs/root/configuration/observability/application_logging.rst index dc048fa0ea480..54ed12037578d 100644 --- a/docs/root/configuration/observability/application_logging.rst +++ b/docs/root/configuration/observability/application_logging.rst @@ -42,8 +42,4 @@ to print the logs in custom JSON format. The json format struct can support all FixedValue: "SomeFixedValue" .. note:: - The JSON log format will be applied only after the bootstrap config initialization. - Therefore, some of the logs will be printed in the default log format, or the one that is specified by the CLI option ``--log-format``. - For full JSON application logs experience, it is possible to set ``--log-format`` with JSON format, - so both logs before bootstrap initialization and after, are written in JSON format. - For example: ``--log-format '{"Timestamp":"%Y-%m-%dT%T.%F","ThreadId":"%t","SourceLine":"%s:%#","Level":"%l","Message":"%_"}'``. + In case the CLI option ``--log-format`` is used, its value will override ``application_log_format`` format. diff --git a/envoy/server/options.h b/envoy/server/options.h index ab90efdd1f0eb..327da389107a8 100644 --- a/envoy/server/options.h +++ b/envoy/server/options.h @@ -169,6 +169,11 @@ class Options { */ virtual const std::string& logFormat() const PURE; + /** + * @return whether or not a log format was set by CLI option. + */ + virtual bool logFormatSet() const PURE; + /** * @return const bool indicating whether to escape c-style escape sequences in logs. */ diff --git a/source/common/common/logger.cc b/source/common/common/logger.cc index ac5bf1afe0469..04ae7ebf6b306 100644 --- a/source/common/common/logger.cc +++ b/source/common/common/logger.cc @@ -6,6 +6,8 @@ #include #include +#include "envoy/common/exception.h" + #include "source/common/common/json_escape_string.h" #include "source/common/common/lock_guard.h" @@ -254,7 +256,7 @@ void Registry::setLogFormat(const std::string& log_format) { } } -ProtobufUtil::Status Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { +void Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { Protobuf::util::JsonPrintOptions json_options; json_options.preserve_proto_field_names = true; json_options.always_print_primitive_fields = true; @@ -262,11 +264,13 @@ ProtobufUtil::Status Registry::setJsonLogFormat(const Protobuf::Message& log_for std::string format_as_json; auto status = Protobuf::util::MessageToJsonString(log_format_struct, &format_as_json, json_options); - if (status.ok()) { - setLogFormat(format_as_json); + + if (!status.ok()) { + throw EnvoyException( + fmt::format("failed to set log format as JSON string from struct: {}", status.ToString())); } - return status; + setLogFormat(format_as_json); } Logger* Registry::logger(const std::string& log_name) { diff --git a/source/common/common/logger.h b/source/common/common/logger.h index 23c96cb896123..d752dd2d915bd 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -357,7 +357,7 @@ class Registry { /** * Sets the log format from a struct as a JSON string. */ - static ProtobufUtil::Status setJsonLogFormat(const Protobuf::Message& log_format_struct); + static void setJsonLogFormat(const Protobuf::Message& log_format_struct); /** * @return std::vector& the installed loggers. diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index c79024b046e5c..b7120298daadc 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -85,14 +85,9 @@ void ValidationInstance::initialize(const Options& options, InstanceUtil::loadBootstrapConfig(bootstrap_, options, messageValidationContext().staticValidationVisitor(), *api_); - if (bootstrap_.has_application_log_format() && + if (!options.logFormatSet() && bootstrap_.has_application_log_format() && bootstrap_.application_log_format().has_json_format()) { - auto status = - Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); - if (!status.ok()) { - throw EnvoyException(fmt::format("failed to set log format as JSON string from struct: {}", - status.ToString())); - } + Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); } // Inject regex engine to singleton. diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index eae78c65d513c..6345c789e8973 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -194,6 +194,7 @@ OptionsImpl::OptionsImpl(std::vector args, } log_format_ = log_format.getValue(); + log_format_set_ = log_format.isSet(); log_format_escaped_ = log_format_escaped.getValue(); enable_fine_grain_logging_ = enable_fine_grain_logging.getValue(); diff --git a/source/server/options_impl.h b/source/server/options_impl.h index 6528e0aeb8a10..67d797a170f03 100644 --- a/source/server/options_impl.h +++ b/source/server/options_impl.h @@ -78,7 +78,10 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable> component_log_levels_; std::string component_log_level_str_; std::string log_format_{Logger::Logger::DEFAULT_LOG_FORMAT}; + bool log_format_set_; bool log_format_escaped_{false}; std::string log_path_; uint64_t restart_epoch_{0}; diff --git a/source/server/server.cc b/source/server/server.cc index b60b9ed74a41a..f236116798e73 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -425,14 +425,9 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add messageValidationContext().staticValidationVisitor(), *api_); bootstrap_config_update_time_ = time_source_.systemTime(); - if (bootstrap_.has_application_log_format() && + if (!options_.logFormatSet() && bootstrap_.has_application_log_format() && bootstrap_.application_log_format().has_json_format()) { - auto status = - Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); - if (!status.ok()) { - throw EnvoyException(fmt::format("failed to set log format as JSON string from struct: {}", - status.ToString())); - } + Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); } #ifdef ENVOY_PERFETTO diff --git a/test/common/common/logger_test.cc b/test/common/common/logger_test.cc index e02aca85a4fd2..0255b02f9b762 100644 --- a/test/common/common/logger_test.cc +++ b/test/common/common/logger_test.cc @@ -1,6 +1,8 @@ #include #include +#include "envoy/common/exception.h" + #include "source/common/common/json_escape_string.h" #include "source/common/common/logger.h" @@ -266,16 +268,13 @@ TEST(LoggerTest, TestJsonFormatError) { log_struct.set_value("asdf"); // This scenario shouldn't happen in production, the test is added mainly for coverage. - auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); - EXPECT_FALSE(status.ok()); + EXPECT_THROW(Envoy::Logger::Registry::setJsonLogFormat(log_struct), EnvoyException); } TEST(LoggerTest, TestJsonFormatEmptyStruct) { ProtobufWkt::Struct log_struct; Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - - auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); - EXPECT_TRUE(status.ok()); + Envoy::Logger::Registry::setJsonLogFormat(log_struct); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { @@ -291,9 +290,7 @@ TEST(LoggerTest, TestJsonFormatNullField) { (*log_struct.mutable_fields())["Message"].set_string_value("%v"); (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - - auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); - EXPECT_TRUE(status.ok()); + Envoy::Logger::Registry::setJsonLogFormat(log_struct); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto&) { @@ -310,9 +307,7 @@ TEST(LoggerTest, TestJsonFormat) { (*log_struct.mutable_fields())["Message"].set_string_value("%v"); (*log_struct.mutable_fields())["FixedValue"].set_string_value("Fixed"); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - - auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); - EXPECT_TRUE(status.ok()); + Envoy::Logger::Registry::setJsonLogFormat(log_struct); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { diff --git a/test/mocks/server/options.h b/test/mocks/server/options.h index fb64f4f1b6642..0a0f7433124c3 100644 --- a/test/mocks/server/options.h +++ b/test/mocks/server/options.h @@ -34,6 +34,7 @@ class MockOptions : public Options { MOCK_METHOD((const std::vector>&), componentLogLevels, (), (const)); MOCK_METHOD(const std::string&, logFormat, (), (const)); + MOCK_METHOD(bool, logFormatSet, (), (const)); MOCK_METHOD(bool, logFormatEscaped, (), (const)); MOCK_METHOD(bool, enableFineGrainLogging, (), (const)); MOCK_METHOD(const std::string&, logPath, (), (const)); diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index 1d76643a247c1..c3c94ccd8be37 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -75,7 +75,7 @@ declare -a KNOWN_LOW_COVERAGE=( "source/server:93.8" # flaky: be careful adjusting. See https://github.com/envoyproxy/envoy/issues/15239 "source/server/admin:profiler-lib:83" "source/extensions/load_balancing_policies/common:94" # Death tests don't report LCOV -"source/server/config_validation:87.3" +"source/server/config_validation:88.2" "source/extensions/health_checkers:95.9" "source/extensions/health_checkers/http:93.8" "source/extensions/health_checkers/grpc:92.0" diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index 8278e77420a56..cc0ca881721bc 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -16,6 +16,8 @@ #include "test/test_common/test_time.h" using testing::HasSubstr; +using testing::Return; +using testing::ReturnRef; namespace Envoy { namespace Server { @@ -219,6 +221,28 @@ INSTANTIATE_TEST_SUITE_P( AllConfigs, RuntimeFeatureValidationServerTest, ::testing::ValuesIn(RuntimeFeatureValidationServerTest::getAllConfigFiles())); +TEST_P(JsonApplicationLogsValidationServerTest, OptionOverridesJsonApplicationLogsConfig) { + Thread::MutexBasicLockable access_log_lock; + Stats::IsolatedStoreImpl stats_store; + DangerousDeprecatedTestTime time_system; + EXPECT_CALL(options_, logFormatSet()).WillRepeatedly(Return(true)); + ValidationInstance server(options_, time_system.timeSystem(), + Network::Address::InstanceConstSharedPtr(), stats_store, + access_log_lock, component_factory_, Thread::threadFactoryForTest(), + Filesystem::fileSystemForTest()); + + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + MockLogSink sink(Envoy::Logger::Registry::getSink()); + EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_THAT(msg, HasSubstr("[info][misc]")); + EXPECT_THAT(msg, HasSubstr("hello")); + EXPECT_EQ(log.logger_name, "misc"); + })); + + ENVOY_LOG_MISC(info, "hello"); + server.shutdown(); +} + TEST_P(JsonApplicationLogsValidationServerTest, ValidateJsonApplicationLogs) { Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index 79136c60e55e7..b09db966bcc08 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -113,6 +113,7 @@ TEST_F(OptionsImplTest, All) { EXPECT_EQ(spdlog::level::info, options->logLevel()); EXPECT_EQ(2, options->componentLogLevels().size()); EXPECT_EQ("[%v]", options->logFormat()); + EXPECT_TRUE(options->logFormatSet()); EXPECT_EQ("/foo/bar", options->logPath()); EXPECT_EQ(true, options->enableFineGrainLogging()); EXPECT_EQ("cluster", options->serviceClusterName()); @@ -209,6 +210,7 @@ TEST_F(OptionsImplTest, SetAll) { EXPECT_EQ(Server::DrainStrategy::Immediate, options->drainStrategy()); EXPECT_EQ(spdlog::level::trace, options->logLevel()); EXPECT_EQ("%L %n %v", options->logFormat()); + EXPECT_TRUE(options->logFormatSet()); EXPECT_EQ("/foo/bar", options->logPath()); EXPECT_EQ(std::chrono::seconds(43), options->parentShutdownTime()); EXPECT_EQ(44, options->restartEpoch()); @@ -528,18 +530,21 @@ TEST_F(OptionsImplTest, SetCpusetOnly) { TEST_F(OptionsImplTest, LogFormatDefault) { std::unique_ptr options = createOptionsImpl({"envoy", "-c", "hello"}); EXPECT_EQ(options->logFormat(), "[%Y-%m-%d %T.%e][%t][%l][%n] [%g:%#] %v"); + EXPECT_FALSE(options->logFormatSet()); } TEST_F(OptionsImplTest, LogFormatOverride) { std::unique_ptr options = createOptionsImpl({"envoy", "-c", "hello", "--log-format", "%%v %v %t %v"}); EXPECT_EQ(options->logFormat(), "%%v %v %t %v"); + EXPECT_TRUE(options->logFormatSet()); } TEST_F(OptionsImplTest, LogFormatOverrideNoPrefix) { std::unique_ptr options = createOptionsImpl({"envoy", "-c", "hello", "--log-format", "%%v %v %t %v"}); EXPECT_EQ(options->logFormat(), "%%v %v %t %v"); + EXPECT_TRUE(options->logFormatSet()); } // Test that --base-id and --restart-epoch with non-default values are accepted. diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 9c6c5dcff920e..8152f66086291 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1625,6 +1625,21 @@ TEST_P(ServerInstanceImplTest, AdminAccessLogFilter) { EXPECT_NO_THROW(initialize("test/server/test_data/server/access_log_filter_bootstrap.yaml")); } +TEST_P(ServerInstanceImplTest, OptionOverridesJsonApplicationLogsConfig) { + EXPECT_CALL(options_, logFormatSet()).WillRepeatedly(Return(true)); + EXPECT_NO_THROW(initialize("test/server/test_data/server/json_application_log.yaml")); + + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + MockLogSink sink(Envoy::Logger::Registry::getSink()); + EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_THAT(msg, HasSubstr("[info][misc]")); + EXPECT_THAT(msg, HasSubstr("hello")); + EXPECT_EQ(log.logger_name, "misc"); + })); + + ENVOY_LOG_MISC(info, "hello"); +} + TEST_P(ServerInstanceImplTest, JsonApplicationLog) { EXPECT_NO_THROW(initialize("test/server/test_data/server/json_application_log.yaml")); From 3af6419bc527646fb7818f96cc40aae21cb18bb0 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Wed, 17 May 2023 09:22:00 +0300 Subject: [PATCH 18/30] fix test Signed-off-by: ohadvano --- source/server/options_impl.h | 2 +- test/server/config_validation/server_test.cc | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/source/server/options_impl.h b/source/server/options_impl.h index 67d797a170f03..d02f37dce5bc1 100644 --- a/source/server/options_impl.h +++ b/source/server/options_impl.h @@ -205,7 +205,7 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable> component_log_levels_; std::string component_log_level_str_; std::string log_format_{Logger::Logger::DEFAULT_LOG_FORMAT}; - bool log_format_set_; + bool log_format_set_{false}; bool log_format_escaped_{false}; std::string log_path_; uint64_t restart_epoch_{0}; diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index cc0ca881721bc..c376b606ad599 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -17,7 +17,6 @@ using testing::HasSubstr; using testing::Return; -using testing::ReturnRef; namespace Envoy { namespace Server { From 2658e13986bd66a3bf2e3166111ba39f74beafaf Mon Sep 17 00:00:00 2001 From: ohadvano Date: Wed, 17 May 2023 14:12:44 +0300 Subject: [PATCH 19/30] forbid %v and add tests Signed-off-by: ohadvano --- api/envoy/config/bootstrap/v3/bootstrap.proto | 2 +- .../observability/application_logging.rst | 3 +- source/common/common/logger.cc | 4 ++ test/common/common/logger_test.cc | 47 +++++++++++++++++-- test/server/config_validation/server_test.cc | 35 +++++++++++++- .../test_data/json_application_logs.yaml | 2 +- .../json_application_logs_forbidden_flag.yaml | 10 ++++ test/server/server_test.cc | 5 ++ .../server/json_application_log.yaml | 2 +- .../json_application_log_forbidden_flag.yaml | 10 ++++ 10 files changed, 111 insertions(+), 9 deletions(-) create mode 100644 test/server/config_validation/test_data/json_application_logs_forbidden_flag.yaml create mode 100644 test/server/test_data/server/json_application_log_forbidden_flag.yaml diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index a57a21a203abc..f4ab30d4a31db 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -107,7 +107,7 @@ message Bootstrap { // Flush application logs in JSON format. The configured JSON struct can // support all the format tags specified in the in :option:`--log-format` - // command line option section. + // command line option section, except for the ``%v`` flag. google.protobuf.Struct json_format = 1; } } diff --git a/docs/root/configuration/observability/application_logging.rst b/docs/root/configuration/observability/application_logging.rst index 54ed12037578d..07d3189d4f5a5 100644 --- a/docs/root/configuration/observability/application_logging.rst +++ b/docs/root/configuration/observability/application_logging.rst @@ -28,7 +28,8 @@ Printing logs in JSON format ---------------------------- It is possible to use the bootstrap config :ref:`json_format ` -to print the logs in custom JSON format. The json format struct can support all the format flags that are specified in :ref:`command line options `. Example: +to print the logs in custom JSON format. The json format struct can support all the format flags that are specified in :ref:`command line options `, +except for the ``%v`` flag, as multi-line logs would break the JSON structure log. Instead, use the ``%_`` flag. Example: .. code-block:: yaml diff --git a/source/common/common/logger.cc b/source/common/common/logger.cc index 04ae7ebf6b306..0efa5bcd00900 100644 --- a/source/common/common/logger.cc +++ b/source/common/common/logger.cc @@ -270,6 +270,10 @@ void Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { fmt::format("failed to set log format as JSON string from struct: {}", status.ToString())); } + if (format_as_json.find("%v") != std::string::npos) { + throw EnvoyException("Usage of %v is unavailable for JSON log formats"); + } + setLogFormat(format_as_json); } diff --git a/test/common/common/logger_test.cc b/test/common/common/logger_test.cc index 0255b02f9b762..2aef9507d20cf 100644 --- a/test/common/common/logger_test.cc +++ b/test/common/common/logger_test.cc @@ -287,13 +287,14 @@ TEST(LoggerTest, TestJsonFormatEmptyStruct) { TEST(LoggerTest, TestJsonFormatNullField) { ProtobufWkt::Struct log_struct; - (*log_struct.mutable_fields())["Message"].set_string_value("%v"); + (*log_struct.mutable_fields())["Message"].set_string_value("%_"); (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); Envoy::Logger::Registry::setJsonLogFormat(log_struct); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto&) { + EXPECT_NO_THROW(Json::Factory::loadFromString(std::string(msg))); EXPECT_THAT(msg, HasSubstr("\"Message\":\"hello\"")); EXPECT_THAT(msg, HasSubstr("\"NullField\":null")); })); @@ -301,23 +302,61 @@ TEST(LoggerTest, TestJsonFormatNullField) { ENVOY_LOG_MISC(info, "hello"); } +TEST(LoggerTest, TestJsonFormatNonEscapedThrows) { + ProtobufWkt::Struct log_struct; + (*log_struct.mutable_fields())["Message"].set_string_value("%v"); + (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + EXPECT_THROW(Envoy::Logger::Registry::setJsonLogFormat(log_struct), EnvoyException); +} + TEST(LoggerTest, TestJsonFormat) { ProtobufWkt::Struct log_struct; (*log_struct.mutable_fields())["Level"].set_string_value("%l"); - (*log_struct.mutable_fields())["Message"].set_string_value("%v"); + (*log_struct.mutable_fields())["Message"].set_string_value("%_"); + (*log_struct.mutable_fields())["FixedValue"].set_string_value("Fixed"); + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + Envoy::Logger::Registry::setJsonLogFormat(log_struct); + + MockLogSink sink(Envoy::Logger::Registry::getSink()); + EXPECT_CALL(sink, log(_, _)) + .WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_NO_THROW(Json::Factory::loadFromString(std::string(msg))); + EXPECT_THAT(msg, HasSubstr("\"Level\":\"info\"")); + EXPECT_THAT(msg, HasSubstr("\"Message\":\"hello\"")); + EXPECT_THAT(msg, HasSubstr("\"FixedValue\":\"Fixed\"")); + EXPECT_EQ(log.logger_name, "misc"); + })) + .WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_NO_THROW(Json::Factory::loadFromString(std::string(msg))); + EXPECT_THAT(msg, HasSubstr("\"Level\":\"info\"")); + EXPECT_THAT(msg, HasSubstr("\"Message\":\"hel\\nlo\"")); + EXPECT_THAT(msg, HasSubstr("\"FixedValue\":\"Fixed\"")); + EXPECT_EQ(log.logger_name, "misc"); + })); + + ENVOY_LOG_MISC(info, "hello"); + ENVOY_LOG_MISC(info, "hel\nlo"); +} + +TEST(LoggerTest, TestJsonFormatWithEscapedJson) { + ProtobufWkt::Struct log_struct; + (*log_struct.mutable_fields())["Level"].set_string_value("%l"); + (*log_struct.mutable_fields())["Message"].set_string_value("%j"); (*log_struct.mutable_fields())["FixedValue"].set_string_value("Fixed"); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); Envoy::Logger::Registry::setJsonLogFormat(log_struct); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_NO_THROW(Json::Factory::loadFromString(std::string(msg))); EXPECT_THAT(msg, HasSubstr("\"Level\":\"info\"")); - EXPECT_THAT(msg, HasSubstr("\"Message\":\"hello\"")); + EXPECT_THAT(msg, HasSubstr("\"Message\":\"{\\\"nested_message\\\":\\\"hello\\\"}\"")); EXPECT_THAT(msg, HasSubstr("\"FixedValue\":\"Fixed\"")); EXPECT_EQ(log.logger_name, "misc"); })); - ENVOY_LOG_MISC(info, "hello"); + ENVOY_LOG_MISC(info, "{\"nested_message\":\"hello\"}"); } } // namespace diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index c376b606ad599..d019ed3fa53e6 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -98,6 +98,23 @@ class JsonApplicationLogsValidationServerTest : public ValidationServerTest { } }; +class JsonApplicationLogsValidationServerForbiddenFlagsTest : public ValidationServerTest { +public: + static void SetUpTestSuite() { // NOLINT(readability-identifier-naming) + setupTestDirectory(); + } + + static void setupTestDirectory() { + directory_ = + TestEnvironment::runfilesDirectory("envoy/test/server/config_validation/test_data/"); + } + + static const std::vector getAllConfigFiles() { + setupTestDirectory(); + return {"json_application_logs_forbidden_flag.yaml"}; + } +}; + TEST_P(ValidationServerTest, Validate) { EXPECT_TRUE(validateConfig(options_, Network::Address::InstanceConstSharedPtr(), component_factory_, Thread::threadFactoryForTest(), @@ -242,7 +259,7 @@ TEST_P(JsonApplicationLogsValidationServerTest, OptionOverridesJsonApplicationLo server.shutdown(); } -TEST_P(JsonApplicationLogsValidationServerTest, ValidateJsonApplicationLogs) { +TEST_P(JsonApplicationLogsValidationServerTest, JsonApplicationLogs) { Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; DangerousDeprecatedTestTime time_system; @@ -266,6 +283,22 @@ INSTANTIATE_TEST_SUITE_P( AllConfigs, JsonApplicationLogsValidationServerTest, ::testing::ValuesIn(JsonApplicationLogsValidationServerTest::getAllConfigFiles())); +TEST_P(JsonApplicationLogsValidationServerForbiddenFlagsTest, TestNewlineForbiddenFlag) { + Thread::MutexBasicLockable access_log_lock; + Stats::IsolatedStoreImpl stats_store; + DangerousDeprecatedTestTime time_system; + EXPECT_THROW(ValidationInstance server( + options_, time_system.timeSystem(), Network::Address::InstanceConstSharedPtr(), + stats_store, access_log_lock, component_factory_, Thread::threadFactoryForTest(), + Filesystem::fileSystemForTest()), + EnvoyException); +} + +INSTANTIATE_TEST_SUITE_P( + AllConfigs, JsonApplicationLogsValidationServerForbiddenFlagsTest, + ::testing::ValuesIn( + JsonApplicationLogsValidationServerForbiddenFlagsTest::getAllConfigFiles())); + } // namespace } // namespace Server } // namespace Envoy diff --git a/test/server/config_validation/test_data/json_application_logs.yaml b/test/server/config_validation/test_data/json_application_logs.yaml index 132cf9d06716f..355fab882699d 100644 --- a/test/server/config_validation/test_data/json_application_logs.yaml +++ b/test/server/config_validation/test_data/json_application_logs.yaml @@ -1,7 +1,7 @@ --- application_log_format: json_format: - MessageFromProto: "%v" + MessageFromProto: "%_" admin: address: diff --git a/test/server/config_validation/test_data/json_application_logs_forbidden_flag.yaml b/test/server/config_validation/test_data/json_application_logs_forbidden_flag.yaml new file mode 100644 index 0000000000000..132cf9d06716f --- /dev/null +++ b/test/server/config_validation/test_data/json_application_logs_forbidden_flag.yaml @@ -0,0 +1,10 @@ +--- +application_log_format: + json_format: + MessageFromProto: "%v" + +admin: + address: + socket_address: + address: 0.0.0.0 + port_value: 9000 diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 8152f66086291..1652c9afb8389 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1653,6 +1653,11 @@ TEST_P(ServerInstanceImplTest, JsonApplicationLog) { ENVOY_LOG_MISC(info, "hello"); } +TEST_P(ServerInstanceImplTest, JsonApplicationLogFailWithForbiddenFlags) { + EXPECT_THROW(initialize("test/server/test_data/server/json_application_log_forbidden_flag.yaml"), + EnvoyException); +} + } // namespace } // namespace Server } // namespace Envoy diff --git a/test/server/test_data/server/json_application_log.yaml b/test/server/test_data/server/json_application_log.yaml index 132cf9d06716f..355fab882699d 100644 --- a/test/server/test_data/server/json_application_log.yaml +++ b/test/server/test_data/server/json_application_log.yaml @@ -1,7 +1,7 @@ --- application_log_format: json_format: - MessageFromProto: "%v" + MessageFromProto: "%_" admin: address: diff --git a/test/server/test_data/server/json_application_log_forbidden_flag.yaml b/test/server/test_data/server/json_application_log_forbidden_flag.yaml new file mode 100644 index 0000000000000..132cf9d06716f --- /dev/null +++ b/test/server/test_data/server/json_application_log_forbidden_flag.yaml @@ -0,0 +1,10 @@ +--- +application_log_format: + json_format: + MessageFromProto: "%v" + +admin: + address: + socket_address: + address: 0.0.0.0 + port_value: 9000 From 89a1e430323d5e0ce04f899f98b941c035cc8ea6 Mon Sep 17 00:00:00 2001 From: ohadvano <49730675+ohadvano@users.noreply.github.com> Date: Sat, 20 May 2023 11:01:54 +0300 Subject: [PATCH 20/30] Add exception for exception Signed-off-by: ohadvano <49730675+ohadvano@users.noreply.github.com> --- tools/code_format/config.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index 0aff6d44d5ae5..4984a2501d31a 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -93,6 +93,7 @@ paths: - source/extensions/common/matcher/trie_matcher.h # legacy core files which throw exceptions. We can add to this list but strongly prefer # StausOr where possible. + - source/common/common/logger.cc - source/common/upstream/wrsq_scheduler.h - source/common/upstream/cds_api_helper.cc - source/common/upstream/thread_aware_lb_impl.cc From 5b64d179705829f379ca4e4d235e6bb3847fc52d Mon Sep 17 00:00:00 2001 From: ohadvano Date: Sat, 20 May 2023 12:28:04 +0300 Subject: [PATCH 21/30] move exception throw from core to server code Signed-off-by: ohadvano --- source/common/common/logger.cc | 10 ++++------ source/common/common/logger.h | 2 +- source/server/config_validation/server.cc | 7 ++++++- source/server/server.cc | 7 ++++++- test/common/common/logger_test.cc | 18 ++++++++++++------ test/server/config_validation/server_test.cc | 12 +++++++----- test/server/server_test.cc | 6 ++++-- tools/code_format/config.yaml | 1 - 8 files changed, 40 insertions(+), 23 deletions(-) diff --git a/source/common/common/logger.cc b/source/common/common/logger.cc index 0efa5bcd00900..cdd6f0510eba1 100644 --- a/source/common/common/logger.cc +++ b/source/common/common/logger.cc @@ -6,8 +6,6 @@ #include #include -#include "envoy/common/exception.h" - #include "source/common/common/json_escape_string.h" #include "source/common/common/lock_guard.h" @@ -256,7 +254,7 @@ void Registry::setLogFormat(const std::string& log_format) { } } -void Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { +absl::Status Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { Protobuf::util::JsonPrintOptions json_options; json_options.preserve_proto_field_names = true; json_options.always_print_primitive_fields = true; @@ -266,15 +264,15 @@ void Registry::setJsonLogFormat(const Protobuf::Message& log_format_struct) { Protobuf::util::MessageToJsonString(log_format_struct, &format_as_json, json_options); if (!status.ok()) { - throw EnvoyException( - fmt::format("failed to set log format as JSON string from struct: {}", status.ToString())); + return absl::InvalidArgumentError("Provided struct cannot be serialized as JSON string"); } if (format_as_json.find("%v") != std::string::npos) { - throw EnvoyException("Usage of %v is unavailable for JSON log formats"); + return absl::InvalidArgumentError("Usage of %v is unavailable for JSON log formats"); } setLogFormat(format_as_json); + return absl::OkStatus(); } Logger* Registry::logger(const std::string& log_name) { diff --git a/source/common/common/logger.h b/source/common/common/logger.h index d752dd2d915bd..28aecaf86640a 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -357,7 +357,7 @@ class Registry { /** * Sets the log format from a struct as a JSON string. */ - static void setJsonLogFormat(const Protobuf::Message& log_format_struct); + static absl::Status setJsonLogFormat(const Protobuf::Message& log_format_struct); /** * @return std::vector& the installed loggers. diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index b7120298daadc..d726ae2c499a4 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -87,7 +87,12 @@ void ValidationInstance::initialize(const Options& options, if (!options.logFormatSet() && bootstrap_.has_application_log_format() && bootstrap_.application_log_format().has_json_format()) { - Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); + auto status = + Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); + + if (!status.ok()) { + throw EnvoyException(fmt::format("setJsonLogFormat error: {}", status.ToString())); + } } // Inject regex engine to singleton. diff --git a/source/server/server.cc b/source/server/server.cc index 744b2c09e587a..fbed16722a523 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -424,7 +424,12 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add if (!options_.logFormatSet() && bootstrap_.has_application_log_format() && bootstrap_.application_log_format().has_json_format()) { - Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); + auto status = + Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); + + if (!status.ok()) { + throw EnvoyException(fmt::format("setJsonLogFormat error: {}", status.ToString())); + } } #ifdef ENVOY_PERFETTO diff --git a/test/common/common/logger_test.cc b/test/common/common/logger_test.cc index 2aef9507d20cf..87e4a67250e6f 100644 --- a/test/common/common/logger_test.cc +++ b/test/common/common/logger_test.cc @@ -268,13 +268,16 @@ TEST(LoggerTest, TestJsonFormatError) { log_struct.set_value("asdf"); // This scenario shouldn't happen in production, the test is added mainly for coverage. - EXPECT_THROW(Envoy::Logger::Registry::setJsonLogFormat(log_struct), EnvoyException); + auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_FALSE(status.ok()); + EXPECT_EQ("INVALID_ARGUMENT: Provided struct cannot be serialized as JSON string", + status.ToString()); } TEST(LoggerTest, TestJsonFormatEmptyStruct) { ProtobufWkt::Struct log_struct; Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_TRUE(Envoy::Logger::Registry::setJsonLogFormat(log_struct).ok()); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { @@ -290,7 +293,7 @@ TEST(LoggerTest, TestJsonFormatNullField) { (*log_struct.mutable_fields())["Message"].set_string_value("%_"); (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_TRUE(Envoy::Logger::Registry::setJsonLogFormat(log_struct).ok()); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto&) { @@ -307,7 +310,10 @@ TEST(LoggerTest, TestJsonFormatNonEscapedThrows) { (*log_struct.mutable_fields())["Message"].set_string_value("%v"); (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - EXPECT_THROW(Envoy::Logger::Registry::setJsonLogFormat(log_struct), EnvoyException); + + auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_FALSE(status.ok()); + EXPECT_EQ("INVALID_ARGUMENT: Usage of %v is unavailable for JSON log formats", status.ToString()); } TEST(LoggerTest, TestJsonFormat) { @@ -316,7 +322,7 @@ TEST(LoggerTest, TestJsonFormat) { (*log_struct.mutable_fields())["Message"].set_string_value("%_"); (*log_struct.mutable_fields())["FixedValue"].set_string_value("Fixed"); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_TRUE(Envoy::Logger::Registry::setJsonLogFormat(log_struct).ok()); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)) @@ -345,7 +351,7 @@ TEST(LoggerTest, TestJsonFormatWithEscapedJson) { (*log_struct.mutable_fields())["Message"].set_string_value("%j"); (*log_struct.mutable_fields())["FixedValue"].set_string_value("Fixed"); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_TRUE(Envoy::Logger::Registry::setJsonLogFormat(log_struct).ok()); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index d019ed3fa53e6..062e1223db84f 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -287,11 +287,13 @@ TEST_P(JsonApplicationLogsValidationServerForbiddenFlagsTest, TestNewlineForbidd Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; DangerousDeprecatedTestTime time_system; - EXPECT_THROW(ValidationInstance server( - options_, time_system.timeSystem(), Network::Address::InstanceConstSharedPtr(), - stats_store, access_log_lock, component_factory_, Thread::threadFactoryForTest(), - Filesystem::fileSystemForTest()), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + ValidationInstance server(options_, time_system.timeSystem(), + Network::Address::InstanceConstSharedPtr(), stats_store, + access_log_lock, component_factory_, Thread::threadFactoryForTest(), + Filesystem::fileSystemForTest()), + EnvoyException, + "setJsonLogFormat error: INVALID_ARGUMENT: Usage of %v is unavailable for JSON log formats"); } INSTANTIATE_TEST_SUITE_P( diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 1652c9afb8389..120e2fd6df023 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1654,8 +1654,10 @@ TEST_P(ServerInstanceImplTest, JsonApplicationLog) { } TEST_P(ServerInstanceImplTest, JsonApplicationLogFailWithForbiddenFlags) { - EXPECT_THROW(initialize("test/server/test_data/server/json_application_log_forbidden_flag.yaml"), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + initialize("test/server/test_data/server/json_application_log_forbidden_flag.yaml"), + EnvoyException, + "setJsonLogFormat error: INVALID_ARGUMENT: Usage of %v is unavailable for JSON log formats"); } } // namespace diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index 4984a2501d31a..0aff6d44d5ae5 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -93,7 +93,6 @@ paths: - source/extensions/common/matcher/trie_matcher.h # legacy core files which throw exceptions. We can add to this list but strongly prefer # StausOr where possible. - - source/common/common/logger.cc - source/common/upstream/wrsq_scheduler.h - source/common/upstream/cds_api_helper.cc - source/common/upstream/thread_aware_lb_impl.cc From a81cfac15c50485d7855de51dee6a9c747600f5c Mon Sep 17 00:00:00 2001 From: ohadvano Date: Sat, 20 May 2023 12:30:00 +0300 Subject: [PATCH 22/30] remove unused include Signed-off-by: ohadvano --- test/common/common/logger_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/common/common/logger_test.cc b/test/common/common/logger_test.cc index 87e4a67250e6f..2203aca0c3616 100644 --- a/test/common/common/logger_test.cc +++ b/test/common/common/logger_test.cc @@ -1,8 +1,6 @@ #include #include -#include "envoy/common/exception.h" - #include "source/common/common/json_escape_string.h" #include "source/common/common/logger.h" From 829f0494ea897fa64678453e6304b4231d1fad09 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Mon, 22 May 2023 19:02:57 +0300 Subject: [PATCH 23/30] add test check and const variable Signed-off-by: ohadvano --- source/common/common/logger.cc | 2 +- source/server/config_validation/server.cc | 2 +- source/server/server.cc | 2 +- test/server/server_test.cc | 1 + 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/source/common/common/logger.cc b/source/common/common/logger.cc index cdd6f0510eba1..cf1feed318c6e 100644 --- a/source/common/common/logger.cc +++ b/source/common/common/logger.cc @@ -260,7 +260,7 @@ absl::Status Registry::setJsonLogFormat(const Protobuf::Message& log_format_stru json_options.always_print_primitive_fields = true; std::string format_as_json; - auto status = + const auto status = Protobuf::util::MessageToJsonString(log_format_struct, &format_as_json, json_options); if (!status.ok()) { diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index d726ae2c499a4..4c677ff20e02f 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -87,7 +87,7 @@ void ValidationInstance::initialize(const Options& options, if (!options.logFormatSet() && bootstrap_.has_application_log_format() && bootstrap_.application_log_format().has_json_format()) { - auto status = + const auto status = Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); if (!status.ok()) { diff --git a/source/server/server.cc b/source/server/server.cc index fbed16722a523..e8ad3224716a8 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -424,7 +424,7 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add if (!options_.logFormatSet() && bootstrap_.has_application_log_format() && bootstrap_.application_log_format().has_json_format()) { - auto status = + const auto status = Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); if (!status.ok()) { diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 120e2fd6df023..d9743555ee33a 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1646,6 +1646,7 @@ TEST_P(ServerInstanceImplTest, JsonApplicationLog) { Envoy::Logger::Registry::setLogLevel(spdlog::level::info); MockLogSink sink(Envoy::Logger::Registry::getSink()); EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_NO_THROW(Json::Factory::loadFromString(std::string(msg))); EXPECT_THAT(msg, HasSubstr("{\"MessageFromProto\":\"hello\"}")); EXPECT_EQ(log.logger_name, "misc"); })); From f3f0a210b35a13b45beb0517e98ed792869b89e5 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Thu, 25 May 2023 17:21:57 +0300 Subject: [PATCH 24/30] reject %_ flag for JSON format Signed-off-by: ohadvano --- api/envoy/config/bootstrap/v3/bootstrap.proto | 2 +- .../observability/application_logging.rst | 4 +- source/common/common/logger.cc | 4 ++ test/common/common/logger_test.cc | 57 +++++++++++++------ test/server/config_validation/server_test.cc | 43 ++++++++++++-- .../test_data/json_application_logs.yaml | 2 +- ...json_application_logs_forbidden_flag_.yaml | 10 ++++ ...son_application_logs_forbidden_flagv.yaml} | 0 test/server/server_test.cc | 11 +++- .../server/json_application_log.yaml | 2 +- .../json_application_log_forbidden_flag_.yaml | 10 ++++ ...json_application_log_forbidden_flagv.yaml} | 0 12 files changed, 114 insertions(+), 31 deletions(-) create mode 100644 test/server/config_validation/test_data/json_application_logs_forbidden_flag_.yaml rename test/server/config_validation/test_data/{json_application_logs_forbidden_flag.yaml => json_application_logs_forbidden_flagv.yaml} (100%) create mode 100644 test/server/test_data/server/json_application_log_forbidden_flag_.yaml rename test/server/test_data/server/{json_application_log_forbidden_flag.yaml => json_application_log_forbidden_flagv.yaml} (100%) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index f4ab30d4a31db..948cf6513ccea 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -107,7 +107,7 @@ message Bootstrap { // Flush application logs in JSON format. The configured JSON struct can // support all the format tags specified in the in :option:`--log-format` - // command line option section, except for the ``%v`` flag. + // command line option section, except for the ``%v`` and ``%_`` flags. google.protobuf.Struct json_format = 1; } } diff --git a/docs/root/configuration/observability/application_logging.rst b/docs/root/configuration/observability/application_logging.rst index 07d3189d4f5a5..7909fa9a8b7eb 100644 --- a/docs/root/configuration/observability/application_logging.rst +++ b/docs/root/configuration/observability/application_logging.rst @@ -29,7 +29,7 @@ Printing logs in JSON format It is possible to use the bootstrap config :ref:`json_format ` to print the logs in custom JSON format. The json format struct can support all the format flags that are specified in :ref:`command line options `, -except for the ``%v`` flag, as multi-line logs would break the JSON structure log. Instead, use the ``%_`` flag. Example: +except for the ``%v`` and ``%_`` flag, as they may break the JSON structure log. Instead, use the ``%j`` flag. Example: .. code-block:: yaml @@ -39,7 +39,7 @@ except for the ``%v`` flag, as multi-line logs would break the JSON structure lo ThreadId: "%t" SourceLine: "%s:%#" Level: "%l" - Message: "%_" + Message: "%j" FixedValue: "SomeFixedValue" .. note:: diff --git a/source/common/common/logger.cc b/source/common/common/logger.cc index cf1feed318c6e..476f2e06a9f9a 100644 --- a/source/common/common/logger.cc +++ b/source/common/common/logger.cc @@ -271,6 +271,10 @@ absl::Status Registry::setJsonLogFormat(const Protobuf::Message& log_format_stru return absl::InvalidArgumentError("Usage of %v is unavailable for JSON log formats"); } + if (format_as_json.find("%_") != std::string::npos) { + return absl::InvalidArgumentError("Usage of %_ is unavailable for JSON log formats"); + } + setLogFormat(format_as_json); return absl::OkStatus(); } diff --git a/test/common/common/logger_test.cc b/test/common/common/logger_test.cc index 2203aca0c3616..e9998db311e3b 100644 --- a/test/common/common/logger_test.cc +++ b/test/common/common/logger_test.cc @@ -272,6 +272,32 @@ TEST(LoggerTest, TestJsonFormatError) { status.ToString()); } +TEST(LoggerTest, TestJsonFormatNonEscapedThrows) { + Envoy::Logger::Registry::setLogLevel(spdlog::level::info); + + { + ProtobufWkt::Struct log_struct; + (*log_struct.mutable_fields())["Message"].set_string_value("%v"); + (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); + + auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_FALSE(status.ok()); + EXPECT_EQ("INVALID_ARGUMENT: Usage of %v is unavailable for JSON log formats", + status.ToString()); + } + + { + ProtobufWkt::Struct log_struct; + (*log_struct.mutable_fields())["Message"].set_string_value("%_"); + (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); + + auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); + EXPECT_FALSE(status.ok()); + EXPECT_EQ("INVALID_ARGUMENT: Usage of %_ is unavailable for JSON log formats", + status.ToString()); + } +} + TEST(LoggerTest, TestJsonFormatEmptyStruct) { ProtobufWkt::Struct log_struct; Envoy::Logger::Registry::setLogLevel(spdlog::level::info); @@ -286,9 +312,10 @@ TEST(LoggerTest, TestJsonFormatEmptyStruct) { ENVOY_LOG_MISC(info, "hello"); } -TEST(LoggerTest, TestJsonFormatNullField) { +TEST(LoggerTest, TestJsonFormatNullAndFixedField) { ProtobufWkt::Struct log_struct; - (*log_struct.mutable_fields())["Message"].set_string_value("%_"); + (*log_struct.mutable_fields())["Message"].set_string_value("%j"); + (*log_struct.mutable_fields())["FixedValue"].set_string_value("Fixed"); (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); EXPECT_TRUE(Envoy::Logger::Registry::setJsonLogFormat(log_struct).ok()); @@ -297,28 +324,17 @@ TEST(LoggerTest, TestJsonFormatNullField) { EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto&) { EXPECT_NO_THROW(Json::Factory::loadFromString(std::string(msg))); EXPECT_THAT(msg, HasSubstr("\"Message\":\"hello\"")); + EXPECT_THAT(msg, HasSubstr("\"FixedValue\":\"Fixed\"")); EXPECT_THAT(msg, HasSubstr("\"NullField\":null")); })); ENVOY_LOG_MISC(info, "hello"); } -TEST(LoggerTest, TestJsonFormatNonEscapedThrows) { - ProtobufWkt::Struct log_struct; - (*log_struct.mutable_fields())["Message"].set_string_value("%v"); - (*log_struct.mutable_fields())["NullField"].set_null_value(ProtobufWkt::NULL_VALUE); - Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - - auto status = Envoy::Logger::Registry::setJsonLogFormat(log_struct); - EXPECT_FALSE(status.ok()); - EXPECT_EQ("INVALID_ARGUMENT: Usage of %v is unavailable for JSON log formats", status.ToString()); -} - TEST(LoggerTest, TestJsonFormat) { ProtobufWkt::Struct log_struct; (*log_struct.mutable_fields())["Level"].set_string_value("%l"); - (*log_struct.mutable_fields())["Message"].set_string_value("%_"); - (*log_struct.mutable_fields())["FixedValue"].set_string_value("Fixed"); + (*log_struct.mutable_fields())["Message"].set_string_value("%j"); Envoy::Logger::Registry::setLogLevel(spdlog::level::info); EXPECT_TRUE(Envoy::Logger::Registry::setJsonLogFormat(log_struct).ok()); @@ -328,22 +344,27 @@ TEST(LoggerTest, TestJsonFormat) { EXPECT_NO_THROW(Json::Factory::loadFromString(std::string(msg))); EXPECT_THAT(msg, HasSubstr("\"Level\":\"info\"")); EXPECT_THAT(msg, HasSubstr("\"Message\":\"hello\"")); - EXPECT_THAT(msg, HasSubstr("\"FixedValue\":\"Fixed\"")); EXPECT_EQ(log.logger_name, "misc"); })) .WillOnce(Invoke([](auto msg, auto& log) { EXPECT_NO_THROW(Json::Factory::loadFromString(std::string(msg))); EXPECT_THAT(msg, HasSubstr("\"Level\":\"info\"")); EXPECT_THAT(msg, HasSubstr("\"Message\":\"hel\\nlo\"")); - EXPECT_THAT(msg, HasSubstr("\"FixedValue\":\"Fixed\"")); + EXPECT_EQ(log.logger_name, "misc"); + })) + .WillOnce(Invoke([](auto msg, auto& log) { + EXPECT_NO_THROW(Json::Factory::loadFromString(std::string(msg))); + EXPECT_THAT(msg, HasSubstr("\"Level\":\"info\"")); + EXPECT_THAT(msg, HasSubstr("\"Message\":\"hel\\\"lo\"")); EXPECT_EQ(log.logger_name, "misc"); })); ENVOY_LOG_MISC(info, "hello"); ENVOY_LOG_MISC(info, "hel\nlo"); + ENVOY_LOG_MISC(info, "hel\"lo"); } -TEST(LoggerTest, TestJsonFormatWithEscapedJson) { +TEST(LoggerTest, TestJsonFormatWithNestedJsonMessage) { ProtobufWkt::Struct log_struct; (*log_struct.mutable_fields())["Level"].set_string_value("%l"); (*log_struct.mutable_fields())["Message"].set_string_value("%j"); diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index 062e1223db84f..a534d18279093 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -98,20 +98,33 @@ class JsonApplicationLogsValidationServerTest : public ValidationServerTest { } }; -class JsonApplicationLogsValidationServerForbiddenFlagsTest : public ValidationServerTest { +class JsonApplicationLogsValidationServerForbiddenFlagvTest : public ValidationServerTest { public: static void SetUpTestSuite() { // NOLINT(readability-identifier-naming) setupTestDirectory(); } - static void setupTestDirectory() { directory_ = TestEnvironment::runfilesDirectory("envoy/test/server/config_validation/test_data/"); } + static const std::vector getAllConfigFiles() { + setupTestDirectory(); + return {"json_application_logs_forbidden_flagv.yaml"}; + } +}; +class JsonApplicationLogsValidationServerForbiddenFlag_Test : public ValidationServerTest { +public: + static void SetUpTestSuite() { // NOLINT(readability-identifier-naming) + setupTestDirectory(); + } + static void setupTestDirectory() { + directory_ = + TestEnvironment::runfilesDirectory("envoy/test/server/config_validation/test_data/"); + } static const std::vector getAllConfigFiles() { setupTestDirectory(); - return {"json_application_logs_forbidden_flag.yaml"}; + return {"json_application_logs_forbidden_flag_.yaml"}; } }; @@ -283,7 +296,7 @@ INSTANTIATE_TEST_SUITE_P( AllConfigs, JsonApplicationLogsValidationServerTest, ::testing::ValuesIn(JsonApplicationLogsValidationServerTest::getAllConfigFiles())); -TEST_P(JsonApplicationLogsValidationServerForbiddenFlagsTest, TestNewlineForbiddenFlag) { +TEST_P(JsonApplicationLogsValidationServerForbiddenFlagvTest, TestForbiddenFlag) { Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; DangerousDeprecatedTestTime time_system; @@ -297,9 +310,27 @@ TEST_P(JsonApplicationLogsValidationServerForbiddenFlagsTest, TestNewlineForbidd } INSTANTIATE_TEST_SUITE_P( - AllConfigs, JsonApplicationLogsValidationServerForbiddenFlagsTest, + AllConfigs, JsonApplicationLogsValidationServerForbiddenFlagvTest, + ::testing::ValuesIn( + JsonApplicationLogsValidationServerForbiddenFlagvTest::getAllConfigFiles())); + +TEST_P(JsonApplicationLogsValidationServerForbiddenFlag_Test, TestForbiddenFlag) { + Thread::MutexBasicLockable access_log_lock; + Stats::IsolatedStoreImpl stats_store; + DangerousDeprecatedTestTime time_system; + EXPECT_THROW_WITH_MESSAGE( + ValidationInstance server(options_, time_system.timeSystem(), + Network::Address::InstanceConstSharedPtr(), stats_store, + access_log_lock, component_factory_, Thread::threadFactoryForTest(), + Filesystem::fileSystemForTest()), + EnvoyException, + "setJsonLogFormat error: INVALID_ARGUMENT: Usage of %_ is unavailable for JSON log formats"); +} + +INSTANTIATE_TEST_SUITE_P( + AllConfigs, JsonApplicationLogsValidationServerForbiddenFlag_Test, ::testing::ValuesIn( - JsonApplicationLogsValidationServerForbiddenFlagsTest::getAllConfigFiles())); + JsonApplicationLogsValidationServerForbiddenFlag_Test::getAllConfigFiles())); } // namespace } // namespace Server diff --git a/test/server/config_validation/test_data/json_application_logs.yaml b/test/server/config_validation/test_data/json_application_logs.yaml index 355fab882699d..57f453a572454 100644 --- a/test/server/config_validation/test_data/json_application_logs.yaml +++ b/test/server/config_validation/test_data/json_application_logs.yaml @@ -1,7 +1,7 @@ --- application_log_format: json_format: - MessageFromProto: "%_" + MessageFromProto: "%j" admin: address: diff --git a/test/server/config_validation/test_data/json_application_logs_forbidden_flag_.yaml b/test/server/config_validation/test_data/json_application_logs_forbidden_flag_.yaml new file mode 100644 index 0000000000000..355fab882699d --- /dev/null +++ b/test/server/config_validation/test_data/json_application_logs_forbidden_flag_.yaml @@ -0,0 +1,10 @@ +--- +application_log_format: + json_format: + MessageFromProto: "%_" + +admin: + address: + socket_address: + address: 0.0.0.0 + port_value: 9000 diff --git a/test/server/config_validation/test_data/json_application_logs_forbidden_flag.yaml b/test/server/config_validation/test_data/json_application_logs_forbidden_flagv.yaml similarity index 100% rename from test/server/config_validation/test_data/json_application_logs_forbidden_flag.yaml rename to test/server/config_validation/test_data/json_application_logs_forbidden_flagv.yaml diff --git a/test/server/server_test.cc b/test/server/server_test.cc index d9743555ee33a..cc17e0cc2ad32 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1654,13 +1654,20 @@ TEST_P(ServerInstanceImplTest, JsonApplicationLog) { ENVOY_LOG_MISC(info, "hello"); } -TEST_P(ServerInstanceImplTest, JsonApplicationLogFailWithForbiddenFlags) { +TEST_P(ServerInstanceImplTest, JsonApplicationLogFailWithForbiddenFlagv) { EXPECT_THROW_WITH_MESSAGE( - initialize("test/server/test_data/server/json_application_log_forbidden_flag.yaml"), + initialize("test/server/test_data/server/json_application_log_forbidden_flagv.yaml"), EnvoyException, "setJsonLogFormat error: INVALID_ARGUMENT: Usage of %v is unavailable for JSON log formats"); } +TEST_P(ServerInstanceImplTest, JsonApplicationLogFailWithForbiddenFlag_) { + EXPECT_THROW_WITH_MESSAGE( + initialize("test/server/test_data/server/json_application_log_forbidden_flag_.yaml"), + EnvoyException, + "setJsonLogFormat error: INVALID_ARGUMENT: Usage of %_ is unavailable for JSON log formats"); +} + } // namespace } // namespace Server } // namespace Envoy diff --git a/test/server/test_data/server/json_application_log.yaml b/test/server/test_data/server/json_application_log.yaml index 355fab882699d..57f453a572454 100644 --- a/test/server/test_data/server/json_application_log.yaml +++ b/test/server/test_data/server/json_application_log.yaml @@ -1,7 +1,7 @@ --- application_log_format: json_format: - MessageFromProto: "%_" + MessageFromProto: "%j" admin: address: diff --git a/test/server/test_data/server/json_application_log_forbidden_flag_.yaml b/test/server/test_data/server/json_application_log_forbidden_flag_.yaml new file mode 100644 index 0000000000000..355fab882699d --- /dev/null +++ b/test/server/test_data/server/json_application_log_forbidden_flag_.yaml @@ -0,0 +1,10 @@ +--- +application_log_format: + json_format: + MessageFromProto: "%_" + +admin: + address: + socket_address: + address: 0.0.0.0 + port_value: 9000 diff --git a/test/server/test_data/server/json_application_log_forbidden_flag.yaml b/test/server/test_data/server/json_application_log_forbidden_flagv.yaml similarity index 100% rename from test/server/test_data/server/json_application_log_forbidden_flag.yaml rename to test/server/test_data/server/json_application_log_forbidden_flagv.yaml From 66b5ddbfdcc48c60245468735440af35ccf2cb18 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 2 Jun 2023 03:06:41 +0300 Subject: [PATCH 25/30] forbid both cli and bootstrap log format Signed-off-by: ohadvano --- changelogs/current.yaml | 4 ++++ .../observability/application_logging.rst | 4 ++-- source/server/config_validation/server.cc | 7 +++++- source/server/server.cc | 7 +++++- test/server/config_validation/server_test.cc | 24 +++++++------------ test/server/server_test.cc | 16 ++++--------- 6 files changed, 30 insertions(+), 32 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 7f7af0bd7f0fa..887409af6e099 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -226,6 +226,10 @@ new_features: change: | added new field ``envoy.extensions.filters.http.fault.v3.HTTPFault.filter_metadata`` to aid in logging. Metadata will be stored in StreamInfo dynamic metadata under a namespace corresponding to the name of the fault filter. +- area: application_logs + change: | + Added bootstrap option :ref:`application_log_format ` + to enable setting application log format as JSON structure. - area: ext_proc change: | added new field ``filter_metadata ` to print the logs in custom JSON format. The json format struct can support all the format flags that are specified in :ref:`command line options `, -except for the ``%v`` and ``%_`` flag, as they may break the JSON structure log. Instead, use the ``%j`` flag. Example: +except for the ``%v`` and ``%_`` flags, as they may break the JSON structure log. Instead, use the ``%j`` flag. Example: .. code-block:: yaml @@ -43,4 +43,4 @@ except for the ``%v`` and ``%_`` flag, as they may break the JSON structure log. FixedValue: "SomeFixedValue" .. note:: - In case the CLI option ``--log-format`` is used, its value will override ``application_log_format`` format. + Setting both ``application_log_format`` and CLI option ``--log-format`` is not allowed, and will cause a bootstrap error. diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 4c677ff20e02f..39bf10364ee21 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -85,7 +85,12 @@ void ValidationInstance::initialize(const Options& options, InstanceUtil::loadBootstrapConfig(bootstrap_, options, messageValidationContext().staticValidationVisitor(), *api_); - if (!options.logFormatSet() && bootstrap_.has_application_log_format() && + if (options_.logFormatSet() && bootstrap_.has_application_log_format()) { + throw EnvoyException( + "Only one of application_log_format or CLI option --log-format can be specified."); + } + + if (bootstrap_.has_application_log_format() && bootstrap_.application_log_format().has_json_format()) { const auto status = Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); diff --git a/source/server/server.cc b/source/server/server.cc index e8ad3224716a8..6b4b9a27c5b0c 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -422,7 +422,12 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add messageValidationContext().staticValidationVisitor(), *api_); bootstrap_config_update_time_ = time_source_.systemTime(); - if (!options_.logFormatSet() && bootstrap_.has_application_log_format() && + if (options_.logFormatSet() && bootstrap_.has_application_log_format()) { + throw EnvoyException( + "Only one of application_log_format or CLI option --log-format can be specified."); + } + + if (bootstrap_.has_application_log_format() && bootstrap_.application_log_format().has_json_format()) { const auto status = Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index a534d18279093..3c031e5b61687 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -250,26 +250,18 @@ INSTANTIATE_TEST_SUITE_P( AllConfigs, RuntimeFeatureValidationServerTest, ::testing::ValuesIn(RuntimeFeatureValidationServerTest::getAllConfigFiles())); -TEST_P(JsonApplicationLogsValidationServerTest, OptionOverridesJsonApplicationLogsConfig) { +TEST_P(JsonApplicationLogsValidationServerTest, BootstrapApplicationLogsAndCLIThrows) { Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; DangerousDeprecatedTestTime time_system; EXPECT_CALL(options_, logFormatSet()).WillRepeatedly(Return(true)); - ValidationInstance server(options_, time_system.timeSystem(), - Network::Address::InstanceConstSharedPtr(), stats_store, - access_log_lock, component_factory_, Thread::threadFactoryForTest(), - Filesystem::fileSystemForTest()); - - Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - MockLogSink sink(Envoy::Logger::Registry::getSink()); - EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { - EXPECT_THAT(msg, HasSubstr("[info][misc]")); - EXPECT_THAT(msg, HasSubstr("hello")); - EXPECT_EQ(log.logger_name, "misc"); - })); - - ENVOY_LOG_MISC(info, "hello"); - server.shutdown(); + EXPECT_THROW_WITH_MESSAGE( + ValidationInstance server(options_, time_system.timeSystem(), + Network::Address::InstanceConstSharedPtr(), stats_store, + access_log_lock, component_factory_, Thread::threadFactoryForTest(), + Filesystem::fileSystemForTest()), + EnvoyException, + "Only one of application_log_format or CLI option --log-format can be specified."); } TEST_P(JsonApplicationLogsValidationServerTest, JsonApplicationLogs) { diff --git a/test/server/server_test.cc b/test/server/server_test.cc index cc17e0cc2ad32..5378d9b32d5b7 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1625,19 +1625,11 @@ TEST_P(ServerInstanceImplTest, AdminAccessLogFilter) { EXPECT_NO_THROW(initialize("test/server/test_data/server/access_log_filter_bootstrap.yaml")); } -TEST_P(ServerInstanceImplTest, OptionOverridesJsonApplicationLogsConfig) { +TEST_P(ServerInstanceImplTest, BootstrapApplicationLogsAndCLIThrows) { EXPECT_CALL(options_, logFormatSet()).WillRepeatedly(Return(true)); - EXPECT_NO_THROW(initialize("test/server/test_data/server/json_application_log.yaml")); - - Envoy::Logger::Registry::setLogLevel(spdlog::level::info); - MockLogSink sink(Envoy::Logger::Registry::getSink()); - EXPECT_CALL(sink, log(_, _)).WillOnce(Invoke([](auto msg, auto& log) { - EXPECT_THAT(msg, HasSubstr("[info][misc]")); - EXPECT_THAT(msg, HasSubstr("hello")); - EXPECT_EQ(log.logger_name, "misc"); - })); - - ENVOY_LOG_MISC(info, "hello"); + EXPECT_THROW_WITH_MESSAGE( + initialize("test/server/test_data/server/json_application_log.yaml"), EnvoyException, + "Only one of application_log_format or CLI option --log-format can be specified."); } TEST_P(ServerInstanceImplTest, JsonApplicationLog) { From 776b80aac0112300455d737c00643be50be4f3ed Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 2 Jun 2023 13:07:43 +0300 Subject: [PATCH 26/30] move common code to utils Signed-off-by: ohadvano --- source/server/BUILD | 2 + source/server/config_validation/BUILD | 1 + source/server/config_validation/server.cc | 17 +----- source/server/server.cc | 16 +---- source/server/utils.cc | 22 +++++++ source/server/utils.h | 7 +++ test/server/BUILD | 1 + test/server/utils_test.cc | 73 +++++++++++++++++++++++ tools/code_format/config.yaml | 1 + 9 files changed, 112 insertions(+), 28 deletions(-) diff --git a/source/server/BUILD b/source/server/BUILD index a5709a05c6e1d..fc81eafc00f38 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -513,7 +513,9 @@ envoy_cc_library( hdrs = ["utils.h"], deps = [ "//envoy/init:manager_interface", + "//envoy/server:options_interface", "//source/common/common:assert_lib", "@envoy_api//envoy/admin/v3:pkg_cc_proto", + "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", ], ) diff --git a/source/server/config_validation/BUILD b/source/server/config_validation/BUILD index 5d79fefce13a4..f5e8bd30b1d90 100644 --- a/source/server/config_validation/BUILD +++ b/source/server/config_validation/BUILD @@ -101,6 +101,7 @@ envoy_cc_library( "//source/common/version:version_lib", "//source/server:configuration_lib", "//source/server:server_lib", + "//source/server:utils_lib", "//source/server/admin:admin_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 39bf10364ee21..5a1a1746e2488 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -15,6 +15,7 @@ #include "source/server/listener_manager_factory.h" #include "source/server/regex_engine.h" #include "source/server/ssl_context_manager.h" +#include "source/server/utils.h" namespace Envoy { namespace Server { @@ -85,20 +86,8 @@ void ValidationInstance::initialize(const Options& options, InstanceUtil::loadBootstrapConfig(bootstrap_, options, messageValidationContext().staticValidationVisitor(), *api_); - if (options_.logFormatSet() && bootstrap_.has_application_log_format()) { - throw EnvoyException( - "Only one of application_log_format or CLI option --log-format can be specified."); - } - - if (bootstrap_.has_application_log_format() && - bootstrap_.application_log_format().has_json_format()) { - const auto status = - Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); - - if (!status.ok()) { - throw EnvoyException(fmt::format("setJsonLogFormat error: {}", status.ToString())); - } - } + Utility::assertExclusiveLogFormatMethod(options_, bootstrap_); + Utility::maybeSetApplicationLogFormat(bootstrap_); // Inject regex engine to singleton. Regex::EnginePtr regex_engine = createRegexEngine( diff --git a/source/server/server.cc b/source/server/server.cc index 6b4b9a27c5b0c..fe7a94ff43d9b 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -422,20 +422,8 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add messageValidationContext().staticValidationVisitor(), *api_); bootstrap_config_update_time_ = time_source_.systemTime(); - if (options_.logFormatSet() && bootstrap_.has_application_log_format()) { - throw EnvoyException( - "Only one of application_log_format or CLI option --log-format can be specified."); - } - - if (bootstrap_.has_application_log_format() && - bootstrap_.application_log_format().has_json_format()) { - const auto status = - Logger::Registry::setJsonLogFormat(bootstrap_.application_log_format().json_format()); - - if (!status.ok()) { - throw EnvoyException(fmt::format("setJsonLogFormat error: {}", status.ToString())); - } - } + Utility::assertExclusiveLogFormatMethod(options_, bootstrap_); + Utility::maybeSetApplicationLogFormat(bootstrap_); #ifdef ENVOY_PERFETTO perfetto::TracingInitArgs args; diff --git a/source/server/utils.cc b/source/server/utils.cc index fcb5e043ec999..4e668a771694e 100644 --- a/source/server/utils.cc +++ b/source/server/utils.cc @@ -1,5 +1,7 @@ #include "source/server/utils.h" +#include "envoy/common/exception.h" + #include "source/common/common/assert.h" namespace Envoy { @@ -21,6 +23,26 @@ envoy::admin::v3::ServerInfo::State serverState(Init::Manager::State state, return envoy::admin::v3::ServerInfo::PRE_INITIALIZING; } +void assertExclusiveLogFormatMethod(const Options& options, + const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + if (options.logFormatSet() && bootstrap.has_application_log_format()) { + throw EnvoyException( + "Only one of application_log_format or CLI option --log-format can be specified."); + } +} + +void maybeSetApplicationLogFormat(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + if (bootstrap.has_application_log_format() && + bootstrap.application_log_format().has_json_format()) { + const auto status = + Logger::Registry::setJsonLogFormat(bootstrap.application_log_format().json_format()); + + if (!status.ok()) { + throw EnvoyException(fmt::format("setJsonLogFormat error: {}", status.ToString())); + } + } +} + } // namespace Utility } // namespace Server } // namespace Envoy diff --git a/source/server/utils.h b/source/server/utils.h index 1ab4b51de275c..ca18bbe218ac5 100644 --- a/source/server/utils.h +++ b/source/server/utils.h @@ -1,7 +1,9 @@ #pragma once #include "envoy/admin/v3/server_info.pb.h" +#include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/init/manager.h" +#include "envoy/server/options.h" namespace Envoy { namespace Server { @@ -14,6 +16,11 @@ namespace Utility { envoy::admin::v3::ServerInfo::State serverState(Init::Manager::State state, bool health_check_failed); +void assertExclusiveLogFormatMethod(const Options& options, + const envoy::config::bootstrap::v3::Bootstrap& bootstrap); + +void maybeSetApplicationLogFormat(const envoy::config::bootstrap::v3::Bootstrap& bootstrap); + } // namespace Utility } // namespace Server } // namespace Envoy diff --git a/test/server/BUILD b/test/server/BUILD index 74ae0259db08f..e2c210dc293e0 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -397,5 +397,6 @@ envoy_cc_test( srcs = envoy_select_admin_functionality(["utils_test.cc"]), deps = [ "//source/server:utils_lib", + "//test/mocks/server:options_mocks", ], ) diff --git a/test/server/utils_test.cc b/test/server/utils_test.cc index 1e2ae258f5a02..5096731111a82 100644 --- a/test/server/utils_test.cc +++ b/test/server/utils_test.cc @@ -1,9 +1,12 @@ #include "source/server/utils.h" +#include "test/mocks/server/options.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" +using testing::Return; + namespace Envoy { namespace Server { namespace Utility { @@ -15,6 +18,76 @@ TEST(UtilsTest, BadServerState) { EXPECT_ENVOY_BUG(Utility::serverState(static_cast(123), true), "unexpected server state"); } + +TEST(UtilsTest, AssertExclusiveLogFormatMethod) { + { + testing::NiceMock options; + envoy::config::bootstrap::v3::Bootstrap bootstrap; + EXPECT_NO_THROW(Utility::assertExclusiveLogFormatMethod(options, bootstrap)); + } + + { + testing::NiceMock options; + envoy::config::bootstrap::v3::Bootstrap bootstrap; + EXPECT_CALL(options, logFormatSet()).WillRepeatedly(Return(true)); + EXPECT_NO_THROW(Utility::assertExclusiveLogFormatMethod(options, bootstrap)); + } + + { + testing::NiceMock options; + envoy::config::bootstrap::v3::Bootstrap bootstrap; + bootstrap.mutable_application_log_format()->mutable_json_format(); + EXPECT_NO_THROW(Utility::assertExclusiveLogFormatMethod(options, bootstrap)); + } + + { + testing::NiceMock options; + envoy::config::bootstrap::v3::Bootstrap bootstrap; + EXPECT_CALL(options, logFormatSet()).WillRepeatedly(Return(true)); + bootstrap.mutable_application_log_format()->mutable_json_format(); + EXPECT_THROW_WITH_MESSAGE( + Utility::assertExclusiveLogFormatMethod(options, bootstrap), EnvoyException, + "Only one of application_log_format or CLI option --log-format can be specified."); + } +} + +TEST(UtilsTest, MaybeSetApplicationLogFormat) { + { + envoy::config::bootstrap::v3::Bootstrap bootstrap; + EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(bootstrap)); + } + + { + envoy::config::bootstrap::v3::Bootstrap bootstrap; + bootstrap.mutable_application_log_format(); + EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(bootstrap)); + } + + { + envoy::config::bootstrap::v3::Bootstrap bootstrap; + bootstrap.mutable_application_log_format()->mutable_json_format(); + EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(bootstrap)); + } + + { + envoy::config::bootstrap::v3::Bootstrap bootstrap; + auto* format = bootstrap.mutable_application_log_format()->mutable_json_format(); + format->mutable_fields()->operator[]("Message").set_string_value("%v"); + EXPECT_THROW_WITH_MESSAGE(Utility::maybeSetApplicationLogFormat(bootstrap), EnvoyException, + "setJsonLogFormat error: INVALID_ARGUMENT: Usage of %v is " + "unavailable for JSON log formats"); + } + + { + envoy::config::bootstrap::v3::Bootstrap bootstrap; + auto* format = bootstrap.mutable_application_log_format()->mutable_json_format(); + format->mutable_fields()->operator[]("Message").set_string_value("%_"); + EXPECT_THROW_WITH_MESSAGE(Utility::maybeSetApplicationLogFormat(bootstrap), EnvoyException, + "setJsonLogFormat error: INVALID_ARGUMENT: Usage of %_ is " + "unavailable for JSON log formats"); + } +} + } // namespace Utility } // namespace Server } // namespace Envoy diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index fed59e9247439..f9c7f231773c8 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -208,6 +208,7 @@ paths: - source/server/config_validation/server.cc - source/server/admin/html/active_stats.js - source/server/server.cc + - source/server/utils.cc - source/server/configuration_impl.h - source/server/hot_restarting_base.cc - source/server/hot_restart_impl.cc From 9659e53f6fccc1459cf877759cf059c24b149d8b Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 2 Jun 2023 14:45:37 +0300 Subject: [PATCH 27/30] nest log format config within general ApplicationLogConfig message Signed-off-by: ohadvano --- api/envoy/config/bootstrap/v3/bootstrap.proto | 29 ++++++----- changelogs/current.yaml | 2 +- .../observability/application_logging.rst | 21 ++++---- source/server/config_validation/server.cc | 6 ++- source/server/server.cc | 6 ++- source/server/utils.cc | 18 ++++--- source/server/utils.h | 8 +-- test/server/config_validation/server_test.cc | 2 +- .../test_data/json_application_logs.yaml | 7 +-- ...json_application_logs_forbidden_flag_.yaml | 7 +-- ...json_application_logs_forbidden_flagv.yaml | 7 +-- test/server/server_test.cc | 2 +- .../server/json_application_log.yaml | 7 +-- .../json_application_log_forbidden_flag_.yaml | 7 +-- .../json_application_log_forbidden_flagv.yaml | 7 +-- test/server/utils_test.cc | 50 +++++++++---------- 16 files changed, 103 insertions(+), 83 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index 948cf6513ccea..a35a63eb6712b 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -101,15 +101,22 @@ message Bootstrap { core.v3.ApiConfigSource ads_config = 3; } - message ApplicationLogFormat { - oneof log_format { - option (validate.required) = true; - - // Flush application logs in JSON format. The configured JSON struct can - // support all the format tags specified in the in :option:`--log-format` - // command line option section, except for the ``%v`` and ``%_`` flags. - google.protobuf.Struct json_format = 1; + message ApplicationLogConfig { + message LogFormat { + oneof log_format { + option (validate.required) = true; + + // Flush application logs in JSON format. The configured JSON struct can + // support all the format tags specified in the in :option:`--log-format` + // command line option section, except for the ``%v`` and ``%_`` flags. + google.protobuf.Struct json_format = 1; + } } + + // Optional field to set the application logs format. If this field is set, it will override + // the default log format. In case the :option:`--log-format` command line option is used, + // it will override ``application_log_format`` configurations. + LogFormat log_format = 1; } reserved 10, 11; @@ -372,10 +379,8 @@ message Bootstrap { // supports ApiListenerManager. core.v3.TypedExtensionConfig listener_manager = 37; - // Optional field to set the application logs format. If this field is set, it will override - // the default log format. In case the :option:`--log-format` command line option is used, - // it will override ``application_log_format`` configurations. - ApplicationLogFormat application_log_format = 38; + // Optional application log configuration. + ApplicationLogConfig application_log_config = 38; } // Administration interface :ref:`operations documentation diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 8836334c502cd..4bb7e582ea2be 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -286,7 +286,7 @@ new_features: Metadata will be stored in StreamInfo dynamic metadata under a namespace corresponding to the name of the fault filter. - area: application_logs change: | - Added bootstrap option :ref:`application_log_format ` + Added bootstrap option :ref:`application_log_format ` to enable setting application log format as JSON structure. - area: ext_proc change: | diff --git a/docs/root/configuration/observability/application_logging.rst b/docs/root/configuration/observability/application_logging.rst index 866a76ba1b7c8..38c966491d4a5 100644 --- a/docs/root/configuration/observability/application_logging.rst +++ b/docs/root/configuration/observability/application_logging.rst @@ -27,20 +27,21 @@ with the following :ref:`command line options `: Printing logs in JSON format ---------------------------- -It is possible to use the bootstrap config :ref:`json_format ` +It is possible to use the bootstrap config :ref:`json_format ` to print the logs in custom JSON format. The json format struct can support all the format flags that are specified in :ref:`command line options `, except for the ``%v`` and ``%_`` flags, as they may break the JSON structure log. Instead, use the ``%j`` flag. Example: .. code-block:: yaml - application_log_format: - json_format: - Timestamp: "%Y-%m-%dT%T.%F" - ThreadId: "%t" - SourceLine: "%s:%#" - Level: "%l" - Message: "%j" - FixedValue: "SomeFixedValue" + application_log_config: + log_format: + json_format: + Timestamp: "%Y-%m-%dT%T.%F" + ThreadId: "%t" + SourceLine: "%s:%#" + Level: "%l" + Message: "%j" + FixedValue: "SomeFixedValue" .. note:: - Setting both ``application_log_format`` and CLI option ``--log-format`` is not allowed, and will cause a bootstrap error. + Setting both ``application_log_config:log_format`` and CLI option ``--log-format`` is not allowed, and will cause a bootstrap error. diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 5a1a1746e2488..dc80c3e7145be 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -86,8 +86,10 @@ void ValidationInstance::initialize(const Options& options, InstanceUtil::loadBootstrapConfig(bootstrap_, options, messageValidationContext().staticValidationVisitor(), *api_); - Utility::assertExclusiveLogFormatMethod(options_, bootstrap_); - Utility::maybeSetApplicationLogFormat(bootstrap_); + if (bootstrap_.has_application_log_config()) { + Utility::assertExclusiveLogFormatMethod(options_, bootstrap_.application_log_config()); + Utility::maybeSetApplicationLogFormat(bootstrap_.application_log_config()); + } // Inject regex engine to singleton. Regex::EnginePtr regex_engine = createRegexEngine( diff --git a/source/server/server.cc b/source/server/server.cc index fe7a94ff43d9b..8ed78eb8b139c 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -422,8 +422,10 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add messageValidationContext().staticValidationVisitor(), *api_); bootstrap_config_update_time_ = time_source_.systemTime(); - Utility::assertExclusiveLogFormatMethod(options_, bootstrap_); - Utility::maybeSetApplicationLogFormat(bootstrap_); + if (bootstrap_.has_application_log_config()) { + Utility::assertExclusiveLogFormatMethod(options_, bootstrap_.application_log_config()); + Utility::maybeSetApplicationLogFormat(bootstrap_.application_log_config()); + } #ifdef ENVOY_PERFETTO perfetto::TracingInitArgs args; diff --git a/source/server/utils.cc b/source/server/utils.cc index 4e668a771694e..7bb8957a59c67 100644 --- a/source/server/utils.cc +++ b/source/server/utils.cc @@ -23,19 +23,21 @@ envoy::admin::v3::ServerInfo::State serverState(Init::Manager::State state, return envoy::admin::v3::ServerInfo::PRE_INITIALIZING; } -void assertExclusiveLogFormatMethod(const Options& options, - const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - if (options.logFormatSet() && bootstrap.has_application_log_format()) { +void assertExclusiveLogFormatMethod( + const Options& options, + const envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig& application_log_config) { + if (options.logFormatSet() && application_log_config.has_log_format()) { throw EnvoyException( - "Only one of application_log_format or CLI option --log-format can be specified."); + "Only one of ApplicationLogConfig.log_format or CLI option --log-format can be specified."); } } -void maybeSetApplicationLogFormat(const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { - if (bootstrap.has_application_log_format() && - bootstrap.application_log_format().has_json_format()) { +void maybeSetApplicationLogFormat( + const envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig& application_log_config) { + if (application_log_config.has_log_format() && + application_log_config.log_format().has_json_format()) { const auto status = - Logger::Registry::setJsonLogFormat(bootstrap.application_log_format().json_format()); + Logger::Registry::setJsonLogFormat(application_log_config.log_format().json_format()); if (!status.ok()) { throw EnvoyException(fmt::format("setJsonLogFormat error: {}", status.ToString())); diff --git a/source/server/utils.h b/source/server/utils.h index ca18bbe218ac5..2d3b981c2c87d 100644 --- a/source/server/utils.h +++ b/source/server/utils.h @@ -16,10 +16,12 @@ namespace Utility { envoy::admin::v3::ServerInfo::State serverState(Init::Manager::State state, bool health_check_failed); -void assertExclusiveLogFormatMethod(const Options& options, - const envoy::config::bootstrap::v3::Bootstrap& bootstrap); +void assertExclusiveLogFormatMethod( + const Options& options, + const envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig& application_log_config); -void maybeSetApplicationLogFormat(const envoy::config::bootstrap::v3::Bootstrap& bootstrap); +void maybeSetApplicationLogFormat( + const envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig& application_log_config); } // namespace Utility } // namespace Server diff --git a/test/server/config_validation/server_test.cc b/test/server/config_validation/server_test.cc index 3c031e5b61687..cda36802a3dd8 100644 --- a/test/server/config_validation/server_test.cc +++ b/test/server/config_validation/server_test.cc @@ -261,7 +261,7 @@ TEST_P(JsonApplicationLogsValidationServerTest, BootstrapApplicationLogsAndCLITh access_log_lock, component_factory_, Thread::threadFactoryForTest(), Filesystem::fileSystemForTest()), EnvoyException, - "Only one of application_log_format or CLI option --log-format can be specified."); + "Only one of ApplicationLogConfig.log_format or CLI option --log-format can be specified."); } TEST_P(JsonApplicationLogsValidationServerTest, JsonApplicationLogs) { diff --git a/test/server/config_validation/test_data/json_application_logs.yaml b/test/server/config_validation/test_data/json_application_logs.yaml index 57f453a572454..2510444d98408 100644 --- a/test/server/config_validation/test_data/json_application_logs.yaml +++ b/test/server/config_validation/test_data/json_application_logs.yaml @@ -1,7 +1,8 @@ --- -application_log_format: - json_format: - MessageFromProto: "%j" +application_log_config: + log_format: + json_format: + MessageFromProto: "%j" admin: address: diff --git a/test/server/config_validation/test_data/json_application_logs_forbidden_flag_.yaml b/test/server/config_validation/test_data/json_application_logs_forbidden_flag_.yaml index 355fab882699d..3b3b8164adaea 100644 --- a/test/server/config_validation/test_data/json_application_logs_forbidden_flag_.yaml +++ b/test/server/config_validation/test_data/json_application_logs_forbidden_flag_.yaml @@ -1,7 +1,8 @@ --- -application_log_format: - json_format: - MessageFromProto: "%_" +application_log_config: + log_format: + json_format: + MessageFromProto: "%_" admin: address: diff --git a/test/server/config_validation/test_data/json_application_logs_forbidden_flagv.yaml b/test/server/config_validation/test_data/json_application_logs_forbidden_flagv.yaml index 132cf9d06716f..16474cea24cd4 100644 --- a/test/server/config_validation/test_data/json_application_logs_forbidden_flagv.yaml +++ b/test/server/config_validation/test_data/json_application_logs_forbidden_flagv.yaml @@ -1,7 +1,8 @@ --- -application_log_format: - json_format: - MessageFromProto: "%v" +application_log_config: + log_format: + json_format: + MessageFromProto: "%v" admin: address: diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 5378d9b32d5b7..c47df5d57c57f 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1629,7 +1629,7 @@ TEST_P(ServerInstanceImplTest, BootstrapApplicationLogsAndCLIThrows) { EXPECT_CALL(options_, logFormatSet()).WillRepeatedly(Return(true)); EXPECT_THROW_WITH_MESSAGE( initialize("test/server/test_data/server/json_application_log.yaml"), EnvoyException, - "Only one of application_log_format or CLI option --log-format can be specified."); + "Only one of ApplicationLogConfig.log_format or CLI option --log-format can be specified."); } TEST_P(ServerInstanceImplTest, JsonApplicationLog) { diff --git a/test/server/test_data/server/json_application_log.yaml b/test/server/test_data/server/json_application_log.yaml index 57f453a572454..2510444d98408 100644 --- a/test/server/test_data/server/json_application_log.yaml +++ b/test/server/test_data/server/json_application_log.yaml @@ -1,7 +1,8 @@ --- -application_log_format: - json_format: - MessageFromProto: "%j" +application_log_config: + log_format: + json_format: + MessageFromProto: "%j" admin: address: diff --git a/test/server/test_data/server/json_application_log_forbidden_flag_.yaml b/test/server/test_data/server/json_application_log_forbidden_flag_.yaml index 355fab882699d..3b3b8164adaea 100644 --- a/test/server/test_data/server/json_application_log_forbidden_flag_.yaml +++ b/test/server/test_data/server/json_application_log_forbidden_flag_.yaml @@ -1,7 +1,8 @@ --- -application_log_format: - json_format: - MessageFromProto: "%_" +application_log_config: + log_format: + json_format: + MessageFromProto: "%_" admin: address: diff --git a/test/server/test_data/server/json_application_log_forbidden_flagv.yaml b/test/server/test_data/server/json_application_log_forbidden_flagv.yaml index 132cf9d06716f..16474cea24cd4 100644 --- a/test/server/test_data/server/json_application_log_forbidden_flagv.yaml +++ b/test/server/test_data/server/json_application_log_forbidden_flagv.yaml @@ -1,7 +1,8 @@ --- -application_log_format: - json_format: - MessageFromProto: "%v" +application_log_config: + log_format: + json_format: + MessageFromProto: "%v" admin: address: diff --git a/test/server/utils_test.cc b/test/server/utils_test.cc index 5096731111a82..7576e41522c1b 100644 --- a/test/server/utils_test.cc +++ b/test/server/utils_test.cc @@ -22,67 +22,67 @@ TEST(UtilsTest, BadServerState) { TEST(UtilsTest, AssertExclusiveLogFormatMethod) { { testing::NiceMock options; - envoy::config::bootstrap::v3::Bootstrap bootstrap; - EXPECT_NO_THROW(Utility::assertExclusiveLogFormatMethod(options, bootstrap)); + envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config; + EXPECT_NO_THROW(Utility::assertExclusiveLogFormatMethod(options, log_config)); } { testing::NiceMock options; - envoy::config::bootstrap::v3::Bootstrap bootstrap; + envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config; EXPECT_CALL(options, logFormatSet()).WillRepeatedly(Return(true)); - EXPECT_NO_THROW(Utility::assertExclusiveLogFormatMethod(options, bootstrap)); + EXPECT_NO_THROW(Utility::assertExclusiveLogFormatMethod(options, log_config)); } { testing::NiceMock options; - envoy::config::bootstrap::v3::Bootstrap bootstrap; - bootstrap.mutable_application_log_format()->mutable_json_format(); - EXPECT_NO_THROW(Utility::assertExclusiveLogFormatMethod(options, bootstrap)); + envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config; + log_config.mutable_log_format(); + EXPECT_NO_THROW(Utility::assertExclusiveLogFormatMethod(options, log_config)); } { testing::NiceMock options; - envoy::config::bootstrap::v3::Bootstrap bootstrap; + envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config; EXPECT_CALL(options, logFormatSet()).WillRepeatedly(Return(true)); - bootstrap.mutable_application_log_format()->mutable_json_format(); + log_config.mutable_log_format(); EXPECT_THROW_WITH_MESSAGE( - Utility::assertExclusiveLogFormatMethod(options, bootstrap), EnvoyException, - "Only one of application_log_format or CLI option --log-format can be specified."); + Utility::assertExclusiveLogFormatMethod(options, log_config), EnvoyException, + "Only one of ApplicationLogConfig.log_format or CLI option --log-format can be specified."); } } TEST(UtilsTest, MaybeSetApplicationLogFormat) { { - envoy::config::bootstrap::v3::Bootstrap bootstrap; - EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(bootstrap)); + envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config; + EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(log_config)); } { - envoy::config::bootstrap::v3::Bootstrap bootstrap; - bootstrap.mutable_application_log_format(); - EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(bootstrap)); + envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config; + log_config.mutable_log_format(); + EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(log_config)); } { - envoy::config::bootstrap::v3::Bootstrap bootstrap; - bootstrap.mutable_application_log_format()->mutable_json_format(); - EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(bootstrap)); + envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config; + log_config.mutable_log_format()->mutable_json_format(); + EXPECT_NO_THROW(Utility::maybeSetApplicationLogFormat(log_config)); } { - envoy::config::bootstrap::v3::Bootstrap bootstrap; - auto* format = bootstrap.mutable_application_log_format()->mutable_json_format(); + envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config; + auto* format = log_config.mutable_log_format()->mutable_json_format(); format->mutable_fields()->operator[]("Message").set_string_value("%v"); - EXPECT_THROW_WITH_MESSAGE(Utility::maybeSetApplicationLogFormat(bootstrap), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(Utility::maybeSetApplicationLogFormat(log_config), EnvoyException, "setJsonLogFormat error: INVALID_ARGUMENT: Usage of %v is " "unavailable for JSON log formats"); } { - envoy::config::bootstrap::v3::Bootstrap bootstrap; - auto* format = bootstrap.mutable_application_log_format()->mutable_json_format(); + envoy::config::bootstrap::v3::Bootstrap::ApplicationLogConfig log_config; + auto* format = log_config.mutable_log_format()->mutable_json_format(); format->mutable_fields()->operator[]("Message").set_string_value("%_"); - EXPECT_THROW_WITH_MESSAGE(Utility::maybeSetApplicationLogFormat(bootstrap), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(Utility::maybeSetApplicationLogFormat(log_config), EnvoyException, "setJsonLogFormat error: INVALID_ARGUMENT: Usage of %_ is " "unavailable for JSON log formats"); } From 89d2c40309592f7c703ea850e156abb89684e0f8 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 2 Jun 2023 14:59:05 +0300 Subject: [PATCH 28/30] fix format Signed-off-by: ohadvano --- api/envoy/config/bootstrap/v3/bootstrap.proto | 4 ++-- changelogs/current.yaml | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index a35a63eb6712b..4ba0136a4ace9 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -114,8 +114,8 @@ message Bootstrap { } // Optional field to set the application logs format. If this field is set, it will override - // the default log format. In case the :option:`--log-format` command line option is used, - // it will override ``application_log_format`` configurations. + // the default log format. Setting both this field and :option:`--log-format` command line + // option is not allowed, and will cause a bootstrap error. LogFormat log_format = 1; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 4bb7e582ea2be..efdbc4904c2d3 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -286,7 +286,8 @@ new_features: Metadata will be stored in StreamInfo dynamic metadata under a namespace corresponding to the name of the fault filter. - area: application_logs change: | - Added bootstrap option :ref:`application_log_format ` + Added bootstrap option + :ref:`application_log_format ` to enable setting application log format as JSON structure. - area: ext_proc change: | From 64a608d4d6d6e0f28df735e1766d44e1db63dc53 Mon Sep 17 00:00:00 2001 From: ohadvano Date: Fri, 2 Jun 2023 15:02:34 +0300 Subject: [PATCH 29/30] fix comment Signed-off-by: ohadvano --- api/envoy/config/bootstrap/v3/bootstrap.proto | 4 ++-- docs/root/configuration/observability/application_logging.rst | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index 4ba0136a4ace9..f171068aaeedc 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -107,8 +107,8 @@ message Bootstrap { option (validate.required) = true; // Flush application logs in JSON format. The configured JSON struct can - // support all the format tags specified in the in :option:`--log-format` - // command line option section, except for the ``%v`` and ``%_`` flags. + // support all the format flags specified in the :option:`--log-format` + // command line options section, except for the ``%v`` and ``%_`` flags. google.protobuf.Struct json_format = 1; } } diff --git a/docs/root/configuration/observability/application_logging.rst b/docs/root/configuration/observability/application_logging.rst index 38c966491d4a5..0e6c29a2a8b97 100644 --- a/docs/root/configuration/observability/application_logging.rst +++ b/docs/root/configuration/observability/application_logging.rst @@ -44,4 +44,4 @@ except for the ``%v`` and ``%_`` flags, as they may break the JSON structure log FixedValue: "SomeFixedValue" .. note:: - Setting both ``application_log_config:log_format`` and CLI option ``--log-format`` is not allowed, and will cause a bootstrap error. + Setting both ``application_log_config.log_format`` and CLI option ``--log-format`` is not allowed, and will cause a bootstrap error. From 48d649e6b4a081fc98b5dc56e3eb8bf42104970d Mon Sep 17 00:00:00 2001 From: ohadvano Date: Mon, 5 Jun 2023 18:04:07 +0300 Subject: [PATCH 30/30] update comment Signed-off-by: ohadvano --- mobile/test/common/integration/client_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mobile/test/common/integration/client_integration_test.cc b/mobile/test/common/integration/client_integration_test.cc index ac06e467d150b..13b2e3997e89d 100644 --- a/mobile/test/common/integration/client_integration_test.cc +++ b/mobile/test/common/integration/client_integration_test.cc @@ -97,7 +97,7 @@ void ClientIntegrationTest::trickleTest() { stream_prototype_->setOnData([this](envoy_data c_data, bool) { if (explicit_flow_control_) { - // Allow reading up to 100 bytes + // Allow reading up to 100 bytes. stream_->readData(100); } cc_.on_data_calls++;