Skip to content

fix: jsonrpc url parsing and dial function#6264

Merged
mergify[bot] merged 11 commits intotendermint:masterfrom
fdymylja:fdymylja/6260-fix-url-parsing
Mar 29, 2021
Merged

fix: jsonrpc url parsing and dial function#6264
mergify[bot] merged 11 commits intotendermint:masterfrom
fdymylja:fdymylja/6260-fix-url-parsing

Conversation

@fdymylja
Copy link
Contributor

@fdymylja fdymylja commented Mar 22, 2021

This PR fixes how the jsonrpc parses the URL, and how the dial function connects to the RPC.

Closes: #6260

@fdymylja fdymylja marked this pull request as draft March 22, 2021 20:28
@alexanderbez alexanderbez requested a review from tychoish March 23, 2021 13:42
@alexanderbez alexanderbez added the C:rpc Component: JSON RPC, gRPC label Mar 23, 2021
@tac0turtle
Copy link
Contributor

tac0turtle commented Mar 23, 2021

@fdymylja is this ready for review? I see its still in draft.

@fdymylja
Copy link
Contributor Author

fdymylja commented Mar 24, 2021

@fdymylja is this ready for review? I see its still in draft.

Nope not yet. Unfortunately I need to use the CI to run tests because I'm on windows :(

@tac0turtle
Copy link
Contributor

tac0turtle commented Mar 24, 2021

Nope not yet. Unfortunately I need to use the CI to run tests because I'm on windows :(

Let me know if you need any help testing, we can hop on a call.

@codecov
Copy link

codecov bot commented Mar 24, 2021 •
edited
Loading

Codecov Report

Merging #6264 (32620bc) into master (ca080b5) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6264      +/-   ##
==========================================
+ Coverage   60.74%   60.86%   +0.11%     
==========================================
  Files         281      281              
  Lines       26638    26648      +10     
==========================================
+ Hits        16180    16218      +38     
+ Misses       8766     8754      -12     
+ Partials     1692     1676      -16
Impacted Files Coverage Δ
rpc/jsonrpc/client/http_json_client.go 17.98% <100.00%> (+7.13%) ⬆️
p2p/transport_memory.go 83.78% <0.00%> (-2.71%) ⬇️
blockchain/v0/pool.go 77.94% <0.00%> (-2.67%) ⬇️
evidence/reactor.go 71.05% <0.00%> (-1.76%) ⬇️
statesync/syncer.go 79.60% <0.00%> (-0.81%) ⬇️
p2p/pex/pex_reactor.go 76.55% <0.00%> (-0.60%) ⬇️
p2p/router.go 78.98% <0.00%> (-0.54%) ⬇️
proxy/multi_app_conn.go 48.05% <0.00%> (ø)
consensus/state.go 66.84% <0.00%> (+0.18%) ⬆️
consensus/peer_state.go 80.70% <0.00%> (+0.87%) ⬆️
... and 8 more

@fdymylja fdymylja marked this pull request as ready for review March 24, 2021 10:07
@fdymylja
Copy link
Contributor Author

fdymylja commented Mar 24, 2021

@alexanderbez @marbar3778 I opened it, would you mind reviewing again?

Sum is: preserve old behaviour only towards unix sockets, if it's not a unix socket use the URL semantics to provide valid paths.

On a side note, it still flies over my head how exposing an HTTP server via unix sockets and calling it through a client does not give path issues when querying tendermint. (it might even bring up hostname headers issues if the client and server do not parse the hostname in the same way, as now we hack it by replacing '/' with '.' to make it look like a valid URL on the client side)

tac0turtle
tac0turtle reviewed Mar 24, 2021
View reviewed changes
tac0turtle
tac0turtle approved these changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@tac0turtle tac0turtle left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK

we should backport this since it doesn't seem to be breaking

@fdymylja fdymylja mentioned this pull request Mar 24, 2021
Closed
@cmwaters cmwaters added the S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x label Mar 25, 2021
@tac0turtle tac0turtle added the S:automerge Automatically merge PR when requirements pass label Mar 29, 2021
@mergify mergify bot merged commit 9ecfcc9 into tendermint:master Mar 29, 2021
mergify bot pushed a commit that referenced this pull request Mar 29, 2021
@fdymylja
fix: jsonrpc url parsing and dial function (#6264)
67aeeed 
This PR fixes how the jsonrpc parses the URL, and how the dial function connects to the RPC.

Closes: #6260

(cherry picked from commit 9ecfcc9)
@mergify mergify bot mentioned this pull request Mar 29, 2021
Merged
@fdymylja fdymylja deleted the fdymylja/6260-fix-url-parsing branch March 29, 2021 09:26
tac0turtle pushed a commit that referenced this pull request Mar 29, 2021
@mergify @fdymylja
fix: jsonrpc url parsing and dial function (#6264) (#6288)
b7a4d5e 
This PR fixes how the jsonrpc parses the URL, and how the dial function connects to the RPC.

Closes: #6260

(cherry picked from commit 9ecfcc9)

Co-authored-by: Frojdi Dymylja <33157909+fdymylja@users.noreply.github.com>
@cmwaters cmwaters mentioned this pull request Feb 22, 2022
Merged
This was referenced Apr 22, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebuchman ebuchman Awaiting requested review from ebuchman

@tessr tessr Awaiting requested review from tessr

@tychoish tychoish Awaiting requested review from tychoish

3 more reviewers

@tac0turtle tac0turtle tac0turtle approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@cmwaters cmwaters cmwaters approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C:rpc Component: JSON RPC, gRPC S:automerge Automatically merge PR when requirements pass S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

jsonrpc: parsedURL parses the port and path incorrectly for path routed domains

4 participants

@fdymylja @tac0turtle @alexanderbez @cmwaters