From 3e7cc5aba2075d6f3a78b75df5d0157a7dd09377 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Mon, 5 Aug 2024 15:58:29 -0400 Subject: [PATCH] Make incompatible command-line args an error (#35570) Commit Message: Make incompatible command-line args an error Additional Description: Per #35073, `--enable-fine-grain-logging` and `--component-log-level` don't make sense at the same time, one of them will be overridden by the other in a way that can be surprising for the user. This PR makes it an error to apply both, so the user is not surprised by not getting the logs they expected at runtime. Risk Level: Small. May make existing command-lines with ineffective options fail. Testing: Added coverage. Docs Changes: Updated command-line options docs. Release Notes: Yes, under minor behavior changes. Platform Specific Features: n/a --------- Signed-off-by: Raven Black --- changelogs/current.yaml | 4 ++++ docs/root/operations/cli.rst | 3 ++- source/server/options_impl.cc | 5 +++++ test/server/options_impl_test.cc | 15 +++++++++++++-- 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index d2729ce93cd2..df8538bad5e4 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -13,6 +13,10 @@ behavior_changes: minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* +- area: command line options + change: | + :option:`--enable-fine-grain-logging` and :option:`--component-log-level` were incompatible in that one + would make the other ineffective. Setting both options at once is now an error, to reduce potential confusion. - area: tcp change: | Added support for :ref:`connection_pool_per_downstream_connection diff --git a/docs/root/operations/cli.rst b/docs/root/operations/cli.rst index cd33505380dc..aeb70858a233 100644 --- a/docs/root/operations/cli.rst +++ b/docs/root/operations/cli.rst @@ -109,6 +109,7 @@ following are the command line options that Envoy supports. never set this option. For example, if you want ``upstream`` component to run at ``debug`` level and ``connection`` component to run at ``trace`` level, you should pass ``upstream:debug,connection:trace`` to this flag. See ``ALL_LOGGER_IDS`` in :repo:`/source/common/common/logger.h` for a list of components. + This option is incompatible with :option:`--enable-fine-grain-logging`. .. option:: --cpuset-threads @@ -193,7 +194,7 @@ following are the command line options that Envoy supports. interface. If enabled, main log macros including ``ENVOY_LOG``, ``ENVOY_CONN_LOG``, ``ENVOY_STREAM_LOG`` and ``ENVOY_FLUSH_LOG`` will use a per-file logger, and the usage doesn't need ``Envoy::Logger::Loggable`` any more. The administration interface usage is similar. Please see :ref:`Administration interface - ` for more detail. + ` for more detail. This option is incompatible with :option:`--component-log-level`. .. option:: --socket-path diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index eaa6005bb260..b72e6237249f 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -214,7 +214,12 @@ 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(); + if (enable_fine_grain_logging_ && !component_log_level.getValue().empty()) { + throw MalformedArgvException( + "error: --component-log-level will not work with --enable-fine-grain-logging"); + } parseComponentLogLevels(component_log_level.getValue()); diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index 732ab9f03ed9..b03fdb15fee3 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -61,6 +61,13 @@ TEST_F(OptionsImplTest, InvalidMode) { EXPECT_THROW_WITH_REGEX(createOptionsImpl("envoy --mode bogus"), MalformedArgvException, "bogus"); } +TEST_F(OptionsImplTest, InvalidComponentLogLevelWithFineGrainLogging) { + EXPECT_THROW_WITH_REGEX( + createOptionsImpl("envoy --enable-fine-grain-logging --component-log-level http:info"), + MalformedArgvException, + "--component-log-level will not work with --enable-fine-grain-logging"); +} + TEST_F(OptionsImplTest, InvalidCommandLine) { EXPECT_THROW_WITH_REGEX(createOptionsImpl("envoy --blah"), MalformedArgvException, "Couldn't find match for argument"); @@ -98,7 +105,7 @@ TEST_F(OptionsImplTest, All) { "--file-flush-interval-msec 9000 " "--skip-hot-restart-on-no-parent " "--skip-hot-restart-parent-stats " - "--drain-time-s 60 --log-format [%v] --enable-fine-grain-logging --parent-shutdown-time-s 90 " + "--drain-time-s 60 --log-format [%v] --parent-shutdown-time-s 90 " "--log-path " "/foo/bar " "--disable-hot-restart --cpuset-threads --allow-unknown-static-fields " @@ -119,7 +126,7 @@ TEST_F(OptionsImplTest, All) { EXPECT_TRUE(options->skipHotRestartParentStats()); EXPECT_TRUE(options->skipHotRestartOnNoParent()); EXPECT_EQ("/foo/bar", options->logPath()); - EXPECT_EQ(true, options->enableFineGrainLogging()); + EXPECT_EQ(false, options->enableFineGrainLogging()); EXPECT_EQ("cluster", options->serviceClusterName()); EXPECT_EQ("node", options->serviceNodeName()); EXPECT_EQ("zone", options->serviceZone()); @@ -139,6 +146,10 @@ TEST_F(OptionsImplTest, All) { options = createOptionsImpl("envoy --mode init_only"); EXPECT_EQ(Server::Mode::InitOnly, options->mode()); + + // Tested separately because it's mutually exclusive with --component-log-level. + options = createOptionsImpl("envoy --enable-fine-grain-logging"); + EXPECT_EQ(true, options->enableFineGrainLogging()); } // Either variants of allow-unknown-[static-]-fields works.