S3 Copy Operator: Support Custom KMS Keys #60597
Conversation
boring-cyborg
bot
added
area:providers
provider:amazon
| self.source_version_id = source_version_id | ||
| self.acl_policy = acl_policy | ||
| self.meta_data_directive = meta_data_directive | ||
| self.kms_key_id = kms_key_id |
There was a problem hiding this comment.
Is there any constraint/dependency between the 2? e.g. If one is specified, does the second one need to be provided as well?
| self.meta_data_directive, | ||
| **copy_args, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Passing the KMS parameters via **kwargs makes the new functionality dependent on copy_object continuing to forward those parameters. The tests you added do help protect against refactors of copy_object breaking this behavior, but the contract itself isn’t explicitly represented in the public API. I understand the motivation to avoid modifying the public API, but that argument is more defensible for breaking changes. Adding optional parameters would be non-breaking here and would make the contract more explicit. This is of course just a suggestion as your tests prevent regressions.
There was a problem hiding this comment.
That's definitely a good point, thanks for the suggestion
3 participants
closes: #55708
This PR introduces 2 new parameters to the S3CopyObjectOperator. With these one can specify a non-default KMS key when copying objects between buckets.
kms_key_id: The ARN, ID or alias of a KMS keykms_encryption_type: Whether it is standard KMS or double-shielded KMSThe parameters are passed to the Hooks
copy_object()method using kwargs, which passes them to the boto3 method.I did this to not introduce new parameters to the method and keep changes minimal, however I also see a point that this is not as clean as passing them to the method via parameters and if deemed preferential will change that.
Was generative AI tooling used to co-author this PR?