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

[Job Snijders <[e-mail]>]

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) <[e-mail]> 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:[e-mail]] On Behalf Of Geoff Huston
> > >>> Sent: Thursday, November 03, 2016 5:14 PM
> > >>> To: IETF IDR WG <[e-mail]>
> > >>> Cc: [e-mail]; Susan Hares <[e-mail]>
> > >>> 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
> > >>> [e-mail]
> > >>> 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
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/idr
> 
> _______________________________________________
> Idr mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/idr