Hi,

thanks a lot for the thorough review. I uploaded version 11 to address your comments. See my reply inline "#Ahmed".

Thanks

Ahmed


On 4/2/18 7:26 AM, Ahmed Bashandy (bashandy) wrote:




-------- Forwarded Message --------
Subject:        AD Review of draft-ietf-spring-segment-routing-ldp-interop-09
Resent-Date:    Wed, 20 Dec 2017 10:37:01 -0800
Resent-From:    alias-boun...@ietf.org
Resent-To: cfils...@cisco.com, stef...@previdi.net, basha...@cisco.com, bruno.decra...@orange.com, stephane.litkow...@orange.com
Date:   Wed, 20 Dec 2017 13:36:54 -0500
From:   Alvaro Retana <aretana.i...@gmail.com>
To:     draft-ietf-spring-segment-routing-ldp-inte...@ietf.org
CC: spring-cha...@ietf.org, Martin Vigoureux <martin.vigour...@nokia.com>, spring@ietf.org



Dear authors:

I just finished reading this document.  I have some Major comments below that I would like to see addressed before starting the IETF LC.

Thanks for your work on this document.

Alvaro.


Major:

M1. From Section 2: "An MCC, operating at node N, MUST ensure that the incoming label it installs in the MPLS data plane of Node N has been uniquely allocated to himself.”  I’m sure this sentence is not meant as a new Normative statement for all MCCs, right?  I think that the “MUST” is out of place since the text is really stating a fact.  s/MUST/must
#Ahmed: done


M2. SRMS Definition and Operation

M2.1. Section 4.2.1. (SR to LDP Behavior) uses normative language to describe the operation of the SRMS in ways that I think are not needed for interoperability.

M2.1.1. "The SRMS MUST be configured by the operator in order to advertise Node-SIDs on behalf of non-SR nodes.”  Section 4.2 already says that "The mappings advertised by one or more SRMSs result from local policy information configured by the operator.”, so the sentence in 4.2.1 is at best redundant. In any case, what can be enforced from a specification point of view by that “MUST”?  s/MUST/must

M2.1.2. "At least one SRMS MUST be present in the routing domain.  Multiple SRMSs SHOULD be present for redundancy.”  These MUST|SHOULD seem to indicate a statement of fact.  Again, from a specification point of view, what can be enforced?  s/MUST|SHOULD/must|should. Note also that in 7.2 the text says that "Multiple SRMSs can be provisioned in a network for redundancy.”, which seems to be the right thing (no Normative language).
#Ahmed
I removed these two statements. Instead I modified the 3rd paragraph in Section 4.2 to indicate that there must be at least one SRMS server in the domain


M2.2. Section 7.2. says that "a preference mechanism may also be used among SRMSs so to deploy a primary/secondary SRMS scheme”…but no other details are included.  This document is where the SRMS is first defined, so I would expect this detail to also be included here.  I note that Section 3.1. (SID Preference) of draft-ietf-spring-conflict-resolution contains the preference specification. Please move that section to this document.
Ahmed: agreed. But since section 7.2 is under the manageability consideration, IMO it should really not contain much specification. Instead, I modified section  4.2.2 specify how to prefer SRMS advertisements and removed section 7.2 completely. Section 7.2 in version 11 is section 7.3 in version 10

M2.3. Section 7.2 also says that: "When the SRMS advertise mappings, an implementation SHOULD provide a mechanism through which the operator determines which of the IP2MPLS mappings are preferred among the one advertised by the SRMS and the ones advertised by LDP.”  First off, I think that the SHOULD is out of place because it is not specifying any specific action (the mechanism is not explicit). Second, this statement (with the Normative SHOULD) is in conflict with (from 2.2. (IP2MPLS co-existence)): "if both LDP and SR propose an IP to MPLS entry (IP2MPLS) for the same IP prefix, then the LDP route SHOULD be selected.”  Solution: s/SHOULD/should
#Ahmed:
Agreed. I replaced SHOULD with should


M3. Manageability Considerations

M3.1. The text in Section 7.1. (SR and LDP co-existence) is almost the same as in Section 2.2. (IP2MPLS co-existence); the difference is that 7.1’s first bullet says that "by default the LDP route MUST be selected”, while 2.2 uses SHOULD instead.  Which one is it?  Obviously, having the same text is two places adds nothing to the document — please consolidate.

M3.2. [minor] The last bullet in 7.1/2.2 says that the "policy MAY be locally defined.  There is no requirement that all routers use the same policy.”  Given that in this case “all routers” really refers to the edge nodes (at the IP2MPLS boundary), it seems like it makes sense that either choice could be ok.  Maybe I’m wrong, but I’m guessing that giving preference to LDP (MUST/SHOULD above) has to do with the assumption that it is supported everywhere, while SR might not yet be…so it supports the migration case in Section 3. Is that a reasonable guess?  It would be nice, to provide some justification for the default LDP preference so that operators have a better idea of when it might be ok to use SR instead.
#Ahmed: I removed section 2.2 as it is very similar to section 7.1 as you correctly pointed out. for IP2MPLS, I referred to Section 7.1. I also added justification for preferring LDP over SR as you suggested and changed the MUST to SHOULD


M4. Security Considerations.  I tend to agree that this document doesn’t introduce anything new…but it does specify something different.  The base SR-related advertisement by an IGP is done for the segments belonging to the local node, but the SRMS lets a node (any node, multiple nodes) adverse any mapping (for nodes that may be anywhere in the network) which may result in conflicting advertisements (in the best case), or even false ones. Cryptographic authentication (any any other current security mechanisms in IGPs) only verify that the information was not changed, but it doesn’t validate the information itself, which can then lead to conflicting and or false advertisements, which could “compromise traffic forwarding”.  You should at least recognize that the risk exists, even if no specific mitigation (except maybe strict configuration/programmability control by the operator) can be mentioned.
#Ahmed: Agreed. Added text to indicate what you mentioned


M5. References: These references don’t need to be Normative and can be made Informative: I-D.ietf-isis-segment-routing-extensions, I-D.ietf-ospf-ospfv3-segment-routing-extensions, I-D.ietf-ospf-segment-routing-extensions. OTOH, this one should be Normative: RFC5036
Ahmed: Agreed and done


Minor:

P1. As with all the other SR-related documents, please take out “service chain” from the text.
#Ahmed: Agreed and done

P2. Please add References for "RSVP-TE, BGP 3107, VPNv4”.   BTW, note that rfc3107 has been obsoleted by rfc8277 — you make references to “BGP3107” routes/label.
#Ahmed Removed VPNv4 and added references to the others

P3. Please define (or reference) the MPLS2MPLS, MPLS2IP and IP2MPLS terminology (only IP2MPLS is expanded).
Ahmed: Done

P4. From Section 3: “...the SR infrastructure is usable, e.g. for Fast Reroute (FRR) or IGP Loop Free Convergence to protect existing IP and LDP traffic.  FRR mechanisms are described in [I-D.bashandy-rtgwg-segment-routing-ti-lfa].”  draft-ietf-spring-resiliency-use-cases may be a better reference.
#Ahmed: Agreed and modified as suggested

P5. Section 3: "However, any traffic switched through LDP entries will still suffer from LDP-IGP synchronization.”  While that statement is true, it seems out of place since there is no other discussion about LDP-IGP synchronization anywhere — if you want to keep it, please add a reference.
#Ahmed: Agree with your assessment and removed the statement

P6. Sections 4 and 4.1.1 have very similar, redundant text.  To avoid confusion, please consolidate it in one place.  This is what I’m referring to:

4:
“  If the SR/LDP node operates in LDP ordered label distribution control
   mode (as defined in [RFC5036]), then the SR/LDP node MUST consider SR
   learned labels as if they were learned through an LDP neighbor and
   create LDP bindings for each Prefix-SID and Node-SID learned in the
   SR domain."

4.1.1:
"  A SR node having LDP neighbors MUST create LDP bindings for each
   Prefix-SID and Node-SID learned in the SR domain and, for each FEC,
   stitch the incoming LDP label to the outgoing SR label.  This has to
   be done in both LDP independent and ordered label distribution
   control modes as defined in [RFC5036]."
#Ahmed: Agree with your assessment and consolidated the text in section 4 and 4.1.1 into section 4.1.1

Note that this same text (as in 4.1.1 above) is repeated exactly in 4.2.1 — where the SRMS is discussed.  To me, it seems out of place there (4.2.1) as the behavior is true whether an SRMS is in use or not.  In line with the above, it may be better to consolidate redundant text in one place — Section 4 seems good to me.
#Ahmed: Agree with your assessment and removed the statement in Section 4.2.1



Nits:

N1. s/This draft(s)/This document

N2. s/Ship-in-the-night/Ships-in-the-night

N3. Please don’t use “we”, use the 3rd person instead.  Just a personal preference (= nit).

N4. s/R-LFA/RLFA

N5. Please put the reference for Option C when it is first mentioned.
#ahmed: Made the changes as suggested

_______________________________________________
spring mailing list
spring@ietf.org
https://www.ietf.org/mailman/listinfo/spring

Reply via email to