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

MarkIannucci · 2022-02-18T12:35:50Z

Description

Fixes #254 by adding a conditional to the mount so we only create the mount when we are adding storage for EFS.

How Has This Been Tested?

I have tested and validated these changes using one or more of the provided examples/* projects

~~Needs an example which I haven't completed yet.~~

bryantbiggs · 2022-02-18T12:38:19Z

main.tf

-  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


Suggested change

if var.enable_ephemeral_storage == false

if var.enable_ephemeral_storage

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

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

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

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.

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?

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

llamahunter · 2022-03-18T22:36:25Z

what's the status of this PR? Seems unmerged for nearly a month after approval. In the meantime, ephemeral storage doesn't work at all.

dgokcin · 2022-04-01T12:37:27Z

ephemeral storage still has problems. I really want to try atlantis and I am blocked because of this issue. Here is my error message

Here is my basic terragrunt config

any help is appreciated.

antonbabenko

It works!

@bryantbiggs WDYT?

antonbabenko · 2022-04-01T15:08:43Z

examples/github-complete/main.tf

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


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

antonbabenko · 2022-04-01T15:11:21Z

main.tf

-  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


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?

### [3.13.1](v3.13.0...v3.13.1) (2022-04-01) ### Bug Fixes * Only create mount point for EFS when using EFS ([#261](#261)) ([b1b61f3](b1b61f3))

antonbabenko · 2022-04-01T15:21:50Z

This PR is included in version 3.13.1 🎉

dgokcin · 2022-04-02T13:21:04Z

@antonbabenko I am still having efs related problems after the fixes that are done yesterday. Here is my config file

include {
  path = find_in_parent_folders()
}

locals {
  common_vars                                     = yamldecode(file(find_in_parent_folders("common_vars.yaml")))
  hosted_zone                                     = "example.com"
  certificate_arn                                 = "arn:aws:acm:eu-west-1:141757035671:certificate/52bbc00f-857c-45dd-b8ea-20bb8dfbcfec"
  alb_ingress_cidr_blocks                         = "MY_EXTERNAL_IP_ADDRESS/32"
}

terraform {
  source = "../../..//modules/terraform-aws-atlantis"
}

inputs = {
  name = "atlantis"

  vpc_id             = dependency.vpc.outputs.vpc_id
  private_subnet_ids = dependency.vpc.outputs.private_subnets
  public_subnet_ids  = dependency.vpc.outputs.public_subnets


  # DNS
  create_route53_record = false
  route53_zone_name = local.hosted_zone
  certificate_arn   = local.certificate_arn

  # Atlantis
  atlantis_github_user       = "atlantis-bot"
  atlantis_github_user_token = "examplegithubtoken"
  atlantis_repo_allowlist    = ["github.com/MyGitHubOrg/*"]

  # EFS
  enable_ephemeral_storage   = false

  allow_unauthenticated_access = true
  allow_github_webhooks        = true

  # alb
  alb_ingress_cidr_blocks         = local.alb_ingress_cidr_blocks


  tags = local.common_vars.tags
}

dependency "vpc" {
  config_path = "../vpc"
}

dependency "alb" {
  config_path = "../alb/main"
}

As you see, I have the latest updates. Any suggestions on what might I be missing?

bryantbiggs · 2022-04-02T13:24:12Z

@dgokcin please file a new issue with a complete reproduction (i.e. - without your terragrunt and other specifics, start with the examples provided in the project and modify to reproduce your issue)

…dules#261) Co-authored-by: Anton Babenko <[email protected]>

### [3.13.1](terraform-aws-modules/terraform-aws-atlantis@v3.13.0...v3.13.1) (2022-04-01) ### Bug Fixes * Only create mount point for EFS when using EFS ([terraform-aws-modules#261](terraform-aws-modules#261)) ([b1b61f3](terraform-aws-modules@b1b61f3))

…dules#261) Co-authored-by: Anton Babenko <[email protected]>

### [3.13.1](terraform-aws-modules/terraform-aws-atlantis@v3.13.0...v3.13.1) (2022-04-01) ### Bug Fixes * Only create mount point for EFS when using EFS ([terraform-aws-modules#261](terraform-aws-modules#261)) ([b1b61f3](terraform-aws-modules@b1b61f3))

…dules#261) Co-authored-by: Anton Babenko <[email protected]>

### [3.13.1](terraform-aws-modules/terraform-aws-atlantis@v3.13.0...v3.13.1) (2022-04-01) ### Bug Fixes * Only create mount point for EFS when using EFS ([terraform-aws-modules#261](terraform-aws-modules#261)) ([b1b61f3](terraform-aws-modules@b1b61f3))

github-actions · 2022-11-08T02:34:12Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

MarkIannucci added 2 commits February 18, 2022 05:04

fixes terraform-aws-modules#254

f6e6842

fixup formatting

e297d6b

MarkIannucci changed the title ~~Fix: only create mount point for EFS when using EFS~~ fix: only create mount point for EFS when using EFS Feb 18, 2022

bryantbiggs reviewed Feb 18, 2022

View reviewed changes

MarkIannucci changed the title ~~fix: only create mount point for EFS when using EFS~~ fix: Only create mount point for EFS when using EFS Feb 18, 2022

MarkIannucci added 6 commits February 20, 2022 00:42

add ephemeral storage example

e7146f3

use the re-use approach instead

1adbfb4

after pre-commit fixes

4964ff3

move locals to variables to hopefully improve linting situation

3d199f3

merge

519738b

after pre-commit scripts

7d25ac1

MarkIannucci requested a review from bryantbiggs February 20, 2022 08:40

aairey approved these changes Feb 22, 2022

View reviewed changes

antonbabenko added 2 commits April 1, 2022 16:30

Merge branch 'master' into Fix254

98cbf6c

Removed copied example

bc27bb8

antonbabenko reviewed Apr 1, 2022

View reviewed changes

antonbabenko merged commit b1b61f3 into terraform-aws-modules:master Apr 1, 2022

antonbabenko pushed a commit that referenced this pull request Apr 1, 2022

chore(release): version 3.13.1 [skip ci]

43a0f71

### [3.13.1](v3.13.0...v3.13.1) (2022-04-01) ### Bug Fixes * Only create mount point for EFS when using EFS ([#261](#261)) ([b1b61f3](b1b61f3))

titanlien pushed a commit to Flaconi/terraform-aws-atlantis that referenced this pull request Jun 8, 2022

fix: Only create mount point for EFS when using EFS (terraform-aws-mo…

c5acbed

…dules#261) Co-authored-by: Anton Babenko <[email protected]>

titanlien pushed a commit to Flaconi/terraform-aws-atlantis that referenced this pull request Jun 8, 2022

fix: Only create mount point for EFS when using EFS (terraform-aws-mo…

561749f

…dules#261) Co-authored-by: Anton Babenko <[email protected]>

titanlien pushed a commit to Flaconi/terraform-aws-atlantis that referenced this pull request Jun 9, 2022

fix: Only create mount point for EFS when using EFS (terraform-aws-mo…

fad46cf

…dules#261) Co-authored-by: Anton Babenko <[email protected]>

github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MarkIannucci commented Feb 18, 2022 •

edited

Loading

bryantbiggs Feb 18, 2022

MarkIannucci Feb 20, 2022

llamahunter Mar 18, 2022

bryantbiggs Mar 18, 2022

MarkIannucci Mar 19, 2022

antonbabenko Apr 1, 2022

bryantbiggs Apr 1, 2022

llamahunter commented Mar 18, 2022

dgokcin commented Apr 1, 2022

antonbabenko left a comment

antonbabenko Apr 1, 2022

antonbabenko Apr 1, 2022

antonbabenko commented Apr 1, 2022

dgokcin commented Apr 2, 2022

bryantbiggs commented Apr 2, 2022 •

edited

Loading

github-actions bot commented Nov 8, 2022

	if var.enable_ephemeral_storage == false
	if var.enable_ephemeral_storage

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

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

Conversation

MarkIannucci commented Feb 18, 2022 • edited Loading

Description

How Has This Been Tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llamahunter commented Mar 18, 2022

dgokcin commented Apr 1, 2022

antonbabenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonbabenko commented Apr 1, 2022

dgokcin commented Apr 2, 2022

bryantbiggs commented Apr 2, 2022 • edited Loading

github-actions bot commented Nov 8, 2022

MarkIannucci commented Feb 18, 2022 •

edited

Loading

bryantbiggs commented Apr 2, 2022 •

edited

Loading