Subject: [Gen-art] Gen-art review of draft-ietf-idr-rfc3392bis-02.txt

From: Elwyn Davies <[email-masked]>
Date: Fri, 12 Dec 2008 19:24:13 +0000

I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
_http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html_).

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-idr-rfc3392bis-02.txt
Reviewer: Elwyn Davies
Review Date: 12 December 2008
IETF LC End Date: 25/12/2008
IESG Telechat date: (if known) -
 
Summary:
The document is almost ready for the IESG.  However after diligent 
searching both in the RFCs and on the IANA web site, I believe that 
neither the original BGP RFCs 1771/4671, RFC 3392 nor this draft define 
a registry for the parameter types in OPEN messages.  The CAPABILITY 
parameter is said to be 'Type 2'.  I presume this means the value of 
Parameter Type in the sense of s4.2 of RFC 4671. BTW: Is there a  'Type 
0' or 'Type 1'?

There is also one minor nit and a clarification which would help.
 
Comments:
IANA Considerations:  There does not appear to be a registry for OPEN 
message (optional) parameter types!

s4: I think it would be helpful to naive readers to make it clear that 
the capability (sub-)TLVs all go 'inside' the overall  CAPABILITY 
parameter TLV in the OPEN message, and the overall length in the 
Parameter TLV covers the whole set of capability options (and in turn 
the total length of all the optional parameters....  you could run out 
of space just counting the counters ;-) )

s4: The encodings of the Capability Type and Length fields are not 
specified.

_______________________________________________
Gen-art mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/gen-art

---

From: "John G. Scudder" <[email-masked]>
Date: Fri, 12 Dec 2008 16:04:53 -0500

Thanks for the review.  A few comments and replies in line below.

On Dec 12, 2008, at 2:24 PM, Elwyn Davies wrote:
> neither the original BGP RFCs 1771/4671, RFC 3392 nor this draft  
> define a registry for the parameter types in OPEN messages.  The  
> CAPABILITY parameter is said to be 'Type 2'.  I presume this means  
> the value of Parameter Type in the sense of s4.2 of RFC 4671. BTW:  
> Is there a  'Type 0' or 'Type 1'?
>
> IANA Considerations:  There does not appear to be a registry for  
> OPEN message (optional) parameter types!

Appendix A of RFC 4271 notes that "Optional Parameter Type 1  
(Authentication Information) has been deprecated."  There was never a  
type 0.

Regarding the lack of registry, that's a good observation but I don't  
think it's within the scope of rfc3392bis to remedy, it seems like an  
RFC 4271 issue.  Perhaps the IDR chairs and/or ADs can address this?   
(As a practical matter I'd be surprised if there were ever another  
OPEN optional parameter defined, but I guess it might be worth fixing  
anyway.)

> s4: I think it would be helpful to naive readers to make it clear  
> that the capability (sub-)TLVs all go 'inside' the overall   
> CAPABILITY parameter TLV in the OPEN message, and the overall length  
> in the Parameter TLV covers the whole set of capability options (and  
> in turn the total length of all the optional parameters....  you  
> could run out of space just counting the counters ;-) )
>

Regarding these two, I think both are already fully specified in RFC  
4271, and my own preference is generally to err on the side of less  
redundancy -- or on the side of terseness, to put it less  
charitably :-).

That said, I don't want a stylistic point to impede progress.  Below  
are two possible rewrites -- one minimal, the other more wordy.  As  
you might imagine, I prefer the minimal one, but I'd appreciate  
further feedback.

Minimal change:

Replace:

    This is an Optional Parameter that is used by a BGP speaker to  
convey
    to its BGP peer the list of capabilities supported by the speaker.

with:

    This is an Optional Parameter that is used by a BGP speaker to  
convey
    to its BGP peer the list of capabilities supported by the speaker.
    The encoding of BGP Optional Parameters is specified in Section 4.2
    of [RFC4271]. The parameter type of the Capabilities Optional
    Parameter is 2.

This (minimally :-) addresses the encoding comment.  I haven't made  
any changes to address the "go 'inside'" comment because it seems to  
me as though it's already clear from this existing text:

    The parameter contains one or more triples <Capability Code,
    Capability Length, Capability Value>, where each triple is encoded  
as
    shown below:

and RFC 4271 makes it clear what the overall length covers.  (I would  
think that by now, the concept of nested TLVs is workaday enough it  
needn't be explained every time it's used.)

More verbose change:

Replace the first part of section 4 with the following:

    This is an Optional Parameter that is used by a BGP speaker to  
convey
    to its BGP peer the list of capabilities supported by the speaker.

    The Optional Parameter encoding is specified in [RFC4271] as a
    <Parameter Type, Parameter Length, Parameter Value> triplet.  The
    parameter type for the Capabilities Optional Parameter is 2.  The
    parameter length is the length of the Capabilities Optional  
Parameter
    field in bytes; that is, the total of the lengths of all enclosed
    capabilities.  The encoding of the parameter value is given below.

    The Capabilities Optional Parameter contains one or more triples
    <Capability Code, Capability Length, Capability Value>, termed
    "capabilities". Each triple is encoded as shown below:

           +------------------------------+
           | Capability Code (1 octet)    |
           +------------------------------+
           | Capability Length (1 octet)  |
           +------------------------------+
           | Capability Value (variable)  |
           ~                              ~
           +------------------------------+

    The use and meaning of these fields are as follows:

       Capability Code:

          Capability Code is a one octet field that unambiguously
          identifies individual capabilities.

       Capability Length:

          Capability Length is a one octet field that contains the  
length
          of the Capability Value field in octets.

          (Note that this is different from the Parameter Length as
          defined in [RFC4271], which is discussed above, and also from
          the Optional Parameters Length, which as defined in [RFC4271]
          gives the length of all Optional Parameters, including but not
          limited to this one.)

       Capability Value:

          Capability Value is a variable length field that is  
interpreted
          according to the value of the Capability Code field.

(the rest of the section remains the same).  The "clarification" about  
length to me just seems to muddy the waters but I'm open to  
suggestions as to a better way to write it.

Regards,

--John

---

From: Elwyn Davies <[email-masked]>
Date: Sat, 13 Dec 2008 00:09:20 +0000

> Thanks for the review.  A few comments and replies in line below.
..and responses also.. Thanks for the fast turnaround.
>> the value of Parameter Type in the sense of s4.2 of RFC 4671. BTW: Is 
>> there a  'Type 0' or 'Type 1'?
>>
OK.  Missed that.
> OPEN optional parameter defined, but I guess it might be worth fixing 
I am inclined to agree with your final point here.. and for that reason 
I wouldn't worry too much if the registry was declared in this document 
(this would not be the first time an oversight had been cleared up in 
this sort of way) but as you say, this is a matter for the Chairs/ADs.
> redundancy -- or on the side of terseness, to put it less charitably :-).
>
>    This is an Optional Parameter that is used by a BGP speaker to convey
>    This is an Optional Parameter that is used by a BGP speaker to convey
I am fine with just the minimal solution to the clarification point.
> me as though it's already clear from this existing text:
I think the few extra words provide all the clues needed.

However I think you do need to say that the type and length are encoded 
as unsigned binary integers.  In fact RFC 4271 states this explicitly 
and individually for most of the numbers but it got missed out on the 
type and length fields of the OPEN optional parameter TLV (and AFAICS 
there isn't a global statement). IMO that is all that is needed extra.

The more verbose form is indeed overkill.

Regards,
Elwyn
>    Capability Length, Capability Value>, where each triple is encoded as
>    shown below:
>
>    This is an Optional Parameter that is used by a BGP speaker to convey
>    parameter length is the length of the Capabilities Optional Parameter
>          Capability Length is a one octet field that contains the length
>          Capability Value is a variable length field that is interpreted

---

From: "John G. Scudder" <[email-masked]>
Date: Fri, 12 Dec 2008 20:21:00 -0500

Very good.  I'll await the chairs'/ADs' comments regarding the  
registry.  Apart from that I think the below covers it.  Once we've  
settled the registry question I'll issue -03.

--John

Section 4., paragraph 1:
OLD:

NEW:

Section 4., paragraph 6:
OLD:

NEW:

           Capability Code is a one octet unsigned binary integer that
           unambiguously identifies individual capabilities.

Section 4., paragraph 8:
OLD:

NEW:

           Capability Length is a one octet unsigned binary integer that
           contains the length of the Capability Value field in octets.