Subject: Re: [Pce] AD review of draft-ietf-pce-binding-label-sid-10

From: "Chengli (Cheng Li)" <[email-masked]>
Date: Fri, 15 Oct 2021 09:09:51 +0000

Hi John,

Sorry for my delay due to the PTO, have updated the draft to address your comments, please take a look of if it works.

Thanks,
Cheng

URL:            https://www.ietf.org/archive/id/draft-ietf-pce-binding-label-sid-11.txt
Status:         https://datatracker.ietf.org/doc/draft-ietf-pce-binding-label-sid/
Htmlized:       https://datatracker.ietf.org/doc/html/draft-ietf-pce-binding-label-sid
Diff:           https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-binding-label-sid-11

-----Original Message-----
From: Pce [mailto:[email-masked]] On Behalf Of John Scudder
Sent: Friday, October 1, 2021 4:14 AM
To: Dhruv Dhody <[email-masked]>
Cc: [email-masked]; [email-masked]; pce-chairs <[email-masked]>
Subject: Re: [Pce] AD review of draft-ietf-pce-binding-label-sid-10

Hi Dhruv,

Thanks for your comments, and some replies in line:

> On Sep 29, 2021, at 4:49 AM, Dhruv Dhody <[email-masked]> wrote:

(…snip...)

> ***************
> *** 490,495 ****
> --- 512,546 ----
>      error, the PCC rejects the PCUpd or PCInitiate message in its
>      entirety and can include the offending TE-PATH-BINDING TLV in the
>      PCEP-ERROR object.
> +    
> + Rejecting the message in its entirety makes sense, but it does imply 
> + that an implementation MUST be prepared to unwind any actions it has 
> + taken earlier on the basis of the message. It's a pretty common 
> + pattern for an implementation to take individual actions implied by 
> + a message immediately as they're encountered, which means this 
> + unwinding is potentially non-trivial. For example, suppose a PCE 
> + requires a PCC to allocate three specific binding values, so it 
> + sends a PCUpd containing three TE-PATH-BINDING TLVs. The first one 
> + is successfully allocated, and so is the second, but for some reason 
> + the third is unable to be allocated. Now the PCC must de-allocate 
> + the first two, in addition to sending a TBD2+TBD4 PCErr message and 
> + possibly includes the third TLV in the PCEP-ERROR object.
> + 
> + Also, I guess the PCE must infer that the first two bindings were 
> + not allocated, even though it has only received an error for the third.
> + This would be true even if the PCC implementation doesn't have to do 
> + any de-allocation -- suppose the PCC has a clever implementation 
> + that only commits the actions in the PCUpd once it knows they will 
> + all succeed, it's still the case that the PCE will get an error for 
> + one but must know that the error applies to the others that were 
> + carried in the PCUpd.
> + 
> + Is this analysis correct, or if not, why not? If it's correct, it 
> + seems to me as though it might well be worth devoting a few 
> + sentences to it, unless these issues are already well covered in one 
> + of the foundational RFCs (in which case, I'd appreciate a pointer, I 
> + haven't done a thorough enough review of the normative documents to 
> + catch all such things).
> 
> 
> [Dhruv]: It is a normal practice to reject the whole message in PCEP. Say a PCE sends a PCUpd message for an LSP with multiple updated parameters, one would apply the update only once it has verified all the parameters and send an error in case any are found to be invalid. The PCErr message includes an SRP object that identifies the full PCUpd message is being rejected with the reason for the rejection optionally identified. 
> 
> This was discussed during the WGLC and the conclusion was to reject the whole message in the case of multiple binding TLVs as well. The SRP in PCErr would identify that the complete message is being rejected here as well. 

As long as this is a well-known pattern in the PCE world, that this spec is just continuing, I’m fine with letting the text stand without modification.

>  
>      If a PCE wishes to request the withdrawal of a previously reported
>      binding value, it MUST send a PCUpd message with the specific TE-
> ***************
> *** 507,513 ****
> --- 558,573 ----
> 
> 
>      In some cases, a stateful PCE can request the PCC to allocate any
> +                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      binding value.  It instructs the PCC by sending a PCUpd message
> +    ^^^^^^^^^^^^^
> +    I don't understand what that means. My *guess* is that you mean
> +    the PCE is just saying "hey PCC, please allocate a binding value
> +    and tell me what it is".  Is that right?  If so I think this 
> +    could be worded even more clearly, perhaps "a stateful PCE may
> +    want to request that the PCE allocate a binding value of the PCE's 
> +    own choosing"?
> +    
> 
> [Dhruv]: It should be PCC's own choosing.

Were you just correcting my comment (to say I should have written “PCC” the latter two times, I agree, my mistake), or was your point also to say that yes, my guess is correct? If the latter, then as I mentioned, I’d appreciate having the wording improved.

> <snip>
> ***************
> *** 655,660 ****
> --- 742,758 ----
>         *  Send a PCErr message with Error-Type=19 (Invalid Operation) and
>            Error-Value=16 (Attempted PCECC operations when PCECC
>            capability was not advertised)
> +          
> + You need to update your reference to point to RFC 9050. You should 
> + then either reference §5.4 and NOT copy-and-paste the relevant text, 
> + or you should (less desirably, IMO) update your copy-and-paste to 
> + the text published with RFC 9050.  Notably, RFC 9050 covers the case 
> + of a legacy PCEP speaker, whereas the text in this spec doesn't.
> + 
> + IMO since RFC 9050 already specifies what to do when the capability 
> + isn't exchanged, there is no need to say anything at all in this 
> + document, so unless you have a compelling reason (what?) to keep it, 
> + please remove this bullet and its two sub-bullets.
> 
>         *  Terminate the PCEP session
> 
> 
> 
> [Dhruv]: The only reason to include this text is for "P=1 in the LSP object" i.e. the presence of the LSP object with P flag set is to be considered as a PCECC operation, which would not be the case in RFC9050. 

I see, thanks. In that case, then I guess the copy-paste needs to be updated to follow the text in 9050, or if that doesn’t work for some reason I’ve overlooked, in any case the text needs to be updated to cover the case of a legacy speaker. 

> ***************
> *** 745,754 ****
>      [RFC8281] and [RFC8664] are applicable to this specification.  No
>      additional security measure is required.
> 
> !    As described [RFC8664], SR allows a network controller to instantiate
>      and control paths in the network.  A rogue PCE can manipulate binding
>      SID allocations to move traffic around for some other LSP that uses
>      BSID in its SR-ERO.
> 
>      Thus, as per [RFC8231], it is RECOMMENDED that these PCEP extensions
>      only be activated on authenticated and encrypted sessions across 
> PCEs
> --- 843,857 ----
>      [RFC8281] and [RFC8664] are applicable to this specification.  No
>      additional security measure is required.
> 
> !    As described in [RFC8664], SR allows a network controller to instantiate
>      and control paths in the network.  A rogue PCE can manipulate binding
>      SID allocations to move traffic around for some other LSP that uses
>      BSID in its SR-ERO.
> +    
> + Try as I might, I'm not able to figure out what, specifically, the 
> + above sentence (that begins "a rogue PCE") means.  I mean, I get it, 
> + a rogue PCE can misdirect traffic.  Beyond that, can you help me 
> + understand what you meant to convey here?
> 
> [Dhruv]: IMHO it highlights how this new capability of "allocating binding SID" can be exploited by a rogue PCE to misdirect traffic. Path {A, B, BSID_1}  can be misdirected just by assigning the BSID_1 value to a different LSP making it a lot easier (and harder to detect).  

I think it would be helpful to add something concrete like that.

>      Thus, as per [RFC8231], it is RECOMMENDED that these PCEP extensions
>      only be activated on authenticated and encrypted sessions across 
> PCEs
> ***************
> *** 773,778 ****
> --- 876,889 ----
> 
>      The PCEP YANG module [I-D.ietf-pce-pcep-yang] could be extended to
>      include policy configuration for binding label/SID allocation.
> +    
> + It looks like pcep-yang expired a couple months ago, but I think 
> + that's just an oversight and doesn't reflect the WG abandoning the 
> + work.  Is there any intention on the part of the WG, to extend it as 
> + described?  My understanding is that yes, the WG does intend to do 
> + this -- in that case I think this section would be stronger if you 
> + indicated that, something like "The PCEP YANG module will be 
> + extended..." instead of "could be".
> 
> 
> 
> [Dhruv]: This one is my fault. It is on my to-do list to update.

OK, thanks. If the WG has consensus that the YANG module will be extended as described, I think making the suggested change might help avoid questions during IESG review.

> ***************
> *** 809,814 ****
> --- 920,928 ----
>      apply to the PCEP extensions defined in this document.  Further, the
>      mechanism described in this document can help the operator to request
>      control of the LSPs at a particular PCE.
> +    
> + I don't understand the final sentence (the one that begins "further"). 
> + Can you help me?
> [Dhruv]: My guess would be it is leftover text -- needs to be removed.

OK.

>   12.  IANA Considerations
> 
> ***************
> *** 852,857 ****
> --- 966,976 ----
>                            Behavior and
>                            Structure
>                   4-255    Unassigned            This document
> +                 
> + Just curious, have you considered any Experimental Use values?  The 
> + PCEP registries are inconsistent in that regard so I can't tell 
> + whether this is a "you should" or "you could" or even something that 
> + doesn't make sense in this specific case.
> 
> [Dhruv]: The experimantal values are kept limited (see https://www.rfc-editor.org/rfc/rfc8356.html#appendix-A). In the case of new BT, there needs to be agreemeent accross WGs, not sure if it is particularly helpful to have experimental codepoint in this case! 

Thanks for the reference. Makes sense.

—John
_______________________________________________
Pce mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/pce