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

aws/route53: Add support for updating records #1122

Closed
radeksimko opened this issue Mar 5, 2015 · 18 comments
Closed

aws/route53: Add support for updating records #1122

radeksimko opened this issue Mar 5, 2015 · 18 comments

Comments

@radeksimko
Copy link
Member

aws_route53_record currently doesn't support update and simply tries to destroy & create all records per each terraform apply (= it doesn't even detect if there's any change):

$ terraform apply
aws_route53_zone.primary: Creating...
  name:    "" => "example.radeksimko.com"
  zone_id: "" => "<computed>"
aws_route53_zone.primary: Creation complete
aws_route53_record.sample: Creating...
  name:               "" => "sample"
  records.#:          "" => "1"
  records.2740947585: "" => "www.terraform.io"
  ttl:                "" => "60"
  type:               "" => "CNAME"
  zone_id:            "" => "Z3CIJPRJAZMMDD"
aws_route53_record.sample: Creation complete

Apply complete! Resources: 2 added, 0 changed, 0 destroyed.

The state of your infrastructure has been saved to the path
below. This state is required to modify and destroy your
infrastructure, so keep it safe. To inspect the complete state
use the `terraform show` command.

State path: terraform.tfstate

Outputs:

  hostname = sample.example.radeksimko.com
$ terraform apply
aws_route53_zone.primary: Refreshing state... (ID: Z3CIJPRJAZMMDD)
aws_route53_record.sample: Refreshing state... (ID: Z3CIJPRJAZMMDD_sample.example.radeksimko.com_CNAME)
aws_route53_record.sample: Destroying...
aws_route53_record.sample: Destruction complete
aws_route53_record.sample: Creating...
  name:               "" => "sample"
  records.#:          "" => "1"
  records.2740947585: "" => "www.terraform.io"
  ttl:                "" => "60"
  type:               "" => "CNAME"
  zone_id:            "" => "Z3CIJPRJAZMMDD"
aws_route53_record.sample: Creation complete

Apply complete! Resources: 1 added, 0 changed, 1 destroyed.

The state of your infrastructure has been saved to the path
below. This state is required to modify and destroy your
infrastructure, so keep it safe. To inspect the complete state
use the `terraform show` command.

State path: terraform.tfstate

Outputs:

  hostname = sample.example.radeksimko.com

It's worth mentioning that there are certain limits on the API:
http://docs.aws.amazon.com/Route53/latest/APIReference/API_ChangeResourceRecordSets.html

Note the following limitations on a ChangeResourceRecordSets request:

  • A request cannot contain more than 100 Change elements.
  • A request cannot contain more than 1000 ResourceRecord elements.
  • The sum of the number of characters (including spaces) in all Value elements in a request cannot exceed 32,000 characters.

which we may not want to care about for the initial implementation, but should keep that in mind for the future and have some mechanisms how to get around that and eventually send more requests if required.

@radeksimko
Copy link
Member Author

The "update all the time" behaviour only happens when using the shorter syntax introduced in #312 or #969 respectively. It actually does work well with the "verbose" syntax (with FQDN):

resource "aws_route53_zone" "primary" {
   name = "radeksimko.com"
}

resource "aws_route53_record" "sample" {
  zone_id = "${aws_route53_zone.primary.id}"
  name = "sample.${aws_route53_zone.primary.name}"
  type = "CNAME"
  ttl = "30"
  records = ["www.terraform.io"]
}

@radeksimko
Copy link
Member Author

^ That said, the original issue title is still valid, because proper Update functionality should be there. Changing things like ttl & records should not require destroy&create (ForceNew is wrong assumption in that context).

@catsby
Copy link
Contributor

catsby commented Mar 23, 2015

Hey @radeksimko – unless I'm reading the docs incorrectly (which is always possible 😄 ), Route 53 doesn't have a distinct update action. The action we use for creating records is UPSERT, which deletes the records and then recreates them for us

The "update all the time" behaviour only happens when using the shorter syntax introduced in #312 or #969 respectively.

sadly yes, though I'm looking into that now

@radeksimko
Copy link
Member Author

The action we use for creating records is UPSERT, which deletes the records and then recreates them for us

I don't think so, as the name suggests, it's UPSERT = Update or Insert, nothing like DELETE should be happening there.

That said, yes, you're going to use the same action name for both create & update, but different data will be passed into that API request, so that we can efficiently update things that can be updated and don't have to be recreated again (ttl + records).

Destroy & Create takes much more time and I don't really see the reason for waiting (not even mentioning the possibility of downtime due to radical DNS changes).

@catsby
Copy link
Contributor

catsby commented Mar 24, 2015

According to the documentation I linked to, and I admit maybe I'm misinterpreting it, Route 53's definition of UPSERT is a bit different here:

Note
If the value of the Action element in a ChangeResourceRecordSets request is UPSERT and the resource record set already exists, Amazon Route 53 automatically performs a DELETE request and a CREATE request. [...]

For what it's worth, I agree with your definition 😄

The more detailed API reference expands some more:

UPSERT: If a resource record set does not already exist, Amazon Route 53 creates it.

If a resource record set does exist, Amazon Route 53 updates it with the values in the request. Amazon Route 53 can update an existing resource record set only when all of the following values match: Name, Type, and SetIdentifier (for weighted, latency, geolocation, and failover resource record sets).

Unfortunately, we aren't yet setting any of those mentioned, beyond Name and Type. I suspect the update mentioned here may still be the implicit delete-and-create mentioned above.

I agree that a genuine update function on our side would be prefered, however I'm fairly confident that we'd still be bound to Route 53's definition of UPSERT, in which case we're stuck with the delete + create behind their curtains, so to speak.

@phinze has a theory: that we could remove any appropriate ForceNew attributes, and add a resourceAwsRoute53RecordUpdate that simply returns resourceAwsRoute53RecordCreate, which already uses UPSERT as it's action. Doing so should avoid Terraform calling delete and then create as part of renewing the resource, however, according to the docs the DELETE -> CREATE will still happen, but simply on AWS's side and not ours. It's not clear if that will be significantly faster, but it's worth a spike to test out.

If you have time, I would welcome your feedback on #1279, which I believe addresses the "update all the time" issue I introduced.

Thanks!

@radeksimko
Copy link
Member Author

I'm fairly confident that we'd still be bound to Route 53's definition of UPSERT, in which case we're stuck with the delete + create behind their curtains, so to speak.

It still means you'll be sending two API requests and waiting for the INSYNC status for both, which in reality takes at least couple minutes and you're doubling that period by this approach.

If you have time, I would welcome your feedback on #1279, which I believe addresses the "update all the time" issue I introduced.

I will have a look.

@radeksimko
Copy link
Member Author

TL;DR what I'm trying to say here is that the current update solution does the job but it's way too expensive in terms of API calls & time spent on waiting and the risk introduced by each such change.

Compare the risk of 1 "atomic" API call VS destroying something and then hoping that nothing goes wrong before you start creating it again.

@catsby
Copy link
Contributor

catsby commented Apr 6, 2015

I've submitted #1396 to address this

@catsby
Copy link
Contributor

catsby commented Apr 8, 2015

I just merged #1396. It adds a resource update function that simply falls through to the create function, and utilizes create's UPSERT. @radeksimko please take a look and let me know if you have ideas of further improvement.

If you're ok with #1396 and satisfied, we'll close this one.

@radeksimko
Copy link
Member Author

@catsby Thanks for pinging me.

The basic update works, but here's an example which demonstrates an issue:

resource "aws_route53_zone" "primary" {
   name = "radeksimko.com"
}

resource "aws_route53_record" "sample" {
  zone_id = "${aws_route53_zone.primary.id}"
  name = "sample"
  type = "CNAME"
  ttl = "30"
  records = ["www.terraform.io"]
}
$ terraform apply # OK
resource "aws_route53_zone" "primary" {
   name = "radeksimko.com"
}

resource "aws_route53_record" "sample" {
  zone_id = "${aws_route53_zone.primary.id}"
  name = "sample"
  type = "A"
  ttl = "30"
  records = ["127.0.0.1", "8.8.8.8"]
}
$ terraform apply
aws_route53_zone.primary: Refreshing state... (ID: Z3AVCL1ZT4U7Y6)
aws_route53_record.sample: Refreshing state... (ID: Z3AVCL1ZT4U7Y6_sample_CNAME)
aws_route53_record.sample: Destroying...
aws_route53_record.sample: Destruction complete
aws_route53_record.sample: Creating...
  name:               "" => "sample"
  records.#:          "" => "2"
  records.3619153832: "" => "127.0.0.1"
  records.3817307869: "" => "8.8.8.8"
  ttl:                "" => "30"
  type:               "" => "A"
  zone_id:            "" => "Z3AVCL1ZT4U7Y6"
aws_route53_record.sample: Error: 1 error(s) occurred:

* RRSet of type A with DNS name sample.radeksimko.com. is not permitted because a conflicting RRSet of type  CNAME with the same DNS name already exists in zone radeksimko.com.
Error applying plan:

1 error(s) occurred:

* 1 error(s) occurred:

* 1 error(s) occurred:

* RRSet of type A with DNS name sample.radeksimko.com. is not permitted because a conflicting RRSet of type  CNAME with the same DNS name already exists in zone radeksimko.com.

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.
  1. after I run the second modified template, it will effectively disown (resource disappears from tfstate) the record making it impossible to do anything with the a record of that name. Is that related to provider/aws: Fix issue in updating Route 53 records on refresh/read. #1430 ?
  2. It is probably right thing to force new resource when changing type, but TF most likely needs to wait until the old one is actually deleted before trying to add a new one.
  3. Another example which demonstrates another, but not so critical problem related to inaccurate plan:

ORIGINAL:

resource "aws_route53_record" "sample" {
  zone_id = "${aws_route53_zone.primary.id}"
  name = "sample"
  type = "A"
  ttl = "30"
  records = ["127.0.0.1", "8.8.8.8"]
}

Modified version:

resource "aws_route53_record" "sample" {
  zone_id = "${aws_route53_zone.primary.id}"
  name = "sample"
  type = "A"
  ttl = "30"
  records = ["127.0.0.1", "7.7.7.7"]
}

Reality after apply (as expected):
screen shot 2015-04-09 at 08 05 09

terraform plan (no mention about removing 8.8.8.8):

~ aws_route53_record.sample
    records.209503049: "" => "7.7.7.7"
    records.3619153832: "127.0.0.1" => "127.0.0.1"

terraform apply (no mention about removing 8.8.8.8):

aws_route53_zone.primary: Refreshing state... (ID: ***)
aws_route53_record.sample: Refreshing state... (ID: ***_sample_A)
aws_route53_record.sample: Modifying...
  records.209503049:  "" => "7.7.7.7"
  records.3619153832: "127.0.0.1" => "127.0.0.1"
aws_route53_record.sample: Modifications complete

Which leads me to a conclusion, that UPSERT (UPdate or inSERT ) in reality means potentially DELETE too, which is good as we don't need to call an explicit endpoint to delete things, but we still need to do the diffing to provide user the correct plan.

@catsby
Copy link
Contributor

catsby commented Apr 9, 2015

Thanks for the detailed follow up!

The first part I can't reproduce anymore, I think #1430 resolved it by actually recording the records in the state file correctly.

The later part is ... surprising (no mention of 8.8.8.8 going away). @phinze any idea what's causing that?

@catsby
Copy link
Contributor

catsby commented Apr 9, 2015

Which leads me to a conclusion, that UPSERT (UPdate or inSERT ) in reality means potentially DELETE too,

I think it means always DELETE too:

@radeksimko
Copy link
Member Author

@catsby Part of this issue seems to be fixed. 👍
I tried executing the last example twice w/ the latest version from current master and it worked well in both cases.

Is it just a coincidence or destroy events now always go before create events? If yes, I think we can close this issue and create a new one just for the issue w/ plan.

@radeksimko
Copy link
Member Author

That said, a simple acceptance test that would be changing type would be probably safe for the future.

@catsby
Copy link
Contributor

catsby commented May 4, 2015

see #1795

@catsby
Copy link
Contributor

catsby commented May 4, 2015

regarding this issue, I believe 32eebf4e fixed an issue where we were not tolerating PriorRequestNotComplete scenarios.

Closing this issue, let me know if there is anything else.

@pawelprazak
Copy link

pawelprazak commented Oct 5, 2016

It still fails to update an aws_route53_record containing an alias.
When using FQDN it doesn't do anything:
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
And falls back to destroy/create for short syntax:
Apply complete! Resources: 1 added, 0 changed, 1 destroyed.

Terraform v0.7.4

@ghost
Copy link

ghost commented Apr 21, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants