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

[Mahesh Jethanandani <[e-mail]>]

> On Feb 23, 2018, at 12:12 PM, Kent Watsen <[e-mail]> wrote:
> 
> 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= <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= <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.

Unless I hear otherwise, I will remove it.

>>  
>> 
>> 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?

Kristian’s suggestion requires changing the module. It is not an editorial change. And that change will have an impact on the MUD draft, which has been sent for publication. 

>> 
>> 
>> 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.

But by saying:

reference “RFC 7223”

you are also implying import by revision, which is not what the module wants to do. The module wants to import the latest revision of ietf-interfaces, not necessarily the module in RFC 7223. 

I would suggest that we let the yang-doctors discussion on this topic decide how we are going to reference import statements.

>> 
>> 
>> 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 <https://xml2rfc.tools.ietf.org/xml2rfcFAQ.html#q_hanging_lists>
Done.

Cheers.

>  
>> 
>> 
>> 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= <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] <mailto:[e-mail]>
>> https://www.ietf.org/mailman/listinfo/netmod <https://www.ietf.org/mailman/listinfo/netmod>
>  
> Mahesh Jethanandani
> [e-mail] <mailto:[e-mail]>
>  

Mahesh Jethanandani
[e-mail]