Correctly group R53_ALIAS records during IncrementalDiff. (#399)

Previously, unnecessary corrections were possible if both an R53_ALIAS
pointing to an A record and to an AAAA record existed for the same label,
and map iteration over existing and desired found them in different orders.
(This is a common configuration for IPv6-enabled records.)

This commit:
 * mirrors key logic in the R53 provider
 * centralizes logic around keys in the models package
 * adds tests

models/record.go

@@ -246,7 +246,15 @@ type RecordKey struct {
 
 // Key converts a RecordConfig into a RecordKey.
 func (rc *RecordConfig) Key() RecordKey {
-	return RecordKey{rc.Name, rc.Type}
+	t := rc.Type
+	if rc.R53Alias != nil {
+		if v, ok := rc.R53Alias["type"]; ok {
+			// Route53 aliases append their alias type, so that records for the same
+			// label with different alias types are considered separate.
+			t = fmt.Sprintf("%s_%s", t, v)
+		}
+	}
+	return RecordKey{rc.Name, t}
 }
 
 // Records is a list of *RecordConfig.

models/record_test.go

@@ -0,0 +1,33 @@
+package models
+
+import "testing"
+
+func TestKey(t *testing.T) {
+	var tests = []struct {
+		rc       RecordConfig
+		expected RecordKey
+	}{
+		{
+			RecordConfig{Type: "A", Name: "@"},
+			RecordKey{Type: "A", Name: "@"},
+		},
+		{
+			RecordConfig{Type: "R53_ALIAS", Name: "@"},
+			RecordKey{Type: "R53_ALIAS", Name: "@"},
+		},
+		{
+			RecordConfig{Type: "R53_ALIAS", Name: "@", R53Alias: map[string]string{"foo": "bar"}},
+			RecordKey{Type: "R53_ALIAS", Name: "@"},
+		},
+		{
+			RecordConfig{Type: "R53_ALIAS", Name: "@", R53Alias: map[string]string{"type": "AAAA"}},
+			RecordKey{Type: "R53_ALIAS_AAAA", Name: "@"},
+		},
+	}
+	for i, test := range tests {
+		actual := test.rc.Key()
+		if test.expected != actual {
+			t.Errorf("%d: Expected %s, got %s", i, test.expected, actual)
+		}
+	}
+}

providers/diff/diff.go

@@ -68,16 +68,14 @@ func (d *differ) IncrementalDiff(existing []*models.RecordConfig) (unchanged, cr
 	desired := d.dc.Records
 
 	// sort existing and desired by name
-	type key struct {
+
-		name, rType string
+	existingByNameAndType := map[models.RecordKey][]*models.RecordConfig{}
-	}
+	desiredByNameAndType := map[models.RecordKey][]*models.RecordConfig{}
-	existingByNameAndType := map[key][]*models.RecordConfig{}
-	desiredByNameAndType := map[key][]*models.RecordConfig{}
 	for _, e := range existing {
 		if d.matchIgnored(e.GetLabel()) {
 			log.Printf("Ignoring record %s %s due to IGNORE", e.GetLabel(), e.Type)
 		} else {
-			k := key{e.GetLabelFQDN(), e.Type}
+			k := e.Key()
 			existingByNameAndType[k] = append(existingByNameAndType[k], e)
 		}
 	}
@@ -85,7 +83,7 @@ func (d *differ) IncrementalDiff(existing []*models.RecordConfig) (unchanged, cr
 		if d.matchIgnored(dr.GetLabel()) {
 			panic(fmt.Sprintf("Trying to update/add IGNOREd record: %s %s", dr.GetLabel(), dr.Type))
 		} else {
-			k := key{dr.GetLabelFQDN(), dr.Type}
+			k := dr.Key()
 			desiredByNameAndType[k] = append(desiredByNameAndType[k], dr)
 		}
 	}
@@ -93,7 +91,7 @@ func (d *differ) IncrementalDiff(existing []*models.RecordConfig) (unchanged, cr
 	if d.dc.KeepUnknown {
 		for k := range existingByNameAndType {
 			if _, ok := desiredByNameAndType[k]; !ok {
-				log.Printf("Ignoring record set %s %s due to NO_PURGE", k.rType, k.name)
+				log.Printf("Ignoring record set %s %s due to NO_PURGE", k.Type, k.Name)
 				delete(existingByNameAndType, k)
 			}
 		}

providers/route53/route53Provider.go

@@ -102,21 +102,6 @@ func (r *route53Provider) getZones() error {
 	return nil
 }
 
-// map key for grouping records
-type key struct {
-	Name, Type string
-}
-
-func getKey(r *models.RecordConfig) key {
-	var recordType = r.Type
-
-	if r.R53Alias != nil {
-		recordType = fmt.Sprintf("%s_%s", recordType, r.R53Alias["type"])
-	}
-
-	return key{r.GetLabelFQDN(), recordType}
-}
-
 type errNoExist struct {
 	domain string
 }
@@ -175,29 +160,18 @@ func (r *route53Provider) GetDomainCorrections(dc *models.DomainConfig) ([]*mode
 
 	// diff
 	differ := diff.New(dc, getAliasMap)
-	_, create, delete, modify := differ.IncrementalDiff(existingRecords)
+	namesToUpdate := differ.ChangedGroups(existingRecords)
-
-	namesToUpdate := map[key][]string{}
-	for _, c := range create {
-		namesToUpdate[getKey(c.Desired)] = append(namesToUpdate[getKey(c.Desired)], c.String())
-	}
-	for _, d := range delete {
-		namesToUpdate[getKey(d.Existing)] = append(namesToUpdate[getKey(d.Existing)], d.String())
-	}
-	for _, m := range modify {
-		namesToUpdate[getKey(m.Desired)] = append(namesToUpdate[getKey(m.Desired)], m.String())
-	}
 
 	if len(namesToUpdate) == 0 {
 		return nil, nil
 	}
 
-	updates := map[key][]*models.RecordConfig{}
+	updates := map[models.RecordKey][]*models.RecordConfig{}
 	// for each name we need to update, collect relevant records from dc
 	for k := range namesToUpdate {
 		updates[k] = nil
 		for _, rc := range dc.Records {
-			if getKey(rc) == k {
+			if rc.Key() == k {
 				updates[k] = append(updates[k], rc)
 			}
 		}