From eb9af96cd317b823ee6822dc10aeb26d63e51608 Mon Sep 17 00:00:00 2001 From: asraa Date: Tue, 13 Oct 2020 08:54:08 -0400 Subject: [PATCH 01/14] router: fix an invalid ASSERT when encoding metadata frames in the router. (#13511) Commit Message: Fix an invalid ASSERT when encoding metadata frames in the router. Additional Description: METADATA frames can't end stream, so there must be data, trailers, or a reset stream frame to end the stream. The ASSERT was meant to verify that there is data following METADATA frames to end the stream (for example, in case a client sends headers only request and metadata is added, FM adds empty data to end the stream). However, trailers could also end the stream, or the client could send body later/never. The ASSERT is invalid. The PR removes the ASSERT. Open to suggestions to have an ASSERT, but I couldn't think of one that was worth it / could be detected here. "if we had a header only request before, ASSERT there is empty data", or "if there is no possibility of stream reset occurring at this stage, ASSERT there is data or trailers." Risk Level: Low. This only affects the ASSERT, has no impact on traffic. Testing: Integration test added as a regression. Fuzz testcase added. Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=26238 Signed-off-by: Asra Ali --- source/common/router/upstream_request.cc | 2 - test/integration/h2_corpus/buffered_body | 61 ++++++++++++++++++++++ test/integration/http2_integration_test.cc | 19 +++++++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 test/integration/h2_corpus/buffered_body diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index e6465f7e73af..b906df34fdc9 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -440,8 +440,6 @@ void UpstreamRequest::encodeBodyAndTrailers() { downstream_metadata_map_vector_); upstream_->encodeMetadata(downstream_metadata_map_vector_); downstream_metadata_map_vector_.clear(); - - ASSERT(buffered_request_body_); } if (buffered_request_body_) { diff --git a/test/integration/h2_corpus/buffered_body b/test/integration/h2_corpus/buffered_body new file mode 100644 index 000000000000..e3f67d32d3cd --- /dev/null +++ b/test/integration/h2_corpus/buffered_body @@ -0,0 +1,61 @@ +events { + downstream_send_event { + h2_frames { + settings { + } + } + h2_frames { + request { + stream_index: 1 + host: "host" + path: "/p?th/to/longo" + } + } + } +} +events { + downstream_send_event { + h2_frames { + metadata { + flags: END_HEADERS + stream_index: 1 + metadata { + metadata { + key: "" + value: "@" + } + metadata { + key: "(" + value: "" + } + metadata { + key: "Timeout Seconds" + value: "10" + } + metadata { + key: "tincoes" + value: "15" + } + } + } + } + } +} +events { + downstream_send_event { + h2_frames { + settings { + flags: ACK + } + } + } +} +events { + upstream_send_event { + h2_frames { + window_update { + stream_index: 2147483648 + } + } + } +} diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 6a4c5bca960c..655181183fb0 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -568,6 +568,25 @@ TEST_P(Http2MetadataIntegrationTest, RequestMetadataReachSizeLimit) { ASSERT_FALSE(response->complete()); } +TEST_P(Http2MetadataIntegrationTest, RequestMetadataThenTrailers) { + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + Http::MetadataMap metadata_map = {{"key", "value"}}; + codec_client_->sendMetadata(*request_encoder_, metadata_map); + Http::TestRequestTrailerMapImpl request_trailers{{"trailer", "trailer"}}; + codec_client_->sendTrailers(*request_encoder_, request_trailers); + + waitForNextUpstreamRequest(); + + upstream_request_->encodeHeaders(default_response_headers_, true); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); +} + static std::string request_metadata_filter = R"EOF( name: request-metadata-filter typed_config: From 9181790e3ab21b53ec268470202986b9517c3723 Mon Sep 17 00:00:00 2001 From: htuch Date: Tue, 13 Oct 2020 09:20:39 -0400 Subject: [PATCH 02/14] dependencies: refactor repository location schema utils, cleanups. (#13452) - Refactor code responsible for processing repository location specs, i.e. checking for the presence of fields like last_updated and interpolation of version. The same code is now usable by both API repository_locations.bzl and bazel/repository_locations.bzl. - Cleanup reference to repo locations in repository_locations.bzl, now using a consistent set of macros. - Add API dependencies to dependency dashboard. Risk level: Low Testing: Docs build. Part of #12673 Signed-off-by: Harvey Tuch --- api/bazel/envoy_http_archive.bzl | 3 +- api/bazel/external_deps.bzl | 116 ++++++++ api/bazel/repositories.bzl | 39 +-- api/bazel/repository_locations.bzl | 23 +- api/bazel/repository_locations_utils.bzl | 20 ++ bazel/repositories.bzl | 276 ++++++------------ bazel/repository_locations.bzl | 105 ++----- docs/generate_external_dep_rst.py | 26 +- .../arch_overview/security/external_deps.rst | 5 + .../bazel/envoy_http_archive.bzl | 3 +- generated_api_shadow/bazel/external_deps.bzl | 116 ++++++++ generated_api_shadow/bazel/repositories.bzl | 39 +-- .../bazel/repository_locations.bzl | 23 +- .../bazel/repository_locations_utils.bzl | 20 ++ tools/dependency/validate.py | 4 +- 15 files changed, 450 insertions(+), 368 deletions(-) create mode 100644 api/bazel/external_deps.bzl create mode 100644 api/bazel/repository_locations_utils.bzl create mode 100644 generated_api_shadow/bazel/external_deps.bzl create mode 100644 generated_api_shadow/bazel/repository_locations_utils.bzl diff --git a/api/bazel/envoy_http_archive.bzl b/api/bazel/envoy_http_archive.bzl index 13b98f770619..15fd65b2af27 100644 --- a/api/bazel/envoy_http_archive.bzl +++ b/api/bazel/envoy_http_archive.bzl @@ -10,8 +10,7 @@ def envoy_http_archive(name, locations, **kwargs): # This repository has already been defined, probably because the user # wants to override the version. Do nothing. return - loc_key = kwargs.pop("repository_key", name) - location = locations[loc_key] + location = locations[name] # HTTP tarball at a given URL. Add a BUILD file if requested. http_archive( diff --git a/api/bazel/external_deps.bzl b/api/bazel/external_deps.bzl new file mode 100644 index 000000000000..cd9b6759f98a --- /dev/null +++ b/api/bazel/external_deps.bzl @@ -0,0 +1,116 @@ +load("@envoy_api//bazel:repository_locations_utils.bzl", "load_repository_locations_spec") + +# Envoy dependencies may be annotated with the following attributes: +DEPENDENCY_ANNOTATIONS = [ + # List of the categories describing how the dependency is being used. This attribute is used + # for automatic tracking of security posture of Envoy's dependencies. + # Possible values are documented in the USE_CATEGORIES list below. + # This attribute is mandatory for each dependecy. + "use_category", + + # Attribute specifying CPE (Common Platform Enumeration, see https://nvd.nist.gov/products/cpe) ID + # of the dependency. The ID may be in v2.3 or v2.2 format, although v2.3 is prefferred. See + # https://nvd.nist.gov/products/cpe for CPE format. Use single wildcard '*' for version and vector elements + # i.e. 'cpe:2.3:a:nghttp2:nghttp2:*'. Use "N/A" for dependencies without CPE assigned. + # This attribute is optional for components with use categories listed in the + # USE_CATEGORIES_WITH_CPE_OPTIONAL + "cpe", +] + +# NOTE: If a dependency use case is either dataplane or controlplane, the other uses are not needed +# to be declared. +USE_CATEGORIES = [ + # This dependency is used in API protos. + "api", + # This dependency is used in build process. + "build", + # This dependency is used to process xDS requests. + "controlplane", + # This dependency is used in processing downstream or upstream requests (core). + "dataplane_core", + # This dependency is used in processing downstream or upstream requests (extensions). + "dataplane_ext", + # This dependecy is used for logging, metrics or tracing (core). It may process unstrusted input. + "observability_core", + # This dependecy is used for logging, metrics or tracing (extensions). It may process unstrusted input. + "observability_ext", + # This dependency does not handle untrusted data and is used for various utility purposes. + "other", + # This dependency is used only in tests. + "test_only", +] + +# Components with these use categories are not required to specify the 'cpe' +# and 'last_updated' annotation. +USE_CATEGORIES_WITH_CPE_OPTIONAL = ["build", "other", "test_only", "api"] + +def _fail_missing_attribute(attr, key): + fail("The '%s' attribute must be defined for external dependecy " % attr + key) + +# Method for verifying content of the repository location specifications. +# +# We also remove repository metadata attributes so that further consumers, e.g. +# http_archive, are not confused by them. +def load_repository_locations(repository_locations_spec): + locations = {} + for key, location in load_repository_locations_spec(repository_locations_spec).items(): + mutable_location = dict(location) + locations[key] = mutable_location + + if "sha256" not in location or len(location["sha256"]) == 0: + _fail_missing_attribute("sha256", key) + + if "project_name" not in location: + _fail_missing_attribute("project_name", key) + mutable_location.pop("project_name") + + if "project_desc" not in location: + _fail_missing_attribute("project_desc", key) + mutable_location.pop("project_desc") + + if "project_url" not in location: + _fail_missing_attribute("project_url", key) + project_url = mutable_location.pop("project_url") + if not project_url.startswith("https://") and not project_url.startswith("http://"): + fail("project_url must start with https:// or http://: " + project_url) + + if "version" not in location: + _fail_missing_attribute("version", key) + mutable_location.pop("version") + + if "use_category" not in location: + _fail_missing_attribute("use_category", key) + use_category = mutable_location.pop("use_category") + + if "dataplane_ext" in use_category or "observability_ext" in use_category: + if "extensions" not in location: + _fail_missing_attribute("extensions", key) + mutable_location.pop("extensions") + + if "last_updated" not in location: + _fail_missing_attribute("last_updated", key) + last_updated = mutable_location.pop("last_updated") + + # Starlark doesn't have regexes. + if len(last_updated) != 10 or last_updated[4] != "-" or last_updated[7] != "-": + fail("last_updated must match YYYY-DD-MM: " + last_updated) + + if "cpe" in location: + cpe = mutable_location.pop("cpe") + + # Starlark doesn't have regexes. + cpe_components = len(cpe.split(":")) + + # We allow cpe:2.3:a:foo:* and cpe:2.3.:a:foo:bar:* only. + cpe_components_valid = cpe_components in [5, 6] + cpe_matches = (cpe == "N/A" or (cpe.startswith("cpe:2.3:a:") and cpe.endswith(":*") and cpe_components_valid)) + if not cpe_matches: + fail("CPE must match cpe:2.3:a:::*: " + cpe) + elif not [category for category in USE_CATEGORIES_WITH_CPE_OPTIONAL if category in location["use_category"]]: + _fail_missing_attribute("cpe", key) + + for category in location["use_category"]: + if category not in USE_CATEGORIES: + fail("Unknown use_category value '" + category + "' for dependecy " + key) + + return locations diff --git a/api/bazel/repositories.bzl b/api/bazel/repositories.bzl index a64e733cf74a..a12a0ea98b3a 100644 --- a/api/bazel/repositories.bzl +++ b/api/bazel/repositories.bzl @@ -1,40 +1,43 @@ load(":envoy_http_archive.bzl", "envoy_http_archive") -load(":repository_locations.bzl", "REPOSITORY_LOCATIONS") +load(":external_deps.bzl", "load_repository_locations") +load(":repository_locations.bzl", "REPOSITORY_LOCATIONS_SPEC") -def api_dependencies(): +REPOSITORY_LOCATIONS = load_repository_locations(REPOSITORY_LOCATIONS_SPEC) + +# Use this macro to reference any HTTP archive from bazel/repository_locations.bzl. +def external_http_archive(name, **kwargs): envoy_http_archive( - "bazel_skylib", + name, locations = REPOSITORY_LOCATIONS, + **kwargs ) - envoy_http_archive( - "com_envoyproxy_protoc_gen_validate", - locations = REPOSITORY_LOCATIONS, + +def api_dependencies(): + external_http_archive( + name = "bazel_skylib", ) - envoy_http_archive( + external_http_archive( + name = "com_envoyproxy_protoc_gen_validate", + ) + external_http_archive( name = "com_google_googleapis", - locations = REPOSITORY_LOCATIONS, ) - envoy_http_archive( + external_http_archive( name = "com_github_cncf_udpa", - locations = REPOSITORY_LOCATIONS, ) - envoy_http_archive( + external_http_archive( name = "prometheus_metrics_model", - locations = REPOSITORY_LOCATIONS, build_file_content = PROMETHEUSMETRICS_BUILD_CONTENT, ) - envoy_http_archive( + external_http_archive( name = "opencensus_proto", - locations = REPOSITORY_LOCATIONS, ) - envoy_http_archive( + external_http_archive( name = "rules_proto", - locations = REPOSITORY_LOCATIONS, ) - envoy_http_archive( + external_http_archive( name = "com_github_openzipkin_zipkinapi", - locations = REPOSITORY_LOCATIONS, build_file_content = ZIPKINAPI_BUILD_CONTENT, ) diff --git a/api/bazel/repository_locations.bzl b/api/bazel/repository_locations.bzl index 152d4be5ee0d..bdcf31e867d2 100644 --- a/api/bazel/repository_locations.bzl +++ b/api/bazel/repository_locations.bzl @@ -1,4 +1,5 @@ -DEPENDENCY_REPOSITORIES_SPEC = dict( +# This should match the schema defined in external_deps.bzl. +REPOSITORY_LOCATIONS_SPEC = dict( bazel_skylib = dict( project_name = "bazel-skylib", project_desc = "Common useful functions and rules for Bazel", @@ -88,23 +89,3 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( use_category = ["api"], ), ) - -def _format_version(s, version): - return s.format(version = version, dash_version = version.replace(".", "-"), underscore_version = version.replace(".", "_")) - -# Interpolate {version} in the above dependency specs. This code should be capable of running in both Python -# and Starlark. -def _dependency_repositories(): - locations = {} - for key, location in DEPENDENCY_REPOSITORIES_SPEC.items(): - mutable_location = dict(location) - locations[key] = mutable_location - - # Fixup with version information. - if "version" in location: - if "strip_prefix" in location: - mutable_location["strip_prefix"] = _format_version(location["strip_prefix"], location["version"]) - mutable_location["urls"] = [_format_version(url, location["version"]) for url in location["urls"]] - return locations - -REPOSITORY_LOCATIONS = _dependency_repositories() diff --git a/api/bazel/repository_locations_utils.bzl b/api/bazel/repository_locations_utils.bzl new file mode 100644 index 000000000000..3b984e1bc580 --- /dev/null +++ b/api/bazel/repository_locations_utils.bzl @@ -0,0 +1,20 @@ +def _format_version(s, version): + return s.format(version = version, dash_version = version.replace(".", "-"), underscore_version = version.replace(".", "_")) + +# Generate a "repository location specification" from raw repository +# specification. The information should match the format required by +# external_deps.bzl. This function mostly does interpolation of {version} in +# the repository info fields. This code should be capable of running in both +# Python and Starlark. +def load_repository_locations_spec(repository_locations_spec): + locations = {} + for key, location in repository_locations_spec.items(): + mutable_location = dict(location) + locations[key] = mutable_location + + # Fixup with version information. + if "version" in location: + if "strip_prefix" in location: + mutable_location["strip_prefix"] = _format_version(location["strip_prefix"], location["version"]) + mutable_location["urls"] = [_format_version(url, location["version"]) for url in location["urls"]] + return locations diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index a8115a021ced..12825602848c 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -1,8 +1,8 @@ -load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") load(":dev_binding.bzl", "envoy_dev_binding") load(":genrule_repository.bzl", "genrule_repository") load("@envoy_api//bazel:envoy_http_archive.bzl", "envoy_http_archive") -load(":repository_locations.bzl", "DEPENDENCY_ANNOTATIONS", "DEPENDENCY_REPOSITORIES", "USE_CATEGORIES", "USE_CATEGORIES_WITH_CPE_OPTIONAL") +load("@envoy_api//bazel:external_deps.bzl", "load_repository_locations") +load(":repository_locations.bzl", "REPOSITORY_LOCATIONS_SPEC") load("@com_google_googleapis//:repository_rules.bzl", "switched_rules_by_language") load(":crates.bzl", "raze_fetch_remote_crates") @@ -18,97 +18,29 @@ WINDOWS_SKIP_TARGETS = [ # Make all contents of an external repository accessible under a filegroup. Used for external HTTP # archives, e.g. cares. -BUILD_ALL_CONTENT = """filegroup(name = "all", srcs = glob(["**"]), visibility = ["//visibility:public"])""" - def _build_all_content(exclude = []): return """filegroup(name = "all", srcs = glob(["**"], exclude={}), visibility = ["//visibility:public"])""".format(repr(exclude)) -def _fail_missing_attribute(attr, key): - fail("The '%s' attribute must be defined for external dependecy " % attr + key) - -# Method for verifying content of the DEPENDENCY_REPOSITORIES defined in bazel/repository_locations.bzl -# Verification is here so that bazel/repository_locations.bzl can be loaded into other tools written in Python, -# and as such needs to be free of bazel specific constructs. -# -# We also remove the attributes for further consumption in this file, since rules such as http_archive -# don't recognize them. -def _repository_locations(): - locations = {} - for key, location in DEPENDENCY_REPOSITORIES.items(): - mutable_location = dict(location) - locations[key] = mutable_location - - if "sha256" not in location or len(location["sha256"]) == 0: - _fail_missing_attribute("sha256", key) - - if "project_name" not in location: - _fail_missing_attribute("project_name", key) - mutable_location.pop("project_name") - - if "project_desc" not in location: - _fail_missing_attribute("project_desc", key) - mutable_location.pop("project_desc") - - if "project_url" not in location: - _fail_missing_attribute("project_url", key) - project_url = mutable_location.pop("project_url") - if not project_url.startswith("https://") and not project_url.startswith("http://"): - fail("project_url must start with https:// or http://: " + project_url) - - if "version" not in location: - _fail_missing_attribute("version", key) - mutable_location.pop("version") - - if "use_category" not in location: - _fail_missing_attribute("use_category", key) - use_category = mutable_location.pop("use_category") - - if "dataplane_ext" in use_category or "observability_ext" in use_category: - if "extensions" not in location: - _fail_missing_attribute("extensions", key) - mutable_location.pop("extensions") - - if "last_updated" not in location: - _fail_missing_attribute("last_updated", key) - last_updated = mutable_location.pop("last_updated") - - # Starlark doesn't have regexes. - if len(last_updated) != 10 or last_updated[4] != "-" or last_updated[7] != "-": - fail("last_updated must match YYYY-DD-MM: " + last_updated) - - if "cpe" in location: - cpe = mutable_location.pop("cpe") - - # Starlark doesn't have regexes. - cpe_matches = (cpe != "N/A" and (not cpe.startswith("cpe:2.3:a:") or not cpe.endswith(":*") and len(cpe.split(":")) != 6)) - if cpe_matches: - fail("CPE must match cpe:2.3:a:::*: " + cpe) - elif not [category for category in USE_CATEGORIES_WITH_CPE_OPTIONAL if category in location["use_category"]]: - _fail_missing_attribute("cpe", key) - - for category in location["use_category"]: - if category not in USE_CATEGORIES: - fail("Unknown use_category value '" + category + "' for dependecy " + key) - - return locations - -REPOSITORY_LOCATIONS = _repository_locations() - -# To initialize http_archive REPOSITORY_LOCATIONS dictionaries must be stripped of annotations. -# See repository_locations.bzl for the list of annotation attributes. -def _get_location(dependency): - stripped = dict(REPOSITORY_LOCATIONS[dependency]) - for attribute in DEPENDENCY_ANNOTATIONS: - stripped.pop(attribute, None) - return stripped - -def _repository_impl(name, **kwargs): +BUILD_ALL_CONTENT = _build_all_content() + +REPOSITORY_LOCATIONS = load_repository_locations(REPOSITORY_LOCATIONS_SPEC) + +# Use this macro to reference any HTTP archive from bazel/repository_locations.bzl. +def external_http_archive(name, **kwargs): envoy_http_archive( name, locations = REPOSITORY_LOCATIONS, **kwargs ) +# Use this macro to reference any genrule_repository sourced from bazel/repository_locations.bzl. +def external_genrule_repository(name, **kwargs): + location = REPOSITORY_LOCATIONS[name] + genrule_repository( + name = name, + **dict(location, **kwargs) + ) + def _default_envoy_build_config_impl(ctx): ctx.file("WORKSPACE", "") ctx.file("BUILD.bazel", "") @@ -124,26 +56,26 @@ _default_envoy_build_config = repository_rule( # Python dependencies. def _python_deps(): # TODO(htuch): convert these to pip3_import. - _repository_impl( + external_http_archive( name = "com_github_twitter_common_lang", build_file = "@envoy//bazel/external:twitter_common_lang.BUILD", ) - _repository_impl( + external_http_archive( name = "com_github_twitter_common_rpc", build_file = "@envoy//bazel/external:twitter_common_rpc.BUILD", ) - _repository_impl( + external_http_archive( name = "com_github_twitter_common_finagle_thrift", build_file = "@envoy//bazel/external:twitter_common_finagle_thrift.BUILD", ) - _repository_impl( + external_http_archive( name = "six", build_file = "@com_google_protobuf//third_party:six.BUILD", ) # Bazel native C++ dependencies. For the dependencies that doesn't provide autoconf/automake builds. def _cc_deps(): - _repository_impl("grpc_httpjson_transcoding") + external_http_archive("grpc_httpjson_transcoding") native.bind( name = "path_matcher", actual = "@grpc_httpjson_transcoding//src:path_matcher", @@ -157,16 +89,16 @@ def _go_deps(skip_targets): # Keep the skip_targets check around until Istio Proxy has stopped using # it to exclude the Go rules. if "io_bazel_rules_go" not in skip_targets: - _repository_impl( + external_http_archive( name = "io_bazel_rules_go", # TODO(wrowe, sunjayBhatia): remove when Windows RBE supports batch file invocation patch_args = ["-p1"], patches = ["@envoy//bazel:rules_go.patch"], ) - _repository_impl("bazel_gazelle") + external_http_archive("bazel_gazelle") def _rust_deps(): - _repository_impl("io_bazel_rules_rust") + external_http_archive("io_bazel_rules_rust") raze_fetch_remote_crates() def envoy_dependencies(skip_targets = []): @@ -232,13 +164,13 @@ def envoy_dependencies(skip_targets = []): _proxy_wasm_cpp_sdk() _proxy_wasm_cpp_host() _emscripten_toolchain() - _repository_impl("com_googlesource_code_re2") + external_http_archive("com_googlesource_code_re2") _com_google_cel_cpp() - _repository_impl("com_github_google_flatbuffers") - _repository_impl("bazel_toolchains") - _repository_impl("bazel_compdb") - _repository_impl("envoy_build_tools") - _repository_impl("rules_cc") + external_http_archive("com_github_google_flatbuffers") + external_http_archive("bazel_toolchains") + external_http_archive("bazel_compdb") + external_http_archive("envoy_build_tools") + external_http_archive("rules_cc") # Unconditional, since we use this only for compiler-agnostic fuzzing utils. _org_llvm_releases_compiler_rt() @@ -267,25 +199,22 @@ def envoy_dependencies(skip_targets = []): ) def _boringssl(): - _repository_impl( + external_http_archive( name = "boringssl", patch_args = ["-p1"], patches = ["@envoy//bazel:boringssl_static.patch"], ) def _boringssl_fips(): - location = REPOSITORY_LOCATIONS["boringssl_fips"] - genrule_repository( + external_genrule_repository( name = "boringssl_fips", - urls = location["urls"], - sha256 = location["sha256"], genrule_cmd_file = "@envoy//bazel/external:boringssl_fips.genrule_cmd", build_file = "@envoy//bazel/external:boringssl_fips.BUILD", patches = ["@envoy//bazel/external:boringssl_fips.patch"], ) def _com_github_circonus_labs_libcircllhist(): - _repository_impl( + external_http_archive( name = "com_github_circonus_labs_libcircllhist", build_file = "@envoy//bazel/external:libcircllhist.BUILD", ) @@ -295,11 +224,9 @@ def _com_github_circonus_labs_libcircllhist(): ) def _com_github_c_ares_c_ares(): - location = _get_location("com_github_c_ares_c_ares") - http_archive( + external_http_archive( name = "com_github_c_ares_c_ares", build_file_content = BUILD_ALL_CONTENT, - **location ) native.bind( name = "ares", @@ -307,7 +234,7 @@ def _com_github_c_ares_c_ares(): ) def _com_github_cyan4973_xxhash(): - _repository_impl( + external_http_archive( name = "com_github_cyan4973_xxhash", build_file = "@envoy//bazel/external:xxhash.BUILD", ) @@ -317,7 +244,7 @@ def _com_github_cyan4973_xxhash(): ) def _com_github_envoyproxy_sqlparser(): - _repository_impl( + external_http_archive( name = "com_github_envoyproxy_sqlparser", build_file = "@envoy//bazel/external:sqlparser.BUILD", ) @@ -327,7 +254,7 @@ def _com_github_envoyproxy_sqlparser(): ) def _com_github_mirror_tclap(): - _repository_impl( + external_http_archive( name = "com_github_mirror_tclap", build_file = "@envoy//bazel/external:tclap.BUILD", patch_args = ["-p1"], @@ -343,7 +270,7 @@ def _com_github_mirror_tclap(): ) def _com_github_fmtlib_fmt(): - _repository_impl( + external_http_archive( name = "com_github_fmtlib_fmt", build_file = "@envoy//bazel/external:fmtlib.BUILD", ) @@ -353,7 +280,7 @@ def _com_github_fmtlib_fmt(): ) def _com_github_gabime_spdlog(): - _repository_impl( + external_http_archive( name = "com_github_gabime_spdlog", build_file = "@envoy//bazel/external:spdlog.BUILD", ) @@ -363,10 +290,8 @@ def _com_github_gabime_spdlog(): ) def _com_github_google_benchmark(): - location = _get_location("com_github_google_benchmark") - http_archive( + external_http_archive( name = "com_github_google_benchmark", - **location ) native.bind( name = "benchmark", @@ -374,13 +299,13 @@ def _com_github_google_benchmark(): ) def _com_github_google_libprotobuf_mutator(): - _repository_impl( + external_http_archive( name = "com_github_google_libprotobuf_mutator", build_file = "@envoy//bazel/external:libprotobuf_mutator.BUILD", ) def _com_github_jbeder_yaml_cpp(): - _repository_impl( + external_http_archive( name = "com_github_jbeder_yaml_cpp", ) native.bind( @@ -389,11 +314,9 @@ def _com_github_jbeder_yaml_cpp(): ) def _com_github_libevent_libevent(): - location = _get_location("com_github_libevent_libevent") - http_archive( + external_http_archive( name = "com_github_libevent_libevent", build_file_content = BUILD_ALL_CONTENT, - **location ) native.bind( name = "event", @@ -401,7 +324,7 @@ def _com_github_libevent_libevent(): ) def _net_zlib(): - _repository_impl( + external_http_archive( name = "net_zlib", build_file_content = BUILD_ALL_CONTENT, patch_args = ["-p1"], @@ -420,16 +343,15 @@ def _net_zlib(): ) def _com_github_zlib_ng_zlib_ng(): - _repository_impl( + external_http_archive( name = "com_github_zlib_ng_zlib_ng", build_file_content = BUILD_ALL_CONTENT, ) def _com_google_cel_cpp(): - _repository_impl("com_google_cel_cpp") - _repository_impl("rules_antlr") - location = _get_location("antlr4_runtimes") - http_archive( + external_http_archive("com_google_cel_cpp") + external_http_archive("rules_antlr") + external_http_archive( name = "antlr4_runtimes", build_file_content = """ package(default_visibility = ["//visibility:public"]) @@ -443,19 +365,18 @@ cc_library( patch_args = ["-p1"], # Patches ASAN violation of initialization fiasco patches = ["@envoy//bazel:antlr.patch"], - **location ) # Parser dependencies # TODO: upgrade this when cel is upgraded to use the latest version - http_archive( + external_http_archive( name = "rules_antlr", sha256 = "7249d1569293d9b239e23c65f6b4c81a07da921738bde0dfeb231ed98be40429", strip_prefix = "rules_antlr-3cc2f9502a54ceb7b79b37383316b23c4da66f9a", urls = ["https://github.com/marcohu/rules_antlr/archive/3cc2f9502a54ceb7b79b37383316b23c4da66f9a.tar.gz"], ) - http_archive( + external_http_archive( name = "antlr4_runtimes", build_file_content = """ package(default_visibility = ["//visibility:public"]) @@ -475,8 +396,7 @@ cc_library( ) def _com_github_nghttp2_nghttp2(): - location = _get_location("com_github_nghttp2_nghttp2") - http_archive( + external_http_archive( name = "com_github_nghttp2_nghttp2", build_file_content = BUILD_ALL_CONTENT, patch_args = ["-p1"], @@ -485,7 +405,6 @@ def _com_github_nghttp2_nghttp2(): # https://github.com/nghttp2/nghttp2/pull/1395 # https://github.com/envoyproxy/envoy/pull/8572#discussion_r334067786 patches = ["@envoy//bazel/foreign_cc:nghttp2.patch"], - **location ) native.bind( name = "nghttp2", @@ -493,7 +412,7 @@ def _com_github_nghttp2_nghttp2(): ) def _io_opentracing_cpp(): - _repository_impl( + external_http_archive( name = "io_opentracing_cpp", patch_args = ["-p1"], # Workaround for LSAN false positive in https://github.com/envoyproxy/envoy/issues/7647 @@ -505,15 +424,15 @@ def _io_opentracing_cpp(): ) def _com_lightstep_tracer_cpp(): - _repository_impl("com_lightstep_tracer_cpp") + external_http_archive("com_lightstep_tracer_cpp") native.bind( name = "lightstep", actual = "@com_lightstep_tracer_cpp//:manual_tracer_lib", ) def _com_github_datadog_dd_opentracing_cpp(): - _repository_impl("com_github_datadog_dd_opentracing_cpp") - _repository_impl( + external_http_archive("com_github_datadog_dd_opentracing_cpp") + external_http_archive( name = "com_github_msgpack_msgpack_c", build_file = "@com_github_datadog_dd_opentracing_cpp//:bazel/external/msgpack.BUILD", ) @@ -523,7 +442,7 @@ def _com_github_datadog_dd_opentracing_cpp(): ) def _com_github_tencent_rapidjson(): - _repository_impl( + external_http_archive( name = "com_github_tencent_rapidjson", build_file = "@envoy//bazel/external:rapidjson.BUILD", ) @@ -533,7 +452,7 @@ def _com_github_tencent_rapidjson(): ) def _com_github_nodejs_http_parser(): - _repository_impl( + external_http_archive( name = "com_github_nodejs_http_parser", build_file = "@envoy//bazel/external:http-parser.BUILD", ) @@ -543,7 +462,7 @@ def _com_github_nodejs_http_parser(): ) def _com_google_googletest(): - _repository_impl("com_google_googletest") + external_http_archive("com_google_googletest") native.bind( name = "googletest", actual = "@com_google_googletest//:gtest", @@ -554,7 +473,7 @@ def _com_google_googletest(): # pull in more bits of abseil as needed, and is now the preferred # method for pure Bazel deps. def _com_google_absl(): - _repository_impl("com_google_absl") + external_http_archive("com_google_absl") native.bind( name = "abseil_any", actual = "@com_google_absl//absl/types:any", @@ -657,8 +576,8 @@ def _com_google_absl(): ) def _com_google_protobuf(): - _repository_impl("rules_python") - _repository_impl( + external_http_archive("rules_python") + external_http_archive( "com_google_protobuf", patches = ["@envoy//bazel:protobuf.patch"], patch_args = ["-p1"], @@ -689,10 +608,8 @@ def _com_google_protobuf(): ) def _io_opencensus_cpp(): - location = _get_location("io_opencensus_cpp") - http_archive( + external_http_archive( name = "io_opencensus_cpp", - **location ) native.bind( name = "opencensus_trace", @@ -733,8 +650,7 @@ def _io_opencensus_cpp(): def _com_github_curl(): # Used by OpenCensus Zipkin exporter. - location = _get_location("com_github_curl") - http_archive( + external_http_archive( name = "com_github_curl", build_file_content = BUILD_ALL_CONTENT + """ cc_library(name = "curl", visibility = ["//visibility:public"], deps = ["@envoy//bazel/foreign_cc:curl"]) @@ -749,7 +665,6 @@ cc_library(name = "curl", visibility = ["//visibility:public"], deps = ["@envoy/ # by elimination of the curl dependency. patches = ["@envoy//bazel/foreign_cc:curl.patch"], patch_args = ["-p1"], - **location ) native.bind( name = "curl", @@ -757,13 +672,11 @@ cc_library(name = "curl", visibility = ["//visibility:public"], deps = ["@envoy/ ) def _com_googlesource_chromium_v8(): - location = _get_location("com_googlesource_chromium_v8") - genrule_repository( + external_genrule_repository( name = "com_googlesource_chromium_v8", genrule_cmd_file = "@envoy//bazel/external:wee8.genrule_cmd", build_file = "@envoy//bazel/external:wee8.BUILD", patches = ["@envoy//bazel/external:wee8.patch"], - **location ) native.bind( name = "wee8", @@ -771,11 +684,8 @@ def _com_googlesource_chromium_v8(): ) def _com_googlesource_quiche(): - location = REPOSITORY_LOCATIONS["com_googlesource_quiche"] - genrule_repository( + external_genrule_repository( name = "com_googlesource_quiche", - urls = location["urls"], - sha256 = location["sha256"], genrule_cmd_file = "@envoy//bazel/external:quiche.genrule_cmd", build_file = "@envoy//bazel/external:quiche.BUILD", ) @@ -801,7 +711,7 @@ def _com_googlesource_quiche(): ) def _com_googlesource_googleurl(): - _repository_impl( + external_http_archive( name = "com_googlesource_googleurl", ) native.bind( @@ -810,14 +720,14 @@ def _com_googlesource_googleurl(): ) def _org_llvm_releases_compiler_rt(): - _repository_impl( + external_http_archive( name = "org_llvm_releases_compiler_rt", build_file = "@envoy//bazel/external:compiler_rt.BUILD", ) def _com_github_grpc_grpc(): - _repository_impl("com_github_grpc_grpc") - _repository_impl("build_bazel_rules_apple") + external_http_archive("com_github_grpc_grpc") + external_http_archive("build_bazel_rules_apple") # Rebind some stuff to match what the gRPC Bazel is expecting. native.bind( @@ -859,7 +769,7 @@ def _com_github_grpc_grpc(): ) def _upb(): - _repository_impl( + external_http_archive( name = "upb", patches = ["@envoy//bazel:upb.patch"], patch_args = ["-p1"], @@ -871,26 +781,28 @@ def _upb(): ) def _proxy_wasm_cpp_sdk(): - _repository_impl(name = "proxy_wasm_cpp_sdk") + external_http_archive(name = "proxy_wasm_cpp_sdk") def _proxy_wasm_cpp_host(): - _repository_impl( + external_http_archive( name = "proxy_wasm_cpp_host", build_file = "@envoy//bazel/external:proxy_wasm_cpp_host.BUILD", ) def _emscripten_toolchain(): - _repository_impl( + external_http_archive( name = "emscripten_toolchain", build_file_content = _build_all_content(exclude = [ "upstream/emscripten/cache/is_vanilla.txt", ".emscripten_sanity", ]), - patch_cmds = REPOSITORY_LOCATIONS["emscripten_toolchain"]["patch_cmds"], + patch_cmds = [ + "[[ \"$(uname -m)\" == \"x86_64\" ]] && ./emsdk install 1.39.6-upstream && ./emsdk activate --embedded 1.39.6-upstream || true", + ], ) def _com_github_google_jwt_verify(): - _repository_impl("com_github_google_jwt_verify") + external_http_archive("com_github_google_jwt_verify") native.bind( name = "jwt_verify_lib", @@ -898,14 +810,12 @@ def _com_github_google_jwt_verify(): ) def _com_github_luajit_luajit(): - location = _get_location("com_github_luajit_luajit") - http_archive( + external_http_archive( name = "com_github_luajit_luajit", build_file_content = BUILD_ALL_CONTENT, patches = ["@envoy//bazel/foreign_cc:luajit.patch"], patch_args = ["-p1"], patch_cmds = ["chmod u+x build.py"], - **location ) native.bind( @@ -914,14 +824,12 @@ def _com_github_luajit_luajit(): ) def _com_github_moonjit_moonjit(): - location = _get_location("com_github_moonjit_moonjit") - http_archive( + external_http_archive( name = "com_github_moonjit_moonjit", build_file_content = BUILD_ALL_CONTENT, patches = ["@envoy//bazel/foreign_cc:moonjit.patch"], patch_args = ["-p1"], patch_cmds = ["chmod u+x build.py"], - **location ) native.bind( @@ -930,7 +838,7 @@ def _com_github_moonjit_moonjit(): ) def _com_github_google_tcmalloc(): - _repository_impl( + external_http_archive( name = "com_github_google_tcmalloc", ) @@ -940,26 +848,21 @@ def _com_github_google_tcmalloc(): ) def _com_github_gperftools_gperftools(): - location = _get_location("com_github_gperftools_gperftools") - http_archive( + external_http_archive( name = "com_github_gperftools_gperftools", build_file_content = BUILD_ALL_CONTENT, - **location ) - native.bind( name = "gperftools", actual = "@envoy//bazel/foreign_cc:gperftools", ) def _org_llvm_llvm(): - location = _get_location("org_llvm_llvm") - http_archive( + external_http_archive( name = "org_llvm_llvm", build_file_content = BUILD_ALL_CONTENT, patch_args = ["-p1"], patches = ["@envoy//bazel/foreign_cc:llvm.patch"], - **location ) native.bind( name = "llvm", @@ -967,11 +870,9 @@ def _org_llvm_llvm(): ) def _com_github_wavm_wavm(): - location = _get_location("com_github_wavm_wavm") - http_archive( + external_http_archive( name = "com_github_wavm_wavm", build_file_content = BUILD_ALL_CONTENT, - **location ) native.bind( name = "wavm", @@ -993,31 +894,28 @@ filegroup( visibility = ["//visibility:public"], ) """ - http_archive( + external_http_archive( name = "kafka_source", build_file_content = KAFKASOURCE_BUILD_CONTENT, patches = ["@envoy//bazel/external:kafka_int32.patch"], - **_get_location("kafka_source") ) # This archive provides Kafka (and Zookeeper) binaries, that are used during Kafka integration # tests. - http_archive( + external_http_archive( name = "kafka_server_binary", build_file_content = BUILD_ALL_CONTENT, - **_get_location("kafka_server_binary") ) # This archive provides Kafka client in Python, so we can use it to interact with Kafka server # during interation tests. - http_archive( + external_http_archive( name = "kafka_python_client", build_file_content = BUILD_ALL_CONTENT, - **_get_location("kafka_python_client") ) def _foreign_cc_dependencies(): - _repository_impl("rules_foreign_cc") + external_http_archive("rules_foreign_cc") def _is_linux(ctxt): return ctxt.os.name == "linux" diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 07038daa5a72..7ab570248cd2 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -1,51 +1,5 @@ -# Validation of content in this file is done on the bazel/repositories.bzl file to make it free of bazel -# constructs. This is to allow this file to be loaded into Python based build and maintenance tools. - -# Envoy dependencies may be annotated with the following attributes: -DEPENDENCY_ANNOTATIONS = [ - # List of the categories describing how the dependency is being used. This attribute is used - # for automatic tracking of security posture of Envoy's dependencies. - # Possible values are documented in the USE_CATEGORIES list below. - # This attribute is mandatory for each dependecy. - "use_category", - - # Attribute specifying CPE (Common Platform Enumeration, see https://nvd.nist.gov/products/cpe) ID - # of the dependency. The ID may be in v2.3 or v2.2 format, although v2.3 is prefferred. See - # https://nvd.nist.gov/products/cpe for CPE format. Use single wildcard '*' for version and vector elements - # i.e. 'cpe:2.3:a:nghttp2:nghttp2:*'. Use "N/A" for dependencies without CPE assigned. - # This attribute is optional for components with use categories listed in the - # USE_CATEGORIES_WITH_CPE_OPTIONAL - "cpe", -] - -# NOTE: If a dependency use case is either dataplane or controlplane, the other uses are not needed -# to be declared. -USE_CATEGORIES = [ - # This dependency is used in API protos. - "api", - # This dependency is used in build process. - "build", - # This dependency is used to process xDS requests. - "controlplane", - # This dependency is used in processing downstream or upstream requests (core). - "dataplane_core", - # This dependency is used in processing downstream or upstream requests (extensions). - "dataplane_ext", - # This dependecy is used for logging, metrics or tracing (core). It may process unstrusted input. - "observability_core", - # This dependecy is used for logging, metrics or tracing (extensions). It may process unstrusted input. - "observability_ext", - # This dependency does not handle untrusted data and is used for various utility purposes. - "other", - # This dependency is used only in tests. - "test_only", -] - -# Components with these use categories are not required to specify the 'cpe' -# and 'last_updated' annotation. -USE_CATEGORIES_WITH_CPE_OPTIONAL = ["build", "other", "test_only"] - -DEPENDENCY_REPOSITORIES_SPEC = dict( +# This should match the schema defined in external_deps.bzl. +REPOSITORY_LOCATIONS_SPEC = dict( bazel_compdb = dict( project_name = "bazel-compilation-database", project_desc = "Clang JSON compilation database support for Bazel", @@ -619,11 +573,11 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( project_name = "LLVM", project_desc = "LLVM Compiler Infrastructure", project_url = "https://llvm.org", - version = "10.0", + version = "10.0.0", sha256 = "df83a44b3a9a71029049ec101fb0077ecbbdf5fe41e395215025779099a98fdf", - strip_prefix = "llvm-{version}.0.src", - urls = ["https://github.com/llvm/llvm-project/releases/download/llvmorg-{version}.0/llvm-{version}.0.src.tar.xz"], - last_updated = "2020-03-24", + strip_prefix = "llvm-{version}.src", + urls = ["https://github.com/llvm/llvm-project/releases/download/llvmorg-{version}/llvm-{version}.src.tar.xz"], + last_updated = "2020-10-09", use_category = ["dataplane_ext"], extensions = [ "envoy.access_loggers.wasm", @@ -632,7 +586,7 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( "envoy.filters.network.wasm", "envoy.stat_sinks.wasm", ], - cpe = "N/A", + cpe = "cpe:2.3:a:llvm:*", ), com_github_wavm_wavm = dict( project_name = "WAVM", @@ -642,7 +596,7 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( sha256 = "cc3fcaf05d57010c9cf8eb920234679dede6c780137b55001fd34e4d14806f7c", strip_prefix = "WAVM-{version}", urls = ["https://github.com/WAVM/WAVM/archive/{version}.tar.gz"], - last_updated = "2020-07-06", + last_updated = "2020-10-09", use_category = ["dataplane_ext"], extensions = [ "envoy.access_loggers.wasm", @@ -651,7 +605,7 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( "envoy.filters.network.wasm", "envoy.stat_sinks.wasm", ], - cpe = "N/A", + cpe = "cpe:2.3:a:webassembly_virtual_machine_project:webassembly_virtual_machine:*", ), io_opencensus_cpp = dict( project_name = "OpenCensus C++", @@ -852,7 +806,6 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( version = "7afb39d868a973caa6216a535c24e37fb666b6f3", sha256 = "213d0b441bcc3df2c87933b24a593b5fd482fa8f4db158b707c60005b9e70040", strip_prefix = "proxy-wasm-cpp-sdk-{version}", - # 2020-09-10 urls = ["https://github.com/proxy-wasm/proxy-wasm-cpp-sdk/archive/{version}.tar.gz"], use_category = ["dataplane_ext"], extensions = [ @@ -862,14 +815,13 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( "envoy.filters.network.wasm", "envoy.stat_sinks.wasm", ], - last_updated = "2020-07-29", + last_updated = "2020-10-09", cpe = "N/A", ), proxy_wasm_cpp_host = dict( project_name = "WebAssembly for Proxies (C++ host implementation)", project_desc = "WebAssembly for Proxies (C++ host implementation)", project_url = "https://github.com/proxy-wasm/proxy-wasm-cpp-host", - # 2020-09-10 version = "49ed20e895b728aae6b811950a2939ecbaf76f7c", sha256 = "fa03293d01450b9164f8f56ef9227301f7d1af4f373f996400f75c93f6ebc822", strip_prefix = "proxy-wasm-cpp-host-{version}", @@ -882,7 +834,7 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( "envoy.filters.network.wasm", "envoy.stat_sinks.wasm", ], - last_updated = "2020-07-29", + last_updated = "2020-10-09", cpe = "N/A", ), # TODO: upgrade to the latest version (1.41 currently fails tests) @@ -890,15 +842,12 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( project_name = "Emscripten SDK", project_desc = "Emscripten SDK (use by Wasm)", project_url = "https://github.com/emscripten-core/emsdk", - version = "1.39", + version = "1.39.6", sha256 = "4ac0f1f3de8b3f1373d435cd7e58bd94de4146e751f099732167749a229b443b", - patch_cmds = [ - "[[ \"$(uname -m)\" == \"x86_64\" ]] && ./emsdk install 1.39.6-upstream && ./emsdk activate --embedded 1.39.6-upstream || true", - ], - strip_prefix = "emsdk-{version}.6", - urls = ["https://github.com/emscripten-core/emsdk/archive/{version}.6.tar.gz"], + strip_prefix = "emsdk-{version}", + urls = ["https://github.com/emscripten-core/emsdk/archive/{version}.tar.gz"], use_category = ["build"], - last_updated = "2020-07-29", + last_updated = "2020-10-09", ), io_bazel_rules_rust = dict( project_name = "Bazel rust rules", @@ -911,7 +860,7 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( strip_prefix = "rules_rust-{version}", urls = ["https://github.com/bazelbuild/rules_rust/archive/{version}.tar.gz"], use_category = ["build"], - last_updated = "2020-07-29", + last_updated = "2020-10-09", ), rules_antlr = dict( project_name = "ANTLR Rules for Bazel", @@ -921,7 +870,7 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( sha256 = "7249d1569293d9b239e23c65f6b4c81a07da921738bde0dfeb231ed98be40429", strip_prefix = "rules_antlr-{version}", urls = ["https://github.com/marcohu/rules_antlr/archive/{version}.tar.gz"], - # This should be "build", but that trips the verification in the docs. + # ANTLR has a runtime component, so is not purely build. use_category = ["dataplane_ext"], extensions = [ "envoy.access_loggers.wasm", @@ -953,23 +902,3 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( cpe = "N/A", ), ) - -def _format_version(s, version): - return s.format(version = version, dash_version = version.replace(".", "-"), underscore_version = version.replace(".", "_")) - -# Interpolate {version} in the above dependency specs. This code should be capable of running in both Python -# and Starlark. -def _dependency_repositories(): - locations = {} - for key, location in DEPENDENCY_REPOSITORIES_SPEC.items(): - mutable_location = dict(location) - locations[key] = mutable_location - - # Fixup with version information. - if "version" in location: - if "strip_prefix" in location: - mutable_location["strip_prefix"] = _format_version(location["strip_prefix"], location["version"]) - mutable_location["urls"] = [_format_version(url, location["version"]) for url in location["urls"]] - return locations - -DEPENDENCY_REPOSITORIES = _dependency_repositories() diff --git a/docs/generate_external_dep_rst.py b/docs/generate_external_dep_rst.py index 7fd4be73824b..3348df538837 100755 --- a/docs/generate_external_dep_rst.py +++ b/docs/generate_external_dep_rst.py @@ -10,13 +10,22 @@ from importlib.util import spec_from_loader, module_from_spec from importlib.machinery import SourceFileLoader -# bazel/repository_locations.bzl must have a .bzl suffix for Starlark import, so + +# Shared Starlark/Python files must have a .bzl suffix for Starlark import, so # we are forced to do this workaround. -_repository_locations_spec = spec_from_loader( - 'repository_locations', - SourceFileLoader('repository_locations', 'bazel/repository_locations.bzl')) -repository_locations = module_from_spec(_repository_locations_spec) -_repository_locations_spec.loader.exec_module(repository_locations) +def LoadModule(name, path): + spec = spec_from_loader(name, SourceFileLoader(name, path)) + module = module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +envoy_repository_locations = LoadModule('envoy_repository_locations', + 'bazel/repository_locations.bzl') +api_repository_locations = LoadModule('api_repository_locations', + 'api/bazel/repository_locations.bzl') +repository_locations_utils = LoadModule('repository_locations_utils', + 'api/bazel/repository_locations_utils.bzl') # Render a CSV table given a list of table headers, widths and list of rows @@ -101,7 +110,10 @@ def GetVersionUrl(metadata): Dep = namedtuple('Dep', ['name', 'sort_name', 'version', 'cpe', 'last_updated']) use_categories = defaultdict(lambda: defaultdict(list)) # Bin rendered dependencies into per-use category lists. - for k, v in repository_locations.DEPENDENCY_REPOSITORIES.items(): + spec_loader = repository_locations_utils.load_repository_locations_spec + spec = spec_loader(envoy_repository_locations.REPOSITORY_LOCATIONS_SPEC) + spec.update(spec_loader(api_repository_locations.REPOSITORY_LOCATIONS_SPEC)) + for k, v in spec.items(): cpe = v.get('cpe', '') if cpe == 'N/A': cpe = '' diff --git a/docs/root/intro/arch_overview/security/external_deps.rst b/docs/root/intro/arch_overview/security/external_deps.rst index e033bb2fc51e..5a0fb0ff5d78 100644 --- a/docs/root/intro/arch_overview/security/external_deps.rst +++ b/docs/root/intro/arch_overview/security/external_deps.rst @@ -21,6 +21,11 @@ Control plane .. include:: external_dep_controlplane.rst +API +--- + +.. include:: external_dep_api.rst + Observability (core) -------------------- diff --git a/generated_api_shadow/bazel/envoy_http_archive.bzl b/generated_api_shadow/bazel/envoy_http_archive.bzl index 13b98f770619..15fd65b2af27 100644 --- a/generated_api_shadow/bazel/envoy_http_archive.bzl +++ b/generated_api_shadow/bazel/envoy_http_archive.bzl @@ -10,8 +10,7 @@ def envoy_http_archive(name, locations, **kwargs): # This repository has already been defined, probably because the user # wants to override the version. Do nothing. return - loc_key = kwargs.pop("repository_key", name) - location = locations[loc_key] + location = locations[name] # HTTP tarball at a given URL. Add a BUILD file if requested. http_archive( diff --git a/generated_api_shadow/bazel/external_deps.bzl b/generated_api_shadow/bazel/external_deps.bzl new file mode 100644 index 000000000000..cd9b6759f98a --- /dev/null +++ b/generated_api_shadow/bazel/external_deps.bzl @@ -0,0 +1,116 @@ +load("@envoy_api//bazel:repository_locations_utils.bzl", "load_repository_locations_spec") + +# Envoy dependencies may be annotated with the following attributes: +DEPENDENCY_ANNOTATIONS = [ + # List of the categories describing how the dependency is being used. This attribute is used + # for automatic tracking of security posture of Envoy's dependencies. + # Possible values are documented in the USE_CATEGORIES list below. + # This attribute is mandatory for each dependecy. + "use_category", + + # Attribute specifying CPE (Common Platform Enumeration, see https://nvd.nist.gov/products/cpe) ID + # of the dependency. The ID may be in v2.3 or v2.2 format, although v2.3 is prefferred. See + # https://nvd.nist.gov/products/cpe for CPE format. Use single wildcard '*' for version and vector elements + # i.e. 'cpe:2.3:a:nghttp2:nghttp2:*'. Use "N/A" for dependencies without CPE assigned. + # This attribute is optional for components with use categories listed in the + # USE_CATEGORIES_WITH_CPE_OPTIONAL + "cpe", +] + +# NOTE: If a dependency use case is either dataplane or controlplane, the other uses are not needed +# to be declared. +USE_CATEGORIES = [ + # This dependency is used in API protos. + "api", + # This dependency is used in build process. + "build", + # This dependency is used to process xDS requests. + "controlplane", + # This dependency is used in processing downstream or upstream requests (core). + "dataplane_core", + # This dependency is used in processing downstream or upstream requests (extensions). + "dataplane_ext", + # This dependecy is used for logging, metrics or tracing (core). It may process unstrusted input. + "observability_core", + # This dependecy is used for logging, metrics or tracing (extensions). It may process unstrusted input. + "observability_ext", + # This dependency does not handle untrusted data and is used for various utility purposes. + "other", + # This dependency is used only in tests. + "test_only", +] + +# Components with these use categories are not required to specify the 'cpe' +# and 'last_updated' annotation. +USE_CATEGORIES_WITH_CPE_OPTIONAL = ["build", "other", "test_only", "api"] + +def _fail_missing_attribute(attr, key): + fail("The '%s' attribute must be defined for external dependecy " % attr + key) + +# Method for verifying content of the repository location specifications. +# +# We also remove repository metadata attributes so that further consumers, e.g. +# http_archive, are not confused by them. +def load_repository_locations(repository_locations_spec): + locations = {} + for key, location in load_repository_locations_spec(repository_locations_spec).items(): + mutable_location = dict(location) + locations[key] = mutable_location + + if "sha256" not in location or len(location["sha256"]) == 0: + _fail_missing_attribute("sha256", key) + + if "project_name" not in location: + _fail_missing_attribute("project_name", key) + mutable_location.pop("project_name") + + if "project_desc" not in location: + _fail_missing_attribute("project_desc", key) + mutable_location.pop("project_desc") + + if "project_url" not in location: + _fail_missing_attribute("project_url", key) + project_url = mutable_location.pop("project_url") + if not project_url.startswith("https://") and not project_url.startswith("http://"): + fail("project_url must start with https:// or http://: " + project_url) + + if "version" not in location: + _fail_missing_attribute("version", key) + mutable_location.pop("version") + + if "use_category" not in location: + _fail_missing_attribute("use_category", key) + use_category = mutable_location.pop("use_category") + + if "dataplane_ext" in use_category or "observability_ext" in use_category: + if "extensions" not in location: + _fail_missing_attribute("extensions", key) + mutable_location.pop("extensions") + + if "last_updated" not in location: + _fail_missing_attribute("last_updated", key) + last_updated = mutable_location.pop("last_updated") + + # Starlark doesn't have regexes. + if len(last_updated) != 10 or last_updated[4] != "-" or last_updated[7] != "-": + fail("last_updated must match YYYY-DD-MM: " + last_updated) + + if "cpe" in location: + cpe = mutable_location.pop("cpe") + + # Starlark doesn't have regexes. + cpe_components = len(cpe.split(":")) + + # We allow cpe:2.3:a:foo:* and cpe:2.3.:a:foo:bar:* only. + cpe_components_valid = cpe_components in [5, 6] + cpe_matches = (cpe == "N/A" or (cpe.startswith("cpe:2.3:a:") and cpe.endswith(":*") and cpe_components_valid)) + if not cpe_matches: + fail("CPE must match cpe:2.3:a:::*: " + cpe) + elif not [category for category in USE_CATEGORIES_WITH_CPE_OPTIONAL if category in location["use_category"]]: + _fail_missing_attribute("cpe", key) + + for category in location["use_category"]: + if category not in USE_CATEGORIES: + fail("Unknown use_category value '" + category + "' for dependecy " + key) + + return locations diff --git a/generated_api_shadow/bazel/repositories.bzl b/generated_api_shadow/bazel/repositories.bzl index a64e733cf74a..a12a0ea98b3a 100644 --- a/generated_api_shadow/bazel/repositories.bzl +++ b/generated_api_shadow/bazel/repositories.bzl @@ -1,40 +1,43 @@ load(":envoy_http_archive.bzl", "envoy_http_archive") -load(":repository_locations.bzl", "REPOSITORY_LOCATIONS") +load(":external_deps.bzl", "load_repository_locations") +load(":repository_locations.bzl", "REPOSITORY_LOCATIONS_SPEC") -def api_dependencies(): +REPOSITORY_LOCATIONS = load_repository_locations(REPOSITORY_LOCATIONS_SPEC) + +# Use this macro to reference any HTTP archive from bazel/repository_locations.bzl. +def external_http_archive(name, **kwargs): envoy_http_archive( - "bazel_skylib", + name, locations = REPOSITORY_LOCATIONS, + **kwargs ) - envoy_http_archive( - "com_envoyproxy_protoc_gen_validate", - locations = REPOSITORY_LOCATIONS, + +def api_dependencies(): + external_http_archive( + name = "bazel_skylib", ) - envoy_http_archive( + external_http_archive( + name = "com_envoyproxy_protoc_gen_validate", + ) + external_http_archive( name = "com_google_googleapis", - locations = REPOSITORY_LOCATIONS, ) - envoy_http_archive( + external_http_archive( name = "com_github_cncf_udpa", - locations = REPOSITORY_LOCATIONS, ) - envoy_http_archive( + external_http_archive( name = "prometheus_metrics_model", - locations = REPOSITORY_LOCATIONS, build_file_content = PROMETHEUSMETRICS_BUILD_CONTENT, ) - envoy_http_archive( + external_http_archive( name = "opencensus_proto", - locations = REPOSITORY_LOCATIONS, ) - envoy_http_archive( + external_http_archive( name = "rules_proto", - locations = REPOSITORY_LOCATIONS, ) - envoy_http_archive( + external_http_archive( name = "com_github_openzipkin_zipkinapi", - locations = REPOSITORY_LOCATIONS, build_file_content = ZIPKINAPI_BUILD_CONTENT, ) diff --git a/generated_api_shadow/bazel/repository_locations.bzl b/generated_api_shadow/bazel/repository_locations.bzl index 152d4be5ee0d..bdcf31e867d2 100644 --- a/generated_api_shadow/bazel/repository_locations.bzl +++ b/generated_api_shadow/bazel/repository_locations.bzl @@ -1,4 +1,5 @@ -DEPENDENCY_REPOSITORIES_SPEC = dict( +# This should match the schema defined in external_deps.bzl. +REPOSITORY_LOCATIONS_SPEC = dict( bazel_skylib = dict( project_name = "bazel-skylib", project_desc = "Common useful functions and rules for Bazel", @@ -88,23 +89,3 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( use_category = ["api"], ), ) - -def _format_version(s, version): - return s.format(version = version, dash_version = version.replace(".", "-"), underscore_version = version.replace(".", "_")) - -# Interpolate {version} in the above dependency specs. This code should be capable of running in both Python -# and Starlark. -def _dependency_repositories(): - locations = {} - for key, location in DEPENDENCY_REPOSITORIES_SPEC.items(): - mutable_location = dict(location) - locations[key] = mutable_location - - # Fixup with version information. - if "version" in location: - if "strip_prefix" in location: - mutable_location["strip_prefix"] = _format_version(location["strip_prefix"], location["version"]) - mutable_location["urls"] = [_format_version(url, location["version"]) for url in location["urls"]] - return locations - -REPOSITORY_LOCATIONS = _dependency_repositories() diff --git a/generated_api_shadow/bazel/repository_locations_utils.bzl b/generated_api_shadow/bazel/repository_locations_utils.bzl new file mode 100644 index 000000000000..3b984e1bc580 --- /dev/null +++ b/generated_api_shadow/bazel/repository_locations_utils.bzl @@ -0,0 +1,20 @@ +def _format_version(s, version): + return s.format(version = version, dash_version = version.replace(".", "-"), underscore_version = version.replace(".", "_")) + +# Generate a "repository location specification" from raw repository +# specification. The information should match the format required by +# external_deps.bzl. This function mostly does interpolation of {version} in +# the repository info fields. This code should be capable of running in both +# Python and Starlark. +def load_repository_locations_spec(repository_locations_spec): + locations = {} + for key, location in repository_locations_spec.items(): + mutable_location = dict(location) + locations[key] = mutable_location + + # Fixup with version information. + if "version" in location: + if "strip_prefix" in location: + mutable_location["strip_prefix"] = _format_version(location["strip_prefix"], location["version"]) + mutable_location["urls"] = [_format_version(url, location["version"]) for url in location["urls"]] + return locations diff --git a/tools/dependency/validate.py b/tools/dependency/validate.py index 273c3117918c..813d9bd90787 100755 --- a/tools/dependency/validate.py +++ b/tools/dependency/validate.py @@ -59,7 +59,7 @@ def DepsByUseCategory(self, use_category): Returns: Set of dependency identifiers that match use_category. """ - return set(name for name, metadata in repository_locations.DEPENDENCY_REPOSITORIES.items() + return set(name for name, metadata in repository_locations.REPOSITORY_LOCATIONS_SPEC.items() if use_category in metadata['use_category']) def GetMetadata(self, dependency): @@ -72,7 +72,7 @@ def GetMetadata(self, dependency): A dictionary with the repository metadata as defined in bazel/repository_locations.bzl. """ - return repository_locations.DEPENDENCY_REPOSITORIES.get(dependency) + return repository_locations.REPOSITORY_LOCATIONS_SPEC.get(dependency) class BuildGraph(object): From 55fb16dcd1ced50b9422ebf1b2bdfa9480cdc511 Mon Sep 17 00:00:00 2001 From: Denis Zaitcev Date: Tue, 13 Oct 2020 15:45:08 +0100 Subject: [PATCH 03/14] Fix runtime feature variable name (#13533) - change the runtime feature var name from `envoy.reloadable_features.header_match_on_all_headers` to `envoy.reloadable_features.http_match_on_all_headers` as this is the actual variable name defined in https://github.com/envoyproxy/envoy/blob/master/source/common/runtime/runtime_features.cc Signed-off-by: Denis Zaitcev --- docs/root/version_history/v1.12.7.rst | 4 ++-- docs/root/version_history/v1.13.5.rst | 4 ++-- docs/root/version_history/v1.14.5.rst | 4 ++-- docs/root/version_history/v1.15.1.rst | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/root/version_history/v1.12.7.rst b/docs/root/version_history/v1.12.7.rst index ea298c22b15f..875e2d683d1c 100644 --- a/docs/root/version_history/v1.12.7.rst +++ b/docs/root/version_history/v1.12.7.rst @@ -6,7 +6,7 @@ Changes changes the default behavior to always logically match on all headers. Multiple individual headers will be logically concatenated with ',' similar to what is done with inline headers. This makes the behavior effectively consistent. This behavior can be temporary reverted by setting - the runtime value "envoy.reloadable_features.header_match_on_all_headers" to "false". + the runtime value "envoy.reloadable_features.http_match_on_all_headers" to "false". Targeted fixes have been additionally performed on the following extensions which make them consider all duplicate headers by default as a comma concatenated list: @@ -17,4 +17,4 @@ Changes 4. The Lua filter. Like primary header matching used in routing, RBAC, etc. this behavior can be disabled by setting - the runtime value "envoy.reloadable_features.header_match_on_all_headers" to false. \ No newline at end of file + the runtime value "envoy.reloadable_features.http_match_on_all_headers" to false. \ No newline at end of file diff --git a/docs/root/version_history/v1.13.5.rst b/docs/root/version_history/v1.13.5.rst index 8e0c9d8001ed..370d9b4f376c 100644 --- a/docs/root/version_history/v1.13.5.rst +++ b/docs/root/version_history/v1.13.5.rst @@ -6,7 +6,7 @@ Changes changes the default behavior to always logically match on all headers. Multiple individual headers will be logically concatenated with ',' similar to what is done with inline headers. This makes the behavior effectively consistent. This behavior can be temporary reverted by setting - the runtime value "envoy.reloadable_features.header_match_on_all_headers" to "false". + the runtime value "envoy.reloadable_features.http_match_on_all_headers" to "false". Targeted fixes have been additionally performed on the following extensions which make them consider all duplicate headers by default as a comma concatenated list: @@ -17,7 +17,7 @@ Changes 4. The Lua filter. Like primary header matching used in routing, RBAC, etc. this behavior can be disabled by setting - the runtime value "envoy.reloadable_features.header_match_on_all_headers" to false. + the runtime value "envoy.reloadable_features.http_match_on_all_headers" to false. * http: fixed CVE-2020-25017. The setCopy() header map API previously only set the first header in the case of duplicate non-inline headers. setCopy() now behaves similarly to the other set*() APIs and replaces all found headers with a single value. This may have had security implications in the extauth filter which diff --git a/docs/root/version_history/v1.14.5.rst b/docs/root/version_history/v1.14.5.rst index aa0e6233d63f..b252c2ac235f 100644 --- a/docs/root/version_history/v1.14.5.rst +++ b/docs/root/version_history/v1.14.5.rst @@ -6,7 +6,7 @@ Changes This patch changes the default behavior to always logically match on all headers. Multiple individual headers will be logically concatenated with ',' similar to what is done with inline headers. This makes the behavior effectively consistent. This behavior can be temporary reverted by setting - the runtime value "envoy.reloadable_features.header_match_on_all_headers" to "false". + the runtime value "envoy.reloadable_features.http_match_on_all_headers" to "false". Targeted fixes have been additionally performed on the following extensions which make them consider all duplicate headers by default as a comma concatenated list: @@ -17,7 +17,7 @@ Changes 4. The Lua filter. Like primary header matching used in routing, RBAC, etc. this behavior can be disabled by setting - the runtime value "envoy.reloadable_features.header_match_on_all_headers" to false. + the runtime value "envoy.reloadable_features.http_match_on_all_headers" to false. * http: fixed CVE-2020-25017. The setCopy() header map API previously only set the first header in the case of duplicate non-inline headers. setCopy() now behaves similarly to the other set*() APIs and replaces all found headers with a single value. This may have had security implications in the extauth filter which diff --git a/docs/root/version_history/v1.15.1.rst b/docs/root/version_history/v1.15.1.rst index e19e6331416c..4bdb55a943cf 100644 --- a/docs/root/version_history/v1.15.1.rst +++ b/docs/root/version_history/v1.15.1.rst @@ -7,7 +7,7 @@ Changes headers. This patch changes the default behavior to always logically match on all headers. Multiple individual headers will be logically concatenated with ',' similar to what is done with inline headers. This makes the behavior effectively consistent. This behavior can be temporary - reverted by setting the runtime value "envoy.reloadable_features.header_match_on_all_headers" to + reverted by setting the runtime value "envoy.reloadable_features.http_match_on_all_headers" to "false". Targeted fixes have been additionally performed on the following extensions which make them @@ -19,7 +19,7 @@ Changes 4. The Lua filter. Like primary header matching used in routing, RBAC, etc. this behavior can be disabled by setting - the runtime value "envoy.reloadable_features.header_match_on_all_headers" to false. + the runtime value "envoy.reloadable_features.http_match_on_all_headers" to false. * http: The setCopy() header map API previously only set the first header in the case of duplicate non-inline headers. setCopy() now behaves similarly to the other set*() APIs and replaces all found headers with a single value. This may have had security implications in the extauth filter which From 366fb79757f1947d56702a62e46c75e7da282bb7 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 13 Oct 2020 10:57:09 -0400 Subject: [PATCH 04/14] Convert overload manager config literals to YAML (#13518) YAML is better documented and more widely understood than textproto. Signed-off-by: Alex Konradi --- test/server/overload_manager_impl_test.cc | 201 +++++++++------------- 1 file changed, 77 insertions(+), 124 deletions(-) diff --git a/test/server/overload_manager_impl_test.cc b/test/server/overload_manager_impl_test.cc index 5dbbd0c88baa..8d730c656d1f 100644 --- a/test/server/overload_manager_impl_test.cc +++ b/test/server/overload_manager_impl_test.cc @@ -119,60 +119,10 @@ class OverloadManagerImplTest : public testing::Test { envoy::config::overload::v3::OverloadManager parseConfig(const std::string& config) { envoy::config::overload::v3::OverloadManager proto; - bool success = Protobuf::TextFormat::ParseFromString(config, &proto); - ASSERT(success); + TestUtility::loadFromYaml(config, proto); return proto; } - std::string getConfig() { - return R"EOF( - refresh_interval { - seconds: 1 - } - resource_monitors { - name: "envoy.resource_monitors.fake_resource1" - } - resource_monitors { - name: "envoy.resource_monitors.fake_resource2" - } - resource_monitors { - name: "envoy.resource_monitors.fake_resource3" - } - resource_monitors { - name: "envoy.resource_monitors.fake_resource4" - } - actions { - name: "envoy.overload_actions.dummy_action" - triggers { - name: "envoy.resource_monitors.fake_resource1" - threshold { - value: 0.9 - } - } - triggers { - name: "envoy.resource_monitors.fake_resource2" - threshold { - value: 0.8 - } - } - triggers { - name: "envoy.resource_monitors.fake_resource3" - scaled { - scaling_threshold: 0.5 - saturation_threshold: 0.8 - } - } - triggers { - name: "envoy.resource_monitors.fake_resource4" - scaled { - scaling_threshold: 0.5 - saturation_threshold: 0.8 - } - } - } - )EOF"; - } - std::unique_ptr createOverloadManager(const std::string& config) { return std::make_unique(dispatcher_, stats_, thread_local_, parseConfig(config), validation_visitor_, *api_); @@ -195,10 +145,37 @@ class OverloadManagerImplTest : public testing::Test { Api::ApiPtr api_; }; +constexpr char kRegularStateConfig[] = R"YAML( + refresh_interval: + seconds: 1 + resource_monitors: + - name: envoy.resource_monitors.fake_resource1 + - name: envoy.resource_monitors.fake_resource2 + - name: envoy.resource_monitors.fake_resource3 + - name: envoy.resource_monitors.fake_resource4 + actions: + - name: envoy.overload_actions.dummy_action + triggers: + - name: envoy.resource_monitors.fake_resource1 + threshold: + value: 0.9 + - name: envoy.resource_monitors.fake_resource2 + threshold: + value: 0.8 + - name: envoy.resource_monitors.fake_resource3 + scaled: + scaling_threshold: 0.5 + saturation_threshold: 0.8 + - name: envoy.resource_monitors.fake_resource4 + scaled: + scaling_threshold: 0.5 + saturation_threshold: 0.8 +)YAML"; + TEST_F(OverloadManagerImplTest, CallbackOnlyFiresWhenStateChanges) { setDispatcherExpectation(); - auto manager(createOverloadManager(getConfig())); + auto manager(createOverloadManager(kRegularStateConfig)); bool is_active = false; int cb_count = 0; manager->registerForAction("envoy.overload_actions.dummy_action", dispatcher_, @@ -306,7 +283,7 @@ TEST_F(OverloadManagerImplTest, CallbackOnlyFiresWhenStateChanges) { TEST_F(OverloadManagerImplTest, ScaledTrigger) { setDispatcherExpectation(); - auto manager(createOverloadManager(getConfig())); + auto manager(createOverloadManager(kRegularStateConfig)); manager->start(); const auto& action_state = manager->getThreadLocalOverloadState().getState("envoy.overload_actions.dummy_action"); @@ -350,7 +327,7 @@ TEST_F(OverloadManagerImplTest, ScaledTrigger) { TEST_F(OverloadManagerImplTest, FailedUpdates) { setDispatcherExpectation(); - auto manager(createOverloadManager(getConfig())); + auto manager(createOverloadManager(kRegularStateConfig)); manager->start(); Stats::Counter& failed_updates = stats_.counter("overload.envoy.resource_monitors.fake_resource1.failed_updates"); @@ -366,7 +343,7 @@ TEST_F(OverloadManagerImplTest, FailedUpdates) { TEST_F(OverloadManagerImplTest, AggregatesMultipleResourceUpdates) { setDispatcherExpectation(); - auto manager(createOverloadManager(getConfig())); + auto manager(createOverloadManager(kRegularStateConfig)); manager->start(); const OverloadActionState& action_state = @@ -388,7 +365,7 @@ TEST_F(OverloadManagerImplTest, AggregatesMultipleResourceUpdates) { TEST_F(OverloadManagerImplTest, DelayedUpdatesAreCoalesced) { setDispatcherExpectation(); - auto manager(createOverloadManager(getConfig())); + auto manager(createOverloadManager(kRegularStateConfig)); manager->start(); const OverloadActionState& action_state = @@ -412,7 +389,7 @@ TEST_F(OverloadManagerImplTest, DelayedUpdatesAreCoalesced) { TEST_F(OverloadManagerImplTest, FlushesUpdatesEvenWithOneUnresponsive) { setDispatcherExpectation(); - auto manager(createOverloadManager(getConfig())); + auto manager(createOverloadManager(kRegularStateConfig)); manager->start(); const OverloadActionState& action_state = @@ -436,7 +413,7 @@ TEST_F(OverloadManagerImplTest, FlushesUpdatesEvenWithOneUnresponsive) { TEST_F(OverloadManagerImplTest, SkippedUpdates) { setDispatcherExpectation(); - auto manager(createOverloadManager(getConfig())); + auto manager(createOverloadManager(kRegularStateConfig)); manager->start(); Stats::Counter& skipped_updates = stats_.counter("overload.envoy.resource_monitors.fake_resource1.skipped_updates"); @@ -466,12 +443,9 @@ TEST_F(OverloadManagerImplTest, SkippedUpdates) { TEST_F(OverloadManagerImplTest, DuplicateResourceMonitor) { const std::string config = R"EOF( - resource_monitors { - name: "envoy.resource_monitors.fake_resource1" - } - resource_monitors { - name: "envoy.resource_monitors.fake_resource1" - } + resource_monitors: + - name: "envoy.resource_monitors.fake_resource1" + - name: "envoy.resource_monitors.fake_resource1" )EOF"; EXPECT_THROW_WITH_REGEX(createOverloadManager(config), EnvoyException, @@ -480,12 +454,9 @@ TEST_F(OverloadManagerImplTest, DuplicateResourceMonitor) { TEST_F(OverloadManagerImplTest, DuplicateOverloadAction) { const std::string config = R"EOF( - actions { - name: "envoy.overload_actions.dummy_action" - } - actions { - name: "envoy.overload_actions.dummy_action" - } + actions: + - name: "envoy.overload_actions.dummy_action" + - name: "envoy.overload_actions.dummy_action" )EOF"; EXPECT_THROW_WITH_REGEX(createOverloadManager(config), EnvoyException, @@ -495,19 +466,15 @@ TEST_F(OverloadManagerImplTest, DuplicateOverloadAction) { // A scaled trigger action's thresholds must conform to scaling < saturation. TEST_F(OverloadManagerImplTest, ScaledTriggerSaturationLessThanScalingThreshold) { const std::string config = R"EOF( - resource_monitors { - name: "envoy.resource_monitors.fake_resource1" - } - actions { - name: "envoy.overload_actions.dummy_action" - triggers { - name: "envoy.resource_monitors.fake_resource1" - scaled { - scaling_threshold: 0.9 - saturation_threshold: 0.8 - } - } - } + resource_monitors: + - name: "envoy.resource_monitors.fake_resource1" + actions: + - name: "envoy.overload_actions.dummy_action" + triggers: + - name: "envoy.resource_monitors.fake_resource1" + scaled: + scaling_threshold: 0.9 + saturation_threshold: 0.8 )EOF"; EXPECT_THROW_WITH_REGEX(createOverloadManager(config), EnvoyException, @@ -517,19 +484,15 @@ TEST_F(OverloadManagerImplTest, ScaledTriggerSaturationLessThanScalingThreshold) // A scaled trigger action can't have threshold values that are equal. TEST_F(OverloadManagerImplTest, ScaledTriggerThresholdsEqual) { const std::string config = R"EOF( - resource_monitors { - name: "envoy.resource_monitors.fake_resource1" - } - actions { - name: "envoy.overload_actions.dummy_action" - triggers { - name: "envoy.resource_monitors.fake_resource1" - scaled { - scaling_threshold: 0.9 - saturation_threshold: 0.9 - } - } - } + resource_monitors: + - name: "envoy.resource_monitors.fake_resource1" + actions: + - name: "envoy.overload_actions.dummy_action" + triggers: + - name: "envoy.resource_monitors.fake_resource1" + scaled: + scaling_threshold: 0.9 + saturation_threshold: 0.9 )EOF"; EXPECT_THROW_WITH_REGEX(createOverloadManager(config), EnvoyException, @@ -538,15 +501,12 @@ TEST_F(OverloadManagerImplTest, ScaledTriggerThresholdsEqual) { TEST_F(OverloadManagerImplTest, UnknownTrigger) { const std::string config = R"EOF( - actions { - name: "envoy.overload_actions.dummy_action" - triggers { - name: "envoy.resource_monitors.fake_resource1" - threshold { - value: 0.9 - } - } - } + actions: + - name: "envoy.overload_actions.dummy_action" + triggers: + - name: "envoy.resource_monitors.fake_resource1" + threshold: + value: 0.9 )EOF"; EXPECT_THROW_WITH_REGEX(createOverloadManager(config), EnvoyException, @@ -555,24 +515,17 @@ TEST_F(OverloadManagerImplTest, UnknownTrigger) { TEST_F(OverloadManagerImplTest, DuplicateTrigger) { const std::string config = R"EOF( - resource_monitors { - name: "envoy.resource_monitors.fake_resource1" - } - actions { - name: "envoy.overload_actions.dummy_action" - triggers { - name: "envoy.resource_monitors.fake_resource1" - threshold { - value: 0.9 - } - } - triggers { - name: "envoy.resource_monitors.fake_resource1" - threshold { - value: 0.8 - } - } - } + resource_monitors: + - name: "envoy.resource_monitors.fake_resource1" + actions: + - name: "envoy.overload_actions.dummy_action" + triggers: + - name: "envoy.resource_monitors.fake_resource1" + threshold: + value: 0.9 + - name: "envoy.resource_monitors.fake_resource1" + threshold: + value: 0.8 )EOF"; EXPECT_THROW_WITH_REGEX(createOverloadManager(config), EnvoyException, "Duplicate trigger .*"); @@ -581,7 +534,7 @@ TEST_F(OverloadManagerImplTest, DuplicateTrigger) { TEST_F(OverloadManagerImplTest, Shutdown) { setDispatcherExpectation(); - auto manager(createOverloadManager(getConfig())); + auto manager(createOverloadManager(kRegularStateConfig)); manager->start(); EXPECT_CALL(*timer_, disableTimer()); From 21c827cefab4fbefd42d05d14e8feb9de65b4b8d Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 13 Oct 2020 10:58:15 -0400 Subject: [PATCH 05/14] Prevent SEGFAULT when disabling listener (#13515) This prevents the stop_listening overload action from causing segmentation faults that can occur if the action is enabled after the listener has already shut down. Signed-off-by: Alex Konradi --- source/server/connection_handler_impl.cc | 12 ++++++++++++ source/server/connection_handler_impl.h | 4 ++-- test/server/connection_handler_test.cc | 16 ++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 0ca74c60c428..fd5586fb78cd 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -423,6 +423,18 @@ void ConnectionHandlerImpl::ActiveTcpListener::onAcceptWorker( } } +void ConnectionHandlerImpl::ActiveTcpListener::pauseListening() { + if (listener_ != nullptr) { + listener_->disable(); + } +} + +void ConnectionHandlerImpl::ActiveTcpListener::resumeListening() { + if (listener_ != nullptr) { + listener_->enable(); + } +} + void ConnectionHandlerImpl::ActiveTcpListener::newConnection( Network::ConnectionSocketPtr&& socket, std::unique_ptr stream_info) { // Find matching filter chain. diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 3eb3e8e58c35..62ddf0c11be7 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -137,8 +137,8 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, // ActiveListenerImplBase Network::Listener* listener() override { return listener_.get(); } - void pauseListening() override { listener_->disable(); } - void resumeListening() override { listener_->enable(); } + void pauseListening() override; + void resumeListening() override; void shutdownListener() override { listener_.reset(); } // Network::BalancedConnectionHandler diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index 47d58f94e256..dbb6a5698e43 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -451,6 +451,22 @@ TEST_F(ConnectionHandlerTest, AddDisabledListener) { handler_->addListener(absl::nullopt, *test_listener); } +TEST_F(ConnectionHandlerTest, DisableListenerAfterStop) { + InSequence s; + + Network::TcpListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, false, false, "test_listener", listener, &listener_callbacks); + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + handler_->addListener(absl::nullopt, *test_listener); + + EXPECT_CALL(*listener, onDestroy()); + + handler_->stopListeners(); + handler_->disableListeners(); +} + TEST_F(ConnectionHandlerTest, DestroyCloseConnections) { InSequence s; From cb7691cb6dde2014b86d05e932fcbc32f4c48eca Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 13 Oct 2020 11:13:04 -0400 Subject: [PATCH 06/14] conn_pool: fixing comments (#13520) Risk Level: n/a (comment + rename) Testing: n/a Docs Changes: n/a Release Notes: n/a Fixes #13369 Signed-off-by: Alyssa Wilk --- include/envoy/common/conn_pool.h | 4 ++-- source/common/conn_pool/conn_pool_base.cc | 6 +++--- source/common/conn_pool/conn_pool_base.h | 5 +++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/include/envoy/common/conn_pool.h b/include/envoy/common/conn_pool.h index 619ec9b0c9e7..c80e80db21ed 100644 --- a/include/envoy/common/conn_pool.h +++ b/include/envoy/common/conn_pool.h @@ -50,8 +50,8 @@ class Instance { using DrainedCb = std::function; /** - * Register a callback that gets called when the connection pool is fully drained. No actual - * draining is done. The owner of the connection pool is responsible for not creating any + * Register a callback that gets called when the connection pool is fully drained and kicks + * off a drain. The owner of the connection pool is responsible for not creating any * new streams. */ virtual void addDrainedCallback(DrainedCb cb) PURE; diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index eb8f1f0e3859..bc5293e99318 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -246,7 +246,7 @@ void ConnPoolImplBase::addDrainedCallbackImpl(Instance::DrainedCb cb) { checkForDrained(); } -void ConnPoolImplBase::closeIdleConnections() { +void ConnPoolImplBase::closeIdleConnectionsForDrainingPool() { // Create a separate list of elements to close to avoid mutate-while-iterating problems. std::list to_close; @@ -268,7 +268,7 @@ void ConnPoolImplBase::closeIdleConnections() { } void ConnPoolImplBase::drainConnectionsImpl() { - closeIdleConnections(); + closeIdleConnectionsForDrainingPool(); // closeIdleConnections() closes all connections in ready_clients_ with no active streams, // so all remaining entries in ready_clients_ are serving streams. Move them and all entries @@ -290,7 +290,7 @@ void ConnPoolImplBase::checkForDrained() { return; } - closeIdleConnections(); + closeIdleConnectionsForDrainingPool(); if (pending_streams_.empty() && ready_clients_.empty() && busy_clients_.empty() && connecting_clients_.empty()) { diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index c258fafa7bf5..85d8f304719d 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -138,14 +138,15 @@ class ConnPoolImplBase : protected Logger::Loggable { absl::string_view failure_reason, ConnectionPool::PoolFailureReason pool_failure_reason); - // Closes any idle connections. - void closeIdleConnections(); + // Closes any idle connections as this pool is drained. + void closeIdleConnectionsForDrainingPool(); // Changes the state_ of an ActiveClient and moves to the appropriate list. void transitionActiveClientState(ActiveClient& client, ActiveClient::State new_state); void onConnectionEvent(ActiveClient& client, absl::string_view failure_reason, Network::ConnectionEvent event); + // See if the drain process has started and/or completed. void checkForDrained(); void onUpstreamReady(); ConnectionPool::Cancellable* newStream(AttachContext& context); From 82e7109c73799197075654808f05c3aab33d2f2b Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 13 Oct 2020 14:42:52 -0400 Subject: [PATCH 07/14] tcp: towards pluggable upstreams (#13331) Commit Message: Refactoring TCP code to match HTTP code in preparation for pluggable TCP upstreams. Risk Level: High - fairly major refactor Testing: existing tests pass Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk --- source/common/tcp_proxy/tcp_proxy.cc | 95 +++++++++------------------ source/common/tcp_proxy/tcp_proxy.h | 36 +++++------ source/common/tcp_proxy/upstream.cc | 96 ++++++++++++++++++++++++++++ source/common/tcp_proxy/upstream.h | 89 ++++++++++++++++++++------ 4 files changed, 212 insertions(+), 104 deletions(-) diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 5ca0ea661d83..3688634f5305 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -225,7 +225,7 @@ Filter::~Filter() { access_log->log(nullptr, nullptr, nullptr, getStreamInfo()); } - ASSERT(upstream_handle_ == nullptr); + ASSERT(generic_conn_pool_ == nullptr); ASSERT(upstream_ == nullptr); } @@ -442,24 +442,17 @@ Network::FilterStatus Filter::initializeUpstreamConnection() { bool Filter::maybeTunnel(const std::string& cluster_name) { if (!config_->tunnelingConfig()) { - Tcp::ConnectionPool::Instance* conn_pool = cluster_manager_.tcpConnPoolForCluster( - cluster_name, Upstream::ResourcePriority::Default, this); - if (conn_pool) { + generic_conn_pool_ = + std::make_unique(cluster_name, cluster_manager_, this, *upstream_callbacks_); + if (generic_conn_pool_->valid()) { connecting_ = true; connect_attempts_++; - - // Given this function is reentrant, make sure we only reset the upstream_handle_ if given a - // valid connection handle. If newConnection fails inline it may result in attempting to - // select a new host, and a recursive call to initializeUpstreamConnection. In this case the - // first call to newConnection will return null and the inner call will persist. - Tcp::ConnectionPool::Cancellable* handle = conn_pool->newConnection(*this); - if (handle) { - ASSERT(upstream_handle_.get() == nullptr); - upstream_handle_ = std::make_shared(handle); - } + generic_conn_pool_->newStream(this); // Because we never return open connections to the pool, this either has a handle waiting on // connection completion, or onPoolFailure has been invoked. Either way, stop iteration. return true; + } else { + generic_conn_pool_.reset(); } } else { auto* cluster = cluster_manager_.get(cluster_name); @@ -474,28 +467,23 @@ bool Filter::maybeTunnel(const std::string& cluster_name) { "http2_protocol_options on the cluster."); return false; } - Http::ConnectionPool::Instance* conn_pool = cluster_manager_.httpConnPoolForCluster( - cluster_name, Upstream::ResourcePriority::Default, absl::nullopt, this); - if (conn_pool) { - upstream_ = std::make_unique(*upstream_callbacks_, - config_->tunnelingConfig()->hostname()); - HttpUpstream* http_upstream = static_cast(upstream_.get()); - Http::ConnectionPool::Cancellable* cancellable = - conn_pool->newStream(http_upstream->responseDecoder(), *this); - if (cancellable) { - ASSERT(upstream_handle_.get() == nullptr); - upstream_handle_ = std::make_shared(cancellable); - } + + generic_conn_pool_ = std::make_unique(cluster_name, cluster_manager_, this, + config_->tunnelingConfig()->hostname(), + *upstream_callbacks_); + if (generic_conn_pool_->valid()) { + generic_conn_pool_->newStream(this); return true; + } else { + generic_conn_pool_.reset(); } } return false; } -void Filter::onPoolFailure(ConnectionPool::PoolFailureReason reason, - Upstream::HostDescriptionConstSharedPtr host) { - upstream_handle_.reset(); - +void Filter::onGenericPoolFailure(ConnectionPool::PoolFailureReason reason, + Upstream::HostDescriptionConstSharedPtr host) { + generic_conn_pool_.reset(); read_callbacks_->upstreamHost(host); getStreamInfo().onUpstreamHostSelected(host); @@ -518,44 +506,22 @@ void Filter::onPoolFailure(ConnectionPool::PoolFailureReason reason, } } -void Filter::onPoolReadyBase(Upstream::HostDescriptionConstSharedPtr& host, - const Network::Address::InstanceConstSharedPtr& local_address, - Ssl::ConnectionInfoConstSharedPtr ssl_info) { - upstream_handle_.reset(); +void Filter::onGenericPoolReady(StreamInfo::StreamInfo* info, + std::unique_ptr&& upstream, + Upstream::HostDescriptionConstSharedPtr& host, + const Network::Address::InstanceConstSharedPtr& local_address, + Ssl::ConnectionInfoConstSharedPtr ssl_info) { + upstream_ = std::move(upstream); + generic_conn_pool_.reset(); read_callbacks_->upstreamHost(host); getStreamInfo().onUpstreamHostSelected(host); getStreamInfo().setUpstreamLocalAddress(local_address); getStreamInfo().setUpstreamSslConnection(ssl_info); onUpstreamConnection(); read_callbacks_->continueReading(); -} - -void Filter::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, - Upstream::HostDescriptionConstSharedPtr host) { - Tcp::ConnectionPool::ConnectionData* latched_data = conn_data.get(); - - upstream_ = std::make_unique(std::move(conn_data), *upstream_callbacks_); - onPoolReadyBase(host, latched_data->connection().localAddress(), - latched_data->connection().streamInfo().downstreamSslConnection()); - read_callbacks_->connection().streamInfo().setUpstreamFilterState( - latched_data->connection().streamInfo().filterState()); -} - -void Filter::onPoolFailure(ConnectionPool::PoolFailureReason failure, absl::string_view, - Upstream::HostDescriptionConstSharedPtr host) { - onPoolFailure(failure, host); -} - -void Filter::onPoolReady(Http::RequestEncoder& request_encoder, - Upstream::HostDescriptionConstSharedPtr host, - const StreamInfo::StreamInfo& info) { - Http::RequestEncoder* latched_encoder = &request_encoder; - HttpUpstream* http_upstream = static_cast(upstream_.get()); - http_upstream->setRequestEncoder(request_encoder, - host->transportSocketFactory().implementsSecureTransport()); - - onPoolReadyBase(host, latched_encoder->getStream().connectionLocalAddress(), - info.downstreamSslConnection()); + if (info) { + read_callbacks_->connection().streamInfo().setUpstreamFilterState(info->filterState()); + } } const Router::MetadataMatchCriteria* Filter::metadataMatchCriteria() { @@ -624,12 +590,11 @@ void Filter::onDownstreamEvent(Network::ConnectionEvent event) { disableIdleTimer(); } } - if (upstream_handle_) { + if (generic_conn_pool_) { if (event == Network::ConnectionEvent::LocalClose || event == Network::ConnectionEvent::RemoteClose) { // Cancel the conn pool request and close any excess pending requests. - upstream_handle_->cancel(); - upstream_handle_.reset(); + generic_conn_pool_.reset(); } } } diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h index c7c91f2c85c2..12b482e01690 100644 --- a/source/common/tcp_proxy/tcp_proxy.h +++ b/source/common/tcp_proxy/tcp_proxy.h @@ -242,9 +242,8 @@ class PerConnectionCluster : public StreamInfo::FilterState::Object { */ class Filter : public Network::ReadFilter, public Upstream::LoadBalancerContextBase, - Tcp::ConnectionPool::Callbacks, - public Http::ConnectionPool::Callbacks, - protected Logger::Loggable { + protected Logger::Loggable, + public GenericConnectionPoolCallbacks { public: Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager); ~Filter() override; @@ -254,23 +253,13 @@ class Filter : public Network::ReadFilter, Network::FilterStatus onNewConnection() override; void initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) override; - // Tcp::ConnectionPool::Callbacks - void onPoolFailure(ConnectionPool::PoolFailureReason reason, - Upstream::HostDescriptionConstSharedPtr host) override; - void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, - Upstream::HostDescriptionConstSharedPtr host) override; - - // Http::ConnectionPool::Callbacks, - void onPoolFailure(ConnectionPool::PoolFailureReason reason, - absl::string_view transport_failure_reason, - Upstream::HostDescriptionConstSharedPtr host) override; - void onPoolReady(Http::RequestEncoder& request_encoder, - Upstream::HostDescriptionConstSharedPtr host, - const StreamInfo::StreamInfo& info) override; - - void onPoolReadyBase(Upstream::HostDescriptionConstSharedPtr& host, - const Network::Address::InstanceConstSharedPtr& local_address, - Ssl::ConnectionInfoConstSharedPtr ssl_info); + // GenericConnectionPoolCallbacks + void onGenericPoolReady(StreamInfo::StreamInfo* info, std::unique_ptr&& upstream, + Upstream::HostDescriptionConstSharedPtr& host, + const Network::Address::InstanceConstSharedPtr& local_address, + Ssl::ConnectionInfoConstSharedPtr ssl_info) override; + void onGenericPoolFailure(ConnectionPool::PoolFailureReason reason, + Upstream::HostDescriptionConstSharedPtr host) override; // Upstream::LoadBalancerContext const Router::MetadataMatchCriteria* metadataMatchCriteria() override; @@ -375,10 +364,15 @@ class Filter : public Network::ReadFilter, Event::TimerPtr idle_timer_; Event::TimerPtr connection_duration_timer_; - std::shared_ptr upstream_handle_; std::shared_ptr upstream_callbacks_; // shared_ptr required for passing as a // read filter. + // The upstream handle (either TCP or HTTP). This is set in onGenericPoolReady and should persist + // until either the upstream or downstream connection is terminated. std::unique_ptr upstream_; + // The connection pool used to set up |upstream_|. + // This will be non-null from when an upstream connection is attempted until + // it either succeeds or fails. + std::unique_ptr generic_conn_pool_; RouteConstSharedPtr route_; Router::MetadataMatchCriteriaConstPtr metadata_match_criteria_; Network::TransportSocketOptionsSharedPtr transport_socket_options_; diff --git a/source/common/tcp_proxy/upstream.cc b/source/common/tcp_proxy/upstream.cc index 451a277e0865..1da6eb915797 100644 --- a/source/common/tcp_proxy/upstream.cc +++ b/source/common/tcp_proxy/upstream.cc @@ -1,5 +1,7 @@ #include "common/tcp_proxy/upstream.h" +#include "envoy/upstream/cluster_manager.h" + #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/http/utility.h" @@ -152,5 +154,99 @@ void HttpUpstream::doneWriting() { } } +TcpConnPool::TcpConnPool(const std::string& cluster_name, Upstream::ClusterManager& cluster_manager, + Upstream::LoadBalancerContext* context, + Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks) + : upstream_callbacks_(upstream_callbacks) { + conn_pool_ = cluster_manager.tcpConnPoolForCluster(cluster_name, + Upstream::ResourcePriority::Default, context); +} + +TcpConnPool::~TcpConnPool() { + if (upstream_handle_ != nullptr) { + upstream_handle_->cancel(ConnectionPool::CancelPolicy::CloseExcess); + } +} + +bool TcpConnPool::valid() const { return conn_pool_ != nullptr; } + +void TcpConnPool::newStream(GenericConnectionPoolCallbacks* callbacks) { + callbacks_ = callbacks; + // Given this function is reentrant, make sure we only reset the upstream_handle_ if given a + // valid connection handle. If newConnection fails inline it may result in attempting to + // select a new host, and a recursive call to initializeUpstreamConnection. In this case the + // first call to newConnection will return null and the inner call will persist. + Tcp::ConnectionPool::Cancellable* handle = conn_pool_->newConnection(*this); + if (handle) { + ASSERT(upstream_handle_ == nullptr); + upstream_handle_ = handle; + } +} + +void TcpConnPool::onPoolFailure(ConnectionPool::PoolFailureReason reason, + Upstream::HostDescriptionConstSharedPtr host) { + upstream_handle_ = nullptr; + callbacks_->onGenericPoolFailure(reason, host); +} + +void TcpConnPool::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, + Upstream::HostDescriptionConstSharedPtr host) { + upstream_handle_ = nullptr; + Tcp::ConnectionPool::ConnectionData* latched_data = conn_data.get(); + Network::Connection& connection = conn_data->connection(); + + auto upstream = std::make_unique(std::move(conn_data), upstream_callbacks_); + callbacks_->onGenericPoolReady(&connection.streamInfo(), std::move(upstream), host, + latched_data->connection().localAddress(), + latched_data->connection().streamInfo().downstreamSslConnection()); +} + +HttpConnPool::HttpConnPool(const std::string& cluster_name, + Upstream::ClusterManager& cluster_manager, + Upstream::LoadBalancerContext* context, std::string hostname, + Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks) + : hostname_(hostname), upstream_callbacks_(upstream_callbacks) { + conn_pool_ = cluster_manager.httpConnPoolForCluster( + cluster_name, Upstream::ResourcePriority::Default, absl::nullopt, context); +} + +HttpConnPool::~HttpConnPool() { + if (upstream_handle_ != nullptr) { + // Because HTTP connections are generally shorter lived and have a higher probability of use + // before going idle, they are closed with Default rather than CloseExcess. + upstream_handle_->cancel(ConnectionPool::CancelPolicy::Default); + } +} + +bool HttpConnPool::valid() const { return conn_pool_ != nullptr; } + +void HttpConnPool::newStream(GenericConnectionPoolCallbacks* callbacks) { + callbacks_ = callbacks; + upstream_ = std::make_unique(upstream_callbacks_, hostname_); + Tcp::ConnectionPool::Cancellable* handle = + conn_pool_->newStream(upstream_->responseDecoder(), *this); + if (handle != nullptr) { + upstream_handle_ = handle; + } +} + +void HttpConnPool::onPoolFailure(ConnectionPool::PoolFailureReason reason, absl::string_view, + Upstream::HostDescriptionConstSharedPtr host) { + upstream_handle_ = nullptr; + callbacks_->onGenericPoolFailure(reason, host); +} + +void HttpConnPool::onPoolReady(Http::RequestEncoder& request_encoder, + Upstream::HostDescriptionConstSharedPtr host, + const StreamInfo::StreamInfo& info) { + upstream_handle_ = nullptr; + Http::RequestEncoder* latched_encoder = &request_encoder; + upstream_->setRequestEncoder(request_encoder, + host->transportSocketFactory().implementsSecureTransport()); + callbacks_->onGenericPoolReady(nullptr, std::move(upstream_), host, + latched_encoder->getStream().connectionLocalAddress(), + info.downstreamSslConnection()); +} + } // namespace TcpProxy } // namespace Envoy diff --git a/source/common/tcp_proxy/upstream.h b/source/common/tcp_proxy/upstream.h index 8d2a301d7137..33943e70b982 100644 --- a/source/common/tcp_proxy/upstream.h +++ b/source/common/tcp_proxy/upstream.h @@ -3,42 +3,95 @@ #include "envoy/http/conn_pool.h" #include "envoy/network/connection.h" #include "envoy/tcp/conn_pool.h" +#include "envoy/upstream/load_balancer.h" #include "envoy/upstream/upstream.h" namespace Envoy { namespace TcpProxy { -// Interface for a generic ConnectionHandle, which can wrap a TcpConnectionHandle -// or an HttpConnectionHandle -class ConnectionHandle { +class GenericConnectionPoolCallbacks; +class GenericUpstream; + +// An API for wrapping either an HTTP or a TCP connection pool. +class GenericConnPool : public Logger::Loggable { public: - virtual ~ConnectionHandle() = default; - // Cancel the conn pool request and close any excess pending requests. - virtual void cancel() PURE; + virtual ~GenericConnPool() = default; + + // Called to create a new HTTP stream or TCP connection. The implementation + // is then responsible for calling either onPoolReady or onPoolFailure on the + // supplied GenericConnectionPoolCallbacks. + virtual void newStream(GenericConnectionPoolCallbacks* callbacks) PURE; + // Returns true if there was a valid connection pool, false otherwise. + virtual bool valid() const PURE; }; -// An implementation of ConnectionHandle which works with the Tcp::ConnectionPool. -class TcpConnectionHandle : public ConnectionHandle { +class TcpConnPool : public GenericConnPool, public Tcp::ConnectionPool::Callbacks { public: - TcpConnectionHandle(Tcp::ConnectionPool::Cancellable* handle) : upstream_handle_(handle) {} + TcpConnPool(const std::string& cluster_name, Upstream::ClusterManager& cluster_manager, + Upstream::LoadBalancerContext* context, + Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks); + ~TcpConnPool() override; + + // GenericConnPool + bool valid() const override; + void newStream(GenericConnectionPoolCallbacks* callbacks) override; - void cancel() override { - upstream_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::CloseExcess); - } + // Tcp::ConnectionPool::Callbacks + void onPoolFailure(ConnectionPool::PoolFailureReason reason, + Upstream::HostDescriptionConstSharedPtr host) override; + void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, + Upstream::HostDescriptionConstSharedPtr host) override; private: + Tcp::ConnectionPool::Instance* conn_pool_{}; Tcp::ConnectionPool::Cancellable* upstream_handle_{}; + GenericConnectionPoolCallbacks* callbacks_{}; + Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks_; }; -class HttpConnectionHandle : public ConnectionHandle { +class HttpUpstream; + +class HttpConnPool : public GenericConnPool, public Http::ConnectionPool::Callbacks { public: - HttpConnectionHandle(Http::ConnectionPool::Cancellable* handle) : upstream_http_handle_(handle) {} - void cancel() override { - upstream_http_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default); - } + HttpConnPool(const std::string& cluster_name, Upstream::ClusterManager& cluster_manager, + Upstream::LoadBalancerContext* context, std::string hostname, + Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks); + ~HttpConnPool() override; + + // GenericConnPool + bool valid() const override; + void newStream(GenericConnectionPoolCallbacks* callbacks) override; + + // Http::ConnectionPool::Callbacks, + void onPoolFailure(ConnectionPool::PoolFailureReason reason, + absl::string_view transport_failure_reason, + Upstream::HostDescriptionConstSharedPtr host) override; + void onPoolReady(Http::RequestEncoder& request_encoder, + Upstream::HostDescriptionConstSharedPtr host, + const StreamInfo::StreamInfo& info) override; private: - Http::ConnectionPool::Cancellable* upstream_http_handle_{}; + const std::string hostname_; + Http::ConnectionPool::Instance* conn_pool_{}; + Http::ConnectionPool::Cancellable* upstream_handle_{}; + GenericConnectionPoolCallbacks* callbacks_{}; + Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks_; + std::unique_ptr upstream_; +}; + +// An API for the UpstreamRequest to get callbacks from either an HTTP or TCP +// connection pool. +class GenericConnectionPoolCallbacks { +public: + virtual ~GenericConnectionPoolCallbacks() = default; + + virtual void onGenericPoolReady(StreamInfo::StreamInfo* info, + std::unique_ptr&& upstream, + Upstream::HostDescriptionConstSharedPtr& host, + const Network::Address::InstanceConstSharedPtr& local_address, + Ssl::ConnectionInfoConstSharedPtr ssl_info) PURE; + virtual void onGenericPoolFailure(ConnectionPool::PoolFailureReason reason, + Upstream::HostDescriptionConstSharedPtr host) PURE; }; // Interface for a generic Upstream, which can communicate with a TCP or HTTP From 5fdf6295efe2fe886b175e7dfb825516d5296d25 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 13 Oct 2020 15:54:26 -0400 Subject: [PATCH 08/14] overload: tcp connection refusal overload action (#13311) This will allow creating an overload action that rejects (accepts the connection and then closes it) some fraction of connections in response to overload conditions. Signed-off-by: Alex Konradi --- docs/root/configuration/listeners/stats.rst | 1 + .../overload_manager/overload_manager.rst | 25 ++- docs/root/version_history/current.rst | 1 + include/envoy/network/connection_handler.h | 6 + include/envoy/network/listener.h | 12 +- include/envoy/server/overload_manager.h | 4 + source/common/event/dispatcher_impl.cc | 4 +- source/common/network/tcp_listener_impl.cc | 19 +- source/common/network/tcp_listener_impl.h | 9 +- source/common/network/udp_listener_impl.h | 1 + source/server/connection_handler_impl.cc | 21 +++ source/server/connection_handler_impl.h | 5 +- source/server/worker_impl.cc | 7 + source/server/worker_impl.h | 1 + test/common/network/dns_impl_test.cc | 2 +- test/common/network/listener_impl_test.cc | 174 ++++++++++++++++-- test/mocks/network/mocks.h | 5 +- test/server/connection_handler_test.cc | 52 +++++- 18 files changed, 313 insertions(+), 36 deletions(-) diff --git a/docs/root/configuration/listeners/stats.rst b/docs/root/configuration/listeners/stats.rst index 09ad6858fbe2..eb82810f6972 100644 --- a/docs/root/configuration/listeners/stats.rst +++ b/docs/root/configuration/listeners/stats.rst @@ -17,6 +17,7 @@ Every listener has a statistics tree rooted at *listener.
.* with the fo downstream_cx_active, Gauge, Total active connections downstream_cx_length_ms, Histogram, Connection length milliseconds downstream_cx_overflow, Counter, Total connections rejected due to enforcement of listener connection limit + downstream_cx_overload_reject, Counter, Total connections rejected due to configured overload actions downstream_pre_cx_timeout, Counter, Sockets that timed out during listener filter processing downstream_pre_cx_active, Gauge, Sockets currently undergoing listener filter processing global_cx_overflow, Counter, Total connections rejected due to enforecement of the global connection limit diff --git a/docs/root/configuration/operations/overload_manager/overload_manager.rst b/docs/root/configuration/operations/overload_manager/overload_manager.rst index ade5201f5c4c..22e7e70b33b5 100644 --- a/docs/root/configuration/operations/overload_manager/overload_manager.rst +++ b/docs/root/configuration/operations/overload_manager/overload_manager.rst @@ -68,14 +68,27 @@ Overload actions The following overload actions are supported: -.. csv-table:: - :header: Name, Description +.. list-table:: + :header-rows: 1 :widths: 1, 2 - envoy.overload_actions.stop_accepting_requests, Envoy will immediately respond with a 503 response code to new requests - envoy.overload_actions.disable_http_keepalive, Envoy will stop accepting streams on incoming HTTP connections - envoy.overload_actions.stop_accepting_connections, Envoy will stop accepting new network connections on its configured listeners - envoy.overload_actions.shrink_heap, Envoy will periodically try to shrink the heap by releasing free memory to the system + * - Name + - Description + + * - envoy.overload_actions.stop_accepting_requests + - Envoy will immediately respond with a 503 response code to new requests + + * - envoy.overload_actions.disable_http_keepalive + - Envoy will stop accepting streams on incoming HTTP connections + + * - envoy.overload_actions.stop_accepting_connections + - Envoy will stop accepting new network connections on its configured listeners + + * - envoy.overload_actions.reject_incoming_connections + - Envoy will reject incoming connections on its configured listeners without processing any data + + * - envoy.overload_actions.shrink_heap + - Envoy will periodically try to shrink the heap by releasing free memory to the system Limiting Active Connections --------------------------- diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 0c24a46dd201..6e5fc92acf08 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -27,6 +27,7 @@ New Features * grpc: implemented header value syntax support when defining :ref:`initial metadata ` for gRPC-based `ext_authz` :ref:`HTTP ` and :ref:`network ` filters, and :ref:`ratelimit ` filters. * health_check: added option to use :ref:`no_traffic_healthy_interval ` which allows a different no traffic interval when the host is healthy. * mongo_proxy: the list of commands to produce metrics for is now :ref:`configurable `. +* tcp: added a new :ref:`envoy.overload_actions.reject_incoming_connections ` action to reject incoming TCP connections. Deprecated ---------- diff --git a/include/envoy/network/connection_handler.h b/include/envoy/network/connection_handler.h index ea804eba5789..c42cc290cd61 100644 --- a/include/envoy/network/connection_handler.h +++ b/include/envoy/network/connection_handler.h @@ -94,6 +94,12 @@ class ConnectionHandler { */ virtual void enableListeners() PURE; + /** + * Set the fraction of connections the listeners should reject. + * @param reject_fraction a value between 0 (reject none) and 1 (reject all). + */ + virtual void setListenerRejectFraction(float reject_fraction) PURE; + /** * @return the stat prefix used for per-handler stats. */ diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index f74d6416103a..4401df6cc20c 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -197,10 +197,14 @@ class TcpListenerCallbacks { */ virtual void onAccept(ConnectionSocketPtr&& socket) PURE; + enum class RejectCause { + GlobalCxLimit, + OverloadAction, + }; /** * Called when a new connection is rejected. */ - virtual void onReject() PURE; + virtual void onReject(RejectCause cause) PURE; }; /** @@ -324,6 +328,12 @@ class Listener { * Enable accepting new connections. */ virtual void enable() PURE; + + /** + * Set the fraction of incoming connections that will be closed immediately + * after being opened. + */ + virtual void setRejectFraction(float reject_fraction) PURE; }; using ListenerPtr = std::unique_ptr; diff --git a/include/envoy/server/overload_manager.h b/include/envoy/server/overload_manager.h index 77c6c21c097d..8f5914fe34f3 100644 --- a/include/envoy/server/overload_manager.h +++ b/include/envoy/server/overload_manager.h @@ -62,6 +62,10 @@ class OverloadActionNameValues { // Overload action to stop accepting new connections. const std::string StopAcceptingConnections = "envoy.overload_actions.stop_accepting_connections"; + // Overload action to reject (accept and then close) new connections. + const std::string RejectIncomingConnections = + "envoy.overload_actions.reject_incoming_connections"; + // Overload action to try to shrink the heap by releasing free memory. const std::string ShrinkHeap = "envoy.overload_actions.shrink_heap"; }; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 612aef3b7c41..6cdbb623b720 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -162,8 +162,8 @@ Network::ListenerPtr DispatcherImpl::createListener(Network::SocketSharedPtr&& s Network::TcpListenerCallbacks& cb, bool bind_to_port, uint32_t backlog_size) { ASSERT(isThreadSafe()); - return std::make_unique(*this, std::move(socket), cb, bind_to_port, - backlog_size); + return std::make_unique( + *this, api_.randomGenerator(), std::move(socket), cb, bind_to_port, backlog_size); } Network::UdpListenerPtr DispatcherImpl::createUdpListener(Network::SocketSharedPtr socket, diff --git a/source/common/network/tcp_listener_impl.cc b/source/common/network/tcp_listener_impl.cc index 5c39ec692f89..91975d6ca5d1 100644 --- a/source/common/network/tcp_listener_impl.cc +++ b/source/common/network/tcp_listener_impl.cc @@ -62,7 +62,11 @@ void TcpListenerImpl::onSocketEvent(short flags) { if (rejectCxOverGlobalLimit()) { // The global connection limit has been reached. io_handle->close(); - cb_.onReject(); + cb_.onReject(TcpListenerCallbacks::RejectCause::GlobalCxLimit); + continue; + } else if (random_.bernoulli(reject_fraction_)) { + io_handle->close(); + cb_.onReject(TcpListenerCallbacks::RejectCause::OverloadAction); continue; } @@ -106,9 +110,11 @@ void TcpListenerImpl::setupServerSocket(Event::DispatcherImpl& dispatcher, Socke } } -TcpListenerImpl::TcpListenerImpl(Event::DispatcherImpl& dispatcher, SocketSharedPtr socket, - TcpListenerCallbacks& cb, bool bind_to_port, uint32_t backlog_size) - : BaseListenerImpl(dispatcher, std::move(socket)), cb_(cb), backlog_size_(backlog_size) { +TcpListenerImpl::TcpListenerImpl(Event::DispatcherImpl& dispatcher, Random::RandomGenerator& random, + SocketSharedPtr socket, TcpListenerCallbacks& cb, + bool bind_to_port, uint32_t backlog_size) + : BaseListenerImpl(dispatcher, std::move(socket)), cb_(cb), backlog_size_(backlog_size), + random_(random), reject_fraction_(0.0) { if (bind_to_port) { setupServerSocket(dispatcher, *socket_); } @@ -118,5 +124,10 @@ void TcpListenerImpl::enable() { file_event_->setEnabled(Event::FileReadyType::R void TcpListenerImpl::disable() { file_event_->setEnabled(0); } +void TcpListenerImpl::setRejectFraction(const float reject_fraction) { + ASSERT(0 <= reject_fraction && reject_fraction <= 1); + reject_fraction_ = reject_fraction; +} + } // namespace Network } // namespace Envoy diff --git a/source/common/network/tcp_listener_impl.h b/source/common/network/tcp_listener_impl.h index 56b6725b36c9..5ecec192abf2 100644 --- a/source/common/network/tcp_listener_impl.h +++ b/source/common/network/tcp_listener_impl.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/common/random_generator.h" #include "envoy/runtime/runtime.h" #include "absl/strings/string_view.h" @@ -13,10 +14,12 @@ namespace Network { */ class TcpListenerImpl : public BaseListenerImpl { public: - TcpListenerImpl(Event::DispatcherImpl& dispatcher, SocketSharedPtr socket, - TcpListenerCallbacks& cb, bool bind_to_port, uint32_t backlog_size); + TcpListenerImpl(Event::DispatcherImpl& dispatcher, Random::RandomGenerator& random, + SocketSharedPtr socket, TcpListenerCallbacks& cb, bool bind_to_port, + uint32_t backlog_size); void disable() override; void enable() override; + void setRejectFraction(float reject_fraction) override; static const absl::string_view GlobalMaxCxRuntimeKey; @@ -33,7 +36,9 @@ class TcpListenerImpl : public BaseListenerImpl { // rejected/closed. If the accepted socket is to be admitted, false is returned. static bool rejectCxOverGlobalLimit(); + Random::RandomGenerator& random_; Event::FileEventPtr file_event_; + float reject_fraction_; }; } // namespace Network diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index e857a0150f25..d555649833bc 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -30,6 +30,7 @@ class UdpListenerImpl : public BaseListenerImpl, // Network::Listener Interface void disable() override; void enable() override; + void setRejectFraction(float) override {} // Network::UdpListener Interface Event::Dispatcher& dispatcher() override; diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index fd5586fb78cd..f819f5843c7b 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -66,6 +66,9 @@ void ConnectionHandlerImpl::addListener(absl::optional overridden_list if (disable_listeners_) { details.listener_->pauseListening(); } + if (auto* listener = details.listener_->listener(); listener != nullptr) { + listener->setRejectFraction(listener_reject_fraction_); + } listeners_.emplace_back(config.listenSocketFactory().localAddress(), std::move(details)); } @@ -148,6 +151,13 @@ void ConnectionHandlerImpl::enableListeners() { } } +void ConnectionHandlerImpl::setListenerRejectFraction(float reject_fraction) { + listener_reject_fraction_ = reject_fraction; + for (auto& listener : listeners_) { + listener.second.listener_->listener()->setRejectFraction(reject_fraction); + } +} + void ConnectionHandlerImpl::ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); ActiveConnections& active_connections = connection.active_connections_; @@ -391,6 +401,17 @@ void ConnectionHandlerImpl::ActiveTcpListener::onAccept(Network::ConnectionSocke onAcceptWorker(std::move(socket), config_->handOffRestoredDestinationConnections(), false); } +void ConnectionHandlerImpl::ActiveTcpListener::onReject(RejectCause cause) { + switch (cause) { + case RejectCause::GlobalCxLimit: + stats_.downstream_global_cx_overflow_.inc(); + break; + case RejectCause::OverloadAction: + stats_.downstream_cx_overload_reject_.inc(); + break; + } +} + void ConnectionHandlerImpl::ActiveTcpListener::onAcceptWorker( Network::ConnectionSocketPtr&& socket, bool hand_off_restored_destination_connections, bool rebalanced) { diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 62ddf0c11be7..66d317e8f10e 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -30,6 +30,7 @@ namespace Server { COUNTER(downstream_cx_destroy) \ COUNTER(downstream_cx_overflow) \ COUNTER(downstream_cx_total) \ + COUNTER(downstream_cx_overload_reject) \ COUNTER(downstream_global_cx_overflow) \ COUNTER(downstream_pre_cx_timeout) \ COUNTER(no_filter_chain_match) \ @@ -82,6 +83,7 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, void stopListeners() override; void disableListeners() override; void enableListeners() override; + void setListenerRejectFraction(float reject_fraction) override; const std::string& statPrefix() const override { return per_handler_stat_prefix_; } /** @@ -133,7 +135,7 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, // Network::TcpListenerCallbacks void onAccept(Network::ConnectionSocketPtr&& socket) override; - void onReject() override { stats_.downstream_global_cx_overflow_.inc(); } + void onReject(RejectCause) override; // ActiveListenerImplBase Network::Listener* listener() override { return listener_.get(); } @@ -361,6 +363,7 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, std::list> listeners_; std::atomic num_handler_connections_{}; bool disable_listeners_; + float listener_reject_fraction_{0}; }; class ActiveUdpListenerBase : public ConnectionHandlerImpl::ActiveListenerImplBase, diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index 1fd18f106618..b659ffec6e06 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -31,6 +31,9 @@ WorkerImpl::WorkerImpl(ThreadLocal::Instance& tls, ListenerHooks& hooks, overload_manager.registerForAction( OverloadActionNames::get().StopAcceptingConnections, *dispatcher_, [this](OverloadActionState state) { stopAcceptingConnectionsCb(state); }); + overload_manager.registerForAction( + OverloadActionNames::get().RejectIncomingConnections, *dispatcher_, + [this](OverloadActionState state) { rejectIncomingConnectionsCb(state); }); } void WorkerImpl::addListener(absl::optional overridden_listener, @@ -149,5 +152,9 @@ void WorkerImpl::stopAcceptingConnectionsCb(OverloadActionState state) { } } +void WorkerImpl::rejectIncomingConnectionsCb(OverloadActionState state) { + handler_->setListenerRejectFraction(static_cast(state.value())); +} + } // namespace Server } // namespace Envoy diff --git a/source/server/worker_impl.h b/source/server/worker_impl.h index c4cb4a58c2b5..22513b594e5d 100644 --- a/source/server/worker_impl.h +++ b/source/server/worker_impl.h @@ -58,6 +58,7 @@ class WorkerImpl : public Worker, Logger::Loggable { private: void threadRoutine(GuardDog& guard_dog); void stopAcceptingConnectionsCb(OverloadActionState state); + void rejectIncomingConnectionsCb(OverloadActionState state); ThreadLocal::Instance& tls_; ListenerHooks& hooks_; diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index ea83c16c21e7..339cfb4abc89 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -281,7 +281,7 @@ class TestDnsServer : public TcpListenerCallbacks { queries_.emplace_back(query); } - void onReject() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + void onReject(RejectCause) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void addHosts(const std::string& hostname, const IpList& ip, const RecordType& type) { if (type == RecordType::A) { diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 8056826de91a..21e4673ce643 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -1,3 +1,5 @@ +#include + #include "envoy/config/core/v3/base.pb.h" #include "envoy/network/exception.h" @@ -65,10 +67,11 @@ TEST_P(ListenerImplDeathTest, ErrorCallback) { class TestTcpListenerImpl : public TcpListenerImpl { public: - TestTcpListenerImpl(Event::DispatcherImpl& dispatcher, SocketSharedPtr socket, - TcpListenerCallbacks& cb, bool bind_to_port, + TestTcpListenerImpl(Event::DispatcherImpl& dispatcher, Random::RandomGenerator& random_generator, + SocketSharedPtr socket, TcpListenerCallbacks& cb, bool bind_to_port, uint32_t tcp_backlog = ENVOY_TCP_BACKLOG_SIZE) - : TcpListenerImpl(dispatcher, std::move(socket), cb, bind_to_port, tcp_backlog) {} + : TcpListenerImpl(dispatcher, random_generator, std::move(socket), cb, bind_to_port, + tcp_backlog) {} MOCK_METHOD(Address::InstanceConstSharedPtr, getLocalAddress, (os_fd_t fd)); }; @@ -82,6 +85,7 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, TcpListenerImplTest, TEST_P(TcpListenerImplTest, SetListeningSocketOptionsSuccess) { Network::MockTcpListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; auto socket = std::make_shared( Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); @@ -89,13 +93,15 @@ TEST_P(TcpListenerImplTest, SetListeningSocketOptionsSuccess) { socket->addOption(option); EXPECT_CALL(*option, setOption(_, envoy::config::core::v3::SocketOption::STATE_LISTENING)) .WillOnce(Return(true)); - TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks, true); + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); } // Test that an exception is thrown if there is an error setting socket options. TEST_P(TcpListenerImplTest, SetListeningSocketOptionsError) { Network::MockTcpListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; auto socket = std::make_shared( Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); @@ -103,10 +109,11 @@ TEST_P(TcpListenerImplTest, SetListeningSocketOptionsError) { socket->addOption(option); EXPECT_CALL(*option, setOption(_, envoy::config::core::v3::SocketOption::STATE_LISTENING)) .WillOnce(Return(false)); - EXPECT_THROW_WITH_MESSAGE(TestTcpListenerImpl(dispatcherImpl(), socket, listener_callbacks, true), - CreateListenerException, - fmt::format("cannot set post-listen socket option on socket: {}", - socket->localAddress()->asString())); + EXPECT_THROW_WITH_MESSAGE( + TestTcpListenerImpl(dispatcherImpl(), random_generator, socket, listener_callbacks, true), + CreateListenerException, + fmt::format("cannot set post-listen socket option on socket: {}", + socket->localAddress()->asString())); } TEST_P(TcpListenerImplTest, UseActualDst) { @@ -115,10 +122,13 @@ TEST_P(TcpListenerImplTest, UseActualDst) { auto socketDst = std::make_shared(alt_address_, nullptr, false); Network::MockTcpListenerCallbacks listener_callbacks1; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; // Do not redirect since use_original_dst is false. - Network::TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks1, true); + Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, + listener_callbacks1, true); Network::MockTcpListenerCallbacks listener_callbacks2; - Network::TestTcpListenerImpl listenerDst(dispatcherImpl(), socketDst, listener_callbacks2, false); + Network::TestTcpListenerImpl listenerDst(dispatcherImpl(), random_generator, socketDst, + listener_callbacks2, false); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket->localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -175,7 +185,8 @@ TEST_P(TcpListenerImplTest, GlobalConnectionLimitEnforcement) { }; initiate_connections(5); - EXPECT_CALL(listener_callbacks, onReject()).Times(3); + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::GlobalCxLimit)) + .Times(3); dispatcher_->run(Event::Dispatcher::RunType::Block); // We expect any server-side connections that get created to populate 'server_connections'. @@ -185,7 +196,8 @@ TEST_P(TcpListenerImplTest, GlobalConnectionLimitEnforcement) { Runtime::LoaderSingleton::getExisting()->mergeValues( {{"overload.global_downstream_max_connections", "3"}}); initiate_connections(5); - EXPECT_CALL(listener_callbacks, onReject()).Times(4); + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::GlobalCxLimit)) + .Times(4); dispatcher_->run(Event::Dispatcher::RunType::Block); EXPECT_EQ(3, server_connections.size()); @@ -214,8 +226,10 @@ TEST_P(TcpListenerImplTest, WildcardListenerUseActualDst) { Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); Network::MockTcpListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; // Do not redirect since use_original_dst is false. - Network::TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks, true); + Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, + listener_callbacks, true); auto local_dst_address = Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(version_), socket->localAddress()->ip()->port()); @@ -253,11 +267,13 @@ TEST_P(TcpListenerImplTest, WildcardListenerIpv4Compat) { options, true); Network::MockTcpListenerCallbacks listener_callbacks; Network::MockConnectionHandler connection_handler; + Random::MockRandomGenerator random_generator; ASSERT_TRUE(socket->localAddress()->ip()->isAnyAddress()); // Do not redirect since use_original_dst is false. - Network::TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks, true); + Network::TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, + listener_callbacks, true); auto listener_address = Network::Utility::getAddressWithPort( *Network::Test::getCanonicalLoopbackAddress(version_), socket->localAddress()->ip()->port()); @@ -291,7 +307,9 @@ TEST_P(TcpListenerImplTest, DisableAndEnableListener) { Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); MockTcpListenerCallbacks listener_callbacks; MockConnectionCallbacks connection_callbacks; - TestTcpListenerImpl listener(dispatcherImpl(), socket, listener_callbacks, true); + Random::MockRandomGenerator random_generator; + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); // When listener is disabled, the timer should fire before any connection is accepted. listener.disable(); @@ -325,6 +343,132 @@ TEST_P(TcpListenerImplTest, DisableAndEnableListener) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +TEST_P(TcpListenerImplTest, SetListenerRejectFractionZero) { + auto socket = std::make_shared( + Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); + MockTcpListenerCallbacks listener_callbacks; + MockConnectionCallbacks connection_callbacks; + Random::MockRandomGenerator random_generator; + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); + + listener.setRejectFraction(0); + + // This connection will be accepted and not rejected. + { + testing::InSequence s1; + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::LocalClose)); + } + EXPECT_CALL(listener_callbacks, onAccept_(_)).WillOnce([&] { dispatcher_->exit(); }); + + ClientConnectionPtr client_connection = + dispatcher_->createClientConnection(socket->localAddress(), Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + client_connection->addConnectionCallbacks(connection_callbacks); + client_connection->connect(); + dispatcher_->run(Event::Dispatcher::RunType::Block); + + // Now that we've seen that the connection hasn't been closed by the listener, make sure to close + // it. + client_connection->close(ConnectionCloseType::NoFlush); +} + +TEST_P(TcpListenerImplTest, SetListenerRejectFractionIntermediate) { + auto socket = std::make_shared( + Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); + MockTcpListenerCallbacks listener_callbacks; + MockConnectionCallbacks connection_callbacks; + Random::MockRandomGenerator random_generator; + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); + + listener.setRejectFraction(0.5f); + + // The first connection will be rejected because the random value is too small. + { + testing::InSequence s1; + EXPECT_CALL(random_generator, random()).WillOnce(Return(0)); + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::OverloadAction)); + } + { + testing::InSequence s2; + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { + dispatcher_->exit(); + }); + } + + { + ClientConnectionPtr client_connection = dispatcher_->createClientConnection( + socket->localAddress(), Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + client_connection->addConnectionCallbacks(connection_callbacks); + client_connection->connect(); + dispatcher_->run(Event::Dispatcher::RunType::Block); + } + + // The second connection rolls better on initiative and is accepted. + { + testing::InSequence s1; + EXPECT_CALL(random_generator, random()).WillOnce(Return(std::numeric_limits::max())); + EXPECT_CALL(listener_callbacks, onAccept_(_)); + } + { + testing::InSequence s2; + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)).WillOnce([&] { + dispatcher_->exit(); + }); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).Times(0); + } + + { + ClientConnectionPtr client_connection = dispatcher_->createClientConnection( + socket->localAddress(), Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + client_connection->addConnectionCallbacks(connection_callbacks); + client_connection->connect(); + dispatcher_->run(Event::Dispatcher::RunType::Block); + + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::LocalClose)); + // Now that we've seen that the connection hasn't been closed by the listener, make sure to + // close it. + client_connection->close(ConnectionCloseType::NoFlush); + } +} + +TEST_P(TcpListenerImplTest, SetListenerRejectFractionAll) { + auto socket = std::make_shared( + Network::Test::getCanonicalLoopbackAddress(version_), nullptr, true); + MockTcpListenerCallbacks listener_callbacks; + MockConnectionCallbacks connection_callbacks; + Random::MockRandomGenerator random_generator; + TestTcpListenerImpl listener(dispatcherImpl(), random_generator, socket, listener_callbacks, + true); + + listener.setRejectFraction(1); + + { + testing::InSequence s1; + EXPECT_CALL(listener_callbacks, onReject(TcpListenerCallbacks::RejectCause::OverloadAction)); + } + + { + testing::InSequence s2; + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(connection_callbacks, onEvent(ConnectionEvent::RemoteClose)).WillOnce([&] { + dispatcher_->exit(); + }); + } + + ClientConnectionPtr client_connection = + dispatcher_->createClientConnection(socket->localAddress(), Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + client_connection->addConnectionCallbacks(connection_callbacks); + client_connection->connect(); + dispatcher_->run(Event::Dispatcher::RunType::Block); +} + } // namespace } // namespace Network } // namespace Envoy diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 304de415e7d3..de7b843a72d0 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -131,7 +131,7 @@ class MockTcpListenerCallbacks : public TcpListenerCallbacks { void onAccept(ConnectionSocketPtr&& socket) override { onAccept_(socket); } MOCK_METHOD(void, onAccept_, (ConnectionSocketPtr & socket)); - MOCK_METHOD(void, onReject, ()); + MOCK_METHOD(void, onReject, (RejectCause), (override)); }; class MockUdpListenerCallbacks : public UdpListenerCallbacks { @@ -391,6 +391,7 @@ class MockListener : public Listener { MOCK_METHOD(void, onDestroy, ()); MOCK_METHOD(void, enable, ()); MOCK_METHOD(void, disable, ()); + MOCK_METHOD(void, setRejectFraction, (float)); }; class MockConnectionHandler : public ConnectionHandler { @@ -412,6 +413,7 @@ class MockConnectionHandler : public ConnectionHandler { MOCK_METHOD(void, stopListeners, ()); MOCK_METHOD(void, disableListeners, ()); MOCK_METHOD(void, enableListeners, ()); + MOCK_METHOD(void, setListenerRejectFraction, (float), (override)); MOCK_METHOD(const std::string&, statPrefix, (), (const)); }; @@ -501,6 +503,7 @@ class MockUdpListener : public UdpListener { MOCK_METHOD(void, onDestroy, ()); MOCK_METHOD(void, enable, ()); MOCK_METHOD(void, disable, ()); + MOCK_METHOD(void, setRejectFraction, (float), (override)); MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); MOCK_METHOD(Address::InstanceConstSharedPtr&, localAddress, (), (const)); MOCK_METHOD(Api::IoCallUint64Result, send, (const UdpSendData&)); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index dbb6a5698e43..b286dc588dd2 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -176,6 +176,7 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::LoggableaddListener(absl::nullopt, *test_listener); } -TEST_F(ConnectionHandlerTest, DisableListenerAfterStop) { +TEST_F(ConnectionHandlerTest, SetListenerRejectFraction) { InSequence s; Network::TcpListenerCallbacks* listener_callbacks; @@ -461,10 +462,25 @@ TEST_F(ConnectionHandlerTest, DisableListenerAfterStop) { EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); handler_->addListener(absl::nullopt, *test_listener); + EXPECT_CALL(*listener, setRejectFraction(0.1234f)); EXPECT_CALL(*listener, onDestroy()); - handler_->stopListeners(); - handler_->disableListeners(); + handler_->setListenerRejectFraction(0.1234f); +} + +TEST_F(ConnectionHandlerTest, AddListenerSetRejectFraction) { + InSequence s; + + Network::TcpListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, false, false, "test_listener", listener, &listener_callbacks); + EXPECT_CALL(*listener, setRejectFraction(0.12345f)); + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + EXPECT_CALL(*listener, onDestroy()); + + handler_->setListenerRejectFraction(0.12345f); + handler_->addListener(absl::nullopt, *test_listener); } TEST_F(ConnectionHandlerTest, DestroyCloseConnections) { @@ -1096,6 +1112,36 @@ TEST_F(ConnectionHandlerTest, TcpListenerRemoveFilterChain) { handler_.reset(); } +TEST_F(ConnectionHandlerTest, TcpListenerGlobalCxLimitReject) { + Network::TcpListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, true, false, "test_listener", listener, &listener_callbacks); + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + handler_->addListener(absl::nullopt, *test_listener); + + listener_callbacks->onReject(Network::TcpListenerCallbacks::RejectCause::GlobalCxLimit); + + EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "downstream_global_cx_overflow")->value()); + EXPECT_EQ(0UL, TestUtility::findCounter(stats_store_, "downstream_cx_overload_reject")->value()); + EXPECT_CALL(*listener, onDestroy()); +} + +TEST_F(ConnectionHandlerTest, TcpListenerOverloadActionReject) { + Network::TcpListenerCallbacks* listener_callbacks; + auto listener = new NiceMock(); + TestListener* test_listener = + addListener(1, true, false, "test_listener", listener, &listener_callbacks); + EXPECT_CALL(*socket_factory_, localAddress()).WillOnce(ReturnRef(local_address_)); + handler_->addListener(absl::nullopt, *test_listener); + + listener_callbacks->onReject(Network::TcpListenerCallbacks::RejectCause::OverloadAction); + + EXPECT_EQ(1UL, TestUtility::findCounter(stats_store_, "downstream_cx_overload_reject")->value()); + EXPECT_EQ(0UL, TestUtility::findCounter(stats_store_, "downstream_global_cx_overflow")->value()); + EXPECT_CALL(*listener, onDestroy()); +} + // Listener Filter matchers works. TEST_F(ConnectionHandlerTest, ListenerFilterWorks) { Network::TcpListenerCallbacks* listener_callbacks; From 328ffbe19b15a8992dc9f7b89bc153cf83efabf9 Mon Sep 17 00:00:00 2001 From: phlax Date: Tue, 13 Oct 2020 20:54:49 +0100 Subject: [PATCH 09/14] examples: Fix more deprecations/warnings in configs (#13529) Signed-off-by: Ryan Northey --- examples/fault-injection/envoy.yaml | 2 +- examples/jaeger-native-tracing/front-envoy-jaeger.yaml | 2 +- examples/jaeger-native-tracing/service1-envoy-jaeger.yaml | 2 +- examples/jaeger-native-tracing/service2-envoy-jaeger.yaml | 2 +- examples/jaeger-tracing/front-envoy-jaeger.yaml | 2 +- examples/jaeger-tracing/service1-envoy-jaeger.yaml | 4 ++-- examples/jaeger-tracing/service2-envoy-jaeger.yaml | 2 +- examples/mysql/envoy.yaml | 4 ++-- examples/redis/envoy.yaml | 2 +- examples/zipkin-tracing/front-envoy-zipkin.yaml | 2 +- examples/zipkin-tracing/service1-envoy-zipkin.yaml | 4 ++-- examples/zipkin-tracing/service2-envoy-zipkin.yaml | 2 +- 12 files changed, 15 insertions(+), 15 deletions(-) diff --git a/examples/fault-injection/envoy.yaml b/examples/fault-injection/envoy.yaml index 06ced8c60c57..db63f9469b8b 100644 --- a/examples/fault-injection/envoy.yaml +++ b/examples/fault-injection/envoy.yaml @@ -30,7 +30,7 @@ static_resources: http_filters: - name: envoy.filters.http.fault typed_config: - "@type": type.googleapis.com/envoy.config.filter.http.fault.v2.HTTPFault + "@type": type.googleapis.com/envoy.extensions.filters.http.fault.v3.HTTPFault abort: http_status: 503 percentage: diff --git a/examples/jaeger-native-tracing/front-envoy-jaeger.yaml b/examples/jaeger-native-tracing/front-envoy-jaeger.yaml index 0bdb5bc3e9cd..f2530538eace 100644 --- a/examples/jaeger-native-tracing/front-envoy-jaeger.yaml +++ b/examples/jaeger-native-tracing/front-envoy-jaeger.yaml @@ -15,7 +15,7 @@ static_resources: provider: name: envoy.tracers.dynamic_ot typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.DynamicOtConfig + "@type": type.googleapis.com/envoy.config.trace.v3.DynamicOtConfig library: /usr/local/lib/libjaegertracing_plugin.so config: service_name: front-proxy diff --git a/examples/jaeger-native-tracing/service1-envoy-jaeger.yaml b/examples/jaeger-native-tracing/service1-envoy-jaeger.yaml index 8b49793be672..19678e297dfb 100644 --- a/examples/jaeger-native-tracing/service1-envoy-jaeger.yaml +++ b/examples/jaeger-native-tracing/service1-envoy-jaeger.yaml @@ -42,7 +42,7 @@ static_resources: provider: name: envoy.tracers.dynamic_ot typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.DynamicOtConfig + "@type": type.googleapis.com/envoy.config.trace.v3.DynamicOtConfig library: /usr/local/lib/libjaegertracing_plugin.so config: service_name: service1 diff --git a/examples/jaeger-native-tracing/service2-envoy-jaeger.yaml b/examples/jaeger-native-tracing/service2-envoy-jaeger.yaml index 850a41917525..92fe6bd69c73 100644 --- a/examples/jaeger-native-tracing/service2-envoy-jaeger.yaml +++ b/examples/jaeger-native-tracing/service2-envoy-jaeger.yaml @@ -14,7 +14,7 @@ static_resources: provider: name: envoy.tracers.dynamic_ot typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.DynamicOtConfig + "@type": type.googleapis.com/envoy.config.trace.v3.DynamicOtConfig library: /usr/local/lib/libjaegertracing_plugin.so config: service_name: service2 diff --git a/examples/jaeger-tracing/front-envoy-jaeger.yaml b/examples/jaeger-tracing/front-envoy-jaeger.yaml index 8dc89c99cb7b..fc65ddefc792 100644 --- a/examples/jaeger-tracing/front-envoy-jaeger.yaml +++ b/examples/jaeger-tracing/front-envoy-jaeger.yaml @@ -15,7 +15,7 @@ static_resources: provider: name: envoy.tracers.zipkin typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig + "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: jaeger collector_endpoint: "/api/v2/spans" shared_span_context: false diff --git a/examples/jaeger-tracing/service1-envoy-jaeger.yaml b/examples/jaeger-tracing/service1-envoy-jaeger.yaml index 64f80b11a960..447a4cad2b34 100644 --- a/examples/jaeger-tracing/service1-envoy-jaeger.yaml +++ b/examples/jaeger-tracing/service1-envoy-jaeger.yaml @@ -14,7 +14,7 @@ static_resources: provider: name: envoy.tracers.zipkin typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig + "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: jaeger collector_endpoint: "/api/v2/spans" shared_span_context: false @@ -51,7 +51,7 @@ static_resources: provider: name: envoy.tracers.zipkin typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig + "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: jaeger collector_endpoint: "/api/v2/spans" shared_span_context: false diff --git a/examples/jaeger-tracing/service2-envoy-jaeger.yaml b/examples/jaeger-tracing/service2-envoy-jaeger.yaml index f01edc340114..517570f2416b 100644 --- a/examples/jaeger-tracing/service2-envoy-jaeger.yaml +++ b/examples/jaeger-tracing/service2-envoy-jaeger.yaml @@ -14,7 +14,7 @@ static_resources: provider: name: envoy.tracers.zipkin typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig + "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: jaeger collector_endpoint: "/api/v2/spans" shared_span_context: false diff --git a/examples/mysql/envoy.yaml b/examples/mysql/envoy.yaml index ee485d33b6c4..e7dc86a53702 100644 --- a/examples/mysql/envoy.yaml +++ b/examples/mysql/envoy.yaml @@ -9,11 +9,11 @@ static_resources: - filters: - name: envoy.filters.network.mysql_proxy typed_config: - "@type": type.googleapis.com/envoy.config.filter.network.mysql_proxy.v1alpha1.MySQLProxy + "@type": type.googleapis.com/envoy.extensions.filters.network.mysql_proxy.v3.MySQLProxy stat_prefix: egress_mysql - name: envoy.filters.network.tcp_proxy typed_config: - "@type": type.googleapis.com/envoy.config.filter.network.tcp_proxy.v2.TcpProxy + "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy stat_prefix: mysql_tcp cluster: mysql_cluster diff --git a/examples/redis/envoy.yaml b/examples/redis/envoy.yaml index e886be7e0750..b228864c7727 100644 --- a/examples/redis/envoy.yaml +++ b/examples/redis/envoy.yaml @@ -9,7 +9,7 @@ static_resources: - filters: - name: envoy.filters.network.redis_proxy typed_config: - "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProxy + "@type": type.googleapis.com/envoy.extensions.filters.network.redis_proxy.v3.RedisProxy stat_prefix: egress_redis settings: op_timeout: 5s diff --git a/examples/zipkin-tracing/front-envoy-zipkin.yaml b/examples/zipkin-tracing/front-envoy-zipkin.yaml index 2453d52d3251..232d76aa8728 100644 --- a/examples/zipkin-tracing/front-envoy-zipkin.yaml +++ b/examples/zipkin-tracing/front-envoy-zipkin.yaml @@ -15,7 +15,7 @@ static_resources: provider: name: envoy.tracers.zipkin typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig + "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: zipkin collector_endpoint: "/api/v2/spans" collector_endpoint_version: HTTP_JSON diff --git a/examples/zipkin-tracing/service1-envoy-zipkin.yaml b/examples/zipkin-tracing/service1-envoy-zipkin.yaml index 7b4dcd6ebdf8..7b592306ecb5 100644 --- a/examples/zipkin-tracing/service1-envoy-zipkin.yaml +++ b/examples/zipkin-tracing/service1-envoy-zipkin.yaml @@ -14,7 +14,7 @@ static_resources: provider: name: envoy.tracers.zipkin typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig + "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: zipkin collector_endpoint: "/api/v2/spans" collector_endpoint_version: HTTP_JSON @@ -50,7 +50,7 @@ static_resources: provider: name: envoy.tracers.zipkin typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig + "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: zipkin collector_endpoint: "/api/v2/spans" collector_endpoint_version: HTTP_JSON diff --git a/examples/zipkin-tracing/service2-envoy-zipkin.yaml b/examples/zipkin-tracing/service2-envoy-zipkin.yaml index 8e10b67d5394..bc929c5b2c2e 100644 --- a/examples/zipkin-tracing/service2-envoy-zipkin.yaml +++ b/examples/zipkin-tracing/service2-envoy-zipkin.yaml @@ -14,7 +14,7 @@ static_resources: provider: name: envoy.tracers.zipkin typed_config: - "@type": type.googleapis.com/envoy.config.trace.v2.ZipkinConfig + "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig collector_cluster: zipkin collector_endpoint: "/api/v2/spans" collector_endpoint_version: HTTP_JSON From db1a35213aa18399ba14ef545fc4894545cf68a2 Mon Sep 17 00:00:00 2001 From: Sorah Fukumori Date: Wed, 14 Oct 2020 04:56:25 +0900 Subject: [PATCH 10/14] listener: respect address.pipe.mode (it didn't work) (#13493) This patch fixes the following configuration working as expected. ```yaml static_resources: listeners: - name: http address: pipe: path: '/tmp/envoy.sock' mode: 438 # 0666 # ... ``` Risk Level: Low Testing: added integration test Docs Changes: N/A Release Notes: N/A Fixes: https://github.com/envoyproxy/envoy/issues/11809 Signed-off-by: Sorah Fukumori --- source/common/network/resolver_impl.cc | 2 +- test/common/network/address_impl_test.cc | 2 ++ test/integration/uds_integration_test.cc | 34 ++++++++++++++++++++++-- test/integration/uds_integration_test.h | 7 +++-- tools/spelling/spelling_dictionary.txt | 1 + 5 files changed, 41 insertions(+), 5 deletions(-) diff --git a/source/common/network/resolver_impl.cc b/source/common/network/resolver_impl.cc index 8aa49b8bb8bc..66554c0d2367 100644 --- a/source/common/network/resolver_impl.cc +++ b/source/common/network/resolver_impl.cc @@ -48,7 +48,7 @@ InstanceConstSharedPtr resolveProtoAddress(const envoy::config::core::v3::Addres case envoy::config::core::v3::Address::AddressCase::kSocketAddress: return resolveProtoSocketAddress(address.socket_address()); case envoy::config::core::v3::Address::AddressCase::kPipe: - return std::make_shared(address.pipe().path()); + return std::make_shared(address.pipe().path(), address.pipe().mode()); case envoy::config::core::v3::Address::AddressCase::kEnvoyInternalAddress: switch (address.envoy_internal_address().address_name_specifier_case()) { case envoy::config::core::v3::EnvoyInternalAddress::AddressNameSpecifierCase:: diff --git a/test/common/network/address_impl_test.cc b/test/common/network/address_impl_test.cc index 2090ac393345..d1a01ca6734b 100644 --- a/test/common/network/address_impl_test.cc +++ b/test/common/network/address_impl_test.cc @@ -335,6 +335,8 @@ TEST(InteralInstanceTest, Basic) { EXPECT_EQ(static_cast(0), address.sockAddrLen()); } +// Excluding Windows; chmod(2) against Windows AF_UNIX socket files succeeds, +// but stat(2) against those returns ENOENT. #ifndef WIN32 TEST(PipeInstanceTest, BasicPermission) { std::string path = TestEnvironment::unixDomainSocketPath("foo.sock"); diff --git a/test/integration/uds_integration_test.cc b/test/integration/uds_integration_test.cc index e1e63b8fbfb5..6fce94e56646 100644 --- a/test/integration/uds_integration_test.cc +++ b/test/integration/uds_integration_test.cc @@ -2,6 +2,7 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" +#include "common/api/os_sys_calls_impl.h" #include "common/event/dispatcher_impl.h" #include "common/network/utility.h" @@ -47,14 +48,20 @@ TEST_P(UdsUpstreamIntegrationTest, RouterDownstreamDisconnectBeforeResponseCompl INSTANTIATE_TEST_SUITE_P( TestParameters, UdsListenerIntegrationTest, testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - testing::Values(false, true))); + testing::Values(false, true), testing::Values(0))); #else INSTANTIATE_TEST_SUITE_P( TestParameters, UdsListenerIntegrationTest, testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - testing::Values(false))); + testing::Values(false), testing::Values(0))); #endif +// Test the mode parameter, excluding abstract namespace enabled +INSTANTIATE_TEST_SUITE_P( + TestModeParameter, UdsListenerIntegrationTest, + testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + testing::Values(false), testing::Values(0662))); + void UdsListenerIntegrationTest::initialize() { config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { auto* admin_addr = bootstrap.mutable_admin()->mutable_address(); @@ -68,6 +75,7 @@ void UdsListenerIntegrationTest::initialize() { auto* listener = listeners->Add(); listener->set_name("listener_0"); listener->mutable_address()->mutable_pipe()->set_path(getListenerSocketName()); + listener->mutable_address()->mutable_pipe()->set_mode(getMode()); *(listener->mutable_filter_chains()) = filter_chains; }); HttpIntegrationTest::initialize(); @@ -84,6 +92,28 @@ HttpIntegrationTest::ConnectionCreationFunction UdsListenerIntegrationTest::crea }; } +// Excluding Windows; chmod(2) against Windows AF_UNIX socket files succeeds, +// but stat(2) against those returns ENOENT. +#ifndef WIN32 +TEST_P(UdsListenerIntegrationTest, TestSocketMode) { + if (abstract_namespace_) { + // stat(2) against sockets in abstract namespace is not possible + GTEST_SKIP(); + } + + initialize(); + + Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get(); + struct stat listener_stat; + EXPECT_EQ(os_sys_calls.stat(getListenerSocketName().c_str(), &listener_stat).rc_, 0); + if (mode_ == 0) { + EXPECT_NE(listener_stat.st_mode & 0777, 0); + } else { + EXPECT_EQ(listener_stat.st_mode & mode_, mode_); + } +} +#endif + TEST_P(UdsListenerIntegrationTest, TestPeerCredentials) { fake_upstreams_count_ = 1; initialize(); diff --git a/test/integration/uds_integration_test.h b/test/integration/uds_integration_test.h index 9ebd30cd006e..43fdeabd5b56 100644 --- a/test/integration/uds_integration_test.h +++ b/test/integration/uds_integration_test.h @@ -54,12 +54,12 @@ class UdsUpstreamIntegrationTest }; class UdsListenerIntegrationTest - : public testing::TestWithParam>, + : public testing::TestWithParam>, public HttpIntegrationTest { public: UdsListenerIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, std::get<0>(GetParam())), - abstract_namespace_(std::get<1>(GetParam())) {} + abstract_namespace_(std::get<1>(GetParam())), mode_(std::get<2>(GetParam())) {} void initialize() override; @@ -71,10 +71,13 @@ class UdsListenerIntegrationTest return TestEnvironment::unixDomainSocketPath("listener_0.sock", abstract_namespace_); } + mode_t getMode() { return mode_; } + protected: HttpIntegrationTest::ConnectionCreationFunction createConnectionFn(); const bool abstract_namespace_; + const mode_t mode_; }; } // namespace Envoy diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 0a28f0011970..5eaaceff978f 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -95,6 +95,7 @@ EINPROGRESS EINVAL ELB EMSGSIZE +ENOENT ENOTFOUND ENOTSUP ENV From cfaefef96bd300cc3b9ab02d3ce3dede0a1a492f Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 13 Oct 2020 18:52:32 -0400 Subject: [PATCH 11/14] http: using CONNECT_ERROR for HTTP/2 (#13519) Signed-off-by: Alyssa Wilk --- docs/root/version_history/current.rst | 2 ++ include/envoy/http/codec.h | 4 ++- source/common/http/conn_manager_impl.cc | 25 ++++++++++++++++++- source/common/http/http2/codec_impl.cc | 21 +++++++++++++--- source/common/http/http2/codec_impl_legacy.cc | 18 ++++++++++--- source/common/http/utility.cc | 2 ++ source/common/router/router.cc | 5 ++-- test/common/http/http2/codec_impl_test.cc | 4 +++ test/common/http/utility_test.cc | 2 ++ test/common/router/router_test.cc | 2 +- .../integration/websocket_integration_test.cc | 2 +- test/integration/websocket_integration_test.h | 6 ++++- 12 files changed, 78 insertions(+), 15 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 6e5fc92acf08..977e3aaa8981 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -15,6 +15,8 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* +* http: sending CONNECT_ERROR for HTTP/2 where appropriate during CONNECT requests. + Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 877313e5f849..5e977c8a7d09 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -251,7 +251,9 @@ enum class StreamResetReason { // If the stream was locally reset due to connection termination. ConnectionTermination, // The stream was reset because of a resource overflow. - Overflow + Overflow, + // Either there was an early TCP error for a CONNECT request or the peer reset with CONNECT_ERROR + ConnectError }; /** diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index d108949cf165..fb87ebfa1fc9 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -52,6 +52,17 @@ namespace Envoy { namespace Http { +bool requestWasConnect(const RequestHeaderMapPtr& headers, Protocol protocol) { + if (!headers) { + return false; + } + if (protocol <= Protocol::Http11) { + return HeaderUtility::isConnect(*headers); + } + // All HTTP/2 style upgrades were originally connect requests. + return HeaderUtility::isConnect(*headers) || Utility::isUpgrade(*headers); +} + ConnectionManagerStats ConnectionManagerImpl::generateStats(const std::string& prefix, Stats::Scope& scope) { return ConnectionManagerStats( @@ -177,7 +188,19 @@ void ConnectionManagerImpl::doEndStream(ActiveStream& stream) { // TODO(snowp): This call might not be necessary, try to clean up + remove setter function. stream.filter_manager_.setLocalComplete(); stream.state_.codec_saw_local_complete_ = true; - stream.response_encoder_->getStream().resetStream(StreamResetReason::LocalReset); + + // Per https://tools.ietf.org/html/rfc7540#section-8.3 if there was an error + // with the TCP connection during a CONNECT request, it should be + // communicated via CONNECT_ERROR + if (requestWasConnect(stream.request_headers_, codec_->protocol()) && + (stream.filter_manager_.streamInfo().hasResponseFlag( + StreamInfo::ResponseFlag::UpstreamConnectionFailure) || + stream.filter_manager_.streamInfo().hasResponseFlag( + StreamInfo::ResponseFlag::UpstreamConnectionTermination))) { + stream.response_encoder_->getStream().resetStream(StreamResetReason::ConnectError); + } else { + stream.response_encoder_->getStream().resetStream(StreamResetReason::LocalReset); + } reset_stream = true; } diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 1881d71c0fb2..d7f507f3c6fe 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -64,6 +64,17 @@ class Http2ResponseCodeDetailValues { } }; +int reasonToReset(StreamResetReason reason) { + switch (reason) { + case StreamResetReason::LocalRefusedStreamReset: + return NGHTTP2_REFUSED_STREAM; + case StreamResetReason::ConnectError: + return NGHTTP2_CONNECT_ERROR; + default: + return NGHTTP2_NO_ERROR; + } +} + using Http2ResponseCodeDetails = ConstSingleton; bool Utility::reconstituteCrumbledCookies(const HeaderString& key, const HeaderString& value, @@ -526,9 +537,7 @@ void ConnectionImpl::StreamImpl::resetStream(StreamResetReason reason) { void ConnectionImpl::StreamImpl::resetStreamWorker(StreamResetReason reason) { int rc = nghttp2_submit_rst_stream(parent_.session_, NGHTTP2_FLAG_NONE, stream_id_, - reason == StreamResetReason::LocalRefusedStreamReset - ? NGHTTP2_REFUSED_STREAM - : NGHTTP2_NO_ERROR); + reasonToReset(reason)); ASSERT(rc == 0); } @@ -986,7 +995,11 @@ int ConnectionImpl::onStreamClose(int32_t stream_id, uint32_t error_code) { reason = StreamResetReason::RemoteRefusedStreamReset; stream->setDetails(Http2ResponseCodeDetails::get().remote_refused); } else { - reason = StreamResetReason::RemoteReset; + if (error_code == NGHTTP2_CONNECT_ERROR) { + reason = StreamResetReason::ConnectError; + } else { + reason = StreamResetReason::RemoteReset; + } stream->setDetails(Http2ResponseCodeDetails::get().remote_reset); } } diff --git a/source/common/http/http2/codec_impl_legacy.cc b/source/common/http/http2/codec_impl_legacy.cc index 0498c6f70655..88e596458fe9 100644 --- a/source/common/http/http2/codec_impl_legacy.cc +++ b/source/common/http/http2/codec_impl_legacy.cc @@ -63,6 +63,17 @@ class Http2ResponseCodeDetailValues { } }; +int reasonToReset(StreamResetReason reason) { + switch (reason) { + case StreamResetReason::LocalRefusedStreamReset: + return NGHTTP2_REFUSED_STREAM; + case StreamResetReason::ConnectError: + return NGHTTP2_CONNECT_ERROR; + default: + return NGHTTP2_NO_ERROR; + } +} + using Http2ResponseCodeDetails = ConstSingleton; using Http::Http2::CodecStats; using Http::Http2::MetadataDecoder; @@ -506,9 +517,7 @@ void ConnectionImpl::StreamImpl::resetStream(StreamResetReason reason) { void ConnectionImpl::StreamImpl::resetStreamWorker(StreamResetReason reason) { int rc = nghttp2_submit_rst_stream(parent_.session_, NGHTTP2_FLAG_NONE, stream_id_, - reason == StreamResetReason::LocalRefusedStreamReset - ? NGHTTP2_REFUSED_STREAM - : NGHTTP2_NO_ERROR); + reasonToReset(reason)); ASSERT(rc == 0); } @@ -954,6 +963,9 @@ int ConnectionImpl::onStreamClose(int32_t stream_id, uint32_t error_code) { if (error_code == NGHTTP2_REFUSED_STREAM) { reason = StreamResetReason::RemoteRefusedStreamReset; stream->setDetails(Http2ResponseCodeDetails::get().remote_refused); + } else if (error_code == NGHTTP2_CONNECT_ERROR) { + reason = StreamResetReason::ConnectError; + stream->setDetails(Http2ResponseCodeDetails::get().remote_reset); } else { reason = StreamResetReason::RemoteReset; stream->setDetails(Http2ResponseCodeDetails::get().remote_reset); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index d522d61e237a..cb018c0c18f2 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -819,6 +819,8 @@ const std::string Utility::resetReasonToString(const Http::StreamResetReason res return "remote reset"; case Http::StreamResetReason::RemoteRefusedStreamReset: return "remote refused stream reset"; + case Http::StreamResetReason::ConnectError: + return "remote error with CONNECT request"; } NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 5e6702c3cb75..739050410f74 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -968,6 +968,7 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_ absl::string_view body, bool dropped, absl::string_view details) { // If we have not yet sent anything downstream, send a response with an appropriate status code. // Otherwise just reset the ongoing response. + callbacks_->streamInfo().setResponseFlag(response_flags); if (downstream_response_started_ && !Runtime::runtimeFeatureEnabled("envoy.reloadable_features.allow_500_after_100")) { // This will destroy any created retry timers. @@ -977,9 +978,6 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_ } else { // This will destroy any created retry timers. cleanup(); - - callbacks_->streamInfo().setResponseFlag(response_flags); - // sendLocalReply may instead reset the stream if downstream_response_started_ is true. callbacks_->sendLocalReply( code, body, @@ -1092,6 +1090,7 @@ Filter::streamResetReasonToResponseFlag(Http::StreamResetReason reset_reason) { return StreamInfo::ResponseFlag::UpstreamOverflow; case Http::StreamResetReason::RemoteReset: case Http::StreamResetReason::RemoteRefusedStreamReset: + case Http::StreamResetReason::ConnectError: return StreamInfo::ResponseFlag::UpstreamRemoteReset; } diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index f616a9e0b89f..bc8cd64953df 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -2662,6 +2662,10 @@ TEST_P(Http2CodecImplTest, ConnectTest) { expected_headers.setReferenceKey(Headers::get().Protocol, "bytestream"); EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); request_encoder_->encodeHeaders(request_headers, false); + + EXPECT_CALL(callbacks, onResetStream(StreamResetReason::ConnectError, _)); + EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::ConnectError, _)); + response_encoder_->getStream().resetStream(StreamResetReason::ConnectError); } template class TestNghttp2SessionFactory; diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 87dabd8f087a..447ed9a54d84 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -790,6 +790,8 @@ TEST(HttpUtility, ResetReasonToString) { EXPECT_EQ("remote reset", Utility::resetReasonToString(Http::StreamResetReason::RemoteReset)); EXPECT_EQ("remote refused stream reset", Utility::resetReasonToString(Http::StreamResetReason::RemoteRefusedStreamReset)); + EXPECT_EQ("remote error with CONNECT request", + Utility::resetReasonToString(Http::StreamResetReason::ConnectError)); } // Verify that it resolveMostSpecificPerFilterConfigGeneric works with nil routes. diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 6b893358a704..752bedba83ea 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -2330,9 +2330,9 @@ TEST_F(RouterTest, UpstreamPerTryTimeoutExcludesNewStream) { EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putResult(Upstream::Outlier::Result::LocalOriginTimeout, _)); EXPECT_CALL(*per_try_timeout_, disableTimer()); - EXPECT_CALL(*response_timeout_, disableTimer()); EXPECT_CALL(callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::UpstreamRequestTimeout)); + EXPECT_CALL(*response_timeout_, disableTimer()); Http::TestResponseHeaderMapImpl response_headers{ {":status", "504"}, {"content-length", "24"}, {"content-type", "text/plain"}}; EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false)); diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index a7e92f4bca6f..cf6682610975 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -208,7 +208,7 @@ TEST_P(WebsocketIntegrationTest, WebSocketConnectionUpstreamDisconnect) { // Verify both the data and the disconnect went through. response_->waitForBodyData(5); EXPECT_EQ("world", response_->body()); - waitForClientDisconnectOrReset(); + waitForClientDisconnectOrReset(Http::StreamResetReason::ConnectError); } TEST_P(WebsocketIntegrationTest, EarlyData) { diff --git a/test/integration/websocket_integration_test.h b/test/integration/websocket_integration_test.h index c060f043c732..5f3aae0edf99 100644 --- a/test/integration/websocket_integration_test.h +++ b/test/integration/websocket_integration_test.h @@ -1,5 +1,7 @@ #pragma once +#include "envoy/http/codec.h" + #include "test/integration/http_protocol_integration.h" #include "gtest/gtest.h" @@ -35,9 +37,11 @@ class WebsocketIntegrationTest : public HttpProtocolIntegrationTest { } } - void waitForClientDisconnectOrReset() { + void waitForClientDisconnectOrReset( + Http::StreamResetReason reason = Http::StreamResetReason::RemoteReset) { if (downstreamProtocol() != Http::CodecClient::Type::HTTP1) { response_->waitForReset(); + ASSERT_EQ(reason, response_->resetReason()); } else { ASSERT_TRUE(codec_client_->waitForDisconnect()); } From a1a9b5f8817a78bcae035b0e1173075202dbd68f Mon Sep 17 00:00:00 2001 From: Sunjay Bhatia <5337253+sunjayBhatia@users.noreply.github.com> Date: Tue, 13 Oct 2020 23:03:58 -0400 Subject: [PATCH 12/14] Add Platform Specific Feature guidance to PR template (#13547) Signed-off-by: Sunjay Bhatia --- PULL_REQUESTS.md | 10 ++++++++++ PULL_REQUEST_TEMPLATE.md | 1 + 2 files changed, 11 insertions(+) diff --git a/PULL_REQUESTS.md b/PULL_REQUESTS.md index 9b3d5cd043ba..aa8c5e750247 100644 --- a/PULL_REQUESTS.md +++ b/PULL_REQUESTS.md @@ -70,6 +70,16 @@ current version. Please include any relevant links. Each release note should be relevant subsystem in **alphabetical order** (see existing examples as a guide) and include links to relevant parts of the documentation. Thank you! Please write in N/A if there are no release notes. +### Platform Specific Features + +If this change involves any platform specific features (e.g. utilizing OS-specific socket options) +or only implements new features for a limited set of platforms (e.g. Linux amd64 only), please +include an explanation that addresses the reasoning behind this. Please also open a new tracking +issue for each platform this change is not implemented on (and link them in the PR) to enable +maintainers and contributors to triage. Reviewers will look for the change to avoid +`#ifdef ` and rather prefer feature guards to not enable the change on a given platform +using the build system. + ### Runtime guard If this PR has a user-visible behavioral change, or otherwise falls under the diff --git a/PULL_REQUEST_TEMPLATE.md b/PULL_REQUEST_TEMPLATE.md index 5a1545aacd7a..366209eed929 100644 --- a/PULL_REQUEST_TEMPLATE.md +++ b/PULL_REQUEST_TEMPLATE.md @@ -18,6 +18,7 @@ Risk Level: Testing: Docs Changes: Release Notes: +Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Deprecated:] From bc5c44a1a842379daa6f810dc3841c71a513661b Mon Sep 17 00:00:00 2001 From: Zach Reyes <39203661+zasweq@users.noreply.github.com> Date: Tue, 13 Oct 2020 23:04:36 -0400 Subject: [PATCH 13/14] [fuzz] Added validation for secrets (#13543) Signed-off-by: Zach --- api/envoy/extensions/transport_sockets/tls/v3/secret.proto | 6 +++++- .../extensions/transport_sockets/tls/v4alpha/secret.proto | 3 ++- .../envoy/extensions/transport_sockets/tls/v3/secret.proto | 6 +++++- .../extensions/transport_sockets/tls/v4alpha/secret.proto | 3 ++- .../clusterfuzz-testcase-filter_fuzz_test-5681522444861440 | 7 +++++++ 5 files changed, 21 insertions(+), 4 deletions(-) create mode 100644 test/extensions/filters/http/common/fuzz/filter_corpus/clusterfuzz-testcase-filter_fuzz_test-5681522444861440 diff --git a/api/envoy/extensions/transport_sockets/tls/v3/secret.proto b/api/envoy/extensions/transport_sockets/tls/v3/secret.proto index 80c68a56f5ce..f25370c3c9f6 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/secret.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/secret.proto @@ -12,6 +12,7 @@ import "udpa/annotations/migrate.proto"; import "udpa/annotations/sensitive.proto"; import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; +import "validate/validate.proto"; option java_package = "io.envoyproxy.envoy.extensions.transport_sockets.tls.v3"; option java_outer_classname = "SecretProto"; @@ -33,7 +34,10 @@ message SdsSecretConfig { // Name (FQDN, UUID, SPKI, SHA256, etc.) by which the secret can be uniquely referred to. // When both name and config are specified, then secret can be fetched and/or reloaded via // SDS. When only name is specified, then secret will be loaded from static resources. - string name = 1 [(udpa.annotations.field_migrate).oneof_promotion = "name_specifier"]; + string name = 1 [ + (validate.rules).string = {min_len: 1}, + (udpa.annotations.field_migrate).oneof_promotion = "name_specifier" + ]; // Resource locator for SDS. This is mutually exclusive to *name*. // [#not-implemented-hide:] diff --git a/api/envoy/extensions/transport_sockets/tls/v4alpha/secret.proto b/api/envoy/extensions/transport_sockets/tls/v4alpha/secret.proto index 11306f21415a..9848eaadef0b 100644 --- a/api/envoy/extensions/transport_sockets/tls/v4alpha/secret.proto +++ b/api/envoy/extensions/transport_sockets/tls/v4alpha/secret.proto @@ -11,6 +11,7 @@ import "udpa/core/v1/resource_locator.proto"; import "udpa/annotations/sensitive.proto"; import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; +import "validate/validate.proto"; option java_package = "io.envoyproxy.envoy.extensions.transport_sockets.tls.v4alpha"; option java_outer_classname = "SecretProto"; @@ -35,7 +36,7 @@ message SdsSecretConfig { // Name (FQDN, UUID, SPKI, SHA256, etc.) by which the secret can be uniquely referred to. // When both name and config are specified, then secret can be fetched and/or reloaded via // SDS. When only name is specified, then secret will be loaded from static resources. - string name = 1; + string name = 1 [(validate.rules).string = {min_len: 1}]; // Resource locator for SDS. This is mutually exclusive to *name*. // [#not-implemented-hide:] diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/secret.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/secret.proto index 80c68a56f5ce..f25370c3c9f6 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/secret.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/secret.proto @@ -12,6 +12,7 @@ import "udpa/annotations/migrate.proto"; import "udpa/annotations/sensitive.proto"; import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; +import "validate/validate.proto"; option java_package = "io.envoyproxy.envoy.extensions.transport_sockets.tls.v3"; option java_outer_classname = "SecretProto"; @@ -33,7 +34,10 @@ message SdsSecretConfig { // Name (FQDN, UUID, SPKI, SHA256, etc.) by which the secret can be uniquely referred to. // When both name and config are specified, then secret can be fetched and/or reloaded via // SDS. When only name is specified, then secret will be loaded from static resources. - string name = 1 [(udpa.annotations.field_migrate).oneof_promotion = "name_specifier"]; + string name = 1 [ + (validate.rules).string = {min_len: 1}, + (udpa.annotations.field_migrate).oneof_promotion = "name_specifier" + ]; // Resource locator for SDS. This is mutually exclusive to *name*. // [#not-implemented-hide:] diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/secret.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/secret.proto index 11306f21415a..9848eaadef0b 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/secret.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/secret.proto @@ -11,6 +11,7 @@ import "udpa/core/v1/resource_locator.proto"; import "udpa/annotations/sensitive.proto"; import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; +import "validate/validate.proto"; option java_package = "io.envoyproxy.envoy.extensions.transport_sockets.tls.v4alpha"; option java_outer_classname = "SecretProto"; @@ -35,7 +36,7 @@ message SdsSecretConfig { // Name (FQDN, UUID, SPKI, SHA256, etc.) by which the secret can be uniquely referred to. // When both name and config are specified, then secret can be fetched and/or reloaded via // SDS. When only name is specified, then secret will be loaded from static resources. - string name = 1; + string name = 1 [(validate.rules).string = {min_len: 1}]; // Resource locator for SDS. This is mutually exclusive to *name*. // [#not-implemented-hide:] diff --git a/test/extensions/filters/http/common/fuzz/filter_corpus/clusterfuzz-testcase-filter_fuzz_test-5681522444861440 b/test/extensions/filters/http/common/fuzz/filter_corpus/clusterfuzz-testcase-filter_fuzz_test-5681522444861440 new file mode 100644 index 000000000000..60ffb84c5ac3 --- /dev/null +++ b/test/extensions/filters/http/common/fuzz/filter_corpus/clusterfuzz-testcase-filter_fuzz_test-5681522444861440 @@ -0,0 +1,7 @@ +config { + name: "envoy.filters.http.oauth" + typed_config { + type_url: "type.googleapis.com/envoy.extensions.filters.http.oauth2.v3alpha.OAuth2" + value: "\n\306\t\022\006\022\001(\032\001r\032<\n\035envoy.filters.\360\222\213\217Qgrpc_stats\022\r\022\013\022\002\010\006\"\005\010\200\200\200\001\032\014\022\n\n\001t\"\005\010\200\200\200\001\"\006\022\001(\032\001r*\005\n\003:\001=2\351\010\n\346\010*\343\010\n\010\n\006\010\200\200\200\200\004\022\326\010^^^^^j!^^.*..............................................*............................config {\n name: \"envoy.filters.http.jwt_authn\"\n typed....._config {\n type_url: \"type.googleapis.com/envoy.extensions.filters.http.jwt_authn.v3.JwtAu........[thentication\"\n value: \"\\n=\\n\\022not_health_check_f\\022\\\'\\032\\010\\n\\006\\n\\004\\177\\177\\177\\177B\\033envoyype/matcher/v3/number.\\n1\\n\\0A_]^06\\000\\000\\000\\000\\000\\002\\022\\\'\\032\\010\\n\\006\\n\\004\\177\\177\\177\\177B\\033envoyype/matche!^^.*..............................................*............................config {\n name: \"envoy.filters.http.jwt_authn\"\n typed....._config {\n type_url: \"type.googleapis.com/envoy.extensions.filters.http.jwt_authn.v3.JwtAu........[thentication\"\n value: \"\\n=\\n\\022not_health_check_f\\022\\\'\\032\\010\\n\\006\\n\\004\\177\\177\\177\\177B\\033envoyype/matcher/v3/number.\\n1\\n\\0A_]^06\\000\\000\\000\\000\\000\\002\\022\\\'\\032\\010\\n\\006\\n\\004\\177\\177\\177\\177B\\033envoyype/matcher/v3/number.\\n+\\n\\000\\022\\\'\\032\\010\\n\\006\\n\\004\\177\\177\\177\\177B\\r/v3/number.\\n+\\n\\000\\022\\\'\\032\\010\\n\\006\\n\\004\\177\\177\\177\\177B\\033envoyype/matcher/v3/number.\"\n }\n}\nB\003\n\001A" + } +} From 78dfea2cc80da8a89d868dc87a4b14f48d141500 Mon Sep 17 00:00:00 2001 From: Pengyuan Bian Date: Tue, 13 Oct 2020 20:25:58 -0700 Subject: [PATCH 14/14] [Wasm] Add cluster metadata fallback and upstream host metadata (#13477) Signed-off-by: Pengyuan Bian --- source/extensions/common/wasm/context.cc | 11 +++- .../filters/http/wasm/test_data/test_cpp.cc | 10 +++- .../filters/http/wasm/wasm_filter_test.cc | 50 +++++++++++++++++++ 3 files changed, 69 insertions(+), 2 deletions(-) diff --git a/source/extensions/common/wasm/context.cc b/source/extensions/common/wasm/context.cc index a064a229492f..e6e4f8ae0f05 100644 --- a/source/extensions/common/wasm/context.cc +++ b/source/extensions/common/wasm/context.cc @@ -403,7 +403,8 @@ WasmResult serializeValue(Filters::Common::Expr::CelValue value, std::string* re _f(METADATA) _f(REQUEST) _f(RESPONSE) _f(CONNECTION) _f(UPSTREAM) _f(NODE) _f(SOURCE) \ _f(DESTINATION) _f(LISTENER_DIRECTION) _f(LISTENER_METADATA) _f(CLUSTER_NAME) \ _f(CLUSTER_METADATA) _f(ROUTE_NAME) _f(ROUTE_METADATA) _f(PLUGIN_NAME) \ - _f(PLUGIN_ROOT_ID) _f(PLUGIN_VM_ID) _f(CONNECTION_ID) _f(FILTER_STATE) + _f(UPSTREAM_HOST_METADATA) _f(PLUGIN_ROOT_ID) _f(PLUGIN_VM_ID) _f(CONNECTION_ID) \ + _f(FILTER_STATE) static inline std::string downCase(std::string s) { std::transform(s.begin(), s.end(), s.begin(), [](unsigned char c) { return std::tolower(c); }); @@ -524,6 +525,14 @@ Context::findValue(absl::string_view name, Protobuf::Arena* arena, bool last) co case PropertyToken::CLUSTER_METADATA: if (info && info->upstreamHost()) { return CelValue::CreateMessage(&info->upstreamHost()->cluster().metadata(), arena); + } else if (info && info->upstreamClusterInfo().has_value() && + info->upstreamClusterInfo().value()) { + return CelValue::CreateMessage(&info->upstreamClusterInfo().value()->metadata(), arena); + } + break; + case PropertyToken::UPSTREAM_HOST_METADATA: + if (info && info->upstreamHost() && info->upstreamHost()->metadata()) { + return CelValue::CreateMessage(info->upstreamHost()->metadata().get(), arena); } break; case PropertyToken::ROUTE_NAME: diff --git a/test/extensions/filters/http/wasm/test_data/test_cpp.cc b/test/extensions/filters/http/wasm/test_data/test_cpp.cc index fe5d7722a6e3..3e009a3da8c3 100644 --- a/test/extensions/filters/http/wasm/test_data/test_cpp.cc +++ b/test/extensions/filters/http/wasm/test_data/test_cpp.cc @@ -328,6 +328,11 @@ void TestContext::onLog() { if (response_trailer && response_trailer->view() != "") { logWarn("response bogus-trailer found"); } + } else if (test == "cluster_metadata") { + std::string cluster_metadata; + if (getValue({"cluster_metadata", "filter_metadata", "namespace", "key"}, &cluster_metadata)) { + logWarn("cluster metadata: " + cluster_metadata); + } } else if (test == "property") { setFilterState("wasm_state", "wasm_value"); auto path = getRequestHeader(":path"); @@ -343,6 +348,10 @@ void TestContext::onLog() { if (getValue({"response", "code"}, &responseCode)) { logWarn("response.code: " + std::to_string(responseCode)); } + std::string upstream_host_metadata; + if (getValue({"upstream_host_metadata", "filter_metadata", "namespace", "key"}, &upstream_host_metadata)) { + logWarn("upstream host metadata: " + upstream_host_metadata); + } logWarn("state: " + getProperty({"wasm_state"}).value()->toString()); } else { logWarn("onLog " + std::to_string(id()) + " " + std::string(path->view())); @@ -541,7 +550,6 @@ void TestContext::onLog() { {{"source", "address"}, "127.0.0.1:0"}, {{"destination", "address"}, "127.0.0.2:0"}, {{"upstream", "address"}, "10.0.0.1:443"}, - {{"cluster_metadata"}, ""}, {{"route_metadata"}, ""}, }; for (const auto& property : properties) { diff --git a/test/extensions/filters/http/wasm/wasm_filter_test.cc b/test/extensions/filters/http/wasm/wasm_filter_test.cc index b001368763b0..538481f36f6c 100644 --- a/test/extensions/filters/http/wasm/wasm_filter_test.cc +++ b/test/extensions/filters/http/wasm/wasm_filter_test.cc @@ -1294,6 +1294,8 @@ TEST_P(WasmHttpFilterTest, Property) { log_(spdlog::level::warn, Eq(absl::string_view("metadata: wasm_request_get_value")))); EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("response.code: 403")))); EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("state: wasm_value")))); + EXPECT_CALL(filter(), + log_(spdlog::level::warn, Eq(absl::string_view("upstream host metadata: endpoint")))); root_context_->onTick(0); Http::TestRequestHeaderMapImpl request_headers{{":path", "/test_context"}}; @@ -1306,6 +1308,54 @@ TEST_P(WasmHttpFilterTest, Property) { EXPECT_CALL(encoder_callbacks_, connection()).WillRepeatedly(Return(&connection)); NiceMock route_entry; EXPECT_CALL(request_stream_info_, routeEntry()).WillRepeatedly(Return(&route_entry)); + std::shared_ptr> host_description( + new NiceMock()); + auto metadata = std::make_shared( + TestUtility::parseYaml( + R"EOF( + filter_metadata: + namespace: + key: endpoint + )EOF")); + EXPECT_CALL(*host_description, metadata()).WillRepeatedly(Return(metadata)); + EXPECT_CALL(request_stream_info_, upstreamHost()).WillRepeatedly(Return(host_description)); + filter().log(&request_headers, nullptr, nullptr, log_stream_info); +} + +TEST_P(WasmHttpFilterTest, ClusterMetadata) { + if (std::get<1>(GetParam()) == "rust") { + // TODO(PiotrSikora): test not yet implemented using Rust SDK. + return; + } + setupTest("", "cluster_metadata"); + setupFilter(); + EXPECT_CALL(filter(), + log_(spdlog::level::warn, Eq(absl::string_view("cluster metadata: cluster")))); + auto cluster = std::make_shared>(); + auto cluster_metadata = std::make_shared( + TestUtility::parseYaml( + R"EOF( + filter_metadata: + namespace: + key: cluster + )EOF")); + + std::shared_ptr> host_description( + new NiceMock()); + StreamInfo::MockStreamInfo log_stream_info; + Http::TestRequestHeaderMapImpl request_headers{{}}; + + EXPECT_CALL(encoder_callbacks_, streamInfo()).WillRepeatedly(ReturnRef(request_stream_info_)); + EXPECT_CALL(*cluster, metadata()).WillRepeatedly(ReturnRef(*cluster_metadata)); + EXPECT_CALL(*host_description, cluster()).WillRepeatedly(ReturnRef(*cluster)); + EXPECT_CALL(request_stream_info_, upstreamHost()).WillRepeatedly(Return(host_description)); + filter().log(&request_headers, nullptr, nullptr, log_stream_info); + + // If upstream host is empty, fallback to upstream cluster info for cluster metadata. + EXPECT_CALL(request_stream_info_, upstreamHost()).WillRepeatedly(Return(nullptr)); + EXPECT_CALL(request_stream_info_, upstreamClusterInfo()).WillRepeatedly(Return(cluster)); + EXPECT_CALL(filter(), + log_(spdlog::level::warn, Eq(absl::string_view("cluster metadata: cluster")))); filter().log(&request_headers, nullptr, nullptr, log_stream_info); }