-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
runtime: changing runtime features to be ABSL_FLAGs based #19880
Changes from 4 commits
7098f07
df5a986
a844e4a
2e1c8a1
7460daf
cefd040
1271875
dce2cc4
a9a0cf0
14b74b0
c24b2a6
71bfa16
c72e652
db4ada6
f9b93ad
3761e7e
0741855
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,34 +1,102 @@ | ||
#include "source/common/runtime/runtime_features.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, ""); | ||
|
||
RUNTIME_GUARD(envoy_reloadable_features_allow_response_for_timeout); | ||
RUNTIME_GUARD(envoy_reloadable_features_allow_upstream_inline_write); | ||
RUNTIME_GUARD(envoy_reloadable_features_append_or_truncate); | ||
RUNTIME_GUARD(envoy_reloadable_features_conn_pool_delete_when_idle); | ||
RUNTIME_GUARD(envoy_reloadable_features_correct_scheme_and_xfp); | ||
RUNTIME_GUARD(envoy_reloadable_features_correctly_validate_alpn); | ||
RUNTIME_GUARD(envoy_reloadable_features_disable_tls_inspector_injection); | ||
RUNTIME_GUARD(envoy_reloadable_features_enable_grpc_async_client_cache); | ||
RUNTIME_GUARD(envoy_reloadable_features_fix_added_trailers); | ||
RUNTIME_GUARD(envoy_reloadable_features_handle_stream_reset_during_hcm_encoding); | ||
RUNTIME_GUARD(envoy_reloadable_features_http2_allow_capacity_increase_by_settings); | ||
RUNTIME_GUARD(envoy_reloadable_features_http_ext_authz_do_not_skip_direct_response_and_redirect); | ||
RUNTIME_GUARD(envoy_reloadable_features_http_reject_path_with_fragment); | ||
RUNTIME_GUARD(envoy_reloadable_features_http_strip_fragment_from_path_unsafe_if_disabled); | ||
RUNTIME_GUARD(envoy_reloadable_features_internal_address); | ||
RUNTIME_GUARD(envoy_reloadable_features_internal_redirects_with_body); | ||
RUNTIME_GUARD(envoy_reloadable_features_listener_reuse_port_default_enabled); | ||
RUNTIME_GUARD(envoy_reloadable_features_listener_wildcard_match_ip_family); | ||
RUNTIME_GUARD(envoy_reloadable_features_new_tcp_connection_pool); | ||
RUNTIME_GUARD(envoy_reloadable_features_proxy_102_103); | ||
RUNTIME_GUARD(envoy_reloadable_features_remove_legacy_json); | ||
RUNTIME_GUARD(envoy_reloadable_features_sanitize_http_header_referer); | ||
RUNTIME_GUARD(envoy_reloadable_features_skip_dispatching_frames_for_closed_connection); | ||
RUNTIME_GUARD(envoy_reloadable_features_support_locality_update_on_eds_cluster_endpoints); | ||
RUNTIME_GUARD(envoy_reloadable_features_test_feature_true); | ||
RUNTIME_GUARD(envoy_reloadable_features_udp_listener_updates_filter_chain_in_place); | ||
RUNTIME_GUARD(envoy_reloadable_features_update_expected_rq_timeout_on_retry); | ||
RUNTIME_GUARD(envoy_reloadable_features_use_dns_ttl); | ||
RUNTIME_GUARD(envoy_reloadable_features_validate_connect); | ||
RUNTIME_GUARD(envoy_reloadable_features_vhds_heartbeats); | ||
RUNTIME_GUARD(envoy_restart_features_explicit_wildcard_resource); | ||
RUNTIME_GUARD(envoy_restart_features_use_apple_api_for_dns_lookups); | ||
|
||
// Begin false flags. These should come with a TODO to flip true. | ||
// Sentinel and test flag. | ||
ABSL_FLAG(bool, envoy_reloadable_features_test_feature_false, false, ""); | ||
// TODO(alyssawilk, junr03) flip (and add release notes + docs) these after Lyft tests | ||
ABSL_FLAG(bool, envoy_reloadable_features_allow_multiple_dns_addresses, false, ""); | ||
// TODO(adisuissa) reset to true to enable unified mux by default | ||
ABSL_FLAG(bool, envoy_reloadable_features_unified_mux, false, ""); | ||
// TODO(birenroy) reset to true after bug fixes | ||
ABSL_FLAG(bool, envoy_reloadable_features_http2_new_codec_wrapper, false, ""); | ||
|
||
// Block of non-boolean flags. These are deprecated.Do not add more. | ||
ABSL_FLAG(uint64_t, envoy_buffer_overflow_multiplier, 0, ""); | ||
ABSL_FLAG(uint64_t, envoy_do_not_use_going_away_max_http2_outbound_response, 2, ""); | ||
ABSL_FLAG(uint64_t, envoy_headermap_lazy_map_min_size, 3, ""); | ||
|
||
namespace Envoy { | ||
namespace Runtime { | ||
|
||
bool isRuntimeFeature(absl::string_view feature) { | ||
return RuntimeFeaturesDefaults::get().enabledByDefault(feature) || | ||
RuntimeFeaturesDefaults::get().existsButDisabled(feature); | ||
return RuntimeFeaturesDefaults::get().getFlag(feature) != nullptr; | ||
} | ||
|
||
bool runtimeFeatureEnabled(absl::string_view feature) { | ||
ASSERT(isRuntimeFeature(feature)); | ||
if (Runtime::LoaderSingleton::getExisting()) { | ||
return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->runtimeFeatureEnabled( | ||
feature); | ||
auto* flag = RuntimeFeaturesDefaults::get().getFlag(feature); | ||
if (flag == nullptr) { | ||
IS_ENVOY_BUG(absl::StrCat("Unable to find runtime feature ", feature)); | ||
return false; | ||
} | ||
ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::runtime), debug, | ||
"Unable to use runtime singleton for feature {}", feature); | ||
return RuntimeFeaturesDefaults::get().enabledByDefault(feature); | ||
return absl::GetFlag(*flag); | ||
} | ||
|
||
uint64_t getInteger(absl::string_view feature, uint64_t default_value) { | ||
ASSERT(absl::StartsWith(feature, "envoy.")); | ||
if (Runtime::LoaderSingleton::getExisting()) { | ||
return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->getInteger( | ||
std::string(feature), default_value); | ||
if (!absl::StartsWith(feature, "envoy.")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking out loud here. Technically, an extension can use this method, and fetch a value that is set by RTDS. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good call, refactored to catch both cases and done. Alternately I could also rename to getDeprecatedInteger and break folks' compile if they use it - think that's worth doing? |
||
return default_value; | ||
} | ||
|
||
// DO NOT ADD MORE FLAGS HERE. This function deprecated and being removed. | ||
if (feature == "envoy.buffer.overflow_multiplier") { | ||
return absl::GetFlag(FLAGS_envoy_buffer_overflow_multiplier); | ||
} else if (feature == "envoy.do_not_use_going_away_max_http2_outbound_responses") { | ||
return absl::GetFlag(FLAGS_envoy_do_not_use_going_away_max_http2_outbound_response); | ||
} else if (feature == "envoy.http.headermap.lazy_map_min_size") { | ||
return absl::GetFlag(FLAGS_envoy_headermap_lazy_map_min_size); | ||
} | ||
ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::runtime), debug, | ||
"Unable to use runtime singleton for feature {}", feature); | ||
return default_value; | ||
} | ||
|
||
|
@@ -52,71 +120,88 @@ uint64_t getInteger(absl::string_view feature, uint64_t default_value) { | |
// 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 const char* runtime_features[] = { | ||
// Enabled | ||
"envoy.reloadable_features.test_feature_true", | ||
// Begin alphabetically sorted section. | ||
"envoy.reloadable_features.allow_response_for_timeout", | ||
"envoy.reloadable_features.allow_upstream_inline_write", | ||
"envoy.reloadable_features.append_or_truncate", | ||
"envoy.reloadable_features.conn_pool_delete_when_idle", | ||
"envoy.reloadable_features.correct_scheme_and_xfp", | ||
"envoy.reloadable_features.correctly_validate_alpn", | ||
"envoy.reloadable_features.disable_tls_inspector_injection", | ||
"envoy.reloadable_features.enable_grpc_async_client_cache", | ||
"envoy.reloadable_features.fix_added_trailers", | ||
"envoy.reloadable_features.handle_stream_reset_during_hcm_encoding", | ||
"envoy.reloadable_features.http2_allow_capacity_increase_by_settings", | ||
"envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect", | ||
"envoy.reloadable_features.http_reject_path_with_fragment", | ||
"envoy.reloadable_features.http_strip_fragment_from_path_unsafe_if_disabled", | ||
"envoy.reloadable_features.internal_address", | ||
"envoy.reloadable_features.internal_redirects_with_body", | ||
"envoy.reloadable_features.listener_reuse_port_default_enabled", | ||
"envoy.reloadable_features.listener_wildcard_match_ip_family", | ||
"envoy.reloadable_features.new_tcp_connection_pool", | ||
"envoy.reloadable_features.proxy_102_103", | ||
"envoy.reloadable_features.remove_legacy_json", | ||
"envoy.reloadable_features.support_locality_update_on_eds_cluster_endpoints", | ||
"envoy.reloadable_features.udp_listener_updates_filter_chain_in_place", | ||
"envoy.reloadable_features.update_expected_rq_timeout_on_retry", | ||
"envoy.reloadable_features.use_dns_ttl", | ||
"envoy.reloadable_features.validate_connect", | ||
"envoy.reloadable_features.vhds_heartbeats", | ||
"envoy.restart_features.explicit_wildcard_resource", | ||
"envoy.restart_features.use_apple_api_for_dns_lookups", | ||
// Misplaced flags: please do not add flags to this section. | ||
"envoy.reloadable_features.sanitize_http_header_referer", | ||
"envoy.reloadable_features.skip_dispatching_frames_for_closed_connection", | ||
// End misplaced flags: please do not add flags in this section. | ||
constexpr absl::Flag<bool>* 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_response_for_timeout, | ||
&FLAGS_envoy_reloadable_features_allow_upstream_inline_write, | ||
&FLAGS_envoy_reloadable_features_append_or_truncate, | ||
&FLAGS_envoy_reloadable_features_conn_pool_delete_when_idle, | ||
&FLAGS_envoy_reloadable_features_correct_scheme_and_xfp, | ||
&FLAGS_envoy_reloadable_features_correctly_validate_alpn, | ||
&FLAGS_envoy_reloadable_features_disable_tls_inspector_injection, | ||
&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_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_internal_redirects_with_body, | ||
&FLAGS_envoy_reloadable_features_listener_reuse_port_default_enabled, | ||
&FLAGS_envoy_reloadable_features_listener_wildcard_match_ip_family, | ||
&FLAGS_envoy_reloadable_features_new_tcp_connection_pool, | ||
&FLAGS_envoy_reloadable_features_proxy_102_103, | ||
&FLAGS_envoy_reloadable_features_remove_legacy_json, | ||
&FLAGS_envoy_reloadable_features_sanitize_http_header_referer, | ||
&FLAGS_envoy_reloadable_features_skip_dispatching_frames_for_closed_connection, | ||
&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_use_dns_ttl, | ||
&FLAGS_envoy_reloadable_features_validate_connect, | ||
&FLAGS_envoy_reloadable_features_vhds_heartbeats, | ||
&FLAGS_envoy_restart_features_explicit_wildcard_resource, | ||
&FLAGS_envoy_restart_features_use_apple_api_for_dns_lookups, | ||
}; | ||
// clang-format on | ||
|
||
// This is a section for officially sanctioned runtime features which are too | ||
// high risk to be enabled by default. Examples where we have opted to land | ||
// features without enabling by default are swapping the underlying buffer | ||
// implementation or the HTTP/1.1 codec implementation. Most features should be | ||
// enabled by default. | ||
// | ||
// When features are added here, there should be a tracking bug assigned to the | ||
// code owner to flip the default after sufficient testing. | ||
constexpr const char* disabled_runtime_features[] = { | ||
// TODO(alyssawilk, junr03) flip (and add release notes + docs) these after Lyft tests | ||
"envoy.reloadable_features.allow_multiple_dns_addresses", | ||
// Sentinel and test flag. | ||
"envoy.reloadable_features.test_feature_false", | ||
// TODO(dmitri-d) reset to true to enable unified mux by default | ||
"envoy.reloadable_features.unified_mux", | ||
// TODO(birenroy) flip after https://github.com/envoyproxy/envoy/issues/19761 sorted. | ||
"envoy.reloadable_features.http2_new_codec_wrapper", | ||
}; | ||
void maybeSetRuntimeGuard(absl::string_view name, bool value) { | ||
auto* flag = RuntimeFeaturesDefaults::get().getFlag(name); | ||
if (flag == nullptr) { | ||
IS_ENVOY_BUG(absl::StrCat("Unable to find runtime feature ", name)); | ||
return; | ||
} | ||
absl::SetFlag(flag, value); | ||
} | ||
|
||
// TODO(alyssawilk) deprecate use of this | ||
void maybeSetDeprecatedInts(absl::string_view name, uint32_t value) { | ||
if (!absl::StartsWith(name, "envoy.")) { | ||
return; | ||
} | ||
|
||
// DO NOT ADD MORE FLAGS HERE. This function deprecated and being removed. | ||
if (name == "envoy.buffer.overflow_multiplier") { | ||
absl::SetFlag(&FLAGS_envoy_buffer_overflow_multiplier, value); | ||
} else if (name == "envoy.do_not_use_going_away_max_http2_outbound_responses") { | ||
absl::SetFlag(&FLAGS_envoy_do_not_use_going_away_max_http2_outbound_response, value); | ||
} else if (name == "envoy.http.headermap.lazy_map_min_size") { | ||
absl::SetFlag(&FLAGS_envoy_headermap_lazy_map_min_size, value); | ||
} | ||
} | ||
|
||
std::string swapPrefix(std::string name) { | ||
return absl::StrReplaceAll(name, {{"envoy_", "envoy."}, {"features_", "features."}}); | ||
adisuissa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
RuntimeFeatures::RuntimeFeatures() { | ||
for (auto& feature : runtime_features) { | ||
enabled_features_.insert(feature); | ||
} | ||
for (auto& feature : disabled_runtime_features) { | ||
disabled_features_.insert(feature); | ||
auto& reflection = absl::GetFlagReflectionHandle(*feature); | ||
std::string envoy_name = swapPrefix(std::string(reflection.Name())); | ||
all_features_.emplace(envoy_name, feature); | ||
absl::optional<bool> value = reflection.TryGet<bool>(); | ||
if (value.has_value() && value.value()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: consider adding:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added an assert - everything in that struct has to be a boolean by definition so really the value check is silly. |
||
enabled_features_.emplace(envoy_name, feature); | ||
} else { | ||
disabled_features_.emplace(envoy_name, feature); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
|
||
#include "absl/container/node_hash_map.h" | ||
#include "absl/container/node_hash_set.h" | ||
#include "absl/flags/flag.h" | ||
#include "absl/strings/match.h" | ||
#include "absl/strings/numbers.h" | ||
|
||
|
@@ -41,20 +42,25 @@ void countDeprecatedFeatureUseInternal(const RuntimeStats& stats) { | |
stats.deprecated_feature_seen_since_process_start_.inc(); | ||
} | ||
|
||
// TODO(12923): Document the Quiche reloadable flag setup. | ||
#ifdef ENVOY_ENABLE_QUIC | ||
void refreshQuicheReloadableFlags(const Snapshot::EntryMap& flag_map) { | ||
void refreshReloadableFlags(const Snapshot::EntryMap& flag_map) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just so I understand this is now being used for all refresh, right? So it's no longer H3 specific? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct, that's why I took quiche out of the name :-) |
||
absl::flat_hash_map<std::string, bool> quiche_flags_override; | ||
for (const auto& it : flag_map) { | ||
if (absl::StartsWith(it.first, quiche::EnvoyQuicheReloadableFlagPrefix) && | ||
it.second.bool_value_.has_value()) { | ||
quiche_flags_override[it.first.substr(quiche::EnvoyFeaturePrefix.length())] = | ||
it.second.bool_value_.value(); | ||
} | ||
if (it.second.bool_value_.has_value() && isRuntimeFeature(it.first)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again for the exceptional case where someone sets a non-conventional runtime flag by RTDS, we should probably have an ENVOY_BUG here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry you're saying if someone sets a boolean in their own code base envoy.reloadable_flag.not_in_runtime_features? Their code would crash on lookup any time they tried to reference the value so I'm not convinced anyone would compile that in but I can add a check regardless if you think it's worth it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe my understanding of the previous runtime and loader is incorrect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes someone can set a random name, and it could be read from the snapshot, which is fine. It will only refresh a flag if the flag exists. I'm not sure what Alyssa's crashing comment is referring to? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Adi was asking if previously we supported a non-envoy-registered envoy.reloadable_feature being set via runtime config and fetched via runtimeFeatureEnabled(). It was not supported previously - you can set it in RTDS but runtimeFeatureEnabled checked that Envoy knew about it so you could only access it via explicit runtime checks. I think basically we have support parity here. |
||
maybeSetRuntimeGuard(it.first, it.second.bool_value_.value()); | ||
} | ||
if (it.second.uint_value_.has_value()) { | ||
maybeSetDeprecatedInts(it.first, it.second.uint_value_.value()); | ||
} | ||
} | ||
#ifdef ENVOY_ENABLE_QUIC | ||
quiche::FlagRegistry::getInstance().updateReloadableFlags(quiche_flags_override); | ||
} | ||
#endif | ||
} | ||
|
||
} // namespace | ||
|
||
|
@@ -83,8 +89,8 @@ bool SnapshotImpl::deprecatedFeatureEnabled(absl::string_view key, bool default_ | |
|
||
bool SnapshotImpl::runtimeFeatureEnabled(absl::string_view key) const { | ||
// If the value is not explicitly set as a runtime boolean, the default value is based on | ||
// enabledByDefault. | ||
return getBoolean(key, RuntimeFeaturesDefaults::get().enabledByDefault(key)); | ||
// the underlying value. | ||
return getBoolean(key, Runtime::runtimeFeatureEnabled(key)); | ||
} | ||
|
||
bool SnapshotImpl::featureEnabled(absl::string_view key, uint64_t default_value, | ||
|
@@ -531,9 +537,7 @@ void LoaderImpl::loadNewSnapshot() { | |
return std::static_pointer_cast<ThreadLocal::ThreadLocalObject>(ptr); | ||
}); | ||
|
||
#ifdef ENVOY_ENABLE_QUIC | ||
refreshQuicheReloadableFlags(ptr->values()); | ||
#endif | ||
refreshReloadableFlags(ptr->values()); | ||
|
||
{ | ||
absl::MutexLock lock(&snapshot_mutex_); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could find a way to fetch the number of abseil flags, but if there is one, I suggest to make this constraint in code.
Specifically, adding a RELEASE_ASSERT in
RuntimeFeatures
c'tor to validate that the number of abseil flags equals to the size of runtime_features + size of non-bool flags.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's necessary? If you add a runtime flag and only add it to the top, then when you try to access it you'll fail the ENVOY_BUG in runtimeFeatureEnabled so all existing tests going near that code path will hard fail. Meanwhile if we add that check it'll 100% break import so I'd prefer to stick with the ENVOY_BUG :-)