From ceac456da1beeeb5949f7e542639def3c86783a3 Mon Sep 17 00:00:00 2001 From: Tianyu <72890320+tyxia@users.noreply.github.com> Date: Tue, 24 Oct 2023 09:19:45 -0400 Subject: [PATCH 01/17] composite_filter: Demonstrate the usage of route-level config (#30442) * valid per route config Signed-off-by: tyxia * tweak names Signed-off-by: tyxia * add new line Signed-off-by: tyxia * update test Signed-off-by: tyxia --------- Signed-off-by: tyxia --- .../composite_filter_integration_test.cc | 37 +++++++++++++ test/integration/BUILD | 2 +- test/integration/filters/BUILD | 1 + .../filters/set_response_code_filter.cc | 52 +++++++++++++++++-- .../set_response_code_filter_config.proto | 7 +++ .../http_typed_per_filter_config_test.cc | 13 +++-- 6 files changed, 99 insertions(+), 13 deletions(-) diff --git a/test/extensions/filters/http/composite/composite_filter_integration_test.cc b/test/extensions/filters/http/composite/composite_filter_integration_test.cc index fe75b1a45213..02ec08a30a86 100644 --- a/test/extensions/filters/http/composite/composite_filter_integration_test.cc +++ b/test/extensions/filters/http/composite/composite_filter_integration_test.cc @@ -33,6 +33,7 @@ using envoy::extensions::common::matching::v3::ExtensionWithMatcherPerRoute; using envoy::extensions::filters::http::composite::v3::ExecuteFilterAction; using envoy::type::matcher::v3::HttpRequestHeaderMatchInput; using test::integration::filters::SetResponseCodeFilterConfig; +using test::integration::filters::SetResponseCodePerRouteFilterConfig; using xds::type::matcher::v3::Matcher_OnMatch; class CompositeFilterIntegrationTest : public testing::TestWithParam, @@ -87,6 +88,25 @@ class CompositeFilterIntegrationTest : public testing::TestWithParammutable_virtual_hosts(0); + auto* route = vh->mutable_routes()->Mutable(0); + route->mutable_match()->set_prefix(route_prefix); + route->mutable_route()->set_cluster("cluster_0"); + (*route->mutable_typed_per_filter_config())[filter_name].PackFrom( + set_response_code_per_route_config); + }); + } + void prependCompositeFilter(const std::string& name = "composite") { config_helper_.prependFilter(absl::StrFormat(R"EOF( name: %s @@ -166,6 +186,23 @@ TEST_P(CompositeFilterIntegrationTest, TestPerRoute) { EXPECT_THAT(response->headers(), Http::HttpStatusIs("401")); } +// Verifies set_response_code filter's per-route config overrides the filter config. +TEST_P(CompositeFilterIntegrationTest, TestPerRouteResponseCodeConfig) { + std::string top_level_filter_name = "match_delegate_filter"; + prependCompositeFilter(top_level_filter_name); + + addResponseCodeFilterPerRouteConfig(/*filter_name=*/top_level_filter_name, + /*route_prefix=*/"/somepath", + /*code=*/406); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeRequestWithBody(match_request_headers_, 1024); + ASSERT_TRUE(response->waitForEndStream()); + // Verifies that 406 from per route config is used, rather than 403 from filter config. + EXPECT_THAT(response->headers(), Http::HttpStatusIs("406")); +} + // Test an empty match tree resolving with a per route config. TEST_P(CompositeFilterIntegrationTest, TestPerRouteEmptyMatcher) { config_helper_.prependFilter(R"EOF( diff --git a/test/integration/BUILD b/test/integration/BUILD index 28155bc57347..926ba2d046f7 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -722,7 +722,7 @@ envoy_cc_test( ]), deps = [ ":http_integration_lib", - "//test/integration/filters:set_response_code_filter_lib", + "//test/integration/filters:set_route_filter_lib", ], ) diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 051c86049bd2..867bca9cef3e 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -487,6 +487,7 @@ envoy_cc_test_library( ":set_response_code_filter_config_proto_cc_proto", "//envoy/http:filter_interface", "//envoy/registry", + "//source/common/http:utility_lib", "//source/extensions/filters/http/common:factory_base_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", ], diff --git a/test/integration/filters/set_response_code_filter.cc b/test/integration/filters/set_response_code_filter.cc index f2cc3dc8e633..08c86df2cb7d 100644 --- a/test/integration/filters/set_response_code_filter.cc +++ b/test/integration/filters/set_response_code_filter.cc @@ -3,6 +3,7 @@ #include "envoy/http/filter.h" #include "envoy/registry/registry.h" +#include "source/common/http/utility.h" #include "source/extensions/filters/http/common/factory_base.h" #include "source/extensions/filters/http/common/pass_through_filter.h" @@ -27,14 +28,45 @@ class SetResponseCodeFilterConfig { ThreadLocal::TypedSlot<> tls_slot_; }; +class SetResponseCodeFilterRouteSpecificConfig : public Envoy::Router::RouteSpecificFilterConfig { +public: + SetResponseCodeFilterRouteSpecificConfig(const std::string& prefix, uint32_t code, + const std::string& body, + Server::Configuration::FactoryContextBase& context) + : prefix_(prefix), code_(code), body_(body), tls_slot_(context.threadLocal()) {} + + const std::string prefix_; + const uint32_t code_; + const std::string body_; + // Allocate a slot to validate that it is destroyed on a main thread only. + ThreadLocal::TypedSlot<> tls_slot_; +}; + class SetResponseCodeFilter : public Http::PassThroughFilter { public: SetResponseCodeFilter(std::shared_ptr config) : config_(config) {} Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool) override { - if (absl::StartsWith(headers.Path()->value().getStringView(), config_->prefix_)) { - decoder_callbacks_->sendLocalReply(static_cast(config_->code_), config_->body_, - nullptr, absl::nullopt, ""); + const auto* per_route_config = Envoy::Http::Utility::resolveMostSpecificPerFilterConfig< + SetResponseCodeFilterRouteSpecificConfig>(decoder_callbacks_); + + std::string prefix; + uint32_t code; + std::string body; + // Route level config takes precedence over filter level config, if present. + if (per_route_config != nullptr) { + prefix = per_route_config->prefix_; + code = per_route_config->code_; + body = per_route_config->body_; + } else { + prefix = config_->prefix_; + code = config_->code_; + body = config_->body_; + } + + if (absl::StartsWith(headers.Path()->value().getStringView(), prefix)) { + decoder_callbacks_->sendLocalReply(static_cast(code), body, nullptr, + absl::nullopt, ""); return Http::FilterHeadersStatus::StopIteration; } return Http::FilterHeadersStatus::Continue; @@ -44,8 +76,10 @@ class SetResponseCodeFilter : public Http::PassThroughFilter { const std::shared_ptr config_; }; -class SetResponseCodeFilterFactory : public Extensions::HttpFilters::Common::FactoryBase< - test::integration::filters::SetResponseCodeFilterConfig> { +class SetResponseCodeFilterFactory + : public Extensions::HttpFilters::Common::FactoryBase< + test::integration::filters::SetResponseCodeFilterConfig, + test::integration::filters::SetResponseCodePerRouteFilterConfig> { public: SetResponseCodeFilterFactory() : FactoryBase("set-response-code-filter") {} @@ -68,6 +102,14 @@ class SetResponseCodeFilterFactory : public Extensions::HttpFilters::Common::Fac callbacks.addStreamFilter(std::make_shared(filter_config)); }; } + + Router::RouteSpecificFilterConfigConstSharedPtr createRouteSpecificFilterConfigTyped( + const test::integration::filters::SetResponseCodePerRouteFilterConfig& proto_config, + Server::Configuration::ServerFactoryContext& context, + ProtobufMessage::ValidationVisitor&) override { + return std::make_shared( + proto_config.prefix(), proto_config.code(), proto_config.body(), context); + } }; REGISTER_FACTORY(SetResponseCodeFilterFactory, Server::Configuration::NamedHttpFilterConfigFactory); diff --git a/test/integration/filters/set_response_code_filter_config.proto b/test/integration/filters/set_response_code_filter_config.proto index 09765c970d01..d9ca4298101f 100644 --- a/test/integration/filters/set_response_code_filter_config.proto +++ b/test/integration/filters/set_response_code_filter_config.proto @@ -9,3 +9,10 @@ message SetResponseCodeFilterConfig { uint32 code = 2 [(validate.rules).uint32 = {lt: 600 gte: 200}]; string body = 3; } + +// Per route config overrides filter config, if present. +message SetResponseCodePerRouteFilterConfig { + string prefix = 1; + uint32 code = 2 [(validate.rules).uint32 = {lt: 600 gte: 200}]; + string body = 3; +} diff --git a/test/integration/http_typed_per_filter_config_test.cc b/test/integration/http_typed_per_filter_config_test.cc index d6a07c91b89f..7235c40f65ae 100644 --- a/test/integration/http_typed_per_filter_config_test.cc +++ b/test/integration/http_typed_per_filter_config_test.cc @@ -1,6 +1,6 @@ #include "envoy/config/route/v3/route_components.pb.h" -#include "test/integration/filters/set_response_code_filter_config.pb.h" +#include "test/integration/filters/set_route_filter_config.pb.h" #include "test/integration/http_integration.h" #include "gtest/gtest.h" @@ -18,21 +18,20 @@ TEST_F(HTTPTypedPerFilterConfigTest, RejectUnsupportedTypedPerFilterConfig) { config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) { - test::integration::filters::SetResponseCodeFilterConfig response_code; - response_code.set_code(403); + test::integration::filters::SetRouteFilterConfig set_route_config; auto* virtual_host = hcm.mutable_route_config()->mutable_virtual_hosts(0); auto* config = virtual_host->mutable_typed_per_filter_config(); - (*config)["set-response-code-filter"].PackFrom(response_code); + (*config)["set-route-filter"].PackFrom(set_route_config); auto* filter = hcm.mutable_http_filters()->Add(); - filter->set_name("set-response-code-filter"); - filter->mutable_typed_config()->PackFrom(response_code); + filter->set_name("set-route-filter"); + filter->mutable_typed_config()->PackFrom(set_route_config); // keep router the last auto size = hcm.http_filters_size(); hcm.mutable_http_filters()->SwapElements(size - 2, size - 1); }); - EXPECT_DEATH(initialize(), "The filter set-response-code-filter doesn't support virtual host or " + EXPECT_DEATH(initialize(), "The filter set-route-filter doesn't support virtual host or " "route specific configurations"); } From b87d861921cfefb2f066bdfa5a90bfb37f1c57f7 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 24 Oct 2023 10:59:58 -0400 Subject: [PATCH 02/17] docs: updating h3 upstream status (#30456) Signed-off-by: Alyssa Wilk --- docs/root/intro/arch_overview/http/http3.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/root/intro/arch_overview/http/http3.rst b/docs/root/intro/arch_overview/http/http3.rst index a8d969c45aab..e87cd4dc6b7b 100644 --- a/docs/root/intro/arch_overview/http/http3.rst +++ b/docs/root/intro/arch_overview/http/http3.rst @@ -8,8 +8,8 @@ HTTP/3 overview While HTTP/3 **downstream support is deemed ready for production use**, improvements are ongoing, tracked in the `area-quic `_ tag. - HTTP/3 **upstream support is fine for locally controlled networks**, but is not ready for - general internet use, and is missing some key latency features. See details below. + HTTP/3 **upstream support is fine for locally controlled networks**, but is alpha for + general internet use - key features are implemented but have not been tested at scale. .. _arch_overview_http3_downstream: From e2a509ef1c0c54646afbdb7c7889a719051b59f7 Mon Sep 17 00:00:00 2001 From: phlax Date: Tue, 24 Oct 2023 16:10:21 +0100 Subject: [PATCH 03/17] bazel: Patch aspect lib to resolve external build issue (#30453) bazel/patch: Patch aspect lib to resolve external build issue upstream issue is https://github.com/aspect-build/bazel-lib/issues/548 pr is: https://github.com/aspect-build/bazel-lib/pull/547 Currently the websites and archive patch this indepenendently This caused a breakage (in all 3) when aspect lib was updated https://github.com/envoyproxy/envoy-website/issues/368 This should prevent that until there is some upstream resolution Signed-off-by: Ryan Northey --- bazel/aspect.patch | 20 ++++++++++++++++++++ bazel/repositories.bzl | 7 ++++++- 2 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 bazel/aspect.patch diff --git a/bazel/aspect.patch b/bazel/aspect.patch new file mode 100644 index 000000000000..b9047daa6df9 --- /dev/null +++ b/bazel/aspect.patch @@ -0,0 +1,20 @@ +diff --git a/lib/private/yq.bzl b/lib/private/yq.bzl +index 29ca3d7..c8cd5eb 100644 +--- a/lib/private/yq.bzl ++++ b/lib/private/yq.bzl +@@ -71,10 +71,13 @@ def _yq_impl(ctx): + + # For split operations, yq outputs files in the same directory so we + # must cd to the correct output dir before executing it +- bin_dir = "/".join([ctx.bin_dir.path, ctx.label.package]) if ctx.label.package else ctx.bin_dir.path ++ bin_dir = ctx.bin_dir.path ++ if ctx.label.workspace_name: ++ bin_dir = "%s/external/%s" % (bin_dir, ctx.label.workspace_name) ++ bin_dir = "/".join([bin_dir, ctx.label.package]) if ctx.label.package else bin_dir + escape_bin_dir = _escape_path(bin_dir) + cmd = "cd {bin_dir} && {yq} {args} {eval_cmd} {expression} {sources} {maybe_out}".format( +- bin_dir = ctx.bin_dir.path + "/" + ctx.label.package, ++ bin_dir = bin_dir, + yq = escape_bin_dir + yq_bin.path, + eval_cmd = "eval" if len(inputs) <= 1 else "eval-all", + args = " ".join(args), diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 26cac737ab04..ccc6ad156701 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -339,7 +339,12 @@ def envoy_dependencies(skip_targets = []): patches = ["@envoy//bazel:rules_pkg.patch"], ) external_http_archive("com_github_aignas_rules_shellcheck") - external_http_archive("aspect_bazel_lib") + external_http_archive( + "aspect_bazel_lib", + patch_args = ["-p1"], + patches = ["@envoy//bazel:aspect.patch"], + ) + _com_github_fdio_vpp_vcl() # Unconditional, since we use this only for compiler-agnostic fuzzing utils. From b42831179d75e55e73124804bf7939c09cc4af97 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 24 Oct 2023 11:20:24 -0400 Subject: [PATCH 04/17] runtime: removing envoy.reloadable_features.correctly_validate_alpn (#30434) Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: inline Fixes #30424 Signed-off-by: Alyssa Wilk --- changelogs/current.yaml | 4 ++++ source/common/http/conn_manager_utility.cc | 4 +--- source/common/runtime/runtime_features.cc | 1 - 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index c1e023b0b488..6563980fc8a3 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -21,6 +21,10 @@ bug_fixes: removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` + +- area: http + change: | + removed ``envoy.reloadable_features.correctly_validate_alpn`` and legacy code paths. - area: router change: | Removed the deprecated ``envoy.reloadable_features.prohibit_route_refresh_after_response_headers_sent`` diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 107cdd9fe270..b6f450af5547 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -298,9 +298,7 @@ void ConnectionManagerUtility::cleanInternalHeaders( request_headers.removeEnvoyDecoratorOperation(); request_headers.removeEnvoyDownstreamServiceCluster(); request_headers.removeEnvoyDownstreamServiceNode(); - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_original_path")) { - request_headers.removeEnvoyOriginalPath(); - } + request_headers.removeEnvoyOriginalPath(); } // Headers to be stripped from edge *and* intermediate-hop external requests. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index b7ea648b2694..dc538c51dc77 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -72,7 +72,6 @@ RUNTIME_GUARD(envoy_reloadable_features_oauth_use_url_encoding); RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout); RUNTIME_GUARD(envoy_reloadable_features_overload_manager_error_unknown_action); RUNTIME_GUARD(envoy_reloadable_features_proxy_status_upstream_request_timeout); -RUNTIME_GUARD(envoy_reloadable_features_sanitize_original_path); RUNTIME_GUARD(envoy_reloadable_features_send_header_raw_value); RUNTIME_GUARD(envoy_reloadable_features_service_sanitize_non_utf8_strings); RUNTIME_GUARD(envoy_reloadable_features_skip_dns_lookup_for_proxied_requests); From 00706c87f5a9eea3bb57fe3aac80a3aa596d76cc Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Tue, 24 Oct 2023 16:25:51 +0000 Subject: [PATCH 05/17] mobile: Add a flag to specify the artifact ID when publishing to Maven (#30457) This PR adds a `--artifact_id` flag in the `sonatype_nexus_upload.py` to allow specifying the artifact ID when publishing to Maven Central. Signed-off-by: Fredy Wijaya --- .github/workflows/mobile-release.yml | 2 ++ mobile/ci/sonatype_nexus_upload.py | 21 +++++++++++++-------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/.github/workflows/mobile-release.yml b/.github/workflows/mobile-release.yml index fb7f3648e0fb..1b82ce622053 100644 --- a/.github/workflows/mobile-release.yml +++ b/.github/workflows/mobile-release.yml @@ -119,6 +119,7 @@ jobs: version="0.5.0.$(date '+%Y%m%d')" python mobile/ci/sonatype_nexus_upload.py \ --profile_id=$SONATYPE_PROFILE_ID \ + --artifact_id=envoy \ --version=$version \ --files \ envoy.aar \ @@ -235,6 +236,7 @@ jobs: version="0.5.0.$(date '+%Y%m%d')" python mobile/ci/sonatype_nexus_upload.py \ --profile_id=$SONATYPE_PROFILE_ID \ + --artifact_id=envoy-xds \ --version=$version \ --files \ envoy_xds.aar \ diff --git a/mobile/ci/sonatype_nexus_upload.py b/mobile/ci/sonatype_nexus_upload.py index bd04883de05c..c62644ad3b03 100755 --- a/mobile/ci/sonatype_nexus_upload.py +++ b/mobile/ci/sonatype_nexus_upload.py @@ -21,7 +21,6 @@ _ARTIFACT_HOST_URL = "https://oss.sonatype.org/service/local/staging" _GROUP_ID = "io.envoyproxy.envoymobile" -_ARTIFACT_ID = "envoy" _LOCAL_INSTALL_PATH = os.path.expanduser( "~/.m2/repository/{directory}/envoy".format(directory=_GROUP_ID.replace(".", "/"))) @@ -52,7 +51,7 @@ def _resolve_name(file): return "", extension -def _install_locally(version, files): +def _install_locally(artifact_id, version, files): path = "{}/{}".format(_LOCAL_INSTALL_PATH, version) if os.path.exists(path): @@ -63,7 +62,7 @@ def _install_locally(version, files): for file in files: suffix, file_extension = _resolve_name(file) basename = "{name}-{version}{suffix}.{extension}".format( - name=_ARTIFACT_ID, version=version, suffix=suffix, extension=file_extension) + name=artifact_id, version=version, suffix=suffix, extension=file_extension) shutil.copyfile(file, os.path.join(path, basename)) print("{file_name}\n{sha}\n".format(file_name=file, sha=_sha256(file))) @@ -116,7 +115,7 @@ def _create_staging_repository(profile_id): raise e -def _upload_files(staging_id, version, files, ascs, sha256): +def _upload_files(staging_id, artifact_id, version, files, ascs, sha256): uploaded_file_count = 0 # aggregate all the files for uploading @@ -126,11 +125,11 @@ def _upload_files(staging_id, version, files, ascs, sha256): print("Uploading file {}".format(file)) suffix, file_extension = _resolve_name(file) basename = "{name}-{version}{suffix}.{extension}".format( - name=_ARTIFACT_ID, version=version, suffix=suffix, extension=file_extension) + name=artifact_id, version=version, suffix=suffix, extension=file_extension) artifact_url = os.path.join( _ARTIFACT_HOST_URL, "deployByRepositoryId/{}".format(staging_id), - _GROUP_ID.replace('.', "/"), _ARTIFACT_ID, version, basename) + _GROUP_ID.replace('.', "/"), artifact_id, version, basename) try: with open(file, "rb") as f: @@ -233,6 +232,12 @@ def _build_parser(): curl -u {usr}:{psswrd} -H "Accept: application/json" https://oss.sonatype.org//nexus/service/local/staging/profile_repositories """) + parser.add_argument( + "--artifact_id", + required=True, + help=""" + The artifact ID to be published. + """) parser.add_argument( "--version", default="LOCAL-SNAPSHOT", @@ -285,7 +290,7 @@ def _build_parser(): version = args.version if args.local: - _install_locally(version, args.files) + _install_locally(args.artifact_id, version, args.files) else: staging_id = "" @@ -301,7 +306,7 @@ def _build_parser(): print("Uploading files...") sha256_files = _create_sha256_files(args.files) uploaded_file_count = _upload_files( - staging_id, version, args.files, args.signed_files, sha256_files) + staging_id, args.artifact_id, version, args.files, args.signed_files, sha256_files) if uploaded_file_count > 0: print("Uploading files complete!") print("Closing staging repository...") From ae0e6ce90fdbf32007b741fbad7806ec57a6414b Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Tue, 24 Oct 2023 16:50:18 +0000 Subject: [PATCH 06/17] mobile: Update pom.xml with protobuf-javalite transitive dependency (#30462) mobile: Update pom.xml with a protobuf-javalite transitive dependency This is to allow depending on `io.envoyproxy.envoymobile:envoy-xds` without having to manually add `com.google.protobuf:protobuf-javalite` dependency. Signed-off-by: Fredy Wijaya --- mobile/bazel/pom_template.xml | 3 ++- mobile/library/kotlin/io/envoyproxy/envoymobile/BUILD | 11 +++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/mobile/bazel/pom_template.xml b/mobile/bazel/pom_template.xml index c15b092c5ad4..a480e794e1c0 100644 --- a/mobile/bazel/pom_template.xml +++ b/mobile/bazel/pom_template.xml @@ -5,11 +5,12 @@ 4.0.0 io.envoyproxy.envoymobile - pom_artifact_id + {pom_artifact_id} {pom_version} aar {generated_bzl_deps} + {pom_extra_dependencies} Envoy Mobile diff --git a/mobile/library/kotlin/io/envoyproxy/envoymobile/BUILD b/mobile/library/kotlin/io/envoyproxy/envoymobile/BUILD index 911f6daafcd4..a4404f565ee2 100644 --- a/mobile/library/kotlin/io/envoyproxy/envoymobile/BUILD +++ b/mobile/library/kotlin/io/envoyproxy/envoymobile/BUILD @@ -17,7 +17,8 @@ android_artifacts( }), proguard_rules = "//library:proguard_rules", substitutions = { - "pom_artifact_id": "envoy", + "{pom_artifact_id}": "envoy", + "{pom_extra_dependencies}": "", }, visibility = ["//visibility:public"], ) @@ -33,7 +34,13 @@ android_artifacts( }), proguard_rules = "//library:proguard_rules", substitutions = { - "pom_artifact_id": "envoy-xds", + "{pom_artifact_id}": "envoy-xds", + "{pom_extra_dependencies}": """ + + com.google.protobuf + protobuf-javalite + 3.24.4 + """, }, visibility = ["//visibility:public"], ) From 42934fba65f0821a7f0997a46eb738447555b813 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Conte=20Mac=20Donell?= Date: Tue, 24 Oct 2023 10:43:45 -0700 Subject: [PATCH 07/17] jwt_authn: Allow to extract keys from jwt struct claims (#30377) Added a new field `list_claim_keys` on :ref:`claim_to_headers ` to extract keys from JWT token claims. This field enables the retrieval of keys from custom JWT token claims, such as `{"tenants": {"bitdrift": {}}` (in this case a claim_name of `tenants would extract the key "bitdrift"). Signed-off-by: Martin Conte Mac Donell --- changelogs/current.yaml | 4 +++ .../http/http_filters/jwt_authn_filter.rst | 5 ++- .../filters/http/jwt_authn/authenticator.cc | 34 ++++++++++++++----- .../http/jwt_authn/authenticator_test.cc | 8 +++++ .../filters/http/jwt_authn/test_common.h | 2 ++ 5 files changed, 43 insertions(+), 10 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 6563980fc8a3..8ddf455f3da8 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -36,5 +36,9 @@ removed_config_or_runtime: runtime flag and legacy code path. new_features: +- area: jwt + change: | + The jwt filter can now serialize non-primitive custom claims when maping claims to headers. + These claims will be serialized as JSON and encoded as Base64. deprecated: diff --git a/docs/root/configuration/http/http_filters/jwt_authn_filter.rst b/docs/root/configuration/http/http_filters/jwt_authn_filter.rst index e84a23a31400..f8dd7e3a66cb 100644 --- a/docs/root/configuration/http/http_filters/jwt_authn_filter.rst +++ b/docs/root/configuration/http/http_filters/jwt_authn_filter.rst @@ -274,10 +274,13 @@ The field :ref:`claim_to_headers x-jwt-claim-nested-key: + x-jwt-tenants: diff --git a/source/extensions/filters/http/jwt_authn/authenticator.cc b/source/extensions/filters/http/jwt_authn/authenticator.cc index d302e066f382..cb380b905eb0 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.cc +++ b/source/extensions/filters/http/jwt_authn/authenticator.cc @@ -306,21 +306,37 @@ void AuthenticatorImpl::addJWTClaimToHeader(const std::string& claim_name, const auto status = payload_getter.GetValue(claim_name, claim_value); std::string str_claim_value; if (status == StructUtils::OK) { - if (claim_value->kind_case() == Envoy::ProtobufWkt::Value::kStringValue) { + switch (claim_value->kind_case()) { + case Envoy::ProtobufWkt::Value::kStringValue: str_claim_value = claim_value->string_value(); - } else if (claim_value->kind_case() == Envoy::ProtobufWkt::Value::kNumberValue) { + break; + case Envoy::ProtobufWkt::Value::kNumberValue: str_claim_value = convertClaimDoubleToString(claim_value->number_value()); - } else if (claim_value->kind_case() == Envoy::ProtobufWkt::Value::kBoolValue) { + break; + case Envoy::ProtobufWkt::Value::kBoolValue: str_claim_value = claim_value->bool_value() ? "true" : "false"; - } else { - ENVOY_LOG( - debug, - "--------claim : {} is not a primitive type of int, double, string, or bool -----------", - claim_name); + break; + case Envoy::ProtobufWkt::Value::kStructValue: + ABSL_FALLTHROUGH_INTENDED; + case Envoy::ProtobufWkt::Value::kListValue: { + std::string output; + auto status = claim_value->has_struct_value() + ? ProtobufUtil::MessageToJsonString(claim_value->struct_value(), &output) + : ProtobufUtil::MessageToJsonString(claim_value->list_value(), &output); + if (status.ok()) { + str_claim_value = Envoy::Base64::encode(output.data(), output.size()); + } + break; + } + default: + ENVOY_LOG(debug, "[jwt_auth] claim : {} is of an unknown type '{}'", claim_name, + claim_value->kind_case()); + break; } + if (!str_claim_value.empty()) { headers_->addCopy(Http::LowerCaseString(header_name), str_claim_value); - ENVOY_LOG(debug, "--------claim : {} with value : {} is added to the header : {} -----------", + ENVOY_LOG(debug, "[jwt_auth] claim : {} with value : {} is added to the header : {}", claim_name, str_claim_value, header_name); } } diff --git a/test/extensions/filters/http/jwt_authn/authenticator_test.cc b/test/extensions/filters/http/jwt_authn/authenticator_test.cc index 4ebe0f9a3feb..70b123c3807f 100644 --- a/test/extensions/filters/http/jwt_authn/authenticator_test.cc +++ b/test/extensions/filters/http/jwt_authn/authenticator_test.cc @@ -1,6 +1,7 @@ #include "envoy/config/core/v3/http_uri.pb.h" #include "envoy/extensions/filters/http/jwt_authn/v3/config.pb.h" +#include "source/common/common/base64.h" #include "source/common/http/message_impl.h" #include "source/common/protobuf/utility.h" #include "source/extensions/filters/http/common/jwks_fetcher.h" @@ -153,6 +154,13 @@ TEST_F(AuthenticatorTest, TestClaimToHeader) { EXPECT_EQ(headers.get_("x-jwt-claim-nested"), "value1"); EXPECT_EQ(headers.get_("x-jwt-bool-claim"), "true"); EXPECT_EQ(headers.get_("x-jwt-int-claim"), "9999"); + + // This check verifies whether the claim with non-primitive type are + // successfully serialized and added to headers. + std::string expected_json = "[\"str1\",\"str2\"]"; + + ASSERT_EQ(headers.get_("x-jwt-claim-object-key"), + Envoy::Base64::encode(expected_json.data(), expected_json.size())); } // This test verifies when wrong claim is passed in claim_to_headers diff --git a/test/extensions/filters/http/jwt_authn/test_common.h b/test/extensions/filters/http/jwt_authn/test_common.h index 95199667d03d..eb696a7a2509 100644 --- a/test/extensions/filters/http/jwt_authn/test_common.h +++ b/test/extensions/filters/http/jwt_authn/test_common.h @@ -97,6 +97,8 @@ const char ExampleConfig[] = R"( claim_name: "nested.nested-2.key-3" - header_name: "x-jwt-int-claim" claim_name: "nested.nested-2.key-4" + - header_name: "x-jwt-claim-object-key" + claim_name: "nested.nested-2.key-5" rules: - match: path: "/" From 6750243cc6a6abba1fca4a545339979f8641a262 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Tue, 24 Oct 2023 13:57:23 -0400 Subject: [PATCH 08/17] test: Move MockedUpdatedClusterManagerImpl to cluster_manager_impl_test (#30464) The class is only used within cluster_manager_impl_test and is very specific in its behavior, so I think it makes more sense to locate it in the same file as the test that uses it, instead of keeping it in test_cluster_manager.h. Signed-off-by: Ali Beyad --- .../upstream/cluster_manager_impl_test.cc | 51 +++++++++++++++++++ test/common/upstream/test_cluster_manager.h | 51 ------------------- 2 files changed, 51 insertions(+), 51 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index ff864b2a6148..26ca8d260fe4 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -87,6 +87,57 @@ void verifyCaresDnsConfigAndUnpack( typed_dns_resolver_config.typed_config().UnpackTo(&cares); } +// Helper to intercept calls to postThreadLocalClusterUpdate. +class MockLocalClusterUpdate { +public: + MOCK_METHOD(void, post, + (uint32_t priority, const HostVector& hosts_added, const HostVector& hosts_removed)); +}; + +class MockLocalHostsRemoved { +public: + MOCK_METHOD(void, post, (const HostVector&)); +}; + +// Override postThreadLocalClusterUpdate so we can test that merged updates calls +// it with the right values at the right times. +class MockedUpdatedClusterManagerImpl : public TestClusterManagerImpl { +public: + using TestClusterManagerImpl::TestClusterManagerImpl; + + MockedUpdatedClusterManagerImpl(const envoy::config::bootstrap::v3::Bootstrap& bootstrap, + ClusterManagerFactory& factory, Stats::Store& stats, + ThreadLocal::Instance& tls, Runtime::Loader& runtime, + const LocalInfo::LocalInfo& local_info, + AccessLog::AccessLogManager& log_manager, + Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, + ProtobufMessage::ValidationContext& validation_context, + Api::Api& api, MockLocalClusterUpdate& local_cluster_update, + MockLocalHostsRemoved& local_hosts_removed, + Http::Context& http_context, Grpc::Context& grpc_context, + Router::Context& router_context, Server::Instance& server) + : TestClusterManagerImpl(bootstrap, factory, stats, tls, runtime, local_info, log_manager, + main_thread_dispatcher, admin, validation_context, api, http_context, + grpc_context, router_context, server), + local_cluster_update_(local_cluster_update), local_hosts_removed_(local_hosts_removed) {} + +protected: + void postThreadLocalClusterUpdate(ClusterManagerCluster&, + ThreadLocalClusterUpdateParams&& params) override { + for (const auto& per_priority : params.per_priority_update_params_) { + local_cluster_update_.post(per_priority.priority_, per_priority.hosts_added_, + per_priority.hosts_removed_); + } + } + + void postThreadLocalRemoveHosts(const Cluster&, const HostVector& hosts_removed) override { + local_hosts_removed_.post(hosts_removed); + } + + MockLocalClusterUpdate& local_cluster_update_; + MockLocalHostsRemoved& local_hosts_removed_; +}; + // A gRPC MuxFactory that returns a MockGrpcMux instance when trying to instantiate a mux for the // `envoy.config_mux.grpc_mux_factory` type. This enables testing call expectations on the ADS gRPC // mux. diff --git a/test/common/upstream/test_cluster_manager.h b/test/common/upstream/test_cluster_manager.h index 1cb1ad9e6adc..edd456d57ab2 100644 --- a/test/common/upstream/test_cluster_manager.h +++ b/test/common/upstream/test_cluster_manager.h @@ -159,18 +159,6 @@ class TestClusterManagerFactory : public ClusterManagerFactory { Server::MockOptions& options_ = server_context_.options_; }; -// Helper to intercept calls to postThreadLocalClusterUpdate. -class MockLocalClusterUpdate { -public: - MOCK_METHOD(void, post, - (uint32_t priority, const HostVector& hosts_added, const HostVector& hosts_removed)); -}; - -class MockLocalHostsRemoved { -public: - MOCK_METHOD(void, post, (const HostVector&)); -}; - // A test version of ClusterManagerImpl that provides a way to get a non-const handle to the // clusters, which is necessary in order to call updateHosts on the priority set. class TestClusterManagerImpl : public ClusterManagerImpl { @@ -231,44 +219,5 @@ class TestClusterManagerImpl : public ClusterManagerImpl { grpc_context, router_context, server) {} }; -// Override postThreadLocalClusterUpdate so we can test that merged updates calls -// it with the right values at the right times. -class MockedUpdatedClusterManagerImpl : public TestClusterManagerImpl { -public: - using TestClusterManagerImpl::TestClusterManagerImpl; - - MockedUpdatedClusterManagerImpl(const envoy::config::bootstrap::v3::Bootstrap& bootstrap, - ClusterManagerFactory& factory, Stats::Store& stats, - ThreadLocal::Instance& tls, Runtime::Loader& runtime, - const LocalInfo::LocalInfo& local_info, - AccessLog::AccessLogManager& log_manager, - Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, - ProtobufMessage::ValidationContext& validation_context, - Api::Api& api, MockLocalClusterUpdate& local_cluster_update, - MockLocalHostsRemoved& local_hosts_removed, - Http::Context& http_context, Grpc::Context& grpc_context, - Router::Context& router_context, Server::Instance& server) - : TestClusterManagerImpl(bootstrap, factory, stats, tls, runtime, local_info, log_manager, - main_thread_dispatcher, admin, validation_context, api, http_context, - grpc_context, router_context, server), - local_cluster_update_(local_cluster_update), local_hosts_removed_(local_hosts_removed) {} - -protected: - void postThreadLocalClusterUpdate(ClusterManagerCluster&, - ThreadLocalClusterUpdateParams&& params) override { - for (const auto& per_priority : params.per_priority_update_params_) { - local_cluster_update_.post(per_priority.priority_, per_priority.hosts_added_, - per_priority.hosts_removed_); - } - } - - void postThreadLocalRemoveHosts(const Cluster&, const HostVector& hosts_removed) override { - local_hosts_removed_.post(hosts_removed); - } - - MockLocalClusterUpdate& local_cluster_update_; - MockLocalHostsRemoved& local_hosts_removed_; -}; - } // namespace Upstream } // namespace Envoy From 6776a435f812513e7024ec0a1a5f3235aec5af94 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Tue, 24 Oct 2023 12:23:17 -0700 Subject: [PATCH 09/17] Make config_validation not have hot_restart panics (#30465) Signed-off-by: Raven Black --- source/server/config_validation/BUILD | 1 + source/server/config_validation/server.h | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source/server/config_validation/BUILD b/source/server/config_validation/BUILD index f5e8bd30b1d9..6d98c6ac7544 100644 --- a/source/server/config_validation/BUILD +++ b/source/server/config_validation/BUILD @@ -100,6 +100,7 @@ envoy_cc_library( "//source/common/thread_local:thread_local_lib", "//source/common/version:version_lib", "//source/server:configuration_lib", + "//source/server:hot_restart_nop_lib", "//source/server:server_lib", "//source/server:utils_lib", "//source/server/admin:admin_lib", diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index f4a982d1602c..67a670423d60 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -28,6 +28,7 @@ #include "source/server/config_validation/api.h" #include "source/server/config_validation/cluster_manager.h" #include "source/server/config_validation/dns.h" +#include "source/server/hot_restart_nop_impl.h" #include "source/server/server.h" #include "absl/types/optional.h" @@ -89,7 +90,7 @@ class ValidationInstance final : Logger::Loggable, DrainManager& drainManager() override { return *drain_manager_; } AccessLog::AccessLogManager& accessLogManager() override { return access_log_manager_; } void failHealthcheck(bool) override {} - HotRestart& hotRestart() override { PANIC("not implemented"); } + HotRestart& hotRestart() override { return nop_hot_restart_; } Init::Manager& initManager() override { return init_manager_; } ServerLifecycleNotifier& lifecycleNotifier() override { return *this; } ListenerManager& listenerManager() override { return *listener_manager_; } @@ -190,6 +191,7 @@ class ValidationInstance final : Logger::Loggable, Quic::QuicStatNames quic_stat_names_; Filter::TcpListenerFilterConfigProviderManagerImpl tcp_listener_config_provider_manager_; Server::DrainManagerPtr drain_manager_; + HotRestartNopImpl nop_hot_restart_; }; } // namespace Server From 705c7c8610d100301f1cff6c6b58915d8a8a5cf9 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 24 Oct 2023 15:24:06 -0400 Subject: [PATCH 10/17] tools: fix pr notifier bug (#30433) Signed-off-by: Alyssa Wilk --- tools/deprecate_version/deprecate_version.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tools/deprecate_version/deprecate_version.py b/tools/deprecate_version/deprecate_version.py index 02ce21afe23c..5ccb22ee7571 100644 --- a/tools/deprecate_version/deprecate_version.py +++ b/tools/deprecate_version/deprecate_version.py @@ -115,16 +115,23 @@ def create_issues(access_token, runtime_and_pr): if get_confirmation(): print('Creating issues...') for title, body, login in issues: + issue_created = False try: - repo.create_issue(title, body=body, assignees=[login], labels=labels) + # for setec backports, we may not find a user, which would make + # create_issue crash. + if login: + repo.create_issue(title, body=body, assignees=[login], labels=labels) + issue_created = True except github.GithubException as e: + print(( + 'unable to assign issue %s to %s. Add them to the Envoy proxy org' + 'and assign it their way.') % (title, login)) + + if not issue_created: try: if login: body += '\ncc @' + login repo.create_issue(title, body=body, labels=labels) - print(( - 'unable to assign issue %s to %s. Add them to the Envoy proxy org' - 'and assign it their way.') % (title, login)) except github.GithubException as e: print('GithubException while creating issue.') raise From 48bf974bf509ae481acb466489e462d713b59227 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 24 Oct 2023 13:13:17 -0700 Subject: [PATCH 11/17] grpc: remove envoy.reloadable_features.service_sanitize_non_utf8_strings (#30469) Fixes #30425 Signed-off-by: Greg Greenway --- changelogs/current.yaml | 4 ++++ source/common/protobuf/yaml_utility.cc | 17 +---------------- source/common/runtime/runtime_features.cc | 1 - test/common/protobuf/utility_test.cc | 13 ------------- 4 files changed, 5 insertions(+), 30 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 8ddf455f3da8..651d3215ecb5 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -34,6 +34,10 @@ removed_config_or_runtime: change: | Removed the deprecated ``envoy.reloadable_features.validate_detailed_override_host_statuses`` runtime flag and legacy code path. +- area: grpc + change: | + Removed the deprecated ``envoy.reloadable_features.service_sanitize_non_utf8_strings`` + runtime flag and legacy code path. new_features: - area: jwt diff --git a/source/common/protobuf/yaml_utility.cc b/source/common/protobuf/yaml_utility.cc index 7aa707864bfc..92d82da896b7 100644 --- a/source/common/protobuf/yaml_utility.cc +++ b/source/common/protobuf/yaml_utility.cc @@ -347,24 +347,9 @@ std::string utf8CoerceToStructurallyValid(absl::string_view str, const char repl } // namespace std::string MessageUtil::sanitizeUtf8String(absl::string_view input) { - if (!Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.service_sanitize_non_utf8_strings")) { - return std::string(input); - } - - // This returns the original string if it was already valid, and returns a pointer to - // `result.data()` if it needed to coerce. The coerced string is always - // the same length as the source string. - // - // Initializing `result` to `input` ensures that `result` is correctly sized to receive the - // modified string, or in the case where no modification is needed it already contains the correct - // value, so `result` can be returned in both cases. - // // The choice of '!' is somewhat arbitrary, but we wanted to avoid any character that has // special semantic meaning in URLs or similar. - std::string result = utf8CoerceToStructurallyValid(input, '!'); - - return result; + return utf8CoerceToStructurallyValid(input, '!'); } } // namespace Envoy diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index dc538c51dc77..cf0511570018 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -73,7 +73,6 @@ RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout); RUNTIME_GUARD(envoy_reloadable_features_overload_manager_error_unknown_action); RUNTIME_GUARD(envoy_reloadable_features_proxy_status_upstream_request_timeout); RUNTIME_GUARD(envoy_reloadable_features_send_header_raw_value); -RUNTIME_GUARD(envoy_reloadable_features_service_sanitize_non_utf8_strings); RUNTIME_GUARD(envoy_reloadable_features_skip_dns_lookup_for_proxied_requests); RUNTIME_GUARD(envoy_reloadable_features_ssl_transport_failure_reason_format); RUNTIME_GUARD(envoy_reloadable_features_stateful_session_encode_ttl_in_cookie); diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 0ed33236b0e7..7b51f2b0e2c7 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1195,19 +1195,6 @@ TEST_F(ProtobufUtilityTest, SanitizeUTF8) { EXPECT_EQ(absl::string_view("valid_prefix!!valid_middle!valid_suffix"), sanitized); EXPECT_EQ(sanitized.length(), original.length()); } - - { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues( - {{"envoy.reloadable_features.service_sanitize_non_utf8_strings", "false"}}); - std::string original("valid_prefix"); - original.append(1, char(0xc3)); - original.append(1, char(0xc7)); - original.append("valid_suffix"); - - std::string non_sanitized = MessageUtil::sanitizeUtf8String(original); - EXPECT_EQ(non_sanitized, original); - } } TEST_F(ProtobufUtilityTest, KeyValueStruct) { From 1f0192d210ebdc6f70054111e0a6b7815c58a02b Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Tue, 24 Oct 2023 17:12:18 -0400 Subject: [PATCH 12/17] test: Enable XdsTest to run without the test HTTP server (#30474) This required fixing a couple issues in XdsTestServer: (1) Initialize Libevent before creating the Dispatcher. If we don't do this, and we start the XdsTestServer without starting the Http2TestServer, the LibeventScheduler fails the Libevent::Global::initialized() assert. (2) Add a Thread::SkipAsserts before calling the YAML to Bootstrap parsing. The YAML utilities make some non-relevant asserts on being on the main thread, which aren't applicable when calling the YAML utilities from JNI and tests. To remove the need for the Http2TestServer, we remove the endpoints from the dynamic cluster returned in the test xDS response. This allows us to test the essential xDS behavior without depending on an endpoint for cluster initialization. Signed-off-by: Ali Beyad --- mobile/test/common/integration/BUILD | 1 + mobile/test/common/integration/xds_test_server.cc | 13 +++++++++---- mobile/test/common/jni/test_jni_interface.cc | 2 ++ mobile/test/kotlin/integration/XdsTest.kt | 13 +------------ 4 files changed, 13 insertions(+), 16 deletions(-) diff --git a/mobile/test/common/integration/BUILD b/mobile/test/common/integration/BUILD index 052a73a748e9..6c2905013611 100644 --- a/mobile/test/common/integration/BUILD +++ b/mobile/test/common/integration/BUILD @@ -188,6 +188,7 @@ envoy_cc_test_library( repository = "@envoy", deps = [ ":base_client_integration_test_lib", + "@envoy//source/common/event:libevent_lib", "@envoy//source/common/grpc:google_grpc_creds_lib", "@envoy//source/exe:process_wide_lib", "@envoy//test/integration:autonomous_upstream_lib", diff --git a/mobile/test/common/integration/xds_test_server.cc b/mobile/test/common/integration/xds_test_server.cc index 35f7887edffe..1c61391af0b3 100644 --- a/mobile/test/common/integration/xds_test_server.cc +++ b/mobile/test/common/integration/xds_test_server.cc @@ -2,6 +2,7 @@ #include +#include "source/common/event/libevent.h" #include "source/common/grpc/google_grpc_creds_impl.h" #include "source/extensions/config_subscription/grpc/grpc_collection_subscription_factory.h" #include "source/extensions/config_subscription/grpc/grpc_mux_impl.h" @@ -17,10 +18,14 @@ namespace Envoy { XdsTestServer::XdsTestServer() : api_(Api::createApiForTest(stats_store_, time_system_)), version_(Network::Address::IpVersion::v4), - mock_buffer_factory_(new NiceMock), - dispatcher_(api_->allocateDispatcher("test_thread", - Buffer::WatermarkFactoryPtr{mock_buffer_factory_})), - upstream_config_(time_system_) { + mock_buffer_factory_(new NiceMock), upstream_config_(time_system_) { + if (!Envoy::Event::Libevent::Global::initialized()) { + // Required by the Dispatcher. + Envoy::Event::Libevent::Global::initialize(); + } + dispatcher_ = + api_->allocateDispatcher("test_thread", Buffer::WatermarkFactoryPtr{mock_buffer_factory_}); + ON_CALL(*mock_buffer_factory_, createBuffer_(_, _, _)) .WillByDefault(Invoke([](std::function below_low, std::function above_high, std::function above_overflow) -> Buffer::Instance* { diff --git a/mobile/test/common/jni/test_jni_interface.cc b/mobile/test/common/jni/test_jni_interface.cc index 61dc7801bd2e..0ffd910c78a2 100644 --- a/mobile/test/common/jni/test_jni_interface.cc +++ b/mobile/test/common/jni/test_jni_interface.cc @@ -73,6 +73,8 @@ Java_io_envoyproxy_envoymobile_engine_testing_TestJni_nativeSendDiscoveryRespons jstring yaml) { jni_log("[XTS]", "sending DiscoveryResponse from the xDS server"); const char* yaml_chars = env->GetStringUTFChars(yaml, /* isCopy= */ nullptr); + // The yaml utilities have non-relevant thread asserts. + Envoy::Thread::SkipAsserts skip; envoy::service::discovery::v3::DiscoveryResponse response; Envoy::TestUtility::loadFromYaml(yaml_chars, response); sendDiscoveryResponse(response); diff --git a/mobile/test/kotlin/integration/XdsTest.kt b/mobile/test/kotlin/integration/XdsTest.kt index 6994a5f15b5e..ebc3a11ebfd8 100644 --- a/mobile/test/kotlin/integration/XdsTest.kt +++ b/mobile/test/kotlin/integration/XdsTest.kt @@ -28,7 +28,6 @@ class XdsTest { @Before fun setUp() { - TestJni.startHttp2TestServer() TestJni.initXdsTestServer() val latch = CountDownLatch(1) engine = @@ -50,7 +49,6 @@ class XdsTest { @After fun tearDown() { engine.terminate() - TestJni.shutdownTestServer() TestJni.shutdownXdsTestServer() } @@ -66,15 +64,6 @@ class XdsTest { name: my_cluster type: STATIC connect_timeout: 5s - load_assignment: - cluster_name: xds_cluster - endpoints: - - lb_endpoints: - - endpoint: - address: - socket_address: - address: ${TestJni.getServerHost()} - port_value: ${TestJni.getServerPort()} typed_extension_protocol_options: envoy.extensions.upstreams.http.v3.HttpProtocolOptions: "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions @@ -86,7 +75,7 @@ class XdsTest { """ .trimIndent() TestJni.sendDiscoveryResponse(cdsResponse) - // There are now 3 clusters: base, base_cluster, and xds_cluster. + // There are now 3 clusters: base, base_cluster, and my_cluster. engine.waitForStatGe("cluster_manager.cluster_added", 3) } } From 2489e249d2a5377fafc57f7ea82d348e542f7452 Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Tue, 24 Oct 2023 17:36:37 -0400 Subject: [PATCH 13/17] Deprecation: Remove allow compact maglev runtime guard. (#30431) Remove allow compact maglev runtime guard. Signed-off-by: Kevin Baichoo --- changelogs/current.yaml | 5 ++- source/common/runtime/runtime_features.cc | 1 - .../load_balancing_policies/maglev/BUILD | 18 ++++++++ .../maglev/maglev_lb.cc | 7 +++- .../load_balancing_policies/maglev/BUILD | 21 ++++++++++ .../maglev/maglev_lb_test.cc | 41 ++++++++----------- 6 files changed, 64 insertions(+), 29 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 651d3215ecb5..a4c947202f6c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -21,15 +21,16 @@ bug_fixes: removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` - - area: http change: | removed ``envoy.reloadable_features.correctly_validate_alpn`` and legacy code paths. +- area: maglev + change: | + Removed ``envoy.reloadable_features.allow_compact_maglev`` and legacy code paths. - area: router change: | Removed the deprecated ``envoy.reloadable_features.prohibit_route_refresh_after_response_headers_sent`` runtime flag and legacy code path. - - area: upstream change: | Removed the deprecated ``envoy.reloadable_features.validate_detailed_override_host_statuses`` diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index cf0511570018..b55270ee36af 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -30,7 +30,6 @@ // ASAP by filing a bug on github. Overriding non-buggy code is strongly discouraged to avoid the // problem of the bugs being found after the old code path has been removed. RUNTIME_GUARD(envoy_reloadable_features_allow_absolute_url_with_mixed_scheme); -RUNTIME_GUARD(envoy_reloadable_features_allow_compact_maglev); RUNTIME_GUARD(envoy_reloadable_features_append_xfh_idempotent); RUNTIME_GUARD(envoy_reloadable_features_check_mep_on_first_eject); RUNTIME_GUARD(envoy_reloadable_features_conn_pool_delete_when_idle); diff --git a/source/extensions/load_balancing_policies/maglev/BUILD b/source/extensions/load_balancing_policies/maglev/BUILD index 0c2dfb181fbe..537b9c1a7765 100644 --- a/source/extensions/load_balancing_policies/maglev/BUILD +++ b/source/extensions/load_balancing_policies/maglev/BUILD @@ -23,6 +23,24 @@ envoy_cc_library( ], ) +# This library target is for testing especially x86 coverage +# which will use the compact representation unless we force the +# original implementation. +envoy_cc_library( + name = "maglev_lb_force_original_impl_lib", + srcs = ["maglev_lb.cc"], + hdrs = ["maglev_lb.h"], + copts = ["-DMAGLEV_LB_FORCE_ORIGINAL_IMPL"], + deps = [ + "//envoy/upstream:load_balancer_interface", + "//source/common/common:bit_array_lib", + "//source/common/runtime:runtime_features_lib", + "//source/common/upstream:thread_aware_lb_lib", + "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/load_balancing_policies/maglev/v3:pkg_cc_proto", + ], +) + envoy_cc_extension( name = "config", srcs = ["config.cc"], diff --git a/source/extensions/load_balancing_policies/maglev/maglev_lb.cc b/source/extensions/load_balancing_policies/maglev/maglev_lb.cc index a00002ba95a4..e4e98b890e5e 100644 --- a/source/extensions/load_balancing_policies/maglev/maglev_lb.cc +++ b/source/extensions/load_balancing_policies/maglev/maglev_lb.cc @@ -13,6 +13,10 @@ bool shouldUseCompactTable(size_t num_hosts, uint64_t table_size) { return false; } +#ifdef MAGLEV_LB_FORCE_ORIGINAL_IMPL + return false; +#endif + if (num_hosts > MaglevTable::MaxNumberOfHostsForCompactMaglev) { return false; } @@ -37,8 +41,7 @@ class MaglevFactory : private Logger::Loggable { bool use_hostname_for_hashing, MaglevLoadBalancerStats& stats) { MaglevTableSharedPtr maglev_table; - if (shouldUseCompactTable(normalized_host_weights.size(), table_size) && - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.allow_compact_maglev")) { + if (shouldUseCompactTable(normalized_host_weights.size(), table_size)) { maglev_table = std::make_shared(normalized_host_weights, max_normalized_weight, table_size, use_hostname_for_hashing, stats); diff --git a/test/extensions/load_balancing_policies/maglev/BUILD b/test/extensions/load_balancing_policies/maglev/BUILD index 87d543682990..c8618de817f8 100644 --- a/test/extensions/load_balancing_policies/maglev/BUILD +++ b/test/extensions/load_balancing_policies/maglev/BUILD @@ -30,6 +30,27 @@ envoy_extension_cc_test( ], ) +# Runs the same test suite as :maglev_lb_test with the forced original implementation +# to ensure coverage on x86. +envoy_extension_cc_test( + name = "maglev_lb_force_original_impl_test", + srcs = ["maglev_lb_test.cc"], + extension_names = ["envoy.load_balancing_policies.maglev"], + deps = [ + "//source/extensions/load_balancing_policies/maglev:maglev_lb_force_original_impl_lib", + "//test/common/upstream:utility_lib", + "//test/mocks:common_lib", + "//test/mocks/upstream:cluster_info_mocks", + "//test/mocks/upstream:host_mocks", + "//test/mocks/upstream:host_set_mocks", + "//test/mocks/upstream:load_balancer_context_mock", + "//test/mocks/upstream:priority_set_mocks", + "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", + "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", + ], +) + envoy_extension_cc_test( name = "config_test", srcs = ["config_test.cc"], diff --git a/test/extensions/load_balancing_policies/maglev/maglev_lb_test.cc b/test/extensions/load_balancing_policies/maglev/maglev_lb_test.cc index e0cf7e3f06c8..e36cd294c016 100644 --- a/test/extensions/load_balancing_policies/maglev/maglev_lb_test.cc +++ b/test/extensions/load_balancing_policies/maglev/maglev_lb_test.cc @@ -45,14 +45,10 @@ class TestLoadBalancerContext : public LoadBalancerContextBase { // Note: ThreadAwareLoadBalancer base is heavily tested by RingHashLoadBalancerTest. Only basic // functionality is covered here. -class MaglevLoadBalancerTest : public Event::TestUsingSimulatedTime, - public testing::TestWithParam { +class MaglevLoadBalancerTest : public Event::TestUsingSimulatedTime, public testing::Test { public: MaglevLoadBalancerTest() - : stat_names_(stats_store_.symbolTable()), stats_(stat_names_, *stats_store_.rootScope()) { - scoped_runtime_.mergeValues( - {{"envoy.reloadable_features.allow_compact_maglev", GetParam() ? "true" : "false"}}); - } + : stat_names_(stats_store_.symbolTable()), stats_(stat_names_, *stats_store_.rootScope()) {} void createLb() { lb_ = std::make_unique( @@ -91,13 +87,10 @@ class MaglevLoadBalancerTest : public Event::TestUsingSimulatedTime, NiceMock runtime_; NiceMock random_; std::unique_ptr lb_; - TestScopedRuntime scoped_runtime_; }; -INSTANTIATE_TEST_SUITE_P(MaglevTests, MaglevLoadBalancerTest, ::testing::Bool()); - // Works correctly without any hosts. -TEST_P(MaglevLoadBalancerTest, NoHost) { +TEST_F(MaglevLoadBalancerTest, NoHost) { init(7); EXPECT_EQ(nullptr, lb_->factory()->create(lb_params_)->chooseHost(nullptr)); }; @@ -106,7 +99,7 @@ TEST_P(MaglevLoadBalancerTest, NoHost) { // cluster, the operation does not immediately reach the worker thread. There may be cases where the // thread aware load balancer is destructed, but the load balancer factory is still used in the // worker thread. -TEST_P(MaglevLoadBalancerTest, LbDestructedBeforeFactory) { +TEST_F(MaglevLoadBalancerTest, LbDestructedBeforeFactory) { init(7); auto factory = lb_->factory(); @@ -116,13 +109,13 @@ TEST_P(MaglevLoadBalancerTest, LbDestructedBeforeFactory) { } // Throws an exception if table size is not a prime number. -TEST_P(MaglevLoadBalancerTest, NoPrimeNumber) { +TEST_F(MaglevLoadBalancerTest, NoPrimeNumber) { EXPECT_THROW_WITH_MESSAGE(init(8), EnvoyException, "The table size of maglev must be prime number"); }; // Check it has default table size if config is null or table size has invalid value. -TEST_P(MaglevLoadBalancerTest, DefaultMaglevTableSize) { +TEST_F(MaglevLoadBalancerTest, DefaultMaglevTableSize) { const uint64_t defaultValue = MaglevTable::DefaultTableSize; config_ = envoy::config::cluster::v3::Cluster::MaglevLbConfig(); @@ -135,7 +128,7 @@ TEST_P(MaglevLoadBalancerTest, DefaultMaglevTableSize) { }; // Basic sanity tests. -TEST_P(MaglevLoadBalancerTest, Basic) { +TEST_F(MaglevLoadBalancerTest, Basic) { host_set_.hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:90", simTime()), makeTestHost(info_, "tcp://127.0.0.1:91", simTime()), makeTestHost(info_, "tcp://127.0.0.1:92", simTime()), @@ -168,7 +161,7 @@ TEST_P(MaglevLoadBalancerTest, Basic) { } // Basic with hostname. -TEST_P(MaglevLoadBalancerTest, BasicWithHostName) { +TEST_F(MaglevLoadBalancerTest, BasicWithHostName) { host_set_.hosts_ = {makeTestHost(info_, "90", "tcp://127.0.0.1:90", simTime()), makeTestHost(info_, "91", "tcp://127.0.0.1:91", simTime()), makeTestHost(info_, "92", "tcp://127.0.0.1:92", simTime()), @@ -203,7 +196,7 @@ TEST_P(MaglevLoadBalancerTest, BasicWithHostName) { } // Basic with metadata hash_key. -TEST_P(MaglevLoadBalancerTest, BasicWithMetadataHashKey) { +TEST_F(MaglevLoadBalancerTest, BasicWithMetadataHashKey) { host_set_.hosts_ = {makeTestHostWithHashKey(info_, "90", "tcp://127.0.0.1:90", simTime()), makeTestHostWithHashKey(info_, "91", "tcp://127.0.0.1:91", simTime()), makeTestHostWithHashKey(info_, "92", "tcp://127.0.0.1:92", simTime()), @@ -238,7 +231,7 @@ TEST_P(MaglevLoadBalancerTest, BasicWithMetadataHashKey) { } // Same ring as the Basic test, but exercise retry host predicate behavior. -TEST_P(MaglevLoadBalancerTest, BasicWithRetryHostPredicate) { +TEST_F(MaglevLoadBalancerTest, BasicWithRetryHostPredicate) { host_set_.hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:90", simTime()), makeTestHost(info_, "tcp://127.0.0.1:91", simTime()), makeTestHost(info_, "tcp://127.0.0.1:92", simTime()), @@ -286,7 +279,7 @@ TEST_P(MaglevLoadBalancerTest, BasicWithRetryHostPredicate) { } // Basic stability test. -TEST_P(MaglevLoadBalancerTest, BasicStability) { +TEST_F(MaglevLoadBalancerTest, BasicStability) { host_set_.hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:90", simTime()), makeTestHost(info_, "tcp://127.0.0.1:91", simTime()), makeTestHost(info_, "tcp://127.0.0.1:92", simTime()), @@ -331,7 +324,7 @@ TEST_P(MaglevLoadBalancerTest, BasicStability) { } // Weighted sanity test. -TEST_P(MaglevLoadBalancerTest, Weighted) { +TEST_F(MaglevLoadBalancerTest, Weighted) { host_set_.hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:90", simTime(), 1), makeTestHost(info_, "tcp://127.0.0.1:91", simTime(), 2)}; host_set_.healthy_hosts_ = host_set_.hosts_; @@ -369,7 +362,7 @@ TEST_P(MaglevLoadBalancerTest, Weighted) { // Locality weighted sanity test when localities have the same weights. Host weights for hosts in // different localities shouldn't matter. -TEST_P(MaglevLoadBalancerTest, LocalityWeightedSameLocalityWeights) { +TEST_F(MaglevLoadBalancerTest, LocalityWeightedSameLocalityWeights) { envoy::config::core::v3::Locality zone_a; zone_a.set_zone("A"); envoy::config::core::v3::Locality zone_b; @@ -417,7 +410,7 @@ TEST_P(MaglevLoadBalancerTest, LocalityWeightedSameLocalityWeights) { // Locality weighted sanity test when localities have different weights. Host weights for hosts in // different localities shouldn't matter. -TEST_P(MaglevLoadBalancerTest, LocalityWeightedDifferentLocalityWeights) { +TEST_F(MaglevLoadBalancerTest, LocalityWeightedDifferentLocalityWeights) { envoy::config::core::v3::Locality zone_a; zone_a.set_zone("A"); envoy::config::core::v3::Locality zone_b; @@ -467,7 +460,7 @@ TEST_P(MaglevLoadBalancerTest, LocalityWeightedDifferentLocalityWeights) { } // Locality weighted with all localities zero weighted. -TEST_P(MaglevLoadBalancerTest, LocalityWeightedAllZeroLocalityWeights) { +TEST_F(MaglevLoadBalancerTest, LocalityWeightedAllZeroLocalityWeights) { host_set_.hosts_ = {makeTestHost(info_, "tcp://127.0.0.1:90", simTime(), 1)}; host_set_.healthy_hosts_ = host_set_.hosts_; host_set_.hosts_per_locality_ = makeHostsPerLocality({{host_set_.hosts_[0]}}); @@ -483,7 +476,7 @@ TEST_P(MaglevLoadBalancerTest, LocalityWeightedAllZeroLocalityWeights) { // Validate that when we are in global panic and have localities, we get sane // results (fall back to non-healthy hosts). -TEST_P(MaglevLoadBalancerTest, LocalityWeightedGlobalPanic) { +TEST_F(MaglevLoadBalancerTest, LocalityWeightedGlobalPanic) { envoy::config::core::v3::Locality zone_a; zone_a.set_zone("A"); envoy::config::core::v3::Locality zone_b; @@ -531,7 +524,7 @@ TEST_P(MaglevLoadBalancerTest, LocalityWeightedGlobalPanic) { // Given extremely lopsided locality weights, and a table that isn't large enough to fit all hosts, // expect that the least-weighted hosts appear once, and the most-weighted host fills the remainder. -TEST_P(MaglevLoadBalancerTest, LocalityWeightedLopsided) { +TEST_F(MaglevLoadBalancerTest, LocalityWeightedLopsided) { envoy::config::core::v3::Locality zone_a; zone_a.set_zone("A"); envoy::config::core::v3::Locality zone_b; From a3b14fc1a06acdcb8b642dfbdf4b24a56181050e Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Tue, 24 Oct 2023 23:33:23 +0000 Subject: [PATCH 14/17] mobile: Fix a bug where ASSERT is elided on a release build (#30476) This fixes a bug in the JNI code where a call to `ASSERT(some_func())` gets elided causing the code to do nothing. Signed-off-by: Fredy Wijaya --- mobile/library/cc/engine_builder.cc | 2 +- mobile/library/common/jni/jni_utility.cc | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/mobile/library/cc/engine_builder.cc b/mobile/library/cc/engine_builder.cc index c410f3951c09..c469f5daf0ec 100644 --- a/mobile/library/cc/engine_builder.cc +++ b/mobile/library/cc/engine_builder.cc @@ -877,10 +877,10 @@ std::unique_ptr EngineBuilder::generate node->mutable_locality()->set_zone(node_locality_->zone); node->mutable_locality()->set_sub_zone(node_locality_->sub_zone); } - ProtobufWkt::Struct& metadata = *node->mutable_metadata(); if (node_metadata_.has_value()) { *node->mutable_metadata() = *node_metadata_; } + ProtobufWkt::Struct& metadata = *node->mutable_metadata(); (*metadata.mutable_fields())["app_id"].set_string_value(app_id_); (*metadata.mutable_fields())["app_version"].set_string_value(app_version_); (*metadata.mutable_fields())["device_os"].set_string_value(device_os_); diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index fae87ed70aff..dc088add207f 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -388,7 +388,8 @@ std::vector javaObjectArrayToMatcherData(JNIEnv* env, jobjectArray void javaByteArrayToProto(JNIEnv* env, jbyteArray source, Envoy::Protobuf::MessageLite* dest) { jbyte* bytes = env->GetByteArrayElements(source, /* isCopy= */ nullptr); jsize size = env->GetArrayLength(source); - ASSERT(dest->ParseFromArray(bytes, size)); + bool success = dest->ParseFromArray(bytes, size); + RELEASE_ASSERT(success, "Failed to parse protobuf message."); env->ReleaseByteArrayElements(source, bytes, 0); } From b9e4260466f74df759061e3a86e38c899c269913 Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Tue, 24 Oct 2023 19:41:08 -0400 Subject: [PATCH 15/17] Deprecation: Remove expand agnostic stream lifetime runtime guard. (#30470) Remove expand agnostic stream lifetime runtime guard. Signed-off-by: Kevin Baichoo --- changelogs/current.yaml | 3 + source/common/http/conn_manager_impl.cc | 28 +++--- source/common/http/conn_manager_impl.h | 2 - source/common/runtime/runtime_features.cc | 1 - source/common/runtime/runtime_features.h | 2 - .../buffer_accounting_integration_test.cc | 99 ------------------- test/integration/filter_integration_test.cc | 12 +-- .../multiplexed_integration_test.cc | 17 +--- test/integration/protocol_integration_test.cc | 24 ++--- 9 files changed, 28 insertions(+), 160 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index a4c947202f6c..defff0e35892 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -21,6 +21,9 @@ bug_fixes: removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` +- area: http + change: | + Removed ``envoy.reloadable_features.expand_agnostic_stream_lifetime`` and legacy code paths. - area: http change: | removed ``envoy.reloadable_features.correctly_validate_alpn`` and legacy code paths. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index fe0a08615e39..4b8b59d8238c 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -314,19 +314,17 @@ void ConnectionManagerImpl::doDeferredStreamDestroy(ActiveStream& stream) { stream.access_log_flush_timer_ = nullptr; } - if (stream.expand_agnostic_stream_lifetime_) { - // Only destroy the active stream if the underlying codec has notified us of - // completion or we've internal redirect the stream. - if (!stream.canDestroyStream()) { - // Track that this stream is not expecting any additional calls apart from - // codec notification. - stream.state_.is_zombie_stream_ = true; - return; - } + // Only destroy the active stream if the underlying codec has notified us of + // completion or we've internal redirect the stream. + if (!stream.canDestroyStream()) { + // Track that this stream is not expecting any additional calls apart from + // codec notification. + stream.state_.is_zombie_stream_ = true; + return; + } - if (stream.response_encoder_ != nullptr) { - stream.response_encoder_->getStream().registerCodecEventCallbacks(nullptr); - } + if (stream.response_encoder_ != nullptr) { + stream.response_encoder_->getStream().registerCodecEventCallbacks(nullptr); } stream.completeRequest(); @@ -421,9 +419,7 @@ RequestDecoder& ConnectionManagerImpl::newStream(ResponseEncoder& response_encod new_stream->state_.is_internally_created_ = is_internally_created; new_stream->response_encoder_ = &response_encoder; new_stream->response_encoder_->getStream().addCallbacks(*new_stream); - if (new_stream->expand_agnostic_stream_lifetime_) { - new_stream->response_encoder_->getStream().registerCodecEventCallbacks(new_stream.get()); - } + new_stream->response_encoder_->getStream().registerCodecEventCallbacks(new_stream.get()); new_stream->response_encoder_->getStream().setFlushTimeout(new_stream->idle_timeout_ms_); new_stream->streamInfo().setDownstreamBytesMeter(response_encoder.getStream().bytesMeter()); // If the network connection is backed up, the stream should be made aware of it on creation. @@ -845,8 +841,6 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect StreamInfo::FilterState::LifeSpan::Connection), request_response_timespan_(new Stats::HistogramCompletableTimespanImpl( connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), - expand_agnostic_stream_lifetime_( - Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)), header_validator_( connection_manager.config_.makeHeaderValidator(connection_manager.codec_->protocol())) { ASSERT(!connection_manager.config_.isRoutable() || diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 30a9d5039719..3b6c9c5a5a41 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -472,8 +472,6 @@ class ConnectionManagerImpl : Logger::Loggable, std::chrono::milliseconds idle_timeout_ms_{}; State state_; - const bool expand_agnostic_stream_lifetime_; - // Snapshot of the route configuration at the time of request is started. This is used to ensure // that the same route configuration is used throughout the lifetime of the request. This // snapshot will be cleared when the cached route is blocked. Because after that we will not diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index b55270ee36af..28ef11a67fda 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -43,7 +43,6 @@ RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection); RUNTIME_GUARD(envoy_reloadable_features_enable_connect_udp_support); RUNTIME_GUARD(envoy_reloadable_features_enable_intermediate_ca); RUNTIME_GUARD(envoy_reloadable_features_enable_zone_routing_different_zone_counts); -RUNTIME_GUARD(envoy_reloadable_features_expand_agnostic_stream_lifetime); RUNTIME_GUARD(envoy_reloadable_features_ext_authz_http_send_original_xff); RUNTIME_GUARD(envoy_reloadable_features_format_ports_as_numbers); RUNTIME_GUARD(envoy_reloadable_features_handle_uppercase_scheme); diff --git a/source/common/runtime/runtime_features.h b/source/common/runtime/runtime_features.h index 0f89b781c352..0c2fb2d9b9e7 100644 --- a/source/common/runtime/runtime_features.h +++ b/source/common/runtime/runtime_features.h @@ -26,8 +26,6 @@ void maybeSetRuntimeGuard(absl::string_view name, bool value); void maybeSetDeprecatedInts(absl::string_view name, uint32_t value); constexpr absl::string_view defer_processing_backedup_streams = "envoy.reloadable_features.defer_processing_backedup_streams"; -constexpr absl::string_view expand_agnostic_stream_lifetime = - "envoy.reloadable_features.expand_agnostic_stream_lifetime"; } // namespace Runtime } // namespace Envoy diff --git a/test/integration/buffer_accounting_integration_test.cc b/test/integration/buffer_accounting_integration_test.cc index 05bb8c1ae3ea..46d5194893d1 100644 --- a/test/integration/buffer_accounting_integration_test.cc +++ b/test/integration/buffer_accounting_integration_test.cc @@ -878,105 +878,6 @@ TEST_P(Http2OverloadManagerIntegrationTest, EXPECT_EQ(smallest_response->headers().getStatusValue(), "200"); } -TEST_P(Http2OverloadManagerIntegrationTest, CanResetStreamIfEnvoyLevelStreamEnded) { - // This test is not applicable if expand_agnostic_stream_lifetime is enabled - // as the gap between lifetimes of the codec level and envoy level stream - // shrinks. - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - return; - } - - useAccessLog("%RESPONSE_CODE%"); - initializeOverloadManagerInBootstrap( - TestUtility::parseYaml(R"EOF( - name: "envoy.overload_actions.reset_high_memory_stream" - triggers: - - name: "envoy.resource_monitors.testonly.fake_resource_monitor" - scaled: - scaling_threshold: 0.90 - saturation_threshold: 0.98 - )EOF")); - initialize(); - - // Set 10MiB receive window for the client. - const int downstream_window_size = 10 * 1024 * 1024; - envoy::config::core::v3::Http2ProtocolOptions http2_options = - ::Envoy::Http2::Utility::initializeAndValidateOptions( - envoy::config::core::v3::Http2ProtocolOptions()); - http2_options.mutable_initial_stream_window_size()->set_value(downstream_window_size); - http2_options.mutable_initial_connection_window_size()->set_value(downstream_window_size); - codec_client_ = makeRawHttpConnection(makeClientConnection(lookupPort("http")), http2_options); - - // Makes us have Envoy's writes to downstream return EAGAIN - write_matcher_->setSourcePort(lookupPort("http")); - write_matcher_->setWriteReturnsEgain(); - - // Send a request - auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ - {":method", "POST"}, - {":path", "/"}, - {":scheme", "http"}, - {":authority", "host"}, - {"content-length", "10"}, - }); - auto& encoder = encoder_decoder.first; - const std::string data(10, 'a'); - codec_client_->sendData(encoder, data, true); - auto response = std::move(encoder_decoder.second); - - waitForNextUpstreamRequest(); - FakeStreamPtr upstream_request_for_response = std::move(upstream_request_); - - // Send the responses back. It is larger than the downstream's receive window - // size. Thus, the codec will not end the stream, but the Envoy level stream - // should. - upstream_request_for_response->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, - false); - const int response_size = downstream_window_size + 1024; // Slightly over the window size. - upstream_request_for_response->encodeData(response_size, true); - - if (streamBufferAccounting()) { - if (deferProcessingBackedUpStreams()) { - // Wait for an accumulation of data, as we cannot rely on the access log - // output since we're deferring the processing of the stream data. - EXPECT_TRUE(buffer_factory_->waitUntilTotalBufferedExceeds(10 * 10 * 1024)); - - } else { - // Wait for access log to know the Envoy level stream has been deleted. - EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("200")); - } - } - - // Set the pressure so the overload action kills the response if doing stream - // accounting - updateResource(0.95); - test_server_->waitForGaugeEq( - "overload.envoy.overload_actions.reset_high_memory_stream.scale_percent", 62); - - if (streamBufferAccounting()) { - test_server_->waitForCounterGe("envoy.overload_actions.reset_high_memory_stream.count", 1); - } - - // Reduce resource pressure - updateResource(0.80); - test_server_->waitForGaugeEq( - "overload.envoy.overload_actions.reset_high_memory_stream.scale_percent", 0); - - // Resume writes to downstream. - write_matcher_->setResumeWrites(); - - if (streamBufferAccounting()) { - EXPECT_TRUE(response->waitForReset()); - EXPECT_TRUE(response->reset()); - } else { - // If we're not doing the accounting, we didn't end up resetting the - // streams. - ASSERT_TRUE(response->waitForEndStream()); - ASSERT_TRUE(response->complete()); - EXPECT_EQ(response->headers().getStatusValue(), "200"); - } -} - class Http2DeferredProcessingIntegrationTest : public Http2BufferWatermarksTest { public: Http2DeferredProcessingIntegrationTest() : registered_tee_factory_(tee_filter_factory_) { diff --git a/test/integration/filter_integration_test.cc b/test/integration/filter_integration_test.cc index 312326bef37b..8657a98034dd 100644 --- a/test/integration/filter_integration_test.cc +++ b/test/integration/filter_integration_test.cc @@ -184,15 +184,9 @@ TEST_P(FilterIntegrationTest, MissingHeadersLocalReplyDownstreamBytesCount) { EXPECT_EQ("200", response->headers().getStatusValue()); if (testing_downstream_filter_) { - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), - BytesCountExpectation(40, 58, 40, 58), - BytesCountExpectation(7, 10, 7, 8)); - } else { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), - BytesCountExpectation(0, 58, 0, 58), - BytesCountExpectation(7, 10, 7, 8)); - } + expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), + BytesCountExpectation(40, 58, 40, 58), + BytesCountExpectation(7, 10, 7, 8)); } } diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index 2d24888fc92e..580eb8b40716 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -2159,11 +2159,6 @@ TEST_P(Http2FrameIntegrationTest, AccessLogOfWireBytesIfResponseSizeGreaterThanW // Check access log if the agnostic stream lifetime is not extended. // It should have access logged since it has received the entire response. int hcm_logged_wire_bytes_sent, hcm_logged_wire_header_bytes_sent; - if (!Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - auto access_log_values = stoiAccessLogString(waitForAccessLog(access_log_name_)); - hcm_logged_wire_bytes_sent = access_log_values[0]; - hcm_logged_wire_header_bytes_sent = access_log_values[1]; - } // Grant the sender (Envoy) additional window so it can finish sending the // stream. @@ -2181,13 +2176,11 @@ TEST_P(Http2FrameIntegrationTest, AccessLogOfWireBytesIfResponseSizeGreaterThanW EXPECT_EQ(accumulator.bodyWireBytesReceivedDiscountingHeaders(), accumulator.bodyWireBytesReceivedGivenPayloadAndFrames()); - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - // Access logs are only available now due to the expanded agnostic stream - // lifetime. - auto access_log_values = stoiAccessLogString(waitForAccessLog(access_log_name_)); - hcm_logged_wire_bytes_sent = access_log_values[0]; - hcm_logged_wire_header_bytes_sent = access_log_values[1]; - } + // Access logs are only available now due to the expanded agnostic stream + // lifetime. + auto access_log_values = stoiAccessLogString(waitForAccessLog(access_log_name_)); + hcm_logged_wire_bytes_sent = access_log_values[0]; + hcm_logged_wire_header_bytes_sent = access_log_values[1]; EXPECT_EQ(accumulator.stream_wire_header_bytes_recieved_, hcm_logged_wire_header_bytes_sent); EXPECT_EQ(accumulator.stream_wire_bytes_recieved_, hcm_logged_wire_bytes_sent) << "Received " << accumulator.stream_wire_bytes_recieved_ diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index e3f35929356c..e554cedad5ea 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -201,15 +201,9 @@ TEST_P(DownstreamProtocolIntegrationTest, RouterRedirectHttpRequest) { EXPECT_EQ("301", response->headers().getStatusValue()); EXPECT_EQ("https://www.redirect.com/foo", response->headers().get(Http::Headers::get().Location)[0]->value().getStringView()); - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(145, 45, 111, 23), - BytesCountExpectation(69, 30, 69, 30), - BytesCountExpectation(0, 30, 0, 30)); - } else { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(145, 45, 111, 23), - BytesCountExpectation(0, 30, 0, 30), - BytesCountExpectation(0, 30, 0, 30)); - } + expectDownstreamBytesSentAndReceived(BytesCountExpectation(145, 45, 111, 23), + BytesCountExpectation(69, 30, 69, 30), + BytesCountExpectation(0, 30, 0, 30)); } else { // All QUIC requests use https, and should not be redirected. (Even those sent with http scheme // will be overridden to https by HCM.) @@ -717,15 +711,9 @@ TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReplyDownstreamByte ASSERT_TRUE(response->waitForEndStream()); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), - BytesCountExpectation(40, 58, 40, 58), - BytesCountExpectation(7, 10, 7, 8)); - } else { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), - BytesCountExpectation(0, 58, 0, 58), - BytesCountExpectation(7, 10, 7, 8)); - } + expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), + BytesCountExpectation(40, 58, 40, 58), + BytesCountExpectation(7, 10, 7, 8)); } TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReplyUpstreamBytesCount) { From 88a80e6bbbee56de8c3899c75eaf36c46fad1aa7 Mon Sep 17 00:00:00 2001 From: Pawan Bishnoi Date: Wed, 25 Oct 2023 07:18:07 +0530 Subject: [PATCH 16/17] Local rate limit - add rate_limited_as_resource_exhausted flag (#29279) * Local rate limit - add rate_limited_as_resource_exhausted flag to change grpc status code Signed-off-by: Pawan Kumar Signed-off-by: Pawan Bishnoi * fix minor renaming error Signed-off-by: Pawan Bishnoi * fix precheck error Signed-off-by: Pawan Bishnoi * add test Signed-off-by: Pawan Bishnoi * fix format and typo Signed-off-by: Pawan Bishnoi * review comments Signed-off-by: Pawan Bishnoi --------- Signed-off-by: Pawan Bishnoi --- .../local_ratelimit/v3/local_rate_limit.proto | 7 ++- changelogs/current.yaml | 5 ++ .../http/local_ratelimit/local_ratelimit.cc | 8 ++- .../http/local_ratelimit/local_ratelimit.h | 4 ++ .../http/local_ratelimit/filter_test.cc | 62 ++++++++++++++++--- 5 files changed, 73 insertions(+), 13 deletions(-) diff --git a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto index 24f43713aece..c253d049731c 100644 --- a/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto +++ b/api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto @@ -22,7 +22,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // Local Rate limit :ref:`configuration overview `. // [#extension: envoy.filters.http.local_ratelimit] -// [#next-free-field: 15] +// [#next-free-field: 16] message LocalRateLimit { // The human readable prefix to use when emitting stats. string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; @@ -125,4 +125,9 @@ message LocalRateLimit { // no matching descriptor. If set to true, default token bucket will always // be consumed. Default is true. google.protobuf.BoolValue always_consume_default_token_bucket = 14; + + // Specifies whether a ``RESOURCE_EXHAUSTED`` gRPC code must be returned instead + // of the default ``UNAVAILABLE`` gRPC code for a rate limited gRPC call. The + // HTTP code will be 200 for a gRPC response. + bool rate_limited_as_resource_exhausted = 15; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index defff0e35892..98170faca3b8 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -5,6 +5,11 @@ behavior_changes: minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* +- area: local_rate_limit + change: | + Added new configuration field :ref:`rate_limited_as_resource_exhausted + ` + to allow for setting if rate limit grpc response should be RESOURCE_EXHAUSTED instead of the default UNAVAILABLE. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc index 2dd744ec2df7..ac0ba0bed89d 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.cc @@ -59,7 +59,11 @@ FilterConfig::FilterConfig( has_descriptors_(!config.descriptors().empty()), enable_x_rate_limit_headers_(config.enable_x_ratelimit_headers() == envoy::extensions::common::ratelimit::v3::DRAFT_VERSION_03), - vh_rate_limits_(config.vh_rate_limits()) { + vh_rate_limits_(config.vh_rate_limits()), + rate_limited_grpc_status_( + config.rate_limited_as_resource_exhausted() + ? absl::make_optional(Grpc::Status::WellKnownGrpcStatus::ResourceExhausted) + : absl::nullopt) { // Note: no token bucket is fine for the global config, which would be the case for enabling // the filter globally but disabled and then applying limits at the virtual host or // route level. At the virtual or route level, it makes no sense to have an no token @@ -147,7 +151,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, [this, config](Http::HeaderMap& headers) { config->responseHeadersParser().evaluateHeaders(headers, decoder_callbacks_->streamInfo()); }, - absl::nullopt, "local_rate_limited"); + config->rateLimitedGrpcStatus(), "local_rate_limited"); decoder_callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::RateLimited); return Http::FilterHeadersStatus::StopIteration; diff --git a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h index 847cd4f4efc3..e816da64e37f 100644 --- a/source/extensions/filters/http/local_ratelimit/local_ratelimit.h +++ b/source/extensions/filters/http/local_ratelimit/local_ratelimit.h @@ -111,6 +111,9 @@ class FilterConfig : public Router::RouteSpecificFilterConfig { return vh_rate_limits_; } bool consumeDefaultTokenBucket() const { return always_consume_default_token_bucket_; } + const absl::optional rateLimitedGrpcStatus() const { + return rate_limited_grpc_status_; + } private: friend class FilterTest; @@ -147,6 +150,7 @@ class FilterConfig : public Router::RouteSpecificFilterConfig { const bool has_descriptors_; const bool enable_x_rate_limit_headers_; const envoy::extensions::common::ratelimit::v3::VhRateLimitsOptions vh_rate_limits_; + const absl::optional rate_limited_grpc_status_; }; using FilterConfigSharedPtr = std::shared_ptr; diff --git a/test/extensions/filters/http/local_ratelimit/filter_test.cc b/test/extensions/filters/http/local_ratelimit/filter_test.cc index fef4ea30bec2..9cacb3d7e953 100644 --- a/test/extensions/filters/http/local_ratelimit/filter_test.cc +++ b/test/extensions/filters/http/local_ratelimit/filter_test.cc @@ -17,6 +17,7 @@ namespace LocalRateLimitFilter { static const std::string config_yaml = R"( stat_prefix: test +rate_limited_as_resource_exhausted: {} token_bucket: max_tokens: {} tokens_per_fill: 1 @@ -105,17 +106,17 @@ class FilterTest : public testing::Test { }; TEST_F(FilterTest, Runtime) { - setup(fmt::format(fmt::runtime(config_yaml), "1", "false", "\"OFF\""), false, false); + setup(fmt::format(fmt::runtime(config_yaml), "false", "1", "false", "\"OFF\""), false, false); EXPECT_EQ(&runtime_, &(config_->runtime())); } TEST_F(FilterTest, ToErrorCode) { - setup(fmt::format(fmt::runtime(config_yaml), "1", "false", "\"OFF\""), false, false); + setup(fmt::format(fmt::runtime(config_yaml), "false", "1", "false", "\"OFF\""), false, false); EXPECT_EQ(Http::Code::BadRequest, toErrorCode(400)); } TEST_F(FilterTest, Disabled) { - setup(fmt::format(fmt::runtime(config_yaml), "1", "false", "\"OFF\""), false, false); + setup(fmt::format(fmt::runtime(config_yaml), "false", "1", "false", "\"OFF\""), false, false); auto headers = Http::TestRequestHeaderMapImpl(); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers, false)); EXPECT_EQ(0U, findCounter("test.http_local_rate_limit.enabled")); @@ -123,7 +124,7 @@ TEST_F(FilterTest, Disabled) { } TEST_F(FilterTest, RequestOk) { - setup(fmt::format(fmt::runtime(config_yaml), "1", "false", "\"OFF\"")); + setup(fmt::format(fmt::runtime(config_yaml), "false", "1", "false", "\"OFF\"")); auto headers = Http::TestRequestHeaderMapImpl(); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers, false)); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_2_->decodeHeaders(headers, false)); @@ -134,7 +135,7 @@ TEST_F(FilterTest, RequestOk) { } TEST_F(FilterTest, RequestOkPerConnection) { - setup(fmt::format(fmt::runtime(config_yaml), "1", "true", "\"OFF\"")); + setup(fmt::format(fmt::runtime(config_yaml), "false", "1", "true", "\"OFF\"")); auto headers = Http::TestRequestHeaderMapImpl(); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers, false)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_2_->decodeHeaders(headers, false)); @@ -145,7 +146,7 @@ TEST_F(FilterTest, RequestOkPerConnection) { } TEST_F(FilterTest, RequestRateLimited) { - setup(fmt::format(fmt::runtime(config_yaml), "1", "false", "\"OFF\"")); + setup(fmt::format(fmt::runtime(config_yaml), "false", "1", "false", "\"OFF\"")); EXPECT_CALL(decoder_callbacks_2_, sendLocalReply(Http::Code::TooManyRequests, _, _, _, _)) .WillOnce(Invoke([](Http::Code code, absl::string_view body, @@ -165,7 +166,6 @@ TEST_F(FilterTest, RequestRateLimited) { EXPECT_EQ("123", response_headers.get(Http::LowerCaseString("test-resp-req-id"))[0] ->value() .getStringView()); - EXPECT_EQ(grpc_status, absl::nullopt); EXPECT_EQ(details, "local_rate_limited"); })); @@ -186,6 +186,48 @@ TEST_F(FilterTest, RequestRateLimited) { EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.rate_limited")); } +TEST_F(FilterTest, RequestRateLimitedResourceExhausted) { + setup(fmt::format(fmt::runtime(config_yaml), "true", "1", "false", "\"OFF\"")); + + EXPECT_CALL(decoder_callbacks_2_, sendLocalReply(Http::Code::TooManyRequests, _, _, _, _)) + .WillOnce(Invoke([](Http::Code code, absl::string_view body, + std::function modify_headers, + const absl::optional grpc_status, + absl::string_view details) { + EXPECT_EQ(Http::Code::TooManyRequests, code); + EXPECT_EQ("local_rate_limited", body); + + Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + modify_headers(response_headers); + EXPECT_EQ("true", response_headers.get(Http::LowerCaseString("x-test-rate-limit"))[0] + ->value() + .getStringView()); + // Make sure that generated local reply headers contain a value dynamically + // generated by header formatter REQ(test-req-id) + EXPECT_EQ("123", response_headers.get(Http::LowerCaseString("test-resp-req-id"))[0] + ->value() + .getStringView()); + EXPECT_EQ(grpc_status, + absl::make_optional(Grpc::Status::WellKnownGrpcStatus::ResourceExhausted)); + EXPECT_EQ(details, "local_rate_limited"); + })); + + // Add a custom header to the request. + // Locally generated reply is configured to refer to this value. + Http::TestRequestHeaderMapImpl request_headers{{"test-req-id", "123"}}; + NiceMock stream_info; + + EXPECT_CALL(decoder_callbacks_2_, streamInfo).WillRepeatedly(testing::ReturnRef(stream_info)); + EXPECT_CALL(stream_info, getRequestHeaders).WillRepeatedly(Return(&request_headers)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false)); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_2_->decodeHeaders(request_headers, false)); + EXPECT_EQ(2U, findCounter("test.http_local_rate_limit.enabled")); + EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.enforced")); + EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.ok")); + EXPECT_EQ(1U, findCounter("test.http_local_rate_limit.rate_limited")); +} + /* This test sets 'local_rate_limit_per_downstream_connection' to true. Doing this enables per connection rate limiting and even though 'max_token' is set to 1, it allows 2 requests to go through @@ -193,7 +235,7 @@ connection rate limiting and even though 'max_token' is set to 1, it allows 2 re allowed (across the process) for the same configuration. */ TEST_F(FilterTest, RequestRateLimitedPerConnection) { - setup(fmt::format(fmt::runtime(config_yaml), "1", "true", "\"OFF\"")); + setup(fmt::format(fmt::runtime(config_yaml), "false", "1", "true", "\"OFF\"")); EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::TooManyRequests, _, _, _, _)) .WillOnce(Invoke([](Http::Code code, absl::string_view body, @@ -230,7 +272,7 @@ TEST_F(FilterTest, RequestRateLimitedPerConnection) { } TEST_F(FilterTest, RequestRateLimitedButNotEnforced) { - setup(fmt::format(fmt::runtime(config_yaml), "0", "false", "\"OFF\""), true, false); + setup(fmt::format(fmt::runtime(config_yaml), "false", "0", "false", "\"OFF\""), true, false); EXPECT_CALL(decoder_callbacks_, sendLocalReply(Http::Code::TooManyRequests, _, _, _, _)).Times(0); @@ -246,7 +288,7 @@ TEST_F(FilterTest, RequestRateLimitedButNotEnforced) { } TEST_F(FilterTest, RequestRateLimitedXRateLimitHeaders) { - setup(fmt::format(fmt::runtime(config_yaml), "1", "false", "DRAFT_VERSION_03")); + setup(fmt::format(fmt::runtime(config_yaml), "false", "1", "false", "DRAFT_VERSION_03")); auto request_headers = Http::TestRequestHeaderMapImpl(); auto response_headers = Http::TestResponseHeaderMapImpl(); From 0268a2bd1893b24e6441f07a3ee7484966952bdf Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Wed, 25 Oct 2023 04:16:46 +0000 Subject: [PATCH 17/17] mobile: Fix envoy-xds AAR (#30478) `--config=mobile-release-android` has `--define=google_grpc=disabled` and if it appears after `--define=google_grpc=enabled`, it will set `--define=google_grpc=disabled` because the last one set wins. This PR fixes this issue by moving the `--define=google_grpc=enabled` last since xDS feature in Envoy Mobile requires Google gRPC. Signed-off-by: Fredy Wijaya --- .github/workflows/mobile-release.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/mobile-release.yml b/.github/workflows/mobile-release.yml index 1b82ce622053..3489f87e9777 100644 --- a/.github/workflows/mobile-release.yml +++ b/.github/workflows/mobile-release.yml @@ -162,14 +162,16 @@ jobs: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} working-directory: mobile run: | + # --define=google_grpc=enabled must be defined last because + # --config=mobile-release-android has --define=google_grpc=disabled version="0.5.0.$(date '+%Y%m%d')" ./bazelw build \ --config=mobile-remote-release-clang \ --remote_header="Authorization=Bearer $GITHUB_TOKEN" \ --fat_apk_cpu=x86,x86_64,armeabi-v7a,arm64-v8a \ --define=pom_version="$version" \ - --define=google_grpc=enabled \ --config=mobile-release-android \ + --define=google_grpc=enabled \ --linkopt=-fuse-ld=lld \ //:android_xds_dist - name: 'Tar artifacts'