diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 50a058aa7961..65999f91b6c1 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -346,5 +346,28 @@ Utility::buildJitteredExponentialBackOffStrategy( default_base_interval_ms, default_base_interval_ms * 10, random); } +absl::Status Utility::validateTerminalFilters(const std::string& name, + const std::string& filter_type, + const std::string& filter_chain_type, + bool is_terminal_filter, + bool last_filter_in_current_config) { + if (is_terminal_filter && !last_filter_in_current_config) { + return absl::InvalidArgumentError( + fmt::format("Error: terminal filter named {} of type {} must be the " + "last filter in a {} filter chain.", + name, filter_type, filter_chain_type)); + } else if (!is_terminal_filter && last_filter_in_current_config) { + absl::string_view extra = ""; + if (filter_chain_type == "router upstream http") { + extra = " When upstream_http_filters are specified, they must explicitly end with an " + "UpstreamCodec filter."; + } + return absl::InvalidArgumentError(fmt::format("Error: non-terminal filter named {} of type " + "{} is the last filter in a {} filter chain.{}", + name, filter_type, filter_chain_type, extra)); + } + return absl::OkStatus(); +} + } // namespace Config } // namespace Envoy diff --git a/source/common/config/utility.h b/source/common/config/utility.h index ae9702984bd2..9467b753c7e6 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -424,19 +424,7 @@ class Utility { const std::string& filter_type, const std::string& filter_chain_type, bool is_terminal_filter, - bool last_filter_in_current_config) { - if (is_terminal_filter && !last_filter_in_current_config) { - return absl::InvalidArgumentError( - fmt::format("Error: terminal filter named {} of type {} must be the " - "last filter in a {} filter chain.", - name, filter_type, filter_chain_type)); - } else if (!is_terminal_filter && last_filter_in_current_config) { - return absl::InvalidArgumentError(fmt::format( - "Error: non-terminal filter named {} of type {} is the last filter in a {} filter chain.", - name, filter_type, filter_chain_type)); - } - return absl::OkStatus(); - } + bool last_filter_in_current_config); /** * Prepares the DNS failure refresh backoff strategy given the cluster configuration. diff --git a/source/common/quic/envoy_quic_client_session.h b/source/common/quic/envoy_quic_client_session.h index 65e69027bc58..8d3d1a8579a2 100644 --- a/source/common/quic/envoy_quic_client_session.h +++ b/source/common/quic/envoy_quic_client_session.h @@ -75,8 +75,7 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, // PacketsToReadDelegate size_t numPacketsExpectedPerEventLoop() const override { - // Do one round of reading per active stream, or to see if there's a new - // active stream. + // Do one round of reading per active stream, or to see if there's a new active stream. return std::max(1, GetNumActiveStreams()) * Network::NUM_DATAGRAMS_PER_RECEIVE; } diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 0b3ffdaefdc0..78b7c56537ce 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -478,7 +478,6 @@ class RetryPolicyImpl : public RetryPolicy { // that should be used when with this policy. std::vector> retry_host_predicate_configs_; - Upstream::RetryPrioritySharedPtr retry_priority_; // Name and config proto to use to create the RetryPriority to use with this policy. Default // initialized when no RetryPriority should be used. std::pair retry_priority_config_; diff --git a/test/common/config/BUILD b/test/common/config/BUILD index bc68571f940a..9111a9019602 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -144,6 +144,7 @@ envoy_cc_test( "//test/mocks/upstream:thread_local_cluster_mocks", "//test/test_common:environment_lib", "//test/test_common:logging_lib", + "//test/test_common:status_utility_lib", "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@com_github_cncf_xds//udpa/type/v1:pkg_cc_proto", diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index b3945df1e62d..98735c89c211 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -20,6 +20,7 @@ #include "test/mocks/upstream/thread_local_cluster.h" #include "test/test_common/environment.h" #include "test/test_common/logging.h" +#include "test/test_common/status_utility.h" #include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" @@ -29,6 +30,7 @@ #include "udpa/type/v1/typed_struct.pb.h" #include "xds/type/v3/typed_struct.pb.h" +using Envoy::StatusHelpers::HasStatusMessage; using testing::ContainsRegex; using testing::Eq; using testing::HasSubstr; @@ -894,6 +896,43 @@ TEST(UtilityTest, GetGrpcControlPlane) { } } +TEST(UtilityTest, ValidateTerminalFiltersSucceeds) { + EXPECT_OK(Utility::validateTerminalFilters("filter_name", "filter_type", "chain_type", + /*is_terminal_filter=*/true, + /*last_filter_in_current_config=*/true)); + EXPECT_OK(Utility::validateTerminalFilters("filter_name", "filter_type", "chain_type", + /*is_terminal_filter=*/false, + /*last_filter_in_current_config=*/false)); +} + +TEST(UtilityTest, ValidateTerminalFilterFailsWithMisplacedTerminalFilter) { + EXPECT_THAT( + Utility::validateTerminalFilters("filter_name", "filter_type", "chain_type", + /*is_terminal_filter=*/true, + /*last_filter_in_current_config=*/false), + HasStatusMessage("Error: terminal filter named filter_name of type filter_type must be the " + "last filter in a chain_type filter chain.")); +} + +TEST(UtilityTest, ValidateTerminalFilterFailsWithMissingTerminalFilter) { + EXPECT_THAT(Utility::validateTerminalFilters("filter_name", "filter_type", "chain_type", + /*is_terminal_filter=*/false, + /*last_filter_in_current_config=*/true), + HasStatusMessage("Error: non-terminal filter named filter_name of type " + "filter_type is the last filter in a chain_type filter chain.")); +} + +TEST(UtilityTest, ValidateTerminalFilterFailsWithMissingUpstreamTerminalFilter) { + EXPECT_THAT( + Utility::validateTerminalFilters("filter_name", "filter_type", "router upstream http", + /*is_terminal_filter=*/false, + /*last_filter_in_current_config=*/true), + HasStatusMessage("Error: non-terminal filter named filter_name of type " + "filter_type is the last filter in a router upstream http filter chain. " + "When upstream_http_filters are specified, they must explicitly end with an " + "UpstreamCodec filter.")); +} + } // namespace } // namespace Config } // namespace Envoy diff --git a/test/common/quic/envoy_quic_client_stream_test.cc b/test/common/quic/envoy_quic_client_stream_test.cc index e8fd682cadd6..6ae790c29d1d 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -210,6 +210,8 @@ TEST_F(EnvoyQuicClientStreamTest, GetRequestAndHeaderOnlyResponse) { const auto result = quic_stream_->encodeHeaders(request_headers_, /*end_stream=*/true); EXPECT_TRUE(result.ok()); + quic_stream_->setFlushTimeout(std::chrono::milliseconds(100)); // No-op + EXPECT_CALL(stream_decoder_, decodeHeaders_(_, /*end_stream=*/false)) .WillOnce(Invoke([](const Http::ResponseHeaderMapPtr& headers, bool) { EXPECT_EQ("200", headers->getStatusValue());