Comments (7)
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 :)
from terraform-aws-alb-web-containers.
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.
from terraform-aws-alb-web-containers.
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 :)
from terraform-aws-alb-web-containers.
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
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 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.
from terraform-aws-alb-web-containers.
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.
from terraform-aws-alb-web-containers.
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 :
- 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.
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
( oralb_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 )
from terraform-aws-alb-web-containers.
@esacteksab What do you think? Am I off base?
from terraform-aws-alb-web-containers.
Related Issues (6)
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from terraform-aws-alb-web-containers.