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

[Jeffrey Haas <[e-mail]>]

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.


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



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

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

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.