Add CMK encryption support to DynamoDB export #3592
Conversation
Signed-off-by: Aiden Dai <[email protected]>
daixba
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
kkondaka,
asifsmohammed,
dlvenable and
oeyh
as code owners
| boolean isKmsKeyIdValid() { | ||
| // If key id is provided, it should be in a format like | ||
| // arn:aws:kms:us-west-2:123456789012:key/0a4bc22f-bb96-4ad3-80ca-63b12b3ec147 | ||
| return s3SseKmsKeyId == null || Arn.fromString(s3SseKmsKeyId).resourceAsString() != null; |
There was a problem hiding this comment.
I'm not sure we really want this validation to be so strict. Users should be allowed to provide KMS keys as aliases. e.g. alias/MyAliasedKey.
There was a problem hiding this comment.
I don't agree with this. Even from DynamoDB console, there is no way to provide an aliaes, can only provide KMS Key ID. Note that alias is optional to KMS CMK. I believe you want more flexibility here, but to get Key ID (which is used in the API call) by alias, we will need to have more permissions (maybe describe keys or something).
There was a problem hiding this comment.
Also alias may not work for multi-region keys.
There was a problem hiding this comment.
@daixba , Typically AWS services allow for either specifying an alias or an ARN. So my ask is not to require the user to provide an alias. It is to allow the user to provide either an ARN or an alias.
The documentation on this is unclear, but their restrictions indicate a minimum length of 1 which is compatible with an alias.
We shouldn't follow the Console, but what the API itself allows. If this API does not permit an alias, then I agree this should be only an ARN. But, it appears to support the alias.
| @@ -52,6 +52,8 @@ source: | |||
|
|
|||
| * s3_bucket (Required): The destination bucket to store the exported data files | |||
| * s3_prefix (Optional): Custom prefix. | |||
| * s3_sse_kms_key_id (Optional): A AWS KMS Customer Managed Key (CMK) to encrypt the export data files. The key id will | |||
There was a problem hiding this comment.
We have at least three options starting with s3_. It may be ideal to make this s3: since we haven't yet released this feature.
s3:
bucket: my-bucket
prefix: /custom/prefix
sse_kms_key_id: arn:...
There was a problem hiding this comment.
It may be confused this way, it seems more options are supported apart from S3.
So I think it's better if we just align this with the API structure: https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_ExportTableToPointInTime.html
There was a problem hiding this comment.
That makes sense - I'm ok with this.
| boolean isKmsKeyIdValid() { | ||
| // If key id is provided, it should be in a format like | ||
| // arn:aws:kms:us-west-2:123456789012:key/0a4bc22f-bb96-4ad3-80ca-63b12b3ec147 | ||
| return s3SseKmsKeyId == null || Arn.fromString(s3SseKmsKeyId).resourceAsString() != null; |
There was a problem hiding this comment.
We should probably wrap this in a try catch to avoid NPE's and exceptions from just throwing here with invalid ARN format, but this can be added as a follow on
Description
Add KMS CMK support to DynamoDB source when for encrypt the export output files using customer managed key.
Issues Resolved
Resolves #3564 #3470
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.