feat(cli): add custom dns resolver option #587
Conversation
|
Converting to draft while I fix the conflicts… |
use an `Option<String>` rather than a `String`, defaulting to `""`.
PsypherPunk
force-pushed
the
531-custom-dns-resolver
branch
from
cf6cf9f to
0e4d8a8
Compare
|
error: could not copy file from 'C:\Users\runneradmin\.cargo\bin\rustup-init.exe' to 'C:\Users\runneradmin\.cargo\bin\rustup.exe': Access is denied. (os error 5)
Unexpected error attempting to determine if executable file exists 'C:\Users\runneradmin\.cargo\bin\rustup.exe': Error: EPERM: operation not permitted, stat 'C:\Users\runneradmin\.cargo\bin\rustup.exe'Update: it fixed itself on the next run. |
- remove extraneous `std::net::` prefixes for already-imported `IpAddr`. - move derivation to a function: - parse input/file as normal; - attempt to use the system DNS resolution; - fall back to previous CloudFlare behaviour.
PsypherPunk
force-pushed
the
531-custom-dns-resolver
branch
from
0e4d8a8 to
8fd1f6e
Compare
| #[test] | ||
| fn resolver_default_cloudflare() { | ||
| let opts = Opts::default(); | ||
|
|
||
| let resolver = get_resolver(&opts.resolver); | ||
| let lookup = resolver.lookup_ip("www.example.com.").unwrap(); | ||
|
|
||
| assert!(opts.resolver.is_none()); | ||
| assert!(lookup.iter().next().is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolver_args_google_dns() { | ||
| let mut opts = Opts::default(); | ||
| // https://developers.google.com/speed/public-dns | ||
| opts.resolver = Some("8.8.8.8,8.8.4.4".to_owned()); | ||
|
|
||
| let resolver = get_resolver(&opts.resolver); | ||
| let lookup = resolver.lookup_ip("www.example.com.").unwrap(); | ||
|
|
||
| assert!(lookup.iter().next().is_some()); | ||
| } |
There was a problem hiding this comment.
I think we try to avoid network tests so you can test on machines without the internet, in this case mocking the test output 🤔
But I think for the things which require this (Homebrew requires no networked tests) we have our own different tests specific to it
Although generally I regualrly make the argument that you cannot test a network CLI tool without some kind of network 😂 so approved!
There was a problem hiding this comment.
Yep, I mulled this one over for a while: the only thing I could think would be to wrap the Resolver in a "New Type" which could capture the config. sent to it an expose that. That way, we could:
a. better test that each branch of get_resolver results in the right DNS resolvers being referenced (and should mitigate the need to actually make DNS requests in the tests).
b. potentially derive a StructOpt from_os_str for it and create it upstream in Opts, isolating a bit more from the parsing code.
…but that seemed a bit much after a hefty rebase 😁
There was a problem hiding this comment.
I'm aware in PyTest that you can add "tags" to tests, so you could create tests like "pytest --tag online" to test the online features
that way we can keep it to testing offline for locally and online for CI etc....
Not sure if cargo supports that, and again it isn't really a good solution :(
Jeeze, testing networking tools is hard!
|
Hey @PsypherPunk I added you as a maintainer of RustScan! I think you should be able to push to master and run your own CI now? 🤔 |
|
Thanks, @bee-san! |
Note: this extends #531 (to which I don't have write-access) but retains all the commits therein (all credit to @khanhnt2).
relates #531
As discussed on that PR:
Resolverhides a lot of its internal fields so validating the exact type built doesn't appear to be straightforward).--resolveris passed orresolveris set in config: