On 4/18/23 10:32, Dumitru Ceara wrote:
On 4/14/23 15:10, Mark Michelson 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 conntack is no longer skipped for traffic

typo: 'conntack'

   involving the localnet port.

Will this change behavior when both ACLs and LBs are applied to a switch
with a localnet port?

The biggest change is that localnet traffic will be sent to conntrack in the PRE_STATEFUL ingress/egress stages, with the intent of defragmenting the traffic. This also will happen if LBs are applied but *no* ACLs are defined on the logical switch.

As far as behavior changes are concerned, this will result in more conntrack entries being created, but it should not affect how traffic is handled when localnet ports are involved.



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.

I'm ok as co-author and, to keep the robot happy, if we decide to apply
this to main:

Signed-off-by: Dumitru Ceara <dce...@redhat.com>

---
  controller/ovn-controller.c | 13 ++++----
  northd/northd.c             | 25 +++++++++++-----
  tests/ovn-northd.at         | 60 +++++++++++++++++++++++++++++++++++++

Would it be possible to add a system test too?

Can do!


Thanks,
Dumitru

  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
+])


_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to