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
