From f57e280e84627ded4a6b23e2825bc7fbbdec4676 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Mon, 27 Jan 2020 12:50:41 -0500 Subject: [PATCH] http2: removing legacy configuration options (#9783) Removing some configuration knobs which were intended to be temporary runtime override knobs for high risk security fixes. They've been running in prod so far so good. Time to clean up. Risk Level: Medium (someone could have missed they were temporary) Testing: n/a Docs Changes: n/a Release Notes: n/a Fixes #8993 Signed-off-by: Alyssa Wilk --- source/common/http/http2/codec_impl.cc | 45 ++++------------------- source/common/runtime/runtime_features.cc | 2 - source/common/runtime/runtime_impl.cc | 14 +------ test/common/http/http2/codec_impl_test.cc | 45 +++-------------------- 4 files changed, 14 insertions(+), 92 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index efbaef04aaae..e2fb633376ce 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -347,57 +347,28 @@ void ConnectionImpl::StreamImpl::onMetadataDecoded(MetadataMapPtr&& metadata_map decoder_->decodeMetadata(std::move(metadata_map_ptr)); } -namespace { - -const char InvalidHttpMessagingOverrideKey[] = - "envoy.reloadable_features.http2_protocol_options.stream_error_on_invalid_http_messaging"; -const char MaxOutboundFramesOverrideKey[] = - "envoy.reloadable_features.http2_protocol_options.max_outbound_frames"; -const char MaxOutboundControlFramesOverrideKey[] = - "envoy.reloadable_features.http2_protocol_options.max_outbound_control_frames"; -const char MaxConsecutiveInboundFramesWithEmptyPayloadOverrideKey[] = - "envoy.reloadable_features.http2_protocol_options." - "max_consecutive_inbound_frames_with_empty_payload"; -const char MaxInboundPriorityFramesPerStreamOverrideKey[] = - "envoy.reloadable_features.http2_protocol_options.max_inbound_priority_frames_per_stream"; -const char MaxInboundWindowUpdateFramesPerDataFrameSentOverrideKey[] = - "envoy.reloadable_features.http2_protocol_options." - "max_inbound_window_update_frames_per_data_frame_sent"; - -bool checkRuntimeOverride(bool config_value, const char* override_key) { - return Runtime::runtimeFeatureEnabled(override_key) ? true : config_value; -} - -} // namespace - ConnectionImpl::ConnectionImpl(Network::Connection& connection, Stats::Scope& stats, const Http2Settings& http2_settings, const uint32_t max_headers_kb, const uint32_t max_headers_count) : stats_{ALL_HTTP2_CODEC_STATS(POOL_COUNTER_PREFIX(stats, "http2."))}, connection_(connection), max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count), per_stream_buffer_limit_(http2_settings.initial_stream_window_size_), - stream_error_on_invalid_http_messaging_(checkRuntimeOverride( - http2_settings.stream_error_on_invalid_http_messaging_, InvalidHttpMessagingOverrideKey)), - flood_detected_(false), - max_outbound_frames_( - Runtime::getInteger(MaxOutboundFramesOverrideKey, http2_settings.max_outbound_frames_)), + stream_error_on_invalid_http_messaging_( + http2_settings.stream_error_on_invalid_http_messaging_), + flood_detected_(false), max_outbound_frames_(http2_settings.max_outbound_frames_), frame_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundFrame(fragment); }), - max_outbound_control_frames_(Runtime::getInteger( - MaxOutboundControlFramesOverrideKey, http2_settings.max_outbound_control_frames_)), + max_outbound_control_frames_(http2_settings.max_outbound_control_frames_), control_frame_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundControlFrame(fragment); }), max_consecutive_inbound_frames_with_empty_payload_( - Runtime::getInteger(MaxConsecutiveInboundFramesWithEmptyPayloadOverrideKey, - http2_settings.max_consecutive_inbound_frames_with_empty_payload_)), + http2_settings.max_consecutive_inbound_frames_with_empty_payload_), max_inbound_priority_frames_per_stream_( - Runtime::getInteger(MaxInboundPriorityFramesPerStreamOverrideKey, - http2_settings.max_inbound_priority_frames_per_stream_)), - max_inbound_window_update_frames_per_data_frame_sent_(Runtime::getInteger( - MaxInboundWindowUpdateFramesPerDataFrameSentOverrideKey, - http2_settings.max_inbound_window_update_frames_per_data_frame_sent_)), + http2_settings.max_inbound_priority_frames_per_stream_), + max_inbound_window_update_frames_per_data_frame_sent_( + http2_settings.max_inbound_window_update_frames_per_data_frame_sent_), dispatching_(false), raised_goaway_(false), pending_deferred_reset_(false) {} ConnectionImpl::~ConnectionImpl() { nghttp2_session_del(session_); } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 0f2432524e26..b4c116604e2e 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -45,8 +45,6 @@ constexpr const char* runtime_features[] = { constexpr const char* disabled_runtime_features[] = { // Sentinel and test flag. "envoy.reloadable_features.test_feature_false", - // Should be removed as part of https://github.com/envoyproxy/envoy/issues/8993 - "envoy.reloadable_features.http2_protocol_options.stream_error_on_invalid_http_messaging", }; RuntimeFeatures::RuntimeFeatures() { diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 56f5a7194891..5a28c0d8f35c 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -34,18 +34,6 @@ namespace Envoy { namespace Runtime { namespace { -// Before ASSERTS were added to ensure runtime features were restricted to -// booleans, several integer features were added. isLegacyFeatures exempts -// existing integer features from the checks until they can be cleaned up. -// This includes -// envoy.reloadable_features.max_[request|response]_headers_count from -// include/envoy/http/codec.h as well as the http2_protocol_options overrides in -// source/common/http/http2/codec_impl.cc -bool isLegacyFeature(absl::string_view feature) { - return absl::StartsWith(feature, "envoy.reloadable_features.http2_protocol_options.") || - absl::StartsWith(feature, "envoy.reloadable_features.max_re"); -} - bool isRuntimeFeature(absl::string_view feature) { return RuntimeFeaturesDefaults::get().enabledByDefault(feature) || RuntimeFeaturesDefaults::get().existsButDisabled(feature); @@ -288,7 +276,7 @@ bool SnapshotImpl::featureEnabled(const std::string& key, } uint64_t SnapshotImpl::getInteger(const std::string& key, uint64_t default_value) const { - ASSERT(isLegacyFeature(key) || !isRuntimeFeature(key)); + ASSERT(!isRuntimeFeature(key)); auto entry = values_.find(key); if (entry == values_.end() || !entry->second.uint_value_) { return default_value; diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 87e6995d9975..85e95250da6c 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -17,7 +17,6 @@ #include "test/mocks/protobuf/mocks.h" #include "test/mocks/thread_local/mocks.h" #include "test/test_common/printers.h" -#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "codec_impl_test_util.h" @@ -234,10 +233,6 @@ class Http2CodecImplTest : public ::testing::TestWithParammergeValues(...) - TestScopedRuntime scoped_runtime_; }; TEST_P(Http2CodecImplTest, ShutdownNotice) { @@ -481,25 +476,6 @@ TEST_P(Http2CodecImplTest, InvalidHeadersFrameAllowed) { server_wrapper_.dispatch(Buffer::OwnedImpl(), *server_); } -TEST_P(Http2CodecImplTest, InvalidHeadersFrameOverridden) { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.http2_protocol_options.stream_error_on_invalid_http_messaging", - "true"}}); - initialize(); - - MockStreamCallbacks request_callbacks; - request_encoder_->getStream().addCallbacks(request_callbacks); - - ON_CALL(client_connection_, write(_, _)) - .WillByDefault( - Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); })); - - request_encoder_->encodeHeaders(TestHeaderMapImpl{}, true); - EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::LocalReset, _)); - EXPECT_CALL(request_callbacks, onResetStream(StreamResetReason::RemoteReset, _)); - server_wrapper_.dispatch(Buffer::OwnedImpl(), *server_); -} - TEST_P(Http2CodecImplTest, TrailingHeaders) { initialize(); @@ -1305,9 +1281,7 @@ TEST_P(Http2CodecImplTest, PingFlood) { // Verify that codec allows PING flood when mitigation is disabled TEST_P(Http2CodecImplTest, PingFloodMitigationDisabled) { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.http2_protocol_options.max_outbound_control_frames", - "2147483647"}}); + max_outbound_control_frames_ = 2147483647; initialize(); TestHeaderMapImpl request_headers; @@ -1432,8 +1406,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFlood) { // Verify that codec allows outbound DATA flood when mitigation is disabled TEST_P(Http2CodecImplTest, ResponseDataFloodMitigationDisabled) { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.http2_protocol_options.max_outbound_frames", "2147483647"}}); + max_outbound_control_frames_ = 2147483647; initialize(); TestHeaderMapImpl request_headers; @@ -1540,9 +1513,7 @@ TEST_P(Http2CodecImplTest, PriorityFlood) { } TEST_P(Http2CodecImplTest, PriorityFloodOverride) { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.http2_protocol_options.max_inbound_priority_frames_per_stream", - "2147483647"}}); + max_inbound_priority_frames_per_stream_ = 2147483647; priorityFlood(); EXPECT_NO_THROW(client_->sendPendingFrames()); @@ -1554,10 +1525,7 @@ TEST_P(Http2CodecImplTest, WindowUpdateFlood) { } TEST_P(Http2CodecImplTest, WindowUpdateFloodOverride) { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.http2_protocol_options.max_inbound_window_update_frames_per_" - "data_frame_sent", - "2147483647"}}); + max_inbound_window_update_frames_per_data_frame_sent_ = 2147483647; windowUpdateFlood(); EXPECT_NO_THROW(client_->sendPendingFrames()); } @@ -1570,10 +1538,7 @@ TEST_P(Http2CodecImplTest, EmptyDataFlood) { } TEST_P(Http2CodecImplTest, EmptyDataFloodOverride) { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.http2_protocol_options.max_consecutive_inbound_frames_with_" - "empty_payload", - "2147483647"}}); + max_consecutive_inbound_frames_with_empty_payload_ = 2147483647; Buffer::OwnedImpl data; emptyDataFlood(data); EXPECT_CALL(request_decoder_, decodeData(_, false))