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

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 2 Nov 2020 19:55:25 +0000

Hi Mahesh,

Sounds good. See a couple inline comments.

From: Mahesh Jethanandani <[email-masked]>
Date: Tuesday, October 27, 2020 at 6:05 PM
To: Acee Lindem <[email-masked]>
Cc: YANG Doctors <[email-masked]>, IDR List <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: Re: Yangdoctors early review of draft-ietf-idr-bgp-model-09

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 <[email-masked]<mailto:[email-masked]>> 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: [email-masked]<mailto:[email-masked]>
        [email-masked]<mailto:[email-masked]>
[email-masked]<mailto:[email-masked]>
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.

Sounds good.

    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?

Since there no IETF RFC specifically describing BGP IPsec, I think some brief text on the intension here is required. Does this simply provision a transport mode SA for the BGP 5-tuple? I’m thinking one of the co-authors could do this. I’d suggest Jeff Haas since it appears Juniper supports this…

    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.

I think this is a poor choice since it should be terms of the device – not the remote device. Perhaps, using your line of thought, even “local-graceful-restart” would be much better.

    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.

Ok – I see this matches RFC 4273. I don’t remember why I thought otherwise.

Thanks,
Acee

    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<http://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<http://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
[email-masked]<mailto:[email-masked]>

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 2 Nov 2020 19:55:27 +0000

Hi Mahesh,

Sounds good. See one inline.

Date: Thursday, October 29, 2020 at 7:34 PM

Picking up from where I last left off ...

Reviewer: Acee Lindem

Major Comments:

Minor Comments:

Could you expand what you have in mind. Choice between which options?

I mean you could have a YANG choice for “inline” versus “reference” option rather than an enum that gets tested with YANG “with” contstraint.

Need to confer with my co-authors.

Ok. Unless my co-authors object, I will remove it.

Need to confer with my co-authors.

Need to confer with my co-authors.

Need to confer with my co-authors.

Ok. A comment up front will clarify this. No change in the model itself. Right?

Agreed. This is one of the sub-modules that is going to be collapsed.

Done.

Nits:

 Status of This Memo
--- 20,26 ----

Fixed.

 Status of This Memo
***************
*** 119,125 ****

Fixed.

*** 146,158 ****

This renders very differently in HTML and other formats, where it highlights them as URLs. Since the source file for all of them is common (.xml), this is something that has to be handled by xml2rfc.

Ok - thanks

Fixed.

!    o  RIB contents.

!    o  BGP RIB contents.

Fixed

*** 210,220 ****

--- 210,220 ----

See above. This is a rendering issue and needs to be resolved by xml2rfc. Will skip over similar comments.

Ok.

Thanks,
Acee

***************
*** 283,289 ****

--- 293,299 ----

***************
*** 374,383 ****

Fixed.

*** 406,412 ****

Fixed

*** 480,487 ****

--- 535,541 ----

***************

Fixed

*** 652,667 ****

--- 652,667 ----

***************
*** 732,739 ****

 5.1.  URI Registration

 5.1.  URI Registration

Fixed

*** 744,750 ****

    name: ietf-bgp
--- 744,750 ----

    name: ietf-bgp
***************

Fixed

*** 816,822 ****

Fixed

*** 1080,1086 ****
         }
         container distance {
           description

Fixed

*** 1148,1154 ****
             key "afi-safi-name";
             description

Fixed

*** 1237,1243 ****
             description

Fixed

*** 1560,1566 ****
               description

--- 1560,1566 ----
               description

***************

Fixed

*** 1575,1583 ****
               config false;
               description

Fixed

*** 1780,1787 ****

Fixed

*** 1814,1821 ****

Fixed.

*** 2065,2071 ****

--- 2065,2071 ----

***************

Fixed

*** 2202,2209 ****
            default "16.0";
            description

Fixed

*** 2345,2351 ****

--- 2345,2351 ----

***************
*** 2452,2458 ****
            description

Fixed

*** 2502,2508 ****
            default "false";
            description

Fixed

*** 2638,2644 ****
          default "false";
          description

--- 2638,2644 ----
          default "false";
          description

***************

Fixed

*** 2811,2823 ****
            type uint32;
            description

Fixed

*** 2842,2848 ****
          type boolean;
          default "false";
          description

--- 2842,2848 ----
          type boolean;
          default "false";
          description

***************

Fixed

*** 2987,2995 ****

Fixed

*** 3001,3012 ****
         groups";

Fixed

*** 3035,3041 ****
         groups";

Fixed

*** 3256,3262 ****
         key "afi-safi-name";
         description

Fixed

*** 3272,3278 ****

Fixed

*** 3312,3318 ****
       container afi-safis {
         description

Fixed

*** 3512,3518 ****
         container prefixes {
           config false;
           description

Fixed

*** 3600,3608 ****

Fixed

*** 3700,3706 ****

     identity route-refresh {
--- 3700,3706 ----

     identity route-refresh {
***************

Fixed

*** 3846,3852 ****

Fixed

*** 3856,3863 ****

--- 3856,3863 ----

***************

Fixed

*** 3895,3901 ****

Fixed

*** 4128,4134 ****
       }
       description

??

*** 4961,4969 ****
           type boolean;
           description

Fixed

*** 4987,4994 ****

           description

           description

Fixed

*** 5064,5070 ****
             type inet:ipv4-address;
             description

Fixed

*** 5256,5262 ****
               }
               description

--- 5257,5263 ----
               }
               description

***************

Fixed

*** 5437,5443 ****

Fixed.

Thanks

Mahesh Jethanandani

---

From: Jeffrey Haas <[email-masked]>
Date: Mon, 2 Nov 2020 19:43:56 -0500

Acee,

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

There is no reference here.  Multiple BGP implentations support running BGP
over an ipsec tunnel.  A previous prod during a prior review suggested that
if we had something to use from an IETF group doing yang work on ipsec for
protecting routing transport sessions might be applicable here.

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

There's actually a few issues here:
- The restarting mode is per AFI-SAFI, while this container doesn't contain
  anything that contains per AFI-SAFI state.
- The local peer can know whether it's acting as a helper and whether it can
  retain forwarding for a given AFI/SAFI.
- We can know if the remote BGP speaker has advertised GR for a given
  AFI/SAFI, which tells us whether it supports GR and can help the local
  speaker with restarting.
- We can know if, on the last restart, forwarding was preserved for that
  AFI/SAFI.

Proposal: Replace leaf "mode" with:
list afi-safi-supported {
  key "afi-safi-name"
  leaf afi-safi-name {
    type identityref { base bt:afi-safi-type; }
  }
  leaf local-supported {
    type boolean;
  }
  leaf remote-supported {
    config false;
    type boolean;
  } 
  leaf remote-forweard-preserved {
    type boolean;
    config false;
  }
}

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

Note from BGP MIB history: The original BGP MIB provided a trap on every
backward transition.  This meant a very noisy set of traps for sessions that
were repeatedly trying to connect.  Many vendors implemented knobs to
restrict this solely to transitions out of established.

I'd suggest the working group consider what it wants here.  We don't have to
repeat the mistakes of the past on this one.

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

RFC 8294 is narrowly focused on supporting a small cluster of VPN extended
communities.  The extended communities feature carries a number of
additional things.  A recent example would include the RPKI origin
validation community.  Others would be communities defined for
link-bandwidth and the BGP flowspec action communities.

The complete registry is here:

https://www.iana.org/assignments/bgp-extended-communities/bgp-extended-communities.xhtml

The issue thus comes where the general purpose registry for communities that
weren't defined in RFC 8294.  A similar issue comes down to how do we
maintain this list and add to it as we extend BGP by adding new extended
communities?

I agree that we should not repeat the items in RFC 8294.

Won't we need the extended communities typedef to be maintained in an IANA
registry to allow easy extension?

Error: the option to set-ext-community doesn't take this list.
Error: We don't have a RIB ext-community oper list.

> I will defer this to my co-authors.

Set med in implementations typically permits additive/subtractive MEDs.  This
permits operations such as "set med = med + 5".

Syntax issue: the implementation is typically "set med", "set med additive
<value>" or "set med-igp".  Thus the syntax that's additive is only
applicable in some of these cases.

I believe we'll hae to restructure this.

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Tue, 3 Nov 2020 16:21:21 +0000

Hi Jeff, 

On 11/2/20, 7:28 PM, "Jeffrey Haas" <[email-masked]> wrote:

    Acee,

If this is an IPsec tunnel then it would be provisioned like other IPsec tunnels as opposed to in the BGP YANG model. I'm assuming that if it is provisioned here, it is transport mode BGP for the TCP session associated with the BGP peer. In any case, the intended usage needs to be specified. 

You need some good descriptions for the booleans as the names aren't enough. I'd replace "remote-forward-preserved" with "remote-forwarding-preserved" as in Forwarding Information Base (FIB).  Note that you no longer have the granularity between support of local restart and local helper (based on only one config true leaf).  Also, it seems that granularity is now wrong - you need the config false values on a per-peer basis so I don't think you can mix the local-configuration with the remote peer state in the same grouping. 

FWIW here is the OSPF config true container.

container graceful-restart {
      if-feature graceful-restart;
      description
        "Graceful restart config state.";
      reference "RFC 3623: OSPF Graceful Restart                                                                                                                                                     
                 RFC 5187: OSPFv3 Graceful Restart";                                                                                                                                                 
      leaf enable {
        type boolean;
        description
          "Enable/Disable graceful restart as defined in RFC 3623                                                                                                                                    
           for OSPFv2 and RFC 5187 for OSPFv3.";                                                                                                                                                     
      }
      leaf helper-enable {
        type boolean;
        description
          "Enable graceful restart helper support for restarting                                                                                                                                     
           routers (RFC 3623 Section 3).";                                                                                                                                                           
      }
      leaf restart-interval {
        type uint16 {
           range "1..1800";
        }
        units seconds;
        default "120";
        description
          "Interval to attempt graceful restart prior                                                                                                                                                
           to failing (RFC 3623 Section B.1) (seconds)";                                                                                                                                             
      }
      leaf helper-strict-lsa-checking {
        type boolean;
        description
          "Terminate graceful restart when an LSA topology change                                                                                                                                    
           is detected (RFC 3623 Section B.2).";                                                                                                                                                     
      }
    }

In OSPF we did not implement the ospfMaxAgeLsa or ospfOriginateLsa traps as YANG model notications for just this reason. 

I guess you want to publish this model at some point in the near future? I'd cover what is standardized now without replicating the communities types in RFC 8294. 

Sounds good.

Thanks,
Acee

---

From: tom petch <[email-masked]>
Date: Mon, 9 Nov 2020 17:27:45 +0000

________________________________________
From: Idr <[email-masked]> on behalf of Jeffrey Haas <[email-masked]>
Sent: 03 November 2020 00:43
To: Acee Lindem; Mahesh Jethanandani
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: Re: [Idr] Yangdoctors early review of draft-ietf-idr-bgp-model-09

Acee,

<tp>

Jeff

I2NSF have produced an I-D, currently with the IESG, on configuring IPsec.  It is part of I2NSF which is a lot more than IPsec but it did attract a comment from IPSECME WG asking that it be changed to make it more widely applicable with, I think, an implication that IPSECME might be about to do something.

IPsec, indeed IPSECME, is not my stock in trade, more like TSL and SSH but it did make me look at all those old IKEv2 RFC and wonder who used them:-)

Tom Petch

_______________________________________________
Idr mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/idr