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

dns_filter: add support for wildcard name in DNS filter inline table #34588

Merged
merged 6 commits into from
Jul 9, 2024
Merged

dns_filter: add support for wildcard name in DNS filter inline table #34588

merged 6 commits into from
Jul 9, 2024

Conversation

StupidScience
Copy link
Contributor

@StupidScience StupidScience commented Jun 6, 2024 •
edited
Loading

Commit Message: Add support for wildcard name in DNS filter inline table
Additional Description:
Risk Level: Medium
Testing: I ran only unit tests relevant to dns_filter
Docs Changes: -
Release Notes: Added support wildcard names in :ref:inline_dns_table <envoy_v3_api_field_extensions.filters.udp.dns_filter.v3.DnsFilterConfig.ServerContextConfig.inline_dns_table>.
Platform Specific Features: N/A
Fixes #32941

@repokitteh-read-only RepoKitteh - Read Only
Copy link

Hi @StupidScience, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #34588 was opened by StupidScience.

see: more, trace.

@StupidScience StupidScience mentioned this pull request Jun 6, 2024
Closed
@adisuissa
Copy link
Contributor

Assigning Yanjun for a first-pass review as a code-owner:
/assign @yanjunxiang-google

@adisuissa
Copy link
Contributor

Assigning to Yan for a review (after Yanjun's pass).
/assign @yanavlasov

@yanjunxiang-google
Copy link
Contributor

Thanks @StupidScience for working on this!
Could you please fix the CI error?

@StupidScience
Copy link
Contributor Author

/retest

@StupidScience
Copy link
Contributor Author

@yanjunxiang-google sorry, original error that is exposed (on screenshot) did not give me a good idea on what's wrong (it was speling) so I thought it was some CI infra failure.
image

Should be fixed now.

@StupidScience
Copy link
Contributor Author

/retest

@StupidScience
Copy link
Contributor Author

@yanjunxiang-google all checks have passed finally

@alyssawilk
Copy link
Contributor

@yanjunxiang-google PTAL

yanjunxiang-google
yanjunxiang-google reviewed Jun 11, 2024
View reviewed changes
changelogs/current.yaml Outdated
@@ -226,6 +226,10 @@ new_features:
change: |
Added :ref:`bypass_overload_manager <envoy_v3_api_field_config.listener.v3.Listener.bypass_overload_manager>`
to bypass the overload manager for a listener. When set to true, the listener will not be subject to overload protection.
- area: dns_filter
change: |
Added support wildcard names in :ref:`inline_dns_table
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change to this: Added support for wildcard resolving when DNS filter is working in server mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed this, please take a look

yanjunxiang-google
yanjunxiang-google reviewed Jun 12, 2024
View reviewed changes
source/extensions/filters/udp/dns_filter/dns_filter_utils.cc
@@ -89,6 +89,19 @@ absl::string_view getDomainSuffix(const absl::string_view name) {
return name.substr(pos + 1);
}

absl::string_view getVirtualDomainName(const absl::string_view domain_name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call this function getWildcardDomainName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be incorrect. It returns not only wildcard domain names but any name.
I'm not quite happy with naming here tho. Will smth like sanitizeVirtualDomainName make intent more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's okay to me then.

yanjunxiang-google
yanjunxiang-google reviewed Jun 12, 2024
View reviewed changes
source/extensions/filters/udp/dns_filter/dns_filter_utils.cc
// wildcard usages are not considered as valid ones, i.e. **.foo.com, *foo.bar.com, foo*.bar.com
// are all invalid.
if (domain_name.starts_with("*.")) {
return domain_name.substr(1);
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What if the domain_name is invalid, like just "*.". This will return an empty string. Can we handle it correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If domain is *. it will return just .. You can see that in tests.
While working on these code I made few assumptions and cut the corners sometimes. Mostly because current code does not have any validation for passed domain names and in proto validations are: length >= 1 + header value regex, that's probably incorrect.
I did not want to increase the scope of my changes so decided not to cover validations. It is probably should be addressed, likely in different PR tho

yanjunxiang-google
yanjunxiang-google reviewed Jun 12, 2024
View reviewed changes
source/extensions/filters/udp/dns_filter/dns_filter.cc
ENVOY_LOG(trace, "Loading configuration for domain: {}. Suffix: {}", domain_name, suffix);
const absl::string_view virtual_domain_name =
Utils::getVirtualDomainName(virtual_domain.name());
const absl::string_view suffix = Utils::getDomainSuffix(virtual_domain_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we getDomainSuffix(virtual_domain.name()),
or getDomainSuffix( Utils::getVirtualDomainName(virtual_domain.name()))

Can these two return different result in some cases?

Copy link
Contributor Author

@StupidScience StupidScience Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I thought about this (getting suffix of "initial" input domain name). But with current implementation result should always be the same and for me it makes more sense to extract suffix after manipulations

yanjunxiang-google
yanjunxiang-google reviewed Jun 12, 2024
View reviewed changes
test/extensions/filters/udp/dns_filter/dns_filter_test.cc
inline_dns_table:
external_retry_count: 0
virtual_domains:
- name: "*.foobaz.com"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some tests for invalid virtual_domain test, like ".com", ".", "com", and see whether there is any issue?

Copy link
Contributor Author

@StupidScience StupidScience Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As I mentioned above, invalid domains will be accepted by envoy and (do not) work due to lack of validations.
Just answering how it will work:

  • .com will behave exactly as *.com and I'm not sure whether it is bug or feature with the current state
  • . will be accepted. We can test if that will work as catch-all wildcard for queries ended with period like foo.bar.. Shall we?
  • com in general looks like valid record

@StupidScience
Copy link
Contributor Author

/retest

@StupidScience
Copy link
Contributor Author

/retest

@yanavlasov
Copy link
Contributor

@StupidScience can you update the doc for the virtual_domain api/envoy/data/dns/v3/dns_table.proto please? It needs to clarify semantics for how how wildcards are matched.

/wait

@StupidScience
Copy link
Contributor Author

@yanavlasov thanks for feedback. Added this info as well as info about matching precedence. Can you think of any other info that should be covered?

yanavlasov
yanavlasov previously approved these changes Jun 27, 2024
View reviewed changes
@yanavlasov
Copy link
Contributor

LGTM from me. Will wait for LGTM from @yanjunxiang-google and then submit

@StupidScience please resolve the merge error.

/wait

@yanjunxiang-google
Copy link
Contributor

@StupidScience Could you please also add a couple of integration test cases here:

envoy/test/extensions/filters/udp/dns_filter/dns_filter_integration_test.cc

Line 263 in f3ee7c7

TEST_P(DnsFilterIntegrationTest, LocalLookupTest) {

It will help show how the wildcard domain name works end-to-end.

StupidScience and others added 5 commits July 1, 2024 13:25
@StupidScience
Add support for wildcard name in DNS filter inline table
dc1a66d 
Signed-off-by: Anton Kaymakchi <tonysignal@gmail.com>
@StupidScience
Change changelog description according to PR comments
b29ec4c 
Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
@StupidScience
Add clarifying comment about virtual domain name in proto files
ce796cb 
Signed-off-by: Anton Kaymakchi <tonysignal@gmail.com>
@StupidScience
Extend virtual domain name description with clarification about wildc…
39d87a3 
…ard names behaviour

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
@StupidScience
Add integration test for wildcard DNS records
957d8bc 
Signed-off-by: Anton Kaymakchi <tonysignal@gmail.com>
@StupidScience
Copy link
Contributor Author

@yanjunxiang-google added integration test, please check if that's smth you had in mind?

wbpcode
wbpcode reviewed Jul 2, 2024
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Jul 2, 2024
@yanjunxiang-google
Copy link
Contributor

LGTM

@yanjunxiang-google
Copy link
Contributor

yanjunxiang-google commented Jul 7, 2024 •
edited
Loading

@yanjunxiang-google added integration test, please check if that's smth you had in mind?

Looks good. Thanks for adding this feature!

yanavlasov
yanavlasov previously approved these changes Jul 9, 2024
View reviewed changes
@yanavlasov
Copy link
Contributor

LGTM. @StupidScience please resolve merge error.

/wait

@StupidScience
Merge branch 'main' into dns-filter-wildcard
ae8b3b2 
Signed-off-by: Anton Kaymakchi <tonysignal@gmail.com>
@StupidScience
Copy link
Contributor Author

@yanavlasov done

@yanavlasov yanavlasov merged commit 61f3681 into envoyproxy:main Jul 9, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wbpcode wbpcode wbpcode left review comments

@yanavlasov yanavlasov yanavlasov approved these changes

@yanjunxiang-google yanjunxiang-google yanjunxiang-google approved these changes

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

Assignees

@yanavlasov yanavlasov

@wbpcode wbpcode

@yanjunxiang-google yanjunxiang-google

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wildcard records support in DNS filter inline table
6 participants
@StupidScience @adisuissa @yanjunxiang-google @alyssawilk @yanavlasov @wbpcode