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 FrameBuilder for building frames with target packet length #39

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sippejw
Copy link
Member

@sippejw sippejw commented May 7, 2024

Adds FrameBuilder that can adjust PADDING frame sizes to achieve a target length. Completes issue #38

@sippejw sippejw added the enhancement New feature or request label May 7, 2024
@sippejw sippejw requested a review from gaukas May 7, 2024 17:15
@sippejw sippejw marked this pull request as draft May 7, 2024 17:29
@sippejw
Copy link
Member Author

sippejw commented May 7, 2024

Marked as draft because this doesn't do exactly what I want yet. We want to be able to specify the length of the entire packet (payload and header) and then add padding accordingly. The FrameBuilder does not have access to the header length (I don't think the header has even been built at this point) so I don't yet see a clean way to do this.

@gaukas
Copy link
Contributor

gaukas commented May 7, 2024

I see what you meant there. Do we have any captured samples showing the variable length of Initial Packet header impacting the length of the frame section?

Actually we DO have the complete header ready at the point we build frame sections (see (*uPacketPacker).appendInitialPacket and (*uPacketPacker).MarshalInitialPacketPayload)

func (p *uPacketPacker) appendInitialPacket(buffer *packetBuffer, header *wire.ExtendedHeader, pl payload, encLevel protocol.EncryptionLevel, sealer sealer, v protocol.Version) (*longHeaderPacket, error) {

func (p *uPacketPacker) MarshalInitialPacketPayload(pl payload, v protocol.Version) ([]byte, error) {


I think originally quic-go does calculate the padding length at run-time. See

func (p *packetPacker) appendLongHeaderPacket(buffer *packetBuffer, header *wire.ExtendedHeader, pl payload, padding protocol.ByteCount, encLevel protocol.EncryptionLevel, sealer sealer, v protocol.Version) (*longHeaderPacket, error) {
from which I derived (*uPacketPacker).appendInitialPacket.

Copy link
Contributor

@gaukas gaukas left a comment

Choose a reason for hiding this comment

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

Looking good.

Can we move the definition and implementation of QUICPacket to after the end of `func (qfs QUICFrames) BuildFromFrames(frames []byte) (payload []byte, err error)? Just to keep each type definition and implementation together in one place.

// and returns the byte representation of all frames as specified in
// the Frames slice. It then calculates the padding sizes needed to
// reach the specified Length and updates the PADDING frames accordingly.
func (qp QUICPacket) Build(cryptoData []byte) (payload []byte, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (qp QUICPacket) Build(cryptoData []byte) (payload []byte, err error) {
func (qp *QUICPacket) Build(cryptoData []byte) (payload []byte, err error) {

Let's make *QUICPacket instead of QUICPacket the one interfacing QUICFrameBuilder, just to align with the style of *QUICRandomFrames. (QUICFrames is an exception as it is a slice instead of a ground-truth struct)

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

Successfully merging this pull request may close these issues.

2 participants