Border Gateway Protocol - Link State (BGP-LS) Extensions for Flexible Algorithm Advertisement
draft-ietf-idr-bgp-ls-flex-algo-12

Thanks for this document, I found it easy to review (I'm sure it helped
that I've recently reviewed draft-ietf-lsr-flex-algo).  I have just a
few comments.

1. In the Security Considerations section, we have

   The TLVs introduced in this document are used to propagate the IGP
   Flexible Algorithm extensions defined in [I-D.ietf-lsr-flex-algo].
   It is assumed that the IGP instances originating these TLVs will
   support all the required security (as described in
   [I-D.ietf-lsr-flex-algo]) to prevent any security issues when
   propagating the TLVs into BGP-LS.

Apart from my usual complaint :-| that "prevent any security issues"
is a notably broad claim, I also have the concern that it isn't clear 
to me what's really being gotten at. 

Maybe you're saying something like "The Security Considerations section
of [I-D.ietf-lsr-flex-algo] discusses potential issues related to 
Flexible Algorithm deployment", in which case just saying that would
seem sufficient (although you don't have to, IMO).  It seems to me that
this document need only address new issues -- if any -- that might 
arise specifically as a result of propagating the information from the 
IGP into BGP-LS.

If it were me I'd probably just remove this paragraph.

Nits:

2. Abstract: "Flexible Algorithm definition" should be "Flexible
   Algorithm Definition" (uppercase D).

3. Various: "ISIS" should be "IS-IS" throughout unless there's 
   some specific reason for the variant spelling.

I concur with John Scudder’s recommended refinements for the Security Considerations text.

I sure wish I'd read / remembered draft-ietf-lsr-flex-algo before getting most of the way through my review and being very confused (I was reviewing it on tablet without Internet access, and didn't have draft-ietf-lsr-flex-algo cached...)

Perhaps it this is just triggered by Eric's "`IGP protocols (OSPF and IS-IS)` kind of assumes that RIPng is not an IGP protocol" comment, but I personally find the first sentence of the Abstract clunky:
"Flexible Algorithm is a solution that allows routing protocols (viz. OSPF and IS-IS) to compute paths over a network based on user-defined (and hence, flexible) constraints and metrics."
I'd think that just "Flexible Algorithm is a solution that allows OSPF and IS-IS to compute paths..." or "Flexible Algorithm is a solution that allows some routing protocols (e.g. OSPF and IS-IS) to compute paths..." (if you think it may be expanded in the future). Again, this may just be because I just read Eric's comment and so was primed... 

3.  Flexible Algorithm Definition
Figure 1:
  *  Length: variable length that represents the total length of the
      value field in octets.  The length value MUST be 4 or larger.

This definition seem wrong / confusing to me; the "variable length that represents the total length" make it sound like the length field is a variable number of octets. This sufficiently confused me that that I had to go and double check that this really isn't a variable length field.

I'm unclear why this isn't instead just something like:
  *  Length: total length of the value field in octets. The length value MUST be 4 or larger.

I realize that this is lifted verbatim from RFC7752 (and IDR often uses the "Length: variable" construct (e.g https://datatracker.ietf.org/doc/rfc9294/), but it still seems like it could be clearer). Actually, the "Length" definitions should be normalized throughout the document -- e.g the descriptions are different between S3.3 and S3.4 -- either one works, but consistency would be nice.








Nits: 
Section 2: "The BGP-LS [RFC7752] specifies the Node Network Layer Reachability" -- it seems like there should be a word before 'specifies', but I cannot really figure out what :-P. "The BGP-LS" isn't really a "protocol", and "attribute" doesn't really seem to fit here. Perhaps "extensions"? Or just drop "The" and hope that no-one notices - I think that the meaning/intent is clear, but how to correctly refer to BGP-LS seems tricky...

Thanks for working on the this specification.

As a part of my discuss resolution I would note that TBD in the type field should be amended by Editor's note or more descriptive text that reflects that this type would be assigned after IANA review.

# Éric Vyncke, INT AD, comments for draft-ietf-idr-bgp-ls-flex-algo-10
CC @evyncke

Thank you for the work put into this document. It is important and easy to read. The -11 revision addresses all my previous DISCUSS/COMMENT/NITS, those are kept below only for archiving.

Special thanks to Jie Dong for the shepherd's detailed write-up including the WG consensus, alas there is no justification for the intended status. 

I hope that this review helps to improve the document,

Regards,

-éric


## Historic DISCUSS 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 3.6 unknown ?

```
      sub-TLV types: Zero or more sub-TLV types that are unknown or
      unsupported by the node originating the BGP-LS advertisement.  The
      size of each sub-TLV type depends on the protocol indicated by the
      Protocol-ID field.  For example, for IS-IS each sub-TLV type would
      be of size 1 byte while for OSPF each sub-TLV type would be of
      size 2 bytes.
```

How would an originating node know that some TLVs are unknown to it ? I am probably missing something obvious here, but some clarification text would be welcome. Or simply use 'unsupported'.

## Historic COMMENTS for archiving


### Section 1

`IGP protocols (OSPF and IS-IS)` kind of assumes that RIPng is not an IGP protocol ;-) Suggest adding 'e.g.,' before OSPF.

### Section 3.1 and others

Suggest adding "sub-TLV" at the end of the section title.

### Section 3.1 sharing tag space ?

Nothing too bad but I wonder why TLV and sub-TLVs share the same 'tag space', i.e., 1039, 1040, ... Mainly out of curiosity...

### Section 3.6, missing reference to IANA

Please add a normative reference to https://www.iana.org/assignments/bgp-ls-parameters/bgp-ls-parameters.xhtml#protocol-ids or at least add the URI in the text.

## NITS

### E.g.,

Please check that all "e.g." are followed by a "," ;-)

### Be consistent with byte or octet

Suggest to select 'octet' everywhere and not mixing 'octet' and 'byte'.

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

# GEN AD review of draft-ietf-idr-bgp-ls-flex-algo-10

CC @larseggert

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

## Comments

### Inclusive language

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term `traditionally`; alternatives might be `classic`, `classical`,
   `common`, `conventional`, `customary`, `fixed`, `habitual`, `historic`,
   `long-established`, `popular`, `prescribed`, `regular`, `rooted`,
   `time-honored`, `universal`, `widely used`, `widespread`

## 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, paragraph 10
```
e is advertising. The following sub-sections define the sub-TLVs for the FAD
                                ^^^^^^^^^^^^
```
This word is normally spelled as one.

#### Section 3.5, paragraph 3
```
ing it for computation purposes. Therefore the FAD is different from most ot
                                 ^^^^^^^^^
```
A comma may be missing after the conjunctive/linking adverb "Therefore".

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

Thanks for resolving my discuss issue.

Regards,
Rob