Re: [Idr] Rtgdir early review of draft-ietf-idr-entropy-label-06

[John Scudder <[e-mail]>]

Hi Gyan,

Thanks for your review! Some comments in line below. I’ve posted version 07 which includes changes related to your review, as well as Wes Hardaker’s secdir early review, and another change in Section 2.2 to make it even clearer that when changing a next hop, a BGP speaker can’t just blindly propagate TLVs.

> On Aug 16, 2023, at 2:16 AM, Gyan Mishra via Datatracker <[e-mail]> wrote:
> 
> [External Email. Be cautious of content]
> 
> 
> Reviewer: Gyan Mishra
> Review result: Has Issues
> 
> The draft has few issues that should be addressed.
> The authors did an excellent job combining ELCv3 with NHC.
> 
> Draft overview & history and issues solved by this draft:
> ELCv1 RFC 6790 - ELC  optional transitive path attribute 28 that unfortunately
> gets propagated by transit nodes not supporting per RFC 4271 and not remove as
> desired.
> 
> RFC 7447 deprecated the RFC 6790 code point - optional transitive attribute
> 
> ELCv2 Juniper implementation copies next hop into data field of the path
> attribute for signaling check, uses the original RFC 6790 code point deprecated
> by RFC 7447.
> 
> ELCv3 rev 00 is identical to ELCv2 Juniper implementation optional transitive
> path attribute, but now with a new code point IANA allocation and thus is
> required per RFC 8216 must be Standards track.
> 
> NHC draft - Generic next hop capability which can be used for the EL signaling-
> non transitive BGP attribute.
> 
> https://urldefense.com/v3/__https://datatracker.ietf.org/doc/html/draft-ietf-idr-next-hop-capability-08__;!!NEt6yMaO-gk!FCkckSMFYnyqzaSi8exhNBxxRXHwXNUgtbfLRoS-igRyaR3V8pJKYeXwjM3MfNoeh5iGigul4tbt$
> 
> ELCv3 rev 01 – ELCv3 + NHC – Generic next hop capability to support other use
> cases
> 
> Juniper customers running ELCv2 with deprecated code point from RFC 7447 need a
> resolution so the goal of IDR is to get a draft.  Juniper has an attribute 28
> discard knob to strip attribute 28.
> 
> An issue was reported recently with attribute 28 ELC caused session reset on
> versions of Juniper code due to incorrectly flagging the packets as corrupt.
> 
> https://urldefense.com/v3/__https://labs.ripe.net/author/emileaben/unknown-attribute-28-a-source-of-entropy-in-interdomain-routing/__;!!NEt6yMaO-gk!FCkckSMFYnyqzaSi8exhNBxxRXHwXNUgtbfLRoS-igRyaR3V8pJKYeXwjM3MfNoeh5iGio9byzCG$
> 
> ELCv3 Rev 2   no change
> ELCv3 Rev 3  Section 3.1 added Readable label depth
> ELCv3 Rev 4  Security consideration next hop capability filtering in/out &
> trust relationship ELCv3 Rev 5  IANA codepoint 0 Reserved, By default RCA must
> not be accepted ELCv3 Rev 6 BY default RCA must be accepted
> 
> There are no implementations of ELCv3 or NHC.

A nit and a correction to the above — I wouldn’t have said “no implementations” but “no reported implementations”, because it’s famously hard to prove a negative. But in fact, there is a reported implementation of ELCv3, see https://wiki.ietf.org/group/idr/BGP-Implementation-report/draft-ietf-idr-entropy-label

> Major issues:
> None
> 
> Minor issues:
> Updates & Recommendations to RFC 2119 Normative language
> 
> 2nd to last paragraph of Introduction
> 
> Old
> 
> An RCA carried in a given BGP UPDATE message conveys information that relates
> to all NLRI advertised in that particular UPDATE, and only to those NLRI. A
> different UPDATE message originated by the same source might not include an
> RCA, and if so, NLRI carried in that UPDATE would not be affected by the RCA.
> By implication, if a router wishes to use RCA to describe all NLRI it
> originates, it needs to include an RCA with each UPDATE it sends. In this
> respect, despite its similar naming, the RCA is unlike RFC 5492 BGP
> Capabilities.
> 
> New
> 
> An RCA carried in a given BGP UPDATE message MAY convey information that
> relates to all NLRI advertised in that particular UPDATE, and only to those
> NLRI. A different UPDATE message originated by the same source might not
> include an RCA, and if so, NLRI carried in that UPDATE would not be affected by
> the RCA. By implication, if a router wishes to use RCA to describe all NLRI it
> originates, it MUST include an RCA with each UPDATE it sends. In this respect,
> despite its similar naming, the RCA is unlike RFC 5492 BGP Capabilities.

I’m electing to let the language stand as written. I think the first suggested change to MAY is mistaken since if you refer to RFC 2119’s definition of MAY it indicates something which is completely optional — and it’s not optional for the RCA to convey this information, it’s the sole purpose of the RCA. The second suggestion, changing “needs to” to “MUST”, would be OK, but I generally prefer to minimize the use of RFC 2119 keywords to actual elements of procedure, and this paragraph is more in the nature of explanation, to elaborate on natural consequences of the design.

> Section 2.1 – I think it would be good to more clearly correlate the “address
> given in the header portion of the RCA” and what that is as it is referred back
> t in section 2.3.

I see what you mean. I added an xref from Section 2.3 back to Figure 1.

> Old
> 
> The BGP Router Capabilities attribute (RCA attribute, or just RCA) is an
> optional, transitive BGP path attribute with type code 39. The RCA has as its
> data a network layer address, representing the next hop of the route the RCA
> accompanies.
> 
> New
> 
> The BGP Router Capabilities attribute (RCA attribute, or just RCA) is an
> optional, transitive BGP path attribute with type code 39. The RCA header has
> its data field set to the network layer address, representing the next hop of
> the NLRI AFI / SAFI route.

The more I looked at this, the more I wanted to rewrite. Version 06 has:

   The BGP Router Capabilities attribute (RCA attribute, or just RCA) is
   an optional, transitive BGP path attribute with type code 39.  The
   RCA has as its data a network layer address, representing the next
   hop of the route the RCA accompanies.  The RCA signals potentially
   useful optimizations, so it is desirable to make it transitive; the
   next hop data is to ensure correctness if it traverses BGP speakers
   that do not understand the RCA.

Which is what you offered an edit to. But then the next paragraph is:

   The Attribute Data field of the RCA attribute is encoded as a header
   portion that identifies the originator of the attribute, followed by
   one or more capability TLVs.

The first clause of this is redundant with the previous paragraph either in your version or the 06 version. I don’t want to simply get rid of the previous paragraph, because I think it provides helpful context for a person trying to understand the spec, but I’ve tried to make it clear in this rewrite that the first paragraph is just providing an overview.

NEW:
   The BGP Router Capabilities attribute (RCA attribute, or just RCA) is
   an optional, transitive BGP path attribute with type code 39.  The
   RCA always includes a network layer address identifying the next hop of
   the route the RCA accompanies.  The RCA signals potentially useful
   Information, so it is desirable to make it transitive; the next hop
   data is to ensure correctness if it traverses BGP speakers that do
   not understand the RCA. This is further explained below.

   The Attribute Data field of the RCA attribute is encoded as a header
   portion that identifies the originator of the attribute, followed by
   one or more capability Type-Length-Value (TLV) triples:

Hopefully, that rewrite, plus the reference in Section 2.3 to “the header portion of the RCA” (which correlates with “a header portion that identifies the originator”, above), and the xref to Figure 1 in Section 2.3, clarifies things as you’re seeking.

> Rewritten to make more clear
> I would  not call RCA an optimization but rather a way to provide next hop
> intelligence in an opaque data fields that is extensible to a variety of
> meanings
> 
> Old
> The RCA signals potentially useful optimizations, so it is desirable to make it
> transitive; the next hop data is to ensure correctness if it traverses BGP
> speakers that do not understand the RCA. New The RCA signals potentially useful
> next hop extensibility logic, so it is desirable to make it transitive; the
> next hop data is to ensure correctness in cases where it may traverses BGP
> speakers that do not understand the RCA.

Fair enough. I went with “… signals potentially useful information” instead, in the interest of brevity. 

> Nits:
> Abstract
> Should say a “new optional transitive attribute” as its optional.

I’ve let it stand as “defines a BGP transitive attribute”. In my view, the purpose of an abstract is to, well, abstract the essence of the RFC. The fact the attribute is transitive is of the essence, it drives the rest of the design. The fact it’s optional is, relatively speaking, a detail. It’s also a detail that will be intuitively obvious to experienced BGP implementors since all new BGP attributes are optional. (I guess we could define a new “well-known” attribute in principle, but it’s hard to see how to do it in practice short of a version bump, and it hasn’t happened in the history of the protocol.) At some point in the revision history, we dropped “new” also, since it doesn’t age well.

> As the NHC was folded into ELCv3 no need to reference this draft.
> 
> https://urldefense.com/v3/__https://datatracker.ietf.org/doc/html/draft-ietf-idr-next-hop-capability-08__;!!NEt6yMaO-gk!FCkckSMFYnyqzaSi8exhNBxxRXHwXNUgtbfLRoS-igRyaR3V8pJKYeXwjM3MfNoeh5iGigul4tbt$

Although the Datatracker state captures the revision history, Datatracker state isn’t archival in the same way the RFC series is, so it seems potentially valuable to future readers to provide this breadcrumb if they’re looking into the history of the document (hey, it could happen). I’m not hell-bent on keeping the reference, but it seems like it falls in the “can’t hurt, might help” category.

As an aside, although your review says it’s for version 06, you’re quoting from 05. This is fine, especially since you’ve provided detailed feedback on the important new content that came in 06, but FYI. (The tell was the “new” in the quoted text.)

> Introduction
> This specification defines a new BGP optional transitive attribute to carry
> such capability information, the "Router Capabilities Attribute", or RCA. (This
> somewhat ponderous name is regrettable but is needed in order to be descriptive
> while still distinguishing it from RFC 5492 BGP Capabilities.) Maybe you can
> exlclude in (This somewhat ponderous name is regrettable but is needed in order
> to be descriptive while still distinguishing it from RFC 5492 BGP
> Capabilities.)  I think RCA is an appropriate term to the use case and as it
> may seem confusing RFC 5492 “capability advertisement with BGP” is subjective
> only that the word “capability” is used in both but RFC 5492 is “BGP
> capability” update with the addition of capability options parameter type 2 &
> error handling  where RCA is about “router capability” advertisement.

Possibly the update made to address Wes Hardaker’s review will help with this concern also? Please take a look.

> ! Making this next two sentences easier to parse
> 
> Old
> Since the RCA is intended chiefly for conveying information about forwarding
> plane features, it needs to be regenerated whenever the BGP route's next hop is
> changed. Since owing to the properties of BGP transitive attributes this can't
> be guaranteed (an intermediate router that doesn't implement this specification
> would be expected to propagate the RCA as opaque data), the RCA identifies
> itself with the next hop of its originator. New Since RCA is intended for
> conveying information about forwarding plane features, RCA MUST to be
> regenerated whenever the BGP route's next hop is changed.  Transitiveness of
> BGP path attributes does not guarantee this behavior (an intermediate router
> that doesn't implement this specification would be expected to propagate the
> RCA as opaque data), thus requiring the RCA to identify itself with the next
> hop of its originator.

As discussed above, I prefer to reserve RFC 2119 keywords for elements of procedure, not supplemental explanation. 

> Last sentence in introduction
> 
> Old
> It updates [RFC6790] and [RFC7447] with regard to this BGP signaling, this is
> further discussed in Section 3. New It updates [RFC6790] and [RFC7447] with
> regard to this BGP signaling, discussed further in Section 3.

AFAICT the change is to drop “this is”? I prefer how it scans the old way. The RFC Editor might make changes of course.

> I am in agreement with the updates from Rev 5 to Rev6 in the changes made in
> Section 2.2 & 2.3.  

Thanks. I am so glad to know we've had extra eyes on that, since it’s a non-trivial change!

> In particular that the implementation should accept the RCA
> by default.  In section 2.2 & 2.3 sending & receiving the RCA is the default
> applicable to all NLRI and if so that should be specified.  As it is
> recommended to have configuration control and fine grain control that maybe
> available for attribute propagation  by implementations for advertising and
> accepting RCA, I think its OK to send & receive RCA for ALL NLRI by default.

I’m not sure if you’re proposing a further change, or emphasizing that you support the 06 change? It’s true that we don’t have “for all NLRI” in 06, but I think that’s implicit in "An implementation SHOULD send the RCA and its contained capabilities by default” — since we don’t limit the statement, by implication there’s “for all NLRI” at least the way I’d construe it… or actually, “for all applicable NLRI”. Related to that last point, we also don’t say that you should only send the RCA when it makes sense to — I suppose a very naive implementor could read this text and say “oh it says I have to send the RCA always” and then send an empty RCA with every update, even if there’s nothing to signal. I don’t *think* we need to go to that level of detail, there’s a fine line to walk between being specific enough, and overspecifying. 

> Since the RCA is being used for signaling of the next hop data field extensible
> to any TLVs that require such as entropy label for load balancing or any other
> use case I am not sure how the path attribute could impact routing and cause a
> routing loop as its not a mandatory path attribute and is optional transitive
> but is not part of the BGP best path selection algorithm.   In cases where all
> nodes do not understand the capability which is understandable and the RCA
> draft accounts for that with regards to propagation w/o regeneration with next
> hop changes results in mismatch and the RCA is discarded.  So that does provide
> the interoperability with other non supporting nodes and provides incremental
> deployment so now sure how the RCA could result in routing loops.  Maybe this
> subject should be expanded upon if its a real danger.
> 
> The presence of a capability SHOULD NOT influence route selection or route
> preference, unless tunneling is used to reach the BGP next hop or the selected
> route has been learned from External BGP (that is, the next hop is in a
> different Autonomous System). Indeed, it is in general impossible for a node to
> know that all BGP routers of the Autonomous System (AS) will understand a given
> capability, and if different routers within an AS were to use a different
> preference for a route, forwarding loops could result unless tunneling is used
> to reach the BGP next hop.¶

I’m not sure if you’re requesting a change in the quoted paragraph? This is more or less a statement of general BGP safety conditions, and I accept that we could leave it out completely and just assume every implementor already knows this stuff — but I don’t see it as causing harm to remind people. In general, with any extensible mechanism like this one, we can’t know what people might use it for in the future, so it seems to me it’s better to suggest some guardrails up front.

> Section 2.5 Talks about the Anycast use case
> 
> AFAIK with MPLS unidirectional LSP is possible and LDP downstream unsolicited
> from downstream to upstream node label mapping message for the FEC label
> binding.  If we had multiple egress PE LER with the same loopback the LSP would
> only build to one of the egress LSRs not all of them.  I think this section
> should be removed as Anycast is really only relevant in the context of SR-MPLS
> using the Anycast SID where the node-sid loopback0 is defined on multiple
> egress PE’s for resiliency or on inter-domain boundaries with node-sid same
> loopback on all ASBRs.

I’ve left it in for now, not because I disagree with you but because I need a little more time to think this through/discuss it and wanted to get 07 published. Noted for a potential update in version 08. (Note that the network operation considerations section has grown another paragraph in 07, so the section will remain even if the paragraph you've analyzed is removed).

> Section 6.1
> 
> Good idea to include OAD draft here to incorporate into the specification for
> RCA use cases "inter admin domain" RCA localization for cooperating ASN’s in an
> OAD. https://urldefense.com/v3/__https://datatracker.ietf.org/doc/draft-uttaro-idr-bgp-oad/__;!!NEt6yMaO-gk!FCkckSMFYnyqzaSi8exhNBxxRXHwXNUgtbfLRoS-igRyaR3V8pJKYeXwjM3MfNoeh5iGijyAGPnR$
> 
> As the RCA optional transitive attribute is likely to be propagated inter
> domain resulting in “attribute escape” it is at least limited to the receiving
> ASBR which will fail the next hop check and discard the RCA.  The next hop
> within the operator domain would be then then exposed to the other carriers
> domain but limited to the 1st ASBR within the domain.  Care must be taken with
> unwanted leakage of next hop data field and unintentional exposure and
> consequences.
> 
> https://urldefense.com/v3/__https://datatracker.ietf.org/doc/draft-haas-idr-bgp-attribute-escape/__;!!NEt6yMaO-gk!FCkckSMFYnyqzaSi8exhNBxxRXHwXNUgtbfLRoS-igRyaR3V8pJKYeXwjM3MfNoeh5iGim1fl6Yq$

Do I understand correctly you’re also suggesting a reference to the Attribute Escape draft? Good suggestion, also queued for consideration in 08.

Thanks again for the review,

—John