Skip to content

geolocation: remove CLI flags for targets, parents, and allowed keys#3462

Merged
ben-dz merged 1 commit intomainfrom
bdz/geolocation-remove-addl-cli
Apr 6, 2026
Merged

geolocation: remove CLI flags for targets, parents, and allowed keys#3462
ben-dz merged 1 commit intomainfrom
bdz/geolocation-remove-addl-cli

Conversation

@ben-dz
Copy link
Copy Markdown
Contributor

@ben-dz ben-dz commented Apr 6, 2026

Resolves: #3453

Summary of Changes

  • Remove --additional-parent, --additional-targets, --additional-icmp-targets, --allowed-pubkeys from geoprobe-agent now that onchain configuration is working
  • Remove --additional-child-probes from telemetry-agent
  • Remove all CLI-to-onchain merge logic from discovery layers — targets, parents, and allowed keys now come exclusively from onchain state
  • Remove ParseProbeAddresses and ParseICMPProbeAddresses (no longer needed)
  • E2E tests adopt geolocation: add ICMP target E2E test and refactor geoprobe container management #3454's refactored helpers (which already removed CLI flag usage)

Depends on: #3454 (e2e container refactoring). The e2e/geoprobe_test.go file is taken from #3454's head to avoid conflicts at merge time.

Diff Breakdown

Category Files Lines (+/-) Net
Core logic 9 +37 / -415 -378
Tests 6 +87 / -527 -440
E2E (#3454) 1 +159 / -174 -15

Heavily deletion-weighted: removes ~800 lines of CLI plumbing and merge logic that onchain discovery makes unnecessary.

Key files (click to expand)

Testing Verification

  • Unit tests pass for geoprobe and telemetry packages (including updated tests for discovery, coordinator, config validation)
  • TestE2E_GeoprobeOnchainTargets passes end-to-end (parent discovery, outbound TWAMP + ICMP measurement, inbound signed TWAMP probing) with onchain-only configuration
  • E2E test build verified with -tags e2e

@ben-dz ben-dz requested a review from nikw9944 April 6, 2026 13:51
@ben-dz ben-dz force-pushed the bdz/geolocation-remove-addl-cli branch 2 times, most recently from bf4c6d8 to 2ead31a Compare April 6, 2026 17:46
@ben-dz ben-dz marked this pull request as ready for review April 6, 2026 17:48
Copy link
Copy Markdown
Contributor

@nikw9944 nikw9944 left a comment

Choose a reason for hiding this comment

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

Review Summary

Clean removal of CLI override flags (--additional-parent, --additional-targets, --additional-icmp-targets, --allowed-pubkeys, --additional-child-probes) from the geoprobe-agent and telemetry-agent, shifting all configuration to onchain state via parent and target discovery.

The PR is well-scoped: 17 files changed, ~1087 lines removed, ~123 added. All references to the removed flags, struct fields (CLIParents, CLITargets, CLIIcmpTargets, CLIAllowedKeys, InitialChildGeoProbes, InitialProbes), parsing functions (parseParentDZD, parseAllowedPubkeys, ParseProbeAddresses, ParseICMPProbeAddresses), and merge helpers (mergeProbes, mergeKeys) are fully cleaned up — no stale references remain.

Verification

  • All tests pass: geoprobe-agent (0.28s), geoprobe internal (105s), telemetry (9.1s)
  • Both binaries compile cleanly
  • Grep across the telemetry tree confirms zero remaining references to any removed identifiers

Notes

  • The signed TWAMP reflector now starts with nil authorized keys and gets populated solely via SetAuthorizedKeys from the target discovery channel — this is correct since the reflector's LinuxReflector handles nil gracefully.
  • The collector.go simplification (gating geoprobe coordinator on GeolocationClient != nil instead of also checking InitialChildGeoProbes) is a clean consequence of the removal and correctly flattens the previously nested conditional.
  • Test updates properly adapt to the new initialization model (e.g., TestCoordinator_HandleProbeUpdate_Remove now seeds probes via handleProbeUpdate instead of InitialProbes).
  • CHANGELOG entry is accurate and complete.

No issues found.

@ben-dz ben-dz force-pushed the bdz/geolocation-remove-addl-cli branch from 2ead31a to ffaa31b Compare April 6, 2026 18:32
@ben-dz ben-dz enabled auto-merge (squash) April 6, 2026 18:33
Remove --additional-parent, --additional-targets, --additional-icmp-targets,
and --allowed-pubkeys from geoprobe-agent, and --additional-child-probes from
telemetry-agent. All configuration now comes from onchain state.
@ben-dz ben-dz force-pushed the bdz/geolocation-remove-addl-cli branch from ffaa31b to 4d5b0fe Compare April 6, 2026 18:35
@ben-dz ben-dz merged commit 124b2f2 into main Apr 6, 2026
33 checks passed
@ben-dz ben-dz deleted the bdz/geolocation-remove-addl-cli branch April 6, 2026 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

geolocation: remove CLI target/parent/child options

2 participants