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
- [Idr] AD Review of draft-ietf-idr-flow-spec-v6-14 Alvaro Retana
- Re: [Idr] AD Review of draft-ietf-idr-flow-spec-v… Christoph Loibl
- Re: [Idr] AD Review of draft-ietf-idr-flow-spec-v… Alvaro Retana
- Re: [Idr] [BULK] AD Review of draft-ietf-idr-flow… Christoph Loibl