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

Robert Raszuk <robert@raszuk.net> Wed, 01 March 2023 00:35 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 7328AC151B05 for <idr@ietfa.amsl.com>; Tue, 28 Feb 2023 16:35:35 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.095
X-Spam-Level:
X-Spam-Status: No, score=-2.095 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_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001, URIBL_DBL_BLOCKED_OPENDNS=0.001, URIBL_ZEN_BLOCKED_OPENDNS=0.001] autolearn=unavailable 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 bsnQ3Ng3HRgl for <idr@ietfa.amsl.com>; Tue, 28 Feb 2023 16:35:30 -0800 (PST)
Received: from mail-wm1-x32f.google.com (mail-wm1-x32f.google.com [IPv6:2a00:1450:4864:20::32f]) (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 9DF62C15152B for <idr@ietf.org>; Tue, 28 Feb 2023 16:35:30 -0800 (PST)
Received: by mail-wm1-x32f.google.com with SMTP id m14-20020a7bce0e000000b003e00c739ce4so6899922wmc.5 for <idr@ietf.org>; Tue, 28 Feb 2023 16:35:30 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raszuk.net; s=google; t=1677630928; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=erLPTfkTPNm+QeLV5mH1/v1YOU2F9yS4ZgmncHzJQbc=; b=Sx77ZU4t48WAVFeXCo8vTEhqrNZ3OXdV7Ki8hDPJ1xsjqMEoXsZjLKrYoy0IDT1ASQ gyZo6L16T5NB13mK6/f+tj/yOx5P/ScCvPC1iyxo59kuTAWtBJZW2vLTlhZ9qNihPyU+ O9Tatlt9LU9QhS6OMm7j4ZPtLbiPsokKm+RaeRAwpwQ9pm6b9Gt4PQyNu2F1QLFIjf6N L8q2YPfQST8l4i1HeTkQpjYdXED7Vk7CFXl60jsyQCvejPIolL7PUWFNTV5+k0aEULOR FW/bgVtt15H7xUwhbgFSW3bgmXUmv5dVxP01ZLBcB6raO8ZCx50H74zE+0gXVCKgFe5y HQ0Q==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1677630928; 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=erLPTfkTPNm+QeLV5mH1/v1YOU2F9yS4ZgmncHzJQbc=; b=TbURJ4ctsEB71+5CeWXQPFtHat+bb0hSy6NIPNaqF9xFwO2P+s/A1tqsneRUrfttyJ Z5foFpV/2bpL1sUz7/A0mS0NIlZVn4T3rx3HIqvnwTutuWyMKldNG6uxmRerA2aEvId4 YssBUWEl1FC/0+8iLzD950eRPWn2XWungIi+YMJ8TAVYFpVeibaAkPuTloRTDR2Ow6/X 7XY2tPCSKAB30JWdduS2+FwsU85Z0WMHvbgzeoaJ4A03ZY5xBlmJR3Dw+3d0qJxiR34R oeANGrQpfR77WKO+KTe55b8OXe+9YrCJJopW+Ww5SpeLFgdDChCf7iWrrIrur3vY8mIS N+sg==
X-Gm-Message-State: AO0yUKX6OnLn8FufsRDTBMnO4epnjpKp3pcT/kousX+MX1N/W8fCacN1 Oxn93Uviq7sErotWsMS9vVHQYzXhuFdS5JoXCgP5lw==
X-Google-Smtp-Source: AK7set+7Sa5EM1lQwpyVvOwADI1HrdL1lQmw0n3iZobEwxXqgrtFq8zymaFCXI3KE/5Lbr6OVlVKmVvL5t80ewlOI3E=
X-Received: by 2002:a05:600c:358f:b0:3df:97ed:dde9 with SMTP id p15-20020a05600c358f00b003df97eddde9mr1341548wmq.6.1677630928033; Tue, 28 Feb 2023 16:35:28 -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: Wed, 01 Mar 2023 01:35:16 +0100
Message-ID: <CAOj+MMG6AqcJZbAL57ynoj4bXg=JfSTgUPPx8nbaCBd=DVHY6Q@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="00000000000087af6805f5cbe307"
Archived-At: <https://mailarchive.ietf.org/arch/msg/idr/55tfw83Zkj0zxqcmzool8yRvDxY>
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: Wed, 01 Mar 2023 00:35:35 -0000

*Hi Alvaro,*

*Huge THANK YOU for an excellent review !!!!*

*Part 1 of 2 below. Part 2 will be done tomorrow. *

*Please see inline - highlighted ... *

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

*Ack. *


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

*I already responded to this one so I hope my clarification here is
helpful. *

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

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

*I recall discussing some of comments with Warren and incorporating some of
them in one of the doc versions. Will review again. I am not sure if what
is posted on IDR github is actually the latest version.*

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

*Current version of the draft has 6 co-authors. *

*This draft took many years and all authors actually contributed to the
work. So unless someone volunteers I am in no position to even ask anyone
to shift to the Contributor's section. I would like to ask for an exception
to allow all 6 co-authors to be published as authors. *

*As a reference I see recently published RFCs in SPRING with more then 5
co-authors. Note that we have already moved 4 authors to the contributor's
section. *

*But if this is going to be the only showstopper to publish this work I
will move myself to the Contributor's or Ack 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.
>


*Replaced - Thank YOU !*



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

*Replaced. *

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

*Fixed. *

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

*Fixed. *



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

*Hmm the IDR github show this text here: *






*<t>When defining any new type of tool there is always aunique opportunity
to specify a subset of well recognizedbehaviors. Lists of the current most
commonly used BGP communities,as well as provision for a new registry for
future definitionswill be contained in a separate document.</t>*

*which I believe is true. This is also related to your (2) comment at the
top I believe. *


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

*Fixed (moved explanation of fields to section 3.1). *

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

*Done.*

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

*Fixed.*

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

*It should be subject to RFC7606 "treat as withdraw" procedure. Updating
section *
*8.1 to say this explicitly. *


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

*There is no restriction on the number of Community Containers in BGP *
*Community Container Attribute. *

*Each container (on a per container granularity) is also subject to *
*selective propagation rules. *

*Section 3 contains this text already which IMHO describes it pretty well: *

*"The attribute contains a set of typed containers. Any given container *
*type may appear multiple times, unless that container type's *
*definition specifies otherwise."*

I added the below to further clarify this:

*"Each such container can have a unique propagation scope and can be *
*subject to local filtering."*



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

*Correct. Now as we moved the description from section 2 here I think it
is *
*even more clear just looking at the text. *


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

*And I believe after the reorg and de-duplication of the fields you have
suggested *
*now it is exactly like this. *

*Indeed it looks much cleaner and more readable. *



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

*After some time digesting it I don't think that IANA should manage it.
Reason being is that *
*new definitions require much more **than new IANA type number and perhaps
new bit(s). *

*It requires a new draft/RFC. And the definition of those bits would be per
Type (some bits *
*would not necessarily need to be legal in other types) so the new doc
could extend the 3..7 bits. *


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

*Yes indeed. I limited the definition of T bit to administrative boundary. *

*Moreover I moved the definition of the administrative boundary to *
*new section 2.4 as it applies to the entire document. *


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

*Done as above. *


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

*Done*

[minor] Please add a Normative reference to rfc3065.
>

*Done*


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

*Fixed. *


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

*Well to the best of my recollection we always mean to describe the
behaviour *
*of exiting the container's originator Member-AS. *

*I added explicit text in all occurrences which limits C-bit to Member-AS *
*boundaries only. *


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

*That got fixed automatically by defining T bit to only control behaviour
across *
*administrative boundary not an AS. *

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


*I am inclined to reword it to say that the length includes the common
header. *
*I have fixed it that way: *

*"The Length field represents the total length (in octets) of a given
container*
*including its header."*


> ...
> 329     4.1.  Community Value
>
> 331        Community Value: 4 octets
>
> [major] §11.5 uses "Community Type".
>


*Type is referring to a community container. *

*Each community container can have a lot of communities - each of different
value. *

*11.5 is a typo. Fixed. It should describe a "container values" not
"community types"*

*We have 4 levels of nesting going on here :) *

*Attribute *
*    |*
*    --------- Container Type*
*                            | *
*                            ------------ Community Value *
*                                                        |*
*                                                        --------------
TLVs with ATOMs*

*/* The original spec was much more flat. But during WG work it ended up
like this. */*



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


*Well it actually does. Notice the ranges: *


*0x00000001-0x7FFFFFFF *

*vs *

*80000001-0xFFFFFEFF*

*And that is the crux of it. *


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


*We are talking about FCFS for registered use. Experimental here would *
*be also registered. I recognize that even for experiment we could use
early *
*allocation model from 80000001-0xFFFFFEFF range but this is just leaving *
*extra space in IANA for some new category more like EFT vs experimental. *

*Note that split of the specs into protocol encoding (this one) and actual *
*definitions draft-ietf-idr-registered-wide-bgp-communities made this as *
*separate documents hence likely generated some confusion. *


> and (2) there may be undefined values...
>

*Not sure what you mean. Undefined should not have I bit set. *


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

*I would/will keep it in the light of the above explanation and fixes. *

*IMO this is very important (if not key) to the entire proposal. *



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


*Yes. But that handling is defined as action. Note that Context AS can *
*be N AS-es away from the sender/originating AS. *

*We do have examples in the companion draft where such behavior *
*is encoded. *


> Should there be a scope indicated with the T and C
> bits if this type of Community Value is used?


*Nope. Reason being that this is far beyond T or C bits jurisdiction. *

What should a router do
> if the Context ASN doesn't correspond to the local ASN?


*That's actually quite expected. It should do exactly as *
*the spec says .. :) *

*Propagate if T/C bits allow and execute when Context *
*AS matches. *


> Are there special considerations for confederations?
>

*I believe this would be orthogonal. If something is defined to be *
*recognized within Member-AS or across Memeber-ASes such *
*encoding will indicate so. *


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

*Right. I added clarification about filtering above. *


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

*Ignore it. *

*I added an explicit sentence (below) to highlight it into section 4.1: *


*"When Community Value is not recognized by the BGP speaker it should
ignore it while maintaining propagation rules as specified by T/C bits.""*



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

*In the light of the above explanation I am not merging this one as it
would *
*not fit with the intention of the overall document. *

   [Text about handling of unknown values.]
>

*Added before. *


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?


*Hmm ... I am under the opinion that if we do not mark any field in the
protocol *
*encoding spec as **option it is mandatory. *

*Should we keep repeating at each field that it is mandatory ? *


> If so, please Normatively indicate so.


*I am happy to do that however I have not seen this in any other documents
that much. *


> What should a receiver do if the field is
> empty (zero)?


*Impossible :) *


> Should there be any additional verification -- for
> example, comparing the AS_PATH to the Source ASN?
>

*Interesting suggestion (ala flow spec validation), but I am not sure about
it here. *


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

*The intention is that we always have it and we always act on given
community *
*container if it matches local AS. *

*BGP wide communities were (at least originally) invented and described *
*as Internet Wide tool. *

[major] Should the Context and Source ASNs always match?


*No - BIG NO*


> If locally
> defined Community Values are to be used locally, then that should be
> the case, right?


*Yes exactly. *


> When would it be ok for the values to not match?
>

*When action is expected to take place N autonomous systems away.  See for *
*local actions we already have bunch of tools including automated policy
insertion. *

*I do believe that this spec would be useful far beyond the local scope. *

*Of course being aware that peers could drop it at will ... *


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

*Not sure I get it. *

*If BGP Community Container, Type 1: BGP Wide Community defines a space
for *
*TLVs and one of the TLV is BGP Wide Community TLVs it will contain a
number *
*of sub-TLVs. *

*Again this spec uses nesting like really complex Recursive Russian Dolls. *

*But this is what WG decided on - not my own idea :(  *


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

*I would say treat as withdraw. Added this case to the Error Handling
section. *

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

*Fixed. *

*Likewise done the same in other TLV titiles. *


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

*Fixed *


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

*ok. *


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

*Fixed.*


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

*Not sure what to do here :( *


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


*Fixed. Good catch ! ATOMs do not intend to have per community semantics. *

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

*Fixed. *

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
>

*Fixed.*

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

*Fixed.*


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

*I reworded this to say: *




*"If the BGP Wide Community Target(s) TLV and the BGP Wide Community
ExcludeTarget(s) TLV result in conflicting actions, priority MUST be given
to the WideCommunity Exclude Target(s) TLV."*

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

*Fixed*

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

*Fixed (into MUST)*

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

*Gone. *

*End of part 1. *

*Rgs,*
*R.*