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

Provide a means for a @CordaService to access a CordaRPCOps to the local node #1479

Closed
dazraf opened this issue Sep 11, 2017 · 25 comments
Closed
Assignees

Comments

@dazraf
Copy link
Contributor

dazraf commented Sep 11, 2017

Context

I'm writing @CordaService that exposes a more advanced webservice. I'm doing this because I want to take advantage of vertx's performance and websockets; and further, I wish this webservice to be co-located with the node to simplify the deployment and lifecycle for the node.

Issue

I cannot create a RPC client easily because the RPC host and port is not exposed on PluginServiceHub. The best I can do is to either:

  1. re-read the node's config - this solution doesn't work with the test driver.
  2. provide a second, private, webserver config containing the RPC host and port - which also doesn't solve for the test driver case.

Suggestion

Provide some means that a @CordaService can acquire a CordaRPCOps.
This can be done in a number of ways. Some suggestions:

  • Provide a builder on PluginServiceHub or ServiceHub to acquire a CordaRPCOps
  • Provide a rpcHostAndPort: NetworkHostAndPort onf PluginServiceHub
  • Provide access to the Corda node Config on ServiceHub or derived.

thanks and regards
Fuzz

@dazraf dazraf changed the title Expose RPC host and port for a @CordaService Provide a means for a @CordaService to access a CordaRPCOps to the local node Sep 11, 2017
@dazraf dazraf changed the title Provide a means for a @CordaService to access a CordaRPCOps to the local node Provide a means for a @CordaService to access a CordaRPCOps to the local node Sep 11, 2017
@mikehearn
Copy link
Contributor

CCing Matthew.

Which RPCs do you want, exactly? As far as I know, the RPCs we have are all essentially just fronts to ServiceHub functionality.

@mnesbit
Copy link
Contributor

mnesbit commented Sep 22, 2017

I also think everything in CordaRPCOps is just thin wrappers over ServiceHub calls at the minute (once the transport layer code and security checks are passed).

@shamsasari
Copy link
Contributor

I believe @dazraf whats the ability to start flows from Corda services. The ServiceHub that's provided doesn't expose the startFlow method, it's only available in ServiceHubInternal.

@mikehearn
Copy link
Contributor

Oh, odd. Why is startFlow not in ServiceHub?

@mnesbit
Copy link
Contributor

mnesbit commented Sep 25, 2017

Because when we last had this discussion we regarded starting flows a key permission checking and audit capturing life cycle point. Therefore flows should only be able to start subFlows, or registered remote flows. That was precisely why startFlow was on PluginServiceHub as a privileged method. However, we now seem to have thinned that down to an empty extension of ServcieHub and then it got removed.

@mikehearn
Copy link
Contributor

Ah, I see, this is "if we put it on ServiceHub then it's available to services and flows".

If you have a service that's running in-process, any audit mechanism can be bypassed by that code if it wants to. We've talked about sandboxing apps in the past, but for now I guess we could have a startFlow(Dynamic) method on ServiceHub that takes an identity/message to use for auditing purposes as well. For RPC it's filled out automatically. When starting flows from a service inside the node, you have to provide it yourself. Would that meet the requirements?

@mnesbit
Copy link
Contributor

mnesbit commented Sep 25, 2017

I was fine with services being passed a special PluginServiceHub extension of ServiceHub. I don't know why the privileged methods got removed from that, they were useful and I have used them historically. It makes sense that a trusted service app e.g. to gather Oracle data feeds might need to call flows to update the ledger. The ServiceHub extension should be able to ensure auditing is safely done by the framework, not regarded as something the app can be trusted with. We do have a generic idea ofFlowInitiator identity that could easily cover service identity.

@mikehearn
Copy link
Contributor

@shamsasari any idea why PluginServiceHub lost the startFlow methods? It's too late for 1.0 but I guess we can bring them back (with thought put into the case where we're trying to audit a sandboxed adversarial app) in a later release.

@shamsasari
Copy link
Contributor

The startFlow method was never in PluginServiceHub, it's always existed in ServiceHubInternal. But yes it makes sense to have some sort of sandboxed startFlow method. The question is whether we want to add it to ServiceHub and thus expose it to flows or re-introduce PluginServiceHub (though perhaps with a different name) and pass that into @CordaService classes so only they can start flows.

@mnesbit
Copy link
Contributor

mnesbit commented Sep 25, 2017

OK my memory does seem to be playing tricks on me, but yes we should add something for this after 1.0.

@dazraf
Copy link
Contributor Author

dazraf commented Oct 2, 2017

Hi, looks like I've been missing notifications on github.
Thanks for looking into this.
One benefit with local-loop RPC, that @rick-r3 and I discussed, is authN/Z.
Alternatively startFlow on resurrected PluginServiceHub would be fine too.

@dazraf
Copy link
Contributor Author

dazraf commented Oct 2, 2017

It turns out, that with some hackery, I can get a RPC client - which works fine in driver tests, but not with corda.jar. The Node initialisation of serialisation is a one time event, due to the way SerializationFactoryImpl.registerSchemes has been implemented.

@mikehearn
Copy link
Contributor

OK, let's go with a PluginServiceHub and a startFlow that takes some internal notion of identity e.g. the name of the app.

@dazraf
Copy link
Contributor Author

dazraf commented Oct 2, 2017

One question regarding ServiceHubInternal.startFlow. The documentation says:

Note that you must be on the server thread to call this method.

Is there a way to dispatch a request from a non-server thread to the server threadpool?

@mikehearn
Copy link
Contributor

Good point. There isn't. That's something that should be on PluginServiceHub as well!

@dazraf
Copy link
Contributor Author

dazraf commented Oct 2, 2017

It may be more safe for startFlow to automatically dispatch to a server thread ...

@dazraf
Copy link
Contributor Author

dazraf commented Oct 2, 2017

Can any thread be initialised appropriately using something like that used for the shell?
e.g.
CURRENT_RPC_CONTEXT.set(RpcContext(User(ArtemisMessagingComponent.NODE_USER, "", setOf())))

@mikehearn
Copy link
Contributor

Yes but the other APIs on ServiceHub have thread affinity requirements too, I think. But yes we could make ServiceHub entirely thread safe: that's easier.

You can write to the current RPC context if you're willing to use/abuse internal APIs.

@dazraf
Copy link
Contributor Author

dazraf commented Oct 2, 2017

Definitely would prefer to use public APIs :-) I was only asking, in case you'd like me to make the changes to ServiceHubInternal and reintroduce PluginServiceHub. For now, my work on the vertx sample is using ServiceHubInternal

@mikehearn
Copy link
Contributor

If you want to submit a PR to do this, that'd be fab!

@mnesbit
Copy link
Contributor

mnesbit commented Oct 2, 2017

For 1.1 I assume

@mikehearn
Copy link
Contributor

For 2.0 (new APIs mean new major version bump in our current version thiking). But yes, of course. Too late for 1.0 now.

@dazraf
Copy link
Contributor Author

dazraf commented Oct 2, 2017

re version numbers
just curious to learn the versioning scheme.
I'm guessing most dev shops follow semantic versioning.
Major bump only needed if not backwards compatible.
Additional classes, methods, optional params etc could be minor?

@dazraf
Copy link
Contributor Author

dazraf commented Oct 2, 2017

The only bit I'm not sure about is the correct way to dispatch a request to a server thread. That CURRENT_RPC_CONTEXT didn't seem like the right thing to do ...

@mnesbit
Copy link
Contributor

mnesbit commented Oct 9, 2017

On master the services can now directly start flows annotated with StartableByService using the AppServiceHub as constructor parameter on the CordaService class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants