geolocation: add ICMP pinger to geoprobe-agent

[ben-dz]

Summary of Changes

• Add an ICMP echo pinger (ICMPPinger) to geoprobe-agent that measures outbound ICMP targets using raw sockets with kernel timestamps (SO_TIMESTAMPNS), interleaved batch send/receive, and epoll-based I/O
• Integrate ICMP targets into the existing measurement loop and target discovery pipeline, discovering OutboundIcmp targets from onchain data alongside TWAMP targets
• Refactor the measurement loop from a standalone function to a measurementLoop struct with a shared reconcileTargets helper that deduplicates the add/remove/measure-new-targets logic for both TWAMP and ICMP channels

Diff Breakdown

Category	Files	Lines (+/-)	Net
Core logic	4	+620 / -104	+516
Tests	4	+737 / -21	+716
Metrics	1	+33 / -15	+18
Config/build	1	+1 / -0	+1
Docs	1	+2 / -0	+2

Most of the new code is the ICMP pinger and its tests; the measurement loop refactor is net-neutral.

Key files (click to expand)

• controlplane/telemetry/internal/geoprobe/icmp_pinger.go — new ICMPPinger with batch send/receive, MeasureOne, MeasureAll, probe lifecycle
• controlplane/telemetry/internal/geoprobe/icmp_pinger_test.go — mock-based unit tests + CAP_NET_RAW integration tests for the pinger
• controlplane/telemetry/cmd/geoprobe-agent/main.go — refactor to measurementLoop struct, wire ICMP pinger + targets, add --additional-icmp-targets flag
• controlplane/telemetry/internal/geoprobe/icmp_conn.go — raw ICMP socket with epoll, kernel timestamp parsing, IPv4 header stripping
• controlplane/telemetry/internal/geoprobe/icmp_conn_test.go — round-trip and edge-case tests for the raw socket layer
• controlplane/telemetry/internal/geoprobe/target_discovery.go — discover OutboundIcmp targets from onchain data, merge with CLI targets
• controlplane/telemetry/internal/geoprobe/address.go — add ValidateICMP(), ParseICMPProbeAddresses(), IPv4-only enforcement
• controlplane/telemetry/internal/metrics/geolocation_metrics.go — add ICMP target count gauge and measurement cycle duration histogram

Testing Verification

• Mock-based unit tests for ICMPPinger cover add/remove, duplicate handling, MeasureOne (success, timeout, mismatched seq/ID), MeasureAll (multi-target, empty, context cancellation), and Close
• Integration tests (gated on CAP_NET_RAW) verify real ICMP echo round-trips to localhost
• icmpConn tests cover raw socket round-trip, deadline expiry, IPv6 rejection, and IPv4 header stripping
• ValidateICMP and ParseICMPProbeAddresses tested including IPv6 rejection, deduplication, and invalid input
• Target discovery tests cover ICMP-only, mixed TWAMP+ICMP, and private-IP rejection for ICMP targets

[nikw9944]

Code review

Found 1 issue:

1. NewICMPPinger is called unconditionally at startup even when no ICMP targets are configured. It opens a raw ICMP socket requiring CAP_NET_RAW, so existing deployments that don't use ICMP and don't grant this capability will fail at startup with os.Exit(1). The pinger should only be created when ICMP targets are actually present.

doublezero/controlplane/telemetry/cmd/geoprobe-agent/main.go

Lines 530 to 540 in 05ba5e3

	// Set up ICMP pinger for outbound ICMP targets.
	icmpPinger, err := geoprobe.NewICMPPinger(&geoprobe.ICMPPingerConfig{
	Logger: log,
	ProbeTimeout: *twampSenderTimeout,
	BatchSize: geoprobe.ICMPDefaultBatchSize,
	StaggerDelay: geoprobe.ICMPDefaultStaggerDelay,
	})
	if err != nil {
	log.Error("Failed to create ICMP pinger (CAP_NET_RAW may be missing)", "error", err)
	os.Exit(1)
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[nikw9944]

Minor: ParseICMPProbeAddresses deduplicates by addr.String() (host:port:0), but ICMPPinger.AddProbe keys by addr.Host only. If the same host appears with different offset ports (e.g. 8.8.8.8:9000,8.8.8.8:9001), the parser accepts both but the pinger silently drops the second. Likely an edge case in practice, but the key mismatch could cause confusion.

doublezero/controlplane/telemetry/internal/geoprobe/icmp_pinger.go

Lines 79 to 84 in 05ba5e3

	probes: make(map[string]*icmpProbeEntry),
	cfg: cfg,
	id: os.Getpid() & 0xffff,
	log: cfg.Logger,
	}, nil
	}

doublezero/controlplane/telemetry/internal/geoprobe/address.go

Lines 81 to 96 in 05ba5e3

	return fmt.Errorf("host %s is not a public unicast address", p.Host)
	}
	return nil
	}
	// ParseICMPProbeAddresses parses a comma-separated list of ICMP probe addresses.
	// Each entry must be host:offset_port. TWAMPPort is always 0 for ICMP targets.
	func ParseICMPProbeAddresses(s string) ([]ProbeAddress, error) {
	if s == "" {
	return nil, nil
	}
	parts := strings.Split(s, ",")
	probes := make([]ProbeAddress, 0, len(parts))
	seen := make(map[string]bool)

[nikw9944]

Minor: In discoverAndSend, the skip-scan guard checks targets == nil && inboundKeys == nil but doesn't check icmpTargets == nil. This works today since discover() returns all-nil on skip, but the guard is fragile — if discover() is later refactored to return non-nil TWAMP targets but nil ICMP targets, the guard won't catch it. Consider adding icmpTargets == nil to the condition for consistency.

doublezero/controlplane/telemetry/internal/geoprobe/target_discovery.go

Lines 105 to 108 in 05ba5e3

	if targets == nil && inboundKeys == nil {
	return
	}

[nikw9944]

I think this can lead to bad offset data. Claude:

uint64(rxTime.Sub(pp.txTime).Nanoseconds()) in readReplies and MeasureOne wraps to ~MaxUint64 (~18 exaseconds) if clock skew or an anomalous kernel timestamp produces a negative duration. The existing TWAMP pinger guards against this with decideRTT which clamps to max(rtt, 0) before casting — the ICMP path has no equivalent.

The impact cascades: the bogus MeasuredRttNs overflows RttNs in sendCompositeOffsets to a small positive value, which then wins offsetCache.GetBest comparisons (evicting legitimate offsets), gets Ed25519-signed, and is embedded in every outbound Signed TWAMP reply — propagating bad data to downstream targets until the cache entry expires.

doublezero/controlplane/telemetry/internal/geoprobe/icmp_pinger.go

Lines 178 to 180 in 05ba5e3

	rtt := uint64(rxTime.Sub(pp.txTime).Nanoseconds())
	results[pp.addr] = rtt

doublezero/controlplane/telemetry/internal/geoprobe/icmp_pinger.go

Lines 239 to 241 in 05ba5e3

	rtt := uint64(rxTime.Sub(txTime).Nanoseconds())
	p.log.Debug("MeasureOne succeeded", "host", addr.Host, "rtt_ns", rtt)

[nikw9944]

Please see PR comments

[ben-dz]

Code review

Found 1 issue:

1. NewICMPPinger is called unconditionally at startup even when no ICMP targets are configured. It opens a raw ICMP socket requiring CAP_NET_RAW, so existing deployments that don't use ICMP and don't grant this capability will fail at startup with os.Exit(1). The pinger should only be created when ICMP targets are actually present.

This is on purpose. Failing later is worse than failing earlier. It should just fail out so we fix the permissions.

[ben-dz]

@nikw9944 : Addressed 2, 3, 4. 1 was intentional.