CORE: Clean up diff2 code in prep for production (#2104)

integrationTest/integration_test.go

@@ -326,7 +326,7 @@ func TestDualProviders(t *testing.T) {
 			t.Fatal(err)
 		}
 		for i, c := range cs {
-			t.Logf("#%d: %s", i+1, c.Msg)
+			t.Logf("#%d:\n%s", i+1, c.Msg)
 			if err = c.F(); err != nil {
 				t.Fatal(err)
 			}
@@ -913,6 +913,7 @@ func makeTests(t *testing.T) []*TestGroup {
 			not(
 				"AZURE_DNS",     // Removed because it is too slow
 				"CLOUDFLAREAPI", // Infinite pagesize but due to slow speed, skipping.
+				"DIGITALOCEAN",  // No paging. Why bother?
 				"CSCGLOBAL",     // Doesn't page. Works fine.  Due to the slow API we skip.
 				"GANDI_V5",      // Their API is so damn slow. We'll add it back as needed.
 				"MSDNS",         //  No paging done. No need to test.

pkg/diff/diff2compat.go

@@ -50,12 +50,7 @@ type differCompat struct {
 //   - The NewCompat() feature `extraValues` is not supported. That
 //     parameter must be set to nil.  If you use that feature, consider
 //     one of the pkg/diff2/By*() functions.
-func (d *differCompat) IncrementalDiff(existing []*models.RecordConfig) (unchanged, create, toDelete, modify Changeset, err error) {
+func (d *differCompat) IncrementalDiff(existing []*models.RecordConfig) (unchanged, toCreate, toDelete, toModify Changeset, err error) {
-	unchanged = Changeset{}
-	create = Changeset{}
-	toDelete = Changeset{}
-	modify = Changeset{}
-
 	instructions, err := diff2.ByRecord(existing, d.dc, nil)
 	if err != nil {
 		return nil, nil, nil, nil, err
@@ -70,11 +65,11 @@ func (d *differCompat) IncrementalDiff(existing []*models.RecordConfig) (unchang
 			fmt.Println(inst.MsgsJoined)
 		case diff2.CREATE:
 			cor.Desired = inst.New[0]
-			create = append(create, cor)
+			toCreate = append(toCreate, cor)
 		case diff2.CHANGE:
 			cor.Existing = inst.Old[0]
 			cor.Desired = inst.New[0]
-			modify = append(modify, cor)
+			toModify = append(toModify, cor)
 		case diff2.DELETE:
 			cor.Existing = inst.Old[0]
 			toDelete = append(toDelete, cor)
@@ -89,17 +84,17 @@ func (d *differCompat) IncrementalDiff(existing []*models.RecordConfig) (unchang
 // ChangedGroups provides the same results as IncrementalDiff but grouped by key.
 func (d *differCompat) ChangedGroups(existing []*models.RecordConfig) (map[models.RecordKey][]string, error) {
 	changedKeys := map[models.RecordKey][]string{}
-	_, create, toDelete, modify, err := d.IncrementalDiff(existing)
+	_, toCreate, toDelete, toModify, err := d.IncrementalDiff(existing)
 	if err != nil {
 		return nil, err
 	}
-	for _, c := range create {
+	for _, c := range toCreate {
 		changedKeys[c.Desired.Key()] = append(changedKeys[c.Desired.Key()], c.String())
 	}
 	for _, d := range toDelete {
 		changedKeys[d.Existing.Key()] = append(changedKeys[d.Existing.Key()], d.String())
 	}
-	for _, m := range modify {
+	for _, m := range toModify {
 		changedKeys[m.Desired.Key()] = append(changedKeys[m.Desired.Key()], m.String())
 	}
 	return changedKeys, nil

pkg/diff2/analyze.go

@@ -20,18 +20,14 @@ func analyzeByRecordSet(cc *CompareConfig) ChangeList {
 			dts := rt.desiredTargets
 			msgs := genmsgs(ets, dts)
 			if len(msgs) == 0 { // No differences?
-				//fmt.Printf("DEBUG: done. Records are the same\n")
 				// The records at this rset are the same. No work to be done.
 				continue
 			}
 			if len(ets) == 0 { // Create a new label.
-				//fmt.Printf("DEBUG: add\n")
 				instructions = append(instructions, mkAdd(lc.label, rt.rType, msgs, rt.desiredRecs))
 			} else if len(dts) == 0 { // Delete that label and all its records.
-				//fmt.Printf("DEBUG: delete\n")
 				instructions = append(instructions, mkDelete(lc.label, rt.rType, msgs, rt.existingRecs))
 			} else { // Change the records at that label
-				//fmt.Printf("DEBUG: change\n")
 				instructions = append(instructions, mkChange(lc.label, rt.rType, msgs, rt.existingRecs, rt.desiredRecs))
 			}
 		}
@@ -41,22 +37,21 @@ func analyzeByRecordSet(cc *CompareConfig) ChangeList {
 
 func analyzeByLabel(cc *CompareConfig) ChangeList {
 	var instructions ChangeList
-	//fmt.Printf("DEBUG: START: analyzeByLabel\n")
+	// Accumulate any changes and collect the info needed to generate instructions.
-	// Accumulate if there are any changes and collect the info needed to generate instructions.
+	for _, lc := range cc.ldata {
-	for i, lc := range cc.ldata {
+		// for each type at that label...
-		//fmt.Printf("DEBUG: START LABEL = %q\n", lc.label)
 		label := lc.label
 		var accMsgs []string
 		var accExisting models.Records
 		var accDesired models.Records
 		msgsByKey := map[models.RecordKey][]string{}
 		for _, rt := range lc.tdata {
-			//fmt.Printf("DEBUG: START RTYPE = %q\n", rt.rType)
+			// for each type at that label...
 			ets := rt.existingTargets
 			dts := rt.desiredTargets
 			msgs := genmsgs(ets, dts)
-			msgsByKey[models.RecordKey{NameFQDN: label, Type: rt.rType}] = msgs
+			k := models.RecordKey{NameFQDN: label, Type: rt.rType}
-			//fmt.Printf("DEBUG:    appending msgs=%v\n", msgs)
+			msgsByKey[k] = msgs
 			accMsgs = append(accMsgs, msgs...)                    // Accumulate the messages
 			accExisting = append(accExisting, rt.existingRecs...) // Accumulate records existing at this label.
 			accDesired = append(accDesired, rt.desiredRecs...)    // Accumulate records desired at this label.
@@ -68,23 +63,14 @@ func analyzeByLabel(cc *CompareConfig) ChangeList {
 		// Based on that info, we can generate the instructions.
 
 		if len(accMsgs) == 0 { // Nothing changed.
-			//fmt.Printf("DEBUG: analyzeByLabel: %02d: no change\n", i)
 		} else if len(accDesired) == 0 { // No new records at the label? This must be a delete.
-			//fmt.Printf("DEBUG: analyzeByLabel: %02d: delete\n", i)
 			instructions = append(instructions, mkDelete(label, "", accMsgs, accExisting))
 		} else if len(accExisting) == 0 { // No old records at the label? This must be a change.
-			//fmt.Printf("DEBUG: analyzeByLabel: %02d: create\n", i)
+			c := mkAdd(label, "", accMsgs, accDesired)
-			//fmt.Printf("DEBUG: analyzeByLabel mkAdd msgs=%d\n", len(accMsgs))
+			c.MsgsByKey = msgsByKey
-			instructions = append(instructions, mkAddByLabel(label, "", accMsgs, accDesired))
+			instructions = append(instructions, c)
 		} else { // If we get here, it must be a change.
-			_ = i
+			instructions = append(instructions, mkChange(label, "", accMsgs, accExisting, accDesired))
-			// fmt.Printf("DEBUG: analyzeByLabel: %02d: change %d{%v} %d{%v} msgs=%v\n", i,
-			// 	len(accExisting), accExisting,
-			// 	len(accDesired), accDesired,
-			// 	accMsgs,
-			// )
-			//fmt.Printf("DEBUG: analyzeByLabel mkchange msgs=%d\n", len(accMsgs))
-			instructions = append(instructions, mkChangeByLabel(label, "", accMsgs, accExisting, accDesired, msgsByKey))
 		}
 	}
 
@@ -92,38 +78,23 @@ func analyzeByLabel(cc *CompareConfig) ChangeList {
 }
 
 func analyzeByRecord(cc *CompareConfig) ChangeList {
-	//fmt.Printf("DEBUG: analyzeByRecord: cc=%v\n", cc)
 
 	var instructions ChangeList
 	// For each label, for each type at that label, see if there are any changes.
 	for _, lc := range cc.ldata {
-		//fmt.Printf("DEBUG: analyzeByRecord: next lc=%v\n", lc)
 		for _, rt := range lc.tdata {
 			ets := rt.existingTargets
 			dts := rt.desiredTargets
 			cs := diffTargets(ets, dts)
-			//fmt.Printf("DEBUG: analyzeByRecord: cs=%v\n", cs)
 			instructions = append(instructions, cs...)
 		}
 	}
 	return instructions
 }
 
-// NB(tlim): there is no analyzeByZone.  ByZone calls anayzeByRecords().
+// FYI: there is no analyzeByZone.  diff2.ByZone() calls analyzeByRecords().
 
-func mkAdd(l string, t string, msgs []string, recs models.Records) Change {
+func mkAdd(l string, t string, msgs []string, newRecs models.Records) Change {
-	//fmt.Printf("DEBUG mkAdd called: (%v, %v, %v, %v)\n", l, t, msgs, recs)
-	c := Change{Type: CREATE, Msgs: msgs, MsgsJoined: strings.Join(msgs, "\n")}
-	c.Key.NameFQDN = l
-	c.Key.Type = t
-	c.New = recs
-	return c
-}
-
-// TODO(tlim): Clean these up. Some of them are exact duplicates!
-
-func mkAddByLabel(l string, t string, msgs []string, newRecs models.Records) Change {
-	//fmt.Printf("DEBUG mkAddByLabel called: (%v, %v, %v, %v)\n", l, t, msgs, newRecs)
 	c := Change{Type: CREATE, Msgs: msgs, MsgsJoined: strings.Join(msgs, "\n")}
 	c.Key.NameFQDN = l
 	c.Key.Type = t
@@ -132,7 +103,6 @@ func mkAddByLabel(l string, t string, msgs []string, newRecs models.Records) Cha
 }
 
 func mkChange(l string, t string, msgs []string, oldRecs, newRecs models.Records) Change {
-	//fmt.Printf("DEBUG mkChange called: (%v, %v, %v, %v, %v)\n", l, t, msgs, oldRecs, newRecs)
 	c := Change{Type: CHANGE, Msgs: msgs, MsgsJoined: strings.Join(msgs, "\n")}
 	c.Key.NameFQDN = l
 	c.Key.Type = t
@@ -141,49 +111,21 @@ func mkChange(l string, t string, msgs []string, oldRecs, newRecs models.Records
 	return c
 }
 
-func mkChangeByLabel(l string, t string, msgs []string, oldRecs, newRecs models.Records, msgsByKey map[models.RecordKey][]string) Change {
-	//fmt.Printf("DEBUG mkChangeLabel called: (%v, %v, %v, %v, %v, %v)\n", l, t, msgs, oldRecs, newRecs, msgsByKey)
-	c := Change{Type: CHANGE, Msgs: msgs, MsgsJoined: strings.Join(msgs, "\n")}
-	c.Key.NameFQDN = l
-	c.Key.Type = t
-	c.Old = oldRecs
-	c.New = newRecs
-	c.MsgsByKey = msgsByKey
-	return c
-}
-
 func mkDelete(l string, t string, msgs []string, oldRecs models.Records) Change {
-	//fmt.Printf("DEBUG mkDelete called: (%v, %v, %v, %v)\n", l, t, msgs, oldRecs)
-	if len(msgs) == 0 {
-		panic("mkDelete with no msg")
-	}
 	c := Change{Type: DELETE, Msgs: msgs, MsgsJoined: strings.Join(msgs, "\n")}
 	c.Key.NameFQDN = l
 	c.Key.Type = t
 	c.Old = oldRecs
 	return c
 }
-func mkDeleteByRecord(l string, t string, msgs []string, rec *models.RecordConfig) Change {
-	//fmt.Printf("DEBUG mkDeleteREc called: (%v, %v, %v, %v)\n", l, t, msgs, rec)
-	c := Change{Type: DELETE, Msgs: msgs, MsgsJoined: strings.Join(msgs, "\n")}
-	c.Key.NameFQDN = l
-	c.Key.Type = t
-	c.Old = models.Records{rec}
-	return c
-}
 
 func removeCommon(existing, desired []targetConfig) ([]targetConfig, []targetConfig) {
 
-	// NB(tlim): We could probably make this faster. Some ideas:
+	// Sort by comparableFull.
-	// * pre-allocate newexisting/newdesired and assign to indexed elements instead of appending.
-	// * iterate backwards (len(d) to 0) and delete items that are the same.
-	// On the other hand, this function typically receives lists of 1-3 elements
-	// and any optimization is probably fruitless.
-
-	// Sort to make comparisons easier
 	sort.Slice(existing, func(i, j int) bool { return existing[i].comparableFull < existing[j].comparableFull })
 	sort.Slice(desired, func(i, j int) bool { return desired[i].comparableFull < desired[j].comparableFull })
 
+	// Build maps required by filterBy
 	eKeys := map[string]*targetConfig{}
 	for _, v := range existing {
 		v := v
@@ -201,13 +143,12 @@ func removeCommon(existing, desired []targetConfig) ([]targetConfig, []targetCon
 // findTTLChanges finds the records that ONLY change their TTL. For those, generate a Change.
 // Remove such items from the list.
 func findTTLChanges(existing, desired []targetConfig) ([]targetConfig, []targetConfig, ChangeList) {
-	//fmt.Printf("DEBUG: findTTLChanges(%v,\n%v)\n", existing, desired)
 
 	if (len(existing) == 0) || (len(desired) == 0) {
 		return existing, desired, nil
 	}
 
-	// Sort to make comparisons easier
+	// Sort by comparableNoTTL
 	sort.Slice(existing, func(i, j int) bool { return existing[i].comparableNoTTL < existing[j].comparableNoTTL })
 	sort.Slice(desired, func(i, j int) bool { return desired[i].comparableNoTTL < desired[j].comparableNoTTL })
 
@@ -222,8 +163,9 @@ func findTTLChanges(existing, desired []targetConfig) ([]targetConfig, []targetC
 		dcomp := desired[di].comparableNoTTL
 
 		if ecomp == dcomp && er.TTL == dr.TTL {
-			fmt.Printf("DEBUG: ecomp=%q dcomp=%q er.TTL=%v dr.TTL=%v\n", ecomp, dcomp, er.TTL, dr.TTL)
+			panic(fmt.Sprintf(
-			panic("Should not happen. There should be some difference!")
+				"Should not happen. There should be some difference! ecomp=%q dcomp=%q er.TTL=%v dr.TTL=%v\n",
+				ecomp, dcomp, er.TTL, dr.TTL))
 		}
 
 		if ecomp == dcomp && er.TTL != dr.TTL {
@@ -247,7 +189,6 @@ func findTTLChanges(existing, desired []targetConfig) ([]targetConfig, []targetC
 
 	// Any remainder goes to the *Diff result:
 	if ei < len(existing) {
-		//fmt.Printf("DEBUG: append e len()=%d\n", ei)
 		existDiff = append(existDiff, existing[ei:]...)
 	}
 	if di < len(desired) {
@@ -259,14 +200,9 @@ func findTTLChanges(existing, desired []targetConfig) ([]targetConfig, []targetC
 
 // Return s but remove any items that can be found in m.
 func filterBy(s []targetConfig, m map[string]*targetConfig) []targetConfig {
-	// fmt.Printf("DEBUG: filterBy called with %v\n", s)
-	// for k := range m {
-	// 	fmt.Printf("DEBUG:    map %q\n", k)
-	// }
 	i := 0 // output index
 	for _, x := range s {
 		if _, ok := m[x.comparableFull]; !ok {
-			//fmt.Printf("DEBUG: comp %q NO\n", x.comparable)
 			// copy and increment index
 			s[i] = x
 			i++
@@ -278,7 +214,6 @@ func filterBy(s []targetConfig, m map[string]*targetConfig) []targetConfig {
 	// 	s[j] = nil
 	// }
 	s = s[:i]
-	// fmt.Printf("DEBUG: filterBy returns %v\n", s)
 	return s
 }
 
@@ -298,22 +233,16 @@ func humanDiff(a, b targetConfig) string {
 }
 
 func diffTargets(existing, desired []targetConfig) ChangeList {
-	//fmt.Printf("DEBUG: diffTargets called with len(e)=%d len(d)=%d\n", len(existing), len(desired))
 
 	// Nothing to do?
 	if len(existing) == 0 && len(desired) == 0 {
-		//fmt.Printf("DEBUG: diffTargets: nothing to do\n")
 		return nil
 	}
 
 	var instructions ChangeList
 
-	//fmt.Printf("DEBUG: diffTargets BEFORE existing=%+v\n", existing)
-	//fmt.Printf("DEBUG: diffTargets BEFORE desired=%+v\n", desired)
 	// remove the exact matches.
 	existing, desired = removeCommon(existing, desired)
-	//fmt.Printf("DEBUG: diffTargets AFTER existing=%+v\n", existing)
-	//fmt.Printf("DEBUG: diffTargets AFTER desired=%+v\n", desired)
 
 	// At this point the exact matches are removed. However there may be
 	// records that have the same GetTargetCombined() but different
@@ -322,37 +251,32 @@ func diffTargets(existing, desired []targetConfig) ChangeList {
 	existing, desired, newChanges := findTTLChanges(existing, desired)
 	instructions = append(instructions, newChanges...)
 
-	// Sort to make comparisons easier
+	// Sort by comparableFull
 	sort.Slice(existing, func(i, j int) bool { return existing[i].comparableFull < existing[j].comparableFull })
 	sort.Slice(desired, func(i, j int) bool { return desired[i].comparableFull < desired[j].comparableFull })
 
 	// the remaining chunks are changes (regardless of TTL)
 	mi := min(len(existing), len(desired))
-
 	for i := 0; i < mi; i++ {
-		//fmt.Println(i, "CHANGE")
 		er := existing[i].rec
 		dr := desired[i].rec
 
 		m := color.YellowString("± MODIFY %s %s %s", dr.NameFQDN, dr.Type, humanDiff(existing[i], desired[i]))
 
-		instructions = append(instructions, mkChange(dr.NameFQDN, dr.Type, []string{m},
+		instructions = append(instructions,
-			models.Records{er},
+			mkChange(dr.NameFQDN, dr.Type, []string{m}, models.Records{er}, models.Records{dr}),
-			models.Records{dr},
+		)
-		))
 	}
 
 	// any left-over existing are deletes
 	for i := mi; i < len(existing); i++ {
-		//fmt.Println(i, "DEL")
 		er := existing[i].rec
 		m := color.RedString("- DELETE %s %s %s", er.NameFQDN, er.Type, existing[i].comparableFull)
-		instructions = append(instructions, mkDeleteByRecord(er.NameFQDN, er.Type, []string{m}, er))
+		instructions = append(instructions, mkDelete(er.NameFQDN, er.Type, []string{m}, models.Records{er}))
 	}
 
 	// any left-over desired are creates
 	for i := mi; i < len(desired); i++ {
-		//fmt.Println(i, "CREATE")
 		dr := desired[i].rec
 		m := color.GreenString("+ CREATE %s %s %s", dr.NameFQDN, dr.Type, desired[i].comparableFull)
 		instructions = append(instructions, mkAdd(dr.NameFQDN, dr.Type, []string{m}, models.Records{dr}))
@@ -362,8 +286,7 @@ func diffTargets(existing, desired []targetConfig) ChangeList {
 }
 
 func genmsgs(existing, desired []targetConfig) []string {
-	cl := diffTargets(existing, desired)
+	return justMsgs(diffTargets(existing, desired))
-	return justMsgs(cl)
 }
 
 func justMsgs(cl ChangeList) []string {

pkg/diff2/analyze_test.go

@@ -398,7 +398,7 @@ ChangeList: len=12
 			compareCL(t, "analyzeByRecord", tt.name, "Rec", cl, tt.wantChangeRec)
 		})
 
-		// NB(tlim): There is no analyzeByZone().  ByZone() uses analyzeByRecord().
+		// NB(tlim): There is no analyzeByZone().  diff2.ByZone() uses analyzeByRecord().
 
 	}
 }

pkg/diff2/compareconfig.go

@@ -82,7 +82,7 @@ type rTypeConfig struct {
 }
 
 type targetConfig struct {
-	comparableFull  string // A string that can be used to compare two rec's for equality.
+	comparableFull  string // A string that can be used to compare two rec's for exact equality.
 	comparableNoTTL string // A string that can be used to compare two rec's for equality, ignoring the TTL
 
 	rec *models.RecordConfig // The RecordConfig itself.
@@ -110,10 +110,6 @@ func NewCompareConfig(origin string, existing, desired models.Records, compFn Co
 
 func (cc *CompareConfig) VerifyCNAMEAssertions() {
 
-	// In theory these assertions do not need to be tested as they test
-	// something that can not happen. In my  I've proved this to be
-	// true.  That said, a little paranoia is healthy.
-
 	// According to the RFCs if a label has a CNAME, it can not have any other
 	// records at that label... even other CNAMEs.  Therefore, we need to be
 	// careful with changes at a label that involve a CNAME.
@@ -131,7 +127,7 @@ func (cc *CompareConfig) VerifyCNAMEAssertions() {
 	//   there is already an A record at that label.
 	//
 	// To assure that DNS providers don't have to think about this, we order
-	// the tdata items so that we generate the instructions in the best order.
+	// the tdata items so that we generate the instructions in the correct order.
 	// In other words:
 	//     If there is a CNAME in existing, it should be in front.
 	//     If there is a CNAME in desired, it should be at the end.
@@ -154,15 +150,12 @@ func (cc *CompareConfig) VerifyCNAMEAssertions() {
 				}
 
 				if len(td.existingTargets) != 0 {
-					//fmt.Printf("DEBUG: cname in existing: index=%d\n", j)
 					if j != 0 {
 						panic("should not happen: (CNAME not in first position)")
 					}
 				}
 
 				if len(td.desiredTargets) != 0 {
-					//fmt.Printf("DEBUG: cname in desired: index=%d\n", j)
-					//fmt.Printf("DEBUG: highest: index=%d\n", j)
 					if j != highest(ld.tdata) {
 						panic("should not happen: (CNAME not in last position)")
 					}
@@ -273,7 +266,6 @@ func (cc *CompareConfig) addRecords(recs models.Records, storeInExisting bool) {
 
 		// Now it is safe to add/modify the records.
 
-		//fmt.Printf("BEFORE E/D: %v/%v\n", len(td.existingRecs), len(td.desiredRecs))
 		if storeInExisting {
 			cc.ldata[labelIdx].tdata[rtIdx].existingRecs = append(cc.ldata[labelIdx].tdata[rtIdx].existingRecs, rec)
 			cc.ldata[labelIdx].tdata[rtIdx].existingTargets = append(cc.ldata[labelIdx].tdata[rtIdx].existingTargets,
@@ -283,8 +275,5 @@ func (cc *CompareConfig) addRecords(recs models.Records, storeInExisting bool) {
 			cc.ldata[labelIdx].tdata[rtIdx].desiredTargets = append(cc.ldata[labelIdx].tdata[rtIdx].desiredTargets,
 				targetConfig{comparableNoTTL: compNoTTL, comparableFull: compFull, rec: rec})
 		}
-		//fmt.Printf("AFTER  L: %v\n", len(cc.ldata))
-		//fmt.Printf("AFTER  E/D: %v/%v\n", len(td.existingRecs), len(td.desiredRecs))
-		//fmt.Printf("\n")
 	}
 }

pkg/diff2/diff2.go

@@ -20,7 +20,7 @@ const (
 	CREATE             // Create a record/recordset/label where none existed before.
 	CHANGE             // Change existing record/recordset/label
 	DELETE             // Delete existing record/recordset/label
-	REPORT             // No change, but boy do I have something to say!
+	REPORT             // No change, but I have something to say!
 )
 
 type ChangeList []Change
@@ -83,7 +83,6 @@ General instructions:
   return corrections, nil
 }
 
-
 */
 
 // ByRecordSet takes two lists of records (existing and desired) and
@@ -95,7 +94,7 @@ General instructions:
 // record, if A records are added, changed, or removed, the API takes
 // www.example.com, A, and a list of all the desired IP addresses.
 //
-// Examples include:
+// Examples include: AZURE_DNS, GCORE, NS1, ROUTE53
 func ByRecordSet(existing models.Records, dc *models.DomainConfig, compFunc ComparableFunc) (ChangeList, error) {
 	return byHelper(analyzeByRecordSet, existing, dc, compFunc)
 }
@@ -107,7 +106,7 @@ func ByRecordSet(existing models.Records, dc *models.DomainConfig, compFunc Comp
 // time. That is, updates are done by sending a list of DNS records
 // to be served at a particular label, or the label itself is deleted.
 //
-// Examples include:
+// Examples include: GANDI_V5
 func ByLabel(existing models.Records, dc *models.DomainConfig, compFunc ComparableFunc) (ChangeList, error) {
 	return byHelper(analyzeByLabel, existing, dc, compFunc)
 }
@@ -123,7 +122,7 @@ func ByLabel(existing models.Records, dc *models.DomainConfig, compFunc Comparab
 // A change always has exactly 1 old and 1 new: .Old[0] and .New[0]
 // A delete always has exactly 1 old: .Old[0]
 //
-// Examples include: INWX
+// Examples include: CLOUDFLAREAPI, HEDNS, INWX, MSDNS, OVH, PORKBUN, VULTR
 func ByRecord(existing models.Records, dc *models.DomainConfig, compFunc ComparableFunc) (ChangeList, error) {
 	return byHelper(analyzeByRecord, existing, dc, compFunc)
 }
@@ -149,7 +148,7 @@ func ByRecord(existing models.Records, dc *models.DomainConfig, compFunc Compara
 //		// (dc.Records are the new records for the zone).
 //	}
 //
-// Example providers include: BIND
+// Example providers include: BIND, AUTODNS
 func ByZone(existing models.Records, dc *models.DomainConfig, compFunc ComparableFunc) ([]string, bool, error) {
 
 	if len(existing) == 0 {
@@ -157,7 +156,7 @@ func ByZone(existing models.Records, dc *models.DomainConfig, compFunc Comparabl
 		return nil, true, nil
 	}
 
-	// Only return the messages.
+	// Only return the messages.  The caller has the list of records needed to build the new zone.
 	instructions, err := byHelper(analyzeByRecord, existing, dc, compFunc)
 	return justMsgs(instructions), len(instructions) != 0, err
 }
@@ -199,7 +198,7 @@ func byHelper(fn func(cc *CompareConfig) ChangeList, existing models.Records, dc
 	return instructions, nil
 }
 
-// Stringify the datastructures for easier debugging
+// Stringify the datastructures (for debugging)
 
 func (c Change) String() string {
 	var buf bytes.Buffer

pkg/diff2/groupsort_test.go

@@ -25,6 +25,7 @@ func makeRecSet(recs ...*models.RecordConfig) *recset {
 	result.Recs = append(result.Recs, recs...)
 	return &result
 }
+
 func Test_groupbyRSet(t *testing.T) {
 
 	wwwa1 := makeRec("www", "A", "1.1.1.1")

pkg/diff2/handsoff.go

@@ -16,12 +16,11 @@ import (
 
 # How do NO_PURGE, IGNORE_*, ENSURE_ABSENT and friends work?
 
-
 ## Terminology:
 
 * "existing" refers to the records downloaded from the provider via the API.
 * "desired" refers to the records generated from dnsconfig.js.
-* "absences" refers to a list of records tagged with ASSURE_ABSENT.
+* "absences" refers to a list of records tagged with ENSURE_ABSENT.
 
 ## What are the features?
 
@@ -39,9 +38,9 @@ and 1 way to make exceptions.
     * IGNORE_TARGET(foo) is the same as UNMANAGED("*", "*", foo)
     * FYI: You CAN have a label with two A records, one controlled by
 	    DNSControl and one controlled by an external system.  DNSControl would
-		  need to have an UNMANAGED() statement with a targetglob that matches
+		need to have an UNMANAGED() statement with a targetglob that matches
 	    the external system's target values.
-* ASSURE_ABSENT: Override NO_PURGE for specific records. i.e. delete them even
+* ENSURE_ABSENT: Override NO_PURGE for specific records. i.e. delete them even
     though NO_PURGE is enabled.
     * If any of these records are in desired (matched on
       label:rtype:target), remove them.  This takes priority over
@@ -53,8 +52,9 @@ The fundamental premise is "if you don't want it deleted, copy it to the
 'desired' list." So, for example, if you want to IGNORE_NAME("www"), then you
 find any records with the label "www" in "existing" and copy them to "desired".
 As a result, the diff2 algorithm won't delete them because they are desired!
+(Of course "desired" can't have duplicate records. Check before you add.)
 
-This is different than in the old system (pkg/diff) which would generate the
+This is different than in the old implementation (pkg/diff) which would generate the
 diff but but then do a bunch of checking to see if the record was one that
 shouldn't be deleted.  Or, in the case of NO_PURGE, would simply not do the
 deletions.  This was complex because there were many edge cases to deal with.
@@ -75,7 +75,7 @@ Here is how we intend to implement these features:
 
   NO_PURGE + ENSURE_ABSENT is implemented as:
   * Take the list of existing records. If any do not appear in desired, add them
-      to desired UNLESS they appear in absences.
+      to desired UNLESS they appear in absences. (Yes, that's complex!)
   * "appear in desired" is done by matching on label:type.
   * "appear in absences" is done by matching on label:type:target.
 
@@ -154,7 +154,7 @@ func processIgnoreAndNoPurge(domain string, existing, desired, absences models.R
 	absentDB := models.NewRecordDBFromRecords(absences, domain)
 	compileUnmanagedConfigs(unmanagedConfigs)
 	for _, rec := range existing {
-		if matchAll(unmanagedConfigs, rec) {
+		if matchAny(unmanagedConfigs, rec) {
 			ignorable = append(ignorable, rec)
 		} else {
 			if noPurge {
@@ -176,7 +176,7 @@ func processIgnoreAndNoPurge(domain string, existing, desired, absences models.R
 func findConflicts(uconfigs []*models.UnmanagedConfig, recs models.Records) models.Records {
 	var conflicts models.Records
 	for _, rec := range recs {
-		if matchAll(uconfigs, rec) {
+		if matchAny(uconfigs, rec) {
 			conflicts = append(conflicts, rec)
 		}
 	}
@@ -219,8 +219,8 @@ func compileUnmanagedConfigs(configs []*models.UnmanagedConfig) error {
 	return nil
 }
 
-// matchAll returns true if rec matches any of the uconfigs.
+// matchAny returns true if rec matches any of the uconfigs.
-func matchAll(uconfigs []*models.UnmanagedConfig, rec *models.RecordConfig) bool {
+func matchAny(uconfigs []*models.UnmanagedConfig, rec *models.RecordConfig) bool {
 	for _, uc := range uconfigs {
 		if matchLabel(uc.LabelGlob, rec.GetLabel()) &&
 			matchType(uc.RTypeMap, rec.Type) &&

pkg/diff2/handsoff_test.go

@@ -49,7 +49,6 @@ func handsoffHelper(t *testing.T, existingZone, desiredJs string, noPurge bool,
 	if err != nil {
 		panic(err)
 	}
-	//fmt.Printf("DEBUG: existing=%s\n", showRecs(existing))
 
 	dnsconfig, err := js.ExecuteJavascriptString([]byte(desiredJs), false, nil)
 	if err != nil {

pkg/diff2/notes.txt

@@ -1,198 +0,0 @@
-EXISTING:
-  laba A 1.2.3.4      [0]
-  laba MX 10 laba     [1]
-  labc CNAME laba     [2]
-  labe A 10.10.10.15  [3]
-  labe A 10.10.10.16  [4]
-  labe A 10.10.10.17  [5]
-  labe A 10.10.10.18  [6]
-  labg NS 10.10.10.15 [7]
-  labg NS 10.10.10.16 [8]
-  labg NS 10.10.10.17 [9]
-  labg NS 10.10.10.18 [10]
-  labh CNAME labd     [11]
-
-DESIRED:
-  laba A 1.2.3.4      [0']
-  laba A 1.2.3.5      [1']
-  laba MX 20 labb     [2']
-  labe A 10.10.10.95  [3']
-  labe A 10.10.10.96  [4']
-  labe A 10.10.10.97  [5']
-  labe A 10.10.10.98  [6']
-  labf TXT "foo"      [7']
-  labg NS 10.10.10.10 [8']
-  labg NS 10.10.10.15 [9']
-  labg NS 10.10.10.16 [10']
-  labg NS 10.10.10.97 [11']
-  labh A 1.2.3.4      [12']
-
-ByRRSet:
-  [] laba:A     CHANGE NewSet: { [0], [1'] } (ByRecords needs: Old [0] )
-  [] laba:MX    CHANGE NewSet: { [2'] }  (ByLabel needs: Old: [2])
-  [] labc:CNAME DELETE Old: { [2 ] }
-  [] labe:A     CHANGE NewSet: { [3'], [4'], [5'], [6'] }
-  [] labf:TXT   CHANGE NewSet: { [7'] }
-  [] labg:NS    CHANGE NewSet: { [7] [8] [8'] [11'] }
-  [] labh:CNAME DELETE Old { [11] }
-  [] labh:A     CREATE NewSet: { [12'] }
-
-ByRecord:
-CREATE [1']
-CHANGE [1] [2']
-DELETE [2]
-CHANGE [3] [3']
-CHANGE [4] [4']
-CHANGE [5] [5']
-CHANGE [6] [6']
-CREATE [7']
-CREATE [8']
-CHANGE [10] [11']
-DELETE [11]
-CREATE [12']
-
-
-ByLabel: (take ByRRSet gather all CHANGES)
-laba   CHANGE NewSet: { [0'], [1'], [2'] }
-labc   DELETE Old: { [2] }
-labe   CHANGE New: { [3'], [4'], [5'], [6'] }
-labf   CREATE New: { [7'] }
-labg   CHANGE NewSet: { [7] [8] [8'] [11'] }
-labh   DELETE Old { [11] }
-labh   CREATE NewSet: { [12'] }
-
-
-
-
-By Record:
-
-rewrite as triples:   FQDN+TYPE, TARGET, RC
-byRecord:
-  group-by key=FQDN+TYPE, use targets to make add/change/delete for each record.
-
-byRSet:
-  group-by key=FQDN+TYPE, use targets to make add/change/delete for each record.
-     for each key:
-        if both have this key:
-            IF targets are the same, skip.
-            Else generate CHANGE for KEY:
-                New = Recs from desired.
-                Msgs = The msgs from targetdiff(e.Recs, d.Recs)
-
-byLabel:
-  group-by key=FQDN, use type+targets to make add/change/delete for each record.
-
-
-rewrite as triples:   FQDN {, TYPE, TARGET, RC
-
-type CompareConfig struct {
-  existing, desired models.Records
-  ldata: []LabelConfig
-}
-
-type ByLabelConfig struct {
-  label string
-  tdata: []ByRTypeConfig
-}
-
-type ByRTypeConfig struct {
-  rtype string
-  existing: []TargetConfig
-  desired: []TargetConfig
-  existingRecs: []*models.RecordConfig
-  desiredRecs: []*models.RecordConfig
-}
-
-type TargetConfig struct {
-  compareable string
-  rec *model.RecordConfig
-}
-
-func highest[S ~[]T, T any](s S) int {
-    return len(s) - 1 
-}
-
-populate CompareConfig.
-for rec := range desired {
-  label = FILL
-  rtype = FILL
-  comp = FILL
-  cc.labelMap[label] = &rc
-  cc.keyMap[key] = &rc
-  if not seen label:
-     append cc.ldata ByLabelConfig{}
-  labelIdx = last(cc.ldata)
-  if not seen key:
-     append cc.ldata[labelIdx].tdata ByRTypeConfig{}
-     rtIdx = last(cc.ldata[labelIdx].tdata)
-  cc.ldata[labelIdx].label = label
-  cc.ldata[labelIdx].tdata[rtIdx].rtype = rtype
-  cc.ldata[labelIdx].tdata[rtIdx].existing[append].comparable = comp
-  cc.ldata[labelIdx].tdata[rtIdx].existing[append].rec = &rc
-}
-
-ByRSet:
-func traverse(cc CompareConfig) {
-  for label := range cc.data {
-    for rtype := range label.data {
-      Msgs := genmsgs(rtype.existing, rtype.desired)
-      if no Msgs, continue
-      if len(rtype.existing) = 0 {
-          yield create(label, rtype, rtype.desiredRecs, Msgs)
-      } else if len(rtype.desired) = 0 {
-          yield delete(label, rtype, rtype.existingRecs, Msgs)
-      } else { // equal
-          yield change(label, rtype, rtype.desiredRecs, Msgs)
-    }
-  }
-}
-
-byLabel:
-func traverse(cc CompareConfig) {
-  for label := range cc.data {
-    initialize msgs, desiredRecords
-    anyExisting = false
-    for rtype := range label.data {
-      accumulate Msgs := genmsgs(rtype.existing, rtype.desired)
-      if Msgs (i.e. there were changes) {
-          accumulate AllDesired := rtype.desiredRecs
-          if len(rtype.existing) != 0 {
-            anyExisting = true
-          }
-      }
-    }
-    if there are Msgs:
-        if len(AllDesired) = 0 {
-          yield delete(label)
-        } else if countAllExisting == 0 {
-          yield create(label, AllDesired)
-        } else {
-          yield change(label, AllDesired)
-        }
-  }
-}
-
-ByRecord:
-func traverse(cc CompareConfig) {
-  for label := range cc.data {
-    for rtype := range label.data {
-      create, change, delete := difftargets(rtype.existing, rtype.desired)
-          yield creates, changes, deletes
-    }
-  }
-}
-
-Byzone:
-func traverse(cc CompareConfig) {
-  for label := range cc.data {
-    for rtype := range label.data {
-      Msgs := genmsgs(rtype.existing, rtype.desired)
-      accumulate Msgs
-    }
-  }
-  if len(accumMsgs) == 0 {
-    return nil, FirstMsg
-  } else {
-    return desired, Msgs
-  }
-}

providers/akamaiedgedns/akamaiEdgeDnsProvider.go

@@ -128,7 +128,6 @@ func (a *edgeDNSProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*mode
 		keysToUpdate, err = (diff.New(dc)).ChangedGroups(existingRecords)
 	} else {
 		keysToUpdate, err = (diff.NewCompat(dc)).ChangedGroups(existingRecords)
-		// TODO(tlim): In the future please adopt `pkg/diff2/By*()`
 	}
 	if err != nil {
 		return nil, err

providers/cloudflare/cloudflareProvider.go

@@ -14,6 +14,7 @@ import (
 	"github.com/StackExchange/dnscontrol/v3/pkg/transform"
 	"github.com/StackExchange/dnscontrol/v3/providers"
 	"github.com/cloudflare/cloudflare-go"
+	"github.com/fatih/color"
 	"github.com/miekg/dns/dnsutil"
 )
 
@@ -415,6 +416,17 @@ func (c *cloudflareProvider) mkCreateCorrection(newrec *models.RecordConfig, dom
 }
 
 func (c *cloudflareProvider) mkChangeCorrection(oldrec, newrec *models.RecordConfig, domainID string, msg string) []*models.Correction {
+	var idTxt string
+	switch oldrec.Type {
+	case "PAGE_RULE":
+		idTxt = oldrec.Original.(cloudflare.PageRule).ID
+	case "WORKER_ROUTE":
+		idTxt = oldrec.Original.(cloudflare.WorkerRoute).ID
+	default:
+		idTxt = oldrec.Original.(cloudflare.DNSRecord).ID
+	}
+	msg = msg + color.YellowString(" id=%v", idTxt)
+
 	switch newrec.Type {
 	case "PAGE_RULE":
 		return []*models.Correction{{
@@ -441,6 +453,7 @@ func (c *cloudflareProvider) mkChangeCorrection(oldrec, newrec *models.RecordCon
 }
 
 func (c *cloudflareProvider) mkDeleteCorrection(recType string, origRec any, domainID string, msg string) []*models.Correction {
+
 	var idTxt string
 	switch recType {
 	case "PAGE_RULE":
@@ -450,7 +463,7 @@ func (c *cloudflareProvider) mkDeleteCorrection(recType string, origRec any, dom
 	default:
 		idTxt = origRec.(cloudflare.DNSRecord).ID
 	}
-	msg = msg + fmt.Sprintf(" id=%v", idTxt)
+	msg = msg + color.RedString(" id=%v", idTxt)
 
 	correction := &models.Correction{
 		Msg: msg,

providers/cscglobal/dns.go

@@ -118,42 +118,18 @@ func (client *providerClient) GenerateDomainCorrections(dc *models.DomainConfig,
 	//txtutil.SplitSingleLongTxt(dc.Records) // Autosplit long TXT records
 
 	var corrections []*models.Correction
-	var creates, dels, modifications diff.Changeset
 	var err error
+	var differ diff.Differ
 	if !diff2.EnableDiff2 {
-		differ := diff.New(dc)
+		differ = diff.New(dc)
-		_, creates, dels, modifications, err = differ.IncrementalDiff(foundRecords)
 	} else {
-		differ := diff.NewCompat(dc)
+		differ = diff.NewCompat(dc)
-		_, creates, dels, modifications, err = differ.IncrementalDiff(foundRecords)
 	}
+	_, creates, dels, modifications, err := differ.IncrementalDiff(foundRecords)
 	if err != nil {
 		return nil, err
 	}
 
-	// How to generate corrections?
-
-	// (1) Most providers take individual deletes, creates, and
-	// modifications:
-
-	// // Generate changes.
-	//	corrections := []*models.Correction{}
-	//	for _, del := range dels {
-	//		corrections = append(corrections, client.deleteRec(client.dnsserver, dc.Name, del))
-	//	}
-	//	for _, cre := range creates {
-	//		corrections = append(corrections, client.createRec(client.dnsserver, dc.Name, cre)...)
-	//	}
-	//	for _, m := range modifications {
-	//		corrections = append(corrections, client.modifyRec(client.dnsserver, dc.Name, m))
-	//	}
-	//	return corrections, nil
-
-	// (2) Some providers upload the entire zone every time.  Look at
-	// GetDomainCorrections for BIND and NAMECHEAP for inspiration.
-
-	// (3) Others do something entirely different. Like CSCGlobal:
-
 	// CSCGlobal has a unique API.  A list of edits is sent in one API
 	// call. Edits aren't permitted if an existing edit is being
 	// processed. Therefore, before we do an edit we block until the

providers/digitalocean/digitaloceanProvider.go

@@ -147,20 +147,19 @@ func (api *digitaloceanProvider) GetDomainCorrections(dc *models.DomainConfig) (
 	txtutil.SplitSingleLongTxt(dc.Records) // Autosplit long TXT records
 
 	var corrections []*models.Correction
-	var create, delete, modify diff.Changeset
+	var differ diff.Differ
 	if !diff2.EnableDiff2 {
-		differ := diff.New(dc)
+		differ = diff.New(dc)
-		_, create, delete, modify, err = differ.IncrementalDiff(existingRecords)
 	} else {
-		differ := diff.NewCompat(dc)
+		differ = diff.NewCompat(dc)
-		_, create, delete, modify, err = differ.IncrementalDiff(existingRecords)
 	}
+	_, toCreate, toDelete, toModify, err := differ.IncrementalDiff(existingRecords)
 	if err != nil {
 		return nil, err
 	}
 
 	// Deletes first so changing type works etc.
-	for _, m := range delete {
+	for _, m := range toDelete {
 		id := m.Existing.Original.(*godo.DomainRecord).ID
 		corr := &models.Correction{
 			Msg: fmt.Sprintf("%s, DO ID: %d", m.String(), id),
@@ -177,7 +176,7 @@ func (api *digitaloceanProvider) GetDomainCorrections(dc *models.DomainConfig) (
 		}
 		corrections = append(corrections, corr)
 	}
-	for _, m := range create {
+	for _, m := range toCreate {
 		req := toReq(dc, m.Desired)
 		corr := &models.Correction{
 			Msg: m.String(),
@@ -194,7 +193,7 @@ func (api *digitaloceanProvider) GetDomainCorrections(dc *models.DomainConfig) (
 		}
 		corrections = append(corrections, corr)
 	}
-	for _, m := range modify {
+	for _, m := range toModify {
 		id := m.Existing.Original.(*godo.DomainRecord).ID
 		req := toReq(dc, m.Desired)
 		corr := &models.Correction{
@@ -281,7 +280,7 @@ func toRc(domain string, r *godo.DomainRecord) *models.RecordConfig {
 	t.SetTarget(target)
 	switch rtype := r.Type; rtype {
 	case "TXT":
-		t.SetTargetTXTString(target)
+		t.SetTargetTXTfromRFC1035Quoted(target)
 	default:
 		// nothing additional required
 	}

providers/dnsimple/dnsimpleProvider.go

@@ -177,10 +177,6 @@ func (c *dnsimpleProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*mod
 		return nil, err
 	}
 
-	if err != nil {
-		return nil, err
-	}
-
 	for _, del := range del {
 		rec := del.Existing.Original.(dnsimpleapi.ZoneRecord)
 		corrections = append(corrections, &models.Correction{

providers/gandiv5/gandi_v5Provider.go

@@ -366,6 +366,8 @@ func (client *gandiv5Provider) GenerateDomainCorrections(dc *models.DomainConfig
 
 		case diff2.CREATE:
 			// We have to create the label one rtype at a time.
+			// In other words, this is a ByRecordSet API for creation, even though
+			// this is a ByLabel() API for everything else.
 			natives := recordsToNative(inst.New, dc.Name)
 			for _, n := range natives {
 				label := inst.Key.NameFQDN

providers/gcloud/gcloudProvider.go

@@ -213,26 +213,22 @@ func (g *gcloudProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*model
 		differ = diff.NewCompat(dc)
 	}
 	_, create, delete, modify, err := differ.IncrementalDiff(existingRecords)
-	if err != nil {
-		return nil, err
-	}
-
 	if err != nil {
 		return nil, fmt.Errorf("incdiff error: %w", err)
 	}
 
 	changedKeys := map[key]bool{}
-	desc := ""
+	var msgs []string
 	for _, c := range create {
-		desc += fmt.Sprintln(c)
+		msgs = append(msgs, fmt.Sprint(c))
 		changedKeys[keyForRec(c.Desired)] = true
 	}
 	for _, d := range delete {
-		desc += fmt.Sprintln(d)
+		msgs = append(msgs, fmt.Sprint(d))
 		changedKeys[keyForRec(d.Existing)] = true
 	}
 	for _, m := range modify {
-		desc += fmt.Sprintln(m)
+		msgs = append(msgs, fmt.Sprint(m))
 		changedKeys[keyForRec(m.Existing)] = true
 	}
 	if len(changedKeys) == 0 {
@@ -282,7 +278,7 @@ func (g *gcloudProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*model
 	}
 
 	return []*models.Correction{{
-		Msg: desc,
+		Msg: strings.Join(msgs, "\n"),
 		F:   runChange,
 	}}, nil
 

providers/namecheap/namecheapProvider.go

@@ -203,9 +203,9 @@ func (n *namecheapProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*mo
 		return nil, err
 	}
 
-	// // because namecheap doesn't have selective create, delete, modify,
+	// because namecheap doesn't have selective create, delete, modify,
-	// // we bundle them all up to send at once.  We *do* want to see the
+	// we bundle them all up to send at once.  We *do* want to see the
-	// // changes though
+	// changes though
 
 	var desc []string
 	for _, i := range create {

providers/namedotcom/records.go

@@ -55,14 +55,13 @@ func (n *namedotcomProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*m
 	models.PostProcessRecords(actual)
 
 	var corrections []*models.Correction
-	var create, del, mod diff.Changeset
+	var differ diff.Differ
 	if !diff2.EnableDiff2 {
-		differ := diff.New(dc)
+		differ = diff.New(dc)
-		_, create, del, mod, err = differ.IncrementalDiff(actual)
 	} else {
-		differ := diff.NewCompat(dc)
+		differ = diff.NewCompat(dc)
-		_, create, del, mod, err = differ.IncrementalDiff(actual)
 	}
+	_, create, del, mod, err := differ.IncrementalDiff(actual)
 	if err != nil {
 		return nil, err
 	}

providers/packetframe/packetframeProvider.go

@@ -137,10 +137,6 @@ func (api *packetframeProvider) GetDomainCorrections(dc *models.DomainConfig) ([
 		return nil, err
 	}
 
-	if err != nil {
-		return nil, err
-	}
-
 	for _, m := range create {
 		req, err := toReq(zone.ID, dc, m.Desired)
 		if err != nil {

providers/rwth/dns.go

@@ -65,10 +65,6 @@ func (api *rwthProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*model
 		return nil, err
 	}
 
-	if err != nil {
-		return nil, err
-	}
-
 	for _, d := range create {
 		des := d.Desired
 		corrections = append(corrections, &models.Correction{