Export of GTP-U Information in IP Flow Information Export (IPFIX)
draft-ietf-opsawg-ipfix-gtpu-18

# Éric Vyncke INT AD comments for draft-ietf-opsawg-ipfix-gtpu-18
CC @evyncke

Thank you for the work put into this document.

Please find below some blocking DISCUSS points (easy to address), some non-blocking COMMENT points/nits, replies would be appreciated even if only for my own education).

Special thanks to Benoît Claise for the shepherd's concise write-up including the WG consensus and the justification of the intended status.

I hope that this review helps to improve the document,

Regards,

-éric

Note: this ballot comments follow the Markdown syntax of https://github.com/mnot/ietf-comments/tree/main, i.e., they can be processed by a tool to create github issues.


## DISCUSS (blocking)

As noted in https://datatracker.ietf.org/doc/statement-iesg-handling-ballot-positions-20220121/, a DISCUSS ballot is a request to have a discussion on the points below; I really think that the document would be improved with a change here, but can be convinced otherwise.

### Section 3 SHOULD

Why not a "MUST" in `When 3GPP specifications state that a field shall not be interpreted, the Exporting Process SHOULD use a Template that omits the corresponding IE` ? Like Med Boucadair, I do not think that BCP14 should be used in the sentence.

Also note https://datatracker.ietf.org/doc/statement-iesg-statement-on-clarifying-the-use-of-bcp-14-key-words/

### Section 5

I find very unusual to have the normative description of several flags *only* inside the IANA section and not in section 3. This only makes the implementers' job more complex. Consider moving the normative text into section 3.

### Section 5.1

In addition to the global DISCUSS on section 5, what is the meaning of `Version(3b)` in the flag description ? I have a guess but a PS-level draft should not leave any room for ambiguities.

The draft is also completely silent on the value & semantic on this 'Version' number.

Finally, what is the expected receiver behaviour when bit 4 (labelled as "0") is set ?

Thanks to the authors and the WG for their work on this document.

Note: this review is based on my very quick skim through the referenced 3GPP spec
and I might have got this wrong. If so, please correct me.

Comments are inline in the idnits output of v18. Look for the <EoRv18> tag at 
the end to ensure you are seeing the full review.

I have a few points that I would like to discuss.

<discuss-1> Sections 5.5 and 5.6

423	   Description:  6-bit QoS flow identifier field defined in PDU Session
424	      Container extension header of GTP-U.  This may be used to
425	      determine the QoS flow and QoS profile which are associated with
426	      the received packet.  The presence of the PDU Session Container
427	      extension header is indicated by the E (Extension Header) bit in
428	      the gtpuFlags IE.  When the E bit is set to 0, this IE is not
429	      meaningful.  The two most significant bits are reserved, MUST be
430	      exported as zero, and MUST be ignored on receipt.

470	   Description:  4-bit PDU type field defined in PDU Session Container
471	      extension header of GTP-U.  This field indicates the structure of
472	      the PDU session user plane frame.  The presence of the PDU Session
473	      Container extension header is indicated by the E (Extension
474	      Header) bit in the gtpuFlags IE.  When the E bit is set to 0, this
475	      IE is not meaningful.  The four most significant bits are
476	      reserved, MUST be exported as zero, and MUST be ignored on
477	      receipt.

The E flag indicates that the Next Extension Header field is valid; it
does not indicated which extension header is present. GTP-U supports multiple
extension-header types and a chain in which each header identifies the next
header. Therefore, E=1 does not mean that the chain contains a PDU
Session Container, and that container need not be the first extension header
in the chain.

As written, an exporter can treat unrelated extension-header bytes as QFI or
PDU Type. Please revise both IE descriptions to require the Exporting Process
to parse the extension-header chain and locate a PDU Session Container before
exporting either IE. When no PDU Session Container is present, or when the
chain is malformed, truncated, or cannot be safely parsed, the IE should be
omitted. The document should also specify the behavior if multiple PDU
Session Containers are encountered; though I don't know if that is even possible.

<discuss-2> Section 5.7

504	5.7.  gtpuTotalHdrLength
506	   Name:  gtpuTotalHdrLength
508	   ElementID:  TBD1
510	   Description:  8-bit field indicating the total length in octets of
511	      the observed GTP-U header, including the mandatory 8-octet header
512	      and any optional fields and extension headers defined in Sections
513	      5.1 and 5.2 of the 3GPP specification [TS.29281].  This IE is
514	      computed from the observed packet header and does not export the
515	      GTP-U Length field defined in Section 5.1 of [TS.29281].
517	   Abstract Data Type:  unsigned8
519	   Data Type Semantics:  quantity
521	   Additional Information:  Refer to Sections 5.1 and 5.2 of [TS.29281].
523	   Reference:  [RFC-to-be]

An IPFIX unsigned8 can represent values only from 0 through 255. GTP-U has a
variable-length header, its extension headers can be chained, and each
Extension Header Length is encoded in four-octet units. A valid GTP-U header
can therefore exceed 255 octets. Please use a data type that is large enough
to cover the headers.

If the intention were to limit to 255, then the specification does not state
what an exporter does when the observed header is longer than 255 octets. 
Implementations could truncate, stop at the value at 255, wrap it modulo 256, 
omit the IE, or reject the record. These outcomes are not interoperable, and 
several report an incorrect length.

<discuss-3> Section 5.8

525	5.8.  gtpuHeaderSection
527	   Name:  gtpuHeaderSection
529	   ElementID:  TBD2
531	   Description:  This Information Element carries a series of n octets
532	      from the observed GTP-U header, including the mandatory header and
533	      any optional fields and extension headers defined in Sections 5.1
534	      and 5.2 of the 3GPP specification [TS.29281], as a series of
535	      octets in IPFIX.
537	   Abstract Data Type:  octetArray
539	   Data Type Semantics:  default
541	   Additional Information:  Refer to Section 5 of [TS.29281].
543	   Reference:  [RFC-to-be]

The phrase "a series of n octets from the observed GTP-U header" does not
indicate whether the value begins at the first GTP-U header octet, whether it
contains the entire header or only a contiguous portion, whether an arbitrary
subsection is permitted, how truncation is detected, whether padding is
allowed, what values of n are valid, or what happens when the captured header
is itself truncated or malformed.

RFC 7012 requires applicable octetArray length constraints to be specified.

Please define one interoperable semantic model. A simple approach is
to require the value to start at the first GTP-U header octet, contain a
contiguous set of bytes, and contain the complete header whenever it is available
and fits. Truncation should either be prohibited or explicitly permitted with
a reliable way for the collector to recognize it. One possible option is to
require gtpuTotalHdrLength whenever gtpuHeaderSection is shorter than the
complete header. The definition should also cover malformed/truncated input
and prohibit bytes beyond the observed GTP-U header.