Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.8.0
    • Fix Version/s: 2.9.0, 3.0.0-alpha4
    • Component/s: fs/s3
    • Labels:
      None
    • Target Version/s:
    • Release Note:
      Hide
      The new encryption options SSE-KMS and especially SSE-C must be considered experimental at present. If you are using SSE-C, problems may arise if the bucket mixes encrypted and unencrypted files. For SSE-KMS, there may be extra throttling of IO, especially with the fadvise=random option. You may wish to request an increase in your KMS IOPs limits.
      Show
      The new encryption options SSE-KMS and especially SSE-C must be considered experimental at present. If you are using SSE-C, problems may arise if the bucket mixes encrypted and unencrypted files. For SSE-KMS, there may be extra throttling of IO, especially with the fadvise=random option. You may wish to request an increase in your KMS IOPs limits.

      Description

      S3 provides 3 types of server-side encryption [1],

      • SSE-S3 (Amazon S3-Managed Keys) [2]
      • SSE-KMS (AWS KMS-Managed Keys) [3]
      • SSE-C (Customer-Provided Keys) [4]

      Of which the S3AFileSystem in hadoop-aws only supports opting into SSE-S3 (HADOOP-10568) – the underlying aws-java-sdk makes that very simple [5]. With native support in aws-java-sdk already available it should be fairly straightforward [6],[7] to support the other two types of SSE with some additional fs.s3a configuration properties.

      [1] http://docs.aws.amazon.com/AmazonS3/latest/dev/serv-side-encryption.html
      [2] http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingServerSideEncryption.html
      [3] http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingKMSEncryption.html
      [4] http://docs.aws.amazon.com/AmazonS3/latest/dev/ServerSideEncryptionCustomerKeys.html
      [5] http://docs.aws.amazon.com/AmazonS3/latest/dev/SSEUsingJavaSDK.html
      [6] http://docs.aws.amazon.com/AmazonS3/latest/dev/kms-using-sdks.html#kms-using-sdks-java
      [7] http://docs.aws.amazon.com/AmazonS3/latest/dev/sse-c-using-java-sdk.html

      1. HADOOP-13075-branch2.002.patch
        59 kB
        Steve Moist
      2. HADOOP-13075-003.patch
        59 kB
        Steve Moist
      3. HADOOP-13075-002.patch
        55 kB
        Steve Moist
      4. HADOOP-13075-001.patch
        55 kB
        Steve Moist

        Issue Links

          Activity

          Hide
          stevel@apache.org Steve Loughran added a comment -

          As usual: which SDK version adds this feature?

          Show
          stevel@apache.org Steve Loughran added a comment - As usual: which SDK version adds this feature?
          Show
          noslowerdna Andrew Olson added a comment - Looks like 1.9.x, https://github.com/aws/aws-sdk-java/blob/1.9.20/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/model/SSEAwsKeyManagementParamsProvider.java https://github.com/aws/aws-sdk-java/blob/1.9.20/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/model/SSECustomerKeyProvider.java
          Hide
          stevel@apache.org Steve Loughran added a comment -

          thanks —means there's no need to bump up the POMs

          new config properties and a test would be nice; the ContractTestUtils code to write then read back a file should be enough

          Show
          stevel@apache.org Steve Loughran added a comment - thanks —means there's no need to bump up the POMs new config properties and a test would be nice; the ContractTestUtils code to write then read back a file should be enough
          Hide
          stevel@apache.org Steve Loughran added a comment -

          There's currently no tests that SSE-S3 works; something there will be needed as part of this patch —for regression testing.

          Show
          stevel@apache.org Steve Loughran added a comment - There's currently no tests that SSE-S3 works; something there will be needed as part of this patch —for regression testing.
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          May I ask what's the status of this ticket? The ticket is Unassigned so I assume nobody is working on this? I'm asking because I have a working patch that enables SSE-KMS when writing/reading to/from S3. I haven't done it for SSE-C though. Is anybody interested?
          Cheers

          Fede

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - May I ask what's the status of this ticket? The ticket is Unassigned so I assume nobody is working on this? I'm asking because I have a working patch that enables SSE-KMS when writing/reading to/from S3. I haven't done it for SSE-C though. Is anybody interested? Cheers Fede
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          BTW, the aws sdk version that introduces KMS, is 1.9.5 https://aws.amazon.com/releasenotes/Java/1865230316820658

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - BTW, the aws sdk version that introduces KMS, is 1.9.5 https://aws.amazon.com/releasenotes/Java/1865230316820658
          Hide
          cnauroth Chris Nauroth added a comment -

          Federico Czerwinski, you're correct that an unassigned JIRA indicates that no one is actively working on it. Thank you very much for volunteering to share your patch. I have assigned this issue to you.

          Please refer to our HowToContribute wiki page for more details about how the contribution process works. In particular, please notice the section on Submitting patches against object stores, where we describe additional testing requirements for S3A patches.

          HADOOP-13131 is a recent patch that introduced test suites for encryption: TestS3AEncryption and TestS3AEncryptionAlgorithmPropagation. We can probably add new tests for SSE-KMS and SSE-C in there.

          If possible, it would be great to cover both SSE-KMS and SSE-C at the same time in one patch.

          BTW, the aws sdk version that introduces KMS, is 1.9.5

          That's great. We're currently on 1.10.6.

          Show
          cnauroth Chris Nauroth added a comment - Federico Czerwinski , you're correct that an unassigned JIRA indicates that no one is actively working on it. Thank you very much for volunteering to share your patch. I have assigned this issue to you. Please refer to our HowToContribute wiki page for more details about how the contribution process works. In particular, please notice the section on Submitting patches against object stores , where we describe additional testing requirements for S3A patches. HADOOP-13131 is a recent patch that introduced test suites for encryption: TestS3AEncryption and TestS3AEncryptionAlgorithmPropagation . We can probably add new tests for SSE-KMS and SSE-C in there. If possible, it would be great to cover both SSE-KMS and SSE-C at the same time in one patch. BTW, the aws sdk version that introduces KMS, is 1.9.5 That's great. We're currently on 1.10.6.
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          Thanks Chris Nauroth, I have my patch done in branch 2.7.2, so I'll need to port it to trunk.

          Fede

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - Thanks Chris Nauroth , I have my patch done in branch 2.7.2, so I'll need to port it to trunk. Fede
          Hide
          spacepluk Oscar Morante added a comment -

          Hi Federico,
          Would you share your patch for 2.7.2 in the meantime? I would love to try it.

          Show
          spacepluk Oscar Morante added a comment - Hi Federico, Would you share your patch for 2.7.2 in the meantime? I would love to try it.
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          hey Chris Nauroth, I've seen that you set the target to 2.9.0, does that mean that I have to make the patch against that version and not trunk? I'm not familiar with Hadoop's release cycle.
          Thanks

          Fede

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - hey Chris Nauroth , I've seen that you set the target to 2.9.0, does that mean that I have to make the patch against that version and not trunk? I'm not familiar with Hadoop's release cycle. Thanks Fede
          Hide
          cnauroth Chris Nauroth added a comment -

          Federico Czerwinski, please develop patches that can be applied to the trunk and branch-2 branches. The S3A codebase is nearly identical between those 2 branches, so it's highly likely that you'll be able to produce one patch file that can apply cleanly to both branches. If you discover that your patch requires different code on trunk and branch-2, then the HowToContribute wiki page has instructions on Naming your patch to indicate which branch it targets.

          I also moved this issue under HADOOP-13204 as a sub-task. That's an umbrella S3A Phase III issue that we're using to aggregate S3A enhancements targeted to Apache Hadoop 2.9.0.

          Show
          cnauroth Chris Nauroth added a comment - Federico Czerwinski , please develop patches that can be applied to the trunk and branch-2 branches. The S3A codebase is nearly identical between those 2 branches, so it's highly likely that you'll be able to produce one patch file that can apply cleanly to both branches. If you discover that your patch requires different code on trunk and branch-2, then the HowToContribute wiki page has instructions on Naming your patch to indicate which branch it targets. I also moved this issue under HADOOP-13204 as a sub-task. That's an umbrella S3A Phase III issue that we're using to aggregate S3A enhancements targeted to Apache Hadoop 2.9.0.
          Hide
          sergey.mazin Sergey Mazin added a comment -

          Could you please share patch here? We would like to check it.

          Show
          sergey.mazin Sergey Mazin added a comment - Could you please share patch here? We would like to check it.
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          That patch is only a proof of concept and doesn't support SSE-C. I have this ticket almost done, just fixing a few tests.

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - That patch is only a proof of concept and doesn't support SSE-C. I have this ticket almost done, just fixing a few tests.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user fedecz opened a pull request:

          https://github.com/apache/hadoop/pull/110

          HADOOP-13075 Adding support for SSE-KMS, SSE-C and SSE-S3

          https://issues.apache.org/jira/browse/HADOOP-13075

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

          $ git pull https://github.com/fedecz/hadoop HADOOP-13075

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

          https://github.com/apache/hadoop/pull/110.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 #110


          commit bd465b90db9ea01c2076cab1d21cb9deb3424536
          Author: Federico Czerwinski <fede@datarepublic.com.au>
          Date: 2016-06-29T00:03:52Z

          HADOOP-13075 Adding support for SSE-KMS and SSE-C


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user fedecz opened a pull request: https://github.com/apache/hadoop/pull/110 HADOOP-13075 Adding support for SSE-KMS, SSE-C and SSE-S3 https://issues.apache.org/jira/browse/HADOOP-13075 You can merge this pull request into a Git repository by running: $ git pull https://github.com/fedecz/hadoop HADOOP-13075 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/hadoop/pull/110.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 #110 commit bd465b90db9ea01c2076cab1d21cb9deb3424536 Author: Federico Czerwinski <fede@datarepublic.com.au> Date: 2016-06-29T00:03:52Z HADOOP-13075 Adding support for SSE-KMS and SSE-C
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          hey Chris Nauroth, as you can see I've created the PR for trunk, I'm working on the PR for branch-2 now. Cheers

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - hey Chris Nauroth , as you can see I've created the PR for trunk, I'm working on the PR for branch-2 now. Cheers
          Hide
          noslowerdna Andrew Olson added a comment -

          The pull request looks good from here. Thanks!

          Show
          noslowerdna Andrew Olson added a comment - The pull request looks good from here. Thanks!
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          When I run mvn test in branch-2, without my patch, I get the following error (with is the same error I'm getting with my patch on):

          testWithMiniCluster(org.apache.hadoop.fs.s3a.yarn.TestS3AMiniYarnCluster)  Time elapsed: 4.112 sec  <<< ERROR!
          java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
          	at org.apache.hadoop.yarn.server.resourcemanager.scheduler.SchedulerHealth.<init>(SchedulerHealth.java:78)
          	at org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacityScheduler.<init>(CapacityScheduler.java:233)
          	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
          	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
          	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
          	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
          	at org.apache.hadoop.util.ReflectionUtils.newInstance(ReflectionUtils.java:132)
          	at org.apache.hadoop.yarn.server.resourcemanager.ResourceManager.createScheduler(ResourceManager.java:386)
          	at org.apache.hadoop.yarn.server.resourcemanager.ResourceManager$RMActiveServices.serviceInit(ResourceManager.java:575)
          	at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163)
          	at org.apache.hadoop.yarn.server.resourcemanager.ResourceManager.createAndInitActiveServices(ResourceManager.java:1001)
          	at org.apache.hadoop.yarn.server.resourcemanager.ResourceManager.serviceInit(ResourceManager.java:290)
          	at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163)
          	at org.apache.hadoop.yarn.server.MiniYARNCluster.initResourceManager(MiniYARNCluster.java:321)
          	at org.apache.hadoop.yarn.server.MiniYARNCluster.access$200(MiniYARNCluster.java:115)
          	at org.apache.hadoop.yarn.server.MiniYARNCluster$ResourceManagerWrapper.serviceInit(MiniYARNCluster.java:461)
          	at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163)
          	at org.apache.hadoop.service.CompositeService.serviceInit(CompositeService.java:107)
          	at org.apache.hadoop.yarn.server.MiniYARNCluster.serviceInit(MiniYARNCluster.java:289)
          	at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163)
          	at org.apache.hadoop.fs.s3a.yarn.TestS3AMiniYarnCluster.beforeTest(TestS3AMiniYarnCluster.java:64)
          

          any ideas? this is with version c3d9ac8, which is from about an hour.

          cheers

          Fede

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - When I run mvn test in branch-2 , without my patch, I get the following error (with is the same error I'm getting with my patch on): testWithMiniCluster(org.apache.hadoop.fs.s3a.yarn.TestS3AMiniYarnCluster) Time elapsed: 4.112 sec <<< ERROR! java.lang.RuntimeException: java.lang.reflect.InvocationTargetException at org.apache.hadoop.yarn.server.resourcemanager.scheduler.SchedulerHealth.<init>(SchedulerHealth.java:78) at org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacityScheduler.<init>(CapacityScheduler.java:233) at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.lang.reflect.Constructor.newInstance(Constructor.java:423) at org.apache.hadoop.util.ReflectionUtils.newInstance(ReflectionUtils.java:132) at org.apache.hadoop.yarn.server.resourcemanager.ResourceManager.createScheduler(ResourceManager.java:386) at org.apache.hadoop.yarn.server.resourcemanager.ResourceManager$RMActiveServices.serviceInit(ResourceManager.java:575) at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163) at org.apache.hadoop.yarn.server.resourcemanager.ResourceManager.createAndInitActiveServices(ResourceManager.java:1001) at org.apache.hadoop.yarn.server.resourcemanager.ResourceManager.serviceInit(ResourceManager.java:290) at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163) at org.apache.hadoop.yarn.server.MiniYARNCluster.initResourceManager(MiniYARNCluster.java:321) at org.apache.hadoop.yarn.server.MiniYARNCluster.access$200(MiniYARNCluster.java:115) at org.apache.hadoop.yarn.server.MiniYARNCluster$ResourceManagerWrapper.serviceInit(MiniYARNCluster.java:461) at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163) at org.apache.hadoop.service.CompositeService.serviceInit(CompositeService.java:107) at org.apache.hadoop.yarn.server.MiniYARNCluster.serviceInit(MiniYARNCluster.java:289) at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163) at org.apache.hadoop.fs.s3a.yarn.TestS3AMiniYarnCluster.beforeTest(TestS3AMiniYarnCluster.java:64) any ideas? this is with version c3d9ac8 , which is from about an hour. cheers Fede
          Hide
          cnauroth Chris Nauroth added a comment -

          Federico Czerwinski, I'm unable to reproduce that test failure. Sometimes this kind of problem happens if a dependent module has changed but not rebuilt. I suggest that you run a fresh mvn clean install -DskipTests -Dmaven.javadoc.skip=true at the root of the Hadoop source tree to install fresh builds of all sub-components in your local Maven repository. Then, retry the test.

          Show
          cnauroth Chris Nauroth added a comment - Federico Czerwinski , I'm unable to reproduce that test failure. Sometimes this kind of problem happens if a dependent module has changed but not rebuilt. I suggest that you run a fresh mvn clean install -DskipTests -Dmaven.javadoc.skip=true at the root of the Hadoop source tree to install fresh builds of all sub-components in your local Maven repository. Then, retry the test.
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          Thanks Chris, will try that.

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - Thanks Chris, will try that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user fedecz opened a pull request:

          https://github.com/apache/hadoop/pull/113

          HADOOP-13075 Adding support for SSE-KMS and SSE-C

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

          $ git pull https://github.com/fedecz/hadoop HADOOP-13075-2.9

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

          https://github.com/apache/hadoop/pull/113.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 #113


          commit 4cd1c9d0c775be2c0c7732c6f7bbd66c797035eb
          Author: Federico Czerwinski <fede@datarepublic.com.au>
          Date: 2016-06-29T00:03:52Z

          HADOOP-13075 Adding support for SSE-KMS and SSE-C


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user fedecz opened a pull request: https://github.com/apache/hadoop/pull/113 HADOOP-13075 Adding support for SSE-KMS and SSE-C You can merge this pull request into a Git repository by running: $ git pull https://github.com/fedecz/hadoop HADOOP-13075 -2.9 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/hadoop/pull/113.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 #113 commit 4cd1c9d0c775be2c0c7732c6f7bbd66c797035eb Author: Federico Czerwinski <fede@datarepublic.com.au> Date: 2016-06-29T00:03:52Z HADOOP-13075 Adding support for SSE-KMS and SSE-C
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          Chris Nauroth hey Chris, I've submitted both PRs, one for trunk the other for branch-2 (you were right, patches were identical). I'm not sure what goes next. Should I just wait till they get reviewed ? cheers

          Fede

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - Chris Nauroth hey Chris, I've submitted both PRs, one for trunk the other for branch-2 (you were right, patches were identical). I'm not sure what goes next. Should I just wait till they get reviewed ? cheers Fede
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70273746

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java —
          @@ -142,12 +142,8 @@ private Constants() {
          public static final String SERVER_SIDE_ENCRYPTION_ALGORITHM =
          "fs.s3a.server-side-encryption-algorithm";

          • /**
          • * The standard encryption algorithm AWS supports.
          • * Different implementations may support others (or none).
          • */
          • public static final String SERVER_SIDE_ENCRYPTION_AES256 =
              • End diff –

          why was this cut?

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70273746 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java — @@ -142,12 +142,8 @@ private Constants() { public static final String SERVER_SIDE_ENCRYPTION_ALGORITHM = "fs.s3a.server-side-encryption-algorithm"; /** * The standard encryption algorithm AWS supports. * Different implementations may support others (or none). */ public static final String SERVER_SIDE_ENCRYPTION_AES256 = End diff – why was this cut?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70274270

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java —
          @@ -1674,6 +1693,111 @@ public void progressChanged(ProgressEvent progressEvent) {
          }
          }

          + protected void setSSEKMSOrCIfRequired(InitiateMultipartUploadRequest req) {
          + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){
          + if(S3AEncryptionMethods.SSE_KMS.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey))

          { + //Use specified key + req.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams(serverSideEncryptionKey) + ); + }

          else

          { + //Use default key + req.setSSEAwsKeyManagementParams(new SSEAwsKeyManagementParams()); + }

          + }else if(S3AEncryptionMethods.SSE_C.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey))

          { + //at the moment, only supports copy using the same key + req.setSSECustomerKey(new SSECustomerKey(serverSideEncryptionKey)); + }

          + }
          + }
          + }
          +
          +
          + protected void setSSEKMSOrCIfRequired(CopyObjectRequest copyObjectRequest) {
          + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){
          + if(S3AEncryptionMethods.SSE_KMS.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey))

          { + //Use specified key + copyObjectRequest.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams(serverSideEncryptionKey) + ); + }

          else

          { + //Use default key + copyObjectRequest.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams() + ); + }

          + }else if(S3AEncryptionMethods.SSE_C.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey))

          { + //at the moment, only supports copy using the same key + copyObjectRequest.setSourceSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) + ); + copyObjectRequest.setDestinationSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) + ); + }

          + }
          + }
          + }
          +
          + protected void setSSECIfRequired(GetObjectMetadataRequest request) {
          + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){
          + if(S3AEncryptionMethods.SSE_C.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey)) {
          + //at the moment, only supports copy using the same key
          + request.setSSECustomerKey(
          + new SSECustomerKey(serverSideEncryptionKey)
          — End diff –

          this is three chained conditions which could be merged through `&&`

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70274270 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java — @@ -1674,6 +1693,111 @@ public void progressChanged(ProgressEvent progressEvent) { } } + protected void setSSEKMSOrCIfRequired(InitiateMultipartUploadRequest req) { + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){ + if(S3AEncryptionMethods.SSE_KMS.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //Use specified key + req.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams(serverSideEncryptionKey) + ); + } else { + //Use default key + req.setSSEAwsKeyManagementParams(new SSEAwsKeyManagementParams()); + } + }else if(S3AEncryptionMethods.SSE_C.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //at the moment, only supports copy using the same key + req.setSSECustomerKey(new SSECustomerKey(serverSideEncryptionKey)); + } + } + } + } + + + protected void setSSEKMSOrCIfRequired(CopyObjectRequest copyObjectRequest) { + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){ + if(S3AEncryptionMethods.SSE_KMS.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //Use specified key + copyObjectRequest.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams(serverSideEncryptionKey) + ); + } else { + //Use default key + copyObjectRequest.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams() + ); + } + }else if(S3AEncryptionMethods.SSE_C.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //at the moment, only supports copy using the same key + copyObjectRequest.setSourceSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) + ); + copyObjectRequest.setDestinationSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) + ); + } + } + } + } + + protected void setSSECIfRequired(GetObjectMetadataRequest request) { + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){ + if(S3AEncryptionMethods.SSE_C.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //at the moment, only supports copy using the same key + request.setSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) — End diff – this is three chained conditions which could be merged through `&&`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70274678

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java —
          @@ -98,24 +101,33 @@
          */
          private long contentRangeStart;

          • public S3AInputStream(String bucket,
          • String key,
            + public S3AInputStream(S3ObjectAttributes s3Attributes,
            long contentLength,
            AmazonS3Client client,
            FileSystem.Statistics stats,
            S3AInstrumentation instrumentation,
            long readahead,
            S3AInputPolicy inputPolicy) {
          • Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket");
          • Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key");
          • Preconditions.checkArgument(contentLength >= 0 , "Negative content length");
          • this.bucket = bucket;
          • this.key = key;
            + Preconditions.checkNotNull(s3Attributes);
            + Preconditions.checkArgument(
              • End diff –

          could you move the preconditions checks into `S3ObjectAttributes` and invoke them from both output streams? That'd reduce duplicate code and perhaps aid future maintenance

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70274678 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java — @@ -98,24 +101,33 @@ */ private long contentRangeStart; public S3AInputStream(String bucket, String key, + public S3AInputStream(S3ObjectAttributes s3Attributes, long contentLength, AmazonS3Client client, FileSystem.Statistics stats, S3AInstrumentation instrumentation, long readahead, S3AInputPolicy inputPolicy) { Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket"); Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key"); Preconditions.checkArgument(contentLength >= 0 , "Negative content length"); this.bucket = bucket; this.key = key; + Preconditions.checkNotNull(s3Attributes); + Preconditions.checkArgument( End diff – could you move the preconditions checks into `S3ObjectAttributes` and invoke them from both output streams? That'd reduce duplicate code and perhaps aid future maintenance
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70274920

          — Diff: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AEncryption.java —
          @@ -27,23 +27,24 @@

          import java.io.IOException;

          -import static org.apache.hadoop.fs.contract.ContractTestUtils.*;
          -import static org.apache.hadoop.fs.s3a.S3ATestUtils.*;
          +import static org.apache.hadoop.fs.contract.ContractTestUtils.dataset;
          — End diff –

          we're ok with .* on static imports here, so you can just skip this bit of the patch

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70274920 — Diff: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AEncryption.java — @@ -27,23 +27,24 @@ import java.io.IOException; -import static org.apache.hadoop.fs.contract.ContractTestUtils.*; -import static org.apache.hadoop.fs.s3a.S3ATestUtils.*; +import static org.apache.hadoop.fs.contract.ContractTestUtils.dataset; — End diff – we're ok with .* on static imports here, so you can just skip this bit of the patch
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70278452

          — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md —
          @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity".
          To work with AWS better, set the DNS time-to-live of an application which
          works with S3 to something lower. See [AWS documentation](http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html).

          +internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to...
          — End diff –

          looks like an accidental paste in of a bit of HADOOP-13224's doc changes

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70278452 — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md — @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity". To work with AWS better, set the DNS time-to-live of an application which works with S3 to something lower. See [AWS documentation] ( http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html ). + internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to... — End diff – looks like an accidental paste in of a bit of HADOOP-13224 's doc changes
          Hide
          stevel@apache.org Steve Loughran added a comment -

          OK, I've reviewed one of the PRs. Let's go with that one for now, once it's in then the other can be derved.

          1. there's some comments on that PR which will need clarification/addressing. Nothing significant.
          2. As usual: which s3 infrastucture(s) have you tested against?

          Assuming you've been using this, have you any statistics on the performance hit of the different SSE options? In particular, how much slower is a GET-with-range request? We ought to know this and then cover it in the docs

          Show
          stevel@apache.org Steve Loughran added a comment - OK, I've reviewed one of the PRs. Let's go with that one for now, once it's in then the other can be derved. there's some comments on that PR which will need clarification/addressing. Nothing significant. As usual: which s3 infrastucture(s) have you tested against? Assuming you've been using this, have you any statistics on the performance hit of the different SSE options? In particular, how much slower is a GET-with-range request? We ought to know this and then cover it in the docs
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fedecz commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70430407

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java —
          @@ -142,12 +142,8 @@ private Constants() {
          public static final String SERVER_SIDE_ENCRYPTION_ALGORITHM =
          "fs.s3a.server-side-encryption-algorithm";

          • /**
          • * The standard encryption algorithm AWS supports.
          • * Different implementations may support others (or none).
          • */
          • public static final String SERVER_SIDE_ENCRYPTION_AES256 =
              • End diff –

          As you said, that constant is only used in that test which I did change. I changed it to abstract and created 3 different implementations: one for SSE-S3, SSE-KMS and SSE-C. Basically I'm running all the tests in TestS3AEncryption, but with different encryption algorithms depending on the concrete class.
          Yes, it builds and all test pass.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fedecz commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70430407 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java — @@ -142,12 +142,8 @@ private Constants() { public static final String SERVER_SIDE_ENCRYPTION_ALGORITHM = "fs.s3a.server-side-encryption-algorithm"; /** * The standard encryption algorithm AWS supports. * Different implementations may support others (or none). */ public static final String SERVER_SIDE_ENCRYPTION_AES256 = End diff – As you said, that constant is only used in that test which I did change. I changed it to abstract and created 3 different implementations: one for SSE-S3, SSE-KMS and SSE-C. Basically I'm running all the tests in TestS3AEncryption, but with different encryption algorithms depending on the concrete class. Yes, it builds and all test pass.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fedecz commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70431908

          — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md —
          @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity".
          To work with AWS better, set the DNS time-to-live of an application which
          works with S3 to something lower. See [AWS documentation](http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html).

          +internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to...
          — End diff –

          Nope, this is the section Other Issues in the documentation, so I wanted to document if the user was having that warning, he/she should specify the endpoint in the config. I guess I could set a title to describe it better instead of just pasting the warning.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fedecz commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70431908 — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md — @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity". To work with AWS better, set the DNS time-to-live of an application which works with S3 to something lower. See [AWS documentation] ( http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html ). + internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to... — End diff – Nope, this is the section Other Issues in the documentation, so I wanted to document if the user was having that warning, he/she should specify the endpoint in the config. I guess I could set a title to describe it better instead of just pasting the warning.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fedecz commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70432238

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java —
          @@ -1674,6 +1693,111 @@ public void progressChanged(ProgressEvent progressEvent) {
          }
          }

          + protected void setSSEKMSOrCIfRequired(InitiateMultipartUploadRequest req) {
          + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){
          + if(S3AEncryptionMethods.SSE_KMS.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey))

          { + //Use specified key + req.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams(serverSideEncryptionKey) + ); + }

          else

          { + //Use default key + req.setSSEAwsKeyManagementParams(new SSEAwsKeyManagementParams()); + }

          + }else if(S3AEncryptionMethods.SSE_C.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey))

          { + //at the moment, only supports copy using the same key + req.setSSECustomerKey(new SSECustomerKey(serverSideEncryptionKey)); + }

          + }
          + }
          + }
          +
          +
          + protected void setSSEKMSOrCIfRequired(CopyObjectRequest copyObjectRequest) {
          + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){
          + if(S3AEncryptionMethods.SSE_KMS.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey))

          { + //Use specified key + copyObjectRequest.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams(serverSideEncryptionKey) + ); + }

          else

          { + //Use default key + copyObjectRequest.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams() + ); + }

          + }else if(S3AEncryptionMethods.SSE_C.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey))

          { + //at the moment, only supports copy using the same key + copyObjectRequest.setSourceSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) + ); + copyObjectRequest.setDestinationSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) + ); + }

          + }
          + }
          + }
          +
          + protected void setSSECIfRequired(GetObjectMetadataRequest request) {
          + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){
          + if(S3AEncryptionMethods.SSE_C.getMethod()
          + .equals(serverSideEncryptionAlgorithm)) {
          + if (StringUtils.isNotBlank(serverSideEncryptionKey)) {
          + //at the moment, only supports copy using the same key
          + request.setSSECustomerKey(
          + new SSECustomerKey(serverSideEncryptionKey)
          — End diff –

          true, but not all of them can be merged. I'm relying in else clauses as well depending on some of the conditions being false. I'll try to rewrite it though and will see how it looks.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fedecz commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70432238 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java — @@ -1674,6 +1693,111 @@ public void progressChanged(ProgressEvent progressEvent) { } } + protected void setSSEKMSOrCIfRequired(InitiateMultipartUploadRequest req) { + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){ + if(S3AEncryptionMethods.SSE_KMS.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //Use specified key + req.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams(serverSideEncryptionKey) + ); + } else { + //Use default key + req.setSSEAwsKeyManagementParams(new SSEAwsKeyManagementParams()); + } + }else if(S3AEncryptionMethods.SSE_C.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //at the moment, only supports copy using the same key + req.setSSECustomerKey(new SSECustomerKey(serverSideEncryptionKey)); + } + } + } + } + + + protected void setSSEKMSOrCIfRequired(CopyObjectRequest copyObjectRequest) { + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){ + if(S3AEncryptionMethods.SSE_KMS.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //Use specified key + copyObjectRequest.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams(serverSideEncryptionKey) + ); + } else { + //Use default key + copyObjectRequest.setSSEAwsKeyManagementParams( + new SSEAwsKeyManagementParams() + ); + } + }else if(S3AEncryptionMethods.SSE_C.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //at the moment, only supports copy using the same key + copyObjectRequest.setSourceSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) + ); + copyObjectRequest.setDestinationSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) + ); + } + } + } + } + + protected void setSSECIfRequired(GetObjectMetadataRequest request) { + if (StringUtils.isNotBlank(serverSideEncryptionAlgorithm)){ + if(S3AEncryptionMethods.SSE_C.getMethod() + .equals(serverSideEncryptionAlgorithm)) { + if (StringUtils.isNotBlank(serverSideEncryptionKey)) { + //at the moment, only supports copy using the same key + request.setSSECustomerKey( + new SSECustomerKey(serverSideEncryptionKey) — End diff – true, but not all of them can be merged. I'm relying in else clauses as well depending on some of the conditions being false. I'll try to rewrite it though and will see how it looks.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fedecz commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70432364

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java —
          @@ -98,24 +101,33 @@
          */
          private long contentRangeStart;

          • public S3AInputStream(String bucket,
          • String key,
            + public S3AInputStream(S3ObjectAttributes s3Attributes,
            long contentLength,
            AmazonS3Client client,
            FileSystem.Statistics stats,
            S3AInstrumentation instrumentation,
            long readahead,
            S3AInputPolicy inputPolicy) {
          • Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket");
          • Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key");
          • Preconditions.checkArgument(contentLength >= 0 , "Negative content length");
          • this.bucket = bucket;
          • this.key = key;
            + Preconditions.checkNotNull(s3Attributes);
            + Preconditions.checkArgument(
              • End diff –

          will do

          Show
          githubbot ASF GitHub Bot added a comment - Github user fedecz commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70432364 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java — @@ -98,24 +101,33 @@ */ private long contentRangeStart; public S3AInputStream(String bucket, String key, + public S3AInputStream(S3ObjectAttributes s3Attributes, long contentLength, AmazonS3Client client, FileSystem.Statistics stats, S3AInstrumentation instrumentation, long readahead, S3AInputPolicy inputPolicy) { Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket"); Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key"); Preconditions.checkArgument(contentLength >= 0 , "Negative content length"); this.bucket = bucket; this.key = key; + Preconditions.checkNotNull(s3Attributes); + Preconditions.checkArgument( End diff – will do
          Hide
          fedecz@gmail.com Federico Czerwinski added a comment -

          thanks Steve for taking the time to review it. I've replied your comments in the PR, hopefully will clarify some things.
          I'll work on the comments and update the PR.

          I've tested against ap-southeast-2, Sydney.

          I haven't been using this patch in particular yet. I've used one based in hadoop 2.7 in a spark cluster but that patch doesn't have support for SSE-C.
          I don't have any performance statistics I'm afraid. What is that GET-with-range request that you mention? I don't remember seeing that in the code.

          Show
          fedecz@gmail.com Federico Czerwinski added a comment - thanks Steve for taking the time to review it. I've replied your comments in the PR, hopefully will clarify some things. I'll work on the comments and update the PR. I've tested against ap-southeast-2, Sydney. I haven't been using this patch in particular yet. I've used one based in hadoop 2.7 in a spark cluster but that patch doesn't have support for SSE-C. I don't have any performance statistics I'm afraid. What is that GET-with-range request that you mention? I don't remember seeing that in the code.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70433629

          — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md —
          @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity".
          To work with AWS better, set the DNS time-to-live of an application which
          works with S3 to something lower. See [AWS documentation](http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html).

          +internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to...
          — End diff –

          Well it's being covered in HADOOP-13224, so it's best to pull it here and review that patch instead

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70433629 — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md — @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity". To work with AWS better, set the DNS time-to-live of an application which works with S3 to something lower. See [AWS documentation] ( http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html ). + internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to... — End diff – Well it's being covered in HADOOP-13224 , so it's best to pull it here and review that patch instead
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fedecz commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70439085

          — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md —
          @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity".
          To work with AWS better, set the DNS time-to-live of an application which
          works with S3 to something lower. See [AWS documentation](http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html).

          +internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to...
          — End diff –

          I don't see anything related to this patch in that ticket's patch, are you sure that's the one? I'm looking at the attached patch in HADOOP-13224

          Show
          githubbot ASF GitHub Bot added a comment - Github user fedecz commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70439085 — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md — @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity". To work with AWS better, set the DNS time-to-live of an application which works with S3 to something lower. See [AWS documentation] ( http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html ). + internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to... — End diff – I don't see anything related to this patch in that ticket's patch, are you sure that's the one? I'm looking at the attached patch in HADOOP-13224
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/113#discussion_r70463002

          — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md —
          @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity".
          To work with AWS better, set the DNS time-to-live of an application which
          works with S3 to something lower. See [AWS documentation](http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html).

          +internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to...
          — End diff –

          sorry, wrong JIRA. https://issues.apache.org/jira/browse/HADOOP-13324

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/113#discussion_r70463002 — Diff: hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md — @@ -940,6 +950,10 @@ the DNS TTL of a JVM is "infinity". To work with AWS better, set the DNS time-to-live of an application which works with S3 to something lower. See [AWS documentation] ( http://docs.aws.amazon.com/AWSSdkDocsJava/latest/DeveloperGuide/java-dg-jvm-ttl.html ). + internal.S3V4AuthErrorRetryStrategy (S3V4AuthErrorRetryStrategy.java:buildRetryParams(117)) - Attempting to re-send the request to... — End diff – sorry, wrong JIRA. https://issues.apache.org/jira/browse/HADOOP-13324
          Hide
          stevel@apache.org Steve Loughran added a comment -

          the get with range is a reference to how, after a seek, we do a GET with a range of pos-EOF (default), or, with fadvise set to random, pos+block size.

          I'm really curious what happens here: if we seek to an offset and then read, how long do those reads take? I think it will depend on how the encryption is done: if it is done from byte 0 in some streaming cipher, then you would have to go through the whole file to decrypt later bytes. If the blob is encrypted in separate blocks, then you'd have to go back a block...something we could potentially work out.

          Overall, I like the patch, it's pretty close to being ready to add

          Show
          stevel@apache.org Steve Loughran added a comment - the get with range is a reference to how, after a seek, we do a GET with a range of pos-EOF (default), or, with fadvise set to random, pos+block size. I'm really curious what happens here: if we seek to an offset and then read, how long do those reads take? I think it will depend on how the encryption is done: if it is done from byte 0 in some streaming cipher, then you would have to go through the whole file to decrypt later bytes. If the blob is encrypted in separate blocks, then you'd have to go back a block...something we could potentially work out. Overall, I like the patch, it's pretty close to being ready to add
          Hide
          spacepluk Oscar Morante added a comment -

          How would you configure it with no encryption?

          Show
          spacepluk Oscar Morante added a comment - How would you configure it with no encryption?
          Hide
          noslowerdna Andrew Olson added a comment -

          Oscar Morante Not encrypting is the default behavior, so no configuration needs to be set in that case.

          Show
          noslowerdna Andrew Olson added a comment - Oscar Morante Not encrypting is the default behavior, so no configuration needs to be set in that case.
          Hide
          spacepluk Oscar Morante added a comment -

          Is there a value that I can use to specify the default behavior? I have a script that populates the configuration from env variables (to use in a docker container) and that would be helpful.

          Show
          spacepluk Oscar Morante added a comment - Is there a value that I can use to specify the default behavior? I have a script that populates the configuration from env variables (to use in a docker container) and that would be helpful.
          Hide
          noslowerdna Andrew Olson added a comment -

          Oscar Morante For specifying the default behavior it would be best to just leave fs.s3a.server-side-encryption-algorithm unset – if for scripted consistency purposes you have concluded that you must set it, I believe an empty string value would work.

          Show
          noslowerdna Andrew Olson added a comment - Oscar Morante For specifying the default behavior it would be best to just leave fs.s3a.server-side-encryption-algorithm unset – if for scripted consistency purposes you have concluded that you must set it, I believe an empty string value would work.
          Hide
          spacepluk Oscar Morante added a comment -

          Thanks, I'll try that.

          Show
          spacepluk Oscar Morante added a comment - Thanks, I'll try that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user elad commented on the issue:

          https://github.com/apache/hadoop/pull/113

          Hello, what's the status of this patch? Would be really great if we could use S3 SSE-C with Hadoop on EMR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user elad commented on the issue: https://github.com/apache/hadoop/pull/113 Hello, what's the status of this patch? Would be really great if we could use S3 SSE-C with Hadoop on EMR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on the issue:

          https://github.com/apache/hadoop/pull/113

          @elad best to discuss that on the JIRA itself; FWIW, EMR's S3 connector is Amazon's codebase, so what we do in the ASF doesn't have any impact there. We are striving to be better

          Status: we've gone and broken it in Hadoop branch-2 by replacing S3AFastOutputStream with S3ABlockOutputStream;

          @fedecz —could you bring this up to sync with branch-2 and I'll add reviewing it to my todo list. thanks

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on the issue: https://github.com/apache/hadoop/pull/113 @elad best to discuss that on the JIRA itself; FWIW, EMR's S3 connector is Amazon's codebase, so what we do in the ASF doesn't have any impact there. We are striving to be better Status: we've gone and broken it in Hadoop branch-2 by replacing S3AFastOutputStream with S3ABlockOutputStream ; @fedecz —could you bring this up to sync with branch-2 and I'll add reviewing it to my todo list. thanks
          Hide
          fabbri Aaron Fabbri added a comment -

          I'm interested in getting this stuff in. We can probably create a new patch based on latest branch-2 code, and do some testing, if that helps.

          Show
          fabbri Aaron Fabbri added a comment - I'm interested in getting this stuff in. We can probably create a new patch based on latest branch-2 code, and do some testing, if that helps.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user orngejaket opened a pull request:

          https://github.com/apache/hadoop/pull/183

          HADOOP-13075 adding sse-c and sse-kms

          Updating fedecz changes to work with changes in trunk. Refactored methods to be clearer as to purpose and added more integration tests.

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

          $ git pull https://github.com/orngejaket/hadoop HADOOP-13075-trunk

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

          https://github.com/apache/hadoop/pull/183.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 #183


          commit eba581f46ab779505862755e263f4c5b68adf0a1
          Author: Stephen Moist <moist@cloudera.com>
          Date: 2017-01-25T16:40:12Z

          HADOOP-13075 importing existing changes to support SSEKMS and SSEC. Adding integration tests, documentation and required constants.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user orngejaket opened a pull request: https://github.com/apache/hadoop/pull/183 HADOOP-13075 adding sse-c and sse-kms Updating fedecz changes to work with changes in trunk. Refactored methods to be clearer as to purpose and added more integration tests. You can merge this pull request into a Git repository by running: $ git pull https://github.com/orngejaket/hadoop HADOOP-13075 -trunk Alternatively you can review and apply these changes as the patch at: https://github.com/apache/hadoop/pull/183.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 #183 commit eba581f46ab779505862755e263f4c5b68adf0a1 Author: Stephen Moist <moist@cloudera.com> Date: 2017-01-25T16:40:12Z HADOOP-13075 importing existing changes to support SSEKMS and SSEC. Adding integration tests, documentation and required constants.
          Hide
          moist Steve Moist added a comment -

          Hi, I've taken Federico's changes and updated them to be consistent with trunk's changes and submitted a pull request to merge into trunk. I will be working on a separate one for branch-2

          Show
          moist Steve Moist added a comment - Hi, I've taken Federico's changes and updated them to be consistent with trunk's changes and submitted a pull request to merge into trunk. I will be working on a separate one for branch-2
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Steve, thanks for taking this up; we'll give you and Frederico joint credit once this is in.

          1. Start with trunk only not branch-2. Or work in branch-2 only and plan to patch forwards...trying to keep 2 branches in sync only adds complications. This month s3guard branch is the active one, so keep an eye out for any changes there that would break things —this patch, being simpler, is likelier to get in first.
          1. We do have a strict "declare the s3 endpoint you tested against" policy for all the object stores; that's something you'll be expected to declare on every patch. Do mention if you saw a transient failure here, always good to keep an eye on those too. And do run {{ -Dparallel-tests -DtestsThreadCount=8}} for execution speed.

          Overall design seems OK, though it'll need some revisions, In particular, I don't like all the f(if(S3AEncryptionMethods.SSE_KMS.getMethod().equals(serverSideEncryptionAlgorithm)) checks everywhere
          S3AEncryptionMethods is an enum; add one for "None" and then you can do case statements around them; conversion of the config option to an enum value is a method which can be tucked into the enum itself.

          Blocker: The server-side encryption key must be supported; use S3AUtils.getPassword for this.

          S3AFileSystem

          L 912 : What if the user has selected SSE_C and the server key is blank? That's downgraded to a no-op, now, yes? I'd rather that was treated
          as a failure: if you ask for server-side encryption and don't supply a key, FS init must fail.

          S3AEncryptionMethods

          L 104-119. seems like a spurious IDE-initiated reformat. not needed, as it only complicates merging branches.

          Show
          stevel@apache.org Steve Loughran added a comment - Steve, thanks for taking this up; we'll give you and Frederico joint credit once this is in. Start with trunk only not branch-2. Or work in branch-2 only and plan to patch forwards...trying to keep 2 branches in sync only adds complications. This month s3guard branch is the active one, so keep an eye out for any changes there that would break things —this patch, being simpler, is likelier to get in first. We do have a strict "declare the s3 endpoint you tested against" policy for all the object stores; that's something you'll be expected to declare on every patch. Do mention if you saw a transient failure here, always good to keep an eye on those too. And do run {{ -Dparallel-tests -DtestsThreadCount=8}} for execution speed. Overall design seems OK, though it'll need some revisions, In particular, I don't like all the f(if(S3AEncryptionMethods.SSE_KMS.getMethod().equals(serverSideEncryptionAlgorithm)) checks everywhere S3AEncryptionMethods is an enum; add one for "None" and then you can do case statements around them; conversion of the config option to an enum value is a method which can be tucked into the enum itself. Blocker: The server-side encryption key must be supported; use S3AUtils.getPassword for this. S3AFileSystem L 912 : What if the user has selected SSE_C and the server key is blank? That's downgraded to a no-op, now, yes? I'd rather that was treated as a failure: if you ask for server-side encryption and don't supply a key, FS init must fail. S3AEncryptionMethods L 104-119. seems like a spurious IDE-initiated reformat. not needed, as it only complicates merging branches.
          Hide
          moist Steve Moist added a comment -

          Sounds good, I'll make changes around the Enum and convert the if statements to a switch.

          > L 912 : What if the user has selected SSE_C and the server key is blank? That's downgraded to a no-op, now, yes? I'd rather that was treated
          as a failure: if you ask for server-side encryption and don't supply a key, FS init must fail.

          I'll move that check into initialize. Is there a preference on what type of exception to be thrown?

          >Blocker: The server-side encryption key must be supported; use S3AUtils.getPassword for this.

          By this do you mean convert instances of
          request.setSSECustomerKey(new SSECustomerKey(serverSideEncryptionKey));
          with
          request.setSSECustomerKey(new SSECustomerKey(S3AUtils.getPassword(getConf(), Constants.SERVER_SIDE_ENCRYPTION_KEY));

          Show
          moist Steve Moist added a comment - Sounds good, I'll make changes around the Enum and convert the if statements to a switch. > L 912 : What if the user has selected SSE_C and the server key is blank? That's downgraded to a no-op, now, yes? I'd rather that was treated as a failure: if you ask for server-side encryption and don't supply a key, FS init must fail. I'll move that check into initialize. Is there a preference on what type of exception to be thrown? >Blocker: The server-side encryption key must be supported; use S3AUtils.getPassword for this. By this do you mean convert instances of request.setSSECustomerKey(new SSECustomerKey(serverSideEncryptionKey)); with request.setSSECustomerKey(new SSECustomerKey(S3AUtils.getPassword(getConf(), Constants.SERVER_SIDE_ENCRYPTION_KEY));
          Hide
          fabbri Aaron Fabbri added a comment -

          Thanks for doing this Steve Moist. One quick question, would it be simpler to not add the new enum + deprecation, and instead just add new constants here?

          +   * Use the S3AEncryptionMethods instead when configuring
           +   * which Server Side Encryption to use.
               */
           +  @Deprecated
              public static final String SERVER_SIDE_ENCRYPTION_AES256 =
                  "AES256";
          
          Show
          fabbri Aaron Fabbri added a comment - Thanks for doing this Steve Moist . One quick question, would it be simpler to not add the new enum + deprecation, and instead just add new constants here? + * Use the S3AEncryptionMethods instead when configuring + * which Server Side Encryption to use. */ + @Deprecated public static final String SERVER_SIDE_ENCRYPTION_AES256 = "AES256" ;
          Hide
          moist Steve Moist added a comment -

          Aaron Fabbri One of Frederico's original changes was removing that. It was commented on saying not to do so (in branch-2). It would be a simpler change but I feel overloading that wouldn't be as clear. I'm not sure what customers are using that field, the only use of it is in ITestS3AEncryption. Since this is 3.0 of Hadoop, is it possible to remove old fields? I'm more supportie of removing that field.

          Show
          moist Steve Moist added a comment - Aaron Fabbri One of Frederico's original changes was removing that. It was commented on saying not to do so (in branch-2). It would be a simpler change but I feel overloading that wouldn't be as clear. I'm not sure what customers are using that field, the only use of it is in ITestS3AEncryption. Since this is 3.0 of Hadoop, is it possible to remove old fields? I'm more supportie of removing that field.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user orngejaket commented on the issue:

          https://github.com/apache/hadoop/pull/183

          Accidently included others commits due to trying to resolve unpushable commit. Update to the pull request is in e7303f9eff316d3d4329de9011cfd4348ac97a23. Will correct this tomorrow and resubmit cleaner patch.

          Show
          githubbot ASF GitHub Bot added a comment - Github user orngejaket commented on the issue: https://github.com/apache/hadoop/pull/183 Accidently included others commits due to trying to resolve unpushable commit. Update to the pull request is in e7303f9eff316d3d4329de9011cfd4348ac97a23. Will correct this tomorrow and resubmit cleaner patch.
          Hide
          moist Steve Moist added a comment -

          Updated pull request with Steve Loughran's comments.

          Show
          moist Steve Moist added a comment - Updated pull request with Steve Loughran 's comments.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Regarding SERVER_SIDE_ENCRYPTION_ALGORITHM

          That is an attribute in a public/evolving set of constants and must considered immutable on the basis that "people may be using it in their own code". That's what the public constants are, be it YarnConfiguration, HdfsConfiguration or somewhere else. They are constants to use when programatically setting/getting constants, so allowing you to avoid typos in your own use, and allow IDE support (completion, find-usages-of).

          While a major release gives you the freedom to remove things, it doesn't give you the obligation. If you look at HDFS-6418 you can see my unbounded happiness at something being cut from the HDFS options, with HDFS-9301 being the followon. I don't want the S3A config options to be the new HDFS config options.

          . It's only a string, not some fundamental API feature; a constant string declaring what configuration option can be used.

          Show
          stevel@apache.org Steve Loughran added a comment - Regarding SERVER_SIDE_ENCRYPTION_ALGORITHM That is an attribute in a public/evolving set of constants and must considered immutable on the basis that "people may be using it in their own code". That's what the public constants are, be it YarnConfiguration , HdfsConfiguration or somewhere else. They are constants to use when programatically setting/getting constants, so allowing you to avoid typos in your own use, and allow IDE support (completion, find-usages-of). While a major release gives you the freedom to remove things, it doesn't give you the obligation. If you look at HDFS-6418 you can see my unbounded happiness at something being cut from the HDFS options, with HDFS-9301 being the followon. I don't want the S3A config options to be the new HDFS config options. . It's only a string, not some fundamental API feature; a constant string declaring what configuration option can be used.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          re

          By this do you mean convert instances of request.setSSECustomerKey(new SSECustomerKey(serverSideEncryptionKey)); with

          request.setSSECustomerKey(new SSECustomerKey(S3AUtils.getPassword(getConf(), Constants.SERVER_SIDE_ENCRYPTION_KEY));

          or just

          serverSideEncryptionKey = S3AUtils.getPassword(getConf(), Constants.SERVER_SIDE_ENCRYPTION_KEY
          

          The key thing is that getPassword() will first attempt to look in any jceks files for the secrets, falling back to inline config. those jceks files let you keep the secrets out the XML documents and share round a cluster (somehow). It's an extra level of defence against key leakage, which, given these are encryption keys, matters. cc Larry McCay

          Show
          stevel@apache.org Steve Loughran added a comment - re By this do you mean convert instances of request.setSSECustomerKey(new SSECustomerKey(serverSideEncryptionKey)); with request.setSSECustomerKey(new SSECustomerKey(S3AUtils.getPassword(getConf(), Constants.SERVER_SIDE_ENCRYPTION_KEY)); or just serverSideEncryptionKey = S3AUtils.getPassword(getConf(), Constants.SERVER_SIDE_ENCRYPTION_KEY The key thing is that getPassword() will first attempt to look in any jceks files for the secrets, falling back to inline config. those jceks files let you keep the secrets out the XML documents and share round a cluster (somehow). It's an extra level of defence against key leakage, which, given these are encryption keys, matters. cc Larry McCay
          Hide
          lmccay Larry McCay added a comment -

          At first glance, this method makes me a bit uneasy:

          +   * Depending on which SSE encryption method is used, SSE-C uses the actual
           +   * key material to encrypt data.  This shouldn't be displayed.  If SSE-KMS is
           +   * enabled then it is a reference to the key id in AWS and is safe to display.
           +   * SSE-S3 is abstracted away and serverSideEncryptionKey would not be
           +   * populated.
           +   *
           +   * @return masked encryption key value, or the SSE KMS key id, or empty if
           +   * there is no encryption key
           +   */
           +  private String displayEncryptionKeyAs() {
           +    if(S3AEncryptionMethods.SSE_C.equals(serverSideEncryptionAlgorithm)) {
           +      return "************";
           +    } else if(S3AEncryptionMethods.SSE_KMS
           +        .equals(serverSideEncryptionAlgorithm)) {
           +      return S3AUtils.getServerSideEncryptionKey(getConf());
           +    }
           +    return "";
           +  }
          

          It seems to be depending on a conf setting that could potentially be changed even if only temporarily to get to the actual key material.
          This sort of thing is generally better done in an object oriented manner.

          I will try and dig into the S3AUtils.getPassword approach as well.
          We may want to consider adding a getKey instead which would be based on the Key Provider API rather than Credential Provider API and allow for versioning of the keys and the use of the KMS server.
          Maybe those things aren't important in the S3 usecase?

          Show
          lmccay Larry McCay added a comment - At first glance, this method makes me a bit uneasy: + * Depending on which SSE encryption method is used, SSE-C uses the actual + * key material to encrypt data. This shouldn't be displayed. If SSE-KMS is + * enabled then it is a reference to the key id in AWS and is safe to display. + * SSE-S3 is abstracted away and serverSideEncryptionKey would not be + * populated. + * + * @return masked encryption key value, or the SSE KMS key id, or empty if + * there is no encryption key + */ + private String displayEncryptionKeyAs() { + if(S3AEncryptionMethods.SSE_C.equals(serverSideEncryptionAlgorithm)) { + return "************"; + } else if(S3AEncryptionMethods.SSE_KMS + .equals(serverSideEncryptionAlgorithm)) { + return S3AUtils.getServerSideEncryptionKey(getConf()); + } + return ""; + } It seems to be depending on a conf setting that could potentially be changed even if only temporarily to get to the actual key material. This sort of thing is generally better done in an object oriented manner. I will try and dig into the S3AUtils.getPassword approach as well. We may want to consider adding a getKey instead which would be based on the Key Provider API rather than Credential Provider API and allow for versioning of the keys and the use of the KMS server. Maybe those things aren't important in the S3 usecase?
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Larry, the S3AUtils.getPassword uses the credential provider API, it just converts the binary data to a string and trims whitespace

          Show
          stevel@apache.org Steve Loughran added a comment - Larry, the S3AUtils.getPassword uses the credential provider API, it just converts the binary data to a string and trims whitespace
          Hide
          lmccay Larry McCay added a comment -

          Yeah, I get that. Credential provider API lacks some key management capabilities that the Key provider API provides though.
          If the S3 usecase doesn't require or accommodate key versioning, etc then getPassword is probably sufficient.

          Show
          lmccay Larry McCay added a comment - Yeah, I get that. Credential provider API lacks some key management capabilities that the Key provider API provides though. If the S3 usecase doesn't require or accommodate key versioning, etc then getPassword is probably sufficient.
          Hide
          moist Steve Moist added a comment -

          Steve Loughran So you're saying don't change the name of the constant but changing the contents of the constant is ok. In this case is that something that we want to do?

          Larry McCay You make a good point. You can switch the mode from SSE_C to SSS_KMS to print out the contents of the key. This is mainly just for the toString method and can be removed. But if the user has the ability to switch modes, wouldn't they also have access to config with the key?

          I also don't believe the use case needs key versioning. If that were to be supported that would have much larger implications than this.

          Show
          moist Steve Moist added a comment - Steve Loughran So you're saying don't change the name of the constant but changing the contents of the constant is ok. In this case is that something that we want to do? Larry McCay You make a good point. You can switch the mode from SSE_C to SSS_KMS to print out the contents of the key. This is mainly just for the toString method and can be removed. But if the user has the ability to switch modes, wouldn't they also have access to config with the key? I also don't believe the use case needs key versioning. If that were to be supported that would have much larger implications than this.
          Hide
          noslowerdna Andrew Olson added a comment -

          Steve Moist I think you're misunderstanding. We absolutely shouldn't change the value of that constant in any case (we could introduce a second alternative value, but the original value must still always be honored). The name changing is also very undesirable, but that would only be a inconvenience for breaking builds and requiring a code change to successfully compile again. However the value changing would quietly break existing deployments where the configuration is not handled programmatically using the constant, but rather loaded from some external resource such as a conf.xml file.

          Show
          noslowerdna Andrew Olson added a comment - Steve Moist I think you're misunderstanding. We absolutely shouldn't change the value of that constant in any case (we could introduce a second alternative value, but the original value must still always be honored). The name changing is also very undesirable, but that would only be a inconvenience for breaking builds and requiring a code change to successfully compile again. However the value changing would quietly break existing deployments where the configuration is not handled programmatically using the constant, but rather loaded from some external resource such as a conf.xml file.
          Hide
          moist Steve Moist added a comment -

          Ok, that's what I originally thought. I will leave the code as is by introducing the new SERVER_SIDE_ENCRYPTION_KEY.

          Show
          moist Steve Moist added a comment - Ok, that's what I originally thought. I will leave the code as is by introducing the new SERVER_SIDE_ENCRYPTION_KEY.
          Hide
          moist Steve Moist added a comment -

          Updated with changes. One issue I found with refactoring the encryptionMethodAlgorthm string into an enum is that it breaks the test ITestS3AEncryptionAlgorithmPropagation. In this test, it sets the algorithm to "DES" and when it places a S3 request, S3 then validates that and throws an exception. However this is no longer the case, in S3aFileSystem.initialize, an exception is thrown here when trying to convert that string into an enum and the tests fail during setup. Should we keep this test or remove it as this behavior isn't expected anymore?

          In general I have found that I cannot run most tests in parallel. As an example, one class expects an empty dir, but the test suites spawn subfolders in the root of the bucket for their tests to be isolated.

          Show
          moist Steve Moist added a comment - Updated with changes. One issue I found with refactoring the encryptionMethodAlgorthm string into an enum is that it breaks the test ITestS3AEncryptionAlgorithmPropagation. In this test, it sets the algorithm to "DES" and when it places a S3 request, S3 then validates that and throws an exception. However this is no longer the case, in S3aFileSystem.initialize, an exception is thrown here when trying to convert that string into an enum and the tests fail during setup. Should we keep this test or remove it as this behavior isn't expected anymore? In general I have found that I cannot run most tests in parallel. As an example, one class expects an empty dir, but the test suites spawn subfolders in the root of the bucket for their tests to be isolated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Don't worry about breaking our tests, those we can adapt if we are confident the new behaviour is OK. That test was designed to implicitly verify that the encryption worked by setting an invalid one. Otherwise, we had no evidence whatsoever that the property was being passed down.

          You can just tweak that test to catch the error in initialization. That way we can verify that invalid options are being rejected.

          This does raise the question of how best to test the encryption work

          1. we should be able to verify that data written with one key cannot be parsed if a different fs + key is used to read it.
          2. we should see what happens if you try to read unencrypted data with an FS with encryption enabled
          3. maybe: if a bucket is set up to require encryption, then unencrypted data cannot be written, encrypted can. This implies that the tester will need a special bucket for this test & declare it in the configs.

          Something to try (now?) is for you to enable encryption in auth-keys.xml and rerun the full test suite with encryption enabled. We've had hints in the past you may end up with a different length of data returned in getFileStatus() than you get when you read in the data, and suspect that it's related to encryption —though probably client-side, rather than server side. running the entire aws test suite with encryption turned on helps check these things.

          Show
          stevel@apache.org Steve Loughran added a comment - Don't worry about breaking our tests, those we can adapt if we are confident the new behaviour is OK. That test was designed to implicitly verify that the encryption worked by setting an invalid one. Otherwise, we had no evidence whatsoever that the property was being passed down. You can just tweak that test to catch the error in initialization. That way we can verify that invalid options are being rejected. This does raise the question of how best to test the encryption work we should be able to verify that data written with one key cannot be parsed if a different fs + key is used to read it. we should see what happens if you try to read unencrypted data with an FS with encryption enabled maybe: if a bucket is set up to require encryption, then unencrypted data cannot be written, encrypted can. This implies that the tester will need a special bucket for this test & declare it in the configs. Something to try (now?) is for you to enable encryption in auth-keys.xml and rerun the full test suite with encryption enabled. We've had hints in the past you may end up with a different length of data returned in getFileStatus() than you get when you read in the data, and suspect that it's related to encryption —though probably client-side, rather than server side. running the entire aws test suite with encryption turned on helps check these things.
          Hide
          moist Steve Moist added a comment -

          I ran some tests using the minicluster against the default s3 endpoint where buckets were configured in Oregon and encryption keys stored in Oregon. One thing to note using SSE-C, there is nothing in the AWS S3 GUI that shows that the file is encrypted.

          > we should be able to verify that data written with one key cannot be parsed if a different fs + key is used to read it.
          I uploaded FileA using the minicluster under aws:kms key1, then did a fs -mv after restarting the minicluster with aws:kms key2. It successfully moved it to FileA' encrypted with aws:kms key2. If SSE-C is enabled, it will throw an error.

          > we should see what happens if you try to read unencrypted data with an FS with encryption enabled
          I uploaded FileA through the AWS S3 GUI. I moved a file from fileA to fileA' where fileA was unencrypted with SSE-KMS enabled. FileA' became encrypted under the aws kms key configured. If SSE-C is enabled, it will throw an error.

          > maybe: if a bucket is set up to require encryption, then unencrypted data cannot be written, encrypted can. This implies that the tester will need a special bucket for this test & declare it in the configs.
          The user can still upload data through the GUI or AWS cli and not have it be encrypted. Based on #2, any new file will be encrypted with SSE-S3/SSE-KMS and any copied/moved file will now be encrypted. This point seems like it will be hard to enforce.

          I ran the full s3a suite with aws:kms enabled and it ran fine on the defaults s3 enpdoint in the oregon region.

          Show
          moist Steve Moist added a comment - I ran some tests using the minicluster against the default s3 endpoint where buckets were configured in Oregon and encryption keys stored in Oregon. One thing to note using SSE-C, there is nothing in the AWS S3 GUI that shows that the file is encrypted. > we should be able to verify that data written with one key cannot be parsed if a different fs + key is used to read it. I uploaded FileA using the minicluster under aws:kms key1, then did a fs -mv after restarting the minicluster with aws:kms key2. It successfully moved it to FileA' encrypted with aws:kms key2. If SSE-C is enabled, it will throw an error. > we should see what happens if you try to read unencrypted data with an FS with encryption enabled I uploaded FileA through the AWS S3 GUI. I moved a file from fileA to fileA' where fileA was unencrypted with SSE-KMS enabled. FileA' became encrypted under the aws kms key configured. If SSE-C is enabled, it will throw an error. > maybe: if a bucket is set up to require encryption, then unencrypted data cannot be written, encrypted can. This implies that the tester will need a special bucket for this test & declare it in the configs. The user can still upload data through the GUI or AWS cli and not have it be encrypted. Based on #2, any new file will be encrypted with SSE-S3/SSE-KMS and any copied/moved file will now be encrypted. This point seems like it will be hard to enforce. I ran the full s3a suite with aws:kms enabled and it ran fine on the defaults s3 enpdoint in the oregon region.
          Hide
          moist Steve Moist added a comment -

          Hey all, I corrected the broken unit test to now validate incorrect configurations and to make sure an encryption key is provided with SSE-C. I've also updated the index.md to include info on how to configure the aws kms encryption key that is used in tests.

          Is this all good to merge in at this point?

          Show
          moist Steve Moist added a comment - Hey all, I corrected the broken unit test to now validate incorrect configurations and to make sure an encryption key is provided with SSE-C. I've also updated the index.md to include info on how to configure the aws kms encryption key that is used in tests. Is this all good to merge in at this point?
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I'd actually like to see the IT tests tests for key/no key FS access, which is possible for SSE-C, I believe

          It's straightforward to create a new S3a FS instance with/without encryption; use S3ATestUtils to first disable caching, with disableFilesystemCaching(conf), then createTestFileSystem to create the FS. Configure one without encryption, one with, and you can try the read-write sequence in both directions.

          The more complex tests, kms and dedicated buckets, I don't see those being as testable, not unless we actually start creating buckets and keys as part of the test run. That's not something we've gone near (yet).

          Show
          stevel@apache.org Steve Loughran added a comment - I'd actually like to see the IT tests tests for key/no key FS access, which is possible for SSE-C, I believe It's straightforward to create a new S3a FS instance with/without encryption; use S3ATestUtils to first disable caching, with disableFilesystemCaching(conf) , then createTestFileSystem to create the FS. Configure one without encryption, one with, and you can try the read-write sequence in both directions. The more complex tests, kms and dedicated buckets, I don't see those being as testable, not unless we actually start creating buckets and keys as part of the test run. That's not something we've gone near (yet).
          Hide
          stevel@apache.org Steve Loughran added a comment -

          assigning to me to submit patch; will assign back to Federico Czerwinski once it's in, credit Frederico & Steve in the commit message

          Show
          stevel@apache.org Steve Loughran added a comment - assigning to me to submit patch; will assign back to Federico Czerwinski once it's in, credit Frederico & Steve in the commit message
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r98646037

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java —
          @@ -227,8 +230,13 @@ public StorageStatistics provide() {

          initMultipartUploads(conf);

          • serverSideEncryptionAlgorithm =
          • conf.getTrimmed(SERVER_SIDE_ENCRYPTION_ALGORITHM);
            + serverSideEncryptionAlgorithm = S3AEncryptionMethods.getMethod(
            + conf.getTrimmed(SERVER_SIDE_ENCRYPTION_ALGORITHM));
            + if(S3AEncryptionMethods.SSE_C.equals(serverSideEncryptionAlgorithm) &&
            + StringUtils.isBlank(S3AUtils.getServerSideEncryptionKey(getConf()))) {
            + throw new IOException("SSE-C is enable and no encryption key " +
              • End diff –

          "is enabled"

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r98646037 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java — @@ -227,8 +230,13 @@ public StorageStatistics provide() { initMultipartUploads(conf); serverSideEncryptionAlgorithm = conf.getTrimmed(SERVER_SIDE_ENCRYPTION_ALGORITHM); + serverSideEncryptionAlgorithm = S3AEncryptionMethods.getMethod( + conf.getTrimmed(SERVER_SIDE_ENCRYPTION_ALGORITHM)); + if(S3AEncryptionMethods.SSE_C.equals(serverSideEncryptionAlgorithm) && + StringUtils.isBlank(S3AUtils.getServerSideEncryptionKey(getConf()))) { + throw new IOException("SSE-C is enable and no encryption key " + End diff – "is enabled"
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r98646282

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java —
          @@ -1788,6 +1811,72 @@ public void progressChanged(ProgressEvent progressEvent) {
          }
          }

          + protected void setOptionalMultipartUploadRequestParameters(
          + InitiateMultipartUploadRequest req) {
          + if(S3AEncryptionMethods.SSE_KMS.equals(serverSideEncryptionAlgorithm)) {
          — End diff –

          I'd like to see case: statements here; it may help readability

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r98646282 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java — @@ -1788,6 +1811,72 @@ public void progressChanged(ProgressEvent progressEvent) { } } + protected void setOptionalMultipartUploadRequestParameters( + InitiateMultipartUploadRequest req) { + if(S3AEncryptionMethods.SSE_KMS.equals(serverSideEncryptionAlgorithm)) { — End diff – I'd like to see case: statements here; it may help readability
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r98646348

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java —
          @@ -1953,6 +2042,9 @@ public String toString()

          { .append(serverSideEncryptionAlgorithm) .append('\''); }

          + if (S3AUtils.getServerSideEncryptionKey(getConf()) != null) {
          — End diff –

          just cut this

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r98646348 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java — @@ -1953,6 +2042,9 @@ public String toString() { .append(serverSideEncryptionAlgorithm) .append('\''); } + if (S3AUtils.getServerSideEncryptionKey(getConf()) != null) { — End diff – just cut this
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r98646557

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java —
          @@ -98,24 +101,30 @@
          */
          private long contentRangeStart;

          • public S3AInputStream(String bucket,
          • String key,
          • long contentLength,
          • AmazonS3 client,
          • FileSystem.Statistics stats,
          • S3AInstrumentation instrumentation,
          • long readahead,
          • S3AInputPolicy inputPolicy) {
          • Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket");
          • Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key");
          • Preconditions.checkArgument(contentLength >= 0 , "Negative content length");
          • this.bucket = bucket;
          • this.key = key;
            + public S3AInputStream(S3ObjectAttributes s3Attributes,
            + long contentLength,
            + AmazonS3 client,
            + FileSystem.Statistics stats,
            + S3AInstrumentation instrumentation,
            + long readahead,
            + S3AInputPolicy inputPolicy) {
            + Preconditions.checkArgument(
              • End diff –

          I'd rather these lines reverted to the previous ones, even if they are > 80 chars long. That is a soft limit on style checks: we break it if it keeps the code looking nice

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r98646557 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java — @@ -98,24 +101,30 @@ */ private long contentRangeStart; public S3AInputStream(String bucket, String key, long contentLength, AmazonS3 client, FileSystem.Statistics stats, S3AInstrumentation instrumentation, long readahead, S3AInputPolicy inputPolicy) { Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket"); Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key"); Preconditions.checkArgument(contentLength >= 0 , "Negative content length"); this.bucket = bucket; this.key = key; + public S3AInputStream(S3ObjectAttributes s3Attributes, + long contentLength, + AmazonS3 client, + FileSystem.Statistics stats, + S3AInstrumentation instrumentation, + long readahead, + S3AInputPolicy inputPolicy) { + Preconditions.checkArgument( End diff – I'd rather these lines reverted to the previous ones, even if they are > 80 chars long. That is a soft limit on style checks: we break it if it keeps the code looking nice
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r98646873

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ObjectAttributes.java —
          @@ -0,0 +1,59 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + * <p>
          + * http://www.apache.org/licenses/LICENSE-2.0
          + * <p>
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.hadoop.fs.s3a;
          +
          +/**
          + * This class is only a holder for bucket, key, SSE Algorithm and SSE key
          + * attributes. It is only used in

          {@link S3AInputStream}

          + * as a way to reduce parameters being passed
          + * to the constructor of such class.
          + */
          +public class S3ObjectAttributes {
          — End diff –

          If this can be made package private, I'd like it to be. Otherwise it must be tagged

              @InterfaceAudience.Private
              
          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r98646873 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ObjectAttributes.java — @@ -0,0 +1,59 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs.s3a; + +/** + * This class is only a holder for bucket, key, SSE Algorithm and SSE key + * attributes. It is only used in {@link S3AInputStream} + * as a way to reduce parameters being passed + * to the constructor of such class. + */ +public class S3ObjectAttributes { — End diff – If this can be made package private, I'd like it to be. Otherwise it must be tagged @InterfaceAudience.Private
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on the issue:

          https://github.com/apache/hadoop/pull/183

          Overall, production code looks good —just some minor tweaks needed. Test code all LGTM, as do docs. As stated on the JIRA, I'd really like tests which verify that sse-c credentials are making it up. Anything we can do to expand coverage of the actual S3 state helps the users, and it helps catch any regressions in our code or the SDK library.

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on the issue: https://github.com/apache/hadoop/pull/183 Overall, production code looks good —just some minor tweaks needed. Test code all LGTM, as do docs. As stated on the JIRA, I'd really like tests which verify that sse-c credentials are making it up. Anything we can do to expand coverage of the actual S3 state helps the users, and it helps catch any regressions in our code or the SDK library.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user orngejaket commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r98723398

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java —
          @@ -98,24 +101,30 @@
          */
          private long contentRangeStart;

          • public S3AInputStream(String bucket,
          • String key,
          • long contentLength,
          • AmazonS3 client,
          • FileSystem.Statistics stats,
          • S3AInstrumentation instrumentation,
          • long readahead,
          • S3AInputPolicy inputPolicy) {
          • Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket");
          • Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key");
          • Preconditions.checkArgument(contentLength >= 0 , "Negative content length");
          • this.bucket = bucket;
          • this.key = key;
            + public S3AInputStream(S3ObjectAttributes s3Attributes,
            + long contentLength,
            + AmazonS3 client,
            + FileSystem.Statistics stats,
            + S3AInstrumentation instrumentation,
            + long readahead,
            + S3AInputPolicy inputPolicy) {
            + Preconditions.checkArgument(
              • End diff –

          I assume that I can break that convention elsewhere? I've got several lines that come in at 82-84 chars.

          Show
          githubbot ASF GitHub Bot added a comment - Github user orngejaket commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r98723398 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java — @@ -98,24 +101,30 @@ */ private long contentRangeStart; public S3AInputStream(String bucket, String key, long contentLength, AmazonS3 client, FileSystem.Statistics stats, S3AInstrumentation instrumentation, long readahead, S3AInputPolicy inputPolicy) { Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket"); Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key"); Preconditions.checkArgument(contentLength >= 0 , "Negative content length"); this.bucket = bucket; this.key = key; + public S3AInputStream(S3ObjectAttributes s3Attributes, + long contentLength, + AmazonS3 client, + FileSystem.Statistics stats, + S3AInstrumentation instrumentation, + long readahead, + S3AInputPolicy inputPolicy) { + Preconditions.checkArgument( End diff – I assume that I can break that convention elsewhere? I've got several lines that come in at 82-84 chars.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user orngejaket commented on the issue:

          https://github.com/apache/hadoop/pull/183

          > As stated on the JIRA, I'd really like tests which verify that sse-c credentials are making it up.

          The ITestS2AEncryptionSSEC tests currently just check to make sure that md5 of the encryption key is filled in. I'll change it to do a md5 on the key to verify it. The AWS UI won't show those files as encrypted in the properties or metadata. But the Java SDK can see if that one field is filled in with the md5.

          Show
          githubbot ASF GitHub Bot added a comment - Github user orngejaket commented on the issue: https://github.com/apache/hadoop/pull/183 > As stated on the JIRA, I'd really like tests which verify that sse-c credentials are making it up. The ITestS2AEncryptionSSEC tests currently just check to make sure that md5 of the encryption key is filled in. I'll change it to do a md5 on the key to verify it. The AWS UI won't show those files as encrypted in the properties or metadata. But the Java SDK can see if that one field is filled in with the md5.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I was thinking of a test which submits a file with one key, and tries to read it with another, expecting a failure/the wrong data. Is that possible if a new FS instance is created for the read and the write?

          Show
          stevel@apache.org Steve Loughran added a comment - I was thinking of a test which submits a file with one key, and tries to read it with another, expecting a failure/the wrong data. Is that possible if a new FS instance is created for the read and the write?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r99231072

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java —
          @@ -1788,6 +1817,83 @@ public void progressChanged(ProgressEvent progressEvent) {
          }
          }

          + protected void setOptionalMultipartUploadRequestParameters(
          + InitiateMultipartUploadRequest req) {
          + switch (serverSideEncryptionAlgorithm) {
          + case SSE_KMS:
          + req.setSSEAwsKeyManagementParams(generateSSEAwsKeyParams());
          + break;
          + case SSE_C:
          + if (StringUtils.isNotBlank(S3AUtils.getServerSideEncryptionKey(getConf()))) {
          — End diff –

          S3aUtils is already statically imported; no need to qualify.

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r99231072 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java — @@ -1788,6 +1817,83 @@ public void progressChanged(ProgressEvent progressEvent) { } } + protected void setOptionalMultipartUploadRequestParameters( + InitiateMultipartUploadRequest req) { + switch (serverSideEncryptionAlgorithm) { + case SSE_KMS: + req.setSSEAwsKeyManagementParams(generateSSEAwsKeyParams()); + break; + case SSE_C: + if (StringUtils.isNotBlank(S3AUtils.getServerSideEncryptionKey(getConf()))) { — End diff – S3aUtils is already statically imported; no need to qualify.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r99232624

          — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java —
          @@ -98,24 +101,26 @@
          */
          private long contentRangeStart;

          • public S3AInputStream(String bucket,
          • String key,
          • long contentLength,
          • AmazonS3 client,
          • FileSystem.Statistics stats,
          • S3AInstrumentation instrumentation,
          • long readahead,
          • S3AInputPolicy inputPolicy) {
          • Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket");
          • Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key");
          • Preconditions.checkArgument(contentLength >= 0 , "Negative content length");
          • this.bucket = bucket;
          • this.key = key;
            + public S3AInputStream(S3ObjectAttributes s3Attributes,
            + long contentLength,
            + AmazonS3 client,
            + FileSystem.Statistics stats,
            + S3AInstrumentation instrumentation,
            + long readahead,
            + S3AInputPolicy inputPolicy) {
            + Preconditions.checkArgument(StringUtils.isNotEmpty(s3Attributes.getBucket()), "No Bucket");
              • End diff –

          I wouldn't adjust the indentation, because it amplifies the diff and makes merging harder. Lines 105-110 should be the same as the original.

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r99232624 — Diff: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java — @@ -98,24 +101,26 @@ */ private long contentRangeStart; public S3AInputStream(String bucket, String key, long contentLength, AmazonS3 client, FileSystem.Statistics stats, S3AInstrumentation instrumentation, long readahead, S3AInputPolicy inputPolicy) { Preconditions.checkArgument(StringUtils.isNotEmpty(bucket), "No Bucket"); Preconditions.checkArgument(StringUtils.isNotEmpty(key), "No Key"); Preconditions.checkArgument(contentLength >= 0 , "Negative content length"); this.bucket = bucket; this.key = key; + public S3AInputStream(S3ObjectAttributes s3Attributes, + long contentLength, + AmazonS3 client, + FileSystem.Statistics stats, + S3AInstrumentation instrumentation, + long readahead, + S3AInputPolicy inputPolicy) { + Preconditions.checkArgument(StringUtils.isNotEmpty(s3Attributes.getBucket()), "No Bucket"); End diff – I wouldn't adjust the indentation, because it amplifies the diff and makes merging harder. Lines 105-110 should be the same as the original.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r99235308

          — Diff: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java —
          @@ -0,0 +1,158 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.hadoop.fs.s3a;
          +
          +import org.apache.hadoop.conf.Configuration;
          +import org.apache.hadoop.fs.FileSystem;
          +import org.apache.hadoop.fs.Path;
          +import org.apache.hadoop.fs.contract.s3a.S3AContract;
          +import org.junit.Ignore;
          +import org.junit.Rule;
          +import org.junit.Test;
          +import org.junit.rules.ExpectedException;
          +
          +import java.io.IOException;
          +import java.net.URI;
          +
          +import static org.apache.hadoop.fs.s3a.S3ATestUtils.*;
          +
          +/**
          + * Test whether or not encryption settings propagate by choosing an invalid
          + * one. We expect the S3AFileSystem to fail to initialize.
          + */
          +@Ignore
          +public class ITestS3AEncryptionAlgorithmValidation
          + extends AbstractS3ATestBase {
          +
          + @Rule
          + public ExpectedException expectedException = ExpectedException.none();
          +
          + @Test
          + public void testEncryptionAlgorithmSetToDES() throws Throwable {
          + expectedException.expect(IOException.class);
          + expectedException.expectMessage("Unknown Server Side algorithm DES");
          +
          + Configuration conf = super.createConfiguration();
          + //DES is an invalid encryption algorithm
          + conf.set(Constants.SERVER_SIDE_ENCRYPTION_ALGORITHM, "DES");
          + S3AContract contract = (S3AContract) createContract(conf);
          + contract.init();
          + //skip tests if they aren't enabled
          + assumeEnabled();
          + //extract the test FS
          + FileSystem fileSystem = contract.getTestFileSystem();
          + assertNotNull("null filesystem", fileSystem);
          + URI fsURI = fileSystem.getUri();
          + LOG.info("Test filesystem = {} implemented by {}", fsURI, fileSystem);
          + assertEquals("wrong filesystem of " + fsURI,
          + contract.getScheme(), fsURI.getScheme());
          + fileSystem.initialize(fsURI, conf);
          +
          + }
          +
          + @Test
          + public void testEncryptionAlgorithmSSECWithNoEncryptionKey() throws
          + Throwable {
          + expectedException.expect(IllegalArgumentException.class);
          + expectedException.expectMessage("The value of property " +
          + "fs.s3a.server-side-encryption-key must not be null");
          +
          — End diff –

          We're generally moving towards `LambdaTestUtils.intercept()`, because if a closure fails, intercept() will print what came back. Less important for voids though, and on branch-2 & java7, not so compelling.

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r99235308 — Diff: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java — @@ -0,0 +1,158 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs.s3a; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.contract.s3a.S3AContract; +import org.junit.Ignore; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +import java.io.IOException; +import java.net.URI; + +import static org.apache.hadoop.fs.s3a.S3ATestUtils.*; + +/** + * Test whether or not encryption settings propagate by choosing an invalid + * one. We expect the S3AFileSystem to fail to initialize. + */ +@Ignore +public class ITestS3AEncryptionAlgorithmValidation + extends AbstractS3ATestBase { + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + @Test + public void testEncryptionAlgorithmSetToDES() throws Throwable { + expectedException.expect(IOException.class); + expectedException.expectMessage("Unknown Server Side algorithm DES"); + + Configuration conf = super.createConfiguration(); + //DES is an invalid encryption algorithm + conf.set(Constants.SERVER_SIDE_ENCRYPTION_ALGORITHM, "DES"); + S3AContract contract = (S3AContract) createContract(conf); + contract.init(); + //skip tests if they aren't enabled + assumeEnabled(); + //extract the test FS + FileSystem fileSystem = contract.getTestFileSystem(); + assertNotNull("null filesystem", fileSystem); + URI fsURI = fileSystem.getUri(); + LOG.info("Test filesystem = {} implemented by {}", fsURI, fileSystem); + assertEquals("wrong filesystem of " + fsURI, + contract.getScheme(), fsURI.getScheme()); + fileSystem.initialize(fsURI, conf); + + } + + @Test + public void testEncryptionAlgorithmSSECWithNoEncryptionKey() throws + Throwable { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("The value of property " + + "fs.s3a.server-side-encryption-key must not be null"); + — End diff – We're generally moving towards `LambdaTestUtils.intercept()`, because if a closure fails, intercept() will print what came back. Less important for voids though, and on branch-2 & java7, not so compelling.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user steveloughran commented on a diff in the pull request:

          https://github.com/apache/hadoop/pull/183#discussion_r99235609

          — Diff: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java —
          @@ -0,0 +1,158 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.hadoop.fs.s3a;
          +
          +import org.apache.hadoop.conf.Configuration;
          +import org.apache.hadoop.fs.FileSystem;
          +import org.apache.hadoop.fs.Path;
          +import org.apache.hadoop.fs.contract.s3a.S3AContract;
          +import org.junit.Ignore;
          +import org.junit.Rule;
          +import org.junit.Test;
          +import org.junit.rules.ExpectedException;
          +
          +import java.io.IOException;
          +import java.net.URI;
          +
          +import static org.apache.hadoop.fs.s3a.S3ATestUtils.*;
          +
          +/**
          + * Test whether or not encryption settings propagate by choosing an invalid
          + * one. We expect the S3AFileSystem to fail to initialize.
          + */
          +@Ignore
          +public class ITestS3AEncryptionAlgorithmValidation
          + extends AbstractS3ATestBase {
          +
          + @Rule
          + public ExpectedException expectedException = ExpectedException.none();
          +
          + @Test
          + public void testEncryptionAlgorithmSetToDES() throws Throwable {
          + expectedException.expect(IOException.class);
          + expectedException.expectMessage("Unknown Server Side algorithm DES");
          +
          + Configuration conf = super.createConfiguration();
          + //DES is an invalid encryption algorithm
          + conf.set(Constants.SERVER_SIDE_ENCRYPTION_ALGORITHM, "DES");
          + S3AContract contract = (S3AContract) createContract(conf);
          + contract.init();
          + //skip tests if they aren't enabled
          + assumeEnabled();
          + //extract the test FS
          + FileSystem fileSystem = contract.getTestFileSystem();
          + assertNotNull("null filesystem", fileSystem);
          + URI fsURI = fileSystem.getUri();
          + LOG.info("Test filesystem = {} implemented by {}", fsURI, fileSystem);
          + assertEquals("wrong filesystem of " + fsURI,
          + contract.getScheme(), fsURI.getScheme());
          + fileSystem.initialize(fsURI, conf);
          +
          + }
          +
          + @Test
          + public void testEncryptionAlgorithmSSECWithNoEncryptionKey() throws
          + Throwable {
          + expectedException.expect(IllegalArgumentException.class);
          + expectedException.expectMessage("The value of property " +
          + "fs.s3a.server-side-encryption-key must not be null");
          +
          + Configuration conf = super.createConfiguration();
          + //SSE-C must be configured with an encryption key
          + conf.set(Constants.SERVER_SIDE_ENCRYPTION_ALGORITHM, S3AEncryptionMethods
          + .SSE_C.getMethod());
          + conf.set(Constants.SERVER_SIDE_ENCRYPTION_KEY, null);
          + S3AContract contract = (S3AContract) createContract(conf);
          + contract.init();
          + //skip tests if they aren't enabled
          + assumeEnabled();
          + //extract the test FS
          + FileSystem fileSystem = contract.getTestFileSystem();
          + assertNotNull("null filesystem", fileSystem);
          + URI fsURI = fileSystem.getUri();
          + LOG.info("Test filesystem = {} implemented by {}", fsURI, fileSystem);
          + assertEquals("wrong filesystem of " + fsURI,
          + contract.getScheme(), fsURI.getScheme());
          + fileSystem.initialize(fsURI, conf);
          + }
          +
          + @Test
          + public void testEncryptionAlgorithmSSECWithBlankEncryptionKey() throws
          + Throwable {
          + expectedException.expect(IOException.class);
          + expectedException.expectMessage("SSE-C is enabled and no " +
          — End diff –

          I do like shared constants here, with the constant in the production code, test reading it. Stops the tests being brittle to change in the message. A flaw with expectMessage is that it looks for the whole string, doesn't it; again, we prefer a .contains() as its not brittle to exceptions adding more diags

          Show
          githubbot ASF GitHub Bot added a comment - Github user steveloughran commented on a diff in the pull request: https://github.com/apache/hadoop/pull/183#discussion_r99235609 — Diff: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java — @@ -0,0 +1,158 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs.s3a; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.contract.s3a.S3AContract; +import org.junit.Ignore; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +import java.io.IOException; +import java.net.URI; + +import static org.apache.hadoop.fs.s3a.S3ATestUtils.*; + +/** + * Test whether or not encryption settings propagate by choosing an invalid + * one. We expect the S3AFileSystem to fail to initialize. + */ +@Ignore +public class ITestS3AEncryptionAlgorithmValidation + extends AbstractS3ATestBase { + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + @Test + public void testEncryptionAlgorithmSetToDES() throws Throwable { + expectedException.expect(IOException.class); + expectedException.expectMessage("Unknown Server Side algorithm DES"); + + Configuration conf = super.createConfiguration(); + //DES is an invalid encryption algorithm + conf.set(Constants.SERVER_SIDE_ENCRYPTION_ALGORITHM, "DES"); + S3AContract contract = (S3AContract) createContract(conf); + contract.init(); + //skip tests if they aren't enabled + assumeEnabled(); + //extract the test FS + FileSystem fileSystem = contract.getTestFileSystem(); + assertNotNull("null filesystem", fileSystem); + URI fsURI = fileSystem.getUri(); + LOG.info("Test filesystem = {} implemented by {}", fsURI, fileSystem); + assertEquals("wrong filesystem of " + fsURI, + contract.getScheme(), fsURI.getScheme()); + fileSystem.initialize(fsURI, conf); + + } + + @Test + public void testEncryptionAlgorithmSSECWithNoEncryptionKey() throws + Throwable { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("The value of property " + + "fs.s3a.server-side-encryption-key must not be null"); + + Configuration conf = super.createConfiguration(); + //SSE-C must be configured with an encryption key + conf.set(Constants.SERVER_SIDE_ENCRYPTION_ALGORITHM, S3AEncryptionMethods + .SSE_C.getMethod()); + conf.set(Constants.SERVER_SIDE_ENCRYPTION_KEY, null); + S3AContract contract = (S3AContract) createContract(conf); + contract.init(); + //skip tests if they aren't enabled + assumeEnabled(); + //extract the test FS + FileSystem fileSystem = contract.getTestFileSystem(); + assertNotNull("null filesystem", fileSystem); + URI fsURI = fileSystem.getUri(); + LOG.info("Test filesystem = {} implemented by {}", fsURI, fileSystem); + assertEquals("wrong filesystem of " + fsURI, + contract.getScheme(), fsURI.getScheme()); + fileSystem.initialize(fsURI, conf); + } + + @Test + public void testEncryptionAlgorithmSSECWithBlankEncryptionKey() throws + Throwable { + expectedException.expect(IOException.class); + expectedException.expectMessage("SSE-C is enabled and no " + — End diff – I do like shared constants here, with the constant in the production code, test reading it. Stops the tests being brittle to change in the message. A flaw with expectMessage is that it looks for the whole string, doesn't it; again, we prefer a .contains() as its not brittle to exceptions adding more diags
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Steve, I tried to kick off a yetus build, here, but it has got confused by the PRs: https://builds.apache.org/view/PreCommit%20Builds/job/PreCommit-HADOOP-Build/11569/console

          Can you just do a git diff branch-2...HEAD > HADOOP-13075-branch-2-001.patch to create the patch (assuming it's for branch2. If its for trunk it's a trunk...head and you can miss out the -branch-2 bit from the patch name. Then attach to the JIRA and hit the submit patch button. Thanks

          Show
          stevel@apache.org Steve Loughran added a comment - Steve, I tried to kick off a yetus build, here, but it has got confused by the PRs: https://builds.apache.org/view/PreCommit%20Builds/job/PreCommit-HADOOP-Build/11569/console Can you just do a git diff branch-2...HEAD > HADOOP-13075 -branch-2-001.patch to create the patch (assuming it's for branch2. If its for trunk it's a trunk...head and you can miss out the -branch-2 bit from the patch name. Then attach to the JIRA and hit the submit patch button. Thanks
          Hide
          moist Steve Moist added a comment -

          trunk patch

          Show
          moist Steve Moist added a comment - trunk patch
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 12 new or modified test files.
          0 mvndep 2m 0s Maven dependency ordering for branch
          +1 mvninstall 13m 45s trunk passed
          +1 compile 14m 16s trunk passed
          +1 checkstyle 1m 35s trunk passed
          +1 mvnsite 1m 29s trunk passed
          +1 mvneclipse 0m 41s trunk passed
          +1 findbugs 2m 5s trunk passed
          +1 javadoc 1m 12s trunk passed
          0 mvndep 0m 16s Maven dependency ordering for patch
          +1 mvninstall 0m 59s the patch passed
          +1 compile 11m 21s the patch passed
          +1 javac 11m 21s the patch passed
          -0 checkstyle 1m 35s root: The patch generated 8 new + 5 unchanged - 1 fixed = 13 total (was 6)
          +1 mvnsite 1m 32s the patch passed
          +1 mvneclipse 0m 43s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 3s The patch has no ill-formed XML file.
          +1 findbugs 2m 41s the patch passed
          +1 javadoc 1m 17s the patch passed
          -1 unit 9m 15s hadoop-common in the patch failed.
          +1 unit 0m 35s hadoop-aws in the patch passed.
          +1 asflicense 0m 36s The patch does not generate ASF License warnings.
          92m 33s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13075
          GITHUB PR https://github.com/apache/hadoop/pull/183
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux dc943a33d0d8 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / cb8f3f3
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11581/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11581/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11581/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11581/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 12 new or modified test files. 0 mvndep 2m 0s Maven dependency ordering for branch +1 mvninstall 13m 45s trunk passed +1 compile 14m 16s trunk passed +1 checkstyle 1m 35s trunk passed +1 mvnsite 1m 29s trunk passed +1 mvneclipse 0m 41s trunk passed +1 findbugs 2m 5s trunk passed +1 javadoc 1m 12s trunk passed 0 mvndep 0m 16s Maven dependency ordering for patch +1 mvninstall 0m 59s the patch passed +1 compile 11m 21s the patch passed +1 javac 11m 21s the patch passed -0 checkstyle 1m 35s root: The patch generated 8 new + 5 unchanged - 1 fixed = 13 total (was 6) +1 mvnsite 1m 32s the patch passed +1 mvneclipse 0m 43s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 3s The patch has no ill-formed XML file. +1 findbugs 2m 41s the patch passed +1 javadoc 1m 17s the patch passed -1 unit 9m 15s hadoop-common in the patch failed. +1 unit 0m 35s hadoop-aws in the patch passed. +1 asflicense 0m 36s The patch does not generate ASF License warnings. 92m 33s Reason Tests Failed junit tests hadoop.security.TestKDiag Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13075 GITHUB PR https://github.com/apache/hadoop/pull/183 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux dc943a33d0d8 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / cb8f3f3 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11581/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11581/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11581/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11581/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          wow, yetus liked it.

          • TestKDiag is unreleated
          • checkstyle, indentation complaints seem legit & trivial to fix.
          • the preconditions ones could be dealt with by making StringUtils.isNotEmpty a static import and so removing "StringUtils" from the line. And if they are still too wide: ignoring it
          Show
          stevel@apache.org Steve Loughran added a comment - wow, yetus liked it. TestKDiag is unreleated checkstyle, indentation complaints seem legit & trivial to fix. the preconditions ones could be dealt with by making StringUtils.isNotEmpty a static import and so removing "StringUtils" from the line. And if they are still too wide: ignoring it
          Hide
          moist Steve Moist added a comment -

          Corrected patch. The precondition lines are still too long with the static import.

          The other checkstyle issues were already corrected in the git commit in the fork but for some reason not included in the first patch.

          Show
          moist Steve Moist added a comment - Corrected patch. The precondition lines are still too long with the static import. The other checkstyle issues were already corrected in the git commit in the fork but for some reason not included in the first patch.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 12 new or modified test files.
          0 mvndep 0m 23s Maven dependency ordering for branch
          +1 mvninstall 14m 27s trunk passed
          +1 compile 13m 2s trunk passed
          +1 checkstyle 1m 35s trunk passed
          +1 mvnsite 1m 36s trunk passed
          +1 mvneclipse 0m 43s trunk passed
          +1 findbugs 2m 5s trunk passed
          +1 javadoc 1m 13s trunk passed
          0 mvndep 0m 17s Maven dependency ordering for patch
          +1 mvninstall 1m 9s the patch passed
          +1 compile 12m 9s the patch passed
          +1 javac 12m 8s the patch passed
          -0 checkstyle 1m 40s root: The patch generated 7 new + 5 unchanged - 1 fixed = 12 total (was 6)
          +1 mvnsite 1m 37s the patch passed
          +1 mvneclipse 0m 45s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 3s The patch has no ill-formed XML file.
          +1 findbugs 2m 22s the patch passed
          +1 javadoc 1m 17s the patch passed
          -1 unit 7m 51s hadoop-common in the patch failed.
          +1 unit 0m 33s hadoop-aws in the patch passed.
          +1 asflicense 0m 39s The patch does not generate ASF License warnings.
          90m 9s



          Reason Tests
          Failed junit tests hadoop.security.TestRaceWhenRelogin



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13075
          GITHUB PR https://github.com/apache/hadoop/pull/183
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux 153a04edff1a 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 663e683
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11590/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11590/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11590/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11590/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 12 new or modified test files. 0 mvndep 0m 23s Maven dependency ordering for branch +1 mvninstall 14m 27s trunk passed +1 compile 13m 2s trunk passed +1 checkstyle 1m 35s trunk passed +1 mvnsite 1m 36s trunk passed +1 mvneclipse 0m 43s trunk passed +1 findbugs 2m 5s trunk passed +1 javadoc 1m 13s trunk passed 0 mvndep 0m 17s Maven dependency ordering for patch +1 mvninstall 1m 9s the patch passed +1 compile 12m 9s the patch passed +1 javac 12m 8s the patch passed -0 checkstyle 1m 40s root: The patch generated 7 new + 5 unchanged - 1 fixed = 12 total (was 6) +1 mvnsite 1m 37s the patch passed +1 mvneclipse 0m 45s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 3s The patch has no ill-formed XML file. +1 findbugs 2m 22s the patch passed +1 javadoc 1m 17s the patch passed -1 unit 7m 51s hadoop-common in the patch failed. +1 unit 0m 33s hadoop-aws in the patch passed. +1 asflicense 0m 39s The patch does not generate ASF License warnings. 90m 9s Reason Tests Failed junit tests hadoop.security.TestRaceWhenRelogin Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13075 GITHUB PR https://github.com/apache/hadoop/pull/183 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux 153a04edff1a 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 663e683 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11590/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11590/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11590/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11590/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          OK, patch LGTM.

          I do want to do the tests myself (i.e run the full test suite with this turned on, and I'm not going to get a chance this week. If someone else wants to have a go and commit it, feel free —otherwise wait until next week (and ping me if I get distracted)

          Steve, Francisco —you've both some some really good work here

          Show
          stevel@apache.org Steve Loughran added a comment - OK, patch LGTM. I do want to do the tests myself (i.e run the full test suite with this turned on, and I'm not going to get a chance this week. If someone else wants to have a go and commit it, feel free —otherwise wait until next week (and ping me if I get distracted) Steve, Francisco —you've both some some really good work here
          Hide
          moist Steve Moist added a comment -

          Steve Loughran I've asked around on our side and we don't have anyone who's a committer that has enough bandwidth to run this. Is it all right if we have a non-committer run it and report back?

          Show
          moist Steve Moist added a comment - Steve Loughran I've asked around on our side and we don't have anyone who's a committer that has enough bandwidth to run this. Is it all right if we have a non-committer run it and report back?
          Hide
          stevel@apache.org Steve Loughran added a comment -

          sure: the more people who play with it the better: if anyone finds a problem, that's usually a valid finding.

          I''l wrap this up next week, regardless

          Show
          stevel@apache.org Steve Loughran added a comment - sure: the more people who play with it the better: if anyone finds a problem, that's usually a valid finding. I''l wrap this up next week, regardless
          Hide
          mackrorysd Sean Mackrory added a comment -

          Can we gracefully skip the SSE-KMS tests when it is not configured? As it is right now, if you run these tests with a configuration that currently works on trunk or with SEE-C configured, a bunch of tests will fail. Things ought to work as best they can with the minimal configuration, and if people configure additional things, fewer tests get skipped.

          Other than that it's looking pretty good in tests. I've tested in the US and Frankfurt. ITestS3AAWSCredentialsProvider.testAnonymousProvider fails any time you specify a region other than it expects (us-west-2, I think), which is definitely not related. I've had a couple of transient failures in the scale tests against Frankfurt, where operations are failing with 400 Bad Request, but it's not all the time, so my guess would be it's not related to a change in encryption. Gonna keep rerunning a bit to see if I can discern any other pattern there...

          Show
          mackrorysd Sean Mackrory added a comment - Can we gracefully skip the SSE-KMS tests when it is not configured? As it is right now, if you run these tests with a configuration that currently works on trunk or with SEE-C configured, a bunch of tests will fail. Things ought to work as best they can with the minimal configuration, and if people configure additional things, fewer tests get skipped. Other than that it's looking pretty good in tests. I've tested in the US and Frankfurt. ITestS3AAWSCredentialsProvider.testAnonymousProvider fails any time you specify a region other than it expects (us-west-2, I think), which is definitely not related. I've had a couple of transient failures in the scale tests against Frankfurt, where operations are failing with 400 Bad Request, but it's not all the time, so my guess would be it's not related to a change in encryption. Gonna keep rerunning a bit to see if I can discern any other pattern there...
          Hide
          moist Steve Moist added a comment -

          So with SSE-KMS, there's 2 cases, one where the user doesn't submit a KMS key and where they do.  I added a new test that will run with the default key(aws auto-configures this) .  The ones that require a key still exist, and now if the user doesn't specify said key, they will be skipped.

          Show
          moist Steve Moist added a comment - So with SSE-KMS, there's 2 cases, one where the user doesn't submit a KMS key and where they do.  I added a new test that will run with the default key(aws auto-configures this) .  The ones that require a key still exist, and now if the user doesn't specify said key, they will be skipped.
          Hide
          mackrorysd Sean Mackrory added a comment -

          I found a couple weird issues in my environment & configuration where some old stuff wasn't cleaned up properly. Having addressed that, I'm getting nice and reliable test runs with SSE-KMS with a non-default key ID, default key ID, and customer provided key, and I've double-checked in the console that it concurs about the encryption method being used in these buckets. I've tested in Ireland, Frankfurt, and a couple of US regions too.

          +1 (non-binding) once we can gracefully skip tests that are expected to fail when not configured.

          Show
          mackrorysd Sean Mackrory added a comment - I found a couple weird issues in my environment & configuration where some old stuff wasn't cleaned up properly. Having addressed that, I'm getting nice and reliable test runs with SSE-KMS with a non-default key ID, default key ID, and customer provided key, and I've double-checked in the console that it concurs about the encryption method being used in these buckets. I've tested in Ireland, Frankfurt, and a couple of US regions too. +1 (non-binding) once we can gracefully skip tests that are expected to fail when not configured.
          Hide
          moist Steve Moist added a comment -

          Branch-2 patch and trunk patches with updated ignoring KMS test if no key is defined.

          Show
          moist Steve Moist added a comment - Branch-2 patch and trunk patches with updated ignoring KMS test if no key is defined.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 1m 12s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 15 new or modified test files.
          0 mvndep 2m 44s Maven dependency ordering for branch
          +1 mvninstall 16m 59s trunk passed
          +1 compile 15m 3s trunk passed
          +1 checkstyle 1m 33s trunk passed
          +1 mvnsite 1m 35s trunk passed
          +1 mvneclipse 0m 50s trunk passed
          +1 findbugs 2m 3s trunk passed
          +1 javadoc 1m 13s trunk passed
          0 mvndep 0m 17s Maven dependency ordering for patch
          +1 mvninstall 1m 2s the patch passed
          +1 compile 11m 29s the patch passed
          +1 javac 11m 29s the patch passed
          -0 checkstyle 3m 50s root: The patch generated 7 new + 5 unchanged - 1 fixed = 12 total (was 6)
          +1 mvnsite 3m 58s the patch passed
          +1 mvneclipse 0m 47s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 3s The patch has no ill-formed XML file.
          +1 findbugs 2m 28s the patch passed
          +1 javadoc 1m 17s the patch passed
          -1 unit 7m 40s hadoop-common in the patch failed.
          +1 unit 0m 35s hadoop-aws in the patch passed.
          +1 asflicense 0m 38s The patch does not generate ASF License warnings.
          102m 23s



          Reason Tests
          Failed junit tests hadoop.net.TestDNS



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13075
          GITHUB PR https://github.com/apache/hadoop/pull/183
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle
          uname Linux b0c493a00425 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 5fb723b
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11603/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11603/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11603/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11603/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 1m 12s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 15 new or modified test files. 0 mvndep 2m 44s Maven dependency ordering for branch +1 mvninstall 16m 59s trunk passed +1 compile 15m 3s trunk passed +1 checkstyle 1m 33s trunk passed +1 mvnsite 1m 35s trunk passed +1 mvneclipse 0m 50s trunk passed +1 findbugs 2m 3s trunk passed +1 javadoc 1m 13s trunk passed 0 mvndep 0m 17s Maven dependency ordering for patch +1 mvninstall 1m 2s the patch passed +1 compile 11m 29s the patch passed +1 javac 11m 29s the patch passed -0 checkstyle 3m 50s root: The patch generated 7 new + 5 unchanged - 1 fixed = 12 total (was 6) +1 mvnsite 3m 58s the patch passed +1 mvneclipse 0m 47s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 3s The patch has no ill-formed XML file. +1 findbugs 2m 28s the patch passed +1 javadoc 1m 17s the patch passed -1 unit 7m 40s hadoop-common in the patch failed. +1 unit 0m 35s hadoop-aws in the patch passed. +1 asflicense 0m 38s The patch does not generate ASF License warnings. 102m 23s Reason Tests Failed junit tests hadoop.net.TestDNS Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13075 GITHUB PR https://github.com/apache/hadoop/pull/183 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit xml findbugs checkstyle uname Linux b0c493a00425 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 5fb723b Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11603/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11603/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11603/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-tools/hadoop-aws U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11603/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          eddyxu Lei (Eddy) Xu added a comment -

          +1. The latest patch LGTM. I ran the integration tests by myself and all tests have passed.

          Since this has already been +1ed by Steve Loughran and Sean Mackrory, I'd commit it now.

          Thanks for the patch, Steve Loughran. Thanks for the reviews and contributions, Steve Loughran, Chris Nauroth, Sean Mackrory, Federico Czerwinski, Aaron Fabbri, Larry McCay, Andrew Olson

          Show
          eddyxu Lei (Eddy) Xu added a comment - +1. The latest patch LGTM. I ran the integration tests by myself and all tests have passed. Since this has already been +1ed by Steve Loughran and Sean Mackrory , I'd commit it now. Thanks for the patch, Steve Loughran . Thanks for the reviews and contributions, Steve Loughran , Chris Nauroth , Sean Mackrory , Federico Czerwinski , Aaron Fabbri , Larry McCay , Andrew Olson
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Jenkins build Hadoop-trunk-Commit #11236 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11236/)
          HADOOP-13075. Add support for SSE-KMS and SSE-C in s3a filesystem. (lei: rev 839b690ed5edc2ac4984640d58c005bb63cd8a07)

          • (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java
          • (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java
          • (delete) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryption.java
          • (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
          • (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java
          • (edit) hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md
          • (delete) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmPropagation.java
          • (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEKMSUserDefinedKey.java
          • (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/AbstractTestS3AEncryption.java
          • (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSECBlockOutputStream.java
          • (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ABlockOutputStream.java
          • (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSES3BlockOutputStream.java
          • (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
          • (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEKMSDefaultKey.java
          • (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEKMSUserDefinedKeyBlockOutputStream.java
          • (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSES3.java
          • (add) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ObjectAttributes.java
          • (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEC.java
          • (delete) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionBlockOutputStream.java
          • (edit) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AGetFileStatus.java
          • (edit) hadoop-tools/hadoop-aws/pom.xml
          • (add) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AEncryptionMethods.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Jenkins build Hadoop-trunk-Commit #11236 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11236/ ) HADOOP-13075 . Add support for SSE-KMS and SSE-C in s3a filesystem. (lei: rev 839b690ed5edc2ac4984640d58c005bb63cd8a07) (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInputStream.java (delete) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryption.java (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AUtils.java (edit) hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/index.md (delete) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmPropagation.java (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEKMSUserDefinedKey.java (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/AbstractTestS3AEncryption.java (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSECBlockOutputStream.java (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ABlockOutputStream.java (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSES3BlockOutputStream.java (edit) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEKMSDefaultKey.java (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEKMSUserDefinedKeyBlockOutputStream.java (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSES3.java (add) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ObjectAttributes.java (add) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEC.java (delete) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionBlockOutputStream.java (edit) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AGetFileStatus.java (edit) hadoop-tools/hadoop-aws/pom.xml (add) hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AEncryptionMethods.java
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Not my patch: Federico Czerwinski & Steve Moist: hope they got the credits on the commit

          Show
          stevel@apache.org Steve Loughran added a comment - Not my patch: Federico Czerwinski & Steve Moist : hope they got the credits on the commit
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user orngejaket closed the pull request at:

          https://github.com/apache/hadoop/pull/183

          Show
          githubbot ASF GitHub Bot added a comment - Github user orngejaket closed the pull request at: https://github.com/apache/hadoop/pull/183
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user orngejaket commented on the issue:

          https://github.com/apache/hadoop/pull/183

          Closing because changes are already pushed in.

          Show
          githubbot ASF GitHub Bot added a comment - Github user orngejaket commented on the issue: https://github.com/apache/hadoop/pull/183 Closing because changes are already pushed in.
          Hide
          stevel@apache.org Steve Loughran added a comment - - edited

          just changed the title to put the word "encryption" in. This patch was committed with the wrong JIRA number BTW: HADOOP-13204

          For anyone looking for this patch in branch-2, the relevant commit is 6d62d0ea87acffb47e97904fc78405a36e9fc9d3

          Show
          stevel@apache.org Steve Loughran added a comment - - edited just changed the title to put the word "encryption" in. This patch was committed with the wrong JIRA number BTW: HADOOP-13204 For anyone looking for this patch in branch-2, the relevant commit is 6d62d0ea87acffb47e97904fc78405a36e9fc9d3
          Hide
          moist Steve Moist added a comment -

          Is that something that we need to fix?

          Show
          moist Steve Moist added a comment - Is that something that we need to fix?
          Hide
          stevel@apache.org Steve Loughran added a comment -

          something that cant be fixed; no playing git games with the main branches. Just a note here in the JIRA for people to find if they look for it by way of the release notes, and if cherry picking

          Show
          stevel@apache.org Steve Loughran added a comment - something that cant be fixed; no playing git games with the main branches. Just a note here in the JIRA for people to find if they look for it by way of the release notes, and if cherry picking
          Hide
          stevel@apache.org Steve Loughran added a comment - - edited

          Remind me, why are these tests not running in the parallel block? It's adding to over a minute to every test run -bumping it up for me from 4 min to 5 min.

          Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 12.864 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEC
          Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSECBlockOutputStream
          Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 12.373 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSECBlockOutputStream
          Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSDefaultKey
          Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 14.648 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSDefaultKey
          Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSUserDefinedKey
          Tests run: 2, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 0.011 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSUserDefinedKey
          Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSUserDefinedKeyBlockOutputStream
          Tests run: 2, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 0.01 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSUserDefinedKeyBlockOutputStream
          Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSES3
          Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 11.573 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSES3
          Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSES3BlockOutputStream
          Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.156 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSES3BlockOutputStream
          
          Show
          stevel@apache.org Steve Loughran added a comment - - edited Remind me, why are these tests not running in the parallel block? It's adding to over a minute to every test run -bumping it up for me from 4 min to 5 min. Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 12.864 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEC Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSECBlockOutputStream Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 12.373 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSECBlockOutputStream Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSDefaultKey Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 14.648 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSDefaultKey Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSUserDefinedKey Tests run: 2, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 0.011 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSUserDefinedKey Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSUserDefinedKeyBlockOutputStream Tests run: 2, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 0.01 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSEKMSUserDefinedKeyBlockOutputStream Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSES3 Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 11.573 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSES3 Running org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSES3BlockOutputStream Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.156 sec - in org.apache.hadoop.fs.s3a.ITestS3AEncryptionSSES3BlockOutputStream
          Hide
          moist Steve Moist added a comment -

          If I remember correctly, I kept getting the runs overwriting each other in S3 when run in parallel. This was mainly due to them being subclasses.

          Show
          moist Steve Moist added a comment - If I remember correctly, I kept getting the runs overwriting each other in S3 when run in parallel. This was mainly due to them being subclasses.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          oh, that'll just be path names not unique enough. Easily fixed.

          Show
          stevel@apache.org Steve Loughran added a comment - oh, that'll just be path names not unique enough. Easily fixed.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          For people following this, HADOOP-14305 is covering what appears to be a serious issue here: try running the encryption tests in parallel and things fail on getFileStatus() of directories. That's potentially a blocker: I don't want to ship this feature until we understand what's up and how make it go away. If its not fixable. we can discuss on that JIRA what to do next...maybe diagnostics and docs are the solution. Step 1 though: work out what's going wrong.

          Show
          stevel@apache.org Steve Loughran added a comment - For people following this, HADOOP-14305 is covering what appears to be a serious issue here: try running the encryption tests in parallel and things fail on getFileStatus() of directories. That's potentially a blocker: I don't want to ship this feature until we understand what's up and how make it go away. If its not fixable. we can discuss on that JIRA what to do next...maybe diagnostics and docs are the solution. Step 1 though: work out what's going wrong.

            People

            • Assignee:
              moist Steve Moist
              Reporter:
              noslowerdna Andrew Olson
            • Votes:
              4 Vote for this issue
              Watchers:
              22 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development