Skip to content

fix: Only create mount point for EFS when using EFS #261

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Apr 1, 2022
Merged

fix: Only create mount point for EFS when using EFS #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f6e6842
fixes #254
MarkIannucci Feb 18, 2022
e297d6b
fixup formatting
MarkIannucci Feb 18, 2022
e7146f3
add ephemeral storage example
MarkIannucci Feb 20, 2022
1adbfb4
use the re-use approach instead
MarkIannucci Feb 20, 2022
4964ff3
after pre-commit fixes
MarkIannucci Feb 20, 2022
3d199f3
move locals to variables to hopefully improve linting situation
MarkIannucci Feb 20, 2022
519738b
merge
MarkIannucci Feb 20, 2022
7d25ac1
after pre-commit scripts
MarkIannucci Feb 20, 2022
98cbf6c
Merge branch 'master' into Fix254
antonbabenko Apr 1, 2022
bc27bb8
Removed copied example
antonbabenko Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions examples/github-complete/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,21 @@ Go to https://eu-west-1.console.aws.amazon.com/ecs/home?region=eu-west-1#/settin
| Name | Version |
|------|---------|
| <a name="requirement_terraform"></a> [terraform](#requirement\_terraform) | >= 0.13.1 |
| <a name="requirement_aws"></a> [aws](#requirement\_aws) | ~> 3.45 |
| <a name="requirement_aws"></a> [aws](#requirement\_aws) | >= 3.45 |
| <a name="requirement_github"></a> [github](#requirement\_github) | >= 4.8 |

## Providers

| Name | Version |
|------|---------|
| <a name="provider_aws"></a> [aws](#provider\_aws) | ~> 3.45 |
| <a name="provider_aws"></a> [aws](#provider\_aws) | >= 3.45 |

## Modules

| Name | Source | Version |
|------|--------|---------|
| <a name="module_atlantis"></a> [atlantis](#module\_atlantis) | ../../ | n/a |
| <a name="module_atlantis_access_log_bucket"></a> [atlantis\_access\_log\_bucket](#module\_atlantis\_access\_log\_bucket) | terraform-aws-modules/s3-bucket/aws | ~> 2 |
| <a name="module_atlantis_access_log_bucket"></a> [atlantis\_access\_log\_bucket](#module\_atlantis\_access\_log\_bucket) | terraform-aws-modules/s3-bucket/aws | ~> 3.0 |
| <a name="module_github_repository_webhook"></a> [github\_repository\_webhook](#module\_github\_repository\_webhook) | ../../modules/github-repository-webhook | n/a |

## Resources
Expand Down
5 changes: 4 additions & 1 deletion examples/github-complete/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ module "atlantis" {
private_subnets = ["10.20.1.0/24", "10.20.2.0/24", "10.20.3.0/24"]
public_subnets = ["10.20.101.0/24", "10.20.102.0/24", "10.20.103.0/24"]

# EFS
enable_ephemeral_storage = true
Copy link
Member

@antonbabenko antonbabenko Apr 1, 2022

Choose a reason for hiding this comment

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

It is enough to have this argument in one example. I have removed the copied one.


# ECS
ecs_service_platform_version = "LATEST"
ecs_container_insights = true
Expand Down Expand Up @@ -111,7 +114,7 @@ module "github_repository_webhook" {
################################################################################
module "atlantis_access_log_bucket" {
source = "terraform-aws-modules/s3-bucket/aws"
version = "~> 2"
version = "~> 3.0"

bucket = "atlantis-access-logs-${data.aws_caller_identity.current.account_id}-${data.aws_region.current.name}"

Expand Down
1 change: 0 additions & 1 deletion examples/github-complete/terraform.tfvars.sample
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,3 @@ alb_ingress_cidr_blocks = ["x.x.x.x/32"]
github_owner = "myorg"
github_user = "atlantis"
github_token = "mygithubpersonalaccesstokenforatlantis"
allowed_repo_names = ["repo1", "repo2"]
2 changes: 1 addition & 1 deletion examples/github-complete/versions.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ terraform {
required_providers {
aws = {
source = "hashicorp/aws"
version = "~> 3.45"
version = ">= 3.45"
}

github = {
Expand Down
5 changes: 4 additions & 1 deletion main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,10 @@ resource "aws_efs_file_system" "this" {

resource "aws_efs_mount_target" "this" {
# we coalescelist in order to specify the resource keys when we create the subnets using the VPC or they're specified for us. This works around the for_each value depends on attributes which can't be determined until apply error
for_each = zipmap(coalescelist(var.private_subnets, var.private_subnet_ids), local.private_subnet_ids)
for_each = {
for k, v in zipmap(coalescelist(var.private_subnets, var.private_subnet_ids), local.private_subnet_ids) : k => v
if var.enable_ephemeral_storage == false
Copy link
Member

@bryantbiggs bryantbiggs Feb 18, 2022

Choose a reason for hiding this comment

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

Suggested change
if var.enable_ephemeral_storage == false
if var.enable_ephemeral_storage

Copy link
Contributor Author

@MarkIannucci MarkIannucci Feb 20, 2022

Choose a reason for hiding this comment

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

I created an example which github-ephemeral-storage which uses ephemeral storage and works with the code as written.

Copy link

@llamahunter llamahunter Mar 18, 2022

Choose a reason for hiding this comment

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

shouldn't this be if ! var.enable_ephemeral_storage? We only want to make this map if we need to mount EFS.

Copy link
Member

@bryantbiggs bryantbiggs Mar 18, 2022

Choose a reason for hiding this comment

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

oh thats my bad, didn't understand the variable fully. this is not great - we should have never accepted the enable_ephemeral_storage and instead it should have been enable_efs_storage which is false by default and users then opt in. reading through the module now, everything is backwards and confusing

Copy link
Contributor Author

@MarkIannucci MarkIannucci Mar 19, 2022

Choose a reason for hiding this comment

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

I struggled with how to simultaneously support ephemeral storage, EFS, and whatever Amazon calls the capacity constrained storage that Fargate provides by default.

Ultimately, I decided to try to encourage EFS adoption because it fixes #206. In doing that, I unintentionally broke ephemeral storage users (who apparently aren't too worried about #206 ).

So... Where to go from here?

I agree with @llamahunter. Merge #261 so we can fix stuff for the people who rely on the ephemeral storage and #257 so we're safer by default.

That doesn't resolve the everything is "backwards and confusing problem" (which is a pretty good way to describe it :)

Long term, I'd like to see us fix #19. Doing so would allow us to remove a lot of complexity (and cost) from this module since we wouldn't need ALB or ephemeral storage support.

Copy link
Member

@antonbabenko antonbabenko Apr 1, 2022

Choose a reason for hiding this comment

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

Not sure I understand the whole picture, but I have executed examples/github-complete with enable_ephemeral_storage set as true and false. Both cases created some resources.

@bryantbiggs Should this be merged as it is, or do we still need something implemented in this PR?

Copy link
Member

@bryantbiggs bryantbiggs Apr 1, 2022

Choose a reason for hiding this comment

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

If its deployable, lets ship it - we can continue making adjustments after

}

file_system_id = aws_efs_file_system.this[0].id
subnet_id = each.value
Expand Down