-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
dns: add dns resolver implementation based on Apple APIs #13074
Changes from 47 commits
6b72142
afb5731
3f6adce
8e3e44e
204f3dd
b5ecbdb
6fb5a14
3ecd18a
1c972cc
d90ab49
09c8946
875458a
44a1ce6
25cc48f
2797de8
1001842
5c3ab10
eb85d38
5f79bd3
6ff5a29
8374eb6
3fd1cc9
a5477a9
c1facb7
04e2861
b220407
7167cb6
8062ad0
29749aa
2bada07
896d88a
2332f23
7516419
ddf8456
41ba439
72cc832
9286227
c3bbdd6
9a6120d
4491132
2d04495
b533f2c
4800d0d
fd2521f
6737b8f
6f14724
40b40e4
80fb3f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be rejecting the config earlier in a more graceful way? If so, shouldn't these be
ASSERT
s?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should be exceptions that fail during config load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It all comes down to snapping the runtime once by making it static local state here vs checking earlier in callers of this function.
I guess I could change this function to return nullptr on failure, and have callers deal with the return value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the DNS resolver is created during config load time in all cases. I think you can just throw an exception here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's the case for all xDS, assuming this also holds for dynamic forward proxy, SG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It holds for the dynamic proxy. Let me check in the UDP filter. If that's the case, I'll update with an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htuch @mattklein123 unfortunately, the UDP DNS filter creates a resolver (underneath its own wrapper) for every filter instance construction:
envoy/source/extensions/filters/udp/dns_filter/dns_filter.cc
Line 229 in 3b5acb2
So can't throw an exception as is (and per @htuch the assert is not great either).
The underlying resolver in the UDP DNS filter should definitely be shared, and I can work to untangle. However, I don't want to block this PR too much (we need it to be able to run envoy mobile on ios 14 devices). How do you feel about landing this PR with a warn log in place of the assert. Then I can work on untangling the filter situation, and subsequently upgrading the warn log here into an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually rather you just leave the RELEASE_ASSERT and make sure the error message is good so the OSX user can flip the runtime flag, then come back and clean it up. So just leave a TODO for that? @htuch?