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

From: Jeffrey Haas <[email-masked]>
Date: Tue, 9 Nov 2021 16:45:01 -0500

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.

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

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.

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

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.

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

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

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

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

-- Jeff

---

From: "Ketan Talaulikar (ketant)" <[email-masked]>
Date: Wed, 10 Nov 2021 08:56:00 +0000

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 <[email-masked]> 
Sent: 10 November 2021 03:15
To: Ketan Talaulikar (ketant) <[email-masked]>
Cc: [email-masked]; [email-masked]
Subject: Re: Document shepherd review for draft-ietf-idr-rfc7752bis

Ketan,

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.

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

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.

[...]

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

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.

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

> representing it in hexadecimal format.  "Standard string comparison" 
> might make sense if you were doing strcmp() on an actual hexadecimal string.

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

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

[KT] Ack.

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

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

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

---

From: Jeffrey Haas <[email-masked]>
Date: Wed, 10 Nov 2021 06:21:46 -0500

Ketan,

> [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 S…

I'll spend some time hopefully today to try to re-read sections 3 and 7.2.

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

That's the suggestion.

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

That would clarify the point.

-- Jeff

---

From: "Ketan Talaulikar (ketant)" <[email-masked]>
Date: Wed, 10 Nov 2021 12:13:03 +0000

Hi Jeff,

I've incorporated the 2 updates that you've ack-ed into the edit buffer. I will wait for your advice/text for the one last point that remains before posting the v10 update.

Thanks,
Ketan

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

Ketan,

That's the suggestion.

-- Jeff

---

From: Jeffrey Haas <[email-masked]>
Date: Wed, 10 Nov 2021 11:42:34 -0500

Ketan,

> I'll spend some time hopefully today to try to re-read sections 3 and 7.2.

I took some time to deeply contemplate those sections.  In particular:

Section 3:
:    The above roles are not mutually exclusive.  The same BGP Speaker may
:    be the producer for some link-state information and propagator for
:    some other link-state information while also providing this
:    information to a consumer application.  Nothing precludes a BGP
:    implementation performing some of the validation and processing on
:    behalf of the BGP-LS Consumer as long as it does not impact the
:    semantics of its role as BGP-LS Propagator as described in this
:    document.

Section 7.2.2:
:    A BGP-LS Propagator SHOULD NOT perform semantic validation of the
:    Link-State NLRI or the BGP-LS Attribute to determine if it is
:    malformed or invalid.

The circumstance we're effectively discussing is BGP Speakers that are
operating in the context of either a mixed Consumer/Propagator or at least
an implementation that "some of the validation" above.

The CBOR RFC provides a nice phrase that covers some of the classes of
errors we're discussing here for BGP-LS, and also for other current IDR
proposals: "Well-formed, but invalid".

BGP-LS puts us in competing philosophies for the link-state we're carrying
in BGP:
- In the IGP, well formed trash gets happily carried.
- In BGP, well formed trash has been responsible for routing outages.

As John Scudder has pointed out over the years, "SHOULD NOT" in some
circumstances can mean MAY.  So, we have the wiggle room for implementations
that wish to not propagate "well-formed, but invalid" if they choose not to
do so.

Given this wiggle room, would the following small change be acceptable?

"A BGP-LS Propagator, even when also operating as a BGP-LS Consumer, SHOULD
 NOT perform semantic validation of the Link-State NLRI or the BGP-LS
 Attribute to determine if it is malformed or invalid."

The motivation for this update is more signaling to an implementor of BGP-LS
that in cases where validation happens that propagation is expected.  If you
can't reduce the BGP-LS state to internal data structures, you had best be
prepared to carry it as opaque binary data.

It's my personal belief that multiple implementations will simply treat
well-formed but invalid as reason to use RFC 7606 treat-as-withdraw.  But
perhaps that will change with time.

-- Jeff

---

From: "Ketan Talaulikar (ketant)" <[email-masked]>
Date: Wed, 10 Nov 2021 17:01:08 +0000

Hi Jeff,

Thanks for your suggestion and it looks good to me.

I also agree on your "wiggle room" comment with respect to "SHOULD NOT" vs "MAY". IMHO given that this is a bis, we are perhaps right in stopping at SHOULD NOT instead of MUST NOT. However, open to feedback.

I will post the v10 later today with the updates discussed.

Thanks,
Ketan

-----Original Message-----
From: Jeffrey Haas <[email-masked]> 
Sent: 10 November 2021 22:13
To: Ketan Talaulikar (ketant) <[email-masked]>
Cc: [email-masked]; [email-masked]
Subject: Re: [Idr] Document shepherd review for draft-ietf-idr-rfc7752bis

Ketan,

Section 3:

Section 7.2.2:

The circumstance we're effectively discussing is BGP Speakers that are operating in the context of either a mixed Consumer/Propagator or at least an implementation that "some of the validation" above.

The CBOR RFC provides a nice phrase that covers some of the classes of errors we're discussing here for BGP-LS, and also for other current IDR
proposals: "Well-formed, but invalid".

BGP-LS puts us in competing philosophies for the link-state we're carrying in BGP:
- In the IGP, well formed trash gets happily carried.
- In BGP, well formed trash has been responsible for routing outages.

As John Scudder has pointed out over the years, "SHOULD NOT" in some circumstances can mean MAY.  So, we have the wiggle room for implementations that wish to not propagate "well-formed, but invalid" if they choose not to do so.

"A BGP-LS Propagator, even when also operating as a BGP-LS Consumer, SHOULD  NOT perform semantic validation of the Link-State NLRI or the BGP-LS  Attribute to determine if it is malformed or invalid."

The motivation for this update is more signaling to an implementor of BGP-LS that in cases where validation happens that propagation is expected.  If you can't reduce the BGP-LS state to internal data structures, you had best be prepared to carry it as opaque binary data.

It's my personal belief that multiple implementations will simply treat well-formed but invalid as reason to use RFC 7606 treat-as-withdraw.  But perhaps that will change with time.

-- Jeff