kalirajv
commented
1 year ago Hi Jeff,
Ref: https://github.com/ietf-wg-idr/draft-ietf-idr-bgp-ct/issues/27
Thanks for the review.
The previous few versions of the draft (after ct-v12) have been nibbling on these issues in small increments.
Now, I made another pass and fixed what still remained.
Updated draft version: https://www.ietf.org/archive/id/draft-ietf-idr-bgp-ct-16.txt
Please see some comments inline. KV>
Notation: ct-v16.sec-X.Y : section X.Y in draft-ietf-idr-bgp-ct-16
I will update these comments on github too.
Thanks Kaliraj
From: Kaliraj Vairavakkalai [kaliraj@juniper.net](mailto:kaliraj@juniper.net) Date: Friday, September 22, 2023 at 12:36 AM To: Kaliraj Vairavakkalai [kaliraj@juniper.net](mailto:kaliraj@juniper.net) Subject: Abc
Sue asked me to spend some time reviewing -ct to review its current state. Here's that status. A similar effort will be coming for the -car document soon.
A high level concern (repeated in my stream of review comments below) is that features such as multinexthop, mpls namespaces, rt-constrain extensions and cpr are used in a normative sense in places in the document. While the intent seems to be more in the lines of "if this feature was deployed, here's how it'd behave", that pushes it more towards normative than informative in many of the cases. My expectation is that during IESG review the same point will be made. I suspect the best answer is to split those cases out of the core CT document, or target doing so for final publication. (Note that we're in the midst of WGLC, so that's probably NOW.)
KV> [Anchor1] KV> That is correct. These mechanism that are using individual IDs are optional, but important aspects relating to scaling, srv6 support, KV> and in some cases, clarifying interaction with other well deployed features(like flowspec, EPE), which is also important thing to do. KV> And is good to keep these in one place, in the CT doc. KV> When multiple technologies are developing simultaneously, they will be in different stages of adoption, and may need to reference each other. KV> So what is the best way to deal with it, we request IESG and Chairs collective guidance on how to proceed with that. KV> Even if we move these sections to those individual IDs, those drafts will still need to be referenced in this CT draft. So, not sure how KV> that solves the problem of not referencing individual IDs.
Note that I also haven't carried out the document cross-check of the text vs. the opened github issues. I believe Sue is tracking that.
-- Jeff
I spent some energy reviewing the draft in its current state with a focus on content rather than generating a grammar diff.
Things in need of some work:
Some terms get use without definition. Service Node/Border node, for example.
Over-use of commas in many places.
Parenthetical remarks made that are part of the main sentence and shouldn't be parenthetical.
Small things like the latin needs commas after its use. "i.e.," rather than "i.e." Removing "For" prior to "e.g.".
inter-as option A/B/C are used without reference.KV> Provided reference to “Section 10, RFC-4364” in first occurance, in Introduction.
Many of these will get cleaned up by the rfc editor.
Figure 1, the "architecture" isn't really an architecture - it's an example topology leveraging multiple of the components.
KV> The intent is to explain the different constructs in the architecture using brief examples.
"import processing" is used in a non-RFC 4271 normative fashion. What's intended here is processing of a route received by a bgp speaker and its interaction with import policy. I'd suggest that rather than do bgp-speak everywhere, "import processing" should be defined in the terminology section.
KV> ‘import processing’ means ‘receive side processing’. Added in Definitions section.
Issue, Section 5: "The first community on the route that matches a Mapping Community". "First community" isn't well defined in community processing procedures. Implementations tend to locally canonicalize the received set of communities and may internally re-order them without regard for the order of the routes as received in the PDU. What I believe is intended here is the first matching community according to the locally configured policy.
If there are multiple mapping communities on a route for whatever reason, this order may cause different nodes to take different mapping actions as part of their processing. Such behavior may lead to inconsistent routing within a domain. The document does state, "If a route contains more than one Mapping Community, it indicates that the route considers these distinct Mapping Communities as equivalent in Intent." There likely needs to be a caveat that intent equivalence needs be wary of consistent route selection across intents to avoid forwarding loops.
KV> Pls see ct-v16.sec-5.1 for clarified text.
Issue, Section 6: "Note that the length will always be the sum of 20 (number of bits in Label field), plus 3 (number of bits in Rsrv field), plus 1 (number of bits in S field), plus the length in bits of the Prefix (RD:IP prefix)."
The text above already referred to the multiple-label behavior as covered in RFC 8277. The related math within the description of the Length field needs to account for the fact that more than one label may be present.
KV> Removed the illustrative text excerpt from RFC8277. And pointed a reference to Section 2 of [RFC8277].
Issue, Section 6: "When the length of Next hop Address field is 16 (or 32) the next hop address is of type IPv6 address (potentially followed by the link-local IPv6 address of the next hop)." This needs a reference to RFC 2545.
KV> added reference to rfc2545, and rfc4659.
Issue, Section 7.3: "If the resolution process does not find a matching route in any of the associated TRDBs, the received BGP CT route MUST be considered unusable for forwarding purpose and be withdrawn."
While the intent of "and be withdrawn" is intended to say the route isn't usable and should be removed from the Adj-Ribs-Out resulting in a previously advertised path no longer being advertised, it'd be better to simply end this as "MUST be considered unresolvable. (See RFC 4271, Section 9.1.2.1)"
KV> Done. Clarified text.
"The received BGP CT route MUST be added to the TRDB corresponding to the Transport Class "C1". So that service routes can resolve over this BGP CT ingress route. RD is stripped by the ingress node from the BGP CT NLRI prefix when a BGP CT route is added to a TRDB."
This could probably be clearer, especially since the text in prior sections drew the analogy to RFC 8277 rather than RFC 4364 L3VPN procedures.
KV> Implementations may chose to implement it analogous to rfc4364 or in a different way. This text was clarified after the previous rounds of KV> feedbacks we received.
Similarly, it's not clear if the "MUST be added" is covered by the case where the route is unresolvable.
KV> yes, the route is added to the TRDB, even if it is unresolved.
Please consider re-working these paragraphs.
Issues, Section 7.6: "When multiple BNs exist such that they advertise a "RD:EP" prefix to Route Reflectors (RRs)"
This is the first occurrence of RD:EP. Personally, I think "endpoint" is good terminology for describing the ip prefix contained in the CT route, however it's not consistently used in sections prior to this point. It'd be useful, for example, in the section describing resolvability. It'd also be useful in the section covering installing the routes in a specific instance of the TRDB as the component that exists after the RD is stripped.
KV> EP is defined in Terminologies section. Added EP usage in those sections.
The scenario described here also says you SHOULD use add-paths. I'd suggest restructuring this section to lay out motivation first: If it's the case that multiple instances of a given RD:EP exist with different forwarding characteristics, then add-paths is helpful.
KV> Clarified text. Ct-v16.sec-7.6
Issue, Section 7.7: This section motivates the scenario where an ABR route reflector resets the nexthop locally. This isn't a common scenario, but it does happen and it's good to discuss it.
However, the mitigation as a "SHOULD provide a way to alter the tie breaking". Doing this inconsistently is likely to cause the problem described when executing this scenario. The remainder of the section offers operational mitigations.
My recommendation is if this scenario is expected to be common, it is necessary to discuss this as a MUST. Further the section should be called out as "changes to the BGP decision process" in its section name.
KV> Added section “Path selection change”, clarified ct-v16.sec-7 text. Leaving it as SHOULD, since other mechanisms to deal with this problem also exist. KV> The other mechanism ct-v16.sec-7.7.2.first-bullet (carefully chosing IGP metric) is what seems to be widely used today to handle this problem.
Issues, Section 7.8: Section 7.8 repeats the "withdrawn" comment above and needs similar changes.
KV> Done.
The discussion of fallback schemes perhaps deserves some discussion about consistency of scheme configuration within a deployment. Inconsistent resolution due to lack of consistency in the various schemes or their various tables can result in forwarding loops.
KV> Clarified text. ct-v16.sec-5
Issue, Section 7.11: The precedence mechanism discusses multinexthop as a feature. That feature is currently listed as informative rather than normative. I suspect the IESG will catch this as well.
The authors may need to make a choice as to whether to exclude multinexthop from the draft or remove normative use cases of it in the draft. The easiest option would be to exclude it from this draft and include the precedence as a discussion the multinexthop draft.
KV> It is important do discuss the precedence and consistently determining ‘effecting color’ for any route, involving all KV> mechanisms currently specified. KV> About referencing individual IDs, pls see [Anchor1].
Issue, Section 7.12: Informative use for redirect-ip here may also wish to be reconsidered.
KV> OK. Moved this as-well to Normative Ref section. And will wait for collective guidance from IESG and Chairs, as noted in [Anchor1]
Issue, Section 8.2: "An egress SN MAY advertise BGP CT route for RD:eSN with two Route Targets: transport-target:0: and a RT carrying :. Where TC is the Transport Class identifier, and eSN is the IP-address used by SN as BGP next hop in its service route advertisements."
RFC4684 and the eSN in question can accomplish IPv4 nexthops via RT-Constrain behavior.
IPv6 route targets in RT-Constrain is work IDR has adopted, but hasn't advanced - and the existing proposal is a matter of some controversy.
The citation of draft-zzhang-idr-bgp-rt-constrains-extension similarly bends the informative vs. normative discussion here.
KV> Actually here it is nothing more than a reference to the draft, saying this draft is working on how RTC is done for wide-RTs. KV> If this amount of referencing is also not allowed, then we lose recording results of valuable discussions and time KV> that WG has spent. These references are citing specific versions of these individual IDs. I feel it is OK to state KV> that some other drafts are working on this problem. Instead of completely removing the information from this draft. KV> Anyways, as stated in [Anchor1], will wait for collective guidance from IESG and Chairs.
Issue, Section 12.1: The topology ASCII diagram appears to be broken.
KV> It was not. But re-arranged it to be more readable.
Sections 12 and 13 have good information in in them, but may be better suited for the Appendices since they show configuration rather than describe protocol procedure.
KV> These are illustrations of protocol procedures, that we got direct feedback from many readers that KV> they found it immensely helpful to understand the proposal. Hence we want to keep it close to the KV> main text, not move to appendix.
Issue, Section 14.2: It's reasonable to want to describe the reservation for the non-transitive extended community to catch its code point. Please make a point that this code point is not currently used in this draft and that operations against it are reserved for future documents.
KV> Done. ct-v16.sec-4.3
This also impacts the name, the "Transport Class". What you really have are the "Transitive Transport Class" and the "non-Transitive Transport Class". The document thus far refers universally to the transitive one. You can simplify your life by labeling them "Transport Class" and "Non-Transitive Transport Class" to make them fully clear.
The IANA Name fields likely should receive a similar update.
Similarly, this impacts the "Route Target" that is named in both of the new registries.
KV> Done. Updated ct-v16.sec-13.2, Also, added some error-handling text in ct-v16.sec-7.14
I'm calling out this point because of the large extended community reorg work done in the past several years. We don't want to repeat the same headache later for these registrations.
Section 14.4: If you're going to call out the reserved value 0 as having a name, that's good. However, note that the remaining values are left for arbitrary use by the operator.
KV> Updated ct-v16.sec-13.4 with Private Use range.
While it seems dumb to have to point this out, we've had people try to reallocate such things after the fact for protocol purposes. See large bgp communities for example.
Section 15: Congratulations on a very nice security considerations section. The security reviewers will assuredly have something negative to say, but that's how they work.
KV> Thanks! Hope Security review goes smooth.
Appendix A.1.1 again makes normative comments based on the multinexthop feature.
Appendix C.1: The references to the performance results may not be sufficiently stable for publication as an RFC.
Appendix D: Mpls-namespaces perhaps introduce the same normative/informative issues that multinexthop does. It may be more appropriate to move the discussion of the interaction with CT to an appendix of that document.
Appendix E.2, similar consideration for CPR for informative/normative references. Perhaps one different is that CPR is an operational recommendation that doesn't otherwise (at the time of its last revision) alter protocol procedure.
KV> I think these also come under [Anchor1]. Will wait for consolidated recommendation from IESG and Chairs, and do the needful. Thanks!
Sue asked me to spend some time reviewing -ct to review its current state. Here's that status. A similar effort will be coming for the -car document soon.
A high level concern (repeated in my stream of review comments below) is that features such as multinexthop, mpls namespaces, rt-constrain extensions and cpr are used in a normative sense in places in the document. While the intent seems to be more in the lines of "if this feature was deployed, here's how it'd behave", that pushes it more towards normative than informative in many of the cases. My expectation is that during IESG review the same point will be made. I suspect the best answer is to split those cases out of the core CT document, or target doing so for final publication. (Note that we're in the midst of WGLC, so that's probably NOW.)
Note that I also haven't carried out the document cross-check of the text vs. the opened github issues. I believe Sue is tracking that.
-- Jeff
I spent some energy reviewing the draft in its current state with a focus on content rather than generating a grammar diff.
Things in need of some work:
Many of these will get cleaned up by the rfc editor.
Figure 1, the "architecture" isn't really an architecture - it's an example topology leveraging multiple of the components.
"import processing" is used in a non-RFC 4271 normative fashion. What's intended here is processing of a route received by a bgp speaker and its interaction with import policy. I'd suggest that rather than do bgp-speak everywhere, "import processing" should be defined in the terminology section.
Issue, Section 5: "The first community on the route that matches a Mapping Community". "First community" isn't well defined in community processing procedures. Implementations tend to locally canonicalize the received set of communities and may internally re-order them without regard for the order of the routes as received in the PDU. What I believe is intended here is the first matching community according to the locally configured policy.
If there are multiple mapping communities on a route for whatever reason, this order may cause different nodes to take different mapping actions as part of their processing. Such behavior may lead to inconsistent routing within a domain. The document does state, "If a route contains more than one Mapping Community, it indicates that the route considers these distinct Mapping Communities as equivalent in Intent." There likely needs to be a caveat that intent equivalence needs be wary of consistent route selection across intents to avoid forwarding loops.
Issue, Section 6: "Note that the length will always be the sum of 20 (number of bits in Label field), plus 3 (number of bits in Rsrv field), plus 1 (number of bits in S field), plus the length in bits of the Prefix (RD:IP prefix)."
The text above already referred to the multiple-label behavior as covered in RFC 8277. The related math within the description of the Length field needs to account for the fact that more than one label may be present.
Issue, Section 6: "When the length of Next hop Address field is 16 (or 32) the next hop address is of type IPv6 address (potentially followed by the link-local IPv6 address of the next hop)." This needs a reference to RFC 2545.
Issue, Section 7.3: "If the resolution process does not find a matching route in any of the associated TRDBs, the received BGP CT route MUST be considered unusable for forwarding purpose and be withdrawn."
While the intent of "and be withdrawn" is intended to say the route isn't usable and should be removed from the Adj-Ribs-Out resulting in a previously advertised path no longer being advertised, it'd be better to simply end this as "MUST be considered unresolvable. (See RFC 4271, Section 9.1.2.1)"
"The received BGP CT route MUST be added to the TRDB corresponding to the Transport Class "C1". So that service routes can resolve over this BGP CT ingress route. RD is stripped by the ingress node from the BGP CT NLRI prefix when a BGP CT route is added to a TRDB."
This could probably be clearer, especially since the text in prior sections drew the analogy to RFC 8277 rather than RFC 4364 L3VPN procedures.
Similarly, it's not clear if the "MUST be added" is covered by the case where the route is unresolvable.
Please consider re-working these paragraphs.
Issues, Section 7.6: "When multiple BNs exist such that they advertise a "RD:EP" prefix to Route Reflectors (RRs)"
This is the first occurrence of RD:EP. Personally, I think "endpoint" is good terminology for describing the ip prefix contained in the CT route, however it's not consistently used in sections prior to this point. It'd be useful, for example, in the section describing resolvability. It'd also be useful in the section covering installing the routes in a specific instance of the TRDB as the component that exists after the RD is stripped.
The scenario described here also says you SHOULD use add-paths. I'd suggest restructuring this section to lay out motivation first: If it's the case that multiple instances of a given RD:EP exist with different forwarding characteristics, then add-paths is helpful.
Issue, Section 7.7: This section motivates the scenario where an ABR route reflector resets the nexthop locally. This isn't a common scenario, but it does happen and it's good to discuss it.
However, the mitigation as a "SHOULD provide a way to alter the tie breaking". Doing this inconsistently is likely to cause the problem described when executing this scenario. The remainder of the section offers operational mitigations.
My recommendation is if this scenario is expected to be common, it is necessary to discuss this as a MUST. Further the section should be called out as "changes to the BGP decision process" in its section name.
Issues, Section 7.8: Section 7.8 repeats the "withdrawn" comment above and needs similar changes.
The discussion of fallback schemes perhaps deserves some discussion about consistency of scheme configuration within a deployment. Inconsistent resolution due to lack of consistency in the various schemes or their various tables can result in forwarding loops.
Issue, Section 7.11: The precedence mechanism discusses multinexthop as a feature. That feature is currently listed as informative rather than normative. I suspect the IESG will catch this as well.
The authors may need to make a choice as to whether to exclude multinexthop from the draft or remove normative use cases of it in the draft. The easiest option would be to exclude it from this draft and include the precedence as a discussion the multinexthop draft.
Issue, Section 7.12: Informative use for redirect-ip here may also wish to be reconsidered.
Issue, Section 8.2: "An egress SN MAY advertise BGP CT route for RD:eSN with two Route Targets: transport-target:0: and a RT carrying :. Where TC is the Transport Class identifier, and eSN is the IP-address used by SN as BGP next hop in its service route advertisements."
RFC4684 and the eSN in question can accomplish IPv4 nexthops via RT-Constrain behavior.
IPv6 route targets in RT-Constrain is work IDR has adopted, but hasn't advanced - and the existing proposal is a matter of some controversy.
The citation of draft-zzhang-idr-bgp-rt-constrains-extension similarly bends the informative vs. normative discussion here.
Issue, Section 12.1: The topology ASCII diagram appears to be broken.
Sections 12 and 13 have good information in in them, but may be better suited for the Appendices since they show configuration rather than describe protocol procedure.
Issue, Section 14.2: It's reasonable to want to describe the reservation for the non-transitive extended community to catch its code point. Please make a point that this code point is not currently used in this draft and that operations against it are reserved for future documents.
This also impacts the name, the "Transport Class". What you really have are the "Transitive Transport Class" and the "non-Transitive Transport Class". The document thus far refers universally to the transitive one. You can simplify your life by labeling them "Transport Class" and "Non-Transitive Transport Class" to make them fully clear.
The IANA Name fields likely should receive a similar update.
Similarly, this impacts the "Route Target" that is named in both of the new registries.
I'm calling out this point because of the large extended community reorg work done in the past several years. We don't want to repeat the same headache later for these registrations.
Section 14.4: If you're going to call out the reserved value 0 as having a name, that's good. However, note that the remaining values are left for arbitrary use by the operator.
While it seems dumb to have to point this out, we've had people try to reallocate such things after the fact for protocol purposes. See large bgp communities for example.
Section 15: Congratulations on a very nice security considerations section. The security reviewers will assuredly have something negative to say, but that's how they work.
Appendix A.1.1 again makes normative comments based on the multinexthop feature.
Appendix C.1: The references to the performance results may not be sufficiently stable for publication as an RFC.
Appendix D: Mpls-namespaces perhaps introduce the same normative/informative issues that multinexthop does. It may be more appropriate to move the discussion of the interaction with CT to an appendix of that document.
Appendix E.2, similar consideration for CPR for informative/normative references. Perhaps one different is that CPR is an operational recommendation that doesn't otherwise (at the time of its last revision) alter protocol procedure.