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

EBS Disk Encryption + Fix an extra log group #47

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

flyinprogrammer
Copy link

Test plan

Using this module I ran these changes locally against an AWS account and it worked.

@@ -113,6 +113,8 @@ resource "aws_launch_template" "executor" {
volume_type = "gp3"
iops = var.boot_disk_iops
throughput = var.boot_disk_throughput
encrypted = true
Copy link
Author

Choose a reason for hiding this comment

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

This is 💥 if people use this code, so maybe we want to shim this with a var for "false" for awhile?

Copy link
Member

Choose a reason for hiding this comment

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

You mean it would depend on the boot_disk_kms_key_id variable being set correctly? Could we say

Suggested change
encrypted = true
encrypted = var.boot_disk_kms_key_id != null ? true : false

to mitigate it?

Copy link
Member

Choose a reason for hiding this comment

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

Wait, no that can't be it.. we set encryption to true for the docker thing as well 🤔 I'm lost, what do you mean? 😬

Copy link
Author

Choose a reason for hiding this comment

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

😅 aws_launch_template

Nevermind, new instances will just start to get encrypted. I thought this part of an aws_instance definition, which would cause the instance to be recreated.

@@ -0,0 +1 @@
1.1.9
Copy link
Author

Choose a reason for hiding this comment

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

maybe this is annoying? it makes my life nicer.

Copy link
Member

Choose a reason for hiding this comment

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

we usually use asdf at sourcegraph which uses .tool-versions, could that help you, too?

@@ -73,6 +73,7 @@ resource "aws_security_group" "metrics_access" {

resource "aws_cloudwatch_log_group" "syslogs" {
# TODO: This is hardcoded in the executor image.
count = var.need_syslogs ? 1 : 0
Copy link
Author

Choose a reason for hiding this comment

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

ideally i think we'd pull some of this stuff out into a different module altogether

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if using a data source to check for existence might work. We don't actually need to create it, that happens automatically when the first log comes in, we just want to configure retention, really.

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