[ovs-dev] [PATCH] vtep: add ACLs to VTEP schema

[Bruce Davie]

> On Sep 10, 2015, at 3:07 PM, Justin Pettit <jpettit at nicira.com> wrote:
> 
> 
>> On Aug 25, 2015, at 1:03 PM, Bruce Davie <bdavie at nicira.com> wrote:
>> 
>> diff --git a/vtep/vtep.xml b/vtep/vtep.xml
>> index ff8d0fe..a554dcf 100644
>> --- a/vtep/vtep.xml
>> +++ b/vtep/vtep.xml
>> @@ -367,7 +367,7 @@
>>      <group title="BFD Local Configuration">
>>        <p>
>>          The HSC writes the key-value pairs in the
>> -          <ref column="bfd_config_local"/> column to specifiy the local
>> +          <ref column="bfd_config_local"/> column to specify the local
> 
> There are a few typo fixes in this patch.  Normally, with a big change like this patch, I wouldn't roll those in.  The main reason is that if the patch gets reverted at some point, these fixes would be lost, too.  It's probably not worth re-spinning this patch, but thought I'd mention it.

Yes, I realized that probably wasn’t ideal. I don’t mind splitting them out.

> 
>> +    <column name="acl_bindings">
>> +      <p>
>> +        Attach Access Control Lists (ACLs) to the physical port. The
>> +        column consists of a map of VLAN tags to <ref table="ACL"/>s. If the value of
>> +        the VLAN tag in the map is 0, this means that the ACL is
>> +        associated with the entire physical port. Non-zero values mean
>> +        that the ACL is to be applied only on packets carrying that VLAN
>> +        tag value. Switches will not necessarily support matching on the
>> +        VLAN tag for all ACLs, and unsupported ACL bindings will cause
>> +        errors to be reported.
>> +      </p>
> 
> This isn't related to this patch, but I didn't see what happens when "vlan_bindings" or "vlan_stats" uses a key of 0.

It seems that this was documented once but fell out of the schema at some point. I believe a value of zero indicates untagged traffic in those fields, and we should do a new patch to address that.

As noted previously, I also intend to update the text quoted above to include a statement about not mixing zero and non-zero values on the same physical port. I don’t believe such a restriction needs to be applied in the places you mentioned.

> 
>> @@ -851,6 +870,15 @@
>>      One or more static routes, mapping IP prefixes to next hop IP addresses.
>>    </column>
>> 
>> +    <column name="acl_binding">
>> +      Maps ACLs to logical router interfaces. The router interfaces
>> +      are indicated using IP address notation, and must be the same
>> +      interfaces created in the <ref column="switch_binding"/>
>> +      column. For example, an ACL could be associated with the logical
>> +      router interface with an address of 192.68.1.1 as defined in the
>> +      example above.
>> +    </column>
> 
> The Physical_Port table has an "invalid_ACL_binding" error.  Do you think it's worth adding one to the Logical_Router table, too?

Yes, that seems reasonable.

> 
>> +  <table name="ACL_entry">
>> +    <p>
>> +      Describes the individual entries that comprise an Access Control List.
>> +    </p>
>> +    <p>
>> +      Each entry in the table is a single rule to match on certain
>> +      header fields. While there are a large number of fields that can
>> +      be matched on, most hardware cannot match on arbitrary
>> +      combinations of fields. It is common to match on either L2
>> +      fields (described below in the L2 group of columns) or L3/L4 fields
>> +      (the L3/L4 group of columns) but not both. The hardware switch
>> +      controller may log an error if an ACL entry requires it to match
>> +      on an incompatible mixture of fields.
> 
> Do you think it's worth mentioning that all packets are allowed by default?

Sure.

> 
>> +    <column name="sequence">
>> +      <p>
>> +        The sequence number for the ACL entry for the purpose of
>> +        ordering entries in an ACL.
> 
> I think it would be helpful to indicate whether a larger number is more likely to hit or less likely.
> 

Yes.

>> +      </column>
>> +            <column name="ethertype">
>> +        <p>
>> +          Ethertype in hexadecimal, in the form
>> +          <var>0xAAA</var> 
> 
> This is very minor, but I think it may be clearer with a fourth "A”.
Agreed.

> 

>> +      <column name="source_port_min">
>> +        <p>
>> +          Lower end of the range of source port values.
>> +        </p>
>> +      </column>
>> +      <column name="source_port_max">
>> +        <p>
>> +          Upper end of the range of source port values.
>> +        </p>
>> +      </column>
>> +      <column name="dest_port_min">
>> +        <p>
>> +          Lower end of the range of destination port values.
>> +        </p>
>> +      </column>
>> +      <column name="dest_port_max">
>> +        <p>
>> +          Upper end of the range of destination port values.
>> +        </p>
> 
> For all of these, it would be good to indicate if the chosen value is included.  (I assume it is.)
> 

Agreed.

>> +      <column name="tcp_flags">
>> +        <p>
>> +          Integer representing the value of TCP flags to match.
>> +        </p>
>> +      </column>
>> +      <column name="tcp_flags_mask">
>> +        <p>
>> +          Integer representing the mask to apply when matching TCP flags.
>> +        </p>
> 
> It might be nice to include an example for "tcp_flags" and its mask.
> 

Yes, good suggestion.
> Otherwise, it looks good.
> 

OK, I’ll work on an update.

Thanks
Bruce


> Thanks,
> 
> --Justin
> 
>