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

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 29 Jul 2014 14:49:26 +0200

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: +49 421 200 3587         Campus Ring 1, 28759 Bremen, Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>

---

From: "Lisa Huang (yihuan)" <[email-masked]>
Date: Wed, 30 Jul 2014 19:16:25 +0000

Juergen,

Thanks for the comments. We just learned to run pyang --ietf during
Saturday session and update the model in git repository. Will definite
update the draft and correct the syntaxes in the future.

You raised a lot of good points in the email. I responded some of them in
line. More can be discussed on Thursday meeting.

Thanks,

Lisa

>  intended status should not be "Informational".
Will change to "Standards Track"
>  perhaps something like "A YANG Data Model for Network Access Control
Will make the title more specific
>  structure designed to be augmented).
Let's discuss this on Thursday meeting.
>- The first paragraph on the introduction has singular/plural
Will correct the grammar error.
>  into normative and informative references and update any outdated
Will correct this.
>  s/that augmented/that can be augmented/
Will correct these.
>  or more precisely data derived from packet headers?
Will correct this.

>  s/is example/is an example/
Will correct these.

>  published data models to see how people usually format YANG data
Thanks for pointing out. We had an update modle in git repository on the
model based on pyang --ietf check but have not update the draft yet. Will
update it in the future draft.
>  I need acl-name? Perhaps this was supposed to be a list keyed by
Let's discuss in the review meeting
>  hence this might be misplaced?
The model need to be updated to make target as config false. We had a lot
of discussion about what should be the right type. We compared string vs
interface-ref vs instance-identifier. Since ACL can apply to interfaces,
or used as classifier in qos and service chaining. There is no common type
between its target. We can discuss more in the meeting.

>  make sure things compile using pyang --ietf.
Will update the draft with the updated module.
>  to the concepts in the propose data model.
Agree. We actually planned to present these questions in the IETF
conference to find out what are the cases that audience would like to see.
If these are general interests, we would add them.

---

From: "Dana Blair (dblair)" <[email-masked]>
Date: Thu, 31 Jul 2014 13:51:48 +0000

Thanks for your thorough review.

>  intended status should not be "Informational".

We'll fix this.

>  perhaps something like "A YANG Data Model for Network Access Control

OK.

>  into normative and informative references and update any outdated

Thanks.  We'll update the voe.

>  instead of simply "Access Control Rule"? What is a _group of_ match

I'm open to the term "rule", but historically the rules in a access control
list are called entries.  This goes way back.  We'll discuss and
see if other comments on this come in.

>  and action criteria? Why is a prefix length meta data? I mean, how

Each "rules" in the access-list, and in fact other kinds of policy on
router, 
contain a set of match criteria for matching packets, and a set of actions
which are executed when a rule is matched.

>  or more precisely data derived from packet headers?

According to our definition of packet metadata, packet metadata is
information
relevant to the packet matching but is not in the actual packet header.
There is no mask or prefix length in the packet header.  So in this strict
sense,
prefix length is metadata.  However, it might be good to relax the metadata
definition for this case.

>  Make sure the YANG module passes pyang --ietf checks. Also note that

Good point.  We learned that on the YANG workshop on Sunday.  I sent out
a new version of the model to the authors which resolves the --ietf checks.
Next version of the draft will have this fixed.

>  published data models to see how people usually format YANG data

Thx. Good advice.  I guess since these names directly translate into
XML text and/or API calls, keeping them concise is good.

>  I need acl-name? Perhaps this was supposed to be a list keyed by

Looks like the YANG list is not only defining a list of things, but also
a shared name space between them.  Is that right ?

There are lists of access-lists but there are many types.  IPv4, Ipv6,
L2, IPv4 and IPv6 rules in the same access-list.  We need to explore if
each needs its own list (namespace), or they have a common name space.
This will dictate how the model defines the lists.

>  data is good or bad design. While I believe acl-oper-data is meant

It'll be interesting to see how that turns out.  I've seen both ways of
expressing.  I don't have a strong opinion either way.  I do believe
including
config and operational state in the model allows easier
understanding/correlation
between the two.

>  to be operational state, it is not marked as config false either.

Will be fixed in the next version.

>  hence this might be misplaced?

Access-lists are applied to things.  We call these targets.  Most
familiar is access-list are applied to interfaces with a direction (input
or output
or both).  But access-lists can also be applied to routing protocols
policies,
routing directly (policy routing which is a kind of static route for things
beyond just destination prefix matching), Š   There are other proprietary
targets within each vendor.

We're still trying to think about whether to include targets in the model
or
where/how to specify them.  This is an open item.

The operational state should include a list of targets than an access-list
has been applied to, in addition to some way to actually configure an
access-list on a target.

>  metadata and a single action it can trigger, no?

The intention is that each "rule" would all multiple match criteria
(dst, prefix, dscp, port number, Š) and multiple actions.
Regarding the actions, the 2 standard actions are permit or deny but
there are others like log and many vendor proprietary actions.

If you look at the newco example, it should show how to augment the model
with an action.  As the community here presents augmentations we can
consider including them in the standard model.

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

Will be fixed.

>  a shorter prefix than 'packet-fields'?

Agreed, same as above.

>  Some fields need more description. Is it useful to represent the

So this a long historical problem.  Some protocols have well known names
and name should be used, others are not well known so numbers are used.

Ideally, there would be separate yang module that contains the
protocol to name relationship standardized by IANA.
http://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml#pro
tocol-numbers-1
This would be very useful and prevent confusion.  As numbers are assigned,
the model/draft is
updated to reflect the standard assignments.

Our more and other models could reference it.

>  (e.g. in a choice)? Right now, the user of the definitions has to

Possible, but we need to be careful that the model does not allow
a user to configure an IPv4 source and IPv6 destination in the same rule.

>  for source-ipv6-address is ...-address - I guess this is a typo.]

Will work on this.  The --ietf option forces descriptions here.

>  be clear everybody does the same thing. What about VLANs? Out of

We can include VLAN header in eth-header-fields.

>  it. That is fine but supposed I need to, can I extend this easily?

This is definitiely an interesting area to explore, and would reveal
how flexible/extensible the model is.  I've seen very explict match
criteria which assumes only 2 IP headers (single tunnel encapsulation).
However we address this in the model, it should allow an arbitrary number
of
header L2 and L3 encapsulations.  Something to think about.

>  what happens if I add say flow-label to the IP header fields?

I personally (not necessarily all authors), would prefer that there
be a seperate model/draft which defines the standard IP and Ethernet
Header fields.
Then this could be used by many models.

If there was a standard model for the IP header, and the acl rule
references, and
a vendor chose not to implement matching on some field, how could an
augmentation exclude this.
I'm not sure feature is useful here, because the number of features might
proliferate
rapidly.

>  transport header matching to make updates and maintenance easier?

Seperate and managed according to RFC definitions and IANA would be better
IMO.

>  to the concepts in the propose data model.

Authors will review these, and comment or make updates in the next version.

Again, thanks for your thorough review.

thanks,
Dana