From 79547a0341818bbe7e1c648d6ff3c969de90cdac Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Mon, 24 Aug 2015 22:02:32 +0100 Subject: [PATCH 01/16] Add Dedup function Add function that dedups a list of RRs. Work on strings, which adds garbage, but seems to be the least intrusive and takes the last amount of memory. Some fmt changes snook in as well. --- msg.go | 2 +- parse_test.go | 12 +++---- privaterr.go | 2 +- sanitize.go | 76 ++++++++++++++++++++++++++++++++++++++++++++ sanitize_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 167 insertions(+), 8 deletions(-) create mode 100644 sanitize.go create mode 100644 sanitize_test.go diff --git a/msg.go b/msg.go index 59a6ede9..3d13135c 100644 --- a/msg.go +++ b/msg.go @@ -36,7 +36,7 @@ var ( // ErrFqdn indicates that a domain name does not have a closing dot. ErrFqdn error = &Error{err: "domain must be fully qualified"} // ErrId indicates there is a mismatch with the message's ID. - ErrId error = &Error{err: "id mismatch"} + ErrId error = &Error{err: "id mismatch"} // ErrKeyAlg indicates that the algorithm in the key is not valid. ErrKeyAlg error = &Error{err: "bad key algorithm"} ErrKey error = &Error{err: "bad key"} diff --git a/parse_test.go b/parse_test.go index c2579bcc..1bf4d871 100644 --- a/parse_test.go +++ b/parse_test.go @@ -907,11 +907,11 @@ func TestILNP(t *testing.T) { func TestGposEidNimloc(t *testing.T) { dt := map[string]string{ - "444433332222111199990123000000ff. NSAP-PTR foo.bar.com.": "444433332222111199990123000000ff.\t3600\tIN\tNSAP-PTR\tfoo.bar.com.", - "lillee. IN GPOS -32.6882 116.8652 10.0": "lillee.\t3600\tIN\tGPOS\t-32.6882 116.8652 10.0", - "hinault. IN GPOS -22.6882 116.8652 250.0": "hinault.\t3600\tIN\tGPOS\t-22.6882 116.8652 250.0", - "VENERA. IN NIMLOC 75234159EAC457800920": "VENERA.\t3600\tIN\tNIMLOC\t75234159EAC457800920", - "VAXA. IN EID 3141592653589793": "VAXA.\t3600\tIN\tEID\t3141592653589793", + "444433332222111199990123000000ff. NSAP-PTR foo.bar.com.": "444433332222111199990123000000ff.\t3600\tIN\tNSAP-PTR\tfoo.bar.com.", + "lillee. IN GPOS -32.6882 116.8652 10.0": "lillee.\t3600\tIN\tGPOS\t-32.6882 116.8652 10.0", + "hinault. IN GPOS -22.6882 116.8652 250.0": "hinault.\t3600\tIN\tGPOS\t-22.6882 116.8652 250.0", + "VENERA. IN NIMLOC 75234159EAC457800920": "VENERA.\t3600\tIN\tNIMLOC\t75234159EAC457800920", + "VAXA. IN EID 3141592653589793": "VAXA.\t3600\tIN\tEID\t3141592653589793", } for i, o := range dt { rr, err := NewRR(i) @@ -1507,7 +1507,7 @@ func TestPackCAA(t *testing.T) { func TestParseURI(t *testing.T) { lt := map[string]string{ "_http._tcp. IN URI 10 1 \"http://www.example.com/path\"": "_http._tcp.\t3600\tIN\tURI\t10 1 \"http://www.example.com/path\"", - "_http._tcp. IN URI 10 1 \"\"": "_http._tcp.\t3600\tIN\tURI\t10 1 \"\"", + "_http._tcp. IN URI 10 1 \"\"": "_http._tcp.\t3600\tIN\tURI\t10 1 \"\"", } for i, o := range lt { rr, err := NewRR(i) diff --git a/privaterr.go b/privaterr.go index 4b9c325b..7290791b 100644 --- a/privaterr.go +++ b/privaterr.go @@ -49,7 +49,7 @@ func mkPrivateRR(rrtype uint16) *PrivateRR { // Header return the RR header of r. func (r *PrivateRR) Header() *RR_Header { return &r.Hdr } -func (r *PrivateRR) String() string { return r.Hdr.String() + r.Data.String() } +func (r *PrivateRR) String() string { return r.Hdr.String() + r.Data.String() } // Private len and copy parts to satisfy RR interface. func (r *PrivateRR) len() int { return r.Hdr.len() + r.Data.Len() } diff --git a/sanitize.go b/sanitize.go new file mode 100644 index 00000000..93545e94 --- /dev/null +++ b/sanitize.go @@ -0,0 +1,76 @@ +package dns + +// Dedup removes identical RRs from rrs. It preserves the original ordering. +// TODO(miek): CNAME, DNAME 'n stuff. +func Dedup(rrs []RR) []RR { + m := make(map[string]RR) + keys := []string{} + + for _, r := range rrs { + key := normalizedString(r) + keys = append(keys, key) + if _, ok := m[key]; ok { + // Shortest TTL wins. + if m[key].Header().Ttl > r.Header().Ttl { + m[key].Header().Ttl = r.Header().Ttl + } + continue + } + m[key] = r + } + // If the length of the result map equals the amount of RRs we got, + // it means they were all different. We can then just return the original + // rrset. + if len(m) == len(rrs) { + return rrs + } + var i = 0 + for i, _ = range rrs { + if len(m) == 0 { + break + } + // We saved the key for each RR. + delete(m, keys[i]) + } + return rrs[:i] +} + +// normalizedString returns a normalized string from r. The TTL +// is removed and the domain name is lowercased. +func normalizedString(r RR) string { + // A string Go DNS makes has: domainnameTTL... + b := []byte(r.String()) + + // find the first non-escaped tab, then another, so we capture + // where the TTL lives. + esc := false + ttlStart, ttlEnd := 0, 0 + for i, c := range b { + if c == '\\' { + esc = true + continue + } + if esc { + esc = false + continue + } + if c == '\t' { + if ttlStart == 0 { + ttlStart = i + continue + } + if ttlEnd == 0 { + ttlEnd = i + break + } + } + if c >= 'A' && c <= 'Z' { + b[i] = c + 32 + } + } + + // remove TTL. + copy(b[ttlStart:], b[ttlEnd:]) + cut := ttlEnd - ttlStart + return string(b[:len(b)-cut]) +} diff --git a/sanitize_test.go b/sanitize_test.go new file mode 100644 index 00000000..c4ef82a6 --- /dev/null +++ b/sanitize_test.go @@ -0,0 +1,83 @@ +package dns + +import "testing" + +func TestDedup(t *testing.T) { + in := []RR{ + newRR(t, "miek.nl. IN A 127.0.0.1"), + newRR(t, "miek.nl. IN A 127.0.0.1"), + } + out := Dedup(in) + if len(out) != 1 && out[0].String() != "miek.nl. IN A 127.0.0.1" { + dump(out, t) + t.Errorf("dedup failed, expected %d, got %d", 1, len(out)) + } + + in = []RR{} + out = Dedup(in) + if len(out) != 0 { + dump(out, t) + t.Errorf("dedup failed, expected %d, got %d", 0, len(out)) + } + + in = []RR{ + newRR(t, "miEk.nl. 2000 IN A 127.0.0.1"), + newRR(t, "mieK.Nl. 1000 IN A 127.0.0.1"), + } + out = Dedup(in) + if len(out) != 1 { + dump(out, t) + t.Errorf("dedup failed, expected %d, got %d", 2, len(out)) + } + + in = []RR{ + newRR(t, "miek.nl. IN A 127.0.0.1"), + newRR(t, "miek.nl. CH A 127.0.0.1"), + } + out = Dedup(in) + if len(out) != 2 { + dump(out, t) + t.Errorf("dedup failed, expected %d, got %d", 2, len(out)) + } + in = []RR{ + newRR(t, "miek.nl. CH A 127.0.0.1"), + newRR(t, "miek.nl. IN A 127.0.0.1"), + newRR(t, "mIek.Nl. IN A 127.0.0.1"), + } + out = Dedup(in) + if len(out) != 2 { + // TODO(miek): check ordering. + dump(out, t) + t.Errorf("dedup failed, expected %d, got %d", 2, len(out)) + } +} + +func dump(rrs []RR, t *testing.T) { + t.Logf("********\n") + for _, r := range rrs { + t.Logf("%v\n", r) + } +} + +func TestNormalizedString(t *testing.T) { + tests := map[RR]string{ + newRR(t, "mIEk.Nl. 3600 IN A 127.0.0.1"): "miek.nl.\tIN\tA\t127.0.0.1", + newRR(t, "m\\ iek.nL. 3600 IN A 127.0.0.1"): "m\\ iek.nl.\tIN\tA\t127.0.0.1", + newRR(t, "m\\\tIeK.nl. 3600 in A 127.0.0.1"): "m\\tiek.nl.\tIN\tA\t127.0.0.1", + } + for tc, expected := range tests { + a1 := normalizedString(tc) + if a1 != expected { + t.Logf("expected %s, got %s", expected, a1) + t.Fail() + } + } +} + +func newRR(t *testing.T, s string) RR { + r, e := NewRR(s) + if e != nil { + t.Logf("newRR: %s", e) + } + return r +} From 3daa798e14aab8a577efb2813ec40ab6f166f9ec Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 25 Aug 2015 06:52:56 +0100 Subject: [PATCH 02/16] Small tweak, next cleanup the tests --- sanitize.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sanitize.go b/sanitize.go index 93545e94..fd83e4c7 100644 --- a/sanitize.go +++ b/sanitize.go @@ -1,10 +1,11 @@ package dns // Dedup removes identical RRs from rrs. It preserves the original ordering. +// The lowest TTL of any duplicates is used in the remaining one. // TODO(miek): CNAME, DNAME 'n stuff. func Dedup(rrs []RR) []RR { m := make(map[string]RR) - keys := []string{} + keys := make([]string, 0, len(rrs)) for _, r := range rrs { key := normalizedString(r) @@ -19,8 +20,7 @@ func Dedup(rrs []RR) []RR { m[key] = r } // If the length of the result map equals the amount of RRs we got, - // it means they were all different. We can then just return the original - // rrset. + // it means they were all different. We can then just return the original rrset. if len(m) == len(rrs) { return rrs } From 4bca3644804f35be46a7ea4fd3f99247ff9a881a Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 25 Aug 2015 08:39:43 +0100 Subject: [PATCH 03/16] Cleanup the tests and add some doc string --- sanitize.go | 6 +++- sanitize_test.go | 82 ++++++++++++++++++------------------------------ 2 files changed, 36 insertions(+), 52 deletions(-) diff --git a/sanitize.go b/sanitize.go index fd83e4c7..29cfd2f2 100644 --- a/sanitize.go +++ b/sanitize.go @@ -2,7 +2,11 @@ package dns // Dedup removes identical RRs from rrs. It preserves the original ordering. // The lowest TTL of any duplicates is used in the remaining one. -// TODO(miek): CNAME, DNAME 'n stuff. +// +// TODO(miek): This function will be extended to also look for CNAMEs and DNAMEs. +// if found, it will prune rrs from the "other data" that can exist. Example: +// if it finds a: a.miek.nl. CNAME foo, all other RRs with the ownername a.miek.nl. +// will be removed. func Dedup(rrs []RR) []RR { m := make(map[string]RR) keys := make([]string, 0, len(rrs)) diff --git a/sanitize_test.go b/sanitize_test.go index c4ef82a6..fc619dc0 100644 --- a/sanitize_test.go +++ b/sanitize_test.go @@ -3,59 +3,39 @@ package dns import "testing" func TestDedup(t *testing.T) { - in := []RR{ - newRR(t, "miek.nl. IN A 127.0.0.1"), - newRR(t, "miek.nl. IN A 127.0.0.1"), - } - out := Dedup(in) - if len(out) != 1 && out[0].String() != "miek.nl. IN A 127.0.0.1" { - dump(out, t) - t.Errorf("dedup failed, expected %d, got %d", 1, len(out)) + testcases := map[[3]RR]string{ + [...]RR{ + newRR(t, "mIek.nl. IN A 127.0.0.1"), + newRR(t, "mieK.nl. IN A 127.0.0.1"), + newRR(t, "miek.Nl. IN A 127.0.0.1"), + }: "mIek.nl.\t3600\tIN\tA\t127.0.0.1", + [...]RR{ + newRR(t, "miEk.nl. 2000 IN A 127.0.0.1"), + newRR(t, "mieK.Nl. 1000 IN A 127.0.0.1"), + newRR(t, "Miek.nL. 500 IN A 127.0.0.1"), + }: "miEk.nl.\t500\tIN\tA\t127.0.0.1", + [...]RR{ + newRR(t, "miek.nl. IN A 127.0.0.1"), + newRR(t, "miek.nl. CH A 127.0.0.1"), + newRR(t, "miek.nl. IN A 127.0.0.1"), + }: "miek.nl.\t3600\tIN\tA\t127.0.0.1", + [...]RR{ + newRR(t, "miek.nl. CH A 127.0.0.1"), + newRR(t, "miek.nl. IN A 127.0.0.1"), + newRR(t, "miek.nl. IN A 127.0.0.1"), + }: "miek.nl.\t3600\tCH\tA\t127.0.0.1", } - in = []RR{} - out = Dedup(in) - if len(out) != 0 { - dump(out, t) - t.Errorf("dedup failed, expected %d, got %d", 0, len(out)) - } - - in = []RR{ - newRR(t, "miEk.nl. 2000 IN A 127.0.0.1"), - newRR(t, "mieK.Nl. 1000 IN A 127.0.0.1"), - } - out = Dedup(in) - if len(out) != 1 { - dump(out, t) - t.Errorf("dedup failed, expected %d, got %d", 2, len(out)) - } - - in = []RR{ - newRR(t, "miek.nl. IN A 127.0.0.1"), - newRR(t, "miek.nl. CH A 127.0.0.1"), - } - out = Dedup(in) - if len(out) != 2 { - dump(out, t) - t.Errorf("dedup failed, expected %d, got %d", 2, len(out)) - } - in = []RR{ - newRR(t, "miek.nl. CH A 127.0.0.1"), - newRR(t, "miek.nl. IN A 127.0.0.1"), - newRR(t, "mIek.Nl. IN A 127.0.0.1"), - } - out = Dedup(in) - if len(out) != 2 { - // TODO(miek): check ordering. - dump(out, t) - t.Errorf("dedup failed, expected %d, got %d", 2, len(out)) - } -} - -func dump(rrs []RR, t *testing.T) { - t.Logf("********\n") - for _, r := range rrs { - t.Logf("%v\n", r) + for rr, expected := range testcases { + out := Dedup([]RR{rr[0], rr[1], rr[2]}) + if len(out) == 0 || len(out) == 3 { + t.Logf("dedup failed, wrong number of RRs returned") + t.Fail() + } + if o := out[0].String(); o != expected { + t.Logf("dedup failed, expected %s, got %s", expected, o) + t.Fail() + } } } From a2fc07f45ae4442f081fdb2cff50c6cd9810a0b5 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 25 Aug 2015 09:30:43 +0100 Subject: [PATCH 04/16] WIP kill otherdata for cname and dname --- sanitize.go | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/sanitize.go b/sanitize.go index 29cfd2f2..aa294305 100644 --- a/sanitize.go +++ b/sanitize.go @@ -6,12 +6,30 @@ package dns // TODO(miek): This function will be extended to also look for CNAMEs and DNAMEs. // if found, it will prune rrs from the "other data" that can exist. Example: // if it finds a: a.miek.nl. CNAME foo, all other RRs with the ownername a.miek.nl. -// will be removed. +// will be removed. When a DNAME is found all RRs with an ownername below that of +// the DNAME will be removed. func Dedup(rrs []RR) []RR { m := make(map[string]RR) keys := make([]string, 0, len(rrs)) + var cname map[nameClass]bool + var dname map[nameClass]bool for _, r := range rrs { + switch r.Header().Rrtype { + case TypeCNAME: + if cname == nil { + cname = make(map[nameClass]bool) + } + cname[nameClass{r.Header().Name, r.Header().Class}] = true + case TypeDNAME: + if dname == nil { + dname = make(map[nameClass]bool) + } + dname[nameClass{r.Header().Name, r.Header().Class}] = true + default: + if + } + key := normalizedString(r) keys = append(keys, key) if _, ok := m[key]; ok { @@ -39,6 +57,12 @@ func Dedup(rrs []RR) []RR { return rrs[:i] } +// nameClass is used to index the CNAME and DNAME maps. +type nameClass struct { + name string + class uint16 +} + // normalizedString returns a normalized string from r. The TTL // is removed and the domain name is lowercased. func normalizedString(r RR) string { From ec2b1b7be3fbe0574ba62f3cec7504de5f21d355 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 25 Aug 2015 10:32:08 +0100 Subject: [PATCH 05/16] wip --- sanitize.go | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/sanitize.go b/sanitize.go index aa294305..979b5bce 100644 --- a/sanitize.go +++ b/sanitize.go @@ -8,28 +8,28 @@ package dns // if it finds a: a.miek.nl. CNAME foo, all other RRs with the ownername a.miek.nl. // will be removed. When a DNAME is found all RRs with an ownername below that of // the DNAME will be removed. +// Note that the class of the CNAME/DNAME is *not* taken into account. TODO(miek)? func Dedup(rrs []RR) []RR { m := make(map[string]RR) keys := make([]string, 0, len(rrs)) - var cname map[nameClass]bool - var dname map[nameClass]bool - for _, r := range rrs { + /* + make separate step that remove cname, dname switch r.Header().Rrtype { case TypeCNAME: - if cname == nil { - cname = make(map[nameClass]bool) - } - cname[nameClass{r.Header().Name, r.Header().Class}] = true + cname = append(cname, strings.ToLower(r.Header().Name)) case TypeDNAME: - if dname == nil { - dname = make(map[nameClass]bool) - } - dname[nameClass{r.Header().Name, r.Header().Class}] = true + dname = append(dname, strings.ToLower(r.Header().Name)) default: - if + if len(cname) == 0 && len(dname) == 0 { + break + } + if strings.EqualFold } + */ + + for _, r := range rrs { key := normalizedString(r) keys = append(keys, key) if _, ok := m[key]; ok { @@ -57,12 +57,6 @@ func Dedup(rrs []RR) []RR { return rrs[:i] } -// nameClass is used to index the CNAME and DNAME maps. -type nameClass struct { - name string - class uint16 -} - // normalizedString returns a normalized string from r. The TTL // is removed and the domain name is lowercased. func normalizedString(r RR) string { From 350b578e76fced148753349d97734469b9255c73 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 25 Aug 2015 13:52:53 +0100 Subject: [PATCH 06/16] more wip --- sanitize.go | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/sanitize.go b/sanitize.go index 979b5bce..1d801885 100644 --- a/sanitize.go +++ b/sanitize.go @@ -13,22 +13,6 @@ func Dedup(rrs []RR) []RR { m := make(map[string]RR) keys := make([]string, 0, len(rrs)) - /* - make separate step that remove cname, dname - switch r.Header().Rrtype { - case TypeCNAME: - cname = append(cname, strings.ToLower(r.Header().Name)) - case TypeDNAME: - dname = append(dname, strings.ToLower(r.Header().Name)) - default: - if len(cname) == 0 && len(dname) == 0 { - break - } - if strings.EqualFold - } - */ - - for _, r := range rrs { key := normalizedString(r) keys = append(keys, key) @@ -96,3 +80,28 @@ func normalizedString(r RR) string { cut := ttlEnd - ttlStart return string(b[:len(b)-cut]) } + +// dropCNAMEAndDNAME drops records from rrs that are not allowed, taking the rules +// for CNAME and DNAME into account. +func dropCNAMEAndDNAME(rrs []RR) []RR { + ret := make([]RR, 0, len(rrs) + + + return nil + /* + make separate step that remove cname, dname + switch r.Header().Rrtype { + case TypeCNAME: + cname = append(cname, strings.ToLower(r.Header().Name)) + case TypeDNAME: + dname = append(dname, strings.ToLower(r.Header().Name)) + default: + if len(cname) == 0 && len(dname) == 0 { + break + } + if strings.EqualFold + } + */ + + +} From 34e4ada6723291c13e0e9bcd203a36137f77cf79 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 25 Aug 2015 14:28:00 +0100 Subject: [PATCH 07/16] mpre --- sanitize.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanitize.go b/sanitize.go index 1d801885..2bb3223e 100644 --- a/sanitize.go +++ b/sanitize.go @@ -84,7 +84,7 @@ func normalizedString(r RR) string { // dropCNAMEAndDNAME drops records from rrs that are not allowed, taking the rules // for CNAME and DNAME into account. func dropCNAMEAndDNAME(rrs []RR) []RR { - ret := make([]RR, 0, len(rrs) + ret := make([]RR, 0, len(rrs)) return nil From fba9ecdf7987dbf01c1d2e5f74228720bb16752c Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 25 Aug 2015 22:35:10 +0100 Subject: [PATCH 08/16] Rework a little and more tests --- sanitize.go | 89 +++++++++++++++++++++++++++++++----------------- sanitize_test.go | 49 ++++++++++++++++++-------- 2 files changed, 93 insertions(+), 45 deletions(-) diff --git a/sanitize.go b/sanitize.go index 2bb3223e..6924db96 100644 --- a/sanitize.go +++ b/sanitize.go @@ -1,5 +1,7 @@ package dns +import "strings" + // Dedup removes identical RRs from rrs. It preserves the original ordering. // The lowest TTL of any duplicates is used in the remaining one. // @@ -8,13 +10,17 @@ package dns // if it finds a: a.miek.nl. CNAME foo, all other RRs with the ownername a.miek.nl. // will be removed. When a DNAME is found all RRs with an ownername below that of // the DNAME will be removed. -// Note that the class of the CNAME/DNAME is *not* taken into account. TODO(miek)? func Dedup(rrs []RR) []RR { m := make(map[string]RR) keys := make([]string, 0, len(rrs)) + // Save possible cname and dname domainnames. Currently a slice, don't + // expect millions here.. + cname := []string{} + dname := []string{} + for _, r := range rrs { - key := normalizedString(r) + key, end := normalizedString(r) keys = append(keys, key) if _, ok := m[key]; ok { // Shortest TTL wins. @@ -23,6 +29,17 @@ func Dedup(rrs []RR) []RR { } continue } + + if r.Header().Rrtype == TypeCNAME { + // we do end+3 here, so we capture the full domain name *and* + // the class field which mnemonic is always two chars. + cname = append(cname, key[:end+3]) + + } + if r.Header().Rrtype == TypeDNAME { + dname = append(dname, key[:end+3]) + } + m[key] = r } // If the length of the result map equals the amount of RRs we got, @@ -30,20 +47,33 @@ func Dedup(rrs []RR) []RR { if len(m) == len(rrs) { return rrs } - var i = 0 - for i, _ = range rrs { + + ret := make([]RR, 0, len(rrs)) + for i, r := range rrs { + // If keys[i] lives in the map, we should copy it and remove + // it from the map. + if _, ok := m[keys[i]]; ok { + if needsDeletion(r, keys[i], cname, dname) { + delete(m, keys[i]) + continue + } + + delete(m, keys[i]) + ret = append(ret, r) + } + if len(m) == 0 { break } - // We saved the key for each RR. - delete(m, keys[i]) } - return rrs[:i] + + return ret } // normalizedString returns a normalized string from r. The TTL -// is removed and the domain name is lowercased. -func normalizedString(r RR) string { +// is removed and the domain name is lowercased. The returned integer +// is the index where the domain name ends + 1. +func normalizedString(r RR) (string, int) { // A string Go DNS makes has: domainnameTTL... b := []byte(r.String()) @@ -78,30 +108,27 @@ func normalizedString(r RR) string { // remove TTL. copy(b[ttlStart:], b[ttlEnd:]) cut := ttlEnd - ttlStart - return string(b[:len(b)-cut]) + // ttlStart + 3 puts us on the start of the rdata + return string(b[:len(b)-cut]), ttlStart } -// dropCNAMEAndDNAME drops records from rrs that are not allowed, taking the rules -// for CNAME and DNAME into account. -func dropCNAMEAndDNAME(rrs []RR) []RR { - ret := make([]RR, 0, len(rrs)) +func needsDeletion(r RR, s string, cname, dname []string) bool { + if r.Header().Rrtype == TypeCNAME || r.Header().Rrtype == TypeDNAME { + return false + } - - return nil - /* - make separate step that remove cname, dname - switch r.Header().Rrtype { - case TypeCNAME: - cname = append(cname, strings.ToLower(r.Header().Name)) - case TypeDNAME: - dname = append(dname, strings.ToLower(r.Header().Name)) - default: - if len(cname) == 0 && len(dname) == 0 { - break - } - if strings.EqualFold + // For CNAME we can do strings.HasPrefix with s. + // For DNAME we can do strings.Contains with s. + // Either signals a removal of this RR. + for _, c := range cname { + if strings.HasPrefix(s, c) { + return true } - */ - - + } + for _, d := range dname { + if strings.Contains(s, d) { + return true + } + } + return false } diff --git a/sanitize_test.go b/sanitize_test.go index fc619dc0..3a1b4481 100644 --- a/sanitize_test.go +++ b/sanitize_test.go @@ -1,44 +1,65 @@ package dns -import "testing" +import ( + "reflect" + "testing" +) func TestDedup(t *testing.T) { - testcases := map[[3]RR]string{ + // make it []string + testcases := map[[3]RR][]string{ [...]RR{ newRR(t, "mIek.nl. IN A 127.0.0.1"), newRR(t, "mieK.nl. IN A 127.0.0.1"), newRR(t, "miek.Nl. IN A 127.0.0.1"), - }: "mIek.nl.\t3600\tIN\tA\t127.0.0.1", + }: []string{"mIek.nl.\t3600\tIN\tA\t127.0.0.1"}, [...]RR{ newRR(t, "miEk.nl. 2000 IN A 127.0.0.1"), newRR(t, "mieK.Nl. 1000 IN A 127.0.0.1"), newRR(t, "Miek.nL. 500 IN A 127.0.0.1"), - }: "miEk.nl.\t500\tIN\tA\t127.0.0.1", + }: []string{"miEk.nl.\t500\tIN\tA\t127.0.0.1"}, [...]RR{ newRR(t, "miek.nl. IN A 127.0.0.1"), newRR(t, "miek.nl. CH A 127.0.0.1"), newRR(t, "miek.nl. IN A 127.0.0.1"), - }: "miek.nl.\t3600\tIN\tA\t127.0.0.1", + }: []string{"miek.nl.\t3600\tIN\tA\t127.0.0.1", + "miek.nl.\t3600\tCH\tA\t127.0.0.1", + }, [...]RR{ newRR(t, "miek.nl. CH A 127.0.0.1"), newRR(t, "miek.nl. IN A 127.0.0.1"), + newRR(t, "miek.de. IN A 127.0.0.1"), + }: []string{"miek.nl.\t3600\tCH\tA\t127.0.0.1", + "miek.de.\t3600\tIN\tA\t127.0.0.1", + }, + [...]RR{ + newRR(t, "miek.de. IN A 127.0.0.1"), newRR(t, "miek.nl. IN A 127.0.0.1"), - }: "miek.nl.\t3600\tCH\tA\t127.0.0.1", + newRR(t, "miek.nl. IN A 127.0.0.1"), + }: []string{"miek.de.\t3600\tIN\tA\t127.0.0.1", + "miek.de.\t3600\tIN\tA\t127.0.0.1", + }, } for rr, expected := range testcases { out := Dedup([]RR{rr[0], rr[1], rr[2]}) - if len(out) == 0 || len(out) == 3 { - t.Logf("dedup failed, wrong number of RRs returned") - t.Fail() - } - if o := out[0].String(); o != expected { - t.Logf("dedup failed, expected %s, got %s", expected, o) - t.Fail() + if !reflect.DeepEqual(out, expected) { + t.Fatalf("expected %v, got %v", expected, out) } } } +func TestDedupWithCNAME(t *testing.T) { + in := []RR{ + newRR(t, "miek.Nl. CNAME a."), + newRR(t, "miEk.nl. IN A 127.0.0.1"), + newRR(t, "miek.Nl. IN A 127.0.0.1"), + newRR(t, "miek.de. IN A 127.0.0.1"), + } + out := Dedup(in) + t.Logf("%+v\n", out) +} + func TestNormalizedString(t *testing.T) { tests := map[RR]string{ newRR(t, "mIEk.Nl. 3600 IN A 127.0.0.1"): "miek.nl.\tIN\tA\t127.0.0.1", @@ -46,7 +67,7 @@ func TestNormalizedString(t *testing.T) { newRR(t, "m\\\tIeK.nl. 3600 in A 127.0.0.1"): "m\\tiek.nl.\tIN\tA\t127.0.0.1", } for tc, expected := range tests { - a1 := normalizedString(tc) + a1, _ := normalizedString(tc) if a1 != expected { t.Logf("expected %s, got %s", expected, a1) t.Fail() From 0ce87cb3da0af1a764c168feed3992cfec503c9d Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Wed, 26 Aug 2015 07:34:51 +0100 Subject: [PATCH 09/16] More and better tests --- sanitize.go | 2 ++ sanitize_test.go | 54 +++++++++++++++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/sanitize.go b/sanitize.go index 6924db96..584b16d4 100644 --- a/sanitize.go +++ b/sanitize.go @@ -54,6 +54,8 @@ func Dedup(rrs []RR) []RR { // it from the map. if _, ok := m[keys[i]]; ok { if needsDeletion(r, keys[i], cname, dname) { + // It the RR is masked by an CNAME or DNAME, we only + // delete it from the map, but don't copy it. delete(m, keys[i]) continue } diff --git a/sanitize_test.go b/sanitize_test.go index 3a1b4481..09c1bd05 100644 --- a/sanitize_test.go +++ b/sanitize_test.go @@ -1,9 +1,6 @@ package dns -import ( - "reflect" - "testing" -) +import "testing" func TestDedup(t *testing.T) { // make it []string @@ -30,34 +27,57 @@ func TestDedup(t *testing.T) { newRR(t, "miek.nl. IN A 127.0.0.1"), newRR(t, "miek.de. IN A 127.0.0.1"), }: []string{"miek.nl.\t3600\tCH\tA\t127.0.0.1", + "miek.nl.\t3600\tIN\tA\t127.0.0.1", "miek.de.\t3600\tIN\tA\t127.0.0.1", }, [...]RR{ newRR(t, "miek.de. IN A 127.0.0.1"), - newRR(t, "miek.nl. IN A 127.0.0.1"), - newRR(t, "miek.nl. IN A 127.0.0.1"), + newRR(t, "miek.nl. 200 IN A 127.0.0.1"), + newRR(t, "miek.nl. 300 IN A 127.0.0.1"), }: []string{"miek.de.\t3600\tIN\tA\t127.0.0.1", - "miek.de.\t3600\tIN\tA\t127.0.0.1", + "miek.nl.\t200\tIN\tA\t127.0.0.1", }, } + T := 0 for rr, expected := range testcases { out := Dedup([]RR{rr[0], rr[1], rr[2]}) - if !reflect.DeepEqual(out, expected) { - t.Fatalf("expected %v, got %v", expected, out) + for i, o := range out { + if o.String() != expected[i] { + t.Fatalf("test %d, expected %v, got %v", T, expected[i], o.String()) + } } + T++ } } -func TestDedupWithCNAME(t *testing.T) { - in := []RR{ - newRR(t, "miek.Nl. CNAME a."), - newRR(t, "miEk.nl. IN A 127.0.0.1"), - newRR(t, "miek.Nl. IN A 127.0.0.1"), - newRR(t, "miek.de. IN A 127.0.0.1"), +func TestDedupWithCNAMEDNAME(t *testing.T) { + testcases := map[[4]RR][]string{ + [...]RR{ + newRR(t, "miek.Nl. CNAME a."), + newRR(t, "miEk.nl. IN A 127.0.0.1"), + newRR(t, "miek.Nl. IN A 127.0.0.1"), + newRR(t, "miek.de. IN A 127.0.0.1"), + }: []string{"miek.Nl.\t3600\tIN\tCNAME\ta.", + "miek.de.\t3600\tIN\tA\t127.0.0.1"}, + [...]RR{ + newRR(t, "Miek.nl. CNAME a."), + newRR(t, "mIek.nl. CNAME a."), + newRR(t, "miEk.nl. CNAME a."), + newRR(t, "mieK.nl. CNAME a."), + }: []string{"Miek.nl.\t3600\tIN\tCNAME\ta."}, + } + + T := 0 + for rr, expected := range testcases { + out := Dedup([]RR{rr[0], rr[1], rr[2]}) + for i, o := range out { + if o.String() != expected[i] { + t.Fatalf("test %d, expected %v, got %v", T, expected[i], o.String()) + } + } + T++ } - out := Dedup(in) - t.Logf("%+v\n", out) } func TestNormalizedString(t *testing.T) { From aec78ad5e40bc9ad75febc70ceda97c10189cabd Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Wed, 26 Aug 2015 07:35:55 +0100 Subject: [PATCH 10/16] more docs --- sanitize.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sanitize.go b/sanitize.go index 584b16d4..9574847e 100644 --- a/sanitize.go +++ b/sanitize.go @@ -114,6 +114,8 @@ func normalizedString(r RR) (string, int) { return string(b[:len(b)-cut]), ttlStart } +// needsDeletion checks if the RR is masked by either a CNAME or a DNAME. +// If so it return true. func needsDeletion(r RR, s string, cname, dname []string) bool { if r.Header().Rrtype == TypeCNAME || r.Header().Rrtype == TypeDNAME { return false From e1d5f172aed60731336a003bd124ee52507d58f4 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Wed, 26 Aug 2015 11:26:45 +0100 Subject: [PATCH 11/16] More tests --- sanitize.go | 2 ++ sanitize_test.go | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/sanitize.go b/sanitize.go index 9574847e..b4f555d9 100644 --- a/sanitize.go +++ b/sanitize.go @@ -117,6 +117,8 @@ func normalizedString(r RR) (string, int) { // needsDeletion checks if the RR is masked by either a CNAME or a DNAME. // If so it return true. func needsDeletion(r RR, s string, cname, dname []string) bool { + // TODO(miek): fix. + // This is too broad, but we have to be care full not to delete ourselves. if r.Header().Rrtype == TypeCNAME || r.Header().Rrtype == TypeDNAME { return false } diff --git a/sanitize_test.go b/sanitize_test.go index 09c1bd05..4b7c10ce 100644 --- a/sanitize_test.go +++ b/sanitize_test.go @@ -66,11 +66,31 @@ func TestDedupWithCNAMEDNAME(t *testing.T) { newRR(t, "miEk.nl. CNAME a."), newRR(t, "mieK.nl. CNAME a."), }: []string{"Miek.nl.\t3600\tIN\tCNAME\ta."}, + [...]RR{ + newRR(t, "miek.nl. CNAME a."), + newRR(t, "a.miek.nl. CNAME a."), + newRR(t, "a.miek.nl. CNAME a."), + newRR(t, "a.miek.nl. CNAME a."), + }: []string{"miek.nl.\t3600\tIN\tCNAME\ta.", + "a.miek.nl.\t3600\tIN\tCNAME\ta."}, + [...]RR{ + newRR(t, "miek.nl. DNAME a."), + newRR(t, "a.miek.nl. CNAME a."), + newRR(t, "b.miek.nl. IN A 127.0.0.1"), + newRR(t, "a.miek.de. IN A 127.0.0.1"), + }: []string{"miek.nl.\t3600\tIN\tDNAME\ta.", + "a.miek.de.\t3600\tIN\tA\t127.0.0.1"}, + [...]RR{ + newRR(t, "miek.nl. DNAME a."), + newRR(t, "a.miek.nl. DNAME a."), + newRR(t, "b.miek.nl. DNAME b."), + newRR(t, "a.b.miek.nl. DNAME a.b"), + }: []string{"miek.nl.\t3600\tIN\tDNAME\ta."}, } T := 0 for rr, expected := range testcases { - out := Dedup([]RR{rr[0], rr[1], rr[2]}) + out := Dedup([]RR{rr[0], rr[1], rr[2], rr[3]}) for i, o := range out { if o.String() != expected[i] { t.Fatalf("test %d, expected %v, got %v", T, expected[i], o.String()) From ac493072c738bc9c0dcb9a51f47b0b35ac7813e9 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Wed, 26 Aug 2015 12:19:22 +0100 Subject: [PATCH 12/16] Correctly handle CNAME/DNAMEs --- sanitize.go | 17 ++++++++++------- sanitize_test.go | 10 ++++------ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/sanitize.go b/sanitize.go index b4f555d9..cbe8c291 100644 --- a/sanitize.go +++ b/sanitize.go @@ -44,7 +44,8 @@ func Dedup(rrs []RR) []RR { } // If the length of the result map equals the amount of RRs we got, // it means they were all different. We can then just return the original rrset. - if len(m) == len(rrs) { + // We can only do this when we haven't found a CNAME or DNAME. + if len(m) == len(rrs) && len(cname) == 0 && len(dname) == 0 { return rrs } @@ -117,22 +118,24 @@ func normalizedString(r RR) (string, int) { // needsDeletion checks if the RR is masked by either a CNAME or a DNAME. // If so it return true. func needsDeletion(r RR, s string, cname, dname []string) bool { - // TODO(miek): fix. - // This is too broad, but we have to be care full not to delete ourselves. - if r.Header().Rrtype == TypeCNAME || r.Header().Rrtype == TypeDNAME { - return false - } - // For CNAME we can do strings.HasPrefix with s. // For DNAME we can do strings.Contains with s. // Either signals a removal of this RR. for _, c := range cname { if strings.HasPrefix(s, c) { + if r.Header().Rrtype == TypeCNAME { + // don't delete yourself + continue + } return true } } for _, d := range dname { if strings.Contains(s, d) { + if r.Header().Rrtype == TypeDNAME && strings.HasPrefix(s, d) { + // don't delete yourself + continue + } return true } } diff --git a/sanitize_test.go b/sanitize_test.go index 4b7c10ce..1eae1a30 100644 --- a/sanitize_test.go +++ b/sanitize_test.go @@ -39,15 +39,13 @@ func TestDedup(t *testing.T) { }, } - T := 0 for rr, expected := range testcases { out := Dedup([]RR{rr[0], rr[1], rr[2]}) for i, o := range out { if o.String() != expected[i] { - t.Fatalf("test %d, expected %v, got %v", T, expected[i], o.String()) + t.Fatalf("expected %v, got %v", expected[i], o.String()) } } - T++ } } @@ -88,18 +86,18 @@ func TestDedupWithCNAMEDNAME(t *testing.T) { }: []string{"miek.nl.\t3600\tIN\tDNAME\ta."}, } - T := 0 for rr, expected := range testcases { out := Dedup([]RR{rr[0], rr[1], rr[2], rr[3]}) for i, o := range out { if o.String() != expected[i] { - t.Fatalf("test %d, expected %v, got %v", T, expected[i], o.String()) + t.Fatalf("expected %v, got %v", expected[i], o.String()) } } - T++ } } +// BenchMark test as well TODO(miek) + func TestNormalizedString(t *testing.T) { tests := map[RR]string{ newRR(t, "mIEk.Nl. 3600 IN A 127.0.0.1"): "miek.nl.\tIN\tA\t127.0.0.1", From 6cf1a0db58f3456b5465c4f186f5957719663964 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Wed, 26 Aug 2015 12:19:38 +0100 Subject: [PATCH 13/16] fmt --- sanitize_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sanitize_test.go b/sanitize_test.go index 1eae1a30..d5ff83ce 100644 --- a/sanitize_test.go +++ b/sanitize_test.go @@ -70,14 +70,14 @@ func TestDedupWithCNAMEDNAME(t *testing.T) { newRR(t, "a.miek.nl. CNAME a."), newRR(t, "a.miek.nl. CNAME a."), }: []string{"miek.nl.\t3600\tIN\tCNAME\ta.", - "a.miek.nl.\t3600\tIN\tCNAME\ta."}, + "a.miek.nl.\t3600\tIN\tCNAME\ta."}, [...]RR{ newRR(t, "miek.nl. DNAME a."), newRR(t, "a.miek.nl. CNAME a."), newRR(t, "b.miek.nl. IN A 127.0.0.1"), newRR(t, "a.miek.de. IN A 127.0.0.1"), }: []string{"miek.nl.\t3600\tIN\tDNAME\ta.", - "a.miek.de.\t3600\tIN\tA\t127.0.0.1"}, + "a.miek.de.\t3600\tIN\tA\t127.0.0.1"}, [...]RR{ newRR(t, "miek.nl. DNAME a."), newRR(t, "a.miek.nl. DNAME a."), From 0206070abf6deb7bb7491168c051a67982538ba8 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Wed, 26 Aug 2015 14:13:14 +0100 Subject: [PATCH 14/16] Address commments from Alex --- sanitize.go | 101 +++++++++-------------------------------------- sanitize_test.go | 58 +++++---------------------- 2 files changed, 28 insertions(+), 131 deletions(-) diff --git a/sanitize.go b/sanitize.go index cbe8c291..d10d6edb 100644 --- a/sanitize.go +++ b/sanitize.go @@ -1,26 +1,13 @@ package dns -import "strings" - // Dedup removes identical RRs from rrs. It preserves the original ordering. // The lowest TTL of any duplicates is used in the remaining one. -// -// TODO(miek): This function will be extended to also look for CNAMEs and DNAMEs. -// if found, it will prune rrs from the "other data" that can exist. Example: -// if it finds a: a.miek.nl. CNAME foo, all other RRs with the ownername a.miek.nl. -// will be removed. When a DNAME is found all RRs with an ownername below that of -// the DNAME will be removed. func Dedup(rrs []RR) []RR { m := make(map[string]RR) keys := make([]string, 0, len(rrs)) - // Save possible cname and dname domainnames. Currently a slice, don't - // expect millions here.. - cname := []string{} - dname := []string{} - for _, r := range rrs { - key, end := normalizedString(r) + key := normalizedString(r) keys = append(keys, key) if _, ok := m[key]; ok { // Shortest TTL wins. @@ -30,37 +17,18 @@ func Dedup(rrs []RR) []RR { continue } - if r.Header().Rrtype == TypeCNAME { - // we do end+3 here, so we capture the full domain name *and* - // the class field which mnemonic is always two chars. - cname = append(cname, key[:end+3]) - - } - if r.Header().Rrtype == TypeDNAME { - dname = append(dname, key[:end+3]) - } - m[key] = r } // If the length of the result map equals the amount of RRs we got, // it means they were all different. We can then just return the original rrset. - // We can only do this when we haven't found a CNAME or DNAME. - if len(m) == len(rrs) && len(cname) == 0 && len(dname) == 0 { + if len(m) == len(rrs) { return rrs } ret := make([]RR, 0, len(rrs)) for i, r := range rrs { - // If keys[i] lives in the map, we should copy it and remove - // it from the map. + // If keys[i] lives in the map, we should copy and remove it. if _, ok := m[keys[i]]; ok { - if needsDeletion(r, keys[i], cname, dname) { - // It the RR is masked by an CNAME or DNAME, we only - // delete it from the map, but don't copy it. - delete(m, keys[i]) - continue - } - delete(m, keys[i]) ret = append(ret, r) } @@ -74,70 +42,37 @@ func Dedup(rrs []RR) []RR { } // normalizedString returns a normalized string from r. The TTL -// is removed and the domain name is lowercased. The returned integer -// is the index where the domain name ends + 1. -func normalizedString(r RR) (string, int) { +// is removed and the domain name is lowercased. We go from this: +// DomainNameTTLCLASSTYPERDATA to: +// lowercasenameCLASSTYPE... +func normalizedString(r RR) string { // A string Go DNS makes has: domainnameTTL... b := []byte(r.String()) - // find the first non-escaped tab, then another, so we capture - // where the TTL lives. + // find the first non-escaped tab, then another, so we capture where the TTL lives. esc := false ttlStart, ttlEnd := 0, 0 - for i, c := range b { - if c == '\\' { - esc = true - continue - } - if esc { - esc = false - continue - } - if c == '\t' { + for i := 0; i < len(b) && ttlEnd == 0; i++ { + switch { + case b[i] == '\\': + esc = !esc + case b[i] == '\t' && !esc: if ttlStart == 0 { ttlStart = i continue } if ttlEnd == 0 { ttlEnd = i - break } - } - if c >= 'A' && c <= 'Z' { - b[i] = c + 32 + case b[i] >= 'A' && b[i] <= 'Z' && !esc: + b[i] += 32 + default: + esc = false } } // remove TTL. copy(b[ttlStart:], b[ttlEnd:]) cut := ttlEnd - ttlStart - // ttlStart + 3 puts us on the start of the rdata - return string(b[:len(b)-cut]), ttlStart -} - -// needsDeletion checks if the RR is masked by either a CNAME or a DNAME. -// If so it return true. -func needsDeletion(r RR, s string, cname, dname []string) bool { - // For CNAME we can do strings.HasPrefix with s. - // For DNAME we can do strings.Contains with s. - // Either signals a removal of this RR. - for _, c := range cname { - if strings.HasPrefix(s, c) { - if r.Header().Rrtype == TypeCNAME { - // don't delete yourself - continue - } - return true - } - } - for _, d := range dname { - if strings.Contains(s, d) { - if r.Header().Rrtype == TypeDNAME && strings.HasPrefix(s, d) { - // don't delete yourself - continue - } - return true - } - } - return false + return string(b[:len(b)-cut]) } diff --git a/sanitize_test.go b/sanitize_test.go index d5ff83ce..eb0248a8 100644 --- a/sanitize_test.go +++ b/sanitize_test.go @@ -49,55 +49,17 @@ func TestDedup(t *testing.T) { } } -func TestDedupWithCNAMEDNAME(t *testing.T) { - testcases := map[[4]RR][]string{ - [...]RR{ - newRR(t, "miek.Nl. CNAME a."), - newRR(t, "miEk.nl. IN A 127.0.0.1"), - newRR(t, "miek.Nl. IN A 127.0.0.1"), - newRR(t, "miek.de. IN A 127.0.0.1"), - }: []string{"miek.Nl.\t3600\tIN\tCNAME\ta.", - "miek.de.\t3600\tIN\tA\t127.0.0.1"}, - [...]RR{ - newRR(t, "Miek.nl. CNAME a."), - newRR(t, "mIek.nl. CNAME a."), - newRR(t, "miEk.nl. CNAME a."), - newRR(t, "mieK.nl. CNAME a."), - }: []string{"Miek.nl.\t3600\tIN\tCNAME\ta."}, - [...]RR{ - newRR(t, "miek.nl. CNAME a."), - newRR(t, "a.miek.nl. CNAME a."), - newRR(t, "a.miek.nl. CNAME a."), - newRR(t, "a.miek.nl. CNAME a."), - }: []string{"miek.nl.\t3600\tIN\tCNAME\ta.", - "a.miek.nl.\t3600\tIN\tCNAME\ta."}, - [...]RR{ - newRR(t, "miek.nl. DNAME a."), - newRR(t, "a.miek.nl. CNAME a."), - newRR(t, "b.miek.nl. IN A 127.0.0.1"), - newRR(t, "a.miek.de. IN A 127.0.0.1"), - }: []string{"miek.nl.\t3600\tIN\tDNAME\ta.", - "a.miek.de.\t3600\tIN\tA\t127.0.0.1"}, - [...]RR{ - newRR(t, "miek.nl. DNAME a."), - newRR(t, "a.miek.nl. DNAME a."), - newRR(t, "b.miek.nl. DNAME b."), - newRR(t, "a.b.miek.nl. DNAME a.b"), - }: []string{"miek.nl.\t3600\tIN\tDNAME\ta."}, +func BenchmarkDedup(b *testing.B) { + rrs := []RR{ + newRR(nil, "miEk.nl. 2000 IN A 127.0.0.1"), + newRR(nil, "mieK.Nl. 1000 IN A 127.0.0.1"), + newRR(nil, "Miek.nL. 500 IN A 127.0.0.1"), } - - for rr, expected := range testcases { - out := Dedup([]RR{rr[0], rr[1], rr[2], rr[3]}) - for i, o := range out { - if o.String() != expected[i] { - t.Fatalf("expected %v, got %v", expected[i], o.String()) - } - } + for i := 0; i < b.N; i++ { + Dedup(rrs) } } -// BenchMark test as well TODO(miek) - func TestNormalizedString(t *testing.T) { tests := map[RR]string{ newRR(t, "mIEk.Nl. 3600 IN A 127.0.0.1"): "miek.nl.\tIN\tA\t127.0.0.1", @@ -105,9 +67,9 @@ func TestNormalizedString(t *testing.T) { newRR(t, "m\\\tIeK.nl. 3600 in A 127.0.0.1"): "m\\tiek.nl.\tIN\tA\t127.0.0.1", } for tc, expected := range tests { - a1, _ := normalizedString(tc) - if a1 != expected { - t.Logf("expected %s, got %s", expected, a1) + n := normalizedString(tc) + if n != expected { + t.Logf("expected %s, got %s", expected, n) t.Fail() } } From fb82119b6631233ebeb3199ea8adda4f742ddfdd Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Mon, 31 Aug 2015 16:48:28 +0100 Subject: [PATCH 15/16] less memory use --- sanitize.go | 27 +++++++++++++++++---------- sanitize_test.go | 5 +++-- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/sanitize.go b/sanitize.go index d10d6edb..4df061f5 100644 --- a/sanitize.go +++ b/sanitize.go @@ -1,14 +1,20 @@ package dns // Dedup removes identical RRs from rrs. It preserves the original ordering. -// The lowest TTL of any duplicates is used in the remaining one. -func Dedup(rrs []RR) []RR { - m := make(map[string]RR) - keys := make([]string, 0, len(rrs)) +// The lowest TTL of any duplicates is used in the remaining one. Dedup modifies +// rrs. +// m is used to store the RRs temporay. If it is nil a new map will be allocated. +func Dedup(rrs []RR, m map[string]RR) []RR { + if m == nil { + m = make(map[string]RR) + } + // We need a (ordered) slice of keys to preserve the original ordering. + // Otherwise we could just range of the map directly. + keys := make([]*string, 0, len(rrs)) for _, r := range rrs { key := normalizedString(r) - keys = append(keys, key) + keys = append(keys, &key) if _, ok := m[key]; ok { // Shortest TTL wins. if m[key].Header().Ttl > r.Header().Ttl { @@ -25,12 +31,13 @@ func Dedup(rrs []RR) []RR { return rrs } - ret := make([]RR, 0, len(rrs)) + j := 0 for i, r := range rrs { // If keys[i] lives in the map, we should copy and remove it. - if _, ok := m[keys[i]]; ok { - delete(m, keys[i]) - ret = append(ret, r) + if _, ok := m[*keys[i]]; ok { + delete(m, *keys[i]) + rrs[j] = r + j++ } if len(m) == 0 { @@ -38,7 +45,7 @@ func Dedup(rrs []RR) []RR { } } - return ret + return rrs[:j] } // normalizedString returns a normalized string from r. The TTL diff --git a/sanitize_test.go b/sanitize_test.go index eb0248a8..79335727 100644 --- a/sanitize_test.go +++ b/sanitize_test.go @@ -40,7 +40,7 @@ func TestDedup(t *testing.T) { } for rr, expected := range testcases { - out := Dedup([]RR{rr[0], rr[1], rr[2]}) + out := Dedup([]RR{rr[0], rr[1], rr[2]}, nil) for i, o := range out { if o.String() != expected[i] { t.Fatalf("expected %v, got %v", expected[i], o.String()) @@ -55,8 +55,9 @@ func BenchmarkDedup(b *testing.B) { newRR(nil, "mieK.Nl. 1000 IN A 127.0.0.1"), newRR(nil, "Miek.nL. 500 IN A 127.0.0.1"), } + m := make(map[string]RR) for i := 0; i < b.N; i++ { - Dedup(rrs) + Dedup(rrs,m ) } } From 957df535677fe2161a0948b08d625f8e9fa6395b Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Mon, 31 Aug 2015 16:50:42 +0100 Subject: [PATCH 16/16] some memory optimisations --- sanitize.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sanitize.go b/sanitize.go index 4df061f5..b489f3f0 100644 --- a/sanitize.go +++ b/sanitize.go @@ -8,8 +8,7 @@ func Dedup(rrs []RR, m map[string]RR) []RR { if m == nil { m = make(map[string]RR) } - // We need a (ordered) slice of keys to preserve the original ordering. - // Otherwise we could just range of the map directly. + // Save the keys, so we don't have to call normalizedString twice. keys := make([]*string, 0, len(rrs)) for _, r := range rrs {