diff --git a/source/common/runtime/BUILD b/source/common/runtime/BUILD index 115d2636f56b6..8e9c5d3bb70af 100644 --- a/source/common/runtime/BUILD +++ b/source/common/runtime/BUILD @@ -21,6 +21,7 @@ envoy_cc_library( # AVOID ADDING TO THESE DEPENDENCIES IF POSSIBLE # Any code using runtime guards depends on this library, and the more dependencies there are, # the harder it is to runtime-guard without dependency loops. + "@com_google_absl//absl/flags:commandlineflag", "@com_google_absl//absl/flags:flag", "//envoy/runtime:runtime_interface", "//source/common/common:hash_lib", diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 3ebedf93a72c9..9c2cb5c278978 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -1,26 +1,34 @@ #include "source/common/runtime/runtime_features.h" +#include "absl/flags/commandlineflag.h" #include "absl/flags/flag.h" #include "absl/strings/match.h" #include "absl/strings/str_replace.h" -/* To add a runtime guard to Envoy, add 2 lines to this file. - * - * RUNTIME_GUARD(envoy_reloadable_features_flag_name) - * - * to the sorted macro block below and - * - * &FLAGS_envoy_reloadable_features_flag_name - * - * to the runtime features constexpr. - * - * The runtime guard to use in source and release notes will then be of the form - * "envoy.reloadable_features.flag_name" due to the prior naming scheme and swapPrefix. - **/ - #define RUNTIME_GUARD(name) ABSL_FLAG(bool, name, true, ""); // NOLINT #define FALSE_RUNTIME_GUARD(name) ABSL_FLAG(bool, name, false, ""); // NOLINT +// Add additional features here to enable the new code paths by default. +// +// Per documentation in CONTRIBUTING.md is expected that new high risk code paths be guarded +// by runtime feature guards. If you add a guard of the form +// RUNTIME_GUARD(envoy_reloadable_features_my_feature_name) +// here you can guard code checking against "envoy.reloadable_features.my_feature_name". +// Please note the swap of envoy_reloadable_features_ to envoy.reloadable_features.! +// +// if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.my_feature_name")) { +// [new code path] +// else { +// [old_code_path] +// } +// +// Runtime features are true by default, so the new code path is exercised. +// To make a runtime feature false by default, use FALSE_RUNTIME_GUARD, and add +// a TODO to change it to true. +// +// If issues are found that require a runtime feature to be disabled, it should be reported +// ASAP by filing a bug on github. Overriding non-buggy code is strongly discouraged to avoid the +// problem of the bugs being found after the old code path has been removed. RUNTIME_GUARD(envoy_reloadable_features_allow_upstream_inline_write); RUNTIME_GUARD(envoy_reloadable_features_append_or_truncate); RUNTIME_GUARD(envoy_reloadable_features_append_to_accept_content_encoding_only_once); @@ -81,18 +89,61 @@ ABSL_FLAG(uint64_t, re2_max_program_size_warn_level, // NOLINT namespace Envoy { namespace Runtime { +namespace { + +std::string swapPrefix(std::string name) { + return absl::StrReplaceAll(name, {{"envoy_", "envoy."}, {"features_", "features."}}); +} + +} // namespace + +// This is a singleton class to map Envoy style flag names to absl flags +class RuntimeFeatures { +public: + RuntimeFeatures(); + + // Get the command line flag corresponding to the Envoy style feature name, or + // nullptr if it is not a registered flag. + absl::CommandLineFlag* getFlag(absl::string_view feature) const { + auto it = all_features_.find(feature); + if (it == all_features_.end()) { + return nullptr; + } + return it->second; + } + +private: + absl::flat_hash_map all_features_; +}; + +using RuntimeFeaturesDefaults = ConstSingleton; + +RuntimeFeatures::RuntimeFeatures() { + absl::flat_hash_map flags = absl::GetAllFlags(); + for (auto& it : flags) { + absl::string_view name = it.second->Name(); + if ((!absl::StartsWith(name, "envoy_reloadable_features_") && + !absl::StartsWith(name, "envoy_restart_features_")) || + !it.second->TryGet().has_value()) { + continue; + } + std::string envoy_name = swapPrefix(std::string(name)); + all_features_.emplace(envoy_name, it.second); + } +} bool isRuntimeFeature(absl::string_view feature) { return RuntimeFeaturesDefaults::get().getFlag(feature) != nullptr; } bool runtimeFeatureEnabled(absl::string_view feature) { - auto* flag = RuntimeFeaturesDefaults::get().getFlag(feature); + absl::CommandLineFlag* flag = RuntimeFeaturesDefaults::get().getFlag(feature); if (flag == nullptr) { IS_ENVOY_BUG(absl::StrCat("Unable to find runtime feature ", feature)); return false; } - return absl::GetFlag(*flag); + // We validate in map creation that the flag is a boolean. + return flag->TryGet().value(); } uint64_t getInteger(absl::string_view feature, uint64_t default_value) { @@ -113,81 +164,14 @@ uint64_t getInteger(absl::string_view feature, uint64_t default_value) { return default_value; } -// Add additional features here to enable the new code paths by default. -// -// Per documentation in CONTRIBUTING.md is expected that new high risk code paths be guarded -// by runtime feature guards, i.e -// -// if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.my_feature_name")) { -// [new code path] -// else { -// [old_code_path] -// } -// -// Runtime features are false by default, so the old code path is exercised. -// To make a runtime feature true by default, add it to the array below. -// New features should be true-by-default for an Envoy release cycle before the -// old code path is removed. -// -// If issues are found that require a runtime feature to be disabled, it should be reported -// ASAP by filing a bug on github. Overriding non-buggy code is strongly discouraged to avoid the -// problem of the bugs being found after the old code path has been removed. -// clang-format off -constexpr absl::Flag* runtime_features[] = { - // Test flags - &FLAGS_envoy_reloadable_features_test_feature_false, - &FLAGS_envoy_reloadable_features_test_feature_true, - // Begin alphabetically sorted section_ - &FLAGS_envoy_reloadable_features_allow_multiple_dns_addresses, - &FLAGS_envoy_reloadable_features_allow_upstream_inline_write, - &FLAGS_envoy_reloadable_features_append_or_truncate, - &FLAGS_envoy_reloadable_features_append_to_accept_content_encoding_only_once, - &FLAGS_envoy_reloadable_features_conn_pool_delete_when_idle, - &FLAGS_envoy_reloadable_features_conn_pool_new_stream_with_early_data_and_http3, - &FLAGS_envoy_reloadable_features_correct_scheme_and_xfp, - &FLAGS_envoy_reloadable_features_correctly_validate_alpn, - &FLAGS_envoy_reloadable_features_defer_processing_backedup_streams, - &FLAGS_envoy_reloadable_features_deprecate_global_ints, - &FLAGS_envoy_reloadable_features_disable_tls_inspector_injection, - &FLAGS_envoy_reloadable_features_do_not_await_headers_on_upstream_timeout_to_emit_stats, - &FLAGS_envoy_reloadable_features_enable_grpc_async_client_cache, - &FLAGS_envoy_reloadable_features_fix_added_trailers, - &FLAGS_envoy_reloadable_features_handle_stream_reset_during_hcm_encoding, - &FLAGS_envoy_reloadable_features_http1_lazy_read_disable, - &FLAGS_envoy_reloadable_features_http2_allow_capacity_increase_by_settings, - &FLAGS_envoy_reloadable_features_http2_new_codec_wrapper, - &FLAGS_envoy_reloadable_features_http_ext_authz_do_not_skip_direct_response_and_redirect, - &FLAGS_envoy_reloadable_features_http_reject_path_with_fragment, - &FLAGS_envoy_reloadable_features_http_strip_fragment_from_path_unsafe_if_disabled, - &FLAGS_envoy_reloadable_features_internal_address, - &FLAGS_envoy_reloadable_features_listener_wildcard_match_ip_family, - &FLAGS_envoy_reloadable_features_new_tcp_connection_pool, - &FLAGS_envoy_reloadable_features_postpone_h3_client_connect_to_next_loop, - &FLAGS_envoy_reloadable_features_proxy_102_103, - &FLAGS_envoy_reloadable_features_sanitize_http_header_referer, - &FLAGS_envoy_reloadable_features_skip_delay_close, - &FLAGS_envoy_reloadable_features_skip_dispatching_frames_for_closed_connection, - &FLAGS_envoy_reloadable_features_strict_check_on_ipv4_compat, - &FLAGS_envoy_reloadable_features_support_locality_update_on_eds_cluster_endpoints, - &FLAGS_envoy_reloadable_features_udp_listener_updates_filter_chain_in_place, - &FLAGS_envoy_reloadable_features_unified_mux, - &FLAGS_envoy_reloadable_features_update_expected_rq_timeout_on_retry, - &FLAGS_envoy_reloadable_features_update_grpc_response_error_tag, - &FLAGS_envoy_reloadable_features_use_dns_ttl, - &FLAGS_envoy_reloadable_features_validate_connect, - &FLAGS_envoy_restart_features_explicit_wildcard_resource, - &FLAGS_envoy_restart_features_use_apple_api_for_dns_lookups, - &FLAGS_envoy_restart_features_no_runtime_singleton, -}; -// clang-format on - void maybeSetRuntimeGuard(absl::string_view name, bool value) { - auto* flag = RuntimeFeaturesDefaults::get().getFlag(name); + absl::CommandLineFlag* flag = RuntimeFeaturesDefaults::get().getFlag(name); if (flag == nullptr) { IS_ENVOY_BUG(absl::StrCat("Unable to find runtime feature ", name)); return; } - absl::SetFlag(flag, value); + std::string err; + flag->ParseFrom(value ? "true" : "false", &err); } void maybeSetDeprecatedInts(absl::string_view name, uint32_t value) { @@ -211,36 +195,5 @@ void maybeSetDeprecatedInts(absl::string_view name, uint32_t value) { } } -std::string swapPrefix(std::string name) { - return absl::StrReplaceAll(name, {{"envoy_", "envoy."}, {"features_", "features."}}); -} - -RuntimeFeatures::RuntimeFeatures() { - for (auto& feature : runtime_features) { - auto& reflection = absl::GetFlagReflectionHandle(*feature); - std::string envoy_name = swapPrefix(std::string(reflection.Name())); - all_features_.emplace(envoy_name, feature); - absl::optional value = reflection.TryGet(); - ASSERT(value.has_value()); - if (value.value()) { - enabled_features_.emplace(envoy_name, feature); - } else { - disabled_features_.emplace(envoy_name, feature); - } - } -} - -void RuntimeFeatures::restoreDefaults() const { - for (const auto& feature : enabled_features_) { - absl::SetFlag(feature.second, true); - } - for (const auto& feature : disabled_features_) { - absl::SetFlag(feature.second, false); - } - absl::SetFlag(&FLAGS_envoy_headermap_lazy_map_min_size, 3); - absl::SetFlag(&FLAGS_re2_max_program_size_error_level, 100); - absl::SetFlag(&FLAGS_re2_max_program_size_warn_level, std::numeric_limits::max()); -} - } // namespace Runtime } // namespace Envoy diff --git a/source/common/runtime/runtime_features.h b/source/common/runtime/runtime_features.h index 6c0d1f6cbe42a..3ac72f1f215ab 100644 --- a/source/common/runtime/runtime_features.h +++ b/source/common/runtime/runtime_features.h @@ -8,6 +8,7 @@ #include "absl/container/flat_hash_set.h" #include "absl/flags/flag.h" +#include "absl/flags/reflection.h" namespace Envoy { namespace Runtime { @@ -24,29 +25,5 @@ constexpr absl::string_view conn_pool_new_stream_with_early_data_and_http3 = constexpr absl::string_view defer_processing_backedup_streams = "envoy.reloadable_features.defer_processing_backedup_streams"; -class RuntimeFeatures { -public: - RuntimeFeatures(); - - absl::Flag* getFlag(absl::string_view feature) const { - auto it = all_features_.find(feature); - if (it == all_features_.end()) { - return nullptr; - } - return it->second; - } - - void restoreDefaults() const; - -private: - friend class RuntimeFeaturesPeer; - - absl::flat_hash_map*> enabled_features_; - absl::flat_hash_map*> disabled_features_; - absl::flat_hash_map*> all_features_; -}; - -using RuntimeFeaturesDefaults = ConstSingleton; - } // namespace Runtime } // namespace Envoy diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 06d90dffa4184..c1ef54a8f42be 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -3002,7 +3002,6 @@ TEST_F(Http1ServerConnectionImplTest, ManyLargeRequestHeadersAccepted) { TEST_F(Http1ServerConnectionImplTest, RuntimeLazyReadDisableTest) { TestScopedRuntime scoped_runtime; - Runtime::RuntimeFeaturesDefaults::get().restoreDefaults(); // No readDisable for normal non-piped HTTP request. { @@ -3077,7 +3076,6 @@ TEST_F(Http1ServerConnectionImplTest, RuntimeLazyReadDisableTest) { // same time. TEST_F(Http1ServerConnectionImplTest, PipedRequestWithSingleEvent) { TestScopedRuntime scoped_runtime; - Runtime::RuntimeFeaturesDefaults::get().restoreDefaults(); initialize(); @@ -3114,7 +3112,6 @@ TEST_F(Http1ServerConnectionImplTest, PipedRequestWithSingleEvent) { // before the end of the first request. TEST_F(Http1ServerConnectionImplTest, PipedRequestWithMutipleEvent) { TestScopedRuntime scoped_runtime; - Runtime::RuntimeFeaturesDefaults::get().restoreDefaults(); initialize(); diff --git a/test/test_common/test_runtime.h b/test/test_common/test_runtime.h index d763309c3be78..59d109f393770 100644 --- a/test/test_common/test_runtime.h +++ b/test/test_common/test_runtime.h @@ -43,12 +43,8 @@ class TestScopedRuntime { loader().mergeValues(values); } - ~TestScopedRuntime() { - Runtime::RuntimeFeatures features; - features.restoreDefaults(); - } - protected: + absl::FlagSaver saver_; Event::MockDispatcher dispatcher_; testing::NiceMock tls_; Stats::TestUtil::TestStore store_; diff --git a/test/test_listener.cc b/test/test_listener.cc index a6ea54f099210..51ca1970a53ad 100644 --- a/test/test_listener.cc +++ b/test/test_listener.cc @@ -20,7 +20,10 @@ void TestListener::OnTestEnd(const ::testing::TestInfo& test_info) { absl::StrCat("MainThreadLeak: [", test_info.test_suite_name(), ".", test_info.name(), "] test exited before main thread shut down")); } - Runtime::RuntimeFeaturesDefaults::get().restoreDefaults(); + // We must do this in two phases; reset the old flags to get back to the clean state before + // constructing a new flag saver to latch the clean values. + saver_.reset(); + saver_ = std::make_unique(); } } // namespace Envoy diff --git a/test/test_listener.h b/test/test_listener.h index 647dae4e2f830..e83f789372470 100644 --- a/test/test_listener.h +++ b/test/test_listener.h @@ -1,5 +1,6 @@ #pragma once +#include "absl/flags/reflection.h" #include "gtest/gtest.h" namespace Envoy { @@ -21,10 +22,13 @@ namespace Envoy { // be a tax paid by every test method in the codebase. class TestListener : public ::testing::EmptyTestEventListener { public: - TestListener(bool validate_singletons = true) : validate_singletons_(validate_singletons) {} + TestListener(bool validate_singletons = true) + : saver_(std::make_unique()), validate_singletons_(validate_singletons) {} void OnTestEnd(const ::testing::TestInfo& test_info) override; private: + // Make sure runtime guards are restored to defaults on test completion. + std::unique_ptr saver_; bool validate_singletons_; };