Hi Carlos,
:-)
Thanks for addressing the comments. I have looked through -08 and there
are a couple of extra points that I noticed - the s3.4 issue was
effectively mentioned wrt s4.5 in my previous notes.
Generally things are in good shape but there are some items that haven't
been addressed or there is a quibble.
If there is another version over the weekend I'll do my very best to
check it before Monday.
Regards,
Elwyn
Extra Points:
===========
I Forgot to mention that there is lack of consistency in capitalisation
of the message names: Personally I would go with Echo Request and Echo
Reply throughout to make it clear that these are message names.
s3,4, para 1:
If the
replying router is the destination (Label Edge Router) of the FEC,
then a Downstream Detailed Mapping TLV SHOULD NOT be included in the
MPLS echo reply. Otherwise, the replying router SHOULD include a
Downstream Detailed Mapping object for each interface over which this
FEC could be forwarded.
I suspect that the SHOULD NOT ought to be MUST NOT. Otherwise it needs
an explanation of the circumstances in which the DDMAP TLV could be
included. Similarly, the SHOULD needs to explain in what circumstances
you wouldn't include one or more DDMAP TLVs.
s6.2.3: The Unassigned row should have a blank reference.
On 28/10/2016 02:29, Carlos Pignataro (cpignata) wrote:
Deal Elwyn,
Many thanks for a great review!
I just finished addressing all your comments: the major issue (easy to
address, editorial fix, but with important implications), the minors,
and all the nits. Surprisingly, I found a few small additional
editorials, which I fixed as well.
Rev -08 would address all outstanding issues, from this review, Mirja,
and a couple others.
Please see inline for a line-by-line set of responses.
On Oct 20, 2016, at 4:42 PM, Elwyn Davies <elw...@dial.pipex.com
<mailto:elw...@dial.pipex.com>> wrote:
I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair. Please treat these comments just
like any other last call comments.
For more information, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
Document: draft-ietf-mpls-rfc4379bis-07.txt
Reviewer: Elwyn Davies
Review Date: 2016/10/21
IETF LC End Date: 2016/10/18
IESG Telechat date: (if known) -
Summary: Not ready. There is one major issue (already notified to
authors and agreed as an issue) and a considerable number of minor
and editorial issues. I have worked through the various RFCs and
errata that are subsumed into the new version and almost everything
has been correctly translated AFAICS. A couple of minor points are
covered in the comments.
Major issues:
============
s3.4: A number of items that are used in the normative Downstream
Detailed Mapping TLV were originally defined in s3.3 (Downstream
Mapping TLV format) but have been shifted to Appendix A.2. This
appendix is marked as non-normative. Thus there are now no normative
definitions for the various TLVs used in s3.4 that are defined in
A.2. I fear that these need to be returned to the normative part of
the specification.
This is an excellent catch. Thank you. The fix is simple and purely
editorial but the implication is clear.
I finished addressing this, which you will see posted as the new
revision. I am super happy with the outcome.
Looks good to me!
[I think it would be simplest and least error prone to swap the text
that was in s3.3 of RFC 4379 back out of A.2 and put backward
references to the new s3.4 into A.2 as necessary.]
Minor issues:
============
Sender/receiver terminology: The document can be somewhat confusing
to a naive reader. Sender and receiver are used in multiple
contexts. Where the context appears to relate to LSP ping, both
senders and receivers are involved in sending LSP ping packets. In
general, sender and receiver appear to imply sending and receiving of
Echo Request messages with their roles reversed with respect to Echo
Responses, with the receiver stimulated to send an Echo Response by
receiving an Echo Request. It would help if this terminology and
usage was explicitly set out early in the document. Additionally,
some instances would be made more comprehensible by making the
function explicit in the text e.g., in the operation of return codes.
Re-reading after fixing all the nits below, which include some sender
clarifications, looks good.
There is one place (s3.1, para 1) where I think it could be made
clearer. Adding a few words to that section will help overall as well
as just in that section.
s1.4/s3/s6.2.3: The R (Global) flag is defined in RFC 6426.
Unfortunately it isn't in the IANA considerations there as was
spotted in RFC Erratum 4012. Might be worth mentioning the erratum
(probably in s1.4?) Alternatively this document can be made to
provide the IANA specification for the R flag and the erratum closed.
The WG decided to keep the definition of the R Flag in RFC 6426 and
not here — consequently, there’s little that can really be done as the
erratum (which really is symbolic since the IANA registry is fixed)
applies to RFC 6426 and not to RFC 4379.
OK
s2.1/s6: An update to
http://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml
is needed to replace RFC 4379 with RFC-to-be for special exceptions
to usage rules.
Done.
s3.5, Clandestine Channel via Pad TLV: As specified the value part
of a Pad TLV can serve as a clandestine channel since the value
field contents are ignored.
Added the following to S5:
The value part of the Pad TLV contains a variable number of octets.
With the exception of the first octet, these contents, if any, are
ignored on receipt, and can therefore serve as a clandestine channel.
Fine.
s3.5, Usefulness of Pad TLV: Could you explain circumstances in
which a Pad TLV would be needed please. I can't see any at present.
Sure — when you want to send pings of various sizes for
troubleshooting. I’ve used it in productions :-)
I think a short note in s3.5 about why it might be present would be
useful. e.g.,
The Pad TLV can be added to an Echo Request to create a message
of a specific length
in cases where pings of various sizes are needed for
troubleshooting.
Nits/editorial comments:
======================
Thank for for these. Unless I make a specific follow-up inline, the
nit is fixed.
There are a couple of places where this doesn't seem to be the case,
especially in a number of places where there are SHOULDs in the text but
the reasons for/consequences of not following the SHOULD are not spelt
out. This can be important for interoperability.
General: s/i.e. /i.e., / (two instances s3.2, last para; s4.5.1, para 3)
s1, para 1: s/methods of reliable reply/methods of providing reliable
reply/
s1.4, bullet 4: Need to expand acronym PW on first use.
s1.4, bullet 4: need to move expansion of FEC acronym to here from s2.
s1.4, bullet 8: Acronyms DSMAP/DDMAP: When defining Return Code 14
in s3.1, the text is 'See DDM TLV...'. DDM is not expanded anywhere
although it is clearly the same as DDMAP. But has by now made it
into the IANA repository and is probably better to use it for
'Downstream Detailed Mapping', so I suggest:
OLD:
o Incorporate the updates from RFC 6424, by deprecating the
Downstream Mapping TLV (DSMAP) and adding the Downstream Detailed
Mapping TLV (DDMAP), updating two new return codes, updating the
procedures, IANA section, Security Considerations, and obsoleting
RFC 6424.
NEW:
o Incorporate the updates from RFC 6424, by deprecating the
Downstream Mapping TLV (DSM) and adding the Downstream Detailed
Mapping TLV (DDM), adding two new return codes, updating the
procedures, IANA section, Security Considerations, and obsoleting
RFC 6424.
END
Then s/DSMAP/DSM/g, s/DDMAP/DDM/g in the rest of the document.
This is a good point, where DDM came from RFC 6424. However, these
fields are known as DSMAP and DDMAP.
Consequently, global replacing DDM -> DDMAP
That's OK by me. I wondered whether the DDM in the return code had been
too firmly ensconced to change.
s1.4: Ought to mention the addition of the motivation (LSP
stitching) for the additions in RFC 6424.
s2.1, paras 7 and 8: This contains "the newly designated IPv4 link
local addresses". Given that RFC 3927 is now over 11 years old, the
qualifier is no longer appropriate, but it might be useful to provide
a ref. Thus:
OLD:
the newly designated IPv4 link local addresses
NEW:
the IPv4 link local addresses [RFC3927]
END
The text in para 8 is also no longer appropriate. Suggest
OLD:
Furthermore, the IPv4 link local address range has only recently been
allocated. Many deployed routers would forward a packet with an
address from that range toward the default route.
NEW:
Older deployed routers may not correctly implement link local
addresses
and would forward a packet with an address from that range toward the
default route.
END
Yes, many thanks. Updated with a slight change “Older deployed routers
may not (correctly) implement IPv4 link local addresses …"
s2.1, para 9: s/embedded in as/embedded in an/
s2.1, para 9: Useful to add a reference to RFC 4291.
This didn't happen. Not essential but useful.
s2.2, para 1: To be clearer about the distinction between IPv4 and
IPv6, suggest:
OLD:
This document requires the use of the Router Alert Option (RAO) set
in IP header in order to have the transit node process the MPLS OAM
payload.
NEW:
This document requires that the Router Alert Option (RAO) is carried
in the IP header in order to have the transit node process the
MPLS OAM
payload. For IPv4 packets the RAO [RFC2111] MUST be added to the
IPv4
header; for IPv6 packets a hop-by-hop RAO [RFC2711] must be
chained to
the IPv6 header.
END
I wanted to keep that paragraph IP version agnostic, since the
specifics for IPv4 and IPv6 come in the next two paragraphs.
Fair enough.
s3, para 1:
An MPLS echo request is a (possibly labeled) IPv4 or IPv6 UDP packet;
This format applies to both requests and responses but the response
case is not made explicit. Suggest:
OLD:
An MPLS echo request is a (possibly labeled) IPv4 or IPv6 UDP packet;
NEW:
An MPLS LSP ping message, is a (possibly labeled) IPv4 or IPv6 UDP
packet;
END
That would leave out “traceroute” mode. I’ll add “request/reply"
OK
s3, main packet format and associated text: The Sender's Handle is
not the packet sender's handle but the Echo Request Sender's Handle -
it is copied in to the corresponding Echo Reply. Suggest renaming
the Sender's Handle and Sequence Number to Echo Request Sender's
Handle and Echo Request Sequence Number. This would affect para 5 of
s4.3, para 2 of s4.5 and para 1 of s4.6 also.
That would be too big of a departure for very well-known fields.
I just realized that the draft doesn't say that the Sequence Number is
also (I assume) returned unchanged in the Echo Reply. To emphasize this
copying, it would probably be good to use the MUST word about both
request -> reply copies.
s3, Timestamp format: RFC 5905 allows for 3 different time formats -
the 32 bit basic format is intended:
OLD:
The TimeStamp Sent is the time-of-day (according to the sender's
clock) in NTP format [RFC5905]
NEW:
The TimeStamp Sent is the time-of-day (according to the sender's
clock) in 32 bit NTP format [RFC5905]
END
64-bit.
I changed to “64-bit NTP Timestamp format”.
Yes.. oops.
s3, Global flags: Technically, this doc only defines the V flag:
Also forcing the other bits to be zero restricts addition of new flags>
OLD:
This document defines three flags, the R, T, and V bits; the rest
MUST be set to zero when sending and ignored on receipt.
NEW:
At the time of writing three flags are defined, the R, T, and V
bits; the rest
SHOULD be set to zero when sending and ignored on receipt.
END
I changed the first part but leave in the MBZ.
OK
s3, TLV types: The values 4, 6 and 8 for TLV type and the value 5 for
Tthe sub-type of TLV type 1 are specified as 'Not assigned': To be
clear for the future, should these really be marked as 'Reserved' or
could they be assigned in future (and hence s/b marked as 'Available
for assignment')?
They are not assigned. IANA now calls these as Not Assigned as
“Unassigned” — updated..
Fine. I was just wondering.
s3: For clarity it would be useful to add a sentence to the end of
the section stating:
In Sections 3.2 - 3.4 and their various sub-sections, only the value
section of the TLV is specified.
Sure. But it’s really from 3.2 through 3.9.
True!
As part of this, I also cleaned up all the “[sub][-]section” citations.
Good.
s3, TLV length calculation: This is shown by example only. I think
it ought to be explained explicitly in text. I suggest:
The length of a sub-TLV or a TLV whose value is not a list of
sub-TLVs
is the number of significant octets in the value part of the
(sub-)TLV
excluding any final padding. If the value of a TLV is a list of
sub-TLVs,
the length of the TLV is the sum of the overall lengths of the
sub-TLVs
including the sub-TLV header and the length of the padding, i.e.
4 + ((sub-TLV.length + 4) mod 4)
The examples are clear enough and have been clear throughout many
implementations.
The market may go down as well as up! I'll have to live with this.
s3.1, para 1: I think this should be interpreted as saying that the
Return Code MUST always be zero in an Echo Request and the Return
Code is set to an appropriate one of the possible values in an Echo
Reply. To be clear: I take it that it would not be normal for an
Echo Reply to carry a zero Return Code. Assuming this is right...
OLD:
The Return Code is set to zero by the sender. The receiver can set
it to one of the values listed below.
NEW:
The Return Code MUST be set to zero in an Echo Request message.
The responder sets the Return Code in the Echo Reply message to
an appropriate value other then zero from the list below.
END
Current text is OK.
Hmm. I found this potentially confusing. This was the main point at
which I thought the use of sender and receiver needed clarifying. Am I
right in thinking that the 'sender' is always the node sending an Echo
Request and the 'receiver' is a node that is triggered by the Echo
Request to send an 'Echo Reply'. The issue for me is that the receiver
also sends messages. How about:
OLD:
The Return Code is set to zero by the sender. The receiver can set
it to one of the values listed below.
NEW:
The Return Code is set to zero in the Echo Request message by the
(Echo Request) sender.
The ((Echo Request) receiver can set it to one of the values listed
below in the corresponding
Echo Reply that it generates.
END
It is possible that I am misinterpreting sender or receiver.. but if so
I would say that some additional words would help.
s3.1, Return code 14: Some of the extra text from Section 3.2.1 of
RFC 6424 ought to be essential as it contains 'MUSTS'. Suggest
adding this as an extra note against Return Code 14:
Note 2:
Return Code 14 is used to indicate that an Echo Reply contains
one or more
DDM TLVs (see Section 3.4). In this case there will be one
Return Code and
corresponding <RSC> for each path described and these are
passed in the
DDM TLV(s). This Return Code MUST only be used in the Echo
Reply message
header and MUST NOT be used in the Echo Request message even if
the message
contains a DDM TLV.
Sure, added different text (above is incorrect), but this is a really
good point. Updated also a section citation to point to this note.
Thew new text is fine.
s3.1: The term IS_EGRESS is used later in the document to indicate
an Echo Reply message with a Return Code of 3. It should defined
here. The meaning is fairly obvious at its first use in s3.4(e) but
there is not a formal definition. (AFAICS textual acronyms are not
used for any of the other codes.)
This didn't happen.
s3.2, last but one para: s/previx/prefix/
s3.2.8/s3.2.9/s3.2.11: It would be useful to use the name of the FEC
type from RFC 4447 (PWid FEC) rather than just its number. (Also in
A.1.1).
The names are wildly used, and citations to 4447 exist. I’ll leave it
as is.
Wild!
s3.2.9: s/sender's PE IPv4 address/Sender's PE IPv4 Address/;
s/remote PE IPv4 address/Remote PE IPv4 Address/
OK, same for the Appendix and IPv6 PE addresses.
s3.2.9, para 3: Need to expand PE acronym on first use.
s3.2.10, para 1: The text uses source PE IPv4 address whereas the
diagram uses Sender's PE IPv4 Address. Consistency is needed. See
also the previous comment regarding consistency and capitalization.
This is explained:
Sender's Provider Edge (PE) IPv4 Address (the
source address of the targeted LDP session),
OK. I think I read the text incorrectly here. On reflection it looks
fine as is.
s3.2.10/s3.2.12: : It would be useful to use the name of the FEC type
from RFC 4447 (Generalized PWid FEC) rather than just its number.
s3.2.12: The text uses source whereas the diagram and field name use
Sender's... consistency again?
s3.4, DS Flags:
I Interface and Label Stack Object Request
When this flag is set, it indicates that the replying
router SHOULD include an Interface and Label Stack
Object in the echo reply message.
What circumstances would cause the replaying router not to do this?
What should it do otherwise?
This hasn't been addressed AFAICS.
s3.4, Return Code:
The Return Code is set to zero by the sender. The receiver can
set it to one of the values specified in the "Multi-Protocol Label
Switching (MPLS) Label Switched Paths (LSPs) Ping Parameters"
registry, "Return Codes" sub-registry.
a) I suspect that in the basic LSP ping described in this document,
the return codes that ought to be available are only those specified
in s3.1 of this document except for 14 (which is specifically only
allowed in the header). The registry now contains a number of other
return code values but a basic implementation wouldn't understand
them in general.
b) See the previous comments on meaning of sender and receiver. Suggest:
OLD:
The Return Subcode is set to zero by the sender. The receiver can
set it to one of the values specified in the "Multi-Protocol Label
Switching (MPLS) Label Switched Paths (LSPs) Ping Parameters"
registry, "Return Codes" sub-registry.
NEW:
The Return Code in the (one) DMM TLV in an Echo Request message
MUST be set to zero. The responder sets the Return Code in any
DMM TLV in the Echo Reply message to an appropriate value other
then zero or 14 ("See DDM TLV for Return Code and Return
Subcode")
taken from the list in Section 3.1.
END
Similar issue with the Subcode (you are mixing RC with RSC in the
OLD/NEW).
Ah! Actually the original text and what I wrote are both wrong! The
text in -08 is arguably not wrong, but is confusing because it looks
like the instructions for Return Code.
VERSION 08 TEXT:
The Return Subcode is set to zero by the sender. The receiver can
set it to an appropriate value as specified in Section 3.1. This
field is filled in with the stack-depth for those codes that
specify the stack-depth. For all other codes, the Return Subcode
MUST be set to zero.
MORE NEWER:
The Return Subcode is set to zero by the sender. The receiver
can [MUST?]
fill this field with the stack-depth for those codes that
specify the stack-depth as indicated in Section 3.1 For all
other codes, the Return Subcode
MUST be set to zero.
s3.4, Sub-tlv Length: I think that the components of the DSM are all
multiples of 4 octets long so there is no padding to consider (apart
from possibly in FECs ).
OLD:
Total length in bytes of the sub-TLVs associated with this TLV.
NEW:
Total length in octets of the sub-TLVs associated with this
TLV including the TLV headers and any padding.
END
Leaving this does not hurt — however, fixed the bytes -> octets
throughout.
Octets: good. I'll live with the rest.
s3.4.1.3, FEC TLV length: Does this include any trailing padding and
the TLV header?
s3.4.1.3, Operation Rules: Shouldn't these be in s4?
Thinking about this some more, I see these aren't operation rules in the
same sense as s4 uses operation. They are actually ordering rules:
OLD:
FEC stack change sub-TLV operation rules are as follows:
NEW:
When a DDMAP TLV needs several FEC stack change sub-TLVs to record
the changes
that the LSR makes to the label stack, the following number and
ordering rules MUST
be respected:
END
s3.6: Should contain a reference to the IANA registry URL.
Sure, why not :-)
s4.1, last para: s/some information how each/some information as to
how each/
s4.2: s/to differentiate whether/to ascertain whether/
s4.3, para 1: s/MUST be set in IP header/MUST be set in appropriate
IP options/
s4.4, item 1: It would be helpful to remind implementers how TLVs are
marked to be ignored:
OLD:
If there are any TLVs not marked as "Ignore"
NEW:
If there are any TLVs not marked as "Ignore" (i.e., if the TLV type
is less than 32768, see Section 3)
END
s4.4: s/subsection/Section/g
s4.4, item 3: s/If there is no entry for L {/If there is no entry for
Label-L {/
s4.4, item 4:
OLD:
Set Best-return-code to Return Code 9, "Label switched
but no MPLS forwarding at stack-depth" and set Best-rtn-
subcode to Label-stack-depth and goto Send_Reply_Packet.
NEW:
Set Best-return-code to Return Code 9, "Label switched
but no MPLS forwarding at stack-depth" and set Best-rtn-
subcode to Label-stack-depth and goto step 7 (Send
Reply Packet).
END
s4.4.1, item 5: s/advertise FEC/advertise the FEC/
s4.5:
If the replying router is the destination of the FEC, then Downstream
Detailed Mapping TLVs SHOULD NOT be included in the echo reply.
Under what circumstances might one be included? I think this is a
MUST NOT.
See comment on s3.4 at head of message.
s4.5.2: This section is derived from s4.1.2 of RFC 6424. Whilst the
new version appears to contain sufficient to define the proper
normative behaviour, RFC 6424 contains additional examples of usage.
These look useful to me. I wonder if it might be useful either to
copy the illustrative material to an appendix or maybe point back to
RFC 6424. I am not sure how the powers-that-be would consider back
pointers to obsoleted documents! Maybe something like:
[RFC6242] which originally specified the techniques needed to
support tunnel transition contains some
examples, in Section 4.1.2, of situations where the technique
would be applied.
This was discussed and decided did not want to over copy when the
current text is enough.
OK
s4.6:
If the echo reply contains Downstream Detailed Mappings, and X wishes
to traceroute further, it SHOULD copy the Downstream Detailed
Mapping(s) into its next echo request(s) (with TTL incremented by
one).
Presumably this means one DMM per Echo Request... might be worth
being more explicit.
s5: Security risks of Router Alert. Mention RFC 6398 and maybe copy
2nd para of s6 of RFC 7506.
I believe the RA usage (which is specific and not generic) is
adequately covered.
I was thinking of whether (e.g.) having a RA option set in a packet
other than a MPLS Echo Request/Response could be a way of doing a DoS
attack on an MPLS network since it could overload the data->control
plane link... might wish to filter any packets with RA set at ingress to
MPLS network. Not sure if the UDP port rate limit would help with this.
s5, Security risks of DoS using Errored TLV?
Injecting MPLS Ping packets with bad TLVs could be a way of creating a
DoS perhaps as it would produce Errored TLV messages.
s6: Given the responses from IANA, a note is needed to say that
entries originated other than from RFC 4379 should remain unaltered
in the registry. The only exception might be the R flag in Global
Flags where it might be sensible to use this document to fix erratum
4012.
s6.2.5, last line: Remove ']]' which appears to be spurious.
s8: Several new references are mentioned in these comments and would
need to be added if the suggestions are actioned.
Very many thanks again for the review!
— Carlos.
_______________________________________________
Gen-art mailing list
Gen-art@ietf.org
https://www.ietf.org/mailman/listinfo/gen-art