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

[Warren Kumari <[e-mail]>]

Hi all,

<no-hats>
I have reviewed draft-ietf-idr-wide-bgp-communities, and wrote up something
similar to a directorate-style review; Alvaro suggested that I send it to
the authors and WG.

Apologies in advance if these questions have already been discussed in the
WG - I haven't been following IDR particularly closely lately.

W
</no-hats>
--------
Document: draft-ietf-idr-wide-bgp-communities
Reviewer: Warren Kumari
Review Date: 2022-10-25
Intended Status: Standards Track
Link: https://datatracker.ietf.org/doc/draft-ietf-idr-wide-bgp-communities/



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).

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...


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.


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.


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).

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...



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
)?


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...


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?



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?


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...


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?


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...



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.



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."


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?



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?







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.


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.


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'


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'