search

twitter / finagle

A fault tolerant, protocol-agnostic RPC system
https://twitter.github.io/finagle
Apache License 2.0
8.79k stars 1.46k forks source link

Parametrised HTTP/2 NACK to RST_STREAM frame handling #906

Closed ssiarkie closed 3 years ago

ssiarkie commented 3 years ago

Thank you for your amazing open source project.

Problem

The Finagle server using HTTP/2 Protocol converts NACK response to RST_STREAM frame, which can be properly interpreted by Finagle client. However we are using ALB (AWS Application Load Balancer), which is responsible for SSL/TLS termination. Unfortunately once ALB receives RST_STREAM frame from Finagle Server (Target Group), the ALB returns to a Finagle client response: 502 Bad Gateway. Consequently we lose information whether the response is a retryable or non-retryable NACK. Moreover Finagle client treats such response as application error and does not perform any action in order to reply such request. There was discussion with AWS Support and created request to gracefully handle such scenario by notifying a client about RST_STREAM, but we do not expect any change, because according to AWS current implementation is correct.

Solution

The Finagle server using HTTP/1.1 Protocol indicates NACKs by sending response with empty body and with specific header (finagle-http-nack, Retry-After: 0, finagle-http-retryable-request or finagle-http-nonretryable-nack). The Finagle server using HTTP/2 Protocol indicates NACKs using the same filters on server, however the main difference is in transport layer. Leveraging on HTTP/2 Protocol Finagle terminates the stream by setting RST_STREAM frame with error code. Nonretryable nacks are represented as ENHANCE_YOUR_CALM (0xB) and retryable NACKs are represented as REFUSED_STREAM (0x7) [Http2NackHandler]. The Finagle client in order to handle both NACKs in the same way converts RST_STREAM frame back to HTTP/1.1 response with NACK Headers (https://github.com/twitter/finagle/blob/develop/finagle-http2/src/main/scala/com/twitter/finagle/http2/transport/common/Http2StreamMessageHandler.scala#L122).

Knowing that we can remove Http2NackHandler from channel pipeline and Finagle clients will understand HTTP/2 NACKS based on HTTP headers, however server will be no longer sending RST_STREAM, which won't cause issues with any intermediate applications such as load balancers.

Result

The default behaviour won't be changed, however we will expose parameter to disable Http2NackHandler

CLAassistant commented 3 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

mosesn commented 3 years ago

This is a great PR, thanks!

ssiarkie commented 3 years ago

Thank you for such positive feedback. Can I expect information in this thread once the PR will be pushed back to GitHub? Looking forward for that and next opportunity to contribute ;)

jyanJing commented 3 years ago

Thank you for such positive feedback. Can I expect information in this thread once the PR will be pushed back to GitHub? Looking forward for that and next opportunity to contribute ;)

Thank you for the amazing PR! I have pulled the pr and getting internal reviews, I will update this thread once I merge your change 😃

jyanJing commented 3 years ago

The change has merged, sorry to keep you waiting, I have missed pulling this pr earlier last week, thank you again @ssiarkie for your amazing work!