-
Notifications
You must be signed in to change notification settings - Fork 160
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
Make hiddenpaths registration work both locally and remotely #4376
Conversation
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 realize that what I'm doing creates a protocol incompatibility. External code trying to discover the hiddenpaths service in the old way will fail, but then... it was broken already. So I do not know how much we're committed to supporting the old way.
I think there might be a ~simple way of supporting dynamic ports in the Discovery service: could we, in the topo file, permit to refer to services addresses instead of only full addresses? Then the discovery service would know to resolve (and cache) the service address into a direct one in order to answer lookups. Am I even making sense? I'm also trying to validate the fragments of understanding that I think I'm gaining :-)
Reviewable status: 0 of 53 files reviewed, all discussions resolved
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.
Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
a discussion (no related file):
I skimmed the linked issue, and it sounds like the main problem is that we try to send SCION packets for intra AS communication.
There is no need to do this exchange with SCION, and you should be able to just use regular TCP/IP.
Currently, the registry is only served on the InterASQUICServer:
Lines 81 to 103 in 4096d87
if roles.Registry { | |
log.Info("Starting hidden path authoritative and registration server") | |
hspb.RegisterAuthoritativeHiddenSegmentLookupServiceServer(c.InterASQUICServer, | |
&hpgrpc.AuthoritativeSegmentServer{ | |
Lookup: c.localAuthServer(groups), | |
Verifier: c.Verifier, | |
}) | |
hspb.RegisterHiddenSegmentRegistrationServiceServer(c.InterASQUICServer, | |
&hpgrpc.RegistrationServer{ | |
Registry: hiddenpath.RegistryServer{ | |
Groups: groups, | |
DB: &hiddenpath.Storer{ | |
DB: c.PathDB, | |
}, | |
Verifier: hiddenpath.VerifierAdapter{ | |
Verifier: c.Verifier, | |
}, | |
LocalIA: c.LocalIA, | |
}, | |
Verifier: c.Verifier, | |
}, | |
) | |
} |
I think the easiest solution would be to also serve it on c.IntraASTCPServer
, and this address should be used for intra AS registrations. That also means that there is no need for discovering the dynamic ports, because the intra AS port is static and should be available in the topology file.
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.
Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @oncilla)
a discussion (no related file):
Previously, oncilla (Dominik Roos) wrote…
I skimmed the linked issue, and it sounds like the main problem is that we try to send SCION packets for intra AS communication.
There is no need to do this exchange with SCION, and you should be able to just use regular TCP/IP.Currently, the registry is only served on the InterASQUICServer:
Lines 81 to 103 in 4096d87
if roles.Registry { log.Info("Starting hidden path authoritative and registration server") hspb.RegisterAuthoritativeHiddenSegmentLookupServiceServer(c.InterASQUICServer, &hpgrpc.AuthoritativeSegmentServer{ Lookup: c.localAuthServer(groups), Verifier: c.Verifier, }) hspb.RegisterHiddenSegmentRegistrationServiceServer(c.InterASQUICServer, &hpgrpc.RegistrationServer{ Registry: hiddenpath.RegistryServer{ Groups: groups, DB: &hiddenpath.Storer{ DB: c.PathDB, }, Verifier: hiddenpath.VerifierAdapter{ Verifier: c.Verifier, }, LocalIA: c.LocalIA, }, Verifier: c.Verifier, }, ) } I think the easiest solution would be to also serve it on
c.IntraASTCPServer
, and this address should be used for intra AS registrations. That also means that there is no need for discovering the dynamic ports, because the intra AS port is static and should be available in the topology file.
Thanks. I understand it doesn't make sense to use the SCION network to communicate within the local AS. However, in this case, the registry is expected to be in a different AS. The fact that it happens to be local is circumstantial (just because it is a minimal setup for testing). So, connecting to it by its scion address is normal.
What is the designed approach for such cases? Is the client expected to have access to both addresses and make its own choice or is there a switch somewhere that bypasses the scion network automatically? In the former case, isn't the sub-optimal choice supposed to work anyway? And if so, how much more expensive is it, really?
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.
Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @oncilla)
a discussion (no related file):
Previously, jiceatscion wrote…
Thanks. I understand it doesn't make sense to use the SCION network to communicate within the local AS. However, in this case, the registry is expected to be in a different AS. The fact that it happens to be local is circumstantial (just because it is a minimal setup for testing). So, connecting to it by its scion address is normal.
What is the designed approach for such cases? Is the client expected to have access to both addresses and make its own choice or is there a switch somewhere that bypasses the scion network automatically? In the former case, isn't the sub-optimal choice supposed to work anyway? And if so, how much more expensive is it, really?
One more question. You say that the main problem here is the use of SCION for intraAS communications. However, I am under the impression that this is just one way to observe the breakage. If the registry was actually remote, the scion address would be needed and the dynamic port would still prevent the discovery service from having an answer to provide. Am I missing something?
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.
Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
a discussion (no related file):
Thanks. I understand it doesn't make sense to use the SCION network to communicate within the local AS. However, in this case, the registry is expected to be in a different AS. The fact that it happens to be local is circumstantial (just because it is a minimal setup for testing). So, connecting to it by its scion address is normal.
I don't quite follow. If the registry is a different AS, it is never local and you always need a SCION path.
But maybe I also didn't quite grasp the initial problem.
What is the designed approach for such cases? Is the client expected to have access to both addresses and make its own choice or is there a switch somewhere that bypasses the scion network automatically? In the former case, isn't the sub-optimal choice supposed to work anyway? And if so, how much more expensive is it, really?
What is the "sub-optimal choice supposed to work anyway" in your scenario?
IMO, if the service is local, you always bypass the SCION network and never use SCION with empty path.
There is no need and/or advantage in using SCION here.
You say that the main problem here is the use of SCION for intraAS communications. However, I am under the impression that this is just one way to observe the breakage. If the registry was actually remote, the scion address would be needed and the dynamic port would still prevent the discovery service from having an answer to provide. Am I missing something?
I have not worked with hidden segments for quite some time now, so my memory is foggy.
But I expect the system to do SVC resolution for remote registries.
We have an acceptance test that tests the functionality https://github.com/scionproto/scion/blob/master/acceptance/hidden_paths/test.py which contains remote registries and has been passing in CI. Do you have a configuration other than local registry where the system does not work?
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.
Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @oncilla)
a discussion (no related file):
Previously, oncilla (Dominik Roos) wrote…
Thanks. I understand it doesn't make sense to use the SCION network to communicate within the local AS. However, in this case, the registry is expected to be in a different AS. The fact that it happens to be local is circumstantial (just because it is a minimal setup for testing). So, connecting to it by its scion address is normal.
I don't quite follow. If the registry is a different AS, it is never local and you always need a SCION path.
But maybe I also didn't quite grasp the initial problem.
What is the designed approach for such cases? Is the client expected to have access to both addresses and make its own choice or is there a switch somewhere that bypasses the scion network automatically? In the former case, isn't the sub-optimal choice supposed to work anyway? And if so, how much more expensive is it, really?
What is the "sub-optimal choice supposed to work anyway" in your scenario?
IMO, if the service is local, you always bypass the SCION network and never use SCION with empty path.
There is no need and/or advantage in using SCION here.You say that the main problem here is the use of SCION for intraAS communications. However, I am under the impression that this is just one way to observe the breakage. If the registry was actually remote, the scion address would be needed and the dynamic port would still prevent the discovery service from having an answer to provide. Am I missing something?
I have not worked with hidden segments for quite some time now, so my memory is foggy.
But I expect the system to do SVC resolution for remote registries.We have an acceptance test that tests the functionality https://github.com/scionproto/scion/blob/master/acceptance/hidden_paths/test.py which contains remote registries and has been passing in CI. Do you have a configuration other than local registry where the system does not work?
The registry would typically be in a different AS, but not necessarily. The filer of the issue was deliberately trying to use a local registry for his own purpose. So the registry may be either. In fact, I remember reading somewhere that it was even advisable to register hidden segments as up segments locally in addition to remotely, so that local entities can be aware that they're using a hidden segment (which would therefore not necessarily usable by the other party (that's as much as I understood anyway). So, both would need to work and the local case is not the majority case, even though it's the issue we just ran into.
What I mean by "sub-optimal" is using the SCION network instead of straight TCP to communicate with a host in the same AS. Yes it is unnecessary, but should it not be possible? When you say "You always bypass..." what is "you" in that phrase? Is the using code expected to make the right choice and use the TCP address when the destination is AS-local, or is there a switch layer that does the bypass for you if given both addresses (I don't remember an API resolving a set of addresses but it could very well be there).
The acceptance test passes because it hard codes the predictable (during tests) dynamic address of the registry :-)
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.
Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
a discussion (no related file):
The registry would typically be in a different AS, but not necessarily. The filer of the issue was deliberately trying to use a local registry for his own purpose. So the registry may be either. In fact, I remember reading somewhere that it was even advisable to register hidden segments as up segments locally in addition to remotely, so that local entities can be aware that they're using a hidden segment (which would therefore not necessarily usable by the other party (that's as much as I understood anyway). So, both would need to work and the local case is not the majority case, even though it's the issue we just ran into.
I agree that the registry should also work locally. I'm just trying to point out that for AS internal communication, you should use TCP/IP and not QUIC/SCION. To facilitate that, the grpc server needs to be exposed on the AS internal gRPC server too (as alluded to above.)
When you say "You always bypass..." what is "you" in that phrase?
By "you" I mean the reference implementation. The reference implementation (i.e., this repository) should use TCP/IP for AS internal communication.
The acceptance test passes because it hard codes the predictable (during tests) dynamic address of the registry :-)
Ack. I was not aware of this hack. That indeed sounds like a problem.
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.
Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
a discussion (no related file):
Previously, oncilla (Dominik Roos) wrote…
The registry would typically be in a different AS, but not necessarily. The filer of the issue was deliberately trying to use a local registry for his own purpose. So the registry may be either. In fact, I remember reading somewhere that it was even advisable to register hidden segments as up segments locally in addition to remotely, so that local entities can be aware that they're using a hidden segment (which would therefore not necessarily usable by the other party (that's as much as I understood anyway). So, both would need to work and the local case is not the majority case, even though it's the issue we just ran into.
I agree that the registry should also work locally. I'm just trying to point out that for AS internal communication, you should use TCP/IP and not QUIC/SCION. To facilitate that, the grpc server needs to be exposed on the AS internal gRPC server too (as alluded to above.)
When you say "You always bypass..." what is "you" in that phrase?
By "you" I mean the reference implementation. The reference implementation (i.e., this repository) should use TCP/IP for AS internal communication.
The acceptance test passes because it hard codes the predictable (during tests) dynamic address of the registry :-)
Ack. I was not aware of this hack. That indeed sounds like a problem.
To solve the dynamic port issue, you could require that an explicit port is required in the QUIC configuration of the control service. In that case, the port is statically allocated and can be configured in the discovery service.
scion/private/app/appnet/infraenv.go
Lines 105 to 107 in 4096d87
if nc.QUIC.Address == "" { | |
nc.QUIC.Address = net.JoinHostPort(nc.Public.IP.String(), "0") | |
} |
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.
Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
a discussion (no related file):
Previously, oncilla (Dominik Roos) wrote…
To solve the dynamic port issue, you could require that an explicit port is required in the QUIC configuration of the control service. In that case, the port is statically allocated and can be configured in the discovery service.
scion/private/app/appnet/infraenv.go
Lines 105 to 107 in 4096d87
if nc.QUIC.Address == "" { nc.QUIC.Address = net.JoinHostPort(nc.Public.IP.String(), "0") }
you could require that an explicit port is required in the QUIC configuration of the control service if a hidden segment registry is served by the control service.
…terfaces() method.
* give the control service a static port. * use that in the topo config as the hidden_path service address for use by the discovery service. * This change does not yet include the associated changes to the config generators.
The CS public address and the CS scion address can now be the same, while the service resolver explicitly gets a different port. I am experimenting with the possibility of configuring the service resolver address as the regular address field of a new service named "service_resolution". (oncilla@ thinks it's a bad idea. I do not understand why yet but I guess I'll find out). Also updated tests to match the intended changes, and to call assert.Equal() with arguments in the correct order.
If the service resolution address isn't configured, let it have a random port. The dispatcher dynamically assigns a port and also registers it as the recipient of resolution requests. It appears that no knowledge of the port is required outside of that.
d8d9033
to
0c24724
Compare
Regarding local communication: using TCP/IP for local communication is of course also doable, but IMO still a bit pointless -- at least currently, we know that the hidden path service is in the same process. Adding special case logic to expose and call this on TCP seems superfluous. I'd either use the QUIC/SCION as in the general case, with the required small fixes to make this work, or alternatively bypass the whole RPC and invoke the procedure directly. On the topic of dynamic port for the QUIC address and service discovery. The current setup with these different addresses is needlessly complex. For reference, the setup occurs in appnet/infaenv.go. This creates two "server sockets" (plus another one for the client):
Note that the service redirector only ever cares about packets to service addresses. It will never receive (or at least never care about) packets to the My suggestion would be: get rid of the separate "QUIC" address in the toml configuration and use the "public" address for QUIC instead. To avoid clashing this binding with the service redirector, there are a number of solutions:
|
Thanks! You're addressing the questions I left in the issue.
Yup. I want to do that too.
Not as simple and not sure of the immediate benefits. I'd rather keep that as part of a dedicated effort to cleanup the service resolution. For example in view of killing the dispatcher? You didn't mention another possibility:
|
… dynamic port. Doing this allows the CS's SCION address to have the same host/port as the CS's public address (by default) instead of being dynamic. This in turn makes it discoverable, which allows the hidden path registry to be reachable. This approach allows the topology file to declare the hidden_path registry address to be obviously identical to the address configured for the CS... much less confusing.
… doesn't need to be configurable.
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.
Reviewable status: 56 of 67 files reviewed, all discussions resolved (waiting on @matzf)
pkg/snet/path.go
line 71 at r7 (raw file):
Previously, jiceatscion wrote…
Anyway. Here's my latest proposal. I'm ok with moving the intraASquerier stuff somewhere else you tell me, but I'd prefer not having to move/refactor more things within this PR.
Move it to private/app/appnet
as @matzf suggested and we should be good to merge.
(my comment was trying to add context, I should have made it clearer that it is not meant as request for more changes)
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.
Reviewable status: 56 of 67 files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @matzf)
pkg/snet/path/intraAS.go
line 15 at r13 (raw file):
// limitations under the License. // Package path implements snet.Path with full metadata
nit: there should only be one package doc string.
(probably copy pasta)
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.
Reviewed 9 of 11 files at r12, 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
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.
Reviewable status: 64 of 68 files reviewed, all discussions resolved (waiting on @matzf and @oncilla)
pkg/snet/path.go
line 71 at r7 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Move it to
private/app/appnet
as @matzf suggested and we should be good to merge.(my comment was trying to add context, I should have made it clearer that it is not meant as request for more changes)
done
pkg/snet/path/intraAS.go
line 15 at r13 (raw file):
Previously, oncilla (Dominik Roos) wrote…
nit: there should only be one package doc string.
(probably copy pasta)
done
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.
moved the querier to appnet as you and oncilla suggested.
Reviewable status: 64 of 68 files reviewed, all discussions resolved (waiting on @matzf and @oncilla)
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.
Reviewed 6 of 11 files at r12, 2 of 2 files at r13, 4 of 4 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion)
private/app/appnet/intraAS.go
line 45 at r14 (raw file):
MTU: q.MTU, Expiry: time.Now().Add(rawpath.MaxTTL * time.Second), },
Also add Dataplane: snetpath.Empty{}
, then this is a proper path, same as the one returned from path queries via the daemon.
… truly complete and safe to use.
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.
Reviewable status: 67 of 68 files reviewed, 1 unresolved discussion (waiting on @matzf)
private/app/appnet/intraAS.go
line 45 at r14 (raw file):
Previously, matzf (Matthias Frei) wrote…
Also add
Dataplane: snetpath.Empty{}
, then this is a proper path, same as the one returned from path queries via the daemon.
done
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.
Reviewed 3 of 4 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)
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.
Reviewed 2 of 17 files at r2, 44 of 44 files at r3, 1 of 11 files at r4, 44 of 50 files at r5, 7 of 9 files at r7, 1 of 4 files at r10, 1 of 1 files at r11, 6 of 11 files at r12, 2 of 2 files at r13, 3 of 4 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jiceatscion and @matzf)
acceptance/common/scion.py
line 70 at r15 (raw file):
with open(file, "w") as f: json.dump(t, f, indent=2)
The changes in this file don't seem to be used anywhere, or am I missing something?
acceptance/common/scion.py
line 72 at r15 (raw file):
def load_from_json(key: str, files: LocalPath) -> Any:
should that be a list of LocalPath
? or does it need to be directory.
Code quote:
files: LocalPath
private/app/appnet/intraAS.go
line 1 at r15 (raw file):
// Copyright 2020 ETH Zurich
nit: camel case is not very usual for Go file names. I would name it intra_as.go
instead.
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.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jiceatscion and @matzf)
a discussion (no related file):
please clean up the title and the description to contain only what it does now.
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.
Reviewable status: 66 of 68 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker, @matzf, and @oncilla)
acceptance/common/scion.py
line 70 at r15 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
The changes in this file don't seem to be used anywhere, or am I missing something?
update_json() and now load_from_json() are used by acceptance/hidden_paths/test.py.
acceptance/common/scion.py
line 72 at r15 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
should that be a list of
LocalPath
? or does it need to be directory.
LocalPath is a weird overengineered beast. It is constructed from a list of paths fragments and, if that ends-up being a directory, is iterable to produce all the files in the directory. Neither features are used but because we use LocalPath we are bound to assume there could be multiple files (at least that is no harder than checking there's only one anyway). I didn't start this. That's how update_json was to begin with. I just added the reciprocal method.
private/app/appnet/intraAS.go
line 1 at r15 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
nit: camel case is not very usual for Go file names. I would name it
intra_as.go
instead.
done
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.
Reviewable status: 66 of 68 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker, @matzf, and @oncilla)
a discussion (no related file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
please clean up the title and the description to contain only what it does now.
Done.
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.
Reviewed 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)
private/app/appnet/intraAS.go
line 45 at r14 (raw file):
Previously, jiceatscion wrote…
done
This should be resolved now. Could one of the esteemed reviewers merge the PR?
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.
Reviewed 2 of 2 files at r16.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)
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.
Reviewed 2 of 2 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)
Reflect the changes introduced in #4376
Reflect the changes introduced in scionproto#4376
hidden_paths: Enable registration of hiddenpaths both locally and remotely.
Fixes #4364
There were a number of issues:
To correct these infelicities:
In passing:
This change is