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

["Jakob Heitz (jheitz)" <[e-mail]>]

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