HADOOP-16794. S3A reverts KMS encryption to the bucket's default KMS … #1875
Conversation
…key in rename/copy. AreContributed by Mukund Thakur. This addresses an issue which surfaced with KMS encryption: the wrong KMS key could be picked up in the S3 COPY operation, so renamed files, while encrypted, would end up with the bucket default key. As well as adding tests in the new suite ITestS3AEncryptionWithDefaultS3Settings, AbstractSTestS3AHugeFiles has a new test method to verify that the encryption settings also work for large files copied via multipart operations.
|
Tested with my bucket in ap-south-1 with params -Ds3guard -Ddynamo -Dparallel-tests -DtestsThreadCount=8 . All tests fine apart from ITestS3GuardToolDynamoDB.testDynamoDBInitDestroyCycle which was failing on base branh-3.2 as well. |
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
+1 with one question. Thanks @mukund-thakur
CC: @steveloughran
| return conf; | ||
| } | ||
|
|
||
| /** | ||
| * This removes the encryption settings from the |
There was a problem hiding this comment.
This does not "removes the encryption settings from the configuration". Could you explain why that is not required in branch-3.2 and/or update the java doc here?
There was a problem hiding this comment.
Yes comment is misleading. I had to fix the merge conflict and as removeBaseAndBucketOverrides() was not present earlier I had to delete that line thinking it is not required.
Although I can see the same method is present in S3ATestUtils but with different parameters and it is not used in this test.
@steveloughran Do you also think tests might break here if bucket level conf are set?
There was a problem hiding this comment.
I tried setting the bucket level conf for fs.s3a.bucket.mthakur-data.server-side-encryption.key and some tests for eg. ITestS3AEncryptionSSES3 started failing. I even tried adding the removeBaseAndBucketOverrides call but it doesn't help.
I tried setting then same bucket level conf in apache/trunk as well and the tests are succeeding there.
The reason for test failure is
java.io.IOException: AES256 is enabled but an encryption key was set in fs.s3a.server-side-encryption.key (key of length 76 ending with 8) at org.apache.hadoop.fs.s3a.S3AUtils.getEncryptionAlgorithm(S3AUtils.java:1440) at org.apache.hadoop.fs.s3a.S3AFileSystem.initialize(S3AFileSystem.java:316) at org.apache.hadoop.fs.FileSystem.createFileSystem(FileSystem.java:3298) at org.apache.hadoop.fs.FileSystem.get(FileSystem.java:476)
removeBaseAndBucketOverrides() call essentially removes this parameter not sure how it is getting added in FileSystem initialisation flow again. It is too hard to compare and debug the difference between branch-3.2 and trunk as there are many changes. Any pointers in this regards would be greatly appreciated.
Also could someone please trigger the test based on their configs and share the results. Thanks.
There was a problem hiding this comment.
I can reproduce the test failure. I ran the test with fs.s3a.bucket.BUCKETNAME.server-side-encryption.key set in auth-keys.xml.
I set the S3 bucket default encryption policy as SSE-KMS, and that is not changing the ITestS3AEncryptionSSES3 test failure (as expected). Well, test ITestS3AEncryptionWithDefaultS3Settings could pass then (again, as expected).
There was a problem hiding this comment.
OK, I did some investigation, and the observation is as following:
- The failing tests are just about tests. The fix in copy options should work as expected. Specially, some encryption tests fail only when bucket level encryption is set.
- The inconsistent encryption algorithm and key situation in conf is partially because of the
addConfResource(CONTRACT_XML);function after creating and patching the conf object, see here. - The call to
addConfResource(CONTRACT_XML);should be considered a tricky unrelated bug, not brought by this patch. Most likely it is fixed by HADOOP-16626 intrunk. Guess that's why we don't see test failures there. I'm wondering howtrunkis not failing this...? Was HADOOP-16626 enough? - Specially, there are some discussions about the Configuration loading resources "after" the conf object has been patched when setting up the S3 tests. In our test case, if you have the change to call
removeBucketOverrides(), and you have the change inS3AContractnot callingaddConfResource(CONTRACT_XML);, then you can test with this: you add multipletestEncryption()and multipletestEncryptionOverRename()test cases inAbstractTestS3AEncryption(example). Guess what, only one test fail and all others are failing...How interesting? This seems very related to static resource loading when creating S3AFileSystem...
I'll stop here since I for now have no more context. I will request @steveloughran to provide more help. Shall we backport HADOOP-16626 to branch-3.2 and see?
liuml07
added
bug
fs/s3
|
@steveloughran and @mukund-thakur Per my previous investigation in this review, I have backported the changes in HADOOP-16626 partially into Then I tested with Question: does this backport make sense to happen separately, or we can merge with this PR? |
|
Again, for the
|
|
Thanks @liuml07 for the investigation. I believe HADOOP-16626 needs to be backported to branch 3.2. We will wait for @steveloughran comments. |
|
I think the way to do 3.2 support properly is to pretty much backport all the 3.3.x stuff in order |
|
@steveloughran Thanks. I think bulk backporting 3.3 stuff to 3.2 for most if not all hadoop-aws module changes makes sense. I see HADOOP-15619 is already fixed. Is this a good time to do the bulk backport? Specially, I think we need to backport this security fix to 2.10 branch. We locally backported (without test coding due to conflicts) and it works well on that branch so far. |
|
yeah, bulk backport it is. Some of the stuff are big features (delegation tokens) that I'd targeted 3.3 with, but they change the code enough its easiest to leave them in. HADOOP-13230 (#1861) is a WiP -a full backport will make cherrypicking that to 3.2 straightforward. I'll have to a new patch for <3.1. Not because I want them to add the ability to retain markers, but I don't want them to mistake a dir marker for an empty dir. That's also why marker retention is off by default, and the "authoritative" option will restrict it to auth mode directories, so Hive can do it with its managed directories while leaving the rest alone |
|
Thanks @steveloughran I can help review #1861 That is a big patch. For the bulk import, I don't have enough context which JIRAs should be left in 3.3 and may not be able to resolve conflicts. I can help with review and testing though. |
|
yeah, #1861 is big. I am going to see if I can get the changed getFileStatus code into hadoop 3.3.0 before it is frozen, so I don't have to worry about cross-version compatibility there. It will still be deleting directory markers as today, simply doing a LIST for marker & children and skipping HEAD + / |
|
what's the status of this? Merge or WONTFIX? |
|
@steveloughran I think we can merge this into branch-2.10. This is very important fix. The failing tests are just about tests per my previous debugging comment. The fix itself is reviewed and already in 3.3.0 releases. We have backported this fix into our internal light-fork 6 months ago, and customers have not complained since then. For the normal testing without per-bucket settings, the test will pass. Thoughts? |
|
@steveloughran Given the failing tests are not related to the fix per se, do you think it's fine if I backport the fix only without including tests into |
fine with me |
|
Thanks @steveloughran I will file a new PR for backporting the fix (not necessarily including test changes) this into So, is my another proposal also valid? Specially, If we plan the effort of bulk backporting those good stuff from 3.3/3.4 back to 3.2, the per-bucket test failures will also be resolved. But I do not see that is planed yet. Since this PR is fixing security issues, I think we can merge this PR into branch-3.2 before that. If so, @mukund-thakur can help us rebase and ideally run another round of tests. |
|
3.2 and 3.3 have diverged so much in s3a. I tried a bit of a backport and gave up. Now, internally, CDH 7/CDP is built on the 3.1+ codebase, with much of the s3a changes backported. But its not always easy, and was done as code was added
I'm reluctant to replicate the same problems here. What would be good
|
…key in rename/copy.
AreContributed by Mukund Thakur.
This addresses an issue which surfaced with KMS encryption: the wrong
KMS key could be picked up in the S3 COPY operation, so
renamed files, while encrypted, would end up with the
bucket default key.
As well as adding tests in the new suite
ITestS3AEncryptionWithDefaultS3Settings,
AbstractSTestS3AHugeFiles has a new test method to
verify that the encryption settings also work
for large files copied via multipart operations.