diff mbox

[ovs-dev,1/2] ovn-nb: Add direction and reduce max priority for ACLs.

Message ID 1441413542-92140-1-git-send-email-jpettit@nicira.com
State Accepted
Headers show

Commit Message

Justin Pettit Sept. 5, 2015, 12:39 a.m. UTC 
Introduce a new "direction" column to the ACL table that accepts the
values "to-lport" and "from-lport".  Also reserve the ACL priority 65535
for return traffic associated with the "allow-related" action.

Signed-off-by: Justin Pettit <jpettit@nicira.com>
---
 ovn/ovn-nb.ovsschema |    4 ++-
 ovn/ovn-nb.xml       |   66 +++++++++++++++++++++++++++++++++++--------------
 2 files changed, 50 insertions(+), 20 deletions(-)

Comments

Ben Pfaff Sept. 5, 2015, 11:40 p.m. UTC | #1 
On Fri, Sep 04, 2015 at 05:39:01PM -0700, Justin Pettit wrote:
> Introduce a new "direction" column to the ACL table that accepts the
> values "to-lport" and "from-lport".  Also reserve the ACL priority 65535
> for return traffic associated with the "allow-related" action.
> 
> Signed-off-by: Justin Pettit <jpettit@nicira.com>

I'd prefer to squash this with the commit (presumably patch 2/2) that
actually implements the newly documented behavior.

Here:

        <ref table="Logical_Flow" db="OVN_Southbound"/> table.  The
        <code>outport</code> logical port is only available in the
        <code>to-lport</code> direction.

I'd consider adding a parenthetical to make it perfectly clear, e.g.:

        <ref table="Logical_Flow" db="OVN_Southbound"/> table.  The
        <code>outport</code> logical port is only available in the
        <code>to-lport</code> direction (the <code>inport</code> is
        available in both directions).

Acked-by: Ben Pfaff <blp@nicira.com>
Ben Pfaff Sept. 5, 2015, 11:43 p.m. UTC | #2 
On Sat, Sep 05, 2015 at 04:40:41PM -0700, Ben Pfaff wrote:
> On Fri, Sep 04, 2015 at 05:39:01PM -0700, Justin Pettit wrote:
> > Introduce a new "direction" column to the ACL table that accepts the
> > values "to-lport" and "from-lport".  Also reserve the ACL priority 65535
> > for return traffic associated with the "allow-related" action.
> > 
> > Signed-off-by: Justin Pettit <jpettit@nicira.com>
> 
> I'd prefer to squash this with the commit (presumably patch 2/2) that
> actually implements the newly documented behavior.

Oh, I see that 2/2 doesn't.  Why not?  It shouldn't be hard, at least
not to divide the ACLs into ingress and egress pipelines; I do
understand that the stateful ACLs are more work.
diff mbox

Patch

diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index f17b649..20fdc79 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -54,7 +54,9 @@ 
             "columns": {
                 "priority": {"type": {"key": {"type": "integer",
                                               "minInteger": 1,
-                                              "maxInteger": 65535}}},
+                                              "maxInteger": 65534}}},
+                "direction": {"type": {"key": {"type": "string",
+                                            "enum": ["set", ["from-lport", "to-lport"]]}}},
                 "match": {"type": "string"},
                 "action": {"type": {"key": {"type": "string",
                                             "enum": ["set", ["allow", "allow-related", "drop", "reject"]]}}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index ade8164..4289631 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -211,22 +211,55 @@ 
     </p>
 
     <column name="priority">
-      The ACL rule's priority.  Rules with numerically higher priority take
-      precedence over those with lower.  If two ACL rules with the same
-      priority both match, then the one actually applied to a packet is
-      undefined.
+      <p>
+        The ACL rule's priority.  Rules with numerically higher priority
+        take precedence over those with lower.  If two ACL rules with
+        the same priority both match, then the one actually applied to a
+        packet is undefined.
+      </p>
+
+      <p>
+        Return traffic from an <code>allow-related</code> flow is always
+        allowed and cannot be changed through an ACL.
+      </p>
+    </column>
+
+    <column name="direction">
+      <p>Direction of the traffic to which this rule should apply:</p>
+      <ul>
+        <li>
+          <code>from-lport</code>: Used to implement filters on traffic
+          arriving from a logical port.  These rules are applied to the
+          logical switch's ingress pipeline.
+        </li>
+        <li>
+          <code>to-lport</code>: Used to implement filters on traffic
+          forwarded to a logical port.  These rules are applied to the
+          logical switch's egress pipeline.
+        </li>
+      </ul>
     </column>
 
     <column name="match">
-      The packets that the ACL should match, in the same expression language
-      used for the <ref column="match" table="Logical_Flow"
-      db="OVN_Southbound"/> column in the OVN Southbound database's <ref
-      table="Logical_Flow" db="OVN_Southbound"/> table.  Match
-      <code>inport</code> and <code>outport</code> against names of logical
-      ports within <ref column="lswitch"/> to implement ingress and egress
-      ACLs, respectively.  In logical switches connected to logical routers,
-      the special port name <code>ROUTER</code> refers to the logical router
-      port.
+      <p>
+        The packets that the ACL should match, in the same expression
+        language used for the <ref column="match" table="Logical_Flow"
+        db="OVN_Southbound"/> column in the OVN Southbound database's
+        <ref table="Logical_Flow" db="OVN_Southbound"/> table.  The
+        <code>outport</code> logical port is only available in the
+        <code>to-lport</code> direction.
+      </p>
+
+      <p>
+        By default all traffic is allowed.  When writing a more
+        restrictive policy, it is important to remember to allow flows
+        such as ARP and IPv6 neighbor discovery packets.
+      </p>
+
+      <p>
+        In logical switches connected to logical routers, the special
+        port name <code>ROUTER</code> refers to the logical router port.
+      </p>
     </column>
 
     <column name="action">
@@ -249,14 +282,9 @@ 
         <li>
           <code>reject</code>: Drop the packet, replying with a RST for TCP or
           ICMP unreachable message for other IP-based protocols.
+          <code>Not implemented--currently treated as drop</code>
         </li>
       </ul>
-
-      <p>
-	Only <code>allow</code> and <code>drop</code> are implemented:
-	<code>allow-related</code> is currently treated as <code>allow</code>,
-	and <code>reject</code> as <code>drop</code>.
-      </p>
     </column>
 
     <column name="log">