Make incompatible command-line args an error (envoyproxy#35570)

Commit Message: Make incompatible command-line args an error
Additional Description: Per envoyproxy#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 <[email protected]>

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

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
-  <operations_admin_interface>` for more detail.
+  <operations_admin_interface>` for more detail. This option is incompatible with :option:`--component-log-level`.
 
 .. option:: --socket-path <path string>
 

source/server/options_impl.cc

@@ -214,7 +214,12 @@ OptionsImpl::OptionsImpl(std::vector<std::string> 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());
 

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.