HADOOP-17922. Lookup old S3 encryption configs for JCEKS #3462
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
Contributor
Author
|
One commit in a test might be required which is in non-CSE mode, which I am currently running. Apart from that main change is in |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
steveloughran
requested changes
Sep 21, 2021
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
This all makes sense, I have one change for the docs
| private static final LogExactlyOnce IGNORE_CSE_WARN = new LogExactlyOnce(LOG); | ||
|
|
||
| /** Bucket name. */ | ||
| private String bucket; |
Contributor
Author
There was a problem hiding this comment.
Cannot make it final. Assigning it a value in a method
|
|
||
| // Get the encryption method for this bucket. | ||
| S3AEncryptionMethods encryptionMethods = | ||
| getEncryptionAlgorithm(uri.getHost(), conf); |
Contributor
|
Can I add some text to the per bucket settings override docs for this patch...I'll compose it and add it in a comment |
Contributor
mukund-thakur
left a comment
mukund-thakur
left a comment
There was a problem hiding this comment.
Looks good during review session. Let's add steve's doc.
Contributor
|
and the news is the method So: for encryption options not in jceks files, We don't get this for jceks files as there is no propagation, just lookup. I'm not going to worry about this, except to say "don't...I will add something in my patch which mentions this." |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
ok, I've added a patch in #3466 which
What now? I'd like that jceks resolution to be more consistent. (Oh, what a PITA). CSE is all good; we are just left trying to stabilise option names. @mehakmeet -can you cherrypick my patch into this PR and switch the failing test to use the new method you've added to look up the encryption settings? Then we can get that method to do what we want. |
Contributor
Author
|
Thanks for the changes @steveloughran. Coming to the tests. As expected the JCEKS test fails and gives the new global priority over old per-bucket configs.
So, basically, we would now make changes to the way we look-up so that this test passes and per-bucket config old or new gets priority over everything else? |
New unit test suite for propagation * adds TestBucketConfiguration for conf file propagation (succeeds) * one for jecks (fails; needs to sync up with the other patch) * moves over some test cases from ITestS3AConfiguration which don't need an FS. Change-Id: Ie1b6e0d1c655d00fc6d47ce054a1aeba01c71044
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
going to take this and then update with ordering of checks for old props following the exact same ordering as normal props |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Region: ap-south-1.
mvn clean verify -Dparallel-tests -DtestsThreadCount=4 -DscaleUsed long and short bucket property names for encryption properties for tests.
Tests run: 575, Failures: 0, Errors: 0, Skipped: 5Tests run: 1477, Failures: 0, Errors: 2, Skipped: 636Tests run: 151, Failures: 0, Errors: 1, Skipped: 28CSE:
Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 28.081 s - in org.apache.hadoop.fs.s3a.ITestS3AClientSideEncryptionKms