rpc: fix RPC client doesn't handle url's without ports #6507
Conversation
JayT106
requested review from
alexanderbez,
cmwaters,
ebuchman,
tessr and
tychoish
as code owners
JayT106
changed the title
JayT106
changed the title
Codecov Report
@@ Coverage Diff @@
## master #6507 +/- ##
==========================================
- Coverage 65.99% 61.08% -4.92%
==========================================
Files 234 295 +61
Lines 20269 27860 +7591
==========================================
+ Hits 13376 17017 +3641
- Misses 5825 9131 +3306
- Partials 1068 1712 +644
|
JayT106
force-pushed
the
rpc-client-url-fix
branch
from
98008d3
to
fa4366e
Compare
| @@ -378,7 +380,13 @@ func makeHTTPDialer(remoteAddr string) (func(string, string) (net.Conn, error), | |||
| } | |||
|
|
|||
| dialFn := func(proto, addr string) (net.Conn, error) { | |||
| return net.Dial(protocol, u.GetDialAddress()) | |||
| var timeout = 10 * time.Second | |||
| if !u.isUnixSocket && strings.LastIndex(u.Host, ":") == -1 { | |||
There was a problem hiding this comment.
Do you have an example of input here and resulting output?
There was a problem hiding this comment.
https://foo-bar.com -> foo-bar.com:https
http://foo-bar.com -> foo-bar.com:http
unix:///tmp/test -> 'unix:///tmp/test' (because it's UnixSocket)
There was a problem hiding this comment.
And why do we need to do this?
There was a problem hiding this comment.
These fixes just try to tackle the issue @marbar3778 posted. We can just close it if we think the current error message is good enough. But the user needs to aware they need to input the URL address with port info.
There was a problem hiding this comment.
in newParsedURL function.if we do url.Parse with https://foo-bar.com.
We will get url.Scheme = "https", url.Host = "foo-bar.com"
So under this input case, It's not able to connect to the remote due to a missing port.
Therefore I am padding the protocol at the end of the Host, and then the socket will look up the default port for the protocol.
There was a problem hiding this comment.
I will let @marbar3778 approve this one 👍
There was a problem hiding this comment.
I think this makes things a bit easier for when a user is connecting to a URL. What is the norm here? Should the application handle this instead?
| @@ -84,3 +84,18 @@ func Test_parsedURL(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func TestMakeHTTPDialerURL(t *testing.T) { | |||
| remotes := []string{"https://foo-bar.com", "http://foo-bar.com"} | |||
There was a problem hiding this comment.
do we want to add other kinds of protocols and/or test error cases?
There was a problem hiding this comment.
@tychoish what other kinds of protocols we will use in the HTTP client?
There was a problem hiding this comment.
I can add some error cases.
There was a problem hiding this comment.
Added 2 error test cases.
JayT106
force-pushed
the
rpc-client-url-fix
branch
from
909b1b7
to
ce351d9
Compare
padding service name as port when URL doesn't have port info, so
net.dialerwill lookup the default port for the service.Closes #5407