Skip to content

[client] Use native facilities to add routes on BSD and Windows #3862

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

lixmal
Copy link
Contributor

@lixmal lixmal commented May 22, 2025

Describe your changes

  • Use syscalls to add/remove routes on Windows
  • Use the routing socket to add/remove routes on BSD variants
  • Replace net.IP and net.IPNet with netip.Addr and netip.Prefix in wgaddr.Address
  • Fix some tests for FreeBSD
  • Remove "add route for gateway" and "duplicate route" logic
  • Add allowed route validation
  • Add better tests

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

@Copilot Copilot AI review requested due to automatic review settings May 22, 2025 11:04
Copy link
Contributor

@Copilot Copilot AI left a 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 replaces use of external route commands on Windows with direct IP Helper syscalls to manage routing entries.

  • Introduces addRoute, deleteRoute, and helper functions to set up MIB_IPFORWARD_ROW2 via Windows APIs
  • Removes exec-based command logic and related imports
  • Adds syscall wrappers for creating, deleting, and querying route entries
Comments suppressed due to low confidence (3)

client/internal/routemanager/systemops/systemops_windows.go:3

  • New code uses fmt.Errorf, binary.BigEndian, and unsafe.Pointer but the imports for "fmt", "encoding/binary", and "unsafe" are missing. Please add these imports to ensure the file compiles.
import (

client/internal/routemanager/systemops/systemops_windows.go:327

  • [nitpick] The function name uses the acronym "LUID" while the type is declared as lowercase luid. Consider renaming the type to LUID or the function to convertInterfaceIndexToLuid for consistency with Go naming conventions.
func convertInterfaceIndexToLUID(interfaceIndex uint32, interfaceLUID *luid) error {

client/internal/routemanager/systemops/systemops_windows.go:192

  • New addRoute (and related syscall wrappers) lack unit tests. Consider adding tests or mocks to verify both successful execution paths and error handling.
func addRoute(prefix netip.Prefix, nexthop Nexthop) (err error) {

@lixmal lixmal force-pushed the improve-windows-client-routes branch 3 times, most recently from 13b4c2b to e802c69 Compare May 22, 2025 22:03
@lixmal lixmal force-pushed the improve-windows-client-routes branch from e802c69 to 79b6efc Compare May 22, 2025 22:51
@lixmal lixmal force-pushed the improve-windows-client-routes branch from ee29432 to 0dc0962 Compare May 23, 2025 19:26
@lixmal lixmal force-pushed the improve-windows-client-routes branch 6 times, most recently from 482ab2d to 824ee96 Compare May 24, 2025 22:06
@lixmal lixmal changed the title [client] Use windows syscalls to configure routes [client] Use native facilities to add routes on BSD and Windows May 24, 2025
@lixmal lixmal force-pushed the improve-windows-client-routes branch from 824ee96 to 442330f Compare May 24, 2025 23:11
@lixmal lixmal force-pushed the improve-windows-client-routes branch from 442330f to 4edcaae Compare May 24, 2025 23:23
@lixmal lixmal force-pushed the improve-windows-client-routes branch from 4f1f05d to 9888287 Compare May 25, 2025 18:21
@lixmal lixmal force-pushed the improve-windows-client-routes branch from ffbe32d to abe8f32 Compare May 26, 2025 13:49
@lixmal lixmal force-pushed the improve-windows-client-routes branch from abe8f32 to 005cfc3 Compare May 26, 2025 14:06
@lixmal lixmal force-pushed the improve-windows-client-routes branch from 005cfc3 to f3dafbd Compare May 26, 2025 14:41
@lixmal lixmal force-pushed the improve-windows-client-routes branch from 4914ddf to 86a3cc7 Compare May 26, 2025 18:44
@lixmal lixmal force-pushed the improve-windows-client-routes branch from 86a3cc7 to 3e026a8 Compare May 26, 2025 19:26
@lixmal lixmal force-pushed the improve-windows-client-routes branch from 19bddf7 to d9b8098 Compare May 27, 2025 11:13
@pappz
Copy link
Contributor

pappz commented May 27, 2025

@lixmal, please merge the main branch to fix the unit tests.

Copy link

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.

2 participants