Skip to content

User in interpreter context#705

Closed
prabhjyotsingh wants to merge 14 commits intoapache:masterfrom
prabhjyotsingh:UserInInterpreterContext
Closed

User in interpreter context#705
prabhjyotsingh wants to merge 14 commits intoapache:masterfrom
prabhjyotsingh:UserInInterpreterContext

Conversation

@prabhjyotsingh
Copy link
Copy Markdown
Contributor

What is this PR for?

The goal of the PR is to pass userName/userInfo from front-end to interpreters.
The PR uses Shiro authentication(#586).

What type of PR is it?

Improvement

Todos

  • - Change RemoteInterpreterService.thrift signature
  • - Change InterpreterContext/RemoteInterpreterContext signature
  • - Modify all existing interpreters to above signature

Is there a relevant Jira issue?

N/A

if fromMessage.principal.equals("anonymous") then set user as null
@prabhjyotsingh prabhjyotsingh force-pushed the UserInInterpreterContext branch from f228b6d to 60e0a6e Compare February 9, 2016 10:20
@prabhjyotsingh prabhjyotsingh force-pushed the UserInInterpreterContext branch from 60e0a6e to 320790c Compare February 9, 2016 10:56
@rconline
Copy link
Copy Markdown
Contributor

rconline commented Feb 9, 2016

This is a requisite first step for getting to multi-user scenarios. I believe this should get discussed a little bit in terms of approach.

@prabhjyotsingh
Copy link
Copy Markdown
Contributor Author

This is ready for review.

@felixcheung
Copy link
Copy Markdown
Member

@rconline how would you want to discuss? Should we open a JIRA - or a JIRA parent with subtests for this multi-user auth use cases?

@felixcheung
Copy link
Copy Markdown
Member

@prabhjyotsingh could you explain more on the reason for this change?
Also this seems like a somewhat big change - could you open a JIRA on this so it could be tracked?

@rconline
Copy link
Copy Markdown
Contributor

@felixcheung @prabhjyotsingh The purpose of this discussion should be to get to an approach that allows us zeppelin to do a couple of things - pass user identity to the underlying spark cluster, and thereby support multi-tenancy. There is JIRA open - https://issues.apache.org/jira/browse/ZEPPELIN-645 which we could use to discuss this further. Does that make sense?

@Leemoonsoo
Copy link
Copy Markdown
Member

I think it make sense to let access authentication info through InterpreterContext.
But i think authentication info will be more than a user name in the future. It can have some credentials/token stuff from Authentication system (shiro), so interpreter process can reuse it without asking authentication again, when they're required.

So my suggestion is, create such class AuthenticationInfo, and pass it into InterpreterContext instead of String userName, for further extension in the future.

@prabhjyotsingh
Copy link
Copy Markdown
Contributor Author

Thank you for the suggestion @Leemoonsoo. Have added AuthenticationInfo class, and made relevant changes.

@Leemoonsoo
Copy link
Copy Markdown
Member

I have tried and it works well.
One thing is, org.apache.zeppelin.display package supposed to have Display System related codes.
I think AuthenticationInfo is less related to Display System. Could you suggest other package name?

@prabhjyotsingh
Copy link
Copy Markdown
Contributor Author

Thanks you @Leemoonsoo, that does make a lot of sense.

@prabhjyotsingh prabhjyotsingh force-pushed the UserInInterpreterContext branch from d955d62 to 033a354 Compare February 19, 2016 06:24
@Leemoonsoo
Copy link
Copy Markdown
Member

Thanks @prabhjyotsingh.
It looks good to me.

@jongyoul
Copy link
Copy Markdown
Member

LGTM

@felixcheung
Copy link
Copy Markdown
Member

Let's rebase and merge this?

…ntext

# Conflicts:
#	zeppelin-server/src/test/java/org/apache/zeppelin/integration/ZeppelinIT.java
@prabhjyotsingh
Copy link
Copy Markdown
Contributor Author

Thank you for reviewing this. Have merged this with master.

@asfgit asfgit closed this in 98cb8e8 Feb 23, 2016
@prabhjyotsingh prabhjyotsingh deleted the UserInInterpreterContext branch June 1, 2016 15:46
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.

5 participants