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

[Colin McDowall <[e-mail]>]

Hi Benoit, all,

This is great feedback, thanks.

Please see replies at CM:

Paul should have draft -06 submitted today.

On 25/04/2014 14:32, Benoit Claise wrote:
> 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.
CM:
updated to refer to IANA. The only remaining reference t rfc7012  is in Paul's
new semantics section that is refering to a section of that draft rather than IEs.


>
> - 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.
CM:
Added the 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.
CM:
All uses of draft in the main body changed, now only mentioned in the Acknowledgements.


>
> - 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
CM: Went the other way and removed the 'a' to make the sentence read:
     
     The simple and application-wide data types specified in SMIv2
     <xref target="RFC2578"/>, along with new Textual Conventions, can be
     exported within IPFIX and then decoded in the Collector.

>
> - 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
CM:
wow didn't realise field wasn't an ipfix term.

I've updated all the references to mibObjectValue fields to refer 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?
CM:
I think it was originally Exporting Process - but the process capturing values from
an on board mib may not be the actual Exporting Process

> Also MAY to may. This is not a RFC 2119 keyword required for the implementation.
CM:
ok moved to lower case.
>
> - 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
CM: Agreed,  fixed.

>
> - 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
CM:
lowercased but kept it in the plural:
     so metadata about fields can be included in Data Templates.

>
> - 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
CM:
This was an attempt to not limit future implementations of IPFIX from
having to implement the export of the OID data in the same format. I've moved
that to lower case though, since we can only define behaviour for the current
version in this draft.

I've considered all the 2119 keywords and I think those left are now required.

>
> - 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
CM:
Ok, the RFC 2119 keywords relating to the option export have been grouped into that section.


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

>
> - 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>)
CM:
I think I just wanted to give a handle/name to the particular option export, to make it clearer
to refer to throughout the document. If it was more confusing that is bad. I've updated the
names of the Options exports to match your suggestions.

>
>
> - 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
CM:
I've removed the duplicate list of required fields and just added a reference to the required minimum section.

There are now no spec rules or keywords in the initial part of this section.


>
> -
>
>      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.
CM:
I've checked and fixed the titles 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.
CM:
Agreed - the point is we want to treat the Data Records with the OIDS like templates.
Removed the single

Updated to
     If Multiple MIB Field Options Data Records that refer to a mibObjectValue are received the latest MUST be used.

> This contradicts:
>     Note that the ID of an identical mibFieldOptionTemplate which has
>     already been exported MAY be reused without exporting the Template
>     again.
CM: Resolved

>
> - 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
CM: Updated to
The MIB Field Options Data Records are defined in a template referred to in this document as a MIB Field Options Template with the format specified in <xref target="sectionFormats"/>.

>
> - 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.
CM:
Not quite.
There are 2 different types of Data records in play here.
The MIB Field Option Data Records contain the OIDs. The Object value data
records contain values.

This is saying that you need the oid data record before
you can understand the values.

I've reworded it as follows which is clearer:
   The MIB Field Options Template and MIB Field Options Data Records MUST be exported in the same IPFIX
   Message as any Template that is using a mibObjectValue Information Element.
   Note that this places an implicit size constraint on the export.
   This whole set of Templates and MIB Field Options Data Records MUST all be exported prior to the corresponding Data  Records  which depend upon them.
i.e. referring to <xref target="figure_overview_field"/>, the export order MUST be:
       <list style="numbers">
     <t>Data Template for mibObjectValue Information Elements</t>
     <t>MIB Field Options Template</t>
     <t>MIB Field Options Data Records</t>
     <t>MIB Object Value Data Records</t>
       </list>
>
> - Major
>     While the following are optional, they are nevertheless RECOMMENDED
>     in certain circumstances as they are per field:
>
> You don't explain those circumstances
CM:
Added a reference for each of these optional IEs to the section that explains when
to use them.
>
> - 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.
CM:

Agreed.

The mibSubIdentifier usage is now part of the standard MIB Field Options Template. This
is clearer.

I've added a note on when to use the mibSubIdentifier and a reference to the correct section.

The MIB Type Options Export is entierly optional. Since it is just RECOMMENDED I've taken all the fields
together into 1 list.

>
> - 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
CM:

Reworded to match standard template behaviour on UDP - use the latest version:
      
      If Multiple MIB Field Options Data Records that refer to a mibObjectValue are received the latest MUST be used.
     This matches the expected behaviour of IPFIX Templates.
  
>
> - 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.
CM:
Done.

>
>
> I stopped my review at section 4.4.2, but plenty of my feedback would apparently be equivalent for the rest of the draft.
I've tried to apply each of your points throughout the document.

Thanks,
colin