Re: [Idr] Document shepherd review for draft-ietf-idr-rfc7752bis

["Ketan Talaulikar (ketant)" <[e-mail]>]

Hi Jeff,

Please check inline below for responses. Need your help and some clarification to close the few remaining points.

-----Original Message-----
From: Jeffrey Haas <[e-mail]> 
Sent: 10 November 2021 03:15
To: Ketan Talaulikar (ketant) <[e-mail]>
Cc: [e-mail]; [e-mail]
Subject: Re: Document shepherd review for draft-ietf-idr-rfc7752bis

Ketan,

Thanks for your continued patience as we iterate.

I've deleted prior text where I accept the changes you've done in -09.

There remain a small number of changes you've proposed to address my comments.  There is one somewhat larger point covering NLRI opacity that deserves Working Group scutiny.  That larger point may result in no changes in text.

On Thu, Oct 21, 2021 at 04:48:05PM +0000, Ketan Talaulikar (ketant) wrote:
> NLRI keying consistency:
[...]
> [KT] I agree with your point. How about we add the follow text towards 
> the end of Sec 4.1 for both this one and the next comment? We want to 
> cover this aspect but not really try to specify the behavior of a 
> BGP-LS Consumer as that is outside the scope of this document.
> 
> <NEW>
> When there are multiple BGP-LS Producers originating the same 
> link-state information, implementation variations of BGP-LS Producers 
> may result in the generation of different and inconsistent BGP-LS 
> updates for the same link-state object based on the inclusion or 
> exclusion of optional TLVs. An inconsistency between BGP-LS Producers 
> with regards to the inclusion of optional TLVs in the NLRI results in 
> multiple NLRIs being generated for the same link-state object. A 
> BGP-LS Consumer would need the ability to merge such duplicate updates 
> to handle such situations. An inconsistency between BGP-LS Producers 
> with regards to the inclusion of optional TLVs in the BGP-LS Attribute 
> results in one of them being delivered to a BGP-LS Consumer as part 
> the BGP propagation and best-path selection procedures in most typical 
> deployments. This can result in a BGP-LS Consumer missing out on some 
> of the information in a potentially unpredictable manner. The use of 
> BGP-LS Producers that have a consistent support for the origination of 
> optional TLVs between them can help mitigate such situations for the BGP-LS Consumers.
> </NEW>

This text was merged into -09 and I think addresses the issue reasonably.

A critical detail is you've added "A BGP-LS Consumer would need the ability to merge..." and we're not asking routers carrying this information to do the work.
[KT] Yes, that is correct. This distinction and boundary between the "BGP protocol module" and "BGP-LS Consumer module" is precisely why we've introduced these terminologies and clarifications in Sec 3. IIRC (it been a few years now), this was one of the key inputs that I got from Alvaro for this bis.

> The remainder of my comments have to do with Error Handling.  My 
> intent is not to re-litigate prior decisions, it's to note that the 
> consequences of those prior decisions are not adequately discussed in the document.

[...]

> The strict TLV structuring of BGP-LS makes the most types of syntactic 
> errors impossible.  However, BGP-LS describes a number of semantic 
> validations using RFC 2119/8174 keywords without adequately describing 
> what to do when those behaviors are violated at the BGP-LS Consumer.

> [KT] The specification of behavior and handling at a BGP-LS Consumer 
> was considered to be out of scope of this draft.

I think I'll add one final comment on this particular matter, then likely let it rest:

The issue we're discussing here is opacity of the attributes as routes traverse BGP implementations.  Flowspec, for example, has been injured by exactly this issue.

If an implementation, upon receiving PDUs that it didn't originate, simply checks the structure of the PDU for well-formedness and then largely stores the information in a datastore as opaque bit-strings, these things all work out okay.  

However, that's not how many real-world implementations behave.

A BGP implementation, that is not the BGP-LS Consumer, that receives and parses the PDU and locally stores its contents in data structures can have problems when things are "invalid" in some form.  Such implementations tend to regenerate the PDU from internal state.  This includes taking optional elements that aren't parsed, but stored opaquely and inserting them appropriately.
[KT] The clarification in Sec 7.2.2 requires all BGP speakers to perform syntactic validation. Only semantic validation is explicitly being excluded. So we are trying to get a fine balance here.

The implicit advice that is given is, "Regardless of what you do with this data, keep a full copy of it in original form as well".  This point deserves explicit acknowledgment by the Working Group if that's our effective intent.
[KT] That is not correct from syntactic validation perspective but correct from semantic validation perspective. That is why the "boundary" between a consumer and a speaker is important.

The similar advice is, "Be careful about being a BGP-LS implementation that serves as a distributing router of BGP-LS while also being a Consumer".
[KT] Ack. Can you please (re)check the text in Sec 3 if there is something there that we can improve (if not clear). Or even 7.2.2 if it is better reminded there?

If that's what we really mean, that's what we should say.  Unlike flowspec v1, we at least have strong TLV rules to ensure it's well-formed before propagating.

> "Make the same mistakes everywhere" is not a great recipe.
> 
> I strongly recommend that this issue be addressed either in section 7.2.2 or in a new section.
> [KT] Give, the previous comment, I am open to inputs on what can be incorporated into the document in this respect.

Let's find a place to add text warning about the above.
[KT] Could you please help me with the text here?

> Some comments on the draft text are noted below.  This is from the idnits output.
> 
> 407	4.1.  TLV Format
> [...]
> 435	   type MUST be in ascending order based on the value field.  Comparison
> 436	   of the value fields is performed by treating the entire field as an
> 437	   opaque hexadecimal string.  Standard string comparison rules apply.
> 
> I suspect what is intended here is opaque binary data rather than 
> representing it in hexadecimal format.  "Standard string comparison" 
> might make sense if you were doing strcmp() on an actual hexadecimal string.
> The intent here is probably lexicographically ordered since "standard 
> string comparison" isn't really defined here.
>
> [KT] Yes, I believe so. How about:
> 
> <OLD>
> Comparison of the value fields is performed by treating the entire field as an opaque hexadecimal string.  Standard string comparison rules apply. 
> </OLD>
> <NEW>
> Comparison of the value fields is performed by treating the entire field as opaque binary data and ordered lexicographically.
> </NEW>

That would be good.  (This didn't make it into -09.)
[KT] Ack - will fix in v10.

> 447	   The TLVs within the BGP-LS Attribute MAY be ordered in ascending
> 448	   order by TLV type.  BGP-LS Attribute with unordered TLVs MUST NOT be
> 449	   considered malformed.
> 
> Probably "SHOULD be ordered"?  If it's MAY be ordered and MUST NOT be malformed, why mention it at all?
> [KT] I am happy to drop both the sentences. The current text is more to clarify given the ambiguity in this regards in RFC7752.

I'd suggest leaving it and update to SHOULD.  Reinforcing that unordered isn't malformed will likely save us implementations that didn't deal with that.
[KT] Just to be confirm - you are saying  "SHOULD be ordered; if not then MUST NOT be considered malformed" - if so, that sounds good to me.

> 525	                            Table 1: NLRI Types
> 
> Note that the contents of this table do not match the IANA Considerations section.
> [KT] I think you are referring to 0 and the Private Use? Do they need to match since the IANA is more about allocation and this is about listing what is being introduced in the document.

Ok, that seems reasonable.

> 753	   Autonomous System:  Opaque value (32-bit AS Number).  This is an
> 754	      optional TLV.  The value SHOULD be set to the AS Number associated
> 755	      with the BGP process originating the link-state information.  An
> 756	      implementation MAY provide a configuration option on the BGP-LS
> 757	      Producer to use a different value.
> 
> Some text discussing private AS numbers would be helpful.  The item of concern is when BGP-LS is used between cooperating ASes not under a single administrative control.  In such cases, avoiding private AS collisions is an edge case.
> [KT] I am not sure if I've understood your point correctly, but how about add the following at the end of the current text. Please clarify or suggest text if you meant something different.
> 
> E.g., to avoid collisions when using private AS numbers.

That'd work.  I'd suggest:
"to use a different value; e.g., to avoid..."
[KT] Ack.

> 988	   where the Type and Length fields of the TLV are defined in Table 5.
> 989	   The OSPF Route Type field values are defined in the OSPF protocol and
> 990	   can be one of the following:
> 
> 992	   o  Intra-Area (0x1)
> 993	   o  Inter-Area (0x2)
> 
> 995	   o  External 1 (0x3)
> 
> 997	   o  External 2 (0x4)
> 
> 999	   o  NSSA 1 (0x5)
> 
> 1001	   o  NSSA 2 (0x6)
> 
> The semantics of these are from OSPF, but what's the matching RFC section reference for the code points?
> [KT] These are actually BGP-LS code points. We don't anticipate new route types in OSPF (been so for decades) and that is why I skipped creating an IANA registry for them. But we can if that is the right thing to do.

I understand now that these are assigned by BGP-LS.  I'm fine that they may not have an IANA considerations section.  The confusing point is the sentence, "... are defined in the OSPF protocol".  

I immediately went to cross-check these vs. OSPF specifications and IANA.  

It'd be helpful to clarify that these are features from the OSPF protocol but that the code points are not defined within OSPF - or at least what OSPF code point feature they're intended to cover.
> 
[KT] How about the following?

s/ The OSPF Route Type field values are defined in the OSPF protocol and can be one of the following: / The OSPF Route Type field follows the route types defined in the OSPF protocol and can be one of the following:

Thanks,
Ketan

-- Jeff