Subject: [netmod] Feedback on ietf-access-control-list

From: "Joe Clarke" <[email-masked]>
Date: Mon, 17 Jul 2017 12:02:37 -0400

I’ve read the latest -11 draft, and I have three comments.  I’ve 
been trying to follow the schema changes and make sure I can properly 
get my head around them in order to spot any obvious shortcomings.  The 
example in the draft is helpful (as most examples are).  However, in the 
latest draft, it seems that the example has drifted from what the model 
specifies.

I want to make sure I’m not missing something.  The current example 
text is:

<?xml version='1.0' encoding='UTF-8'?>
      <data xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
        <access-lists xmlns="urn:ietf:params:xml:ns:yang:
         ietf-access-control-list">
          <acl>
            <acl-name>sample-ipv4-acl</acl-name>
            <acl-type>ipv4</acl-type>
            <access-list-entries>
              <ace>
                <rule-name>rule1</rule-name>
                <matches>
                  <source-ipv4-network>
                    10.10.10.1/24
                  </source-ipv4-network>
                  <destination-ipv4-network>
                    11.11.11.1/24
                  </destination-ipv4-network>
                </matches>
                <actions>
                  <deny />
                </actions>
                <protocol>
                 tcp
                </protocol>
              </ace>
            </access-list-entries>
          </acl>
        </access-lists>
      </data>

But I believe in the current model, it should be:

<?xml version='1.0' encoding='UTF-8'?>
      <data xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
        <access-lists xmlns="urn:ietf:params:xml:ns:yang:
         ietf-access-control-list">
          <acl>
            <acl-name>sample-ipv4-acl</acl-name>
            <acl-type>ipv4-acl</acl-type>
            <access-list-entries>
              <ace>
                <rule-name>rule1</rule-name>
                <matches>
                  <ipv4-acl>
                    <source-ipv4-network>
                      10.10.10.1/24
                    </source-ipv4-network>
                    <destination-ipv4-network>
                      11.11.11.1/24
                    </destination-ipv4-network>
                    <protocol>
                      tcp
                    </protocol>
                  </ipv4-acl>
                </matches>
                <actions>
                  <deny />
                </actions>
              </ace>
            </access-list-entries>
          </acl>
        </access-lists>
      </data>

Based on the presentation, this helps with the typing, and this makes 
sense to me.

===

Should there be a default for “logging”?  Currently, it’s defined 
to be an optional boolean without a default.  One could infer that if 
it’s not present, logging is disabled, but the text does not make that 
explicit.  I think it would be good to make this default false.

===

While I see in the appendix uses for acl-oper-data, it is not clear from 
the model or main draft text what this would be used for.  Again, I 
inferred what the appendix did, but perhaps there could be some main 
draft text that speaks to this container explicitly.

Joe

---

From: Mahesh Jethanandani <[email-masked]>
Date: Wed, 19 Jul 2017 07:42:13 +0200

Joe,

> On Jul 17, 2017, at 6:02 PM, Joe Clarke <[email-masked]> wrote:
> 
> I’ve read the latest -11 draft, and I have three comments.  I’ve been trying to follow the schema changes and make sure I can properly get my head around them in order to spot any obvious shortcomings.  The example in the draft is helpful (as most examples are).  However, in the latest draft, it seems that the example has drifted from what the model specifies.

Noted. We will fix it.

> 
> I want to make sure I’m not missing something.  The current example text is:
> 
> Based on the presentation, this helps with the typing, and this makes sense to me.
> 
> ===
> 
> Should there be a default for “logging”?  Currently, it’s defined to be an optional boolean without a default.  One could infer that if it’s not present, logging is disabled, but the text does not make that explicit.  I think it would be good to make this default false.

Agreed. Will add default and update the description.

> 
> ===
> 
> While I see in the appendix uses for acl-oper-data, it is not clear from the model or main draft text what this would be used for.  Again, I inferred what the appendix did, but perhaps there could be some main draft text that speaks to this container explicitly.

Gathering of statistics has been identified as an open issue, opened in GitHub and a separate thread started on netmod. Will identify the solution we are looking for in that thread.

Thanks.

> 
> Joe
> 
> _______________________________________________
> netmod mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/netmod

Mahesh Jethanandani
[email-masked]

---

From: "Joe Clarke" <[email-masked]>
Date: Wed, 19 Jul 2017 02:43:38 -0400

On 19 Jul 2017, at 1:42, Mahesh Jethanandani wrote:

>> get my head around them in order to spot any obvious shortcomings.  
>> The example in the draft is helpful (as most examples are).  However, 
>> in the latest draft, it seems that the example has drifted from what 
>> the model specifies.
>
> Noted. We will fix it.

Thanks.  So my understanding is correct.

>> While I see in the appendix uses for acl-oper-data, it is not clear 
>> from the model or main draft text what this would be used for.  
>> Again, I inferred what the appendix did, but perhaps there could be 
>> some main draft text that speaks to this container explicitly.
>
> Gathering of statistics has been identified as an open issue, opened 
> in GitHub and a separate thread started on netmod. Will identify the 
> solution we are looking for in that thread.

Understood.  Thanks for the update.

Joe