plugin/dnstap: various cleanups #4179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stickler-ci
reviewed
Oct 5, 2020
|
ok, this needs more work, msg/msg.go just wraps the tap struct which seems to add an indirection that creates confusion. I'm going to see where this ends and how big of a code reduction I can create (if any) |
stickler-ci
reviewed
Oct 5, 2020
miekg
force-pushed
the
dnstap2
branch
2 times, most recently
from
bcbb283
to
6add490
Compare
Codecov Report
@@ Coverage Diff @@
## master #4179 +/- ##
==========================================
+ Coverage 55.96% 56.11% +0.15%
==========================================
Files 225 224 -1
Lines 9938 9856 -82
==========================================
- Hits 5562 5531 -31
+ Misses 3910 3866 -44
+ Partials 466 459 -7
Continue to review full report at Codecov.
|
rdrozhdzh
reviewed
Oct 6, 2020
There was a problem hiding this comment.
I like your idea to simplify this plugin. However, your changes make new version of the plugin incompatible with old version. There were a lot of public properties and methods in dnstap package. I think we should not simply remove them without deprecation for some reasonable period of time.
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] plugin/dnstap..." ]
@miekg , I think we can just fix the issue #4165 now and rework dnstap plugin
later. What do you think?
there is no backward compat quarantee for this *and* we releasing 1.8.0 anyway, so I can't
see the point. Fixing code to be thrown away a day later seems a non-optimal time spend
|
|
So, are you going to merge your version? And release it in 1.8.0? |
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] plugin/dnstap..." ]
So, are you going to merge your version? And release it in 1.8.0?
well, once it's in a good shape, yes
Please, add notes about backward incompatibility then. And I will close my PR,
if so.
yep this will get a note in the relase notes. How to use is updated in the README of
dnstap.
/Miek
…--
Miek Gieben
|
stickler-ci
reviewed
Oct 9, 2020
Codecov Report
@@ Coverage Diff @@
## master #4179 +/- ##
==========================================
+ Coverage 55.96% 56.07% +0.10%
==========================================
Files 225 223 -2
Lines 9938 9839 -99
==========================================
- Hits 5562 5517 -45
+ Misses 3910 3863 -47
+ Partials 466 459 -7
Continue to review full report at Codecov.
|
|
comments addressed. PTAL |
A recent issue made me look into this plugin, I suspect various other cleanups (hopefully deletion of code) can be made as well Remove identical functions ToClientQuery etc, and just use tap.Message as the base type in plugin. Keep msg/ for a few helper functions that may proof useful. This remove the whole test directory as we will just check the things we are interested in which gives much better feedback and keeps that code closer together. tapwr dir is also not needed, writer_test.go was just duplicating the tests already done. This moves writer.go to the top directory. Make the only user of dnstap, the forward plugin, use the newer code also remove the test, a better test there would be a full e2e test to see the correct thing happens. Cleanup the Tapper interface and move it to dnstapio where it belongs, remove higher level interfaces that are not used. This remove dnstap.Tapper and dnstap.IORoutines. Use the standard mechanism for getting access to a plugin and remove shuffling the plugin into the context. Signed-off-by: Miek Gieben <miek@miek.nl>
rdrozhdzh
reviewed
Oct 9, 2020
|
Thanks for reviewing.
Adding those options lgtm and should be done in follow up PR
…On Fri, 9 Oct 2020, 18:42 Ruslan Drozhdzh, ***@***.***> wrote:
***@***.**** commented on this pull request.
Overall looks good to me. The config options for message types are highly
desired
------------------------------
In plugin/dnstap/writer.go
<#4179 (comment)>:
> + msg.SetQueryTime(tm, w.QueryTime)
+ if err := msg.SetQueryAddress(tm, w.RemoteAddr()); err != nil {
+ w.Err = err
+ return err
+ }
+
+ if w.IncludeRawMessage {
+ buf, err := w.Query.Pack()
+ if err != nil {
+ w.Err = err
+ return err
+ }
+ tm.QueryMessage = buf
+ }
+ msg.SetType(tm, tap.Message_CLIENT_QUERY)
+ w.TapMessage(tm)
ideally, CQ message should be loged from func (h Dnstap) ServeDNS(). So
that in case if some plugin chooses to drop DNS response (i.e. doesn't
write DNS response), the CQ still to be sent. Though, this was never
implemented
------------------------------
In plugin/forward/dnstap.go
<#4179 (comment)>:
> opts := f.opts
- t := ""
+ t := state.Proto()
switch {
case opts.forceTCP: // TCP flag has precedence over UDP flag
t = "tcp"
case opts.preferUDP:
t = "udp"
this is not quite reliable. Forward may try udp and finally use TCP. The
opts from the line
https://github.com/coredns/coredns/blob/02f52b5d271d47a035c5432cca4fcf4174c023fa/plugin/forward/forward.go#L126
has more accurate value, since it is updated after UDP failure
------------------------------
In plugin/dnstap/handler.go
<#4179 (comment)>:
> @@ -32,14 +31,6 @@ type (
}
)
-// ContextKey defines the type of key that is used to save data into the context.
-type ContextKey string
-
-const (
- // DnstapSendOption specifies the Dnstap message to be send. Default is sent all.
- DnstapSendOption ContextKey = "dnstap-send-option"
Yes, these options are really needed. AFAIK bind and unbound allow
configuring per message type as well.
The config may look like
dnstap tcp://127.0.0.1:9123 full CR FR
where all parameters after endpoint are optional
or
dnstap tcp://127.0.0.1:9123 full {
allow CR FR
}
Other plugins should be able to check these config options. Bunch of
booleans is ok.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4179 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACWIW4PGAJFCGH345WLEF3SJ44RBANCNFSM4SEU6L5A>
.
|
Signed-off-by: Miek Gieben <miek@miek.nl>
Signed-off-by: Miek Gieben <miek@miek.nl>
…ere these fields have been preparsed Signed-off-by: Miek Gieben <miek@miek.nl>
all these fields have been preparsed, no need for dnstap to be pedantic and check (and save!) this error again. Simplifies it a bit more. Signed-off-by: Miek Gieben <miek@miek.nl>
rdrozhdzh
reviewed
Oct 12, 2020
rdrozhdzh
reviewed
Oct 12, 2020
rdrozhdzh
reviewed
Oct 12, 2020
rdrozhdzh
reviewed
Oct 12, 2020
Co-authored-by: Ruslan Drozhdzh <30860269+rdrozhdzh@users.noreply.github.com>
Signed-off-by: Miek Gieben <miek@miek.nl>
Signed-off-by: Miek Gieben <miek@miek.nl>
|
thanks for the thorough review @rdrozhdzh |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove identical functions ToClientQuery etc, and just use ToTapMessage
with the type. Remove the Builder type and just make it Msg and make
error handling explicit instead of using a Err field that gets checked
only at the end.
A recent issue made me look into this plugin, I suspect various other
cleanups (hopefully deletion of code) can be made as well