pkg/replace: make it more efficient. (#2544)

* pkg/replace: make it more efficient.

Remove the map that is allocated on every write and make it more static,
but just defining a function that gets called for a label and returns
its value.

Remove the interface definition and just implement what is needed in our
case. Add benchmark test for replace as well.

Extend metadata test to test multiple values (pretty sure this didn't
work, but there wasn't a test for it, so can't be sure).

Update all callers to use it - concurrent use should be fine as we pass
everything by value.

Benchmarks in replacer:

new: BenchmarkReplacer-4   300000      4717 ns/op     240 B/op       8 allocs/op
old: BenchmarkReplacer-4   300000      4368 ns/op     384 B/op      11 allocs/op

Added benchmark function to the old code to test it.

~~~
func BenchmarkReplacer(b *testing.B) {
	w := dnstest.NewRecorder(&test.ResponseWriter{})
	r := new(dns.Msg)
	r.SetQuestion("example.org.", dns.TypeHINFO)
	r.MsgHdr.AuthenticatedData = true
	b.ResetTimer()
	b.ReportAllocs()
	repl := New(context.TODO(), r, w, "")
	for i := 0; i < b.N; i++ {
		repl.Replace("{type} {name} {size}")
	}
}
~~~

New code contains (of course a different one). The amount of ops is
more, which might be good to look at some more. For all the allocations
is seems it was quite performant.

This looks to be 50% faster, and there is less allocations in log
plugin:

old: BenchmarkLogged-4   	   20000	     70526 ns/op
new: BenchmarkLogged-4   	   30000	     57558 ns/op

Signed-off-by: Miek Gieben <miek@miek.nl>

* Stickler bot

Signed-off-by: Miek Gieben <miek@miek.nl>

* Improve test coverage

Signed-off-by: Miek Gieben <miek@miek.nl>

* typo

Signed-off-by: Miek Gieben <miek@miek.nl>

* Add test for malformed log lines

Signed-off-by: Miek Gieben <miek@miek.nl>

1 files changed, 112 insertions, 54 deletions

diff --git a/plugin/pkg/replacer/replacer_test.go b/plugin/pkg/replacer/replacer_test.go
index a813057d8..8f86e91a5 100644
--- a/plugin/pkg/replacer/replacer_test.go
+++ b/plugin/pkg/replacer/replacer_test.go
@@ -2,69 +2,99 @@ package replacer
 
 import (
 	"context"
-	"strings"
 	"testing"
 
 	"github.com/coredns/coredns/plugin/metadata"
-	"github.com/coredns/coredns/request"
-
 	"github.com/coredns/coredns/plugin/pkg/dnstest"
 	"github.com/coredns/coredns/plugin/test"
+	"github.com/coredns/coredns/request"
 
 	"github.com/miekg/dns"
 )
 
-func TestNewReplacer(t *testing.T) {
+func TestReplacer(t *testing.T) {
 	w := dnstest.NewRecorder(&test.ResponseWriter{})
-
 	r := new(dns.Msg)
 	r.SetQuestion("example.org.", dns.TypeHINFO)
 	r.MsgHdr.AuthenticatedData = true
+	state := request.Request{W: w, Req: r}
 
-	replaceValues := New(context.TODO(), r, w, "")
+	replacer := New()
 
-	switch v := replaceValues.(type) {
-	case replacer:
+	if x := replacer.Replace(context.TODO(), state, nil, "{type}"); x != "HINFO" {
+		t.Errorf("Expected type to be HINFO, got %q", x)
+	}
+	if x := replacer.Replace(context.TODO(), state, nil, "{name}"); x != "example.org." {
+		t.Errorf("Expected request name to be example.org., got %q", x)
+	}
+	if x := replacer.Replace(context.TODO(), state, nil, "{size}"); x != "29" {
+		t.Errorf("Expected size to be 29, got %q", x)
+	}
+}
 
-		if v.replacements["{type}"] != "HINFO" {
-			t.Errorf("Expected type to be HINFO, got %q", v.replacements["{type}"])
-		}
-		if v.replacements["{name}"] != "example.org." {
-			t.Errorf("Expected request name to be example.org., got %q", v.replacements["{name}"])
-		}
-		if v.replacements["{size}"] != "29" { // size of request
-			t.Errorf("Expected size to be 29, got %q", v.replacements["{size}"])
+func TestLabels(t *testing.T) {
+	w := dnstest.NewRecorder(&test.ResponseWriter{})
+	r := new(dns.Msg)
+	r.SetQuestion("example.org.", dns.TypeHINFO)
+	r.Id = 1053
+	r.AuthenticatedData = true
+	r.CheckingDisabled = true
+	w.WriteMsg(r)
+	state := request.Request{W: w, Req: r}
+
+	replacer := New()
+	ctx := context.TODO()
+
+	// This couples the test very tightly to the code, but so be it.
+	expect := map[string]string{
+		"{type}":                    "HINFO",
+		"{name}":                    "example.org.",
+		"{class}":                   "IN",
+		"{proto}":                   "udp",
+		"{size}":                    "29",
+		"{remote}":                  "10.240.0.1",
+		"{port}":                    "40212",
+		"{local}":                   "127.0.0.1",
+		headerReplacer + "id}":      "1053",
+		headerReplacer + "opcode}":  "0",
+		headerReplacer + "do}":      "false",
+		headerReplacer + "bufsize}": "512",
+		"{rcode}":                   "NOERROR",
+		"{rsize}":                   "29",
+		"{duration}":                "0",
+		headerReplacer + "rrflags}": "rd,ad,cd",
+	}
+	if len(expect) != len(labels) {
+		t.Fatalf("Expect %d labels, got %d", len(expect), len(labels))
+	}
+
+	for _, lbl := range labels {
+		repl := replacer.Replace(ctx, state, w, lbl)
+		if lbl == "{duration}" {
+			if repl[len(repl)-1] != 's' {
+				t.Errorf("Expected seconds, got %q", repl)
+			}
+			continue
 		}
-		if !strings.Contains(v.replacements["{duration}"], "s") {
-			t.Errorf("Expected units of time to be in seconds")
+		if repl != expect[lbl] {
+			t.Errorf("Expected value %q, got %q", expect[lbl], repl)
 		}
-
-	default:
-		t.Fatal("Return Value from New Replacer expected pass type assertion into a replacer type\n")
 	}
 }
 
-func TestSet(t *testing.T) {
+func BenchmarkReplacer(b *testing.B) {
 	w := dnstest.NewRecorder(&test.ResponseWriter{})
-
 	r := new(dns.Msg)
 	r.SetQuestion("example.org.", dns.TypeHINFO)
 	r.MsgHdr.AuthenticatedData = true
+	state := request.Request{W: w, Req: r}
 
-	repl := New(context.TODO(), r, w, "")
+	b.ResetTimer()
+	b.ReportAllocs()
 
-	repl.Set("name", "coredns.io.")
-	repl.Set("type", "A")
-	repl.Set("size", "20")
-
-	if repl.Replace("This name is {name}") != "This name is coredns.io." {
-		t.Error("Expected name replacement failed")
-	}
-	if repl.Replace("This type is {type}") != "This type is A" {
-		t.Error("Expected type replacement failed")
-	}
-	if repl.Replace("The request size is {size}") != "The request size is 20" {
-		t.Error("Expected size replacement failed")
+	replacer := New()
+	for i := 0; i < b.N; i++ {
+		replacer.Replace(context.TODO(), state, nil, "{type} {name} {size}")
 	}
 }
 
@@ -87,46 +117,74 @@ func (m *testHandler) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns
 }
 
 func TestMetadataReplacement(t *testing.T) {
+	tests := []struct {
+		expr   string
+		result string
+	}{
+		{"{/test/meta2}", "two"},
+		{"{/test/meta2} {/test/key4}", "two -"},
+		{"{/test/meta2} {/test/meta3}", "two three"},
+	}
+
+	next := &testHandler{}
+	m := metadata.Metadata{
+		Zones: []string{"."},
+		Providers: []metadata.Provider{
+			testProvider{"test/meta2": func() string { return "two" }},
+			testProvider{"test/meta3": func() string { return "three" }},
+		},
+		Next: next,
+	}
+
+	m.ServeDNS(context.TODO(), &test.ResponseWriter{}, new(dns.Msg))
+	ctx := next.ctx // important because the m.ServeDNS has only now populated the context
+
+	w := dnstest.NewRecorder(&test.ResponseWriter{})
+	r := new(dns.Msg)
+	r.SetQuestion("example.org.", dns.TypeHINFO)
 
-	mdata := testProvider{
-		"test/key2": func() string { return "two" },
+	repl := New()
+	state := request.Request{W: w, Req: r}
+
+	for i, ts := range tests {
+		r := repl.Replace(ctx, state, nil, ts.expr)
+		if r != ts.result {
+			t.Errorf("Test %d - expr : %s, expected %q, got %q", i, ts.expr, ts.result, r)
+		}
 	}
+}
 
+func TestMetadataMalformed(t *testing.T) {
 	tests := []struct {
 		expr   string
 		result string
 	}{
-		{"{name}", "example.org."},
-		{"{/test/key2}", "two"},
-		{"TYPE={type}, NAME={name}, BUFSIZE={>bufsize}, WHAT={/test/key2} .. and more", "TYPE=HINFO, NAME=example.org., BUFSIZE=512, WHAT=two .. and more"},
-		{"{/test/key2}{/test/key4}", "two-"},
-		{"{test/key2", "{test/key2"}, // if last } is missing, the end of format is considered static text
-		{"{/test-key2}", "-"},        // everything that is not a placeholder for log or a metadata label is invalid
+		{"{/test/meta2", "{/test/meta2"},
+		{"{test/meta2} {/test/meta4}", "{test/meta2} -"},
+		{"{test}", "{test}"},
 	}
 
-	next := &testHandler{} // fake handler which stores the resulting context
+	next := &testHandler{}
 	m := metadata.Metadata{
 		Zones:     []string{"."},
-		Providers: []metadata.Provider{mdata},
+		Providers: []metadata.Provider{testProvider{"test/meta2": func() string { return "two" }}},
 		Next:      next,
 	}
 
-	ctx := context.TODO()
-	m.ServeDNS(ctx, &test.ResponseWriter{}, new(dns.Msg))
-	nctx := next.ctx
+	m.ServeDNS(context.TODO(), &test.ResponseWriter{}, new(dns.Msg))
+	ctx := next.ctx // important because the m.ServeDNS has only now populated the context
 
 	w := dnstest.NewRecorder(&test.ResponseWriter{})
-
 	r := new(dns.Msg)
 	r.SetQuestion("example.org.", dns.TypeHINFO)
-	r.MsgHdr.AuthenticatedData = true
 
-	repl := New(nctx, r, w, "-")
+	repl := New()
+	state := request.Request{W: w, Req: r}
 
 	for i, ts := range tests {
-		r := repl.Replace(ts.expr)
+		r := repl.Replace(ctx, state, nil, ts.expr)
 		if r != ts.result {
-			t.Errorf("Test %d - expr : %s, expected replacement being %s, and got %s", i, ts.expr, ts.result, r)
+			t.Errorf("Test %d - expr : %s, expected %q, got %q", i, ts.expr, ts.result, r)
 		}
 	}
 }