In Situ Operations, Administration, and Maintenance (IOAM) Loopback and Active Flags
draft-ietf-ippm-ioam-flags-10

# Internet AD comments for {draft-ietf-ippm-ioam-flags-09}
CC @ekline

## Comments

### S4.1.1

* "if the encapsulating node's identity is not available in
   the encapsulation header"

   Can you say a little more about what circumstances would cause the
   encapsulating node's identity to not be available?  This seemed a
   little surprising to me.

### S4.2

* I think "address of the node performing the copy" should be expanded
  to say that RFC 6724 source address selection MUST apply.

## Nits

### S4.4

* "Specificallly" -> "Specifically"

### S5

* "layer needs not" -> "layer need not"

Thanks for addressing my DISCUSS and other comments, LGTM. 

Previous DISCUSS for posterity:

Thanks for this document. I have one issue I'd like to be sure we clear up.

1. In §4.1.1,

   The loopback flag MUST NOT be set if it is not guaranteed that there
   is a return path from each of the IOAM transit and IOAM decapsulating
   nodes, 
   
This is heartwarming but I can’t see how you could guarantee this property at all times in any network using dynamic routing or even subject to dynamic conditions (and that would be all networks), and for that matter I’m not sure how to write code to even determine this in any general way. Is it your intention that this MUST NOT is directed to the operator and not to the code implementor? Or perhaps is it for very small values of “guarantee”? That is, is this an aspirational MUST and not a MUST MUST?

In general it's a little problematic when we use RFC 2119 keywords in a protocol document, to express desires about how a protocol's operator should deploy it. They are at their best when used to express requirements for how a coder should implement the protocol. Please consider creating an operational considerations section, and grouping operational requirements and advice there, at least in that case it becomes clear to whom the RFC 2119 keywords are speaking. 

Alternately, please qualify the keywords appropriately in-line, e.g. in the above text you could say something like

   The domain MUST be configured such that there is expected to be a return
   path from each of the IOAM transit and IOAM decapsulating nodes; if this
   expectation does not apply then configuration MUST NOT enable the loopback
   flag to be set,
   
To me it seems as though it might be less painful to group these into an operational considerations section, but whatever works for you, as long as it's clear.

I did a cursory check over the document with this in mind, the other place I identified what looks like operational guidance to me is also in §4.1.1, the paragraph about how you "SHOULD NOT exceed 1/N of the interface capacity". At first blush that looks like something that could be computed automatically by inspection of the router's hardware, but by the time we get to the end of the paragraph we see that "prior knowledge about the network topology or size" is needed, so it must really be operational guidance. (Possibly this applies to the 1/N paragraphs in §4.2 and §5 also, although it's less clearly the case.)

COMMENTs:

2. The document cites RFCs 7014 and 5475 normatively. They don't seem normative to me, they seem informative.

3. In §4.2,

                                              The L-bit MUST be cleared
   in the copy of the packet that a node sends back towards the source.

This makes me wonder, does the looped back packet inherit the IP TTL/hop limit of the parent packet? The description of it as a “copy” makes me think it does. Should this be explicit? 

NITS:

4. In §5, 

   This draft focuses on three possible use cases of active measurement

Should be "this document focuses".

5. Again in §5,

                                                              A selected
      data packet that is replicated, and its (possibly truncated) copy
      is forwarded with one or more IOAM options, while the original
      packet is forwarded normally, without IOAM options.
      
I think you need to delete the "that" from the first clause?

6. And once again in §5,

   o  IOAM active measurement using replicated data packets: probe
      packets are created by the encapsulating node by selecting some or
      all of the en route data packets and replicating them.  
      
The 1/N requirement calls into question "or all" above, unless N=1, something you strongly discourage. Although you don't technically *forbid* N=1, I think the inclusion of "or all" creates confusion and you could and should leave it out while still not technically forbidding N=1.

7. In §8,

                                                        The attacker can
      potentially leverage the Loopback flag for a Distributed Denial of
      Service (DDoS) attack, as multiple devices send looped-back copies
      of a packet to a single source.

The use of "source" is odd here. By the nature of an attack, the looped-back copies wouldn't be targeted at the actual source of the packets. Possibly "target" or even "victim"?

Thank you to the Working Group for tackling the issue of the author count.  I know those conversations can be quite un-fun.

I concur with John that the references to RFCs 7014 and 5475 should be informative.

Section 6 should indicate explicitly that this document is the reference document for the two new registry entries.  I realize that may be obvious, but you need to say it.

Thanks for addressing the area review items that were brought up.

Thank you to Donald Eastlake for the SECDIR review.

Section 4.*.  The same thing appears to be said twice, but with different normative language:

-- Section 4
   Loopback can be used only if a return path from transit nodes and
   destination nodes towards the source (encapsulating node) exists.”

-- Section 4.1.  
   The loopback flag MUST NOT be set if it is not guaranteed that there
   is a return path from each of the IOAM transit and IOAM decapsulating
   nodes, or if the encapsulating node's identity is not available in
   the encapsulation header.

** Section 4.4.

   In either case, when the packet reaches the IOAM
   boundary its IOAM encapsulation is removed, preventing IOAM
   information from leaking out from the IOAM domain.

Isn’t it more than removing the IOAM encapsulation?  Wouldn’t the packet just be dropped at that point since it is a loop back packet with no place to go?

** Section 5.  How does a deencapsulating node distinguish between the two use cases of “IOAM active measurement using probe packets within the IOAM domain” and “IOAM active measurement using replicated data packets”? Specifically, how does the deencapsulating know it is a getting a probe packet vs. a replicated packet?

Thanks for working on this specification and special thanks to Dr. Bernard Aboba for a very good TSVART early review.

# Éric Vyncke, INT AD, comments for draft-ietf-ippm-ioam-flags-09
CC @evyncke

Thank you for the work put into this document. 

Thank you for addressing my previous blocking DISCUSS (kept below for archiving). I would have appreciated a reply to my non-blocking COMMENT though. Anyway, I am clearing my DISCUSS.

Thanks to Pascal Thubert for his internet directorate review at:
https://datatracker.ietf.org/doc/review-ietf-ippm-ioam-flags-09-intdir-telechat-thubert-2022-06-28/ (please consider Pascal's comments as mine).

Special thanks to Tommy Pauly for the shepherd's detailed write-up including the WG consensus and the justification of the intended status. 

I hope that this helps to improve the document,

Regards,

-éric


## Previous DISCUSS (kept for archiving)

As noted in https://www.ietf.org/blog/handling-iesg-ballot-positions/, a DISCUSS ballot is a request to have a discussion on the following topics:

### Section 4.2 which address

`The address of the node performing the copy operation` is confusing in the case of multiple interfaces (typical for a transit device BTW)... Which address should be used ? If the packet was received through an interface with a global address, then this should be the obvious choice or a loopback interface or ???

### Section 4.2 just truncation ?

```
   The copy is also truncated, i.e., any payload that
   resides after the IOAM option(s) is removed before transmitting the
   looped back packet back towards the encapsulating node.
```
It is unclear what happens to the IPv6 Next header field... Should the IP header length field be modified ?

### Section 4.2 forwarding ?

It is unclear whether the packet is sent back to the source via the received interface or whether the packet is forwarded based on the FIB.

### IANA considerations conflicting text ?
In section 4.1:
```
   An IOAM trace option that has the Loopback flag set MUST have the
   value '1' in the most significant bit of IOAM-Trace-Type, and '0' in
   the rest of the bits of IOAM-Trace-Type. 
```
but in section 6:
```
   IANA is requested to allocate the following bits in the "IOAM Trace
   Flags Registry" as follows:

   Bit 1  "Loopback" (L-bit)

   Bit 2  "Active" (A-bit)

   Note that bit 0 is the most significant bit in the Flags Registry.
```

Is it bit 0 or bit 1 ?

## COMMENTS

### "loopback"

No need to reply, but every time I read "loopback", I think of the local "loopback interface". The use of "echo" would probably have made my reading easier ;-)

### Section 4.1

```
   An IOAM trace option that has the Loopback flag set MUST have the
   value '1' in the most significant bit of IOAM-Trace-Type, and '0' in
   the rest of the bits of IOAM-Trace-Type. 
```
Does it prevent further enhancements to Trace types ?

### Section 4.1.1

"SHOULD NOT exceed 1/N of the interface capacity" but this recommendation is only for the encapsulating node while there are nodes / links on the path that may have much more constrained capacity. I suggest to remove this part and replace it by text not refering to encapsulating node interface.

### Section 5

```
   The
   IOAM options are encapsulated in one of the IOAM encapsulation types,
   e.g., [I-D.ietf-sfc-ioam-nsh], or [I-D.ietf-ippm-ioam-ipv6-options].
```
Should this text also appear in the section 4? 

### Section 5 capacity

In
```
   Thus, the rate of the traffic that
   includes the Active flag rate SHOULD NOT exceed 1/N of the interface
   capacity on any of the IOAM node's interfaces. 
```
Should thie be "IOAM nodes' interfaces" to take into account all IOAM nodes (including transit).

## NITS  


### Section 5
s/This draft focuses on three possible use cases/This document focuses on three possible use cases/

## 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. 

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

Went through section 4.1.1/4.2/5 and after thinking about this, I believe the normative language in these sections address the concerns I raised in my previous discuss relating to section 8, hence clearing the discuss point.

I support John's Discuss.

One Minor comment that matches the comment that I put on the DEX document.

2.
   N >> M

I'm assuming that by ">>", this means much greater than?  It would be better use words here, or at least define what this means (e.g., as opposed to a bit-shift).

Thanks,
Rob