Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-sr-yang-13

[Yingzhen Qu <[e-mail]>]

Hi Jan,

Thank you for the review, really appreciate. Please see my answers below inline.

Version -14 has been submitted including all the suggested changes.

Thanks,
Yingzhen

> On Aug 17, 2022, at 8:49 AM, Jan Lindblad via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Jan Lindblad
> Review result: Ready with Issues
> 
> 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";
>  }
[Yingzhen]: You’re right about the dependency here. We’ve been waiting for the publication
of the base ISIS model, which has a dependency of https://datatracker.ietf.org/doc/draft-ietf-bfd-rfc9127-bis/ <https://datatracker.ietf.org/doc/draft-ietf-bfd-rfc9127-bis/>.
Now they’re close to be published, so we requested the review of this document to get it ready.
I added a place holder for the reference as you suggested.

> 
> #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.
[Yingzhen]: Thanks for pointing this out. I’ve changed the model to use relative path.
Please kindly let me know if you see any issues.
> 
> #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.
> 
[Yingzhen]: I added default value for these two leaves. 

> #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
> 
[Yingzhen]: fixed.

> #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 {
>        …
[Yingzhen]: fixed as suggested.

> #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.
> 
[Yingzhen]: Thanks for the suggestion. However for these two specific lists, the number
of elements are expected to be a few, or only one or two, so I suppose keyless is ok.

> #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.
> 
[Yingzhen]: I looked at RFC 8667 and changed this to ip-prefix. Thanks for catching this.
> ##
> 
> 
>