On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrac...@redhat.com> 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-related ACLs with higher priority were
> present.
>
> Now, when allow-stateless ACLs are present on the switch, for each
> allow-related, 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")
Thanks for the Fix. I think we need to update northd document for the flow
changes. Please also see some inline comments below.
>
> Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
> ---
> northd/lswitch.dl | 20 +++++++++
> northd/ovn-northd.c | 91 +++++++++++++++++++++++++++++++--------
> northd/ovn_northd.dl | 24 ++++++++++-
> tests/ovn-northd.at | 100 ++++++++++++++++++++++++++++++++++++++++---
> 4 files changed, 210 insertions(+), 25 deletions(-)
>
> diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> index a1aaebb6d..b73cfd047 100644
> --- a/northd/lswitch.dl
> +++ b/northd/lswitch.dl
> @@ -129,6 +129,23 @@ LogicalSwitchHasStatefulACL(ls, false) :-
> nb::Logical_Switch(._uuid = ls),
> not LogicalSwitchStatefulACL(ls, _).
>
> +relation LogicalSwitchStatelessACL(ls: uuid, acl: uuid)
> +
> +LogicalSwitchStatelessACL(ls, acl) :-
> + LogicalSwitchACL(ls, acl),
> + nb::ACL(._uuid = acl, .action = "allow-stateless").
> +
> +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> +// is an output relation:
> +output relation LogicalSwitchHasStatelessACL(ls: uuid,
has_stateless_acl: bool)
> +
> +LogicalSwitchHasStatelessACL(ls, true) :-
> + LogicalSwitchStatelessACL(ls, _).
> +
> +LogicalSwitchHasStatelessACL(ls, false) :-
> + nb::Logical_Switch(._uuid = ls),
> + not LogicalSwitchStatelessACL(ls, _).
> +
> // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> // is an output relation:
> output relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool)
> @@ -208,6 +225,7 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> relation &Switch(
> ls: nb::Logical_Switch,
> has_stateful_acl: bool,
> + has_stateless_acl: bool,
> has_acls: bool,
> has_lb_vip: bool,
> has_dns_records: bool,
> @@ -235,6 +253,7 @@ function ipv6_parse_prefix(s: string):
Option<in6_addr> {
>
> &Switch(.ls = ls,
> .has_stateful_acl = has_stateful_acl,
> + .has_stateless_acl = has_stateless_acl,
> .has_acls = has_acls,
> .has_lb_vip = has_lb_vip,
> .has_dns_records = has_dns_records,
> @@ -247,6 +266,7 @@ function ipv6_parse_prefix(s: string):
Option<in6_addr> {
> .is_vlan_transparent = is_vlan_transparent) :-
> nb::Logical_Switch[ls],
> LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
> + LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl),
> LogicalSwitchHasACLs(ls._uuid, has_acls),
> LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
> LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records),
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 7464b7fba..26b723165 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4983,6 +4983,38 @@ skip_port_from_conntrack(struct ovn_datapath *od,
struct ovn_port *op,
> ds_destroy(&match_out);
> }
>
> +static bool
> +apply_to_each_acl_of_action(struct ovn_datapath *od,
> + const struct hmap *port_groups,
> + struct hmap *lflows, const char *action,
> + void (*func)(struct ovn_datapath *,
> + const struct nbrec_acl *,
> + struct hmap *))
> +{
> + bool applied = false;
> + for (size_t i = 0; i < od->nbs->n_acls; i++) {
> + const struct nbrec_acl *acl = od->nbs->acls[i];
> + if (!strcmp(acl->action, action)) {
> + func(od, acl, lflows);
> + applied = true;
> + }
> + }
> +
> + struct ovn_port_group *pg;
> + HMAP_FOR_EACH (pg, key_node, port_groups) {
> + if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) {
> + for (size_t i = 0; i < pg->nb_pg->n_acls; i++) {
> + const struct nbrec_acl *acl = pg->nb_pg->acls[i];
> + if (!strcmp(acl->action, action)) {
> + func(od, acl, lflows);
> + applied = true;
> + }
> + }
> + }
> + }
> + return applied;
> +}
> +
> static void
> build_stateless_filter(struct ovn_datapath *od,
> const struct nbrec_acl *acl,
> @@ -5003,28 +5035,47 @@ build_stateless_filter(struct ovn_datapath *od,
> }
> }
>
> -static void
> -build_stateless_filters(struct ovn_datapath *od, struct hmap
*port_groups,
> +static bool
> +build_stateless_filters(struct ovn_datapath *od,
> + const struct hmap *port_groups,
> struct hmap *lflows)
> {
> - for (size_t i = 0; i < od->nbs->n_acls; i++) {
> - const struct nbrec_acl *acl = od->nbs->acls[i];
> - if (!strcmp(acl->action, "allow-stateless")) {
> - build_stateless_filter(od, acl, lflows);
> - }
> - }
> + return apply_to_each_acl_of_action(
> + od, port_groups, lflows, "allow-stateless",
build_stateless_filter);
> +}
>
> - struct ovn_port_group *pg;
> - HMAP_FOR_EACH (pg, key_node, port_groups) {
> - if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) {
> - for (size_t i = 0; i < pg->nb_pg->n_acls; i++) {
> - const struct nbrec_acl *acl = pg->nb_pg->acls[i];
> - if (!strcmp(acl->action, "allow-stateless")) {
> - build_stateless_filter(od, acl, lflows);
> - }
> - }
> - }
> +static void
> +build_stateful_override_filter(struct ovn_datapath *od,
> + const struct nbrec_acl *acl,
> + struct hmap *lflows)
> +{
> + struct ds match = DS_EMPTY_INITIALIZER;
> + ds_put_format(&match, "ip && %s", acl->match);
> +
> + if (!strcmp(acl->direction, "from-lport")) {
> + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL,
> + acl->priority + OVN_ACL_PRI_OFFSET,
> + ds_cstr(&match),
> + REGBIT_CONNTRACK_DEFRAG" = 1; next;",
> + &acl->header_);
> + } else {
> + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL,
> + acl->priority + OVN_ACL_PRI_OFFSET,
> + ds_cstr(&match),
> + REGBIT_CONNTRACK_DEFRAG" = 1; next;",
> + &acl->header_);
> }
> + ds_destroy(&match);
> +}
> +
> +static void
> +build_stateful_override_filters(struct ovn_datapath *od,
> + const struct hmap *port_groups,
> + struct hmap *lflows)
> +{
> + apply_to_each_acl_of_action(
> + od, port_groups, lflows, "allow-related",
> + build_stateful_override_filter);
> }
>
> static void
> @@ -5057,7 +5108,9 @@ build_pre_acls(struct ovn_datapath *od, struct hmap
*port_groups,
> 110, lflows);
> }
>
> - build_stateless_filters(od, port_groups, lflows);
> + if (build_stateless_filters(od, port_groups, lflows)) {
> + build_stateful_override_filters(od, port_groups, lflows);
> + }
>
> /* Ingress and Egress Pre-ACL Table (Priority 110).
> *
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 5e1788351..7c2402be4 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1822,7 +1822,7 @@ for (&Switch(.ls =ls)) {
> .external_ids = map_empty())
> }
>
> -for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter
= fair_meter)) {
> +for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter
= _)) {
Why do we need ".has_fair_meter = _" here if it is not used?
> if (sw.has_stateful_acl) {
> if (acl.action == "allow-stateless") {
> if (acl.direction == "from-lport") {
> @@ -1844,6 +1844,28 @@ for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl =
&acl, .has_fair_meter = fair_
> }
> }
>
> +for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter
= _)) {
Maybe it's better to combine this loop with the above one since the loop
condition is the same.
> + if (sw.has_stateless_acl) {
> + if (acl.action == "allow-related") {
> + if (acl.direction == "from-lport") {
> + Flow(.logical_datapath = ls._uuid,
> + .stage = s_SWITCH_IN_PRE_ACL(),
> + .priority = acl.priority +
oVN_ACL_PRI_OFFSET(),
> + .__match = "ip && ${acl.__match}",
Why need to add "ip &&" in front of the match? If the match has fields
indicating it is IP, ovn-controller should add it automatically. Is this an
optimization when the match doesn't require the protocol to be IP so when a
non-ip packet comes it doesn't need to be directed to CT? Or is it for some
other reason?
> + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} =
1; next;",
> + .external_ids = stage_hint(acl._uuid))
> + } else {
> + Flow(.logical_datapath = ls._uuid,
> + .stage = s_SWITCH_OUT_PRE_ACL(),
> + .priority = acl.priority +
oVN_ACL_PRI_OFFSET(),
> + .__match = "ip && ${acl.__match}",
> + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} =
1; next;",
> + .external_ids = stage_hint(acl._uuid))
> + }
> + }
> + }
> +}
> +
> /* If there are any stateful ACL rules in this datapath, we must
> * send all IP packets through the conntrack action, which handles
> * defragmentation, in order to match L4 headers. */
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 6c2745ed1..da75434b6 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2664,6 +2664,97 @@ sed 's/reg8\[[0..15\]] == [[0-9]]*/reg8\[[0..15\]]
== <cleared>/' | sort], [0],
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related])
> +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-related
> +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 go to conntrack.
> +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}"
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
[0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,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 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 go to conntrack.
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
[0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,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 another 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
> +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,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");
> +])
> +
> +# Remove the higher priority allow-stateless.
> +for direction in from to; do
> + ovn-nbctl acl-del ls ${direction}-lport 5 tcp
> +done
> +ovn-nbctl --wait=sb sync
> +
> +# TCP packets should again go to conntrack.
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"],
[0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,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");
> + };
> +};
> +])
> +
> +# Remove the allow-related ACL.
> +for direction in from to; do
> + ovn-nbctl acl-del ls ${direction}-lport 3 tcp
> +done
> +ovn-nbctl --wait=sb sync
> +
> +# TCP packets should no longer go to conntrack
> +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl
> +#
tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,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 - Logical_Switch])
> ovn_start
> @@ -2712,7 +2803,7 @@ ct_next(ct_state=new|trk) {
>
> # Allow stateless for TCP.
> for direction in from to; do
> - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless
> + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless
> done
> ovn-nbctl --wait=sb sync
>
> @@ -2768,7 +2859,7 @@ ct_lb {
>
> # Allow stateless for TCP.
> for direction in from to; do
> - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless
> + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless
> done
> ovn-nbctl --wait=sb sync
>
> @@ -2817,7 +2908,6 @@ done
> ovn-nbctl --wait=sb sync
>
> lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1)
> -echo $lsp1_inport
>
> 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'
> @@ -2848,7 +2938,7 @@ ct_next(ct_state=new|trk) {
>
> # Allow stateless for TCP.
> for direction in from to; do
> - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless
> + ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless
> done
> ovn-nbctl --wait=sb sync
>
> @@ -2904,7 +2994,7 @@ ct_lb {
>
> # Allow stateless for TCP.
> for direction in from to; do
> - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless
> + ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless
> done
> ovn-nbctl --wait=sb sync
>
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
