| Message ID | 20210517214708.3961445-1-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: > > For each allow-stateless ACL, a rule was added earlier in the pipeline > that circumvented setting REGBIT_CONNTRACK_DEFRAG regardless of > whether other, e.g. allow-related ACLs with higher priority were > present. > > Now, when allow-stateless ACLs are present on the switch, for each > allow-related, insert an early pipeline rule that would set DEFRAG bit > for the corresponding match and priority. > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") Thanks for the Fix. I think we need to update northd document for the flow changes. Please also see some inline comments below. > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > --- > northd/lswitch.dl | 20 +++++++++ > northd/ovn-northd.c | 91 +++++++++++++++++++++++++++++++-------- > northd/ovn_northd.dl | 24 ++++++++++- > tests/ovn-northd.at | 100 ++++++++++++++++++++++++++++++++++++++++--- > 4 files changed, 210 insertions(+), 25 deletions(-) > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > index a1aaebb6d..b73cfd047 100644 > --- a/northd/lswitch.dl > +++ b/northd/lswitch.dl > @@ -129,6 +129,23 @@ LogicalSwitchHasStatefulACL(ls, false) :- > nb::Logical_Switch(._uuid = ls), > not LogicalSwitchStatefulACL(ls, _). > > +relation LogicalSwitchStatelessACL(ls: uuid, acl: uuid) > + > +LogicalSwitchStatelessACL(ls, acl) :- > + LogicalSwitchACL(ls, acl), > + nb::ACL(._uuid = acl, .action = "allow-stateless"). > + > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this > +// is an output relation: > +output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) > + > +LogicalSwitchHasStatelessACL(ls, true) :- > + LogicalSwitchStatelessACL(ls, _). > + > +LogicalSwitchHasStatelessACL(ls, false) :- > + nb::Logical_Switch(._uuid = ls), > + not LogicalSwitchStatelessACL(ls, _). > + > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > // is an output relation: > output relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) > @@ -208,6 +225,7 @@ LogicalSwitchHasNonRouterPort(ls, false) :- > relation &Switch( > ls: nb::Logical_Switch, > has_stateful_acl: bool, > + has_stateless_acl: bool, > has_acls: bool, > has_lb_vip: bool, > has_dns_records: bool, > @@ -235,6 +253,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> { > > &Switch(.ls = ls, > .has_stateful_acl = has_stateful_acl, > + .has_stateless_acl = has_stateless_acl, > .has_acls = has_acls, > .has_lb_vip = has_lb_vip, > .has_dns_records = has_dns_records, > @@ -247,6 +266,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> { > .is_vlan_transparent = is_vlan_transparent) :- > nb::Logical_Switch[ls], > LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), > + LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), > LogicalSwitchHasACLs(ls._uuid, has_acls), > LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip), > LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records), > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 7464b7fba..26b723165 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -4983,6 +4983,38 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, > ds_destroy(&match_out); > } > > +static bool > +apply_to_each_acl_of_action(struct ovn_datapath *od, > + const struct hmap *port_groups, > + struct hmap *lflows, const char *action, > + void (*func)(struct ovn_datapath *, > + const struct nbrec_acl *, > + struct hmap *)) > +{ > + bool applied = false; > + for (size_t i = 0; i < od->nbs->n_acls; i++) { > + const struct nbrec_acl *acl = od->nbs->acls[i]; > + if (!strcmp(acl->action, action)) { > + func(od, acl, lflows); > + applied = true; > + } > + } > + > + struct ovn_port_group *pg; > + HMAP_FOR_EACH (pg, key_node, port_groups) { > + if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { > + for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { > + const struct nbrec_acl *acl = pg->nb_pg->acls[i]; > + if (!strcmp(acl->action, action)) { > + func(od, acl, lflows); > + applied = true; > + } > + } > + } > + } > + return applied; > +} > + > static void > build_stateless_filter(struct ovn_datapath *od, > const struct nbrec_acl *acl, > @@ -5003,28 +5035,47 @@ build_stateless_filter(struct ovn_datapath *od, > } > } > > -static void > -build_stateless_filters(struct ovn_datapath *od, struct hmap *port_groups, > +static bool > +build_stateless_filters(struct ovn_datapath *od, > + const struct hmap *port_groups, > struct hmap *lflows) > { > - for (size_t i = 0; i < od->nbs->n_acls; i++) { > - const struct nbrec_acl *acl = od->nbs->acls[i]; > - if (!strcmp(acl->action, "allow-stateless")) { > - build_stateless_filter(od, acl, lflows); > - } > - } > + return apply_to_each_acl_of_action( > + od, port_groups, lflows, "allow-stateless", build_stateless_filter); > +} > > - struct ovn_port_group *pg; > - HMAP_FOR_EACH (pg, key_node, port_groups) { > - if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { > - for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { > - const struct nbrec_acl *acl = pg->nb_pg->acls[i]; > - if (!strcmp(acl->action, "allow-stateless")) { > - build_stateless_filter(od, acl, lflows); > - } > - } > - } > +static void > +build_stateful_override_filter(struct ovn_datapath *od, > + const struct nbrec_acl *acl, > + struct hmap *lflows) > +{ > + struct ds match = DS_EMPTY_INITIALIZER; > + ds_put_format(&match, "ip && %s", acl->match); > + > + if (!strcmp(acl->direction, "from-lport")) { > + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL, > + acl->priority + OVN_ACL_PRI_OFFSET, > + ds_cstr(&match), > + REGBIT_CONNTRACK_DEFRAG" = 1; next;", > + &acl->header_); > + } else { > + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL, > + acl->priority + OVN_ACL_PRI_OFFSET, > + ds_cstr(&match), > + REGBIT_CONNTRACK_DEFRAG" = 1; next;", > + &acl->header_); > } > + ds_destroy(&match); > +} > + > +static void > +build_stateful_override_filters(struct ovn_datapath *od, > + const struct hmap *port_groups, > + struct hmap *lflows) > +{ > + apply_to_each_acl_of_action( > + od, port_groups, lflows, "allow-related", > + build_stateful_override_filter); > } > > static void > @@ -5057,7 +5108,9 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *port_groups, > 110, lflows); > } > > - build_stateless_filters(od, port_groups, lflows); > + if (build_stateless_filters(od, port_groups, lflows)) { > + build_stateful_override_filters(od, port_groups, lflows); > + } > > /* Ingress and Egress Pre-ACL Table (Priority 110). > * > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index 5e1788351..7c2402be4 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -1822,7 +1822,7 @@ for (&Switch(.ls =ls)) { > .external_ids = map_empty()) > } > > -for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = fair_meter)) { > +for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = _)) { Why do we need ".has_fair_meter = _" here if it is not used? > if (sw.has_stateful_acl) { > if (acl.action == "allow-stateless") { > if (acl.direction == "from-lport") { > @@ -1844,6 +1844,28 @@ for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = fair_ > } > } > > +for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = _)) { Maybe it's better to combine this loop with the above one since the loop condition is the same. > + if (sw.has_stateless_acl) { > + if (acl.action == "allow-related") { > + if (acl.direction == "from-lport") { > + Flow(.logical_datapath = ls._uuid, > + .stage = s_SWITCH_IN_PRE_ACL(), > + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > + .__match = "ip && ${acl.__match}", Why need to add "ip &&" in front of the match? If the match has fields indicating it is IP, ovn-controller should add it automatically. Is this an optimization when the match doesn't require the protocol to be IP so when a non-ip packet comes it doesn't need to be directed to CT? Or is it for some other reason? > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > + .external_ids = stage_hint(acl._uuid)) > + } else { > + Flow(.logical_datapath = ls._uuid, > + .stage = s_SWITCH_OUT_PRE_ACL(), > + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > + .__match = "ip && ${acl.__match}", > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > + .external_ids = stage_hint(acl._uuid)) > + } > + } > + } > +} > + > /* If there are any stateful ACL rules in this datapath, we must > * send all IP packets through the conntrack action, which handles > * defragmentation, in order to match L4 headers. */ > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 6c2745ed1..da75434b6 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2664,6 +2664,97 @@ sed 's/reg8\[[0..15\]] == [[0-9]]*/reg8\[[0..15\]] == <cleared>/' | sort], [0], > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) > +ovn_start > + > +ovn-nbctl ls-add ls > +ovn-nbctl lsp-add ls lsp1 > +ovn-nbctl lsp-set-addresses lsp1 00:00:00:00:00:01 > +ovn-nbctl lsp-add ls lsp2 > +ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02 > + > +for direction in from to; do > + ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow-related > +done > +ovn-nbctl --wait=sb sync > + > +flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02' > +flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66' > +flow_tcp='tcp && tcp.dst == 80' > +flow_udp='udp && udp.dst == 80' > +lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) > + > +# TCP packets should go to conntrack. > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +ct_next(ct_state=new|trk) { > + ct_next(ct_state=new|trk) { > + output("lsp2"); > + }; > +}; > +]) > + > +# Add allow-stateless with lower priority. > +for direction in from to; do > + ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless > +done > +ovn-nbctl --wait=sb sync > + > +# TCP packets should still go to conntrack. > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +ct_next(ct_state=new|trk) { > + ct_next(ct_state=new|trk) { > + output("lsp2"); > + }; > +}; > +]) > + > +# Add another allow-stateless with higher priority. > +for direction in from to; do > + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless > +done > +ovn-nbctl --wait=sb sync > + > +# TCP packets should no longer go to conntrack > +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +output("lsp2"); > +]) > + > +# Remove the higher priority allow-stateless. > +for direction in from to; do > + ovn-nbctl acl-del ls ${direction}-lport 5 tcp > +done > +ovn-nbctl --wait=sb sync > + > +# TCP packets should again go to conntrack. > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +ct_next(ct_state=new|trk) { > + ct_next(ct_state=new|trk) { > + output("lsp2"); > + }; > +}; > +]) > + > +# Remove the allow-related ACL. > +for direction in from to; do > + ovn-nbctl acl-del ls ${direction}-lport 3 tcp > +done > +ovn-nbctl --wait=sb sync > + > +# TCP packets should no longer go to conntrack > +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +output("lsp2"); > +]) > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([ovn -- ACL allow-stateless omit conntrack - Logical_Switch]) > ovn_start > @@ -2712,7 +2803,7 @@ ct_next(ct_state=new|trk) { > > # Allow stateless for TCP. > for direction in from to; do > - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless > + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless > done > ovn-nbctl --wait=sb sync > > @@ -2768,7 +2859,7 @@ ct_lb { > > # Allow stateless for TCP. > for direction in from to; do > - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless > + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless > done > ovn-nbctl --wait=sb sync > > @@ -2817,7 +2908,6 @@ done > ovn-nbctl --wait=sb sync > > lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) > -echo $lsp1_inport > > flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02' > flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66' > @@ -2848,7 +2938,7 @@ ct_next(ct_state=new|trk) { > > # Allow stateless for TCP. > for direction in from to; do > - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless > + ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless > done > ovn-nbctl --wait=sb sync > > @@ -2904,7 +2994,7 @@ ct_lb { > > # Allow stateless for TCP. > for direction in from to; do > - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless > + ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless > done > ovn-nbctl --wait=sb sync > > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, May 20, 2021 at 5:07 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > > > For each allow-stateless ACL, a rule was added earlier in the pipeline > > that circumvented setting REGBIT_CONNTRACK_DEFRAG regardless of > > whether other, e.g. allow-related ACLs with higher priority were > > present. > > > > Now, when allow-stateless ACLs are present on the switch, for each > > allow-related, insert an early pipeline rule that would set DEFRAG bit > > for the corresponding match and priority. > > > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") > > Thanks for the Fix. I think we need to update northd document for the flow changes. Please also see some inline comments below. Docs updated. > > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > > --- > > northd/lswitch.dl | 20 +++++++++ > > northd/ovn-northd.c | 91 +++++++++++++++++++++++++++++++-------- > > northd/ovn_northd.dl | 24 ++++++++++- > > tests/ovn-northd.at | 100 ++++++++++++++++++++++++++++++++++++++++--- > > 4 files changed, 210 insertions(+), 25 deletions(-) > > > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > > index a1aaebb6d..b73cfd047 100644 > > --- a/northd/lswitch.dl > > +++ b/northd/lswitch.dl > > @@ -129,6 +129,23 @@ LogicalSwitchHasStatefulACL(ls, false) :- > > nb::Logical_Switch(._uuid = ls), > > not LogicalSwitchStatefulACL(ls, _). > > > > +relation LogicalSwitchStatelessACL(ls: uuid, acl: uuid) > > + > > +LogicalSwitchStatelessACL(ls, acl) :- > > + LogicalSwitchACL(ls, acl), > > + nb::ACL(._uuid = acl, .action = "allow-stateless"). > > + > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > +// is an output relation: > > +output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) > > + > > +LogicalSwitchHasStatelessACL(ls, true) :- > > + LogicalSwitchStatelessACL(ls, _). > > + > > +LogicalSwitchHasStatelessACL(ls, false) :- > > + nb::Logical_Switch(._uuid = ls), > > + not LogicalSwitchStatelessACL(ls, _). > > + > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this > > // is an output relation: > > output relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) > > @@ -208,6 +225,7 @@ LogicalSwitchHasNonRouterPort(ls, false) :- > > relation &Switch( > > ls: nb::Logical_Switch, > > has_stateful_acl: bool, > > + has_stateless_acl: bool, > > has_acls: bool, > > has_lb_vip: bool, > > has_dns_records: bool, > > @@ -235,6 +253,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> { > > > > &Switch(.ls = ls, > > .has_stateful_acl = has_stateful_acl, > > + .has_stateless_acl = has_stateless_acl, > > .has_acls = has_acls, > > .has_lb_vip = has_lb_vip, > > .has_dns_records = has_dns_records, > > @@ -247,6 +266,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> { > > .is_vlan_transparent = is_vlan_transparent) :- > > nb::Logical_Switch[ls], > > LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), > > + LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), > > LogicalSwitchHasACLs(ls._uuid, has_acls), > > LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip), > > LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records), > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 7464b7fba..26b723165 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -4983,6 +4983,38 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, > > ds_destroy(&match_out); > > } > > > > +static bool > > +apply_to_each_acl_of_action(struct ovn_datapath *od, > > + const struct hmap *port_groups, > > + struct hmap *lflows, const char *action, > > + void (*func)(struct ovn_datapath *, > > + const struct nbrec_acl *, > > + struct hmap *)) > > +{ > > + bool applied = false; > > + for (size_t i = 0; i < od->nbs->n_acls; i++) { > > + const struct nbrec_acl *acl = od->nbs->acls[i]; > > + if (!strcmp(acl->action, action)) { > > + func(od, acl, lflows); > > + applied = true; > > + } > > + } > > + > > + struct ovn_port_group *pg; > > + HMAP_FOR_EACH (pg, key_node, port_groups) { > > + if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { > > + for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { > > + const struct nbrec_acl *acl = pg->nb_pg->acls[i]; > > + if (!strcmp(acl->action, action)) { > > + func(od, acl, lflows); > > + applied = true; > > + } > > + } > > + } > > + } > > + return applied; > > +} > > + > > static void > > build_stateless_filter(struct ovn_datapath *od, > > const struct nbrec_acl *acl, > > @@ -5003,28 +5035,47 @@ build_stateless_filter(struct ovn_datapath *od, > > } > > } > > > > -static void > > -build_stateless_filters(struct ovn_datapath *od, struct hmap *port_groups, > > +static bool > > +build_stateless_filters(struct ovn_datapath *od, > > + const struct hmap *port_groups, > > struct hmap *lflows) > > { > > - for (size_t i = 0; i < od->nbs->n_acls; i++) { > > - const struct nbrec_acl *acl = od->nbs->acls[i]; > > - if (!strcmp(acl->action, "allow-stateless")) { > > - build_stateless_filter(od, acl, lflows); > > - } > > - } > > + return apply_to_each_acl_of_action( > > + od, port_groups, lflows, "allow-stateless", build_stateless_filter); > > +} > > > > - struct ovn_port_group *pg; > > - HMAP_FOR_EACH (pg, key_node, port_groups) { > > - if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { > > - for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { > > - const struct nbrec_acl *acl = pg->nb_pg->acls[i]; > > - if (!strcmp(acl->action, "allow-stateless")) { > > - build_stateless_filter(od, acl, lflows); > > - } > > - } > > - } > > +static void > > +build_stateful_override_filter(struct ovn_datapath *od, > > + const struct nbrec_acl *acl, > > + struct hmap *lflows) > > +{ > > + struct ds match = DS_EMPTY_INITIALIZER; > > + ds_put_format(&match, "ip && %s", acl->match); > > + > > + if (!strcmp(acl->direction, "from-lport")) { > > + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL, > > + acl->priority + OVN_ACL_PRI_OFFSET, > > + ds_cstr(&match), > > + REGBIT_CONNTRACK_DEFRAG" = 1; next;", > > + &acl->header_); > > + } else { > > + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL, > > + acl->priority + OVN_ACL_PRI_OFFSET, > > + ds_cstr(&match), > > + REGBIT_CONNTRACK_DEFRAG" = 1; next;", > > + &acl->header_); > > } > > + ds_destroy(&match); > > +} > > + > > +static void > > +build_stateful_override_filters(struct ovn_datapath *od, > > + const struct hmap *port_groups, > > + struct hmap *lflows) > > +{ > > + apply_to_each_acl_of_action( > > + od, port_groups, lflows, "allow-related", > > + build_stateful_override_filter); > > } > > > > static void > > @@ -5057,7 +5108,9 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *port_groups, > > 110, lflows); > > } > > > > - build_stateless_filters(od, port_groups, lflows); > > + if (build_stateless_filters(od, port_groups, lflows)) { > > + build_stateful_override_filters(od, port_groups, lflows); > > + } > > > > /* Ingress and Egress Pre-ACL Table (Priority 110). > > * > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > > index 5e1788351..7c2402be4 100644 > > --- a/northd/ovn_northd.dl > > +++ b/northd/ovn_northd.dl > > @@ -1822,7 +1822,7 @@ for (&Switch(.ls =ls)) { > > .external_ids = map_empty()) > > } > > > > -for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = fair_meter)) { > > +for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = _)) { > > Why do we need ".has_fair_meter = _" here if it is not used? > This is just me not knowing too much about ddlog; I removed the attribute mapping and it still works. > > if (sw.has_stateful_acl) { > > if (acl.action == "allow-stateless") { > > if (acl.direction == "from-lport") { > > @@ -1844,6 +1844,28 @@ for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = fair_ > > } > > } > > > > +for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = _)) { > > Maybe it's better to combine this loop with the above one since the loop condition is the same. I will do so though I am not sure ddlog code generator will make any difference. > > > + if (sw.has_stateless_acl) { > > + if (acl.action == "allow-related") { > > + if (acl.direction == "from-lport") { > > + Flow(.logical_datapath = ls._uuid, > > + .stage = s_SWITCH_IN_PRE_ACL(), > > + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > + .__match = "ip && ${acl.__match}", > > Why need to add "ip &&" in front of the match? If the match has fields indicating it is IP, ovn-controller should add it automatically. Is this an optimization when the match doesn't require the protocol to be IP so when a non-ip packet comes it doesn't need to be directed to CT? Or is it for some other reason? This is because we already send to conntrack only when traffic matches "ip" (in the general rule that sends everything there unless circumvented by stateless rule): ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 100, "ip", REGBIT_CONNTRACK_DEFRAG" = 1; next;"); ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 100, "ip", REGBIT_CONNTRACK_DEFRAG" = 1; next;"); > > > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > > + .external_ids = stage_hint(acl._uuid)) > > + } else { > > + Flow(.logical_datapath = ls._uuid, > > + .stage = s_SWITCH_OUT_PRE_ACL(), > > + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > + .__match = "ip && ${acl.__match}", > > + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", > > + .external_ids = stage_hint(acl._uuid)) > > + } > > + } > > + } > > +} > > + > > /* If there are any stateful ACL rules in this datapath, we must > > * send all IP packets through the conntrack action, which handles > > * defragmentation, in order to match L4 headers. */ > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 6c2745ed1..da75434b6 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -2664,6 +2664,97 @@ sed 's/reg8\[[0..15\]] == [[0-9]]*/reg8\[[0..15\]] == <cleared>/' | sort], [0], > > AT_CLEANUP > > ]) > > > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) > > +ovn_start > > + > > +ovn-nbctl ls-add ls > > +ovn-nbctl lsp-add ls lsp1 > > +ovn-nbctl lsp-set-addresses lsp1 00:00:00:00:00:01 > > +ovn-nbctl lsp-add ls lsp2 > > +ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02 > > + > > +for direction in from to; do > > + ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow-related > > +done > > +ovn-nbctl --wait=sb sync > > + > > +flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02' > > +flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66' > > +flow_tcp='tcp && tcp.dst == 80' > > +flow_udp='udp && udp.dst == 80' > > +lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) > > + > > +# TCP packets should go to conntrack. > > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +ct_next(ct_state=new|trk) { > > + ct_next(ct_state=new|trk) { > > + output("lsp2"); > > + }; > > +}; > > +]) > > + > > +# Add allow-stateless with lower priority. > > +for direction in from to; do > > + ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless > > +done > > +ovn-nbctl --wait=sb sync > > + > > +# TCP packets should still go to conntrack. > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +ct_next(ct_state=new|trk) { > > + ct_next(ct_state=new|trk) { > > + output("lsp2"); > > + }; > > +}; > > +]) > > + > > +# Add another allow-stateless with higher priority. > > +for direction in from to; do > > + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless > > +done > > +ovn-nbctl --wait=sb sync > > + > > +# TCP packets should no longer go to conntrack > > +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl > > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +output("lsp2"); > > +]) > > + > > +# Remove the higher priority allow-stateless. > > +for direction in from to; do > > + ovn-nbctl acl-del ls ${direction}-lport 5 tcp > > +done > > +ovn-nbctl --wait=sb sync > > + > > +# TCP packets should again go to conntrack. > > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +ct_next(ct_state=new|trk) { > > + ct_next(ct_state=new|trk) { > > + output("lsp2"); > > + }; > > +}; > > +]) > > + > > +# Remove the allow-related ACL. > > +for direction in from to; do > > + ovn-nbctl acl-del ls ${direction}-lport 3 tcp > > +done > > +ovn-nbctl --wait=sb sync > > + > > +# TCP packets should no longer go to conntrack > > +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl > > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > > +output("lsp2"); > > +]) > > + > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([ovn -- ACL allow-stateless omit conntrack - Logical_Switch]) > > ovn_start > > @@ -2712,7 +2803,7 @@ ct_next(ct_state=new|trk) { > > > > # Allow stateless for TCP. > > for direction in from to; do > > - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless > > + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless > > done > > ovn-nbctl --wait=sb sync > > > > @@ -2768,7 +2859,7 @@ ct_lb { > > > > # Allow stateless for TCP. > > for direction in from to; do > > - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless > > + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless > > done > > ovn-nbctl --wait=sb sync > > > > @@ -2817,7 +2908,6 @@ done > > ovn-nbctl --wait=sb sync > > > > lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) > > -echo $lsp1_inport > > > > flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02' > > flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66' > > @@ -2848,7 +2938,7 @@ ct_next(ct_state=new|trk) { > > > > # Allow stateless for TCP. > > for direction in from to; do > > - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless > > + ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless > > done > > ovn-nbctl --wait=sb sync > > > > @@ -2904,7 +2994,7 @@ ct_lb { > > > > # Allow stateless for TCP. > > for direction in from to; do > > - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless > > + ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless > > done > > ovn-nbctl --wait=sb sync > > > > -- > > 2.31.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/northd/lswitch.dl b/northd/lswitch.dl index a1aaebb6d..b73cfd047 100644 --- a/northd/lswitch.dl +++ b/northd/lswitch.dl @@ -129,6 +129,23 @@ LogicalSwitchHasStatefulACL(ls, false) :- nb::Logical_Switch(._uuid = ls), not LogicalSwitchStatefulACL(ls, _). +relation LogicalSwitchStatelessACL(ls: uuid, acl: uuid) + +LogicalSwitchStatelessACL(ls, acl) :- + LogicalSwitchACL(ls, acl), + nb::ACL(._uuid = acl, .action = "allow-stateless"). + +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this +// is an output relation: +output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool) + +LogicalSwitchHasStatelessACL(ls, true) :- + LogicalSwitchStatelessACL(ls, _). + +LogicalSwitchHasStatelessACL(ls, false) :- + nb::Logical_Switch(._uuid = ls), + not LogicalSwitchStatelessACL(ls, _). + // "Pitfalls of projections" in ddlog-new-feature.rst explains why this // is an output relation: output relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool) @@ -208,6 +225,7 @@ LogicalSwitchHasNonRouterPort(ls, false) :- relation &Switch( ls: nb::Logical_Switch, has_stateful_acl: bool, + has_stateless_acl: bool, has_acls: bool, has_lb_vip: bool, has_dns_records: bool, @@ -235,6 +253,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> { &Switch(.ls = ls, .has_stateful_acl = has_stateful_acl, + .has_stateless_acl = has_stateless_acl, .has_acls = has_acls, .has_lb_vip = has_lb_vip, .has_dns_records = has_dns_records, @@ -247,6 +266,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> { .is_vlan_transparent = is_vlan_transparent) :- nb::Logical_Switch[ls], LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), + LogicalSwitchHasStatelessACL(ls._uuid, has_stateless_acl), LogicalSwitchHasACLs(ls._uuid, has_acls), LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip), LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records), diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 7464b7fba..26b723165 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -4983,6 +4983,38 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, ds_destroy(&match_out); } +static bool +apply_to_each_acl_of_action(struct ovn_datapath *od, + const struct hmap *port_groups, + struct hmap *lflows, const char *action, + void (*func)(struct ovn_datapath *, + const struct nbrec_acl *, + struct hmap *)) +{ + bool applied = false; + for (size_t i = 0; i < od->nbs->n_acls; i++) { + const struct nbrec_acl *acl = od->nbs->acls[i]; + if (!strcmp(acl->action, action)) { + func(od, acl, lflows); + applied = true; + } + } + + struct ovn_port_group *pg; + HMAP_FOR_EACH (pg, key_node, port_groups) { + if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { + for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { + const struct nbrec_acl *acl = pg->nb_pg->acls[i]; + if (!strcmp(acl->action, action)) { + func(od, acl, lflows); + applied = true; + } + } + } + } + return applied; +} + static void build_stateless_filter(struct ovn_datapath *od, const struct nbrec_acl *acl, @@ -5003,28 +5035,47 @@ build_stateless_filter(struct ovn_datapath *od, } } -static void -build_stateless_filters(struct ovn_datapath *od, struct hmap *port_groups, +static bool +build_stateless_filters(struct ovn_datapath *od, + const struct hmap *port_groups, struct hmap *lflows) { - for (size_t i = 0; i < od->nbs->n_acls; i++) { - const struct nbrec_acl *acl = od->nbs->acls[i]; - if (!strcmp(acl->action, "allow-stateless")) { - build_stateless_filter(od, acl, lflows); - } - } + return apply_to_each_acl_of_action( + od, port_groups, lflows, "allow-stateless", build_stateless_filter); +} - struct ovn_port_group *pg; - HMAP_FOR_EACH (pg, key_node, port_groups) { - if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { - for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { - const struct nbrec_acl *acl = pg->nb_pg->acls[i]; - if (!strcmp(acl->action, "allow-stateless")) { - build_stateless_filter(od, acl, lflows); - } - } - } +static void +build_stateful_override_filter(struct ovn_datapath *od, + const struct nbrec_acl *acl, + struct hmap *lflows) +{ + struct ds match = DS_EMPTY_INITIALIZER; + ds_put_format(&match, "ip && %s", acl->match); + + if (!strcmp(acl->direction, "from-lport")) { + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL, + acl->priority + OVN_ACL_PRI_OFFSET, + ds_cstr(&match), + REGBIT_CONNTRACK_DEFRAG" = 1; next;", + &acl->header_); + } else { + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL, + acl->priority + OVN_ACL_PRI_OFFSET, + ds_cstr(&match), + REGBIT_CONNTRACK_DEFRAG" = 1; next;", + &acl->header_); } + ds_destroy(&match); +} + +static void +build_stateful_override_filters(struct ovn_datapath *od, + const struct hmap *port_groups, + struct hmap *lflows) +{ + apply_to_each_acl_of_action( + od, port_groups, lflows, "allow-related", + build_stateful_override_filter); } static void @@ -5057,7 +5108,9 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *port_groups, 110, lflows); } - build_stateless_filters(od, port_groups, lflows); + if (build_stateless_filters(od, port_groups, lflows)) { + build_stateful_override_filters(od, port_groups, lflows); + } /* Ingress and Egress Pre-ACL Table (Priority 110). * diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index 5e1788351..7c2402be4 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -1822,7 +1822,7 @@ for (&Switch(.ls =ls)) { .external_ids = map_empty()) } -for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = fair_meter)) { +for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = _)) { if (sw.has_stateful_acl) { if (acl.action == "allow-stateless") { if (acl.direction == "from-lport") { @@ -1844,6 +1844,28 @@ for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = fair_ } } +for (&SwitchACL(.sw = sw@&Switch{.ls = ls}, .acl = &acl, .has_fair_meter = _)) { + if (sw.has_stateless_acl) { + if (acl.action == "allow-related") { + if (acl.direction == "from-lport") { + Flow(.logical_datapath = ls._uuid, + .stage = s_SWITCH_IN_PRE_ACL(), + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), + .__match = "ip && ${acl.__match}", + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", + .external_ids = stage_hint(acl._uuid)) + } else { + Flow(.logical_datapath = ls._uuid, + .stage = s_SWITCH_OUT_PRE_ACL(), + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), + .__match = "ip && ${acl.__match}", + .actions = "${rEGBIT_CONNTRACK_DEFRAG()} = 1; next;", + .external_ids = stage_hint(acl._uuid)) + } + } + } +} + /* If there are any stateful ACL rules in this datapath, we must * send all IP packets through the conntrack action, which handles * defragmentation, in order to match L4 headers. */ diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 6c2745ed1..da75434b6 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2664,6 +2664,97 @@ sed 's/reg8\[[0..15\]] == [[0-9]]*/reg8\[[0..15\]] == <cleared>/' | sort], [0], AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn -- ACL priority: allow-stateless vs allow-related]) +ovn_start + +ovn-nbctl ls-add ls +ovn-nbctl lsp-add ls lsp1 +ovn-nbctl lsp-set-addresses lsp1 00:00:00:00:00:01 +ovn-nbctl lsp-add ls lsp2 +ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02 + +for direction in from to; do + ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow-related +done +ovn-nbctl --wait=sb sync + +flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02' +flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66' +flow_tcp='tcp && tcp.dst == 80' +flow_udp='udp && udp.dst == 80' +lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) + +# TCP packets should go to conntrack. +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +ct_next(ct_state=new|trk) { + ct_next(ct_state=new|trk) { + output("lsp2"); + }; +}; +]) + +# Add allow-stateless with lower priority. +for direction in from to; do + ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless +done +ovn-nbctl --wait=sb sync + +# TCP packets should still go to conntrack. +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +ct_next(ct_state=new|trk) { + ct_next(ct_state=new|trk) { + output("lsp2"); + }; +}; +]) + +# Add another allow-stateless with higher priority. +for direction in from to; do + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless +done +ovn-nbctl --wait=sb sync + +# TCP packets should no longer go to conntrack +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +output("lsp2"); +]) + +# Remove the higher priority allow-stateless. +for direction in from to; do + ovn-nbctl acl-del ls ${direction}-lport 5 tcp +done +ovn-nbctl --wait=sb sync + +# TCP packets should again go to conntrack. +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +ct_next(ct_state=new|trk) { + ct_next(ct_state=new|trk) { + output("lsp2"); + }; +}; +]) + +# Remove the allow-related ACL. +for direction in from to; do + ovn-nbctl acl-del ls ${direction}-lport 3 tcp +done +ovn-nbctl --wait=sb sync + +# TCP packets should no longer go to conntrack +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +output("lsp2"); +]) + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([ovn -- ACL allow-stateless omit conntrack - Logical_Switch]) ovn_start @@ -2712,7 +2803,7 @@ ct_next(ct_state=new|trk) { # Allow stateless for TCP. for direction in from to; do - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless done ovn-nbctl --wait=sb sync @@ -2768,7 +2859,7 @@ ct_lb { # Allow stateless for TCP. for direction in from to; do - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add ls ${direction}-lport 5 tcp allow-stateless done ovn-nbctl --wait=sb sync @@ -2817,7 +2908,6 @@ done ovn-nbctl --wait=sb sync lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) -echo $lsp1_inport flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02' flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66' @@ -2848,7 +2938,7 @@ ct_next(ct_state=new|trk) { # Allow stateless for TCP. for direction in from to; do - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless done ovn-nbctl --wait=sb sync @@ -2904,7 +2994,7 @@ ct_lb { # Allow stateless for TCP. for direction in from to; do - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add pg ${direction}-lport 5 tcp allow-stateless done ovn-nbctl --wait=sb sync
For each allow-stateless ACL, a rule was added earlier in the pipeline that circumvented setting REGBIT_CONNTRACK_DEFRAG regardless of whether other, e.g. allow-related ACLs with higher priority were present. Now, when allow-stateless ACLs are present on the switch, for each allow-related, insert an early pipeline rule that would set DEFRAG bit for the corresponding match and priority. Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb") Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> --- northd/lswitch.dl | 20 +++++++++ northd/ovn-northd.c | 91 +++++++++++++++++++++++++++++++-------- northd/ovn_northd.dl | 24 ++++++++++- tests/ovn-northd.at | 100 ++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 210 insertions(+), 25 deletions(-)