Skip to content

dfp: deprecate flag dfp_cluster_resolves_hosts and remove legacy code paths #41557

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

dfp: deprecate flag dfp_cluster_resolves_hosts and remove legacy code paths #41557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ removed_config_or_runtime:
- area: http
change: |
Removed runtime guard ``envoy.reloadable_features.http1_balsa_allow_cr_or_lf_at_request_start`` and legacy code paths.
- area: dfp
change: |
Removed runtime guard ``envoy.reloadable_features.dfp_cluster_resolves_hosts`` and legacy code paths.

new_features:

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 @@ -36,7 +36,6 @@
// problem of the bugs being found after the old code path has been removed.
RUNTIME_GUARD(envoy_reloadable_features_async_host_selection);
RUNTIME_GUARD(envoy_reloadable_features_decouple_explicit_drain_pools_and_dns_refresh);
RUNTIME_GUARD(envoy_reloadable_features_dfp_cluster_resolves_hosts);
RUNTIME_GUARD(envoy_reloadable_features_disallow_quic_client_udp_mmsg);
RUNTIME_GUARD(envoy_reloadable_features_enable_cel_regex_precompilation);
RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection);
Expand Down
4 changes: 1 addition & 3 deletions source/extensions/clusters/dynamic_forward_proxy/cluster.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,10 +432,8 @@ Cluster::LoadBalancer::chooseHost(Upstream::LoadBalancerContext* context) {
Upstream::HostConstSharedPtr host = findHostByName(hostname);
bool force_refresh =
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.reresolve_if_no_connections") &&
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts") &&
host && !host->used();
if ((host && !force_refresh) ||
!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts")) {
if (host && !force_refresh) {
return {host};
}

Expand Down
36 changes: 0 additions & 36 deletions test/extensions/filters/http/dynamic_forward_proxy/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,39 +112,3 @@ envoy_extension_cc_test(
"@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto",
],
)

envoy_extension_cc_test(
name = "legacy_proxy_filter_integration_test",
size = "large",
srcs = ["proxy_filter_integration_test.cc"],
args = [
"--runtime-feature-disable-for-tests=envoy.reloadable_features.dfp_cluster_resolves_hosts",
],
data = [
"//test/config/integration/certs",
],
extension_names = ["envoy.filters.http.dynamic_forward_proxy"],
rbe_pool = "2core",
# TODO(envoyproxy/windows-dev): Diagnose failure shown on clang-cl build, see:
# https://gist.github.com/wrowe/a152cb1d12c2f751916122aed39d8517
tags = ["fails_on_clang_cl"],
deps = [
":modify_host_filter_lib",
":test_resolver_lib",
"//source/extensions/clusters/dns:dns_cluster_lib",
"//source/extensions/clusters/dynamic_forward_proxy:cluster",
"//source/extensions/filters/http/dynamic_forward_proxy:config",
"//source/extensions/filters/http/set_filter_state:config",
"//source/extensions/key_value/file_based:config_lib",
"//source/extensions/network/dns_resolver/getaddrinfo:config",
"//test/integration:http_integration_lib",
"//test/integration/filters:stream_info_to_headers_filter_lib",
"//test/test_common:test_runtime_lib",
"//test/test_common:threadsafe_singleton_injector_lib",
"@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
"@envoy_api//envoy/config/cluster/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/transport_sockets/quic/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto",
],
)
44 changes: 9 additions & 35 deletions test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,9 @@ name: stream-info-to-headers-filter

if (prepend_custom_filter_config_yaml.has_value()) {
// Prepend DFP filter.
if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts") ||
use_dfp_even_when_cluster_resolves_hosts) {
if (use_dfp_even_when_cluster_resolves_hosts) {
config_helper_.prependFilter(use_sub_cluster ? filter_use_sub_cluster
: filter_use_dns_cache);
} else if (use_sub_cluster) {
config_helper_.addRuntimeOverride("envoy.reloadable_features.dfp_cluster_resolves_hosts",
"false");
config_helper_.prependFilter(filter_use_sub_cluster);
}

// Prepend the custom_filter from the parameter.
Expand All @@ -164,14 +159,9 @@ name: stream-info-to-headers-filter
// Prepend stream_info_filter.
config_helper_.prependFilter(stream_info_filter_config_str);
} else {
if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts") ||
use_dfp_even_when_cluster_resolves_hosts) {
if (use_dfp_even_when_cluster_resolves_hosts) {
config_helper_.prependFilter(use_sub_cluster ? filter_use_sub_cluster
: filter_use_dns_cache);
} else if (use_sub_cluster) {
config_helper_.addRuntimeOverride("envoy.reloadable_features.dfp_cluster_resolves_hosts",
"false");
config_helper_.prependFilter(filter_use_sub_cluster);
}

config_helper_.prependFilter(stream_info_filter_config_str);
Expand Down Expand Up @@ -687,21 +677,11 @@ TEST_P(ProxyFilterIntegrationTest, DisableRefreshOnFailureContainsSuccessfulHost
EXPECT_EQ(1, test_server_->counter("dns_cache.foo.dns_query_attempt")->value());

// This should result in a cache hit because the previous DNS query succeeded.
if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts")) {
EXPECT_LOG_CONTAINS("debug", "cache hit for host 'localhost", {
auto response2 = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
ASSERT_TRUE(response2->waitForEndStream());
EXPECT_EQ("200", response2->headers().getStatusValue());
// No new query attempt.
EXPECT_EQ(1, test_server_->counter("dns_cache.foo.dns_query_attempt")->value());
});
} else {
auto response2 = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
ASSERT_TRUE(response2->waitForEndStream());
EXPECT_EQ("200", response2->headers().getStatusValue());
// No new query attempt.
EXPECT_EQ(1, test_server_->counter("dns_cache.foo.dns_query_attempt")->value());
}
auto response2 = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
ASSERT_TRUE(response2->waitForEndStream());
EXPECT_EQ("200", response2->headers().getStatusValue());
// No new query attempt.
EXPECT_EQ(1, test_server_->counter("dns_cache.foo.dns_query_attempt")->value());
}

// TODO(yanavlasov) Enable per #26642
Expand Down Expand Up @@ -788,13 +768,8 @@ TEST_P(ProxyFilterIntegrationTest, RequestWithSuspectDomain) {

auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
ASSERT_TRUE(response->waitForEndStream());
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts")) {
// The suspicious host will be set to an empty host resulting in host not found (503)
EXPECT_EQ("503", response->headers().getStatusValue());
} else {
// The suspicious host will be set to an empty host resulting in a bad request (400)
EXPECT_EQ("400", response->headers().getStatusValue());
}
// The suspicious host will be set to an empty host resulting in host not found (503).
EXPECT_EQ("503", response->headers().getStatusValue());
std::string access_log = waitForAccessLog(access_log_name_);
EXPECT_THAT(access_log, HasSubstr("empty_host_header"));
EXPECT_FALSE(StringUtil::hasEmptySpace(access_log));
Expand Down Expand Up @@ -1687,7 +1662,6 @@ TEST_P(ProxyFilterIntegrationTest, ResetStreamDuringDnsLookup) {
// also requires the Host header to be modified between DFP and Router filters to trigger abnormal
// behavior in the DNS resolution processing loop.
TEST_P(ProxyFilterIntegrationTest, DoubleResolution) {
config_helper_.addRuntimeOverride("envoy.reloadable_features.dfp_cluster_resolves_hosts", "true");
config_helper_.addRuntimeOverride(
"envoy.reloadable_features.skip_dns_lookup_for_proxied_requests", "true");
upstream_tls_ = false;
Expand Down
6 changes: 2 additions & 4 deletions test/integration/filters/stream_info_to_headers_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,8 @@ class StreamInfoToHeadersFilter : public Http::PassThroughFilter {
Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool) override {
std::string dns_start = "envoy.dynamic_forward_proxy.dns_start_ms";
std::string dns_end = "envoy.dynamic_forward_proxy.dns_end_ms";
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts")) {
dns_start = "envoy.router.host_selection_start_ms";
dns_end = "envoy.router.host_selection_end_ms";
}
dns_start = "envoy.router.host_selection_start_ms";
dns_end = "envoy.router.host_selection_end_ms";
StreamInfo::StreamInfo& stream_info = decoder_callbacks_->streamInfo();
const StreamInfo::StreamInfo& conn_stream_info = decoder_callbacks_->connection()->streamInfo();

Expand Down
Loading