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.
