Re: [OPSAWG] AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh

2024-04-15 Thread mohamed . boucadair 
Hi Mahesh,

Please see inline.

Cheers,
Med



Orange Restricted
De : Mahesh Jethanandani 
Envoyé : lundi 15 avril 2024 22:47
À : BOUCADAIR Mohamed INNOV/NET 
Cc : draft-ietf-opsawg-ipfix-tcpo-v...@ietf.org; opsawg@ietf.org; 
pait...@ciena.com
Objet : Re: AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh


Hi Med,

Thanks for addressing most of the comments. Two follow-up comments. See below 
with [mj]

On Apr 15, 2024, at 4:51 AM, 
mohamed.boucad...@orange.com wrote:

Hi Mahesh,

Thank you for the review.

The changes can be tracked at: Diff: draft-ietf-opsawg-ipfix-tcpo-v6eh.txt - 
draft-ietf-opsawg-ipfix-tcpo-v6eh.txt.

Please see more context inline.

Cheers,
Med

De : Mahesh Jethanandani 
mailto:mjethanand...@gmail.com>>
Envoyé : dimanche 14 avril 2024 21:42
À : 
draft-ietf-opsawg-ipfix-tcpo-v...@ietf.org
Cc : opsawg@ietf.org; 
pait...@ciena.com
Objet : AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh

Hi Authors,

Thanks for working on this document.

Here is my AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh-10 version of the 
draft. They are divided between COMMENT and NIT. I expect that the COMMENTs 
will be resolved before the document is sent to IETF Last Call.

---
COMMENT
---

Section 1, paragraph 1

Should the draft-ietf-opsawg-ipfix-fixes be referenced also?

[Med] We used to refer to that I-D till the WG decided to deprecate the IEs. No 
need to have that ref back.

How about reference to RFC 7012 which is only mentioned in the Security 
Considerations section?
[Med] That's OK as the authoritative reference for the IEs/data types is the 
IANA registry itself.

Section 1.1, paragraph 12
>Section 3 addresses these issues.  Also, ipv6ExtensionHeaders IPFIX
>IE is deprecated in favor of the new IEs defined in this document.

On the question of how a legacy client receiver receiving this new 
ipv6ExtensionHeader definition would react, I was wondering if a forward 
reference to the Operational Considerations be made. Otherwise, till one reads 
that section, it is not clear what a legacy client should do.

[Med] Not sure what you meant by "legacy client receiver", but I suspect you 
mean the collector. If that is what you had in mind, then usual rfc7011 applies 
for manipulating templates, etc.

Section 1.2, paragraph 3
>*  Describe how any observed TCP option in a Flow can be exported
>   using IPFIX.  Only TCP options having a kind <= 63 can be exported
>   in a tcpOptions IE.


Is the problem that no TCP options can be exported using IPFIX, or is that TCP 
options having a Kind value >= 64 cannot be exported? Note, the first sentence 
starts by saying "how any observed TCP option in a Flow can(not) be exported", 
the not added from the sentence above.

[Med] That's an issue because we don't have full visibility on the options 
present in flow. For example, TCP-ENO or shared option can't be reported with 
(deprecated) tcpOptions.

[mj] În that case, do you want to:

s/Describe how any observed TCP options/Describe how some observed TCP options/

[Med] Works for me. Fixed. Thanks.



Section 1.2, paragraph 4
>Section 4 addresses these issues.  Also, tcpOptions IE is deprecated
>in favor of the new IEs defined in this document.

Same comment as above on providing a forward reference.

Section 3.3, paragraph 7
>Abstract Data Type:  unsigned256

I wonder why the opinion of a Expert Reviewer was overridden on the choice of 
defining this as unsigned256 instead of a bitfield.  I understand that there is 
precedence of using unsigned values for "flags", but as Paul noted, the value 
of a unsigned number is meaningless in the case where the individual bits hold 
values, not the whole unsigned number. Was it because of reduced-encoding?
[Med] The current design is consistent with how flags are handled in IPFIX. As 
you also rightfully mentioned, support of reduced-encoding is a key feature to 
support here given the long type. Please note that RFC7011 is explicit about 
target types:


   Reduced-size encoding MAY be applied to the following integer types:

   unsigned64, signed64, unsigned32, signed32, unsigned16, and signed16.

   The signed versus unsigned property of the reported value MUST be

   preserved.  The reduction in size can be to any number of octets

   smaller than the original type if the data value still fits, i.e., so

   that only leading zeroes are dropped.  For example, an unsigned64 can

   be reduced in size to 7, 6, 5, 4, 3,

Re: [OPSAWG] AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh

2024-04-15 Thread Mahesh Jethanandani 
Hi Med,

Thanks for addressing most of the comments. Two follow-up comments. See below 
with [mj]

> On Apr 15, 2024, at 4:51 AM, mohamed.boucad...@orange.com wrote:
> 
> Hi Mahesh,
>  
> Thank you for the review.
>  
> The changes can be tracked at: Diff: draft-ietf-opsawg-ipfix-tcpo-v6eh.txt - 
> draft-ietf-opsawg-ipfix-tcpo-v6eh.txt 
> .
>  
> Please see more context inline.
>  
> Cheers,
> Med
>  
> De : Mahesh Jethanandani  > 
> Envoyé : dimanche 14 avril 2024 21:42
> À : draft-ietf-opsawg-ipfix-tcpo-v...@ietf.org 
> 
> Cc : opsawg@ietf.org ; pait...@ciena.com 
> 
> Objet : AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh
>  
> Hi Authors,
>  
> Thanks for working on this document.
>  
> Here is my AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh-10 version of the 
> draft. They are divided between COMMENT and NIT. I expect that the COMMENTs 
> will be resolved before the document is sent to IETF Last Call.
>  
> ---
> COMMENT
> ---
>  
> Section 1, paragraph 1
>  
> Should the draft-ietf-opsawg-ipfix-fixes be referenced also?
>  
> [Med] We used to refer to that I-D till the WG decided to deprecate the IEs. 
> No need to have that ref back.
>  
> How about reference to RFC 7012 which is only mentioned in the Security 
> Considerations section?
> [Med] That’s OK as the authoritative reference for the IEs/data types is the 
> IANA registry itself.
>  
> Section 1.1, paragraph 12
> >Section 3 addresses these issues.  Also, ipv6ExtensionHeaders IPFIX
> >IE is deprecated in favor of the new IEs defined in this document.
>  
> On the question of how a legacy client receiver receiving this new 
> ipv6ExtensionHeader definition would react, I was wondering if a forward 
> reference to the Operational Considerations be made. Otherwise, till one 
> reads that section, it is not clear what a legacy client should do. 
>  
> [Med] Not sure what you meant by “legacy client receiver”, but I suspect you 
> mean the collector. If that is what you had in mind, then usual rfc7011 
> applies for manipulating templates, etc.
>  
> Section 1.2, paragraph 3
> >*  Describe how any observed TCP option in a Flow can be exported
> >   using IPFIX.  Only TCP options having a kind <= 63 can be exported
> >   in a tcpOptions IE.
>  
>  
> Is the problem that no TCP options can be exported using IPFIX, or is that 
> TCP options having a Kind value >= 64 cannot be exported? Note, the first 
> sentence starts by saying “how any observed TCP option in a Flow can(not) be 
> exported”, the not added from the sentence above.
>  
> [Med] That’s an issue because we don’t have full visibility on the options 
> present in flow. For example, TCP-ENO or shared option can’t be reported with 
> (deprecated) tcpOptions.

[mj] În that case, do you want to:

s/Describe how any observed TCP options/Describe how some observed TCP options/

>  
>  
> Section 1.2, paragraph 4
> >Section 4 addresses these issues.  Also, tcpOptions IE is deprecated
> >in favor of the new IEs defined in this document.
>  
> Same comment as above on providing a forward reference.
>  
> Section 3.3, paragraph 7
> >Abstract Data Type:  unsigned256
>  
> I wonder why the opinion of a Expert Reviewer was overridden on the choice of 
> defining this as unsigned256 instead of a bitfield.  I understand that there 
> is precedence of using unsigned values for "flags", but as Paul noted, the 
> value of a unsigned number is meaningless in the case where the individual 
> bits hold values, not the whole unsigned number. Was it because of 
> reduced-encoding?
> [Med] The current design is consistent with how flags are handled in IPFIX. 
> As you also rightfully mentioned, support of reduced-encoding is a key 
> feature to support here given the long type. Please note that RFC7011 is 
> explicit about target types:
>  
>Reduced-size encoding MAY be applied to the following integer types:
>unsigned64, signed64, unsigned32, signed32, unsigned16, and signed16.
>The signed versus unsigned property of the reported value MUST be
>preserved.  The reduction in size can be to any number of octets
>smaller than the original type if the data value still fits, i.e., so
>that only leading zeroes are dropped.  For example, an unsigned64 can
>be reduced in size to 7, 6, 5, 4, 3, 2, or 1 octet(s).
>  
> As a side note, we discussed with Benoît whether we need we tag this I-D as 
> formally updating RFC7011 but we finally

Re: [OPSAWG] AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh

2024-04-15 Thread mohamed . boucadair 
Hi Mahesh,

Thank you for the review.

The changes can be tracked at: Diff: draft-ietf-opsawg-ipfix-tcpo-v6eh.txt - 
draft-ietf-opsawg-ipfix-tcpo-v6eh.txt.

Please see more context inline.

Cheers,
Med

De : Mahesh Jethanandani 
Envoyé : dimanche 14 avril 2024 21:42
À : draft-ietf-opsawg-ipfix-tcpo-v...@ietf.org
Cc : opsawg@ietf.org; pait...@ciena.com
Objet : AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh

Hi Authors,

Thanks for working on this document.

Here is my AD review of draft-ietf-opsawg-ipfix-tcpo-v6eh-10 version of the 
draft. They are divided between COMMENT and NIT. I expect that the COMMENTs 
will be resolved before the document is sent to IETF Last Call.

---
COMMENT
---

Section 1, paragraph 1

Should the draft-ietf-opsawg-ipfix-fixes be referenced also?

[Med] We used to refer to that I-D till the WG decided to deprecate the IEs. No 
need to have that ref back.

How about reference to RFC 7012 which is only mentioned in the Security 
Considerations section?
[Med] That's OK as the authoritative reference for the IEs/data types is the 
IANA registry itself.

Section 1.1, paragraph 12
>Section 3 addresses these issues.  Also, ipv6ExtensionHeaders IPFIX
>IE is deprecated in favor of the new IEs defined in this document.

On the question of how a legacy client receiver receiving this new 
ipv6ExtensionHeader definition would react, I was wondering if a forward 
reference to the Operational Considerations be made. Otherwise, till one reads 
that section, it is not clear what a legacy client should do.

[Med] Not sure what you meant by "legacy client receiver", but I suspect you 
mean the collector. If that is what you had in mind, then usual rfc7011 applies 
for manipulating templates, etc.

Section 1.2, paragraph 3
>*  Describe how any observed TCP option in a Flow can be exported
>   using IPFIX.  Only TCP options having a kind <= 63 can be exported
>   in a tcpOptions IE.


Is the problem that no TCP options can be exported using IPFIX, or is that TCP 
options having a Kind value >= 64 cannot be exported? Note, the first sentence 
starts by saying "how any observed TCP option in a Flow can(not) be exported", 
the not added from the sentence above.

[Med] That's an issue because we don't have full visibility on the options 
present in flow. For example, TCP-ENO or shared option can't be reported with 
(deprecated) tcpOptions.


Section 1.2, paragraph 4
>Section 4 addresses these issues.  Also, tcpOptions IE is deprecated
>in favor of the new IEs defined in this document.

Same comment as above on providing a forward reference.

Section 3.3, paragraph 7
>Abstract Data Type:  unsigned256

I wonder why the opinion of a Expert Reviewer was overridden on the choice of 
defining this as unsigned256 instead of a bitfield.  I understand that there is 
precedence of using unsigned values for "flags", but as Paul noted, the value 
of a unsigned number is meaningless in the case where the individual bits hold 
values, not the whole unsigned number. Was it because of reduced-encoding?
[Med] The current design is consistent with how flags are handled in IPFIX. As 
you also rightfully mentioned, support of reduced-encoding is a key feature to 
support here given the long type. Please note that RFC7011 is explicit about 
target types:


   Reduced-size encoding MAY be applied to the following integer types:

   unsigned64, signed64, unsigned32, signed32, unsigned16, and signed16.

   The signed versus unsigned property of the reported value MUST be

   preserved.  The reduction in size can be to any number of octets

   smaller than the original type if the data value still fits, i.e., so

   that only leading zeroes are dropped.  For example, an unsigned64 can

   be reduced in size to 7, 6, 5, 4, 3, 2, or 1 octet(s).

As a side note, we discussed with Benoît whether we need we tag this I-D as 
formally updating RFC7011 but we finally preferred to no do so because the 
authoritative registry for the data type is the registry.

If so, that should be stated as the reason. BTW, can a bitfield not have 
similar semantics of reduced-encoding, if all the (MSB) bits are not being used?

[Med] There is no "bitfield" data type. The only mention of "bit field" in 7011 
is the following


   "flags" is an integral value that represents a set of bit fields.
   Logical operations are appropriate on such values, but other
   mathematical operations are not.  Flags MUST always be of an unsigned
   data type.

"bit field" is used for almost all IEs with flags (IP Flow Information Export 
(IPFIX) En