hashicorp / terraform-aws-consul

A Terraform Module for how to run Consul on AWS using Terraform and Packer
Apache License 2.0
401 stars 484 forks source link

Add encrypted root device and additional EBS volumes #196

Closed mirkop-mattr closed 3 years ago

mirkop-mattr commented 3 years ago

Fixes #160

Add option to use an encrypted root volume

Allow for additional EBS volumes to be mounted

mirkop-mattr commented 3 years ago

Do you want me to create an example for an additional volume?

mirkop-mattr commented 3 years ago

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).

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. I have not found a simple way of having persistent disk attached ( similar to a Statefulset in Kubernetes) to store the consul data. It's a bit scary to think that if there is not a single EC2 instances running all your consul data is lost.

brikis98 commented 3 years ago

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).

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. I have not found a simple way of having persistent disk attached ( similar to a Statefulset in Kubernetes) to store the consul data. It's a bit scary to think that if there is not a single EC2 instances running all your consul data is lost.

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 aws_ebs_volume resources. You then add code to your EC2 instances to attach volumes when they are booting. But there's more complexity there, as you can only attach an EBS volume in the same AZ as the EC2 instance, whereas an ASG could launch an instance in any AZ...

perlboy commented 3 years ago

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.

mirkop-mattr commented 3 years ago

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.

perlboy commented 3 years ago

@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.

mirkop-mattr commented 3 years ago

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.

perlboy commented 3 years ago

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.

:+1: (selfishly) Yes, it would be helpful otherwise I would look at forking and creating my own PR with just the root volume part added.

mirkop-mattr commented 3 years ago

Leave that with me. I will create two PRs: one for root volume encryption and another one for ubuntu 20.04 support.