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

[Geoff Huston <[e-mail]>]

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>