add cp and port-forward commands for VM file copying and port forwarding#369
add cp and port-forward commands for VM file copying and port forwarding#369darioajr wants to merge 5 commits intocrc-org:mainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds two new macadam CLI commands: ChangesFile copy command
Port forwarding command
Sequence DiagramssequenceDiagram
participant User
participant CLI as macadam cp
participant Provider as VM Provider
participant SCP
participant VM
User->>CLI: macadam cp SRC DST
CLI->>CLI: Parse operands, ensure single VM
CLI->>Provider: Resolve provider, lookup VM
Provider-->>CLI: VM info (state, SSH user, address, port, key)
CLI->>SCP: Exec scp with SSH options
SCP->>VM: SSH connection to VM
SCP->>VM: Transfer files
SCP-->>User: Exit status
sequenceDiagram
participant User
participant CLI as macadam port-forward
participant Provider as VM Provider
participant SSH
participant VM
User->>CLI: macadam port-forward NAME [ports...]
CLI->>CLI: Parse port specs, validate, build -L flags
CLI->>Provider: Resolve provider, lookup VM
Provider-->>CLI: VM info (state, SSH user, address, port, key)
CLI->>SSH: Exec ssh -N with -L rules and SSH options
SSH->>VM: Establish SSH connection
SSH->>VM: Create port forward tunnels
SSH-->>User: Local ports listening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 34 minutes and 38 seconds.Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds two new macadam CLI commands to improve day-to-day interaction with running VMs: copying files via SCP (macadam cp) and forwarding local ports over SSH (macadam port-forward / macadam pf).
Changes:
- Add
macadam port-forward(pf) command to forward one or more ports to a running VM using SSH-L. - Add
macadam cpcommand to copy files between host and VM usingVMNAME:-prefixed path syntax andscp.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/macadam/port_forward.go | Implements port-forward / pf command and port-spec parsing for SSH -L forwarding. |
| cmd/macadam/cp.go | Implements cp command with VMNAME: path parsing and scp execution using VM SSH config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/macadam/cp.go`:
- Around line 45-46: splitCpPath currently treats any ':' as a VM separator
which misparses local paths containing colons; change splitCpPath so it only
treats the first ':' as a VM-prefix when the segment before that ':' contains no
path separators ('/' or '\') and is not a Windows drive letter pattern (single
ASCII letter), and optionally validate the prefix against allowed VM name
characters (alnum, '-', '_'); update all call sites (the splitCpPath calls used
in cp.go around the src/dst parsing) to rely on this tightened logic so local
filenames like "C:\foo" or "name:with:colon" are not mistaken for VM:PATH.
In `@cmd/macadam/port_forward.go`:
- Around line 101-114: The toSSHForwardFlag function currently only checks
numeric syntax via strconv.Atoi but allows 0, negatives, and >65535; update
validation to ensure both parsed localPort and remotePort are within the valid
TCP port range (1–65535). After parsing with strconv.Atoi, check each value: if
p < 1 || p > 65535 return an error like fmt.Errorf("invalid port spec %q: local
port must be between 1 and 65535", spec) (and similarly for remote port), and
otherwise continue to format the SSH flag as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ab8d29c-4fb4-4c6b-b95d-dfb668059eff
📒 Files selected for processing (2)
cmd/macadam/cp.gocmd/macadam/port_forward.go
… instructions; add port number validation function
|
Currently, I use my extension at https://github.com/darioajr/macadam-ext, but I would like this support to be native, which is why I created this PR. |
|
All the issues pointed out were fixed in the second commit. |
| if address == "" { | ||
| address = "localhost" | ||
| } |
There was a problem hiding this comment.
this can be removed. GetAddress will never return an empty string
func (mc *MachineConfig) GetAddress() string {
if mc.IPAddress != "" {
return mc.IPAddress
}
// Default to the IPv4 localhost address "127.0.0.1".
// When using mirrored networking mode in WSL, attempting to connect by the name "localhost"
// often resolves to the IPv6 address (::1) first. Mirrored networking
// does not support this IPv6 localhost address resolution, leading to connection timeouts.
// By using "127.0.0.1" it avoids this resolution issue.
return "127.0.0.1"
}
| if address == "" { | ||
| address = "localhost" | ||
| } |
- Remove redundant empty-address fallback: GetAddress() always returns a non-empty string (falls back to 127.0.0.1 internally) - Guard splitCpPath against paths like ./data:backup.txt being misidentified as VM names by rejecting any prefix containing a slash
darioajr
force-pushed
the
feature/cp-and-portforward
branch
from
57b8e85 to
5775da4
Compare
|
Hi @lstocchi, thanks for the review! 🙏 I've addressed all your comments in commit 5775da4d:
prefix := path[:idx] Let me know if there's anything else you'd like me to adjust! |
Replace the local splitCpPath helper with
pcopy.ParseSourceAndDestination from
github.com/containers/podman/v5/pkg/copy, addressing the reviewer's
suggestion to reuse podman code where possible.
Why:
- Podman's parser uses filepath.IsAbs to detect absolute paths,
which correctly handles Windows UNC paths (\\server\share\...)
and other edge cases that our 1-letter-prefix heuristic missed.
- It also rejects empty paths early (e.g. `cp myvm: ./local`),
surfacing a clear error instead of letting scp fail with a
confusing message.
- Less custom parsing code for us to maintain, and the behavior
now matches podman's own `cp` conventions.
What did NOT change, and why:
- scp invocation stays local: machine.LocalhostSSHCopy() is not
yet available in the vendored podman v5 (only on podman main).
Once macadam moves to v6, the exec.Command("scp", ...) block
can be replaced by that helper.
- The "at least one side must be a VM" and "src/dst VMs must
match" checks remain here, since they are macadam-specific
rules; podman's parser is generic and does not enforce them.
- resolveCpPath stays as a small local helper for formatting
user@address:path; no equivalent exists in podman v5.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/macadam/cp.go (1)
102-106: ⚡ Quick winUse
exec.CommandContextto honour the Cobra command's context.
exec.Commandhas no knowledge ofcmd.Context(), so a programmatic context cancellation (e.g. timeout, test teardown) will not kill the childscpprocess. For normal terminal Ctrl+C the process group signal saves you, but the idiomatic Go pattern is to propagate the context explicitly.♻️ Proposed fix
- scpExec := exec.Command("scp", scpArgs...) + scpExec := exec.CommandContext(cmd.Context(), "scp", scpArgs...) scpExec.Stdin = os.Stdin scpExec.Stdout = os.Stdout scpExec.Stderr = os.Stderr return scpExec.Run()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/macadam/cp.go` around lines 102 - 106, Replace the exec.Command call with exec.CommandContext so the child scp process is cancelled when the Cobra command context is done: use cmd.Context() (the Cobra command's context) as the context argument when creating the command that currently assigns to scpExec, keep wiring scpExec.Stdin/Stdout/Stderr and return scpExec.Run() unchanged so the command honors cancellation/timeouts; update the code around the scpExec variable in cp.go accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/macadam/cp.go`:
- Around line 102-106: Replace the exec.Command call with exec.CommandContext so
the child scp process is cancelled when the Cobra command context is done: use
cmd.Context() (the Cobra command's context) as the context argument when
creating the command that currently assigns to scpExec, keep wiring
scpExec.Stdin/Stdout/Stderr and return scpExec.Run() unchanged so the command
honors cancellation/timeouts; update the code around the scpExec variable in
cp.go accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5385f6bb-38df-407a-a551-4cef54c0fbb2
📒 Files selected for processing (2)
cmd/macadam/cp.gocmd/macadam/port_forward.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/macadam/port_forward.go
|
Hi @lstocchi, just a quick update on your reuse suggestion 🙌 I had a closer look at the podman codebase to see how much we could share. The path-parsing part has now been addressed in this PR: ✅ Done in this PR Replaced our custom splitCpPath with pcopy.ParseSourceAndDestination from github.com/containers/podman/v5/pkg/copy. This gives us: Robust Windows path handling (uses filepath.IsAbs, so UNC paths and other edge cases are handled correctly). A few things still look reusable but require either an upstream change or a podman version bump: machine.LocalhostSSHCopy() — exists on podman main but is not yet in the vendored v5. Once macadam moves to v6 (or if a backport is accepted upstream), we can drop our local exec.Command("scp", ...) block and use it directly. Let me know if you'd like me to file the upstream issue now, or if you'd rather we revisit it after this PR lands 👍 |
|
Sorry for the late answer @darioajr , i've been quite busy on other stuff. I'll give it a try tomorrow morning. Looks good at first sight. Could you please squash the commits? I think having one for cp and one for port-forward is good enough |
| if len(args) != 2 { | ||
| return errors.New("usage: macadam cp [options] SRC DST") | ||
| } |
There was a problem hiding this comment.
instead of this you could add a check at command definition (like https://github.com/containers/podman/blob/main/cmd/podman/machine/cp.go#L31)
| if len(args) < 2 { | ||
| return errors.New("VM name and at least one port spec are required") | ||
| } |
There was a problem hiding this comment.
same as cp. You could use cobra.MinimumNArgs(2)
| sshArgs = append(sshArgs, machine.LocalhostSSHArgs()...) | ||
| sshArgs = append(sshArgs, "-N", mc.SSH.RemoteUsername+"@"+address) | ||
|
|
||
| sshExec := exec.Command("ssh", sshArgs...) | ||
| sshExec.Stdin = os.Stdin | ||
| sshExec.Stdout = os.Stdout | ||
| sshExec.Stderr = os.Stderr | ||
| return sshExec.Run() |
There was a problem hiding this comment.
Before executing the command we could display a message similar to what does kubectl or other tools
Forwarding <local>→<remote>... Press Ctrl+C to stop
| scpArgs = append(scpArgs, machine.LocalhostSSHArgs()...) | ||
| scpArgs = append(scpArgs, src, dst) | ||
|
|
||
| scpExec := exec.CommandContext(cmd.Context(), "scp", scpArgs...) |
There was a problem hiding this comment.
This is a good approach. Why don't use the same in port-forward? I think it would make sense so the process is killed if the command is cancelled
Title: add cp and port-forward commands
Description:
This PR adds two new commands that make it easier to interact with running VMs managed by macadam, bringing the CLI closer to the ergonomics of podman machine, docker, and kubectl.
macadam cp
Copies files between the host and a running VM using SCP under the hood. Follows the same [VMNAME:]PATH convention used by podman cp, docker cp, and kubectl cp — no separate VM name argument needed.
macadam cp ./file.txt myvm:/tmp/
macadam cp myvm:/tmp/file.txt ./
macadam port-forward (alias: pf)
Forwards one or more local ports to a running VM over SSH. Follows the [LOCAL:]REMOTE port spec convention used by kubectl port-forward, hiding the raw SSH -L syntax from the user. Supports multiple port specs as positional arguments and a --address flag to bind on a specific local interface.
macadam port-forward myvm 8080
macadam port-forward myvm 8080:8080 9090:9090
macadam port-forward --address 0.0.0.0 myvm 8080:8080
macadam pf myvm 8080:8080
Design notes
Both commands resolve SSH credentials (username, identity file, port) directly from macadam inspect, so no manual configuration is required.
cp validates that at least one path uses the VMNAME: prefix and errors early if src and dst reference different VMs.
port-forward runs in the foreground (Ctrl+C to stop), consistent with kubectl port-forward. Background execution can be achieved with the shell & operator.
Both commands use LocalhostSSHArgs() for consistent SSH options across all macadam commands.
Summary by CodeRabbit
macadam cpto copy files between host and VM with VM-qualified paths and recursive--recursive/-rsupport; requires the target VM to be running.macadam port-forward(aliaspf) to forward one or more local ports to a running VM via SSH; acceptsREMOTEorLOCAL:REMOTEspecs and a bind address option.