-
Notifications
You must be signed in to change notification settings - Fork 487
Add encrypted root device and additional EBS volumes #196
Conversation
Do you want me to create an example for an additional volume? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Sanity check: is this type of EBS volume useful? EBS volumes you specify in a launch configuration are not "persistent" volumes that survive between redeploys of the EC2 instances in the ASG. So if you just need ephemeral disk storage that goes away when the instances are replaced, then this works fine, but if you are looking for persistent storage that will survive redeploys (e.g., the EBS volume is detached from an old instance and reattached to its replacement), this will not work.
If the ebs_block_devices
param does work, then yes, it's worth updating one of the existing examples to use it. That way, the tests will execute that code. As it is now, I don't think this code works (see the comment on using a map instead of list so the for_each
works).
variable "ebs_block_devices" { | ||
description = "List of EBS volumes" | ||
type = list | ||
default = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment with an example of the value you can set this too.
modules/consul-cluster/variables.tf
Outdated
|
||
variable "ebs_block_devices" { | ||
description = "List of EBS volumes" | ||
type = list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a map, not a list so that (a) the for_each
works and (b) each volume has a unique name/ID, so if a user removes one or adds one, it only affects that one, rather than all the items after it in a list.
modules/consul-cluster/variables.tf
Outdated
} | ||
|
||
variable "ebs_block_devices" { | ||
description = "List of EBS volumes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this description as follows:
- Mention that this is the list of EBS volumes to create... But the user will have to attach and mount these volumes themselves: e.g., in a User Data script.
- Specify that the type is a map from some unique name / ID for the volume (see comment below), which is used solely in this Terraform code, and that the values are objects with the fields supported by the
ebs_block_device
block inaws_launch_configuration
. You may want to link to theaws_launch_configuration
docs too.
Hi @brikis98, thanks for your comments. Yes initially i thought those EBS volumes would persist and I was obviously wrong. Do you still think there is value of attaching additional EBS volumes rather than increasing the root volume size? I can think of using cheap storage for OS, high performance storage for consul data. Or avoiding issues of the OS filling up. |
Separate EBS volumes, even ephemeral ones, could be useful for a variety of reasons. That said, I worry many will assume they are persistent volumes, and that'll lead to data loss & confusion... The way to add persistent volumes is to create them outside the ASG via |
Just want to +1 on this PR. While the volumes may be ephemeral, allowing for encryption to be enabled on them allows for global policy within organisations (ie. "Disk Encryption must be enabled") to be maintained. I accept that in the context of this specific module that may not be seen as relevant but adding this support inline helps technical folk skate past the somewhat less aware ("What's ephemeral?") compliance folk and "earn" their tick on that policy checkpoint. Also @robmorgan this addition snuck in still. I support that change too but it's not aligned with the PR. |
I have actually refactored the code completely, but not yet updated the PR yet. I am not happy with using an autoscaling group for consul and changed it to use a fixed number of instances with persistent storage. |
@mirkop-mattr So are you intending to abandon this PR then? To be honest my interest here is purely centered around the root volume within the ASG which is a fairly trivial change versus the addition of dynamic EBS volumes. |
I was not quite sure what to do as I was still experimenting with changing it to fixed instances. If it helps you I am happy to break this up and change the PR to enabling encryption for the root volume only. |
👍 (selfishly) Yes, it would be helpful otherwise I would look at forking and creating my own PR with just the root volume part added. |
Leave that with me. I will create two PRs: one for root volume encryption and another one for ubuntu 20.04 support. |
Fixes #160
Add option to use an encrypted root volume
Allow for additional EBS volumes to be mounted