Skip to content
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

Update: Adds support for specifying optional assume role #60

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

bachu20
Copy link
Contributor

@bachu20 bachu20 commented Apr 24, 2024

Adds support for passing in an optional role arn argument for instances in which the module configuration is being applied via an assumed role.

  • Updates the relevant kms key policies to include this optional role as another principal. Otherwise it'll attempt to infer the current caller via the aws_caller_identity.

Copy link
Contributor

@closb closb left a comment

Choose a reason for hiding this comment

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

LGTM!

@bachu20 bachu20 merged commit 3c381cb into main Apr 24, 2024
4 checks passed
@bachu20 bachu20 deleted the update/add-support-for-specifying-assumed-role branch April 24, 2024 16:31
@@ -1,3 +1,9 @@
locals {
provisioner_role_arn = var.assume_role_arn == null ? data.aws_caller_identity.current.arn : var.assume_role_arn
cloudtrail_key_policy_root = var.is_existing_cloudtrail_cross_account == false ? "arn:aws:iam::${local.customer_aws_account_id}:root" : "arn:aws:iam::${var.existing_cloudtrail_log_bucket_account_id}:root"
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this always be the cloudtrail account & not the log bucket account? probably the else part is never applied since we don't even create the cloudtrial key for existing cloudtrails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I'll correct as a follow-on task but would rather not introduce new variables rn. I'll create a ticket to validate / remove.

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.

3 participants