Subject: AD Review of draft-ietf-bfd-seamless-base

From: "Alvaro Retana (aretana)" <[email-masked]>
Date: Fri, 25 Sep 2015 16:49:28 +0000

Hi!

In general, I think that this document could use being cleaned up and tightened; there are parts that could be clearer and better specified.  For example, many of state variables in rfc5880 seem to apply here, but they are not explicitly mentioned, the document claims to solve the use cases (in I-D.ietf-bfd-seamless-use-case), but is not explicit about how it does it, etc.  Please take a look at my Major comments below.

As I mention in the review for draft-ietf-bfd-seamless-ip, please consider merging the two documents.  I will keep this document under AD Evaluation (and not return it to the WG) unless the WG Chairs consider that it is necessary to WGLC the resulting document.

Thanks!

Alvaro.

Major:

  1.  Section 1. (Introduction) says that "This document extends BFD to provide solutions to use cases listed in [I-D.ietf-bfd-seamless-use-case].”  Maybe it’s just me, but I fail to see how all the use cases are satisfied — in part because the requirements in I-D.ietf-bfd-seamless-use-case are not clear (see my review for that document), and in part because this document isn’t explicit about how the specification solves the use cases.  For example, how does this document provide a solution for the use case in section 3.6. (BFD for Anycast Address)?
  2.  Normative References
     *   I-D.ietf-bfd-multipoint should clearly be Normative because of the new bfd.SessionType state variable
     *   I-D.ietf-bfd-generic-crypto-auth should also be Normative because of how the Security Considerations are written: pointing to is as a “MUST".  Given that (as far as I can tell) there aren’t implementations of I-D.ietf-bfd-generic-crypto-auth, we could end up with a Normative reference that blocks the publication of this document.  I want to suggest that the comments be reworded as a suggestion or pointer to potential solutions, not as a mandate to use them.  [Disclaimer: we will still need the SecDir to review.]
  3.  Related to 7.2.2 and 7.3.2 (see below for more), I’m assuming that all session variables are the same state variables defined in rfc5880, and that there are no new S-BFD-specific variables.  Is that correct?  If so, then many of my questions/comments below refer to the use of those variables and their mention in the text.  What seems to be different is not a specific variable, but how it is maintained.
  4.  Section 7.2.2. (Transmission of S-BFD Control Packet by SBFDInitiator)
     *   The header of the description uses a “MUST”, so the other rfc2119 keywords used later are redundant (as in another “MUST”) or in contradiction:  what does “MAY” mean in presence of a preceding “MUST”?
     *   BTW, it may be easier to just point out the differences with respect to Section 6.8.7. (Transmitting BFD Control Packets) in rfc5880.
     *   "Diagnostic (Diag)   MAY be set to appropriate value for communicating with peer”  The obvious question is: which values?  Section 7.4. (Diagnostic Values) tries to address that, but it says that "Diagnostic value in both directions MAY be set to a certain value…details of such are outside the scope of this specification.”  What about bfd.LocalDiag?  Are those values still valid?  As long as 7.4 is there, you could explain a little bit more why this doesn’t matter.
     *   "State (Sta)   Set to the value indicated by local state.”  Would that local state be bfd.SessionState, or is there a new state variable?
     *   "Poll (P)” …add "or 0 if not.”  (?)
     *   "Detect Mult   MUST be set to a value describing locally used multiplier value.”  Do you mean bfd.DetectMult?
     *   "My Discriminator   Set to value assigned by local node.”  Do you mean bfd.LocalDiscr?
     *   "Your Discriminator   Set to value corresponding to remote entity.”  Do you mean bfd.RemoteDiscr?  BTW, I think you should explain how this variable is initialized.
     *   "Desired Min TX Interval   MUST be set to a value describing local desired minimum transmit interval.”  Do you mean bfd.DesiredMinTxInterval?
     *   You didn’t say anything about the "Authentication Section”.
  5.  Section 7.3.2. (Transmission of S-BFD Control Packet by SBFDReflector): note that some of the comments are different.
     *   The header of the description uses a “MUST”, so the other rfc2119 keywords used later are redundant (as in another “MUST”) or in contradiction:  what does “MAY” mean in presence of a preceding “MUST”?
     *   BTW, it may be easier to just point out the differences with respect to Section 6.8.7. (Transmitting BFD Control Packets) in rfc5880.
     *   "Diagnostic (Diag)   MAY be set to appropriate value for communicating with peer”  The obvious question is: which values?  Section 7.4. (Diagnostic Values) tries to address that, but it says that "Diagnostic value in both directions MAY be set to a certain value…details of such are outside the scope of this specification.”  What about bfd.LocalDiag?  Are those values still valid?  As long as 7.4 is there, you could explain a little bit more why this doesn’t matter.
     *   "State (Sta)   MUST be set to UP or ADMINDOWN.”  Even though the states are not all of the ones in rfc5880, the state itself still refers to bfd.SessionState, right?   It would be really nice to include a state machine for the SBFDReflector (similar to the one in Section 7.2.1. (SBFDInitiator State Machine), where the states are clarified).  BTW, the text also says "Clarification of reflector BFD session state is described in Section 7.8.”, but that section doesn’t really clarify anything related to the session states.
     *   Several fields are set to the value copied from the received packet (Detect Mult, My Discriminator, Your Discriminator and Desired Min TX Interval).  I’m assuming that all these values are copied into the state variables defined in rfc5880.  What may change then is not the fact that Detect Mult (for example) is set to bfd.DetectMult, but how the variable is initialized/maintained.
     *   "Required Min RX Interval   …how many incoming control packets this reflector BFD session can handle.”  Over a period or time?  If it end up being different than bfd.RequiredMinRxInterval, then we’ll need a new state variable
     *   You didn’t say anything about the "Authentication Section”.
  6.  Section 10. (S-BFD Echo Function)
     *   “…it is RECOMMENDED that the "Required Min Echo RX Interval” field simply be set to zero in both directions.”, but Section 7.3.2. (Transmission of S-BFD Control Packet by SBFDReflector) says  about the same field that "If device supports looping back S-BFD echo packets” then it "MUST set non-zero value desired by local device.”   Those two statements are in contradiction.
     *   Unlike rfc5880, this document doesn’t explicitly mention that "Some form of authentication SHOULD be included, since Echo packets may be spoofed.”   The recommendation of sending both S-BFD control and echo packets points at alleviating some of the spoofing concerns even though they are independent packets (in other words: the Echo packet can still u-turn at a different node).  Please include a discussion of the alleviated security concern in the Security Considerations (since it is different than rfc5880).  Also, it would be nice if it was mentioned explicitly whether authentication for Echo packets is needed/recommended or not.
  7.  Section 11. (Security Considerations)
     *   "crypto sequence number”  What are you referring to?  I’m guessing the Sequence Number field in the Authentication Section — is that a good guess?  Please be specific and include a reference.
     *   The text says that the "SBFDReflector MUST compute the Authentication data”, but that it "MUST NOT look at the crypto sequence number”.  Is that a contradiction?  As defined in rfc5880, the Authentication Data seems to include everything in the Authentication Section, including the sequence number.
     *   Why isn’t the “loop problem” in Appendix A mentioned?
  8.  Nowhere in this document (or draft-ietf-bfd-seamless-ip) is congestion mentioned.  rfc5880 talks about some of the considerations.  Are there new congestion-related considerations that arise because of eliminating some of the negotiation aspects?  Thinking out loud: if a session doesn’t have to be established (and everyone knows a remote discriminator), then there’s a possibility of more nodes sending traffic to a specific reflector (just as an example).  Please include some text indicating any congestion issues — or at least explaining why there aren’t any new ones.

Minor:

  1.  Updates to rfc5880.  This document is marked as updating rfc5880.  It would be nice to have a section (or maybe just a couple of sentences) summarizing what the updates are.
  2.  The base document is not describing the operation in IP and MPLS environments, are  the references to rfc5881, rfc5883, rfc5884 and rfc5885 needed in 7.3 and 11?  And do the procedures and security considerations really apply?
  3.  Section 4.1. (S-BFD Discriminator Uniqueness)  I think the text in this section is a little confusing.  The requirement (the "S-BFD discriminator…MUST be unique within an administrative domain”) is clearly stated at the start, but then the justification of why goes into how is IP is used (with no reference to where S-BFD for IP is specified) and concludes that (in that case) the "S-BFD discriminator only has to be unique within a local node” — at first read it sounds like there is a contradiction in the text.  The paragraph closes with a reiteration of the uniqueness.  Please clarify — maybe specifically state that the discussion (maybe in a separate paragraph) is to justify the uniqueness..
  4.  Section 6.2. (State Variable Initialization and Maintenance)  "Some state variables…”  But only one is listed.  Is this the only one?  Please include a reference to rfc5880 (for ease of readability).
  5.  Section 7.1. (Demultiplexing of S-BFD Control Packet)
     *   What happens if the “your discriminator” field doesn’t match an existing SBFDReflector session?  I think the piece where the packet is discarded is missing.  Section 7.3.1. (Responder Demultiplexing) does say that in that case the packet "MUST NOT be considered for this mechanism”.  Does that mean that the packets are to be discarded, or something else?
     *   Is there a reason for having 7.3.1?  IOW, can anything extra there be included in 7.1?
     *   It may be the indenting, but it looks like the logic says: if the packet doesn’t correspond to an SBFDInitiator session, then discard it.
     *   "More details on S-BFD control packet demultiplexing are described in relevant S-BFD data plane documents.”  Section 4. (S-BFD Control Packet Demultiplexing) of draft-ietf-bfd-seamless-ip actually has less details: it doesn’t include the step about discarding if there’s no corresponding SBFDInitiator session, nor the last ELSE.
  6.  Appendix A. (Loop Problem)  s/reflectors MUST set the TTL/reflectors must set the TTL    This text is not the normative text, put a reference to rfc5880 instead.

Nits:

  1.  Missing references in Terminology: "The reader is expected to be familiar with the BFD, IP and MPLS terminologies and protocol constructs."
  2.  Section 3. (Seamless BFD Overview): "The IS-IS with..”   The IS-IS what?
  3.  Section 7.2. (Initiator Procedures)
     *   The example and the figure seem out of place in this section as the responder procedures haven’t been introduced yet.
     *   s/ASCII art/Figure 3
  4.  Section 7.6. (Control Plane Independent (C)) does not seem to say anything different from rfc5880.  Maybe we can get rid of it.
  5.  Maybe Section 7.7. (Additional SBFDInitiator Behaviors) should be a subsection of Section 7.2. (Initiator Procedures).
     *   The same for 7.8. (Additional SBFDReflector Behaviors) and 7.3. (Responder Procedures)
  6.  Section 8. (Scaling Aspect)  The text indirectly implies that the scaling is better by saying that the number of sessions is less.. I understand the point, but it just sounds like a superfluous section to me.
  7.  Section 9. (Co-existence with Classical BFD Sessions) is another superfluous section; there’s nothing here that you couldn’t have said in 7.1.

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Sun, 27 Sep 2015 20:50:23 +0000

Hi, Alvaro,

Thanks for your thorough review!

Before the point-by-point response, I wanted to provide some perspective on a small subset of your comments:

> On Sep 25, 2015, at 12:49 PM, Alvaro Retana (aretana) <[email-masked]> wrote:
> 

This is a recurring theme, one that you mention in every email about draft-ietf-bfd-seamless-*. It might be useful to share some additional context and perspective.

When we first started drafting ideas and early text of S-BFD, there was a single document for base + IP. At some point, we started drafting what S-BFD for Segment Routing would look like, and different optimizations for different environments. At that point, we realized that it was more sensible to have a common -base specification, followed by a number of adjunct documents, each about a different environment/transport.

Yes, draft-akiya-bfd-seamless-ip is quite basic and straightforward, specifying ports, addresses, and small specifics in procedures. And if there were only these two documents, it would make perfect sense (to me) to merge them. However, there’s more! :-) For example, draft-akiya-bfd-seamless-sr specifies S-BFD for Segment Routing, in which a different set of specifics are needed (given for example the ability to control the return path). Similarly, there are other uses of S-BFD such as S-BFD for Tunnels, S-BFD for Pseudowires (draft-ietf-pals-seamless-vccv), etc. All of these define very narrowly scoped specifics (IP vs. SR vs. PW) — however, all of those adjunct specifics would pollute and dramatically lower the SNR if all merged to the -base spec.

Net-net, we opted for modularity in the specifications.

The BFD WG chose to first progress -base and -ip, and only after those were more mature, then advance -sr. This would not imply that the first two documents on the sbfd set are the only ones.

We did consider merging the two documents, even after we purposely split them. If these were the only two docs in the set, I’d support merging them (or not having split them). But in the larger constellation of S-BFD docs, I consider it best to advance the -base separated from all the different specifics.

> Normative References
> I-D.ietf-bfd-multipoint should clearly be Normative because of the new bfd.SessionType state variable
Great catch.

The new bfd.SessionType state variable is new for both documents, and neither has a dependency on values of the variable from the other document. Consequently, I do wonder if this document should define the variable and bfd-multipoint refer to this — there is no serialization needed other than a common variable with independent use.
> I-D.ietf-bfd-generic-crypto-auth should also be Normative because of how the Security Considerations are written: pointing to is as a “MUST".  Given that (as far as I can tell) there aren’t implementations of I-D.ietf-bfd-generic-crypto-auth, we could end up with a Normative reference that blocks the publication of this document.  I want to suggest that the comments be reworded as a suggestion or pointer to potential solutions, not as a mandate to use them.  [Disclaimer: we will still need the SecDir to review.]
Another good point — thanks — given how the text is written.

I am not sure, however, the text is written in the best way. :-)

> Nowhere in this document (or draft-ietf-bfd-seamless-ip) is congestion mentioned.  rfc5880 talks about some of the considerations.  Are there new congestion-related considerations that arise because of eliminating some of the negotiation aspects?  Thinking out loud: if a session doesn’t have to be established (and everyone knows a remote discriminator), then there’s a possibility of more nodes sending traffic to a specific reflector (just as an example).  Please include some text indicating any congestion issues — or at least explaining why there aren’t any new ones.
> 

Very much agree. I believe there are subtle differences in the congestion considerations, because if the simplified negotiation.

Further, I believe those should live in the -base doc. That is missing, and we should fill in this gap.

Thanks!

— Carlos.

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Sun, 27 Sep 2015 20:54:37 +0000

Hi, Alvaro,

On Sep 25, 2015, at 12:49 PM, Alvaro Retana (aretana) <[email-masked]<mailto:[email-masked]>> wrote:

  1.  Normative References

Great catch.

  1.

  1.  Nowhere in this document (or draft-ietf-bfd-seamless-ip) is congestion mentioned.  rfc5880 talks about some of the considerations.  Are there new congestion-related considerations that arise because of eliminating some of the negotiation aspects?  Thinking out loud: if a session doesn’t have to be established (and everyone knows a remote discriminator), then there’s a possibility of more nodes sending traffic to a specific reflector (just as an example).  Please include some text indicating any congestion issues — or at least explaining why there aren’t any new ones.

Thanks!

— Carlos.