[config] Better comma separated list support (#33182)

Allow for multiple `--grpc_experiments`, `--grpc_trace` command line
arguments to be added, accumulate them, and provide them to gRPC as one
thing.

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

---------

Co-authored-by: ctiller <ctiller@users.noreply.github.com>

CMakeLists.txt

@@ -1049,6 +1049,7 @@ if(gRPC_BUILD_TESTS)
   add_dependencies(buildtests_cxx latch_test)
   add_dependencies(buildtests_cxx lb_get_cpu_stats_test)
   add_dependencies(buildtests_cxx lb_load_data_store_test)
+  add_dependencies(buildtests_cxx load_config_test)
   if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_POSIX)
     add_dependencies(buildtests_cxx lock_free_event_test)
   endif()
@@ -14804,6 +14805,43 @@ target_link_libraries(lb_load_data_store_test
 )
 
 
+endif()
+if(gRPC_BUILD_TESTS)
+
+add_executable(load_config_test
+  test/core/config/load_config_test.cc
+  third_party/googletest/googletest/src/gtest-all.cc
+  third_party/googletest/googlemock/src/gmock-all.cc
+)
+target_compile_features(load_config_test PUBLIC cxx_std_14)
+target_include_directories(load_config_test
+  PRIVATE
+    ${CMAKE_CURRENT_SOURCE_DIR}
+    ${CMAKE_CURRENT_SOURCE_DIR}/include
+    ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR}
+    ${_gRPC_RE2_INCLUDE_DIR}
+    ${_gRPC_SSL_INCLUDE_DIR}
+    ${_gRPC_UPB_GENERATED_DIR}
+    ${_gRPC_UPB_GRPC_GENERATED_DIR}
+    ${_gRPC_UPB_INCLUDE_DIR}
+    ${_gRPC_XXHASH_INCLUDE_DIR}
+    ${_gRPC_ZLIB_INCLUDE_DIR}
+    third_party/googletest/googletest/include
+    third_party/googletest/googletest
+    third_party/googletest/googlemock/include
+    third_party/googletest/googlemock
+    ${_gRPC_PROTO_GENS_DIR}
+)
+
+target_link_libraries(load_config_test
+  ${_gRPC_BASELIB_LIBRARIES}
+  ${_gRPC_PROTOBUF_LIBRARIES}
+  ${_gRPC_ZLIB_LIBRARIES}
+  ${_gRPC_ALLTARGETS_LIBRARIES}
+  grpc
+)
+
+
 endif()
 if(gRPC_BUILD_TESTS)
 if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_POSIX)

build_autogenerated.yaml

@@ -9331,6 +9331,16 @@ targets:
   deps:
   - grpc++
   - grpc_test_util
+- name: load_config_test
+  gtest: true
+  build: test
+  language: c++
+  headers: []
+  src:
+  - test/core/config/load_config_test.cc
+  deps:
+  - grpc
+  uses_polling: false
 - name: lock_free_event_test
   gtest: true
   build: test

src/core/lib/config/config_vars.cc

@@ -20,6 +20,9 @@
 
 #include "src/core/lib/config/config_vars.h"
 
+#include <algorithm>
+#include <vector>
+
 #include "absl/flags/flag.h"
 #include "absl/strings/escaping.h"
 #include "absl/strings/str_cat.h"
@@ -36,46 +39,45 @@
 #define GRPC_ENABLE_FORK_SUPPORT_DEFAULT false
 #endif  // GRPC_ENABLE_FORK_SUPPORT
 
-ABSL_FLAG(absl::optional<std::string>, grpc_experiments, absl::nullopt,
+ABSL_FLAG(std::vector<std::string>, grpc_experiments, {},
           "A comma separated list of currently active experiments. Experiments "
           "may be prefixed with a '-' to disable them.");
 ABSL_FLAG(absl::optional<int32_t>, grpc_client_channel_backup_poll_interval_ms,
-          absl::nullopt,
+          {},
           "Declares the interval in ms between two backup polls on client "
           "channels. These polls are run in the timer thread so that gRPC can "
           "process connection failures while there is no active polling "
           "thread. They help reconnect disconnected client channels (mostly "
           "due to idleness), so that the next RPC on this channel won't fail. "
           "Set to 0 to turn off the backup polls.");
-ABSL_FLAG(absl::optional<std::string>, grpc_dns_resolver, absl::nullopt,
+ABSL_FLAG(absl::optional<std::string>, grpc_dns_resolver, {},
           "Declares which DNS resolver to use. The default is ares if gRPC is "
           "built with c-ares support. Otherwise, the value of this environment "
           "variable is ignored.");
-ABSL_FLAG(absl::optional<std::string>, grpc_trace, absl::nullopt,
+ABSL_FLAG(std::vector<std::string>, grpc_trace, {},
           "A comma separated list of tracers that provide additional insight "
           "into how gRPC C core is processing requests via debug logs.");
-ABSL_FLAG(absl::optional<std::string>, grpc_verbosity, absl::nullopt,
+ABSL_FLAG(absl::optional<std::string>, grpc_verbosity, {},
           "Default gRPC logging verbosity");
-ABSL_FLAG(absl::optional<std::string>, grpc_stacktrace_minloglevel,
+ABSL_FLAG(absl::optional<std::string>, grpc_stacktrace_minloglevel, {},
-          absl::nullopt,
           "Messages logged at the same or higher level than this will print "
           "stacktrace");
-ABSL_FLAG(absl::optional<bool>, grpc_enable_fork_support, absl::nullopt,
+ABSL_FLAG(absl::optional<bool>, grpc_enable_fork_support, {},
           "Enable fork support");
-ABSL_FLAG(absl::optional<std::string>, grpc_poll_strategy, absl::nullopt,
+ABSL_FLAG(absl::optional<std::string>, grpc_poll_strategy, {},
           "Declares which polling engines to try when starting gRPC. This is a "
           "comma-separated list of engines, which are tried in priority order "
           "first -> last.");
-ABSL_FLAG(absl::optional<bool>, grpc_abort_on_leaks, absl::nullopt,
+ABSL_FLAG(absl::optional<bool>, grpc_abort_on_leaks, {},
           "A debugging aid to cause a call to abort() when gRPC objects are "
           "leaked past grpc_shutdown()");
-ABSL_FLAG(absl::optional<std::string>, grpc_system_ssl_roots_dir, absl::nullopt,
+ABSL_FLAG(absl::optional<std::string>, grpc_system_ssl_roots_dir, {},
           "Custom directory to SSL Roots");
-ABSL_FLAG(absl::optional<std::string>, grpc_default_ssl_roots_file_path,
+ABSL_FLAG(absl::optional<std::string>, grpc_default_ssl_roots_file_path, {},
-          absl::nullopt, "Path to the default SSL roots file.");
+          "Path to the default SSL roots file.");
-ABSL_FLAG(absl::optional<bool>, grpc_not_use_system_ssl_roots, absl::nullopt,
+ABSL_FLAG(absl::optional<bool>, grpc_not_use_system_ssl_roots, {},
           "Disable loading system root certificates.");
-ABSL_FLAG(absl::optional<std::string>, grpc_ssl_cipher_suites, absl::nullopt,
+ABSL_FLAG(absl::optional<std::string>, grpc_ssl_cipher_suites, {},
           "A colon separated list of cipher suites to use with OpenSSL");
 
 namespace grpc_core {
@@ -94,11 +96,8 @@ ConfigVars::ConfigVars(const Overrides& overrides)
       not_use_system_ssl_roots_(LoadConfig(
           FLAGS_grpc_not_use_system_ssl_roots, "GRPC_NOT_USE_SYSTEM_SSL_ROOTS",
           overrides.not_use_system_ssl_roots, false)),
-      experiments_(LoadConfig(FLAGS_grpc_experiments, "GRPC_EXPERIMENTS",
-                              overrides.experiments, "")),
       dns_resolver_(LoadConfig(FLAGS_grpc_dns_resolver, "GRPC_DNS_RESOLVER",
                                overrides.dns_resolver, "")),
-      trace_(LoadConfig(FLAGS_grpc_trace, "GRPC_TRACE", overrides.trace, "")),
       verbosity_(LoadConfig(FLAGS_grpc_verbosity, "GRPC_VERBOSITY",
                             overrides.verbosity,
                             GPR_DEFAULT_LOG_VERBOSITY_STRING)),
@@ -113,6 +112,9 @@ ConfigVars::ConfigVars(const Overrides& overrides)
           "TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_"
           "SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:"
           "ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384")),
+      experiments_(LoadConfig(FLAGS_grpc_experiments, "GRPC_EXPERIMENTS",
+                              overrides.experiments, "")),
+      trace_(LoadConfig(FLAGS_grpc_trace, "GRPC_TRACE", overrides.trace, "")),
       override_system_ssl_roots_dir_(overrides.system_ssl_roots_dir),
       override_default_ssl_roots_file_path_(
           overrides.default_ssl_roots_file_path) {}

src/core/lib/config/config_vars.h

@@ -38,15 +38,15 @@ class ConfigVars {
     absl::optional<bool> enable_fork_support;
     absl::optional<bool> abort_on_leaks;
     absl::optional<bool> not_use_system_ssl_roots;
-    absl::optional<std::string> experiments;
     absl::optional<std::string> dns_resolver;
-    absl::optional<std::string> trace;
     absl::optional<std::string> verbosity;
     absl::optional<std::string> stacktrace_minloglevel;
     absl::optional<std::string> poll_strategy;
     absl::optional<std::string> system_ssl_roots_dir;
     absl::optional<std::string> default_ssl_roots_file_path;
     absl::optional<std::string> ssl_cipher_suites;
+    absl::optional<std::string> experiments;
+    absl::optional<std::string> trace;
   };
   ConfigVars(const ConfigVars&) = delete;
   ConfigVars& operator=(const ConfigVars&) = delete;
@@ -111,13 +111,13 @@ class ConfigVars {
   bool enable_fork_support_;
   bool abort_on_leaks_;
   bool not_use_system_ssl_roots_;
-  std::string experiments_;
   std::string dns_resolver_;
-  std::string trace_;
   std::string verbosity_;
   std::string stacktrace_minloglevel_;
   std::string poll_strategy_;
   std::string ssl_cipher_suites_;
+  std::string experiments_;
+  std::string trace_;
   absl::optional<std::string> override_system_ssl_roots_dir_;
   absl::optional<std::string> override_default_ssl_roots_file_path_;
 };

src/core/lib/config/config_vars.yaml

@@ -30,7 +30,7 @@
 # Such functions should be listed in fuzz_config_vars_helpers.h.
 
 - name: experiments
-  type: string
+  type: comma_separated_string
   description:
     A comma separated list of currently active experiments. Experiments may be
     prefixed with a '-' to disable them.
@@ -55,7 +55,7 @@
     ignored.
   fuzz: true
 - name: trace
-  type: string
+  type: comma_separated_string
   default:
   description:
     A comma separated list of tracers that provide additional insight into

src/core/lib/config/load_config.cc

@@ -20,6 +20,7 @@
 
 #include "absl/flags/marshalling.h"
 #include "absl/strings/numbers.h"
+#include "absl/strings/str_join.h"
 #include "absl/types/optional.h"
 
 #include "src/core/lib/gpr/log_internal.h"
@@ -65,4 +66,14 @@ bool LoadConfigFromEnv(absl::string_view environment_variable,
   return default_value;
 }
 
+std::string LoadConfig(const absl::Flag<std::vector<std::string>>& flag,
+                       absl::string_view environment_variable,
+                       const absl::optional<std::string>& override,
+                       const char* default_value) {
+  if (override.has_value()) return *override;
+  auto from_flag = absl::GetFlag(flag);
+  if (!from_flag.empty()) return absl::StrJoin(from_flag, ",");
+  return LoadConfigFromEnv(environment_variable, default_value);
+}
+
 }  // namespace grpc_core

src/core/lib/config/load_config.h

@@ -20,6 +20,7 @@
 #include <stdint.h>
 
 #include <string>
+#include <vector>
 
 #include "absl/flags/flag.h"
 #include "absl/strings/string_view.h"
@@ -44,6 +45,11 @@ T LoadConfig(const absl::Flag<absl::optional<T>>& flag,
   return LoadConfigFromEnv(environment_variable, default_value);
 }
 
+std::string LoadConfig(const absl::Flag<std::vector<std::string>>& flag,
+                       absl::string_view environment_variable,
+                       const absl::optional<std::string>& override,
+                       const char* default_value);
+
 }  // namespace grpc_core
 
 #endif  // GRPC_SRC_CORE_LIB_CONFIG_LOAD_CONFIG_H

test/core/config/BUILD

@@ -30,3 +30,16 @@ grpc_cc_test(
         "//:grpc",
     ],
 )
+
+grpc_cc_test(
+    name = "load_config_test",
+    srcs = ["load_config_test.cc"],
+    external_deps = ["gtest"],
+    language = "c++",
+    uses_event_engine = False,
+    uses_polling = False,
+    deps = [
+        "//:config",
+        "//:grpc",
+    ],
+)

test/core/config/load_config_test.cc

@@ -0,0 +1,55 @@
+// Copyright 2023 gRPC authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "src/core/lib/config/load_config.h"
+
+#include "absl/flags/flag.h"
+#include "gtest/gtest.h"
+
+#include "src/core/lib/gprpp/env.h"
+
+ABSL_FLAG(std::vector<std::string>, comma_separated_strings, {}, "");
+
+namespace grpc_core {
+
+TEST(LoadConfigTest, LoadCommaSeparated) {
+  SetEnv("grpc_comma_separated_strings", "foo");
+  EXPECT_EQ(LoadConfig(FLAGS_comma_separated_strings,
+                       "grpc_comma_separated_strings", {}, ""),
+            "foo");
+  EXPECT_EQ(LoadConfig(FLAGS_comma_separated_strings,
+                       "grpc_comma_separated_strings", "bar", ""),
+            "bar");
+  absl::SetFlag(&FLAGS_comma_separated_strings, {"hello"});
+  EXPECT_EQ(LoadConfig(FLAGS_comma_separated_strings,
+                       "grpc_comma_separated_strings", {}, ""),
+            "hello");
+  EXPECT_EQ(LoadConfig(FLAGS_comma_separated_strings,
+                       "grpc_comma_separated_strings", "bar", ""),
+            "bar");
+  absl::SetFlag(&FLAGS_comma_separated_strings, {"hello", "world"});
+  EXPECT_EQ(LoadConfig(FLAGS_comma_separated_strings,
+                       "grpc_comma_separated_strings", {}, ""),
+            "hello,world");
+  EXPECT_EQ(LoadConfig(FLAGS_comma_separated_strings,
+                       "grpc_comma_separated_strings", "bar", ""),
+            "bar");
+}
+
+}  // namespace grpc_core
+
+int main(int argc, char** argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}

test/core/util/fuzz_config_vars.cc

@@ -32,22 +32,22 @@ ConfigVars::Overrides OverridesFromFuzzConfigVars(
   if (vars.has_enable_fork_support()) {
     overrides.enable_fork_support = vars.enable_fork_support();
   }
-  if (vars.has_experiments()) {
-    overrides.experiments =
-        ValidateExperimentsStringForFuzzing(vars.experiments());
-  }
   if (vars.has_dns_resolver()) {
     overrides.dns_resolver = vars.dns_resolver();
   }
-  if (vars.has_trace()) {
-    overrides.trace = vars.trace();
-  }
   if (vars.has_verbosity()) {
     overrides.verbosity = vars.verbosity();
   }
   if (vars.has_stacktrace_minloglevel()) {
     overrides.stacktrace_minloglevel = vars.stacktrace_minloglevel();
   }
+  if (vars.has_experiments()) {
+    overrides.experiments =
+        ValidateExperimentsStringForFuzzing(vars.experiments());
+  }
+  if (vars.has_trace()) {
+    overrides.trace = vars.trace();
+  }
   return overrides;
 }
 void ApplyFuzzConfigVars(const grpc::testing::FuzzConfigVars& vars) {

test/core/util/fuzz_config_vars.proto

@@ -22,9 +22,9 @@ package grpc.testing;
 
 message FuzzConfigVars {
   optional bool enable_fork_support = 61008869;
-  optional string experiments = 510817011;
   optional string dns_resolver = 76817901;
-  optional string trace = 291231137;
   optional string verbosity = 68420950;
   optional string stacktrace_minloglevel = 273035715;
+  optional string experiments = 510817011;
+  optional string trace = 291231137;
 };

tools/codegen/core/gen_config_vars.py

@@ -101,22 +101,37 @@ def put_copyright(file):
 RETURN_TYPE = {
     "int": "int32_t",
     "string": "absl::string_view",
+    "comma_separated_string": "absl::string_view",
     "bool": "bool",
 }
 
 MEMBER_TYPE = {
     "int": "int32_t",
     "string": "std::string",
+    "comma_separated_string": "std::string",
     "bool": "bool",
 }
 
+FLAG_TYPE = {
+    "int": "absl::optional<int32_t>",
+    "string": "absl::optional<std::string>",
+    "comma_separated_string": "std::vector<std::string>",
+    "bool": "absl::optional<bool>",
+}
+
 PROTO_TYPE = {
     "int": "int32",
     "string": "string",
+    "comma_separated_string": "string",
     "bool": "bool",
 }
 
-SORT_ORDER_FOR_PACKING = {"int": 0, "bool": 1, "string": 2}
+SORT_ORDER_FOR_PACKING = {
+    "int": 0,
+    "bool": 1,
+    "string": 2,
+    "comma_separated_string": 3
+}
 
 
 def bool_default_value(x, name):
@@ -147,12 +162,14 @@ DEFAULT_VALUE = {
     "int": int_default_value,
     "bool": bool_default_value,
     "string": string_default_value,
+    "comma_separated_string": string_default_value,
 }
 
 TO_STRING = {
     "int": "$",
     "bool": "$?\"true\":\"false\"",
     "string": "\"\\\"\", absl::CEscape($), \"\\\"\"",
+    "comma_separated_string": "\"\\\"\", absl::CEscape($), \"\\\"\"",
 }
 
 attrs_in_packing_order = sorted(attrs,
@@ -347,8 +364,8 @@ with open('src/core/lib/config/config_vars.cc', 'w') as C:
             print(attr['prelude'], file=C)
 
     for attr in attrs:
-        print("ABSL_FLAG(absl::optional<%s>, %s, absl::nullopt, %s);" %
+        print("ABSL_FLAG(%s, %s, {}, %s);" %
-              (MEMBER_TYPE[attr["type"]], 'grpc_' + attr['name'],
+              (FLAG_TYPE[attr["type"]], 'grpc_' + attr['name'],
                c_str(attr['description'])),
               file=C)
     print(file=C)

tools/run_tests/generated/tests.json

@@ -4527,6 +4527,30 @@
     ],
     "uses_polling": true
   },
+  {
+    "args": [],
+    "benchmark": false,
+    "ci_platforms": [
+      "linux",
+      "mac",
+      "posix",
+      "windows"
+    ],
+    "cpu_cost": 1.0,
+    "exclude_configs": [],
+    "exclude_iomgrs": [],
+    "flaky": false,
+    "gtest": true,
+    "language": "c++",
+    "name": "load_config_test",
+    "platforms": [
+      "linux",
+      "mac",
+      "posix",
+      "windows"
+    ],
+    "uses_polling": false
+  },
   {
     "args": [],
     "benchmark": true,