Re: [Idr] Yangdoctors early review of draft-ietf-idr-bgp-model-09

Mahesh Jethanandani <mjethanandani@gmail.com> Tue, 27 October 2020 22:05 UTC

Return-Path: <mjethanandani@gmail.com>
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 F345E3A1000; Tue, 27 Oct 2020 15:05:16 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.097
X-Spam-Level:
X-Spam-Status: No, score=-2.097 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, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GK7QtNg4xEjE; Tue, 27 Oct 2020 15:05:10 -0700 (PDT)
Received: from mail-pf1-x432.google.com (mail-pf1-x432.google.com [IPv6:2607:f8b0:4864:20::432]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 92AD23A0FD7; Tue, 27 Oct 2020 15:05:10 -0700 (PDT)
Received: by mail-pf1-x432.google.com with SMTP id j18so1737105pfa.0; Tue, 27 Oct 2020 15:05:10 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=OIsemGOY8vRA4pcMdMhE4hgdQs9cThbqqU/yZBuyCdU=; b=ohazxY9CboDBK3p7AAeNdti7ZvI2ZekIkoWyMIPrR9x5pXu1wD2lDdVh4XtbhkWEiT vB+5H7ktFYrbNIBm3sUqVzkXQYT9rbkNRX0DuOZQ967HdFvTFjUx49DZ1uSKNau4aRrD bAnwc1WXbYLYQn8N5ZG1zCpiuloEcZs3TltOCN4QqHizzio6nIsbgTJEKdsObm2YmoVr DA4yXmJ1/0oEFm07HAW1pLhSu9oFoPga8eBCYfxhJJ8xKFPGS4RjZ+1VjiJbl6XrW2jV UKKuF5lGkhhPlyTcX2lhNcJtk+1hVC0bQyJA0wZlCLUJ1yr5LWcyPZqIEQ1hYNp3/f/0 kqsA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=OIsemGOY8vRA4pcMdMhE4hgdQs9cThbqqU/yZBuyCdU=; b=oQviptFsEBJV+qLxL9b0va9SxyKhoOa1c2X3dj0XtYcGTM1DOF1ZCclWRta/EhGk+j i01VH2FzPIVDPeoA4BxhOgr6R6FPf/vGwULNaFX0v7LeCztOYidjmktpZBovipWHME1H NVbcZhLEIMLTXyFS5tE3xaI7VreTmTET+ynfLw4f8N1JIyRjAGJwIuxls6xDlrpOQRPE 10pnvd+fgb5DsDq3biGqC9emCCbkA6kadZJS368T3/uI5QBhrq7MIl2zG3KQ80P0lAEW IfUrdkiSbUALTxBlGgSC6Xp+5CrT+/58SSuYdUyIsdJ1191JQmkHZoAbj/kRrBVkPG7r JGjw==
X-Gm-Message-State: AOAM533oybx8eeH6o+0Wwa43IVnmjrHjh0l90XSqR+N5hDb3TgEM+0wM Ul8gQVVMub1LR99eR+1UB3k=
X-Google-Smtp-Source: ABdhPJyBPRkQdoq7SnLjxOr8boxrMhrmjDGifz+Zy8dDLAxBSsF85s8oMt6HrLHPhZxcYNePkrpQdA==
X-Received: by 2002:a63:1903:: with SMTP id z3mr3500972pgl.449.1603836309731; Tue, 27 Oct 2020 15:05:09 -0700 (PDT)
Received: from ?IPv6:2601:647:5600:5020:84da:f66b:9a88:6cb8? ([2601:647:5600:5020:84da:f66b:9a88:6cb8]) by smtp.gmail.com with ESMTPSA id c1sm3507053pfo.207.2020.10.27.15.05.07 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 27 Oct 2020 15:05:08 -0700 (PDT)
From: Mahesh Jethanandani <mjethanandani@gmail.com>
Message-Id: <CFC9CFFA-2933-4A59-A6BB-BBA90D65EC16@gmail.com>
Content-Type: multipart/alternative; boundary="Apple-Mail=_5CDACB1B-FC68-4977-9EE3-1CC3D9854F6D"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Date: Tue, 27 Oct 2020 15:05:06 -0700
In-Reply-To: <159752099321.32075.3042069364754449151@ietfa.amsl.com>
Cc: yang-doctors@ietf.org, idr@ietf.org, draft-ietf-idr-bgp-model.all@ietf.org
To: Acee Lindem <acee@cisco.com>
References: <159752099321.32075.3042069364754449151@ietfa.amsl.com>
X-Mailer: Apple Mail (2.3608.120.23.2.4)
Archived-At: <https://mailarchive.ietf.org/arch/msg/idr/bF4NGeGXD6wRYrdJ8UkczCk3v9M>
Subject: Re: [Idr] Yangdoctors early review of draft-ietf-idr-bgp-model-09
X-BeenThere: idr@ietf.org
X-Mailman-Version: 2.1.29
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: Tue, 27 Oct 2020 22:05:17 -0000

Hi Acee,

Thanks first of all for a detailed review. I have to apologize for the delay in responding. This was a large review, and while I will try to address most of the comments, there are a few that I have deferred to my co-authors on the draft.

Also, this is part 1 of the response to the review. I will address the remaining comments and update the draft at the same time in a subsequent response.

> On Aug 15, 2020, at 12:49 PM, Acee Lindem via Datatracker <noreply@ietf.org> wrote:
> 
> Reviewer: Acee Lindem
> Review result: On the Right Track
> 
> Document: draft-ietf-idr-bgp-model-09.txt
> Reviewer: Acee Lindem
> Review Date: Aug 15, 2020
> Review Type: Working Group Last Call
> Intended Status: Standards Track
> Summary: On the Right Track - Major Comments need to be addressed.
> 
> Modules: ietf-bgp@2020-06-28.yang
>         ietf-bgp-types@2020-06-28.yang
> 	 ietf-bgp-policy@2020-06-28.yang
> 	 Plus a plethora of sub-modules 
> 
> Tech Summary: The document contains the base configuration and operational
>              YANG model for the BGP protocol. The basic structure is good
> 	      but the major comments need to be addressed.
> 
> Major Comments:
> 
>     1. The draft claims to support a number of AFs in section 2.1. However,
>        the only AFs that are complete are ipv4-unicast and ipv6-unicast. The
> 	draft shouldn't claim to fully support these AFs and they should be
> 	left to subsequent augmentations. There are already BESS models to
> 	cover many of thes AFI/SAFI combinations.

Ok. I will update the comment to say that the model supports ipv4-unicast and ipv6-unicast and defers the remaining AFI/SAFIs to other drafts.

>     2. The RIB sub-module is split into seven separate sub-module. This makes
>        it virtually incomprehesible. At least some of the sub-modules have
> 	very little content. I would suggest these sub-modules be collasped
> 	into a single sub-module - ietf-bgp-rib.

I can see that some of the sub-modules can be collapsed, but collapsing them into one sub-module is going to make them equally incomprehensible. For now, I will collapse just the smaller sub-modules.

>     3. I know the discussion sub-modules in general is ongoing. However, the
>        current state of the tools with respect to sub-modules makes the
> 	validation useless. Since YANG 1.0 and 1.1 both support sub-modules,
> 	I think the tools need to be fixed ASAP.

I would agree.

>     4. The Security Considerations section is just the template with no
>        discussion of the sutrees and data node vulnerabilities.

Ok.

>     5. Add complete tree diagrams in appendicies. 	

Good idea.

> 
> 
> Minor Comments:
> 
>     1. General - Punctuation of descriptions is inconsistent. I'd consistently
>        use punctuation for anything that isn't just replication of the YANG
> 	identifier (which isn't a good description anyway).
>     2. Page 20, "container distance": I believe that "leaf local" has been
>        ommitted.

Good catch.

>     3. Page 25, "case ipsec": What is the reference for usage of transport
>        mode IPsec with BGP? Please elaborate in description.

I see release notes from vendors on IPsec for BGP, but no references that I can cite. Did you have something specific in mind?

>     4. Page 27, "supported-capabilities": The statement "BGP capabilities
>        negotiated as supported by peer" mean? Shoudln't this just be
> 	"Negotiated BGP capabilities"? Shouldn't the leaf-list be named
> 	"negotiated-capabilities" rather than "supported-capabilities"?

Updated the leaf-list to “negotiated-capabilities” and updated the description accordingly.

>     5. Page 29, "remote-helper": I'd support this to be renamed
>        "graceful-restart-only" since "helper-only" is relative to the local
> 	peer.

The enumeration definitions talk about the mode in terms of local and remote. Calling it ‘graceful-restart-only’ conveys a completely different meaning.

>     6. Page 30, "fsm-established-transitions": What is the difference with
>        "established-transitions"? Is the description a cut-n-paste error?

I will rename “established-transitions” to “peer-fsm-established-transitions”.

>     7. Page 33, "backward-transition": This implies any backward transition.
>        I'd suggest renaming to "established-state-lost"
> 	or "established-backward-transition".

But backward-transition is defined as when the BGP new status is lower than the last one, not just from established -> idle.

>     8. Page 34, "clear-at": Is there a way to say to process the clear
>        immediately?

One can, by setting the time to current time.

>     9. Page 41, "mtu-discovery": Please add a reference. Also, shouldn't the
>        default be true?

Ok. Changed the default to true, and added reference to RFC 1191.

>    10. Page 42, "auth-password": Add a reference to RFC 2385.

Done.

>    11. Page 62, "leaf enabled": What is more specific than per neighbor
>        configuration?

Changed the description to just say "Whether the use of multiple paths for the same NLRI is enabled for the neighbor.”

>    12. Page 65, "module description": I don't think usage is restricted to
>        BGP policy. There could be other reasons to import ietf-bgp-types.

Dropped the word ‘policy’.

>    13. Page 70, "no-peer": It would be good to include the standard
>        community value in the description like the others.

Added the following statement.  "This community has a value of 0xFFFFFF04.”

>    14. Page 71, "bgp-session-direction": Why uppercase for the values?

Thanks for that catch.

>    15. Page 72, "bgp-ext-community-type": Many of these string patterns are
>        defined in RFC 8294. Also, it would be good to add the syntax of the
> 	pattern you are representing in the description.

I will let my co-authors to comment on this.

>    16. Page 74, "peer-type", "description": RFC 4271 uses EBGP and IBGP rather
>        than eBGP and iBGP.

Fixed.

>    17. Page 74, "REMOVE_PRIVATE_AS_OPTION": Why is this all uppercase?

Fixed.

>    18. Page 75, "percentage": This type is RFC 8294.

Removed the typedef and reference the definition in the RFC.

>    19. Page 77, "module ietf-bgp-policy": This is missing the stardard
>        RFC 8407 module template.

Added it.

>    20. Page 78, "bgp-set-med-type": Please describe the syntax of the pattern
>        in the description.

I will defer this to my co-authors.

>    21. Page 79, "reference": Remove the "WG needs to decide...". We are making
>        routing policy a standard in these models.

Done.

>    22. Page 80, "member": Why is this a string? Is this a regular expression?
>        Please describe the syntax in the description. If regular expressions, add
> 	a reference.

Again, a question for my co-authors to address.

>    23. Page 81, "set-community-action-common": This seems like a terribly
>        complex way to model this. Why didn't you just use a YANG choice?
>    24. Page 82, "next-hop-in": This is an inconsistent name. I'd suggest
>        "next-hop-list-eq".
>    25. Page 82, "afi-safi-in": Policies are normally applied at the AFI/SAFI
>        level. By putting a match condition for these, you are adding a
> 	second way of doing this and adding considerable complexity to debugging
> 	policies.
>    26. Page 82, "community-count": This container is empty and, even if it
>        weren't, what possible use-case would there be to match on the number
> 	of communities as opposed to specific communities?
>    27. Page 82, "as-path-length": This container is also empty. The use cases
>        for matching on AS Path Length are well-known and should include ge/le
> 	matching.
>    28. Page 84, "set-local-pref": The description indicates this is limited
>        to route updates. This should apply to route attributes as well.
>    29. Page 84, "set-med":  The description indicates this is limited
>        to route updates. This should apply to route attributes as well.
>    30. Pages 84-85, "set-community": This could be modeled with a YANG
>        choice rather than method/when statements.
>    31. Pages 85-86, "set-ext-community": This could be modeled with a YANG
>        choice rather than method/when statements.
>    32. Pages 94-95, "afi-saifs": As I stated in my major comment, only
>        ipv4-unicast and ipv6-unicast are fully specified.
>    33. Pages 95-96, "ietf-bgp-rib-ext": Since this model only has a single
>        grouping, it doesn't seem like a very useful functional division.
>    34. Page 107, Remove all the empty groups. They are useless.
> 	
> 
> 
> Nits:
> 
>    1. Update all the copyrights to 2020.
> 
> *** draft-ietf-idr-bgp-model-09.txt.orig	2020-08-11 09:21:07.000000000 -0400
> --- draft-ietf-idr-bgp-model-09.txt	2020-08-15 15:31:20.000000000 -0400
> ***************
> *** 20,26 ****
> 
>     This document defines a YANG data model for configuring and managing
>     BGP, including protocol, policy, and operational aspects, such as
> !    RIB, based on data center, carrier and content provider operational
>     requirements.
> 
>  Status of This Memo
> --- 20,26 ----
> 
>     This document defines a YANG data model for configuring and managing
>     BGP, including protocol, policy, and operational aspects, such as
> !    RIB, based on data center, carrier, and content provider operational
>     requirements.
> 
>  Status of This Memo
> ***************
> *** 119,125 ****
>     This document describes a YANG [RFC7950] data model for the BGP-4
>     [RFC4271] protocol, including various protocol extensions, policy
>     configuration, as well as defining key operational state data,
> !    including Routing Information Base (RIB).  The model is intended to
>     be vendor-neutral, in order to allow operators to manage BGP
>     configuration in heterogeneous environments with routers supplied by
>     multiple vendors.  The model is also intended to be readily mapped to
> --- 119,125 ----
>     This document describes a YANG [RFC7950] data model for the BGP-4
>     [RFC4271] protocol, including various protocol extensions, policy
>     configuration, as well as defining key operational state data,
> !    including a BGP Routing Information Base (RIB).  The model is intended to
>     be vendor-neutral, in order to allow operators to manage BGP
>     configuration in heterogeneous environments with routers supplied by
>     multiple vendors.  The model is also intended to be readily mapped to
> ***************
> *** 146,158 ****
>     addresses policy configuration, by providing "hooks" for applying
>     policies, and also defining BGP-specific policy features.  The BGP
>     policy features are intended to be used with the general routing
> !    policy model defined in A YANG Data Model for Routing Policy
> !    Management [I-D.ietf-rtgwg-policy-model].
> 
>     The model conforms to the NMDA [RFC8342] architecture.  It has
>     support for securing BGP sessions using TCP-AO [RFC5925] or TCP-MD5,
>     and for configuring Bidirectional Forward Detection (BFD) [RFC5880]
> !    for fast next hop liveliness check.
> 
>     For the base BGP features, the focus of the model described in this
>     document is on providing configuration and operational state
> --- 146,158 ----
>     addresses policy configuration, by providing "hooks" for applying
>     policies, and also defining BGP-specific policy features.  The BGP
>     policy features are intended to be used with the general routing
> !    policy model defined in "A YANG Data Model for Routing Policy
> !    Management" [I-D.ietf-rtgwg-policy-model].
> 
>     The model conforms to the NMDA [RFC8342] architecture.  It has
>     support for securing BGP sessions using TCP-AO [RFC5925] or TCP-MD5,
>     and for configuring Bidirectional Forward Detection (BFD) [RFC5880]
> !    for fast next-hop liveliness checking.
> 
>     For the base BGP features, the focus of the model described in this
>     document is on providing configuration and operational state
> ***************
> *** 177,183 ****
>        that relate to a neighbor - controlling the import and export of
>        NLRIs.
> 
> !    o  RIB contents.
> 
>     As mentioned earlier, any configuration items that are deemed to be
>     widely available in existing major BGP implementations are included
> --- 177,183 ----
>        that relate to a neighbor - controlling the import and export of
>        NLRIs.
> 
> !    o  BGP RIB contents.
> 
>     As mentioned earlier, any configuration items that are deemed to be
>     widely available in existing major BGP implementations are included
> ***************
> *** 210,220 ****
> 
>     2020-06-28 with the actual date of the publication of this document.
> 
> !    RFC ZZZZ, where ZZZZ is the number assigned to A YANG Data Model for
> !    Routing Policy Management [I-D.ietf-rtgwg-policy-model].
> 
> !    RFC BBBB, where BBBB is the number assigned to YANG Data Model for
> !    Bidirectional Forward Detection [I-D.ietf-bfd-yang].
> 
> 
> 
> --- 210,220 ----
> 
>     2020-06-28 with the actual date of the publication of this document.
> 
> !    RFC ZZZZ, where ZZZZ is the number assigned to "A YANG Data Model for
> !    Routing Policy Management" [I-D.ietf-rtgwg-policy-model].
> 
> !    RFC BBBB, where BBBB is the number assigned to "YANG Data Model for
> !    Bidirectional Forward Detection" [I-D.ietf-bfd-yang].
> 
> 
> 
> ***************
> *** 283,289 ****
> 
> 
>     o  policy configuration -- hooks for application of the policies
> !       defined in A YANG Data Model for Routing Policy Management
>        [I-D.ietf-rtgwg-policy-model] that act on routes sent (received)
>        to (from) peers or other routing protocols and BGP-specific policy
>        features.
> --- 283,289 ----
> 
> 
>     o  policy configuration -- hooks for application of the policies
> !       defined in "A YANG Data Model for Routing Policy Management"
>        [I-D.ietf-rtgwg-policy-model] that act on routes sent (received)
>        to (from) peers or other routing protocols and BGP-specific policy
>        features.
> ***************
> *** 293,299 ****
> 
>     These modules also make use of standard Internet types, such as IP
>     addresses and prefixes, autonomous system numbers, etc., defined in
> !    Common YANG Data Types [RFC6991].
> 
>  2.1.  BGP protocol configuration
> 
> --- 293,299 ----
> 
>     These modules also make use of standard Internet types, such as IP
>     addresses and prefixes, autonomous system numbers, etc., defined in
> !    "Common YANG Data Types" [RFC6991].
> 
>  2.1.  BGP protocol configuration
> 
> ***************
> *** 374,383 ****
>     Users may specify configuration at a higher level and have it apply
>     to all lower-level items, or provide overriding configuration at a
>     lower level of the hierarchy.  Overriding configuration items are
> !    optional, with neighbor specific configuration being the most
>     specific or lowest level, followed by peer-group, and finally global.
>     Global configuration options reflect a subset of the peer-group or
> !    neighbor specific configuration options which are relevant to the
>     entire BGP instance.
> 
>     The model makes the simplifying assumption that most of the
> --- 374,383 ----
>     Users may specify configuration at a higher level and have it apply
>     to all lower-level items, or provide overriding configuration at a
>     lower level of the hierarchy.  Overriding configuration items are
> !    optional, with neighbor-specific configuration being the most
>     specific or lowest level, followed by peer-group, and finally global.
>     Global configuration options reflect a subset of the peer-group or
> !    neighbor-specific configuration options which are relevant to the
>     entire BGP instance.
> 
>     The model makes the simplifying assumption that most of the
> ***************
> *** 406,412 ****
>     Address-family configuration is made available in multiple points
>     within the model - primarily within the global container, where
>     instance-wide configuration can be set (for example, global protocol
> !    parameters, the BGP best path route selection options, or global
>     policies relating to the address-family); and on a per-neighbor or
>     per-peer-group basis, where address-families can be enabled or
>     disabled, and policy associated with the parent entity applied.
> --- 406,412 ----
>     Address-family configuration is made available in multiple points
>     within the model - primarily within the global container, where
>     instance-wide configuration can be set (for example, global protocol
> !    parameters, the BGP best-path route selection options, or global
>     policies relating to the address-family); and on a per-neighbor or
>     per-peer-group basis, where address-families can be enabled or
>     disabled, and policy associated with the parent entity applied.
> ***************
> *** 480,487 ****
>  2.2.  Policy configuration overview
> 
>     The BGP policy configuration model augments the generic YANG routing
> !    policy model described in A YANG Data Model for Routing Policy
> !    Management [I-D.ietf-rtgwg-policy-model], which represents a
>     condition-action policy framework for routing.  This model adds BGP-
>     specific conditions (e.g., matching on the community attribute), and
>     actions (e.g., setting local preference) to the generic policy
> --- 480,487 ----
>  2.2.  Policy configuration overview
> 
>     The BGP policy configuration model augments the generic YANG routing
> !    policy model described in "A YANG Data Model for Routing Policy
> !    Management" [I-D.ietf-rtgwg-policy-model], which represents a
>     condition-action policy framework for routing.  This model adds BGP-
>     specific conditions (e.g., matching on the community attribute), and
>     actions (e.g., setting local preference) to the generic policy
> ***************
> *** 535,541 ****
>     The RIB data model represents the BGP RIB contents.  The model
>     supports five logical RIBs per address family.
> 
> !    A abridged version of the tree shows the RIB portion of the tree
>     diagram.
> 
> 
> --- 535,541 ----
>     The RIB data model represents the BGP RIB contents.  The model
>     supports five logical RIBs per address family.
> 
> !    An abridged version of the tree shows the RIB portion of the tree
>     diagram.
> 
> 
> ***************
> *** 652,667 ****
> 
>     The adj-rib-out-post table is a per-neighbor table containing routes
>     eligible for sending (advertising) to the neighbor after output
> !    policy rules have been applied
> 
>  3.  Relation to other YANG data models
> 
> !    The BGP model augments the Routing Management model A YANG Data Model
> !    for Routing Management [RFC8349] which defines the notion of routing,
>     routing protocols, and RIBs.  The notion of Virtual Routing and
> !    Forwarding (VRF) is derived by using the YANG Schema Mount [RFC8528]
> !    to mount the Routing Management module under the YANG Data Model for
> !    Network Instances [RFC8529].
> 
> 
> 
> --- 652,667 ----
> 
>     The adj-rib-out-post table is a per-neighbor table containing routes
>     eligible for sending (advertising) to the neighbor after output
> !    policy rules have been applied.
> 
>  3.  Relation to other YANG data models
> 
> !    The BGP model augments the Routing Management model "A YANG Data Model
> !    for Routing Management" [RFC8349] which defines the notion of routing,
>     routing protocols, and RIBs.  The notion of Virtual Routing and
> !    Forwarding (VRF) is derived by using the "YANG Schema Mount" [RFC8528]
> !    to mount the ietf-routing module under the "YANG Data Model for
> !    Network Instances" [RFC8529].
> 
> 
> 
> ***************
> *** 732,739 ****
> 
>  5.1.  URI Registration
> 
> !    in the IETF XML registry [RFC3688] [RFC3688].  Following the format
> !    in RFC 3688, the following registration is requested to be made:
> 
>     URI: urn:ietf:params:xml:ns:yang:ietf-bgp
>     URI: urn:ietf:params:xml:ns:yang:ietf-bgp-policy
> --- 732,739 ----
> 
>  5.1.  URI Registration
> 
> !    Following the format in RFC 3688, the following registrations are requested
> !    in the IETF XML registry [RFC3688]:
> 
>     URI: urn:ietf:params:xml:ns:yang:ietf-bgp
>     URI: urn:ietf:params:xml:ns:yang:ietf-bgp-policy
> ***************
> *** 744,750 ****
> 
>  5.2.  YANG Module Name Registration
> 
> !    This document registers three YANG module in the YANG Module Names
>     registry YANG [RFC6020].
> 
>     name: ietf-bgp
> --- 744,750 ----
> 
>  5.2.  YANG Module Name Registration
> 
> !    This document registers three YANG modules in the YANG Module Names
>     registry YANG [RFC6020].
> 
>     name: ietf-bgp
> ***************
> *** 816,822 ****
>     The YANG model can be subdivided between the main module for base
>     items, types, policy data, and the RIB module.  It references BGP
>     Communities Attribute [RFC1997], Route Refresh Capability for BGP-4
> !    [RFC2918], , NOPEER Community for BGP [RFC3765], BGP/MPLS IP Virtual
>     Private Networks (VPNs) [RFC4364], BGP-MPLS IP Virtual Private
>     Network (VPN) Extension for IPv6 VPN [RFC4659], Graceful Restart
>     Mechanism for BGP [RFC4724], Multiprotocol Extenstions for BGP-4
> --- 816,822 ----
>     The YANG model can be subdivided between the main module for base
>     items, types, policy data, and the RIB module.  It references BGP
>     Communities Attribute [RFC1997], Route Refresh Capability for BGP-4
> !    [RFC2918], NOPEER Community for BGP [RFC3765], BGP/MPLS IP Virtual
>     Private Networks (VPNs) [RFC4364], BGP-MPLS IP Virtual Private
>     Network (VPN) Extension for IPv6 VPN [RFC4659], Graceful Restart
>     Mechanism for BGP [RFC4724], Multiprotocol Extenstions for BGP-4
> ***************
> *** 1080,1086 ****
>          }
>          container distance {
>            description
> !             "Administrative distance (or preference) assigned to
>               routes received from different sources
>               (external, internal, and local).";
>            leaf external {
> --- 1080,1086 ----
>          }
>          container distance {
>            description
> !             "Administrative distances (or preferences) assigned to
>               routes received from different sources
>               (external, internal, and local).";
>            leaf external {
> ***************
> *** 1148,1154 ****
>              key "afi-safi-name";
>              description
>                "AFI,SAFI configuration available for the
> !                neighbour or group";
>              uses mp-afi-safi-config;
>              uses state;
>              container graceful-restart {
> --- 1148,1154 ----
>              key "afi-safi-name";
>              description
>                "AFI,SAFI configuration available for the
> !                neighbor or group";
>              uses mp-afi-safi-config;
>              uses state;
>              container graceful-restart {
> ***************
> *** 1237,1243 ****
>              description
>                "The BGP Identifier of this entry's BGP peer.
>                 This entry MUST be 0.0.0.0 unless the
> !                sessionstate is in the openconfirm or the
>                 established state.";
>              reference
>                "RFC 4271, Section 4.2, 'BGP Identifier'.";
> --- 1237,1243 ----
>              description
>                "The BGP Identifier of this entry's BGP peer.
>                 This entry MUST be 0.0.0.0 unless the
> !                session state is in the openconfirm or the
>                 established state.";
>              reference
>                "RFC 4271, Section 4.2, 'BGP Identifier'.";
> ***************
> *** 1560,1566 ****
>                description
>                  "This flag indicates whether the remote neighbor is
>                   currently in the process of restarting, and hence
> !                  received routes are currently stale";
>              }
> 
> 
> --- 1560,1566 ----
>                description
>                  "This flag indicates whether the remote neighbor is
>                   currently in the process of restarting, and hence
> !                  received routes are currently stale.";
>              }
> 
> 
> ***************
> *** 1575,1583 ****
>                config false;
>                description
>                  "This flag indicates whether the local neighbor is
> !                  currently restarting. The flag is unset after all NLRI
>                   have been advertised to the peer, and the End-of-RIB
> !                  (EOR) marker has been unset";
>              }
>              leaf mode {
>                type enumeration {
> --- 1575,1583 ----
>                config false;
>                description
>                  "This flag indicates whether the local neighbor is
> !                  currently restarting. The flag is cleared after all NLRI
>                   have been advertised to the peer, and the End-of-RIB
> !                  (EOR) marker has been cleared.";
>              }
>              leaf mode {
>                type enumeration {
> ***************
> *** 1780,1787 ****
>                "The last error code and subcode seen by this
>                 peer on this connection.  If no error has
>                 occurred, this field is zero.  Otherwise, the
> !                first byte of this two byte OCTET STRING
> !                contains the error code, and the second byte
>                 contains the subcode.";
>              reference
>                "RFC 4271, Section 4.5.";
> --- 1780,1787 ----
>                "The last error code and subcode seen by this
>                 peer on this connection.  If no error has
>                 occurred, this field is zero.  Otherwise, the
> !                first octet of this two octet OCTET STRING
> !                contains the error code, and the second octet
>                 contains the subcode.";
>              reference
>                "RFC 4271, Section 4.5.";
> ***************
> *** 1814,1821 ****
>                path "../../neighbor/remote-address";
>              }
>              description
> !               "IP address of the neighbor that went away from
> !                established state.";
>            }
>            leaf last-error {
>              type leafref {
> --- 1814,1821 ----
>                path "../../neighbor/remote-address";
>              }
>              description
> !               "IP address of the neighbor that fell from
> !                established state."
>            }
>            leaf last-error {
>              type leafref {
> ***************
> *** 1963,1969 ****
> 
> 
>          multiple contexts within the BGP module. That is to say that
> !         they may be application to a subset of global, peer-group or
>          neighbor contexts.
> 
>          Copyright (c) 2019 IETF Trust and the persons identified as
> --- 1963,1969 ----
> 
> 
>          multiple contexts within the BGP module. That is to say that
> !         they may be applicable to a subset of global, peer-group, or
>          neighbor contexts.
> 
>          Copyright (c) 2019 IETF Trust and the persons identified as
> ***************
> *** 2065,2071 ****
>              this timer is 30 seconds.;
> 
>              The actual time interval for the KEEPALIVE messages is
> !             indicated by operational value of keepalive. That value
> 
> 
> 
> --- 2065,2071 ----
>              this timer is 30 seconds.;
> 
>              The actual time interval for the KEEPALIVE messages is
> !             indicated by operational value of keepalive. The value
> 
> 
> 
> ***************
> *** 2202,2209 ****
>             default "16.0";
>             description
>               "This is the upper limit of the instability metric. This
> !                      value must be greater than the larger of 1 and
> !                      suppress-above.";
>           }
>           leaf reach-decay {
>             type yang:gauge32;
> --- 2202,2209 ----
>             default "16.0";
>             description
>               "This is the upper limit of the instability metric. This
> !               value must be greater than the larger of 1 and
> ! 	      suppress-above.";
>           }
>           leaf reach-decay {
>             type yang:gauge32;
> ***************
> *** 2345,2351 ****
>             "An upper-bound on the time that stale routes will be
>              retained by a router after a session is restarted. If an
>              End-of-RIB (EOR) marker is received prior to this timer
> !             expiring stale-routes will be flushed upon its receipt - if
> 
> 
> 
> --- 2345,2351 ----
>             "An upper-bound on the time that stale routes will be
>              retained by a router after a session is restarted. If an
>              End-of-RIB (EOR) marker is received prior to this timer
> !             expiring, stale-routes will be flushed upon its receipt - if
> 
> 
> 
> ***************
> *** 2452,2458 ****
>             description
>               "Ignore the AS path length when selecting the best path.
>                The default is to use the AS path length and prefer paths
> !               with shorter length.";
>           }
>           leaf external-compare-router-id {
>             type boolean;
> --- 2452,2458 ----
>             description
>               "Ignore the AS path length when selecting the best path.
>                The default is to use the AS path length and prefer paths
> !               with a shorter length.";
>           }
>           leaf external-compare-router-id {
>             type boolean;
> ***************
> *** 2502,2508 ****
>             default "false";
>             description
>               "Flag to enable sending/receiving of MED metric attribute
> !                      in routing updates.";
>           }
>         }
>       }
> --- 2502,2508 ----
>             default "false";
>             description
>               "Flag to enable sending/receiving of MED metric attribute
> !               in routing updates.";
>           }
>         }
>       }
> ***************
> *** 2638,2644 ****
>           default "false";
>           description
>             "This leaf indicates whether the IPv4 Unicast AFI,SAFI is
> !             enabled for the neighbour or group";
>         }
>       }
> 
> --- 2638,2644 ----
>           default "false";
>           description
>             "This leaf indicates whether the IPv4 Unicast AFI,SAFI is
> !             enabled for the neighbor or group";
>         }
>       }
> 
> ***************
> *** 2811,2823 ****
>             type uint32;
>             description
>               "Maximum number of prefixes that will be accepted from the
> !               neighbour";
>           }
>           leaf shutdown-threshold-pct {
>             type bt:percentage;
>             description
>               "Threshold on number of prefixes that can be received from
> !               a neighbour before generation of warning messages or log
>                entries. Expressed as a percentage of max-prefixes";
>           }
>           leaf restart-timer {
> --- 2811,2823 ----
>             type uint32;
>             description
>               "Maximum number of prefixes that will be accepted from the
> !               neighbor";
>           }
>           leaf shutdown-threshold-pct {
>             type bt:percentage;
>             description
>               "Threshold on number of prefixes that can be received from
> !               a neighbor before generation of warning messages or log
>                entries. Expressed as a percentage of max-prefixes";
>           }
>           leaf restart-timer {
> ***************
> *** 2842,2848 ****
>           type boolean;
>           default "false";
>           description
> !            "If set to true, send the default-route to the neighbour(s)";
>         }
>       }
> 
> --- 2842,2848 ----
>           type boolean;
>           default "false";
>           description
> !            "If set to true, send the default-route to the neighbor(s)";
>         }
>       }
> 
> ***************
> *** 2987,2995 ****
>             "Configure logging of peer state changes.  Default is to
>              enable logging of peer state changes.
> 
> !             Note: Documenting out of ESTABLISHED state is desirable,
> !                   but documenting all backward transitions is
> !                   problematic, and should be avoided.";
>         }
>       }
>     }
> --- 2987,2995 ----
>             "Configure logging of peer state changes.  Default is to
>              enable logging of peer state changes.
> 
> !             Note: Documenting demotion from ESTABLISHED state is
> ! 	          desirable, but documenting all backward transitions
> ! 		  is problematic, and should be avoided.";
>         }
>       }
>     }
> ***************
> *** 3001,3012 ****
>          groups";
>       container ebgp-multihop {
>         description
> !          "eBGP multi-hop parameters for the BGPgroup";
>         leaf enabled {
>           type boolean;
>           default "false";
>           description
> !            "When enabled the referenced group or neighbors are
>              permitted to be indirectly connected - including cases
>              where the TTL can be decremented between the BGP peers";
>         }
> --- 3001,3012 ----
>          groups";
>       container ebgp-multihop {
>         description
> !          "eBGP multi-hop parameters for the BGP peer-group";
>         leaf enabled {
>           type boolean;
>           default "false";
>           description
> !            "When enabled, the referenced group or neighbors are
>              permitted to be indirectly connected - including cases
>              where the TTL can be decremented between the BGP peers";
>         }
> ***************
> *** 3035,3041 ****
>          groups";
>       container route-reflector {
>         description
> !          "Route reflector parameters for the BGPgroup";
>         reference
>           "RFC 4456: BGP Route Reflection.";
>         leaf route-reflector-cluster-id {
> --- 3035,3041 ----
>          groups";
>       container route-reflector {
>         description
> !          "Route reflector parameters for the BGP peer-group";
>         reference
>           "RFC 4456: BGP Route Reflection.";
>         leaf route-reflector-cluster-id {
> ***************
> *** 3256,3262 ****
>          key "afi-safi-name";
>          description
>            "AFI, SAFI configuration available for the
> !            neighbour or group";
>          uses mp-afi-safi-config;
>          container graceful-restart {
>            if-feature "bt:graceful-restart";
> --- 3256,3262 ----
>          key "afi-safi-name";
>          description
>            "AFI, SAFI configuration available for the
> !            neighbor or group";
>          uses mp-afi-safi-config;
>          container graceful-restart {
>            if-feature "bt:graceful-restart";
> ***************
> *** 3272,3278 ****
> 
>      grouping bgp-peer-group-base {
>        description
> !         "Parameters related to a BGP group.";
>        leaf peer-group-name {
>          type string;
>          description
> --- 3272,3278 ----
> 
>      grouping bgp-peer-group-base {
>        description
> !         "Parameters related to a BGP peer-group.";
>        leaf peer-group-name {
>          type string;
>          description
> ***************
> *** 3312,3318 ****
>        container afi-safis {
>          description
>            "Per-address-family configuration parameters associated with
> !            the group.";
>          uses bgp-peer-group-afi-safi-list;
>        }
>      }
> --- 3312,3318 ----
>        container afi-safis {
>          description
>            "Per-address-family configuration parameters associated with
> !            the BGP peer-group.";
>          uses bgp-peer-group-afi-safi-list;
>        }
>      }
> ***************
> *** 3512,3518 ****
>          container prefixes {
>            config false;
>            description
> !             "Prefix counters for the BGP session";
>            leaf received {
>              type uint32;
>              description
> --- 3512,3518 ----
>          container prefixes {
>            config false;
>            description
> !             "Prefix counters for for the AFI/SAFI in this BGP session";
>            leaf received {
>              type uint32;
>              description
> ***************
> *** 3600,3608 ****
>                  Jeffrey Haas (jhaas at pfrc.org).";
>      description
>        "This module contains general data definitions for use in BGP
> !        policy. It can be imported by modules that make use of BGP
> !        attributes";
> ! 
>      revision 2020-06-28 {
>        description
>          "Initial Version";
> --- 3600,3608 ----
>                  Jeffrey Haas (jhaas at pfrc.org).";
>      description
>        "This module contains general data definitions for use in BGP
> !        other BGP modules and modules requiring BGP types. It can be
> !        imported by modules that make use of BGP attributes";
> !        
>      revision 2020-06-28 {
>        description
>          "Initial Version";
> ***************
> *** 3700,3706 ****
> 
>          "Multi-protocol extensions to BGP";
>        reference
> !         "RFC 4760: Multiprotocol Extenstions for BGP-4.";
>      }
> 
>      identity route-refresh {
> --- 3700,3706 ----
> 
>          "Multi-protocol extensions to BGP";
>        reference
> !         "RFC 4760: Multiprotocol Extensions for BGP-4.";
>      }
> 
>      identity route-refresh {
> ***************
> *** 3846,3852 ****
>      identity bgp-well-known-std-community {
>        description
>          "Base identity for reserved communities within the standard
> !          community space defined by RFC1997. These communities must
>           fall within the range 0xFFFF0000 to 0xFFFFFFFF";
>        reference
>          "RFC 1997: BGP Communities Attribute.";
> --- 3846,3852 ----
>      identity bgp-well-known-std-community {
>        description
>          "Base identity for reserved communities within the standard
> !          community space defined by RFC 1997. These communities must
>           fall within the range 0xFFFF0000 to 0xFFFFFFFF";
>        reference
>          "RFC 1997: BGP Communities Attribute.";
> ***************
> *** 3856,3863 ****
>        base bgp-well-known-std-community;
>        description
>          "Do not export NLRI received carrying this community outside
> !          the bounds of this autonomous system, or this confederation if
> !          the local autonomous system is a confederation member AS. This
> 
> 
> 
> --- 3856,3863 ----
>        base bgp-well-known-std-community;
>        description
>          "Do not export NLRI received carrying this community outside
> !          the bounds of this autonomous system, or this confederation (if
> !          the local autonomous system is a confederation member AS). This
> 
> 
> 
> ***************
> *** 3895,3901 ****
>        base bgp-well-known-std-community;
>        description
>          "An autonomous system receiving NLRI tagged with this community
> !          is advised not to re-advertise the NLRI to external bi-lateral
>           peer autonomous systems. An AS may also filter received NLRI
>           from bilateral peer sessions when they are tagged with this
>           community value";
> --- 3895,3901 ----
>        base bgp-well-known-std-community;
>        description
>          "An autonomous system receiving NLRI tagged with this community
> !          is advised not to re-advertise the NLRI to external bilateral
>           peer autonomous systems. An AS may also filter received NLRI
>           from bilateral peer sessions when they are tagged with this
>           community value";
> ***************
> *** 4128,4134 ****
>        }
>        description
>          "Labels a peer or peer group as explicitly internal,
> !          external or confederation.";
>      }
> 
>      identity REMOVE_PRIVATE_AS_OPTION {
> --- 4128,4134 ----
>        }
>        description
>          "Labels a peer or peer group as explicitly internal,
> !          external, or confederation.";
>      }
> 
>      identity REMOVE_PRIVATE_AS_OPTION {
> ***************
> *** 4546,4551 ****
> --- 4546,4552 ----
>              update";
>         }
>         leaf-list afi-safi-in {
> +       I guess it will be for inheritance? Also, would name differently. 
>           type identityref {
>             base bt:afi-safi-type;
>           }
> ***************
> *** 4961,4969 ****
>            type boolean;
>            description
>              "BGP attribute indicating that the prefix is an atomic
> !              aggregate; i.e., the peer selected a less specific
>               route without selecting a more specific route that is
> !              included in it.";
>            reference
>              "RFC 4271: Section 5.1.6.";
>          }
> --- 4962,4970 ----
>            type boolean;
>            description
>              "BGP attribute indicating that the prefix is an atomic
> !              aggregate, i.e., the peer selected a less specific
>               route without selecting a more specific route that is
> !              subsumed by it.";
>            reference
>              "RFC 4271: Section 5.1.6.";
>          }
> ***************
> *** 4987,4994 ****
> 
> 
>            description
> !             "BGP multi-exit discriminator attribute used in BGP route
> !              selection process";
>            reference
>              "RFC 4271: Section 5.1.4.";
>          }
> --- 4988,4995 ----
> 
> 
>            description
> !             "BGP multi-exit discriminator attribute used in the BGP
> ! 	     route selection process";
>            reference
>              "RFC 4271: Section 5.1.4.";
>          }
> ***************
> *** 5064,5070 ****
>              type inet:ipv4-address;
>              description
>                "IP address of the router that performed the
> !                  aggregation.";
>            }
>          }
>          container as-path {
> --- 5065,5071 ----
>              type inet:ipv4-address;
>              description
>                "IP address of the router that performed the
> !                aggregation.";
>            }
>          }
>          container as-path {
> ***************
> *** 5256,5262 ****
>                }
>                description
>                  "Routing tables for IPv4 unicast -- active when the
> !                        afi-safi name is ipv4-unicast";
>                uses ipv4-loc-rib;
> 
> 
> --- 5257,5263 ----
>                }
>                description
>                  "Routing tables for IPv4 unicast -- active when the
> !                  afi-safi name is ipv4-unicast";
>                uses ipv4-loc-rib;
> 
> 
> ***************
> *** 5437,5443 ****
>       identity invalid-route-reason {
>         description
>           "Base identity for reason code for routes that are rejected as
> !           invalid.  Some derived entities are based on BMP v3";
>         reference
>           "RFC 7854: BGP Monitoring Protocol.";
>       }
> --- 5438,5444 ----
>       identity invalid-route-reason {
>         description
>           "Base identity for reason code for routes that are rejected as
> !           invalid. Some derived entities are based on BMP v3.";
>         reference
>           "RFC 7854: BGP Monitoring Protocol.";
>       }
> 
> 
> 

Mahesh Jethanandani
mjethanandani@gmail.com