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 > > [email protected] > > https://www.ietf.org/mailman/listinfo/pce _______________________________________________ Pce mailing list [email protected] https://www.ietf.org/mailman/listinfo/pce
