Add checks in referenceclient to validate raw gRPC-Web trailers and actual HTTP trailers #780
Conversation
jhump
force-pushed
the
jh/grpc-web-trailers
branch
2 times, most recently
from
0738a26 to
13c7660
Compare
jhump
changed the title
jhump
force-pushed
the
jh/grpc-web-trailers
branch
from
55286a5 to
9c0b785
Compare
| return errs | ||
| } | ||
|
|
||
| func checkGRPCWebTrailers(rawGRPCWebTrailers string, reportedTrailers []*conformancev1.Header) multiErrors { |
There was a problem hiding this comment.
Might be good to have unit tests for this. It seems sufficiently complex enough that we'd want to test it.
There was a problem hiding this comment.
You're right. I was being lazy programmer. I'll add tests.
jhump
force-pushed
the
jh/grpc-web-trailers
branch
from
9c0b785 to
a0c5006
Compare
jhump
changed the title
|
@smaye81, okay, I've got this rebased now that all of the wire detail checks are just in the reference client. BTW, I think the best will be easiest to review one commit at a time (particularly because I renamed a file in the first commit, and viewing the diff for the whole PR shows it as one file removed and another added). I updated this to use the same "printer" abstraction that the reference server uses for logging issues, but instead of getting written to stderr, these get recorded into a I've also added a test with numerous cases, to verify the logic. And, admittedly, that logic got more complicated so that (1) it can handle different kinds of line endings and complain about them, (2) it can correctly handle "obsolete line folding" described in the spec and complain about it, and (3) validate the field name (in previous iteration, I somehow missed that, and it was only checking the value). |
jhump
force-pushed
the
jh/grpc-web-trailers
branch
from
aefbd29 to
dfb7e40
Compare
jhump
force-pushed
the
jh/grpc-web-trailers
branch
from
dfb7e40 to
c307412
Compare
| // TODO | ||
| } | ||
|
|
||
| func examineGRPCEndStream(endStream string, printer internal.Printer) { |
There was a problem hiding this comment.
Holy cow, haha, this is great. This almost feels like it could be its own package to help others validate end streams.
…o some of the wire-details-related functions; fix racy behavior that can result in flaky failures with timeout test cases
|
@smaye81, I just pushed one more commit. Some stuff in that commit were a few small renames and additional doc comments in the wire details stuff, in the hopes of making things a little more clearly named and easier to read. But I also had to change the capture stuff from using an atomic pointer to using a channel to await the value being available. This is because, sadly, it can't be helped that sometimes there will be some raciness between the goroutine executing the HTTP operation and the RPC caller's goroutine for the timeout cases (only for client- and bidi-stream RPCs; for other kinds of RPCs, the HTTP operation is issued on the same goroutine as the RPC caller). See 6386c4c. |
This adds handling of the HTTP trailers and gRPC-Web end-stream messages to the reference client. When in reference mode, it will examine them and make sure they are appropriate, using stricter checks than connect-go (so we can't just rely on the base connect-go client implementation to raise these issues in the conformance results).