Subject: [Pce] TSV-ART review of draft-ietf-pce-stateful-sync-optimizations

From: "Adrian Farrel" <[email-masked]>
Date: Fri, 24 Feb 2017 17:41:33 -0000

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/

---

From: Dhruv Dhody <[email-masked]>
Date: Sun, 26 Feb 2017 14:06:15 +0000

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:[email-masked]] On Behalf Of Adrian Farrel
> Sent: 24 February 2017 23:12
> To: [email-masked]
> Cc: [email-masked]; [email-masked]; [email-masked]; draft-ietf-pce-
> [email-masked]
> Subject: [Pce] TSV-ART review of draft-ietf-pce-stateful-sync-
> optimizations
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 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
> 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
> 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".
> 
> 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.
> 
> 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.
> 
> - 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
> 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.
> 
> consequences, and you are trying to optimise a terribly small corner case.
> 

[[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".
[[Dhruv Dhody]] Removed "reliable". 
> 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:
> 
[[Dhruv Dhody]] Ack. 
>    But why "SHOULD" not "MUST"? What is the optional alternative?
[[Dhruv Dhody]] Ack. Changed to "MUST".
> Why "SHOULD ignore" and not "MUST ignore"? What else might a PCE do with
[[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. 
> 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. 
> represented across the 8 octets.
[[Dhruv Dhody]] Added Network Byte order. 
> need to force anything with the D flag. Maybe the PCC can "announce"
[[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
> 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...
> 

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

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

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

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

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

> 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. 
> - a forward pointer to section 8.3
[[Dhruv Dhody]] Updated.
> 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. 
> 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.
> 
> 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.  

> s/synchronization/re-synchronization/

[[Dhruv Dhody]] All nits updated. 

Thanks again for detailed comments. 

Regards,
Dhruv

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

---

From: "Adrian Farrel" <[email-masked]>
Date: Sun, 26 Feb 2017 17:31:08 -0000

Thanks Dhruv, speedy as usual!

> > b. support for synchronisation avoidance if DBvs match with full
> > resynch otherwise
> [[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.
> 
> - 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)

Presumably, "last sent by PCC" is in Open from PCC, and "last seen by PCE" is in
Open from PCE.
Only the value in the Open from the PCE actually causes any action.
Ah no, I see the case that when the two value match, there will be no Sync so
the PCE does not have to wait for SYNC = 0.
You could make that explicit!

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

Yes, I think that was the confusion. 
Also just a little confusion around start of day when the PCE has no record of a
DBv and so must send an Open without a LSP-DB-VERSION TLV rather than one with
value zero.

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

Thanks.

> 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 version don't match and D flag is set, we do Incremental sync; otherwise
full
> sync.

Erm, where's that then?

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

OK. The first use case is important, and the error code will help.
I am still unsure about the wider impact.

> [[Dhruv Dhody]] Removed "reliable".

:-)

[snip]

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

Hmmm. Mutter, mutter, mutter.

[snip]

> [[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 agree with all this, but I don't think you have addressed for me how the PCC
knows what information it has to retain. 
The text says it only has to retain state changes that happened while the
session was down.
I am saying it also has to retain state changes that might not have made it into
the PCE's DB before the session went down or the PCE restarted.

I agree that you have closed this window by saying "in the event that the
information is not available to the PCC, do a full synch." 
It just doesn't seem very graceful, nor in the spirit of synch avoidance.

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

But still, why tear down the session and start again instead of just getting on
with the synch?

BTW, I think the D bit MUST be 0 on the new session or the wold will end in an
infinite loop of session thrash.

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

OK. And sorry for my bad reading.

I think you still have the converse of the "SHOULD NOT" to address.

[snip]

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

Oh, well, if you're going to update that draft as well, why not make it
optional?

Anyway, please make sure the right people are triggered to make the right
updates to the other draft!

[snip]

Cheers,
Adrian

---

From: Dhruv Dhody <[email-masked]>
Date: Mon, 27 Feb 2017 14:24:19 +0000

Hi Adrian, 

Thanks for your reply. See inline. 

> From: Adrian Farrel [mailto:[email-masked]]
> Sent: 26 February 2017 23:01
> To: Dhruv Dhody <[email-masked]>; [email-masked]
> Cc: [email-masked]; [email-masked]; [email-masked];
> [email-masked]
> Subject: RE: [Pce] TSV-ART review of
> draft-ietf-pce-stateful-sync-optimizations
> 
> > > 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
> > > 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
> > > 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.
> > >
> > > LSP-DB- VERSION TLV in its Open message. I can see that it helps the
> > [[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
> > - 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)
> 
> Presumably, "last sent by PCC" is in Open from PCC, and "last seen by PCE"
> is in Open from PCE.
> Ah no, I see the case that when the two value match, there will be no Sync
> so the PCE does not have to wait for SYNC = 0.

[Dhruv] Done! 

> > - 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)
> > - F (TRIGGERED-INITIAL-SYNC): Wait for PCE to trigger state sync
> > procedure
> > *now*
> >
> Also just a little confusion around start of day when the PCE has no record
> of a DBv and so must send an Open without a LSP-DB-VERSION TLV rather than
> one with value zero.
> 

[Dhruv] This is stated as - 

   If a PCEP speaker's LSP state database did not survive the
   restart of a PCEP session or at startup when the database is empty,
   the PCEP speaker MUST NOT include the LSP-DB-VERSION TLV in the OPEN
   object.

> > Please have a look at the working copy and if further changes are needed.

[Dhruv] It was(/is) included in the email.
Also at - https://github.com/dhruvdhody-huawei/ietf/raw/master/draft-ietf-pce-stateful-sync-optimizations-09.txt 

> 
> > > ---
> > >
> > > 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.
> > >
> > > provide a reliable mechanism (actually, even the mechanism for this
> "SHOULD" is a bit vague).
> > >
> > > re- assume a PCEP session without any further impact.
> > >
> > > 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.
> > >
> > > - 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).
> > >
> > > 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.)
> >
> 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 -
> >
> > 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.
> > Please have a look at the working copy and see if further details
> > should be
> added.
> 
[Dhruv]: Will recheck with our shepherd/AD on this point. 

> > > 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...
> > >
> > 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
> I agree with all this, but I don't think you have addressed for me how the
> PCC knows what information it has to retain.
> I am saying it also has to retain state changes that might not have made
> it into the PCE's DB before the session went down or the PCE restarted.

[Dhruv] The text now says - 

   In order to carry out incremental state synchronization, it is not
   necessary for a PCC to store a complete history of LSP Database
   change for all time, but remember the LSP state changes (including
   LSP modification, setup and deletion), that the PCE did not get to
   process during the session down.  Note that, a PCC would be unaware
   that a particular LSP report has been processed by the PCE before the
   session to PCE went down.  So a PCC implementation MAY choose to
   store the LSP State Database Version Number with each LSP at the time
   its status changed, so that when a session is re-established and
   incremental synchronization could be attempted based on the PCE's
   last LSP State Database Version Number.  For LSP that is deleted at
   the PCC, the PCC implementation would need to remember the deleted
   LSP in some way to make sure this could be reported as part of
   incremental synchronization later.  The PCC would discard this
   information based on a local policy, or when it determines that this
   information is no longer needed with sufficient confidence.  

> > 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
> But still, why tear down the session and start again instead of just getting
> on with the synch?
> 

[Dhruv] It is only the PCC that knows that it can't do incremental, and the PCE is expecting incremental sync. 
In this case if the PCC attempt full sync, for the new and existing LSPs there won't be a problem  but for the LSP that were deleted at PCC would not be updated at the PCE, because the PCE in case of incremental did not do stale marking and purge unreported LSP at the end. 
Thus we could not find a way around it. 

> BTW, I think the D bit MUST be 0 on the new session or the wold will end
> in an infinite loop of session thrash.
[Dhruv] This is already in the draft as - 

   The PCC
> > > 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
> > >
> > > 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.
> > >
> > > 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.
> >
> > 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.
> >
[Dhruv] The text was updated to - 

   If the
   TRIGGERED-INITIAL-SYNC capability is advertised by a PCE and the PCC,
   the PCC MUST NOT trigger state synchronization on its own.  If the
   PCE receives a PCRpt message before the PCE has triggered the state
   synchronization, the PCE MUST send a PCErr with Error-Type 20 and
   Error-Value TBD3 (suggested value - 3) 'Attempt to trigger
   synchronization before PCE trigger' (see Section 8.1).

> > > 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:
[Dhruv] They are in the cc, will do my part and remind them. 

Regards,
Dhruv

---

From: "Adrian Farrel" <[email-masked]>
Date: Mon, 27 Feb 2017 18:04:58 -0000

Thanks Dhruv,

Looks like you have all points covered.
I will try to look again when you post the new version, but if you are to be
trusted ;-) then my concerns are cleared.

Adrian

> From: Dhruv Dhody [mailto:[email-masked]]
> Sent: 27 February 2017 14:24
> To: [email-masked]; [email-masked]
draft-ietf-pce-stateful-sync-
> [email-masked]
> address.
> [Dhruv] It is only the PCC that knows that it can't do incremental, and the
PCE is
> expecting incremental sync.
> In this case if the PCC attempt full sync, for the new and existing LSPs there
won't
> be a problem  but for the LSP that were deleted at PCC would not be updated at
> the PCE, because the PCE in case of incremental did not do stale marking and
> purge unreported LSP at the end.
> respond?