Re: [dtn-interest] IRSG review of draft-irtf-dtnrg-tcp-clayer-06.txt

Simon Perreault <simon.perreault@viagenie.ca> Wed, 17 July 2013 11:33 UTC

Return-Path: <simon.perreault@viagenie.ca>
X-Original-To: dtn-interest@ietfa.amsl.com
Delivered-To: dtn-interest@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 13AC821F999C; Wed, 17 Jul 2013 04:33:07 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.2
X-Spam-Level:
X-Spam-Status: No, score=-2.2 tagged_above=-999 required=5 tests=[AWL=-0.399, BAYES_00=-2.599, NO_RELAYS=-0.001, SARE_SUB_RAND_LETTRS4=0.799]
Received: from mail.ietf.org ([12.22.58.30]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id cHRkIpTRoTzV; Wed, 17 Jul 2013 04:33:04 -0700 (PDT)
Received: from jazz.viagenie.ca (jazz.viagenie.ca [IPv6:2620:0:230:8000::2]) by ietfa.amsl.com (Postfix) with ESMTP id 6C5E121F9133; Wed, 17 Jul 2013 04:33:04 -0700 (PDT)
Received: from [IPv6:::1] (unknown [IPv6:2001:660:3001:4012:5b8:1f9e:c21b:48d7]) by jazz.viagenie.ca (Postfix) with ESMTPSA id 2B622403EE; Wed, 17 Jul 2013 07:33:03 -0400 (EDT)
Message-ID: <51E680ED.6070004@viagenie.ca>
Date: Wed, 17 Jul 2013 13:33:01 +0200
From: Simon Perreault <simon.perreault@viagenie.ca>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7
MIME-Version: 1.0
To: Dirk Kutscher <Dirk.Kutscher@neclab.eu>
References: <82AB329A76E2484D934BBCA77E9F5249553A5FE9@DAPHNIS.office.hd>
In-Reply-To: <82AB329A76E2484D934BBCA77E9F5249553A5FE9@DAPHNIS.office.hd>
X-Enigmail-Version: 1.5.1
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 8bit
Cc: "draft-irtf-dtnrg-tcp-clayer@tools.ietf.org" <draft-irtf-dtnrg-tcp-clayer@tools.ietf.org>, "dtn-interest@irtf.org" <dtn-interest@irtf.org>, "Internet Research Steering Group (irsg@irtf.org)" <irsg@irtf.org>
Subject: Re: [dtn-interest] IRSG review of draft-irtf-dtnrg-tcp-clayer-06.txt
X-BeenThere: dtn-interest@irtf.org
X-Mailman-Version: 2.1.12
Precedence: list
List-Id: "The Delay-Tolerant Networking Research Group \(DTNRG\) - Announce." <dtn-interest.irtf.org>
List-Unsubscribe: <https://www.irtf.org/mailman/options/dtn-interest>, <mailto:dtn-interest-request@irtf.org?subject=unsubscribe>
List-Archive: <http://www.irtf.org/mail-archive/web/dtn-interest>
List-Post: <mailto:dtn-interest@irtf.org>
List-Help: <mailto:dtn-interest-request@irtf.org?subject=help>
List-Subscribe: <https://www.irtf.org/mailman/listinfo/dtn-interest>, <mailto:dtn-interest-request@irtf.org?subject=subscribe>
X-List-Received-Date: Wed, 17 Jul 2013 11:33:07 -0000

Thanks for your very detailed review! I agree with almost all your
proposed changes. I'll publish a new revision.

Le 2013-07-16 22:41, Dirk Kutscher a écrit :
> - section 3:
>   <comment>
>    The protocol overview needs to say that a TCPCL can be used to
>    transmit several bundles, but that multiple bundles MUST be
>    transmitted consecutively (i.e., no inter-leaving of bundle
>    segments). In addition, I think, you also have to say that the
>    blocks of a particular bundle MUST be sent sequentially (no change
>    of block order).
>   </comment>

Agreed. I would also add that interleaving can be accomplished with
bundle fragmentation.

> - section 3:
>   <oldtext>
>    This protocol provides bundle conveyance over a TCP connection and
>    specifies the encapsulation of bundles as well as procedures for TCP
>    connection setup and teardown.
>    </oldtext>
> 
>    <comment>
>     "The service of this protocol is the transmission of DTN bundles
>      over TCP. This document specifies the encapsulation of bundles,
>      procedures for TCP setup and teardown, and a set of messages and
>      node requirements."?
>    </comment>

Agreed.

> - section 3.1:
> 
>   <comment>
>    The first paragraph is not very clear. You want to say that there
>    are different specific messages for sending and receiving
>    operations (in addition to connection setup/teardown).  TCPCL is
>    symmetric, i.e., both sides can start sending data segments in a
>    connection, and one side's bundle transfer does not have to
>    complete before the other side can start sending data segments on
>    its own. Hence, the protocol allows for a bi-directional mode of
>    communication.
>   </comment>

Agreed.

> - section 3.2:
> 
>   <oldtext> 
>    Note that the sending node may transmit multiple
>    DATA_SEGMENT messages without necessarily waiting for the
>    corresponding ACK_SEGMENT responses.  This enables pipelining of
>    messages on a channel.
>   </oldtext> 
> 
>   <comment>
>    This is a too important property to just mention it on the side of
>    an example description. You should mention this in the protocol
>    overview. You should also mention that there is no explicit flow
>    control on the TCPCL layer.
>   </comment>

Agreed.

> - section 3.2:
> 
>   <oldtext> 
>     However, interleaving data segments from different bundles is not
>     allowed.
>   </oldtext> 
> 
>   <comment>
>    Again, this is too important to not mention it earlier.
>   </comment>

Agreed.

> -section 4:
> 
>  <oldtext>
>    The manner in which a bundle node makes the decision to establish
>    such a connection is implementation-dependent.
>  </oldtext>
> 
>  <comment> 
>   I think you want to say that the implementation can decide, possibly
>   case-by-case, at runtime (unless it is fixed). Perhaps better so
>   say: "It is up to the implementation to decide how and when
>   connection setup is triggered. For example..."
>  </comment>

Agreed.

> -section 4: 
>  <oldtext>
>    Therefore, the node MUST retry
>    the connection setup only after some delay and it SHOULD use a
>    (binary) exponential backoff mechanism to increase this delay in case
>    of repeated failures.
>  </oldtext>
> 
>  <comment>
>   You should specify what is meant by "some delay". Is it completely
>   up to the implementation? Do you want to RECOMMEND a minimum delay?
>  </comment>

How about recommending 1 second minimum?

> - section 4.1: 
> 
>  <oldtext>
>    magic:  A four byte field that always contains the byte sequence 0x64
>         0x74 0x6e 0x21, i.e., the text string "dtn!".
>  </oldtext>
> 
> 
>  <newtext>
>    magic:  A four byte field that always contains the byte sequence 0x64
>         0x74 0x6e 0x21, i.e., the text string "dtn!" in US-ASCII.
>  </newtext>

Agreed.

> - section 4.1: 
> 
>  <oldtext>
>    version:  A one byte field value containing the current version of
>         the protocol.
>  </oldtext>
> 
> 
>  <newtext>
>    version:  A one byte field value containing the value 3 (current version of
>         the protocol).
>  </newtext>

Agreed.

> - section 5.2
> 
>  <oldtext>
>   However, a node MUST be able to receive a bundle that has been
>    transmitted in any segment size.
>  </oldtext>
> 
>  <comment>
> 
>   How feasible is this, for example for constrained devices? Even if a
>   node does not intend to store and reassemble bundles, there may be
>   certain limits for the maximum size of segments that it can
>   receive. Has there been a discussion to make this
>   negotiable/configurable? How should a node react if it cannot
>   receive/process any more bytes for a transmitted segment?
>  </comment>

The intent here is not to override a node's intrinsic limitation on
bundle sizes. The intent is that if a node is able to receive, over TCP,
a bundle of size X with segment size Y, then it MUST also be able to do
it with any segment size <= X. I'll adjust the text.

> - section 5.2
> 
>  <oldtext>
>    Following the message header, the length field is an SDNV containing
>    the number of bytes of bundle data that are transmitted in this
>    segment.  Following this length is the actual data contents.
>  </oldtext>
> 
> 
>  <comment>
>   This should be mentioned earlier in section 5.2
>  </comment>

Agreed.

> - section 5.3 
> 
>   <oldtext>
>    To transmit an acknowledgment, a node first transmits a message
>    header with the ACK_SEGMENT type code and all flags set to zero, then
>    transmits an SDNV containing the cumulative length of the received
>    segment(s) of the current bundle.
>   </oldtext>
> 
> 
>   <newtext>
>    To transmit an acknowledgment, a node first transmits a message
>    header with the ACK_SEGMENT type code and all flags set to zero,
>    then transmits an SDNV containing the cumulative length in bytes of
>    the received segment(s) of the current bundle.
>   </newtext>

Agreed.

> - section 5.4
> 
>   <oldtext>
>    Note: If a bundle transmission if aborted in this way, the receiver
>    may not receive a segment with the 'E' flag set to '1' for the
>    aborted bundle.  The beginning of the next bundle is identified by
>    the 'S' bit set to '1', indicating the start of a new bundle.
>   </oldtext>
>   
>   <comment>
> 
>    Should this be turned into a general rule? I.e.: "if a receiver
>    receives a segment for a new bundle without having seen the final
>    segment of a previous bundle, it SHOULD disregard all segments of
>    the incompletely received bundle."

I don't think so. The received segments are valid. The receiver can do
whatever it wants with them.

>    (wording may need some work)
> 
>    It's probably good to use "SHOULD" here, because the node may
>    already have forwarded the segments...
>   </comment>
> 
> 
> - section 6.1
> 
>   <oldtext>
>    The format of the shutdown message is as follows:
> 
>                         1 1 1 1 1 1 1 1 1 1 2 2 2 2 2 2 2 2 2 2 3 3
>     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |  0x5  |0|0|R|D| reason (opt)  | reconnection delay (opt)      |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
>                Figure 8: Format of bundle shutdown messages
> 
>   </oldtext>
> 
>   <comment>
>    Is it really efficient to have variable length messages here by
>    making the reason code optional? How about always requiring it and
>    then allow for a default value (0x00)?
>   </comment>

I think it's a backward compatibility wart that we need to live with.
That is, earlier versions of the protocol did not send those two fields.
(not 100% sure)

> - section 6.1
> 
>   <oldtext>
>    If either node terminates a connection prematurely in this manner, it
>    SHOULD send a SHUTDOWN message and MUST indicate a reason code unless
>    the incoming connection did not include the magic string.
>   </oldtext>
> 
>   <comment>
>    Why not send a reason like "illegal syntax"?
>   </comment>

Wasn't there talks of a draft registering a bunch of additional reason
codes?

> - section 6.7
> 
>   <comment>
>   
>   Regarding the peer entity authentication issue, I think you have to
>   use a stronger statement, like "a node SHALL NO" use the endpoint
>   identifier conveyed in the TCPCL connection message to derive a peer
>   node's entity unless it can ascertain it via other means."

Agreed.

>   If it is possible to do that through the Bundle Security Protocol,
>   you could be a bit more specific on how the Bundle Authentication
>   Block would be used. I assume one TCPCL peer node would become a
>   security source in an RFC 6257 sense?
> 
>   BTW, you are using the term "Bundle Authentication Header" -- I
>   think it's "Bundle Authentication Block".
> 
>   In general, it could be worthwhile stating that TCPCL does not
>   provide any security services and that RFC 6257's mechanisms should
>   be used.

Agreed.

>   BTW, is the use of TLS for TCPCL defined or excluded? You may want
>   to say something about why it's not applicable
>   </comment>

Why wouldn't it be applicable? I think we *want* it to be applicable!

> - section 10.2
> 
>   <oldtext>
>    [RFC6256]  Eddy, W. and E. Davies, "Using Self-Delimiting Numeric
>               Values in Protocols", RFC 6256, May 2011.
>   </oldtext>
> 
>   <comment>
>     Isn't that a normative reference for this spec?
>   </comment>

No, because that's an informational RFC.

> EDITORIAL COMMENTS
> ------------------
> 
> General:
> 
> - make usage and capitalization of Bundle Protocol (sometimes referred
>   to as "Bundling Protocol", "bundle protocol" etc.) consistent

Agreed.

> - section 2.1
>    <oldtext>
>      Bundle payload --  A bundle payload (or simply "payload") is the
>         application data whose conveyance to the bundle's destination is
>         the purpose for the transmission of a given bundle.
>    </oldtext>
> 
>    <comment>
>      IMO this can be said in a clearer way: "... application
>      data that is transmitted to a BP application at a Bundle
>      Endpoint"?
>    </comment>

We should just remove this and refer to RFC 5050 section 3.1.

> - section 2.1:
>    <oldtext>
>    Bundle node --  A bundle node (or simply a "node") is any entity that
>         can send and/or receive bundles.  The particular instantiation
>         of this entity is deliberately unconstrained, allowing for
>         implementations in software libraries, long-running processes,
>         or even hardware.  One component of the bundle node is the
>         implementation of a convergence layer adapter.
>    </oldtext>
> 
>    <comment>
>      I don't think the second sentence ("The particular
>      instantiation...") is really needed.  This holds for most network
>      elements these days.
>    </comment>

We should just remove this and refer to RFC 5050 section 3.1.

> - section 2.1:
>    <oldtext>
>    Convergence layer adapter --  A convergence layer adapter (CLA) sends
>         and receives bundles utilizing the services of some 'native'
>         link, network, or internet protocol.  This document describes
>         the manner in which a CLA sends and receives bundles when using
>         the TCP protocol for inter-node communication.
>    </oldtext>
> 
>    <comment>
>       Does it make make sense to define "Convergence Layer" first,
>       before defining CLA? Is the CLA definition needed at all
>       (assuming CL is defined)?
>    </comment>

We should just remove this and refer to RFC 5050 section 3.1.

> - section 2.2:
>   <oldtext>
>    TCPCL Connection --  [...] The lifetime of a TCPCL connection is one-to-one
>         with the lifetime of an underlying TCP connection.
> 	[...]
>    </oldtext>
> 
>    <comment>
>       "The lifetime of a TCPCL connection is bound to the
>       lifetime of the underlying TCP connection."?
>    </comment>

Agreed.

> -section 4:
> 
>  <oldtext>
>    Once a TCP connection is established, each node MUST immediately
>    transmit a contact header over the TCP connection.  The format of the
>    contact header is described in Section 4.1).
>  </oldtext>
> 
>  <newtext>
>    Once a TCP connection is established, each node MUST immediately
>    transmit a contact header over the TCP connection.  The format of the
>    contact header is described in Section 4.1.
>  </newtext>

Agreed.

> - section 4.1:
> 
>  <oldtext>
>    local EID:  An octet string containing the EID of some singleton
>         endpoint in which the sending node is a member, in the canonical
>         format of <scheme name>:<scheme-specific part>.  A eight byte
>         EID is shown the clarity of the figure.
>  </oldtext>
> 
> 
>  <newtext>
>    local EID:  An octet string containing the EID of some singleton
>         endpoint in which the sending node is a member, in the canonical
>         format of <scheme name>:<scheme-specific part>.  A eight byte
>         EID is shown for clarity of the figure.
>  </newtext>

Agreed.

> - section 4.2:
> 
> 
>  <oldtext>
>         The reactive fragmentation enabled parameter is set to true iff
>         the corresponding flag is set in both contact headers.
> 
>         The bundle refusal capability may only be used iff both peers
>         indicate support for it in their contact header and segment
>         acknowledgement has been enabled.
>  </oldtext>
> 
> 
>  <newtext>
>         The reactive fragmentation enabled parameter is set to true iff
>         the corresponding flag is set in both contact headers.
> 
>         The bundle refusal capability is set to true if the
>         corresponding flag is set in both contact headers and if
>         segment acknowledgment has been enabled.
>  </newtext>
> 
>  <comment>
>   I'd raher use the same or similar wording to not irritate the implementer.
>  </comment>

Agreed.

> - section 5.1
> 
>  <oldtext>
>   Table 2: TCPCL Header Types
>  </oldtext>
> 
> 
>  <newtext>
>   Table 2: TCPCL Message Types
>  </newtext>

Agreed.

> - section 5
> 
>   <comment>
>    For the different subsections for the individual messages, please
>    use the message type names from table 2 in the headings, for
>    example: "Bundle Data Transmission (DATA_SEGMENT)". Also, I'd
>    recommend to align the order of the subsections with the order of
>    message types in table 2.
>   </comment>

Agreed.

>   <comment>
>    I would also recommend to use a consistent structure for each of
>    the subsections, i.e., perhaps
>      - purpose
>      - syntax
>      - procedures / node requirements
>   </comment>

Agreed.

> - section 5.3
> 
>   <oldtext>
>    If so, then
>    the receiver MUST transmit a bundle acknowledgment header when it
>    successfully receives each data segment.
>   </oldtext>
> 
> 
>   <oldtext>
>    If so, then
>    the receiver MUST transmit a bundle acknowledgment message when it
>    successfully receives each data segment.
>   </oldtext>

Agreed.

> - section 5.6
> 
>   <oldtext>
>    If no message (keepalive or other) has been received for at least
>    twice the keepalive interval, then either party may terminate the
>    session by transmitting a one byte message type code of SHUTDOWN (as
>    described in Table 2) and closing the TCP connection.
>   </oldtext>
> 
> 
>   <newtext>
>    If no message (keepalive or other) has been received for at least
>    twice the keepalive interval, then either party MAY terminate the
>    session by transmitting a one byte SHUTDOWN message  (as
>    described in Table 2) and by closing the TCP connection.
>   </newtext>

Agreed.

> - section 6.1
> 
>   <oldtext>
>    In case acknowledgments have been
>    negotiated, it is advisable to acknowledge all received data segments
>    first and then shut down the connection.
>   </oldtext>
> 
> 
>   <newtext>
>    In case acknowledgments have been
>    negotiated, a node SHOULD  acknowledge all received data segments
>    first and then shut down the connection.
>   </newtext>

Agreed.

> - section 6.1
> 
>   <oldtext>
>    This may, for example, be used to notify
>    that the node is currently not capable of or willing to communicate.
>   </oldtext>
> 
>   <newtext>
>    This may, for example, be used to notify
>    that the node is currently not able or willing to communicate.
>   </newtext>

Agreed.

> - section 10.2
> 
>   <oldtext>
>    [refs.dtnsecurity]
>               Symington, S., Farrell, S., and H. Weiss, "Bundle Security
>               Protocol Specification", Internet Draft, work in progress
>               draft-irtf-dtnrg-bundle-security-03.txt, April 2007.
>   </oldtext>
> 
>   <comment>
>      This is RFC 6257.
>   </comment>

Agreed.

Simon