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.

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

go/oasis-node/cmd: Allow using non local gRPC connections #4617

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

tjanez
Copy link
Member

@tjanez tjanez commented Mar 30, 2022

Require server-side TLS for non unix-socket gRPC connections.

@tjanez tjanez added the c:cli Category: command line interface label Mar 30, 2022
@tjanez tjanez force-pushed the tjanez/cli-grpc-tls branch from f014f05 to 4fed149 Compare March 30, 2022 17:02
@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #4617 (474266c) into master (998363c) will decrease coverage by 0.14%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #4617      +/-   ##
==========================================
- Coverage   67.17%   67.02%   -0.15%     
==========================================
  Files         430      430              
  Lines       48666    48671       +5     
==========================================
- Hits        32690    32624      -66     
- Misses      11964    12017      +53     
- Partials     4012     4030      +18     
Impacted Files Coverage Δ
go/oasis-node/cmd/common/grpc/grpc.go 83.01% <66.66%> (-2.40%) ⬇️
...onsensus/tendermint/apps/beacon/state/state_vrf.go 73.33% <0.00%> (-13.34%) ⬇️
go/worker/storage/committee/utils.go 92.30% <0.00%> (-7.70%) ⬇️
go/worker/storage/p2p/sync/client.go 80.64% <0.00%> (-6.46%) ⬇️
go/consensus/tendermint/apps/beacon/genesis.go 63.63% <0.00%> (-6.07%) ⬇️
go/storage/mkvs/lookup.go 75.00% <0.00%> (-4.17%) ⬇️
go/worker/common/p2p/rpc/client.go 79.77% <0.00%> (-3.38%) ⬇️
go/sentry/api/grpc.go 40.62% <0.00%> (-3.13%) ⬇️
...consensus/tendermint/apps/roothash/transactions.go 66.66% <0.00%> (-3.12%) ⬇️
go/common/grpc/grpc.go 79.88% <0.00%> (-2.96%) ⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53494a9...474266c. Read the comment docs.

@Yawning
Copy link
Contributor

Yawning commented Mar 31, 2022

Ultimately I don't see the point of this. The only time when this really is an issue is when voting on governance, and the current voting period which ends in a day should be the last governance vote where this tooling needs to be used in the first place.

tldr; use the sdk-cli. If the sdk-cli doesn't provide what you need, go add to it.

@tjanez tjanez force-pushed the tjanez/cli-grpc-tls branch from 4fed149 to 772b2f3 Compare March 31, 2022 12:00
@tjanez
Copy link
Member Author

tjanez commented Mar 31, 2022

Ultimately I don't see the point of this. The only time when this really is an issue is when voting on governance, and the current voting period which ends in a day should be the last governance vote where this tooling needs to be used in the first place.

I agree that the future of CLI tooling is to migrate to the new Oasis CLI we are developing in the Oasis SDK repo but I think this small change will still improve the UX until people migrate to that.

Another example where this would have impact is that people who don't have access to a synced node would like to use State Sync and need the list of nodes that offer that.

With this change, they could use:

oasis-node registry node list -v -a grpc.oasis.dev:443 | \
  jq 'select(.roles | contains("consensus-rpc")) | .tls.addresses' 

Require server-side TLS for non unix-socket gRPC connections.
@tjanez tjanez force-pushed the tjanez/cli-grpc-tls branch from 772b2f3 to 474266c Compare March 31, 2022 15:43
@tjanez tjanez merged commit 5c00ba1 into master Mar 31, 2022
@tjanez tjanez deleted the tjanez/cli-grpc-tls branch March 31, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:cli Category: command line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants