From e8448e89608e4823bd1bd531b8ec8743fc259e44 Mon Sep 17 00:00:00 2001 From: Nigel Brittain <108375408+nbaws@users.noreply.github.com> Date: Fri, 7 Jun 2024 00:28:29 +1000 Subject: [PATCH 01/30] aws: add nbaws as aws codeowner (#34376) Signed-off-by: Nigel Brittain --- CODEOWNERS | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CODEOWNERS b/CODEOWNERS index e0d262de1d4d..030d1385a036 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -90,8 +90,8 @@ extensions/filters/common/original_src @klarose @mattklein123 /*/extensions/filters/http/cache @toddmgreer @jmarantz @penguingao @mpwarres @capoferro /*/extensions/http/cache/simple_http_cache @toddmgreer @jmarantz @penguingao @mpwarres @capoferro # aws_iam grpc credentials -/*/extensions/grpc_credentials/aws_iam @suniltheta @lavignes @mattklein123 -/*/extensions/common/aws @suniltheta @lavignes @mattklein123 +/*/extensions/grpc_credentials/aws_iam @suniltheta @mattklein123 @nbaws +/*/extensions/common/aws @suniltheta @mattklein123 @nbaws # adaptive concurrency limit extension. /*/extensions/filters/http/adaptive_concurrency @tonya11en @mattklein123 # admission control extension. @@ -153,8 +153,8 @@ extensions/filters/common/original_src @klarose @mattklein123 # support for on-demand VHDS requests /*/extensions/filters/http/on_demand @dmitri-d @htuch @kyessenov /*/extensions/filters/network/connection_limit @mattklein123 @alyssawilk @delong-coder -/*/extensions/filters/http/aws_request_signing @derekargueta @suniltheta @mattklein123 @marcomagdy -/*/extensions/filters/http/aws_lambda @suniltheta @mattklein123 @marcomagdy @lavignes +/*/extensions/filters/http/aws_request_signing @derekargueta @suniltheta @mattklein123 @marcomagdy @nbaws +/*/extensions/filters/http/aws_lambda @suniltheta @mattklein123 @marcomagdy @nbaws /*/extensions/filters/http/buffer @alyssawilk @mattklein123 /*/extensions/transport_sockets/raw_buffer @alyssawilk @mattklein123 # Watchdog Extensions From 8a6570ed0706b410fb7032c00f1d0d01795824e2 Mon Sep 17 00:00:00 2001 From: Sankalp <125449768+sankalpjha555@users.noreply.github.com> Date: Thu, 6 Jun 2024 20:53:37 +0530 Subject: [PATCH 02/30] Fix mongo_proxy:codec_impl_test and mongo_proxy:bson_impl_test on big endian (#34453) Signed-off-by: sankalpjha555 --- .../extensions/filters/network/mongo_proxy/bson_impl.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/source/extensions/filters/network/mongo_proxy/bson_impl.cc b/source/extensions/filters/network/mongo_proxy/bson_impl.cc index 8ea6cf028376..669f26e08917 100644 --- a/source/extensions/filters/network/mongo_proxy/bson_impl.cc +++ b/source/extensions/filters/network/mongo_proxy/bson_impl.cc @@ -23,7 +23,11 @@ int32_t BufferHelper::peekInt32(Buffer::Instance& data) { int32_t val; val = data.peekLEInt(); +#ifdef ABSL_IS_BIG_ENDIAN + return val; +#else return le32toh(val); +#endif } uint8_t BufferHelper::removeByte(Buffer::Instance& data) { @@ -88,7 +92,11 @@ int64_t BufferHelper::removeInt64(Buffer::Instance& data) { int64_t val; val = data.drainLEInt(); +#ifdef ABSL_IS_BIG_ENDIAN + return val; +#else return le64toh(val); +#endif } std::string BufferHelper::removeString(Buffer::Instance& data) { From bb3b8834be96aa07e0780ba5ff4534393ce3e445 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Thu, 6 Jun 2024 21:37:58 +0530 Subject: [PATCH 03/30] add support for dynamic direct response for files (#32789) * add support for dynamic direct response for files Signed-off-by: Rama Chavali * add locking Signed-off-by: Rama Chavali * fix test and add release notes Signed-off-by: Rama Chavali * improve lamda Signed-off-by: Rama Chavali * move to tls storage Signed-off-by: Rama Chavali * compile errors Signed-off-by: Rama Chavali * move to shared pointer with mutex Signed-off-by: Rama Chavali * fix it Signed-off-by: Rama Chavali * use new datasource provider Signed-off-by: Rama Chavali * fix format Signed-off-by: Rama Chavali * fix test Signed-off-by: Rama Chavali * fix condition Signed-off-by: Rama Chavali * change false Signed-off-by: Rama Chavali * move to create Signed-off-by: Rama Chavali * fix test Signed-off-by: Rama Chavali * fix test Signed-off-by: Rama Chavali --------- Signed-off-by: Rama Chavali --- changelogs/current.yaml | 6 ++ source/common/config/datasource.cc | 26 ++++++--- source/common/config/datasource.h | 14 +++-- source/common/router/config_impl.cc | 14 +++-- source/common/router/config_impl.h | 8 ++- test/common/config/datasource_test.cc | 16 ++--- .../direct_response_integration_test.cc | 58 +++++++++++++++++++ 7 files changed, 113 insertions(+), 29 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 19bbdf4287c9..7c20910cd10c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -248,6 +248,12 @@ new_features: added :ref:`stat_prefix ` configuration to support additional stat prefix for the OpenTelemetry logger. +- area: routing + change: | + added support in :ref:`file datasource ` implementation + to listen to file changes and dynamically update the response when :ref:`watched_directory + ` + is configured in :ref:`DataSource `. - area: listener change: | Added :ref:`bypass_overload_manager ` diff --git a/source/common/config/datasource.cc b/source/common/config/datasource.cc index 95816710b1b2..8cb923f74aeb 100644 --- a/source/common/config/datasource.cc +++ b/source/common/config/datasource.cc @@ -100,29 +100,36 @@ DynamicData::~DynamicData() { } } -absl::string_view DynamicData::data() const { +const std::string& DynamicData::data() const { const auto thread_local_data = slot_->get(); return thread_local_data.has_value() ? *thread_local_data->data_ : EMPTY_STRING; } -absl::string_view DataSourceProvider::data() const { +const std::string& DataSourceProvider::data() const { if (absl::holds_alternative(data_)) { return absl::get(data_); } return absl::get(data_).data(); } -absl::StatusOr DataSourceProvider::create(const ProtoDataSource& source, - Event::Dispatcher& main_dispatcher, - ThreadLocal::SlotAllocator& tls, - Api::Api& api, bool allow_empty, - uint64_t max_size) { +absl::StatusOr DataSourceProvider::create(const ProtoDataSource& source, + Event::Dispatcher& main_dispatcher, + ThreadLocal::SlotAllocator& tls, + Api::Api& api, bool allow_empty, + uint64_t max_size) { auto initial_data_or_error = read(source, allow_empty, api, max_size); RETURN_IF_STATUS_NOT_OK(initial_data_or_error); if (!source.has_watched_directory() || source.specifier_case() != envoy::config::core::v3::DataSource::kFilename) { - return DataSourceProvider(std::move(initial_data_or_error).value()); + if (source.specifier_case() != envoy::config::core::v3::DataSource::kFilename && + initial_data_or_error.value().length() > max_size) { + return absl::InvalidArgumentError(fmt::format("response body size is {} bytes; maximum is {}", + initial_data_or_error.value().length(), + max_size)); + } + return std::unique_ptr( + new DataSourceProvider(std::move(initial_data_or_error).value())); } auto slot = ThreadLocal::TypedSlot::makeUnique(tls); @@ -157,7 +164,8 @@ absl::StatusOr DataSourceProvider::create(const ProtoDataSou }); RETURN_IF_NOT_OK(watcher_status); - return DataSourceProvider(DynamicData(main_dispatcher, std::move(slot), std::move(watcher))); + return std::unique_ptr( + new DataSourceProvider(DynamicData(main_dispatcher, std::move(slot), std::move(watcher)))); } } // namespace DataSource diff --git a/source/common/config/datasource.h b/source/common/config/datasource.h index 1c0f47879392..6dc6131338f9 100644 --- a/source/common/config/datasource.h +++ b/source/common/config/datasource.h @@ -20,8 +20,11 @@ namespace Envoy { namespace Config { namespace DataSource { +class DataSourceProvider; + using ProtoDataSource = envoy::config::core::v3::DataSource; using ProtoWatchedDirectory = envoy::config::core::v3::WatchedDirectory; +using DataSourceProviderPtr = std::unique_ptr; /** * Read contents of the DataSource. @@ -54,7 +57,7 @@ class DynamicData { Filesystem::WatcherPtr watcher); ~DynamicData(); - absl::string_view data() const; + const std::string& data() const; private: Event::Dispatcher& dispatcher_; @@ -85,12 +88,11 @@ class DataSourceProvider { * NOTE: If file watch is enabled and the new file content does not meet the * requirements (allow_empty, max_size), the provider will keep the old content. */ - static absl::StatusOr create(const ProtoDataSource& source, - Event::Dispatcher& main_dispatcher, - ThreadLocal::SlotAllocator& tls, Api::Api& api, - bool allow_empty, uint64_t max_size = 0); + static absl::StatusOr + create(const ProtoDataSource& source, Event::Dispatcher& main_dispatcher, + ThreadLocal::SlotAllocator& tls, Api::Api& api, bool allow_empty, uint64_t max_size = 0); - absl::string_view data() const; + const std::string& data() const; private: DataSourceProvider(std::string&& data) : data_(std::move(data)) {} diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 612efe3489f4..6384850d3b4b 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -572,10 +572,16 @@ RouteEntryImplBase::RouteEntryImplBase(const CommonVirtualHostSharedPtr& vhost, using_new_timeouts_(route.route().has_max_stream_duration()), match_grpc_(route.match().has_grpc()), case_sensitive_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(route.match(), case_sensitive, true)) { - auto body_or_error = ConfigUtility::parseDirectResponseBody( - route, factory_context.api(), vhost_->globalRouteConfig().maxDirectResponseBodySizeBytes()); - SET_AND_RETURN_IF_NOT_OK(body_or_error.status(), creation_status); - direct_response_body_ = body_or_error.value(); + + if (route.has_direct_response() && route.direct_response().has_body()) { + + auto provider_or_error = Envoy::Config::DataSource::DataSourceProvider::create( + route.direct_response().body(), factory_context.mainThreadDispatcher(), + factory_context.threadLocal(), factory_context.api(), true, + vhost_->globalRouteConfig().maxDirectResponseBodySizeBytes()); + SET_AND_RETURN_IF_NOT_OK(provider_or_error.status(), creation_status); + direct_response_body_provider_ = std::move(provider_or_error.value()); + } if (!route.request_headers_to_add().empty() || !route.request_headers_to_remove().empty()) { request_headers_parser_ = THROW_OR_RETURN_VALUE( HeaderParser::configure(route.request_headers_to_add(), route.request_headers_to_remove()), diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 38ea31d46b41..a70e5c8669b0 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -24,6 +24,7 @@ #include "source/common/common/matchers.h" #include "source/common/common/packed_struct.h" +#include "source/common/config/datasource.h" #include "source/common/config/metadata.h" #include "source/common/http/hash_policy.h" #include "source/common/http/header_utility.h" @@ -798,7 +799,10 @@ class RouteEntryImplBase : public RouteEntryAndRoute, std::string newUri(const Http::RequestHeaderMap& headers) const override; void rewritePathHeader(Http::RequestHeaderMap&, bool) const override {} Http::Code responseCode() const override { return direct_response_code_.value(); } - const std::string& responseBody() const override { return direct_response_body_; } + const std::string& responseBody() const override { + return direct_response_body_provider_ != nullptr ? direct_response_body_provider_->data() + : EMPTY_STRING; + } // Router::Route const DirectResponseEntry* directResponseEntry() const override; @@ -1227,7 +1231,7 @@ class RouteEntryImplBase : public RouteEntryAndRoute, const DecoratorConstPtr decorator_; const RouteTracingConstPtr route_tracing_; - std::string direct_response_body_; + Envoy::Config::DataSource::DataSourceProviderPtr direct_response_body_provider_; PerFilterConfigs per_filter_configs_; const std::string route_name_; TimeSource& time_source_; diff --git a/test/common/config/datasource_test.cc b/test/common/config/datasource_test.cc index 015613eaa695..9c4ee31aa56c 100644 --- a/test/common/config/datasource_test.cc +++ b/test/common/config/datasource_test.cc @@ -176,8 +176,8 @@ TEST(DataSourceProviderTest, NonFileDataSourceTest) { NiceMock tls; auto provider_or_error = - DataSource::DataSourceProvider::create(config, *dispatcher, tls, *api, false, 0); - EXPECT_EQ(provider_or_error.value().data(), "Hello, world!"); + DataSource::DataSourceProvider::create(config, *dispatcher, tls, *api, false, 15); + EXPECT_EQ(provider_or_error.value()->data(), "Hello, world!"); } TEST(DataSourceProviderTest, FileDataSourceButNoWatch) { @@ -218,7 +218,7 @@ TEST(DataSourceProviderTest, FileDataSourceButNoWatch) { auto provider_or_error = DataSource::DataSourceProvider::create(config, *dispatcher, tls, *api, false, 0); - EXPECT_EQ(provider_or_error.value().data(), "Hello, world!"); + EXPECT_EQ(provider_or_error.value()->data(), "Hello, world!"); // Update the symlink to point to the new file. TestEnvironment::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"), @@ -227,7 +227,7 @@ TEST(DataSourceProviderTest, FileDataSourceButNoWatch) { dispatcher->run(Event::Dispatcher::RunType::NonBlock); // The provider should still return the old content. - EXPECT_EQ(provider_or_error.value().data(), "Hello, world!"); + EXPECT_EQ(provider_or_error.value()->data(), "Hello, world!"); // Remove the file. unlink(TestEnvironment::temporaryPath("envoy_test/watcher_target").c_str()); @@ -278,7 +278,7 @@ TEST(DataSourceProviderTest, FileDataSourceAndWithWatch) { // Create a provider with watch. auto provider_or_error = DataSource::DataSourceProvider::create(config, *dispatcher, tls, *api, false, 0); - EXPECT_EQ(provider_or_error.value().data(), "Hello, world!"); + EXPECT_EQ(provider_or_error.value()->data(), "Hello, world!"); // Update the symlink to point to the new file. TestEnvironment::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"), @@ -287,7 +287,7 @@ TEST(DataSourceProviderTest, FileDataSourceAndWithWatch) { dispatcher->run(Event::Dispatcher::RunType::NonBlock); // The provider should return the updated content. - EXPECT_EQ(provider_or_error.value().data(), "Hello, world! Updated!"); + EXPECT_EQ(provider_or_error.value()->data(), "Hello, world! Updated!"); // Remove the file. unlink(TestEnvironment::temporaryPath("envoy_test/watcher_target").c_str()); @@ -339,7 +339,7 @@ TEST(DataSourceProviderTest, FileDataSourceAndWithWatchButUpdateError) { // ignored. auto provider_or_error = DataSource::DataSourceProvider::create(config, *dispatcher, tls, *api, false, 15); - EXPECT_EQ(provider_or_error.value().data(), "Hello, world!"); + EXPECT_EQ(provider_or_error.value()->data(), "Hello, world!"); // Update the symlink to point to the new file. TestEnvironment::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"), @@ -348,7 +348,7 @@ TEST(DataSourceProviderTest, FileDataSourceAndWithWatchButUpdateError) { dispatcher->run(Event::Dispatcher::RunType::NonBlock); // The provider should return the old content because the updated content is ignored. - EXPECT_EQ(provider_or_error.value().data(), "Hello, world!"); + EXPECT_EQ(provider_or_error.value()->data(), "Hello, world!"); // Remove the file. unlink(TestEnvironment::temporaryPath("envoy_test/watcher_target").c_str()); diff --git a/test/integration/direct_response_integration_test.cc b/test/integration/direct_response_integration_test.cc index bc79926acaff..7000502d0b74 100644 --- a/test/integration/direct_response_integration_test.cc +++ b/test/integration/direct_response_integration_test.cc @@ -44,6 +44,62 @@ class DirectResponseIntegrationTest : public testing::TestWithParambody().size()); EXPECT_EQ(body_content, response->body()); } + + // Test direct response with a file as the body. + void testDirectResponseFile() { + TestEnvironment::writeStringToFileForTest("file_direct.txt", "dummy"); + const std::string filename = TestEnvironment::temporaryPath("file_direct.txt"); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + auto* route_config = hcm.mutable_route_config(); + auto* route = route_config->mutable_virtual_hosts(0)->mutable_routes(0); + + route->mutable_match()->set_prefix("/direct"); + + auto* direct_response = route->mutable_direct_response(); + direct_response->set_status(200); + direct_response->mutable_body()->set_filename(filename); + direct_response->mutable_body()->mutable_watched_directory()->set_path( + TestEnvironment::temporaryDirectory()); + }); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/direct"}, + {":scheme", "http"}, + {":authority", "host"}, + }); + auto response = std::move(encoder_decoder.second); + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ("dummy", response->body()); + + codec_client_->close(); + + // Update the file and validate that the response is updated. + TestEnvironment::writeStringToFileForTest("file_direct_updated.txt", "dummy-updated"); + TestEnvironment::renameFile(TestEnvironment::temporaryPath("file_direct_updated.txt"), + TestEnvironment::temporaryPath("file_direct.txt")); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder_updated = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/direct"}, + {":scheme", "http"}, + {":authority", "host"}, + }); + auto updated_response = std::move(encoder_decoder_updated.second); + ASSERT_TRUE(updated_response->waitForEndStream()); + ASSERT_TRUE(updated_response->complete()); + EXPECT_EQ("200", updated_response->headers().getStatusValue()); + EXPECT_EQ("dummy-updated", updated_response->body()); + codec_client_->close(); + } }; INSTANTIATE_TEST_SUITE_P(IpVersions, DirectResponseIntegrationTest, @@ -65,4 +121,6 @@ TEST_P(DirectResponseIntegrationTest, DirectResponseBodySizeSmall) { testDirectResponseBodySize(1); } +TEST_P(DirectResponseIntegrationTest, DefaultDirectResponseFile) { testDirectResponseFile(); } + } // namespace Envoy From fbce85914421145b5ae3210c9313eced63e535b0 Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Thu, 6 Jun 2024 11:58:20 -0500 Subject: [PATCH 04/30] mobile: Remove unnecessary copies in the JvmFilterContext (#34590) The filter implementations don't typically run in a separate thread, so creating a copy is unnecessary. Risk Level: low Testing: unit tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: mobile Signed-off-by: Fredy Wijaya --- .../envoymobile/engine/JvmFilterContext.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/mobile/library/java/io/envoyproxy/envoymobile/engine/JvmFilterContext.java b/mobile/library/java/io/envoyproxy/envoymobile/engine/JvmFilterContext.java index 59822278408f..9c665a699689 100644 --- a/mobile/library/java/io/envoyproxy/envoymobile/engine/JvmFilterContext.java +++ b/mobile/library/java/io/envoyproxy/envoymobile/engine/JvmFilterContext.java @@ -68,11 +68,8 @@ public Object onRequestHeaders(long headerCount, boolean endStream, long[] strea * @return Object[], pair of HTTP filter status and optional modified data. */ public Object onRequestData(ByteBuffer data, boolean endStream, long[] streamIntel) { - // Create a copy of the `data` because the `data` uses direct `ByteBuffer` and the `data` will - // be destroyed after calling this callback. - ByteBuffer copiedData = ByteBuffers.copy(data); return toJniFilterDataStatus( - filter.onRequestData(copiedData, endStream, new EnvoyStreamIntel(streamIntel))); + filter.onRequestData(data, endStream, new EnvoyStreamIntel(streamIntel))); } /** @@ -113,11 +110,8 @@ public Object onResponseHeaders(long headerCount, boolean endStream, long[] stre * @return Object[], pair of HTTP filter status and optional modified data. */ public Object onResponseData(ByteBuffer data, boolean endStream, long[] streamIntel) { - // Create a copy of the `data` because the `data` uses direct `ByteBuffer` and the `data` will - // be destroyed after calling this callback. - ByteBuffer copiedData = ByteBuffers.copy(data); return toJniFilterDataStatus( - filter.onResponseData(copiedData, endStream, new EnvoyStreamIntel(streamIntel))); + filter.onResponseData(data, endStream, new EnvoyStreamIntel(streamIntel))); } /** From 84d8fdd11e78013cd50596fa3b704e152512455e Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Thu, 6 Jun 2024 13:57:57 -0500 Subject: [PATCH 05/30] mobile: Fix RtdsIntegrationTest (#34592) Fixes #34537 Risk Level: low Testing: unit test Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Fredy Wijaya --- mobile/test/common/integration/rtds_integration_test.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/mobile/test/common/integration/rtds_integration_test.cc b/mobile/test/common/integration/rtds_integration_test.cc index 60c72c66cc93..8845abd36188 100644 --- a/mobile/test/common/integration/rtds_integration_test.cc +++ b/mobile/test/common/integration/rtds_integration_test.cc @@ -29,7 +29,7 @@ class RtdsIntegrationTest : public XdsIntegrationTest { void createEnvoy() override { Platform::XdsBuilder xds_builder( /*xds_server_address=*/Network::Test::getLoopbackAddressUrlString(ipVersion()), - /*xds_server_port=*/fake_upstreams_[1]->localAddress()->ip()->port()); + /*xds_server_port=*/fake_upstreams_.back()->localAddress()->ip()->port()); // Add the layered runtime config, which includes the RTDS layer. xds_builder.addRuntimeDiscoveryService("some_rtds_resource", /*timeout_in_seconds=*/1) .setSslRootCerts(getUpstreamCert()); @@ -40,6 +40,7 @@ class RtdsIntegrationTest : public XdsIntegrationTest { void SetUp() override { initialize(); } void runReloadTest() { + stream_ = createNewStream(createDefaultStreamCallbacks()); // Send a request on the data plane. stream_->sendHeaders(std::make_unique(default_request_headers_), true); @@ -100,14 +101,12 @@ INSTANTIATE_TEST_SUITE_P( // Envoy Mobile's xDS APIs only support state-of-the-world, not delta. testing::Values(Grpc::SotwOrDelta::Sotw, Grpc::SotwOrDelta::UnifiedSotw))); -// https://github.com/envoyproxy/envoy/issues/34537 -TEST_P(RtdsIntegrationTest, DISABLED_RtdsReloadWithDfpMixedScheme) { +TEST_P(RtdsIntegrationTest, RtdsReloadWithDfpMixedScheme) { TestScopedStaticReloadableFeaturesRuntime scoped_runtime({{"dfp_mixed_scheme", true}}); runReloadTest(); } -// https://github.com/envoyproxy/envoy/issues/34537 -TEST_P(RtdsIntegrationTest, DISABLED_RtdsReloadWithoutDfpMixedScheme) { +TEST_P(RtdsIntegrationTest, RtdsReloadWithoutDfpMixedScheme) { TestScopedStaticReloadableFeaturesRuntime scoped_runtime({{"dfp_mixed_scheme", false}}); runReloadTest(); } From 139fc0fb504850e6d16d7d5e5838956f76a1b27b Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Thu, 6 Jun 2024 14:30:21 -0500 Subject: [PATCH 06/30] mobile: Add protocol info in the error details (#34571) Risk Level: low Testing: unit tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: mobile Signed-off-by: Fredy Wijaya --- mobile/library/common/http/client.cc | 7 ++- .../java/org/chromium/net/impl/Errors.java | 2 +- mobile/test/common/http/client_test.cc | 6 +-- .../chromium/net/CronetUrlRequestTest.java | 52 ++++++++++--------- 4 files changed, 38 insertions(+), 29 deletions(-) diff --git a/mobile/library/common/http/client.cc b/mobile/library/common/http/client.cc index 9db7e5db94f5..463bebdd97d0 100644 --- a/mobile/library/common/http/client.cc +++ b/mobile/library/common/http/client.cc @@ -454,12 +454,17 @@ void Client::DirectStreamCallbacks::latchError() { std::transform(info.responseFlags().begin(), info.responseFlags().end(), response_flags.begin(), [](StreamInfo::ResponseFlag flag) { return std::to_string(flag.value()); }); error_msg_details.push_back(absl::StrCat("RESPONSE_FLAGS: ", absl::StrJoin(response_flags, ","))); + if (info.protocol().has_value()) { + // https://github.com/envoyproxy/envoy/blob/fbce85914421145b5ae3210c9313eced63e535b0/envoy/http/protocol.h#L13 + error_msg_details.push_back(absl::StrCat("PROTOCOL: ", *info.protocol())); + } if (std::string resp_code_details = info.responseCodeDetails().value_or(""); !resp_code_details.empty()) { error_msg_details.push_back(absl::StrCat("DETAILS: ", std::move(resp_code_details))); } // The format of the error message propogated to callbacks is: - // RESPONSE_CODE: {value}|ERROR_CODE: {value}|RESPONSE_FLAGS: {value}|DETAILS: {value} + // RESPONSE_CODE: {value}|ERROR_CODE: {value}|RESPONSE_FLAGS: {value}|PROTOCOL: {value}|DETAILS: + // {value} // // Where RESPONSE_CODE is the HTTP response code from StreamInfo::responseCode(). // ERROR_CODE is of the envoy_error_code_t enum type, and gets mapped from RESPONSE_CODE. diff --git a/mobile/library/java/org/chromium/net/impl/Errors.java b/mobile/library/java/org/chromium/net/impl/Errors.java index 7c6aa9132b7e..c5709aba3b78 100644 --- a/mobile/library/java/org/chromium/net/impl/Errors.java +++ b/mobile/library/java/org/chromium/net/impl/Errors.java @@ -71,7 +71,7 @@ public String toString() { /** * Maps Envoymobile's errorcode to chromium's net errorcode - * @param responseFlag envoymobile's finalStreamIntel responseFlag + * @param finalStreamIntel envoymobile's finalStreamIntel * @return the NetError that the EnvoyMobileError maps to */ public static NetError mapEnvoyMobileErrorToNetError(EnvoyFinalStreamIntel finalStreamIntel) { diff --git a/mobile/test/common/http/client_test.cc b/mobile/test/common/http/client_test.cc index a65e38333642..8d3f8d286314 100644 --- a/mobile/test/common/http/client_test.cc +++ b/mobile/test/common/http/client_test.cc @@ -477,9 +477,8 @@ TEST_P(ClientTest, EnvoyLocalError) { stream_callbacks.on_error_ = [&](const EnvoyError& error, envoy_stream_intel, envoy_final_stream_intel) -> void { EXPECT_EQ(error.error_code_, ENVOY_CONNECTION_FAILURE); - EXPECT_THAT( - error.message_, - Eq("RESPONSE_CODE: 503|ERROR_CODE: 2|RESPONSE_FLAGS: 4,26|DETAILS: failed miserably")); + EXPECT_THAT(error.message_, Eq("RESPONSE_CODE: 503|ERROR_CODE: 2|RESPONSE_FLAGS: " + "4,26|PROTOCOL: 3|DETAILS: failed miserably")); EXPECT_EQ(error.attempt_count_, 123); callbacks_called.on_error_calls_++; }; @@ -503,6 +502,7 @@ TEST_P(ClientTest, EnvoyLocalError) { stream_info_.setResponseFlag(StreamInfo::ResponseFlag(StreamInfo::UpstreamRemoteReset)); stream_info_.setResponseFlag(StreamInfo::ResponseFlag(StreamInfo::DnsResolutionFailed)); stream_info_.setAttemptCount(123); + EXPECT_CALL(stream_info_, protocol()).WillRepeatedly(Return(Http::Protocol::Http3)); response_encoder_->getStream().resetStream(Http::StreamResetReason::LocalConnectionFailure); ASSERT_EQ(callbacks_called.on_headers_calls_, 0); // Ensure that the callbacks on the EnvoyStreamCallbacks were called. diff --git a/mobile/test/java/org/chromium/net/CronetUrlRequestTest.java b/mobile/test/java/org/chromium/net/CronetUrlRequestTest.java index a442014b3315..7e8c6d8f39ec 100644 --- a/mobile/test/java/org/chromium/net/CronetUrlRequestTest.java +++ b/mobile/test/java/org/chromium/net/CronetUrlRequestTest.java @@ -2055,25 +2055,29 @@ public void testDestroyUploadDataStreamAdapterOnSucceededCallback() throws Excep @SmallTest @Feature({"Cronet"}) public void testErrorCodes() throws Exception { - checkSpecificErrorCode(EnvoyMobileError.DNS_RESOLUTION_FAILED, NetError.ERR_NAME_NOT_RESOLVED, - NetworkException.ERROR_HOSTNAME_NOT_RESOLVED, "NAME_NOT_RESOLVED", false, - /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 26"); - checkSpecificErrorCode(EnvoyMobileError.UPSTREAM_CONNECTION_TERMINATION, - NetError.ERR_CONNECTION_CLOSED, NetworkException.ERROR_CONNECTION_CLOSED, - "CONNECTION_CLOSED", true, - /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 6"); - checkSpecificErrorCode(EnvoyMobileError.UPSTREAM_CONNECTION_FAILURE, - NetError.ERR_CONNECTION_REFUSED, - NetworkException.ERROR_CONNECTION_REFUSED, "CONNECTION_REFUSED", false, - /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 5"); - checkSpecificErrorCode(EnvoyMobileError.UPSTREAM_REMOTE_RESET, NetError.ERR_CONNECTION_RESET, - NetworkException.ERROR_CONNECTION_RESET, "CONNECTION_RESET", true, - /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 4"); - checkSpecificErrorCode(EnvoyMobileError.STREAM_IDLE_TIMEOUT, NetError.ERR_TIMED_OUT, - NetworkException.ERROR_TIMED_OUT, "TIMED_OUT", true, - /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 16"); - checkSpecificErrorCode(0x2000, NetError.ERR_OTHER, NetworkException.ERROR_OTHER, "OTHER", false, - /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 13"); + checkSpecificErrorCode( + EnvoyMobileError.DNS_RESOLUTION_FAILED, NetError.ERR_NAME_NOT_RESOLVED, + NetworkException.ERROR_HOSTNAME_NOT_RESOLVED, "NAME_NOT_RESOLVED", false, + /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 26|PROTOCOL: 1"); + checkSpecificErrorCode( + EnvoyMobileError.UPSTREAM_CONNECTION_TERMINATION, NetError.ERR_CONNECTION_CLOSED, + NetworkException.ERROR_CONNECTION_CLOSED, "CONNECTION_CLOSED", true, + /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 6|PROTOCOL: 1"); + checkSpecificErrorCode( + EnvoyMobileError.UPSTREAM_CONNECTION_FAILURE, NetError.ERR_CONNECTION_REFUSED, + NetworkException.ERROR_CONNECTION_REFUSED, "CONNECTION_REFUSED", false, + /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 5|PROTOCOL: 1"); + checkSpecificErrorCode( + EnvoyMobileError.UPSTREAM_REMOTE_RESET, NetError.ERR_CONNECTION_RESET, + NetworkException.ERROR_CONNECTION_RESET, "CONNECTION_RESET", true, + /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 4|PROTOCOL: 1"); + checkSpecificErrorCode( + EnvoyMobileError.STREAM_IDLE_TIMEOUT, NetError.ERR_TIMED_OUT, + NetworkException.ERROR_TIMED_OUT, "TIMED_OUT", true, + /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 16|PROTOCOL: 1"); + checkSpecificErrorCode( + 0x2000, NetError.ERR_OTHER, NetworkException.ERROR_OTHER, "OTHER", false, + /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 13|PROTOCOL: 1"); } /* @@ -2098,7 +2102,7 @@ public void testInternetDisconnectedError() throws Exception { checkSpecificErrorCode( EnvoyMobileError.DNS_RESOLUTION_FAILED, NetError.ERR_INTERNET_DISCONNECTED, NetworkException.ERROR_INTERNET_DISCONNECTED, "INTERNET_DISCONNECTED", false, - /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 26"); + /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 26|PROTOCOL: 1"); // bring back online since the AndroidNetworkMonitor class is a singleton connectivityManager.setActiveNetworkInfo(networkInfo); @@ -2316,10 +2320,10 @@ public void test404Head() throws Exception { NativeTestServer.getFileURL("/notfound.html"), callback, callback.getExecutor()); builder.setHttpMethod("HEAD").build().start(); callback.blockForDone(); - checkSpecificErrorCode(EnvoyMobileError.UPSTREAM_CONNECTION_FAILURE, - NetError.ERR_CONNECTION_REFUSED, - NetworkException.ERROR_CONNECTION_REFUSED, "CONNECTION_REFUSED", false, - /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 5"); + checkSpecificErrorCode( + EnvoyMobileError.UPSTREAM_CONNECTION_FAILURE, NetError.ERR_CONNECTION_REFUSED, + NetworkException.ERROR_CONNECTION_REFUSED, "CONNECTION_REFUSED", false, + /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0|RESPONSE_FLAGS: 5|PROTOCOL: 1"); } @Test From 7fcc47414c9ebc3915616730612b0608031ea8e9 Mon Sep 17 00:00:00 2001 From: Dennis Kniep Date: Fri, 7 Jun 2024 03:45:45 +0200 Subject: [PATCH 07/30] oauth filter: preserve existing auth header (#34470) Allows to preserve the exsting authorization header in oauth2 filter Signed-off-by: Dennis Kniep --- .../filters/http/oauth2/v3/oauth.proto | 8 ++- .../extensions/filters/http/oauth2/config.cc | 7 +++ .../extensions/filters/http/oauth2/filter.cc | 12 ++-- .../extensions/filters/http/oauth2/filter.h | 2 + .../filters/http/oauth2/config_test.cc | 58 +++++++++++++++++++ .../filters/http/oauth2/filter_test.cc | 51 +++++++++++++++- 6 files changed, 132 insertions(+), 6 deletions(-) diff --git a/api/envoy/extensions/filters/http/oauth2/v3/oauth.proto b/api/envoy/extensions/filters/http/oauth2/v3/oauth.proto index aa5f70b2897a..22696745f630 100644 --- a/api/envoy/extensions/filters/http/oauth2/v3/oauth.proto +++ b/api/envoy/extensions/filters/http/oauth2/v3/oauth.proto @@ -74,7 +74,7 @@ message OAuth2Credentials { // OAuth config // -// [#next-free-field: 16] +// [#next-free-field: 17] message OAuth2Config { enum AuthType { // The ``client_id`` and ``client_secret`` will be sent in the URL encoded request body. @@ -111,6 +111,12 @@ message OAuth2Config { // Forward the OAuth token as a Bearer to upstream web service. bool forward_bearer_token = 7; + // If set to true, preserve the existing authorization header. + // By default Envoy strips the existing authorization header before forwarding upstream. + // Can not be set to true if forward_bearer_token is already set to true. + // Default value is false. + bool preserve_authorization_header = 16; + // Any request that matches any of the provided matchers will be passed through without OAuth validation. repeated config.route.v3.HeaderMatcher pass_through_matcher = 8; diff --git a/source/extensions/filters/http/oauth2/config.cc b/source/extensions/filters/http/oauth2/config.cc index 771b4d307457..322c6e2907db 100644 --- a/source/extensions/filters/http/oauth2/config.cc +++ b/source/extensions/filters/http/oauth2/config.cc @@ -64,6 +64,13 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped( throw EnvoyException("invalid HMAC secret configuration"); } + if (proto_config.preserve_authorization_header() && proto_config.forward_bearer_token()) { + throw EnvoyException( + "invalid combination of forward_bearer_token and preserve_authorization_header " + "configuration. If forward_bearer_token is set to true, then " + "preserve_authorization_header must be false"); + } + auto secret_reader = std::make_shared( std::move(secret_provider_token_secret), std::move(secret_provider_hmac_secret), context.serverFactoryContext().threadLocal(), context.serverFactoryContext().api()); diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc index 8ccf894bfa9e..d5d97279cf7c 100644 --- a/source/extensions/filters/http/oauth2/filter.cc +++ b/source/extensions/filters/http/oauth2/filter.cc @@ -198,6 +198,7 @@ FilterConfig::FilterConfig( stats_(FilterConfig::generateStats(stats_prefix, scope)), encoded_resource_query_params_(encodeResourceList(proto_config.resources())), forward_bearer_token_(proto_config.forward_bearer_token()), + preserve_authorization_header_(proto_config.preserve_authorization_header()), pass_through_header_matchers_(headerMatchers(proto_config.pass_through_matcher(), context)), deny_redirect_header_matchers_(headerMatchers(proto_config.deny_redirect_matcher(), context)), cookie_names_(proto_config.credentials().cookie_names()), @@ -289,10 +290,13 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he } } - // Sanitize the Authorization header, since we have no way to validate its content. Also, - // if token forwarding is enabled, this header will be set based on what is on the HMAC cookie - // before forwarding the request upstream. - headers.removeInline(authorization_handle.handle()); + // Only sanitize the Authorization header if preserveAuthorizationHeader is false + if (!config_->preserveAuthorizationHeader()) { + // Sanitize the Authorization header, since we have no way to validate its content. Also, + // if token forwarding is enabled, this header will be set based on what is on the HMAC cookie + // before forwarding the request upstream. + headers.removeInline(authorization_handle.handle()); + } // The following 2 headers are guaranteed for regular requests. The asserts are helpful when // writing test code to not forget these important variables in mock requests diff --git a/source/extensions/filters/http/oauth2/filter.h b/source/extensions/filters/http/oauth2/filter.h index 69b1acfad215..f1ed914a931b 100644 --- a/source/extensions/filters/http/oauth2/filter.h +++ b/source/extensions/filters/http/oauth2/filter.h @@ -119,6 +119,7 @@ class FilterConfig { const std::string& clusterName() const { return oauth_token_endpoint_.cluster(); } const std::string& clientId() const { return client_id_; } bool forwardBearerToken() const { return forward_bearer_token_; } + bool preserveAuthorizationHeader() const { return preserve_authorization_header_; } const std::vector& passThroughMatchers() const { return pass_through_header_matchers_; } @@ -164,6 +165,7 @@ class FilterConfig { const std::string encoded_auth_scopes_; const std::string encoded_resource_query_params_; const bool forward_bearer_token_ : 1; + const bool preserve_authorization_header_ : 1; const std::vector pass_through_header_matchers_; const std::vector deny_redirect_header_matchers_; const CookieNames cookie_names_; diff --git a/test/extensions/filters/http/oauth2/config_test.cc b/test/extensions/filters/http/oauth2/config_test.cc index 8abe960cd904..e16fb0ca9b9d 100644 --- a/test/extensions/filters/http/oauth2/config_test.cc +++ b/test/extensions/filters/http/oauth2/config_test.cc @@ -201,6 +201,64 @@ TEST(ConfigTest, WrongCookieName) { EnvoyException, "value does not match regex pattern"); } +TEST(ConfigTest, WrongCombinationOfPreserveAuthorizationAndForwardBearer) { + const std::string yaml = R"EOF( +config: + forward_bearer_token: true + preserve_authorization_header: true + token_endpoint: + cluster: foo + uri: oauth.com/token + timeout: 3s + credentials: + client_id: "secret" + token_secret: + name: token + hmac_secret: + name: hmac + cookie_names: + bearer_token: BearerToken + oauth_hmac: OauthHMAC + oauth_expires: OauthExpires + id_token: IdToken + refresh_token: RefreshToken + authorization_endpoint: https://oauth.com/oauth/authorize/ + redirect_uri: "%REQ(x-forwarded-proto)%://%REQ(:authority)%/callback" + redirect_path_matcher: + path: + exact: /callback + signout_path: + path: + exact: /signout + auth_scopes: + - user + - openid + - email + resources: + - oauth2-resource + - http://example.com + - https://example.com + )EOF"; + + OAuth2Config factory; + ProtobufTypes::MessagePtr proto_config = factory.createEmptyConfigProto(); + TestUtility::loadFromYaml(yaml, *proto_config); + NiceMock context; + context.server_factory_context_.cluster_manager_.initializeClusters({"foo"}, {}); + + // This returns non-nullptr for token_secret and hmac_secret. + auto& secret_manager = + context.server_factory_context_.cluster_manager_.cluster_manager_factory_.secretManager(); + ON_CALL(secret_manager, findStaticGenericSecretProvider(_)) + .WillByDefault(Return(std::make_shared( + envoy::extensions::transport_sockets::tls::v3::GenericSecret()))); + + EXPECT_THROW_WITH_REGEX( + factory.createFilterFactoryFromProto(*proto_config, "stats", context).status().IgnoreError(), + EnvoyException, + "invalid combination of forward_bearer_token and preserve_authorization_header"); +} + } // namespace Oauth2 } // namespace HttpFilters } // namespace Extensions diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 99ee26e79585..78a49df1e710 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -112,7 +112,7 @@ class OAuth2Test : public testing::TestWithParam { ::envoy::extensions::filters::http::oauth2::v3::OAuth2Config_AuthType auth_type = ::envoy::extensions::filters::http::oauth2::v3::OAuth2Config_AuthType:: OAuth2Config_AuthType_URL_ENCODED_BODY, - int default_refresh_token_expires_in = 0) { + int default_refresh_token_expires_in = 0, bool preserve_authorization_header = false) { envoy::extensions::filters::http::oauth2::v3::OAuth2Config p; auto* endpoint = p.mutable_token_endpoint(); endpoint->set_cluster("auth.example.com"); @@ -123,6 +123,7 @@ class OAuth2Test : public testing::TestWithParam { p.set_authorization_endpoint("https://auth.example.com/oauth/authorize/"); p.mutable_signout_path()->mutable_path()->set_exact("/_signout"); p.set_forward_bearer_token(forward_bearer_token); + p.set_preserve_authorization_header(preserve_authorization_header); auto* useRefreshToken = p.mutable_use_refresh_token(); useRefreshToken->set_value(use_refresh_token); @@ -618,6 +619,54 @@ TEST_F(OAuth2Test, OAuthOkPassButInvalidToken) { EXPECT_EQ(scope_.counterFromString("test.oauth_success").value(), 1); } +/** + * Scenario: The OAuth filter receives a request with a foreign token in the Authorization + * header. This header should be forwarded when preserve authorization header is enabled + * and forwarding bearer token is disabled. + * + * Expected behavior: the filter should forward the foreign token and let the request proceed. + */ +TEST_F(OAuth2Test, OAuthOkPreserveForeignAuthHeader) { + init(getConfig(false /* forward_bearer_token */, true /* use_refresh_token */, + ::envoy::extensions::filters::http::oauth2::v3::OAuth2Config_AuthType:: + OAuth2Config_AuthType_URL_ENCODED_BODY /* encoded_body_type */, + 1200 /* default_refresh_token_expires_in */, + true /* preserve_authorization_header */)); + + Http::TestRequestHeaderMapImpl mock_request_headers{ + {Http::Headers::get().Path.get(), "/anypath"}, + {Http::Headers::get().Host.get(), "traffic.example.com"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Scheme.get(), "https"}, + {Http::CustomHeaders::get().Authorization.get(), "Bearer ValidAuthorizationHeader"}, + }; + + Http::TestRequestHeaderMapImpl expected_headers{ + {Http::Headers::get().Path.get(), "/anypath"}, + {Http::Headers::get().Host.get(), "traffic.example.com"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Scheme.get(), "https"}, + {Http::CustomHeaders::get().Authorization.get(), "Bearer ValidAuthorizationHeader"}, + }; + + // cookie-validation mocking + EXPECT_CALL(*validator_, setParams(_, _)); + EXPECT_CALL(*validator_, isValid()).WillOnce(Return(true)); + + // Sanitized return reference mocking + std::string legit_token{"legit_token"}; + EXPECT_CALL(*validator_, token()).WillRepeatedly(ReturnRef(legit_token)); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_->decodeHeaders(mock_request_headers, false)); + + // Ensure that existing OAuth forwarded headers got sanitized. + EXPECT_EQ(mock_request_headers, expected_headers); + + EXPECT_EQ(scope_.counterFromString("test.oauth_failure").value(), 0); + EXPECT_EQ(scope_.counterFromString("test.oauth_success").value(), 1); +} + /** * Scenario: The OAuth filter receives a request without valid OAuth cookies to a non-callback URL * (indicating that the user needs to re-validate cookies or get 401'd). From 74f327dc8b5a0f8ab67d7a47535c88cc89fed681 Mon Sep 17 00:00:00 2001 From: "Antonio V. Leonti" <53806445+antoniovleonti@users.noreply.github.com> Date: Thu, 6 Jun 2024 21:46:20 -0400 Subject: [PATCH 08/30] Add decoder header mutation rules (#34182) * add header mutation rules Signed-off-by: antoniovleonti * fix spelling errors Signed-off-by: antoniovleonti * use proto field link in proto doc string Signed-off-by: antoniovleonti * Fix proto link Signed-off-by: antoniovleonti * fix changelog proto link too Signed-off-by: antoniovleonti * use correct link in changelog Signed-off-by: antoniovleonti * fix integration test Signed-off-by: antoniovleonti * remove redundant LowerCaseString conversion Signed-off-by: antoniovleonti * use decoder_header_mutation_rules as optional field Signed-off-by: antoniovleonti * remove lambda from check header func Signed-off-by: antoniovleonti * fix doc reference/link Signed-off-by: antoniovleonti * formatting fixes Signed-off-by: antoniovleonti --------- Signed-off-by: antoniovleonti Signed-off-by: Antonio V. Leonti <53806445+antoniovleonti@users.noreply.github.com> --- .../filters/http/ext_authz/v3/BUILD | 1 + .../filters/http/ext_authz/v3/ext_authz.proto | 22 +- changelogs/current.yaml | 6 + .../extensions/filters/http/ext_authz/BUILD | 1 + .../filters/http/ext_authz/ext_authz.cc | 142 +++++--- .../filters/http/ext_authz/ext_authz.h | 29 ++ .../ext_authz/ext_authz_integration_test.cc | 139 +++++--- .../filters/http/ext_authz/ext_authz_test.cc | 323 ++++++++++++++++++ 8 files changed, 566 insertions(+), 97 deletions(-) diff --git a/api/envoy/extensions/filters/http/ext_authz/v3/BUILD b/api/envoy/extensions/filters/http/ext_authz/v3/BUILD index cabe849e71d1..7bbb39173ac6 100644 --- a/api/envoy/extensions/filters/http/ext_authz/v3/BUILD +++ b/api/envoy/extensions/filters/http/ext_authz/v3/BUILD @@ -7,6 +7,7 @@ licenses(["notice"]) # Apache 2 api_proto_package( deps = [ "//envoy/annotations:pkg", + "//envoy/config/common/mutation_rules/v3:pkg", "//envoy/config/core/v3:pkg", "//envoy/type/matcher/v3:pkg", "//envoy/type/v3:pkg", diff --git a/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto b/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto index f4750864c91b..986fa2c9166c 100644 --- a/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto +++ b/api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto @@ -2,6 +2,7 @@ syntax = "proto3"; package envoy.extensions.filters.http.ext_authz.v3; +import "envoy/config/common/mutation_rules/v3/mutation_rules.proto"; import "envoy/config/core/v3/base.proto"; import "envoy/config/core/v3/config_source.proto"; import "envoy/config/core/v3/grpc_service.proto"; @@ -28,10 +29,10 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // External Authorization :ref:`configuration overview `. // [#extension: envoy.filters.http.ext_authz] -// [#next-free-field: 26] +// [#next-free-field: 27] message ExtAuthz { option (udpa.annotations.versioning).previous_message_type = - "envoy.config.filter.http.ext_authz.v2.ExtAuthz"; + "envoy.config.filter.http.ext_authz.v3.ExtAuthz"; reserved 4; @@ -261,6 +262,23 @@ message ExtAuthz { // It's recommended you set this to true unless you already rely on the old behavior. False is the // default only for backwards compatibility. bool encode_raw_headers = 23; + + // Rules for what modifications an ext_authz server may make to the request headers before + // continuing decoding / forwarding upstream. + // + // If set to anything, enables header mutation checking against configured rules. Note that + // :ref:`HeaderMutationRules ` + // has defaults that change ext_authz behavior. Also note that if this field is set to anything, + // ext_authz can no longer append to :-prefixed headers. + // + // If empty, header mutation rule checking is completely disabled. + // + // Regardless of what is configured here, ext_authz cannot remove :-prefixed headers. + // + // This field and ``validate_mutations`` have different use cases. ``validate_mutations`` enables + // correctness checks for all header / query parameter mutations (e.g. for invalid characters). + // This field allows the filter to reject mutations to specific headers. + config.common.mutation_rules.v3.HeaderMutationRules decoder_header_mutation_rules = 26; } // Configuration for buffering the request data. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 7c20910cd10c..0f340c48d22c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -238,6 +238,12 @@ new_features: change: | Added support to healthcheck with ProxyProtocol in TCP Healthcheck by setting :ref:`health_check_config `. +- area: ext_authz + change: | + added + :ref:`decoder_header_mutation_rules ` + which allows you to configure what decoder header mutations are allowed from the ext_authz + service as well as whether to fail the downstream request if disallowed mutations are requested. - area: access_log change: | added new ``access_log`` command operators to retrieve upstream connection information change: ``%UPSTREAM_PEER_URI_SAN%``, diff --git a/source/extensions/filters/http/ext_authz/BUILD b/source/extensions/filters/http/ext_authz/BUILD index 6fbb0b871ed1..02970d791e31 100644 --- a/source/extensions/filters/http/ext_authz/BUILD +++ b/source/extensions/filters/http/ext_authz/BUILD @@ -31,6 +31,7 @@ envoy_cc_library( "//source/common/runtime:runtime_protos_lib", "//source/extensions/filters/common/ext_authz:ext_authz_grpc_lib", "//source/extensions/filters/common/ext_authz:ext_authz_http_lib", + "//source/extensions/filters/common/mutation_rules:mutation_rules_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/http/ext_authz/v3:pkg_cc_proto", "@envoy_api//envoy/service/auth/v3:pkg_cc_proto", diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 824682477bc3..88b27ec663a1 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -21,6 +21,8 @@ namespace ExtAuthz { namespace { using MetadataProto = ::envoy::config::core::v3::Metadata; +using Filters::Common::MutationRules::CheckOperation; +using Filters::Common::MutationRules::CheckResult; void fillMetadataContext(const std::vector& source_metadata, const std::vector& metadata_context_namespaces, @@ -289,6 +291,17 @@ void Filter::onDestroy() { } } +CheckResult Filter::validateAndCheckDecoderHeaderMutation( + Filters::Common::MutationRules::CheckOperation operation, absl::string_view key, + absl::string_view value) const { + if (config_->validateMutations() && (!Http::HeaderUtility::headerNameIsValid(key) || + !Http::HeaderUtility::headerValueIsValid(value))) { + return CheckResult::FAIL; + } + // Check header mutation is valid according to configured decoder mutation rules. + return config_->checkDecoderHeaderMutation(operation, Http::LowerCaseString(key), value); +} + void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { state_ = State::Complete; using Filters::Common::ExtAuthz::CheckStatus; @@ -325,69 +338,116 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { ENVOY_STREAM_LOG(trace, "ext_authz filter added header(s) to the request:", *decoder_callbacks_); for (const auto& [key, value] : response->headers_to_set) { - if (config_->validateMutations() && (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value))) { - ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, key, - value); + CheckResult check_result = validateAndCheckDecoderHeaderMutation( + Filters::Common::MutationRules::CheckOperation::SET, key, value); + switch (check_result) { + case CheckResult::OK: + ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); + request_headers_->setCopy(Http::LowerCaseString(key), value); + break; + case CheckResult::IGNORE: + ENVOY_STREAM_LOG(trace, "Ignoring invalid header to set '{}':'{}'.", *decoder_callbacks_, + key, value); + break; + case CheckResult::FAIL: + ENVOY_STREAM_LOG(trace, "Rejecting invalid header to set '{}':'{}'.", *decoder_callbacks_, + key, value); rejectResponse(); return; } - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); - request_headers_->setCopy(Http::LowerCaseString(key), value); } for (const auto& [key, value] : response->headers_to_add) { - if (config_->validateMutations() && (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value))) { - ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, key, - value); + CheckResult check_result = validateAndCheckDecoderHeaderMutation( + Filters::Common::MutationRules::CheckOperation::SET, key, value); + switch (check_result) { + case CheckResult::OK: + ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); + request_headers_->addCopy(Http::LowerCaseString(key), value); + break; + case CheckResult::IGNORE: + ENVOY_STREAM_LOG(trace, "Ignoring invalid header to add '{}':'{}'.", *decoder_callbacks_, + key, value); + break; + case CheckResult::FAIL: + ENVOY_STREAM_LOG(trace, "Rejecting invalid header to add '{}':'{}'.", *decoder_callbacks_, + key, value); rejectResponse(); return; } - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); - request_headers_->addCopy(Http::LowerCaseString(key), value); } for (const auto& [key, value] : response->headers_to_append) { - if (config_->validateMutations() && (!Http::HeaderUtility::headerNameIsValid(key) || - !Http::HeaderUtility::headerValueIsValid(value))) { - ENVOY_STREAM_LOG(trace, "Rejected invalid header '{}':'{}'.", *decoder_callbacks_, key, - value); + CheckResult check_result = validateAndCheckDecoderHeaderMutation( + Filters::Common::MutationRules::CheckOperation::APPEND, key, value); + switch (check_result) { + case CheckResult::OK: { + ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); + Http::LowerCaseString lowercase_key(key); + const auto header_to_modify = request_headers_->get(lowercase_key); + // TODO(dio): Add a flag to allow appending non-existent headers, without setting it + // first (via `headers_to_add`). For example, given: + // 1. Original headers {"original": "true"} + // 2. Response headers from the authorization servers {{"append": "1"}, {"append": + // "2"}} + // + // Currently it is not possible to add {{"append": "1"}, {"append": "2"}} (the + // intended combined headers: {{"original": "true"}, {"append": "1"}, {"append": + // "2"}}) to the request to upstream server by only sets `headers_to_append`. + if (!header_to_modify.empty()) { + ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); + // The current behavior of appending is by combining entries with the same key, + // into one entry. The value of that combined entry is separated by ",". + // TODO(dio): Consider to use addCopy instead. + request_headers_->appendCopy(lowercase_key, value); + } + break; + } + case CheckResult::IGNORE: + ENVOY_STREAM_LOG(trace, "Ignoring invalid header to append '{}':'{}'.", *decoder_callbacks_, + key, value); + break; + case CheckResult::FAIL: + ENVOY_STREAM_LOG(trace, "Rejecting invalid header to append '{}':'{}'.", + *decoder_callbacks_, key, value); rejectResponse(); return; } - Http::LowerCaseString lowercase_key(key); - const auto header_to_modify = request_headers_->get(lowercase_key); - // TODO(dio): Add a flag to allow appending non-existent headers, without setting it first - // (via `headers_to_add`). For example, given: - // 1. Original headers {"original": "true"} - // 2. Response headers from the authorization servers {{"append": "1"}, {"append": "2"}} - // - // Currently it is not possible to add {{"append": "1"}, {"append": "2"}} (the intended - // combined headers: {{"original": "true"}, {"append": "1"}, {"append": "2"}}) to the request - // to upstream server by only sets `headers_to_append`. - if (!header_to_modify.empty()) { - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *decoder_callbacks_, key, value); - // The current behavior of appending is by combining entries with the same key, into one - // entry. The value of that combined entry is separated by ",". - // TODO(dio): Consider to use addCopy instead. - request_headers_->appendCopy(lowercase_key, value); - } } ENVOY_STREAM_LOG(trace, "ext_authz filter removed header(s) from the request:", *decoder_callbacks_); for (const auto& key : response->headers_to_remove) { - // We don't allow removing any :-prefixed headers, nor Host, as removing them would make the - // request malformed. - // // If the response contains an invalid header to remove, it's the same as trying to remove a // header that doesn't exist, so just ignore it. - if (!Http::HeaderUtility::isRemovableHeader(key) || - (config_->validateMutations() && !Http::HeaderUtility::headerNameIsValid(key))) { - ENVOY_STREAM_LOG(trace, "Not removing header '{}'.", *decoder_callbacks_, key); + if (config_->validateMutations() && !Http::HeaderUtility::headerNameIsValid(key)) { + ENVOY_STREAM_LOG(trace, "Ignoring invalid header removal '{}'.", *decoder_callbacks_, key); + continue; + } + // We don't allow removing any :-prefixed headers, nor Host, as removing them would make the + // request malformed. checkDecoderHeaderMutation also performs this check, however, so only + // perform this check explicitly if decoder header mutation rules is empty. + if (!config_->hasDecoderHeaderMutationRules() && + !Http::HeaderUtility::isRemovableHeader(key)) { + ENVOY_STREAM_LOG(trace, "Ignoring invalid header removal '{}'.", *decoder_callbacks_, key); continue; } - ENVOY_STREAM_LOG(trace, "'{}'", *decoder_callbacks_, key); - request_headers_->remove(Http::LowerCaseString(key)); + // Check header mutation is valid according to configured decoder header mutation rules. + Http::LowerCaseString lowercase_key(key); + switch (config_->checkDecoderHeaderMutation(CheckOperation::REMOVE, lowercase_key, + EMPTY_STRING)) { + case CheckResult::OK: + ENVOY_STREAM_LOG(trace, "'{}'", *decoder_callbacks_, key); + request_headers_->remove(lowercase_key); + break; + case CheckResult::IGNORE: + ENVOY_STREAM_LOG(trace, "Ignoring disallowed header removal '{}'.", *decoder_callbacks_, + key); + break; + case CheckResult::FAIL: + ENVOY_STREAM_LOG(trace, "Rejecting disallowed header removal '{}'.", *decoder_callbacks_, + key); + rejectResponse(); + return; + } } if (!response->response_headers_to_add.empty()) { diff --git a/source/extensions/filters/http/ext_authz/ext_authz.h b/source/extensions/filters/http/ext_authz/ext_authz.h index ad2b14f92408..81e6cee23d81 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.h +++ b/source/extensions/filters/http/ext_authz/ext_authz.h @@ -24,6 +24,7 @@ #include "source/extensions/filters/common/ext_authz/ext_authz.h" #include "source/extensions/filters/common/ext_authz/ext_authz_grpc_impl.h" #include "source/extensions/filters/common/ext_authz/ext_authz_http_impl.h" +#include "source/extensions/filters/common/mutation_rules/mutation_rules.h" namespace Envoy { namespace Extensions { @@ -74,6 +75,12 @@ class FilterConfig { status_on_error_(toErrorCode(config.status_on_error().code())), validate_mutations_(config.validate_mutations()), scope_(scope), + decoder_header_mutation_checker_( + config.has_decoder_header_mutation_rules() + ? absl::optional( + Filters::Common::MutationRules::Checker( + config.decoder_header_mutation_rules(), factory_context.regexEngine())) + : absl::nullopt), runtime_(factory_context.runtime()), http_context_(factory_context.httpContext()), filter_enabled_(config.has_filter_enabled() ? absl::optional( @@ -162,6 +169,20 @@ class FilterConfig { bool headersAsBytes() const { return encode_raw_headers_; } + Filters::Common::MutationRules::CheckResult + checkDecoderHeaderMutation(const Filters::Common::MutationRules::CheckOperation& operation, + const Http::LowerCaseString& key, absl::string_view value) const { + if (!decoder_header_mutation_checker_.has_value()) { + return Filters::Common::MutationRules::CheckResult::OK; + } + return decoder_header_mutation_checker_->check(operation, key, value); + } + + // Used for headers_to_remove to avoid a redundant pseudo header check. + bool hasDecoderHeaderMutationRules() const { + return decoder_header_mutation_checker_.has_value(); + } + Http::Code statusOnError() const { return status_on_error_; } bool validateMutations() const { return validate_mutations_; } @@ -252,6 +273,7 @@ class FilterConfig { const Http::Code status_on_error_; const bool validate_mutations_; Stats::Scope& scope_; + const absl::optional decoder_header_mutation_checker_; Runtime::Loader& runtime_; Http::Context& http_context_; LabelsMap destination_labels_; @@ -372,6 +394,13 @@ class Filter : public Logger::Loggable, void onComplete(Filters::Common::ExtAuthz::ResponsePtr&&) override; private: + // Convenience function for the following: + // 1. If `validate_mutations` is set to true, validate header key and value. + // 2. If `decoder_header_mutation_rules` is set, check that mutation is allowed. + Filters::Common::MutationRules::CheckResult + validateAndCheckDecoderHeaderMutation(Filters::Common::MutationRules::CheckOperation operation, + absl::string_view key, absl::string_view value) const; + // Called when the filter is configured to reject invalid responses & the authz response contains // invalid header or query parameters. Sends a local response with the configured rejection status // code. diff --git a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc index 0495698266bf..a31789636957 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc @@ -176,6 +176,7 @@ class ExtAuthzGrpcIntegrationTest {"not-allowed", "nope"}, {"regex-food", "food"}, {"regex-fool", "fool"}, + {"disallow-mutation-downstream-req", "authz resp cannot set or append to this header"}, // If the below header exists in the downstream request, it is NOT copied in authz request. {Envoy::Extensions::Filters::Common::ExtAuthz::Headers::get().EnvoyAuthPartialBody.get(), "shouldn't be visible in authz request"}, @@ -330,6 +331,11 @@ class ExtAuthzGrpcIntegrationTest EXPECT_THAT(upstream_request_->headers(), Http::HeaderValueOf("x-envoy-auth-failure-mode-allowed", "true")); } + // Check that ext_authz didn't remove this downstream header which should be immune to + // mutations. + EXPECT_THAT(upstream_request_->headers(), + Http::HeaderValueOf("disallow-mutation-downstream-req", + "authz resp cannot set or append to this header")); for (const auto& header_to_add : opts.headers_to_add) { EXPECT_THAT(upstream_request_->headers(), @@ -616,6 +622,10 @@ class ExtAuthzGrpcIntegrationTest patterns: - prefix: allowed-prefix-denied + decoder_header_mutation_rules: + disallow_expression: + regex: ^disallow-mutation.* + with_request_body: max_request_bytes: 1024 allow_partial_message: true @@ -645,26 +655,27 @@ class ExtAuthzHttpIntegrationTest void initiateClientConnection() { auto conn = makeClientConnection(lookupPort("http")); codec_client_ = makeHttpConnection(std::move(conn)); - const auto headers = - Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/"}, - {":scheme", "http"}, - {":authority", "host"}, - {"x-case-sensitive-header", case_sensitive_header_value_}, - {"baz", "foo"}, - {"bat", "foo"}, - {"remove-me", "upstream-should-not-see-me"}, - {"x-duplicate", "one"}, - {"x-duplicate", "two"}, - {"x-duplicate", "three"}, - {"allowed-prefix-one", "one"}, - {"allowed-prefix-two", "two"}, - {"allowed-prefix-denied", "blah"}, - {"not-allowed", "nope"}, - {"authorization", "legit"}, - {"regex-food", "food"}, - {"regex-fool", "fool"}, - {"x-forwarded-for", "1.2.3.4"}}; + const auto headers = Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-case-sensitive-header", case_sensitive_header_value_}, + {"baz", "foo"}, + {"bat", "foo"}, + {"remove-me", "upstream-should-not-see-me"}, + {"x-duplicate", "one"}, + {"x-duplicate", "two"}, + {"x-duplicate", "three"}, + {"allowed-prefix-one", "one"}, + {"allowed-prefix-two", "two"}, + {"allowed-prefix-denied", "blah"}, + {"not-allowed", "nope"}, + {"authorization", "legit"}, + {"regex-food", "food"}, + {"regex-fool", "fool"}, + {"disallow-mutation-downstream-req", "authz resp cannot set or append to this header"}, + {"x-forwarded-for", "1.2.3.4"}}; if (client_request_body_.empty()) { response_ = codec_client_->makeHeaderOnlyRequest(headers); } else { @@ -681,32 +692,19 @@ class ExtAuthzHttpIntegrationTest result = ext_authz_request_->waitForEndStream(*dispatcher_); RELEASE_ASSERT(result, result.message()); - EXPECT_EQ("one", ext_authz_request_->headers() - .get(Http::LowerCaseString(std::string("allowed-prefix-one")))[0] - ->value() - .getStringView()); - EXPECT_EQ("two", ext_authz_request_->headers() - .get(Http::LowerCaseString(std::string("allowed-prefix-two")))[0] - ->value() - .getStringView()); - EXPECT_EQ("legit", ext_authz_request_->headers() - .get(Http::LowerCaseString(std::string("authorization")))[0] - ->value() - .getStringView()); + EXPECT_THAT(ext_authz_request_->headers(), Http::HeaderValueOf("allowed-prefix-one", "one")); + EXPECT_THAT(ext_authz_request_->headers(), Http::HeaderValueOf("allowed-prefix-two", "two")); + EXPECT_THAT(ext_authz_request_->headers(), Http::HeaderValueOf("authorization", "legit")); + EXPECT_THAT(ext_authz_request_->headers(), Http::HeaderValueOf("regex-food", "food")); + EXPECT_THAT(ext_authz_request_->headers(), Http::HeaderValueOf("regex-fool", "fool")); + EXPECT_TRUE(ext_authz_request_->headers() .get(Http::LowerCaseString(std::string("not-allowed"))) .empty()); EXPECT_TRUE(ext_authz_request_->headers() .get(Http::LowerCaseString(std::string("allowed-prefix-denied"))) .empty()); - EXPECT_EQ("food", ext_authz_request_->headers() - .get(Http::LowerCaseString(std::string("regex-food")))[0] - ->value() - .getStringView()); - EXPECT_EQ("fool", ext_authz_request_->headers() - .get(Http::LowerCaseString(std::string("regex-fool")))[0] - ->value() - .getStringView()); + if (encodeRawHeaders()) { // Duplicate headers should NOT be merged. const auto duplicate = @@ -740,9 +738,12 @@ class ExtAuthzHttpIntegrationTest {":status", "200"}, {"baz", "baz"}, {"bat", "bar"}, + {"authz-add-disallow-mutation", "this should not be allowed due to disallow_expression"}, {"x-append-bat", "append-foo"}, {"x-append-bat", "append-bar"}, {"x-envoy-auth-headers-to-remove", "remove-me"}, + // Try to remove this header that should not be able to be removed. + {"x-envoy-auth-headers-to-remove", "disallow-mutation-downstream-req"}, }; ext_authz_request_->encodeHeaders(response_headers, true); } @@ -831,6 +832,11 @@ class ExtAuthzHttpIntegrationTest EXPECT_TRUE(upstream_request_->headers() .get(Http::LowerCaseString{"x-envoy-auth-headers-to-remove"}) .empty()); + // The side stream tried to add this header that violates the disallow_expression header + // mutation rule. Make sure it did not get added. + EXPECT_TRUE(upstream_request_->headers() + .get(Http::LowerCaseString{"authz-add-disallow-mutation"}) + .empty()); upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); ASSERT_TRUE(response_->waitForEndStream()); @@ -847,10 +853,16 @@ class ExtAuthzHttpIntegrationTest std::string client_request_body_; const Http::LowerCaseString case_sensitive_header_name_{"x-case-sensitive-header"}; const std::string case_sensitive_header_value_{"Case-Sensitive"}; + // TODO: mutation rule const std::string legacy_default_config_ = R"EOF( disallowed_headers: patterns: - prefix: allowed-prefix-denied + + decoder_header_mutation_rules: + disallow_expression: + regex: disallow-mutation.* + http_service: server_uri: uri: "ext_authz:9000" @@ -889,6 +901,10 @@ class ExtAuthzHttpIntegrationTest patterns: - prefix: allowed-prefix-denied + decoder_header_mutation_rules: + disallow_expression: + regex: disallow-mutation.* + http_service: server_uri: uri: "ext_authz:9000" @@ -977,13 +993,20 @@ TEST_P(ExtAuthzGrpcIntegrationTest, CheckAfterBufferingComplete) { // Start a client connection and start request. Http::TestRequestHeaderMapImpl headers{ - {":method", "POST"}, {":path", "/test"}, - {":scheme", "http"}, {":authority", "host"}, - {"x-duplicate", "one"}, {"x-duplicate", "two"}, - {"x-duplicate", "three"}, {"allowed-prefix-one", "one"}, - {"allowed-prefix-two", "two"}, {"allowed-prefix-denied", "will not be sent"}, - {"not-allowed", "nope"}, {"regex-food", "food"}, - {"regex-fool", "fool"}}; + {":method", "POST"}, + {":path", "/test"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-duplicate", "one"}, + {"x-duplicate", "two"}, + {"x-duplicate", "three"}, + {"allowed-prefix-one", "one"}, + {"allowed-prefix-two", "two"}, + {"allowed-prefix-denied", "will not be sent"}, + {"not-allowed", "nope"}, + {"regex-food", "food"}, + {"regex-fool", "fool"}, + {"disallow-mutation-downstream-req", "authz resp cannot set or append to this header"}}; auto conn = makeClientConnection(lookupPort("http")); codec_client_ = makeHttpConnection(std::move(conn)); @@ -1156,13 +1179,21 @@ TEST_P(ExtAuthzGrpcIntegrationTest, FailureModeAllowNonUtf8) { invalid_unicode.append(1, char(0x28)); invalid_unicode.append("valid_suffix"); Http::TestRequestHeaderMapImpl headers{ - {":method", "POST"}, {":path", "/test"}, - {":scheme", "http"}, {":authority", "host"}, - {"x-bypass", invalid_unicode}, {"allowed-prefix-one", "one"}, - {"allowed-prefix-two", "two"}, {"allowed-prefix-denied", "denied"}, - {"not-allowed", "nope"}, {"x-duplicate", "one"}, - {"x-duplicate", "two"}, {"x-duplicate", "three"}, - {"regex-food", "food"}, {"regex-fool", "fool"}}; + {":method", "POST"}, + {":path", "/test"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-bypass", invalid_unicode}, + {"allowed-prefix-one", "one"}, + {"allowed-prefix-two", "two"}, + {"allowed-prefix-denied", "denied"}, + {"not-allowed", "nope"}, + {"x-duplicate", "one"}, + {"x-duplicate", "two"}, + {"x-duplicate", "three"}, + {"regex-food", "food"}, + {"regex-fool", "fool"}, + {"disallow-mutation-downstream-req", "authz resp cannot set or append to this header"}}; response_ = codec_client_->makeRequestWithBody(headers, {}); diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index 042d640bcd8c..53f2fdf33583 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -388,6 +388,329 @@ TEST_F(InvalidMutationTest, QueryParametersToSetValue) { testResponse(response); } +struct DecoderHeaderMutationRulesTestOpts { + absl::optional rules; + bool expect_reject_response = false; + Filters::Common::ExtAuthz::UnsafeHeaderVector allowed_headers_to_add; + Filters::Common::ExtAuthz::UnsafeHeaderVector disallowed_headers_to_add; + Filters::Common::ExtAuthz::UnsafeHeaderVector allowed_headers_to_append; + Filters::Common::ExtAuthz::UnsafeHeaderVector disallowed_headers_to_append; + Filters::Common::ExtAuthz::UnsafeHeaderVector allowed_headers_to_set; + Filters::Common::ExtAuthz::UnsafeHeaderVector disallowed_headers_to_set; + std::vector allowed_headers_to_remove; + std::vector disallowed_headers_to_remove; +}; +class DecoderHeaderMutationRulesTest + : public HttpFilterTestBase> { +public: + void runTest(DecoderHeaderMutationRulesTestOpts opts) { + InSequence s; + + envoy::extensions::filters::http::ext_authz::v3::ExtAuthz proto_config{}; + TestUtility::loadFromYaml(R"( + transport_api_version: V3 + grpc_service: + envoy_grpc: + cluster_name: "ext_authz_server" + failure_mode_allow: false + )", + proto_config); + if (opts.rules != std::nullopt) { + *(proto_config.mutable_decoder_header_mutation_rules()) = *opts.rules; + } + + initialize(proto_config); + + // Simulate a downstream request. + populateRequestHeadersFromOpts(opts); + + ON_CALL(decoder_filter_callbacks_, connection()) + .WillByDefault(Return(OptRef{connection_})); + connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_); + connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_); + EXPECT_CALL(*client_, check(_, _, _, _)) + .WillOnce(Invoke( + [&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, + const envoy::service::auth::v3::CheckRequest&, Tracing::Span&, + const StreamInfo::StreamInfo&) -> void { request_callbacks_ = &callbacks; })); + + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, + filter_->decodeHeaders(request_headers_, false)); + if (opts.expect_reject_response) { + EXPECT_CALL(decoder_filter_callbacks_, encodeHeaders_(_, true)) + .WillOnce(Invoke([&](const Http::ResponseHeaderMap& headers, bool) -> void { + EXPECT_EQ(headers.getStatusValue(), + std::to_string(enumToInt(Http::Code::InternalServerError))); + })); + } else { + EXPECT_CALL(decoder_filter_callbacks_, continueDecoding()); + } + + // Construct authz response from opts. + Filters::Common::ExtAuthz::Response response = getResponseFromOpts(opts); + + request_callbacks_->onComplete(std::make_unique(response)); + + if (!opts.expect_reject_response) { + // Now make sure the downstream header is / is not there depending on the test. + checkRequestHeadersFromOpts(opts); + } + } + + void populateRequestHeadersFromOpts(const DecoderHeaderMutationRulesTestOpts& opts) { + for (const auto& [key, _] : opts.allowed_headers_to_set) { + request_headers_.addCopy(Http::LowerCaseString(key), "will be overridden"); + } + for (const auto& [key, _] : opts.disallowed_headers_to_set) { + request_headers_.addCopy(Http::LowerCaseString(key), "will not be overridden"); + } + + for (const auto& [key, _] : opts.allowed_headers_to_append) { + request_headers_.addCopy(Http::LowerCaseString(key), "will be appended to"); + } + for (const auto& [key, _] : opts.disallowed_headers_to_append) { + request_headers_.addCopy(Http::LowerCaseString(key), "will not be appended to"); + } + + for (const auto& key : opts.allowed_headers_to_remove) { + request_headers_.addCopy(Http::LowerCaseString(key), "will be removed"); + } + for (const auto& key : opts.disallowed_headers_to_remove) { + request_headers_.addCopy(Http::LowerCaseString(key), "will not be removed"); + } + } + + void checkRequestHeadersFromOpts(const DecoderHeaderMutationRulesTestOpts& opts) { + for (const auto& [key, value] : opts.allowed_headers_to_add) { + EXPECT_EQ(request_headers_.get_(Http::LowerCaseString(key)), value) + << "(key: '" << key << "')"; + } + for (const auto& [key, _] : opts.disallowed_headers_to_add) { + EXPECT_FALSE(request_headers_.has(Http::LowerCaseString(key))) << "(key: '" << key << "')"; + } + + for (const auto& [key, value] : opts.allowed_headers_to_set) { + EXPECT_EQ(request_headers_.get_(Http::LowerCaseString(key)), value) + << "(key: '" << key << "')"; + } + for (const auto& [key, _] : opts.disallowed_headers_to_set) { + EXPECT_EQ(request_headers_.get_(Http::LowerCaseString(key)), "will not be overridden") + << "(key: '" << key << "')"; + } + + for (const auto& [key, value] : opts.allowed_headers_to_append) { + EXPECT_EQ(request_headers_.get_(Http::LowerCaseString(key)), + absl::StrCat("will be appended to,", value)) + << "(key: '" << key << "')"; + } + for (const auto& [key, value] : opts.disallowed_headers_to_append) { + EXPECT_EQ(request_headers_.get_(Http::LowerCaseString(key)), "will not be appended to") + << "(key: '" << key << "')"; + } + + for (const auto& key : opts.allowed_headers_to_remove) { + EXPECT_FALSE(request_headers_.has(Http::LowerCaseString(key))) << "(key: '" << key << "')"; + } + for (const auto& key : opts.disallowed_headers_to_remove) { + EXPECT_EQ(request_headers_.get_(Http::LowerCaseString(key)), "will not be removed") + << "(key: '" << key << "')"; + } + } + + static Filters::Common::ExtAuthz::Response + getResponseFromOpts(const DecoderHeaderMutationRulesTestOpts& opts) { + Filters::Common::ExtAuthz::Response response; + response.status = Filters::Common::ExtAuthz::CheckStatus::OK; + + for (const auto& vec : {opts.allowed_headers_to_add, opts.disallowed_headers_to_add}) { + for (const auto& [key, value] : vec) { + response.headers_to_add.emplace_back(key, value); + } + } + + for (const auto& vec : {opts.allowed_headers_to_set, opts.disallowed_headers_to_set}) { + for (const auto& [key, value] : vec) { + response.headers_to_set.emplace_back(key, value); + } + } + + for (const auto& vec : {opts.allowed_headers_to_append, opts.disallowed_headers_to_append}) { + for (const auto& [key, value] : vec) { + response.headers_to_append.emplace_back(key, value); + } + } + + for (const auto& vec : {opts.allowed_headers_to_remove, opts.disallowed_headers_to_remove}) { + for (const auto& key : vec) { + response.headers_to_remove.emplace_back(key); + } + } + + return response; + } +}; + +// If decoder_header_mutation_rules is empty, there should be no additional restrictions to +// ext_authz header mutations. +TEST_F(DecoderHeaderMutationRulesTest, EmptyConfig) { + DecoderHeaderMutationRulesTestOpts opts; + opts.allowed_headers_to_add = {{":authority", "google"}}; + opts.allowed_headers_to_append = {{"normal", "one"}, {":fake-pseudo-header", "append me"}}; + opts.allowed_headers_to_set = {{"x-envoy-whatever", "override"}}; + opts.allowed_headers_to_remove = {"delete-me"}; + // These are not allowed not because of the header mutation rules config, but because ext_authz + // doesn't allow the removable of headers starting with `:` anyway. + opts.disallowed_headers_to_remove = {":method", ":fake-pseudoheader"}; + runTest(opts); +} + +// Test behavior when the rules field exists but all sub-fields are their default values (should be +// exactly the same as above) +TEST_F(DecoderHeaderMutationRulesTest, ExplicitDefaultConfig) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.disallowed_headers_to_add = {{":authority", "google"}}; + opts.allowed_headers_to_append = {{"normal", "one"}}; + opts.disallowed_headers_to_append = {{":fake-pseudo-header", "append me"}}; + opts.disallowed_headers_to_set = {{"x-envoy-whatever", "override"}}; + opts.allowed_headers_to_remove = {"delete-me"}; + // These are not allowed not because of the header mutation rules config, but because ext_authz + // doesn't allow the removable of headers starting with `:` anyway. + opts.disallowed_headers_to_remove = {":method", ":fake-pseudoheader"}; + runTest(opts); +} + +TEST_F(DecoderHeaderMutationRulesTest, DisallowAll) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_all()->set_value(true); + + opts.disallowed_headers_to_add = {{"cant-add-me", "sad"}}; + opts.disallowed_headers_to_append = {{"cant-append-to-me", "fail"}}; + opts.disallowed_headers_to_set = {{"cant-override-me", "nope"}}; + opts.disallowed_headers_to_remove = {"cant-delete-me"}; + runTest(opts); +} + +TEST_F(DecoderHeaderMutationRulesTest, RejectResponseAdd) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_all()->set_value(true); + opts.rules->mutable_disallow_is_error()->set_value(true); + opts.expect_reject_response = true; + + opts.disallowed_headers_to_add = {{"cant-add-me", "sad"}}; + runTest(opts); +} + +TEST_F(DecoderHeaderMutationRulesTest, RejectResponseAppend) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_all()->set_value(true); + opts.rules->mutable_disallow_is_error()->set_value(true); + opts.expect_reject_response = true; + + opts.disallowed_headers_to_append = {{"cant-append-to-me", "fail"}}; + runTest(opts); +} + +TEST_F(DecoderHeaderMutationRulesTest, RejectResponseAppendPseudoheader) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_is_error()->set_value(true); + opts.expect_reject_response = true; + + opts.disallowed_headers_to_append = {{":fake-pseudo-header", "fail"}}; + runTest(opts); +} + +TEST_F(DecoderHeaderMutationRulesTest, RejectResponseSet) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_all()->set_value(true); + opts.rules->mutable_disallow_is_error()->set_value(true); + opts.expect_reject_response = true; + + opts.disallowed_headers_to_set = {{"cant-override-me", "nope"}}; + runTest(opts); +} + +TEST_F(DecoderHeaderMutationRulesTest, RejectResponseRemove) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_all()->set_value(true); + opts.rules->mutable_disallow_is_error()->set_value(true); + opts.expect_reject_response = true; + + opts.disallowed_headers_to_remove = {"cant-delete-me"}; + runTest(opts); +} + +TEST_F(DecoderHeaderMutationRulesTest, RejectResponseRemovePseudoHeader) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_is_error()->set_value(true); + opts.expect_reject_response = true; + + opts.disallowed_headers_to_remove = {":fake-pseudo-header"}; + runTest(opts); +} + +TEST_F(DecoderHeaderMutationRulesTest, DisallowExpression) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_expression()->set_regex("^x-example-.*"); + + opts.allowed_headers_to_add = {{"add-me-one", "one"}, {"add-me-two", "two"}}; + opts.disallowed_headers_to_add = {{"x-example-add", "nope"}}; + opts.allowed_headers_to_append = {{"append-to-me", "appended value"}}; + opts.disallowed_headers_to_append = {{"x-example-append", "no sir"}}; + opts.allowed_headers_to_set = {{"override-me", "new value"}}; + opts.disallowed_headers_to_set = {{"x-example-set", "no can do"}}; + opts.allowed_headers_to_remove = {"delete-me"}; + opts.disallowed_headers_to_remove = {"x-example-remove"}; + runTest(opts); +} + +// Tests that allow_expression overrides other settings (except disallow, which is tested +// elsewhere). +TEST_F(DecoderHeaderMutationRulesTest, AllowExpression) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_all()->set_value(true); + opts.rules->mutable_allow_expression()->set_regex("^x-allow-.*"); + + opts.allowed_headers_to_add = {{"x-allow-add-me-one", "one"}, {"x-allow-add-me-two", "two"}}; + opts.disallowed_headers_to_add = {{"not-allowed", "nope"}}; + opts.allowed_headers_to_append = {{"x-allow-append-to-me", "appended value"}}; + opts.disallowed_headers_to_append = {{"xx-allow-wrong-prefix", "no sir"}}; + opts.allowed_headers_to_set = {{"x-allow-override-me", "new value"}}; + opts.disallowed_headers_to_set = {{"cant-set-me", "no can do"}}; + opts.allowed_headers_to_remove = {"x-allow-delete-me"}; + opts.disallowed_headers_to_remove = {"cannot-remove"}; + runTest(opts); +} + +// Tests that disallow_expression overrides allow_expression. +TEST_F(DecoderHeaderMutationRulesTest, OverlappingAllowAndDisallowExpressions) { + DecoderHeaderMutationRulesTestOpts opts; + opts.rules = envoy::config::common::mutation_rules::v3::HeaderMutationRules(); + opts.rules->mutable_disallow_all()->set_value(true); + // Note the disallow expression's matches are a subset of the allow expression's matches. + opts.rules->mutable_allow_expression()->set_regex(".*allowed.*"); + opts.rules->mutable_disallow_expression()->set_regex(".*disallowed.*"); + + opts.allowed_headers_to_add = {{"allowed-add", "yes"}}; + opts.disallowed_headers_to_add = {{"disallowed-add", "nope"}}; + opts.allowed_headers_to_append = {{"allowed-append", "appended value"}}; + opts.disallowed_headers_to_append = {{"disallowed-append", "no sir"}}; + opts.allowed_headers_to_set = {{"allowed-set", "new value"}}; + opts.disallowed_headers_to_set = {{"disallowed-set", "no can do"}}; + opts.allowed_headers_to_remove = {"allowed-remove"}; + opts.disallowed_headers_to_remove = {"disallowed-remove"}; + runTest(opts); +} + // Test that the per route config is properly merged: more specific keys override previous keys. TEST_F(HttpFilterTest, MergeConfig) { envoy::extensions::filters::http::ext_authz::v3::ExtAuthzPerRoute settings; From 244feec4af4cceb5e331301f89dbbdeca6b84291 Mon Sep 17 00:00:00 2001 From: zirain Date: Fri, 7 Jun 2024 10:06:45 +0800 Subject: [PATCH 09/30] OpenTelemetry access logger support extension formatters (#34469) * api change Signed-off-by: zirain * implement Signed-off-by: zirain * format Signed-off-by: zirain * fix test Signed-off-by: zirain * use Formatter::CommandParserPtr Signed-off-by: zirain * changelogs Signed-off-by: zirain * format Signed-off-by: zirain --------- Signed-off-by: zirain --- .../access_loggers/open_telemetry/v3/BUILD | 1 + .../open_telemetry/v3/logs_service.proto | 8 +- changelogs/current.yaml | 5 ++ .../open_telemetry/access_log_impl.cc | 8 +- .../open_telemetry/access_log_impl.h | 3 +- .../access_loggers/open_telemetry/config.cc | 10 ++- .../open_telemetry/substitution_formatter.cc | 7 +- .../open_telemetry/substitution_formatter.h | 8 +- .../access_loggers/open_telemetry/BUILD | 22 +++++- .../open_telemetry/access_log_impl_test.cc | 9 ++- .../substitution_formatter_speed_test.cc | 3 +- .../substitution_formatter_test.cc | 74 ++++++++++++++----- 12 files changed, 125 insertions(+), 33 deletions(-) diff --git a/api/envoy/extensions/access_loggers/open_telemetry/v3/BUILD b/api/envoy/extensions/access_loggers/open_telemetry/v3/BUILD index 95eff6986b7f..dad7cd1aef35 100644 --- a/api/envoy/extensions/access_loggers/open_telemetry/v3/BUILD +++ b/api/envoy/extensions/access_loggers/open_telemetry/v3/BUILD @@ -6,6 +6,7 @@ licenses(["notice"]) # Apache 2 api_proto_package( deps = [ + "//envoy/config/core/v3:pkg", "//envoy/extensions/access_loggers/grpc/v3:pkg", "@com_github_cncf_xds//udpa/annotations:pkg", "@opentelemetry_proto//:common", diff --git a/api/envoy/extensions/access_loggers/open_telemetry/v3/logs_service.proto b/api/envoy/extensions/access_loggers/open_telemetry/v3/logs_service.proto index 0e4e89cdb6ac..641276a64bd6 100644 --- a/api/envoy/extensions/access_loggers/open_telemetry/v3/logs_service.proto +++ b/api/envoy/extensions/access_loggers/open_telemetry/v3/logs_service.proto @@ -2,6 +2,7 @@ syntax = "proto3"; package envoy.extensions.access_loggers.open_telemetry.v3; +import "envoy/config/core/v3/extension.proto"; import "envoy/extensions/access_loggers/grpc/v3/als.proto"; import "opentelemetry/proto/common/v1/common.proto"; @@ -22,7 +23,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // populate `opentelemetry.proto.collector.v1.logs.ExportLogsServiceRequest.resource_logs `_. // In addition, the request start time is set in the dedicated field. // [#extension: envoy.access_loggers.open_telemetry] -// [#next-free-field: 7] +// [#next-free-field: 8] message OpenTelemetryAccessLogConfig { // [#comment:TODO(itamarkam): add 'filter_state_objects_to_log' to logs.] grpc.v3.CommonGrpcAccessLogConfig common_config = 1 [(validate.rules).message = {required: true}]; @@ -51,4 +52,9 @@ message OpenTelemetryAccessLogConfig { // ``access_logs.open_telemetry_access_log.``. If non-empty, stats will be rooted at // ``access_logs.open_telemetry_access_log..``. string stat_prefix = 6; + + // Specifies a collection of Formatter plugins that can be called from the access log configuration. + // See the formatters extensions documentation for details. + // [#extension-category: envoy.formatter] + repeated config.core.v3.TypedExtensionConfig formatters = 7; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 0f340c48d22c..6da420210a7e 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -254,6 +254,11 @@ new_features: added :ref:`stat_prefix ` configuration to support additional stat prefix for the OpenTelemetry logger. +- area: open_telemetry + change: | + added :ref:`formatters + ` + configuration to support extension formatter for the OpenTelemetry logger. - area: routing change: | added support in :ref:`file datasource ` implementation diff --git a/source/extensions/access_loggers/open_telemetry/access_log_impl.cc b/source/extensions/access_loggers/open_telemetry/access_log_impl.cc index b630c9c0cf53..f86fda60b50b 100644 --- a/source/extensions/access_loggers/open_telemetry/access_log_impl.cc +++ b/source/extensions/access_loggers/open_telemetry/access_log_impl.cc @@ -9,6 +9,7 @@ #include "source/common/common/assert.h" #include "source/common/config/utility.h" +#include "source/common/formatter/substitution_formatter.h" #include "source/common/http/headers.h" #include "source/common/network/utility.h" #include "source/common/protobuf/message_validator_impl.h" @@ -58,7 +59,8 @@ AccessLog::ThreadLocalLogger::ThreadLocalLogger(GrpcAccessLoggerSharedPtr logger AccessLog::AccessLog( ::Envoy::AccessLog::FilterPtr&& filter, envoy::extensions::access_loggers::open_telemetry::v3::OpenTelemetryAccessLogConfig config, - ThreadLocal::SlotAllocator& tls, GrpcAccessLoggerCacheSharedPtr access_logger_cache) + ThreadLocal::SlotAllocator& tls, GrpcAccessLoggerCacheSharedPtr access_logger_cache, + const std::vector& commands) : Common::ImplBase(std::move(filter)), tls_slot_(tls.allocateSlot()), access_logger_cache_(std::move(access_logger_cache)) { @@ -71,9 +73,9 @@ AccessLog::AccessLog( // Packing the body "AnyValue" to a "KeyValueList" only if it's not empty, otherwise the // formatter would fail to parse it. if (config.body().value_case() != ::opentelemetry::proto::common::v1::AnyValue::VALUE_NOT_SET) { - body_formatter_ = std::make_unique(packBody(config.body())); + body_formatter_ = std::make_unique(packBody(config.body()), commands); } - attributes_formatter_ = std::make_unique(config.attributes()); + attributes_formatter_ = std::make_unique(config.attributes(), commands); } void AccessLog::emitLog(const Formatter::HttpFormatterContext& log_context, diff --git a/source/extensions/access_loggers/open_telemetry/access_log_impl.h b/source/extensions/access_loggers/open_telemetry/access_log_impl.h index 7d1013488eb4..308ec8ac1bdb 100644 --- a/source/extensions/access_loggers/open_telemetry/access_log_impl.h +++ b/source/extensions/access_loggers/open_telemetry/access_log_impl.h @@ -36,7 +36,8 @@ class AccessLog : public Common::ImplBase { AccessLog( ::Envoy::AccessLog::FilterPtr&& filter, envoy::extensions::access_loggers::open_telemetry::v3::OpenTelemetryAccessLogConfig config, - ThreadLocal::SlotAllocator& tls, GrpcAccessLoggerCacheSharedPtr access_logger_cache); + ThreadLocal::SlotAllocator& tls, GrpcAccessLoggerCacheSharedPtr access_logger_cache, + const std::vector& commands); private: /** diff --git a/source/extensions/access_loggers/open_telemetry/config.cc b/source/extensions/access_loggers/open_telemetry/config.cc index c371b5b724a4..237c5c9fa839 100644 --- a/source/extensions/access_loggers/open_telemetry/config.cc +++ b/source/extensions/access_loggers/open_telemetry/config.cc @@ -7,6 +7,7 @@ #include "source/common/common/assert.h" #include "source/common/common/macros.h" +#include "source/common/formatter/substitution_format_string.h" #include "source/common/grpc/async_client_impl.h" #include "source/common/protobuf/protobuf.h" #include "source/extensions/access_loggers/open_telemetry/access_log_impl.h" @@ -40,9 +41,12 @@ AccessLogFactory::createAccessLogInstance(const Protobuf::Message& config, const envoy::extensions::access_loggers::open_telemetry::v3::OpenTelemetryAccessLogConfig&>( config, context.messageValidationVisitor()); - return std::make_shared(std::move(filter), proto_config, - context.serverFactoryContext().threadLocal(), - getAccessLoggerCacheSingleton(context.serverFactoryContext())); + auto commands = + Formatter::SubstitutionFormatStringUtils::parseFormatters(proto_config.formatters(), context); + + return std::make_shared( + std::move(filter), proto_config, context.serverFactoryContext().threadLocal(), + getAccessLoggerCacheSingleton(context.serverFactoryContext()), commands); } ProtobufTypes::MessagePtr AccessLogFactory::createEmptyConfigProto() { diff --git a/source/extensions/access_loggers/open_telemetry/substitution_formatter.cc b/source/extensions/access_loggers/open_telemetry/substitution_formatter.cc index e48522bc54ef..a99b80878dd2 100644 --- a/source/extensions/access_loggers/open_telemetry/substitution_formatter.cc +++ b/source/extensions/access_loggers/open_telemetry/substitution_formatter.cc @@ -21,8 +21,9 @@ namespace AccessLoggers { namespace OpenTelemetry { OpenTelemetryFormatter::OpenTelemetryFormatter( - const ::opentelemetry::proto::common::v1::KeyValueList& format_mapping) - : kv_list_output_format_(FormatBuilder().toFormatMapValue(format_mapping)) {} + const ::opentelemetry::proto::common::v1::KeyValueList& format_mapping, + const std::vector& commands) + : kv_list_output_format_(FormatBuilder(commands).toFormatMapValue(format_mapping)) {} OpenTelemetryFormatter::OpenTelemetryFormatMapWrapper OpenTelemetryFormatter::FormatBuilder::toFormatMapValue( @@ -77,7 +78,7 @@ OpenTelemetryFormatter::FormatBuilder::toFormatListValue( std::vector OpenTelemetryFormatter::FormatBuilder::toFormatStringValue(const std::string& string_format) const { - return Formatter::SubstitutionFormatParser::parse(string_format, {}); + return Formatter::SubstitutionFormatParser::parse(string_format, commands_); } ::opentelemetry::proto::common::v1::AnyValue OpenTelemetryFormatter::providersCallback( diff --git a/source/extensions/access_loggers/open_telemetry/substitution_formatter.h b/source/extensions/access_loggers/open_telemetry/substitution_formatter.h index ea797fff13e2..bc97fe91d5b9 100644 --- a/source/extensions/access_loggers/open_telemetry/substitution_formatter.h +++ b/source/extensions/access_loggers/open_telemetry/substitution_formatter.h @@ -26,7 +26,8 @@ OpenTelemetryFormatMapVisitorHelper(Ts...) -> OpenTelemetryFormatMapVisitorHelpe */ class OpenTelemetryFormatter { public: - OpenTelemetryFormatter(const ::opentelemetry::proto::common::v1::KeyValueList& format_mapping); + OpenTelemetryFormatter(const ::opentelemetry::proto::common::v1::KeyValueList& format_mapping, + const std::vector& commands); ::opentelemetry::proto::common::v1::KeyValueList format(const Formatter::HttpFormatterContext& context, const StreamInfo::StreamInfo& info) const; @@ -62,12 +63,17 @@ class OpenTelemetryFormatter { // Methods for building the format map. class FormatBuilder { public: + explicit FormatBuilder(const std::vector& commands) + : commands_(commands) {} std::vector toFormatStringValue(const std::string& string_format) const; OpenTelemetryFormatMapWrapper toFormatMapValue(const ::opentelemetry::proto::common::v1::KeyValueList& struct_format) const; OpenTelemetryFormatListWrapper toFormatListValue( const ::opentelemetry::proto::common::v1::ArrayValue& list_value_format) const; + + private: + const std::vector& commands_; }; // Methods for doing the actual formatting. diff --git a/test/extensions/access_loggers/open_telemetry/BUILD b/test/extensions/access_loggers/open_telemetry/BUILD index 659be55579cf..9764ee5ecd18 100644 --- a/test/extensions/access_loggers/open_telemetry/BUILD +++ b/test/extensions/access_loggers/open_telemetry/BUILD @@ -43,9 +43,11 @@ envoy_extension_cc_test( "//test/mocks/access_log:access_log_mocks", "//test/mocks/grpc:grpc_mocks", "//test/mocks/local_info:local_info_mocks", + "//test/mocks/server:server_mocks", "//test/mocks/ssl:ssl_mocks", "//test/mocks/stream_info:stream_info_mocks", "//test/mocks/thread_local:thread_local_mocks", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/data/accesslog/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/access_loggers/grpc/v3:pkg_cc_proto", "@opentelemetry_proto//:logs_cc_proto", @@ -89,18 +91,36 @@ envoy_extension_cc_test( envoy_extension_cc_test( name = "substitution_formatter_test", srcs = ["substitution_formatter_test.cc"], + copts = select({ + "//bazel:windows_x86_64": [], # TODO: fix the windows ANTLR build + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), extension_names = ["envoy.access_loggers.open_telemetry"], + tags = ["skip_on_windows"], deps = [ "//source/common/formatter:substitution_formatter_lib", "//source/common/http:exception_lib", "//source/common/http:header_map_lib", "//source/common/router:string_accessor_lib", + "//source/extensions/access_loggers/open_telemetry:config", "//source/extensions/access_loggers/open_telemetry:substitution_formatter_lib", + "//source/extensions/formatter/cel:cel_lib", + "//source/extensions/formatter/cel:config", + "//test/mocks/server:factory_context_mocks", "//test/mocks/stream_info:stream_info_mocks", "//test/mocks/upstream:cluster_info_mocks", "//test/test_common:utility_lib", "@opentelemetry_proto//:logs_cc_proto", - ], + ] + select( + { + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "@com_google_cel_cpp//parser", + ], + }, + ), ) envoy_cc_benchmark_binary( diff --git a/test/extensions/access_loggers/open_telemetry/access_log_impl_test.cc b/test/extensions/access_loggers/open_telemetry/access_log_impl_test.cc index 14c19f5bd185..532d2b288eef 100644 --- a/test/extensions/access_loggers/open_telemetry/access_log_impl_test.cc +++ b/test/extensions/access_loggers/open_telemetry/access_log_impl_test.cc @@ -2,10 +2,12 @@ #include #include "envoy/common/time.h" +#include "envoy/config/core/v3/extension.pb.h" #include "envoy/data/accesslog/v3/accesslog.pb.h" #include "envoy/extensions/access_loggers/grpc/v3/als.pb.h" #include "source/common/buffer/zero_copy_input_stream_impl.h" +#include "source/common/formatter/substitution_format_string.h" #include "source/common/network/address_impl.h" #include "source/common/protobuf/protobuf.h" #include "source/common/router/string_accessor_impl.h" @@ -15,6 +17,7 @@ #include "test/mocks/common.h" #include "test/mocks/grpc/mocks.h" #include "test/mocks/local_info/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/mocks/ssl/mocks.h" #include "test/mocks/stream_info/mocks.h" #include "test/mocks/thread_local/mocks.h" @@ -80,7 +83,10 @@ class AccessLogTest : public testing::Test { EXPECT_EQ(Common::GrpcAccessLoggerType::HTTP, logger_type); return logger_; }); - return std::make_unique(FilterPtr{filter_}, config_, tls_, logger_cache_); + auto commands = + Formatter::SubstitutionFormatStringUtils::parseFormatters(config_.formatters(), context_); + + return std::make_unique(FilterPtr{filter_}, config_, tls_, logger_cache_, commands); } void expectLog(const std::string& expected_log_entry_yaml) { @@ -94,6 +100,7 @@ class AccessLogTest : public testing::Test { MockFilter* filter_{new NiceMock()}; NiceMock tls_; + NiceMock context_; envoy::extensions::access_loggers::open_telemetry::v3::OpenTelemetryAccessLogConfig config_; std::shared_ptr logger_{new MockGrpcAccessLogger()}; std::shared_ptr logger_cache_{new MockGrpcAccessLoggerCache()}; diff --git a/test/extensions/access_loggers/open_telemetry/substitution_formatter_speed_test.cc b/test/extensions/access_loggers/open_telemetry/substitution_formatter_speed_test.cc index 11557702df2b..31975e697e2b 100644 --- a/test/extensions/access_loggers/open_telemetry/substitution_formatter_speed_test.cc +++ b/test/extensions/access_loggers/open_telemetry/substitution_formatter_speed_test.cc @@ -53,7 +53,8 @@ std::unique_ptr makeOpenTelemetryFormatter() { string_value: '%REQ(USER-AGENT)%' )EOF"; TestUtility::loadFromYaml(format_yaml, otel_log_format); - return std::make_unique(otel_log_format); + std::vector commands = {}; + return std::make_unique(otel_log_format, commands); } std::unique_ptr makeStreamInfo() { diff --git a/test/extensions/access_loggers/open_telemetry/substitution_formatter_test.cc b/test/extensions/access_loggers/open_telemetry/substitution_formatter_test.cc index 2adedb958afc..54fe710a47f3 100644 --- a/test/extensions/access_loggers/open_telemetry/substitution_formatter_test.cc +++ b/test/extensions/access_loggers/open_telemetry/substitution_formatter_test.cc @@ -5,11 +5,14 @@ #include "envoy/common/exception.h" #include "envoy/stream_info/stream_info.h" +#include "source/common/formatter/substitution_format_string.h" #include "source/common/formatter/substitution_formatter.h" #include "source/common/http/header_map_impl.h" #include "source/common/router/string_accessor_impl.h" +#include "source/extensions/access_loggers/open_telemetry/grpc_access_log_impl.h" #include "source/extensions/access_loggers/open_telemetry/substitution_formatter.h" +#include "test/mocks/server/factory_context.h" #include "test/mocks/stream_info/mocks.h" #include "test/mocks/upstream/cluster_info.h" #include "test/test_common/utility.h" @@ -127,7 +130,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterPlainStringTest) { string_value: "plain_string_value" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput(formatter.format({}, stream_info), expected); } @@ -162,7 +165,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterTypesTest) { - string_value: "%PROTOCOL%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); KeyValueList expected; TestUtility::loadFromYaml(R"EOF( @@ -298,7 +301,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterNestedObjectsTest) { - string_value: "%PROTOCOL%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); KeyValueList expected; TestUtility::loadFromYaml(R"EOF( values: @@ -416,7 +419,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterSingleOperatorTest) { string_value: "%PROTOCOL%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput(formatter.format({}, stream_info), expected); } @@ -437,7 +440,7 @@ TEST(SubstitutionFormatterTest, EmptyOpenTelemetryFormatterTest) { string_value: "" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput(formatter.format({}, stream_info), expected); } @@ -469,7 +472,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterNonExistentHeaderTest) { string_value: "%RESP(some_response_header)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); @@ -508,7 +511,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterAlternateHeaderTest) { string_value: "%RESP(response_present_header?response_absent_header)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); @@ -546,7 +549,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterDynamicMetadataTest) { string_value: "%DYNAMIC_METADATA(com.test:test_obj:inner_key)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput( formatter.format({&request_header, &response_header, &response_trailer}, stream_info), @@ -591,7 +594,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterClusterMetadataTest) { string_value: "%CLUSTER_METADATA(com.test:test_obj:non_existing_key)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput( formatter.format({&request_header, &response_header, &response_trailer}, stream_info), @@ -614,7 +617,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterClusterMetadataNoClusterIn string_value: "%CLUSTER_METADATA(com.test:test_key)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); // Empty optional (absl::nullopt) { @@ -656,7 +659,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterFilterStateTest) { string_value: "%FILTER_STATE(test_obj)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput(formatter.format({}, stream_info), expected); } @@ -696,7 +699,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterUpstreamFilterStateTest) { string_value: "%UPSTREAM_FILTER_STATE(test_obj)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput(formatter.format({}, stream_info), expected); } @@ -726,7 +729,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterFilterStateSpeciferTest) { string_value: "%FILTER_STATE(test_key:TYPED)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput(formatter.format({}, stream_info), expected); } @@ -762,7 +765,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterUpstreamFilterStateSpecife string_value: "%UPSTREAM_FILTER_STATE(test_key:TYPED)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput(formatter.format({}, stream_info), expected); } @@ -787,7 +790,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterFilterStateErrorSpeciferTe string_value: "%FILTER_STATE(test_key:TYPED)%" )EOF", key_mapping); - EXPECT_THROW_WITH_MESSAGE(OpenTelemetryFormatter formatter(key_mapping), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(OpenTelemetryFormatter formatter(key_mapping, {}), EnvoyException, "Invalid filter state serialize type, only support PLAIN/TYPED/FIELD."); } @@ -818,7 +821,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterUpstreamFilterStateErrorSp string_value: "%UPSTREAM_FILTER_STATE(test_key:TYPED)%" )EOF", key_mapping); - EXPECT_THROW_WITH_MESSAGE(OpenTelemetryFormatter formatter(key_mapping), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(OpenTelemetryFormatter formatter(key_mapping, {}), EnvoyException, "Invalid filter state serialize type, only support PLAIN/TYPED/FIELD."); } @@ -855,7 +858,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterStartTimeTest) { string_value: "%START_TIME(%f.%1f.%2f.%3f)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); verifyOpenTelemetryOutput(formatter.format({}, stream_info), expected); } @@ -878,7 +881,7 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterMultiTokenTest) { string_value: "%PROTOCOL% plainstring %REQ(some_request_header)% %RESP(some_response_header)%" )EOF", key_mapping); - OpenTelemetryFormatter formatter(key_mapping); + OpenTelemetryFormatter formatter(key_mapping, {}); absl::optional protocol = Http::Protocol::Http11; EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol)); @@ -888,6 +891,41 @@ TEST(SubstitutionFormatterTest, OpenTelemetryFormatterMultiTokenTest) { } } +#ifdef USE_CEL_PARSER +TEST(SubstitutionFormatterTest, CELFormatterTest) { + { + NiceMock context; + StreamInfo::MockStreamInfo stream_info; + Http::TestRequestHeaderMapImpl request_header{{"some_request_header", "SOME_REQUEST_HEADER"}}; + Http::TestResponseHeaderMapImpl response_header{ + {"some_response_header", "SOME_RESPONSE_HEADER"}}; + + OpenTelemetryFormatMap expected = {{"cel_field", "SOME_REQUEST_HEADER SOME_RESPONSE_HEADER"}}; + + envoy::extensions::access_loggers::open_telemetry::v3::OpenTelemetryAccessLogConfig otel_config; + TestUtility::loadFromYaml(R"EOF( + resource_attributes: + values: + - key: "cel_field" + value: + string_value: "%CEL(request.headers['some_request_header'])% %CEL(response.headers['some_response_header'])%" + formatters: + - name: envoy.formatter.cel + typed_config: + "@type": type.googleapis.com/envoy.extensions.formatter.cel.v3.Cel + )EOF", + otel_config); + auto commands = Formatter::SubstitutionFormatStringUtils::parseFormatters( + otel_config.formatters(), context); + + OpenTelemetryFormatter formatter(otel_config.resource_attributes(), commands); + + verifyOpenTelemetryOutput(formatter.format({&request_header, &response_header}, stream_info), + expected); + } +} +#endif + } // namespace } // namespace OpenTelemetry } // namespace AccessLoggers From f039f4d305916a7ac10a9ab83503ae51554b2ca9 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:20:00 +0100 Subject: [PATCH 10/30] build(deps): bump elasticsearch from 8.13.4 to 8.14.0 in /examples/skywalking (#34576) build(deps): bump elasticsearch in /examples/skywalking Bumps elasticsearch from 8.13.4 to 8.14.0. --- updated-dependencies: - dependency-name: elasticsearch dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- examples/skywalking/Dockerfile-elasticsearch | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/skywalking/Dockerfile-elasticsearch b/examples/skywalking/Dockerfile-elasticsearch index 3942fa1cf21f..4c2b5e562218 100644 --- a/examples/skywalking/Dockerfile-elasticsearch +++ b/examples/skywalking/Dockerfile-elasticsearch @@ -1 +1 @@ -FROM elasticsearch:8.13.4@sha256:11e1717dd78f46fbecf64f7f27a358d331abb5e95b5451c00730243ee44fb665 +FROM elasticsearch:8.14.0@sha256:dfd92a2938094bdd0fc4203796d7ad3426b72b19b4a29d8b26bb032510c5bed6 From 62645ec64cba62aabd2206f1e581a535cb051f7f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:20:10 +0100 Subject: [PATCH 11/30] build(deps): bump otel/opentelemetry-collector from `1cffbc3` to `8473e4b` in /examples/opentelemetry (#34575) build(deps): bump otel/opentelemetry-collector Bumps otel/opentelemetry-collector from `1cffbc3` to `8473e4b`. --- updated-dependencies: - dependency-name: otel/opentelemetry-collector dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- examples/opentelemetry/Dockerfile-opentelemetry | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/opentelemetry/Dockerfile-opentelemetry b/examples/opentelemetry/Dockerfile-opentelemetry index e27962f834f3..9e7f6e5ecced 100644 --- a/examples/opentelemetry/Dockerfile-opentelemetry +++ b/examples/opentelemetry/Dockerfile-opentelemetry @@ -1,7 +1,7 @@ FROM alpine:3.20@sha256:77726ef6b57ddf65bb551896826ec38bc3e53f75cdde31354fbffb4f25238ebd as otelc_curl RUN apk --update add curl -FROM otel/opentelemetry-collector:latest@sha256:1cffbc33556826c5185cebc1b1dbe1e056b3417bd422cdd4a03747dc6d19388f +FROM otel/opentelemetry-collector:latest@sha256:8473e4b0e81ecc8aa238b5e10a53659ce0e6559d5159d77f8f01f3ecbb1f3391 COPY --from=otelc_curl / / From 54266a082654f21b409f9d0af53a009bdd31b9de Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:20:19 +0100 Subject: [PATCH 12/30] build(deps): bump actions/dependency-review-action from 4.3.2 to 4.3.3 (#34574) Bumps [actions/dependency-review-action](https://github.com/actions/dependency-review-action) from 4.3.2 to 4.3.3. - [Release notes](https://github.com/actions/dependency-review-action/releases) - [Commits](https://github.com/actions/dependency-review-action/compare/0c155c5e8556a497adf53f2c18edabf945ed8e70...72eb03d02c7872a771aacd928f3123ac62ad6d3a) --- updated-dependencies: - dependency-name: actions/dependency-review-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/_precheck_deps.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/_precheck_deps.yml b/.github/workflows/_precheck_deps.yml index 17facaf412d0..14ccb10ff127 100644 --- a/.github/workflows/_precheck_deps.yml +++ b/.github/workflows/_precheck_deps.yml @@ -55,4 +55,4 @@ jobs: ref: ${{ fromJSON(inputs.request).request.sha }} persist-credentials: false - name: Dependency Review - uses: actions/dependency-review-action@0c155c5e8556a497adf53f2c18edabf945ed8e70 # v4.3.2 + uses: actions/dependency-review-action@72eb03d02c7872a771aacd928f3123ac62ad6d3a # v4.3.3 From 328ea27c978f902502b1cf4e7ca75e4a81e113c5 Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Fri, 7 Jun 2024 13:29:14 -0500 Subject: [PATCH 13/30] mobile: Remove sandboxNetwork=standard (#34606) sandboxNetwork=standard is no longer needed. Risk Level: low (tests only) Testing: CI Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Fredy Wijaya --- mobile/test/common/integration/BUILD | 16 ---------------- mobile/test/java/integration/BUILD | 16 ---------------- .../envoyproxy/envoymobile/engine/testing/BUILD | 4 ---- mobile/test/java/org/chromium/net/BUILD | 16 ---------------- mobile/test/java/org/chromium/net/impl/BUILD | 4 ---- mobile/test/java/org/chromium/net/testing/BUILD | 8 -------- .../java/org/chromium/net/urlconnection/BUILD | 4 ---- mobile/test/kotlin/integration/BUILD | 2 -- mobile/test/kotlin/integration/proxying/BUILD | 12 ------------ 9 files changed, 82 deletions(-) diff --git a/mobile/test/common/integration/BUILD b/mobile/test/common/integration/BUILD index 9ee2a0c7cbea..e7d938ce874e 100644 --- a/mobile/test/common/integration/BUILD +++ b/mobile/test/common/integration/BUILD @@ -14,10 +14,6 @@ envoy_mobile_package() envoy_cc_test( name = "client_integration_test", srcs = ["client_integration_test.cc"], - exec_properties = { - # TODO(willengflow): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, linkopts = select({ "@envoy//bazel:apple": [ # For the TestProxyResolutionApi test. @@ -52,10 +48,6 @@ envoy_cc_test( data = [ "@envoy//test/config/integration/certs", ], - exec_properties = { - # TODO(willengflow): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, external_deps = [ "abseil_strings", ], @@ -79,10 +71,6 @@ envoy_cc_test( data = [ "@envoy//test/config/integration/certs", ], - exec_properties = { - # TODO(willengflow): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, external_deps = [ "abseil_strings", ], @@ -105,10 +93,6 @@ envoy_cc_test( data = [ "@envoy//test/config/integration/certs", ], - exec_properties = { - # TODO(willengflow): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, repository = "@envoy", deps = [ ":xds_integration_test_lib", diff --git a/mobile/test/java/integration/BUILD b/mobile/test/java/integration/BUILD index a45c135d28a9..a08f3616130a 100644 --- a/mobile/test/java/integration/BUILD +++ b/mobile/test/java/integration/BUILD @@ -10,10 +10,6 @@ envoy_mobile_android_test( srcs = [ "AndroidEngineStartUpTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ @@ -33,10 +29,6 @@ envoy_mobile_android_test( srcs = [ "AndroidEngineFlowTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ @@ -57,10 +49,6 @@ envoy_mobile_android_test( srcs = [ "AndroidEngineExplicitFlowTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ @@ -81,10 +69,6 @@ envoy_mobile_android_test( srcs = [ "AndroidEngineSocketTagTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ diff --git a/mobile/test/java/io/envoyproxy/envoymobile/engine/testing/BUILD b/mobile/test/java/io/envoyproxy/envoymobile/engine/testing/BUILD index 367f4261dc73..fb431ad3148f 100644 --- a/mobile/test/java/io/envoyproxy/envoymobile/engine/testing/BUILD +++ b/mobile/test/java/io/envoyproxy/envoymobile/engine/testing/BUILD @@ -74,10 +74,6 @@ envoy_mobile_android_test( srcs = [ "QuicTestServerTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ diff --git a/mobile/test/java/org/chromium/net/BUILD b/mobile/test/java/org/chromium/net/BUILD index f22d692ee86c..8e615ab387b2 100644 --- a/mobile/test/java/org/chromium/net/BUILD +++ b/mobile/test/java/org/chromium/net/BUILD @@ -17,10 +17,6 @@ envoy_mobile_android_test( "UploadDataProvidersTest.java", "UrlResponseInfoTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ @@ -47,10 +43,6 @@ envoy_mobile_android_test( srcs = [ "CronetUrlRequestContextTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ @@ -76,10 +68,6 @@ envoy_mobile_android_test( srcs = [ "CronetUrlRequestTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ @@ -105,10 +93,6 @@ envoy_mobile_android_test( srcs = [ "BidirectionalStreamTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ diff --git a/mobile/test/java/org/chromium/net/impl/BUILD b/mobile/test/java/org/chromium/net/impl/BUILD index 9605525ebffd..3c34f8e7c9f3 100644 --- a/mobile/test/java/org/chromium/net/impl/BUILD +++ b/mobile/test/java/org/chromium/net/impl/BUILD @@ -16,10 +16,6 @@ envoy_mobile_android_test( "ErrorsTest.java", "UrlRequestCallbackTester.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ diff --git a/mobile/test/java/org/chromium/net/testing/BUILD b/mobile/test/java/org/chromium/net/testing/BUILD index ff20d29a8295..96b8b41c1724 100644 --- a/mobile/test/java/org/chromium/net/testing/BUILD +++ b/mobile/test/java/org/chromium/net/testing/BUILD @@ -84,10 +84,6 @@ envoy_mobile_android_test( srcs = [ "Http2TestServerTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ @@ -108,10 +104,6 @@ envoy_mobile_android_test( srcs = [ "AndroidEnvoyExplicitH2FlowTest.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ diff --git a/mobile/test/java/org/chromium/net/urlconnection/BUILD b/mobile/test/java/org/chromium/net/urlconnection/BUILD index 2872d85568cd..8505f860f917 100644 --- a/mobile/test/java/org/chromium/net/urlconnection/BUILD +++ b/mobile/test/java/org/chromium/net/urlconnection/BUILD @@ -17,10 +17,6 @@ envoy_mobile_android_test( "MessageLoopTest.java", "TestUtil.java", ], - exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", - }, native_deps = [ "//test/jni:libenvoy_jni_with_test_extensions.so", ] + select({ diff --git a/mobile/test/kotlin/integration/BUILD b/mobile/test/kotlin/integration/BUILD index a648eb74c43e..2c376e6322aa 100644 --- a/mobile/test/kotlin/integration/BUILD +++ b/mobile/test/kotlin/integration/BUILD @@ -332,8 +332,6 @@ envoy_mobile_android_test( "FilterThrowingExceptionTest.kt", ], exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", "dockerNetwork": "standard", }, native_deps = [ diff --git a/mobile/test/kotlin/integration/proxying/BUILD b/mobile/test/kotlin/integration/proxying/BUILD index ef4c24c53327..0f8f5864cfa1 100644 --- a/mobile/test/kotlin/integration/proxying/BUILD +++ b/mobile/test/kotlin/integration/proxying/BUILD @@ -11,8 +11,6 @@ envoy_mobile_android_test( "ProxyInfoIntentPerformHTTPRequestUsingProxyTest.kt", ], exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", "dockerNetwork": "standard", }, native_deps = [ @@ -38,8 +36,6 @@ envoy_mobile_android_test( "ProxyInfoIntentPerformHTTPSRequestUsingProxyTest.kt", ], exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", "dockerNetwork": "standard", }, native_deps = [ @@ -65,8 +61,6 @@ envoy_mobile_android_test( "ProxyInfoIntentPerformHTTPSRequestUsingAsyncProxyTest.kt", ], exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", "dockerNetwork": "standard", }, native_deps = [ @@ -92,8 +86,6 @@ envoy_mobile_android_test( "ProxyInfoIntentPerformHTTPSRequestBadHostnameTest.kt", ], exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", "dockerNetwork": "standard", }, native_deps = [ @@ -119,8 +111,6 @@ envoy_mobile_android_test( "ProxyPollPerformHTTPRequestUsingProxyTest.kt", ], exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", "dockerNetwork": "standard", }, native_deps = [ @@ -146,8 +136,6 @@ envoy_mobile_android_test( "ProxyPollPerformHTTPRequestWithoutUsingPACProxyTest.kt", ], exec_properties = { - # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. - "sandboxNetwork": "standard", "dockerNetwork": "standard", }, native_deps = [ From fd19754daaf158830541729d1b10676a113fdc60 Mon Sep 17 00:00:00 2001 From: RenjieTang Date: Fri, 7 Jun 2024 11:50:51 -0700 Subject: [PATCH 14/30] Update QUICHE from cea6f57f9 to c5d1ed730 (#34526) Signed-off-by: Renjie Tang --- bazel/repository_locations.bzl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index ddf8a30a8a96..a675c338594c 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -1192,12 +1192,12 @@ REPOSITORY_LOCATIONS_SPEC = dict( project_name = "QUICHE", project_desc = "QUICHE (QUIC, HTTP/2, Etc) is Google‘s implementation of QUIC and related protocols", project_url = "https://github.com/google/quiche", - version = "cea6f57f9ce03a5aa2bb0e0d8adcdf3ab452c0c3", - sha256 = "d0187c4c3c74a709727549b020ef90471113d70047dff7d8fd9f2bfd37a6da5b", + version = "c5d1ed730f6062c6647aebe130d78f088028e52f", + sha256 = "d4d61c3189d989d7e51323823ce6f9de6970a09803624e80283e9f8135c9fe4b", urls = ["https://github.com/google/quiche/archive/{version}.tar.gz"], strip_prefix = "quiche-{version}", use_category = ["controlplane", "dataplane_core"], - release_date = "2024-05-29", + release_date = "2024-06-04", cpe = "N/A", license = "BSD-3-Clause", license_url = "https://github.com/google/quiche/blob/{version}/LICENSE", From ff862a246420cf5cb6628346e8ac384a02d7ae36 Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Fri, 7 Jun 2024 14:34:25 -0500 Subject: [PATCH 15/30] mobile: Remove no-remote-exec (#34610) This PR removes the -no-remote-exec tag and replaces it with sandboxNetwork=standard exec_properties to allow binding a socket to localhost so that the tests are executed with EngFlow instead of the GitHub runner. This should speed up the iOS tests significantly. Risk Level: low Testing: CI Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Fredy Wijaya --- mobile/bazel/apple.bzl | 3 +- mobile/test/objective-c/EnvoyTestServer.h | 7 +- mobile/test/swift/BUILD | 4 +- mobile/test/swift/integration/BUILD | 76 +++++++++++++------- mobile/test/swift/integration/proxying/BUILD | 7 +- mobile/test/swift/stats/BUILD | 4 +- 6 files changed, 66 insertions(+), 35 deletions(-) diff --git a/mobile/bazel/apple.bzl b/mobile/bazel/apple.bzl index 309f9aa10670..e3600e1eee32 100644 --- a/mobile/bazel/apple.bzl +++ b/mobile/bazel/apple.bzl @@ -34,7 +34,7 @@ def envoy_objc_library(name, hdrs = [], visibility = [], data = [], deps = [], m # ], # ) # -def envoy_mobile_swift_test(name, srcs, size = None, data = [], deps = [], tags = [], repository = "", visibility = [], flaky = False): +def envoy_mobile_swift_test(name, srcs, size = None, data = [], deps = [], tags = [], repository = "", visibility = [], flaky = False, exec_properties = {}): test_lib_name = name + "_lib" swift_library( name = test_lib_name, @@ -57,6 +57,7 @@ def envoy_mobile_swift_test(name, srcs, size = None, data = [], deps = [], tags tags = tags, visibility = visibility, flaky = flaky, + exec_properties = exec_properties, ) def envoy_mobile_objc_test(name, srcs, data = [], deps = [], tags = [], visibility = [], flaky = False): diff --git a/mobile/test/objective-c/EnvoyTestServer.h b/mobile/test/objective-c/EnvoyTestServer.h index 13b0ea1f48bf..dbc9758d9e64 100644 --- a/mobile/test/objective-c/EnvoyTestServer.h +++ b/mobile/test/objective-c/EnvoyTestServer.h @@ -4,9 +4,10 @@ // Interface for starting and managing a test server. Calls into to test_server.cc // -// NB: Any test that utilizes this class must have a `no-remote-exec` tag in its BUILD target. -// EnvoyTestServer binds to a listening socket on the machine it runs on, and on CI, this -// operation is not permitted in remote execution environments. +// NB: Any test that utilizes this class must have a `"sandboxNetwork": "standard"` +// `exec_properties` in its BUILD target to allow binding a listening socket on +// the EngFlow machines +// (https://docs.engflow.com/re/client/platform-options-reference.html#sandboxallowed). @interface EnvoyTestServer : NSObject // Get the port of the upstream server. diff --git a/mobile/test/swift/BUILD b/mobile/test/swift/BUILD index 80b407888809..7077c25b6368 100644 --- a/mobile/test/swift/BUILD +++ b/mobile/test/swift/BUILD @@ -20,8 +20,10 @@ envoy_mobile_swift_test( "RetryPolicyMapperTests.swift", "RetryPolicyTests.swift", ], + exec_properties = { + "sandboxNetwork": "standard", + }, flaky = True, # TODO(jpsim): Fix timeouts when running these tests on CI - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec visibility = ["//visibility:public"], deps = [ "//library/objective-c:envoy_engine_objc_lib", diff --git a/mobile/test/swift/integration/BUILD b/mobile/test/swift/integration/BUILD index 5eeac3b43fdf..fb0f71f20fd0 100644 --- a/mobile/test/swift/integration/BUILD +++ b/mobile/test/swift/integration/BUILD @@ -10,11 +10,9 @@ envoy_mobile_swift_test( srcs = [ "EndToEndNetworkingTest.swift", ], - # The test starts an Envoy server, which binds to a local socket. Binding to a local socket is - # not permitted on remote execution hosts, so we have to add the `no-remote-exec` tag. - tags = [ - "no-remote-exec", - ], + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -28,7 +26,9 @@ envoy_mobile_swift_test( srcs = [ "CancelStreamTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -42,7 +42,9 @@ envoy_mobile_swift_test( srcs = [ "EngineApiTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -55,7 +57,9 @@ envoy_mobile_swift_test( srcs = [ "FilterResetIdleTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -69,7 +73,9 @@ envoy_mobile_swift_test( srcs = [ "GRPCReceiveErrorTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -83,11 +89,9 @@ envoy_mobile_swift_test( srcs = [ "IdleTimeoutTest.swift", ], - # The test starts an Envoy server, which binds to a local socket. Binding to a local socket is - # not permitted on remote execution hosts, so we have to add the `no-remote-exec` tag. - tags = [ - "no-remote-exec", - ], + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -101,7 +105,9 @@ envoy_mobile_swift_test( srcs = [ "KeyValueStoreTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -115,7 +121,9 @@ envoy_mobile_swift_test( srcs = [ "ReceiveDataTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -129,7 +137,9 @@ envoy_mobile_swift_test( srcs = [ "ReceiveErrorTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -142,7 +152,9 @@ envoy_mobile_swift_test( srcs = [ "ResetConnectivityStateTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -156,7 +168,9 @@ envoy_mobile_swift_test( srcs = [ "SendDataTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -170,7 +184,9 @@ envoy_mobile_swift_test( srcs = [ "SendHeadersTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -184,7 +200,9 @@ envoy_mobile_swift_test( srcs = [ "SendTrailersTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -198,7 +216,9 @@ envoy_mobile_swift_test( srcs = [ "SetEventTrackerTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -212,7 +232,9 @@ envoy_mobile_swift_test( srcs = [ "SetEventTrackerTestNoTracker.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -226,7 +248,9 @@ envoy_mobile_swift_test( srcs = [ "SetLoggerTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", @@ -240,7 +264,9 @@ envoy_mobile_swift_test( srcs = [ "CancelGRPCStreamTest.swift", ], - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ ":test_extensions", diff --git a/mobile/test/swift/integration/proxying/BUILD b/mobile/test/swift/integration/proxying/BUILD index d15f5a6f6526..b891bf3b39c7 100644 --- a/mobile/test/swift/integration/proxying/BUILD +++ b/mobile/test/swift/integration/proxying/BUILD @@ -10,10 +10,9 @@ envoy_mobile_swift_test( srcs = [ "HTTPRequestUsingProxyTest.swift", ], - # The test starts an Envoy test server, which requires the `no-remote-exec` tag. - tags = [ - "no-remote-exec", - ], + exec_properties = { + "sandboxNetwork": "standard", + }, visibility = ["//visibility:public"], deps = [ "//library/objective-c:envoy_engine_objc_lib", diff --git a/mobile/test/swift/stats/BUILD b/mobile/test/swift/stats/BUILD index 14e1d31bd8d0..bbecf8d2e710 100644 --- a/mobile/test/swift/stats/BUILD +++ b/mobile/test/swift/stats/BUILD @@ -12,8 +12,10 @@ envoy_mobile_swift_test( "ElementTests.swift", "TagsBuilderTests.swift", ], + exec_properties = { + "sandboxNetwork": "standard", + }, flaky = True, # TODO(jpsim): Fix timeouts when running these tests on CI - tags = ["no-remote-exec"], # TODO(32551): Re-enable remote exec visibility = ["//visibility:public"], deps = [ "//library/objective-c:envoy_engine_objc_lib", From 208dac8607f6223224ed105a995933bf4961726d Mon Sep 17 00:00:00 2001 From: botengyao Date: Fri, 7 Jun 2024 15:50:56 -0400 Subject: [PATCH 16/30] coverage: add datadog test and bring back `extensions/tracers` threshhold (#34587) Signed-off-by: Boteng Yao --- .../tracers/datadog/agent_http_client_test.cc | 37 +++++++++++++++++++ test/per_file_coverage.sh | 2 +- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 57e4bcceb4d3..9b367cca6067 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -352,6 +352,43 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorStreamReset) { callbacks_->onFailure(request_, Http::AsyncClient::FailureReason::Reset); } +TEST_F(DatadogAgentHttpClientTest, OnErrorExceedResponseBufferLimit) { + // When `onFailure` is invoked on the `Http::AsyncClient::Callbacks` with + // `FailureReason::ExceedResponseBufferLimit`, the associated `on_error` callback is invoked + // with a corresponding `datadog::tracing::Error`. + + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([this](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks_ = &callbacks_arg; + return &request_; + })); + + // `callbacks_->onFailure(...)` will cause `on_error_` to be called. + // `on_response_` will not be called. + EXPECT_CALL(on_error_, Call(_)).WillOnce(Invoke([](datadog::tracing::Error error) { + EXPECT_EQ(error.code, datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE); + })); + EXPECT_CALL(on_response_, Call(_, _, _)).Times(0); + + // The request will not be canceled; neither explicitly nor in + // `~AgentHTTPClient`, because it will have been fulfilled. + EXPECT_CALL(request_, cancel()).Times(0); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction(), + time_.monotonicTime() + std::chrono::seconds(1)); + EXPECT_TRUE(result) << result.error(); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); + msg->body().add("{}"); + + callbacks_->onFailure(request_, Http::AsyncClient::FailureReason::ExceedResponseBufferLimit); +} + TEST_F(DatadogAgentHttpClientTest, OnErrorOther) { // When `onFailure` is invoked on the `Http::AsyncClient::Callbacks` with any // value other than `FailureReason::Reset`, the associated `on_error` callback diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index f48c76c288a0..2d2b536f3949 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -44,7 +44,7 @@ declare -a KNOWN_LOW_COVERAGE=( "source/extensions/rate_limit_descriptors/expr:95.0" "source/extensions/stat_sinks/graphite_statsd:82.8" # Death tests don't report LCOV "source/extensions/stat_sinks/statsd:85.2" # Death tests don't report LCOV -"source/extensions/tracers:96.4" +"source/extensions/tracers:96.5" "source/extensions/tracers/common:74.8" "source/extensions/tracers/common/ot:72.9" "source/extensions/tracers/opencensus:93.9" From 89d7b57c5d8563c5aec98345a9ea10fd6ee3ed15 Mon Sep 17 00:00:00 2001 From: code Date: Sat, 8 Jun 2024 08:52:44 +0800 Subject: [PATCH 17/30] runtime: remove refresh rtt runtime flag (#34599) Signed-off-by: wbpcode Co-authored-by: wbpcode --- changelogs/current.yaml | 3 ++ source/common/http/conn_manager_impl.cc | 17 +------- source/common/http/conn_manager_impl.h | 2 - source/common/runtime/runtime_features.cc | 2 - test/common/http/conn_manager_impl_test.cc | 38 ----------------- test/integration/filter_integration_test.cc | 45 --------------------- 6 files changed, 5 insertions(+), 102 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 6da420210a7e..8b2cfad31f1a 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -82,6 +82,9 @@ minor_behavior_changes: - area: filters change: | Set ``WWW-Authenticate`` header for 401 responses from the Basic Auth filter. +- area: http + change: | + Removed runtime guard ``envoy.reloadable_features.refresh_rtt_after_request`` and legacy code path. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index d8ddacb89e9b..b217c1e2f7b2 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -126,10 +126,8 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfigSharedPtr co /*node_id=*/local_info_.node().id(), /*server_name=*/config_->serverName(), /*proxy_status_config=*/config_->proxyStatusConfig())), - max_requests_during_dispatch_( - runtime_.snapshot().getInteger(ConnectionManagerImpl::MaxRequestsPerIoCycle, UINT32_MAX)), - refresh_rtt_after_request_( - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.refresh_rtt_after_request")) { + max_requests_during_dispatch_(runtime_.snapshot().getInteger( + ConnectionManagerImpl::MaxRequestsPerIoCycle, UINT32_MAX)) { ENVOY_LOG_ONCE_IF( trace, accept_new_http_stream_ == nullptr, "LoadShedPoint envoy.load_shed_points.http_connection_manager_decode_headers is not " @@ -337,17 +335,6 @@ void ConnectionManagerImpl::doDeferredStreamDestroy(ActiveStream& stream) { } stream.completeRequest(); - - // If refresh rtt after request is required explicitly, then try to get rtt again set it into - // connection info. - if (refresh_rtt_after_request_) { - // Set roundtrip time in connectionInfoSetter before OnStreamComplete - absl::optional t = read_callbacks_->connection().lastRoundTripTime(); - if (t.has_value()) { - read_callbacks_->connection().connectionInfoSetter().setRoundTripTime(t.value()); - } - } - stream.filter_manager_.onStreamComplete(); // For HTTP/3, skip access logging here and add deferred logging info diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 5f546ed9be3d..a325dff0dfb2 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -638,8 +638,6 @@ class ConnectionManagerImpl : Logger::Loggable, uint32_t requests_during_dispatch_count_{0}; const uint32_t max_requests_during_dispatch_{UINT32_MAX}; Event::SchedulableCallbackPtr deferred_request_processing_callback_; - - const bool refresh_rtt_after_request_{}; }; } // namespace Http diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 708f029cdac2..f1a9815ec6eb 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -132,8 +132,6 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_runtime_initialized); // TODO(mattklein123): Also unit test this if this sticks and this becomes the default for Apple & // Android. FALSE_RUNTIME_GUARD(envoy_reloadable_features_always_use_v6); -// TODO(wbpcode) complete remove this feature is no one use it. -FALSE_RUNTIME_GUARD(envoy_reloadable_features_refresh_rtt_after_request); // TODO(vikaschoudhary16) flip this to true only after all the // TcpProxy::Filter::HttpStreamDecoderFilterCallbacks are implemented or commented as unnecessary FALSE_RUNTIME_GUARD(envoy_restart_features_upstream_http_filters_with_tcp_proxy); diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 27e876a43669..51cc25624285 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -3570,44 +3570,6 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterBidiData) { EXPECT_EQ("world", response_body); } -TEST_F(HttpConnectionManagerImplTest, RoundTripTimeHasValue) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.refresh_rtt_after_request", "true"}}); - - setup(false, ""); - - // Set up the codec. - Buffer::OwnedImpl fake_input("input"); - conn_manager_->createCodec(fake_input); - - startRequest(true); - - EXPECT_CALL(filter_callbacks_.connection_, lastRoundTripTime()) - .WillOnce(Return(absl::optional(300))); - EXPECT_CALL(filter_callbacks_.connection_, connectionInfoSetter()); - - filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); -} - -TEST_F(HttpConnectionManagerImplTest, RoundTripTimeHasNoValue) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.refresh_rtt_after_request", "true"}}); - - setup(false, ""); - - // Set up the codec. - Buffer::OwnedImpl fake_input("input"); - conn_manager_->createCodec(fake_input); - - startRequest(true); - - EXPECT_CALL(filter_callbacks_.connection_, lastRoundTripTime()) - .WillOnce(Return(absl::optional())); - EXPECT_CALL(filter_callbacks_.connection_, connectionInfoSetter()).Times(0); - - filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); -} - TEST_F(HttpConnectionManagerImplTest, RequestTimeoutDisabledByDefault) { setup(false, ""); diff --git a/test/integration/filter_integration_test.cc b/test/integration/filter_integration_test.cc index dad67eb89edd..3d1af43a8c7e 100644 --- a/test/integration/filter_integration_test.cc +++ b/test/integration/filter_integration_test.cc @@ -224,51 +224,6 @@ TEST_P(FilterIntegrationTest, MissingHeadersLocalReplyDownstreamBytesCount) { } } -TEST_P(FilterIntegrationTest, RoundTripTimeForDownstreamConnection) { - config_helper_.addRuntimeOverride("envoy.reloadable_features.refresh_rtt_after_request", "true"); - - config_helper_.prependFilter(R"EOF( - name: stream-info-to-headers-filter - )EOF"); - initialize(); - - codec_client_ = makeHttpConnection(lookupPort("http")); - - // Send first request. - { - // Send a headers only request. - auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); - waitForNextUpstreamRequest(); - - // Make sure that the body was injected to the request. - EXPECT_TRUE(upstream_request_->complete()); - - // Send a headers only response. - upstream_request_->encodeHeaders(default_response_headers_, true); - ASSERT_TRUE(response->waitForEndStream()); - - // Make sure that round trip time was populated - EXPECT_FALSE(response->headers().get(Http::LowerCaseString("round_trip_time")).empty()); - } - - // Send second request. - { - // Send a headers only request. - auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); - waitForNextUpstreamRequest(); - - // Make sure that the body was injected to the request. - EXPECT_TRUE(upstream_request_->complete()); - - // Send a headers only response. - upstream_request_->encodeHeaders(default_response_headers_, true); - ASSERT_TRUE(response->waitForEndStream()); - - // Make sure that round trip time was populated - EXPECT_FALSE(response->headers().get(Http::LowerCaseString("round_trip_time")).empty()); - } -} - TEST_P(FilterIntegrationTest, MissingHeadersLocalReplyUpstreamBytesCount) { useAccessLog("%UPSTREAM_WIRE_BYTES_SENT% %UPSTREAM_WIRE_BYTES_RECEIVED% " "%UPSTREAM_HEADER_BYTES_SENT% %UPSTREAM_HEADER_BYTES_RECEIVED%"); From a6792927ff71015069326490af5306ad56d769c9 Mon Sep 17 00:00:00 2001 From: Jonathan Albrecht Date: Sat, 8 Jun 2024 13:17:35 -0400 Subject: [PATCH 18/30] Add big endian support to the dns parser (#34456) Fixes: //test/extensions/filters/udp/dns_filter:dns_filter_integration_test //test/extensions/filters/udp/dns_filter:dns_filter_test on big endian platforms. Signed-off-by: Jonathan Albrecht --- .../filters/udp/dns_filter/dns_parser.cc | 5 +++++ .../filters/udp/dns_filter/dns_parser.h | 21 ++++++++++++++++--- .../filters/udp/dns_filter/dns_filter_test.cc | 6 ++++++ 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/udp/dns_filter/dns_parser.cc b/source/extensions/filters/udp/dns_filter/dns_parser.cc index b63f69278d89..ed4b850a5c8b 100644 --- a/source/extensions/filters/udp/dns_filter/dns_parser.cc +++ b/source/extensions/filters/udp/dns_filter/dns_parser.cc @@ -91,8 +91,13 @@ bool DnsAnswerRecord::serialize(Buffer::OwnedImpl& output) { // Store the 128bit address with 2 64 bit writes const absl::uint128 addr6 = ip_address->ipv6()->address(); output.writeBEInt(sizeof(addr6)); +#ifdef ABSL_IS_BIG_ENDIAN + output.writeBEInt(absl::Uint128High64(addr6)); + output.writeBEInt(absl::Uint128Low64(addr6)); +#else output.writeLEInt(absl::Uint128Low64(addr6)); output.writeLEInt(absl::Uint128High64(addr6)); +#endif } else if (ip_address->ipv4() != nullptr) { output.writeBEInt(4); output.writeLEInt(ip_address->ipv4()->address()); diff --git a/source/extensions/filters/udp/dns_filter/dns_parser.h b/source/extensions/filters/udp/dns_filter/dns_parser.h index 80ebabd398ae..3449c19d6478 100644 --- a/source/extensions/filters/udp/dns_filter/dns_parser.h +++ b/source/extensions/filters/udp/dns_filter/dns_parser.h @@ -137,9 +137,23 @@ struct DnsParserCounters { queries_with_ans_or_authority_rrs(queries_with_ans_or_authority_rrs) {} }; -// The flags have been verified with dig and this structure should not be modified. The flag -// order here does not match the RFC, but takes byte ordering into account so that serialization -// does not bitwise operations. +// The flags have been verified with dig and this structure should not be modified. On little +// endian platforms, the flag order here does not match the RFC, but takes byte ordering into +// account so that serialization does not bitwise operations. +#if ABSL_IS_BIG_ENDIAN +PACKED_STRUCT(struct DnsHeaderFlags { + unsigned qr : 1; // query or response + unsigned opcode : 4; // operation code + unsigned aa : 1; // authoritative answer + unsigned tc : 1; // truncated response + unsigned rd : 1; // recursion desired + unsigned ra : 1; // recursion available + unsigned z : 1; // z - bit (must be zero in queries per RFC1035) + unsigned ad : 1; // authenticated data + unsigned cd : 1; // checking disabled + unsigned rcode : 4; // return code +}); +#else PACKED_STRUCT(struct DnsHeaderFlags { unsigned rcode : 4; // return code unsigned cd : 1; // checking disabled @@ -152,6 +166,7 @@ PACKED_STRUCT(struct DnsHeaderFlags { unsigned opcode : 4; // operation code unsigned qr : 1; // query or response }); +#endif /** * Structure representing the DNS header as it appears in a packet diff --git a/test/extensions/filters/udp/dns_filter/dns_filter_test.cc b/test/extensions/filters/udp/dns_filter/dns_filter_test.cc index 58620fc5191a..ba7a0e61aa86 100644 --- a/test/extensions/filters/udp/dns_filter/dns_filter_test.cc +++ b/test/extensions/filters/udp/dns_filter/dns_filter_test.cc @@ -1910,6 +1910,12 @@ TEST_F(DnsFilterTest, InvalidShortBufferTest) { } TEST_F(DnsFilterTest, RandomizeFirstAnswerTest) { +#if defined(__linux__) && defined(__s390x__) + // Skip on s390x because this test incorrectly depends on the ordering of + // addresses that happens to work on other platforms. + // See https://github.com/envoyproxy/envoy/pull/24330 + GTEST_SKIP() << "Skipping RandomizeFirstAnswerTest on s390x"; +#endif InSequence s; setup(forward_query_off_config); From c2a2cfe43e5c842ba0f62ac55ae3e56ad136fb74 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 10 Jun 2024 08:38:22 +0100 Subject: [PATCH 19/30] build(deps-dev): bump @vitejs/plugin-react from 4.3.0 to 4.3.1 in /examples/single-page-app/ui (#34623) build(deps-dev): bump @vitejs/plugin-react Bumps [@vitejs/plugin-react](https://github.com/vitejs/vite-plugin-react/tree/HEAD/packages/plugin-react) from 4.3.0 to 4.3.1. - [Release notes](https://github.com/vitejs/vite-plugin-react/releases) - [Changelog](https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-react/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite-plugin-react/commits/v4.3.1/packages/plugin-react) --- updated-dependencies: - dependency-name: "@vitejs/plugin-react" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- examples/single-page-app/ui/package.json | 2 +- examples/single-page-app/ui/yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/examples/single-page-app/ui/package.json b/examples/single-page-app/ui/package.json index e57c089c2230..a8989ee685f3 100644 --- a/examples/single-page-app/ui/package.json +++ b/examples/single-page-app/ui/package.json @@ -25,7 +25,7 @@ "@types/react-dom": "^18.3.0", "@typescript-eslint/eslint-plugin": "^6.21.0", "@typescript-eslint/parser": "^6.21.0", - "@vitejs/plugin-react": "^4.3.0", + "@vitejs/plugin-react": "^4.3.1", "eslint": "^8.57.0", "eslint-config-standard-with-typescript": "^43.0.0", "eslint-plugin-import": "^2.25.2", diff --git a/examples/single-page-app/ui/yarn.lock b/examples/single-page-app/ui/yarn.lock index 9684f150be14..3fcbe07b6d4e 100644 --- a/examples/single-page-app/ui/yarn.lock +++ b/examples/single-page-app/ui/yarn.lock @@ -1700,10 +1700,10 @@ resolved "https://registry.yarnpkg.com/@ungap/structured-clone/-/structured-clone-1.2.0.tgz#756641adb587851b5ccb3e095daf27ae581c8406" integrity sha512-zuVdFrMJiuCDQUMCzQaD6KL28MjnqqN8XnAqiEq9PNm/hCPTSGfrXCOfwj1ow4LFb/tNymJPwsNbVePc1xFqrQ== -"@vitejs/plugin-react@^4.3.0": - version "4.3.0" - resolved "https://registry.yarnpkg.com/@vitejs/plugin-react/-/plugin-react-4.3.0.tgz#f20ec2369a92d8abaaefa60da8b7157819d20481" - integrity sha512-KcEbMsn4Dpk+LIbHMj7gDPRKaTMStxxWRkRmxsg/jVdFdJCZWt1SchZcf0M4t8lIKdwwMsEyzhrcOXRrDPtOBw== +"@vitejs/plugin-react@^4.3.1": + version "4.3.1" + resolved "https://registry.yarnpkg.com/@vitejs/plugin-react/-/plugin-react-4.3.1.tgz#d0be6594051ded8957df555ff07a991fb618b48e" + integrity sha512-m/V2syj5CuVnaxcUJOQRel/Wr31FFXRFlnOoq1TVtkCxsY5veGMTEmpWHndrhB2U8ScHtCQB1e+4hWYExQc6Lg== dependencies: "@babel/core" "^7.24.5" "@babel/plugin-transform-react-jsx-self" "^7.24.5" From 783d7510e15041b95109fa5e3a953afd136d2388 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 10 Jun 2024 08:38:36 +0100 Subject: [PATCH 20/30] build(deps-dev): bump vite from 5.2.12 to 5.2.13 in /examples/single-page-app/ui (#34622) build(deps-dev): bump vite in /examples/single-page-app/ui Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 5.2.12 to 5.2.13. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/v5.2.13/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v5.2.13/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- examples/single-page-app/ui/package.json | 2 +- examples/single-page-app/ui/yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/examples/single-page-app/ui/package.json b/examples/single-page-app/ui/package.json index a8989ee685f3..3df85522663a 100644 --- a/examples/single-page-app/ui/package.json +++ b/examples/single-page-app/ui/package.json @@ -35,6 +35,6 @@ "eslint-plugin-react-hooks": "^4.6.2", "eslint-plugin-react-refresh": "^0.4.7", "typescript": "*", - "vite": "^5.2.12" + "vite": "^5.2.13" } } diff --git a/examples/single-page-app/ui/yarn.lock b/examples/single-page-app/ui/yarn.lock index 3fcbe07b6d4e..b2125546276a 100644 --- a/examples/single-page-app/ui/yarn.lock +++ b/examples/single-page-app/ui/yarn.lock @@ -4290,10 +4290,10 @@ use-sidecar@^1.1.2: detect-node-es "^1.1.0" tslib "^2.0.0" -vite@^5.2.12: - version "5.2.12" - resolved "https://registry.yarnpkg.com/vite/-/vite-5.2.12.tgz#3536c93c58ba18edea4915a2ac573e6537409d97" - integrity sha512-/gC8GxzxMK5ntBwb48pR32GGhENnjtY30G4A0jemunsBkiEZFw60s8InGpN8gkhHEkjnRK1aSAxeQgwvFhUHAA== +vite@^5.2.13: + version "5.2.13" + resolved "https://registry.yarnpkg.com/vite/-/vite-5.2.13.tgz#945ababcbe3d837ae2479c29f661cd20bc5e1a80" + integrity sha512-SSq1noJfY9pR3I1TUENL3rQYDQCFqgD+lM6fTRAM8Nv6Lsg5hDLaXkjETVeBt+7vZBCMoibD+6IWnT2mJ+Zb/A== dependencies: esbuild "^0.20.1" postcss "^8.4.38" From a17fd267503b8b1cc865f433d0c7c000aa358a21 Mon Sep 17 00:00:00 2001 From: botengyao Date: Mon, 10 Jun 2024 03:42:05 -0400 Subject: [PATCH 21/30] sec-release: updated 2024 q2 release (#34551) Signed-off-by: Boteng Yao --- RELEASES.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/RELEASES.md b/RELEASES.md index 3d645c62ce9f..bab49dae35e8 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -65,6 +65,7 @@ actual mechanics of the release itself. | 2022 Q4 | Can Cecen ([cancecen](https://github.com/cancecen)) | Tony Allen ([tonya11en](https://github.com/tonya11en)) | | 2023 Q3 | Boteng Yao ([botengyao](https://github.com/botengyao)) | Kateryna Nezdolii ([nezdolik](https://github.com/nezdolik)) | | 2023 Q4 | Paul Merrison ([pmerrison](https://github.com/pmerrison)) | Brian Sonnenberg ([briansonnenberg](https://github.com/briansonnenberg)) | +| 2024 Q2 | Ryan Northey ([phlax](https://github.com/phlax)) | Boteng Yao ([botengyao](https://github.com/botengyao)) | ## Major release schedule @@ -135,6 +136,7 @@ Security releases are published on a 3-monthly cycle, around the mid point betwe | Quarter | Expected | Actual | Difference | |:-------:|:----------:|:----------:|:----------:| -| 2024 Q2 | 2024/06/04 | | | +| 2024 Q2 | 2024/06/04 | 2024/06/04 | 0 days | +| 2024 Q3 | 2024/09/03 | NOTE: Zero-day vulnerabilities, and upstream vulnerabilities disclosed to us under embargo, may necessitate an emergency release with little or no warning. From 79881c21e152bb033ebe048ba05a0709a3b0f2da Mon Sep 17 00:00:00 2001 From: Shivam7-1 <55046031+Shivam7-1@users.noreply.github.com> Date: Mon, 10 Jun 2024 18:30:16 +0530 Subject: [PATCH 22/30] Update searchtools.js DOM text reinterpreted as HTML (#34607) Signed-off-by: Shivam7-1 <55046031+Shivam7-1@users.noreply.github.com> --- docs/root/_static/searchtools.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/_static/searchtools.js b/docs/root/_static/searchtools.js index d460d8033155..02d0473059b3 100644 --- a/docs/root/_static/searchtools.js +++ b/docs/root/_static/searchtools.js @@ -129,7 +129,7 @@ const _displayItem = (item, searchTerms) => { }; const _finishSearch = (resultCount) => { Search.stopPulse(); - Search.title.innerText = _("Search Results"); + Search.title.textContent = _("Search Results"); if (!resultCount) Search.status.innerText = Documentation.gettext( "Your search did not match any documents. Please make sure that all words are spelled correctly and that you've selected enough categories." From cd23d24304511de10ba3113e70b8338762557399 Mon Sep 17 00:00:00 2001 From: Namrata Bhave Date: Mon, 10 Jun 2024 18:34:43 +0530 Subject: [PATCH 23/30] Fix //test/common/websocket:codec_test on big endian (#34443) Fix //test/common/websocket:codec_test on big endian(s390x) As length is set using memcpy and further processed as per LE format, the test fails on s390x. Signed-off-by: namrata-ibm --- source/common/websocket/codec.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/source/common/websocket/codec.cc b/source/common/websocket/codec.cc index 1c1fc4e40b1a..e95d8ab3d256 100644 --- a/source/common/websocket/codec.cc +++ b/source/common/websocket/codec.cc @@ -128,7 +128,11 @@ uint8_t Decoder::doDecodeExtendedLength(absl::Span& data) { num_remaining_extended_length_bytes_ -= bytes_to_decode; if (num_remaining_extended_length_bytes_ == 0) { +#if ABSL_IS_BIG_ENDIAN + length_ = state_ == State::FrameHeaderExtendedLength16Bit ? htole16(le64toh(length_)) : length_; +#else length_ = state_ == State::FrameHeaderExtendedLength16Bit ? htobe16(length_) : htobe64(length_); +#endif if (num_remaining_masking_key_bytes_ > 0) { state_ = State::FrameHeaderMaskingKey; } else { From 69d4ef8d04678710ec1633e1e7effbda6623cc8d Mon Sep 17 00:00:00 2001 From: Teju Nareddy Date: Mon, 10 Jun 2024 08:07:40 -0500 Subject: [PATCH 24/30] proxy_protocol_filter: Add field `stat_prefix` to the filter configuration (#34414) Commit Message: proxy_protocol_filter: Add field stat_prefix to the filter configuration Additional Description: This field allows for differentiating statistics when multiple proxy protocol listener filters are configured. This PR is a follow-up from previous conversation: #32861 (comment) Risk Level: Low All client-facing behavior changes are guarded by new filter config field. Testing: Stats unit tests Proxy protocol listener filter integration tests Docs Changes: Done Release Notes: Done Platform Specific Features: None Signed-off-by: Teju Nareddy --- .../proxy_protocol/v3/proxy_protocol.proto | 7 ++++++ changelogs/current.yaml | 5 ++++ .../listener_filters/proxy_protocol.rst | 6 ++--- source/common/config/well_known_names.cc | 14 ++++++++--- source/common/config/well_known_names.h | 2 ++ .../listener/proxy_protocol/proxy_protocol.cc | 15 ++++++++---- .../listener/proxy_protocol/proxy_protocol.h | 2 +- test/common/stats/tag_extractor_impl_test.cc | 11 +++++++++ .../proxy_proto_integration_test.cc | 23 ++++++++++++++----- 9 files changed, 67 insertions(+), 18 deletions(-) diff --git a/api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto b/api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto index 31ca8a6950e4..cc96c4822e23 100644 --- a/api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto +++ b/api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto @@ -18,6 +18,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // PROXY protocol listener filter. // [#extension: envoy.filters.listener.proxy_protocol] +// [#next-free-field: 6] message ProxyProtocol { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.listener.proxy_protocol.v2.ProxyProtocol"; @@ -85,4 +86,10 @@ message ProxyProtocol { // and an incoming request matches the V2 signature, the filter will allow the request through without any modification. // The filter treats this request as if it did not have any PROXY protocol information. repeated config.core.v3.ProxyProtocolConfig.Version disallowed_versions = 4; + + // The human readable prefix to use when emitting statistics for the filter. + // If not configured, statistics will be emitted without the prefix segment. + // See the :ref:`filter's statistics documentation ` for + // more information. + string stat_prefix = 5; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 8b2cfad31f1a..3c3e6ba20947 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -225,6 +225,11 @@ new_features: - area: redis change: | Added support for `inline commands `_. +- area: proxy_protocol + change: | + Added field :ref:`stat_prefix ` + to the proxy protocol listener filter configuration, allowing for differentiating statistics when multiple proxy + protocol listener filters are configured. - area: aws_lambda change: | The ``aws_lambda`` filter now supports the diff --git a/docs/root/configuration/listeners/listener_filters/proxy_protocol.rst b/docs/root/configuration/listeners/listener_filters/proxy_protocol.rst index e96d457e7725..c545f117c570 100644 --- a/docs/root/configuration/listeners/listener_filters/proxy_protocol.rst +++ b/docs/root/configuration/listeners/listener_filters/proxy_protocol.rst @@ -33,7 +33,7 @@ If there is a protocol error or an unsupported address family Statistics ---------- -This filter emits the following general statistics, rooted at *downstream_proxy_proto* +This filter emits the following general statistics, rooted at *proxy_proto.[.]* .. csv-table:: :header: Name, Type, Description @@ -42,7 +42,7 @@ This filter emits the following general statistics, rooted at *downstream_proxy_ not_found_disallowed, Counter, "Total number of connections that don't contain the PROXY protocol header and are rejected." not_found_allowed, Counter, "Total number of connections that don't contain the PROXY protocol header, but are allowed due to :ref:`allow_requests_without_proxy_protocol `." -The filter also emits the statistics rooted at *downstream_proxy_proto.versions.* +The filter also emits the statistics rooted at *proxy_proto.[.]versions.* for each matched PROXY protocol version. Proxy protocol versions include ``v1`` and ``v2``. .. csv-table:: @@ -53,7 +53,7 @@ for each matched PROXY protocol version. Proxy protocol versions include ``v1`` disallowed, Counter, "Total number of ``found`` connections that are rejected due to :ref:`disallowed_versions `." error, Counter, "Total number of connections where the PROXY protocol header was malformed (and the connection was rejected)." -The filter also emits the following legacy statistics, rooted at its own scope: +The filter also emits the following legacy statistics, rooted at its own scope and **not** including the *stat_prefix*: .. csv-table:: :header: Name, Type, Description diff --git a/source/common/config/well_known_names.cc b/source/common/config/well_known_names.cc index 1cc7913d77f4..c9b73b1ab089 100644 --- a/source/common/config/well_known_names.cc +++ b/source/common/config/well_known_names.cc @@ -27,7 +27,8 @@ std::string expandRegex(const std::string& regex) { // alphanumerics, underscores, and dashes. {"", R"([\w-\./]+)"}, // Match a prefix that is either a listener plus name or cluster plus name - {"", R"((?:listener|cluster)\..*?)"}}); + {"", R"((?:listener|cluster)\..*?)"}, + {"", R"(\d)"}}); } const Regex::CompiledGoogleReMatcher& validTagValueRegex() { @@ -218,8 +219,15 @@ TagNameValues::TagNameValues() { // http..rbac.(.)* but excluding policy addRe2(RBAC_HTTP_PREFIX, R"(^http\.\.rbac\.(()\.).*?)", "", "policy"); - // proxy_proto.(versions.v.)** - addRe2(PROXY_PROTOCOL_VERSION, R"(^proxy_proto\.(versions\.v(\d)\.)\w+)", "proxy_proto.versions"); + // proxy_proto.(.)** + addRe2(PROXY_PROTOCOL_PREFIX, R"(^proxy_proto\.(()\.).+$)", "", "versions"); + + // proxy_proto.([.]versions.v().)(found|disallowed|error) + // + // Strips out: [.]versions.v(). + // Leaving: proxy_proto.(found|disallowed|error) + addRe2(PROXY_PROTOCOL_VERSION, + R"(^proxy_proto\.((?:\.)?versions\.v()\.)\w+$)"); } void TagNameValues::addRe2(const std::string& name, const std::string& regex, diff --git a/source/common/config/well_known_names.h b/source/common/config/well_known_names.h index 86b522b9678f..334e50cc6aa2 100644 --- a/source/common/config/well_known_names.h +++ b/source/common/config/well_known_names.h @@ -165,6 +165,8 @@ class TagNameValues { const std::string REDIS_PREFIX = "envoy.redis_prefix"; // Proxy Protocol version for a connection (Proxy Protocol listener filter). const std::string PROXY_PROTOCOL_VERSION = "envoy.proxy_protocol_version"; + // Stats prefix for the proxy protocol listener filter. + const std::string PROXY_PROTOCOL_PREFIX = "envoy.proxy_protocol_prefix"; // Mapping from the names above to their respective regex strings. const std::vector> name_regex_pairs_; diff --git a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc index 847ce6d45e7b..5af1c2ca4094 100644 --- a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc +++ b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc @@ -55,17 +55,22 @@ namespace ProxyProtocol { constexpr absl::string_view kProxyProtoStatsPrefix = "proxy_proto."; constexpr absl::string_view kVersionStatsPrefix = "versions."; -ProxyProtocolStats ProxyProtocolStats::create(Stats::Scope& scope) { +ProxyProtocolStats ProxyProtocolStats::create(Stats::Scope& scope, absl::string_view stat_prefix) { + std::string filter_stat_prefix = std::string(kProxyProtoStatsPrefix); + if (!stat_prefix.empty()) { + filter_stat_prefix = absl::StrCat(kProxyProtoStatsPrefix, stat_prefix, "."); + } + return { /*legacy_=*/{LEGACY_PROXY_PROTOCOL_STATS(POOL_COUNTER(scope))}, /*general_=*/ - {GENERAL_PROXY_PROTOCOL_STATS(POOL_COUNTER_PREFIX(scope, kProxyProtoStatsPrefix))}, + {GENERAL_PROXY_PROTOCOL_STATS(POOL_COUNTER_PREFIX(scope, filter_stat_prefix))}, /*v1_=*/ {VERSIONED_PROXY_PROTOCOL_STATS(POOL_COUNTER_PREFIX( - scope, absl::StrCat(kProxyProtoStatsPrefix, kVersionStatsPrefix, "v1.")))}, + scope, absl::StrCat(filter_stat_prefix, kVersionStatsPrefix, "v1.")))}, /*v2_=*/ {VERSIONED_PROXY_PROTOCOL_STATS(POOL_COUNTER_PREFIX( - scope, absl::StrCat(kProxyProtoStatsPrefix, kVersionStatsPrefix, "v2.")))}, + scope, absl::StrCat(filter_stat_prefix, kVersionStatsPrefix, "v2.")))}, }; } @@ -105,7 +110,7 @@ void VersionedProxyProtocolStats::increment(ReadOrParseState decision) { Config::Config( Stats::Scope& scope, const envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol& proto_config) - : stats_(ProxyProtocolStats::create(scope)), + : stats_(ProxyProtocolStats::create(scope, proto_config.stat_prefix())), allow_requests_without_proxy_protocol_(proto_config.allow_requests_without_proxy_protocol()), pass_all_tlvs_(proto_config.has_pass_through_tlvs() ? proto_config.pass_through_tlvs().match_type() == diff --git a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.h b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.h index b3507e1a6071..3a508813891a 100644 --- a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.h +++ b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.h @@ -96,7 +96,7 @@ struct ProxyProtocolStats { * For backwards compatibility, the legacy stats are rooted under their own scope. * The general and versioned stats are correctly rooted at this filter's own scope. */ - static ProxyProtocolStats create(Stats::Scope& scope); + static ProxyProtocolStats create(Stats::Scope& scope, absl::string_view stat_prefix); }; /** diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index e7ed185c9b1b..6bb2e8443b1e 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -485,12 +485,23 @@ TEST(TagExtractorTest, DefaultTagExtractors) { "http.rbac.policy.shadow_denied", {rbac_http_hcm_prefix, rbac_http_prefix, rbac_policy_name}); + // Proxy Protocol stat prefix + Tag proxy_protocol_prefix; + proxy_protocol_prefix.name_ = tag_names.PROXY_PROTOCOL_PREFIX; + proxy_protocol_prefix.value_ = "test_stat_prefix"; + regex_tester.testRegex("proxy_proto.not_found_disallowed", "proxy_proto.not_found_disallowed", + {}); + regex_tester.testRegex("proxy_proto.test_stat_prefix.not_found_disallowed", + "proxy_proto.not_found_disallowed", {proxy_protocol_prefix}); + // Proxy Protocol version prefix Tag proxy_protocol_version; proxy_protocol_version.name_ = tag_names.PROXY_PROTOCOL_VERSION; proxy_protocol_version.value_ = "2"; regex_tester.testRegex("proxy_proto.versions.v2.error", "proxy_proto.error", {proxy_protocol_version}); + regex_tester.testRegex("proxy_proto.test_stat_prefix.versions.v2.error", "proxy_proto.error", + {proxy_protocol_prefix, proxy_protocol_version}); } TEST(TagExtractorTest, ExtAuthzTagExtractors) { diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_proto_integration_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_proto_integration_test.cc index 8ad20b803384..4ca41df0b729 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_proto_integration_test.cc +++ b/test/extensions/filters/listener/proxy_protocol/proxy_proto_integration_test.cc @@ -116,8 +116,9 @@ TEST_P(ProxyProtoIntegrationTest, V2RouterRequestAndResponseWithBodyNoBufferV6) testRouterRequestAndResponseWithBody(1024, 512, false, false, &creator); - // Verify stats (with tags for proxy protocol version). + // Verify stats (with tags for proxy protocol version, but no stat prefix). const auto found_counter = test_server_->counter("proxy_proto.versions.v2.found"); + ASSERT_NE(found_counter.get(), nullptr); EXPECT_EQ(found_counter->value(), 1UL); EXPECT_EQ(found_counter->tagExtractedName(), "proxy_proto.found"); EXPECT_THAT(found_counter->tags(), IsSupersetOf(Stats::TagVector{ @@ -378,6 +379,7 @@ ProxyProtoDisallowedVersionsIntegrationTest::ProxyProtoDisallowedVersionsIntegra // V1 is disallowed. ::envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proxy_protocol; proxy_protocol.add_disallowed_versions(::envoy::config::core::v3::ProxyProtocolConfig::V1); + proxy_protocol.set_stat_prefix("test_stat_prefix"); auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0); auto* ppv_filter = listener->mutable_listener_filters(0); @@ -400,19 +402,25 @@ TEST_P(ProxyProtoDisallowedVersionsIntegrationTest, V1Disallowed) { /*end_stream=*/false, /*verify=*/false)); tcp_client->waitForDisconnect(); - // Verify stats (with tags for proxy protocol version). - const auto found_counter = test_server_->counter("proxy_proto.versions.v1.found"); + // Verify stats (with tags for proxy protocol version and stat prefix). + const auto found_counter = + test_server_->counter("proxy_proto.test_stat_prefix.versions.v1.found"); + ASSERT_NE(found_counter.get(), nullptr); EXPECT_EQ(found_counter->value(), 1UL); EXPECT_EQ(found_counter->tagExtractedName(), "proxy_proto.found"); EXPECT_THAT(found_counter->tags(), IsSupersetOf(Stats::TagVector{ {"envoy.proxy_protocol_version", "1"}, + {"envoy.proxy_protocol_prefix", "test_stat_prefix"}, })); - const auto disallowed_counter = test_server_->counter("proxy_proto.versions.v1.disallowed"); + const auto disallowed_counter = + test_server_->counter("proxy_proto.test_stat_prefix.versions.v1.disallowed"); + ASSERT_NE(disallowed_counter.get(), nullptr); EXPECT_EQ(disallowed_counter->value(), 1UL); EXPECT_EQ(disallowed_counter->tagExtractedName(), "proxy_proto.disallowed"); EXPECT_THAT(disallowed_counter->tags(), IsSupersetOf(Stats::TagVector{ {"envoy.proxy_protocol_version", "1"}, + {"envoy.proxy_protocol_prefix", "test_stat_prefix"}, })); } @@ -430,12 +438,15 @@ TEST_P(ProxyProtoDisallowedVersionsIntegrationTest, V2Error) { ASSERT_TRUE(tcp_client->write(buf.toString(), /*end_stream=*/false, /*verify=*/false)); tcp_client->waitForDisconnect(); - // Verify stats (with tags for proxy protocol version). - const auto found_counter = test_server_->counter("proxy_proto.versions.v2.error"); + // Verify stats (with tags for proxy protocol version and stat prefix). + const auto found_counter = + test_server_->counter("proxy_proto.test_stat_prefix.versions.v2.error"); + ASSERT_NE(found_counter.get(), nullptr); EXPECT_EQ(found_counter->value(), 1UL); EXPECT_EQ(found_counter->tagExtractedName(), "proxy_proto.error"); EXPECT_THAT(found_counter->tags(), IsSupersetOf(Stats::TagVector{ {"envoy.proxy_protocol_version", "2"}, + {"envoy.proxy_protocol_prefix", "test_stat_prefix"}, })); } From 0650db1380f2ebf654c5b629764b2a6e8e0818e9 Mon Sep 17 00:00:00 2001 From: Paul Sohn Date: Mon, 10 Jun 2024 09:13:54 -0400 Subject: [PATCH 25/30] Fix QUIC deferred access logging for black-holed client (#34433) Commit Message: Fix QUIC deferred access logging when client terminates connection before final ack Additional Description: Addresses bug reported in #29930. Risk Level: Low Testing: Integration test Docs Changes: N/A Signed-off-by: Paul Sohn --- source/common/quic/quic_stats_gatherer.h | 5 ++ source/common/runtime/runtime_features.cc | 2 +- .../integration/quic_http_integration_test.cc | 55 +++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/source/common/quic/quic_stats_gatherer.h b/source/common/quic/quic_stats_gatherer.h index f704eeebc238..8a7b55fdc448 100644 --- a/source/common/quic/quic_stats_gatherer.h +++ b/source/common/quic/quic_stats_gatherer.h @@ -17,6 +17,11 @@ namespace Quic { class QuicStatsGatherer : public quic::QuicAckListenerInterface { public: explicit QuicStatsGatherer(Envoy::TimeSource* time_source) : time_source_(time_source) {} + ~QuicStatsGatherer() override { + if (!logging_done_) { + maybeDoDeferredLog(false); + } + } // QuicAckListenerInterface void OnPacketAcked(int acked_bytes, quic::QuicTime::Delta delta_largest_observed) override; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index f1a9815ec6eb..50bc1655a634 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -148,7 +148,7 @@ FALSE_RUNTIME_GUARD(envoy_restart_features_use_eds_cache_for_ads); // For more information about Universal Header Validation, please see // https://github.com/envoyproxy/envoy/issues/10646 FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_universal_header_validator); -// TODO(pksohn): enable after fixing https://github.com/envoyproxy/envoy/issues/29930 +// TODO(pksohn): enable after canarying fix for https://github.com/envoyproxy/envoy/issues/29930 FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_defer_logging_to_ack_listener); // TODO(panting): flip this to true after some test time. FALSE_RUNTIME_GUARD(envoy_reloadable_features_use_config_in_happy_eyeballs); diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index 33a28f2dda61..ce687bb7a122 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -1359,6 +1359,61 @@ TEST_P(QuicHttpIntegrationTest, DeferredLogging) { EXPECT_EQ(/* request headers */ metrics.at(19), metrics.at(20)); } +TEST_P(QuicHttpIntegrationTest, DeferredLoggingWithBlackholedClient) { + config_helper_.addRuntimeOverride("envoy.reloadable_features.quic_defer_logging_to_ack_listener", + "true"); + useAccessLog( + "%PROTOCOL%,%ROUNDTRIP_DURATION%,%REQUEST_DURATION%,%RESPONSE_DURATION%,%RESPONSE_" + "CODE%,%BYTES_RECEIVED%,%ROUTE_NAME%,%VIRTUAL_CLUSTER_NAME%,%RESPONSE_CODE_DETAILS%,%" + "CONNECTION_TERMINATION_DETAILS%,%START_TIME%,%UPSTREAM_HOST%,%DURATION%,%BYTES_SENT%,%" + "RESPONSE_FLAGS%,%DOWNSTREAM_LOCAL_ADDRESS%,%UPSTREAM_CLUSTER%,%STREAM_ID%,%DYNAMIC_" + "METADATA(" + "udp.proxy.session:bytes_sent)%,%REQ(:path)%,%STREAM_INFO_REQ(:path)%"); + initialize(); + + // Make a header-only request and delay the response by 1ms to ensure that the ROUNDTRIP_DURATION + // metric is > 0 if exists. + codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); + IntegrationStreamDecoderPtr response = + codec_client_->makeHeaderOnlyRequest(default_request_headers_); + absl::SleepFor(absl::Milliseconds(1)); + waitForNextUpstreamRequest(0, TestUtility::DefaultTimeout); + upstream_request_->encodeHeaders(default_response_headers_, true); + + // Prevent the client's dispatcher from running by not calling waitForEndStream or closing the + // client's connection, then wait for server to tear down connection due to too many + // retransmissions. + int iterations = 0; + std::string contents = TestEnvironment::readFileToStringForTest(access_log_name_); + while (iterations < 20) { + timeSystem().advanceTimeWait(std::chrono::seconds(1)); + // check for deferred logs from connection teardown. + contents = TestEnvironment::readFileToStringForTest(access_log_name_); + if (!contents.empty()) { + break; + } + iterations++; + } + + std::vector entries = absl::StrSplit(contents, '\n', absl::SkipEmpty()); + EXPECT_EQ(entries.size(), 1); + std::string log = entries[0]; + + std::vector metrics = absl::StrSplit(log, ','); + ASSERT_EQ(metrics.size(), 21); + EXPECT_EQ(/* PROTOCOL */ metrics.at(0), "HTTP/3"); + EXPECT_EQ(/* ROUNDTRIP_DURATION */ metrics.at(1), "-"); + EXPECT_GE(/* REQUEST_DURATION */ std::stoi(metrics.at(2)), 0); + EXPECT_GE(/* RESPONSE_DURATION */ std::stoi(metrics.at(3)), 0); + EXPECT_EQ(/* RESPONSE_CODE */ metrics.at(4), "200"); + EXPECT_EQ(/* BYTES_RECEIVED */ metrics.at(5), "0"); + // Ensure that request headers from top-level access logger parameter and stream info are + // consistent. + EXPECT_EQ(/* request headers */ metrics.at(19), metrics.at(20)); + + codec_client_->close(); +} + TEST_P(QuicHttpIntegrationTest, DeferredLoggingDisabled) { config_helper_.addRuntimeOverride("envoy.reloadable_features.quic_defer_logging_to_ack_listener", "false"); From b3b2c1afacbd2513a65989cc3cc75a625562ee1b Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Mon, 10 Jun 2024 09:48:32 -0400 Subject: [PATCH 26/30] tooling: fixing a string (#34580) Signed-off-by: Alyssa Wilk --- tools/code_format/check_format.py | 4 +++- tools/code_format/config.yaml | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index e0ae1fdf1a35..99c15b031ddf 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -806,7 +806,9 @@ def has_non_comment_throw(line): return False if self.deny_listed_for_exceptions(file_path): - if has_non_comment_throw(line) or "THROW" in line or "throwExceptionOrPanic" in line: + if has_non_comment_throw( + line) or "THROW" in line or "throwEnvoyExceptionOrPanic" in line: + report_error( "Don't introduce throws into exception-free files, use error " "statuses instead.") diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index fa8766048cad..fccba5e6cdd3 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -161,6 +161,7 @@ paths: - source/common/event/file_event_impl.cc - source/common/http/async_client_impl.cc - source/common/grpc/google_async_client_impl.cc + - source/common/formatter/substitution_format_utility.cc # Extensions can exempt entire directories but new extensions # points should ideally use StatusOr - source/extensions/access_loggers From 2ca8d41bc84183d0ea10b94c223c4dc295f668a2 Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Mon, 10 Jun 2024 10:02:16 -0400 Subject: [PATCH 27/30] Promote tyxia to maintainer status (#34593) Signed-off-by: Yan Avlasov --- OWNERS.md | 2 ++ SECURITY-INSIGHTS.yml | 1 + 2 files changed, 3 insertions(+) diff --git a/OWNERS.md b/OWNERS.md index 782dc7cb99d0..c5652b94fad1 100644 --- a/OWNERS.md +++ b/OWNERS.md @@ -52,6 +52,8 @@ routing PRs, questions, etc. to the right place. * Listeners, iouring, data plane. * Kateryna Nezdolii ([nezdolik](https://github.com/nezdolik)) (kateryna.nezdolii@gmail.com) * Load balancing, GeoIP, overload manager, security. +* Tianyu Xia ([tyxia](https://github.com/tyxia)) (tyxia@google.com) + * ext_proc, data plane, flow control, CEL. # Envoy mobile maintainers diff --git a/SECURITY-INSIGHTS.yml b/SECURITY-INSIGHTS.yml index 9498aaee3757..9fb54737a199 100644 --- a/SECURITY-INSIGHTS.yml +++ b/SECURITY-INSIGHTS.yml @@ -30,6 +30,7 @@ project-lifecycle: - github:ravenblackx - github:soulxu - github:nezdolik + - github:tyxia contribution-policy: accepts-pull-requests: true accepts-automated-pull-requests: true From 0a6b167c11ae1ba054f9d6fdc7a696ca97b3abd5 Mon Sep 17 00:00:00 2001 From: "Antonio V. Leonti" <53806445+antoniovleonti@users.noreply.github.com> Date: Mon, 10 Jun 2024 10:07:03 -0400 Subject: [PATCH 28/30] fuzz: ext_authz fuzz request body (#34608) Signed-off-by: antoniovleonti --- test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.cc | 4 ++++ test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.h | 2 ++ 2 files changed, 6 insertions(+) diff --git a/test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.cc b/test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.cc index 166b98132c2e..17fbf2505df2 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.cc @@ -26,12 +26,16 @@ ReusableFilterFactory::ReusableFilterFactory() .WillByDefault(Return(OptRef{connection_})); ON_CALL(decoder_callbacks_.stream_info_, dynamicMetadata()) .WillByDefault(testing::ReturnRef(metadata_)); + ON_CALL(decoder_callbacks_, decodingBuffer()).WillByDefault([this]() { + return decoding_buffer_.get(); + }); } std::unique_ptr ReusableFilterFactory::newFilter(FilterConfigSharedPtr config, Filters::Common::ExtAuthz::ClientPtr&& client, const envoy::config::core::v3::Metadata& metadata) { + decoding_buffer_ = std::make_unique(); metadata_ = metadata; std::unique_ptr filter = std::make_unique(std::move(config), std::move(client)); filter->setDecoderFilterCallbacks(decoder_callbacks_); diff --git a/test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.h b/test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.h index 1aa876843347..2330f34f2fc0 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.h +++ b/test/extensions/filters/http/ext_authz/ext_authz_fuzz_lib.h @@ -3,6 +3,7 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/network/address.h" +#include "source/common/buffer/buffer_impl.h" #include "source/extensions/filters/http/ext_authz/ext_authz.h" #include "test/extensions/filters/http/ext_authz/ext_authz_fuzz.pb.h" @@ -31,6 +32,7 @@ class ReusableFilterFactory { // Do not use ON_CALL outside of constructor on these mocks. Each ON_CALL has a memory cost and // will cause OOMs if the fuzzer runs long enough. NiceMock decoder_callbacks_; + std::unique_ptr decoding_buffer_; NiceMock encoder_callbacks_; Network::Address::InstanceConstSharedPtr addr_; NiceMock connection_; From ccd1bf931f3336e36e7ce95c27b898f92c12bdd8 Mon Sep 17 00:00:00 2001 From: Nigel Brittain <108375408+nbaws@users.noreply.github.com> Date: Tue, 11 Jun 2024 00:12:37 +1000 Subject: [PATCH 29/30] aws: key derivation fix and test case (#34377) Signed-off-by: Nigel Brittain --- .../common/aws/sigv4a_key_derivation.cc | 9 ++-- .../common/aws/sigv4a_signer_impl_test.cc | 42 +++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/source/extensions/common/aws/sigv4a_key_derivation.cc b/source/extensions/common/aws/sigv4a_key_derivation.cc index 9185aa9fdcb8..0ea42e2d7b12 100644 --- a/source/extensions/common/aws/sigv4a_key_derivation.cc +++ b/source/extensions/common/aws/sigv4a_key_derivation.cc @@ -36,11 +36,11 @@ EC_KEY* SigV4AKeyDerivation::derivePrivateKey(absl::string_view access_key_id, (external_counter <= 254)) // MAX_KEY_DERIVATION_COUNTER_VALUE { fixed_input.clear(); - fixed_input.insert(fixed_input.begin(), {0x00, 0x00, 0x00, 0x01}); - fixed_input.insert(fixed_input.end(), SigV4ASignatureConstants::SigV4AAlgorithm[0], - SigV4ASignatureConstants::SigV4AAlgorithm[0] + - sizeof(SigV4ASignatureConstants::SigV4AAlgorithm)); + // Workaround for asan optimization issue described here + // https://github.com/envoyproxy/envoy/pull/34377 + absl::string_view s(SigV4ASignatureConstants::SigV4AAlgorithm); + fixed_input.insert(fixed_input.end(), s.begin(), s.end()); fixed_input.insert(fixed_input.end(), 0x00); fixed_input.insert(fixed_input.end(), access_key_id.begin(), access_key_id.end()); fixed_input.insert(fixed_input.end(), external_counter); @@ -67,7 +67,6 @@ EC_KEY* SigV4AKeyDerivation::derivePrivateKey(absl::string_view access_key_id, result = SigV4AKeyDerivationResult::AkdrSuccess; // PrivateKey d = c+1 constantTimeAddOne(&k0); - priv_key_num = BN_bin2bn(k0.data(), k0.size(), nullptr); // Create a new OpenSSL EC_KEY by curve nid for secp256r1 (NIST P-256) diff --git a/test/extensions/common/aws/sigv4a_signer_impl_test.cc b/test/extensions/common/aws/sigv4a_signer_impl_test.cc index 626620074643..a47c8fc15590 100644 --- a/test/extensions/common/aws/sigv4a_signer_impl_test.cc +++ b/test/extensions/common/aws/sigv4a_signer_impl_test.cc @@ -501,6 +501,48 @@ TEST_F(SigV4ASignerImplTest, QueryStringDefault5s) { EXPECT_TRUE(absl::StrContains(headers.getPathValue(), "X-Amz-Expires=5&")); } +// Verify specific key derivations, using values generated from the AWS SDK implementation +TEST(SigV4AKeyDerivationTest, TestKeyDerivations) { + auto ec_key = + SigV4AKeyDerivation::derivePrivateKey(absl::string_view("akid"), absl::string_view("secret")); + + auto ec_private_key = EC_KEY_get0_private_key(ec_key); + auto hexkey = BN_bn2hex(ec_private_key); + EXPECT_STREQ(hexkey, "0a56b8224b63e587ab069a15a730a103add19b45a644a197d24415ff89b993dc"); + OPENSSL_free(hexkey); + EC_KEY_free(ec_key); + ec_key = SigV4AKeyDerivation::derivePrivateKey(absl::string_view("akid"), + absl::string_view("testkey2")); + + ec_private_key = EC_KEY_get0_private_key(ec_key); + hexkey = BN_bn2hex(ec_private_key); + + EXPECT_STREQ(hexkey, "7b6c4f9d70561838cd1160e5e8674bf3a40e8bb3865ccfee37b3c423035a5c43"); + OPENSSL_free(hexkey); + EC_KEY_free(ec_key); + + ec_key = SigV4AKeyDerivation::derivePrivateKey(absl::string_view("akid"), + absl::string_view("abcdefghi")); + + ec_private_key = EC_KEY_get0_private_key(ec_key); + hexkey = BN_bn2hex(ec_private_key); + EXPECT_STREQ(hexkey, "31ce325f5a7e167ce0659aa8fec550c005b892833bcb5627fba6b5c55023f1cc"); + OPENSSL_free(hexkey); + EC_KEY_free(ec_key); + + // This access key secret key combination will push our key derivation into two cycles, for more + // code coverage + ec_key = + SigV4AKeyDerivation::derivePrivateKey(absl::string_view("eb63466a7cf7ee3cd4880df6dc4aaed"), + absl::string_view("d7e7f9c8f2344a12bc51f3d05a2fb8")); + + ec_private_key = EC_KEY_get0_private_key(ec_key); + hexkey = BN_bn2hex(ec_private_key); + EXPECT_STREQ(hexkey, "d0fdb7810916566bd91ec0b3d45dcfc01de8f3ffe783754cf7ce4c6dd86f584b"); + OPENSSL_free(hexkey); + EC_KEY_free(ec_key); +} + } // namespace } // namespace Aws } // namespace Common From 0c9301f92743afd4ae0d48fe63dba3c9f3c10383 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Mon, 10 Jun 2024 10:27:24 -0400 Subject: [PATCH 30/30] mobile: removing some PANIC calls from Enovy mobile (#34586) Signed-off-by: Alyssa Wilk --- .../platform_bridge/platform_bridge_cert_validator.h | 2 +- .../extensions/filters/http/platform_bridge/filter.cc | 6 ------ mobile/library/common/http/client.cc | 2 +- mobile/library/common/http/client.h | 5 ++--- mobile/test/common/http/client_test.cc | 5 +++-- mobile/test/common/integration/client_integration_test.cc | 1 + 6 files changed, 8 insertions(+), 13 deletions(-) diff --git a/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h b/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h index 65a52a555894..7bde7390a38b 100644 --- a/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h +++ b/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h @@ -38,7 +38,7 @@ class PlatformBridgeCertValidator : public CertValidator, Logger::Loggable daysUntilFirstCertExpires() const override { return absl::nullopt; } Envoy::Ssl::CertificateDetailsPtr getCaCertInformation() const override { return nullptr; } diff --git a/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc b/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc index dfd22171c461..73fe0946c39e 100644 --- a/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc +++ b/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc @@ -299,8 +299,6 @@ Http::FilterHeadersStatus PlatformBridgeFilter::FilterBase::onHeaders(Http::Head default: PANIC("invalid filter state: unsupported status for platform filters"); } - - PANIC("not reached"); } Http::FilterDataStatus PlatformBridgeFilter::FilterBase::onData(Buffer::Instance& data, @@ -407,8 +405,6 @@ Http::FilterDataStatus PlatformBridgeFilter::FilterBase::onData(Buffer::Instance default: PANIC("invalid filter state: unsupported status for platform filters"); } - - PANIC("not reached"); } Http::FilterTrailersStatus PlatformBridgeFilter::FilterBase::onTrailers(Http::HeaderMap& trailers) { @@ -484,8 +480,6 @@ Http::FilterTrailersStatus PlatformBridgeFilter::FilterBase::onTrailers(Http::He default: PANIC("invalid filter state: unsupported status for platform filters"); } - - PANIC("not reached"); } Http::FilterHeadersStatus PlatformBridgeFilter::decodeHeaders(Http::RequestHeaderMap& headers, diff --git a/mobile/library/common/http/client.cc b/mobile/library/common/http/client.cc index 463bebdd97d0..0b94fcec9d14 100644 --- a/mobile/library/common/http/client.cc +++ b/mobile/library/common/http/client.cc @@ -661,7 +661,7 @@ void Client::sendData(envoy_stream_t stream, Buffer::InstancePtr buffer, bool en } } -void Client::sendMetadata(envoy_stream_t, envoy_headers) { PANIC("not implemented"); } +void Client::sendMetadata(envoy_stream_t, envoy_headers) { IS_ENVOY_BUG("not implemented"); } void Client::sendTrailers(envoy_stream_t stream, RequestTrailerMapPtr trailers) { ASSERT(dispatcher_.isThreadSafe()); diff --git a/mobile/library/common/http/client.h b/mobile/library/common/http/client.h index e59517b39376..1c66baabb8c6 100644 --- a/mobile/library/common/http/client.h +++ b/mobile/library/common/http/client.h @@ -161,8 +161,7 @@ class Client : public Logger::Loggable { Stream& getStream() override { return direct_stream_; } Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() override { return absl::nullopt; } void encode1xxHeaders(const ResponseHeaderMap&) override { - // TODO(goaway): implement? - PANIC("not implemented"); + IS_ENVOY_BUG("Unexpected 100 continue"); // proxy_100_continue_ false by default. } bool streamErrorOnInvalidHttpMessage() const override { return false; } void setRequestDecoder(RequestDecoder& /*decoder*/) override{}; @@ -171,7 +170,7 @@ class Client : public Logger::Loggable { Http::ResponseTrailerMapConstSharedPtr, StreamInfo::StreamInfo&) override {} - void encodeMetadata(const MetadataMapVector&) override { PANIC("not implemented"); } + void encodeMetadata(const MetadataMapVector&) override { IS_ENVOY_BUG("Unexpected metadata"); } void onHasBufferedData(); void onBufferedDataDrained(); diff --git a/mobile/test/common/http/client_test.cc b/mobile/test/common/http/client_test.cc index 8d3f8d286314..13004325bd1c 100644 --- a/mobile/test/common/http/client_test.cc +++ b/mobile/test/common/http/client_test.cc @@ -658,7 +658,8 @@ TEST_P(ClientTest, Encode100Continue) { TestResponseHeaderMapImpl response_headers{{":status", "200"}}; // Death tests are not supported on iOS. #ifndef TARGET_OS_IOS - EXPECT_DEATH(response_encoder_->encode1xxHeaders(response_headers), "panic: not implemented"); + EXPECT_ENVOY_BUG(response_encoder_->encode1xxHeaders(response_headers), + "Unexpected 100 continue"); #endif } @@ -688,7 +689,7 @@ TEST_P(ClientTest, EncodeMetadata) { metadata_map_vector.push_back(std::move(metadata_map_ptr)); // Death tests are not supported on iOS. #ifndef TARGET_OS_IOS - EXPECT_DEATH(response_encoder_->encodeMetadata(metadata_map_vector), "panic: not implemented"); + EXPECT_ENVOY_BUG(response_encoder_->encodeMetadata(metadata_map_vector), "Unexpected metadata"); #endif } diff --git a/mobile/test/common/integration/client_integration_test.cc b/mobile/test/common/integration/client_integration_test.cc index 37a3e94636cb..dbb3380c201b 100644 --- a/mobile/test/common/integration/client_integration_test.cc +++ b/mobile/test/common/integration/client_integration_test.cc @@ -314,6 +314,7 @@ void ClientIntegrationTest::trickleTest(bool final_chunk_has_data) { upstream_connection_->waitForNewStream(*BaseIntegrationTest::dispatcher_, upstream_request_)); ASSERT_TRUE(upstream_request_->waitForEndStream(*BaseIntegrationTest::dispatcher_)); + upstream_request_->encode1xxHeaders(Http::TestResponseHeaderMapImpl{{":status", "100"}}); upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, false); // This will be read immediately. on_data_ will kick off more chunks. upstream_request_->encodeData(1, false);