Skip to content

Commit

Permalink
[ZEPPELIN-1376] Add proxy credentials for dependency repo for corpora…
Browse files Browse the repository at this point in the history
…te firewall use-cases

### What is this PR for?
When using Zeppelin behind corporate firewall, sometimes the dependencies download just fails silently. This PR has 2 objectives:

* add proxy credentials information for dependencies repo
* raise clear error message in case of dependencies download failure

There are 3 commits.

The first one add extra inputs in the form for adding new repository

![add_repo](https://cloud.githubusercontent.com/assets/1532977/18017489/0b486fda-6bd2-11e6-90c7-ceda18c53575.png)

The second commit fixes some issues and display a clear and explicit error message when download of dependencies fail.

Before that, when the download fails, we can see the below behaviour

![irrelevant_double_error_message](https://cloud.githubusercontent.com/assets/1532977/18017541/3cf0de1e-6bd2-11e6-8285-af03f222e8d2.gif)

* the error message is displayed twice because the call twice the method `checkDownloadingDependencies();`. One in the success callback of:

```javascript
 $scope.updateInterpreterSetting = function(form, settingId) {
              ...
            $http.put(baseUrlSrv.getRestApiBase() + '/interpreter/setting/' + settingId, request)
              .success(function(data, status, headers, config) {
                $scope.interpreterSettings[index] = data.body;
                removeTMPSettings(index);
                thisConfirm.close();
                checkDownloadingDependencies();
                $route.reload();
              })
              .error(function(data, status, headers, config) {
             ...
    };
```

Another call is inside success callback of `getInterpreterSettings()`

```javascript
var getInterpreterSettings = function() {
      $http.get(baseUrlSrv.getRestApiBase() + '/interpreter/setting')
      .success(function(data, status, headers, config) {
        $scope.interpreterSettings = data.body;
        checkDownloadingDependencies();
      }).error(function(data, status, headers, config) {
      ....
```

The problem is that `$route.reload();` in the success callback of `updateInterpreterSetting()` will trigger `init()` then `getInterpreterSettings()` so `checkDownloadingDependencies()` is called twice.

I remove the call to `checkDownloadingDependencies()` from success callback of `updateInterpreterSetting()`

The second modification is on class `DependencyResolver`. In the screen capture above, we get a **cryptic** NullPointerException coming from `DefaultRepositorySystem`. I now catch this NPE to wrap it into a more sensible and clearer exception:

```java

  public List<ArtifactResult> getArtifactsWithDep(String dependency,
    Collection<String> excludes) throws RepositoryException {
    Artifact artifact = new DefaultArtifact(dependency);
    DependencyFilter classpathFilter = DependencyFilterUtils.classpathFilter(JavaScopes.COMPILE);
    PatternExclusionsDependencyFilter exclusionFilter =
            new PatternExclusionsDependencyFilter(excludes);

    CollectRequest collectRequest = new CollectRequest();
    collectRequest.setRoot(new Dependency(artifact, JavaScopes.COMPILE));

    synchronized (repos) {
      for (RemoteRepository repo : repos) {
        collectRequest.addRepository(repo);
      }
    }
    DependencyRequest dependencyRequest = new DependencyRequest(collectRequest,
            DependencyFilterUtils.andFilter(exclusionFilter, classpathFilter));

 //Catch NPE thrown by aether and give a proper error message
    try {
      return system.resolveDependencies(session, dependencyRequest).getArtifactResults();
    } catch (NullPointerException ex) {
      throw new RepositoryException(String.format("Cannot fetch dependencies for %s", dependency));
    }
  }
```

The result is much more cleaner

![dependencies_download_error_popup](https://cloud.githubusercontent.com/assets/1532977/18033855/1be5fe9a-6d2e-11e6-91f9-2f5ea66cab26.gif)

The last commit is just doc update

![updated_docs](https://cloud.githubusercontent.com/assets/1532977/18017797/97302f14-6bd3-11e6-97cc-77bd52f25cde.png)

### What type of PR is it?
[Improvement]

### Todos
* [ ] - Code Review
* [ ] - Simple test with no Internet connection
* [ ] - Test within a corporate firewall env with a third-party dependency, requiring download

### What is the Jira issue?
**[ZEPPELIN-1376]**

### How should this be tested?

##### Simple test
* `git fetch origin pull/1369/head:WebProxy`
* `git checkout WebProxy`
* `mvn clean package -DskipTests`
* `bin/zeppelin-daemon.sh restart`
* disconnect from the Internet (pull out the cable, shutdown wifi ...)
* add a random dependency to the Spark interpreter (take `info.archinnov:achilles-core:4.2.2` for example)
* validate the change, you should see an error popup on the top-right corner saying that Zeppelin cannot download the dependency

##### Corporate firewall test
* follow the steps above for simple test
* create a new repository (see how to **[here]**) and set the proxy information
* retry the steps above to ensure that the download is successful

### Screenshots (if appropriate)
See above

### Questions:
* Does the licenses files need update? --> **NO**
* Is there breaking changes for older versions? --> **NO**
* Does this needs documentation?  --> **YES, DONE**

[ZEPPELIN-1376]: https://issues.apache.org/jira/browse/ZEPPELIN-1376
[here]: http://localhost:4000/manual/dependencymanagement.html

Author: DuyHai DOAN <doanduyhai@gmail.com>
Author: doanduyhai <doanduyhai@apache.org>

Closes #1369 from doanduyhai/ZEPPELIN-1376 and squashes the following commits:

b8d44e7 [doanduyhai] [ZEPPELIN-1376] Improve error popup display
177fbd3 [DuyHai DOAN] [ZEPPELIN-1376] Fixes JS bug to display error popup for other interpreters
9f76ef4 [DuyHai DOAN] [ZEPPELIN-1376] Do not repeat the same error popup multiple times
b264193 [DuyHai DOAN] [ZEPPELIN-1376] Add unit test and fix impl for DependencyResolver to catch NPE
1913a0a [DuyHai DOAN] [ZEPPELIN-1376] Update documentation
f01be9b [DuyHai DOAN] [ZEPPELIN-1376] Raise clear error message in case of dependencies download failure
6f2b6f8 [DuyHai DOAN] [ZEPPELIN-1376] Add proxy credentials information for dependencies repo
  • Loading branch information
doanduyhai authored and doanduyhai committed Oct 10, 2016
1 parent 821b61b commit 79fd3a0
Show file tree
Hide file tree
Showing 17 changed files with 132 additions and 27 deletions.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions docs/manual/dependencymanagement.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ When your code requires external library, instead of doing download/copy/restart
<li> If you need to resolve dependencies from other than central maven repository or
local ~/.m2 repository, hit <i class="fa fa-plus"></i> icon next to repository lists. </li>
<li> Fill out the form and click 'Add' button, then you will be able to see that new repository is added. </li>
<li> Optionally, if you are behind a corporate firewall, you can specify also all proxy settings so that Zeppelin can download the dependencies using the given credentials</li>
</ol>
</div>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public void addRepo(String id, String url, boolean snapshot) {
}
}

public void addRepo(String id, String url, boolean snapshot, Authentication auth) {
public void addRepo(String id, String url, boolean snapshot, Authentication auth, Proxy proxy) {
synchronized (repos) {
delRepo(id);
RemoteRepository rr = new RemoteRepository(id, "default", url);
Expand All @@ -81,6 +81,7 @@ public void addRepo(String id, String url, boolean snapshot, Authentication auth
RepositoryPolicy.UPDATE_POLICY_DAILY,
RepositoryPolicy.CHECKSUM_POLICY_WARN));
rr.setAuthentication(auth);
rr.setProxy(proxy);
repos.add(rr);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.sonatype.aether.RepositoryException;
import org.sonatype.aether.artifact.Artifact;
import org.sonatype.aether.collection.CollectRequest;
import org.sonatype.aether.collection.DependencyCollectionException;
import org.sonatype.aether.graph.Dependency;
import org.sonatype.aether.graph.DependencyFilter;
import org.sonatype.aether.repository.RemoteRepository;
Expand All @@ -42,6 +43,7 @@
import org.sonatype.aether.util.artifact.JavaScopes;
import org.sonatype.aether.util.filter.DependencyFilterUtils;
import org.sonatype.aether.util.filter.PatternExclusionsDependencyFilter;
import org.sonatype.aether.util.graph.DefaultDependencyNode;


/**
Expand Down Expand Up @@ -157,11 +159,11 @@ private List<File> loadFromMvn(String artifact, Collection<String> excludes)
*/
@Override
public List<ArtifactResult> getArtifactsWithDep(String dependency,
Collection<String> excludes) throws RepositoryException {
Collection<String> excludes) throws RepositoryException {
Artifact artifact = new DefaultArtifact(dependency);
DependencyFilter classpathFilter = DependencyFilterUtils.classpathFilter(JavaScopes.COMPILE);
PatternExclusionsDependencyFilter exclusionFilter =
new PatternExclusionsDependencyFilter(excludes);
new PatternExclusionsDependencyFilter(excludes);

CollectRequest collectRequest = new CollectRequest();
collectRequest.setRoot(new Dependency(artifact, JavaScopes.COMPILE));
Expand All @@ -172,7 +174,11 @@ public List<ArtifactResult> getArtifactsWithDep(String dependency,
}
}
DependencyRequest dependencyRequest = new DependencyRequest(collectRequest,
DependencyFilterUtils.andFilter(exclusionFilter, classpathFilter));
return system.resolveDependencies(session, dependencyRequest).getArtifactResults();
DependencyFilterUtils.andFilter(exclusionFilter, classpathFilter));
try {
return system.resolveDependencies(session, dependencyRequest).getArtifactResults();
} catch (NullPointerException ex) {
throw new RepositoryException(String.format("Cannot fetch dependencies for %s", dependency));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@
*/

package org.apache.zeppelin.dep;
import static org.apache.commons.lang3.StringUtils.isNotBlank;

import org.sonatype.aether.repository.Authentication;
import org.sonatype.aether.repository.Proxy;

/**
*
*
Expand All @@ -27,6 +31,11 @@ public class Repository {
private String url;
private String username = null;
private String password = null;
private String proxyProtocol = "HTTP";
private String proxyHost = null;
private Integer proxyPort = null;
private String proxyLogin = null;
private String proxyPassword = null;

public Repository(String id){
this.id = id;
Expand Down Expand Up @@ -77,4 +86,16 @@ public Authentication getAuthentication() {
}
return auth;
}

public Proxy getProxy() {
if (isNotBlank(proxyHost) && proxyPort != null) {
if (isNotBlank(proxyLogin)) {
return new Proxy(proxyProtocol, proxyHost, proxyPort,
new Authentication(proxyLogin, proxyPassword));
} else {
return new Proxy(proxyProtocol, proxyHost, proxyPort, null);
}
}
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.junit.Assert.assertEquals;

import java.io.File;
import java.io.FileNotFoundException;
import java.util.Collections;

import org.apache.commons.io.FileUtils;
Expand All @@ -36,6 +37,9 @@ public class DependencyResolverTest {
private static File testCopyPath;
private static File tmpDir;

@Rule
public ExpectedException expectedException = ExpectedException.none();

@BeforeClass
public static void setUp() throws Exception {
tmpDir = new File(System.getProperty("java.io.tmpdir")+"/ZeppelinLTest_"+System.currentTimeMillis());
Expand Down Expand Up @@ -90,4 +94,13 @@ public void testLoad() throws Exception {
exception.expect(RepositoryException.class);
resolver.load("com.agimatec:agimatec-validation:0.9.3", testCopyPath);
}

@Test
public void should_throw_exception_if_dependency_not_found() throws Exception {
expectedException.expectMessage("Source 'one.two:1.0' does not exist");
expectedException.expect(FileNotFoundException.class);

resolver.load("one.two:1.0", testCopyPath);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.apache.commons.lang.exception.ExceptionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.sonatype.aether.RepositoryException;
import org.sonatype.aether.repository.RemoteRepository;

import org.apache.zeppelin.annotation.ZeppelinApi;
Expand Down Expand Up @@ -73,9 +72,7 @@ public InterpreterRestApi(InterpreterFactory interpreterFactory) {
@Path("setting")
@ZeppelinApi
public Response listSettings() {
List<InterpreterSetting> interpreterSettings;
interpreterSettings = interpreterFactory.get();
return new JsonResponse<>(Status.OK, "", interpreterSettings).build();
return new JsonResponse<>(Status.OK, "", interpreterFactory.get()).build();
}

/**
Expand Down Expand Up @@ -202,7 +199,7 @@ public Response addRepository(String message) {
try {
Repository request = gson.fromJson(message, Repository.class);
interpreterFactory.addRepository(request.getId(), request.getUrl(), request.isSnapshot(),
request.getAuthentication());
request.getAuthentication(), request.getProxy());
logger.info("New repository {} added", request.getId());
} catch (Exception e) {
logger.error("Exception in InterpreterRestApi while adding repository ", e);
Expand Down
1 change: 1 addition & 0 deletions zeppelin-web/src/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
ngToastProvider.configure({
dismissButton: true,
dismissOnClick: false,
combineDuplications: true,
timeout: 6000
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
function ConfigurationCtrl($scope, $rootScope, $http, baseUrlSrv, ngToast) {
$scope.configrations = [];
$scope._ = _;
ngToast.dismiss();

var getConfigurations = function() {
$http.get(baseUrlSrv.getRestApiBase() + '/configurations/all').
Expand Down
1 change: 1 addition & 0 deletions zeppelin-web/src/app/credential/credential.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

function CredentialCtrl($scope, $rootScope, $http, baseUrlSrv, ngToast) {
$scope._ = _;
ngToast.dismiss();

$scope.credentialInfo = [];
$scope.showAddNewCredentialInfo = false;
Expand Down
6 changes: 4 additions & 2 deletions zeppelin-web/src/app/home/home.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@
'notebookListDataFactory',
'websocketMsgSrv',
'$rootScope',
'arrayOrderingSrv'
'arrayOrderingSrv',
'ngToast'
];

function HomeCtrl($scope, notebookListDataFactory, websocketMsgSrv, $rootScope, arrayOrderingSrv) {
function HomeCtrl($scope, notebookListDataFactory, websocketMsgSrv, $rootScope, arrayOrderingSrv, ngToast) {
ngToast.dismiss();
var vm = this;
vm.notes = notebookListDataFactory;
vm.websocketMsgSrv = websocketMsgSrv;
Expand Down
22 changes: 17 additions & 5 deletions zeppelin-web/src/app/interpreter/interpreter.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
$scope.showAddNewSetting = false;
$scope.showRepositoryInfo = false;
$scope._ = _;
ngToast.dismiss();

$scope.openPermissions = function() {
$scope.showInterpreterAuth = true;
Expand Down Expand Up @@ -108,12 +109,19 @@

var checkDownloadingDependencies = function() {
var isDownloading = false;
for (var setting = 0; setting < $scope.interpreterSettings.length; setting++) {
if ($scope.interpreterSettings[setting].status === 'DOWNLOADING_DEPENDENCIES') {
for (var index = 0; index < $scope.interpreterSettings.length; index++) {
var setting = $scope.interpreterSettings[index];
if (setting.status === 'DOWNLOADING_DEPENDENCIES') {
isDownloading = true;
break;
}

if (setting.status === 'ERROR' || setting.errorReason) {
ngToast.danger({content: 'Error setting properties for interpreter \'' +
setting.group + '.' + setting.name + '\': ' + setting.errorReason,
verticalPosition: 'top', dismissOnTimeout: false});
}
}

if (isDownloading) {
$timeout(function() {
if ($route.current.$$route.originalPath === '/interpreter') {
Expand Down Expand Up @@ -236,7 +244,6 @@
$scope.interpreterSettings[index] = data.body;
removeTMPSettings(index);
thisConfirm.close();
checkDownloadingDependencies();
$route.reload();
})
.error(function(data, status, headers, config) {
Expand Down Expand Up @@ -508,7 +515,12 @@
url: '',
snapshot: false,
username: '',
password: ''
password: '',
proxyProtocol: 'HTTP',
proxyHost: '',
proxyPort: null,
proxyLogin: '',
proxyPassword: ''
};
};

Expand Down
4 changes: 3 additions & 1 deletion zeppelin-web/src/app/interpreter/interpreter.html
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ <h4>Repositories</h4>
popover-html-unsafe="<label>URL: </label>
{{repo.url}}<br>
<label>Username: </label>
{{repo.authentication.username}}">
{{repo.authentication.username}}<br>
<label>Proxy host: </label>
{{repo.proxy.host}}">
<span class="fa fa-database"></span>
{{repo.id}}&nbsp;
<span ng-if="!isDefaultRepository(repo.id)" class="fa fa-close blackOpc"
Expand Down
5 changes: 3 additions & 2 deletions zeppelin-web/src/app/jobmanager/jobmanager.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@

angular.module('zeppelinWebApp').controller('JobmanagerCtrl', JobmanagerCtrl);

JobmanagerCtrl.$inject = ['$scope', 'websocketMsgSrv', '$interval'];
JobmanagerCtrl.$inject = ['$scope', 'websocketMsgSrv', '$interval', 'ngToast'];

function JobmanagerCtrl($scope, websocketMsgSrv, $interval) {
function JobmanagerCtrl($scope, websocketMsgSrv, $interval, ngToast) {
ngToast.dismiss();
$scope.filterValueToName = function(filterValue) {
var index = _.findIndex($scope.ACTIVE_INTERPRETERS, {value: filterValue});

Expand Down
9 changes: 7 additions & 2 deletions zeppelin-web/src/app/notebook/notebook.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,16 @@
'websocketMsgSrv',
'baseUrlSrv',
'$timeout',
'saveAsService'
'saveAsService',
'ngToast'
];

function NotebookCtrl($scope, $route, $routeParams, $location, $rootScope,
$http, websocketMsgSrv, baseUrlSrv, $timeout, saveAsService) {
$http, websocketMsgSrv, baseUrlSrv, $timeout, saveAsService,
ngToast) {

ngToast.dismiss();

$scope.note = null;
$scope.moment = moment;
$scope.editorToggled = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,44 @@ <h4 class="modal-title">Add New Repository</h4>
<input type="password" class="form-control" id="repoPassword" ng-model="newRepoSetting.password" />
</div>
</div>
<hr/>
<div class="center-block"><h4>Proxy Settings (optional)</h4></div>
<br/>
<div class="form-group">
<div class="col-sm-10">
<label class="control-label col-sm-2" for="proxyProtocol1">Protocol</label>
<label class="radio-inline">
<input type="radio" name="proxyProtocol" id="proxyProtocol1" value="HTTP" ng-model="newRepoSetting.proxyProtocol"/> HTTP
</label>
<label class="radio-inline">
<input type="radio" name="proxyProtocol" id="proxyProtocol2" value="HTTPS" ng-model="newRepoSetting.proxyProtocol"/> HTTPS
</label>
</div>
</div>
<div class="form-group">
<label class="control-label col-sm-2" for="proxyHost">Host</label>
<div class="col-sm-10">
<input type="text" class="form-control" id="proxyHost" ng-model="newRepoSetting.proxyHost" />
</div>
</div>
<div class="form-group">
<label class="control-label col-sm-2" for="proxyPort">Port</label>
<div class="col-sm-10">
<input type="text" class="form-control" id="proxyPort" ng-model="newRepoSetting.proxyPort" />
</div>
</div>
<div class="form-group">
<label class="control-label col-sm-2" for="proxyLogin">Login</label>
<div class="col-sm-10">
<input type="text" class="form-control" id="proxyLogin" ng-model="newRepoSetting.proxyLogin" />
</div>
</div>
<div class="form-group">
<label class="control-label col-sm-2" for="proxyPassword">Password</label>
<div class="col-sm-10">
<input type="text" class="form-control" id="proxyPassword" ng-model="newRepoSetting.proxyPassword" />
</div>
</div>
</div>
<div class="modal-footer">
<button type="submit"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import org.slf4j.LoggerFactory;
import org.sonatype.aether.RepositoryException;
import org.sonatype.aether.repository.Authentication;
import org.sonatype.aether.repository.Proxy;
import org.sonatype.aether.repository.RemoteRepository;

import org.apache.zeppelin.conf.ZeppelinConfiguration;
Expand Down Expand Up @@ -432,6 +433,7 @@ public Map<String, Object> getEditorFromSettingByClassName(InterpreterSetting in

private void loadInterpreterDependencies(final InterpreterSetting setting) {
setting.setStatus(InterpreterSetting.Status.DOWNLOADING_DEPENDENCIES);
setting.setErrorReason(null);
interpreterSettings.put(setting.getId(), setting);
synchronized (interpreterSettings) {
final Thread t = new Thread() {
Expand Down Expand Up @@ -460,11 +462,12 @@ public void run() {
}

setting.setStatus(InterpreterSetting.Status.READY);
setting.setErrorReason(null);
} catch (Exception e) {
logger.error(String.format("Error while downloading repos for interpreter group : %s," +
" go to interpreter setting page click on edit and save it again to make " +
"this interpreter work properly.",
setting.getGroup()), e);
"this interpreter work properly. : %s",
setting.getGroup(), e.getLocalizedMessage()), e);
setting.setErrorReason(e.getLocalizedMessage());
setting.setStatus(InterpreterSetting.Status.ERROR);
} finally {
Expand Down Expand Up @@ -1307,9 +1310,9 @@ public List<RemoteRepository> getRepositories() {
return this.interpreterRepositories;
}

public void addRepository(String id, String url, boolean snapshot, Authentication auth)
throws IOException {
depResolver.addRepo(id, url, snapshot, auth);
public void addRepository(String id, String url, boolean snapshot, Authentication auth,
Proxy proxy) throws IOException {
depResolver.addRepo(id, url, snapshot, auth, proxy);
saveToFile();
}

Expand Down

0 comments on commit 79fd3a0

Please sign in to comment.