[HWORKS-876] Optimize permission fixer (#1433)

logicalclocks · Dec 18, 2023 · 3050dd2 · 3050dd2
1 parent 7e261eb
commit 3050dd2
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 21 deletions.
diff --git a/hopsworks-api/src/main/java/io/hops/hopsworks/api/util/DownloadService.java b/hopsworks-api/src/main/java/io/hops/hopsworks/api/util/DownloadService.java
@@ -249,7 +249,8 @@ private Pair<Path, StreamingOutput> downloadFromHDFS(Project project, DatasetPat
 
     if (!ds.isPublicDs() && ds.isShared(project) &&
       DatasetPermissions.fromFilePermissions(fsPermission).equals(DatasetPermissions.OWNER_ONLY)) {
-      throw new DatasetException(RESTCodes.DatasetErrorCode.DOWNLOAD_ERROR, Level.FINE);
+      throw new DatasetException(RESTCodes.DatasetErrorCode.DOWNLOAD_ERROR, Level.FINE,
+        "You do not have the rights to download from this dataset");
     }
 
     FSDataInputStream stream;
@@ -262,11 +263,12 @@ private Pair<Path, StreamingOutput> downloadFromHDFS(Project project, DatasetPat
         return new Pair<>(p, buildOutputStream(stream, udfso));
 
       } else {
-        throw new DatasetException(RESTCodes.DatasetErrorCode.DOWNLOAD_ERROR, Level.WARNING);
+        throw new DatasetException(RESTCodes.DatasetErrorCode.DOWNLOAD_ERROR, Level.WARNING, "Project user not found.");
       }
 
     } catch (IOException ex) {
-      throw new DatasetException(RESTCodes.DatasetErrorCode.DOWNLOAD_ERROR, Level.SEVERE, "path: " + fullPath,
+      throw new DatasetException(RESTCodes.DatasetErrorCode.DOWNLOAD_ERROR, Level.SEVERE,
+        "Failed to download path: " + fullPath,
         ex.getMessage(), ex);
     }
   }

diff --git a/hopsworks-common/src/main/java/io/hops/hopsworks/common/dataset/acl/PermissionsFixer.java b/hopsworks-common/src/main/java/io/hops/hopsworks/common/dataset/acl/PermissionsFixer.java
@@ -91,46 +91,63 @@ public void fixPermissions() {
 
   public int fixPermissions(int index, Long startTime) {
     List<Project> projectList = projectFacade.findAllOrderByCreated();
-    for (int i = index; i < projectList.size(); i++) {
-      Project project = projectList.get(i);
-      fixPermissions(project);
-      index++;
-      if (startTime > 0 && System.currentTimeMillis() - startTime > 300000) {
-        break;
+    DistributedFileSystemOps dfso = null;
+    try {
+      dfso = dfsService.getDfsOps();
+      for (int i = index; i < projectList.size(); i++) {
+        Project project = projectList.get(i);
+        fixPermissions(project, dfso);
+        index++;
+        if (startTime > 0 && System.currentTimeMillis() - startTime > 300000) {
+          break;
+        }
       }
+    } finally {
+      dfsService.closeDfsClient(dfso);
     }
     if (index >= projectList.size()) {
       index = 0;
     }
     return index;
   }
 
-  public void fixPermissions(Project project) {
+  public void fixPermissions(Project project, DistributedFileSystemOps dfso) {
     if (isUnderRemoval(project)) {
       return;
     }
     try {
-      fixProject(project);
+      fixProject(project, dfso);
     } catch (Exception e) {
       LOGGER.log(Level.WARNING, "Failed to fix dataset permissions for project {0}. Error: {1}",
         new Object[]{project.getName(), e.getMessage()});
     }
   }
 
-  private void fixProject(Project project) throws IOException {
-    List<Dataset> datasetList = datasetFacade.findByProject(project);
-    Inode projectInode = inodeController.getProjectRoot(project.getName());
+  public void fixPermissions(Project project) {
+    if (isUnderRemoval(project)) {
+      return;
+    }
     DistributedFileSystemOps dfso = null;
     try {
       dfso = dfsService.getDfsOps();
-      for (Dataset dataset : datasetList) {
-        fixDataset(projectInode, dataset, dfso);
-      }
+      fixProject(project, dfso);
+    } catch (Exception e) {
+      LOGGER.log(Level.WARNING, "Failed to fix dataset permissions for project {0}. Error: {1}",
+        new Object[]{project.getName(), e.getMessage()});
     } finally {
       dfsService.closeDfsClient(dfso);
     }
   }
 
+  private void fixProject(Project project, DistributedFileSystemOps dfso) throws IOException {
+    List<Dataset> datasetList = datasetFacade.findByProject(project);
+    Inode projectInode = inodeController.getProjectRoot(project.getName());
+
+    for (Dataset dataset : datasetList) {
+      fixDataset(projectInode, dataset, dfso);
+    }
+  }
+
   private void fixDataset(Inode projectInode, Dataset dataset, DistributedFileSystemOps dfso) throws IOException {
     String datasetGroup = hdfsUsersController.getHdfsGroupName(dataset.getProject(), dataset);
     String datasetAclGroup = hdfsUsersController.getHdfsAclGroupName(dataset.getProject(), dataset);

diff --git a/hopsworks-common/src/main/java/io/hops/hopsworks/common/project/ProjectController.java b/hopsworks-common/src/main/java/io/hops/hopsworks/common/project/ProjectController.java
@@ -2400,8 +2400,8 @@ public AccessCredentialsDTO credentials(Integer projectId, Users user) throws Pr
       return getAccessCredentials(project, user);
     } catch (Exception ex) {
       LOGGER.log(Level.SEVERE, null, ex);
-      throw new DatasetException(RESTCodes.DatasetErrorCode.DOWNLOAD_ERROR, Level.SEVERE, "projectId: " + projectId,
-        ex.getMessage(), ex);
+      throw new DatasetException(RESTCodes.DatasetErrorCode.DOWNLOAD_ERROR, Level.SEVERE,
+        "Failed to download credentials for projectId: " + projectId, ex.getMessage(), ex);
     } finally {
       certificateMaterializer.removeCertificatesLocal(user.getUsername(), project.getName());
     }

diff --git a/hopsworks-rest-utils/src/main/java/io/hops/hopsworks/restutils/RESTCodes.java b/hopsworks-rest-utils/src/main/java/io/hops/hopsworks/restutils/RESTCodes.java
@@ -267,8 +267,7 @@ public enum DatasetErrorCode implements RESTErrorCode {
     PATH_NOT_FOUND(18, "Path not found", Response.Status.BAD_REQUEST),
     PATH_NOT_DIRECTORY(19, "Requested path is not a directory", Response.Status.BAD_REQUEST),
     PATH_IS_DIRECTORY(20, "Requested path is a directory", Response.Status.BAD_REQUEST),
-    DOWNLOAD_ERROR(21, "You cannot download from a non public shared dataset",
-        Response.Status.BAD_REQUEST),
+    DOWNLOAD_ERROR(21, "Failed to download.", Response.Status.BAD_REQUEST),
     DOWNLOAD_PERMISSION_ERROR(22, "Your role does not allow to download this file",
         Response.Status.BAD_REQUEST),
     DATASET_PERMISSION_ERROR(23, "Could not update dataset permissions",