[Idr] AD Review of draft-ietf-idr-flow-spec-v6-14

[Alvaro Retana <[e-mail]>]

Dear authors:

Thank you for the work on this document.  I have moved it up in the
queue to fulfill a promise I made to the IESG while processing
rfc5575bis; namely that I would process the IPv6 work as soon as
possible to try and get the two documents published together.


Please take a look at my comments on this document (in line below); I
believe that most of them should be easy to address.  I have a couple
of major items that I would like to see addressed before starting the
IETF Last Call.

Thanks!

Alvaro.



[Line numbers from idnits.]

...
13	Abstract

15	   Dissemination of Flow Specification Rules I-D.ietf-idr-rfc5575bis
16	   provides a protocol extension for propagation of traffic flow
17	   information for the purpose of rate limiting or filtering.  I-D.ietf-
18	   idr-rfc5575bis specifies those extensions for IPv4 protocol data
19	   packets only.

[nit] s/for propagation of/for the propagation of


[minor] The Abstract shouldn't contain references; remove the first
mention of I-D.ietf-idr-rfc5575bis.


21	   This specification extends I-D.ietf-idr-rfc5575bis and defines
22	   changes to the original document in order to make it also usable and
23	   applicable to IPv6 data packets.

[minor] "extends...defines changes" sounds as if rfc5575bis is being
formally updated.

Suggestion>
   This specification extends I-D.ietf-idr-rfc5575bis with IPv6 functionality.


...
85	1.  Introduction

87	   The growing amount of IPv6 traffic in private and public networks
88	   requires the extension of tools used in the IPv4 only networks to be
89	   also capable of supporting IPv6 data packets.

[nit] s/in the IPv4 only networks/in IPv4-only networks


91	   In this document authors analyze the differences of IPv6 [RFC8200]
92	   flows description from those of traditional IPv4 packets and propose
93	   subset of new encoding formats to enable Dissemination of Flow
94	   Specification Rules [I-D.ietf-idr-rfc5575bis] for IPv6.

[nit] s/In this document authors analyze/This document analyzes

[nit] s/propose subset/propose a subset


96	   This specification should be treated as an extension of base
97	   [I-D.ietf-idr-rfc5575bis] specification and not its replacement.  It
98	   only defines the delta changes required to support IPv6 while all
99	   other definitions and operation mechanisms of Dissemination of Flow
100	   Specification Rules will remain in the main specification and will
101	   not be repeated here.

[nit] s/of base/of the base


[minor] Suggestion>
   This specification is an extension of base [I-D.ietf-idr-rfc5575bis].  It
   only defines the delta changes required to support IPv6 while all other
   definitions and operation mechanisms of Dissemination of Flow Specification
   Rules will remain in the main specification and will not be repeated here.


...
121	2.  IPv6 Flow Specification encoding in BGP

123	   The [I-D.ietf-idr-rfc5575bis] defines new SAFIs 133 (Dissemination of
124	   Flow Specification) and 134 (L3VPN Dissemination of Flow
125	   Specification) applications in order to carry corresponding to each
126	   such application Flow Specification.

[nit] s/The [I-D.ietf-idr-rfc5575bis]/[I-D.ietf-idr-rfc5575bis]


[nit] s/defines new SAFIs/defines SAFIs


[minor] s/applications in order to carry corresponding to each such
application Flow Specification./in order to carry the corresponding
Flow Specifications.


128	   Implementations wishing to exchange IPv6 Flow Specifications MUST use
129	   BGP's Capability Advertisement facility to exchange the Multiprotocol
130	   Extension Capability Code (Code 1) as defined in [RFC4760].  While
131	   [I-D.ietf-idr-rfc5575bis] specifies Flow Specification for IPv4
132	   (AFI=1) only, the (AFI, SAFI) pair carried in the Multiprotocol
133	   Extension Capability MUST be: (AFI=2, SAFI=133) for IPv6 Flow
134	   Specification, and (AFI=2, SAFI=134) for VPNv6 Flow Specification.

[minor] s/While [I-D.ietf-idr-rfc5575bis] specifies Flow Specification
for IPv4 (AFI=1) only, the/The


136	   For both SAFIs the indication to which address family they are
137	   referring to will be recognized by AFI value (AFI=1 for IPv4 or
138	   VPNv4, AFI=2 for IPv6 and VPNv6 respectively).

[nit] s/IPv6 and VPNv6 respectively/IPv6 and VPNv6, respectively


140	   It needs to be observed that such choice of proposed encoding is
141	   compatible with filter validation against routing reachability
142	   information as described in Section 6 of [I-D.ietf-idr-rfc5575bis].

[nit] I don't think these last two paragraphs are necessary.


144	3.  IPv6 Flow Specification components

146	   The following components are redefined or added for the purpose of
147	   accommodating the IPv6 header encoding.  Unless otherwise specified
148	   all other components defined in [I-D.ietf-idr-rfc5575bis]
149	   Section 4.2.2 also apply to IPv6 Flow Specification.

[major] "The following components are redefined or added...all other
components...also apply to IPv6"   This document is really defining a
whole new set of components, where some of the definitions coincide
with rfc5575bis.

[Note that the new registry is what results in a new set of
components.  See my comments about that later in the IANA section.]

Suggestion>

   The encoding of each of the components begins with a type field (1 octet)
   followed by a variable length parameter.  The following sections define
   component types and parameter encodings for IPv6.

   Types 4, 5, 6, 9, 10 and 11, as defined in [I-D.ietf-idr-rfc5575bis],
   also apply to IPv6.  Note that even if the definitions are the same (and
   not repeated here), the number space is managed separately (Section 8).


151	3.1.  Type 1 - Destination IPv6 Prefix
...
156	   Defines the destination prefix to match.  The offset has been defined
157	   to allow for flexible matching on part of the IPv6 address where it
158	   is required to skip (don't care) of N first bits of the address.
159	   This can be especially useful where part of the IPv6 address consists
160	   of an embedded IPv4 address and matching needs to happen only on the
161	   embedded IPv4 address.  The encoded prefix contains enough octets for
162	   the bits used in matching (length minus offset bits).

[nit] s/on part of/on the part of


[major] Maybe it's just me, but I find the description confusing.
After reading it several times, and looking at the examples, I think
that the length indicates the "length, in bits, of the address prefix"
(quoting rfc4760).  But the length of the prefix field (which is
different than the address prefix) is the "length minus offset bits"
(not related to rfc4760).

[I am mentioning rfc4760 because in rfc5575bis the reference to
rfc4271 was used (and very convenient).]

Please explicitly indicate what each field means.  I'm thinking of
something like this:

   the length field indicates the length, in bits of the address prefix

   the offset field indicates the N first bits that are ignored/skipped/don't
   carefully

   the prefix field includes the address prefix, it's length can be
derived from...

[Using paragraphs instead of bullets is ok of course, to keep with the
style of the document.]


[major] Looking at the examples, it is not obvious to me how the
encoding is done.  Both examples use 40 bits to encode the prefix
field...but the 'length-offset' operation is 39 in the second example.
The use of trailing bits is not mentioned -- see the definition in
rfc4760.


164	3.2.  Type 2 - Source IPv6 Prefix
...
169	   Defines the source prefix to match.  The length, offset and prefix
170	   are the same as in Section 3.1

[minor] s/The length, offset and prefix are the same as in Section
3.1/The definition of the length, offset and prefix field is the same
as in Section 3.1.


172	3.3.  Type 3 - Next Header
...
176	   Contains a list of {numeric_op, value} pairs that are used to match
177	   the last Next Header ([RFC8200] Section 3) value octet in IPv6
178	   packets.

[major] "last Next Header ([RFC8200] Section 3)"

I see Next Header, but nothing in rfc8200 about the "last" one.

§4 of rfc8200 says this:

   When processing a sequence of Next Header values in a packet, the first
   one that is not an extension header [IANA-EH] indicates that the next
   item in the packet is the corresponding upper-layer header.

Is the "last Next Header" the same as "the first one that is not an
extension header"?  If so, then I recommend using the same terminology
as rfc8200.

It might be a good idea to add an example using this component.


180	   This component uses the Numeric Operator (numeric_op) described in
181	   [I-D.ietf-idr-rfc5575bis] Section 4.2.1.1.  Type 3 component values
182	   SHOULD be encoded as single byte (numeric_op len=00).

[nit] s/single byte/single octet
To match the text in rfc5575bis.


184	   Note: While IPv6 allows for more then one Next Header field in the
185	   packet the main goal of Type 3 flow specification component is to
186	   match on the subsequent IP protocol value.  Therefore the definition
187	   is limited to match only on last Next Header field in the packet.

[nit] s/more then one/more than one

[nit] s/in the packet/in the packet,

[nit] s/of Type 3/of the Type 3

[nit] s/on last/on the last

[] See my related comment above.  This paragraph talks about "the
subsequent IP protocol value", while rfc8200 uses "upper-layer".


189	3.4.  Type 7 - ICMPv6 type
...
193	   Defines a list of {numeric_op, value} pairs used to match the type
194	   field of an ICMPv6 packet (see also [RFC4443] Section 2.1).

[nit] s/type/Type


196	   This component uses the Numeric Operator (numeric_op) described in
197	   [I-D.ietf-idr-rfc5575bis] Section 4.2.1.1.  Type 7 component values
198	   SHOULD be encoded as single byte (numeric_op len=00).

[nit] s/single byte/single octet
To match the text in rfc5575bis.


200	   In case of the presence of the ICMPv6 type component only ICMPv6
201	   packets can match the entire Flow Specification.  The ICMPv6 type
202	   component, if present, never matches when the packet's last Next
203	   Header field value is not 58 (ICMPv6), if the packet is fragmented
204	   and this is not the first fragment, or if the system is unable to
205	   locate the transport header.  Different implementations may or may
206	   not be able to decode the transport header.

[] rfc5575bis qualified this last sentence:

   Different implementations may or may not be able to decode the
   transport header in the presence of IP options or Encapsulating
   Security Payload (ESP) NULL [RFC4303] encryption.

Is something similar appropriate here?


208	3.5.  Type 8 - ICMPv6 code
...
215	   This component uses the Numeric Operator (numeric_op) described in
216	   [I-D.ietf-idr-rfc5575bis] Section 4.2.1.1.  Type 8 component values
217	   SHOULD be encoded as single byte (numeric_op len=00).

[nit] s/single byte/single octet
To match the text in rfc5575bis.


219	   In case of the presence of the ICMPv6 code component only ICMPv6
220	   packets can match the entire Flow Specification.  The ICMPv6 code
221	   component, if present, never matches when the packet's last Next
222	   Header field value is not 58 (ICMPv6), if the packet is fragmented
223	   and this is not the first fragment, or if the system is unable to
224	   locate the transport header.  Different implementations may or may
225	   not be able to decode the transport header.

[] Is a qualification needed?


227	3.6.  Type 12 - Fragment
...
234	   This component uses the Bitmask Operator (bitmask_op) described in
235	   [I-D.ietf-idr-rfc5575bis] Section 4.2.1.2.  The Type 12 component
236	   bitmask MUST be encoded as single byte bitmask (bitmask_op len=00).

[nit] s/single byte/single octet
To match the text in rfc5575bis.


...
273	3.8.1.  Example 1

275	   The following example demonstrates the prefix encoding for: "all
276	   packets to ::1234:5678:9A00:0/64-104 from 100::/8 and port 25".

[major] Please use IPv6 addresses from the documentation pool
(rfc3849): 2001:DB8::/32

IOW, use 2001:DB8::/32 instead of 100::/8


...
286	       +-------+------------+------------------------------+
287	       | Value |            |                              |
288	       +-------+------------+------------------------------+
289	       |  0x0f | length     | 16 octets (len<240 1-octet)  |

[] s/16/15

290	       |  0x01 | type       | Type 1 - Dest. IPv6 Prefix   |
291	       |  0x68 | length     | 104 bit                      |
292	       |  0x40 | offset     | 64 bit                       |
293	       |  0x12 | prefix     |                              |
294	       |  0x34 | prefix     |                              |
295	       |  0x56 | prefix     |                              |
296	       |  0x78 | prefix     |                              |
297	       |  0x9A | prefix     |                              |
298	       |  0x02 | type       | Type 2 - Source IPv6 Prefix  |
299	       |  0x08 | length     | 8 bit                        |
300	       |  0x00 | offset     | 0 bit                        |
301	       |  0x01 | prefix     |                              |

[] s/0x01/0x64

302	       |  0x04 | type       | Type 4 - Port                |
303	       |  0x81 | numeric_op | end-of-list, value size=1, = |

[minor] s/=/==
That's the notation used in rfc5575bis.

304	       |  0x19 | value      | 25                           |
305	       +-------+------------+------------------------------+

307	   This constitutes a NLRI with a NLRI length of 16 octets.

[] s/16/15


...
338	4.  Ordering of Flow Specifications

340	   The definition for the order of traffic filtering rules from
341	   [I-D.ietf-idr-rfc5575bis] Section 5.1 can be reused with new
342	   consideration for the IPv6 prefix offset.  As long as the offsets are
343	   equal, the comparison is the same, retaining longest-prefix-match
344	   semantics.  If the offsets are not equal, the lowest offset has
345	   precedence, as this flow matches the most significant bit.

[minor] s/can be reused/is reused


...
352	5.  Validation Procedure

354	   The validation procedure is the same as specified in
355	   [I-D.ietf-idr-rfc5575bis] Section 6 with the exception that item a)
356	   of the validation procedure should now read as follows:

358	      a) A destination prefix component with offset=0 is embedded in the
359	      Flow Specification

[major] Why is the FS considered valid only if offset=0 is used?
Given that each component can exist only once, then there seems to be
no point in using the offset in the destination component.  Also, the
examples above are then unfeasible because neither uses offset=0.  Am
I missing something?


361	6.  IPv6 Traffic Filtering Action changes

363	   Traffic Filtering Actions from [I-D.ietf-idr-rfc5575bis] Section 7
364	   can also be applied to IPv6 Flow Specifications.  To allow an IPv6
365	   address specific route-target, a new Traffic Filtering Action IPv6
366	   address specific extended community is specified in Section 6.1
367	   below:

[nit] s/below:/below.


369	6.1.  Redirect IPv6 (rt-redirect-ipv6) Type/Sub-Type 0x80/TBD
...
383	   Interferes with: All BGP Flow Specification redirect Traffic
384	   Filtering Actions (with itself and those specified in
385	   [I-D.ietf-idr-rfc5575bis] Section 7.4).

[major] Ok, so this community conflicts with other redirect Traffic
Filtering Actions, including itself (?)...but the actions in
rfc5575bis/§7.4 don't conflict with anything, apparently including
themselves.  Hmmm..   Maybe we had the discussion before (I don't
remember) about conflicting with themselves -- the text in rfc5575bis
may be saying the same thing as the text here, but with different
words.

IOW, is 'doesn't conflict with anything *else*' the same as 'only
conflicts with itself'?

Let's please use consistent language.

Suggestion (assuming I'm understanding)>
   Interferes with: All BGP Flow Specification redirect Traffic Filtering
   Actions (specified in [I-D.ietf-idr-rfc5575bis] Section 7.4).



387	7.  Security Considerations

389	   No new security issues are introduced to the BGP protocol by this
390	   specification over the security considerations in
391	   [I-D.ietf-idr-rfc5575bis]

[major] This section is not complete:  The fact that the functionality
from rfc5575bis is being extended to IPv6 means that the security
issues are now also present for IPv6.  Yes, the issues may be
equivalent, but they didn't exist before.

Suggestion>
   This document extends the functionality in rfc5575bis to be applicable to
   IPv6 data packets.  The same Security Considerations from rfc5575bis now
   also apply to IPv6 networks.  Otherwise, no new security issues are added
   to the BGP protocol.



393	8.  IANA Considerations

395	   This section complies with [RFC7153]

[nit] s/[RFC7153]/[RFC7153].

[] Please add subsections to separate the 2 different actions below.


397	   IANA is requested to create and maintain a new registry entitled:
398	   "Flow Spec IPv6 Component Types" containing the initial entries as
399	   specified in Table 1.

[major] Where should this registry be located?  Right now there is a
dedicated "Flow Spec Component Types" section -- I think that's the
appropriate place for this new registry.


401	           +-------+-------------------------+-----------------+
402	           | Value | Name                    | Reference       |
403	           +-------+-------------------------+-----------------+
404	           | 1     | Destination IPv6 Prefix | [this document] |
405	           | 2     | Source IPv6 Prefix      | [this document] |
406	           | 3     | Next Header             | [this document] |
407	           | 4     | Port                    | [this document] |
408	           | 5     | Destination port        | [this document] |
409	           | 6     | Source port             | [this document] |
410	           | 7     | ICMPv6 type             | [this document] |
411	           | 8     | ICMPv6 code             | [this document] |
412	           | 9     | TCP flags               | [this document] |
413	           | 10    | Packet length           | [this document] |
414	           | 11    | DSCP                    | [this document] |
415	           | 12    | Fragment                | [this document] |
416	           | 13    | Flow Label              | [this document] |
417	           +-------+-------------------------+-----------------+

[major] The definition of some of these values is not done in "this
document", but in rfc5575bis.  In those cases, please list both as a
reference.


[major] Even if the probability is low...  If new components are
added, if they apply to both IPv4/IPv6, how will the registries be
kept in sync?  Should the Experts assign/request that an IPv4-related
request be extended to also cover IPv6 (or viceversa)?  Should the
Experts reserve the corresponding code points in the other registry?
Will we reply on the Experts to keep both registries the same?

Here's an idea:  Currently all the components either are exactly the
same, or share a name (modulo using ICMPv6 instead of ICMP, for
example).  Can we design a table that incorporates common Type/Name,
and separate columns for IPv4 and IPv6 with the proper references?

I'm thinking of something like this:

  +------+--------------------+------+------+----------------------------+
  | Type | Name               | IPv4 | IPv6 |References                  |
  +------+--------------------+------+------+----------------------------+
  | 1    | Reserved           | Y    | Y    | [rfc5575bis]               |
  +------+--------------------+------+------+----------------------------+
  | 2    | Destination Prefix | Y    |      | [rfc5575bis]               |
  |      |                    |      | Y    | [ietf-idr-flow-spec-v6]    |
  +------+--------------------+------+------+----------------------------+

...or we could put all the information for "2" in the same line with
both references.

This way we end up with one registry and no additional coordination.
Note that we can have the same Experts for both registries...but this
way they'll never fall out of sync.

If we go this way, or something similar, then we'll need to formally
Update rfc5575bis to change the registry format (no change needed in
that other document).


...
451	   +------------+----------------------------------------+-------------+
452	   | Sub-Type   | Name                                   | Reference   |
453	   | Value      |                                        |             |
454	   +------------+----------------------------------------+-------------+
455	   | TBD        |             Flow spec rt-redirect-ipv6 | [this       |
456	   |            | format                                 | document]   |
457	   +------------+----------------------------------------+-------------+

[nit] s/            Flow/Flow


...
462	9.  Acknowledgements

464	   Authors would like to thank Pedro Marques, Hannes Gredler and Bruno
465	   Rijsman, Brian Carpenter, and Thomas Mangin for their valuable input.

[nit] s/Gredler and Bruno/Gredler, Bruno


...
488	11.1.  Normative References
...
500	   [RFC4271]  Rekhter, Y., Ed., Li, T., Ed., and S. Hares, Ed., "A
501	              Border Gateway Protocol 4 (BGP-4)", RFC 4271,
502	              DOI 10.17487/RFC4271, January 2006,
503	              <https://www.rfc-editor.org/info/rfc4271>.

== Unused Reference: 'RFC4271' is defined on line 500, but no explicit
   reference was found in the text