provider/aws: Refactor Route53 record to fix regression in deleting

[catsby]

Route 53 records created in Terraform versions pre-0.6.9 can not be deleted by versions post-0.6.9. This is because of the addition of the weight attribute and how we calculate our records for changes after introducing the sentinel value of -1 for weights:

    w := d.Get("weight").(int)
    if w > -1 {
        rec.Weight = aws.Int64(int64(w))
    }

Records created pre-0.6.9 do not have the weight sentinel value of -1, which was added so that a value of zero could be used for the weight (prior to this it could not be). Because they do not have the -1 value, a d.Get("weight") call returns 0, which is greater than -1. As a result, delete commands includes a 0 weight on records that don't have a weight, so the delete record set doesn't match.

Since the actual record is not a thing we can query directly (it’s a property of the zone), we can’t just issue a DELETE to a specific record, we have to isolate it from the zone’s records and send an UPDATE / DELETE ChangeBatch call to the zone. Currently, we do this by rebuilding the record set from the state.

Additions to the state (via new attributes, et. al) in new versions of Terraform can cause this rebuild to be different than a previous run, if the state file was created in a previous version of Terraform.

In this PR we refactor out the code used in the READ method to range over a record set and find the matching record. We can then issue a delete call on that record, or update the local state file if we’re in the READ method.

Fixes #4641

[phinze]

Looks like any kind of error here is treated the same. Up in findRecord there's code to recognize NoSuchHostedZone specifically, but then it's wrapped back up in a regular error return value.

I believe this would mean that, for example, if ran terraform plan w/o network connectivity, I could lose all my R53 records from the state.

Maybe we can change the "NoSuchHostedZone" case to just return nil, nil and switch this up to something like:

if err != nil {
  return err
}
if rec == nil {
  log.Printf("[WARN] No matching record found...")
  d.SetId("")
}

What do you think?

[catsby]

Agree; this was refactored in 3bbb21d

[phinze]

Switching to findRecord makes a lot of sense to me! One Q about error handling, but looks good generally.

[jen20]

Changes to error handling LGTM. Build is because of broken upstream dep rather than anything on this PR.

[mrwacky42]

Please get this in 0.6.12. This is a PITA.

[ghost]

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.