Subject: Re: [mpls] Updated rev -08 posted [Re: Benjamin Kaduk's Discuss on draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 17 Mar 2020 13:55:53 -0700

On Fri, Feb 28, 2020 at 04:39:14AM +0000, Kamran Raza (skraza) wrote:
> Thanks for your detailed review, Ben. 
> We have posted a new rev -08 that takes care almost all of your comments and comments from other IESG reviewers. 
> On behalf of authors, Please see inline [skraza] for each of your comment/Q:
> 
>  
>     ----------------------------------------------------------------------
>     DISCUSS:
>     ----------------------------------------------------------------------
>     
>     The YANG doctor last call review of the -06 notes that these modules are
>     in violation of the expectations of RFC 8349 (at least w.r.t. defining a
>     new identity for the control-plane protocol and possibly more).  This
>     document should either be brought into compliance or explain why it
>     diverges.
> 
> [skraza]: Fixed.  
>     
>     The YANG doctor's comments about default values for "local"
>     configuration (e.g., graceful-restart configuration) causing the global
>     configuration to never take effect should also be addressed.
> 
> [skraza]: Fixed. 

These both look addressed, to my eye, but I am very much not a YANG expert.
I'd recommend trying to get a dedicated YANG review, perhaps from the
incoming Management AD Rob Wilton.

>     Please include some justification for why LDP IPv6 is considered an
>     "extended feature" (which is particularly surprising given that Section
>     2 classifies "IP" to refer to both IPv4 and IPv6 together).
>     
> [skraza]:  This was an early decision taken and presented to WG and WG had agreed.
> Moreover, Sec 1.1 does points out why LDP IPv6 is considered as an extended feature. The "base" features are the one most commonly implemented and deployed features.
> The idea was that base must not contain any if-feature - given that LDPv6 is not widely implemented and was branded as if-feature, it is classified in the extended category then.
> BTW, note that LDP has separate RFC for LDP IPv6. 

It's not really clear to me that just because the protocol is described in
separate RFCs for IPv4 and IPv6 we cannot present a unified management
interface on top of it.  I note that several other ADs expressed concern
about this topic, so I've moved this document onto the agenda for this
week's IESG telechat to get a better sense on what the right thing to do is
here.

> 
>     We need to define the format/semantics of the md5-key string (e.g., is
>     it hex? base64{url,}?) either directly or by reference (as the YANG
>     doctor notes).  Using a crypto-type would probably be appropriate, as
>     would adding a note that tcp-md5 is obsoleted by TCP-AO.
>     
> [skraza]: It is beyond our control as we are using it as-is from the RFC. Made the change anyways to add crypto-algo to widen the scope/type of key beyond MD5.

I'm still concerned that there isn't enough text here for me to be able to
implement something and have confidence it will interoperate.  For example,
the recommendations in RFC 3562 include the ability to use a direct binary
key for TCP-md5.  Does this YANG stanza allow for doing so?  If yes, what
mechanism is used for converting input into "raw" binary, if needed?

That is, even if the actual protocol semantics for how TCP-MD5 uses a given
key/password are out of your control, there is still ambiguity as to what
mapping (if any) occurs between the YANG-configured value and what is
passed to the TCP-MD5 implementation.

Furthermore, the authentication-type still only has a "string" leaf for the
key, with (if I am reading the YANG correctly) the recommended key-chain
functionality for protected key storage being left as an extension.  It's
not clear to me why we should be leaving the best-practice formulation as
an extension.

>     In the peer-af-policy-container grouping's
>     label-policy/advertise/prefix-list, we need to say if the filter is for
>     inclusion or exclusion of outgoing label advertisements.
>     Similarly for the incoming label advertisements in the 'accept'
>     container (and most of the <foo>-list-ref usages?).
>     
>     In the policy-container grouping's
>     label-policy/assign/independent-mode/prefix-list, the description
>     suggests that the contents will provide not just a list of prefixes that
>     act as a filter, but also a map from prefix to label.  This is a
>     qualitatively different usage than the previous occurrences of
>     prefix-list-ref, and it seems like a different type may be needed for
>     it.  Similarly for ordered-mode.
>     
> [skraza]: Fixed as per latest changes that use rtgwg policy model constructs.

Ah, this took me a little more work to find, since the changes were in the
prefix-list-ref typedef.  Thanks.

>     (pro-forma) This document has 6 authors, and per RFC 7322, as this is
>     more than five, we are supposed to consider whether that's appropriate.
>     Seeing nothing in the shepherd writeup or similar, I mention it here.
>     
> [skraza]: We had taken an advise from Loa (MPLS WG chair) and would like to keep it to 6.

Sure; this was presumed to just be "pro forma" but I put it in since the
shepherd writeup didn't mention it specifically.

>     ----------------------------------------------------------------------
>     COMMENT:
>     ----------------------------------------------------------------------
>     
>     Please also respond to the other YANG doctor, rtg-dir, and other
>     directorate reviewers' comments.
>     
>     I support Roman's Discuss.
>     
>     I suggest making a reference to RFC 5920 at some point, perhaps from the
>     security considerations.
> [skraza]: Done.
>     
>     We have several instances of a "container ipv4" that's a "container with
>     presence" in the jargon of RFC 7950.  As SEC AD one thing I look for is
>     edge cases, such as when a subset of these containers are present but
>     not all (and not none).  While for this specific case, it seems fairly
>     natural to assume that IPv4 is enabled if any are present, it does make
>     me wonder if the modeling is at an optimal state if there's even the
>     possibility for such skewed signals.  (I didn't specifically audit for
>     any other cases of "redundant" containers-with-presence.)
>     
> [skraza]: This is the same structure as RFC 8344. The edge cases are OK. The ipv4/ipv6 container may contain none, one, or more nodes in the container. ipv4/ipv6 address family is enabled as long as the container is “present”. Your assumption is correct, and the modeling (and the protocol as well) is in a good state, even with such possibilities. The signals should be clear enough.
> 
>     It's not entirely clear to me why we need (all of) the specialization of
>     structures into using inet:ipv4-address and inet:ipv6-address as opposed
>     to just inet:ip-address.  I do see that the latter is used in several
>     places already, and so assume that this was already considered; perhaps
>     some of the discussion/motivation could be included in the document,
>     though.
>     
> [skraza]: When the type of an attribute is for a specific address family (ipv4 or ipv6), we use the address family specific type (inet:ipv4-address or inet:ipv6-address), so that user cannot provide a value of the wrong format.  inet:ip-address is simply a union of net:ipv4-address and inet:ipv6-address, and is used when the address family is not restricted. This is particular convenient when the user makes a query on a particular peer when the user does not have a particular address family in mind. The system will provide the address in a proper format.
> 
>     Section 1
>     
>        The data model is defined for following constructs that are used for
>        managing the protocol:
>     
>     nit: "the following".
> [skraza]: Fixed.    
> 
>     I share the YANG doctor's concern that separating the configuration and
>     operational state constructs is not consistent with the NMDA model.
> [skraza]: Fixed.
>     
>     Section 1.1
>     
>        The "base" category contains the basic and fundamental features that
>        are covered in LDP base specification [RFC5036] and constitute the
>        minumum requirements for a typical base LDP deployment.  Whereas, the
>        "extended" category contains all other non-base features.  All the
>     
>     This statement ("all other") is not future-proof.
> [skraza]:  Removed the "all".
>     
>     Section 3
>     
>        o  This module aims to address only the core LDP parameters as per
>           RFC specification, as well as some widely deployed non-RFC
>           features (such as label policies, session authentication etc).
>           Any vendor specific feature should be defined in a vendor-specific
>           augmentation of this model.
>     
>     [Even for widely-deployed non-RFC features we still need to provide a
>     clear description of what semantics are being covered, whether
>     in-document or via an external reference.]
>     
> [skraza]: The only non-RFC config was related to label policies - Reworked/enhanced the text.
> 
>        o  This model is a VPN Forwarding and Routing (VRF)-centric model.
>           It is important to note that [RFC4364] defines VRF tables and
>     
>     side note: I would like the IETF to someday get to a point where "VPN"
>     (the 'P' is for "private") is used only for protocols/deployments that
>     provide confidentiality protection ("privacy") for the data being
>     conveyed.  We are, of course, not there now, and I cannot insist on any
>     change here.  But please consider whether an alternate phrasing would
>     suffice, that includes just the "virtual" part but not "VPN".  Since we
>     are mostly using "VRF" in the rest of the document, this is not as
>     daunting as it sometimes is...
>     
>     Section 4
>     
>     It might be nice to have the tree show the ipv4 and ipv6 children in
>     as similar an order as is possible (with the understanding that the
>     semantics are not affected by the order of appearance, of course).
>     
>            |  +--rw address-families
>            |  |  +--rw ipv4!
>            |  |  |  +--rw enable?                           boolean
>            |  |  |  +--ro label-distribution-controlmode?   enumeration
>            |  |  |  +--ro bindings
>            |  |  |  |  +--ro address* [address]
>            |  |  |  |  |  +--ro address               inet:ipv4-address
>            |  |  |  |  |  +--ro advertisement-type?   advertised-received
>            |  |  |  |  |  +--ro peer
>            |  |  |  |  |     +--ro lsr-id?           leafref
>            |  |  |  |  |     +--ro label-space-id?   leafref
>     
>     Does this imply that there is only one peer per address?  Is that going
>     to be a limitation for any deployment cases?
> 
> [skraza]: Not an issue. Note that a peer may have one or more bound addresses. 
>     
>               +--rw ldp-ext:session-downstream-on-demand
>               |       {session-downstream-on-demand-config}?
>               |  +--rw ldp-ext:enable?      boolean
>     
>     nit(?): missing '|' to connect down to ldp-ext:enable?
>     Similarly for ldp-ext:max-wait a few lines later, and some other
>     occurrences later in the document.
> 
> 
> [skraza]: Some of these instances/typos were due to hand picked sub-tree. 
> Not an issue anymore as we are printing the entire tree as generated by the tool.
> 
>     
>     Section 5.1.2
>     
>     Am I reading this right that (e.g.) per-interface hello-holdtime is
>     configurable as an extension and only the global hello-holdtime is
>     present in the "base" module?
> 
> [skraza]: Yes.
>     
>     Section 7
>     
>     "NHLFE" is used only once in this document (and is not listed as
>     "well-known" at
>     https://www.rfc-editor.org/materials/abbrev.expansion.txt); please
>     expand it out.
>     
> [skraza]: It is well known and listed in the above url. Please x-chk.

To be clear, the linked content uses the convention of an "*" marking to
indicate that the expansion is "well-known" and does not need to be
provided in a given document.  The entry for "Next Hop Label Forwarding
Entry" does not include such a marking; perhaps the responsible AD should
request it to be added, if it is indeed well-known?

>     Section 9
>     
>     Is 2018 still the right copyright year for the modules themselves?
> 
> 
> [skraza]: 2020-02-25 rev now.
> 
>     
>     Section 9.1
>     
>          typedef ldp-address-family {
>            type identityref {
>              base rt:address-family;
>            }
>     
>     Why can't we use rt:address-family directly in the one container that
>     uses this?
>     
> [skraza]: Fixed now.
> 
>            description
>              "Duration represented as 32 bit seconds with infinite.";
>     
>     nits: "32-bit" (hyphenated), "with infinity".
> [skraza]:  Fixed.    
> 
>          typedef downstream-upstream {
>            type enumeration {
>              enum downstream {
>                description "Downstream information.";
>              }
>              enum upstream {
>                description "Upstream information.";
>              }
>            }
>            description
>              "Received or advertised.";
>          }
>     
>     nit: copy/paste on the description?
> [skraza]:  Fixed.        
> 
>              leaf used-in-forwarding {
>                type boolean;
>                description
>                  "'true' if the lable is used in forwarding.";
>     
>     nit: s/lable/label/
> [skraza]:  Fixed.        
>     
>          grouping graceful-restart-attributes-per-peer {
>            description
>              "Per peer graceful restart attributes.
>               On the local side, these attributes are configuration and
>               operational state data. One the peer side, these attributes
>               are operational state data reveiced from the peer.";
>     
>     nit: s/reveiced/received/
> [skraza]:  Fixed.        
>     
>          grouping peer-state-derived {
>            description
>              "The peer state information derived from the LDP protocol
>               operatoins.";
>     
>     nit: s/operatoins/operations/
> [skraza]:  Fixed.            
> 
>          grouping peer-state-derived {
>          [...]
>            leaf next-keep-alive {
>              type uint16;
>              units seconds;
>              config false;
>              description "Time to send the next KeepAlive message.";
>            }
>     
>     nit: this description text sounds like it's giving an absolute time, but
>     given that it's a uint16 I presume it's "time [from now] until sending".
> [skraza]:  Clarified.       
> 
>            container received-peer-state {
>              config false;
>              description
>                "Operational state information learned from the peer.";
>     
>              uses graceful-restart-attributes-per-peer;
>     
>              container capability {
>                description "Configure capability.";
>     
>     This is config-false; "Configure" in the description cannot be correct,
>     though I suppose "Configured" could be.
>     (Also applies to several of the child containers' descriptions.)
>     
> [skraza]:  Fixed all these.
>   
>            leaf up-time {
>              type string;
>              config false;
>              description "Up time. The interval format in ISO 8601.";
>     
>     We probably should have some sort of reference for ISO 8601 (though of
>     course it cannot actually *be* a <xref> in the YANG module itself)
>     
> [skraza]:  Changed to use timeticks64 now.
> 
>            leaf notification {
>              type yang:counter64;
>              description
>                "The number of messages sent or received.";
>     
>     Is this the same as or different than "leaf total-messages"?
> [skraza]:  Fixed.    
> 
>                    leaf label-distribution-controlmode {
>     
>     nit: any reason to not hyphenate control-mode as well?
> [skraza]:  Fixed.        
> 
>                container interfaces {
>                  description
>                    "A list of interfaces for LDP Basic Descovery.";
>     
>     nit: s/Descovery/Discovery/
> [skraza]:  Fixed.    
>     
>              container discovery {
>                description
>                  "Neibgbor discovery configuration and operational state.";
>     
>     nit: s/Neibgbor/Neighbor/
> [skraza]:  Fixed.    
>     
>                        // ipv4
>                        container hello-adjacencies {
>                          config false;
>                          description
>                            "Containing a list of hello adjacencies.";
>     
>                          list hello-adjacency {
>                            key "adjacent-address";
>                            config false;
>                            description "List of hello adjacencies.";
>     
>                            leaf adjacent-address {
>                              type inet:ipv4-address;
>                              description
>                                "Neighbor address of the hello adjacency.";
>                            }
>     
>     I'm not an expert here, but I'm not really seeing why we need to
>     specialize this container for v4 vs v6 as opposed to just using
>     inet:ip-address and having something that's applicable to both.
> 
> [skraza]: We could. There are pros and cons either way. The reasons to use separate containers in this model are:
> 1) Operators usually plan and configure IPv4 and IPv6 separately, rather than mixing the address family together. The separate containers provide more focus and clear separations.
> 2) Often times, there are attributes with different types for different address families. Mixing two together brings up some modeling difficulties to specifies differently for different address families.
> 3) Sometimes, there are attributes that are applicable to one address family but not the other. Mixing the two containers together would require some difficult conditions, which would complicate the model and make the model less usable.
> 4) Mixing the two containers can make the YANG file more compact, but not the configuration data and operational state data.
> 5) The existing RFCs use the style of separate containers, such as RFC 8344 and RFC 8349.

Thanks for writing this out; it helps me understand the situation better.

>                        leaf adjacent-address {
>                          type inet:ipv4-address;
>                          description
>                            "Configures a remote LDP neighbor and enables
>                             extended LDP discovery of the specified
>                             neighbor.";
>     
>     Given that there's a sibling leaf "enable", do we still want "and
>     enables" here?
> [skraza]: We don’t need “enables” here. Removed it.
> 
>     
>     Should peer/capability have a comment in its description that the
>     container exists to be augmented by other (extension) modules?
>     
> [skraza]: Yes. It is better. Added as suggested.
> 
>          notification mpls-ldp-fec-event {
>     
>     Am I reading this right that we only generate notifications on 0->1 and
>     1->0 transitions of "number of prefixes active for a given FEC"?  Would
>     we ever want to know about addition/removal of prefixes that do leave
>     the FEC up?
>     
> [skraza]: Here, the prefix is the FEC. Addition/removal of a prefix is equivalent to the addition/removal of the FEC. Changed the leaf name from prefix to fec to avoid the confusion.
> 
>     Section 9.2
>     
>     I can't find a pattern into which if-feature statements go into
>     containers used to augment things vs. the augment directives themselves.
>     Can we be more consistent (or am I missing the pattern)?
>     
> [skraza]: It is not intended to set up a strict pattern between feature name and containers, while the purpose is to provide more descriptive feature names. That said, the parent container names are often used to provide the contexts for the feature name. e.g. In the case of “dual-stack-transport-pereference”, it seems that the parent container name “peers” is also beneficial, so we have the feature name “dual-stack-transport-preference” to “peers-dual-stack-transport-preference” to make the feature name fit better with the pattern and more descriptive.

Thanks for the explanation

>          feature dual-stack-transport-pereference {
>            description
>              "This feature indicates that the system allows to configure
>               the transport connection pereference in a dual-stack setup.";
>     
>     nit: s/pereference/preference/ (twice)
> [skraza]: Fixed.
>     
>          feature policy-targeted-discovery-config {
>            description
>              "This feature indicates that the system allows to configure
>               policies to control the acceptance of targeted neighbor
>               discovery hello messages.";
>     
>     nit: s/hello/Hello/
>     [skraza]: Fixed.
> 
>          typedef neighbor-list-ref {
>            type string;
>            description
>              "A type for a reference to a neighbor address list.
>               The string value is the name identifier for uniquely
>               identifying the referenced address list, which contains a list
>               of addresses that a routing policy can applied. The definition
>               of such an address list is outside the scope of this
>               document.";
>     
>     If I'm reading correctly between here and where this is used, the
>     semantics of this are better described as "implementation-specific" --
>     that is, this is a label that refers to some local configured state,
>     specifically, a list of neighbor addresses used as an ACL.  (Emphasis on
>     *local* -- it seems like it does not need to interact with anything
>     else.)
>     I do sympathize with the YANG doctor, though -- as-is, this feels
>     underspecified.
>     (BTW, IP ACLs are not considered to be a security measure by the broader
>     security community.)
>     Similarly for the subsequent <foo>-list-refs.
> 
> [skraza]: We do understand the concerns, and have fixed these two types by using the https://tools.ietf.org/html/draft-ietf-rtgwg-policy-model, which was not available when we initially started to work on this model.

Great!

>              container interfaces {
>                description
>                  "A list of interfaces on which forwarding is disabled.";
>     
>     nit: given the separate 'ldp-disable' leaf, perhaps s/is/can be/ is in
>     order.
> [skraza]: Understand the confusion. Changed as suggested.
> 
>     Also, the description of the list itself implies additional
>     functionality, of listing the interfaces used for basic discovery.
>     
> [skraza]: The description was wrong. We don’t intend to use this list for discovery. Fixed.
> 
>          grouping graceful-restart-augment {
>            description "Augmentation to graceful restart.";
>     
>            leaf helper-enable {
>              if-feature graceful-restart-helper-mode;
>              type boolean;
>              default false;
>              description
>                "Enable or disable graceful restart helper mode.";
>     
>     Where is this "helper mode" documented?
> [skraza]: This is not an RFC defined term but commonly known and used when GR timer is set to 0.
>   Fixed the description in yang to highlight this.

Is this just staged for the -09?  I am not seeing it in the -08.

>          augment "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
>            + "ldp:global" {
>            description "Graceful forwarding nexthop augmentation.";
>            uses global-forwarding-nexthop-augment;
>          }
>     
>     nit: I don't think "Graceful" is correct, in the description.
>     [skraza]: Removed it.
> 
>     Why is (the augmented)
>     /rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/ldp:global/ldp:address-families/ipv6/label-distribution-controlmode
>     'config false'?  Doesn't RFC 5036 say that an LSR might support either
>     as a configuration option?
>     
> [skraza]: Yes, but no implementation supports such a cfg as change of such a mode is a massive disruptive operation - all implementation pick one mode as the default and only supported mode and stick to it.
>   The LDP YANG design team had discussed this early enough and agreed to not provide this a cfg option.

Ah, understood.

>                leaf adjacent-address {
>                  type inet:ipv6-address;
>                  description
>                    "Configures a remote LDP neighbor and enables
>                     extended LDP discovery of the specified
>                     neighbor.";
>     
>     As for the ipv4 case, do we still want "and enables" here?
> [skraza]: Fixed by removing it.    
> 
>          augment "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
>            + "ldp:discovery/ldp:interfaces/ldp:interface/"
>            + "ldp:address-families/ldp-ext:ipv6" {
>            description "Interface address family IPv6 augmentation.";
>            uses interface-address-family-ipv6-augment;
>     
>     Didn't we just create .../ldp:address-families/ldp-ext:ipv6 with an
>     earlier augmentation in this module?  (Similarly for
>     /rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/ldp:peers/ldp:peer/ldp:address-families/ldp-ext:ipv6
>     which we augment with "uses peer-af-policy-container".)
>     
> [skraza]: Yes. It is ok to augment a node which is part of another augmentation. The result is to add them all together. Doing so is to avoid too much irregular patterns, so that we have more similar construction between IPv4 and IPv6.
> 
>     Appendix A
>     
>                      "is-router": [null],
>     
>     This looks weird to me ("array containing one element with value literal
>     null") -- why does the array come into play?
>     
>     [Xufeng]: Here is a little explanation to the seemingly confusing notation: the pair of square brackets does not indicate a normal array. Instead, it is a special notation to indicate that the value of a “empty” type leaf is “present” (Section 6.9. of RFC 7951). Semantically the line above says that “(the neighbor) is a router.”

Ah, thanks for the explanations.

-Ben

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Thu, 19 Mar 2020 16:04:24 +0000

Hi Ben, Kamran, 

> -----Original Message-----
> From: iesg <[email-masked]> On Behalf Of Benjamin Kaduk
> Sent: 17 March 2020 20:56
> To: Kamran Raza (skraza) <[email-masked]>
> Cc: [email-masked]; [email-masked]; Nicolai Leymann
> <[email-masked]>; The IESG <[email-masked]>; draft-ietf-mpls-ldp-
> [email-masked]; Tarek Saad <[email-masked]>
> Subject: Re: Updated rev -08 posted [Re: Benjamin Kaduk's Discuss on
> draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]
> 
[RW] 
I've checked the graceful-restart configuration and it looks fine. 

Is there further hierarchical configuration that needs to be checked?

> > Moreover, Sec 1.1 does points out why LDP IPv6 is considered as an
> extended feature. The "base" features are the one most commonly
> implemented and deployed features.
> > The idea was that base must not contain any if-feature - given that
> LDPv6 is not widely implemented and was branded as if-feature, it is
> classified in the extended category then.
> week's IESG telechat to get a better sense on what the right thing to do
> is here.
[RW] 
I note that IPv6 isn't under if-feature in the extension module.  This would imply that implementations that wish to implement optional features in the extension module must also support LDP IPv6 (or deviate).  Is that intentional?

> > [skraza]: It is beyond our control as we are using it as-is from the
> RFC. Made the change anyways to add crypto-algo to widen the scope/type of
> key beyond MD5.
> 
> implement something and have confidence it will interoperate.  For
> example, the recommendations in RFC 3562 include the ability to use a
> direct binary key for TCP-md5.  Does this YANG stanza allow for doing so?
> If yes, what mechanism is used for converting input into "raw" binary, if
> needed?
> 
> That is, even if the actual protocol semantics for how TCP-MD5 uses a
> given key/password are out of your control, there is still ambiguity as to
> what mapping (if any) occurs between the YANG-configured value and what is
> Furthermore, the authentication-type still only has a "string" leaf for
> the key, with (if I am reading the YANG correctly) the recommended key-
> chain functionality for protected key storage being left as an extension.
> It's not clear to me why we should be leaving the best-practice
> formulation as an extension.
> 
> > [skraza]: This is the same structure as RFC 8344. The edge cases are OK.
> The ipv4/ipv6 container may contain none, one, or more nodes in the
> container. ipv4/ipv6 address family is enabled as long as the container is
> “present”. Your assumption is correct, and the modeling (and the protocol
> as well) is in a good state, even with such possibilities. The signals
> should be clear enough.
[RW] 

Intuitively, I don't like the combination of a presence container + enable leaf below it.  I've asked Martin (author of RFC 8344) why the construct is used in this way, since he must have had a good reason.  It isn't immediately obvious to me why what is written isn't a convoluted way of making it a non-presence container.

Regards,
Rob

> > [skraza]: When the type of an attribute is for a specific address family
> (ipv4 or ipv6), we use the address family specific type (inet:ipv4-address
> or inet:ipv6-address), so that user cannot provide a value of the wrong
> format.  inet:ip-address is simply a union of net:ipv4-address and
> inet:ipv6-address, and is used when the address family is not restricted.
> This is particular convenient when the user makes a query on a particular
> peer when the user does not have a particular address family in mind. The
> system will provide the address in a proper format.
> >
> >     Section 1
> >
> > [skraza]: We could. There are pros and cons either way. The reasons to
> use separate containers in this model are:
> > 1) Operators usually plan and configure IPv4 and IPv6 separately, rather
> than mixing the address family together. The separate containers provide
> more focus and clear separations.
> > 2) Often times, there are attributes with different types for different
> address families. Mixing two together brings up some modeling difficulties
> to specifies differently for different address families.
> > 3) Sometimes, there are attributes that are applicable to one address
> family but not the other. Mixing the two containers together would require
> some difficult conditions, which would complicate the model and make the
> model less usable.
> > 4) Mixing the two containers can make the YANG file more compact, but
> not the configuration data and operational state data.
> > [skraza]: Here, the prefix is the FEC. Addition/removal of a prefix is
> equivalent to the addition/removal of the FEC. Changed the leaf name from
> prefix to fec to avoid the confusion.
> >
> >     Section 9.2
> >
> > [skraza]: It is not intended to set up a strict pattern between feature
> name and containers, while the purpose is to provide more descriptive
> feature names. That said, the parent container names are often used to
> provide the contexts for the feature name. e.g. In the case of “dual-
> stack-transport-pereference”, it seems that the parent container name
> “peers” is also beneficial, so we have the feature name “dual-stack-
> transport-preference” to “peers-dual-stack-transport-preference” to make
> the feature name fit better with the pattern and more descriptive.
> 
> > [skraza]: We do understand the concerns, and have fixed these two types
> by using the https://tools.ietf.org/html/draft-ietf-rtgwg-policy-model,
> which was not available when we initially started to work on this model.
> 
> Great!
> 
> >              container interfaces {
> >                description
> >     /rt:routing/rt:control-plane-protocols/ldp:mpls-
> ldp/ldp:global/ldp:address-families/ipv6/label-distribution-controlmode
> > [skraza]: Yes, but no implementation supports such a cfg as change of
> such a mode is a massive disruptive operation - all implementation pick
> one mode as the default and only supported mode and stick to it.
> >     /rt:routing/rt:control-plane-protocols/ldp:mpls-
> ldp/ldp:peers/ldp:peer/ldp:address-families/ldp-ext:ipv6
> > [skraza]: Yes. It is ok to augment a node which is part of another
> augmentation. The result is to add them all together. Doing so is to avoid
> too much irregular patterns, so that we have more similar construction
> between IPv4 and IPv6.
> >
> >     Appendix A
> >
> >                      "is-router": [null],
> >
> >     [Xufeng]: Here is a little explanation to the seemingly confusing
> notation: the pair of square brackets does not indicate a normal array.
> Instead, it is a special notation to indicate that the value of a “empty”
> type leaf is “present” (Section 6.9. of RFC 7951). Semantically the line
> above says that “(the neighbor) is a router.”
>

---

From: tom petch <[email-masked]>
Date: Thu, 19 Mar 2020 17:03:35 +0000

Rob sorry for the top posting but my e-mail is difficult currently.

The use of four presence containers seems wrong; it allows ipv4  or ipv6 to be enabled for one part of LDP and not for others which is not LDP as I know it (which could be ignorance).  The author cites IP YANG module but I think that different; there there is only one presence container which can be set to turn on IPv4 without needing to add any more parameters ie no need to set the enable leaf.  This not unlike  the use of presence in  RIP, OSPF, BFD  but there is only one presence container. in all such cases.  Having four presence containers and four enable leaf for IPV4 (not for the features) seem to me only a possible source of misconfiguration.

I echo Ben's concern about the string password.  Usually this is base64 and I would hope to reference the basic Netconf configuration I-D on such matters but progress on these has not been NETCONF WG finest hour.

I agree that the issue with hello timers is sorted (although the use of refine and uses and the mix of the terminology 'all-interfaces' and 'global' does not make for an easy read.

Sorry this is after the telechat.

Tom Petch
________________________________________
From: mpls <[email-masked]> on behalf of Rob Wilton (rwilton) <rwilton=[email-masked]>
Sent: 19 March 2020 16:04
To: Benjamin Kaduk; Kamran Raza (skraza)
Cc: [email-masked]; [email-masked]; The IESG; [email-masked]
Subject: Re: [mpls] Updated rev -08 posted [Re: Benjamin Kaduk's Discuss on draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]

Hi Ben, Kamran,

[RW]

[RW]

[RW]

Regards,
Rob

> >     semantics of this are better described as "implementation-specific"

_______________________________________________
mpls mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/mpls

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Thu, 19 Mar 2020 18:41:06 +0000

Hi Tom, Ben, Kamran,

Thanks for the comments.

> From: tom petch <[email-masked]>
> Sent: 19 March 2020 17:04
> To: Rob Wilton (rwilton) <[email-masked]>; Benjamin Kaduk
> <[email-masked]>; Kamran Raza (skraza) <[email-masked]>
> Cc: [email-masked]; [email-masked]; The IESG <[email-masked]>; draft-
> [email-masked]
[RW] 
No problem.  I have to use Outlook, and that doesn't seem to be optimized for replying to emails either!

> 
> The use of four presence containers seems wrong; it allows ipv4  or ipv6
> to be enabled for one part of LDP and not for others which is not LDP as I
> know it (which could be ignorance).
[RW] 
Yes, I agree that this is also not ideal and not the problem that I responded to previously.  I have now commented below.

  The author cites IP YANG module but I
> think that different; there there is only one presence container which can
> be set to turn on IPv4 without needing to add any more parameters ie no
> need to set the enable leaf.
[RW] 
In the general case, this feels like a poor optimization to me.  It feels like it adds quite a lot of extra bespoke complexity for little gain, e.g. one data node (the ipv4 presence container) is used to enable IPv4, and a different data node (the enable leaf) is used to disable it. It also looks a bit like an adhoc version of inactive configuration, i.e. why does a client need to configure disabled IPv4 configuration in the first place.

However, I appreciate that this construct is used elsewhere in IETF models, and hence it is at least consistent across the IETF YANG models to use it here.

  This not unlike  the use of presence in
> RIP, OSPF, BFD  but there is only one presence container. in all such
> cases.  Having four presence containers and four enable leaf for IPV4 (not
> for the features) seem to me only a possible source of misconfiguration.
> 
[RW] 
I agree that having the presence/enable leaf in 4 places is not ideal.  Although, it looks like the enable leaf is only in two of those places, but an ipv4 p-container is used in all 4 places.

I'm not really familiar with the details of LDP, but it seems that the configuration for basic discovery, extended discovery, and statically configured peers logically sits at the same level.  Hence I think I can see why IPv4/v6 might all need to be configured/enabled separately for each of them.

But I wasn't sure why the global ipv4 configuration needs a presence container and an enable keyword.  Perhaps it could be changed to a non-presence container, and the enable keyword removed?  I.e. does it make sense to enable IPv4 globally in LDP but not also in the basic discovery, extended discovery, or peer configuration?  Or does the global IPv4 configuration only really apply if IPv4 has been enabled elsewhere?

These comments may equivalently apply to IPv6 as well.

> I echo Ben's concern about the string password.  Usually this is base64
> and I would hope to reference the basic Netconf configuration I-D on such
> matters but progress on these has not been NETCONF WG finest hour.
[RW] 
Yes, we need to get those drafts finished.  This might be something that I need to discuss with the NETCONF chairs and possibly the security ADs to figure out how we have something that is good enough.

But I also have to confess that I don't fully understand the nuances of this issue well enough to really know whether string vs binary is the right answer here.

> 
> I agree that the issue with hello timers is sorted (although the use of
> refine and uses and the mix of the terminology 'all-interfaces' and
> 'global' does not make for an easy read.
[RW] 
I'm okay with the refine statements.  Do you have a specific suggestion on how the terminology could perhaps be tweaked to make it easier to read/understand?

> Sorry this is after the telechat.
[RW] 
No worries.  All constructive input is appreciated by this incoming AD who is trying to find his feet ;-)

Regards,
Rob

> Subject: Re: [mpls] Updated rev -08 posted [Re: Benjamin Kaduk's Discuss
> on draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]
> 
> Hi Ben, Kamran,
> 
> > > We have posted a new rev -08 that takes care almost all of your
> > > comments
> > and comments from other IESG reviewers.
> > >     The YANG doctor last call review of the -06 notes that these
> > > modules
> > are
> > > [skraza]:  This was an early decision taken and presented to WG and
> > > WG
> > had agreed.
> > It's not really clear to me that just because the protocol is
> > described in separate RFCs for IPv4 and IPv6 we cannot present a
> > unified management interface on top of it.  I note that several other
> > ADs expressed concern about this topic, so I've moved this document
> > onto the agenda for this week's IESG telechat to get a better sense on
> > what the right thing to do is here.
> [RW]
> I note that IPv6 isn't under if-feature in the extension module.  This
> would imply that implementations that wish to implement optional features
> in the extension module must also support LDP IPv6 (or deviate).  Is that
> intentional?
> 
> 
> >
> > >
> > >     We need to define the format/semantics of the md5-key string
> > > (e.g.,
> > is
> > >     doctor notes).  Using a crypto-type would probably be
> > > appropriate,
> > as
> > RFC. Made the change anyways to add crypto-algo to widen the
> > scope/type of key beyond MD5.
> >
> > I'm still concerned that there isn't enough text here for me to be
> > able to implement something and have confidence it will interoperate.
> > For example, the recommendations in RFC 3562 include the ability to
> > use a direct binary key for TCP-md5.  Does this YANG stanza allow for
> doing so?
> > If yes, what mechanism is used for converting input into "raw" binary,
> > if needed?
> >
> > given key/password are out of your control, there is still ambiguity
> > as to what mapping (if any) occurs between the YANG-configured value
> > and what is passed to the TCP-MD5 implementation.
> >
> > Furthermore, the authentication-type still only has a "string" leaf
> > for the key, with (if I am reading the YANG correctly) the recommended
> > key- chain functionality for protected key storage being left as an
> extension.
> > >     label-policy/advertise/prefix-list, we need to say if the filter
> > > is
> > for
> > >     suggests that the contents will provide not just a list of
> > > prefixes
> > that
> > >     prefix-list-ref, and it seems like a different type may be
> > > needed
> > for
> > Ah, this took me a little more work to find, since the changes were in
> > the prefix-list-ref typedef.  Thanks.
> >
> > >     (pro-forma) This document has 6 authors, and per RFC 7322, as
> > > this
> > is
> > > [skraza]: We had taken an advise from Loa (MPLS WG chair) and would
> > > like
> > to keep it to 6.
> >
> > Sure; this was presumed to just be "pro forma" but I put it in since
> > the shepherd writeup didn't mention it specifically.
> >
> > >
> > >     I suggest making a reference to RFC 5920 at some point, perhaps
> > > from
> > the
> > >     security considerations.
> > > [skraza]: Done.
> > >
> > >     We have several instances of a "container ipv4" that's a
> > > "container
> > with
> > >     presence" in the jargon of RFC 7950.  As SEC AD one thing I look
> > > for
> > is
> > >     edge cases, such as when a subset of these containers are
> > > present
> > but
> > >     natural to assume that IPv4 is enabled if any are present, it
> > > does
> > make
> > >     possibility for such skewed signals.  (I didn't specifically
> > > audit
> > for
> > > [skraza]: This is the same structure as RFC 8344. The edge cases are
> OK..
> > container. ipv4/ipv6 address family is enabled as long as the
> > container is "present". Your assumption is correct, and the modeling
> > (and the protocol as well) is in a good state, even with such
> > possibilities. The signals should be clear enough.
> [RW]
> 
> Intuitively, I don't like the combination of a presence container + enable
> leaf below it.  I've asked Martin (author of RFC 8344) why the construct
> is used in this way, since he must have had a good reason.  It isn't
> immediately obvious to me why what is written isn't a convoluted way of
> making it a non-presence container.
> 
> Regards,
> Rob
> 
> 
> > >
> > (ipv4 or ipv6), we use the address family specific type
> > (inet:ipv4-address or inet:ipv6-address), so that user cannot provide
> > a value of the wrong format.  inet:ip-address is simply a union of
> > net:ipv4-address and inet:ipv6-address, and is used when the address
> family is not restricted.
> > This is particular convenient when the user makes a query on a
> > particular peer when the user does not have a particular address
> > family in mind. The system will provide the address in a proper format.
> > >
> > >     Section 1
> > >
> > >        The data model is defined for following constructs that are
> > > used
> > for
> > >        managing the protocol:
> > >
> > >     nit: "the following".
> > > [skraza]: Fixed.
> > >
> > >     I share the YANG doctor's concern that separating the
> > > configuration
> > and
> > >        The "base" category contains the basic and fundamental
> > > features
> > that
> > >        are covered in LDP base specification [RFC5036] and
> > > constitute
> > the
> > >        minumum requirements for a typical base LDP deployment.
> > > Whereas,
> > the
> > >        o  This module aims to address only the core LDP parameters
> > > as
> > per
> > >           features (such as label policies, session authentication
> etc)..
> > >     [Even for widely-deployed non-RFC features we still need to
> > > provide
> > a
> > >     (the 'P' is for "private") is used only for
> > > protocols/deployments
> > that
> > >     conveyed.  We are, of course, not there now, and I cannot insist
> > > on
> > any
> > >     suffice, that includes just the "virtual" part but not "VPN".
> > > Since
> > we
> > >     Does this imply that there is only one peer per address?  Is
> > > that
> > going
> > > Not an issue anymore as we are printing the entire tree as generated
> > > by
> > the tool.
> > >
> > >
> > >     Section 5.1.2
> > >
> > To be clear, the linked content uses the convention of an "*" marking
> > to indicate that the expansion is "well-known" and does not need to be
> > provided in a given document.  The entry for "Next Hop Label
> > Forwarding Entry" does not include such a marking; perhaps the
> > responsible AD should request it to be added, if it is indeed well-
> known?
> >
> > >     Section 9
> > >
> > >     nit: this description text sounds like it's giving an absolute
> > > time,
> > but
> > >     We probably should have some sort of reference for ISO 8601
> > > (though
> > of
> > than mixing the address family together. The separate containers
> > provide more focus and clear separations.
> > address families. Mixing two together brings up some modeling
> > difficulties to specifies differently for different address families.
> > family but not the other. Mixing the two containers together would
> > require some difficult conditions, which would complicate the model
> > and make the model less usable.
> > > 5) The existing RFCs use the style of separate containers, such as
> > > RFC
> > 8344 and RFC 8349.
> >
> > > [skraza]: We don't need "enables" here. Removed it.
> > >
> > >
> > >     Am I reading this right that we only generate notifications on
> > > 0->1
> > and
> > equivalent to the addition/removal of the FEC. Changed the leaf name
> > from prefix to fec to avoid the confusion.
> > >
> > >     Section 9.2
> > >
> > provide the contexts for the feature name. e.g. In the case of "dual-
> > stack-transport-pereference", it seems that the parent container name
> > "peers" is also beneficial, so we have the feature name "dual-stack-
> > transport-preference" to "peers-dual-stack-transport-preference" to
> > make the feature name fit better with the pattern and more descriptive.
> >
> > >               identifying the referenced address list, which
> > > contains a
> > list
> > >     semantics of this are better described as "implementation-
> specific"
> > --
> > by using the
> > https://tools.ietf.org/html/draft-ietf-rtgwg-policy-model,
> > >     nit: given the separate 'ldp-disable' leaf, perhaps s/is/can be/
> > > is
> > in
> > >     order.
> > > [skraza]: The description was wrong. We don't intend to use this
> > > list
> > for discovery. Fixed.
> > >
> > > [skraza]: This is not an RFC defined term but commonly known and
> > > used
> > when GR timer is set to 0.
> > such a mode is a massive disruptive operation - all implementation
> > pick one mode as the default and only supported mode and stick to it.
> > >   The LDP YANG design team had discussed this early enough and
> > > agreed to
> > not provide this a cfg option.
> >
> > Ah, understood.
> >
> > >                leaf adjacent-address {
> > >                  type inet:ipv6-address;
> > >                  description
> > augmentation. The result is to add them all together. Doing so is to
> > avoid too much irregular patterns, so that we have more similar
> > construction between IPv4 and IPv6.
> > >
> > >     Appendix A
> > >
> > >                      "is-router": [null],
> > >
> > Instead, it is a special notation to indicate that the value of a
> "empty"
> > type leaf is "present" (Section 6.9. of RFC 7951). Semantically the
> > line above says that "(the neighbor) is a router."
> >

---

From: tom petch <[email-masked]>
Date: Fri, 20 Mar 2020 12:42:17 +0000

From: Rob Wilton (rwilton) <[email-masked]>
Sent: 19 March 2020 18:41
Hi Tom, Ben, Kamran,
Thanks for the comments.

[RW]
No problem.  I have to use Outlook, and that doesn't seem to be optimized for replying to emails either!
<tp> 
I used to have Outlook Express which I thought basic but I did get very productive with it and now I am on Outlook!.  Also, I am getting a bounce from juniper.net so someone is not seeing this - else inline
[RW]

[RW]

However, I appreciate that this construct is used elsewhere in IETF models, and hence it is at least consistent across the IETF YANG models to use it here.
<tp>  Rob - I think that the use of presence+enable-leaf is the exception and intended for the corner case that is the Netmod interface model.  The issue as I see it was the two scenarios.  A blade is hot-plugged, the device reads its properties and sets up the 'configuration' as state data which OAM can then NETCONF get.  Or OAM sets up the configuration in advance as configuration, the blade is hot-plugged, the configuration data is matched on some parameter and the device sets the blade to what OAM wants.  NETMOD WG spent years on this without success and this was (in part) the driver for NMDA with its multiple datastores.

Anywhere else I think that having two ways to enable/disable something is confusing.  In this case, I find the (four) description clauses unclear and those should at least be rewritten.

more below

[RW]

<tp> 
I am sure they apply to IPv6 just as well!.  The current model allows LDP targeted to be IPV6 only and LDP discovery to be IPv4 only.  If that is what the authors want then I think that needs spelling out in the body of the document.  Personally I do not think it is what is wanted so I want just one place to enable IPv4 and one place to enable IPv6 and also for those places to be enable leaf only. no presence.  As I said, I see presence used for optional features when no configuration leafs are needed or when there are few and likely to default.  This is how I see BFD, RIP, OSPF IETF YANG modules use presence containers
One more below

[RW]

[RW]
I'm okay with the refine statements.  Do you have a specific suggestion on how the terminology could perhaps be tweaked to make it easier to read/understand?
<tp>
I do not think that the problem is the terminology, it is the way YANG is being used.  You know that the original YANG could not do what the authors wanted, courtesy of the way YANG handles default values (which I think is weird).  But for me the fix is to stop trying to save a few lines  of code with a grouping with two uses one with a refine.  Ditch the YANG grouping and uses and spell it out.  This is a hobby horse of mine, that those writing code want to save a few lines by creating lots of subroutines/groupings with an increase in complexity that adversely affects all users ever after!  Like I say a hobby horse.

Tom Petch 

[RW]

Regards,
Rob

---

From: tom petch <[email-masked]>
Date: Fri, 20 Mar 2020 18:01:39 +0000

<tp>
Rob
picking up separately on the question of security inline

Thanks for the comments.

[RW]

[RW]

[RW]

[RW]

[RW]

<tp>
I think that it can be summed up as the Routing Area uses string with an associated algorithm which implies the format - could be ASCII, Hex, base64, ... and so covers anything.

Operations Areas, at least in Netconf and Netmod, uses  binary which in YANG means base64 and so can be anything.  draft-ietf-netconf-crypto-types defines YANG identity such as one-symmetric-key-format referencing RFC6031 and DER encoding.

So string or binary may not matter or at least not as much as having a format/algorithm alongside it which nails down the format and gives interoperability and entropy etc.

I suspect that I-D such as ldp-yang should specify what is acceptable in Security Considerations in terms of format/algorithm eg saying that MD5 is not or is as the case may be - currently the I-D only mentions MD5, as an example, and I thought the IETF was seeking to wean people off that.

I think that the work in the Routing Area, such as RFC8177, is somewhat ahead of work in the Operations Area and could provide a source of guidance on what is acceptable.
Tom Petch
(not a security expert)

[RW]

[RW]

Regards,
Rob

---

From: "Kamran Raza (skraza)" <[email-masked]>
Date: Fri, 20 Mar 2020 20:34:03 +0000

Rob/Tom,

We are going to post rev -09 shortly with some more edits to take care of some of the comments.
Please see inline [skraza20]

On 2020-03-20, 8:42 AM, "tom petch" <[email-masked]> wrote:

    [RW]
    [RW]
    
    [RW]

[skraza20] Updated the descriptions a bit . See in rev -09.
    
    more below
    
    [RW]

    
    
    <tp> 
    I am sure they apply to IPv6 just as well!.  The current model allows LDP targeted to be IPV6 only and LDP discovery to be IPv4 only.  If that is what the authors want then I think that needs spelling out in the body of the document.  

[skraza20]: Yes, intf discovery and tgt discovery are separate and not linked - it is entirely possible to have v4 intf disc an v6 tgt and vice versa. This is obvious from the LDP protocol specs and hence we see need to call this out. 
  Otherwise, there are 100 other things that may need call out - It is expected that user of this model is familiar with the LDP base specs.

Personally I do not think it is what is wanted so I want just one place to enable IPv4 and one place to enable IPv6 and also for those places to be enable leaf only. no presence.  As I said, I see presence used for optional features when no configuration leafs are needed or when there are few and likely to default.  This is how I see BFD, RIP, OSPF IETF YANG modules use presence containers
    One more below
    
    [RW]
    
    [RW]
    
    Tom Petch 
    
    [RW]
    
    Regards,
    Rob

---

From: "Kamran Raza (skraza)" <[email-masked]>
Date: Fri, 20 Mar 2020 20:34:32 +0000

Thanks for your x-chk. 

Please note that following items are now closed as per telechat discussions:
 - LDPv6 remains in the extended model
 - Use of string for password

Please see inline [skraza20] answers to some of your Qs.
[ Note: We will upload rev -09 shortly with minor changes shortly ] 

On 2020-03-17, 4:56 PM, "Benjamin Kaduk" <[email-masked]> wrote:

[skraza20]: FYI, this document has been extensively reviewed twice by YANG Dr and AD.	
   Moreover, I see the Rob has also checked and confirmed.

[skraza20: Closed.

    

[skraza]: Closed.

    
    
[skraza]: Thx.

[skraza20]: Please see our response sent on other email thread (w/ Rob/Tom)

[skraza20]: Ok thx. RFC3031 should have added but will ask AD/Deborah. In any case, we have also expanded in our doc - rev -09.

    

    
    Great!

    Is this just staged for the -09?  I am not seeing it in the -08.
[skraza20]: It is there - we had added it in rev -08 under description for "feature graceful-restart-helper-mode".

    Ah, understood.

    
    -Ben

---

From: tom petch <[email-masked]>
Date: Mon, 23 Mar 2020 17:36:34 +0000

From: Kamran Raza (skraza) <[email-masked]>
Sent: 20 March 2020 20:34

Kamran
Thanks for -09;  plucking out your paragraph

[skraza20]: Yes, intf discovery and tgt discovery are separate and not linked - it is entirely possible to have v4 intf disc an v6 tgt and vice versa. This is obvious from the LDP protocol specs and hence we see need to call this out.
  Otherwise, there are 100 other things that may need call out - It is expected that user of this model is familiar with the LDP base specs
<tp>
Clearly I am not familiar enough with the base specs!  I did reread RFC7552 and still did not see it clearly spelt out there, rather it seems to bundle the parts of LDP together in its characterisation of LDP/LSR as single or dual stack.  No matter, for the benefit of such as me, I would value extra words in the description clauses of those presence containers, simply adding what it is that IPv4/IPv6 is being enabled for ie replace
Present if IPv4 is enabled
with
Present if IPv4 is enabled for discovery/targeted discovery/ etc

Given the structure of YANG, I find it easy to lose sight of what it is is being enabled or disable.

In the same vein, I am unclear about the interaction between the presence container for IPv4 or IPv6 under global and that under eg targeted discovery.  What is it that the global value enables and disables?  Can the global IPv6 be disabled but IPv6 targeted discovery be enabled?  Again, a few words about this in the description clauses would help with this.

On a different note, I suggest
/per-peer setting can/per-peer setting and/
/confgiration/configuration/
/Please refer RFC4378/reference " RFC4378
/establishement/establishment/

HTH

Tom Petch
Rob/Tom,

    [RW]
    [RW]

    [RW]

    more below

    [RW]

    <tp>

    [RW]

    [RW]

    Tom Petch

    [RW]

    Regards,
    Rob

---

From: "Kamran Raza (skraza)" <[email-masked]>
Date: Mon, 23 Mar 2020 17:49:47 +0000

Thanks Tom.

We will try to sneak in better description when taking up our next rev. 

On 2020-03-23, 1:36 PM, "tom petch" <[email-masked]> wrote:

    Kamran

    

    HTH
    
    Tom Petch
    Rob/Tom,

    
    
        [RW]
        [RW]
    
        [RW]

    
    
        more below
    
        [RW]

    
    
        <tp>

    
        [RW]
    
        [RW]
    
        Tom Petch
    
        [RW]
    
        Regards,
        Rob

---

From: tom petch <[email-masked]>
Date: Wed, 25 Mar 2020 10:09:17 +0000

From: Kamran Raza (skraza) <[email-masked]>
Sent: 23 March 2020 17:49
Subject: Re: Updated rev -08 posted [Re: Benjamin Kaduk's Discuss on draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]

Thanks Tom.
<tp>
Kamran,

Now that ldp-yang is on its way, would it be a good time to attend to mldp-yang?  It has many of the same issues as ldp-yang had (as I seem to recall saying some time back:-).  I notice that it has expired so keeping AD and chairs on this message to ask the question is it going to progress and if so when?  From my point of view, having spent time on ldp-yang and got to understand its unfamiliar - to me - constructs, now is the time to look at it but equally there is no point in doing so unless the issues get taken up reasonably quickly, ie weeks not months.

Tom Petch

    Kamran

    HTH

    Tom Petch
    Rob/Tom,

        [RW]
        [RW]

        [RW]

        more below

        [RW]

        <tp>

        [RW]

        [RW]

        Tom Petch

        [RW]

        Regards,
        Rob

---

From: "Rajiv Asati (rajiva)" <[email-masked]>
Date: Wed, 25 Mar 2020 12:07:40 +0000

Tom,

Indeed. The intent is to now get mLDP yang draft updated quickly as well. Thanks for the reminder. 

Cheers,
Rajiv  

> On Mar 25, 2020, at 6:10 AM, tom petch <[email-masked]> wrote:
>

---

From: tom petch <[email-masked]>
Date: Wed, 25 Mar 2020 12:33:40 +0000

From: Rajiv Asati (rajiva) <[email-masked]>
Sent: 25 March 2020 12:07
Tom,

<tp>
Good stuff.  In no particular order
- needs refreshing to get it into the directory
- augments lack control-plane-protocol
- no identity; derive from ldp? separate for P2MP and MP2MP?
- security lacks specific leaf at risk
- multiple enable leaf with identical description making it unclear to me just which aspect is being enabled
- line lengths too long
- no reference for tree diagram RFC
etc

Cheers,
Rajiv

---

From: "Kamran Raza (skraza)" <[email-masked]>
Date: Wed, 25 Mar 2020 16:20:47 +0000

Hi Tom,

Yes, we have not forgotten mLDP __ and I had already started the [alignment/update] activity right after Fri posting of LDP draft.
Thanks for summarizing ... 

Hoping to post something by end of next week ...

On 2020-03-25, 8:34 AM, "tom petch" <[email-masked]> wrote:

    
    <tp>
    
    Cheers,
    Rajiv