Skip to content
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

Jump to bottom

rpc: fix RPC client doesn't handle url's without ports #6507

Merged
merged 11 commits into from Jun 14, 2021
Merged

rpc: fix RPC client doesn't handle url's without ports #6507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bd8ba9e
Fix RPC client doesn't handle url's without ports
JayT106 May 28, 2021
dc26fe6
update pending changelog
JayT106 May 28, 2021
fa4366e
nix
JayT106 May 28, 2021
30608d0
consistency variable assign
JayT106 Jun 1, 2021
ce351d9
add error url tests
JayT106 Jun 1, 2021
d34845d
Merge branch 'master' into rpc-client-url-fix
JayT106 Jun 1, 2021
9036c5e
Merge branch 'master' into rpc-client-url-fix
JayT106 Jun 3, 2021
a6a777b
Merge branch 'master' into rpc-client-url-fix
JayT106 Jun 4, 2021
c896d70
Merge branch 'master' into rpc-client-url-fix
JayT106 Jun 7, 2021
554f472
Merge branch 'master' into rpc-client-url-fix
JayT106 Jun 8, 2021
44c00ae
Merge branch 'master' into rpc-client-url-fix
cmwaters Jun 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG_PENDING.md
View file
Open in desktop
Expand Up @@ -126,5 +126,6 @@ Friendly reminder: We have a [bug bounty program](https://hackerone.com/tendermi
- [privval] \#5638 Increase read/write timeout to 5s and calculate ping interval based on it (@JoeKash)
- [blockchain/v1] [\#5701](https://github.com/tendermint/tendermint/pull/5701) Handle peers without blocks (@melekes)
- [blockchain/v1] \#5711 Fix deadlock (@melekes)
- [evidence] \#6375 Fix bug with inconsistent LightClientAttackEvidence hashing (@cmwaters)
- [evidence] \#6375 Fix bug with inconsistent LightClientAttackEvidence hashing (cmwaters)
- [rpc] \#6507 fix RPC client doesn't handle url's without ports (@JayT106)
- [statesync] \#6463 Adds Reverse Sync feature to fetch historical light blocks after state sync in order to verify any evidence (@cmwaters)
10 changes: 9 additions & 1 deletion rpc/jsonrpc/client/http_json_client.go
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@ import (
"net/http"
"net/url"
"strings"
"time"

tmsync "github.com/tendermint/tendermint/internal/libs/sync"
types "github.com/tendermint/tendermint/rpc/jsonrpc/types"
Expand Down Expand Up @@ -370,6 +371,7 @@ func makeHTTPDialer(remoteAddr string) (func(string, string) (net.Conn, error),
}

protocol := u.Scheme
padding := u.Scheme

// accept http(s) as an alias for tcp
switch protocol {
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an example of input here and resulting output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

ref: https://cs.opensource.google/go/go/+/refs/tags/go1.16.4:src/net/ipsock.go;drc=193d5141318d65cea310d995258288bd000d734c;l=248

Copy link
Contributor

Choose a reason for hiding this comment

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

And why do we need to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will let @marbar3778 approve this one 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

u.Host = fmt.Sprintf("%s:%s", u.Host, padding)
return net.DialTimeout(protocol, u.GetDialAddress(), timeout)
}

return net.DialTimeout(protocol, u.GetDialAddress(), timeout)
}

return dialFn, nil
Expand Down
28 changes: 28 additions & 0 deletions rpc/jsonrpc/client/http_json_client_test.go
View file
Open in desktop
Expand Up @@ -84,3 +84,31 @@ func Test_parsedURL(t *testing.T) {
})
}
}

func TestMakeHTTPDialerURL(t *testing.T) {
remotes := []string{"https://foo-bar.com", "http://foo-bar.com"}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to add other kinds of protocols and/or test error cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tychoish what other kinds of protocols we will use in the HTTP client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add some error cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 2 error test cases.


for _, remote := range remotes {
u, err := newParsedURL(remote)
require.NoError(t, err)
dialFn, err := makeHTTPDialer(remote)
require.Nil(t, err)

addr, err := dialFn(u.Scheme, u.GetHostWithPath())
require.NoError(t, err)
require.NotNil(t, addr)
}

errorURLs := []string{"tcp://foo-bar.com", "ftp://foo-bar.com"}

for _, errorURL := range errorURLs {
u, err := newParsedURL(errorURL)
require.NoError(t, err)
dialFn, err := makeHTTPDialer(errorURL)
require.Nil(t, err)

addr, err := dialFn(u.Scheme, u.GetHostWithPath())
require.Error(t, err)
require.Nil(t, addr)
}
}