Hadoop Common
  1. Hadoop Common
  2. HADOOP-2288

Change FileSystem API to support access control.

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.15.0
    • Fix Version/s: 0.16.0
    • Component/s: fs
    • Labels:
      None

      Description

      • Some FileSystem methods like create and mkdir need an additional parameter for permission.
      • FileSystem has to provide methods for setting permission, changing ownership, etc.
      1. 2288_20071203.patch
        39 kB
        Tsz Wo Nicholas Sze
      2. 2288_20071203b.patch
        40 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Hide
          Hairong Kuang added a comment -

          I'd like to add one more method to FSPermission that allows to apply a umask.
          <code>
          /** Appy a umask to this permission and return a new one */
          public FSPermission applyUMask( FSPermission umask);
          <code>

          Show
          Hairong Kuang added a comment - I'd like to add one more method to FSPermission that allows to apply a umask. <code> /** Appy a umask to this permission and return a new one */ public FSPermission applyUMask( FSPermission umask); <code>
          Hide
          Tsz Wo Nicholas Sze added a comment -
          • added methods for applyMask(...) instead of applyUMask(...) since UMask is file creation specific.
          • added a junit test
          Show
          Tsz Wo Nicholas Sze added a comment - added methods for applyMask(...) instead of applyUMask(...) since UMask is file creation specific. added a junit test
          Hide
          Raghu Angadi added a comment -

          Are we not borrowing unixism whenever it makes sense? Advantage is that not many need big explanations and its a great tie-breaker if there are minor difference of opinion about interface e.g.

          1. setUserOwner(path, user) would become chown(path, user, group).
          2. setGroupOwner() is not needed.
          3. we need chmod(). not sure if it replaces setPermission(), may be it should.
          Show
          Raghu Angadi added a comment - Are we not borrowing unixism whenever it makes sense? Advantage is that not many need big explanations and its a great tie-breaker if there are minor difference of opinion about interface e.g. setUserOwner(path, user) would become chown(path, user, group) . setGroupOwner() is not needed. we need chmod() . not sure if it replaces setPermission() , may be it should.
          Hide
          Sanjay Radia added a comment -

          FSPermissions:
          change name of set(short) to fromShort(short) to match your toShort() method.

          Names of methods.
          Overall I would have preferred to use the Unix names for all our methods. However HDFS has its own style of naming the
          methods. Nicholas's method names seem more in line with the HDFS style.

          Show
          Sanjay Radia added a comment - FSPermissions: change name of set(short) to fromShort(short) to match your toShort() method. Names of methods. Overall I would have preferred to use the Unix names for all our methods. However HDFS has its own style of naming the methods. Nicholas's method names seem more in line with the HDFS style.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          2288_20071128b.patch:

          • change set(short) to fromShort(short)
          • make the tests better
          • setPermission(...), setUserOwner(...) and setGroupOwner(...) remain unchanged.
          Show
          Tsz Wo Nicholas Sze added a comment - 2288_20071128b.patch: change set(short) to fromShort(short) make the tests better setPermission(...), setUserOwner(...) and setGroupOwner(...) remain unchanged.
          Hide
          Doug Cutting added a comment -

          This looks mostly good to me.

          This patch should implement the new API for the local filesystem. It'd be good to include at least one implementation, to make sure that it is easily and usefully implementable. Adding some tests for that implementation would also be good.

          We should not throw UnsupportedOperationException from the the default implementation of chmod, chown, chgrp, etc. The API will be easier to use if the default is to do nothing. That way one can, e.g., set permissions in JobClient without adding exception handling to ignore this exception. We want applications to still work with FileSystems that don't implement protections, and want folks to write FileSystem-independent code. Every application would end up ignoring these exceptions, so it would be best to not throw them in the first place.

          Perhaps these methods can return a boolean indicating success, so that if an application really cares it can tell, but that might be even more confusing. We want FileSystems that implement the API to throw exceptions when a protection operation fails, not return false. So false should only be returned if the FileSystem doesn't implement protections at all. My vote would be to leave them returning void and have the default implementation do nothing.

          A few nits:

          • all public and protected elements need javadoc.
          • the naming style FsFoo is preferred to FSFoo
          Show
          Doug Cutting added a comment - This looks mostly good to me. This patch should implement the new API for the local filesystem. It'd be good to include at least one implementation, to make sure that it is easily and usefully implementable. Adding some tests for that implementation would also be good. We should not throw UnsupportedOperationException from the the default implementation of chmod, chown, chgrp, etc. The API will be easier to use if the default is to do nothing. That way one can, e.g., set permissions in JobClient without adding exception handling to ignore this exception. We want applications to still work with FileSystems that don't implement protections, and want folks to write FileSystem-independent code. Every application would end up ignoring these exceptions, so it would be best to not throw them in the first place. Perhaps these methods can return a boolean indicating success, so that if an application really cares it can tell, but that might be even more confusing. We want FileSystems that implement the API to throw exceptions when a protection operation fails, not return false. So false should only be returned if the FileSystem doesn't implement protections at all. My vote would be to leave them returning void and have the default implementation do nothing. A few nits: all public and protected elements need javadoc. the naming style FsFoo is preferred to FSFoo
          Hide
          Tsz Wo Nicholas Sze added a comment -
          • merge setUserOwner(...) and setGroupOwner(...) to setOwner(...) since the name "setUserOwner" is not good as mentioned by Raghu.
          • implemented setOwner(...) and setPermssion(...) in RawLocalFileSystem and FilterFileSystem
          • Not throwing UnsupportedOperationException
          • provide default create(...) (and mkdir(...)) implementation by first call original create(...) and then call setPermission(...).
          • added javadoc
          • changed class names from FSFoo to FsFoo
          • added junit tests for LocalFileSystem
          Show
          Tsz Wo Nicholas Sze added a comment - merge setUserOwner(...) and setGroupOwner(...) to setOwner(...) since the name "setUserOwner" is not good as mentioned by Raghu. implemented setOwner(...) and setPermssion(...) in RawLocalFileSystem and FilterFileSystem Not throwing UnsupportedOperationException provide default create(...) (and mkdir(...)) implementation by first call original create(...) and then call setPermission(...). added javadoc changed class names from FSFoo to FsFoo added junit tests for LocalFileSystem
          Hide
          Tsz Wo Nicholas Sze added a comment -

          2288_20071129b.patch: fixed a bug.

          Show
          Tsz Wo Nicholas Sze added a comment - 2288_20071129b.patch: fixed a bug.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          2288_20071130.patch: Change applyMask(...) to applyUMask(...) since it is useful to HADOOP-2229.

          Show
          Tsz Wo Nicholas Sze added a comment - 2288_20071130.patch: Change applyMask(...) to applyUMask(...) since it is useful to HADOOP-2229 .
          Hide
          Tsz Wo Nicholas Sze added a comment -

          2288_20071130b.patch:
          In FileSystem, the original create(Path f, boolean overwrite, ...) is no longer abstract. It call the new create(Path f, FsPermission permission, boolean overwrite, ...) by default. Similar thing is done for mkdirs.

          Show
          Tsz Wo Nicholas Sze added a comment - 2288_20071130b.patch: In FileSystem, the original create(Path f, boolean overwrite, ...) is no longer abstract. It call the new create(Path f, FsPermission permission, boolean overwrite, ...) by default. Similar thing is done for mkdirs.
          Hide
          Doug Cutting added a comment -

          +1 This looks good to me.

          Show
          Doug Cutting added a comment - +1 This looks good to me.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12370725/2288_20071130c.patch
          against trunk revision r600019.

          @author +1. The patch does not contain any @author tags.

          javadoc -1. The javadoc tool appears to have generated messages.

          javac +1. The applied patch does not generate any new compiler warnings.

          findbugs -1. The patch appears to introduce 1 new Findbugs warnings.

          core tests -1. The patch failed core unit tests.

          contrib tests -1. The patch failed contrib unit tests.

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1224/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1224/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1224/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1224/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12370725/2288_20071130c.patch against trunk revision r600019. @author +1. The patch does not contain any @author tags. javadoc -1. The javadoc tool appears to have generated messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs -1. The patch appears to introduce 1 new Findbugs warnings. core tests -1. The patch failed core unit tests. contrib tests -1. The patch failed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1224/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1224/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1224/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1224/console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12370784/2288_20071201.patch
          against trunk revision r600244.

          @author +1. The patch does not contain any @author tags.

          javadoc +1. The javadoc tool did not generate any warning messages.

          javac +1. The applied patch does not generate any new compiler warnings.

          findbugs +1. The patch does not introduce any new Findbugs warnings.

          core tests +1. The patch passed core unit tests.

          contrib tests -1. The patch failed contrib unit tests.

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1233/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1233/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1233/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1233/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12370784/2288_20071201.patch against trunk revision r600244. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests -1. The patch failed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1233/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1233/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1233/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1233/console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          use "ls -ld" instead of "stat"

          Show
          Tsz Wo Nicholas Sze added a comment - use "ls -ld" instead of "stat"
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12370877/2288_20071203.patch
          against trunk revision r600627.

          @author +1. The patch does not contain any @author tags.

          javadoc +1. The javadoc tool did not generate any warning messages.

          javac +1. The applied patch does not generate any new compiler warnings.

          findbugs -1. The patch appears to introduce 3 new Findbugs warnings.

          core tests -1. The patch failed core unit tests.

          contrib tests -1. The patch failed contrib unit tests.

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1247/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1247/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1247/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1247/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12370877/2288_20071203.patch against trunk revision r600627. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs -1. The patch appears to introduce 3 new Findbugs warnings. core tests -1. The patch failed core unit tests. contrib tests -1. The patch failed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1247/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1247/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1247/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1247/console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12370894/2288_20071203b.patch
          against trunk revision r600707.

          @author +1. The patch does not contain any @author tags.

          javadoc +1. The javadoc tool did not generate any warning messages.

          javac +1. The applied patch does not generate any new compiler warnings.

          findbugs +1. The patch does not introduce any new Findbugs warnings.

          core tests +1. The patch passed core unit tests.

          contrib tests -1. The patch failed contrib unit tests.

          Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1249/testReport/
          Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1249/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1249/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1249/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12370894/2288_20071203b.patch against trunk revision r600707. @author +1. The patch does not contain any @author tags. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new compiler warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests -1. The patch failed contrib unit tests. Test results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1249/testReport/ Findbugs warnings: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1249/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1249/artifact/trunk/build/test/checkstyle-errors.html Console output: http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch/1249/console This message is automatically generated.
          Hide
          dhruba borthakur added a comment -

          I just committed this. Thanks Nicholas!

          Show
          dhruba borthakur added a comment - I just committed this. Thanks Nicholas!

            People

            • Assignee:
              Tsz Wo Nicholas Sze
              Reporter:
              Tsz Wo Nicholas Sze
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development