[Idr] AD Review of draft-ietf-idr-wide-bgp-communities-08

[Alvaro Retana <[e-mail]>]

Dear authors:

Thank you for your continued efforts with this document!

I have many comments (see inline) that I will want to see addressed
before starting the IETF Last Call.

(1) In general, I found that the naming is not consistent across the document.
    For example, "BGP Wide Community", "Wide Community", "Wide community", and
    "side community" are all used.  The same treatment applies to the name of
    the TLVs, Atoms, etc.  I mentioned this below, but may have missed several
    occurrences.  Please take a look.

(2) Should this document be marked as replacing draft-ietf-idr-registered-wide-
    bgp-communities?  I know draft-ietf-idr-registered-wide-bgp-communities and
    draft-ietf-idr-wide-bgp-communities were both adopted at about the same
    time...


Besides my notes, please also address Warren's review [1].

Thanks!

Alvaro.


[1] https://mailarchive.ietf.org/arch/msg/idr/aW8XfPljF0YlrptpXPWf9bj96w4/


[Line numbers from idnits.]


2	IDR Working Group                                         R. Raszuk, Ed.
3	Internet-Draft                                                Individual
4	Intended status: Standards Track                            J. Haas, Ed.
5	Expires: 12 January 2023                                Juniper Networks
6	                                                           A. Lange, Ed.
7	                                                                   Nokia
8	                                                        B. Decraene, Ed.
9	                                                                  Orange
10	                                                               S. Amante
11	                                                             Apple, Inc.
12	                                                                P. Jakma
13	                                          Huawei Ireland Research Centre
14	                                                            11 July 2022

[major] Shepherd/Chairs: There are more than the recommended number of
authors in the front page (rfc7322).  Please work to reduce the list
to 5 (or less).  Some are already identified as Editors.  Others can
be added to the Contributors section.



...
19	Abstract
...
28	   This document defines a new encoding which will enhance and simplify
29	   what can be accomplished today with the use of BGP communities.  The
30	   most important addition this specification makes over currently
31	   defined BGP communities is the ability to specify and advertise an
32	   operator's parameters for execution It also provides an extensible
33	   platform for any future community encoding requirements.

[nit] Suggestion>
   This document defines a new encoding that enhances and simplifies
   what can be accomplished with BGP communities. This specification's
   most important addition is the ability to specify and advertise an
   operator's parameters for execution. It also provides an extensible
   platform for any future community encoding requirements.



35	Requirements Language

37	   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
38	   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
39	   document are to be interpreted as described in RFC 2119 [RFC2119].

[major] Use the template from rfc8174.



...
128	1.  Introduction
...
145	   The authors believe that defining a new BGP Path Attribute, with the
146	   ability to contain locally defined parameters will enhance the
147	   current level of network policies, as well as simplify BGP policy
148	   management.  The proposed encoding will also facilitate the delivery
149	   of new network services without the need to define a new BGP
150	   extension for each new application.

[minor] "The authors believe..."

This document is the product of the WG...

s/The authors believe that defining a new BGP Path Attribute/The
definition of a new BGP Path Attribute


[nit] Some of the claims (in this paragraph, for example) are written
in future tense (will enhance...will facilitate), but this
specification is the present.  Consider "enhances...facilitates...".



152	   When defining any new type of tool there is always a unique
153	   opportunity to specify a subset of well recognized behaviors.  Lists
154	   of the current most commonly used BGP communities, as well as
155	   creation of a new registry for future definitions will be described
156	   in an extension-specific document.

[minor] It seems that this paragraph may have been overtaken by
events: there's no "extension-specific document", the registries are
created here, etc..



...
166	2.1.  BGP Community Container Common Header
...
177	   The BGP Community Container Common Header is defined in Section 3.1
178	   and contains following encoding:

[minor] Please keep the description/specification of the encoding in
§3.1.  No need to specify/describe things more than once (and with
different words).


180	   Container Type:
181	       Container Type 1, BGP Wide Community is defined in this document.

183	   Flags:
184	       Flags control common behavior including the transitivity of the
185	       Container.

187	   Length:
188	       Length of the Container contents.

[] The description in §3.1 is more complete than this.  Adding partial
descriptions may result in confusion.



190	2.2.  Community Containers

192	   This document defines one Community Container with the following
193	   encoding:

[minor] Please keep the description/specification of the encoding in
§4.  No need to specify/describe things more than once (and with
different words).



...
230	3.  BGP Community Container Attribute
...
235	   The BGP Community Container attribute is an optional, transitive BGP
236	   attribute, and may be present only once in the BGP UPDATE message.

[major] What should a receiver do if the attribute is present more than once?


[major] Allowing only one Community Container doesn't permit the
ability to create both local *and* external policies for a given
prefix.  All BGP Wide Communities would either be transitive or not.
If transitive, any local policy would be propagated.  There is a
mention (in §10.1) of the ability to remove unwanted received
communities, but the document says nothing about controlling the
advertisement by filtering.  Should this functionality exist?  Please
add some text.



...
242	3.1.  BGP Community Container Attribute Common Header

244	   Containers always start with the following common header:

246	      0                   1                   2                   3
247	      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
248	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
249	     |             Type              |    Flags  |C|T|   Reserved    |
250	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
251	     |            Length             |
252	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

254	                     Figure 1: Common container header

256	   This document defines container type 1.  See the Section 11 for
257	   information on additional type registration policies.

[minor] Just to confirm: the definition in this section applies to all
types, not just type 1, is that correct?

If so, it would be nice if the specification of the common header was
presented first, including the general description of *all* fields.
This sentence (add a pointer to Section 4) should then be placed at
the end of the section.



259	    +======+=======+==================================================+
260	    | Bit  | Value | Meaning                                          |
261	    +======+=======+==================================================+
262	    |  T   |   0   | Not Transitive across administrative boundaries. |
263	    +------+-------+--------------------------------------------------+
264	    |      |   1   | Transitive across AS/administrative boundaries.  |
265	    +------+-------+--------------------------------------------------+
266	    |  C   |   0   | Not transitive across confederation boundaries.  |
267	    +------+-------+--------------------------------------------------+
268	    |      |   1   | Transitive across confederation boundaries.      |
269	    +------+-------+--------------------------------------------------+
270	    | 3..7 |   -   | RESERVED - MUST be zero when originated and      |
271	    |      |       | SHOULD be ignored upon receipt.                  |
272	    +------+-------+--------------------------------------------------+

[major] Should IANA manage the potential allocation of the reserved
bits?  If so, please include the request for a new registry in §11.



...
278	   Bit 0 (T bit) Transitivity bit:

280	      When not set (value 0), the community in the container is
281	      transitive across AS boundaries, but not across an administrative
282	      boundary.

[major] "transitive across AS boundaries, but not across an
administrative boundary"

This definition is confusing, specially for the case where the
"administrative boundary" is defined as only containing the local AS
-- the texts seems to contradict itself: transitive across AS
boundaries, but not "this" AS boundary.  Please clarify.



284	      When set (value 1), the community in the container is transitive
285	      across all ASes.  An administrative boundary, in this sense, is an
286	      arbitrary set of connected ASes, possibly under control of a
287	      single entity.  How such an administrative boundary is determined
288	      is out of the scope of this document.

[major] Please pull out the definition of an "administrative boundary"
from the description of the bit being set.  Should it be defined in
terms of an "administrative domain" (mentioned in §9.2) instead?



290	   Bit 1 (C bit) Confederation bit:

292	      The confederation bit is used to manage the propagation scope of a
293	      given BGP Wide Community across confederation boundaries.

[minor] s/given BGP Wide Community/given container

The Wide Community is just one type.


[minor] Please add a Normative reference to rfc3065.



295	      When not set (value 0) community is not transitive across
296	      confederation sub-AS boundary.  When set (value of 1) indicates
297	      that community in a given container is transitive across
298	      confederation boundary.

[major] "confederation sub-AS"

rfc3065 talks about a Member-AS, not a sub-AS.  Please be consistent
with existing terminology.


[major] A "confederation sub-AS boundary" and "confederation boundary"
are used apparently to mean the same thing.  But I don't think they
do.  Specifically, a "confederation boundary" seems to imply exiting a
confederation, which is different from advertising something from one
Member-AS to another.  Please clarify.


[major] What about the interaction between the T and C bits?
Specifically, if the T bit is set ("transitive across all ASes") and
the C bit is not set ("not transitive across confederation sub-AS
boundary").  A Member-AS is an AS, so it would fit in the description
of "all ASes".



...
303	   The Length field represents the total length in octets of a given
304	   container's contents.

[major] ...not including the common header, right?  Given that it
could be interpreted that the container starts with the header, it may
be worth clarifying.



...
329	4.1.  Community Value

331	   Community Value: 4 octets

[major] §11.5 uses "Community Type".



333	   The Community Value indicates what set of actions a router is
334	   requested to take upon reception of a route containing this
335	   community.  The semantics of this value depend on whether this is a
336	   private/local community or IANA registered.

338	   When the high order bit of the Community Value field - I - is set,
339	   the value is IANA Registered and has a well defined meaning with
340	   underlying semantics.  See the documentation for each Registered BGP
341	   Wide Community for its semantics and validation requirements.

343	   When the high order bit of the Community Value field is clear, the
344	   value is Locally defined and has semantics solely within the control
345	   of the AS defining that community.  The Context AS Number provides
346	   the namespace in which this Community Value is interpreted.  It is
347	   that AS's responsibility to provide the semantics and validation
348	   requirements for the BGP Wide Community.

350	   See Section 11.5 for code point space partitioning.

[major] Given how the ranges are allocated, the definition of the I
bit is not providing any additional information.  It is also not
correct to say that it indicates that the value is "IANA Registered
and has a well defined meaning with underlying semantics" for two
reasons: (1) values in the experimental range are not recorded by
IANA, and (2) there may be undefined values...

It may be too late to remove the I bit -- and simply let the 32-bit
field reflect the value. [The suggestion below doesn't use it.]


[major] "...clear, the value is Locally defined and has semantics
solely within the control of the AS defining that community."

This phrase seems to indicate a willingness for the Wide Community to
not be propagated outside the Context AS, or only be used inside it,
or maybe both.  Should there be a scope indicated with the T and C
bits if this type of Community Value is used?  What should a router do
if the Context ASN doesn't correspond to the local ASN?  Are there
special considerations for confederations?

[This question is related to the filtering question above.]


[major] What should a receiver do if it encounters an unknown Community Value?


[] Suggestion>
   The Community Value indicates what set of actions a router is
   requested to take upon reception of a route containing this
   community.  All new BGP Wide Community Values MUST be defined
   with specific semantics and  validation requirements.

   Some Community Values are reserved for Private or Experimental
   Use (Section 11.5).  When these values are used, the Context AS
   Number provides the namespace in which this Community Value is
   interpreted.  The definition of the semantics and validation
   requirements and their communication are out of scope.

   [Text about scoping and handling of the Context ASN.]

   [Text about handling of unknown values.]



352	4.2.  Source AS Number
...
356	   The Autonomous System number indicates the AS originating this BGP
357	   Wide Community.

[major] Should this field always have a value?  If so, please
Normatively indicate so.  What should a receiver do if the field is
empty (zero)?  Should there be any additional verification -- for
example, comparing the AS_PATH to the Source ASN?



359	4.3.  Context AS Number
...
363	   This identifies the AS that provides the semantics to interpret this
364	   Community.

[major] It is clear from above that this field should be considered
when a Private/Experimental value is used.  But, what about when
registered values are used?  Please indicate what should be done (I
assume ignore, but better to be explicit).


[major] Should the Context and Source ASNs always match?  If locally
defined Community Values are to be used locally, then that should be
the case, right?  When would it be ok for the values to not match?



366	4.4.  BGP Wide Community TLVs
...
382	   Sub-Type:
383	       The sub-type of the BGP Wide Community TLV.  A given Sub-Type
384	       MUST NOT appear more than once.

[nit] These are not sub-TLVs... Just a nit.


[major] What should a receiver do if a given TLV appears more than once?



...
392	4.4.1.  Sub-Type 1, BGP Wide Community Target(s) TLV

[minor] Even if only one Target can be configured, should the name of
this TLV be generalized by using "Targets" (and not "Target(s)"?  It
would make the text clearer.



...
398	   BGP Wide Community Targets define the matching criteria for the
399	   community.  A given wide community may have a number of targets to
400	   which it applies.  The semantics of these targets will vary on a per-
401	   community basis.  Depending on the definition of the community,
402	   targets may be optional.

[nit] s/wide community/Wide Community/g

Most of the text used the capitalized version.  Please be consistent.



404	   Wide Community Targets consist of a series of Atoms that have "match
405	   any" semantics.  Thus, if any given target matches per the semantics
406	   of that Atom for the community, the community is considered to match
407	   and the action defined by the community should be executed.

[nit] s/Wide Community Targets/BGP Wide Community Targets


[minor] It may just be me, but I had to look up what "match any" referred to.



...
411	   If the semantics of a given Atom is undefined for the community in
412	   question, this Atom MUST be ignored.

[major] If reuse is possible, what does it mean for an Atom to be
"undefined for the community"?  Does this sentence require that new
Community Values mention which Atoms are applicable (defined) to them?
 And that if a document defines a new Atom it then has to list all the
applicable Communities?



...
419	4.4.2.  Sub-Type 2, BGP Wide Community Exclude Target(s) TLV

421	   The BGP Wide Community Exclude Target(s) TLV (Sub-Type 2) contains a
422	   list of Atoms.

[minor] A "list of Atoms" seems to be, well, a list of Atoms...instead
of "series of Atom TLVs", as the specification in §4.4.1 says.  Please
be consistent and precise in the specification.



424	   Wide Community Exclude Targets define criteria by which the community
425	   is considered to NOT match.  Depending on the semantics of the BGP
426	   Wide Community, Exclude Target(s) may be optional.

[nit] s/Wide Community Exclude Targets/BGP Wide Community Exclude Targets


[major] "NOT" is not an rfc2119 keyword and will require to be written
in lowercase -- even if the intent is to add emphasis.



...
439	   If the BGP Wide Community Target(s) TLV and the BGP Wide Community
440	   Exclude Target(s) TLV have conflicting semantics, priority MUST be
441	   given to the Wide Community Exclude Target(s) TLV.

[major] "conflicting semantics...priority MUST be given"

I think you may mean "conflicting results" (or something like that) as
the semantics (include vs exclude) are already conflicting.  To be
clear, I mean that the Targets TLV matched on a set of targets, but
the Exclude Targets TLV also matches on the same.  Is that an example
of that you mean?

If so, then it seems to me that the result is to ignore the matches
from the Targets TLV and act (exclude) on the matched from the Exclude
Targets TLV.  I'm sure there must be a succinct way of specifying the
behavior clearly.

All this comes from the fact that "priority" implies (to me) that the
Targets may be considered *after* the Exclude Targets.



...
449	4.4.3.  Sub-Type 3, BGP Wide Community Parameter(s) TLV

451	   The BGP Wide Community Parameter(s) TLV (Sub-Type 3) contains a list
452	   of Atoms.

[] "list of Atoms" -- the mention below of "an ordered set of Atom
sub-TLVs" is a better description.



...
461	   Care must be taken when using Atoms with list semantics.  If the
462	   desired behavior is a single or limited number of instances of that
463	   type, this should be documented as part of the use case of that BGP
464	   Wide Community.

[major] "should be documented"

Other mentions of documentation requirements use MUST, but this one
uses a non-normative statement.  Why the difference?  Should this
mention be Normative too?



...
478	4.4.4.  Usage

480	   The detailed interpretation of the targets or parameters SHALL be
481	   provided when describing given community type in a separate document
482	   or when locally defined by an operator.

[] This section seems to just be a restatement of the requirements
listed above.  Please remove it.



484	5.  BGP Community Container Atoms
...
504	   The Type field contains a value of 1-254.  The values 0 and 255 are
505	   reserved for future use.  The TLV types are to be assigned and
506	   maintained by IANA registry; see Section 11.2.

[major] No need to include any IANA-related text here.



...
512	   Supported format of the TLVs can be:

[minor] These are the defined TLVs, but other may be (eventually)
supported.  In any case, if the Atoms are defined in the following
sections it seems redundant to list them here.

514	     Type  1: Autonomous System Number List.
515	     Type  2: IPv4 Prefix (1 octet prefix length + prefix) List.
516	     Type  3: IPv6 Prefix (1 octet prefix length + prefix) List.

[major] The TLVs are called "IPvx Prefix List" -- no patenthesis for
clarification.



...
526	   In the following sections defining the different Atoms, validation
527	   rules for the Length of the Atom will be presented.  If the Length of
528	   the Atom does not match the rules for that Atom, it SHALL be
529	   considered malformed.  (See Section 8.)

[nit] The first sentence seems to be missing something...

Suggestion>
   The following sections define the Atoms and the validation rules
   for their Length.



531	   In general, Atoms of List type have the semantics of sets.  Duplicate
532	   entries SHOULD NOT be present and MAY be removed by a BGP Speaker
533	   propagating the Lists.  The presence of duplicate entries have no
534	   additional semantics.

[major] "SHOULD NOT be present"

This phrase brings up the question of when is it ok to include
duplicate entries...but I think that the statement should not be
normative.

Suggestion>
   In general, Atoms of List type have the semantics of sets.  The
   presence of duplicate entries have no additional semantics, and
   they MAY be removed by a BGP Speaker propagating the Lists.



536	5.1.  Atom Type 1, The Autonomous System Number List

538	   This Atom represents a list of Autonomous System numbers, each 4
539	   octets in size.  When encoding two-octet ASes, the first two octets
540	   of this four-octet value MUST be filled with zeros.  The minimum
541	   Length of this Atom is 4 octets.  The Length MUST be a multiple of 4.

[major] "When encoding two-octet ASes, the first two octets of this
four-octet value MUST be filled with zeros."

This encoding is already in the base spec (rfc6793 formally Updated
rfc4271) so it doesn't need to be specified here again.  Note also
that the specification of the Source/Context ASN doesn't include this
behavior specification -- it is not needed there either because it is
part of the base spec.



543	   Two special values are reserved for the Autonomous System Atoms:

[major] "values are reserved"

In general, the concept of reserving values carries the impression
that they have been set aside in a registry.  That is not the case
here.

Suggestion>
   When used in an Autonomous System Number List Atom, the following
   values have special meaning:



...
548	5.2.  Atom Types 2 and 3, The IPv4 and IPv6 Prefix Lists

550	   This Atom represents a list of IPv4 or IPv6 prefixes.  IPv4 and IPv6
551	   Prefix Atom values are encoded in the same format used by BGP NLRI in
552	   Section 4.3 of [RFC4271].

[minor] s/This Atom represents/These Atoms represent


[minor] s/IPv4 and IPv6 Prefix Atom/IPv4 and IPv6 Prefix List Atom


[minor] s/encoded in the same format used by BGP NLRI in Section 4.3
of [RFC4271]./encoded as specified for the BGP NLRI in Section 4.3 of
[RFC4271].



...
562	   The Prefix Length for IPv4 prefixes MUST be in the range of 0..32.

564	   The Prefix Length for IPv6 prefixes MUST be in the range of 0..128.

[major] If using the same specification from rfc4271 there's no need
to respecify the behavior here.



566	   The Length field must be able to accommodate the list of prefixes
567	   according to the encoding rules.  If the Length cannot fully
568	   accommodate the required number of octets to encode the Prefix Length
569	   and the Prefix, the Atom SHALL be considered malformed.  (See section
570	   Section 8

[major] "Length cannot fully accommodate the required number of octets"

Without knowing upfront the number of prefixes in the list, how can it
be determined that the Length is wrong?  If just one prefix is
included then there's no issue, but more than one can result in the
next value (or Atom) being overrun/underrun.

The length of the NLRI is not explicitly encoded in an UPDATE, but
rfc4271 offers a way to calculate it.  In this case, a calculation (if
possible) could be used to asses the validity of the Length.



...
589	5.5.  Atom Type 6, The Neighbor Class List

591	   The Neighbor Class list Atom represents a list of Neighbor classes,
592	   each 4 octets in size.  Neighbor class currently can contain three
593	   values:

595	   Peer (1):
596	       This class is typically applied to sessions where a transit-free
597	       relationship exists between two providers.

599	   Customer (2):
600	       This class is typically applied to sessions where the remote end
601	       of the session is operated by a customer.

603	   Upstream (3):
604	       This class is typically applied to sessions where the remote end
605	       of the session is operated by a network from which you receive
606	       transit routes.

[major] RFC9234 defined the concept of a Role, which seems to align
with the Neighbor Classes defined here.  Why wasn't that work reused?



...
611	5.6.  Atom Type 7, The User-defined Class List
...
619	   Examples of User-defined Class properties include geography (East,
620	   West), continent (North America, Asia, Europe), etc.  Similar to the
621	   [RFC1997] BGP Communities, it is necessary that the Context AS
622	   provide a registry of the value and the semantics of a given
623	   community.

[minor] Add: "; how this is done is out of scope"



...
642	6.  Well Known Standard BGP Communities

644	   According to RFC 1997, as well as IANA's Well-Known BGP Communities
645	   registry, the following BGP communities are defined to have global
646	   significance:

648	        0xFFFF0000   planned-shut         [draft-francois-bgp-gshut]
649	        0xFFFFFF01   NO_EXPORT            [RFC1997]
650	        0xFFFFFF02   NO_ADVERTISE         [RFC1997]
651	        0xFFFFFF03   NO_EXPORT_SUBCONFED  [RFC1997]
652	        0xFFFFFF04   NOPEER               [RFC3765]

[major] There are more communities listed in the registry.  Instead of
listing them, a reference to the registry may be more appropriate.


654	   This document recommends for simplicity as well as for avoidance of
655	   backward compatibility issues that the continued use of BGP Standard
656	   Community Path Attribute, type 8, as defined in [RFC1997] and
657	   [RFC3765] to distribute non-Autonomous System specific Well-Known BGP
658	   Communities.

[minor] Where are global vs "non-Autonomous System specific" WKCs
categorized?  Given that the recommendation is not Normative, would it
be ok to extend it to all WKCs?


660	   For the same reason, this document does not intend to obsolete the
661	   currently defined and deployed BGP Extended Communities.

[nit] "document does not intend" -- this document is being written
now, if it didn't obsolete then there's no point in mentioning what it
didn't do.


[] Suggestion>
   A number of Well-Known BGP Communities are documented by IANA
   [Informative-Reference-to-the-Registy].  It is recommended to
   continue using the BGP Community Attribute [RFC1997] for the
   propagation of these Well-Known Communities.



663	7.  Operational Considerations

665	   Having multiple ways to propagate locally assigned BGP Communities -
666	   via the use of Standard, Extended or Large BGP Communities versus the
667	   use of BGP Wide Communities - may seem to potentially cause problems
668	   when considering propagation of conflicting actions.  However, even
669	   at present, an operator may append such Communities with conflicting
670	   information.

[minor] Please add references.


[major] This first paragraph doesn't sound like much operational
guidance -- it reads: "this new container can cause problems, but you
can shoot yourself in the foot already!".

Perhaps, something like this might be better:

Suggestion>
   Multiple mechanisms exist for an operator to propagate actions into the
   network.  Besides BGP Wide Communities, an operator can use the BGP
   Community Attribute [rfc1997], the BGP Extended Communities Attribute
   [rfc4360], or the BGP Large Communities Attribute [rfc8092] to achieve
   similar objectives.  Care should be taken when using more than one of
   these tools.  The interaction between the different community attributes
   is out of the scope of this document.



672	   Implementations SHOULD provide mechanisms to control the order of
673	   processing and manipulation of the varying types of BGP communities.
674	   With such a mechanism, operators will have the ability to control the
675	   outcome of potentially conflicting actions.

[major] What does "manipulation" include?


[major] What about filtering?  I ask because §10.2 says that "the
Community Value the Source AS may provide sufficient context to strip
unwanted or unexpected communities." -- but there is no corresponding
requirement/recommendation for an implementation to provide that
functionality.


[] Perhaps put the sentence about the interaction being out of scope
at the end of the second paragraph.



...
679	8.1.  General Error Handling for BGP Community Containers

681	   [RFC7606] "treat as withdraw" behavior is expected for any malformed
682	   Community Containers or malformation of their contents.

[nit] s/[RFC7606] "treat as withdraw" behavior/The "treat as withdraw"
behavior [RFC7606]


[major] "is expected" -- can we make this Normative?  SHOULD/MUST?



684	   Each Community Container type may have additional validation rules,
685	   including permitted length of Atoms.  Failure to conform to those
686	   additional rules MUST also be treated as a malformed Community
687	   Container.

[] I asked several times about the behavior specified...it wasn't
clear to me that the result would be to treat the container/community
as malformed.  Please indicate which errors should result in the
behavior specified here.



689	8.2.  BGP Wide Community Container Error Handling

691	   If any Atom in a BGP Wide Community container's Exclude Targets TLV
692	   is unrecognized, that Wide Community MUST NOT be considered a match
693	   and no actions for that community should be processed.  While the
694	   Targets TLV is meant to be inclusive, the Exclude Targets TLV is
695	   meant to be proscriptive of applying the action.

[major] §4.4.1 talks about what to do if the Atom is "undefined for
the community", but there's no indication of what to do in the
unrecognized case.  Please add text for that there.


[major] In the Targets TLV case, if an unrecognized Atom is ignored
(?), there may be routes that are not matched -- leaving them without
action.  The action (and not the logic of the match) is what is
important here; for example, a Target Atom may leave out routes that
should have been announced, or announced ones that shouldn't have.
The same result is possible based on the Exclude Targets Atom.



697	9.  Example

[] Nice example!  Consider moving it to an Appendix.



699	9.1.  Example Type 1 Wide Community Definition
...
707	   AS 64496 has established a registered set of values to use for its
708	   User-defined Class:

[nit] s/established a registered set/established a set



...
725	      The Targets TLV MUST contain at least one entry.

[major] This is an example, don't use Normative language: s/MUST/must



727	      The Exclude Targets TLV MAY contain entries of the above supported
728	      Atoms.

[major] This is an example, don't use Normative language: s/MAY/may



...
739	9.2.  Example Type 1 BGP Wide Community Encoding

741	   In this example, the BGP Wide Community defined above is used by a
742	   BGP Speaker to AS_PATH prepend BGP routes containing this community
743	   AS_PATH prepend 4 TIMES when this route is to be distribute to any of
744	   AS 2424, AS 8888, to peers marked as User Class Amsterdam (100) or to
745	   peers marked User Class Moscow (104).  However, such prepending would
746	   not be done to peers that have been configured in the User Class of,
747	   but not to peers of User Class New York (101), regardless of their AS
748	   number.

[minor] s/AS_PATH prepend BGP routes containing this community AS_PATH
prepend 4 TIMES/AS_PATH prepend BGP routes containing this community 4
times


[minor] s/when this route is to be distribute to any of AS 2424, AS
8888, to peers marked as/when advertising to AS 2424 or AS 8888, and
to peers marked as


[minor] s/been configured in the User Class of, but not to peers of
User Class New York (101)/been configured in the User Class New York
(101)



...
753	      0                   1                   2                   3
754	      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
755	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
756	     |         Type (1, Wide)        |    Flags  |0|0|  Reserved(0)  |
757	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
758	     | Length: 53                    |
759	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[] If I counted correctly, the length should be 57.

760	     | Community: LOCAL PREPEND ACTION CATEGORY                    1 |
761	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
762	     | Source AS 64496                                               |
763	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
764	     | Context AS 64496                                              |
765	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
766	     | Target TLV (1)|   Length: 18                  |
767	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[] If I counted correctly, the length should be 22.

768	     | ASN List (1)  |   Length: 8                   |
769	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
770	     | Target ASN# 2424                                              |
771	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
772	     | Target ASN# 8888                                              |
773	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
774	     |  User List(7) |   Length: 8                   |
775	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
776	     | Amsterdam (100)                                               |
777	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
778	     | Moscow (104)                                                  |
779	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
780	     |ExcTargetTLV(2)|   Length: 5                   |
781	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[] If I counted correctly, the length should be 7.

782	     |  User List(7) |   Length: 4                   |
783	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
784	     | New York (101)                                                |
785	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
786	     | Param TLV (3) |   Length: 5                   |
787	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

[] If I counted correctly, the length should be 7.

788	     | Integer32 (4) |   Length: 4                   |
789	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
790	     | Prepend # 4                                                   |
791	     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+



...
796	10.1.  BGP Community Container Security Considerations

798	   Transitive BGP Community Container communities could unintentionally
799	   spread far from their origin.  If a router receives many routes from
800	   multiple sources on the Internet with different communities, it could
801	   cause significant memory usage.  To prevent excessive memory usage,
802	   routers should be configured to strip unexpected communities from
803	   received routes.

[] The "far from their origin" doesn't seem to directly result in
"cause significant memory usage".  Maybe a better justification may be
the potential impact (in terms of actions) that could result from
receiving and parsing.


[major] "routers should be configured to strip unexpected communities"

This sounds like a good place to be Normative: MUST/SHOULD?  I think
that RECOMMENDED might be best.

Also, given that we're talking about the Container, there may be other
types which are not communities.  Please generalize.



805	   All the security considerations for BGP Communities [RFC1997], BGP
806	   Extended Communities [RFC4360], and BGP Large Communities [RFC8092]
807	   apply to BGP Community Containers.

[minor] s/All the security considerations/The security considerations


[minor] Put this paragraph first.



[] Other work that used UTF-8 has been required to include text like this:

   This document uses UTF-8 encoding for the UTF-8 String Atom.
   There are a number of security issues with Unicode. Implementers
   and operators are advised to review Unicode Technical Report #36
   [UTR36] to learn about these issues. UTF-8 "Shortest Form" encoding
   is REQUIRED to guard against the technical issues outlined in [UTR36].

An Informative reference to UTR36 should be added as follows:

   [UTR36]  Davis, M., Ed. and M. Suignard, Ed., "Unicode Security
            Considerations", Unicode Technical Report #36, August
            2010, <http://unicode.org/reports/tr36/>.



809	10.2.  BGP Wide Community Security Considerations

811	   For BGP Wide Communities, the Community Value the Source AS may
812	   provide sufficient context to strip unwanted or unexpected
813	   communities.

[minor] s/Community Value the Source AS/Community Value and the Source AS Number


[] Stripping communities ("removing" may be a better word) was also
mentioned in the previous section.  Repeating the recommendation made
there would be appropriate.



815	   Given the flexibility and power offered by BGP Wide communities, it
816	   is important to consider the additional possibilities allowed by
817	   their definition.  In particular, for locally defined BGP Wide
818	   Communities, it may be wise to restrict the range of parameters.  For
819	   registered BGP Wide Communities, the security considerations of the
820	   document defining them MUST address issues specific to those newly
821	   defined Communities.

[minor] "it may be wise"

Can we turn this into a Normative recommendation?



823	11.  IANA Considerations

[major] This section requests the creation of new registries, but it
doesn't indicate where they should be located.  Other
community-related documents have created a separate group (from the
general bgp-parameters group [1]), for example: [2] and [3].

I suggest that you request a new group here too.  You can then make a
general statement that all the new requested registries should be
located there (instead of doing so for each individual one).

Also, please indicate that all the Registration Policies are per
rfc8126, and add a Normative reference to it.

[1] https://www.iana.org/assignments/bgp-parameters/
[2] https://www.iana.org/assignments/bgp-well-known-communities/
[3] https://www.iana.org/assignments/bgp-extended-communities/



...
832	11.2.  BGP Community Container Atoms Types
...
838	   Registration procedures:

840	            0x00: Reserved.
841	       0x01-0x08: Defined in this document.
842	       0x09-0xFE: IETF Review.
843	            0xFF: Reserved.

[minor] Please number all the tables in this section.


[major] This table, and others in this section,  mixes the
registration policies and the assignments.  Please create separate
tables.  The table indicating the Registration Policies should
indicate the whole range:  0x01-0xFE: IETF Review.



...
848	       Name                             Type Value
849	       ----                             ----------
850	       Autonomous System Number List      0x01
851	       IPv4 Prefix list                   0x02
852	       IPv6 Prefix list                   0x03
853	       Unsigned Integer32 list            0x04
854	       IEEE Floating Point Number list    0x05
855	       Neighbor Class list                0x06
856	       User-defined Class list            0x07
857	       UTF-8 string                       0x08

[major] This table, and others in this section, should have an extra
column for references.  For the values defined here, use "[this
document]".



...
881	11.4.  BGP Community Container Types
...
890	                      0x0000 : Reserved.
891	                      0x0001 : BGP Wide Community (defined in this
892	                               document).
893	               0x0002-0x0004 : Reserved.
894	               0x0005-0x00FF : IETF Review.
895	               0x0100-0xFF00 : First Come, First Served.
896	               0xFF01-0xFFFE : Experimental.
897	                      0xFFFF : Reserved.

[] s/First Come, First Served/First Come First Served


[] s/Experimental/Experimental Use


[major] This table mixes the registration policies and the
assignments.  Please create separate tables.

The table indicating the Registration Policies should indicate the
whole range:  0x0001-0x00FF: IETF Review.

A different table should then include any allocations made in this
document, including a column that indicates the reference (to "[this
document]").  In this case 0x0001-0x0004.  BTW, why are 0x0002-0x0004
reserved?  Should they be marked as deprecated?



899	11.5.  Registered Type 1 BGP Wide Communities Community Types

[major] §4.1 calls this field "Community Value".



...
907	                  0x00000000 : Reserved.
908	       0x00000001-0x7FFFFFFF : Available for private/local use.
909	                  0x80000000 : Reserved.
910	       0x80000001-0xFFFFFEFF : First Come, First Served for
911	                               registered use.
912	       0xFFFFFF00-0xFFFFFFFE : Experimental.
913	                  0xFFFFFFFF : Reserved.

[major] The Registration Policies, per rfc8126, should be:

"Available for private/local use." = Private Use

"First Come, First Served for registered use" = First Come First Served


[] Note that Private Use and Experimental Use are similar in that IANA
doesn't keep track of them and there's no restriction on their use.
This means that, unless specified in this document (§4.1), a Private
Use Community Values cannot be constrained to local use.

OTOH, there's an opportunity to clarify the expectations of an
experiment.  From rfc8126:

   4.2.  Experimental Use

   Experimental Use is similar to Private Use, but with the purpose
   being to facilitate experimentation.  See [RFC3692] for details.
   IANA does not record assignments from registries or ranges with this
   policy (and therefore there is no need for IANA to review them) and
   assignments are not generally useful for broad interoperability.
   Unless the registry explicitly allows it, it is not appropriate for
   documents to select explicit values from registries or ranges with
   this policy.  Specific experiments will select a value to use during
   the experiment.

   When code points are set aside for Experimental Use, it's important
   to make clear any expected restrictions on experimental scope.  For
   example, say whether it's acceptable to run experiments using those
   code points over the open Internet or whether such experiments should
   be confined to more closed environments.  See [RFC6994] for an
   example of such considerations.



915	11.6.  Registered Type 1 BGP Wide Community Optional Sub-Types
...
931	       Name                             Type Value
932	       ----                             ----------
933	       Targets                              1
934	       Exclude Targets                      2
935	       Parameters                           3

[major] The specification in §4.4.* uses an extended name: BGP Wide
Community Target(s) TLV, for example.  Please be consistent.

[EoR -08]