diff mbox

[ovs-dev,4/7] ovn-northd: Introduce stateful table.

Message ID 1467188231-10194-5-git-send-email-guru@ovn.org
State Accepted
Headers show

Commit Message

Gurucharan Shetty June 29, 2016, 8:17 a.m. UTC
Currently, the only use of stateful services in conntrack is
OVN ACLs. In table ACL, we commit the packet to conntrack
via ct_commit action.

As we introduce more stateful services, the ACL feature will
have to share the conntrack module with others. As
preparation for more stateful features like load balancing,
this commit introduces a new stateful table
that is responsible to commit packets to conntrack via
ct_commit action. If ACL table needs to commit a packet,
it sets 'reg0[1]' as 1. Stateful table in-turn will commit
the packet if 'reg0[1]' is 1.

Signed-off-by: Gurucharan Shetty <guru@ovn.org>
---
 ovn/northd/ovn-northd.8.xml | 40 ++++++++++++++++++++++++++++------------
 ovn/northd/ovn-northd.c     | 41 ++++++++++++++++++++++++++++++++---------
 2 files changed, 60 insertions(+), 21 deletions(-)

Comments

Zong Kai LI June 30, 2016, 8:41 a.m. UTC | #1
>
> @@ -1429,9 +1432,9 @@ build_acls(struct ovn_datapath *od, struct hmap
> *lflows)
>           * and then its return traffic would not have an associated
>           * conntrack entry and would return "+invalid". */
>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1, "ip",
> -                      "ct_commit; next;");
> +                      REGBIT_CONNTRACK_COMMIT" = 1; next;");
>          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1, "ip",
> -                      "ct_commit; next;");
> +                      REGBIT_CONNTRACK_COMMIT" = 1; next;");
>
>
Just like I commented in patch 3, try to directly resubmit non-stateful
stuff to ARP_RSP table, for they will meet just another "next;" in stateful
table.

         /* Ingress and Egress ACL Table (Priority 65535).
>           *
> @@ -1484,7 +1487,9 @@ build_acls(struct ovn_datapath *od, struct hmap
> *lflows)
>               * direction may not have any stateful rules, the server's
>               * may and then its return traffic would not have an
>               * associated conntrack entry and would return "+invalid". */
> -            const char *actions = has_stateful ? "ct_commit; next;" :
> "next;";
> +            const char *actions = has_stateful
> +                                    ? REGBIT_CONNTRACK_COMMIT" = 1; next;"
> +                                    : "next;";
>

ditto, try to using resubmit.


> static void
> +build_stateful(struct ovn_datapath *od, struct hmap *lflows)
> +{
> +    /* Ingress and Egress stateful Table (Priority 0): Packets are
> +     * allowed by default. */
> +    ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 0, "1", "next;");
> +    ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 0, "1", "next;");
>

if we allow non-stateful stuff to skip PRE_STATEFUL and STATEFUL table, we
will don't need the default next action in the two tables.

Thanks,
Zong Kai, LI
Gurucharan Shetty June 30, 2016, 2:59 p.m. UTC | #2
On 30 June 2016 at 01:41, Zong Kai LI <zealokii@gmail.com> wrote:

> >
> > @@ -1429,9 +1432,9 @@ build_acls(struct ovn_datapath *od, struct hmap
> > *lflows)
> >           * and then its return traffic would not have an associated
> >           * conntrack entry and would return "+invalid". */
> >          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1, "ip",
> > -                      "ct_commit; next;");
> > +                      REGBIT_CONNTRACK_COMMIT" = 1; next;");
> >          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1, "ip",
> > -                      "ct_commit; next;");
> > +                      REGBIT_CONNTRACK_COMMIT" = 1; next;");
> >
> >
> Just like I commented in patch 3, try to directly resubmit non-stateful
> stuff to ARP_RSP table, for they will meet just another "next;" in stateful
> table.
>
> A future commit in this series introduces a new table "lb" between "ACLs"
and "stateful" table. If I do the jump, I will miss any load balancing
rules. Ditto for all your other comments.


>          /* Ingress and Egress ACL Table (Priority 65535).
> >           *
> > @@ -1484,7 +1487,9 @@ build_acls(struct ovn_datapath *od, struct hmap
> > *lflows)
> >               * direction may not have any stateful rules, the server's
> >               * may and then its return traffic would not have an
> >               * associated conntrack entry and would return "+invalid".
> */
> > -            const char *actions = has_stateful ? "ct_commit; next;" :
> > "next;";
> > +            const char *actions = has_stateful
> > +                                    ? REGBIT_CONNTRACK_COMMIT" = 1;
> next;"
> > +                                    : "next;";
> >
>
> ditto, try to using resubmit.
>
>
> > static void
> > +build_stateful(struct ovn_datapath *od, struct hmap *lflows)
> > +{
> > +    /* Ingress and Egress stateful Table (Priority 0): Packets are
> > +     * allowed by default. */
> > +    ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 0, "1", "next;");
> > +    ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 0, "1", "next;");
> >
>
> if we allow non-stateful stuff to skip PRE_STATEFUL and STATEFUL table, we
> will don't need the default next action in the two tables.
>
> Thanks,
> Zong Kai, LI
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
Ben Pfaff July 3, 2016, 4:42 a.m. UTC | #3
On Wed, Jun 29, 2016 at 01:17:08AM -0700, Gurucharan Shetty wrote:
> Currently, the only use of stateful services in conntrack is
> OVN ACLs. In table ACL, we commit the packet to conntrack
> via ct_commit action.
> 
> As we introduce more stateful services, the ACL feature will
> have to share the conntrack module with others. As
> preparation for more stateful features like load balancing,
> this commit introduces a new stateful table
> that is responsible to commit packets to conntrack via
> ct_commit action. If ACL table needs to commit a packet,
> it sets 'reg0[1]' as 1. Stateful table in-turn will commit
> the packet if 'reg0[1]' is 1.
> 
> Signed-off-by: Gurucharan Shetty <guru@ovn.org>

s/initiater's/initiator's/ in the documentation.

The code introduces symbolic names, or at least macros, for reg0[0] and
reg0[1].  Do you think that the documentation would be improved by
giving these bits names for reference?  I do not know for sure, but
maybe it would make the documentation easier to understand.

Acked-by: Ben Pfaff <blp@ovn.org>
Gurucharan Shetty July 3, 2016, 8:06 p.m. UTC | #4
On 2 July 2016 at 21:42, Ben Pfaff <blp@ovn.org> wrote:

> On Wed, Jun 29, 2016 at 01:17:08AM -0700, Gurucharan Shetty wrote:
> > Currently, the only use of stateful services in conntrack is
> > OVN ACLs. In table ACL, we commit the packet to conntrack
> > via ct_commit action.
> >
> > As we introduce more stateful services, the ACL feature will
> > have to share the conntrack module with others. As
> > preparation for more stateful features like load balancing,
> > this commit introduces a new stateful table
> > that is responsible to commit packets to conntrack via
> > ct_commit action. If ACL table needs to commit a packet,
> > it sets 'reg0[1]' as 1. Stateful table in-turn will commit
> > the packet if 'reg0[1]' is 1.
> >
> > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
>
> s/initiater's/initiator's/ in the documentation.
>

Fixed it.

>
> The code introduces symbolic names, or at least macros, for reg0[0] and
> reg0[1].  Do you think that the documentation would be improved by
> giving these bits names for reference?  I do not know for sure, but
> maybe it would make the documentation easier to understand.
>

I couldn't quite fit in a non-controversial wording that could be committed
without a review. I will come up with a separate documentation fix up.


>
> Acked-by: Ben Pfaff <blp@ovn.org>
>
Thanks, I applied the first 4 patches of the series.

> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
diff mbox

Patch

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index a63e1cb..f7179fd 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -271,9 +271,10 @@ 
       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_commit; next;</code> actions, other ACLs
-      translate to <code>drop;</code>.  The <code>priority</code> values
-      from the <code>ACL</code> table have a limited range and have 1000
+      flows with the <code>reg0[1] = 1; next;</code> actions (which acts
+      as a hint for the next tables to commit the connection to conntrack),
+      other ACLs translate to <code>drop;</code>.  The <code>priority</code>
+      values from the <code>ACL</code> table have a limited range and have 1000
       added to them to leave room for OVN default flows at both higher
       and lower priorities.
     </p>
@@ -287,11 +288,11 @@ 
 
     <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.
+        A priority-1 flow that sets the hint to commit IP traffic to the
+        connection tracker (with action <code>reg0[1] = 1; next;</code>).  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>
@@ -311,7 +312,16 @@ 
       </li>
     </ul>
 
-    <h3>Ingress Table 6: ARP responder</h3>
+    <h3>Ingress Table 6: Stateful</h3>
+
+    <p>
+      It contains a priority-0 flow that simply moves traffic to the next
+      table.  A priority-100 flow commits packets to connection tracker using
+      <code>ct_commit; next;</code> action based on a hint provided by
+      the previous tables (with a match for <code>reg0[1] == 1</code>).
+    </p>
+
+    <h3>Ingress Table 7: ARP responder</h3>
 
     <p>
       This table implements ARP responder for known IPs.  It contains these
@@ -356,7 +366,7 @@  output;
       </li>
     </ul>
 
-    <h3>Ingress Table 7: Destination Lookup</h3>
+    <h3>Ingress Table 8: Destination Lookup</h3>
 
     <p>
       This table implements switching behavior.  It contains these logical
@@ -407,7 +417,13 @@  output;
       <code>to-lport</code> ACLs.
     </p>
 
-    <h3>Egress Table 3: Egress Port Security - IP</h3>
+    <h3>Egress Table 3: Stateful</h3>
+
+    <p>
+      This is similar to ingress table <code>Stateful</code>.
+    </p>
+
+    <h3>Egress Table 4: Egress Port Security - IP</h3>
 
     <p>
       This is similar to the port security logic in table
@@ -417,7 +433,7 @@  output;
       <code>ip4.src</code> and <code>ip6.src</code>
     </p>
 
-    <h3>Egress Table 4: Egress Port Security - L2</h3>
+    <h3>Egress Table 5: Egress Port Security - L2</h3>
 
     <p>
       This is similar to the ingress port security logic in ingress table
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 952a118..ba80fbe 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -94,15 +94,17 @@  enum ovn_stage {
     PIPELINE_STAGE(SWITCH, IN,  PRE_ACL,        3, "ls_in_pre_acl")      \
     PIPELINE_STAGE(SWITCH, IN,  PRE_STATEFUL,   4, "ls_in_pre_stateful")    \
     PIPELINE_STAGE(SWITCH, IN,  ACL,            5, "ls_in_acl")          \
-    PIPELINE_STAGE(SWITCH, IN,  ARP_RSP,        6, "ls_in_arp_rsp")      \
-    PIPELINE_STAGE(SWITCH, IN,  L2_LKUP,        7, "ls_in_l2_lkup")      \
+    PIPELINE_STAGE(SWITCH, IN,  STATEFUL,       6, "ls_in_stateful")     \
+    PIPELINE_STAGE(SWITCH, IN,  ARP_RSP,        7, "ls_in_arp_rsp")      \
+    PIPELINE_STAGE(SWITCH, IN,  L2_LKUP,        8, "ls_in_l2_lkup")      \
                                                                       \
     /* Logical switch egress stages. */                               \
     PIPELINE_STAGE(SWITCH, OUT, PRE_ACL,      0, "ls_out_pre_acl")     \
     PIPELINE_STAGE(SWITCH, OUT, PRE_STATEFUL, 1, "ls_out_pre_stateful")  \
-    PIPELINE_STAGE(SWITCH, OUT, ACL,          2, "ls_out_acl")         \
-    PIPELINE_STAGE(SWITCH, OUT, PORT_SEC_IP,  3, "ls_out_port_sec_ip")    \
-    PIPELINE_STAGE(SWITCH, OUT, PORT_SEC_L2,  4, "ls_out_port_sec_l2")    \
+    PIPELINE_STAGE(SWITCH, OUT, ACL,          2, "ls_out_acl")            \
+    PIPELINE_STAGE(SWITCH, OUT, STATEFUL,     3, "ls_out_stateful")       \
+    PIPELINE_STAGE(SWITCH, OUT, PORT_SEC_IP,  4, "ls_out_port_sec_ip")    \
+    PIPELINE_STAGE(SWITCH, OUT, PORT_SEC_L2,  5, "ls_out_port_sec_l2")    \
                                                                       \
     /* Logical router ingress stages. */                              \
     PIPELINE_STAGE(ROUTER, IN,  ADMISSION,   0, "lr_in_admission")    \
@@ -131,6 +133,7 @@  enum ovn_stage {
 #define OVN_ACL_PRI_OFFSET 1000
 
 #define REGBIT_CONNTRACK_DEFRAG "reg0[0]"
+#define REGBIT_CONNTRACK_COMMIT "reg0[1]"
 
 /* Returns an "enum ovn_stage" built from the arguments. */
 static enum ovn_stage
@@ -1429,9 +1432,9 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
          * and then its return traffic would not have an associated
          * conntrack entry and would return "+invalid". */
         ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1, "ip",
-                      "ct_commit; next;");
+                      REGBIT_CONNTRACK_COMMIT" = 1; next;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1, "ip",
-                      "ct_commit; next;");
+                      REGBIT_CONNTRACK_COMMIT" = 1; next;");
 
         /* Ingress and Egress ACL Table (Priority 65535).
          *
@@ -1484,7 +1487,9 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
              * direction may not have any stateful rules, the server's
              * may and then its return traffic would not have an
              * associated conntrack entry and would return "+invalid". */
-            const char *actions = has_stateful ? "ct_commit; next;" : "next;";
+            const char *actions = has_stateful
+                                    ? REGBIT_CONNTRACK_COMMIT" = 1; next;"
+                                    : "next;";
             ovn_lflow_add(lflows, od, stage,
                           acl->priority + OVN_ACL_PRI_OFFSET,
                           acl->match, actions);
@@ -1497,7 +1502,8 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
             ds_put_format(&match, "ct.new && (%s)", acl->match);
             ovn_lflow_add(lflows, od, stage,
                           acl->priority + OVN_ACL_PRI_OFFSET,
-                          ds_cstr(&match), "ct_commit; next;");
+                          ds_cstr(&match),
+                          REGBIT_CONNTRACK_COMMIT" = 1; next;");
 
             ds_destroy(&match);
         } else if (!strcmp(acl->action, "drop")) {
@@ -1515,6 +1521,22 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows)
 }
 
 static void
+build_stateful(struct ovn_datapath *od, struct hmap *lflows)
+{
+    /* Ingress and Egress stateful Table (Priority 0): Packets are
+     * allowed by default. */
+    ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 0, "1", "next;");
+    ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 0, "1", "next;");
+
+    /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be
+     * committed to conntrack. */
+    ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,
+                  REGBIT_CONNTRACK_COMMIT" == 1", "ct_commit; next;");
+    ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100,
+                  REGBIT_CONNTRACK_COMMIT" == 1", "ct_commit; next;");
+}
+
+static void
 build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
                     struct hmap *lflows, struct hmap *mcgroups)
 {
@@ -1532,6 +1554,7 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
         build_pre_acls(od, lflows, ports);
         build_pre_stateful(od, lflows);
         build_acls(od, lflows);
+        build_stateful(od, lflows);
     }
 
     /* Logical switch ingress table 0: Admission control framework (priority