IGP Flexible Algorithm
draft-ietf-lsr-flex-algo-26

# Internet AD comments for {draft-ietf-lsr-flex-algo-24}
CC @ekline

## Comments

### S6.4, S7.4

* Is it safer to perhaps say that the M-flag is not only "not applicable"
  for SRv6 locators but add some kind of "MUST NOT be sent" and "MUST be
  ignored" ~type clauses?

  S8 has this kind of language and I think it may help grab the attention
  of an implementer.

### S9

* Should there be any qualifications here for SRv6 locator prefixes?

Thank you to Charlie Kaufman for the SECDIR review.

** Section 4.
   We want the mapping between the Flex-Algorithm and its
   meaning to be flexible and defined by the user.  

Is a “Flex-Algorithm” (with hyphen) the same as a “Flex Algorithm” (no hyphen) defined in Section 3?  Why the difference in notation?

** Section 4.
   The set consisting of (a) calculation-type, (b) metric-type, and (c)
   a set of constraints is referred to as a Flexible-Algorithm
   Definition.

   Flexible-Algorithm is a numeric identifier in the range 128-255 that
   is associated via configuration with the Flexible-Algorithm
   Definition.

This text repeats the text in Section 3 verbatim.  Is it needed twice? 

** Section 5.

... and (b) are in the same Flex-Algorithm
   definition advertisement scope

What is a “FAD advertisement scope?”  Should this be an additional term defined in Section 3?

** Section 5.1.  Should the explanation of “Metric-Type” say that it’s value comes from the “IGP Metric-Type Registry” per Section 18.1.2?

Section 5.1.  Is it possible for a peer not to support a particular Metric-Type?  If so, have is that signaled?

** Section 5.1.
   An IS-IS L1/L2 router MAY be configured to re-generate the winning
   FAD from level 2, ...

What is the “winning FAD?”

** Section 5.1 and 5.2.  Editorial. Definition of Flex-Algorithm in the TLV.

-- Section 5.1
Flex-Algorithm: Single octet value between 128 and 255 inclusive.

-- Section 5.2
      Flex-Algorithm:: Flex-Algorithm number.  Value between 128 and 255
      inclusive.

Should this text be the same?

** Section 5.2.  Editorial. Why is the text defining Metric-Type repeated verbatim from what is written in Section 5.1, but Calc-Type and Priority cite Section 5.1?

** Section 6.4.
   New flag bits may be defined in the future.  Implementations MUST
   check all advertised flag bits in the received IS-IS FADF Sub-TLV -
   not just the subset currently defined.

Let’s assume bits other than those define in this document are set.  Is there an IANA registry to check to understand their semantics?

** Section 13.1.

   During the route computation, it is possible for the Flex-Algorithm
   specific metric to exceed the maximum value that can be stored in an
   unsigned 32-bit variable.  In such scenarios, the value MUST be
   considered to be of value 0xFFFFFFFF during the computation and
   advertised as such.

Should this guidance be referenced in Section 8 – that 0xFFFFFFFF could be 0xFFFFFFFF or an overflow value?

** Section 17.  
   This draft adds two new ways to disrupt IGP networks:

      An attacker can hijack a particular Flex-Algorithm by advertising
      a FAD with a priority of 255 (or any priority higher than that of
      the legitimate nodes).

      An attacker could make it look like a router supports a particular
      Flex-Algorithm when it actually doesn't, or vice versa.


What are the consequences of the described attacker behaviors?  Is “disrupt[ing] the IGP networks” just a denial of service?  Could it also be steering traffic in a particular way to allow inspection or modification; or to avoid network based mitigations?

Thank you very much for this document, and also for working with Linda to address her OpsDir review (https://datatracker.ietf.org/doc/review-ietf-lsr-flex-algo-23-opsdir-lc-dunbar-2022-09-21/ )
Also (obviously!) thanks to Linda for the OpsDir review.

I don't have much to add, other than to support Alvaro's DISCUSS position - a bit more text would be helpful.

Thanks for the specification, I have reviewed this document and haven't find transport protocol related issues.

# Éric Vyncke, INT AD, comments for draft-ietf-lsr-flex-algo-24
CC @evyncke

Thank you for the work put into this document. The underlying concept is brillant and can be useful, but it was hard for me to understand all the details as the document is rather tedious to read (and to author -- so congrats!), I may have misunderstood several points.

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

Special thanks to Acee Lindem for the shepherd's detailed write-up including the WG consensus and the justification of the intended status. ***But***, I regret the lack of reply to the question about `WG discussion and conclusion regarding the IPR disclosures`.

I hope that this review helps to improve the document,

Regards,

-éric

## COMMENTS

### Alvaro's DISCUSS

Just to write that I find Alvaro's DISCUSS points sensible, but I am sure that the authors will address those points.

### Who allocates the flex algo ID

After reading the I-D (and skipping some too deeply technical parts to be honest), I still wonder whether it is the operator who defines locally significant flex algo ID or whether it will be the IETF by standard actions. I was about to raise a DISCUSS on this point.

### Section 1

For the non-expert reader, it would be nice to shortly indicate why this document is about SRv6 locators and not SRv6 SIDs. (no need to explain this to me BTW ;-) )

### Section 4

In `We want the mapping`, who is the "we" ? Please do not use such ambiguous sentence patterns.

In `As long as all routers in the domain`, what is the "domain" ? The OSPF area? the AS? another concept ?

In `The following values area allocated from this registry for Flex-Algorithms`, the reader would benefit to already understand which party (operator/IANA/?) will assign specific values in that range.

### Section 5.1

Even if obvious, it will not hurt to have the unit specified in `Length: variable,`.

### Section 5.1 Calc-Type

I am probably confused and lost due to my lack of knowledge... But the definition of Calc-Type:

1) specifies that type 0 is SPF, which is useless as the IANA https://www.iana.org/assignments/igp-parameters/igp-parameters.xhtml#igp-algorithm-types already says so. I.e., why addin useless/redundant information in the normative part (use of MUST)

2) the same IANA registry https://www.iana.org/assignments/igp-parameters/igp-parameters.xhtml#igp-algorithm-types has a clear indication that value 2-127 are unassigned. I.e., this I-D should not add constraints in this registry.

### Section 5.1 Priority tie

What is the expected behavior when there is a priority tie ? Perhaps add a forward reference to section 5.3 ?

### Sections 6.2 and 6.3

Suggest to repeat the TLV format from section 6.1.

### Sections 6.4, 7.4

Sorry, but cannot understand/parse `Bits that are not transmitted MUST be treated as if they are set to 0 on receipt.` Is the meaning "bits defined by further specification but not in the actual transmitted TLV" ?

What is the expected behavior when length is 0, i.e., I would assume that SRv6 nodes will either do not send this TLV or send it with length=0.

### Section 11

Again a "we" in `we say it`, please rephrase without using ambiguous "we".

### Section 20.1

Is RFC 8986 really normative? I would have assumed it is informative.

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


************* 
As noted in https://www.ietf.org/blog/handling-iesg-ballot-positions/, a
DISCUSS ballot is a request to have a discussion; I really think that the
document would be improved with a change here, but can be convinced otherwise.

Check for IPv4, IPv6, address, NAT, ICMP, MTU

As usual, as the responsible AD for the ADD WG, I have done an AD review before the IETF Last Call. Please find a MD-formatted review below. Before going further, I am requesting the authors to act/reply/comment on all the points below. The end goal is to ease the rest of the publication process.

Please note that Tim Winters is the Internet directorate reviewer (at my request) and you may want to consider this int-dir reviews as well when Tim will complete the review (no need to wait for it though):
https://datatracker.ietf.org/doc/draft-ietf-ippm-rfc8321bis/reviewrequest/16061/

Thanks for addressing my DISCUSS.
https://mailarchive.ietf.org/arch/msg/lsr/F6MnAKpxO0JqmSrr3Ng-C8KnylY/

# GEN AD review of draft-ietf-lsr-flex-algo-23

CC @larseggert

Thanks to Dan Romascanu for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/ttCI9O3hhN2Ryyic_0GWCj-ZDMY).

## 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`
 * Term `native`; alternatives might be `built-in`, `fundamental`, `ingrained`,
   `intrinsic`, `original`

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

### Typos

#### Section 4, paragraph 4
```
-    is associated via configuratin with the Flexible-Algorithm
+    is associated via configuration with the Flexible-Algorithm
+                                 +
```

#### Section 5.3, paragraph 9
```
-    A router that is not participating in a particular Flex-Algorithm is
-                                                                      ^^
-    allowed to advertise FAD for such Flex-Algorithm.  Receiving routers
-   -----------
+    A router that is not participating in a particular Flex-Algorithm MAY
+                                                                      ^^^
```

#### Section 6, paragraph 1
```
-    FAD may be split into multiple such sub-TLVs and the content of the
-        ^^^
+    FAD MAY be split into multiple such sub-TLVs and the content of the
+        ^^^
```

#### Section 6.4, paragraph 10
```
-    Bits that are NOT transmitted MUST be treated as if they are set to 0
-                  ^^^
+    Bits that are not transmitted MUST be treated as if they are set to 0
+                  ^^^
```

#### Section 6.4, paragraph 13
```
-    TLV, all the bits are assumed to be set to 0.
+    TLV, all the bits MUST be treated as set to 0.
```

#### Section 7.4, paragraph 10
```
-    Bits that are NOT transmitted MUST be treated as if they are set to 0
-                  ^^^
+    Bits that are not transmitted MUST be treated as if they are set to 0
+                  ^^^
```

#### Section 7.4, paragraph 12
```
-    the bits are assumed to be set to 0.
+    the bits MUST be treated as set to 0.
```

#### Section 9, paragraph 17
```
-       Reserved: Must be set to 0, ignored at reception.
-                  ^^^
+       Reserved: MUST be set to 0, ignored at reception.
+                  ^^^
```

#### Section 10.2, paragraph 11
```
-       Reserved: Three octets.  Must be set to 0, ignored at reception.
-                                 ^^^
+       Reserved: Three octets.  MUST be set to 0, ignored at reception.
+                                 ^^^
```

#### Section 12, paragraph 2
```
-    then legacy advertisements are to be used, subject to the procedures
-                               ^^^^^^
+    then legacy advertisements MUST be used, subject to the procedures
+                               ^^^^
```

#### Section 13.1, paragraph 1
```
-    Algorithm in the next area or domain, the traffic may be dropped by
-                                                      ^^^
+    Algorithm in the next area or domain, the traffic MAY be dropped by
+                                                      ^^^
```

#### Section 13.1, paragraph 13
```
-    considered to be of value 4,294,967,295 during the computation and
-                              ^^^^^^^^^^^^^
+    considered to be of value 0xFFFFFFFF during the computation and
+                              ^^^^^^^^^^
```

#### Section 14.1, paragraph 4
```
-    paths, MUST be dropped when there are no such paths available.
-         -
```

### Grammar/style

#### Section 2, paragraph 1
```
ation. IGP Algorithm - value from the the "IGP Algorithm Types" registry def
                                  ^^^^^^^
```
Possible typo: you repeated a word.

#### Section 6, paragraph 1
```
red LSP from a given IS MUST be used and any other occurrences MUST be ignor
                                    ^^^^
```
Use a comma before "and" if it connects two independent clauses (unless they
are closely connected and short).

#### Section 6.1, paragraph 4
```
red LSP from a given IS MUST be used and any other occurrences MUST be ignor
                                    ^^^^
```
Use a comma before "and" if it connects two independent clauses (unless they
are closely connected and short).

#### Section 6.2, paragraph 2
```
red LSP from a given IS MUST be used and any other occurrences MUST be ignor
                                    ^^^^
```
Use a comma before "and" if it connects two independent clauses (unless they
are closely connected and short).

#### Section 6.3, paragraph 6
```
red LSP from a given IS MUST be used and any other occurrences MUST be ignor
                                    ^^^^
```
Use a comma before "and" if it connects two independent clauses (unless they
are closely connected and short).

#### Section 7.4, paragraph 11
```
h a Flex-Algorithm prefix metric larger then MAX_PATH_METRIC as defined in [R
                                 ^^^^^^^^^^^
```
Did you mean "larger than"?

#### Section 10.2, paragraph 15
```
efined for each data-plane and is outside of the scope of this document. 12.
                                  ^^^^^^^^^^
```
This phrase is redundant. Consider using "outside".

#### Section 11, paragraph 1
```
 that Flex-Algorithm. No specific two way connectivity check is performed du
                                  ^^^^^^^
```
This word is normally spelled with a hyphen.

#### Section 11, paragraph 2
```
existing, Flex-Algorithm agnostic, two way connectivity check is used during
                                   ^^^^^^^
```
This word is normally spelled with a hyphen.

#### Section 11, paragraph 2
```
-Algorithm value at the same time, and and as such, share the FAD for it. Tr
                                   ^^^^^^^
```
Possible typo: you repeated a word.

#### Section 12, paragraph 8
```
uce an end-to-end path, which is sub-optimal based on Flex-Algorithm constra
                                 ^^^^^^^^^^^
```
This word is normally spelled as one.

#### Section 13, paragraph 7
```
achable for that Flex-Algorithm. Similarly in the case of OSPF, for ASBRs in
                                 ^^^^^^^^^
```
A comma may be missing after the conjunctive/linking adverb "Similarly".

#### Section 13, paragraph 14
```
s defined in Section 5.3 does not includes the M-flag, then the IGP metrics
                                  ^^^^^^^^
```
The auxiliary verb "do" requires the base form of the verb.

#### Section 13.1, paragraph 17
```
efined for each data-plane and is outside of the scope of this document. 15.
                                  ^^^^^^^^^^
```
This phrase is redundant. Consider using "outside".

#### Section 13.1, paragraph 18
```
ch prevents it from being flooded outside of the level in which it was origin
                                  ^^^^^^^^^^
```
This phrase is redundant. Consider using "outside".

#### Section 14.2, paragraph 5
```
-IS, OSPFv2 and OSPFv3 all have well defined handling of unrecognized TLVs an
                                ^^^^^^^^^^^^
```
This word is normally spelled with a hyphen.

#### Section 15.4, paragraph 2
```
Prefix Metric Bits" registry under the under the "Open Shortest Path First (O
                             ^^^^^^^^^^^^^^^^^^^
```
This phrase is duplicated. You should probably use "under the" only once.

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

Hi,

Thanks for this document.  I don't really have much to say.

I was slightly surprised by the IPR declaration 3910, which unless I am misreading it, seems to be asserting possible IPR on the BCP 14 text (i.e., section 2 of revision 03)!  Perhaps worth flagging to see if an update to the IPR declaration is needed?

One minor nit:

(1) p 3, sec 1.  Introduction

   This document also specifies a way for a router to use IGPs to
   associate one or more Segment Routing with the MPLS Data Plane (SR-
   MPLS) Prefix-SIDs [RFC8660], or Segment Routing over IPv6 (SRv6)
   locators [RFC8986] with a particular Flex-Algorithm.

I found this sentence clunky to parse/understand - possibly putting quotes around "Segment Routing with the MPLS Data Plane" and "Segment Routing over IPv6 (SRv6) locators" would help.

Regards,
Rob