trafficserver Http2 to origin

This implements the HTTP/2 to origin feature.

Jul 14 '22 20:07 bneradt

Since H2 to Origin has been unmarged for very long time, I'm slightly lowering a bar for merging this.

I still think issue https://github.com/apache/trafficserver/issues/5230 should be fixed eventually (not just for performance but also privacy), but I'm going to let you off the hook. I actually have an incomplete patch (which works for simple use case at minimum) to pass an HttpHdr for request to HttpSM without marshaling and unmarshaling. Maybe we can add sensitive flag later to keep HPACK header representation. For now, I'd like to see some warning or note on the doc that says header representation won't be kept and sensitive headers can be stored into HPACK dynamic table.

Jul 15 '22 02:07 maskit

I appreciate the thoughtful review, @maskit. And thank you, too, for repeating some of your comments from the previous PR. I'm working through your observations now and will spin off other PRs per your suggestions.

Jul 18 '22 17:07 bneradt

If we don't support Server Push on origin side for now, we need to indicate it by sending SETTINGS_ENABLE_PUSH:0.

SETTINGS_ENABLE_PUSH (0x02): This setting can be used to enable or disable server push. A server MUST NOT send a PUSH_PROMISE frame if it receives this parameter set to a value of 0; see Section 8.4. A client that has both set this parameter to 0 and had it acknowledged MUST treat the receipt of a PUSH_PROMISE frame as a connection error (Section 5.4.1) of type PROTOCOL_ERROR.

The initial value of SETTINGS_ENABLE_PUSH is 1. For a client, this value indicates that it is willing to receive PUSH_PROMISE frames.

Oct 07 '22 00:10 maskit

If we don't support Server Push on origin side for now, we need to indicate it by sending SETTINGS_ENABLE_PUSH:0.

Yes, this is a good observation. I'll work on this when #9085 is finished.

Oct 17 '22 19:10 bneradt

If we don't support Server Push on origin side for now, we need to indicate it by sending SETTINGS_ENABLE_PUSH:0.

Yes, this is a good observation. I'll work on this when #9085 is finished.

Updated to set SETTINGS_ENABLE_PUSH to 0. Good observation.

Nov 11 '22 18:11 bneradt

The http2_to_origin AuTest will fail until the new Proxy Verifier await directive is merged in: https://github.com/yahoo/proxy-verifier/pull/228

Nov 26 '22 23:11 bneradt

@bneradt Is it possible to separate out the change for ConnectionPool and ConnectingEntry?

Since this PR is for H2 to Origin, the code on this PR should not do anything bad unless H2 to Origin is enabled by having h2 in proxy.config.ssl.client.alpn_protocols. I understand nobody can guarantee that, but ConnectionPool and ConnectingEntry are new and those may affect H1 because the code is used for the both protocols.

As you can see, people who are unfamiliar with H2 haven't been reviewing this PR at all, but the change can bother them. I think you should gather feedback from them as well. Otherwise, we would be blamed like "H2 to Origin made this mess where we don't need it!".

Nov 28 '22 18:11 maskit

The http2_to_origin AuTest will fail until the new Proxy Verifier await directive is merged in: yahoo/proxy-verifier#228

https://github.com/apache/trafficserver/pull/9221

Nov 28 '22 22:11 bneradt

As you can see, people who are unfamiliar with H2 haven't been reviewing this PR at all, but the change can bother them. I think you should gather feedback from them as well. Otherwise, we would be blamed like "H2 to Origin made this mess where we don't need it!".

Just to follow up on this, after mentioning this suggestion in the PR/issue call this Monday, it was suggested that @keesspoelstra would be a good candidate to review this PR as well. He mentioned that he already has been following it and would spend some time looking at that patch.

Thank you again, @maskit , for the time you have spent reviewing this.

Nov 30 '22 22:11 bneradt

This is an approval for changes around H2.

I'm not sure about connection management part. I still think we should make the change separately, but if that's not an option, please wait for another approval from someone else.

Thank you @maskit . Again, I appreciate all the feedback you've provided on this feature.

@keesspoelstra is carefully reviewing and testing this in the meantime too. I'll wait upon at least his review.

Jan 26 '23 19:01 bneradt

Going to close this and re-open on master now that 10-Dev is master.

See: #9366

Feb 02 '23 17:02 bneradt