Subject: Re: [Teas] Yangdoctors early review of draft-ietf-teas-yang-te-types-01

From: Lou Berger <[email-masked]>
Date: Thu, 01 Nov 2018 18:46:46 -0400

Jan,

Thanks for the quick turnaround on this!  I'll wait for the comments to be 
resolved before submitting for publication.  See below for one specific 
response.

----------
On October 30, 2018 9:57:35 AM Jan Lindblad <[email-masked]> wrote:

> Reviewer: Jan Lindblad
> Review result: Ready with Issues
>
> This is my YANG Doctor early review of draft-ietf-teas-yang-te-types, or more
> specifically ietf-te-types.yang . I find the module ready with a couple of
> issues. I have also noted a few nits. Generally speaking, I'm not sure if
> reviewing a -types module with plenty of groupings on its own is the best way
> to go about it. The groupings defined here may be used in good or not so good
> ways later.

You can see how the types/groupings are be used in docs listed on the 
following page:

https://datatracker.ietf.org/doc/draft-ietf-teas-yang-te-types/referencedby/

Thanks again,
Lou
>
> General questions:
>
> #1: There are many locations in the YANG talking about an "ERO subobject index"
> (and once RRO index, record route subobject). What is this, and how is it
> supposed to be used? The document is silent on this matter, and I have seen
> modules with problems around numeric index leafs much like this earlier. Are
> these numbers stable, i.e. remains the same forever?
>
> #2: There are few leafs (5) with default values given, and none with mandatory.
> Probably needs to increase before we get to last call.
>
> #3: There are several choices in the module that are meant to be augmented with
> additional cases. In many instances, this is explicitly spelled out, very good.
> If this is meant to happen in all choices, it would be nice to point this out
> in every instance. Also, if there are any specific assumptions or
> considerations to keep in mind when augmenting in a new technology, please note
> that in the description as well.
>
> Issues and nits:
>
> #4: Unclear data type
> 419:
>   typedef admin-group {
>     type binary {
>       length 4;
>
> What is the format of this binary? If this is always a 4-byte binary, wouldn't
> a numeric type be more user friendly, e.g. uint32?
>
> #5: identifier in container with optional leafs
> 1496:
>   grouping te-topology-identifier {
>
> The name suggests this is used as an identifier, but all the leafs are
> optional. This is not typical. They are also in a container, precluding them
> from being used as list keys. Is that as intended?
>
> #6: Optional -id leafs again
> 1700:
>           leaf node-id {
>           leaf link-tp-id {
> 1768:
>         leaf address {
> 1783:
>         leaf node-id {
>         leaf link-tp-id {
>
> Leafs that appear to be used as identifiers are optional
>
> #7: binary length in bits?
>
> 1731:
>            leaf as-number {
>             type binary {
>               length 16;
> 1773:
>         leaf ip-flags {
>           type binary {
>             length 8;
> 1805:
>           leaf label-flags {
>             type binary {
>               length 8;
>
> It appears to me the modeler might have thought the length is given in bits.
> The value of length is in bytes, however.
>
> #8: Must expression copy paste
> 1852:
>     container label-end {
>       must "not(../label-end/te-label/direction) or "
>         + "not(te-label/direction) "
>         + "or ../label-end/te-label/direction = te-label/direction" {
>
> This must expression appears to have been copied from label-start. In any case,
> it always evaluates to true and has no effect.
>
> #9: Unclear bit field
> 1885:
>     leaf range-bitmap {
>       type binary;
>       description
>         "When there are gaps between label-start and label-end,
>          this attribute is used to specify the positions
>          of the used labels.";
>     }
>
> Need more information on how to interpret this leaf. Which bits map to what,
> and what does the bit field values 0 and 1 indicate?
>
> #10: Canonical representation
> 67:
>   typedef te-bandwidth {
>
> The type is based on a string with a pattern allowing hex characters and an
> upper or lowercase P. Since the pattern allows multiple representations of the
> same underlaying value (0x1p10 presumably means the same as 0x1p0xa and
> 0x1P0XA) the question comes up if there is a canonical representation of this
> value, e.g. using all lowercase and all hex, or if the string must be
> remembered exactly as given by the client. The description could answer this
> question.
>
> #11: Mix of upper and lowercase
> The module specifies many enumeration and identity values. Some are all
> lowercase. Some are all uppercase. The principle of least astonishment suggests
> to pick one and stick with it. YANG recommendations suggest to use all
> lowercase when in doubt.
>
>   typedef te-link-direction {
>   typedef te-label-direction {
>   typedef te-hop-type {
>   identity LSP_METRIC_TYPE {
>   identity LSP_METRIC_RELATIVE {
>   identity LSP_METRIC_ABSOLUTE {
>   identity LSP_METRIC_INHERITED {
>
> Best Regards,
> /jan
>
>
> _______________________________________________
> Teas mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/teas
>

---

From: Jan Lindblad <[email-masked]>
Date: Fri, 2 Nov 2018 07:17:34 +0100

Lou,

> Thanks for the quick turnaround on this!  I'll wait for the comments to be resolved before submitting for publication.  See below for one specific response.
...
> You can see how the types/groupings are be used in docs listed on the following page:
> 

Very good. I didn't know if the referencing modules were ready for review, or if there might be additional referencing modules in the works. Anyway, my point is to suggest to bundle the next review of this module with (one of) the modules that use it.

/jan

---

From: "Tarek Saad (tsaad)" <[email-masked]>
Date: Sat, 3 Nov 2018 05:27:35 +0000

Hi Jan,

Thanks much for the thorough review and comments. Please see inline for responses and intended closure. Let us know if you have further comments/concerns.

-----Original Message-----
From: Jan Lindblad <[email-masked]<mailto:[email-masked]>>
Date: Tuesday, October 30, 2018 at 8:56 PM
To: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Cc: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Subject: Yangdoctors early review of draft-ietf-teas-yang-te-types-01
Resent-From: <[email-masked]<mailto:[email-masked]>>
Resent-To: Tarek Saad <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, <[email-masked]<mailto:[email-masked]>>, Lou Berger <[email-masked]<mailto:[email-masked]>>
Resent-Date: Tuesday, October 30, 2018 at 8:56 PM

    Reviewer: Jan Lindblad

    General questions:

[TS]: The list of explicit route objects defines a TE path (e.g. hops), where the index is used as a key to identify a specific entry in the list. A lower index implies the TE path traverses this entry earlier. We will clarify the description and remove mention of “subobject” in the next update to the document.

    #2: There are few leafs (5) with default values given, and none with mandatory.
    Probably needs to increase before we get to last call.
[TS]: In general, the team tried to avoid setting defaults unless it is strictly dictated by another RFC/standard. The team will review again to see if any was missed and will take action.

[TS]: yes, we will update as necessary.

    Issues and nits:

    #4: Unclear data type
    419:
      typedef admin-group {
        type binary {
          length 4;

    What is the format of this binary? If this is always a 4-byte binary, wouldn't
    a numeric type be more user friendly, e.g. uint32?
[TS]: the standard defines admin group as 4-octet binary (9 - Administrative group (4 octets) ), and for extended-admin-group as series of those. We are also defining a union to encompass both as admin-groups below. Do you still have concerns on this?

  typedef admin-groups {
    type union {
      type admin-group;
      type extended-admin-group;
    }
    description "TE administrative group derived type";
  }

    The name suggests this is used as an identifier, but all the leafs are
    optional. This is not typical. They are also in a container, precluding them
    from being used as list keys. Is that as intended?
[TS]: this container is not meant to be reused for a list, rather contains ID(s) that together can be used to lookup a specific topology that the TE tunnel is using. We can clarify this in the description

    Leafs that appear to be used as identifiers are optional
[TS]: such IDs identify a specific node, link, or address in the network but in our applications we do not to use them as keys in such lists. For example list explicit-route-objects defines a separate index leaf to be used as key -- since, a path may contain the same node-id or address repeated multiple times (looping path)..

    It appears to me the modeler might have thought the length is given in bits.
    The value of length is in bytes, however.
[TS]: good catch, thanks. We'll correct it to indicate number of octets as RFC6020 defines.

    This must expression appears to have been copied from label-start. In any case,
    it always evaluates to true and has no effect.
[TS]: thanks, we can see the bug in the expression as is, and this will be fixed/updated.

        container label-end {
          must "not(../label-start/te-label/direction) or "
            + "not(te-label/direction) "
            + "or ../label-start/te-label/direction = te-label/direction" {

    #9: Unclear bit field
    1885:
        leaf range-bitmap {
          type binary;
          description

    Need more information on how to interpret this leaf. Which bits map to what,
    and what does the bit field values 0 and 1 indicate?
[TS]: ]: Each bit-position in the bitmap will map to an offset from the label-start. For example, if label-start=16000 and bitmap=0x11, then labels={16000, 16004} are relevant. We will clarify this in the next update.

[TS]: We will update the description to indicate more strict canonical form (all upper case).

[TS]: we will update to all lower-case to align with YANG recommendations.

    Best Regards,
    /jan

Regards,

Tarek

---

From: Jan Lindblad <[email-masked]>
Date: Mon, 5 Nov 2018 10:19:51 +0100

Tarek,

See my comments below [janl].

> [TS]: The list of explicit route objects defines a TE path (e.g. hops), where the index is used as a key to identify a specific entry in the list. A lower index implies the TE path traverses this e…

[janl] Very good re description changes.

I'm a little worried that numeric indices will end up in the same quagmire+impasse where ACLs have been the last decades. Isn't there a risk that people will want to insert entries between two consecutive indices? Is "renumber" an operation that someone might ask for on these indices?

> [TS]: In general, the team tried to avoid setting defaults unless it is strictly dictated by another RFC/standard. The team will review again to see if any was missed and will take action.

[janl] That's all fine. Just remember to describe what happens when a leaf has no value every time there is no default or mandatory. Easy to forget, experience shows. Missing this causes non-interoperability in the standard.
    
> [TS]: yes, we will update as necessary.

[janl] Great!
    
>     description "TE administrative group derived type";

[janl] I'm have two concerns. The first is that I don't know what the format of the type is. How do I know how to construct one of these values, or interpret an existing one? Is this a randomly chosen value? The other concern is how an operator would enter this value. The binary type is not the most user friendly of types. A numeric representation in the management interface might make better sense.

> [TS]: this container is not meant to be reused for a list, rather contains ID(s) that together can be used to lookup a specific topology that the TE tunnel is using. We can clarify this in the desc…

[janl] Hmm, ok. Still not sure what happens if you omit some value(s), but I guess I should wait to see your clarification.
    
> [TS]: such IDs identify a specific node, link, or address in the network but in our applications we do not to use them as keys in such lists. For example list explicit-route-objects defines a separ…

[janl] So what happens if you omit specifying some of them?
    
>             + "or ../label-start/te-label/direction = te-label/direction" {

[janl] This revised must statement is superfluous. The must statement on container label-start already covers this case.
    
> [TS]: ]: Each bit-position in the bitmap will map to an offset from the label-start. For example, if label-start=16000 and bitmap=0x11, then labels={16000, 16004} are relevant. We will clarify this…

[janl] Very good. Since you are using type binary and refer to the contents using integers, you should also specify how you map those integer values to bit positions in the binary. E.g. big-endian?

> [TS]: we will update to all lower-case to align with YANG recommendations.

[janl] Great!

/jan