From 9060f754aefee9e45aa2daafaac056d37158bc53 Mon Sep 17 00:00:00 2001 From: Looong Dai Date: Wed, 19 Jan 2022 01:40:19 +0800 Subject: [PATCH] runtime: envoy.reloadable_features.use_observable_cluster_name removal (#19475) See #15139 ([cluster] Use alt_stat_name for general observability purposes (access log, tracing, admin)), which introduced a runtime guarded feature, which has been enabled by default for 6 months, so remove the old code path. Risk Level: Low Testing: n/a Docs Changes: updated Release Notes: Deprecate envoy.reloadable_features.use_observable_cluster_name. Platform Specific Features: n/a Signed-off-by: Loong Signed-off-by: Josh Perry --- api/envoy/config/cluster/v3/cluster.proto | 8 +++----- .../observability/access_log/usage.rst | 3 +-- docs/root/version_history/current.rst | 1 + .../common/formatter/substitution_formatter.cc | 9 ++------- source/common/runtime/runtime_features.cc | 1 - .../formatter/substitution_formatter_test.cc | 17 ----------------- test/common/tcp_proxy/tcp_proxy_test.cc | 13 ------------- 7 files changed, 7 insertions(+), 45 deletions(-) diff --git a/api/envoy/config/cluster/v3/cluster.proto b/api/envoy/config/cluster/v3/cluster.proto index 704cfd3347d2..e98414f8f2a4 100644 --- a/api/envoy/config/cluster/v3/cluster.proto +++ b/api/envoy/config/cluster/v3/cluster.proto @@ -735,11 +735,9 @@ message Cluster { // emitting stats for the cluster and access logging the cluster name. This will appear as // additional information in configuration dumps of a cluster's current status as // :ref:`observability_name ` - // and as an additional tag "upstream_cluster.name" while tracing. Note: access logging using - // this field is presently enabled with runtime feature - // `envoy.reloadable_features.use_observable_cluster_name`. Any ``:`` in the name will be - // converted to ``_`` when emitting statistics. This should not be confused with :ref:`Router - // Filter Header `. + // and as an additional tag "upstream_cluster.name" while tracing. Note: Any ``:`` in the name + // will be converted to ``_`` when emitting statistics. This should not be confused with + // :ref:`Router Filter Header `. string alt_stat_name = 28 [(udpa.annotations.field_migrate).rename = "observability_name"]; oneof cluster_discovery_type { diff --git a/docs/root/configuration/observability/access_log/usage.rst b/docs/root/configuration/observability/access_log/usage.rst index 8599f838f4f3..afaa7938b3fd 100644 --- a/docs/root/configuration/observability/access_log/usage.rst +++ b/docs/root/configuration/observability/access_log/usage.rst @@ -406,8 +406,7 @@ The following command operators are supported: Upstream host URL (e.g., tcp://ip:port for TCP connections). %UPSTREAM_CLUSTER% - Upstream cluster to which the upstream host belongs to. If runtime feature - ``envoy.reloadable_features.use_observable_cluster_name`` is enabled, then :ref:`alt_stat_name + Upstream cluster to which the upstream host belongs to. :ref:`alt_stat_name ` will be used if provided. %UPSTREAM_LOCAL_ADDRESS% diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f66453d09e8b..b339fc803873 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -27,6 +27,7 @@ Removed Config or Runtime * http: removed ``envoy.reloadable_features.hash_multiple_header_values`` and legacy code paths. * http: removed ``envoy.reloadable_features.require_strict_1xx_and_204_response_headers`` and ``envoy.reloadable_features.send_strict_1xx_and_204_response_headers`` and legacy code paths. * http: removed ``envoy.reloadable_features.strip_port_from_connect`` and legacy code paths. +* http: removed ``envoy.reloadable_features.use_observable_cluster_name`` and legacy code paths. New Features diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 368a51d79968..9c4b04639c81 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -856,13 +856,8 @@ const StreamInfoFormatter::FieldExtractorLookupTbl& StreamInfoFormatter::getKnow std::string upstream_cluster_name; if (stream_info.upstreamClusterInfo().has_value() && stream_info.upstreamClusterInfo().value() != nullptr) { - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.use_observable_cluster_name")) { - upstream_cluster_name = - stream_info.upstreamClusterInfo().value()->observabilityName(); - } else { - upstream_cluster_name = stream_info.upstreamClusterInfo().value()->name(); - } + upstream_cluster_name = + stream_info.upstreamClusterInfo().value()->observabilityName(); } return upstream_cluster_name.empty() diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 0058f1ad5605..4240b02dbaf1 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -87,7 +87,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.unquote_log_string_values", "envoy.reloadable_features.update_expected_rq_timeout_on_retry", "envoy.reloadable_features.use_dns_ttl", - "envoy.reloadable_features.use_observable_cluster_name", "envoy.reloadable_features.validate_connect", "envoy.reloadable_features.vhds_heartbeats", "envoy.restart_features.explicit_wildcard_resource", diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index fb0e627534e7..4df8aadd9bd3 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -539,23 +539,6 @@ TEST(SubstitutionFormatterTest, streamInfoFormatter) { ProtoEq(ValueUtil::stringValue("observability_name"))); } - { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.use_observable_cluster_name", "false"}}); - StreamInfoFormatter upstream_format("UPSTREAM_CLUSTER"); - const std::string upstream_cluster_name = "cluster_name"; - auto cluster_info_mock = std::make_shared(); - absl::optional cluster_info = cluster_info_mock; - EXPECT_CALL(stream_info, upstreamClusterInfo()).WillRepeatedly(Return(cluster_info)); - EXPECT_CALL(*cluster_info_mock, name()).WillRepeatedly(ReturnRef(upstream_cluster_name)); - EXPECT_EQ("cluster_name", upstream_format.format(request_headers, response_headers, - response_trailers, stream_info, body)); - EXPECT_THAT(upstream_format.formatValue(request_headers, response_headers, response_trailers, - stream_info, body), - ProtoEq(ValueUtil::stringValue("cluster_name"))); - } - { StreamInfoFormatter upstream_format("UPSTREAM_CLUSTER"); absl::optional cluster_info = nullptr; diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index f7b5549bf6fd..ffb154a72195 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -852,19 +852,6 @@ TEST_F(TcpProxyTest, AccessLogUpstreamHost) { EXPECT_EQ(access_log_data_, "127.0.0.1:80 observability_name"); } -// Test that access log fields %UPSTREAM_HOST% and %UPSTREAM_CLUSTER% are correctly logged with the -// cluster name. -TEST_F(TcpProxyTest, AccessLogUpstreamHostLegacyName) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.use_observable_cluster_name", "false"}}); - setup(1, accessLogConfig("%UPSTREAM_HOST% %UPSTREAM_CLUSTER%")); - raiseEventUpstreamConnected(0); - filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); - filter_.reset(); - EXPECT_EQ(access_log_data_, "127.0.0.1:80 fake_cluster"); -} - // Test that access log field %UPSTREAM_LOCAL_ADDRESS% is correctly logged. TEST_F(TcpProxyTest, AccessLogUpstreamLocalAddress) { setup(1, accessLogConfig("%UPSTREAM_LOCAL_ADDRESS%"));