HADOOP-18708. AWS SDK V2 - Implement CSE #6164
Open
+413
−30
Conversation
This file contains 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. |
ahmarsuhail
commented
Oct 10, 2023
| configureClientBuilder(S3AsyncClient.builder(), parameters, conf, bucket).httpClientBuilder( | ||
| httpClientBuilder); | ||
|
|
||
| if (!parameters.isClientSideEncryptionEnabled()) { |
There was a problem hiding this comment.
We need to do this because currently if you try to do a ranged GET with multipart enabled, it fails. This is intentional, as the SDK team is working on adding "automatic multipart download" for a GET to the async client, and this capability is disabled till then.
When we build the S3EncryptionClient we configure and pass in a S3AsyncClient. Under the hood, S3EC uses the S3Async client for all it's encryption related operations (eg: GET). And so if we pass in a client with multipartEnabled, all GETs will start failing.
The impact of this that if you're using CSE, there's no multipart mode, so you copies become slower again.
A workaround can be to create a new async client with multipart disabled. So we have two async clients, one with MPU enable, one with disabled. and share underlying HTTP client b/w the two async clients, so it shouldn't have too much of an impact.
I'll make that change if required, once I have more clarity from SDK team on when ranged GETs become available in multipart mode.
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Looks good, but we need the new jar to be optional.
One possibility here would to make CSEMaterials a parameter in S3ClientCreationParameters, and when encyption is used switch to a new client factory, one which creates encrypting clients internally, if there's a classload/binding problem, that'll be reported, but the default impl is nicely isolated and s3afs code simpler.
|
|
||
| S3ClientFactory clientFactory = ReflectionUtils.newInstance(s3ClientFactoryClass, conf); | ||
| s3Client = clientFactory.createS3Client(getUri(), parameters); | ||
| createS3AsyncClient(clientFactory, parameters); | ||
|
|
There was a problem hiding this comment.
given its a new jar, any way we can isolate this stuff?
| @@ -575,6 +579,12 @@ public void testS3SpecificSignerOverride() throws Exception { | |||
| .describedAs("Custom STS signer not called").isTrue(); | |||
| } | |||
|
|
|||
| private void unsetClientSideConfiguration(Configuration conf) { | |||
There was a problem hiding this comment.
change name. e.g unsetEncryption
hadoop-tools/hadoop-aws/pom.xml
Outdated
| <dependency> | ||
| <groupId>software.amazon.encryption.s3</groupId> | ||
| <artifactId>amazon-s3-encryption-client-java</artifactId> | ||
| <scope>compile</scope> |
There was a problem hiding this comment.
we need this to be scoped as provided, with matching changes in the code
| * @param cseMaterials cse key and type to use | ||
| * @return S3EncryptionClient | ||
| */ | ||
| S3Client createS3EncryptionClient(S3AsyncClient s3AsyncClient, S3Client s3Client, |
There was a problem hiding this comment.
- have an EncryptionClientCreationParameters with CSEMaterials part of it
- can we actually use a subclass of the default factory which does support encryption.
| * @return S3EncryptionClient | ||
| */ | ||
| S3Client createS3EncryptionClient(S3AsyncClient s3AsyncClient, S3Client s3Client, | ||
| CSEMaterials cseMaterials); |
There was a problem hiding this comment.
add default throws new UnsupportedOperationException(). i've already got a separate implementation for HBoss and this patch breaks it right now
|
@steveloughran thanks for the review, isolated all encryption stuff to a new client factory which makes things much cleaner now. Test latest changes in eu-west-1 with |
|
This is the first time I'm working with a |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
You can add a rule to the enforcer plugin to ban imports except in managed places, the way we do for mapreduce. |
steveloughran
changed the title
|
...oh and cloudstore storediag is some implicit validation. |
steveloughran
requested changes
Nov 15, 2023
| @@ -32,12 +32,12 @@ | |||
| import software.amazon.awssdk.http.apache.ApacheHttpClient; | |||
| import software.amazon.awssdk.http.apache.ProxyConfiguration; | |||
| import software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient; | |||
| import software.amazon.awssdk.thirdparty.org.apache.http.client.utils.URIBuilder; | |||
There was a problem hiding this comment.
this stops people building docker images with more minimal packaging than the bundle.jar; something we had to fix before.
| @@ -170,6 +171,15 @@ public static IOException translateException(@Nullable String operation, | |||
| operation, | |||
| StringUtils.isNotEmpty(path)? (" on " + path) : "", | |||
| exception); | |||
|
|
|||
| // Exceptions from encryption client are wrapped in S3EncryptionClientException, so unwrap. | |||
| if (exception instanceof S3EncryptionClientException) { | |||
There was a problem hiding this comment.
this will make the new library mandatory. need some more work here, such as simply looking a class details of nested exception. should also go into org.apache.hadoop.fs.s3a.impl.ErrorTranslation
with a unit test
ahmarsuhail
force-pushed
the
HADOOP-18708-implement-cse
branch
from
1dc57cd
to
ef3534e
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
thanks @steveloughran , addressed review comments |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
updated review.
I think we are going to have to look hard at the bundle.jar and what we can do there w.r.t going to something leaner with all we need (inc all transitive dependencies *except SLF4J) and nothing we don't. some "hadoop-aws-bundle" jar in its own module
docs will need updating for this
hadoop-project/pom.xml
Outdated
| @@ -188,6 +188,7 @@ | |||
| <surefire.fork.timeout>900</surefire.fork.timeout> | |||
| <aws-java-sdk.version>1.12.565</aws-java-sdk.version> | |||
| <aws-java-sdk-v2.version>2.20.160</aws-java-sdk-v2.version> | |||
| <amazon-s3-encryption-client-java.version>3.1.0</amazon-s3-encryption-client-java.version> | |||
There was a problem hiding this comment.
so this isn't in the big bundle? what does it depend on transitively?
| @@ -106,6 +110,24 @@ public static IOException maybeExtractIOException(String path, Throwable thrown) | |||
|
|
|||
| } | |||
|
|
|||
| /** | |||
| * Extracts the underlying exception from an S3EncryptionClientException. | |||
There was a problem hiding this comment.
nit: wrong class named
| */ | ||
| public static SdkException maybeExtractSdkException(SdkException exception) { | ||
| SdkException extractedException = exception; | ||
| if (exception.toString().contains(ENCRYPTION_CLIENT_EXCEPTION)) { |
There was a problem hiding this comment.
- should this be in the classname, or is it one of those things which gets passed down as strings?
- also include check for cause being instanceof SdkException so class cast problems don't lose stack trace of any other problem
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/impl/TestErrorTranslation.java
Show resolved
Hide resolved
4 tasks
|
@ahmarsuhail can you rebase this and we can merge in to branch-3.4. thanks |
ahmarsuhail
force-pushed
the
HADOOP-18708-implement-cse
branch
from
43e8308
to
2a0407a
Compare
|
rebased, testing in progress. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Tested in eu-west-1, with Seeing a few failures: Suspect the vectoreIO failures are intermittent, and |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
4 tasks
| @@ -1149,6 +1150,17 @@ | |||
| </exclusion> | |||
| </exclusions> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>software.amazon.encryption.s3</groupId> | |||
There was a problem hiding this comment.
is this going to get into bundle.jar?
| public static SdkException maybeExtractSdkException(SdkException exception) { | ||
| SdkException extractedException = exception; | ||
| if (exception.toString().contains(ENCRYPTION_CLIENT_EXCEPTION) | ||
| && exception.getCause() instanceof SdkException) { |
There was a problem hiding this comment.
lets put this at the top and return null immediately; lines up for adding more probes.
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/ErrorTranslation.java
Show resolved
Hide resolved
ahmarsuhail
force-pushed
the
HADOOP-18708-implement-cse
branch
from
30acceb
to
eb56cbf
Compare
|
Thanks @steveloughran . While looking at the test failures, I found a couple of issues with S3 Encryption Client, opened aws/amazon-s3-encryption-client-java#200 and aws/amazon-s3-encryption-client-java#201 there.
which gets translated to which currently doesn't get translated properly, so need to add some handling there. Will wait for the fixes to S3EC, and then update error translation + address comments. Also asked about adding it to bundle. Looking at S3EC dependencies, I think it may cause us some issues, but not sure: What do you think? |
easily added to the check
just less data? not ideal. |
steveloughran
requested changes
Apr 11, 2024
There was a problem hiding this comment.
commented. nice to see our tests catching bugs in the sdk bits. they should make it part of the nightlies (hint, hint,...)
going to need some doc updates on
- how to use in installations "add more stuff"
- how to use in downstream builds
wonder if a hadoop-aws-cse module should be added as a pom-only dependency for that
| * This class is for storing information about key type and corresponding key | ||
| * to be used for client side encryption. | ||
| */ | ||
| public class CSEMaterials { |
There was a problem hiding this comment.
does this stuff get passed through delegation tokens? as they can be used to pass encryption secrets into a cluster
|
|
||
| import static org.apache.hadoop.fs.s3a.impl.InstantiationIOException.unavailable; | ||
|
|
||
| public class EncryptionS3ClientFactory extends DefaultS3ClientFactory { |
There was a problem hiding this comment.
javadocs, which include mentioning that this needs the cse on the runtime
| size).build(); | ||
| size); | ||
|
|
||
| if (isLast) { |
There was a problem hiding this comment.
add this as a param to pass down to WriteOperationHelper and let the factory do the work
| .withCSEKeyType(CSEMaterials.CSEKeyType.KMS) | ||
| .withKmsKeyId(kmsKeyId); | ||
|
|
||
| clientFactory = ReflectionUtils.newInstance(EncryptionS3ClientFactory.class, conf); |
There was a problem hiding this comment.
so if encryption is set, no custom client factory (done in places for unit tests) work. proposed: warn if cse is enabled and the value of (s3ClientFactoryClass is not the default
the other way would be to choose the default client factory based on the encryption flag, so tests will always override it (the documentation will have to cover it). I think I prefer that
| @@ -1034,6 +1035,21 @@ private void bindAWSClient(URI name, boolean dtEnabled) throws IOException { | |||
| S3_CLIENT_FACTORY_IMPL, DEFAULT_S3_CLIENT_FACTORY_IMPL, | |||
| S3ClientFactory.class); | |||
|
|
|||
| S3ClientFactory clientFactory; | |||
| CSEMaterials cseMaterials = null; | |||
There was a problem hiding this comment.
this stuff MUST pick up what is in EncryptionSecrets (which you can extend but MUST NOT use any aws sdk classes). this is what gets passed round with delegation tokens, so allows you to pass secrets with a job and without the cluster having the config
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
This adds in CSE back in back in.
Also adds a new CSEMaterials class which will allow you set different key types (RSA, AES or KMS), which can be extended to allow uses to specify their own keys. That work can be done separately.
This was previously part of #5767 which had been reviewed but had some other changes as well and had fallen quite behind. Closed that PR in favour of this.
How was this patch tested?
Tested in eu-west-1 with
mvn -Dparallel-tests -DtestsThreadCount=16 clean verifyand scale tests enabled.