Subject: [IPFIX] review of YANG module ietf-ipfix-psamp@2010-10-25

From: Ladislav Lhotka <[email-masked]>
Date: Mon, 13 Dec 2010 16:38:56 +0100

Hi,

this is my review of the ietf-ipfix-psamp@2010-10-25 module:

*** General comments

This module is probably the largest (public) data modelling effort
that has been done with YANG so far. It is therefore also important as
a serious test case for YANG itself.

The module demonstrates that object-based data modelling can be done
with the current YANG syntax in an elegant and extensible way. I like
the approach of mapping UML diagrams to YANG structures where object
properties are represented by groupings, aggregated components as
subordinate nodes and unidirectional associations with leafrefs.

The module closely obeys all the recommendations of
draft-ietf-netmod-yang-usage-11.

My main concern is the size of the module. I believe that splitting the
data model into smaller coherent modules makes it more understandable
and manageable - also in terms of the standardisation process. It
seems that the present module could relatively easily be divided into
two modules, one describing the collector subsystem and the other
dealing with the remaining functionality. As a matter of fact, flow
collectors are in most cases implemented in a separate device, so
their configuration won't be mixed with configurations of the other
parts. This change would also remove one level of containment in the
schema, at a very reasonable price of adding one more namespace.

The module could certainly make a good use of a generic model of
network interfaces.

*** Specific comments

- Some description statements only retell in prose what is succinctly
  expressed by other statements. Such description statements should
  be removed. For example the type description in
    typedef ieIdType {
      type uint16 {
        range "1..32767" {
          description "Valid range of Information Element
              identifiers.";
          reference "RFC5102, Section 4.";
        }
      }
      description "Type for Information Element identifiers.";
    }

- Several leafs with string type could be further restricted:
  + The length of "ifName" is restricted to 255 characters.
  + Distinguished names of certification authorities and subjects also
    have a structure and a set of reserved characters - of course, a DN
    datatype would normally be imported from an external module, but
    it doesn't exist yet.
  + I am not sure about "ieName" - is it a completely arbitrary string?

- The description of "ieEnterpriseNumber" leafs says: "If omitted or
  zero, the Information Element is registered in the IANA registry of
  IPFIX Information Elements." Why not define a default value of "0"
  here (perhaps in a special typedef)?

Lada

-- 
Ladislav Lhotka, CESNET
PGP Key ID: E74E8C0C

---

From: Andy Bierman <[email-masked]>
Date: Mon, 13 Dec 2010 08:46:10 -0800

Hi,

I do not agree with removing description-stmts.
They are not required for range-stmt, but they are for typedef=stmt.
Strings intended for humans to read are important.
They may get displayed in help text and translated to other languages.

Andy

-----Original Message-----
From: [email-masked] [mailto:[email-masked]] On Behalf Of Ladislav Lhotka
Sent: Monday, December 13, 2010 7:39 AM
To: [email-masked]
Cc: YANG Doctors
Subject: [yang-doctors] review of YANG module ietf-ipfix-psamp@2010-10-25

Hi,

*** General comments

*** Specific comments

Lada

-- 
Ladislav Lhotka, CESNET
PGP Key ID: E74E8C0C

_______________________________________________
yang-doctors mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/yang-doctors

---

From: Ladislav Lhotka <[email-masked]>
Date: Mon, 13 Dec 2010 19:29:50 +0100

> They are not required for range-stmt, but they are for typedef=stmt.

Yes, I meant only the description inside the 'type' statement, not the one for 'typedef'.

Lada

--
Ladislav Lhotka, CESNET
PGP Key ID: E74E8C0C

---

From: Gerhard Muenz <[email-masked]>
Date: Sun, 19 Dec 2010 11:49:03 +0100

Dear Lada,

Thanks a lot for your valuable comments.

Please find my replies inline.

Gerhard

> schema, at a very reasonable price of adding one more namespace.

With the upcoming IPFIX mediators, we will have devices implementing 
Collecting Processes, Intermediate Processes, and Exporting Processes.
Therefore, I think that keeping the configuration data of all IPFIX 
processes in one module makes sense.

>        description "Type for Information Element identifiers.";

Please note that this is not a description for type but for range!
I want to motivate the range restriction for ieIdType and provide a 
reference to the corresponding section in RFC5102.

>      datatype would normally be imported from an external module, but

Is there somebody who feels capable of providing a regular expression 
for DN that covers all valid cases?

>    + I am not sure about "ieName" - is it a completely arbitrary string?

I am not aware of any restriction (apart from white spaces, which should 
probably be excluded). I have to check.

>    here (perhaps in a special typedef)?

I'm not sure whether 0 is a valid enterprise number.

In http://www.iana.org/assignments/enterprise-numbers , we have:

Decimal
| Organization
| | Contact
| | | Email
| | | |
0
   Reserved
     Internet Assigned Numbers Authority
       iana&iana.org

Does this mean that 0 can be used to refer to IANA?
If yes, we could use 0 as a default value. Otherwise, I would rather 
remove "or zero" and create a type that does not allow zero values.

---

From: Benoit Claise <[email-masked]>
Date: Mon, 20 Dec 2010 10:37:52 +0100

Hi,
> processes in one module makes sense.
I agree with Gerhard here.

Regards, Benoit.

---

From: Ladislav Lhotka <[email-masked]>
Date: Mon, 20 Dec 2010 15:11:42 +0100

> processes in one module makes sense.

All functions could be combined anyway, even if they are defined in
different modules, by advertising all modules that apply in the hello
message rather than having just one module and using various
combinations of features. By way of analogy, entire router configuration
could also be specified in one module and various routing protocols
selected by means of features - I just don't think this is the right
approach.
 
> reference to the corresponding section in RFC5102.

The 'reference' is fine but otherwise the 'range' statement says it all.

> remove "or zero" and create a type that does not allow zero values.

I don't know but the text as it is now sounds exactly like zero is the
default value for this leaf.

Lada

-- 
Ladislav Lhotka, CESNET
PGP Key ID: E74E8C0C