On 4/26/23 05:05, Ales Musil wrote:
On Tue, Apr 25, 2023 at 10:07 PM Mark Michelson <[email protected] <mailto:[email protected]>> wrote:Current code always skips conntrack for traffic that ingresses or egresses on a localnet port. However, this makes it impossible for traffic to be load-balanced when it arrives on a localnet port. This patch allows for traffic to be load balanced on localnet ports by making two changes: * Localnet ports now have a conntrack zone assigned. * When a load balancer is configured on a logical switch containing a localnet port, then conntrack is no longer skipped for traffic involving the localnet port. Co-authored by: Dumitru Ceara <[email protected] <mailto:[email protected]>> Signed-off-by: Dumitru Ceara <[email protected] <mailto:[email protected]>> Signed-off-by: Mark Michelson <[email protected] <mailto:[email protected]>> Hi Mark, I have two comments below. --- controller/ovn-controller.c | 13 +++--- northd/northd.c | 26 +++++++---- tests/ovn-northd.at <http://ovn-northd.at> | 60 ++++++++++++++++++++++++++ tests/system-ovn.at <http://system-ovn.at> | 86 +++++++++++++++++++++++++++++++++++++ 4 files changed, 171 insertions(+), 14 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index c094cb74d..b62f26a14 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -708,7 +708,7 @@ get_snat_ct_zone(const struct sbrec_datapath_binding *dp) } static void -update_ct_zones(const struct shash *binding_lports, +update_ct_zones(const struct sset *local_lports, const struct hmap *local_datapaths, struct simap *ct_zones, unsigned long *ct_zone_bitmap, struct shash *pending_ct_zones) @@ -721,9 +721,9 @@ update_ct_zones(const struct shash *binding_lports, unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)]; struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones); - struct shash_node *shash_node; - SHASH_FOR_EACH (shash_node, binding_lports) { - sset_add(&all_users, shash_node->name); + const char *local_lport; + SSET_FOR_EACH (local_lport, local_lports) { + sset_add(&all_users, local_lport); } /* Local patched datapath (gateway routers) need zones assigned. */ @@ -2377,7 +2377,7 @@ en_ct_zones_run(struct engine_node *node, void *data) EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); restore_ct_zones(bridge_table, ovs_table, ct_zones_data); - update_ct_zones(&rt_data->lbinding_data.lports, &rt_data->local_datapaths, + update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths, &ct_zones_data->current, ct_zones_data->bitmap, &ct_zones_data->pending); @@ -2467,7 +2467,8 @@ ct_zones_runtime_data_handler(struct engine_node *node, void *data) SHASH_FOR_EACH (shash_node, &tdp->lports) { struct tracked_lport *t_lport = shash_node->data; if (strcmp(t_lport->pb->type, "") - && strcmp(t_lport->pb->type, "localport")) { + && strcmp(t_lport->pb->type, "localport") + && strcmp(t_lport->pb->type, "localnet")) { /* We allocate zone-id's only to VIF and localport lports. */ continue; } diff --git a/northd/northd.c b/northd/northd.c index d59a54b32..3b14b46ca 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -5962,10 +5962,13 @@ build_pre_acls(struct ovn_datapath *od, const struct hmap *port_groups, S_SWITCH_IN_PRE_ACL, S_SWITCH_OUT_PRE_ACL, 110, lflows); } - for (size_t i = 0; i < od->n_localnet_ports; i++) { - skip_port_from_conntrack(od, od->localnet_ports[i], - S_SWITCH_IN_PRE_ACL, S_SWITCH_OUT_PRE_ACL, - 110, lflows); + if (!od->has_lb_vip) { + for (size_t i = 0; i < od->n_localnet_ports; i++) { + skip_port_from_conntrack(od, od->localnet_ports[i], + S_SWITCH_IN_PRE_ACL, + S_SWITCH_OUT_PRE_ACL, + 110, lflows); + }As Dumitru pointed out before, this will "sinently" allow ACLs if there is load balancer configured. And looking through the code it doesn't seem to be necessary, unless I'm missing something. I guess we can probably check if it's needed by testing LB + ACLs on the localnet port?
I was about to make this change but then got a bit curious about doing this differently. What if we continue to skip_port_from_conntrack() here in the PRE_ACL stage for localnet ports at all times? Then we only skip_port_from_conntrack() in the PRE_LB stage if there are no load balancers configured? Would this still result in silently permitting ACLs for localnet ports? Or would this have some weird behavior where, say, ACLs on localnet ports would only work in the reply direction if the request direction had previously been load balanced?
} /* stateless filters always take precedence over stateful ACLs. */ @@ -6133,10 +6136,17 @@ build_pre_lb(struct ovn_datapath *od, const struct shash *meter_groups, S_SWITCH_IN_PRE_LB, S_SWITCH_OUT_PRE_LB, 110, lflows); } - for (size_t i = 0; i < od->n_localnet_ports; i++) { - skip_port_from_conntrack(od, od->localnet_ports[i], - S_SWITCH_IN_PRE_LB, S_SWITCH_OUT_PRE_LB, - 110, lflows); + /* Localnet ports have no need for going through conntrack, unless + * the logical switch has a load balancer. Then, conntrack is necessary + * so that traffic arriving via the localnet port can be load + * balanced. + */ + if (!od->has_lb_vip) { + for (size_t i = 0; i < od->n_localnet_ports; i++) { + skip_port_from_conntrack(od, od->localnet_ports[i], + S_SWITCH_IN_PRE_LB, S_SWITCH_OUT_PRE_LB, + 110, lflows); + } } /* Do not sent statless flows via conntrack */ diff --git a/tests/ovn-northd.at <http://ovn-northd.at> b/tests/ovn-northd.at <http://ovn-northd.at> index 047b8b6ad..850bc25a4 100644 --- a/tests/ovn-northd.at <http://ovn-northd.at> +++ b/tests/ovn-northd.at <http://ovn-northd.at> @@ -8975,3 +8975,63 @@ mac_binding_timestamp: true AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Localnet ports on LS with LB]) +ovn_start +# In the past, traffic arriving on localnet ports has skipped conntrack. +# This test ensures that we still skip conntrack for localnet ports, +# *except* for the case where the logical switch has a load balancer +# configured. In this case, the localnet port will not skip conntrack, +# allowing for traffic to be load balanced on the localnet port. + +check ovn-nbctl ls-add sw +check ovn-nbctl lsp-add sw sw-ln +check ovn-nbctl lsp-set-type sw-ln localnet +check ovn-nbctl lsp-set-addresses sw-ln unknown +check ovn-nbctl --wait=sb sync + +# Since this test is only concerned with logical flows, we don't need to +# configure anything else that we normally would with regards to localnet +# ports + + +# First, ensure that conntrack is skipped for the localnet port since there +# isn't a load balancer configured. + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_pre_lb | grep priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl + table=??(ls_in_pre_lb ), priority=110 , match=(ip && inport == "sw-ln"), action=(next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_pre_lb | grep priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl + table=??(ls_out_pre_lb ), priority=110 , match=(ip && outport == "sw-ln"), action=(ct_clear; next;) +]) + +# Now add a load balancer and ensure that we no longer are skipping conntrack +# for the localnet port + +check ovn-nbctl lb-add lb 10.0.0.1:80 <http://10.0.0.1:80> 10.0.0.100:8080 <http://10.0.0.100:8080> tcp +check ovn-nbctl ls-lb-add sw lb +check ovn-nbctl --wait=sb sync + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_pre_lb | grep priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_pre_lb | grep priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl +]) + +# And ensure that removing the load balancer from the switch results in skipping +# conntrack again +check ovn-nbctl ls-lb-del sw lb +check ovn-nbctl --wait=sb sync + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_pre_lb | grep priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl + table=??(ls_in_pre_lb ), priority=110 , match=(ip && inport == "sw-ln"), action=(next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_pre_lb | grep priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl + table=??(ls_out_pre_lb ), priority=110 , match=(ip && outport == "sw-ln"), action=(ct_clear; next;) +]) + +AT_CLEANUP +]) diff --git a/tests/system-ovn.at <http://system-ovn.at> b/tests/system-ovn.at <http://system-ovn.at> index b46f67636..a98072f23 100644 --- a/tests/system-ovn.at <http://system-ovn.at> +++ b/tests/system-ovn.at <http://system-ovn.at> @@ -10667,3 +10667,89 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d /connection dropped.*/d"]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([load balancer with localnet port]) +CHECK_CONNTRACK() +CHECK_CONNTRACK_NAT() +ovn_start +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) +ADD_BR([br-phys], [set Bridge br-phys fail-mode=standalone]) + +# Set external-ids in br-int needed for ovn-controller +ovs-vsctl \ + -- set Open_vSwitch . external-ids:system-id=hv1 \ + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true + +# Start ovn-controller +start_daemon ovn-controller + +check ovn-nbctl lr-add ro +check ovn-nbctl lrp-add ro ro-sw 00:00:00:00:00:01 192.168.0.1/24 <http://192.168.0.1/24> +check ovn-nbctl lrp-add ro ro-pub 00:00:00:00:01:01 10.0.0.1/24 <http://10.0.0.1/24> + +check ovn-nbctl ls-add sw +check ovn-nbctl lsp-add sw sw-vm1 \ + -- lsp-set-addresses sw-vm1 "00:00:00:00:00:02 192.168.0.2" +check ovn-nbctl lsp-add sw sw-ro \ + -- lsp-set-type sw-ro router \ + -- lsp-set-addresses sw-ro router \ + -- lsp-set-options sw-ro router-port=ro-sw + +check ovn-nbctl ls-add pub +check ovn-nbctl lsp-add pub sw-ln \ + -- lsp-set-type sw-ln localnet \ + -- lsp-set-addresses sw-ln unknown \ + -- lsp-set-options sw-ln network_name=phys +check ovn-nbctl lsp-add pub pub-ro \ + -- lsp-set-type pub-ro router \ + -- lsp-set-addresses pub-ro router \ + -- lsp-set-options pub-ro router-port=ro-pub + +check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys + +ADD_NAMESPACES(sw-vm1) +ADD_VETH(sw-vm1, sw-vm1, br-int, "192.168.0.2/24 <http://192.168.0.2/24>", "00:00:00:00:00:02", \ + "192.168.0.1") + +ADD_NAMESPACES(ln) +ADD_VETH(ln, ln, br-phys, "10.0.0.2/24 <http://10.0.0.2/24>", "00:00:00:00:01:02", \ + "10.0.0.1") + +# We have the basic network set up. Now let's add a load balancer +# on the "pub" logical switch. + +check ovn-nbctl lb-add ln-lb 172.16.0.1:80 <http://172.16.0.1:80> 192.168.0.2:80 <http://192.168.0.2:80> tcp +check ovn-nbctl ls-lb-add pub ln-lb +check ovn-nbctl --wait=hv sync + +# Add a route so that the localnet port can reach the load balancer +# VIP. +NS_CHECK_EXEC([ln], [ip route add 172.16.0.1 via 10.0.0.1]) +NS_CHECK_EXEC([ln], [ip route add 192.168.0.0/24 <http://192.168.0.0/24> via 10.0.0.1]) + +OVS_START_L7([sw-vm1], [http]) + +NS_CHECK_EXEC([ln], [wget 172.16.0.1 -t 5 -T 1 --retry-connrefused -v -o wget.log]) Just a nit, but it would be better to have a CT dump check. + +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + +AT_CLEANUP +])-- 2.39.2_______________________________________________ dev mailing list [email protected] <mailto:[email protected]> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <mailto:[email protected]> IM: amusil <https://red.ht/sig>
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
