From 95c57d9ff5afe1b46069ab7a4189dc95f9b5c174 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 18 May 2023 20:55:49 -0700 Subject: [PATCH] [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. --------- Co-authored-by: ctiller --- CMakeLists.txt | 38 ++++++++++++++++++ build_autogenerated.yaml | 10 +++++ src/core/lib/config/config_vars.cc | 38 +++++++++--------- src/core/lib/config/config_vars.h | 8 ++-- src/core/lib/config/config_vars.yaml | 4 +- src/core/lib/config/load_config.cc | 11 ++++++ src/core/lib/config/load_config.h | 6 +++ test/core/config/BUILD | 13 +++++++ test/core/config/load_config_test.cc | 55 +++++++++++++++++++++++++++ test/core/util/fuzz_config_vars.cc | 14 +++---- test/core/util/fuzz_config_vars.proto | 4 +- tools/codegen/core/gen_config_vars.py | 23 +++++++++-- tools/run_tests/generated/tests.json | 24 ++++++++++++ 13 files changed, 212 insertions(+), 36 deletions(-) create mode 100644 test/core/config/load_config_test.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 27e6846c024..28ebb186e8e 100644 --- a/CMakeLists.txt +++ b/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) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index b0179041634..6d2e8139e98 100644 --- a/build_autogenerated.yaml +++ b/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 diff --git a/src/core/lib/config/config_vars.cc b/src/core/lib/config/config_vars.cc index 5745d868780..b00017c763d 100644 --- a/src/core/lib/config/config_vars.cc +++ b/src/core/lib/config/config_vars.cc @@ -20,6 +20,9 @@ #include "src/core/lib/config/config_vars.h" +#include +#include + #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, grpc_experiments, absl::nullopt, +ABSL_FLAG(std::vector, grpc_experiments, {}, "A comma separated list of currently active experiments. Experiments " "may be prefixed with a '-' to disable them."); ABSL_FLAG(absl::optional, 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, grpc_dns_resolver, absl::nullopt, +ABSL_FLAG(absl::optional, 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, grpc_trace, absl::nullopt, +ABSL_FLAG(std::vector, 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, grpc_verbosity, absl::nullopt, +ABSL_FLAG(absl::optional, grpc_verbosity, {}, "Default gRPC logging verbosity"); -ABSL_FLAG(absl::optional, grpc_stacktrace_minloglevel, - absl::nullopt, +ABSL_FLAG(absl::optional, grpc_stacktrace_minloglevel, {}, "Messages logged at the same or higher level than this will print " "stacktrace"); -ABSL_FLAG(absl::optional, grpc_enable_fork_support, absl::nullopt, +ABSL_FLAG(absl::optional, grpc_enable_fork_support, {}, "Enable fork support"); -ABSL_FLAG(absl::optional, grpc_poll_strategy, absl::nullopt, +ABSL_FLAG(absl::optional, 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, grpc_abort_on_leaks, absl::nullopt, +ABSL_FLAG(absl::optional, 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, grpc_system_ssl_roots_dir, absl::nullopt, +ABSL_FLAG(absl::optional, grpc_system_ssl_roots_dir, {}, "Custom directory to SSL Roots"); -ABSL_FLAG(absl::optional, grpc_default_ssl_roots_file_path, - absl::nullopt, "Path to the default SSL roots file."); -ABSL_FLAG(absl::optional, grpc_not_use_system_ssl_roots, absl::nullopt, +ABSL_FLAG(absl::optional, grpc_default_ssl_roots_file_path, {}, + "Path to the default SSL roots file."); +ABSL_FLAG(absl::optional, grpc_not_use_system_ssl_roots, {}, "Disable loading system root certificates."); -ABSL_FLAG(absl::optional, grpc_ssl_cipher_suites, absl::nullopt, +ABSL_FLAG(absl::optional, 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) {} diff --git a/src/core/lib/config/config_vars.h b/src/core/lib/config/config_vars.h index e3ce74fad1d..d465ad32047 100644 --- a/src/core/lib/config/config_vars.h +++ b/src/core/lib/config/config_vars.h @@ -38,15 +38,15 @@ class ConfigVars { absl::optional enable_fork_support; absl::optional abort_on_leaks; absl::optional not_use_system_ssl_roots; - absl::optional experiments; absl::optional dns_resolver; - absl::optional trace; absl::optional verbosity; absl::optional stacktrace_minloglevel; absl::optional poll_strategy; absl::optional system_ssl_roots_dir; absl::optional default_ssl_roots_file_path; absl::optional ssl_cipher_suites; + absl::optional experiments; + absl::optional 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 override_system_ssl_roots_dir_; absl::optional override_default_ssl_roots_file_path_; }; diff --git a/src/core/lib/config/config_vars.yaml b/src/core/lib/config/config_vars.yaml index 99916f93409..1f300ab9640 100644 --- a/src/core/lib/config/config_vars.yaml +++ b/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 diff --git a/src/core/lib/config/load_config.cc b/src/core/lib/config/load_config.cc index e69270d8862..6389d5e140f 100644 --- a/src/core/lib/config/load_config.cc +++ b/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>& flag, + absl::string_view environment_variable, + const absl::optional& 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 diff --git a/src/core/lib/config/load_config.h b/src/core/lib/config/load_config.h index a80d133020e..2ee4891dca1 100644 --- a/src/core/lib/config/load_config.h +++ b/src/core/lib/config/load_config.h @@ -20,6 +20,7 @@ #include #include +#include #include "absl/flags/flag.h" #include "absl/strings/string_view.h" @@ -44,6 +45,11 @@ T LoadConfig(const absl::Flag>& flag, return LoadConfigFromEnv(environment_variable, default_value); } +std::string LoadConfig(const absl::Flag>& flag, + absl::string_view environment_variable, + const absl::optional& override, + const char* default_value); + } // namespace grpc_core #endif // GRPC_SRC_CORE_LIB_CONFIG_LOAD_CONFIG_H diff --git a/test/core/config/BUILD b/test/core/config/BUILD index bf0b4b97bb2..1ba9122159e 100644 --- a/test/core/config/BUILD +++ b/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", + ], +) diff --git a/test/core/config/load_config_test.cc b/test/core/config/load_config_test.cc new file mode 100644 index 00000000000..7e136c0f7d7 --- /dev/null +++ b/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, 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(); +} diff --git a/test/core/util/fuzz_config_vars.cc b/test/core/util/fuzz_config_vars.cc index 734e6c10c0d..7fa9a4e51c7 100644 --- a/test/core/util/fuzz_config_vars.cc +++ b/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) { diff --git a/test/core/util/fuzz_config_vars.proto b/test/core/util/fuzz_config_vars.proto index 065e2f84856..df71f6540f9 100644 --- a/test/core/util/fuzz_config_vars.proto +++ b/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; }; diff --git a/tools/codegen/core/gen_config_vars.py b/tools/codegen/core/gen_config_vars.py index 5fd4b727ce9..43b48871a7b 100755 --- a/tools/codegen/core/gen_config_vars.py +++ b/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", + "string": "absl::optional", + "comma_separated_string": "std::vector", + "bool": "absl::optional", +} + 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);" % - (MEMBER_TYPE[attr["type"]], 'grpc_' + attr['name'], + print("ABSL_FLAG(%s, %s, {}, %s);" % + (FLAG_TYPE[attr["type"]], 'grpc_' + attr['name'], c_str(attr['description'])), file=C) print(file=C) diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 5382f0edb07..cad391c6d13 100644 --- a/tools/run_tests/generated/tests.json +++ b/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,