Re: [netmod] ACL draft issues found during shepherd writeup

[Kent Watsen <[e-mail]>]

Hi Mahesh,

Please search for <KENT> below (6 instances)

Thanks,
Kent // shepherd


On 2/17/18, 8:26 PM, "Mahesh Jethanandani" <[e-mail]<mailto:[e-mail]>> wrote:

Kent,

Thanks for a detailed review. See inline.


On Feb 13, 2018, at 2:30 PM, Kent Watsen <[e-mail]<mailto:[e-mail]>> wrote:

[sorry, wrong WG, moving netconf to BCC!]


ACL Authors,

Below are some issues I found while looking at doing the Shepherd
write-up today.  Please take a look.

Also, with regards to the request for those having Last Call comments
to please verify that their comments were addressed, I only saw one
response from Kristian, but should we be expecting respeonses from
others too, perhaps Einar or Elliot?

Eliot can confirm if he feels his issues have been addressed.




1 IDNITS

 - some issues found by idnits
 - using https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_tools_idnits_&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=7Bx3hgofSFxvNRV7Xz7FuaJcKKfzEB0sBJzN_KOCtSg&s=_5f-lxCoJW2TidcrjW_KbDkdPhfxLoL67kn1A2okgs0&e=
 - without selecting "verbose output"


1.1

 ** There are 5 instances of too long lines in the document, the longest one
    being 5 characters in excess of 72.

Fixed.




 This "**" is being flagged as an "error".
 Idnits label, not mine.  Please fix.


1.2

 == There are 7 instances of lines with non-RFC6890-compliant IPv4 addresses
    in the document.  If these are example addresses, they should be changed.

 This is just a warning, but given that there are seven occurrences, it
 might be a good idea to fix.  Please see Section 3, point #6 in this
 document for details: https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_id-2Dinfo_checklist&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=7Bx3hgofSFxvNRV7Xz7FuaJcKKfzEB0sBJzN_KOCtSg&s=AYo8ZHPY4SAHMqcy1qV9yr7BjoxGy_C9zcJ_ZbwXBT4&e=.

Fixed.




1.3

 ** The document seems to lack a both a reference to RFC 2119 and the
    recommended RFC 2119 boilerplate, even if it appears to use RFC 2119
    keywords.

    RFC 2119 keyword, line 797: '...s-list. A device MAY restrict the leng...'


 There needs to be a section that looks like RFC 8174, paragraph 11:

    The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
    "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY",
    and "OPTIONAL" in this document are to be interpreted as
    described in BCP 14 [RFC2119] [RFC8174] when, and only when, they
    appear in all capitals, as shown here.

Added.




1.4.

 -- The document date (February 2, 2018) is 11 days in the past.  Is this
    intentional?

 This is fine, ignore it.

1.5

 ** Obsolete normative reference: RFC 2460

 This needs to be fixed.

Updated the reference to RFC 8200.



1.6

 ** Downref: Normative reference to an Historic RFC: RFC 3540

 Hmmmm, another HISTORIC document, but this time not due to an IESG
 action.  The question is how important this reference is, is this
 "ns" bit (ECN-nonce concealment protection) commonly used in the
 industry?

I do not know enough to know it is not used. If the consensus is that we do not use it, I can drop it from the model.

<KENT> As shepherd, I would like the normative reference to a historic RFC removed from this draft.   My recommendation is to remove it.  As chair, if anyone wants to make a case for keeping the "ns" bit, *now* is
your time to say something.


1.7

 == Outdated reference: A later version (-06) exists of
    draft-ietf-netmod-yang-tree-diagrams-04

 Please update to -06

This might be because the draft was last published when -04 was around. I do not reference any particular version. My reference is to
<?rfc include='reference.I-D.ietf-netmod-yang-tree-diagrams’?>. The tool pulls in the latest when it generates the draft.


1.8

 -- Obsolete informational reference (is this intentional?): RFC 5101
    (Obsoleted by RFC 7011)

 Please update to RFC 7011

Done.





2  YANG VALIDATION

2.1 Normative Modules

 All of the following passed:

   pyang --ietf ietf-access-control-list\@2018-02-02.yang
   pyang --ietf ietf-packet-fields\@2018-02-02.yang
   pyang --ietf ietf-ethertypes\@2018-02-02.yang

   yanglint -s ietf-access-control-list\@2018-02-02.yang
   yanglint -s ietf-packet-fields\@2018-02-02.yang
   yanglint -s ietf-ethertypes\@2018-02-02.yang

2.2 Example Module

 Example module passed `yanglint -s`, but not `pyang --lint`:

   yanglint -s example-newco-acl.yang
   pyang --lint example-newco-acl.yang

   example-newco-acl.yang:78: error: keyword "description" not in
   canonical order, expected "type", (See RFC 6020, Section 12)

   example-newco-acl.yang:79: error: keyword "type" not in
   canonical order, (See RFC 6020, Section 12)

   example-newco-acl.yang:82: error: keyword "default" not in
   canonical order, (See RFC 6020, Section 12)

 Please fix.

Fixed.




2.3 XML Examples from Section 4.3

 yanglint didn't find any issues:

   yanglint ietf-access-control-list\@2018-02-02.yang ex-4.3.xml


2.4 Examples from Section 4.4

 I had to stitch these into the 4.3 example.  It found one issue, a typo
 in the last closing tag in the first example in this section:

   yanglint ietf-access-control-list\@2018-02-02.yang ex-4.4++.xml
   err : Invalid (mixed names) opening (source-port-range-or-operator) and closing (tcp) element tags. (/data/access-lists/acl/aces/ace/matches/l4/tcp/source-port-range-or-operator/source-port-range-or-operator)

 Please fix.

Made them complete examples so you do not have to stitch them anymore. And made sure yanglint validated the examples before it includes it in the draft.




 PS: And this is not a shepherd directive, but I found the whole
     "source-port-range-or-operator" syntax clumsy.  I'm surprised
     it didn't look something like:

         OLD
               <source-port-range-or-operator>
                  <port-range-or-operator>
                    <range>
                      <lower-port>16384</lower-port>
                      <upper-port>65535</upper-port>
                    </range>
                  </port-range-or-operator>
               </source-port-range-or-operator>

               <source-port-range-or-operator>
                 <port-range-or-operator>
                   <operator>
                     <operator>eq</operator>
                     <port>21</port>
                   </operator>
                 </port-range-or-operator>
               </source-port-range-or-operator>

         NEW

               <source-port>
                 <range>
                   <lower>16384</lower>
                   <upper>65535</upper>
                 </range>
               </source-port>

               <source-port>
                 <operator>
                   <operator>eq</operator>
                   <port>21</port>
                 </operator>
               </source-port>

Did you try making the change in the model to see if it work? It will complain that <range> is already used within the container and that it cannot be repeated (for destination-port).

<KENT> No, I did not, nor do I intend to get that deep into it.  But I recall that Kristian made the same comment before, and was making pull requests before, so maybe he can suggest something?


3 Key Draft Sections


3.1 Abstract

 First, I'm unsure if that first "sentence" is properly worded, but I
 definitely think that it is a bit too much on the terse side.  Can you
 embellish it a little?

How about this:

OLD:

  This document describes a data model of Access Control List (ACL)

  basic building blocks.
NEW:

  This document describes a data model for Access Control List (ACL).
  ACL is a ordered-by-user set of rules, used to configure the forwarding
  behavior in device.  Each rule is used to find a match on a packet,
  and define actions that will be performed on the packet.

<KENT> good.


 Second, am I reading it correct? - is the "Editorial Note" in the
 Abstract section.  I strongly advise moving

Moved it to Introduction section.

<KENT> was it before in a <note> element?   It may have just been a rendering issue that made it look like it was par of the abstract.  If it was a <note> before, then that is actually a common use of the <note> element.  It doesn't really matter, the RFC Editor will remove the section anyway.


3.2 RFC Editor Note

 There is no request to replace "I-D.ietf-netmod-yang-tree-diagrams" with
 the final RFC assignment.

Added.



 You might want to add what the current date value used in the draft is
 (i.e., 2018-02-02).   PS: my draft build tools, which I think you're
 using, should set the value for you automatically if you put YYYY-MM-DD
 into the text.

Added text to replace the revision date in the model with the date the draft gets published.



3.3 Import statements missing references

 All import statements in all modules are missing reference statements
    - why wasn't this caught by the tools?!

 Please see rfc6087bis Section 4.7.

Adding reference implies import by revision, which we want to avoid, specially since we do not want to import by revision. Right?

<KENT> I wrote "reference" (not "revision").  A reference statement just specifies which RFC the imported module is defined in.


3.4 Security Considerations

 Please reformat the last paragraph so the "aces" path is more pronounced.
 Perhaps use hangText.

What is hangText? I italicized it.

<KENT> https://xml2rfc.tools.ietf.org/xml2rfcFAQ.html#q_hanging_lists



3.5 IANA Considerations

 This section is hard to read.

 Consideration breaking up the "XML" and the "YANG Module Names" registry
 requests into two subsections.

 Consider making the registration entry requests themselves artwork so
 they're line-spaced and indented as such.

 The first paragraph of the "XML" registry request says "a URI", but it
 should be "two URIs"

 The first paragraph of the "YANG Module Names" registry request says
 "a YANG module", but it should be "two YANG modules”

Split into two sections and upped the count of URIs and YANG models to three (was missing the ietf-ethertypes module).




3.6 References

 I haven't checked yet, but please verify that all the references are
 properly sorted as to being Normative or Informative.


3.7 Appendix A

 It took me awhile to figure out what I was looking at.  The tree-diagram
 is poorly indented and there is no text preceding the example module.

I have moved the example module after the first paragraph, that describes the module. Let me know if that looks ok.



 I recommend you fold the lines of your tree diagram at a certain column
 whilst adding a '\' character.  I've since added this ability to my draft
 build tools, let me know if interested in an update.  You might also want
 to look at draft-wu-netmod-yang-xml-doc-conventions.

Shortened the prefix so the augment statement fits within 72 columns.

BTW, I use 'pyang  -f tree —tree-line-length=69' to generate the tree. Plus I use fold -w 71 to fold the diagram, but I guess it does not work for augment statement.



 Also, please fix the example module's namespace per the end of
 rfc6087bis Section 4.9.

Updated the namespace to “http://example.com/ns/example-newco-acl<https://urldefense.proofpoint.com/v2/url?u=http-3A__example.com_ns_example-2Dnewco-2Dacl&d=DwMFaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=yWaNP3COBT8TgmVD5YCM5MT3RKbupJRm6BWCzPyiC4k&s=LcLSD4FXu5W7vqtE66AfFokTq8N66aosAOuBuO3G73I&e=>”

Cheers.





Thanks,
Kent




_______________________________________________
Netconf mailing list
[e-mail]<mailto:[e-mail]>
https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_netconf&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=7Bx3hgofSFxvNRV7Xz7FuaJcKKfzEB0sBJzN_KOCtSg&s=XknLqgAu3Z9t1ME6FO-_mZY2oCN61867VB0ubLeiv3Q&e=


_______________________________________________
netmod mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/netmod

Mahesh Jethanandani
[e-mail]<mailto:[e-mail]>