Subject: [Pce] Benjamin Kaduk's No Objection on draft-ietf-pce-association-group-09: (with COMMENT)

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Thu, 11 Jul 2019 00:38:10 -0700

Benjamin Kaduk has entered the following ballot position for
draft-ietf-pce-association-group-09: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-pce-association-group/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I'm always a little reluctant to publish framework documents without any
examples of using that framework (i.e., this document does not define
any association types), but this work seems well thought-out and it
looks like there are a handful of association types in development by
the WG.  Would it be worth listing a few as informational references to
give the reader a broader sense of what associations might be used for?

Thanks for the note in the shepherd writeup about the author count!

Thank you also for the very readable document -- it's clear that the
authors took care to organize the content in a manner accessible to
the reader.

As a general note, do we need to say anything about the multi-byte
integer values being encoded in network byte order?  (Perhaps following
RFC 5440's implicit convention is the right thing to do.)

Section 1

   [RFC6780] defines the RSVP ASSOCIATION object, which was defined in
   the context of GMPLS-controlled Label Switched Paths (LSPs) to be

nit: is this supposed to be  RFC 4872?

Section 3.3

   The dynamically created association can be reported to the PCEP peer
   via the PCEP messages as per the stateful extensions.  While the
   operator-configured association is known to the PCEP peer before
   hand, a PCEP peer could ask for a LSP to join the operator-configured
   association via the stateful PCEP messages.

nit: I suggest s/While/When/, if I understand correctly.

   Multiple types of associations can exist, each with their own
   association identifier space.  The definition of the different
   association types and their behaviours is outside the scope of this
   document.  The establishment and removal of the association
   relationship can be done on a per LSP basis.  An LSP may join
   multiple association groups, of different or of the same association
   type.

Is it possible for an LSP to join multiple association groups of the
same type and then for configuration to be assigned to two groups in a
manner that conflicts?  What procedure is used for conflict resolution
in such a case?

Section 3.4

   Association identifier range for sources other than the PCEP speaker
   (for example an NMS system) is not communicated in PCEP and the
   procedure for operator-configured association range setting is
   outside the scope of this document.

Given the discussion in the rest of the document, it seems that
reserving a specific range for operator configuration (across all
association types) is too rigid to meet the various anticipated use
cases.  Is that a correct assessment?

Section 5.1

   If the Assoc-type is not recognized or supported by the PCEP speaker,
   it MUST ignore that respective Start-Assoc-ID and Range.  If the
   Start-Assoc-ID or Range are set incorrectly, the PCEP session MUST be
   rejected with error type 1 and error value 1 (PCEP session
   establishment failure / Reception of an invalid Open message).

I could maybe see competent engineers disagreeing about which of these
MUSTs would take precedence in a case where both apply.

   The Assoc-type MAY appear more than once in the OP-CONF-ASSOC-RANGE
   TLV in the case of a non-contiguous Operator-configured Association
   Range.  The PCEP speaker originating this TLV MUST NOT carry
   overlapping ranges for an association type.  If a PCEP peer receives
   overlapping ranges for an association type, it MUST consider the Open
   message malformed and MUST reject the PCEP session with error type 1
   and error value 1 (PCEP session establishment failure / Reception of
   an invalid Open message).

This might be a good place to specify the  handling if a
received range would cross the 0xffff boundary.

Section 6.1.1

   The Global Association Source TLV is an optional TLV for use in the
   Association Object.  The meaning and the usage of Global Association
   Source is as per [RFC6780].

Do we want to say Section 4 specifically of 6780?
(Similarly for Extended Association ID.)

Section 6.1.4

   the association group.  In this document, all these fields are called
   'association parameters'.  Note that the ASSOCIATION object MAY

I would humbly suggest s/all these fields are called 'association
parameters'/these fields are collectively called 'association
parameters'/.

Section 6.3.1

   The ASSOCIATION Object is OPTIONAL and MAY be carried in the Path
   Computation Update (PCUpd), Path Computation Report (PCRpt) and Path
   Computation Initiate (PCInitiate) messages.

   When carried in PCRpt message, it is used to report the association
   group membership pertaining to a LSP to a stateful PCE.  The PCRpt
   message are used for both initial state synchronization operations
   (Section 5.6 of [RFC8231]) as well as whenever the state of the LSP
   changes.  The associations MUST be included during the state
   synchronization operations.

I suspect this is just my hazy memory of OPTIONAL, but how does "MUST be
included" match up with "OPTIONAL".

Section 6.4

Do I understand correctly that for dynamically created association
groups, the creation is effected by just using the relevant parameters
in a request(/update/etc.) and there is no need to separately create or
allocate the association?

   If a PCE peer is unwilling or unable to process the ASSOCIATION
   object, it MUST return a PCErr message with the Error-Type "Not
   supported object" and follow the relevant procedures described in
   [RFC5440].  [...]

Does this imply that the P flag in the common header should always be
set for ASSOCIATION objects?

   In case the LSP is delegated to another PCE on session failure, the
   associations (and association information) set by the PCE remains
   intact, unless updated by the new PCE that takes over.

This includes the association source address?

Section 8

   attack vector.  An attacker could report too many associations in an
   attempt to load the PCEP peer.  The PCEP peer responds with PCErr as

"report" in the sense of causing the peer to create state to track them?

Section 12.2

Since the RFC 7525 procedures are RECOMMENDED, I think that reference
needs to be normative.

---

From: Dhruv Dhody <[email-masked]>
Date: Thu, 11 Jul 2019 14:17:56 +0000

Hi Benjamin, 

Thanks for your review, please see inline.. 

> examples of using that framework (i.e., this document does not define any
> association types), but this work seems well thought-out and it looks like
> there are a handful of association types in development by the WG.  Would
> it be worth listing a few as informational references to give the reader a
> broader sense of what associations might be used for?
> 
[[Dhruv Dhody]] Added. 

> authors took care to organize the content in a manner accessible to the
> reader.
> 
> As a general note, do we need to say anything about the multi-byte integer
> values being encoded in network byte order?  (Perhaps following RFC 5440's
> implicit convention is the right thing to do.)
> 
[[Dhruv Dhody]] In PCEP extensions, this has been implicit.  

> nit: is this supposed to be  RFC 4872?
[[Dhruv Dhody]] Ack.  

> nit: I suggest s/While/When/, if I understand correctly.
[[Dhruv Dhody]] Updated. 

> Is it possible for an LSP to join multiple association groups of the same
> type and then for configuration to be assigned to two groups in a manner
> that conflicts?  What procedure is used for conflict resolution in such a
> case?
> 
[[Dhruv Dhody]] This should be handled per association type. I will make sure that future documents that define association types include text for this. 

> Given the discussion in the rest of the document, it seems that reserving
> a specific range for operator configuration (across all association types)
> is too rigid to meet the various anticipated use cases.  Is that a correct
> assessment?
> 
[[Dhruv Dhody]] Yes. 

> MUSTs would take precedence in a case where both apply.
[[Dhruv Dhody]] Updated. 

   If the Assoc-type is not recognized or supported by the PCEP speaker,
   it MUST ignore that respective Start-Assoc-ID and Range.  If the
   Assoc-type is recognized/supported but the Start-Assoc-ID or Range
   are set incorrectly, the PCEP session MUST be rejected with error
   type 1 and error value 1 (PCEP session establishment failure /
   Reception of an invalid Open message).  

> This might be a good place to specify the  handling if a received range
> would cross the 0xffff boundary.
> 
[[Dhruv Dhody]] Added - 

   The incorrect range include
   the case when the (Start-Assoc-ID + Range) crosses the association
   identifier range of 0xffff.

> (Similarly for Extended Association ID.)
[[Dhruv Dhody]] Ack. 

> parameters'/these fields are collectively called 'association parameters'/.
> 
[[Dhruv Dhody]] Ack. 

> included" match up with "OPTIONAL".
[[Dhruv Dhody]] Added this - 

   If the LSP belongs to an association group, then the
   associations MUST be included during the state synchronization
   operations.

> Section 6.4
> 
> Do I understand correctly that for dynamically created association groups,
> the creation is effected by just using the relevant parameters in a
> request(/update/etc.) and there is no need to separately create or
> Does this imply that the P flag in the common header should always be set
> for ASSOCIATION objects?
> 
[[Dhruv Dhody]] No, that was not the intention. I have made this change - 

   If a PCEP speaker does not recognize the ASSOCIATION object in the
   stateful message, it will return a PCErr message with Error-Type
   "Unknown Object" as described in [RFC5440].  In case of PCReq
   message, the PCE would react based on the P flag as per [RFC5440].

   If a PCE peer is unwilling or unable to process the ASSOCIATION
   object in the stateful message, it MUST return a PCErr message with
   the Error-Type "Not supported object" and follow the relevant
   procedures described in [RFC5440].  In case of PCReq message, the PCE
   would react based on the P flag as per [RFC5440].

> This includes the association source address?
[[Dhruv Dhody]] Yes. If the association source needs to change, then the new PCE would create a new association with itself as source and remove the old one. 

> "report" in the sense of causing the peer to create state to track them?
[[Dhruv Dhody]] Yes, basically to overwhelm the peer.

> Since the RFC 7525 procedures are RECOMMENDED, I think that reference
[[Dhruv Dhody]] Ack. 

Working Copy: https://raw.githubusercontent.com/dhruvdhody-huawei/ietf/master/draft-ietf-pce-association-group-10.txt
Diff: https://tools.ietf.org/rfcdiff?url1=draft-ietf-pce-association-group-09&url2=https://raw.githubusercontent.com/dhruvdhody-huawei/ietf/master/draft-ietf-pce-association-group-10.txt

Thanks! 
Dhruv

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

---

From: Benjamin Kaduk <[email-masked]>
Date: Thu, 11 Jul 2019 17:01:06 -0500

Hi Dhruv,

Also inline

Thanks!

> [[Dhruv Dhody]] In PCEP extensions, this has been implicit.  

Okay; that's what it looked like to me, but there's no harm in checking.

> [[Dhruv Dhody]] This should be handled per association type. I will make sure that future documents that define association types include text for this. 

Okay; thank you.

>    associations MUST be included during the state synchronization

Ah, that helps; thank you.

>    would react based on the P flag as per [RFC5440].

I think I may have just confused myself previously; feel free to rever this
change if you don't think it's helpful.

> [[Dhruv Dhody]] Yes. If the association source needs to change, then the new PCE would create a new association with itself as source and remove the old one. 

Thanks for confirming.  I don't necessarily think there needs to be any
change to the text, here, since the current text is unambiguous.

> [[Dhruv Dhody]] Yes, basically to overwhelm the peer.

Okay. I might suggest "attempt to create" instead of "report", but I
recognize that there are reasons to use "report" in the context of PCRpt.

Thanks for all the updates!

-Ben

---

From: Dhruv Dhody <[email-masked]>
Date: Fri, 12 Jul 2019 05:21:06 +0000

Hi Ben, 

> > > Does this imply that the P flag in the common header should always
> > > be set for ASSOCIATION objects?
> > >
> I think I may have just confused myself previously; feel free to rever
> this change if you don't think it's helpful.
> 

[[Dhruv Dhody]] The extra text for PCReq doesn't hurt. So I would let the update stay. 

[[Dhruv Dhody]] Ack. 

Thanks for your detailed reviews. 

Dhruv