Skip to content

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented Aug 5, 2025

Please review commit by commit, and refer to the individual commit messages for additional information.

The [mdlayher/arp.Dial] function strictly requires an IPv4 address to be
assigned to the target interface, returning an error otherwise, even
if not actually necessary to send a gARP. Let's relax this requirement
by manually creating a [packet.Conn] object and reimplementing a copy of
[Client.WriteTo] based on it. Even better, we could simply relax the
check upstream, but there's already an open PR for that, and the repo
has seen no updates since 2022. Still, the logic is fairly simple, and
doesn't add much extra boilerplate here.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
According to RFC5227 [1], ARP announcements should be performed
using ARP request packets, instead of reply ones. Even though
this is mostly for historical reasons, and it is unlikely to
make a practical difference, let's still make this change to
better comply with the standard.

[1]: https://www.rfc-editor.org/rfc/rfc5227.html#section-3

Suggested-by: Harsimran Pabla <hpabla@isovalent.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Currently, a new RAW socket needs to be always instantiated every time a
new gratuitous ARP or ND packet is to be sent. While this is simple and
works great in most occasions, there may be cases in which multiple
packets need to be sent out of the same interface, and having to create
a new socket every time looks suboptimal from a performance point of
view. Hence, let's extend the [gneigh.Sender] interface to also allow
creating a client that can be reused multiple times, if appropriate.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
The ARP/ND clients are only used to send packets, never to receive them.
Hence, let's configure the appropriate filters to make sure that they
get discarded upfront by the kernel, instead of unnecessarily sitting
in the read buffer forever. This is especially important now that we
can create long-lived clients.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94 giorio94 requested review from a team as code owners August 5, 2025 07:45
@giorio94 giorio94 requested a review from HadrienPatte August 5, 2025 07:45
@giorio94 giorio94 added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. area/agent Cilium agent related. labels Aug 5, 2025
@giorio94
Copy link
Member Author

giorio94 commented Aug 5, 2025

/test

Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. Thanks!

Copy link
Member

@HadrienPatte HadrienPatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vendor LGTM

@giorio94 giorio94 added this pull request to the merge queue Aug 7, 2025
Merged via the queue into cilium:main with commit 90dd3fe Aug 7, 2025
80 checks passed
@giorio94 giorio94 deleted the mio/gneigh-misc branch August 7, 2025 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent Cilium agent related. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants