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

["Acee Lindem (acee)" <[e-mail]>]

Hi Jeff, 

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

    Acee,


    On Tue, Oct 27, 2020 at 03:05:06PM -0700, Mahesh Jethanandani wrote:
    > >     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?

    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.

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. 


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

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

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).";                                                                                                                                                     
      }
    }

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

    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.

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

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

    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?

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. 

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

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

    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.

Sounds good.

Thanks,
Acee