On Thu, May 20, 2021 at 5:07 PM Han Zhou <[email protected]> wrote: > > > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka <[email protected]> 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 <[email protected]> > > --- > > 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
