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

Robert Raszuk <robert@raszuk.net> Thu, 02 March 2023 00:10 UTC

Return-Path: <robert@raszuk.net>
X-Original-To: idr@ietfa.amsl.com
Delivered-To: idr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 6B2BDC151AF5 for <idr@ietfa.amsl.com>; Wed, 1 Mar 2023 16:10:31 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -7.096
X-Spam-Level:
X-Spam-Status: No, score=-7.096 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_HI=-5, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_DBL_BLOCKED_OPENDNS=0.001, URIBL_ZEN_BLOCKED_OPENDNS=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=raszuk.net
Received: from mail.ietf.org ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 3dqqEaefjFPr for <idr@ietfa.amsl.com>; Wed, 1 Mar 2023 16:10:26 -0800 (PST)
Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 8065CC14CE25 for <idr@ietf.org>; Wed, 1 Mar 2023 16:10:26 -0800 (PST)
Received: by mail-wr1-x42e.google.com with SMTP id bx12so11791309wrb.11 for <idr@ietf.org>; Wed, 01 Mar 2023 16:10:26 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raszuk.net; s=google; t=1677715825; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=+bVhLhxlI2C3bvIg0JIBXRQSN6fs7IboiirYETSP3x4=; b=HNGwfxmxR8u4xqbIV+aDEFNpC2vV2nDQ4MfSyV21WXEbaxjJf9VQF/4HWIJor+4ruE uDhSet96RX3KndScM8uz8zL6UiAdiuku9I/eJjfUxFV1fqAztgAbNLaHCT6VMxbycqv5 +dZ8RE0qoMPVSgkDk7vYxOsXvgAn8XSzlUP+Y9TV64v3iqcGHSjcCG2+dBChmsgiSHYL WLhIShxgiKQZWxGS0ACv6cw3M6gthh9jrrllPSr7oXt7ntPMpo1uEWXj125V9lovId7S Rym8H8E/itTHAAf+20y9WRQW64xKuz9/n1XNF1xCC3yCD3H73+YzFAFL5g7bWnRrOD3D goFg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1677715825; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=+bVhLhxlI2C3bvIg0JIBXRQSN6fs7IboiirYETSP3x4=; b=T+dNgzacucZ4WM97aqB9f310nY290giP70krT8ztrh4i6asZ5SpuUjYEUHKWTj4tOE PUQhPsBGd1/QGTDBW+huAUTMQp0v1wV05oAy9M1m3FXJ7yLAhV83FhIqrtnRhwiAdjsA FaJtUzRb/D6fvRuS71H2m4NlLiZxGTwrX7NU+yI8q6DArMF/isQnBVzx5vAfKOqFk/7b aY0R3n9E1f9HmDfnU/8//ny2xPH43fhd+0viBfwno08tpnIQ3lTrDJe3bbTyJsRCDEiT pODxeZfdBUw6nVPkgEMWgoO+NkfcwSwNe+3TXMvLsajkZ0XmRnB7gSjKgnyt3ELhEuKQ T5Jw==
X-Gm-Message-State: AO0yUKWRKvXez8OwrLFurGc/BKCzIsD9DG4oUFqxLPZRdYeUjJutJII4 AX+nX/o0mbqJJTHoVV/oL84CKclejIyXNwCDOfrJeg==
X-Google-Smtp-Source: AK7set84hll03a88RuTmwS8FaHigHAMXWVP8YSJk5OHGdSBT15CO/BEoMMoii3Oq6aMPnZ2WjFV5aJMFhRg8+zTnKSU=
X-Received: by 2002:a5d:5583:0:b0:2c5:4e07:7987 with SMTP id i3-20020a5d5583000000b002c54e077987mr1417776wrv.6.1677715824562; Wed, 01 Mar 2023 16:10:24 -0800 (PST)
MIME-Version: 1.0
References: <CAMMESsw_kC_V-7px1xNbgxsinWW0RFUrugDmu2ziF8ioxqxCRQ@mail.gmail.com>
In-Reply-To: <CAMMESsw_kC_V-7px1xNbgxsinWW0RFUrugDmu2ziF8ioxqxCRQ@mail.gmail.com>
From: Robert Raszuk <robert@raszuk.net>
Date: Thu, 02 Mar 2023 01:10:13 +0100
Message-ID: <CAOj+MMHUwaOLJudD9XKd1-d-MmQQxV-uNCxqOgF42sh6P7B-cw@mail.gmail.com>
To: Alvaro Retana <aretana.ietf@gmail.com>
Cc: draft-ietf-idr-wide-bgp-communities@ietf.org, Keyur Patel <keyur@arrcus.com>, idr-chairs@ietf.org, idr@ietf.org
Content-Type: multipart/alternative; boundary="000000000000c1f3a005f5dfa725"
Archived-At: <https://mailarchive.ietf.org/arch/msg/idr/Qyc6pTb_IY6iTuDma86b3dWGFpg>
Subject: Re: [Idr] AD Review of draft-ietf-idr-wide-bgp-communities-08
X-BeenThere: idr@ietf.org
X-Mailman-Version: 2.1.39
Precedence: list
List-Id: Inter-Domain Routing <idr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/idr>, <mailto:idr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/idr/>
List-Post: <mailto:idr@ietf.org>
List-Help: <mailto:idr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/idr>, <mailto:idr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 02 Mar 2023 00:10:31 -0000

*Part 2 ... Resuming when left yesterday: *


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

*Removed. *


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

*True. Reworded as: *

*"Supported formats of the TLVs defined in this document are:"*


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

*Fixed.*

...
> 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.
>
>
*I reworded it as follows: *


*"The following sections define the Atoms and their validation rules for
contained data within provided 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.
>

*Fixed as suggested. *


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

*Replaced with: *


*"This Atom represents a list of four octet Autonomous System numbers. The
minimum Length of this Atom is 4 octets. The Length MUST be a multiple of
4." *



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

*Fixed. *


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

*Both fixed. *


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

*Fixed. *

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

*RFC4271 does not say it verbatim however you are right that specifying it
here is not *
*much needed. Removed - (commented out). *


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

*I think the draft here leads to reader's confusion between Container
Length vs Atom Length. *

*I am rewording it as follows: *





*The Container's Length field must be able to accommodate the list of
prefixesaccording to the encoding rules. If the Container's Length cannot
fully accommodate the required number of octets to encode the Atom's Prefix
Length and the actual Prefix, the Atom is considered malformed.*


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

*Well that RFC did not exist when this draft was written. *

*I am hesitant to replace it here with definitions from RFC9234. *

*First it defines "Peer" as "ASes are Peers" - looks like not very helpful
:) *

*It lacks Upstream definition. *

*Defines "Customer" using local AS which to many means something completely
different in BGP then what is intended. *

*I will leave current text for now subject to subsequent fix. *

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

*Added. *

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

*Fixed. *


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

*Yes indeed this is exactly how I modified the text to fix the above. *



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

*:) Removed. *

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

*Added. *

*Sorry - I do not know how to encode ref to the registry :( *

*https://www.xmlhack.com/read.php_item=913
<https://www.xmlhack.com/read.php_item=913> is not of huge help. *



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

*Done. *

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

*Replaced as suggested. *

*However your src text was quite different from the latest version which
read: *

*"However, even at present, an operator may append such Communities with *
*conflicting information.  It is therefore recommended that any
implementation, *
*in supporting both standard and BGP Wide Communities, allow for their
easy *
*inbound and outbound processing. The actual execution of all communities *
*should be treated as a union and, if supported by an implementation,
their *

*execution permissions are to be a local configuration matter."*

*I commented it out so we can always come back to it easily. *

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

*Fixed. *


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

*Switched to 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.
>

*The rules can be specified as the local definition(s) and can different *
*on a per container/atom basis. *

*I think it would be pretty hard to enumerate them in the document. *


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

*That section §4.4.1 contains text: *


*"If the semantics of a given Atom is undefined this Atom MUST be ignored."*

*Is "undefined" different from "unrecognized" ? Assuming so **I am adding
there to **§4.4.1*

*s/undefined/undefined or unrecognized/*



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

*True. But I think we need to rely on the operator's awareness of what is
supported and *
*activated on their nodes within Context AS. *


697     9.  Example
>
> [] Nice example!  Consider moving it to an Appendix.
>


*Apologies - I tried but failed. :( *



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

*Fixed. *

...
> 725           The Targets TLV MUST contain at least one entry.
>
> [major] This is an example, don't use Normative language: s/MUST/must
>

*Fixed. *


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
>

*Fixed. *

...
> 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)
>
>
*IDR github version does not have this text. Instead it just says: *

*AS_PATH prepend 4 TIMES TO AS 2424, AS 8888, to peers marked as *

*Amsterdam (100) or to peers marked Moscow (104), but not to peers in New
York (101).¶
<https://author-tools.ietf.org/api/export/2935dc13-fd0e-4a9b-871d-da260aabac08/draft-ietf-idr-wide-bgp-communities.html#section-9.2-1>*

*The T Flag (transitive) is set to prevent propagation of this community.*

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

*Well as we moved to now include header it seems it should be 63 (with no*
*header it looks like 59). There was a mistake in the drawing which may
have *
*looked like two more octets)*


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

*True - Fixed. *

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

*Yes - Fixed. *


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

*Again true ! Fixed. *


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

*Reworded. *


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

*Fixed. *


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

*But this security consideration talks about what is defined in this
document. *
*I am not sure how we could generalize this here. *

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
>

*Fixed. *


> [minor] Put this paragraph first.
>

*Done. *


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

*Added. *


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

*No idea how to encode it in xml. *


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

*Fixed. *

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

*I think the previous text is generalized and covers special case of Wide
Communities. *

*So adding it again here may be redundant and perhaps even confusing. *


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

*Reworded to "RECOMMENDED"*

823     11.  IANA Considerations
>
> [major] This section requests the creation of new registries, but it
> doesn't indicate where they should be located.


*Well for BGP Community Container Attribute it says: *
*Registry Name: BGP Path Attributes*



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

*I think the current IANA section was already reworked and does not seem *
*to contain the issues listed. I will leave it for now subject to re-edit
when needed. *



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

*Added. *



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


*I checked few other RFCs and do not see table/artwork numbers. *



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

*Isn't this added automatically by IANA ?  In any case added. *

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

*Fixed. *

[] s/Experimental/Experimental Use
>

*Fixed.*


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

*Fixed. *


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

*Fixed. *


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

*Fixed.*



> 899     11.5.  Registered Type 1 BGP Wide Communities Community Types
>
> [major] §4.1 calls this field "Community Value".
>

*Fixed. Actually used "Container Value" across*

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

*Fixed. *

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

*Right - I think the distinction which was made in this document was *
*more of operational nature - for example easy of filtering of given value
ranges. *


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

*Pls see above justification for different classification. *

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

*Fixed. *


>
> [EoR -08]
>

Will be posting a new version soon.

Still remember about checking Warren's review against this latest version.

Kind regards,
Robert