Skip to content

feat: allow for extra_container_definitions #162

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

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

marcoceppi
Copy link
Contributor

Description

This expands the options by allowing users to add supplimentary container definitions in addition to the existing replacement of all container definitions.

The current limit is var.ecs_task_cpu is used for both the atlantis container definition and the task definition which means there's no room for any additional container definitions to use CPU. This is okay for me, as my sidecar exits 0 after writing files to the shared volume, but I may add a var.ecs_container_cpu which will default to ecs_task_cpu if it's not set, but will allow an author to expand the task CPU limits and the container limits independently.

Motivation and Context

This allows me, in particular, to provide additional sidecars while still using the recommended container definitions. It primarily addresses #133

Breaking Changes

This is a supplementary change and should not break compatibility

How Has This Been Tested?

This has been tested today using a very similar template below:

module "container_definition_atlantis_sidecar" {
  source  = "cloudposse/ecs-container-definition/aws"
  version = "v0.40.0"

  container_name  = "atlantis_sidecar"
  container_image = data.aws_ecr_repository.atlantis_sidecar.repository_url

  container_cpu                = 0
  container_memory             = 64
  container_memory_reservation = 64

  readonly_root_filesystem = true

  essential = false

  secrets = [
    {
      name = "scrubed",
      valueFrom = data.aws_secretsmanager_secret.scrubed.arn,
    }
  ]
}

module "atlantis" {
  source = "github.com/stacklet/terraform-aws-atlantis?ref=issue-133"

  name = "atlantis"

  cidr            = "x.x.x.x/x"
  azs             = ["us-east-2a", "us-east-2b", "us-east-2c"]
  private_subnets = ["x.x.x.x/x", "x.x.x.x/x", "x.x.x.x/x"]
  public_subnets  = ["x.x.x.x/x", "x.x.x.x/x", "x.x.x.x/x"]

  route53_zone_name = "xxx.yyy"

  atlantis_github_user           = "marcoceppi"
  atlantis_github_user_token     = data.aws_kms_secrets.atlantis.plaintext["github_user_token"]

  extra_container_definitions = [module.container_definition_atlantis_sidecar.json_map_object]

  allow_unauthenticated_access = true
  allow_github_webhooks        = true
  allow_repo_config            = true

  custom_environment_variables = [
    {
      name  = "AWS_CONFIG_FILE",
      value = "/data/scrubed"
    },
    {
      name  = "AWS_SDK_LOAD_CONFIG",
      value = "1",
    }
  ]

  volumes_from = [
    {
      sourceContainer = "atlantis_sidecar",
      readOnly = true,
    }
  ]
}

@marcoceppi marcoceppi force-pushed the issue-133 branch 2 times, most recently from 2d60cdf to e43cdc5 Compare October 29, 2020 04:38
@marcoceppi
Copy link
Contributor Author

@antonbabenko any chance I could get some feedback on this?

@antonbabenko
Copy link
Member

@marcoceppi I will look into this in detail and comment/merge on Friday.

@antonbabenko
Copy link
Member

There is no need to have another variable just for this because there is already a variable you can use for this.

Here is the example code:

module "main_container_definition" {
  source  = "cloudposse/ecs-container-definition/aws"
  version = "v0.40.0"

  container_name  = "atlantis"
  container_image = "atlantis/atlantis:latest"

  container_cpu                = 1024
  container_memory             = 1024
  container_memory_reservation = 512

  port_mappings = [
    {
      containerPort = 4141
      hostPort      = 4141
      protocol      = "tcp"
    },
  ]
}

module "sidecar_container_definition" {
  source  = "cloudposse/ecs-container-definition/aws"
  version = "v0.40.0"

  container_name  = "sidecar"
  container_image = "sidecar/sidecar:latest"
}

module "atlantis" {
  source = "........"

  #######
  custom_container_definitions = "[${module.main_container_definition.json_map_encoded}, ${module.sidecar_container_definition.json_map_encoded}]"

  #######
}

@marcoceppi
Copy link
Contributor Author

I'm aware the option to override all container definitions. However, I don't think it's a very good configuration option - especially for this use case.

First, it requires authors to understand how the atlantis ecs-definition was created - which I'd argue is the main allure to this module. It also means that users now need to track changes to the atlantis definition as subsequent upgrades / configurations / changes to this module that might fix bugs, address security concerns, or instrument newer best practices for running Atlantis would be lost because the user as forced to re-define the definition.

Furthermore, the custom_container_definitions effectively nerfs a large portion of the existing configuration for this module, rendering them no-ops on the module and requiring users to re-instrument them outside of the module (managing source control secrets, environment variables, atlantis configuration flags, log configuration, etc).

Hopefully you can see that these two options, while similar, produce a vastly different outcome.

@antonbabenko antonbabenko merged commit aaa9f9c into terraform-aws-modules:master Nov 13, 2020
@antonbabenko
Copy link
Member

I agree that your arguments are very valid. custom_container_definitions effectively nerfs a large portion of the existing configuration for this module :)

v2.26.0 has been just released.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants