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 conntack is no longer skipped for traffic involving the localnet port. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2164652 Co-authored-by: Dumitru Ceara <dce...@redhat.com> Signed-off-by: Mark Michelson <mmich...@redhat.com> --- This patch is based on an inlined patch provided by Dumitru Ceara on the referenced bugzilla issue. I modified it to only skip conntrack when a load balancer is present on the logical switch, and I added a simple test case to verify the logical flows are populated as expected. Dumitru, if you want more than the "Co-authored-by" citation on this issue, please let me know and I'll adjust it. --- controller/ovn-controller.c | 13 ++++---- northd/northd.c | 25 +++++++++++----- tests/ovn-northd.at | 60 +++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 14 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index e170e9262..3a89e386f 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -723,7 +723,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) @@ -736,9 +736,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. */ @@ -2392,7 +2392,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); @@ -2482,7 +2482,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 da3c8cf77..8439446aa 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -5959,10 +5959,12 @@ 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); + } } /* stateless filters always take precedence over stateful ACLs. */ @@ -6130,10 +6132,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 b/tests/ovn-northd.at index 9fee00094..909b06719 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -8830,3 +8830,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 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 +]) -- 2.38.1 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
