Subject: [netmod] comments on draft-ietf-netmod-acl-model-02.txt

From: Martin Bjorklund <[email-masked]>
Date: Sat, 07 Mar 2015 12:52:54 +0100 (CET)

Hi,

I have reviewed draft-ietf-netmod-acl-model-02.txt, and here are my
comments:

o The extensibility model is a bit unclear.

  - Each ACL has a type, an indentityref, which is good, but then this
    type is not really used in the model.  It is just a leaf with a
    value, with no indications of how it is supposed to be implemented
    or used by the operator.

    I would assume that the type restricts what can be configured in
    each ACE.  But that it is not the case, which means that you can
    set the ACL type to be "eth-access-control-list", but still
    configure an ACE with "source-ipv6-network" (for example).

    Also, the type is optional to configure, i.e., you can configure a
    type-less ACL.

    This makes it unclear what the type is supposed to do.

  - Section 3.1 says:

      The "ietf-packet-fields" modules can easily be extended to reuse
      definitions from other modules such as IPFIX [RFC5101] or
      migrate proprietary augmented module definitions into the
      standard module.

    I do not understand this.  What does "easily be extended" refer
    to?  The only way to extend this model - as it is written - is by
    publishing a new revision of the model (in a new RFC).

    Also, I do not understand the last part of the sentence:

      migrate proprietary augmented module definitions into the
      standard module.

  - Section 4.2 says:

    The packet fields module defines the necessary groups for matching on
    fields in the packet including ethernet, ipv4, ipv6, transport layer
    fields and metadata.  These groupings can be augmented to include
    other proprietary matching criteria. 

    This is not correct.  A grouping cannot be augmented.

    This makes me wonder about the design where the generic module
    imports and uses definitions from the special module.  Wouldn't it
    make more sense to do it the other way around?  I.e., let the
    generic module be generic, and augment it from the specific
    module?

  - There are some "choices" in the model that I *think* are supposed
    to be augmented by type-specific parameters.  If this is correct,
    it should be described in the data model (in the description of
    the choice).

o In the container "access-control-list-oper-data" there is something
  called "targets".  This concept needs to be explained.  I *think*
  the idea is that if you want to apply an ACL to interface, you would
  augment /if:interfaces/if:interface with some parameter that points
  out the ACL (as is done in A.3), and then the implementation would
  keep track of this in the "targets" choice.  If this is correct, it
  needs to be explained.

  Further, the model as it is defined is not complete.  Why is the
  special target "interface-name" defined in the standard model, but
  the other part (the augment of if:interface) is not defined in the
  standard?

  Note that the "target" name is not visible in the encoding, so all
  you see if do a GET on an ACL is:

    access-list foo
      access-control-list-oper-data
        interface-name eth0

  It would be better with an explicit name that tells the operator
  that the interface is really the "target":

    access-list foo
      access-control-list-oper-data
        target
          interface-name eth0

  Or maybe "used-by"?

o Why did you change the name from "acl" to "access-control-list" in
  many places between -01 and -02?  I didn't see any comments about
  this on the ML.  The current naming is not consistent, and the names
  are pretty long.  For example, you have "access-control-list",
  "access-list", and "acl".

  I suggest the following names:

  module ietf-access-control-lists

  top-level container access-control-lists  (or just acls)

  then remove redundant "access-control-list-" prefix from the names
  to get something like this:

  module: ietf-access-control-lists
   +--rw access-control-lists
      +--rw acl* [name]
         +--rw name                   string
         +--rw type?                  access-control-list-type
         +--ro oper-data
         |  +--ro (targets)?
         |     +--:(interface-name)
         |        +--ro interface-name*   string
         +--rw access-list-entries
            +--rw ace* [name]
               +--rw name                             string
               +--rw matches

  In general, I think you should review all node names and make sure
  that they are good.  It is often very useful to try out a model in
  one of the generic implementations that are available, to get a
  feeling for the structure and names of the data model.

o Section 3.1

  The tree output is not idented correctly.  (maybe a tooling issue,
  since the lines are so long - if you change the names this issue
  will be solved automatically).

o Section 4.1

   "ietf-acl" is the standard top level module for Access lists.  It has
   a container for "access-list" to store access list information. 
                    ^^^^^^^^^^^

  There is no such container.

   This
   container has information identifying the access list by a name("acl-
   name") and a list("access-list-entries") of rules associated with the
   "acl-name".

  There is no leaf called "acl-name".

o Change the prefix in the module to "acl" - "access-control-list" is
  too long. 

o identity IP-access-control-list

  For constistency, change this name to "ip-access-control-list".

  The description says:

    description "IP-access control list is common name for layer 3 and 4 access
    control list types. It is common among vendors to call 3-tupple or 5 tupple
    IP access control lists";

  I do not understand the last sentence.

o Readability

  The YANG modules are inconsistently indented and formatted, which
  makes them difficult to read.  I suggest you run:

    pyang -f yang --yang-canonical ietf-acl.yang

  and use that output (possibly with some additional hand-editing,
  e.g., put back comments that this tool unfortunately doesn't handle
  properly).

  Do this for all modules in this draft.

o leaf upper-port

  The model has:

      leaf lower-port {
        type inet:port-number;
        mandatory true;
        description "Lower boundary.";
      }
      leaf upper-port {
        type inet:port-number;
        description "Upper boundary. If exist, upper port must be greater or
        equal to lower port.";
      }

  I suggest you add a must statement to cover this requirement:

      leaf upper-port {
        ...
        must ". >= ../lower-port" {
          error-message
            "The upper-port must be greater than or equal to lower-port";
        }
        ...
      }

o descriptions

  Many statements need better description.  For example:

    leaf destination-mac-address-mask {
      type yang:mac-address;
      description "Mac addresses mask.";
    }

  This description doesn't explain anything.  In fact, it tells me
  *less* than the name of the node.

o grouping access-control-list-transport-header-fields

  The way your model is designed, it is not possible to create any
  type of ACE without also specifiying source-port-range/lower-port
  and destination-port-range/lower-port.  I don't think this is
  intentional?  If not, make "source-port-range" and
  "destination-port-range" to presence containers.

o grouping timerange

  The name "absolute" is very generic.  I suggest you change the name
  "absolute" to "absolute-time" or something.

  Is absolute time ranges supported across implementations?  I would
  expect this to be optional to implement (i.e., add if-feature).

  Are such absolute time ranges commonly used by operators?  What
  about periodic times?

  Also, this grouping doesn't really belong to a module called
  "ietf-packet-headers".

o leaf active

      leaf active {
        type boolean;
        default "true";
        description

          "Specify the associated function

          active or inactive state when
          starts going into effect";
  
  I do not undertand the description.

o Section 5. Linux nftables

  The purpose of this section is unclear.  It would make sense w/ an
  example that shows how to map this model to nftables, but that is
  not what it is.

o Section 4.3

  These examples are very useful.  Unfortunately, the XML in the
  example has numerous errors.  I suggest you carefully fix it so that
  it matches the CLI snippet and the data model.

o Section 4.4

  This is an example, but it states:

    When only a lower-port presents, it
    represents a single port.
  
  This is an important piece of information that should go into the
  decription of lower-port/upper-port in the data model!

o module std-ext-route-filter doesn't pass YANG validation.

o module newco-acl doesn't pass YANG validation.

o We should have some consistency in how we name our example modules.

  RFC 7223 uses:

    module ex-ethernet-bonding {
      namespace "http://example.com/ethernet-bonding";

  draft-ietf-netmod-routing-cfg-17 uses:

    module example-rip {
     namespace "http://example.com/rip";

  I suggest you use one of these as well (we should put this into the
  guidelines document!).

  Specifically, an example called "std-..." might give people the
  wrong idea...

  Also, remove the "contact" and "organization" statements from the
  example modules.

o  In the example module, remove

    reference " ";

  It looks very weird and doesn't give you anything.

/martin

---

From: Dean Bogdanovic <[email-masked]>
Date: Mon, 16 Mar 2015 12:56:03 +0000

Martin,

Thank you for the review of the draft.
On Mar 7, 2015, at 6:52 AM, Martin Bjorklund <[email-masked]<mailto:[email-masked]>> wrote:

Hi,

   I would assume that the type restricts what can be configured in
   each ACE.  But that it is not the case, which means that you can
   set the ACL type to be "eth-access-control-list", but still
   configure an ACE with "source-ipv6-network" (for example).
instead of this
leaf access-control-list-type {
type access-control-list-type;
description "Type of access control list. When this type is not explicitely specified, if vendor implementation permits, the access control
entires in the list can be mixed, by containing L2, L3 and L4 entries";
}

does this description answer your concern
description "It is recommended to have a access control list with uniform access list entries, all of the same type. When this type is not explicitely specified, if vendor implementation permits, the access control entries in the list can be mixed, by containing L2, L3 and L4 entries";

With typeless ACL we are future proofing the model, as you can put anything in the model.

 - Section 3.1 says:

We have changed the wording and replaced extended with augmented, so it reads now

"...can be easily augmented."

We have deleted the above part for clarity.

 - Section 4.2 says:

Thank you for catching our mistake. We meant choices within container matches can be augmented with new matching criteria.

 - There are some "choices" in the model that I *think* are supposed
   to be augmented by type-specific parameters.  If this is correct,
   it should be described in the data model (in the description of
   the choice).
Please see above, as we have corrected the wording.

Yes, your comment explains our intention and we have clarified this in the new version.

We wanted to add operational data to the standard model and have an option to add choice of targets.  We will leave the empty container in the standard model and move the target as example in the appendix

   access-list foo

   access-list foo

 Or maybe "used-by"?

In general, we prefer to use fully extended names, instead of abbreviations as it removes ambiguity and it is easier to read. Your suggestion is a pretty good compromise, but would like to hear more from other people on this topic as well.

o Section 3.1

This is corrected.

o Section 4.1

This is corrected.

                   ^^^^^^^^^^^

  This

done

 The description says:

We rewrote this paragraph.

o Readability

We have updated the yang models according to your suggestion

o leaf upper-port

 The model has:

     leaf lower-port {
       type inet:port-number;
       mandatory true;

     leaf upper-port {
       ...

thank you for suggestion and we did changed it accordingly
o descriptions

Descriptions have been improved.

the source-port-range and destination-port-range are containers within grouping access-control-list-transport-header-fields in ietf-packet-fields.yang . Are you saying if containers are used within groupings, they are forced?

o grouping timerange

agree with you and changed it.

Juniper, Cisco, Brocade support absolute and periodic/relative time ranges and from our experience it is pretty common across implementations.

 Are such absolute time ranges commonly used by operators?  What
 about periodic times?
from my experience, for packet filtering it is mostly used with absolute times. When filters are used for packet counting, then periodic/relative times are preferred.

o leaf active

     leaf active {
       type boolean;
       default "true";
       description

description has been improved

We took this suggestion and changed it in the next version

o Section 4.3

done

o Section 4.4

we changed it

fixed

fixed

 RFC 7223 uses:

   module example-rip {

And we should fix the pyang compiler as well, as it puts out warning when writing YANG as you mentioned above

pyang --ietf example-newco-acl.yang
example-newco-acl.yang:4: warning: IETF rule (RFC 6087: 4.8): namespace value should be "urn:ietf:params:xml:ns:yang:example-newco-acl"

 wrong idea…

I wanted to show how to do a standard extension, but the naming is ambiguous.

I would, but then have problems with compiling the yang module
pyang --ietf example-newco-acl.yang
example-newco-acl.yang:1: error: IETF rule (RFC 6087: 4.7): statement "module" must have a "contact" substatement
example-newco-acl.yang:1: error: IETF rule (RFC 6087: 4.7): statement "module" must have a "organization" substatement
example-newco-acl.yang:4: warning: IETF rule (RFC 6087: 4.8): namespace value should be "urn:ietf:params:xml:ns:yang:example-newco-acl"
example-newco-acl.yang:21: error: IETF rule (RFC 6087: 4.7): statement "revision" must have a "reference" substatement

   reference " ";

Please see above

/martin

_______________________________________________
netmod mailing list
[email-masked]<mailto:[email-masked]>
https://www.ietf.org/mailman/listinfo/netmod

---

From: Martin Bjorklund <[email-masked]>
Date: Wed, 18 Mar 2015 10:24:51 +0100 (CET)

Hi,

Comments inline.

> description "It is recommended to have a access control list with uniform access list entries, all of the same type. When this type is not explicitely specified, if vendor implementation permits, t…

No not really.  It is clear from the design that each ACE has its own
type:
            choice access-list-entries-type {
              description "Type of access list entry.";

But this type is not in any way tied to the ACL type.  I think you are
saying that depending on implementation, there MAY be some unknown
connection between the ACL type and the ACE type.  Some
implementations may be ok with not setting the ACL type, some may be
ok with having an ACL type but then allows mixed ACEs anyway, and some
may require the ACL type to match the ACE type.  How will a client
know?  How can this be interoperable?

> "...can be easily augmented."

But this is not correct.   This module contains a bunch of groupings,
and groupings cannot be augmented.

[...]

>    generic module be generic, and augment it from the specific

Did you consider this alternative design?

[...]

> In general, we prefer to use fully extended names, instead of abbreviations as it removes ambiguity and it is easier to read. Your suggestion is a pretty good compromise, but would like to hear mor…

Ok.

[...]

>  and destination-port-range/lower-port.

Actually, I was wrong here.  If the ACE type is access-list-entries-ip
then these leafs MUST be set.

>  I don't think this is

No fact that they are defined in groupings doesn't matter.  The
problem is that source-port-range is a non-presence container, and
lower-port is marked as mandatory.  This means that lower-port MUST be
configured (... if any node in the same access-list-entries-ip case is
set).

> from my experience, for packet filtering it is mostly used with
> absolute times.

So you create it once for a certain absolute time, and once that has
passed, the filter is removed?

>  Also, this grouping doesn't really belong to a module called

Did you consider moving it?

[...]

> example-newco-acl.yang:4: warning: IETF rule (RFC 6087: 4.8): namespace value should be "urn:ietf:params:xml:ns:yang:example-newco-acl"

Well, the --ietf flag to pyang is not intended to be used for
examples.  All rules in 6087 are indented for standard modules, not
for examples.  I think we need to clarify this in 6087bis.

> pyang --ietf example-newco-acl.yang

See above.

>  It looks very weird and doesn't give you anything.

Since --ietf doesn't apply to example modules, it is safe to remove
this unnecessary statement.

/martin

---

From: Dean Bogdanovic <[email-masked]>
Date: Wed, 18 Mar 2015 14:52:19 +0000

Martin,

Will you be present at the YANG Doctor session this Sunday? If so, lets have a discussion on your comments there

Dean

---

From: Martin Bjorklund <[email-masked]>
Date: Wed, 18 Mar 2015 18:27:44 +0100 (CET)

Dean Bogdanovic <[email-masked]> wrote:
> Martin,
> 
> Will you be present at the YANG Doctor session this Sunday? If so,
> lets have a discussion on your comments there

Yes I will be there, let's talk then.

/martin

> >> On Mar 7, 2015, at 6:52 AM, Martin Bjorklund
> >> <[email-masked]<mailto:[email-masked]>> wrote:
> >> 
> >> Hi,
> >> 
> >> description "Type of access control list. When this type is not
> >> explicitely specified, if vendor implementation permits, the access
> >> control
> >> description "It is recommended to have a access control list with
> >> uniform access list entries, all of the same type. When this type is
> >> not explicitely specified, if vendor implementation permits, the
> >> access control entries in the list can be mixed, by containing L2, L3
> >> and L4 entries";
> > 
> > 
> >> In general, we prefer to use fully extended names, instead of
> >> abbreviations as it removes ambiguity and it is easier to read. Your
> >> suggestion is a pretty good compromise, but would like to hear more
> >> from other people on this topic as well.
> > 
> > Ok.
> > 
> > [...]
> > 
> >> the source-port-range and destination-port-range are containers within
> >> grouping access-control-list-transport-header-fields in
> >> ietf-packet-fields.yang . Are you saying if containers are used within
> >> groupings, they are forced?
> > 
> >> Juniper, Cisco, Brocade support absolute and periodic/relative time
> >> ranges and from our experience it is pretty common across
> >> implementations.
> >> 
> >> example-newco-acl.yang:4: warning: IETF rule (RFC 6087: 4.8):
> >> namespace value should be
> >> "urn:ietf:params:xml:ns:yang:example-newco-acl"
> >