Move to CDDL - HTTP3, QPACK

[lnicco]

No description provided.

[lnicco]

I have checked the CDDL syntax with this command

cat draft-ietf-quic-qlog-h3-events.md | awk 'BEGIN{flag=0} /~~~ cddl-definition/{flag=1; printf "\n"; next} /~~~/{flag=0; next} flag' | cddl - generate | jq .

most definitions are unused though at this time
command output

*** Unused rule HTTP3ParametersRestore
*** Unused rule HTTP3StreamType
*** Unused rule HTTP3StreamTypeSet
*** Unused rule HTTP3FrameCreated
*** Unused rule HTTP3FrameParsed
*** Unused rule HTTP3PushResolved
*** Unused rule QPACKStateUpdate
*** Unused rule QPACKStreamStateUpdate
*** Unused rule QPACKStreamState
*** Unused rule QPACKDynamicTableUpdate
*** Unused rule QPACKDynamicTableUpdateType
*** Unused rule QPACKDynamicTableEntry
*** Unused rule QPACKHeadersEncoded
*** Unused rule QPACKHeadersDecoded
*** Unused rule QPACKInstructionCreated
*** Unused rule QPACKInstructionParsed
*** Unused rule HTTP3Frame
*** Unused rule HTTP3DataFrame
*** Unused rule HTTP3HeadersFrame
*** Unused rule HTTPHeader
*** Unused rule HTTP3CancelPushFrame
*** Unused rule HTTP3SettingsFrame
*** Unused rule HTTP3Settings
*** Unused rule HTTP3PushPromiseFrame
*** Unused rule HTTP3GoawayFrame
*** Unused rule HTTP3MaxPushIdFrame
*** Unused rule HTTP3ReservedFrame
*** Unused rule HTTP3ApplicationError
*** Unused rule QPACKInstruction
*** Unused rule SetDynamicTableCapacityInstruction
*** Unused rule InsertWithNameReferenceInstruction
*** Unused rule InsertWithoutNameReferenceInstruction
*** Unused rule DuplicateInstruction
*** Unused rule SectionAcknowledgementInstruction
*** Unused rule StreamCancellationInstruction
*** Unused rule InsertCountIncrementInstruction
*** Unused rule QPACKHeaderBlockRepresentation
*** Unused rule IndexedHeaderField
*** Unused rule LiteralHeaderFieldWithName
*** Unused rule LiteralHeaderFieldWithoutName
*** Unused rule QPACKHeaderBlockPrefix
*** Unused rule QPACKTableType
{
  "bather": 1044930698034052400,
  "upaithric": 15980487929328420000,
  "uxorially": 14932785096873822000,
  "verminer": 3648320349101929000
}

[rmarx]

Hey @lnicco,

Thanks for the work and the cool one-liner to extract CDDL from the draft.

Some initial feedback:

1. I've found that the CDDL tool doesn't really properly validate the "unused rules" so you need to make sure they are referenced somewhere. For the main schema I've forced this with kind of an ugly hack by adding the rules manually to a debugging definition (see here). It's probably best to have that here as well (can even be in the draft for now imo).
2. using the ~~~ cddl-definition ends up with slightly different generated HTML than the main schema where I use the {: .language-cddl}. Your version generates class="lang-cddl-definition sourcecode" while mine does class="lang-cddl sourcecode". I don't really care which method we use, as long as it creates consistent output.
3. We're not totally consistent about how we use string "enums". E.g., you've added a separate QPACKDynamicTableUpdateType, but HTTP3PushResolved has an inline decision: "claimed" / "abandoned". I'd prefer having separate types for these things (have done this in the main schema as well).
4. You've prefixed the HTTP/3 type names with HTTP3 instead of HTTP. My idea was that tools can eventually generate the qlog event name from the type names (i.e., TransportPacketSent can be transformed to transport:packet_sent). In this case, the category name is http and not http3, so your events should be called HTTPMyEventName not HTTP3MyEventName. At least for now... maybe we should discuss renaming the category to http3 instead... (now I assumed it would be implied by the protocol_type field) cc @LPardue and see Make protocol_type more well defined #146.
5. I think now in CDDL you can indeed have a single definition for FrameParsed and FrameCreated and have both alias to that.
6. Please also run the make command (at least for the final PR commit). This didn't have glaring errors, but a few trailing whitespaces (fixed with make fix-lint).

[lnicco]

Thanks @rmarx! Very valuable feedback and advice.
I'll try to fix the generated HTML and use {: .language-cddl}.
For the time being I admit that I cut corners to get a quick continuous CDDL validation :)

As far as using the HTTP3 prefix instead of HTTP I thought that it was valuable to distinguish between HTTP/3 and HTTP/2 especially around Frames (e.g. HTTP/2 Frames have flags?) but I'll revert to using HTTP and we can discuss further for future versions.

I'll have a second iteration ready soon.

[lnicco]

@rmarx I addressed almost all the comments. I can use some guidance on how to address having shared types for Parameters and Frames.

I also created a PR for the full CDDL file in case you want to link it all together

quiclog/qlog#7

[rmarx]

Hey @lnicco,

Thanks for the updates and the very useful script, which I was also able to use for the QUIC events draft!

I have added a new commit that primarily adds some overall consistency with the other two drafts (e.g., in terms of the code block ids, nothing major). I also noticed you were using ( and ) to wrap your enums, which isn't necessary (it works without as well, so I removed the brackets).

4 main things though:

1. You stated that HTTPHeader should become HTTPMessage, but it seems to me that should be HTTPField instead? Reading this part of HTTP/3, it seems that the Message is -everything- (headers, content, trailers), while the HEADERS frame (and similarly QPACK) only deals with the headers/trailers Fields? I haven't kept up enough with the new HTTP semantics documents or the QPACK rewording, but Fields does seem to be the proper term if I look at those documents (e.g., https://datatracker.ietf.org/doc/html/draft-ietf-quic-qpack-21#section-4.5 and https://www.ietf.org/archive/id/draft-ietf-httpbis-semantics-19.html#section-5 and https://datatracker.ietf.org/doc/html/draft-ietf-quic-http-34#section-7.2.2). As such, I've renamed HTTPHeader to HTTPField.

2. That makes it even clearer however that the QPACK event terminology is woefully out of date and we have to rename a lot of that to align with the newest QPACK draft text. I do propose we keep that for the draft after this however (-02, as the new one we're now preparing will be -01), as that would take us too much time and be too error prone to get done by Monday.

3. I found a relatively clean way to have re-usable HTTPParameters, though it does require a new piece of CDDL syntax called the "unwrap operator" ~, which is defined here. Essentially, that just takes the contents of the unwrapped type and puts them at the unwrap location, extending the target type. I do feel that will not be very clear for qlog readers, so we should explain that (better than what I tried atm...). Alternatively though, since they are just a few fields and only re-used at two locations, I wonder if it wouldn't make more sense to just keep it as it was (define them twice), to improve readability.

4. Finally, while I understood what you meant by reuse for the HTTPParameters (see point 3 above), I wasn't sure what you meant with "shared types for Frames"? Could you clarify? Maybe my approach for the HTTPParameters works for that as well and you can adapt it?

With that, I believe this draft is (99%) ready for submission on Monday, so please review and let me know if you find any issues @lnicco @marten-seemann @LPardue.

[lnicco]

@rmarx thanks for the fixes and comments.
You are right that Fields is the most updated terminology to refer to headers.
However, my comment about using HTTPMessage instead of HTTPHeaders is pointing to the fact that Headers alone are not representing a full message and that Method and Target for a request and status code for a response would be good to have. Also, Trailers should be considered. All of these are QPACK-encoded in HTTP/3 and HPACK-encoded in HTTP/2.

I was thinking that we could even use the Known-length binary HTTP Message representation?
https://www.ietf.org/archive/id/draft-thomson-http-binary-message-00.html#name-known-length-messages

[lnicco]

on a second thought the binary representation is not needed at all.
And for a request we can use the pseudo-headers to represent method/scheme/authority/path, and status pseudo-header for a response.

[rmarx]

@lnicco I'm not entirely sure I follow what you're saying wrt representation of full HTTPMessage here.

However, I do get the impression that that is something that goes (way) beyond simply updating to CDDL here, and so I'd propose you open a new issue describing this (with ideally a concrete example/proposal) so we don't hold up this PR on that. Would you agree?

[lnicco]

absolutely, not related to the CDDL conversion. It's a separate discussion.
Sorry for the noise