Re: [dhcwg] Yangdoctors early review of draft-ietf-dhc-dhcpv6-yang-19

["Bernie Volz (volz)" <[e-mail]>]

Also, in the -20 there are some issues:

     typedef duid-llt {
       type duid-base {
         pattern '^0001' '0001'
           + '[0-9a-fA-F]{12,}';
       }
       description
         "DUID type 1, based on Link-Layer Address Plus Time
         (DUID-LLT). Constructed with a 2-byte hardware type assigned
         by IANA,  4-bytes containing the time the DUID is generated
         (represented in seconds since midnight (UTC), January 1, 2000,
         modulo 2^32), and a link-layer address. The address is encoded
         without separator characters. For example:

         +------+----+----------+--------------+
         | 0001 | 06 | 28490058 | 00005E005300 |
         +------+----+----------+--------------+

         This example includes the 2-octet DUID type of 1 (0001), the
         hardware type is 06 (IEEE Hardware Types) the creation
         time is 0x284900580 (constructed as described above). Finally,
         the link-layer address is 00005E005300 (EUI-48 address
         00-00-5E-00-53-00)";

Probably best if the hardware type (which is a 2-byte field) be written as 0006, not 06 (as 0001 is used for type).

Note also that bytes and octet seem to be mixed.  There are 5 uses of "byte" in the document and 16 uses of "octet"; I'd suggest replacing all "byte" with "octet".

Similarly, for duid-en, 0009 is used as vendor id when it should be 00000009 to indicate that it is a 4-octet field. (I think if you want to avoid leading 0's to not indicate the size of the field, you should do so consistently; otherwise use the leading 0s for hex values to indicate their size?)

And duid-ll has same issue with hardware type as duid-llt.

- Bernie

-----Original Message-----
From: t petch <[e-mail]> 
Sent: Thursday, June 3, 2021 7:43 AM
To: [e-mail]
Cc: [e-mail]; [e-mail]; [e-mail]
Subject: Re: [dhcwg] Yangdoctors early review of draft-ietf-dhc-dhcpv6-yang-19

----- Original Message -----
From: <[e-mail]>
Sent: Tuesday, June 01, 2021 5:48 PM

> Hi Acee,
>
> Thanks again for your review. My apologies for how long to work
through them all and prepare the update. I've just posted -20 which aims to address your comments.
>
> Please see inline below.
>
> Additionally in this version, there are some small errors corrected
(typos, incorrect regexs) and the example YANG modules have been renamed and have namespaces according to RFC847.

Ian

Piggybacking on your note to Acee...

When I saw the revised prefixes I thought 'I wonder if the IANA Considerations have been updated'!

The revision statement for the RFC-to-be needs the publication date, a change which the RFC Editor is now well used to, and needs to say "Initial Revision" in all the modules and  reference of "RFC XXXX: YANG Data Model for
DHCPv6 Configuration" i.e. the I-D current title; again, the RFC Editor knows what to do with XXXX.

The description of 'typedef duid-base' looks corrupt.

Tom Petch

> BR,
> Ian
>
> >> On 5. May 2021, at 23:32, Acee Lindem via Datatracker
<[e-mail]> wrote:
> >>
> >> Reviewer: Acee Lindem
> >> Review result: On the Right Track
> >>
> >> Document: draft-ietf-dhc-dhcpv6-yang-19
> >> Reviewer: Acee Lindem
> >> Review Date: May 5, 2021
> >> Review Type: Early Review
> >> Intended Status: Standards Track
> >> Summary: On the right track - Issues and questions need to be
resolved.
> >>
> >> Modules: [e-mail]
> >>         [e-mail]
> >>         [e-mail]
> >>         [e-mail]
> >>
> >> Tech Summary: The document contains the base configuration and
operational
> >>              YANG model for the DHCPv6 protocol. The basic
structure is
> >>              very good but the major issues need to be addressed
prior to WG
> >>              last call.
> >>
> >> Major Comments:
> >>
> >>    1. Should the DHCP server, relay, and client functions be
enabled by
> >>       default? It seems they are require specific configuration to
be
> >>       viable.
>
>
> [if - I've removed 'default enabled' from server and relay and left it 
> present for client, as discussed]
>
> >>
> >>    2. The threshold type in the ietf-dhcpv6-common is strange. It
is a
> >>       union with an enumeration to disable the threshold. Normally,
if
> >>       there is no threshold, you would simply not specify it.
However,
> >>       the data nodes of this type are mandatory. I'd make it a
simple
> >>       type and remove the mandatory designations for the data
nodes. Also,
> >>       the range should not start at 0% since this % makes no sense.
>
> [if - Removed enum and changed type to uint8 Removed 'mandatory true' 
> from the data nodes in the server module Changed range to 1..100]
>
> >>
> >>    3. There are examples of augmenting the ietf-dhcpv6-server
module but
> >>       no "Module Usage Examples" as specified in section 3.12 of
RFC 8407.
>
> [if - Added Appendix A with XML examples for all of the element 
> modules]
>
> >>
> >> Minor Comments:
> >>
> >>    1. While not required by RFC 8407, many YANG RFCs explcitly call
out
> >>       the interaction with imported YANG modules in a separate
section.
>
> [if - I've extended the description in the introduction to describe 
> interactions]
>
> >>
> >>    2. No sense in maintaining all the intermediate revisions in the
> >>       modules. Just update the one that is the initial version and
update
> >>       the date.
>
> [if - Removed]
>
> >>
> >>    3. The module prefixes are very descriptive but a bit long.
Given
> >>       the examples of augmentations, this will be especially true
for
> >>       DHCPv6 server augmentations. Perhaps, dhc6-serv, dhc6-rly,
and
> >>       dhc6-clnt would be better.
>
> [if - Changed. 'dhcpv6-common' has also been shortened to 'dhc6']
>
> >>
> >>    4. Can host-reservation prefixes overlap with holes? If so,
> >>       reserved-prefix may not be unique. If not, no problem.
>
> [if - For the DHCP server implementations that I am familiar with, the 
> prefixes will be checked when config is applied and any prefix 
> overlaps will be rejected as invalid.]
>
> >>
> >>    5. For nodes with patterns, describe what the pattern allows in
> >>       the description with an example or two. This applies to
> >>       link-address, duid-base, duid-llt, duid-en, duid-ll,
> >>       duid-unstructured, and sub-option-data.
>
> [if - added examples in the description fields]
> >>
> >>    6. With respect to link-address, what type of address is this?
If it is
> >>       an IPv6 link-local address, there is an ipv6-adddress type in
RFC 6021.
>
> [if - link-address should be a GUA. I've changed the type to
ipv6-address.]
>
> >>
> >> Nits:
> >>
> >>    1. IETF documents should use US English - not UK English. I've
> >>       changed in suggested edits.
>
> [if - Incoporated the proposed changes, see below]
> >>
> >>    2. Description format - Sometimes starting right have
"description"
> >>       and sometimes starting on the next line.
>
> [if - Moved description text to start on the next line throughout.]
>
> >>
> >>    3. sol-max-rt-option-group and inf-max-rt-option-group should
spell out
> >>       the words in the description rather than just repeating the
short
> >>       abreviations.
>
> [if - Changed]
>
> >>
> >>    4. In ietf-dhcpv6-client,  for ia_ta and ia_pd, spell our
> >>       acronyms in the descriptions rather than just repeating them
(which
> >>       is useless).Is "ia" "interface address"? Don't make the
reader
> >>       go to the DHCPv6 RFC to know what you mean. What is "ia_ta"?
>
> [if - Added expanded version in the description for each IA (e.g.
IA_PD (Identity
> Association for Prefix Delegation)).]
>
> [if - for the remaining diffs, I've incorporated them exactly as
suggested, with the
> Exception of one comment below:
>
> >> ***************
> >> *** 1216,1222 ****
> >>             path
"/dhcpv6-server/option-sets/option-set/option-set-id";
> >>           }
> >>           description "The ID field of relevant set of DHCPv6
options
> >> !            (option-set) to be provisioned to clients of this
> >>             network-range.";
> >>         }
> >>         leaf valid-lifetime {
> >> --- 1218,1224 ----
> >>             path
"/dhcpv6-server/option-sets/option-set/option-set-id";
> >>           }
> >>           description "The ID field of relevant set of DHCPv6
options
> >> !            (option-set) to be provisioned to clients of the using
> >>             network-range.";
> >>         }
> >>         leaf valid-lifetime {
>
> [if - I'm not sure if the intended change here is correct. I've 
> changed the wording to 'clients using the network-range.']
>
> _______________________________________________
> dhcwg mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/dhcwg
> .
>
>