runtime: removing http2_decode_metadata_with_quiche

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

changelogs/current.yaml

@@ -185,6 +185,9 @@ removed_config_or_runtime:
 - area: router
   change: |
     Removed ``envoy.reloadable_features.copy_response_code_to_downstream_stream_info`` runtime flag and legacy code paths.
+- area: http2
+  change: |
+    Removed ``envoy.reloadable_features.http2_decode_metadata_with_quiche`` runtime flag and legacy code paths.
 - area: jwt
   change: |
     Removed ``envoy.reloadable_features.token_passed_entirely`` runtime flag and legacy code paths.

source/common/http/http2/BUILD

@@ -97,7 +97,6 @@ envoy_cc_library(
     srcs = ["metadata_decoder.cc"],
     hdrs = ["metadata_decoder.h"],
     external_deps = [
-        "nghttp2",
         "quiche_http2_hpack_decoder",
     ],
     deps = [

source/common/http/http2/metadata_decoder.cc

@@ -44,11 +44,6 @@ struct MetadataDecoder::HpackDecoderContext {
 };
 
 MetadataDecoder::MetadataDecoder(MetadataCallback cb) {
-  nghttp2_hd_inflater* inflater;
-  int rv = nghttp2_hd_inflate_new(&inflater);
-  ASSERT(rv == 0);
-  inflater_ = Inflater(inflater);
-
   ASSERT(cb != nullptr);
   callback_ = std::move(cb);
 
@@ -66,13 +61,7 @@ bool MetadataDecoder::receiveMetadata(const uint8_t* data, size_t len) {
 }
 
 bool MetadataDecoder::onMetadataFrameComplete(bool end_metadata) {
-  bool success;
-  if (Runtime::runtimeFeatureEnabled(
-          "envoy.reloadable_features.http2_decode_metadata_with_quiche")) {
-    success = decodeMetadataPayload(end_metadata);
-  } else {
-    success = decodeMetadataPayloadUsingNghttp2(end_metadata);
-  }
+  const bool success = decodeMetadataPayload(end_metadata);
   if (!success) {
     return false;
   }
@@ -84,55 +73,6 @@ bool MetadataDecoder::onMetadataFrameComplete(bool end_metadata) {
   return true;
 }
 
-bool MetadataDecoder::decodeMetadataPayloadUsingNghttp2(bool end_metadata) {
-  Buffer::RawSliceVector slices = payload_.getRawSlices();
-  const int num_slices = slices.size();
-
-  // Data consumed by nghttp2 so far.
-  ssize_t payload_size_consumed = 0;
-  // Decodes header block using nghttp2.
-  for (int i = 0; i < num_slices; i++) {
-    nghttp2_nv nv;
-    int inflate_flags = 0;
-    auto slice = slices[i];
-    // is_end indicates if the data in slice is the last data in the current
-    // header block.
-    bool is_end = i == (num_slices - 1) && end_metadata;
-
-    // Feeds data to nghttp2 to decode.
-    while (slice.len_ > 0) {
-      ssize_t result =
-          nghttp2_hd_inflate_hd2(inflater_.get(), &nv, &inflate_flags,
-                                 reinterpret_cast<uint8_t*>(slice.mem_), slice.len_, is_end);
-      if (result < 0 || (result == 0 && slice.len_ > 0)) {
-        // If decoding fails, or there is data left in slice, but no data can be consumed by
-        // nghttp2, return false.
-        ENVOY_LOG(error, "Failed to decode payload.");
-        return false;
-      }
-
-      slice.mem_ = reinterpret_cast<void*>(reinterpret_cast<uint8_t*>(slice.mem_) + result);
-      slice.len_ -= result;
-      payload_size_consumed += result;
-
-      if (inflate_flags & NGHTTP2_HD_INFLATE_EMIT) {
-        // One header key value pair has been successfully decoded.
-        metadata_map_->emplace(std::string(reinterpret_cast<char*>(nv.name), nv.namelen),
-                               std::string(reinterpret_cast<char*>(nv.value), nv.valuelen));
-      }
-    }
-
-    if (slice.len_ == 0 && is_end) {
-      // After one header block is decoded, reset inflater.
-      ASSERT(end_metadata);
-      nghttp2_hd_inflate_end_headers(inflater_.get());
-    }
-  }
-
-  payload_.drain(payload_size_consumed);
-  return true;
-}
-
 bool MetadataDecoder::decodeMetadataPayload(bool end_metadata) {
   Buffer::RawSliceVector slices = payload_.getRawSlices();
 

source/common/http/http2/metadata_decoder.h

@@ -8,8 +8,6 @@
 #include "source/common/buffer/buffer_impl.h"
 #include "source/common/common/logger.h"
 
-#include "nghttp2/nghttp2.h"
-
 namespace Envoy {
 namespace Http {
 namespace Http2 {
@@ -58,13 +56,6 @@ class MetadataDecoder : Logger::Loggable<Logger::Id::http2> {
 
   struct HpackDecoderContext;
 
-  /**
-   * Decodes METADATA payload using nghttp2.
-   * @param end_metadata indicates is END_METADATA is true.
-   * @return if decoding succeeds.
-   */
-  bool decodeMetadataPayloadUsingNghttp2(bool end_metadata);
-
   /**
    * Decodes METADATA payload using QUICHE.
    * @param end_metadata indicates is END_METADATA is true.
@@ -88,11 +79,6 @@ class MetadataDecoder : Logger::Loggable<Logger::Id::http2> {
 
   uint64_t total_payload_size_ = 0;
 
-  // TODO(soya3129): consider sharing the inflater with all streams in a connection. Caveat:
-  // inflater failure on one stream can impact other streams.
-  using Inflater = CSmartPtr<nghttp2_hd_inflater, nghttp2_hd_inflate_del>;
-  Inflater inflater_;
-
   std::unique_ptr<HpackDecoderContext> decoder_context_;
 };
 

source/common/runtime/runtime_features.cc

@@ -57,7 +57,6 @@ RUNTIME_GUARD(envoy_reloadable_features_http1_balsa_delay_reset);
 RUNTIME_GUARD(envoy_reloadable_features_http1_connection_close_header_in_redirect);
 // Ignore the automated "remove this flag" issue: we should keep this for 1 year.
 RUNTIME_GUARD(envoy_reloadable_features_http1_use_balsa_parser);
-RUNTIME_GUARD(envoy_reloadable_features_http2_decode_metadata_with_quiche);
 RUNTIME_GUARD(envoy_reloadable_features_http2_discard_host_header);
 // Ignore the automated "remove this flag" issue: we should keep this for 1 year.
 RUNTIME_GUARD(envoy_reloadable_features_http2_use_oghttp2);

test/common/http/http2/metadata_encoder_test.cc

@@ -77,13 +77,8 @@ class MetadataUnpackingVisitor : public http2::adapter::test::MockHttp2Visitor {
 
 } // namespace
 
-class MetadataEncoderTest : public testing::TestWithParam<bool> {
+class MetadataEncoderTest : public testing::Test {
 public:
-  void SetUp() override {
-    scoped_runtime_.mergeValues({{"envoy.reloadable_features.http2_decode_metadata_with_quiche",
-                                  GetParam() ? "true" : "false"}});
-  }
-
   void initialize(MetadataCallback cb) {
     decoder_ = std::make_unique<MetadataDecoder>(cb);
 
@@ -129,12 +124,9 @@ class MetadataEncoderTest : public testing::TestWithParam<bool> {
   TestBuffer output_buffer_;
 
   Random::RandomGeneratorImpl random_generator_;
-  TestScopedRuntime scoped_runtime_;
 };
 
-INSTANTIATE_TEST_SUITE_P(BothBoolValues, MetadataEncoderTest, ::testing::Bool());
-
-TEST_P(MetadataEncoderTest, TestTotalPayloadSize) {
+TEST_F(MetadataEncoderTest, TestTotalPayloadSize) {
   initialize([](MetadataMapPtr&&) {});
 
   const std::string payload = std::string(1024, 'a');
@@ -147,7 +139,7 @@ TEST_P(MetadataEncoderTest, TestTotalPayloadSize) {
   EXPECT_EQ(2 * payload.size(), decoder_->totalPayloadSize());
 }
 
-TEST_P(MetadataEncoderTest, TestDecodeBadData) {
+TEST_F(MetadataEncoderTest, TestDecodeBadData) {
   MetadataMap metadata_map = {
       {"header_key1", "header_value1"},
   };
@@ -168,7 +160,7 @@ TEST_P(MetadataEncoderTest, TestDecodeBadData) {
 }
 
 // Checks if accumulated metadata size reaches size limit, returns failure.
-TEST_P(MetadataEncoderTest, VerifyEncoderDecoderMultipleMetadataReachSizeLimit) {
+TEST_F(MetadataEncoderTest, VerifyEncoderDecoderMultipleMetadataReachSizeLimit) {
   MetadataMap metadata_map_empty = {};
   MetadataCallback cb = [](std::unique_ptr<MetadataMap>) -> void {};
   initialize(cb);
@@ -206,7 +198,7 @@ TEST_P(MetadataEncoderTest, VerifyEncoderDecoderMultipleMetadataReachSizeLimit)
 }
 
 // Tests encoding an empty map.
-TEST_P(MetadataEncoderTest, EncodeMetadataMapEmpty) {
+TEST_F(MetadataEncoderTest, EncodeMetadataMapEmpty) {
   MetadataMap empty = {};
   MetadataMapPtr metadata_map_ptr = std::make_unique<MetadataMap>(empty);
 
@@ -224,7 +216,7 @@ TEST_P(MetadataEncoderTest, EncodeMetadataMapEmpty) {
 }
 
 // Tests encoding/decoding small metadata map vectors.
-TEST_P(MetadataEncoderTest, EncodeMetadataMapVectorSmall) {
+TEST_F(MetadataEncoderTest, EncodeMetadataMapVectorSmall) {
   MetadataMap metadata_map = {
       {"header_key1", std::string(5, 'a')},
       {"header_key2", std::string(5, 'b')},
@@ -260,7 +252,7 @@ TEST_P(MetadataEncoderTest, EncodeMetadataMapVectorSmall) {
 }
 
 // Tests encoding/decoding large metadata map vectors.
-TEST_P(MetadataEncoderTest, EncodeMetadataMapVectorLarge) {
+TEST_F(MetadataEncoderTest, EncodeMetadataMapVectorLarge) {
   MetadataMapVector metadata_map_vector;
   for (int i = 0; i < 10; i++) {
     MetadataMap metadata_map = {
@@ -283,7 +275,7 @@ TEST_P(MetadataEncoderTest, EncodeMetadataMapVectorLarge) {
 }
 
 // Tests encoding/decoding with fuzzed metadata size.
-TEST_P(MetadataEncoderTest, EncodeFuzzedMetadata) {
+TEST_F(MetadataEncoderTest, EncodeFuzzedMetadata) {
   MetadataMapVector metadata_map_vector;
   for (int i = 0; i < 10; i++) {
     Random::RandomGeneratorImpl random;
@@ -307,7 +299,7 @@ TEST_P(MetadataEncoderTest, EncodeFuzzedMetadata) {
   session_->ProcessBytes(toStringView(output_buffer_.buf, output_buffer_.length));
 }
 
-TEST_P(MetadataEncoderTest, EncodeDecodeFrameTest) {
+TEST_F(MetadataEncoderTest, EncodeDecodeFrameTest) {
   MetadataMap metadataMap = {
       {"Connections", "15"},
       {"Timeout Seconds", "10"},