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

[Gyan Mishra <[e-mail]>]

Hi John

On Wed, Aug 16, 2023 at 2:46 PM John Scudder <[e-mail]> wrote:

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

    Gyan> I reviewed V7 and it looks good!

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

    Gyan> Thanks for the correction.

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


    Gyan> Should it then be a MUST?

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.


    Gyan> Understood however I do think the introduction if it loosely
implies something without the RFC 2119 language it may be unclear and could
possibly contradict the mention in the procedure.  For consistency it maybe
good to use RFC 2119 language wherever it is applicable which could be any
instance within the specification.

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

 Gyan> Perfect

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

    Gyan> 07 looks good here

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


    Gyan> Yes that did the trick

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


    Gyan>  I am good

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

    Gyan> Agreed I think NHC should be mentioned

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

    Gyan> I reviewed rev 06 however I was going back and forth checking
diff between revisions and may have got stuck on 05 accidentally

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


    Gyan> much better

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


    Gyan> Understood, see my above comments on this subject

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


    Gyan> Yes I support 06 change of default however mentioning about
explicitly stating applicable to all NLRI.  Reason for that is RCA in
itself is for special signaling use cases and for now we have Entropy label
which applies to underlay prefixes next hop FEC, however since the RCA
future use cases that pop up will also be very specialized I think to
mention all NLRI maybe a good idea in this case explicit is better than
implicit.  I don’t think we are over specifying.

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


     Gyan> Will look out for the removal in rev 8, thanks

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


   Gyan> Yes I think as the ELCv3 as Jeff’s draft is directly applicable to
the attribute escape issue here  I think it’s a very good idea to include.

>
>
> Thanks again for the review,
>
> —John
>
-- 

<http://www.verizon.com/>

*Gyan Mishra*

*Network Solutions A**rchitect *

*Email [e-mail] <[e-mail]>*



*M 301 502-1347*