Subject: [IPFIX] WGLC for draft-ietf-ipfix-mib-variable-export-05

From: Nevil Brownlee <[email-masked]>
Date: Tue, 15 Apr 2014 12:01:11 +1200

Hi all:

Sorry to be a little slow with this - the dust has settled from IETF-89,
and I've almost managed to catch up.

The WG Last Call for our MIB Variable Export draft starts now,
and will end on Wednesday, 30 April.

Please read it, and - better still - post reviews of it to the IPFIX list!

Cheers, Nevil

-- 
---------------------------------------------------------------------
  Nevil Brownlee                    Computer Science Department | ITS
  Phone: +64 9 373 7599 x88941             The University of Auckland
  FAX: +64 9 373 7453   Private Bag 92019, Auckland 1142, New Zealand

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Fri, 18 Apr 2014 18:21:17 +0200

> Please read it, and - better still - post reviews of it to the IPFIX list!

I have reviewed the document and I believe it is not ready. The main
points:

- Terminology needs to be aligned with SMIv2
- I spotted several errors in the examples
- Major issues with the IE definitions in section 10
- I also believe the introductionary text needs to improve.

/js

I have reviewed draft-ietf-ipfix-mib-export-05 and I think it is not
ready and requires improvements.

1) The introduction contains details of the solution. I think it
   should instead contain the motivation and the architectural model
   currently in section 2. The new introduction should then be
   followed by a terminology section before an overview of the
   solution is provided (so that the terminology is defined). This is
   primarily text reorganization.

2) I have trouble to understand the Figure "Architectural Overview".
   I think this should be removed. Figure 4 is more useful and it is
   at the right place in the document.

3) I have terminology issues in several places. For example, RFC 2578
   uses the term 'columnar object' for what this document seems to
   call 'indexed object'. Why is it useful to call Flow Records Data
   Records? Using two terms for the same thing may just adds potential
   for confusion. I also think more SMIv2 terms must be imported, such
   as MIB module, MIB object, columnar object, ...

4) There is a statement that providing index information for columnar
   objects is optional. I have some difficulty to understand why this
   is useful. What does "a MIB object is used purely as type
   information" mean? How can an exporting process not have access to
   index information of a columnar object?

5) The terminology sections says a "MIB Object Identifier" is an ASCII
   character string of a certain format but it seems later a BER
   encoded representation is used for mibObjectIdentifier. This seems
   inconsistent.

6) "exporting a value from a MIB does not imply that the SNMP process
   on the device supports that MIB"

   Proper wording would be something like this:

   "exporting a value of a MIB object defined in a certain MIB module
   does not imply that the SNMP prcess on the device supports that
   MIB module"

   Similarly, "exporting MIBs in IPFIX" should be "exporting values of
   MIB objects".

   The paragraph on page 9 is also the first occurance of the term
   "MIB Field" - without a clear definition what this means.

7) This text needs rewrite:

   Since these values are statically defined in the MIB they are not
   expected to change frequently.  However the additional information
   about the MIB may help a Collecting Process that does not have access
   to the MIB.

   NEW

   Type information is statically defined in a MIB module, it is not
   expected to change.  However, the additional information about the
   MIB Object may help a Collecting Process that does not have access
   to the MIB module.

8) Is it mibTypeOption of mibSubTypeOption? Both words are used, or
   are these different things?

9) I fail to parse/understand these statements:

     Forf each of these the options this draft specifies exactly which
     mibObjectValue to use.

   [...]

     If the SYNTAX clause contains a Textual Convention or Subtyping the
     mibObjectSyntax Information Element SHOULD be used to export this
     detail to the Collecting Process.

10) It escapes me how mibObjectValueSequence and mibObjectValueTable
    are used / encoded. And why does the first one use an ASN.1 type
    name while the second uses an SMIv2 conceptual term? This seems
    inconsistent, either call them mibObjectValueSequence and
    mibObjectValueSequenceOf or mibObjectValueRow and
    mibObjectValueTable.

11) This text is unclear and needs a rewrite. I think I understand
    what you are trying to communicate but the wording needs
    improvements. (And it is a MIB module here, not a MIB.)

     This holds true even if the data carried inside the mibObjectValue or
     mibObjectIdentifier may be related to an enterprise specific MIB.
     The OID itself encodes if the Object is in an Enterprise specific MIB
     Module.

12) What does "Field Length (mib)" stand for? Is there anything
    special or is this just a plain normal "Field Length"? If so, I
    would rather remove the "(mib)" annotation.

13) I suggest to replace "more information about MIB indexing, extra
    data from the MIB" with "additional information about the MIB
    Object definition"

14) Rewrite "... a reference to a MIB that ..." - I think you mean a
    MIB Object.

    Rewrite "may not have access to the MIB" -> "may not have access to
    the MIB module".

    And then I am getting lost here:

      [..] It also allows the IPFIX Field Types to be extended with
      any MIB Variable already defined purely through IPFIX.

    It is not clear what is communicated here.

15) Section 4.4.6 has more inaccurate usages of the term "MIB" but
    more important, I wonder what actually the OID is that is being
    shipped in mibObjectIdentifier. Is it the OID assigned to the
    object type definition or is it the OID of the instance of an
    object type? Have I overlooked where this is clearly defined?

16) Terminology: "MIB Sequence Object's INDEX clause" -> "INDEX
    clause of a conceptual row object".

17) s/then the MIB context MUST/then the context MUST/

    MIB context is not a defined term. There are only SNMP contexts.
    (Perhaps they should have been MIB contexts but this is a
    different story.)

18) s/the exported to export/the exporter to export/

19) Section 4.8 is rather confusing, primarily because the terminology
    is not aligned with SMIv2 terminology. This affects almost all
    sentences. In SMIv2, terms like "MIB SEQUENCE Objects" do not
    exist. SMIv2 calls this a 'conceptual row object'. This section
    needs to be rewritten to align with SMIv2 terminology (this means
    getting rid of almost all occurrences of 'sequence' (in any
    capitalization)). Terms such as "full OID" are ambiguous unless
    you say what the OID is supposed to refer to.

    The first item in 4.8 talks about ingoring the indexing of
    columnar objects and later says "a columnar object may be used
    purely as a data type. I have a difficulty to understand how that
    would be useful.

    On page 33, a paragraph refers to section 7.7. of RFC 2578 and I
    am confused what it tries to say here. So far, I assumed that the
    OIDs all refer to columnar objects or conceptual row objects and
    include no instance identifiers.

    I do not understand the last paragraph before section 5.

20) The first enumerated item in Section 5.1 says "flowStartSeconds
    from [RFC7012]" but then RFC 7012 does not define this anymore
    (but its predecessor RFC 5102 did). Since Section 5.2 has the same
    text, the same issue needs to be resolved there.

21) Sections 5.1. and 5.2 both contain references to themselfs "(see
    Section 5.x)" which does not seem to make much sense. Last sentence
    on page 38: s/this identical/this is identical/

22) I looked up the definition of cpmCPUTotal1minRev and it turns out
    that this is a columnar object (which makes sense since you can
    have multiple CPUs). Since you do not export information about
    which CPU the load value is coming from, how useful is the
    information and does this make a good example?

23) Section 5.3. needs a title change. It is about exporting a subset
    of a conceptual row. Perhaps something like this:

    5.3.  Exporting a Conceptual Row: The OSPF Neighbor Sequence

    The last sentence on page 40 refers to Table 7 but I assume you
    mean Table 5.

24) The BER encoding of 1.3.6.1.2.1.14.10.1 is 06082B060102010E0A01
    (10 octets) and not the 22 octets shown in Figure 28.

25) Section 5.4. also needs a better title, e.g.

    5.4.  Exporting an Augmented Conceptual Row: ifTable and ifXTable

    The text in section 5.4 confuses ifXEntry and IfXEntry. A possible
    rewrite:

      The ifTable defined in the IF-MIB [RFC2863] is augmented by the
      ifXTable (defined in the same MIB module). The OID of the
      ifEntry is 1.3.6.1.2.1.2.2.1 while the OID of the augmenting
      ifXEntry is 1.3.6.1.2.1.31.1.1.1. This example demonstrates how
      columnar objects from the base conceptual row and the augmenting
      row can be exported in a single mibObjectValueSequence data
      record.

26) Why is the Field Length of mibObjectValueOctetString (on page 46)
    16 octets? The ifName's restriction is 255 octets. And should this
    not be variable length?

27) page 47: s/VLAN=11/VLAN=10/

    On page 48, I am surprised that ifType and ifMTU are both 16-bit
    fields. Does the template not say they are 4 octets long?

28) Section 5.5. also needs a new title and the terminology used is
    wrong as well. Perhaps it is:

    5.5.  Exporting a Columnar Object: ipIfStatsInForwDatagrams

    Please do not write 'ipIfStatsTable SEQUENCE'. The correct SMIv2
    term is conceptual table. (And also note that the ASN.1 type of a
    table is a SEQUENCE OF and not a SEQUENCE.)

    The template in Figure 33 says the interface index is two octets.
    This may be true for a certain exporter but may not be generally
    true - it may be worth to be explicit about the assumption made
    here that all possible interfaces are numbered such that they fit
    into 16 bits.

    The BER encodings and the corresponding VLEN fields are all wrong
    in Figure 35:

    1.3.6.1.2.1.4.31.3.1.1  -> 060A2B06010201041F030101 (12 octets)

    1.3.6.1.2.1.4.31.3.1.2  -> 060A2B06010201041F030102 (12 octets)

    1.3.6.1.2.1.4.31.3.1.12 -> 060A2B06010201041F03010c (12 octets)

29) Section 5.6. needs a better title. This is about a columnar object
    where the index information is provided by IPFIX information
    elements. 

    5.6.  Exporting a Columnar Object index by IEs: ifOutQLen

    s/be done be/be done by/

    The text below Table 8 needs to be rewritten, e.g.

    The ifOutQLen MIB object defined in the IF-MIB [RFC2863] provides
    the length of the output packet queue. This columnar object is
    part of the ifEntry conceptual row and indexed by the interface
    index (ifIndex).

    I am not sure I agree on the second paragraph on page 53. I tend
    to believe that the index information must be provided for generic
    applications to make sense out of the data.

    The BER encoding and the corresponding VLEN in Figure 39 is wrong:

    1.3.6.1.2.1.2.2.1.21 -> 06092B0601020102020115 (11 octets)

30) I find the example in section 5.7 rather strange. Why would one
    export the ifIndex value using mibSubIdentifier and not by
    including the ifIndex proper? There does not seem to be any
    savings and this mibSubIdentifier approach of course is only
    applicable where the number of sub-identifier is constant for all
    conceptual rows. Since I do not see that this approach is needed
    nor that it adds any value in terms of more compact encodings,
    I would actually prefer this to be removed and perhaps even be
    disallowed.

    The BER encoding of the ifOutQLen OID is wrong, see above for the
    correct value.

    (The caption of Figure 44 is kind of strange because it says
     "using ifIndex" but then the example is about not using ifIndex
     but instead an opaque number.)

    This section 5.7. should really be removed I think.

31) In section 5.8,

    s/ospfNbarEntry/ospfNbrEntry/

    The OID encoding and the VLEN field in Figure 46 is wrong:

    1.3.6.1.2.1.14.10.1 -> 06082B060102010E0A01 (10 octets)

    RFC 3411 uses '800002b804616263'H as an snmpEngineID in examples.

32) What is the meaning of this:

      If a Collecting Process receives a MIB Object Identifier that it
      cannot decode, it SHOULD log an error.

    What does 'cannot decode' mean? I mean, a simple collector may
    just store the records in some file / database. So what is
    expected here? And why is it an error and not a warning?

    I am also not sure what the last paragraph in section 7 tells me.
    What does 'purely semantic information' mean? For me, if you miss
    the semantics, the data has no value. But I understand that IPFIX
    people have a very different terminology at times.

33) It is unclear to me how the export of conceptual rows deals with
    non-existing variables (aka table holes). Is there a mechanism in
    IPFIX to indicate that a certain field of a flow record does not
    have a value?

Issues with section 10:

a) In the SMIv2, we distinguish between Counter32 and Counter64 and
   I think this difference important to capture since the rollover
   is different. The I-D seems to map both to mibObjectValueCounter.

b) The descriptions all use the phrase "from a MIB" but I think it
   should be "of a MIB object".

c) The SMIv2 type is IpAddress and not IPAddress.

d) The definition of mibObjectValueCounter says "Data Type Semantics:
   totalCounter". I think this is wrong since SMIv2 counters do not
   start with zero. Furthermore, a Counter32 rolls over after 32-bit
   and not after 64-bit (but mibObjectValueCounter is an unsigned64).

e) The definition of mibObjectValueGauge "Data Type Semantics:
   totalCounter". I think this is wrong, a Gauge32 is not a counter.
   And it might be useful to call this mibObjectValueGauge32.

f) Why did you call mibObjectValueTime not mibObjectValueTimeTicks?
   The "Abstract Data Type: dateTimeMilliseconds" also seems to be
   wrong since TimeTicks are _not_ measured in milliseconds since
   1970-01-01T00:00. You may want to use unsigned32 and spell out the
   semantics.

g) mibObjectValueUnsigned used the "Abstract Data Type: unsigned64"
   which is wrong since there is only a 32-bit unsigned type in the
   SMIv2. I suggest to use unsigned32 and to rename this to
   mibObjectValueUnsigned32.

h) The phrase "a complete MIB 'SEQUENCE OF X' or conceptual table
   value" reads strange. Similarly, "a MIB SEQUENCE or a row from a
   conceptual table" reads strange. Perhaps simply use "a complete
   conceptual table" and "a row of a conceptual table".

i) The lest sentence of the Description in 10.1.11 seems to be missing
   some words.

j) The description of 10.2.2 is somewhat confusing: "... that serves
   as INDEX MIB Objects of Information Elements for a mibField". In
   SMIv2, the INDEX is associated with a conceptual row.

k) What does "sampled by SNMP" mean? Do you hook into the
   instrumentation or do you access things via the SNMP agent?

l) p74: s/the MIB the Flow/the MIB when the Flow/

   (Note that you are talking about a flow here and not about a data
   record. I think this is goodness and proves that a new term is not
   needed.)

m) Replace the Description in 10.3.4 with this:

      Description: The textual name of the MIB module that defines a MIB
      Object.

n) I do not understand what mibObjectSyntax would contain for an object
   representing a conceptual table or a conceptual row. For say ifEntry,
   would it be just "SEQUENCE {
        ifIndex                 InterfaceIndex,
	ifDescr                 DisplayString,
	-- lots of stuff left out
   }"? And what would it be for the object representing the conceptual
   table? Or do you just mean what is literally in the SYNTAX clause,
   excluding the referenced ASN.1 type definitions?

   For other MIB Objects, do you include subtyping information such as
   range of length restrictions? What about named number enumerations
   of the BITS construct, you include the whole definition (mind you
   that RowStatus is really long).

o) Remove "( also known as snmpEngineID) that" since it is potentially
   misleading.

   Explanation: Every SNMP engine has a unique snmpEngineID. The
   combination of a snmpEngineID value and a context names provides
   the SNMP context. Note that an SNMP engine may provide access to a
   non-local context, in which case the values of snmpEngineID and
   contextEngineID would be different.

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1, 28759 Bremen, Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>

---

From: Benoit Claise <[email-masked]>
Date: Fri, 25 Apr 2014 15:32:12 +0200

Dear all,

I stopped my review at section 4.4.2 (spent a couple of hours already), 
and I'm afraid the draft is not ready.
Here is my feedback.

- Section 1

    For example,
    in the IPFIX information model [RFC7012  <http://tools.ietf.org/html/rfc7012>], Information Elements coming
    from the SNMP world have already been specified, e.g.,
    ingressInterface and egressInterface both refer to the ifIndex
    defined in [RFC2863  <http://tools.ietf.org/html/rfc2863>].

As Jürgen noted, RFC 7012 doesn't specify the IPFIX information elements 
any longer.
This is done in the IANA registry.

- Section 2

       In the best case, assuming very tight integration of
       an IPFIX Collector with and SNMP polling engine, SNMP data is
       retrieved shortly after Data Records have been received, which
       implies a delay of the sum of the active or inactive timeouts (if
       not null) plus the time to export the Flow Record to the
       Collector.

For active and inactive timeouts, provide a reference to RFC 5470 , 
section 5.1.1. "Flow Expiration"
Even if RFC 5470 doesn't call them active and inactive timeouts (btw, 
don't change that, these are well known terms), this is a good reference.

-  Section 2

    This draft does not specify SNMP notifications, even if the
    specifications in this document could potentially allow this.

draft -> document
Same remark for:

    For each of these the options this draft specifies exactly which
    mibObjectValue to use.

    This draft defines two forms of indexing that can be used for
    SEQUENCE MIB Objects.

- Section 2

    The
    simple and application-wide data types specified in SMIv2 [RFC2578  <http://tools.ietf.org/html/rfc2578>],
    along with a new Textual Conventions

Remove the s

- Section 3

  mibObjectValue

       Refers to any and all of the mibObjectValue Information Elements
       generically.  Any restriction or requirement in this document that
       refers to mibObjectValue applies to the following Information
       Elements defined inSection 10.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-10.1>: mibObjectValueInteger,
       mibObjectValueOctetString, mibObjectValueOID, mibObjectValueBITS,
       mibObjectValueCounter, mibObjectValueGauge, mibObjectValueTime,
       mibObjectValueUnsigned, mibObjectValueTable and
       mibObjectValueSequence.

Change mibObjectValue to mibObjectValue Information Elements.
This is how it's used in some sentences in the rest of the document.
However, I also see:  mibObjectValue Field or mibObjectValue field
I don't know what the term field refers to. Please change this to mibObjectValue Information Elements

- Section 4

    The Exporting process MAY extract the data values for mibObjectValue
    fields from a Process that resides on the same device or MAY capture/
    create the data required to match the definition of the MIB Object.
    In particular exporting a value from a MIB does not imply that the
    SNMP process on the Device supports that MIB.
  
Why is Process capitalized?
Also MAY to may. This is not a RFC 2119 keyword required for the implementation.

- Section 4
    The main issue that arises from exporting MIBs in IPFIX is that MIB
    Object Identifiers do not fit into the standard IPFIX Template format
    [RFC7011  <http://tools.ietf.org/html/rfc7011>], as this only provides a 16-bit Information Element
    identifier and the length of the Information Element.

I would remove "and the length of the Information Element", which doesn't add anything to this sentence

- Section 4

    One approach to this problem would be to extend the IPFIX standard to
    allow extended field specifiers so metadata about Fields can be
    included in Data Templates.

Fields -> field

- Section 4

    However, future versions of IPFIX MAY export the required MIB
    metadata as part of newer set versions.

MAY -> may.
Please review all RFC 2119 keywords

- Section 4
    This is a
    standard IPFIX Option Template Set that MUST include a minimum set of
    required fields (seeSection 4.2.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.1>) and MAY include extra fields to
    provide more meta information about one of the mibObjectValue fields.

While you're not yet in the specifications in Section 4, I would not introduce MUST and MAY here.
This also contradicts the REQUIRED and RECOMMENDED in section 4.2.1
Please group the RFC 2119 keywords in section 4.2.1

- Throughout the document
Option Template -> Options Template
Note: IIRC, there were inconsistencies in RFC 5101, which were corrected in RFC 7011

- Major issue: I confused by mibFieldOption
Is this a IE or an Options Template?

The TOC shows:
    4.2.  MIB Field Options - Specifications and Required Fields  .  11
        4.2.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.1>.  mibFieldOption  . . . . . . . . . . . . . . . . . . .12  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-12>
        4.2.2  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.2>.  mibSubFieldOption . . . . . . . . . . . . . . . . . .13  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-13>
        4.2.3  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.3>.  mibTypeOption . . . . . . . . . . . . . . . . . . . .13  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-13>
      4.3  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.3>.  IPFIX and MIB Data Model  . . . . . . . . . . . . . . . .14  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-14>
      4.4  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.4>.  MIB Field Option Template Formats . . . . . . . . . . . .15  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-15>
        4.4.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.4.1>.  Data Template containing a mibObject Field  . . . . .15  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-15>
        4.4.2  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.4.2>.  mibFieldOption Template . . . . . . . . . . . . . . .17  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-17>
        4.4.3  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.4.3>.  mibFieldOption Data Records . . . . . . . . . . . . .18  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-18>
        4.4.4  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.4.4>.  Option Template containing a mibObject Field  . . . .19  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-19>
        4.4.5  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.4.5>.  mibFieldOption Template with Semantics Fields . . . .20  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#page-20>
        4.4.6.  mibFieldOption Template with extra MIB Object Details  21

It's not clear!

   This document also defines three standard Option Templates (see
    Section 4.2  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2>) that are used as part of the mechanism to export MIB
    Object meta data:

    o  mibFieldOption (Section 4.2.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.1>)

    o  mibSubFieldOption (Section 4.2.2  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.2>)

    o  mibTypeOption (Section 4.2.3  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.3>)

Ok, so these are Options Templates.
If this is the case, change the TOC section titles

Then I see in figure 1
                                        +------------------------+
                                        | mibFieldOptionTemplate |
                                        +------------------------+
What is a mibFieldOptionTemplate ?

    This document defines a mibFieldOption Template to export the extra
    meta information required for a mibObjectValue field.

Well, this one above is wrong. This should be Options Template, right?

Then I see
   2.  A mibFieldOption Template Set

           The mibFieldOption Template describes which metadata will be
           sent for each mibObjectValue being exported.

These should be Options Template Set, right?

Then, I look at

        4.2.1. mibFieldOption

    Three fields are REQUIRED to unambiguously export a standalone
    mibObjectValue Field with a mibFieldOption:

Then I look at

        4.4.2. mibFieldOption Template

    The mibFieldOption Template is a Standard Option Template which
    defines the Fields that will be exported to provide enough metadata
    about a mibObjectValue so that the Collector can tie the data values
    in the mibObjectValue back to the definition of the MIB Object.

And I finally understand that mibFieldOption is an Options Template.

What is very confusing to me is that mibFieldOption is apparently simply an Options Template, like we have defined in IPFIX (http://tools.ietf.org/html/rfc7011#section-4.1) or  PSAMP (http://tools.ietf.org/html/rfc5476#section-6.5.1)
However, this Options Template is called like an IPFIX IE: mibFieldOption
This should be called "MIB Field Options Template" throughout the document.

OLD:
    This document also defines three standard Option Templates (see
    Section 4.2  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2>) that are used as part of the mechanism to export MIB
    Object meta data:

    o  mibFieldOption (Section 4.2.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.1>)

    o  mibSubFieldOption (Section 4.2.2  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.2>)

    o  mibTypeOption (Section 4.2.3  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.3>)

NEW:
    This document also defines three standard Option Templates (see
    Section 4.2  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2>) that are used as part of the mechanism to export MIB
    Object meta data:

    o  MIB Field Options Template (Section 4.2.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.1>)

    o  MIB SubField Options Template (Section 4.2.2  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.2>)

    o  MIB Type Options Template (Section 4.2.3  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.3>)

- Major: Why do we have ...
http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.1
http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.4.2
... with slightly different specifications?
As I mentioned previously, there are also some conflicting spec rules at the beginning of the section 4

-

     0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |          Set ID = 3           |          Length = 26          |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |        Template ID = 258      |        Field Count = 4        |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |  Scope field count = 2        |0| IE = templateId             |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |        Field Length = 2       |0| IE = informationElementIndex|
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |        Field Length = 2       |0| IE = mibObjectIdentifier    |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |        Field Length = 65535   |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

       Figure 19: Example of tcpCurrEstab mibFieldOption Template Set

This is not an Template Set, but an Options Template Set.
Same remark for all the examples.

- Section 4.2

Why a single in this sentence?
    For each mibObjectValue field that is defined in an IPFIX Template, a
    single mibFieldOption Data Record MUST be exported that provides the
    required minimum information to define the MIB object that is being
    exported (seeSection 4.2.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.2.1>).

What if we export with UDP? We might want to re-export this metadata information, without which the collector can't interpret the data.
This contradicts:
    Note that the ID of an identical mibFieldOptionTemplate which has
    already been exported MAY be reused without exporting the Template
    again.

- Section 4.2

    This mibFieldOption Data is defined in a template referred to in this
    document as a mibFieldOption Template with the format specified in
    Section 4.4  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.4>.

What's mibFieldOption Data?
Template -> Options Template

- Section 4.2

   _  A Template that uses a mibObjectValue Field MUST be exported prior to
    the corresponding mibFieldOption Data Records._   It is RECOMMENDED
    that these are all exported in the same IPFIX Message.  Note that
    this places an implicit size constraint on the export.

_    These MUST all be exported prior to the corresponding Data Records
    which depend upon them._   i.e. referring to Figure 1, the export order
    MUST be:

The two underlined sentences are redundant.

- Major
    While the following are optional, they are nevertheless RECOMMENDED
    in certain circumstances as they are per field:

You don't explain those circumstances

- I arrived at section 4.3, I'm not clear when I need to use mibSubFieldOption ormibTypeOption  

For mibSubFieldOption, this only thing I read so far is:
    To export a mibObjectValue that is contained in a
    mibObjectValueSequence Field with a mibSubFieldOption three fields
    are REQUIRED:

Not too clear.

- Major, Section 4.4.1

    Multiple
    mibFieldOption that refer to a single mibObjectValue MUST NOT be
    exported.

What is Multiple mibFieldOption?
Multiple Data Records. No
Multiple different Data Records. I agree
Multiple Options Template Records. No
Multiple different Options Template Records. I agree
  
- Major, Section 4.4.1

OLD:
     0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |          Set ID = 2           |          Length               |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |       Template ID             |         Field Count = 2       |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |0| IE = Existing IPFIX Field   |        Field Length           |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |0| IE = mibObjectValueInteger  |        Field Length (mib)     |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

           Figure 5: IPFIX Template Set using mibObjectValue Field

NEW:
     0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |          Set ID = 2           |          Length               |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |       Template ID             |         Field Count = 2       |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |0| IE = Existing IPFIX Field   |        Field Length           |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |0| IE = <mibObjectValue>      |        Field Length (mib)     |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

           Figure 5: IPFIX Template Set using mibObjectValue Field

Then you have to explain that <mibObjectValue> could be
      mibObjectValueInteger,
       mibObjectValueOctetString, mibObjectValueOID, mibObjectValueBITS,
       mibObjectValueCounter, mibObjectValueGauge, mibObjectValueTime,
       mibObjectValueUnsigned, mibObjectValueTable and
       mibObjectValueSequence.

I stopped my review at section 4.4.2, but plenty of my feedback would apparently be equivalent for the rest of the draft.

Regards, Benoit

---

From: Benoit Claise <[email-masked]>
Date: Fri, 25 Apr 2014 15:34:44 +0200

Dear all,

Some feedback on the sections I already reviewed.
Note that I agree with Juergen's feedback.
>     primarily text reorganization.
The following text could be cut/pasted from the Introduction to a new 
section "High Level Solution Overview", somewhere after the terminology 
section:

    This document specifies a method for creating IPFIX Option Templates
    that are used to export the extra data required to describe MIB
    variables (seeSection 4.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-4.1>).

    This allows IPFIX Templates to contain any combination of fields
    defined by traditional IPFIX Information Element(s) and/or MIB Object
    Identifier(s).  The MIB Object Identifiers can reference either non-
    indexed or indexed MIB object(s).  Enterprise-specific MIB Object
    Identifiers are also supported.

    This document defines three classes of new IPFIX Information
    Elements.  These are used to export values from the MIB, export
    required Object Identifier information, and optionally export type
    data from a MIB Module:

    o  mibObjectValue Information Elements (Section 10.1  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-10.1>)

    o  mibFieldOption Information Elements (Section 10.2  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-10.2>)

    o  mibTypeInformation Information Elements (Section 10.3  <http://tools.ietf.org/html/draft-ietf-ipfix-mib-variable-export-05#section-10.3>)

>     at the right place in the document.
Agreed.
>     for confusion.
 From RFC 7011:

     +------------------+---------------------------------------------+
     |                  |                 Contents                    |
     |                  +--------------------+------------------------+
     |       Set        |      Template      |         Record         |
     +------------------+--------------------+------------------------+
     |     Data Set     |          /         |     Data Record(s)     |
     +------------------+--------------------+------------------------+
     |   Template Set   | Template Record(s) |           /            |
     +------------------+--------------------+------------------------+
     | Options Template |  Options Template  |           /            |
     |       Set        |      Record(s)     |                        |
     +------------------+--------------------+------------------------+

                     Figure A: Terminology Summary Table

Section 3 provides:

    This document prefers the more generic term "Data Record" (as opposed
    to "Flow Record") in relation to the export of MIB objects.

So flow record should be changed to data records throughout the doc.
> I also think more SMIv2 terms must be imported, such

Regards, Benoit