Skip to content

dns_filter: Generate responses to queries#11074

Merged
mattklein123 merged 39 commits into
envoyproxy:masterfrom
abaptiste:dns_filter_upstream3
May 26, 2020
Merged

dns_filter: Generate responses to queries#11074
mattklein123 merged 39 commits into
envoyproxy:masterfrom
abaptiste:dns_filter_upstream3

Conversation

@abaptiste
Copy link
Copy Markdown
Contributor

@abaptiste abaptiste commented May 5, 2020

Commit Message: dns_filter: Generate responses to DNS queries

Additional Description: This change adds the DNS filter response generation. The filter can now operate and respond to queries for domains that have been statically configured. The filter will also return addresses for names that match clusters.

Documentation has been updated to reflect the V3 api and the ability to specify an external DNS table.

Multiple unit tests have been added along with a fuzz test for the DNS Filter.

Signed-off-by: Alvin Baptiste alvinsb@gmail.com

Risk Level: Low
Testing: Unit tests, Coverage, Clang-tidy, Fuzz
Docs Changes: Filter configuration and description updates
[#Issue] #6748

@abaptiste
dns_filter: Generate responses to queries
7a21520 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste abaptiste requested a review from mattklein123 as a code owner May 5, 2020 21:27
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only Bot commented May 5, 2020

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11074 was opened by abaptiste.

see: more, trace.

htuch
htuch reviewed May 6, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

API changes LGTM modulo nits.

Comment thread api/envoy/data/dns/v3/dns_table.proto Outdated
Comment thread docs/root/configuration/listeners/udp_filters/dns_filter.rst Outdated
@abaptiste
dns_filter: Address PR feedback
4d49bf1 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented May 6, 2020

@sumukhs do you mind taking a first pass?

@sumukhs
Copy link
Copy Markdown
Contributor

sumukhs commented May 7, 2020

Will take a look today

sumukhs
sumukhs suggested changes May 7, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@sumukhs sumukhs left a comment

Choose a reason for hiding this comment

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

I haven't looked at the parser changes yet. But had a few clarifications in the response path for clusters.

Comment thread source/extensions/filters/udp/dns_filter/dns_filter.cc
Comment thread source/extensions/filters/udp/dns_filter/dns_filter.cc Outdated
Comment thread test/extensions/filters/udp/dns_filter/dns_filter_fuzz_test.cc Outdated
Comment thread source/extensions/filters/udp/dns_filter/dns_filter.cc
Comment thread source/extensions/filters/udp/dns_filter/dns_filter.h
Comment thread source/extensions/filters/udp/dns_filter/dns_parser.cc
Comment thread test/extensions/filters/udp/dns_filter/dns_filter_test.cc
Comment thread source/extensions/filters/udp/dns_filter/dns_filter.cc
Comment thread source/extensions/filters/udp/dns_filter/dns_filter.cc
Comment thread source/extensions/filters/udp/dns_filter/dns_filter.cc Outdated
abaptiste added 2 commits May 7, 2020 21:52
@abaptiste
dns_filter: Address PR Feedback. Add integ test with cluster lookup
35d44e6 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: fix spelling
63da5db 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: Add the ability to redirect a domain to a cluster
94cd081 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
sumukhs
sumukhs reviewed May 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@sumukhs sumukhs left a comment

Choose a reason for hiding this comment

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

had missed submitting these

Comment thread source/extensions/filters/udp/dns_filter/dns_filter.cc
Comment thread source/extensions/filters/udp/dns_filter/dns_parser.h
Comment thread test/extensions/filters/udp/dns_filter/dns_filter_test.cc
Comment thread source/extensions/filters/udp/dns_filter/dns_parser.cc
Comment thread source/extensions/filters/udp/dns_filter/dns_parser.h Outdated
abaptiste added 3 commits May 11, 2020 15:36
@abaptiste
dns_filter: update docs
c66c56d 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
Merge branch 'master' into dns_filter_upstream3
9737ab3 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: Address PR Feedback
da44574 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: attempt to appease the Windows CI gods
dd5aeb3 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
sumukhs
sumukhs previously approved these changes May 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@sumukhs sumukhs left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback. Things look good from the config and basic flow.

Will let @mattklein123 take a look.

dio
dio reviewed May 12, 2020
View reviewed changes
Comment thread api/envoy/data/dns/v3/dns_table.proto
mattklein123
mattklein123 requested changes May 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks awesome from a high level skim. Thanks a ton for adding the fuzz test and the integration test. I've asked @asraa to take a quick look at the fuzz test as she is our fuzz guru. Thank you!

/wait

Comment thread source/extensions/filters/udp/dns_filter/dns_filter.cc Outdated
Comment thread source/extensions/filters/udp/dns_filter/dns_filter.cc Outdated
Comment thread source/extensions/filters/udp/dns_filter/dns_parser.cc
Comment thread test/extensions/filters/udp/dns_filter/dns_filter_fuzz_test.cc
Comment thread test/extensions/filters/udp/dns_filter/dns_filter_integration_test.cc Outdated
abaptiste added 3 commits May 13, 2020 19:08
@abaptiste
dns_filter: Address PR Feedback
b239d00 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
Merge branch 'master' into dns_filter_upstream3
44a271c 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: minor tweak to index randomization
2f8620f 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
abaptiste added 3 commits May 19, 2020 21:32
@abaptiste
dns_filter: update random resolver test
0a897a0 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: code format fix
8a6e962 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: make things const
b570aac 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: Update fuzz test
3fad1da 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
asraa
asraa reviewed May 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Wow, thank you so much! This is looking great.

How is the execution speed of the fuzzer, and can you try running test/run_envoy_bazel_coverage.sh //test/extensions/filters/udp/dns_filter:dns_filter_fuzz_test_with_libfuzzer and see the coverage over the dns parser code?

Comment thread test/extensions/filters/udp/dns_filter/dns_filter_fuzz_test.cc Outdated
Comment thread test/extensions/filters/udp/dns_filter/dns_filter_fuzz_test.cc
Comment thread test/extensions/filters/udp/dns_filter/dns_filter_fuzz_test.cc Outdated
Comment thread test/extensions/filters/udp/dns_filter/dns_filter_fuzz_test.cc
abaptiste added 3 commits May 20, 2020 17:44
@abaptiste
Merge branch 'master' into dns_filter_upstream3
d169ece 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: Update fuzz test
6db899a 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
Merge branch 'master' into dns_filter_upstream3
04daa25 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
marcomagdy
marcomagdy reviewed May 20, 2020
View reviewed changes
Comment thread test/extensions/filters/udp/dns_filter/dns_filter_fuzz_test.cc
@abaptiste
Copy link
Copy Markdown
Contributor Author

abaptiste commented May 20, 2020 •
edited
Loading

Looking into the coverage test failures.

EDIT: Issues with the dns filter tests in the coverage build are resolved. //test/integration:http2_integration_test is failing in CI, but not for me locally.

abaptiste added 3 commits May 20, 2020 23:14
@abaptiste
dns_filter: Use an index offset instead of std::shuffle
2a9621a 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: Add a larger corpus for fuzz testing
d6f3a56 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: check format fix
9dbc5e9 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
abaptiste added 5 commits May 21, 2020 00:26
@abaptiste
dns_filter: clang-tidy fixes
0da1b1f 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
Merge branch 'master' into dns_filter_upstream3
098f23d 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: Fix test
843637e 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
dns_filter: Fix test #2
608f9d0 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste
Merge branch 'master' into dns_filter_upstream3
5479b69 
Signed-off-by: Alvin Baptiste <alvinsb@gmail.com>
@abaptiste abaptiste requested a review from mattklein123 May 21, 2020 20:02
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented May 21, 2020

OSX issue is known, see #11290

mattklein123
mattklein123 approved these changes May 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM to ship and iterate, thank you!

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented May 21, 2020

@asraa any further comments on this?

@asraa
Copy link
Copy Markdown
Contributor

asraa commented May 26, 2020

LGTM for now! Thanks so much for adding a fuzzer. Was waiting for an update on the coverage for the parser code specifically, but that can be checked manually and improved later.

@mattklein123 mattklein123 merged commit c8c7d1a into envoyproxy:master May 26, 2020
@abaptiste abaptiste deleted the dns_filter_upstream3 branch May 26, 2020 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

+5 more reviewers

@dio dio dio left review comments

@marcomagdy marcomagdy marcomagdy left review comments

@htuch htuch htuch left review comments

@asraa asraa asraa approved these changes

@sumukhs sumukhs sumukhs left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sumukhs sumukhs

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@abaptiste @mattklein123 @sumukhs @asraa @dio @marcomagdy @htuch