Re: [spring] AD Review of draft-ietf-spring-segment-routing-ldp-interop-09

[Ahmed Bashandy <[e-mail]>]

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: 	[e-mail]
> Resent-To: 	[e-mail], [e-mail], 
> [e-mail], [e-mail], 
> [e-mail]
> Date: 	Wed, 20 Dec 2017 13:36:54 -0500
> From: 	Alvaro Retana <[e-mail]>
> To: 	[e-mail]
> CC: 	[e-mail], Martin Vigoureux 
> <[e-mail]>, [e-mail]
>
>
>
> 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