On 16/09/2021 08:18, Liubing (Remy) wrote:
Hello Jan,
Many thanks for your review. We will make related changes in the next version.
Bing
This e-mail caught my eye as I was looking at the recent revision of
nvo3-yang.
You might also want to fix
IANA Considerations
Security Considerations
Copyright
Contact
Organisation
etc
none of which conform to YANG Guidelines!
They are admin details which have to be fixed sometime and for me, the
sooner the better. I have a mindset that says if the admin is wrong,
then the technical aspects probably are as well:-)
And whether you use 'action' or 'RPC' I would always add
nacm:default deny-all
as a basic security precaution.
I agree with all Jan's comments.
Tom Petch
Best regards,
Bing
-----邮件原件-----
发件人: Jan Lindblad via Datatracker [mailto:[email protected]]
发送时间: 2021年9月16日 15:11
Reviewer: Jan Lindblad
Review result: Not Ready
This is the YANG Doctor review of draft-ietf-nvo3-yang-cfg. In my judgement the
draft is not ready for last call. This is not to say that there isn't plenty of
good work that has gone into this draft and YANG module, but since I have been
unable to compile it, due to unclear dependencies, the fundamental requirements
for performing a review aren't met.
As soon as you import another module, you become dependent on that module and
inherit any problems it may have. If the modules you depend upon are not stable
enough, it may not be viable to perform a last call review. As long as the
modules you depend upon are not published, I would think you cannot go through
with publication of your own module either.
There's always a bit of guesswork involved when a YANG module imports unpublished modules. By
looking in the IETF Git repository, under "standards/ietf/RFC" as well as under
"experimental/ietf-extracted-YANG-modules", i.e. the section for automatically extracted
YANG modules from drafts, I was able to guess at which modules that are transitively referenced.
Based on this, I believe we will need to have stable versions of the following modules before the
draft-ietf-nvo3-yang-cfg can proceed. Experimental modules are marked (*).
iana-bfd-types.yang (*)
iana-if-type.yang
ietf-bfd-types.yang (*)
ietf-bgp-common-multiprotocol.yang (*)
ietf-bgp-common-structure.yang (*)
ietf-bgp-common.yang (*)
ietf-bgp-l3vpn.yang (*)
ietf-bgp-neighbor.yang (*)
ietf-bgp-peer-group.yang (*)
ietf-bgp-policy.yang (*)
ietf-bgp-rib-attributes.yang (*)
ietf-bgp-rib-tables.yang (*)
ietf-bgp-rib-types.yang (*)
ietf-bgp-rib.yang (*)
ietf-bgp-types.yang (*)
ietf-bgp.yang (*)
ietf-inet-types.yang
ietf-interfaces.yang
ietf-ip.yang
ietf-key-chain.yang
ietf-l2vpn.yang (*)
ietf-netconf-acm.yang
ietf-network-instance.yang
ietf-pseudowires.yang (*)
ietf-routing-policy.yang (*)
ietf-routing-types.yang
ietf-routing.yang
ietf-tcp-common.yang (*)
ietf-tcp.yang (*)
ietf-yang-schema-mount.yang
ietf-yang-types.yang
Since I already did a first reading and found a few things, I'll list them here
for the WG's benefit rather than me holding them to myself.
1. YANG 1.1
The draft says "YANG [RFC6020] is a data definition language that was introduced
to". Since you are using yang-version 1.1, it would be more appropriate with a
reference to RFC 7950.
2. Identity equality tests
There are a couple of places in the YANG that check that an interface is of Nve
type. Currently this is done using an equality test. While that can work, a
better and more future proof way is to use derived-from-or-self(). So change
must "(/if:interfaces/if:interface[if:name=current()]/if:type='Nve')";
to
must
"derived-from-or-self(/if:interfaces/if:interface[if:name=current()]/if:type,
'Nve')";
3. Behavioral constraints defined in English text
list static-peer {
key "peer-ip";
...
leaf out-vni-id {
type uint32 {
range "1..16777215";
}
description
"The ID of VNI for outbound. Do not support separate
deletion.";
}
I'm not completely clear as to the "Do not support separate deletion" is
supposed to mean. If every static-peer must have an out-vni-id, it should be marked
mandatory. If this is supposed to mean that an out-vni-id may be added, but not deleted,
that violates one of the basic principles in YANG. The validity of a configuration should
not depend on anything else than the configuration itself. In particular, the validity of
an upcoming configuration should not depend on the prior configuration.
If you don't want out-vni-id to be mandatory, but also don't want people to be
able to delete it without deleting the static-peer, I would recommend making
out-vni-id part of the key for static-peer, and ensure it can take a value that
essentially means none. For example like this:
list static-peer {
key "peer-ip out-vni-id";
...
leaf out-vni-id {
type union {
type enumeration {
enum none;
}
type uint32 {
range "1..16777215";
}
}
description
"The ID of VNI for outbound. Do not support separate
deletion.";
}
4. Repetition of config false
In many places within the module, there are config false statements inside
elements that are already config false. While this is perfectly legal, it is
superfluous. Once a container or other element is marked config false,
everything below that point will always be config false. The IETF convention is
to not write config false in more places than necessary.
container peers {
config false; <--- Everything within/below container peers is now
config false description
"Operational data of remote NVE address in a VNI.";
list peer {
key "vni-id source-ip peer-ip";
config false; <--- Superfluous
5. Weak data format
leaf up-time {
type string {
length "1..10";
}
config false;
description
"The continuous time as NVO3 tunnel is reachable.";
}
YANG strings are great when used for names that the client can choose
arbitrarily. They are not so great when they encode information that is not a
name, unless there is a detailed description of the encoding. Since the
encoding of the up-time leaf is not specified, the contents will not be
interoperable. Either remove this leaf, or better, describe the format of the
contents so that it can be decoded. Changing the type from string to some time
type might be a good option.
6. Identities conventionally start with lowercase letters
By convention, all identities defined by IETF start with a lower case letter.
Citing the principle of least surprise, I would recommend changing
identity Nve {
to
identity nve {
7. RPC or Action
In YANG 1.1, rpc:s that pertain to a certain element in the data tree can be
modeled as actions instead. This often leads to a more coherent model with less
text. In this module, that seems to be the case for the two rpc:s defined.
Therefore I would suggest changing
rpc reset-vni-peer-statistic {
description
"Clear traffic statistics about the VXLAN tunnel.";
input {
leaf vni-id {
type uint32 {
range "1..16777215";
}
mandatory true;
description
"The ID of the VNI.";
}
leaf peer-ip {
type inet:ip-address-no-zone;
mandatory true;
description
"The address of the remote NVE.";
}
leaf direction{
type direction-type;
mandatory true;
description
"Traffic statistics direction for the tunnel.";
}
}
}
to
list statistic {
key "vni-id peer-ip direction"; ...
action reset-vni-peer-statistic {
description
"Clear traffic statistics about the VXLAN tunnel.";
}
Best Regards,
/jan
_______________________________________________
nvo3 mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/nvo3
