Network Service Header (NSH) Encapsulation for In Situ OAM (IOAM) Data
draft-ietf-sfc-ioam-nsh-13

General comment. There are several typos or grammatical errors in the document. Suggest running an error checker against the document. 

Section 1: "In-situ OAM (IOAM)" in the first sentence does not need to be expanded as the authors already did so in the abstract.

Section 1: "Service Function Chaining (SFC) [RFC7665]" should read "Service Function Chaining (SFC) Architecture [RFC7665]".

Section 3: 6th sentence use of "service path" should probably be changed to the correct terminology used by the SFC architecture which is Service Function Path (SFP). Further, the next sentence should be folded into the 6th sentence as a single sentence to make it more readable.

Section 3: "See [I-D.ietf-ippm-ioam-deployment] for a discussion of deployment related aspects of IOAM-Data-fields.". This reference should be changed to [RFC9378].

Section 3: "[I-D.ietf-ippm-ioam-flags]". This reference should be changed to [RFC9322].

Several outdated references need to be updated:

  == Outdated reference: A later version (-03) exists of
     draft-ietf-sfc-oam-packet-01

  == Outdated reference: draft-ietf-ippm-ioam-deployment has been published
     as RFC 9378

  == Outdated reference: draft-ietf-ippm-ioam-direct-export has been
     published as RFC 9326

  == Outdated reference: draft-ietf-ippm-ioam-flags has been published as RFC
     9322

I support Jim's and Roman's discuss points.

Thank you for addressing my DISCUSS and COMMENT feedback.

# John Scudder, RTG AD, comments for draft-ietf-sfc-ioam-nsh-11
CC @jgscudder

Thanks for this document. I have a few brief comments. 

## COMMENTS

### Section 1, implementation

"An implementation of IOAM which leverages NSH to carry the IOAM data is available from the FD.io open source software project [FD.io]."

- Is it appropriate to include this here? If you wanted to flag this for reviewers' and WG's information, maybe following the practice recommended in RFC 7942 would be better? That document makes the case (Section 2) for not leaving it in the RFC:

   Since this information is necessarily time dependent, it is
   inappropriate for inclusion in a published RFC.

- The contradiction with the shepherd writeup is a little concerning: "Although no implementations have been reported..." The shepherd writeup was last updated in July of 2022 but the draft text appears to have been present since the 01 version published March 11, 2019. 

### Section 3, header length

Perhaps in "IOAM HDR Len: 8 bit Length field contains the length of the IOAM header in 4-octet units", mention that the length covers the type and length fields? I think this is already implicit in what you've written, but better explicit than implicit.

### Section 5, "several operators"

I support Roman Danyliw's DISCUSS.

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

Thanks to Mirja Kuhlewind for the TSVART review.

I support Jim's and Roman's DISCUSS positions.

Thanks for a well done shepherd writeup.  I note that the writeup advises notifying IPPM of this work.  Was that done?

Section 2 defines "SFC", but the document doesn't actually use that term anywhere.

Updated position.  Thank you for addressing my discuss concern and comments.

Regards,
Rob

Thanks for working on this specification.

I am supporting Jim's and Roman's discuss.