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

Client code for ServiceRegistry with ability to set service URI in CLI #456

Merged
merged 5 commits into from Aug 2, 2018

Conversation

Projects
None yet
3 participants
@yondonfu
Copy link
Member

yondonfu commented Jun 5, 2018

Adds support for storing a service URI as a transcoder/orchestrator in the ServiceRegistry contract.

Dependent on the smart contract changes here: livepeer/protocol#223

Branched off of the current WIP V2 Networking branch because these changes only make sense in the context of the V2 networking architecture.

@yondonfu yondonfu requested review from j0sh and ericxtang Jun 5, 2018

@yondonfu yondonfu force-pushed the yf/registry-client branch from d9d61a6 to 2dcf136 Jun 5, 2018

@@ -30,15 +30,18 @@ func (w *wizard) promptTranscoderConfig() (float64, float64, *big.Int) {
fmt.Printf("Enter price per segment (default: 1) - ")
pricePerSegment = w.readDefaultBigInt(big.NewInt(1))

return blockRewardCut, feeShare, pricePerSegment
fmt.Printf("Enter service URI - ")
serviceURI := w.readString()

This comment has been minimized.

@ericxtang

ericxtang Jun 6, 2018

Member

Should we validate the URI here to make sure it's actually usable? Otherwise a typo here can be expensive.

Also, should we enforce https? Seems like a good thing, but adds some operational complexity. @j0sh do you have https thoughts since you've been working on the new networking scheme?

This comment has been minimized.

@j0sh

j0sh Jun 6, 2018

Contributor

Setting the Service URI is probably going to require a good bit of education and documentation for new and existing transcoders, so they understand what this entails. We could auto-select the value [0] and have the user confirm, to make it easier all-around. Wasn't expecting that feature in this basic PR though.

About HTTPS, it's actually easier to get a combined grpc+plain-http handler on the same port if TLS is enabled. We already generate self-signed certs at runtime [1], and set the client to ignore them [2]. Once we settle on an approach for #437 then we can start checking the certs rather than ignoring them.

[0] By finding the transcoder's own public address, then prefixing the default scheme. Also, print a loud warning if the transcoder appears to be NAT'd.
[1] 5627be7
[2] 066dcf7#diff-44bb03d888e11d6429223a7b0e55220dR204

This comment has been minimized.

@yondonfu

yondonfu Jun 6, 2018

Author Member

Agreed that there should definitely be validation for the service URI since the contract will store arbitrary strings. For starters, we could probably use url.ParseRequestURI just to check if the URI is valid

@j0sh j0sh force-pushed the ja/rpc branch from d26f428 to 944cc94 Jun 6, 2018

@j0sh

This comment has been minimized.

Copy link
Contributor

j0sh commented Jun 6, 2018

Thanks for this -- I've cherry-picked the commit into #447 . While this PR looks good from my end, I'll fixup my branch as needed based on feedback to this PR.

@yondonfu

This comment has been minimized.

Copy link
Member Author

yondonfu commented Jun 6, 2018

Hm looks like the commit history for this PR got a little messy on the most recent push...This is the relevant newest commit which contains some basic URI validation on user input: 51e6449

@j0sh j0sh force-pushed the ja/rpc branch from 944cc94 to 482db44 Jun 7, 2018

@j0sh

This comment has been minimized.

Copy link
Contributor

j0sh commented Jun 7, 2018

Cherry-picked 51e6449 into my branch, thanks

@j0sh j0sh force-pushed the ja/rpc branch 3 times, most recently from 2bf4378 to e8d2df6 Jun 11, 2018

@j0sh j0sh force-pushed the ja/rpc branch 2 times, most recently from 9878d54 to 1522370 Jun 19, 2018

@j0sh j0sh force-pushed the yf/registry-client branch from 51e6449 to d4bdca0 Jun 22, 2018

@j0sh j0sh changed the base branch from ja/rpc to master Jun 22, 2018

@j0sh j0sh force-pushed the yf/registry-client branch 2 times, most recently from 09988ab to 4835711 Jun 22, 2018

@j0sh

This comment has been minimized.

Copy link
Contributor

j0sh commented Jun 22, 2018

Added a few more commits around the ServiceURI. I'd like to get consensus on these changes since it's something that affects how users deal with command line arguments (ergo, a breaking change if anyone has scripts to automate node setup). Happy to spin this off into a separate PR if that works better.

Two things prompted these changes:

  • Usability concerns such as #449
  • In the new networking protocol, we don't have a fallback method of talking to the transcoder through the p2p network. So we need to ensure that the transcoder's public information is up-to-date.

6d637af : Usability improvement in auto-discovering the public IP and informing the user that their node needs to be rebooted in order to pick up changes to ServiceURI. Note that the https:// scheme is not required from the user here anymore. A hostname is also acceptable.

f3d0bec : publicIP not needed anymore.

4835711 : Checks the ServiceURI matches the public IP on startup. Ideally we'd kill the node if it doesn't match (since the transcoder would then be inaccessible), but we need to give active transcoders time to migrate, so we only print a warning for now.

Also offers a publicAddress option to bypass the IP check, but the publicAddress still needs to match what's in ServiceURI. The reason we aren't reusing the publicIP option here is because this could be a hostname, and is also useful in other contexts, eg devenv or private networks.

We really need to communicate to our transcoders (through release notes, Discord, forums, readme, readthedocs, wiki) about the new public address requirement and the need to set the Service URI. I'll write up something so folks aren't caught by surprise once the release hits, but this set of commits needs to be signed off first.

@yondonfu
Copy link
Member Author

yondonfu left a comment

Changes make sense to me - I agree that communication around the new service URI requirement will be really important

return
}
nw, err := bnet.NewBasicVideoNetwork(node, *publicIP, *port)
nw, err := bnet.NewBasicVideoNetwork(node, "127.0.0.1", *port)

This comment has been minimized.

@yondonfu

yondonfu Jun 22, 2018

Author Member

Do we default to passing in 127.0.0.1 with the expectation that with the V2 networking changes, this basicnet construct won't be used anyway so it doesn't matter that a public IP isn't set for this?

This comment has been minimized.

@j0sh

j0sh Jun 22, 2018

Contributor

Correct, the basicnet publicIP won't be used anymore with the v2 network. Technically this API could be cleaned up too, but I'm not sure how much further work the p2p layer will see; it depends what we do with the relay network (if anything).

}

httpPostWithParams(fmt.Sprintf("http://%v:%v/setTranscoderConfig", w.host, w.httpPort), val)
// TODO we should confirm if the transaction was actually sent
fmt.Println("\nTransaction sent. Once confirmed, please restart your node if the ServiceURI has been reset")

This comment has been minimized.

@yondonfu

yondonfu Jun 22, 2018

Author Member

The reset is just to allow the node to check for you if your service URI matches your publicAddr before moving forward with any other operations?

This comment has been minimized.

@j0sh

j0sh Jun 22, 2018

Contributor

The restart also allows the node to bind to the new port, if that has changed.

if uri.Hostname() != publicAddr {
glog.Errorf("Service address %v did not match discovered address %v; set the correct address in livepeer_cli or use -publicAddr", uri.Hostname(), publicAddr)
// TODO remove '&& false' after all transcoders have set a service URI
if active && false {

This comment has been minimized.

@yondonfu

yondonfu Jun 22, 2018

Author Member

Hm so after we remove the && false to kill the node if an active transcoder has mismatched service URI + publicAddr if it is still the case that an active transcoder did not properly set a serviceURI to match their publicAddr they won't be able to use the CLI to set a service URI anymore since the transcoder configuration options would only be available when the node is running with the -transcoder flag, but if the account for the node is already an active transcoder without a service URI then every time the node boots up with the -transcoder flag the node will be killed. I guess the workaround here is that the user could pass in -publicAddr 127.0.0.1 so that it matches the default service URI, successfully boot up the node, set an actual service URI and the reboot the node without the -publicAddr flag.

Not really a big concern IMO since the CLI makes you pick a service URI upon transcoder registration (the scenario would really only happen if someone registered as a transcoder outside of the CLI and chose to leave out the service URI), but just thought I would note the scenario.

This comment has been minimized.

@j0sh

j0sh Jun 22, 2018

Contributor

Correct, the &&false is to give currently active transcoders an opportunity to set up their service URIs; new transcoders registering after the network switchover wouldn't have this issue.

Also correct that -publicAddr 127.0.0.1 would be the fallback if current transcoders still don't set a service URI. Note that transcoders will still start up by default on port 4433 if there is no service URI set; see https://github.com/livepeer/go-livepeer/pull/447/files#diff-236217e7f473dfbde9134c6a391f3408R126

Since we have the fallback, we could still kill the node by default to force transcoders to make the adjustment, but I figured it's nicer to be a little less abrupt during the transition. We probably should monitor this though. The current transcoder pool is small enough that we could reach out to individual transcoders after a few weeks, if needed.

@j0sh j0sh added this to Active in Weekly Sprints Jul 9, 2018

@j0sh j0sh removed this from Active in Weekly Sprints Jul 9, 2018

yondonfu and others added some commits Jun 5, 2018

cli: Set a default ServiceURI.
Call out to a IP address service to find the node's public IP.

Also add https to the input, since that is the only scheme for now.
livepeer: remove publicIP option.
No longer needed with new networking protocol.
livepeer: Verify ServiceURI matches on startup.
Also add an option to bypass IP checking by passing in -publicAddr.

Enforcement is disabled for now while transcoders set the URI.

@j0sh j0sh force-pushed the yf/registry-client branch from 4835711 to 7331df4 Aug 2, 2018

@j0sh j0sh merged commit 7331df4 into master Aug 2, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@yondonfu yondonfu deleted the yf/registry-client branch Aug 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment