Allow max datagram frame size to be configurable

[chungthuang]

https://datatracker.ietf.org/doc/html/draft-ietf-quic-datagram#section-3 doesn't place a limit on the max datagram frame size. One of the use case for datagram frame is to tunnel UDP over a QUIC, but the current limit of 1220 bytes is too small. Can it be configured by Config?
I tried to open a PR, but I was getting 403 forbidden.

[marten-seemann]

It's not that easy. The MTU on the outer connection must be large enough to hold the datagrams (plus the additional packet header and crypto overhead).

[chungthuang]

Does a session have access to packet header and crypto overhead size? I was thinking about updating https://github.com/lucas-clemente/quic-go/blob/e6572e3d0a1e754e91c4d0648cbd598d7e5ca7bc/session.go#L1969 to check datagram frame size + packet header + crypto overhead against MTU. The session can get current MTU from a new method of mtuDiscoverer.

[marten-seemann]

The size of the header unfortunately is not constant, it depends on the value of the packet number and on the connection ID, for example. If you want a guarantee that things will fit, the only safe thing to do would be to calculate the maximum size of the header. Unfortunately, this size will be quite a bit bigger than the actual size in almost all cases.

[chungthuang]

Can you point me to how the header size is computed? Perhaps we can also signal the max payload size to the caller during dequeue https://github.com/lucas-clemente/quic-go/blob/b8f07c728cf901da96ad445306a3190a51b2b399/packet_packer.go#L594-L599
by returning an error via the dequeue channel in https://github.com/lucas-clemente/quic-go/blob/b8f07c728cf901da96ad445306a3190a51b2b399/datagram_queue.go#L45-L46

[LPardue]

An alternative is to just set a "very large" value for max_datagram_frame_size because the actual size of DATAGRAM frames will always be bound by the actual maximum size of QUIC packets on any given connection.

[marten-seemann]

@LPardue's suggestion definitely makes sense for the receive side.
The complication here is that the send side is not very sophisticated: https://github.com/lucas-clemente/quic-go/blob/496fbe9fda4bb094b1cbdb162a52130b6beda6b1/session.go#L1973-L1981

By using a datagram size that's 60 bytes smaller than the (minimum) packet size quic-go uses, we can make sure that the DATAGRAM frame is guaranteed to fit into the payload.
I haven't spent too much time thinking about the best API here, as I've not used datagrams in production yet.

[neilalexander]

It's not that easy. The MTU on the outer connection must be large enough to hold the datagrams (plus the additional packet header and crypto overhead).

We'd like to use quic-go over an overlay network where the effective MTU is always 65535 because the underlying transport guarantees that. We don't see any way to set a maximum packet size manually to take advantage of that and leaving PMTUD enabled causes us headaches (wastes a lot of bandwidth over otherwise slow transports). It might be an "edge case" but there are arguably cases where it makes sense to be able to tune these things.

[marten-seemann]

@neilalexander This issue is about the QUIC DATAGRAM frame, not the MTU size. I wouldn't be opposed to adding a config option to increase the QUIC packet size, but let's discuss this in a separate issue.

[neilalexander]

@marten-seemann You're right, I misread — I've created #3385 to discuss the possibility of larger packet sizes.

[nanokatze]

Have you considered a pull-style API, like io.ReaderFrom? quic-go could call Read, once for each datagram, on a user-provided io.Reader, and the length of destination buffer would convey the maximum datagram size.

[DevinCarr]

Following up on this for our use-case in cloudflared.

We have been using something similar to this change: DevinCarr@d1f4eda for a while to allow for the max_datagram_frame_size to be increased beyond the previously set value of 1220 (currently 1200). We use this configuration parameter to increase the value of max_datagram_frame_size to 1350.

The work done in this change (DevinCarr@d1f4eda) was adopted from an older change our team had been using on top of v0.28.1 but I recently forward-ported it on-top of some of the recent master changes you have been working on, specifically the transport changes have helped make this change very easy to bring forward.

Would you consider something like this change to help address users looking to increase the max_datagram_frame_size to a custom value? Or would you prefer to just set the max_datagram_frame_size size to the maximum value since it is bounded by the QUIC packet size anyways as previously mentioned by LPardue?

[joliveirinha]

@marten-seemann what do you think of the last comment?
Looking at the code and RFC, there doesn't seem to be a good reason to have such a low number in the MTU. maybe we can just increase it, instead of making it configurable.

[marten-seemann]

Sending a large value as @LPardue suggests seems reasonable.

For testing purposes, it would be nice if we had the option to send a smaller value though. I suggest introducing a global variable in the wire package:

// MaxDatagramSize is the maximum size of a DATAGRAM frame (RFC 9221).
// By setting it to a large value, we allow all datagrams that fit into a QUIC packet. 
// The value is chosen such that it can still be encoded as a 2 byte varint.
var MaxDatagramSize uint64 = 16383

In tests, we could then modify this value and check that the peer behaves correctly. Users of the library won't be able to change this value, since the wire package is an internal package.

Does anybody want to contribute a patch?

[chungthuang]

Sending a large value as @LPardue suggests seems reasonable.

For testing purposes, it would be nice if we had the option to send a smaller value though. I suggest introducing a global variable in the wire package:

// MaxDatagramSize is the maximum size of a DATAGRAM frame (RFC 9221).
// By setting it to a large value, we allow all datagrams that fit into a QUIC packet. 
// The value is chosen such that it can still be encoded as a 2 byte varint.
var MaxDatagramSize uint64 = 16383

In tests, we could then modify this value and check that the peer behaves correctly. Users of the library won't be able to change this value, since the wire package is an internal package.

Does anybody want to contribute a patch?

Hi @marten-seemann I followed your suggestions to create a patch #4143. Please let me know what you think.