Skip to content

feat: Add KMS policy to Velero IAM policy for CMK KMS keys #578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zepellin
Copy link

Description

Adds and option to specify ARNs of KMS keys, for which additional policy blocks will be created containing actions to operate with these keys.

Motivation and Context

Using this module, if the var.velero_s3_kms_key_arns is set to an S3 bucket with default encryption set to customer managed KMS key (CMK), the backup will fail since the policy doesn't include statement with policies allowing operating this KMS keys (such as kms:Encrypt, kms:Decrypt etc.).

This is similar to the cases of EBS (var.ebs_csi_kms_cmk_ids) and S3 mountpoint (var.mountpoint_s3_csi_kms_arns) controllers IAM policies created by this module that contain similar blocks for the same reason.

Breaking Changes

No breaking changes, the newly introduced var.velero_s3_kms_key_arns variable has default of [] which doesn't change the behaviour of the current configuration.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@zepellin
Copy link
Author

Hi @bryantbiggs, any chance you could have a look at this one?

@@ -1565,6 +1565,22 @@ data "aws_iam_policy_document" "velero" {
]
resources = var.velero_s3_bucket_arns
}

dynamic "statement" {
for_each = length(var.velero_s3_kms_key_arns) > 0 ? [1] : []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work if you pass in computed values since those are unknown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants