Acked-by: Mark Michelson

I have a small nit below, but since it's strictly with a comment, it's easy for whoever merges this to correct the comment when they do it. There's no reason to spin up a new version of this.

On 5/4/21 1:59 PM, Lorenzo Bianconi wrote:
Since the localnet port is available on each hv, do not forward traffic
to the localnet port if it is present in order to avoid switch fdb
misconfiguration.
Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1942877

Signed-off-by: Lorenzo Bianconi <[email protected]>
---
  controller/physical.c        | 23 +++++++++++++++++++++++
  include/ovn/logical-fields.h |  4 ++++
  tests/ovn.at                 | 17 +++++++++++++++++
  3 files changed, 44 insertions(+)

diff --git a/controller/physical.c b/controller/physical.c
index 96c959d18..258842634 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1193,6 +1193,11 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
load_logical_ingress_metadata(binding, &zone_ids, ofpacts_p); + if (!strcmp(binding->type, "localport")) {
+            /* mark the packet as incoming from a localport */
+            put_load(1, MFF_LOG_FLAGS, MLF_LOCALPORT_BIT, 1, ofpacts_p);
+        }
+
          /* Resubmit to first logical ingress pipeline table. */
          put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
          ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG,
@@ -1251,6 +1256,24 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
                                                ofport, flow_table);
          }
+ /* Table 34, priority 160.

Nit: Table 34 is incorrect here. OFTABLE_CHECK_LOOPBACK is table 39 now. I think the issue here is that when the tables shifted, nobody updated the comments in this file, and so it's wrong here and several other places in physical.c

+         * =======================
+         *
+         * Do not forward local traffic from a localport to a localnet port.
+         */
+        if (!strcmp(binding->type, "localnet")) {
+            /* do not forward traffic from localport to localnet port */
+            match_init_catchall(&match);
+            ofpbuf_clear(ofpacts_p);
+            match_set_metadata(&match, htonll(dp_key));
+            match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key);
+            match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
+                                 MLF_LOCALPORT, MLF_LOCALPORT);
+            ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160,
+                            binding->header_.uuid.parts[0], &match,
+                            ofpacts_p, &binding->header_.uuid);
+        }
+
      } else if (!tun && !is_ha_remote) {
          /* Remote port connected by localnet port */
          /* Table 33, priority 100.
diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
index d44b30b30..ef97117b9 100644
--- a/include/ovn/logical-fields.h
+++ b/include/ovn/logical-fields.h
@@ -67,6 +67,7 @@ enum mff_log_flags_bits {
      MLF_LOOKUP_LB_HAIRPIN_BIT = 7,
      MLF_LOOKUP_FDB_BIT = 8,
      MLF_SKIP_SNAT_FOR_LB_BIT = 9,
+    MLF_LOCALPORT_BIT = 10,
  };
/* MFF_LOG_FLAGS_REG flag assignments */
@@ -107,6 +108,9 @@ enum mff_log_flags {
      /* Indicate that a packet must not SNAT in the gateway router when
       * load-balancing has taken place. */
      MLF_SKIP_SNAT_FOR_LB = (1 << MLF_SKIP_SNAT_FOR_LB_BIT),
+
+    /* Indicate the packet has been received from a localport */
+    MLF_LOCALPORT = (1 << MLF_LOCALPORT_BIT),
  };
/* OVN logical fields
diff --git a/tests/ovn.at b/tests/ovn.at
index fe6a7c85b..99764b24b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -11823,10 +11823,17 @@ OVN_FOR_EACH_NORTHD([
  AT_SETUP([ovn -- localport suppress gARP])
  ovn_start
+send_garp() {
+    local inport=$1 eth_src=$2 eth_dst=$3 spa=$4 tpa=$5
+    local 
request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
+    as hv1 ovs-appctl netdev-dummy/receive vif$inport $request
+}
+
  net_add n1
  sim_add hv1
  as hv1
  check ovs-vsctl add-br br-phys
+ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
  ovn_attach n1 br-phys 192.168.0.1
check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
@@ -11837,6 +11844,7 @@ check ovn-nbctl ls-add ls \
      -- lsp-set-addresses lp "00:00:00:00:00:01 10.0.0.1" \
      -- lsp-add ls ln \
      -- lsp-set-type ln localnet \
+    -- lsp-set-addresses ln unknown \
      -- lsp-set-options ln network_name=phys \
      -- lsp-add ls lsp \
      -- lsp-set-addresses lsp "00:00:00:00:00:02 10.0.0.2"
@@ -11870,6 +11878,15 @@ AT_CHECK([
      test 0 -eq $pkts
  ])
+spa=$(ip_to_hex 10 0 0 1)
+tpa=$(ip_to_hex 10 0 0 100)
+send_garp 1 000000000001 ffffffffffff $spa $tpa
+
+dnl traffic from localport should not be sent to localnet
+AT_CHECK([tcpdump -r hv1/br-phys_n1-tx.pcap arp[[24:4]]=0x0a000064 | wc 
-l],[0],[dnl
+0
+],[ignore])
+
  OVN_CLEANUP([hv1])
  AT_CLEANUP
  ])


_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to