[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1395: HDDS-4088. Adding Owner info for Authorizer plugin to honor owner access rights

2020-10-22 Thread GitBox 


smengcl commented on a change in pull request #1395:
URL: https://github.com/apache/hadoop-ozone/pull/1395#discussion_r510376263



##
File path: 
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##
@@ -1642,25 +1644,42 @@ private boolean hasAcls(String userName, ResourceType 
resType,
   UserGroupInformation.createRemoteUser(userName),
   ProtobufRpcEngine.Server.getRemoteIp(),
   ProtobufRpcEngine.Server.getRemoteIp().getHostName(),
-  false);
+  false, getVolumeOwner(vol, acl));
 } catch (OMException ex) {
   // Should not trigger exception here at all
   return false;
 }
   }
 
-  /**
-   * CheckAcls for the ozone object.
-   *
-   * @throws OMException ResultCodes.PERMISSION_DENIED if permission denied.
-   */
-  @SuppressWarnings("parameternumber")
-  public void checkAcls(ResourceType resType, StoreType storeType,
-  ACLType aclType, String vol, String bucket, String key,
-  UserGroupInformation ugi, InetAddress remoteAddress, String hostName)
-  throws OMException {
-checkAcls(resType, storeType, aclType, vol, bucket, key,
-ugi, remoteAddress, hostName, true);
+  public String getVolumeOwner(String vol, ACLType type) throws OMException {
+String volOwnerName = null;
+if (!vol.equals(OzoneConsts.OZONE_ROOT) && (type != ACLType.CREATE)) {
+  volOwnerName = getVolumeOwner(vol);
+}
+return volOwnerName;
+  }
+
+  private String getVolumeOwner(String volume) throws OMException {
+Boolean lockAcquired = metadataManager.getLock().acquireReadLock(
+VOLUME_LOCK, volume);
+String dbVolumeKey = metadataManager.getVolumeKey(volume);
+OmVolumeArgs volumeArgs = null;
+try {
+  volumeArgs = metadataManager.getVolumeTable().get(dbVolumeKey);

Review comment:
   Yes @xiaoyuyao . I do see the Line 1677-1682 check and message.
   
   I meant Line 1670-1671 OMException, since `volumeArgs` won't throw if volume 
is not found, we should change that message. Or just wrap IOException inside 
OMException?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org

[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1395: HDDS-4088. Adding Owner info for Authorizer plugin to honor owner access rights

2020-10-22 Thread GitBox 


smengcl commented on a change in pull request #1395:
URL: https://github.com/apache/hadoop-ozone/pull/1395#discussion_r510375521



##
File path: 
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##
@@ -1642,25 +1644,42 @@ private boolean hasAcls(String userName, ResourceType 
resType,
   UserGroupInformation.createRemoteUser(userName),
   ProtobufRpcEngine.Server.getRemoteIp(),
   ProtobufRpcEngine.Server.getRemoteIp().getHostName(),
-  false);
+  false, getVolumeOwner(vol, acl));
 } catch (OMException ex) {
   // Should not trigger exception here at all
   return false;
 }
   }
 
-  /**
-   * CheckAcls for the ozone object.
-   *
-   * @throws OMException ResultCodes.PERMISSION_DENIED if permission denied.
-   */
-  @SuppressWarnings("parameternumber")
-  public void checkAcls(ResourceType resType, StoreType storeType,
-  ACLType aclType, String vol, String bucket, String key,
-  UserGroupInformation ugi, InetAddress remoteAddress, String hostName)
-  throws OMException {
-checkAcls(resType, storeType, aclType, vol, bucket, key,
-ugi, remoteAddress, hostName, true);
+  public String getVolumeOwner(String vol, ACLType type) throws OMException {
+String volOwnerName = null;
+if (!vol.equals(OzoneConsts.OZONE_ROOT) && (type != ACLType.CREATE)) {
+  volOwnerName = getVolumeOwner(vol);
+}
+return volOwnerName;
+  }
+
+  private String getVolumeOwner(String volume) throws OMException {
+Boolean lockAcquired = metadataManager.getLock().acquireReadLock(
+VOLUME_LOCK, volume);
+String dbVolumeKey = metadataManager.getVolumeKey(volume);
+OmVolumeArgs volumeArgs = null;
+try {
+  volumeArgs = metadataManager.getVolumeTable().get(dbVolumeKey);
+} catch (IOException ioe) {
+  throw new OMException("Volume " + volume + " is not found",
+  OMException.ResultCodes.VOLUME_NOT_FOUND);

Review comment:
   I should tag here :) #tag





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org

[GitHub] [hadoop-ozone] smengcl commented on a change in pull request #1395: HDDS-4088. Adding Owner info for Authorizer plugin to honor owner access rights

2020-10-22 Thread GitBox 


smengcl commented on a change in pull request #1395:
URL: https://github.com/apache/hadoop-ozone/pull/1395#discussion_r510108891



##
File path: 
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##
@@ -1642,25 +1644,42 @@ private boolean hasAcls(String userName, ResourceType 
resType,
   UserGroupInformation.createRemoteUser(userName),
   ProtobufRpcEngine.Server.getRemoteIp(),
   ProtobufRpcEngine.Server.getRemoteIp().getHostName(),
-  false);
+  false, getVolumeOwner(vol, acl));
 } catch (OMException ex) {
   // Should not trigger exception here at all
   return false;
 }
   }
 
-  /**
-   * CheckAcls for the ozone object.
-   *
-   * @throws OMException ResultCodes.PERMISSION_DENIED if permission denied.
-   */
-  @SuppressWarnings("parameternumber")
-  public void checkAcls(ResourceType resType, StoreType storeType,
-  ACLType aclType, String vol, String bucket, String key,
-  UserGroupInformation ugi, InetAddress remoteAddress, String hostName)
-  throws OMException {
-checkAcls(resType, storeType, aclType, vol, bucket, key,
-ugi, remoteAddress, hostName, true);
+  public String getVolumeOwner(String vol, ACLType type) throws OMException {
+String volOwnerName = null;
+if (!vol.equals(OzoneConsts.OZONE_ROOT) && (type != ACLType.CREATE)) {
+  volOwnerName = getVolumeOwner(vol);
+}
+return volOwnerName;
+  }
+
+  private String getVolumeOwner(String volume) throws OMException {
+Boolean lockAcquired = metadataManager.getLock().acquireReadLock(
+VOLUME_LOCK, volume);
+String dbVolumeKey = metadataManager.getVolumeKey(volume);
+OmVolumeArgs volumeArgs = null;
+try {
+  volumeArgs = metadataManager.getVolumeTable().get(dbVolumeKey);

Review comment:
   `getVolumeTable().get(vol)` seems to return `null` when volume not found 
rather than throwing.
   
   the exception message in the following catch block needs to be changed.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



-
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org