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

On Tue, Apr 15, 2014 at 12:01:11PM +1200, Nevil Brownlee wrote:
> 
> 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!
> 

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/>

_______________________________________________
IPFIX mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/ipfix