From 05dc26bf2e203a6c4d0d72a3c5a88aa93a2dd702 Mon Sep 17 00:00:00 2001 From: Tom Limoncelli Date: Sat, 25 Feb 2023 22:40:54 -0500 Subject: [PATCH] BUG: diff2: ttl changes don't always work (#2093) --- integrationTest/integration_test.go | 1 - models/record.go | 12 +++ pkg/diff2/analyze.go | 112 ++++++++++++++-------------- pkg/diff2/analyze_test.go | 51 +++++++------ pkg/diff2/compareconfig.go | 36 +++++---- pkg/diff2/compareconfig_test.go | 33 ++++++++ pkg/diff2/diff2.go | 4 +- providers/namedotcom/records.go | 3 +- 8 files changed, 151 insertions(+), 101 deletions(-) diff --git a/integrationTest/integration_test.go b/integrationTest/integration_test.go index 324d9aa92..59b34fdc3 100644 --- a/integrationTest/integration_test.go +++ b/integrationTest/integration_test.go @@ -750,7 +750,6 @@ func makeTests(t *testing.T) []*TestGroup { // This is a strange one. It adds a new record to an existing // label but the pre-existing label has its TTL change. testgroup("add to label and change orig ttl", - not("NAMEDOTCOM"), // Known bug: https://github.com/StackExchange/dnscontrol/issues/2088 tc("Setup", ttl(a("www", "5.6.7.8"), 400)), tc("Add at same label, new ttl", ttl(a("www", "5.6.7.8"), 700), ttl(a("www", "1.2.3.4"), 700)), ), diff --git a/models/record.go b/models/record.go index d780fd0c1..f81af8bbd 100644 --- a/models/record.go +++ b/models/record.go @@ -307,6 +307,8 @@ func (rc *RecordConfig) GetLabelFQDN() string { // ToDiffable returns a string that is comparable by a differ. // extraMaps: a list of maps that should be included in the comparison. +// NB(tlim): This will be deprecated when pkg/diff is replaced by pkg/diff2. +// Use // ToComparableNoTTL() instead. func (rc *RecordConfig) ToDiffable(extraMaps ...map[string]string) string { var content string switch rc.Type { @@ -337,6 +339,16 @@ func (rc *RecordConfig) ToDiffable(extraMaps ...map[string]string) string { return content } +// ToComparableNoTTL returns a comparison string. If you need to compare two +// RecordConfigs, you can simply compare the string returned by this function. +// THe comparison includes all fields except TTL and any provider-specific +// metafields. Provider-specific metafields like CF_PROXY are not the same as +// pseudo-records like ANAME or R53_ALIAS +// This replaces ToDiff() +func (rc *RecordConfig) ToComparableNoTTL() string { + return rc.GetTargetCombined() +} + // ToRR converts a RecordConfig to a dns.RR. func (rc *RecordConfig) ToRR() dns.RR { diff --git a/pkg/diff2/analyze.go b/pkg/diff2/analyze.go index 6c142e7f3..33109d9e4 100644 --- a/pkg/diff2/analyze.go +++ b/pkg/diff2/analyze.go @@ -179,58 +179,70 @@ func removeCommon(existing, desired []targetConfig) ([]targetConfig, []targetCon // 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 }) + eKeys := map[string]*targetConfig{} for _, v := range existing { v := v - eKeys[v.compareable] = &v + eKeys[v.comparableFull] = &v } dKeys := map[string]*targetConfig{} for _, v := range desired { v := v - dKeys[v.compareable] = &v + dKeys[v.comparableFull] = &v } return filterBy(existing, dKeys), filterBy(desired, eKeys) } -// Find the changes that are exclusively changes in TTL. -func splitTTLOnly(existing, desired []targetConfig) ( - existDiff []targetConfig, desireDiff []targetConfig, - existTTL models.Records, desireTTL models.Records, -) { +// 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.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 }) + + var instructions ChangeList + var existDiff, desiredDiff []targetConfig ei := 0 di := 0 - for (ei < len(existing)) && (di < len(desired)) { er := existing[ei].rec dr := desired[di].rec - ecomp := existing[ei].compareable - dcomp := desired[di].compareable + ecomp := existing[ei].comparableNoTTL + 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("Should not happen. There should be some difference!") } - //fmt.Printf("DEBUG ecomp=%q dcomp=%q ettl=%d dttl=%d\n", ecomp, dcomp, er.TTL, dr.TTL) if ecomp == dcomp && er.TTL != dr.TTL { - //fmt.Printf("DEBUG: equal\n") - existTTL = append(existTTL, er) - desireTTL = append(desireTTL, dr) + //m := fmt.Sprintf("CHANGE-TTL %s %s %s ttl=%d->%d", dr.NameFQDN, dr.Type, dr.GetTargetRFC1035Quoted(), er.TTL, dr.TTL) + m := fmt.Sprintf("CHANGE-TTL %s %s ", dr.NameFQDN, dr.Type) + humanDiff(existing[ei], desired[di]) + instructions = append(instructions, mkChange(dr.NameFQDN, dr.Type, []string{m}, + models.Records{er}, + models.Records{dr}, + )) ei++ di++ } else if ecomp < dcomp { - //fmt.Printf("DEBUG: less\n") existDiff = append(existDiff, existing[ei]) ei++ } else if ecomp > dcomp { - //fmt.Printf("DEBUG: greater\n") - desireDiff = append(desireDiff, desired[di]) + desiredDiff = append(desiredDiff, desired[di]) di++ } else { - panic("Should not happen. e and d can not be both equal, less and greater") + panic("should not happen. ecomp cant be both == and != dcomp") } - } // Any remainder goes to the *Diff result: @@ -240,10 +252,10 @@ func splitTTLOnly(existing, desired []targetConfig) ( } if di < len(desired) { //fmt.Printf("DEBUG: append d len()=%d\n", di) - desireDiff = append(desireDiff, desired[di:]...) + desiredDiff = append(desiredDiff, desired[di:]...) } - return + return existDiff, desiredDiff, instructions } // Return s but remove any items that can be found in m. @@ -254,8 +266,8 @@ func filterBy(s []targetConfig, m map[string]*targetConfig) []targetConfig { // } i := 0 // output index for _, x := range s { - if _, ok := m[x.compareable]; !ok { - //fmt.Printf("DEBUG: comp %q NO\n", x.compareable) + if _, ok := m[x.comparableFull]; !ok { + //fmt.Printf("DEBUG: comp %q NO\n", x.comparable) // copy and increment index s[i] = x i++ @@ -271,22 +283,19 @@ func filterBy(s []targetConfig, m map[string]*targetConfig) []targetConfig { return s } +// humanDiff returns a human-friendly string showing what changed +// between a and b. func humanDiff(a, b targetConfig) string { - aTTL := a.rec.TTL - bTTL := b.rec.TTL - acombined := a.compareable - bcombined := b.compareable - combinedDiff := acombined != bcombined - ttlDiff := aTTL != bTTL // TODO(tlim): Records like MX and SRV should have more clever output. - // For example if only the MX priority changes, show that. - if combinedDiff && ttlDiff { - return fmt.Sprintf("(%s ttl=%d) -> (%s ttl=%d)", acombined, aTTL, bcombined, bTTL) + // For example if only the MX priority changes, show just that. + + if a.comparableNoTTL != b.comparableNoTTL { + // The recorddata is different: + return fmt.Sprintf("(%s) -> (%s)", a.comparableFull, b.comparableFull) } - if combinedDiff { - return fmt.Sprintf("(%s) -> (%s)", acombined, bcombined) - } - return fmt.Sprintf("%s (ttl %d->%d)", acombined, aTTL, bTTL) + + // Just the TTLs are different: + return fmt.Sprintf("%s ttl=(%d->%d)", a.comparableNoTTL, a.rec.TTL, b.rec.TTL) } func diffTargets(existing, desired []targetConfig) ChangeList { @@ -298,15 +307,11 @@ func diffTargets(existing, desired []targetConfig) ChangeList { return nil } - // Sort to make comparisons easier - sort.Slice(existing, func(i, j int) bool { return existing[i].compareable < existing[j].compareable }) - sort.Slice(desired, func(i, j int) bool { return desired[i].compareable < desired[j].compareable }) - var instructions ChangeList - // remove the exact matches. //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) @@ -315,23 +320,16 @@ func diffTargets(existing, desired []targetConfig) ChangeList { // records that have the same GetTargetCombined() but different // TTLs. - // Find TTL changes: - existing, desired, existingTTL, desiredTTL := splitTTLOnly(existing, desired) - for i := range desiredTTL { - er := existingTTL[i] - dr := desiredTTL[i] + existing, desired, newChanges := findTTLChanges(existing, desired) + instructions = append(instructions, newChanges...) - m := fmt.Sprintf("CHANGE %s %s ", dr.NameFQDN, dr.Type) + humanDiff(existing[i], desired[i]) + // 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 }) - instructions = append(instructions, mkChange(dr.NameFQDN, dr.Type, []string{m}, - models.Records{er}, - models.Records{dr}, - )) - } - - // the common chunk are changes (regardless of TTL) + // the remaining chunks are changes (regardless of TTL) mi := min(len(existing), len(desired)) - //fmt.Printf("DEBUG: min=%d\n", mi) + for i := 0; i < mi; i++ { //fmt.Println(i, "CHANGE") er := existing[i].rec @@ -349,7 +347,7 @@ func diffTargets(existing, desired []targetConfig) ChangeList { for i := mi; i < len(existing); i++ { //fmt.Println(i, "DEL") er := existing[i].rec - m := fmt.Sprintf("DELETE %s %s %s", er.NameFQDN, er.Type, existing[i].compareable) + m := fmt.Sprintf("DELETE %s %s %s", er.NameFQDN, er.Type, existing[i].comparableFull) instructions = append(instructions, mkDeleteRec(er.NameFQDN, er.Type, []string{m}, er)) } @@ -357,7 +355,7 @@ func diffTargets(existing, desired []targetConfig) ChangeList { for i := mi; i < len(desired); i++ { //fmt.Println(i, "CREATE") dr := desired[i].rec - m := fmt.Sprintf("CREATE %s %s %s", dr.NameFQDN, dr.Type, desired[i].compareable) + m := fmt.Sprintf("CREATE %s %s %s", dr.NameFQDN, dr.Type, desired[i].comparableFull) instructions = append(instructions, mkAdd(dr.NameFQDN, dr.Type, []string{m}, models.Records{dr})) } diff --git a/pkg/diff2/analyze_test.go b/pkg/diff2/analyze_test.go index 92dba0bdf..812e21d0a 100644 --- a/pkg/diff2/analyze_test.go +++ b/pkg/diff2/analyze_test.go @@ -83,8 +83,8 @@ func Test_analyzeByRecordSet(t *testing.T) { } origin := "f.com" - existing := models.Records{testDataAA1234, testDataAMX10a, testDataCCa, testDataEA15, e4, e5, e6, e7, e8, e9, e10, e11} - desired := models.Records{testDataAA1234clone, testDataAA12345, testDataAMX20b, d3, d4, d5, d6, d7, d8, d9, d10, d11, d12} + existingSample := models.Records{testDataAA1234, testDataAMX10a, testDataCCa, testDataEA15, e4, e5, e6, e7, e8, e9, e10, e11} + desiredSample := models.Records{testDataAA1234clone, testDataAA12345, testDataAMX20b, d3, d4, d5, d6, d7, d8, d9, d10, d11, d12} tests := []struct { name string @@ -227,8 +227,8 @@ ChangeList: len=2 name: "big", args: args{ origin: origin, - existing: existing, - desired: desired, + existing: existingSample, + desired: desiredSample, }, wantMsgs: ` CREATE laba.f.com A 1.2.3.5 ttl=300 @@ -406,9 +406,11 @@ ChangeList: len=12 func mkTargetConfig(x ...*models.RecordConfig) []targetConfig { var tc []targetConfig for _, r := range x { + ct, cf := mkCompareBlobs(r, nil) tc = append(tc, targetConfig{ - compareable: mkCompareBlob(r, nil), - rec: r, + comparableNoTTL: ct, + comparableFull: cf, + rec: r, }) } return tc @@ -417,7 +419,7 @@ func mkTargetConfig(x ...*models.RecordConfig) []targetConfig { func mkTargetConfigMap(x ...*models.RecordConfig) map[string]*targetConfig { var m = map[string]*targetConfig{} for _, v := range mkTargetConfig(x...) { - m[v.compareable] = &v + m[v.comparableFull] = &v } return m } @@ -445,8 +447,8 @@ func Test_diffTargets(t *testing.T) { Key: models.RecordKey{NameFQDN: "laba.f.com", Type: "A"}, New: models.Records{testDataAA5678ttl700, testDataAA1234ttl700}, Msgs: []string{ - "CHANGE laba.f.com A (5.6.7.8 ttl=300 ttl=300) -> (1.2.3.4 ttl=700 ttl=700)", - "CREATE laba.f.com A 5.6.7.8 ttl=700", + "CHANGE-TTL laba.f.com A 5.6.7.8 ttl=(300->700)", + "CREATE laba.f.com A 1.2.3.4 ttl=700", }, }, }, @@ -527,9 +529,11 @@ func Test_diffTargets(t *testing.T) { t.Run(tt.name, func(t *testing.T) { //fmt.Printf("DEBUG: Test %02d\n", i) got := diffTargets(tt.args.existing, tt.args.desired) - d := diff.Diff(strings.TrimSpace(justMsgString(got)), strings.TrimSpace(justMsgString(tt.want))) + g := strings.TrimSpace(justMsgString(got)) + w := strings.TrimSpace(justMsgString(tt.want)) + d := diff.Diff(g, w) if d != "" { - //fmt.Printf("DEBUG: %d %d\n", len(got), len(tt.want)) + //fmt.Printf("DEBUG: fail %q %q\n", g, w) t.Errorf("diffTargets()\n diff=%s", d) } }) @@ -584,7 +588,7 @@ func Test_removeCommon(t *testing.T) { func comparables(s []targetConfig) []string { var r []string for _, j := range s { - r = append(r, j.compareable) + r = append(r, j.comparableFull) } return r } @@ -646,8 +650,7 @@ func Test_splitTTLOnly(t *testing.T) { args args wantExistDiff []targetConfig wantDesireDiff []targetConfig - wantExistTTL models.Records - wantDesireTTL models.Records + wantChanges string }{ { @@ -656,10 +659,9 @@ func Test_splitTTLOnly(t *testing.T) { existing: mkTargetConfig(testDataAA1234), desired: mkTargetConfig(testDataAA1234ttl700), }, - wantExistDiff: mkTargetConfig(testDataAA1234), - wantDesireDiff: mkTargetConfig(testDataAA1234ttl700), - wantExistTTL: models.Records{testDataAA1234}, - wantDesireTTL: models.Records{testDataAA1234ttl700}, + wantExistDiff: nil, + wantDesireDiff: nil, + wantChanges: "ChangeList: len=1\n00: Change: verb=CHANGE\n key={laba.f.com A}\n old=[1.2.3.4]\n new=[1.2.3.4]\n msg=[\"CHANGE-TTL laba.f.com A 1.2.3.4 ttl=(300->700)\"]\n", }, { @@ -670,24 +672,21 @@ func Test_splitTTLOnly(t *testing.T) { }, wantExistDiff: mkTargetConfig(testDataAA1234), wantDesireDiff: mkTargetConfig(testDataAA5678), - wantExistTTL: models.Records{}, - wantDesireTTL: models.Records{}, + wantChanges: "ChangeList: len=0\n", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotExistDiff, gotDesireDiff, gotExistTTL, gotDesireTTL := splitTTLOnly(tt.args.existing, tt.args.desired) + gotExistDiff, gotDesireDiff, gotChanges := findTTLChanges(tt.args.existing, tt.args.desired) if !reflect.DeepEqual(gotExistDiff, tt.wantExistDiff) { t.Errorf("splitTTLOnly() gotExistDiff = %v, want %v", gotExistDiff, tt.wantExistDiff) } if !reflect.DeepEqual(gotDesireDiff, tt.wantDesireDiff) { t.Errorf("splitTTLOnly() gotDesireDiff = %v, want %v", gotDesireDiff, tt.wantDesireDiff) } - if ((len(tt.wantExistTTL) != 0) && len(gotExistTTL) != 0) && !reflect.DeepEqual(gotExistTTL, tt.wantExistTTL) { - t.Errorf("splitTTLOnly() gotExistTTL = %v, want %v (len=%d %d)", gotExistTTL, tt.wantExistTTL, len(gotExistTTL), len(tt.wantExistTTL)) - } - if ((len(tt.wantDesireTTL) != 0) && len(gotDesireTTL) != 0) && !reflect.DeepEqual(gotDesireTTL, tt.wantDesireTTL) { - t.Errorf("splitTTLOnly() gotDesireTTL = %v, want %v", gotDesireTTL, tt.wantDesireTTL) + gotChangesString := gotChanges.String() + if gotChangesString != tt.wantChanges { + t.Errorf("splitTTLOnly() gotChanges=\n%q, want=\n%q", gotChangesString, tt.wantChanges) } }) } diff --git a/pkg/diff2/compareconfig.go b/pkg/diff2/compareconfig.go index 3cd93a51b..22249385b 100644 --- a/pkg/diff2/compareconfig.go +++ b/pkg/diff2/compareconfig.go @@ -82,8 +82,10 @@ type rTypeConfig struct { } type targetConfig struct { - compareable string // A string that can be used to compare two rec's for equality. - rec *models.RecordConfig // The RecordConfig itself. + comparableFull string // A string that can be used to compare two rec's for 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. } func NewCompareConfig(origin string, existing, desired models.Records, compFn ComparableFunc) *CompareConfig { @@ -202,15 +204,23 @@ func (cc *CompareConfig) String() string { // Generate a string that can be used to compare this record to others // for equality. -func mkCompareBlob(rc *models.RecordConfig, f func(*models.RecordConfig) string) string { - if f == nil { - return rc.ToDiffable() +func mkCompareBlobs(rc *models.RecordConfig, f func(*models.RecordConfig) string) (string, string) { + + // Start with the comparable string + comp := rc.ToComparableNoTTL() + + // If the custom function exists, add its output + if f != nil { + addOn := f(rc) + if addOn != "" { + comp += " " + f(rc) + } } - addOn := f(rc) - if addOn != "" { - return rc.ToDiffable() + " " + f(rc) - } - return rc.ToDiffable() + + lenWithoutTTL := len(comp) + compFull := comp + fmt.Sprintf(" ttl=%d", rc.TTL) + + return compFull[:lenWithoutTTL], compFull } func (cc *CompareConfig) addRecords(recs models.Records, storeInExisting bool) { @@ -229,7 +239,7 @@ func (cc *CompareConfig) addRecords(recs models.Records, storeInExisting bool) { key := rec.Key() label := key.NameFQDN rtype := key.Type - comp := mkCompareBlob(rec, cc.compareableFunc) + compNoTTL, compFull := mkCompareBlobs(rec, cc.compareableFunc) // Are we seeing this label for the first time? var labelIdx int @@ -267,11 +277,11 @@ func (cc *CompareConfig) addRecords(recs models.Records, storeInExisting bool) { 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, - targetConfig{compareable: comp, rec: rec}) + targetConfig{comparableNoTTL: compNoTTL, comparableFull: compFull, rec: rec}) } else { cc.ldata[labelIdx].tdata[rtIdx].desiredRecs = append(cc.ldata[labelIdx].tdata[rtIdx].desiredRecs, rec) cc.ldata[labelIdx].tdata[rtIdx].desiredTargets = append(cc.ldata[labelIdx].tdata[rtIdx].desiredTargets, - targetConfig{compareable: comp, rec: rec}) + 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)) diff --git a/pkg/diff2/compareconfig_test.go b/pkg/diff2/compareconfig_test.go index 54e3da9cc..e1e6645d2 100644 --- a/pkg/diff2/compareconfig_test.go +++ b/pkg/diff2/compareconfig_test.go @@ -206,3 +206,36 @@ compFn: }) } } + +func Test_mkCompareBlobs(t *testing.T) { + type args struct { + rc *models.RecordConfig + f func(*models.RecordConfig) string + } + tests := []struct { + name string + args args + want string + want1 string + }{ + { + name: "simple", + args: args{ + rc: makeRec("label", "A", "1.1.1.1"), + }, + want: "1.1.1.1", + want1: "1.1.1.1 ttl=300", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, got1 := mkCompareBlobs(tt.args.rc, tt.args.f) + if got != tt.want { + t.Errorf("mkCompareBlobs() got = %q, want %q", got, tt.want) + } + if got1 != tt.want1 { + t.Errorf("mkCompareBlobs() got1 = %q, want %q", got1, tt.want1) + } + }) + } +} diff --git a/pkg/diff2/diff2.go b/pkg/diff2/diff2.go index 4a4b2c84b..c5bc17826 100644 --- a/pkg/diff2/diff2.go +++ b/pkg/diff2/diff2.go @@ -140,12 +140,10 @@ func ByRecord(existing models.Records, dc *models.DomainConfig, compFunc Compara // // Example usage: // -// msgs, changes, err := diff2.ByZone(foundRecords, dc, nil) -// +// msgs, changes, err := diff2.ByZone(foundRecords, dc, nil) // if err != nil { // return nil, err // } -// // if changes { // // Generate a "correction" that uploads the entire zone. // // (dc.Records are the new records for the zone). diff --git a/providers/namedotcom/records.go b/providers/namedotcom/records.go index da180141d..56a3a8878 100644 --- a/providers/namedotcom/records.go +++ b/providers/namedotcom/records.go @@ -105,10 +105,11 @@ func checkNSModifications(dc *models.DomainConfig) { } func toRecord(r *namecom.Record, origin string) *models.RecordConfig { + heapr := r // NB(tlim): Unsure if this is actually needed. rc := &models.RecordConfig{ Type: r.Type, TTL: r.TTL, - Original: r, + Original: heapr, } if !strings.HasSuffix(r.Fqdn, ".") { panic(fmt.Errorf("namedotcom suddenly changed protocol. Bailing. (%v)", r.Fqdn))