[http] Initial codec splitting with test parametrization (envoyproxy#…

…10591) This just introduces plumbing and clones codecs for a legacy versions. There is no changes to the codecs at this point (the files are just duplicated and the namespace Legacy is used). Users can set which codecs are used at runtime via the feature new_codec_behavior. This will be the case for O(4-6 weeks) while the change to new no-exception codecs are pushed (see issue linked) to the new codecs. Format scripts check that h/1 header and source, h/2 header and source, and h/2 codec tests are kept in sync to enforce that devs who make changes to one codec port them to the old one. Devs will need to update the diff if they are making a change that exists in one and not the other. The format script will alert them on what to change in the diff script. Signed-off-by: Asra Ali <[email protected]>
scheler · Jul 16, 2020 · 568b759 · 568b759
1 parent b7a4788
commit 568b759
Show file tree

Hide file tree

Showing 43 changed files with 7,201 additions and 358 deletions.
diff --git a/bazel/BUILD b/bazel/BUILD
@@ -199,6 +199,11 @@ config_setting(
     values = {"define": "path_normalization_by_default=true"},
 )
 
+config_setting(
+    name = "enable_legacy_codecs_in_integration_tests",
+    values = {"define": "use_legacy_codecs_in_integration_tests=true"},
+)
+
 cc_proto_library(
     name = "grpc_health_proto",
     deps = ["@com_github_grpc_grpc//src/proto/grpc/health/v1:_health_proto_only"],

diff --git a/bazel/envoy_build_system.bzl b/bazel/envoy_build_system.bzl
@@ -18,6 +18,7 @@ load(
     _envoy_select_boringssl = "envoy_select_boringssl",
     _envoy_select_google_grpc = "envoy_select_google_grpc",
     _envoy_select_hot_restart = "envoy_select_hot_restart",
+    _envoy_select_legacy_codecs_in_integration_tests = "envoy_select_legacy_codecs_in_integration_tests",
 )
 load(
     ":envoy_test.bzl",
@@ -168,6 +169,7 @@ def envoy_google_grpc_external_deps():
 envoy_select_boringssl = _envoy_select_boringssl
 envoy_select_google_grpc = _envoy_select_google_grpc
 envoy_select_hot_restart = _envoy_select_hot_restart
+envoy_select_legacy_codecs_in_integration_tests = _envoy_select_legacy_codecs_in_integration_tests
 
 # Binary wrappers (from envoy_binary.bzl)
 envoy_cc_binary = _envoy_cc_binary

diff --git a/bazel/envoy_select.bzl b/bazel/envoy_select.bzl
@@ -31,3 +31,10 @@ def envoy_select_hot_restart(xs, repository = ""):
         repository + "//bazel:disable_hot_restart_or_apple": [],
         "//conditions:default": xs,
     })
+
+# Select the given values if use legacy codecs in test is on in the current build.
+def envoy_select_legacy_codecs_in_integration_tests(xs, repository = ""):
+    return select({
+        repository + "//bazel:enable_legacy_codecs_in_integration_tests": xs,
+        "//conditions:default": [],
+    })
diff --git a/ci/do_ci.sh b/ci/do_ci.sh
@@ -220,6 +220,7 @@ elif [[ "$CI_TARGET" == "bazel.compile_time_options" ]]; then
     --define quiche=enabled \
     --define path_normalization_by_default=true \
     --define deprecated_features=disabled \
+    --define use_legacy_codecs_in_integration_tests=true \
     --define --cxxopt=-std=c++14 \
   "
   ENVOY_STDLIB="${ENVOY_STDLIB:-libstdc++}"
@@ -237,6 +238,10 @@ elif [[ "$CI_TARGET" == "bazel.compile_time_options" ]]; then
   echo "Building and testing ${TEST_TARGETS}"
   bazel test ${BAZEL_BUILD_OPTIONS} ${COMPILE_TIME_OPTIONS} -c dbg ${TEST_TARGETS} --test_tag_filters=-nofips --build_tests_only
 
+  # Legacy codecs "--define legacy_codecs_in_integration_tests=true" should also be tested in
+  # integration tests with asan.
+  bazel test ${BAZEL_BUILD_OPTIONS} ${COMPILE_TIME_OPTIONS} -c dbg @envoy//test/integration/... --config=clang-asan --build_tests_only
+
   # "--define log_debug_assert_in_release=enabled" must be tested with a release build, so run only
   # these tests under "-c opt" to save time in CI.
   bazel test ${BAZEL_BUILD_OPTIONS} ${COMPILE_TIME_OPTIONS} -c opt @envoy//test/common/common:assert_test @envoy//test/server:server_test

diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -36,6 +36,7 @@ New Features
 
 * ext_authz filter: added support for emitting dynamic metadata for both :ref:`HTTP <config_http_filters_ext_authz_dynamic_metadata>` and :ref:`network <config_network_filters_ext_authz_dynamic_metadata>` filters.
 * grpc-json: support specifying `response_body` field in for `google.api.HttpBody` message.
+* http: introduced new HTTP/1 and HTTP/2 codec implementations that will remove the use of exceptions for control flow due to high risk factors and instead use error statuses. The old behavior is deprecated, but can be used during the removal period by setting the runtime feature `envoy.reloadable_features.new_codec_behavior` to false. The removal period will be one month.
 * load balancer: added a :ref:`configuration<envoy_v3_api_msg_config.cluster.v3.Cluster.LeastRequestLbConfig>` option to specify the active request bias used by the least request load balancer.
 * tap: added :ref:`generic body matcher<envoy_v3_api_msg_config.tap.v3.HttpGenericBodyMatch>` to scan http requests and responses for text or hex patterns.
 

diff --git a/source/common/http/BUILD b/source/common/http/BUILD
@@ -57,16 +57,21 @@ envoy_cc_library(
         "//include/envoy/http:codec_interface",
         "//include/envoy/network:connection_interface",
         "//include/envoy/network:filter_interface",
+        "//include/envoy/runtime:runtime_interface",
         "//source/common/common:assert_lib",
         "//source/common/common:enum_to_int",
         "//source/common/common:linked_object",
         "//source/common/common:minimal_logger_lib",
         "//source/common/config:utility_lib",
+        "//source/common/http/http1:codec_legacy_lib",
         "//source/common/http/http1:codec_lib",
+        "//source/common/http/http2:codec_legacy_lib",
         "//source/common/http/http2:codec_lib",
         "//source/common/http/http3:quic_codec_factory_lib",
         "//source/common/http/http3:well_known_names",
         "//source/common/network:filter_lib",
+        "//source/common/runtime:runtime_features_lib",
+        "//source/common/runtime:runtime_lib",
     ],
 )
 
@@ -207,7 +212,9 @@ envoy_cc_library(
         "//source/common/common:scope_tracker",
         "//source/common/common:utility_lib",
         "//source/common/config:utility_lib",
+        "//source/common/http/http1:codec_legacy_lib",
         "//source/common/http/http1:codec_lib",
+        "//source/common/http/http2:codec_legacy_lib",
         "//source/common/http/http2:codec_lib",
         "//source/common/http/http3:quic_codec_factory_lib",
         "//source/common/http/http3:well_known_names",

diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc
@@ -9,11 +9,15 @@
 #include "common/config/utility.h"
 #include "common/http/exception.h"
 #include "common/http/http1/codec_impl.h"
+#include "common/http/http1/codec_impl_legacy.h"
 #include "common/http/http2/codec_impl.h"
+#include "common/http/http2/codec_impl_legacy.h"
 #include "common/http/http3/quic_codec_factory.h"
 #include "common/http/http3/well_known_names.h"
 #include "common/http/status.h"
 #include "common/http/utility.h"
+#include "common/runtime/runtime_features.h"
+#include "common/runtime/runtime_impl.h"
 
 namespace Envoy {
 namespace Http {
@@ -150,23 +154,37 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne
 
   switch (type) {
   case Type::HTTP1: {
-    codec_ = std::make_unique<Http1::ClientConnectionImpl>(
-        *connection_, host->cluster().http1CodecStats(), *this, host->cluster().http1Settings(),
-        host->cluster().maxResponseHeadersCount());
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior")) {
+      codec_ = std::make_unique<Http1::ClientConnectionImpl>(
+          *connection_, host->cluster().http1CodecStats(), *this, host->cluster().http1Settings(),
+          host->cluster().maxResponseHeadersCount());
+    } else {
+      codec_ = std::make_unique<Legacy::Http1::ClientConnectionImpl>(
+          *connection_, host->cluster().http1CodecStats(), *this, host->cluster().http1Settings(),
+          host->cluster().maxResponseHeadersCount());
+    }
     break;
   }
   case Type::HTTP2: {
-    codec_ = std::make_unique<Http2::ClientConnectionImpl>(
-        *connection_, *this, host->cluster().http2CodecStats(), host->cluster().http2Options(),
-        Http::DEFAULT_MAX_REQUEST_HEADERS_KB, host->cluster().maxResponseHeadersCount(),
-        Http2::ProdNghttp2SessionFactory::get());
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior")) {
+      codec_ = std::make_unique<Http2::ClientConnectionImpl>(
+          *connection_, *this, host->cluster().http2CodecStats(), host->cluster().http2Options(),
+          Http::DEFAULT_MAX_REQUEST_HEADERS_KB, host->cluster().maxResponseHeadersCount(),
+          Http2::ProdNghttp2SessionFactory::get());
+    } else {
+      codec_ = std::make_unique<Http2::ClientConnectionImpl>(
+          *connection_, *this, host->cluster().http2CodecStats(), host->cluster().http2Options(),
+          Http::DEFAULT_MAX_REQUEST_HEADERS_KB, host->cluster().maxResponseHeadersCount(),
+          Http2::ProdNghttp2SessionFactory::get());
+    }
     break;
   }
   case Type::HTTP3: {
     codec_ = std::unique_ptr<ClientConnection>(
         Config::Utility::getAndCheckFactoryByName<Http::QuicHttpClientConnectionFactory>(
             Http::QuicCodecNames::get().Quiche)
             .createQuicClientConnection(*connection_, *this));
+    break;
   }
   }
 }

diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc
@@ -11,7 +11,9 @@
 #include "common/http/header_utility.h"
 #include "common/http/headers.h"
 #include "common/http/http1/codec_impl.h"
+#include "common/http/http1/codec_impl_legacy.h"
 #include "common/http/http2/codec_impl.h"
+#include "common/http/http2/codec_impl_legacy.h"
 #include "common/http/path_utility.h"
 #include "common/http/utility.h"
 #include "common/network/utility.h"
@@ -51,14 +53,26 @@ ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec(
         headers_with_underscores_action) {
   if (determineNextProtocol(connection, data) == Utility::AlpnNames::get().Http2) {
     Http2::CodecStats& stats = Http2::CodecStats::atomicGet(http2_codec_stats, scope);
-    return std::make_unique<Http2::ServerConnectionImpl>(
-        connection, callbacks, stats, http2_options, max_request_headers_kb,
-        max_request_headers_count, headers_with_underscores_action);
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior")) {
+      return std::make_unique<Http2::ServerConnectionImpl>(
+          connection, callbacks, stats, http2_options, max_request_headers_kb,
+          max_request_headers_count, headers_with_underscores_action);
+    } else {
+      return std::make_unique<Legacy::Http2::ServerConnectionImpl>(
+          connection, callbacks, stats, http2_options, max_request_headers_kb,
+          max_request_headers_count, headers_with_underscores_action);
+    }
   } else {
     Http1::CodecStats& stats = Http1::CodecStats::atomicGet(http1_codec_stats, scope);
-    return std::make_unique<Http1::ServerConnectionImpl>(
-        connection, stats, callbacks, http1_settings, max_request_headers_kb,
-        max_request_headers_count, headers_with_underscores_action);
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior")) {
+      return std::make_unique<Http1::ServerConnectionImpl>(
+          connection, stats, callbacks, http1_settings, max_request_headers_kb,
+          max_request_headers_count, headers_with_underscores_action);
+    } else {
+      return std::make_unique<Legacy::Http1::ServerConnectionImpl>(
+          connection, stats, callbacks, http1_settings, max_request_headers_kb,
+          max_request_headers_count, headers_with_underscores_action);
+    }
   }
 }
 

diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD
@@ -24,36 +24,46 @@ envoy_cc_library(
     ],
 )
 
+CODEC_LIB_DEPS = [
+    ":codec_stats_lib",
+    ":header_formatter_lib",
+    "//include/envoy/buffer:buffer_interface",
+    "//include/envoy/http:codec_interface",
+    "//include/envoy/http:header_map_interface",
+    "//include/envoy/network:connection_interface",
+    "//source/common/buffer:buffer_lib",
+    "//source/common/buffer:watermark_buffer_lib",
+    "//source/common/common:assert_lib",
+    "//source/common/common:statusor_lib",
+    "//source/common/common:utility_lib",
+    "//source/common/grpc:common_lib",
+    "//source/common/http:codec_helper_lib",
+    "//source/common/http:codes_lib",
+    "//source/common/http:exception_lib",
+    "//source/common/http:header_map_lib",
+    "//source/common/http:header_utility_lib",
+    "//source/common/http:headers_lib",
+    "//source/common/http:status_lib",
+    "//source/common/http:url_utility_lib",
+    "//source/common/http:utility_lib",
+    "//source/common/runtime:runtime_features_lib",
+    "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
+]
+
 envoy_cc_library(
     name = "codec_lib",
     srcs = ["codec_impl.cc"],
     hdrs = ["codec_impl.h"],
     external_deps = ["http_parser"],
-    deps = [
-        ":codec_stats_lib",
-        ":header_formatter_lib",
-        "//include/envoy/buffer:buffer_interface",
-        "//include/envoy/http:codec_interface",
-        "//include/envoy/http:header_map_interface",
-        "//include/envoy/network:connection_interface",
-        "//source/common/buffer:buffer_lib",
-        "//source/common/buffer:watermark_buffer_lib",
-        "//source/common/common:assert_lib",
-        "//source/common/common:statusor_lib",
-        "//source/common/common:utility_lib",
-        "//source/common/grpc:common_lib",
-        "//source/common/http:codec_helper_lib",
-        "//source/common/http:codes_lib",
-        "//source/common/http:exception_lib",
-        "//source/common/http:header_map_lib",
-        "//source/common/http:header_utility_lib",
-        "//source/common/http:headers_lib",
-        "//source/common/http:status_lib",
-        "//source/common/http:url_utility_lib",
-        "//source/common/http:utility_lib",
-        "//source/common/runtime:runtime_features_lib",
-        "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
-    ],
+    deps = CODEC_LIB_DEPS,
+)
+
+envoy_cc_library(
+    name = "codec_legacy_lib",
+    srcs = ["codec_impl_legacy.cc"],
+    hdrs = ["codec_impl_legacy.h"],
+    external_deps = ["http_parser"],
+    deps = CODEC_LIB_DEPS,
 )
 
 envoy_cc_library(