Skip to content
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

Incomplete first pass of underpass new infrastructure #472

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dakotabenjamin
Copy link
Member

@dakotabenjamin dakotabenjamin commented Feb 22, 2024

DO NOT MERGE

I wanted to put out some progress and outline some blockers I still need to address before we can say its in a deployable state:

  • @eternaltyro for the peering connection, i need a little guidance on setting the cidr_block param. should that come from the vpc module or be defined internally?
  • the major incomplete part is the api ECS. plan is to use this module: https://gitlab.com/eternaltyro/terraform-aws-ecs
  • should we update the route53 zone to underpass.hotosm.org?
  • there were 2 file processor blocks. idk why
  • there are few todo's ive added for random things, as well as todos from before

Copy link
Contributor

@eternaltyro eternaltyro left a comment

Choose a reason for hiding this comment

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

All good except for a few changes I recommend. But even without, this changeset is good to go.

container_min_count = 1
container_max_count = 5
}
service_subnets = module.vpc.public_subnets
Copy link
Contributor

Choose a reason for hiding this comment

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

This will create the containers in public subnets. No change if this is what you want. But the module does not assign public IP to the containers. Yet.

If they are going to be behind a load balancer, it is safe to put them in private subnets.

}
service_subnets = module.vpc.public_subnets
aws_vpc_id = module.vpc.vpc_id
service_security_groups = [module.vpc.default_security_group_id] // update for update
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the two security groups output by the ALB module - load_balancer_app_security_group can be added here directly.

service_subnets = module.vpc.public_subnets
aws_vpc_id = module.vpc.vpc_id
service_security_groups = [module.vpc.default_security_group_id] // update for update
alb_security_group = module.alb.load_balancer_app_security_group //public or private
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not required. I have removed it from the module.

Copy link
Member Author

Choose a reason for hiding this comment

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

ope, yeah missed this.

data "aws_acm_certificate" "wildcard" {
domain = "hotosm.org"
statuses = ["ISSUED"]
most_recent = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add additional line for reliability:

key_types = ["RSA_2048", "RSA_3072", "RSA_4096", "EC_prime256v1", "EC_secp384r1", "EC_secp521r1"]

AWS API by default only returns RSA certs unless you give it a list of cert types to consider. Granted we have RSA cert for our home domain. But this prepares for future.

Comment on lines +251 to +272
resource "aws_secretsmanager_secret_version" "configfile" {
secret_id = aws_secretsmanager_secret.configfile.id
secret_string = base64encode(
templatefile(
"${path.module}/config.txt.tftpl",
{
underpass_db_creds = var.underpass_db_credentials

oauth2_creds = var.oauth2_credentials

api_url = "${var.api_url_scheme}${var.api_host}"
api_port = var.api_port
api_export_max_area = var.api_export_max_area
api_log_level = var.api_log_level

sentry_dsn = lookup(var.sentry, "dsn")
sentry_app_environment = var.deployment_environment
sentry_app_release_tag = "underpass-api@${var.container_image_tag}"
}
)
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. We need app releases to be done by terraform for the sentry tags to be set properly. Or we need to at least run terraform after every deployment.

@@ -123,7 +125,7 @@ resource "aws_security_group" "database" {
from_port = 5432
to_port = 5432
protocol = "tcp"
security_groups = [aws_security_group.api.id, aws_security_group.app.id]
security_groups = [module.vpc.default_security_group_id]
Copy link
Contributor

Choose a reason for hiding this comment

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

This ingress block can be removed. The self ingress block alone should be sufficient. We can simply add this security_group_id as one of the service security groups in the ECS service.

Role = "Database server"
}

}

resource "aws_db_proxy" "galaxy" {
name = "galaxy"
resource "aws_db_proxy" "underpass" {
Copy link
Contributor

Choose a reason for hiding this comment

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

If app and DB are in the same VPC we can skip creating the proxy resource.

resource "aws_db_proxy_target" "underpass" {
db_instance_identifier = aws_db_instance.underpass.id
db_proxy_name = aws_db_proxy.underpass.name
target_group_name = aws_db_proxy_default_target_group.underpass.name
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can skip all other proxy related resources if we skip the proxy resource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants