[ovs-dev] [PATCH] ovn: Recirculate packets after a unSNAT.
Gurucharan Shetty
guru at ovn.org
Mon Mar 26 21:18:28 UTC 2018
commit f6fabcc6245 (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 <guru at ovn.org>
---
include/ovn/actions.h | 3 ---
ovn/controller/lflow.c | 10 ----------
ovn/lib/actions.c | 11 -----------
ovn/northd/ovn-northd.8.xml | 6 +++---
ovn/northd/ovn-northd.c | 17 ++++++-----------
ovn/ovn-sb.xml | 21 +++------------------
tests/system-ovn.at | 5 +++++
7 files changed, 17 insertions(+), 56 deletions(-)
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index b1988d6..7076f78 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -499,9 +499,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
index df125b1..fbd0fef 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -131,15 +131,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,
@@ -303,7 +294,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
index fc5ace1..6ed200c 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -832,17 +832,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
index 444be72..d6d270b 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -1428,14 +1428,14 @@ 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 && 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 && ip4.dst == <var>B</var></code> with
- an action <code>ct_snat; next;</code>.
+ an action <code>ct_snat; </code>.
</p>
<p>
@@ -1443,7 +1443,7 @@ icmp4 {
to change the source IP address of a packet from <var>A</var> to
<var>B</var>, a priority-90 flow matches <code>ip &&
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
index 3963810..8f9e803 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -5190,7 +5190,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. */
@@ -5422,7 +5422,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
@@ -5441,7 +5441,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. */
@@ -5455,19 +5455,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
index 6a8b818..7bcd3b9 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1151,25 +1151,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
index 638c0b6..b7e2d77 100644
--- 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 | \
--
2.7.4
More information about the dev
mailing list