add user-mode networking option #247
Conversation
WalkthroughThis PR adds a Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant Init as init.go
participant Preflights as preflights.go
participant Validate as validateOptions
participant CheckGVP as checkGvproxyVersion
participant RuntimeOpts as Runtime Init
CLI->>Init: --user-mode-networking flag
Init->>Init: Set initOptionalFlags.UserModeNetworking
Init->>Preflights: RunPreflights(vmProvider, userModeNetworking)
Preflights->>Validate: validateOptions(provider, userModeNetworking)
alt WSL + user-mode networking
Validate-->>Preflights: Error: unsupported
Preflights-->>Init: Error
else Valid configuration
Validate-->>Preflights: OK
Preflights->>CheckGVP: checkGvproxyVersion(provider, userModeNetworking)
alt user-mode networking inactive
CheckGVP->>CheckGVP: Skip for WSL/Hyper-V
else user-mode networking active
CheckGVP->>CheckGVP: Enforce --services capability
end
CheckGVP-->>Preflights: OK
Preflights-->>Init: OK
Init->>RuntimeOpts: initOpts.UserModeNetworking = &flag
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This assumes Let's restrict maybe what we will 'support' (for now)? when dealing with usermode on Windows, 'systemd + networkmanager' ... for now |
|
just a reminder, we should not allow to set usermode networking on wsl. We should verify how to deal with/replace the additional |
why not? https://github.com/sakai135/wsl-vpnkit. We worked with @sakai135, to simplify his work as it addresses the same problem we solve: connectivity issues when using a VPN. |
My comment was referring to the current implementation. I mean I need to add a guard here bc if you enable user-mode on wsl right now it won't work. And thanks for the link, i'll give that a look. I'll open an issue to track user-mode/wsl |
|
Wanted to be sure... and hope to also give some background. 👍 |
it adds a flag to enable user-mode networking on hyperv. While on other OS/hypervisor user-mode is always enabled, on WSL podman code supports both cases. For this reason, it adds a constraint check on Win to prevent users from using the user-mode-networking flag when the provider is not hyperv Signed-off-by: lstocchi <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
vendor/github.com/containers/podman/v5/pkg/machine/hyperv/stubber.go (1)
80-82: LGTM - Logic is correct!The UserModeNetworking configuration properly distinguishes between ignition (always enabled) and cloud-init (controlled by flag). The comments clearly explain the behavior.
For slightly improved readability, consider this alternative:
- // Set userModeNetworking based on cloudInit value for backwards compatibility - // Usermode networking is true by default when working with ignition - // If cloud-init is enabled, use userModeNetworking from options - mc.HyperVHypervisor.UserModeNetworking = !mc.CloudInit || opts.UserModeNetworking + // Set userModeNetworking based on cloudInit value for backwards compatibility + if mc.CloudInit { + // If cloud-init is enabled, use userModeNetworking from options + mc.HyperVHypervisor.UserModeNetworking = opts.UserModeNetworking + } else { + // Usermode networking is true by default when working with ignition + mc.HyperVHypervisor.UserModeNetworking = true + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cmd/macadam/init.go(5 hunks)cmd/macadam/preflight.go(1 hunks)go.mod(1 hunks)pkg/preflights/preflights.go(2 hunks)vendor/github.com/containers/podman/v5/pkg/domain/infra/abi/play_linux.go(1 hunks)vendor/github.com/containers/podman/v5/pkg/domain/infra/abi/play_unsupported.go(1 hunks)vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit.go(2 hunks)vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_unix.go(1 hunks)vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_windows.go(1 hunks)vendor/github.com/containers/podman/v5/pkg/machine/hyperv/hutil/hutil.go(1 hunks)vendor/github.com/containers/podman/v5/pkg/machine/hyperv/stubber.go(5 hunks)vendor/modules.txt(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-04T09:00:52.558Z
Learnt from: cfergeau
Repo: crc-org/macadam PR: 196
File: cmd/macadam/inspect.go:54-57
Timestamp: 2025-07-04T09:00:52.558Z
Learning: In the macadam project, the `provider` variable is defined as a global variable in cmd/macadam/root.go and is used for command-line flag parsing across the package.
Applied to files:
cmd/macadam/init.go
🧬 Code graph analysis (8)
cmd/macadam/preflight.go (1)
pkg/preflights/preflights.go (1)
RunPreflights(15-33)
vendor/github.com/containers/podman/v5/pkg/machine/hyperv/hutil/hutil.go (1)
vendor/github.com/containers/podman/v5/pkg/systemd/parser/unitfile.go (1)
NewUnitFile(152-159)
pkg/preflights/preflights.go (1)
vendor/github.com/containers/podman/v5/pkg/machine/vmconfigs/config.go (1)
VMProvider(71-93)
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_unix.go (3)
vendor/github.com/containers/podman/v5/pkg/machine/vmconfigs/config.go (1)
MachineConfig(15-63)vendor/github.com/containers/podman/v5/pkg/machine/keys.go (1)
GetSSHKeys(40-50)vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit.go (3)
UserData(31-36)User(15-21)EmbeddedResource(38-41)
vendor/github.com/containers/podman/v5/pkg/machine/hyperv/stubber.go (3)
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit.go (1)
GenerateISO(74-157)vendor/github.com/containers/podman/v5/pkg/machine/hyperv/hutil/hutil.go (2)
CreateNetworkUnit(27-35)HyperVVsockNMConnection(9-25)vendor/github.com/containers/podman/v5/pkg/machine/ignition/ignition.go (1)
EncodeDataURLPtr(558-560)
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_windows.go (5)
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_unix.go (2)
GenerateUserData(12-40)GetEmbeddedResources(42-44)vendor/github.com/containers/podman/v5/pkg/machine/vmconfigs/config.go (1)
MachineConfig(15-63)vendor/github.com/containers/podman/v5/pkg/machine/keys.go (1)
GetSSHKeys(40-50)vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit.go (4)
UserData(31-36)User(15-21)WriteFile(23-29)EmbeddedResource(38-41)vendor/github.com/containers/podman/v5/pkg/machine/hyperv/hutil/hutil.go (2)
CreateNetworkUnit(27-35)HyperVVsockNMConnection(9-25)
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit.go (2)
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_unix.go (1)
GetEmbeddedResources(42-44)vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_windows.go (1)
GetEmbeddedResources(144-161)
cmd/macadam/init.go (2)
vendor/github.com/containers/podman/v5/pkg/machine/define/initopts.go (1)
InitOptions(26-50)pkg/preflights/preflights.go (1)
RunPreflights(15-33)
🔇 Additional comments (7)
vendor/github.com/containers/podman/v5/pkg/machine/hyperv/stubber.go (4)
27-27: LGTM - Good refactoring!The import of hutil enables the use of shared utility functions for network unit creation and NM connection configuration, reducing code duplication.
123-126: LGTM - Good refactoring!Using
hutil.CreateNetworkUnitconsolidates the network unit creation logic. Note that this implementation requires NetworkManager and systemd in the guest VM, as discussed in the PR comments.
141-141: LGTM - Good refactoring!Using the shared
hutil.HyperVVsockNMConnectionconstant reduces code duplication and centralizes the NetworkManager connection configuration.
96-103: Network configuration for cloud-init user-mode networking is properly implemented — no action needed.The vsock0 NetworkManager connection and network unit are already configured in the cloud-init generation process (vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_windows.go, lines 36-50). When
UserModeNetworkingis enabled, the code creates the network unit file and writes the vsock0.nmconnection file with appropriate permissions during cloud-init ISO generation. The implementation is complete and correct.cmd/macadam/preflight.go (1)
32-32: Looks goodHard-coding
userModeNetworkingto false here keeps the standalonepreflightcommand aligned with the existing defaults, while the new init flag handles the opt-in path.cmd/macadam/init.go (1)
158-161: Preflight plumbing is on pointPassing the flag value into
RunPreflightsensures the gvproxy validation runs only when user-mode networking is requested.pkg/preflights/preflights.go (1)
35-40: Nice guard rail for unsupported combosEarly validation cleanly blocks the WSL + user-mode combination with a clear message.
| HasReadyUnit: false, | ||
| ForwardSockets: false, | ||
| } | ||
| initOpts.UserModeNetworking = &initOptionalFlags.UserModeNetworking |
There was a problem hiding this comment.
Preserve the tri-state semantics for UserModeNetworking
define.InitOptions documents nil as “use backend/system default”, false as “force disable”, and true as “force enable”. Assigning &initOptionalFlags.UserModeNetworking unconditionally forces false whenever the flag isn’t provided, overriding the provider’s default behaviour. This regresses existing flows (e.g. providers that rely on nil to auto-select networking).
Only populate the pointer when the user explicitly sets the flag; otherwise leave it nil.
[suggested diff]
- initOpts.UserModeNetworking = &initOptionalFlags.UserModeNetworking
+ if flag := initCmd.Flags().Lookup("user-mode-networking"); flag != nil && flag.Changed {
+ initOpts.UserModeNetworking = &initOptionalFlags.UserModeNetworking
+ }As per coding guidelines
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| initOpts.UserModeNetworking = &initOptionalFlags.UserModeNetworking | |
| if flag := initCmd.Flags().Lookup("user-mode-networking"); flag != nil && flag.Changed { | |
| initOpts.UserModeNetworking = &initOptionalFlags.UserModeNetworking | |
| } |
🤖 Prompt for AI Agents
In cmd/macadam/init.go around line 218, the code currently always sets
initOpts.UserModeNetworking = &initOptionalFlags.UserModeNetworking which
collapses the tri-state into false when the flag isn't provided; change it to
only populate initOpts.UserModeNetworking when the user explicitly set the flag
(e.g. check the command flag was provided via
Flags().Changed("user-mode-networking") or an explicit UserModeNetworkingSet
boolean) and otherwise leave initOpts.UserModeNetworking nil so backend/system
defaults are preserved.
| userData.RunCmd = []string{ | ||
| "install -o root -g root -m 0755 /mnt/gvforwarder /usr/local/bin/gvforwarder", | ||
| "nmcli connection reload", | ||
| "systemctl daemon-reload", | ||
| "systemctl enable --now vsock-network.service", | ||
| } | ||
|
|
||
| userData.Mounts = [][]string{ | ||
| {"/dev/sr0", "/mnt", "iso9660", "defaults,ro", "0", "0"}, | ||
| } |
There was a problem hiding this comment.
Fail fast when gvforwarder payload is missing
RunCmd always runs install /mnt/gvforwarder …, but GetEmbeddedResources() just logs and returns an empty slice when the download fails. In that case the ISO lacks the binary, the install command bombs during cloud-init, and user‑mode networking never comes up, yet we still report success. Please treat the download failure as fatal (e.g., make GetEmbeddedResources return an error that bubbles up, or only add the RunCmd/WriteFiles after successfully fetching the bytes) so we don’t configure user-mode networking without its payload.
Also applies to: 150-160
| netUnit.Add("Service", "ExecStart", fmt.Sprintf("/usr/libexec/podman/gvforwarder -preexisting -iface vsock0 -url vsock://2:%d/connect", netPort)) | ||
| netUnit.Add("Service", "ExecStartPost", "/usr/bin/nmcli c up vsock0") | ||
| netUnit.Add("Install", "WantedBy", "multi-user.target") |
There was a problem hiding this comment.
ExecStartPost never executes with the long-running gvforwarder
gvforwarder is the main daemon for this unit, so the ExecStart command never exits while the service is active. In systemd, ExecStartPost commands are only run after all ExecStart commands complete successfully, meaning this nmcli invocation will never fire and the connection stays down. Please move the nmcli call to ExecStartPre (or otherwise ensure it runs before gvforwarder blocks) so the link actually comes up when the service starts.
- netUnit.Add("Service", "ExecStart", fmt.Sprintf("/usr/libexec/podman/gvforwarder -preexisting -iface vsock0 -url vsock://2:%d/connect", netPort))
- netUnit.Add("Service", "ExecStartPost", "/usr/bin/nmcli c up vsock0")
+ netUnit.Add("Service", "ExecStartPre", "/usr/bin/nmcli c up vsock0")
+ netUnit.Add("Service", "ExecStart", fmt.Sprintf("/usr/libexec/podman/gvforwarder -preexisting -iface vsock0 -url vsock://2:%d/connect", netPort))🤖 Prompt for AI Agents
In vendor/github.com/containers/podman/v5/pkg/machine/hyperv/hutil/hutil.go
around lines 31-33, the systemd unit adds nmcli as ExecStartPost but gvforwarder
is a long-running ExecStart so ExecStartPost never runs; change the unit to run
the nmcli invocation before gvforwarder blocks by moving the nmcli command to
ExecStartPre (or otherwise ensure it runs prior to the long-running ExecStart),
e.g., replace the ExecStartPost entry with an ExecStartPre entry for
"/usr/bin/nmcli c up vsock0" so the link is brought up before gvforwarder
starts.
2 participants
it adds a flag to enable user-mode networking on hyperv.
it uses cfergeau/podman#31
Summary by CodeRabbit
New Features
--user-mode-networkingflag for VM initializationBug Fixes
Dependencies