Re: [clue] AD Review: draft-ietf-clue-protocol-13

[Adam Roach <[e-mail]>]

Ah, I almost forgot -- please replace the RFC2119 boilerplate in section 
3 with RFC8174 boilerplate. Thanks!

/a

On 11/3/17 3:06 PM, Adam Roach wrote:
> Thanks for your thorough response! I've removed those issues that 
> you've addressed, and included responses to the remaining issues below.
>
> IMPORTANT NOTE TO CLUE WORKING GROUP: There is a discussion regarding 
> retransmission timers that you need to have. See the comments below 
> for details.
>
>
> On 10/6/17 1:37 PM, Simon Pietro Romano wrote:
>>
>>> Title: The rule of thumb is that all but a small handful of 
>>> well-known acronyms need to be expanded in titles and abstracts. I 
>>> recognize that "CLUE" is a bit tortured, as acronyms go, but the 
>>> title of this document is, broadly speaking, opaque. Please change 
>>> it to something meaningful, such as "Protocol for Controlling 
>>> Multiple Streams for Telepresence (CLUE)”
>>>
>> [SPR] Done.
>
> I think this got missed. The header of the current version still reads:
>
> CLUE Working Group R. Presta
> Internet-Draft S. P. Romano
> Intended status: Standards Track University of Napoli
> Expires: April 9, 2018 October 6, 2017
>
>                              CLUE protocol
>                       draft-ietf-clue-protocol-14
>
>>
>>> General: There are three instances of excessively long lines in the 
>>> document.
>>>
>> [SPR] ???
>
> The reformatting of XML in -14 has fixed this issue.
>
>> 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.
>
> 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.
>
>>
>>> Section 5: The XML definition allows zero or more <clueId> elements 
>>> to appear in a message. If more than one is allowed, the document 
>>> should explain how multiple IDs are handled. If they are not, then 
>>> the schema and/or text needs to prohibit having more than one.
>>>
>> [SPR] Actually, the schema states:
>>
>> <xs:element name="clueId" type="xs:string" minOccurs="0”/>
>>
>> This should indicate that clueId can appear either 0 or 1 times in a 
>> valid XML document. The default value for maxOccurs is in fact 1. 
>> Don’t you agree on that?
>
> Ah, you're correct. I'm not used to seeing minOccurs without 
> maxOccurs, but this is entirely fine as it's currently written.
>
>>
>>> BLOCKER: Section 5.1: The description of <supportedVersions> 
>>> describes a scheme in which multiple supported versions can be 
>>> listed; and, if the list is omitted, it implies that only the 
>>> version described in <v> is supported. This text does not define 
>>> (nor does any other text that I can find) what <v> should be set to 
>>> when <supportedVersions> is used. Intuitively, it seems that <v> 
>>> should be set to the largest minor version of the smallest major 
>>> version advertised in <supportedVersions>, but that (or whatever the 
>>> correct answer is) needs to be clearly spelled out.
>>>
>> [SPR] I believe we were giving for granted that “v” should be, in 
>> such case, the highest supported version (i.e., largest major and 
>> larger minor numbers). Now that you make me think around that, I 
>> would simply say that the “v” attribute MUST be set to one of the 
>> versions that the entity in question supports, as per the 
>> <supportedVersions> list. I can, e.g., decide that I send an 
>> ‘options’ with “v=12.23” if the <supportedVersions> list is like in 
>> the following:
>>
>> <version>33.44</version>
>> <version>27.0</version>
>> <version>12.345</version>
>> <version>1.44</version>
>
> I think this formulation is a problem. I assume the intention behind 
> the "v" field is that some implementation that receives a version 
> number in the "v" field with a major number higher than it understands 
> is supposed to close the connection, since it runs a risk of 
> misinterpreting the contents of messages. So, in your example, if you 
> sent "v=12.23" to my implementation that only spoke major version 1, I 
> would reject it, even though you also support major version 1. That's 
> why I was thinking it should contain the smallest major version being 
> advertised.
>
> The minor version is obviously less useful in this context, since 
> they're defined to be backwards and forwards compatible, but it's more 
> useful to know the highest minor version supported than some random 
> minor version, as it indicates the full feature set that is supported. 
> The reason it's less useful is that the value can be parsed out of the 
> <version> list.
>
>>> BLOCKING: The state machines in section 6 and its subsections don't 
>>> have transitions for all possible messages that could arrive in a 
>>> state. This can cause interop issues. Please add text that clearly 
>>> indicates whether such messages do or do not cause a transition. 
>>> (This might be as simple as "messages not shown for a state do not 
>>> cause the state to change," but only if you carefully check that 
>>> this is true -- for example, what should an MP state machine do do 
>>> if it gets a "CONF + ACK" in the state "WAIT FOR CONF"?)
>>>
>> [SPR] In the specific case you mention, I would expect that a 302 is 
>> generated (Invalid Parameter), since the incoming CONF should not 
>> contain the <ack> element. This said, I see your point related to the 
>> potential lack of some edges in the state machine graph. This point 
>> is obviously also related to the “timeout” issue you raised before. 
>> Different ways of managing timeouts will definitely have different 
>> impacts on the current state machines.
>> We will defer the modification of the diagrams in question to the 
>> next revision round. In the meanwhile, we’ll give a thought to 
>> potential further unexpected transactions to be taken into account.
>
> Based on my analysis above, I think the state machines will get a bit 
> simpler, as they won't have retransmission counts any longer.
>
>>
>>> Section 6.2 describes a state machine that starts in a state called 
>>> "WAIT FOR ADV." This state does not appear to be timer-supervised, 
>>> meaning that implementations of this state machine can stay in this 
>>> state literally forever. Is that the intention?
>>>
>> [SPR] The idea would be to open a socket and start listening for 
>> incoming advertisements. Isn’t that OK?
>
> At some point, I think you're going to want to conclude that the CLUE 
> data channel isn't going to get set up correctly, and signal that to 
> the application in some way. This is probably on the order of a minute 
> or so.
>
>>> Section 8 indicates that extensions need to indicate "the standard 
>>> version of the protocol the extension refers to." Given that there 
>>> is compatibility within a major version of a protocol, I think this 
>>> means to say "the major standard version of the protocol that the 
>>> extension refers to.”
>>>
>> [SPR] My extension might be associated with version 3.6 of the 
>> protocol. This means that it will need to be supported by all 
>> versions higher than that (3.7 onwards). Though, this does not entail 
>> that versions 3.5 and lower should understand it. Right?
>
> I'm a bit confused. Is the notion here that extensions, once 
> introduced, always become a mandatory-to-implement part of the 
> protocol? If that's the case, it's not clear why you would need to 
> advertise them explicitly, since support would be implied by the 
> version number itself. (This isn't unheard of: it's how NFS has 
> historically handled version numbers, for example.) However, I didn't 
> get the impression that CLUE protocol extensions were handled in such 
> a way. If this is the intention, there needs to be a lot more text 
> that makes this MTI aspect of extensions clear, and a reconsideration 
> of why you're signaling them individually rather than inferring them 
> from the version number.
>
>>> Section 8 contains schema that contains a <version> element. It 
>>> should be clarified that this is the *protocol* version, not the 
>>> *extension* version (or, if it's the extension version, that needs 
>>> to be spelled out too, but I think you'll need a new namespace for 
>>> that...?)
>>>
>> [SPR] I thought this was clear from the third bullet point in the list:
>>
>> - the standard version of the protocol the extension refers to.
>>
>> Isn’t this enough? In such a case, what would you suggest to add?
>
> Ah, I see. It wasn't clear that these three bullets corresponded to 
> the elements in the XML. Perhaps make this clearer with something like 
> "The values of the fields of the <extension> element take the 
> following values:" (and maybe move it closer to the actual XML).
>
>>
>>> The remainder of this comment is non-blocking: I also find the 
>>> "SHOULD" in this section to be highly perplexing. Can you explain 
>>> the rationale behind requiring schema, but not requiring any 
>>> description of what the schema *means?)
>>>
>> [SPR] It is like coding without adding comments/documentation. 
>> Documentation is most welcome, but not mandatory. Don’t you agree?
>
> I don't. It seems more like committing a header file without the 
> corresponding implementation file. Just because people know what 
> things are called doesn't mean that they can handle them. For interop, 
> you're going to need some description of what the fields mean and how 
> the messages are handled.
>
>>>
>>> The schema in section 9 contains:
>>>
>>> <xs:import namespace="urn:ietf:params:xml:ns:clue-info"
>>> schemaLocation="data-model-schema-17.xsd"/>
>>>
>>> If you do not intend to bake this "-17" into the document (and I 
>>> can't imagine you do), please add an RFC editor note to change it to 
>>> something else upon publication.
>>>
>> [SPR] Done. I am not very familiar with RFC editor notes, indeed. 
>> Here is what we added at the beginning of section 9:
>>
>> "NOTE TO THE RFC-Editor: Please replace "data-model-schema-17.xsd" 
>> with the right schema location for the CLUE data model schema 
>> document (which is still to be defined at the time of this writing) 
>> in this section prior to publication as an RFC.”
>>
>> Does this work for you?
>
> That's perfect. Thanks!
>
>>> Section 10 in general: While it does consume a lot of space, I don't 
>>> think that defining six rather different message types and then 
>>> showing only *one* type in the examples is very illustrative of the 
>>> protocol. I would *STRONGLY* suggest adding at least a response 
>>> message, and ideally the example section should contain at least one 
>>> example for each of the six message types.
>>>
>> [SPR] Will do in the (hopefully final) revision of the document. 
>> Let’s first double check that the proposed modifications are OK.
>
> Sounds good.
>
>>
>>> Section 12.1 has a strange double-double quote around the URN name, 
>>> and section 12.3 repeats this for the MIME type.
>>>
>> [SPR] I don’t understand this. Can you please clarify?
>>
>
> Replace
>
>    ""urn:ietf:params:xml:ns:clue-protocol"".
>
> with
>
>    "urn:ietf:params:xml:ns:clue-protocol".
>
>
> And then replace
>
>    This section registers the ""application/clue+xml"" MIME type.
>
> with
>
>    This section registers the "application/clue+xml" MIME type.
>
>
> Thanks!
>
> /a