On Tue, Jun 1, 2021 at 2:38 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") > > Signed-off-by: Ihar Hrachyshka <[email protected]> >
Thanks Ihar for the update. Each individual patch in this series looks good, but I have some concerns not sorted out overall. Please see my reply here (I replied before I see this new version): https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/383513.html Han > === > > v1: initial commit > v2: document northd flow change > v2: combine two similar ddlog statements > v2: don't map has_fair_meter in ddlog statement. > --- > northd/lswitch.dl | 20 ++++++++ > northd/ovn-northd.8.xml | 4 +- > northd/ovn-northd.c | 91 ++++++++++++++++++++++++++++-------- > northd/ovn_northd.dl | 21 ++++++++- > tests/ovn-northd.at | 100 ++++++++++++++++++++++++++++++++++++++-- > 5 files changed, 210 insertions(+), 26 deletions(-) > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > index 402df48ef..c81b871cf 100644 > --- a/northd/lswitch.dl > +++ b/northd/lswitch.dl > @@ -128,6 +128,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) > @@ -214,6 +231,7 @@ typedef Switch = Switch { > > /* Additional computed fields. */ > has_stateful_acl: bool, > + has_stateless_acl: bool, > has_acls: bool, > has_lb_vip: bool, > has_dns_records: bool, > @@ -250,6 +268,7 @@ Switch[Switch{ > .external_ids = ls.external_ids, > > .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, > @@ -263,6 +282,7 @@ Switch[Switch{ > }.intern()] :- > 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.8.xml b/northd/ovn-northd.8.xml > index 407464602..656534491 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -450,7 +450,9 @@ > priority-110 flow is added to skip over stateful ACLs. IPv6 Neighbor > Discovery and MLD traffic also skips stateful ACLs. For "allow-stateless" > ACLs, a flow is added to bypass setting the hint for connection tracker > - processing. > + processing. If both "allow-stateless" and "allow-related" rules are > + present, rules with appropriate priority are added to set > + <code>reg0[0]</code> for stateful traffic. > </p> > > <p> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 9652ce252..fd57e62e8 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -4989,6 +4989,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, > @@ -5009,28 +5041,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 > @@ -5063,7 +5114,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 cb8418540..a29c9fba8 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -1841,7 +1841,7 @@ for (&Switch(._uuid =ls_uuid)) { > .external_ids = map_empty()) > } > > -for (&SwitchACL(.sw = sw@&Switch{._uuid = ls_uuid}, .acl = &acl, .has_fair_meter = fair_meter)) { > +for (&SwitchACL(.sw = sw@&Switch{._uuid = ls_uuid}, .acl = &acl)) { > if (sw.has_stateful_acl) { > if (acl.action == "allow-stateless") { > if (acl.direction == "from-lport") { > @@ -1860,6 +1860,25 @@ for (&SwitchACL(.sw = sw@&Switch{._uuid = ls_uuid}, .acl = &acl, .has_fair_meter > .external_ids = stage_hint(acl._uuid)) > } > } > + }; > + 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)) > + } > + } > } > } > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 8bd6e48ec..0ff367af6 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2674,6 +2674,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 > @@ -2722,7 +2813,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 > > @@ -2778,7 +2869,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 > > @@ -2827,7 +2918,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' > @@ -2858,7 +2948,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 > > @@ -2914,7 +3004,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
