retrieve IP address by using machineConfig function #284
Conversation
WalkthroughDefaults SSH address resolution to IPv4 (127.0.0.1) via Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as macadam CLI
participant MC as MachineConfig (mc)
participant CloudInit as cloudinit package
participant ISO as ISO generator
participant HV as Hyper-V (hutil)
CLI->>MC: request SSH / start
CLI->>MC: mc.GetAddress()
note right of MC `#E6F0FF`: Address default now 127.0.0.1
MC-->>CLI: return address
CLI->>CloudInit: GenerateUserData(mc)
CloudInit->>CloudInit: build UserData (users, writefiles, runCmd, mounts)
CloudInit-->>ISO: user-data + embedded resources
ISO->>ISO: create ISO (user-data, meta-data, network-config, embedded)
ISO-->>HV: attach ISO / set DVDDiskPath
CLI->>HV: request network unit
HV->>HV: CreateNetworkUnit(netPort)
HV-->>CLI: return unit content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
update podman to version containining IPv4 fix cfergeau/podman#38 Signed-off-by: lstocchi <[email protected]>
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: 2
📜 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 (11)
cmd/macadam/ssh.go(1 hunks)go.mod(1 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/github.com/containers/podman/v5/pkg/machine/vmconfigs/machine.go(1 hunks)vendor/modules.txt(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_unix.go (4)
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_windows.go (2)
GenerateUserData(18-78)GetEmbeddedResources(144-161)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/cloudinit/cloudinit_windows.go (5)
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_unix.go (1)
GenerateUserData(12-40)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/hyperv/stubber.go (1)
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/ssh.go (1)
vendor/github.com/containers/podman/v5/pkg/machine/ssh.go (1)
LocalhostSSHShellWithAddress(24-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (windows-latest)
🔇 Additional comments (16)
vendor/github.com/containers/podman/v5/pkg/machine/vmconfigs/machine.go (1)
315-326: LGTM! IPv4 localhost default addresses the WSL mirrored networking issue.The change to default to "127.0.0.1" instead of "localhost" directly resolves the issue where WSL mirrored networking prefers IPv6 ::1 resolution, which it doesn't support. The detailed comment explains the rationale clearly.
go.mod (1)
194-194: LGTM! Vendored dependency updated to match PR requirements.The pseudo-version update aligns with the PR description noting dependency on cfergeau/podman#38.
cmd/macadam/ssh.go (1)
119-119: LGTM! Centralized address resolution improves maintainability.The refactoring to use
mc.GetAddress()creates a single source of truth for address resolution, ensuring consistent behavior across the codebase.vendor/modules.txt (1)
320-320: LGTM! Vendor manifest updated correctly.The module resolution and new hutil package import path are properly recorded in the vendor manifest.
Also applies to: 399-399
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit_unix.go (2)
12-40: LGTM! Unix cloud-init generation is well-structured.The function correctly:
- Retrieves SSH keys with proper error handling
- Constructs user-data with appropriate permissions (passwordless sudo)
- Marshals to YAML with error logging
- Prepends the required "#cloud-config" header
42-44: LGTM! Unix platform requires no embedded resources.Correctly returns nil as Unix platforms don't require embedded resources like the Windows gvforwarder.
vendor/github.com/containers/podman/v5/pkg/machine/hyperv/hutil/hutil.go (2)
9-25: LGTM! NetworkManager configuration is well-defined.The vsock0 connection configuration is appropriate for Hyper-V vsock networking with TUN mode and IPv4 auto configuration.
27-35: LGTM! Unit file generation is clean and correct.The function properly constructs a systemd unit with:
- Dependency on NetworkManager.service
- gvforwarder execution with the provided port
- NetworkManager connection activation via ExecStartPost
- Proper installation target
vendor/github.com/containers/podman/v5/pkg/machine/hyperv/stubber.go (3)
80-82: Verify the UserModeNetworking logic change.The logic changed from always disabling UserModeNetworking when CloudInit is present to a conditional:
!mc.CloudInit || opts.UserModeNetworking. This means:
- When CloudInit is false (ignition mode): UserModeNetworking is true (default)
- When CloudInit is true: UserModeNetworking follows
opts.UserModeNetworkingEnsure this behavior aligns with expectations for both ignition and cloud-init workflows, especially regarding backward compatibility.
96-103: LGTM! Cloud-init ISO generation properly integrated.The ISO generation is correctly positioned after networking setup and properly assigns the DVDDiskPath. Error handling is appropriate.
27-27: LGTM! Refactoring to hutil package improves code organization.Moving the network unit creation and vsock connection configuration to the centralized hutil package eliminates duplication and improves maintainability.
Also applies to: 123-123, 141-141
vendor/github.com/containers/podman/v5/pkg/machine/cloudinit/cloudinit.go (4)
23-29: LGTM! WriteFile struct aligns with cloud-init standards.The structure properly represents cloud-init's write_files directive with appropriate field tags.
31-36: LGTM! UserData expansion enables richer VM customization.The expanded UserData structure now supports:
- File writing (write_files)
- Command execution (runcmd)
- Filesystem mounts (mounts)
This aligns with cloud-init's standard schema and enables more sophisticated VM initialization.
38-41: LGTM! EmbeddedResource supports binary resource inclusion.The structure appropriately represents resources (like binaries) that need to be embedded into the cloud-init ISO.
125-132: LGTM! Resource embedding properly integrated into ISO generation.The code correctly retrieves and embeds platform-specific resources (like gvforwarder for Windows) into the ISO alongside the cloud-init configuration files.
vendor/github.com/containers/podman/v5/pkg/domain/infra/abi/play_unsupported.go (1)
1-13: LGTM! Appropriate platform stub implementation.The stub correctly returns an error for unsupported platforms. The build tags ensure this implementation is only used on non-Linux, non-remote builds where secure symlink opening is not supported.
| // openSymlinkPath opens the path under root using securejoin.OpenatInRoot(). | ||
| func openSymlinkPath(root *os.File, unsafePath string, flags int) (*os.File, error) { | ||
| file, err := securejoin.OpenatInRoot(root, unsafePath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return securejoin.Reopen(file, flags) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify resource handling in securejoin.Reopen.
The implementation opens a file with OpenatInRoot and then reopens it with different flags using Reopen. If Reopen fails, verify that the intermediate file descriptor from OpenatInRoot is properly closed to avoid a resource leak.
Check the securejoin library documentation to confirm whether Reopen closes the input file on error:
🌐 Web query:
Does securejoin.Reopen close the input file descriptor on error in Go?
💡 Result:
No. securejoin.Reopen does not close the input *os.File on error — it opens a new descriptor (via /proc/self/fd) and returns it or an error; the original handle is not closed by Reopen, so the caller must close it if desired. [1][2]
Sources:
[1] package docs for github.com/cyphar/filepath-securejoin (Reopen description).
[2] securejoin open_linux.go implementation showing Reopen uses handle.Fd()/Name() and returns a new *os.File (does not close the input).
Close the original file descriptor after Reopen to prevent resource leak.
The function has a resource leak: OpenatInRoot opens a file, then Reopen creates a new descriptor from it. Since Reopen does not close the original file, the descriptor from OpenatInRoot is never closed—whether Reopen succeeds or fails.
Fix by explicitly closing the original file after Reopen completes. If Reopen succeeds, close before returning the new descriptor. If it fails, close before returning the error.
🤖 Prompt for AI Agents
In vendor/github.com/containers/podman/v5/pkg/domain/infra/abi/play_linux.go
around lines 11 to 18, the file descriptor returned by securejoin.OpenatInRoot
is never closed after calling securejoin.Reopen, leaking the original
descriptor; modify the function so that after calling securejoin.Reopen you
always close the original file: if Reopen returns an error, close the original
file before returning the error; if Reopen succeeds, close the original file
prior to returning the new file descriptor (ensuring errors from Close are
handled/logged as appropriate).
| 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.
Do not schedule install /mnt/gvforwarder when the binary is missing.
GetEmbeddedResources() logs the download failure and returns an empty slice, which means the ISO is created without gvforwarder. However, RunCmd here still unconditionally runs install -o root -g root -m 0755 /mnt/gvforwarder .... On any network hiccup, offline environment, or GitHub API outage, /mnt/gvforwarder will not exist and this command will fail, aborting the cloud-init sequence and leaving user-mode networking broken. We need to either propagate the download error so VM creation aborts earlier, or gate the RunCmd setup on the resource being present (and skip the vsock setup if it isn't). As written this is a deterministic failure path in common environments without outbound GitHub access.
1 participant
This needs cfergeau/podman#38 and fixes #280
Summary by CodeRabbit
New Features
Bug Fixes
Chores