Re: [Idr] WGLC is for "BGP Router Capabilities Attribute” [was: Re: WG LC for draft-ietf-idr-entropy-09 (8/29 to 9/12/2023)]

[John Scudder <[e-mail]>]

Hi Ketan,

Thanks for your review. Comments in line.

> On Sep 22, 2023, at 3:03 PM, Ketan Talaulikar <[e-mail]> wrote:
> 
> Hello All,
> 
> I am probably quite late in sending out these comments for the WGLC. Nevertheless, they are for the v11:
> 
> Sec 1:
> 
> Although ELCv3 is only relevant to labeled NLRI, ...
> 
> Is "labeled NLRI" a well-known/defined term. Does it indicate only those SAFIs that use RFC8277 based encoding or would it also cover EVPN as an example? Would it not be better to just say SAFIs that use MPLS forwarding ... or something like that?

I don’t have a strong opinion about this and am open to further input. I wouldn’t be keen to re-introduce the term “SAFIs” if we can avoid it, though, it seems better to stick with talking about NLRI formats.

> Sec 2.1:
> 
> The NHC 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 NHC.
> 
> I would expect we would never put info that is not useful into BGP ;-) ... Perhaps what was meant here was that the choice of making it transitive was to ensure it would get propagated via BGP speakers (like RRs) that do not change the NH and therefore are not in the forwarding path?

True enough. Do you think the sentence as written is incorrect, misleading, or otherwise problematic? I don’t mind revising it, but at this stage, I don’t want to rewrite it just for fun. :-) 

> Sec 2.1
> 
> The Attribute Data field of the NHC attribute is encoded as a header portion that identifies the router that created or most recently updated the attribute, ...
> 
> Strictly speaking, what is encoded is a NH and can we say that it "identifies the router that ..."? Considering anycast scenario described in sec 2.5?

Since we are being pedantic :-) I think yes, we can say that. In the anycast case it may not *uniquely* identify the router that created/updated it, but as you say, that is explored in §2.5, so I think the imprecision isn’t harmful.

> Sec 2.1
> 
> Capability Code: a two-octet unsigned binary integer that indicates the type of capability advertised and unambiguously identifies an individual capability.
> 
> The use of "binary integer" is somewhat confusing. Is it just a number code point similar to what the IANA considerations suggest, or is it something that can be encoded differently as a binary string?

TBH I don’t know why “binary” is there. Removed in my local copy. (Same for the use in “Capability Length”.)

> Sec 2.2
> 
> Any capability TLVs received by S that are for capabilities not supported by S will not be included in the version of R constructed by S.
> 
> Perhaps it should be "... the version of NHC constructed by S"?


I think the text is correct as it stands, but I concede it could be more precise (not sure if it needs to be). If making it more precise, I think,

OLD:
Any capability TLVs received by S that are for capabilities not supported by S will not be included in the version of R constructed by S.

NEW:
Any capability TLVs received by S that are for capabilities not supported by S will not be included in the newly-constructed NHC attribute S includes with R.

This wording is chosen to closely mirror the earlier sentences in the paragraph. I’ll make this change in my working copy pending any further discussion.

> Sec 2.2
> 
> An implementation SHOULD send the NHC and its contained capabilities by default. An implementation SHOULD provide configuration control of whether any given capability is sent. 
> 
> It is not clear what is meant by "send" here? Is this referring to propagation or origination or both? The next sentence is clear in this respect.

If I recall correctly, the intent here was both propagation and origination, but if we just changed “send” accordingly that would produce a silly result because the minimal change would result in text like “… SHOULD send (either propagate or originate) the NHC…” and we really don’t want to *insist* that the attribute be originated, just that an implementation should be free to originate it whenever that would make sense to do (based on other aspects of the router). The question of origination, then, really comes down the the contained capabilities, I think probably the most important thing to be clear about here is propagation.

So, I’ve changed “send” and “sent” to “propagate” and “propagated” in my working copy, pending further discussion. 

I hope we won’t need to introduce a definition of what it means to propagate a path attribute, next? <fingers crossed>

> Sec 2.3
> 
> An implementation SHOULD accept the NHC and its contained capabilities by default. An implementation SHOULD provide configuration control of whether any given capability is accepted.
> 
> Could you clarify what exactly is meant by "accept" above? The rest of this section explains the receiving, validation and use. Is this about a route policy that discards the attribute on reception or which scrubs out certain capabilities?

The context of this is that versions 05 and prior had,

   By default, the RCA MUST NOT be accepted from peers not under the
   administrative control of the local network administrator (so,
   generally, from EBGP peers); if received it MUST be discarded without
   further processing, except that the contents MAY be logged.  An
   implementation MAY enable RCA processing by default from peers under
   the administrative control of the local network administrator (so,
   generally, from IBGP peers).  An implementation SHOULD provide the
   ability to modify these default settings by configuration.

I imagine you can see how, once the decision was made to flip the default behavior, the current language ensued. If you genuinely think “accept” is too ambiguous in the present version, I’m open to suggested text. I really did think “accept” was enough of a part of the BGP vernacular that it was clear enough, though. For example, RFC 4271 uses the term pretty freely, e.g. "Operations of a BGP speaker that is configured to accept routes with its own autonomous system number in the AS path are outside the scope of this document."

> Sec 3.2/3.3
> 
> These sections do not discuss the scenario where a router receives the same route from different peers and some of them are received with ELCv3 but others are not. Since ELC3 does not affect best path calculation, whether the router propagates ELCv3 further depends on the ELCv3 on the best path that it has selected. There is also the scenario where we have multipath with a mix of capabilities.

Well, that’s fun! And pathological, too, but that doesn’t mean it won’t happen.

I think in the simpler case, where the best path has (or doesn’t have) ELCv3 and a non-selected path doesn’t have (or has) it, it’s OK to remain silent. The router should propagate the selected route. If the selected route doesn’t have ELCv3, so be it. Is there some reason NOT to do it this way? It seems obvious, correct, and fully-specified.

In the messier case where there’s a multipath… ugh. Remind me, do we have something in our document set that standardizes how to form multipaths? A naive search in the IDR documents tab doesn’t turn anything up, and I don’t immediately recall a relevant doc, but that doesn’t mean there isn’t one. I suppose formally speaking, a multipath is an aggregate, so what you’re asking for is the definition of aggregation rules for NHC? I think maybe the rules can be specific on a per-capability basis, and for ELCv3 they could be "ELCv3 MUST NOT be included unless all members of the aggregate include it”. There would then need to be guidance reminding people who define new capabilities that they should think about this question.

> Sec 3.4
> 
> I believe multiple instances of ELCv3 would not be considered malformed?

Yes. I think this is fully covered in §2.1 already.

> Appendix A
> 
> The point that EL capability can be derived from configuration is already covered in sec 3.2. What would be the motivation to cover ELCv2 in this document? I would suggest removing Appendix A since the ELC attribute (28) is deprecated and this document asks that it be discarded in sec 4. The Appendix A seems to contradict that in some form by indicating that implementations leverage ELC attribute 28.

This is a case of trying to say something helpful (the fact is that ELCv2 is currently used in the wild) while trying very hard not to appear to give it some kind of official imprimatur. Yes, we can remove the appendix if that’s what the consensus is to do, although my own opinion is that we’ve made our point and there’s no further benefit from removing this one breadcrumb that may be helpful to implementors deploying into networks that do have remaining ELCv2 installations. 

If we were going to remove the appendix, I’d want to add one additional bullet to the list in §3.2:

	* Knows by implementation-specific means that the egress is EL-capable.

Or something like that. 

Thanks,

—J