Re: [netmod] WG Last Call: draft-ietf-netmod-acl-model-14

Martin Bjorklund <mbj@tail-f.com> Thu, 02 November 2017 12:28 UTC

Return-Path: <mbj@tail-f.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 CEA8113F585 for <netmod@ietfa.amsl.com>; Thu, 2 Nov 2017 05:28:03 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.901
X-Spam-Level:
X-Spam-Status: No, score=-1.901 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
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 4pt5DlRiuMsk for <netmod@ietfa.amsl.com>; Thu, 2 Nov 2017 05:28:00 -0700 (PDT)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id 6E0A613B133 for <netmod@ietf.org>; Thu, 2 Nov 2017 05:28:00 -0700 (PDT)
Received: from localhost (unknown [173.38.220.41]) by mail.tail-f.com (Postfix) with ESMTPSA id 0C75D1AE01AA; Thu, 2 Nov 2017 13:27:58 +0100 (CET)
Date: Thu, 02 Nov 2017 13:26:34 +0100
Message-Id: <20171102.132634.1363976895007772742.mbj@tail-f.com>
To: rwilton@cisco.com
Cc: mjethanandani@gmail.com, kristian@spritelink.net, netmod@ietf.org
From: Martin Bjorklund <mbj@tail-f.com>
In-Reply-To: <ac9fc676-80f7-723d-9a85-c99fbb122476@cisco.com>
References: <20171102074318.GC12688@spritelink.se> <6359CD50-0F0D-4315-A58B-1D4CF0583475@gmail.com> <ac9fc676-80f7-723d-9a85-c99fbb122476@cisco.com>
X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Text/Plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://mailarchive.ietf.org/arch/msg/netmod/CHdlD1GeuGwKUyIDknSKlKoeQRs>
Subject: Re: [netmod] WG Last Call: draft-ietf-netmod-acl-model-14
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: Thu, 02 Nov 2017 12:28:04 -0000

Robert Wilton <rwilton@cisco.com> wrote:
> Hi Mahesh,
> 
> I also think that the model would be cleaner if you don't have
> separate containers for each "type of ACL".  In particular, I think
> that the model is easier for clients, and perhaps easier to implement,
> if a given ACE field is always on the same path rather that being on a
> different path based on ACL type.

+1

> I've suggested a minor change to
> the model that may retain the same benefits, but keep consistent paths
> for the ACE fields.
> 
> Hence, would the following help improve the model?
> 
> 1) Move the L2, IPv4, IPv6 match fields under their own containers, as
> Juergen/Kristian have suggested.
> 2) Under each of those containers remove the "if-feature", and change
> the "must" statement to a "when statement".
> 3) Add if-feature statements under the acl-type identity definitions,
> allowing a device to control what acl-types mixes they support.
> 
> E.g.
> 
>   // ACL features defined as below.
> 
>   // ACL type identities updated to:
>   identity ipv4-acl {
>     base acl:acl-base;
>     if-feature "ipv4-acl";
>   }
>   identity ipv6-acl {
>     base acl:acl-base;
>     if-feature "ipv6-acl";
>  }
>   identity eth-acl {
>     base acl:acl-base;
>     if-feature "l2-acl";
>  }
>   identity mixed-l2-l3-ipv4-acl {
>     base "acl:ipv4-acl";
>     base "acl:eth-acl";
>     if-feature "mixed-ipv4-acl";
>  }

There seem to be some inconsistencies in these names.  For example,
why is the identity called "eth-acl" but the feature "l2-acl"?  And
when the identity for eth is used with "mixed-" it is called "l2".

Also, why is "mixed-l2-l3-ipv4-acl" not called "mixed-l2-ipv4-acl"?
And why is the corresponding feature just called "mixed-ipv4-acl"?



/martin





>   identity mixed-l2-l3-ipv6-acl {
>     base "acl:ipv6-acl";
>     base "acl:eth-acl";
>     if-feature "mixed-ipv6-acl";
>  }
> 
> ...
> 
> ...
>       leaf acl-type {
>         type acl-type;
>       }
>       container aces {
>         list ace {
>           key "rule-name";
>           ordered-by user;
>           leaf rule-name ...
> 
>           container matches {
>             container l2 {
>               when "derived-from(../../../../acl-type, 'acl:eth-acl')";
>               uses packet-fields:acl-eth-header-fields;
>               description
>                 "Rule set for L2 ACL.";
>             }
>             container ipv4 {
>               when "derived-from(../../../../acl-type, acl:ipv4-acl')";
>               uses packet-fields:acl-ip-header-fields;
>                     uses packet-fields:acl-ipv4-header-fields;
>               description
>                 "Rule set that supports IPv4 headers.";
>             }
>             container ipv6 {
>               ...
>             }
> 
> 
> Thanks,
> Rob
> 
> 
> On 02/11/2017 08:50, Mahesh Jethanandani wrote:
> > Kristian,
> >
> > I hear you. What I am providing is the rational for the current
> > design.
> >
> > I would like to hear from others in the WG. We have been reviewing
> > this draft for the last couple of years, and we are now at the tail
> > end of the LC. I would really like to see this draft move forward,
> > particularly since it is not broken.
> >
> > Thanks.
> >
> >> On Nov 2, 2017, at 2:13 PM, Kristian Larsson <kristian@spritelink.net>
> >> wrote:
> >>
> >>> On Thu, Nov 02, 2017 at 06:13:04AM +0630, Mahesh Jethanandani wrote:
> >>>     On Nov 1, 2017, at 5:52 PM, Juergen Schoenwaelder <
> >>>     j.schoenwaelder@jacobs-university.de> wrote:
> >>>
> >>>     Mahesh,
> >>>
> >>>     I think the question is why we need to have different match containers
> >>>     for each possible feature set combination instead of having a single
> >>>     match container with groups of leafs in it marked as features. This
> >>>     would seem to cut down the size of the module and the tree diagram
> >>>     significantly. I think this will also make clients simpler sicne they
> >>>     do not have to select a certain container based on the feature set
> >>>     announced.
> >>>
> >>>
> >>> The current design of match containers was chosen to allow platforms
> >>> to select
> >>> one container that matched what the hardware supported from a l2, l3
> >>> and ipv
> >>> {4,6} perspective.
> >> Sure, but you are conflating the structure of the model with the
> >> feature-wraps. Without changing the features of the model, we can
> >> structure it in a different way where there is not a 1:1 mapping
> >> between features and containers under the matches container.
> >>
> >>
> >>> I would argue that even though the overall diagram is bigger
> >>> with this design, once the platform selects the container of choice,
> >>> the tree
> >>> and the configuration itself would be a little simpler/smaller.
> >> I am arguing the opposite. It's really awkward to place the same
> >> type of data, like IPv4 match conditions, under different paths
> >> based on a feature.
> >>
> >> If the system model had done the same we would have:
> >> /system
> >> /system-with-ntp
> >> /system-with-ntp-and-radius
> >>
> >> How do you augment in new leaves? While you are using groupings
> >> which allows for a reuse of data across all these containers,
> >> someone who is augmenting can't, since you can't augment a
> >> container, only where it is used. Someone wishing to add a leaf
> >> to the model needs to augment in three different locations to
> >> support a new match condition for IPv4 (let's say some meta-data
> >> attribute).
> >>
> >>
> >>> Take the case where the desired selection is l2,-l3, ipv4 and
> >>> ipv6. The current
> >>> tree looks like this:
> >>>
> >>>
> >>>         |        |  +--rw l2-l3-ipv4-ipv6-acl {l2-l3-ipv4-ipv6-acl}?
> >>>         |        |  |  +--rw destination-mac-address?  yang:mac-address
> >>>         |        |  |  +--rw destination-mac-address-mask?
> >>>         |        |  |  yang:mac-address
> >>>         |        |  |  +--rw source-mac-address?  yang:mac-address
> >>>         |        |  |  +--rw source-mac-address-mask?  yang:mac-address
> >>>         |        |  |  +--rw ethertype?                      eth:ethertype
> >>>         |        |  |  +--rw dscp?                           inet:dscp
> >>>         |        |  |  +--rw ecn?                            uint8
> >>>         |        |  |  +--rw length?                         uint16
> >>>         |        |  |  +--rw ttl?                            uint8
> >>>         |        |  |  +--rw protocol?                       uint8
> >>>         |        |  |  +--rw source-port-range!
> >>>         |        |  |  |  +--rw lower-port    inet:port-number
> >>>         |        |  |  |  +--rw upper-port?   inet:port-number
> >>>         |        |  |  |  +--rw operation?    operator
> >>>         |        |  |  +--rw destination-port-range!
> >>>         |        |  |  |  +--rw lower-port    inet:port-number
> >>>         |        |  |  |  +--rw upper-port?   inet:port-number
> >>>         |        |  |  |  +--rw operations?   operator
> >>>         |        |  |  +--rw ihl?                            uint8
> >>>         |        |  |  +--rw flags?                          bits
> >>>         |        |  |  +--rw offset?                         uint16
> >>>         |        |  |  +--rw identification?                 uint16
> >>>         |        |  |  +--rw destination-ipv4-network?  inet:ipv4-prefix
> >>>         |        |  |  +--rw source-ipv4-network?  inet:ipv4-prefix
> >>>         |        |  |  +--rw next-header?                    uint8
> >>>         |        |  |  +--rw destination-ipv6-network?  inet:ipv6-prefix
> >>>         |        |  |  +--rw source-ipv6-network?  inet:ipv6-prefix
> >>>
> >>>         |        |  |  +--rw flow-label?
> >>>         |        |  |          inet:ipv6-flow-label
> >>>
> >>>
> >>>
> >>> whereas, if the design went with one match container with each group
> >>> of leafs
> >>> in their own container (to support the if-feature statement for that
> >>> container), the tree would look like this:
> >>>
> >>>
> >>>         |        |  +--rw l2-acl {l2-acl}?
> >>>         |        |  |  +--rw destination-mac-address?  yang:mac-address
> >>>         |        |  |  +--rw destination-mac-address-mask?
> >>>         |        |  |  yang:mac-address
> >>>         |        |  |  +--rw source-mac-address?  yang:mac-address
> >>>         |        |  |  +--rw source-mac-address-mask?  yang:mac-address
> >>>         |        |  |  +--rw ethertype?                      eth:ethertype
> >>>
> >>>         |        |  +--rw ipv4-acl {ipv4-acl}?
> >>>         |        |  |  +--rw dscp?                       inet:dscp
> >>>         |        |  |  +--rw ecn?                        uint8
> >>>         |        |  |  +--rw length?                     uint16
> >>>         |        |  |  +--rw ttl?                        uint8
> >>>         |        |  |  +--rw protocol?                   uint8
> >>>         |        |  |  +--rw source-port-range!
> >>>         |        |  |  |  +--rw lower-port    inet:port-number
> >>>         |        |  |  |  +--rw upper-port?   inet:port-number
> >>>         |        |  |  |  +--rw operation?    operator
> >>>         |        |  |  +--rw destination-port-range!
> >>>         |        |  |  |  +--rw lower-port    inet:port-number
> >>>         |        |  |  |  +--rw upper-port?   inet:port-number
> >>>         |        |  |  |  +--rw operations?   operator
> >>>         |        |  |  +--rw ihl?                        uint8
> >>>         |        |  |  +--rw flags?                      bits
> >>>         |        |  |  +--rw offset?                     uint16
> >>>         |        |  |  +--rw identification?             uint16
> >>>         |        |  |  +--rw destination-ipv4-network?   inet:ipv4-prefix
> >>>         |        |  |  +--rw source-ipv4-network?        inet:ipv4-prefix
> >>>         |        |  +--rw ipv6-acl {ipv6-acl}?
> >>>         |        |  |  +--rw dscp?                       inet:dscp
> >>>         |        |  |  +--rw ecn?                        uint8
> >>>         |        |  |  +--rw length?                     uint16
> >>>         |        |  |  +--rw ttl?                        uint8
> >>>         |        |  |  +--rw protocol?                   uint8
> >>>         |        |  |  +--rw source-port-range!
> >>>         |        |  |  |  +--rw lower-port    inet:port-number
> >>>         |        |  |  |  +--rw upper-port?   inet:port-number
> >>>         |        |  |  |  +--rw operation?    operator
> >>>         |        |  |  +--rw destination-port-range!
> >>>         |        |  |  |  +--rw lower-port    inet:port-number
> >>>         |        |  |  |  +--rw upper-port?   inet:port-number
> >>>         |        |  |  |  +--rw operations?   operator
> >>>         |        |  |  +--rw next-header?                uint8
> >>>         |        |  |  +--rw destination-ipv6-network?   inet:ipv6-prefix
> >>>         |        |  |  +--rw source-ipv6-network?        inet:ipv6-prefix
> >>>         |        |  |  +--rw flow-label?  inet:ipv6-flow-label
> >>>
> >>>
> >>> The difference though is small and comes down to a preference. Select
> >>> one
> >>> feature statement and get one container with everything in it, or
> >>> define
> >>> multiple feature statements and assemble together the pieces to define
> >>> the ACE
> >>> entry.
> >> Again, I think you mix up the features available vs the structure
> >> of the data.
> >>
> >> This is the current list of features (which again, I want to
> >> change but that's separate):
> >> * l2-acl
> >> * ipv4-acl
> >> * ipv6-acl
> >> * mixed-ipv4-acl
> >> * mixed-ipv6-acl
> >> * l2-l3-ipv4-ipv6-acl
> >> * tcp-acl
> >> * udp-acl
> >> * icmp-acl
> >> * any-acl
> >>
> >> As per your second example above we have an ipv4-acl container
> >> under matches, i.e. /access-list/acl/aces/ace/matches/ipv4-acl.
> >> It is only possible to define ipv4 matches if one of the
> >> following features are present:
> >> * ipv4-acl
> >> * mixed-ipv4-acl
> >> * l2-l3-ipv4-ipv6-acl
> >>
> >> Thus you write an if-feature statement to reflect that, like
> >> this:
> >>
> >> if-feature "ipv4-acl mixed-ipv4-acl l2-l3-ipv4-ipv6-acl";
> >>
> >> So you see, the tight coupling you have between the data
> >> structure and the if-features isn't necessary.
> >>
> >> I think the structure should first be established without
> >> features and then features can be inserted where they make sense
> >> to make part of the model optional. Starting with the list of
> >> features and then designing the structure around them makes for a
> >> much less natural data structure IMHO.
> >>
> >> Kind regards,
> >>    Kristian.
> > Mahesh Jethanandani
> > mjethanandani@gmail.com
> > _______________________________________________
> > netmod mailing list
> > netmod@ietf.org
> > https://www.ietf.org/mailman/listinfo/netmod
> > .
> >
>