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

Robert Wilton <rwilton@cisco.com> Thu, 02 November 2017 12:53 UTC

Return-Path: <rwilton@cisco.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 CDE0B13F45D for <netmod@ietfa.amsl.com>; Thu, 2 Nov 2017 05:53:35 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -14.49
X-Spam-Level:
X-Spam-Status: No, score=-14.49 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_HI=-5, SPF_PASS=-0.001, T_FILL_THIS_FORM_SHORT=0.01, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=cisco.com
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 cbzxGS1LlAcA for <netmod@ietfa.amsl.com>; Thu, 2 Nov 2017 05:53:33 -0700 (PDT)
Received: from aer-iport-2.cisco.com (aer-iport-2.cisco.com [173.38.203.52]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 87607138103 for <netmod@ietf.org>; Thu, 2 Nov 2017 05:53:32 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=34439; q=dns/txt; s=iport; t=1509627212; x=1510836812; h=subject:to:cc:references:from:message-id:date: mime-version:in-reply-to; bh=AfK25Ra2W/iGFgKUF/LdqnlZsOs+EHeUwi4bS0/rrFc=; b=V8ZL0M8c1sIxQlrlD8d+mbDjgxEQ5Do+PpfV457KQ0bTaPca4NmtK0cr rgtHM4jMGdofUJhYmAPPs4kM/1UiEvZwlc1JxC88DWxfvDMe0d06Imj35 iv2u8/IzgSpAYccFcLjNn67cL4gHkpkS5O2Byf2AVkhLxhxdW8WWWGVT/ 8=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: A0CZAADmE/tZ/xbLJq1TCRkBAQEBAQEBAQEBAQcBAQEBAYJEQoESbieDfYofdI99JohQjXWCEQoYAQqESU8ChQ0YAQEBAQEBAQEBayiFHgEBBAEBGAlLCxALDgIIIAoCAiEGMAYBDAYCAQGKBwMVEKhngicmhxwNg0gBAQEBAQEBAQEBAQEBAQEBAQEBAQEYBYMug1qBaSkLgnaCaoF9gz+CYgWRXo9zPJADhHmLeIc6jRqBD4dtgTkfOIFsNCEIHRVJgmSEX0E2jUMBAQE
X-IronPort-AV: E=Sophos;i="5.44,334,1505779200"; d="scan'208,217";a="12364"
Received: from aer-iport-nat.cisco.com (HELO aer-core-3.cisco.com) ([173.38.203.22]) by aer-iport-2.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Nov 2017 12:53:30 +0000
Received: from [10.63.23.76] (dhcp-ensft1-uk-vla370-10-63-23-76.cisco.com [10.63.23.76]) by aer-core-3.cisco.com (8.14.5/8.14.5) with ESMTP id vA2CrUF9000999; Thu, 2 Nov 2017 12:53:30 GMT
To: Martin Bjorklund <mbj@tail-f.com>, mjethanandani@gmail.com, netmod@ietf.org
References: <20171102074318.GC12688@spritelink.se> <6359CD50-0F0D-4315-A58B-1D4CF0583475@gmail.com> <ac9fc676-80f7-723d-9a85-c99fbb122476@cisco.com> <20171102.132634.1363976895007772742.mbj@tail-f.com>
From: Robert Wilton <rwilton@cisco.com>
Message-ID: <c90aa6c1-340e-2225-f960-73c1395041c5@cisco.com>
Date: Thu, 02 Nov 2017 12:53:29 +0000
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171102.132634.1363976895007772742.mbj@tail-f.com>
Content-Type: multipart/alternative; boundary="------------7242F4202282C92A9E9E246A"
Content-Language: en-US
Archived-At: <https://mailarchive.ietf.org/arch/msg/netmod/k57ROq97G8UUqe35Pcz6IQOSlBA>
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:53:36 -0000


On 02/11/2017 12:26, Martin Bjorklund wrote:
> 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"?
I agree that aligning the feature names and identity names would be 
helpful.   In my example of how the model could be tweaked (and below) I 
was just reusing the existing ones that are currently defined in the draft.

One further refinement might also be to make the ACL type features a bit 
more hierarchical as well, but I don't know if that makes it too complex?

For example, the model could define separate features for what type of 
ACE matching is supported by the device, separately from what types of 
ACE combinations are allowed.

E.g.

// New 'match type' features.

feature match-on-l2-eth-hdr {
    // Device can match on L2 Ethernet header fields.
}
feature match-on-ipv4-hdr {
    // Device can match on IPv4 header fields.
}
feature match-on-ipv6-hdr {
    // Device can match on IPv6 header fields.
}


The existing ACL type features could then depend on these:

   feature l2-acl {
     if-feature "match-on-l2-eth-hdr";
     description "Layer 2 ACL supported";
   }

   feature ipv4-acl {
     if-feature "match-on-ipv4-hdr";
     description "Layer 3 IPv4 ACL supported";
   }

   feature ipv6-acl {
    if-feature "match-on-ipv6-hdr";
    description "Layer 3 IPv6 ACL supported";
   }

   feature mixed-ipv4-acl {
     if-feature "match-on-l2-eth-hdr and "match-on-ipv4-hdr";
     description "Layer 2 and Layer 3 IPv4 ACL supported";
   }
   ...

This then means that the ACE match fields can be predicated on a "match type" if-feature statement:

   ...
           container matches {
             container l2 {
*if-feature "**match-on-l2-eth-hdr**";*
               when "derived-from(../../../../acl-type, 'acl:eth-acl')";
               uses packet-fields:acl-eth-header-fields;
               description
                 "Rule set for L2 ACL.";
             }
             container ipv4 {
*if-feature "**match-on-ipv4-hdr**";*
               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 {
*if-feature "**match-on-ipv6-hdr**";*
               ...
             }


This would keep the actual model a bit smaller for devices that don't 
support all match types (regardless of what combinations they support).

Thanks,
Rob


>
>
>
> /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
>>> .
>>>
> .
>