Skip to content

parse ssh host info from per host string using 'net/url' #143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kadefor wants to merge 9 commits into
base: master
Choose a base branch
from
Open

parse ssh host info from per host string using 'net/url' #143

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1512ca0
parse host string with 'net/url'
kadefor Jun 11, 2018
8118b09
check err when get port
kadefor Jun 12, 2018
608c101
parse env from host string
kadefor Jun 12, 2018
5884c60
set ssh client's user when user in host string
kadefor Jun 12, 2018
38cad8e
fix logic when set default port
kadefor Jun 13, 2018
6d0e7ce
change host 'info' variable name to 'hostURL'
kadefor Jun 13, 2018
18fe97f
disable parse env from host url
kadefor Jun 13, 2018
5ad1410
refactor host url parse
kadefor Jun 15, 2018
bf2dc98
fix a bug when get user from host url
kadefor Jun 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 29 additions & 14 deletions ssh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"io"
"io/ioutil"
"net"
"net/url"
"os"
"os/user"
"path/filepath"
Expand Down Expand Up @@ -43,16 +44,23 @@ func (e ErrConnect) Error() string {

// parseHost parses and normalizes <user>@<host:port> from a given string.
func (c *SSHClient) parseHost(host string) error {
c.host = host
if !strings.Contains(host, "://") {
host = "ssh://" + host
}

info, err := url.Parse(host)
Copy link
Copy Markdown
Collaborator

@VojtechVitek VojtechVitek Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we call this u or url?

Copy link
Copy Markdown
Author

@kadefor kadefor Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the object is about host info, can we change the variable name to "h" or "hostInfo"?

Copy link
Copy Markdown
Collaborator

@VojtechVitek VojtechVitek Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's host URL :) I prefer u, url or hostURL if you want to be more specific

Copy link
Copy Markdown
Author

@kadefor kadefor Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the same name, "url" and package "net/url". I'll use the "hostURL" :-)

if err != nil {
return err
}

// Remove extra "ssh://" schema
if len(c.host) > 6 && c.host[:6] == "ssh://" {
c.host = c.host[6:]
c.host = info.Host
if u := info.User.Username(); u != "" {
c.user = u
}
Copy link
Copy Markdown
Collaborator

@VojtechVitek VojtechVitek Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice refactor, thanks!


if at := strings.Index(c.host, "@"); at != -1 {
c.user = c.host[:at]
c.host = c.host[at+1:]
// Add default port, if not set
if _, p, err := net.SplitHostPort(info.Host); err != nil && p == "" {
Copy link
Copy Markdown
Collaborator

@VojtechVitek VojtechVitek Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this condition looks suspicious

should this be err == nil && p == ""?

or err != nil || p == "" ?

Copy link
Copy Markdown
Author

@kadefor kadefor Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, It's my mistake! Thanks for your review! :-)

c.host += ":22"
}

// Add default user, if not set
Expand All @@ -64,13 +72,20 @@ func (c *SSHClient) parseHost(host string) error {
c.user = u.Username
}

if strings.Index(c.host, "/") != -1 {
return ErrConnect{c.user, c.host, "unexpected slash in the host URL"}
}

// Add default port, if not set
if strings.Index(c.host, ":") == -1 {
c.host += ":22"
c.env = c.env + `export SUP_HOST="` + c.host + `";`
if m, _ := url.ParseQuery(info.RawQuery); len(m) > 0 {
for k, vs := range m {
if len(vs) == 0 || vs[len(vs)-1] == "" {
continue
}

v := vs[len(vs)-1]
if (v[0] == '\'' && v[len(v)-1] == '\'') || (v[0] == '"' && v[len(v)-1] == '"') {
Copy link
Copy Markdown
Collaborator

@VojtechVitek VojtechVitek Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this complex condition is for.

Why don't you strings.Trim() it without these checks?

Copy link
Copy Markdown
Author

@kadefor kadefor Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason is the difference between ' and " in shell

example:

ssh://root:123@456@192.168.16.10:22/~/.ssh/id_rsa?MY_VAR="aa bb cc dd"&ABC_VAR='help, run: echo $HOME'&XYZ_VAR="my home dir is $HOME"&other_var='abc$xyz'

if no the 'complex condition' and just use strings.Trim(), the shell variable in c.env will be:

export MY_VAR="aa bb cc dd"; 
export ABC_VAR="help, run: echo $HOME"; 
export XYZ_VAR="my home dir is $HOME"; 
export other_var="abc$xyz";

but, I want:

export MY_VAR="aa bb cc dd";
export ABC_VAR='help, run: echo $HOME'; 
export XYZ_VAR="my home dir is $HOME"; 
export other_var='abc$xyz';

the difference:

mac-dev:~ kadefor$ abc='help, run: echo $HOME'
mac-dev:~ kadefor$ echo $abc
help, run: echo $HOME

mac-dev:~ kadefor$ abc="help, run: echo $HOME"
mac-dev:~ kadefor$ echo $abc
help, run: echo /Users/kadefor

mac-dev:~ kadefor$ abc='abc$xyz'
mac-dev:~ kadefor$ echo $abc
abc$xyz

mac-dev:~ kadefor$ abc="abc$xyz"
mac-dev:~ kadefor$ echo $abc
abc

Copy link
Copy Markdown
Collaborator

@VojtechVitek VojtechVitek Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't understand what ssh://root:123@456@192.168.16.10:22/~/.ssh/id_rsa?MY_VAR="aa bb cc dd"&ABC_VAR='help, run: echo $HOME'&XYZ_VAR="my home dir is $HOME"&other_var='abc$xyz' represents.

We should be parsing pure SSH connection string. Imho, there shouldn't be any path after the [scheme://username:passwd@]host[:port].

Copy link
Copy Markdown
Author

@kadefor kadefor Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a way to set env for per host, currently. If have a new and better method in the future( save host info to struct in Supfile?), we can disable the feature.

Need to disable it in this PR now? :-)

Copy link
Copy Markdown
Collaborator

@VojtechVitek VojtechVitek Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can set env vars via Supfile :)

imho we should just strip off hostUrl.Path

Copy link
Copy Markdown
Author

@kadefor kadefor Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently, how to set env for per host in Supfile? (about #111)

Could I leave it as an experimental function? If not, remove it :-D

Thank you for your review, again!

Copy link
Copy Markdown
Collaborator

@VojtechVitek VojtechVitek Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I don't think sup supports setting env vars per host right now. It supports setting env vars globally or per network.

Yes please, remove this feature for now. You can create a new issue for it and we'll see if there's any more interest for it out there. I doubt it, though.

We might think about per-host env vars, though. I like it.

Or, I think you could even set those env vars on the server side in the ~/.ssh/authorized_keys file:
command="export VAR=1; /bin/sh" ssh-key

Copy link
Copy Markdown
Author

@kadefor kadefor Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll remove it. Thanks for your suggestions! :)

c.env = c.env + fmt.Sprintf(`export %s=%s; `, k, vs[len(vs)-1])
} else {
c.env = c.env + fmt.Sprintf(`export %s="%s"; `, k, strings.Trim(vs[len(vs)-1], `'"`))
}
}
}

return nil
Expand Down
2 changes: 1 addition & 1 deletion sup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (sup *Stackup) Run(network *Network, envVars EnvList, commands ...*Command)

// SSH client.
remote := &SSHClient{
env: env + `export SUP_HOST="` + host + `";`,
env: env,
user: network.User,
color: Colors[i%len(Colors)],
}
Expand Down