Re: Review of draft-ietf-nsis-qspec-18.txt

Hi Jerry,

Thanks for getting back this. I hope you're doing fine in retirement.

I have just a few things below. How fast can you post an updated version?


> 
> > Hi all,
> >
> > Please find my review of draft-ietf-nsis-qspec-18.txt.
> >
> > There are some minor technical issues to fix. In general 
> the document 
> > seems to be ready to go for AD review,  once the editorials and 
> > technical issues have been fixed.
> >
> > Please address the issues and submit an updated version of 
> the draft 
> > if d one so. I will go ahead with the updated draft for the 
> protocol 
> > write up (i.e., the step before sending to Magnus and the IESG).
> >
> >  Martin
> >
> > ****
> > General
> > ****
> > I found the first part of the document (i.e., until section 
> 5) quite 
> > confusing and after section five quite clear. This does not 
> mean that 
> > there is an issue, as typically Internet drafts have the need to 
> > sketch tech things in the beginning without having the 
> reader already 
> > at the technical level. I assume that the text is fine 
> right now, as 
> > the WG has not pushed to change it.
> 
> These sections have been rewritten many times.  Please let us 
> know if you have any particular suggestion to improve clarity.

I can't say how to improve it. As said, if the WG is otherwise fine with it, we just leave it as is.



> 
> >
> > ****
> > Editorial:
> > ****
> >
> > - the idnits tool has found some issues (see below)
> 
> OK, will address.
> 
> > - Add line in memo heading with: Intended Status: Informational
> 
> Although the NSIS milestone says "Informational", the ID 
> tracker lists the status as "Proposed Standard"
> https://datatracker.ietf.org/idtracker/draft-ietf-nsis-qspec/ 
> <mhtml:{B096A9AC-8D5F-4D5A-AACE-EC56A1B5E65A}mid://00000038/!x
> -usc:https://datatracker.ietf.org/idtracker/draft-ietf-nsis-qs
> pec/> .  IMO the QSPEC document should be a PS document, in 
> close alignment with the NSLP PS document.  Many IANA 
> registries are created, etc.  I can't see the rationale for 
> NSLP being PS and QSPEC being Informational.  I do recall a 
> discussion on this point earlier and several WG members 
> expressed the same view.  Perhaps the WG members and chairs 
> can comment.

I remember to have discussed this with the ADs and agreement was that the QSPEC is informational. The charter has the final say.



> 
> > -  Section 1: Move to the very end of memo. It it is good to 
> > acknowledge other people, but it is fair enough to do it at 
> the very 
> > end, as technical protocol stuff comes first.
> 
> OK.
> 
> > - section 2: Adding a line saying that QSPEC readers should 
> (or better
> > must?) read the QoS NSLP in advance would be advantageous for the 
> > whole document.
> 
> Note that the first sentence of Section 2 references QoS NSLP 
> since the documents are closely tied.  The QSPEC document is 
> intended to be (and should be) self contained so that one 
> doesn't have to ("must") read something else first before 
> reading and understanding QSPEC.  However, your point is well 
> taken.  We can add a sentence in Section 2 such as "See 
> [QoS-SIG] for further details on QoS NSLP."

Adding the sentence would be great!

> 
> > - section 2, page 5, para starting with "Interoperability 
> between QoS 
> > NSIS entities (QNEs)..." says that parameters can be flagged or not 
> > flagged.
> > I
> > suggest to put the terms "flagged"/"non flagged" in 
> brackets and write 
> > mandatory and optional parameters. This makes it easier for 
> first time 
> > readers.
> 
> We've eliminated the terminology "mandatory" and "optional" 
> parameters.
> Parameters can be flagged by the M flag if they MUST be 
> interpreted, or otherwise, as follows:
> 
>  M Flag: When set indicates the subsequent parameter MUST be
>            interpreted. Otherwise the parameter can be ignored if not
>            understood.
> 
> We don't want to get back into the mandatory/optional 
> terminology at this point.

Ok, fine with me.

> 
> > - - section 2, page 5, para starting with " A local QSPEC can be 
> > defined in a local domain with the initiator QSPEC 
> encapsulated, ...". 
> > This paragraph is brief but describes a very powerful 
> mechanism of the 
> > QSPEC. I would suggest to remove the hint the TMOD 
> parameter and just 
> > write that the QSPEC allows local QPSECs that are domain specific. 
> > This also helps first time readers to understand a basic 
> feature of  
> > QPSEC but not the full implementation of it.
> 
> OK.
> 
> > - section 4.1, 2nd para, text in para starting with " New QoS NSLP 
> > message processing rules can only be defined in Standard...". Is it 
> > required to say this at this part of the document at all? I 
> remember 
> > this again stated in the document somewhere later on at an 
> appropriate 
> > place.
> 
> It's an important point and I think it appropriate to mention 
> it in Section 4.1.  Furthermore, I recall extensive 
> discussion on this sentence and I believe it was explicitly 
> agreed to include it here as well as later in the document.  
> Also, mentioning important points more than once often helps clarity.

Ok, fine with me.

> 
> > - section 4.2 two paras below figure 1. Suddenly there is 
> text about 
> > RESERVE and QUERY, but no hint to the QoS NSLP at all. Adding a 
> > reference to the QoS NSLP at this occasion would be excellent. 
> > Otherwise it  is hard to see where those terms are coming from
> 
> Good point, will do.
> 
> > - section 4.3.1, para starting with " When the TMOD parameter in 
> > included in QoS Available, it..." should read " When the TMOD 
> > parameter are included in QoS Available, it..."
> 
> I think the English is correct as it stands.  However, we 
> could say "When the TMOD sub-parameters are included ..."

I would prefer your suggestion, as this improves readability for non-native speakers (as I'm).

> 
> > - section 4.3.1. This section nevers talks about tmod-2! tmod-2 is 
> > mentioned only  in sections 6.2.1 and 6.2.2.
> 
> Right, good point.  We'll revise the text to refer to TMOD-1 
> and TMOD-2 and explain the difference.  We could include the 
> example of the need for the
> TMOD-2 parameter in Section 4.3.1 (see discussion/example 
> below of the need for the TMOD-2 parameter).
> 
> > - section 5, 1st and 2nd para suddenly use the E-/N-flag 
> without every 
> > explaining their meaning.
> 
> Well, this is where the M flag, E flag, and N flag are being 
> explained.  The first paragraph introduces them and there are 
> details later.  We could add text in the beginning such as 
> "Three flags are used in QSPEC, the M flag, E flag, and N 
> flag, which are explained in this section."

Please add this sentence, it also improves readability.

> 
> > - section 5.1, again the support of local QSPECs is quite 
> powerful but 
> > never really good explained with a figure. For instance, the first 
> > para of section
> > 5.1 could refer to a simple figure showing a network 
> configuration for 
> > a single flow, but with indications where which QSPEC is used. This 
> > instantly makes the whole concept better undestandable.
> 
> OK, we could add a figure analogous to Figure 3 in the 
> Y.1541-QOSM document 
> http://www.ietf.org/internet-drafts/draft-ietf-nsis-y1541-qosm
> -05.txt 
> <mhtml:{B096A9AC-8D5F-4D5A-AACE-EC56A1B5E65A}mid://00000038/!x
> -usc:http://www.ietf.org/internet-drafts/draft-ietf-nsis-y1541
> -qosm-05.txt>  to explain the concept.

This would be great!

> 
> > - section 5.2.5. formatting error
> 
> OK.
> 
> > - section 5.3.1 and related. You use tables to define cases 
> and also 
> > refer to them later in the object definition. Thus it would 
> be good to 
> > label those tables.
> 
> OK, good suggestion.
> 
> > - section 5.3.5 has one formatting issue and one spelling error 
> > (s/described In [QoS-SIG]/ described in [QoS-SIG])
> 
> OK.
> 
> > - section 5.4. remove reference to NSIS extensibility 
> document, as we 
> > do not have it ready by now.
> 
> OK.
> 
> > - section 6.1. I got confused by all the different header 
> definition. 
> > I would propose to split 6.1. in subsections, i.e., one for each 
> > header definition. This improves readability.
> 
> OK, good suggestion.
> 
> > - section 6.2.1: Why is there this line    <TMOD-1> = <r> 
> <b> <p> <m>
> > [RFC2210, RFC2215]? It is now required for the object definition.
> 
> Right, the format is inconsistent with the other subsections 
> and will be revised accordingly.
> 
> > - section 8, text says " Object Types (12 bits):" but should read 
> > "QSPEC Object Types (12 bits):".
> 
> But then Section 6.1 would have to be revised to be 
> consistent in terminology.  While I have no big objection to 
> changing all of this, IMO it should stay as is; I think it is 
> clear that this is a QSPEC object type.

Ok.

> 
> > - appendix b: add a note in the first line that this 
> appendix should 
> > be removed by the RFC editor before publication.
> 
> OK.
> 
> >
> >
> >
> > ***
> > Technical
> > ***
> > - section 5.3.1, para starting with " The QSPEC parameter IDs and 
> > values included in the QoS Reserved...". This paragraph talks about 
> > how to set the parameters in the RESPONSE message for case 2. It is 
> > basically fine, but the second sentence is for me not 
> understandable 
> > (" For those QSPEC parameters that were also included in the QoS 
> > Available object in the RESERVE message, their value is copied into 
> > the QoS Desired object.") The paragraph is talking about responses, 
> > but in RESPONSE there is no QoS desired object anymore (at least 
> > according to the table above). I probably do misunderstand 
> something 
> > here?
> 
> Good catch.  The sentence needs to be corrected.  It should 
> be: "For those QSPEC parameters that were also included in 
> the QoS Available object in the RESERVE message, their value 
> is copied from the QoS Available object (in
> RESERVE) into the QoS Reserved object (in RESPONSE) ."
> 
> > - section 6.1 qspec header element definition. The length 
> definition 
> > in the qspec header definition contradicts the length definition at 
> > the beginning of section 6.1 One says "total length of qspec" the 
> > other "length is always exept header"
> 
> Very good catch!  IMO the section definition should be revised as
> follows:
> 
>  Length: The total length of the QSPEC -->
>  Length: The total length of the QSPEC excluding the common header

OK.

> 
> > - section 6.2.2., see also my comment above about tmod-2. 
> Even after 
> > reading section 6.2.2. I do not have any clue why there two tmod 
> > parameters.
> 
> This is based on discussions with David Black, who 
> recommended the second TMOD parameter to support DiffServ 
> applications (as noted in Section 6.2.2).
> Earlier versions of QSPEC included the following example (due 
> to David), which can be added back into the draft (probably 
> in Section 4.3.1):
> 
> "It is typically assumed that DiffServ EF traffic is shaped 
> at the ingress by a single rate token bucket.  Therefore, a 
> single TMOD parameter is sufficient to signal DiffServ EF 
> traffic.  However, for DiffServ AF traffic two sets of token 
> bucket parameters are needed, one token bucket for the 
> average traffic and one token bucket for the burst traffic.  
> [RFC2697] defines a Single Rate Three Color Marker (srTCM), 
> which meters a traffic stream and marks its packets according 
> to three traffic parameters, Committed Information Rate 
> (CIR), Committed Burst Size (CBS), and Excess Burst Size 
> (EBS), to be either green, yellow, or red.  A packet is 
> marked green if it does not exceed the CBS, yellow if it does 
> exceed the CBS, but not the EBS, and red otherwise.  
> [RFC2697] defines specific procedures using two token buckets 
> that run at the same rate.  Therefore 2 TMOD parameters are 
> sufficient to distinguish among 3 levels of drop precedence.  
> An example is also described in the Appendix to [RFC2597]."

This would be good!

> 
> Also, the first sentence in Section 6.2.2 should be changed 
> slightly to: ''A second, QSPEC <TMOD-2> parameter is 
> specified, as could be needed for example to support some 
> Diffserv applications." (added word "some").

Ok.

> 
> > - section 6.2.7. The slack term is 32 bits as integer. However the 
> > slack term must only be non-negative, i.e., it is actually 
> an unsigned 
> > int.
> > Why is
> > is not specified as unsigned int? However, the current value range 
> > definition is broken, as an integer of 32 bits cannot have 
> a maximum 
> > range to  2**32-1 (there are only 31 bits left...).
> 
> The coding of the slack term is based on RSVP coding:
> 
> RFC 2210 says:
> 
>       
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 11  |  Slack Term [S]  (32-bit integer)                             |
>        
> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> RFC 2212 says:
> "The Slack term, S, can be represented as a 32-bit integer.  Its value
>    can range from 0 to (2**32)-1 microseconds."
> 
> I agree it should be specified as an unsigned integer, and 
> the notation revised to (2**32)-1 as the maximum, to be 
> clear.  I believe the range 0 to
> (2**32)-1 is correct for an unsigned integer.

Right.

> 
> > This also holds true for
> > other
> > occasions where integer values are used (e.g. section 6.2.3!
> 
> Same comments as above.
> 
> > - section 6.2.11. para starting with " When excess 
> treatment is set to 
> > 'shape', ". What is the error handling if excess treatment 
> is set to 
> > 'shape'
> > and no TMOD parameter is given?
> 
> The E flag is set for the parameter and the reservation 
> fails.  This can be specified in Section 6.2.11.

Fine.

> 
> > - section 7. This section is extremely short. This might a 
> non-issue, 
> > but I personally had the feeling the security discussion is 
> too brief. 
> > This might be OK, but the document authors and the WG should be 
> > prepared for tough questions regarding this when going to the IESG. 
> > What about this document defining just the basics, but 
> other documents 
> > defining the real behaviour?
> > Is it assumed that this does not raise any new security issue?
> 
> Good points and questions.  QSPEC security is directly tied 
> to QoS NSLP security, and the QoS NSLP document has a very 
> detailed security section.
> All these considerations apply to QSPEC and that should be 
> noted at the beginning of Section 7.  I'm not aware of any 
> new security issues introduced by QSPEC beyond the point made 
> already in Section 7 on priority.
> Suggestions welcome from the WG on other security issues that 
> might arise from QSPEC.

I'm fine with leaving the section as short as it is. However, adding a reference to the QOS NSLp would be beneficial.

> 
> > - section 8 says in several places " A specification is required to 
> > depreciate, delete, or modify QSPEC versions." What happens 
> to deleted 
> > IANA values? Is IANA allowed to re-assign those or should they be 
> > treated as deprecated values, i.e., they cannot be used 
> anymore, but 
> > the value is blocked for ever?
> 
> These are IANA specific rules that I don't think need to be 
> reviewed in Section 8.

Ok, I'm just noting now to have misread this sentence, as a result of reading too much at the same time.

Thanks,

  Martin


[email protected]   <== NEW ADDRESS

NEC Laboratories Europe - Network Research Division

NEC Europe Limited | Registered Office: NEC House, 1 Victoria Road, London W3 6BL | Registered in England 2832014