Re: [ovs-dev] [PATCH ovn branch-21.06] Disable ARP/NA responders for vlan-passthru switches

2021-07-09 Thread Mark Michelson

I pushed this to branch-21.06.

On 6/29/21 12:20 PM, Ihar Hrachyshka wrote:

When vlan-passthru is on, VIFs may attach different VLAN tags. In this
case, VIFs are not guaranteed to belong to the same L2 broadcast domain.
Because of that, we don't know if a peer port on the switch has the same
tag used and should not allow the local responder to generate neighbour
traffic. Instead, pass ARP and ND requests to the peer port owner and
allow it to reply, if needed.

Signed-off-by: Ihar Hrachyshka 
Signed-off-by: Numan Siddique 
(cherry picked from commit ea57f666f6eef1eb1d578f0e975baa14c5d23ec9)
---
  northd/ovn-northd.8.xml |   6 ++-
  northd/ovn-northd.c |   4 ++
  northd/ovn_northd.dl|   6 ++-
  tests/ovn.at| 112 
  4 files changed, 124 insertions(+), 4 deletions(-)

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 407464602..21ae0ca60 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1072,8 +1072,10 @@ output;
localport ports) that are down (unless 
ignore_lsp_down is configured as true in options
column of NB_Global table of the 
Northbound
-  database), for logical ports of type virtual and for
-  logical ports with 'unknown' address set.
+  database), for logical ports of type virtual, for
+  logical ports with 'unknown' address set and for logical ports of
+  a logical switch configured with
+  other_config:vlan-passthru=true.
  

  
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c

index 3dae7bb1c..17bcede5a 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -7007,6 +7007,10 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port 
*op,
  return;
  }
  
+if (is_vlan_transparent(op->od)) {

+return;
+}
+
  for (size_t i = 0; i < op->n_lsp_addrs; i++) {
  for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; j++) {
  ds_clear(match);
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 3afa80a3b..a09aea6ee 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -3309,7 +3309,8 @@ for (CheckLspIsUp[check_lsp_is_up]) {
  ((lsp_is_up(lsp) or not check_lsp_is_up)
   or lsp.__type == "router" or lsp.__type == "localport") and
  lsp.__type != "external" and lsp.__type != "virtual" and
-not lsp.addresses.contains("unknown"))
+not lsp.addresses.contains("unknown") and
+not sw.is_vlan_transparent)
  {
  var __match = "arp.tpa == ${addr.addr} && arp.op == 1" in
  {
@@ -3359,7 +3360,8 @@ for (SwitchPortIPv6Address(.port = &SwitchPort{.lsp = 
lsp, .json_name = json_nam
 .ea = ea, .addr = addr)
   if lsp.is_enabled() and
  (lsp_is_up(lsp) or lsp.__type == "router" or lsp.__type == 
"localport") and
-lsp.__type != "external" and lsp.__type != "virtual")
+lsp.__type != "external" and lsp.__type != "virtual" and
+not sw.is_vlan_transparent)
  {
  var __match = "nd_ns && ip6.dst == {${addr.addr}, ${addr.solicited_node()}} && 
nd.target == ${addr.addr}" in
  var actions = "${if (lsp.__type == \"router\") \"nd_na_router\" else 
\"nd_na\"} { "
diff --git a/tests/ovn.at b/tests/ovn.at
index b6523c328..811a05c5a 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3169,6 +3169,118 @@ OVN_CLEANUP([hv-1],[hv-2])
  AT_CLEANUP
  ])
  
+OVN_FOR_EACH_NORTHD([

+AT_SETUP([ovn -- VLAN transparency, passthru=true, ARP responder disabled])
+ovn_start
+
+net_add net
+check ovs-vsctl add-br br-phys
+ovn_attach net br-phys 192.168.0.1
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl --wait=sb add Logical-Switch ls other_config vlan-passthru=true
+
+for i in 1 2; do
+check ovn-nbctl lsp-add ls lsp$i
+check ovn-nbctl lsp-set-addresses lsp$i "f0:00:00:00:00:0$i 10.0.0.$i"
+done
+
+for i in 1 2; do
+check ovs-vsctl add-port br-int vif$i -- set Interface vif$i 
external-ids:iface-id=lsp$i \
+  options:tx_pcap=vif$i-tx.pcap \
+  options:rxq_pcap=vif$i-rx.pcap \
+  ofport-request=$i
+done
+
+wait_for_ports_up
+
+ovn-sbctl dump-flows ls > lsflows
+AT_CAPTURE_FILE([lsflows])
+
+AT_CHECK([grep -w "ls_in_arp_rsp" lsflows | sort], [0], [dnl
+  table=16(ls_in_arp_rsp  ), priority=0, match=(1), action=(next;)
+])
+
+test_arp() {
+local inport=$1 outport=$2 sha=$3 spa=$4 tpa=$5 reply_ha=$6
+tag=8100fefe
+local 
request=${sha}${tag}08060001080006040001${sha}${spa}${tpa}
+ovs-appctl netdev-dummy/receive vif$inport $request
+echo $request >> $outport.expected
+
+local 
reply=${sha}${reply_ha}${tag}08060001080006040002${reply_ha}${tpa}${sha}${spa}
+ovs-appctl netdev-dummy/receive vif$outp

Re: [ovs-dev] [PATCH ovn branch-21.06] Disable ARP/NA responders for vlan-passthru switches

2021-06-29 Thread 0-day Robot
Bleep bloop.  Greetings Ihar Hrachyshka, 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.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Numan Siddique 
Lines checked: 204, Warnings: 1, Errors: 0


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

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH ovn branch-21.06] Disable ARP/NA responders for vlan-passthru switches

2021-06-29 Thread Ihar Hrachyshka
When vlan-passthru is on, VIFs may attach different VLAN tags. In this
case, VIFs are not guaranteed to belong to the same L2 broadcast domain.
Because of that, we don't know if a peer port on the switch has the same
tag used and should not allow the local responder to generate neighbour
traffic. Instead, pass ARP and ND requests to the peer port owner and
allow it to reply, if needed.

Signed-off-by: Ihar Hrachyshka 
Signed-off-by: Numan Siddique 
(cherry picked from commit ea57f666f6eef1eb1d578f0e975baa14c5d23ec9)
---
 northd/ovn-northd.8.xml |   6 ++-
 northd/ovn-northd.c |   4 ++
 northd/ovn_northd.dl|   6 ++-
 tests/ovn.at| 112 
 4 files changed, 124 insertions(+), 4 deletions(-)

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 407464602..21ae0ca60 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1072,8 +1072,10 @@ output;
   localport ports) that are down (unless 
   ignore_lsp_down is configured as true in options
   column of NB_Global table of the Northbound
-  database), for logical ports of type virtual and for
-  logical ports with 'unknown' address set.
+  database), for logical ports of type virtual, for
+  logical ports with 'unknown' address set and for logical ports of
+  a logical switch configured with
+  other_config:vlan-passthru=true.
 
   
 
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3dae7bb1c..17bcede5a 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -7007,6 +7007,10 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port 
*op,
 return;
 }
 
+if (is_vlan_transparent(op->od)) {
+return;
+}
+
 for (size_t i = 0; i < op->n_lsp_addrs; i++) {
 for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; j++) {
 ds_clear(match);
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 3afa80a3b..a09aea6ee 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -3309,7 +3309,8 @@ for (CheckLspIsUp[check_lsp_is_up]) {
 ((lsp_is_up(lsp) or not check_lsp_is_up)
  or lsp.__type == "router" or lsp.__type == "localport") and
 lsp.__type != "external" and lsp.__type != "virtual" and
-not lsp.addresses.contains("unknown"))
+not lsp.addresses.contains("unknown") and
+not sw.is_vlan_transparent)
 {
 var __match = "arp.tpa == ${addr.addr} && arp.op == 1" in
 {
@@ -3359,7 +3360,8 @@ for (SwitchPortIPv6Address(.port = &SwitchPort{.lsp = 
lsp, .json_name = json_nam
.ea = ea, .addr = addr)
  if lsp.is_enabled() and
 (lsp_is_up(lsp) or lsp.__type == "router" or lsp.__type == 
"localport") and
-lsp.__type != "external" and lsp.__type != "virtual")
+lsp.__type != "external" and lsp.__type != "virtual" and
+not sw.is_vlan_transparent)
 {
 var __match = "nd_ns && ip6.dst == {${addr.addr}, 
${addr.solicited_node()}} && nd.target == ${addr.addr}" in
 var actions = "${if (lsp.__type == \"router\") \"nd_na_router\" else 
\"nd_na\"} { "
diff --git a/tests/ovn.at b/tests/ovn.at
index b6523c328..811a05c5a 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3169,6 +3169,118 @@ OVN_CLEANUP([hv-1],[hv-2])
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- VLAN transparency, passthru=true, ARP responder disabled])
+ovn_start
+
+net_add net
+check ovs-vsctl add-br br-phys
+ovn_attach net br-phys 192.168.0.1
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl --wait=sb add Logical-Switch ls other_config vlan-passthru=true
+
+for i in 1 2; do
+check ovn-nbctl lsp-add ls lsp$i
+check ovn-nbctl lsp-set-addresses lsp$i "f0:00:00:00:00:0$i 10.0.0.$i"
+done
+
+for i in 1 2; do
+check ovs-vsctl add-port br-int vif$i -- set Interface vif$i 
external-ids:iface-id=lsp$i \
+  options:tx_pcap=vif$i-tx.pcap \
+  options:rxq_pcap=vif$i-rx.pcap \
+  ofport-request=$i
+done
+
+wait_for_ports_up
+
+ovn-sbctl dump-flows ls > lsflows
+AT_CAPTURE_FILE([lsflows])
+
+AT_CHECK([grep -w "ls_in_arp_rsp" lsflows | sort], [0], [dnl
+  table=16(ls_in_arp_rsp  ), priority=0, match=(1), action=(next;)
+])
+
+test_arp() {
+local inport=$1 outport=$2 sha=$3 spa=$4 tpa=$5 reply_ha=$6
+tag=8100fefe
+local 
request=${sha}${tag}08060001080006040001${sha}${spa}${tpa}
+ovs-appctl netdev-dummy/receive vif$inport $request
+echo $request >> $outport.expected
+
+local 
reply=${sha}${reply_ha}${tag}08060001080006040002${reply_ha}${tpa}${sha}${spa}
+ovs-appctl netdev-dummy/receive vif$outport $reply
+echo $reply >> $inport.expected
+}
+
+test_arp 1 2 f001 0a01 0a02 f002
+te