Hi Jan,
Thanks for your detailed review and apologies for the delayed addressing.
We have also some other comments to address from other reviewers and are planning to spin a new rev right after new year.
Please see our reply inline below - tagged as [skraza]
On 2019-10-15, 8:07 AM, "Jan Lindblad via Datatracker" <noreply@xxxxxxxx> wrote:
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.
[skraza]: We had discussed this in our prelim design mtgs at time of startup of this work and x-vendors
had agreed to limit our scope to single instance LDP/mLDP. So that’s why the current (non-list) hierarchy.
Having said, we are open to follow (list-based) hierarchy and limit to a single instance, as you suggested.
This is not most user friendly but we can do it.
#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.
[skraza]: Ack. We will remove “default” from local leafs and update the description of the local leaf.
#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";
}
[skraza]: Looks like we missed this. Thanks for pointing out. We will fix this and x-chk others.
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.
[skraza]: This is the unnecessary complexity that comes into play if we go with multi-instance. Isn't it?
But sure, we will address this as well.
#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.
[skraza]: At the time, there was no routing-policy model available for us – and hence the above.
Given that draft-ietf-rtgwg-policy-model now defines routing-policies with prefix-set, we would use those here and in all other places where we need to define a list of peers or list of prefixes.
#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.
[skraza]: This is a discussion about “interface-centric” vs “protocol-centric” config model.
We had also discussed this day1 of our modeling activity and had decided x-vendor to go with protocol-centric way.
We will keep it this way.
#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.
[skraza]: Firstly, as far as we understand, the crypto does not define/cover “md5” authentication case.
Secondly, in our module we are dealing with the protocol/network message/connection authentications and not a user authentication.
So not sure if the “crypto” define/procedures apply. Last but not least, the YANG for other routing protocols (BBGP, OSPF) seems to be doing/defining md5 authentication same way as us.
Perhaps, we need a common solution/strategy at the RTG area level for routing protocols. If there already is one available, please let us know.
><
