Conversation
Drop the range byte from the v2 message format to match the updated protocol. Replace interface enumeration (getifaddrs) with a UDP socket probe to Google DNS for detecting the public IPv6 address. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch back to v1 message format (no family byte). Inline message generation methods, add proper global IPv6 validation to reject loopback, link-local, site-local, multicast, v4-mapped, and ULA addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
antondalgren
changed the title
|
Isn't this two PRs? One for removing v2 and one for changing the "CLI" interface? |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
antondalgren
force-pushed
the
client-v2-only
branch
from
6a651d5 to
05af6ba
Compare
There was a problem hiding this comment.
Pull request overview
This PR simplifies the Sparoid Ruby client by removing the v2 message format and extending the v1 format to support both IPv4 (4-byte) and IPv6 (16-byte) IP addresses. The server was updated to accept the variable-length v1 format, enabling the client to drop the separate v2 code path. The IPv6 source address detection was also improved: instead of enumerating network interfaces, it now queries the OS via a connected UDP socket to determine the correct source IPv6 address (more reliable on multi-homed hosts).
Changes:
- The
messagemethod's pack format changed from fixeda4toa*, allowing it to handle both IPv4 (4 bytes) and IPv6 (16 bytes) addresses without a separate v2 method. generate_messageswas simplified to produce a single message per IP, replacing the two-format (v1 + v2) approach for IPv4.- IPv6 source address detection was replaced from interface enumeration (
Socket.getifaddrs) to a connected UDP socket approach (public_ipv6_by_udp), which more reliably identifies the preferred outbound IPv6 address.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/sparoid.rb |
Removes message_v2, create_messages, and generate_public_ip_messages; updates message to support both IPv4 and IPv6; replaces public_ipv6_with_range with public_ipv6_by_udp; updates global_ipv6? to check ipv6_unspecified? and broaden ULA prefix exclusion |
test/sparoid_test.rb |
Removes v2-specific tests; adds renamed test_message_ipv4, test_message_ipv6, test_generate_messages_ipv4, and test_generate_messages_ipv6 tests reflecting the simplified format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
lib/sparoid.rb
Outdated
| def global_ipv6?(addr) | ||
| !(addr.ipv6_loopback? || addr.ipv6_linklocal? || addr.ipv6_unspecified? || | ||
| addr.ipv6_sitelocal? || addr.ipv6_multicast? || addr.ipv6_v4mapped? || | ||
| addr.ip_address.start_with?("fd")) |
There was a problem hiding this comment.
The global_ipv6? method filters out ULA (Unique Local Address) addresses using addr.ip_address.start_with?("fd"). Per RFC 4193, the full ULA range is fc00::/7, which covers both fc:: and fd:: prefixes. The fc::/8 sub-range (where the locally assigned bit is 0) is theoretically part of ULA, though it is currently unassigned in practice. The check should also exclude addresses starting with "fc" for correctness. The previous code checked "fd00", which was also too narrow (only the fd00::/16 sub-range). Updating the check to cover both "fc" and "fd" would be more complete.
| if native_ipv6 | ||
| ips = ips.grep_v(Resolv::IPv6) | ||
| ips << Resolv::IPv6.create(native_ipv6) |
There was a problem hiding this comment.
If I get this right, we use this IP instead of the one we get via the public lookup?
maybe a bit out of scope for this PR, but I wonder if we should not have a separate ipv4 and ipv6 paths instead of the first pass with ipv4 and ipv6 only to potentially throw away the ipv6 result here 😅
lib/sparoid.rb
Outdated
| def global_ipv6?(addr) | ||
| !(addr.ipv6_loopback? || addr.ipv6_linklocal? || addr.ipv6_unspecified? || | ||
| addr.ipv6_sitelocal? || addr.ipv6_multicast? || addr.ipv6_v4mapped? || | ||
| addr.ip_address.start_with?("fd")) |
WHY are these changes introduced?
The server software was simplified to allow message v1 format to have a IP of 16 bytes. Which means that we can simplify the ruby client by removing that message version and revert back to v1 and include the ipv6 address in that format.
WHAT is this pull request doing?
HOW was this pull request tested?
Specs and ran manually towards a server with sparoid server 2.0.0