Re: [Tsv-art] [Pce] TSV-ART review of draft-ietf-pce-stateful-sync-optimizations

[Dhruv Dhody <[e-mail]>]

Hi Adrian, 

Thanks for your review and comments. I have provided explanation in the mail for the broader points, as well as made changes in the working copy. Please let me know if I have missed handling any comment, and better yet, if you have text suggestions :). 

Please see inline. 

> -----Original Message-----
> From: Pce [mailto:[e-mail]] On Behalf Of Adrian Farrel
> Sent: 24 February 2017 23:12
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]; draft-ietf-pce-
> [e-mail]
> Subject: [Pce] TSV-ART review of draft-ietf-pce-stateful-sync-
> optimizations
> 
> Document: draft-ietf-pce-stateful-sync-optimizations
> Reviewer: Adrian Farrel
> Review Date: February 21, 2017
> IETF LC End Date: February 28, 2017
> IESG Telechat date: March 16, 2017
> 
> Review result: Has Issues
> 
> I've reviewed this document as part of TSV-ART's ongoing effort to review
> key IETF documents. These comments were written primarily for the
> transport area directors, but are copied to the document's authors and the
> working group for information and to allow any issues raised to be
> addressed.
> 
> Please always CC tsv-art at ietf.org if you reply to or forward this
> review.
> 
> This draft presents motivations for optimizations to the base state
> synchronization procedures used by a Path Computation Element (PCE) to
> ensure its state matches that held in the network, that held by Path
> Computation Clients (PCCs), and that held by cooperating PCEs. Those
> procedures are described in draft-ietf-pce-stateful-pce also in IETF last
> call. The draft then defines extensions to the Path Computation Element
> Communication Protocol (PCEP) to achieve these optimizations.
> 
> In the spirit of full disclosure, I have been an active member of the PCE
> working group and participated in the work that resulted in draft-ietf-
> pce-stateful-pce, but I have not previously paid attention to this
> document.
> 
> -- TSV-ART review comments:
> 
> PCEP is a TCP-based protocol. A separate piece of work (draft-ietf-pce-
> pceps) examines how to secure PCEP using secure transport.
> 
> This document makes no changes to how PCEP uses the transport. However, it
> does discuss (section 3.3.2) re-establishing peering after one peer moves
> and changes its "transport-level identity (such as IP address)". This is
> done by establishing a new TCP connection and referencing the old PCEP
> session as the new one is established (i.e., all issues - including this
> rather obvious attack
> vector) are in PCEP and do not affect transport.
> 
> -- Other review comments
> 
> == Major issues
> 
> I found it quite hard to work out the objects and flags to include on an
> Open. The problem (for me) is that there are three levels of function a.
> no support for this I-D b. support for synchronisation avoidance if DBvs
> match with full resynch
>    otherwise
> c. support for synchronisation avoidance if DBvs match with partial
>    resynch otherwise
> and three indicators:
> - S flag on Open
> - D flag on Open
> - presence of LSP-DB-VERSION TLV on Open
> 
> S flag seems to serve two purposes:
> - indicates that LSP-DB-VERSION TLV must be used on PCRpt messages
> - indicates that resynch avoidance can be attempted
> 
> D flag seems to mean:
> - Partial (incremental) synchronisation can be attempted
> 
> Presence of LSP-DB-VERSION TLV on Open seems to indicate:
> - value of DBv to compare to see if resynch can be avoided
> - value of DBv to compare to perform partial resynch
> 
> I can't find a description of the behavior at startup with no saved DB.
> I think it is to omit the LSP-DB-VERSION TLV but still set the S flag.
> Is the only reason you need the S flag that you don't support the value of
> zero for a DBv? That is, you could use the presence of the LSP-DB-VERSION
> TLV as indication that synchronisation avoidance is supported if you had a
> value you could transmit to indicate "I have no DB".
> 
> I am missing a statement in Section 3 that makes it clear that in the
> absence of the LSP-SYNC-CAPABILITY (D bit) from section 4, all synchs are
> either NOP or full.
> 
> Finally, it is not clear (to me) why the PCC needs to include an LSP-DB-
> VERSION TLV in its Open message. I can see that it helps the PCE to
> predict what will happen next, but it isn't actually a necessary part of
> the protocol AFAICS.
> 

[[Dhruv Dhody]] This document resulted from merging of sync avoidance from the stateful-pce draft and others from an individual draft. The idea was to keep all these procedures together (in one document), but separate so that an implementation/deployment is free to pick and choose the various optimizations based on the need. The structure of the document reflect that, it does have the side effect as you noted.  

Let me try to state the purpose of each flag and the TLV in open message -  

- LSP-DB-VERSION TLV: The last LSP State Database Version Number on this session (useful only for re-establishment of PCEP session, ex session flap) 
- S (INCLUDE-DB-VERSION):  indicates that LSP-DB-VERSION TLV must be used on the *to be sent* PCRpt messages on this session (useful in future re-establishment when current session goes down)
- D (DELTA-LSP-SYNC-CAPABILITY): Allow incremental state sync during the state synchronization phase *now* (useful during re-establishment)
- T (TRIGGERED-RESYNC): Allowed to trigger re-sync by PCE in *future* on this session 
- F (TRIGGERED-INITIAL-SYNC): Wait for PCE to trigger state sync procedure *now*

>From your comments I get that, the S flag was the reason for confusion. 
The S flag does not play any role during the current state sync, it facilitates future state sync. This is shown in figure 3, where even though the S flag is unset but state synchronization is skipped anyway. 

I agree that the text in section 3.2 does not make this fact clear, I have updated that in the working copy.  

To further clarify, the presence of LSP-DB-VERSION TLV indicate that this is a case of re-establishment of PCEP session and the peer remembers the LSP-state. 
If the version match, state sync can be skipped.
If version don't match and D flag is set, we do Incremental sync; otherwise full sync.  

Please have a look at the working copy and if further changes are needed. 
   
> ---
> 
> The function introduced for 3.3.2 has far reaching implications for a PCE
> system. I wonder whether the WG noticed it and considered those
> implications.
> 
> Up until now a PCEP session has been known in the context of the PCE/PCC
> address pair. You are changing this by introducing a SPEAKER-ENTITY-ID
> that represents the PCEP speaker for all time.
> You are not saying anything about how the assignment of this value is made
> consistent across the domain (indeed, you seem to offer a range of
> implementation choices that could clash), or how uniqueness might be
> policed. Furthermore, you only give a "SHOULD" as the mechanism to
> identify interdomain PCEs, and that doesn't seem to provide a reliable
> mechanism (actually, even the mechanism for this "SHOULD" is a bit vague).
> 
> You are saying that one of the PCE/PCC can change its address and re-
> assume a PCEP session without any further impact.
> 
> Now, if the PCC changes its address there will be "interesting"
> consequences for the LSPs for which it is the head end. Renumbering events
> under the feet of live TE-LSPs are not well examined, but it looks like
> the LSPs might cease to be.
> 
> So, perhaps you are looking at what happens when a PCE changes its address.
> This *might* be a more likely event if the PCE is virtualised and moves
> around in a DC.
> 
> But, how does a PCC/PCE pairing get discovered and authorised? If you are
> relying on discovery through the routing protocol, then you will need to
> add the SPEAKER-ENTITY-ID to the advertisement or else it is hit or miss
> for the PCC to find the right PCE. If you are relying on configuration,
> then I suspect that the SPEAKER-ENTITY-ID is not needed because the
> configuration actions can achieve the right effect.
> 
> Perhaps the use case you have in mind is when:
> - PCEP sessions are initiated by the PCE
> - The PCC is configured to accept any PCE
> - The PCE might wander taking its DB with it This has a some utility, but
> a lot of security concerns and error cases.
> What if a new PCE comes alive using a SPEAKER-ENTITY-ID that is already in
> use? What if a bogus PCE claims to have the same SPEAKER-ENTITY-ID as a
> PCE that has just failed? (And, of course, what stops a bogus PCE from
> randomly latching on to PCCs in the network).
> 
> Maybe your plan is that the PCC will be configured with acceptable
> SPEAKER-ENTITY-IDs?
> 
> You might also have had some ideas about PCE redundancy here. For example,
> a backup PCE with access to the DB used by the primary PCE could want to
> step in and take over seamlessly. In this case, of course, you would have
> two PCEs with the same SPEAKER-ENTITY-ID.
> 
> In short, I don't think you have described this use case and its
> consequences, and you are trying to optimise a terribly small corner case.
> 
> (Actually, this would all have been a lot easier if I had known about
> D=0 all the way through section 3, but that is just a comment about how
> hard the document is to parse for a slow learner like me.)
> 

[[Dhruv Dhody]] The origin of this idea is from PREDUNDANCY-GROUP-ID TLV in https://tools.ietf.org/html/draft-ietf-pce-stateful-pce-03#section-7.1.3
When we moved this to this draft, we made it generic as SPEAKER-ENTITY-ID to allow two cases - 

- In case of PCC, TCP client can choose any IP address to connect to the PCE. And in case a physical interface IP address is used and for example because of line card change, the PCEP session might go down and come back up again but with a different IP address used by the TCP client. We wanted a way to identify the same speaker even if this IP address has changed. 
- PCE redundancy as you describe.    

The key use-case for us is to allow us to skip state sync if only IP address has changed and limited to within the State Timeout Interval, till we have the state data to associate with the same session. 

I have added a separate section in the working copy to drive this point. 

An error-code is added when there is a clash of this ID. 

Please have a look at the working copy and see if further details should be added. 

> === Minor issues
> 
> The phrase "reliable state synchronization mechanism" is interesting as it
> is not used in draft-ietf-pce-stateful-pce-18.txt where synchronization is
> described only in terms of "checkpointing state".
> Is any implication to be drawn from this use of "reliable"?
> 
[[Dhruv Dhody]] Removed "reliable". 
> ---
> 
> In Section 2 you have...
> 
>    Within this document, when describing PCE-PCE communications, the
>    requesting PCE fills the role of a PCC.  This provides a saving in
>    documentation without loss of function.
> 
> I know what you intend, but this is a little confusing in the case that of
> "PCE-triggered Initial Synchronization" when (of course) the requesting
> PCE fills the role of a PCE and the responding PCE fills the role of PCC.
> 
> Maybe you can write:
> 
>    Within this document, when describing PCE-PCE communications, one of
>    the PCEs fills the role of a PCC.  This provides a saving in
>    documentation without loss of generality.
> 
[[Dhruv Dhody]] Ack. 
> ---
> 
> 3.2
> 
>    "PCE speaker receiving this node should send back a PCErr
>    s/node/value/
>    s/should/SHOULD/
>    But why "SHOULD" not "MUST"? What is the optional alternative?
> 
[[Dhruv Dhody]] Ack. Changed to "MUST".
> ---
> 
> 3.2
> 
>    If state synchronization avoidance has not been enabled on
>    a PCEP session, the PCC SHOULD NOT include the LSP-DB-VERSION TLV in
>    the LSP Object and the PCE SHOULD ignore it were it to receive one.
> 
> Why "SHOULD ignore" and not "MUST ignore"? What else might a PCE do with
> the information?
> 
[[Dhruv Dhody]] Ack. Changed to "MUST".
> ---
> 
> Because of wrapping you will get situations where PCE(DBv) > PCC(DBv) yet
> the PCC has more recent state.  This is normal and I think you just have
> to say that the assumption is *always* that the PCC has more recent state
> and that updates must be done from the value of PCE(DBv) incrementally and
> potentially through the wrap until the value of the
> PCC(DBv) is reached.
> 
[[Dhruv Dhody]] Updated. 
> ---
> 
> 3.3.1 has
> 
>    The LSP State Database Version Number (LSP-DB-VERSION) TLV is an
>    optional TLV that MAY be included in the OPEN object and the LSP
>    object.
> 
> The LSP object is allowed on the PCRpt, the PCUpd, the PCReq, and the
> PCRep message. What does the LSP-DB-VERSION TLV mean on a PCUpd, a PCReq,
> or a PCRep?
> 
[[Dhruv Dhody]] Updated. 
> ---
> 
> It would help IANA and the RFC editor, and avoid any issues if you used
> different TBD flags (such as TBD1, TBD2,...) for the different cases of
> code point assignments to be made.
> 
[[Dhruv Dhody]] Ack.

> Why do you feel the need to recommend values for assignment in the IANA
> considerations section?
> 
[[Dhruv Dhody]] There are multiple Stateful PCE documents (some with early code point allocation), we wanted to make sure the alignment between those assignments and existing implementation. The IANA review has noted these suggestions. 
> ---
> 
> In 3.3.1 it would probably be wise to state how the 64 bit number is
> represented across the 8 octets.
> 
[[Dhruv Dhody]] Added Network Byte order. 
> ---
> 
> 4.2
>    The PCC MAY force a full LSP DB
>    synchronization by setting the D flag to zero in the OPEN message.
> Do you mean "The PCE"?
> The PCC is in control of the synchronization transmissions and doesn't
> need to force anything with the D flag. Maybe the PCC can "announce"
> full synchronization.
> 
[[Dhruv Dhody]] Made it generic - 
By setting the D flag to zero in the OPEN message, a PCEP speaker can skip the incremental synchronization optimization, resulting in a full LSP DB synchronization
> ---
> 
> 4.2
>    It is not necessary for a PCC to store a complete history of LSP
>    Database change, but rather remember the LSP state changes (including
>    LSP modification, setup and deletion) that happened between the PCEP
>    session(s) restart in order to carry out incremental state
>    synchronization.
> 
> Nice, but...
> Consider the PCRpt messages that were in flight when the PCE or the
> session went down.  Did they get included in the PCE's DB? That doesn't
> matter because the PCE will announce it via the DBv in the new Open. But
> does the PCC need to have remembered them? That depends on whether it
> thinks they arrived at the PCE and were processed. A situation made worse
> by the inclusion of multiple LSPs on one PCRpt message which could give
> rise to some being added to the DB and some lost.
> 
> Similarly...
> 
>    After the synchronization procedure finishes, the
>    PCC can dump this history information.
> 
> How does the PCC know that this state has made it into the PCE's DB?
> 
> Maybe you intend to get around this with...
> 
>    If a PCC finds out it does not have sufficient information to
>    complete incremental synchronisation after advertising incremental
>    LSP state synchronization capability, it MUST send a PCErr with
>    Error-Type 20 and Error-Value 5 'A PCC indicates to a PCE that it can
>    not complete the state synchronization' (defined in
>    [I-D.ietf-pce-stateful-pce]) and terminate the session.  The PCC
>    SHOULD re-establish the session with the D bit set to 0 in the OPEN
>    message.
> 
> This seems extreme, since the PCC could just do full resynch anyway.
> 

[[Dhruv Dhody]] You are right, we do not have an explicit ack from PCE, and thus we have to find a way around. 
Each LSP at the PCC stores the DBv at the time this LSP was reported. So based on the PCE's DBv we could find the LSP that needs to be reported. This works fine for new LSP, LSP with state change etc. The problem would be when the LSP are explicitly deleted at the PCC and PCC would like to delete it from its database. In this case we need to store this information at PCC so that we could report this deleted LSP as part of incremental state synchronization. We kept things generic in the document, thinking this would be implementation details.
 
I have added this to the working copy. 

Regarding the use of PCE triggered re-sync instead of session termination, we were worried that technically this is not re-synchronization, this is the first sync of the session and considered it wise to avoid it here. 

> ---
> 
> In 5.2
> 
> Why do you need the 'Attempt to trigger synchronization when the
> TRIGGERED-SYNC capability has not been advertised' error code? Surely it
> is enough that the PCE will have received F=0. Recall that the Open
> messages can overlap. Tearing down the session just to change the setting
> of the F bit is unnecessary.
> 
> Then
> 
>    The PCC SHOULD NOT send PCRpt messages to the stateful PCE before it
>    triggers the State Synchronization.
> 
> Please tell us about the circumstances under which it MAY send PCRpt and
> how, given the PCE has to be able to handle those, it is helpful
> to have a "SHOULD NOT" here.
> 
> And to be clear, you are saying that all updates to the DB, including all
> new delegations must be deferred and held as though the PCE was not in the
> session.
> 
> But what happens if the PCE sends a PCUpd? Does the PCC decline to respond?
> Or does it respond but leave out the LSP-DB-VERSION? Or do we apply
> section 5.6 of draft-ietf-pce-stateful-pce
>    A PCE SHOULD NOT send PCUpd messages to a PCC before State
>    Synchronization is complete.  A PCC SHOULD NOT send PCReq messages to
>    a PCE before State Synchronization is complete.  This is to allow the
>    PCE to get the best possible view of the network before it starts
>    computing new paths.
> In which case I wonder why the PCE even opened the session since nothing
> can be done on the session until the PCE is ready to synch.
> 

[[Dhruv Dhody]] The error code "Attempt to trigger..." is not in response to Open message, it is in response to receiving an PCUpd message with SYNC flag set when the capability in the open message is not advertised and agreed upon, PCC does not close the session in this case.  

I have further updated this section and added an explicit error-code. 

> ---
> 
> 6.2
>    To trigger re-synchronization for an LSP, the PCE MUST first mark the
>    LSP as stale and then send a Path Computation State Update (PCUpd)
>    for it, with the SYNC flag in the LSP object set to 1.
> 
> I think "mark the LSP as stale" is a local implementation thing. It
> doesn't go on the wire, does it? So probably no need to tell us about it.
> The subsequent PCRpt will cause an update to the DB anyway.
> 
[[Dhruv Dhody]] Updated. 
And also added - If the PCC cannot find the LSP in its database, PCC MUST also set the R (remove) flag in the LSP object in the PCRpt message.
> ---
> 
> 6.2
> 
>    The PCUpd message MUST include an empty ERO as its
>    intended path
> 
> I guess you have this because draft-ietf-pce-stateful-pce says an ERO is
> mandatory in a PCUpd. That's a bit heavy, and you could choose to say the
> ERO is ignored on a PCUpd if the SYNC flag is set. But anyway can you
> clarify that an empty ERO is:
> 1. sent with object length 4
> 2. the empty ERO is not an indication to the PCC that it can perform
>    local path computation and resignal the LSP according to its own
>    whim (i.e., even the empty ERO has to be ignored)
> 

[[Dhruv Dhody]] Ack. This should be added in stateful pce draft too


> ---
> 6.2
>    If the TRIGGERED-RESYNC capability is not advertised by a PCE and the
>    PCC receives a PCUpd with the SYNC flag set to 1, it MUST send a
>    PCErr with the SRP-ID-number of the PCUpd, Error-Type 20 and Error-
>    Value TBD (suggested value - 4) 'Attempt to trigger synchronization
>    when the TRIGGERED-SYNC capability has not been advertised' (see
>    Section 8.1).
> 
> This is all very well for PCCs that support this I-D but don't want to do
> triggered resynch. What about a PCC that doesn't even support this I-D?
> How will it process this PCUpd (which, obviously, it should not receive)?
> I'm sure you can just find a relevant paragraph of draft-ietf-pce-
> stateful-pce to point to.
> 
[[Dhruv Dhody]]Added but kept it generic. 
> ---
> 
> 7. and 8.3
> Hang on! Section 7 is commandeering bits while section 8.3 is requesting
> them from the registry.
> I suggest Section 7 should be reduced to:
> - a pointer to draft-ietf-pce-stateful-pce where the TLV is defined
> - a list of the new bits, their identifier letters, and their meanings
> - a forward pointer to section 8.3
> 
[[Dhruv Dhody]] Updated.
> ---
> 
> 9.6.  Impact On Network Operations
> I think you under-value your work. The whole point (as explained in your
> various motivation sections) of your work is to reduce and control the
> loading on the management plane in the operational network.
> Conversely, as discussed above, the use of the T-flag means that a PCE
> session is up but unusable. That has an impact on network operations and
> is something the operator will need to be able to see ("My PCE is up,
> there is a PCEP session, but nothing is happening!")
> 
[[Dhruv Dhody]] Updated. 
> ---
> 
> I believe section 10 needs to discuss attacks that come through
> impersonating a "PCE that has moved". I.e., I harvest the SPEAKER-ENTITY-
> ID on a current session, I bring that session down, I open a new session
> from my pseudo-PCE and impersonate the old PCE by spoofing the SPEAKER-
> ENTITY-ID. I can then attack the PCC through repeated re-synch; I can
> gather private info from the PCC; I can pretend to synch so that when the
> old PCE comes back the PCC has discarded incremental info and has to do a
> full resynch.
> 
> Section 10 should also discuss mitigations, not just point out attacks.
> 
> Lastly, you say in section 10
>    The PCC is free to drop any trigger re-synchronization
>    request without additional processing.
> This might have been good to explore in an earlier section. I think that
> PCEP generally makes an assumption that a received message will be acted
> on, but what you have done is lock a PCE/PCC session by discarding a
> trigger request. Without a response the PCE and PCC are in a form of
> deadlock - the PCE thinks all state is stale and is waiting for an update.
> Depending on how you read things, it is not allowed to receive (or process)
> and PCRpt that is not a SYNC or any PCReq.
> 

[[Dhruv Dhody]] I updated this section. I have added an error processing when the PCC cannot re-synchronize.  

> === Nits
> 
> Please s/draft/document/ in the Abstract and Introduction
> 
> ---
> 
> In Section 2 you have...
> 
>    This document uses the following terms defined in
>    [I-D.ietf-pce-stateful-pce]: Delegation, Redelegation Timeout
>    Interval, LSP State Report, LSP Update Request, LSP State Database.
> 
> But draft-ietf-pce-stateful-pce defers to draft-ietf-pce-stateful-pce-app
> for "Delegation".
> 
> ---
> 
> 4.2
>    [I-D.ietf-pce-stateful-pce] describes state synchronization and
>    Section 3 describes state synchronization avoidance by using LSP-DB-
>    VERSION TLV in its OPEN object.
> s/Section 3/Section 3 of this document/
> 
> ---
> 
> 4.2
>    After the synchronization procedure finishes, the
>    PCC can dump this history information.
> s/dump/discard/
> 
> ---
> 
> 6.2
>    Support of PCE-triggered state synchronization is advertised by both
> s/synchronization/re-synchronization/
> 

[[Dhruv Dhody]] All nits updated. 

Thanks again for detailed comments. 

Regards,
Dhruv

> _______________________________________________
> Pce mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/pce