Re: [Idr] Initial review of draft-ietf-idr-wide-bgp-communities

[Robert Raszuk <[e-mail]>]

Hello,

As indicated earlier in addressing Alvaro's review I went back and
incorporated most Warren's comments.

Thank you for those !!!

Some of those were already added but apparently between many versions  and
many githubs accounts were lost - so I added them again in version -10
(soon to be posted).

*Comments in bold for easy reading*


Meta comment:
> There are 6 authors listed. This exceeds our standard 5 (just mentioning,
> but the reason should be noted in the writeup or during balloting).
>

*Yes - already addressed as part of Alvaro's review. I think we have an
action plan to justify 6. *

The only mentions of Large BGP Communities are in the Operational
> Considerations ("Having multiple ways [...] - via the use of Standard,
> Extended or Large BGP Communities versus the use of BGP Wide Communities
> …") and Security Considerations ("All the security considerations for BGP
> Communities [RFC1997], BGP  Extended Communities [RFC4360], and BGP Large
> Communities [RFC8092] apply to BGP Community Containers."). This isn't
> really a problem, and I understand that these are somewhat competing
> documents, but it did feel a little odd...
>

*Fixed.*

Questions and concerns:
> Section 2.1 - BGP Community Container Common Header:
> O: "The BGP Community Container Common Header is defined in Section 3.1
> and contains following encoding:"
> C: The document would be much easier to understand if Section 3 ("BGP
> Community Container Attribute"), 3.1 ("BGP Community Container Attribute
> Common Header"), etc were to be moved here (and also if it included an
> example early). Section 2.1 defines part of the BGP Community format, but
> does it somewhat poorly, and confusingly. The actual definition is in 3.1,
> and it repeats a bunch of Section 2.1. This makes is somewhat confusing -
> there isn't really enough information in 2.1 to get a handle on how the
> protocol works, and so the reader is somewhat confused (or has to flip to
> 3.1 and then come back). A simple reorganization and removal of duplicates
> would make this all much much simpler to read (and would avoid having to
> figure if things have changed between the sections, etc.).
>
> Section 2.3.  BGP Community Container Atoms
> O: "Atoms provide data types that can be used to encode contents of BGP
> Community Containers.  They are in the format of TLVs and are defined later
> in this document in Section 5."
>
> C: This is the entirety of Section 2.3 - it's just a forward reference to
> Section 5. This illustrates my above point -- reorganizing the document to
> be either "this is the purpose and here are some examples. This is the
> protocol…", or better yet "These are the primitives/foundation (format &
> header, definitions, etc), here is an example of why you might use it,  and
> this is how you assemble the parts into Wide Communities" would be much
> more understandable.
>

*Understood but this required massive rework of the entire draft which at
this point we are in no position to do. *


Section 3.  BGP Community Container Attribute
>
> "This document defines a BGP Path Attribute, the BGP Community Container.
> The attribute type code is 34.
>
>    The BGP Community Container attribute is an optional, transitive BGP
>    attribute, and may be present only once in the BGP UPDATE message."
>
> Okey dokey… but, what do I do if there are multiple BGP Community
> Container attributes? This is only somewhat answered in Section 8.1
> ("General Error Handling for BGP Community Containers"), which says:
> "[RFC7606] "treat as withdraw" behavior is expected for any malformed
> Community Containers or malformation of their contents.". I'm *assuming*
> that the right behavior in this case is treat as withdraw, but I'll point
> out that more than one isn't actually a "malformed Community Container".
> The behavior here should be made more explicit.
>

*Fixed. *


Section 3.1.  BGP Community Container Attribute Common Header
> The table list 2 flags (T and C), and then says that bits 3..7 are
> reserved. AFAICT (and I may just be dumb at this point) it should either
> say bits "2..7" (if they are counting from 0 which is what I'd assume), or
> "3..8" (if counting from 1).
>

*Fixed ... actually 3..7 :) *



> For the definition of the flags. For the T (Transitivity bit) it says:
> "When not set (value 0), the community in the container is transitive
> across AS boundaries, but not across an administrative boundary.
>
> When set (value 1), the community in the container is transitive across
> all ASes.  An administrative boundary, in this sense, is an arbitrary set
> of connected ASes, possibly under control of a single entity.  How such an
> administrative boundary is determined is out of the scope of this document."
>
> Is this OK? If the document specifies the behavior for a bit, and
> especially one that is presumably important like this, is this level of
> vagueness around "possibly under control of a single entity" acceptable?
> Note: This may already be well known / understood in the IDR group, but it
> certainly wasn't clear to me exactly how far I would expect this to
> propagate, nor, if I managed multiple ASes, how I would delineate "this AS
> is part of my admin group, but *this* is the boundary, don't propagate past
> here". Or, is the intent that
> operators are expected to manually write route policy filters which match
> on this bit and manually filter them? I'm *assuming* that the default
> behavior on an eBGP session (to some other network) is to filter on this
> bit, but that means that I need a way to override this to say "Nah, AS42 is
> also me, so don't filter these".
> It seems that more text is needed here...
>

*We all discussed this one at length. We decided that especially now IETF
wide there are more solid definitions of "administrative boundaries" that
this document can not mess with. So we decided to go with "out of scope
here"*


Section 4.1.  Community Value
> O: "When the high order bit of the Community Value field - I - is set, the
> value is IANA Registered and has a well defined meaning with underlying
> semantics."
> C: It seems like it would be nice to have a link to the registry here --
> I'm *assuming** this is meaning the  "Border Gateway Protocol (BGP)
> Well-known Communities" (
> https://www.iana.org/assignments/bgp-well-known-communities/bgp-well-known-communities.xhtml
> )?
>

*Nope. It means a new registry for the companion draft which is not defined
yet. *

Section 4.4.1.  Sub-Type 1, BGP Wide Community Target(s) TLV
> O: "Thus, if any given target matches per the semantics of that Atom for
> the community, the community is considered to match and the action defined
> by the community should be executed."
> C: This is probably fine, especially because this is a lowercase 'should',
> but it also feels like it should be something like "the action defined by
> the community is requested to be executed", or some "subject to
> administrative/operator policy" or similar. Not really a bit issue, but...
>

*Ack.*


>
> 4.4.2.  Sub-Type 2, BGP Wide Community Exclude Target(s) TLV
> O: "The semantic of the BGP Wide Community Exclude Target(s) is to match
> all specified Target(s) with the exception of those listed in this TLV." and
> "If the BGP Wide Community Target(s) TLV and the BGP Wide Community
> Exclude Target(s) TLV have conflicting semantics, priority MUST be given to
> the Wide Community Exclude Target(s) TLV."
>
> C: It seems like this section needs a fair bit more explanation of the
> semantics.
> Let's pretend that the "include" set (Target) contains [1,2,3,4] and
> "exclude" contains [3] - I'm *assuming* that the result should be the set
> difference of include\exclude ([1,2,4]), but this isn't stated as
> unambiguously as it could be...
>
> Or, if the "include" set contains [1,2,3,4] and "exclude" contains [10],
> is this OK? Presumably yes, but... ¯\_(ツ)_/¯
>
> Section 8.2 ("BGP Wide Community Container Error Handling") If any Atom in
> a BGP Wide Community container's Exclude Targets TLV is unrecognized, that
> Wide Community MUST NOT be considered a match and no actions for that
> community should be processed.", but this doesn't actually cover the above
> case -- I might "recognize" (this is actually not very well defined in the
> document) [1..20], but is a value outside the "include" set acceptable?
>

*Alvaro comments and yours I think fixed this ambiguity now. *

Section 5.1.  Atom Type 1, The Autonomous System Number List
>
> O: " Two special values are reserved for the Autonomous System Atoms:
>
>      0x00000000 - to indicate "No Autonomous Systems".
>      0xFFFFFFFF - to indicate "All Autonomous Systems"."
>
> C: I'm not actually sure why we need a "No Autonomous Systems" value.
> AFAICT, wherever you would you this you could just leave out the option -
> not really important, but I'm wondering if this means that I missed
> something?
>

*The paramount idea was that the community can distribute information which
may be useful but may not result in any actions. Sort of "informational
mode" if you like :) *


> Section 5.4.  Atom Type 5, The IEEE Floating Point Number List
> C: I don't really understand the use-case for this, and it makes me a
> little worried what the expectation is, but, well, meh...
>

*Just for completeness I think .. Or maybe decimal notation of geo
coordinates  ?*


Section 5.7.  Atom Type 8, the UTF-8 String
> O: "Implementations MUST be prepared for truncated/improperly formed
>    UTF-8 strings.  When detecting such a string, the implementation
>    should remove trailing octets of a multi-octet sequence in order to
>    have a well-formed string.
>
>    Implementations MUST be prepared to receive empty (zero-length) UTF-8
>    String Atoms as they may be used as Parameters."
>
> C: I'm really not sure if this is an issue, but what if I get a string of
> length 4, and the last octet makes it not-well formed, so I drop it… and
> then the 3rd octet makes it not well formed, so I drop it as well… and then
> the second octet makes it not well formed, so I drop it, and then the last
> octet makes it not well formed, so I drop it, and now the string is empty.
> This *might* not be a problem because I think that all 1 octet UTF-8
> strings are, by definition, valid, but I don't actually know. Also, the
> "... String Atoms as they may be used as Parameters" text also makes me a
> little worried about the expected use case and what exactly is being
> created  here -- do people expect that I could sent a community to e.g NTT
> with the UTF-8 string "Make me a sandwich"? Or "request system zeroize"?
> ASCII art pictures of a cat? What's the use-case?
>

*Frankly it is not my text. I don't really know what Jeff meant to say here
... Very sorry.  *

*I can only guess that he meant to say that if we specify atom type 8 to be
of X octets and operator configures Y octets where Y > X it should be
truncated. *

Section 6.  Well Known Standard BGP Communities
>
> O: "According to RFC 1997, as well as IANA's Well-Known BGP Communities
> registry, the following BGP communities are defined to have global
> significance:"
> P: "At the time of this writing, the IANA's Well-Known BGP Communities
> registry lists the following BGP communities as having have global
> significance:"
> C: What if someone adds e.g 0xFFFFFF42 to the IANA registry? I don't
> really understand why the document is copying and pasting values, but if
> there is a reason, it should allow for future changes...
>

*That got fixed based also on Alvaro's review. We honor all current and
future  members of BGP Well-Known BGP Communities*


Section 7.  Operational Considerations
> C: This section feels quite short (but, I'm biased!)
> Are operators *expected* to use these? Or just if they want to? If they
> define local use ones, should they be documented?
>
> Presumably if e.g Telefonica sends NTT a Wide community saying "Please set
> localpref to 1", NTT can choose to ignore this, yes? What if *I* send this
> to NTT and I'm not one of their customers, presumably they should ignore it
> as well, yes? (Some of the above can be handled with something like
> "Operators should apply the same policies to Wide Communities that they
> apply to regular communities" or similar).
>
> However, this still leaves the whole "transitive" but and the not very
> well defined "what is an administrative boundary?" question. Perhaps the
> intent is that the T bit is used just like "normal" BGP transitive vs
> non-transitive logic? If so, that's not really clear, especially because of
> the new admin text.
>

*copy: *

*We all discussed this one at length. We decided that especially now IETF
wide there are more solid definitions of "administrative boundaries" that
this document can not mess with. So we decided to go with "out of scope
here"*


Section 8.1.  General Error Handling for BGP Community Containers
>
> O: "[RFC7606] "treat as withdraw" behavior is expected for any malformed
> Community Containers or malformation of their contents."
> C: 'malformation' doesn't seem right here - perhaps "for any malformed
> Community Containers or with malformed contents."
>

*Fixed. *

Section 8.2.  BGP Wide Community Container Error Handling
>
> O: "If any Atom in a BGP Wide Community container's Exclude Targets TLV is
> unrecognized, that Wide Community MUST NOT be considered a match and no
> actions for that community should be processed.  While the Targets TLV is
> meant to be inclusive, the Exclude Targets TLV is meant to be proscriptive
> of applying the action."
> C: As mentioned above, this isn't especially clear -- I think that if I
> only recognize targets [1, 2, 3] and I get an include (Targets TLV) of
> [1,2,3,4] I should still process this and only apply the ones I recognize,
> but if I get an exclude of [17] I should ignore the whole thing? But, there
> is a fair bit of guessing going on here...
> Also, why is "include" inclusive and "exclude" proscriptive?
>

*Fixed. *

Section 9.2.  Example Type 1 BGP Wide Community Encoding
> O: "The T Flag (transitive) is unset to prevent propagation of this
> community outside of the provider's administrative domain."
> C: I'm uncomfortable with "prevent propagation" without better text around
> how a provider is expected to set the administrative domain. From what I
> can tell, this doesn't mean "AS", so it's not actually clear how far it
> should propagate. I'd assume it is "AS or some set of ASs which are acting
> as one", but the Section 3.1 description is sufficiently vague that I don't
> know what would happen if I sent this to e.g AS209 should I expect it to
> propagate to AS3356? AS3561?
>

*See  above discussion about administrative boundary. It is IETF wide
issue. *

Editorial and Nits:
> Abstract:
> O: "It is also a very common best practice for operators ..."
> P: "It is also a very common practice for operators..." or maybe "It is
> also a common best practice for operators ..."
> C: "very common best practice" feels redundant / reads oddly. Very much
> just a nit though.
>

*Fixed.*

Section 1:
> O: "the encoding of community values mandates that the first two octets
> contain the Autonomous System number, with the next two octets containing
> some locally defined value."
> P: "... some locally defined opaque value.""
> C: Also a nit, but it seems like this helps clarify the scope / expected
> understandability of the value.
>

*Fixed. *


> Section 2.1:
> O: "This provides hierarchy for future Community-like features."
> P: "This provides a hierarchy for future Community-like features."
> C: Nit - missing 'a'
>

*Fixed.*

Section 4.1:
> O: "When the high order bit of the Community Value field is clear, the
> value is Locally defined"
> P: "When the high order bit of the Community Value field is clear, the
> value is locally defined"
> C: Uppercase 'L' in 'Locally'
>

*Fixed. *

*Many many thx,*
*Robert*