[netmod] review of draft-bogdanovic-netmod-acl-model-01

[Juergen Schoenwaelder <[e-mail]>]

Hi,

during the Sunday session before IETF 90, I got nominated to be the
Yang Doctor for draft-bogdanovic-netmod-acl-model-01. And as a
consequence, I have done a review of this I-D and since this was
submitted as a contribution to the IETF, I thought I post the review
right here. Note that many comments are not really Yang Doctors
concerns but once I review a document, I often end up reviewing the
whole thing.

/js

- If this document is supposed to be standards-track, then the
  intended status should not be "Informational".

- The title should be more descriptive. Following recent YANG RFCs,
  perhaps something like "A YANG Data Model for Network Access Control
  List (ACL) Management".

- I am not sure you define an information model and a data model. I
  think you define just a data model and hence you may want to update
  the abstract. And I am not sure what 'basic building blocks' means,
  perhaps expand on the approach taken (i.e. that it is a basic
  structure designed to be augmented).

- The first paragraph on the introduction has singular/plural
  mismatches.

- Run idnits and fix lines that are too long and split the references
  into normative and informative references and update any outdated
  references.

- Section 2:

  s/that model can be/that the model can be/
  s/Actions/actions/
  s/that augmented/that can be augmented/

- Section 3: You say that a access control list containers an ordered
  set of rules. Why do you choose the term "Access Control Entry"
  instead of simply "Access Control Rule"? What is a _group of_ match
  and action criteria? Why is a prefix length meta data? I mean, how
  do you determine the source prefix length? According to a lookup in
  the local forwarding table(s)? Is this strictly speaking meta data
  or more precisely data derived from packet headers?

  s/daufault/default/

- Section 3.1: You can't write 'imports module "packet-headers" into
  the match container'. While you claim it is easy to reuse
  definitions of say IPFIX [RFC5101], I am not sure I see the details
  how this would work. Better work out the details and then leave the
  judgment whether it is easy or not to the reader.

  s/is example/is an example/

  You may want to consider to describe the standard models in section
  3 and to discuss proprietary extensions in a separate section or
  even an appendix to make it very clear which text is normative and
  which is not.

- Section 4.1.

  s/Access/access/

  Make sure the YANG module passes pyang --ietf checks. Also note that
  the description of the revision statement should refer to the
  document defining the data model. Note that the prefix does not have
  to carry 'ietf-'. (As you will see, shorter identifiers make path
  expressions shorter and much more readable.) Look at some other
  published data models to see how people usually format YANG data
  models.

  It wonder why access-list is a container, that is this is a
  singleton. In Linux, I can have multiple tables and multiple chains
  that contain multiple rules. And if this is a singleton, why do
  I need acl-name? Perhaps this was supposed to be a list keyed by
  acl-name?

  There is ongoing debate whether operational state mixed with config
  data is good or bad design. While I believe acl-oper-data is meant
  to be operational state, it is not marked as config false either.
  And note that the common terminology is operational state and not
  operational data.

  I do not know what a target in the leaf-list of targets are. The
  type just says string, which is a bit too open ended I guess. And
  it seems this is really configuration and not operational state,
  hence this might be misplaced?

  It seems the matching is rather limited per rule. This may be fine
  but seems at odd with a previous statement "Each ACE has a group of
  match criteria and a group of action criteria." It seems a rule
  (oops ACE) has a single simple match against header files and
  metadata and a single action it can trigger, no?

  The counters in ace-oper-data should at least be config false.

- Section 4.2:

  The module needs to get a proper name that starts with ietf- and
  make sure things compile using pyang --ietf.

  Since the packet fields definitions may be reused, do not prefix
  them with acl- - in fact those prefixes are not really needed in
  YANG since you prefix identifiers when use them. Perhaps think about
  a shorter prefix than 'packet-fields'?

  Some fields need more description. Is it useful to represent the
  protocol by number?

  Should the ip-header-fields not include the ipv4/ipv6 header fields
  (e.g. in a choice)? Right now, the user of the definitions has to
  create and maintain the choice. I also note that the fields are
  called ...-address but the type is really a prefix. So the idea is
  that you match an address taken from the packet header against a
  prefix. I guess this (though perhaps obvious to us) should be
  explained in the missing description statements. [Note that the type
  for source-ipv6-address is ...-address - I guess this is a typo.]

  You should explain how address masking is supposed to work just to
  be clear everybody does the same thing. What about VLANs? Out of
  scope?

  What is the purpose of the absolute container of the timerange
  definition? There is no other timerange defined. The description
  of 'active' is somewhat confusing.

  While the Internet ages, we see interesting layerings coming
  along. (I do not want to mention WebRTC.) How does your framework
  apply to tunnels etc? I assume all this applies only to the outer
  headers and I can't say filter IP in IP encapsulated traffic with
  it. That is fine but supposed I need to, can I extend this easily?

  A bigger question I have is how the 'packet-header' module (needs a
  better name) is being maintained. Is this cast into stone via an RFC
  and then RFC update rules apply? Or is this to be maintained by IANA
  with a careful review process? And if changes are made, how does a
  client figure out which set of filters an implementation supports?
  Do we need to define suitable features? Are features sufficient -
  what happens if I add say flow-label to the IP header fields?

  Should things perhaps be split into several modules, one for
  Ethernet header matching, one for IP header matching, one for
  transport header matching to make updates and maintenance easier?

- Section 4.3: I do not understand the 'range' enum of the
  match-simple-payload-protocol-value grouping.

- Several citations are duplicated in the text, such as [RFC6241]
  [RFC6241].

- Section 4.4 should use IP addresses reserved for examples and not
  just 1.1.1.1 and 2.2.2.2. I am also not sure that showing some
  random proprietary CLI is a good way of describing an example in an
  I-D - simple remove Figure 1. In Figure 2, I would remove all the
  edit-config noise and focus only on the config data. That said, I am
  not sure what <top> is nor am I convinced you got the namespaces
  right. Make sure you validate your examples against the data model.
  (See the pyang tutorial on how to do that.)

- Section 5 probably needs to be expanded. Linux has tables, chains,
  and rules and it would be nice to document how things actually map
  to the concepts in the propose data model.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: [phone]         Campus Ring 1, 28759 Bremen, Germany
Fax:   [phone]         <http://www.jacobs-university.de/>