Re: [Idr] AD Review of draft-ietf-idr-ext-opt-param-09

[John Scudder <[e-mail]>]

Hi Alvaro, thanks for the review. I’ve uploaded -10 to address your comments, see also my replies below.

—John

> On Mar 18, 2021, at 5:54 PM, Alvaro Retana <[e-mail]> wrote:
> 
> [External Email. Be cautious of content]
> 
> 
> Enke/John:
> 
> Hi!
> 
> Thank you for your persistence in getting this work done!
> 
> I have several comments inline below.  I believe that all of them
> should be easy to address.
> 
> Thanks!
> 
> Alvaro.
> 
> 
> 
> [Line numbers from idnits.]
> 
> ...
> 11      Abstract
> ...
> 19         In this document we update RFC 4271 by extending, in a backward-
> 20         compatible manner, the length of the Optional Parameters in the BGP
> 21         OPEN.  The Parameter Length field of individual Optional Parameters
> 22         is also extended.
> 
> [style nit] s/In this document we update/This document updates/g

Done.

> ...
> 72      1.1.  Requirements Language
> 
> 74         The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
> 75         "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
> 76         document are to be interpreted as described in RFC 2119 [RFC2119].
> 
> [major] Use the template from rfc8174.

Done.

> 78      2.  Protocol Extensions
> ...
> 83         In the event that the length of Optional Parameters in the BGP OPEN
> 84         message does not exceed 255, the encodings of the base BGP
> 85         specification [RFC4271] MUST be used without alteration.  However, an
> 86         implementation MUST be prepared to accept an OPEN message that uses
> 87         the encoding of this specification for Optional Parameters of any
> 88         length.
> 
> [major] "MUST be prepared to accept an OPEN message...of any length."
> 
> The OPEN message is not covered by rfc8654, so the length is still
> limited to 4k...which means that the full Extended OP length/Parameter
> Length cannot be used.

I’m inclined to leave this alone. I mean, we could go into that nuance, but then if some future specification extends the OPEN, do we have to go back and update this spec too? Better to leave it alone, I think. It is genuinely obvious, IMO, that you can’t encode more than the enclosing PDU format supports.

> Besides clarifying that, I think that new OPEN Message Error subcodes
> are needed for the cases where the length is invalid.  This type of
> error is possible in the non-extended version of the OPEN too -- I
> guess rfc4271 just assumed that this error would never happen.

RFC 4271 left a lot to be desired in terms of nuanced error codes. I agree that we could introduce a subcode called something like “optional parameter field length mismatch” to be sent if the optional parameters length exceeds the unconsumed OPEN length. However, I’m disinclined to do it in this document, because it’s a pre-existing problem, it exists with the non-extended encoding as well, so any fix for it should really fix both cases. As such, it seems a funny fit for this doc. 

> 90         If the length of Optional Parameters is greater than 255, the
> 91         extended encoding defined here MUST be used.  The (non-extended)
> 92         length field MUST be set to 255.  The subsequent octet (which would
> 93         be the first Optional Parameter Type in the non-extended format) MUST
> 94         be set to 255 as well.  The subsequent two octets carry the actual
> 95         length.  In addition, the "Parameter Length" field of each Optional
> 96         Parameter is enlarged to two octets.  Other than the larger sizes of
> 97         the given fields, there is no change to the BGP OPEN message defined
> 98         in [RFC4271].
> 
> [minor] This paragraph references the modified OPEN message, but it
> hasn't been introduced yet.  Please refer to the figures or move them
> (and the description of the fields) to the beginning of this section.
> Also, please be consistent with the field names from the figure.

On reviewing this, I think that paragraph and the following two paragraphs were redundant with the paragraphs that follow the figure. So, I simply removed them, and replaced them with this: 

   However, if the length of Optional Parameters in the BGP OPEN message 
   does exceed 255, the OPEN message MUST be encoded according to the 
   procedure below.

> ...
> 111            0                   1                   2                   3
> 112            0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> 113            +-+-+-+-+-+-+-+-+
> 114            |    Version    |
> 115            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 116            |     My Autonomous System      |
> 117            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 118            |           Hold Time           |
> 119            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 120            |                         BGP Identifier                        |
> 121            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 122            |Non-Ext OP Len.|Non-Ext OP Type|  Extended Opt. Parm. Length   |
> 123            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 124            |                                                               |
> 125            |             Optional Parameters (variable)                    |
> 126            |                                                               |
> 127            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> [] Please add a figure number and title.

Done.

> 129        The non-extended Optional Parameters Length field MUST be set to 255
> 130        on transmission, and MUST be ignored on receipt once the use of the
> 131        extended format is determined positively by inspection of the (non-
> 132        extended) Optional Parameters Type field.
> 
> [major] "non-extended Optional Parameters Length"  Please refer to the
> names of the fields in the figure.   s/.../non-extended Optional
> Parameters Length (Non-Ext OP Len.)

Done.

> ...
> 148            0                   1                   2
> 149            0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4
> 150            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 151            |  Parm. Type   |        Parameter Length       |
> 152            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 153            ~            Parameter Value (variable)         ~
> 154            |                                               |
> 155            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> [] Please add a figure number and title.

Done.

> ...
> 178        The choice to mandate that when the extended encoding is in use, the
> 179        (non-extended) Optional Parameters Length field must be 255 was made
> 180        for backward compatibility with implementations of earlier versions
> 181        of this specification.  When the extended encoding is in use, the
> 182        value 0 MUST NOT be used in that field since the presence of that
> 183        value could have the effect of causing a message parser to never
> 184        inspect the following octet.
> 
> [] I don't think it's a good idea to justify anything based on
> pre-standard implementations.  s/MUST be set to 255 on
> transmission/SHOULD be set to 255 on transmission  will cover both
> cases without the justification.

Now that you mention it, these two paragraphs read strangely to me now — it’s unnecessary historical baggage to be going on about why certain choices were made. I’ve removed them and rewritten the first paragraph following the figure as:

  The non-extended Optional Parameters Length field (Non-Ext OP Len)
  SHOULD be set to 255 on transmission and in any event MUST NOT be set to
  0, and MUST be ignored on receipt once the use of the extended format is
  determined positively by inspection of the Non-Extended Optional
  Parameters Type (Non-Ext OP Type) field.

since the only normative text in those paragraphs was the MUST NOT part. 

If anyone feels the paragraphs should be retained for some reason, we could move them to an appendix. But I think students of history can rely on looking at the draft versions and it’s OK to just leave it out altogether.

> [major] "0 MUST NOT be used"  Move this part to where the field is described.

Done. (See language above.)

> 186     3.  Errors
> 
> 188        If a BGP speaker supporting this specification (a "new speaker") is
> 189        peering with one which does not (an "old speaker") no
> 190        interoperability issues arise unless the new speaker needs to encode
> 191        Optional Parameters whose length exceeds 255.  In that case, it will
> 192        transmit an OPEN message which the old speaker will interpret as
> 193        containing an Optional Parameter with type code 255.  Since by
> 194        definition the old speaker will not recognize that type code, the old
> 195        speaker may be expected to close the connection with a NOTIFICATION
> 196        with an Error Code of OPEN Message Error and an Error Subcode of
> 197        Unsupported Optional Parameters, according to Section 6.2 of
> 198        [RFC4271].
> 
> [minor] s/may be expected/is expected

OK.

> 200        Although the above is the most likely error to be sent, it is not
> 201        impossible that the old speaker might detect some other error first,
> 202        such as a length error, depending on the details of the
> 203        implementation.  In no case would the peering be expected to
> 204        establish successfully; the only question is which NOTIFICATION would
> 205        be generated.
> 
> [] No need to speculate about implementations.  The text above points
> at rfc4271 anyway.

OK. I cut that paragraph. It’s similar to the other two I cut, discussed above.

> 207        We note that in any case, such a peering could not succeed, since by
> 208        definition the extended length encoding would not be used by the new
> 209        speaker unless the basic encoding was insufficient.
> 
> [] I don't understand what you mean here.  It sounds as if the new
> speaker shouldn't have used the encoding anyway...  ??

I could explain, but instead I’ve just cut that paragraph as well.

> ...
> 219        It is not considered an error to receive an OPEN message whose
> 220        Extended Optional Parameters Length value is less than or equal to
> 221        255, any value SHOULD be silently accepted.  It is not considered a
> 222        fatal error to receive an OPEN message whose (non-extended) Optional
> 223        Parameters Length value is not 255, and whose first Optional
> 224        Parameter type code is 255 -- in this case the encoding of this
> 225        specification MUST be used for decoding the message.  A warning MAY
> 226        be logged.
> 
> [major] "any value SHOULD be silently accepted"  When is it ok to not
> (silently) accept the message?  IOW, why is this recommended and not
> required?  In any case, I don't think that the last part of the
> sentence is needed because the first part already says this is not an
> error.

OK, I cut "any value SHOULD be silently accepted”. (And you’re right, it should have been a MUST.)

> [End of Review]