-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat(stdlib): add dns_lookup
function
#764
Conversation
Adds `dns_lookup` function, relying on the `domain` crate (`dns_lookup` crate doesn't support any options). Fixes: vectordotdev#720
I have tried implementing this with
|
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.
@johnhtodd do you mind taking a look at this one to make sure the interface matches what you expected?
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.
domain
does pull in quite a few new dependencies including Tokio 😅 @esensar if you are open to it, I wouldn't mind evaluating the other options more thoroughly to see if there is one that is a bit lighter.
Yeah, definitely. Seeing tokio made me reconsider it :D |
I admit I'm not able to follow the threads of functionality features all the way out to the individual crate options. However, the comment that "dns_lookup" supports very few options means that really can't be useful in the context that most users of this would want. The ability to specify target servers, dnssec yes/no, quantity of attempts, timeouts, or alternative DNS formats (tcp, doh, dot) make this not viable. I would be fine with hickory_client, and I think it could even be "dumbed down" to not do internal DNSSEC validation and just be a forwarding proxy. I do not think that vector should take on the tasks of DNS resolution, even in a crate add-on, so my interests are in how the functionality can be minimized in Vector and outsourced to an upstream persistent DNS resolver. Does hickory_crate or dns_lookup provide these minimal specs: Required in query:
Required supported additional flags/options in query:
Mandatory response items:
Everything else is optional, as far as my own (selfish) interests when looking at the original list of options I noted in #720 |
I think almost all of these are supported by If using |
Thanks for researching @esensar . I guess it's not too surprising given they would need to issue asynchronous requests. If |
I have started moving this to
|
I don't think it is a problem to always specify a resolver in the command, so if that is the solution then I'd say that's reasonable. Using /etc/resolv.conf would have been convenient as a default, but that is not a requirement. |
Okay, maybe we can start with required parameter for simplicity. |
I converted this back to draft since it seems like you are still iterating on it, but feel free to mark it ready when it is ready for review. Thanks for your work on this one! |
Sorry for once again bringing up the crate concern here, but after spending some time trying to get On the other hand Not sure how to progress on this, I am worried about making this function unnecessarily complex and hard to maintain. If DoH and DoT are not critical to be included, I believe going the |
I agree, |
DoT and DoH are not absolutely required, so if |
Unfortunately, I don't see a way to control dnssec in |
DNSSEC is requested by explicitly setting/unsetting the "DO" bit on the request. NSID has to be requested explicitly on the outbound connection in order to appear in the reply, though EDNS EDE is returned if EDNS is flagged as being supported in the outbound query (which most modern DNS stacks should do, I assume.) |
I don't see an option to contol the |
@@ -0,0 +1,4 @@ | |||
Added experimental `dns_lookup` function. It should be used with caution, since it involves network |
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.
Perhaps we should put it behind a feature flag, at least until we introduce some function result caching mechanism.
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.
That makes sense, although doesn't reverse_dns
also include network calls? Should that one be put behind a network flag too, or does that one utilize some kind of caching?
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.
That one predates me 😅 But point taken, let's just document this appropriately.
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.
Sorry, just checking if I understood correctly. Based on discussion from: #720 I thought this function was supposed to be undocumented (at least until caching is sorted out), similar to reverse_dns
.
I can add a warning to make it more visible that it involves network calls and should be avoided in most cases if we want to have it documented in the end though.
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.
for the above question cc @jszwedko
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'm still fine with adding this but not documenting it, for the time being, until we can sort out a caching strategy.
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.
Code looks good. One thing I noticed is that the tests are not checking all the possible fields. It would be good to strengthen them.
That makes sense, I will expand them. |
Test coverage is good now, but I see a few test failures. You can run the CI locally with |
Yeah, I was just about to write about that. I will have to fix that, I don't think it will take long. It shouldn't interfere with review though, if you want to take a final look. |
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.
Nice work @esensar
Adds
dns_lookup
function, relying on thedomain
crate (dns_lookup
crate doesn't support any options).Fixes: #720