search

terraform-aws-modules / terraform-aws-atlantis

Terraform module to deploy Atlantis on AWS Fargate 🇺🇦
https://registry.terraform.io/modules/terraform-aws-modules/atlantis/aws
Apache License 2.0
520 stars 350 forks source link

Using an old version of terraform-aws-modules/ecs/aws #297

Closed llamahunter closed 12 months ago

llamahunter commented 2 years ago

Description

When running using aws provider v4+, a warning is listed that deprecated resources are being used

Versions

Reproduction Code [Required]

Steps to reproduce the behavior:

apply a use of the atlantis module

Expected behavior

No warnings

Actual behavior

Warnings from use of the ecs submodule

Terminal Output Screenshot(s)

│ Warning: Argument is deprecated │ │ with module.atlantis.module.ecs.aws_ecs_cluster.this, │ on .terraform/modules/atlantis.ecs/main.tf line 1, in resource "aws_ecs_cluster" "this": │ 1: resource "aws_ecs_cluster" "this" { │ │ Use the aws_ecs_cluster_capacity_providers resource instead │ │ (and 4 more similar warnings elsewhere)

Additional context

Current module is pinned to use v3.3.0 of the ecs module, but the current version is v4.1.0

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

llamahunter commented 2 years ago

Still a problem. See https://github.com/terraform-aws-modules/terraform-aws-atlantis/blob/master/main.tf#L488

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

llamahunter commented 2 years ago

still an issue

snovikov commented 2 years ago

Still an issue for me as well

bryantbiggs commented 2 years ago

this will be updated once the new ECS service sub-module lands https://github.com/terraform-aws-modules/terraform-aws-ecs/tree/master/modules/service#ecs-service-module - should be before the end of the year

bryantbiggs commented 1 year ago

once https://github.com/terraform-aws-modules/terraform-aws-ecs/pull/76 lands I will update the project here with those associated changes

jseiser commented 1 year ago

@bryantbiggs

I believe the MR you indicated you were waiting on, has merged.

bryantbiggs commented 1 year ago

Sharing some of my thoughts on proposed changes to get feedback from the group (and hopefully convince @antonbabenko to let us drop the bundled VPC module from this project 😬 ). This is all very rough at this point to align on the main points, some of the finer details may shift as we develop, test, and validate the changes to reach the desired functionality.

1. Remove VPC

Instead, users can utilize an existing VPC (what I suspect is the most common scenario) that is created elsewhere, or they can add the VPC module to supplement this functionality. VPCs are massively complex and its not practical to re-expose every bit of configuration that the underlying module provides - in the past we had said we would draw a line and not accept any further changes to expose additional functionality provided by the VPC module but we have not adhered to this. More importantly, I do not believe it provides a reasonable pattern - a VPC is a container that is used by many different services which makes it an unlikely candidate to be provisioned and controlled from within an "application" type module. This should be a rather minor change since we would be swapping out like for like which should result in only one Terraform state move command (TBD - just thinking outloud here)

Remove

Add

2. Replace ALB with API Gateway

ALBs are not cheap nor are we really flexing the full extent of what an ALB provides - we're merely using it as the means to field webhook requests down to the Atlantis task/container. It turns out that we can utilize API Gateway to route traffic into a private ECS task - here is a quick and dirty example of this setup https://github.com/clowdhaus/api-gateway-ecs-razzle-dazzle/tree/main. In the end, the proposal would be to replace the ALB with the API Gateway module; users would still be able to disable this and route traffic to Atlantis via their externally created ALB or an externally created API Gateway. Some more testing and validation needs to be checked here first - specifically with regards to the Atlantis UI under the API Gateway setup.

Remove

Add

tl;dr on why - cheaper and less configuration when compared to ALB, but users still have ability to use external ALB or API Gateway

3. Replace EFS resources with EFS Module

Since the Atlantis module was first created, we have added an EFS module. This helps clean up and simplify the resources down into the module, exposing a handful of variables for configuration. While making this replacement, we will look at properly addressing the task storage (re: past issues) to ensure setting the storage requirements, wither ephemeral, EFS, or none, is more seamless and intuitive.

Remove

Add

4. Consolidate ECS resources

This is largely composed of two parts:

  1. Replace the ECS module with the new ECS cluster sub-module. This matches nearly like-for-like functionality - allowing users to create a new ECS cluster for Atlantis, or use an existing ECS cluster
  2. Replace the ECS standalone resources and container definition module with the new ECS service sub-module. This bundles most of the functionality that is provided in this module which simplifies and standardizes the interface presented to users.

Before we make this change, we should update the ECS module to align with v5.0 of the AWS provider since there are changes that affect these resources. This should provide stability for at least 12 months by the provider

Remove

Add

5. Simplify secrets & environment variables

Due to the number of source control providers that Atlantis integrates with (GitHub, GitLab, Bitbucket, etc.) and the various possible forms of secret management - secret and environment variable management under the current implementation has become quite complex and untenable to manage. This poses challenges when users attempt to integrate the module with other source control providers. It also poses challenges in terms of recommended practices regarding secret management and storage.

Through the changes listed above regarding changes to leverage the latest modules available, we can start to make the module interface more generic and allow users to select how they wish to manage and store secrets as well as select which source control provider they integrate with without requiring code changes within this module. To achieve this, we will need to provide better documentation on how users can achieve some of these various configurations to suite their specific needs but the end result will be a simpler module that is more easily applied to a number of different user scenarios.

Remove

Add

6. Misc

Those are my initial thoughts - open to hear feedback, questions, comments though!

jseiser commented 1 year ago
  1. Remove VPC

The VPC should 100% be dropped in this module. Its never going to be as maintained/flexible as the main VPC Module.

  1. Replace ALB with API Gateway

My concern here, is that API Gateway has its own set of limitations in AWS Govcloud, which is where all of our atlantis deployments run. I would not be opposed to just removing the ingress configuration all together, and requiring the user to deploy an ALB module or an API Gateway module. As long as there is a way to route the UI to okta or keycloak etc, and the /events webhook can be public but restricted by a security group or something similar, it doesnt really matter to me.

  1. Replace EFS resources with EFS Module I agree with this, for the same reason as the #1.

  2. Consolidate ECS resources I agree with this.

for 5/6 i really dont have a deep enough understanding of the problem trying to be solved. I will say, we run Atlantis in kubernetes, via the helm chart against Gitlab, and then in 1 Govcloud env we run Atlantis via this module against govcloud. This module was 100% more complicated to parse, than the helm chart was with respect to the gitlab specific configurations.

I would also like to state, there should be little apprehension with making a breaking change here, since a destroy/re-deploy of atlantis is not a huge deal.

bryantbiggs commented 1 year ago

I would also like to state, there should be little apprehension with making a breaking change here, since a destroy/re-deploy of atlantis is not a huge deal.

100% - great point I forgot to mention. Since the remote state and any locking is handled outside of this module, the risk during changes is very low. It is primarily centered around maintaining and translating functionality between versions

antonbabenko commented 1 year ago

I did not become an experienced user of Atlantis since I published this module a few years ago, so I don't want to sound like I know how it can be deployed or used efficiently. Still, here are my 5 cents :)

  1. This module had support for optionally creating VPC resources. In my installation, I use existing VPC. I think it is not harmful to continue to have a way to provision VPC resources by the module. I believe, it can be rather handy to call one module and get everything provisioned in some cases (e.g. demos, small-scale setups, POC, "with single terragrunt.hcl config file").

2-6. Sounds like a good path forward. Looking forward to seeing the usage of other Terraform AWS modules by this one!

dynamike commented 1 year ago

I think this is an interesting path to take. It seems like the original intention with the module is to cover all the bootstrapping bits to get Atlantis up and running without much peripheral components (e.g., everything's in the module). The trouble is the existing implementation is a bit unwieldy as you've covered :). The trade off is the changes suggested above will make it slightly more cumbersome to bootstrap Atlantis to Anton's point, but we gain a more customizable module to allow Atlantis to run in more diverse environments.

Generally 👍 as I think the benefits out way the negatives especially from a maintainability standpoint.

jseiser commented 1 year ago

I assume this issue, is directly related to this: https://github.com/terraform-aws-modules/terraform-aws-atlantis/issues/349

jseiser commented 1 year ago

@bryantbiggs

Any decisions made on this one?

jseiser commented 1 year ago

@bryantbiggs

This module is holding back our provider upgrades for one specific environment.

Trying to determine if we should just re-write this all ourselves using the standard modules available, or if you have a plan for this one? If you have a plan for this one, I would def. be able to assist.

bryantbiggs commented 1 year ago

I am working on it - I need to update the ALB module first (https://github.com/terraform-aws-modules/terraform-aws-alb/issues/289#issuecomment-1684449408) and then we can finish this upgrade here

bryantbiggs commented 12 months ago

Ok here is an initial draft - still working through a lot of the finer details, but I think the direction is mostly there

https://github.com/terraform-aws-modules/terraform-aws-atlantis/compare/master...bryantbiggs:terraform-aws-atlantis:refactor/ecr-module?expand=1

Some of the larger'ish ToDos still:

But it would be great to hear any initial feedback

jseiser commented 12 months ago

We use gitlab, so I can probably provide some docs for that

I wouldn't worry about a migration, there is no state, the biggest issue is redoing your webhooks to point to this new url

On Thu, Nov 2, 2023, 8:08 PM Bryant Biggs @.***> wrote:

Ok here is an initial draft - still working through a lot of the finer details, but I think the direction is mostly there

https://github.com/terraform-aws-modules/terraform-aws-atlantis/compare/master...bryantbiggs:terraform-aws-atlantis:refactor/ecr-module?expand=1

Some of the larger'ish ToDos still:

  • Ensure the BYO routes are supported - BYO cluster and ALB
  • Need to figure out what to do with other git providers - ideally these would just be shown in examples but I don't think we maintainers use BitBucket, GitLab, etc. so that might be a challenge to host those. Also not sure if there is benefit other than showing the the environment variables and webhook configs which seems rather trivial. Leaning towards - "Should work, we don't demonstrate how to do that here"
  • Not sure what to even put on migration guide nor the value of that at this point. Its a significant amount of work and not sure how practical it would be, more inclined to provide docs and guidance on "Heres how you configure x and y with this module" and guide users to tear down current setup and re-deploy with new setup
  • Doc updates

But it would be great to hear any initial feedback

— Reply to this email directly, view it on GitHub https://github.com/terraform-aws-modules/terraform-aws-atlantis/issues/297#issuecomment-1791725320, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFBNZYRFC3KDYEA5YQHFHLYCQYWNAVCNFSM52HJOSVKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZZGE3TENJTGIYA . You are receiving this because you commented.Message ID: @.*** .com>

antonbabenko commented 12 months ago

This issue has been resolved in version 4.0.0 :tada:

github-actions[bot] commented 11 months ago

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.