Skip to content

fix: handle request trailers in the test server#1548

Open
immanuwell wants to merge 1 commit into
envoyproxy:mainfrom
immanuwell:fix/test-server-request-trailers
Open

fix: handle request trailers in the test server#1548
immanuwell wants to merge 1 commit into
envoyproxy:mainfrom
immanuwell:fix/test-server-request-trailers

Conversation

@immanuwell

Copy link
Copy Markdown

Description

This PR is related to #1103

If a request ends with trailers, the test server can miss the local reply and just hang.
Right now it only replies from decodeHeaders(..., end_stream=true) or decodeData(..., end_stream=true).

This patch sends the reply from decodeTrailers() too, adds a regression test, and notes the behavior in docs.

Repro on main:

  1. Build and run bazel-bin/nighthawk_test_server
  2. Send printf 'POST / HTTP/1.1\r\nHost: 127.0.0.1:10000\r\nTransfer-Encoding: chunked\r\nTrailer: x-test-trailer\r\n\r\n1\r\na\r\n0\r\nx-test-trailer: yep\r\n\r\n' | nc 127.0.0.1 10000
  3. The request hangs, no reply comes back.

With this patch it returns 200 with the configured body. tiny fix, real gap.

Notes for Reviewers

No API changes.

Signed-off-by: immanuwell <pchpr.00@list.ru>
request_headers_dump_ = headers_dump.str();
}
sendReply(*effective_config_.value());
if (effective_config_.ok() && effective_config_.value()->echo_request_headers()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but I think there might be another bug that we might be able to address at the same time:

If there are trailers, that might mean decodeHeaders is always called with end_stream=false. If so, we would fail to dump the request headers to the response body.

To fix that, maybe we should just lift the if (effective_config.ok()... to before if (end_stream).

If you can't quickly determine if that would solve the problem correctly (possibly with an extra testcase), feel free to skip, and we can work on the case of header dumping with request trailers later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants