[mpls] Benjamin Kaduk's Discuss on draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)

[Benjamin Kaduk via Datatracker <[e-mail]>]

Benjamin Kaduk has entered the following ballot position for
draft-ietf-mpls-ldp-yang-07: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-mpls-ldp-yang/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

The YANG doctor last call review of the -06 notes that these modules are
in violation of the expectations of RFC 8349 (at least w.r.t. defining a
new identity for the control-plane protocol and possibly more).  This
document should either be brought into compliance or explain why it
diverges.

The YANG doctor's comments about default values for "local"
configuration (e.g., graceful-restart configuration) causing the global
configuration to never take effect should also be addressed.

Please include some justification for why LDP IPv6 is considered an
"extended feature" (which is particularly surprising given that Section
2 classifies "IP" to refer to both IPv4 and IPv6 together).

We need to define the format/semantics of the md5-key string (e.g., is
it hex? base64{url,}?) either directly or by reference (as the YANG
doctor notes).  Using a crypto-type would probably be appropriate, as
would adding a note that tcp-md5 is obsoleted by TCP-AO.

In the peer-af-policy-container grouping's
label-policy/advertise/prefix-list, we need to say if the filter is for
inclusion or exclusion of outgoing label advertisements.
Similarly for the incoming label advertisements in the 'accept'
container (and most of the <foo>-list-ref usages?).

In the policy-container grouping's
label-policy/assign/independent-mode/prefix-list, the description
suggests that the contents will provide not just a list of prefixes that
act as a filter, but also a map from prefix to label.  This is a
qualitatively different usage than the previous occurrences of
prefix-list-ref, and it seems like a different type may be needed for
it.  Similarly for ordered-mode.

(pro-forma) This document has 6 authors, and per RFC 7322, as this is
more than five, we are supposed to consider whether that's appropriate.
Seeing nothing in the shepherd writeup or similar, I mention it here.


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Please also respond to the other YANG doctor, rtg-dir, and other
directorate reviewers' comments.

I support Roman's Discuss.

I suggest making a reference to RFC 5920 at some point, perhaps from the
security considerations.

We have several instances of a "container ipv4" that's a "container with
presence" in the jargon of RFC 7950.  As SEC AD one thing I look for is
edge cases, such as when a subset of these containers are present but
not all (and not none).  While for this specific case, it seems fairly
natural to assume that IPv4 is enabled if any are present, it does make
me wonder if the modeling is at an optimal state if there's even the
possibility for such skewed signals.  (I didn't specifically audit for
any other cases of "redundant" containers-with-presence.)

It's not entirely clear to me why we need (all of) the specialization of
structures into using inet:ipv4-address and inet:ipv6-address as opposed
to just inet:ip-address.  I do see that the latter is used in several
places already, and so assume that this was already considered; perhaps
some of the discussion/motivation could be included in the document,
though.

Section 1

   The data model is defined for following constructs that are used for
   managing the protocol:

nit: "the following".

I share the YANG doctor's concern that separating the configuration and
operational state constructs is not consistent with the NMDA model.

Section 1.1

   The "base" category contains the basic and fundamental features that
   are covered in LDP base specification [RFC5036] and constitute the
   minumum requirements for a typical base LDP deployment.  Whereas, the
   "extended" category contains all other non-base features.  All the

This statement ("all other") is not future-proof.

Section 3

   o  This module aims to address only the core LDP parameters as per
      RFC specification, as well as some widely deployed non-RFC
      features (such as label policies, session authentication etc).
      Any vendor specific feature should be defined in a vendor-specific
      augmentation of this model.

[Even for widely-deployed non-RFC features we still need to provide a
clear description of what semantics are being covered, whether
in-document or via an external reference.]

   o  This model is a VPN Forwarding and Routing (VRF)-centric model.
      It is important to note that [RFC4364] defines VRF tables and

side note: I would like the IETF to someday get to a point where "VPN"
(the 'P' is for "private") is used only for protocols/deployments that
provide confidentiality protection ("privacy") for the data being
conveyed.  We are, of course, not there now, and I cannot insist on any
change here.  But please consider whether an alternate phrasing would
suffice, that includes just the "virtual" part but not "VPN".  Since we
are mostly using "VRF" in the rest of the document, this is not as
daunting as it sometimes is...

Section 4

It might be nice to have the tree show the ipv4 and ipv6 children in
as similar an order as is possible (with the understanding that the
semantics are not affected by the order of appearance, of course).

       |  +--rw address-families
       |  |  +--rw ipv4!
       |  |  |  +--rw enable?                           boolean
       |  |  |  +--ro label-distribution-controlmode?   enumeration
       |  |  |  +--ro bindings
       |  |  |  |  +--ro address* [address]
       |  |  |  |  |  +--ro address               inet:ipv4-address
       |  |  |  |  |  +--ro advertisement-type?   advertised-received
       |  |  |  |  |  +--ro peer
       |  |  |  |  |     +--ro lsr-id?           leafref
       |  |  |  |  |     +--ro label-space-id?   leafref

Does this imply that there is only one peer per address?  Is that going
to be a limitation for any deployment cases?

          +--rw ldp-ext:session-downstream-on-demand
          |       {session-downstream-on-demand-config}?
          |  +--rw ldp-ext:enable?      boolean

nit(?): missing '|' to connect down to ldp-ext:enable?
Similarly for ldp-ext:max-wait a few lines later, and some other
occurrences later in the document.

Section 5.1.2

Am I reading this right that (e.g.) per-interface hello-holdtime is
configurable as an extension and only the global hello-holdtime is
present in the "base" module?

Section 7

"NHLFE" is used only once in this document (and is not listed as
"well-known" at
https://www.rfc-editor.org/materials/abbrev.expansion.txt); please
expand it out.

Section 9

Is 2018 still the right copyright year for the modules themselves?

Section 9.1

     typedef ldp-address-family {
       type identityref {
         base rt:address-family;
       }

Why can't we use rt:address-family directly in the one container that
uses this?

       description
         "Duration represented as 32 bit seconds with infinite.";

nits: "32-bit" (hyphenated), "with infinity".

     typedef downstream-upstream {
       type enumeration {
         enum downstream {
           description "Downstream information.";
         }
         enum upstream {
           description "Upstream information.";
         }
       }
       description
         "Received or advertised.";
     }

nit: copy/paste on the description?

         leaf used-in-forwarding {
           type boolean;
           description
             "'true' if the lable is used in forwarding.";

nit: s/lable/label/

     grouping graceful-restart-attributes-per-peer {
       description
         "Per peer graceful restart attributes.
          On the local side, these attributes are configuration and
          operational state data. One the peer side, these attributes
          are operational state data reveiced from the peer.";

nit: s/reveiced/received/

     grouping peer-state-derived {
       description
         "The peer state information derived from the LDP protocol
          operatoins.";

nit: s/operatoins/operations/

     grouping peer-state-derived {
     [...]
       leaf next-keep-alive {
         type uint16;
         units seconds;
         config false;
         description "Time to send the next KeepAlive message.";
       }

nit: this description text sounds like it's giving an absolute time, but
given that it's a uint16 I presume it's "time [from now] until sending".

       container received-peer-state {
         config false;
         description
           "Operational state information learned from the peer.";

         uses graceful-restart-attributes-per-peer;

         container capability {
           description "Configure capability.";

This is config-false; "Configure" in the description cannot be correct,
though I suppose "Configured" could be.
(Also applies to several of the child containers' descriptions.)

       leaf up-time {
         type string;
         config false;
         description "Up time. The interval format in ISO 8601.";

We probably should have some sort of reference for ISO 8601 (though of
course it cannot actually *be* a <xref> in the YANG module itself)

       leaf notification {
         type yang:counter64;
         description
           "The number of messages sent or received.";

Is this the same as or different than "leaf total-messages"?

               leaf label-distribution-controlmode {

nit: any reason to not hyphenate control-mode as well?

           container interfaces {
             description
               "A list of interfaces for LDP Basic Descovery.";

nit: s/Descovery/Discovery/

         container discovery {
           description
             "Neibgbor discovery configuration and operational state.";

nit: s/Neibgbor/Neighbor/

                   // ipv4
                   container hello-adjacencies {
                     config false;
                     description
                       "Containing a list of hello adjacencies.";

                     list hello-adjacency {
                       key "adjacent-address";
                       config false;
                       description "List of hello adjacencies.";

                       leaf adjacent-address {
                         type inet:ipv4-address;
                         description
                           "Neighbor address of the hello adjacency.";
                       }

I'm not an expert here, but I'm not really seeing why we need to
specialize this container for v4 vs v6 as opposed to just using
inet:ip-address and having something that's applicable to both.

                   leaf adjacent-address {
                     type inet:ipv4-address;
                     description
                       "Configures a remote LDP neighbor and enables
                        extended LDP discovery of the specified
                        neighbor.";

Given that there's a sibling leaf "enable", do we still want "and
enables" here?

Should peer/capability have a comment in its description that the
container exists to be augmented by other (extension) modules?

     notification mpls-ldp-fec-event {

Am I reading this right that we only generate notifications on 0->1 and
1->0 transitions of "number of prefixes active for a given FEC"?  Would
we ever want to know about addition/removal of prefixes that do leave
the FEC up?

Section 9.2

I can't find a pattern into which if-feature statements go into
containers used to augment things vs. the augment directives themselves.
Can we be more consistent (or am I missing the pattern)?

     feature dual-stack-transport-pereference {
       description
         "This feature indicates that the system allows to configure
          the transport connection pereference in a dual-stack setup.";

nit: s/pereference/preference/ (twice)

     feature policy-targeted-discovery-config {
       description
         "This feature indicates that the system allows to configure
          policies to control the acceptance of targeted neighbor
          discovery hello messages.";

nit: s/hello/Hello/

     typedef neighbor-list-ref {
       type string;
       description
         "A type for a reference to a neighbor address list.
          The string value is the name identifier for uniquely
          identifying the referenced address list, which contains a list
          of addresses that a routing policy can applied. The definition
          of such an address list is outside the scope of this
          document.";

If I'm reading correctly between here and where this is used, the
semantics of this are better described as "implementation-specific" --
that is, this is a label that refers to some local configured state,
specifically, a list of neighbor addresses used as an ACL.  (Emphasis on
*local* -- it seems like it does not need to interact with anything
else.)
I do sympathize with the YANG doctor, though -- as-is, this feels
underspecified.
(BTW, IP ACLs are not considered to be a security measure by the broader
security community.)
Similarly for the subsequent <foo>-list-refs.

         container interfaces {
           description
             "A list of interfaces on which forwarding is disabled.";

nit: given the separate 'ldp-disable' leaf, perhaps s/is/can be/ is in
order.
Also, the description of the list itself implies additional
functionality, of listing the interfaces used for basic discovery.

     grouping graceful-restart-augment {
       description "Augmentation to graceful restart.";

       leaf helper-enable {
         if-feature graceful-restart-helper-mode;
         type boolean;
         default false;
         description
           "Enable or disable graceful restart helper mode.";

Where is this "helper mode" documented?

     augment "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
       + "ldp:global" {
       description "Graceful forwarding nexthop augmentation.";
       uses global-forwarding-nexthop-augment;
     }

nit: I don't think "Graceful" is correct, in the description.

Why is (the augmented)
/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/ldp:global/ldp:address-families/ipv6/label-distribution-controlmode
'config false'?  Doesn't RFC 5036 say that an LSR might support either
as a configuration option?

           leaf adjacent-address {
             type inet:ipv6-address;
             description
               "Configures a remote LDP neighbor and enables
                extended LDP discovery of the specified
                neighbor.";

As for the ipv4 case, do we still want "and enables" here?

     augment "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
       + "ldp:discovery/ldp:interfaces/ldp:interface/"
       + "ldp:address-families/ldp-ext:ipv6" {
       description "Interface address family IPv6 augmentation.";
       uses interface-address-family-ipv6-augment;

Didn't we just create .../ldp:address-families/ldp-ext:ipv6 with an
earlier augmentation in this module?  (Similarly for
/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/ldp:peers/ldp:peer/ldp:address-families/ldp-ext:ipv6
which we augment with "uses peer-af-policy-container".)

Appendix A

                 "is-router": [null],

This looks weird to me ("array containing one element with value literal
null") -- why does the array come into play?