[GitHub] [trafficserver] bneradt commented on issue #9115: Improper HTTP/2 enforcement of a reduced session window size

[GitBox <gi...@apache.org>]

bneradt commented on issue #9115:
URL: https://github.com/apache/trafficserver/issues/9115#issuecomment-1263843587

   Thank you @maskit for taking the time to double checking my observations. This is helpful.  I did not realize that the origin-side transactions were on separate connections.
   
   So this issue is entirely different than my previous interpretation of things. Replaying the test traffic again and reading through the logs, it turns out that the problem isn't on the origin side at all. Rather the problem has to do with how ATS handles the HTTP/2 session window calculation on the client side. The test sets the `proxy.config.http2.initial_window_size_in` value to 10 bytes. The ATS implementation expects this to apply to both stream and session windows. ATS correctly adjusts the stream window size for the client via a `SETTINGS` frame. The HTTP/2 client (implemented via nghttp2 in Proxy Verifier) correctly respects this new stream setting in all of its `DATA` frames and correctly awaits for `WINDOW_UPDATE` frames before sending more data. But the HTTP/2  protocol has no way to explicitly reduce the session window size like it does the stream window size. A receiver is simply expected to let the session window decline over time without incrementing it via WINDOW_UP
 DATE frames for the session. ATS, on the other hand, incorrectly enforces the reduced session window size (which the client can know nothing about) and replies with a `GOAWAY` frame to the client when it exceeds a 10 byte session window. That happens here:
   
   https://github.com/apache/trafficserver/blob/48d358ff731a002b7aee6aeadb22b67497cdfe62/proxy/http2/Http2ConnectionState.cc#L158-L162
   
   Locally, I modified that log message to emit the window sizes. ATS is emitting this error:
   
   ```
   [Sep 30 16:54:35.983] [ET_NET 0] ERROR: HTTP/2 connection error code=0x03 client_ip=127.0.0.1 session_id=0 stream_id=5 recv data cstate.server_rwnd (0) < payload_length (10)
   ```
   
   Note that ATS's internal accounting of the session window has incorrectly dropped to 10 bytes. The test only sends 4,800 bytes across all streams in the test session, so if ATS properly started the window at 65,535 bytes, this would be impossible. Looking at the code, we indiscriminately reduce the session window to the configured value here:
   
   https://github.com/apache/trafficserver/blob/48d358ff731a002b7aee6aeadb22b67497cdfe62/proxy/http2/Http2ConnectionState.cc#L1043
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org