LINBIT / virter

Virter is a command line tool for simple creation and cloning of virtual machines based on libvirt
Apache License 2.0
216 stars 11 forks source link

Remove hardcoded ssh path (issue #23) #24

Closed bashfulrobot closed 1 year ago

bashfulrobot commented 1 year ago

Changes to be committed: modified: pkg/netcopy/netcopy.go

Testing

I built virter locally and ran the same commands identified in issue #23 .


# Original virter in system path
λ virter vm cp default.xml rke-cp:~ --loglevel debug
DEBU[0000] Using config file: /home/dustin/.config/virter/virter.toml 
DEBU[0000] executing rsync command:                     
DEBU[0000] RSYNC_RSH=/usr/bin/ssh -i "/home/dustin/.ssh/id_rsa_temp" -o UserKnownHostsFile=/tmp/rsync-known-hosts-854713461 -o PubkeyAcceptedKeyTypes=+ssh-rsa /run/current-system/sw/bin/rsync --recursive --perms --times --protect-args default.xml root@192.168.122.100:~ 
DEBU[0000] rsync output:
rsync: [sender] Failed to exec /usr/bin/ssh: No such file or directory (2)
rsync error: error in IPC code (code 14) at pipe.c(85) [sender=3.2.7]
rsync: [sender] safe_write failed to write 6 bytes to fd 4: Broken pipe (32)
rsync error: error in rsync protocol data stream (code 12) at io.c(326) [sender=3.2.7] 
FATA[0000] error executing rsync: exit status 12        

# Updated Virter not in path
dustin in 🌐 rembot in ☸ default in ~ on ☁️  (us-west-2) at 10:46:48 
λ ./virter vm cp default.xml rke-cp:~ --loglevel debug
DEBU[0000] Using config file: /home/dustin/.config/virter/virter.toml 
DEBU[0000] executing rsync command:                     
DEBU[0000] RSYNC_RSH=ssh -i "/home/dustin/.ssh/id_rsa_temp" -o UserKnownHostsFile=/tmp/rsync-known-hosts-725856400 -o PubkeyAcceptedKeyTypes=+ssh-rsa /run/current-system/sw/bin/rsync --recursive --perms --times --protect-args default.xml root@192.168.122.100:~ 

As you can see the copy was successful.

Admittedly, logic could be added to test for the binaries in $PATH

bashfulrobot commented 1 year ago

I am not sure if you would want something like this added (pseudo code) - or if the current commit follows your current coding patterns/preferences.

package main

import (
    "fmt"
    "os/exec"
)

func main() {
    binaries := []string{"ssh", "rsync", "grep"}

    for _, bin := range binaries {
        path, err := exec.LookPath(bin)
        if err != nil {
            fmt.Println(bin, "not found in the user path")
        } else {
            fmt.Println(bin, "found at", path)
        }
    }
}
JoelColledge commented 1 year ago

@chrboe Do you remember any reason for using an absolute path for ssh here?

chrboe commented 1 year ago

I have no idea, honestly.

I have not tested this or anything, but it seems like the default value for RSYNC_RSH is ssh, which to me implies that rsync takes care of the $PATH lookup. So I would expect the above to just work.

chrboe commented 1 year ago

Works fine on my machine too:

$ virter --loglevel debug vm cp values.yml centos-7-200:/root
DEBU[0000] Using config file: /home/christoph/.config/virter/virter.toml
DEBU[0000] executing rsync command:
DEBU[0000] RSYNC_RSH=ssh -i "/home/christoph/.config/virter/id_rsa" -o UserKnownHostsFile=/tmp/rsync-known-hosts-3204555196 -o PubkeyAcceptedKeyTypes=+ssh-rsa /usr/bin/rsync --recursive --perms --times --protect-args values.yml root@192.168.122.200:/root

So I'm pretty sure this is good to merge. Thanks for the fix @bashfulrobot.