This is the YANG Doctor last call review of draft-ietf-isis-sr-yang-13. I think this module is written in a nice, straight forward way. There are a few things that have to be fixed before this can be published, however, so I will call it ready with issues.


#1: Dependency on unpublished module

The ietf-isis-sr module under discussion here imports module ietf-isis. That module is still not published as an RFC. For the review here I used the ietf-isis.yang as defined in draft-ietf-isis-yang-isis-cfg-42. If the ietf-isis.yang module changes before it is published, that could potentially affect ietf-isis-sr.

A closely related observation is that the import statement lacks an import reference. Maybe you could add a placeholder reference until you know what RFC number to reference?

  import ietf-isis {
    prefix "isis";
  }


#2: Inappropriate when expressions

The following when expression appears 13 times in the module. There are two problems with it.

    when "/rt:routing/rt:control-plane-protocols/"+
         "rt:control-plane-protocol/rt:type = 'isis:isis'" {

Direct equality comparison with identities is generally not recommended, as that removes the possibility to "subclass" isis:isis into variants in the future. The recommended approach is to use the XPath function derived-from-or-self() instead. See below.

The other problem with this when expression is worse. Since an unqualified absolute path is used, the expression becomes true for all routing-plane-protocol instances as soon as there is at least one of isis type. This enables the isis-sr extensions inside all routing-plane-protocol instances as soon as there is at least one isis instance in the system. I don't believe this was what the authors intended.

There are several possible ways to remedy the situation. The one I would recommend is to use relative paths in the when expression so that the path never leaves the current control-plane instance. Since the relative path is a little different depending on where it is used, I'll provide two examples. Just let me know once you've updated all 13 of them, and I'll review.

// Line 503
  augment "/rt:routing/" +
          "rt:control-plane-protocols/rt:control-plane-protocol"+
          "/isis:isis" {
//OLD    when "/rt:routing/rt:control-plane-protocols/"+
//OLD         "rt:control-plane-protocol/rt:type = 'isis:isis'" {
    when "derived-from-or-self(../rt:type, 'isis:isis')" {

// Line 587
  augment "/rt:routing/" +
          "rt:control-plane-protocols/rt:control-plane-protocol"+
          "/isis:isis/isis:interfaces/isis:interface" +
          "/isis:adjacencies/isis:adjacency" {
//OLD    when "/rt:routing/rt:control-plane-protocols/"+
//OLD         "rt:control-plane-protocol/rt:type = 'isis:isis'" {
    when "derived-from-or-self(../../../../../rt:type, 'isis:isis')" {

Basically, you add one ../ for each name in the augment path, starting at the tail end working towards the beginning, until you hit the name rt:control-plane-protocol (because it is the parent node of rt:type).

If you prefer, there are solutions using absolute paths too. In that case you need to add filters ("predicates") for the control-plane-protocol keys to the when expression path. But IMO the method I explained above is probably easier to follow.


#3: No mandatory, no default, no description

Some leafs in the module are not mandatory, have no default and no text in the description that would explain how to interpret this ambiguous situation.

    container ti-lfa {
      if-feature ti-lfa;
      leaf enable {
        type boolean;
        description
          "Enables TI-LFA computation.";
      }
...
    leaf use-segment-routing-path {
      if-feature remote-lfa-sr;
      type boolean;
      description
        "force remote LFA to use segment routing
         path instead of LDP path.";
    }

The client may configure leaf enable to 'true' or 'false', in which case the meaning of the configuration is clear. The client may also omit giving these leafs any value. What happens then? You could explain what happens in the description, e.g. "When left without a value, the system will bla bla...". Or you could add a default statement, resolving any ambiguity, e.g. default false. Or you could declare the leaf mandatory, in which case the client would be forced to make a choice, i.e., mandatory true;

There are also many operational leafs that have this property, which makes it easier for server implementors to get away with implementations that omit returning some of these leafs. So if you think some of them are particularly important for clients, consider marking them mandatory, so that servers understand it's not ok to omit them.


#4: Indentation

The indentation of the module is apparently done by hand and a tad sloppy. By running the following pyang command, you get a file with the indentation cleaned up. It also removes unnecessary quotes and a few other things. In case you want full control over your own module, you can just diff the two to find places where the indentation might not be done properly.

pyang -f yang ietf-isis-sr\@2022-08-09.yang > ietf-isis-sr\@2022-08-09-indented.yang


#5: List keys not going first

The module contains four lists with keys. In three of them, the definitions of the key element does not go first in the list. Here, for example, there is a leaf af before leaf value (the key). This is perfectly valid YANG, but server implementors MUST return the key first in the NETCONF payload. Therefore it is customary to define the key(s) first inside the list (in the same order as in the key statement), to adhere to the principle of least surprise.

    list adjacency-sid {
      key value;
      config false;
      leaf af {
        type iana-rt-types:address-family;
        description
          "Address-family associated with the
           segment ID";
      }
      leaf value {
        ...


#6: Keyless lists

There are two keyless lists in the module. Keyless lists are allowed in YANG for operational data. But that they are allowed does not necessarily mean they are a good idea.

        list global-block {
          description "Segment Routing Global Block.";
          leaf range-size {
            type uint32;
            description "The SID range.";
          }
          uses sid-sub-tlv;
        }
...
      list local-block {
        description "Segment Routing Local Block.";
        leaf range-size {
          type uint32;
          description "The SID range.";
        }
        uses sid-sub-tlv;
      }

If there is no natural key for these lists and there aren't too many elements in them in a typical system, then maybe leaving the list as keyless may be fine. But if you can have hundreds or thousands of these list elements, keyless lists are problematic. It's basically impossible for interested clients to read anything else than the entire list every time when there is no key. There is no way to identify list entries that have been added, removed or changed, so all this data has to be read every time. If the data might be large, adding a key (natural or synthetic) would improve performance.


#7: Unclear+open ended string format

The leaf fec has a string type and a pretty vague description. What is the valid format for this leaf? Different implementors will probably allow different formats here, rendering the module non-interoperable, despite efforts to standardize.

      leaf fec {
        type string;
        description
        "IP (v4 or v6) range to be bound to SIDs.";
      }

Worse, since this is a key, a client might specify multiple list entries with different strings that map to the same underlaying range. For example, the YANG model currently allows a client to configure all of these as distinct list entries:

"2001:0000::47/50"
"2001:0::47/50"
"2001::47/50"
"::1.2.3.4"
"::1:2:3:4"
"0::1:2:3:4"

Sorry if I got the format wrong, I have to guess here :-) Hopefully you get my point anyway. Changing to a more precise type would certainly be a good idea, and particularly so for a key leaf.


##