-
Notifications
You must be signed in to change notification settings - Fork 6
feat(cli): remove one tunnel restriction (#2725) #2771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables simultaneous unicast (IBRL) and multicast tunnels from a single client by creating separate user accounts per tunnel type. Previously, only one tunnel was allowed per client IP.
Changes:
- Added capability helper methods in the smart contract to determine user capabilities based on state rather than UserType alone
- Modified CLI logic to create separate user accounts (PDAs) for different tunnel types on the same client IP
- Enhanced device selection to exclude IPs already used by the client's existing tunnels
- Extended E2E tests to verify concurrent tunnel scenarios
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| smartcontract/programs/doublezero-serviceability/src/state/user.rs | Added capability helper methods to determine user state and comprehensive tests for tunnel state detection |
| smartcontract/programs/doublezero-serviceability/src/processors/accesspass/set.rs | Added test to verify rent allocation constant covers concurrent publisher+subscriber configurations |
| e2e/multicast_test.go | Removed test that enforced single tunnel restriction |
| e2e/internal/devnet/client.go | Added constants for user types and method to wait for multiple tunnels |
| e2e/internal/arista/commands.go | Enhanced documentation for PIM neighbor JSON structures |
| e2e/ibrl_test.go | Removed test that enforced single tunnel restriction |
| e2e/ibrl_multicast_coexistence_test.go | Re-enabled coexistence tests and added new single-client concurrent tunnel tests |
| client/doublezero/src/dzd_latency.rs | Added IP exclusion parameter to device selection to ensure concurrent tunnels use different devices |
| client/doublezero/src/command/status.rs | Updated latency test values to reflect new tolerance |
| client/doublezero/src/command/connect.rs | Refactored user lookup by UserType, added logic to create separate user accounts for concurrent tunnels |
| CHANGELOG.md | Added entry documenting removal of single tunnel constraint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| require.NoError(t, err) | ||
| log.Info("--> Device added", "deviceID", device.ID) | ||
|
|
||
| // Add second device for multicast (separate device required for concurrent tunnels) |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "separate device required for concurrent tunnels" but this requirement isn't enforced in the code. The second device is added but the actual device selection happens through latency-based selection. Consider clarifying whether this is truly required or just preferred for the test setup.
| // Add second device for multicast (separate device required for concurrent tunnels) | |
| // Add a second device primarily for multicast; a separate device is preferred for concurrent tunnels, but actual device selection is latency-based. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code in connect.rs with best_latency does the device exclusion
| if snippet != lastConfigSnippet { | ||
| log.Info("Agent config does not yet include client", | ||
| "clientIP", client.CYOANetworkIP, | ||
| "deviceCode", device.Spec.Code, | ||
| "deviceID", device.ID, | ||
| "configLen", configLen, | ||
| "configSnippet", snippet) | ||
| lastConfigSnippet = snippet | ||
| } else { | ||
| log.Debug("Agent config does not yet include client", "clientIP", client.CYOANetworkIP) | ||
| } |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to prevent duplicate logging could be simplified by moving the condition check earlier or extracting it into a helper function to reduce nesting and improve readability.
| if snippet != lastConfigSnippet { | |
| log.Info("Agent config does not yet include client", | |
| "clientIP", client.CYOANetworkIP, | |
| "deviceCode", device.Spec.Code, | |
| "deviceID", device.ID, | |
| "configLen", configLen, | |
| "configSnippet", snippet) | |
| lastConfigSnippet = snippet | |
| } else { | |
| log.Debug("Agent config does not yet include client", "clientIP", client.CYOANetworkIP) | |
| } | |
| if snippet == lastConfigSnippet { | |
| log.Debug("Agent config does not yet include client", "clientIP", client.CYOANetworkIP) | |
| return false | |
| } | |
| log.Info("Agent config does not yet include client", | |
| "clientIP", client.CYOANetworkIP, | |
| "deviceCode", device.Spec.Code, | |
| "deviceID", device.ID, | |
| "configLen", configLen, | |
| "configSnippet", snippet) | |
| lastConfigSnippet = snippet |
| } | ||
| let lowest_latency_device = | ||
| match best_latency(controller, &devices, true, None, current_device).await { | ||
| match best_latency(controller, &devices, true, None, current_device, &[]).await { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a bug in here when reporting the status output. The onchain data says my IBRL tunnel is connected to dz2 but in the doublezero status output, the current device keeps flipping between dz1 and dz2 as well as the metro field. Smells like map keys somewhere that we're assuming are ordered. Also, the multicast tunnel isn't reporting a connected device (N/A):
root@client-41JaXfSxFmuggZSxJrrHyG7fQ75hKHJF2RFbpFqq27HJ:/# doublezero user list
account | user_type | groups | device | location | cyoa_type | client_ip | dz_ip | accesspass | tunnel_id | tunnel_net | status | owner
F9i9HWuaMG6fcRpwGE85sPYFtyATv9MeeaTj9cjBeASu | Multicast | S:demo | dz1 | Amsterdam | GREOverDIA | 9.169.90.110 | 9.169.90.110 | Prepaid: (MAX) | 500 | 169.254.0.4/31 | activated | 41JaXfSxFmuggZSxJrrHyG7fQ75hKHJF2RFbpFqq27HJ
CvshkNksx7PqNCeMxA3zn2zUL55BskYAszJvkoRhpVLY | IBRL | | dz2 | Los Angeles | GREOverDIA | 9.169.90.110 | 9.169.90.110 | Prepaid: (MAX) | 501 | 169.254.0.0/31 | activated | 41JaXfSxFmuggZSxJrrHyG7fQ75hKHJF2RFbpFqq27HJ
root@client-41JaXfSxFmuggZSxJrrHyG7fQ75hKHJF2RFbpFqq27HJ:/# doublezero status
Tunnel Status | Last Session Update | Tunnel Name | Tunnel Src | Tunnel Dst | Doublezero IP | User Type | Current Device | Lowest Latency Device | Metro | Network
BGP Session Up | 2026-02-03 15:20:16 UTC | doublezero0 | 9.169.90.110 | 9.169.90.16 | 9.169.90.110 | IBRL | dz1 | ✅ dz1 | Amsterdam | local
BGP Session Up | 2026-02-03 15:23:22 UTC | doublezero1 | 9.169.90.110 | 9.169.90.8 | | Multicast | N/A | ✅ dz1 | N/A | local
root@client-41JaXfSxFmuggZSxJrrHyG7fQ75hKHJF2RFbpFqq27HJ:/# doublezero status
Tunnel Status | Last Session Update | Tunnel Name | Tunnel Src | Tunnel Dst | Doublezero IP | User Type | Current Device | Lowest Latency Device | Metro | Network
BGP Session Up | 2026-02-03 15:20:16 UTC | doublezero0 | 9.169.90.110 | 9.169.90.16 | 9.169.90.110 | IBRL | dz2 | ✅ dz2 | Los Angeles | local
BGP Session Up | 2026-02-03 15:23:22 UTC | doublezero1 | 9.169.90.110 | 9.169.90.8 | | Multicast | N/A | ✅ dz1 | N/A | local
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch - indeed a bug
This reverts commit d869be3.
Summary of Changes
This PR adds the ability from the CLI and the smart contract to add simultaneous tunnels: a single unicast and a single mulitcast tunnel (subscriber or publisher only). This also ensures that a second tunnel appears on a second device.
This PR is split out from #2728 so that we can roll this out more safely and in two discrete steps (cli changes then activator,controller changes)
Closes #2725
Testing Verification
pubkeyis always the last deserialized field in both go and rust