Thanks for the clear introduction of how this models was designed (Section 2.2 and 2.3) and builds on prior work.

Thank you for addressing my DISCUSS and COMMENT points.

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.

[[ nits ]]

[ section 2.2 ]

* "represents support possible" -> "represents support for possible"?

[ section 2.3 ]

* s/Adjecency/Adjacency/

[ section 2.5 ]

* "Next-hop acts as primary traffic carrying."

  May read as slightly awkward.  Consider perhaps:

  "Next-hop acts as primary for carrying traffic."

  ...or something.

* "aaugmentation" -> "augmentation"

In Section 3, it says: "This document registers the following URIs in the IETF XML registry."  I think it should say "This document registers the following URIs in the 'ns' sub-registry of the IETF XML registry."

I can relate to Barry's comment about "module" vs. "model".  I think I understand the difference, but I've run into this with every YANG document that's come up lately:  I think the "model" is a formal expression for a configuration of some network component, while "module" is a chunk of YANG code that describes that model, which can be evaluated (a la ABNF) against some input to validate a configuration.

I also concur with Barry's remark about BCP 14 being cited when it's not used anywhere.  This should be cleaned up (by either using it, or removing the citation).  I'm tempted to DISCUSS this, but I assume since it's easily resolved that that'll happen in due course.

The document sometimes says “YANG model” and sometimes “YANG module”.  It should use the correct term and be consistent, no?

I note that there is BCP 14 boilerplate here but no BCP 14 key words in the document.  The shepherd writeup notes that also and says that the shepherd wanted to discuss this with the authors... but there is no resolution to that.  Did the shepherd discuss it with the authors?  Should some “must”s become “MUST”s?  Or should the boilerplate and references be removed?

— Section 1 —

   The MPLS base model also defines a new instance of the generic RIB
   model as defined in {!RFC8349}} to store native MPLS routes.

What is the notation around RFC8349?  Does it mean something, or is it a markup artifact that needs to be fixed during editing?

Thank you for the work put into this document. I really appreciated the clear and sharp introduction. I am also trusting the Management and routing ADs for the accuracy of the YANG module.

Please find below two minors nits

I hope that this helps to improve the document,

Regards,

-éric

== NITS ==

-- Section 1 --
s/A core routing data model is defined/A core routing YANG data model is defined/

-- Section 5 --
Naming all design team members could be more friendly.

Discuss cleared.

I also have some comments on the YANG module itself:

  feature mpls {
    description
      "Indicates support for MPLS switching.";
    reference
      "RFC3031";
  }

Is the MPLS feature really required?  I.e. is it plausible that a device would implement this YANG module without supporting MPLS switching?  If not, then just implementing the module should probably be sufficient without a separate feature.

Alternatively, it might be worth looking at the ISIS YANG module that define a feature to control with ISIS can be administratively controlled.  E.g. draft-ietf-isis-yang-isis-cfg-42, feature admin-control.  On a related point:

     augment /rt:routing:
       +--rw mpls {mpls}?
           ...
          +--rw interface* [name]
             +--rw name                      if:interface-ref
             +--rw enabled?                  boolean
             +--rw maximum-labeled-packet?   uint32

Is the "enabled" leaf definitely required here?  Isn't having the interface under the MPLS list sufficient to indicate that MPLS is enabled?  If you really want this leaf then I would change its name to "enable" rather than "enabled", change it's default to true, and consider putting it under a feature like admin-control from the ISIS YANG module.


     augment /rt:routing/rt:ribs/rt:rib/rt:routes/rt:route:
       +--ro mpls-enabled?         boolean {mpls}?
       +--ro local-label?          rt-types:mpls-label {mpls}?
       +--ro destination-prefix?   -> ../local-label {mpls}?
       +--ro route-context?        string {mpls}?

Is "local-label" clearly enough associated with MPLS in the IP RIB?  E.g. should this be mpls-local-label, or under an mpls container?
Presumably the mpls-enabled leaf shouldn't appear under the MPLS RIB?  It might be cleaner to split this into two augments, one for the MPLS RIB, and one for other RIBS.


The mpls-operations-type is defined, but not actually used by this module.  I wanted to check that this is expected.  I also note that the operations-type doesn't include a "no-operation" case statement which I thought might potentially be useful (but I'm not an MPLS expert).

Regarding  grouping label-blocks:

Ben's comment is right that the must statements associated with the start-label/end-label have the wrong sense.  There should also only be one of them (since they would always fail/pass in unison), and I would recommend just keeping the end-label must statement.

I wasn't sure whether the unique statement is really helpful.  Am I right in assuming that the blocks should be disjoint and non overlapping? If so, the unique statement doesn't achieve this, and although it could probably be done in a must statement, it would be tricky to express and get right.  However, if this is a constraint then it would certainly be helpful for the description of label-block to mention this.

Regarding grouping rib-active-route-mpls-input:

I know that this has been discussed with yang-doctors and the authors of the base RIB YANG module that is being extended but seeing the YANG I'm not quite sure we have the best outcome, since users of the RPC would be required to specify both destination-address and local-label as input parameters.  Possibly a better choice would to make this a choice so that a client could query either using a destination-address or a local-label (with both using the same type definition)?