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

[Mahesh Jethanandani <[e-mail]>]

Kent,

> On Mar 7, 2018, at 1:55 PM, Kent Watsen <[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

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

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

>  - 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/)

>  - "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).

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

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

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

>  - "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.

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

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

>  - does the second example provide any value of the first? - seems the same to me…

Will change the example.

>  - 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
> 
> 
> ===== 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] 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 <http://tools.ietf.org/>.
>> 
>> 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]