Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e1f7fb7af
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if slices.Contains(services, vnetDBusServiceName) { | ||
| return nil |
There was a problem hiding this comment.
Treat activatable D-Bus names as unavailable until usable
This check returns success as soon as org.teleport.vnet1 appears in ListActivatableNames, but execAdminProcess only falls back to the root child-process path when this function returns an error. In partial/misconfigured installs (activatable name present but unit or executable broken), runService then fails at Start and sudo tsh vnet exits instead of using the intended fallback path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It's probably a good idea to consider fixing the Codex comments on master and backporting if they indeed point out real issues.
Sometimes it behaves like the worst reviewer ever where it doesn't point out all potential problems all at once, like in this PR #64769.
| if addr.IsUnspecified() || addr.IsLoopback() { | ||
| continue |
There was a problem hiding this comment.
Preserve loopback upstream DNS servers when loading Linux DNS
Filtering out all loopback DNS entries means systems that intentionally use a local resolver (for example dnsmasq/unbound on 127.0.0.1 or ::1) can end up with no upstream resolvers at all. In that case unmatched lookups hit dns.Server.forward with an empty upstream list and fail, breaking non-Teleport DNS while VNet is enabled.
Useful? React with 👍 / 👎.
ravicious
left a comment
There was a problem hiding this comment.
I built Connect using code from tangyatsu/vnet-linux-v18 and tangyatsu/vnet-linux-systemd-dbus-polkit-files-v18. I did some light testing and it seems to work.
…#64493) * replace TCP socket with unix socket for VNet user-admin IPC for Linux * review improvements and fixes
|
@tangyatsu - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes. |
c48c961 to
3113071
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3113071b8f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case <-ticker.C: | ||
| state, err := getSystemdUnitState(ctx, conn, vnetSystemdUnitName) | ||
| if err != nil { | ||
| return trace.Wrap(err, "querying systemd service %s", vnetSystemdUnitName) |
There was a problem hiding this comment.
Prioritize shutdown path when context is canceled
When ctx is canceled, this loop can still take the ticker.C branch if a tick is ready at the same time, and then getSystemdUnitState(ctx, ...) returns a canceled-context error. That exits runService before calling stopService, so tsh vnet can terminate while teleport-vnet.service is left running. This is most visible on normal shutdowns that race with the 1s poll tick.
Useful? React with 👍 / 👎.
|
Cherry-picked the commit that replaces TCP socket with Unix socket and went through the test cases again |
…linux-v18 # Conflicts: # go.mod
3113071 to
9074652
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9074652cbf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| conn, err := systemddbus.NewWithContext(ctx) | ||
| if err != nil { | ||
| return trace.NotFound("systemd D-Bus is unavailable: %v", err) | ||
| } |
There was a problem hiding this comment.
Stop service when post-start systemd attach fails
runService starts teleport-vnet.service before opening the systemd D-Bus monitoring connection, but if systemddbus.NewWithContext fails here it returns immediately and never calls stopService. In that failure path, tsh vnet exits with an error while the privileged service can continue running in the background, which is especially problematic for transient D-Bus/systemd access issues.
Useful? React with 👍 / 👎.
Backports #63664 and #64493
changelog: Added Linux support for VNet
Manual Test Plan
Test Environment
it relies on the presence of systemd, D-Bus and polkit, most modern desktop Linux distros include them.
To make it work, you need to install the required polkit and D-Bus configuration files.
Polkit action:
D‑Bus config
By default, d-bus will not allow any user (including root) to own the
org.teleport.vnet1name:D‑Bus service file (activates systemd unit)
Systemd unit
I tested this on a cluster with:
I also set
google.comas a custom DNS zone for VNet and checked access toworkspace.google.comwith VNet turned on.It is actually convenient to inspect DNS behavior with
resolvectl.resolvectl statusshows all active links.resolvectl domainshows domains attached to each link, for example:Link 37 (TeleportVNet): ~internal.example.com ~google.com ~teleport.tangyatsu.com ~teleport.devYou can query and verify which link handled DNS:
Example output:
Test Cases
tsh vnetjournalctl -u teleport-vnet.servicesudo tsh vnetwithout presence of config files