Subject: Re: [clue] Benjamin Kaduk's Discuss on draft-ietf-clue-protocol-17: (with DISCUSS and COMMENT)

From: Simon Pietro Romano <[email-masked]>
Date: Thu, 4 Jul 2019 14:21:55 +0200

Hello Benjamin,

we finally managed to work on the new version of the draft. Please find below our answers (preceded by [SPR]) to your comments and suggestions.
Thanks a lot for your precious review!

Simon & Roberta

******************************************************************************************************************************************
Benjamin Kaduk [KADUK]
Discuss
Discuss (2018-11-19)

[KADUK] Thanks for the generally clear and well-written document!
I would like to discuss whether there needs to be more prominent coverage
of timers/timeouts, especially as relating to the state machines.  (I'd be happy
to learn that this is well-covered elsewhere in the document set; I just haven't
run into it yet.)

[SPR] We have indeed discussed this extensively within the WG. I believe the final resolution 
was not to cover timers/timeouts in a more prominent fashion and go for an experimental RFC type.

For the sake of completeness, I am reporting below the core part of the above mentioned discussions.
The following excerpts relate to Adam Roach’s AD review (and associated responses from the authors):

[ADAM] BLOCKER: General: There are several mentions of timeouts and retry thresholds in the text and its corresponding state machines; however, the document neither defines nor cites a document as defining what these timeout and retry values are. These need to be defined and described. If the timer and retry scheme allows the two ends of the connection to have different values for timeouts and number of retries, then there need to be additional error procedures that allow the MC and MP state machines to stay in sync (if the timer/retry values can be different, it's possible for one state machine to transition to "terminated," while the other is still active, and you need messaging to clean this up).

[SPR] Our idea was to rely on a single, fixed, application-level (i.e., CLUE-level) timeout value, rather than leveraging a number of them (T1, T2, T4, plus Timer A through Timer K) as in SIP. The value of the one and only timer would be the usual RTT estimate (~500ms). Differently from protocols like SIP, we are also leveraging retransmit counts to get out of a timeout-driven retransmission loop. We’re not adding such information to the revised version of the draft, because we’d first like to get your feedback on the approach. If you (and the rest of the group) believe this is a feasible approach, we can expand on that in a further version of the document.

[ADAM] In thinking through what this scheme should look like, it occurs to me that the CLUE messages are defined to be sent over a reliable transport (SCTP), which has its own retransmission timers and eventual timeouts. Implementing a retransmission scheme on top of a reliable transport -- especially one as aggressive as you suggest above -- will put more traffic on the network when congestion occurs rather than less.
So, in the final analysis, I think the action here is to remove retransmission timers and retry counts altogether. If the underlying transport takes longer to detect a failure than is sensible for CLUE (and it likely does), then a supervisory timer that declares the session failed might make sense.

[KADUK] In a similar vein, do we want to have any treatment of avoiding infinite loops
(e.g., when a 'configure' or 'advertisement' is rejected in expectation of modification
but the sending implementation continues to generate an identical message)?

[SPR] As per above.

[KADUK] It is not clear to me that any change to the document text is needed in either case,
but I don't know to what extent the topics have already been discussed.

******************************************************************************************************************************************

[KADUK] 
I also have some substantial comments that do not rise to Discuss-level.

How do I know which endpoint is the channel initiator and which is the
channel receiver?
draft-ietf-clue-signaling suggests that the DTLS client is the channel
initiator, but even that is not explicit about it -- the protocol could be
considered under-specified if there is insufficient clarity.

[SPR] The CLUE data Chanel is established as described in draft-ietf-clue-datachannel. The cited document clearly states the following:

"This document defines how to use the WebRTC data channel mechanism
   [I-D.ietf-rtcweb-data-channel] to realize a data channel, referred to
   as a CLUE data channel, for transporting CLUE protocol
   [I-D.ietf-clue-protocol] messages between two CLUE entities."

The CLUE channel initiator is hence the peer which creates the WebRTC data channel (e.g., which calls 
RTCPeerConnection.createDataChannel()) in the first place.

[KADUK] Section 2

The MCU definition doesn't actually expand the acronym, which seems a
little reader-unfriendly.

[SPR] Fixed

[KADUK]

Section 5.1

There are perhaps more XML extension points in here than is reasonable for
some of these elements (e.g., <versionsListType>).

[SPR] I'm not sure I understand this correctly. Is this a request for changing the schema? If so, what is the suggested change?

[KADUK]

Section 5.2

                           If the responseCode is between 200 and 299
   inclusive, the response MUST also include <mediaProvider>,
   <mediaConsumer>, <version> and <commonExtensions> elements;

Maybe re-mention that MP and MC are booleans here.

[SPR] Done.

[KADUK]

   Finally, the commonly supported extensions are copied in the
   <commonExtensions> field.

Does this need to say that only extensions that are applicable to the
negotiated protocol version are included?  

[SPR] YES.

[KADUK]

(Also, how does one handle an
extension that exists for multiple major versions -- are there two
<extension> elments for it in the <options> message?)

[SPR] YES. Each <extension> element includes a child <version> element for this purpose. See, e.g., the example in section 10.1

[KADUK] 

   Upon reception of the 'optionsResponse' the version to be used is the
   one provided in the <version> tag of the message.  The following CLUE
   messages MUST use such a version number in the "v" attribute.  The
   allowed extensions in the CLUE dialogue are those indicated in the
   <commonExtensions> of the 'optionsResponse' message.

Couldn't this restriction on the "v" value apply even to the
'optionsResponse' message?

[SPR] The "v" attribute value in the "optionsResponse" message contains the name of the agreed-upon version. If present, the <commonExtensions>
child element in the optionsResponse message contains the list of the commonly extensions extensions for the agree-upon protocol version.
See, e.g., section 10.2 in the draft.

[KADUK]

Section 5.3

   The 'advertisement' message is used by the MP to advertise the
   available media captures and related information to the MC.  [...]

I'd consider avoiding the definite article "the" to refer to MP/MC roles,
since in many caess there will be 2+ of each, and we don't want to confuse
the reader into thinking that there is an MP/CR equivalence or something
like that.  So, perhaps "each MP" and "the corresponding MC".

[SPR] Good catch. Fixed.

[KADUK]

Section 5.4

                             As it can be seen from the message schema
   provided in the following excerpt (Figure 6), the 'ack' contains a
   response code and a reason string for describing the processing
   result of the 'advertisement'.  [...]

[the reason string is part of the base clueResponseType]
The text quoted here could be read as implying that the reason string is
required in the 'ack' message, a stronger requirement than of the base
clueResponseType where it has minOccurs=0.  Some greater clarity in the
text here is probably called for, especially since when the 'ack' is
piggybacked on a 'configure' message, there is no provision for a reason
string at all. 

[SPR] Right. Modified as follows: "the 'ack' contains a
   response code and may contain a reason string for describing the processing
   result of the 'advertisement'".

[KADUK]

Section 5.5

                             The <ack> element MUST NOT be present if an
   'ack' message has been already sent back to the MP.

I think you need to clarify that this is scoped to the current
advSequenceNr.

[SPR] Right. Modified accordingly.

[KADUK]

Section 5.6

                                             It contains (Figure 8) a
   response code with a reason string indicating either the success or 
   the failure (along with failure details) of a 'configure' request
   processing.  [...]

[Same comment about reason string as for 'ack']

[SPR] Same fix as per above.

[KADUK]

Section 5.7
   
   Such new response codes MUST NOT overwrite the ones here defined and
   they MUST respect the semantics of the first code digit.

nit: is this "overwrite" or "override"?

[SPR] "Override", thanx for spotting that out.
  
[KADUK]

   This document does not define response codes starting with "1", and
   such response codes are not allowed to appear in major version 1 of
   the CLUE protocol.  The range from 100 to 199 inclusive is reserved
   for future major versions of the protocol to define response codes
   for delayed or incomplete operations if necessary.  Response codes
   starting with "5" through "9" are reserved for future major versions
   of the protocol to define new classes of response, and are not 
   allowed in major version 1 of the CLUE protocol.  Response codes 
   starting with "0" are not allowed.
   
This text seems to also preclude extensions to major version '1' from
defining 1xx or [5-9]xx reason codes; is that the intent?

[SPR] Yes, it is. Extensions can indeed define new pieces of information to be carried within existing CLUE messages; 
though, they must provide their own schema and be defined in the framework of external namespaces.

[KADUK]

Section 6

   When the CLUE data channel set up starts ("start channel"), the CP
   moves from the IDLE state to the CHANNEL SETUP state.

nit: only one of "sets up" and "starts" is needed.

[SPR] Fixed. 

[KADUK]

   When in the ACTIVE state, the CP starts the envisioned sub-state
   machines (i.e., the MP state machine and the MC state machine)
   according to the roles it plays in the telepresence sessions.  Such
   roles have been previously declared in the 'options' and
   'optionsResponse' messages involved in the initiation phase (see
   OPTIONS sections Section 5.1 and Section 5.2 for the details).  [...]
   
My reading of the initiation phase is that each CP sends only a boolean
indication of whether it supports the MP/MC roles, and so each party has to
determine on its own whether it will act as a MP and/or MC; is that
correct?  If so, do we need to say anything about how the boolean matrix
translates to activating the respective sub-state machines?

[SPR] Are you suggesting that we remove the second part of the sentence from there?

[KADUK]

Section 6.1

                         'configure+ack' messages referring to out-of-
   date (i.e., having a sequence number equal to or less than the
   highest seen so far) advertisements MUST be ignored, i.e., they do
   not trigger any state transition.  [...]

Is this really less than or equal or just less than?  Also, is "seen" the
right verb, since IIUC these are sequence numbers that the MP has
*generated* in its advertisements?

[SPR] Good catch. Both comments are valid. Text modified accordingly.

[KADUK]

Section 7

                                                          In other
   words, in this example, the MP MUST use version 1.4 and downgrade to
   the lower version.  [...]

nit: does the phrase "and downgrade to the lower version" add any value
here?  The word "downgrade" can have negative connotations in some other
contexts, so if it's not adding value I'd suggest avoiding it.

[SPR] Agreed.

[KADUK]

Section 8

   As reported in Figure 13, the values of the fields of the <extension>
   element (either new information or new messages) take the following
   values:
[...]
   o  the major standard version of the protocol that the extension
      refers to.

The XSL includes a full version (including minor), even though the
semantics basically only use the major version.  That said, why is the
'version' element minOccurs="0" -- what are the semantics when it is
absent?

[SPR] You're right. We removed the minoccurs="0" from both "schemaRef" and "version".

[KADUK]

Section 8.1

   The CLUE data model document ([I-D.ietf-clue-data-model-schema])
   envisions the possibility of adding this kind of "extra" information
   in the description of a video capture by keeping the compatibility
   with the CLUE data model schema.  [...]

nit: I don't think this is grammatical; maybe just "keeping compatibility".

[SPR] We removed "by keeping compatibility..." from the sentence.

[KADUK]

Section 10

This claims to be a "call flow" example, but the described flows only
contain a single unidirectional media flow, which is not really consistent
with the normal usage of the word "call".  Buried in the body text there is
a disclaimer:
   [...]                      For the sake of simplicity, the following
   call flow focuses only on the dialogue between MP CP1 and MC CP2.
I would suggest making the presence of this simplification much clearer
from the start, perhaps "CLUE protocol messages exchanged in the following
call flow are detailed; only one direction of media is shown for
simplicity, as the other direction is precisely symmetric".

[SPR] Fixed.

[KADUK]

   CP2 acknowledges the second 'advertisement' message with an 'ack'
   message (Section 10.7).

   In a second moment, CP2 changes the requested media streams from CP1
   by sending to CP1 a 'configure' message replacing the previously
   selected video streams with the new composed media streams advertised
   by CP1 (Section 10.8).

This might be an appropriate place to indicate that media from the previous
configuration continue to flow during the reconfiguration process.

[SPR] Done.

[KADUK]

It might also be worth noting again somewhere in here (or a subsection)
that there are three (well, two, since we only show one direction of media)
distinct sequence number spaces per sender, and that the discontinuity
between Section 10.2 and 10.3's numbers is correct.

[SPR] Done

[KADUK]

Section 11

Thanks for the well-thought-through security considerations; in combination
with the linked documents (particularly the framework), they cover all the
considerations (especially privacy considerations) that I had in mind.

[SPR] Thanx!

[KADUK]

Section 14.2

We appear to be citing both 5117 and 7667, whereas the latter obsoletes the
former.

[SPR] Fixed.

                     				            _\\|//_
                           				   ( O-O )
      ~~~~~~~~~~~~~~~~~~~~~~o00~~(_)~~00o~~~~~~~~~~~~~~~~~~~~~~~~
                    				Simon Pietro Romano
             				 Universita' di Napoli Federico II
                		     Computer Engineering Department 
	             Phone: +39 081 7683823 -- Fax: +39 081 7683816
                                           e-mail: [email-masked]

		    <<Molti mi dicono che lo scoraggiamento è l'alibi degli 
		    idioti. Ci rifletto un istante; e mi scoraggio>>. Magritte.
               			                     oooO
       ~~~~~~~~~~~~~~~~~~~~~~~(   )~~~ Oooo~~~~~~~~~~~~~~~~~~~~~~~~~
					                 \ (            (   )
			                                  \_)          ) /
                                                                       (_/

> Il giorno 19 nov 2018, alle ore 19:25, Benjamin Kaduk <[email-masked]> ha scritto:
> 
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-clue-protocol-17: Discuss
> 
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-clue-protocol/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Thanks for the generally clear and well-written document!
> I would like to discuss whether there needs to be more prominent coverage
> of timers/timeouts, especially as relating to the state machines.  (I'd be happy
> to learn that this is well-covered elsewhere in the document set; I just haven't
> run into it yet.)
> 
> In a similar vein, do we want to have any treatment of avoiding infinite loops
> (e.g., when a 'configure' or 'advertisement' is rejected in expectation of modification
> but the sending implementation continues to generate an identical message)?
> 
> It is not clear to me that any change to the document text is needed in either case,
> but I don't know to what extent the topics have already been discussed.
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> I also have some substantial comments that do not rise to Discuss-level.
> 
> How do I know which endpoint is the channel initiator and which is the
> channel receiver?
> draft-ietf-clue-signaling suggests that the DTLS client is the channel
> initiator, but even that is not explicit about it -- the protocol could be
> considered under-specified if there is insufficient clarity.
> 
> Section 2
> 
> The MCU definition doesn't actually expand the acronym, which seems a
> little reader-unfriendly.
> 
> Section 5.1
> 
> There are perhaps more XML extension points in here than is reasonable for
> some of these elements (e.g., <versionsListType>).
> 
> Section 5.2
> 
>                           If the responseCode is between 200 and 299
>   inclusive, the response MUST also include <mediaProvider>,
>   <mediaConsumer>, <version> and <commonExtensions> elements;
> 
> Maybe re-mention that MP and MC are booleans here.
> 
>   Finally, the commonly supported extensions are copied in the
>   <commonExtensions> field.
> 
> Does this need to say that only extensions that are applicable to the
> negotiated protocol version are included?  (Also, how does one handle an
> extension that exists for multiple major versions -- are there two
> <extension> elments for it in the <options> message?)
> 
>   Upon reception of the 'optionsResponse' the version to be used is the
>   one provided in the <version> tag of the message.  The following CLUE
>   messages MUST use such a version number in the "v" attribute.  The
>   allowed extensions in the CLUE dialogue are those indicated in the
>   <commonExtensions> of the 'optionsResponse' message.
> 
> Couldn't this restriction on the "v" value apply even to the
> 'optionsResponse' message?
> 
> Section 5.3
> 
>   The 'advertisement' message is used by the MP to advertise the
>   available media captures and related information to the MC.  [...]
> 
> I'd consider avoiding the definite article "the" to refer to MP/MC roles,
> since in many caess there will be 2+ of each, and we don't want to confuse
> the reader into thinking that there is an MP/CR equivalence or something
> like that.  So, perhaps "each MP" and "the corresponding MC".
> 
> Section 5.4
> 
>                             As it can be seen from the message schema
>   provided in the following excerpt (Figure 6), the 'ack' contains a
>   response code and a reason string for describing the processing
>   result of the 'advertisement'.  [...]
> 
> [the reason string is part of the base clueResponseType]
> The text quoted here could be read as implying that the reason string is
> required in the 'ack' message, a stronger requirement than of the base
> clueResponseType where it has minOccurs=0.  Some greater clarity in the
> text here is probably called for, especially since when the 'ack' is
> piggybacked on a 'configure' message, there is no provision for a reason
> string at all.
> 
> Section 5.5
> 
>                             The <ack> element MUST NOT be present if an
>   'ack' message has been already sent back to the MP.
> 
> I think you need to clarify that this is scoped to the current
> advSequenceNr.
> 
> Section 5.6
> 
>                                             It contains (Figure 8) a
>   response code with a reason string indicating either the success or
>   the failure (along with failure details) of a 'configure' request
>   processing.  [...]
> 
> [Same comment about reason string as for 'ack']
> 
> Section 5.7
> 
>   Such new response codes MUST NOT overwrite the ones here defined and
>   they MUST respect the semantics of the first code digit.
> 
> nit: is this "overwrite" or "override"?
> 
>   This document does not define response codes starting with "1", and
>   such response codes are not allowed to appear in major version 1 of
>   the CLUE protocol.  The range from 100 to 199 inclusive is reserved
>   for future major versions of the protocol to define response codes
>   for delayed or incomplete operations if necessary.  Response codes
>   starting with "5" through "9" are reserved for future major versions
>   of the protocol to define new classes of response, and are not
>   allowed in major version 1 of the CLUE protocol.  Response codes
>   starting with "0" are not allowed.
> 
> This text seems to also preclude extensions to major version '1' from
> defining 1xx or [5-9]xx reason codes; is that the intent?
> 
> Section 6
> 
>   When the CLUE data channel set up starts ("start channel"), the CP
>   moves from the IDLE state to the CHANNEL SETUP state.
> 
> nit: only one of "sets up" and "starts" is needed.
> 
>   When in the ACTIVE state, the CP starts the envisioned sub-state
>   machines (i.e., the MP state machine and the MC state machine)
>   according to the roles it plays in the telepresence sessions.  Such
>   roles have been previously declared in the 'options' and
>   'optionsResponse' messages involved in the initiation phase (see
>   OPTIONS sections Section 5.1 and Section 5.2 for the details).  [...]
> 
> My reading of the initiation phase is that each CP sends only a boolean
> indication of whether it supports the MP/MC roles, and so each party has to
> determine on its own whether it will act as a MP and/or MC; is that
> correct?  If so, do we need to say anything about how the boolean matrix
> translates to activating the respective sub-state machines?
> 
> Section 6.1
> 
>                         'configure+ack' messages referring to out-of-
>   date (i.e., having a sequence number equal to or less than the
>   highest seen so far) advertisements MUST be ignored, i.e., they do
>   not trigger any state transition.  [...]
> 
> Is this really less than or equal or just less than?  Also, is "seen" the
> right verb, since IIUC these are sequence numbers that the MP has
> *generated* in its advertisements?
> 
> Section 7
> 
>                                                          In other
>   words, in this example, the MP MUST use version 1.4 and downgrade to
>   the lower version.  [...]
> 
> nit: does the phrase "and downgrade to the lower version" add any value
> here?  The word "downgrade" can have negative connotations in some other
> contexts, so if it's not adding value I'd suggest avoiding it.
> 
> Section 8
> 
>   As reported in Figure 13, the values of the fields of the <extension>
>   element (either new information or new messages) take the following
>   values:
> [...]
>   o  the major standard version of the protocol that the extension
>      refers to.
> 
> The XSL includes a full version (including minor), even though the
> semantics basically only use the major version.  That said, why is the
> 'version' element minOccurs="0" -- what are the semantics when it is
> absent?
> 
> Section 8.1
> 
>   The CLUE data model document ([I-D.ietf-clue-data-model-schema])
>   envisions the possibility of adding this kind of "extra" information
>   in the description of a video capture by keeping the compatibility
>   with the CLUE data model schema.  [...]
> 
> nit: I don't think this is grammatical; maybe just "keeping compatibility".
> 
> Section 10
> 
> This claims to be a "call flow" example, but the described flows only
> contain a single unidirectional media flow, which is not really consistent
> with the normal usage of the word "call".  Buried in the body text there is
> a disclaimer:
>   [...]                      For the sake of simplicity, the following
>   call flow focuses only on the dialogue between MP CP1 and MC CP2.
> I would suggest making the presence of this simplification much clearer
> from the start, perhaps "CLUE protocol messages exchanged in the following
> call flow are detailed; only one direction of media is shown for
> simplicity, as the other direction is precisely symmetric".
> 
>   CP2 acknowledges the second 'advertisement' message with an 'ack'
>   message (Section 10.7).
> 
>   In a second moment, CP2 changes the requested media streams from CP1
>   by sending to CP1 a 'configure' message replacing the previously
>   selected video streams with the new composed media streams advertised
>   by CP1 (Section 10.8).
> 
> This might be an appropriate place to indicate that media from the previous
> configuration continue to flow during the reconfiguration process.
> 
> It might also be worth noting again somewhere in here (or a subsection)
> that there are three (well, two, since we only show one direction of media)
> distinct sequence number spaces per sender, and that the discontinuity
> between Section 10.2 and 10.3's numbers is correct.
> 
> Section 11
> 
> Thanks for the well-thought-through security considerations; in combination
> with the linked documents (particularly the framework), they cover all the
> considerations (especially privacy considerations) that I had in mind.
> 
> Section 14.2
> 
> We appear to be citing both 5117 and 7667, whereas the latter obsoletes the
> former.
> 
> 
> _______________________________________________
> clue mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/clue
>