[ippm] draft-ietf-ippm-stamp-cos-ecn-01 early Tsvart review

[Lars Eggert via Datatracker <[e-mail]>]

Document: draft-ietf-ippm-stamp-cos-ecn
Title: Update of the Simple Two-way Active Measurement Protocol
Class-of-Service Extension - ECN Reviewer: Lars Eggert Review result: On the
Right Track

# tsvart review of draft-ietf-ippm-stamp-cos-ecn-01

CC @larseggert

This document has been reviewed as part of the transport
area review team's ongoing effort to review key IETF
documents. These comments were written primarily for the
transport area directors, but are copied to the
document's authors and WG to allow them to address any
issues raised and also to the IETF discussion list for
information.

When done at the time of IETF Last Call, the authors
should consider this review as part of the last-call
comments they receive. Please always CC [e-mail]
if you reply to or forward this review.

## Comments

### Missing RFC status

Datatracker does not record an intended RFC status for this document.

### Section 1, paragraph 3
```
     Another STAMP extension header [I-D.ietf-ippm-stamp-ext-hdr] defines
     a mechanism that can be used by a Session-Sender to request that the
     Session-Reflector copy the entire IP header from the received test
     packet into the reflected test packet.  That capability could be used
     in place of the CoS TLV to observe the DSCP and ECN values in the
     forward direction in situations where there is no need to control the
     return path DSCP or ECN value.
```
Either mechanism depends on the middlebox not **also** rewriting the CoS TLV or
the IP header copy when it rewrites the IP header. (Because IIRC there is no
hash-based tamper detection built into STAMP.) Given that middleboxes do all
kinds of "creative" things, I wouldn't bet on these extensions enabling
detectability, esp. not in the long run.

### Section 3.1, paragraph 14
```
     *  DSCP1: DSCP value intended by the Session-Sender to be used as the
        DSCP value of the reflected test packet.

     *  DSCP2: received value in the DSCP field at the ingress of the
        Session-Reflector.

     *  EC2: received value in the ECN field at the ingress of the
        Session-Reflector.

     *  RPD (reverse path DSCP): two-bit field indicating whether the
        Session-Reflector used DSCP1 or DSCP2 as the DSCP value of the
        reflected test packet; a Session-Sender MUST set the value of the
        RPD field to 0b00 on transmission.

     *  EC1: ECN value intended by the Session-Sender to be used as the
        ECN value of the reflected test packet.

     *  RPE (reverse path ECN): two-bit field indicating whether the
        Session-Reflector used EC1 as the ECN value of the reflected test
        packet; a Session-Sender MUST set the value of the RPE field to
        0b00 on transmission.
```
The order of these is a bit surprising; I had expected DSCP1/EC1 to share a
byte (ditto for 2). Is this order here somehow more processing-efficient?

### Section 3.1, paragraph 13
```
     *  Reserved: twelve-bit field; MUST be zeroed on transmission and
        ignored on receipt.
```
How are you planning to enable the eventual use of these, given there is no
version fields in the TLV? (Burn a new type?) Also, if you don't grease them,
you're gonna loose them.

### Section 3.2, paragraph 2
```
     the received test packet.  Otherwise, the Session-Reflector MUST use
     the DSCP value of the received STAMP packet and set the value of the
     RPD field to 0b01.  Upon receiving the reflected packet, if the value
```
What if *that* value is not permitted by local configuration for outbound
traffic?

### Section 3.4, paragraph 3
```
     the EC1 field, it MUST observe the ECN value in the IP header of the
     reflected packets and reduce its sending rate upon observation of a
     CE value.  If a Session-Sender sends a STAMP packet containing both
```
"Reduce" here is pretty vague. I realize it's difficult to be more mnormatively
precise here, but maybe it would be possible to say "for example, halve" or
point to what CUBIC and/or circuit breakers suggest?

### DOWNREFs

DOWNREF `[RFC9330]` from this Standards Track to Informational `RFC9330`.

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool) so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Uncited references

Uncited references: `[RFC2474]`, `[I-D.ietf-tsvwg-udp-ecn]`, `[RFC9331]`, and
`[RFC8311]`.

### Outdated references

Document references `draft-ietf-ippm-asymmetrical-pkts-08`, but `-14` is the
latest available revision.

Document references `draft-ietf-ippm-stamp-ext-hdr-07`, but `-14` is the latest
available revision.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF]. You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool