On 7/9/26 8:19 AM, Han Zhou wrote:
> A /31 point-to-point link has no broadcast address; both addresses in
> the range are usable hosts. However, ovn-northd computed a traditional
> subnet broadcast for the router port and included it in the priority-100
> lr_in_ip_input L3 admission control drop flow (ip4.src == {...}). On a
> /31, that "broadcast" address is the peer, so all traffic originating
> from the /31 peer was dropped.
>
> Skip the broadcast address in the admission control drop set for /31
> networks so that /31 router ports work as expected.
>
> Fixes: 936b640f4934 ("ovn: Implement basic logical L3 routing.")
> Assisted-by: Claude Opus 4.8, Cursor
> Signed-off-by: Han Zhou <[email protected]>
> ---
Hi Han,
Thanks for the patch!
> v1 -> v2: rebase on main
>
> Documentation/ref/ovn-logical-flows.7.rst | 2 ++
> NEWS | 4 ++++
> northd/northd.c | 6 ++++-
> tests/ovn-northd.at | 27 +++++++++++++++++++++++
> 4 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ref/ovn-logical-flows.7.rst
> b/Documentation/ref/ovn-logical-flows.7.rst
> index 3d9936414f25..61a486a98301 100644
> --- a/Documentation/ref/ovn-logical-flows.7.rst
> +++ b/Documentation/ref/ovn-logical-flows.7.rst
> @@ -2273,6 +2273,8 @@ contains the following flows to implement very basic IP
> host functionality.
> ``REGBIT_EGRESS_LOOPBACK``.
>
> - ``ip4.src`` is the broadcast address of any IP network known to the
> router.
> + Point-to-point (``/31``, RFC 3021) networks have no broadcast address and
> + are excluded, so that traffic from a ``/31`` peer is not dropped.
>
> - A priority-100 flow parses DHCPv6 replies from IPv6 prefix delegation
> routers
> (``udp.src == 547 && udp.dst == 546``). The ``handle_dhcpv6_reply`` is
> used to
> diff --git a/NEWS b/NEWS
> index d1a6ad27fea8..360c32d4b1f5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -66,6 +66,10 @@ Post v26.03.0
> (type 11) and Parameter Problem (type 12) - generated by an external
> router are un-NATed correctly. This makes Path MTU discovery and
> traceroute work through stateless NAT.
> + - Logical router ports configured with a /31 (RFC 3021 point-to-point)
> + network are now supported. A /31 has no broadcast address, so the L3
> + admission control no longer drops traffic from the /31 peer as a
> + broadcast source.
I'd argue that this is a bug fix and we wouldn't need a news item.
>
> OVN v26.03.0 - xxx xx xxxx
> --------------------------
> diff --git a/northd/northd.c b/northd/northd.c
> index 62270893ab41..8f75fd860c5b 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -13146,7 +13146,11 @@ op_put_v4_networks(struct ds *ds, const struct
> ovn_port *op, bool add_bcast)
> ds_put_cstr(ds, "{");
> for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
While at it, can we also replace "int i" with "size_t i" here?
> ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s);
> - if (add_bcast) {
> + /* A /31 point-to-point link (RFC 3021) has no broadcast address:
> + * both addresses in the range are usable hosts. Including the
> + * computed "broadcast" here would drop legitimate traffic from the
> + * /31 peer, so skip it for /31 networks. */
> + if (add_bcast && op->lrp_networks.ipv4_addrs[i].plen != 31) {
> ds_put_format(ds, "%s, ",
> op->lrp_networks.ipv4_addrs[i].bcast_s);
> }
It's a bit weird but it seems that indeed we don't need any ipv6
changes, is that correct?
> }
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index f82902c61da5..420328e116d9 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2476,6 +2476,33 @@ OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([router LRP /31 L3 admission control])
> +ovn_start
> +
> +# A /31 point-to-point link (RFC 3021) has no broadcast address; both
> +# addresses of the /31 are usable hosts. The L3 admission control flow must
> +# therefore not drop the computed "broadcast" address, otherwise traffic from
> +# the /31 peer is dropped.
> +check ovn-nbctl lr-add lr
> +check ovn-nbctl lrp-add lr lrp31 00:00:00:00:00:01 10.0.0.0/31
> +check ovn-nbctl lrp-add lr lrp24 00:00:00:00:00:03 10.0.2.1/24
> +
> +check ovn-nbctl --wait=sb sync
> +ovn-sbctl dump-flows lr > lrflows
> +AT_CAPTURE_FILE([lrflows])
> +
> +# The /31 peer (10.0.0.1) must not appear as a dropped broadcast source;
> +# only the wider /24 network keeps its broadcast address.
> +AT_CHECK([grep "lr_in_ip_input" lrflows | grep "priority=100" | grep "reg9"
> | ovn_strip_lflows], [0], [dnl
> + table=??(lr_in_ip_input ), priority=100 , match=(ip4.src ==
> {10.0.0.0} && reg9[[0]] == 0), action=(drop;)
> + table=??(lr_in_ip_input ), priority=100 , match=(ip4.src ==
> {10.0.2.1, 10.0.2.255} && reg9[[0]] == 0), action=(drop;)
> +])
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
The pipeline is complex so this specific flow check might not be enough
to ensure we don't break the feature in the end.
Would it be possible to add a small test in ovn.at that actually injects
a packet that traverses a /31 router port link?
> # This test case tests that when a logical switch has load balancers
> associated
> # (with VIPs configured), the below logical flow is added by ovn-northd.
> # table=ls_out_pre_lb, priority=100, match=(ip), action=(reg0[[0]] = 1;
> next;)
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
