ovn: Recirculate packets after a unSNAT.

commit f6fabcc (ofproto-dpif: Mark packets as "untracked" after call to ct().) changed the behavior after a call to ct(). The +trk bit would automatically be unset if packet is sent to ct() and not forked. This caused a bug in the OVN gateway pipeline when there is SNAT rule as well as load-balancing rule. In the OVN gateway pipeline for the gateway router, we had an optimization where the packets sent to unSNAT need not go through a recirculation. But since doing this now means that the +trk bit gets unset, the DNAT rules for load-balancing a new packet in the next table won't get hit. This commit removes the optimization for unSNAT packets so that there is always a recirculation. Signed-off-by: Gurucharan Shetty <[email protected]>
aneeshkp · Apr 13, 2018 · e5fd03e · e5fd03e
1 parent 3d2848b
commit e5fd03e
Show file tree

Hide file tree

Showing 7 changed files with 17 additions and 56 deletions.
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
@@ -507,9 +507,6 @@ struct ovnact_encode_params {
     /* 'true' if the flow is for a switch. */
     bool is_switch;
 
-    /* 'true' if the flow is for a gateway router. */
-    bool is_gateway_router;
-
     /* A struct to figure out the group_id for group actions. */
     struct ovn_extend_table *group_table;
 

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
@@ -132,15 +132,6 @@ is_switch(const struct sbrec_datapath_binding *ldp)
 
 }
 
-static bool
-is_gateway_router(const struct sbrec_datapath_binding *ldp,
-                  const struct hmap *local_datapaths)
-{
-    struct local_datapath *ld =
-        get_local_datapath(local_datapaths, ldp->tunnel_key);
-    return ld ? ld->has_local_l3gateway : false;
-}
-
 /* Adds the logical flows from the Logical_Flow table to flow tables. */
 static void
 add_logical_flows(struct controller_ctx *ctx,
@@ -307,7 +298,6 @@ consider_logical_flow(struct controller_ctx *ctx,
         .lookup_port = lookup_port_cb,
         .aux = &aux,
         .is_switch = is_switch(ldp),
-        .is_gateway_router = is_gateway_router(ldp, local_datapaths),
         .group_table = group_table,
         .meter_table = meter_table,
 

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
@@ -833,17 +833,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
     ct = ofpacts->header;
     if (cn->ip) {
         ct->flags |= NX_CT_F_COMMIT;
-    } else if (snat && ep->is_gateway_router) {
-        /* For performance reasons, we try to prevent additional
-         * recirculations.  ct_snat which is used in a gateway router
-         * does not need a recirculation.  ct_snat(IP) does need a
-         * recirculation.  ct_snat in a distributed router needs
-         * recirculation regardless of whether an IP address is
-         * specified.
-         * XXX Should we consider a method to let the actions specify
-         * whether an action needs recirculation if there are more use
-         * cases?. */
-        ct->recirc_table = NX_CT_RECIRC_NONE;
     }
     ofpact_finish(ofpacts, &ct->ofpact);
     ofpbuf_push_uninit(ofpacts, ct_offset);

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
@@ -1428,22 +1428,22 @@ icmp4 {
           If the Gateway router has been configured to force SNAT any
           previously DNATted packets to <var>B</var>, a priority-110 flow
           matches <code>ip &amp;&amp; ip4.dst == <var>B</var></code> with
-          an action <code>ct_snat; next;</code>.
+          an action <code>ct_snat; </code>.
         </p>
 
         <p>
           If the Gateway router has been configured to force SNAT any
           previously load-balanced packets to <var>B</var>, a priority-100 flow
           matches <code>ip &amp;&amp; ip4.dst == <var>B</var></code> with
-          an action <code>ct_snat; next;</code>.
+          an action <code>ct_snat; </code>.
         </p>
 
         <p>
           For each NAT configuration in the OVN Northbound database, that asks
           to change the source IP address of a packet from <var>A</var> to
           <var>B</var>, a priority-90 flow matches <code>ip &amp;&amp;
           ip4.dst == <var>B</var></code> with an action
-          <code>ct_snat; next;</code>.
+          <code>ct_snat; </code>.
         </p>
 
         <p>

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
@@ -5233,7 +5233,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                     ds_put_format(&match, "ip && ip4.dst == %s",
                                   nat->external_ip);
                     ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 90,
-                                  ds_cstr(&match), "ct_snat; next;");
+                                  ds_cstr(&match), "ct_snat;");
                 } else {
                     /* Distributed router. */
 
@@ -5465,7 +5465,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
             ds_clear(&match);
             ds_put_format(&match, "ip && ip4.dst == %s", dnat_force_snat_ip);
             ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 110,
-                          ds_cstr(&match), "ct_snat; next;");
+                          ds_cstr(&match), "ct_snat;");
 
             /* Higher priority rules to force SNAT with the IP addresses
              * configured in the Gateway router.  This only takes effect
@@ -5484,7 +5484,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
             ds_clear(&match);
             ds_put_format(&match, "ip && ip4.dst == %s", lb_force_snat_ip);
             ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 100,
-                          ds_cstr(&match), "ct_snat; next;");
+                          ds_cstr(&match), "ct_snat;");
 
             /* Load balanced traffic will have flags.force_snat_for_lb set.
              * Force SNAT it. */
@@ -5498,19 +5498,14 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
 
         if (!od->l3dgw_port) {
             /* For gateway router, re-circulate every packet through
-            * the DNAT zone.  This helps with two things.
+            * the DNAT zone.  This helps with the following.
             *
-            * 1. Any packet that needs to be unDNATed in the reverse
+            * Any packet that needs to be unDNATed in the reverse
             * direction gets unDNATed. Ideally this could be done in
             * the egress pipeline. But since the gateway router
             * does not have any feature that depends on the source
             * ip address being external IP address for IP routing,
-            * we can do it here, saving a future re-circulation.
-            *
-            * 2. Any packet that was sent through SNAT zone in the
-            * previous table automatically gets re-circulated to get
-            * back the new destination IP address that is needed for
-            * routing in the openflow pipeline. */
+            * we can do it here, saving a future re-circulation. */
             ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50,
                           "ip", "flags.loopback = 1; ct_dnat;");
         } else {

diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
@@ -1171,25 +1171,10 @@
           <p>
             <code>ct_snat</code> sends the packet through the SNAT zone to
             unSNAT any packet that was SNATed in the opposite direction.  The
-            behavior on gateway routers differs from the behavior on a
-            distributed router:
+            packet is automatically sent to the next tables as if followed by
+            the <code>next;</code> action.   The next tables will see the
+            changes in the packet caused by the connection tracker.
           </p>
-          <ul>
-            <li>
-              On a gateway router, if the packet needs to be sent to the next
-              tables, then it should be followed by a <code>next;</code>
-              action.  The next tables will not see the changes in the packet
-              caused by the connection tracker.
-            </li>
-            <li>
-              On a distributed router, if the connection tracker finds a
-              connection that was SNATed in the opposite direction, then the
-              destination IP address of the packet is UNSNATed.  The packet is
-              automatically sent to the next tables as if followed by the
-              <code>next;</code> action.  The next tables will see the changes
-              in the packet caused by the connection tracker.
-            </li>
-          </ul>
           <p>
             <code>ct_snat(<var>IP</var>)</code> sends the packet through the
             SNAT zone to change the source IP address of the packet to

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
@@ -855,6 +855,11 @@ ovn-nbctl set logical_router R2 load_balancer=$uuid
 # Config OVN load-balancer with another VIP (this time with ports).
 ovn-nbctl set load_balancer $uuid vips:'"30.0.0.2:8000"'='"192.168.1.2:80,192.168.2.2:80"'
 
+# Add SNAT rule to make sure that Load-balancing still works with a SNAT rule.
+ovn-nbctl -- --id=@nat create nat type="snat" logical_ip=192.168.2.2 \
+    external_ip=30.0.0.2 -- add logical_router R2 nat @nat
+
+
 # Wait for ovn-controller to catch up.
 ovn-nbctl --wait=hv sync
 OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-groups br-int | \