Repository: scionproto/scion
Pull request #3423 (2019-11-22 13:48:04)

invraenv: use net.UDPAddr for public/bind addr


<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/scionproto/scion/3423)
<!-- Reviewable:end -->



Patch:
diff --git a/go/beacon_srv/main.go b/go/beacon_srv/main.go
index 945fee44e..c626793ba 100644
--- a/go/beacon_srv/main.go
+++ b/go/beacon_srv/main.go
@@ -132,7 +132,7 @@ func realMain() int {
 	opentracing.SetGlobalTracer(tracer)
 	nc := infraenv.NetworkConfig{
 		IA:                    topo.IA(),
-		Public:                topo.SPublicAddress(addr.SvcBS, cfg.General.ID),
+		Public:                topo.PublicAddress(addr.SvcBS, cfg.General.ID),
 		SVC:                   addr.SvcBS,
 		ReconnectToDispatcher: cfg.General.ReconnectToDispatcher,
 		QUIC: infraenv.QUIC{
@@ -191,8 +191,9 @@ func realMain() int {
 	// We do not need to drain the connection, since the src address is spoofed
 	// to contain the topo address.
 	ovAddr := topo.PublicAddress(addr.SvcBS, cfg.General.ID)
-	ovAddr.L4 = 0
-	conn, _, err := pktDisp.RegisterTimeout(topo.IA(), ovAddr, nil, addr.SvcNone, time.Second)
+	t := addr.AppAddrFromUDP(ovAddr)
+	t.L4 = 0
+	conn, _, err := pktDisp.RegisterTimeout(topo.IA(), t, nil, addr.SvcNone, time.Second)
 	if err != nil {
 		log.Crit("Unable to create SCION packet conn", "err", err)
 		return 1
@@ -290,15 +291,10 @@ func (t *periodicTasks) Start() error {
 	}
 	t.running = true
 	topo := t.topoProvider.Get()
-	topoAddress := topo.PublicAddress(addr.SvcBS, cfg.General.ID)
-	if topoAddress == nil {
+	bs := topo.PublicAddress(addr.SvcBS, cfg.General.ID)
+	if bs == nil {
 		return serrors.New("Unable to find topo address")
 	}
-	bs := &net.UDPAddr{
-		IP:   topoAddress.L3.IP(),
-		Port: int(topoAddress.L4),
-	}
-
 	var err error
 	if t.registrars, err = t.startSegRegRunners(); err != nil {
 		return err
diff --git a/go/cert_srv/setup.go b/go/cert_srv/setup.go
index aea8cc50e..945c20f27 100644
--- a/go/cert_srv/setup.go
+++ b/go/cert_srv/setup.go
@@ -164,7 +164,7 @@ func setMessenger(cfg *config.Config, router snet.Router) error {
 	}
 	nc := infraenv.NetworkConfig{
 		IA:                    topo.IA(),
-		Public:                topo.SPublicAddress(addr.SvcCS, cfg.General.ID),
+		Public:                topo.PublicAddress(addr.SvcCS, cfg.General.ID),
 		SVC:                   addr.SvcCS,
 		ReconnectToDispatcher: cfg.General.ReconnectToDispatcher,
 		QUIC: infraenv.QUIC{
diff --git a/go/lib/infra/infraenv/infraenv.go b/go/lib/infra/infraenv/infraenv.go
index b40383c76..a7b74316f 100644
--- a/go/lib/infra/infraenv/infraenv.go
+++ b/go/lib/infra/infraenv/infraenv.go
@@ -61,9 +61,7 @@ type NetworkConfig struct {
 	IA addr.IA
 	// Public is the Internet-reachable address in the case where the service
 	// is behind NAT.
-	Public *snet.Addr
-	// Bind is the local address the server should listen on.
-	Bind *snet.Addr
+	Public *net.UDPAddr
 	// SVC registers this server to receive packets with the specified SVC
 	// destination address.
 	SVC addr.HostSVC
@@ -152,7 +150,10 @@ func (nc *NetworkConfig) AddressRewriter(
 		Resolver: &svc.Resolver{
 			LocalIA:     nc.IA,
 			ConnFactory: connFactory,
-			Machine:     buildLocalMachine(nc.Bind, nc.Public),
+			Machine: snet.LocalMachine{
+				PublicIP:    nc.Public.IP,
+				InterfaceIP: nc.Public.IP,
+			},
 			// Legacy control payloads have a 4-byte length prefix. A
 			// 0-value for the prefix is invalid, so SVC resolution-aware
 			// servers can use this to detect that the client is attempting
@@ -170,7 +171,12 @@ func (nc *NetworkConfig) AddressRewriter(
 // resolution requests. If argument address is not the empty string, it will be
 // included as the QUIC address in SVC resolution replies.
 func (nc *NetworkConfig) initUDPSocket(quicAddress string) (net.PacketConn, error) {
-	reply := messenger.BuildReply(nc.Public.Host)
+	reply := &svc.Reply{
+		Transports: map[svc.Transport]string{
+			svc.UDP: nc.Public.String(),
+		},
+	}
+
 	if quicAddress != "" {
 		reply.Transports[svc.QUIC] = quicAddress
 	}
@@ -196,7 +202,12 @@ func (nc *NetworkConfig) initUDPSocket(quicAddress string) (net.PacketConn, erro
 		},
 	)
 	network := snet.NewCustomNetworkWithPR(nc.IA, packetDispatcher)
-	conn, err := network.ListenSCIONWithBindSVC("udp4", nc.Public, nc.Bind, nc.SVC, 0)
+	var listenAddr *snet.Addr
+	if nc.Public != nil {
+		listenAddr = &snet.Addr{IA: nc.IA, Host: addr.AppAddrFromUDP(nc.Public)}
+	}
+
+	conn, err := network.ListenSCIONWithBindSVC("udp4", listenAddr, nil, nc.SVC, 0)
 	if err != nil {
 		return nil, common.NewBasicError("Unable to listen on SCION", err)
 	}
@@ -248,17 +259,6 @@ func (nc *NetworkConfig) buildQUICConfig(conn net.PacketConn) (*messenger.QUICCo
 	}, nil
 }
 
-func buildLocalMachine(bind, public *snet.Addr) snet.LocalMachine {
-	var mi snet.LocalMachine
-	mi.PublicIP = public.Host.L3.IP()
-	if bind != nil {
-		mi.InterfaceIP = bind.Host.L3.IP()
-	} else {
-		mi.InterfaceIP = mi.PublicIP
-	}
-	return mi
-}
-
 // LegacyForwardingHandler is an SVC resolution handler that only responds to
 // packets that have an SVC destination address and contain exactly 4 0x00
 // bytes in their payload. All other packets are considered to originate from
diff --git a/go/lib/infra/messenger/addr.go b/go/lib/infra/messenger/addr.go
index 59d15b753..da45f2966 100644
--- a/go/lib/infra/messenger/addr.go
+++ b/go/lib/infra/messenger/addr.go
@@ -16,7 +16,6 @@ package messenger
 
 import (
 	"context"
-	"fmt"
 	"net"
 	"time"
 
@@ -223,31 +222,6 @@ func parseReply(reply *svc.Reply) (*addr.AppAddr, error) {
 	}, nil
 }
 
-// BuildReply constructs a reply from an application address. If the
-// application address is not well formed (has L3, has L4, UDP/IP protocols),
-// the returned reply is non-nil and empty.
-func BuildReply(address *addr.AppAddr) *svc.Reply {
-	if address == nil || address.L3 == nil {
-		return &svc.Reply{}
-	}
-	port := fmt.Sprintf("%v", address.L4)
-
-	var ip string
-	switch t := address.L3.(type) {
-	case addr.HostIPv4:
-		ip = t.String()
-	case addr.HostIPv6:
-		ip = t.String()
-	default:
-		return &svc.Reply{}
-	}
-	return &svc.Reply{
-		Transports: map[svc.Transport]string{
-			svc.UDP: net.JoinHostPort(ip, port),
-		},
-	}
-}
-
 // LocalSVCRouter is used to construct overlay information for SVC servers
 // running in the local AS.
 type LocalSVCRouter interface {
diff --git a/go/lib/infra/modules/itopo/topology.go b/go/lib/infra/modules/itopo/topology.go
index 111c40756..abdc0c490 100644
--- a/go/lib/infra/modules/itopo/topology.go
+++ b/go/lib/infra/modules/itopo/topology.go
@@ -39,14 +39,7 @@ type Topology interface {
 
 	// PublicAddress gets the public address of a server with the requested type and name, and nil
 	// if no such server exists.
-	//
-	// FIXME(scrye): See whether this or its snet variant below can be removed.
-	PublicAddress(svc addr.HostSVC, name string) *addr.AppAddr
-	// SPublicAddress gets the public address of a server with the requested type and name, and nil
-	// if no such server exists.
-	//
-	// FIXME(scrye): See whether this or its app variant above can be removed.
-	SPublicAddress(svc addr.HostSVC, name string) *snet.Addr
+	PublicAddress(svc addr.HostSVC, name string) *net.UDPAddr
 
 	// Exists returns true if the service and name are present in the topology file.
 	Exists(svc addr.HostSVC, name string) bool
@@ -225,27 +218,16 @@ func (t *topologyS) BR(name string) (topology.BRInfo, bool) {
 	return br, ok
 }
 
-func (t *topologyS) SPublicAddress(svc addr.HostSVC, name string) *snet.Addr {
-	address := t.PublicAddress(svc, name)
-	if address == nil {
-		return nil
-	}
-	return &snet.Addr{
-		IA:   t.IA(),
-		Host: address.Copy(),
-	}
-}
-
-func (t *topologyS) PublicAddress(svc addr.HostSVC, name string) *addr.AppAddr {
+func (t *topologyS) PublicAddress(svc addr.HostSVC, name string) *net.UDPAddr {
 	topoAddr := t.topoAddress(svc, name)
 	if topoAddr == nil {
 		return nil
 	}
-	publicAddr := topoAddr.SCIONAddress
-	if publicAddr == nil {
+	pa := topoAddr.SCIONAddress
+	if pa == nil {
 		return nil
 	}
-	return publicAddr.Copy()
+	return &net.UDPAddr{IP: pa.L3.IP(), Port: int(pa.L4)}
 }
 
 func (t *topologyS) Exists(svc addr.HostSVC, name string) bool {
diff --git a/go/lib/sciond/mock_sciond/BUILD.bazel b/go/lib/sciond/mock_sciond/BUILD.bazel
index 35ac0eb93..59ab63cd0 100644
--- a/go/lib/sciond/mock_sciond/BUILD.bazel
+++ b/go/lib/sciond/mock_sciond/BUILD.bazel
@@ -7,7 +7,6 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//go/lib/addr:go_default_library",
-        "//go/lib/common:go_default_library",
         "//go/lib/ctrl/path_mgmt:go_default_library",
         "//go/lib/sciond:go_default_library",
         "//go/lib/snet:go_default_library",
diff --git a/go/lib/sciond/mock_sciond/sciond.go b/go/lib/sciond/mock_sciond/sciond.go
index 7c9704e14..67545f3d2 100644
--- a/go/lib/sciond/mock_sciond/sciond.go
+++ b/go/lib/sciond/mock_sciond/sciond.go
@@ -8,7 +8,6 @@ import (
 	context "context"
 	gomock "github.com/golang/mock/gomock"
 	addr "github.com/scionproto/scion/go/lib/addr"
-	common "github.com/scionproto/scion/go/lib/common"
 	path_mgmt "github.com/scionproto/scion/go/lib/ctrl/path_mgmt"
 	sciond "github.com/scionproto/scion/go/lib/sciond"
 	snet "github.com/scionproto/scion/go/lib/snet"
@@ -106,21 +105,6 @@ func (mr *MockConnectorMockRecorder) Close(arg0 interface{}) *gomock.Call {
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockConnector)(nil).Close), arg0)
 }
 
-// IFInfo mocks base method
-func (m *MockConnector) IFInfo(arg0 context.Context, arg1 []common.IFIDType) (*sciond.IFInfoReply, error) {
-	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "IFInfo", arg0, arg1)
-	ret0, _ := ret[0].(*sciond.IFInfoReply)
-	ret1, _ := ret[1].(error)
-	return ret0, ret1
-}
-
-// IFInfo indicates an expected call of IFInfo
-func (mr *MockConnectorMockRecorder) IFInfo(arg0, arg1 interface{}) *gomock.Call {
-	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IFInfo", reflect.TypeOf((*MockConnector)(nil).IFInfo), arg0, arg1)
-}
-
 // Paths mocks base method
 func (m *MockConnector) Paths(arg0 context.Context, arg1, arg2 addr.IA, arg3 uint16, arg4 sciond.PathReqFlags) ([]snet.Path, error) {
 	m.ctrl.T.Helper()
diff --git a/go/lib/snet/addr.go b/go/lib/snet/addr.go
index 05f7e6277..2b0a46187 100644
--- a/go/lib/snet/addr.go
+++ b/go/lib/snet/addr.go
@@ -39,6 +39,15 @@ type Addr struct {
 	NextHop *net.UDPAddr
 }
 
+// ToNetUDPAddr returns a net.UDPAddr or nil.
+func (a *Addr) ToNetUDPAddr() *net.UDPAddr {
+	switch a.Host.L3.Type() {
+	case addr.HostTypeIPv4, addr.HostTypeIPv6:
+		return &net.UDPAddr{IP: a.Host.L3.IP(), Port: int(a.Host.L4)}
+	}
+	return nil
+}
+
 func (a *Addr) Network() string {
 	return "scion"
 }
diff --git a/go/lib/snet/dispatcher.go b/go/lib/snet/dispatcher.go
index 557d59b58..3a4c45bc5 100644
--- a/go/lib/snet/dispatcher.go
+++ b/go/lib/snet/dispatcher.go
@@ -56,7 +56,7 @@ func (s *DefaultPacketDispatcherService) RegisterTimeout(ia addr.IA, public *add
 	if err != nil {
 		return nil, 0, err
 	}
-	return &SCIONPacketConn{conn: rconn, scmpHandler: s.SCMPHandler}, port, err
+	return &SCIONPacketConn{conn: rconn, scmpHandler: s.SCMPHandler}, port, nil
 }
 
 // RevocationHandler is called by the default SCMP Handler whenever revocations are encountered.
diff --git a/go/path_srv/main.go b/go/path_srv/main.go
index 6c92d7e2b..6c0d299f9 100644
--- a/go/path_srv/main.go
+++ b/go/path_srv/main.go
@@ -124,7 +124,7 @@ func realMain() int {
 	opentracing.SetGlobalTracer(tracer)
 	nc := infraenv.NetworkConfig{
 		IA:                    topo.IA(),
-		Public:                topo.SPublicAddress(addr.SvcPS, cfg.General.ID),
+		Public:                topo.PublicAddress(addr.SvcPS, cfg.General.ID),
 		SVC:                   addr.SvcPS,
 		ReconnectToDispatcher: cfg.General.ReconnectToDispatcher,
 		QUIC: infraenv.QUIC{
diff --git a/go/sciond/main.go b/go/sciond/main.go
index 75329cbcb..6ea95c92d 100644
--- a/go/sciond/main.go
+++ b/go/sciond/main.go
@@ -18,6 +18,7 @@ import (
 	"context"
 	"flag"
 	"fmt"
+	"net"
 	_ "net/http/pprof"
 	"os"
 	"path/filepath"
@@ -115,10 +116,15 @@ func realMain() int {
 	}
 	defer trCloser.Close()
 	opentracing.SetGlobalTracer(tracer)
+
+	var publicIP *net.UDPAddr
+	if p := cfg.SD.Public; p != nil {
+		publicIP = p.ToNetUDPAddr()
+	}
+
 	nc := infraenv.NetworkConfig{
 		IA:                    itopo.Get().IA(),
-		Public:                cfg.SD.Public,
-		Bind:                  cfg.SD.Bind,
+		Public:                publicIP,
 		SVC:                   addr.SvcNone,
 		ReconnectToDispatcher: cfg.General.ReconnectToDispatcher,
 		QUIC: infraenv.QUIC{


Test patch:
diff --git a/go/lib/infra/messenger/addr_test.go b/go/lib/infra/messenger/addr_test.go
index b8f1068e1..b587b603a 100644
--- a/go/lib/infra/messenger/addr_test.go
+++ b/go/lib/infra/messenger/addr_test.go
@@ -415,48 +415,6 @@ func TestParseReply(t *testing.T) {
 	}
 }
 
-func TestBuildReply(t *testing.T) {
-	testCases := map[string]struct {
-		input *addr.AppAddr
-		want  *svc.Reply
-	}{
-		"nil app address": {
-			want: &svc.Reply{},
-		},
-		"nil L3": {
-			input: &addr.AppAddr{L4: 1},
-			want:  &svc.Reply{},
-		},
-		"nil L4": {
-			input: newSVCAppAddr(addr.SvcBS),
-			want:  &svc.Reply{},
-		},
-		"IPv4 L3, UDP L4": {
-			input: newUDPAppAddr(&net.UDPAddr{IP: net.IP{192, 168, 0, 1}, Port: 1}),
-			want: &svc.Reply{
-				Transports: map[svc.Transport]string{
-					svc.UDP: "192.168.0.1:1",
-				},
-			},
-		},
-		"IPv6 L3, UDP L4": {
-			input: newUDPAppAddr(&net.UDPAddr{IP: net.ParseIP("2001:db8::1"), Port: 1}),
-			want: &svc.Reply{
-				Transports: map[svc.Transport]string{
-					svc.UDP: "[2001:db8::1]:1",
-				},
-			},
-		},
-	}
-
-	for tn, tc := range testCases {
-		t.Run(tn, func(t *testing.T) {
-			got := messenger.BuildReply(tc.input)
-			assert.Equal(t, got, tc.want)
-		})
-	}
-}
-
 func initResolver(resolver *mock_messenger.MockResolver, f func(*mock_messenger.MockResolver)) {
 	if f != nil {
 		f(resolver)