[mpls] Benjamin Kaduk's No Objection on draft-ietf-mpls-base-yang-15: (with COMMENT)

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

Benjamin Kaduk has entered the following ballot position for
draft-ietf-mpls-base-yang-15: No Objection

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-base-yang/



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

Before getting into the section-by-section comments, I'll note that
there are a few things in the YANG module itself that seem awry to my
untrained eye.

Section 2.2

   The 'ietf-mpls' module defines the following identities:
   [...]
   label-block-alloc-mode:

(nit?) We also define two identities based on this one; is the list of
"defines the following identities" supposed to be comprehensive (vs.
"high-level")?

   nhlfe-multiple-contents:

      A YANG grouping that describes a set of NHLFE(s) and their
      associated parameters as described in the MPLS architecture
      document [RFC3031].

(editorial) Perhaps we could say a few more words about how these NHLFEs
are related/why they are being grouped together?  (I do not recall such
grouping being specifically covered in 3031.)

   rib-mpls-properties:

      A YANG grouping for the augmentation of MPLS label forwarding data
      to the generic RIB as defined in [RFC3031].

nit(?): the "as defined in <reference>" seems more applicable to "MPLS
label forwarding data" than "generic RIB".

Section 2.3

   o  routes that cross-connect an MPLS local label to a Layer-3
      adjacency or interface - such as MPLS Segment-Routing (SR)
      Adjecency Segments (Adj-SIDs), SR MPLS Binding SIDs, etc. as
      defined in [RFC8402].

nit(?): is there a semantic difference between "MPLS SR" and "SR MPLS"
for the two types of route mentioned?

Section 2.5

  identity label-block-alloc-mode {
    description
      "Base identity label-block allocation mode.";
  }

nit: missing "for".

  grouping nhlfe-multiple-contents {
    description
      "MPLS NHLFE contents.";

nit: this description feels a little terse; is this the "generic" case
or something similar?

    leaf index {
      type string;
      description
        "A user-specified identifier utilised to uniquely
         reference the next-hop entry in the next-hop list.
         The value of this index has no semantic meaning
         other than for referencing the entry.";
    }
    leaf backup-index {
      type string;
      description
        "A user-specified identifier utilised to uniquely
         reference the backup next-hop entry in the NHLFE list.
         The value of this index has no semantic meaning
         other than for referencing the entry.";
    }

I'm not sure I understand the semantics, here -- does 'index'
authoritatively name the current entry with 'backup-index' being
effectively a pointer to a different entry?  I don't see any leafs of
type string in, e.g., rt-types:mpls-label-stack that this would be
referring to.  If the backup-index is indeed just a pointer to a
different entry, why is the string name used instead of a YANG
reference?

Also, what's a good reference for "backup" MPLS next-hops?  I don't see
the string "backup" in either RFC 3031 or 3032.

  grouping interfaces-mpls {
      [...]
      leaf name {
        type if:interface-ref;
        description
          "The name of a configured MPLS interface.";
      }

pedantic nit: is this really a "name"?

        leaf start-label {
          type rt-types:mpls-label;
          must '. >= ../end-label' {
            error-message "The start-label must be less than or equal "
                        + "to end-label";
          }

I think the sense of the YANG comparator is reversed, for both
start-label (this) and end-label (not quoted).  (Also, IIUC, it's
redundant to specify both, but harmless.)

        leaf free-labels-count {
          when "derived-from-or-self(../block-allocation-mode, "
             + "'mpls:label-block-alloc-mode-manager')";
          type yang:counter32;
          config false;
          description
            "Label-block free labels count.";

I think that counter32 is semantically the wrong type, here (and for
inuse-labels-count as well) -- IIRC the 'counter' types are for thigns
like counting a particular type of event, and only ever monotonically
increase.  Even if the free label count behaves monotonically (which is
far from clear to me), wouldn't it be decreasing, not increasing?

Also, won't it always be true that free-labels-count +
inuse-labels-count == end-label - start-label + 1?  I'm not sure why
there's a need to introduce such redundant fields and the corresponding
risk of internal inconsistency among them.

    uses rib-mpls-properties {
      /* MPLS AF aaugmentation to native MPLS RIB */

nit: s/aaugmentation/augmentation/

    uses rib-active-route-mpls-input {
      /* MPLS AF aaugmentation to native MPLS RIB */

(ditto)

Section 4

Why is the rt:simple-next-hop augmentation listed but not the
rt:next-hop-list augmentation?  IIUC they are functionally identical in
terms of the sensitivity of information content.

Also, it seems like the augmented RPC output is similarly sensitive to
the readable nodes that are already mentioned.

It looks like the main writeable nodes are the global
configuration/state, and while it's perhaps defensible to say that these
particular nodes are not sensitive, I did want to check what the
behavior would be if (e.g.) the label-blocks subtree was overwritten.
Would things just silently start using the new label block and keep
working?

Perhaps it would be too banal, but should we reference the security
considerations of 3031/3032 as applying to MPLS usage?

Section 7.1

It's not clear that we reference RFC 8402 in a normative manner
anywhere.

Section 7.2

I agree with the document shepherd that RFCs 3031 and 3032 naturally
would be classified as normative references.  Similarly, RFC 7424 is
refrenced by the YANG module itself, which usually indicates a normative
reference.