Network Service Header (NSH) Metadata Type 2 Variable-Length Context Headers
draft-ietf-sfc-nsh-tlv-15

Thank you for the work on this document, and for addressing my previous DISCUSS.

Francesca

Thanks for the updates!

---
Previous DISCUSS:

1. I notice that in his RTGDIR review of version 08 [*], Stig Venaas suggested some improvements to the security considerations section. This was subsequently discussed and Yuehua Wei proposed some new text [**] for version 09. That text isn’t present, and I don’t see any further resolution on the mailing list either. I’d appreciate it if the topic were closed by either adding the proposed text, or some other text to resolve Stig’s concern, or explanation of why no change was made.

[*] https://datatracker.ietf.org/doc/review-ietf-sfc-nsh-tlv-08-rtgdir-lc-venaas-2021-09-29/
[**] https://mailarchive.ietf.org/arch/msg/sfc/Q2Snf_ZLTkJ1augbaWpmNYlwFBU/

2. In §8.2, the two first references, [GROUPBASEDPOLICY] and [GROUPPOLICY] are deficient. At a minimum, a reference should provide enough information to allow a reader to straightforwardly determine how to retrieve it. This is true even if it’s not an openly-available online source. These two references have less than the bare bones, I don’t know how to find them or refer to them.
---
Previous comments:

1. I support all of Ben’s discuss points. I also want to reiterate his comment about the desirability of having useful captions on the figures. 

2. In §4.2, you write, 

                          This context header carries both the format
   and value of the Tenant identifier.

However, I don’t see anywhere that the header “carries… the format”. Indeed, you write that the Tenant ID is an opaque value. As far as I can tell, there’s no way to infer anything about its structure without a priori knowledge.

If that is correct, you can simplify the sentence to “This context header carries the Tenant Identifier.” If it’s not correct, please explain?

3. Nit, in §4.7 the words “quite efficiently” don’t seem to serve any useful purpose; the document would be better off without them IMO.

Thank you for a well done shepherd writeup.

Thanks also for tidying up the IANA Considerations.

I support Ben's DISCUSS position, particularly the first point.

A suggestion on organization: I think what you have as the top of Section 7 should be a subsection (e.g., 7.1), and then the other subsections moved down.  It doesn't make sense to me to have these registrations be dominant over the others; they're all just a set of IANA actions.

I have the same comment as Francesca about Section 4.1.

I strongly support Ben Kaduk’s DISCUSS position.  To repeat (Ben’s first point), the privacy and security implications of each of these new meta-data items defined in Section 4 needs to be evaluated and documented.

Thanks to Charlie Kaufman for the SECDIR review.

** Section 1.  Typo. s/serveral/several/

** Section 4.1  References needed for certain the CT values.
-- 0x0, Shouldn’t this also reference [IEEE.802.1Q_2018] (just like with 0x1)?
-- 0x2 and 0x3 is there a reference for MPLS VPN and VNI?

** Section 4.5.  Editorial. Multiple Instances of s/Dest Group/Destination Group/

I had started writing this as a DISCUSS position, mainly about the lack of Security Considerations and lack of context / detail around fields, but changed to NoObj because the SecADs are already holding the DISCUESSes, and the fields *can* be figured out with sufficient effort. For future documents, it would be really helpful to have some more information / detail.

As an example: 
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |    Metadata Class = 0x0000    |  Type = TBA4  |U| Length = var|
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      ~                     Source Interface                          ~
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

                Figure 10: Ingress Network Source Interface

   The fields are described as follows:
      Length: Indicates the length of the Source Interface in bytes (see
      Section 2.5.1 of [RFC8300]). 
      Source Interface: Identifier of the ingress interface of the
      ingress network node."

Erm, what format is the Source Interface in? Is "XE-0/0/0.23" an option? What do I *do* with this?! Only after rereading the section multiple times and then hunting down some other SFC documents did I notice the text above the document which says: "This is an opaque quantity to the NSH." -- Ok, fair enough (yes, I'm dumb for not having noticed that in the intro, but this is just an example of a common thread in the document)

A "good" example is:
"Node ID: Represents an opaque value of the ingress network node
      ID.  The structure and semantics of this field are deployment
      specific.  For example, Node ID may be a 4 bytes IPv4 address Node
      ID, or a 16 bytes IPv6 address Node ID, or a 6 bytes MAC address,
      or 8 bytes MAC address (EUI-64), etc,." -- this explains what goes in the field, and a helpful example to situate the reader.

Thanks to Roni Even for their General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/_Ko2OFHveBlBtmwVVHVjz1hewZo).

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

Section 4.2. , paragraph 3, nit:
> , or 8 bytes MAC address (EUI-64), etc,. 
>                                       ^^
Did you just mean "," or "."?

These URLs in the document can probably be converted to HTTPS:
 * http://ieeexplore.ieee.org/servlet/opac?punumber=8403925

I support Ben's discuss for clarifying the length and padding for the flow id in section 4.5.  In particular, I note that the diagram suggests that the size is fixed at 4 bytes, but the text suggests that the length is variable.