Subject: [yang-doctors] YANG doctor review of draft-ietf-dhc-dhcpv6-yang-03

From: Ladislav Lhotka <[email-masked]>
Date: Wed, 14 Sep 2016 16:14:29 +0200

Hi,

I was assigned to be the YANG doctor for this document. Here is my
review:

**** General comments

***** Data model organisation

The "ietf-dhcpv6" module covers schemas for DHCPv6 servers, clients and
relays. All these data are defined in the same main module, not in
"sub-modules" as the I-D says on p. 2 (unless we make a distinction
between "submodule" and "sub-module"). In my view, though, each of these
three parts should in fact be in a separate *main* module. I assume that
in many (most?) cases a particular managed device will play just one of
the three roles, so it should then advertise only the part of the data
model that's corresponds to its role. If multiple functions are
co-located in the same device, it can advertise multiple modules in YANG
library.

***** Descriptions

Many descriptions in the "ietf-dhcpv6" module provide no information
at all. This is quite typical:

    container oro-options {
        description "oro options";
        ...
    }

Such descriptions need to be expanded. In the case above, the
description should at least explain what "oro" means. In many cases,
it would also be useful to provide references to relevant RFCs and
other documents using the "reference" statement.

On the other hand, some descriptions (e.g., of "mo-tab" container) are
longer but difficult to read because of sloppy formatting. Giving these
descriptions more structure (paragraphs, itemised lists) would
certainly help.

Also, some of the descriptions are copied verbatim in Sec. 2. It may
prove difficult to keep this information synchronised in the future
revisions of the I-D. I'd suggest to trim the items in "Introduction
of important nodes" to a single sentence or so. The authoritative
description of each parameter can always be found in the module.

***** Metadata

My suggestion is to use the same or similar template for module
metadata as the existing NETMOD modules (e.g. in RFC 7223). In
particular:

- The I-D is intended for Standards Track, so the "contact" statement
  must also contain WG mailing-list and web information (see sec. 4.7
  of RFC 6087).

- Adding a new "revision" statement for every I-D revision becomes
  unwieldy. RFC 6087 permits to use just one "revision" statement
  throughout the I-D stage, and just bump the revision date in its
  argument each time after the module was changed. To keep track of
  the changes in the I-D stage, it is better to have a change log
  section in the I-D that will get removed just before the document
  becomes an RFC.

**** Specific comments

- Leafs that are list keys are mandatory by definition, the
  "mandatory" statement is not necessary for them.

- The "duidtype" type is an union with overlapping members. For
  example, in the XML serialisation it is impossible to distinguish
  between a decimal and hexadecimal number if the latter contains only
  digits. I suggest to prefix the hexadecimal string with "0x".

- The top-level nodes "server", "relay" and "client" are containers
  with presence, and their "presence" statement says "Enables server
  (relay, client)". But then each of them has the mandatory "enable"
  child. I think it would be better to define "true" as the default
  value for "enable" - then it has to be present with the "false"
  value only if the corresponding role is to be disabled while keeping
  the container in configuration.

- Each server option also has an "enable" leaf with identical
  definition, so this leaf should be defined in a grouping to avoid
  copying and pasting it to every option. Also, could this leaf have
  the default of "false" instead of being mandatory? The options that
  are not present then needn't appear in the configuration.

- Given the large number of options and their possible combination, I
  wonder whether "uint8" is enough as the type for "option-set-id".

- Are all the options required to be supported by every server
  implementation? If not, then defining corresponding features might
  be good.

- I don't understand how the "new-or-standard-option" is supposed to
  work. It seems it is there to allow for new options to be added in
  the future, but then I don't get why it is called
  "new-or-standard". More explanation in the description and an
  example in the text would be helpful.

- What is the purpose of the "ipv6-address" leafs in
  "server-attributes" and "relay-if"? How are they related to an IPv6
  address that can be configured via the "ietf-ip" module [RFC 7277]?

- Could the leaf-list "interfaces-config" under "serv-attributes"
  contain just leafref references to interfaces configured via
  "ietf-ip"?

-- 
Ladislav Lhotka, CZ.NIC Labs
PGP Key ID: E74E8C0C

---

From: Linhui Sun <[email-masked]>
Date: Sat, 17 Sep 2016 15:27:46 +0800

Hi Ladislav,

Really thanks for the review and valuable comments, we will modify the
model as you suggested. And for some comments, I'm not sure whether I've
understood them correctly, please see my feedbacks inline.

Best,
Linhui

> co-located in the same device, it can advertise multiple modules in YANG
[LH]: Yes, in most cases, they are on different devices. We will make them
as three separate modules in the next version.

> description of each parameter can always be found in the module.
[LH]: Do you mean that the “description” in the YANG module should be a
comprehensive one? It seems that in the current version, we just made them
diametrically opposed.

>   the container in configuration.
[LH]: If we make three separate main modules for server, relay and client,
do we still need the "presence" statements?

>   are not present then needn't appear in the configuration.
[LH]: Grouping is really a good suggestion. However, I’m just wondering
whether the “mandatory true/false” statement plays the same role as the
“enable” leaf? IMHO, if we could use the “mandatory true/false” statement
to let a option not appear in the configuration, we don’t need the “enable”
leaf at all.

>   wonder whether "uint8" is enough as the type for "option-set-id".
[LH]: Yes, you are right, existing implementations use at least uint32.
Will modify it.

>   implementation? If not, then defining corresponding features might
[LH]: Ideally, all the options should be supported by implementations, but
it is not realistic since not all the implementations will support
everything defined in the WG. And what does the “features” here refer to?

>   example in the text would be helpful.
[LH]: This container’s name is really ambiguous, the original idea is to
allow a generic format/way to define standard DHCPv6 options. As you can
see from the existing model, we suggest the administrator to use a detailed
list of options definition to configure the server. However, many popular
implementations do not support the detailed list, thus we decided to add
the “new-or-standard-option” container here, the “new” is for those new
options that will be standardized by the WG in the future.
And now the WG has some discussions on whether we need such multiple
alternatives (detailed list v.s. generic format) to configure options.
What’s your opinion on that? We are eager to hear from you from the
perspective of YANG modeling.

>   address that can be configured via the "ietf-ip" module [RFC 7277]?
[LH]: I think they are the same, so it would be better to remove them from
the existing model.

>   contain just leafref references to interfaces configured via
[LH]: The “interface-config” is required by the configuration of a
individual DHCPv6 server, I don’t know whether using references to another
model will add complexity to the mapping to implementations.

---

From: "Bernie Volz (volz)" <[email-masked]>
Date: Sat, 17 Sep 2016 11:10:15 +0000

In the past I think I asked for this to be a string name, not a number (uint). String id much more flexible & you can use numbers if you like.

- Bernie (from iPad)

On Sep 17, 2016, at 3:28 AM, Linhui Sun <[email-masked]<mailto:[email-masked]>> wrote:

- Given the large number of options and their possible combination, I
  wonder whether "uint8" is enough as the type for "option-set-id".
[LH]: Yes, you are right, existing implementations use at least uint32. Will modify it.

---

From: Ladislav Lhotka <[email-masked]>
Date: Mon, 19 Sep 2016 16:06:21 +0200

> comprehensive one? It seems that in the current version, we just made them

Some may be short but some deserve to be comprehensive. As an
approximate model, you can check RFC 6991. Basically, descriptions serve
three purposes:

- state requirements that cannot be captured formally, such as semantic
  constraints or dynamic default values;
- aid developers implementing the data model;
- provide help text that can be displayed in client user interfaces.

Anyway, a description containing just the data node name is not very
helpful.

...

> do we still need the "presence" statements?

Containers "server-attributes" and "relay-attributes" contain
mandatory leafs which makes these containers mandatory as well. If the top-level
nodes "server" and "relay" aren't containers with presence, they would
be mandatory, too, but this is not permitted (sec. 4.9 in RFC 6087). I'd
suggest to keep the "presence" statement for "server" and "relay", and
remove it from "client".

> to let a option not appear in the configuration, we don’t need the “enable”

I don't understand what you mean, could you give an example?. My
suggestion was to remove the "mandatory" statement from the "enable"
leaf definition and instead put

    default false;

there. Alternatively, you could add "presence" statements to the option
containers, and then the "enable" leafs wouldn't be needed.

> everything defined in the WG. And what does the “features” here refer

See sec. 5.6.2 in RFC 7950. Every server can then advertise supported
features, and if you add corresponding "if-feature" statements into
option containers, it won't be possible to configure unsupported options.

>From this perspective, the detailed list of options is still extensible
via the augment statement so the generic format seems unnecessary. If a
NETCONF/RESTCONF server is used with a DHCPv6 server that doesn't
support the detailed list, it can translate the options configured via
the detailed list into the generic format, thus hiding the differences
in DHCPv6 server implementations.

> model will add complexity to the mapping to implementations.

If "ietf-interfaces" and "ietf-ip" are prerequisite for this module,
then it makes sense to make sure that addressed used in the latter are
indeed configured on interfaces.

Lada