| Message ID | 20210517214708.3961445-3-ihrachys@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [ovs-dev,1/3] Honor allow-related priority when stateless present | expand |
On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > While we *should not* circumvent conntrack when a stateful ACL of higher > priority is present on the switch, we should do so only when > allow-stateless and allow-stateful directions are the same, otherwise we > should still skip conntrack for allow-stateless ACLs. > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") Is this patch a "fix" or an "optimization"? > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > --- > northd/lswitch.dl | 88 ++++++++++++++++++++++++++++--------------- > northd/ovn-northd.c | 89 ++++++++++++++++++++++++++++++++++---------- > northd/ovn_northd.dl | 32 ++++++++-------- > tests/ovn-northd.at | 72 +++++++++++++++++++++++++++++++++++ > 4 files changed, 213 insertions(+), 68 deletions(-) > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > index b73cfd047..8a4c9154c 100644 > --- a/northd/lswitch.dl > +++ b/northd/lswitch.dl > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :- > LogicalSwitchACL(ls, acl), > nb::ACL(._uuid = acl, .action = "allow-stateless"). > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid) > + > +LogicalSwitchStatelessFromACL(ls, acl) :- > + LogicalSwitchStatelessACL(ls, acl), > + nb::ACL(._uuid = acl, .direction = "from-lport"). > + > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this > +// is an output relation: > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid, has_stateless_from_acl: bool) > + > +LogicalSwitchHasStatelessFromACL(ls, true) :- > + LogicalSwitchStatelessFromACL(ls, _). > + > +LogicalSwitchHasStatelessFromACL(ls, false) :- > + nb::Logical_Switch(._uuid = ls), > + not LogicalSwitchStatelessFromACL(ls, _). > + > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid) > + > +LogicalSwitchStatelessToACL(ls, acl) :- > + LogicalSwitchStatelessACL(ls, acl), > + nb::ACL(._uuid = acl, .direction = "to-lport"). > + > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > // is an output relation: > -output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) > +output relation LogicalSwitchHasStatelessToACL(ls: uuid, has_stateless_to_acl: bool) > > -LogicalSwitchHasStatelessACL(ls, true) :- > - LogicalSwitchStatelessACL(ls, _). > +LogicalSwitchHasStatelessToACL(ls, true) :- > + LogicalSwitchStatelessToACL(ls, _). > > -LogicalSwitchHasStatelessACL(ls, false) :- > +LogicalSwitchHasStatelessToACL(ls, false) :- > nb::Logical_Switch(._uuid = ls), > - not LogicalSwitchStatelessACL(ls, _). > + not LogicalSwitchStatelessToACL(ls, _). > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > // is an output relation: > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :- > /* Switch relation collects all attributes of a logical switch */ > > 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, > - has_unknown_ports: bool, > - localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > - subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > - ipv6_prefix: Option<in6_addr>, > - mcast_cfg: Ref<McastSwitchCfg>, > - is_vlan_transparent: bool, > + ls: nb::Logical_Switch, > + has_stateful_acl: bool, > + has_stateless_from_acl: bool, > + has_stateless_to_acl: bool, > + has_acls: bool, > + has_lb_vip: bool, > + has_dns_records: bool, > + has_unknown_ports: bool, > + localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > + subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > + ipv6_prefix: Option<in6_addr>, > + mcast_cfg: Ref<McastSwitchCfg>, > + is_vlan_transparent: bool, > > /* Does this switch have at least one port with type != "router"? */ > has_non_router_port: bool > @@ -251,22 +275,24 @@ 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, > - .has_unknown_ports = has_unknown_ports, > - .localnet_ports = localnet_ports, > - .subnet = subnet, > - .ipv6_prefix = ipv6_prefix, > - .mcast_cfg = mcast_cfg, > - .has_non_router_port = has_non_router_port, > - .is_vlan_transparent = is_vlan_transparent) :- > +&Switch(.ls = ls, > + .has_stateful_acl = has_stateful_acl, > + .has_stateless_from_acl = has_stateless_from_acl, > + .has_stateless_to_acl = has_stateless_to_acl, > + .has_acls = has_acls, > + .has_lb_vip = has_lb_vip, > + .has_dns_records = has_dns_records, > + .has_unknown_ports = has_unknown_ports, > + .localnet_ports = localnet_ports, > + .subnet = subnet, > + .ipv6_prefix = ipv6_prefix, > + .mcast_cfg = mcast_cfg, > + .has_non_router_port = has_non_router_port, > + .is_vlan_transparent = is_vlan_transparent) :- > nb::Logical_Switch[ls], > LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), > - LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), > + LogicalSwitchHasStatelessFromACL(ls._uuid, has_stateless_from_acl), > + LogicalSwitchHasStatelessToACL(ls._uuid, has_stateless_to_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 a410be1de..1e027eab2 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -627,6 +627,8 @@ struct ovn_datapath { > uint32_t port_key_hint; > > bool has_stateful_acl; > + bool has_stateless_from; > + bool has_stateless_to; > bool has_lb_vip; > bool has_unknown; > bool has_acls; > @@ -4759,19 +4761,46 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs) > hmap_destroy(nb_pgs); > } > > +static bool > +ls_get_acl_flags_for_acl(struct ovn_datapath *od, > + const struct nbrec_acl *acl) > +{ > + if (!strcmp(acl->action, "allow-related")) { > + od->has_stateful_acl = true; > + if (od->has_stateless_to && od->has_stateless_from) { > + return true; > + } > + } > + if (!strcmp(acl->action, "allow-stateless")) { > + if (!strcmp(acl->direction, "from-lport")) { > + od->has_stateless_from = true; > + if (od->has_stateful_acl && od->has_stateless_to) { > + return true; > + } > + } else { > + od->has_stateless_to = true; > + if (od->has_stateful_acl && od->has_stateless_from) { > + return true; > + } > + } > + } > + return false; > +} > + > static void > ls_get_acl_flags(struct ovn_datapath *od) > { > od->has_acls = false; > od->has_stateful_acl = false; > + od->has_stateless_from = false; > + od->has_stateless_to = false; > > if (od->nbs->n_acls) { > od->has_acls = true; > > for (size_t i = 0; i < od->nbs->n_acls; i++) { > struct nbrec_acl *acl = od->nbs->acls[i]; > - if (!strcmp(acl->action, "allow-related")) { > - od->has_stateful_acl = true; > + if (ls_get_acl_flags_for_acl(od, acl)) { > return; > } > } > @@ -4784,8 +4813,7 @@ ls_get_acl_flags(struct ovn_datapath *od) > > for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) { > struct nbrec_acl *acl = ls_pg->nb_pg->acls[i]; > - if (!strcmp(acl->action, "allow-related")) { > - od->has_stateful_acl = true; > + if (ls_get_acl_flags_for_acl(od, acl)) { > return; > } > } > @@ -4984,17 +5012,20 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, > } > > 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 *)) > +apply_to_each_acl_of_action_and_direction( > + struct ovn_datapath *od, > + const struct hmap *port_groups, > + struct hmap *lflows, > + const char *action, const char *direction, > + 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)) { > + if (!strcmp(acl->action, action) && > + (!direction || !strcmp(acl->direction, direction))) { > func(od, acl, lflows); > applied = true; > } > @@ -5005,7 +5036,8 @@ apply_to_each_acl_of_action(struct ovn_datapath *od, > 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)) { > + if (!strcmp(acl->action, action) && > + (!direction || !strcmp(acl->direction, direction))) { > func(od, acl, lflows); > applied = true; > } > @@ -5040,8 +5072,9 @@ build_stateless_filters(struct ovn_datapath *od, > const struct hmap *port_groups, > struct hmap *lflows) > { > - return apply_to_each_acl_of_action( > - od, port_groups, lflows, "allow-stateless", build_stateless_filter); > + return apply_to_each_acl_of_action_and_direction( > + od, port_groups, lflows, "allow-stateless", NULL, > + build_stateless_filter); > } > > static void > @@ -5068,17 +5101,33 @@ build_stateful_override_filter(struct ovn_datapath *od, > ds_destroy(&match); > } > > +static void > +build_stateful_override_filters_for_direction(struct ovn_datapath *od, > + const struct hmap *port_groups, > + struct hmap *lflows, > + const char *direction) > +{ > + apply_to_each_acl_of_action_and_direction( > + od, port_groups, lflows, "allow-related", direction, > + build_stateful_override_filter); > + apply_to_each_acl_of_action_and_direction( > + od, port_groups, lflows, "allow", direction, > + build_stateful_override_filter); > +} > + > 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); > - apply_to_each_acl_of_action( > - od, port_groups, lflows, "allow", > - build_stateful_override_filter); > + if (od->has_stateless_from) { > + build_stateful_override_filters_for_direction( > + od, port_groups, lflows, "from-lport"); > + } > + if (od->has_stateless_to) { > + build_stateful_override_filters_for_direction( > + od, port_groups, lflows, "to-lport"); > + } > } > > static void > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index fc703f62e..8cbc959f0 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -1845,23 +1845,21 @@ 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 ((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(), > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > - .__match = "ip && ${acl.__match}", > - .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 ((sw.has_stateful_acl and acl.action == "allow") or acl.action == "allow-related") { > + if (sw.has_stateless_from_acl and 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}", > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > + .external_ids = stage_hint(acl._uuid)) > + } else if (sw.has_stateless_to_acl) { Should this be: else if (sw.has_stateless_to_acl and acl.direction == "to-lport") ? > + 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)) > } > } > } > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 6c38e1a97..1c55310b2 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2664,6 +2664,78 @@ 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 of different direction]) > +ovn_start > + > +# Create two switches to validate direction. We can't use the same switch for > +# both ports, otherwise both in and out pipelines are triggered. > +ovn-nbctl lr-add r0 > +for i in $(seq 1 2); do > + check ovn-nbctl --wait=sb ls-add sw$i > + > + check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i f0:00:00:00:00:0$i 192.168.$i.1/24 > + check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0 > + check ovn-nbctl --wait=sb lsp-set-type sw$i-r0 router > + check ovn-nbctl --wait=sb lsp-set-options sw$i-r0 router-port=r0-sw$i > + check ovn-nbctl --wait=sb lsp-set-addresses sw$i-r0 router > + > + check ovn-nbctl --wait=sb lsp-add sw$i lsp$i > + check ovn-nbctl --wait=sb lsp-set-addresses lsp$i "fe:00:00:00:00:0$i 192.168.$i.100/24" > +done > + > +ovn-nbctl acl-add sw1 from-lport 3 tcp allow-stateless > +ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related > +ovn-nbctl --wait=sb sync > + > +flow_eth='eth.src == fe:00:00:00:00:01 && eth.dst == f0:00:00:00:00:01' > +flow_ip='ip.ttl==64 && ip4.src == 192.168.1.100 && ip4.dst == 192.168.2.100' > +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 because allow-related direction is different. This test case makes me wonder if the idea of this patch (as an optimization) is not correct. What if the packet is a return packet of a request packet allowed by "ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related"? In that case the packet does need to go to CT, otherwise the CT state will never be "established". I also had the impression that the priorities for different directions are independent, but it seems we will have to consider them together for the stateless + stateful scenario. Any thoughts on this? Thanks, Han > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > +AT_CHECK_UNQUOTED([ovn-trace --minimal sw1 "${flow}"], [0], [dnl > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +ip.ttl--; > +eth.src = f0:00:00:00:00:02; > +eth.dst = fe:00:00:00:00:02; > +output("lsp2"); > +]) > + > +# Add allow-related with the same direction. > +ovn-nbctl acl-add sw1 from-lport 4 tcp allow-related > +ovn-nbctl --wait=sb sync > + > +# TCP packets should now go to conntrack. > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +ct_next(ct_state=new|trk) { > + ip.ttl--; > + eth.src = f0:00:00:00:00:02; > + eth.dst = fe:00:00:00:00:02; > + output("lsp2"); > +}; > +]) > + > +# Reduce priority for allow-related rule of the same direction. > +ovn-nbctl acl-del sw1 from-lport 4 tcp > +ovn-nbctl acl-add sw1 from-lport 2 tcp allow-related > +ovn-nbctl --wait=sb sync > + > +# TCP packets should no longer go to conntrack > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +ip.ttl--; > +eth.src = f0:00:00:00:00:02; > +eth.dst = fe:00:00:00:00:02; > +output("lsp2"); > +]) > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) > ovn_start > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, May 20, 2021 at 3:22 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > > > While we *should not* circumvent conntrack when a stateful ACL of higher > > priority is present on the switch, we should do so only when > > allow-stateless and allow-stateful directions are the same, otherwise we > > should still skip conntrack for allow-stateless ACLs. > > > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") > > Is this patch a "fix" or an "optimization"? > > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > > --- > > northd/lswitch.dl | 88 ++++++++++++++++++++++++++++--------------- > > northd/ovn-northd.c | 89 ++++++++++++++++++++++++++++++++++---------- > > northd/ovn_northd.dl | 32 ++++++++-------- > > tests/ovn-northd.at | 72 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 213 insertions(+), 68 deletions(-) > > > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > > index b73cfd047..8a4c9154c 100644 > > --- a/northd/lswitch.dl > > +++ b/northd/lswitch.dl > > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :- > > LogicalSwitchACL(ls, acl), > > nb::ACL(._uuid = acl, .action = "allow-stateless"). > > > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid) > > + > > +LogicalSwitchStatelessFromACL(ls, acl) :- > > + LogicalSwitchStatelessACL(ls, acl), > > + nb::ACL(._uuid = acl, .direction = "from-lport"). > > + > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > +// is an output relation: > > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid, has_stateless_from_acl: bool) > > + > > +LogicalSwitchHasStatelessFromACL(ls, true) :- > > + LogicalSwitchStatelessFromACL(ls, _). > > + > > +LogicalSwitchHasStatelessFromACL(ls, false) :- > > + nb::Logical_Switch(._uuid = ls), > > + not LogicalSwitchStatelessFromACL(ls, _). > > + > > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid) > > + > > +LogicalSwitchStatelessToACL(ls, acl) :- > > + LogicalSwitchStatelessACL(ls, acl), > > + nb::ACL(._uuid = acl, .direction = "to-lport"). > > + > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > // is an output relation: > > -output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) > > +output relation LogicalSwitchHasStatelessToACL(ls: uuid, has_stateless_to_acl: bool) > > > > -LogicalSwitchHasStatelessACL(ls, true) :- > > - LogicalSwitchStatelessACL(ls, _). > > +LogicalSwitchHasStatelessToACL(ls, true) :- > > + LogicalSwitchStatelessToACL(ls, _). > > > > -LogicalSwitchHasStatelessACL(ls, false) :- > > +LogicalSwitchHasStatelessToACL(ls, false) :- > > nb::Logical_Switch(._uuid = ls), > > - not LogicalSwitchStatelessACL(ls, _). > > + not LogicalSwitchStatelessToACL(ls, _). > > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > // is an output relation: > > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :- > > /* Switch relation collects all attributes of a logical switch */ > > > > 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, > > - has_unknown_ports: bool, > > - localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > - subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > - ipv6_prefix: Option<in6_addr>, > > - mcast_cfg: Ref<McastSwitchCfg>, > > - is_vlan_transparent: bool, > > + ls: nb::Logical_Switch, > > + has_stateful_acl: bool, > > + has_stateless_from_acl: bool, > > + has_stateless_to_acl: bool, > > + has_acls: bool, > > + has_lb_vip: bool, > > + has_dns_records: bool, > > + has_unknown_ports: bool, > > + localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > + subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > + ipv6_prefix: Option<in6_addr>, > > + mcast_cfg: Ref<McastSwitchCfg>, > > + is_vlan_transparent: bool, > > > > /* Does this switch have at least one port with type != "router"? */ > > has_non_router_port: bool > > @@ -251,22 +275,24 @@ 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, > > - .has_unknown_ports = has_unknown_ports, > > - .localnet_ports = localnet_ports, > > - .subnet = subnet, > > - .ipv6_prefix = ipv6_prefix, > > - .mcast_cfg = mcast_cfg, > > - .has_non_router_port = has_non_router_port, > > - .is_vlan_transparent = is_vlan_transparent) :- > > +&Switch(.ls = ls, > > + .has_stateful_acl = has_stateful_acl, > > + .has_stateless_from_acl = has_stateless_from_acl, > > + .has_stateless_to_acl = has_stateless_to_acl, > > + .has_acls = has_acls, > > + .has_lb_vip = has_lb_vip, > > + .has_dns_records = has_dns_records, > > + .has_unknown_ports = has_unknown_ports, > > + .localnet_ports = localnet_ports, > > + .subnet = subnet, > > + .ipv6_prefix = ipv6_prefix, > > + .mcast_cfg = mcast_cfg, > > + .has_non_router_port = has_non_router_port, > > + .is_vlan_transparent = is_vlan_transparent) :- > > nb::Logical_Switch[ls], > > LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), > > - LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), > > + LogicalSwitchHasStatelessFromACL(ls._uuid, has_stateless_from_acl), > > + LogicalSwitchHasStatelessToACL(ls._uuid, has_stateless_to_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 a410be1de..1e027eab2 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -627,6 +627,8 @@ struct ovn_datapath { > > uint32_t port_key_hint; > > > > bool has_stateful_acl; > > + bool has_stateless_from; > > + bool has_stateless_to; > > bool has_lb_vip; > > bool has_unknown; > > bool has_acls; > > @@ -4759,19 +4761,46 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs) > > hmap_destroy(nb_pgs); > > } > > > > +static bool > > +ls_get_acl_flags_for_acl(struct ovn_datapath *od, > > + const struct nbrec_acl *acl) > > +{ > > + if (!strcmp(acl->action, "allow-related")) { > > + od->has_stateful_acl = true; > > + if (od->has_stateless_to && od->has_stateless_from) { > > + return true; > > + } > > + } > > + if (!strcmp(acl->action, "allow-stateless")) { > > + if (!strcmp(acl->direction, "from-lport")) { > > + od->has_stateless_from = true; > > + if (od->has_stateful_acl && od->has_stateless_to) { > > + return true; > > + } > > + } else { > > + od->has_stateless_to = true; > > + if (od->has_stateful_acl && od->has_stateless_from) { > > + return true; > > + } > > + } > > + } > > + return false; > > +} > > + > > static void > > ls_get_acl_flags(struct ovn_datapath *od) > > { > > od->has_acls = false; > > od->has_stateful_acl = false; > > + od->has_stateless_from = false; > > + od->has_stateless_to = false; > > > > if (od->nbs->n_acls) { > > od->has_acls = true; > > > > for (size_t i = 0; i < od->nbs->n_acls; i++) { > > struct nbrec_acl *acl = od->nbs->acls[i]; > > - if (!strcmp(acl->action, "allow-related")) { > > - od->has_stateful_acl = true; > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > return; > > } > > } > > @@ -4784,8 +4813,7 @@ ls_get_acl_flags(struct ovn_datapath *od) > > > > for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) { > > struct nbrec_acl *acl = ls_pg->nb_pg->acls[i]; > > - if (!strcmp(acl->action, "allow-related")) { > > - od->has_stateful_acl = true; > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > return; > > } > > } > > @@ -4984,17 +5012,20 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, > > } > > > > 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 *)) > > +apply_to_each_acl_of_action_and_direction( > > + struct ovn_datapath *od, > > + const struct hmap *port_groups, > > + struct hmap *lflows, > > + const char *action, const char *direction, > > + 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)) { > > + if (!strcmp(acl->action, action) && > > + (!direction || !strcmp(acl->direction, direction))) { > > func(od, acl, lflows); > > applied = true; > > } > > @@ -5005,7 +5036,8 @@ apply_to_each_acl_of_action(struct ovn_datapath *od, > > 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)) { > > + if (!strcmp(acl->action, action) && > > + (!direction || !strcmp(acl->direction, direction))) { > > func(od, acl, lflows); > > applied = true; > > } > > @@ -5040,8 +5072,9 @@ build_stateless_filters(struct ovn_datapath *od, > > const struct hmap *port_groups, > > struct hmap *lflows) > > { > > - return apply_to_each_acl_of_action( > > - od, port_groups, lflows, "allow-stateless", build_stateless_filter); > > + return apply_to_each_acl_of_action_and_direction( > > + od, port_groups, lflows, "allow-stateless", NULL, > > + build_stateless_filter); > > } > > > > static void > > @@ -5068,17 +5101,33 @@ build_stateful_override_filter(struct ovn_datapath *od, > > ds_destroy(&match); > > } > > > > +static void > > +build_stateful_override_filters_for_direction(struct ovn_datapath *od, > > + const struct hmap *port_groups, > > + struct hmap *lflows, > > + const char *direction) > > +{ > > + apply_to_each_acl_of_action_and_direction( > > + od, port_groups, lflows, "allow-related", direction, > > + build_stateful_override_filter); > > + apply_to_each_acl_of_action_and_direction( > > + od, port_groups, lflows, "allow", direction, > > + build_stateful_override_filter); > > +} > > + > > 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); > > - apply_to_each_acl_of_action( > > - od, port_groups, lflows, "allow", > > - build_stateful_override_filter); > > + if (od->has_stateless_from) { > > + build_stateful_override_filters_for_direction( > > + od, port_groups, lflows, "from-lport"); > > + } > > + if (od->has_stateless_to) { > > + build_stateful_override_filters_for_direction( > > + od, port_groups, lflows, "to-lport"); > > + } > > } > > > > static void > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > > index fc703f62e..8cbc959f0 100644 > > --- a/northd/ovn_northd.dl > > +++ b/northd/ovn_northd.dl > > @@ -1845,23 +1845,21 @@ 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 ((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(), > > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > - .__match = "ip && ${acl.__match}", > > - .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 ((sw.has_stateful_acl and acl.action == "allow") or acl.action == "allow-related") { > > + if (sw.has_stateless_from_acl and 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}", > > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > > + .external_ids = stage_hint(acl._uuid)) > > + } else if (sw.has_stateless_to_acl) { > > Should this be: else if (sw.has_stateless_to_acl and acl.direction == "to-lport") ? > > > + 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)) > > } > > } > > } > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 6c38e1a97..1c55310b2 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -2664,6 +2664,78 @@ 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 of different direction]) > > +ovn_start > > + > > +# Create two switches to validate direction. We can't use the same switch for > > +# both ports, otherwise both in and out pipelines are triggered. > > +ovn-nbctl lr-add r0 > > +for i in $(seq 1 2); do > > + check ovn-nbctl --wait=sb ls-add sw$i > > + > > + check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i f0:00:00:00:00:0$i 192.168.$i.1/24 > > + check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0 > > + check ovn-nbctl --wait=sb lsp-set-type sw$i-r0 router > > + check ovn-nbctl --wait=sb lsp-set-options sw$i-r0 router-port=r0-sw$i > > + check ovn-nbctl --wait=sb lsp-set-addresses sw$i-r0 router > > + > > + check ovn-nbctl --wait=sb lsp-add sw$i lsp$i > > + check ovn-nbctl --wait=sb lsp-set-addresses lsp$i "fe:00:00:00:00:0$i 192.168.$i.100/24" > > +done > > + > > +ovn-nbctl acl-add sw1 from-lport 3 tcp allow-stateless > > +ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related > > +ovn-nbctl --wait=sb sync > > + > > +flow_eth='eth.src == fe:00:00:00:00:01 && eth.dst == f0:00:00:00:00:01' > > +flow_ip='ip.ttl==64 && ip4.src == 192.168.1.100 && ip4.dst == 192.168.2.100' > > +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 because allow-related direction is different. > > This test case makes me wonder if the idea of this patch (as an optimization) is not correct. What if the packet is a return packet of a request packet allowed by "ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related"? In that case the packet does need to go to CT, otherwise the CT state will never be "established". I also had the impression that the priorities for different directions are independent, but it seems we will have to consider them together for the stateless + stateful scenario. Any thoughts on this? > I thought more about it, and it seems the return traffic problem is there for the mixed stateful and stateless scenarios regardless of this patch. For the return traffic belonging to a flow allowed by a "allow_related" ACL to work, we will have to send all packets that are possibly related to a stateful ACL to conntrack, regardless of its priority (because priority should be examined for the initiation direction only). However, it seems really complex to define logical flows to identify if a packet is "related" to a stateful ACL (if possible at all). I'd suggest considering this as a limitation of stateless ACLs and document it clearly so that when a user uses "allow-stateless" they understand the consequences: if there is an allow-stateless rule defined on direction A, any stateful rule of direction B that could have any return packets matching this allow-stateless rule may not work. Probably the ideal situation when "allow-stateless" is particularly useful is when the stateful and stateless rules don't have any overlapping, e.g. one for TCP and the other for UDP. Think about this further, if we make it clear that allow-stateless should be used when there is no overlapping with stateful rules, the original patch (that has been merged) works perfectly because priority doesn't matter if there is no overlapping, and the code complexity and number of flows are both reduced. This way, the limitation is also easier to be documented and understood (comparing with clarifying accurately the limitation of priority + direction + "possible return traffic"). What do you think? > Thanks, > Han > > > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > > +AT_CHECK_UNQUOTED([ovn-trace --minimal sw1 "${flow}"], [0], [dnl > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +ip.ttl--; > > +eth.src = f0:00:00:00:00:02; > > +eth.dst = fe:00:00:00:00:02; > > +output("lsp2"); > > +]) > > + > > +# Add allow-related with the same direction. > > +ovn-nbctl acl-add sw1 from-lport 4 tcp allow-related > > +ovn-nbctl --wait=sb sync > > + > > +# TCP packets should now go to conntrack. > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +ct_next(ct_state=new|trk) { > > + ip.ttl--; > > + eth.src = f0:00:00:00:00:02; > > + eth.dst = fe:00:00:00:00:02; > > + output("lsp2"); > > +}; > > +]) > > + > > +# Reduce priority for allow-related rule of the same direction. > > +ovn-nbctl acl-del sw1 from-lport 4 tcp > > +ovn-nbctl acl-add sw1 from-lport 2 tcp allow-related > > +ovn-nbctl --wait=sb sync > > + > > +# TCP packets should no longer go to conntrack > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +ip.ttl--; > > +eth.src = f0:00:00:00:00:02; > > +eth.dst = fe:00:00:00:00:02; > > +output("lsp2"); > > +]) > > + > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) > > ovn_start > > -- > > 2.31.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, May 20, 2021 at 9:55 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Thu, May 20, 2021 at 3:22 PM Han Zhou <hzhou@ovn.org> wrote: > > > > > > > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > > > > > While we *should not* circumvent conntrack when a stateful ACL of higher > > > priority is present on the switch, we should do so only when > > > allow-stateless and allow-stateful directions are the same, otherwise we > > > should still skip conntrack for allow-stateless ACLs. > > > > > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") > > > > Is this patch a "fix" or an "optimization"? > > > > > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > > > --- > > > northd/lswitch.dl | 88 ++++++++++++++++++++++++++++--------------- > > > northd/ovn-northd.c | 89 ++++++++++++++++++++++++++++++++++---------- > > > northd/ovn_northd.dl | 32 ++++++++-------- > > > tests/ovn-northd.at | 72 +++++++++++++++++++++++++++++++++++ > > > 4 files changed, 213 insertions(+), 68 deletions(-) > > > > > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > > > index b73cfd047..8a4c9154c 100644 > > > --- a/northd/lswitch.dl > > > +++ b/northd/lswitch.dl > > > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :- > > > LogicalSwitchACL(ls, acl), > > > nb::ACL(._uuid = acl, .action = "allow-stateless"). > > > > > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid) > > > + > > > +LogicalSwitchStatelessFromACL(ls, acl) :- > > > + LogicalSwitchStatelessACL(ls, acl), > > > + nb::ACL(._uuid = acl, .direction = "from-lport"). > > > + > > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > > +// is an output relation: > > > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid, has_stateless_from_acl: bool) > > > + > > > +LogicalSwitchHasStatelessFromACL(ls, true) :- > > > + LogicalSwitchStatelessFromACL(ls, _). > > > + > > > +LogicalSwitchHasStatelessFromACL(ls, false) :- > > > + nb::Logical_Switch(._uuid = ls), > > > + not LogicalSwitchStatelessFromACL(ls, _). > > > + > > > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid) > > > + > > > +LogicalSwitchStatelessToACL(ls, acl) :- > > > + LogicalSwitchStatelessACL(ls, acl), > > > + nb::ACL(._uuid = acl, .direction = "to-lport"). > > > + > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > > // is an output relation: > > > -output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) > > > +output relation LogicalSwitchHasStatelessToACL(ls: uuid, has_stateless_to_acl: bool) > > > > > > -LogicalSwitchHasStatelessACL(ls, true) :- > > > - LogicalSwitchStatelessACL(ls, _). > > > +LogicalSwitchHasStatelessToACL(ls, true) :- > > > + LogicalSwitchStatelessToACL(ls, _). > > > > > > -LogicalSwitchHasStatelessACL(ls, false) :- > > > +LogicalSwitchHasStatelessToACL(ls, false) :- > > > nb::Logical_Switch(._uuid = ls), > > > - not LogicalSwitchStatelessACL(ls, _). > > > + not LogicalSwitchStatelessToACL(ls, _). > > > > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > > // is an output relation: > > > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :- > > > /* Switch relation collects all attributes of a logical switch */ > > > > > > 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, > > > - has_unknown_ports: bool, > > > - localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > > - subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > > - ipv6_prefix: Option<in6_addr>, > > > - mcast_cfg: Ref<McastSwitchCfg>, > > > - is_vlan_transparent: bool, > > > + ls: nb::Logical_Switch, > > > + has_stateful_acl: bool, > > > + has_stateless_from_acl: bool, > > > + has_stateless_to_acl: bool, > > > + has_acls: bool, > > > + has_lb_vip: bool, > > > + has_dns_records: bool, > > > + has_unknown_ports: bool, > > > + localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > > + subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > > + ipv6_prefix: Option<in6_addr>, > > > + mcast_cfg: Ref<McastSwitchCfg>, > > > + is_vlan_transparent: bool, > > > > > > /* Does this switch have at least one port with type != "router"? */ > > > has_non_router_port: bool > > > @@ -251,22 +275,24 @@ 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, > > > - .has_unknown_ports = has_unknown_ports, > > > - .localnet_ports = localnet_ports, > > > - .subnet = subnet, > > > - .ipv6_prefix = ipv6_prefix, > > > - .mcast_cfg = mcast_cfg, > > > - .has_non_router_port = has_non_router_port, > > > - .is_vlan_transparent = is_vlan_transparent) :- > > > +&Switch(.ls = ls, > > > + .has_stateful_acl = has_stateful_acl, > > > + .has_stateless_from_acl = has_stateless_from_acl, > > > + .has_stateless_to_acl = has_stateless_to_acl, > > > + .has_acls = has_acls, > > > + .has_lb_vip = has_lb_vip, > > > + .has_dns_records = has_dns_records, > > > + .has_unknown_ports = has_unknown_ports, > > > + .localnet_ports = localnet_ports, > > > + .subnet = subnet, > > > + .ipv6_prefix = ipv6_prefix, > > > + .mcast_cfg = mcast_cfg, > > > + .has_non_router_port = has_non_router_port, > > > + .is_vlan_transparent = is_vlan_transparent) :- > > > nb::Logical_Switch[ls], > > > LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), > > > - LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), > > > + LogicalSwitchHasStatelessFromACL(ls._uuid, has_stateless_from_acl), > > > + LogicalSwitchHasStatelessToACL(ls._uuid, has_stateless_to_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 a410be1de..1e027eab2 100644 > > > --- a/northd/ovn-northd.c > > > +++ b/northd/ovn-northd.c > > > @@ -627,6 +627,8 @@ struct ovn_datapath { > > > uint32_t port_key_hint; > > > > > > bool has_stateful_acl; > > > + bool has_stateless_from; > > > + bool has_stateless_to; > > > bool has_lb_vip; > > > bool has_unknown; > > > bool has_acls; > > > @@ -4759,19 +4761,46 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs) > > > hmap_destroy(nb_pgs); > > > } > > > > > > +static bool > > > +ls_get_acl_flags_for_acl(struct ovn_datapath *od, > > > + const struct nbrec_acl *acl) > > > +{ > > > + if (!strcmp(acl->action, "allow-related")) { > > > + od->has_stateful_acl = true; > > > + if (od->has_stateless_to && od->has_stateless_from) { > > > + return true; > > > + } > > > + } > > > + if (!strcmp(acl->action, "allow-stateless")) { > > > + if (!strcmp(acl->direction, "from-lport")) { > > > + od->has_stateless_from = true; > > > + if (od->has_stateful_acl && od->has_stateless_to) { > > > + return true; > > > + } > > > + } else { > > > + od->has_stateless_to = true; > > > + if (od->has_stateful_acl && od->has_stateless_from) { > > > + return true; > > > + } > > > + } > > > + } > > > + return false; > > > +} > > > + > > > static void > > > ls_get_acl_flags(struct ovn_datapath *od) > > > { > > > od->has_acls = false; > > > od->has_stateful_acl = false; > > > + od->has_stateless_from = false; > > > + od->has_stateless_to = false; > > > > > > if (od->nbs->n_acls) { > > > od->has_acls = true; > > > > > > for (size_t i = 0; i < od->nbs->n_acls; i++) { > > > struct nbrec_acl *acl = od->nbs->acls[i]; > > > - if (!strcmp(acl->action, "allow-related")) { > > > - od->has_stateful_acl = true; > > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > > return; > > > } > > > } > > > @@ -4784,8 +4813,7 @@ ls_get_acl_flags(struct ovn_datapath *od) > > > > > > for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) { > > > struct nbrec_acl *acl = ls_pg->nb_pg->acls[i]; > > > - if (!strcmp(acl->action, "allow-related")) { > > > - od->has_stateful_acl = true; > > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > > return; > > > } > > > } > > > @@ -4984,17 +5012,20 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, > > > } > > > > > > 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 *)) > > > +apply_to_each_acl_of_action_and_direction( > > > + struct ovn_datapath *od, > > > + const struct hmap *port_groups, > > > + struct hmap *lflows, > > > + const char *action, const char *direction, > > > + 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)) { > > > + if (!strcmp(acl->action, action) && > > > + (!direction || !strcmp(acl->direction, direction))) { > > > func(od, acl, lflows); > > > applied = true; > > > } > > > @@ -5005,7 +5036,8 @@ apply_to_each_acl_of_action(struct ovn_datapath *od, > > > 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)) { > > > + if (!strcmp(acl->action, action) && > > > + (!direction || !strcmp(acl->direction, direction))) { > > > func(od, acl, lflows); > > > applied = true; > > > } > > > @@ -5040,8 +5072,9 @@ build_stateless_filters(struct ovn_datapath *od, > > > const struct hmap *port_groups, > > > struct hmap *lflows) > > > { > > > - return apply_to_each_acl_of_action( > > > - od, port_groups, lflows, "allow-stateless", build_stateless_filter); > > > + return apply_to_each_acl_of_action_and_direction( > > > + od, port_groups, lflows, "allow-stateless", NULL, > > > + build_stateless_filter); > > > } > > > > > > static void > > > @@ -5068,17 +5101,33 @@ build_stateful_override_filter(struct ovn_datapath *od, > > > ds_destroy(&match); > > > } > > > > > > +static void > > > +build_stateful_override_filters_for_direction(struct ovn_datapath *od, > > > + const struct hmap *port_groups, > > > + struct hmap *lflows, > > > + const char *direction) > > > +{ > > > + apply_to_each_acl_of_action_and_direction( > > > + od, port_groups, lflows, "allow-related", direction, > > > + build_stateful_override_filter); > > > + apply_to_each_acl_of_action_and_direction( > > > + od, port_groups, lflows, "allow", direction, > > > + build_stateful_override_filter); > > > +} > > > + > > > 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); > > > - apply_to_each_acl_of_action( > > > - od, port_groups, lflows, "allow", > > > - build_stateful_override_filter); > > > + if (od->has_stateless_from) { > > > + build_stateful_override_filters_for_direction( > > > + od, port_groups, lflows, "from-lport"); > > > + } > > > + if (od->has_stateless_to) { > > > + build_stateful_override_filters_for_direction( > > > + od, port_groups, lflows, "to-lport"); > > > + } > > > } > > > > > > static void > > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > > > index fc703f62e..8cbc959f0 100644 > > > --- a/northd/ovn_northd.dl > > > +++ b/northd/ovn_northd.dl > > > @@ -1845,23 +1845,21 @@ 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 ((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(), > > > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > > - .__match = "ip && ${acl.__match}", > > > - .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 ((sw.has_stateful_acl and acl.action == "allow") or acl.action == "allow-related") { > > > + if (sw.has_stateless_from_acl and 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}", > > > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > > > + .external_ids = stage_hint(acl._uuid)) > > > + } else if (sw.has_stateless_to_acl) { > > > > Should this be: else if (sw.has_stateless_to_acl and acl.direction == "to-lport") ? > > > > > + 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)) > > > } > > > } > > > } > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > index 6c38e1a97..1c55310b2 100644 > > > --- a/tests/ovn-northd.at > > > +++ b/tests/ovn-northd.at > > > @@ -2664,6 +2664,78 @@ 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 of different direction]) > > > +ovn_start > > > + > > > +# Create two switches to validate direction. We can't use the same switch for > > > +# both ports, otherwise both in and out pipelines are triggered. > > > +ovn-nbctl lr-add r0 > > > +for i in $(seq 1 2); do > > > + check ovn-nbctl --wait=sb ls-add sw$i > > > + > > > + check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i f0:00:00:00:00:0$i 192.168.$i.1/24 > > > + check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0 > > > + check ovn-nbctl --wait=sb lsp-set-type sw$i-r0 router > > > + check ovn-nbctl --wait=sb lsp-set-options sw$i-r0 router-port=r0-sw$i > > > + check ovn-nbctl --wait=sb lsp-set-addresses sw$i-r0 router > > > + > > > + check ovn-nbctl --wait=sb lsp-add sw$i lsp$i > > > + check ovn-nbctl --wait=sb lsp-set-addresses lsp$i "fe:00:00:00:00:0$i 192.168.$i.100/24" > > > +done > > > + > > > +ovn-nbctl acl-add sw1 from-lport 3 tcp allow-stateless > > > +ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related > > > +ovn-nbctl --wait=sb sync > > > + > > > +flow_eth='eth.src == fe:00:00:00:00:01 && eth.dst == f0:00:00:00:00:01' > > > +flow_ip='ip.ttl==64 && ip4.src == 192.168.1.100 && ip4.dst == 192.168.2.100' > > > +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 because allow-related direction is different. > > > > This test case makes me wonder if the idea of this patch (as an optimization) is not correct. What if the packet is a return packet of a request packet allowed by "ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related"? In that case the packet does need to go to CT, otherwise the CT state will never be "established". I also had the impression that the priorities for different directions are independent, but it seems we will have to consider them together for the stateless + stateful scenario. Any thoughts on this? > > > I thought more about it, and it seems the return traffic problem is there for the mixed stateful and stateless scenarios regardless of this patch. For the return traffic belonging to a flow allowed by a "allow_related" ACL to work, we will have to send all packets that are possibly related to a stateful ACL to conntrack, regardless of its priority (because priority should be examined for the initiation direction only). However, it seems really complex to define logical flows to identify if a packet is "related" to a stateful ACL (if possible at all). I'd suggest considering this as a limitation of stateless ACLs and document it clearly so that when a user uses "allow-stateless" they understand the consequences: if there is an allow-stateless rule defined on direction A, any stateful rule of direction B that could have any return packets matching this allow-stateless rule may not work. Probably the ideal situation when "allow-stateless" is particularly useful is when the stateful an d stateless rules don't have any overlapping, e.g. one for TCP and the other for UDP. Think about this further, if we make it clear that allow-stateless should be used when there is no overlapping with stateful rules, the original patch (that has been merged) works perfectly because priority doesn't matter if there is no overlapping, and the code complexity and number of flows are both reduced. This way, the limitation is also easier to be documented and understood (comparing with clarifying accurately the limitation of priority + direction + "possible return traffic"). What do you think? I am for documenting the mechanics, but I think we should implement the API as defined in schema (incl. priority and direction). There is already some text in documentation that explains that allow-stateless requires more legwork and attention to define proper rules for returning traffic, we can expand that more to also explain that overlapping allow-stateless/allow-related rules may also interact. Ihar > > > Thanks, > > Han > > > > > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > > > +AT_CHECK_UNQUOTED([ovn-trace --minimal sw1 "${flow}"], [0], [dnl > > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > > +ip.ttl--; > > > +eth.src = f0:00:00:00:00:02; > > > +eth.dst = fe:00:00:00:00:02; > > > +output("lsp2"); > > > +]) > > > + > > > +# Add allow-related with the same direction. > > > +ovn-nbctl acl-add sw1 from-lport 4 tcp allow-related > > > +ovn-nbctl --wait=sb sync > > > + > > > +# TCP packets should now go to conntrack. > > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > > +ct_next(ct_state=new|trk) { > > > + ip.ttl--; > > > + eth.src = f0:00:00:00:00:02; > > > + eth.dst = fe:00:00:00:00:02; > > > + output("lsp2"); > > > +}; > > > +]) > > > + > > > +# Reduce priority for allow-related rule of the same direction. > > > +ovn-nbctl acl-del sw1 from-lport 4 tcp > > > +ovn-nbctl acl-add sw1 from-lport 2 tcp allow-related > > > +ovn-nbctl --wait=sb sync > > > + > > > +# TCP packets should no longer go to conntrack > > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > > +ip.ttl--; > > > +eth.src = f0:00:00:00:00:02; > > > +eth.dst = fe:00:00:00:00:02; > > > +output("lsp2"); > > > +]) > > > + > > > +AT_CLEANUP > > > +]) > > > + > > > OVN_FOR_EACH_NORTHD([ > > > AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) > > > ovn_start > > > -- > > > 2.31.1 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, May 20, 2021 at 6:22 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > > > While we *should not* circumvent conntrack when a stateful ACL of higher > > priority is present on the switch, we should do so only when > > allow-stateless and allow-stateful directions are the same, otherwise we > > should still skip conntrack for allow-stateless ACLs. > > > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") > > Is this patch a "fix" or an "optimization"? > It can be argued both ways. On one hand, indeed, it reduces the number of flows in some scenarios. On the other hand, sending traffic to conntrack when it, considering direction, should not be sent there, may be considered a bug. I tried to split the series into parts but here there are intersecting in such a way that the first patch in the series introduces incorrect behavior in a corner case that is then adjusted/fixed by this patch under review. > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > > --- > > northd/lswitch.dl | 88 ++++++++++++++++++++++++++++--------------- > > northd/ovn-northd.c | 89 ++++++++++++++++++++++++++++++++++---------- > > northd/ovn_northd.dl | 32 ++++++++-------- > > tests/ovn-northd.at | 72 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 213 insertions(+), 68 deletions(-) > > > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > > index b73cfd047..8a4c9154c 100644 > > --- a/northd/lswitch.dl > > +++ b/northd/lswitch.dl > > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :- > > LogicalSwitchACL(ls, acl), > > nb::ACL(._uuid = acl, .action = "allow-stateless"). > > > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid) > > + > > +LogicalSwitchStatelessFromACL(ls, acl) :- > > + LogicalSwitchStatelessACL(ls, acl), > > + nb::ACL(._uuid = acl, .direction = "from-lport"). > > + > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > +// is an output relation: > > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid, has_stateless_from_acl: bool) > > + > > +LogicalSwitchHasStatelessFromACL(ls, true) :- > > + LogicalSwitchStatelessFromACL(ls, _). > > + > > +LogicalSwitchHasStatelessFromACL(ls, false) :- > > + nb::Logical_Switch(._uuid = ls), > > + not LogicalSwitchStatelessFromACL(ls, _). > > + > > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid) > > + > > +LogicalSwitchStatelessToACL(ls, acl) :- > > + LogicalSwitchStatelessACL(ls, acl), > > + nb::ACL(._uuid = acl, .direction = "to-lport"). > > + > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > // is an output relation: > > -output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) > > +output relation LogicalSwitchHasStatelessToACL(ls: uuid, has_stateless_to_acl: bool) > > > > -LogicalSwitchHasStatelessACL(ls, true) :- > > - LogicalSwitchStatelessACL(ls, _). > > +LogicalSwitchHasStatelessToACL(ls, true) :- > > + LogicalSwitchStatelessToACL(ls, _). > > > > -LogicalSwitchHasStatelessACL(ls, false) :- > > +LogicalSwitchHasStatelessToACL(ls, false) :- > > nb::Logical_Switch(._uuid = ls), > > - not LogicalSwitchStatelessACL(ls, _). > > + not LogicalSwitchStatelessToACL(ls, _). > > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > // is an output relation: > > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :- > > /* Switch relation collects all attributes of a logical switch */ > > > > 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, > > - has_unknown_ports: bool, > > - localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > - subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > - ipv6_prefix: Option<in6_addr>, > > - mcast_cfg: Ref<McastSwitchCfg>, > > - is_vlan_transparent: bool, > > + ls: nb::Logical_Switch, > > + has_stateful_acl: bool, > > + has_stateless_from_acl: bool, > > + has_stateless_to_acl: bool, > > + has_acls: bool, > > + has_lb_vip: bool, > > + has_dns_records: bool, > > + has_unknown_ports: bool, > > + localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > + subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > + ipv6_prefix: Option<in6_addr>, > > + mcast_cfg: Ref<McastSwitchCfg>, > > + is_vlan_transparent: bool, > > > > /* Does this switch have at least one port with type != "router"? */ > > has_non_router_port: bool > > @@ -251,22 +275,24 @@ 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, > > - .has_unknown_ports = has_unknown_ports, > > - .localnet_ports = localnet_ports, > > - .subnet = subnet, > > - .ipv6_prefix = ipv6_prefix, > > - .mcast_cfg = mcast_cfg, > > - .has_non_router_port = has_non_router_port, > > - .is_vlan_transparent = is_vlan_transparent) :- > > +&Switch(.ls = ls, > > + .has_stateful_acl = has_stateful_acl, > > + .has_stateless_from_acl = has_stateless_from_acl, > > + .has_stateless_to_acl = has_stateless_to_acl, > > + .has_acls = has_acls, > > + .has_lb_vip = has_lb_vip, > > + .has_dns_records = has_dns_records, > > + .has_unknown_ports = has_unknown_ports, > > + .localnet_ports = localnet_ports, > > + .subnet = subnet, > > + .ipv6_prefix = ipv6_prefix, > > + .mcast_cfg = mcast_cfg, > > + .has_non_router_port = has_non_router_port, > > + .is_vlan_transparent = is_vlan_transparent) :- > > nb::Logical_Switch[ls], > > LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), > > - LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), > > + LogicalSwitchHasStatelessFromACL(ls._uuid, has_stateless_from_acl), > > + LogicalSwitchHasStatelessToACL(ls._uuid, has_stateless_to_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 a410be1de..1e027eab2 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -627,6 +627,8 @@ struct ovn_datapath { > > uint32_t port_key_hint; > > > > bool has_stateful_acl; > > + bool has_stateless_from; > > + bool has_stateless_to; > > bool has_lb_vip; > > bool has_unknown; > > bool has_acls; > > @@ -4759,19 +4761,46 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs) > > hmap_destroy(nb_pgs); > > } > > > > +static bool > > +ls_get_acl_flags_for_acl(struct ovn_datapath *od, > > + const struct nbrec_acl *acl) > > +{ > > + if (!strcmp(acl->action, "allow-related")) { > > + od->has_stateful_acl = true; > > + if (od->has_stateless_to && od->has_stateless_from) { > > + return true; > > + } > > + } > > + if (!strcmp(acl->action, "allow-stateless")) { > > + if (!strcmp(acl->direction, "from-lport")) { > > + od->has_stateless_from = true; > > + if (od->has_stateful_acl && od->has_stateless_to) { > > + return true; > > + } > > + } else { > > + od->has_stateless_to = true; > > + if (od->has_stateful_acl && od->has_stateless_from) { > > + return true; > > + } > > + } > > + } > > + return false; > > +} > > + > > static void > > ls_get_acl_flags(struct ovn_datapath *od) > > { > > od->has_acls = false; > > od->has_stateful_acl = false; > > + od->has_stateless_from = false; > > + od->has_stateless_to = false; > > > > if (od->nbs->n_acls) { > > od->has_acls = true; > > > > for (size_t i = 0; i < od->nbs->n_acls; i++) { > > struct nbrec_acl *acl = od->nbs->acls[i]; > > - if (!strcmp(acl->action, "allow-related")) { > > - od->has_stateful_acl = true; > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > return; > > } > > } > > @@ -4784,8 +4813,7 @@ ls_get_acl_flags(struct ovn_datapath *od) > > > > for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) { > > struct nbrec_acl *acl = ls_pg->nb_pg->acls[i]; > > - if (!strcmp(acl->action, "allow-related")) { > > - od->has_stateful_acl = true; > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > return; > > } > > } > > @@ -4984,17 +5012,20 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, > > } > > > > 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 *)) > > +apply_to_each_acl_of_action_and_direction( > > + struct ovn_datapath *od, > > + const struct hmap *port_groups, > > + struct hmap *lflows, > > + const char *action, const char *direction, > > + 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)) { > > + if (!strcmp(acl->action, action) && > > + (!direction || !strcmp(acl->direction, direction))) { > > func(od, acl, lflows); > > applied = true; > > } > > @@ -5005,7 +5036,8 @@ apply_to_each_acl_of_action(struct ovn_datapath *od, > > 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)) { > > + if (!strcmp(acl->action, action) && > > + (!direction || !strcmp(acl->direction, direction))) { > > func(od, acl, lflows); > > applied = true; > > } > > @@ -5040,8 +5072,9 @@ build_stateless_filters(struct ovn_datapath *od, > > const struct hmap *port_groups, > > struct hmap *lflows) > > { > > - return apply_to_each_acl_of_action( > > - od, port_groups, lflows, "allow-stateless", build_stateless_filter); > > + return apply_to_each_acl_of_action_and_direction( > > + od, port_groups, lflows, "allow-stateless", NULL, > > + build_stateless_filter); > > } > > > > static void > > @@ -5068,17 +5101,33 @@ build_stateful_override_filter(struct ovn_datapath *od, > > ds_destroy(&match); > > } > > > > +static void > > +build_stateful_override_filters_for_direction(struct ovn_datapath *od, > > + const struct hmap *port_groups, > > + struct hmap *lflows, > > + const char *direction) > > +{ > > + apply_to_each_acl_of_action_and_direction( > > + od, port_groups, lflows, "allow-related", direction, > > + build_stateful_override_filter); > > + apply_to_each_acl_of_action_and_direction( > > + od, port_groups, lflows, "allow", direction, > > + build_stateful_override_filter); > > +} > > + > > 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); > > - apply_to_each_acl_of_action( > > - od, port_groups, lflows, "allow", > > - build_stateful_override_filter); > > + if (od->has_stateless_from) { > > + build_stateful_override_filters_for_direction( > > + od, port_groups, lflows, "from-lport"); > > + } > > + if (od->has_stateless_to) { > > + build_stateful_override_filters_for_direction( > > + od, port_groups, lflows, "to-lport"); > > + } > > } > > > > static void > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > > index fc703f62e..8cbc959f0 100644 > > --- a/northd/ovn_northd.dl > > +++ b/northd/ovn_northd.dl > > @@ -1845,23 +1845,21 @@ 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 ((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(), > > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > - .__match = "ip && ${acl.__match}", > > - .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 ((sw.has_stateful_acl and acl.action == "allow") or acl.action == "allow-related") { > > + if (sw.has_stateless_from_acl and 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}", > > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > > + .external_ids = stage_hint(acl._uuid)) > > + } else if (sw.has_stateless_to_acl) { > > Should this be: else if (sw.has_stateless_to_acl and acl.direction == "to-lport") ? > Ouch. You are right. > > + 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)) > > } > > } > > } > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 6c38e1a97..1c55310b2 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -2664,6 +2664,78 @@ 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 of different direction]) > > +ovn_start > > + > > +# Create two switches to validate direction. We can't use the same switch for > > +# both ports, otherwise both in and out pipelines are triggered. > > +ovn-nbctl lr-add r0 > > +for i in $(seq 1 2); do > > + check ovn-nbctl --wait=sb ls-add sw$i > > + > > + check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i f0:00:00:00:00:0$i 192.168.$i.1/24 > > + check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0 > > + check ovn-nbctl --wait=sb lsp-set-type sw$i-r0 router > > + check ovn-nbctl --wait=sb lsp-set-options sw$i-r0 router-port=r0-sw$i > > + check ovn-nbctl --wait=sb lsp-set-addresses sw$i-r0 router > > + > > + check ovn-nbctl --wait=sb lsp-add sw$i lsp$i > > + check ovn-nbctl --wait=sb lsp-set-addresses lsp$i "fe:00:00:00:00:0$i 192.168.$i.100/24" > > +done > > + > > +ovn-nbctl acl-add sw1 from-lport 3 tcp allow-stateless > > +ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related > > +ovn-nbctl --wait=sb sync > > + > > +flow_eth='eth.src == fe:00:00:00:00:01 && eth.dst == f0:00:00:00:00:01' > > +flow_ip='ip.ttl==64 && ip4.src == 192.168.1.100 && ip4.dst == 192.168.2.100' > > +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 because allow-related direction is different. > > This test case makes me wonder if the idea of this patch (as an optimization) is not correct. What if the packet is a return packet of a request packet allowed by "ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related"? In that case the packet does need to go to CT, otherwise the CT state will never be "established". I also had the impression that the priorities for different directions are independent, but it seems we will have to consider them together for the stateless + stateful scenario. Any thoughts on this? > > Thanks, > Han > > > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > > +AT_CHECK_UNQUOTED([ovn-trace --minimal sw1 "${flow}"], [0], [dnl > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +ip.ttl--; > > +eth.src = f0:00:00:00:00:02; > > +eth.dst = fe:00:00:00:00:02; > > +output("lsp2"); > > +]) > > + > > +# Add allow-related with the same direction. > > +ovn-nbctl acl-add sw1 from-lport 4 tcp allow-related > > +ovn-nbctl --wait=sb sync > > + > > +# TCP packets should now go to conntrack. > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +ct_next(ct_state=new|trk) { > > + ip.ttl--; > > + eth.src = f0:00:00:00:00:02; > > + eth.dst = fe:00:00:00:00:02; > > + output("lsp2"); > > +}; > > +]) > > + > > +# Reduce priority for allow-related rule of the same direction. > > +ovn-nbctl acl-del sw1 from-lport 4 tcp > > +ovn-nbctl acl-add sw1 from-lport 2 tcp allow-related > > +ovn-nbctl --wait=sb sync > > + > > +# TCP packets should no longer go to conntrack > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +ip.ttl--; > > +eth.src = f0:00:00:00:00:02; > > +eth.dst = fe:00:00:00:00:02; > > +output("lsp2"); > > +]) > > + > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) > > ovn_start > > -- > > 2.31.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Jun 1, 2021 at 12:28 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > On Thu, May 20, 2021 at 9:55 PM Han Zhou <hzhou@ovn.org> wrote: > > > > > > > > On Thu, May 20, 2021 at 3:22 PM Han Zhou <hzhou@ovn.org> wrote: > > > > > > > > > > > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > > > > > > > While we *should not* circumvent conntrack when a stateful ACL of higher > > > > priority is present on the switch, we should do so only when > > > > allow-stateless and allow-stateful directions are the same, otherwise we > > > > should still skip conntrack for allow-stateless ACLs. > > > > > > > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") > > > > > > Is this patch a "fix" or an "optimization"? > > > > > > > > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > > > > --- > > > > northd/lswitch.dl | 88 ++++++++++++++++++++++++++++--------------- > > > > northd/ovn-northd.c | 89 ++++++++++++++++++++++++++++++++++---------- > > > > northd/ovn_northd.dl | 32 ++++++++-------- > > > > tests/ovn-northd.at | 72 +++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 213 insertions(+), 68 deletions(-) > > > > > > > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > > > > index b73cfd047..8a4c9154c 100644 > > > > --- a/northd/lswitch.dl > > > > +++ b/northd/lswitch.dl > > > > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :- > > > > LogicalSwitchACL(ls, acl), > > > > nb::ACL(._uuid = acl, .action = "allow-stateless"). > > > > > > > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid) > > > > + > > > > +LogicalSwitchStatelessFromACL(ls, acl) :- > > > > + LogicalSwitchStatelessACL(ls, acl), > > > > + nb::ACL(._uuid = acl, .direction = "from-lport"). > > > > + > > > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > > > +// is an output relation: > > > > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid, has_stateless_from_acl: bool) > > > > + > > > > +LogicalSwitchHasStatelessFromACL(ls, true) :- > > > > + LogicalSwitchStatelessFromACL(ls, _). > > > > + > > > > +LogicalSwitchHasStatelessFromACL(ls, false) :- > > > > + nb::Logical_Switch(._uuid = ls), > > > > + not LogicalSwitchStatelessFromACL(ls, _). > > > > + > > > > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid) > > > > + > > > > +LogicalSwitchStatelessToACL(ls, acl) :- > > > > + LogicalSwitchStatelessACL(ls, acl), > > > > + nb::ACL(._uuid = acl, .direction = "to-lport"). > > > > + > > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > > > // is an output relation: > > > > -output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) > > > > +output relation LogicalSwitchHasStatelessToACL(ls: uuid, has_stateless_to_acl: bool) > > > > > > > > -LogicalSwitchHasStatelessACL(ls, true) :- > > > > - LogicalSwitchStatelessACL(ls, _). > > > > +LogicalSwitchHasStatelessToACL(ls, true) :- > > > > + LogicalSwitchStatelessToACL(ls, _). > > > > > > > > -LogicalSwitchHasStatelessACL(ls, false) :- > > > > +LogicalSwitchHasStatelessToACL(ls, false) :- > > > > nb::Logical_Switch(._uuid = ls), > > > > - not LogicalSwitchStatelessACL(ls, _). > > > > + not LogicalSwitchStatelessToACL(ls, _). > > > > > > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > > > // is an output relation: > > > > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :- > > > > /* Switch relation collects all attributes of a logical switch */ > > > > > > > > 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, > > > > - has_unknown_ports: bool, > > > > - localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > > > - subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > > > - ipv6_prefix: Option<in6_addr>, > > > > - mcast_cfg: Ref<McastSwitchCfg>, > > > > - is_vlan_transparent: bool, > > > > + ls: nb::Logical_Switch, > > > > + has_stateful_acl: bool, > > > > + has_stateless_from_acl: bool, > > > > + has_stateless_to_acl: bool, > > > > + has_acls: bool, > > > > + has_lb_vip: bool, > > > > + has_dns_records: bool, > > > > + has_unknown_ports: bool, > > > > + localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > > > + subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > > > + ipv6_prefix: Option<in6_addr>, > > > > + mcast_cfg: Ref<McastSwitchCfg>, > > > > + is_vlan_transparent: bool, > > > > > > > > /* Does this switch have at least one port with type != "router"? */ > > > > has_non_router_port: bool > > > > @@ -251,22 +275,24 @@ 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, > > > > - .has_unknown_ports = has_unknown_ports, > > > > - .localnet_ports = localnet_ports, > > > > - .subnet = subnet, > > > > - .ipv6_prefix = ipv6_prefix, > > > > - .mcast_cfg = mcast_cfg, > > > > - .has_non_router_port = has_non_router_port, > > > > - .is_vlan_transparent = is_vlan_transparent) :- > > > > +&Switch(.ls = ls, > > > > + .has_stateful_acl = has_stateful_acl, > > > > + .has_stateless_from_acl = has_stateless_from_acl, > > > > + .has_stateless_to_acl = has_stateless_to_acl, > > > > + .has_acls = has_acls, > > > > + .has_lb_vip = has_lb_vip, > > > > + .has_dns_records = has_dns_records, > > > > + .has_unknown_ports = has_unknown_ports, > > > > + .localnet_ports = localnet_ports, > > > > + .subnet = subnet, > > > > + .ipv6_prefix = ipv6_prefix, > > > > + .mcast_cfg = mcast_cfg, > > > > + .has_non_router_port = has_non_router_port, > > > > + .is_vlan_transparent = is_vlan_transparent) :- > > > > nb::Logical_Switch[ls], > > > > LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), > > > > - LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), > > > > + LogicalSwitchHasStatelessFromACL(ls._uuid, has_stateless_from_acl), > > > > + LogicalSwitchHasStatelessToACL(ls._uuid, has_stateless_to_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 a410be1de..1e027eab2 100644 > > > > --- a/northd/ovn-northd.c > > > > +++ b/northd/ovn-northd.c > > > > @@ -627,6 +627,8 @@ struct ovn_datapath { > > > > uint32_t port_key_hint; > > > > > > > > bool has_stateful_acl; > > > > + bool has_stateless_from; > > > > + bool has_stateless_to; > > > > bool has_lb_vip; > > > > bool has_unknown; > > > > bool has_acls; > > > > @@ -4759,19 +4761,46 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs) > > > > hmap_destroy(nb_pgs); > > > > } > > > > > > > > +static bool > > > > +ls_get_acl_flags_for_acl(struct ovn_datapath *od, > > > > + const struct nbrec_acl *acl) > > > > +{ > > > > + if (!strcmp(acl->action, "allow-related")) { > > > > + od->has_stateful_acl = true; > > > > + if (od->has_stateless_to && od->has_stateless_from) { > > > > + return true; > > > > + } > > > > + } > > > > + if (!strcmp(acl->action, "allow-stateless")) { > > > > + if (!strcmp(acl->direction, "from-lport")) { > > > > + od->has_stateless_from = true; > > > > + if (od->has_stateful_acl && od->has_stateless_to) { > > > > + return true; > > > > + } > > > > + } else { > > > > + od->has_stateless_to = true; > > > > + if (od->has_stateful_acl && od->has_stateless_from) { > > > > + return true; > > > > + } > > > > + } > > > > + } > > > > + return false; > > > > +} > > > > + > > > > static void > > > > ls_get_acl_flags(struct ovn_datapath *od) > > > > { > > > > od->has_acls = false; > > > > od->has_stateful_acl = false; > > > > + od->has_stateless_from = false; > > > > + od->has_stateless_to = false; > > > > > > > > if (od->nbs->n_acls) { > > > > od->has_acls = true; > > > > > > > > for (size_t i = 0; i < od->nbs->n_acls; i++) { > > > > struct nbrec_acl *acl = od->nbs->acls[i]; > > > > - if (!strcmp(acl->action, "allow-related")) { > > > > - od->has_stateful_acl = true; > > > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > > > return; > > > > } > > > > } > > > > @@ -4784,8 +4813,7 @@ ls_get_acl_flags(struct ovn_datapath *od) > > > > > > > > for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) { > > > > struct nbrec_acl *acl = ls_pg->nb_pg->acls[i]; > > > > - if (!strcmp(acl->action, "allow-related")) { > > > > - od->has_stateful_acl = true; > > > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > > > return; > > > > } > > > > } > > > > @@ -4984,17 +5012,20 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, > > > > } > > > > > > > > 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 *)) > > > > +apply_to_each_acl_of_action_and_direction( > > > > + struct ovn_datapath *od, > > > > + const struct hmap *port_groups, > > > > + struct hmap *lflows, > > > > + const char *action, const char *direction, > > > > + 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)) { > > > > + if (!strcmp(acl->action, action) && > > > > + (!direction || !strcmp(acl->direction, direction))) { > > > > func(od, acl, lflows); > > > > applied = true; > > > > } > > > > @@ -5005,7 +5036,8 @@ apply_to_each_acl_of_action(struct ovn_datapath *od, > > > > 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)) { > > > > + if (!strcmp(acl->action, action) && > > > > + (!direction || !strcmp(acl->direction, direction))) { > > > > func(od, acl, lflows); > > > > applied = true; > > > > } > > > > @@ -5040,8 +5072,9 @@ build_stateless_filters(struct ovn_datapath *od, > > > > const struct hmap *port_groups, > > > > struct hmap *lflows) > > > > { > > > > - return apply_to_each_acl_of_action( > > > > - od, port_groups, lflows, "allow-stateless", build_stateless_filter); > > > > + return apply_to_each_acl_of_action_and_direction( > > > > + od, port_groups, lflows, "allow-stateless", NULL, > > > > + build_stateless_filter); > > > > } > > > > > > > > static void > > > > @@ -5068,17 +5101,33 @@ build_stateful_override_filter(struct ovn_datapath *od, > > > > ds_destroy(&match); > > > > } > > > > > > > > +static void > > > > +build_stateful_override_filters_for_direction(struct ovn_datapath *od, > > > > + const struct hmap *port_groups, > > > > + struct hmap *lflows, > > > > + const char *direction) > > > > +{ > > > > + apply_to_each_acl_of_action_and_direction( > > > > + od, port_groups, lflows, "allow-related", direction, > > > > + build_stateful_override_filter); > > > > + apply_to_each_acl_of_action_and_direction( > > > > + od, port_groups, lflows, "allow", direction, > > > > + build_stateful_override_filter); > > > > +} > > > > + > > > > 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); > > > > - apply_to_each_acl_of_action( > > > > - od, port_groups, lflows, "allow", > > > > - build_stateful_override_filter); > > > > + if (od->has_stateless_from) { > > > > + build_stateful_override_filters_for_direction( > > > > + od, port_groups, lflows, "from-lport"); > > > > + } > > > > + if (od->has_stateless_to) { > > > > + build_stateful_override_filters_for_direction( > > > > + od, port_groups, lflows, "to-lport"); > > > > + } > > > > } > > > > > > > > static void > > > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > > > > index fc703f62e..8cbc959f0 100644 > > > > --- a/northd/ovn_northd.dl > > > > +++ b/northd/ovn_northd.dl > > > > @@ -1845,23 +1845,21 @@ 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 ((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(), > > > > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > > > - .__match = "ip && ${acl.__match}", > > > > - .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 ((sw.has_stateful_acl and acl.action == "allow") or acl.action == "allow-related") { > > > > + if (sw.has_stateless_from_acl and 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}", > > > > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > > > > + .external_ids = stage_hint(acl._uuid)) > > > > + } else if (sw.has_stateless_to_acl) { > > > > > > Should this be: else if (sw.has_stateless_to_acl and acl.direction == "to-lport") ? > > > > > > > + 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)) > > > > } > > > > } > > > > } > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > > index 6c38e1a97..1c55310b2 100644 > > > > --- a/tests/ovn-northd.at > > > > +++ b/tests/ovn-northd.at > > > > @@ -2664,6 +2664,78 @@ 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 of different direction]) > > > > +ovn_start > > > > + > > > > +# Create two switches to validate direction. We can't use the same switch for > > > > +# both ports, otherwise both in and out pipelines are triggered. > > > > +ovn-nbctl lr-add r0 > > > > +for i in $(seq 1 2); do > > > > + check ovn-nbctl --wait=sb ls-add sw$i > > > > + > > > > + check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i f0:00:00:00:00:0$i 192.168.$i.1/24 > > > > + check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0 > > > > + check ovn-nbctl --wait=sb lsp-set-type sw$i-r0 router > > > > + check ovn-nbctl --wait=sb lsp-set-options sw$i-r0 router-port=r0-sw$i > > > > + check ovn-nbctl --wait=sb lsp-set-addresses sw$i-r0 router > > > > + > > > > + check ovn-nbctl --wait=sb lsp-add sw$i lsp$i > > > > + check ovn-nbctl --wait=sb lsp-set-addresses lsp$i "fe:00:00:00:00:0$i 192.168.$i.100/24" > > > > +done > > > > + > > > > +ovn-nbctl acl-add sw1 from-lport 3 tcp allow-stateless > > > > +ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related > > > > +ovn-nbctl --wait=sb sync > > > > + > > > > +flow_eth='eth.src == fe:00:00:00:00:01 && eth.dst == f0:00:00:00:00:01' > > > > +flow_ip='ip.ttl==64 && ip4.src == 192.168.1.100 && ip4.dst == 192.168.2.100' > > > > +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 because allow-related direction is different. > > > > > > This test case makes me wonder if the idea of this patch (as an optimization) is not correct. What if the packet is a return packet of a request packet allowed by "ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related"? In that case the packet does need to go to CT, otherwise the CT state will never be "established". I also had the impression that the priorities for different directions are independent, but it seems we will have to consider them together for the stateless + stateful scenario. Any thoughts on this? > > > > > I thought more about it, and it seems the return traffic problem is there for the mixed stateful and stateless scenarios regardless of this patch. For the return traffic belonging to a flow allowed by a "allow_related" ACL to work, we will have to send all packets that are possibly related to a stateful ACL to conntrack, regardless of its priority (because priority should be examined for the initiation direction only). However, it seems really complex to define logical flows to identify if a packet is "related" to a stateful ACL (if possible at all). I'd suggest considering this as a limitation of stateless ACLs and document it clearly so that when a user uses "allow-stateless" they understand the consequences: if there is an allow-stateless rule defined on direction A, any stateful rule of direction B that could have any return packets matching this allow-stateless rule may not work. Probably the ideal situation when "allow-stateless" is particularly useful is when the stateful and stateless rules don't have any overlapping, e.g. one for TCP and the other for UDP. Think about this further, if we make it clear that allow-stateless should be used when there is no overlapping with stateful rules, the original patch (that has been merged) works perfectly because priority doesn't matter if there is no overlapping, and the code complexity and number of flows are both reduced. This way, the limitation is also easier to be documented and understood (comparing with clarifying accurately the limitation of priority + direction + "possible return traffic"). What do you think? > > I am for documenting the mechanics, but I think we should implement > the API as defined in schema (incl. priority and direction). There is > already some text in documentation that explains that allow-stateless > requires more legwork and attention to define proper rules for > returning traffic, we can expand that more to also explain that > overlapping allow-stateless/allow-related rules may also interact. > Hi Ihar, It would be great if we can define the behavior clearly with the proper implementation. However, at least it is still not clear to me what these 3 patches could achieve on top of the original patch for real world use cases. With your original patch for "allow-stateless", if there are overlapping stateful and stateless rules, the stateless rules will override regardless of priority. Now with these 3 patches, it is expected to honor priority. But consider a typical example: All traffic from A to B (egress) is stateless: ACL1: from-lport 100 'inport == "A" && ip.dst == B' allow-stateless So does the return traffic: ACL2: to-lport 100 'outport == "A" && ip.src == B' allow-stateless Now we need the traffic initiated from A to B's TCP port 80 to be stateful: ACL3: from-lport 200 'inport == "X" && ip.dst == B && tcp.dst == 80' allow-related This seems to be what we want to achieve, right? With the priority 200 we hope that TCP traffic between A <-> B:80 is handled as stateful and allows the return traffic (i.e. B:80 -> A) automatically, and also probably prevent invalid packets. However, even with these 3 patches, it doesn't work, because packets of the direction B:80 -> A will match the stateless ACL2 and bypass conntrack, so the CT entry committed by the A->B:80 SYN packets is never going to become "established" state because the CT table only sees one direction of the traffic. I hope this example is general enough for the discussion. So we need to answer these questions: 1. How do we define the meaning of "priority" in above scenario: what is expected and what extra configuration needs to be done to achieve the desired behavior. 2. If the above example is not what we want to support, then what use cases are actually supported by adding these 3 patches (but not by the original patch). 3. If 2) has an answer, does it worth the complexity and the extra flows added? Could you clarify more on this? Thanks, Han > Ihar > > > > > > Thanks, > > > Han > > > > > > > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > > > > +AT_CHECK_UNQUOTED([ovn-trace --minimal sw1 "${flow}"], [0], [dnl > > > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > > > +ip.ttl--; > > > > +eth.src = f0:00:00:00:00:02; > > > > +eth.dst = fe:00:00:00:00:02; > > > > +output("lsp2"); > > > > +]) > > > > + > > > > +# Add allow-related with the same direction. > > > > +ovn-nbctl acl-add sw1 from-lport 4 tcp allow-related > > > > +ovn-nbctl --wait=sb sync > > > > + > > > > +# TCP packets should now go to conntrack. > > > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > > > +ct_next(ct_state=new|trk) { > > > > + ip.ttl--; > > > > + eth.src = f0:00:00:00:00:02; > > > > + eth.dst = fe:00:00:00:00:02; > > > > + output("lsp2"); > > > > +}; > > > > +]) > > > > + > > > > +# Reduce priority for allow-related rule of the same direction. > > > > +ovn-nbctl acl-del sw1 from-lport 4 tcp > > > > +ovn-nbctl acl-add sw1 from-lport 2 tcp allow-related > > > > +ovn-nbctl --wait=sb sync > > > > + > > > > +# TCP packets should no longer go to conntrack > > > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > > > +ip.ttl--; > > > > +eth.src = f0:00:00:00:00:02; > > > > +eth.dst = fe:00:00:00:00:02; > > > > +output("lsp2"); > > > > +]) > > > > + > > > > +AT_CLEANUP > > > > +]) > > > > + > > > > OVN_FOR_EACH_NORTHD([ > > > > AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) > > > > ovn_start > > > > -- > > > > 2.31.1 > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Jun 2, 2021 at 12:22 AM Han Zhou <hzhou@ovn.org> wrote: > > > > On Tue, Jun 1, 2021 at 12:28 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > > > On Thu, May 20, 2021 at 9:55 PM Han Zhou <hzhou@ovn.org> wrote: > > > > > > > > > > > > On Thu, May 20, 2021 at 3:22 PM Han Zhou <hzhou@ovn.org> wrote: > > > > > > > > > > > > > > > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > > > > > > > > > While we *should not* circumvent conntrack when a stateful ACL of higher > > > > > priority is present on the switch, we should do so only when > > > > > allow-stateless and allow-stateful directions are the same, otherwise we > > > > > should still skip conntrack for allow-stateless ACLs. > > > > > > > > > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") > > > > > > > > Is this patch a "fix" or an "optimization"? > > > > > > > > > > > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > > > > > --- > > > > > northd/lswitch.dl | 88 ++++++++++++++++++++++++++++--------------- > > > > > northd/ovn-northd.c | 89 ++++++++++++++++++++++++++++++++++---------- > > > > > northd/ovn_northd.dl | 32 ++++++++-------- > > > > > tests/ovn-northd.at | 72 +++++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 213 insertions(+), 68 deletions(-) > > > > > > > > > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > > > > > index b73cfd047..8a4c9154c 100644 > > > > > --- a/northd/lswitch.dl > > > > > +++ b/northd/lswitch.dl > > > > > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :- > > > > > LogicalSwitchACL(ls, acl), > > > > > nb::ACL(._uuid = acl, .action = "allow-stateless"). > > > > > > > > > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid) > > > > > + > > > > > +LogicalSwitchStatelessFromACL(ls, acl) :- > > > > > + LogicalSwitchStatelessACL(ls, acl), > > > > > + nb::ACL(._uuid = acl, .direction = "from-lport"). > > > > > + > > > > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > > > > +// is an output relation: > > > > > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid, has_stateless_from_acl: bool) > > > > > + > > > > > +LogicalSwitchHasStatelessFromACL(ls, true) :- > > > > > + LogicalSwitchStatelessFromACL(ls, _). > > > > > + > > > > > +LogicalSwitchHasStatelessFromACL(ls, false) :- > > > > > + nb::Logical_Switch(._uuid = ls), > > > > > + not LogicalSwitchStatelessFromACL(ls, _). > > > > > + > > > > > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid) > > > > > + > > > > > +LogicalSwitchStatelessToACL(ls, acl) :- > > > > > + LogicalSwitchStatelessACL(ls, acl), > > > > > + nb::ACL(._uuid = acl, .direction = "to-lport"). > > > > > + > > > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > > > > // is an output relation: > > > > > -output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) > > > > > +output relation LogicalSwitchHasStatelessToACL(ls: uuid, has_stateless_to_acl: bool) > > > > > > > > > > -LogicalSwitchHasStatelessACL(ls, true) :- > > > > > - LogicalSwitchStatelessACL(ls, _). > > > > > +LogicalSwitchHasStatelessToACL(ls, true) :- > > > > > + LogicalSwitchStatelessToACL(ls, _). > > > > > > > > > > -LogicalSwitchHasStatelessACL(ls, false) :- > > > > > +LogicalSwitchHasStatelessToACL(ls, false) :- > > > > > nb::Logical_Switch(._uuid = ls), > > > > > - not LogicalSwitchStatelessACL(ls, _). > > > > > + not LogicalSwitchStatelessToACL(ls, _). > > > > > > > > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > > > > // is an output relation: > > > > > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :- > > > > > /* Switch relation collects all attributes of a logical switch */ > > > > > > > > > > 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, > > > > > - has_unknown_ports: bool, > > > > > - localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > > > > - subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > > > > - ipv6_prefix: Option<in6_addr>, > > > > > - mcast_cfg: Ref<McastSwitchCfg>, > > > > > - is_vlan_transparent: bool, > > > > > + ls: nb::Logical_Switch, > > > > > + has_stateful_acl: bool, > > > > > + has_stateless_from_acl: bool, > > > > > + has_stateless_to_acl: bool, > > > > > + has_acls: bool, > > > > > + has_lb_vip: bool, > > > > > + has_dns_records: bool, > > > > > + has_unknown_ports: bool, > > > > > + localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. > > > > > + subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, > > > > > + ipv6_prefix: Option<in6_addr>, > > > > > + mcast_cfg: Ref<McastSwitchCfg>, > > > > > + is_vlan_transparent: bool, > > > > > > > > > > /* Does this switch have at least one port with type != "router"? */ > > > > > has_non_router_port: bool > > > > > @@ -251,22 +275,24 @@ 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, > > > > > - .has_unknown_ports = has_unknown_ports, > > > > > - .localnet_ports = localnet_ports, > > > > > - .subnet = subnet, > > > > > - .ipv6_prefix = ipv6_prefix, > > > > > - .mcast_cfg = mcast_cfg, > > > > > - .has_non_router_port = has_non_router_port, > > > > > - .is_vlan_transparent = is_vlan_transparent) :- > > > > > +&Switch(.ls = ls, > > > > > + .has_stateful_acl = has_stateful_acl, > > > > > + .has_stateless_from_acl = has_stateless_from_acl, > > > > > + .has_stateless_to_acl = has_stateless_to_acl, > > > > > + .has_acls = has_acls, > > > > > + .has_lb_vip = has_lb_vip, > > > > > + .has_dns_records = has_dns_records, > > > > > + .has_unknown_ports = has_unknown_ports, > > > > > + .localnet_ports = localnet_ports, > > > > > + .subnet = subnet, > > > > > + .ipv6_prefix = ipv6_prefix, > > > > > + .mcast_cfg = mcast_cfg, > > > > > + .has_non_router_port = has_non_router_port, > > > > > + .is_vlan_transparent = is_vlan_transparent) :- > > > > > nb::Logical_Switch[ls], > > > > > LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), > > > > > - LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), > > > > > + LogicalSwitchHasStatelessFromACL(ls._uuid, has_stateless_from_acl), > > > > > + LogicalSwitchHasStatelessToACL(ls._uuid, has_stateless_to_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 a410be1de..1e027eab2 100644 > > > > > --- a/northd/ovn-northd.c > > > > > +++ b/northd/ovn-northd.c > > > > > @@ -627,6 +627,8 @@ struct ovn_datapath { > > > > > uint32_t port_key_hint; > > > > > > > > > > bool has_stateful_acl; > > > > > + bool has_stateless_from; > > > > > + bool has_stateless_to; > > > > > bool has_lb_vip; > > > > > bool has_unknown; > > > > > bool has_acls; > > > > > @@ -4759,19 +4761,46 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs) > > > > > hmap_destroy(nb_pgs); > > > > > } > > > > > > > > > > +static bool > > > > > +ls_get_acl_flags_for_acl(struct ovn_datapath *od, > > > > > + const struct nbrec_acl *acl) > > > > > +{ > > > > > + if (!strcmp(acl->action, "allow-related")) { > > > > > + od->has_stateful_acl = true; > > > > > + if (od->has_stateless_to && od->has_stateless_from) { > > > > > + return true; > > > > > + } > > > > > + } > > > > > + if (!strcmp(acl->action, "allow-stateless")) { > > > > > + if (!strcmp(acl->direction, "from-lport")) { > > > > > + od->has_stateless_from = true; > > > > > + if (od->has_stateful_acl && od->has_stateless_to) { > > > > > + return true; > > > > > + } > > > > > + } else { > > > > > + od->has_stateless_to = true; > > > > > + if (od->has_stateful_acl && od->has_stateless_from) { > > > > > + return true; > > > > > + } > > > > > + } > > > > > + } > > > > > + return false; > > > > > +} > > > > > + > > > > > static void > > > > > ls_get_acl_flags(struct ovn_datapath *od) > > > > > { > > > > > od->has_acls = false; > > > > > od->has_stateful_acl = false; > > > > > + od->has_stateless_from = false; > > > > > + od->has_stateless_to = false; > > > > > > > > > > if (od->nbs->n_acls) { > > > > > od->has_acls = true; > > > > > > > > > > for (size_t i = 0; i < od->nbs->n_acls; i++) { > > > > > struct nbrec_acl *acl = od->nbs->acls[i]; > > > > > - if (!strcmp(acl->action, "allow-related")) { > > > > > - od->has_stateful_acl = true; > > > > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > > > > return; > > > > > } > > > > > } > > > > > @@ -4784,8 +4813,7 @@ ls_get_acl_flags(struct ovn_datapath *od) > > > > > > > > > > for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) { > > > > > struct nbrec_acl *acl = ls_pg->nb_pg->acls[i]; > > > > > - if (!strcmp(acl->action, "allow-related")) { > > > > > - od->has_stateful_acl = true; > > > > > + if (ls_get_acl_flags_for_acl(od, acl)) { > > > > > return; > > > > > } > > > > > } > > > > > @@ -4984,17 +5012,20 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, > > > > > } > > > > > > > > > > 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 *)) > > > > > +apply_to_each_acl_of_action_and_direction( > > > > > + struct ovn_datapath *od, > > > > > + const struct hmap *port_groups, > > > > > + struct hmap *lflows, > > > > > + const char *action, const char *direction, > > > > > + 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)) { > > > > > + if (!strcmp(acl->action, action) && > > > > > + (!direction || !strcmp(acl->direction, direction))) { > > > > > func(od, acl, lflows); > > > > > applied = true; > > > > > } > > > > > @@ -5005,7 +5036,8 @@ apply_to_each_acl_of_action(struct ovn_datapath *od, > > > > > 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)) { > > > > > + if (!strcmp(acl->action, action) && > > > > > + (!direction || !strcmp(acl->direction, direction))) { > > > > > func(od, acl, lflows); > > > > > applied = true; > > > > > } > > > > > @@ -5040,8 +5072,9 @@ build_stateless_filters(struct ovn_datapath *od, > > > > > const struct hmap *port_groups, > > > > > struct hmap *lflows) > > > > > { > > > > > - return apply_to_each_acl_of_action( > > > > > - od, port_groups, lflows, "allow-stateless", build_stateless_filter); > > > > > + return apply_to_each_acl_of_action_and_direction( > > > > > + od, port_groups, lflows, "allow-stateless", NULL, > > > > > + build_stateless_filter); > > > > > } > > > > > > > > > > static void > > > > > @@ -5068,17 +5101,33 @@ build_stateful_override_filter(struct ovn_datapath *od, > > > > > ds_destroy(&match); > > > > > } > > > > > > > > > > +static void > > > > > +build_stateful_override_filters_for_direction(struct ovn_datapath *od, > > > > > + const struct hmap *port_groups, > > > > > + struct hmap *lflows, > > > > > + const char *direction) > > > > > +{ > > > > > + apply_to_each_acl_of_action_and_direction( > > > > > + od, port_groups, lflows, "allow-related", direction, > > > > > + build_stateful_override_filter); > > > > > + apply_to_each_acl_of_action_and_direction( > > > > > + od, port_groups, lflows, "allow", direction, > > > > > + build_stateful_override_filter); > > > > > +} > > > > > + > > > > > 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); > > > > > - apply_to_each_acl_of_action( > > > > > - od, port_groups, lflows, "allow", > > > > > - build_stateful_override_filter); > > > > > + if (od->has_stateless_from) { > > > > > + build_stateful_override_filters_for_direction( > > > > > + od, port_groups, lflows, "from-lport"); > > > > > + } > > > > > + if (od->has_stateless_to) { > > > > > + build_stateful_override_filters_for_direction( > > > > > + od, port_groups, lflows, "to-lport"); > > > > > + } > > > > > } > > > > > > > > > > static void > > > > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > > > > > index fc703f62e..8cbc959f0 100644 > > > > > --- a/northd/ovn_northd.dl > > > > > +++ b/northd/ovn_northd.dl > > > > > @@ -1845,23 +1845,21 @@ 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 ((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(), > > > > > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > > > > - .__match = "ip && ${acl.__match}", > > > > > - .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 ((sw.has_stateful_acl and acl.action == "allow") or acl.action == "allow-related") { > > > > > + if (sw.has_stateless_from_acl and 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}", > > > > > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > > > > > + .external_ids = stage_hint(acl._uuid)) > > > > > + } else if (sw.has_stateless_to_acl) { > > > > > > > > Should this be: else if (sw.has_stateless_to_acl and acl.direction == "to-lport") ? > > > > > > > > > + 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)) > > > > > } > > > > > } > > > > > } > > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > > > index 6c38e1a97..1c55310b2 100644 > > > > > --- a/tests/ovn-northd.at > > > > > +++ b/tests/ovn-northd.at > > > > > @@ -2664,6 +2664,78 @@ 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 of different direction]) > > > > > +ovn_start > > > > > + > > > > > +# Create two switches to validate direction. We can't use the same switch for > > > > > +# both ports, otherwise both in and out pipelines are triggered. > > > > > +ovn-nbctl lr-add r0 > > > > > +for i in $(seq 1 2); do > > > > > + check ovn-nbctl --wait=sb ls-add sw$i > > > > > + > > > > > + check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i f0:00:00:00:00:0$i 192.168.$i.1/24 > > > > > + check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0 > > > > > + check ovn-nbctl --wait=sb lsp-set-type sw$i-r0 router > > > > > + check ovn-nbctl --wait=sb lsp-set-options sw$i-r0 router-port=r0-sw$i > > > > > + check ovn-nbctl --wait=sb lsp-set-addresses sw$i-r0 router > > > > > + > > > > > + check ovn-nbctl --wait=sb lsp-add sw$i lsp$i > > > > > + check ovn-nbctl --wait=sb lsp-set-addresses lsp$i "fe:00:00:00:00:0$i 192.168.$i.100/24" > > > > > +done > > > > > + > > > > > +ovn-nbctl acl-add sw1 from-lport 3 tcp allow-stateless > > > > > +ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related > > > > > +ovn-nbctl --wait=sb sync > > > > > + > > > > > +flow_eth='eth.src == fe:00:00:00:00:01 && eth.dst == f0:00:00:00:00:01' > > > > > +flow_ip='ip.ttl==64 && ip4.src == 192.168.1.100 && ip4.dst == 192.168.2.100' > > > > > +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 because allow-related direction is different. > > > > > > > > This test case makes me wonder if the idea of this patch (as an optimization) is not correct. What if the packet is a return packet of a request packet allowed by "ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related"? In that case the packet does need to go to CT, otherwise the CT state will never be "established". I also had the impression that the priorities for different directions are independent, but it seems we will have to consider them together for the stateless + stateful scenario. Any thoughts on this? > > > > > > > I thought more about it, and it seems the return traffic problem is there for the mixed stateful and stateless scenarios regardless of this patch. For the return traffic belonging to a flow allowed by a "allow_related" ACL to work, we will have to send all packets that are possibly related to a stateful ACL to conntrack, regardless of its priority (because priority should be examined for the initiation direction only). However, it seems really complex to define logical flows to identify if a packet is "related" to a stateful ACL (if possible at all). I'd suggest considering this as a limitation of stateless ACLs and document it clearly so that when a user uses "allow-stateless" they understand the consequences: if there is an allow-stateless rule defined on direction A, any stateful rule of direction B that could have any return packets matching this allow-stateless rule may not work. Probably the ideal situation when "allow-stateless" is particularly useful is when the statefu l and stateless rules don't have any overlapping, e.g. one for TCP and the other for UDP. Think about this further, if we make it clear that allow-stateless should be used when there is no overlapping with stateful rules, the original patch (that has been merged) works perfectly because priority doesn't matter if there is no overlapping, and the code complexity and number of flows are both reduced. This way, the limitation is also easier to be documented and understood (comparing with clarifying accurately the limitation of priority + direction + "possible return traffic"). What do you think? > > > > I am for documenting the mechanics, but I think we should implement > > the API as defined in schema (incl. priority and direction). There is > > already some text in documentation that explains that allow-stateless > > requires more legwork and attention to define proper rules for > > returning traffic, we can expand that more to also explain that > > overlapping allow-stateless/allow-related rules may also interact. > > > > Hi Ihar, > > It would be great if we can define the behavior clearly with the proper implementation. However, at least it is still not clear to me what these 3 patches could achieve on top of the original patch for real world use cases. With your original patch for "allow-stateless", if there are overlapping stateful and stateless rules, the stateless rules will override regardless of priority. Now with these 3 patches, it is expected to honor priority. But consider a typical example: > > All traffic from A to B (egress) is stateless: > ACL1: from-lport 100 'inport == "A" && ip.dst == B' allow-stateless > > So does the return traffic: > ACL2: to-lport 100 'outport == "A" && ip.src == B' allow-stateless > > Now we need the traffic initiated from A to B's TCP port 80 to be stateful: > ACL3: from-lport 200 'inport == "X" && ip.dst == B && tcp.dst == 80' allow-related > > This seems to be what we want to achieve, right? With the priority 200 we hope that TCP traffic between A <-> B:80 is handled as stateful and allows the return traffic (i.e. B:80 -> A) automatically, and also probably prevent invalid packets. However, even with these 3 patches, it doesn't work, because packets of the direction B:80 -> A will match the stateless ACL2 and bypass conntrack, so the CT entry committed by the A->B:80 SYN packets is never going to become "established" state because the CT table only sees one direction of the traffic. > OK, I think I misunderstood your original reply. Yes, the scenario you describe seems to not work as intended. (It was the plan here.) I can't figure out a practical solution for the problem. We would have to insert a counterpart flow with 200-priority in to-lport table to steer relevant traffic to ct, but since match rules are not structured, it's impossible to narrow the scope of traffic to affect more than just "everything returning to the port", which defeats the purpose of 'allow-stateless' (in contrast to 'allow' verb). Yes, I now think your original suggestion - to document the limitation (basically, saying that allow-stateless are always of higher priority than any other matching stateful rules) - is the way to go. It's not great since priority API is not strictly implemented, but I don't think another way around. Please disregard the series, I will send a small update to the documentation. Ihar > I hope this example is general enough for the discussion. So we need to answer these questions: > 1. How do we define the meaning of "priority" in above scenario: what is expected and what extra configuration needs to be done to achieve the desired behavior. > 2. If the above example is not what we want to support, then what use cases are actually supported by adding these 3 patches (but not by the original patch). > 3. If 2) has an answer, does it worth the complexity and the extra flows added? > > Could you clarify more on this? > > Thanks, > Han > > > Ihar > > > > > > > > > Thanks, > > > > Han > > > > > > > > > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > > > > > +AT_CHECK_UNQUOTED([ovn-trace --minimal sw1 "${flow}"], [0], [dnl > > > > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > > > > +ip.ttl--; > > > > > +eth.src = f0:00:00:00:00:02; > > > > > +eth.dst = fe:00:00:00:00:02; > > > > > +output("lsp2"); > > > > > +]) > > > > > + > > > > > +# Add allow-related with the same direction. > > > > > +ovn-nbctl acl-add sw1 from-lport 4 tcp allow-related > > > > > +ovn-nbctl --wait=sb sync > > > > > + > > > > > +# TCP packets should now go to conntrack. > > > > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > > > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > > > > +ct_next(ct_state=new|trk) { > > > > > + ip.ttl--; > > > > > + eth.src = f0:00:00:00:00:02; > > > > > + eth.dst = fe:00:00:00:00:02; > > > > > + output("lsp2"); > > > > > +}; > > > > > +]) > > > > > + > > > > > +# Reduce priority for allow-related rule of the same direction. > > > > > +ovn-nbctl acl-del sw1 from-lport 4 tcp > > > > > +ovn-nbctl acl-add sw1 from-lport 2 tcp allow-related > > > > > +ovn-nbctl --wait=sb sync > > > > > + > > > > > +# TCP packets should no longer go to conntrack > > > > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl > > > > > +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > > > > +ip.ttl--; > > > > > +eth.src = f0:00:00:00:00:02; > > > > > +eth.dst = fe:00:00:00:00:02; > > > > > +output("lsp2"); > > > > > +]) > > > > > + > > > > > +AT_CLEANUP > > > > > +]) > > > > > + > > > > > OVN_FOR_EACH_NORTHD([ > > > > > AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) > > > > > ovn_start > > > > > -- > > > > > 2.31.1 > > > > > > > > > > _______________________________________________ > > > > > dev mailing list > > > > > dev@openvswitch.org > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/northd/lswitch.dl b/northd/lswitch.dl index b73cfd047..8a4c9154c 100644 --- a/northd/lswitch.dl +++ b/northd/lswitch.dl @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :- LogicalSwitchACL(ls, acl), nb::ACL(._uuid = acl, .action = "allow-stateless"). +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid) + +LogicalSwitchStatelessFromACL(ls, acl) :- + LogicalSwitchStatelessACL(ls, acl), + nb::ACL(._uuid = acl, .direction = "from-lport"). + +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this +// is an output relation: +output relation LogicalSwitchHasStatelessFromACL(ls: uuid, has_stateless_from_acl: bool) + +LogicalSwitchHasStatelessFromACL(ls, true) :- + LogicalSwitchStatelessFromACL(ls, _). + +LogicalSwitchHasStatelessFromACL(ls, false) :- + nb::Logical_Switch(._uuid = ls), + not LogicalSwitchStatelessFromACL(ls, _). + +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid) + +LogicalSwitchStatelessToACL(ls, acl) :- + LogicalSwitchStatelessACL(ls, acl), + nb::ACL(._uuid = acl, .direction = "to-lport"). + // "Pitfalls of projections" in ddlog-new-feature.rst explains why this // is an output relation: -output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) +output relation LogicalSwitchHasStatelessToACL(ls: uuid, has_stateless_to_acl: bool) -LogicalSwitchHasStatelessACL(ls, true) :- - LogicalSwitchStatelessACL(ls, _). +LogicalSwitchHasStatelessToACL(ls, true) :- + LogicalSwitchStatelessToACL(ls, _). -LogicalSwitchHasStatelessACL(ls, false) :- +LogicalSwitchHasStatelessToACL(ls, false) :- nb::Logical_Switch(._uuid = ls), - not LogicalSwitchStatelessACL(ls, _). + not LogicalSwitchStatelessToACL(ls, _). // "Pitfalls of projections" in ddlog-new-feature.rst explains why this // is an output relation: @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :- /* Switch relation collects all attributes of a logical switch */ 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, - has_unknown_ports: bool, - localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. - subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, - ipv6_prefix: Option<in6_addr>, - mcast_cfg: Ref<McastSwitchCfg>, - is_vlan_transparent: bool, + ls: nb::Logical_Switch, + has_stateful_acl: bool, + has_stateless_from_acl: bool, + has_stateless_to_acl: bool, + has_acls: bool, + has_lb_vip: bool, + has_dns_records: bool, + has_unknown_ports: bool, + localnet_ports: Vec<(uuid, string)>, // UUID and name of each localnet port. + subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>, + ipv6_prefix: Option<in6_addr>, + mcast_cfg: Ref<McastSwitchCfg>, + is_vlan_transparent: bool, /* Does this switch have at least one port with type != "router"? */ has_non_router_port: bool @@ -251,22 +275,24 @@ 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, - .has_unknown_ports = has_unknown_ports, - .localnet_ports = localnet_ports, - .subnet = subnet, - .ipv6_prefix = ipv6_prefix, - .mcast_cfg = mcast_cfg, - .has_non_router_port = has_non_router_port, - .is_vlan_transparent = is_vlan_transparent) :- +&Switch(.ls = ls, + .has_stateful_acl = has_stateful_acl, + .has_stateless_from_acl = has_stateless_from_acl, + .has_stateless_to_acl = has_stateless_to_acl, + .has_acls = has_acls, + .has_lb_vip = has_lb_vip, + .has_dns_records = has_dns_records, + .has_unknown_ports = has_unknown_ports, + .localnet_ports = localnet_ports, + .subnet = subnet, + .ipv6_prefix = ipv6_prefix, + .mcast_cfg = mcast_cfg, + .has_non_router_port = has_non_router_port, + .is_vlan_transparent = is_vlan_transparent) :- nb::Logical_Switch[ls], LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), - LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), + LogicalSwitchHasStatelessFromACL(ls._uuid, has_stateless_from_acl), + LogicalSwitchHasStatelessToACL(ls._uuid, has_stateless_to_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 a410be1de..1e027eab2 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -627,6 +627,8 @@ struct ovn_datapath { uint32_t port_key_hint; bool has_stateful_acl; + bool has_stateless_from; + bool has_stateless_to; bool has_lb_vip; bool has_unknown; bool has_acls; @@ -4759,19 +4761,46 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs) hmap_destroy(nb_pgs); } +static bool +ls_get_acl_flags_for_acl(struct ovn_datapath *od, + const struct nbrec_acl *acl) +{ + if (!strcmp(acl->action, "allow-related")) { + od->has_stateful_acl = true; + if (od->has_stateless_to && od->has_stateless_from) { + return true; + } + } + if (!strcmp(acl->action, "allow-stateless")) { + if (!strcmp(acl->direction, "from-lport")) { + od->has_stateless_from = true; + if (od->has_stateful_acl && od->has_stateless_to) { + return true; + } + } else { + od->has_stateless_to = true; + if (od->has_stateful_acl && od->has_stateless_from) { + return true; + } + } + } + return false; +} + static void ls_get_acl_flags(struct ovn_datapath *od) { od->has_acls = false; od->has_stateful_acl = false; + od->has_stateless_from = false; + od->has_stateless_to = false; if (od->nbs->n_acls) { od->has_acls = true; for (size_t i = 0; i < od->nbs->n_acls; i++) { struct nbrec_acl *acl = od->nbs->acls[i]; - if (!strcmp(acl->action, "allow-related")) { - od->has_stateful_acl = true; + if (ls_get_acl_flags_for_acl(od, acl)) { return; } } @@ -4784,8 +4813,7 @@ ls_get_acl_flags(struct ovn_datapath *od) for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) { struct nbrec_acl *acl = ls_pg->nb_pg->acls[i]; - if (!strcmp(acl->action, "allow-related")) { - od->has_stateful_acl = true; + if (ls_get_acl_flags_for_acl(od, acl)) { return; } } @@ -4984,17 +5012,20 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, } 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 *)) +apply_to_each_acl_of_action_and_direction( + struct ovn_datapath *od, + const struct hmap *port_groups, + struct hmap *lflows, + const char *action, const char *direction, + 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)) { + if (!strcmp(acl->action, action) && + (!direction || !strcmp(acl->direction, direction))) { func(od, acl, lflows); applied = true; } @@ -5005,7 +5036,8 @@ apply_to_each_acl_of_action(struct ovn_datapath *od, 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)) { + if (!strcmp(acl->action, action) && + (!direction || !strcmp(acl->direction, direction))) { func(od, acl, lflows); applied = true; } @@ -5040,8 +5072,9 @@ build_stateless_filters(struct ovn_datapath *od, const struct hmap *port_groups, struct hmap *lflows) { - return apply_to_each_acl_of_action( - od, port_groups, lflows, "allow-stateless", build_stateless_filter); + return apply_to_each_acl_of_action_and_direction( + od, port_groups, lflows, "allow-stateless", NULL, + build_stateless_filter); } static void @@ -5068,17 +5101,33 @@ build_stateful_override_filter(struct ovn_datapath *od, ds_destroy(&match); } +static void +build_stateful_override_filters_for_direction(struct ovn_datapath *od, + const struct hmap *port_groups, + struct hmap *lflows, + const char *direction) +{ + apply_to_each_acl_of_action_and_direction( + od, port_groups, lflows, "allow-related", direction, + build_stateful_override_filter); + apply_to_each_acl_of_action_and_direction( + od, port_groups, lflows, "allow", direction, + build_stateful_override_filter); +} + 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); - apply_to_each_acl_of_action( - od, port_groups, lflows, "allow", - build_stateful_override_filter); + if (od->has_stateless_from) { + build_stateful_override_filters_for_direction( + od, port_groups, lflows, "from-lport"); + } + if (od->has_stateless_to) { + build_stateful_override_filters_for_direction( + od, port_groups, lflows, "to-lport"); + } } static void diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index fc703f62e..8cbc959f0 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -1845,23 +1845,21 @@ 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 ((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(), - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = "ip && ${acl.__match}", - .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 ((sw.has_stateful_acl and acl.action == "allow") or acl.action == "allow-related") { + if (sw.has_stateless_from_acl and 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}", + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", + .external_ids = stage_hint(acl._uuid)) + } else if (sw.has_stateless_to_acl) { + 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)) } } } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 6c38e1a97..1c55310b2 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2664,6 +2664,78 @@ 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 of different direction]) +ovn_start + +# Create two switches to validate direction. We can't use the same switch for +# both ports, otherwise both in and out pipelines are triggered. +ovn-nbctl lr-add r0 +for i in $(seq 1 2); do + check ovn-nbctl --wait=sb ls-add sw$i + + check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i f0:00:00:00:00:0$i 192.168.$i.1/24 + check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0 + check ovn-nbctl --wait=sb lsp-set-type sw$i-r0 router + check ovn-nbctl --wait=sb lsp-set-options sw$i-r0 router-port=r0-sw$i + check ovn-nbctl --wait=sb lsp-set-addresses sw$i-r0 router + + check ovn-nbctl --wait=sb lsp-add sw$i lsp$i + check ovn-nbctl --wait=sb lsp-set-addresses lsp$i "fe:00:00:00:00:0$i 192.168.$i.100/24" +done + +ovn-nbctl acl-add sw1 from-lport 3 tcp allow-stateless +ovn-nbctl acl-add sw1 to-lport 4 tcp allow-related +ovn-nbctl --wait=sb sync + +flow_eth='eth.src == fe:00:00:00:00:01 && eth.dst == f0:00:00:00:00:01' +flow_ip='ip.ttl==64 && ip4.src == 192.168.1.100 && ip4.dst == 192.168.2.100' +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 because allow-related direction is different. +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" +AT_CHECK_UNQUOTED([ovn-trace --minimal sw1 "${flow}"], [0], [dnl +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +ip.ttl--; +eth.src = f0:00:00:00:00:02; +eth.dst = fe:00:00:00:00:02; +output("lsp2"); +]) + +# Add allow-related with the same direction. +ovn-nbctl acl-add sw1 from-lport 4 tcp allow-related +ovn-nbctl --wait=sb sync + +# TCP packets should now go to conntrack. +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +ct_next(ct_state=new|trk) { + ip.ttl--; + eth.src = f0:00:00:00:00:02; + eth.dst = fe:00:00:00:00:02; + output("lsp2"); +}; +]) + +# Reduce priority for allow-related rule of the same direction. +ovn-nbctl acl-del sw1 from-lport 4 tcp +ovn-nbctl acl-add sw1 from-lport 2 tcp allow-related +ovn-nbctl --wait=sb sync + +# TCP packets should no longer go to conntrack +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal sw1 "${flow}"], [0], [dnl +# tcp,reg14=0x2,vlan_tci=0x0000,dl_src=fe:00:00:00:00:01,dl_dst=f0:00:00:00:00:01,nw_src=192.168.1.100,nw_dst=192.168.2.100,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +ip.ttl--; +eth.src = f0:00:00:00:00:02; +eth.dst = fe:00:00:00:00:02; +output("lsp2"); +]) + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) ovn_start
While we *should not* circumvent conntrack when a stateful ACL of higher priority is present on the switch, we should do so only when allow-stateless and allow-stateful directions are the same, otherwise we should still skip conntrack for allow-stateless ACLs. Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> --- northd/lswitch.dl | 88 ++++++++++++++++++++++++++++--------------- northd/ovn-northd.c | 89 ++++++++++++++++++++++++++++++++++---------- northd/ovn_northd.dl | 32 ++++++++-------- tests/ovn-northd.at | 72 +++++++++++++++++++++++++++++++++++ 4 files changed, 213 insertions(+), 68 deletions(-)