Re: [netmod] I-D Action: draft-ietf-netmod-acl-model-17.txt

[Mahesh Jethanandani <[e-mail]>]

> On Mar 13, 2018, at 3:23 PM, Kent Watsen <[e-mail]> wrote:
> 
> Hi Mahesh,
>  
> Please look for <KENT> below.
>  
> Thanks,
> Kent
>  
>  
> On 3/8/18, 7:40 PM, "Mahesh Jethanandani" <[e-mail] <mailto:[e-mail]>> wrote:
>  
> Kent,
> 
> 
>> On Mar 7, 2018, at 1:55 PM, Kent Watsen <[e-mail] <mailto:[e-mail]>> 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, [e-mail] <mailto:[e-mail]> 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
>>> [e-mail] <mailto:[e-mail]>
>>> 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
>> [e-mail] <mailto:[e-mail]>
>> 
>> _______________________________________________
>> netmod mailing list
>> [e-mail] <mailto:[e-mail]>
>> 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
> [e-mail] <mailto:[e-mail]>
>  

Mahesh Jethanandani
[e-mail]