Benjamin Kaduk has entered the following ballot position for draft-ietf-pce-pcep-flowspec-10: Discuss
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-pcep-flowspec/ ---------------------------------------------------------------------- DISCUSS: ---------------------------------------------------------------------- As with the others, I also found this document to be quite easy to read and well-structured; thank you! I just have a couple points I'd like to discuss, but I am not pressing for a specific resolution and expect to change to No Objection once the discussion has occurred (whatever the conclusion is). This is a Discuss because I want to have a discussion, not because I'm confident in the correctness of my position. But it seems like the ambiguity about when multiple flow specifications in single FLOWSPEC object are treated as logical AND to narrow a single flow specification versus treated as separate flow specifications per Section 8.4 could lead to confusion, and it would be simpler and have less risk to stick to the "one flow specification per FLOWSPEC object" model as discussed in the rest of the document. If the ability to define multiple flows within a single FLOWSPEC object is retained, I think we need more specific procedures for identifying when that is the case, quite possibly with a specific enumeration of cases. I also mention in the per-section comments several places where (IIUC) there seems to be a need to match the Speaker Entity Identifier TLV as well as the FS-ID value. It might even be an exhaustive list, but please do a pass to check. ---------------------------------------------------------------------- COMMENT: ---------------------------------------------------------------------- 5575bis's validation procedures (§6) include things like "originator of the flow spec matches originator of best-match unicast route for the destination prefix in the flowspec". This doesn't seem to apply to PCEP, so presumably we are supposed to ignore such requirements. Is there a concrete list of which parts of the procedures are affected in this way? I agree with the TSV-ART reviewer that the Abstract and Introduction could likely be improved by a restructuring. In some places we call out that our usage of "Flow Specification" diverges from that of 5575bis, since we do not have an "action", whereas in other places we say that there is an implicit action of "forward all matching traffic onto the associated path". It might be better to try to stick to just one of these two worldviews. Abstract I did a little bit of a double-take at "may be unsolicited instructions", given my understanding that PCE-initiated LSPs are at a protocol level just requests to the PCC to initiate the path. Section 3.1 2. Decide what properties to assign to an LSP. This can include bandwidth reservations, priorities, and DSCP (i.e., MPLS Traffic Class field). This function is also determined by user configuration or response to predicted or observed traffic demands. nit: I think there's a missing word/words before "response", perhaps "as a response to" or "in response to". side note(?): I see that (4) refers to the "PCC" as signalling the LSP across the network, but (5) refers to the "head end" being told what traffic to put on the LSP. I can see how the respective functionalities are important for those functions, but I am not sure if the roles will ever be played by different entities (if they are always the same entity there may be some rhetorical value in using a consistent term for that single entity). Section 3.2 o Flow Specification information/state must be synchronized between PCEP peers so that, on recovery, the peers have the same understanding of which Flow Specifications apply. I don't remember seeing much specifically about recovery and state synchronization in this document; should we have a little more exposition (somewhere later on, perhaps §3.2.3) about how normal recovery mechanisms suffice to synchronize the flowspec state along with other LSP state? Section 3.2.1.1 The presence of the PCE FlowSpec Capability TLV in the OPEN Object in a PCE's OPEN message indicates that the PCE can distribute FlowSpecs to PCCs and can receive FlowSpecs in messages from PCCs. The presence of the PCE FlowSpec Capability TLV in the OPEN Object in a PCC's OPEN message indicates that the PCC supports the FlowSpec functionality described in this document. (nitty nit): is there a reason to not use the "supports the FlowSpec functionality described in this document" for both cases? If either one of a pair of PCEP peers does not indicate support of the functionality described in this document by not including the PCE FlowSpec Capability TLV in the OPEN Object in its OPEN message, then (I expect the RFC Editor to ask about the apparent double negative here if the text doesn't change before it gets to them.) Section 3.2.2 o PCRpt messages so that a PCC can report the traffic that the PCC plans to place on the path. (nit) I just want to confirm that "plans to place" is still correct, given that RFC 8231 defines PCRpt as "a PCEP message sent by a PCC to a PCE to report the current state of an LSP". The PCEP FLOWSPEC object carries zero or one Flow Filter TLV or one L2 Flow Filter TLV or both Flow Filter TLV as well as L2 Flow Filter TLV, which describes a traffic flow. (nit): this sentence was hard for me to parse ("zero or one <X> or one <Y> or ..." leaves the grouping of conditionals unclear). From subsequent text, I believe that the allowed possibilities are (one Flow Filter TLV and no L2 Flow Filter TLV), (no Flow Filter TLV and one L2 Flow Filter TLV), and (one Flow Filter TLV and one L2 Flow Filter TLV). If that's correct, I'd suggest rephrasing this more like "carries either or both of the Flow Filter TLV and the L2 Flow Filter TLV". Section 4 I'm a bit confused as to the usage of the two-octet "value" field that is apparently always set to 0, the same as the padding. I would perhaps have expected some kind of flags word if there was to be any non-trivial content in this capability TLV. Section 5 the PCEP object format defined in [RFC5440]. It is OPTIONAL in the PCReq, PCRep, PCErr, PCInitiate, PCRpt, and PCUpd messages and MAY be present zero, one, or more times. Each instance of the object specifies a traffic flow. (I know we've covered this before, and apologize for the noise, but I feel obligated to note that "zero, one, or more times" is equiavlent to "zero or more times" anyway.) The PCEP FLOWSPEC object carries a FlowSpec filter rule encoded in a TLV (as defined in Section 6). nit: not just a single TLV, per se, right? FS-ID (32-bits): A PCEP-specific identifier for the FlowSpec information. A PCE or PCC creates an FS-ID for each FlowSpec that it originates, and the value is unique within the scope of that PCE or PCC and is constant for the lifetime of a PCEP session. All subsequent PCEP messages can identify the FlowSpec using the FS-ID. The values 0 and 0xFFFFFFFF are reserved and MUST NOT be used. It might be worth a reference to draft-gont-numeric-ids-sec-considerations (I'm AD sponsoring it) as guidance for how the FS-ID is generated. There does not seem to be a need even for monotonicity of assignment, so some of the random assignment schemes might be best. Section 6 Section 7. Only one Flow Filter TLV or L2 Flow Filter TLV can be present and represents the complete definition of a Flow Specification for traffic to be placed on the tunnel. This tunnel is nit: I suggest rewording, as the current wording might be taken as implying that having both Flow Filter and L2 Flow Filter at the same time is forbidden; perhaps, "at most one Flow Filter TLV and one L2 Flow Filter TLV can be present". (I do see that the explicit list of options is given again a few sentences later.) Section 7 Though value 0 is currently reserved by the BGP specs, we are perhaps setting ourselves up for a conflict if it ever becomes "un-reserved" for BGP. Lumping it into the 1..255 range doesn't seem like it would introduce any problems, and would ameliorate this risk. (Doing this would have knock-on effects on text throughout the rest of the document.) Type values are chosen so that there can be commonality with Flow Specifications defined for use with BGP [I-D.ietf-idr-rfc5575bis]. We probably should reference draft-ietf-idr-flow-spec-v6 here as well, since our TLV can work with either IPv4 or IPv6, but the corresponding BGP registry is different. The content of the Value field in each TLV is specific to the type/ AFI and describes the parameters of the Flow Specification. The This suggests that any new allocations from the PCEP-only range should say what AFI(s) they are defined for, but no such guidance is provided in Section 10.3. Section 7.1 (Same comment about the value 0 as above.) All L2 Flow Specification TLVs with Types in the range 1 to 255 have Values defined for use in BGP (for example, in [I-D.ietf-idr-rfc5575bis] and [I-D.ietf-idr-flow-spec-v6]) and are set using the BGP encoding, but without the type octet (the relevant information is in the Type field of the TLV). The Value field is nit: this "all [...] in the range 1 to 255 have Values defined for use in BGP" makes it sound like they are all currently defined, which does not seem to be the case. Rewording to something like "L2 Flow Specification TLVs with Types in the range 1 to 255 have their Value interpreted as defined for use in BGP ([...]) and are set using the BGP encoding, but without the type octet" might help. Section 8.3 not the addition of a new flow as described in Section 8.4. The FS- ID field of the PCEP FLOWSPEC object is used to identify a specific Flow Specification. I'd suggest mentioning again that this is in conjunction with the Speaker Entity Identity TLV. When modifying a Flow Specification, all Flow Specification TLVs for the intended specification of the flow MUST be included in the PCEP FLOWSPEC object and the FS-ID MUST be retained from the previous description of the flow. (and likewise, if there is a requirement to preserve the Speaker Entity Identity TLV, though perhaps that's already required by RFC 8232.) Section 8.5 To remove a Flow Specification, a PCEP FLOWSPEC object is included with the FS-ID matching the one being removed, and the R bit set to (Presumably the Speaker Entity Identity TLV also has to match?) If the R bit is set and Flow Specification TLVs are present, an implementation MAY ignore them. If the implementation checks the Flow Specification TLVs against those recorded for the FS-ID of the Flow Specification being removed and finds a mismatch, the Flow Specification MUST still be removed and the implementation SHOULD record a local exception or log. Thank you for specifying the behavior in the event of a mismatch! Section 8.7 PCCs MUST apply the same ordering rules as defined in [I-D.ietf-idr-rfc5575bis]. (side note) I noted in my ballot comments on 5575bis that the ordering rules allow for situations where the priority of a rule with given semantic content can be different depending on how it is encoded, which seems risky to me. It would in principle be possible for this document to impose additional restrictions on how things are encoded to remove this potential disparity, though I do not actually expect us to want to do so (hence, this is just a side note). Furthermore, it is possible that Flow Specifications will be distributed by BGP as well as by PCEP as described in this document. In such cases implementations supporting both approaches MUST apply the prioritization and ordering rules as set out in [I-D.ietf-idr-rfc5575bis] regardless of which protocol distributed This MUST seems to just be saying the same thing as the previous paragraph? the Flow Specifications, however implementations MAY provide a configuration control to allow one protocol to take precedence over the other as this may be particularly useful if the Flow Specification make identical matches on traffic but have different actions. [...] And this MAY seems to be contradicting the MUST, making it "no longer a MUST". An implementation that receives a PCEP message carrying a Flow Specification that it cannot resolve against other Flow Specifications already installed MUST respond with a PCErr message I'm not entirely sure I understand what "resolve against" means in this context -- if I had to guess, it would be something about having a unique interpretation that is not in conflict with existing rules, but I'm pretty hazy on what the details of that would entail. Section 9 We should probably say that <Common Header> is specified in RFC 5440. I also couldn't tell if there was a clear rule we are using for when to expand out sub-structures that we are not modifying vs. leaving them to the referenced document. Many are left out, but we do expand, e.g., <path> in PCUpd/PCRpt. (I also note that we cover PCUpd and PCRpt in the reverse order to RFC 8231, not that it really matters for much of anything.) Section 12 modified or torn down. Such systems, therefore, apply security considerations as described in [RFC5440], [RFC6952], and [RFC8253]. In my reading, the security considerations of RFC 6952 are directed more at protocol designers than at operators; could you say a little more about what you expect the reader to take away from that reference? Also, I think that at least some of the security considerations from 5575bis are also relevant to our usage of the data structures. The description of Flow Specifications associated with paths set up or controlled by a PCE add a further detail that could be attacked without tearing down LSPs or SR paths, but causing traffic to be misrouted within the network. Therefore, the use of the security mechanisms for PCEP referenced above is important. I might consider mentioning that the BGP flowspec work can use the "same originator" check for flowspec destination address and longest-prefix match for routes to that address, which is unavailable in the PCEP scenario. On the other hand, the PCE is fairly trusted already, so maybe there is less need for such a check in the PCEP case. usually considered private customer information. Therefore, implementations or deployments concerned with protecting privacy MUST apply the mechanisms described in the documents referenced above. (A construction of the form "if you care about <X>, you MUST <Y>" doesn't actually impose much of a normative requirement...) Although this is not directly a security issue per se, the confusion and unexpected forwarding behavior may be engineered or exploited by an attacker. Therefore, implementers and operators SHOULD pay careful attention to the Manageability Considerations described in Section 13. I appreciate that you mention this risk in this way; thanks! Section 13.1 which path. Unlike the behavior in a distributed routing system, it is not important that each head-end implementation applies the same rules to disambiguate overlapping Flow Specifications, but it is important that: Just to check my understanding: this is "important" in the sense of "important to the stability of the network"? Section 13.3 This section seems like it could be roughly summarized as "someone should please write some YANG". My personal preference would be to say this more along the lines of "YANG models describing the functionality in this document have not yet been written, but are desirable. Some relevant preexisting work is: [...]", but I can understand if your preferences are different (or there is precedent for this kind of formulation). Section 15.2 Since we depend on RFC 4364 for the format of the RD, that seems to make it a normative reference. (We also use RFC 7399 as the source for a couple of defined terms, but that doesn't seem like a big deal to me, personally.) I think technically you need RFC 8231 to implement the updated PCUpd message we define, which arguably makes it normative as well. (Likewise 8281 for PCInitiate.) We do, however, require RFC 8232 for the Speaker Entity Identifier TLV, which is required in the FLOWSPEC object, so it's clearly a normative reference. _______________________________________________ Pce mailing list Pce@ietf.org https://www.ietf.org/mailman/listinfo/pce
