Skip to content

Conversation

@anfeng
Copy link
Collaborator

@anfeng anfeng commented Mar 20, 2013

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.

Copy link
Owner

Choose a reason for hiding this comment

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

What's going on in the code here? This seems to just be completely breaking the functionality of this method.

@nathanmarz
Copy link
Owner

Comments:

  1. 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.
  2. 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?

@anfeng
Copy link
Collaborator Author

anfeng commented Apr 4, 2013

The revised code should address issues raised in your comments. Hopefully we are ready to merge.

@nathanmarz
Copy link
Owner

Looks like this pull request doesn't merge anymore.

@anfeng
Copy link
Collaborator Author

anfeng commented Apr 17, 2013

I merged the changes from nathanmarz/master, and it should be able to merge now.

@tvpavan
Copy link

tvpavan commented Jul 2, 2013

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.

@ankitoshniwal
Copy link
Contributor

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.

@anfeng
Copy link
Collaborator Author

anfeng commented Sep 25, 2013

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.

@ptgoetz
Copy link
Collaborator

ptgoetz commented Sep 27, 2013

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The simple-acl configs do not appear to be used anywhere in the code. Looks like it was a merge error.

@anfeng
Copy link
Collaborator Author

anfeng commented Sep 30, 2013

@revans2 All your comments should have been addressed. Please let me know if I miss anything.

@ptgoetz
Copy link
Collaborator

ptgoetz commented Sep 30, 2013

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?

@nathanmarz
Copy link
Owner

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.

@ptgoetz
Copy link
Collaborator

ptgoetz commented Sep 30, 2013

Moved to milestone 0.9.1

@revans2
Copy link
Contributor

revans2 commented Dec 13, 2013

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 ":" or if we want to be explicit we can create a new interface that takes both an op and a func.

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

Successfully merging this pull request may close these issues.

6 participants