Closing and draining tidy #3988

martinthomson · 2020-08-06T06:57:17Z

I took a careful look at the text here and found a few things:

There wasn't much redundancy, but there was a little. Mostly the problem was scattering of content between two locations in the document.
The closing and draining states only apply to immediate close.

So I moved things up. I trimmed the top-level immediate close text, and moved text specific to closing or draining to sections corresponding to each.

There were a few bits that could be cut, and a few that needed to be better aligned with existing text, but nothing major. Most of this is unmodified text.

There are a few bits that I had to tweak. I will add comments for those.

Closes #3907.

I took a careful look at the text here and found a few things: 1. There wasn't much redundancy, but there was a little. 2. The closing and draining states only apply to immediate close. So I moved things up. Shortened the top-level immediate close text, and moved text specific to closing or draining to the corresponding sections. There were a few bits that could be cut, and a few that needed to be better aligned with existing text, but nothing major. Most of this is unmodified text. There are a few bits that I had to rewrite. I will add comments for those. Closes #3907.

martinthomson · 2020-08-06T06:58:02Z

draft-ietf-quic-transport.md

 After sending a CONNECTION_CLOSE frame, an endpoint immediately enters the
-closing state.
+closing state; see {{closing}}. After receiving a CONNECTION_CLOSE frame,
+endpoints enter the draining state; see {{draining}}.


This is a new consolidated introduction. It's all the functional requirements that are relevant to this section.

martinthomson · 2020-08-06T07:00:46Z

draft-ietf-quic-transport.md

+An endpoint MAY drop packet protection keys when starting the closing period
+and send a packet containing a CONNECTION_CLOSE in response to any UDP datagram


This changes to use MAY instead of "is allowed to".

Otherwise, this changes only to remove the self-reference (and s/entering/starting).

martinthomson · 2020-08-06T07:01:32Z

draft-ietf-quic-transport.md

+While in the closing period, an endpoint could receive packets from a new
+source address, indicating a connection migration; see {{migration}}. New
+packets from unverified addresses could be used to create an amplification
+attack; see {{address-validation}}. To avoid being used for an amplication
+attack, an endpoint in the closing state MUST either discard packets received
+from unvalidated addresses or limit the cumulative size of packets it sends to
+unvalidated addresses to 3 times the size of packets it receives from the
+address.


This required a little better alignment with the other text above. The sentence "To avoid being used for an amplification attack" is now the same in both places.

martinthomson · 2020-08-06T07:02:12Z

draft-ietf-quic-transport.md

+The draining state is entered once an endpoint receives a CONNECTION_CLOSE
+frame, which indicates that its peer is closing or draining. While otherwise
+identical to the closing state, an endpoint in the draining state MUST NOT send
+any packets. Retaining packet protection keys is unnecessary once a connection
+is in the draining state.


We used to think that you could enter draining as a result of a stateless reset or idle timeout. That is no longer the case, so this just mentions CONNECTION_CLOSE directly.

martinthomson · 2020-08-06T07:03:22Z

draft-ietf-quic-transport.md

+same end time, but cease retransmission of the closing packet.
+
+
+### Terminal Connection State


This stuff is awkward. I will move it up into the introductory part instead.

And a few grammar fixes

Also stop using 'closing period', which is confusing.

martinthomson · 2020-08-06T07:20:40Z

draft-ietf-quic-transport.md

+In the closing state, an endpoint retains only enough information to generate a
+packet containing a CONNECTION_CLOSE frame and to identify packets as belonging
+to the connection. An endpoint in the closing state sends a packet containing a
+CONNECTION_CLOSE frame in response to any incoming packet that it attributes to
+the connection.
+
+An endpoint SHOULD limit the rate at which it generates packets in the closing
+state. For instance, an endpoint could wait for a progressively increasing
+number of received packets or amount of time before responding to received
+packets.
+
+The endpoint's selected connection ID and the QUIC version are sufficient
+information to identify packets for a closing connection; an endpoint MAY
+discard all other connection state. An endpoint that is closing is not required
+to process the frames contained in packets. An endpoint MAY retain packet
+protection keys for incoming packets to allow it to read and process a
+CONNECTION_CLOSE frame.


This is factored fairly heavily.

martinthomson · 2020-08-06T07:21:15Z

draft-ietf-quic-transport.md

+An endpoint MAY drop packet protection keys when entering the closing state
+and send a packet containing a CONNECTION_CLOSE in response to any UDP datagram
+that is received. However, an endpoint that discards packet protection keys
+cannot identify and discard invalid packets. To avoid being used for an
+amplication attack, such endpoints MUST limit the cumulative size of packets
+containing a CONNECTION_CLOSE frame to 3 times the cumulative size of the
+packets that cause those packets to be sent. To minimize the state that an
+endpoint maintains for a closing connection, endpoints MAY send the exact same
+packet.


This is only factored lightly:

use MAY in place of "is allowed to"

use state instead of period

remove self-reference

martinthomson · 2020-08-06T07:22:09Z

draft-ietf-quic-transport.md

+An immediate close can be used after an application protocol has arranged to
+close a connection.  This might be after the application protocols negotiates a
+graceful shutdown.  The application protocol exchanges whatever messages that
+are needed to cause both endpoints to agree to close the connection, after which
+the application requests that the connection be closed.  When the application
+closes the connection, a CONNECTION_CLOSE frame with an appropriate error code
+will be used to signal closure.
+
+The closing and draining connection states exist to ensure that connections
+close cleanly and that delayed or reordered packets are properly discarded.
+These states SHOULD persist for at least three times the current Probe Timeout
+(PTO) interval as defined in {{QUIC-RECOVERY}}.
+
+Disposing of connection state prior to the exiting the closing or draining state
+could cause delayed or reordered packets to generate an unnecessary stateless
+reset. Endpoints that have some alternative means to ensure that late-arriving
+packets on the connection do not induce a response, such as those that are able
+to close the UDP socket, MAY end these states earlier to allow
+for faster resource recovery.  Servers that retain an open socket for accepting
+new connections SHOULD NOT end the closing or draining states early.
+
+Once the closing or draining state ends, an endpoint SHOULD discard all
+connection state.  This results in new packets on the connection being handled
+generically.  For instance, an endpoint MAY send a stateless reset in response
+to any further incoming packets.


This stuff is mostly just reordered. The main change is to talk about states rather than periods, for consistency reasons.

martinthomson · 2020-08-06T07:22:24Z

draft-ietf-quic-transport.md

+
+### Closing Connection State {#closing}
+
+An endpoint enters a closing state after initiating an immediate close.


Removed a self-reference here, that's all.

draft-ietf-quic-transport.md

Co-authored-by: ianswett <ianswett@users.noreply.github.com>

ianswett

This is a big change, but seems good. Some specific suggestions on improving the text.

draft-ietf-quic-transport.md

ianswett · 2020-08-11T00:49:35Z

draft-ietf-quic-transport.md

+
+Once the closing or draining state ends, an endpoint SHOULD discard all
+connection state.  This results in new packets on the connection being handled
+generically.  For instance, an endpoint MAY send a stateless reset in response


This sentence is not very helpful, so I'd remove it unless you have a better suggestion.

draft-ietf-quic-transport.md

Co-authored-by: ianswett <ianswett@users.noreply.github.com>

martinthomson

Thanks Ian, I took the suggestions. Not sure about the reordering thing.

draft-ietf-quic-transport.md

MikeBishop

I think this flows a lot better. Thanks for the time you put into this!

draft-ietf-quic-transport.md

Co-authored-by: Mike Bishop <mbishop@evequefou.be>

martinthomson · 2020-08-17T04:03:26Z

@janaiyengar, you can add this to your review load.

janaiyengar

LGTM, just a large number of small nits.

draft-ietf-quic-transport.md

Co-authored-by: Jana Iyengar <jri.ietf@gmail.com>

janaiyengar

Thanks, Martin!

martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Aug 6, 2020

Remove self-reference

0c9512f

martinthomson commented Aug 6, 2020

View reviewed changes

martinthomson added 3 commits August 6, 2020 17:05

Move general stuff into introductory piece

d44244c

And a few grammar fixes

Better factoring of closing state introduction

43176fb

Also stop using 'closing period', which is confusing.

Remove mention of stateless reset

cb1bca7

martinthomson commented Aug 6, 2020

View reviewed changes

nibanks reviewed Aug 6, 2020

View reviewed changes

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

s/the//

a31d8dc

Co-authored-by: ianswett <ianswett@users.noreply.github.com>

ianswett reviewed Aug 11, 2020

View reviewed changes

Ian's cleanup

6aef5e4

Co-authored-by: ianswett <ianswett@users.noreply.github.com>

martinthomson commented Aug 11, 2020

View reviewed changes

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

Maybe we don't need to specify reordering/delay at all

e980fc7

MikeBishop approved these changes Aug 11, 2020

View reviewed changes

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved

martinthomson and others added 2 commits August 12, 2020 10:10

Small tweaks

e127cca

Co-authored-by: Mike Bishop <mbishop@evequefou.be>

Rewrap

37a2d52

janaiyengar reviewed Aug 27, 2020

View reviewed changes

martinthomson and others added 2 commits August 27, 2020 15:56

Many little nits

6cc520d

Co-authored-by: Jana Iyengar <jri.ietf@gmail.com>

Wrap

f6c037d

janaiyengar approved these changes Aug 27, 2020

View reviewed changes

janaiyengar merged commit 7e359d2 into master Aug 27, 2020

janaiyengar deleted the closing-draining-refactor branch August 27, 2020 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closing and draining tidy #3988

Closing and draining tidy #3988

martinthomson commented Aug 6, 2020 •

edited

Loading

martinthomson Aug 6, 2020

martinthomson Aug 6, 2020

martinthomson Aug 6, 2020

martinthomson Aug 6, 2020

martinthomson Aug 6, 2020

martinthomson Aug 6, 2020

martinthomson Aug 6, 2020

martinthomson Aug 6, 2020

martinthomson Aug 6, 2020

ianswett left a comment

ianswett Aug 11, 2020

martinthomson left a comment

MikeBishop left a comment

martinthomson commented Aug 17, 2020

janaiyengar left a comment

janaiyengar left a comment

		An endpoint MAY drop packet protection keys when starting the closing period
		and send a packet containing a CONNECTION_CLOSE in response to any UDP datagram

		same end time, but cease retransmission of the closing packet.


		### Terminal Connection State


		### Closing Connection State {#closing}

		An endpoint enters a closing state after initiating an immediate close.

Closing and draining tidy #3988

Closing and draining tidy #3988

Conversation

martinthomson commented Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

MikeBishop left a comment

Choose a reason for hiding this comment

martinthomson commented Aug 17, 2020

janaiyengar left a comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment

martinthomson commented Aug 6, 2020 •

edited

Loading