hidden_paths: enable registration of hiddenpaths both locally and rem…

…otely.

Fixes #4364

There were a number of issues:

* Because the normal case is that the registry is remote, the registerer makes no attempt at using the intraAS network to reach the registry (and the registry isn't listening on TCP). However, quic/scion path resolution to an AS-local host was broken: the resulting paths are expected to be complete paths with Metadata, but intraAS paths were meta-data-less paths, resulting in nill-ptr dereference.
* By default the control service port (shared with the hidden_segment registry) was a dynamic port. The discovery service cannot only knows of statically configured port (configured in the topo file). So the only way to make the hidden_segments service discoverable is to give the control service a static port. While possible, this would result in a confusing and conter-intuitive configuration:
  * the control_service quic address would be configured by the cs yaml file and be different from what appears in the topo file for the CS but identical to the hidden_segment service address.
  * the CS address in the topo file was acually the TCP address and could never be mirrored by the quic address (because that quic address was occupied by the service resolver).
  * the address of the hidden_segments registry in the topo file would be different from that of the CS, eventhough they share the same port.
* This was all invisible to the integration test which was skillfully predicting the dynamic port and configuring the discovery service for it.

To correct these infelicities:
* Give the Svc resolver a dynamic port
* By default, give to the CS and the hidden_segment registry the quic address that has the same IP and port as the control service's TCP address.
* Produce complete Path objects to represent intraAS paths.

In passing:
* A handful of AssertEqual() had arguments in the wrong order.
* In one case we were constructing a full SVC address with a nil path instead of an empty path.

acceptance/common/scion.py

@@ -14,7 +14,7 @@
 
 import logging
 import json
-from typing import Any, Dict, List, MutableMapping
+from typing import Any, Dict, List, MutableMapping, Mapping
 
 import toml
 import yaml
@@ -69,6 +69,29 @@ def update_json(change_dict: Dict[str, Any], files: LocalPath):
             json.dump(t, f, indent=2)
 
 
+def load_from_json(key: str, files: LocalPath) -> Any:
+    """ Reads the value associated with the given key from the given json files.
+
+    The first value found is returned. If not found, None is returned.
+
+    Args:
+        key: dot separated path of the JSON key.
+        files: names of file or files to read.
+
+    Returns:
+        The value. None if the path doesn't exist in the dictionary tree.
+
+    Raises:
+        IOError / FileNotFoundError: File path is not valid
+    """
+    for file in files:
+        with open(file, "r") as f:
+            t = json.load(f)
+            v = val_at_path(t, key)
+            if v is not None:
+                return v
+
+
 class ASList:
     """
     ASList is a list of AS separated by core and non-core ASes. It can be loaded
@@ -117,6 +140,18 @@ def path_to_dict(path: str, val: Any) -> Dict:
     return d
 
 
+def val_at_path(d: Mapping[str, Any], path: str) -> Any:
+    """
+    Walks nested dictionaries by following the given path and returns the value
+    associated with the leaf key.
+    """
+    v = d
+    for k in path.split('.'):
+        v = v.get(k, None)
+        if not isinstance(v, Mapping):
+            return v
+
+
 def merge_dict(change_dict: Dict[str, Any], orig_dict: MutableMapping[str, Any]):
     """
     Merge changes into the original dictionary. Leaf values in the change dict

acceptance/hidden_paths/test.py

@@ -61,16 +61,6 @@ def setup_prepare(self):
             "4": "172.20.0.65",
             "5": "172.20.0.73",
         }
-        # XXX(lukedirtwalker): The ports below are the dynamic QUIC server
-        # ports. Thanks to the docker setup they are setup consistently so we
-        # can use them. Optimally we would define a static server port inside
-        # the CS and use that one instead.
-        control_addresses = {
-            "2": "172.20.0.51:32768",
-            "3": "172.20.0.59:32768",
-            "4": "172.20.0.67:32768",
-            "5": "172.20.0.75:32768",
-        }
         # Each AS participating in hidden paths has their own hidden paths configuration file.
         hp_configs = {
             "2": "hp_groups_as2_as5.yml",
@@ -98,13 +88,16 @@ def setup_prepare(self):
             # even though some don't need the registration service.
             as_dir_path = self.artifacts / "gen" / ("ASff00_0_%s" % as_number)
 
+            # The hidden_segment services are behind the same server as the control_service.
+            topology_file = as_dir_path / "topology.json"
+            control_service_addr = scion.load_from_json(
+                'control_service.%s.addr' % control_id, [topology_file])
             topology_update = {
                 "hidden_segment_lookup_service.%s.addr" % control_id:
-                    control_addresses[as_number],
+                    control_service_addr,
                 "hidden_segment_registration_service.%s.addr" % control_id:
-                    control_addresses[as_number],
+                    control_service_addr,
             }
-            topology_file = as_dir_path / "topology.json"
             scion.update_json(topology_update, [topology_file])
 
     def setup_start(self):

control/cmd/control/main.go

@@ -200,11 +200,17 @@ func realMain(ctx context.Context) error {
 		return err
 	}
 
+	// FIXME: readability would be improved if we could be consistent with address
+	// representations in NetworkConfig (string or cooked, chose one).
 	nc := infraenv.NetworkConfig{
-		IA:                    topo.IA(),
+		IA: topo.IA(),
+		// Public: (Historical name) The TCP/IP:port address for the control service.
 		Public:                topo.ControlServiceAddress(globalCfg.General.ID),
 		ReconnectToDispatcher: globalCfg.General.ReconnectToDispatcher,
 		QUIC: infraenv.QUIC{
+			// Address: the QUIC/SCION address of this service. If not
+			// configured, QUICStack() uses the same IP and port as
+			// for the public address.
 			Address:     globalCfg.QUIC.Address,
 			TLSVerifier: trust.NewTLSCryptoVerifier(trustDB),
 			GetCertificate: cs.NewTLSCertificateLoader(
@@ -221,6 +227,7 @@ func realMain(ctx context.Context) error {
 		},
 		SCIONNetworkMetrics:    metrics.SCIONNetworkMetrics,
 		SCIONPacketConnMetrics: metrics.SCIONPacketConnMetrics,
+		MTU:                    topo.MTU(),
 	}
 	quicStack, err := nc.QUICStack()
 	if err != nil {

control/hiddenpaths.go

@@ -84,7 +84,8 @@ func (c HiddenPathConfigurator) Setup(location string) (*HiddenPathRegistrationC
 			&hpgrpc.AuthoritativeSegmentServer{
 				Lookup:   c.localAuthServer(groups),
 				Verifier: c.Verifier,
-			})
+			},
+		)
 		hspb.RegisterHiddenSegmentRegistrationServiceServer(c.InterASQUICServer,
 			&hpgrpc.RegistrationServer{
 				Registry: hiddenpath.RegistryServer{

control/tasks.go

@@ -196,6 +196,11 @@ func (t *TasksConfig) segmentWriter(segType seg.Type,
 		Writer:   writer,
 		Tick:     beaconing.NewTick(t.RegistrationInterval),
 	}
+	// The period of the task is short because we want to retry quickly
+	// if we fail fast. So during one interval we'll make as many attempts
+	// as we can until we succeed. After succeeding, the task does nothing
+	// until the end of the interval. The interval itself is used as a
+	// timeout. If we fail slow we give up at the end of the cycle.
 	return periodic.Start(r, 500*time.Millisecond, t.RegistrationInterval)
 }
 

pkg/experimental/hiddenpath/BUILD.bazel

@@ -25,6 +25,7 @@ go_library(
         "//pkg/private/serrors:go_default_library",
         "//pkg/segment:go_default_library",
         "//pkg/snet:go_default_library",
+        "//pkg/snet/path:go_default_library",
         "//private/config:go_default_library",
         "//private/pathdb:go_default_library",
         "//private/pathdb/query:go_default_library",

pkg/experimental/hiddenpath/discovery.go

@@ -21,6 +21,7 @@ import (
 	"github.com/scionproto/scion/pkg/addr"
 	"github.com/scionproto/scion/pkg/private/serrors"
 	"github.com/scionproto/scion/pkg/snet"
+	"github.com/scionproto/scion/pkg/snet/path"
 )
 
 // Servers is a list of discovered remote hidden segment server.
@@ -80,19 +81,22 @@ func (r LookupResolver) Resolve(ctx context.Context, ia addr.IA) (net.Addr, erro
 func resolve(ctx context.Context, ia addr.IA, discoverer Discoverer, router snet.Router,
 	extractAddr func(Servers) (*net.UDPAddr, error)) (net.Addr, error) {
 
-	path, err := router.Route(ctx, ia)
+	p, err := router.Route(ctx, ia)
 	if err != nil {
 		return nil, serrors.WrapStr("looking up path", err)
 	}
-	if path == nil {
+	if p == nil {
 		return nil, serrors.WrapStr("no path found to remote", err)
 	}
 	dsAddr := &snet.SVCAddr{
 		IA:      ia,
-		NextHop: path.UnderlayNextHop(),
-		Path:    path.Dataplane(),
+		NextHop: p.UnderlayNextHop(),
+		Path:    p.Dataplane(),
 		SVC:     addr.SvcDS,
 	}
+	if dsAddr.Path == nil {
+		dsAddr.Path = path.Empty{}
+	}
 	hps, err := discoverer.Discover(ctx, dsAddr)
 	if err != nil {
 		return nil, serrors.WrapStr("discovering hidden path server", err)
@@ -104,7 +108,7 @@ func resolve(ctx context.Context, ia addr.IA, discoverer Discoverer, router snet
 	return &snet.UDPAddr{
 		IA:      ia,
 		Host:    a,
-		NextHop: path.UnderlayNextHop(),
-		Path:    path.Dataplane(),
+		NextHop: dsAddr.NextHop,
+		Path:    dsAddr.Path,
 	}, nil
 }

pkg/snet/path.go

@@ -249,10 +249,6 @@ func (p *partialPath) Dataplane() DataplanePath {
 	return p.dataplane
 }
 
-func (p *partialPath) Interfaces() []PathInterface {
-	return nil
-}
-
 func (p *partialPath) Source() addr.IA {
 	return p.source
 }

pkg/snet/router.go

@@ -55,16 +55,3 @@ func (r *BaseRouter) Route(ctx context.Context, dst addr.IA) (Path, error) {
 func (r *BaseRouter) AllRoutes(ctx context.Context, dst addr.IA) ([]Path, error) {
 	return r.Querier.Query(ctx, dst)
 }
-
-// IntraASPathQuerier implements the PathQuerier interface. It will only provide
-// AS internal paths, i.e., empty paths with only the IA as destination. This
-// should only be used in places where you know that you only need to
-// communicate inside the AS.
-type IntraASPathQuerier struct {
-	IA addr.IA
-}
-
-// Query implements PathQuerier.
-func (q IntraASPathQuerier) Query(_ context.Context, _ addr.IA) ([]Path, error) {
-	return []Path{&partialPath{source: q.IA, destination: q.IA}}, nil
-}

private/app/appnet/BUILD.bazel

@@ -5,6 +5,7 @@ go_library(
     srcs = [
         "addr.go",
         "infraenv.go",
+        "intra_as.go",
     ],
     importpath = "github.com/scionproto/scion/private/app/appnet",
     visibility = ["//visibility:public"],
@@ -13,6 +14,7 @@ go_library(
         "//pkg/daemon:go_default_library",
         "//pkg/log:go_default_library",
         "//pkg/private/serrors:go_default_library",
+        "//pkg/slayers/path:go_default_library",
         "//pkg/snet:go_default_library",
         "//pkg/snet/path:go_default_library",
         "//pkg/snet/squic:go_default_library",

private/app/appnet/addr.go

@@ -161,6 +161,7 @@ func (r AddressRewriter) buildFullAddress(ctx context.Context,
 			return nil, serrors.WrapStr("Unable to resolve underlay", err)
 		}
 		ret.NextHop = ov
+		ret.Path = path.Empty{}
 	}
 
 	return ret, nil

private/app/appnet/addr_test.go

@@ -80,8 +80,8 @@ func TestRedirectQUIC(t *testing.T) {
 
 			a, r, err := aw.RedirectToQUIC(context.Background(), tc.input)
 			tc.assertErr(t, err)
-			assert.Equal(t, a, tc.wantAddr)
-			assert.Equal(t, r, tc.wantRedirect)
+			assert.Equal(t, tc.wantAddr, a)
+			assert.Equal(t, tc.wantRedirect, r)
 		})
 	}
 
@@ -100,7 +100,6 @@ func TestRedirectQUIC(t *testing.T) {
 		path.EXPECT().Metadata().Return(&snet.PathMetadata{
 			Interfaces: make([]snet.PathInterface, 1), // just non-empty
 		})
-
 		aw := infraenv.AddressRewriter{
 			Router:                router,
 			Resolver:              resolver,
@@ -137,7 +136,6 @@ func TestRedirectQUIC(t *testing.T) {
 		path.EXPECT().Metadata().Return(&snet.PathMetadata{
 			Interfaces: make([]snet.PathInterface, 1), // just non-empty
 		})
-
 		aw := infraenv.AddressRewriter{
 			Router:                router,
 			Resolver:              resolver,
@@ -182,7 +180,7 @@ func TestRedirectQUIC(t *testing.T) {
 		want := &snet.SVCAddr{
 			SVC:     addr.SvcCS,
 			NextHop: &net.UDPAddr{IP: net.ParseIP("10.1.1.1")},
-			Path:    snetpath.SCION{},
+			Path:    snetpath.Empty{},
 		}
 		a, r, err := aw.RedirectToQUIC(context.Background(), input)
 		assert.NoError(t, err)
@@ -225,7 +223,7 @@ func TestBuildFullAddress(t *testing.T) {
 			SVC:  addr.SvcCS,
 		}
 		a, err := aw.BuildFullAddress(context.Background(), input)
-		assert.Equal(t, a, input)
+		assert.Equal(t, input, a)
 		assert.NoError(t, err)
 	})
 
@@ -255,7 +253,7 @@ func TestBuildFullAddress(t *testing.T) {
 			NextHop: &net.UDPAddr{},
 			SVC:     addr.SvcCS,
 		}
-		assert.Equal(t, a, want)
+		assert.Equal(t, want, a)
 		assert.NoError(t, err)
 	})
 
@@ -285,8 +283,13 @@ func TestBuildFullAddress(t *testing.T) {
 		input := &snet.SVCAddr{IA: localIA, SVC: addr.SvcCS, Path: snetpath.Empty{}}
 		a, err := aw.BuildFullAddress(context.Background(), input)
 
-		want := &snet.SVCAddr{IA: localIA, NextHop: underlayAddr, SVC: addr.SvcCS}
-		assert.Equal(t, a, want)
+		want := &snet.SVCAddr{
+			IA:      localIA,
+			NextHop: underlayAddr,
+			SVC:     addr.SvcCS,
+			Path:    snetpath.Empty{},
+		}
+		assert.Equal(t, want, a)
 		assert.NoError(t, err)
 	})
 
@@ -388,9 +391,9 @@ func TestResolve(t *testing.T) {
 			}
 			initResolver(resolver, tc.ResolverSetup)
 			p, a, redirect, err := aw.ResolveSVC(context.Background(), path, tc.input)
-			assert.Equal(t, p, tc.wantPath)
-			assert.Equal(t, a.String(), tc.want.String())
-			assert.Equal(t, redirect, tc.wantQUICRedirect)
+			assert.Equal(t, tc.wantPath, p)
+			assert.Equal(t, tc.want.String(), a.String())
+			assert.Equal(t, tc.wantQUICRedirect, redirect)
 			tc.assertErr(t, err)
 		})
 	}
@@ -448,7 +451,7 @@ func TestParseReply(t *testing.T) {
 			if err != nil {
 				return
 			}
-			assert.Equal(t, a.String(), tc.want.String())
+			assert.Equal(t, tc.want.String(), a.String())
 		})
 	}
 }

private/app/appnet/infraenv.go

@@ -83,6 +83,8 @@ type NetworkConfig struct {
 	SCIONNetworkMetrics snet.SCIONNetworkMetrics
 	// Metrics injected into DefaultPacketDispatcherService.
 	SCIONPacketConnMetrics snet.SCIONPacketConnMetrics
+	// MTU of the local AS
+	MTU uint16
 }
 
 // QUICStack contains everything to run a QUIC based RPC stack.
@@ -103,8 +105,9 @@ func (nc *NetworkConfig) TCPStack() (net.Listener, error) {
 
 func (nc *NetworkConfig) QUICStack() (*QUICStack, error) {
 	if nc.QUIC.Address == "" {
-		nc.QUIC.Address = net.JoinHostPort(nc.Public.IP.String(), "0")
+		nc.QUIC.Address = nc.Public.String()
 	}
+
 	client, server, err := nc.initQUICSockets()
 	if err != nil {
 		return nil, err
@@ -221,7 +224,7 @@ func (nc *NetworkConfig) AddressRewriter(
 		}
 	}
 	return &AddressRewriter{
-		Router:    &snet.BaseRouter{Querier: snet.IntraASPathQuerier{IA: nc.IA}},
+		Router:    &snet.BaseRouter{Querier: IntraASPathQuerier{IA: nc.IA, MTU: nc.MTU}},
 		SVCRouter: nc.SVCResolver,
 		Resolver: &svc.Resolver{
 			LocalIA:     nc.IA,
@@ -266,9 +269,15 @@ func (nc *NetworkConfig) initSvcRedirect(quicAddress string) (func(), error) {
 		Dispatcher: packetDispatcher,
 		Metrics:    nc.SCIONNetworkMetrics,
 	}
-	conn, err := network.Listen(context.Background(), "udp", nc.Public, addr.SvcWildcard)
+
+	// The service resolution address gets a dynamic port. In reality, neither the
+	// address nor the port are needed to address the resolver, but the dispatcher still
+	// requires them and checks unicity. At least a dynamic port is allowed.
+	srAddr := &net.UDPAddr{IP: nc.Public.IP, Port: 0}
+	conn, err := network.Listen(context.Background(), "udp", srAddr, addr.SvcWildcard)
 	if err != nil {
-		return nil, serrors.WrapStr("listening on SCION", err, "addr", nc.Public)
+		log.Info("Listen failed", "err", err)
+		return nil, serrors.WrapStr("listening on SCION", err, "addr", srAddr)
 	}
 
 	ctx, cancel := context.WithCancel(context.Background())