diff mbox series

[ovs-dev,v4,2/2] Pass localnet traffic through CT when a LB is configured.

Message ID 20230518201117.1864052-2-mmichels@redhat.com
State Accepted
Headers show
Series [ovs-dev,v4,1/2] tests: Use stricter IP match for FORMAT_CT. | expand

Checks

Context Check Description
ovsrobot/apply-robot fail apply and check: fail

Commit Message

Mark Michelson May 18, 2023, 8:11 p.m. UTC 
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 <dceara@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Mark Michelson <mmichels@redhat.com>
---
 controller/ovn-controller.c | 16 ++++---
 northd/northd.c             | 18 ++++++--
 tests/ovn-northd.at         | 60 ++++++++++++++++++++++++
 tests/system-ovn.at         | 91 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 172 insertions(+), 13 deletions(-)

Comments

0-day Robot May 18, 2023, 8:18 p.m. UTC | #1 
Bleep bloop.  Greetings Mark Michelson, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Pass localnet traffic through CT when a LB is configured.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Ales Musil May 19, 2023, 7:01 a.m. UTC | #2 
On Thu, May 18, 2023 at 10:11 PM Mark Michelson <mmichels@redhat.com> 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 <dceara@redhat.com>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> Signed-off-by: Mark Michelson <mmichels@redhat.com>
> ---
>  controller/ovn-controller.c | 16 ++++---
>  northd/northd.c             | 18 ++++++--
>  tests/ovn-northd.at         | 60 ++++++++++++++++++++++++
>  tests/system-ovn.at         | 91 ++++++++++++++++++++++++++++++++++++-
>  4 files changed, 172 insertions(+), 13 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index de90025f0..662029597 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,8 +2467,10 @@ 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")) {
> -                /* We allocate zone-id's only to VIF and localport
> lports. */
> +                && strcmp(t_lport->pb->type, "localport")
> +                && strcmp(t_lport->pb->type, "localnet")) {
> +                /* We allocate zone-id's only to VIF, localport, and
> localnet
> +                 * lports. */
>                  continue;
>              }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index b69fcf321..41d0f5994 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5968,7 +5968,8 @@ build_pre_acls(struct ovn_datapath *od, const struct
> hmap *port_groups,
>          }
>          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,
> +                                     S_SWITCH_IN_PRE_ACL,
> +                                     S_SWITCH_OUT_PRE_ACL,
>                                       110, lflows);
>          }
>
> @@ -6137,10 +6138,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 047b8b6ad..850bc25a4 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/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 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 b/tests/system-ovn.at
> index df0dd99fb..61fb47865 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -10692,7 +10692,7 @@ ovn_start
>  ADD_BR([br-int])
>
>  # Set external-ids in br-int needed for ovn-controller
> -check ovs-vsctl \
> +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 \
> @@ -11009,3 +11009,92 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
> patch-.*/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_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
> +check ovn-nbctl lrp-add ro ro-pub 00:00:00:00:01:01 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", "00:00:00:00:00:02", \
> +         "192.168.0.1")
> +
> +ADD_NAMESPACES(ln)
> +ADD_VETH(ln, ln, br-phys, "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 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 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])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.1) | \
> +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>
> +tcp,orig=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.0.2,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>)
> +])
> +
> +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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil <amusil@redhat.com>
Mark Michelson May 19, 2023, 4:44 p.m. UTC | #3 
Thanks for the review Ales.

I added a bugzilla citation to the commit message and pushed the change 
to main and all branches back to 22.03.

On 5/19/23 03:01, Ales Musil wrote:
> 
> 
> On Thu, May 18, 2023 at 10:11 PM Mark Michelson <mmichels@redhat.com 
> <mailto:mmichels@redhat.com>> 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 <dceara@redhat.com
>     <mailto:dceara@redhat.com>>
>     Signed-off-by: Dumitru Ceara <dceara@redhat.com
>     <mailto:dceara@redhat.com>>
>     Signed-off-by: Mark Michelson <mmichels@redhat.com
>     <mailto:mmichels@redhat.com>>
>     ---
>       controller/ovn-controller.c | 16 ++++---
>       northd/northd.c             | 18 ++++++--
>       tests/ovn-northd.at <http://ovn-northd.at>         | 60
>     ++++++++++++++++++++++++
>       tests/system-ovn.at <http://system-ovn.at>         | 91
>     ++++++++++++++++++++++++++++++++++++-
>       4 files changed, 172 insertions(+), 13 deletions(-)
> 
>     diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>     index de90025f0..662029597 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,8 +2467,10 @@ 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")) {
>     -                /* We allocate zone-id's only to VIF and localport
>     lports. */
>     +                && strcmp(t_lport->pb->type, "localport")
>     +                && strcmp(t_lport->pb->type, "localnet")) {
>     +                /* We allocate zone-id's only to VIF, localport,
>     and localnet
>     +                 * lports. */
>                       continue;
>                   }
> 
>     diff --git a/northd/northd.c b/northd/northd.c
>     index b69fcf321..41d0f5994 100644
>     --- a/northd/northd.c
>     +++ b/northd/northd.c
>     @@ -5968,7 +5968,8 @@ build_pre_acls(struct ovn_datapath *od, const
>     struct hmap *port_groups,
>               }
>               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,
>     +                                     S_SWITCH_IN_PRE_ACL,
>     +                                     S_SWITCH_OUT_PRE_ACL,
>                                            110, lflows);
>               }
> 
>     @@ -6137,10 +6138,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 df0dd99fb..61fb47865 100644
>     --- a/tests/system-ovn.at <http://system-ovn.at>
>     +++ b/tests/system-ovn.at <http://system-ovn.at>
>     @@ -10692,7 +10692,7 @@ ovn_start
>       ADD_BR([br-int])
> 
>       # Set external-ids in br-int needed for ovn-controller
>     -check ovs-vsctl \
>     +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 \
>     @@ -11009,3 +11009,92 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to
>     query port patch-.*/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_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])
>     +
>     +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.1) | \
>     +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>     +tcp,orig=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.0.2,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>)
>     +])
>     +
>     +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
>     dev@openvswitch.org <mailto:dev@openvswitch.org>
>     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>     <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
> 
> 
> Looks good to me, thanks.
> 
> Acked-by: Ales Musil <amusil@redhat.com <mailto:amusil@redhat.com>>
> 
> 
> -- 
> 
> Ales Musil
> 
> Senior Software Engineer - OVN Core
> 
> Red Hat EMEA <https://www.redhat.com>
> 
> amusil@redhat.com <mailto:amusil@redhat.com> IM: amusil
> 
> <https://red.ht/sig>
>
diff mbox series

Patch

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index de90025f0..662029597 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,8 +2467,10 @@  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")) {
-                /* We allocate zone-id's only to VIF and localport lports. */
+                && strcmp(t_lport->pb->type, "localport")
+                && strcmp(t_lport->pb->type, "localnet")) {
+                /* We allocate zone-id's only to VIF, localport, and localnet
+                 * lports. */
                 continue;
             }
 
diff --git a/northd/northd.c b/northd/northd.c
index b69fcf321..41d0f5994 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5968,7 +5968,8 @@  build_pre_acls(struct ovn_datapath *od, const struct hmap *port_groups,
         }
         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,
+                                     S_SWITCH_IN_PRE_ACL,
+                                     S_SWITCH_OUT_PRE_ACL,
                                      110, lflows);
         }
 
@@ -6137,10 +6138,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 047b8b6ad..850bc25a4 100644
--- a/tests/ovn-northd.at
+++ b/tests/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 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 b/tests/system-ovn.at
index df0dd99fb..61fb47865 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -10692,7 +10692,7 @@  ovn_start
 ADD_BR([br-int])
 
 # Set external-ids in br-int needed for ovn-controller
-check ovs-vsctl \
+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 \
@@ -11009,3 +11009,92 @@  OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/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_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
+check ovn-nbctl lrp-add ro ro-pub 00:00:00:00:01:01 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", "00:00:00:00:00:02", \
+         "192.168.0.1")
+
+ADD_NAMESPACES(ln)
+ADD_VETH(ln, ln, br-phys, "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 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 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])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.1) | \
+sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
+tcp,orig=(src=10.0.0.2,dst=172.16.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.0.2,dst=10.0.0.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>)
+])
+
+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
+])