Re: [IPFIX] draft-kashima-ipfix-data-link-layer-monitoring

[Paul Aitken <[e-mail]>]

Kashima-san,

I reviewed the draft. Please find some feedback inline.


> 1. Introduction
>
>    While these renovations provide flexibility, scalability, and

I think you mean "innovations".


> 3. Future Traffic Measurement
>
>    The device (Exporter) filters and/or samples Ethernet frames using
>    PSAMP Selector, extracts the header of the frame, and exports the
>    header information encoded by IPFIX protocol to the Collector.

Actually the exporter doesn't do this; the metering process does. A 
combination of MP and exporter is required.


>    Furthermore, the device (Exporter) filters and samples traffic for
>    each VPN using a Composite Selector of PSAMP [RFC5475].  This makes
>    it possible to change the granularity of the traffic monitoring for
>    each VPN.

Same comment again. Also, consider adding some details of the PSAMP process.


> 4.2. dataLinkFrameSection
>
>
>    Description:
>
>       This Information Element carries n octets from the data link frame
>       of a selected frame, starting dataLinkFrameOffset octets into the
>       frame.
>
>       When the dataLinkFrameSectionObservedOctets field corresponding to
>       this Information Element does not exist, this Information Element
>       MUST have a variable length and MUST NOT be padded.  In this case,
>       the size of the exported section may be constrained due to
>       limitations in the IPFIX protocol.

I would relax the "MUST have a variable length" to a SHOULD, since fixed 
length sections may be exported.


>       When the dataLinkFrameSectionObservedOctets field corresponding to
>       this Information Element exists, this Information Element MAY have
>       a fixed length and MAY be padded, or MAY have a variable length.
>
>       The dataLinkFrameSectionObservedOctets expresses how much data was
>       observed, while the remainder is padding.

These two paragraphs follow the one above. I would write them in exactly 
the opposite order: 3, 2, 1.


>       Further Information Elements, i.e., dataLinkFrameType and
>       dataLinkFrameSize are needed to specify the data link type and the
>       size of the data link frame of this Information Element.  A set of
>       these Information Elements MAY be contained in a structured data
>       type, as expressed in another IPFIX WG draft.

You should specify the draft and include a reference.


> 4.4. dataLinkFrameOffset
>
>
>    Description:
>
>       This Information Element specifies the offset of the observed
>       dataLinkFrameSection within the data link frame.  If this
>       Information Element is omitted, it defaults to zero.

I thought the intention was to create a generic "offset" field which 
could be used with any packet section?


> 4.5. dataLinkFrameSectionObservedOctets
>
>
>    Description:
>
>       This Information Element specifies the observed length of the
>       dataLinkFrameSection.
>
>       This Information Element is especially needed for NetFlow version
>       9 [RFC3954] because NetFlow does not support a variable-length
>       Information Element.


I think it would be better to say "This Information Element specifies 
the observed length of the dataLinkFrameSection when padding is used", 
and remove the second paragraph.


> 6. IANA Considerations
>
>
>    This document requests that the Information Element IDs are allocated
>    as shown in section 4.
>
>    In addition, the dataLinkFrameType Information Element requires the
>    creation of new IANA registries.

I think only one new registry is needed for this. However, Nevil is 
leading some discussion whether the values should be defined in the IE 
description, rather than in separate registries.


> Appendix C. Template Formats Example
>
>
>    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 (0x0002)        |             Length            |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |      Template ID (0x0103)     |      Field Count (0x0006)     |

The field count is 8.


>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |   ingressInterface (0x000A)   |     Field Length (0x0004)     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |   egressInterface (0x000E)    |     Field Length (0x0004)     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |observationTimeSeconds (0x0142)|     Field Length (0x0008)     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |   dataLinkFrameSize (0x0138)  |     Field Length (0x0002)     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    | dataLinkFrameSection (0x013B) |     Field Length (0xFFFF)     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |   dataLinkFrameType (0x015B)  |     Field Length (0x0002)     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |  dataLinkFrameOffset (0x015C) |     Field Length (0x0002)     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    | dataLinkFrameSection (0x015D) |     Field Length (0x0002)     |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Call it "dataLinkFrameSectionOO" (OO = Observed Octets). Else 0x015D 
looks like the earlier 0x013B.

>
>                    Figure C-1: Template Fortmat Example

Typo, "Format".


Cheers,
P.