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

kadefor · 2018-06-12T16:58:07Z

Currently, get some info from host string:

ssh host
ssh port
ssh user
~~ssh env variable (per host env vars #111)~~

VojtechVitek

Thanks for your contribution. Few questions/suggestions. Looks good otherwise.

VojtechVitek · 2018-06-13T10:28:52Z

ssh.go

-		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 == "" {


this condition looks suspicious

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

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

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

VojtechVitek · 2018-06-13T10:30:17Z

ssh.go

+		host = "ssh://" + host
+	}
+
+	info, err := url.Parse(host)


can we call this u or url?

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

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

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

VojtechVitek · 2018-06-13T10:31:24Z

ssh.go

-		c.host = c.host[6:]
+	c.host = info.Host
+	if u := info.User.Username(); u != "" {
+		c.user = u
 	}


This is nice refactor, thanks!

VojtechVitek · 2018-06-13T10:32:54Z

ssh.go

+			}
+
+			v := vs[len(vs)-1]
+			if (v[0] == '\'' && v[len(v)-1] == '\'') || (v[0] == '"' && v[len(v)-1] == '"') {


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

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

the reason is the difference between ' and " in shell

example:

ssh://root:123@[email protected]: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

Sorry, I don't understand what ssh://root:123@[email protected]: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].

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? :-)

you can set env vars via Supfile :)

imho we should just strip off hostUrl.Path

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!

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

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

VojtechVitek

LGTM.

Any volunteers to test this feature?

VojtechVitek · 2018-06-18T16:05:07Z

ssh.go

+		host = "ssh://" + host
+	}
+
+	hostURL, err := url.Parse(host)


Can you test this with:

127.0.0.1 localhost localhost:22 ssh://localhost

etc.?

I know url.Parse(host) had some issues with localhost URLs.

@VojtechVitek what is the issues?

here is a simple test:

sup kadefor$ cat ssh_test.go package sup import ( "fmt" "testing" ) func Test_parseHost(t *testing.T) { hosts := []string{ "127.0.0.1", "localhost", "localhost:22", "ssh://localhost", "[email protected]", "tom@localhost", "tom@localhost:22", "ssh://tom@localhost", "tom:11@@[email protected]", "tom:11@@33@localhost", "tom:11@@33@localhost:22", "ssh://tom:11@@33@localhost", } for _, host := range hosts { c := &SSHClient{} err := c.parseHost(host) if err != nil { fmt.Println(err) } else { fmt.Printf("%s\n\t%v\n", host, c) } } }

sup kadefor$ go test 127.0.0.1 &{<nil> <nil> kadefor 127.0.0.1:22 <nil> <nil> <nil> false false false export SUP_HOST="127.0.0.1:22"; } localhost &{<nil> <nil> kadefor localhost:22 <nil> <nil> <nil> false false false export SUP_HOST="localhost:22"; } localhost:22 &{<nil> <nil> kadefor localhost:22 <nil> <nil> <nil> false false false export SUP_HOST="localhost:22"; } ssh://localhost &{<nil> <nil> kadefor localhost:22 <nil> <nil> <nil> false false false export SUP_HOST="localhost:22"; } [email protected] &{<nil> <nil> tom 127.0.0.1:22 <nil> <nil> <nil> false false false export SUP_HOST="127.0.0.1:22"; } tom@localhost &{<nil> <nil> tom localhost:22 <nil> <nil> <nil> false false false export SUP_HOST="localhost:22"; } tom@localhost:22 &{<nil> <nil> tom localhost:22 <nil> <nil> <nil> false false false export SUP_HOST="localhost:22"; } ssh://tom@localhost &{<nil> <nil> tom localhost:22 <nil> <nil> <nil> false false false export SUP_HOST="localhost:22"; } tom:11@@[email protected] &{<nil> <nil> tom 127.0.0.1:22 <nil> <nil> <nil> false false false export SUP_HOST="127.0.0.1:22"; } tom:11@@33@localhost &{<nil> <nil> tom localhost:22 <nil> <nil> <nil> false false false export SUP_HOST="localhost:22"; } tom:11@@33@localhost:22 &{<nil> <nil> tom localhost:22 <nil> <nil> <nil> false false false export SUP_HOST="localhost:22"; } ssh://tom:11@@33@localhost &{<nil> <nil> tom localhost:22 <nil> <nil> <nil> false false false export SUP_HOST="localhost:22"; } PASS ok github.com/pressly/sup 0.014s

runsisi · 2019-03-04T12:47:46Z

@VojtechVitek @kadefor any updates on this? thanks.

VojtechVitek · 2019-03-04T15:43:18Z

This is pending QA. Someone from the community should test this PR and then we can merge.

I don't have time to test this out at this time.

kadefor added 4 commits June 11, 2018 19:42

parse host string with 'net/url'

1512ca0

check err when get port

8118b09

parse env from host string

608c101

set ssh client's user when user in host string

5884c60

VojtechVitek reviewed Jun 13, 2018

View reviewed changes

kadefor changed the title ~~parse ssh host info from per host string using 'net/url'~~ [WIP] parse ssh host info from per host string using 'net/url' Jun 13, 2018

kadefor added 3 commits June 13, 2018 22:46

fix logic when set default port

38cad8e

change host 'info' variable name to 'hostURL'

6d0e7ce

disable parse env from host url

18fe97f

kadefor changed the title ~~[WIP] parse ssh host info from per host string using 'net/url'~~ parse ssh host info from per host string using 'net/url' Jun 13, 2018

kadefor changed the title ~~parse ssh host info from per host string using 'net/url'~~ [WIP]parse ssh host info from per host string using 'net/url' Jun 15, 2018

refactor host url parse

5ad1410

VojtechVitek approved these changes Jun 18, 2018

View reviewed changes

fix a bug when get user from host url

bf2dc98

kadefor changed the title ~~[WIP]parse ssh host info from per host string using 'net/url'~~ parse ssh host info from per host string using 'net/url' Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kadefor commented Jun 12, 2018 •

edited

Loading

VojtechVitek left a comment

VojtechVitek Jun 13, 2018

kadefor Jun 13, 2018 •

edited

Loading

VojtechVitek Jun 13, 2018

kadefor Jun 13, 2018

VojtechVitek Jun 13, 2018

kadefor Jun 13, 2018

VojtechVitek Jun 13, 2018

VojtechVitek Jun 13, 2018

kadefor Jun 13, 2018 •

edited

Loading

VojtechVitek Jun 13, 2018

kadefor Jun 13, 2018

VojtechVitek Jun 13, 2018

kadefor Jun 13, 2018

VojtechVitek Jun 13, 2018 •

edited

Loading

kadefor Jun 13, 2018

VojtechVitek left a comment

VojtechVitek Jun 18, 2018

kadefor Jun 18, 2018

runsisi commented Mar 4, 2019 •

edited

Loading

VojtechVitek commented Mar 4, 2019

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

Are you sure you want to change the base?

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

Conversation

kadefor commented Jun 12, 2018 • edited Loading

VojtechVitek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadefor Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadefor Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VojtechVitek Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VojtechVitek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runsisi commented Mar 4, 2019 • edited Loading

VojtechVitek commented Mar 4, 2019

kadefor commented Jun 12, 2018 •

edited

Loading

kadefor Jun 13, 2018 •

edited

Loading

kadefor Jun 13, 2018 •

edited

Loading

VojtechVitek Jun 13, 2018 •

edited

Loading

runsisi commented Mar 4, 2019 •

edited

Loading