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

[Alvaro Retana <[e-mail]>]

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


...
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.


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.

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.


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.


...
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.


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.)


...
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.


...
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.


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


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


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.


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...  ??


...
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.


[End of Review]