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

Clarify section on negotiating connection IDs #3349

Merged
merged 5 commits into from Mar 10, 2020
Merged

Conversation

janaiyengar
Copy link
Contributor

@janaiyengar janaiyengar commented Jan 15, 2020

In thinking about #3348, I started this editorial PR to clarify language in the section on negotiating connection IDs. I suspect much of it was left over from before we had the DCID / SCID split, and also before the new handshake was established.

Addresses #3348.

The connection ID can change over the lifetime of a connection, especially in
response to connection migration ({{migration}}); see {{issue-cid}} for details.
the SCID supplied by the server as the DCID for subsequent packets, including
all subsequent Handshake and 0-RTT packets. This means that a client might
Copy link
Member

Choose a reason for hiding this comment

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

Why require that all subsequent Handshake packets use the same CID? It's definitely requires for Initial and 0-RTT to have consistent routing through the LB, but I haven't heard any reason to require it for handshake (taking into account possible new CIDs being received, i.e. #3348). IMO, adding extra restrictions here just complicates NEW_CONNECTION_ID handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gah! That was my mistake. Fixed.

@janaiyengar janaiyengar added -transport editorial An issue that does not affect the design of the protocol; does not require consensus. labels Jan 15, 2020
Comment on lines 1441 to 1442
the SCID supplied by the server as the DCID for subsequent packets, including
all subsequent 0-RTT packets. This means that a client might change the DCID
Copy link
Member

Choose a reason for hiding this comment

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

I think it might just be best to explicitly state which packet types require use of the new DCID. Not mentioning handshake packets here just leaves it unclear IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, and as I noted in the description, this PR exposes this issue.

Ok, I'm going to give it a shot based on what I understand our intent was; I'll see how it goes.

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

I have an editorial disagreement with this separate from the more substantive changes: I don't like acronyms for field names. Can we separate that out (or not do it, ideally)?

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. and removed editorial An issue that does not affect the design of the protocol; does not require consensus. labels Feb 4, 2020
Initial packet from the server, it MUST discard any subsequent packet it
receives with a different Source Connection ID.

A client MUST change the DCID value it sends in response to only the first
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A client MUST change the DCID value it sends in response to only the first
A client MUST change the Destination Connection ID value it uses for sending packets in response to only the first

receives with a different Source Connection ID.

A client MUST change the DCID value it sends in response to only the first
received Initial or Retry packet. A server MUST set its DCID value based on the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
received Initial or Retry packet. A server MUST set its DCID value based on the
received Initial or Retry packet. A server MUST set the Destination Connection ID it uses for sending packets based on the

A client MUST change the DCID value it sends in response to only the first
received Initial or Retry packet. A server MUST set its DCID value based on the
first received Initial packet. Any further changes to the DCID are not
permitted; if subsequent Initial packets include a different SCID, they MUST be
Copy link
Member

Choose a reason for hiding this comment

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

are only permitted if the values are taken from NEW_CONNECTION_ID frames; if subsequent

response to connection migration ({{migration}}); see {{issue-cid}} for details.
the SCID supplied by the server as the DCID for subsequent packets, including
all subsequent 0-RTT packets. This means that a client might change the DCID
twice during connection establishment, once in response to a Retry and once in
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the resolution of #3348, we should likely update this to not set an expectation that you change twice maximum, technically they can change as many times as you give them CIDs / as many times as they want to switch between all active CIDs.

The reference to {{issue-cid}} later will point people at some clarification about this too, so that looks great.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps: "This means that a client might have to change the connection ID it puts in the Destination Connection ID field twice during connection establishment, [..]"

These changes are mandatory, so it is appropriate to point to those as being special.

@martinthomson
Copy link
Member

martinthomson commented Mar 5, 2020

I would like to merge this PR as the corresponding issue has consensus, but it has outstanding issues. @janaiyengar would you mind?

One thing to add is that #3499 touches some of this text. I'll need to rebase that I guess.

@janaiyengar
Copy link
Contributor Author

I've taken in the comments, and revised this PR based on the resolution to #3348.

Until a packet is received from the server, the client MUST use the same DCID
value on all packets in this connection. This DCID is used to determine packet
Initial or Retry packet from the server, the client populates the Destination
Connection ID field with an unpredictable value. This Destination Connection ID
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth clarifying that the 'unpredictable value' is constant for a given connection?

Copy link
Member

Choose a reason for hiding this comment

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

That is below.

Until a packet is received from the server, the client MUST use the same DCID
value on all packets in this connection. This DCID is used to determine packet
Initial or Retry packet from the server, the client populates the Destination
Connection ID field with an unpredictable value. This Destination Connection ID
Copy link
Member

Choose a reason for hiding this comment

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

That is below.

in response to only the first received Initial or Retry packet. A server MUST
set the Destination Connection ID it uses for sending packets based on the first
received Initial packet. Any further changes to the Destination Connection ID
are are only permitted if the values are taken from any received
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
are are only permitted if the values are taken from any received
are only permitted if the values are taken from any received

you know it is hard for me to type that without typing "area"
@martinthomson martinthomson merged commit d3adc44 into master Mar 10, 2020
@martinthomson martinthomson deleted the jri/cids branch March 10, 2020 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants