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 zero-length CIDs and active_connection_id_limit TP #3426

Merged
merged 7 commits into from
May 17, 2020

Conversation

nharper
Copy link
Contributor

@nharper nharper commented Feb 5, 2020

Fixes #3427.

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 think that I have larger concerns than this addresses. The special case for the server only highlighted that.

parameter MUST NOT be sent.
transport parameter is absent, a default of 2 is assumed. When the client
is using a zero-length connection ID, the server MUST NOT send the
active_connection_id_limit parameter.
Copy link
Member

Choose a reason for hiding this comment

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

I would say here that the server MAY omit the parameter. Sending a fixed set of transport parameters is a good practice.

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 was trying to keep this an editorial PR - saying that the server MAY omit the parameter changes normative language, which I thought would make it design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MUST NOT is gone, leaving an implicit MAY.

@@ -4697,11 +4697,11 @@ active_connection_id_limit (0x000e):
to store. This value includes the connection ID received during the handshake,
that received in the preferred_address transport parameter, and those received
in NEW_CONNECTION_ID frames.
Unless a zero-length connection ID is being used, the value of the
Unless the peer is using a zero-length connection ID, the value of the
Copy link
Member

Choose a reason for hiding this comment

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

I would instead prefer to say that the value is assumed to be zero if the peer requests that a zero length CID be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than "assumed to be zero," just say that value of the TP is not used in that case.

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've changed this to say that the TP is ignored if an endpoint is using a zero-len CID.

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.

This is good. Thanks for putting this together Nick.

than 2. If this transport parameter is absent, a default of 2 is assumed.
If an endpoint uses a zero-length connection ID, the
active_connection_id_limit value received from its peer is ignored and
not used.
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
not used.
not used, as NEW_CONNECTION_ID frames can't be sent by a peer that opts for
a zero-length connection ID.

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 slightly tweaked the suggested change: s/peer/endpoint/ and s/can't/cannot/.

This is @martinthomson's suggestion from the PR with a slight tweak.
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
transport parameter is absent, a default of 2 is assumed.
When a zero-length connection ID is being used, the active_connection_id_limit
parameter MUST NOT be sent.
The value of the active_connection_id_limit parameter MUST NOT be less
Copy link
Member

Choose a reason for hiding this comment

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

How about "MUST be at least 2"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@martinthomson martinthomson added -transport editorial An issue that does not affect the design of the protocol; does not require consensus. 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 11, 2020
@martinthomson martinthomson linked an issue Feb 11, 2020 that may be closed by this pull request
Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

LGTM, modulo one suggestion.

draft-ietf-quic-transport.md Show resolved Hide resolved
Co-Authored-By: Jana Iyengar <jri.ietf@gmail.com>
@MikeBishop
Copy link
Contributor

Purely editorial nit: This text has a lot of line breaks that won't render as such in the output. If you want to have separate paragraphs in the definition, you need a new : for each new paragraph.

An endpoint that receives a value less than 2 MUST close the connection
with an error of type TRANSPORT_PARAMETER_ERROR.
If this transport parameter is absent, a default of 2 is assumed. If an
endpoint uses a zero-length connection ID, the active_connection_id_limit
Copy link
Collaborator

Choose a reason for hiding this comment

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

"uses" here seems confusing

If an endpoint issues a zero-length connection-id, the active_connection_id_limit value it says is ignored, as...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the situation I'm trying to explain:

Suppose the client (C) uses 0-length CIDs. Since C uses 0-length CIDs, it will never send a NEW_CONNECTION_ID frame. The server (S) can still send an active_connection_id_limit transport param, but C will ignore it because it is irrelevant.

If an endpoint issues a zero-length connection-id, the active_connection_id_limit value it says is ignored, as...

My interpretation of this is that it says if a client (for example) C uses the 0-length CID, the active_connection_id_limit sent by C should be ignored. I believe this is incorrect, as C's active_connection_id_limit is regarding how many NEW_CONNECTION_IDs S can send to C.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My problem here is the text "uses", which is non-specific. A issues a CID and B sends packets with it. So they both use it.

You're right that my text above is backwards, which seems like an argument in favor of "this is confusing text". How about:

"If an endpoint issues a zero-length connection ID, it will never send NEW_CONNECTION_ID and therefore ignores any active_connection_id limits values it receives"

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've changed "uses" to "issues" and tightened up the language.

Copy link
Collaborator

@ekr ekr left a comment

Choose a reason for hiding this comment

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

LGTM

@martinthomson martinthomson merged commit 66cc63c into quicwg:master May 17, 2020
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
8 participants