On 6/18/22 10:32, Lorenzo Bianconi wrote:
Hi Lorenzo, I have a few comments below
On 6/10/22 13:49, Lorenzo Bianconi wrote:
Match the drop flow for stateless dnat_and_snat flow in S_ROUTER_IN_GW_REDIRECT
stage just if allowed_ext_ips or exempted_ext_ips conditions do not
match since it breaks the hairping scenario with stateless nat.
s/hairping/hairpin
ops, I will fix it, thx
I do like the idea of calling a ping that is expected to hairpin a
"hairping" though :)
Fix the northd documentation.
Fixes: c0224a14f ("northd: fix stateless nat with allowed_ext_ips")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2094980
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
northd/northd.c | 43 ++++++++++++++++++++++++++++-------------
northd/ovn-northd.8.xml | 20 ++++++++++++++-----
tests/system-ovn.at | 15 ++++++++++++++
3 files changed, 60 insertions(+), 18 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index 0207f6ce1..5264dbd71 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12035,6 +12035,7 @@ build_gateway_redirect_flows_for_lrouter(
}
for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
const struct ovsdb_idl_row *stage_hint = NULL;
+ bool add_def_flow = true;
if (od->l3dgw_ports[i]->nbrp) {
stage_hint = &od->l3dgw_ports[i]->nbrp->header_;
@@ -12053,22 +12054,38 @@ build_gateway_redirect_flows_for_lrouter(
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_GW_REDIRECT, 50,
ds_cstr(match), ds_cstr(actions),
stage_hint);
- }
+ for (int j = 0; j < od->n_nat_entries; j++) {
+ const struct ovn_nat *nat = &od->nat_entries[j];
- for (int i = 0; i < od->n_nat_entries; i++) {
- const struct ovn_nat *nat = &od->nat_entries[i];
+ if (!lrouter_nat_is_stateless(nat->nb) ||
+ strcmp(nat->nb->type, "dnat_and_snat")) {
+ continue;
+ }
- if (!lrouter_nat_is_stateless(nat->nb) ||
- strcmp(nat->nb->type, "dnat_and_snat")) {
- continue;
- }
+ if (nat->nb->allowed_ext_ips || nat->nb->exempted_ext_ips) {
+ struct nbrec_address_set *as = nat->nb->allowed_ext_ips
+ ? nat->nb->allowed_ext_ips : nat->nb->exempted_ext_ips;
+ struct ds match_ext = DS_EMPTY_INITIALIZER;
- ds_clear(match);
- ds_put_format(match, "ip && ip%s.dst == %s",
- nat_entry_is_v6(nat) ? "6" : "4",
- nat->nb->external_ip);
- ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 100,
- ds_cstr(match), "drop;");
+ ds_put_format(&match_ext, "%s && ip%s.src == $%s",
+ ds_cstr(match), nat_entry_is_v6(nat) ? "6" : "4",
+ as->name);
+ ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_GW_REDIRECT,
+ 75, ds_cstr(&match_ext),
+ ds_cstr(actions), stage_hint);
+
+ if (add_def_flow) {
+ ds_clear(&match_ext);
+ ds_put_format(&match_ext, "ip && ip%s.dst == %s",
+ nat_entry_is_v6(nat) ? "6" : "4",
+ nat->nb->external_ip);
+ ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 70,
+ ds_cstr(&match_ext), "drop;");
+ add_def_flow = false;
+ }
+ ds_destroy(&match_ext);
+ }
+ }
I think the correct thing to do is to remove the original loop from the
code, and allow all traffic to be output to the gateway port. The allowed
and exempted IP evaluation should be done by the gateway router after the
traffic is hairpinned. Even if you wanted to try to add an optimization here
(drop traffic early), the exempted_ips logic is not correct. The new code
allows traffic from exempted IPs to be output to the redirect port. The
problem is that once that traffic is hairpinned, the traffic will be dropped
since the IP is in the exempted_ext_ips set. If anything, the code should be
allowing all IPs that do not match the extempted_ext_ips set. However, that
would translate to a lot of flows. It's easier to just allow all traffic to
be redirected and filter the traffic after redirecting.
(as we discussed offline) the point here is for N/S traffic the does not match
the the allowed_ext_ips set and so it is recirculated on the gw node. I added
the drop flow to take care of it.
Regarding the exempted_ext_ips I agree, we can do someting different here,
something like:
struct ds match_ext = DS_EMPTY_INITIALIZER;
struct nbrec_address_set *as = nat->nb->allowed_ext_ips
? nat->nb->allowed_ext_ips : nat->nb->exempted_ext_ips;
ds_put_format(&match_ext, "%s && ip%s.src == $%s",
ds_cstr(match), nat_entry_is_v6(nat) ? "6" : "4",
as->name);
if (nat->nb->allowed_ext_ips) {
....
} else if (nat->nb->exempted_ext_ips) {
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_GW_REDIRECT,
75, ds_cstr(&match_ext), "drop;",
stage_hint);
}
what do you think?
Yes I think this is a good way to resolve the issue.
}
/* Packets are allowed by default. */
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 1f7022490..ad1a66cd4 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -3018,8 +3018,7 @@ icmp6 {
<code>ip && ip6.dst == <var>B</var></code>
with an action <code>ct_snat; </code>. If the NAT rule is of type
dnat_and_snat and has <code>stateless=true</code> in the
- options, then the action would be <code>ip4/6.dst=
- (<var>B</var>)</code>.
+ options, then the action would be <code>next;</code>.
</p>
<p>
@@ -3059,7 +3058,7 @@ icmp6 {
action <code>ct_snat_in_czone;</code> to unSNAT in the common
zone. If the NAT rule is of type dnat_and_snat and has
<code>stateless=true</code> in the options, then the action
- would be <code>ip4/6.dst=(<var>B</var>)</code>.
+ would be <code>next;</code>.
</p>
<p>
@@ -4235,6 +4234,18 @@ icmp6 {
gateway chassis.
</li>
+ <li>
+ For each <code>dnat_and_snat</code> NAT rule with
+ <code>stateless=true</code> and <code>allowed_ext_ips</code>
+ or <code>exempted_ext_ips</code> configured, a similar priority-75
+ flow is programmed with an additional match
+ <code>ip4.src == <var>allowed_ext_ips</var></code> or
+ <code>ip4.src == <var>exempted_ext_ips</var></code>.
+ Moreover a priority-70 drop flow is added with match
+ <code>ip4.dst == <var>B</var></code> where <var>B</var> is the NAT
+ rule external IP. A specular flow is added for IPv6 traffic.
Did you mean to use the word specular here? It's so uncommon that my email
client flags it as a misspelling. I looked it up, and it is a word, and it
*kind of* fits here too (it means behaving like a mirror). But I have a
feeling you meant to use "similar", right?
yes, I mean "similar" :)
Regards,
Lorenzo
+ </li>
+
<li>
A priority-0 logical flow with match <code>1</code> has actions
<code>next;</code>.
@@ -4415,8 +4426,7 @@ nd_ns {
is the logical router gateway port, with an action
<code>ct_dnat_in_czone;</code>. If the NAT rule is of type
dnat_and_snat and has <code>stateless=true</code> in the
- options, then the action would be <code>ip4/6.src=
- (<var>B</var>)</code>.
+ options, then the action would be <code>next;</code>.
</p>
<p>
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 4bf22593a..df2da3408 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6741,6 +6741,21 @@ NS_CHECK_EXEC([vm1], [ping -q -c 3 -i 0.3 -w 2
172.18.2.10 | FORMAT_PING], \
[0], [dnl
3 packets transmitted, 3 received, 0% packet loss, time 0ms
])
+
+dnat_and_snat_uuid=$(fetch_column nb:NAT _uuid external_ip=172.18.2.10)
+ovn-nbctl set NAT $dnat_and_snat_uuid options:stateless=true
+
+# A ping from vm1 should hairpin in lr1 and successfully DNAT to vm2
+NS_CHECK_EXEC([vm1], [ping -q -c 3 -i 0.3 -w 2 172.18.2.10 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+# A ping from vm2 should hairpin in lr1 and successfully DNAT to vm2
+NS_CHECK_EXEC([vm2], [ping -q -c 3 -i 0.3 -w 2 172.18.2.10 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
kill $(pidof ovn-controller)
as ovn-sb
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
