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. --- 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.) === 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"? --- 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. --- 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? --- 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? --- 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. --- 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? --- 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. Why do you feel the need to recommend values for assignment in the IANA considerations section? --- In 3.3.1 it would probably be wise to state how the 64 bit number is represented across the 8 octets. --- 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. --- 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. --- 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. --- 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. --- 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) --- 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. --- 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 --- 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!") --- 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. === 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/