> On Mar 13, 2018, at 3:23 PM, Kent Watsen <kwat...@juniper.net> wrote:
> 
> Hi Mahesh,
>  
> Please look for <KENT> below.
>  
> Thanks,
> Kent
>  
>  
> On 3/8/18, 7:40 PM, "Mahesh Jethanandani" <mjethanand...@gmail.com 
> <mailto:mjethanand...@gmail.com>> wrote:
>  
> Kent,
> 
> 
>> On Mar 7, 2018, at 1:55 PM, Kent Watsen <kwat...@juniper.net 
>> <mailto:kwat...@juniper.net>> wrote:
>>  
>> [To all those that said this draft was ready, really?]
>> 
>> 
>> Hi Mahesh,
>> 
>> Thanks for the update.  I found some more issues.  Some must be fixed, 
>> others are nits, and might be caught by the RFC Editor.  But I think
>> that it's embarrassing to receive comments for such things from the 
>> IESG, as is recently the case for the syslog draft, so please see 
>> what you can do.
>> 
>> Thanks,
>> Kent
>> 
>> 
>> From Idnits:
>> 
>>  ** There are 6 instances of too long lines in the document, the longest one
>>     being 7 characters in excess of 72.
>  
> Hmm. The idnits at submission time did not complain. Will apply the new 
> script that you provided to make sure I wrap them around.
> 
> 
>> 
>>  You wrote before that it was "Fixed", but it's still here?  Note: "**" is
>>  an error (idnits label)
>> 
>>  -- The document has examples using IPv4 documentation addresses according
>>     to RFC6890, but does not use any IPv6 documentation addresses.  Maybe
>>     there should be IPv6 examples, too?
>> 
>>  I don't feel strongly about this, but if it's easy enough to do...
>> 
>> In the Abstract:
>>  - I think the word "an" is missing (e.g., an ACL)
>  
> Added.
> 
> 
>> 
>> In the Introduction:
>>  - should "ordered-by-user" be "ordered-by user" to avoid confusion, or 
>> perhaps say it another way?
>  
> How about this in both the Abstract and the Introduction.
>  
> OLD:
> ACL is a ordered-by-user set of rules
>  
> NEW:
> An ACL is a set of rules, in an order set by the user
>  
> <KENT>or how about "An ACL is a user-ordered set of rules”?

Ok.

> 
>  
>>  - what does "a tuple of" mean?  Can this be restated?
>  
> How about this?
>  
> OLD:
> The match criteria consist of a tuple of packet header match criteria and can 
> have metadata match criteria as well.
>  
> NEW:
> The match criteria consist of packet header matches, and or or metadata as 
> described below:
>  
> <KENT>or how about "The match criteria can be a multiplicity of criteria, all 
> of which must be true for the match to occur.   The match criteria may match 
> against values in the packet header or against vendor-specific metadata about 
> the packet."?   - or something in between?

Or simply as:

“The match criteria allows for definition of packet headers and metadata, all 
of which must be true for the match to occur."

>  
>  
>  
>>  - s/In case vendor supports it/In case a vendor supports it/ ?
>  
> Ok.
> 
> 
>>  - "The list of X is endless depending on...".  Is "endless" the right word, 
>> perhaps restate?
> OLD:
> The list of potential actions is endless
>  
> NEW:
> The list of potential actions is limitless
>  
> <KENT> or maybe "unbounded”?

Ok.

>  
>  
>>  - same sentence as above, should "networked devices" be "network" or 
>> "networking" devices?
>  
> Will change “networked devices” to “networking devices”.
> 
> 
>> 
>> In Section 3:
>>  - "A network system usually have a list of ACLs"  (s/system/systems/ or 
>> s/have/has/?)
>  
> s/have/has/.
> 
> 
>>  - "The match criteria consist of packet header matching" - is consist the 
>> right word?
>  
> How would you restate it? (After I have s/consist/consists/)
>  
> <KENT> see above (my comment before last, it is the same sentence, right?)

Once we agree on the above comment, I will replicate it.

> 
> 
>>  - "It as also possible for ACE to match on metadata"  s/as/is/ and s/ACE/an 
>> ACE/
>  
> Ok
> 
> 
>>  - "When applied to interfaces of a networked device, the ACL is applied in 
>> a direction
>>     which indicates if it should be applied to packet entering (input) or 
>> leaving the
>>     device (output)."  - restate to talk about "ingress" and "egress”?
>  
> How about:
>  
> When applied to interfaces of a networked device, the ACL is applied in a 
> direction which indicates if it should be applied to ingress interface 
> (input) or egress interface (output).
> 
> <KENT>or maybe "When applied to interfaces of a networked device, distinct 
> ACLs are defined for the ingress (input) and egress (output) directions.”

Ok.

>  
>  
>>  - "An example in the appendix shows how to express it in YANG model." - 
>> either this
>>    is not true, or the sentence should not be at the end of this paragraph
>  
> Removed.
> 
> 
>> 
>> In Section 3.1:
>>  - s/and must statements/and 'must' statements/
>  
> Done and s/if-feature/‘if-feature’/
> 
> 
>>  - s/define new "matches" choice/define a new "matches" choice/ ?
>  
> Done.
> 
> 
>> 
>> In Section 4.1:
>>  - "ietf-access-control-list" is the standard top level module for access 
>> lists
>>      - what does this mean?
>  
> OLD:
> "ietf-access-control-list" is the standard top level module for access lists
>  
> NEW:
> "ietf-access-control-list" is the top level module for access lists
> 
> <KENT> it's more than the word "standard".  Maybe something like this:  The 
> "ietf-access-control-list"
> module defines a container called "access-list"  - what do you think?  

Ok.

>  
> <KENT>BTW, why is the container called "access-lists" and not e.g., "acls".  
> I thought that there was a node-naming idiom along the lines of 
> "/widgets/widget" for when a list is a descendent of a container.

History. When we inherited the draft, it was named access-lists. I can change 
it to “acls”.

>  
>  
>  
>>  - The "access-lists" container stores a list of "acl". - s/stores/has or 
>> contains?/ 
>  
> s/stores/has/
> 
> 
>>  - "...that can be used to determine which rule was matched upon" - not sure 
>> if this
>>    part is needed, or maybe better restated ", which can later be used to 
>> determine…"?
>  
> Ok.
> 
> 
>>  - s/ability for ACL's to be/ability for ACLs to be/
>  
> Ok.
> 
> 
>> 
>> In Section 4.1 (in the YANG module):
>>  - A number of identities read "ACL that primarily matches...".  Is 
>> "primarily"
>>    an accurate word? - if so, then do we need to say anything about when it's
>>    not the case? 
>  
> As one of the text says. It primarily matches IPv4, and does not match either 
> ethernet or IPv6 headers. Such ACL types are different from the mixed ACL 
> types that might match on a combination of ethernet and IPv4 headers etc.
>  
> <KENT>But my comment is more that "primarily" seems wishy-washy.  It seems 
> like it doesn't *primarily* do something, it actually does it.   If there is 
> a grey area, where it might match something else, if possible, maybe it would 
> help to call that out?

Ok. Will drop the word “primarily”.

> 
> 
>>  Separately, s/ACL/an ACL/?
>  
> Ok.
> 
> 
>>  - A number of features read "Device can support..." - s/Device/The device/?
>  
> Ok.
> 
> 
>>  - "It can have one or more Access Control Lists" - lists should be singular.
>  
> Really? English grammar says that if a sentence has both a singular and a 
> plural, the one nearest to the subject is the one you select.
>  
> <KENT>actually, I'm just going off the fact that the list node is call "acl", 
> which is singular.  Perhaps even better would be to say:  It can contain one 
> or more "acl" nodes - thoughts?

But even nodes is a plural. So what would be the difference between “acls” and 
“acl nodes”. I would rather have the RFC editor deal with this.

>  
>  
>>  - "An Access Control List(ACL)" - put a space before (ACL)
>  
> Ok.
> 
> 
>>  - " Indicates the primary intended" - here's that word "primary" again...
>>  - s/a list of access-list-entries(ACE)/ a list of access-list-entry nodes 
>> (ACE)/?
>  
> Ok.
> 
> 
>>  - s/List of access list entries(ACE)/List of access list entry nodes (ACE)/?
>>      - there is more than one instance of this in the model
>  
> Fixed.
> 
> 
>>  - "../../../../type" - still some long relative XPaths
>  
> Fixed.
> 
> 
>>  - " or referring to a group of source ports" - this isn't there yet.  I 
>> think you
>>    want to say something like "this is a choice so as to support future 
>> 'case'
>>    statements, such as one enabling a group of source ports to be referenced”
>  
> How about:
>  
> Choice of source port definition using range/operator or referring to a group 
> of source ports, to be added as a future 'case' statement.
>  
> <KENT>I like my framing better because 1) it is less committal about the 
> future and 2) it doesn't limit there to being just one 'case' statement that 
> might be added in the future.

Ok.

>  
>  
>>  - ditto for "or referring to a group of destination ports."
>>  - ditto on both of the above for the "udp" container
>>  - is it possible for both "egress-interface" and "ingress-interface" leafs 
>> to 
>>    be specified at the same time?  - if not, should there a 'must' statement 
>> to
>>    prevent that possibility? - or an explanation for what happens if it 
>> occurs?
>  
> Let me discuss this with my co-authors.
>  
> <KENT>any update on this?

Yes, it is possible for both the “egress-interface” and “ingress-interface” to 
be specified at the same time.

> 
>>  - s/The ACL's applied/The ACLs applied/   (this happens more than once in 
>> model)
>  
> Fixed.
>  
>> 
>> In Section 4.2:
>>  - references them by "uses" --> references them by 'uses' statements  ???
>  
> Ok.
> 
> 
>>  - not all your 'reference' statements have the title of the referenced 
>> document.
>  
> Fixed.
> 
> 
>>  - "then the datagram must be destroyed" - s/destroyed/dropped/?
>  
> Ok.
> 
> 
>>  - "or referring to a group of ..."  - same comments as for previous module
>>  - "ece" is missing a 'reference' statement?  - 
>  
> Added.
> 
> 
>>  - "Indicates that the Urgent pointer field is significant" - urgent is
>>    capitalized, but there's no context as for why.  Perhaps missing a
>>    reference statement too?
>  
> Added a reference statement.
> 
> 
>>  - in "window-size" leaf description, remove parentheses
>  
> Ok.
> 
> 
>> 
>> In Section 4.3:
>>  - the text says that it drops traffic from X to Y, but the example seems to 
>> do
>>    the reverse.
>  
> Fixed.
> 
> 
>> 
>> In Section 4.4:
>>  - The "With the follow XML example:" <EXAMPLE> "This represents..." is 
>>    difficult to read.  How about just having "The following XML example 
>> ...:”?
>  
> Fixed.
>  
>  
> <KENT> BTW, I missed it before, but I think the 4.4 section title should be
> plural: "Port Range Usage Examples”

The title now reads:

"Port Range Usage and Other Examples”. See below.

> 
>  
>>  - does the second example provide any value of the first? - seems the same 
>> to me…
>  
> Will change the example.
>  
> <KENT> was it changed per the next item below, or something else?

Changed the example to an ACE entry that drops all ping requests.

> 
>>  - seems like example 3 could also be expressed as 
>> "<lower-port>21</lower-port>",
>>    right?  - the text at the beginning of the section says this construct is
>>    possible, but there is no example for it.  Maybe this makes a better ex 
>> #2?
>  
> Have changed the language in the beginning of the section to say:
>  
> "When only a port is present, it represents a port, with the operator 
> specifying the range."
>  
> That is because, it now a choice between specifying a range or specifying a 
> single port with an operator.
> 
> 
>> 
>> In all your YANG modules:
>>  - replace "NETMOD (NETCONF Data Modeling Language)" with "NETMOD (Network 
>>    Modeling) Working Group”
>  
> Ok.
> 
> 
>> 
>> In Section ??:
>>  In the examples, why did you add the "<?xml version="1.0" 
>> encoding="UTF-8"?>"
>>  line and the "config" element?  - the examples validate equally well when
>>  these are removed.
>  
> The examples can then be cut and pasted into any client such as ncclient 
> which takes an entire <rpc>.
> 
> 
>> 
>> In Section 6:
>>  - s/three YANG module/three YANG modules/
>  
> Fixed.
> 
> 
>> 
>> In Section 6.1:
>>  - The first paragraph says "three URI", but it should be "three URIs”
>  
> Fixed.
> 
> 
>> 
>> In Section A.1:
>>  - "The following figure is the tree structure" - should say "tree diagram" 
>> and
>>    should reference the tree-diagrams draft, or else have a draft-wide "Tree
>>    Diagram Notation" section in the Introduction.
>  
> Added a section in the Introduction.
> 
> 
>>  - s/In other example/In another example/?
>>  - s/with new choice of actions/with a new choice of actions/?
>  
> Both fixed.
> 
> 
>> 
>> In Section A.3;
>>  - some 'reference' statements are missing titles
>  
> Added.
> 
> 
>>  - some 'description' statements might benefit from a 'reference’ statement
>  
> I have added references that I could find.
> 
> 
>>  - "The uint16 type placeholder type..." - is this a typo?
>  
> Dropped the second “type”.
>  
> Thanks.
> 
> <KENT>np
>  
>  
>  
> Kent // shepherd
>  
>> 
>> 
>> 
>> ===== original message ======
>> 
>> This version of the draft addresses comments raised during LC, shepherd 
>> review and other comments received during that period.
>> 
>> 
>>> On Mar 3, 2018, at 2:13 PM, internet-dra...@ietf.org 
>>> <mailto:internet-dra...@ietf.org> wrote:
>>> 
>>> 
>>> A New Internet-Draft is available from the on-line Internet-Drafts 
>>> directories.
>>> This draft is a work item of the Network Modeling WG of the IETF.
>>> 
>>>       Title           : Network Access Control List (ACL) YANG Data Model
>>>       Authors         : Mahesh Jethanandani
>>>                         Lisa Huang
>>>                         Sonal Agarwal
>>>                         Dana Blair
>>> Filename        : draft-ietf-netmod-acl-model-17.txt
>>> Pages           : 57
>>> Date            : 2018-03-03
>>> 
>>> Abstract:
>>>  This document defines 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.
>>> 
>>> 
>>> The IETF datatracker status page for this draft is:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__datatracker.ietf.org_doc_draft-2Dietf-2Dnetmod-2Dacl-2Dmodel_&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=44GJlqxqB0YK5G9gb1TUzAobugMHxDWflaPCZ3IYpKA&e=
>>>  
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__datatracker.ietf.org_doc_draft-2Dietf-2Dnetmod-2Dacl-2Dmodel_&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=44GJlqxqB0YK5G9gb1TUzAobugMHxDWflaPCZ3IYpKA&e=>
>>> 
>>> There are also htmlized versions available at:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Dacl-2Dmodel-2D17&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=rbm91SSJ_0sxFxb692d0FH0G-dbBTAUCf2KRySyztJQ&e=
>>>  
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Dacl-2Dmodel-2D17&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=rbm91SSJ_0sxFxb692d0FH0G-dbBTAUCf2KRySyztJQ&e=>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__datatracker.ietf.org_doc_html_draft-2Dietf-2Dnetmod-2Dacl-2Dmodel-2D17&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=siypyBn3F8o6bsB3Z3E5qS0uaSq2EUGUPwirx_a_KDw&e=
>>>  
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__datatracker.ietf.org_doc_html_draft-2Dietf-2Dnetmod-2Dacl-2Dmodel-2D17&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=siypyBn3F8o6bsB3Z3E5qS0uaSq2EUGUPwirx_a_KDw&e=>
>>> 
>>> A diff from the previous version is available at:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_rfcdiff-3Furl2-3Ddraft-2Dietf-2Dnetmod-2Dacl-2Dmodel-2D17&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=t2lpzSSW72BvQK1VjPoxX0ADxhb9ZD0fp3fXqcd80g8&e=
>>>  
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_rfcdiff-3Furl2-3Ddraft-2Dietf-2Dnetmod-2Dacl-2Dmodel-2D17&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=t2lpzSSW72BvQK1VjPoxX0ADxhb9ZD0fp3fXqcd80g8&e=>
>>> 
>>> 
>>> Please note that it may take a couple of minutes from the time of submission
>>> until the htmlized version and diff are available at tools.ietf.org 
>>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__tools.ietf.org_&d=DwMFaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=BgyjnfSrZfswWrTMiW-PdKEJUEl3IXtwCSo1PQyVUaA&s=4FjGOld49GwI0moZ7h6ltluv0RXN1rPmGp0d-8mjDmA&e=>.
>>> 
>>> Internet-Drafts are also available by anonymous FTP at:
>>> https://urldefense.proofpoint.com/v2/url?u=ftp-3A__ftp.ietf.org_internet-2Ddrafts_&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=xP7z9VxUgtOtSVIgqPF5RKIqTOi6wj-HEXvZKBRTiUw&e=
>>>  
>>> <https://urldefense.proofpoint.com/v2/url?u=ftp-3A__ftp.ietf.org_internet-2Ddrafts_&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=xP7z9VxUgtOtSVIgqPF5RKIqTOi6wj-HEXvZKBRTiUw&e=>
>>> 
>>> _______________________________________________
>>> netmod mailing list
>>> netmod@ietf.org <mailto:netmod@ietf.org>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_netmod&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=OKIVLXLo0Rsrf1DSoLWSyHj97DuE6vuaJ4Cqk_oi1HA&e=
>>>  
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_netmod&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=OKIVLXLo0Rsrf1DSoLWSyHj97DuE6vuaJ4Cqk_oi1HA&e=>
>> Mahesh Jethanandani
>> mjethanand...@gmail.com <mailto:mjethanand...@gmail.com>
>> 
>> _______________________________________________
>> netmod mailing list
>> netmod@ietf.org <mailto:netmod@ietf.org>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_netmod&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=OKIVLXLo0Rsrf1DSoLWSyHj97DuE6vuaJ4Cqk_oi1HA&e=
>>  
>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_netmod&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=9zkP0xnJUvZGJ9EPoOH7Yhqn2gsBYaGTvjISlaJdcZo&m=huBe-BRKk8B5XCRf7lG_gWUwZNr67SAA8GHfgYnyZoc&s=OKIVLXLo0Rsrf1DSoLWSyHj97DuE6vuaJ4Cqk_oi1HA&e=>
>  
> Mahesh Jethanandani
> mjethanand...@gmail.com <mailto:mjethanand...@gmail.com>
>  

Mahesh Jethanandani
mjethanand...@gmail.com

_______________________________________________
netmod mailing list
netmod@ietf.org
https://www.ietf.org/mailman/listinfo/netmod

Reply via email to