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

Christoph Loibl <c@tix.at> Mon, 21 September 2020 07:15 UTC

Return-Path: <c@tix.at>
X-Original-To: idr@ietfa.amsl.com
Delivered-To: idr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 5DB3B3A0BF3; Mon, 21 Sep 2020 00:15:24 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.099
X-Spam-Level:
X-Spam-Status: No, score=-2.099 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=tix.at
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id WmkHPrwVm6QE; Mon, 21 Sep 2020 00:15:19 -0700 (PDT)
Received: from mail.fbsd.host (mail.fbsd.host [IPv6:2001:858:58::22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 9E9603A147A; Mon, 21 Sep 2020 00:15:18 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tix.at; s=rev1; h=To:References:Message-Id:Content-Transfer-Encoding:Cc:Date: In-Reply-To:From:Subject:Mime-Version:Content-Type:Sender:Reply-To:Content-ID :Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To: Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe :List-Post:List-Owner:List-Archive; bh=N2jUkZUfCTtvgpJOYOB44rc2wx/6kO7p0YRNcWgXpCM=; b=RNrQV5UjktGZOC3mWjqEESpfua fOQlT74Rhu32Zkt5Uhto+0MUSW7LS9gquJNPqqXd+AsBpFYeTboKbWia2TaOMwFI9/dcLXlEyZW5U dJ2rLfJf3n6p1NmA4ZfKsndZd0TgWNZh0Mf2fyqkOXeLC3bkXq0UKa+iNN32X4oOE3JXpDaKex/27 L8fqe6q28qpDbrSxJBRsWIjhj0OcCo7QHluwE/aNC8o0YNnN+YO+MyDfS5xy3+kqbv9FSBQapffi4 XcBt1GNtScY/Ey2qESpahGPHQIP966NAatPNkuPybhdf1tmTYaNh+maLC34wynpkRSn8rAHRGM9Fa qAA8LdSw==;
Received: from 80-110-113-91.cgn.dynamic.surfer.at ([80.110.113.91] helo=[192.168.66.207]) by mail.fbsd.host with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92.3) (envelope-from <c@tix.at>) id 1kKG2c-000LGO-Lo; Mon, 21 Sep 2020 09:15:15 +0200
Content-Type: text/plain; charset="utf-8"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\))
From: Christoph Loibl <c@tix.at>
In-Reply-To: <CAMMESswsbgV3pJJU7i=9F3N3w2zc6--oCVtxrVXKd3APg3+zhw@mail.gmail.com>
Date: Mon, 21 Sep 2020 09:15:13 +0200
Cc: draft-ietf-idr-flow-spec-v6@ietf.org, idr-chairs@ietf.org, IDR List <idr@ietf.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <CA851932-4E1B-4882-B8F3-842BB5E288DA@tix.at>
References: <CAMMESswsbgV3pJJU7i=9F3N3w2zc6--oCVtxrVXKd3APg3+zhw@mail.gmail.com>
To: Alvaro Retana <aretana.ietf@gmail.com>
X-Mailer: Apple Mail (2.3608.120.23.2.1)
X-Scanned-By: primary on mail.fbsd.host (78.142.178.22); Mon, 21 Sep 2020 09:15:14 +0200
Archived-At: <https://mailarchive.ietf.org/arch/msg/idr/wKypUngz-EOr-7LlzgSNnjokcX0>
Subject: Re: [Idr] AD Review of draft-ietf-idr-flow-spec-v6-14
X-BeenThere: idr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Inter-Domain Routing <idr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/idr>, <mailto:idr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/idr/>
List-Post: <mailto:idr@ietf.org>
List-Help: <mailto:idr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/idr>, <mailto:idr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 21 Sep 2020 07:15:24 -0000

Hi Alvaro,


> On 18.08.2020, at 00:44, Alvaro Retana <aretana.ietf@gmail.com> wrote:
> 
> 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


Alvaro, thank you for your review of draft-ietf-idr-flow-spec-v6-14!

We worked on the issues that you raised and I just uploaded a -15 that addresses the issues you raised. All issues have been tracked on Github and the commits mention the assigned issue#:

	https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6

Please see all the links to Github below. For the major issues I also added some comments inline.

Cheers Christoph


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

<-- Authors
Tracked via issue #19: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/19
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/430a1512915795499f035237da35e487b4f37b7a
-->

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

<-- Authors
Tracked via issue #20: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/20
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/ecb76322384f352429c1da92744823d49cd689f1
-->

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

<-- Authors
Tracked via issue #21: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/21
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/b1364e43168e11ffb8798363025fd53d41860156
-->

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

<-- Authors
Tracked via issue #22: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/22
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/b3207317b37e57dcb68d73203d07a8f297369541
-->

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

<-- Authors
Tracked via issue #23: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/23
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/7aa27b32bb2c87d9741259492eddafa44e15f97e
-->

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

<-- Authors
Tracked via issue #24: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/24
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/e73764790033971e0c4d4f87a48204405c39398e
-->

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

<-- Authors
Tracked via issue #25: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/25
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/b53214f6b07cc401d69504ad3e2cf81535d6f6e6
-->

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

<-- Authors
Tracked via issue #26: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/26
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/e2ebf707420bf91857173931007d206311411477
-->

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

<-- Authors
Tracked via issue #27: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/27
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/a6f9b7447009278814a0a9ae2e93e79802503743
-->

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

<-- Authors
Tracked via issue #28: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/28
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/a751f2affc97060d6c572e418b8d49aaf75312e6

Solved as suggested.
-->

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

<-- Authors
Tracked via issue #29: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/29
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/0a316b3cfb24af2c6311f8612a681c0062a5b2f0

Added explicit individual description of the fields. Also edited the list so that it is clear that we have optional padding and a pattern that gets matched.
-->

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

<-- Authors
Tracked via issue #30: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/30
Related issue mentioned: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/29

Solved with the Type 1 rewrite
-->

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

<-- Authors
Tracked via issue #31: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/31
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/f6f524e5fb530a750231989161a6efbd8c8f8b28

Used the correct terms from rfc8200. Also changed the name to "Upper-Layer Protocol" which seems to be the better choice.
-->

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

<-- Authors
Tracked via issue #32: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/32
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/022b1e186b61d8f360cd51ec61e804272163f75c
-->

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

<-- Authors
Tracked via issue #33: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/33
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/c4f424126ef92b87a9509160a9f0611f045b2748
-->

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

<-- Authors
Tracked via issue #34: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/34
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/7829138daecf52f41f0e2a9ff3cebecf9cc328c7
-->

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

<-- Authors
Tracked via issue #35: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/35
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/7829138daecf52f41f0e2a9ff3cebecf9cc328c7
-->

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

<-- Authors
Tracked via issue #36: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/36
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/7829138daecf52f41f0e2a9ff3cebecf9cc328c7

We changed the "last Next Header field value" to "upper-layer IP protocol value" in that paragraph. But I do not think that additionally qualifying what the reasons for not being able to decode the headers is needed. 
-->

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

<-- Authors
Tracked via issue #37: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/37
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/29d57d659357e1e4aaa0529b6b9114e2b2f4e28d
-->

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

<-- Authors
Tracked via issue #38: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/38
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/29d57d659357e1e4aaa0529b6b9114e2b2f4e28d

Same as above.
-->

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

<-- Authors
Tracked via issue #39: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/39
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/511499d7f2d9d03ab63a6aa3309d8119fa915cd8
-->

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

<-- Authors
Tracked via issue #40: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/40
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/3ad353b7a3c7713866c9921360199082304fbc2a

I think that s/0x01/0x64 is not correct. Since the prefix has been changed this is not relevant anymore.
The example now contains the upper-layer protocol matching and some explanation on the src/dst-prefix pattern encoding.
-->

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

<-- Authors
Tracked via issue #41: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/41
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/1b897d221aa2862fff6731287b3bee419bc83856
-->

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

<-- Authors
Tracked via issue #42: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/42
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/fd8b37158788842479c32c782a670e0f3bb94a32

The encoding of the src/dst prefix-bitpattern (Type 1,2) allow flexible matching on an arbitrary bitstring (with offset and length) within the prefix. Validation is only looking at the dst-prefix component. Src-prefix with offset!=0 is still valid. 

The problem with offset != 0  in general is that it generates non-contiguous prefix matches.

For example a matching pattern of ::1234:5678:9A00:0/64-104:

```
0000:0000:0000:0000:1234:5678:9A00:0000 - 0000:0000:0000:0000:1234:5678:9A03:FFFF matches
0000:0000:0000:0000:1234:5678:9A04:0000 - 0000:0000:0000:0000:1234:5678:99FF:FFFF does not
... (many in between)
2001:0DB8:0000:0000:1234:5678:9A00:0000 - 2001:0DB8:0000:0000:1234:5678:9A03:FFFF matches
2001:0DB8:0000:0000:1234:5678:9A04:0000 - 2001:0DB8:0000:0001:1234:5678:99FF:FFFF does not
2001:0DB8:0000:0001:1234:5678:9A00:0000 - 2001:0DB8:0000:0001:1234:5678:9A03:FFFF matches
and so on...
```
Dst-Prefix with offset != 0 may be useful in a trusted environment when the validation has been relaxed, but this is out of the scope of this document.

Furthermore the examples have been modified so that both of them are feasible. 
-->

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

<-- Authors
Tracked via issue #43: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/43
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/fccc426cfb3c6e7242cbefbed4f704896b301aa4
-->

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

<-- Authors
Tracked via issue #44: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/44

Actually this is fine. Those (actually it is only one) from rfc5575bis only interfere with themselves (since there is only one redirect community specified in rfc5575bis). Now, because fs-v6 adds another redirect community this interferes with the other specified communities in rfc5575bis and also with itself. Section 7.7. of rfc5575bis also contains considerations on interfering traffic actions.
-->

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

<-- Authors
Tracked via issue #45: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/45
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/2816b23ecf27e5a8a7561a6a05f087b069136f02

Solved as suggested.
-->

> 
> 393	8.  IANA Considerations
> 
> 395	   This section complies with [RFC7153]
> 
> [nit] s/[RFC7153]/[RFC7153].
> 
> [] Please add subsections to separate the 2 different actions below.
> 

<-- Authors
Tracked via issue #46: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/46
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/6d11e04e73d6450aed3773dd7541e9d48866b914
-->

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

<-- Authors
Tracked via issue #47: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/47
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/94cf8662d5d487b216a9593a307caed8a860b051
Related issue mentioned: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/48

As of #48 the ipv4 and ipv6 FS registries have been joined togather. This paragraph has been removed. 
-->

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

<-- Authors
Tracked via issue #48: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/48
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/358ef1926dc136c7513ac3f7d4cfd45cce824d16

Changed IANA considerations to merge IPv4/IPv6 FS types into one registry.
-->

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

<-- Authors
Tracked via issue #49: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/49
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/699cce7b37458f954d1433517ac46bc3a8765cd6
-->

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

<-- Authors
Tracked via issue #50: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/50
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/cf2d842e723a463a08633199bf8966aa7e32d15d
-->

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

<-- Authors
Tracked via issue #51: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/issues/51
Commit mention: https://github.com/stoffi92/draft-ietf-idr-flow-spec-v6/commit/59164d517bf675550c9368388876cc90c3b3e969
-->



-- 
Christoph Loibl
c@tix.at | CL8-RIPE | PGP-Key-ID: 0x4B2C0055 | http://www.nextlayer.at