-
Notifications
You must be signed in to change notification settings - Fork 8
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Route53 support? #123
Comments
Hello @shinenelson !
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.
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 |
Hey @rpdelaney, The ACM certificates and CloudWatch metric alarms were more of an extra ask. The primary ask was to
Let me explain this a little further : Currently, the module accepts Similarly, we could ask for a Previously, we were dictating a convention in a particular format terraform-aws-alb-web-containers/main.tf Line 31 in 4bc0a3a
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 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. |
I think I understand the ask, except I'm not sure how this wouldn't fall into the pit trap I mentioned:
You also said:
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. |
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 :
for the above inputs, if I provide the following values :
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 :
problems I can think of :
( Disclaimer : I am projecting all this in my head. I have not tested this with code myself ) |
@esacteksab What do you think? Am I off base? |
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.
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 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 So I would support a PR that put this back with a conditional (e.g 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. |
I think that would address the objection I was raising. Thanks :) |
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 馃
I am assuming that the previous implementation had a specific format for the sub-domain
terraform-aws-alb-web-containers/main.tf
Line 31 in 4bc0a3a
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,
HTTPCode_ELB_5xx_Count
andHTTPCode_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.
The text was updated successfully, but these errors were encountered: