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

Version 1.46 aws_route53_zone changed behavior which breaks a lot of configurations #6535

Closed
voroniys opened this issue Nov 20, 2018 · 18 comments · Fixed by #6844
Closed

Version 1.46 aws_route53_zone changed behavior which breaks a lot of configurations #6535

voroniys opened this issue Nov 20, 2018 · 18 comments · Fixed by #6844
Labels
service/acm Issues and PRs that pertain to the acm service. service/route53 Issues and PRs that pertain to the route53 service.
Milestone

Comments

@voroniys
Copy link

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform Version

Terraform v0.11.10

  • provider.aws v1.46.0

Affected Resource(s)

  • aws_route53_zone
    in previous versions (at least in 1.41) property name was returned without trailing dot, what allow to use it in creation certificates, dns records, etc.

Terraform Configuration Files

For instance (related to issue #6533 )

resource "aws_acm_certificate" "my_cert" {
domain_name = "${var.dns_name}.${aws_route53_zone.myzone.name}"
validation_method = "DNS"
}

This construction is failing now due to traling dot in ${aws_route53_zone.myzone.name}

Expected Behavior

To maintain compatibility with previous versions and remove traling from the name property.
It is much easier to add a dot where it is needed, than remove it in all places where it is harmful.

@brianabston001
Copy link

Interesting.. I have mine pinned to version 1.40.0 and I am having the issue.

@voroniys
Copy link
Author

A possible solution for the issue could be a creation of interpolation function to strip trailing dot.

@brianabston001
Copy link

brianabston001 commented Nov 20, 2018

To get around this I am having to do a replace on code which is not ideal.

  domain_name = "api.${replace(aws_route53_zone.public_io.name, "/.$/", "")}"
  validation_method = "DNS"

  tags {
    Name        = "${var.environment}-integration-cert"
    Environment = "${var.environment}"
  }
}

@voroniys
Copy link
Author

Interesting.. I have mine pinned to version 1.40.0 and I am having the issue.

For me it is bigger issue, than certificates only. Between 1.41 and 1.42 the behaviour of property name has changed, so it is returning trailing dot now. I've pinned my version to 1.41 to avoid revriting of configurations that use this property.
And there is no way to rewrite it BTW in many cases, as there is no function for sripping the trailing dot.

@voroniys
Copy link
Author

To get around this I am having to do a replace on code which is not ideal.

  domain_name = "api.${replace(aws_route53_zone.public_io.name, "/.$/", "")}"

I'm doing pretty much the same, but I'm using this name in dozens of the places, so I assign it once to local variable and than everywhere use ${local.dns_zone}.
And btw be careful - your regex is not correct - you removing any last symbol from the string. It should be /\.$/ to remove last dot only.

@brianabston001
Copy link

To get around this I am having to do a replace on code which is not ideal.

  domain_name = "api.${replace(aws_route53_zone.public_io.name, "/.$/", "")}"

I'm doing pretty much the same, but I'm using this name in dozens of the places, so I assign it once to local variable and than everywhere use ${local.dns_zone}.
And btw be careful - your regex is not correct - you removing any last symbol from the string. It should be /.$/ to remove last dot only.

hmmm when I do an init with it like below I get an error illegal char escape

domain_name = "api.${replace(aws_route53_zone.public_io.name, "/\.$/", "")}"

Can you show me the example on how you are using it? I might just change over to that setup. I am using it almost a dozen times as well.

@voroniys
Copy link
Author

voroniys commented Nov 22, 2018

locals {
  dns_zone = "${replace(aws_route53_zone.myzone.name, "/\\.$/", "")}"
}
resource "aws_acm_certificate" "my_cert" {
  domain_name       = "${var.dns_name}.${local.dns_zone}"
  subject_alternative_names = [
    "prefix1-${var.dns_name}.${local.dns_zone}",
    "prefix2-${var.dns_name}.${local.dns_zone}",
    "prefix3-mutual-${var.dns_name}.${local.dns_zone}"
  ]
  validation_method = "DNS"
  lifecycle {
    create_before_destroy = true
  }
}

@brianabston001
Copy link

locals { dns_zone = "${replace(aws_route53_zone.myzone.name, "/\.$/", "")}" } resource "aws_acm_certificate" "my_cert" { domain_name = "${var.dns_name}.${local.dns_zone}" subject_alternative_names = [ "prefix1-${var.dns_name}.${local.dns_zone}", "prefix2-${var.dns_name}.${local.dns_zone}", "prefix3-mutual-${var.dns_name}.${local.dns_zone}" ] validation_method = "DNS" lifecycle { create_before_destroy = true } }

Perfect.. That is cleaner than what I was doing... Also see what I was missing. Thanks!!

@horjulf
Copy link

horjulf commented Nov 26, 2018

The change was introduced here.

@bflad was the behavior change intended ?

@bflad bflad added service/route53 Issues and PRs that pertain to the route53 service. service/acm Issues and PRs that pertain to the acm service. labels Nov 26, 2018
@bflad
Copy link
Contributor

bflad commented Nov 26, 2018

The change was intentional (we should always refresh the Terraform state from the API), but the behavior change (the Terraform attribute always returning a trailing period even if the resource was created without the trailing period) was unintentional. So sorry about that. Previously the resource was just honoring what was passed via the Terraform configuration during resource creation (the name with or without trailing period). It would always return the trailing period in the name attribute during import. The name attribute was not being acceptance tested to this detail before.

This leaves us in an awkward position now that the breaking change for the creation without trailing period situation has been introduced. In my opinion, the resource should always be consistent with its attribute outputs no matter how the resource got to be into the Terraform state, otherwise the behavior, like before, can be confusing. Consistently refreshing attributes and returning their result from the API is the operating model for almost all other resource attributes in the AWS provider. Unfortunately in this case, consistency in this case always means including the trailing period if we always just take the API response as-is.

I see one or more options here:

  • As a stop-gap fix to restore the previous (inconsistent) behavior, during Create, we can set the name attribute into the Terraform state to match the configuration and add a conditional during the Read function to strip the trailing period from the name attribute if the trailing period is not already in the state. This would be likely only remain in place until the next major version of the AWS provider.
  • We can roll forward and just retroactively add a note in the CHANGELOG for the new behavior of the attribute. We should probably do this regardless of other decisions.
  • We can roll forward and enhance the aws_acm_certificate resource to automatically strip the trailing period to prevent the error.
  • We can plan on always stripping trailing periods in the next major version of the AWS provider. Of course this can have the opposite effect of breaking configurations for those places that might require the trailing period.

@bflad
Copy link
Contributor

bflad commented Nov 26, 2018

Thinking about this more, I should mention for the first option there, its not possible to fix the Terraform state to remove the trailing period based on the configuration if the state has already been refreshed to include the trailing period. The Terraform provider SDK does not give us a way to forcibly read (d.Get(), etc.) from only the Terraform configuration and not include the value from the Terraform state.

To put it another way, if you've already upgraded the provider and refreshed the Terraform state like #6533, the trailing period is "stuck" at this point even if you try to downgrade versions and refresh.

@horjulf
Copy link

horjulf commented Nov 26, 2018

I think a combination of both the 2nd and 3rd points is the way to go.
The unexpected change of behavior was the biggest impact IMO, having it mentioned in the Changelog and updating aws_acm_certificate to handle both cases should be enough to address this.

@horjulf
Copy link

horjulf commented Nov 26, 2018

Something to keep in mind is that the content of the field name is changed from its original value if created without the trailing dot (suppressRoute53ZoneNameWithTrailingDot), maybe a warning note should be added to the docs.

@bflad bflad added this to the v1.51.0 milestone Nov 30, 2018
@mordax7
Copy link

mordax7 commented Dec 4, 2018

Same here thing here for me, between 1.41 and 1.42 the behaviour of property name has changed and it adds a trailing dot in the end. Many of my certificates are broken right now.

@bflad
Copy link
Contributor

bflad commented Dec 13, 2018

Pull request submitted to have the aws_acm_certificate resource automatically trim a trailing period for domain_name and subject_alternative_names: #6844

@bflad
Copy link
Contributor

bflad commented Dec 13, 2018

The above has been released in version 1.52.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@nicwise
Copy link
Contributor

nicwise commented Dec 19, 2018

I had this on aws_api_gateway_domain_name too - the domain_name field doesn't like a . at the end. I'm guessing there might be more? - @bflad

@ghost
Copy link

ghost commented Apr 1, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/acm Issues and PRs that pertain to the acm service. service/route53 Issues and PRs that pertain to the route53 service.
Projects
None yet
6 participants