Repository: coredns/coredns (go)

Issue:
plugin/trace - wrong rcode tag in case when no response is written
**What happened**:
This issue is very similar as reported in https://github.com/coredns/coredns/issues/4726 and https://github.com/coredns/coredns/issues/4724. When no response is written from upstream plugins (for example due to timeout in forward plugin), the `trace` plugin tags span `servedns` with tag `coredns.io/rcode` with value `NOERROR`

**What you expected to happen**:
I would expect that `trace` plugin would add tag `coredns.io/rcode` with value `SERVFAIL` (as SERVFAIL is returned to the client) or no value at all

**How to reproduce it (as minimally and precisely as possible)**:
run zipkin in docker
```
docker run -d -p 9411:9411 openzipkin/zipkin
```

run CoreDNS with Corefile (the invalid IP in forward plugin is there on purpose to better simulate errors)

```
.:53 {
    forward . 9.8.7.6
    trace zipkin 127.0.0.1:9411 {
        service dns-service
    }
}
```

run dig
```
dig @127.0.0.1 example.com
```

open Zipkin in browser and find the trace and you will see that `servedns` span has `coredns.io/rcode` tag with value `NOERROR`, but in fact the client (dig) received SERVFAIL

![image](https://user-images.githubusercontent.com/5522817/125122979-65d58200-e0f6-11eb-98e7-4bbe979c77b0.png)


**Anything else we need to know?**:

**Environment**:

- the version of CoreDNS: 1.8.4
- Corefile:
```
.:53 {
    forward . 9.8.7.6
    trace zipkin 127.0.0.1:9411 {
        service dns-service
    }
}
```
- logs, if applicable:
- OS (e.g: `cat /etc/os-release`):
- Others:


Pull request description:
plugin/trace: fix rcode tag in case of no response
### 1. Why is this pull request needed and what does it do?
when no response is written from up the chain of plugins, instead of using the default value of `dnstest.Recorder` for rcode (0) as value for `coredns.io/rcode` tag, I rather fallback to the status of the next plugin as that status is used for constructing the error response the client sees. A similar fix was already done to `prometheus` plugin, see https://github.com/coredns/coredns/pull/4727

### 2. Which issues (if any) are related?
https://github.com/coredns/coredns/issues/4741

### 3. Which documentation changes (if any) need to be made?
no documentation changes needed

### 4. Does this introduce a backward incompatible change or deprecation?
no, I don't think so

closes #4741

Patch:
diff --git a/plugin/trace/trace.go b/plugin/trace/trace.go
index 4d3deb207..1b74dcbc3 100644
--- a/plugin/trace/trace.go
+++ b/plugin/trace/trace.go
@@ -149,7 +149,14 @@ func (t *trace) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 	span.SetTag(t.tagSet.Type, req.Type())
 	span.SetTag(t.tagSet.Proto, req.Proto())
 	span.SetTag(t.tagSet.Remote, req.IP())
-	span.SetTag(t.tagSet.Rcode, rcode.ToString(rw.Rcode))
+	rc := rw.Rcode
+	if !plugin.ClientWrite(status) {
+		// when no response was written, fallback to status returned from next plugin as this status
+		// is actually used as rcode of DNS response
+		// see https://github.com/coredns/coredns/blob/master/core/dnsserver/server.go#L318
+		rc = status
+	}
+	span.SetTag(t.tagSet.Rcode, rcode.ToString(rc))
 	if err != nil {
 		otext.Error.Set(span, true)
 		span.LogFields(otlog.Event("error"), otlog.Error(err))


Test patch:
diff --git a/plugin/trace/trace_test.go b/plugin/trace/trace_test.go
index 233f63207..dae546f8d 100644
--- a/plugin/trace/trace_test.go
+++ b/plugin/trace/trace_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/coredns/caddy"
+	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/pkg/dnstest"
 	"github.com/coredns/coredns/plugin/pkg/rcode"
 	"github.com/coredns/coredns/plugin/test"
@@ -43,23 +44,33 @@ func TestTrace(t *testing.T) {
 	cases := []struct {
 		name     string
 		rcode    int
+		status   int
 		question *dns.Msg
-		server   string
 		err      error
 	}{
 		{
 			name:     "NXDOMAIN",
 			rcode:    dns.RcodeNameError,
+			status:   dns.RcodeSuccess,
 			question: new(dns.Msg).SetQuestion("example.org.", dns.TypeA),
 		},
 		{
 			name:     "NOERROR",
 			rcode:    dns.RcodeSuccess,
+			status:   dns.RcodeSuccess,
 			question: new(dns.Msg).SetQuestion("example.net.", dns.TypeCNAME),
 		},
 		{
 			name:     "SERVFAIL",
 			rcode:    dns.RcodeServerFailure,
+			status:   dns.RcodeSuccess,
+			question: new(dns.Msg).SetQuestion("example.net.", dns.TypeA),
+			err:      errors.New("test error"),
+		},
+		{
+			name:     "No response written",
+			rcode:    dns.RcodeServerFailure,
+			status:   dns.RcodeServerFailure,
 			question: new(dns.Msg).SetQuestion("example.net.", dns.TypeA),
 			err:      errors.New("test error"),
 		},
@@ -71,13 +82,12 @@ func TestTrace(t *testing.T) {
 			m := mocktracer.New()
 			tr := &trace{
 				Next: test.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
-					m := new(dns.Msg)
-					m.SetRcode(r, tc.rcode)
-					w.WriteMsg(m)
-					if tc.err != nil {
-						return tc.rcode, tc.err
+					if plugin.ClientWrite(tc.status) {
+						m := new(dns.Msg)
+						m.SetRcode(r, tc.rcode)
+						w.WriteMsg(m)
 					}
-					return tc.rcode, nil
+					return tc.status, tc.err
 				}),
 				every:  1,
 				tracer: m,