Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3733

Audit logs should include WebHDFS access

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: 2.0.2-alpha
    • Component/s: webhdfs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Access via WebHdfs does not result in audit log entries. It should.

      % curl "http://nn1:50070/webhdfs/v1/user/adi/hello.txt?op=GETFILESTATUS"
      {"FileStatus":{"accessTime":1343351432395,"blockSize":134217728,"group":"supergroup","length":12,"modificationTime":1342808158399,"owner":"adi","pathSuffix":"","permission":"644","replication":1,"type":"FILE"}}
      

      and observe that no audit log entry is generated.

      Interestingly, OPEN requests do not generate audit log entries when the NN generates the redirect, but do generate audit log entries when the second phase against the DN is executed.

      % curl -v 'http://nn1:50070/webhdfs/v1/user/adi/hello.txt?op=OPEN'
      ...
      < HTTP/1.1 307 TEMPORARY_REDIRECT
      < Location: http://dn01:50075/webhdfs/v1/user/adi/hello.txt?op=OPEN&namenoderpcaddress=nn1:8020&offset=0
      ...
      % curl -v 'http://dn01:50075/webhdfs/v1/user/adi/hello.txt?op=OPEN&namenoderpcaddress=nn1:8020'
      ...
      < HTTP/1.1 200 OK
      < Content-Type: application/octet-stream
      < Content-Length: 12
      < Server: Jetty(6.1.26.cloudera.1)
      < 
      hello world
      

      This happens because DatanodeWebHdfsMethods#get uses DFSClient#open thereby triggering the existing logAuditEvent code.

      1. hdfs-3733-7.txt
        26 kB
        Andy Isaacson
      2. hdfs-3733-6.txt
        27 kB
        Andy Isaacson
      3. hdfs-3733-4.txt
        27 kB
        Andy Isaacson
      4. hdfs-3733-3.txt
        28 kB
        Andy Isaacson
      5. hdfs-3733-2.txt
        26 kB
        Andy Isaacson
      6. hdfs-3733-1.txt
        24 kB
        Andy Isaacson
      7. hdfs-3733.txt
        20 kB
        Andy Isaacson

        Activity

        Hide
        Andy Isaacson added a comment -

        Additionally, since a denied OPEN does not hit the DN, only allowed OPEN requests generate audit log entries.

        Show
        Andy Isaacson added a comment - Additionally, since a denied OPEN does not hit the DN, only allowed OPEN requests generate audit log entries.
        Hide
        Andy Isaacson added a comment -

        Attaching a patch which addresses this issue.

        • add getRemoteIp helper to FSNamesystem
        • teach FSNamesystem#isExternalInvocation to know about NameNodeRpcServer.isWebHdfsInvocation as well as isRpcInvocation
        • add a ThreadLocal to NameNodeRpcServer and use it to implement isWebHdfsInvocation
        • add tests for WebHdfs to TestAuditLogs

        This patch has the following issues:

        • NameNodeRpcServer#CurClient needs to be set in more places; currently it is only set for open().
        • as a result the getFileStatus test does not succeed.

        I'm posting this work in progress to get feedback on the general approach. If people would like me to take a different tack, I'm happy to hear that.

        Show
        Andy Isaacson added a comment - Attaching a patch which addresses this issue. add getRemoteIp helper to FSNamesystem teach FSNamesystem#isExternalInvocation to know about NameNodeRpcServer.isWebHdfsInvocation as well as isRpcInvocation add a ThreadLocal to NameNodeRpcServer and use it to implement isWebHdfsInvocation add tests for WebHdfs to TestAuditLogs This patch has the following issues: NameNodeRpcServer#CurClient needs to be set in more places; currently it is only set for open(). as a result the getFileStatus test does not succeed. I'm posting this work in progress to get feedback on the general approach. If people would like me to take a different tack, I'm happy to hear that.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Hi Andy,

        WebHDFS access HDFS via NameNodeRpcServer. The access paths of WebHDFS should be similar to the ones in RPC. So I think the problems you found are not specific to WebHDFS. Have you checked DistributedFileSystem.getFileStatus(..)? Does it generate audit logs?

        For OPEN, WebHDFS calls getFileInfo(..) and getBlockLocations(..) via NameNodeRpcServer in the namenode side. These two methods seem not generating audit log.

        Suppose audit log is required. We should change FSNamesystem but not WebHDFS.

        Show
        Tsz Wo Nicholas Sze added a comment - Hi Andy, WebHDFS access HDFS via NameNodeRpcServer. The access paths of WebHDFS should be similar to the ones in RPC. So I think the problems you found are not specific to WebHDFS. Have you checked DistributedFileSystem.getFileStatus(..)? Does it generate audit logs? For OPEN, WebHDFS calls getFileInfo(..) and getBlockLocations(..) via NameNodeRpcServer in the namenode side. These two methods seem not generating audit log. Suppose audit log is required. We should change FSNamesystem but not WebHDFS.
        Hide
        Andy Isaacson added a comment -

        WebHDFS access HDFS via NameNodeRpcServer.

        Yes, and NNRS calls into FSNamesystem which does generate audit log entries if isExternalInvocation() returns true.

        The access paths of WebHDFS should be similar to the ones in RPC.

        The codepath is similar, but FSNamesystem only generates audit log events if the current request is coming from an RPC. It tells this by checking Server.isRpcInvocation.

        The attached patch does the following:

           private boolean isExternalInvocation() {
        -    return Server.isRpcInvocation();
        +    return Server.isRpcInvocation() || NameNodeRpcServer.isWebHdfsInvocation();
        +  }
        

        I think this is the approach you're suggesting, no?

        Have you checked DistributedFileSystem.getFileStatus(..)? Does it generate audit logs?

        Indeed, it does not.

        For OPEN, WebHDFS calls getFileInfo(..) and getBlockLocations(..) via NameNodeRpcServer in the namenode side. These two methods seem not generating audit log.

        Those get called in the NN (and don't generate audit log events), and then the NN sends the client a 307 redirect to a DN, and then the DN calls getBlockLocations again, which does generate an audit log on successful access. With my patch the NN side generates audit logs for both allowed and disallowed access.

        Show
        Andy Isaacson added a comment - WebHDFS access HDFS via NameNodeRpcServer. Yes, and NNRS calls into FSNamesystem which does generate audit log entries if isExternalInvocation() returns true. The access paths of WebHDFS should be similar to the ones in RPC. The codepath is similar, but FSNamesystem only generates audit log events if the current request is coming from an RPC. It tells this by checking Server.isRpcInvocation . The attached patch does the following: private boolean isExternalInvocation() { - return Server.isRpcInvocation(); + return Server.isRpcInvocation() || NameNodeRpcServer.isWebHdfsInvocation(); + } I think this is the approach you're suggesting, no? Have you checked DistributedFileSystem.getFileStatus(..)? Does it generate audit logs? Indeed, it does not. For OPEN, WebHDFS calls getFileInfo(..) and getBlockLocations(..) via NameNodeRpcServer in the namenode side. These two methods seem not generating audit log. Those get called in the NN (and don't generate audit log events), and then the NN sends the client a 307 redirect to a DN, and then the DN calls getBlockLocations again, which does generate an audit log on successful access. With my patch the NN side generates audit logs for both allowed and disallowed access.
        Hide
        Aaron T. Myers added a comment -

        Marking PA for Andy so that Jenkins runs.

        Show
        Aaron T. Myers added a comment - Marking PA for Andy so that Jenkins runs.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12538118/hdfs-3733.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.server.namenode.metrics.TestNameNodeMetrics
        org.apache.hadoop.hdfs.TestPersistBlocks
        org.apache.hadoop.hdfs.server.namenode.TestFsck

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2919//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2919//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/12538118/hdfs-3733.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.namenode.metrics.TestNameNodeMetrics org.apache.hadoop.hdfs.TestPersistBlocks org.apache.hadoop.hdfs.server.namenode.TestFsck +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2919//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2919//console This message is automatically generated.
        Hide
        Eli Collins added a comment -

        Overall approach makes sense.

        • s/CurClient/curClient/
        • Use {@link..}

          to refer to a.h.ipc.Server#isRpcInvocation

        • isWebHdfsInvocation returns true if client != null, and client is intialized with getClientMachine which can return non-null values in the non-webhdfs case, doesn't this imply isWebHdfsInvocation might return true even if the invocation is not for webhdfs?
        • getRemoteIp should not just return NamenodeWebHdfsMethods#getRemoteAddress because it returns null in the non-webhdfs case even though Server.getRemoteAddress() has a value, ie we're losing the remote IP for the non-webhdfs case
        • Looks like NamenodeWebHdfsMethods#setRemoteAddress is only used by the tests, can we remove it?
        • Please add a TestAuditLogs test that covers access via Hftp so we make sure we're not just doing something webhdfs specific
        Show
        Eli Collins added a comment - Overall approach makes sense. s/CurClient/curClient/ Use {@link..} to refer to a.h.ipc.Server#isRpcInvocation isWebHdfsInvocation returns true if client != null, and client is intialized with getClientMachine which can return non-null values in the non-webhdfs case, doesn't this imply isWebHdfsInvocation might return true even if the invocation is not for webhdfs? getRemoteIp should not just return NamenodeWebHdfsMethods#getRemoteAddress because it returns null in the non-webhdfs case even though Server.getRemoteAddress() has a value, ie we're losing the remote IP for the non-webhdfs case Looks like NamenodeWebHdfsMethods#setRemoteAddress is only used by the tests, can we remove it? Please add a TestAuditLogs test that covers access via Hftp so we make sure we're not just doing something webhdfs specific
        Hide
        Andy Isaacson added a comment -

        OK, backing up – I think my addition of CurClient just duplicates functionality already provided by NamenodeWebHdfsMethods#REMOTE_ADDRESS . So I can drop that new ThreadLocal and just teach NameNodeRpcServer to use REMOTE_ADDRESS appropriately.

        Or am I missing something?

        getRemoteIp should not just return NamenodeWebHdfsMethods#getRemoteAddress

        (I assume you are referring to my newly added FSNamesystem#getRemoteIp.)

        Agreed, FSNamesystem should support all remote methods: RPC, WebHdfs ... and Hftp? The FSNamesystem#getRemoteIp should handle them all.

        The helper NameNodeRpcServer#getRemoteIp implements the WebHdfs portion of FSNamesystem#getRemoteIp just as Server#getRemoteIp implements the RPC portion.

        Show
        Andy Isaacson added a comment - OK, backing up – I think my addition of CurClient just duplicates functionality already provided by NamenodeWebHdfsMethods#REMOTE_ADDRESS . So I can drop that new ThreadLocal and just teach NameNodeRpcServer to use REMOTE_ADDRESS appropriately. Or am I missing something? getRemoteIp should not just return NamenodeWebHdfsMethods#getRemoteAddress (I assume you are referring to my newly added FSNamesystem#getRemoteIp .) Agreed, FSNamesystem should support all remote methods: RPC, WebHdfs ... and Hftp? The FSNamesystem#getRemoteIp should handle them all. The helper NameNodeRpcServer#getRemoteIp implements the WebHdfs portion of FSNamesystem#getRemoteIp just as Server#getRemoteIp implements the RPC portion.
        Hide
        Andy Isaacson added a comment -

        Attaching new patch, removing the need for the ThreadLocal curClient by simply using NamenodeWebHdfsMethods#REMOTE_ADDRESS. Various other cleanups, and add test showing that Hftp is also audited.

        Show
        Andy Isaacson added a comment - Attaching new patch, removing the need for the ThreadLocal curClient by simply using NamenodeWebHdfsMethods#REMOTE_ADDRESS. Various other cleanups, and add test showing that Hftp is also audited.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12542898/hdfs-3733-1.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 1 new or modified test files.

        -1 javac. The patch appears to cause the build to fail.

        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3118//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/12542898/hdfs-3733-1.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. -1 javac. The patch appears to cause the build to fail. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3118//console This message is automatically generated.
        Hide
        Andy Isaacson added a comment -

        Sync patch with trunk and revert a gratuitous NL-at-EOF change that might be messing up patch application.

        Show
        Andy Isaacson added a comment - Sync patch with trunk and revert a gratuitous NL-at-EOF change that might be messing up patch application.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12542959/hdfs-3733-2.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 3 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.server.namenode.TestFsck
        org.apache.hadoop.hdfs.TestHftpDelegationToken

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3120//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3120//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3120//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/12542959/hdfs-3733-2.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.namenode.TestFsck org.apache.hadoop.hdfs.TestHftpDelegationToken +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3120//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3120//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3120//console This message is automatically generated.
        Hide
        Andy Isaacson added a comment -

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        TestFsck is due to audit log changes. TestHftpDelegationToken may be due to the change, it does fail for me but the linkage to audit logs is not obviuos.

        Show
        Andy Isaacson added a comment - -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: TestFsck is due to audit log changes. TestHftpDelegationToken may be due to the change, it does fail for me but the linkage to audit logs is not obviuos.
        Hide
        Andy Isaacson added a comment -

        TestHftpDelegationToken is known flaky. TestFsck broke because Fsck uses getFileInfo under the covers (see NamenodeFsck#fsck) and we now audit log those accesses correctly.

        So I've updated TestFsck's verifyAuditLogs to handle this case correctly. New patch attached.

        Show
        Andy Isaacson added a comment - TestHftpDelegationToken is known flaky. TestFsck broke because Fsck uses getFileInfo under the covers (see NamenodeFsck#fsck) and we now audit log those accesses correctly. So I've updated TestFsck's verifyAuditLogs to handle this case correctly. New patch attached.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12543005/hdfs-3733-3.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 4 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.TestHftpDelegationToken

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3121//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3121//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3121//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/12543005/hdfs-3733-3.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestHftpDelegationToken +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3121//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3121//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3121//console This message is automatically generated.
        Hide
        Andy Isaacson added a comment -

        The findbugs are unrelated and the HftpDelegationToken failure is unrelated. I believe this is ready to go.

        Show
        Andy Isaacson added a comment - The findbugs are unrelated and the HftpDelegationToken failure is unrelated. I believe this is ready to go.
        Hide
        Tsz Wo Nicholas Sze added a comment -
        • How about moving isWebHdfsInvocation() and getRemoteIp() from NameNodeRpcServer to NamenodeWebHdfsMethods? These two methods are not RPC related.
        • FSNamesystem.getRemoteIp() should be static.
        • The following change seems not useful.
          @@ -380,8 +389,11 @@ class NameNodeRpcServer implements NamenodeProtocols {
                                                     long length) 
                 throws IOException {
               metrics.incrGetBlockLocations();
          -    return namesystem.getBlockLocations(getClientMachine(), 
          -                                        src, offset, length);
          +    try {
          +      return namesystem.getBlockLocations(getClientMachine(), 
          +                                          src, offset, length);
          +    } finally {
          +    }
          
        Show
        Tsz Wo Nicholas Sze added a comment - How about moving isWebHdfsInvocation() and getRemoteIp() from NameNodeRpcServer to NamenodeWebHdfsMethods? These two methods are not RPC related. FSNamesystem.getRemoteIp() should be static. The following change seems not useful. @@ -380,8 +389,11 @@ class NameNodeRpcServer implements NamenodeProtocols { long length) throws IOException { metrics.incrGetBlockLocations(); - return namesystem.getBlockLocations(getClientMachine(), - src, offset, length); + try { + return namesystem.getBlockLocations(getClientMachine(), + src, offset, length); + } finally { + }
        Hide
        Andy Isaacson added a comment -

        How about moving isWebHdfsInvocation() and getRemoteIp() from NameNodeRpcServer to NamenodeWebHdfsMethods? These two methods are not RPC related.

        Fair enough, done.

        FSNamesystem.getRemoteIp() should be static.

        Yep, thanks.

        The following change seems not useful.

        It made more sense in a previous version of the patch. Fixed!

        Show
        Andy Isaacson added a comment - How about moving isWebHdfsInvocation() and getRemoteIp() from NameNodeRpcServer to NamenodeWebHdfsMethods? These two methods are not RPC related. Fair enough, done. FSNamesystem.getRemoteIp() should be static. Yep, thanks. The following change seems not useful. It made more sense in a previous version of the patch. Fixed!
        Hide
        Andy Isaacson added a comment -

        Attaching latest version of patch.

        Show
        Andy Isaacson added a comment - Attaching latest version of patch.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        +1
        Andy, thanks for the update. The new patch looks good.

        Show
        Tsz Wo Nicholas Sze added a comment - +1 Andy, thanks for the update. The new patch looks good.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12543049/hdfs-3733-4.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 4 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.TestHftpDelegationToken
        org.apache.hadoop.hdfs.TestClientReportBadBlock

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3122//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3122//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3122//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/12543049/hdfs-3733-4.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestHftpDelegationToken org.apache.hadoop.hdfs.TestClientReportBadBlock +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3122//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3122//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3122//console This message is automatically generated.
        Hide
        Eli Collins added a comment -

        Andy, looking good!

        • In FSN#getFileInfo why catch UnresolvedLinkException and StandbyException, just AccessControlException is sufficient right?
        • Nit, I'd remove the System.out.printlns for debugging in the tests?
        • Per jenkins there's a javadoc warning:
          [WARNING] /home/eli/src/hadoop2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/web/resources/NamenodeWebHdfsMethods.java:132: warning - Tag @link: reference not found: Server#isRpcInvocation()
          
        Show
        Eli Collins added a comment - Andy, looking good! In FSN#getFileInfo why catch UnresolvedLinkException and StandbyException, just AccessControlException is sufficient right? Nit, I'd remove the System.out.printlns for debugging in the tests? Per jenkins there's a javadoc warning: [WARNING] /home/eli/src/hadoop2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/web/resources/NamenodeWebHdfsMethods.java:132: warning - Tag @link: reference not found: Server#isRpcInvocation()
        Hide
        Andy Isaacson added a comment -

        In FSN#getFileInfo why catch UnresolvedLinkException and StandbyException, just AccessControlException is sufficient right?

        I have to logAuditEvent(false under any exception. Todd suggested doing this instead:

        +    } catch (Throwable e) {
               if (auditLog.isInfoEnabled() && isExternalInvocation()) {
                 logAuditEvent(false, UserGroupInformation.getCurrentUser(),
                               getRemoteIp(),
                               "getfileinfo", src, null, null);
               }
        -      throw e;
        -    } catch (StandbyException e) {
        -      if (auditLog.isInfoEnabled() && isExternalInvocation()) {
        -        logAuditEvent(false, UserGroupInformation.getCurrentUser(),
        -                      getRemoteIp(),
        -                      "getfileinfo", src, null, null);
        -      }
        -      throw e;
        +      Throwables.propagateIfPossible(e, AccessControlException.class);
        +      Throwables.propagateIfPossible(e, UnresolvedLinkException.class);
        +      Throwables.propagateIfPossible(e, StandbyException.class);
        +      Throwables.propagateIfPossible(e, IOException.class);
        +      throw new RuntimeException("unexpected", e);
        

        Nit, I'd remove the System.out.printlns for debugging in the tests?

        Where's the upside to removing them? It adds a few KB at most to the MBs of test output, and I always end up adding the prinlns when trying to grok failures.

        But, whatever. Removed.

        javadoc warning

        Turns out you have to import anything you want to @link. Fixed.

        Show
        Andy Isaacson added a comment - In FSN#getFileInfo why catch UnresolvedLinkException and StandbyException, just AccessControlException is sufficient right? I have to logAuditEvent(false under any exception. Todd suggested doing this instead: + } catch (Throwable e) { if (auditLog.isInfoEnabled() && isExternalInvocation()) { logAuditEvent( false , UserGroupInformation.getCurrentUser(), getRemoteIp(), "getfileinfo" , src, null , null ); } - throw e; - } catch (StandbyException e) { - if (auditLog.isInfoEnabled() && isExternalInvocation()) { - logAuditEvent( false , UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "getfileinfo" , src, null , null ); - } - throw e; + Throwables.propagateIfPossible(e, AccessControlException.class); + Throwables.propagateIfPossible(e, UnresolvedLinkException.class); + Throwables.propagateIfPossible(e, StandbyException.class); + Throwables.propagateIfPossible(e, IOException.class); + throw new RuntimeException( "unexpected" , e); Nit, I'd remove the System.out.printlns for debugging in the tests? Where's the upside to removing them? It adds a few KB at most to the MBs of test output, and I always end up adding the prinlns when trying to grok failures. But, whatever. Removed. javadoc warning Turns out you have to import anything you want to @link . Fixed.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12543151/hdfs-3733-6.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 4 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.TestDatanodeBlockScanner
        org.apache.hadoop.hdfs.TestHftpDelegationToken

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3127//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3127//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3127//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/12543151/hdfs-3733-6.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDatanodeBlockScanner org.apache.hadoop.hdfs.TestHftpDelegationToken +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3127//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/3127//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3127//console This message is automatically generated.
        Hide
        Andy Isaacson added a comment -

        I have to logAuditEvent(false under any exception.

        This false assumption was the root of my confusion. In fact, if an exception other than ACE occurs, there's no need to logAuditEvent. None of the other callsites do so.

        Thanks for bringing this up, Eli. New patch attached.

        Show
        Andy Isaacson added a comment - I have to logAuditEvent(false under any exception. This false assumption was the root of my confusion. In fact, if an exception other than ACE occurs, there's no need to logAuditEvent. None of the other callsites do so. Thanks for bringing this up, Eli. New patch attached.
        Hide
        Eli Collins added a comment -

        Looks great Andy.

        +1 pending jenkins.

        Show
        Eli Collins added a comment - Looks great Andy. +1 pending jenkins.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12543203/hdfs-3733-7.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 4 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.TestHftpDelegationToken

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3131//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3131//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/12543203/hdfs-3733-7.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestHftpDelegationToken +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/3131//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/3131//console This message is automatically generated.
        Hide
        Eli Collins added a comment -

        Test failure is unrelated.

        I've committed this. Thanks Andy!

        Show
        Eli Collins added a comment - Test failure is unrelated. I've committed this. Thanks Andy!
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #1151 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1151/)
        HDFS-3733. Audit logs should include WebHDFS access. Contributed by Andy Isaacson (Revision 1379278)

        Result = FAILURE
        eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379278
        Files :

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/web/resources/NamenodeWebHdfsMethods.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogs.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFsck.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/web/resources/TestWebHdfsDataLocality.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1151 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1151/ ) HDFS-3733 . Audit logs should include WebHDFS access. Contributed by Andy Isaacson (Revision 1379278) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379278 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/web/resources/NamenodeWebHdfsMethods.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogs.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFsck.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/web/resources/TestWebHdfsDataLocality.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #1182 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1182/)
        HDFS-3733. Audit logs should include WebHDFS access. Contributed by Andy Isaacson (Revision 1379278)

        Result = SUCCESS
        eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379278
        Files :

        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/web/resources/NamenodeWebHdfsMethods.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogs.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFsck.java
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/web/resources/TestWebHdfsDataLocality.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1182 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1182/ ) HDFS-3733 . Audit logs should include WebHDFS access. Contributed by Andy Isaacson (Revision 1379278) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379278 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/web/resources/NamenodeWebHdfsMethods.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NameNodeAdapter.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogs.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFsck.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/web/resources/TestWebHdfsDataLocality.java

          People

          • Assignee:
            Andy Isaacson
            Reporter:
            Andy Isaacson
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development