Early Review of draft-ietf-idr-large-community-06
review-ietf-idr-large-community-06-rtgdir-early-huston-2016-11-14-00

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