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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Jan 29, 2016
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1259879
provider/aws: Refactor Route53 record to fix regression in deleting
catsby Jan 28, 2016
3bbb21d
refactor error handling in findRecord
catsby Jan 29, 2016
39f5a7e
use the same error checking in DELETE
catsby Jan 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 39 additions & 51 deletions builtin/providers/aws/resource_aws_route53_record.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func resourceAwsRoute53RecordCreate(d *schema.ResourceData, meta interface{}) er
return fmt.Errorf("[WARN] No Route53 Zone found for id (%s)", zone)
}

// Get the record
// Build the record
rec, err := resourceAwsRoute53RecordBuildSet(d, *zoneRecord.HostedZone.Name)
if err != nil {
return err
Expand Down Expand Up @@ -242,19 +242,44 @@ func resourceAwsRoute53RecordCreate(d *schema.ResourceData, meta interface{}) er
}

func resourceAwsRoute53RecordRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).r53conn
record, err := findRecord(d, meta)
if err != nil {
log.Printf("[DEBUG] No matching record found for: %s, removing from state file", d.Id())
d.SetId("")
return nil
}

err = d.Set("records", flattenResourceRecords(record.ResourceRecords))
if err != nil {
return fmt.Errorf("[DEBUG] Error setting records for: %s, error: %#v", d.Id(), err)
}

d.Set("ttl", record.TTL)
// Only set the weight if it's non-nil, otherwise we end up with a 0 weight
// which has actual contextual meaning with Route 53 records
// See http://docs.aws.amazon.com/fr_fr/Route53/latest/APIReference/API_ChangeResourceRecordSets_Examples.html
if record.Weight != nil {
d.Set("weight", record.Weight)
}
d.Set("set_identifier", record.SetIdentifier)
d.Set("failover", record.Failover)
d.Set("health_check_id", record.HealthCheckId)

return nil
}

func findRecord(d *schema.ResourceData, meta interface{}) (*route53.ResourceRecordSet, error) {
conn := meta.(*AWSClient).r53conn
// Scan for a
zone := cleanZoneID(d.Get("zone_id").(string))

// get expanded name
zoneRecord, err := conn.GetHostedZone(&route53.GetHostedZoneInput{Id: aws.String(zone)})
if err != nil {
if r53err, ok := err.(awserr.Error); ok && r53err.Code() == "NoSuchHostedZone" {
log.Printf("[DEBUG] No matching Route 53 Record found for: %s, removing from state file", d.Id())
d.SetId("")
return nil
return nil, fmt.Errorf("no such hosted zone")
}
return err
return nil, err
}
en := expandRecordName(d.Get("name").(string), *zoneRecord.HostedZone.Name)
log.Printf("[DEBUG] Expanded record name: %s", en)
Expand All @@ -270,11 +295,9 @@ func resourceAwsRoute53RecordRead(d *schema.ResourceData, meta interface{}) erro
zone, lopts)
resp, err := conn.ListResourceRecordSets(lopts)
if err != nil {
return err
return nil, err
}

// Scan for a matching record
found := false
for _, record := range resp.ResourceRecordSets {
name := cleanRecordName(*record.Name)
if FQDN(strings.ToLower(name)) != FQDN(strings.ToLower(*lopts.StartRecordName)) {
Expand All @@ -287,55 +310,18 @@ func resourceAwsRoute53RecordRead(d *schema.ResourceData, meta interface{}) erro
if record.SetIdentifier != nil && *record.SetIdentifier != d.Get("set_identifier") {
continue
}

found = true

err := d.Set("records", flattenResourceRecords(record.ResourceRecords))
if err != nil {
return fmt.Errorf("[DEBUG] Error setting records for: %s, error: %#v", en, err)
}

d.Set("ttl", record.TTL)
// Only set the weight if it's non-nil, otherwise we end up with a 0 weight
// which has actual contextual meaning with Route 53 records
// See http://docs.aws.amazon.com/fr_fr/Route53/latest/APIReference/API_ChangeResourceRecordSets_Examples.html
if record.Weight != nil {
d.Set("weight", record.Weight)
}
d.Set("set_identifier", record.SetIdentifier)
d.Set("failover", record.Failover)
d.Set("health_check_id", record.HealthCheckId)

break
return record, nil
}

if !found {
log.Printf("[DEBUG] No matching record found for: %s, removing from state file", en)
d.SetId("")
}

return nil
return nil, fmt.Errorf("nothing found")
}

func resourceAwsRoute53RecordDelete(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).r53conn

zone := cleanZoneID(d.Get("zone_id").(string))
log.Printf("[DEBUG] Deleting resource records for zone: %s, name: %s",
zone, d.Get("name").(string))
var err error
zoneRecord, err := conn.GetHostedZone(&route53.GetHostedZoneInput{Id: aws.String(zone)})
if err != nil {
if r53err, ok := err.(awserr.Error); ok && r53err.Code() == "NoSuchHostedZone" {
log.Printf("[DEBUG] No matching Route 53 Record found for: %s, removing from state file", d.Id())
d.SetId("")
return nil
}
return err
}
// Get the records
rec, err := resourceAwsRoute53RecordBuildSet(d, *zoneRecord.HostedZone.Name)
rec, err := findRecord(d, meta)
if err != nil {
log.Printf("[DEBUG] No matching record found for: %s, removing from state file", d.Id())
d.SetId("")
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree; this was refactored in 3bbb21d

}

Expand All @@ -350,6 +336,8 @@ func resourceAwsRoute53RecordDelete(d *schema.ResourceData, meta interface{}) er
},
}

zone := cleanZoneID(d.Get("zone_id").(string))

req := &route53.ChangeResourceRecordSetsInput{
HostedZoneId: aws.String(cleanZoneID(zone)),
ChangeBatch: changeBatch,
Expand Down