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

[Mahesh Jethanandani <[e-mail]>]

Kent,

Thanks for a detailed review. See inline.

> On Feb 13, 2018, at 2:30 PM, Kent Watsen <[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.

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