ECS infrastructure with the Flagsmith task definition #4

deltacodepl · 2023-06-01T09:33:43Z

Thanks for submitting a PR! Please check the boxes below:

I have filled in the "Changes" section below?
I have filled in the "How did you test this code" section below?

Changes

Added "flagsmith-on-ecs" subproject with Terraform code that spins up ECS Fargate test cluster with the Flagsmith basic task definition.
The project provides instant access to the Flagsmith instance under specified domain by adding the flagsmith prefix such as flagsmith.yourdomain.com

How did you test this code?

How to test this project

Register AWS Route53 Hosted Zone
for example yourdomain.com
Generate certificate with AWS Certificate Manager
with *.yourdomain.com pattern
Define your variables such as certificate arn, hosted zone domain, and other desired settings in terraform.tfvars file
```
route53_hosted_zone = "yourdomain.com"
certificate_arn = "arn:aws:acm:"
```
Generate plan for infrastructure terraform plan -out flagsmith.tfplan
Apply infrastructure by running terraform apply flagsmith.tfplan

khvn26 · 2023-06-01T10:19:44Z

This is awesome, @deltacodepl, thank you so much for your contribution! Bonus points for the docs 🔥

There are formatting issues detected in CI, any chance you could look into it?

deltacodepl · 2023-06-02T16:39:52Z

Sure thing,
Thank you for your appreciation, Kim

plumdog

I think we need to be careful about what is being promised with this code.

If it is intended to be something that a user - a beginner with Terraform and AWS - could pick up and use to deploy their production environment, then I think the bar is very high. We'd need to accommodate lots of different options (things like the database sizing, database snapshotting, fargate resource allocation options and autoscaling, VPC and CIDR options etc) and absolutely need to be very careful with the IAM policies to reduce them to be least privilege.

Or, we could make very clear that this is a prove-to-yourself-and-your-boss-that-it-works example, but nothing more. Remove all references to "production" from this code and put a big warning at the top of the README to explain that this is not production ready, and that a user who is familiar with Terraform and AWS should use this as a reference to build their own solution.

plumdog · 2023-06-06T08:22:05Z

examples/flagsmith-on-ecs/terraform/04_loadbalancer.tf

+# Listener (redirects traffic from the load balancer to the target group)
+resource "aws_alb_listener" "ecs-alb-http-listener" {
+  load_balancer_arn = aws_lb.production.id
+  port              = "443"


I think that the load balancer SG allows 443 and 80, but there's only a 443 listener.

Suggest either:

443 only on the listener, 443 only on the load balancer SG

443 on the listener that forwards to the workload, 80 on the listener that redirects to 443, 443 and 80 on the load balancer SG (probably with a comment to say that 80 is only used for redirecting to 443)

plumdog · 2023-06-06T08:27:46Z

examples/flagsmith-on-ecs/terraform/04_loadbalancer.tf

+  default_action {
+    type             = "forward"
+    target_group_arn = aws_alb_target_group.default-target-group.arn
+  }


We've seen issues on ALB+ECS workloads, where requests like curl -k https://[load balancer ip] get to the load balancer and are forwarded, and generate lots of DisallowedHost errors from Django.

For this reason, think it is preferable here to have an action that matches on the expected host name, and forward that to the ALB target group. Then the default action is 404. As it happens, here is me suggesting this as an improvement in the CDK: aws/aws-cdk#25434.

plumdog · 2023-06-06T08:29:16Z

examples/flagsmith-on-ecs/terraform/04_loadbalancer.tf

+  load_balancer_arn = aws_lb.production.id
+  port              = "443"
+  protocol          = "HTTPS"
+  ssl_policy        = "ELBSecurityPolicy-2016-08"


This should probably be ELBSecurityPolicy-TLS13-1-0-2021-06, with a comment to say that the user should ensure it is appropriate for their usage, and use a newer one if available when they are deploying, see https://docs.aws.amazon.com/elasticloadbalancing/latest/application/create-https-listener.html#describe-ssl-policies.

plumdog · 2023-06-06T08:29:52Z

examples/flagsmith-on-ecs/terraform/05_iam.tf

+          "ssmmessages:OpenControlChannel",
+          "ssmmessages:OpenDataChannel"
+        ],
+        "Resource" : "*"


Does this have to be *?

plumdog · 2023-06-06T08:30:58Z

examples/flagsmith-on-ecs/terraform/05_iam.tf

+        Action   = ["secretsmanager:GetSecretValue", "ssm:GetParameters"],
+        Resource = ["arn:aws:ssm:${var.region}:${local.AWS_ACCOUNT_ID}:parameter/${var.app_name}/"]


This has a secretsmanager:... permission action, but the resource is arn:aws:ssm:..., which looks wrong to me.

plumdog · 2023-06-06T08:33:43Z

examples/flagsmith-on-ecs/terraform/07_route53.tf

+resource "aws_route53_record" "subdomain" {
+  zone_id = data.aws_route53_zone.selected.zone_id
+  name    = "${var.app_name}.${data.aws_route53_zone.selected.name}"
+  type    = "CNAME"
+  ttl     = "300"
+  records = [aws_lb.production.dns_name]
+}


Is there a reason not to use an Alias record here?

plumdog · 2023-06-06T08:37:01Z

examples/flagsmith-on-ecs/terraform/11_rds.tf

+
+resource "aws_db_subnet_group" "production" {
+  name = "main"
+  # subnet_ids = module.vpc.public_subnets


I think this line should probably go away, as I think we'd always want to encourage using private subnets for a database.

plumdog · 2023-06-06T08:37:23Z

examples/flagsmith-on-ecs/terraform/11_rds.tf

+  password                = aws_ssm_parameter.db_password.value
+  port                    = "5432"
+  engine                  = "postgres"
+  engine_version          = "11.12"


Is there a reason not to use a newer Postgres?

plumdog · 2023-06-06T08:38:31Z

examples/flagsmith-on-ecs/terraform/auto_scaling.tf

@@ -0,0 +1 @@
+## Fargate autoscaling


Is this to be removed, or should there be some resources here?

plumdog · 2023-06-06T08:39:17Z

examples/flagsmith-on-ecs/terraform/policies/ecs-host-role-policy.json

+      "Action": [
+        "ecs:*",
+        "elasticloadbalancing:Describe*",
+        "elasticloadbalancing:DeregisterInstancesFromLoadBalancer",
+        "elasticloadbalancing:RegisterInstancesWithLoadBalancer",
+        "elasticloadbalancing:RegisterTargets",
+        "elasticloadbalancing:DeregisterTargets",
+        "cloudwatch:*",
+        "s3:*",
+        "rds:*",
+        "logs:*",
+        "elasticache:*",
+        "secretsmanager:*",
+        "ssm:*"
+      ],
+      "Resource": "*"


This is very permissive. Could this be pruned?

…ostgress 14

deltacodepl · 2023-06-13T10:08:29Z

Hi @plumdog
Thank you for the review
I have cleaned up the code, tightened IAM permissions, and removed unnecessary elements based on all your suggestions.

plumdog

LGTM 👍

Thanks for applying the changes, this looks great, thanks for the hard work.

I must confess I haven't run it though (I feel like someone Flagsmith-side should actually run this code at least once before we merge... [looks around for volunteers...])

flagsmith-on-ecs/terraform/variables.tf

khvn26 · 2023-06-21T20:55:53Z

It works, with some minor comments I've added. @deltacodepl I'll leave those for a bit of time in case you want to address them. Otherwise, I'll merge the PR and fix those myself. Again, many thanks for your work! Great job! 👍

dabeeeenster · 2023-06-21T21:14:30Z

Yep thanks again for this @deltacodepl

plumdog reviewed Jun 6, 2023

View reviewed changes

deltacodepl added 3 commits June 13, 2023 11:30

ecs infra with flagsmith task

b1f5caf

readme cosmetic change

ce73e1f

run terraform fmt

985d62d

deltacodepl force-pushed the flagsmith-on-ecs branch from 2655235 to 4586796 Compare June 13, 2023 09:37

general clean up, tightened IAM permissions, ALB listener & SG fix, P…

9721911

…ostgress 14

deltacodepl force-pushed the flagsmith-on-ecs branch from 4586796 to 9721911 Compare June 13, 2023 09:45

dabeeeenster requested review from plumdog and khvn26 June 13, 2023 10:42

plumdog approved these changes Jun 15, 2023

View reviewed changes

khvn26 previously approved these changes Jun 21, 2023

View reviewed changes

flagsmith-on-ecs/terraform/variables.tf Outdated Show resolved Hide resolved

flagsmith-on-ecs/terraform/variables.tf Show resolved Hide resolved

flagsmith-on-ecs/terraform/variables.tf Outdated Show resolved Hide resolved

set Fargate defaults, Django application settings

4571b6d

deltacodepl dismissed khvn26’s stale review via 4571b6d June 27, 2023 19:23

khvn26 self-requested a review June 28, 2023 10:22

khvn26 approved these changes Jun 28, 2023

View reviewed changes

khvn26 merged commit c38d6f9 into Flagsmith:main Jun 28, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECS infrastructure with the Flagsmith task definition #4

ECS infrastructure with the Flagsmith task definition #4

deltacodepl commented Jun 1, 2023

khvn26 commented Jun 1, 2023

deltacodepl commented Jun 2, 2023

plumdog left a comment

plumdog Jun 6, 2023

plumdog Jun 6, 2023

plumdog Jun 6, 2023

plumdog Jun 6, 2023

plumdog Jun 6, 2023

plumdog Jun 6, 2023

plumdog Jun 6, 2023

plumdog Jun 6, 2023

plumdog Jun 6, 2023

plumdog Jun 6, 2023

deltacodepl commented Jun 13, 2023

plumdog left a comment

khvn26 commented Jun 21, 2023

dabeeeenster commented Jun 21, 2023

		Action = ["secretsmanager:GetSecretValue", "ssm:GetParameters"],
		Resource = ["arn:aws:ssm:${var.region}:${local.AWS_ACCOUNT_ID}:parameter/${var.app_name}/"]

ECS infrastructure with the Flagsmith task definition #4

ECS infrastructure with the Flagsmith task definition #4

Conversation

deltacodepl commented Jun 1, 2023

Changes

How did you test this code?

How to test this project

khvn26 commented Jun 1, 2023

deltacodepl commented Jun 2, 2023

plumdog left a 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

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

deltacodepl commented Jun 13, 2023

plumdog left a comment

Choose a reason for hiding this comment

khvn26 commented Jun 21, 2023

dabeeeenster commented Jun 21, 2023