Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dnstap: adds message_types option for filtering #6643

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

dnstap: adds message_types option for filtering #6643

wants to merge 14 commits into from
+331 −93

Conversation

mathetake
Copy link
Contributor

@mathetake mathetake commented Apr 26, 2024 •
edited

Note: I am completely open to discuss the naming of the option

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

This resolves #4198 and #6640 by introducing message_types configuration in the dnstap plugin.

2. Which issues (if any) are related?

#4198 and #6640

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

dnstap/README.md

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

No.

I left this as draft until we agreed on the naming of the option. I would appreciate it if you folks can take a look @rdrozhdzh @miekg (pinging the people in the original issue #4198)

@mathetake mathetake mentioned this pull request Apr 26, 2024
Open
@mathetake
dnstap: adds message_types option for filtering
549c56a 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
time.Now only when needed
89dbadc 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
Fixes import ordering
9d450e0 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@codecov Codecov
Copy link

codecov bot commented Apr 30, 2024 •
edited

Codecov Report

Attention: Patch coverage is 58.33333% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 59.28%. Comparing base (93c57b6) to head (2a493b7).
Report is 1184 commits behind head on master.

Files Patch % Lines
plugin/forward/dnstap.go 0.00% 22 Missing ⚠️
plugin/dnstap/setup.go 83.33% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6643      +/-   ##
==========================================
+ Coverage   55.70%   59.28%   +3.58%     
==========================================
  Files         224      252      +28     
  Lines       10016    13501    +3485     
==========================================
+ Hits         5579     8004    +2425     
- Misses       3978     4904     +926     
- Partials      459      593     +134

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mathetake
More test coverage
1497ac8 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
rdrozhdzh
rdrozhdzh reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@rdrozhdzh rdrozhdzh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/coredns/coredns/blob/master/plugin/dnstap/README.md should be updated as well

plugin/dnstap/handler.go Outdated Show resolved Hide resolved
plugin/dnstap/handler_test.go Show resolved Hide resolved
plugin/dnstap/setup.go Outdated Show resolved Hide resolved
plugin/dnstap/setup_test.go Outdated Show resolved Hide resolved
plugin/forward/dnstap.go Show resolved Hide resolved
@mathetake
review: use the same query time in ServeDNS
4db7c5a 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
review: add negative assertion case in TestDnstap_MessageTypeEnabled
00edfa6 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
review: use RemainingArgs instead of manual parsing
e7ce126 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
consistent use of bitmap in comments
3c66669 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
review: use hardcoded value in Test_messageTypesMap
6e9188f 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
review: only appends forwarder enabled taps in forward pluin
109b204 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
review: update dnstap/README.md
70f296c 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
Copy link
Contributor Author

@rdrozhdzh thank you for the thorough reviews. I think I addressed all your comments, so I would appreciate it if you can take a look again!

@mathetake mathetake marked this pull request as ready for review May 1, 2024 01:40
rdrozhdzh
rdrozhdzh reviewed May 2, 2024
View reviewed changes
plugin/dnstap/README.md Show resolved Hide resolved
plugin/dnstap/setup_test.go Outdated Show resolved Hide resolved
plugin/forward/forward_test.go Outdated Show resolved Hide resolved
plugin/forward/forward_test.go Outdated Show resolved Hide resolved
plugin/forward/forward_test.go Outdated Show resolved Hide resolved
plugin/forward/forward_test.go Outdated Show resolved Hide resolved
@rdrozhdzh
Copy link
Contributor

@mathetake , just curious, are you going to implement the dnstap message filtering based on the DNS response RCODE, as you mentioned in your original request?

@mathetake
review: tweak test cases
e7610dd 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
review: mentions that custom plugins can tap any message type
82a9996 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
Copy link
Contributor Author

@rdrozhdzh actually, I am not planning to introduce that RCODE based filter as that seems not worth it vs the complexity in the API.

@mathetake
review: tweak test case part2
2a493b7 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
rdrozhdzh
rdrozhdzh approved these changes May 3, 2024
View reviewed changes
Copy link
Contributor

@rdrozhdzh rdrozhdzh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me

@mathetake
Copy link
Contributor Author

kindly ping any codeowner for a quick look?

@mathetake
Copy link
Contributor Author

I really hate to be nagging, and I understand all of you guys are busy doing other stuff, but by any chance could one of you code owners take a look? @johnbelamaric @miekg @Tantalor93 @chrisohaver @yongtang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codefromthecrypt codefromthecrypt codefromthecrypt left review comments

@rdrozhdzh rdrozhdzh rdrozhdzh approved these changes

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric johnbelamaric is a code owner

@miekg miekg Awaiting requested review from miekg miekg is a code owner

@Tantalor93 Tantalor93 Awaiting requested review from Tantalor93 Tantalor93 is a code owner

@chrisohaver chrisohaver Awaiting requested review from chrisohaver chrisohaver is a code owner

@yongtang yongtang Awaiting requested review from yongtang yongtang is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhance configuration of dnstap plugin
3 participants
@mathetake @rdrozhdzh @codefromthecrypt