Early check for dependency when set kmsKeyId #398
Conversation
| KmsClient kmsClient = KmsClient.builder() | ||
| .credentialsProvider(_awsCredentialsProvider) | ||
| .region(_region) |
There was a problem hiding this comment.
I do not think this check is sufficient to prevent the exception.
The KMS Client is an import of this class.
I think the Java Compiled Byte Code will require KMS,
even if a KMS key is not passed,
because the Java class has references to the objects.
But I could be wrong;
Maybe the Java compiler is fancier than I thought,
and masks the KMS objects unless this if statement is encountered?
@hieuwu did you run a test that proved that this mitigates your issue?
Because the current testing of the S3EC will not cover this.
There was a problem hiding this comment.
@texastony I got the same issue when run the app. As kms is dependency of S3EC, I would expect that it should fail at Compile Time. However the build is still successful, the exception is thrown only we start running the app or test.
I have removed the check. Also I have tried making software.amazon.awssdk:kms not optional, the lib works as expected, no exception thrown and we do not need to add additional dependency.
There was a problem hiding this comment.
I suspect this is the correct fix.
I'll approve CI run.
There was a problem hiding this comment.
Great, thank you Tony
|
Ahh, I'll will have to make this a first party PR, as compared to a 3rd party. We have had trouble with 3rd parties trying to bit coin mine via our CI, so it only runs if the PR is from the same repo. I'll take care of this. |
Issue #397, if available:
Got exception when run test and run application when provide S3EncryptionClient with kms
Caused by: java.lang.NoClassDefFoundError: software/amazon/awssdk/services/kms/KmsClient at software.amazon.encryption.s3.S3EncryptionClient$Builder.build(S3EncryptionClient.java:1142)The root cause of the problem is that kmsClient class comes from different dependencies, while other type of security key is in the same package of the SDK.
Description of changes:
Check for existence of
KmsClientclass when kmsKeyId is set.This would help developer easier to troubleshoot the problem & take proper action
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: