From 090c8a4193ce96b6a24d85b65268e216769ea182 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Tue, 15 Oct 2024 19:32:15 +0000 Subject: [PATCH 1/2] wasm: remove redundant xds attributes Change-Id: I152e259f3f8e5f69463a64fedce4181ce606824b Signed-off-by: Kuat Yessenov --- changelogs/current.yaml | 3 + .../arch_overview/advanced/attributes.rst | 8 --- source/extensions/common/wasm/context.cc | 63 +------------------ .../http/wasm/test_data/metadata_rust.rs | 2 +- .../filters/http/wasm/test_data/test_cpp.cc | 25 +++----- .../filters/http/wasm/wasm_filter_test.cc | 8 --- 6 files changed, 14 insertions(+), 95 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9ecf0d6e48ce..6e13c1cbac6f 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -2,6 +2,9 @@ date: Pending behavior_changes: # *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* +- area: wasm + change: | + Remove previously deprecated xDS attributes from ``get_property``, use ``xds`` attributes instead. minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* diff --git a/docs/root/intro/arch_overview/advanced/attributes.rst b/docs/root/intro/arch_overview/advanced/attributes.rst index f3f8e6fb84c0..30c5578841a3 100644 --- a/docs/root/intro/arch_overview/advanced/attributes.rst +++ b/docs/root/intro/arch_overview/advanced/attributes.rst @@ -211,14 +211,6 @@ In addition to all above, the following extra attributes are available to Wasm e plugin_name, string, Plugin name plugin_root_id, string, Plugin root ID plugin_vm_id, string, Plugin VM ID - node, :ref:`Node`, Local node description. DEPRECATED: please use `xds` attributes. - cluster_name, string, Upstream cluster name. DEPRECATED: please use `xds` attributes. - cluster_metadata, :ref:`Metadata`, Upstream cluster metadata. DEPRECATED: please use `xds` attributes. - listener_direction, int, Enumeration value of the :ref:`listener traffic direction`. DEPRECATED: please use `xds` attributes. - listener_metadata, :ref:`Metadata`, Listener metadata. DEPRECATED: please use `xds` attributes. - route_name, string, Route name. DEPRECATED: please use `xds` attributes. - route_metadata, :ref:`Metadata`, Route metadata. DEPRECATED: please use `xds` attributes. - upstream_host_metadata, :ref:`Metadata`, Upstream host metadata. DEPRECATED: please use `xds` attributes. Path expressions ---------------- diff --git a/source/extensions/common/wasm/context.cc b/source/extensions/common/wasm/context.cc index 64c34a6f20bd..80e477d70125 100644 --- a/source/extensions/common/wasm/context.cc +++ b/source/extensions/common/wasm/context.cc @@ -100,13 +100,6 @@ Http::RequestHeaderMapPtr buildRequestHeaderMapFromPairs(const Pairs& pairs) { template static uint32_t headerSize(const P& p) { return p ? p->size() : 0; } -Upstream::HostDescriptionConstSharedPtr getHost(const StreamInfo::StreamInfo* info) { - if (info && info->upstreamInfo() && info->upstreamInfo().value().get().upstreamHost()) { - return info->upstreamInfo().value().get().upstreamHost(); - } - return nullptr; -} - } // namespace // Test support. @@ -435,10 +428,7 @@ WasmResult serializeValue(Filters::Common::Expr::CelValue value, std::string* re return WasmResult::SerializationFailure; } -#define PROPERTY_TOKENS(_f) \ - _f(NODE) _f(LISTENER_DIRECTION) _f(LISTENER_METADATA) _f(CLUSTER_NAME) _f(CLUSTER_METADATA) \ - _f(ROUTE_NAME) _f(ROUTE_METADATA) _f(PLUGIN_NAME) _f(UPSTREAM_HOST_METADATA) \ - _f(PLUGIN_ROOT_ID) _f(PLUGIN_VM_ID) _f(CONNECTION_ID) +#define PROPERTY_TOKENS(_f) _f(PLUGIN_NAME) _f(PLUGIN_ROOT_ID) _f(PLUGIN_VM_ID) _f(CONNECTION_ID) static inline std::string downCase(std::string s) { std::transform(s.begin(), s.end(), s.begin(), [](unsigned char c) { return std::tolower(c); }); @@ -513,57 +503,6 @@ Context::findValue(absl::string_view name, Protobuf::Arena* arena, bool last) co } break; } - case PropertyToken::NODE: - if (root_local_info_) { - return CelProtoWrapper::CreateMessage(&root_local_info_->node(), arena); - } else if (plugin_) { - return CelProtoWrapper::CreateMessage(&plugin()->localInfo().node(), arena); - } - break; - case PropertyToken::LISTENER_DIRECTION: - if (plugin_) { - return CelValue::CreateInt64(plugin()->direction()); - } - break; - case PropertyToken::LISTENER_METADATA: - if (plugin_) { - return CelProtoWrapper::CreateMessage(plugin()->listenerMetadata(), arena); - } - break; - case PropertyToken::CLUSTER_NAME: - if (getHost(info)) { - return CelValue::CreateString(&getHost(info)->cluster().name()); - } else if (info && info->route() && info->route()->routeEntry()) { - return CelValue::CreateString(&info->route()->routeEntry()->clusterName()); - } else if (info && info->upstreamClusterInfo().has_value() && - info->upstreamClusterInfo().value()) { - return CelValue::CreateString(&info->upstreamClusterInfo().value()->name()); - } - break; - case PropertyToken::CLUSTER_METADATA: - if (getHost(info)) { - return CelProtoWrapper::CreateMessage(&getHost(info)->cluster().metadata(), arena); - } else if (info && info->upstreamClusterInfo().has_value() && - info->upstreamClusterInfo().value()) { - return CelProtoWrapper::CreateMessage(&info->upstreamClusterInfo().value()->metadata(), - arena); - } - break; - case PropertyToken::UPSTREAM_HOST_METADATA: - if (getHost(info)) { - return CelProtoWrapper::CreateMessage(getHost(info)->metadata().get(), arena); - } - break; - case PropertyToken::ROUTE_NAME: - if (info) { - return CelValue::CreateString(&info->getRouteName()); - } - break; - case PropertyToken::ROUTE_METADATA: - if (info && info->route()) { - return CelProtoWrapper::CreateMessage(&info->route()->metadata(), arena); - } - break; case PropertyToken::PLUGIN_NAME: if (plugin_) { return CelValue::CreateStringView(plugin()->name_); diff --git a/test/extensions/filters/http/wasm/test_data/metadata_rust.rs b/test/extensions/filters/http/wasm/test_data/metadata_rust.rs index 901ca7dfa870..597ded5f4a8e 100644 --- a/test/extensions/filters/http/wasm/test_data/metadata_rust.rs +++ b/test/extensions/filters/http/wasm/test_data/metadata_rust.rs @@ -59,7 +59,7 @@ impl HttpContext for TestStream { } fn on_http_request_body(&mut self, _: usize, _: bool) -> Action { - if let Some(value) = self.get_property(vec!["node", "metadata", "wasm_node_get_key"]) { + if let Some(value) = self.get_property(vec!["xds", "node", "metadata", "wasm_node_get_key"]) { error!("onBody {}", String::from_utf8(value).unwrap()); } else { debug!("missing node metadata"); diff --git a/test/extensions/filters/http/wasm/test_data/test_cpp.cc b/test/extensions/filters/http/wasm/test_data/test_cpp.cc index 16fbd59f3c9f..a419c589a036 100644 --- a/test/extensions/filters/http/wasm/test_data/test_cpp.cc +++ b/test/extensions/filters/http/wasm/test_data/test_cpp.cc @@ -67,7 +67,7 @@ bool TestRootContext::onConfigure(size_t size) { "string_state", "metadata", "request", "response", "connection", "connection_id", "upstream", "source", "destination", "cluster_name", "cluster_metadata", "route_name", "route_metadata", "upstream_host_metadata", - "filter_state", + "filter_state", "listener_direction" ,"listener_metadata", }; for (const auto& property : properties) { if (getProperty({property}).has_value()) { @@ -80,8 +80,6 @@ bool TestRootContext::onConfigure(size_t size) { std::vector, std::string>> properties = { {{"plugin_name"}, "plugin_name"}, {{"plugin_vm_id"}, "vm_id"}, - {{"listener_direction"}, std::string("\x1\0\0\0\0\0\0\0\0", 8)}, // INBOUND - {{"listener_metadata"}, ""}, {{"xds", "node", "metadata", "istio.io/metadata"}, "sample_data"}, }; for (const auto& property : properties) { @@ -324,7 +322,7 @@ FilterDataStatus TestContext::onRequestBody(size_t body_buffer_length, bool end_ } } else if (test == "metadata") { std::string value; - if (!getValue({"node", "metadata", "wasm_node_get_key"}, &value)) { + if (!getValue({"xds", "node", "metadata", "wasm_node_get_key"}, &value)) { logDebug("missing node metadata"); } logError(std::string("onBody ") + value); @@ -377,7 +375,7 @@ void TestContext::onLog() { } } else if (test == "cluster_metadata") { std::string cluster_metadata; - if (getValue({"cluster_metadata", "filter_metadata", "namespace", "key"}, &cluster_metadata)) { + if (getValue({"xds", "cluster_metadata", "filter_metadata", "namespace", "key"}, &cluster_metadata)) { logWarn("cluster metadata: " + cluster_metadata); } } else if (test == "property") { @@ -386,7 +384,7 @@ void TestContext::onLog() { if (path->view() == "/test_context") { logWarn("request.path: " + getProperty({"request", "path"}).value()->toString()); logWarn("node.metadata: " + - getProperty({"node", "metadata", "istio.io/metadata"}).value()->toString()); + getProperty({"xds", "node", "metadata", "istio.io/metadata"}).value()->toString()); logWarn("metadata: " + getProperty({"metadata", "filter_metadata", "envoy.filters.http.wasm", "wasm_request_get_key"}) .value() @@ -396,7 +394,7 @@ void TestContext::onLog() { logWarn("response.code: " + std::to_string(responseCode)); } std::string upstream_host_metadata; - if (getValue({"upstream_host_metadata", "filter_metadata", "namespace", "key"}, &upstream_host_metadata)) { + if (getValue({"xds", "upstream_host_metadata", "filter_metadata", "namespace", "key"}, &upstream_host_metadata)) { logWarn("upstream host metadata: " + upstream_host_metadata); } logWarn("state: " + getProperty({"wasm_state"}).value()->toString()); @@ -588,16 +586,11 @@ void TestContext::onLog() { std::vector, std::string>> properties = { {{"plugin_name"}, "plugin_name"}, {{"plugin_vm_id"}, "vm_id"}, - {{"listener_direction"}, std::string("\x1\0\0\0\0\0\0\0\0", 8)}, // INBOUND - {{"listener_metadata"}, ""}, - {{"route_name"}, "route12"}, - {{"cluster_name"}, "fake_cluster"}, {{"connection_id"}, std::string("\x4\0\0\0\0\0\0\0\0", 8)}, {{"connection", "requested_server_name"}, "w3.org"}, {{"source", "address"}, "127.0.0.1:0"}, {{"destination", "address"}, "127.0.0.2:0"}, {{"upstream", "address"}, "10.0.0.1:443"}, - {{"route_metadata"}, ""}, }; for (const auto& property : properties) { std::string value; @@ -629,22 +622,22 @@ void TestRootContext::onTick() { } } else if (test_ == "metadata") { // NOLINT(clang-analyzer-optin.portability.UnixAPI) std::string value; - if (!getValue({"node", "metadata", "wasm_node_get_key"}, &value)) { // NOLINT(clang-analyzer-optin.portability.UnixAPI) + if (!getValue({"xds", "node", "metadata", "wasm_node_get_key"}, &value)) { // NOLINT(clang-analyzer-optin.portability.UnixAPI) logDebug("missing node metadata"); } logDebug(std::string("onTick ") + value); std::string list_value; - if (!getValue({"node", "metadata", "wasm_node_list_key", "0"}, &list_value)) { + if (!getValue({"xds", "node", "metadata", "wasm_node_list_key", "0"}, &list_value)) { logDebug("missing node metadata list value"); } if (list_value != "wasm_node_get_value") { logWarn("unexpected list value: " + list_value); } - if (getValue({"node", "metadata", "wasm_node_list_key", "bad_key"}, &list_value)) { + if (getValue({"xds", "node", "metadata", "wasm_node_list_key", "bad_key"}, &list_value)) { logDebug("unexpected list value for a bad_key"); } - if (getValue({"node", "metadata", "wasm_node_list_key", "1"}, &list_value)) { + if (getValue({"xds", "node", "metadata", "wasm_node_list_key", "1"}, &list_value)) { logDebug("unexpected list value outside the range"); } } else if (test_ == "property") { diff --git a/test/extensions/filters/http/wasm/wasm_filter_test.cc b/test/extensions/filters/http/wasm/wasm_filter_test.cc index cfaee6b90df3..8ffd4263898f 100644 --- a/test/extensions/filters/http/wasm/wasm_filter_test.cc +++ b/test/extensions/filters/http/wasm/wasm_filter_test.cc @@ -1835,8 +1835,6 @@ TEST_P(WasmHttpFilterTest, ClusterMetadata) { } setupTest("", "cluster_metadata"); setupFilter(); - EXPECT_CALL(filter(), - log_(spdlog::level::warn, Eq(absl::string_view("cluster metadata: cluster")))); auto cluster = std::make_shared>(); auto cluster_metadata = std::make_shared( TestUtility::parseYaml( @@ -1852,14 +1850,8 @@ TEST_P(WasmHttpFilterTest, ClusterMetadata) { EXPECT_CALL(encoder_callbacks_, streamInfo()).WillRepeatedly(ReturnRef(request_stream_info_)); EXPECT_CALL(*cluster, metadata()).WillRepeatedly(ReturnRef(*cluster_metadata)); - EXPECT_CALL(*host_description, cluster()).WillRepeatedly(ReturnRef(*cluster)); - request_stream_info_.upstreamInfo()->setUpstreamHost(host_description); EXPECT_CALL(request_stream_info_, requestComplete) .WillRepeatedly(Return(std::chrono::milliseconds(30))); - filter().log({&request_headers}, request_stream_info_); - - // If upstream host is empty, fallback to upstream cluster info for cluster metadata. - request_stream_info_.upstreamInfo()->setUpstreamHost(nullptr); EXPECT_CALL(request_stream_info_, upstreamClusterInfo()).WillRepeatedly(Return(cluster)); EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("cluster metadata: cluster")))); From 11d81e7860790e0733cff2d40af35756b944aed3 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 16 Oct 2024 16:29:54 +0000 Subject: [PATCH 2/2] delete dead code Change-Id: I729d48ded84ce3451c936cbe228eb3b0f2fc04d0 Signed-off-by: Kuat Yessenov --- source/extensions/common/wasm/remote_async_datasource.cc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/source/extensions/common/wasm/remote_async_datasource.cc b/source/extensions/common/wasm/remote_async_datasource.cc index b3e8989e7a9a..ec51b0facb5d 100644 --- a/source/extensions/common/wasm/remote_async_datasource.cc +++ b/source/extensions/common/wasm/remote_async_datasource.cc @@ -13,12 +13,6 @@ static constexpr uint32_t RetryInitialDelayMilliseconds = 1000; static constexpr uint32_t RetryMaxDelayMilliseconds = 10 * 1000; static constexpr uint32_t RetryCount = 1; -absl::optional getPath(const envoy::config::core::v3::DataSource& source) { - return source.specifier_case() == envoy::config::core::v3::DataSource::SpecifierCase::kFilename - ? absl::make_optional(source.filename()) - : absl::nullopt; -} - RemoteAsyncDataProvider::RemoteAsyncDataProvider( Upstream::ClusterManager& cm, Init::Manager& manager, const envoy::config::core::v3::RemoteDataSource& source, Event::Dispatcher& dispatcher,