On Fri, Apr 28, 2023 at 8:50 PM Mark Michelson <[email protected]> wrote:
> 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? > > That' really the tricky bit. I would be in favor of your suggestion, having the condition in PRE_LB only, however we need to test out the possible effects of the reply direction. > > > > > } > > > > /* 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> > > > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
