Re: [Idr] Review of draft-ietf-large-community-06.txt

Job Snijders <job@ntt.net> Fri, 04 November 2016 20:16 UTC

Return-Path: <job@ntt.net>
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 6FE6C1295DA for <idr@ietfa.amsl.com>; Fri, 4 Nov 2016 13:16:56 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -3.421
X-Spam-Level:
X-Spam-Status: No, score=-3.421 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, RP_MATCHES_RCVD=-1.497, SPF_SOFTFAIL=0.665, T_HTML_ATTACH=0.01] autolearn=ham autolearn_force=no
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 E2SOym9o1zeU for <idr@ietfa.amsl.com>; Fri, 4 Nov 2016 13:16:49 -0700 (PDT)
Received: from mail3.dllstx09.us.to.gin.ntt.net (mail3.dllstx09.us.to.gin.ntt.net [IPv6:2001:418:3ff:5::26]) (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 127A112958D for <idr@ietf.org>; Fri, 4 Nov 2016 13:16:49 -0700 (PDT)
Received: by mail3.dllstx09.us.to.gin.ntt.net with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.84_2) (envelope-from <job@ntt.net>) id 1c2kuh-0003YV-UY (job@us.ntt.net); Fri, 04 Nov 2016 20:16:48 +0000
Date: Fri, 4 Nov 2016 21:16:31 +0100
From: Job Snijders <job@ntt.net>
To: Geoff Huston <gih@apnic.net>
Message-ID: <20161104201631.GA35942@Vurt.lan>
References: <112dc01d235fd$57f9c370$07ed4a50$@ndzh.com> <C2DABF02-D3CB-4646-B869-FBCE5F05FDA1@apnic.net> <117ea01d23611$a28513e0$e78f3ba0$@ndzh.com> <CED07D95-A426-469C-85B4-DB2FBE52D14A@apnic.net> <6c5d83aa1d6a4a04b651b8f14f4b445b@XCH-ALN-014.cisco.com> <40D942F5-0710-46D1-BF09-76C827377479@cisco.com> <95F42982-7DCF-46A9-A26C-71EF70DB3C59@apnic.net> <20161104195346.GK961@Vurt.local>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="J/dobhs11T7y2rNN"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20161104195346.GK961@Vurt.local>
X-Clacks-Overhead: GNU Terry Pratchett
User-Agent: Mutt/1.7.1 (2016-10-04)
Archived-At: <https://mailarchive.ietf.org/arch/msg/idr/Z5jWuZyprHW7ZZnqnxhmeT266vU>
Cc: "idr@ietf.org" <idr@ietf.org>
Subject: Re: [Idr] Review of draft-ietf-large-community-06.txt
X-BeenThere: idr@ietf.org
X-Mailman-Version: 2.1.17
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: Fri, 04 Nov 2016 20:16:56 -0000

Hi everyone,

Attached is the rfcdiff html file that represents the current -06 -> -07
changes with the work from Jakob and Geoff and some nits that were
picked by John Scudder.

I should note the following, in "Reserved BGP Large Community values"
the 'reasons' behind each reserved value were removed. Some in the
working group made a case that it could be good to provide a
justification what makes these numbers special.

Other then that, i feel that these changes have improved the document's
flow and readability.

Kind regards,

Job

On Fri, Nov 04, 2016 at 08:53:46PM +0100, Job Snijders wrote:
> On Sat, Nov 05, 2016 at 06:41:32AM +1100, Geoff Huston wrote:
> > Jakob,
> > 
> > Many thanks for sending this.
> > 
> > I have used Word to mark up my reactions to this proposed revisions
> > for your further review as this tool has a good track changes option -
> > the PDF of the marked up text is attached to this note.
> > 
> > My suggested changes to your proposed test are:
> > 
> > 1) change to the Introduction to alter the description of Communities
> > to conform more exactly to the language used in RFC 1997
> > 
> > 2) change to the considerations of RFC4360 in the Introduction. (I
> > also note that RFC4360 calls the Global Administrator and Local
> > Administrator parts of the community value “sub-fileds” rather than
> > “fields”, while this draft calls these elements just “fields” )
> > 
> > 3) the set of values is explicitly called out to be “unordered” in the
> > Introduction
> > 
> > 4) The description of the attribute fields is altered to be consistent
> > with RFC4360
> > 
> > 5) the treatment of the Global Administrator value is unclear. I have
> > proposed removing the last two sentences and propose changing the MUST
> > to a SHOULD, as, later in the draft in Section 6, it is noted that the
> > Global Administrator field can be any value!
> > 
> > 6) I have explicitly called out that the values are in any order, and
> > allowed for two or more individual Large Communities attributes to be
> > contained in a single BGP Update message, again in any order. If this
> > is not your intent then you should look at this part of section 2 and
> > tighten up the wording.
> > 
> > 7) you have not explicitly disambiguated between duplicate instances
> > of the same attribute and duplicate instances of the same Community
> > value. I have offerred some text to clarify this to the level of
> > values.
> > 
> > 8) I have proposed changing the MUST to a SHOULD in rthe canonical
> > representation section
> > 
> > 9) The sentences about the use of a colon make no sense given that you
> > have not specified a colon separator in the preceeding paragraph. You
> > probably need to either drop the reference to the colon, or add a
> > colon as part of the Canonical Representation. I have proposed the
> > former.
> > 
> > 10) - in section 6 I changed the “may” to a “MAY” - seemed like the
> > right thing to do.
> > 
> > 11) I added a paragraph in the Security Considerations section
> > explicitly that the values in this attribute are not specifically
> > protected and then pointed to BGP security (just because an ASN is
> > used in a community attribute, that does not mean that the ASN in
> > question attached that attribute, or that the ASN will honour that
> > attribute. - its just a signal without any integrity protection)
> > 
> > 12) I have added my name as a reviewer - hope that’s ok! :-)
> > 
> > I _assume_ that the atomic aggregator mess will be fixed up by the
> > action of making this an historic attribute, so I have left the text
> > on aggregation unaltered.
> > 
> > Regards,
> > 
> >   Geoff
> > 
> > 
> > 
> > > On 5 Nov. 2016, at 1:20 am, Jakob Heitz (jheitz) <jheitz@cisco.com> wrote:
> > > 
> > > Geoff,
> > > 
> > > Here is my proposed revision. I made all the changes, except the ATOMIC_AGGREGATE. If after the latest discussion, you still want it, I'll make that change too.
> > > 
> > > Thanks,
> > > Jakob.
> > > 
> > > 
> > > Begin forwarded message:
> > > 
> > >>> -----Original Message-----
> > >>> From: Idr [mailto:idr-bounces@ietf.org] On Behalf Of Geoff Huston
> > >>> Sent: Thursday, November 03, 2016 5:14 PM
> > >>> To: IETF IDR WG <idr@ietf.org>
> > >>> Cc: rtg-dir@ietf.org; Susan Hares <shares@ndzh.com>
> > >>> Subject: [Idr] Review of draft-ietf-large-community-06.txt
> > >>> 
> > >>> I have reviewed this draft and have the following 9 suggestions. All of
> > >>> these fall into the category of minor suggestions. I do not believe that
> > >>> there is may major structural deficiency in this specification, and the
> > >>> document is largely ready, in my view.
> > >>> 
> > >>> Here are my specific suggestions.
> > >>> 
> > >>> 
> > >>> 1. ----------------
> > >>> 
> > >>> Title: Large BGP Communities
> > >>> 
> > >>> to be consistent with previous attribute specifications, how about
> > >>> 
> > >>> "BGP Large Communities Attribute"
> > >>> 
> > >>> i.e. switch the order of "Large BGP" to "BGP Large" to be consistent with
> > >>> earlier BGP Extended Communities specification in RFC4360
> > >>> 
> > >>> 2. ----------------
> > >>> 
> > >>> "Network operators
> > >>> attach BGP communities to routes to identify intrinsic properties of
> > >>> these routes."
> > >>> 
> > >>> I don't think community attributes are an intrinsic property of a route
> > >>> advertisement - they are more appropriately expressed as an attached attribute
> > >>> that expresses some desired property.
> > >>> 
> > >>> how about:
> > >>> 
> > >>> "Network operators attach BGP communities to routes to associate
> > >>> particular properties with these routes."
> > >>> 
> > >>> 3. ----------------
> > >>> 
> > >>> "and may apply to an individual route or to a group of routes."
> > >>> 
> > >>> I am confused - surely the attributes in an Update BGP message apply to the
> > >>> collection of routes contained in the Update. It cannot be applied to a
> > >>> single route when the update itself contains multiple routes. Why not
> > >>> use the text:
> > >>> 
> > >>> "and is applied to all routes contained in a BGP Update Message where
> > >>> the Communities Attribute is included."
> > >>> 
> > >>> 4. ----------------
> > >>> 
> > >>> "[RFC1997] BGP Communities attributes are four-octet values split into
> > >>> two two-octet words."
> > >>> 
> > >>> This is not the case - RFC1997 says: "Communities are treated as 32 bit values"
> > >>> I think it would be more accurate to say:
> > >>> 
> > >>> "BGP Communities attributes are four-octet values [RFC1997]. Common use of
> > >>> this attribute type splits this single 32-bit value field into two 16-bit values."
> > >>> 
> > >>> 5. ----------------
> > >>> 
> > >>> "The BGP Extended Communities
> > >>> attribute [RFC4360] is also unsuitable, as the protocol limit of six
> > >>> octets cannot accommodate both a four-octet Global Administrator
> > >>> value and a four-octet Local Administrator value, which precludes the
> > >>> common operational practice of encoding a target ASN in the Local
> > >>> Administrator field."
> > >>> 
> > >>> 
> > >>> Thats a very long sentence. Try:
> > >>> 
> > >>> "The BGP Extended Communities attribute [RFC4360] is also unsuitable, as
> > >>> the protocol limit of six octets for each community value cannot
> > >>> accommodate both a four-octet Global Administrator value and a four-octet
> > >>> Local Administrator value. This limitation precludes the common
> > >>> operational practice of encoding a target ASN in the Local Administrator
> > >>> field."
> > >>> 
> > >>> 6. ----------------
> > >>> 
> > >>> The aggregation section contains fewer constraints then either RFC1997 or
> > >>> RFC4360. It also contains a confusing non-normative “should".
> > >>> 
> > >>> For reference, RFC4360 states: By default if a range of routes is to be
> > >>> aggregated and the resultant aggregates path attributes do not carry the
> > >>> ATOMIC_AGGREGATE attribute, then the resulting aggregate should have an
> > >>> Extended Communities path attribute that contains the set union of all the
> > >>> Extended Communities from all of the aggregated routes.  The default
> > >>> behavior could be overridden via local configuration, in which case
> > >>> handling the Extended Communities attribute in the presence of route
> > >>> aggregation becomes a matter of the local policy of the BGP speaker that
> > >>> performs the aggregation.
> > >>> 
> > >>> Why not use this formulation? i.e.
> > >>> 
> > >>> "3. Aggregation
> > >>> 
> > >>> If a set of routes is to be aggregated and the resulting aggregate route's
> > >>> path attributes do not include the ATOMIC_AGGREGATE attribute, then the
> > >>> resulting aggregate route SHOULD have a Large Communities Attribute formed
> > >>> from the set union of all the Large Community values from all of the
> > >>> aggregated set of routes.  This behavior MAY be overridden via local
> > >>> configuration, in which case handling the Large Communities attribute in
> > >>> the presence of route aggregation is determined by the local policy of the
> > >>> BGP speaker that performs the aggregation."
> > >>> 
> > >>> 7. ----------------
> > >>> 
> > >>> 4.  Canonical Representation
> > >>> 
> > >>> I am confused here - this section used an example with TWO canonical
> > >>> representations:
> > >>> 
> > >>>  "For example: 64496:4294967295:2, 64496:0:0, or (64496, 111, 222)."
> > >>> 
> > >>> 
> > >>> Conventionally, it's better to use a single canonical representation, so the
> > >>> authors should pick either a colon-delimited list, or a bracketed comma+space
> > >>> separated object.
> > >>> 
> > >>> 
> > >>> 8. ----------------
> > >>> 
> > >>> 5.  Reserved Large BGP Community values
> > >>> 
> > >>> 
> > >>> The text reads:
> > >>> 
> > >>> "the Global Administrator values specified above could be
> > >>> used if there is a future need for them."
> > >>> 
> > >>> This is entirely unclear. Is it referring to the values that the previous
> > >>> paragraph specified as "SHOULD NOT use”? Also "could be used" is a poor
> > >>> choice of words for a normative specification.
> > >>> 
> > >>> I suggest deleting completely the paragraph that contains this quote from
> > >>> the document.
> > >>> 
> > >>> 
> > >>> 9. ----------------
> > >>> 
> > >>> The text reads:
> > >>> 
> > >>> "The error handling of Large BGP Communities is as follows:
> > >>> 
> > >>>  o  A Large BGP Communities attribute SHALL be considered malformed if
> > >>>     its length is not a non-zero multiple of 12."
> > >>> 
> > >>> 
> > >>> I think the author is trying to dayL
> > >>> 
> > >>> "The error handling of Large BGP Communities is as follows:
> > >>> 
> > >>>  o  A Large Communities attribute SHALL be considered malformed if the
> > >>>    length of the Large Communities value, expressed in octets, is not a
> > >>>    non-zero multiple of 12."
> > >>> 
> > >>> 
> > >>> thanks,
> > >>> 
> > >>> Geoff
> > >>> 
> > >>> _______________________________________________
> > >>> Idr mailing list
> > >>> Idr@ietf.org
> > >>> https://www.ietf.org/mailman/listinfo/idr
> > > <draft-ietf-idr-large-community.xml><draft-ietf-idr-large-community-07.txt><draft-ietf-idr-large-community-07-diff-from-06.html>
> > 
> 
> > _______________________________________________
> > Idr mailing list
> > Idr@ietf.org
> > https://www.ietf.org/mailman/listinfo/idr
> 
> _______________________________________________
> Idr mailing list
> Idr@ietf.org
> https://www.ietf.org/mailman/listinfo/idr