HADOOP-18778. Fixes failing tests when CSE is enabled. #5763
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@mukund-thakur / @mehakmeet and @dannycjones could you please review? |
dannycjones
reviewed
Jun 22, 2023
Comment on lines
448
to
451
| policy( | ||
| statement(false, S3_ALL_BUCKETS, S3_PATH_WRITE_OPERATIONS), | ||
| STATEMENT_ALL_S3, | ||
| STATEMENT_ALLOW_SSE_KMS_READ)); | ||
| STATEMENT_ALLOW_SSE_KMS_RW)); |
There was a problem hiding this comment.
Why does this need changing? It's for Server Side Encryption. (Was it always broken?)
There was a problem hiding this comment.
yes, think it may have always been broken when CSE is enabled. Without RW access, encryption client is not able to generate the key required to encrypt for a PUT. So the test fails with a "unable to generate" key exception.
There was a problem hiding this comment.
Oh yes... We also saw this error and this is the fix.
CC @mehakmeet
| @@ -118,6 +118,7 @@ private static int calculateNumBlocks(long largeFileSize, int blockSize) { | |||
| @Test | |||
| public void testReadLargeFileFully() throws Throwable { | |||
| describe("read a large file fully, uses S3ACachingInputStream"); | |||
| skipIfClientSideEncryption(); | |||
There was a problem hiding this comment.
Shall we just move these into openFS() since we're assuming the FS it provides is not compatible with CSE for now?
|
🎊 +1 overall
This message was automatically generated. |
mukund-thakur
approved these changes
Jun 22, 2023
Comment on lines
448
to
451
| policy( | ||
| statement(false, S3_ALL_BUCKETS, S3_PATH_WRITE_OPERATIONS), | ||
| STATEMENT_ALL_S3, | ||
| STATEMENT_ALLOW_SSE_KMS_READ)); | ||
| STATEMENT_ALLOW_SSE_KMS_RW)); |
There was a problem hiding this comment.
Oh yes... We also saw this error and this is the fix.
CC @mehakmeet
|
Had the same patch in my local repo 😄. Thanks for taking this up @ahmarsuhail. Yes, there was a gap in testing with Assume role + CSE enabled which led to this test being missed. Since there are quite a few toggleable features the testing coverage does get some blind spots. Not sure if there's already one, but a documented list of features that can cover the whole test suite could be helpful like CSE, Assume Roles, Access point ARN, SSE etc. |
|
thanks for the reviews! I looked into ITestPartialRenamesDeletes as well, it fails because the scope down policy does not include a wildcard for /test/testRenameParentPathNotWriteable[bulk-delete=true]-multi-1687342403.363/writableDir. The encryption client creates an instruction file test/testRenameParentPathNotWriteable[bulk-delete=false]-single-1687344364.529/writableDir.instruction, and since this is not in the policy, the delete fails with a 403. I am asking the SDK team if this is expected..as the instruction file is created by the encryption client, and not something users are aware of. For now, I've left it as failing. but maybe we can skip the test or add in a wildcard |
mehakmeet
approved these changes
Jun 23, 2023
| @@ -448,7 +448,7 @@ public void testReadOnlyOperations() throws Throwable { | |||
| policy( | |||
| statement(false, S3_ALL_BUCKETS, S3_PATH_WRITE_OPERATIONS), | |||
| STATEMENT_ALL_S3, | |||
| STATEMENT_ALLOW_SSE_KMS_READ)); | |||
| STATEMENT_ALLOW_SSE_KMS_RW)); | |||
There was a problem hiding this comment.
nit: We can maybe rename this constant to STATEMENT_ALLOW_KMS_RW since kms can be used server-side or client-side and both would apply the same policy.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks @mehakmeet , have updated the constant name, and fixed the test in |
|
@mehakmeet @mukund-thakur can we merge this if all looks ok? |
|
Looks good to me. |
|
thanks @mukund-thakur, could you please merge? |
|
Yet to be done for branch-3.3. |
ahmarsuhail
added a commit
to ahmarsuhail/hadoop
that referenced
this pull request
Nov 27, 2023
Contributed By: Ahmar Suhail <ahmarsu@amazon.co.uk>
ahmarsuhail
added a commit
to ahmarsuhail/hadoop
that referenced
this pull request
Dec 5, 2023
Contributed By: Ahmar Suhail <ahmarsu@amazon.co.uk>
ahmarsuhail
added a commit
to ahmarsuhail/hadoop
that referenced
this pull request
Dec 5, 2023
Contributed By: Ahmar Suhail <ahmarsu@amazon.co.uk>
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
Contributed By: Ahmar Suhail <ahmarsu@amazon.co.uk>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
JIRA: https://issues.apache.org/jira/browse/HADOOP-18778
ITestAssumeRole.testReadOnlyOperationswithout which KMS is not able to generate the Key required for writing, so failing before it is expected to.How was this patch tested?
Tested in
eu-west-1withmvn -Dparallel-tests -DtestsThreadCount=16 clean verify.ITestPartialRenamesDeletes.testRenameParentPathNotWriteablefails because the scope down policy does not include a wildcard for/test/testRenameParentPathNotWriteable[bulk-delete=true]-multi-1687342403.363/writableDir. The encryption client creates an instruction filetest/testRenameParentPathNotWriteable[bulk-delete=false]-single-1687344364.529/writableDir.instruction, and since this is not in the policy, the delete fails with a 403.I am checking with encryption client team to see if this expected. If it's expected, we can either skip this test for CSE or add in the wildcard.