On 8/27/20 9:04 PM, Han Zhou wrote: > > > On Thu, Aug 27, 2020 at 12:24 AM Dumitru Ceara <[email protected] > <mailto:[email protected]>> wrote: >> >> On 8/27/20 1:39 AM, Han Zhou wrote: >> > Hi Dumitru, >> > >> > Please see my comments below. >> > >> > Thanks, >> > Han >> >> Hi Han, >> >> Thanks for your review. >> >> > >> > On Thu, Aug 20, 2020 at 4:19 AM Dumitru Ceara <[email protected] > <mailto:[email protected]> >> > <mailto:[email protected] <mailto:[email protected]>>> wrote: >> >> >> >> A new table is added to OVN_Northbound: Stateless_Filter. Users can >> >> populate this table with records consisting of <priority, match>. These >> >> records generate logical flows in the PRE_ACL stages of the logical >> >> switch pipeline. >> >> >> >> Packets matching these flows will completely bypass connection tracking >> >> for ACL purposes. In specific scenarios CMSs can predetermine which >> >> traffic must be firewalled statefully or not, e.g., UDP vs TCP. > However, >> >> until now, if at least one stateful ACL (allow-related) is configured >> >> on the switch, all traffic gets sent to connection tracking. >> >> This induces a hit in performance when forwarding packets that don't >> >> need stateful processing. >> >> >> >> New command line arguments are added to ovn-nbctl (stateless-filter-*) >> >> to allow the users to interact with the Stateless_Filter table. >> >> >> >> Signed-off-by: Dumitru Ceara <[email protected] > <mailto:[email protected]> >> > <mailto:[email protected] <mailto:[email protected]>>> >> >> --- >> >> V2: >> >> - address Numan's comments: >> >> - fix spacing in the logical flow match. >> >> - add a new table to the NB DB instead of using a config option > on the >> >> logical switch. >> >> - add ovn-nbctl CLI commands for the new table and also unit tests for >> >> them. >> >> - reword the commit message. >> >> NOTE: checkpatch.py will complain about lines lacking whitespacec > around >> >> operators in the ovn-nbctl help string but this is a false positive and >> >> should be ignored. >> >> --- >> >> NEWS | 3 + >> >> northd/ovn-northd.8.xml | 20 ++++ >> >> northd/ovn-northd.c | 146 ++++++++++++++++++----- >> >> ovn-nb.ovsschema | 26 ++++- >> >> ovn-nb.xml | 57 ++++++++- >> >> tests/ovn-nbctl.at <http://ovn-nbctl.at> <http://ovn-nbctl.at> > | 53 +++++++++ >> >> tests/ovn-northd.at <http://ovn-northd.at> <http://ovn-northd.at> > | 263 >> > ++++++++++++++++++++++++++++++++++++++++++ >> >> tests/system-common-macros.at <http://system-common-macros.at> > <http://system-common-macros.at> | 8 ++ >> >> tests/system-ovn.at <http://system-ovn.at> <http://system-ovn.at> > | 113 >> > ++++++++++++++++++ >> >> utilities/ovn-detrace.in <http://ovn-detrace.in> > <http://ovn-detrace.in> | 12 ++ >> >> utilities/ovn-nbctl.c | 213 ++++++++++++++++++++++++++++++++-- >> >> 11 files changed, 871 insertions(+), 43 deletions(-) >> >> >> >> diff --git a/NEWS b/NEWS >> >> index a1ce4e8..eedd091 100644 >> >> --- a/NEWS >> >> +++ b/NEWS >> >> @@ -11,6 +11,9 @@ Post-v20.06.0 >> >> called Chassis_Private now contains the nb_cfg column which is >> > updated >> >> by incrementing the value in the NB_Global table, CMSes > relying on >> >> this mechanism should update their code to use this new table. >> >> + - Added support for bypassing connection tracking for ACL >> > processing for >> >> + specific types of traffic through the user supplied > Stateless_Filter >> >> + configuration. >> >> >> >> OVN v20.06.0 >> >> -------------------------- >> >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml >> >> index 989e364..1f89942 100644 >> >> --- a/northd/ovn-northd.8.xml >> >> +++ b/northd/ovn-northd.8.xml >> >> @@ -322,6 +322,16 @@ >> >> </p> >> >> >> >> <p> >> >> + For each record in table <code>Stateful_Filter</code> in the >> >> + <code>OVN_Northbound</code> database, a flow with >> >> + <code>priority + 1000</code> is added and sets <code>reg0[7] = >> > 1</code> >> >> + for traffic that matches the condition in the <code>match</code> >> >> + column and advances to next table. <code>reg0[7]</code> acts >> > as a hint >> >> + for tables <code>Pre-Stateful</code> and <code>ACL</code> to > avoid >> >> + sending this traffic to the connection tracker. >> >> + </p> >> >> + >> > >> > It seems documentation is missing for the flows that uses reg0[7] in >> > Pre-Stateful and ACL stages. >> > >> >> Ack, I'll add the documentation for ACL stage but in Pre-Stateful I >> didn't use reg0[7] (REGBIT_SKIP_ACL_CT). >> >> >> + <p> >> >> This table also has a priority-110 flow with the match >> >> <code>eth.dst == <var>E</var></code> for all logical switch >> >> datapaths to move traffic to the next table. Where <var>E</var> >> >> @@ -1383,6 +1393,16 @@ output; >> >> </p> >> >> >> >> <p> >> >> + For each record in table <code>Stateful_Filter</code> in the >> >> + <code>OVN_Northbound</code> database, a flow with >> >> + <code>priority + 1000</code> is added and sets <code>reg0[7] = >> > 1</code> >> >> + for traffic that matches the condition in the <code>match</code> >> >> + column and advances to next table. <code>reg0[7]</code> acts >> > as a hint >> >> + for tables <code>Pre-Stateful</code> and <code>ACL</code> to > avoid >> >> + sending this traffic to the connection tracker. >> >> + </p> >> >> + >> >> + <p> >> >> This table also has a priority-110 flow with the match >> >> <code>eth.src == <var>E</var></code> for all logical switch >> >> datapaths to move traffic to the next table. Where <var>E</var> >> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> >> index 212de2f..b8f457b 100644 >> >> --- a/northd/ovn-northd.c >> >> +++ b/northd/ovn-northd.c >> >> @@ -211,6 +211,7 @@ enum ovn_stage { >> >> #define REGBIT_DNS_LOOKUP_RESULT "reg0[4]" >> >> #define REGBIT_ND_RA_OPTS_RESULT "reg0[5]" >> >> #define REGBIT_HAIRPIN "reg0[6]" >> >> +#define REGBIT_SKIP_ACL_CT "reg0[7]" >> >> >> >> /* Register definitions for switches and routers. */ >> >> >> >> @@ -245,11 +246,11 @@ enum ovn_stage { >> >> * OVS register usage: >> >> * >> >> * Logical Switch pipeline: >> >> - * +---------+-------------------------------------+ >> >> - * | R0 | REGBIT_{CONNTRACK/DHCP/DNS/HAIRPIN} | >> >> - * +---------+-------------------------------------+ >> >> - * | R1 - R9 | UNUSED | >> >> - * +---------+-------------------------------------+ >> >> + * +---------+-------------------------------------------------+ >> >> + * | R0 | REGBIT_{CONNTRACK/DHCP/DNS/HAIRPIN/SKIP_ACL_CT} | >> >> + * +---------+-------------------------------------------------+ >> >> + * | R1 - R9 | UNUSED | >> >> + * +---------+-------------------------------------------------+ >> >> * >> >> * Logical Router pipeline: >> >> * >> > > +-----+--------------------------+---+-----------------+---+---------------+ >> >> @@ -4713,6 +4714,12 @@ has_stateful_acl(struct ovn_datapath *od) >> >> return false; >> >> } >> >> >> >> +static bool >> >> +has_stateful_acl_bypass(struct ovn_datapath *od) >> >> +{ >> >> + return od->nbs->n_stateless_filters > 0; >> >> +} >> >> + >> >> static void >> >> build_lswitch_input_port_sec(struct hmap *ports, struct hmap > *datapaths, >> >> struct hmap *lflows) >> >> @@ -4881,7 +4888,47 @@ skip_port_from_conntrack(struct ovn_datapath >> > *od, struct ovn_port *op, >> >> } >> >> >> >> static void >> >> -build_pre_acls(struct ovn_datapath *od, struct hmap *lflows) >> >> +build_stateless_filter(struct ovn_datapath *od, >> >> + const struct nbrec_stateless_filter *filter, >> >> + struct hmap *lflows) >> >> +{ >> >> + /* Stateless filters must be applied in both directions so > that reply >> >> + * traffic bypasses conntrack too. >> >> + */ >> >> + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL, >> >> + filter->priority + OVN_ACL_PRI_OFFSET, >> >> + filter->match, >> >> + REGBIT_SKIP_ACL_CT" = 1; next;", >> >> + &filter->header_); >> >> + ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL, >> >> + filter->priority + OVN_ACL_PRI_OFFSET, >> >> + filter->match, >> >> + REGBIT_SKIP_ACL_CT" = 1; next;", >> >> + &filter->header_); >> >> +} >> >> + >> >> +static void >> >> +build_stateless_filters(struct ovn_datapath *od, struct hmap >> > *port_groups, >> >> + struct hmap *lflows) >> >> +{ >> >> + for (size_t i = 0; i < od->nbs->n_stateless_filters; i++) { >> >> + build_stateless_filter(od, od->nbs->stateless_filters[i], >> > lflows); >> >> + } >> >> + >> >> + 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_stateless_filters; > i++) { >> >> + build_stateless_filter(od, >> > pg->nb_pg->stateless_filters[i], >> >> + lflows); >> >> + } >> >> + } >> >> + } >> >> +} >> >> + >> >> +static void >> >> +build_pre_acls(struct ovn_datapath *od, struct hmap *port_groups, >> >> + struct hmap *lflows) >> >> { >> >> bool has_stateful = has_stateful_acl(od); >> >> >> >> @@ -4926,6 +4973,13 @@ build_pre_acls(struct ovn_datapath *od, struct >> > hmap *lflows) >> >> "nd || nd_rs || nd_ra || " >> >> "(udp && udp.src == 546 && udp.dst == 547)", >> > "next;"); >> >> >> >> + /* Ingress and Egress Pre-ACL Table (Stateless_Filter). >> >> + * >> >> + * If the logical switch is configured to bypass conntrack for >> >> + * specific types of traffic, skip conntrack for that traffic. >> >> + */ >> >> + build_stateless_filters(od, port_groups, lflows); >> >> + >> >> /* Ingress and Egress Pre-ACL Table (Priority 100). >> >> * >> >> * Regardless of whether the ACL is "from-lport" or > "to-lport", >> >> @@ -5260,7 +5314,8 @@ build_reject_acl_rules(struct ovn_datapath *od, >> > struct hmap *lflows, >> >> >> >> static void >> >> consider_acl(struct hmap *lflows, struct ovn_datapath *od, >> >> - struct nbrec_acl *acl, bool has_stateful) >> >> + struct nbrec_acl *acl, bool has_stateful, >> >> + bool has_stateful_bypass) >> >> { >> >> bool ingress = !strcmp(acl->direction, "from-lport") ? true > :false; >> >> enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : > S_SWITCH_OUT_ACL; >> >> @@ -5285,7 +5340,19 @@ consider_acl(struct hmap *lflows, struct >> > ovn_datapath *od, >> >> struct ds match = DS_EMPTY_INITIALIZER; >> >> struct ds actions = DS_EMPTY_INITIALIZER; >> >> >> >> - /* Commit the connection tracking entry if it's a new >> >> + /* If traffic matched the acl-stateful-bypass rule, we > don't >> >> + * need to commit the connection tracking entry. >> >> + */ >> >> + if (has_stateful_bypass) { >> >> + ds_put_format(&match, "(" REGBIT_SKIP_ACL_CT "== 1 && >> > (%s)", >> >> + acl->match); >> >> + build_acl_log(&actions, acl); >> >> + ds_put_format(&actions, "next;"); >> >> + ds_clear(&match); >> >> + ds_clear(&actions); >> > >> > It seems this whole "if" block is useless. The match and actions are set >> > but then cleared without being used. >> > >> >> Ooops, thanks for pointing it out. I'll remove it. >> >> >> + } >> >> + >> >> + /* Otherwise commit the connection tracking entry if it's >> > a new >> >> * connection that matches this ACL. After this commit, >> >> * the reply traffic is allowed by a flow we create at >> >> * priority 65535, defined earlier. >> >> @@ -5297,10 +5364,11 @@ consider_acl(struct hmap *lflows, struct >> > ovn_datapath *od, >> >> * by ct_commit in the "stateful" stage) to indicate > that the >> >> * connection should be allowed to resume. >> >> */ >> >> - ds_put_format(&match, "((ct.new && !ct.est)" >> >> - " || (!ct.new && ct.est && !ct.rpl " >> >> - "&& ct_label.blocked == 1)) " >> >> - "&& (%s)", acl->match); >> >> + ds_put_format(&match, REGBIT_SKIP_ACL_CT " == 0 " >> >> + "&& ((ct.new && !ct.est)" >> >> + " || (!ct.new && ct.est && !ct.rpl " >> >> + "&& ct_label.blocked == 1)) " >> >> + "&& (%s)", acl->match); >> >> ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; "); >> >> build_acl_log(&actions, acl); >> >> ds_put_cstr(&actions, "next;"); >> >> @@ -5315,11 +5383,16 @@ consider_acl(struct hmap *lflows, struct >> > ovn_datapath *od, >> >> * deletion. There is no need to commit here, so we > can just >> >> * proceed to the next table. We use this to ensure > that this >> >> * connection is still allowed by the currently defined >> >> - * policy. Match untracked packets too. */ >> >> + * policy. Match untracked packets too. >> >> + * >> >> + * This flow also allows traffic that matches the >> >> + * acl-stateful-bypass rule. >> >> + */ >> >> ds_clear(&match); >> >> ds_clear(&actions); >> >> ds_put_format(&match, >> >> - "(!ct.trk || (!ct.new && ct.est && !ct.rpl" >> >> + "(" REGBIT_SKIP_ACL_CT " == 1 || !ct.trk > || " >> >> + "(!ct.new && ct.est && !ct.rpl" >> >> " && ct_label.blocked == 0)) && (%s)", >> >> acl->match); >> > >> > Because of this lflow, each ACL is translated to 3 extra OVS flows (2 >> > before this patch). If large address set/port groups used in the ACL the >> > cost can be huge. One way to optimize it could be introducing a new >> > stage with a single logical flow to match (" REGBIT_SKIP_ACL_CT " == 1 >> > || !ct.trk || (!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), >> > and set a new flag "NO_TRACK", and then in the current ACL table it only >> > needs a single (extra) flow for each ACL: NO_TRACK == 1 && <acl match>. >> > >> > Something similar can be done for "reject/drop" rules handling for the >> > lflows with several (x '||' y) operators plus a "&&" with the real ACL >> > match. >> > >> > I am not 100% sure if a new stage worth it, but I think at least it is >> > something to be considered. >> > >> >> Sounds good to me, it does reduce the number of OVS flows. I'll change >> it as you suggested. >> >> >> >> >> @@ -5346,7 +5419,7 @@ consider_acl(struct hmap *lflows, struct >> > ovn_datapath *od, >> >> /* If the packet is not tracked or not part of an > established >> >> * connection, then we can simply reject/drop it. */ >> >> ds_put_cstr(&match, >> >> - "(!ct.trk || !ct.est" >> >> + "(" REGBIT_SKIP_ACL_CT " == 1 || !ct.trk || >> > !ct.est" >> >> " || (ct.est && ct_label.blocked == 1))"); >> >> if (!strcmp(acl->action, "reject")) { >> >> build_reject_acl_rules(od, lflows, stage, acl, &match, >> >> @@ -5373,7 +5446,8 @@ consider_acl(struct hmap *lflows, struct >> > ovn_datapath *od, >> >> */ >> >> ds_clear(&match); >> >> ds_clear(&actions); >> >> - ds_put_cstr(&match, "ct.est && ct_label.blocked == 0"); >> >> + ds_put_cstr(&match, REGBIT_SKIP_ACL_CT " == 0 " >> >> + "&& ct.est && ct_label.blocked == 0"); >> >> ds_put_cstr(&actions, "ct_commit { ct_label.blocked = 1; >> > }; "); >> >> if (!strcmp(acl->action, "reject")) { >> >> build_reject_acl_rules(od, lflows, stage, acl, &match, >> >> @@ -5478,6 +5552,7 @@ build_acls(struct ovn_datapath *od, struct hmap >> > *lflows, >> >> struct hmap *port_groups) >> >> { >> >> bool has_stateful = has_stateful_acl(od); >> >> + bool has_stateful_bypass = has_stateful_acl_bypass(od); >> >> >> >> /* Ingress and Egress ACL Table (Priority 0): Packets are > allowed by >> >> * default. A related rule at priority 1 is added below if there >> >> @@ -5508,11 +5583,15 @@ build_acls(struct ovn_datapath *od, struct >> > hmap *lflows, >> >> * Subsequent packets will hit the flow at priority 0 that > just >> >> * uses "next;". */ >> >> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1, >> >> - "ip && (!ct.est || (ct.est && ct_label.blocked >> > == 1))", >> >> - REGBIT_CONNTRACK_COMMIT" = 1; next;"); >> >> + REGBIT_SKIP_ACL_CT " == 0 " >> >> + "&& ip " >> >> + "&& (!ct.est || (ct.est && ct_label.blocked == >> > 1))", >> >> + REGBIT_CONNTRACK_COMMIT" = 1; next;"); >> >> ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1, >> >> - "ip && (!ct.est || (ct.est && ct_label.blocked >> > == 1))", >> >> - REGBIT_CONNTRACK_COMMIT" = 1; next;"); >> >> + REGBIT_SKIP_ACL_CT " == 0 " >> >> + "&& ip " >> >> + "&& (!ct.est || (ct.est && ct_label.blocked == >> > 1))", >> >> + REGBIT_CONNTRACK_COMMIT" = 1; next;"); >> >> >> >> /* Ingress and Egress ACL Table (Priority 65535). >> >> * >> >> @@ -5522,10 +5601,14 @@ build_acls(struct ovn_datapath *od, struct >> > hmap *lflows, >> >> * >> >> * This is enforced at a higher priority than ACLs can be >> > defined. */ >> >> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, >> >> - "ct.inv || (ct.est && ct.rpl && >> > ct_label.blocked == 1)", >> >> + REGBIT_SKIP_ACL_CT " == 0 " >> >> + "&& (ct.inv " >> >> + "|| (ct.est && ct.rpl && ct_label.blocked >> > == 1))", >> >> "drop;"); >> >> ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, >> >> - "ct.inv || (ct.est && ct.rpl && >> > ct_label.blocked == 1)", >> >> + REGBIT_SKIP_ACL_CT " == 0 " >> >> + "&& (ct.inv " >> >> + "|| (ct.est && ct.rpl && ct_label.blocked >> > == 1))", >> >> "drop;"); >> >> >> >> /* Ingress and Egress ACL Table (Priority 65535). >> >> @@ -5538,11 +5621,13 @@ build_acls(struct ovn_datapath *od, struct >> > hmap *lflows, >> >> * >> >> * This is enforced at a higher priority than ACLs can be >> > defined. */ >> >> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, >> >> - "ct.est && !ct.rel && !ct.new && !ct.inv " >> >> + REGBIT_SKIP_ACL_CT "== 0 " >> >> + "&& ct.est && !ct.rel && !ct.new && !ct.inv " >> >> "&& ct.rpl && ct_label.blocked == 0", >> >> "next;"); >> >> ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, >> >> - "ct.est && !ct.rel && !ct.new && !ct.inv " >> >> + REGBIT_SKIP_ACL_CT "== 0 " >> >> + "&& ct.est && !ct.rel && !ct.new && !ct.inv " >> >> "&& ct.rpl && ct_label.blocked == 0", >> >> "next;"); >> >> >> >> @@ -5558,11 +5643,13 @@ build_acls(struct ovn_datapath *od, struct >> > hmap *lflows, >> >> * related traffic such as an ICMP Port Unreachable through >> >> * that's generated from a non-listening UDP port. */ >> >> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, >> >> - "!ct.est && ct.rel && !ct.new && !ct.inv " >> >> + REGBIT_SKIP_ACL_CT "== 0 " >> >> + "&& !ct.est && ct.rel && !ct.new && !ct.inv " >> >> "&& ct_label.blocked == 0", >> >> "next;"); >> >> ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, >> >> - "!ct.est && ct.rel && !ct.new && !ct.inv " >> >> + REGBIT_SKIP_ACL_CT "== 0 " >> >> + "&& !ct.est && ct.rel && !ct.new && !ct.inv " >> >> "&& ct_label.blocked == 0", >> >> "next;"); >> >> >> >> @@ -5578,13 +5665,14 @@ build_acls(struct ovn_datapath *od, struct >> > hmap *lflows, >> >> /* Ingress or Egress ACL Table (Various priorities). */ >> >> for (size_t i = 0; i < od->nbs->n_acls; i++) { >> >> struct nbrec_acl *acl = od->nbs->acls[i]; >> >> - consider_acl(lflows, od, acl, has_stateful); >> >> + consider_acl(lflows, od, acl, has_stateful, > has_stateful_bypass); >> >> } >> >> 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++) { >> >> - consider_acl(lflows, od, pg->nb_pg->acls[i], >> > has_stateful); >> >> + consider_acl(lflows, od, pg->nb_pg->acls[i], >> > has_stateful, >> >> + has_stateful_bypass); >> >> } >> >> } >> >> } >> >> @@ -6617,7 +6705,7 @@ build_lswitch_flows(struct hmap *datapaths, >> > struct hmap *ports, >> >> continue; >> >> } >> >> >> >> - build_pre_acls(od, lflows); >> >> + build_pre_acls(od, port_groups, lflows); >> >> build_pre_lb(od, lflows, meter_groups, lbs); >> >> build_pre_stateful(od, lflows); >> >> build_acls(od, lflows, port_groups); >> >> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema >> >> index 0c939b7..ef0121d 100644 >> >> --- a/ovn-nb.ovsschema >> >> +++ b/ovn-nb.ovsschema >> >> @@ -1,7 +1,7 @@ >> >> { >> >> "name": "OVN_Northbound", >> >> - "version": "5.25.0", >> >> - "cksum": "1354137211 26116", >> >> + "version": "5.26.0", >> >> + "cksum": "1450952466 27225", >> >> "tables": { >> >> "NB_Global": { >> >> "columns": { >> >> @@ -35,6 +35,12 @@ >> >> "refType": "strong"}, >> >> "min": 0, >> >> "max": "unlimited"}}, >> >> + "stateless_filters": { >> >> + "type": {"key": {"type": "uuid", >> >> + "refTable": "Stateless_Filter", >> >> + "refType": "strong"}, >> >> + "min": 0, >> >> + "max": "unlimited"}}, >> >> "acls": {"type": {"key": {"type": "uuid", >> >> "refTable": "ACL", >> >> "refType": "strong"}, >> >> @@ -150,6 +156,12 @@ >> >> "refType": "weak"}, >> >> "min": 0, >> >> "max": "unlimited"}}, >> >> + "stateless_filters": { >> >> + "type": {"key": {"type": "uuid", >> >> + "refTable": "Stateless_Filter", >> >> + "refType": "strong"}, >> >> + "min": 0, >> >> + "max": "unlimited"}}, >> >> "acls": {"type": {"key": {"type": "uuid", >> >> "refTable": "ACL", >> >> "refType": "strong"}, >> >> @@ -201,6 +213,16 @@ >> >> "type": {"key": "string", "value": "string", >> >> "min": 0, "max": "unlimited"}}}, >> >> "isRoot": false}, >> >> + "Stateless_Filter": { >> >> + "columns": { >> >> + "priority": {"type": {"key": {"type": "integer", >> >> + "minInteger": 0, >> >> + "maxInteger": 32767}}}, >> >> + "match": {"type": "string"}, >> >> + "external_ids": { >> >> + "type": {"key": "string", "value": "string", >> >> + "min": 0, "max": "unlimited"}}}, >> >> + "isRoot": false}, >> > >> > Is there any specific consideration that "direction" is not needed? >> > >> >> Initially I had added direction too but found it a bit confusing to use. >> My idea was that if traffic hits a stateless filter in one direction >> then there's no point for replies to that type of traffic to be sent to >> conntrack. >> >> Not having a "direction" field does move a bit the responsibility on the >> CMS to make sure that the "match" is true both for request and for reply >> traffic. >> >> Alternatively, we could add a "direction" field but then the CMS will >> (probably always) have to define the stateless_filters for both > directions. >> >> I don't expect a lot of stateless filters to be configured and the ones >> that will be configured should probably be quite generic. The only use >> case I know of today is for ovn-k8s where all TCP traffic could be >> firewalled in a stateless way while UDP would still need to go to > conntrack. >> >> I don't have a strong preference though so I can add a "direction" field >> if you think that may become useful in the future. >> > I don't have a preference either. Just wanted to understand the > considerations behind. Thanks for explaining. >
Hi Han, I sent a v3, turning this into a series out of which the first patch can be applied earlier if accepted as it should reduce the number of OVS flows generated for stateful ACLs: http://patchwork.ozlabs.org/project/ovn/list/?series=199101 Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
