Repository: scionproto/scion
Pull request #3421 (2019-11-22 12:15:28)

messenger: refactor to avoid using addr.AppAddr


<!-- 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/3421)
<!-- Reviewable:end -->



Patch:
diff --git a/go/lib/infra/messenger/addr.go b/go/lib/infra/messenger/addr.go
index da45f2966..9627798b5 100644
--- a/go/lib/infra/messenger/addr.go
+++ b/go/lib/infra/messenger/addr.go
@@ -70,28 +70,47 @@ type AddressRewriter struct {
 //
 // If the address is already unicast, no redirection to QUIC is attempted.
 func (r AddressRewriter) RedirectToQUIC(ctx context.Context, a net.Addr) (net.Addr, bool, error) {
+
 	// FIXME(scrye): This is not legitimate use. It's only included for
 	// compatibility with older unit tests. See
 	// https://github.com/scionproto/scion/issues/2611.
-	if a == nil {
-		return nil, false, nil
+	if a == nil || r.SVCResolutionFraction <= 0.0 {
+		return a, false, nil
+	}
+
+	// If already of type UDPAddr then return
+	if v, ok := a.(*snet.Addr); ok && v.Host.L3.Type() != addr.HostTypeSVC {
+		return a, false, nil
 	}
-	fullAddress, err := r.buildFullAddress(ctx, a)
+
+	t, err := r.buildFullAddress(ctx, a)
 	if err != nil {
 		return nil, false, err
 	}
+
+	fullAddress, ok := t.(*snet.Addr)
+	if !ok {
+		return nil, false, common.NewBasicError("address type not supported", nil, "addr", a)
+	}
 	path, err := fullAddress.GetPath()
 	if err != nil {
 		return nil, false, common.NewBasicError("bad path", err)
 	}
-	var quicRedirect bool
-	var p snet.Path
-	// During One-Hop Path operation, use SVC resolution to also bootstrap the
-	// path.
-	p, fullAddress.Host, quicRedirect, err = r.resolveIfSVC(ctx, path, fullAddress.Host)
+
+	v, ok := fullAddress.Host.L3.(addr.HostSVC)
+	if !ok { //if not SVC
+		return fullAddress, false, nil
+	}
+
+	// During One-Hop Path operation, use SVC resolution to also bootstrap the path.
+	p, u, quicRedirect, err := r.resolveSVC(ctx, path, v)
 	if p != nil {
 		fullAddress.Path = p.Path()
 	}
+	if u != nil {
+		fullAddress.Host = addr.AppAddrFromUDP(u)
+	}
+
 	return fullAddress, quicRedirect, err
 }
 
@@ -99,7 +118,7 @@ func (r AddressRewriter) RedirectToQUIC(ctx context.Context, a net.Addr) (net.Ad
 // non-nil, only supported protocols). If the path is missing, the path and
 // next-hop are added by performing a routing lookup. The returned address is
 // always a copy, and the input address is guaranteed to not change.
-func (r AddressRewriter) buildFullAddress(ctx context.Context, a net.Addr) (*snet.Addr, error) {
+func (r AddressRewriter) buildFullAddress(ctx context.Context, a net.Addr) (net.Addr, error) {
 	snetAddr, ok := a.(*snet.Addr)
 	if !ok {
 		return nil, common.NewBasicError("address type not supported", nil, "addr", a)
@@ -138,54 +157,46 @@ func (r AddressRewriter) buildFullAddress(ctx context.Context, a net.Addr) (*sne
 	return newAddr, nil
 }
 
-// resolveIfSvc performs SVC resolution and returns an UDP/IP address if the
-// input address is an SVC destination. If the UDP/IP address is for a
-// QUIC-compatible server, the returned boolean value is set to true. If the
-// address does not have an SVC destination, it is returned unchanged. If
-// address is not a well-formed application address (all fields set, non-nil,
-// supported protocols), the function's behavior is undefined. The returned
-// address is always a copy. The returned path is the path contained in the
-// reply; the path can be used to talk to the remote AS after One-Hop Path
-// construction.
-func (r AddressRewriter) resolveIfSVC(ctx context.Context, p snet.Path,
-	address *addr.AppAddr) (snet.Path, *addr.AppAddr, bool, error) {
-
-	svcAddress, ok := address.L3.(addr.HostSVC)
-	if !ok {
-		return nil, address.Copy(), false, nil
-	}
-	if r.SVCResolutionFraction <= 0.0 {
-		return nil, address.Copy(), false, nil
-	}
-
+// resolveSVC performs SVC resolution and returns an UDP/IP address. If the UDP/IP
+// address is for a QUIC-compatible server, the returned boolean value is set
+// to true. If the address does not have an SVC destination, it is returned
+// unchanged. If address is not a well-formed application address (all fields
+// set, non-nil, supported protocols), the function's behavior is undefined.
+// The returned path is the path contained in the reply; the path can be used
+// to talk to the remote AS after One-Hop Path construction.
+func (r AddressRewriter) resolveSVC(ctx context.Context, p snet.Path,
+	s addr.HostSVC) (snet.Path, *net.UDPAddr, bool, error) {
+	logger := log.FromCtx(ctx)
 	if r.SVCResolutionFraction < 1.0 {
 		var cancelF context.CancelFunc
 		ctx, cancelF = r.resolutionCtx(ctx)
 		defer cancelF()
 	}
-	logger := log.FromCtx(ctx)
-	logger.Trace("Sending SVC resolution request", "ia", p.Destination(), "svc", svcAddress,
+
+	logger.Trace("Sending SVC resolution request", "ia", p.Destination(), "svc", s,
 		"svcResFraction", r.SVCResolutionFraction)
-	reply, err := r.Resolver.LookupSVC(ctx, p, svcAddress)
+
+	reply, err := r.Resolver.LookupSVC(ctx, p, s)
 	if err != nil {
+		logger.Trace("SVC resolution failed", "err", err)
 		if r.SVCResolutionFraction < 1.0 {
 			// SVC resolution failed but we allow legacy behavior and have some
 			// fraction of the timeout left for data transfers, so return
 			// address with SVC destination still set
-			logger.Trace("SVC resolution failed, falling back to legacy mode", "err", err)
-			return nil, address.Copy(), false, nil
+			logger.Trace("Falling back to legacy mode, ignore error", "err", err)
+			return nil, nil, false, nil
 		}
 		// Legacy behavior is disallowed, so propagate a hard failure back to the app.
-		logger.Trace("SVC resolution failed and legacy mode disabled", "err", err)
+		logger.Trace("Legacy mode disabled, propagate error", "err", err)
 		return nil, nil, false, err
 	}
-	logger.Trace("SVC resolution successful", "reply", reply)
 
-	appAddr, err := parseReply(reply)
+	logger.Trace("SVC resolution successful", "reply", reply)
+	u, err := parseReply(reply)
 	if err != nil {
 		return nil, nil, false, err
 	}
-	return reply.ReturnPath, appAddr, true, nil
+	return reply.ReturnPath, u, true, nil
 }
 
 func (r AddressRewriter) resolutionCtx(ctx context.Context) (context.Context, context.CancelFunc) {
@@ -201,7 +212,7 @@ func (r AddressRewriter) resolutionCtx(ctx context.Context) (context.Context, co
 
 // parseReply searches for a QUIC server on the remote address. If one is not
 // found, an error is returned.
-func parseReply(reply *svc.Reply) (*addr.AppAddr, error) {
+func parseReply(reply *svc.Reply) (*net.UDPAddr, error) {
 	if reply == nil {
 		return nil, serrors.New("nil reply")
 	}
@@ -212,14 +223,7 @@ func parseReply(reply *svc.Reply) (*addr.AppAddr, error) {
 	if !ok {
 		return nil, serrors.New("QUIC server address not found")
 	}
-	udpAddr, err := net.ResolveUDPAddr("udp", addressStr)
-	if err != nil {
-		return nil, common.NewBasicError("Unable to parse address", err)
-	}
-	return &addr.AppAddr{
-		L3: addr.HostFromIP(udpAddr.IP),
-		L4: uint16(udpAddr.Port),
-	}, nil
+	return net.ResolveUDPAddr("udp", addressStr)
 }
 
 // LocalSVCRouter is used to construct overlay information for SVC servers


Test patch:
diff --git a/go/lib/infra/messenger/addr_test.go b/go/lib/infra/messenger/addr_test.go
index b587b603a..c54542a22 100644
--- a/go/lib/infra/messenger/addr_test.go
+++ b/go/lib/infra/messenger/addr_test.go
@@ -37,16 +37,47 @@ import (
 
 func TestRedirectQUIC(t *testing.T) {
 	testCases := map[string]struct {
-		input        net.Addr
-		wantAddr     net.Addr
-		wantRedirect bool
-		assertErr    assert.ErrorAssertionFunc
+		input                 net.Addr
+		wantAddr              net.Addr
+		wantRedirect          bool
+		SVCResolutionFraction float64
+		assertErr             assert.ErrorAssertionFunc
 	}{
-		"nil input": {
-			input:        nil,
-			wantAddr:     nil,
-			wantRedirect: false,
-			assertErr:    assert.NoError,
+		"nil addr, no error": {
+			input:     nil,
+			wantAddr:  nil,
+			assertErr: assert.NoError,
+		},
+		"disabling SVC resolution, returns unchanged": {
+			input:                 &snet.Addr{Host: newSVCAppAddr(addr.SvcBS)},
+			wantAddr:              &snet.Addr{Host: newSVCAppAddr(addr.SvcBS)},
+			SVCResolutionFraction: 0.0,
+			assertErr:             assert.NoError,
+		},
+		"not nil invalid addr, error": {
+			input:                 &net.TCPAddr{},
+			wantAddr:              nil,
+			wantRedirect:          false,
+			SVCResolutionFraction: 1.0,
+			assertErr:             assert.Error,
+		},
+		"valid empty UDP, ?error": {
+			input:                 newUDPAppAddr(&net.UDPAddr{}),
+			wantAddr:              nil,
+			wantRedirect:          false,
+			SVCResolutionFraction: 1.0,
+			assertErr:             assert.Error,
+		},
+		"valid UDPAddr, returns unchanged": {
+			input: &snet.Addr{
+				Host: newUDPAppAddr(&net.UDPAddr{IP: net.IP{192, 168, 0, 1}, Port: 1}),
+			},
+			wantAddr: &snet.Addr{
+				Host: newUDPAppAddr(&net.UDPAddr{IP: net.IP{192, 168, 0, 1}, Port: 1}),
+			},
+			SVCResolutionFraction: 1.0,
+			wantRedirect:          false,
+			assertErr:             assert.NoError,
 		},
 	}
 	for tn, tc := range testCases {
@@ -54,10 +85,14 @@ func TestRedirectQUIC(t *testing.T) {
 			ctrl := gomock.NewController(t)
 			defer ctrl.Finish()
 			router := mock_snet.NewMockRouter(ctrl)
+			router.EXPECT().Route(gomock.Any(), gomock.Any()).Times(0)
 			resolver := mock_messenger.NewMockResolver(ctrl)
+			resolver.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+
 			aw := messenger.AddressRewriter{
-				Resolver: resolver,
-				Router:   router,
+				Resolver:              resolver,
+				Router:                router,
+				SVCResolutionFraction: tc.SVCResolutionFraction,
 			}
 
 			a, r, err := aw.RedirectToQUIC(context.Background(), tc.input)
@@ -71,7 +106,7 @@ func TestRedirectQUIC(t *testing.T) {
 func TestBuildFullAddress(t *testing.T) {
 	testCases := map[string]struct {
 		input     net.Addr
-		want      *snet.Addr
+		want      net.Addr
 		assertErr assert.ErrorAssertionFunc
 	}{
 		"non-snet address": {
@@ -251,32 +286,18 @@ func TestBuildFullAddress(t *testing.T) {
 	})
 }
 
-func TestResolveIfSVC(t *testing.T) {
+func TestResolve(t *testing.T) {
 	testCases := map[string]struct {
-		input                 *addr.AppAddr
+		input                 addr.HostSVC
 		ResolverSetup         func(*mock_messenger.MockResolver)
 		SVCResolutionFraction float64
 		wantPath              snet.Path
-		want                  *addr.AppAddr
+		want                  *net.UDPAddr
 		wantQUICRedirect      bool
 		assertErr             assert.ErrorAssertionFunc
 	}{
-		"non-svc address does not trigger lookup": {
-			input: newUDPAppAddr(
-				&net.UDPAddr{IP: net.IP{192, 168, 0, 1}, Port: 1}),
-			SVCResolutionFraction: 1.0,
-			want: newUDPAppAddr(&net.UDPAddr{
-				IP: net.IP{192, 168, 0, 1}, Port: 1}),
-			assertErr: assert.NoError,
-		},
-		"disabling SVC resolution does not trigger lookup, same addr": {
-			input:                 newSVCAppAddr(addr.SvcBS),
-			SVCResolutionFraction: 0.0,
-			want:                  newSVCAppAddr(addr.SvcBS),
-			assertErr:             assert.NoError,
-		},
 		"svc address, lookup fails": {
-			input: newSVCAppAddr(addr.SvcBS),
+			input: addr.SvcBS,
 			ResolverSetup: func(r *mock_messenger.MockResolver) {
 				r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()).
 					Return(nil, fmt.Errorf("err"))
@@ -285,17 +306,17 @@ func TestResolveIfSVC(t *testing.T) {
 			assertErr:             assert.Error,
 		},
 		"svc address, half time allowed for resolution, lookup fails": {
-			input: newSVCAppAddr(addr.SvcBS),
+			input: addr.SvcBS,
 			ResolverSetup: func(r *mock_messenger.MockResolver) {
 				r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()).
 					Return(nil, fmt.Errorf("err"))
 			},
 			SVCResolutionFraction: 0.5,
-			want:                  newSVCAppAddr(addr.SvcBS),
+			want:                  nil,
 			assertErr:             assert.NoError,
 		},
 		"svc address, lookup succeeds": {
-			input: newSVCAppAddr(addr.SvcBS),
+			input: addr.SvcBS,
 			ResolverSetup: func(r *mock_messenger.MockResolver) {
 				r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()).
 					Return(
@@ -310,13 +331,12 @@ func TestResolveIfSVC(t *testing.T) {
 			},
 			SVCResolutionFraction: 1.0,
 			wantPath:              &testPath{},
-			want: newUDPAppAddr(
-				&net.UDPAddr{IP: net.IP{192, 168, 1, 1}, Port: 8000}),
-			wantQUICRedirect: true,
-			assertErr:        assert.NoError,
+			want:                  &net.UDPAddr{IP: net.IP{192, 168, 1, 1}, Port: 8000},
+			wantQUICRedirect:      true,
+			assertErr:             assert.NoError,
 		},
 		"svc address, half time allowed for resolution, lookup succeeds": {
-			input: newSVCAppAddr(addr.SvcBS),
+			input: addr.SvcBS,
 			ResolverSetup: func(r *mock_messenger.MockResolver) {
 				r.EXPECT().LookupSVC(gomock.Any(), gomock.Any(), gomock.Any()).
 					Return(
@@ -329,10 +349,9 @@ func TestResolveIfSVC(t *testing.T) {
 					)
 			},
 			SVCResolutionFraction: 0.5,
-			want: newUDPAppAddr(
-				&net.UDPAddr{IP: net.IP{192, 168, 1, 1}, Port: 8000}),
-			wantQUICRedirect: true,
-			assertErr:        assert.NoError,
+			want:                  &net.UDPAddr{IP: net.ParseIP("192.168.1.1"), Port: 8000},
+			wantQUICRedirect:      true,
+			assertErr:             assert.NoError,
 		},
 	}
 
@@ -348,9 +367,9 @@ func TestResolveIfSVC(t *testing.T) {
 				SVCResolutionFraction: tc.SVCResolutionFraction,
 			}
 			initResolver(resolver, tc.ResolverSetup)
-			p, a, redirect, err := aw.ResolveIfSVC(context.Background(), path, tc.input)
+			p, a, redirect, err := aw.ResolveSVC(context.Background(), path, tc.input)
 			assert.Equal(t, p, tc.wantPath)
-			assert.Equal(t, a, tc.want)
+			assert.Equal(t, a.String(), tc.want.String())
 			assert.Equal(t, redirect, tc.wantQUICRedirect)
 			tc.assertErr(t, err)
 		})
@@ -359,7 +378,7 @@ func TestResolveIfSVC(t *testing.T) {
 
 func TestParseReply(t *testing.T) {
 	testCases := map[string]struct {
-		Reply     *svc.Reply
+		mockReply *svc.Reply
 		want      *net.UDPAddr
 		assertErr assert.ErrorAssertionFunc
 	}{
@@ -367,15 +386,15 @@ func TestParseReply(t *testing.T) {
 			assertErr: assert.Error,
 		},
 		"empty reply": {
-			Reply:     &svc.Reply{},
+			mockReply: &svc.Reply{},
 			assertErr: assert.Error,
 		},
 		"key not found in reply": {
-			Reply:     &svc.Reply{Transports: map[svc.Transport]string{svc.UDP: "foo"}},
+			mockReply: &svc.Reply{Transports: map[svc.Transport]string{svc.UDP: "foo"}},
 			assertErr: assert.Error,
 		},
 		"key found in reply, but parsing fails": {
-			Reply: &svc.Reply{
+			mockReply: &svc.Reply{
 				Transports: map[svc.Transport]string{
 					svc.QUIC: "foo",
 				},
@@ -383,7 +402,7 @@ func TestParseReply(t *testing.T) {
 			assertErr: assert.Error,
 		},
 		"key found in reply, IPv4 address": {
-			Reply: &svc.Reply{
+			mockReply: &svc.Reply{
 				Transports: map[svc.Transport]string{
 					svc.QUIC: "192.168.1.1:8000",
 				},
@@ -392,7 +411,7 @@ func TestParseReply(t *testing.T) {
 			assertErr: assert.NoError,
 		},
 		"key found in reply, IPv6 address": {
-			Reply: &svc.Reply{
+			mockReply: &svc.Reply{
 				Transports: map[svc.Transport]string{
 					svc.QUIC: "[2001:db8::1]:8000",
 				},
@@ -404,13 +423,12 @@ func TestParseReply(t *testing.T) {
 
 	for tn, tc := range testCases {
 		t.Run(tn, func(t *testing.T) {
-			a, err := messenger.ParseReply(tc.Reply)
+			a, err := messenger.ParseReply(tc.mockReply)
 			tc.assertErr(t, err)
 			if err != nil {
 				return
 			}
-			want := newUDPAppAddr(tc.want)
-			assert.Equal(t, a, want)
+			assert.Equal(t, a.String(), tc.want.String())
 		})
 	}
 }
diff --git a/go/lib/infra/messenger/export_test.go b/go/lib/infra/messenger/export_test.go
index bfdd4b277..ececfa239 100644
--- a/go/lib/infra/messenger/export_test.go
+++ b/go/lib/infra/messenger/export_test.go
@@ -9,15 +9,15 @@ import (
 	"github.com/scionproto/scion/go/lib/svc"
 )
 
-func (r AddressRewriter) BuildFullAddress(ctx context.Context, a net.Addr) (*snet.Addr, error) {
+func (r AddressRewriter) BuildFullAddress(ctx context.Context, a net.Addr) (net.Addr, error) {
 	return r.buildFullAddress(ctx, a)
 }
 
-func (r AddressRewriter) ResolveIfSVC(ctx context.Context, p snet.Path,
-	a *addr.AppAddr) (snet.Path, *addr.AppAddr, bool, error) {
-	return r.resolveIfSVC(ctx, p, a)
+func (r AddressRewriter) ResolveSVC(ctx context.Context, p snet.Path,
+	s addr.HostSVC) (snet.Path, *net.UDPAddr, bool, error) {
+	return r.resolveSVC(ctx, p, s)
 }
 
-func ParseReply(reply *svc.Reply) (*addr.AppAddr, error) {
+func ParseReply(reply *svc.Reply) (*net.UDPAddr, error) {
 	return parseReply(reply)
 }