Route53 support?

[shinenelson]

I found that domain support was removed in commit 4bc0a3a, but since there was no context in the commit message, I was wondering if there was some reason for removing the support for domains for the web containers :thinking:

I am assuming that the previous implementation had a specific format for the sub-domain https://github.com/trussworks/terraform-aws-alb-web-containers/blob/4bc0a3a0f8546ca415540871fd38ba0187110dc5/main.tf#L31 which did not align with generalized use of the module.

But I think it would be useful if we included a provision to use a route53 domain for a web service. It is probably a missing part of a web service module. The implementation could be similar to how the module accepts variables for the ALB certificates.

If you are open to it, I could come up with a pull request for it.

---

Personal wish-list : I would prefer if,

1. we created the Amazon Certificate Manager ( ACM ) certificates for the domains as well, but I am okay with passing an Amazon Resource Name ( ARN ) too ( because my use-case uses a wildcard certificate for the whole sub-domain :smile: )
2. we created basic cloudwatch metric alarms - HTTPCode_ELB_5xx_Count and HTTPCode_Target_5xx_Count; but I understand that different people might have their own metrics, conditions and thresholds.

PS : These are holding me back from using this module directly in my project and not having to redefine the whole module.

[rpdelaney]

Hello @shinenelson !

we created the Amazon Certificate Manager ( ACM ) certificates for the domains as well

IIRC we took out the Route53 stuff because including it in the module means Terraform would destroy the DNS record / ACM cert any time the other resources defined in this module are replaced, and that's not a problem we want to create by supporting it. :) We find that clearly separating concerns gives more granular control: for instance, we have a separate module for terraform-aws-acm-cert that dovetails with this one.

we created basic cloudwatch metric alarms ... I understand that different people might have their own metrics, conditions and thresholds.

Yeah... that would be convenient, but as you mentioned, any defaults in the module would always be wrong.

I'm going to close this issue as WONT-DO, but if I've misunderstood something please feel free to continue the conversation. We appreciate the feedback :)

[shinenelson]

Hey @rpdelaney,

The ACM certificates and CloudWatch metric alarms were more of an extra ask.

The primary ask was to

include a provision to use a route53 domain for a web service. It is probably a missing part of a web service module. The implementation could be similar to how the module accepts variables for the ALB certificates.

Let me explain this a little further :

Currently, the module accepts alb_default_certificate_arn as an input. We trust the user to provide a valid ACM certificate ARN.

Similarly, we could ask for a zone_name ( like we used to previously ), and create the route53 record directly into the the zone.

Previously, we were dictating a convention in a particular format https://github.com/trussworks/terraform-aws-alb-web-containers/blob/4bc0a3a0f8546ca415540871fd38ba0187110dc5/main.tf#L31

What I was suggesting was to put all of the route53 code back, but instead of the module dictating the structure of the (sub-)domain, we make it user-configurable. We would have to trust that the user provides the right sub-domain that corresponds to the provided zone_name. And also expect the sub-domain to be covered by the ACM certificate that is being passed into the module.

We could document all of these requirements; but since these are user inputs, it is bound to fail due to human error as well.

I hope this clarifies the ask.

[rpdelaney]

I think I understand the ask, except I'm not sure how this wouldn't fall into the pit trap I mentioned:

including it in the module means Terraform would destroy the DNS record / ACM cert any time the other resources defined in this module are replaced

You also said:

since these are user inputs, it is bound to fail due to human error as well.

We generally aren't concerned with protecting users from their own "error", since we peer review Terraform plans before making changes, especially in production. Simple validation rules like type checking are useful, but it's just not a concern for what you're suggesting here.

[shinenelson]

I think I understand the ask, except I'm not sure how this wouldn't fall into the pit trap I mentioned:

including it in the module means Terraform would destroy the DNS record / ACM cert any time the other resources defined in this module are replaced

I do not think terraform would replace the DNS record unless the sub-domain itself is changed ( that should be expected for obvious reasons ). But it will not be updated if, for example, the load balancer changes.

inputs :

• domain_resource_record
• zone_name

for the above inputs, if I provide the following values :

• domain_resource_record = "test"
• zone_name = "example.com"

---

If we put the route53 code back, the module would have the following route53 resource :

data "aws_route53_zone" "main" {
  name = var.zone_name
}

resource "aws_route53_record" "sub_domain" {
  zone_id = data.aws_route53_zone.main.zone_id
  name    = var.domain_resource_record
  type    = "CNAME"
  ttl     = 300
  records = [aws_lb.main.dns_name]
}

this would translate to :

resource "aws_route53_record" "sub_domain" {
  zone_id = "Z1079234UK5WC3T9CAWS"
  name    = "test"
  type    = "CNAME"
  ttl     = 300
  records = ["service-test-635942310.us-east-1.elb.amazonaws.com"]
}

---

scenarios :

1. If the load balancer itself is replaced, then this resource will have an update ( not a replace ) to the record value with the new DNS name of the load balancer
2. if domain_resource_record value is updated, that is a conscious decision by the user to replace the record.
3. if zone_name is updated - that again would be the responsibility of the user to keep track of. Also, that update is triggered by the same resource itself, not another resource changing within this module.

problems I can think of :

• The hosted zone for zone_name might not be declared in the AWS account that is being run.
• The ACM certificates passed in through alb_default_certificate_arn ( or alb_default_certificate_arns ) does not cover ${var.domain_resource_record}.${var.zone_name}.

( Disclaimer : I am projecting all this in my head. I have not tested this with code myself )

[rpdelaney]

@esacteksab What do you think? Am I off base?

[esacteksab]

I agree with @shinenelson here. I think there is value in this existing. These scenarios are valid in my opinion and with regards to the first one listed, my experience.

If the load balancer itself is replaced, then this resource will have an update ( not a replace ) to the record value with the new DNS name of the load balancer if domain_resource_record value is updated, that is a conscious decision by the user to replace the record. if zone_name is updated - that again would be the responsibility of the user to keep track of. Also, that update is triggered by the same resource itself, not another resource changing within this module.

I have no concerns about disturbing a zone by managing a record here. I don't have an issue with destroying or recreating a record that is attached to the alb. And should the ALB be destroyed to never be brought back up and a person wanted the route53 record, they could import it elsewhere or do what's appropriate for them to preserve it, otherwise, because of it's association with the ALB and the ALB is going away, so should the record in my opinion.

With regards to the cloud watch request, I'd follow a pattern we use here and make the checks optional. I think 400's and 500's are common checks. But I do agree with @rpdelaney whatever value we choose will likely be "wrong" because every use case is unique. So I would set their values to "" and require the person to insert a value based on their needs, usage, patterns.

Like the cloudwatch metrics, I would put the DNS record creation behind a bool to enable it. I see value in it existing, but I feel like folks should not have to use it if they don't want to or if they're managing their records in a different way and this gets in their way. The default value should be false as folks should explicitly choose to create and manage the record here because they're not managing DNS somewhere else.

So I would support a PR that put this back with a conditional (e.g count = var.create_route53_record ? 1 : 0) with the metrics also behind conditionals (e.gcount = var.cloudwatch_400_errors ? 1 : 0 and count = var.cloudwatch_500_errors ? 1 : 0)

The variable names are purely suggestions for the example in this conversation. You do not have to use my convention here if you feel like you have better names.

Since @rpdelaney closed this, I'll let him reopen it if he agrees.

[rpdelaney]

The default value should be false as folks should explicitly choose to create and manage the record here because they're not managing DNS somewhere else.

I think that would address the objection I was raising. Thanks :)