Update deps. Fix HTTP/2 support. Add master key file input. #63
Conversation
nirhaas
force-pushed
the
nirhaas/fix_http2_support
branch
from
98c0a2c to
0a9d499
Compare
|
This is awesome! All of these changes look great, not going to require any major changes. I'll make sure to review this soon 🙂 |
|
Sure :) Let me know the next tag you choose for this (v0.3.0 ?) so I can update the CHANGELOG. |
| Certificates: []tls.Certificate{tlsCert}, | ||
| KeyLogWriter: keyLogWriter, | ||
| } | ||
| // Support HTTP/2: https://golang.org/pkg/net/http/?m=all#Serve |
There was a problem hiding this comment.
As far as I understood, HTTP2 was already supported because tlsMux "unwraps" the TLS and the http server uses h2s to allow unencrypted HTTP2?
Was this not working for you?
There was a problem hiding this comment.
net/http package supports HTTP/2 out of the box if using TLS, but tlsMux uses tls.NewListener to wrap the raw listener, and then proxy uses http.Serve (and not http.ServerTLS). If I understand this correctly, itmeans that net/http is now unaware of the TLS part and therefore is not handling the HTTP/2 as expected.
Just for fun, I have experimented (different project) with http.ServeTLS with "a plain" listener, and it seems to handle HTTP/2 well.
Anyway, it was not working for me, and this small change (following the docs) fixed it.
| trailers metadata.MD | ||
| } | ||
|
|
||
| func (ss *recordedServerStream) SendHeader(headers metadata.MD) error { |
There was a problem hiding this comment.
Unrelated to this PR but I'd be interested to get your thoughts on #6 which would effectively be replacing this batching of events with logging events as they happen.
Benefits would be that the dump would much more accurately represent what happened during an RPC (especially streaming ones) but comes at a cost of being a bit harder to reassembly the output
The result would look a bit like https://github.com/bradleyjkemp/grpc-tools/blob/21195c2497c51fab93b0adcac105a6da83f1ccb3/integration_test/.snapshots/TestIntegration.json. Would this negatively impact usability for you?
There was a problem hiding this comment.
Great idea. This would not really change the way I am working with grpc-tools right now, but anyway would be a great improvement. Of course we would need to tie the response headers / trailers with the id as well.
|
@nirhaas I don't see any breaking changes here so I think this can be a v0.2.4? |
|
@bradleyjkemp According to SemVer, this is a MAJOR increment as this is an added functionality in a backwards compatible manner. But you are the lead here. Let me know if what you prefer (v0.2.4 or v0.3.0). |
|
Pre v1.0 I believe SemVer acts slightly differently so v0.X.Y becomes v0.X+1.Y for a breaking change and v0.X.Y+1 for added functionality. |
|
Updated CHANGELOG with v0.2.4. |
|
I'm not sure why CircleCI isn't running for this PR but I ran the tests locally and updated the snapshot file so this should pass the tests once merged. Thanks for adding this! Closes #43 |
Hi, great project!
I made the following changes for myself:
Let me know if any changes are necessary.