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

[ZEPPELIN-828] Use individual user credentials for data source authentication #860

Closed
wants to merge 11 commits into from

Conversation

prasadwagle
Copy link

@prasadwagle prasadwagle commented Apr 27, 2016

What is this PR for?

The purpose of this pull request is to allow users to use their own credentials to authenticate with data sources.
It contains the following:

  • web UI and rest API to let users input their credentials
  • changes to AuthenticationInfo to add data source credentials
  • changes to Paragraph to set data source credentials in AuthenticationInfo
  • changes to HiveInterpreter to use the user credentials and simplification to use a new connection for every query. We made this change since we found an issue with closed vertica connections. Since we are using HiveInterpreter for analytics, the cost of creating a new connection is small compared to query execution. We don't have to merge this change into master.

What type of PR is it?

Feature

Todos

  • - Save user credentials in a file
  • - Do not persist AuthenticationInfo in paragraph since it contains sensitive information
  • - Add tests
  • - Add documentation

What is the Jira issue?

ZEPPELIN-828

How should this be tested?

  1. In conf/shiro.ini, use /** = authc
  2. Login to Zeppelin
  3. Enter credentials for a data source
  4. Check if the interpreter is using the correct credentials

Screenshots (if appropriate)

https://gfycat.com/LiquidGentleKatydid

Questions:

  • Does this needs documentation? Yes.

@Leemoonsoo
Copy link
Member

Hi @prasadwagle. Thanks for sharing great work.

I think this is nice solution for http://apache-zeppelin-users-incubating-mailing-list.75479.x6.nabble.com/Storing-Interpreter-Passwords-tp2773.html

One feedback is, about passing credential information to interpreter process.
Currently, single credential passed to interpreter process, when DATSOURCE_KEY matches to '%hive(DATASOURCE_KEY)'.

But currently, hive (jdbc) is only interpreter that leverages '%hive(DATASOURCE_KEY)'. So most of other interpreters are not able to leverage this new features.

So, how about passing all credential map (of login user) to interpreter process?
And let interpreter find appropriate credential from the map.
What do you think?

@prasadwagle
Copy link
Author

@Leemoonsoo I agree with you and have implemented the change to pass all user credentials to the interpreter. I also updated the screencast. Please let me know your feedback.

@Leemoonsoo
Copy link
Member

@prasadwagle Tried and looks good. I think separate issue can handle address HiveInterpreter changes. If you'd prefer.

One thing is, credential here not only can be credentials for 'datasource' but also credentials for anything, depends on how individual interpreter uses this information. right?

So how about use little bit more generic name such as 'Credential Id' or 'Credential Name', instead of 'Data source' on 'Credential' page?

@prasadwagle
Copy link
Author

@Leemoonsoo I agree. I removed HiveInterpreter changes from this PR. What do you think about using Entity instead of Data source?

@prasadwagle
Copy link
Author

The "TEST_SELENIUM=true" job in last build failed with the error below. I am going to close/reopen the PR to start the build.

12076 -------------------------------------------------------^M
12077 T E S T S^M
12078 -------------------------------------------------------^M
12079 Running org.apache.zeppelin.rest.ZeppelinSparkClusterTest^M
...
12390 17:25:22,650 INFO org.apache.zeppelin.notebook.Paragraph:235 - run paragraph 20160512-172522_1287820414 using pyspark org. apache.zeppelin.interpreter.LazyOpenInterpreter@14fda3ee^M
12391
12392
12393 No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.
12394
12395 The build has been terminated

@prasadwagle
Copy link
Author

The build finally succeeded. I have created a jira issue with flaky-test label. Is it ok to merge #860 and #856?

@Leemoonsoo
Copy link
Member

Thanks @prasadwagle. It looks good to me!

CredentialsInfoSaving info = gson.fromJson(json, CredentialsInfoSaving.class);
this.credentialsMap = info.credentialsMap;
} catch (IOException e) {
LOG.error("Error loading credentials file");
Copy link
Member

Choose a reason for hiding this comment

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

please use LOG.error("message", e); instead?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @felixcheung. I made the change. Unfortunately, the selenium flaky test is failing again. I am going to close/reopen the PR again. I created https://issues.apache.org/jira/browse/ZEPPELIN-878.

Copy link
Member

@bzz bzz Jun 1, 2016

Choose a reason for hiding this comment

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

@prasadwagle Looks like there is still this guy left

Copy link
Author

Choose a reason for hiding this comment

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

@bzz Sorry I missed that. Fixed.

@prasadwagle prasadwagle reopened this May 24, 2016
@prasadwagle
Copy link
Author

The build failed again because of the flaky test. https://issues.apache.org/jira/browse/ZEPPELIN-878
@Leemoonsoo @bzz @felixcheung - what should we do about this?

@prasadwagle
Copy link
Author

I just merged with the latest master and still got two build failures.
cc: @bzz @corneadoug @sagarkulkarni3592

https://travis-ci.org/apache/incubator-zeppelin/jobs/133223324
#4352.7 failed
Tests in error:
ZeppelinIT.testSparkInterpreterDependencyLoading:243 » NoSuchElement Unable to...

https://travis-ci.org/apache/incubator-zeppelin/jobs/133223320
#4352.3 errored
21:24:32,109 INFO org.apache.zeppelin.notebook.Paragraph:241 - run paragraph 20160526-212432_3024637 using pyspark org.apache.zeppelin.interpreter.LazyOpenInterpreter@49dfce80
No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

@prasadwagle prasadwagle reopened this May 26, 2016
@bzz
Copy link
Member

bzz commented May 27, 2016

@prasadwagle I think it's better to actually rebase (may be even interactive, to get rid of last 2 commits) the whole branch on top of the latest pull from master, instead of just merging it in. Could you please try that and see if it solves the first issue?

@prasadwagle
Copy link
Author

@bzz I tried your suggestion to rebase of the latest master but got the same error:
https://travis-ci.org/apache/incubator-zeppelin/jobs/133661149
Tests in error:
ZeppelinIT.testSparkInterpreterDependencyLoading:243 » NoSuchElement Unable to...

@prasadwagle prasadwagle reopened this May 29, 2016
@bzz
Copy link
Member

bzz commented May 31, 2016

Let me look into that again and get back to you

@bzz
Copy link
Member

bzz commented May 31, 2016

It's different issue now - I have found the reason of CI failure and described at ZEPPELIN-878. Will fix it from there.

@prasadwagle I think it's good to merge now, as soon as you address felix's feedback

@prasadwagle
Copy link
Author

Thanks @bzz. I think I addressed @felixcheung feedback. @felixcheung - can you please review?

@bzz
Copy link
Member

bzz commented Jun 1, 2016

@prasadwagle thank you! CI fix was merged, so may be we could try rebasing on master again and see if it makes CI happy here as well

@prasadwagle
Copy link
Author

@bzz I rebased off the latest master and got the error below. Should I create a new jira issue or is this a duplicate?

https://travis-ci.org/apache/incubator-zeppelin/jobs/134535308

18:38:26,926 ERROR org.apache.zeppelin.AbstractZeppelinIT:153 - Exception in ZeppelinIT while testSparkInterpreterDependencyLoading
org.openqa.selenium.NoSuchElementException: Unable to locate element: {"method":"xpath","selector":"//div[@id='spark']//form//button[1]"}
Command duration or timeout: 30.08 seconds
...
testSparkInterpreterDependencyLoading(org.apache.zeppelin.integration.ZeppelinIT) Time elapsed: 82.819 sec <<< ERROR!
org.openqa.selenium.NoSuchElementException: Unable to locate element: {"method":"xpath","selector":"//div[@id='spark']//form//button[1]"}
Command duration or timeout: 30.08 seconds
...
Tests run: 3, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 109.101 sec <<< FAILURE! - in org.apache.zeppelin.integration.ZeppelinIT
testSparkInterpreterDependencyLoading(org.apache.zeppelin.integration.ZeppelinIT) Time elapsed: 82.819 sec <<< ERROR!
org.openqa.selenium.NoSuchElementException: Unable to locate element: {"method":"xpath","selector":"//div[@id='spark']//form//button[1]"}
Command duration or timeout: 30.08 seconds
For documentation on this error, please visit: http://seleniumhq.org/exceptions/no_such_element.html

...
Results :

Tests in error:
ZeppelinIT.testSparkInterpreterDependencyLoading:243 » NoSuchElement Unable to...

Tests run: 14, Failures: 0, Errors: 1, Skipped: 0

@bzz
Copy link
Member

bzz commented Jun 2, 2016

@praagarw thanks for poining it out, it's a new one, I'm lookin into it under ZEPPELIN-942

It does not block merging this PR, so since all feedback was addressed - merge it, if there is no further discussion

@asfgit asfgit closed this in bdc78da Jun 2, 2016
asfgit pushed a commit that referenced this pull request Jun 27, 2016
### What is this PR for?
There were several changes in Zeppelin UI after #860, #1006, #1013, #1081 so update screenshot of documents accordingly.

### What type of PR is it?
Documentation

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: AhyoungRyu <fbdkdud93@hanmail.net>
Author: Mina Lee <mina.hyeji.lee@gmail.com>
Author: Mina Lee <minalee@apache.org>

Closes #1089 from minahlee/doc/ZEPPELIN-1002 and squashes the following commits:

b237caf [Mina Lee] Merge pull request #1 from AhyoungRyu/doc/ZEPPELIN-1002/again
b18544a [AhyoungRyu] Update screenshot images in interpreters.md
add97fb [AhyoungRyu] Update screenshot images in notebookashomepage.md
cdaeb30 [AhyoungRyu] Update screenshot images in index.md
b21444a [AhyoungRyu] Update screenshot images in notebook_authorization.md
b23f7e4 [AhyoungRyu] Update screenshot images in dependencymanagement.md
e7a85f3 [AhyoungRyu] Update screenshot images in lens.md
cecd161 [AhyoungRyu] Update screenshot images in ignite.md
9f8cb71 [AhyoungRyu] Update screenshot images in elasticsearch.md
0c9a688 [AhyoungRyu] Hide dynamicinterpreterloading.md temporarily
a17f31f [Mina Lee] Update doc image in Explore Zeppelin UI page
asfgit pushed a commit that referenced this pull request Jun 27, 2016
### What is this PR for?
There were several changes in Zeppelin UI after #860, #1006, #1013, #1081 so update screenshot of documents accordingly.

### What type of PR is it?
Documentation

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: AhyoungRyu <fbdkdud93@hanmail.net>
Author: Mina Lee <mina.hyeji.lee@gmail.com>
Author: Mina Lee <minalee@apache.org>

Closes #1089 from minahlee/doc/ZEPPELIN-1002 and squashes the following commits:

b237caf [Mina Lee] Merge pull request #1 from AhyoungRyu/doc/ZEPPELIN-1002/again
b18544a [AhyoungRyu] Update screenshot images in interpreters.md
add97fb [AhyoungRyu] Update screenshot images in notebookashomepage.md
cdaeb30 [AhyoungRyu] Update screenshot images in index.md
b21444a [AhyoungRyu] Update screenshot images in notebook_authorization.md
b23f7e4 [AhyoungRyu] Update screenshot images in dependencymanagement.md
e7a85f3 [AhyoungRyu] Update screenshot images in lens.md
cecd161 [AhyoungRyu] Update screenshot images in ignite.md
9f8cb71 [AhyoungRyu] Update screenshot images in elasticsearch.md
0c9a688 [AhyoungRyu] Hide dynamicinterpreterloading.md temporarily
a17f31f [Mina Lee] Update doc image in Explore Zeppelin UI page

(cherry picked from commit 09f0ebd)
Signed-off-by: Mina Lee <minalee@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants