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

Simplify and extend hostname resolution #155

Merged
merged 6 commits into from
Aug 6, 2020
Merged

Conversation

matzf
Copy link
Contributor

@matzf matzf commented Jul 22, 2020

Make the hostname resolution more "pluggable", using separate Resolvers
for reading from /etc/hosts and RAINS.

Resolving from the /etc/hosts file (by hostsfileResolver) now reads the
file on every request instead of parsing on startup. This is more
aligned with what e.g. gethostbyname does, making modifications to the
file visible to running applications.

Because it's easy, add an additional resolver that loads
/etc/scion/hosts, in the same format as /etc/hosts. The idea is that by
having a separate file, we can now easily distribute a simple hosts file
from the coordinator.

Remove the fixed hosts table with AddHost and the only
half-implemented reverse lookup GetHostnamesByAddress.

Add explicit build tag 'norains' to build without RAINS support.
Building with make 'TAGS=norains' will build without rains.
This is handy e.g. when updating the SCION dependency with API breaking
changes here in scion-apps but RAINS has not yet been updated.


This change is Reviewable

@matzf matzf force-pushed the hostname-resolution-cleanup branch from 59edd2d to 7927638 Compare July 22, 2020 12:09
Copy link
Contributor

@FR4NK-W FR4NK-W left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 9 files at r1.
Reviewable status: 2 of 9 files reviewed, 2 unresolved discussions (waiting on @matzf)


Makefile, line 49 at r1 (raw file):

-tags=$(TAGS)

See below.


pkg/appnet/rains.go, line 15 at r1 (raw file):

rains

Can we invert the build constraint, i.e. !norains?
So that by default if no tag is specified applications get built with rains support? Especially since one might easily forget it when not using the Makefile.

Copy link
Contributor

@FR4NK-W FR4NK-W left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 9 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @matzf)


pkg/appnet/hosts.go, line 52 at r1 (raw file):

resolverList

I think the precedence of the resolvers in defaultResolver should be documented more explicitly, since Resolve on resolverList does not show if an entry in a earlier Resolver shadows an entry in a later one (as expected), but also fails if resolving in one Resolver does, but would succeed on a later one.


pkg/appnet/hosts.go, line 62 at r1 (raw file):

:port

Why include the port?
The port is not used for resolving the host name, maybe I'm just confused by the function name. So maybe ParseUDPAddrAt, since resolving is just one (possible) step?


pkg/appnet/hosts.go, line 136 at r1 (raw file):

const iaIndex = 1

those consts are directly related to addrRegexp, is there a reason we don't define them together?


pkg/appnet/hosts_test_file, line 12 at r1 (raw file):

[10.0.8.10] 

There is a tab character after the closing ]. Please remove it.


pkg/appnet/resolver_test.go, line 43 at r1 (raw file):

fmt.Println("no haz rains")

I don't see a test for the override of an entry of one of the hosts files by an other.


pkg/shttp/transport_test.go, line 96 at r1 (raw file):

17-ffaa:0:1

Nit: I think we should use the test / documentation IAs to be consistent with https://github.com/scionproto/scion/wiki/ISD-and-AS-numbering.

@matzf matzf force-pushed the hostname-resolution-cleanup branch from 980e1b9 to f37dec0 Compare August 5, 2020 09:09
Copy link
Contributor Author

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 12 files reviewed, 7 unresolved discussions (waiting on @FR4NK-W)


Makefile, line 49 at r1 (raw file):

Previously, FR4NK-W wrote…
-tags=$(TAGS)

See below.

Done.


pkg/appnet/hosts.go, line 52 at r1 (raw file):
Ok, done.

but also fails if resolving in one Resolver does, but would succeed on a later one

Yes, that just seems like the most sensible thing to do, no? The alternatives I see are to either ignore the errors, or return the result of a lower-precedence resolver_and_ the error. Ignoring the error just seems bad and returning a result and an error is very confusing for the caller.
Btw. while errors are of course possible, the hostfile resolvers will only fail when there are issues with reading the (existing) file, file system problems and such.


pkg/appnet/hosts.go, line 62 at r1 (raw file):

Previously, FR4NK-W wrote…
:port

Why include the port?
The port is not used for resolving the host name, maybe I'm just confused by the function name. So maybe ParseUDPAddrAt, since resolving is just one (possible) step?

It includes the port, because handling UDP addresses with ports is what this function does. 😕
The function does the equivalent of net.ResolveUDPAddr, I think it makes sense to use the same name.


pkg/appnet/hosts.go, line 136 at r1 (raw file):

Previously, FR4NK-W wrote…
const iaIndex = 1

those consts are directly related to addrRegexp, is there a reason we don't define them together?

Done.


pkg/appnet/hosts_test_file, line 12 at r1 (raw file):

Previously, FR4NK-W wrote…
[10.0.8.10] 

There is a tab character after the closing ]. Please remove it.

Done.


pkg/appnet/rains.go, line 15 at r1 (raw file):

Previously, FR4NK-W wrote…
rains

Can we invert the build constraint, i.e. !norains?
So that by default if no tag is specified applications get built with rains support? Especially since one might easily forget it when not using the Makefile.

Ok. I dislike the double negative, but there does not seem to be any way to avoid it. /shrug


pkg/appnet/resolver_test.go, line 43 at r1 (raw file):

Previously, FR4NK-W wrote…
fmt.Println("no haz rains")

I don't see a test for the override of an entry of one of the hosts files by an other.

Ok


pkg/shttp/transport_test.go, line 96 at r1 (raw file):

Previously, FR4NK-W wrote…
17-ffaa:0:1

Nit: I think we should use the test / documentation IAs to be consistent with https://github.com/scionproto/scion/wiki/ISD-and-AS-numbering.

Ok, fixed here, but I'm not fixing up all of these right now.

@matzf matzf force-pushed the hostname-resolution-cleanup branch 2 times, most recently from 173740e to 54a2304 Compare August 5, 2020 10:21
matzf added 5 commits August 5, 2020 12:23
Make the hostname resolution more "pluggable", using separate Resolvers
for reading from /etc/hosts and RAINS.

Resolving from the /etc/hosts file (by hostsfileResolver) now reads the
file on every request instead of parsing on startup. This is more
aligned with what e.g. gethostbyname does, making modifications to the
file visible to running applications.

Because it's easy, add an additional resolver that loads
/etc/scion/hosts, in the same format as /etc/hosts. The idea is that by
having a separate file, we can now easily distribute a simple hosts file
from the coordinator.

Remove the fixed hosts table with `AddHost` and the only
half-implemented reverse lookup `GetHostnamesByAddress`.

Add explicit build tag 'rains' to build with RAINS support.
Building with `make 'TAGS=!rains'` (or just empty tags) will build
without rains.
This is handy e.g. when updating the SCION dependency with API breaking
changes here in scion-apps but RAINS has not yet been updated.
Define indexes for regexes as global consts
@matzf matzf force-pushed the hostname-resolution-cleanup branch from 54a2304 to ab230f9 Compare August 5, 2020 10:24
Copy link
Contributor

@FR4NK-W FR4NK-W left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 10 of 10 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@matzf matzf merged commit af13021 into master Aug 6, 2020
@matzf matzf deleted the hostname-resolution-cleanup branch August 6, 2020 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants