Document reviewed: draft-ietf-netmod-acl-model-07

Status: Ready with work

I have marked it ready with work because it appears that there were points raised on the mailing list that are not reflected in this draft. At the minimum the authors will need to post a new draft. While they are doing that, it would be helpful if they can address some of the points raised in this review.

Summary:

This document describes a data model of ACL basic building blocks. As such the model cannot be used as is and is not a standalone model. It needs to be augmented by vendors for their particular implementation of ACLs. Examples of how the model needs to be extended are described in the Appendix section of the document.

The above point needs to come out clearly in the document. Maybe the paragraph from Appendix A.3 can be moved up into the introduction to clarify the scope of the document. Besides a statement of the scope of the document hardly belongs in the Appendix section.

The document has been extensively reviewed on the mailing list and comments provided. Two issues that were particularly debated were around:

how ACEs are ordered inside the ACL 
use of metadata to filter traffic.

There was much debate on the use of ‘ace-type’ (which is a string) as a key in the list of ACE. Implementations that use numbers to identify the order of entires need to map the ‘ordered-by-user’ list to whatever the internal implementation allows. This is just an observation and authors are not expected to take any action on it.

The consensus on the mailing list seems to be for the removal of definition of metadata from the base model and moving it into an extension module. If that turns out to be the case, the authors need to update the Introduction and other sections to update references to metadata in the document.


Section 1: Introduction

s/basic elements to configure/basic elements used to configure/
s/networking concepts/networking technologies/
s/depending on the innovations/depending on the capabilities/
s/are interchangeable/are used interchangeably/

Section 2: Problem Statement

The second statement of the first paragraph is not clear. I think the authors meant to say that the model can be used by vendors and applications. Why are the models “reused” and what is “between applications”?

Also, what is a “simple model”? Further down, there is reference to “common model”. I think the idea is that this model represents what is common across several implementations of ACL. Therefore the use of “common model” rather than “simple model” makes more sense.

Section 3: Design of the ACL model.

This section is titled “Design of the ACL Model”, but is talking about how ACLs work on a system. The actual design of the model is in section 3.1 and should have that title. A more appropriate title for Section 3 would be “How ACLs Work” or something similar.

But a more important question is whether ietf-packet-fields a module or a submodule of ietf-access-control-list. The authors talk about ietf-access-control-list as a “superclass”, and it using groupings out of ietf-packet-fields. If that is the case, and there is no plan to use ietf-packet-fields outside of this model, it would probably make more sense to make it a submodule.

Section 4.1 title says “IETF Access Contorl (sp?) List module” while Section 4.2 says IETF-PACKET-FIELDS module. Can we be consistent in the titles of the sections?

Section 4.1 IETF Access Contorl List module

In general spaces between different fields and parts of the model would go a long way towards making the model more readable. See RFC 7223 for an example.

Do we need to specify yang-version in the YANG module?

The contact references in the model are dated. Please update.

Please add a note in the draft letting the RFC editor know that the Revision date in the model needs to be updated to the date when the draft gets approved. That date also needs to be reflected on the line with <CODE BEGINS>.

Ditto for “RFC XXXX” in the reference section.

Please add headers for each section of the module as follows to make the draft more readable.

/*
 * Identities
 */

/*
 * Typedefs
 */

There is extensive use of acl instead of access-control-list. Why not use it in typedef of access-control-list-ref and call it acl-ref? Same for the use of aces instead of access-list-entries in other parts of the module, like

container aces {

    list ace {

Would it make sense to have a separate container for operational data? Keeping it with configuration data might make some sense, but keeping it separate allows one to get all operational data by doing a <get> on the top level container.

There also seems to dearth of operational data. For example, I do not see any operational data for actions in the model. How about a count for packets that are denied or permitted?

Section 4.3 An ACL Example.

The example is very confusing. It seems to represent many different examples all randomly put together. The CLI example does not match the XML version. It would help to have a complete example all the way from CLI to XML that is the same. And yes, more examples would be even better.

Section 4.4 Port Range Usage Example

The explanation for the second snippet is incomplete. Yes, the port is greater/equal to 16384, but is it not also less than/equal to 65535?

Section 5 Linux nftables

The first paragraph needs to be rewritten for spelling mistakes and readability. 

More importantly, it is not clear what the intent of this section is. Yes, Linux is popular and is used for network platforms, but this model is not about Linux or how it is going to be implemented there. It would be best to remove it completely.

Thanks.

Mahesh Jethanandani
mjethanandani@gmail.com