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

[Eliot Lear <[e-mail]>]

On 18.02.18 02:26, Mahesh Jethanandani 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.

Yes, I do.  The changes below are equally acceptable.

Thanks,

Eliot

>
>>
>>
>> 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.
>
>>
>> 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).
>
>>
>>
>> 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.
>
>>
>>  Second, am I reading it correct? - is the "Editorial Note" in the
>>  Abstract section.  I strongly advise moving
>
> Moved it to Introduction section.
>
>>
>> 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?
>
>>
>>
>> 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.
>
>>
>>
>> 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”
>
> 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]>
>
>
>
> _______________________________________________
> netmod mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netmod