[ovs-dev] ovn: Add stateful ACL support.
diff mbox

Message ID DD3D6823-32F7-47C8-A363-BB94656461D7@nicira.com
State Accepted
Headers show

Commit Message

Justin Pettit Oct. 16, 2015, 7:25 a.m. UTC
> On Oct 15, 2015, at 5:21 PM, Ben Pfaff <blp@nicira.com> wrote:
> 
> On Thu, Oct 15, 2015 at 10:32:51AM -0700, Justin Pettit wrote:
>> Add support for the "allow-related" ACL action.  This is dependent on
>> the OVS conntrack functionality, which is not available on all platforms
>> or kernel versions.
>> 
>> Here is a sample policy that will allow all tenants in logical switch
>> "ls0" to SSH to each other.  Anyone can make an HTTP request to "lp0".
>> All other IP traffic is dropped:
>> 
>>  ovn-nbctl acl-add ls0 from-lport 100 ip allow-related
>>  ovn-nbctl acl-add ls0 to-lport 100 tcp.dst==22 allow-related
>>  ovn-nbctl acl-add ls0 to-lport 100 "outport == \"lp0\" \
>>            && tcp.dst==80" allow-related
>>  ovn-nbctl acl-add ls0 to-lport 1 ip drop
>> 
>> Note: Kernel conntrack support is checked into the mainline Linux
>> kernel, but hasn't been backported to the main OVS repo yet.
>> ---
>> I've pushed this patch on a partial backport of conntrack here:
>> 
>>    https://github.com/justinpettit/ovs/tree/ovn-acl
> 
> Thanks!  This is going to be awesome.
> 
> This lacks a Signed-off-by.

Whoops.  Fixed.

> ovn-northd.xml needs an update to explain all the new flows and
> renumbered flow tables.

I totally missed that.  Thanks.

> I get one "sparse" warning:
> 
>    ../ovn/lib/actions.c:151:13: warning: incorrect type in assignment (different base types)
>    ../ovn/lib/actions.c:151:13:    expected unsigned short [unsigned] [usertype] alg
>    ../ovn/lib/actions.c:151:13:    got restricted ovs_be16

D'oh.  Fixed.

> In symtab_init() in ovn/controller/lflow.c, I think it would be a little
> better to define ct.trk as a subfield, instead of a predicate, since
> subfields are a little more general-purpose.

I couldn't get it to work by making it a predicate.  I think it's related to the other ct_state fields depending on it, but let's discuss it tomorrow, because I'm probably missing something.

> Acked-by: Ben Pfaff <blp@nicira.com>

Thanks!

I went ahead an pushed it.  I've appended an incremental.

--Justin


-=-=-=-=-=-=-=-=-=-=-=-

Patch
diff mbox

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 5a9562e..aebe5ce 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -206,7 +206,7 @@  emit_ct(struct action_context *ctx, bool recirc_next, bool c
     ct->zone_src.n_bits = 16;
 
     /* We do not support ALGs yet. */
-    ct->alg = htons(0);
+    ct->alg = 0;
 
     /* CT only works with IP, so set up a prerequisite. */
     struct expr *expr;
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 3c5d362..f51852e 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -137,24 +137,63 @@ 
       be dropped.
     </p>
 
-    <h2>Ingress table 1: <code>from-lport</code> ACLs</h2>
+    <h2>Ingress Table 1: <code>from-lport</code> Pre-ACLs</h2>
+
+    <p>
+      Ingress table 1 prepares flows for possible stateful ACL processing
+      in table 2.  It contains a priority-0 flow that simply moves
+      traffic to table 2.  If stateful ACLs are used in the logical
+      datapath, a priority-100 flow is added that sends IP packets to
+      the connection tracker before advancing to table 2.
+    </p>
+
+    <h2>Ingress table 2: <code>from-lport</code> ACLs</h2>
 
     <p>
       Logical flows in this table closely reproduce those in the
-      <code>ACL</code> table in the <code>OVN_Northbound</code> database for
-      the <code>from-lport</code> direction.  <code>allow</code> and
-      <code>allow-related</code> ACLs translate into logical flows with the
-      <code>next;</code> action, others to <code>drop;</code>.  The
-      <code>priority</code> values from the <code>ACL</code> table are used
-      directly.
+      <code>ACL</code> table in the <code>OVN_Northbound</code> database
+      for the <code>from-lport</code> direction.  <code>allow</code>
+      ACLs translate into logical flows with the <code>next;</code>
+      action, <code>allow-related</code> ACLs translate into logical
+      flows with the <code>ct_next;</code> action, other ACLs translate
+      to <code>drop;</code>.  The <code>priority</code> values from the
+      <code>ACL</code> table are used directly.
     </p>
 
     <p>
-      Ingress table 1 also contains a priority 0 flow with action
-      <code>next;</code>, so that ACLs allow packets by default.
+      Ingress table 2 also contains a priority 0 flow with action
+      <code>next;</code>, so that ACLs allow packets by default.  If the
+      logical datapath has a statetful ACL, the following flows will
+      also be added:
     </p>
 
-    <h2>Ingress Table 2: Destination Lookup</h2>
+    <ul>
+      <li>
+        A priority-1 flow to commit IP traffic to the connection
+        tracker.  This is needed for the default allow policy because,
+        while the initiater's direction may not have any stateful rules,
+        the server's may and then its return traffic would not be known
+        and marked as invalid.
+      </li>
+
+      <li>
+        A priority-65535 flow that allows any traffic that has been
+        committed to the connection tracker (i.e., established flows).
+      </li>
+
+      <li>
+        A priority-65535 flow that allows any traffic that is considered
+        related to a committed flow in the connection tracker (e.g., an
+        ICMP Port Unreachable from a non-listening UDP port).
+      </li>
+
+      <li>
+        A priority-65535 flow that drops all traffic marked by the
+        connection tracker as invalid.
+      </li>
+    </ul>
+
+    <h2>Ingress Table 3: Destination Lookup</h2>
 
     <p>
       This table implements switching behavior.  It contains these logical
@@ -185,13 +224,20 @@ 
       </li>
     </ul>
 
-    <h2>Egress Table 0: <code>to-lport</code> ACLs</h2>
+    <h2>Egress Table 0: <code>to-lport</code> Pre-ACLs</h2>
+
+    <p>
+      This is similar to ingress table 1 except for <code>to-lport</code>
+      traffic.
+    </p>
+
+    <h2>Egress Table 1: <code>to-lport</code> ACLs</h2>
 
     <p>
-      This is similar to ingress table 1 except for <code>to-lport</code> ACLs.
+      This is similar to ingress table 2 except for <code>to-lport</code> ACLs.
     </p>
 
-    <h2>Egress Table 1: Egress Port Security</h2>
+    <h2>Egress Table 2: Egress Port Security</h2>
 
     <p>
       This is similar to the ingress port security logic in ingress table 0,