Label Switched Path (LSP) Object Flag Extension for Stateful PCE
RFC 9357

I support Lars's discuss on this, and think the change to RFC2119 terminology would ensure more consistent implementations.

# GEN AD review of draft-ietf-pce-lsp-extended-flags-07

CC @larseggert

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

## Comments

### Section 3.1, paragraph 7
```
     LSP Extended Flags: this contains an array of units of 32-bit flags
     numbered from the most significant as bit zero, where each bit
     represents one LSP flag (for operation, feature, or state).
     Currently no bits are assigned.  Unassigned bits MUST be set to zero
     on transmission and MUST be ignored on receipt.
```
Should this document give some encoding guidance, e.g., "the LSP Extended Flags
field SHOULD (MUST?) use the minimal amount of space needed to encode the flag
bits"? Otherwise one would be free to have trailing 32-bit values with all flags
zero.

### Section 3.2, paragraph 2
```
     The LSP Extended Flags field is an array of units of 32 flags, to be
     allocated starting from the most significant bit.  The bits of the
     LSP Extended Flags field will be assigned by future documents.  This
     document does not define any flags.  Unassigned flags MUST be set to
     zero on transmission and MUST be ignored on receipt.  Implementations
     that do not understand any particular flag MUST ignore the flag.
```
What "unassigned flags" are will change as assignments are made, so this text
would require implementations to (closely) track IANA assignments. Did you maybe
mean "flags that an implementation is not supporting" instead?

### Section 4, paragraph 2
```
     Following the model provided in [RFC8786] Section 3.1, we provide the
     following advice for new specifications that define additional flags.
     Each such specification is expected to describe the interaction
     between these new flags and any existing flags.  In particular, new
     specifications are expected to explain how to handle the cases when
     both new and pre-existing flags are set.  They are also expected to
     discuss any security implications of the additional flags (if any)
     and their interactions with existing flags.
```
I think you mean "describe the interaction between these new flags and *all
existing assigned* flags". That still leaves the issue of what to do when two
documents simultaneously define new flags; that would need to be caught during
standardization.

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

### Grammar/style

#### Section 3.1, paragraph 2
```
or operation, feature, or state). Currently no bits are assigned. Unassigned
                                  ^^^^^^^^^
```
A comma may be missing after the conjunctive/linking adverb "Currently".

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

In the security considerations it says:

   This document provides for future addition of flags in the LSP
   Object.  No additional security issues are raised in this document
   beyond those that exist in the referenced documents.  Note that the
   [RFC8231] recommends that the stateful PCEP extension are
   authenticated and encrypted using Transport Layer Security (TLS)
   [RFC8253], as per the recommendations and best current practices in
   [RFC7525].

It feels that it is trying to say "these flags are protected by the TLS recommendation", but it could probably say that a bit more clearly.

Thanks for this document.  It was well written and clear.  No comments, not even any nits.

Regards,
Rob

Thank you to Shivan Kaul Sahib for the SECDIR review.

** Section 9.  Perhaps repeat what is already said in Section 4 here (“They are also expected to discuss any security implications of the additional flags (if any) and their interactions with existing flags.”):

OLD
  This document provides for future addition of flags in the LSP
   Object.

NEW
This document provides for the future addition of flags in the LSP Object.  The documents which will specific these flags must discuss their associate security implications.

Thank you for writing this document - it seems clear and useful.

Also, much thanks to Bo Wu for the very helpful OpsDir review (https://datatracker.ietf.org/doc/review-ietf-pce-lsp-extended-flags-05-opsdir-lc-wu-2022-10-11/), and to the authors for working with them to address it.

# Éric Vyncke, INT AD, comments for draft-ietf-pce-lsp-extended-flags-07
CC @evyncke

Thank you for the work put into this document. 

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education).

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

I hope that this review helps to improve the document,

Regards,

-éric

## COMMENTS

### Section 1

While I am not sure whether the description of the current use of the 12 bits is useful: suggest to replace `Thus, it is required to extend the flag field of the LSP Object for future use.` but ``This document extends the flag field of the LSP Object for other use.` (it should age better)

### Section 3.1

`Length (16 bits): multiple of 4 octets.` is rather confusing... Is this field counting 32-bit words ? I had to read RFC 5440 to understand that the 'value' part is always a multiple of 4 octets. Strongly suggest to say "Length (16 bits): length of the value expressed in octets".

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