Hi Ben, Need your eye on this patch for the ddlog stuff. I was wondering if there is a better way to do than what I've done.
Not urgent. Please take your time. Thanks Numan On Mon, Mar 22, 2021 at 4:29 PM <[email protected]> wrote: > > From: Numan Siddique <[email protected]> > > Presently we add 65535 priority lflows in the stages - > 'ls_in_acl' and 'ls_out_acl' to drop packets which > match on 'ct.inv'. > > This patch provides an option to not use this field in the > logical flow matches for the following > reasons: > > • Some of the smart NICs which support offloading datapath > flows may not support this field. In such cases, almost > all the datapath flows cannot be offloaded if ACLs/load balancers > are configured on the logical switch datapath. > > • A recent commit in kernel ovs datapath sets the committed > connection tracking entry to be liberal for out-of-window > tcp packets (nf_ct_set_tcp_be_liberal()). Such TCP > packets will not be marked as invalid. > > There are still certain scenarios where a packet can be marked > invalid. Like > • If the tcp flags are not correct. > > • If there are checksum errors. > > In such cases, the packet will be delivered to the destination > port. So CMS should evaluate their usecases before enabling > this option. > > Signed-off-by: Numan Siddique <[email protected]> > --- > NEWS | 5 +++ > northd/lswitch.dl | 13 +++++++- > northd/ovn-northd.c | 41 ++++++++++++++--------- > northd/ovn_northd.dl | 51 +++++++++++++++++++++++------ > ovn-nb.xml | 11 +++++++ > tests/ovn-northd.at | 77 ++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 171 insertions(+), 27 deletions(-) > > diff --git a/NEWS b/NEWS > index 530c5d42fe..3181649ba8 100644 > --- a/NEWS > +++ b/NEWS > @@ -7,6 +7,11 @@ Post-v21.03.0 > (This may take testing and tuning to be effective.) This version of OVN > requires DDLog 0.36. > - Introduce ovn-controller incremetal processing engine statistics > + - Add a new NB Global option - us_ct_inv_match with the default value of > true. > + If this is set to false, then the logical field - "ct.inv" will not be > + used in the logical flow matches. CMS can consider setting this to > false, > + if they want to use smart NICs which don't support offloading datapath > flows > + with this field used. > > OVN v21.03.0 - 12 Mar 2021 > ------------------------- > diff --git a/northd/lswitch.dl b/northd/lswitch.dl > index 4bf8a5b907..1daf249382 100644 > --- a/northd/lswitch.dl > +++ b/northd/lswitch.dl > @@ -197,6 +197,7 @@ relation &Switch( > ipv6_prefix: Option<in6_addr>, > mcast_cfg: Ref<McastSwitchCfg>, > is_vlan_transparent: bool, > + use_ct_inv_match: bool, > > /* Does this switch have at least one port with type != "router"? */ > has_non_router_port: bool > @@ -223,7 +224,8 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> { > .ipv6_prefix = ipv6_prefix, > .mcast_cfg = mcast_cfg, > .has_non_router_port = has_non_router_port, > - .is_vlan_transparent = is_vlan_transparent) :- > + .is_vlan_transparent = is_vlan_transparent, > + .use_ct_inv_match = use_ct_inv_match) :- > nb::Logical_Switch[ls], > LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl), > LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip), > @@ -232,6 +234,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> { > LogicalSwitchLocalnetPorts(ls._uuid, localnet_ports), > LogicalSwitchHasNonRouterPort(ls._uuid, has_non_router_port), > mcast_cfg in &McastSwitchCfg(.datapath = ls._uuid), > + UseCtInvMatch(use_ct_inv_match), > var subnet = > match (ls.other_config.get("subnet")) { > None -> None, > @@ -744,3 +747,11 @@ function put_svc_monitor_mac(options: Map<string,string>, > relation SvcMonitorMac(mac: eth_addr) > SvcMonitorMac(get_svc_monitor_mac(options, uuid)) :- > nb::NB_Global(._uuid = uuid, .options = options). > + > +function can_use_ct_inv_match(options: Map<string,string>): bool { > + map_get_bool_def(options, "use_ct_inv_match", true) > +} > + > +relation UseCtInvMatch(use_ct_inv_match: bool) > +UseCtInvMatch(can_use_ct_inv_match(options)) :- > + nb::NB_Global(._uuid = uuid, .options = options). > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 3221fb9ff7..6baed2a418 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -4066,6 +4066,10 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct > ovn_datapath *od, > * logical datapath only by creating a datapath group. */ > static bool use_logical_dp_groups = false; > > +/* If this option is 'true' northd will make use of ct.inv match fields. > + * Otherwise, it will avoid using it. The default is true. */ > +static bool use_ct_inv_match = true; > + > /* Adds a row with the specified contents to the Logical_Flow table. */ > static void > ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od, > @@ -5681,12 +5685,13 @@ build_acls(struct ovn_datapath *od, struct hmap > *lflows, > * for deletion (bit 0 of ct_label is set). > * > * This is enforced at a higher priority than ACLs can be defined. */ > + char *match = xasprintf("%s(ct.est && ct.rpl && ct_label.blocked == > 1)", > + use_ct_inv_match ? "ct.inv || " : ""); > ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > - "ct.inv || (ct.est && ct.rpl && ct_label.blocked == > 1)", > - "drop;"); > + match, "drop;"); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > - "ct.inv || (ct.est && ct.rpl && ct_label.blocked == > 1)", > - "drop;"); > + match, "drop;"); > + free(match); > > /* Ingress and Egress ACL Table (Priority 65535). > * > @@ -5697,14 +5702,15 @@ build_acls(struct ovn_datapath *od, struct hmap > *lflows, > * direction to hit the currently defined policy from ACLs. > * > * This is enforced at a higher priority than ACLs can be defined. */ > + match = xasprintf("ct.est && !ct.rel && !ct.new%s && " > + "ct.rpl && ct_label.blocked == 0", > + use_ct_inv_match ? " && !ct.inv" : ""); > + > ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > - "ct.est && !ct.rel && !ct.new && !ct.inv " > - "&& ct.rpl && ct_label.blocked == 0", > - "next;"); > + match, "next;"); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > - "ct.est && !ct.rel && !ct.new && !ct.inv " > - "&& ct.rpl && ct_label.blocked == 0", > - "next;"); > + match, "next;"); > + free(match); > > /* Ingress and Egress ACL Table (Priority 65535). > * > @@ -5717,14 +5723,14 @@ build_acls(struct ovn_datapath *od, struct hmap > *lflows, > * a dynamically negotiated FTP data channel), but will allow > * related traffic such as an ICMP Port Unreachable through > * that's generated from a non-listening UDP port. */ > + match = xasprintf("!ct.est && ct.rel && !ct.new%s && " > + "ct_label.blocked == 0", > + use_ct_inv_match ? " && !ct.inv" : ""); > ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > - "!ct.est && ct.rel && !ct.new && !ct.inv " > - "&& ct_label.blocked == 0", > - "next;"); > + match, "next;"); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > - "!ct.est && ct.rel && !ct.new && !ct.inv " > - "&& ct_label.blocked == 0", > - "next;"); > + match, "next;"); > + free(match); > > /* Ingress and Egress ACL Table (Priority 65535). > * > @@ -12897,6 +12903,9 @@ ovnnb_db_run(struct northd_context *ctx, > > use_logical_dp_groups = smap_get_bool(&nb->options, > "use_logical_dp_groups", false); > + use_ct_inv_match = smap_get_bool(&nb->options, > + "use_ct_inv_match", true); > + > /* deprecated, use --event instead */ > controller_event_en = smap_get_bool(&nb->options, > "controller_event", false); > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index 35e6ab76cb..9e1919cd80 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -2396,16 +2396,25 @@ var has_stateful = sw.has_stateful_acl or > sw.has_lb_vip in > * for deletion (bit 0 of ct_label is set). > * > * This is enforced at a higher priority than ACLs can be defined. */ > + var __match = match (sw.use_ct_inv_match) { > + true -> "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)", > + false -> "(ct.est && ct.rpl && ct_label.blocked == 1)" > + } in > Flow(.logical_datapath = ls._uuid, > .stage = switch_stage(IN, ACL), > .priority = 65535, > - .__match = "ct.inv || (ct.est && ct.rpl && > ct_label.blocked == 1)", > + .__match = __match, > .actions = "drop;", > .external_ids = map_empty()); > + > + var __match = match (sw.use_ct_inv_match) { > + true -> "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)", > + false -> "(ct.est && ct.rpl && ct_label.blocked == 1)" > + } in > Flow(.logical_datapath = ls._uuid, > .stage = switch_stage(OUT, ACL), > .priority = 65535, > - .__match = "ct.inv || (ct.est && ct.rpl && > ct_label.blocked == 1)", > + .__match = __match, > .actions = "drop;", > .external_ids = map_empty()); > > @@ -2418,18 +2427,29 @@ var has_stateful = sw.has_stateful_acl or > sw.has_lb_vip in > * direction to hit the currently defined policy from ACLs. > * > * This is enforced at a higher priority than ACLs can be defined. */ > + var __match = match (sw.use_ct_inv_match) { > + true -> "ct.est && !ct.rel && !ct.new && !ct.inv " > + "&& ct.rpl && ct_label.blocked == 0", > + false -> "ct.est && !ct.rel && !ct.new " > + "&& ct.rpl && ct_label.blocked == 0" > + } in > Flow(.logical_datapath = ls._uuid, > .stage = switch_stage(IN, ACL), > .priority = 65535, > - .__match = "ct.est && !ct.rel && !ct.new && !ct.inv " > - "&& ct.rpl && ct_label.blocked == 0", > + .__match = __match, > .actions = "next;", > .external_ids = map_empty()); > + > + var __match = match (sw.use_ct_inv_match) { > + true -> "ct.est && !ct.rel && !ct.new && !ct.inv " > + "&& ct.rpl && ct_label.blocked == 0", > + false -> "ct.est && !ct.rel && !ct.new " > + "&& ct.rpl && ct_label.blocked == 0" > + } in > Flow(.logical_datapath = ls._uuid, > .stage = switch_stage(OUT, ACL), > .priority = 65535, > - .__match = "ct.est && !ct.rel && !ct.new && !ct.inv " > - "&& ct.rpl && ct_label.blocked == 0", > + .__match = __match, > .actions = "next;", > .external_ids = map_empty()); > > @@ -2444,18 +2464,29 @@ var has_stateful = sw.has_stateful_acl or > sw.has_lb_vip in > * a dynamically negotiated FTP data channel), but will allow > * related traffic such as an ICMP Port Unreachable through > * that's generated from a non-listening UDP port. */ > + var __match = match (sw.use_ct_inv_match) { > + true -> "!ct.est && ct.rel && !ct.new && !ct.inv " > + "&& ct_label.blocked == 0", > + false -> "!ct.est && ct.rel && !ct.new " > + "&& ct_label.blocked == 0", > + } in > Flow(.logical_datapath = ls._uuid, > .stage = switch_stage(IN, ACL), > .priority = 65535, > - .__match = "!ct.est && ct.rel && !ct.new && !ct.inv " > - "&& ct_label.blocked == 0", > + .__match = __match, > .actions = "next;", > .external_ids = map_empty()); > + > + var __match = match (sw.use_ct_inv_match) { > + true -> "!ct.est && ct.rel && !ct.new && !ct.inv " > + "&& ct_label.blocked == 0", > + false -> "!ct.est && ct.rel && !ct.new " > + "&& ct_label.blocked == 0", > + } in > Flow(.logical_datapath = ls._uuid, > .stage = switch_stage(OUT, ACL), > .priority = 65535, > - .__match = "!ct.est && ct.rel && !ct.new && !ct.inv " > - "&& ct_label.blocked == 0", > + .__match = __match, > .actions = "next;", > .external_ids = map_empty()); > > diff --git a/ovn-nb.xml b/ovn-nb.xml > index b0a4adffe5..c3ff3b586a 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -227,6 +227,17 @@ > </p> > </column> > > + <column name="options" key="use_ct_inv_match"> > + <p> > + If set to false, <code>ovn-northd</code> will not use the > + <code>ct.inv</code> field in any of the logical flow matches. > + The default value is true. CMS can consider setting this to > + false, in case the NIC doesn't support offloading OVS datapath > + flows having matches <code>ct_state(..+inv..)</code> or > + <code>ct_state(..-inv..)</code>. > + </p> > + </column> > + > <group title="Options for configuring interconnection route > advertisement"> > <p> > These options control how routes are advertised between OVN > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 32f956a797..3a3a443a4e 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -3066,3 +3066,80 @@ AT_CHECK([grep "ls_out_stateful" sw0flows | sort], > [0], [dnl > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn -- ct.inv usage]) > +ovn_start > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lsp-add sw0 sw0p1 > + > +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 ip allow-related > + > +ovn-sbctl dump-flows sw0 > sw0flows > +AT_CAPTURE_FILE([sw0flows]) > + > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl > + table=9 (ls_in_acl ), priority=65535, match=(!ct.est && ct.rel && > !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) > + table=9 (ls_in_acl ), priority=65535, match=(ct.est && !ct.rel && > !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) > + table=9 (ls_in_acl ), priority=65535, match=(ct.inv || (ct.est && > ct.rpl && ct_label.blocked == 1)), action=(drop;) > + table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra || nd_rs > || mldv1 || mldv2), action=(next;) > +]) > + > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl > + table=4 (ls_out_acl ), priority=65535, match=(!ct.est && ct.rel && > !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) > + table=4 (ls_out_acl ), priority=65535, match=(ct.est && !ct.rel && > !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) > + table=4 (ls_out_acl ), priority=65535, match=(ct.inv || (ct.est && > ct.rpl && ct_label.blocked == 1)), action=(drop;) > + table=4 (ls_out_acl ), priority=65535, match=(nd || nd_ra || nd_rs > || mldv1 || mldv2), action=(next;) > +]) > + > +# Disable ct.inv usage. > +check ovn-nbctl --wait=sb set NB_Global . options:use_ct_inv_match=false > + > +ovn-sbctl dump-flows sw0 > sw0flows > +AT_CAPTURE_FILE([sw0flows]) > + > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl > + table=9 (ls_in_acl ), priority=65535, match=(!ct.est && ct.rel && > !ct.new && ct_label.blocked == 0), action=(next;) > + table=9 (ls_in_acl ), priority=65535, match=((ct.est && ct.rpl && > ct_label.blocked == 1)), action=(drop;) > + table=9 (ls_in_acl ), priority=65535, match=(ct.est && !ct.rel && > !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) > + table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra || nd_rs > || mldv1 || mldv2), action=(next;) > +]) > + > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl > + table=4 (ls_out_acl ), priority=65535, match=(!ct.est && ct.rel && > !ct.new && ct_label.blocked == 0), action=(next;) > + table=4 (ls_out_acl ), priority=65535, match=((ct.est && ct.rpl && > ct_label.blocked == 1)), action=(drop;) > + table=4 (ls_out_acl ), priority=65535, match=(ct.est && !ct.rel && > !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) > + table=4 (ls_out_acl ), priority=65535, match=(nd || nd_ra || nd_rs > || mldv1 || mldv2), action=(next;) > +]) > + > +AT_CHECK([grep -c "ct.inv" sw0flows], [1], [dnl > +0 > +]) > + > +# Enable ct.inv usage. > +check ovn-nbctl --wait=sb set NB_Global . options:use_ct_inv_match=true > + > +ovn-sbctl dump-flows sw0 > sw0flows > +AT_CAPTURE_FILE([sw0flows]) > + > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl > + table=9 (ls_in_acl ), priority=65535, match=(!ct.est && ct.rel && > !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) > + table=9 (ls_in_acl ), priority=65535, match=(ct.est && !ct.rel && > !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) > + table=9 (ls_in_acl ), priority=65535, match=(ct.inv || (ct.est && > ct.rpl && ct_label.blocked == 1)), action=(drop;) > + table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra || nd_rs > || mldv1 || mldv2), action=(next;) > +]) > + > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl > + table=4 (ls_out_acl ), priority=65535, match=(!ct.est && ct.rel && > !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) > + table=4 (ls_out_acl ), priority=65535, match=(ct.est && !ct.rel && > !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;) > + table=4 (ls_out_acl ), priority=65535, match=(ct.inv || (ct.est && > ct.rpl && ct_label.blocked == 1)), action=(drop;) > + table=4 (ls_out_acl ), priority=65535, match=(nd || nd_ra || nd_rs > || mldv1 || mldv2), action=(next;) > +]) > + > +AT_CHECK([grep -c "ct.inv" sw0flows], [0], [dnl > +6 > +]) > + > +AT_CLEANUP > +]) > -- > 2.29.2 > > _______________________________________________ > 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
