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

Mahesh Jethanandani <mjethanandani@gmail.com> Sun, 18 February 2018 01:20 UTC

Return-Path: <mjethanandani@gmail.com>
X-Original-To: netmod@ietfa.amsl.com
Delivered-To: netmod@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id B530B12D872; Sat, 17 Feb 2018 17:20:52 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -0.099
X-Spam-Level:
X-Spam-Status: No, score=-0.099 tagged_above=-999 required=5 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Y1BpHA6d8PBf; Sat, 17 Feb 2018 17:20:49 -0800 (PST)
Received: from mail-pl0-x231.google.com (mail-pl0-x231.google.com [IPv6:2607:f8b0:400e:c01::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 7FFEB12D874; Sat, 17 Feb 2018 17:20:49 -0800 (PST)
Received: by mail-pl0-x231.google.com with SMTP id p5so3731722plo.12; Sat, 17 Feb 2018 17:20:49 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=psFK7Viwdpj3ykUPulgDUPGbYJzcv6ulDIL7UWa2ykg=; b=fcOsVIK27EH9l3Wu+zURiWO+luVdS0koA7wrgWLxfrGr2B3DpwtJbQqA+w33ulgh7B 6W+46xy0QudP0Eece9z/7cxkj6utpyPAPHcjwmGLkJuUubWBkDekS6Nlz1hYZf+S3n2U 2qU4SoVQG1rz0ManwSrqNfbIec8HsuWX8Fh0Jbp/8ouGn+emeHiQbOgHkpO6twzormIR 5LEGvKYuU266brCYEivKO/2hHdlCHIGT58JfFs0fiN0eE4cvGrmxRHPzZ0/SvKLLACBE Zv78UUXbKwTU+Lbx70mrE2cz2O9W4mln4C+oxYT9EcuBflKOVywDE0FQPI7UCxpaPKvJ tudg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=psFK7Viwdpj3ykUPulgDUPGbYJzcv6ulDIL7UWa2ykg=; b=SJWlaA2q5ZJ5c8g5LS0cdM8ZtoeiTaq0gd2LsV8XA/RhXvpIoO+6Vp+Zkcy5W2phuo HZdsNJdNgLtzcX+F/MSlxEf4Yzho2jE1KyHmv5nfHJXXOLhGoDNV4QuGTG6bKAJzt+He y3rl2h5l4ZnnqEioAXdnmv38JO4/fjo3cNYCpfZjlA6uoY7TIgmarM6N1y1NnYrVEl6w CBqtlT1u0gqjn6QrNFPlEpJ/ohIUkv+np4UxsENCe59D/XO9DcOGR6ZBFSlzT6GW/iTR usBO5BdNhxdfXxlw8n9fCUKe/ZAHJ71KF4yaD6D5lNC3U0nqN+7WRfCUiIFp3e4a7Dyu COJA==
X-Gm-Message-State: APf1xPBtncb4dOOYF3A6aGFZqdzz5XDe7yTIIYDvK3s+cVeCLf6+sfpo S1X1J7b44S3coXb5xWR42S8zu9Me
X-Google-Smtp-Source: AH8x2241tmklhZ/MKdhNct/L6rMpiT2h4rtAiZUah7frJEZoJhFrYTqp7kJJFuiyiUkBGYEnDryftw==
X-Received: by 2002:a17:902:f81:: with SMTP id 1-v6mr9817381plz.265.1518916848798; Sat, 17 Feb 2018 17:20:48 -0800 (PST)
Received: from ?IPv6:2601:647:4700:1280:91a0:5e0b:a8bb:ae? ([2601:647:4700:1280:91a0:5e0b:a8bb:ae]) by smtp.gmail.com with ESMTPSA id k13sm9427042pfk.22.2018.02.17.17.20.47 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 17 Feb 2018 17:20:48 -0800 (PST)
From: Mahesh Jethanandani <mjethanandani@gmail.com>
Message-Id: <2864E0CF-D038-4FDA-B69C-FD43F486BF17@gmail.com>
Content-Type: multipart/alternative; boundary="Apple-Mail=_B437985A-7462-4213-8708-2A8397C4C07C"
Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\))
Date: Sat, 17 Feb 2018 17:26:13 -0800
In-Reply-To: <14BA9086-69D4-4BAF-A7C7-0EB1F3F400BB@juniper.net>
Cc: "draft-ietf-netmod-acl-model@ietf.org" <draft-ietf-netmod-acl-model@ietf.org>, "netmod@ietf.org" <netmod@ietf.org>
To: Kent Watsen <kwatsen@juniper.net>
References: <14BA9086-69D4-4BAF-A7C7-0EB1F3F400BB@juniper.net>
X-Mailer: Apple Mail (2.3445.5.20)
Archived-At: <https://mailarchive.ietf.org/arch/msg/netmod/Rxxhl2IH2BHZbcUE0FoaAkzE1mI>
Subject: Re: [netmod] ACL draft issues found during shepherd writeup
X-BeenThere: netmod@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: NETMOD WG list <netmod.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netmod>, <mailto:netmod-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netmod/>
List-Post: <mailto:netmod@ietf.org>
List-Help: <mailto:netmod-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netmod>, <mailto:netmod-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sun, 18 Feb 2018 01:20:53 -0000

Kent,

Thanks for a detailed review. See inline.

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

> 
> 
> 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=xGy_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
> Netconf@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
> netmod@ietf.org
> https://www.ietf.org/mailman/listinfo/netmod

Mahesh Jethanandani
mjethanandani@gmail.com