Skip to content

Commit

Permalink
northd: revert ct.inv drop flows
Browse files Browse the repository at this point in the history 
Partially revert the following commit since it introduces a regression
when we want to directly connect to a backend ip from a client outside
the cluster for gw-router-port scenario. For this kind of traffic we do
not commit to CT the 'original' incoming packet but we send the reply one
to CT in undnat and snat stages in the router egress pipeline. Since we
do not have any entry in CT table for the original traffic the reply one
is marked as invalid.
Even if the issue is not directly introduced by the commit below, it is
not easy to fix it without committing all IP traffic to connection
tracking or adding a flow per load-balancer backend.

commit e3bc68c
Author: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Date:   Mon Mar 20 19:30:13 2023 +0100

    northd: drop ct.inv packets in post snat and lb_aff_learn stages

    Drop ip packets with ct status set to invalid in post snat and
    lb_aff_learn router stages.
    Skip ICMPv{4,6} error messages packet in ct.inv rules in order to
    avoid to introduce too complicated code.

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
  • Loading branch information
@LorenzoBianconi @dceara
LorenzoBianconi authored and dceara committed Apr 11, 2023
1 parent ce6ef8f commit 0c71712
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 59 deletions.
25 changes: 0 additions & 25 deletions northd/northd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13901,30 +13901,6 @@ build_lrouter_out_is_dnat_local(struct hmap *lflows, struct ovn_datapath *od,
&nat->header_);
}

static void
build_lrouter_drop_ct_inv_flow(struct ovn_datapath *od, struct hmap *lflows)
{
ovs_assert(od->nbr);
if (!use_ct_inv_match) {
return;
}
/* Advance ICMP Destination Unreachable - Fragmentation Needed
* packets and drop other packets which are ct tracked and invalid. */
ovn_lflow_add(lflows, od, S_ROUTER_IN_LB_AFF_LEARN, 250,
"((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
" (ip6 && icmp6.type == 2 && icmp6.code == 0))",
"next;");
ovn_lflow_add(lflows, od, S_ROUTER_IN_LB_AFF_LEARN, 200,
"ip && ct.trk && ct.inv", debug_drop_action());

ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 150,
"((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
" (ip6 && icmp6.type == 2 && icmp6.code == 0))",
"next;");
ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 100,
"ip && ct.trk && ct.inv", debug_drop_action());
}

static void
build_lrouter_out_snat_flow(struct hmap *lflows, struct ovn_datapath *od,
const struct nbrec_nat *nat, struct ds *match,
Expand Down Expand Up @@ -14725,7 +14701,6 @@ build_lswitch_and_lrouter_iterate_by_lr(struct ovn_datapath *od,
lsi->lr_ports, &lsi->match,
&lsi->actions, lsi->meter_groups,
lsi->features);
build_lrouter_drop_ct_inv_flow(od, lsi->lflows);
build_lrouter_lb_affinity_default_flows(od, lsi->lflows);
}

Expand Down
22 changes: 0 additions & 22 deletions northd/ovn-northd.8.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3602,17 +3602,6 @@ icmp6 {
</p>

<ul>
<li>
For ICMPv4/ICMPv6 packet too big traffic, a priority-250 flow with
action <code>next;</code>.
</li>

<li>
If <code>use_ct_inv_match</code> is set, a priority-200 flow
matches <code>ip &amp;&amp; ct.trk &amp;&amp; ct.inv</code> with
action <code>drop;</code>.
</li>

<li>
For all the configured load balancing rules for a logical router where
a positive affinity timeout <var>T</var> is specified in <code>options
Expand Down Expand Up @@ -4959,17 +4948,6 @@ nd_ns {
<p>
Packets reaching this table are processed according to the flows below:
<ul>
<li>
For ICMPv4/ICMPv6 packet too big traffic, a priority-150 flow with
action <code>next;</code>.
</li>

<li>
If <code>use_ct_inv_match</code> is set, a priority-100 flow
matches <code>ip &amp;&amp; ct.trk &amp;&amp; ct.inv</code> with
action <code>drop;</code>.
</li>

<li>
A priority-0 logical flow that matches all packets not already
handled (match <code>1</code>) and action <code>next;</code>.
Expand Down
4 changes: 0 additions & 4 deletions tests/ovn-northd.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8329,8 +8329,6 @@ AT_CHECK([grep "lr_in_lb_aff_check" R1flows | sort], [0], [dnl
])
AT_CHECK([grep "lr_in_lb_aff_learn" R1flows | sort], [0], [dnl
table=8 (lr_in_lb_aff_learn ), priority=0 , match=(1), action=(next;)
table=8 (lr_in_lb_aff_learn ), priority=200 , match=(ip && ct.trk && ct.inv), action=(drop;)
table=8 (lr_in_lb_aff_learn ), priority=250 , match=(((ip4 && icmp4.type == 3 && icmp4.code == 4) || (ip6 && icmp6.type == 2 && icmp6.code == 0))), action=(next;)
])

ovn-nbctl --wait=sb set load_balancer lb0 options:affinity_timeout=60
Expand Down Expand Up @@ -8379,8 +8377,6 @@ AT_CHECK([grep "lr_in_lb_aff_learn" R1flows | sort], [0], [dnl
table=8 (lr_in_lb_aff_learn ), priority=0 , match=(1), action=(next;)
table=8 (lr_in_lb_aff_learn ), priority=100 , match=(reg9[[6]] == 0 && ct.new && ip4 && reg0 == 172.16.0.10 && reg9[[16..31]] == 80 && ip4.dst == 10.0.0.2 && tcp.dst == 80), action=(commit_lb_aff(vip = "172.16.0.10:80", backend = "10.0.0.2:80", proto = tcp, timeout = 60); /* drop */)
table=8 (lr_in_lb_aff_learn ), priority=100 , match=(reg9[[6]] == 0 && ct.new && ip4 && reg0 == 172.16.0.10 && reg9[[16..31]] == 80 && ip4.dst == 20.0.0.2 && tcp.dst == 80), action=(commit_lb_aff(vip = "172.16.0.10:80", backend = "20.0.0.2:80", proto = tcp, timeout = 60); /* drop */)
table=8 (lr_in_lb_aff_learn ), priority=200 , match=(ip && ct.trk && ct.inv), action=(drop;)
table=8 (lr_in_lb_aff_learn ), priority=250 , match=(((ip4 && icmp4.type == 3 && icmp4.code == 4) || (ip6 && icmp6.type == 2 && icmp6.code == 0))), action=(next;)
])

AS_BOX([Test LR flows - skip_snat=true])
Expand Down
16 changes: 8 additions & 8 deletions tests/system-ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6131,10 +6131,10 @@ tcp,orig=(src=172.16.0.1,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),reply=(sr
# Ensure datapaths show conntrack states as expected
# Like with conntrack entries, we shouldn't try to predict
# port binding tunnel keys. So omit them from expected labels.
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl-inv+trk).*ct(.*label=0x401020400000000/.*)' -c], [0], [dnl
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x401020400000000/.*)' -c], [0], [dnl
1
])
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl-inv+trk).*ct_label(0x401020400000000)' -c], [0], [dnl
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_label(0x401020400000000)' -c], [0], [dnl
1
])

Expand All @@ -6149,10 +6149,10 @@ ovn-nbctl set Logical_Switch_Port r2-ext \
ovn-nbctl --wait=hv sync

NS_CHECK_EXEC([bob1], [nc -z 10.0.0.2 80], [0])
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl-inv+trk).*ct(.*label=0x1001020400000000/.*)' -c], [0], [dnl
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x1001020400000000/.*)' -c], [0], [dnl
1
])
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl-inv+trk).*ct_label(0x1001020400000000)' -c], [0], [dnl
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_label(0x1001020400000000)' -c], [0], [dnl
1
])

Expand Down Expand Up @@ -6316,11 +6316,11 @@ NS_CHECK_EXEC([bob1], [nc -6 -z fd01::2 80], [0])
# Ensure datapaths show conntrack states as expected
# Like with conntrack entries, we shouldn't try to predict
# port binding tunnel keys. So omit them from expected labels.
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl-inv+trk).*ct(.*label=0x401020400000000/.*)' -c], [0], [dnl
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x401020400000000/.*)' -c], [0], [dnl
1
])

AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl-inv+trk).*ct_label(0x401020400000000)' -c], [0], [dnl
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_label(0x401020400000000)' -c], [0], [dnl
1
])

Expand All @@ -6343,10 +6343,10 @@ ovn-nbctl set Logical_Switch_Port r2-ext \

NS_CHECK_EXEC([bob1], [nc -6 -z fd01::2 80], [0])

AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl-inv+trk).*ct(.*label=0x1001020400000000/.*)' -c], [0], [dnl
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(+new-est-rpl+trk).*ct(.*label=0x1001020400000000/.*)' -c], [0], [dnl
1
])
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl-inv+trk).*ct_label(0x1001020400000000)' -c], [0], [dnl
AT_CHECK([ovs-appctl dpctl/dump-flows | grep 'ct_state(-new+est+rpl+trk).*ct_label(0x1001020400000000)' -c], [0], [dnl
1
])

Expand Down

0 comments on commit 0c71712

Please sign in to comment.