Hi Adrian,
Getting back to the COMMENT portions...
On Wed, Aug 26, 2020 at 09:51:52PM +0100, Adrian Farrel wrote:
> Hi Ben,
>
> Thanks for the review.
> A lot of very helpful comments and discussions.
> All answers in line.
> I have a working copy with the edits (hint to co-authors: *I* have the
> working copy :-)
>
> Best,
> Adrian
>
[...]
>
> > 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?
>
>
[Did I miss a comment here?]
>
>
> > I agree with the TSV-ART reviewer that the Abstract and Introduction
> > could likely be improved by a restructuring.
>
> Oh โน
> I admit that I tend to write lengthy Abstracts, and that my style tends
> towards wordy.
>
> However, looking back at this document it seems to me that both sections
> attempt to describe the background before saying what the document does. We
> could flip that, but I think it would cause confusion by using terms and
> concepts that haven't been established in the text.
>
> Joe suggested adding paragraph 7 from the Introduction to the Abstract, so I
> will do that.
>
> For the Introduction, Joe suggests splitting the Introduction to create a new
> Background section. This seems like personal style to me: I dislike terse
> Introduction sections that simply repeat the Abstract; I prefer to follow the
> sequence of the Abstract, but add more substance along the way.
I agree that it's largely personal style, so go with what you like best...
> Joe's other comment on the Introduction is as follows. It flows nicely into
> your next Comment so I'll handle the two together.
>
> | It also seems odd that this paragraph (#6 of the intro) undermines
> | the terminology of the document that this supplements (as cited
> | in the abstract). These documents as a pair should have consistent
> | use of terminology, coining new terms as needed rather than
> | redefining a key term as different in the two.
>
> > 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.
>
> In BGP, Flow Specifications have a choice of associated actions that are
> exchanged alongside the Flow Specifications. Although 5575bis is clear that
> there is a distinction between the specification of a flow and the associated
> actions, common usage describes the combination as a Flow Specification. This
> is partly because the "Flow Specification NLRI" contains both the description
> of the flow (the Flow Specification) and the Extended Communities that define
> the actions.
>
> What we all missed in this is that the text defining a Flow Specification in
> 5575bis has changed since we copied it into this document!
>
> But we can tighten our language a bit. The point being that we use the
> encoding of the pieces of the Flow Specification that describe the traffic,
> but we do not present a range of choices of action since our usage is for one
> action only. And obviously, we will resynchronise with 5575bis.
Sounds good; thanks.
> > 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.
>
> OK. s/instructions/requests/
>
> > 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".
>
> "or in response"
>
> > 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).
>
> Yup, this is sloppy of us.
> In practice, a PCC could be something like an NMS that would then instruct
> the head end to signal.
> But in this context we are dealing with PCC==head end.
> Tidying the text for this.
Thanks for clarifying for me.
> > 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?
>
> I've added a reference to section 5.6 of RFC 8231
Thanks!
> > 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):
>
> (the best type)
>
> > is there a reason to not use the "supports the FlowSpec
> > functionality described in this document" for both cases?
>
> The author was thinking that this document doesn't really describe what the
> PCE actually does beyond the messages it sends and receives, while the
> document has more details about how the PCC processes.
>
> Is it important?
Of course not; if it was important it wouldn't be a nit :)
> > 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.)
>
> That's not a double negative, just clumsy text.
>
> If a PCEP speaker does not include the TLV then it is saying that it doesn't
> (or doesn't want to) support the function.
>
> NEW
> If either one of a pair of PCEP peers does not include the PCE
> FlowSpec
> Capability TLV in the OPEN Object in its OPEN message, then the
> other
> peer MUST NOT include a FLOWSPEC object in any PCEP message sent
> to the peer.
Thanks!
> > 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".
>
> s/plans to place/will/
Er ... I hope it's "will place" ;)
> > 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".
>
> I think bullets make this clearer...
Definitely!
> ...carries one of:
> o zero or one Flow Filter TLV
Though I'm not entirely sure what the "zero" case looks like...does that
only happen for removals?
> o one L2 Flow Filter TLV
> o both a Flow Filter TLV and an 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.
>
> The distinction is that the Value field can be used in a future spec to
> define capabilities, the padding is reserved.
> A thin distinction.
>
> > 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?
>
> A single Flow Filter TLV (which may contain multiple Flow Specification
> TLVs), or a single L2 Flow Filter TLV, or both.
Right. (Though, looking back, I think when I wrote this I had misparsed,
and was trying to point out that the Speaker Entity Identifier TLV is also
a TLV and in the FLOWSPEC object ... but that one has nothing to do with
carrying FlowSpec filter rules, so it's not relevant in this clause.)
> > 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.
>
> OK. That is an interesting document, I hadn't seen it before. Shame there
> isn't a WG that wants to discuss it. Would be good to drag it around to at
> least the various Area-specific mailing lists because it has pretty broad
> impact.
We've talked about it at secdispatch and SAAG; it should be heading out for
IETF LC fairly soon (ball is in my court). Please feel free to send me
specific suggestions for places to forward the IETF LC note, offline.
> > 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.)
>
> Yup
>
> > 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.)
>
> Agreed and rippled through 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.
>
> You're right. This is caught in other places, but should be fixed here.
I did notice it was caught in other places, and should have mentioned that
this seemed a lone straggler -- thanks for the fix.
> > 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.
>
> I think this 'Just Works' (TM)
> Section 5 shows where the AFI is placed for a FlowSpec.
> If the Type is from the PCEP range then there is only one place to go to find
> the meaning and the AFI says v4 or v6.
> If the Type is from the lower range, the AFI tells you both v4 or v6, and
> where to go to find the meaning.
And here I was thinking that we could do things for AFIs other than IP ;)
> > Section 7.1
> >
> > (Same comment about the value 0 as above.)
>
> Same fix.
>
> > 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.
>
> OK
>
> > 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.
>
> Yes
>
> > 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.)
>
> Yes
>
> > 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?)
>
> Yes (again)
>
> > 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!
>
> You are entirely welcome.
>
> > 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).
>
> I agree that we don't want to try to fix 5575bis in this document!
> In fact, while wanting to highlight the existence or issues around
> prioritisation and overlap, we very much wanted to punt this issue to the IDR
> WG which has far more experience dealing with FlowSpecs.
>
> > 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?
>
> I think you are right. Yet it says it slightly differently because some had
> suggested "I could have one rule for BGP and another rule for PCEP".
>
> > 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".
>
> Not quite. Suppose BGP and PCEP each distribute an identical FlowSpec (thus
> capturing exactly the same packets) but with different actions: BGP saying,
> for example, "drop packets"; PCEP (implicitly) saying "put them on this LSP".
> The rule might be to discard the second received, or to replace the first
> received. This knob would allow the operator to say what to do.
Ah, good point.
> > 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.
>
> We're into an area that we should probably have left to 5575bis.
> Yes, "cannot resolve against" means that there is already a FlowSpec
> installed, and the new one is somehow in conflict with it so that having both
> installed would imply some form of contradiction. I did have some examples of
> this, but my brain is now cloudy.
So I guess this is "resolve" in the sense of "has irresolvable conflicts
with other Flow Specifications that are already installed", then.
> > Section 9
> >
> > We should probably say that <Common Header> is specified in RFC 5440.
>
> I think the inheritance is through 8281 and 8231.
>
> > 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.
>
> No clear rule at all. Just trying to do what seemed clearest.
Okay. My opinion on what seems clearest is probably not of much value,
given how far outside the target audience I am :)
> > (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.)
>
> Curious ๐
>
> > 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?
>
> The 'instructions' in 6952 are principally targeted at implementations, but I
> think the considerations (highlighting risks/threats) seems to apply to
> operators (and perhaps their choice of which implementation to purchase).
Thanks for helping me understand.
> > Also, I think that at least some of the security considerations from
> > 5575bis are also relevant to our usage of the data structures.
>
> That's fair enough.
>
> > 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.
>
> In general, I prefer not to comment on the security of BGP in the PCEP
> document.
>
> A consideration might be to compare the address of the far end of the LSP
> with the lonest prefix route, but I am not really sure that is consistent
> with the use of LSP tunnels that are often intended to take traffic off the
> normal route. Indeed, it seems that the correct way to achieve this is to
> secure the communication/authentication with the PCE and assume that the
> PCE/operator is doint what they want to happen - having the PCC interfere or
> reject the command might actually be very frustrating.
Right; we have to trust the PCE a lot anyway so applying differential trust
here is arguably un-helpful.
> > 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...)
>
> True. Are we gong to say "an operator MUST care about privacy"? I don't think
> so.
> What we are offering is that if an operator does care about privacy then this
> is how they achieve it.
IMO that can be done without the 2119 keywords, but your opinion takes
precedence.
> > 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"?
>
> Yeah (not for reasons of poetic justice).
> For the first "important", "stability" is it.
> For the second "important", it is more likely "goodly functioning".
> Clarifying the text.
Thanks. (I meant the first one and was sloppy about checking that it was
the only one...)
> > 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).
>
> Far be it from me to plead for more YANG models!
> We were just trying to point to existing work and how it could be extended.
> However, I have changed the "will need" to "would need" to soften the message.
Okay.
> > Section 15.2
> >
> > Since we depend on RFC 4364 for the format of the RD, that seems to make
> > it a normative reference.
>
> Agreed
>
> > (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 prefer to avoid the resulting downref for these two largely descriptive
> terms.
>
> > 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.)
>
> Absolutely!
>
> > 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.
>
> Right again.
Thanks for all the clarifications and updates!
-Ben
_______________________________________________
Pce mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/pce
