Re: [ovs-dev] [PATCH ovn v2] northd: prevents sending packet to conntrack for router ports
On 3/7/23 09:27, Ales Musil wrote: > On Mon, Mar 6, 2023 at 12:07 PM Xavier Simonart wrote: > >> As commented in northd.c, we should not use ct() for router ports. >> When there are no stateful_acl, this patch prevents sending packet to >> conntrack >> for router ports. >> The patch does this by issuing ct_clear in ls_out_pre_lb stage so that >> hints >> are not set in ls_out_acl_hint and ls_out_acl stages. >> >> Note that ct_clear is not added for ingress for router ports as already >> done >> for patch ports (no change by this patch on this aspect). >> >> Also, this patch does not change the behavior for ACLs such as >> allow-related: >> packets are still sent to conntrack, even for router ports. While this does >> not work if router ports are distributed, allow-related ACLs work today on >> router ports when those ports are handled on the same chassis for ingress >> and >> egress traffic. This patch does not change that behavior. >> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431 >> >> Signed-off-by: Xavier Simonart >> >> --- >> v2: - handled Dumitru's comments >> - handled Ales' comments >> - added change to xml documentation >> - do not do ct_clear for ingress as already done >> --- [...] >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil > Thanks, Xavier and Ales! I applied this to the main branch! Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd: prevents sending packet to conntrack for router ports
On Mon, Mar 6, 2023 at 12:07 PM Xavier Simonart wrote:
> As commented in northd.c, we should not use ct() for router ports.
> When there are no stateful_acl, this patch prevents sending packet to
> conntrack
> for router ports.
> The patch does this by issuing ct_clear in ls_out_pre_lb stage so that
> hints
> are not set in ls_out_acl_hint and ls_out_acl stages.
>
> Note that ct_clear is not added for ingress for router ports as already
> done
> for patch ports (no change by this patch on this aspect).
>
> Also, this patch does not change the behavior for ACLs such as
> allow-related:
> packets are still sent to conntrack, even for router ports. While this does
> not work if router ports are distributed, allow-related ACLs work today on
> router ports when those ports are handled on the same chassis for ingress
> and
> egress traffic. This patch does not change that behavior.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431
>
> Signed-off-by: Xavier Simonart
>
> ---
> v2: - handled Dumitru's comments
> - handled Ales' comments
> - added change to xml documentation
> - do not do ct_clear for ingress as already done
> ---
> northd/northd.c | 24 +++---
> northd/ovn-northd.8.xml | 11 +++
> tests/ovn-northd.at | 11 +--
> tests/system-ovn.at | 166
> 4 files changed, 197 insertions(+), 15 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 7ad4cdfad..b356faf64 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5779,20 +5779,24 @@ skip_port_from_conntrack(struct ovn_datapath *od,
> struct ovn_port *op,
> * know about the connection, as the icmp request went through the
> logical
> * router on hostA, not hostB. This would only work with distributed
> * conntrack state across all chassis. */
> -struct ds match_in = DS_EMPTY_INITIALIZER;
> -struct ds match_out = DS_EMPTY_INITIALIZER;
>
> -ds_put_format(_in, "ip && inport == %s", op->json_key);
> -ds_put_format(_out, "ip && outport == %s", op->json_key);
> +const char *ingress_action = "next;";
> +const char *egress_action = od->has_stateful_acl
> +? "next;"
> +: "ct_clear; next;";
> +
> +char *ingress_match = xasprintf("ip && inport == %s", op->json_key);
> +char *egress_match = xasprintf("ip && outport == %s", op->json_key);
> +
> ovn_lflow_add_with_lport_and_hint(lflows, od, in_stage, priority,
> - ds_cstr(_in), "next;",
> op->key,
> - >nbsp->header_);
> + ingress_match, ingress_action,
> + op->key, >nbsp->header_);
> ovn_lflow_add_with_lport_and_hint(lflows, od, out_stage, priority,
> - ds_cstr(_out), "next;",
> op->key,
> - >nbsp->header_);
> + egress_match, egress_action,
> + op->key, >nbsp->header_);
>
> -ds_destroy(_in);
> -ds_destroy(_out);
> +free(ingress_match);
> +free(egress_match);
> }
>
> static void
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 2eab2c4ae..efadfe808 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2056,6 +2056,17 @@ output;
>db="OVN_Northbound"/> table.
>
>
> +
> + This table also has a priority-110 flow with the match
> + outport == I for all logical switch
> + datapaths to move traffic to the next table, and, if there are no
> + stateful_acl, clear the ct_state. Where I
> + is the peer of a logical router port. This flow is added to
> + skip the connection tracking of packets which will be entering
> + logical router datapath from logical switch datapath for routing.
> +
> +
> +
> Egress Table 2: Pre-stateful
>
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 3fa02d2b3..d0f6893e9 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -4111,6 +4111,7 @@ check ovn-nbctl lsp-set-options sw0-lr0
> router-port=lr0-sw0
> check ovn-nbctl --wait=sb sync
>
> check_stateful_flows() {
> +action=$1
> ovn-sbctl dump-flows sw0 > sw0flows
> AT_CAPTURE_FILE([sw0flows])
>
> @@ -4144,12 +4145,12 @@ check_stateful_flows() {
>table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> ])
>
> -AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl
> +AT_CHECK_UNQUOTED([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl
>table=1 (ls_out_pre_lb ), priority=0, match=(1), action=(next;)
>table=1 (ls_out_pre_lb ), priority=100 , match=(ip),
> action=(reg0[[2]] = 1; next;)
>table=1
[ovs-dev] [PATCH ovn v2] northd: prevents sending packet to conntrack for router ports
As commented in northd.c, we should not use ct() for router ports.
When there are no stateful_acl, this patch prevents sending packet to conntrack
for router ports.
The patch does this by issuing ct_clear in ls_out_pre_lb stage so that hints
are not set in ls_out_acl_hint and ls_out_acl stages.
Note that ct_clear is not added for ingress for router ports as already done
for patch ports (no change by this patch on this aspect).
Also, this patch does not change the behavior for ACLs such as allow-related:
packets are still sent to conntrack, even for router ports. While this does
not work if router ports are distributed, allow-related ACLs work today on
router ports when those ports are handled on the same chassis for ingress and
egress traffic. This patch does not change that behavior.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431
Signed-off-by: Xavier Simonart
---
v2: - handled Dumitru's comments
- handled Ales' comments
- added change to xml documentation
- do not do ct_clear for ingress as already done
---
northd/northd.c | 24 +++---
northd/ovn-northd.8.xml | 11 +++
tests/ovn-northd.at | 11 +--
tests/system-ovn.at | 166
4 files changed, 197 insertions(+), 15 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index 7ad4cdfad..b356faf64 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5779,20 +5779,24 @@ skip_port_from_conntrack(struct ovn_datapath *od,
struct ovn_port *op,
* know about the connection, as the icmp request went through the logical
* router on hostA, not hostB. This would only work with distributed
* conntrack state across all chassis. */
-struct ds match_in = DS_EMPTY_INITIALIZER;
-struct ds match_out = DS_EMPTY_INITIALIZER;
-ds_put_format(_in, "ip && inport == %s", op->json_key);
-ds_put_format(_out, "ip && outport == %s", op->json_key);
+const char *ingress_action = "next;";
+const char *egress_action = od->has_stateful_acl
+? "next;"
+: "ct_clear; next;";
+
+char *ingress_match = xasprintf("ip && inport == %s", op->json_key);
+char *egress_match = xasprintf("ip && outport == %s", op->json_key);
+
ovn_lflow_add_with_lport_and_hint(lflows, od, in_stage, priority,
- ds_cstr(_in), "next;", op->key,
- >nbsp->header_);
+ ingress_match, ingress_action,
+ op->key, >nbsp->header_);
ovn_lflow_add_with_lport_and_hint(lflows, od, out_stage, priority,
- ds_cstr(_out), "next;", op->key,
- >nbsp->header_);
+ egress_match, egress_action,
+ op->key, >nbsp->header_);
-ds_destroy(_in);
-ds_destroy(_out);
+free(ingress_match);
+free(egress_match);
}
static void
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 2eab2c4ae..efadfe808 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -2056,6 +2056,17 @@ output;
db="OVN_Northbound"/> table.
+
+ This table also has a priority-110 flow with the match
+ outport == I for all logical switch
+ datapaths to move traffic to the next table, and, if there are no
+ stateful_acl, clear the ct_state. Where I
+ is the peer of a logical router port. This flow is added to
+ skip the connection tracking of packets which will be entering
+ logical router datapath from logical switch datapath for routing.
+
+
+
Egress Table 2: Pre-stateful
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 3fa02d2b3..d0f6893e9 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -4111,6 +4111,7 @@ check ovn-nbctl lsp-set-options sw0-lr0
router-port=lr0-sw0
check ovn-nbctl --wait=sb sync
check_stateful_flows() {
+action=$1
ovn-sbctl dump-flows sw0 > sw0flows
AT_CAPTURE_FILE([sw0flows])
@@ -4144,12 +4145,12 @@ check_stateful_flows() {
table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
reg3; }; next;)
])
-AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl
+AT_CHECK_UNQUOTED([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl
table=1 (ls_out_pre_lb ), priority=0, match=(1), action=(next;)
table=1 (ls_out_pre_lb ), priority=100 , match=(ip), action=(reg0[[2]]
= 1; next;)
table=1 (ls_out_pre_lb ), priority=110 , match=(eth.mcast),
action=(next;)
- table=1 (ls_out_pre_lb ), priority=110 , match=(eth.src ==
$svc_monitor_mac), action=(next;)
- table=1 (ls_out_pre_lb ), priority=110 , match=(ip && outport ==
"sw0-lr0"), action=(next;)
+
