Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add raw bytes field support to CONNECTION_CLOSE. #412

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

LPardue
Copy link
Member

@LPardue LPardue commented Mar 16, 2024

Fixes #411

Same approach as the one we look for ALPNInformation. Allows endpoints to log either, because sometimes it is more useful to print and consume a string like "It's borked" than it's hex encoded equivalent.

draft-ietf-quic-qlog-quic-events.md Outdated Show resolved Hide resolved
@@ -2051,6 +2051,11 @@ error. For known frame types, the appropriate string value is used. For unknown
frame types, the numerical value without variable-length integer encoding is
used.

The CONNECTION_CLOSE reason is a byte sequences, that may be possible to
present as UTF-8. The `reason_text` and `reason_bytes` fields provide support
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not how I read RFC 9000. The field SHOULD be UTF-8, so the expectation is that it actually is. We're just creating an escape hatch here in case this assumption doesn't hold.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - need better wording alignment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're logging a reason sent by a peer you don't have any relationship with, there is no guarantee it is following the SHOULD. Therefore, it is only a possibility that the reason is presentable as UTF-8. I can't think of a more succint way to describe this possibility than the current text. If you have concrete suggestions for alternative text, please propose them :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke offline with Marten and came up with a slight variant

@rmarx
Copy link
Contributor

rmarx commented Mar 16, 2024

I'm not the biggest fan of this not using the same pattern as we did for ALPNInformation... which would be more like this:

...
? reason: CloseReason
...

CloseReason = {
    ? byte_value: hexstring
    ? string_value: text
}

Is there a particular reason to change from that established precedent?

@LPardue
Copy link
Member Author

LPardue commented Mar 16, 2024

The main reason (pun intended) is that the QUICALPNInformation event contains 3 instances of ALPNInformation, which would make a flatter structure really hard to describe. Here, there is only one instance of the reason, so it seems of less value to create another structure type to hold it.

I won't defend that choice with much vigor if others prefer a Structured approach.

@rmarx
Copy link
Contributor

rmarx commented Mar 18, 2024

I don't have a super-strong opinion on this, as this approach also makes sense. Especially seeing another need for this in #414, we could reason this approach is the default, and ALPNInformation deviates from that for good reasons™

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging non-UTF-8 reasons
4 participants