[ovs-dev] [PATCH ovn 3/3] Honor ACL direction when omitting ct for stateless
Ihar Hrachyshka
ihrachys at redhat.com
Tue Jun 8 00:06:08 UTC 2021
On Wed, Jun 2, 2021 at 12:22 AM Han Zhou <hzhou at ovn.org> wrote:
>
>
>
> On Tue, Jun 1, 2021 at 12:28 PM Ihar Hrachyshka <ihrachys at redhat.com> wrote:
> >
> > On Thu, May 20, 2021 at 9:55 PM Han Zhou <hzhou at ovn.org> wrote:
> > >
> > >
> > >
> > > On Thu, May 20, 2021 at 3:22 PM Han Zhou <hzhou at ovn.org> wrote:
> > > >
> > > >
> > > >
> > > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrachys at 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 at 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.
>
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 at openvswitch.org
> > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
More information about the dev
mailing list