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

Hi Dhruv,

Also inline

On Thu, Jul 11, 2019 at 02:17:56PM +0000, Dhruv Dhody wrote:
> Hi Benjamin, 
> 
> Thanks for your review, please see inline.. 
> 
> > ----------------------------------------------------------------------
> > 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?
> > 
> [[Dhruv Dhody]] Added. 

Thanks!

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

> > 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?
> > 
> [[Dhruv Dhody]] Ack.  
> 
> > 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.
> > 
> [[Dhruv Dhody]] Updated. 
> 
> >    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?
> > 
> [[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.

> > 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?
> > 
> [[Dhruv Dhody]] Yes. 
> 
> > 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.
> [[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).  
> 
> > 
> >    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.
> > 
> [[Dhruv Dhody]] Added - 
> 
>    The incorrect range include
>    the case when the (Start-Assoc-ID + Range) crosses the association
>    identifier range of 0xffff.
> 
> 
> > 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.)
> > 
> [[Dhruv Dhody]] Ack. 
> 
> > 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'/.
> > 
> [[Dhruv Dhody]] Ack. 
> 
> > 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".
> > 
> [[Dhruv Dhody]] Added this - 
> 
>    If the LSP belongs to an association group, then the
>    associations MUST be included during the state synchronization
>    operations.

Ah, that helps; thank you.

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

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

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

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

> > Section 12.2
> > 
> > Since the RFC 7525 procedures are RECOMMENDED, I think that reference
> > needs to be normative.
> > 
> [[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
> > Pce@ietf.org
> > https://www.ietf.org/mailman/listinfo/pce