Subject: [Tsv-art] Tsvart telechat review of draft-ietf-clue-datachannel-15

From: Joerg Ott via Datatracker <[email-masked]>
Date: Sat, 06 Apr 2019 03:10:20 -0700

Reviewer: Joerg Ott
Review result: Almost Ready

This document has been reviewed as part of the transport area review team's
ongoing effort to review key IETF documents. These comments were written
primarily for the transport area directors, but are copied to the document's
authors and WG to allow them to address any issues raised and also to the IETF
discussion list for information.

When done at the time of IETF Last Call, the authors should consider this
review as part of the last-call comments they receive. Please always CC
[email-masked] if you reply to or forward this review.

Please see the comments below.

Best,
Jörg

Reviewing this document for tsv-art, I don't see any any transport-specific
issues in the document, but a bunch of other (smaller) issues and nits came up.

I am not sure that that the reference to a "SIP User Agent" as an entity is
strictly correct here. SIP User Agents are defined in RFC 3261 to carry out
SIP transactions. Just speaking about "User Agents" may be more accurate.
But this is just a side note.

Section 3.2.3 defines behaviour in a negative way, i.e., via MUST NOT
statements, excluding all but one possibility. This could be problematic in
the future if another option gets added to SCTP usage, which would then
implicitly be allowed. It would be better if the behaviour was defined i
 a positive way, i.e., using MUST.

Sect. 3.2.5, 2nd para: use "MUST" instead of "must" since this appears to
be normative, so RFC 2119 wording should be used.

Sect. 3.2.7, 1st para: this appears t be normative language and thus
should use the RFC 2119 keywords.

Sect. 3.3.1.1, table 1 + 2nd para after table 1: the text in the 2nd para
defines the value for the "application usage"; this should also be reflected
in table 1 since it seems that only one application usage is defined.

Sect. 3.3.1.2.: this again appears to be normative, so RFC 2119 language
should be used.
What does the sentence "A CLUE entity can choose any valid SCTP port
value." mean in this context?  Is a "valid SCTP port" value that of a
previously established SCTP connection within the context of the
SIP session? If such a match is desired it should be specified or a
reference to a peer document (draft-ietf-mmusic-sctp-sdp-26?) 
should be provided.

Sect. 3.3.2: NOTE statement: It is ok to have a note but this still is
somewhat normative. It should also be specified what happens if
the values _are_ set by the peer. What is the error handling?
Ignore? Reject the connection setup?

Figure 1 is a nice example. But it would be even better if a complete
example with the SDP for the data channel setup (in the previous
or the same offer/answer exchange) be given. Makes life easier
for readers and implementers.

Editorial:
Don't just copy the first paragraph(s) from the introduction to create an abstract.
3.2.2, 2nd para, 3rd line: "what" -> "which"

Sect. 3.3.1: the ref to the clue-signalling draft is missing a link (all other refs have one).

Fix the formatting of table 2 to avoid letter breaking from words.

---

From: Christer Holmberg <[email-masked]>
Date: Wed, 10 Apr 2019 18:07:14 +0000

Hi Joerg,

Thank You for the comments! Please see inline.
   
>    But this is just a side note.

I think it is good to have "SIP" there for clarity.
    
----

>     a positive way, i.e., using MUST.
  
In general I agree with you. But, in the case the base data channel spec mandates a set of SCTP extensions, and the text says that a couple of them must not be used for CLUE. I think that is the clearest way.

(And, if additional SCTP extensions are added to rtcweb-data-channel in the future, that could cause problems no matter if we use MUST or MUST NOT, depending on whether such extensions can be used with CLUE or not.)

----

>    be normative, so RFC 2119 wording should be used.

The reason I use "must" is because the text is referencing requirements in another specification, i.e., the requirement is not defined or introduced by this draft. That is based on guidance I have received on earlier drafts.

----    

>    should use the RFC 2119 keywords.

As for your comment on 3.2.5, the text is simply referencing requirements defined elsewhere.

----
    
>    in table 1 since it seems that only one application usage is defined.
  
I don't know how I would get it into the table, as it is a generic description of an m- line for SCTP over DTLS.

I could of course copy/paste the table, indicate that it shows the m- line for CLUE, and replace "application usage" with "webrtc-datachannel". But, I am not sure that would make things more clear.

----
  
>    reference to a peer document (draft-ietf-mmusic-sctp-sdp-26?) 
  
draft-ietf-mmusic-sctp-sdp specifies how the port is used, and the port range, so suggest adding a reference to draft-ietf-mmusic-sctp-sdp.

---

>    Ignore? Reject the connection setup?
  
I guess we could allow two options: either discard the parameters, or reject the SDP and take proper actions to release the connection.

I am also ok "de-noting" the text.
  
----

>    for readers and implementers.
  
draft-ietf-clue-signaling contains more complete SDP examples, so I would suggest to add a reference there.

--------

>    Don't just copy the first paragraph(s) from the introduction to create an abstract.

I will see if I can make the Abstract shorter.

----

>    3.2.2, 2nd para, 3rd line: "what" -> "which"
  
Will fix.

----

>    Sect. 3.3.1: the ref to the clue-signalling draft is missing a link (all other refs have one).
  
Not sure I understand. What do you mean by "missing a link"?

----
  
>    Fix the formatting of table 2 to avoid letter breaking from words.
  
Will do.

Thanks!

Regards,

Christer

---

From: Christer Holmberg <[email-masked]>
Date: Fri, 12 Apr 2019 15:47:09 +0000

Hi,

...
      
>>  Fix the formatting of table 2 to avoid letter breaking from words.
   
I am not really sure how to avoid the letter breaking.

Regards,

Christer

---

From: Joerg Ott <[email-masked]>
Date: Wed, 17 Apr 2019 22:31:58 +0200

Hi Christer,

thanks, also for the updated draft.  See inline.

> I think it is good to have "SIP" there for clarity.

I don't think this is correct but it is marginal and I see your point.

> (And, if additional SCTP extensions are added to rtcweb-data-channel in the future, that could cause problems no matter if we use MUST or MUST NOT, depending on whether such extensions can be used…

Well, if you have 4 features A, B, C, D and you state that B and C MUST
NOT be used, this leaves A and D.  If somebody adds E to SCTP, then E is
implicitly allowed.  I think it is just easier to spell out what
implementers are supposed to do.

> The reason I use "must" is because the text is referencing requirements in another specification, i.e., the requirement is not defined or introduced by this draft. That is based on guidance I have…

Ok.  Maybe make this explicit in the text.  "According to RFCxxxx, ..."

> As for your comment on 3.2.5, the text is simply referencing requirements defined elsewhere.

See above.

> I could of course copy/paste the table, indicate that it shows the m- line for CLUE, and replace "application usage" with "webrtc-datachannel". But, I am not sure that would make things more clear.

All I am saying is that all other table entries refer to explicit values
while this one points indirectly to the text.  There is nothing wrong
technically, rather a matter of presentation.

> draft-ietf-mmusic-sctp-sdp specifies how the port is used, and the port range, so suggest adding a reference to draft-ietf-mmusic-sctp-sdp.

Sounds good.

> I am also ok "de-noting" the text.

Ok

> draft-ietf-clue-signaling contains more complete SDP examples, so I would suggest to add a reference there.

Yepp, this works for me.

> I will see if I can make the Abstract shorter.

I want you to make it different.  An abstract is _not_ the introduction
even though the first bits may overlap.

> Not sure I understand. What do you mean by "missing a link"?

It's missing a hyperlink.  No idea why but all other citations seem to
have hyperlinks.

>>     Fix the formatting of table 2 to avoid letter breaking from words.

As you said in a different email, this may be hard.  Hyphenation is the
only thing that comes to mind.  Or abbreviation.  The present way looks
just broken.

But essentially, this is close to done.

Best,
Jörg

---

From: Christer Holmberg <[email-masked]>
Date: Thu, 18 Apr 2019 13:33:52 +0000

Hi Joerg,

    >> In general I agree with you. But, in the case the base data channel spec mandates a set of SCTP extensions, 
    >> and the text says that a couple of them must not be used for CLUE. I think that is the clearest way.
    >> 
    >> (And, if additional SCTP extensions are added to rtcweb-data-channel in the future, that could cause problems no matter 
    >> if we use MUST or MUST NOT, depending on whether such extensions can be used with CLUE or not.)
    >
    
    The default is that implementers do whatever rtcweb-data-channel says, and then we only explicitly indicate the exceptions. 
  
    Also, if someone writes an RFC X that adds feature "E" to SCTP, "E" will not automatically be incorporated into rtcweb-data-channel and clue-datachannel. Those specs would have to be updated, with an explicit reference to RFC X in order to incorporate "E".

    ANY technology we reference could be extended with new features.
 
    ----
     
    >> The reason I use "must" is because the text is referencing requirements in another specification, i.e., the 
    >> requirement is not defined or introduced by this draft. That is based on guidance I have received on earlier drafts.
    >
    
     The text in the latest version (-17) now says:

   "As defined in [I-D.ietf-rtcweb-data-channel], the dynamic address
   reconfiguration extension ('Supported Extensions Parameter'
   parameter) defined in [RFC5061] must be used to signal the support of
   the stream reset extension defined in [RFC6525].  Other features of
   [RFC5061] MUST NOT be used for CLUE data channels."

    ----
     
    >> As for your comment on 3.2.5, the text is simply referencing requirements defined elsewhere.
    
    The text in version -17 says:

   "As described in [I-D.ietf-rtcweb-data-channel], in order to close a
   data channel, an entity sends an SCTP reset message [RFC6525] on its
   outgoing SCTP stream associated with the data channel.  When the
   remote peer receives the reset message, it also sends (unless already
   sent) a reset message on its outgoing SCTP stream associated with the
   data channel.  The SCTPoDTLS association, and other data channels
   established on the same association, are not affected by the SCTP
   reset messages."

    ----
          
    >> I could of course copy/paste the table, indicate that it shows the m- line for CLUE, and replace "application usage" 
    >> with "webrtc-datachannel". But, I am not sure that would make things more clear.
    >

    I guess I could keep the generic text saying that mmusic-sctp-sdp defines how the m= line parts are set, and then say that the table shows the values for a CLUE data channel.
    
    --------
     
    > even though the first bits may overlap.
    
    I agree, but as this is a relatively simple draft, I see no reason in making it different just to make it different __

    But, in version -17 I did remove stuff that I don't think need to be in the Abstract, and it now says:

    "This document defines how to use the WebRTC data channel mechanism in
     order to realize a data channel, referred to as a CLUE data channel,
     for transporting CLUE protocol messages between two CLUE entities."

    ---

    > It's missing a hyperlink.  No idea why but all other citations seem to
    
    Aaah... now I get it. I was reading the pure text version, without any links __

    The XML code for this reference is identical to the code for same as references, where the link does exist, so I am not sure where the problem is.

    ----
        
    > only thing that comes to mind.  Or abbreviation.  The present way looks
    
    I was thinking about abbreviation, but I'd like to use the same terminology as in mmusic-data-channel-sdpneg.
 
    But, I tried by using "sub-protocol" and "Appli-cation", and that makes it better.
    
   Regards,

   Christer

---

From: Christer Holmberg <[email-masked]>
Date: Sun, 21 Apr 2019 11:15:20 +0000

Hi,

I have created a pull request with some additional fixes based on Joerg's comments:

https://github.com/cdh4u/draft-clue-datachannel/pull/3

Regards,

Christer

On 18/04/2019, 16.34, "Christer Holmberg" <[email-masked]> wrote:

    Hi Joerg,

      

        ----

    
    
        ----

    
    
        ----

        
        --------

    

        ---

    
    
        ----

     
        
       Regards,
    
       Christer