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 <kwat...@juniper.net >> <mailto:kwat...@juniper.net>> 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 >> netc...@ietf.org <mailto:netc...@ietf.org> >> 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 >> email@example.com >> https://www.ietf.org/mailman/listinfo/netmod > > Mahesh Jethanandani > mjethanand...@gmail.com <mailto:mjethanand...@gmail.com> > > > > _______________________________________________ > netmod mailing list > firstname.lastname@example.org > https://www.ietf.org/mailman/listinfo/netmod
Description: OpenPGP digital signature
_______________________________________________ netmod mailing list email@example.com https://www.ietf.org/mailman/listinfo/netmod