Hi MJ, thanks for the patch. From a pure functionality perspective, it
looks good. I have a comment about an implementation detail, and I have
a comment for the test you added. See below.
On 1/6/25 05:10, MJ Ponsonby wrote:
Modifies the rule responsible for dropping the MLF_LOCAL_ONLY packets
to only drop them if the MLF_KEEP_RA bit flag is not there.
This does also include the addition of MLF_KEEP_RA bitflag applied
if a router announcement is being sent from either a gateway
or distributed router.
This is part of an ongoing unnumbered BGP effort.
Signed-off-by: MJ Ponsonby <[email protected]>
---
controller/physical.c | 3 +-
controller/pinctrl.c | 11 ++-
include/ovn/logical-fields.h | 3 +
ovn-architecture.7.xml | 4 +-
tests/ovn.at | 152 +++++++++++++++++++++++++++++++++++
5 files changed, 169 insertions(+), 4 deletions(-)
diff --git a/controller/physical.c b/controller/physical.c
index 3ca4e0783..615a11c42 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1868,7 +1868,8 @@ consider_port_binding(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
put_drop(debug, OFTABLE_CHECK_LOOPBACK, ofpacts_p);
match_outport_dp_and_port_keys(&match, dp_key, port_key);
match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
- MLF_LOCAL_ONLY, MLF_LOCAL_ONLY);
+ MLF_LOCAL_ONLY,
+ MLF_LOCAL_ONLY | MLF_KEEP_RA);
ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160,
binding->header_.uuid.parts[0], &match,
ofpacts_p, &binding->header_.uuid);
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 032aca118..078be8b4b 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4107,6 +4107,7 @@ struct ipv6_ra_state {
struct ipv6_ra_config *config;
int64_t port_key;
int64_t metadata;
+ bool preserved;
bool delete_me;
};
@@ -4432,6 +4433,9 @@ ipv6_ra_send(struct rconn *swconn, struct ipv6_ra_state *ra)
put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
put_load(port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
put_load(1, MFF_LOG_FLAGS, MLF_LOCAL_ONLY_BIT, 1, &ofpacts);
+ if (ra->preserved) {
+ put_load(1, MFF_LOG_FLAGS, MLF_KEEP_RA_BIT, 1, &ofpacts);
+ }
struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
resubmit->in_port = OFPP_CONTROLLER;
resubmit->table_id = OFTABLE_LOG_INGRESS_PIPELINE;
@@ -4542,8 +4546,11 @@ prepare_ipv6_ras(const struct shash
*local_active_ports_ras,
* router port is connected to. The RA is injected
* into that logical switch port.
*/
- ra->port_key = peer->tunnel_key;
- ra->metadata = peer->datapath->tunnel_key;
+ ra->port_key = peer->tunnel_key;
+ ra->metadata = peer->datapath->tunnel_key;
+ ra->preserved = (!strcmp(pb->type,"l2gateway") ||
+ !strcmp(pb->type,"l3gateway") ||
+ !strcmp(pb->type,"chassisredirect"));
ra->delete_me = false;
/* pinctrl_handler thread will send the IPv6 RAs. */
diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
index 70c6b93c4..88ca5e064 100644
--- a/include/ovn/logical-fields.h
+++ b/include/ovn/logical-fields.h
@@ -87,6 +87,7 @@ enum mff_log_flags_bits {
MLF_LOCALNET_BIT = 15,
MLF_RX_FROM_TUNNEL_BIT = 16,
MLF_ICMP_SNAT_BIT = 17,
+ MLF_KEEP_RA_BIT = 18,
};
I think this bit should be renamed. Its current use is for RAs, but the
general purpose of the bit is to allow for traffic to be sent on a
localnet port when it otherwise would be blocked. In the future, we may
find other types of packets that are marked "local only" but should be
allowed on localnet ports. If that happens, then using an "RA" bit for
those packets will seem weird.
Therefore, I think RA doesn't belong in the name of the bit. Something
like "MLF_ALLOW_LOCALNET" could work instead, or
"MLF_OVERRIDE_LOCAL_ONLY" would also do the trick in case we start using
it for more than just localnet ports.
/* MFF_LOG_FLAGS_REG flag assignments */
@@ -142,6 +143,8 @@ enum mff_log_flags {
MLF_RX_FROM_TUNNEL = (1 << MLF_RX_FROM_TUNNEL_BIT),
MLF_ICMP_SNAT = (1 << MLF_ICMP_SNAT_BIT),
+
+ MLF_KEEP_RA = (1 << MLF_KEEP_RA_BIT),
};
/* OVN logical fields
diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml
index b11277bb9..5e92449e9 100644
--- a/ovn-architecture.7.xml
+++ b/ovn-architecture.7.xml
@@ -1546,7 +1546,9 @@
<p>
Table 41 matches and drops packets for which the logical input and
output ports are the same and the MLF_ALLOW_LOOPBACK flag is not
- set. It also drops MLF_LOCAL_ONLY packets directed to a localnet port.
+ set. It also drops MLF_LOCAL_ONLY packets directed to a localnet port,
+ provided they aren't RAs sent from a gateway or distributed router
+ which is checked via the presence of the bitflag MLF_KEEP_RA.
It resubmits other packets to table 42.
</p>
</li>
diff --git a/tests/ovn.at b/tests/ovn.at
index b12719895..ad6ab3d9d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -16935,6 +16935,158 @@ OVN_CLEANUP([hv1],[hv2])
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([IPv6 periodic gateway RA enabled for localnet adjacent switch ports])
+ovn_start
+
+net_add n1
+sim_add hv1
+sim_add hv2
+as hv1
+check ovs-vsctl add-br br-phys
+check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+ovn_attach n1 br-phys 192.168.0.2
+as hv2
+check ovs-vsctl add-br br-phys
+check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+ovn_attach n1 br-phys 192.168.0.3
+
+check ovn-nbctl lr-add ro -- set Logical_Router ro options:chassis="hv1"
+check ovn-nbctl lrp-add ro ro-sw 00:00:00:00:00:01
+
+check ovn-nbctl ls-add sw
+check ovn-nbctl lsp-add sw ln
+check ovn-nbctl lsp-set-addresses ln unknown
+check ovn-nbctl lsp-set-type ln localnet
+check ovn-nbctl lsp-set-options ln network_name=phys
+
+check ovn-nbctl lsp-add sw sw-ro
+check ovn-nbctl lsp-set-type sw-ro router
+check ovn-nbctl lsp-set-options sw-ro router-port=ro-sw
+check ovn-nbctl lsp-set-addresses sw-ro 00:00:00:00:00:01
+check ovn-nbctl lsp-add sw sw-p1
+check ovn-nbctl lsp-set-addresses sw-p1 "00:00:00:00:00:02 aef0::200:ff:fe00:2"
+check ovn-nbctl lsp-add sw sw-p2
+check ovn-nbctl lsp-set-addresses sw-p2 "00:00:00:00:00:03 aef0::200:ff:fe00:3"
+
+AT_CHECK([ovn-sbctl get Port_Binding ro-sw type | tr -d '\n'],[0],[l3gateway])
+
+check ovn-nbctl set Logical_Router_Port ro-sw
ipv6_ra_configs:send_periodic=true
+check ovn-nbctl set Logical_Router_Port ro-sw
ipv6_ra_configs:address_mode=slaac
+check ovn-nbctl set Logical_Router_Port ro-sw ipv6_ra_configs:max_interval=1
+check ovn-nbctl set Logical_Router_Port ro-sw ipv6_ra_configs:min_interval=1
+
+for i in 1 2 ; do
+ as hv$i
+ check ovs-vsctl -- add-port br-int hv$i-vif1 -- \
+ set interface hv$i-vif1 external-ids:iface-id=sw-p$i \
+ options:tx_pcap=hv$i/vif1-tx.pcap \
+ options:rxq_pcap=hv$i/vif1-rx.pcap \
+ ofport-request=1
+done
+
+wait_for_ports_up
+
+construct_expected_ra() {
These days it's rare for us to construct packets manually this way when
writing tests. Instead, we use the fmt_pkt() function, which uses the
scapy python library under the hood to format the packets as hex
strings. If you want to see an example of an IPv6 RA packet being
constructed using fmt_pkt(), see the "allow IPv6 RA / NA / MLD by
default" test in tests/ovn.at. For reference, in that test, the packet
is constructed like this:
expected_packet=$(fmt_pkt "
Ether(dst='${src_mac}', src='${router_mac}') /
IPv6(src='${router_ip}', dst='${src_ip}') /
ICMPv6ND_RA(chlim=255, prf=0, routerlifetime=65535) /
ICMPv6NDOptSrcLLAddr(lladdr='${router_mac}') /
ICMPv6NDOptPrefixInfo(prefix='${router_prefix}')
")
Have a look at the test for further context behind the variables
referenced there.
This is much easier to read and to maintain than constructing the packet
manually, so I encourage you to use this method instead.
+ local src_mac=000000000001
+ local dst_mac=333300000001
+ local src_addr=fe80000000000000020000fffe000001
+ local dst_addr=ff020000000000000000000000000001
+
+ local mtu=$1
+ local ra_mo=$2
+ local rdnss=$3
+ local dnssl=$4
+ local route_info=$5
+ local ra_prefix_la=$6
+
+ local slla=0101${src_mac}
+ local mtu_opt=""
+ if test $mtu != 0; then
+ mtu_opt=05010000${mtu}
+ fi
+ shift 6
+
+ local prefix=""
+ while [[ $# -gt 0 ]] ; do
+ local size=$1
+ local net=$2
+
prefix=${prefix}0304${size}${ra_prefix_la}ffffffffffffffff00000000${net}
+ shift 2
+ done
+
+ local rdnss_opt=""
+ if test $rdnss != 0; then
+ rdnss_opt=19030000ffffffff${rdnss}
+ fi
+ local dnssl_opt=""
+ if test $dnssl != 0; then
+ dnssl_opt=1f030000ffffffff${dnssl}
+ fi
+ local route_info_opt=""
+ if test $route_info != 0; then
+ route_info_opt=${route_info}
+ fi
+
+ local
ra=ff${ra_mo}ffff0000000000000000${slla}${mtu_opt}${prefix}${rdnss_opt}${dnssl_opt}${route_info_opt}
+ local icmp=8600XXXX${ra}
+
+ local ip_len=$(expr ${#icmp} / 2)
+ ip_len=$(echo "$ip_len" | awk '{printf "%0.4x\n", $0}')
+
+ local ip=60000000${ip_len}3aff${src_addr}${dst_addr}${icmp}
+ local eth=${dst_mac}${src_mac}86dd${ip}
+ local packet=${eth}
+ echo $packet >> expected
+}
+
+ra_received() {
+ $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" $1 | sed '/^ffffffffffff/d' |
wc -l
+}
+
+ra_test() {
+ interface=$1
+ shift 1
+ construct_expected_ra $@
+ intname="$interface"
+
+ for i in hv1 hv2 ; do
+ if echo "$interface" | grep -q -v "br"; then
+ intname="$i-$interface"
+ fi
+ echo $intname
+ as $i reset_pcap_file $intname $i/$interface
+
+ OVS_WAIT_WHILE([test 0 = $(ra_received $i/$interface-tx.pcap)])
+
+ $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" $i/$interface-tx.pcap >
packets
+ sed -i '/^ffffffffffff/d' packets
+
+ cat expected | cut -c -112 > expout
+ AT_CHECK([head -1 packets | cut -c -112], [0], [expout])
+
+ # Skip ICMPv6 checksum.
+ cat expected | cut -c 117- > expout
+ AT_CHECK([head -1 packets | cut -c 117-], [0], [expout])
+
+ rm -f packets
+ as $i reset_pcap_file $intname $i/$interface
+ done
+
+ rm -f expected
+}
+
+# check that RAs are sent
+ra_test vif1 0 00 0 0 0 c0
+
+# check that RAs are recived on br-phys
+ra_test br-phys 0 00 0 0 0 c0
+
+OVN_CLEANUP([hv1],[hv2])
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD([
AT_SETUP([ACL reject rule test])
AT_KEYWORDS([acl-reject])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
