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.

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

Run DNS lookup for --api endpoint provided in CLI #5372

Merged
merged 5 commits into from
Aug 23, 2018

Conversation

raulk
Copy link
Member

@raulk raulk commented Aug 11, 2018

This is my first PR to go-ipfs, so please be gentle ;-)

Imports go-multiaddr-dns and calls madns.Resolve() with the provided multiaddr. There's no need to call Matches() explicitly, as the resolution will do no-op if no DNS protocols are found in the input multiaddr (I'm assuming this is part of the contract of madns.Resolve()).

If multiple results are returned, only the first one is used. See #5249 (comment).

I've added several unit tests in the dnsresolve_test.go file. Not sure if that's the best place. I'm happy to change it.

I initially added the call to madns.Resolve() in apiClientForAddr() but I couldn't assert on the address of http.Client as its private, so I had to put it in a separate function to enable unit testing.

Fixes #5249.

@raulk raulk requested a review from Kubuxu as a code owner August 11, 2018 22:39
@raulk raulk force-pushed the dns-resolution-api-endpoint branch 2 times, most recently from 9a1d041 to 03c1752 Compare August 11, 2018 22:49
@Stebalien
Copy link
Member

Nice! We usually don't bother unit testing small helper functions like that but that's just because we're horrible, lazy coders so I definitely won't say no to it.

Mind adding an integration test? You can find the others in test/sharness. Basically, all we need to do is start an ipfs daemon test_init_ipfs and test_launch_ipfs_daemon, test that ipfs --api=/dns4/localhost/tcp/5001 SOMETHINGworks, and then stop the daemon withtest_kill_ipfs_daemon`.

@Stebalien Stebalien added the need/author-input Needs input from the original author label Aug 16, 2018
@raulk
Copy link
Member Author

raulk commented Aug 17, 2018

@Stebalien – added a sharness test, modelled after t0235.

Only testing the appearance of the POST, which is enough to assert that the DNS resolution is happening.

Had to use an existing domain that resolves to 127.0.0.1, localtest.me, as the kernel resolves localhost through /etc/hosts, not a DNS lookup.

@Stebalien
Copy link
Member

Had to use an existing domain that resolves to 127.0.0.1, localtest.me, as the kernel resolves localhost through /etc/hosts, not a DNS lookup.

Actually, I'd much rather use localhost. That way the tests pass while offline (e.g., when on a plane). Really, we don't care about actually using DNS, we just want to hit the multiaddr-dns resolver.

@ghost ghost assigned Stebalien Aug 23, 2018
@ghost ghost added the status/in-progress In progress label Aug 23, 2018
Raúl Kripalani and others added 5 commits August 22, 2018 18:17
License: MIT
Signed-off-by: Raúl Kripalani <raul.kripalani@consensys.net>
Resolves ipfs#5249. Calls multiaddr-dns, and picks the first result.
Uses a fixed timeout of 10 seconds. Adds test cases for one, multiple,
and no DNS results.

License: MIT
Signed-off-by: Raúl Kripalani <raul.kripalani@consensys.net>
License: MIT
Signed-off-by: Raúl Kripalani <raul.kripalani@consensys.net>
License: MIT
Signed-off-by: Raúl Kripalani <raul.kripalani@gmail.com>
License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@Stebalien Stebalien added need/review Needs a review and removed status/in-progress In progress need/author-input Needs input from the original author labels Aug 23, 2018
@Stebalien
Copy link
Member

(figured I might as well just fix that rather than make you do yet another round of CR).

Rebased and probably ready...

@Stebalien Stebalien requested a review from kevina August 23, 2018 03:32
@Stebalien Stebalien merged commit 4fb2666 into ipfs:master Aug 23, 2018
@raulk
Copy link
Member Author

raulk commented Aug 23, 2018

Thanks @Stebalien, somehow I had missed this.

I had wrongly assumed that net.Resolver.LookupIPAddr() behaved like nslookup, which does not resolve localhost because it goes straight to the nameservers. However, LookupIPAddr() does resolve localhost properly as it goes via the kernel.

All good, thanks 👍

@raulk raulk deleted the dns-resolution-api-endpoint branch August 23, 2018 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--api should accept dns addresses
2 participants