Re: [ovs-dev] [PATCH ovn 2/3] Honor allow priority when stateless present
On Thu, May 20, 2021 at 5:58 PM Han Zhou wrote:
>
>
>
> On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka wrote:
> >
> > For each allow-stateless ACL, a rule was added earlier in the pipeline
> > that circumvented setting REGBIT_CONNTRACK_DEFRAG regardless of
> > whether other, e.g. allow ACLs with higher priority were present.
> >
> > Now, when allow-stateless ACLs are present on the switch with other
> > allow-related ACLs, for each allow, insert an early pipeline rule that
> > would set DEFRAG bit for the corresponding match and priority.
> >
> > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")
> >
>
> Looks good except that flow changes need to be documented.
> Acked-by: Han Zhou
The wording from the v2 version of the first patch in the series will cover it.
>
> > Signed-off-by: Ihar Hrachyshka
> > ---
> > northd/ovn-northd.c | 3 ++
> > northd/ovn_northd.dl | 2 +-
> > tests/ovn-northd.at | 72
> > 3 files changed, 76 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 26b723165..a410be1de 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -5076,6 +5076,9 @@ build_stateful_override_filters(struct ovn_datapath
> > *od,
> > apply_to_each_acl_of_action(
> > od, port_groups, lflows, "allow-related",
> > build_stateful_override_filter);
> > +apply_to_each_acl_of_action(
> > +od, port_groups, lflows, "allow",
> > +build_stateful_override_filter);
> > }
> >
> > static void
> > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> > index 7c2402be4..fc703f62e 100644
> > --- a/northd/ovn_northd.dl
> > +++ b/northd/ovn_northd.dl
> > @@ -1846,7 +1846,7 @@ for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl =
> > &acl, .has_fair_meter = _)) {
> >
> > for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter =
> > _)) {
> > if (sw.has_stateless_acl) {
> > -if (acl.action == "allow-related") {
> > +if ((sw.has_stateful_acl and acl.action == "allow") or acl.action
> > == "allow-related") {
> > if (acl.direction == "from-lport") {
> > Flow(.logical_datapath = ls._uuid,
> > .stage= s_SWITCH_IN_PRE_ACL(),
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index da75434b6..6c38e1a97 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -2755,6 +2755,78 @@ output("lsp2");
> > AT_CLEANUP
> > ])
> >
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ovn -- ACL priority: allow-stateless vs allow])
> > +ovn_start
> > +
> > +ovn-nbctl ls-add ls
> > +ovn-nbctl lsp-add ls lsp1
> > +ovn-nbctl lsp-set-addresses lsp1 00:00:00:00:00:01
> > +ovn-nbctl lsp-add ls lsp2
> > +ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02
> > +
> > +for direction in from to; do
> > +ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow
> > +done
> > +ovn-nbctl --wait=sb sync
> > +
> > +flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02'
> > +flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66'
> > +flow_tcp='tcp && tcp.dst == 80'
> > +flow_udp='udp && udp.dst == 80'
> > +lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1)
> > +
> > +# TCP packets should not go to conntrack.
> > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}"
> > +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl
> > +#
> > tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> > +output("lsp2");
> > +])
> > +
> > +# Add allow-stateless with lower priority.
> > +for direction in from to; do
> > +ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless
> > +done
> > +ovn-nbctl --wait=sb sync
> > +
> > +# TCP packets should still not go to conntrack (allow ACL in effect).
> > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
> > [0], [dnl
> > +#
> > tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> > +output("lsp2");
> > +])
> > +
> > +# Add allow-related to the switch.
> > +for direction in from to; do
> > +ovn-nbctl acl-add ls ${direction}-lport 3 icmp allow-related
> > +done
> > +ovn-nbctl --wait=sb sync
> > +
> > +# TCP packets should now go to conntrack (allow ACL in effect, flipped its
> > meaning to apply conntrack)
> > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
> > [0], [dnl
> > +#
> > tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> > +ct_next(ct_state=new|trk) {
> > +ct_ne
Re: [ovs-dev] [PATCH ovn 2/3] Honor allow priority when stateless present
On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka wrote:
>
> For each allow-stateless ACL, a rule was added earlier in the pipeline
> that circumvented setting REGBIT_CONNTRACK_DEFRAG regardless of
> whether other, e.g. allow ACLs with higher priority were present.
>
> Now, when allow-stateless ACLs are present on the switch with other
> allow-related ACLs, for each allow, insert an early pipeline rule that
> would set DEFRAG bit for the corresponding match and priority.
>
> Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")
>
Looks good except that flow changes need to be documented.
Acked-by: Han Zhou
> Signed-off-by: Ihar Hrachyshka
> ---
> northd/ovn-northd.c | 3 ++
> northd/ovn_northd.dl | 2 +-
> tests/ovn-northd.at | 72
> 3 files changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 26b723165..a410be1de 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5076,6 +5076,9 @@ build_stateful_override_filters(struct ovn_datapath
*od,
> apply_to_each_acl_of_action(
> od, port_groups, lflows, "allow-related",
> build_stateful_override_filter);
> +apply_to_each_acl_of_action(
> +od, port_groups, lflows, "allow",
> +build_stateful_override_filter);
> }
>
> static void
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 7c2402be4..fc703f62e 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1846,7 +1846,7 @@ for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl =
&acl, .has_fair_meter = _)) {
>
> for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter
= _)) {
> if (sw.has_stateless_acl) {
> -if (acl.action == "allow-related") {
> +if ((sw.has_stateful_acl and acl.action == "allow") or
acl.action == "allow-related") {
> if (acl.direction == "from-lport") {
> Flow(.logical_datapath = ls._uuid,
> .stage= s_SWITCH_IN_PRE_ACL(),
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index da75434b6..6c38e1a97 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2755,6 +2755,78 @@ output("lsp2");
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn -- ACL priority: allow-stateless vs allow])
> +ovn_start
> +
> +ovn-nbctl ls-add ls
> +ovn-nbctl lsp-add ls lsp1
> +ovn-nbctl lsp-set-addresses lsp1 00:00:00:00:00:01
> +ovn-nbctl lsp-add ls lsp2
> +ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02
> +
> +for direction in from to; do
> +ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow
> +done
> +ovn-nbctl --wait=sb sync
> +
> +flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02'
> +flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66'
> +flow_tcp='tcp && tcp.dst == 80'
> +flow_udp='udp && udp.dst == 80'
> +lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1)
> +
> +# TCP packets should not go to conntrack.
> +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}"
> +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> +output("lsp2");
> +])
> +
> +# Add allow-stateless with lower priority.
> +for direction in from to; do
> +ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless
> +done
> +ovn-nbctl --wait=sb sync
> +
> +# TCP packets should still not go to conntrack (allow ACL in effect).
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
[0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> +output("lsp2");
> +])
> +
> +# Add allow-related to the switch.
> +for direction in from to; do
> +ovn-nbctl acl-add ls ${direction}-lport 3 icmp allow-related
> +done
> +ovn-nbctl --wait=sb sync
> +
> +# TCP packets should now go to conntrack (allow ACL in effect, flipped
its meaning to apply conntrack)
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
[0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
> +ct_next(ct_state=new|trk) {
> +ct_next(ct_state=new|trk) {
> +output("lsp2");
> +};
> +};
> +])
> +
> +# Add allow-stateless with higher priority.
> +for direction in from to; do
> +ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless
> +done
> +ovn-nbctl --wait=sb sync
> +
> +
> +# TCP packets should no longer go to conntrack (the new allow-stateless
ACL in effect).
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --c
[ovs-dev] [PATCH ovn 2/3] Honor allow priority when stateless present
For each allow-stateless ACL, a rule was added earlier in the pipeline
that circumvented setting REGBIT_CONNTRACK_DEFRAG regardless of
whether other, e.g. allow ACLs with higher priority were present.
Now, when allow-stateless ACLs are present on the switch with other
allow-related ACLs, for each allow, insert an early pipeline rule that
would set DEFRAG bit for the corresponding match and priority.
Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")
Signed-off-by: Ihar Hrachyshka
---
northd/ovn-northd.c | 3 ++
northd/ovn_northd.dl | 2 +-
tests/ovn-northd.at | 72
3 files changed, 76 insertions(+), 1 deletion(-)
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 26b723165..a410be1de 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5076,6 +5076,9 @@ build_stateful_override_filters(struct ovn_datapath *od,
apply_to_each_acl_of_action(
od, port_groups, lflows, "allow-related",
build_stateful_override_filter);
+apply_to_each_acl_of_action(
+od, port_groups, lflows, "allow",
+build_stateful_override_filter);
}
static void
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 7c2402be4..fc703f62e 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -1846,7 +1846,7 @@ for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl,
.has_fair_meter = _)) {
for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = _))
{
if (sw.has_stateless_acl) {
-if (acl.action == "allow-related") {
+if ((sw.has_stateful_acl and acl.action == "allow") or acl.action ==
"allow-related") {
if (acl.direction == "from-lport") {
Flow(.logical_datapath = ls._uuid,
.stage= s_SWITCH_IN_PRE_ACL(),
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index da75434b6..6c38e1a97 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2755,6 +2755,78 @@ output("lsp2");
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- ACL priority: allow-stateless vs allow])
+ovn_start
+
+ovn-nbctl ls-add ls
+ovn-nbctl lsp-add ls lsp1
+ovn-nbctl lsp-set-addresses lsp1 00:00:00:00:00:01
+ovn-nbctl lsp-add ls lsp2
+ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02
+
+for direction in from to; do
+ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow
+done
+ovn-nbctl --wait=sb sync
+
+flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02'
+flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66'
+flow_tcp='tcp && tcp.dst == 80'
+flow_udp='udp && udp.dst == 80'
+lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1)
+
+# TCP packets should not go to conntrack.
+flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}"
+AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl
+#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
+output("lsp2");
+])
+
+# Add allow-stateless with lower priority.
+for direction in from to; do
+ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless
+done
+ovn-nbctl --wait=sb sync
+
+# TCP packets should still not go to conntrack (allow ACL in effect).
+AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0],
[dnl
+#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
+output("lsp2");
+])
+
+# Add allow-related to the switch.
+for direction in from to; do
+ovn-nbctl acl-add ls ${direction}-lport 3 icmp allow-related
+done
+ovn-nbctl --wait=sb sync
+
+# TCP packets should now go to conntrack (allow ACL in effect, flipped its
meaning to apply conntrack)
+AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0],
[dnl
+#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
+ct_next(ct_state=new|trk) {
+ct_next(ct_state=new|trk) {
+output("lsp2");
+};
+};
+])
+
+# Add allow-stateless with higher priority.
+for direction in from to; do
+ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless
+done
+ovn-nbctl --wait=sb sync
+
+
+# TCP packets should no longer go to conntrack (the new allow-stateless ACL in
effect).
+AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0],
[dnl
+#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0
+output("lsp2");
+])
+
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn -- ACL allow-stateless omit conntrack -
