Skip to content

listener: envoy.reloadable_features.listener_reuse_port_default_enabled deprecation #19977

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.

Sign up for GitHub

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

Merged
mattklein123 merged 3 commits into from
Feb 23, 2022
Merged

listener: envoy.reloadable_features.listener_reuse_port_default_enabled deprecation #19977

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43a4a36
remove listener_reuse_port_default_enabled
Feb 18, 2022
c2f2b04
remove enum ReusePortDefault
Feb 21, 2022
8596bac
recover tests
Feb 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 0 additions & 1 deletion source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@ constexpr absl::Flag<bool>* 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,
Expand Down
5 changes: 1 addition & 4 deletions source/server/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add
: ReusePortDefault::False;

@soulxu soulxu Feb 20, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question, does it mean the user never can upgrade to the true default value by hot restart? So that means we should keep those code forever?

@soulxu soulxu Feb 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 this question probably needs your help. I thought we should remove all the upgrade code. But from the code and comments, I understand the hot restart can't change the default value. That means we only can remove those codes when all the users have a cold restart, but that can't be forced. Then probably this code never can be deleted?

@mattklein123 mattklein123 Feb 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a lot of precedent for this. I think technically we could likely remove the code when the last version that had the old default falls out of support. You could leave a comment/TODO about this and we can circle back later? cc @envoyproxy/envoy-maintainers

} 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;
Comment thread
giantcroc marked this conversation as resolved.
Outdated
}
admin_ = std::make_unique<AdminImpl>(initial_config.admin().profilePath(), *this,
initial_config.admin().ignoreGlobalConnLimit());
Expand Down Expand Up @@ -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");
Comment thread
giantcroc marked this conversation as resolved.
}

return false; // for gcc
Expand Down
2 changes: 1 addition & 1 deletion source/server/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ class InstanceImpl final : Logger::Loggable<Logger::Id::main>,
registerCallback(Stage stage, StageCallbackWithCompletion callback) override;

private:
enum class ReusePortDefault { True, False, Runtime };
enum class ReusePortDefault { True, False };
Comment thread
giantcroc marked this conversation as resolved.
Outdated

ProtobufTypes::MessagePtr dumpBootstrapConfig();
void flushStatsInternal();
Expand Down
27 changes: 0 additions & 27 deletions test/server/server_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Comment thread
giantcroc marked this conversation as resolved.
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) {
Comment thread
giantcroc marked this conversation as resolved.
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";
Expand Down
10 changes: 0 additions & 10 deletions test/server/test_data/server/runtime_bootstrap.yaml
View file
Open in desktop

This file was deleted.