Try to fix envoyproxy#24735 and unit test

Signed-off-by: Zhewei Hu <[email protected]>
Winbobob · Aug 4, 2024 · f01b778 · f01b778
1 parent 54c36f5
commit f01b778
Show file tree

Hide file tree

Showing 5 changed files with 10 additions and 29 deletions.
diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -1074,19 +1074,13 @@ bool ConnectionManagerImpl::ActiveStream::validateTrailers() {
     failure_details = std::string(transformation_result.details());
   }
 
-  Code response_code = Code::BadRequest;
   absl::optional<Grpc::Status::GrpcStatus> grpc_status;
   if (Grpc::Common::hasGrpcContentType(*request_headers_)) {
     grpc_status = Grpc::Status::WellKnownGrpcStatus::Internal;
   }
 
-  // TODO(#24735): Harmonize H/2 and H/3 behavior with H/1
-  if (connection_manager_.codec_->protocol() < Protocol::Http2) {
-    sendLocalReply(response_code, "", nullptr, grpc_status, failure_details);
-  } else {
-    filter_manager_.streamInfo().setResponseCodeDetails(failure_details);
-    resetStream();
-  }
+  sendLocalReply(Code::BadRequest, "", nullptr, grpc_status, failure_details);
+
   if (!response_encoder_->streamErrorOnInvalidHttpMessage()) {
     connection_manager_.handleCodecError(failure_details);
   }

diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h
@@ -405,10 +405,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     // Note: this method is a noop unless ENVOY_ENABLE_UHV is defined
     // Call header validator extension to validate the request trailer map after it was
     // deserialized. If the trailer map failed validation, this method does the following:
-    // 1. For H/1 it sends 400 response and returns false.
-    // 2. For H/2 and H/3 it resets the stream (without error response). Issue #24735 is filed to
-    //    harmonize this behavior with H/1.
-    // 3. If the `stream_error_on_invalid_http_message` is set to `false` (it is by default) in the
+    // 1. For H/1, H2, and H3, it sends 400 response and returns false.
+    // 2. If the `stream_error_on_invalid_http_message` is set to `false` (it is by default) in the
     // HTTP connection manager configuration, then the entire connection is closed.
     bool validateTrailers();
 

diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc
@@ -1470,7 +1470,7 @@ TEST_P(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreAreDropped) {
 
 // Ensures that request with header names containing the underscore character are rejected
 // when the option is set to reject request.
-TEST_P(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreAreRejected) {
+TEST_P(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreCauseRequestRejected) {
   headers_with_underscores_action_ = envoy::config::core::v3::HttpProtocolOptions::REJECT_REQUEST;
   initialize();
 

diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc
@@ -2865,8 +2865,7 @@ TEST_P(Http2CodecImplTest, HeaderNameWithUnderscoreAreRejected) {
   TestRequestHeaderMapImpl request_headers;
   HttpTestUtility::addDefaultHeaders(request_headers);
   request_headers.addCopy("bad_header", "something");
-  EXPECT_CALL(request_decoder_, sendLocalReply(Http::Code::BadRequest, _, _, _, _));
-  EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0);
+  EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _));
   EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok());
   driveToCompletion();
   EXPECT_EQ(

diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc
@@ -1828,7 +1828,7 @@ TEST_P(DownstreamProtocolIntegrationTest, HeadersWithUnderscoresCauseRequestReje
     ASSERT_TRUE(response->reset());
     EXPECT_EQ((downstream_protocol_ == Http::CodecType::HTTP3
                    ? Http::StreamResetReason::ProtocolError
-                   : Http::StreamResetReason::ConnectionTermination),
+                   : Http::StreamResetReason::RemoteReset),
               response->resetReason());
   }
   EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("unexpected_underscore"));
@@ -1859,19 +1859,9 @@ TEST_P(DownstreamProtocolIntegrationTest, TrailerWithUnderscoresCauseRequestReje
       *request_encoder_,
       Http::TestRequestTrailerMapImpl{{"trailer1", "value1"}, {"trailer_2", "value2"}});
 
-  if (downstream_protocol_ == Http::CodecType::HTTP1) {
-    ASSERT_TRUE(codec_client_->waitForDisconnect());
-    ASSERT_TRUE(response->complete());
-    EXPECT_EQ("400", response->headers().getStatusValue());
-  } else {
-    ASSERT_TRUE(response->waitForReset());
-    codec_client_->close();
-    ASSERT_TRUE(response->reset());
-    EXPECT_EQ((downstream_protocol_ == Http::CodecType::HTTP3
-                   ? Http::StreamResetReason::ProtocolError
-                   : Http::StreamResetReason::ConnectionTermination),
-              response->resetReason());
-  }
+  ASSERT_TRUE(codec_client_->waitForDisconnect());
+  ASSERT_TRUE(response->complete());
+  EXPECT_EQ("400", response->headers().getStatusValue());
   EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("unexpected_underscore"));
 }