Early Review of draft-ietf-mpls-mldp-yang-12
review-ietf-mpls-mldp-yang-12-rtgdir-early-zhang-2025-01-29-00

I have only very limited comprehension of YANG from more than 10 years ago.
Therefore, my review does not include "9.  YANG Specification".
Security considerations are always a headache to me as well, and I skipped
"10. Security Considerations".

Many of the following questions/comments are because of my fuzzy
YANG understanding - perhaps some clarfication in the email or in the document
will clear them.

   [Ed Note: Some of the topics in the above list are to be addressed/
   extended in a later revision of this document].

Shouldn't the above be removed?

   For manually provisioned Multipoint LSPs, we represent its associated
   FEC as (root-address, opaque-element) tuple.  The following table
   lists various type of opaque elements with their keys, as later used
   in the configuration and state model:

Why "manually provisioned" only? Shouldn't it apply to both provisioned
ones (on leaves) and signaled ones (on roots/transit nodes)? In fact,
the last sentence does say "in the ... state model". Perhaps "manually
provisioned"?

  augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:
    +--rw mpls-ldp
       +--rw global
       |  +--rw capability
       |  |  +--rw mldp:mldp
       |  |     +--rw mldp:p2mp
       |  |     |  +--rw mldp:enabled?   boolean
       |  |     +--rw mldp:mp2mp
       |  |     |  +--rw mldp:enabled?   boolean

Shouldn't all the "enabled" be "supported"? A router may "support" (or not)
a capability, and a supported capability nay be "enabled" (or not) by
provisioning as in the following (notice the "mldp:enabled"):

       |  +--rw mldp:mldp
       |     +--rw mldp:enabled?            boolean
       |     +--rw mldp:address-families
       |        +--rw mldp:ipv4!
       |        |  +--rw mldp:enabled?                  boolean
       |        |  +--ro mldp:roots
       |        |  |  +--ro mldp:root* [root-address]

For the following parameters in the "capability" portion,

       |  |     |  +--rw mldp:switchover-delay?   uint16
       |  |     |  +--rw mldp:timeout?            uint16
       ...
       |  |           +--rw mldp-ext:targeted-session-teardown-delay?   uint16

Shouldn't they be the under the following?

       |  +--rw mldp:mldp
       |     +--rw mldp:enabled?            boolean

If the "capability" is really about whether a feature is enabled on this
node or not, then I don't think we should call them out as "capabitlity" -
they'd just be like if mldp is enabled/configured or not.

I noticed that the global/peers part uses "capable" - should we make it
consistent?

     +--ro mldp:peer* [direction peer advertisement-type]
     |  +--ro mldp:peer                  ->
     ../../../../../../../../../../../ldp:peers/peer/lsr-id |  +--ro
     mldp:direction             ldp:downstream-upstream |  +--ro
     mldp:advertisement-type    ldp:advertised-received |  +--ro mldp:label?   
                 rt-types:mpls-label |  +--ro mldp:mbb-status?          
     lsp-mbb-role |  +--ro mldp-ext:mofrr-status?     lsp-mofrr-role

I noticed the a similar mldp-ext:peer (compared to mldp:peer)

     +--ro mldp-ext:peer* [direction peer advertisement-type]
        +--ro mldp-ext:peer                  ->
        ../../../../../../../../../../../../ldp:peers/peer/lsr-id +--ro
        mldp-ext:direction             ldp:downstream-upstream +--ro
        mldp-ext:advertisement-type    ldp:advertised-received +--ro
        mldp-ext:label?                rt-types:mpls-label +--ro
        mldp-ext:mbb-status?           lsp-mbb-role +--ro
        mldp-ext:mofrr-status?         lsp-mofrr-role

Can they not share a same defintition (is that called "grouping")?
The key is [direction peer advertisement-type], but the peer is listed before
direction. Does that matter?

     +--ro mldp:bindings
        +--ro mldp:opaque-element-lspid
        |     +--ro mldp-ext:recursive-fec* [recur-root-addr
        |        +--ro mldp-ext:recur-root-address    inet:ip-address
        |        +--ro mldp-ext:recur-rd             
        rt-types:route-distinguisher |        +--ro mldp-ext:multipoint-type?  
           mldp:multipoint-type +--ro mldp-ext:opaque-element-transit +--ro
        mldp-ext:opaque-element-bidir

Why is mldp-ext:recursive-fec nested under mldp:opaque-element-lspid instead
of parallel to all bindings? and where is the recursive part of the fec
(besides the recur-root-address/rd)?

  +--rw mldp:configured-leaf-lsps
  |  +--rw mldp:opaque-element-lspid
  |  |  +--rw mldp:fec-label* [root-address lsp-id]
  |  |     +--rw mldp:root-address         inet:ipv4-address
  |  |     +--rw mldp:lsp-id               uint32
  |  |     +--rw mldp:multipoint-type?     multipoint-type
  |  |     +--rw mldp-ext:recursive-fec* [recur-root-address recur-rd]
  |  |        +--rw mldp-ext:recur-root-address    inet:ip-address
  |  |        +--rw mldp-ext:recur-rd              rt-types:route-distinguisher
  |  |        +--rw mldp-ext:multipoint-type?      mldp:multipoint-type

Same question for the configured leaf lsp, and notifications.

Looking at the other fec types, I wonder if the sub-fields of
mldp-ext:recursive-fec should be pulled out, and the extra multipoint-type
be removed. Or, use the mldp-ext:recursive-fec for all (though the
multipoint-type seems to be extra).

I noticed that the configured leaf lsps and the notifications are very similar.
Could they share the same grouping (or whatever is the proper yang terminology)?

   *  mLDP enablement: To enable mLDP under a (VRF) routing instance,
      mldp is enabled in the mldp container under LDP.  It is to remind
      that the LDP modules reside under a network-instance and the scope
      of any configuration defined under this tree is network-instance
      (VRF) scoped.  Given that mLDP requires LDP signaling, it is not
      sensible to allow disabling the LDP control plane under a (VRF)
      network-instance while requiring mLDP to be enabled for the same.
      However, if a user wants to only allow signaling for multipoint
      FECs on an LDP/mLDP enabled VRF instance, he/she can use LDP
      label-policies to disable unicast FECs under the VRF.
      Alternatively, the same can be achieved by advertising the
      capabilities and the procedures defined in [RFC7473].

For the two alternatives (policy or RFC7473), how is it refected in the
YANG configuration/state?

   In most common deployments, it is desirable to disable mLDP
   (capabilities announcements) on a targeted-only LDP peering, where
   targeted-only peer is the one whose discovery sources are the
   targeted type only.

Why is that? It there a reference?