From 3ab38277ef0ce19c0f859339cb9af223c7cbaca2 Mon Sep 17 00:00:00 2001 From: Nigel Brittain <108375408+nbaws@users.noreply.github.com> Date: Wed, 12 Jun 2024 22:49:59 +1000 Subject: [PATCH 01/21] aws: sts and webidentity fix for sigv4a edge case (#34466) Signed-off-by: Nigel Brittain Signed-off-by: Nigel Brittain <108375408+nbaws@users.noreply.github.com> --- .../http_filters/_include/aws_credentials.rst | 17 +++++--- source/extensions/common/aws/utility.cc | 39 ++++++++++++++++--- test/extensions/common/aws/utility_test.cc | 18 +++++++++ 3 files changed, 63 insertions(+), 11 deletions(-) diff --git a/docs/root/configuration/http/http_filters/_include/aws_credentials.rst b/docs/root/configuration/http/http_filters/_include/aws_credentials.rst index 05d4eaaaf5d1..45f7266a69ca 100644 --- a/docs/root/configuration/http/http_filters/_include/aws_credentials.rst +++ b/docs/root/configuration/http/http_filters/_include/aws_credentials.rst @@ -17,11 +17,18 @@ secret access key (the session token is optional). ``SecretAccessKey``, and ``SessionToken`` are used, and credentials are cached for 1 hour or until they expire (according to the field ``Expiration``). To enable this credentials provider set ``envoy.reloadable_features.use_http_client_to_fetch_aws_credentials`` to ``true`` so that it can use http async client to fetch the credentials. This provider is not compatible with :ref:`Grpc Credentials AWS AwsIamConfig - ` plugin which can only support deprecated libcurl credentials fetcher - , see https://github.com/envoyproxy/envoy/pull/30626. To fetch the credentials a static cluster is required with the name - ``sts_token_service_internal`` pointing towards regional AWS Security Token Service. The static internal cluster will still be added even - if initially ``envoy.reloadable_features.use_http_client_to_fetch_aws_credentials`` is not set so that subsequently if the reloadable feature - is set to ``true`` the cluster config is available to fetch the credentials. + ` plugin which can only support deprecated libcurl credentials + fetcher (see `issue #30626 `_). To fetch the credentials a static cluster is created with the name + ``sts_token_service_internal-`` pointing towards regional AWS Security Token Service. + + Note: If ``signing_algorithm: AWS_SIGV4A`` is set, the logic for STS cluster host generation is as follows: + - If the ``region`` is configured (either through profile, environment or inline) as a SigV4A region set + - And if the first region in the region set contains a wildcard + - Then STS cluster host is set to ``sts.amazonaws.com`` (or ``sts-fips.us-east-1.amazonaws.com`` if compiled with FIPS support + - Else STS cluster host is set to ``sts..amazonaws.com`` + + If you require the use of SigV4A signing and you are using an alternate partition, such as cn or GovCloud, you can ensure correct generation + of the STS endpoint by setting the first region in your SigV4A region set to the correct region (such as ``cn-northwest-1`` with no wildcard) 4. Either EC2 instance metadata, ECS task metadata or EKS Pod Identity. For EC2 instance metadata, the fields ``AccessKeyId``, ``SecretAccessKey``, and ``Token`` are used, and credentials are cached for 1 hour. diff --git a/source/extensions/common/aws/utility.cc b/source/extensions/common/aws/utility.cc index 39594159df96..99988e40366b 100644 --- a/source/extensions/common/aws/utility.cc +++ b/source/extensions/common/aws/utility.cc @@ -233,20 +233,47 @@ Utility::joinCanonicalHeaderNames(const std::map& cano }); } +/** + * This function generates an STS Endpoint from a region string. + * If a SigV4A region set has been provided, it will use the first region in region set, and if + * that region still contains a wildcard, the STS Endpoint will be set to us-east-1 global endpoint + * (or FIPS if compiled for FIPS support) + */ std::string Utility::getSTSEndpoint(absl::string_view region) { - if (region == "cn-northwest-1" || region == "cn-north-1") { - return fmt::format("sts.{}.amazonaws.com.cn", region); + std::string single_region; + + // If we contain a comma or asterisk it looks like a region set. + if (absl::StrContains(region, ",") || (absl::StrContains(region, "*"))) { + // Use the first element from a region set if we have multiple regions specified. + const std::vector region_v = absl::StrSplit(region, ','); + // If we still have a * in the first element, then send them to us-east-1 fips or global + // endpoint. + if (absl::StrContains(region_v[0], '*')) { +#ifdef ENVOY_SSL_FIPS + return "sts-fips.us-east-1.amazonaws.com"; +#else + return "sts.amazonaws.com"; +#endif + } + single_region = region_v[0]; + } else { + // Otherwise it's a standard region, so use that. + single_region = region; + } + + if (single_region == "cn-northwest-1" || single_region == "cn-north-1") { + return fmt::format("sts.{}.amazonaws.com.cn", single_region); } #ifdef ENVOY_SSL_FIPS // Use AWS STS FIPS endpoints in FIPS mode https://docs.aws.amazon.com/general/latest/gr/sts.html. // Note: AWS GovCloud doesn't have separate fips endpoints. // TODO(suniltheta): Include `ca-central-1` when sts supports a dedicated FIPS endpoint. - if (region == "us-east-1" || region == "us-east-2" || region == "us-west-1" || - region == "us-west-2") { - return fmt::format("sts-fips.{}.amazonaws.com", region); + if (single_region == "us-east-1" || single_region == "us-east-2" || + single_region == "us-west-1" || single_region == "us-west-2") { + return fmt::format("sts-fips.{}.amazonaws.com", single_region); } #endif - return fmt::format("sts.{}.amazonaws.com", region); + return fmt::format("sts.{}.amazonaws.com", single_region); } static size_t curlCallback(char* ptr, size_t, size_t nmemb, void* data) { diff --git a/test/extensions/common/aws/utility_test.cc b/test/extensions/common/aws/utility_test.cc index f11c2e1db8ff..936ba14a130d 100644 --- a/test/extensions/common/aws/utility_test.cc +++ b/test/extensions/common/aws/utility_test.cc @@ -488,6 +488,24 @@ TEST(UtilityTest, GetGovCloudSTSEndpoints) { EXPECT_EQ("sts.us-gov-west-1.amazonaws.com", Utility::getSTSEndpoint("us-gov-west-1")); } +// Test edge case where a SigV4a region set is provided and also web identity provider is in use +TEST(UtilityTest, CorrectlyConvertRegionSet) { +#ifdef ENVOY_SSL_FIPS + EXPECT_EQ("sts-fips.us-east-1.amazonaws.com", Utility::getSTSEndpoint("*")); + EXPECT_EQ("sts-fips.us-east-1.amazonaws.com", Utility::getSTSEndpoint("*,ap-southeast-2")); + EXPECT_EQ("sts-fips.us-east-1.amazonaws.com", + Utility::getSTSEndpoint("ca-central-*,ap-southeast-2")); +#else + EXPECT_EQ("sts.amazonaws.com", Utility::getSTSEndpoint("*")); + EXPECT_EQ("sts.amazonaws.com", Utility::getSTSEndpoint("*,ap-southeast-2")); + EXPECT_EQ("sts.amazonaws.com", Utility::getSTSEndpoint("ca-central-*,ap-southeast-2")); +#endif + EXPECT_EQ("sts.ap-southeast-2.amazonaws.com", + Utility::getSTSEndpoint("ap-southeast-2,us-east-2")); + EXPECT_EQ("sts.ca-central-1.amazonaws.com", + Utility::getSTSEndpoint("ca-central-1,ap-southeast-2,eu-central-1")); +} + TEST(UtilityTest, JsonStringFound) { auto test_json = Json::Factory::loadFromStringNoThrow("{\"access_key_id\":\"testvalue\"}"); EXPECT_TRUE(test_json.ok()); From 6f78c2571bf55693372aee7e415d03fe6cfc1362 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 12 Jun 2024 08:53:30 -0400 Subject: [PATCH 02/21] Tcp tunneling: adding a test (#34670) Signed-off-by: Alyssa Wilk --- .../tcp_tunneling_integration_test.cc | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/integration/tcp_tunneling_integration_test.cc b/test/integration/tcp_tunneling_integration_test.cc index ecac330e1c1f..74dab5f08256 100644 --- a/test/integration/tcp_tunneling_integration_test.cc +++ b/test/integration/tcp_tunneling_integration_test.cc @@ -171,6 +171,48 @@ TEST_P(ConnectTerminationIntegrationTest, Basic) { sendBidirectionalDataAndCleanShutdown(); } +TEST_P(ConnectTerminationIntegrationTest, ManyStreams) { + if (downstream_protocol_ == Http::CodecType::HTTP1) { + // Resetting an individual stream requires HTTP/2 or later. + return; + } + autonomous_upstream_ = true; // Sending raw HTTP/1.1 + setUpstreamProtocol(Http::CodecType::HTTP1); + initialize(); + + auto upstream = reinterpret_cast(fake_upstreams_.front().get()); + upstream->setResponseHeaders(std::make_unique( + Http::TestResponseHeaderMapImpl({{":status", "200"}, {"content-length", "0"}}))); + upstream->setResponseBody(""); + std::string response_body = "HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n"; + codec_client_ = makeHttpConnection(lookupPort("http")); + + std::vector encoders; + std::vector responses; + const int num_loops = 50; + + // Do 2x loops and reset half the streams to fuzz lifetime issues + for (int i = 0; i < num_loops * 2; ++i) { + auto encoder_decoder = codec_client_->startRequest(connect_headers_); + if (i % 2 == 0) { + codec_client_->sendReset(encoder_decoder.first); + } else { + encoders.push_back(&encoder_decoder.first); + responses.push_back(std::move(encoder_decoder.second)); + } + } + + // Finish up the non-reset streams. The autonomous upstream will send the response. + for (int i = 0; i < num_loops; ++i) { + // Send some data upstream. + codec_client_->sendData(*encoders[i], "GET / HTTP/1.1\r\nHost: www.google.com\r\n\r\n", false); + responses[i]->waitForBodyData(response_body.length()); + EXPECT_EQ(response_body, responses[i]->body()); + } + + codec_client_->close(); +} + TEST_P(ConnectTerminationIntegrationTest, LogOnSuccessfulTunnel) { config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& From 40e116946d2250b19ec9e2b9888cac8f114056fb Mon Sep 17 00:00:00 2001 From: Raven Black Date: Wed, 12 Jun 2024 11:01:26 -0400 Subject: [PATCH 03/21] Make trie structure less memory-hogging (#33668) * Make trie structure less memory-hogging Signed-off-by: Raven Black --- bazel/test_for_benchmark_wrapper.sh | 2 +- source/common/common/utility.h | 115 ++++++++++++++---- test/common/common/BUILD | 17 +++ test/common/common/utility_test.cc | 11 ++ test/common/common/utility_trie_speed_test.cc | 89 ++++++++++++++ 5 files changed, 212 insertions(+), 22 deletions(-) create mode 100644 test/common/common/utility_trie_speed_test.cc diff --git a/bazel/test_for_benchmark_wrapper.sh b/bazel/test_for_benchmark_wrapper.sh index f5086aa41c7d..b644d82b5ed5 100755 --- a/bazel/test_for_benchmark_wrapper.sh +++ b/bazel/test_for_benchmark_wrapper.sh @@ -3,4 +3,4 @@ # Set the benchmark time to 0 to just verify that the benchmark runs to # completion. We're interacting with two different flag parsers, so the order # of flags and the -- matters. -"${TEST_SRCDIR}/envoy/${1}" "${@:2}" --skip_expensive_benchmarks -- --benchmark_min_time=0 +"${TEST_SRCDIR}/envoy/${1}" "${@:2}" --skip_expensive_benchmarks -- --benchmark_min_time=0s diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 4a07b8c4467a..02d851d5d9c9 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -636,16 +636,85 @@ class WelfordStandardDeviation { double m2_{0}; }; -template struct TrieEntry { - Value value_{}; - std::array, 256> entries_; -}; - /** * A trie used for faster lookup with lookup time at most equal to the size of the key. + * + * Type of Value must be empty-constructible and moveable, e.g. smart pointers and POD types. */ -template struct TrieLookupTable { +template class TrieLookupTable { + // A TrieNode aims to be a good balance of performant and + // space-efficient, by allocating a vector the size of the range of children + // the node contains. This should be good for most use-cases. + // + // For example, a node with children 'a' and 'z' will contain a vector of + // size 26, containing two values and 24 nulls. A node with only one + // child will contain a vector of size 1. A node with no children will + // contain an empty vector. + // + // Compared to allocating 256 entries for every node, this makes insertions + // a little bit inefficient (especially insertions in reverse order), but + // trie lookups remain O(length-of-longest-matching-prefix) with just a + // couple of very cheap operations extra per step. + // + // By size, having 256 entries for every node makes each node's overhead + // (excluding values) consume 8KB; even a trie containing only a single + // prefix "foobar" consumes 56KB. + // Using ranged vectors like this makes a single prefix "foobar" consume + // less than 20 bytes per node, for a total of 0.14KB. + class TrieNode { + public: + Value value_{}; + + // Returns a pointer to the child branch at child_index, or nullptr if + // there are no entries in the trie on that branch. + const TrieNode* operator[](uint8_t child_index) const { + if (child_index >= min_child_ && child_index < (min_child_ + children_.size())) { + return children_[child_index - min_child_].get(); + } + return nullptr; + } + + // Returns a pointer to the child branch at child_index, or nullptr if + // there are no entries in the trie on that branch. + TrieNode* operator[](uint8_t child_index) { + return const_cast(std::as_const(*this)[child_index]); + } + + // Populates a child branch at child_index, with the child specified + // by `node`. If the branch already exists it will be overwritten. + void set(uint8_t child_index, std::unique_ptr node) { + if (children_.empty()) { + children_.reserve(1); + children_.emplace_back(std::move(node)); + min_child_ = child_index; + return; + } + if (child_index < min_child_) { + // Expand the vector backwards, by moving the existing vector onto + // the end of a newly allocated one. + std::vector> new_children; + new_children.reserve(min_child_ - child_index + children_.size()); + new_children.resize(min_child_ - child_index); + std::move(children_.begin(), children_.end(), std::back_inserter(new_children)); + new_children[0] = std::move(node); + min_child_ = child_index; + children_ = std::move(new_children); + return; + } + if (child_index >= (min_child_ + children_.size())) { + // Expand the vector forwards. + children_.resize(child_index - min_child_ + 1); + // Fall through to "insert" behavior. + } + children_[child_index - min_child_] = std::move(node); + } + private: + uint8_t min_child_{0}; + std::vector> children_; + }; + +public: /** * Adds an entry to the Trie at the given Key. * @param key the key used to add the entry. @@ -655,12 +724,12 @@ template struct TrieLookupTable { * @return false when a value already exists for the given key. */ bool add(absl::string_view key, Value value, bool overwrite_existing = true) { - TrieEntry* current = &root_; + TrieNode* current = &root_; for (uint8_t c : key) { - if (!current->entries_[c]) { - current->entries_[c] = std::make_unique>(); + if ((*current)[c] == nullptr) { + current->set(c, std::make_unique()); } - current = current->entries_[c].get(); + current = (*current)[c]; } if (current->value_ && !overwrite_existing) { return false; @@ -672,31 +741,34 @@ template struct TrieLookupTable { /** * Finds the entry associated with the key. * @param key the key used to find. - * @return the value associated with the key. + * @return the Value associated with the key, or an empty-initialized Value + * if there is no matching key. */ Value find(absl::string_view key) const { - const TrieEntry* current = &root_; + const TrieNode* current = &root_; for (uint8_t c : key) { - current = current->entries_[c].get(); + current = (*current)[c]; if (current == nullptr) { - return nullptr; + return {}; } } return current->value_; } /** - * Finds the entry associated with the longest prefix. Complexity is O(min(longest key prefix, - * key length)). + * Finds the entry with the longest key that is a prefix of the specified key. + * Complexity is O(min(longest key prefix, key length)). * @param key the key used to find. - * @return the value matching the longest prefix based on the key. + * @return a value whose key is a prefix of the specified key. If there are + * multiple such values, the one with the longest key. If there are + * no keys that are a prefix of the input key, an empty-initialized Value. */ Value findLongestPrefix(absl::string_view key) const { - const TrieEntry* current = &root_; - const TrieEntry* result = current; + const TrieNode* current = &root_; + const TrieNode* result = current; for (uint8_t c : key) { - current = current->entries_[c].get(); + current = (*current)[c]; if (current == nullptr) { return result ? result->value_ : nullptr; @@ -707,7 +779,8 @@ template struct TrieLookupTable { return result ? result->value_ : nullptr; } - TrieEntry root_; +private: + TrieNode root_; }; /** diff --git a/test/common/common/BUILD b/test/common/common/BUILD index f5876ec08f19..2e4d4251a606 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -392,6 +392,23 @@ envoy_benchmark_test( benchmark_binary = "utility_speed_test", ) +envoy_cc_benchmark_binary( + name = "utility_trie_speed_test", + srcs = ["utility_trie_speed_test.cc"], + external_deps = [ + "abseil_strings", + "benchmark", + ], + deps = [ + "//source/common/common:utility_lib", + ], +) + +envoy_benchmark_test( + name = "utility_trie_speed_test_benchmark_test", + benchmark_binary = "utility_trie_speed_test", +) + envoy_cc_test( name = "lock_guard_test", srcs = ["lock_guard_test.cc"], diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 5b0a8b47530c..6b8d18420dca 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -1083,11 +1083,16 @@ TEST(TrieLookupTable, LongestPrefix) { const char* cstr_b = "b"; const char* cstr_c = "c"; const char* cstr_d = "d"; + const char* cstr_e = "e"; + const char* cstr_f = "f"; EXPECT_TRUE(trie.add("foo", cstr_a)); EXPECT_TRUE(trie.add("bar", cstr_b)); EXPECT_TRUE(trie.add("baro", cstr_c)); EXPECT_TRUE(trie.add("foo/bar", cstr_d)); + // Verify that prepending and appending branches to a node both work. + EXPECT_TRUE(trie.add("barn", cstr_e)); + EXPECT_TRUE(trie.add("barp", cstr_f)); EXPECT_EQ(cstr_a, trie.find("foo")); EXPECT_EQ(cstr_a, trie.findLongestPrefix("foo")); @@ -1101,6 +1106,12 @@ TEST(TrieLookupTable, LongestPrefix) { EXPECT_EQ(cstr_b, trie.findLongestPrefix("baritone")); EXPECT_EQ(cstr_c, trie.findLongestPrefix("barometer")); + EXPECT_EQ(cstr_e, trie.find("barn")); + EXPECT_EQ(cstr_e, trie.findLongestPrefix("barnacle")); + + EXPECT_EQ(cstr_f, trie.find("barp")); + EXPECT_EQ(cstr_f, trie.findLongestPrefix("barpomus")); + EXPECT_EQ(nullptr, trie.find("toto")); EXPECT_EQ(nullptr, trie.findLongestPrefix("toto")); EXPECT_EQ(nullptr, trie.find(" ")); diff --git a/test/common/common/utility_trie_speed_test.cc b/test/common/common/utility_trie_speed_test.cc new file mode 100644 index 000000000000..4046f10a7643 --- /dev/null +++ b/test/common/common/utility_trie_speed_test.cc @@ -0,0 +1,89 @@ +// Note: this should be run with --compilation_mode=opt, and would benefit from a +// quiescent system with disabled cstate power management. + +#include + +#include "envoy/http/header_map.h" + +#include "source/common/common/utility.h" +#include "source/common/http/headers.h" + +#include "benchmark/benchmark.h" + +namespace Envoy { + +// NOLINT(namespace-envoy) + +template +static void typedBmTrieLookups(benchmark::State& state, std::vector& keys) { + std::mt19937 prng(1); // PRNG with a fixed seed, for repeatability + std::uniform_int_distribution keyindex_distribution(0, keys.size() - 1); + TableType trie; + for (const std::string& key : keys) { + trie.add(key, nullptr); + } + std::vector key_selections; + for (size_t i = 0; i < 1024; i++) { + key_selections.push_back(keyindex_distribution(prng)); + } + + // key_index indexes into key_selections which is a pre-selected + // random ordering of 1024 indexes into the existing keys. This + // way we read from all over the trie, without spending time during + // the performance test generating these random choices. + size_t key_index = 0; + for (auto _ : state) { + UNREFERENCED_PARAMETER(_); + auto v = trie.find(keys[key_selections[key_index++]]); + // Reset key_index to 0 whenever it reaches 1024. + key_index &= 1023; + benchmark::DoNotOptimize(v); + } +} + +// Range args are: +// 0 - num_keys +// 1 - key_length (0 is a special case that generates mixed-length keys) +template static void typedBmTrieLookups(benchmark::State& state) { + std::mt19937 prng(1); // PRNG with a fixed seed, for repeatability + int num_keys = state.range(0); + int key_length = state.range(1); + std::uniform_int_distribution char_distribution('a', 'z'); + std::uniform_int_distribution key_length_distribution(key_length == 0 ? 8 : key_length, + key_length == 0 ? 128 : key_length); + auto make_key = [&](size_t len) { + std::string ret; + for (size_t i = 0; i < len; i++) { + ret.push_back(char_distribution(prng)); + } + return ret; + }; + std::vector keys; + for (int i = 0; i < num_keys; i++) { + std::string key = make_key(key_length_distribution(prng)); + keys.push_back(std::move(key)); + } + typedBmTrieLookups(state, keys); +} + +static void bmTrieLookups(benchmark::State& s) { + typedBmTrieLookups>(s); +} + +#define ADD_HEADER_TO_KEYS(name) keys.emplace_back(Http::Headers::get().name); +static void bmTrieLookupsRequestHeaders(benchmark::State& s) { + std::vector keys; + INLINE_REQ_HEADERS(ADD_HEADER_TO_KEYS); + typedBmTrieLookups>(s, keys); +} +static void bmTrieLookupsResponseHeaders(benchmark::State& s) { + std::vector keys; + INLINE_RESP_HEADERS(ADD_HEADER_TO_KEYS); + typedBmTrieLookups>(s, keys); +} + +BENCHMARK(bmTrieLookupsRequestHeaders); +BENCHMARK(bmTrieLookupsResponseHeaders); +BENCHMARK(bmTrieLookups)->ArgsProduct({{10, 100, 1000, 10000}, {0, 8, 128}}); + +} // namespace Envoy From 4b7c0f0609794ba1a1644cf9b530a29210cae18b Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Wed, 12 Jun 2024 10:01:48 -0500 Subject: [PATCH 04/21] mobile: Add more coverage in fetch_client_test.cc (#34699) Add more coverage for fetch_client_test Signed-off-by: Fredy Wijaya --- .../examples/cc/fetch_client/fetch_client.cc | 33 ++++++++++++++----- .../examples/cc/fetch_client/fetch_client.h | 11 +++++-- mobile/test/cc/unit/BUILD | 3 ++ mobile/test/cc/unit/fetch_client_test.cc | 11 +++++-- 4 files changed, 45 insertions(+), 13 deletions(-) diff --git a/mobile/examples/cc/fetch_client/fetch_client.cc b/mobile/examples/cc/fetch_client/fetch_client.cc index f7123e94fadf..b81f6dc38f80 100644 --- a/mobile/examples/cc/fetch_client/fetch_client.cc +++ b/mobile/examples/cc/fetch_client/fetch_client.cc @@ -27,14 +27,19 @@ Fetch::Fetch() Envoy::Event::Libevent::Global::initialize(); } -void Fetch::fetch(const std::vector& urls) { +envoy_status_t Fetch::fetch(const std::vector& urls, + const std::vector& quic_hints) { absl::Notification engine_running; dispatcher_ = api_->allocateDispatcher("fetch_client"); Thread::ThreadPtr envoy_thread = api_->threadFactory().createThread( - [this, &engine_running]() -> void { runEngine(engine_running); }); + [this, &engine_running, &quic_hints]() -> void { runEngine(engine_running, quic_hints); }); engine_running.WaitForNotification(); + envoy_status_t status = ENVOY_SUCCESS; for (const absl::string_view url : urls) { - sendRequest(url); + status = sendRequest(url); + if (status == ENVOY_FAILURE) { + break; + } } dispatcher_->exit(); envoy_thread->join(); @@ -42,13 +47,14 @@ void Fetch::fetch(const std::vector& urls) { absl::MutexLock lock(&engine_mutex_); engine_->terminate(); } + return status; } -void Fetch::sendRequest(absl::string_view url_string) { +envoy_status_t Fetch::sendRequest(absl::string_view url_string) { Http::Utility::Url url; if (!url.initialize(url_string, /*is_connect_request=*/false)) { std::cerr << "Unable to parse url: '" << url_string << "'\n"; - return; + return ENVOY_FAILURE; } std::cout << "Fetching url: " << url.toString() << "\n"; @@ -58,6 +64,7 @@ void Fetch::sendRequest(absl::string_view url_string) { absl::MutexLock lock(&engine_mutex_); stream_prototype = engine_->streamClient()->newStreamPrototype(); } + envoy_status_t status = ENVOY_SUCCESS; EnvoyStreamCallbacks stream_callbacks; stream_callbacks.on_headers_ = [](const Http::ResponseHeaderMap& headers, bool /* end_stream */, envoy_stream_intel intel) { @@ -79,8 +86,10 @@ void Fetch::sendRequest(absl::string_view url_string) { << final_intel.stream_end_ms - final_intel.stream_start_ms << "ms\n"; request_finished.Notify(); }; - stream_callbacks.on_error_ = [&request_finished](const EnvoyError& error, envoy_stream_intel, - envoy_final_stream_intel final_intel) { + stream_callbacks.on_error_ = [&request_finished, &status](const EnvoyError& error, + envoy_stream_intel, + envoy_final_stream_intel final_intel) { + status = ENVOY_FAILURE; std::cerr << "Request failed after " << final_intel.stream_end_ms - final_intel.stream_start_ms << "ms with error message: " << error.message_ << "\n"; request_finished.Notify(); @@ -102,12 +111,20 @@ void Fetch::sendRequest(absl::string_view url_string) { stream->sendHeaders(std::move(headers), true); request_finished.WaitForNotification(); + return status; } -void Fetch::runEngine(absl::Notification& engine_running) { +void Fetch::runEngine(absl::Notification& engine_running, + const std::vector& quic_hints) { Platform::EngineBuilder engine_builder; engine_builder.setLogLevel(Logger::Logger::debug); engine_builder.setOnEngineRunning([&engine_running]() { engine_running.Notify(); }); + if (!quic_hints.empty()) { + engine_builder.enableHttp3(true); + for (const auto& quic_hint : quic_hints) { + engine_builder.addQuicHint(std::string(quic_hint), 443); + } + } { absl::MutexLock lock(&engine_mutex_); diff --git a/mobile/examples/cc/fetch_client/fetch_client.h b/mobile/examples/cc/fetch_client/fetch_client.h index 46aa9de920d1..ca1a16cbdaed 100644 --- a/mobile/examples/cc/fetch_client/fetch_client.h +++ b/mobile/examples/cc/fetch_client/fetch_client.h @@ -26,11 +26,16 @@ class Fetch { public: Fetch(); - void fetch(const std::vector& urls); + /** + * Sends requests to the specified URLs. When QUIC hints are not empty, HTTP/3 will be enabled. + */ + envoy_status_t fetch(const std::vector& urls, + const std::vector& quic_hints = {}); private: - void runEngine(absl::Notification& engine_running); - void sendRequest(absl::string_view url); + void runEngine(absl::Notification& engine_running, + const std::vector& quic_hints); + envoy_status_t sendRequest(absl::string_view url); Thread::MutexBasicLockable lock_; Logger::Context logging_context_; diff --git a/mobile/test/cc/unit/BUILD b/mobile/test/cc/unit/BUILD index 53ef9a35620e..80cfdd16c6f1 100644 --- a/mobile/test/cc/unit/BUILD +++ b/mobile/test/cc/unit/BUILD @@ -30,6 +30,9 @@ envoy_cc_test( envoy_cc_test( name = "fetch_client_test", srcs = ["fetch_client_test.cc"], + exec_properties = { + "dockerNetwork": "standard", + }, repository = "@envoy", deps = [ "//examples/cc/fetch_client:fetch_client_lib", diff --git a/mobile/test/cc/unit/fetch_client_test.cc b/mobile/test/cc/unit/fetch_client_test.cc index 396e6a58787c..6baa5abdd61c 100644 --- a/mobile/test/cc/unit/fetch_client_test.cc +++ b/mobile/test/cc/unit/fetch_client_test.cc @@ -11,9 +11,16 @@ namespace { // This test verifies that the fetch client is able to successfully // build and start the Envoy engine. It will panic if it is unable // to do so. -TEST(FetchClientTest, Foo) { +TEST(FetchClientTest, Http2) { Envoy::Fetch client; - client.fetch({"https://www.google.com/"}); + // TODO(fredyw): Connecting to www.google.com is currently broken with oghttp2. + ASSERT_EQ(client.fetch({"https://www.example.com/"}), ENVOY_SUCCESS); +} + +TEST(FetchClientTest, Http3) { + Envoy::Fetch client; + // TODO(fredyw): Connecting to www.google.com is currently broken with oghttp2. + ASSERT_EQ(client.fetch({"https://www.example.com/"}, {"www.example.com"}), ENVOY_SUCCESS); } } // namespace From 5fa66dde856270f347b1634c004f39f35e9bc4fd Mon Sep 17 00:00:00 2001 From: code Date: Wed, 12 Jun 2024 23:03:42 +0800 Subject: [PATCH 05/21] data source: clean up unnecessary method and correct the behavior of max_size (#34600) * data source: clean up unnecessary method and correct the behavior of max_size Signed-off-by: wbpcode * update test Signed-off-by: wbpcode * improve coverage Signed-off-by: wbpcode --------- Signed-off-by: wbpcode Co-authored-by: wbpcode --- source/common/config/datasource.cc | 15 +++--- source/common/router/config_impl.cc | 1 - source/common/router/config_utility.cc | 18 -------- source/common/router/config_utility.h | 14 ------ test/common/config/datasource_test.cc | 46 ++++++++++++++++++- test/common/router/config_impl_test.cc | 63 -------------------------- 6 files changed, 54 insertions(+), 103 deletions(-) diff --git a/source/common/config/datasource.cc b/source/common/config/datasource.cc index 8cb923f74aeb..14457b3bff28 100644 --- a/source/common/config/datasource.cc +++ b/source/common/config/datasource.cc @@ -120,14 +120,17 @@ absl::StatusOr DataSourceProvider::create(const ProtoData auto initial_data_or_error = read(source, allow_empty, api, max_size); RETURN_IF_STATUS_NOT_OK(initial_data_or_error); + // read() only validates the size of the file and does not check the size of inline data. + // We check the size of inline data here. + // TODO(wbpcode): consider moving this check to read() to avoid duplicate checks. + if (max_size > 0 && 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)); + } + if (!source.has_watched_directory() || source.specifier_case() != envoy::config::core::v3::DataSource::kFilename) { - 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())); } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 6384850d3b4b..9ba3f4da137b 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -574,7 +574,6 @@ RouteEntryImplBase::RouteEntryImplBase(const CommonVirtualHostSharedPtr& vhost, case_sensitive_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(route.match(), case_sensitive, true)) { 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, diff --git a/source/common/router/config_utility.cc b/source/common/router/config_utility.cc index 3281c3ed688b..4d0a6c393c2e 100644 --- a/source/common/router/config_utility.cc +++ b/source/common/router/config_utility.cc @@ -108,24 +108,6 @@ ConfigUtility::parseDirectResponseCode(const envoy::config::route::v3::Route& ro return {}; } -absl::StatusOr -ConfigUtility::parseDirectResponseBody(const envoy::config::route::v3::Route& route, Api::Api& api, - uint32_t max_body_size_bytes) { - if (!route.has_direct_response() || !route.direct_response().has_body()) { - return EMPTY_STRING; - } - const auto& body = route.direct_response().body(); - - auto body_or_error = Envoy::Config::DataSource::read(body, true, api, max_body_size_bytes); - RETURN_IF_STATUS_NOT_OK(body_or_error); - const std::string& string_body = body_or_error.value(); - if (string_body.length() > max_body_size_bytes) { - return absl::InvalidArgumentError(fmt::format("response body size is {} bytes; maximum is {}", - string_body.length(), max_body_size_bytes)); - } - return string_body; -} - Http::Code ConfigUtility::parseClusterNotFoundResponseCode( const envoy::config::route::v3::RouteAction::ClusterNotFoundResponseCode& code) { switch (code) { diff --git a/source/common/router/config_utility.h b/source/common/router/config_utility.h index f83f0d417f18..83f60406ee34 100644 --- a/source/common/router/config_utility.h +++ b/source/common/router/config_utility.h @@ -84,20 +84,6 @@ class ConfigUtility { static absl::optional parseDirectResponseCode(const envoy::config::route::v3::Route& route); - /** - * Returns the content of the response body to send with direct responses from a route or an - * error. - * @param route supplies the Route configuration. - * @param api reference to the Api object - * @param max_body_size_bytes supplies the maximum response body size in bytes. - * @return absl::optional the response body provided inline in the route's - * direct_response if specified, or the contents of the file named in the - * route's direct_response if specified, or an empty string otherwise. - */ - static absl::StatusOr - parseDirectResponseBody(const envoy::config::route::v3::Route& route, Api::Api& api, - uint32_t max_body_size_bytes); - /** * Returns the HTTP Status Code enum parsed from proto. * @param code supplies the ClusterNotFoundResponseCode enum. diff --git a/test/common/config/datasource_test.cc b/test/common/config/datasource_test.cc index 9c4ee31aa56c..b5bfcbceac5b 100644 --- a/test/common/config/datasource_test.cc +++ b/test/common/config/datasource_test.cc @@ -156,6 +156,50 @@ TEST(DataSourceTest, EmptyEnvironmentVariableTest) { #endif } +TEST(DataSourceTest, NotExistFileTest) { + envoy::config::core::v3::DataSource config; + TestEnvironment::createPath(TestEnvironment::temporaryPath("envoy_test")); + const std::string filename = TestEnvironment::temporaryPath("envoy_test/not_exist_file"); + + const std::string yaml = fmt::format(R"EOF( + filename: "{}" + )EOF", + filename); + TestUtility::loadFromYamlAndValidate(yaml, config); + + EXPECT_EQ(envoy::config::core::v3::DataSource::SpecifierCase::kFilename, config.specifier_case()); + EXPECT_EQ(config.filename(), filename); + Api::ApiPtr api = Api::createApiForTest(); + EXPECT_EQ(DataSource::read(config, false, *api, 555).status().message(), + fmt::format("file {} does not exist", filename)); +} + +TEST(DataSourceTest, EmptyFileTest) { + envoy::config::core::v3::DataSource config; + TestEnvironment::createPath(TestEnvironment::temporaryPath("envoy_test")); + const std::string filename = TestEnvironment::temporaryPath("envoy_test/empty_file"); + { + std::ofstream file(filename); + file.close(); + } + + const std::string yaml = fmt::format(R"EOF( + filename: "{}" + )EOF", + filename); + TestUtility::loadFromYamlAndValidate(yaml, config); + EXPECT_EQ(envoy::config::core::v3::DataSource::SpecifierCase::kFilename, config.specifier_case()); + EXPECT_EQ(config.filename(), filename); + + Api::ApiPtr api = Api::createApiForTest(); + + EXPECT_EQ(DataSource::read(config, false, *api, 555).status().message(), + fmt::format("file {} is empty", filename)); + + const auto file_data = DataSource::read(config, true, *api, 555).value(); + EXPECT_TRUE(file_data.empty()); +} + TEST(DataSourceProviderTest, NonFileDataSourceTest) { envoy::config::core::v3::DataSource config; TestEnvironment::createPath(TestEnvironment::temporaryPath("envoy_test")); @@ -176,7 +220,7 @@ TEST(DataSourceProviderTest, NonFileDataSourceTest) { NiceMock tls; auto provider_or_error = - DataSource::DataSourceProvider::create(config, *dispatcher, tls, *api, false, 15); + DataSource::DataSourceProvider::create(config, *dispatcher, tls, *api, false, 0); EXPECT_EQ(provider_or_error.value()->data(), "Hello, world!"); } diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 80fb82fb6a51..8b1dde569c22 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -7567,69 +7567,6 @@ TEST_F(ConfigUtilityTest, ParseResponseCode) { } } -TEST_F(ConfigUtilityTest, ParseDirectResponseBody) { - constexpr uint64_t MaxResponseBodySizeBytes = 4096; - envoy::config::route::v3::Route route; - EXPECT_EQ(EMPTY_STRING, - ConfigUtility::parseDirectResponseBody(route, *api_, MaxResponseBodySizeBytes).value()); - - route.mutable_direct_response()->mutable_body()->set_filename("missing_file"); - EXPECT_EQ(ConfigUtility::parseDirectResponseBody(route, *api_, MaxResponseBodySizeBytes) - .status() - .message(), - "file missing_file does not exist"); - - // The default max body size in bytes is 4096 (MaxResponseBodySizeBytes). - const std::string body(MaxResponseBodySizeBytes + 1, '*'); - std::string expected_message("response body size is 4097 bytes; maximum is 4096"); - route.mutable_direct_response()->mutable_body()->set_inline_string(body); - EXPECT_EQ(ConfigUtility::parseDirectResponseBody(route, *api_, MaxResponseBodySizeBytes) - .status() - .message(), - expected_message); - - // Change the max body size to 2048 (MaxResponseBodySizeBytes/2) bytes. - auto filename = TestEnvironment::writeStringToFileForTest("body", body); - route.mutable_direct_response()->mutable_body()->set_filename(filename); - expected_message = "file " + filename + " size is 4097 bytes; maximum is 2048"; - EXPECT_EQ(ConfigUtility::parseDirectResponseBody(route, *api_, MaxResponseBodySizeBytes / 2) - .status() - .message(), - expected_message); - - // Update the max body size to 4098 bytes (MaxResponseBodySizeBytes + 2), hence the parsing is - // successful. - EXPECT_EQ( - MaxResponseBodySizeBytes + 1, - ConfigUtility::parseDirectResponseBody(route, *api_, MaxResponseBodySizeBytes + 2)->size()); - route.mutable_direct_response()->mutable_body()->set_filename(EMPTY_STRING); - route.mutable_direct_response()->mutable_body()->set_inline_bytes(body); - EXPECT_EQ( - MaxResponseBodySizeBytes + 1, - ConfigUtility::parseDirectResponseBody(route, *api_, MaxResponseBodySizeBytes + 2)->size()); -} - -TEST_F(ConfigUtilityTest, ParseDirectResponseBodyWithEnv) { - constexpr uint64_t MaxResponseBodySizeBytes = 4096; - envoy::config::route::v3::Route route; - - const std::string body(MaxResponseBodySizeBytes + 1, '*'); - TestEnvironment::setEnvVar("ResponseBodyContents", body, 1); - Envoy::Cleanup cleanup([]() { TestEnvironment::unsetEnvVar("ResponseBodyContents"); }); - - route.mutable_direct_response()->mutable_body()->set_environment_variable("ResponseBodyContents"); - - std::string expected_message("response body size is 4097 bytes; maximum is 4096"); - - EXPECT_EQ(ConfigUtility::parseDirectResponseBody(route, *api_, MaxResponseBodySizeBytes) - .status() - .message(), - expected_message); - EXPECT_EQ( - MaxResponseBodySizeBytes + 1, - ConfigUtility::parseDirectResponseBody(route, *api_, MaxResponseBodySizeBytes + 2)->size()); -} - TEST_F(RouteConfigurationV2, RedirectCode) { const std::string yaml = R"EOF( virtual_hosts: From 56f532e8f13d5c9f4309339e7f6ed4169554c70c Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 12 Jun 2024 11:09:40 -0400 Subject: [PATCH 06/21] runtime: removing enable_zone_routing_different_zone_counts (#34688) Signed-off-by: Alyssa Wilk --- changelogs/current.yaml | 3 + source/common/runtime/runtime_features.cc | 1 - .../common/load_balancer_impl.cc | 13 ----- .../round_robin/round_robin_lb_test.cc | 58 ------------------- 4 files changed, 3 insertions(+), 72 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9eb60589c804..7eb49fe28bd0 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -183,6 +183,9 @@ removed_config_or_runtime: - area: http change: | Removed ``envoy.reloadable_features.use_cluster_cache_for_alt_protocols_filter`` runtime flag and lagacy code paths. +- area: load_balancing + change: | + Removed ``envoy.reloadable_features.enable_zone_routing_different_zone_counts`` runtime flag and legacy code paths. - area: http change: | Removed ``envoy.reloadable_features.proxy_status_upstream_request_timeout`` runtime flag and lagacy code paths. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 82f6cd2f950a..ff2b9e7c82cc 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -46,7 +46,6 @@ RUNTIME_GUARD(envoy_reloadable_features_edf_lb_locality_scheduler_init_fix); RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection); RUNTIME_GUARD(envoy_reloadable_features_enable_connect_udp_support); RUNTIME_GUARD(envoy_reloadable_features_enable_include_histograms); -RUNTIME_GUARD(envoy_reloadable_features_enable_zone_routing_different_zone_counts); RUNTIME_GUARD(envoy_reloadable_features_exclude_host_in_eds_status_draining); RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_change_http_status); RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_handle_empty_response); diff --git a/source/extensions/load_balancing_policies/common/load_balancer_impl.cc b/source/extensions/load_balancing_policies/common/load_balancer_impl.cc index ed6bee8a68ce..7b3e023c960c 100644 --- a/source/extensions/load_balancing_policies/common/load_balancer_impl.cc +++ b/source/extensions/load_balancing_policies/common/load_balancer_impl.cc @@ -665,19 +665,6 @@ bool ZoneAwareLoadBalancerBase::earlyExitNonLocalityRoutingNew() { return true; } - // If the runtime guard is not enabled, keep the old behavior of not performing locality routing - // if the number of localities in the local cluster is different from the number of localities - // in the upstream cluster. - // The lb_zone_number_differs stat is only relevant if the runtime guard is disabled, - // so it is only incremented in that case. - if (!Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.enable_zone_routing_different_zone_counts") && - host_set.healthyHostsPerLocality().get().size() != - localHostSet().healthyHostsPerLocality().get().size()) { - stats_.lb_zone_number_differs_.inc(); - return true; - } - // Do not perform locality routing for small clusters. const uint64_t min_cluster_size = runtime_.snapshot().getInteger(RuntimeMinClusterSize, min_cluster_size_); diff --git a/test/extensions/load_balancing_policies/round_robin/round_robin_lb_test.cc b/test/extensions/load_balancing_policies/round_robin/round_robin_lb_test.cc index 25ff046acb72..5a5c70e16925 100644 --- a/test/extensions/load_balancing_policies/round_robin/round_robin_lb_test.cc +++ b/test/extensions/load_balancing_policies/round_robin/round_robin_lb_test.cc @@ -1036,64 +1036,6 @@ TEST_P(RoundRobinLoadBalancerTest, ZoneAwareDifferentZoneSize) { } } -TEST_P(RoundRobinLoadBalancerTest, NoZoneAwareDifferentZoneSizeDisabled) { - if (&hostSet() == &failover_host_set_) { // P = 1 does not support zone-aware routing. - return; - } - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues( - {{"envoy.reloadable_features.enable_zone_routing_different_zone_counts", "false"}}); - - envoy::config::core::v3::Locality zone_a; - zone_a.set_zone("A"); - envoy::config::core::v3::Locality zone_b; - zone_b.set_zone("B"); - envoy::config::core::v3::Locality zone_c; - zone_c.set_zone("C"); - - HostVectorSharedPtr upstream_hosts( - new HostVector({makeTestHost(info_, "tcp://127.0.0.1:80", simTime(), zone_a), - makeTestHost(info_, "tcp://127.0.0.1:81", simTime(), zone_b), - makeTestHost(info_, "tcp://127.0.0.1:82", simTime(), zone_c)})); - HostVectorSharedPtr local_hosts( - new HostVector({makeTestHost(info_, "tcp://127.0.0.1:0", simTime(), zone_a), - makeTestHost(info_, "tcp://127.0.0.1:1", simTime(), zone_b)})); - HostsPerLocalitySharedPtr upstream_hosts_per_locality = - makeHostsPerLocality({{makeTestHost(info_, "tcp://127.0.0.1:81", simTime(), zone_b)}, - {makeTestHost(info_, "tcp://127.0.0.1:80", simTime(), zone_a)}, - {makeTestHost(info_, "tcp://127.0.0.1:82", simTime(), zone_c)}}); - HostsPerLocalitySharedPtr local_hosts_per_locality = - makeHostsPerLocality({{makeTestHost(info_, "tcp://127.0.0.1:1", simTime(), zone_b)}, - {makeTestHost(info_, "tcp://127.0.0.1:0", simTime(), zone_a)}}); - - hostSet().healthy_hosts_ = *upstream_hosts; - hostSet().hosts_ = *upstream_hosts; - hostSet().healthy_hosts_per_locality_ = upstream_hosts_per_locality; - common_config_.mutable_healthy_panic_threshold()->set_value(100); - common_config_.mutable_zone_aware_lb_config()->mutable_routing_enabled()->set_value(98); - common_config_.mutable_zone_aware_lb_config()->mutable_min_cluster_size()->set_value(3); - init(true); - updateHosts(local_hosts, local_hosts_per_locality); - - EXPECT_CALL(runtime_.snapshot_, getInteger("upstream.healthy_panic_threshold", 100)) - .WillRepeatedly(Return(50)); - EXPECT_CALL(runtime_.snapshot_, featureEnabled("upstream.zone_routing.enabled", 98)) - .WillRepeatedly(Return(true)); - EXPECT_CALL(runtime_.snapshot_, getInteger("upstream.zone_routing.min_cluster_size", 3)) - .WillRepeatedly(Return(3)); - - // Zone-aware routing should be disabled because the local zone and upstream zone count - // are different and the runtime feature flag is disabled. - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[0], lb_->chooseHost(nullptr)); - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr)); - EXPECT_EQ(0U, stats_.lb_zone_routing_all_directly_.value()); - EXPECT_EQ(0U, stats_.lb_zone_routing_sampled_.value()); - EXPECT_EQ(0U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_EQ(1U, stats_.lb_zone_number_differs_.value()); -} - TEST_P(RoundRobinLoadBalancerTest, ZoneAwareRoutingLargeZoneSwitchOnOff) { if (&hostSet() == &failover_host_set_) { // P = 1 does not support zone-aware routing. return; From 814b6b2a282972bc690efdeb07d2942ab5ab3922 Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Wed, 12 Jun 2024 11:40:35 -0500 Subject: [PATCH 07/21] mobile: Rename setRuntimeGuard to addRuntimeGuard (#34711) Calling it addRuntimeGuard makes more sense since we can add multiple runtime guards. This PR also adds comments related to what the guard name is. Risk Level: low (refactoring) Testing: CI Docs Changes: n/a Release Notes: n/a Platform Specific Features: mobile Signed-off-by: Fredy Wijaya --- mobile/docs/root/api/starting_envoy.rst | 6 +++--- mobile/library/cc/engine_builder.cc | 2 +- mobile/library/cc/engine_builder.h | 6 ++++-- .../chromium/net/impl/NativeCronvoyEngineBuilderImpl.java | 8 +++----- mobile/library/jni/jni_impl.cc | 2 +- .../kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt | 5 +++-- mobile/library/objective-c/EnvoyConfiguration.mm | 2 +- mobile/library/swift/EngineBuilder.swift | 6 ++++-- mobile/test/cc/unit/envoy_config_test.cc | 8 ++++---- mobile/test/common/integration/client_integration_test.cc | 6 +++--- .../kotlin/io/envoyproxy/envoymobile/EngineBuilderTest.kt | 4 ++-- mobile/test/swift/EngineBuilderTests.swift | 6 +++--- mobile/test/swift/integration/KeyValueStoreTest.swift | 2 +- 13 files changed, 33 insertions(+), 30 deletions(-) diff --git a/mobile/docs/root/api/starting_envoy.rst b/mobile/docs/root/api/starting_envoy.rst index 6a8d498f8dd4..de8daa348b3a 100644 --- a/mobile/docs/root/api/starting_envoy.rst +++ b/mobile/docs/root/api/starting_envoy.rst @@ -490,7 +490,7 @@ A maximum of 100 entries will be stored. ~~~~~~~~~~~~~~~~~~~~~~~~~~ -``setRuntimeGuard`` +``addRuntimeGuard`` ~~~~~~~~~~~~~~~~~~~~~~~~~~ Adds a runtime guard key value pair to envoy configuration. The guard is of the short form "feature" @@ -500,10 +500,10 @@ Note that Envoy will fail to start up in debug mode if an unknown guard is speci **Example**:: // Kotlin - builder.setRuntimeGuard("feature", true) + builder.addRuntimeGuard("feature", true) // Swift - builder.setRuntimeGuard("feature", true) + builder.addRuntimeGuard("feature", true) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``setXds`` diff --git a/mobile/library/cc/engine_builder.cc b/mobile/library/cc/engine_builder.cc index 515d0000cf59..758cd5190396 100644 --- a/mobile/library/cc/engine_builder.cc +++ b/mobile/library/cc/engine_builder.cc @@ -410,7 +410,7 @@ EngineBuilder& EngineBuilder::addPlatformFilter(const std::string& name) { return *this; } -EngineBuilder& EngineBuilder::setRuntimeGuard(std::string guard, bool value) { +EngineBuilder& EngineBuilder::addRuntimeGuard(std::string guard, bool value) { runtime_guards_.emplace_back(std::move(guard), value); return *this; } diff --git a/mobile/library/cc/engine_builder.h b/mobile/library/cc/engine_builder.h index cc8a27f25805..0206fc079103 100644 --- a/mobile/library/cc/engine_builder.h +++ b/mobile/library/cc/engine_builder.h @@ -188,8 +188,10 @@ class EngineBuilder { EngineBuilder& addNativeFilter(std::string name, std::string typed_config); EngineBuilder& addPlatformFilter(const std::string& name); - - EngineBuilder& setRuntimeGuard(std::string guard, bool value); + // Adds a runtime guard for the `envoy.reloadable_features.`. + // For example if the runtime guard is `envoy.reloadable_features.use_foo`, the guard name is + // `use_foo`. + EngineBuilder& addRuntimeGuard(std::string guard, bool value); // These functions don't affect the Bootstrap configuration but instead perform registrations. EngineBuilder& addKeyValueStore(std::string name, KeyValueStoreSharedPtr key_value_store); diff --git a/mobile/library/java/org/chromium/net/impl/NativeCronvoyEngineBuilderImpl.java b/mobile/library/java/org/chromium/net/impl/NativeCronvoyEngineBuilderImpl.java index c15f891f013f..ce2045401cbd 100644 --- a/mobile/library/java/org/chromium/net/impl/NativeCronvoyEngineBuilderImpl.java +++ b/mobile/library/java/org/chromium/net/impl/NativeCronvoyEngineBuilderImpl.java @@ -158,16 +158,14 @@ public NativeCronvoyEngineBuilderImpl setStreamIdleTimeoutSeconds(int timeout) { } /** - * Sets the boolean value for the reloadable runtime feature flag value. For example, to set the + * Adds the boolean value for the reloadable runtime feature flag value. For example, to set the * Envoy runtime flag `envoy.reloadable_features.http_allow_partial_urls_in_referer` to true, - * call `setRuntimeGuard("http_allow_partial_urls_in_referer", true)`. - * - * TODO(abeyad): Change the name to setRuntimeFeature here and in the C++ APIs. + * call `addRuntimeGuard("http_allow_partial_urls_in_referer", true)`. * * @param feature The reloadable runtime feature flag name. * @param value The Boolean value to set the runtime feature flag to. */ - public NativeCronvoyEngineBuilderImpl setRuntimeGuard(String feature, boolean value) { + public NativeCronvoyEngineBuilderImpl addRuntimeGuard(String feature, boolean value) { mRuntimeGuards.put(feature, value); return this; } diff --git a/mobile/library/jni/jni_impl.cc b/mobile/library/jni/jni_impl.cc index 1cd9b8923e4e..35da3faf067f 100644 --- a/mobile/library/jni/jni_impl.cc +++ b/mobile/library/jni/jni_impl.cc @@ -1205,7 +1205,7 @@ void configureBuilder(Envoy::JNI::JniHelper& jni_helper, jlong connect_timeout_s auto guards = javaObjectArrayToStringPairVector(jni_helper, runtime_guards); for (std::pair& entry : guards) { - builder.setRuntimeGuard(entry.first, entry.second == "true"); + builder.addRuntimeGuard(entry.first, entry.second == "true"); } auto filters = javaObjectArrayToStringPairVector(jni_helper, filter_chain); diff --git a/mobile/library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt b/mobile/library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt index 19d474c0796f..d62db835933a 100644 --- a/mobile/library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt +++ b/mobile/library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt @@ -631,13 +631,14 @@ open class EngineBuilder() { } /** - * Set a runtime guard with the provided value. + * Adds a runtime guard for the `envoy.reloadable_features.`. For example if the runtime + * guard is `envoy.reloadable_features.use_foo`, the guard name is `use_foo`. * * @param name the name of the runtime guard, e.g. test_feature_false. * @param value the value for the runtime guard. * @return This builder. */ - fun setRuntimeGuard(name: String, value: Boolean): EngineBuilder { + fun addRuntimeGuard(name: String, value: Boolean): EngineBuilder { this.runtimeGuards.put(name, value) return this } diff --git a/mobile/library/objective-c/EnvoyConfiguration.mm b/mobile/library/objective-c/EnvoyConfiguration.mm index b1176c5eb237..8285569e9a3d 100644 --- a/mobile/library/objective-c/EnvoyConfiguration.mm +++ b/mobile/library/objective-c/EnvoyConfiguration.mm @@ -208,7 +208,7 @@ - (instancetype)initWithConnectTimeoutSeconds:(UInt32)connectTimeoutSeconds for (NSString *key in self.runtimeGuards) { BOOL value = [[self.runtimeGuards objectForKey:key] isEqualToString:@"true"]; - builder.setRuntimeGuard([key toCXXString], value); + builder.addRuntimeGuard([key toCXXString], value); } builder.addConnectTimeoutSeconds(self.connectTimeoutSeconds); diff --git a/mobile/library/swift/EngineBuilder.swift b/mobile/library/swift/EngineBuilder.swift index 7eb9d22f9c66..fcadd8685fa1 100644 --- a/mobile/library/swift/EngineBuilder.swift +++ b/mobile/library/swift/EngineBuilder.swift @@ -553,14 +553,16 @@ open class EngineBuilder: NSObject { return self } - /// Set a runtime guard with the provided value. + // Adds a runtime guard for the `envoy.reloadable_features.`. + // For example if the runtime guard is `envoy.reloadable_features.use_foo`, the guard name is + // `use_foo`. /// /// - parameter name: the name of the runtime guard, e.g. test_feature_false. /// - parameter value: the value for the runtime guard. /// /// - returns: This builder. @discardableResult - public func setRuntimeGuard(_ name: String, _ value: Bool) -> Self { + public func addRuntimeGuard(_ name: String, _ value: Bool) -> Self { self.runtimeGuards[name] = value return self } diff --git a/mobile/test/cc/unit/envoy_config_test.cc b/mobile/test/cc/unit/envoy_config_test.cc index 026c801548a8..432ce250c2f8 100644 --- a/mobile/test/cc/unit/envoy_config_test.cc +++ b/mobile/test/cc/unit/envoy_config_test.cc @@ -83,7 +83,7 @@ TEST(TestConfig, ConfigIsApplied) { .addH2ConnectionKeepaliveTimeoutSeconds(333) .setAppVersion("1.2.3") .setAppId("1234-1234-1234") - .setRuntimeGuard("test_feature_false", true) + .addRuntimeGuard("test_feature_false", true) .enableDnsCache(true, /* save_interval_seconds */ 101) .addDnsPreresolveHostnames({"lyft.com", "google.com"}) .setForceAlwaysUsev6(true) @@ -129,8 +129,8 @@ TEST(TestConfig, ConfigIsApplied) { TEST(TestConfig, MultiFlag) { EngineBuilder engine_builder; - engine_builder.setRuntimeGuard("test_feature_false", true) - .setRuntimeGuard("test_feature_true", false); + engine_builder.addRuntimeGuard("test_feature_false", true) + .addRuntimeGuard("test_feature_true", false); std::unique_ptr bootstrap = engine_builder.generateBootstrap(); const std::string bootstrap_str = bootstrap->ShortDebugString(); @@ -409,7 +409,7 @@ TEST(TestConfig, XdsConfig) { TEST(TestConfig, MoveConstructor) { EngineBuilder engine_builder; - engine_builder.setRuntimeGuard("test_feature_false", true).enableGzipDecompression(false); + engine_builder.addRuntimeGuard("test_feature_false", true).enableGzipDecompression(false); std::unique_ptr bootstrap = engine_builder.generateBootstrap(); std::string bootstrap_str = bootstrap->ShortDebugString(); diff --git a/mobile/test/common/integration/client_integration_test.cc b/mobile/test/common/integration/client_integration_test.cc index 3d1f287c354e..c1255cb34a94 100644 --- a/mobile/test/common/integration/client_integration_test.cc +++ b/mobile/test/common/integration/client_integration_test.cc @@ -674,7 +674,7 @@ TEST_P(ClientIntegrationTest, InvalidDomainFakeResolver) { } TEST_P(ClientIntegrationTest, InvalidDomainReresolveWithNoAddresses) { - builder_.setRuntimeGuard("reresolve_null_addresses", true); + builder_.addRuntimeGuard("reresolve_null_addresses", true); Network::OverrideAddrInfoDnsResolverFactory factory; Registry::InjectFactory inject_factory(factory); Registry::InjectFactory::forceAllowDuplicates(); @@ -1333,8 +1333,8 @@ TEST_P(ClientIntegrationTest, DirectResponse) { } TEST_P(ClientIntegrationTest, TestRuntimeSet) { - builder_.setRuntimeGuard("test_feature_true", false); - builder_.setRuntimeGuard("test_feature_false", true); + builder_.addRuntimeGuard("test_feature_true", false); + builder_.addRuntimeGuard("test_feature_false", true); initialize(); // Verify that the Runtime config values are from the RTDS response. diff --git a/mobile/test/kotlin/io/envoyproxy/envoymobile/EngineBuilderTest.kt b/mobile/test/kotlin/io/envoyproxy/envoymobile/EngineBuilderTest.kt index 0ffab1b45b70..a8b841f2777b 100644 --- a/mobile/test/kotlin/io/envoyproxy/envoymobile/EngineBuilderTest.kt +++ b/mobile/test/kotlin/io/envoyproxy/envoymobile/EngineBuilderTest.kt @@ -217,8 +217,8 @@ class EngineBuilderTest { fun `specifying runtime guards work`() { engineBuilder = EngineBuilder() engineBuilder - .setRuntimeGuard("test_feature_false", true) - .setRuntimeGuard("test_feature_true", false) + .addRuntimeGuard("test_feature_false", true) + .addRuntimeGuard("test_feature_true", false) val engine = engineBuilder.build() as EngineImpl assertThat(engine.envoyConfiguration.runtimeGuards["test_feature_false"]).isEqualTo("true") assertThat(engine.envoyConfiguration.runtimeGuards["test_feature_true"]).isEqualTo("false") diff --git a/mobile/test/swift/EngineBuilderTests.swift b/mobile/test/swift/EngineBuilderTests.swift index b4a2ae7b5e71..31c649b94eb8 100644 --- a/mobile/test/swift/EngineBuilderTests.swift +++ b/mobile/test/swift/EngineBuilderTests.swift @@ -13,10 +13,10 @@ final class EngineBuilderTests: XCTestCase { MockEnvoyEngine.onRunWithConfig = nil } - func testSetRuntimeGuard() { + func testAddRuntimeGuard() { let bootstrapDebugDescription = EngineBuilder() - .setRuntimeGuard("test_feature_false", true) - .setRuntimeGuard("test_feature_true", false) + .addRuntimeGuard("test_feature_false", true) + .addRuntimeGuard("test_feature_true", false) .bootstrapDebugDescription() XCTAssertTrue( bootstrapDebugDescription.contains(#""test_feature_false" value { bool_value: true }"#) diff --git a/mobile/test/swift/integration/KeyValueStoreTest.swift b/mobile/test/swift/integration/KeyValueStoreTest.swift index 644b79e2dc01..b33204b2c52b 100644 --- a/mobile/test/swift/integration/KeyValueStoreTest.swift +++ b/mobile/test/swift/integration/KeyValueStoreTest.swift @@ -52,7 +52,7 @@ final class KeyValueStoreTests: XCTestCase { // swiftlint:disable:next line_length typedConfig: "[\(kvStoreType)]{ kv_store_name: 'envoy.key_value.platform_test', test_key: 'foo', test_value: 'bar'}" ) - .setRuntimeGuard("test_feature_false", true) + .addRuntimeGuard("test_feature_false", true) .build() let client = engine.streamClient() From 37648d1624b4161111816c4a1b979504c05ae633 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 12 Jun 2024 15:19:32 -0400 Subject: [PATCH 08/21] runtime: removing locality_routing_use_new_routing_logic (#34712) Signed-off-by: Alyssa Wilk --- changelogs/current.yaml | 3 + source/common/runtime/runtime_features.cc | 1 - .../common/load_balancer_impl.cc | 153 +-------- .../common/load_balancer_impl.h | 32 +- .../common/load_balancer_impl_base_test.cc | 4 +- .../common/load_balancer_impl_base_test.h | 1 - .../least_request/least_request_lb_test.cc | 6 +- .../random/random_lb_test.cc | 6 +- .../round_robin/round_robin_lb_test.cc | 320 ++++++------------ 9 files changed, 129 insertions(+), 397 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 7eb49fe28bd0..622f9c4c0e2a 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -186,6 +186,9 @@ removed_config_or_runtime: - area: load_balancing change: | Removed ``envoy.reloadable_features.enable_zone_routing_different_zone_counts`` runtime flag and legacy code paths. +- area: load_balancing + change: | + Removed ``envoy.reloadable_features.locality_routing_use_new_routing_logic`` runtime flag and legacy code paths. - area: http change: | Removed ``envoy.reloadable_features.proxy_status_upstream_request_timeout`` runtime flag and lagacy code paths. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index ff2b9e7c82cc..d4901982e08a 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -64,7 +64,6 @@ RUNTIME_GUARD(envoy_reloadable_features_http_filter_avoid_reentrant_local_reply) RUNTIME_GUARD(envoy_reloadable_features_http_reject_path_with_fragment); RUNTIME_GUARD(envoy_reloadable_features_http_route_connect_proxy_by_default); RUNTIME_GUARD(envoy_reloadable_features_immediate_response_use_filter_mutation_rule); -RUNTIME_GUARD(envoy_reloadable_features_locality_routing_use_new_routing_logic); RUNTIME_GUARD(envoy_reloadable_features_no_downgrade_to_canonical_name); RUNTIME_GUARD(envoy_reloadable_features_no_extension_lookup_by_name); RUNTIME_GUARD(envoy_reloadable_features_normalize_host_for_preresolve_dfp_dns); diff --git a/source/extensions/load_balancing_policies/common/load_balancer_impl.cc b/source/extensions/load_balancing_policies/common/load_balancer_impl.cc index 7b3e023c960c..96ef80bcd400 100644 --- a/source/extensions/load_balancing_policies/common/load_balancer_impl.cc +++ b/source/extensions/load_balancing_policies/common/load_balancer_impl.cc @@ -415,8 +415,6 @@ ZoneAwareLoadBalancerBase::ZoneAwareLoadBalancerBase( fail_traffic_on_panic_(locality_config.has_value() ? locality_config->zone_aware_lb_config().fail_traffic_on_panic() : false), - use_new_locality_routing_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.locality_routing_use_new_routing_logic")), locality_weighted_balancing_(locality_config.has_value() && locality_config->has_locality_weighted_lb_config()) { ASSERT(!priority_set.hostSetsPerPriority().empty()); @@ -428,11 +426,7 @@ ZoneAwareLoadBalancerBase::ZoneAwareLoadBalancerBase( // If P=0 changes, regenerate locality routing structures. Locality based routing is // disabled at all other levels. if (local_priority_set_ && priority == 0) { - if (use_new_locality_routing_) { - regenerateLocalityRoutingStructuresNew(); - } else { - regenerateLocalityRoutingStructures(); - } + regenerateLocalityRoutingStructures(); } return absl::OkStatus(); }); @@ -447,17 +441,13 @@ ZoneAwareLoadBalancerBase::ZoneAwareLoadBalancerBase( ASSERT(priority == 0); // If the set of local Envoys changes, regenerate routing for P=0 as it does priority // based routing. - if (use_new_locality_routing_) { - regenerateLocalityRoutingStructuresNew(); - } else { - regenerateLocalityRoutingStructures(); - } + regenerateLocalityRoutingStructures(); return absl::OkStatus(); }); } } -void ZoneAwareLoadBalancerBase::regenerateLocalityRoutingStructuresNew() { +void ZoneAwareLoadBalancerBase::regenerateLocalityRoutingStructures() { ASSERT(local_priority_set_); stats_.lb_recalculate_zone_structures_.inc(); // resizePerPriorityState should ensure these stay in sync. @@ -468,7 +458,7 @@ void ZoneAwareLoadBalancerBase::regenerateLocalityRoutingStructuresNew() { PerPriorityState& state = *per_priority_state_[priority]; // Do not perform any calculations if we cannot perform locality routing based on non runtime // params. - if (earlyExitNonLocalityRoutingNew()) { + if (earlyExitNonLocalityRouting()) { state.locality_routing_state_ = LocalityRoutingState::NoLocalityRouting; return; } @@ -488,7 +478,7 @@ void ZoneAwareLoadBalancerBase::regenerateLocalityRoutingStructuresNew() { // localities across priorities is not. const HostsPerLocality& localHostsPerLocality = localHostSet().healthyHostsPerLocality(); auto locality_percentages = - calculateLocalityPercentagesNew(localHostsPerLocality, upstreamHostsPerLocality); + calculateLocalityPercentages(localHostsPerLocality, upstreamHostsPerLocality); // If we have lower percent of hosts in the local cluster in the same locality, // we can push all of the requests directly to upstream cluster in the same locality. @@ -551,87 +541,6 @@ void ZoneAwareLoadBalancerBase::regenerateLocalityRoutingStructuresNew() { } } -void ZoneAwareLoadBalancerBase::regenerateLocalityRoutingStructures() { - ASSERT(local_priority_set_); - stats_.lb_recalculate_zone_structures_.inc(); - // resizePerPriorityState should ensure these stay in sync. - ASSERT(per_priority_state_.size() == priority_set_.hostSetsPerPriority().size()); - - // We only do locality routing for P=0 - uint32_t priority = 0; - PerPriorityState& state = *per_priority_state_[priority]; - // Do not perform any calculations if we cannot perform locality routing based on non runtime - // params. - if (earlyExitNonLocalityRouting()) { - state.locality_routing_state_ = LocalityRoutingState::NoLocalityRouting; - return; - } - HostSet& host_set = *priority_set_.hostSetsPerPriority()[priority]; - ASSERT(host_set.healthyHostsPerLocality().hasLocalLocality()); - const size_t num_localities = host_set.healthyHostsPerLocality().get().size(); - ASSERT(num_localities > 0); - - // It is worth noting that all of the percentages calculated are orthogonal from - // how much load this priority level receives, percentageLoad(priority). - // - // If the host sets are such that 20% of load is handled locally and 80% is residual, and then - // half the hosts in all host sets go unhealthy, this priority set will - // still send half of the incoming load to the local locality and 80% to residual. - // - // Basically, fairness across localities within a priority is guaranteed. Fairness across - // localities across priorities is not. - absl::FixedArray local_percentage(num_localities); - calculateLocalityPercentage(localHostSet().healthyHostsPerLocality(), local_percentage.begin()); - absl::FixedArray upstream_percentage(num_localities); - calculateLocalityPercentage(host_set.healthyHostsPerLocality(), upstream_percentage.begin()); - - // If we have lower percent of hosts in the local cluster in the same locality, - // we can push all of the requests directly to upstream cluster in the same locality. - if (upstream_percentage[0] >= local_percentage[0]) { - state.locality_routing_state_ = LocalityRoutingState::LocalityDirect; - return; - } - - state.locality_routing_state_ = LocalityRoutingState::LocalityResidual; - - // If we cannot route all requests to the same locality, calculate what percentage can be routed. - // For example, if local percentage is 20% and upstream is 10% - // we can route only 50% of requests directly. - state.local_percent_to_route_ = upstream_percentage[0] * 10000 / local_percentage[0]; - - // Local locality does not have additional capacity (we have already routed what we could). - // Now we need to figure out how much traffic we can route cross locality and to which exact - // locality we should route. Percentage of requests routed cross locality to a specific locality - // needed be proportional to the residual capacity upstream locality has. - // - // residual_capacity contains capacity left in a given locality, we keep accumulating residual - // capacity to make search for sampled value easier. - // For example, if we have the following upstream and local percentage: - // local_percentage: 40000 40000 20000 - // upstream_percentage: 25000 50000 25000 - // Residual capacity would look like: 0 10000 5000. Now we need to sample proportionally to - // bucket sizes (residual capacity). For simplicity of finding where specific - // sampled value is, we accumulate values in residual capacity. This is what it will look like: - // residual_capacity: 0 10000 15000 - // Now to find a locality to route (bucket) we could simply iterate over residual_capacity - // searching where sampled value is placed. - state.residual_capacity_.resize(num_localities); - - // Local locality (index 0) does not have residual capacity as we have routed all we could. - state.residual_capacity_[0] = 0; - for (size_t i = 1; i < num_localities; ++i) { - // Only route to the localities that have additional capacity. - if (upstream_percentage[i] > local_percentage[i]) { - state.residual_capacity_[i] = - state.residual_capacity_[i - 1] + upstream_percentage[i] - local_percentage[i]; - } else { - // Locality with index "i" does not have residual capacity, but we keep accumulating previous - // values to make search easier on the next step. - state.residual_capacity_[i] = state.residual_capacity_[i - 1]; - } - } -} - void ZoneAwareLoadBalancerBase::resizePerPriorityState() { const uint32_t size = priority_set_.hostSetsPerPriority().size(); while (per_priority_state_.size() < size) { @@ -640,7 +549,7 @@ void ZoneAwareLoadBalancerBase::resizePerPriorityState() { } } -bool ZoneAwareLoadBalancerBase::earlyExitNonLocalityRoutingNew() { +bool ZoneAwareLoadBalancerBase::earlyExitNonLocalityRouting() { // We only do locality routing for P=0. HostSet& host_set = *priority_set_.hostSetsPerPriority()[0]; if (host_set.healthyHostsPerLocality().get().size() < 2) { @@ -676,39 +585,6 @@ bool ZoneAwareLoadBalancerBase::earlyExitNonLocalityRoutingNew() { return false; } -bool ZoneAwareLoadBalancerBase::earlyExitNonLocalityRouting() { - // We only do locality routing for P=0. - HostSet& host_set = *priority_set_.hostSetsPerPriority()[0]; - if (host_set.healthyHostsPerLocality().get().size() < 2) { - return true; - } - - // lb_local_cluster_not_ok is bumped for "Local host set is not set or it is - // panic mode for local cluster". - if (!host_set.healthyHostsPerLocality().hasLocalLocality() || - host_set.healthyHostsPerLocality().get()[0].empty()) { - stats_.lb_local_cluster_not_ok_.inc(); - return true; - } - - // Same number of localities should be for local and upstream cluster. - if (host_set.healthyHostsPerLocality().get().size() != - localHostSet().healthyHostsPerLocality().get().size()) { - stats_.lb_zone_number_differs_.inc(); - return true; - } - - // Do not perform locality routing for small clusters. - const uint64_t min_cluster_size = - runtime_.snapshot().getInteger(RuntimeMinClusterSize, min_cluster_size_); - if (host_set.healthyHosts().size() < min_cluster_size) { - stats_.lb_zone_cluster_too_small_.inc(); - return true; - } - - return false; -} - HostConstSharedPtr ZoneAwareLoadBalancerBase::chooseHost(LoadBalancerContext* context) { HostConstSharedPtr host; @@ -746,7 +622,7 @@ bool LoadBalancerBase::isHostSetInPanic(const HostSet& host_set) const { } absl::FixedArray -ZoneAwareLoadBalancerBase::calculateLocalityPercentagesNew( +ZoneAwareLoadBalancerBase::calculateLocalityPercentages( const HostsPerLocality& local_hosts_per_locality, const HostsPerLocality& upstream_hosts_per_locality) { uint64_t total_local_hosts = 0; @@ -790,21 +666,6 @@ ZoneAwareLoadBalancerBase::calculateLocalityPercentagesNew( return percentages; } -void ZoneAwareLoadBalancerBase::calculateLocalityPercentage( - const HostsPerLocality& hosts_per_locality, uint64_t* ret) { - uint64_t total_hosts = 0; - for (const auto& locality_hosts : hosts_per_locality.get()) { - total_hosts += locality_hosts.size(); - } - - // TODO(snowp): Should we ignore excluded hosts here too? - - size_t i = 0; - for (const auto& locality_hosts : hosts_per_locality.get()) { - ret[i++] = total_hosts > 0 ? 10000ULL * locality_hosts.size() / total_hosts : 0; - } -} - uint32_t ZoneAwareLoadBalancerBase::tryChooseLocalLocalityHosts(const HostSet& host_set) const { PerPriorityState& state = *per_priority_state_[host_set.priority()]; ASSERT(state.locality_routing_state_ != LocalityRoutingState::NoLocalityRouting); diff --git a/source/extensions/load_balancing_policies/common/load_balancer_impl.h b/source/extensions/load_balancing_policies/common/load_balancer_impl.h index c9f8e46b68fc..d01e3f551203 100644 --- a/source/extensions/load_balancing_policies/common/load_balancer_impl.h +++ b/source/extensions/load_balancing_policies/common/load_balancer_impl.h @@ -367,15 +367,6 @@ class ZoneAwareLoadBalancerBase : public LoadBalancerBase { * @return decision on quick exit from locality aware routing based on cluster configuration. * This gets recalculated on update callback. */ - bool earlyExitNonLocalityRoutingNew(); - - /** - * @return decision on quick exit from locality aware routing based on cluster configuration. - * This gets recalculated on update callback. - * - * This is the legacy version of the function from previous versions of Envoy, kept temporarily - * as an alternate code-path to reduce the risk of changes. - */ bool earlyExitNonLocalityRouting(); /** @@ -390,30 +381,12 @@ class ZoneAwareLoadBalancerBase : public LoadBalancerBase { * matches the ordering of upstream localities in the input upstream_hosts_per_locality. */ absl::FixedArray - calculateLocalityPercentagesNew(const HostsPerLocality& local_hosts_per_locality, - const HostsPerLocality& upstream_hosts_per_locality); - - /** - * @return (number of hosts in a given locality)/(total number of hosts) in `ret` param. - * The result is stored as integer number and scaled by 10000 multiplier for better precision. - * Caller is responsible for allocation/de-allocation of `ret`. - * - * This is the legacy version of the function from previous versions of Envoy, kept temporarily - * as an alternate code-path to reduce the risk of changes. - */ - void calculateLocalityPercentage(const HostsPerLocality& hosts_per_locality, uint64_t* ret); + calculateLocalityPercentages(const HostsPerLocality& local_hosts_per_locality, + const HostsPerLocality& upstream_hosts_per_locality); /** * Regenerate locality aware routing structures for fast decisions on upstream locality selection. */ - void regenerateLocalityRoutingStructuresNew(); - - /** - * Regenerate locality aware routing structures for fast decisions on upstream locality selection. - * - * This is the legacy version of the function from previous versions of Envoy, kept temporarily - * as an alternate code-path to reduce the risk of changes. - */ void regenerateLocalityRoutingStructures(); HostSet& localHostSet() const { return *local_priority_set_->hostSetsPerPriority()[0]; } @@ -468,7 +441,6 @@ class ZoneAwareLoadBalancerBase : public LoadBalancerBase { // Keep small members (bools and enums) at the end of class, to reduce alignment overhead. const uint32_t routing_enabled_; const bool fail_traffic_on_panic_ : 1; - const bool use_new_locality_routing_ : 1; // If locality weight aware routing is enabled. const bool locality_weighted_balancing_ : 1; diff --git a/test/extensions/load_balancing_policies/common/load_balancer_impl_base_test.cc b/test/extensions/load_balancing_policies/common/load_balancer_impl_base_test.cc index de15d50286e4..8b3280fdbb25 100644 --- a/test/extensions/load_balancing_policies/common/load_balancer_impl_base_test.cc +++ b/test/extensions/load_balancing_policies/common/load_balancer_impl_base_test.cc @@ -84,8 +84,8 @@ class LoadBalancerBaseTest : public LoadBalancerTestBase { }; INSTANTIATE_TEST_SUITE_P(PrimaryOrFailoverAndLegacyOrNew, LoadBalancerBaseTest, - ::testing::Values(LoadBalancerTestParam{true, false}, - LoadBalancerTestParam{true, true})); + ::testing::Values(LoadBalancerTestParam{true}, + LoadBalancerTestParam{false})); // Basic test of host set selection. TEST_P(LoadBalancerBaseTest, PrioritySelection) { diff --git a/test/extensions/load_balancing_policies/common/load_balancer_impl_base_test.h b/test/extensions/load_balancing_policies/common/load_balancer_impl_base_test.h index 3252b4140056..6911aeb20955 100644 --- a/test/extensions/load_balancing_policies/common/load_balancer_impl_base_test.h +++ b/test/extensions/load_balancing_policies/common/load_balancer_impl_base_test.h @@ -70,7 +70,6 @@ class TestZoneAwareLoadBalancer : public ZoneAwareLoadBalancerBase { struct LoadBalancerTestParam { bool use_default_host_set; - bool use_new_locality_routing; }; class LoadBalancerTestBase : public Event::TestUsingSimulatedTime, diff --git a/test/extensions/load_balancing_policies/least_request/least_request_lb_test.cc b/test/extensions/load_balancing_policies/least_request/least_request_lb_test.cc index d5d3e11f2fc2..4f1f44723601 100644 --- a/test/extensions/load_balancing_policies/least_request/least_request_lb_test.cc +++ b/test/extensions/load_balancing_policies/least_request/least_request_lb_test.cc @@ -560,10 +560,8 @@ TEST_P(LeastRequestLoadBalancerTest, SlowStartWithActiveHC) { } INSTANTIATE_TEST_SUITE_P(PrimaryOrFailoverAndLegacyOrNew, LeastRequestLoadBalancerTest, - ::testing::Values(LoadBalancerTestParam{true, false}, - LoadBalancerTestParam{true, true}, - LoadBalancerTestParam{false, false}, - LoadBalancerTestParam{false, true})); + ::testing::Values(LoadBalancerTestParam{true}, + LoadBalancerTestParam{false})); } // namespace } // namespace Upstream diff --git a/test/extensions/load_balancing_policies/random/random_lb_test.cc b/test/extensions/load_balancing_policies/random/random_lb_test.cc index afd7b4ed3b94..117cdcf11cba 100644 --- a/test/extensions/load_balancing_policies/random/random_lb_test.cc +++ b/test/extensions/load_balancing_policies/random/random_lb_test.cc @@ -57,10 +57,8 @@ TEST_P(RandomLoadBalancerTest, FailClusterOnPanic) { } INSTANTIATE_TEST_SUITE_P(PrimaryOrFailoverAndLegacyOrNew, RandomLoadBalancerTest, - ::testing::Values(LoadBalancerTestParam{true, false}, - LoadBalancerTestParam{true, true}, - LoadBalancerTestParam{false, false}, - LoadBalancerTestParam{false, true})); + ::testing::Values(LoadBalancerTestParam{true}, + LoadBalancerTestParam{false})); } // namespace } // namespace Upstream diff --git a/test/extensions/load_balancing_policies/round_robin/round_robin_lb_test.cc b/test/extensions/load_balancing_policies/round_robin/round_robin_lb_test.cc index 5a5c70e16925..c522bd377a3f 100644 --- a/test/extensions/load_balancing_policies/round_robin/round_robin_lb_test.cc +++ b/test/extensions/load_balancing_policies/round_robin/round_robin_lb_test.cc @@ -21,10 +21,6 @@ class RoundRobinLoadBalancerTest : public LoadBalancerTestBase { common_config_.mutable_locality_weighted_lb_config(); } - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.locality_routing_use_new_routing_logic", - GetParam().use_new_locality_routing ? "true" : "false"}}); - lb_ = std::make_shared(priority_set_, local_priority_set_.get(), stats_, runtime_, random_, common_config_, round_robin_lb_config_, simTime()); @@ -245,8 +241,8 @@ TEST_P(FailoverTest, PrioritiesWithZeroWarmedHosts) { } INSTANTIATE_TEST_SUITE_P(PrimaryOrFailoverAndLegacyOrNew, FailoverTest, - ::testing::Values(LoadBalancerTestParam{true, false}, - LoadBalancerTestParam{true, true})); + ::testing::Values(LoadBalancerTestParam{true}, + LoadBalancerTestParam{false})); TEST_P(RoundRobinLoadBalancerTest, NoHosts) { init(false); @@ -490,8 +486,6 @@ TEST_P(RoundRobinLoadBalancerTest, WeightedSeedOldInit) { // Validate that low weighted hosts will be chosen when the LB is created. TEST_P(RoundRobinLoadBalancerTest, WeightedInitializationPicksAllHosts) { TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.locality_routing_use_new_routing_logic", - GetParam().use_new_locality_routing ? "true" : "false"}}); // This test should be kept just the runtime override removed once the // feature-flag is deprecated. scoped_runtime.mergeValues( @@ -766,23 +760,14 @@ TEST_P(RoundRobinLoadBalancerTest, ZoneAwareZonesMismatched) { hostSet().healthy_hosts_per_locality_ = upstream_hosts_per_locality_b; updateHosts(hosts, local_hosts_per_locality_b); - if (GetParam().use_new_locality_routing) { - // Expect zone-aware routing still enabled even though there is no upstream host in zone B - // Since zone A has no residual, we should always pick an upstream from zone C. - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(4999)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); - } else { - // When using the legacy locality routing algorithm, it falls back to non-zone aware routing in - // zone B. - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[0], lb_->chooseHost(nullptr)); - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr)); - } + // Expect zone-aware routing still enabled even though there is no upstream host in zone B + // Since zone A has no residual, we should always pick an upstream from zone C. + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(0)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(4999)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); } TEST_P(RoundRobinLoadBalancerTest, ZoneAwareResidualsMismatched) { @@ -891,74 +876,31 @@ TEST_P(RoundRobinLoadBalancerTest, ZoneAwareResidualsMismatched) { EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[0][0], lb_->chooseHost(nullptr)); EXPECT_EQ(2U, stats_.lb_zone_routing_sampled_.value()); - if (GetParam().use_new_locality_routing) { - // The other 5/8 of the time, traffic will go to cross-zone upstreams with residual capacity - // Zone B has no upstream hosts - // Zone C has a residual capacity of 20.83% (20.84% with rounding error): sampled value 0-2083 - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(3750)).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(3750)) - .WillOnce(Return(2083)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][1], lb_->chooseHost(nullptr)); - EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); - // Zone D has a residual capacity of 20.83% (20.84% with rounding error): sampled value - // 2084-4167 - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(9999)) - .WillOnce(Return(2084)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(3U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(9999)) - .WillOnce(Return(4167)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][1], lb_->chooseHost(nullptr)); - EXPECT_EQ(4U, stats_.lb_zone_routing_cross_zone_.value()); - // Zone E has a residual capacity of 12.5%: sampled value 4168-5417 - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(9999)) - .WillOnce(Return(4168)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[3][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(5U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(9999)) - .WillOnce(Return(5417)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[3][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(6U, stats_.lb_zone_routing_cross_zone_.value()); - // At sampled value 5418, we loop back to the beginning of the vector and select zone C again - } else { - // When using the legacy locality routing algorithm, the routing is unfair. - // Zone C has a residual capacity of 4.16% (4.17% with rounding error): sampled value 0-416 - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(3750)).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(3750)).WillOnce(Return(416)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][1], lb_->chooseHost(nullptr)); - EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); - // Zone D has a residual capacity of 20.83%: sampled value 417-2500 - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(9999)).WillOnce(Return(417)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(3U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(9999)) - .WillOnce(Return(2500)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][1], lb_->chooseHost(nullptr)); - EXPECT_EQ(4U, stats_.lb_zone_routing_cross_zone_.value()); - // After this, we loop back to the beginning of the vector and select zone C again - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(3750)) - .WillOnce(Return(2501)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][2], lb_->chooseHost(nullptr)); - EXPECT_EQ(5U, stats_.lb_zone_routing_cross_zone_.value()); - } + // The other 5/8 of the time, traffic will go to cross-zone upstreams with residual capacity + // Zone B has no upstream hosts + // Zone C has a residual capacity of 20.83% (20.84% with rounding error): sampled value 0-2083 + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(3750)).WillOnce(Return(0)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(3750)).WillOnce(Return(2083)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][1], lb_->chooseHost(nullptr)); + EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); + // Zone D has a residual capacity of 20.83% (20.84% with rounding error): sampled value + // 2084-4167 + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(9999)).WillOnce(Return(2084)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(3U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(9999)).WillOnce(Return(4167)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][1], lb_->chooseHost(nullptr)); + EXPECT_EQ(4U, stats_.lb_zone_routing_cross_zone_.value()); + // Zone E has a residual capacity of 12.5%: sampled value 4168-5417 + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(9999)).WillOnce(Return(4168)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[3][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(5U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(9999)).WillOnce(Return(5417)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[3][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(6U, stats_.lb_zone_routing_cross_zone_.value()); + // At sampled value 5418, we loop back to the beginning of the vector and select zone C again } TEST_P(RoundRobinLoadBalancerTest, ZoneAwareDifferentZoneSize) { @@ -1004,36 +946,22 @@ TEST_P(RoundRobinLoadBalancerTest, ZoneAwareDifferentZoneSize) { EXPECT_CALL(runtime_.snapshot_, getInteger("upstream.zone_routing.min_cluster_size", 3)) .WillRepeatedly(Return(3)); - if (GetParam().use_new_locality_routing) { - // 2/3 of the time we should get the host in zone B - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[0][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(1U, stats_.lb_zone_routing_sampled_.value()); - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(6665)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[0][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(2U, stats_.lb_zone_routing_sampled_.value()); - - // 1/3 of the time we should sample the residuals across zones - // The only upstream zone with residual capacity is zone C - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(6666)).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(9999)) - .WillOnce(Return(3333)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); - } else { - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[0], lb_->chooseHost(nullptr)); - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr)); - EXPECT_EQ(0U, stats_.lb_zone_routing_all_directly_.value()); - EXPECT_EQ(0U, stats_.lb_zone_routing_sampled_.value()); - EXPECT_EQ(0U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_EQ(1U, stats_.lb_zone_number_differs_.value()); - } + // 2/3 of the time we should get the host in zone B + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[0][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(1U, stats_.lb_zone_routing_sampled_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(6665)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[0][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(2U, stats_.lb_zone_routing_sampled_.value()); + + // 1/3 of the time we should sample the residuals across zones + // The only upstream zone with residual capacity is zone C + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(6666)).WillOnce(Return(0)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(9999)).WillOnce(Return(3333)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); } TEST_P(RoundRobinLoadBalancerTest, ZoneAwareRoutingLargeZoneSwitchOnOff) { @@ -1186,36 +1114,24 @@ TEST_P(RoundRobinLoadBalancerTest, ZoneAwareNoMatchingZones) { init(true); updateHosts(local_hosts, local_hosts_per_locality); - if (GetParam().use_new_locality_routing) { - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(3332)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[0][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(3333)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(6665)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(3U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(6666)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(4U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(0)) - .WillOnce(Return(9998)); // Rounding error: 3333 * 3 = 9999 != 10000 - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(5U, stats_.lb_zone_routing_cross_zone_.value()); - } else { - // When using the legacy locality routing algorithm, locality routing is disabled as the - // upstream has no hosts in the local zone. - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[0], lb_->chooseHost(nullptr)); - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr)); - EXPECT_EQ(0U, stats_.lb_zone_routing_all_directly_.value()); - EXPECT_EQ(0U, stats_.lb_zone_routing_sampled_.value()); - EXPECT_EQ(0U, stats_.lb_zone_routing_cross_zone_.value()); - } + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(3332)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[0][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(3333)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(6665)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[1][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(3U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(6666)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(4U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()) + .WillOnce(Return(0)) + .WillOnce(Return(0)) + .WillOnce(Return(9998)); // Rounding error: 3333 * 3 = 9999 != 10000 + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(5U, stats_.lb_zone_routing_cross_zone_.value()); } TEST_P(RoundRobinLoadBalancerTest, NoZoneAwareNotEnoughLocalZones) { @@ -1383,58 +1299,46 @@ TEST_P(RoundRobinLoadBalancerTest, ZoneAwareEmptyLocalities) { init(true); updateHosts(local_hosts, local_hosts_per_locality); - if (GetParam().use_new_locality_routing) { - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(832)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][1], lb_->chooseHost(nullptr)); - EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); - - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(833)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[3][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(3U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(0)) - .WillOnce(Return(2498)); // rounding error - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[3][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(4U, stats_.lb_zone_routing_cross_zone_.value()); - - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(2499)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[4][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(5U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(3331)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[4][1], lb_->chooseHost(nullptr)); - EXPECT_EQ(6U, stats_.lb_zone_routing_cross_zone_.value()); - - EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(3332)); - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[6][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(7U, stats_.lb_zone_routing_cross_zone_.value()); - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(0)) - .WillOnce(Return(4997)); // rounding error - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[6][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(8U, stats_.lb_zone_routing_cross_zone_.value()); - - EXPECT_CALL(random_, random()) - .WillOnce(Return(0)) - .WillOnce(Return(0)) - .WillOnce(Return(4998)); // wrap around - EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); - EXPECT_EQ(9U, stats_.lb_zone_routing_cross_zone_.value()); - } else { - // When using the legacy locality routing algorithm, locality routing is disabled as the - // upstream has no hosts in the local zone. - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[0], lb_->chooseHost(nullptr)); - EXPECT_CALL(random_, random()).WillOnce(Return(0)); - EXPECT_EQ(hostSet().healthy_hosts_[1], lb_->chooseHost(nullptr)); - EXPECT_EQ(0U, stats_.lb_zone_routing_all_directly_.value()); - EXPECT_EQ(0U, stats_.lb_zone_routing_sampled_.value()); - EXPECT_EQ(0U, stats_.lb_zone_routing_cross_zone_.value()); - } + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(0)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(1U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(832)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][1], lb_->chooseHost(nullptr)); + EXPECT_EQ(2U, stats_.lb_zone_routing_cross_zone_.value()); + + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(833)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[3][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(3U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()) + .WillOnce(Return(0)) + .WillOnce(Return(0)) + .WillOnce(Return(2498)); // rounding error + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[3][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(4U, stats_.lb_zone_routing_cross_zone_.value()); + + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(2499)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[4][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(5U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(3331)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[4][1], lb_->chooseHost(nullptr)); + EXPECT_EQ(6U, stats_.lb_zone_routing_cross_zone_.value()); + + EXPECT_CALL(random_, random()).WillOnce(Return(0)).WillOnce(Return(0)).WillOnce(Return(3332)); + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[6][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(7U, stats_.lb_zone_routing_cross_zone_.value()); + EXPECT_CALL(random_, random()) + .WillOnce(Return(0)) + .WillOnce(Return(0)) + .WillOnce(Return(4997)); // rounding error + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[6][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(8U, stats_.lb_zone_routing_cross_zone_.value()); + + EXPECT_CALL(random_, random()) + .WillOnce(Return(0)) + .WillOnce(Return(0)) + .WillOnce(Return(4998)); // wrap around + EXPECT_EQ(hostSet().healthy_hosts_per_locality_->get()[2][0], lb_->chooseHost(nullptr)); + EXPECT_EQ(9U, stats_.lb_zone_routing_cross_zone_.value()); } TEST_P(RoundRobinLoadBalancerTest, LowPrecisionForDistribution) { @@ -1667,10 +1571,8 @@ TEST_P(RoundRobinLoadBalancerTest, NoZoneAwareRoutingNoLocalLocality) { } INSTANTIATE_TEST_SUITE_P(PrimaryOrFailoverAndLegacyOrNew, RoundRobinLoadBalancerTest, - ::testing::Values(LoadBalancerTestParam{true, false}, - LoadBalancerTestParam{true, true}, - LoadBalancerTestParam{false, false}, - LoadBalancerTestParam{false, true})); + ::testing::Values(LoadBalancerTestParam{true}, + LoadBalancerTestParam{false})); TEST_P(RoundRobinLoadBalancerTest, SlowStartWithDefaultParams) { init(false); From d67fe8ff331322a5ff92734779373eb93952a001 Mon Sep 17 00:00:00 2001 From: botengyao Date: Wed, 12 Jun 2024 17:14:18 -0400 Subject: [PATCH 09/21] opentelemetry: decouple dynatrace dependency from always_on sampler tests (#34701) remove dynatrace dependency Signed-off-by: Boteng Yao --- test/extensions/tracers/opentelemetry/samplers/always_on/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/test/extensions/tracers/opentelemetry/samplers/always_on/BUILD b/test/extensions/tracers/opentelemetry/samplers/always_on/BUILD index 3379c6fb5bb6..24c2c52e32d2 100644 --- a/test/extensions/tracers/opentelemetry/samplers/always_on/BUILD +++ b/test/extensions/tracers/opentelemetry/samplers/always_on/BUILD @@ -46,7 +46,6 @@ envoy_extension_cc_test( deps = [ "//source/exe:main_common_lib", "//source/extensions/tracers/opentelemetry:config", - "//source/extensions/tracers/opentelemetry/resource_detectors/dynatrace:config", "//source/extensions/tracers/opentelemetry/samplers/always_on:config", "//test/integration:http_integration_lib", "//test/test_common:utility_lib", From 245da145f2922824832101bfd7f1f95afcde9551 Mon Sep 17 00:00:00 2001 From: doujiang24 Date: Thu, 13 Jun 2024 09:11:33 +0800 Subject: [PATCH 10/21] Golang filter: make asan clean (#34702) Signed-off-by: doujiang24 --- contrib/golang/common/dso/dso.cc | 7 ++++ contrib/golang/common/dso/dso.h | 30 +++++++++++++--- contrib/golang/common/dso/libgolang.h | 4 +++ contrib/golang/common/dso/test/mocks.h | 1 + .../common/dso/test/test_data/simple.go | 4 +++ .../filters/http/source/go/pkg/http/BUILD | 1 + .../filters/http/source/go/pkg/http/asan.go | 34 +++++++++++++++++++ .../filters/http/source/go/pkg/http/config.go | 3 ++ .../filters/http/source/go/pkg/http/shim.go | 8 +++++ .../golang/filters/http/test/config_test.cc | 13 +++++-- .../filters/http/test/golang_filter_test.cc | 3 ++ .../http/test/golang_integration_test.cc | 6 +++- .../http/test/websocket_integration_test.cc | 12 +++++-- 13 files changed, 117 insertions(+), 9 deletions(-) create mode 100644 contrib/golang/filters/http/source/go/pkg/http/asan.go diff --git a/contrib/golang/common/dso/dso.cc b/contrib/golang/common/dso/dso.cc index ad7904ebda06..ce7336453831 100644 --- a/contrib/golang/common/dso/dso.cc +++ b/contrib/golang/common/dso/dso.cc @@ -63,6 +63,8 @@ HttpFilterDsoImpl::HttpFilterDsoImpl(const std::string dso_name) : HttpFilterDso envoy_go_filter_on_http_destroy_, handler_, dso_name, "envoyGoFilterOnHttpDestroy"); loaded_ &= dlsymInternal( envoy_go_filter_go_request_sema_dec_, handler_, dso_name, "envoyGoRequestSemaDec"); + loaded_ &= dlsymInternal(envoy_go_filter_cleanup_, handler_, + dso_name, "envoyGoFilterCleanUp"); } GoUint64 HttpFilterDsoImpl::envoyGoFilterNewHttpPluginConfig(httpConfig* p0) { @@ -108,6 +110,11 @@ void HttpFilterDsoImpl::envoyGoRequestSemaDec(httpRequest* p0) { envoy_go_filter_go_request_sema_dec_(p0); } +void HttpFilterDsoImpl::cleanup() { + ASSERT(envoy_go_filter_cleanup_ != nullptr); + envoy_go_filter_cleanup_(); +} + ClusterSpecifierDsoImpl::ClusterSpecifierDsoImpl(const std::string dso_name) : ClusterSpecifierDso(dso_name) { loaded_ &= dlsymInternal( diff --git a/contrib/golang/common/dso/dso.h b/contrib/golang/common/dso/dso.h index f18c32f01e91..78c7c0d768ef 100644 --- a/contrib/golang/common/dso/dso.h +++ b/contrib/golang/common/dso/dso.h @@ -17,8 +17,12 @@ class Dso { public: Dso() = default; Dso(const std::string dso_name); - ~Dso(); + virtual ~Dso(); bool loaded() { return loaded_; } + /* + * Clean up resources that are referenced on the Golang side. + */ + virtual void cleanup(){}; protected: const std::string dso_name_; @@ -29,7 +33,7 @@ class Dso { class HttpFilterDso : public Dso { public: HttpFilterDso(const std::string dso_name) : Dso(dso_name){}; - virtual ~HttpFilterDso() = default; + ~HttpFilterDso() override = default; virtual GoUint64 envoyGoFilterNewHttpPluginConfig(httpConfig* p0) PURE; virtual GoUint64 envoyGoFilterMergeHttpPluginConfig(GoUint64 p0, GoUint64 p1, GoUint64 p2, @@ -60,6 +64,7 @@ class HttpFilterDsoImpl : public HttpFilterDso { void envoyGoFilterOnHttpLog(httpRequest* p0, int p1) override; void envoyGoFilterOnHttpDestroy(httpRequest* p0, int p1) override; void envoyGoRequestSemaDec(httpRequest* p0) override; + void cleanup() override; private: GoUint64 (*envoy_go_filter_new_http_plugin_config_)(httpConfig* p0) = {nullptr}; @@ -73,12 +78,13 @@ class HttpFilterDsoImpl : public HttpFilterDso { void (*envoy_go_filter_on_http_log_)(httpRequest* p0, GoUint64 p1) = {nullptr}; void (*envoy_go_filter_on_http_destroy_)(httpRequest* p0, GoUint64 p1) = {nullptr}; void (*envoy_go_filter_go_request_sema_dec_)(httpRequest* p0) = {nullptr}; + void (*envoy_go_filter_cleanup_)() = {nullptr}; }; class ClusterSpecifierDso : public Dso { public: ClusterSpecifierDso(const std::string dso_name) : Dso(dso_name){}; - virtual ~ClusterSpecifierDso() = default; + ~ClusterSpecifierDso() override = default; virtual GoInt64 envoyGoOnClusterSpecify(GoUint64 plugin_ptr, GoUint64 header_ptr, GoUint64 plugin_id, GoUint64 buffer_ptr, @@ -110,7 +116,7 @@ class NetworkFilterDso : public Dso { public: NetworkFilterDso() = default; NetworkFilterDso(const std::string dso_name) : Dso(dso_name){}; - virtual ~NetworkFilterDso() = default; + ~NetworkFilterDso() override = default; virtual GoUint64 envoyGoFilterOnNetworkFilterConfig(GoUint64 library_id_ptr, GoUint64 library_id_len, GoUint64 config_ptr, @@ -265,6 +271,22 @@ template class DsoManager { return nullptr; }; + /** + * Clean up all golang runtime to make asan happy in testing. + */ + static void cleanUpForTest() { + DsoStoreType& dsoStore = getDsoStore(); + absl::WriterMutexLock lock(&dsoStore.mutex_); + for (auto it = dsoStore.id_to_dso_.begin(); it != dsoStore.id_to_dso_.end(); it++) { + auto dso = it->second; + if (dso != nullptr) { + dso->cleanup(); + } + } + dsoStore.id_to_dso_.clear(); + dsoStore.plugin_name_to_dso_.clear(); + }; + private: using DsoMapType = absl::flat_hash_map>; struct DsoStoreType { diff --git a/contrib/golang/common/dso/libgolang.h b/contrib/golang/common/dso/libgolang.h index b94f0c066f36..b477b5b55e6b 100644 --- a/contrib/golang/common/dso/libgolang.h +++ b/contrib/golang/common/dso/libgolang.h @@ -130,6 +130,10 @@ extern void envoyGoFilterOnHttpDestroy(httpRequest* r, GoUint64 reason); // github.com/envoyproxy/envoy/contrib/golang/filters/http/source/go/pkg/http.envoyGoRequestSemaDec extern void envoyGoRequestSemaDec(httpRequest* r); +// go:linkname envoyGoFilterCleanUp +// github.com/envoyproxy/envoy/contrib/golang/filters/http/source/go/pkg/http.envoyGoFilterCleanUp +extern void envoyGoFilterCleanUp(); + // go:linkname envoyGoOnClusterSpecify // github.com/envoyproxy/envoy/contrib/golang/router/cluster_specifier/source/go/pkg/cluster_specifier.envoyGoOnClusterSpecify extern GoInt64 envoyGoOnClusterSpecify(GoUint64 plugin_ptr, GoUint64 header_ptr, GoUint64 plugin_id, diff --git a/contrib/golang/common/dso/test/mocks.h b/contrib/golang/common/dso/test/mocks.h index 5a9de43656c9..973a096bfb10 100644 --- a/contrib/golang/common/dso/test/mocks.h +++ b/contrib/golang/common/dso/test/mocks.h @@ -24,6 +24,7 @@ class MockHttpFilterDsoImpl : public HttpFilterDso { MOCK_METHOD(void, envoyGoFilterOnHttpLog, (httpRequest * p0, int p1)); MOCK_METHOD(void, envoyGoFilterOnHttpDestroy, (httpRequest * p0, int p1)); MOCK_METHOD(void, envoyGoRequestSemaDec, (httpRequest * p0)); + MOCK_METHOD(void, envoyGoFilterCleanUp, ()); }; class MockNetworkFilterDsoImpl : public NetworkFilterDso { diff --git a/contrib/golang/common/dso/test/test_data/simple.go b/contrib/golang/common/dso/test/test_data/simple.go index bf5f028f8f9a..a0ce2419bb2f 100644 --- a/contrib/golang/common/dso/test/test_data/simple.go +++ b/contrib/golang/common/dso/test/test_data/simple.go @@ -119,5 +119,9 @@ func envoyGoFilterOnSemaDec(wrapper unsafe.Pointer) { func envoyGoRequestSemaDec(r *C.httpRequest) { } +//export envoyGoFilterCleanUp +func envoyGoFilterCleanUp() { +} + func main() { } diff --git a/contrib/golang/filters/http/source/go/pkg/http/BUILD b/contrib/golang/filters/http/source/go/pkg/http/BUILD index ae49c9487324..f8a616992f72 100644 --- a/contrib/golang/filters/http/source/go/pkg/http/BUILD +++ b/contrib/golang/filters/http/source/go/pkg/http/BUILD @@ -6,6 +6,7 @@ go_library( name = "http", srcs = [ "api.h", + "asan.go", "capi_impl.go", "config.go", "filter.go", diff --git a/contrib/golang/filters/http/source/go/pkg/http/asan.go b/contrib/golang/filters/http/source/go/pkg/http/asan.go new file mode 100644 index 000000000000..fb659ee0debe --- /dev/null +++ b/contrib/golang/filters/http/source/go/pkg/http/asan.go @@ -0,0 +1,34 @@ +package http + +import ( + "runtime" + "sync" + + "github.com/envoyproxy/envoy/contrib/golang/common/go/api" +) + +var ( + asanTestEnabled = false +) + +// forceGCFinalizer enforce GC and wait GC finalizer finished. +// Just for testing, to make asan happy. +func forceGCFinalizer() { + var wg sync.WaitGroup + wg.Add(1) + + { + // create a fake httpRequest to trigger GC finalizer. + fake := &httpRequest{} + runtime.SetFinalizer(fake, func(*httpRequest) { + wg.Done() + }) + } + + api.LogWarn("golang filter enforcing GC") + // enforce a GC cycle. + runtime.GC() + + // wait GC finalizers finished. + wg.Wait() +} diff --git a/contrib/golang/filters/http/source/go/pkg/http/config.go b/contrib/golang/filters/http/source/go/pkg/http/config.go index 21c461ca25d6..96df66457961 100644 --- a/contrib/golang/filters/http/source/go/pkg/http/config.go +++ b/contrib/golang/filters/http/source/go/pkg/http/config.go @@ -111,6 +111,9 @@ func envoyGoFilterDestroyHttpPluginConfig(id uint64, needDelay int) { // there is no race for non-merged config. configCache.Delete(id) } + if asanTestEnabled { + forceGCFinalizer() + } } //export envoyGoFilterMergeHttpPluginConfig diff --git a/contrib/golang/filters/http/source/go/pkg/http/shim.go b/contrib/golang/filters/http/source/go/pkg/http/shim.go index 229f2c9aa0fc..c904ee5f98a2 100644 --- a/contrib/golang/filters/http/source/go/pkg/http/shim.go +++ b/contrib/golang/filters/http/source/go/pkg/http/shim.go @@ -316,3 +316,11 @@ func envoyGoRequestSemaDec(r *C.httpRequest) { defer req.recoverPanic() req.resumeWaitCallback() } + +// This is unsafe, just for asan testing. +// +//export envoyGoFilterCleanUp +func envoyGoFilterCleanUp() { + asanTestEnabled = true + forceGCFinalizer() +} diff --git a/contrib/golang/filters/http/test/config_test.cc b/contrib/golang/filters/http/test/config_test.cc index 315f815c8c42..4dd471302711 100644 --- a/contrib/golang/filters/http/test/config_test.cc +++ b/contrib/golang/filters/http/test/config_test.cc @@ -13,6 +13,7 @@ #include "gtest/gtest.h" using testing::_; +using ::testing::Invoke; namespace Envoy { namespace Extensions { @@ -25,6 +26,8 @@ std::string genSoPath(std::string name) { "{{ test_rundir }}/contrib/golang/filters/http/test/test_data/" + name + "/filter.so"); } +void cleanup() { Dso::DsoManager::cleanUpForTest(); } + TEST(GolangFilterConfigTest, InvalidateEmptyConfig) { NiceMock context; EXPECT_THROW_WITH_REGEX( @@ -62,12 +65,15 @@ TEST(GolangFilterConfigTest, GolangFilterWithValidConfig) { NiceMock filter_callback; NiceMock dispatcher{"worker_0"}; ON_CALL(filter_callback, dispatcher()).WillByDefault(ReturnRef(dispatcher)); - EXPECT_CALL(filter_callback, addStreamFilter(_)); + EXPECT_CALL(filter_callback, addStreamFilter(_)) + .WillOnce(Invoke([](Http::StreamDecoderFilterSharedPtr filter) { filter->onDestroy(); })); EXPECT_CALL(filter_callback, addAccessLogHandler(_)); auto plugin_config = proto_config.plugin_config(); std::string str; EXPECT_TRUE(plugin_config.SerializeToString(&str)); cb(filter_callback); + + cleanup(); } TEST(GolangFilterConfigTest, GolangFilterWithNilPluginConfig) { @@ -88,12 +94,15 @@ TEST(GolangFilterConfigTest, GolangFilterWithNilPluginConfig) { NiceMock filter_callback; NiceMock dispatcher{"worker_0"}; ON_CALL(filter_callback, dispatcher()).WillByDefault(ReturnRef(dispatcher)); - EXPECT_CALL(filter_callback, addStreamFilter(_)); + EXPECT_CALL(filter_callback, addStreamFilter(_)) + .WillOnce(Invoke([](Http::StreamDecoderFilterSharedPtr filter) { filter->onDestroy(); })); EXPECT_CALL(filter_callback, addAccessLogHandler(_)); auto plugin_config = proto_config.plugin_config(); std::string str; EXPECT_TRUE(plugin_config.SerializeToString(&str)); cb(filter_callback); + + cleanup(); } } // namespace diff --git a/contrib/golang/filters/http/test/golang_filter_test.cc b/contrib/golang/filters/http/test/golang_filter_test.cc index 916203470031..365f69899f91 100644 --- a/contrib/golang/filters/http/test/golang_filter_test.cc +++ b/contrib/golang/filters/http/test/golang_filter_test.cc @@ -76,6 +76,7 @@ class GolangHttpFilterTest : public testing::Test { if (filter_ != nullptr) { filter_->onDestroy(); } + Dso::DsoManager::cleanUpForTest(); } void setup(const std::string& lib_id, const std::string& lib_path, @@ -179,6 +180,8 @@ TEST_F(GolangHttpFilterTest, SetHeaderAtWrongStage) { auto req = new HttpRequestInternal(*filter_); EXPECT_EQ(CAPINotInGo, filter_->setHeader(req->decodingState(), "foo", "bar", HeaderSet)); + + delete req; } // invalid config for routeconfig filter diff --git a/contrib/golang/filters/http/test/golang_integration_test.cc b/contrib/golang/filters/http/test/golang_integration_test.cc index bbc543632e91..cddf317e9368 100644 --- a/contrib/golang/filters/http/test/golang_integration_test.cc +++ b/contrib/golang/filters/http/test/golang_integration_test.cc @@ -664,7 +664,11 @@ name: golang cleanup(); } - void cleanup() { cleanupUpstreamAndDownstream(); } + void cleanup() { + cleanupUpstreamAndDownstream(); + + Dso::DsoManager::cleanUpForTest(); + } void testDynamicMetadata(std::string path) { initializeBasicFilter(BASIC, "*", true); diff --git a/contrib/golang/filters/http/test/websocket_integration_test.cc b/contrib/golang/filters/http/test/websocket_integration_test.cc index 455bfeae67f2..271eaf03073e 100644 --- a/contrib/golang/filters/http/test/websocket_integration_test.cc +++ b/contrib/golang/filters/http/test/websocket_integration_test.cc @@ -13,11 +13,17 @@ #include "test/test_common/utility.h" #include "absl/strings/str_cat.h" +#include "contrib/golang/filters/http/source/golang_filter.h" #include "gtest/gtest.h" namespace Envoy { -INSTANTIATE_TEST_SUITE_P(Protocols, WebsocketIntegrationTest, +class GolangWebsocketIntegrationTest : public WebsocketIntegrationTest { +public: + void cleanup() { Dso::DsoManager::cleanUpForTest(); } +}; + +INSTANTIATE_TEST_SUITE_P(Protocols, GolangWebsocketIntegrationTest, testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), HttpProtocolIntegrationTest::protocolTestParamsToString); @@ -51,7 +57,7 @@ name: golang return absl::StrFormat(yaml_fmt, name, genSoPath(name), name); } -TEST_P(WebsocketIntegrationTest, WebsocketGolangFilterChain) { +TEST_P(GolangWebsocketIntegrationTest, WebsocketGolangFilterChain) { if (downstreamProtocol() != Http::CodecType::HTTP1 || upstreamProtocol() != Http::CodecType::HTTP1) { return; @@ -108,6 +114,8 @@ TEST_P(WebsocketIntegrationTest, WebsocketGolangFilterChain) { ASSERT_TRUE(absl::StrContains(tcp_client->data(), "Bye_bar foo")); tcp_client->close(); ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + + cleanup(); } } // namespace Envoy From 7d8791802c1d7e0230752e3d3b2c29b6c0621bc7 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 12 Jun 2024 22:58:31 -0400 Subject: [PATCH 11/21] exceptions: minor cleanup (#34655) * exceptions: minor cleanup Signed-off-by: Alyssa Wilk * file cleanup Signed-off-by: Alyssa Wilk --------- Signed-off-by: Alyssa Wilk --- 46 | 1 - .../filters/network/source/config.cc | 34 ++++++- source/common/access_log/access_log_impl.h | 39 -------- source/common/common/utility.cc | 3 +- .../filters/http/ext_authz/ext_authz.cc | 96 +++++++++++++++++++ .../filters/http/ext_authz/ext_authz.h | 94 +----------------- .../filters/http/ext_proc/ext_proc.cc | 44 +++++++++ .../filters/http/ext_proc/ext_proc.h | 41 +------- .../filters/network/mongo_proxy/bson_impl.cc | 5 + .../filters/network/mongo_proxy/bson_impl.h | 6 +- .../filters/udp/udp_proxy/config.cc | 2 +- test/common/common/utility_test.cc | 2 +- 12 files changed, 185 insertions(+), 182 deletions(-) delete mode 100644 46 diff --git a/46 b/46 deleted file mode 100644 index 9eef263bd8ab..000000000000 --- a/46 +++ /dev/null @@ -1 +0,0 @@ -22 - diff --git a/contrib/generic_proxy/filters/network/source/config.cc b/contrib/generic_proxy/filters/network/source/config.cc index abe02586646b..6a10fd79446d 100644 --- a/contrib/generic_proxy/filters/network/source/config.cc +++ b/contrib/generic_proxy/filters/network/source/config.cc @@ -12,6 +12,38 @@ namespace Extensions { namespace NetworkFilters { namespace GenericProxy { +template +static AccessLog::FilterBasePtr +accessLogFilterFromProto(const envoy::config::accesslog::v3::AccessLogFilter& config, + Server::Configuration::FactoryContext& context) { + if (!config.has_extension_filter()) { + ExceptionUtil::throwEnvoyException( + "Access log filter: only extension filter is supported by non-HTTP access loggers."); + } + + auto& factory = + Config::Utility::getAndCheckFactory>( + config.extension_filter()); + return factory.createFilter(config.extension_filter(), context); +} + +template +static AccessLog::InstanceBaseSharedPtr +accessLoggerFromProto(const envoy::config::accesslog::v3::AccessLog& config, + Server::Configuration::FactoryContext& context) { + AccessLog::FilterBasePtr filter; + if (config.has_filter()) { + filter = accessLogFilterFromProto(config.filter(), context); + } + + auto& factory = + Config::Utility::getAndCheckFactory>( + config); + ProtobufTypes::MessagePtr message = Config::Utility::translateToFactoryConfig( + config, context.messageValidationVisitor(), factory); + + return factory.createAccessLogInstance(*message, std::move(filter), context); +} SINGLETON_MANAGER_REGISTRATION(generic_route_config_provider_manager); SINGLETON_MANAGER_REGISTRATION(generic_proxy_code_or_flag_stats); @@ -127,7 +159,7 @@ Factory::createFilterFactoryFromProtoTyped(const ProxyConfig& proto_config, std::vector access_logs; for (const auto& access_log : proto_config.access_log()) { AccessLogInstanceSharedPtr current_access_log = - AccessLog::AccessLogFactory::accessLoggerFromProto(access_log, context); + accessLoggerFromProto(access_log, context); access_logs.push_back(current_access_log); } diff --git a/source/common/access_log/access_log_impl.h b/source/common/access_log/access_log_impl.h index 8f28b6e93fb6..860285614780 100644 --- a/source/common/access_log/access_log_impl.h +++ b/source/common/access_log/access_log_impl.h @@ -270,45 +270,6 @@ class AccessLogFactory { */ static InstanceSharedPtr fromProto(const envoy::config::accesslog::v3::AccessLog& config, Server::Configuration::FactoryContext& context); - - /** - * Template method to create an access log filter from proto configuration for non-HTTP access - * loggers. - */ - template - static FilterBasePtr - accessLogFilterFromProto(const envoy::config::accesslog::v3::AccessLogFilter& config, - Server::Configuration::FactoryContext& context) { - if (!config.has_extension_filter()) { - ExceptionUtil::throwEnvoyException( - "Access log filter: only extension filter is supported by non-HTTP access loggers."); - } - - auto& factory = Config::Utility::getAndCheckFactory>( - config.extension_filter()); - return factory.createFilter(config.extension_filter(), context); - } - - /** - * Template method to create an access logger instance from proto configuration for non-HTTP - * access loggers. - */ - template - static InstanceBaseSharedPtr - accessLoggerFromProto(const envoy::config::accesslog::v3::AccessLog& config, - Server::Configuration::FactoryContext& context) { - FilterBasePtr filter; - if (config.has_filter()) { - filter = accessLogFilterFromProto(config.filter(), context); - } - - auto& factory = - Config::Utility::getAndCheckFactory>(config); - ProtobufTypes::MessagePtr message = Config::Utility::translateToFactoryConfig( - config, context.messageValidationVisitor(), factory); - - return factory.createAccessLogInstance(*message, std::move(filter), context); - } }; } // namespace AccessLog diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 144d57f69a12..de988db79f42 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -408,7 +408,8 @@ std::string StringUtil::removeTokens(absl::string_view source, absl::string_view uint32_t StringUtil::itoa(char* out, size_t buffer_size, uint64_t i) { // The maximum size required for an unsigned 64-bit integer is 21 chars (including null). if (buffer_size < 21) { - throwExceptionOrPanic(std::invalid_argument, "itoa buffer too small"); + IS_ENVOY_BUG("itoa buffer too small"); + return 0; } char* current = out; diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 88b27ec663a1..187721c466d6 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -60,6 +60,102 @@ void fillMetadataContext(const std::vector& source_metadat } // namespace +FilterConfig::FilterConfig(const envoy::extensions::filters::http::ext_authz::v3::ExtAuthz& config, + Stats::Scope& scope, const std::string& stats_prefix, + Server::Configuration::ServerFactoryContext& factory_context) + : allow_partial_message_(config.with_request_body().allow_partial_message()), + failure_mode_allow_(config.failure_mode_allow()), + failure_mode_allow_header_add_(config.failure_mode_allow_header_add()), + clear_route_cache_(config.clear_route_cache()), + max_request_bytes_(config.with_request_body().max_request_bytes()), + + // `pack_as_bytes_` should be true when configured with the HTTP service because there is no + // difference to where the body is written in http requests, and a value of false here will + // cause non UTF-8 body content to be changed when it doesn't need to. + pack_as_bytes_(config.has_http_service() || config.with_request_body().pack_as_bytes()), + + encode_raw_headers_(config.encode_raw_headers()), + + 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( + Runtime::FractionalPercent(config.filter_enabled(), runtime_)) + : absl::nullopt), + filter_enabled_metadata_( + config.has_filter_enabled_metadata() + ? absl::optional( + Matchers::MetadataMatcher(config.filter_enabled_metadata(), factory_context)) + : absl::nullopt), + deny_at_disable_(config.has_deny_at_disable() + ? absl::optional( + Runtime::FeatureFlag(config.deny_at_disable(), runtime_)) + : absl::nullopt), + pool_(scope_.symbolTable()), + metadata_context_namespaces_(config.metadata_context_namespaces().begin(), + config.metadata_context_namespaces().end()), + typed_metadata_context_namespaces_(config.typed_metadata_context_namespaces().begin(), + config.typed_metadata_context_namespaces().end()), + route_metadata_context_namespaces_(config.route_metadata_context_namespaces().begin(), + config.route_metadata_context_namespaces().end()), + route_typed_metadata_context_namespaces_( + config.route_typed_metadata_context_namespaces().begin(), + config.route_typed_metadata_context_namespaces().end()), + include_peer_certificate_(config.include_peer_certificate()), + include_tls_session_(config.include_tls_session()), + charge_cluster_response_stats_( + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, charge_cluster_response_stats, true)), + stats_(generateStats(stats_prefix, config.stat_prefix(), scope)), + ext_authz_ok_(pool_.add(createPoolStatName(config.stat_prefix(), "ok"))), + ext_authz_denied_(pool_.add(createPoolStatName(config.stat_prefix(), "denied"))), + ext_authz_error_(pool_.add(createPoolStatName(config.stat_prefix(), "error"))), + ext_authz_invalid_(pool_.add(createPoolStatName(config.stat_prefix(), "invalid"))), + ext_authz_failure_mode_allowed_( + pool_.add(createPoolStatName(config.stat_prefix(), "failure_mode_allowed"))) { + auto bootstrap = factory_context.bootstrap(); + auto labels_key_it = + bootstrap.node().metadata().fields().find(config.bootstrap_metadata_labels_key()); + if (labels_key_it != bootstrap.node().metadata().fields().end()) { + for (const auto& labels_it : labels_key_it->second.struct_value().fields()) { + destination_labels_[labels_it.first] = labels_it.second.string_value(); + } + } + + if (config.has_allowed_headers() && + config.http_service().authorization_request().has_allowed_headers()) { + throw EnvoyException("Invalid duplicate configuration for allowed_headers."); + } + + // An unset request_headers_matchers_ means that all client request headers are allowed through + // to the authz server; this is to preserve backwards compatibility when introducing + // allowlisting of request headers for gRPC authz servers. Pre-existing support is for + // HTTP authz servers only and defaults to blocking all but a few headers (i.e. Authorization, + // Method, Path and Host). + if (config.has_grpc_service() && config.has_allowed_headers()) { + allowed_headers_matcher_ = Filters::Common::ExtAuthz::CheckRequestUtils::toRequestMatchers( + config.allowed_headers(), false, factory_context); + } else if (config.has_http_service()) { + if (config.http_service().authorization_request().has_allowed_headers()) { + allowed_headers_matcher_ = Filters::Common::ExtAuthz::CheckRequestUtils::toRequestMatchers( + config.http_service().authorization_request().allowed_headers(), true, factory_context); + } else { + allowed_headers_matcher_ = Filters::Common::ExtAuthz::CheckRequestUtils::toRequestMatchers( + config.allowed_headers(), true, factory_context); + } + } + if (config.has_disallowed_headers()) { + disallowed_headers_matcher_ = Filters::Common::ExtAuthz::CheckRequestUtils::toRequestMatchers( + config.disallowed_headers(), false, factory_context); + } +} + void FilterConfigPerRoute::merge(const FilterConfigPerRoute& other) { // We only merge context extensions here, and leave boolean flags untouched since those flags are // not used from the merged config. diff --git a/source/extensions/filters/http/ext_authz/ext_authz.h b/source/extensions/filters/http/ext_authz/ext_authz.h index 81e6cee23d81..96f1f02ccce6 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.h +++ b/source/extensions/filters/http/ext_authz/ext_authz.h @@ -59,99 +59,7 @@ class FilterConfig { public: FilterConfig(const envoy::extensions::filters::http::ext_authz::v3::ExtAuthz& config, Stats::Scope& scope, const std::string& stats_prefix, - Server::Configuration::ServerFactoryContext& factory_context) - : allow_partial_message_(config.with_request_body().allow_partial_message()), - failure_mode_allow_(config.failure_mode_allow()), - failure_mode_allow_header_add_(config.failure_mode_allow_header_add()), - clear_route_cache_(config.clear_route_cache()), - max_request_bytes_(config.with_request_body().max_request_bytes()), - - // `pack_as_bytes_` should be true when configured with the HTTP service because there is no - // difference to where the body is written in http requests, and a value of false here will - // cause non UTF-8 body content to be changed when it doesn't need to. - pack_as_bytes_(config.has_http_service() || config.with_request_body().pack_as_bytes()), - - encode_raw_headers_(config.encode_raw_headers()), - - 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( - Runtime::FractionalPercent(config.filter_enabled(), runtime_)) - : absl::nullopt), - filter_enabled_metadata_( - config.has_filter_enabled_metadata() - ? absl::optional( - Matchers::MetadataMatcher(config.filter_enabled_metadata(), factory_context)) - : absl::nullopt), - deny_at_disable_(config.has_deny_at_disable() - ? absl::optional( - Runtime::FeatureFlag(config.deny_at_disable(), runtime_)) - : absl::nullopt), - pool_(scope_.symbolTable()), - metadata_context_namespaces_(config.metadata_context_namespaces().begin(), - config.metadata_context_namespaces().end()), - typed_metadata_context_namespaces_(config.typed_metadata_context_namespaces().begin(), - config.typed_metadata_context_namespaces().end()), - route_metadata_context_namespaces_(config.route_metadata_context_namespaces().begin(), - config.route_metadata_context_namespaces().end()), - route_typed_metadata_context_namespaces_( - config.route_typed_metadata_context_namespaces().begin(), - config.route_typed_metadata_context_namespaces().end()), - include_peer_certificate_(config.include_peer_certificate()), - include_tls_session_(config.include_tls_session()), - charge_cluster_response_stats_( - PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, charge_cluster_response_stats, true)), - stats_(generateStats(stats_prefix, config.stat_prefix(), scope)), - ext_authz_ok_(pool_.add(createPoolStatName(config.stat_prefix(), "ok"))), - ext_authz_denied_(pool_.add(createPoolStatName(config.stat_prefix(), "denied"))), - ext_authz_error_(pool_.add(createPoolStatName(config.stat_prefix(), "error"))), - ext_authz_invalid_(pool_.add(createPoolStatName(config.stat_prefix(), "invalid"))), - ext_authz_failure_mode_allowed_( - pool_.add(createPoolStatName(config.stat_prefix(), "failure_mode_allowed"))) { - auto bootstrap = factory_context.bootstrap(); - auto labels_key_it = - bootstrap.node().metadata().fields().find(config.bootstrap_metadata_labels_key()); - if (labels_key_it != bootstrap.node().metadata().fields().end()) { - for (const auto& labels_it : labels_key_it->second.struct_value().fields()) { - destination_labels_[labels_it.first] = labels_it.second.string_value(); - } - } - - if (config.has_allowed_headers() && - config.http_service().authorization_request().has_allowed_headers()) { - ExceptionUtil::throwEnvoyException("Invalid duplicate configuration for allowed_headers."); - } - - // An unset request_headers_matchers_ means that all client request headers are allowed through - // to the authz server; this is to preserve backwards compatibility when introducing - // allowlisting of request headers for gRPC authz servers. Pre-existing support is for - // HTTP authz servers only and defaults to blocking all but a few headers (i.e. Authorization, - // Method, Path and Host). - if (config.has_grpc_service() && config.has_allowed_headers()) { - allowed_headers_matcher_ = Filters::Common::ExtAuthz::CheckRequestUtils::toRequestMatchers( - config.allowed_headers(), false, factory_context); - } else if (config.has_http_service()) { - if (config.http_service().authorization_request().has_allowed_headers()) { - allowed_headers_matcher_ = Filters::Common::ExtAuthz::CheckRequestUtils::toRequestMatchers( - config.http_service().authorization_request().allowed_headers(), true, factory_context); - } else { - allowed_headers_matcher_ = Filters::Common::ExtAuthz::CheckRequestUtils::toRequestMatchers( - config.allowed_headers(), true, factory_context); - } - } - if (config.has_disallowed_headers()) { - disallowed_headers_matcher_ = Filters::Common::ExtAuthz::CheckRequestUtils::toRequestMatchers( - config.disallowed_headers(), false, factory_context); - } - } + Server::Configuration::ServerFactoryContext& factory_context); bool allowPartialMessage() const { return allow_partial_message_; } diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 11ab97a3c84c..5fe4728749ad 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -176,6 +176,50 @@ ProcessingMode allDisabledMode() { } // namespace +FilterConfig::FilterConfig( + const envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor& config, + const std::chrono::milliseconds message_timeout, const uint32_t max_message_timeout_ms, + Stats::Scope& scope, const std::string& stats_prefix, bool is_upstream, + Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder, + Server::Configuration::CommonFactoryContext& context) + : failure_mode_allow_(config.failure_mode_allow()), + route_cache_action_(config.route_cache_action()), message_timeout_(message_timeout), + max_message_timeout_ms_(max_message_timeout_ms), + stats_(generateStats(stats_prefix, config.stat_prefix(), scope)), + processing_mode_(config.processing_mode()), + mutation_checker_(config.mutation_rules(), context.regexEngine()), + filter_metadata_(config.filter_metadata()), + allow_mode_override_(config.allow_mode_override()), + disable_immediate_response_(config.disable_immediate_response()), + allowed_headers_(initHeaderMatchers(config.forward_rules().allowed_headers(), context)), + disallowed_headers_(initHeaderMatchers(config.forward_rules().disallowed_headers(), context)), + is_upstream_(is_upstream), + untyped_forwarding_namespaces_( + config.metadata_options().forwarding_namespaces().untyped().begin(), + config.metadata_options().forwarding_namespaces().untyped().end()), + typed_forwarding_namespaces_( + config.metadata_options().forwarding_namespaces().typed().begin(), + config.metadata_options().forwarding_namespaces().typed().end()), + untyped_receiving_namespaces_( + config.metadata_options().receiving_namespaces().untyped().begin(), + config.metadata_options().receiving_namespaces().untyped().end()), + expression_manager_(builder, context.localInfo(), config.request_attributes(), + config.response_attributes()), + immediate_mutation_checker_(context.regexEngine()), + thread_local_stream_manager_slot_(context.threadLocal().allocateSlot()) { + if (config.disable_clear_route_cache() && + (route_cache_action_ != + envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::DEFAULT)) { + throw EnvoyException("disable_clear_route_cache and route_cache_action can not " + "be set to none-default at the same time."); + } + if (config.disable_clear_route_cache()) { + route_cache_action_ = envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::RETAIN; + } + thread_local_stream_manager_slot_->set( + [](Envoy::Event::Dispatcher&) { return std::make_shared(); }); +} + void ExtProcLoggingInfo::recordGrpcCall( std::chrono::microseconds latency, Grpc::Status::GrpcStatus call_status, ProcessorState::CallbackState callback_state, diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 0c1bb164848b..d7d5d9bc2fde 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -167,46 +167,7 @@ class FilterConfig { const uint32_t max_message_timeout_ms, Stats::Scope& scope, const std::string& stats_prefix, bool is_upstream, Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder, - Server::Configuration::CommonFactoryContext& context) - : failure_mode_allow_(config.failure_mode_allow()), - route_cache_action_(config.route_cache_action()), message_timeout_(message_timeout), - max_message_timeout_ms_(max_message_timeout_ms), - stats_(generateStats(stats_prefix, config.stat_prefix(), scope)), - processing_mode_(config.processing_mode()), - mutation_checker_(config.mutation_rules(), context.regexEngine()), - filter_metadata_(config.filter_metadata()), - allow_mode_override_(config.allow_mode_override()), - disable_immediate_response_(config.disable_immediate_response()), - allowed_headers_(initHeaderMatchers(config.forward_rules().allowed_headers(), context)), - disallowed_headers_( - initHeaderMatchers(config.forward_rules().disallowed_headers(), context)), - is_upstream_(is_upstream), - untyped_forwarding_namespaces_( - config.metadata_options().forwarding_namespaces().untyped().begin(), - config.metadata_options().forwarding_namespaces().untyped().end()), - typed_forwarding_namespaces_( - config.metadata_options().forwarding_namespaces().typed().begin(), - config.metadata_options().forwarding_namespaces().typed().end()), - untyped_receiving_namespaces_( - config.metadata_options().receiving_namespaces().untyped().begin(), - config.metadata_options().receiving_namespaces().untyped().end()), - expression_manager_(builder, context.localInfo(), config.request_attributes(), - config.response_attributes()), - immediate_mutation_checker_(context.regexEngine()), - thread_local_stream_manager_slot_(context.threadLocal().allocateSlot()) { - if (config.disable_clear_route_cache() && - (route_cache_action_ != - envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::DEFAULT)) { - ExceptionUtil::throwEnvoyException("disable_clear_route_cache and route_cache_action can not " - "be set to none-default at the same time."); - } - if (config.disable_clear_route_cache()) { - route_cache_action_ = - envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::RETAIN; - } - thread_local_stream_manager_slot_->set( - [](Envoy::Event::Dispatcher&) { return std::make_shared(); }); - } + Server::Configuration::CommonFactoryContext& context); bool failureModeAllow() const { return failure_mode_allow_; } diff --git a/source/extensions/filters/network/mongo_proxy/bson_impl.cc b/source/extensions/filters/network/mongo_proxy/bson_impl.cc index 669f26e08917..48df02c2fa21 100644 --- a/source/extensions/filters/network/mongo_proxy/bson_impl.cc +++ b/source/extensions/filters/network/mongo_proxy/bson_impl.cc @@ -211,6 +211,11 @@ int32_t FieldImpl::byteSize() const { return 0; // for gcc } +void FieldImpl::checkType(Type type) const { + if (type_ != type) { + throw EnvoyException("invalid BSON field type cast"); + } +} void FieldImpl::encode(Buffer::Instance& output) const { output.add(&type_, sizeof(type_)); BufferHelper::writeCString(output, key_); diff --git a/source/extensions/filters/network/mongo_proxy/bson_impl.h b/source/extensions/filters/network/mongo_proxy/bson_impl.h index 38bc2b91bc29..c2df37e8fea9 100644 --- a/source/extensions/filters/network/mongo_proxy/bson_impl.h +++ b/source/extensions/filters/network/mongo_proxy/bson_impl.h @@ -151,11 +151,7 @@ class FieldImpl : public Field { Type type() const override { return type_; } private: - void checkType(Type type) const { - if (type_ != type) { - ExceptionUtil::throwEnvoyException("invalid BSON field type cast"); - } - } + void checkType(Type type) const; /** * All of the possible variadic values that a field can be. diff --git a/source/extensions/filters/udp/udp_proxy/config.cc b/source/extensions/filters/udp/udp_proxy/config.cc index d0ee38433434..58dec2bb53a0 100644 --- a/source/extensions/filters/udp/udp_proxy/config.cc +++ b/source/extensions/filters/udp/udp_proxy/config.cc @@ -102,7 +102,7 @@ UdpProxyFilterConfigImpl::UdpProxyFilterConfigImpl( if (use_original_src_ip_ && !Api::OsSysCallsSingleton::get().supportsIpTransparent( context.serverFactoryContext().options().localAddressIpVersion())) { - ExceptionUtil::throwEnvoyException( + throw EnvoyException( "The platform does not support either IP_TRANSPARENT or IPV6_TRANSPARENT. Or the envoy " "is not running with the CAP_NET_ADMIN capability."); } diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 6b8d18420dca..0f2c72155e28 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -266,7 +266,7 @@ TEST(StringUtil, WhitespaceChars) { TEST(StringUtil, itoa) { char buf[32]; - EXPECT_THROW(StringUtil::itoa(buf, 20, 1), std::invalid_argument); + EXPECT_ENVOY_BUG(EXPECT_EQ(StringUtil::itoa(buf, 20, 1), 0), "itoa buffer too small"); EXPECT_EQ(1UL, StringUtil::itoa(buf, sizeof(buf), 0)); EXPECT_STREQ("0", buf); From 5791be667652676665228789d9c2186cdcb2a2a4 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Thu, 13 Jun 2024 08:39:11 -0400 Subject: [PATCH 12/21] runtime: oauth_make_token_cookie_httponly (#34672) Signed-off-by: Alyssa Wilk --- changelogs/current.yaml | 3 ++ source/common/runtime/runtime_features.cc | 1 - .../extensions/filters/http/oauth2/filter.cc | 10 +--- .../filters/http/oauth2/filter_test.cc | 49 ------------------- 4 files changed, 5 insertions(+), 58 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 622f9c4c0e2a..cbcead927b85 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -204,6 +204,9 @@ removed_config_or_runtime: - area: http change: | Removed ``envoy.reloadable_features.http_allow_partial_urls_in_referer`` runtime flag and legacy code paths. +- area: oauth + change: | + Removed ``envoy.reloadable_features.oauth_make_token_cookie_httponly`` runtime flag and legacy code paths. - area: http change: | Removed ``envoy.reloadable_features.lowercase_scheme`` runtime flag and legacy code paths. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index d4901982e08a..bb6806e58b46 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -67,7 +67,6 @@ RUNTIME_GUARD(envoy_reloadable_features_immediate_response_use_filter_mutation_r RUNTIME_GUARD(envoy_reloadable_features_no_downgrade_to_canonical_name); RUNTIME_GUARD(envoy_reloadable_features_no_extension_lookup_by_name); RUNTIME_GUARD(envoy_reloadable_features_normalize_host_for_preresolve_dfp_dns); -RUNTIME_GUARD(envoy_reloadable_features_oauth_make_token_cookie_httponly); RUNTIME_GUARD(envoy_reloadable_features_oauth_use_standard_max_age_value); RUNTIME_GUARD(envoy_reloadable_features_oauth_use_url_encoding); RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout); diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc index d5d97279cf7c..0824ae3bdd57 100644 --- a/source/extensions/filters/http/oauth2/filter.cc +++ b/source/extensions/filters/http/oauth2/filter.cc @@ -40,7 +40,6 @@ Http::RegisterCustomInlineHeadercookieNames(); @@ -697,17 +695,13 @@ void OAuth2Filter::addResponseCookies(Http::ResponseHeaderMap& headers, // If opted-in, we also create a new Bearer cookie for the authorization token provided by the // auth server. if (config_->forwardBearerToken()) { - std::string cookie_attribute_httponly = - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.oauth_make_token_cookie_httponly") - ? cookie_tail_http_only - : cookie_tail; headers.addReferenceKey( Http::Headers::get().SetCookie, - absl::StrCat(cookie_names.bearer_token_, "=", access_token_, cookie_attribute_httponly)); + absl::StrCat(cookie_names.bearer_token_, "=", access_token_, cookie_tail_http_only)); if (!id_token_.empty()) { headers.addReferenceKey( Http::Headers::get().SetCookie, - absl::StrCat(cookie_names.id_token_, "=", id_token_, cookie_attribute_httponly)); + absl::StrCat(cookie_names.id_token_, "=", id_token_, cookie_tail_http_only)); } if (!refresh_token_.empty()) { diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 78a49df1e710..8f9850f4cfdf 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -2151,55 +2151,6 @@ TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokens_oauth_use_standard_max_age_v std::chrono::seconds(600)); } -TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokens_oauth_make_token_cookie_httponly) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.oauth_make_token_cookie_httponly", "false"}, - }); - if (GetParam() == 1) { - oauthHMAC = - "ZTEzMmIyYzRmNTdmMTdiY2IyYmViZDE3ODA5ZDliOTE2MTRlNzNjYjc4MjBlMTVlOWY1OTM2ZjViZjM4MzAwNA==;"; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - } else { - oauthHMAC = "4TKyxPV/F7yyvr0XgJ2bkWFOc8t4IOFen1k29b84MAQ=;"; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - } - - // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. - test_time_.setSystemTime(SystemTime(std::chrono::seconds(1000))); - - // host_ must be set, which is guaranteed (ASAN). - Http::TestRequestHeaderMapImpl request_headers{ - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Path.get(), "/_signout"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - }; - filter_->decodeHeaders(request_headers, false); - - // Expected response after the callback is complete. - Http::TestRequestHeaderMapImpl expected_headers{ - {Http::Headers::get().Status.get(), "302"}, - {Http::Headers::get().SetCookie.get(), - "OauthHMAC=" + oauthHMAC + "path=/;Max-Age=600;secure;HttpOnly"}, - {Http::Headers::get().SetCookie.get(), - "OauthExpires=1600;path=/;Max-Age=600;secure;HttpOnly"}, - {Http::Headers::get().SetCookie.get(), "BearerToken=access_code;path=/;Max-Age=600;secure"}, - {Http::Headers::get().SetCookie.get(), "IdToken=some-id-token;path=/;Max-Age=600;secure"}, - {Http::Headers::get().SetCookie.get(), - "RefreshToken=some-refresh-token;path=/;Max-Age=600;secure;HttpOnly"}, - {Http::Headers::get().Location.get(), ""}, - }; - - EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_headers), true)); - - filter_->onGetAccessTokenSuccess("access_code", "some-id-token", "some-refresh-token", - std::chrono::seconds(600)); -} - TEST_F(OAuth2Test, OAuthBearerTokenFlowFromHeader) { Http::TestRequestHeaderMapImpl request_headers{ {Http::Headers::get().Path.get(), "/test?role=bearer"}, From b1ba2f1bd012c70555e05f0fb8d326035c2520b4 Mon Sep 17 00:00:00 2001 From: Namrata Bhave Date: Thu, 13 Jun 2024 19:21:36 +0530 Subject: [PATCH 13/21] Fix //test/common/common:bit_array_test on s390x (#34441) Fix bit_array Test on big endian Signed-off-by: namrata-ibm --- source/common/common/bit_array.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/common/bit_array.h b/source/common/common/bit_array.h index c4aced97dac3..9a6dff4a2989 100644 --- a/source/common/common/bit_array.h +++ b/source/common/common/bit_array.h @@ -129,13 +129,14 @@ class BitArray { } static inline void storeUnsignedWord(void* destination, uint64_t value) { + value = htole64(value); safeMemcpyUnsafeDst(destination, &value); } static inline uint64_t loadUnsignedWord(const void* source) { uint64_t destination; safeMemcpyUnsafeSrc(&destination, source); - return destination; + return le64toh(destination); } // Backing storage for the underlying array of bits. From b0f4332867267913d9aa80c5c0befda14a00d826 Mon Sep 17 00:00:00 2001 From: code Date: Fri, 14 Jun 2024 00:07:49 +0800 Subject: [PATCH 14/21] generic proxy: filter chain support for local reply and refactored the stream flags (#34730) * generic proxy: filter chain support for local reply and refactored the stream flags Signed-off-by: wbpcode * generic proxy: minor update the dubbo codec to get flags from metadata Signed-off-by: wbpcode --------- Signed-off-by: wbpcode Co-authored-by: wbpcode --- .../network/source/codecs/dubbo/config.h | 37 +- .../network/source/codecs/http1/config.h | 14 +- .../network/source/codecs/kafka/config.h | 5 +- .../filters/network/source/interface/stream.h | 116 ++---- .../filters/network/source/proxy.cc | 50 ++- .../filters/network/source/router/router.cc | 13 +- .../filters/network/source/router/router.h | 2 +- .../network/test/codecs/http1/config_test.cc | 4 +- .../filters/network/test/fake_codec.h | 27 +- .../filters/network/test/integration_test.cc | 364 +++++++++--------- .../filters/network/test/proxy_test.cc | 266 +++++++++++-- .../network/test/router/router_test.cc | 13 +- .../network/test/router/upstream_test.cc | 26 +- 13 files changed, 596 insertions(+), 341 deletions(-) diff --git a/contrib/generic_proxy/filters/network/source/codecs/dubbo/config.h b/contrib/generic_proxy/filters/network/source/codecs/dubbo/config.h index c9b4da229373..379784363c15 100644 --- a/contrib/generic_proxy/filters/network/source/codecs/dubbo/config.h +++ b/contrib/generic_proxy/filters/network/source/codecs/dubbo/config.h @@ -28,6 +28,16 @@ class DubboRequest : public Request { ASSERT(inner_metadata_ != nullptr); ASSERT(inner_metadata_->hasContext()); ASSERT(inner_metadata_->hasRequest()); + + uint32_t frame_flags = FrameFlags::FLAG_END_STREAM; // Dubbo message only has one frame. + if (!inner_metadata_->context().isTwoWay()) { + frame_flags |= FrameFlags::FLAG_ONE_WAY; + } + if (inner_metadata_->context().heartbeat()) { + frame_flags |= FrameFlags::FLAG_HEARTBEAT; + } + + stream_frame_flags_ = {static_cast(inner_metadata_->requestId()), frame_flags}; } // Request @@ -43,9 +53,10 @@ class DubboRequest : public Request { // StreamFrame FrameFlags frameFlags() const override { return stream_frame_flags_; } - FrameFlags stream_frame_flags_; - Common::Dubbo::MessageMetadataSharedPtr inner_metadata_; + +private: + FrameFlags stream_frame_flags_; }; class DubboResponse : public Response { @@ -56,6 +67,16 @@ class DubboResponse : public Response { ASSERT(inner_metadata_->hasContext()); ASSERT(inner_metadata_->hasResponse()); refreshStatus(); + + uint32_t frame_flags = FrameFlags::FLAG_END_STREAM; // Dubbo message only has one frame. + if (!inner_metadata_->context().isTwoWay()) { + frame_flags |= FrameFlags::FLAG_ONE_WAY; + } + if (inner_metadata_->context().heartbeat()) { + frame_flags |= FrameFlags::FLAG_HEARTBEAT; + } + + stream_frame_flags_ = {static_cast(inner_metadata_->requestId()), frame_flags}; } void refreshStatus(); @@ -67,10 +88,11 @@ class DubboResponse : public Response { // StreamFrame FrameFlags frameFlags() const override { return stream_frame_flags_; } - FrameFlags stream_frame_flags_; - StreamStatus status_; Common::Dubbo::MessageMetadataSharedPtr inner_metadata_; + +private: + FrameFlags stream_frame_flags_; }; class DubboCodecBase : public Logger::Loggable { @@ -136,12 +158,9 @@ class DubboDecoderBase : public DubboCodecBase, public CodecType { return Common::Dubbo::DecodeStatus::Success; } - auto message = std::make_unique(metadata_); - message->stream_frame_flags_ = {{static_cast(metadata_->requestId()), - !metadata_->context().isTwoWay(), false, - metadata_->context().heartbeat()}, - true}; + auto message = std::make_unique(std::move(metadata_)); metadata_.reset(); + callback_->onDecodingSuccess(std::move(message)); return Common::Dubbo::DecodeStatus::Success; diff --git a/contrib/generic_proxy/filters/network/source/codecs/http1/config.h b/contrib/generic_proxy/filters/network/source/codecs/http1/config.h index 253665c62926..f96223bcb35b 100644 --- a/contrib/generic_proxy/filters/network/source/codecs/http1/config.h +++ b/contrib/generic_proxy/filters/network/source/codecs/http1/config.h @@ -60,7 +60,7 @@ class HttpRequestFrame : public HttpHeaderFrame { HttpRequestFrame(Http::RequestHeaderMapPtr request, bool end_stream) : request_(std::move(request)) { ASSERT(request_ != nullptr); - frame_flags_ = {StreamFlags{}, end_stream}; + frame_flags_ = {0, end_stream ? FrameFlags::FLAG_END_STREAM : FrameFlags::FLAG_EMPTY}; } absl::string_view host() const override { return request_->getHostValue(); } @@ -80,7 +80,15 @@ class HttpResponseFrame : public HttpHeaderFrame { const bool drain_close = Envoy::StringUtil::caseFindToken( response_->getConnectionValue(), ",", Http::Headers::get().ConnectionValues.Close); - frame_flags_ = {StreamFlags{0, false, drain_close, false}, end_stream}; + uint32_t flags = 0; + if (end_stream) { + flags |= FrameFlags::FLAG_END_STREAM; + } + if (drain_close) { + flags |= FrameFlags::FLAG_DRAIN_CLOSE; + } + + frame_flags_ = {0, flags}; } StreamStatus status() const override { @@ -101,7 +109,7 @@ class HttpResponseFrame : public HttpHeaderFrame { class HttpRawBodyFrame : public CommonFrame { public: HttpRawBodyFrame(Envoy::Buffer::Instance& buffer, bool end_stream) - : frame_flags_({StreamFlags{}, end_stream}) { + : frame_flags_(0, end_stream ? FrameFlags::FLAG_END_STREAM : FrameFlags::FLAG_EMPTY) { buffer_.move(buffer); } FrameFlags frameFlags() const override { return frame_flags_; } diff --git a/contrib/generic_proxy/filters/network/source/codecs/kafka/config.h b/contrib/generic_proxy/filters/network/source/codecs/kafka/config.h index 1c8e72aa6d76..b4b9f9d65ff6 100644 --- a/contrib/generic_proxy/filters/network/source/codecs/kafka/config.h +++ b/contrib/generic_proxy/filters/network/source/codecs/kafka/config.h @@ -28,8 +28,7 @@ class KafkaRequestFrame : public GenericProxy::StreamRequest { if (request_ == nullptr) { return FrameFlags{}; } - return FrameFlags{ - StreamFlags{static_cast(request_->request_header_.correlation_id_)}}; + return FrameFlags{static_cast(request_->request_header_.correlation_id_)}; } absl::string_view protocol() const override { return "kafka"; } @@ -46,7 +45,7 @@ class KafkaResponseFrame : public GenericProxy::StreamResponse { if (response_ == nullptr) { return FrameFlags{}; } - return FrameFlags{StreamFlags{static_cast(response_->metadata_.correlation_id_)}}; + return FrameFlags{static_cast(response_->metadata_.correlation_id_)}; } absl::string_view protocol() const override { return "kafka"; } diff --git a/contrib/generic_proxy/filters/network/source/interface/stream.h b/contrib/generic_proxy/filters/network/source/interface/stream.h index ccc5e1d169ce..41e66ca7c865 100644 --- a/contrib/generic_proxy/filters/network/source/interface/stream.h +++ b/contrib/generic_proxy/filters/network/source/interface/stream.h @@ -17,92 +17,35 @@ namespace Extensions { namespace NetworkFilters { namespace GenericProxy { -/** - * Stream flags from request or response to control the behavior of the - * generic proxy filter. This is mainly used as part of FrameFlags. - * All these flags could be ignored for the simple ping-pong use case. - */ -class StreamFlags { -public: - StreamFlags(uint64_t stream_id = 0, bool one_way_stream = false, bool drain_close = false, - bool is_heartbeat = false) - : stream_id_(stream_id), one_way_stream_(one_way_stream), drain_close_(drain_close), - is_heartbeat_(is_heartbeat) {} - - /** - * @return the stream id of the request or response. This is used to match the - * downstream request with the upstream response. - - * NOTE: In most cases, the stream id is not needed and will be ignored completely. - * The stream id is only used when we can't match the downstream request - * with the upstream response by the active stream instance self directly. - * For example, when the multiple downstream requests are multiplexed into one - * upstream connection. - */ - uint64_t streamId() const { return stream_id_; } - - /** - * @return whether the stream is one way stream. If request is one way stream, the - * generic proxy filter will not wait for the response from the upstream. - */ - bool oneWayStream() const { return one_way_stream_; } - - /** - * @return whether the downstream/upstream connection should be drained after - * current active stream are finished. - */ - bool drainClose() const { return drain_close_; } - - /** - * @return whether the current request/response is a heartbeat request/response. - * NOTE: It would be better to handle heartbeat request/response by another L4 - * filter. Then the generic proxy filter can be used for the simple ping-pong - * use case. - */ - bool isHeartbeat() const { return is_heartbeat_; } - -private: - uint64_t stream_id_{0}; - - bool one_way_stream_{false}; - bool drain_close_{false}; - bool is_heartbeat_{false}; -}; - /** * Flags of stream frame. This is used to control the behavior of the generic proxy filter. * All these flags could be ignored for the simple ping-pong use case. */ class FrameFlags { public: + static constexpr uint32_t FLAG_EMPTY = 0x0000; + static constexpr uint32_t FLAG_END_STREAM = 0x0001; + static constexpr uint32_t FLAG_ONE_WAY = 0x0002; + static constexpr uint32_t FLAG_DRAIN_CLOSE = 0x0004; + static constexpr uint32_t FLAG_HEARTBEAT = 0x0008; + /** * Construct FrameFlags with stream flags and end stream flag. The stream flags MUST be * same for all frames of the same stream. - * @param stream_flags StreamFlags of the stream. - * @param end_stream whether the current frame is the last frame of the request or response. + * @param stream_id the stream id of the request or response. + * @param flags flags of the current frame. Only the flags that defined in FrameFlags + * could be used. Multiple flags could be combined by bitwise OR. * @param frame_tags frame tags of the current frame. The meaning of the frame tags is * application protocol specific. */ - FrameFlags(StreamFlags stream_flags = StreamFlags(), bool end_stream = true, - uint32_t frame_tags = 0) - : stream_flags_(stream_flags), frame_tags_(frame_tags), end_stream_(end_stream) {} + FrameFlags(uint64_t stream_id = 0, uint32_t flags = FLAG_END_STREAM, uint32_t frame_tags = 0) + : stream_id_(stream_id), flags_(flags), frame_tags_(frame_tags) {} /** - * Get flags of stream that the frame belongs to. The flags MUST be same for all frames of the - * same stream. Copy semantics is used because the flags are lightweight (only 16 bytes for now). - * @return StreamFlags of the stream. + * @return the stream id of the request or response. All frames of the same stream + * MUST have the same stream id. */ - StreamFlags streamFlags() const { return stream_flags_; } - - /** - * @return the stream id of the request or response. - */ - uint64_t streamId() const { return stream_flags_.streamId(); } - - /** - * @return whether the current frame is the last frame of the request or response. - */ - bool endStream() const { return end_stream_; } + uint64_t streamId() const { return stream_id_; } /** * @return frame tags of the current frame. The meaning of the frame tags is application @@ -114,12 +57,35 @@ class FrameFlags { */ uint32_t frameTags() const { return frame_tags_; } -private: - StreamFlags stream_flags_{}; + /** + * @return whether the current frame is the last frame of the request or response. + */ + bool endStream() const { return flags_ & FLAG_END_STREAM; } + /** + * @return whether the downstream/upstream connection should be drained after + * current active stream are finished. + * NOTE: Only the response header frame's drainClose() flag will be used. + */ + bool drainClose() const { return flags_ & FLAG_DRAIN_CLOSE; } + + /** + * @return whether the stream is one way stream. If request is one way stream, the + * generic proxy filter will not wait for the response from the upstream. + * NOTE: Only the request header frame's oneWayStream() flag will be used. + */ + bool oneWayStream() const { return flags_ & FLAG_ONE_WAY; } + + /** + * @return whether the current request/response is a heartbeat request/response. + * NOTE: Only the header frame's isHeartbeat() flag will be used. + */ + bool isHeartbeat() const { return flags_ & FLAG_HEARTBEAT; } + +private: + uint64_t stream_id_{}; + uint32_t flags_{}; uint32_t frame_tags_{}; - // Default to true for backward compatibility. - bool end_stream_{true}; }; /** diff --git a/contrib/generic_proxy/filters/network/source/proxy.cc b/contrib/generic_proxy/filters/network/source/proxy.cc index c898f9df5b46..310058f54aaa 100644 --- a/contrib/generic_proxy/filters/network/source/proxy.cc +++ b/contrib/generic_proxy/filters/network/source/proxy.cc @@ -144,11 +144,27 @@ bool ActiveStream::sendFrameToDownstream(const StreamFrame& frame, bool header_f return true; } -// TODO(wbpcode): add the short_response_flags support to the sendLocalReply -// method. void ActiveStream::sendLocalReply(Status status, absl::string_view data, ResponseUpdateFunction func) { - // Clear possible response frames. + // To ensure only one response header frame is sent for local reply. + // If there is a previsous response header but has not been sent to the downstream, + // we should send the latest local reply to the downstream directly without any + // filter processing. + // If the previous response header has been sent to the downstream, it is an invalid + // situation that we cannot handle. Then, we should reset the stream and return. + // In other cases, we should continue the filter chain to process the local reply. + + const bool has_prev_response = response_header_frame_ != nullptr; + const bool has_sent_response = + has_prev_response && encoder_filter_iter_header_ == encoder_filters_.end(); + + if (has_sent_response) { + ENVOY_LOG(error, "Generic proxy: invalid local reply: response is already sent."); + resetStream(DownstreamStreamResetReason::ProtocolError); + return; + } + + // Clear possible response frames anyway now. response_header_frame_ = nullptr; response_common_frames_.clear(); @@ -167,14 +183,20 @@ void ActiveStream::sendLocalReply(Status status, absl::string_view data, } response_header_frame_ = std::move(response); + parent_.stream_drain_decision_ |= response_header_frame_->frameFlags().drainClose(); local_reply_ = true; // status message will be used as response code details. stream_info_.setResponseCodeDetails(status.message()); // Set the response code to the stream info. stream_info_.setResponseCode(response_header_frame_->status().code()); - // Send the response header frame to downstream. - sendFrameToDownstream(*response_header_frame_, true); + if (has_prev_response) { + // There was a previous response. Send the new response to the downstream directly. + sendFrameToDownstream(*response_header_frame_, true); + return; + } else { + continueEncoding(); + } } void ActiveStream::processRequestHeaderFrame() { @@ -277,7 +299,7 @@ void ActiveStream::processResponseHeaderFrame() { response_header_frame_->frameFlags().endStream()); // Send the header frame to downstream. - if (!sendFrameToDownstream(*response_header_frame_, false)) { + if (!sendFrameToDownstream(*response_header_frame_, true)) { stop_encoder_filter_chain_ = true; } }; @@ -465,15 +487,25 @@ void ActiveStream::onRequestCommonFrame(RequestCommonFramePtr request_common_fra } void ActiveStream::onResponseHeaderFrame(ResponsePtr response) { - ASSERT(response_header_frame_ == nullptr); + // To ensure only one response header frame is handled for upstream response. + // If there is a previous response header frame, no matter it is come from + // the upstream or the local reply, it is an invalid situation that we cannot + // handle. Then, we should reset the stream and return. + const bool has_prev_response = response_header_frame_ != nullptr; + if (has_prev_response) { + ENVOY_LOG(error, "Generic proxy: invalid response: has previous response."); + resetStream(DownstreamStreamResetReason::ProtocolError); + return; + } + response_header_frame_ = std::move(response); - ASSERT(response_header_frame_ != nullptr); - parent_.stream_drain_decision_ = response_header_frame_->frameFlags().streamFlags().drainClose(); + parent_.stream_drain_decision_ |= response_header_frame_->frameFlags().drainClose(); // The response code details always be "via_upstream" for response from upstream. stream_info_.setResponseCodeDetails("via_upstream"); // Set the response code to the stream info. stream_info_.setResponseCode(response_header_frame_->status().code()); + continueEncoding(); } diff --git a/contrib/generic_proxy/filters/network/source/router/router.cc b/contrib/generic_proxy/filters/network/source/router/router.cc index 52f9d56e161f..8ebf60642c46 100644 --- a/contrib/generic_proxy/filters/network/source/router/router.cc +++ b/contrib/generic_proxy/filters/network/source/router/router.cc @@ -40,12 +40,13 @@ ReasonViewAndFlag resetReasonToViewAndFlag(StreamResetReason reason) { } // namespace -UpstreamRequest::UpstreamRequest(RouterFilter& parent, StreamFlags stream_flags, +UpstreamRequest::UpstreamRequest(RouterFilter& parent, FrameFlags header_frame_flags, GenericUpstreamSharedPtr generic_upstream) : parent_(parent), generic_upstream_(std::move(generic_upstream)), stream_info_(parent.time_source_, nullptr, StreamInfo::FilterState::LifeSpan::FilterChain), upstream_info_(std::make_shared()), - stream_id_(stream_flags.streamId()), expects_response_(!stream_flags.oneWayStream()) { + stream_id_(header_frame_flags.streamId()), + expects_response_(!header_frame_flags.oneWayStream()) { // Host is known at this point and set the initial upstream host. onUpstreamHostSelected(generic_upstream_->upstreamHost()); @@ -244,7 +245,7 @@ void UpstreamRequest::onDecodingSuccess(ResponseHeaderFramePtr response_header_f } if (response_header_frame->frameFlags().endStream()) { - onUpstreamResponseComplete(response_header_frame->frameFlags().streamFlags().drainClose()); + onUpstreamResponseComplete(response_header_frame->frameFlags().drainClose()); } parent_.onResponseHeaderFrame(std::move(response_header_frame)); @@ -258,7 +259,7 @@ void UpstreamRequest::onDecodingSuccess(ResponseCommonFramePtr response_common_f } if (response_common_frame->frameFlags().endStream()) { - onUpstreamResponseComplete(response_common_frame->frameFlags().streamFlags().drainClose()); + onUpstreamResponseComplete(response_common_frame->frameFlags().drainClose()); } parent_.onResponseCommonFrame(std::move(response_common_frame)); @@ -448,8 +449,8 @@ void RouterFilter::kickOffNewUpstreamRequest() { return; } - auto upstream_request = std::make_unique( - *this, request_stream_->frameFlags().streamFlags(), std::move(generic_upstream)); + auto upstream_request = std::make_unique(*this, request_stream_->frameFlags(), + std::move(generic_upstream)); auto raw_upstream_request = upstream_request.get(); LinkedList::moveIntoList(std::move(upstream_request), upstream_requests_); raw_upstream_request->startStream(); diff --git a/contrib/generic_proxy/filters/network/source/router/router.h b/contrib/generic_proxy/filters/network/source/router/router.h index fcc71457d6c5..a2effd2c1db6 100644 --- a/contrib/generic_proxy/filters/network/source/router/router.h +++ b/contrib/generic_proxy/filters/network/source/router/router.h @@ -48,7 +48,7 @@ class UpstreamRequest : public UpstreamRequestCallbacks, public EncodingContext, Logger::Loggable { public: - UpstreamRequest(RouterFilter& parent, StreamFlags stream_flags, + UpstreamRequest(RouterFilter& parent, FrameFlags header_frame_flags, GenericUpstreamSharedPtr generic_upstream); void startStream(); diff --git a/contrib/generic_proxy/filters/network/test/codecs/http1/config_test.cc b/contrib/generic_proxy/filters/network/test/codecs/http1/config_test.cc index a9e2dcbdd784..5441bd19ddfe 100644 --- a/contrib/generic_proxy/filters/network/test/codecs/http1/config_test.cc +++ b/contrib/generic_proxy/filters/network/test/codecs/http1/config_test.cc @@ -132,14 +132,14 @@ TEST(Http1MessageFrameTest, Http1MessageFrameTest) { HttpResponseFrame frame(std::move(headers), true); EXPECT_EQ(true, frame.frameFlags().endStream()); - EXPECT_EQ(false, frame.frameFlags().streamFlags().drainClose()); + EXPECT_EQ(false, frame.frameFlags().drainClose()); auto headers2 = Http::ResponseHeaderMapImpl::create(); headers2->setConnection("close"); HttpResponseFrame frame2(std::move(headers2), false); EXPECT_EQ(false, frame2.frameFlags().endStream()); - EXPECT_EQ(true, frame2.frameFlags().streamFlags().drainClose()); + EXPECT_EQ(true, frame2.frameFlags().drainClose()); } // Request FrameType. diff --git a/contrib/generic_proxy/filters/network/test/fake_codec.h b/contrib/generic_proxy/filters/network/test/fake_codec.h index e82055f5e6c0..b03e1a61f476 100644 --- a/contrib/generic_proxy/filters/network/test/fake_codec.h +++ b/contrib/generic_proxy/filters/network/test/fake_codec.h @@ -115,8 +115,15 @@ class FakeStreamCodecFactory : public CodecFactory { data.erase("one_way"); } - auto frame_flags = - FrameFlags(StreamFlags(stream_id.value_or(0), one_way_stream, false, false), end_stream); + uint32_t flags = 0; + if (end_stream) { + flags |= FrameFlags::FLAG_END_STREAM; + } + if (one_way_stream) { + flags |= FrameFlags::FLAG_ONE_WAY; + } + + FrameFlags frame_flags(stream_id.value_or(0), flags); const auto it = data.find("message_type"); @@ -208,8 +215,7 @@ class FakeStreamCodecFactory : public CodecFactory { buffer += fmt::format("stream_id:{};", response.frameFlags().streamId()); buffer += fmt::format("end_stream:{};", response.frameFlags().endStream()); - buffer += - fmt::format("close_connection:{};", response.frameFlags().streamFlags().drainClose()); + buffer += fmt::format("close_connection:{};", response.frameFlags().drainClose()); ENVOY_LOG(debug, "FakeServerCodec::encode: {}", buffer); @@ -274,8 +280,15 @@ class FakeStreamCodecFactory : public CodecFactory { data.erase("close_connection"); } - auto frame_flags = FrameFlags( - StreamFlags(stream_id.value_or(0), false, close_connection, false), end_stream); + uint32_t flags = 0; + if (end_stream) { + flags |= FrameFlags::FLAG_END_STREAM; + } + if (close_connection) { + flags |= FrameFlags::FLAG_DRAIN_CLOSE; + } + + FrameFlags frame_flags(stream_id.value_or(0), flags); const auto it = data.find("message_type"); @@ -371,7 +384,7 @@ class FakeStreamCodecFactory : public CodecFactory { buffer += fmt::format("stream_id:{};", request.frameFlags().streamId()); buffer += fmt::format("end_stream:{};", request.frameFlags().endStream()); - buffer += fmt::format("one_way:{};", request.frameFlags().streamFlags().oneWayStream()); + buffer += fmt::format("one_way:{};", request.frameFlags().oneWayStream()); ENVOY_LOG(debug, "FakeClientCodec::encode: {}", buffer); diff --git a/contrib/generic_proxy/filters/network/test/integration_test.cc b/contrib/generic_proxy/filters/network/test/integration_test.cc index 41e10d2bede6..f6c441f88f6a 100644 --- a/contrib/generic_proxy/filters/network/test/integration_test.cc +++ b/contrib/generic_proxy/filters/network/test/integration_test.cc @@ -425,235 +425,235 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, IntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); -// TEST_P(IntegrationTest, InitializeInstance) { -// FakeStreamCodecFactoryConfig codec_factory_config; -// Registry::InjectFactory registration(codec_factory_config); +TEST_P(IntegrationTest, InitializeInstance) { + FakeStreamCodecFactoryConfig codec_factory_config; + Registry::InjectFactory registration(codec_factory_config); -// initialize(defaultConfig(), std::make_unique()); -// } + initialize(defaultConfig(), std::make_unique()); +} -// TEST_P(IntegrationTest, RequestRouteNotFound) { -// FakeStreamCodecFactoryConfig codec_factory_config; -// Registry::InjectFactory registration(codec_factory_config); +TEST_P(IntegrationTest, RequestRouteNotFound) { + FakeStreamCodecFactoryConfig codec_factory_config; + Registry::InjectFactory registration(codec_factory_config); -// initialize(defaultConfig(), std::make_unique()); -// EXPECT_TRUE(makeClientConnectionForTest()); + initialize(defaultConfig(), std::make_unique()); + EXPECT_TRUE(makeClientConnectionForTest()); -// FakeStreamCodecFactory::FakeRequest request; -// request.host_ = "service_name_0"; -// request.method_ = "hello"; -// request.path_ = "/path_or_anything"; -// request.protocol_ = "fake_fake_fake"; -// request.data_ = {{"version", "v2"}}; + FakeStreamCodecFactory::FakeRequest request; + request.host_ = "service_name_0"; + request.method_ = "hello"; + request.path_ = "/path_or_anything"; + request.protocol_ = "fake_fake_fake"; + request.data_ = {{"version", "v2"}}; -// sendRequestForTest(request); + sendRequestForTest(request); -// RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 0), -// "unexpected timeout"); + RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 0), + "unexpected timeout"); -// EXPECT_NE(client_codec_callabcks_->responses_[0].response_, nullptr); -// EXPECT_EQ(client_codec_callabcks_->responses_[0].response_->status().code(), -// static_cast(absl::StatusCode::kNotFound)); + EXPECT_NE(client_codec_callabcks_->responses_[0].response_, nullptr); + EXPECT_EQ(client_codec_callabcks_->responses_[0].response_->status().code(), + static_cast(absl::StatusCode::kNotFound)); -// cleanup(); -// } + cleanup(); +} -// TEST_P(IntegrationTest, RequestAndResponse) { -// FakeStreamCodecFactoryConfig codec_factory_config; -// Registry::InjectFactory registration(codec_factory_config); +TEST_P(IntegrationTest, RequestAndResponse) { + FakeStreamCodecFactoryConfig codec_factory_config; + Registry::InjectFactory registration(codec_factory_config); -// initialize(defaultConfig(), std::make_unique()); + initialize(defaultConfig(), std::make_unique()); -// EXPECT_TRUE(makeClientConnectionForTest()); + EXPECT_TRUE(makeClientConnectionForTest()); -// FakeStreamCodecFactory::FakeRequest request; -// request.host_ = "service_name_0"; -// request.method_ = "hello"; -// request.path_ = "/path_or_anything"; -// request.protocol_ = "fake_fake_fake"; -// request.data_ = {{"version", "v1"}}; + FakeStreamCodecFactory::FakeRequest request; + request.host_ = "service_name_0"; + request.method_ = "hello"; + request.path_ = "/path_or_anything"; + request.protocol_ = "fake_fake_fake"; + request.data_ = {{"version", "v1"}}; -// sendRequestForTest(request); + sendRequestForTest(request); -// waitForUpstreamConnectionForTest(); -// const std::function data_validator = -// [](const std::string& data) -> bool { return data.find("v1") != std::string::npos; }; -// waitForUpstreamRequestForTest(data_validator); + waitForUpstreamConnectionForTest(); + const std::function data_validator = + [](const std::string& data) -> bool { return data.find("v1") != std::string::npos; }; + waitForUpstreamRequestForTest(data_validator); -// FakeStreamCodecFactory::FakeResponse response; -// response.protocol_ = "fake_fake_fake"; -// response.status_ = StreamStatus(); -// response.data_["zzzz"] = "xxxx"; + FakeStreamCodecFactory::FakeResponse response; + response.protocol_ = "fake_fake_fake"; + response.status_ = StreamStatus(); + response.data_["zzzz"] = "xxxx"; -// sendResponseForTest(response); + sendResponseForTest(response); -// RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 0), -// "unexpected timeout"); + RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 0), + "unexpected timeout"); + + EXPECT_NE(client_codec_callabcks_->responses_[0].response_, nullptr); + EXPECT_EQ(client_codec_callabcks_->responses_[0].response_->status().code(), 0); + EXPECT_EQ(client_codec_callabcks_->responses_[0].response_->get("zzzz"), "xxxx"); + + cleanup(); +} + +TEST_P(IntegrationTest, RequestTimeout) { + FakeStreamCodecFactoryConfig codec_factory_config; + Registry::InjectFactory registration(codec_factory_config); -// EXPECT_NE(client_codec_callabcks_->responses_[0].response_, nullptr); -// EXPECT_EQ(client_codec_callabcks_->responses_[0].response_->status().code(), 0); -// EXPECT_EQ(client_codec_callabcks_->responses_[0].response_->get("zzzz"), "xxxx"); + initialize(timeoutConfig(), std::make_unique()); -// cleanup(); -// } + EXPECT_TRUE(makeClientConnectionForTest()); -// TEST_P(IntegrationTest, RequestTimeout) { -// FakeStreamCodecFactoryConfig codec_factory_config; -// Registry::InjectFactory registration(codec_factory_config); + FakeStreamCodecFactory::FakeRequest request; + request.host_ = "service_name_0"; + request.method_ = "hello"; + request.path_ = "/path_or_anything"; + request.protocol_ = "fake_fake_fake"; + request.data_ = {{"version", "v1"}}; -// initialize(timeoutConfig(), std::make_unique()); + sendRequestForTest(request); -// EXPECT_TRUE(makeClientConnectionForTest()); + waitForUpstreamConnectionForTest(); + const std::function data_validator = + [](const std::string& data) -> bool { return data.find("v1") != std::string::npos; }; + waitForUpstreamRequestForTest(data_validator); -// FakeStreamCodecFactory::FakeRequest request; -// request.host_ = "service_name_0"; -// request.method_ = "hello"; -// request.path_ = "/path_or_anything"; -// request.protocol_ = "fake_fake_fake"; -// request.data_ = {{"version", "v1"}}; + // No response is sent, so the downstream should timeout. -// sendRequestForTest(request); + RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 0), + "unexpected timeout"); -// waitForUpstreamConnectionForTest(); -// const std::function data_validator = -// [](const std::string& data) -> bool { return data.find("v1") != std::string::npos; }; -// waitForUpstreamRequestForTest(data_validator); + EXPECT_NE(client_codec_callabcks_->responses_[0].response_, nullptr); + EXPECT_EQ(client_codec_callabcks_->responses_[0].response_->status().code(), 4); -// // No response is sent, so the downstream should timeout. + cleanup(); +} -// RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 0), -// "unexpected timeout"); +TEST_P(IntegrationTest, MultipleRequestsWithSameStreamId) { + FakeStreamCodecFactoryConfig codec_factory_config; + Registry::InjectFactory registration(codec_factory_config); -// EXPECT_NE(client_codec_callabcks_->responses_[0].response_, nullptr); -// EXPECT_EQ(client_codec_callabcks_->responses_[0].response_->status().code(), 4); + auto codec_factory = std::make_unique(); -// cleanup(); -// } + initialize(defaultConfig(true), std::move(codec_factory)); -// TEST_P(IntegrationTest, MultipleRequestsWithSameStreamId) { -// FakeStreamCodecFactoryConfig codec_factory_config; -// Registry::InjectFactory registration(codec_factory_config); + EXPECT_TRUE(makeClientConnectionForTest()); -// auto codec_factory = std::make_unique(); + FakeStreamCodecFactory::FakeRequest request_1; + request_1.host_ = "service_name_0"; + request_1.method_ = "hello"; + request_1.path_ = "/path_or_anything"; + request_1.protocol_ = "fake_fake_fake"; + request_1.data_ = {{"version", "v1"}}; + request_1.stream_frame_flags_ = FrameFlags(1); -// initialize(defaultConfig(true), std::move(codec_factory)); + sendRequestForTest(request_1); -// EXPECT_TRUE(makeClientConnectionForTest()); + waitForUpstreamConnectionForTest(); + const std::function data_validator = + [](const std::string& data) -> bool { return data.find("v1") != std::string::npos; }; + waitForUpstreamRequestForTest(data_validator); -// FakeStreamCodecFactory::FakeRequest request_1; -// request_1.host_ = "service_name_0"; -// request_1.method_ = "hello"; -// request_1.path_ = "/path_or_anything"; -// request_1.protocol_ = "fake_fake_fake"; -// request_1.data_ = {{"version", "v1"}}; -// request_1.stream_frame_flags_ = FrameFlags(StreamFlags(1)); + FakeStreamCodecFactory::FakeRequest request_2; + request_2.host_ = "service_name_0"; + request_2.method_ = "hello"; + request_2.path_ = "/path_or_anything"; + request_2.protocol_ = "fake_fake_fake"; + request_2.data_ = {{"version", "v1"}}; + request_2.stream_frame_flags_ = FrameFlags(1); -// sendRequestForTest(request_1); + // Send the second request with the same stream id and expect the connection to be closed. + sendRequestForTest(request_2); -// waitForUpstreamConnectionForTest(); -// const std::function data_validator = -// [](const std::string& data) -> bool { return data.find("v1") != std::string::npos; }; -// waitForUpstreamRequestForTest(data_validator); + // Wait for the connection to be closed. + auto result = upstream_connection_->waitForDisconnect(); + RELEASE_ASSERT(result, result.message()); -// FakeStreamCodecFactory::FakeRequest request_2; -// request_2.host_ = "service_name_0"; -// request_2.method_ = "hello"; -// request_2.path_ = "/path_or_anything"; -// request_2.protocol_ = "fake_fake_fake"; -// request_2.data_ = {{"version", "v1"}}; -// request_2.stream_frame_flags_ = FrameFlags(StreamFlags(1)); + cleanup(); +} -// // Send the second request with the same stream id and expect the connection to be closed. -// sendRequestForTest(request_2); +TEST_P(IntegrationTest, MultipleRequests) { + FakeStreamCodecFactoryConfig codec_factory_config; + Registry::InjectFactory registration(codec_factory_config); -// // Wait for the connection to be closed. -// auto result = upstream_connection_->waitForDisconnect(); -// RELEASE_ASSERT(result, result.message()); + auto codec_factory = std::make_unique(); -// cleanup(); -// } + initialize(defaultConfig(true), std::move(codec_factory)); -// TEST_P(IntegrationTest, MultipleRequests) { -// FakeStreamCodecFactoryConfig codec_factory_config; -// Registry::InjectFactory registration(codec_factory_config); + EXPECT_TRUE(makeClientConnectionForTest()); -// auto codec_factory = std::make_unique(); + FakeStreamCodecFactory::FakeRequest request_1; + request_1.host_ = "service_name_0"; + request_1.method_ = "hello"; + request_1.path_ = "/path_or_anything"; + request_1.protocol_ = "fake_fake_fake"; + request_1.data_ = {{"version", "v1"}, {"frame", "1_header"}}; + request_1.stream_frame_flags_ = FrameFlags(1); -// initialize(defaultConfig(true), std::move(codec_factory)); + sendRequestForTest(request_1); -// EXPECT_TRUE(makeClientConnectionForTest()); + waitForUpstreamConnectionForTest(); -// FakeStreamCodecFactory::FakeRequest request_1; -// request_1.host_ = "service_name_0"; -// request_1.method_ = "hello"; -// request_1.path_ = "/path_or_anything"; -// request_1.protocol_ = "fake_fake_fake"; -// request_1.data_ = {{"version", "v1"}, {"frame", "1_header"}}; -// request_1.stream_frame_flags_ = FrameFlags(StreamFlags(1)); + const std::function data_validator_1 = + [](const std::string& data) -> bool { + return data.find("frame:1_header") != std::string::npos; + }; + waitForUpstreamRequestForTest(data_validator_1); -// sendRequestForTest(request_1); + FakeStreamCodecFactory::FakeRequest request_2; + request_2.host_ = "service_name_0"; + request_2.method_ = "hello"; + request_2.path_ = "/path_or_anything"; + request_2.protocol_ = "fake_fake_fake"; + request_2.data_ = {{"version", "v1"}, {"frame", "2_header"}}; + request_2.stream_frame_flags_ = FrameFlags(2); -// waitForUpstreamConnectionForTest(); + // Reset request encoder callback. + test_encoding_context_ = std::make_shared(); -// const std::function data_validator_1 = -// [](const std::string& data) -> bool { -// return data.find("frame:1_header") != std::string::npos; -// }; -// waitForUpstreamRequestForTest(data_validator_1); + // Send the second request with the different stream id and expect the connection to be alive. + sendRequestForTest(request_2); + const std::function data_validator_2 = + [](const std::string& data) -> bool { + return data.find("frame:2_header") != std::string::npos; + }; + waitForUpstreamRequestForTest(data_validator_2); -// FakeStreamCodecFactory::FakeRequest request_2; -// request_2.host_ = "service_name_0"; -// request_2.method_ = "hello"; -// request_2.path_ = "/path_or_anything"; -// request_2.protocol_ = "fake_fake_fake"; -// request_2.data_ = {{"version", "v1"}, {"frame", "2_header"}}; -// request_2.stream_frame_flags_ = FrameFlags(StreamFlags(2)); + FakeStreamCodecFactory::FakeResponse response_2; + response_2.protocol_ = "fake_fake_fake"; + response_2.status_ = StreamStatus(); + response_2.data_["zzzz"] = "xxxx"; + response_2.stream_frame_flags_ = FrameFlags(2); -// // Reset request encoder callback. -// test_encoding_context_ = std::make_shared(); + sendResponseForTest(response_2); -// // Send the second request with the different stream id and expect the connection to be alive. -// sendRequestForTest(request_2); -// const std::function data_validator_2 = -// [](const std::string& data) -> bool { -// return data.find("frame:2_header") != std::string::npos; -// }; -// waitForUpstreamRequestForTest(data_validator_2); + RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 2), + "unexpected timeout"); -// FakeStreamCodecFactory::FakeResponse response_2; -// response_2.protocol_ = "fake_fake_fake"; -// response_2.status_ = StreamStatus(); -// response_2.data_["zzzz"] = "xxxx"; -// response_2.stream_frame_flags_ = FrameFlags(StreamFlags(2)); + EXPECT_NE(client_codec_callabcks_->responses_[2].response_, nullptr); + EXPECT_EQ(client_codec_callabcks_->responses_[2].response_->status().code(), 0); + EXPECT_EQ(client_codec_callabcks_->responses_[2].response_->get("zzzz"), "xxxx"); + EXPECT_EQ(client_codec_callabcks_->responses_[2].response_->frameFlags().streamId(), 2); -// sendResponseForTest(response_2); + FakeStreamCodecFactory::FakeResponse response_1; + response_1.protocol_ = "fake_fake_fake"; + response_1.status_ = StreamStatus(); + response_1.data_["zzzz"] = "yyyy"; + response_1.stream_frame_flags_ = FrameFlags(1); -// RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 2), -// "unexpected timeout"); + sendResponseForTest(response_1); -// EXPECT_NE(client_codec_callabcks_->responses_[2].response_, nullptr); -// EXPECT_EQ(client_codec_callabcks_->responses_[2].response_->status().code(), 0); -// EXPECT_EQ(client_codec_callabcks_->responses_[2].response_->get("zzzz"), "xxxx"); -// EXPECT_EQ(client_codec_callabcks_->responses_[2].response_->frameFlags().streamId(), 2); + RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 1), + "unexpected timeout"); -// FakeStreamCodecFactory::FakeResponse response_1; -// response_1.protocol_ = "fake_fake_fake"; -// response_1.status_ = StreamStatus(); -// response_1.data_["zzzz"] = "yyyy"; -// response_1.stream_frame_flags_ = FrameFlags(StreamFlags(1)); - -// sendResponseForTest(response_1); - -// RELEASE_ASSERT(waitDownstreamResponseForTest(TestUtility::DefaultTimeout, 1), -// "unexpected timeout"); + EXPECT_NE(client_codec_callabcks_->responses_[1].response_, nullptr); + EXPECT_EQ(client_codec_callabcks_->responses_[1].response_->status().code(), 0); + EXPECT_EQ(client_codec_callabcks_->responses_[1].response_->get("zzzz"), "yyyy"); + EXPECT_EQ(client_codec_callabcks_->responses_[1].response_->frameFlags().streamId(), 1); -// EXPECT_NE(client_codec_callabcks_->responses_[1].response_, nullptr); -// EXPECT_EQ(client_codec_callabcks_->responses_[1].response_->status().code(), 0); -// EXPECT_EQ(client_codec_callabcks_->responses_[1].response_->get("zzzz"), "yyyy"); -// EXPECT_EQ(client_codec_callabcks_->responses_[1].response_->frameFlags().streamId(), 1); - -// cleanup(); -// } + cleanup(); +} TEST_P(IntegrationTest, MultipleRequestsWithMultipleFrames) { FakeStreamCodecFactoryConfig codec_factory_config; @@ -671,15 +671,15 @@ TEST_P(IntegrationTest, MultipleRequestsWithMultipleFrames) { request_1.path_ = "/path_or_anything"; request_1.protocol_ = "fake_fake_fake"; request_1.data_ = {{"version", "v1"}, {"frame", "1_header"}}; - request_1.stream_frame_flags_ = FrameFlags(StreamFlags(1), false); + request_1.stream_frame_flags_ = FrameFlags(1, FrameFlags::FLAG_EMPTY); FakeStreamCodecFactory::FakeCommonFrame request_1_frame_1; request_1_frame_1.data_ = {{"frame", "1_frame_1"}}; - request_1_frame_1.stream_frame_flags_ = FrameFlags(StreamFlags(1), false); + request_1_frame_1.stream_frame_flags_ = FrameFlags(1, FrameFlags::FLAG_EMPTY); FakeStreamCodecFactory::FakeCommonFrame request_1_frame_2; request_1_frame_2.data_ = {{"frame", "1_frame_2"}}; - request_1_frame_2.stream_frame_flags_ = FrameFlags(StreamFlags(1), true); + request_1_frame_2.stream_frame_flags_ = FrameFlags(1); FakeStreamCodecFactory::FakeRequest request_2; request_2.host_ = "service_name_0"; @@ -687,15 +687,15 @@ TEST_P(IntegrationTest, MultipleRequestsWithMultipleFrames) { request_2.path_ = "/path_or_anything"; request_2.protocol_ = "fake_fake_fake"; request_2.data_ = {{"version", "v1"}, {"frame", "2_header"}}; - request_2.stream_frame_flags_ = FrameFlags(StreamFlags(2), false); + request_2.stream_frame_flags_ = FrameFlags(2, FrameFlags::FLAG_EMPTY); FakeStreamCodecFactory::FakeCommonFrame request_2_frame_1; request_2_frame_1.data_ = {{"frame", "2_frame_1"}}; - request_2_frame_1.stream_frame_flags_ = FrameFlags(StreamFlags(2), false); + request_2_frame_1.stream_frame_flags_ = FrameFlags(2, FrameFlags::FLAG_EMPTY); FakeStreamCodecFactory::FakeCommonFrame request_2_frame_2; request_2_frame_2.data_ = {{"frame", "2_frame_2"}}; - request_2_frame_2.stream_frame_flags_ = FrameFlags(StreamFlags(2), true); + request_2_frame_2.stream_frame_flags_ = FrameFlags(2); // We handle frame one by one to make sure the order is correct. @@ -753,10 +753,10 @@ TEST_P(IntegrationTest, MultipleRequestsWithMultipleFrames) { response_2.protocol_ = "fake_fake_fake"; response_2.status_ = StreamStatus(); response_2.data_["zzzz"] = "xxxx"; - response_2.stream_frame_flags_ = FrameFlags(StreamFlags(2), false); + response_2.stream_frame_flags_ = FrameFlags(2, FrameFlags::FLAG_EMPTY); FakeStreamCodecFactory::FakeCommonFrame response_2_frame_1; - response_2_frame_1.stream_frame_flags_ = FrameFlags(StreamFlags(2), true); + response_2_frame_1.stream_frame_flags_ = FrameFlags(2); sendResponseForTest(response_2); sendResponseForTest(response_2_frame_1); @@ -773,10 +773,10 @@ TEST_P(IntegrationTest, MultipleRequestsWithMultipleFrames) { response_1.protocol_ = "fake_fake_fake"; response_1.status_ = StreamStatus(); response_1.data_["zzzz"] = "yyyy"; - response_1.stream_frame_flags_ = FrameFlags(StreamFlags(1), false); + response_1.stream_frame_flags_ = FrameFlags(1, FrameFlags::FLAG_EMPTY); FakeStreamCodecFactory::FakeCommonFrame response_1_frame_1; - response_1_frame_1.stream_frame_flags_ = FrameFlags(StreamFlags(1), true); + response_1_frame_1.stream_frame_flags_ = FrameFlags(1); sendResponseForTest(response_1); sendResponseForTest(response_1_frame_1); diff --git a/contrib/generic_proxy/filters/network/test/proxy_test.cc b/contrib/generic_proxy/filters/network/test/proxy_test.cc index 701e2c26faac..75246e829e3f 100644 --- a/contrib/generic_proxy/filters/network/test/proxy_test.cc +++ b/contrib/generic_proxy/filters/network/test/proxy_test.cc @@ -199,6 +199,13 @@ class FilterTest : public FilterConfigTest { .WillOnce(Invoke( [this](ServerCodecCallbacks& callback) { server_codec_callbacks_ = &callback; })); + ON_CALL(*server_codec_, respond(_, _, _)) + .WillByDefault( + Invoke([](Status status, absl::string_view data, const RequestHeaderFrame& req) { + FakeStreamCodecFactory::FakeServerCodec codec; + return codec.respond(status, data, req); + })); + filter_ = std::make_shared(filter_config_, factory_context_); EXPECT_EQ(filter_.get(), server_codec_callbacks_); @@ -640,8 +647,6 @@ TEST_F(FilterTest, ActiveStreamSingleFrameFiltersContinueDecodingAfterSendLocalR EXPECT_CALL(*mock_stream_filter_1, decodeHeaderFrame(_)) .WillOnce(Invoke([&](const RequestHeaderFrame&) { - EXPECT_CALL(*server_codec_, respond(_, _, _)) - .WillOnce(Return(ByMove(std::make_unique()))); EXPECT_CALL(*server_codec_, encode(_, _)); active_stream->sendLocalReply(Status(StatusCode::kUnknown, "test_detail"), {}, nullptr); // Make no sense because the filter chain will be always stopped if @@ -784,10 +789,7 @@ TEST_F(FilterTest, ActiveStreamSingleFrameFiltersContinueEncodingAfterSendLocalR EXPECT_CALL(*mock_stream_filter_1, encodeHeaderFrame(_)) .WillOnce(Invoke([&](const ResponseHeaderFrame&) { - EXPECT_CALL(*server_codec_, respond(_, _, _)) - .WillOnce(Return(ByMove(std::make_unique()))); EXPECT_CALL(*server_codec_, encode(_, _)); - active_stream->sendLocalReply(Status(StatusCode::kUnknown, "test_detail"), {}, nullptr); // Make no sense because the filter chain will be always stopped if // the stream is reset or completed. @@ -824,11 +826,11 @@ TEST_F(FilterTest, ActiveStreamMultipleFrameFiltersContinueDecoding) { .WillOnce(Return(HeaderFilterStatus::StopIteration)); auto request = std::make_unique(); - request->stream_frame_flags_ = {StreamFlags(0), false}; + request->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); auto request_frame_0 = std::make_unique(); - request_frame_0->stream_frame_flags_ = {StreamFlags(0), false}; + request_frame_0->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); auto request_frame_1 = std::make_unique(); - request_frame_1->stream_frame_flags_ = {StreamFlags(0), false}; + request_frame_1->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); auto request_frame_2 = std::make_unique(); filter_->onDecodingSuccess(std::move(request)); @@ -901,11 +903,11 @@ TEST_F(FilterTest, ActiveStreamMultipleFrameFiltersContinueEncoding) { auto active_stream = filter_->activeStreamsForTest().begin()->get(); auto response = std::make_unique(); - response->stream_frame_flags_ = {StreamFlags(0), false}; + response->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); auto response_frame_0 = std::make_unique(); - response_frame_0->stream_frame_flags_ = {StreamFlags(0), false}; + response_frame_0->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); auto response_frame_1 = std::make_unique(); - response_frame_1->stream_frame_flags_ = {StreamFlags(0), false}; + response_frame_1->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); auto response_frame_2 = std::make_unique(); EXPECT_CALL(*mock_stream_filter_1, encodeHeaderFrame(_)) @@ -980,7 +982,220 @@ TEST_F(FilterTest, ActiveStreamMultipleFrameFiltersContinueEncoding) { active_stream->onResponseCommonFrame(std::move(response_frame_2)); } +TEST_F(FilterTest, UpstreamResponseAfterPreviousUpstreamResponse) { + auto mock_stream_filter_0 = std::make_shared>(); + auto mock_stream_filter_1 = std::make_shared>(); + + mock_stream_filters_ = {{"mock_0", mock_stream_filter_0}, {"mock_1", mock_stream_filter_1}}; + + // The logger is used to test the log format. + initializeFilter(false, loggerFormFormat()); + + auto request = std::make_unique(); + + filter_->onDecodingSuccess(std::move(request)); + EXPECT_EQ(1, filter_->activeStreamsForTest().size()); + + EXPECT_EQ(filter_config_->stats().downstream_rq_total_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_active_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_error_.value(), 0); + EXPECT_EQ(filter_config_->stats().downstream_rq_reset_.value(), 0); + + auto active_stream = filter_->activeStreamsForTest().begin()->get(); + + EXPECT_CALL(factory_context_.drain_manager_, drainClose()).WillOnce(Return(false)); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); + + // Response filter chain is stopped by the first filter. + EXPECT_CALL(*mock_stream_filter_1, encodeHeaderFrame(_)) + .WillOnce(Return(HeaderFilterStatus::StopIteration)); + + auto response = std::make_unique(); + active_stream->onResponseHeaderFrame(std::move(response)); + + // The repeated response will result in the stream reset. + auto response_again = std::make_unique(); + active_stream->onResponseHeaderFrame(std::move(response_again)); + + EXPECT_EQ(filter_config_->stats().downstream_rq_total_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_active_.value(), 0); + EXPECT_EQ(filter_config_->stats().downstream_rq_error_.value(), 0); + EXPECT_EQ(filter_config_->stats().downstream_rq_reset_.value(), 1); +} + +TEST_F(FilterTest, UpstreamResponseAfterPreviousLocalReply) { + auto mock_stream_filter_0 = std::make_shared>(); + auto mock_stream_filter_1 = std::make_shared>(); + + mock_stream_filters_ = {{"mock_0", mock_stream_filter_0}, {"mock_1", mock_stream_filter_1}}; + + // The logger is used to test the log format. + initializeFilter(false, loggerFormFormat()); + + auto request = std::make_unique(); + + filter_->onDecodingSuccess(std::move(request)); + EXPECT_EQ(1, filter_->activeStreamsForTest().size()); + + EXPECT_EQ(filter_config_->stats().downstream_rq_total_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_active_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_error_.value(), 0); + EXPECT_EQ(filter_config_->stats().downstream_rq_reset_.value(), 0); + + auto active_stream = filter_->activeStreamsForTest().begin()->get(); + + EXPECT_CALL(factory_context_.drain_manager_, drainClose()).WillOnce(Return(false)); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); + + // Response filter chain of local reply is stopped by the first filter. + EXPECT_CALL(*mock_stream_filter_1, encodeHeaderFrame(_)) + .WillOnce(Return(HeaderFilterStatus::StopIteration)); + + active_stream->sendLocalReply(Status(StatusCode::kUnknown, "test_detail"), {}, nullptr); + + // The repeated response will result in the stream reset. + auto response_again = std::make_unique(); + active_stream->onResponseHeaderFrame(std::move(response_again)); + + EXPECT_EQ(filter_config_->stats().downstream_rq_total_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_active_.value(), 0); + // Although the original local reply is not sent to the downstream, it will still be counted as an + // error reply. This is corner case. + EXPECT_EQ(filter_config_->stats().downstream_rq_error_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_reset_.value(), 1); +} + +TEST_F(FilterTest, SendLocalReplyAfterPreviousLocalReply) { + auto mock_stream_filter_0 = std::make_shared>(); + auto mock_stream_filter_1 = std::make_shared>(); + + mock_stream_filters_ = {{"mock_0", mock_stream_filter_0}, {"mock_1", mock_stream_filter_1}}; + + // The logger is used to test the log format. + initializeFilter(false, loggerFormFormat()); + + auto request = std::make_unique(); + + filter_->onDecodingSuccess(std::move(request)); + EXPECT_EQ(1, filter_->activeStreamsForTest().size()); + + EXPECT_EQ(filter_config_->stats().downstream_rq_total_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_active_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_error_.value(), 0); + EXPECT_EQ(filter_config_->stats().downstream_rq_reset_.value(), 0); + + auto active_stream = filter_->activeStreamsForTest().begin()->get(); + + EXPECT_CALL(factory_context_.drain_manager_, drainClose()).WillOnce(Return(false)); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); + + // Response filter chain of local reply is stopped by the first filter. + EXPECT_CALL(*mock_stream_filter_1, encodeHeaderFrame(_)) + .WillOnce(Return(HeaderFilterStatus::StopIteration)); + + active_stream->sendLocalReply(Status(StatusCode::kUnknown, "test_detail_1"), {}, nullptr); + + EXPECT_CALL(*server_codec_, encode(_, _)) + .WillOnce(Invoke([&](const StreamFrame& stream, EncodingContext&) { + auto* typed_response = dynamic_cast(&stream); + EXPECT_EQ(typed_response->status_.code(), static_cast(StatusCode::kUnknown)); + EXPECT_EQ(typed_response->message_, "test_detail_2"); + + Buffer::OwnedImpl buffer; + buffer.add("test"); + + server_codec_callbacks_->writeToConnection(buffer); + buffer.drain(buffer.length()); + + return EncodingResult{4}; + })); + EXPECT_CALL(filter_callbacks_.connection_, write(BufferStringEqual("test"), false)); + + // The latest local reply will skip the filter chain processing and be sent to the downstream + // directly. + active_stream->sendLocalReply(Status(StatusCode::kUnknown, "test_detail_2"), {}, nullptr); + + EXPECT_EQ(filter_config_->stats().downstream_rq_total_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_active_.value(), 0); + EXPECT_EQ(filter_config_->stats().downstream_rq_error_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_reset_.value(), 0); +} + +TEST_F(FilterTest, SendLocalReplyAfterPreviousUpstreamResponseHeaderIsSent) { + auto mock_stream_filter_0 = std::make_shared>(); + auto mock_stream_filter_1 = std::make_shared>(); + + mock_stream_filters_ = {{"mock_0", mock_stream_filter_0}, {"mock_1", mock_stream_filter_1}}; + + // The logger is used to test the log format. + initializeFilter(false, loggerFormFormat()); + + auto request = std::make_unique(); + + filter_->onDecodingSuccess(std::move(request)); + EXPECT_EQ(1, filter_->activeStreamsForTest().size()); + + EXPECT_EQ(filter_config_->stats().downstream_rq_total_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_active_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_error_.value(), 0); + EXPECT_EQ(filter_config_->stats().downstream_rq_reset_.value(), 0); + + auto active_stream = filter_->activeStreamsForTest().begin()->get(); + + EXPECT_CALL(factory_context_.drain_manager_, drainClose()).WillOnce(Return(false)); + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); + + testing::Sequence s; + + // Filter chain is completed and the response header is sent to the downstream. + EXPECT_CALL(*mock_stream_filter_1, encodeHeaderFrame(_)) + .WillOnce(Return(HeaderFilterStatus::Continue)); + EXPECT_CALL(*mock_stream_filter_0, encodeHeaderFrame(_)) + .WillOnce(Return(HeaderFilterStatus::Continue)); + + EXPECT_CALL(*server_codec_, encode(_, _)) + .WillOnce(Invoke([&](const StreamFrame& stream, EncodingContext&) { + auto* typed_response = dynamic_cast(&stream); + EXPECT_EQ(typed_response->status_.code(), 0); + EXPECT_EQ(typed_response->message_, "anything"); + + Buffer::OwnedImpl buffer; + buffer.add("test"); + + server_codec_callbacks_->writeToConnection(buffer); + buffer.drain(buffer.length()); + + return EncodingResult{4}; + })); + EXPECT_CALL(filter_callbacks_.connection_, write(BufferStringEqual("test"), false)); + + auto response = std::make_unique(); + response->message_ = "anything"; + // The response header is not end frame to ensure the stream won't be cleared after the response + // header is sent. + response->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); + + active_stream->onResponseHeaderFrame(std::move(response)); + + // The latest local reply will result in the stream reset because the previous response header is + // sent. + active_stream->sendLocalReply(Status(StatusCode::kUnknown, "test_detail_2"), {}, nullptr); + + EXPECT_EQ(filter_config_->stats().downstream_rq_total_.value(), 1); + EXPECT_EQ(filter_config_->stats().downstream_rq_active_.value(), 0); + EXPECT_EQ(filter_config_->stats().downstream_rq_error_.value(), 0); + EXPECT_EQ(filter_config_->stats().downstream_rq_reset_.value(), 1); +} + TEST_F(FilterTest, ActiveStreamSendLocalReply) { + auto mock_stream_filter_0 = std::make_shared>(); + auto mock_stream_filter_1 = std::make_shared>(); + auto mock_stream_filter_2 = std::make_shared>(); + + mock_stream_filters_ = {{"mock_0", mock_stream_filter_0}, + {"mock_1", mock_stream_filter_1}, + {"mock_2", mock_stream_filter_2}}; + initializeFilter(false, loggerFormFormat()); auto request = std::make_unique(); @@ -999,14 +1214,6 @@ TEST_F(FilterTest, ActiveStreamSendLocalReply) { auto active_stream = filter_->activeStreamsForTest().begin()->get(); - EXPECT_CALL(*server_codec_, respond(_, _, _)) - .WillOnce(Invoke([&](Status status, absl::string_view, const Request&) -> ResponsePtr { - auto response = std::make_unique(); - response->status_ = {static_cast(status.code()), status.code() == StatusCode::kOk}; - response->message_ = status.message(); - return response; - })); - EXPECT_CALL(*factory_context_.server_factory_context_.access_log_manager_.file_, write("host-value /path-value method-value protocol-value request-value " "response-value - 2 test_detail")); @@ -1029,6 +1236,14 @@ TEST_F(FilterTest, ActiveStreamSendLocalReply) { return EncodingResult{4}; })); + testing::InSequence s; + EXPECT_CALL(*mock_stream_filter_2, encodeHeaderFrame(_)) + .WillOnce(Return(HeaderFilterStatus::Continue)); + EXPECT_CALL(*mock_stream_filter_1, encodeHeaderFrame(_)) + .WillOnce(Return(HeaderFilterStatus::Continue)); + EXPECT_CALL(*mock_stream_filter_0, encodeHeaderFrame(_)) + .WillOnce(Return(HeaderFilterStatus::Continue)); + active_stream->sendLocalReply( Status(StatusCode::kUnknown, "test_detail"), {}, [](StreamResponse& response) { response.set("response-key", "response-value"); }); @@ -1160,7 +1375,7 @@ TEST_F(FilterTest, NewStreamAndReplyNormallyWithMultipleFrames) { request->method_ = "method-value"; request->protocol_ = "protocol-value"; request->data_["request-key"] = "request-value"; - request->stream_frame_flags_ = FrameFlags(StreamFlags(), false); + request->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); // The first frame is not the end stream and we will create a frame handler for it. filter_->onDecodingSuccess(std::move(request)); @@ -1176,14 +1391,14 @@ TEST_F(FilterTest, NewStreamAndReplyNormallyWithMultipleFrames) { EXPECT_CALL(mock_stream_frame_handler, onRequestCommonFrame(_)).Times(2); auto request_frame_1 = std::make_unique(); - request_frame_1->stream_frame_flags_ = FrameFlags(StreamFlags(), false); + request_frame_1->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); filter_->onDecodingSuccess(std::move(request_frame_1)); EXPECT_EQ(1, filter_->activeStreamsForTest().size()); EXPECT_EQ(1, filter_->frameHandlersForTest().size()); // When the last frame is the end stream, we will delete the frame handler. auto request_frame_2 = std::make_unique(); - request_frame_2->stream_frame_flags_ = FrameFlags(StreamFlags(), true); + request_frame_2->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_END_STREAM); filter_->onDecodingSuccess(std::move(request_frame_2)); EXPECT_EQ(1, filter_->activeStreamsForTest().size()); EXPECT_EQ(0, filter_->frameHandlersForTest().size()); @@ -1212,13 +1427,13 @@ TEST_F(FilterTest, NewStreamAndReplyNormallyWithMultipleFrames) { auto response = std::make_unique(); response->data_["response-key"] = "response-value"; - response->stream_frame_flags_ = FrameFlags(StreamFlags(), false); + response->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); response->status_ = {123, false}; // Response non-OK. active_stream->onResponseHeaderFrame(std::move(response)); auto response_frame_1 = std::make_unique(); - response_frame_1->stream_frame_flags_ = FrameFlags(StreamFlags(), true); + response_frame_1->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_END_STREAM); active_stream->onResponseCommonFrame(std::move(response_frame_1)); @@ -1313,7 +1528,8 @@ TEST_F(FilterTest, NewStreamAndReplyNormallyWithStreamDrainClose) { EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); auto response = std::make_unique(); - response->stream_frame_flags_ = FrameFlags(StreamFlags(0, false, true, false), true); + response->stream_frame_flags_ = + FrameFlags(0, FrameFlags::FLAG_END_STREAM | FrameFlags::FLAG_DRAIN_CLOSE); active_stream->onResponseHeaderFrame(std::move(response)); } diff --git a/contrib/generic_proxy/filters/network/test/router/router_test.cc b/contrib/generic_proxy/filters/network/test/router/router_test.cc index ad2ff8cf5124..c1cbdebefefe 100644 --- a/contrib/generic_proxy/filters/network/test/router/router_test.cc +++ b/contrib/generic_proxy/filters/network/test/router/router_test.cc @@ -516,7 +516,7 @@ TEST_F(RouterFilterTest, UpstreamRequestPoolFailureConnctionTimeoutAndWithRetryW } TEST_F(RouterFilterTest, UpstreamRequestPoolReadyAndExpectNoResponse) { - setup(FrameFlags(StreamFlags(0, true, false, false), true)); + setup(FrameFlags(0, FrameFlags::FLAG_END_STREAM | FrameFlags::FLAG_ONE_WAY)); kickOffNewUpstreamRequest(true); EXPECT_CALL(mock_filter_callback_, completeDirectly()).WillOnce(Invoke([this]() -> void { @@ -700,11 +700,11 @@ TEST_F(RouterFilterTest, UpstreamRequestPoolReadyAndResponseAndTimeout) { TEST_F(RouterFilterTest, UpstreamRequestPoolReadyAndResponseWithMultipleFrames) { // There are multiple frames in the request. - setup(FrameFlags(StreamFlags(0, false, false, true), /*end_stream*/ false)); + setup(FrameFlags(0, FrameFlags::FLAG_EMPTY)); kickOffNewUpstreamRequest(true); auto frame_1 = std::make_unique(); - frame_1->stream_frame_flags_ = FrameFlags(StreamFlags(0, false, false, true), false); + frame_1->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); EXPECT_EQ(CommonFilterStatus::StopIteration, filter_->decodeCommonFrame(*frame_1)); // This only store the frame and does nothing else because the pool is not ready yet. @@ -744,12 +744,12 @@ TEST_F(RouterFilterTest, UpstreamRequestPoolReadyAndResponseWithMultipleFrames) })); auto response = std::make_unique(); - response->stream_frame_flags_ = FrameFlags(StreamFlags(0, false, false, false), false); + response->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); notifyDecodingSuccess(std::move(response)); auto response_frame_1 = std::make_unique(); - response_frame_1->stream_frame_flags_ = FrameFlags(StreamFlags(0, false, false, false), false); + response_frame_1->stream_frame_flags_ = FrameFlags(0, FrameFlags::FLAG_EMPTY); notifyDecodingSuccess(std::move(response_frame_1)); // End stream is set to true by default. @@ -777,7 +777,8 @@ TEST_F(RouterFilterTest, UpstreamRequestPoolReadyAndResponseWithDrainCloseSetInR })); auto response = std::make_unique(); - response->stream_frame_flags_ = FrameFlags(StreamFlags(0, false, true, false), true); + response->stream_frame_flags_ = + FrameFlags(0, FrameFlags::FLAG_END_STREAM | FrameFlags::FLAG_DRAIN_CLOSE); notifyDecodingSuccess(std::move(response)); // Mock downstream closing. diff --git a/contrib/generic_proxy/filters/network/test/router/upstream_test.cc b/contrib/generic_proxy/filters/network/test/router/upstream_test.cc index cabb2d639b41..856b3afb3fde 100644 --- a/contrib/generic_proxy/filters/network/test/router/upstream_test.cc +++ b/contrib/generic_proxy/filters/network/test/router/upstream_test.cc @@ -377,10 +377,10 @@ TEST_F(UpstreamTest, BoundGenericUpstreamDecodingSuccess) { EXPECT_EQ(2, generic_upstream->waitingResponseRequestsSize()); auto request_1 = std::make_unique(); - request_1->stream_frame_flags_ = FrameFlags(StreamFlags(1)); + request_1->stream_frame_flags_ = FrameFlags(1); auto request_2 = std::make_unique(); - request_2->stream_frame_flags_ = FrameFlags(StreamFlags(2)); + request_2->stream_frame_flags_ = FrameFlags(2); EXPECT_CALL(*mock_client_codec_raw_, decode(_, _)) .WillOnce(testing::Invoke([&](Buffer::Instance&, bool) { @@ -431,10 +431,10 @@ TEST_F(UpstreamTest, BoundGenericUpstreamDecodingSuccessWithMultipleFrames) { EXPECT_EQ(2, generic_upstream->waitingResponseRequestsSize()); auto request_1 = std::make_unique(); - request_1->stream_frame_flags_ = FrameFlags(StreamFlags(1), false); + request_1->stream_frame_flags_ = FrameFlags(1, FrameFlags::FLAG_EMPTY); auto request_2 = std::make_unique(); - request_2->stream_frame_flags_ = FrameFlags(StreamFlags(2), false); + request_2->stream_frame_flags_ = FrameFlags(2, FrameFlags::FLAG_EMPTY); EXPECT_CALL(*mock_client_codec_raw_, decode(_, _)) .WillOnce(testing::Invoke([&](Buffer::Instance&, bool) { @@ -453,10 +453,10 @@ TEST_F(UpstreamTest, BoundGenericUpstreamDecodingSuccessWithMultipleFrames) { EXPECT_EQ(2, generic_upstream->waitingResponseRequestsSize()); auto request_1_frame = std::make_unique(); - request_1_frame->stream_frame_flags_ = FrameFlags(StreamFlags(1), true); + request_1_frame->stream_frame_flags_ = FrameFlags(1); auto request_2_frame = std::make_unique(); - request_2_frame->stream_frame_flags_ = FrameFlags(StreamFlags(2), true); + request_2_frame->stream_frame_flags_ = FrameFlags(2); EXPECT_CALL(*mock_client_codec_raw_, decode(_, _)) .WillOnce(testing::Invoke([&](Buffer::Instance&, bool) { @@ -504,10 +504,10 @@ TEST_F(UpstreamTest, BoundGenericUpstreamDecodingFailureAndNoUpstreamConnectionC EXPECT_EQ(2, generic_upstream->waitingResponseRequestsSize()); auto request_1 = std::make_unique(); - request_1->stream_frame_flags_ = FrameFlags(StreamFlags(1)); + request_1->stream_frame_flags_ = FrameFlags(1); auto request_2 = std::make_unique(); - request_2->stream_frame_flags_ = FrameFlags(StreamFlags(2)); + request_2->stream_frame_flags_ = FrameFlags(2); EXPECT_CALL(*mock_client_codec_raw_, decode(_, _)) .WillOnce(testing::Invoke([&](Buffer::Instance&, bool) { @@ -557,10 +557,10 @@ TEST_F(UpstreamTest, BoundGenericUpstreamDecodingFailure) { EXPECT_EQ(2, generic_upstream->waitingResponseRequestsSize()); auto request_1 = std::make_unique(); - request_1->stream_frame_flags_ = FrameFlags(StreamFlags(1)); + request_1->stream_frame_flags_ = FrameFlags(1); auto request_2 = std::make_unique(); - request_2->stream_frame_flags_ = FrameFlags(StreamFlags(2)); + request_2->stream_frame_flags_ = FrameFlags(2); EXPECT_CALL(*mock_client_codec_raw_, decode(_, _)) .WillOnce(testing::Invoke([&](Buffer::Instance&, bool) { @@ -733,7 +733,7 @@ TEST_F(UpstreamTest, OwnedGenericUpstreamDecodingSuccess) { EXPECT_EQ(1, generic_upstream->waitingResponseRequestsSize()); auto request_1 = std::make_unique(); - request_1->stream_frame_flags_ = FrameFlags(StreamFlags(1)); + request_1->stream_frame_flags_ = FrameFlags(1); EXPECT_CALL(*mock_client_codec_raw_, decode(_, _)) .WillOnce(testing::Invoke([&](Buffer::Instance&, bool) { @@ -773,7 +773,7 @@ TEST_F(UpstreamTest, OwnedGenericUpstreamDecodingSuccessWithMultipleFrames) { EXPECT_EQ(1, generic_upstream->waitingResponseRequestsSize()); auto request_1 = std::make_unique(); - request_1->stream_frame_flags_ = FrameFlags(StreamFlags(1), false); + request_1->stream_frame_flags_ = FrameFlags(1, FrameFlags::FLAG_EMPTY); EXPECT_CALL(*mock_client_codec_raw_, decode(_, _)) .WillOnce(testing::Invoke([&](Buffer::Instance&, bool) { @@ -789,7 +789,7 @@ TEST_F(UpstreamTest, OwnedGenericUpstreamDecodingSuccessWithMultipleFrames) { EXPECT_EQ(1, generic_upstream->waitingResponseRequestsSize()); auto request_1_frame = std::make_unique(); - request_1_frame->stream_frame_flags_ = FrameFlags(StreamFlags(1), true); + request_1_frame->stream_frame_flags_ = FrameFlags(1); EXPECT_CALL(*mock_client_codec_raw_, decode(_, _)) .WillOnce(testing::Invoke([&](Buffer::Instance&, bool) { From a0b2ec237c0c095b890bfc0f06686e20fae3adf8 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Thu, 13 Jun 2024 13:37:05 -0400 Subject: [PATCH 15/21] ci: changing bes values per bazel team advice (#34716) * ci: changing bes values per bazel team advice Signed-off-by: Alyssa Wilk * fix Signed-off-by: Alyssa Wilk * probably the correct section Signed-off-by: Alyssa Wilk * backing out retries Signed-off-by: Alyssa Wilk --------- Signed-off-by: Alyssa Wilk --- .bazelrc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.bazelrc b/.bazelrc index 14db9a58c5a1..8426cc384eae 100644 --- a/.bazelrc +++ b/.bazelrc @@ -54,8 +54,6 @@ build:docs-ci --action_env=DOCS_RST_CHECK=1 --host_action_env=DOCS_RST_CHECK=1 build --incompatible_config_setting_private_default_visibility build --incompatible_enforce_config_setting_visibility -test --bes_upload_mode=nowait_for_upload_complete -test --bes_timeout=30s test --test_verbose_timeout_warnings # Allow tags to influence execution requirements @@ -511,6 +509,7 @@ build:rbe-google --config=cache-google build:rbe-google-bes --bes_backend=grpcs://buildeventservice.googleapis.com build:rbe-google-bes --bes_results_url=https://source.cloud.google.com/results/invocations/ +build:rbe-google-bes --bes_upload_mode=fully_async # RBE (Engflow mobile) build:rbe-engflow --google_default_credentials=false From 89593f97d23d388d5c97c679baf6b0020f4f2c32 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Thu, 13 Jun 2024 14:06:47 -0400 Subject: [PATCH 16/21] ci: hopefully getting better test logs (#34643) Signed-off-by: Alyssa Wilk --- .bazelrc | 1 + 1 file changed, 1 insertion(+) diff --git a/.bazelrc b/.bazelrc index 8426cc384eae..d7294707b7f0 100644 --- a/.bazelrc +++ b/.bazelrc @@ -55,6 +55,7 @@ build --incompatible_config_setting_private_default_visibility build --incompatible_enforce_config_setting_visibility test --test_verbose_timeout_warnings +test --experimental_ui_max_stdouterr_bytes=3048576 #default 1048576 # Allow tags to influence execution requirements common --experimental_allow_tags_propagation From f094d24759f739df9fe84cc10c2a315165e790a2 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Thu, 13 Jun 2024 16:55:45 -0400 Subject: [PATCH 17/21] runtime: removing hmac_base64_encoding_only (#34710) Signed-off-by: Alyssa Wilk --- changelogs/current.yaml | 3 + source/common/runtime/runtime_features.cc | 1 - .../extensions/filters/http/oauth2/filter.cc | 6 +- .../filters/http/oauth2/filter_test.cc | 771 +++++------------- 4 files changed, 224 insertions(+), 557 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index cbcead927b85..6efda2a78003 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -210,6 +210,9 @@ removed_config_or_runtime: - area: http change: | Removed ``envoy.reloadable_features.lowercase_scheme`` runtime flag and legacy code paths. +- area: oauth + change: | + Removed ``envoy.reloadable_features.hmac_base64_encoding_only`` runtime flag and legacy code paths. - area: upstream change: | Removed ``envoy.reloadable_features.convert_legacy_lb_config`` runtime flag and legacy code paths. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index bb6806e58b46..6b63e6b08081 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -49,7 +49,6 @@ RUNTIME_GUARD(envoy_reloadable_features_enable_include_histograms); RUNTIME_GUARD(envoy_reloadable_features_exclude_host_in_eds_status_draining); RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_change_http_status); RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_handle_empty_response); -RUNTIME_GUARD(envoy_reloadable_features_hmac_base64_encoding_only); RUNTIME_GUARD(envoy_reloadable_features_http1_balsa_delay_reset); RUNTIME_GUARD(envoy_reloadable_features_http1_connection_close_header_in_redirect); // Ignore the automated "remove this flag" issue: we should keep this for 1 year. diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc index 0824ae3bdd57..dda635f73349 100644 --- a/source/extensions/filters/http/oauth2/filter.cc +++ b/source/extensions/filters/http/oauth2/filter.cc @@ -173,11 +173,7 @@ std::string encodeHmacBase64(const std::vector& secret, absl::string_vi std::string encodeHmac(const std::vector& secret, absl::string_view host, absl::string_view expires, absl::string_view token = "", absl::string_view id_token = "", absl::string_view refresh_token = "") { - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.hmac_base64_encoding_only")) { - return encodeHmacBase64(secret, host, expires, token, id_token, refresh_token); - } else { - return encodeHmacHexBase64(secret, host, expires, token, id_token, refresh_token); - } + return encodeHmacBase64(secret, host, expires, token, id_token, refresh_token); } } // namespace diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 8f9850f4cfdf..5c889533ee3c 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -679,39 +679,101 @@ TEST_F(OAuth2Test, OAuthErrorNonOAuthHttpCallbackLegacyEncoding) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({ {"envoy.reloadable_features.oauth_use_url_encoding", "false"}, + {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, }); init(); - Http::TestRequestHeaderMapImpl request_headers{ - {Http::Headers::get().Path.get(), "/not/_oauth"}, + // First construct the initial request to the oauth filter with URI parameters. + Http::TestRequestHeaderMapImpl first_request_headers{ + {Http::Headers::get().Path.get(), "/test?name=admin&level=trace"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Scheme.get(), "http"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Post}, + {Http::Headers::get().Scheme.get(), "https"}, }; - Http::TestResponseHeaderMapImpl response_headers{ + // This is the immediate response - a redirect to the auth cluster. + Http::TestResponseHeaderMapImpl first_response_headers{ {Http::Headers::get().Status.get(), "302"}, {Http::Headers::get().Location.get(), "https://auth.example.com/oauth/" "authorize/?client_id=" + TEST_CLIENT_ID + - "&redirect_uri=http%3A%2F%2Ftraffic.example.com%2F_oauth" + "&redirect_uri=https%3A%2F%2Ftraffic.example.com%2F_oauth" "&response_type=code" "&scope=" + TEST_ENCODED_AUTH_SCOPES + - "&state=http%3A%2F%2Ftraffic.example.com%2Fnot%2F_oauth" + "&state=https%3A%2F%2Ftraffic.example.com%2Ftest%3Fname%3Dadmin%26level%3Dtrace" "&resource=oauth2-resource&resource=http%3A%2F%2Fexample.com" "&resource=https%3A%2F%2Fexample.com%2Fsome%2Fpath%2F..%2F%2Futf8%C3%83;foo%3Dbar%" "3Fvar1%3D1%26var2%3D2"}, }; - // explicitly tell the validator to fail the validation + // Fail the validation to trigger the OAuth flow. EXPECT_CALL(*validator_, setParams(_, _)); EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); - EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); + // Check that the redirect includes the escaped parameter characters, '?', '&' and '='. + EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&first_response_headers), true)); + // This represents the beginning of the OAuth filter. EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, - filter_->decodeHeaders(request_headers, false)); + filter_->decodeHeaders(first_request_headers, false)); + + // This represents the callback request from the authorization server. + Http::TestRequestHeaderMapImpl second_request_headers{ + {Http::Headers::get().Path.get(), "/_oauth?code=123&state=https%3A%2F%2Ftraffic.example.com%" + "2Ftest%3Fname%3Dadmin%26level%3Dtrace"}, + {Http::Headers::get().Host.get(), "traffic.example.com"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Scheme.get(), "https"}, + }; + + // Deliberately fail the HMAC validation check. + EXPECT_CALL(*validator_, setParams(_, _)); + EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); + + EXPECT_CALL(*oauth_client_, asyncGetAccessToken("123", TEST_CLIENT_ID, "asdf_client_secret_fdsa", + "https://traffic.example.com" + TEST_CALLBACK, + AuthType::UrlEncodedBody)); + + // Invoke the callback logic. As a side effect, state_ will be populated. + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + filter_->decodeHeaders(second_request_headers, false)); + + EXPECT_EQ(1, config_->stats().oauth_unauthorized_rq_.value()); + EXPECT_EQ(config_->clusterName(), "auth.example.com"); + + // Expected response after the callback & validation is complete - verifying we kept the + // state and method of the original request, including the query string parameters. + Http::TestRequestHeaderMapImpl second_response_headers{ + {Http::Headers::get().Status.get(), "302"}, + {Http::Headers::get().SetCookie.get(), "OauthHMAC=" + "fV62OgLipChTQQC3UFgDp+l5sCiSb3zt7nCoJiVivWw=;" + "path=/;Max-Age=;secure;HttpOnly"}, + {Http::Headers::get().SetCookie.get(), "OauthExpires=;path=/;Max-Age=;secure;HttpOnly"}, + {Http::Headers::get().SetCookie.get(), "BearerToken=;path=/;Max-Age=;secure;HttpOnly"}, + {Http::Headers::get().Location.get(), + "https://traffic.example.com/test?name=admin&level=trace"}, + }; + + EXPECT_CALL(decoder_callbacks_, + encodeHeaders_(HeaderMapEqualRef(&second_response_headers), true)); + + filter_->finishGetAccessTokenFlow(); + + // Deliberately fail the HMAC validation check. + EXPECT_CALL(*validator_, setParams(_, _)); + EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); + + EXPECT_CALL(*oauth_client_, asyncGetAccessToken("123", TEST_CLIENT_ID, "asdf_client_secret_fdsa", + "https://traffic.example.com" + TEST_CALLBACK, + AuthType::UrlEncodedBody)); + + // Invoke the callback logic. As a side effect, state_ will be populated. + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + filter_->decodeHeaders(second_request_headers, false)); + + EXPECT_EQ(1, config_->stats().oauth_unauthorized_rq_.value()); + EXPECT_EQ(config_->clusterName(), "auth.example.com"); } TEST_F(OAuth2Test, OAuthErrorNonOAuthHttpCallback) { @@ -878,192 +940,86 @@ TEST_F(OAuth2Test, CookieValidatorWithCustomNames) { // Validates the behavior of the cookie validator when the combination of some fields could be same. TEST_F(OAuth2Test, CookieValidatorSame) { - { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); - auto cookie_names = - CookieNames{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", "RefreshToken"}; - const auto expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 5; - - // Host name is `traffic.example.com:101` and the expire time is 5. - Http::TestRequestHeaderMapImpl request_headers{ - {Http::Headers::get().Host.get(), "traffic.example.com:101"}, - {Http::Headers::get().Path.get(), "/anypath"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Cookie.get(), - fmt::format("{}={}", cookie_names.oauth_expires_, expires_at_s)}, - {Http::Headers::get().Cookie.get(), absl::StrCat(cookie_names.bearer_token_, "=xyztoken")}, - {Http::Headers::get().Cookie.get(), - absl::StrCat(cookie_names.oauth_hmac_, "=MSq8mkNQGdXx2LKGlLHMwSIj8rLZRnrHE6EWvvTUFx0=")}, - }; - - auto cookie_validator = std::make_shared(test_time_, cookie_names); - EXPECT_EQ(cookie_validator->token(), ""); - cookie_validator->setParams(request_headers, "mock-secret"); - - EXPECT_TRUE(cookie_validator->hmacIsValid()); - EXPECT_TRUE(cookie_validator->timestampIsValid()); - EXPECT_TRUE(cookie_validator->isValid()); - - // If we advance time beyond 5s the timestamp should no longer be valid. - test_time_.advanceTimeWait(std::chrono::seconds(6)); - - EXPECT_FALSE(cookie_validator->timestampIsValid()); - EXPECT_FALSE(cookie_validator->isValid()); - - test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); - const auto new_expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 15; - - // Host name is `traffic.example.com:10` and the expire time is 15. - // HMAC should be different from the above one with the separator fix. - Http::TestRequestHeaderMapImpl request_headers_second{ - {Http::Headers::get().Host.get(), "traffic.example.com:10"}, - {Http::Headers::get().Path.get(), "/anypath"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Cookie.get(), - fmt::format("{}={}", cookie_names.oauth_expires_, new_expires_at_s)}, - {Http::Headers::get().Cookie.get(), absl::StrCat(cookie_names.bearer_token_, "=xyztoken")}, - {Http::Headers::get().Cookie.get(), - absl::StrCat(cookie_names.oauth_hmac_, "=dbl04CSr6eWF52wdNDCRt/Uw6A4y41wbpmtUWRyD2Fo=")}, - }; - - cookie_validator->setParams(request_headers_second, "mock-secret"); - - EXPECT_TRUE(cookie_validator->hmacIsValid()); - EXPECT_TRUE(cookie_validator->timestampIsValid()); - EXPECT_TRUE(cookie_validator->isValid()); - - // If we advance time beyond 15s the timestamp should no longer be valid. - test_time_.advanceTimeWait(std::chrono::seconds(16)); - - EXPECT_FALSE(cookie_validator->timestampIsValid()); - EXPECT_FALSE(cookie_validator->isValid()); - } - { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); - auto cookie_names = - CookieNames{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", "RefreshToken"}; - const auto expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 5; - - // Host name is `traffic.example.com:101` and the expire time is 5. - Http::TestRequestHeaderMapImpl request_headers{ - {Http::Headers::get().Host.get(), "traffic.example.com:101"}, - {Http::Headers::get().Path.get(), "/anypath"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Cookie.get(), - fmt::format("{}={}", cookie_names.oauth_expires_, expires_at_s)}, - {Http::Headers::get().Cookie.get(), absl::StrCat(cookie_names.bearer_token_, "=xyztoken")}, - {Http::Headers::get().Cookie.get(), + test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); + auto cookie_names = + CookieNames{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", "RefreshToken"}; + const auto expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 5; - absl::StrCat(cookie_names.oauth_hmac_, "=" - "MzEyYWJjOWE0MzUwMTlkNWYxZDhiMjg2OTRiMWNjYzEyMjIzZj" - "JiMmQ5NDY3YWM3MTNhMTE2YmVmNGQ0MTcxZA==")}, - }; + // Host name is `traffic.example.com:101` and the expire time is 5. + Http::TestRequestHeaderMapImpl request_headers{ + {Http::Headers::get().Host.get(), "traffic.example.com:101"}, + {Http::Headers::get().Path.get(), "/anypath"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Cookie.get(), + fmt::format("{}={}", cookie_names.oauth_expires_, expires_at_s)}, + {Http::Headers::get().Cookie.get(), absl::StrCat(cookie_names.bearer_token_, "=xyztoken")}, + {Http::Headers::get().Cookie.get(), + absl::StrCat(cookie_names.oauth_hmac_, "=MSq8mkNQGdXx2LKGlLHMwSIj8rLZRnrHE6EWvvTUFx0=")}, + }; - auto cookie_validator = std::make_shared(test_time_, cookie_names); - EXPECT_EQ(cookie_validator->token(), ""); - cookie_validator->setParams(request_headers, "mock-secret"); + auto cookie_validator = std::make_shared(test_time_, cookie_names); + EXPECT_EQ(cookie_validator->token(), ""); + cookie_validator->setParams(request_headers, "mock-secret"); - EXPECT_TRUE(cookie_validator->hmacIsValid()); - EXPECT_TRUE(cookie_validator->timestampIsValid()); - EXPECT_TRUE(cookie_validator->isValid()); + EXPECT_TRUE(cookie_validator->hmacIsValid()); + EXPECT_TRUE(cookie_validator->timestampIsValid()); + EXPECT_TRUE(cookie_validator->isValid()); - // If we advance time beyond 5s the timestamp should no longer be valid. - test_time_.advanceTimeWait(std::chrono::seconds(6)); + // If we advance time beyond 5s the timestamp should no longer be valid. + test_time_.advanceTimeWait(std::chrono::seconds(6)); - EXPECT_FALSE(cookie_validator->timestampIsValid()); - EXPECT_FALSE(cookie_validator->isValid()); + EXPECT_FALSE(cookie_validator->timestampIsValid()); + EXPECT_FALSE(cookie_validator->isValid()); - test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); - const auto new_expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 15; + test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); + const auto new_expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 15; - // Host name is `traffic.example.com:10` and the expire time is 15. - // HMAC should be different from the above one with the separator fix. - Http::TestRequestHeaderMapImpl request_headers_second{ - {Http::Headers::get().Host.get(), "traffic.example.com:10"}, - {Http::Headers::get().Path.get(), "/anypath"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Cookie.get(), - fmt::format("{}={}", cookie_names.oauth_expires_, new_expires_at_s)}, - {Http::Headers::get().Cookie.get(), absl::StrCat(cookie_names.bearer_token_, "=xyztoken")}, - {Http::Headers::get().Cookie.get(), - absl::StrCat(cookie_names.oauth_hmac_, "=" - "NzViOTc0ZTAyNGFiZTllNTg1ZTc2YzFkMzQzMDkxYjdmNTMwZT" - "gwZTMyZTM1YzFiYTY2YjU0NTkxYzgzZDg1YQ==")}, - }; + // Host name is `traffic.example.com:10` and the expire time is 15. + // HMAC should be different from the above one with the separator fix. + Http::TestRequestHeaderMapImpl request_headers_second{ + {Http::Headers::get().Host.get(), "traffic.example.com:10"}, + {Http::Headers::get().Path.get(), "/anypath"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Cookie.get(), + fmt::format("{}={}", cookie_names.oauth_expires_, new_expires_at_s)}, + {Http::Headers::get().Cookie.get(), absl::StrCat(cookie_names.bearer_token_, "=xyztoken")}, + {Http::Headers::get().Cookie.get(), + absl::StrCat(cookie_names.oauth_hmac_, "=dbl04CSr6eWF52wdNDCRt/Uw6A4y41wbpmtUWRyD2Fo=")}, + }; - cookie_validator->setParams(request_headers_second, "mock-secret"); + cookie_validator->setParams(request_headers_second, "mock-secret"); - EXPECT_TRUE(cookie_validator->hmacIsValid()); - EXPECT_TRUE(cookie_validator->timestampIsValid()); - EXPECT_TRUE(cookie_validator->isValid()); + EXPECT_TRUE(cookie_validator->hmacIsValid()); + EXPECT_TRUE(cookie_validator->timestampIsValid()); + EXPECT_TRUE(cookie_validator->isValid()); - // If we advance time beyond 15s the timestamp should no longer be valid. - test_time_.advanceTimeWait(std::chrono::seconds(16)); + // If we advance time beyond 15s the timestamp should no longer be valid. + test_time_.advanceTimeWait(std::chrono::seconds(16)); - EXPECT_FALSE(cookie_validator->timestampIsValid()); - EXPECT_FALSE(cookie_validator->isValid()); - } + EXPECT_FALSE(cookie_validator->timestampIsValid()); + EXPECT_FALSE(cookie_validator->isValid()); } // Validates the behavior of the cookie validator when the expires_at value is not a valid integer. TEST_F(OAuth2Test, CookieValidatorInvalidExpiresAt) { - { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - Http::TestRequestHeaderMapImpl request_headers{ - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Path.get(), "/anypath"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Cookie.get(), "OauthExpires=notanumber"}, - {Http::Headers::get().Cookie.get(), "BearerToken=xyztoken"}, - {Http::Headers::get().Cookie.get(), "OauthHMAC=" - "c+1qzyrMmqG8+O4dn7b28OvNNDWcb04yJfNbZCE1zYE="}, - }; - - auto cookie_validator = std::make_shared( - test_time_, - CookieNames{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", "RefreshToken"}); - cookie_validator->setParams(request_headers, "mock-secret"); - - EXPECT_TRUE(cookie_validator->hmacIsValid()); - EXPECT_FALSE(cookie_validator->timestampIsValid()); - EXPECT_FALSE(cookie_validator->isValid()); - } - { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - Http::TestRequestHeaderMapImpl request_headers{ - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Path.get(), "/anypath"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Cookie.get(), "OauthExpires=notanumber"}, - {Http::Headers::get().Cookie.get(), "BearerToken=xyztoken"}, - {Http::Headers::get().Cookie.get(), "OauthHMAC=" - "NzNlZDZhY2YyYWNjOWFhMWJjZjhlZTFkOWZiNmY2ZjBlYmNkMzQzNT" - "ljNmY0ZTMyMjVmMzViNjQyMTM1Y2Q4MQ=="}, - }; + Http::TestRequestHeaderMapImpl request_headers{ + {Http::Headers::get().Host.get(), "traffic.example.com"}, + {Http::Headers::get().Path.get(), "/anypath"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Cookie.get(), "OauthExpires=notanumber"}, + {Http::Headers::get().Cookie.get(), "BearerToken=xyztoken"}, + {Http::Headers::get().Cookie.get(), "OauthHMAC=" + "c+1qzyrMmqG8+O4dn7b28OvNNDWcb04yJfNbZCE1zYE="}, + }; - auto cookie_validator = std::make_shared( - test_time_, - CookieNames{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", "RefreshToken"}); - cookie_validator->setParams(request_headers, "mock-secret"); + auto cookie_validator = std::make_shared( + test_time_, + CookieNames{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", "RefreshToken"}); + cookie_validator->setParams(request_headers, "mock-secret"); - EXPECT_TRUE(cookie_validator->hmacIsValid()); - EXPECT_FALSE(cookie_validator->timestampIsValid()); - EXPECT_FALSE(cookie_validator->isValid()); - } + EXPECT_TRUE(cookie_validator->hmacIsValid()); + EXPECT_FALSE(cookie_validator->timestampIsValid()); + EXPECT_FALSE(cookie_validator->isValid()); } // Validates the behavior of the cookie validator when the expires_at value is not a valid integer. @@ -1348,7 +1304,6 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParametersLegacyEncoding) { {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, {Http::Headers::get().Scheme.get(), "https"}, }; - // Deliberately fail the HMAC validation check. EXPECT_CALL(*validator_, setParams(_, _)); EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); @@ -1383,96 +1338,7 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParametersLegacyEncoding) { filter_->finishGetAccessTokenFlow(); } - { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.oauth_use_url_encoding", "false"}, - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - init(); - // First construct the initial request to the oauth filter with URI parameters. - Http::TestRequestHeaderMapImpl first_request_headers{ - {Http::Headers::get().Path.get(), "/test?name=admin&level=trace"}, - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Post}, - {Http::Headers::get().Scheme.get(), "https"}, - }; - - // This is the immediate response - a redirect to the auth cluster. - Http::TestResponseHeaderMapImpl first_response_headers{ - {Http::Headers::get().Status.get(), "302"}, - {Http::Headers::get().Location.get(), - "https://auth.example.com/oauth/" - "authorize/?client_id=" + - TEST_CLIENT_ID + - "&redirect_uri=https%3A%2F%2Ftraffic.example.com%2F_oauth" - "&response_type=code" - "&scope=" + - TEST_ENCODED_AUTH_SCOPES + - "&state=https%3A%2F%2Ftraffic.example.com%2Ftest%3Fname%3Dadmin%26level%3Dtrace" - "&resource=oauth2-resource&resource=http%3A%2F%2Fexample.com" - "&resource=https%3A%2F%2Fexample.com%2Fsome%2Fpath%2F..%2F%2Futf8%C3%83;foo%3Dbar%" - "3Fvar1%3D1%26var2%3D2"}, - }; - - // Fail the validation to trigger the OAuth flow. - EXPECT_CALL(*validator_, setParams(_, _)); - EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); - - // Check that the redirect includes the escaped parameter characters, '?', '&' and '='. - EXPECT_CALL(decoder_callbacks_, - encodeHeaders_(HeaderMapEqualRef(&first_response_headers), true)); - - // This represents the beginning of the OAuth filter. - EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, - filter_->decodeHeaders(first_request_headers, false)); - - // This represents the callback request from the authorization server. - Http::TestRequestHeaderMapImpl second_request_headers{ - {Http::Headers::get().Path.get(), - "/_oauth?code=123&state=https%3A%2F%2Ftraffic.example.com%" - "2Ftest%3Fname%3Dadmin%26level%3Dtrace"}, - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Scheme.get(), "https"}, - }; - - // Deliberately fail the HMAC validation check. - EXPECT_CALL(*validator_, setParams(_, _)); - EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); - - EXPECT_CALL(*oauth_client_, - asyncGetAccessToken("123", TEST_CLIENT_ID, "asdf_client_secret_fdsa", - "https://traffic.example.com" + TEST_CALLBACK, - AuthType::UrlEncodedBody)); - - // Invoke the callback logic. As a side effect, state_ will be populated. - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, - filter_->decodeHeaders(second_request_headers, false)); - - EXPECT_EQ(2, config_->stats().oauth_unauthorized_rq_.value()); - EXPECT_EQ(config_->clusterName(), "auth.example.com"); - - // Expected response after the callback & validation is complete - verifying we kept the - // state and method of the original request, including the query string parameters. - Http::TestRequestHeaderMapImpl second_response_headers{ - {Http::Headers::get().Status.get(), "302"}, - {Http::Headers::get().SetCookie.get(), - "OauthHMAC=" - "N2Q1ZWI2M2EwMmUyYTQyODUzNDEwMGI3NTA1ODAzYTdlOTc5YjAyODkyNmY3Y2VkZWU3MGE4MjYyNTYyYmQ2Yw==;" - "path=/;Max-Age=;secure;HttpOnly"}, - {Http::Headers::get().SetCookie.get(), "OauthExpires=;path=/;Max-Age=;secure;HttpOnly"}, - {Http::Headers::get().SetCookie.get(), "BearerToken=;path=/;Max-Age=;secure;HttpOnly"}, - {Http::Headers::get().Location.get(), - "https://traffic.example.com/test?name=admin&level=trace"}, - }; - - EXPECT_CALL(decoder_callbacks_, - encodeHeaders_(HeaderMapEqualRef(&second_response_headers), true)); - - filter_->finishGetAccessTokenFlow(); - } -} +} TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParametersFillRefreshAndIdToken) { // First construct the initial request to the oauth filter with URI parameters. @@ -1566,189 +1432,87 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParametersFillRefreshAndIdToken) { // This test adds %-encoded UTF-8 characters to the URL and shows that // the new decoding correctly handles that case. TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { - { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.oauth_use_url_encoding", "true"}, - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - init(); - // First construct the initial request to the oauth filter with URI parameters. - Http::TestRequestHeaderMapImpl first_request_headers{ - {Http::Headers::get().Path.get(), "/test/utf8%C3%83?name=admin&level=trace"}, - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Post}, - {Http::Headers::get().Scheme.get(), "https"}, - }; - - // This is the immediate response - a redirect to the auth cluster. - Http::TestResponseHeaderMapImpl first_response_headers{ - {Http::Headers::get().Status.get(), "302"}, - {Http::Headers::get().Location.get(), - "https://auth.example.com/oauth/" - "authorize/?client_id=" + - TEST_CLIENT_ID + - "&redirect_uri=https%3A%2F%2Ftraffic.example.com%2F_oauth" - "&response_type=code" - "&scope=" + - TEST_ENCODED_AUTH_SCOPES + - "&state=https%3A%2F%2Ftraffic.example.com%2Ftest%2Futf8%25C3%2583%3Fname%3Dadmin%" - "26level%3Dtrace" - "&resource=oauth2-resource&resource=http%3A%2F%2Fexample.com" - "&resource=https%3A%2F%2Fexample.com%2Fsome%2Fpath%252F..%252F%2Futf8%C3%83%3Bfoo%" - "3Dbar%" - "3Fvar1%3D1%26var2%3D2"}, - }; - - // Fail the validation to trigger the OAuth flow. - EXPECT_CALL(*validator_, setParams(_, _)); - EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); - - // Check that the redirect includes the escaped parameter characters using URL encoding. - EXPECT_CALL(decoder_callbacks_, - encodeHeaders_(HeaderMapEqualRef(&first_response_headers), true)); - - // This represents the beginning of the OAuth filter. - EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, - filter_->decodeHeaders(first_request_headers, false)); - - // This represents the callback request from the authorization server. - Http::TestRequestHeaderMapImpl second_request_headers{ - {Http::Headers::get().Path.get(), - "/_oauth?code=123&state=https%3A%2F%2Ftraffic.example.com%" - "2Ftest%2Futf8%25C3%2583%3Fname%3Dadmin%26level%3Dtrace"}, - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Scheme.get(), "https"}, - }; - - // Deliberately fail the HMAC validation check. - EXPECT_CALL(*validator_, setParams(_, _)); - EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); - - EXPECT_CALL(*oauth_client_, - asyncGetAccessToken("123", TEST_CLIENT_ID, "asdf_client_secret_fdsa", - "https://traffic.example.com" + TEST_CALLBACK, - AuthType::UrlEncodedBody)); - - // Invoke the callback logic. As a side effect, state_ will be populated. - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, - filter_->decodeHeaders(second_request_headers, false)); - - EXPECT_EQ(1, config_->stats().oauth_unauthorized_rq_.value()); - EXPECT_EQ(config_->clusterName(), "auth.example.com"); - - // Expected response after the callback & validation is complete - verifying we kept the - // state and method of the original request, including the query string parameters and UTF8 - // sequences. - Http::TestRequestHeaderMapImpl second_response_headers{ - {Http::Headers::get().Status.get(), "302"}, - {Http::Headers::get().SetCookie.get(), "OauthHMAC=" - "fV62OgLipChTQQC3UFgDp+l5sCiSb3zt7nCoJiVivWw=;" - "path=/;Max-Age=;secure;HttpOnly"}, - {Http::Headers::get().SetCookie.get(), "OauthExpires=;path=/;Max-Age=;secure;HttpOnly"}, - {Http::Headers::get().SetCookie.get(), "BearerToken=;path=/;Max-Age=;secure;HttpOnly"}, - {Http::Headers::get().Location.get(), - "https://traffic.example.com/test/utf8%C3%83?name=admin&level=trace"}, - }; - - EXPECT_CALL(decoder_callbacks_, - encodeHeaders_(HeaderMapEqualRef(&second_response_headers), true)); - - filter_->finishGetAccessTokenFlow(); - } - { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.oauth_use_url_encoding", "true"}, - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - init(); - // First construct the initial request to the oauth filter with URI parameters. - Http::TestRequestHeaderMapImpl first_request_headers{ - {Http::Headers::get().Path.get(), "/test/utf8%C3%83?name=admin&level=trace"}, - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Post}, - {Http::Headers::get().Scheme.get(), "https"}, - }; + init(); + // First construct the initial request to the oauth filter with URI parameters. + Http::TestRequestHeaderMapImpl first_request_headers{ + {Http::Headers::get().Path.get(), "/test/utf8%C3%83?name=admin&level=trace"}, + {Http::Headers::get().Host.get(), "traffic.example.com"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Post}, + {Http::Headers::get().Scheme.get(), "https"}, + }; - // This is the immediate response - a redirect to the auth cluster. - Http::TestResponseHeaderMapImpl first_response_headers{ - {Http::Headers::get().Status.get(), "302"}, - {Http::Headers::get().Location.get(), - "https://auth.example.com/oauth/" - "authorize/?client_id=" + - TEST_CLIENT_ID + - "&redirect_uri=https%3A%2F%2Ftraffic.example.com%2F_oauth" - "&response_type=code" - "&scope=" + - TEST_ENCODED_AUTH_SCOPES + - "&state=https%3A%2F%2Ftraffic.example.com%2Ftest%2Futf8%25C3%2583%3Fname%3Dadmin%" - "26level%3Dtrace" - "&resource=oauth2-resource&resource=http%3A%2F%2Fexample.com" - "&resource=https%3A%2F%2Fexample.com%2Fsome%2Fpath%252F..%252F%2Futf8%C3%83%3Bfoo%" - "3Dbar%" - "3Fvar1%3D1%26var2%3D2"}, - }; + // This is the immediate response - a redirect to the auth cluster. + Http::TestResponseHeaderMapImpl first_response_headers{ + {Http::Headers::get().Status.get(), "302"}, + {Http::Headers::get().Location.get(), + "https://auth.example.com/oauth/" + "authorize/?client_id=" + + TEST_CLIENT_ID + + "&redirect_uri=https%3A%2F%2Ftraffic.example.com%2F_oauth" + "&response_type=code" + "&scope=" + + TEST_ENCODED_AUTH_SCOPES + + "&state=https%3A%2F%2Ftraffic.example.com%2Ftest%2Futf8%25C3%2583%3Fname%3Dadmin%" + "26level%3Dtrace" + "&resource=oauth2-resource&resource=http%3A%2F%2Fexample.com" + "&resource=https%3A%2F%2Fexample.com%2Fsome%2Fpath%252F..%252F%2Futf8%C3%83%3Bfoo%" + "3Dbar%" + "3Fvar1%3D1%26var2%3D2"}, + }; - // Fail the validation to trigger the OAuth flow. - EXPECT_CALL(*validator_, setParams(_, _)); - EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); + // Fail the validation to trigger the OAuth flow. + EXPECT_CALL(*validator_, setParams(_, _)); + EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); - // Check that the redirect includes the escaped parameter characters using URL encoding. - EXPECT_CALL(decoder_callbacks_, - encodeHeaders_(HeaderMapEqualRef(&first_response_headers), true)); + // Check that the redirect includes the escaped parameter characters using URL encoding. + EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&first_response_headers), true)); - // This represents the beginning of the OAuth filter. - EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, - filter_->decodeHeaders(first_request_headers, false)); + // This represents the beginning of the OAuth filter. + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(first_request_headers, false)); - // This represents the callback request from the authorization server. - Http::TestRequestHeaderMapImpl second_request_headers{ - {Http::Headers::get().Path.get(), - "/_oauth?code=123&state=https%3A%2F%2Ftraffic.example.com%" - "2Ftest%2Futf8%25C3%2583%3Fname%3Dadmin%26level%3Dtrace"}, - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Scheme.get(), "https"}, - }; + // This represents the callback request from the authorization server. + Http::TestRequestHeaderMapImpl second_request_headers{ + {Http::Headers::get().Path.get(), "/_oauth?code=123&state=https%3A%2F%2Ftraffic.example.com%" + "2Ftest%2Futf8%25C3%2583%3Fname%3Dadmin%26level%3Dtrace"}, + {Http::Headers::get().Host.get(), "traffic.example.com"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Scheme.get(), "https"}, + }; - // Deliberately fail the HMAC validation check. - EXPECT_CALL(*validator_, setParams(_, _)); - EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); + // Deliberately fail the HMAC validation check. + EXPECT_CALL(*validator_, setParams(_, _)); + EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); - EXPECT_CALL(*oauth_client_, - asyncGetAccessToken("123", TEST_CLIENT_ID, "asdf_client_secret_fdsa", - "https://traffic.example.com" + TEST_CALLBACK, - AuthType::UrlEncodedBody)); + EXPECT_CALL(*oauth_client_, asyncGetAccessToken("123", TEST_CLIENT_ID, "asdf_client_secret_fdsa", + "https://traffic.example.com" + TEST_CALLBACK, + AuthType::UrlEncodedBody)); - // Invoke the callback logic. As a side effect, state_ will be populated. - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, - filter_->decodeHeaders(second_request_headers, false)); + // Invoke the callback logic. As a side effect, state_ will be populated. + EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndBuffer, + filter_->decodeHeaders(second_request_headers, false)); - EXPECT_EQ(2, config_->stats().oauth_unauthorized_rq_.value()); - EXPECT_EQ(config_->clusterName(), "auth.example.com"); + EXPECT_EQ(1, config_->stats().oauth_unauthorized_rq_.value()); + EXPECT_EQ(config_->clusterName(), "auth.example.com"); - // Expected response after the callback & validation is complete - verifying we kept the - // state and method of the original request, including the query string parameters and UTF8 - // sequences. - Http::TestRequestHeaderMapImpl second_response_headers{ - {Http::Headers::get().Status.get(), "302"}, - {Http::Headers::get().SetCookie.get(), - "OauthHMAC=" - "N2Q1ZWI2M2EwMmUyYTQyODUzNDEwMGI3NTA1ODAzYTdlOTc5YjAyODkyNmY3Y2VkZWU3MGE4MjYyNTYyYmQ2Yw==;" - "path=/;Max-Age=;secure;HttpOnly"}, - {Http::Headers::get().SetCookie.get(), "OauthExpires=;path=/;Max-Age=;secure;HttpOnly"}, - {Http::Headers::get().SetCookie.get(), "BearerToken=;path=/;Max-Age=;secure;HttpOnly"}, - {Http::Headers::get().Location.get(), - "https://traffic.example.com/test/utf8%C3%83?name=admin&level=trace"}, - }; + // Expected response after the callback & validation is complete - verifying we kept the + // state and method of the original request, including the query string parameters and UTF8 + // sequences. + Http::TestRequestHeaderMapImpl second_response_headers{ + {Http::Headers::get().Status.get(), "302"}, + {Http::Headers::get().SetCookie.get(), "OauthHMAC=" + "fV62OgLipChTQQC3UFgDp+l5sCiSb3zt7nCoJiVivWw=;" + "path=/;Max-Age=;secure;HttpOnly"}, + {Http::Headers::get().SetCookie.get(), "OauthExpires=;path=/;Max-Age=;secure;HttpOnly"}, + {Http::Headers::get().SetCookie.get(), "BearerToken=;path=/;Max-Age=;secure;HttpOnly"}, + {Http::Headers::get().Location.get(), + "https://traffic.example.com/test/utf8%C3%83?name=admin&level=trace"}, + }; - EXPECT_CALL(decoder_callbacks_, - encodeHeaders_(HeaderMapEqualRef(&second_response_headers), true)); + EXPECT_CALL(decoder_callbacks_, + encodeHeaders_(HeaderMapEqualRef(&second_response_headers), true)); - filter_->finishGetAccessTokenFlow(); - } + filter_->finishGetAccessTokenFlow(); } /** @@ -1759,20 +1523,8 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { std::string oauthHMAC; -TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokens) { - TestScopedRuntime scoped_runtime; - if (GetParam() == 1) { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - oauthHMAC = - "ZTEzMmIyYzRmNTdmMTdiY2IyYmViZDE3ODA5ZDliOTE2MTRlNzNjYjc4MjBlMTVlOWY1OTM2ZjViZjM4MzAwNA==;"; - } else { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - oauthHMAC = "4TKyxPV/F7yyvr0XgJ2bkWFOc8t4IOFen1k29b84MAQ=;"; - } +TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokens) { + oauthHMAC = "4TKyxPV/F7yyvr0XgJ2bkWFOc8t4IOFen1k29b84MAQ=;"; // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. test_time_.setSystemTime(SystemTime(std::chrono::seconds(1000))); @@ -1806,21 +1558,9 @@ TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokens) { std::chrono::seconds(600)); } -TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshToken) { +TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshToken) { init(getConfig(true /* forward_bearer_token */, true /* use_refresh_token */)); - TestScopedRuntime scoped_runtime; - if (GetParam() == 1) { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - oauthHMAC = - "ZTEzMmIyYzRmNTdmMTdiY2IyYmViZDE3ODA5ZDliOTE2MTRlNzNjYjc4MjBlMTVlOWY1OTM2ZjViZjM4MzAwNA==;"; - } else { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - oauthHMAC = "4TKyxPV/F7yyvr0XgJ2bkWFOc8t4IOFen1k29b84MAQ=;"; - } + oauthHMAC = "4TKyxPV/F7yyvr0XgJ2bkWFOc8t4IOFen1k29b84MAQ=;"; // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. test_time_.setSystemTime(SystemTime(std::chrono::seconds(1000))); @@ -1854,25 +1594,14 @@ TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshToken) { std::chrono::seconds(600)); } -TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndDefaultRefreshTokenExpiresIn) { +TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndDefaultRefreshTokenExpiresIn) { init(getConfig(true /* 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 */)); TestScopedRuntime scoped_runtime; - if (GetParam() == 1) { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - oauthHMAC = - "ZTEzMmIyYzRmNTdmMTdiY2IyYmViZDE3ODA5ZDliOTE2MTRlNzNjYjc4MjBlMTVlOWY1OTM2ZjViZjM4MzAwNA==;"; - } else { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - oauthHMAC = "4TKyxPV/F7yyvr0XgJ2bkWFOc8t4IOFen1k29b84MAQ=;"; - } + oauthHMAC = "4TKyxPV/F7yyvr0XgJ2bkWFOc8t4IOFen1k29b84MAQ=;"; // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. test_time_.setSystemTime(SystemTime(std::chrono::seconds(1000))); @@ -1913,25 +1642,13 @@ TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndDefaultRefr * refresh token. */ -TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndRefreshTokenExpiresInFromJwt) { +TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndRefreshTokenExpiresInFromJwt) { init(getConfig(true /* 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 */)); - TestScopedRuntime scoped_runtime; - if (GetParam() == 1) { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - oauthHMAC = - "MGE2YWQyNjU0YjBmMTA1ZDQzZTE0ODA0OWVlY2Y2Nzc2YjNjZWZjNjI3MDI4M2E5YzUwMGFkMTNkMmM5ZjNkMw==;"; - } else { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - oauthHMAC = "CmrSZUsPEF1D4UgEnuz2d2s878YnAoOpxQCtE9LJ89M=;"; - } + oauthHMAC = "CmrSZUsPEF1D4UgEnuz2d2s878YnAoOpxQCtE9LJ89M=;"; // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. test_time_.setSystemTime(SystemTime(std::chrono::seconds(1000))); @@ -1977,25 +1694,14 @@ TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndRefreshToke * Expected behavior: The age of the cookie with refresh token is equal to zero. */ -TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndExpiredRefreshToken) { +TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndExpiredRefreshToken) { init(getConfig(true /* 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 */)); TestScopedRuntime scoped_runtime; - if (GetParam() == 1) { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - oauthHMAC = - "ZWY3NDZlMDcwNTM3MmIxZmZiNDRmZTBkZDcyY2JlZjEwOWUxMDExOGMwZDc5NDBlYTMxNzRhMGZiY2U0ZDY5Mg==;"; - } else { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - oauthHMAC = "73RuBwU3Kx/7RP4N1yy+8QnhARjA15QOoxdKD7zk1pI=;"; - } + oauthHMAC = "73RuBwU3Kx/7RP4N1yy+8QnhARjA15QOoxdKD7zk1pI=;"; // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. test_time_.setSystemTime(SystemTime(std::chrono::seconds(2554515000))); @@ -2041,25 +1747,14 @@ TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndExpiredRefr * Expected behavior: The age of the cookie with refresh token is equal to default value. */ -TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndNoExpClaimInRefreshToken) { +TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndNoExpClaimInRefreshToken) { init(getConfig(true /* 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 */)); TestScopedRuntime scoped_runtime; - if (GetParam() == 1) { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - oauthHMAC = - "N2FlNDRlNzQwZjgyNmI4YTdmZjQ5YTBjOWQ3ZTc0N2UyYTg3YTJiOTg4NThmZmQyZjg1YjFlZmIwMGZlNTdjMg==;"; - } else { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - oauthHMAC = "euROdA+Ca4p/9JoMnX50fiqHormIWP/S+Fse+wD+V8I=;"; - } + oauthHMAC = "euROdA+Ca4p/9JoMnX50fiqHormIWP/S+Fse+wD+V8I=;"; // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. test_time_.setSystemTime(SystemTime(std::chrono::seconds(1000))); @@ -2098,26 +1793,13 @@ TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokensUseRefreshTokenAndNoExpClaimI std::chrono::seconds(600)); } -INSTANTIATE_TEST_SUITE_P(EndcodingParams, OAuth2Test, testing::Values(1, 0)); - -TEST_P(OAuth2Test, OAuthAccessTokenSucessWithTokens_oauth_use_standard_max_age_value) { +TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokens_oauth_use_standard_max_age_value) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({ {"envoy.reloadable_features.oauth_use_standard_max_age_value", "false"}, }); - if (GetParam() == 1) { - oauthHMAC = - "ZmMzNzFkOWVkY2ZmNzc3M2NjYjk0ZTA0NDM4YTlkOWIxMTUxNmI3NDkyMGRkYjM1Mzg4YTBiMzc4NGRmOWU4Mw==;"; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - } else { - oauthHMAC = "/Dcdntz/d3PMuU4EQ4qdmxFRa3SSDds1OIoLN4TfnoM=;"; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - } + oauthHMAC = "/Dcdntz/d3PMuU4EQ4qdmxFRa3SSDds1OIoLN4TfnoM=;"; // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); @@ -2184,18 +1866,7 @@ TEST_F(OAuth2Test, OAuthBearerTokenFlowFromQueryParameters) { filter_->decodeHeaders(request_headers, false)); } -TEST_P(OAuth2Test, CookieValidatorInTransition) { - TestScopedRuntime scoped_runtime; - if (GetParam() == 0) { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "true"}, - }); - } else { - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.hmac_base64_encoding_only", "false"}, - }); - } - +TEST_F(OAuth2Test, CookieValidatorInTransition) { Http::TestRequestHeaderMapImpl request_headers_base64only{ {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Path.get(), "/_signout"}, @@ -2212,9 +1883,7 @@ TEST_P(OAuth2Test, CookieValidatorInTransition) { test_time_, CookieNames{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", "RefreshToken"}); cookie_validator->setParams(request_headers_base64only, "mock-secret"); - std::cout << "before first valid() " << GetParam() << std::endl; EXPECT_TRUE(cookie_validator->hmacIsValid()); - std::cout << "after first valid() " << GetParam() << std::endl; Http::TestRequestHeaderMapImpl request_headers_hexbase64{ {Http::Headers::get().Host.get(), "traffic.example.com"}, From 73bf655edd1b84815e9348d545da704046402460 Mon Sep 17 00:00:00 2001 From: Juan Manuel Olle Date: Thu, 13 Jun 2024 17:55:52 -0300 Subject: [PATCH 18/21] Sanitize ext_authz path_prefix (#34609) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Juan Manuel Ollé --- changelogs/current.yaml | 5 +++++ .../filters/common/ext_authz/ext_authz_http_impl.cc | 9 ++++++++- .../common/ext_authz/ext_authz_http_impl_test.cc | 11 +++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 6efda2a78003..b827bb1d6d27 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -174,6 +174,11 @@ bug_fixes: change: | Added one option to disable the response body buffering for mirror request. Also introduced a 32MB cap for the response buffer, which can be changed by the runtime flag ``http.async_response_buffer_limit`` based on the product needs. +- area: ext_authz + change: | + Validate http service path_prefix + :ref:`path_prefix `, + Validate http service path_prefix configuration must start with ``/``. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index b00e3d2cca8a..c83ea223f81a 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -98,6 +98,13 @@ struct SuccessResponse { ResponsePtr response_; }; +absl::StatusOr validatePathPrefix(absl::string_view path_prefix) { + if (!path_prefix.empty() && path_prefix[0] != '/') { + return absl::InvalidArgumentError("path_prefix should start with \"/\"."); + } + return std::string(path_prefix); +} + } // namespace // Config @@ -117,7 +124,7 @@ ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3 config.http_service().authorization_response().allowed_upstream_headers_to_append(), context)), cluster_name_(config.http_service().server_uri().cluster()), timeout_(timeout), - path_prefix_(path_prefix), + path_prefix_(THROW_OR_RETURN_VALUE(validatePathPrefix(path_prefix), std::string)), tracing_name_(fmt::format("async {} egress", config.http_service().server_uri().cluster())), request_headers_parser_(THROW_OR_RETURN_VALUE( Router::HeaderParser::configure( diff --git a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc index a093cb710c44..ad5aa4d809ba 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_http_impl_test.cc @@ -259,6 +259,17 @@ TEST_F(ExtAuthzHttpClientTest, TestDefaultAllowedClientAndUpstreamHeaders) { config_->upstreamHeaderMatchers()->matches(Http::Headers::get().ContentLength.get())); } +TEST_F(ExtAuthzHttpClientTest, PathPrefixShouldBeSanitized) { + auto empty_config = createConfig(EMPTY_STRING, 250, ""); + EXPECT_TRUE(empty_config->pathPrefix().empty()); + + auto slash_prefix_config = createConfig(EMPTY_STRING, 250, "/the_prefix"); + EXPECT_EQ(slash_prefix_config->pathPrefix(), "/the_prefix"); + + EXPECT_THROW_WITH_MESSAGE(createConfig(EMPTY_STRING, 250, "the_prefix"), EnvoyException, + "path_prefix should start with \"/\"."); +} + // Verify client response when the authorization server returns a 200 OK and path_prefix is // configured. TEST_F(ExtAuthzHttpClientTest, AuthorizationOkWithPathRewrite) { From 3a0c2a4ffb5e199eed0be1738ff8e6590dcf94c6 Mon Sep 17 00:00:00 2001 From: code Date: Fri, 14 Jun 2024 12:43:34 +0800 Subject: [PATCH 19/21] api: common key/value pair and options for key/value appending (#34550) Seems until now, we still have no a common key/value API that be applied to string-map-like structures. This PR add one. Then we can use this API for query mutation, cookie mutation. And this could also be used by non-HTTP headers (like dubbo attachment) or any string-map-like structures. Risk Level: low. API only. Testing: n/a. Signed-off-by: wbpcode --- api/envoy/config/core/v3/base.proto | 53 +++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index 2f2b06a46756..df91565d0a7b 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -303,6 +303,59 @@ message RuntimeFeatureFlag { string runtime_key = 2 [(validate.rules).string = {min_len: 1}]; } +message KeyValue { + // The key of the key/value pair. + string key = 1 [(validate.rules).string = {min_len: 1 max_bytes: 16384}]; + + // The value of the key/value pair. + bytes value = 2; +} + +// Key/value pair plus option to control append behavior. This is used to specify +// key/value pairs that should be appended to a set of existing key/value pairs. +message KeyValueAppend { + // Describes the supported actions types for key/value pair append action. + enum KeyValueAppendAction { + // If the key already exists, this action will result in the following behavior: + // + // - Comma-concatenated value if multiple values are not allowed. + // - New value added to the list of values if multiple values are allowed. + // + // If the key doesn't exist then this will add pair with specified key and value. + APPEND_IF_EXISTS_OR_ADD = 0; + + // This action will add the key/value pair if it doesn't already exist. If the + // key already exists then this will be a no-op. + ADD_IF_ABSENT = 1; + + // This action will overwrite the specified value by discarding any existing + // values if the key already exists. If the key doesn't exist then this will add + // the pair with specified key and value. + OVERWRITE_IF_EXISTS_OR_ADD = 2; + + // This action will overwrite the specified value by discarding any existing + // values if the key already exists. If the key doesn't exist then this will + // be no-op. + OVERWRITE_IF_EXISTS = 3; + } + + // Key/value pair entry that this option to append or overwrite. + KeyValue entry = 1 [(validate.rules).message = {required: true}]; + + // Describes the action taken to append/overwrite the given value for an existing + // key or to only add this key if it's absent. + KeyValueAppendAction action = 2 [(validate.rules).enum = {defined_only: true}]; +} + +// Key/value pair to append or remove. +message KeyValueMutation { + // Key/value pair to append or overwrite. Only one of ``append`` or ``remove`` can be set. + KeyValueAppend append = 1; + + // Key to remove. Only one of ``append`` or ``remove`` can be set. + string remove = 2 [(validate.rules).string = {max_bytes: 16384}]; +} + // Query parameter name/value pair. message QueryParameter { // The key of the query parameter. Case sensitive. From 2b422e99b0ae97c1b9e01df1a33b6252d7e77674 Mon Sep 17 00:00:00 2001 From: Fernando Cainelli Date: Fri, 14 Jun 2024 01:53:55 -0300 Subject: [PATCH 20/21] grpc: propagate tracing headers when using google grpc streams (#34395) Following up on #33665, which adds tracing to Envoy gRPC streams, this PR introduces the same support using Google gRPC for completeness to #21119. Risk Level: Low Testing: The same manual tests described in #33665, besides the integration tests of extproc are also testing google grpc now. Signed-off-by: Fernando Cainelli --- changelogs/current.yaml | 5 ++- .../common/grpc/google_async_client_impl.cc | 36 +++++++++++++++- source/common/grpc/google_async_client_impl.h | 1 + .../grpc/google_async_client_impl_test.cc | 21 +++++++++- .../ext_proc/ext_proc_integration_test.cc | 42 ++++++++++++------- .../http/ext_proc/tracer_test_filter.cc | 8 +++- 6 files changed, 92 insertions(+), 21 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index b827bb1d6d27..557639cd1f39 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -41,8 +41,9 @@ minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* - area: grpc change: | - Changes in ``AsyncStreamImpl`` now propagate tracing context headers in bidirectional streams when using - :ref:`Envoy gRPC client `. Previously, tracing context headers + Changes in ``AsyncStreamImpl`` and ``GoogleAsyncStreamImpl`` now propagate tracing context headers in bidirectional streams when using + :ref:`Envoy gRPC client ` or + :ref:`Google C++ gRPC client `. Previously, tracing context headers were not being set when calling external services such as ``ext_proc``. - area: http change: | diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index 1d78db8abc0f..ef6fdb86c525 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -169,7 +169,26 @@ GoogleAsyncStreamImpl::GoogleAsyncStreamImpl(GoogleAsyncClientImpl& parent, service_full_name_(service_full_name), method_name_(method_name), callbacks_(callbacks), options_(options), unused_stream_info_(Http::Protocol::Http2, dispatcher_.timeSource(), Network::ConnectionInfoProviderSharedPtr{}, - StreamInfo::FilterState::LifeSpan::FilterChain) {} + StreamInfo::FilterState::LifeSpan::FilterChain) { + // TODO(cainelli): add a common library for tracing tags between gRPC implementations. + if (options.parent_span_ != nullptr) { + const std::string child_span_name = + options.child_span_name_.empty() + ? absl::StrCat("async ", service_full_name, ".", method_name, " egress") + : options.child_span_name_; + current_span_ = options.parent_span_->spawnChild(Tracing::EgressConfig::get(), child_span_name, + parent.timeSource().systemTime()); + current_span_->setTag(Tracing::Tags::get().UpstreamCluster, parent.stat_prefix_); + current_span_->setTag(Tracing::Tags::get().UpstreamAddress, parent.target_uri_); + current_span_->setTag(Tracing::Tags::get().Component, Tracing::Tags::get().Proxy); + } else { + current_span_ = std::make_unique(); + } + + if (options.sampled_.has_value()) { + current_span_->setSampled(options.sampled_.value()); + } +} GoogleAsyncStreamImpl::~GoogleAsyncStreamImpl() { ENVOY_LOG(debug, "GoogleAsyncStreamImpl destruct"); @@ -194,6 +213,13 @@ void GoogleAsyncStreamImpl::initialize(bool /*buffer_body_for_retry*/) { // request headers should not be stored in stream_info. // Maybe put it to parent_context? parent_.metadata_parser_->evaluateHeaders(*initial_metadata, options_.parent_context.stream_info); + Tracing::HttpTraceContext trace_context(*initial_metadata); + Tracing::UpstreamContext upstream_context(nullptr, // host_ + nullptr, // cluster_ + Tracing::ServiceType::GoogleGrpc, // service_type_ + true // async_client_span_ + ); + current_span_->injectContext(trace_context, upstream_context); callbacks_.onCreateInitialMetadata(*initial_metadata); initial_metadata->iterate([this](const Http::HeaderEntry& header) { ctxt_.AddMetadata(std::string(header.key().getStringView()), @@ -226,6 +252,11 @@ void GoogleAsyncStreamImpl::notifyRemoteClose(Status::GrpcStatus grpc_status, parent_.stats_.streams_closed_[grpc_status]->inc(); } ENVOY_LOG(debug, "notifyRemoteClose {} {}", grpc_status, message); + current_span_->setTag(Tracing::Tags::get().GrpcStatusCode, std::to_string(grpc_status)); + if (grpc_status != Status::WellKnownGrpcStatus::Ok) { + current_span_->setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True); + } + current_span_->finishSpan(); callbacks_.onReceiveTrailingMetadata(trailing_metadata ? std::move(trailing_metadata) : Http::ResponseTrailerMapImpl::create()); callbacks_.onRemoteClose(grpc_status, message); @@ -241,6 +272,9 @@ void GoogleAsyncStreamImpl::sendMessageRaw(Buffer::InstancePtr&& request, bool e void GoogleAsyncStreamImpl::closeStream() { // Empty EOS write queued. + current_span_->setTag(Tracing::Tags::get().Status, Tracing::Tags::get().Canceled); + current_span_->finishSpan(); + write_pending_queue_.emplace(); writeQueued(); } diff --git a/source/common/grpc/google_async_client_impl.h b/source/common/grpc/google_async_client_impl.h index 45eceae18434..67326b01b0a0 100644 --- a/source/common/grpc/google_async_client_impl.h +++ b/source/common/grpc/google_async_client_impl.h @@ -314,6 +314,7 @@ class GoogleAsyncStreamImpl : public RawAsyncStream, // freed. uint32_t inflight_tags_{}; + Tracing::SpanPtr current_span_; // This is unused. StreamInfo::StreamInfoImpl unused_stream_info_; diff --git a/test/common/grpc/google_async_client_impl_test.cc b/test/common/grpc/google_async_client_impl_test.cc index cbc2b0b90aa2..f75d30528f55 100644 --- a/test/common/grpc/google_async_client_impl_test.cc +++ b/test/common/grpc/google_async_client_impl_test.cc @@ -108,13 +108,30 @@ TEST_F(EnvoyGoogleAsyncClientImplTest, ThreadSafe) { TEST_F(EnvoyGoogleAsyncClientImplTest, StreamHttpStartFail) { initialize(); + Tracing::MockSpan parent_span; + Tracing::MockSpan* child_span{new Tracing::MockSpan()}; + + EXPECT_CALL(parent_span, spawnChild_(_, "async helloworld.Greeter.SayHello egress", _)) + .WillOnce(Return(child_span)); + EXPECT_CALL(*child_span, + setTag(Eq(Tracing::Tags::get().Component), Eq(Tracing::Tags::get().Proxy))); + EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), Eq("test_cluster"))); + EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().UpstreamAddress), Eq("fake_address"))); + EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("14"))); + EXPECT_CALL(*child_span, injectContext(_, _)); + EXPECT_CALL(*child_span, finishSpan()); + EXPECT_CALL(*child_span, setSampled(true)); + EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); + EXPECT_CALL(*stub_factory_.stub_, PrepareCall_(_, _, _)).WillOnce(Return(nullptr)); MockAsyncStreamCallbacks grpc_callbacks; EXPECT_CALL(grpc_callbacks, onCreateInitialMetadata(_)); EXPECT_CALL(grpc_callbacks, onReceiveTrailingMetadata_(_)); EXPECT_CALL(grpc_callbacks, onRemoteClose(Status::WellKnownGrpcStatus::Unavailable, "")); - auto grpc_stream = - grpc_client_->start(*method_descriptor_, grpc_callbacks, Http::AsyncClient::StreamOptions()); + Http::AsyncClient::StreamOptions stream_options; + stream_options.setParentSpan(parent_span).setSampled(true); + + auto grpc_stream = grpc_client_->start(*method_descriptor_, grpc_callbacks, stream_options); EXPECT_TRUE(grpc_stream == nullptr); } diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index f5ab80b2b707..9d8dfdcc0521 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -630,9 +630,6 @@ TEST_P(ExtProcIntegrationTest, GetAndCloseStream) { } TEST_P(ExtProcIntegrationTest, GetAndCloseStreamWithTracing) { - if (!IsEnvoyGrpc()) { - GTEST_SKIP() << "Tracing is currently only supported for Envoy gRPC"; - } initializeConfig(); config_helper_.addConfigModifier([&](HttpConnectionManager& cm) { test::integration::filters::ExpectSpan ext_proc_span; @@ -641,9 +638,13 @@ TEST_P(ExtProcIntegrationTest, GetAndCloseStreamWithTracing) { ext_proc_span.set_context_injected(true); ext_proc_span.set_sampled(true); ext_proc_span.mutable_tags()->insert({"grpc.status_code", "0"}); - ext_proc_span.mutable_tags()->insert({"upstream_address", "ext_proc_server_0"}); ext_proc_span.mutable_tags()->insert({"upstream_cluster", "ext_proc_server_0"}); - + if (IsEnvoyGrpc()) { + ext_proc_span.mutable_tags()->insert({"upstream_address", "ext_proc_server_0"}); + } else { + ext_proc_span.mutable_tags()->insert( + {"upstream_address", grpc_upstreams_[0]->localAddress()->asString()}); + } test::integration::filters::TracerTestConfig test_config; test_config.mutable_expect_spans()->Add()->CopyFrom(ext_proc_span); @@ -659,6 +660,9 @@ TEST_P(ExtProcIntegrationTest, GetAndCloseStreamWithTracing) { waitForFirstMessage(*grpc_upstreams_[0], request_headers_msg); processor_stream_->startGrpcStream(); + EXPECT_FALSE(processor_stream_->headers().get(LowerCaseString("traceparent")).empty()) + << "expected traceparent header"; + processor_stream_->finishGrpcStream(Grpc::Status::Ok); handleUpstreamRequest(); verifyDownstreamResponse(*response, 200); @@ -697,9 +701,6 @@ TEST_P(ExtProcIntegrationTest, GetAndFailStream) { } TEST_P(ExtProcIntegrationTest, GetAndFailStreamWithTracing) { - if (!IsEnvoyGrpc()) { - GTEST_SKIP() << "Tracing is currently only supported for Envoy gRPC"; - } initializeConfig(); config_helper_.addConfigModifier([&](HttpConnectionManager& cm) { test::integration::filters::ExpectSpan ext_proc_span; @@ -709,8 +710,13 @@ TEST_P(ExtProcIntegrationTest, GetAndFailStreamWithTracing) { ext_proc_span.set_sampled(true); ext_proc_span.mutable_tags()->insert({"grpc.status_code", "2"}); ext_proc_span.mutable_tags()->insert({"error", "true"}); - ext_proc_span.mutable_tags()->insert({"upstream_address", "ext_proc_server_0"}); ext_proc_span.mutable_tags()->insert({"upstream_cluster", "ext_proc_server_0"}); + if (IsEnvoyGrpc()) { + ext_proc_span.mutable_tags()->insert({"upstream_address", "ext_proc_server_0"}); + } else { + ext_proc_span.mutable_tags()->insert( + {"upstream_address", grpc_upstreams_[0]->localAddress()->asString()}); + } test::integration::filters::TracerTestConfig test_config; test_config.mutable_expect_spans()->Add()->CopyFrom(ext_proc_span); @@ -725,6 +731,9 @@ TEST_P(ExtProcIntegrationTest, GetAndFailStreamWithTracing) { ProcessingRequest request_headers_msg; waitForFirstMessage(*grpc_upstreams_[0], request_headers_msg); + EXPECT_FALSE(processor_stream_->headers().get(LowerCaseString("traceparent")).empty()) + << "expected traceparent header"; + // Fail the stream immediately processor_stream_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "500"}}, true); verifyDownstreamResponse(*response, 500); @@ -2355,10 +2364,6 @@ TEST_P(ExtProcIntegrationTest, RequestMessageTimeout) { } TEST_P(ExtProcIntegrationTest, RequestMessageTimeoutWithTracing) { - if (!IsEnvoyGrpc()) { - GTEST_SKIP() << "Tracing is currently only supported for Envoy gRPC"; - } - // ensure 200 ms timeout proto_config_.mutable_message_timeout()->set_nanos(200000000); initializeConfig(); @@ -2371,9 +2376,13 @@ TEST_P(ExtProcIntegrationTest, RequestMessageTimeoutWithTracing) { ext_proc_span.set_sampled(true); ext_proc_span.mutable_tags()->insert({"status", "canceled"}); ext_proc_span.mutable_tags()->insert({"error", ""}); // not an error - ext_proc_span.mutable_tags()->insert({"upstream_address", "ext_proc_server_0"}); ext_proc_span.mutable_tags()->insert({"upstream_cluster", "ext_proc_server_0"}); - + if (IsEnvoyGrpc()) { + ext_proc_span.mutable_tags()->insert({"upstream_address", "ext_proc_server_0"}); + } else { + ext_proc_span.mutable_tags()->insert( + {"upstream_address", grpc_upstreams_[0]->localAddress()->asString()}); + } test::integration::filters::TracerTestConfig test_config; test_config.mutable_expect_spans()->Add()->CopyFrom(ext_proc_span); @@ -2391,6 +2400,9 @@ TEST_P(ExtProcIntegrationTest, RequestMessageTimeoutWithTracing) { return false; }); + EXPECT_FALSE(processor_stream_->headers().get(LowerCaseString("traceparent")).empty()) + << "expected traceparent header"; + // We should immediately have an error response now verifyDownstreamResponse(*response, 500); } diff --git a/test/extensions/filters/http/ext_proc/tracer_test_filter.cc b/test/extensions/filters/http/ext_proc/tracer_test_filter.cc index 15c7b1b348ea..a76b91e911f8 100644 --- a/test/extensions/filters/http/ext_proc/tracer_test_filter.cc +++ b/test/extensions/filters/http/ext_proc/tracer_test_filter.cc @@ -14,6 +14,10 @@ namespace Extensions { namespace HttpFilters { namespace ExternalProcessing { +const Tracing::TraceContextHandler& traceParentHeader() { + CONSTRUCT_ON_FIRST_USE(Tracing::TraceContextHandler, "traceparent"); +} + struct ExpectedSpan { std::string operation_name; bool sampled; @@ -58,7 +62,9 @@ class Span : public Tracing::Span { void setOperation(absl::string_view operation_name) { operation_name_ = operation_name; } void setSampled(bool do_sample) { sampled_ = do_sample; } - void injectContext(Tracing::TraceContext&, const Tracing::UpstreamContext&) { + void injectContext(Tracing::TraceContext& trace_context, const Tracing::UpstreamContext&) { + std::string traceparent_header_value = "1"; + traceParentHeader().setRefKey(trace_context, traceparent_header_value); context_injected_ = true; } void setBaggage(absl::string_view, absl::string_view) { /* not implemented */ From bd5a73e09a5f1ef993abf2415c6526172e3c97b5 Mon Sep 17 00:00:00 2001 From: "Aaron.Zhang" Date: Fri, 14 Jun 2024 13:01:37 +0800 Subject: [PATCH 21/21] use exact match for illegal path check (#34539) In our environment, the file system directory is as follows: Tue Jun 04 22:28:35][#48# ]$df -h Filesystem Size Used Avail Use% Mounted on tmpfs 77G 104K 77G 1% /dev/shm tmpfs 31G 9.8M 31G 1% /run tmpfs 5.0M 0 5.0M 0% /run/lock tmpfs 4.0M 0 4.0M 0% /sys/fs/cgroup /dev/mapper/atomicos-root 150G 144G 5.8G 97% /sysroot /dev/vda2 483M 84M 400M 18% /boot /dev/vdc 1.2T 87G 1.1T 8% /sysroot/home/centos/external We have a directory named /sysroot. If the envoy config file is the that directory, envoy can not start up. [2024-06-04 22:28:35.581][3382724][critical][main] [source/server/server.cc:131] error initializing configuration 'configs/envoy.yaml': Invalid path: configs/envoy.yaml [2024-06-04 22:28:35.581][3382724][info][main] [source/server/server.cc:972] exiting Invalid path: configs/envoy.yaml In my mind, envoy should only check the default system directory such as /dev /sys /proc as illegal path. So it is better to use exact match instead of startwith match. Signed-off-by: Zhang Bo --- source/common/filesystem/posix/filesystem_impl.cc | 10 +++++++--- tools/spelling/spelling_dictionary.txt | 1 + 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc index d9714ce37b9b..b10aa7bdee18 100644 --- a/source/common/filesystem/posix/filesystem_impl.cc +++ b/source/common/filesystem/posix/filesystem_impl.cc @@ -339,9 +339,13 @@ bool InstanceImplPosix::illegalPath(const std::string& path) { // platform in the future, growing these or relaxing some constraints (e.g. // there are valid reasons to go via /proc for file paths). // TODO(htuch): Optimize this as a hash lookup if we grow any further. - if (absl::StartsWith(canonical_path.return_value_, "/dev") || - absl::StartsWith(canonical_path.return_value_, "/sys") || - absl::StartsWith(canonical_path.return_value_, "/proc")) { + // It will allow the canonical path such as /sysroot/ which is not the + // default reserved directories (/dev, /sys, /proc) + if (absl::StartsWith(canonical_path.return_value_, "/dev/") || + absl::StartsWith(canonical_path.return_value_, "/sys/") || + absl::StartsWith(canonical_path.return_value_, "/proc/") || + canonical_path.return_value_ == "/dev" || canonical_path.return_value_ == "/sys" || + canonical_path.return_value_ == "/proc") { return true; } return false; diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 4c8efee1ae6c..670192ef17d7 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -1349,6 +1349,7 @@ sys syscall syscalls sysctl +sysroot sz tchar tchars