add support for CRYPTO frames and multiple packet number spaces

google / quic-trace

Library and tools for transcribing QUIC connections.

Apache License 2.0

113 stars 19 forks source link

add support for CRYPTO frames and multiple packet number spaces #21

Closed marten-seemann closed 4 years ago

marten-seemann commented 5 years ago

Fixes #10.

This PR adds support for CRYPTO frames. CRYPTO data is treated similar to STREAM data (it consumes the same space in the y-dimension). I added a new frame type to the protobuf, so we're now able to properly display them in the frame list as well.

Furthermore, this PR now adds support for the 3 packet number spaces that IETF QUIC introduced. Packets are saved in separate maps depending on the packet number space, so it's possible to reset packet number (e.g. have a packet number 0 in Initial, Handshake and 1-RTT space), and have that displayed correctly.

We might want to consider to change the color depending on the packet number space (or on the encryption level?) in the future. Maybe we could use different shades of blue and green.

@kazuho and @janaiyengar, would you mind running this PR with some quicly traces and see if everything works fine for you?

janaiyengar commented 5 years ago

Thanks for working on this, @marten-seemann ! I finally got a chance to try this out, and the json converter needs to be fixed as well for it to work.

marten-seemann commented 5 years ago

@janaiyengar Could you contribute a patch for that? quic-go is exporting the protobuf directly, so it will be easier for you to test.

janaiyengar commented 5 years ago

Yes, I am planning to. This PR can go ahead regardless, looks good to me!

On Thu, May 30, 2019 at 10:26 PM Marten Seemann notifications@github.com wrote:

@janaiyengar https://github.com/janaiyengar Could you contribute a patch for that? quic-go is exporting the protobuf directly, so it will be easier for you to test.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/quic-trace/pull/21?email_source=notifications&email_token=ACUOBVBMDGILDLDVKB7D2NDPYCZJ3A5CNFSM4HJCGY5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWUHTRY#issuecomment-497580487, or mute the thread https://github.com/notifications/unsubscribe-auth/ACUOBVAGZCXWOBKEDCBXC4LPYCZJ3ANCNFSM4HJCGY5A .

marten-seemann commented 5 years ago

@vasilvv Is there anything I can do to get this PR merged soon?

vasilvv commented 5 years ago

Sorry, I forgot about this. I need to figure out why this segfaults with traces that gQUIC outputs.

marten-seemann commented 5 years ago

My guess is that this happens because gQUIC allows acknowledging packets across encryption levels, whereas in IETF QUIC ACKs are always sent in the same packet number space.

vasilvv commented 4 years ago

Hi Marten,

Thank you for sending this, and sorry it took me so long to get to this!

There are a bunch of switch statements here that do not handle ENCRYPTION_UNKNOWN. Could you add a default handler to those that just has a LOG(FATAL) in them? I had some trouble identifying why the renderer was crashing.

Do you have any new example traces for this PR? I tried loading the existing examples/example.qtr, but it crashes the renderer.

marten-seemann commented 4 years ago

@vasilvv I added default cases for the switch statements I introduced. I also updated the example.qtr. I don't have a json-encoded file though, since quic-go output the protobuf directly.

vasilvv commented 4 years ago

Thanks a lot for doing this! I'll try to get gquic to support this as well, since it actually has multiple packet number spaces now.