storm
storm copied to clipboard
nathanmarz
Integrate Nimbus and DRPC with authentication/authorization framework
This pull request integrates Nimbus and DRPC with authentication/authorization framework.
Basically,
- Nimbus and DRPC are now implemented as ThriftServer and ThriftClient, introduced by auth framework
- All Nimbus/DRPC methods will perform an authorization check before taking any actions
- Authorization plugins are configured by 3 new configuration parameters: - nimbus.authorizer for nimbus requests - drpc.authorizer for drpc requests - drpc.invocations.authorizer for drpc invocation requests
- nimbus_auth_test.clj and drpc_auth_test.clj are built upon existing test framework and illustrates the expected behavior of NImbus amd DRPC components
Limitation:
- DRPC server will use the default executor service if SASL transport is used. This is due to the limitation of Thrift7, and will be removed with Thrift9.
Comments:
- You should use the Thrift from that branch in my repo to generate the code. We made one small patch to the generated python code to fix how hashes are done.
- Some of the stuff in here is blatantly broken. I pointed out a few spots just from glancing at it, there may be more trouble spots. Can you please go through this more carefully and make sure the code is good?
The revised code should address issues raised in your comments. Hopefully we are ready to merge.
I merged the changes from nathanmarz/master, and it should be able to merge now.
Lots of interesting work to add Auth infrastructure to Storm. Nice work @anfeng
Can it go in 0.9.0-Wip18 ? so that community can test and iron out issues before it can head for release versions.
I am running a test cluster with these code changes, so far things looks good.
@nathanmarz : Do you have any concerns on this one? If not can we get it merged, so that I can run this out of master branch on our prod clusters.
Thanks, Ankit.
We have been running the code at Yahoo for a while. Folks are happy with it. Let me take another look of the code, and ask other committers for their input.
@anfeng looks like this has merge conflicts. Can you update so it merges cleanly to facilitate testing?
I'd be interested in it since we have some multi-tenancy requirements.
Some documentation on setting it up, etc. would also be a bonus.
@revans2 All your comments should have been addressed. Please let me know if I miss anything.
This is a big pull request and I haven't had the time to fully delve into it. But at first glance I noticed a few style related issues (some of which existed prior to this pull request).
In the java code I see some non-java conventions used: the name of the "_Fields" enum for example, as well numerous methods and variables using underscore (I.e. "method_name" vs. "mehtodName").
I know it sounds nit-picky... But we already have a mishmash of somewhat different styles in thy code base. It would be nice if we could avoid inconsistencies up-front so they aren't allowed to proliferate.
Finally, @nathanmarz do we think this is too much functionality to add to an rc release, or is it okay for 0.9.0?
No, we can't add it to the RC. A release candidate can only have bug fixes added to it. We can make a 0.9.1 branch and merge this into that when we agree on merging it.
Looking through the DRPC code, it occurred to me that for all of the authorizations for DRPC we probably also want to pass in the "func" along with the "execute", "result","failRequest", and "fetchRequest".
Essentially I want to be able to say Users A, B, and C are allowed to use func "foo", and users D, E, and F are allowed to use func "bar". Or func "open" is there for anyone to send in requests, but only user A is allowed to handle those requests and send the results back, because I know the topology is going to be running as user A.
We can probably do it without changing the API, by making the operation a composite one with "
