From 43a4a36fe3adda6c4d9dcff9174cdf82a5649c1d Mon Sep 17 00:00:00 2001 From: changran Date: Fri, 18 Feb 2022 14:06:32 +0800 Subject: [PATCH 1/3] remove listener_reuse_port_default_enabled Signed-off-by: changran --- docs/root/version_history/current.rst | 1 + source/common/runtime/runtime_features.cc | 1 - source/server/server.cc | 5 +--- source/server/server.h | 2 +- test/server/server_test.cc | 27 ------------------- .../test_data/server/runtime_bootstrap.yaml | 10 ------- 6 files changed, 3 insertions(+), 43 deletions(-) delete mode 100644 test/server/test_data/server/runtime_bootstrap.yaml diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index b7cb1565e3d97..fc0139abf14b0 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -46,6 +46,7 @@ Removed Config or Runtime * http: removed ``envoy.reloadable_features.allow_response_for_timeout`` and legacy code paths. * http: removed ``envoy.reloadable_features.http2_consume_stream_refused_errors`` and legacy code paths. * http: removed ``envoy.reloadable_features.internal_redirects_with_body`` and legacy code paths. +* listener: removed ``envoy.reloadable_features.listener_reuse_port_default_enabled`` and legacy code paths. * udp: removed ``envoy.reloadable_features.udp_per_event_loop_read_limit`` and legacy code paths. * upstream: removed ``envoy.reloadable_features.health_check.graceful_goaway_handling`` and legacy code paths. * xds: removed ``envoy.reloadable_features.vhds_heartbeats`` and legacy code paths. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 9c17a0a7bfa2a..549d9f9c74c4b 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -151,7 +151,6 @@ constexpr absl::Flag* runtime_features[] = { &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_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, diff --git a/source/server/server.cc b/source/server/server.cc index 57aeb9b225fbe..9b250e255d28a 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -530,7 +530,7 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add : ReusePortDefault::False; } else { // This is needed so that we don't read the value until runtime is fully initialized. - enable_reuse_port_default_ = ReusePortDefault::Runtime; + enable_reuse_port_default_ = ReusePortDefault::True; } admin_ = std::make_unique(initial_config.admin().profilePath(), *this, initial_config.admin().ignoreGlobalConnLimit()); @@ -1009,9 +1009,6 @@ bool InstanceImpl::enableReusePortDefault() { return true; case ReusePortDefault::False: return false; - case ReusePortDefault::Runtime: - return Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.listener_reuse_port_default_enabled"); } return false; // for gcc diff --git a/source/server/server.h b/source/server/server.h index 29f4944f74d41..51007c88c5645 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -304,7 +304,7 @@ class InstanceImpl final : Logger::Loggable, registerCallback(Stage stage, StageCallbackWithCompletion callback) override; private: - enum class ReusePortDefault { True, False, Runtime }; + enum class ReusePortDefault { True, False }; ProtobufTypes::MessagePtr dumpBootstrapConfig(); void flushStatsInternal(); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 5230712b2ec54..9b2a62d9e6376 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1024,33 +1024,6 @@ TEST_P(ServerInstanceImplTest, BootstrapNodeWithOptionsOverride) { EXPECT_EQ("bootstrap_sub_zone", server_->localInfo().node().locality().sub_zone()); } -// Validate server runtime is parsed from bootstrap and that we can read from -// service cluster specified disk-based overrides. -TEST_P(ServerInstanceImplTest, BootstrapRuntime) { - options_.service_cluster_name_ = "some_service"; - initialize("test/server/test_data/server/runtime_bootstrap.yaml"); - EXPECT_FALSE(server_->enableReusePortDefault()); - EXPECT_EQ("bar", server_->runtime().snapshot().get("foo").value().get()); - // This should access via the override/some_service overlay. - EXPECT_EQ("fozz", server_->runtime().snapshot().get("fizz").value().get()); -} - -// Validate that a runtime absent an admin layer will fail mutating operations -// but still support inspection of runtime values. -TEST_P(ServerInstanceImplTest, RuntimeNoAdminLayer) { - options_.service_cluster_name_ = "some_service"; - initialize("test/server/test_data/server/runtime_bootstrap.yaml"); - Http::TestResponseHeaderMapImpl response_headers; - std::string response_body; - EXPECT_EQ(Http::Code::OK, - server_->admin().request("/runtime", "GET", response_headers, response_body)); - EXPECT_THAT(response_body, HasSubstr("fozz")); - EXPECT_EQ( - Http::Code::ServiceUnavailable, - server_->admin().request("/runtime_modify?foo=bar", "POST", response_headers, response_body)); - EXPECT_EQ("No admin layer specified", response_body); -} - // Verify that bootstrap fails if RTDS is configured through an EDS cluster TEST_P(ServerInstanceImplTest, BootstrapRtdsThroughEdsFails) { options_.service_cluster_name_ = "some_service"; diff --git a/test/server/test_data/server/runtime_bootstrap.yaml b/test/server/test_data/server/runtime_bootstrap.yaml deleted file mode 100644 index 6329649b0f497..0000000000000 --- a/test/server/test_data/server/runtime_bootstrap.yaml +++ /dev/null @@ -1,10 +0,0 @@ -layered_runtime: - layers: - - name: some_static_layer - static_layer: - foo: bar - envoy.reloadable_features.listener_reuse_port_default_enabled: false - - name: base_disk_layer - disk_layer: {symlink_root: "{{ test_rundir }}/test/server/test_data/runtime/primary"} - - name: overlay_disk_layer - disk_layer: {symlink_root: "{{ test_rundir }}/test/server/test_data/runtime/override", append_service_cluster: true} From c2f2b041c54e17c02e3e8a787cc09be0ccb16249 Mon Sep 17 00:00:00 2001 From: changran Date: Mon, 21 Feb 2022 14:41:25 +0800 Subject: [PATCH 2/3] remove enum ReusePortDefault Signed-off-by: changran --- source/server/server.cc | 22 ++++------------------ source/server/server.h | 2 +- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/source/server/server.cc b/source/server/server.cc index 9b250e255d28a..d6c75e6392cbb 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -93,7 +93,7 @@ InstanceImpl::InstanceImpl( grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), quic_stat_names_(store.symbolTable()), server_contexts_(*this), - stats_flush_in_progress_(false) { + enable_reuse_port_default_(true), stats_flush_in_progress_(false) { TRY_ASSERT_MAIN_THREAD { if (!options.logPath().empty()) { TRY_ASSERT_MAIN_THREAD { @@ -525,12 +525,8 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add const auto parent_admin_shutdown_response = restarter_.sendParentAdminShutdownRequest(); if (parent_admin_shutdown_response.has_value()) { original_start_time_ = parent_admin_shutdown_response.value().original_start_time_; - enable_reuse_port_default_ = parent_admin_shutdown_response.value().enable_reuse_port_default_ - ? ReusePortDefault::True - : ReusePortDefault::False; - } else { - // This is needed so that we don't read the value until runtime is fully initialized. - enable_reuse_port_default_ = ReusePortDefault::True; + enable_reuse_port_default_ = + parent_admin_shutdown_response.value().enable_reuse_port_default_ ? true : false; } admin_ = std::make_unique(initial_config.admin().profilePath(), *this, initial_config.admin().ignoreGlobalConnLimit()); @@ -1002,17 +998,7 @@ ProtobufTypes::MessagePtr InstanceImpl::dumpBootstrapConfig() { return config_dump; } -bool InstanceImpl::enableReusePortDefault() { - ASSERT(enable_reuse_port_default_.has_value()); - switch (enable_reuse_port_default_.value()) { - case ReusePortDefault::True: - return true; - case ReusePortDefault::False: - return false; - } - - return false; // for gcc -} +bool InstanceImpl::enableReusePortDefault() { return enable_reuse_port_default_; } } // namespace Server } // namespace Envoy diff --git a/source/server/server.h b/source/server/server.h index 51007c88c5645..5f399d576403a 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -394,7 +394,7 @@ class InstanceImpl final : Logger::Loggable, ListenerHooks& hooks_; Quic::QuicStatNames quic_stat_names_; ServerFactoryContextImpl server_contexts_; - absl::optional enable_reuse_port_default_; + bool enable_reuse_port_default_; bool stats_flush_in_progress_ : 1; From 8596bacb37cdfcfbb4c398d2c9f4e72b1c12853e Mon Sep 17 00:00:00 2001 From: changran Date: Wed, 23 Feb 2022 16:54:51 +0800 Subject: [PATCH 3/3] recover tests Signed-off-by: changran --- source/server/server.h | 2 -- test/server/server_test.cc | 26 +++++++++++++++++++ .../test_data/server/runtime_bootstrap.yaml | 9 +++++++ 3 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 test/server/test_data/server/runtime_bootstrap.yaml diff --git a/source/server/server.h b/source/server/server.h index 5f399d576403a..1df37560ecfb0 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -304,8 +304,6 @@ class InstanceImpl final : Logger::Loggable, registerCallback(Stage stage, StageCallbackWithCompletion callback) override; private: - enum class ReusePortDefault { True, False }; - ProtobufTypes::MessagePtr dumpBootstrapConfig(); void flushStatsInternal(); void updateServerStats(); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 9b2a62d9e6376..796bc69d4fcde 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -1024,6 +1024,32 @@ TEST_P(ServerInstanceImplTest, BootstrapNodeWithOptionsOverride) { EXPECT_EQ("bootstrap_sub_zone", server_->localInfo().node().locality().sub_zone()); } +// Validate server runtime is parsed from bootstrap and that we can read from +// service cluster specified disk-based overrides. +TEST_P(ServerInstanceImplTest, BootstrapRuntime) { + options_.service_cluster_name_ = "some_service"; + initialize("test/server/test_data/server/runtime_bootstrap.yaml"); + EXPECT_EQ("bar", server_->runtime().snapshot().get("foo").value().get()); + // This should access via the override/some_service overlay. + EXPECT_EQ("fozz", server_->runtime().snapshot().get("fizz").value().get()); +} + +// Validate that a runtime absent an admin layer will fail mutating operations +// but still support inspection of runtime values. +TEST_P(ServerInstanceImplTest, RuntimeNoAdminLayer) { + options_.service_cluster_name_ = "some_service"; + initialize("test/server/test_data/server/runtime_bootstrap.yaml"); + Http::TestResponseHeaderMapImpl response_headers; + std::string response_body; + EXPECT_EQ(Http::Code::OK, + server_->admin().request("/runtime", "GET", response_headers, response_body)); + EXPECT_THAT(response_body, HasSubstr("fozz")); + EXPECT_EQ( + Http::Code::ServiceUnavailable, + server_->admin().request("/runtime_modify?foo=bar", "POST", response_headers, response_body)); + EXPECT_EQ("No admin layer specified", response_body); +} + // Verify that bootstrap fails if RTDS is configured through an EDS cluster TEST_P(ServerInstanceImplTest, BootstrapRtdsThroughEdsFails) { options_.service_cluster_name_ = "some_service"; diff --git a/test/server/test_data/server/runtime_bootstrap.yaml b/test/server/test_data/server/runtime_bootstrap.yaml new file mode 100644 index 0000000000000..9d14d094e7d8d --- /dev/null +++ b/test/server/test_data/server/runtime_bootstrap.yaml @@ -0,0 +1,9 @@ +layered_runtime: + layers: + - name: some_static_layer + static_layer: + foo: bar + - name: base_disk_layer + disk_layer: {symlink_root: "{{ test_rundir }}/test/server/test_data/runtime/primary"} + - name: overlay_disk_layer + disk_layer: {symlink_root: "{{ test_rundir }}/test/server/test_data/runtime/override", append_service_cluster: true}