[mpls] mldp was Re: Yangdoctors last call review of draft-ietf-mpls-ldp-yang-06

[tom petch <[e-mail]>]

Jan

It would likely help if you could also look at
draft-ietf-mpls-mldp-yang-06
which augments
draft-ietf-mpls-ldp-yang-06
with such as
[widescreen needed]
module: ietf-mpls-mldp
  augment
/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/ldp:global/ldp:capab
ility:
and
     augment "/rt:routing/rt:control-plane-protocols/"
       + "ldp:mpls-ldp/ldp:peers/ldp:peer/ldp:received-peer-state/"
       + "ldp:capability/mldp:mldp" {
i.e. there are another 20 or so augment which are affected by the
changes you propose.

Note that the prefixes are ldp: and mldp: so the identities for the
control plane protocols are probably better as those and not mpls-ldp or
mpls-mldp.  But is mldp a separate protocol in RFC8349 terms or just
part of ldp? I do not know.

The mldp draft has just completed WGLC. I looked at it but gave up on it
because the line lengths were too long to display on my screen.

Tom Petch


----- Original Message -----
From: "Jan Lindblad via Datatracker" <[e-mail]>
To: <[e-mail]>
Cc: <[e-mail]>; <[e-mail]>;
<[e-mail]>
Sent: Tuesday, October 15, 2019 1:07 PM
Subject: [mpls] Yangdoctors last call review of
draft-ietf-mpls-ldp-yang-06


> Reviewer: Jan Lindblad
> Review result: Ready with Issues
>
> Modules: ietf-mpls-ldp.yang, ietf-mpls-ldp-extended.yang
> Review result: Ready with Issues
> Reviewer: Jan Lindblad
>
> This is my YANG doctor LC review of draft-ietf-mpls-ldp-yang-06. I
find the
> module ready with issues, even if there are a couple rather
fundamental things
> to work out. It is evident that the authors have spent considerable
time to
> make the module clean and tidy, which is why I can say it's more or
less ready
> even when there are fundamentals to work out. In my judgement it may
be
> possible to work out all remaining issues rather quickly.
>
> #1) Fitting mpls-ldp into the ietf-routing framework
>
> The modules at hand, ietf-mpls-ldp.yang and
ietf-mpls-ldp-extended.yang, are
> augmenting into the ietf-routing module (RFC 8022). I find this highly
> appropriate. RFC 8022 has a section on how control plane protocols are
meant to
> fit into the ietf-routing framework:
>
> 5.3.2.  Defining New Control-Plane Protocols
>
>    It is expected that future YANG modules will create data models for
>    additional control-plane protocol types.  Such a new module has to
>    define the protocol-specific configuration and state data, and it
has
>    to integrate it into the core routing framework in the following
way:
>
>    o  A new identity MUST be defined for the control-plane protocol,
and
>       its base identity MUST be set to "rt:control-plane-protocol" or
to
>       an identity derived from "rt:control-plane-protocol".
> ....
>    o  Configuration parameters and/or state data for the new protocol
>       can be defined by augmenting the "control-plane-protocol" data
>       node under both "/routing" and "/routing-state".
>
> The current draft is doing neither of the above two points. IMHO, it
should do
> both. Or if there is strong reason not to do that, incorporate a
statement from
> the ietf-routing authors and a clear explanation of why the mpls-ldp
related
> modules would be exempted. At present, this deviation from the RFC
8022
> architecture is never mentioned in the draft.
>
> Even if this issue falls under fundamental issues, the way I look at
it, it
> would be quite easy to fix. Adjusting the two draft modules to comply
with the
> points above requires changes to two dozen augment statement paths and
a
> handful of leafref paths. Adding the identity and corresponding when
derived
> from statement are a couple of lines.
>
> In principle, this change opens up for multiple instances of mpls-ldp.
This
> might be useful if a system is used with two entirely separate
networks (such
> as two separate customers), where each one might be running an
mpls-ldp
> instance. If this is not desired, a must statement could be introduced
to limit
> the number of instances to one.
>
> #2) Global level vs. local (e.g. interface or peer) level
>
> The module has several configurations that have a global and local
level, such
> as configurations per peer or per interface. Presumably, these local
level
> configurations are meant to override the global default when set and
let the
> global default prevail when nothing specific is specified at the local
level.
>
> The way this is being currently implemented in the modules, however,
makes it
> impossible not to set a value at the local level, so the local setting
will
> always prevail. The global configuration would have no effect at all
(except in
> those cases where a local level configuration is marked with an
if-feature and
> the server would not announce that feature).
>
> For example:
>
> On the global level, there is a graceful-restart/enable leaf:
>
>       container global {
> ....
>         container graceful-restart {
>           leaf enable {
>             type boolean;
>             default false;
>           }
>
> Further down, we have another graceful-restart/enable leaf, used per
peer:
>
>   grouping graceful-restart-attributes-per-peer {
> ....
>     container graceful-restart {
>       leaf enable {
>         type boolean;
>         default false;
>       }
>
> I would assume this latter enable leaf should take precedence over the
global
> graceful-restart when set on the peer. But if not set on the peer, the
global
> graceful-restart value should be used. Since the peer
graceful-restart/enable
> leaf has a default value, however, it will never happen that it is not
set, and
> will thus render the global value completely irrelevant for all peers.
>
> The fix is simple: Do not have a default statement on the local leafs.
Explain
> in the description statement what happens when the element has no
value (the
> globally configured value will be used). Make sure the global leaf has
a
> default (they all do already).
>
> The same applies to a rather long list of leafs in the module. I don't
dare
> listing them here, because I would surely miss a few. My point is that
the
> global/local relationships needs to be identified, local defaults
removed. It
> would also make sense to mention how this is supposed to work in the
> descriptions.
>
> #3) Unlinked multikey leafrefs
>
> The grouping ldp-peer-ref has two leafrefs, one for each key in the
list of ldp
> peers. Two leafrefs are exactly what you need to reference an entry in
a list
> with two keys, so that's good. It is not enough to simply have two
unlinked
> leafrefs, however. That would still allow them to have values that are
valid
> separately, but not point to any entry in the list combined. If you
have a copy
> of "Network Programmability with YANG", this is explained in detail on
pp.
> 451-454.
>
> To link the leafrefs, you'd need to update the path expression in the
> label-space-id leaf as follows:
>
>   grouping ldp-peer-ref {
>     description
>       "An absolute reference to an LDP peer, by the LDP ID, which
>        consists of the LSR ID and the Label Space ID.";
>
>     leaf lsr-id {
>       type leafref {
>         path "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
>           + "ldp:peers/ldp:peer/ldp:lsr-id";
>       }
>       description
>         "The LSR ID of the peer, as a portion of the peer LDP ID.";
>     }
>     leaf label-space-id {
>       type leafref {
>         path "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
>           + "ldp:peers/ldp:peer[lsr-id =
>           current()/../lsr-id]/ldp:label-space-id";
>       }
>
> The paths above would also have to be adjusted as part of fixing issue
#1
> above. If this fixing results in allowing multiple mpls-ldp instances,
the
> grouping above needs to be extended with one additional, linked
leafref that
> points out which mpls-ldp instance is being targeted. Or if that is
always the
> same instance as the reference is coming from, a reformulation of the
paths
> above could ensure no mpls-ldp instance name would be required. In
that case,
> the paths should be relative and never venture outside the mpls-ldp
container.
> I would be happy to explain this further.
>
> #4) Weakly defined types for neighbor-list-ref, prefix-list-ref,
peer-list-ref
>
> These types are not leafrefs, but strings without any YANG
substatements to
> define the format. The only thing the description does is to claim
that the
> entities they refer to are outside the scope of this document. For an
> operator/programmer encountering this type, that isn't very helpful,
and is not
> going to be interoperable.
>
>   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.";
>   }
>
> I'm not sure if this is fixable by sharpening the YANG module, but
maybe more
> could be done to guide a confused reader. What would the user do to
find out
> the format of this type and valid values? Add to the description.
>
> #5) Interfaces or mpls, who's on top?
>
> In the mpls-ldp modules, there are two interface lists and two
leafrefs
> pointing to interfaces. This makes me wonder if some of the YANG
structure
> would belong more naturally as an augment to ietf-interfaces instead?
I mean,
> it doesn't sound fun to define the same interfaces in a lot of
different
> places, just because a protocol needs some additional information.
This is just
> a casual observation from an mpls-innocent guy, so you can disregard
if you
> feel it's irrelevant.
>
> #6) MD5 key
>
> There is a leaf md5-key of type string. Is this leaf sensitive from a
security
> point of view? A plaintext string would not be ideal if that is the
case.
> Choose a crypto type instead. Also, the format of this string is not
defined.
> Hex without spaces? Explain the format in the description.
>
> ><
>
> _______________________________________________
> mpls mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/mpls