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.*
- [Idr] AD Review of draft-ietf-idr-wide-bgp-commun… Alvaro Retana
- Re: [Idr] AD Review of draft-ietf-idr-wide-bgp-co… Robert Raszuk
- Re: [Idr] AD Review of draft-ietf-idr-wide-bgp-co… Alvaro Retana
- Re: [Idr] AD Review of draft-ietf-idr-wide-bgp-co… Robert Raszuk
- Re: [Idr] AD Review of draft-ietf-idr-wide-bgp-co… Alvaro Retana
- Re: [Idr] AD Review of draft-ietf-idr-wide-bgp-co… Robert Raszuk
- Re: [Idr] AD Review of draft-ietf-idr-wide-bgp-co… Robert Raszuk
- [Idr] draft-ietf-idr-wide-bgp-communities-11 Robert Raszuk
- Re: [Idr] AD Review of draft-ietf-idr-wide-bgp-co… Alvaro Retana