Plugin dnstap: add support for "extra" field in payload by chenyuheng · Pull Request #6226 · coredns/coredns

chenyuheng · 2023-07-25T21:29:23Z

1. Why is this pull request needed and what does it do?

As proposed in #6092 , this PR adds supports to customize "extra" field in dnstap payload, the field could be set in the config file.

The "extra" field in defined in dnstap.proto.

2. Which issues (if any) are related?

#6092

3. Which documentation changes (if any) need to be made?

The plugin README: https://github.com/coredns/coredns/blob/master/plugin/dnstap/README.md

4. Does this introduce a backward incompatible change or deprecation?

No

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

chenyuheng · 2023-07-25T21:59:56Z

This comment shows the result of manually testting for this change.

Note that the current dnstap command line tool won't output "extra" field, so I tested this change with my edited dnstap command line tool, I will try to make another PR to update the change to the golang-dnstap repository later.

With Corefile:

.:8053 {
	dnstap /tmp/dnstap.sock {
		identity test_1
		version test0.1
		extra EXTRA_FIELD_TEST_233
	}
	forward . 8.8.8.8
}

The dnstap command line tool will have:

rdrozhdzh · 2023-07-26T11:03:45Z

+		Message: m,
+		Identity: h.Identity,
+		Version: h.Version,
+		Extra: h.Extra,


what you implemented here is assigning a preconfigured text to Extra (which can be useful as well), whereas #6092 suggests extracting and assigning value of preconfigured metadata label.

The metadata value is normally created by other plugin, e.g. if you specify label "forward/upstream", it is expected that plugin 'forward' should provide this value. The plugin which provides metadata can generate different values for different queries, and this is the key difference from what you have implemented.

Please get familiar with plugin metadata https://github.com/coredns/coredns/tree/master/plugin/metadata as well as the package https://github.com/coredns/coredns/tree/master/plugin/pkg/replacer which can make your work easier

I also recommend to look at plugins 'rewrite' and 'log' as examples of plugins which read metadata from other plugins.

Thanks for your kind review! I have introduced the replacer package in my new commits.

Since the replacer requires context, status (request.Request) and rr (*dnstest.Recorder), I have added a new function called TapMessageWithMetadata to receive these context objects and process the metadata replacement accordingly.

The old function TapMessage has not been removed, as external plugins may have dependencies on it.

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

rdrozhdzh

Please add also more unit tests to make sure the new functionality works as expected

rdrozhdzh · 2023-07-28T11:13:18Z


 // TapMessageWithMetadata sends the message m to the dnstap interface, with "Extra" field being interpreted by the provided metadata context.
-func (h Dnstap) TapMessageWithMetadata(m *tap.Message, ctx context.Context, state *request.Request, rr *dnstest.Recorder) {
+func (h Dnstap) TapMessageWithMetadata(m *tap.Message, ctx context.Context, state request.Request, rr *dnstest.Recorder) {


I would suggest to remove the rr *dnstest.Recorder at all, and always pass nil to Replacer. I believe this parameter creates more inconveniences than benefits.

The recorder parameter is needed only for handling several standard labels like

coredns/plugin/pkg/replacer/replacer.go

Lines 87 to 111 in b95bc0a

// Recorded replacements.

case "{rcode}":

if rr == nil || rr.Msg == nil {

return append(b, EmptyValue...)

}

if rcode := dns.RcodeToString[rr.Rcode]; rcode != "" {

return append(b, rcode...)

}

return strconv.AppendInt(b, int64(rr.Rcode), 10)

case "{rsize}":

if rr == nil {

return append(b, EmptyValue...)

}

return strconv.AppendInt(b, int64(rr.Len), 10)

case "{duration}":

if rr == nil {

return append(b, EmptyValue...)

}

secs := time.Since(rr.Start).Seconds()

return append(strconv.AppendFloat(b, secs, 'f', -1, 64), 's')

case headerReplacer + "rflags}":

if rr != nil && rr.Msg != nil {

return appendFlags(b, rr.Msg.MsgHdr)

}

return append(b, EmptyValue...)

This code can safely work with nil, i.e. it will not panic.

The reason why it's not so important to support these DNS response labels (as well as other DNS labels) is because DNSTAP already provides an alternative standardized way for transferring this data.

Regarding parameter state, this is more commonly used data type, and we can keep it. This should not be a big problem for developers of other plugins to provide it. However, if we decide to not support the standard DNS labels (as explained above), we can eliminate this parameter as well. You can pass empty Request{} to the replacer (this needs to be tested with all standard labels to avoid panics)

The only important parameter here is Context, which provides access to metadata.

I would appreciate a feedback from community regarding support of standard DNS labels (do we really need them?) for DNSTAP extra section, i.e.

coredns/plugin/pkg/replacer/replacer.go

Lines 37 to 57 in b95bc0a

// labels are all supported labels that can be used in the default Replacer.

var labels = map[string]struct{}{

"{type}": {},

"{name}": {},

"{class}": {},

"{proto}": {},

"{size}": {},

"{remote}": {},

"{port}": {},

"{local}": {},

// Header values.

headerReplacer + "id}": {},

headerReplacer + "opcode}": {},

headerReplacer + "do}": {},

headerReplacer + "bufsize}": {},

// Recorded replacements.

"{rcode}": {},

"{rsize}": {},

"{duration}": {},

headerReplacer + "rflags}": {},

}

Thanks! I have updated these codes in my new commits. I will update the unit tests tomorrow.

For the state parameter, maybe we can just keep the function as:

func (h Dnstap) TapMessageWithMetadata(ctx context.Context, m *tap.Message, state request.Request)

Leave the choice to plugin developers, if they think their plugins does not need to support these standard labels, they simply just pass the empty State{}

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

rdrozhdzh

In general looks good, see minor comments

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

codecov · 2023-08-01T03:02:22Z

Codecov Report

Merging #6226 (74094f9) into master (93c57b6) will increase coverage by 2.67%.
Report is 1000 commits behind head on master.
The diff coverage is 61.52%.

@@            Coverage Diff             @@
##           master    #6226      +/-   ##
==========================================
+ Coverage   55.70%   58.37%   +2.67%     
==========================================
  Files         224      251      +27     
  Lines       10016    16418    +6402     
==========================================
+ Hits         5579     9584    +4005     
- Misses       3978     6247    +2269     
- Partials      459      587     +128

Files Changed	Coverage Δ
core/dnsserver/config.go	`0.00% <ø> (ø)`
core/dnsserver/register.go	`13.27% <0.00%> (-4.71%)`	⬇️
plugin/auto/auto.go	`0.00% <0.00%> (ø)`
plugin/auto/xfr.go	`0.00% <0.00%> (ø)`
plugin/auto/zone.go	`78.94% <ø> (+0.68%)`	⬆️
plugin/autopath/autopath.go	`73.61% <ø> (-1.39%)`	⬇️
plugin/azure/azure.go	`10.61% <0.00%> (-0.57%)`	⬇️
plugin/azure/setup.go	`56.36% <0.00%> (-4.14%)`	⬇️
plugin/backend_lookup.go	`0.00% <0.00%> (ø)`
plugin/bind/bind.go	`50.00% <0.00%> (-50.00%)`	⬇️
... and 78 more

... and 168 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

rdrozhdzh

looks good to me

dmachard · 2023-08-28T19:20:09Z

@chenyuheng FYI the DNS-collector tool supports the extra field, any contributions will be appreciated.

chenyuheng added 3 commits July 26, 2023 04:59

dnstap: add 'extra' field

6728c7f

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

dnstap: add setup_test for 'extra' field

9a029d0

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

udnstap: update document and test

c1e29dd

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

chenyuheng requested a review from yongtang as a code owner July 25, 2023 21:29

dnstap: update setup_test for more coverage

1e8e232

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

rdrozhdzh reviewed Jul 26, 2023

View reviewed changes

dnstap: add TapMessageWithMetadata function to Dnstap

cbfd37f

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

chenyuheng force-pushed the master branch from 7912a21 to cbfd37f Compare July 27, 2023 21:19

dnstap: adapt dnstap and forward plugins to use TapMessageWithMetadata

201541d

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

chenyuheng requested review from Tantalor93, chrisohaver, johnbelamaric and miekg as code owners July 27, 2023 23:34

rdrozhdzh reviewed Jul 28, 2023

View reviewed changes

chenyuheng added 3 commits July 29, 2023 02:36

change TapMessageWithMetadata function

728081c

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

tab inconsistency fix

e0abebf

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

fix replacer to support empty state

5d4b382

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

chenyuheng requested a review from stp-ip as a code owner July 28, 2023 19:38

chenyuheng added 2 commits July 29, 2023 13:39

add replacer test for empty status parameter

9368c61

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

dnstap: update unit test for 'extra' field

dcdd3d2

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

chenyuheng requested a review from rdrozhdzh July 30, 2023 01:13

rdrozhdzh reviewed Jul 31, 2023

View reviewed changes

clean up code

b32a816

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

chenyuheng force-pushed the master branch from 0fd64df to b32a816 Compare August 1, 2023 02:46

gofmt fix & static analysis fix

8c72b60

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

rdrozhdzh reviewed Aug 1, 2023

View reviewed changes

Comment thread plugin/dnstap/handler.go Outdated

Comment thread plugin/dnstap/writer.go Outdated

Comment thread plugin/pkg/replacer/replacer_test.go Outdated

dnstap: refactor

74094f9

Signed-off-by: chenyuheng <chenyuheng99@qq.com>

rdrozhdzh approved these changes Aug 2, 2023

View reviewed changes

yongtang merged commit 90d5561 into coredns:master Aug 14, 2023

BrewTestBot mentioned this pull request Feb 26, 2024

coredns 1.11.2 Homebrew/homebrew-core#164265

Merged

BrewTestBot mentioned this pull request May 1, 2024

coredns 1.11.3 Homebrew/homebrew-core#170526

Merged

	// Recorded replacements.
	case "{rcode}":
	if rr == nil \|\| rr.Msg == nil {
	return append(b, EmptyValue...)
	}
	if rcode := dns.RcodeToString[rr.Rcode]; rcode != "" {
	return append(b, rcode...)
	}
	return strconv.AppendInt(b, int64(rr.Rcode), 10)
	case "{rsize}":
	if rr == nil {
	return append(b, EmptyValue...)
	}
	return strconv.AppendInt(b, int64(rr.Len), 10)
	case "{duration}":
	if rr == nil {
	return append(b, EmptyValue...)
	}
	secs := time.Since(rr.Start).Seconds()
	return append(strconv.AppendFloat(b, secs, 'f', -1, 64), 's')
	case headerReplacer + "rflags}":
	if rr != nil && rr.Msg != nil {
	return appendFlags(b, rr.Msg.MsgHdr)
	}
	return append(b, EmptyValue...)

	// labels are all supported labels that can be used in the default Replacer.
	var labels = map[string]struct{}{
	"{type}": {},
	"{name}": {},
	"{class}": {},
	"{proto}": {},
	"{size}": {},
	"{remote}": {},
	"{port}": {},
	"{local}": {},
	// Header values.
	headerReplacer + "id}": {},
	headerReplacer + "opcode}": {},
	headerReplacer + "do}": {},
	headerReplacer + "bufsize}": {},
	// Recorded replacements.
	"{rcode}": {},
	"{rsize}": {},
	"{duration}": {},
	headerReplacer + "rflags}": {},
	}

Conversation

chenyuheng commented Jul 25, 2023

1. Why is this pull request needed and what does it do?

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

Uh oh!

chenyuheng commented Jul 25, 2023

Uh oh!

rdrozhdzh Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

chenyuheng Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rdrozhdzh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rdrozhdzh Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenyuheng Jul 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rdrozhdzh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rdrozhdzh left a comment

Choose a reason for hiding this comment

Uh oh!

dmachard commented Aug 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rdrozhdzh Jul 28, 2023 •

edited

Loading

codecov Bot commented Aug 1, 2023 •

edited

Loading