Skip to content

Comments

Add a record layer design document#17969

Closed
mattcaswell wants to merge 3 commits intoopenssl:masterfrom
mattcaswell:record-layer-design
Closed

Add a record layer design document#17969
mattcaswell wants to merge 3 commits intoopenssl:masterfrom
mattcaswell:record-layer-design

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Mar 25, 2022

No description provided.

@mattcaswell mattcaswell added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending hold: discussion The community needs to establish a consensus how to move forward with the issue or PR labels Mar 25, 2022
@mattcaswell
Copy link
Member Author

mattcaswell commented Mar 25, 2022

Added the "need OTC decision" label since this needs discussion at OTC.

@t8m t8m added the triaged: design The issue/pr deals with a design document label Mar 28, 2022
slontis
slontis reviewed Apr 8, 2022
View reviewed changes
doc/designs/quic-design/record-layer.md Outdated

A record within this document refers to a packet of data. It will typically
contain some form of header data and some payload data, and will often be
cryptographically protected. A record may or may have a one-to-one
Copy link
Member

@slontis slontis Apr 8, 2022

Choose a reason for hiding this comment

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

may not ?

romen
romen reviewed Apr 12, 2022
View reviewed changes
doc/designs/quic-design/record-layer.md Outdated
actions that can be performed by a record layer.

An `OSSL_RECORD_LAYER` object represents a specific instantiation of a
paritcular `OSSL_RECORD_METHOD`. It contains the state used by that
Copy link
Member

@romen romen Apr 12, 2022

Choose a reason for hiding this comment

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

particular

@mattcaswell mattcaswell mentioned this pull request Apr 19, 2022
Closed
@openssl-machine
Copy link
Collaborator

openssl-machine commented May 13, 2022

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

@t8m t8m marked this pull request as draft May 24, 2022 08:52
@t8m t8m removed approval: review pending This pull request needs review by a committer approval: otc review pending hold: discussion The community needs to establish a consensus how to move forward with the issue or PR labels May 24, 2022
@paulidale
Copy link
Contributor

paulidale commented Jun 20, 2022

Is this ready for review?

@levitte levitte mentioned this pull request Jul 19, 2022
Closed
@t8m t8m mentioned this pull request Jul 21, 2022
Closed
@levitte
Copy link
Member

levitte commented Sep 6, 2022

Is this still in progress? Considering that #18132, #18610 and #18795 have been merged, does that indicate that this has fulfilled its purpose? Should it be merged or just closed?

@mattcaswell
Copy link
Member Author

mattcaswell commented Sep 7, 2022

This is the basis I am using for the implementation. There are already numerous changes from what is described here implemented in #18132. My intention is to circle back and update this design document with the latest changes once things have settled down with the implementation of the write side.

@paulidale
Copy link
Contributor

paulidale commented Sep 8, 2022

I'd much prefer to have seen this merged and updated separately.
Anything that depends on the record layer is currently in limbo.

@mattcaswell mattcaswell marked this pull request as ready for review November 8, 2022 14:53
@mattcaswell mattcaswell added approval: review pending This pull request needs review by a committer approval: otc review pending labels Nov 8, 2022
@mattcaswell
Copy link
Member Author

mattcaswell commented Nov 8, 2022

This PR has now been updated to reflect the current state of the record layer as it has been implemented.

I have taken it out of draft and it is now ready for review.

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 10, 2022
@openssl-machine
Copy link
Collaborator

openssl-machine commented Nov 11, 2022

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

@hlandau
Copy link
Member

hlandau commented Nov 14, 2022

@mattcaswell Some MD nits.

@mattcaswell
Copy link
Member Author

mattcaswell commented Nov 14, 2022

Fixup pushed to address md-nits issues. @hlandau, @t8m please reconfirm.

hlandau
hlandau approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@hlandau hlandau left a comment

Choose a reason for hiding this comment

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

Still OK

@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Nov 16, 2022
@hlandau
Copy link
Member

hlandau commented Nov 18, 2022

CIs are not relevant.

Merged to master. Thank you.

@hlandau hlandau closed this Nov 18, 2022
openssl-machine pushed a commit that referenced this pull request Nov 18, 2022
@mattcaswell @hlandau
Add a record layer design document
14b5447 
Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Hugo Landau <[email protected]>
(Merged from #17969)
openssl-machine pushed a commit that referenced this pull request Nov 18, 2022
@mattcaswell @hlandau
Update the record layer design based on implementation experience
48cc4e0 
Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Hugo Landau <[email protected]>
(Merged from #17969)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@mattcaswell @beldmit
Add a record layer design document
8c6632b 
Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Hugo Landau <[email protected]>
(Merged from openssl#17969)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@mattcaswell @beldmit
Update the record layer design based on implementation experience
720d29e 
Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Hugo Landau <[email protected]>
(Merged from openssl#17969)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slontis slontis slontis left review comments

@t8m t8m t8m approved these changes

+2 more reviewers

@romen romen romen left review comments

@hlandau hlandau hlandau approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch triaged: design The issue/pr deals with a design document

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@mattcaswell @openssl-machine @paulidale @levitte @hlandau @romen @t8m @slontis