Hi Acee, Yeah, TBH I probably wouldn’t have flagged it as a problem if the dup case hadn’t been covered at all — but having covered it, I found some bugs in the text. No good deed goes unpunished? I’m sure others in the WG can comment better than I as to why it was seen as necessary to cover such cases — whether it reflects actual experience in the field, or some past reviewer complaining, or some past author being especially diligent.
I think in general it’s a good thing to try to be as specific as we can about what kind of inputs are, and aren’t, allowed and how to deal with unexpected ones, so it’s basically a positive and I would probably not want to tear the text out now that the work has been done. But you make a good point that it’s sort of nuts to ask every document going forward to have similar language. Should there be a short spec (or set of specs) to update the respective base documents, so we can do this once and be done? Thanks, —John > On Apr 28, 2023, at 11:16 AM, Acee Lindem <[email protected]> wrote: > > Hi John, > > All your comments regarding duplication of TLVs got me thinking. We have > covered this in recent RFCs in a more generalized manner. For example, see > https://urldefense.com/v3/__https://datatracker.ietf.org/doc/rfc7684/__;!!NEt6yMaO-gk!E_W7f-scmqTyFPdlBqNfV8awLMiY664KyFwdFTj7PS_jvPbWGnkiSdQjDQ3WmRmqvUBhLdMjNSS3DaM$ > and search for “same”. However, handling this general case of duplicate > information isn’t completely addressed in the base RFCs (at least not for > OSPF with which I’m infinitely more familiar with than IS-IS). For example, > duplicate links in router-LSAs is not covered in RFC 2328. And, with OSPFv3 > (RFC 5340) where the LSA ID is decoupled from the advertised prefix or router > link, it is not covered in the base documents. It was just assumed that no > rationale implementations wouldn’t do this… I’m not sure if we need a > document to fully explain this but this document shouldn’t be burdened with > this specification. > > Peter can comment for IS-IS as to whether this duplication is addressed in > IS-IS specifications. > > Thanks, > Acee > >> On Apr 28, 2023, at 9:13 AM, Peter Psenak >> <[email protected]> wrote: >> >> Hi John, >> >> Shradha and I have worked to address your comments. >> The new version of the draft has been published. >> >> thanks, >> Peter >> >> On 20/04/2023 02:27, John Scudder wrote: >>> Hi Authors, WG, >>> Thanks for this spec and for your patience as it waited for me to review it. >>> I’ve supplied my questions and comments in the form of an edited copy of >>> the draft. Minor editorial suggestions I’ve made in place without further >>> comment, more substantive questions and comments are done in-line and >>> prefixed with “jgs:”. You can use your favorite diff tool to review them; >>> I’ve attached the iddiff output for your convenience if you’d like to use >>> it. I’ve also pasted a traditional diff below in case you want to use it >>> for in-line reply. >>> Thanks, >>> —John >>> --- draft-ietf-lsr-ip-flexalgo-08.txt 2023-04-19 19:07:41.000000000 -0400 >>> +++ draft-ietf-lsr-ip-flexalgo-08-jgs-comments.txt 2023-04-19 >>> 20:24:36.000000000 -0400 >>> @@ -97,6 +97,10 @@ >>> 1. Introduction >>> An IGP Flex-Algorithm as specified in [I-D.ietf-lsr-flex-algo] >>> +--- >>> +jgs: All references to [I-D.ietf-lsr-flex-algo] should be updated to >>> +reference [RFC9350]. >>> +--- >>> computes a constraint-based path to: >>> * All Flex-Algorithm specific Prefix Segment Identifiers (SIDs) >>> @@ -104,7 +108,7 @@ >>> * All Flex-Algorithm specific SRv6 Locators [RFC8986]. >>> - Therefore, Flex-Algorithm cannot be deployed in the absence of SR and >>> + Therefore, Flex-Algorithm cannot be deployed in the absence of SR or >>> SRv6. >>> @@ -128,7 +132,14 @@ >>> 3. Use Case Example >>> Mobile networks are becoming more and more IP centric. Each end-user >>> - session from a gNB (gNodeB) can be destined to a specific UPFs (User >>> + session from a gNB (gNodeB) can be destined to a specific UPF (User >>> +--- >>> +jgs: thanks for expanding gNB, but if you think about it I think you'll >>> +realize that "gNodeB" isn't any more informative to a reader who isn't >>> +already conversant with 3GPP terminology. Please either supply a reference, >>> +rewrite this section in more accessible terms, or remove the section, your >>> +choice. >>> +--- >>> Plane Function) based on the session requirements. For example, some >>> sessions require high bandwidth, others need to be routed along the >>> lowest latency path. Each UPF is assigned a unique IP address. As a >>> @@ -136,14 +147,14 @@ >>> destination IP address. >>> By associating the prefix that contains the UPF addresses with a >>> - specific IP algorithm and routing the algorithm specific traffic >>> - according to a certain constraints, e.g., low latency, a session >>> + specific IP algorithm and routing the algorithm-specific traffic >>> + according to a certain constraints, e.g., low latency, a session's >>> traffic is routed according to the SLA (Service Level Agreement) >>> appropriate for such session. >>> 4. Advertising Flex-Algorithm Definitions (FAD) >>> - To guarantee loop free forwarding, all routers that participate in a >>> + To guarantee loop-free forwarding, all routers that participate in a >>> Flex-Algorithm MUST agree on the Flex-Algorithm Definition (FAD). >>> Selected nodes within the IGP domain MUST advertise FADs as described >>> @@ -183,13 +194,19 @@ >>> Advertisement of participation in IP Flex-Algorithm MUST NOT impact >>> the router participation signaled for other data-planes. >>> +--- >>> +jgs: would it be correct to rewrite the two MUST NOT above as "will not" >>> +or "does not"? If so, please do that. If you are actually using MUST NOT >>> +advisedly in the RFC 2119 sense, I don't understand what you're doing >>> +with these paragraphs -- please help me out. >>> +--- >>> The following sections describe how the IP Flex-Algorithm >>> participation is advertised in IGP protocols. >>> 5.1. The IS-IS IP Algorithm Sub-TLV >>> - The ISIS [ISO10589] IP Algorithm Sub-TLV is a sub-TLV of the IS-IS >>> + The IS-IS [ISO10589] IP Algorithm Sub-TLV is a sub-TLV of the IS-IS >>> Router Capability TLV [RFC7981] and has the following format: >>> 0 1 2 3 >>> @@ -226,7 +243,7 @@ >>> Internet-Draft IP Flex-Algorithm December 2022 >>> - The use of IP Algorithm Sub-TLV to advertise support for algorithms >>> + The use of the IP Algorithm Sub-TLV to advertise support for algorithms >>> outside the Flex-Algorithm range (128-255) is outside the scope of >>> this document. >>> @@ -253,6 +270,13 @@ >>> Figure 2: OSPF IP Algorithm TLV >>> * Type: IP Algorithm TLV (Value TBD by IANA) >>> +--- >>> +jgs: it's a common, and pleasant, convention for authors to stub in >>> +distinguished TBD1, TBD2, ... , TBDn for the different pending code >>> +points. This is a desirable thing from point of view of making sure >>> +the final RFC is error-free; please consider updating your different >>> +placeholders this way. >>> +--- >>> * Length: Variable >>> @@ -286,6 +310,10 @@ >>> flooding scopes (link, area, or Autonomous System (AS)). For the >>> purpose of IP Algorithm TLV advertisement, area-scoped flooding is >>> REQUIRED. >>> +--- >>> +jgs: I take it then, that link and AS-scoped flooding are OPTIONAL? >>> +Although this is implicit, I think it wouldn't hurt to say so. >>> +--- >>> The IP Flex-Algorithm participation advertised in the OSPF IP >>> Algorithm TLV is topology independent. When a router advertises >>> @@ -293,14 +321,18 @@ >>> all topologies in which the advertising node participates. >>> 6. Advertising IP Flex-Algorthm Reachability >>> +--- >>> +jgs: Please do a global search-and-replace of "algorthm" by "algorithm". >>> +I count seven instances of this misspelling. >>> +--- >>> To be able to associate the prefix with the Flex-Algorithm, the >>> - existing prefix reachability advertisements can not be used, because >>> + existing prefix reachability advertisements cannot be used, because >>> they advertise the prefix reachability in default algorithm 0. >>> - Instead, a new IP Flex-Algorithm reachability advertisements are >>> + Instead, new IP Flex-Algorithm reachability advertisements are >>> defined in IS-IS and OSPF. >>> - The M-flag in FAD is not applicable to IP Algorithm Prefixes. Any IP >>> + The M-flag in the FAD is not applicable to IP Algorithm Prefixes. Any >>> IP >>> Algorithm Prefix advertisement includes the Algorithm and Metric >>> fields. When an IP Algorithm Prefix is advertised between areas or >>> domains, the metric field in the IP Algorithm Prefix advertisement >>> @@ -308,8 +340,8 @@ >>> 6.1. The IS-IS IPv4 Algorithm Prefix Reachability TLV >>> - A top-level TLV is defined for advertising IPv4 Flex-Algorithm Prefix >>> - Reachability in IS-IS - IPv4 Algorithm Prefix Reachability TLV. >>> + The IPv4 Algorithm Prefix Reachability top-level TLV is defined for >>> advertising IPv4 Flex-Algorithm Prefix >>> + Reachability in IS-IS. >>> This new TLV shares the sub-TLV space defined for TLVs Advertising >>> Prefix Reachability. >>> @@ -328,6 +360,15 @@ >>> * Type: IPv4 Algorithm Prefix Reachability TLV (Value 126). >>> * Length: Variable. >>> +--- >>> +jgs: "Variable" is a bit terse. I looked at a few other IS-IS RFCs. >>> +There doesn't seem to be a single standard pattern, but they all say >>> +something more helpful than 'variable'. For example, RFC 6232 says, >>> + >>> + LENGTH - total length of the value field. >>> + >>> +Please update the description to be something a little more helpful. >>> +--- >>> @@ -340,6 +381,12 @@ >>> * R bits (4 bits): Reserved for future use. They MUST be set to >>> zero on transmission and MUST be ignored on receipt. >>> +--- >>> +jgs: It's a little unusual in my experience to see this drawn as >>> +four one-bit fields all named "R" instead of one four-bit field >>> +named "Rsvd" or similar. It's fine to leave it if you prefer it >>> +this way, but know that it may surprise others too. >>> +--- >>> * MTID (12 bits): Multitopology Identifier as defined in [RFC5120]. >>> Note that the value 0 is legal. >>> @@ -400,12 +447,47 @@ >>> the lowest-numbered LSP and ignore any subsequent IPv4 Algorithm >>> Prefix Reachability advertisements for the same prefix for any other >>> Algorithm. >>> +--- >>> +jgs: The way you've written this ("each with a different Algorithm") I >>> +don't think you get the outcome you want. Consider if a router receives >>> +four advertisements for the same prefix, with algorithms 128, 129, 130, >>> +and 130, respectively. They are not *each* with a different algorithm, >>> +the last two have the same algorithm, therefore this paragraph doesn't >>> +apply. I don't think that's what you want, in fact in this case I >>> +the algorithm is a red herring. I think you mean something like >>> + >>> + If a router receives multiple IPv4 Algorithm Prefix Reachability >>> + advertisements for the same prefix from the same originator, it >>> + MUST select the first advertisement in >>> + the lowest-numbered LSP and ignore any subsequent IPv4 Algorithm >>> + Prefix Reachability advertisements for the same prefix. >>> + >>> +Or is there some other defined behavior for what the router should do >>> +if the originator sends two or more advertisements for a given prefix >>> +in the same algorithm? >>> +--- >>> A router receiving multiple IPv4 Algorithm Prefix Reachability >>> advertisements for the same prefix, from different originators, each >>> with a different Algorithm, MUST ignore all of them and MUST NOT >>> install any forwarding entries based on these advertisements. This >>> situation SHOULD be logged as an error. >>> +--- >>> +jgs: A similar problem exists here. In this case I don't think you can >>> +ignore talking about the algorithm, because of course it's >>> +normal and expected for different routers to sometimes advertise the >>> +same prefix. So I think you want something like, >>> + >>> + If a router receives multiple IPv4 Algorithm Prefix Reachability >>> + advertisements for the same prefix, from different originators, that >>> + do not all use the same Algorithm, it MUST ignore all of them and >>> + MUST NOT install any forwarding entries based on these >>> + advertisements. This situation SHOULD be logged as an error. >>> + >>> +I think you have six occurrences of this "each with a different >>> +Algorithm" problem. I'll flag the others I find, but please double-check >>> +that I didn't miss any. >>> +--- >>> In cases where a prefix advertisement is received in both a IPv4 >>> Prefix Reachability TLV and an IPv4 Algorithm Prefix Reachability >>> @@ -416,7 +498,7 @@ >>> The IS-IS IPv6 Algorithm Prefix Reachability TLV is identical to the >>> IS-IS IPv4 Algorithm Prefix Reachability TLV, except that it has a >>> - unique type. The type is 127. >>> + distinct type. The type is 127. >>> A router receiving multiple IPv6 Algorithm Prefix Reachability >>> advertisements for the same prefix, from the same originator, each >>> @@ -424,19 +506,25 @@ >>> the lowest-numbered LSP and ignore any subsequent IPv6 Algorithm >>> Prefix Reachability advertisements for the same prefix for any other >>> Algorithm. >>> +--- >>> +jgs: here's another >>> +--- >>> A router receiving multiple IPv6 Algorithm Prefix Reachability >>> advertisements for the same prefix, from different originators, each >>> with a different Algorithm, MUST ignore all of them and MUST NOT >>> install any forwarding entries based on these advertisements. This >>> situation SHOULD be logged as an error. >>> +--- >>> +jgs: and another >>> +--- >>> In cases where a prefix advertisement is received in both an IPv6 >>> Prefix Reachability TLV and an IPv6 Algorithm Prefix Reachability >>> TLV, the IPv6 Prefix Reachability advertisement MUST be preferred >>> when installing entries in the forwarding plane. >>> - In cases where a prefix advertisement is received in both IS-IS SRv6 >>> + In cases where a prefix advertisement is received in both an IS-IS SRv6 >>> Locator TLV and in IS-IS IPv6 Algorithm Prefix Reachability TLV, the >>> receiver MUST ignore both of them and MUST NOT install any forwarding >>> entries based on these advertisements. This situation SHOULD be >>> @@ -496,6 +584,9 @@ >>> each with a different Algorithm, MUST ignore all of them and MUST NOT >>> install any forwarding entries based on these advertisements. This >>> situation SHOULD be logged as an error. >>> +--- >>> +jgs: and another >>> +--- >>> @@ -562,19 +653,24 @@ >>> Internet-Draft IP Flex-Algorithm December 2022 >>> - Metric (4 octets): The algorithm specific metric value. The >>> + Metric (4 octets): The algorithm-specific metric value. The >>> metric value of 0XFFFFFFFF MUST be considered as unreachable. >>> When the OSPFv3 IP Algorithm Prefix Reachability Sub-TLV is present, >>> the NU-bit in the PrefixOptions field of the parent TLV MUST be set. >>> - This is needed to avoid the OSPFv3 IP Algorithm Prefix Reachability >>> - advertisement to contribute to the base algorithm reachability. If >>> + This is needed to prevent the OSPFv3 IP Algorithm Prefix Reachability >>> + advertisement from contributing to the base algorithm reachability. If >>> the NU-bit in the PrefixOptions field of the parent TLV is not set, >>> the OSPFv3 IP Algorithm Prefix Sub-TLV MUST be ignored by the >>> receiver. >>> The metric value in the parent TLV is RECOMMENDED to be set to >>> LSInfinity [RFC2328]. >>> +--- >>> +jgs: if the RECOMMENDATION is ignored, what will be the outcome? In >>> +other words, why is this not a MUST? Under what circumstances would it >>> +be OK for an implementor to ignore the recommendation? >>> +--- >>> An OSPFv3 router receiving multiple OSPFv3 IP Algorithm Prefix >>> Reachability Sub-TLVs in the same parent TLV, MUST select the first >>> @@ -586,6 +682,9 @@ >>> each with a different Algorithm, MUST ignore all of them and MUST NOT >>> install any forwarding entries based on these advertisements. This >>> situation SHOULD be logged as an error. >>> +--- >>> +jgs: and another >>> +--- >>> In cases where a prefix advertisement is received in any of the LSAs >>> advertising the prefix reachability for algorithm 0 and in an OSPFv3 >>> @@ -607,9 +706,9 @@ >>> advertise a Flex-Algorithm specific metric associated with the >>> corresponding ASBR LSA. >>> - As described in [I-D.ietf-lsr-flex-algo] each data-plane signals the >>> + As described in [I-D.ietf-lsr-flex-algo] each data-plane signals its >>> participation independently. IP Flex-Algorithm participation is >>> - signaled independent of the Segment Routing (SR) Flex-Algorithm >>> + signaled independent of Segment Routing (SR) Flex-Algorithm >>> @@ -685,9 +784,9 @@ >>> 7. Calculating of IP Flex-Algorthm Paths >>> The IP Flex-Algorthm is considered as yet another data-plane of the >>> - Flex-Algorithm as described [I-D.ietf-lsr-flex-algo]. >>> + Flex-Algorithm as described in [I-D.ietf-lsr-flex-algo]. >>> - Participation for the IP Flex-Algorithm is signalled as described in >>> + Participation in the IP Flex-Algorithm is signalled as described in >>> Section 5 and is specific to the IP Flex-Algorithm data-plane. >>> Calculation of IP Flex-Algorithm paths follows what is described in >>> @@ -734,6 +833,15 @@ >>> Algorithm, LFA paths to the prefix MUST be calculated using such >>> Flex-Algorithm in the associated topology, to guarantee that they >>> follow the same constraints as the calculation of the primary paths. >>> +--- >>> +jgs: The above paragraph appears to be redundant with the final >>> +paragraph of Section 10, Protection. Consider removing it, or providing >>> +a forward reference to Section 10, or alternately (no pun intended) remove >>> +the redundant paragraph in Section 10 and refer back to this paragraph. >>> + >>> +If you don't remove it, please expand LFA on first use and supply >>> +a citation for it. >>> +--- >>> 9. Deployment Considerations >>> @@ -798,6 +906,10 @@ >>> This specification updates the OSPF Router Information (RI) TLVs >>> Registry as follows: >>> +--- >>> +jgs: this is where I remind you about my strong suggestion to change out >>> +"TBD" for "TBD1", "TBD2", etc. >>> +--- >>> +=======+==================+===========================+ >>> @@ -808,7 +920,7 @@ >>> Table 1 >>> - This document also updates the ISIS "Sub-TLVs for TLV 242" registry >>> + This document also updates the IS-IS "Sub-TLVs for TLV 242" registry >>> as follows: >>> @@ -862,6 +974,18 @@ >>> Reachability TLV and IPv6 Algorithm Prefix Reachability TLV. It also >>> includes these TLVs in a table which lists the presence of Sub-TLVs >>> in a parent TLVs as follows: >>> +--- >>> +jgs: I think the above could be a little clearer. Perhaps, >>> + >>> + Since the above TLVs share the sub-TLV space managed in the "IS-IS >>> + Sub-TLVs for TLVs Advertising Prefix Reachability" registry, IANA is >>> + requested to add "IPv4 Algorithm Prefix Reachability TLV (126)" and >>> + "IPv6 Algorithm Prefix Reachability TLV (127)" to the list of TLVs in >>> + the description of that registry. >>> + >>> + In addition, columns headed '126' and '127' are added to that registry, >>> + as follows: >>> +--- >>> Type Description 126 127 >>> @@ -931,6 +1055,25 @@ >>> This document inherits security considerations from >>> [I-D.ietf-lsr-flex-algo]. >>> +--- >>> +jgs: This is an excessively terse Security Considerations section. Have >>> +you carefully thought through the security implications of the current >>> +proposal and concluded that it really introduces nothing new? For >>> +example, the 'each with a different Algorithm' paragraphs could make it >>> +possible for an attacker to cause a prefix to be ignored -- >>> + >>> +Consider the case where a legitimate router, A, advertises prefix P in >>> +algorithm 128. An attacker wants to prevent P from being reachable, and >>> +uses a subverted router, B, to advertise prefix P in algorithm 129. >>> +Following the paragraphs I mention, I think what happens is the attacker >>> +successfully has prevented P from being reachable. I don't think this >>> +attack exists in the underlying RFC 9350. >>> + >>> +Quite likely your conclusion after considering this would be just like >>> +RFC 9350: "... it is not different from advertising any other incorrect >>> +information through IS-IS or OSPF". But that doesn't relieve you of the >>> +need to say that. >>> +--- >>> 13. Acknowledgements >>> @@ -965,6 +1108,10 @@ >>> information exchange protocol for use in conjunction with >>> the Protocol for providing the Connectionless-mode Network >>> Service (ISO 8473)", August 1987, <ISO/IEC 10589:2002>. >>> +--- >>> +jgs: "IANA" can't possibly be the correct author, I'm sure you mean "ISO". >>> +Please check the above citation carefully. >>> +--- >>> [RFC2119] Bradner, S., "Key words for use in RFCs to Indicate >>> Requirement Levels", BCP 14, RFC 2119, >>> _______________________________________________ >>> Lsr mailing list >>> [email protected] >>> https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/lsr__;!!NEt6yMaO-gk!E_W7f-scmqTyFPdlBqNfV8awLMiY664KyFwdFTj7PS_jvPbWGnkiSdQjDQ3WmRmqvUBhLdMjNqjovBY$ >>> >>> <https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/lsr__;!!NEt6yMaO-gk!E_W7f-scmqTyFPdlBqNfV8awLMiY664KyFwdFTj7PS_jvPbWGnkiSdQjDQ3WmRmqvUBhLdMjNqjovBY$ >>> > >> >> _______________________________________________ >> Lsr mailing list >> [email protected] >> https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/lsr__;!!NEt6yMaO-gk!E_W7f-scmqTyFPdlBqNfV8awLMiY664KyFwdFTj7PS_jvPbWGnkiSdQjDQ3WmRmqvUBhLdMjNqjovBY$ _______________________________________________ Lsr mailing list [email protected] https://www.ietf.org/mailman/listinfo/lsr
