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

support-kms-key-id-for-root-volume #128

Merged

Conversation

woz5999
Copy link
Contributor

@woz5999 woz5999 commented May 18, 2022

what

  • Support customer managed kms key for block device

why

  • This is supported for other EBS volumes in the module
  • CMK should be supported for root device as well

@woz5999 woz5999 requested review from a team as code owners May 18, 2022 21:53
@woz5999 woz5999 requested review from nitrocode and RothAndrew May 18, 2022 21:53
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

Bridgecrew has found infrastructure configuration errors in this PR ⬇️

@@ -141,6 +141,7 @@ resource "aws_instance" "default" {
iops = local.root_iops
delete_on_termination = var.delete_on_termination
encrypted = var.root_block_device_encrypted
kms_key_id = var.root_block_device_kms_key_id
Copy link

Choose a reason for hiding this comment

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

LOW   Ensure that EC2 is EBS optimized
    Resource: aws_instance.default | ID: BC_AWS_GENERAL_68

How to Fix

resource "aws_instance" "foo" {
  ...
+ ebs_optimized = true
}

Description

TBA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nitrocode what's the best way to address this, since optimization isn't in the scope of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@woz5999 I would just add a new input called ebs_optimized and default it to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nitrocode upon further inspection, it looks like the var already exists and is applied to the instance, but is defaulted to false. I'm happy to make the change to update the default, but the advisability of that is probably for someone at cloudposse to decide. lmk what you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nitrocode any guidance here?

Copy link
Member

Choose a reason for hiding this comment

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

No worries then. We can fix that in a follow up pr.

@woz5999
Copy link
Contributor Author

woz5999 commented May 18, 2022

/rebuild-readme

variables.tf Show resolved Hide resolved
Co-authored-by: nitrocode <[email protected]>
@woz5999
Copy link
Contributor Author

woz5999 commented Jun 6, 2022

/rebuild-readme

@woz5999
Copy link
Contributor Author

woz5999 commented Jun 6, 2022

/test all

@nitrocode
Copy link
Member

/test all

@nitrocode nitrocode merged commit e3aee1c into cloudposse:master Jun 17, 2022
joelsdc pushed a commit to TRIPP-Inc/terraform-aws-ec2-instance that referenced this pull request Apr 10, 2023
* support-kms-key-id-for-root-volume

* Auto Format

* Update variables.tf

Co-authored-by: nitrocode <[email protected]>

* Auto Format

* Update main.tf

* Update variables.tf

* Update main.tf

* Auto Format

Co-authored-by: cloudpossebot <[email protected]>
Co-authored-by: nitrocode <[email protected]>
joelsdc pushed a commit to TRIPP-Inc/terraform-aws-ec2-instance that referenced this pull request Apr 10, 2023
* support-kms-key-id-for-root-volume

* Auto Format

* Update variables.tf

Co-authored-by: nitrocode <[email protected]>

* Auto Format

* Update main.tf

* Update variables.tf

* Update main.tf

* Auto Format

Co-authored-by: cloudpossebot <[email protected]>
Co-authored-by: nitrocode <[email protected]>
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