Uploaded image for project: 'Flume'
  1. Flume
  2. FLUME-3049

Wrapping the exception into SecurityException in UGIExecutor.execute hides the original one

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.0
    • Component/s: None
    • Labels:
      None

      Description

      see: https://github.com/apache/flume/blob/trunk/flume-ng-auth/src/main/java/org/apache/flume/auth/UGIExecutor.java#L49

      This has unexpected side effects as the callers try to catch the wrapped exception, for example in BucketWriter.append(): https://github.com/apache/flume/blob/trunk/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java#L563
      Here IOException is considered as non-transient failure thus the close() is called, but when the original exception is wrapped into SecurityException it doesn't trigger the close of the file.
      Similarly in HDFSEventSink.process() method the `IOException` is handled in a different way than any other exception. It might come from BucketWriter.append() or BucketWriter.flush() for example, and both of them invoke the hdfs call via a PrivilegedExecutor instance which might be the problematic UGIExecutor.

      The bottom line is that the contract in PrivilegedExecutor.execute() is that they shouldn't change the exception thrown in the business logic - at least it's not indicated in its signature in any way. The default implementation (SimpleAuthenticator) behaves according to this.

      I don't know the original intend behind this wrapping, Johny Rufus or Hari Shreedharan, do you happen to remember? (You were involved in the original implementation in FLUME-2631)

      Right now I don't see any problem in removing this and letting the original exception to propagate as the org.apache.flume.auth.SecurityException doesn't appear anywhere in the public interface.

        Issue Links

          Activity

          Hide
          hudson Hudson added a comment -

          ABORTED: Integrated in Jenkins build Flume-trunk-hbase-1 #239 (See https://builds.apache.org/job/Flume-trunk-hbase-1/239/)
          FLUME-3049. Make HDFS sink rotate more reliably in secure mode (mpercy: http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=f215374bdf9a08b16fa7ccd3b40098024afe8677)

          • (edit) flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java
          • (edit) flume-ng-auth/src/main/java/org/apache/flume/auth/UGIExecutor.java
          • (edit) flume-ng-auth/src/test/java/org/apache/flume/auth/TestFlumeAuthenticator.java
          Show
          hudson Hudson added a comment - ABORTED: Integrated in Jenkins build Flume-trunk-hbase-1 #239 (See https://builds.apache.org/job/Flume-trunk-hbase-1/239/ ) FLUME-3049 . Make HDFS sink rotate more reliably in secure mode (mpercy: http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=f215374bdf9a08b16fa7ccd3b40098024afe8677 ) (edit) flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java (edit) flume-ng-auth/src/main/java/org/apache/flume/auth/UGIExecutor.java (edit) flume-ng-auth/src/test/java/org/apache/flume/auth/TestFlumeAuthenticator.java
          Hide
          mpercy Mike Percy added a comment -

          Pushed to trunk. Thanks for the patch, Denes!

          Show
          mpercy Mike Percy added a comment - Pushed to trunk. Thanks for the patch, Denes!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flume/pull/106

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flume/pull/106
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f215374bdf9a08b16fa7ccd3b40098024afe8677 in flume's branch refs/heads/trunk from Denes Arvay
          [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=f215374 ]

          FLUME-3049. Make HDFS sink rotate more reliably in secure mode

          It was reported that the HDFS sink had a bug where file rotation was not
          reliable in secure mode.

          After investigating, it turns out that this was caused by a bug in the
          FlumeAuthenticator code: A "try" block in UGIExecutor.execute() was
          wrapping exceptions (such as IOException) with a SecurityException.

          That exception wrapping was breaking the contract of BucketWriter
          because the caller (HDFSEventSink) did not understand how to react to
          the SecurityException. This also likely had other negative effects in
          exceptional cases.

          The following changes are included in this patch:

          • Remove the exception wrapping in UGIExecutor.execute().
          • Add tests for exception propagation in FlumeAuthenticator
            implementations.
          • Add testRotateBucketOnIOException() to TestBucketWriter as a
            regression test for the HDFS sink issue.

          Closes #106.

          Reviewers: Attila Simon, Mike Percy

          (Denes Arvay via Mike Percy)

          Show
          jira-bot ASF subversion and git services added a comment - Commit f215374bdf9a08b16fa7ccd3b40098024afe8677 in flume's branch refs/heads/trunk from Denes Arvay [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=f215374 ] FLUME-3049 . Make HDFS sink rotate more reliably in secure mode It was reported that the HDFS sink had a bug where file rotation was not reliable in secure mode. After investigating, it turns out that this was caused by a bug in the FlumeAuthenticator code: A "try" block in UGIExecutor.execute() was wrapping exceptions (such as IOException) with a SecurityException. That exception wrapping was breaking the contract of BucketWriter because the caller (HDFSEventSink) did not understand how to react to the SecurityException. This also likely had other negative effects in exceptional cases. The following changes are included in this patch: Remove the exception wrapping in UGIExecutor.execute(). Add tests for exception propagation in FlumeAuthenticator implementations. Add testRotateBucketOnIOException() to TestBucketWriter as a regression test for the HDFS sink issue. Closes #106. Reviewers: Attila Simon, Mike Percy (Denes Arvay via Mike Percy)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f215374bdf9a08b16fa7ccd3b40098024afe8677 in flume's branch refs/heads/flume-3049-3 from Denes Arvay
          [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=f215374 ]

          FLUME-3049. Make HDFS sink rotate more reliably in secure mode

          It was reported that the HDFS sink had a bug where file rotation was not
          reliable in secure mode.

          After investigating, it turns out that this was caused by a bug in the
          FlumeAuthenticator code: A "try" block in UGIExecutor.execute() was
          wrapping exceptions (such as IOException) with a SecurityException.

          That exception wrapping was breaking the contract of BucketWriter
          because the caller (HDFSEventSink) did not understand how to react to
          the SecurityException. This also likely had other negative effects in
          exceptional cases.

          The following changes are included in this patch:

          • Remove the exception wrapping in UGIExecutor.execute().
          • Add tests for exception propagation in FlumeAuthenticator
            implementations.
          • Add testRotateBucketOnIOException() to TestBucketWriter as a
            regression test for the HDFS sink issue.

          Closes #106.

          Reviewers: Attila Simon, Mike Percy

          (Denes Arvay via Mike Percy)

          Show
          jira-bot ASF subversion and git services added a comment - Commit f215374bdf9a08b16fa7ccd3b40098024afe8677 in flume's branch refs/heads/flume-3049-3 from Denes Arvay [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=f215374 ] FLUME-3049 . Make HDFS sink rotate more reliably in secure mode It was reported that the HDFS sink had a bug where file rotation was not reliable in secure mode. After investigating, it turns out that this was caused by a bug in the FlumeAuthenticator code: A "try" block in UGIExecutor.execute() was wrapping exceptions (such as IOException) with a SecurityException. That exception wrapping was breaking the contract of BucketWriter because the caller (HDFSEventSink) did not understand how to react to the SecurityException. This also likely had other negative effects in exceptional cases. The following changes are included in this patch: Remove the exception wrapping in UGIExecutor.execute(). Add tests for exception propagation in FlumeAuthenticator implementations. Add testRotateBucketOnIOException() to TestBucketWriter as a regression test for the HDFS sink issue. Closes #106. Reviewers: Attila Simon, Mike Percy (Denes Arvay via Mike Percy)
          Hide
          denes Denes Arvay added a comment -

          Note: I updated the description to make the intent more clear.

          Show
          denes Denes Arvay added a comment - Note: I updated the description to make the intent more clear.
          Hide
          denes Denes Arvay added a comment -

          Thanks Hari Shreedharan, I have opened a pull request for this change.

          Show
          denes Denes Arvay added a comment - Thanks Hari Shreedharan , I have opened a pull request for this change.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user adenes opened a pull request:

          https://github.com/apache/flume/pull/106

          FLUME-3049: Remove exception wrapping in UGIExecutor.execute()

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/adenes/flume FLUME-3049

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flume/pull/106.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #106


          commit a4286afc5eb28cb7d89137a53bfe47c365e5a170
          Author: Denes Arvay <denes@cloudera.com>
          Date: 2017-01-27T10:43:27Z

          FLUME-3049: Remove exception wrapping in UGIExecutor.execute()


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user adenes opened a pull request: https://github.com/apache/flume/pull/106 FLUME-3049 : Remove exception wrapping in UGIExecutor.execute() You can merge this pull request into a Git repository by running: $ git pull https://github.com/adenes/flume FLUME-3049 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flume/pull/106.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #106 commit a4286afc5eb28cb7d89137a53bfe47c365e5a170 Author: Denes Arvay <denes@cloudera.com> Date: 2017-01-27T10:43:27Z FLUME-3049 : Remove exception wrapping in UGIExecutor.execute()
          Hide
          hshreedharan Hari Shreedharan added a comment -

          I think that is a bug and was an oversight when I reviewed it. I think we should throw the actual exception itself. Please go ahead and throw the actual exception and removing the SecurityException

          Show
          hshreedharan Hari Shreedharan added a comment - I think that is a bug and was an oversight when I reviewed it. I think we should throw the actual exception itself. Please go ahead and throw the actual exception and removing the SecurityException

            People

            • Assignee:
              denes Denes Arvay
              Reporter:
              denes Denes Arvay
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development